RE: [PATCH 1/3] clk: imx: Add PLLs driver for imx8mm soc

[Jacky Bai <ping.bai@nxp.com>]

> 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: