| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP6Zq1hOHJWQSmGoVDz5bSjwdhNyQmaZVOEE8_dX6S4HCFQ2Jg@mail.gmail.com>
Date: Fri, 31 Mar 2023 21:07:19 +0300
From: Tomer Maimon <tmaimon77@...il.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: avifishman70@...il.com, benjaminfair@...gle.com, joel@....id.au,
mturquette@...libre.com, tali.perry1@...il.com, venture@...gle.com,
yuenn@...gle.com, openbmc@...ts.ozlabs.org,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
HI Stephen,
Thanks for your comments
On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@...nel.org> wrote:
>
> 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?
Will add
>
> > + { 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.
I thought using .name was done when the clock defines in the DT, like
the ref clock.
If the other clocks are not defined both .fw_name and .name the clocks
are not registered properly.
>
> > + { .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.
Should the clk_parent_data mux use only .index? if yes how should the
clock tree have a connection between the parent's clock and the mux
for example:
for example, how should the driver connect between
NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?
>
> > + { .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?
Will remove.
>
> > +
> > + 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?
We should use devm_ioremap since the clock register is used for the
reset driver as well.
>
> > + 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?
I will try to do it.
>
> > + &div_data->parent_data,
> > + div_data->flags,
Thanks,
Tomer
Powered by blists - more mailing lists