Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock: support spread spectrum clocking

[Adam Ford <aford173@gmail.com>]

On Fri, Nov 8, 2024 at 6:37 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock: support
> > spread spectrum clocking
> >
> > On 08/11/2024 13:50, Peng Fan wrote:
> > >> Subject: Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock:
> > support
> > >> spread spectrum clocking
> > >>
> > >> On 07/11/2024 15:57, Dario Binacchi wrote:
> > >>>     clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
> > >>>                   <&clk_ext3>, <&clk_ext4>;
> > >>>     clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
> > >>>                              "clk_ext3", "clk_ext4";
> > >>>     assigned-clocks = <&clk IMX8MN_CLK_A53_SRC>,
> > >>>                                   <&clk IMX8MN_CLK_A53_CORE>,
> > >>>                                   <&clk IMX8MN_CLK_NOC>,
> > >>>                                   <&clk IMX8MN_CLK_AUDIO_AHB>,
> > >>>                                   <&clk IMX8MN_CLK_IPG_AUDIO_ROOT>,
> > >>>                                   <&clk IMX8MN_SYS_PLL3>,
> > >>>                                   <&clk IMX8MN_AUDIO_PLL1>,
> > >>>                                   <&clk IMX8MN_AUDIO_PLL2>;
> > >>>     assigned-clock-parents = <&clk IMX8MN_SYS_PLL1_800M>,
> > >>>                                              <&clk IMX8MN_ARM_PLL_OUT>,
> > >>>                                              <&clk IMX8MN_SYS_PLL3_OUT>,
> > >>>                                              <&clk IMX8MN_SYS_PLL1_800M>;
> > >>>     assigned-clock-rates = <0>, <0>, <0>,
> > >>>                                          <400000000>,
> > >>>                                          <400000000>,
> > >>>                                          <600000000>,
> > >>>                                          <393216000>,
> > >>>                                          <361267200>; };
> > >>>
> > >>> The spread spectrum is not configurable on these clocks or, more
> > >>> generally, may not be configurable (only 4 PLLs have this
> > capability).
> > >>> Therefore, I need the "fsl,ssc-clocks"
> > >>
> > >> No. That's not true. You do not need it.
> > >>
> > >
> > > i.MX8M clock hardware is similar as:
> > >
> > > OSC->ANATOP->CCM
> > >
> > > ANATOP will produce PLLs.
> > > CCM use PLLs as input source.
> > >
> > > Currently there is no dedicated ANATOP driver in linux.
> > > The CCM linux driver will parse the ANATOP node and register clk_hw
> > > for the PLLs.
> >
> > I do not know what is CCM and how does it fit here. What's more, I
> > don't get driver context here. We talk about bindings.
>
>
> CCM: Clock Control Module, it accepts PLL from anatop as inputs,
> and outputs clocks to various modules, I2C, CAN, NET, SAI and ...
>
> >
> >
> > >
> > >
> > >> First, the clock inputs for this device are listed in clocks *only*.
> > >> What is no there, is not an input to the device. Including also Linux
> > >> aspect (missing devlinks etc). Therefore how can you configure
> > spread
> > >> spectrum on clocks which are not connected to this device?
> > >
> > > I not understand this well, you mean
> > > add clocks = <xx CLK_IMX8MM_VIDEO_PLL> in the ccm dtb node?
> >
> > Yes. Let me re-iterate and please respond to this exactly comment
> > instead of ignoring it.
> >
> > How a device can care about spread spectrum of a clock which is not
> > supplied to this device?
>
> I hope we are on same page of what spread spectrum means.
> spread spectrum of a clock is the clock could produce freq in a range,
> saying [500MHz - 100KHz, 500MHz + 100KHz]. software only need
> to configure the middle frequency and choose the up/down border
> range(100KHz here) and enable spread spectrum.
>
> device: I suppose you mean the Clock Control Module(CCM) here.
> CCM does not care, it just accepts the PLL as input, and output
> divided clock to various IPs(Video here). The video IPs care about
> the spread spectrum of the clock.
>
> The clock hardware path is as below:
>
> OSC(24M) --> Anatop(produce PLL with spread spectrum) ->
> Clock Control Module(output clock to modules) -> Video IP
>
> From hardware perspective, Clock Control Module does not
> care spread spectrum. Video IP cares spread spectrum.
>
>
> >
> > Why would you care about spread spectrum of some clock which is not
> > coming to this device?
>
> device, I suppose you mean clock control module(CCM).
>
> There is no 'clocks = <&ccm CLK_IMX8M_VIDEO_PLL>' under ccm node.
> Because in current design, ccm is taken as producer of
> CLK_IMX8M_VIDEO_PLL, not consumer.
>
> >
> > Please address these precisely because we talk about this for weeks in
> > multiple places.
>
> Sorry for coming into this feature in late stage.
>
> Dario, thanks for working on such feature, good to see. Spread Spectrum
> is indeed good feature what makes clock quality high.

I am also excited to see the spread-spectum clocks enabled.  We've
struggled with EMC testing in the past, and I want to reevaluate at
least one board with the spread-spectrum enabled to see how it
compares.
Thank you for working on this.

adam
>
> Thanks,
> Peng.
>
> I finish with this patchset if you do not provide such
> > context.
> >
> > Best regards,
> > Krzysztof
>