Re: [PATCH v20] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

Thanks for your comments and sorry for the late reply.

On Sat, 7 Oct 2023 at 02:50, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Tomer Maimon (2023-09-28 15:40:51)
> > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > new file mode 100644
> > index 000000000000..e575a8676ca3
> > --- /dev/null
> > +++ b/drivers/clk/clk-npcm8xx.c
> > @@ -0,0 +1,547 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> [...]
> > +
> > +/* configurable dividers: */
> > +static const struct npcm8xx_clk_div_data npcm8xx_divs[] = {
> > +       { NPCM8XX_CLKDIV1, 28, 3, NPCM8XX_CLK_S_ADC,
> > +       { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 },
> > +       CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_ADC },
>
> Please format this some other way. I assume one line means one clk, but
> here it is actually three lines. Perhaps something like this?
Ready in V21
>
> > +       {
> > +             NPCM8XX_CLKDIV1, 28, 3, NPCM8XX_CLK_S_ADC,
> > +             { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 },
> > +             CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_ADC
> > +       },
>
> Please stop using the .name member of struct clk_parent_data. That
> member is only there to support drivers that are migrating from a
> binding that didn't specify the parents of clks that are outside of the
> clk controller with the clocks property in their DT node. I see that the
> dts exists upstream, but luckily we don't have a driver merged, so we're
> free to change the binding to specify clks external to the node. The
> .fw_name member will match a 'clock-names' element for the registering
> driver's node. The .index member will match the index in the 'clocks'
> property. Neither of those properties exist in the nuvoton,npcm845-clk
> DT binding, so neither of those members shall be present. This means
> that either the binding needs to be updated, or the clk_parent_data
> structure should be replaced with clk_hw pointers to describe parents.
> I'm going to guess that there aren't any external clk parents, so to
> keep things simple this driver should change to use direct clk_hw
> pointers to describe topology.
Ready in V21
>
> > +       { NPCM8XX_CLKDIV1, 26, 2, NPCM8XX_CLK_S_AHB, { .hw = &hw_pre_clk },
> > +       CLK_DIVIDER_READ_ONLY, CLK_IS_CRITICAL, NPCM8XX_CLK_AHB },
> > +       { NPCM8XX_CLKDIV1, 21, 5, NPCM8XX_CLK_S_PRE_ADC,
> > +       { .hw = &npcm8xx_muxes[6].hw }, CLK_DIVIDER_READ_ONLY, 0, -1 },
> > +       { NPCM8XX_CLKDIV1, 16, 5, NPCM8XX_CLK_S_UART,
> > +       { .hw = &npcm8xx_muxes[3].hw }, 0, 0, NPCM8XX_CLK_UART },
> > +       { NPCM8XX_CLKDIV1, 11, 5, NPCM8XX_CLK_S_MMC,
> > +       { .hw = &npcm8xx_muxes[2].hw }, CLK_DIVIDER_READ_ONLY, 0,
> > +       NPCM8XX_CLK_MMC },
> > +       { NPCM8XX_CLKDIV1, 6, 5, NPCM8XX_CLK_S_SPI3,
> > +       { .fw_name = NPCM8XX_CLK_S_AHB, .name = NPCM8XX_CLK_S_AHB }, 0, 0,
> > +       NPCM8XX_CLK_SPI3 },
> > +       { NPCM8XX_CLKDIV1, 2, 4, NPCM8XX_CLK_S_PCI,
> > +       { .hw = &npcm8xx_muxes[7].hw }, CLK_DIVIDER_READ_ONLY, 0,
> > +       NPCM8XX_CLK_PCI },
>
> BTW, I looked at the dts file upstream (nuvoton-common-npcm8xx.dtsi).
> The reset and clock controller start at the same address, which can only
> mean that they're actually the same device. The two nodes should be
unfortunately, It is two services (reset and clock) that are handled
in the same memory space.
> combined into one node and one driver should match that compatible so
> that one IO mapping is made for the entire clock and reset contoller
> register space. If you want, that driver can make two auxiliary device
> drivers for the reset and clk parts of the io space and then those two
> drivers can reside in drivers/reset and drivers/clk. I don't know where
> the driver goes that matches the compatible node though, probably in
> drivers/soc. This allows us to properly model the logical components
> that make up the device in hardware (clks and resets) while also
> allowing any device specific things for that entire register space to
> live in the soc driver. For example, if some power domain needs to be
> enabled to access that register space it would be attached to the soc
> driver.
Sorry I didn't understand, do you mean to have one driver that handles
the clock and the reset modules and will sis under driver/soc
or one driver that handles the reset and clock IO space?

What about using regmap to handle the clock and the reset? for this,
the NPCM clock driver will use a unique clock setting like it is done
in Tegra clk.
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/clk/tegra/clk-divider.c
instead of using clk_divide and clk_mux default services.

Thanks,

Tomer