linux-kernel - Re: [PATCH v4 5/6] arm64: dts: imx8mp: Add fsl,dsp-ctrl property for dsp

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] [day] [month] [year] [list]

Message-ID: <Z6z6ojBTTFKIkc2/@lizhi-Precision-Tower-5810>
Date: Wed, 12 Feb 2025 14:48:46 -0500
From: Frank Li <Frank.li@....com>
To: Laurentiu Mihalcea <laurentiumihalcea111@...il.com>
Cc: Daniel Baluta <daniel.baluta@....com>, shawnguo@...nel.org,
	robh@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
	krzk+dt@...nel.org, conor+dt@...nel.org, festevam@...il.com,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	aisheng.dong@....com, daniel.baluta@...il.com,
	laurentiu.mihalcea@....com, shengjiu.wang@....com,
	iuliana.prodan@....com, a.fatoum@...gutronix.de,
	Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v4 5/6] arm64: dts: imx8mp: Add fsl,dsp-ctrl property for
 dsp

On Wed, Feb 12, 2025 at 09:04:47PM +0200, Laurentiu Mihalcea wrote:
> On 2/12/2025 6:02 PM, Frank Li wrote:
> > On Wed, Feb 12, 2025 at 10:52:21AM +0200, Daniel Baluta wrote:
> >> Audio block control contains a set of registers and some of them used for
> >> DSP configuration.
> >>
> >> Drivers (rproc, SOF) are using fsl,dsp-ctrl property in order to control
> >> the DSP, particularly for Run/Stall bit.
> >>
> >> Note that audio block control doesn't offer the functionality to reset
> >> thte DSP. Reset is done via DAP interface.
> >>
> >> Reviewed-by: Iuliana Prodan <iuliana.prodan@....com>
> >> Reviewed-by: Peng Fan <peng.fan@....com>
> >> Signed-off-by: Daniel Baluta <daniel.baluta@....com>
> >> ---
> >>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> index 14cfbd228b45..46b33fbb9bd1 100644
> >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> >> @@ -1609,7 +1609,7 @@ sdma2: dma-controller@...10000 {
> >>  			};
> >>
> >>  			audio_blk_ctrl: clock-controller@...20000 {
> >> -				compatible = "fsl,imx8mp-audio-blk-ctrl";
> >> +				compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon";
> >>  				reg = <0x30e20000 0x10000>;
> >>  				#clock-cells = <1>;
> >>  				#reset-cells = <1>;
> >> @@ -2425,6 +2425,7 @@ dsp: dsp@...e8000 {
> >>  			mboxes = <&mu2 0 0>, <&mu2 1 0>, <&mu2 3 0>;
> >>  			firmware-name = "imx/dsp/hifi4.bin";
> >>  			memory-region = <&dsp_reserved>;
> >> +			fsl,dsp-ctrl = <&audio_blk_ctrl>;
> > I think kk's comments in v3
> >
> > "This should have been implemented as reset controller, not syscon. It's
> > really poor choice to call everything syscon. It does not scale, does
> > not provide you runtime PM or probe ordering (device links). Quick look
> > at your drivers suggest that you have exacvtly that problems."
> >
> > The above is quite good suggestion.
> >
> > Your reply "But for Run/Stall bits we need to use a syscon.",
> >
> > Run/Stall actually is reset, Most system, release reset signal means dsp/core
> > RUN.
> >
> > Frank
>
> RESET and STALL are quite different signals w/ different purposes so
> calling them both RESET is confusing and inaccurate.
>
> The syscon is used here to control the STALL signal (name of the bit we're using is RunStall)
> and has nothing to do with the RESET signal, which is why it's implemented as a syscon rather
> than a reset controller.
>
> While Krzysztof's comment does make sense, I still find it odd to have this implemented as a
> reset controller despite it not having anything to do with the RESET signal.

Regardless hardware signal name, the logic function is that release a
signal and let DSP core running. So we still model it as reset-controllers.

Did you actaully pause/resume DSP running at your drivers?

Frank

>
> Also, do note that the two nodes are in clock provider-consumer relationship
> so unless I'm gravely mistaken this should at least guarantee the probe order.