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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d137cc7e-9b78-4cfb-ac50-023d1ea4c82b@kernel.org>
Date: Tue, 19 Nov 2024 15:07:08 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Peng Fan <peng.fan@....com>,
 Dario Binacchi <dario.binacchi@...rulasolutions.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-amarula@...rulasolutions.com" <linux-amarula@...rulasolutions.com>,
 Abel Vesa <abelvesa@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Fabio Estevam <festevam@...il.com>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Rob Herring <robh@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
 Shawn Guo <shawnguo@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
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.

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