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

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

On 6/30/25 6:05 PM, Stephen Boyd wrote:
> Quoting Matthew Gerlach (2025-05-13 16:48:37)
> > diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> > index 8dd94f64756b..43c1e4e26cf0 100644
> > --- a/drivers/clk/socfpga/clk-agilex.c
> > +++ b/drivers/clk/socfpga/clk-agilex.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - * Copyright (C) 2019, Intel Corporation
> > + * Copyright (C) 2019-2024, Intel Corporation
> > + * Copyright (C) 2025, Altera Corporation
> >   */
> >  #include <linux/slab.h>
> >  #include <linux/clk-provider.h>
> > @@ -8,6 +9,7 @@
> >  #include <linux/platform_device.h>
> >  
> >  #include <dt-bindings/clock/agilex-clock.h>
> > +#include <dt-bindings/clock/intel,agilex5-clkmgr.h>
> >  
> >  #include "stratix10-clk.h"
> >  
> > @@ -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", },
> > +       { .name = "cb-intosc-hs-div2-clk", },
> > +       { .name = "f2s-free-clk", },
>
> Please don't use clk_parent_data with only .name set with dot
> initializers. This is actually { .index = 0, .name = "..." } which means
> the core is looking at the DT node for the first index of the 'clocks'
If the core should be looking for the first index of the 'clocks' 
property, is it better to explicitly to state ".index = 0" or just have 
the name?
> property. If you're using clk_parent_data you should have a DT node that
> has a 'clocks' property. If the clks are all internal to the device then
> use clk_hw pointers directly with clk_hws.
The clocks are not all internal to the device. The "osc1" clock is 
external to the device and connected to a pin.

Thanks for the feedback,
Matthew Gerlach