From: Stephen Boyd <sboyd@kernel.org>
To: Tomer Maimon <tmaimon77@gmail.com>
Cc: benjaminfair@google.com, joel@jms.id.au,
krzysztof.kozlowski+dt@linaro.org, mturquette@baylibre.com,
robh+dt@kernel.org, tali.perry1@gmail.com, venture@google.com,
yuenn@google.com, openbmc@lists.ozlabs.org,
linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller
Date: Wed, 28 Feb 2024 14:47:58 -0800 [thread overview]
Message-ID: <8acf846e767884978f3bb98646433551.sboyd@kernel.org> (raw)
In-Reply-To: <CAP6Zq1g5gwXvYzO5fnHxG-6__gSCpNBY7VeEPyr4Qtijya6EfQ@mail.gmail.com>
Quoting Tomer Maimon (2024-02-25 10:00:35)
> Hi Stephen,
>
> On Thu, 22 Feb 2024 at 07:58, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2024-01-31 10:26:53)
> > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > new file mode 100644
> > > index 000000000000..eacb579d30af
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-npcm8xx.c
> > > @@ -0,0 +1,509 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Nuvoton NPCM8xx Clock Generator
> > > + * All the clocks are initialized by the bootloader, so this driver allows only
> > [...]
> > > +
> > > +/* external clock definition */
> > > +#define NPCM8XX_CLK_S_REFCLK "refclk"
> > > +
> > > +/* pll definition */
> > > +#define NPCM8XX_CLK_S_PLL0 "pll0"
> > > +#define NPCM8XX_CLK_S_PLL1 "pll1"
> > > +#define NPCM8XX_CLK_S_PLL2 "pll2"
> > > +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx"
> > > +
> > > +/* early divider definition */
> > > +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2"
> > > +#define NPCM8XX_CLK_S_PLL_GFX_DIV2 "pll_gfx_div2"
> > > +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2"
> > > +
> > > +/* mux definition */
> > > +#define NPCM8XX_CLK_S_CPU_MUX "cpu_mux"
> > > +
> > > +/* div definition */
> > > +#define NPCM8XX_CLK_S_TH "th"
> > > +#define NPCM8XX_CLK_S_AXI "axi"
> >
> > Please inline all these string #defines to the place they're used.
> The version V21 you mention using define only when the definition is
> used more than once
> https://www.spinics.net/lists/kernel/msg5045826.html
> Should I remove all the string definitions and add the string to the array?
If it's a clk name for a clk registered in this file it should be
inlined. Is that the case for everything besides refclk? And even refclk
could be inlined so that we don't have to jump to the definition of a
string.
> > > +
> > > +static unsigned long npcm8xx_clk_div_get_parent(struct clk_hw *hw,
> > > + unsigned long parent_rate)
> > > +{
> > > + struct npcm8xx_clk *div = to_npcm8xx_clk(hw);
> > > + unsigned int val;
> > > +
> > > + regmap_read(div->clk_regmap, div->offset, &val);
> > > + val = val >> div->shift;
> > > + val &= clk_div_mask(div->width);
> > > +
> > > + return divider_recalc_rate(hw, parent_rate, val, NULL, div->flags,
> > > + div->width);
> > > +}
> > > +
> > > +static const struct clk_ops npcm8xx_clk_div_ops = {
> > > + .recalc_rate = npcm8xx_clk_div_get_parent,
> > > +};
> > > +
> > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > +{
> > > + struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> >
> > The parent of this device is not a syscon.
> Once I have registered the map that handles both reset and the clock
> in general is syscon, this is why we will modify the DTS so the clock
> and the reset will be under syscon father node
> sysctrl: system-controller@f0801000 {
> compatible = "syscon", "simple-mfd";
> reg = <0x0 0xf0801000 0x0 0x1000>;
>
> rstc: reset-controller {
> compatible = "nuvoton,npcm845-reset";
> reg = <0x0 0xf0801000 0x0 0xC4>;
> #reset-cells = <2>;
> nuvoton,sysgcr = <&gcr>;
> };
>
> clk: clock-controller {
> compatible = "nuvoton,npcm845-clk";
> #clock-cells = <1>;
> clocks = <&refclk>;
> clock-names = "refclk";
> };
> };
> You can see other drivers that using the same method like
> https://elixir.bootlin.com/linux/v6.8-rc5/source/Documentation/devicetree/bindings/clock/socionext,uniphier-clock.yaml
You will need a similar file like
Documentation/devicetree/bindings/soc/socionext/socionext,uniphier-perictrl.yaml
then to describe the child nodes.
Socionext may not be the best example to follow. I generally try to
avoid syscon and simply put #reset-cells and #clock-cells in the node
for the device. You can use the auxiliary bus to register drivers for
clk and reset and put them into the resepective driver directories.
Avoid syscon means random drivers can't reach into the device with a
regmap handle and read/write registers that they're not supposed to.
> >
> > > + struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > + struct device *dev = &pdev->dev;
> > > + struct regmap *clk_regmap;
> > > + struct clk_hw *hw;
> > > + unsigned int i;
> > > +
> > > + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > > + NPCM8XX_NUM_CLOCKS),
> > > + GFP_KERNEL);
> > > + if (!npcm8xx_clk_data)
> > > + return -ENOMEM;
> > > +
> > > + clk_regmap = syscon_node_to_regmap(parent_np);
> > > + of_node_put(parent_np);
> >
> > Is there another binding update that is going to move this node to be a
> > child of the syscon?
> >
> > gcr: system-controller@f0800000 {
> > compatible = "nuvoton,npcm845-gcr", "syscon";
> > reg = <0x0 0xf0800000 0x0 0x1000>;
> > };
> No, sorry but I'm not going to use the GCR node the handle the clock
> and reset modules, the GCR has different memory space.
> the clock driver will have the following device tree
What does the reset driver use the CGR node for? The driver looks like
it's using it to control USB phy resets.
> sysctrl: system-controller@f0801000 {
> compatible = "syscon", "simple-mfd";
> reg = <0x0 0xf0801000 0x0 0x1000>;
>
> rstc: reset-controller {
> compatible = "nuvoton,npcm845-reset";
> reg = <0x0 0xf0801000 0x0 0xC4>;
This isn't a valid reg property for a child node like this.
> #reset-cells = <2>;
> nuvoton,sysgcr = <&gcr>;
> };
>
> clk: clock-controller {
> compatible = "nuvoton,npcm845-clk";
> #clock-cells = <1>;
> clocks = <&refclk>;
> clock-names = "refclk";
> };
> };
next prev parent reply other threads:[~2024-02-28 22:48 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-31 18:26 [PATCH v23 0/3] Introduce Nuvoton Arbel NPCM8XX BMC SoC Tomer Maimon
2024-01-31 18:26 ` [PATCH v23 1/3] dt-bindings: clock: npcm845: Add reference 25m clock property Tomer Maimon
2024-02-01 8:45 ` Krzysztof Kozlowski
2024-02-22 5:58 ` Stephen Boyd
2024-02-25 18:06 ` Tomer Maimon
2024-01-31 18:26 ` [PATCH v23 2/3] arm64: dts: nuvoton: npcm8xx: add reference 25m clock Tomer Maimon
2024-01-31 18:26 ` [PATCH v23 3/3] clk: npcm8xx: add clock controller Tomer Maimon
2024-02-22 5:58 ` Stephen Boyd
2024-02-25 18:00 ` Tomer Maimon
2024-02-28 22:47 ` Stephen Boyd [this message]
2024-02-29 21:29 ` Tomer Maimon
2024-03-05 15:59 ` Tomer Maimon
2024-04-02 19:42 ` Tomer Maimon
2024-04-11 4:51 ` Stephen Boyd
2024-02-01 8:42 ` [PATCH v23 0/3] Introduce Nuvoton Arbel NPCM8XX BMC SoC Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8acf846e767884978f3bb98646433551.sboyd@kernel.org \
--to=sboyd@kernel.org \
--cc=benjaminfair@google.com \
--cc=devicetree@vger.kernel.org \
--cc=joel@jms.id.au \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=openbmc@lists.ozlabs.org \
--cc=robh+dt@kernel.org \
--cc=tali.perry1@gmail.com \
--cc=tmaimon77@gmail.com \
--cc=venture@google.com \
--cc=yuenn@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).