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

[Stephen Boyd]

Quoting niravkumar.l.rabara@xxxxxxxxx (2023-10-03 05:04:02)
> From: Niravkumar L Rabara <niravkumar.l.rabara@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Reviewed-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> Signed-off-by: Niravkumar L Rabara <niravkumar.l.rabara@xxxxxxxxx>
> ---
> 
> 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" },
> +};
> +