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

From: Peng Fan
Date: Wed Feb 05 2025 - 19:53:59 EST

> 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@xxxxxxxxxxx> wrote:
> >
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > Add support for spread spectrum clock (SSC) generation to the
> pll14xxx
> > driver.
> >
> > Co-developed-by: Dario Binacchi
> <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxx/.

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
>
> dario.binacchi@xxxxxxxxxxxxxxxxxxxx
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> info@xxxxxxxxxxxxxxxxxxxx
>
> 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