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

[Frank Li <Frank.li@nxp.com>]

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@nxp.com>
> >> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> >> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.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@30e10000 {
> >>  			};
> >>
> >>  			audio_blk_ctrl: clock-controller@30e20000 {
> >> -				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@3b6e8000 {
> >>  			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.