Received-SPF: pass (google.com: domain of linux-kernel+bounces-85799-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8acf846e767884978f3bb98646433551.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAP6Zq1g5gwXvYzO5fnHxG-6__gSCpNBY7VeEPyr4Qtijya6EfQ@mail.gmail.com>
References: <20240131182653.2673554-1-tmaimon77@gmail.com> <20240131182653.2673554-4-tmaimon77@gmail.com> <74e003c6d80611ddd826ac21f48b4b3a.sboyd@kernel.org> <CAP6Zq1g5gwXvYzO5fnHxG-6__gSCpNBY7VeEPyr4Qtijya6EfQ@mail.gmail.com>
Subject: Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller
From: Stephen Boyd <sboyd@kernel.org>
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
To: Tomer Maimon <tmaimon77@gmail.com>
Date: Wed, 28 Feb 2024 14:47:58 -0800
User-Agent: alot/0.10

Quoting Tomer Maimon (2024-02-25 10:00:35)
> Hi Stephen,
>=20
> 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 arra=
y?

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 =3D to_npcm8xx_clk(hw);
> > > +       unsigned int val;
> > > +
> > > +       regmap_read(div->clk_regmap, div->offset, &val);
> > > +       val =3D val >> div->shift;
> > > +       val &=3D clk_div_mask(div->width);
> > > +
> > > +       return divider_recalc_rate(hw, parent_rate, val, NULL, div->f=
lags,
> > > +                                  div->width);
> > > +}
> > > +
> > > +static const struct clk_ops npcm8xx_clk_div_ops =3D {
> > > +       .recalc_rate =3D npcm8xx_clk_div_get_parent,
> > > +};
> > > +
> > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device_node *parent_np =3D 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 =3D "syscon", "simple-mfd";
>                         reg =3D <0x0 0xf0801000 0x0 0x1000>;
>=20
>                         rstc: reset-controller {
>                                 compatible =3D "nuvoton,npcm845-reset";
>                                 reg =3D <0x0 0xf0801000 0x0 0xC4>;
>                                 #reset-cells =3D <2>;
>                                 nuvoton,sysgcr =3D <&gcr>;
>                         };
>=20
>                         clk: clock-controller {
>                                 compatible =3D "nuvoton,npcm845-clk";
>                                 #clock-cells =3D <1>;
>                                 clocks =3D <&refclk>;
>                                 clock-names =3D "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 =3D &pdev->dev;
> > > +       struct regmap *clk_regmap;
> > > +       struct clk_hw *hw;
> > > +       unsigned int i;
> > > +
> > > +       npcm8xx_clk_data =3D devm_kzalloc(dev, struct_size(npcm8xx_cl=
k_data, hws,
> > > +                                                        NPCM8XX_NUM_=
CLOCKS),
> > > +                                       GFP_KERNEL);
> > > +       if (!npcm8xx_clk_data)
> > > +               return -ENOMEM;
> > > +
> > > +       clk_regmap =3D 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 =3D "nuvoton,npcm845-gcr", "syscon";
> >                         reg =3D <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 =3D "syscon", "simple-mfd";
>                         reg =3D <0x0 0xf0801000 0x0 0x1000>;
>=20
>                         rstc: reset-controller {
>                                 compatible =3D "nuvoton,npcm845-reset";
>                                 reg =3D <0x0 0xf0801000 0x0 0xC4>;

This isn't a valid reg property for a child node like this.

>                                 #reset-cells =3D <2>;
>                                 nuvoton,sysgcr =3D <&gcr>;
>                         };
>=20
>                         clk: clock-controller {
>                                 compatible =3D "nuvoton,npcm845-clk";
>                                 #clock-cells =3D <1>;
>                                 clocks =3D <&refclk>;
>                                 clock-names =3D "refclk";
>                         };
>                 };