[0/3] clk: Support spread spectrum and use it in clk-scmi

Message

Peng Fan (OSS) Jan. 24, 2025, 2:25 p.m. UTC

- Introduce clk_set_spread_spectrum to set the parameters for enabling
  spread spectrum of a clock.
- Parse 'assigned-clock-sscs' and configure it by default before using the
  clock. The pull request for this property is at [1]
  This property is parsed before parsing clock rate.

- Enable this feature for clk-scmi on i.MX95.
  This may not the best, since checking machine compatibles.
  I am thinking to provide an API scmi_get_vendor_info, then driver
  could use it for OEM stuff, such as
  if (scmi_get_vendor_info returns NXP_IMX)
      ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;

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

Signed-off-by: Peng Fan <[e-mail]>
---
Peng Fan (3):
      clk: Introduce clk_set_spread_spectrum
      clk: conf: Support assigned-clock-sscs
      clk: scmi: Support spread spectrum

 drivers/clk/clk-conf.c        | 68 +++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-scmi.c        | 37 +++++++++++++++++++++++
 drivers/clk/clk.c             | 39 +++++++++++++++++++++++++
 include/linux/clk-provider.h  | 22 ++++++++++++++
 include/linux/clk.h           | 22 ++++++++++++++
 include/linux/scmi_protocol.h |  5 ++++
 6 files changed, 193 insertions(+)
---
base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183
change-id: 20250124-clk-ssc-fccd4f60d7e5

Best regards,

Comments

Dario Binacchi Jan. 25, 2025, 12:58 p.m. UTC | #1

On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <[e-mail]> wrote:
>
> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>   spread spectrum of a clock.
> - Parse 'assigned-clock-sscs' and configure it by default before using the
>   clock. The pull request for this property is at [1]
>   This property is parsed before parsing clock rate.
>
> - Enable this feature for clk-scmi on i.MX95.
>   This may not the best, since checking machine compatibles.
>   I am thinking to provide an API scmi_get_vendor_info, then driver
>   could use it for OEM stuff, such as
>   if (scmi_get_vendor_info returns NXP_IMX)
>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>

I wonder if your solution is truly generic or merely a generalization
of your use case, which seems significantly simpler compared to what
happens on the i.MX8M platform, as discussed in thread
https://lore.kernel.org/lkml/[e-mail]/,
or on the STM32F platform, where parameters are not written directly
to registers but are instead used in calculations involving the
parent_rate and the PLL divider, for example.

I am the author of the patches that introduced spread spectrum
management for the AM33x and STM32Fx platforms, as well as the
series, still pending acceptance, for the i.MX8M.
From my perspective, this functionality varies significantly
from platform to platform, with key differences that must be
considered.

Thanks and regards,
Dario

> [1] https://github.com/devicetree-org/dt-schema/pull/154
>
> Signed-off-by: Peng Fan <[e-mail]>
> ---
> Peng Fan (3):
>       clk: Introduce clk_set_spread_spectrum
>       clk: conf: Support assigned-clock-sscs
>       clk: scmi: Support spread spectrum
>
>  drivers/clk/clk-conf.c        | 68 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-scmi.c        | 37 +++++++++++++++++++++++
>  drivers/clk/clk.c             | 39 +++++++++++++++++++++++++
>  include/linux/clk-provider.h  | 22 ++++++++++++++
>  include/linux/clk.h           | 22 ++++++++++++++
>  include/linux/scmi_protocol.h |  5 ++++
>  6 files changed, 193 insertions(+)
> ---
> base-commit: 5ffa57f6eecefababb8cbe327222ef171943b183
> change-id: 20250124-clk-ssc-fccd4f60d7e5
>
> Best regards,
> --
> Peng Fan <[e-mail]>
>

Krzysztof Kozlowski Jan. 27, 2025, 7:42 a.m. UTC | #2

On 25/01/2025 13:58, Dario Binacchi wrote:
> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <[e-mail]> wrote:
>>
>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>>   spread spectrum of a clock.
>> - Parse 'assigned-clock-sscs' and configure it by default before using the
>>   clock. The pull request for this property is at [1]
>>   This property is parsed before parsing clock rate.
>>
>> - Enable this feature for clk-scmi on i.MX95.
>>   This may not the best, since checking machine compatibles.
>>   I am thinking to provide an API scmi_get_vendor_info, then driver
>>   could use it for OEM stuff, such as
>>   if (scmi_get_vendor_info returns NXP_IMX)
>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>>
> 
> I wonder if your solution is truly generic or merely a generalization
> of your use case, which seems significantly simpler compared to what

Please come with specific arguments why this is not generic enough, not
just FUD. Does it fit your case? If not, what would had to be changed?
These are the comments needed to actually work on generic solution.

> happens on the i.MX8M platform, as discussed in thread
> https://lore.kernel.org/lkml/[e-mail]/,
> or on the STM32F platform, where parameters are not written directly
> to registers but are instead used in calculations involving the
> parent_rate and the PLL divider, for example.
> 
> I am the author of the patches that introduced spread spectrum
> management for the AM33x and STM32Fx platforms, as well as the
> series, still pending acceptance, for the i.MX8M.
> From my perspective, this functionality varies significantly
> from platform to platform, with key differences that must be
> considered.

So what exactly varies? Come with specifics.

To me look addressing exactly the same.

Best regards,
Krzysztof

Dario Binacchi Jan. 27, 2025, 7:59 a.m. UTC | #3

On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <[e-mail]> wrote:
>
> On 25/01/2025 13:58, Dario Binacchi wrote:
> > On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <[e-mail]> wrote:
> >>
> >> - Introduce clk_set_spread_spectrum to set the parameters for enabling
> >>   spread spectrum of a clock.
> >> - Parse 'assigned-clock-sscs' and configure it by default before using the
> >>   clock. The pull request for this property is at [1]
> >>   This property is parsed before parsing clock rate.
> >>
> >> - Enable this feature for clk-scmi on i.MX95.
> >>   This may not the best, since checking machine compatibles.
> >>   I am thinking to provide an API scmi_get_vendor_info, then driver
> >>   could use it for OEM stuff, such as
> >>   if (scmi_get_vendor_info returns NXP_IMX)
> >>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
> >>
> >
> > I wonder if your solution is truly generic or merely a generalization
> > of your use case, which seems significantly simpler compared to what
>
> Please come with specific arguments why this is not generic enough, not
> just FUD. Does it fit your case? If not, what would had to be changed?
> These are the comments needed to actually work on generic solution.
>
> > happens on the i.MX8M platform, as discussed in thread
> > https://lore.kernel.org/lkml/[e-mail]/,
> > or on the STM32F platform, where parameters are not written directly
> > to registers but are instead used in calculations involving the
> > parent_rate and the PLL divider, for example.
> >
> > I am the author of the patches that introduced spread spectrum
> > management for the AM33x and STM32Fx platforms, as well as the
> > series, still pending acceptance, for the i.MX8M.
> > From my perspective, this functionality varies significantly
> > from platform to platform, with key differences that must be
> > considered.
>
> So what exactly varies? Come with specifics.

In all the cases I implemented, I enabled spread spectrum within
the set_rate of the clock/PLL in question, as information such as
the parent rate or the divisor used was necessary to perform the
calculations needed to extract the data for setting the SSCG
register bitfields.

If I'm not mistaken, I think this is not the case implemented by this
series.

Thanks and regards,
Dario

> To me look addressing exactly the same.
>
> Best regards,
> Krzysztof

Krzysztof Kozlowski Jan. 27, 2025, 8:31 a.m. UTC | #4

On 27/01/2025 08:59, Dario Binacchi wrote:
> On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <[e-mail]> wrote:
>>
>> On 25/01/2025 13:58, Dario Binacchi wrote:
>>> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <[e-mail]> wrote:
>>>>
>>>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
>>>>   spread spectrum of a clock.
>>>> - Parse 'assigned-clock-sscs' and configure it by default before using the
>>>>   clock. The pull request for this property is at [1]
>>>>   This property is parsed before parsing clock rate.
>>>>
>>>> - Enable this feature for clk-scmi on i.MX95.
>>>>   This may not the best, since checking machine compatibles.
>>>>   I am thinking to provide an API scmi_get_vendor_info, then driver
>>>>   could use it for OEM stuff, such as
>>>>   if (scmi_get_vendor_info returns NXP_IMX)
>>>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
>>>>
>>>
>>> I wonder if your solution is truly generic or merely a generalization
>>> of your use case, which seems significantly simpler compared to what
>>
>> Please come with specific arguments why this is not generic enough, not
>> just FUD. Does it fit your case? If not, what would had to be changed?
>> These are the comments needed to actually work on generic solution.
>>
>>> happens on the i.MX8M platform, as discussed in thread
>>> https://lore.kernel.org/lkml/[e-mail]/,
>>> or on the STM32F platform, where parameters are not written directly
>>> to registers but are instead used in calculations involving the
>>> parent_rate and the PLL divider, for example.
>>>
>>> I am the author of the patches that introduced spread spectrum
>>> management for the AM33x and STM32Fx platforms, as well as the
>>> series, still pending acceptance, for the i.MX8M.
>>> From my perspective, this functionality varies significantly
>>> from platform to platform, with key differences that must be
>>> considered.
>>
>> So what exactly varies? Come with specifics.
> 
> In all the cases I implemented, I enabled spread spectrum within
> the set_rate of the clock/PLL in question, as information such as
> the parent rate or the divisor used was necessary to perform the
> calculations needed to extract the data for setting the SSCG
> register bitfields.
> 
> If I'm not mistaken, I think this is not the case implemented by this
> series.

It feels like you speak about driver, so I misunderstood the concerns. I
did not check the drivers at all, so here I do not claim patchsets are
compatible.

But the binding takes the same values - the main PLL/clock rate.

Best regards,
Krzysztof

Dario Binacchi Jan. 27, 2025, 8:35 a.m. UTC | #5

On Mon, Jan 27, 2025 at 9:31 AM Krzysztof Kozlowski <[e-mail]> wrote:
>
> On 27/01/2025 08:59, Dario Binacchi wrote:
> > On Mon, Jan 27, 2025 at 8:42 AM Krzysztof Kozlowski <[e-mail]> wrote:
> >>
> >> On 25/01/2025 13:58, Dario Binacchi wrote:
> >>> On Fri, Jan 24, 2025 at 2:19 PM Peng Fan (OSS) <[e-mail]> wrote:
> >>>>
> >>>> - Introduce clk_set_spread_spectrum to set the parameters for enabling
> >>>>   spread spectrum of a clock.
> >>>> - Parse 'assigned-clock-sscs' and configure it by default before using the
> >>>>   clock. The pull request for this property is at [1]
> >>>>   This property is parsed before parsing clock rate.
> >>>>
> >>>> - Enable this feature for clk-scmi on i.MX95.
> >>>>   This may not the best, since checking machine compatibles.
> >>>>   I am thinking to provide an API scmi_get_vendor_info, then driver
> >>>>   could use it for OEM stuff, such as
> >>>>   if (scmi_get_vendor_info returns NXP_IMX)
> >>>>       ops->set_spread_spectrum = scmi_clk_set_spread_spectrum_imx;
> >>>>
> >>>
> >>> I wonder if your solution is truly generic or merely a generalization
> >>> of your use case, which seems significantly simpler compared to what
> >>
> >> Please come with specific arguments why this is not generic enough, not
> >> just FUD. Does it fit your case? If not, what would had to be changed?
> >> These are the comments needed to actually work on generic solution.
> >>
> >>> happens on the i.MX8M platform, as discussed in thread
> >>> https://lore.kernel.org/lkml/[e-mail]/,
> >>> or on the STM32F platform, where parameters are not written directly
> >>> to registers but are instead used in calculations involving the
> >>> parent_rate and the PLL divider, for example.
> >>>
> >>> I am the author of the patches that introduced spread spectrum
> >>> management for the AM33x and STM32Fx platforms, as well as the
> >>> series, still pending acceptance, for the i.MX8M.
> >>> From my perspective, this functionality varies significantly
> >>> from platform to platform, with key differences that must be
> >>> considered.
> >>
> >> So what exactly varies? Come with specifics.
> >
> > In all the cases I implemented, I enabled spread spectrum within
> > the set_rate of the clock/PLL in question, as information such as
> > the parent rate or the divisor used was necessary to perform the
> > calculations needed to extract the data for setting the SSCG
> > register bitfields.
> >
> > If I'm not mistaken, I think this is not the case implemented by this
> > series.
>
> It feels like you speak about driver, so I misunderstood the concerns. I
> did not check the drivers at all, so here I do not claim patchsets are
> compatible.

Yes, I commented on the driver.
For the dt-bindings I added a comment in the github PR
https://github.com/devicetree-org/dt-schema/pull/154

Thanks and regards,
Dario

>
> But the binding takes the same values - the main PLL/clock rate.
>
> Best regards,
> Krzysztof