| 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: <495fcc93ab28ff8949569ededee954c1.sboyd@kernel.org>
Date: Mon, 20 Mar 2023 12:50:48 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Tomer Maimon <tmaimon77@...il.com>, avifishman70@...il.com,
benjaminfair@...gle.com, joel@....id.au, mturquette@...libre.com,
tali.perry1@...il.com, venture@...gle.com, yuenn@...gle.com
Cc: openbmc@...ts.ozlabs.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, Tomer Maimon <tmaimon77@...il.com>
Subject: Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
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,
Powered by blists - more mailing lists