Message-ID: <CABGWkvqkmo9O-O1taR651W4xo=yqar=p71e0LKqRte2CGZ2Z8w@mail.gmail.com>
Date: Mon, 7 Oct 2024 17:02:37 +0200
From: Dario Binacchi <dario.binacchi@...rulasolutions.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-amarula@...rulasolutions.com, 
	Conor Dooley <conor+dt@...nel.org>, Fabio Estevam <festevam@...il.com>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Peng Fan <peng.fan@....com>, Pengutronix Kernel Team <kernel@...gutronix.de>, Rob Herring <robh@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Shawn Guo <shawnguo@...nel.org>, 
	Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH 1/6] dt-bindings: clock: imx8m-anatop: support spread
 spectrum clocking

On Sun, Oct 6, 2024 at 3:13 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 05/10/2024 10:57, Dario Binacchi wrote:
> > On Thu, Oct 3, 2024 at 12:46 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>
> >> On 01/10/2024 08:29, Dario Binacchi wrote:
> >>> On Mon, Sep 30, 2024 at 8:45 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>>>
> >>>> On 29/09/2024 22:00, Dario Binacchi wrote:
> >>>>>>
> >>>>>>
> >>>>>>> +  properties:
> >>>>>>> +    compatible:
> >>>>>>> +      contains:
> >>>>>>> +        enum:
> >>>>>>> +          - fsl,imx8mm-anatop
> >>>>>>> +
> >>>>>>> +then:
> >>>>>>> +  properties:
> >>>>>>> +    fsl,ssc-clocks:
> >>>>>>
> >>>>>> Nope. Properties must be defined in top-level.
> >>>>>>
> >>>>>>> +      $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>>>>> +      description:
> >>>>>>> +        The phandles to the PLLs with spread spectrum clock generation
> >>>>>>> +        hardware capability.
> >>>>>>
> >>>>>> These should be clocks.
> >>>>>
> >>>>> Sorry, but I can't understand what you're asking me.
> >>>>> Could you kindly explain it to me in more detail?
> >>>>
> >>>> You added new property instead of using existing one for this purpose:
> >>>> 'clocks'.
> >>>
> >>>>
> >>>>
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>>
> >>>
> >>> I added this new property specifically for managing spread-spectrum.
> >>> Indeed, not all clocks/PLLs
> >>> managed by the node/peripheral support spread-spectrum, and the added
> >>> properties specify
> >>> parameters for enabling and tuning SSC for each individual PLL based
> >>> on the index of each list.
> >>> If I were to use the 'clocks' property and add a clock to this list
> >>> that does not support SSC, IMHO
> >>> the pairings would be less clear.
> >>
> >> You duplicate property with argument "pairings shall match". Well, I am
> >> not happy with the duplication. Clocks have specific order, thus it is
> >> explicit which one needs tuning. Your other properties can match them as
> >> well, just index from clocks is offset...
> >
> > Just to check if I understood correctly what you are suggesting before
> > submitting version 3 of the patch.
> > Something, for example, like:
> >
> > clocks = <&clk, IMX8MP_AUDIO_PLL1>,  <&clk, IMX8MP_AUDIO_PLL2>, <&clk
> > IMX8MP_VIDEO_PLL1>;
> > fsl,ssc-modfreq-hz = <0, 3517>, <2, 6818>;
>
> Hm, what is 0? If clock index, then no, it's redundant. The first item
> in cannot point to other clock.
>
> Also, what exactly are you setting here

I am enabling and configuring the spread spectrum.

Normal clock: Without spread spectrum, the clock signal has a fixed and
repetitive frequency (e.g., 100 MHz). This frequency generates an
electromagnetic
signal concentrated on a single frequency, and if strong enough, it can disturb
other devices.

Spread spectrum:  With spread spectrum, the clock frequency is
slightly "modulated,"
meaning it oscillates around a central value. For example, if the base
frequency is 100 MHz,
the clock might vary between 99.5 MHz and 100.5 MHz in a cyclic manner. This
small variation spreads the energy over a wider range of frequencies
(from 99.5 to 100.5 MHz),
reducing the intensity of the signal at any one frequency.

> and why assigned-clock-rates are
> not working?

The traditional clock properties, such as clocks,
assigned-clocks-rates, etc retain their usual
meaning even when spread spectrum is applied. However, to implement
the spread spectrum
mechanism in a circuit with a PLL (Phase-Locked Loop), additional
specific parameters are
introduced to properly configure the frequency modulation:

 - Modulation frequency: i. e. fsl,ssc-modfreq-hz
 - Modulation rate: i.e.  fsl,ssc-modrate-percent
 - Modulation type:  i. e. fsl,ssc-modmethod (center-spread, down-spread)

Additionally, it should be noted that not all anatop PLLs are equipped
with circuitry for spread
spectrum, but only a small subset of them. This is the reason why I
introduced the property
"fsl, ssc-clocks".

This is another commit [1] on enabling spread spectrum that I
implemented some time ago for
the am335x. The most evident difference is that in that case the node
was a clock node and not
a clock controller, as in the case of anatop. The parameters are also
not exactly the same, but
that depends on the platform.

[1] 4a8bc2644ef0cbf8e ("dt-bindings: ti: dpll: add spread spectrum support")

Thanks and regards,
Dario

>
> Best regards,
> Krzysztof
>


-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@...rulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@...rulasolutions.com

www.amarulasolutions.com

Powered by blists - more mailing lists