Re: [PATCH v4 RESEND] clk: socfpga: agilex: add support for the Intel Agilex5

[Matthew Gerlach <matthew.gerlach@altera.com>]

On 5/6/25 1:52 PM, Stephen Boyd wrote:
> Quoting Matthew Gerlach (2025-04-17 07:52:38)
> > From: Niravkumar L Rabara <niravkumar.l.rabara@intel.com>
> >
> > Add support for Intel's SoCFPGA Agilex5 platform. The clock manager
> > driver for the Agilex5 is very similar to the Agilex platform, so
> > it is reusing most of the Agilex clock driver code.
> >
> > Signed-off-by: Teh Wen Ping <wen.ping.teh@intel.com>
> > Reviewed-by: Dinh Nguyen <dinguyen@kernel.org>
> > Signed-off-by: Niravkumar L Rabara <niravkumar.l.rabara@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@altera.com>
> > ---
> > Changes in v4:
> > - Add .index to clk_parent_data.
>
> It's useful to link to the previous round with lore links. Please do it
> next time.

Thanks for the useful recommendation. I will be sure to add such links.

So I should have added something like the following:

      - Link to v3: 
https://lore.kernel.org/linux-clk/20231003120402.4186270-1-niravkumar.l.rabara@intel.com/

>
> >
> > Changes in v3:
> > - Used different name for stratix10_clock_data pointer.
> > - Used a single function call, devm_platform_ioremap_resource().
> > - Used only .name in clk_parent_data.
> >
> > Stephen suggested to use .fw_name or .index, But since the changes are on top
> > of existing driver and current driver code is not using clk_hw and removing
> > .name and using .fw_name and/or .index resulting in parent clock_rate &
> > recalc_rate to 0.
> >
> > diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> > index 8dd94f64756b..a5ed2a22426e 100644
> > --- a/drivers/clk/socfpga/clk-agilex.c
> > +++ b/drivers/clk/socfpga/clk-agilex.c
> > @@ -334,6 +336,375 @@ static const struct stratix10_gate_clock agilex_gate_clks[] = {
> >           10, 0, 0, 0, 0, 0, 4},
> >  };
> >
> > +static const struct clk_parent_data agilex5_pll_mux[] = {
> > +       { .name = "osc1", .index = AGILEX5_OSC1, },
> > +       { .name = "cb-intosc-hs-div2-clk", .index = AGILEX5_CB_INTOSC_HS_DIV2_CLK, },
> > +       { .name = "f2s-free-clk", .index = AGILEX5_F2S_FREE_CLK, },
> > +};
> > +
> > +static const struct clk_parent_data agilex5_boot_mux[] = {
> > +       { .name = "osc1", .index = AGILEX5_OSC1, },
> > +       { .name = "cb-intosc-hs-div2-clk", .index = AGILEX5_CB_INTOSC_HS_DIV2_CLK, },
> > +};
> > +
> > +static const struct clk_parent_data agilex5_core0_free_mux[] = {
> > +       { .name = "main_pll_c1", .index = AGILEX5_MAIN_PLL_C1_CLK, },
> > +       { .name = "peri_pll_c0", .index = AGILEX5_MAIN_PLL_C0_CLK, },
>
> The index doesn't work this way. The number indicates which index in the
> DT node's 'clocks' property to use as the parent. It doesn't indicate
> which index in this clk provider to use. I don't see any 'clocks'
> property in the binding for this compatible "intel,agilex5-clkmgr", so
> this doesn't make any sense either.
Thanks for the explanation. I misunderstood how .index works.
>
> If you can't use clk_hw pointers then just stick to the old way of doing
> it with string names and no struct clk_parent_data usage.

Continuing to do the old way with string names does maximizes code 
reuse. I will remove the .index in v5.

Thanks for the review,

Matthew Gerlach

>
> > +       { .name = "osc1", .index = AGILEX5_OSC1, },
> > +       { .name = "cb-intosc-hs-div2-clk", .index = AGILEX5_CB_INTOSC_HS_DIV2_CLK, },
> > +       { .name = "f2s-free-clk", .index = AGILEX5_F2S_FREE_CLK, },
> > +};
> > +