[v9,3/5] clk: imx: add SCCG PLL type

Commit Message

Abel Vesa Sept. 24, 2018, 10:39 a.m. UTC

From: Lucas Stach <[e-mail]>

The SCCG is a new PLL type introduced on i.MX8. Add support for this.
The driver currently misses the PLL lock check, as the preliminary
documentation mentions lock configurations, but is quiet about where
to find the actual lock status signal.

Signed-off-by: Lucas Stach <[e-mail]>
Signed-off-by: Abel Vesa <[e-mail]>
---
 drivers/clk/imx/Makefile       |   3 +-
 drivers/clk/imx/clk-sccg-pll.c | 237 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk.h          |   9 ++
 3 files changed, 248 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/imx/clk-sccg-pll.c

Comments

Stephen Boyd Oct. 17, 2018, 7:55 p.m. UTC | #1

Quoting Abel Vesa (2018-09-24 03:39:55)
> diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> new file mode 100644
> index 0000000..a9837fa
> --- /dev/null
> +++ b/drivers/clk/imx/clk-sccg-pll.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright 2018 NXP.
> + */
> +
> +#include <linux/clk.h>

Is this include used? Otherwise should see clk-provider.h included here.

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/bitfield.h>
> +
> +#include "clk.h"
> +
> +/* PLL CFGs */
> +#define PLL_CFG0               0x0
> +#define PLL_CFG1               0x4
> +#define PLL_CFG2               0x8
> +
> +#define PLL_DIVF1_MASK         GENMASK(18, 13)
> +#define PLL_DIVF2_MASK         GENMASK(12, 7)
> +#define PLL_DIVR1_MASK         GENMASK(27, 25)
> +#define PLL_DIVR2_MASK         GENMASK(24, 19)
> +#define PLL_REF_MASK           GENMASK(2, 0)
> +
> +#define PLL_LOCK_MASK          BIT(31)
> +#define PLL_PD_MASK            BIT(7)
> +
> +#define OSC_25M                        25000000
> +#define OSC_27M                        27000000
> +
> +#define PLL_SCCG_LOCK_TIMEOUT  70
> +
> +struct clk_sccg_pll {
> +       struct clk_hw   hw;
> +       void __iomem    *base;
> +};
> +
> +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> +
> +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> +{
> +       u32 val;
> +
> +       return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> +                                       PLL_SCCG_LOCK_TIMEOUT);
> +}
> +
> +static int clk_pll1_is_prepared(struct clk_hw *hw)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(pll->base + PLL_CFG0);
> +       return (val & PLL_PD_MASK) ? 0 : 1;
> +}
> +
> +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val, divf;
> +
> +       val = readl_relaxed(pll->base + PLL_CFG2);
> +       divf = FIELD_GET(PLL_DIVF1_MASK, val);
> +
> +       return parent_rate * 2 * (divf + 1);
> +}
> +
> +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> +                              unsigned long *prate)
> +{
> +       unsigned long parent_rate = *prate;
> +       u32 div;
> +
> +       div = rate / (parent_rate * 2);

Can parent_rate be 0?

> +
> +       return parent_rate * div * 2;
> +}
> +
> +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> +                           unsigned long parent_rate)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val;
> +       u32 divf;
> +
> +       divf = rate / (parent_rate * 2);

Can parent_rate be 0?

> +
> +       val = readl_relaxed(pll->base + PLL_CFG2);
> +       val &= ~PLL_DIVF1_MASK;
> +       val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> +       writel_relaxed(val, pll->base + PLL_CFG2);
> +
> +       return clk_pll_wait_lock(pll);
> +}
> +
> +static int clk_pll1_prepare(struct clk_hw *hw)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(pll->base + PLL_CFG0);
> +       val &= ~PLL_PD_MASK;
> +       writel_relaxed(val, pll->base + PLL_CFG0);
> +
> +       return clk_pll_wait_lock(pll);
> +}
> +
> +static void clk_pll1_unprepare(struct clk_hw *hw)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(pll->base + PLL_CFG0);
> +       val |= PLL_PD_MASK;
> +       writel_relaxed(val, pll->base + PLL_CFG0);
> +
> +}
> +
> +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +       u32 val, ref, divr1, divf1, divr2, divf2;
> +       u64 temp64;
> +
> +       val = readl_relaxed(pll->base + PLL_CFG0);
> +       switch (FIELD_GET(PLL_REF_MASK, val)) {
> +       case 0:
> +               ref = OSC_25M;
> +               break;
> +       case 1:
> +               ref = OSC_27M;
> +               break;
> +       default:
> +               ref = OSC_25M;

Does this information not come through 'parent_rate'?

> +               break;
> +       }
> +
> +       val = readl_relaxed(pll->base + PLL_CFG2);
> +       divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> +       divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> +       divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> +       divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> +
> +       temp64 = ref * 2;
> +       temp64 *= (divf1 + 1) * (divf2 + 1);
> +
> +       do_div(temp64, (divr1 + 1) * (divr2 + 1));

Nitpicks: A comment with the equation may be helpful to newcomers.

> +
> +       return (unsigned long)temp64;

Drop useless cast please.

> +}
> +
> +static long clk_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> +                              unsigned long *prate)
> +{
> +       u32 div;
> +       unsigned long parent_rate = *prate;
> +
> +       div = rate / (parent_rate);
> +
> +       return parent_rate * div;
> +}
> +
> +static int clk_pll2_set_rate(struct clk_hw *hw, unsigned long rate,
> +                           unsigned long parent_rate)
> +{
> +       u32 val;
> +       u32 divf;
> +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> +
> +       divf = rate / (parent_rate);

Drop useless parenthesis please.

> +
> +       val = readl_relaxed(pll->base + PLL_CFG2);
> +       val &= ~PLL_DIVF2_MASK;
> +       val |= FIELD_PREP(PLL_DIVF2_MASK, divf - 1);
> +       writel_relaxed(val, pll->base + PLL_CFG2);
> +
> +       return clk_pll_wait_lock(pll);
> +}
> +
> +static const struct clk_ops clk_sccg_pll1_ops = {
> +       .is_prepared    = clk_pll1_is_prepared,
> +       .recalc_rate    = clk_pll1_recalc_rate,
> +       .round_rate     = clk_pll1_round_rate,
> +       .set_rate       = clk_pll1_set_rate,
> +};
> +
> +static const struct clk_ops clk_sccg_pll2_ops = {
> +       .prepare        = clk_pll1_prepare,
> +       .unprepare      = clk_pll1_unprepare,
> +       .recalc_rate    = clk_pll2_recalc_rate,
> +       .round_rate     = clk_pll2_round_rate,
> +       .set_rate       = clk_pll2_set_rate,
> +};
> +
> +struct clk *imx_clk_sccg_pll(const char *name,
> +                               const char *parent_name,
> +                               void __iomem *base,
> +                               enum imx_sccg_pll_type pll_type)
> +{
> +       struct clk_sccg_pll *pll;
> +       struct clk *clk;
> +       struct clk_init_data init;
> +
> +       pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +       if (!pll)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pll->base = base;
> +       init.name = name;
> +       switch (pll_type) {
> +       case SCCG_PLL1:
> +               init.ops = &clk_sccg_pll1_ops;
> +               break;
> +       case SCCG_PLL2:
> +               init.ops = &clk_sccg_pll2_ops;
> +               break;
> +       default:
> +               kfree(pll);
> +               return ERR_PTR(-EINVAL);

Do this case statement before allocating? So that kfree() isn't required
here?

> +       }
> +
> +       init.flags = 0;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       pll->hw.init = &init;
> +
> +       clk = clk_register(NULL, &pll->hw);

Any chance to use clk_hw based registration APIs?

> +       if (IS_ERR(clk))
> +               kfree(pll);
> +
> +       return clk;

Abel Vesa Nov. 7, 2018, 11:54 a.m. UTC | #2

On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-09-24 03:39:55)
> > diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
> > new file mode 100644
> > index 0000000..a9837fa
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-sccg-pll.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2018 NXP.
> > + */
> > +
> > +#include <linux/clk.h>
> 
> Is this include used? Otherwise should see clk-provider.h included here.
> 

Fixed in the next version.

> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/bitfield.h>
> > +
> > +#include "clk.h"
> > +
> > +/* PLL CFGs */
> > +#define PLL_CFG0               0x0
> > +#define PLL_CFG1               0x4
> > +#define PLL_CFG2               0x8
> > +
> > +#define PLL_DIVF1_MASK         GENMASK(18, 13)
> > +#define PLL_DIVF2_MASK         GENMASK(12, 7)
> > +#define PLL_DIVR1_MASK         GENMASK(27, 25)
> > +#define PLL_DIVR2_MASK         GENMASK(24, 19)
> > +#define PLL_REF_MASK           GENMASK(2, 0)
> > +
> > +#define PLL_LOCK_MASK          BIT(31)
> > +#define PLL_PD_MASK            BIT(7)
> > +
> > +#define OSC_25M                        25000000
> > +#define OSC_27M                        27000000
> > +
> > +#define PLL_SCCG_LOCK_TIMEOUT  70
> > +
> > +struct clk_sccg_pll {
> > +       struct clk_hw   hw;
> > +       void __iomem    *base;
> > +};
> > +
> > +#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
> > +
> > +static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
> > +{
> > +       u32 val;
> > +
> > +       return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
> > +                                       PLL_SCCG_LOCK_TIMEOUT);
> > +}
> > +
> > +static int clk_pll1_is_prepared(struct clk_hw *hw)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val;
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > +       return (val & PLL_PD_MASK) ? 0 : 1;
> > +}
> > +
> > +static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long parent_rate)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val, divf;
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > +       divf = FIELD_GET(PLL_DIVF1_MASK, val);
> > +
> > +       return parent_rate * 2 * (divf + 1);
> > +}
> > +
> > +static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long *prate)
> > +{
> > +       unsigned long parent_rate = *prate;
> > +       u32 div;
> > +
> > +       div = rate / (parent_rate * 2);
> 
> Can parent_rate be 0?
> 

Fixed in the next version.

> > +
> > +       return parent_rate * div * 2;
> > +}
> > +
> > +static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                           unsigned long parent_rate)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val;
> > +       u32 divf;
> > +
> > +       divf = rate / (parent_rate * 2);
> 
> Can parent_rate be 0?
> 

Fixed in the next version.

> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > +       val &= ~PLL_DIVF1_MASK;
> > +       val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
> > +       writel_relaxed(val, pll->base + PLL_CFG2);
> > +
> > +       return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static int clk_pll1_prepare(struct clk_hw *hw)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val;
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > +       val &= ~PLL_PD_MASK;
> > +       writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > +       return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static void clk_pll1_unprepare(struct clk_hw *hw)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val;
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > +       val |= PLL_PD_MASK;
> > +       writel_relaxed(val, pll->base + PLL_CFG0);
> > +
> > +}
> > +
> > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long parent_rate)
> > +{
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +       u32 val, ref, divr1, divf1, divr2, divf2;
> > +       u64 temp64;
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > +       switch (FIELD_GET(PLL_REF_MASK, val)) {
> > +       case 0:
> > +               ref = OSC_25M;
> > +               break;
> > +       case 1:
> > +               ref = OSC_27M;
> > +               break;
> > +       default:
> > +               ref = OSC_25M;
> 
> Does this information not come through 'parent_rate'?
> 

No. So basically both pll1 and pll2 and the divider after it form together this SCCG:

https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834

See: Figure 5-8. SSCG PLL Block Diagram

We're basically reading the input of the pll 1 in order to compute the output of the entire SCCG.

I know it's a mess. I'm working on cleaning it up, but for now we need this in in order to boot up.

> > +               break;
> > +       }
> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > +       divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > +       divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > +       divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > +       divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > +
> > +       temp64 = ref * 2;
> > +       temp64 *= (divf1 + 1) * (divf2 + 1);
> > +
> > +       do_div(temp64, (divr1 + 1) * (divr2 + 1));
> 
> Nitpicks: A comment with the equation may be helpful to newcomers.

Since the SCCG is contructed by multiple different types of clocks here, the equation doesn't help
since it is spread in all constructing blocks.

> 
> > +
> > +       return (unsigned long)temp64;
> 
> Drop useless cast please.
> 

Fixed in the next version.

> > +}
> > +
> > +static long clk_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long *prate)
> > +{
> > +       u32 div;
> > +       unsigned long parent_rate = *prate;
> > +
> > +       div = rate / (parent_rate);
> > +
> > +       return parent_rate * div;
> > +}
> > +
> > +static int clk_pll2_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                           unsigned long parent_rate)
> > +{
> > +       u32 val;
> > +       u32 divf;
> > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > +
> > +       divf = rate / (parent_rate);
> 
> Drop useless parenthesis please.
> 

Fixed in the next version.

> > +
> > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > +       val &= ~PLL_DIVF2_MASK;
> > +       val |= FIELD_PREP(PLL_DIVF2_MASK, divf - 1);
> > +       writel_relaxed(val, pll->base + PLL_CFG2);
> > +
> > +       return clk_pll_wait_lock(pll);
> > +}
> > +
> > +static const struct clk_ops clk_sccg_pll1_ops = {
> > +       .is_prepared    = clk_pll1_is_prepared,
> > +       .recalc_rate    = clk_pll1_recalc_rate,
> > +       .round_rate     = clk_pll1_round_rate,
> > +       .set_rate       = clk_pll1_set_rate,
> > +};
> > +
> > +static const struct clk_ops clk_sccg_pll2_ops = {
> > +       .prepare        = clk_pll1_prepare,
> > +       .unprepare      = clk_pll1_unprepare,
> > +       .recalc_rate    = clk_pll2_recalc_rate,
> > +       .round_rate     = clk_pll2_round_rate,
> > +       .set_rate       = clk_pll2_set_rate,
> > +};
> > +
> > +struct clk *imx_clk_sccg_pll(const char *name,
> > +                               const char *parent_name,
> > +                               void __iomem *base,
> > +                               enum imx_sccg_pll_type pll_type)
> > +{
> > +       struct clk_sccg_pll *pll;
> > +       struct clk *clk;
> > +       struct clk_init_data init;
> > +
> > +       pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > +       if (!pll)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       pll->base = base;
> > +       init.name = name;
> > +       switch (pll_type) {
> > +       case SCCG_PLL1:
> > +               init.ops = &clk_sccg_pll1_ops;
> > +               break;
> > +       case SCCG_PLL2:
> > +               init.ops = &clk_sccg_pll2_ops;
> > +               break;
> > +       default:
> > +               kfree(pll);
> > +               return ERR_PTR(-EINVAL);
> 
> Do this case statement before allocating? So that kfree() isn't required
> here?
> 

Fixed in the next version.

> > +       }
> > +
> > +       init.flags = 0;
> > +       init.parent_names = &parent_name;
> > +       init.num_parents = 1;
> > +
> > +       pll->hw.init = &init;
> > +
> > +       clk = clk_register(NULL, &pll->hw);
> 
> Any chance to use clk_hw based registration APIs?
> 

Fixed in the next version.

> > +       if (IS_ERR(clk))
> > +               kfree(pll);
> > +
> > +       return clk;

Stephen Boyd Nov. 7, 2018, 7:01 p.m. UTC | #3

Quoting Abel Vesa (2018-11-07 03:54:45)
> On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > +                                        unsigned long parent_rate)
> > > +{
> > > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > +       u32 val, ref, divr1, divf1, divr2, divf2;
> > > +       u64 temp64;
> > > +
> > > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > > +       switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > +       case 0:
> > > +               ref = OSC_25M;
> > > +               break;
> > > +       case 1:
> > > +               ref = OSC_27M;
> > > +               break;
> > > +       default:
> > > +               ref = OSC_25M;
> > 
> > Does this information not come through 'parent_rate'?
> > 
> 
> No. So basically both pll1 and pll2 and the divider after it form together this SCCG:
> 
> https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> 
> See: Figure 5-8. SSCG PLL Block Diagram

Thanks for the link!

> 
> We're basically reading the input of the pll 1 in order to compute the output of the entire SCCG.
> 
> I know it's a mess. I'm working on cleaning it up, but for now we need this in in order to boot up.

What's the plan to clean it up?

> 
> > > +               break;
> > > +       }
> > > +
> > > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > > +       divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > +       divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > +       divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > +       divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > +
> > > +       temp64 = ref * 2;
> > > +       temp64 *= (divf1 + 1) * (divf2 + 1);
> > > +
> > > +       do_div(temp64, (divr1 + 1) * (divr2 + 1));
> > 
> > Nitpicks: A comment with the equation may be helpful to newcomers.
> 
> Since the SCCG is contructed by multiple different types of clocks here, the equation doesn't help
> since it is spread in all constructing blocks.

Ok.

Abel Vesa Nov. 7, 2018, 8:26 p.m. UTC | #4

On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-11-07 03:54:45)
> > On Wed, Oct 17, 2018 at 12:55:52PM -0700, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2018-09-24 03:39:55)
> > > > +static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
> > > > +                                        unsigned long parent_rate)
> > > > +{
> > > > +       struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
> > > > +       u32 val, ref, divr1, divf1, divr2, divf2;
> > > > +       u64 temp64;
> > > > +
> > > > +       val = readl_relaxed(pll->base + PLL_CFG0);
> > > > +       switch (FIELD_GET(PLL_REF_MASK, val)) {
> > > > +       case 0:
> > > > +               ref = OSC_25M;
> > > > +               break;
> > > > +       case 1:
> > > > +               ref = OSC_27M;
> > > > +               break;
> > > > +       default:
> > > > +               ref = OSC_25M;
> > > 
> > > Does this information not come through 'parent_rate'?
> > > 
> > 
> > No. So basically both pll1 and pll2 and the divider after it form together this SCCG:
> > 
> > https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> > 
> > See: Figure 5-8. SSCG PLL Block Diagram
> 
> Thanks for the link!
> 
> > 
> > We're basically reading the input of the pll 1 in order to compute the output of the entire SCCG.
> > 
> > I know it's a mess. I'm working on cleaning it up, but for now we need this in in order to boot up.
> 
> What's the plan to clean it up?

So I'm doing this in our internal tree first to make sure I don't break the
other (newer) socs.

I already have a prototype in testing but it's a long way to upstream it.

Basically, I'm replacing all of this with a single, more like a composite,
more complex, clock type that does all the magic inside.

One of the problems is the fact that the bypasses can have the same sources
and in my case, I'm implementing that as a list of parents name, but the
parent names list doesn't work with duplicates, so I have to find some other
way to do it.

Once I have something clean and tested enough I'll send it upstream.

> 
> > 
> > > > +               break;
> > > > +       }
> > > > +
> > > > +       val = readl_relaxed(pll->base + PLL_CFG2);
> > > > +       divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
> > > > +       divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
> > > > +       divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
> > > > +       divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
> > > > +
> > > > +       temp64 = ref * 2;
> > > > +       temp64 *= (divf1 + 1) * (divf2 + 1);
> > > > +
> > > > +       do_div(temp64, (divr1 + 1) * (divr2 + 1));
> > > 
> > > Nitpicks: A comment with the equation may be helpful to newcomers.
> > 
> > Since the SCCG is contructed by multiple different types of clocks here, the equation doesn't help
> > since it is spread in all constructing blocks.
> 
> Ok.
>

Stephen Boyd Nov. 8, 2018, 12:18 a.m. UTC | #5

Quoting Abel Vesa (2018-11-07 12:26:25)
> On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> > 
> > 
> > What's the plan to clean it up?
> 
> So I'm doing this in our internal tree first to make sure I don't break the
> other (newer) socs.
> 
> I already have a prototype in testing but it's a long way to upstream it.
> 
> Basically, I'm replacing all of this with a single, more like a composite,
> more complex, clock type that does all the magic inside.
> 
> One of the problems is the fact that the bypasses can have the same sources
> and in my case, I'm implementing that as a list of parents name, but the
> parent names list doesn't work with duplicates, so I have to find some other
> way to do it.
> 
> Once I have something clean and tested enough I'll send it upstream.

Ok. Thanks for the info.

Abel Vesa Nov. 8, 2018, 12:29 p.m. UTC | #6

On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-11-07 12:26:25)
> > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> > > 
> > > 
> > > What's the plan to clean it up?
> > 
> > So I'm doing this in our internal tree first to make sure I don't break the
> > other (newer) socs.
> > 
> > I already have a prototype in testing but it's a long way to upstream it.
> > 
> > Basically, I'm replacing all of this with a single, more like a composite,
> > more complex, clock type that does all the magic inside.
> > 
> > One of the problems is the fact that the bypasses can have the same sources
> > and in my case, I'm implementing that as a list of parents name, but the
> > parent names list doesn't work with duplicates, so I have to find some other
> > way to do it.
> > 
> > Once I have something clean and tested enough I'll send it upstream.
> 
> Ok. Thanks for the info.
> 

Just to avoid any kind of confusion.

The whole refactoring of the SCCG clock will be done in a separate (later) change
and will not be part of this patchset.

I already sent the 12th version of this current patch series and I would really
like to get this in ASAP so that the booting up of imx8mq will not be delayed.

Thanks
Abel

Stephen Boyd Nov. 8, 2018, 6:28 p.m. UTC | #7

Quoting Abel Vesa (2018-11-08 04:29:39)
> On Wed, Nov 07, 2018 at 04:18:35PM -0800, Stephen Boyd wrote:
> > Quoting Abel Vesa (2018-11-07 12:26:25)
> > > On Wed, Nov 07, 2018 at 11:01:02AM -0800, Stephen Boyd wrote:
> > > > 
> > > > 
> > > > What's the plan to clean it up?
> > > 
> > > So I'm doing this in our internal tree first to make sure I don't break the
> > > other (newer) socs.
> > > 
> > > I already have a prototype in testing but it's a long way to upstream it.
> > > 
> > > Basically, I'm replacing all of this with a single, more like a composite,
> > > more complex, clock type that does all the magic inside.
> > > 
> > > One of the problems is the fact that the bypasses can have the same sources
> > > and in my case, I'm implementing that as a list of parents name, but the
> > > parent names list doesn't work with duplicates, so I have to find some other
> > > way to do it.
> > > 
> > > Once I have something clean and tested enough I'll send it upstream.
> > 
> > Ok. Thanks for the info.
> > 
> 
> Just to avoid any kind of confusion.
> 
> The whole refactoring of the SCCG clock will be done in a separate (later) change
> and will not be part of this patchset.
> 
> I already sent the 12th version of this current patch series and I would really
> like to get this in ASAP so that the booting up of imx8mq will not be delayed.
> 

Ok. Well we're in rc1 right now, and so we're not merging new drivers
into mainline. I can merge the clk driver into clk-next, but you'll have
to wait for the stabilization period to end (approximately 6 or 7 weeks)
before this can get into the next kernel version. It will be in
linux-next much sooner of course.

Dong Aisheng Nov. 10, 2018, 4:05 p.m. UTC | #8

Hi Stephen,

[...]
> > I already sent the 12th version of this current patch series and I
> > would really like to get this in ASAP so that the booting up of imx8mq will
> not be delayed.
> >
> 
> Ok. Well we're in rc1 right now, and so we're not merging new drivers into
> mainline. I can merge the clk driver into clk-next, but you'll have to wait for the
> stabilization period to end (approximately 6 or 7 weeks) before this can get
> into the next kernel version. It will be in linux-next much sooner of course.

That would be great if you can help that.
We're now working with SUSE to enable i.MX8 support.
Their criteria is only backporting patches which must be at least in maintainer's
next tree already. So either picked up by you or Shawn would help a lot on it.

BTW, one simple question is that because MX8MQ DTS patches depends on
this clock driver series. How would you suggest this series to go through
your tree or Shawn's tree?

Regards
Dong Aisheng

Shawn Guo Nov. 13, 2018, 2:25 p.m. UTC | #9

On Sat, Nov 10, 2018 at 04:05:44PM +0000, A.s. Dong wrote:
> Hi Stephen,
> 
> [...]
> > > I already sent the 12th version of this current patch series and I
> > > would really like to get this in ASAP so that the booting up of imx8mq will
> > not be delayed.
> > >
> > 
> > Ok. Well we're in rc1 right now, and so we're not merging new drivers into
> > mainline. I can merge the clk driver into clk-next, but you'll have to wait for the
> > stabilization period to end (approximately 6 or 7 weeks) before this can get
> > into the next kernel version. It will be in linux-next much sooner of course.
> 
> That would be great if you can help that.
> We're now working with SUSE to enable i.MX8 support.
> Their criteria is only backporting patches which must be at least in maintainer's
> next tree already. So either picked up by you or Shawn would help a lot on it.
> 
> BTW, one simple question is that because MX8MQ DTS patches depends on
> this clock driver series. How would you suggest this series to go through
> your tree or Shawn's tree?

Once Stephen has a topic branch for the patches, I can pull it into my
tree to resolve the DT dependency.

Shawn

Stephen Boyd Nov. 14, 2018, 11:21 p.m. UTC | #10

Quoting Shawn Guo (2018-11-13 06:25:36)
> On Sat, Nov 10, 2018 at 04:05:44PM +0000, A.s. Dong wrote:
> > Hi Stephen,
> > 
> > [...]
> > > > I already sent the 12th version of this current patch series and I
> > > > would really like to get this in ASAP so that the booting up of imx8mq will
> > > not be delayed.
> > > >
> > > 
> > > Ok. Well we're in rc1 right now, and so we're not merging new drivers into
> > > mainline. I can merge the clk driver into clk-next, but you'll have to wait for the
> > > stabilization period to end (approximately 6 or 7 weeks) before this can get
> > > into the next kernel version. It will be in linux-next much sooner of course.
> > 
> > That would be great if you can help that.
> > We're now working with SUSE to enable i.MX8 support.
> > Their criteria is only backporting patches which must be at least in maintainer's
> > next tree already. So either picked up by you or Shawn would help a lot on it.
> > 
> > BTW, one simple question is that because MX8MQ DTS patches depends on
> > this clock driver series. How would you suggest this series to go through
> > your tree or Shawn's tree?
> 
> Once Stephen has a topic branch for the patches, I can pull it into my
> tree to resolve the DT dependency.
> 

Sounds like a plan. Expect something next week.

Patch

diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index 4893c1f..b87513c 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -12,7 +12,8 @@  obj-y += \
 	clk-pllv1.o \
 	clk-pllv2.o \
 	clk-pllv3.o \
-	clk-pfd.o
+	clk-pfd.o \
+	clk-sccg-pll.o
 
 obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
 obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
diff --git a/drivers/clk/imx/clk-sccg-pll.c b/drivers/clk/imx/clk-sccg-pll.c
new file mode 100644
index 0000000..a9837fa
--- /dev/null
+++ b/drivers/clk/imx/clk-sccg-pll.c
@@ -0,0 +1,237 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright 2018 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/bitfield.h>
+
+#include "clk.h"
+
+/* PLL CFGs */
+#define PLL_CFG0		0x0
+#define PLL_CFG1		0x4
+#define PLL_CFG2		0x8
+
+#define PLL_DIVF1_MASK		GENMASK(18, 13)
+#define PLL_DIVF2_MASK		GENMASK(12, 7)
+#define PLL_DIVR1_MASK		GENMASK(27, 25)
+#define PLL_DIVR2_MASK		GENMASK(24, 19)
+#define PLL_REF_MASK		GENMASK(2, 0)
+
+#define PLL_LOCK_MASK		BIT(31)
+#define PLL_PD_MASK		BIT(7)
+
+#define OSC_25M			25000000
+#define OSC_27M			27000000
+
+#define PLL_SCCG_LOCK_TIMEOUT	70
+
+struct clk_sccg_pll {
+	struct clk_hw	hw;
+	void __iomem	*base;
+};
+
+#define to_clk_sccg_pll(_hw) container_of(_hw, struct clk_sccg_pll, hw)
+
+static int clk_pll_wait_lock(struct clk_sccg_pll *pll)
+{
+	u32 val;
+
+	return readl_poll_timeout(pll->base, val, val & PLL_LOCK_MASK, 0,
+					PLL_SCCG_LOCK_TIMEOUT);
+}
+
+static int clk_pll1_is_prepared(struct clk_hw *hw)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val;
+
+	val = readl_relaxed(pll->base + PLL_CFG0);
+	return (val & PLL_PD_MASK) ? 0 : 1;
+}
+
+static unsigned long clk_pll1_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val, divf;
+
+	val = readl_relaxed(pll->base + PLL_CFG2);
+	divf = FIELD_GET(PLL_DIVF1_MASK, val);
+
+	return parent_rate * 2 * (divf + 1);
+}
+
+static long clk_pll1_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *prate)
+{
+	unsigned long parent_rate = *prate;
+	u32 div;
+
+	div = rate / (parent_rate * 2);
+
+	return parent_rate * div * 2;
+}
+
+static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val;
+	u32 divf;
+
+	divf = rate / (parent_rate * 2);
+
+	val = readl_relaxed(pll->base + PLL_CFG2);
+	val &= ~PLL_DIVF1_MASK;
+	val |= FIELD_PREP(PLL_DIVF1_MASK, divf - 1);
+	writel_relaxed(val, pll->base + PLL_CFG2);
+
+	return clk_pll_wait_lock(pll);
+}
+
+static int clk_pll1_prepare(struct clk_hw *hw)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val;
+
+	val = readl_relaxed(pll->base + PLL_CFG0);
+	val &= ~PLL_PD_MASK;
+	writel_relaxed(val, pll->base + PLL_CFG0);
+
+	return clk_pll_wait_lock(pll);
+}
+
+static void clk_pll1_unprepare(struct clk_hw *hw)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val;
+
+	val = readl_relaxed(pll->base + PLL_CFG0);
+	val |= PLL_PD_MASK;
+	writel_relaxed(val, pll->base + PLL_CFG0);
+
+}
+
+static unsigned long clk_pll2_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+	u32 val, ref, divr1, divf1, divr2, divf2;
+	u64 temp64;
+
+	val = readl_relaxed(pll->base + PLL_CFG0);
+	switch (FIELD_GET(PLL_REF_MASK, val)) {
+	case 0:
+		ref = OSC_25M;
+		break;
+	case 1:
+		ref = OSC_27M;
+		break;
+	default:
+		ref = OSC_25M;
+		break;
+	}
+
+	val = readl_relaxed(pll->base + PLL_CFG2);
+	divr1 = FIELD_GET(PLL_DIVR1_MASK, val);
+	divr2 = FIELD_GET(PLL_DIVR2_MASK, val);
+	divf1 = FIELD_GET(PLL_DIVF1_MASK, val);
+	divf2 = FIELD_GET(PLL_DIVF2_MASK, val);
+
+	temp64 = ref * 2;
+	temp64 *= (divf1 + 1) * (divf2 + 1);
+
+	do_div(temp64, (divr1 + 1) * (divr2 + 1));
+
+	return (unsigned long)temp64;
+}
+
+static long clk_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *prate)
+{
+	u32 div;
+	unsigned long parent_rate = *prate;
+
+	div = rate / (parent_rate);
+
+	return parent_rate * div;
+}
+
+static int clk_pll2_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	u32 val;
+	u32 divf;
+	struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+
+	divf = rate / (parent_rate);
+
+	val = readl_relaxed(pll->base + PLL_CFG2);
+	val &= ~PLL_DIVF2_MASK;
+	val |= FIELD_PREP(PLL_DIVF2_MASK, divf - 1);
+	writel_relaxed(val, pll->base + PLL_CFG2);
+
+	return clk_pll_wait_lock(pll);
+}
+
+static const struct clk_ops clk_sccg_pll1_ops = {
+	.is_prepared	= clk_pll1_is_prepared,
+	.recalc_rate	= clk_pll1_recalc_rate,
+	.round_rate	= clk_pll1_round_rate,
+	.set_rate	= clk_pll1_set_rate,
+};
+
+static const struct clk_ops clk_sccg_pll2_ops = {
+	.prepare	= clk_pll1_prepare,
+	.unprepare	= clk_pll1_unprepare,
+	.recalc_rate	= clk_pll2_recalc_rate,
+	.round_rate	= clk_pll2_round_rate,
+	.set_rate	= clk_pll2_set_rate,
+};
+
+struct clk *imx_clk_sccg_pll(const char *name,
+				const char *parent_name,
+				void __iomem *base,
+				enum imx_sccg_pll_type pll_type)
+{
+	struct clk_sccg_pll *pll;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	pll->base = base;
+	init.name = name;
+	switch (pll_type) {
+	case SCCG_PLL1:
+		init.ops = &clk_sccg_pll1_ops;
+		break;
+	case SCCG_PLL2:
+		init.ops = &clk_sccg_pll2_ops;
+		break;
+	default:
+		kfree(pll);
+		return ERR_PTR(-EINVAL);
+	}
+
+	init.flags = 0;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	pll->hw.init = &init;
+
+	clk = clk_register(NULL, &pll->hw);
+	if (IS_ERR(clk))
+		kfree(pll);
+
+	return clk;
+}
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 13daf1c..12b3fd6 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -21,6 +21,11 @@  enum imx_pllv1_type {
 	IMX_PLLV1_IMX35,
 };
 
+enum imx_sccg_pll_type {
+	SCCG_PLL1,
+	SCCG_PLL2,
+};
+
 struct clk *imx_clk_pllv1(enum imx_pllv1_type type, const char *name,
 		const char *parent, void __iomem *base);
 
@@ -30,6 +35,10 @@  struct clk *imx_clk_pllv2(const char *name, const char *parent,
 struct clk *imx_clk_frac_pll(const char *name, const char *parent_name,
 			     void __iomem *base);
 
+struct clk *imx_clk_sccg_pll(const char *name, const char *parent_name,
+			     void __iomem *base,
+			     enum imx_sccg_pll_type pll_type);
+
 enum imx_pllv3_type {
 	IMX_PLLV3_GENERIC,
 	IMX_PLLV3_SYS,