Messages in this thread | | | Subject | Re: [PATCH v3] clk: socfpga: agilex: add support for the Intel Agilex5 | From | Stephen Boyd <> | Date | Thu, 12 Oct 2023 16:30:02 -0700 |
| |
Quoting niravkumar.l.rabara@intel.com (2023-10-03 05:04:02) > 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> > --- > > 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. > > In order to use .index, I would need to refactor the common code that is shared > by a few Intel SoCFPGA platforms (S10, Agilex and N5x). So, if using .name for > this patch is acceptable then I will upgrade clk-agilex.c in future submission.
It is not acceptable. We don't want there to only be a name member set in a clk_parent_data structure. In fact, this driver is simply wrong because it has many clk_parent_data structures with .fw_name == .name and I don't see any 'clock-names' property in the DT bindings. The driver looks like it should simply use clk_hw pointers directly and stop using clk_parent_data structures entirely.
> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c > index 6b65a74aefa6..38ea7e7f600b 100644 > --- a/drivers/clk/socfpga/clk-agilex.c > +++ b/drivers/clk/socfpga/clk-agilex.c > @@ -1,6 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > - * Copyright (C) 2019, Intel Corporation > + * Copyright (C) 2019-2023, Intel Corporation > */ > #include <linux/slab.h> > #include <linux/clk-provider.h> > @@ -8,6 +8,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" > > @@ -40,6 +41,44 @@ static const struct clk_parent_data mpu_free_mux[] = { > .name = "f2s-free-clk", }, > }; > > +static const struct clk_parent_data core0_free_mux[] = { > + { .name = "main_pll_c1" },
This is equivalent to the above.
{ .name = "main_pll_c1", .index = 0 },
and thus the index will be used. Luckily there's no clocks property in DT? But it also means that you're trying to lookup a clk from DT and falling back to the name field eventually, i.e. we're wasting time during parent discovery.
> + { .name = "peri_pll_c0" }, > + { .name = "osc1" }, > + { .name = "cb-intosc-hs-div2-clk" }, > + { .name = "f2s-free-clk" }, > +}; > +
| |