> Subject: Re: [PATCH 1/3] clk: imx: Add PLLs driver for imx8mm soc
>
> Quoting Jacky Bai (2019-01-08 01:00:45)
> > diff --git a/drivers/clk/imx/clk-pll14xx.c
> > b/drivers/clk/imx/clk-pll14xx.c new file mode 100644 index
> > 0000000..c9f83fd
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -0,0 +1,409 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2017-2018 NXP.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/jiffies.h>
> > +
> > +#include "clk.h"
> > +
> > +#define GNRL_CTL 0x0
> > +#define DIV_CTL 0x4
> > +#define LOCK_STATUS BIT(31)
> > +#define LOCK_SEL_MASK BIT(29)
> > +#define CLKE_MASK BIT(11)
> > +#define RST_MASK BIT(9)
> > +#define BYPASS_MASK BIT(4)
> > +#define MDIV_SHIFT 12
> > +#define MDIV_MASK GENMASK(21, 12)
> > +#define PDIV_SHIFT 4
> > +#define PDIV_MASK GENMASK(9, 4)
> > +#define SDIV_SHIFT 0
> > +#define SDIV_MASK GENMASK(2, 0)
> > +#define KDIV_SHIFT 0
> > +#define KDIV_MASK GENMASK(15, 0)
> > +
> > +struct clk_pll14xx {
> > + struct clk_hw hw;
> > + void __iomem *base;
> > + enum imx_pll14xx_type type;
> > + struct imx_pll14xx_rate_table *rate_table;
> > + int rate_count;
> > +};
> > +
> > +#define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> > +
> > +static const struct imx_pll14xx_rate_table *imx_get_pll_settings(
> > + struct clk_pll14xx *pll, unsigned long rate) {
> > + const struct imx_pll14xx_rate_table *rate_table = pll->rate_table;
> > + int i;
> > +
> > + for (i = 0; i < pll->rate_count; i++) {
> > + if (rate == rate_table[i].rate)
> > + return &rate_table[i];
> > + }
>
> Nitpick: Drop the brackets.
>
Thanks, will fix it in V2.
> > +
> > + return NULL;
> > +}
> > +
> > +static long clk_pll14xx_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *prate) {
> > + struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > + const struct imx_pll14xx_rate_table *rate_table = pll->rate_table;
> > + int i;
> > +
> > + /* Assumming rate_table is in descending order */
> > + for (i = 0; i < pll->rate_count; i++) {
> > + if (rate >= rate_table[i].rate)
> > + return rate_table[i].rate;
> > + }
> > + /* return minimum supported value */
> > + return rate_table[i - 1].rate; }
> > +
> > +static unsigned long clk_pll1416x_recalc_rate(struct clk_hw *hw,
> > + unsigned long
> > +parent_rate) {
> > + struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > + u32 mdiv, pdiv, sdiv, pll_gnrl, pll_div;
> > + u64 fvco = parent_rate;
> > +
> > + pll_gnrl = readl_relaxed(pll->base);
> > + pll_div = readl_relaxed(pll->base + 4);
> > + mdiv = (pll_div & MDIV_MASK) >> MDIV_SHIFT;
> > + pdiv = (pll_div & PDIV_MASK) >> PDIV_SHIFT;
> > + sdiv = (pll_div & SDIV_MASK) >> SDIV_SHIFT;
> > +
> > + fvco *= mdiv;
> > + do_div(fvco, pdiv << sdiv);
> > +
> > + return (unsigned long)fvco;
>
> Nitpick: Drop the cast.
>
Thanks, will fix it in V2.
> > +}
> > +
> > +static unsigned long clk_pll1443x_recalc_rate(struct clk_hw *hw,
> > + unsigned long
> > +parent_rate) {
> > + struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > + u32 mdiv, pdiv, sdiv, pll_gnrl, pll_div_ctl0, pll_div_ctl1;
> > + short int kdiv;
> > + u64 fvco = parent_rate;
> > +
> > + pll_gnrl = readl_relaxed(pll->base);
> > + pll_div_ctl0 = readl_relaxed(pll->base + 4);
> > + pll_div_ctl1 = readl_relaxed(pll->base + 8);
> > + mdiv = (pll_div_ctl0 & MDIV_MASK) >> MDIV_SHIFT;
> > + pdiv = (pll_div_ctl0 & PDIV_MASK) >> PDIV_SHIFT;
> > + sdiv = (pll_div_ctl0 & SDIV_MASK) >> SDIV_SHIFT;
> > + kdiv = pll_div_ctl1 & KDIV_MASK;
> > +
> > + /* fvco = (m * 65536 + k) * Fin / (p * 65536) */
> > + fvco *= (mdiv * 65536 + kdiv);
> > + pdiv *= 65536;
> > +
> > + do_div(fvco, pdiv << sdiv);
> > +
> > + return (unsigned long)fvco;
>
> Nitpick: Drop the cast.
>
Will fix in V2.
> > +}
> > +
> > +static inline bool clk_pll1416x_mp_change(const struct
> imx_pll14xx_rate_table *rate,
> > + u32 pll_div) {
> > + u32 old_mdiv, old_pdiv;
> > +
> > + old_mdiv = (pll_div >> MDIV_SHIFT) & MDIV_MASK;
> > + old_pdiv = (pll_div >> PDIV_SHIFT) & PDIV_MASK;
> > +
> > + return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv); }
> > +
> > +static inline bool clk_pll1443x_mpk_change(const struct
> imx_pll14xx_rate_table *rate,
> > + u32 pll_div_ctl0, u32
> > +pll_div_ctl1) {
> > + u32 old_mdiv, old_pdiv, old_kdiv;
> > +
> > + old_mdiv = (pll_div_ctl0 >> MDIV_SHIFT) & MDIV_MASK;
> > + old_pdiv = (pll_div_ctl0 >> PDIV_SHIFT) & PDIV_MASK;
> > + old_kdiv = (pll_div_ctl1 >> KDIV_SHIFT) & KDIV_MASK;
> > +
> > + return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv ||
> > + rate->kdiv != old_kdiv); }
> > +
> > +static inline bool clk_pll1443x_mp_change(const struct
> imx_pll14xx_rate_table *rate,
> > + u32 pll_div_ctl0, u32
> > +pll_div_ctl1) {
> > + u32 old_mdiv, old_pdiv, old_kdiv;
> > +
> > + old_mdiv = (pll_div_ctl0 >> MDIV_SHIFT) & MDIV_MASK;
> > + old_pdiv = (pll_div_ctl0 >> PDIV_SHIFT) & PDIV_MASK;
> > + old_kdiv = (pll_div_ctl1 >> KDIV_SHIFT) & KDIV_MASK;
> > +
> > + return (rate->mdiv != old_mdiv || rate->pdiv != old_pdiv ||
> > + rate->kdiv != old_kdiv);
>
> Nitpick: Please drop the parenthesis on all the returns above.
>
Will fix in V2.
> > +}
> > +
> > +static int clk_pll14xx_wait_lock(struct clk_pll14xx *pll) {
> > + unsigned long timeout = jiffies + msecs_to_jiffies(10);
> > +
> > + /* Wait for PLL to lock */
> > + do {
> > + if (readl_relaxed(pll->base) & LOCK_STATUS)
> > + break;
> > + if (time_after(jiffies, timeout))
> > + break;
> > + } while (1);
> > +
> > + return readl_relaxed(pll->base) & LOCK_STATUS ? 0 :
> > + -ETIMEDOUT;
>
> Is this readl_poll_timeout()?
>
Yes, will fix it.
> > +}
> > +
> [...]
> > +
> > +struct clk *imx_clk_pll14xx(const char *name, const char *parent_name,
> > + void __iomem *base,
> > + const struct imx_pll14xx_clk *pll_clk) {
> > + struct clk_pll14xx *pll;
> > + struct clk *clk;
> > + struct clk_init_data init;
> > + int len;
> > +
> > + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > + if (!pll)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = name;
> > + init.flags = pll_clk->flags;
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + if (pll_clk->rate_table) {
> > + for (len = 0; pll_clk->rate_table[len].rate != 0; )
> > + len++;
> > +
> > + pll->rate_count = len;
> > + pll->rate_table = kmemdup(pll_clk->rate_table,
> > + pll->rate_count *
> > + sizeof(struct
> > + imx_pll14xx_rate_table),
>
> Is this struct_size()?
Will fix it in V2
>
> > + GFP_KERNEL);
> > + WARN(!pll->rate_table, "%s : could not alloc rate
> > + table\n", __func__);
>
> Allocations will fail with a loud message indicating as such, so this WARN is not
> necessary.
>
Will fix in V2
BR
> > + }
> > +
> > + switch (pll_clk->type) {
> > + case PLL_1416X:
> > + if (!pll->rate_table)
> > + init.ops = &clk_pll1416x_min_ops;
> > + else
> > + init.ops = &clk_pll1416x_ops;
> > + break;
> > + case PLL_1443X:
