Messages in this thread | | | Subject | Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller | From | Stephen Boyd <> | Date | Mon, 20 Mar 2023 12:50:48 -0700 |
| |
Quoting Tomer Maimon (2023-03-09 11:44:02) > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c > new file mode 100644 > index 000000000000..67058f121251 > --- /dev/null > +++ b/drivers/clk/clk-npcm8xx.c > @@ -0,0 +1,561 @@ [...] > + > +struct npcm8xx_pll_data { > + const char *name; > + struct clk_parent_data parent; > + unsigned int reg; > + unsigned long flags; > +}; > + > +struct npcm8xx_clk_div_data { > + u32 reg; > + u8 shift; > + u8 width; > + const char *name; > + const struct clk_parent_data parent_data; > + u8 clk_divider_flags; > + unsigned long flags; > + int onecell_idx; > +}; > + > +struct npcm8xx_clk_mux_data { > + u8 shift; > + u32 mask; > + const u32 *table; > + const char *name; > + const struct clk_parent_data *parent_data; > + u8 num_parents; > + unsigned long flags; > +}; > + [...] > + > +static struct npcm8xx_pll_data npcm8xx_pll_clks[] = {
Can this be const?
> + { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 }, > + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 }, > + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 }, > + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 }, > +}; > + > +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 }; > +static const struct clk_parent_data cpuck_mux_parents[] = { > + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
You should only have .fw_name or .index when introducing new drivers. The .name field is for existing drivers that want to migrate to clk_parent_data.
> + { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 }, > + { .name = NPCM8XX_CLK_S_REFCLK },
Note, this line says to use '.index = 0', and .name will be ignored. Maybe just use the index for everything? That makes it simpler and potentially faster because we don't have to do string comparisons anywhere.
> + { .fw_name = NPCM8XX_CLK_S_PLL2, .name = NPCM8XX_CLK_S_PLL2 } > +}; > + > +static const u32 pixcksel_mux_table[] = { 0, 2 }; > +static const struct clk_parent_data pixcksel_mux_parents[] = { > + { .fw_name = NPCM8XX_CLK_S_PLL_GFX, .name = NPCM8XX_CLK_S_PLL_GFX }, > + { .name = NPCM8XX_CLK_S_REFCLK } > +}; > + [...] > + > +static unsigned long npcm8xx_clk_pll_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct npcm8xx_clk_pll *pll = to_npcm8xx_clk_pll(hw); > + unsigned long fbdv, indv, otdv1, otdv2; > + unsigned int val; > + u64 ret; > + > + if (parent_rate == 0) { > + pr_debug("%s: parent rate is zero\n", __func__); > + return 0; > + } > + > + val = readl_relaxed(pll->pllcon + pll->reg);
Is pll->reg ever set?
> + > + indv = FIELD_GET(PLLCON_INDV, val); > + fbdv = FIELD_GET(PLLCON_FBDV, val); > + otdv1 = FIELD_GET(PLLCON_OTDV1, val); > + otdv2 = FIELD_GET(PLLCON_OTDV2, val); > + > + ret = (u64)parent_rate * fbdv; > + do_div(ret, indv * otdv1 * otdv2); > + > + return ret; > +} > + > +static const struct clk_ops npcm8xx_clk_pll_ops = { > + .recalc_rate = npcm8xx_clk_pll_recalc_rate, > +}; > + > +static struct clk_hw * > +npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon, > + const char *name, const struct clk_parent_data *parent, > + unsigned long flags) > +{ > + struct npcm8xx_clk_pll *pll; > + struct clk_init_data init = {}; > + int ret; > + > + pll = kzalloc(sizeof(*pll), GFP_KERNEL); > + if (!pll) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &npcm8xx_clk_pll_ops; > + init.parent_data = parent; > + init.num_parents = 1; > + init.flags = flags; > + > + pll->pllcon = pllcon; > + pll->hw.init = &init; > + > + ret = devm_clk_hw_register(dev, &pll->hw); > + if (ret) { > + kfree(pll); > + return ERR_PTR(ret); > + } > + > + return &pll->hw; > +} > + > +static DEFINE_SPINLOCK(npcm8xx_clk_lock); > + > +static int npcm8xx_clk_probe(struct platform_device *pdev) > +{ > + struct clk_hw_onecell_data *npcm8xx_clk_data; > + struct device *dev = &pdev->dev; > + void __iomem *clk_base; > + struct resource *res; > + struct clk_hw *hw; > + unsigned int i; > + int err; > + > + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws, > + NPCM8XX_NUM_CLOCKS), > + GFP_KERNEL); > + if (!npcm8xx_clk_data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + clk_base = devm_ioremap(dev, res->start, resource_size(res));
Can you use devm_platform_ioremap_resource() instead?
> + if (!clk_base) {
Then this is checked for error pointer.
> + dev_err(&pdev->dev, "Failed to remap I/O memory\n");
And no error message.
> + return -ENOMEM; > + } > + > + npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS; > + [....] > + if (IS_ERR(hw)) { > + dev_err(dev, "npcm8xx_clk: Can't register axi div2\n"); > + return PTR_ERR(hw); > + } > + npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = hw; > + > + hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB, > + NPCM8XX_CLK_S_AXI, 0, 1, 2); > + if (IS_ERR(hw)) { > + dev_err(dev, "npcm8xx_clk: Can't register atb div2\n"); > + return PTR_ERR(hw); > + } > + npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = hw; > + > + /* Register clock dividers specified in npcm8xx_divs */ > + for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) { > + const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i]; > + > + hw = clk_hw_register_divider_parent_data(dev, div_data->name,
Do we have a devm_ variant of this function? If not, can you add it?
> + &div_data->parent_data, > + div_data->flags,
| |