Messages in this thread Patch in this message | | | Subject | Re: [PATCH] clk: socfpga: agilex: add clock driver for eASIC N5X platform | From | Stephen Boyd <> | Date | Wed, 10 Feb 2021 19:05:35 -0800 |
| |
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@kernel.org> > ---
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; > +}
| |