Re: [PATCH] clk: socfpga: agilex: add clock driver for eASIC N5X platform
From: Stephen Boyd
Date: Wed Feb 10 2021 - 22:06:42 EST
Quoting Dinh Nguyen (2021-01-05 11:29:56)
> Add support for Intel's eASIC N5X platform. The clock manager driver for
> the N5X is very similar to the Agilex platform, we can re-use most of
> the Agilex clock driver.
>
> This patch makes the necessary changes for the driver to differentiate
> between the Agilex and the N5X platforms.
>
> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> ---
Sorry this patch fell off my radar.
I ran checkpatch
WARNING: function definition argument 'struct platform_device *' should also have an identifier name
#135: FILE: drivers/clk/socfpga/clk-agilex.c:500:
+ int (*probe_func)(struct platform_device *);
WARNING: Statements terminations use 1 semicolon
#140: FILE: drivers/clk/socfpga/clk-agilex.c:505:
+ return probe_func(pdev);;
WARNING: DT compatible string "intel,n5x-clkmgr" appears un-documented -- check ./Documentation/devicetree/bindings/
#147: FILE: drivers/clk/socfpga/clk-agilex.c:511:
+ { .compatible = "intel,n5x-clkmgr",
WARNING: struct clk_ops should normally be const
#290: FILE: drivers/clk/socfpga/clk-pll-s10.c:166:
+static struct clk_ops n5x_clk_pll_ops = {
WARNING: struct clk_ops should normally be const
#296: FILE: drivers/clk/socfpga/clk-pll-s10.c:172:
+static struct clk_ops agilex_clk_pll_ops = {
WARNING: function definition argument 'const struct stratix10_pll_clock *' should also have an identifier name
#367: FILE: drivers/clk/socfpga/stratix10-clk.h:78:
+struct clk *n5x_register_pll(const struct stratix10_pll_clock *,
WARNING: function definition argument 'void __iomem *' should also have an identifier name
#367: FILE: drivers/clk/socfpga/stratix10-clk.h:78:
+struct clk *n5x_register_pll(const struct stratix10_pll_clock *,
WARNING: function definition argument 'const struct n5x_perip_c_clock *' should also have an identifier name
#371: FILE: drivers/clk/socfpga/stratix10-clk.h:82:
+struct clk *n5x_register_periph(const struct n5x_perip_c_clock *,
WARNING: function definition argument 'void __iomem *' should also have an identifier name
#371: FILE: drivers/clk/socfpga/stratix10-clk.h:82:
+struct clk *n5x_register_periph(const struct n5x_perip_c_clock *,
Can you fix these up and resend?
> drivers/clk/socfpga/clk-agilex.c | 88 +++++++++++++++++++++++++++-
> drivers/clk/socfpga/clk-periph-s10.c | 53 +++++++++++++++++
> drivers/clk/socfpga/clk-pll-s10.c | 85 ++++++++++++++++++++++++++-
> drivers/clk/socfpga/stratix10-clk.h | 15 +++++
> 4 files changed, 238 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/socfpga/clk-periph-s10.c b/drivers/clk/socfpga/clk-periph-s10.c
> index 397b77b89b16..135581c41c05 100644
> --- a/drivers/clk/socfpga/clk-periph-s10.c
> +++ b/drivers/clk/socfpga/clk-periph-s10.c
> @@ -15,6 +15,21 @@
>
> #define to_periph_clk(p) container_of(p, struct socfpga_periph_clk, hw.hw)
>
> +static unsigned long n5x_clk_peri_c_clk_recalc_rate(struct clk_hw *hwclk,
> + unsigned long parent_rate)
> +{
> + struct socfpga_periph_clk *socfpgaclk = to_periph_clk(hwclk);
> + unsigned long div;
> + unsigned long shift = socfpgaclk->shift;
> + u32 val;
> +
> + val = readl(socfpgaclk->hw.reg);
> + val &= (0x1F << shift);
Prefer lowercase hex.
> + div = (val >> shift) + 1;
> +
> + return parent_rate / div;
> +}
> +
> static unsigned long clk_peri_c_clk_recalc_rate(struct clk_hw *hwclk,
> unsigned long parent_rate)
> {
> @@ -63,6 +78,11 @@ static u8 clk_periclk_get_parent(struct clk_hw *hwclk)
> return parent;
> }
>
> +static const struct clk_ops n5x_peri_c_clk_ops = {
> + .recalc_rate = n5x_clk_peri_c_clk_recalc_rate,
> + .get_parent = clk_periclk_get_parent,
> +};
> +
> static const struct clk_ops peri_c_clk_ops = {
> .recalc_rate = clk_peri_c_clk_recalc_rate,
> .get_parent = clk_periclk_get_parent,
> @@ -107,6 +127,39 @@ struct clk *s10_register_periph(const struct stratix10_perip_c_clock *clks,
> return clk;
> }
>
> +struct clk *n5x_register_periph(const struct n5x_perip_c_clock *clks,
> + void __iomem *regbase)
> +{
> + struct clk *clk;
> + struct socfpga_periph_clk *periph_clk;
> + struct clk_init_data init;
> + const char *name = clks->name;
> + const char *parent_name = clks->parent_name;
> +
> + periph_clk = kzalloc(sizeof(*periph_clk), GFP_KERNEL);
> + if (WARN_ON(!periph_clk))
> + return NULL;
> +
> + periph_clk->hw.reg = regbase + clks->offset;
> + periph_clk->shift = clks->shift;
> +
> + init.name = name;
> + init.ops = &n5x_peri_c_clk_ops;
> + init.flags = clks->flags;
> +
> + init.num_parents = clks->num_parents;
> + init.parent_names = parent_name ? &parent_name : NULL;
> +
> + periph_clk->hw.hw.init = &init;
> +
> + clk = clk_register(NULL, &periph_clk->hw.hw);
Can you use clk_hw_register?
> + if (WARN_ON(IS_ERR(clk))) {
> + kfree(periph_clk);
> + return NULL;
> + }
> + return clk;
> +}
> +
> struct clk *s10_register_cnt_periph(const struct stratix10_perip_cnt_clock *clks,
> void __iomem *regbase)
> {
[...]
> @@ -214,3 +260,40 @@ struct clk *agilex_register_pll(const struct stratix10_pll_clock *clks,
> }
> return clk;
> }
> +
> +struct clk *n5x_register_pll(const struct stratix10_pll_clock *clks,
> + void __iomem *reg)
> +{
> + struct clk *clk;
> + struct socfpga_pll *pll_clk;
> + struct clk_init_data init;
> + const char *name = clks->name;
> +
> + pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
> + if (WARN_ON(!pll_clk))
> + return NULL;
> +
> + pll_clk->hw.reg = reg + clks->offset;
> +
> + if (streq(name, SOCFPGA_BOOT_CLK))
> + init.ops = &clk_boot_ops;
> + else
> + init.ops = &n5x_clk_pll_ops;
> +
> + init.name = name;
> + init.flags = clks->flags;
> +
> + init.num_parents = clks->num_parents;
> + init.parent_names = NULL;
> + init.parent_data = clks->parent_data;
> + pll_clk->hw.hw.init = &init;
> +
> + pll_clk->hw.bit_idx = SOCFPGA_PLL_POWER;
> +
> + clk = clk_register(NULL, &pll_clk->hw.hw);
Can you use clk_hw_register?
> + if (WARN_ON(IS_ERR(clk))) {
> + kfree(pll_clk);
> + return NULL;
> + }
> + return clk;
> +}