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

[Peng Fan <peng.fan@oss.nxp.com>]

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 <peng.fan@nxp.com> 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)
>> > <peng.fan@oss.nxp.com> wrote:
>> > >
>> > > From: Peng Fan <peng.fan@nxp.com>
>> > >
>> > > Add support for spread spectrum clock (SSC) generation to the
>> > pll14xxx
>> > > driver.
>> > >
>> > > Co-developed-by: Dario Binacchi
>> > <dario.binacchi@amarulasolutions.com>
>> > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> >
>> > 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/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/.
>>
>> 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/20250118124044.157308-18-dario.binacchi@amarulasolutions.com/
>
>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
>> >
>> > dario.binacchi@amarulasolutions.com
>> >
>> > __________________________________
>> >
>> >
>> > Amarula Solutions SRL
>> >
>> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
>> >
>> > T. +39 042 243 5310
>> > info@amarulasolutions.com
>> >
>> > 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
>
>dario.binacchi@amarulasolutions.com
>
>__________________________________
>
>
>Amarula Solutions SRL
>
>Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
>T. +39 042 243 5310
>info@amarulasolutions.com
>
>www.amarulasolutions.com