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?
>
> 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://lore.kernel.org/lkml/20230101175740.1010258-1-dario.binacchi@xxxxxxxxxxxxxxxxxxxx/
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
>
> www.amarulasolutions.com
--
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
www.amarulasolutions.com
