Message-ID: <CABGWkvrR-vVTpNSBD_etjn4SteO8cpUed+dTvYguHR67UUSsYA@mail.gmail.com>
Date: Wed, 23 Oct 2024 16:58:28 +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 11:16 AM Dario Binacchi
<dario.binacchi@...rulasolutions.com> wrote:
>
> 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?

A gentle ping.
Sorry, but I haven't yet received your response to the previous email,
and I'm not sure how to proceed.

Thanks and regards,
Dario

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



-- 

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