Message-ID: <CABGWkvqFi_y8OzKbi=K7ucW4RuY_zh6Z4a=uO2oqQRoVE8LaCQ@mail.gmail.com>
Date: Tue, 8 Oct 2024 11:16:41 +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 Tue, Oct 8, 2024 at 10:20 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 07/10/2024 17:02, Dario Binacchi wrote:
> > 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.
>
> Sure, so each board will come with its own, different values and you
> will not put into the SoC DTSI?

Yes, exactly.

>
> Where is the DTS? I received only this patch.

I haven't had time to push the board I'm working on upstream yet.
Locally, I apply this patch:

--- a/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp-ctouch2-of10.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-icore-mx8mp-ctouch2-of10.dts
@@ -113,6 +113,13 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
        };
 };

+&anatop {
+       fsl,ssc-clocks = <&clk IMX8MP_VIDEO_PLL1>;
+       fsl,ssc-modfreq-hz = <6818>;
+       fsl,ssc-modrate-percent = <3>;
+       fsl,ssc-modmethod = "down-spread";
+};
+
 /* Ethernet */
 &eqos {
        pinctrl-names = "default";

>
> >
> >> 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")
>
>
> OK, I still do not know what "0" was, but the items are fixed, so you
> know exactly which clock you are configuring here.

So, after delving deeper into the topic, is it now acceptable to use
the property
"fsl,ssc-clocks" instead of "clocks"?  As in the patch I applied locally?

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