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. 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
