> 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
