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

[Peng Fan <peng.fan@nxp.com>]

> Subject: Re: [PATCH v3 1/8] dt-bindings: clock: imx8m-clock: support
> spread spectrum clocking
> 
> On 11/11/2024 02:49, Peng Fan wrote:
> >>> 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,
> 
> All above makes sense. The previous message:
> "Because in current design, ccm is taken as producer of
> CLK_IMX8M_VIDEO_PLL, not consumer. "
> 
> confused me a lot because it suggests that these PLLs are provided by
> CCM. It turns out not... so the answer is like I said long time ago: you
> must take these clocks as inputs and this is done via clocks property.
> Not fsl,clocks or fsc,i-want-more-properties-clocks.
> 
> >
> > 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.
> 
> I understand. This looks however as misrepresentation. If you do not
> have the video IP block enabled, why would you configure spread
> spectrum? IOW, spread spectrum as you described is needed for the
> final IP block and this final IP block should configure it. Properties
> belong there.

Multiple IPs use same PLLs as source and share same pll settings,
it is better to configure Spread Spectrum(SS) at clock producer side,
I think.

Dario, 

Without talking about dt-bindings, another approach to enable SS
is to enable SS for Video/Audio PLLs using driver parameters,
and the parameter that needs for the PLLs could be passed
from module parameter, such as clk_imx8mm.audio_ss_xx=.

Then you no need bindings.

Regards,
Peng.

> 
> It's kind of similar for some OPP/performance/bandwidth requests.
> Even more similar to clock frequencies. Which device requests to
> configure given clock frequencies? Final consumer, not clock controller.
> 
> 
> >
> > 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.
> >
> >
> > We could have a talk on IRC if Dario, Abel and you are available to
> > discuss on this topic.
> 
> 
> 
> Best regards,
> Krzysztof