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:
 <PAXPR04MB8459695A42922E52FCC9295488212@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Wed, 20 Nov 2024 10:11:39 +0000
From: Peng Fan <peng.fan@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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

> 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