> Subject: Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock: support
> spread spectrum clocking
>
> On Mon, Nov 11, 2024 at 12:57 PM Dario Binacchi
> <dario.binacchi@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, Nov 11, 2024 at 2:49 AM Peng Fan <peng.fan@xxxxxxx>
> wrote:
> > >
> > > > Subject: Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock:
> > > > support spread spectrum clocking
> > > >
> > > > On 09/11/2024 11:05, Krzysztof Kozlowski wrote:
> > > > > On 09/11/2024 01:37, Peng Fan 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.
> > > > >
> > > > > I mean the device we discuss here, in this binding. Whatever
> > > > > this device is. CCM or CCX
> > > > >
> > > > >> CCM does not care, it just accepts the PLL as input, and output
> > > > >
> > > > > Takes PLL as input but you refuse to add it as clocks? Are you
> > > > > really responding to my question?
> > > > >
> > > > > I asked how can you set spread spectrum on clock which you do
> > > > > not receive. Why you do not receive? Because you refuse to add
> > > > > it to clocks even though I speak about this since months.
> > > > >
> > > > >> divided clock to various IPs(Video here). The video IPs care
> > > > >> about the spread spectrum of the clock.
> > > > >
> > > > > So which device do we talk about? I am not a NXP clock
> developer
> > > > and I
> > > > > care zero about NXP, so keep it simple. We discuss this one
> > > > > specific binding for specific device which is called
> > > > > "imx8m-clock" - see subject prefix.
> > > > >
> > > > >>
> > > > >> 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.
> > > > >
> > > > > I don't understand now even more. Or I understand even less
> now.
> > > > Why
> > > > > binding references its own clocks via phandle? This makes no
> > > > > sense
> > > > at
> > > > > all, except of course assigned clocks, but that's because we
> > > > > have one property for multiple cases.
> > > >
> > > > And BTW if that was the point then the example is confusing
> > > > because the &clk phandle is not the device node in the example
> but it should.
> > > > Neither description says which device's clocks are these.
> > > >
> > > > This is expressed very poorly in the binding, look:
> > > > "Phandles of the PLL" - it clearly suggests some other clocks, not
> > > > its own, that's so obvious I did not even think of asking.
> > > > Patchset goes slow also because of poor explanation, lack of
> > > > diagrams and expecting me to remember your clock hierarchy.
> > >
> > >
> > > Dario may improve the patchset in new version. But let me just try
> > > to explain a bit more about the hardware logic, I hope this could
> > > give you some knowledge on i.MX clock and we could get some
> > > suggestions on next:
> > >
> > >
> > > OSC will generate 24MHz clock to Anatop module.
> > > Anatop module takes 24MHz as input and produces various PLLs.
> > > Clock Control Module(CCM) takes PLLs as input, and outputs the
> final
> > > clocks to various IPs, saying video IPs.
> > >
> > > The Anatop module could produce PLLs with spread spectrum
> enabled.
> > > The Clock Control module just divides the freq and output the end
> IPs.
> > > The end IPs cares about spread spectrum for high quality clock, the
> > > Clock Control modules does not care. Now back to binding,
> > >
> > > There is a imx8m-anatop binding fsl,imx8m-anatop.yaml for anatop
> and
> > > a imx8m-clock.yaml binding for clock control module.
> > >
> > > I think the patchset is to enable spread spectrum of a PLL globally,
> > > not for a specific device saying video IP here. So the patchset put
> > > the properties under the clock control module.
> > >
> > > For example, there are VPU_JPEG, VPU_DECODE, both will use
> video PLL
> > > with high quality. So the clock producer just produce PLLs with
> > > Spread Spectrum(SS) enabled, and put the SS properties under CCM
> or
> > > anatop node, not video IP nodes.
> >
> > Thank you Peng, for the information.
> >
> > Do you think it would make sense to add the PLL nodes with SSCG to
> the
> > anatop node?
I not know Krzysztof's view on what he expects on spread spectrum of
a clock. I just think we are not on same page, but not know where
we misunderstands, or I am wrong.
For your below dt, I think clock maintainer would not agree.
So my idea is
Using clocks to replace fsl,ssc-clocks is possible under
ccm mode, but you need to develop the
fsl,imx8mm-anatop clock driver.
Or just replace fsl,ssc-clocks with clock id under anatop node,
no phandle under anatop node.
Then let ccm driver to handle it.
Regards,
Peng.
> >
> > anatop: clock-controller@30360000 {
> > compatible = "fsl,imx8mn-anatop", "fsl,imx8mm-anatop";
> > reg = <0x30360000 0x10000>;
> > #clock-cells = <1>;
> >
> > clk_video_pll1_ref_sel: clock-video-pll1-ref-sel@28 {
> > compatible = "fsl,imx8mn-mux-clock";
> > #clock-cells = <0>;
> > clocks = <&osc_24m>, <&clk_dummy>, <&clk_dummy>,
> <&clk_dummy>;
> > fsl,anatop = <&anatop 0x28>;
> > fsl,bit-shift = <0>;
> > clock-output-names = "video_pll1_ref_sel";
> > };
> >
> > clk_video_pll1: clock-video-pll1@28 {
> > compatible = "fsl,pll14xx-clock";
> > #clock-cells = <0>;
> > clocks = <&clk_video_pll1_ref_sel>;
> > ...
> > fsl,ssc-modfreq-hz = <6000>;
> > fsl,ssc-modrate-percent = <3>;
> > fsl,ssc-modmethod = "down-spread";
> > };
> > };
> >
> > This example only considers the video PLL, so to be complete, it
> > should also add the clk_audio_pll1,
> > clk_audio_pll2 and clk_dram_pll nodes. It is based on an RFC series
> > that I sent about a year ago, which was not accepted. In this way, the
> > SSCG properties (i.e., "fsl,ssc-modfreq-hz", "fsl,ssc-modrate-percent"
> > and "fsl,ssc-modmethod") would be added to the relevant nodes, and
> I
> > would take only the essential parts from that series. This would still
> > mean implementing the PLL driver
> > ("fsl,pll14xx-clock") and its mux ("fsl,imx8mn-mux-clock").
> >
> > These clocks can then be added to the "clocks" list of the "ccm" node:
> >
> > clk: clock-controller@30380000 {
> > compatible = "fsl,imx8mn-ccm";
> > ...
> > clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
> > <&clk_ext3>, <&clk_ext4>, <&clk_video_pll1>,
> > <&clk_audio_pll1>, <&clk_audio_pll2>, <&clk_dram_pll>;
> > ...
> > }
> >
> > >
> > >
>
> Next the series I forgot to reference in the previous email:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> lore.kernel.org%2Flkml%2F20230101175740.1010258-1-
> dario.binacchi%40amarulasolutions.com%2F&data=05%7C02%7Cpeng.
> fan%40nxp.com%7C47c26bb6c16549ff778d08dd0257261d%7C686ea
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63866929551955069
> 0%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiO
> iIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
> 3D%7C0%7C%7C%7C&sdata=1uWbb%2Fxa4PjemrEpSFbsBQP2X466i2c
> jYaxjQKl1xfE%3D&reserved=0
>
> Thanks and regards,
> Dario
>
> > > We could have a talk on IRC if Dario, Abel and you are available to
> > > discuss on this topic.
> >
> > Yes, I am available.
> >
> > Thanks and regards,
> > Dario
> >
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Best regards,
> > > > Krzysztof
> > >
> >
> >
> > --
> >
> > Dario Binacchi
> >
> > Senior Embedded Linux Developer
> >
> > dario.binacchi@xxxxxxxxxxxxxxxxxxxx
> >
> > __________________________________
> >
> >
> > Amarula Solutions SRL
> >
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> >
> > T. +39 042 243 5310
> > info@xxxxxxxxxxxxxxxxxxxx
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.a
> >
> marulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7
> C47c26bb6c16
> >
> 549ff778d08dd0257261d%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C638
> >
> 669295519581821%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcG
> kiOnRydWUsIlYiOi
> >
> IwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3
> D%7C0%7C%
> >
> 7C%7C&sdata=tK%2B9QB7Ri3eXJI%2FdXmCbwr%2BDwzdLt51CPo0DB
> pd%2BqOw%3D&res
> > erved=0
>
>
>
> --
>
> Dario Binacchi
>
> Senior Embedded Linux Developer
>
> dario.binacchi@xxxxxxxxxxxxxxxxxxxx
>
> __________________________________
>
>
> Amarula Solutions SRL
>
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
>
> T. +39 042 243 5310
> info@xxxxxxxxxxxxxxxxxxxx
>
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.amarulasolutions.com%2F&data=05%7C02%7Cpeng.fan%40nxp.
> com%7C47c26bb6c16549ff778d08dd0257261d%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C638669295519598876%7CUnkno
> wn%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDA
> wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C
> %7C%7C&sdata=ypZC9LExvvra%2BPcQIOE8HHnuRs3fmHjMVvRpox7Vd
> AU%3D&reserved=0
