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

[Stephen Boyd <sboyd@kernel.org>]

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?

> +       {
> +		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.

> +       { 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
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.