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

[Tomer Maimon]

Hi Stephen,

Sorry for the late reply and thanks a lot for your clarifications.

I have sent today V16 with some changes according to your comments.

On Wed, 5 Apr 2023 at 22:09, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Tomer Maimon (2023-03-31 11:07:19)
> > On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@xxxxxxxxxx> 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.
>
We using the same ioremap sequence in npcm7xx clock driver since the
clock and reset share the same register region.
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
clk_base = devm_ioremap(dev, res->start, resource_size(res));
https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/clk/clk-npcm7xx.c#L413
Why we cannot use it in the NPCM8XX clock driver?

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

Sorry, but I didn't understand what you mean by using the auxiliary
bus to register the reset driver do you have an example of how it
should be done?

Thanks,

Tomer