[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4be60c499a39fcca374bc8f8574a952e.sboyd@kernel.org>
Date: Thu, 12 Oct 2023 16:30:02 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Dinh Nguyen <dinguyen@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
niravkumar.l.rabara@...el.com
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
netdev@...r.kernel.org,
Niravkumar L Rabara <niravkumar.l.rabara@...el.com>,
Teh Wen Ping <wen.ping.teh@...el.com>
Subject: Re: [PATCH v3] clk: socfpga: agilex: add support for the Intel Agilex5
Quoting niravkumar.l.rabara@...el.com (2023-10-03 05:04:02)
> From: Niravkumar L Rabara <niravkumar.l.rabara@...el.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@...el.com>
> Reviewed-by: Dinh Nguyen <dinguyen@...nel.org>
> Signed-off-by: Niravkumar L Rabara <niravkumar.l.rabara@...el.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" },
> +};
> +
Powered by blists - more mailing lists