[PATCH v2 0/8] imx8mp: Add support to Run/Stall DSP via reset API

[PATCH v2 0/8] imx8mp: Add support to Run/Stall DSP via reset API

[Daniel Baluta]

This patch series adds support to Run/Stall DSP found on i.MX8MP via the
reset controller API.

I've addressed all review comments from v1
(https://www.spinics.net/lists/kernel/msg5556023.html)
and because of the review I've introduced 3 new patches (1-3) in order
to support resets ids via dt-bindings from
include/dt-bindings/reset/imx8mp-reset-audiomix.h

Also picked up all R-b from Frank Li, Peng Fan and  Philipp Zabel.

Patches 1,2, 4-7 - should go via reset controller tree.
Patch 8 - should go via remoteproc tree.

Later after all these patches are merged we can also merge patch 3/8
via dts tree (Shawn's tree).

Daniel Baluta (8):
  dt-bindings: reset: audiomix: Add reset ids for EARC and DSP
  dt-bindings: dsp: fsl,dsp: Add resets property
  arm64: dts: imx8mp: Add resets to dsp node
  reset: imx8mp-audiomix: Add prefix for internal macro
  reset: imx8mp-audiomix: Prepare the code for more reset bits
  reset: imx8mp-audiomix: Introduce active_low configuration option
  reset: imx8mp-audiomix: Add support for DSP run/stall
  imx_dsp_rproc: Use reset controller API to control the DSP

 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++-
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  2 +
 drivers/remoteproc/imx_dsp_rproc.c            | 25 ++++--
 drivers/remoteproc/imx_rproc.h                |  2 +
 drivers/reset/reset-imx8mp-audiomix.c         | 77 +++++++++++++------
 .../dt-bindings/reset/imx8mp-reset-audiomix.h | 13 ++++
 6 files changed, 107 insertions(+), 31 deletions(-)
 create mode 100644 include/dt-bindings/reset/imx8mp-reset-audiomix.h

-- 
2.25.1

[PATCH v2 1/8] dt-bindings: reset: audiomix: Add reset ids for EARC and DSP

[Daniel Baluta]

Add reset ids used for EARC and DSP on i.MX8MP platform.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 include/dt-bindings/reset/imx8mp-reset-audiomix.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 include/dt-bindings/reset/imx8mp-reset-audiomix.h

diff --git a/include/dt-bindings/reset/imx8mp-reset-audiomix.h b/include/dt-bindings/reset/imx8mp-reset-audiomix.h
new file mode 100644
index 000000000000..3349bf311764
--- /dev/null
+++ b/include/dt-bindings/reset/imx8mp-reset-audiomix.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
+/*
+ * Copyright 2025 NXP
+ */
+
+#ifndef DT_BINDING_RESET_IMX8MP_AUDIOMIX_H
+#define DT_BINDING_RESET_IMX8MP_AUDIOMIX_H
+
+#define IMX8MP_AUDIOMIX_EARC		0
+#define IMX8MP_AUDIOMIX_EARC_PHY	1
+#define IMX8MP_AUDIOMIX_DSP		2
+
+#endif /* DT_BINDING_RESET_IMX8MP_AUDIOMIX_H */
-- 
2.25.1

Re: [PATCH v2 1/8] dt-bindings: reset: audiomix: Add reset ids for EARC and DSP

[Frank Li]

On Wed, Feb 19, 2025 at 09:20:55PM +0200, Daniel Baluta wrote:
> Add reset ids used for EARC and DSP on i.MX8MP platform.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  include/dt-bindings/reset/imx8mp-reset-audiomix.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 include/dt-bindings/reset/imx8mp-reset-audiomix.h
>
> diff --git a/include/dt-bindings/reset/imx8mp-reset-audiomix.h b/include/dt-bindings/reset/imx8mp-reset-audiomix.h
> new file mode 100644
> index 000000000000..3349bf311764
> --- /dev/null
> +++ b/include/dt-bindings/reset/imx8mp-reset-audiomix.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#ifndef DT_BINDING_RESET_IMX8MP_AUDIOMIX_H
> +#define DT_BINDING_RESET_IMX8MP_AUDIOMIX_H
> +
> +#define IMX8MP_AUDIOMIX_EARC		0
> +#define IMX8MP_AUDIOMIX_EARC_PHY	1
> +#define IMX8MP_AUDIOMIX_DSP		2
> +
> +#endif /* DT_BINDING_RESET_IMX8MP_AUDIOMIX_H */
> --
> 2.25.1
>

Re: [PATCH v2 1/8] dt-bindings: reset: audiomix: Add reset ids for EARC and DSP

[Rob Herring (Arm)]

On Wed, 19 Feb 2025 21:20:55 +0200, Daniel Baluta wrote:
> Add reset ids used for EARC and DSP on i.MX8MP platform.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  include/dt-bindings/reset/imx8mp-reset-audiomix.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 include/dt-bindings/reset/imx8mp-reset-audiomix.h
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Daniel Baluta]

On i.MX8MP we introduced support for using a reset controller
to control DSP operation.

This patch adds reset property which is required for i.MX8MP.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
index ab93ffd3d2e5..923e7f079f1b 100644
--- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
+++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
@@ -82,6 +82,13 @@ properties:
     description:
       Phandle to syscon block which provide access for processor enablement
 
+  resets:
+    description:
+      A pair consisting of phandle to audio-blk-control and an index referencing
+      the DSP Run/Stall bit in audiomix registers.
+      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -164,6 +171,16 @@ allOf:
             - const: txdb1
             - const: rxdb0
             - const: rxdb1
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx8mp-dsp
+              - fsl,imx8mp-hifi4
+    then:
+      required:
+        - "resets"
 
 additionalProperties: false
 
@@ -220,5 +237,5 @@ examples:
                <&mu2 3 0>;
       memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
                       <&dsp_vdev0vring1>, <&dsp_reserved>;
-      fsl,dsp-ctrl = <&audio_blk_ctrl>;
+      resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
     };
-- 
2.25.1

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Rob Herring (Arm)]

On Wed, 19 Feb 2025 21:20:56 +0200, Daniel Baluta wrote:
> On i.MX8MP we introduced support for using a reset controller
> to control DSP operation.
> 
> This patch adds reset property which is required for i.MX8MP.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/dsp/fsl,dsp.yaml:183:11: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/dsp/fsl,dsp.example.dts:85.37-38 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/dsp/fsl,dsp.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250219192102.423850-3-daniel.baluta@nxp.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Frank Li]

On Wed, Feb 19, 2025 at 09:20:56PM +0200, Daniel Baluta wrote:
> On i.MX8MP we introduced support for using a reset controller
> to control DSP operation.
>
> This patch adds reset property which is required for i.MX8MP.

Avoid words "this patch" according to kernel submit patch document.

Just said:  Add reset property ...

>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index ab93ffd3d2e5..923e7f079f1b 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -82,6 +82,13 @@ properties:
>      description:
>        Phandle to syscon block which provide access for processor enablement
>
> +  resets:
> +    description:
> +      A pair consisting of phandle to audio-blk-control and an index referencing
> +      the DSP Run/Stall bit in audiomix registers.
> +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.

Generally, needn't description for such common property. Or just said
for example, i.MX8MP, ... , because it may change to difference reset
providor in future.

Anyway, I think it is fine leave here.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -164,6 +171,16 @@ allOf:
>              - const: txdb1
>              - const: rxdb0
>              - const: rxdb1
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx8mp-dsp
> +              - fsl,imx8mp-hifi4
> +    then:
> +      required:
> +        - "resets"
>
>  additionalProperties: false
>
> @@ -220,5 +237,5 @@ examples:
>                 <&mu2 3 0>;
>        memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
>                        <&dsp_vdev0vring1>, <&dsp_reserved>;
> -      fsl,dsp-ctrl = <&audio_blk_ctrl>;
> +      resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
>      };
> --
> 2.25.1
>

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Alexander Stein]

Hi,

Am Mittwoch, 19. Februar 2025, 20:20:56 CET schrieb Daniel Baluta:
> On i.MX8MP we introduced support for using a reset controller
> to control DSP operation.
> 
> This patch adds reset property which is required for i.MX8MP.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index ab93ffd3d2e5..923e7f079f1b 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -82,6 +82,13 @@ properties:
>      description:
>        Phandle to syscon block which provide access for processor enablement
>  
> +  resets:
> +    description:
> +      A pair consisting of phandle to audio-blk-control and an index referencing
> +      the DSP Run/Stall bit in audiomix registers.
> +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -164,6 +171,16 @@ allOf:
>              - const: txdb1
>              - const: rxdb0
>              - const: rxdb1
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - fsl,imx8mp-dsp
> +              - fsl,imx8mp-hifi4
> +    then:
> +      required:
> +        - "resets"
>  
>  additionalProperties: false
>  
> @@ -220,5 +237,5 @@ examples:
>                 <&mu2 3 0>;
>        memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
>                        <&dsp_vdev0vring1>, <&dsp_reserved>;
> -      fsl,dsp-ctrl = <&audio_blk_ctrl>;
> +      resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;

Am I missing something here? fsl,dsp-ctrl is used to get the regmap from syscon.

Best regards,
Alexander

>      };
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Daniel Baluta]

On Thu, Feb 20, 2025 at 12:37 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Mittwoch, 19. Februar 2025, 20:20:56 CET schrieb Daniel Baluta:
> > On i.MX8MP we introduced support for using a reset controller
> > to control DSP operation.
> >
> > This patch adds reset property which is required for i.MX8MP.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > index ab93ffd3d2e5..923e7f079f1b 100644
> > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > @@ -82,6 +82,13 @@ properties:
> >      description:
> >        Phandle to syscon block which provide access for processor enablement
> >
> > +  resets:
> > +    description:
> > +      A pair consisting of phandle to audio-blk-control and an index referencing
> > +      the DSP Run/Stall bit in audiomix registers.
> > +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -164,6 +171,16 @@ allOf:
> >              - const: txdb1
> >              - const: rxdb0
> >              - const: rxdb1
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - fsl,imx8mp-dsp
> > +              - fsl,imx8mp-hifi4
> > +    then:
> > +      required:
> > +        - "resets"
> >
> >  additionalProperties: false
> >
> > @@ -220,5 +237,5 @@ examples:
> >                 <&mu2 3 0>;
> >        memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
> >                        <&dsp_vdev0vring1>, <&dsp_reserved>;
> > -      fsl,dsp-ctrl = <&audio_blk_ctrl>;
> > +      resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
>
> Am I missing something here? fsl,dsp-ctrl is used to get the regmap from syscon.

fsl,dsp-ctrl was used to control the DSP. This functionality for
imx8mp have been now replaced by
using the reset controller.

The example where the diff happens is for imx8mp so for this reason I
changed it to use the
new way of doing the rest.

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Alexander Stein]

Hi,

Am Donnerstag, 20. Februar 2025, 14:11:30 CET schrieb Daniel Baluta:
> On Thu, Feb 20, 2025 at 12:37 PM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> >
> > Hi,
> >
> > Am Mittwoch, 19. Februar 2025, 20:20:56 CET schrieb Daniel Baluta:
> > > On i.MX8MP we introduced support for using a reset controller
> > > to control DSP operation.
> > >
> > > This patch adds reset property which is required for i.MX8MP.
> > >
> > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > > ---
> > >  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > > index ab93ffd3d2e5..923e7f079f1b 100644
> > > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > > @@ -82,6 +82,13 @@ properties:
> > >      description:
> > >        Phandle to syscon block which provide access for processor enablement
> > >
> > > +  resets:
> > > +    description:
> > > +      A pair consisting of phandle to audio-blk-control and an index referencing
> > > +      the DSP Run/Stall bit in audiomix registers.
> > > +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
> > > +    maxItems: 1
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > > @@ -164,6 +171,16 @@ allOf:
> > >              - const: txdb1
> > >              - const: rxdb0
> > >              - const: rxdb1
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - fsl,imx8mp-dsp
> > > +              - fsl,imx8mp-hifi4
> > > +    then:
> > > +      required:
> > > +        - "resets"
> > >
> > >  additionalProperties: false
> > >
> > > @@ -220,5 +237,5 @@ examples:
> > >                 <&mu2 3 0>;
> > >        memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
> > >                        <&dsp_vdev0vring1>, <&dsp_reserved>;
> > > -      fsl,dsp-ctrl = <&audio_blk_ctrl>;
> > > +      resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
> >
> > Am I missing something here? fsl,dsp-ctrl is used to get the regmap from syscon.
> 
> fsl,dsp-ctrl was used to control the DSP. This functionality for
> imx8mp have been now replaced by
> using the reset controller.
> 
> The example where the diff happens is for imx8mp so for this reason I
> changed it to use the
> new way of doing the rest.

Okay, maybe add another example for 8ulp then.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Philipp Zabel]

On Mi, 2025-02-19 at 21:20 +0200, Daniel Baluta wrote:
> On i.MX8MP we introduced support for using a reset controller
> to control DSP operation.
> 
> This patch adds reset property which is required for i.MX8MP.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> index ab93ffd3d2e5..923e7f079f1b 100644
> --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> @@ -82,6 +82,13 @@ properties:
>      description:
>        Phandle to syscon block which provide access for processor enablement
>  
> +  resets:
> +    description:
> +      A pair consisting of phandle to audio-blk-control and an index referencing
> +      the DSP Run/Stall bit in audiomix registers.
> +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
> +    maxItems: 1

This is going to be confusing when there is an actual (undocumented?)
DSP core reset that is not described in the device tree bindings, see
patch 8.

To me this looks like a bit of a gray zone, as I don't know how the
hardware actually works, but if you wouldn't call the Run/Stall bit a
reset, it probably shouldn't be described as such in the device tree
bindings.

I'm not sure. Should both core and runstall reset be described in the
device tree? Or only the core reset, or neither? Either way we should
try not to lie about the hardware here.

regards
Philipp

Re: [PATCH v2 2/8] dt-bindings: dsp: fsl,dsp: Add resets property

[Frank Li]

On Thu, Feb 20, 2025 at 04:45:42PM +0100, Philipp Zabel wrote:
> On Mi, 2025-02-19 at 21:20 +0200, Daniel Baluta wrote:
> > On i.MX8MP we introduced support for using a reset controller
> > to control DSP operation.
> >
> > This patch adds reset property which is required for i.MX8MP.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > index ab93ffd3d2e5..923e7f079f1b 100644
> > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> > @@ -82,6 +82,13 @@ properties:
> >      description:
> >        Phandle to syscon block which provide access for processor enablement
> >
> > +  resets:
> > +    description:
> > +      A pair consisting of phandle to audio-blk-control and an index referencing
> > +      the DSP Run/Stall bit in audiomix registers.
> > +      See include/dt-bindings/reset/imx8mp-reset-audiomix.h for each index meaning.
> > +    maxItems: 1
>
> This is going to be confusing when there is an actual (undocumented?)
> DSP core reset that is not described in the device tree bindings, see
> patch 8.
>
> To me this looks like a bit of a gray zone, as I don't know how the
> hardware actually works, but if you wouldn't call the Run/Stall bit a
> reset, it probably shouldn't be described as such in the device tree
> bindings.

According to some hardware common sense for cpu core. Generally release
RESET Pin to let core runnings. Difference system use difference signal
name. Spec/RM generally copy from hardware sign name. The functionaltiy
is work as core reset.  Release 'reset' let core go. The module is abstract
layer for the function.

>
> I'm not sure. Should both core and runstall reset be described in the
> device tree? Or only the core reset, or neither? Either way we should
> try not to lie about the hardware here.

Not lie about hardware. Try match hardware behavior to existed abstract
module. Hardware is back box, which we often only observe it from outside.
But I can assume hardware implement like

	swtich(state) {
		...
		state = new state;
		...
		case RUNSTALL:
		state = state;
	}

Of course, hardware may use simple gate a input clock to run/stall a core.

Frank

>
> regards
> Philipp

[PATCH v2 3/8] arm64: dts: imx8mp: Add resets to dsp node

[Daniel Baluta]

This change adds resets to dsp node in order to be
able to control the dsp run/stall bit.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index e0d3b8cba221..780245d4ce61 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -6,6 +6,7 @@
 #include <dt-bindings/clock/imx8mp-clock.h>
 #include <dt-bindings/power/imx8mp-power.h>
 #include <dt-bindings/reset/imx8mp-reset.h>
+#include <dt-bindings/reset/imx8mp-reset-audiomix.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interconnect/fsl,imx8mp.h>
@@ -2421,6 +2422,7 @@ dsp: dsp@3b6e8000 {
 			mboxes = <&mu2 2 0>, <&mu2 2 1>,
 				<&mu2 3 0>, <&mu2 3 1>;
 			memory-region = <&dsp_reserved>;
+			resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
 			status = "disabled";
 		};
 	};
-- 
2.25.1

Re: [PATCH v2 3/8] arm64: dts: imx8mp: Add resets to dsp node

[Frank Li]

On Wed, Feb 19, 2025 at 09:20:57PM +0200, Daniel Baluta wrote:
> This change adds resets to dsp node in order to be
> able to control the dsp run/stall bit.

Nit: Add resets to ... and wrap at 75 chars

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index e0d3b8cba221..780245d4ce61 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -6,6 +6,7 @@
>  #include <dt-bindings/clock/imx8mp-clock.h>
>  #include <dt-bindings/power/imx8mp-power.h>
>  #include <dt-bindings/reset/imx8mp-reset.h>
> +#include <dt-bindings/reset/imx8mp-reset-audiomix.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interconnect/fsl,imx8mp.h>
> @@ -2421,6 +2422,7 @@ dsp: dsp@3b6e8000 {
>  			mboxes = <&mu2 2 0>, <&mu2 2 1>,
>  				<&mu2 3 0>, <&mu2 3 1>;
>  			memory-region = <&dsp_reserved>;
> +			resets = <&audio_blk_ctrl IMX8MP_AUDIOMIX_DSP>;
>  			status = "disabled";
>  		};
>  	};
> --
> 2.25.1
>

[PATCH v2 4/8] reset: imx8mp-audiomix: Add prefix for internal macro

[Daniel Baluta]

This adds IMX8MP_AUDIOMIX_ prefix to internal macros in order to show
that specific macros are related to audiomix.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/reset/reset-imx8mp-audiomix.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 6e3f3069f727..1fe21980a66c 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -11,8 +11,8 @@
 #include <linux/of_address.h>
 #include <linux/reset-controller.h>
 
-#define EARC			0x200
-#define EARC_RESET_MASK		0x3
+#define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
+#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
 
 struct imx8mp_audiomix_reset {
 	struct reset_controller_dev rcdev;
@@ -35,8 +35,8 @@ static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
 
 	mask = BIT(id);
 	spin_lock_irqsave(&priv->lock, flags);
-	reg = readl(reg_addr + EARC);
-	writel(reg & ~mask, reg_addr + EARC);
+	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
+	writel(reg & ~mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
@@ -52,8 +52,8 @@ static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
 
 	mask = BIT(id);
 	spin_lock_irqsave(&priv->lock, flags);
-	reg = readl(reg_addr + EARC);
-	writel(reg | mask, reg_addr + EARC);
+	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
+	writel(reg | mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
@@ -78,7 +78,7 @@ static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev,
 	spin_lock_init(&priv->lock);
 
 	priv->rcdev.owner     = THIS_MODULE;
-	priv->rcdev.nr_resets = fls(EARC_RESET_MASK);
+	priv->rcdev.nr_resets = fls(IMX8MP_AUDIOMIX_EARC_RESET_MASK);
 	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
 	priv->rcdev.of_node   = dev->parent->of_node;
 	priv->rcdev.dev	      = dev;
-- 
2.25.1

[PATCH v2 5/8] reset: imx8mp-audiomix: Prepare the code for more reset bits

[Daniel Baluta]

Current code supports EARC PHY Software Reset and EARC 	Software
Reset but it is not easily extensible to more reset bits.

So, refactor the code in order to easily allow more reset bits
in the future.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 drivers/reset/reset-imx8mp-audiomix.c | 42 ++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 1fe21980a66c..17f78ccc7820 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -3,6 +3,7 @@
  * Copyright 2024 NXP
  */
 
+#include <dt-bindings/reset/imx8mp-reset-audiomix.h>
 #include <linux/auxiliary_bus.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -12,7 +13,24 @@
 #include <linux/reset-controller.h>
 
 #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
-#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
+#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		BIT(1)
+#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	BIT(2)
+
+struct imx8mp_reset_map {
+	unsigned int offset;
+	unsigned int mask;
+};
+
+static const struct imx8mp_reset_map reset_map[] = {
+	[IMX8MP_AUDIOMIX_EARC] = {
+		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
+		.mask	= IMX8MP_AUDIOMIX_EARC_RESET_MASK,
+	},
+	[IMX8MP_AUDIOMIX_EARC_PHY] = {
+		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
+		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
+	},
+};
 
 struct imx8mp_audiomix_reset {
 	struct reset_controller_dev rcdev;
@@ -30,13 +48,15 @@ static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
 {
 	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
 	void __iomem *reg_addr = priv->base;
-	unsigned int mask, reg;
+	unsigned int mask, offset, reg;
 	unsigned long flags;
 
-	mask = BIT(id);
+	mask = reset_map[id].mask;
+	offset = reset_map[id].offset;
+
 	spin_lock_irqsave(&priv->lock, flags);
-	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
-	writel(reg & ~mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
+	reg = readl(reg_addr + offset);
+	writel(reg & ~mask, reg_addr + offset);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
@@ -47,13 +67,15 @@ static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
 {
 	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
 	void __iomem *reg_addr = priv->base;
-	unsigned int mask, reg;
+	unsigned int mask, offset, reg;
 	unsigned long flags;
 
-	mask = BIT(id);
+	mask = reset_map[id].mask;
+	offset = reset_map[id].offset;
+
 	spin_lock_irqsave(&priv->lock, flags);
-	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
-	writel(reg | mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
+	reg = readl(reg_addr + offset);
+	writel(reg | mask, reg_addr + offset);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
@@ -78,7 +100,7 @@ static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev,
 	spin_lock_init(&priv->lock);
 
 	priv->rcdev.owner     = THIS_MODULE;
-	priv->rcdev.nr_resets = fls(IMX8MP_AUDIOMIX_EARC_RESET_MASK);
+	priv->rcdev.nr_resets = ARRAY_SIZE(reset_map);
 	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
 	priv->rcdev.of_node   = dev->parent->of_node;
 	priv->rcdev.dev	      = dev;
-- 
2.25.1

Re: [PATCH v2 5/8] reset: imx8mp-audiomix: Prepare the code for more reset bits

[Frank Li]

On Wed, Feb 19, 2025 at 09:20:59PM +0200, Daniel Baluta wrote:
> Current code supports EARC PHY Software Reset and EARC 	Software
> Reset but it is not easily extensible to more reset bits.
>
> So, refactor the code in order to easily allow more reset bits
> in the future.

If respin patch, please wrap at 75 char.

>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 42 ++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 1fe21980a66c..17f78ccc7820 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -3,6 +3,7 @@
>   * Copyright 2024 NXP
>   */
>
> +#include <dt-bindings/reset/imx8mp-reset-audiomix.h>

Add empty line here

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  #include <linux/auxiliary_bus.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
> @@ -12,7 +13,24 @@
>  #include <linux/reset-controller.h>
>
>  #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
> -#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
> +#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		BIT(1)
> +#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	BIT(2)
> +
> +struct imx8mp_reset_map {
> +	unsigned int offset;
> +	unsigned int mask;
> +};
> +
> +static const struct imx8mp_reset_map reset_map[] = {
> +	[IMX8MP_AUDIOMIX_EARC] = {
> +		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
> +		.mask	= IMX8MP_AUDIOMIX_EARC_RESET_MASK,
> +	},
> +	[IMX8MP_AUDIOMIX_EARC_PHY] = {
> +		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
> +		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
> +	},
> +};
>
>  struct imx8mp_audiomix_reset {
>  	struct reset_controller_dev rcdev;
> @@ -30,13 +48,15 @@ static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
>  {
>  	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
>  	void __iomem *reg_addr = priv->base;
> -	unsigned int mask, reg;
> +	unsigned int mask, offset, reg;
>  	unsigned long flags;
>
> -	mask = BIT(id);
> +	mask = reset_map[id].mask;
> +	offset = reset_map[id].offset;
> +
>  	spin_lock_irqsave(&priv->lock, flags);
> -	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
> -	writel(reg & ~mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
> +	reg = readl(reg_addr + offset);
> +	writel(reg & ~mask, reg_addr + offset);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>
>  	return 0;
> @@ -47,13 +67,15 @@ static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
>  {
>  	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
>  	void __iomem *reg_addr = priv->base;
> -	unsigned int mask, reg;
> +	unsigned int mask, offset, reg;
>  	unsigned long flags;
>
> -	mask = BIT(id);
> +	mask = reset_map[id].mask;
> +	offset = reset_map[id].offset;
> +
>  	spin_lock_irqsave(&priv->lock, flags);
> -	reg = readl(reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
> -	writel(reg | mask, reg_addr + IMX8MP_AUDIOMIX_EARC_OFFSET);
> +	reg = readl(reg_addr + offset);
> +	writel(reg | mask, reg_addr + offset);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>
>  	return 0;
> @@ -78,7 +100,7 @@ static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev,
>  	spin_lock_init(&priv->lock);
>
>  	priv->rcdev.owner     = THIS_MODULE;
> -	priv->rcdev.nr_resets = fls(IMX8MP_AUDIOMIX_EARC_RESET_MASK);
> +	priv->rcdev.nr_resets = ARRAY_SIZE(reset_map);
>  	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
>  	priv->rcdev.of_node   = dev->parent->of_node;
>  	priv->rcdev.dev	      = dev;
> --
> 2.25.1
>

[PATCH v2 6/8] reset: imx8mp-audiomix: Introduce active_low configuration option

[Daniel Baluta]

For EARC and EARC PHY the reset happens when clearing the reset bits.
Refactor assert/deassert function in order to take into account the
active_low configuration option.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/reset/reset-imx8mp-audiomix.c | 41 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 17f78ccc7820..42539cafbccc 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -19,16 +19,19 @@
 struct imx8mp_reset_map {
 	unsigned int offset;
 	unsigned int mask;
+	bool active_low;
 };
 
 static const struct imx8mp_reset_map reset_map[] = {
 	[IMX8MP_AUDIOMIX_EARC] = {
 		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
 		.mask	= IMX8MP_AUDIOMIX_EARC_RESET_MASK,
+		.active_low = true,
 	},
 	[IMX8MP_AUDIOMIX_EARC_PHY] = {
 		.offset	= IMX8MP_AUDIOMIX_EARC_OFFSET,
 		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
+		.active_low = true,
 	},
 };
 
@@ -43,42 +46,42 @@ static struct imx8mp_audiomix_reset *to_imx8mp_audiomix_reset(struct reset_contr
 	return container_of(rcdev, struct imx8mp_audiomix_reset, rcdev);
 }
 
-static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
-					unsigned long id)
+static int imx8mp_audiomix_update(struct reset_controller_dev *rcdev,
+				  unsigned long id, bool assert)
 {
 	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
 	void __iomem *reg_addr = priv->base;
-	unsigned int mask, offset, reg;
-	unsigned long flags;
+	unsigned int mask, offset, active_low;
+	unsigned long reg, flags;
 
 	mask = reset_map[id].mask;
 	offset = reset_map[id].offset;
+	active_low = reset_map[id].active_low;
 
 	spin_lock_irqsave(&priv->lock, flags);
+
 	reg = readl(reg_addr + offset);
-	writel(reg & ~mask, reg_addr + offset);
+	if (active_low ^ assert)
+		reg |= mask;
+	else
+		reg &= ~mask;
+	writel(reg, reg_addr + offset);
+
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	return 0;
 }
 
+static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
+					unsigned long id)
+{
+	return imx8mp_audiomix_update(rcdev, id, true);
+}
+
 static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
 					  unsigned long id)
 {
-	struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev);
-	void __iomem *reg_addr = priv->base;
-	unsigned int mask, offset, reg;
-	unsigned long flags;
-
-	mask = reset_map[id].mask;
-	offset = reset_map[id].offset;
-
-	spin_lock_irqsave(&priv->lock, flags);
-	reg = readl(reg_addr + offset);
-	writel(reg | mask, reg_addr + offset);
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	return 0;
+	return imx8mp_audiomix_update(rcdev, id, false);
 }
 
 static const struct reset_control_ops imx8mp_audiomix_reset_ops = {
-- 
2.25.1

[PATCH v2 7/8] reset: imx8mp-audiomix: Add support for DSP run/stall

[Daniel Baluta]

We can Run/Stall the DSP via audio block control bits found in audiomix.
Implement this functionality using the reset controller and use assert
for Stall and deassert for Run.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 drivers/reset/reset-imx8mp-audiomix.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 42539cafbccc..4a713b772429 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -16,6 +16,9 @@
 #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		BIT(1)
 #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	BIT(2)
 
+#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
+#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	BIT(5)
+
 struct imx8mp_reset_map {
 	unsigned int offset;
 	unsigned int mask;
@@ -33,6 +36,11 @@ static const struct imx8mp_reset_map reset_map[] = {
 		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
 		.active_low = true,
 	},
+	[IMX8MP_AUDIOMIX_DSP] = {
+		.offset	= IMX8MP_AUDIOMIX_DSP_OFFSET,
+		.mask	= IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK,
+		.active_low = false,
+	},
 };
 
 struct imx8mp_audiomix_reset {
-- 
2.25.1

Re: [PATCH v2 7/8] reset: imx8mp-audiomix: Add support for DSP run/stall

[Frank Li]

On Wed, Feb 19, 2025 at 09:21:01PM +0200, Daniel Baluta wrote:
> We can Run/Stall the DSP via audio block control bits found in audiomix.
> Implement this functionality using the reset controller and use assert
> for Stall and deassert for Run.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 42539cafbccc..4a713b772429 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -16,6 +16,9 @@
>  #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		BIT(1)
>  #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	BIT(2)
>
> +#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
> +#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	BIT(5)
> +
>  struct imx8mp_reset_map {
>  	unsigned int offset;
>  	unsigned int mask;
> @@ -33,6 +36,11 @@ static const struct imx8mp_reset_map reset_map[] = {
>  		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
>  		.active_low = true,
>  	},
> +	[IMX8MP_AUDIOMIX_DSP] = {
> +		.offset	= IMX8MP_AUDIOMIX_DSP_OFFSET,
> +		.mask	= IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK,
> +		.active_low = false,
> +	},
>  };
>
>  struct imx8mp_audiomix_reset {
> --
> 2.25.1
>

[PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Daniel Baluta]

Use the reset controller API to control the DSP on i.MX8MP. This way
we can have a better control of the resources and avoid using a syscon
to access the audiomix bits.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
 drivers/remoteproc/imx_rproc.h     |  2 ++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index ea5024919c2f..631563e4f86d 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -19,6 +19,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 
 #include "imx_rproc.h"
@@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
  */
 struct imx_dsp_rproc {
 	struct regmap				*regmap;
+	struct reset_control			*reset;
 	struct rproc				*rproc;
 	const struct imx_dsp_rproc_dcfg		*dsp_dcfg;
 	struct clk_bulk_data			clks[DSP_RPROC_CLK_MAX];
@@ -192,9 +194,7 @@ static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv)
 	/* Keep reset asserted for 10 cycles */
 	usleep_range(1, 2);
 
-	regmap_update_bits(priv->regmap, IMX8M_AudioDSP_REG2,
-			   IMX8M_AudioDSP_REG2_RUNSTALL,
-			   IMX8M_AudioDSP_REG2_RUNSTALL);
+	reset_control_assert(priv->reset);
 
 	/* Take the DSP out of reset and keep stalled for FW loading */
 	pwrctl = readl(dap + IMX8M_DAP_PWRCTL);
@@ -231,13 +231,9 @@ static int imx8ulp_dsp_reset(struct imx_dsp_rproc *priv)
 
 /* Specific configuration for i.MX8MP */
 static const struct imx_rproc_dcfg dsp_rproc_cfg_imx8mp = {
-	.src_reg	= IMX8M_AudioDSP_REG2,
-	.src_mask	= IMX8M_AudioDSP_REG2_RUNSTALL,
-	.src_start	= 0,
-	.src_stop	= IMX8M_AudioDSP_REG2_RUNSTALL,
 	.att		= imx_dsp_rproc_att_imx8mp,
 	.att_size	= ARRAY_SIZE(imx_dsp_rproc_att_imx8mp),
-	.method		= IMX_RPROC_MMIO,
+	.method		= IMX_RPROC_RESET_CONTROLLER,
 };
 
 static const struct imx_dsp_rproc_dcfg imx_dsp_rproc_cfg_imx8mp = {
@@ -329,6 +325,9 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
 					  true,
 					  rproc->bootaddr);
 		break;
+	case IMX_RPROC_RESET_CONTROLLER:
+		ret = reset_control_deassert(priv->reset);
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -369,6 +368,9 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
 					  false,
 					  rproc->bootaddr);
 		break;
+	case IMX_RPROC_RESET_CONTROLLER:
+		ret = reset_control_assert(priv->reset);
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -995,6 +997,13 @@ static int imx_dsp_rproc_detect_mode(struct imx_dsp_rproc *priv)
 
 		priv->regmap = regmap;
 		break;
+	case IMX_RPROC_RESET_CONTROLLER:
+		priv->reset = devm_reset_control_get_exclusive(dev, NULL);
+		if (IS_ERR(priv->reset)) {
+			dev_err(dev, "Failed to get DSP reset control\n");
+			return PTR_ERR(priv->reset);
+		}
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
index 17a7d051c531..cfd38d37e146 100644
--- a/drivers/remoteproc/imx_rproc.h
+++ b/drivers/remoteproc/imx_rproc.h
@@ -24,6 +24,8 @@ enum imx_rproc_method {
 	IMX_RPROC_SMC,
 	/* Through System Control Unit API */
 	IMX_RPROC_SCU_API,
+	/* Through Reset Controller API */
+	IMX_RPROC_RESET_CONTROLLER,
 };
 
 /* dcfg flags */
-- 
2.25.1

Re: [PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Frank Li]

On Wed, Feb 19, 2025 at 09:21:02PM +0200, Daniel Baluta wrote:
> Use the reset controller API to control the DSP on i.MX8MP. This way
> we can have a better control of the resources and avoid using a syscon
> to access the audiomix bits.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
>  drivers/remoteproc/imx_rproc.h     |  2 ++
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index ea5024919c2f..631563e4f86d 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -19,6 +19,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>
>  #include "imx_rproc.h"
> @@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
>   */
>  struct imx_dsp_rproc {
>  	struct regmap				*regmap;
> +	struct reset_control			*reset;
>  	struct rproc				*rproc;
>  	const struct imx_dsp_rproc_dcfg		*dsp_dcfg;
>  	struct clk_bulk_data			clks[DSP_RPROC_CLK_MAX];
> @@ -192,9 +194,7 @@ static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv)
>  	/* Keep reset asserted for 10 cycles */
>  	usleep_range(1, 2);
>
> -	regmap_update_bits(priv->regmap, IMX8M_AudioDSP_REG2,
> -			   IMX8M_AudioDSP_REG2_RUNSTALL,
> -			   IMX8M_AudioDSP_REG2_RUNSTALL);
> +	reset_control_assert(priv->reset);
>
>  	/* Take the DSP out of reset and keep stalled for FW loading */
>  	pwrctl = readl(dap + IMX8M_DAP_PWRCTL);
> @@ -231,13 +231,9 @@ static int imx8ulp_dsp_reset(struct imx_dsp_rproc *priv)
>
>  /* Specific configuration for i.MX8MP */
>  static const struct imx_rproc_dcfg dsp_rproc_cfg_imx8mp = {
> -	.src_reg	= IMX8M_AudioDSP_REG2,
> -	.src_mask	= IMX8M_AudioDSP_REG2_RUNSTALL,
> -	.src_start	= 0,
> -	.src_stop	= IMX8M_AudioDSP_REG2_RUNSTALL,
>  	.att		= imx_dsp_rproc_att_imx8mp,
>  	.att_size	= ARRAY_SIZE(imx_dsp_rproc_att_imx8mp),
> -	.method		= IMX_RPROC_MMIO,
> +	.method		= IMX_RPROC_RESET_CONTROLLER,
>  };
>
>  static const struct imx_dsp_rproc_dcfg imx_dsp_rproc_cfg_imx8mp = {
> @@ -329,6 +325,9 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
>  					  true,
>  					  rproc->bootaddr);
>  		break;
> +	case IMX_RPROC_RESET_CONTROLLER:
> +		ret = reset_control_deassert(priv->reset);
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -369,6 +368,9 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
>  					  false,
>  					  rproc->bootaddr);
>  		break;
> +	case IMX_RPROC_RESET_CONTROLLER:
> +		ret = reset_control_assert(priv->reset);
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -995,6 +997,13 @@ static int imx_dsp_rproc_detect_mode(struct imx_dsp_rproc *priv)
>
>  		priv->regmap = regmap;
>  		break;
> +	case IMX_RPROC_RESET_CONTROLLER:
> +		priv->reset = devm_reset_control_get_exclusive(dev, NULL);
> +		if (IS_ERR(priv->reset)) {
> +			dev_err(dev, "Failed to get DSP reset control\n");
> +			return PTR_ERR(priv->reset);
> +		}
> +		break;
>  	default:
>  		ret = -EOPNOTSUPP;
>  		break;
> diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
> index 17a7d051c531..cfd38d37e146 100644
> --- a/drivers/remoteproc/imx_rproc.h
> +++ b/drivers/remoteproc/imx_rproc.h
> @@ -24,6 +24,8 @@ enum imx_rproc_method {
>  	IMX_RPROC_SMC,
>  	/* Through System Control Unit API */
>  	IMX_RPROC_SCU_API,
> +	/* Through Reset Controller API */
> +	IMX_RPROC_RESET_CONTROLLER,
>  };
>
>  /* dcfg flags */
> --
> 2.25.1
>

Re: [PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Laurentiu Mihalcea]

On 2/19/2025 9:21 PM, Daniel Baluta wrote:
> Use the reset controller API to control the DSP on i.MX8MP. This way
> we can have a better control of the resources and avoid using a syscon
> to access the audiomix bits.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
>  drivers/remoteproc/imx_rproc.h     |  2 ++
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index ea5024919c2f..631563e4f86d 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -19,6 +19,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  
>  #include "imx_rproc.h"
> @@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
>   */
>  struct imx_dsp_rproc {
>  	struct regmap				*regmap;
> +	struct reset_control			*reset;

Maybe rename this to "stall"? There's also the DAP stuff that's actually used
to reset the core so this might be a bit confusing?

Re: [PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Philipp Zabel]

On Do, 2025-02-20 at 00:22 +0200, Laurentiu Mihalcea wrote:
> 
> On 2/19/2025 9:21 PM, Daniel Baluta wrote:
> > Use the reset controller API to control the DSP on i.MX8MP. This way
> > we can have a better control of the resources and avoid using a syscon
> > to access the audiomix bits.
> > 
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
> >  drivers/remoteproc/imx_rproc.h     |  2 ++
> >  2 files changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > index ea5024919c2f..631563e4f86d 100644
> > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >  #include <linux/remoteproc.h>
> > +#include <linux/reset.h>
> >  #include <linux/slab.h>
> >  
> >  #include "imx_rproc.h"
> > @@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
> >   */
> >  struct imx_dsp_rproc {
> >  	struct regmap				*regmap;
> > +	struct reset_control			*reset;
> 
> Maybe rename this to "stall"? There's also the DAP stuff that's actually used
> to reset the core so this might be a bit confusing?

So Run/Stall does not actually reset anything? Maybe then it should not
be modeled as a reset control. It looks to me like the
DAP_PWRCTL[CORERESET] bit in the Audio Processor Debug region would be
a much better fit.

regards
Philipp

Re: [PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Laurentiu Mihalcea]

On 2/20/2025 5:45 PM, Philipp Zabel wrote:
> On Do, 2025-02-20 at 00:22 +0200, Laurentiu Mihalcea wrote:
>> On 2/19/2025 9:21 PM, Daniel Baluta wrote:
>>> Use the reset controller API to control the DSP on i.MX8MP. This way
>>> we can have a better control of the resources and avoid using a syscon
>>> to access the audiomix bits.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
>>>  drivers/remoteproc/imx_rproc.h     |  2 ++
>>>  2 files changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>>> index ea5024919c2f..631563e4f86d 100644
>>> --- a/drivers/remoteproc/imx_dsp_rproc.c
>>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
>>> @@ -19,6 +19,7 @@
>>>  #include <linux/pm_runtime.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/remoteproc.h>
>>> +#include <linux/reset.h>
>>>  #include <linux/slab.h>
>>>  
>>>  #include "imx_rproc.h"
>>> @@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
>>>   */
>>>  struct imx_dsp_rproc {
>>>  	struct regmap				*regmap;
>>> +	struct reset_control			*reset;
>> Maybe rename this to "stall"? There's also the DAP stuff that's actually used
>> to reset the core so this might be a bit confusing?
> So Run/Stall does not actually reset anything? Maybe then it should not
> be modeled as a reset control. It looks to me like the
> DAP_PWRCTL[CORERESET] bit in the Audio Processor Debug region would be
> a much better fit.

Yep, it does not as far as I'm aware. This bit is used to insert stall cycles
into the accelerator's pipeline. As for the DAP bit: agreed.

(Daniel pls feel free to correct me if I got something wrong here)

For a bit of context here:

Previous approach used a syscon to manage the Run/Stall bit (see [1], "fsl,dsp-ctrl"
property). The main issue with that is that syscon doesn't enforce any device dependency
(e.g: PM) and you do need the AUDIOMIX power domain to be powered on before accessing
the registers from said syscon.

Now, AUDIO_BLK_CTRL offers multiple DSP-related configuration bits (Run/Stall bit included).
If we ever decide we want to access those bits we can't model them as reset controllers as it
makes no sense whatsoever. As such, I think that modelling Run/Stall as a reset controller is
just an arguably unneeded and inaccurate (assuming reset controllers are supposed
to model only actual reset signals) solution.

IMO, unless someone can think of a scenario in which this would break, we should just cut our
losses and go back to the syscon-based approach. The DSP is already attached to the AUDIOMIX
power domain so it should be on when attempting to access its registers. Also, the DSP
should be the only device wanting to configure the DSP-related AUDIO_BLK_CTRL bits anyways?

What are your thoughts on this?


[1]: https://lore.kernel.org/imx/20250212085222.107102-1-daniel.baluta@nxp.com/

Thanks,
Laurentiu

Re: [PATCH v2 8/8] imx_dsp_rproc: Use reset controller API to control the DSP

[Daniel Baluta]

On Fri, Feb 21, 2025 at 3:20 AM Laurentiu Mihalcea
<laurentiumihalcea111@gmail.com> wrote:
>
>
>
> On 2/20/2025 5:45 PM, Philipp Zabel wrote:
> > On Do, 2025-02-20 at 00:22 +0200, Laurentiu Mihalcea wrote:
> >> On 2/19/2025 9:21 PM, Daniel Baluta wrote:
> >>> Use the reset controller API to control the DSP on i.MX8MP. This way
> >>> we can have a better control of the resources and avoid using a syscon
> >>> to access the audiomix bits.
> >>>
> >>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>  drivers/remoteproc/imx_dsp_rproc.c | 25 +++++++++++++++++--------
> >>>  drivers/remoteproc/imx_rproc.h     |  2 ++
> >>>  2 files changed, 19 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> >>> index ea5024919c2f..631563e4f86d 100644
> >>> --- a/drivers/remoteproc/imx_dsp_rproc.c
> >>> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> >>> @@ -19,6 +19,7 @@
> >>>  #include <linux/pm_runtime.h>
> >>>  #include <linux/regmap.h>
> >>>  #include <linux/remoteproc.h>
> >>> +#include <linux/reset.h>
> >>>  #include <linux/slab.h>
> >>>
> >>>  #include "imx_rproc.h"
> >>> @@ -111,6 +112,7 @@ enum imx_dsp_rp_mbox_messages {
> >>>   */
> >>>  struct imx_dsp_rproc {
> >>>     struct regmap                           *regmap;
> >>> +   struct reset_control                    *reset;
> >> Maybe rename this to "stall"? There's also the DAP stuff that's actually used
> >> to reset the core so this might be a bit confusing?
> > So Run/Stall does not actually reset anything? Maybe then it should not
> > be modeled as a reset control. It looks to me like the
> > DAP_PWRCTL[CORERESET] bit in the Audio Processor Debug region would be
> > a much better fit.
>
> Yep, it does not as far as I'm aware. This bit is used to insert stall cycles
> into the accelerator's pipeline. As for the DAP bit: agreed.
>
> (Daniel pls feel free to correct me if I got something wrong here)
>
> For a bit of context here:
>
> Previous approach used a syscon to manage the Run/Stall bit (see [1], "fsl,dsp-ctrl"
> property). The main issue with that is that syscon doesn't enforce any device dependency
> (e.g: PM) and you do need the AUDIOMIX power domain to be powered on before accessing
> the registers from said syscon.
>
> Now, AUDIO_BLK_CTRL offers multiple DSP-related configuration bits (Run/Stall bit included).
> If we ever decide we want to access those bits we can't model them as reset controllers as it
> makes no sense whatsoever. As such, I think that modelling Run/Stall as a reset controller is
> just an arguably unneeded and inaccurate (assuming reset controllers are supposed
> to model only actual reset signals) solution.
>
> IMO, unless someone can think of a scenario in which this would break, we should just cut our
> losses and go back to the syscon-based approach. The DSP is already attached to the AUDIOMIX
> power domain so it should be on when attempting to access its registers. Also, the DSP
> should be the only device wanting to configure the DSP-related AUDIO_BLK_CTRL bits anyways?
>
> What are your thoughts on this?

Thanks Laurentiu. Your summary is spot on.

Controlling DSP is done via two memory areas:
 (1) Audiomix area for Run/Stall bits
 (2) Debug Acces Port (DAP)  area for software reset

We need to model this in our drivers and we need both of the areas.

For (1) the DSP node needs somehow to refer to audiomix dt node and our
initial choice was to use a syscon binding to Audiomix. This got us a NACK
from Frank  and  Krzysztof as we naturally should always bring syscon in
discussion if we can model the behavior using an existing system like reset
for example.

Modelling the Run/Stall bits as a reset controller is a little bit in
the grey area
but Frank Li made some valid points in the patch 2/8 of the series.

Daniel.