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

From: Peng Fan
Date: Fri Feb 07 2025 - 04:37:12 EST

Next message: Petr Mladek: "Re: [RFC PATCH 2/2] livepatch: Implement livepatch hybrid mode"
Previous message: Shuai Xue: "[PATCH v3 0/4] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd"
In reply to: Sudeep Holla: "Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation"
Next in thread: Peng Fan (OSS): "[PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@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!"

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@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

Next message: Petr Mladek: "Re: [RFC PATCH 2/2] livepatch: Implement livepatch hybrid mode"
Previous message: Shuai Xue: "[PATCH v3 0/4] PCI/AER: Report fatal errors of RCiEP and EP if link recoverd"
In reply to: Sudeep Holla: "Re: [PATCH v2 3/4] clk: imx: pll14xx: support spread spectrum clock generation"
Next in thread: Peng Fan (OSS): "[PATCH NOT APPLY v2 4/4] clk: scmi: Support spread spectrum"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]