[v2,3/4] clk: imx: pll14xx: support spread spectrum clock generation

Commit Message

Peng Fan Feb. 5, 2025, 9:49 a.m. UTC

From: Peng Fan <[e-mail]>

Add support for spread spectrum clock (SSC) generation to the pll14xxx
driver.

Co-developed-by: Dario Binacchi <[e-mail]>
Signed-off-by: Dario Binacchi <[e-mail]>
Signed-off-by: Peng Fan <[e-mail]>
---
 drivers/clk/imx/clk-pll14xx.c | 66 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

Dario Binacchi Feb. 5, 2025, 11:19 a.m. UTC | #1

Hi Peng,

On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS) <[e-mail]> wrote:
>
> From: Peng Fan <[e-mail]>
>
> Add support for spread spectrum clock (SSC) generation to the pll14xxx
> driver.
>
> Co-developed-by: Dario Binacchi <[e-mail]>
> Signed-off-by: Dario Binacchi <[e-mail]>
> Signed-off-by: Peng Fan <[e-mail]>

It doesn’t seem right to me.
You can’t take 90% of my patch, where the SSC management was actually
implemented,
add 10%, and consider yourself the author of the patch.
Please correct it in version 3.

Thanks and regards,
Dario

> ---
>  drivers/clk/imx/clk-pll14xx.c | 66 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb048dcbf58db396af9d3aaf 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -20,6 +20,8 @@
>  #define GNRL_CTL       0x0
>  #define DIV_CTL0       0x4
>  #define DIV_CTL1       0x8
> +#define SSCG_CTRL      0xc
> +
>  #define LOCK_STATUS    BIT(31)
>  #define LOCK_SEL_MASK  BIT(29)
>  #define CLKE_MASK      BIT(11)
> @@ -31,15 +33,26 @@
>  #define KDIV_MASK      GENMASK(15, 0)
>  #define KDIV_MIN       SHRT_MIN
>  #define KDIV_MAX       SHRT_MAX
> +#define SSCG_ENABLE    BIT(31)
> +#define MFREQ_CTL_MASK GENMASK(19, 12)
> +#define MRAT_CTL_MASK  GENMASK(9, 4)
> +#define SEL_PF_MASK    GENMASK(1, 0)
>
>  #define LOCK_TIMEOUT_US                10000
>
> +enum imx_pll14xx_ssc_mod_type {
> +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> +       IMX_PLL14XX_SSC_UP_SPREAD,
> +       IMX_PLL14XX_SSC_CENTER_SPREAD,
> +};
> +
>  struct clk_pll14xx {
>         struct clk_hw                   hw;
>         void __iomem                    *base;
>         enum imx_pll14xx_type           type;
>         const struct imx_pll14xx_rate_table *rate_table;
>         int rate_count;
> +       struct clk_spread_spectrum      ssc_conf;
>  };
>
>  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
>         return 0;
>  }
>
> +static void clk_pll1443x_enable_ssc(struct clk_hw *hw, unsigned long parent_rate,
> +                                   unsigned int pdiv, unsigned int mdiv)
> +{
> +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +       struct clk_spread_spectrum *conf = &pll->ssc_conf;
> +       u32 sscg_ctrl, mfr, mrr, mod_type;
> +
> +       sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
> +       sscg_ctrl &=
> +               ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
> +
> +       mfr = parent_rate / (conf->modfreq * pdiv * (1 << 5));
> +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 * mfr);
> +
> +       switch (conf->method) {
> +       case CLK_SSC_CENTER_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> +               break;
> +       case CLK_SSC_UP_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> +               break;
> +       case CLK_SSC_DOWN_SPREAD:
> +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> +               break;
> +       default:
> +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> +               break;
> +       }
> +
> +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
> +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> +               FIELD_PREP(SEL_PF_MASK, mod_type);
> +
> +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
> +}
> +
>  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>                                  unsigned long prate)
>  {
> @@ -370,6 +419,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>                 writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
>                                pll->base + DIV_CTL1);
>
> +               if (pll->ssc_conf.enable)
> +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
> +
>                 return 0;
>         }
>
> @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
>         gnrl_ctl &= ~BYPASS_MASK;
>         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>
> +       if (pll->ssc_conf.enable)
> +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
> +
>         return 0;
>  }
>
> @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct clk_hw *hw)
>         writel_relaxed(val, pll->base + GNRL_CTL);
>  }
>
> +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +                                           struct clk_spread_spectrum *clk_ss)
> +{
> +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +
> +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> +
> +       return 0;
> +}
> +
>  static const struct clk_ops clk_pll1416x_ops = {
>         .prepare        = clk_pll14xx_prepare,
>         .unprepare      = clk_pll14xx_unprepare,
> @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops = {
>         .recalc_rate    = clk_pll14xx_recalc_rate,
>         .round_rate     = clk_pll1443x_round_rate,
>         .set_rate       = clk_pll1443x_set_rate,
> +       .set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
>  };
>
>  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
>
> --
> 2.37.1
>

Peng Fan Feb. 6, 2025, 12:53 a.m. UTC | #2

> Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
> clock generation
> 
> Hi Peng,
> 
> On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
> <[e-mail]> wrote:
> >
> > From: Peng Fan <[e-mail]>
> >
> > Add support for spread spectrum clock (SSC) generation to the
> pll14xxx
> > driver.
> >
> > Co-developed-by: Dario Binacchi
> <[e-mail]>
> > Signed-off-by: Dario Binacchi <[e-mail]>
> > Signed-off-by: Peng Fan <[e-mail]>
> 
> It doesn’t seem right to me.
> You can’t take 90% of my patch, where the SSC management was
> actually implemented, add 10%, and consider yourself the author of
> the patch.
> Please correct it in version 3.

Ah. But if you look into the patches, 10% is not accurate
per lines I change.
you could see more changes compared with your patch
https://lore.kernel.org/all/[e-mail]/.

1. Use set_spread_spectrm ops
2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
 count over 50% changes. 

The logic that I only did minor update is the function
clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum

If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
I take this patch in the patchset, mainly to ease your work and make
assigned-clock-sscs moving forward, considering SCMI spec needs update
(clk-scmi.c changes will not land soon).

Regards
Peng.

> 
> Thanks and regards,
> Dario
> 
> > ---
> >  drivers/clk/imx/clk-pll14xx.c | 66
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-pll14xx.c
> > b/drivers/clk/imx/clk-pll14xx.c index
> >
> f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
> 048dcbf58d
> > b396af9d3aaf 100644
> > --- a/drivers/clk/imx/clk-pll14xx.c
> > +++ b/drivers/clk/imx/clk-pll14xx.c
> > @@ -20,6 +20,8 @@
> >  #define GNRL_CTL       0x0
> >  #define DIV_CTL0       0x4
> >  #define DIV_CTL1       0x8
> > +#define SSCG_CTRL      0xc
> > +
> >  #define LOCK_STATUS    BIT(31)
> >  #define LOCK_SEL_MASK  BIT(29)
> >  #define CLKE_MASK      BIT(11)
> > @@ -31,15 +33,26 @@
> >  #define KDIV_MASK      GENMASK(15, 0)
> >  #define KDIV_MIN       SHRT_MIN
> >  #define KDIV_MAX       SHRT_MAX
> > +#define SSCG_ENABLE    BIT(31)
> > +#define MFREQ_CTL_MASK GENMASK(19, 12) #define
> MRAT_CTL_MASK
> > +GENMASK(9, 4)
> > +#define SEL_PF_MASK    GENMASK(1, 0)
> >
> >  #define LOCK_TIMEOUT_US                10000
> >
> > +enum imx_pll14xx_ssc_mod_type {
> > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> > +       IMX_PLL14XX_SSC_UP_SPREAD,
> > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
> > +
> >  struct clk_pll14xx {
> >         struct clk_hw                   hw;
> >         void __iomem                    *base;
> >         enum imx_pll14xx_type           type;
> >         const struct imx_pll14xx_rate_table *rate_table;
> >         int rate_count;
> > +       struct clk_spread_spectrum      ssc_conf;
> >  };
> >
> >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
> clk_hw *hw, unsigned long drate,
> >         return 0;
> >  }
> >
> > +static void clk_pll1443x_enable_ssc(struct clk_hw *hw, unsigned
> long parent_rate,
> > +                                   unsigned int pdiv, unsigned int
> > +mdiv) {
> > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > +       struct clk_spread_spectrum *conf = &pll->ssc_conf;
> > +       u32 sscg_ctrl, mfr, mrr, mod_type;
> > +
> > +       sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
> > +       sscg_ctrl &=
> > +               ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK |
> > + SEL_PF_MASK);
> > +
> > +       mfr = parent_rate / (conf->modfreq * pdiv * (1 << 5));
> > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
> > + mfr);
> > +
> > +       switch (conf->method) {
> > +       case CLK_SSC_CENTER_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> > +               break;
> > +       case CLK_SSC_UP_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> > +               break;
> > +       case CLK_SSC_DOWN_SPREAD:
> > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > +               break;
> > +       default:
> > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > +               break;
> > +       }
> > +
> > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
> mfr) |
> > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> > +               FIELD_PREP(SEL_PF_MASK, mod_type);
> > +
> > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
> > +
> >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
> drate,
> >                                  unsigned long prate)  { @@ -370,6
> > +419,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw,
> unsigned long drate,
> >                 writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
> >                                pll->base + DIV_CTL1);
> >
> > +               if (pll->ssc_conf.enable)
> > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > + rate.mdiv);
> > +
> >                 return 0;
> >         }
> >
> > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
> clk_hw *hw, unsigned long drate,
> >         gnrl_ctl &= ~BYPASS_MASK;
> >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
> >
> > +       if (pll->ssc_conf.enable)
> > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > + rate.mdiv);
> > +
> >         return 0;
> >  }
> >
> > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
> clk_hw *hw)
> >         writel_relaxed(val, pll->base + GNRL_CTL);  }
> >
> > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> > +                                           struct clk_spread_spectrum
> > +*clk_ss) {
> > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > +
> > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct clk_ops clk_pll1416x_ops = {
> >         .prepare        = clk_pll14xx_prepare,
> >         .unprepare      = clk_pll14xx_unprepare,
> > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
> = {
> >         .recalc_rate    = clk_pll14xx_recalc_rate,
> >         .round_rate     = clk_pll1443x_round_rate,
> >         .set_rate       = clk_pll1443x_set_rate,
> > +       .set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
> >  };
> >
> >  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const
> char
> > *name,
> >
> > --
> > 2.37.1
> >
> 
> 
> --
> 
> Dario Binacchi
> 
> Senior Embedded Linux Developer
> 
> [e-mail]
> 
> __________________________________
> 
> 
> Amarula Solutions SRL
> 
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> 
> T. [phone]
> [e-mail]
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
> wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
> wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
> 8%3D&reserved=0

Dario Binacchi Feb. 6, 2025, 3:31 p.m. UTC | #3

Hi Peng,

On Thu, Feb 6, 2025 at 1:53 AM Peng Fan <[e-mail]> wrote:
>
> > Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
> > clock generation
> >
> > Hi Peng,
> >
> > On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
> > <[e-mail]> wrote:
> > >
> > > From: Peng Fan <[e-mail]>
> > >
> > > Add support for spread spectrum clock (SSC) generation to the
> > pll14xxx
> > > driver.
> > >
> > > Co-developed-by: Dario Binacchi
> > <[e-mail]>
> > > Signed-off-by: Dario Binacchi <[e-mail]>
> > > Signed-off-by: Peng Fan <[e-mail]>
> >
> > It doesn’t seem right to me.
> > You can’t take 90% of my patch, where the SSC management was
> > actually implemented, add 10%, and consider yourself the author of
> > the patch.
> > Please correct it in version 3.
>
> Ah. But if you look into the patches, 10% is not accurate
> per lines I change.
> you could see more changes compared with your patch
> https://lore.kernel.org/all/[e-mail]/.
>
> 1. Use set_spread_spectrm ops
> 2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
> 3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
>  count over 50% changes.
>
> The logic that I only did minor update is the function
> clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum

Sorry if I miscounted the lines, but here we are not considering who
actually implemented
the algorithmic part of the SSC management and all the time spent
testing the code on more
than one platform/board with each submission of the series for all 9 versions.

[1] https://lore.kernel.org/all/[e-mail]/

Your changes, which are unnecessary for the clk-scmi.c changes, only
serve to support the
DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:

https://github.com/devicetree-org/dt-schema/pull/154

you should have proposed during the review of series [1]. You are the
NXP reviewer.

>
> If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
> I take this patch in the patchset, mainly to ease your work and make

Sorry for quoting Krzysztof again, but:
"Three months iMX8 patchsets, multiple reviews and no single comment
from you till January!"

So please, if you really want to ease my work, then remove this patch
from this series and resume
reviewing series [1].

Thanks and regards,
Dario

> assigned-clock-sscs moving forward, considering SCMI spec needs update
> (clk-scmi.c changes will not land soon).
>
> Regards
> Peng.
>
> >
> > Thanks and regards,
> > Dario
> >
> > > ---
> > >  drivers/clk/imx/clk-pll14xx.c | 66
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-pll14xx.c
> > > b/drivers/clk/imx/clk-pll14xx.c index
> > >
> > f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
> > 048dcbf58d
> > > b396af9d3aaf 100644
> > > --- a/drivers/clk/imx/clk-pll14xx.c
> > > +++ b/drivers/clk/imx/clk-pll14xx.c
> > > @@ -20,6 +20,8 @@
> > >  #define GNRL_CTL       0x0
> > >  #define DIV_CTL0       0x4
> > >  #define DIV_CTL1       0x8
> > > +#define SSCG_CTRL      0xc
> > > +
> > >  #define LOCK_STATUS    BIT(31)
> > >  #define LOCK_SEL_MASK  BIT(29)
> > >  #define CLKE_MASK      BIT(11)
> > > @@ -31,15 +33,26 @@
> > >  #define KDIV_MASK      GENMASK(15, 0)
> > >  #define KDIV_MIN       SHRT_MIN
> > >  #define KDIV_MAX       SHRT_MAX
> > > +#define SSCG_ENABLE    BIT(31)
> > > +#define MFREQ_CTL_MASK GENMASK(19, 12) #define
> > MRAT_CTL_MASK
> > > +GENMASK(9, 4)
> > > +#define SEL_PF_MASK    GENMASK(1, 0)
> > >
> > >  #define LOCK_TIMEOUT_US                10000
> > >
> > > +enum imx_pll14xx_ssc_mod_type {
> > > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
> > > +       IMX_PLL14XX_SSC_UP_SPREAD,
> > > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
> > > +
> > >  struct clk_pll14xx {
> > >         struct clk_hw                   hw;
> > >         void __iomem                    *base;
> > >         enum imx_pll14xx_type           type;
> > >         const struct imx_pll14xx_rate_table *rate_table;
> > >         int rate_count;
> > > +       struct clk_spread_spectrum      ssc_conf;
> > >  };
> > >
> > >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
> > > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
> > clk_hw *hw, unsigned long drate,
> > >         return 0;
> > >  }
> > >
> > > +static void clk_pll1443x_enable_ssc(struct clk_hw *hw, unsigned
> > long parent_rate,
> > > +                                   unsigned int pdiv, unsigned int
> > > +mdiv) {
> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > > +       struct clk_spread_spectrum *conf = &pll->ssc_conf;
> > > +       u32 sscg_ctrl, mfr, mrr, mod_type;
> > > +
> > > +       sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
> > > +       sscg_ctrl &=
> > > +               ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK |
> > > + SEL_PF_MASK);
> > > +
> > > +       mfr = parent_rate / (conf->modfreq * pdiv * (1 << 5));
> > > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
> > > + mfr);
> > > +
> > > +       switch (conf->method) {
> > > +       case CLK_SSC_CENTER_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
> > > +               break;
> > > +       case CLK_SSC_UP_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
> > > +               break;
> > > +       case CLK_SSC_DOWN_SPREAD:
> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > > +               break;
> > > +       default:
> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
> > > +               break;
> > > +       }
> > > +
> > > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
> > mfr) |
> > > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
> > > +               FIELD_PREP(SEL_PF_MASK, mod_type);
> > > +
> > > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
> > > +
> > >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
> > drate,
> > >                                  unsigned long prate)  { @@ -370,6
> > > +419,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw,
> > unsigned long drate,
> > >                 writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
> > >                                pll->base + DIV_CTL1);
> > >
> > > +               if (pll->ssc_conf.enable)
> > > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > > + rate.mdiv);
> > > +
> > >                 return 0;
> > >         }
> > >
> > > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
> > clk_hw *hw, unsigned long drate,
> > >         gnrl_ctl &= ~BYPASS_MASK;
> > >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
> > >
> > > +       if (pll->ssc_conf.enable)
> > > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
> > > + rate.mdiv);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
> > clk_hw *hw)
> > >         writel_relaxed(val, pll->base + GNRL_CTL);  }
> > >
> > > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> > > +                                           struct clk_spread_spectrum
> > > +*clk_ss) {
> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> > > +
> > > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static const struct clk_ops clk_pll1416x_ops = {
> > >         .prepare        = clk_pll14xx_prepare,
> > >         .unprepare      = clk_pll14xx_unprepare,
> > > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
> > = {
> > >         .recalc_rate    = clk_pll14xx_recalc_rate,
> > >         .round_rate     = clk_pll1443x_round_rate,
> > >         .set_rate       = clk_pll1443x_set_rate,
> > > +       .set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
> > >  };
> > >
> > >  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const
> > char
> > > *name,
> > >
> > > --
> > > 2.37.1
> > >
> >
> >
> > --
> >
> > Dario Binacchi
> >
> > Senior Embedded Linux Developer
> >
> > [e-mail]
> >
> > __________________________________
> >
> >
> > Amarula Solutions SRL
> >
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> >
> > T. [phone]
> > [e-mail]
> >
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> > com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
> > fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
> > wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
> > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> > %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
> > 8%3D&reserved=0

Sudeep Holla Feb. 6, 2025, 4:16 p.m. UTC | #4

Hi Peng,

I apologise in advance for exploiting this thread to make my point.

On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>
> Sorry if I miscounted the lines, but here we are not considering who
> actually implemented
> the algorithmic part of the SSC management and all the time spent
> testing the code on more
> than one platform/board with each submission of the series for all 9 versions.
>
> [1] https://lore.kernel.org/all/[e-mail]/
>
> Your changes, which are unnecessary for the clk-scmi.c changes, only
> serve to support the
> DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>
> https://github.com/devicetree-org/dt-schema/pull/154
>
> you should have proposed during the review of series [1]. You are the
> NXP reviewer.
>
> >
> > If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
> > I take this patch in the patchset, mainly to ease your work and make
>
> Sorry for quoting Krzysztof again, but:
> "Three months iMX8 patchsets, multiple reviews and no single comment
> from you till January!"
>
> So please, if you really want to ease my work, then remove this patch
> from this series and resume
> reviewing series [1].
>

I had complained once in the past. I am repeating that again. You are not
new to the kernel development, yet at times I get really surprised with
the way you manage your patches and create so much confusion. It gets
extremely difficult to track what is happening if one doesn't follow all
your patches for a week(week is too lenient IMO, you manage sometime to
create same amount of confusion in just 2 days).

And as usually you ignore merge window and post a whole set of new series
on the first day of merge window. Which is fine especially if you are new
to kernel development(not true in your case though) or even otherwise if
you don't regularly track upstream cycle so much because of corporate
commitments(which may be true in your case and I am fine with that). But
you need to wait at-least a few days after the merge window so you give
every one a chance to follow your work.

And in this case, I would have avoided scmi changes are you have non-scmi
specific driver to get the core clock changes review first and then added
SCMI as it is OEM specific and we need to analyse it without other things
in flux or under discussion.

--
Regards,
Sudeep

Peng Fan Feb. 7, 2025, 10:42 a.m. UTC | #5

Hi,

On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>Hi Peng,
>
>On Thu, Feb 6, 2025 at 1:53 AM Peng Fan <[e-mail]> wrote:
>>
>> > Subject: Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum
>> > clock generation
>> >
>> > Hi Peng,
>> >
>> > On Wed, Feb 5, 2025 at 10:51 AM Peng Fan (OSS)
>> > <[e-mail]> wrote:
>> > >
>> > > From: Peng Fan <[e-mail]>
>> > >
>> > > Add support for spread spectrum clock (SSC) generation to the
>> > pll14xxx
>> > > driver.
>> > >
>> > > Co-developed-by: Dario Binacchi
>> > <[e-mail]>
>> > > Signed-off-by: Dario Binacchi <[e-mail]>
>> > > Signed-off-by: Peng Fan <[e-mail]>
>> >
>> > It doesn’t seem right to me.
>> > You can’t take 90% of my patch, where the SSC management was
>> > actually implemented, add 10%, and consider yourself the author of
>> > the patch.
>> > Please correct it in version 3.
>>
>> Ah. But if you look into the patches, 10% is not accurate
>> per lines I change.
>> you could see more changes compared with your patch
>> https://lore.kernel.org/all/[e-mail]/.
>>
>> 1. Use set_spread_spectrm ops
>> 2. Use clk_spread_spectrum to replace imx_pll14xx_ssc
>> 3. Drop imx_clk_pll14xx_ssc_parse_dt and clk_pll14xx_ssc_mod_type. This one would
>>  count over 50% changes.
>>
>> The logic that I only did minor update is the function
>> clk_pll1443x_enable_ssc with switching to use clk_spread_sectrum
>
>Sorry if I miscounted the lines, but here we are not considering who
>actually implemented
>the algorithmic part of the SSC management and all the time spent
>testing the code on more
>than one platform/board with each submission of the series for all 9 versions.
>
>[1] https://lore.kernel.org/all/[e-mail]/
>
>Your changes, which are unnecessary for the clk-scmi.c changes, only
>serve to support the
>DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>
>https://github.com/devicetree-org/dt-schema/pull/154
>
>you should have proposed during the review of series [1]. You are the
>NXP reviewer.
>
>>
>> If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
>> I take this patch in the patchset, mainly to ease your work and make
>
>Sorry for quoting Krzysztof again, but:
>"Three months iMX8 patchsets, multiple reviews and no single comment
>from you till January!"


Sigh! I feel depressed, frustrated on such conclusion.

My previous reviewing work got ignored.

For the i.MX clk patches that changes dt-bindings. Only when the dt-binding
patches got R-b/A-b or not have big issues, I start to review the driver
changes.

So in your V1/V2, I not engage, because Krzysztof has major
comments on your bindings that needs big driver changes.

In you V3,
Krzysztof is still not happy with the binding part, so I start to propose
a potential solution to split anatop and ccm. This is in 2024 Nov-8th which is
two days just after you posted the patchset.

In your V4, you still have binding changes required from Krzysztof.
In your V6, after you got R-b/A-b from Krzysztof and addressed some
binding comments, I provided my R-b for some patches. This is in 2024 Dec-24th.

In your V8, you got my R-b for all the changes related to driver changes.
This is 2024 Jan-6th, which is 7 days after you posted the patchset.

In your V9, you added extra 5 patches. I not continue my reviewing for
the extra 5 patches.

>
>So please, if you really want to ease my work, then remove this patch
>from this series and resume
>reviewing series [1].

Krzysztof raised that "assigned-clock-sscs" makes the vendor properties
legacy, so I will not review the extra 5 patches unless the
'assigned-clock-sscs' stuff got rejected. Sorry.

I had similar situation that my access-controller v8 patch got a comment
that needs big changes. Complain does not make things good. This may not
be common, but it could suddenly jump in.

So please not blame me.

I will drop this patch in future version of this patchset.

Thanks,
Peng.

>
>Thanks and regards,
>Dario
>
>> assigned-clock-sscs moving forward, considering SCMI spec needs update
>> (clk-scmi.c changes will not land soon).
>>
>> Regards
>> Peng.
>>
>> >
>> > Thanks and regards,
>> > Dario
>> >
>> > > ---
>> > >  drivers/clk/imx/clk-pll14xx.c | 66
>> > > +++++++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 66 insertions(+)
>> > >
>> > > diff --git a/drivers/clk/imx/clk-pll14xx.c
>> > > b/drivers/clk/imx/clk-pll14xx.c index
>> > >
>> > f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb
>> > 048dcbf58d
>> > > b396af9d3aaf 100644
>> > > --- a/drivers/clk/imx/clk-pll14xx.c
>> > > +++ b/drivers/clk/imx/clk-pll14xx.c
>> > > @@ -20,6 +20,8 @@
>> > >  #define GNRL_CTL       0x0
>> > >  #define DIV_CTL0       0x4
>> > >  #define DIV_CTL1       0x8
>> > > +#define SSCG_CTRL      0xc
>> > > +
>> > >  #define LOCK_STATUS    BIT(31)
>> > >  #define LOCK_SEL_MASK  BIT(29)
>> > >  #define CLKE_MASK      BIT(11)
>> > > @@ -31,15 +33,26 @@
>> > >  #define KDIV_MASK      GENMASK(15, 0)
>> > >  #define KDIV_MIN       SHRT_MIN
>> > >  #define KDIV_MAX       SHRT_MAX
>> > > +#define SSCG_ENABLE    BIT(31)
>> > > +#define MFREQ_CTL_MASK GENMASK(19, 12) #define
>> > MRAT_CTL_MASK
>> > > +GENMASK(9, 4)
>> > > +#define SEL_PF_MASK    GENMASK(1, 0)
>> > >
>> > >  #define LOCK_TIMEOUT_US                10000
>> > >
>> > > +enum imx_pll14xx_ssc_mod_type {
>> > > +       IMX_PLL14XX_SSC_DOWN_SPREAD,
>> > > +       IMX_PLL14XX_SSC_UP_SPREAD,
>> > > +       IMX_PLL14XX_SSC_CENTER_SPREAD, };
>> > > +
>> > >  struct clk_pll14xx {
>> > >         struct clk_hw                   hw;
>> > >         void __iomem                    *base;
>> > >         enum imx_pll14xx_type           type;
>> > >         const struct imx_pll14xx_rate_table *rate_table;
>> > >         int rate_count;
>> > > +       struct clk_spread_spectrum      ssc_conf;
>> > >  };
>> > >
>> > >  #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
>> > > @@ -349,6 +362,42 @@ static int clk_pll1416x_set_rate(struct
>> > clk_hw *hw, unsigned long drate,
>> > >         return 0;
>> > >  }
>> > >
>> > > +static void clk_pll1443x_enable_ssc(struct clk_hw *hw, unsigned
>> > long parent_rate,
>> > > +                                   unsigned int pdiv, unsigned int
>> > > +mdiv) {
>> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
>> > > +       struct clk_spread_spectrum *conf = &pll->ssc_conf;
>> > > +       u32 sscg_ctrl, mfr, mrr, mod_type;
>> > > +
>> > > +       sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
>> > > +       sscg_ctrl &=
>> > > +               ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK |
>> > > + SEL_PF_MASK);
>> > > +
>> > > +       mfr = parent_rate / (conf->modfreq * pdiv * (1 << 5));
>> > > +       mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 *
>> > > + mfr);
>> > > +
>> > > +       switch (conf->method) {
>> > > +       case CLK_SSC_CENTER_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
>> > > +               break;
>> > > +       case CLK_SSC_UP_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
>> > > +               break;
>> > > +       case CLK_SSC_DOWN_SPREAD:
>> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
>> > > +               break;
>> > > +       default:
>> > > +               mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
>> > > +               break;
>> > > +       }
>> > > +
>> > > +       sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK,
>> > mfr) |
>> > > +               FIELD_PREP(MRAT_CTL_MASK, mrr) |
>> > > +               FIELD_PREP(SEL_PF_MASK, mod_type);
>> > > +
>> > > +       writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL); }
>> > > +
>> > >  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long
>> > drate,
>> > >                                  unsigned long prate)  { @@ -370,6
>> > > +419,9 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw,
>> > unsigned long drate,
>> > >                 writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
>> > >                                pll->base + DIV_CTL1);
>> > >
>> > > +               if (pll->ssc_conf.enable)
>> > > +                       clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
>> > > + rate.mdiv);
>> > > +
>> > >                 return 0;
>> > >         }
>> > >
>> > > @@ -410,6 +462,9 @@ static int clk_pll1443x_set_rate(struct
>> > clk_hw *hw, unsigned long drate,
>> > >         gnrl_ctl &= ~BYPASS_MASK;
>> > >         writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
>> > >
>> > > +       if (pll->ssc_conf.enable)
>> > > +               clk_pll1443x_enable_ssc(hw, prate, rate.pdiv,
>> > > + rate.mdiv);
>> > > +
>> > >         return 0;
>> > >  }
>> > >
>> > > @@ -465,6 +520,16 @@ static void clk_pll14xx_unprepare(struct
>> > clk_hw *hw)
>> > >         writel_relaxed(val, pll->base + GNRL_CTL);  }
>> > >
>> > > +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
>> > > +                                           struct clk_spread_spectrum
>> > > +*clk_ss) {
>> > > +       struct clk_pll14xx *pll = to_clk_pll14xx(hw);
>> > > +
>> > > +       memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > >  static const struct clk_ops clk_pll1416x_ops = {
>> > >         .prepare        = clk_pll14xx_prepare,
>> > >         .unprepare      = clk_pll14xx_unprepare,
>> > > @@ -485,6 +550,7 @@ static const struct clk_ops clk_pll1443x_ops
>> > = {
>> > >         .recalc_rate    = clk_pll14xx_recalc_rate,
>> > >         .round_rate     = clk_pll1443x_round_rate,
>> > >         .set_rate       = clk_pll1443x_set_rate,
>> > > +       .set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
>> > >  };
>> > >
>> > >  struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const
>> > char
>> > > *name,
>> > >
>> > > --
>> > > 2.37.1
>> > >
>> >
>> >
>> > --
>> >
>> > Dario Binacchi
>> >
>> > Senior Embedded Linux Developer
>> >
>> > [e-mail]
>> >
>> > __________________________________
>> >
>> >
>> > Amarula Solutions SRL
>> >
>> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
>> >
>> > T. [phone]
>> > [e-mail]
>> >
>> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
>> > www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
>> > com%7Cbeaf5bdcc6694a5a1a1708dd45d701db%7C686ea1d3bc2b4c6
>> > fa92cd99c5c301635%7C0%7C0%7C638743511953067272%7CUnkno
>> > wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
>> > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
>> > %7C%7C&sdata=UFgy1bS7QJ7qenzKFTkPBNfOGn0V89CGR9NLOBka0U
>> > 8%3D&reserved=0
>
>
>
>-- 
>
>Dario Binacchi
>
>Senior Embedded Linux Developer
>
>[e-mail]
>
>__________________________________
>
>
>Amarula Solutions SRL
>
>Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
>T. [phone]
>[e-mail]
>
>www.amarulasolutions.com

Peng Fan Feb. 7, 2025, 11:26 a.m. UTC | #6

Hi Sudeep,

On Thu, Feb 06, 2025 at 04:16:58PM +0000, Sudeep Holla wrote:
>Hi Peng,
>
>I apologise in advance for exploiting this thread to make my point.
>
>On Thu, Feb 06, 2025 at 04:31:46PM +0100, Dario Binacchi wrote:
>>
>> Sorry if I miscounted the lines, but here we are not considering who
>> actually implemented
>> the algorithmic part of the SSC management and all the time spent
>> testing the code on more
>> than one platform/board with each submission of the series for all 9 versions.
>>
>> [1] https://lore.kernel.org/all/[e-mail]/
>>
>> Your changes, which are unnecessary for the clk-scmi.c changes, only
>> serve to support the
>> DT binding `assigned-clock-sscs`, which, as Krzysztof also reiterated:
>>
>> https://github.com/devicetree-org/dt-schema/pull/154
>>
>> you should have proposed during the review of series [1]. You are the
>> NXP reviewer.
>>
>> >
>> > If you think it is not fair, I could drop this patch in V3 and leave it to you to handle.
>> > I take this patch in the patchset, mainly to ease your work and make
>>
>> Sorry for quoting Krzysztof again, but:
>> "Three months iMX8 patchsets, multiple reviews and no single comment
>> from you till January!"
>>
>> So please, if you really want to ease my work, then remove this patch
>> from this series and resume
>> reviewing series [1].
>>
>
>I had complained once in the past. I am repeating that again. You are not
>new to the kernel development, yet at times I get really surprised with
>the way you manage your patches and create so much confusion. It gets
>extremely difficult to track what is happening if one doesn't follow all
>your patches for a week(week is too lenient IMO, you manage sometime to
>create same amount of confusion in just 2 days).

V2 is actually 2 weeks after V1. So after addressing the comments
from Stephen and Dan, also updated clk-scmi.c to use a non-vendor
changes, I posted out V2.

2 days, this is just after got Cristian's comments. Then I posted V2.
I try to follow your working style on handling scmi patches, but seems you are
not active, so I mainly count on Cristian's comments and update patches.

The i.MX pll patches in V2 is orthogonal to clk scmi, I did not expect
complains. But ...

>
>And as usually you ignore merge window and post a whole set of new series
>on the first day of merge window. Which is fine especially if you are new
>to kernel development(not true in your case though) or even otherwise if
>you don't regularly track upstream cycle so much because of corporate
>commitments(which may be true in your case and I am fine with that). But
>you need to wait at-least a few days after the merge window so you give
>every one a chance to follow your work.

In my view, maintainers have patchwork to maintain patches. patches send
out in merge window will not be reviewed in short time or surely not
picked up, I understand this. patches could just be marked new in patchwork.
If new version is out, old version just marked as not apply.
And I use b4 to manage patchset, and each revision has changelog.

Indeed I not track merge window since I am not maintainer role. I was
not aware this would introduce complain (: I will track the cycle
in following days.

>
>And in this case, I would have avoided scmi changes are you have non-scmi
>specific driver to get the core clock changes review first and then added
>SCMI as it is OEM specific and we need to analyse it without other things
>in flux or under discussion.

the pll changes is orthogonal to clk scmi.

it is just like common changes + several driver changes in a patchset
and this is just an early version (V2). Such as people use devm_x to 
simplify various drivers.
1. Introduce devm_x
2. driver1: use devm_x
3. driver2: use devm_x

2 and 3 not impact each other.

My initial idea to introduce pll changes in V2 is to ease Dario's work,
but seems not. Not expect that would introduce confusion to you.

The goal is still to use assigned-clock-sscs and let clk scmi support 
spread spectrum.

Thanks,
Peng.

>
>--
>Regards,
>Sudeep

Sudeep Holla Feb. 7, 2025, 1:14 p.m. UTC | #7

On Fri, Feb 07, 2025 at 07:26:22PM +0800, Peng Fan wrote:
> Hi Sudeep,
>
> V2 is actually 2 weeks after V1. So after addressing the comments
> from Stephen and Dan, also updated clk-scmi.c to use a non-vendor
> changes, I posted out V2.
>

Sure, but as I said you posted on the very first day of the merge window.
So 2 weeks just cover the end of merge window.

> 2 days, this is just after got Cristian's comments. Then I posted V2.
> I try to follow your working style on handling scmi patches, but seems you are
> not active, so I mainly count on Cristian's comments and update patches.
>

Yes, his comments were for more discussions internally and externally.
Not to churn up another patch set.

> The i.MX pll patches in V2 is orthogonal to clk scmi, I did not expect
> complains. But ...
>

Sorry if I overlooked, but with not all the platform specific
knowledge it is just too much info to consume at once. Again it is
fine if you don't make it hard but churning newer versions. So please
give time.

> In my view, maintainers have patchwork to maintain patches. patches send
> out in merge window will not be reviewed in short time or surely not
> picked up, I understand this. patches could just be marked new in patchwork.
> If new version is out, old version just marked as not apply.

Though I don't use patchwork(probably I should not your problem).
However, sometimes I see all versions to understand the changes and
evolution sometimes. And it just gets hard if there are too many
versions in short duration.

> And I use b4 to manage patchset, and each revision has changelog.
>

Good.

> Indeed I not track merge window since I am not maintainer role. I was
> not aware this would introduce complain (: I will track the cycle
> in following days.
>

I don't say it is a must. But good if you manage to.

I will look at all the pending patches from you soon, give me until
middle of next week.

--
Regards,
Sudeep

Patch

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index f290981ea13bdba3602af7aa44aaadfe0b78dcf9..3bdce762a9d651a6fb048dcbf58db396af9d3aaf 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -20,6 +20,8 @@ 
 #define GNRL_CTL	0x0
 #define DIV_CTL0	0x4
 #define DIV_CTL1	0x8
+#define SSCG_CTRL	0xc
+
 #define LOCK_STATUS	BIT(31)
 #define LOCK_SEL_MASK	BIT(29)
 #define CLKE_MASK	BIT(11)
@@ -31,15 +33,26 @@ 
 #define KDIV_MASK	GENMASK(15, 0)
 #define KDIV_MIN	SHRT_MIN
 #define KDIV_MAX	SHRT_MAX
+#define SSCG_ENABLE	BIT(31)
+#define MFREQ_CTL_MASK	GENMASK(19, 12)
+#define MRAT_CTL_MASK	GENMASK(9, 4)
+#define SEL_PF_MASK	GENMASK(1, 0)
 
 #define LOCK_TIMEOUT_US		10000
 
+enum imx_pll14xx_ssc_mod_type {
+	IMX_PLL14XX_SSC_DOWN_SPREAD,
+	IMX_PLL14XX_SSC_UP_SPREAD,
+	IMX_PLL14XX_SSC_CENTER_SPREAD,
+};
+
 struct clk_pll14xx {
 	struct clk_hw			hw;
 	void __iomem			*base;
 	enum imx_pll14xx_type		type;
 	const struct imx_pll14xx_rate_table *rate_table;
 	int rate_count;
+	struct clk_spread_spectrum	ssc_conf;
 };
 
 #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
@@ -349,6 +362,42 @@  static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	return 0;
 }
 
+static void clk_pll1443x_enable_ssc(struct clk_hw *hw, unsigned long parent_rate,
+				    unsigned int pdiv, unsigned int mdiv)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+	struct clk_spread_spectrum *conf = &pll->ssc_conf;
+	u32 sscg_ctrl, mfr, mrr, mod_type;
+
+	sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
+	sscg_ctrl &=
+		~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
+
+	mfr = parent_rate / (conf->modfreq * pdiv * (1 << 5));
+	mrr = ((conf->spreaddepth / 100) * mdiv * (1 << 6)) / (100 * mfr);
+
+	switch (conf->method) {
+	case CLK_SSC_CENTER_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_CENTER_SPREAD;
+		break;
+	case CLK_SSC_UP_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_UP_SPREAD;
+		break;
+	case CLK_SSC_DOWN_SPREAD:
+		mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
+		break;
+	default:
+		mod_type = IMX_PLL14XX_SSC_DOWN_SPREAD;
+		break;
+	}
+
+	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
+		FIELD_PREP(MRAT_CTL_MASK, mrr) |
+		FIELD_PREP(SEL_PF_MASK, mod_type);
+
+	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
+}
+
 static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 				 unsigned long prate)
 {
@@ -370,6 +419,9 @@  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
 			       pll->base + DIV_CTL1);
 
+		if (pll->ssc_conf.enable)
+			clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
+
 		return 0;
 	}
 
@@ -410,6 +462,9 @@  static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 	gnrl_ctl &= ~BYPASS_MASK;
 	writel_relaxed(gnrl_ctl, pll->base + GNRL_CTL);
 
+	if (pll->ssc_conf.enable)
+		clk_pll1443x_enable_ssc(hw, prate, rate.pdiv, rate.mdiv);
+
 	return 0;
 }
 
@@ -465,6 +520,16 @@  static void clk_pll14xx_unprepare(struct clk_hw *hw)
 	writel_relaxed(val, pll->base + GNRL_CTL);
 }
 
+static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
+					    struct clk_spread_spectrum *clk_ss)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
+
+	memcpy(&pll->ssc_conf, clk_ss, sizeof(pll->ssc_conf));
+
+	return 0;
+}
+
 static const struct clk_ops clk_pll1416x_ops = {
 	.prepare	= clk_pll14xx_prepare,
 	.unprepare	= clk_pll14xx_unprepare,
@@ -485,6 +550,7 @@  static const struct clk_ops clk_pll1443x_ops = {
 	.recalc_rate	= clk_pll14xx_recalc_rate,
 	.round_rate	= clk_pll1443x_round_rate,
 	.set_rate	= clk_pll1443x_set_rate,
+	.set_spread_spectrum = clk_pll1443x_set_spread_spectrum,
 };
 
 struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,