Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller

[Stephen Boyd <sboyd@kernel.org>]

Quoting Tomer Maimon (2023-03-31 11:07:19)
> On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2023-03-09 11:44:02)
> > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > new file mode 100644
> > > index 000000000000..67058f121251
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-npcm8xx.c
> > > +       { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 },
> > > +       { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 },
> > > +       { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 },
> > > +       { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 },
> > > +};
> > > +
> > > +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
> > > +static const struct clk_parent_data cpuck_mux_parents[] = {
> > > +       { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
> >
> > You should only have .fw_name or .index when introducing new drivers.
> > The .name field is for existing drivers that want to migrate to
> > clk_parent_data.
> I thought using .name was done when the clock defines in the DT, like
> the ref clock.
> If the other clocks are not defined both .fw_name and .name the clocks
> are not registered properly.

Are you saying that having .name fixes it?

> >
> > > +       { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> > > +       { .name = NPCM8XX_CLK_S_REFCLK },
> >
> > Note, this line says to use '.index = 0', and .name will be ignored.
> > Maybe just use the index for everything? That makes it simpler and
> > potentially faster because we don't have to do string comparisons
> > anywhere.
> Should the clk_parent_data mux use only .index? if yes how should the
> clock tree have a connection between the parent's clock and the mux
> for example:
> for example, how should the driver connect between
> NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?

It's not required, but it makes things simpler to only use .index or
direct clk_hw pointers (.hw). I'm working on a clk documentation
overhaul series right now, about 4 years later than I should have done
it. It will cover this.

The .index field corresponds to the cell index in your devicetree
'clocks' property of the clk provider (the node with #clock-cells
property). If the clk is internal, just use a .hw member and point to it
directly. Don't consume your own clks in DT. If NPCM8XX_CLK_S_PLL1_DIV2
is a clk provided/registered by this device then it should be pointed to
directly with the clk_hw pointer. If NPCM8XX_CLK_S_PLL1_DIV2 is an
external clk that is consumed via the 'clocks' property in DT, then it
should be specified as a parent via the .index member.

> > > +
> > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > +{
> > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > +       struct device *dev = &pdev->dev;
> > > +       void __iomem *clk_base;
> > > +       struct resource *res;
> > > +       struct clk_hw *hw;
> > > +       unsigned int i;
> > > +       int err;
> > > +
> > > +       npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > > +                                                        NPCM8XX_NUM_CLOCKS),
> > > +                                       GFP_KERNEL);
> > > +       if (!npcm8xx_clk_data)
> > > +               return -ENOMEM;
> > > +
> > > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +       clk_base = devm_ioremap(dev, res->start, resource_size(res));
> >
> > Can you use devm_platform_ioremap_resource() instead?
> We should use devm_ioremap since the clock register is used for the
> reset driver as well.

Are the clk and reset drivers sharing the register range? If so, please
use auxiliary bus to register the reset driver, and map the register
region once in the driver that registers the auxiliary device. Pass the
iomem pointer to the auxiliary device.