Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation
From: Dario Binacchi
Date: Thu Feb 06 2025 - 10:32:09 EST
Hi Peng,
On Thu, Feb 6, 2025 at 1:53 AM Peng Fan <peng.fan@xxxxxxx> 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@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
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@xxxxxxxxxxxxxxxxxxxx/
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
> >
> > 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
--
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
www.amarulasolutions.com