| Message ID | 20250225102005.408773-3-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: Add support to Run/Stall DSP via reset API | expand |
On Di, 2025-02-25 at 12:19 +0200, Daniel Baluta wrote: > Assert and deassert functionality of the DSP found on i.MX8MP is > realized by combining control bits from two modules: Audio Block > Control and Debug Access Port. > > Audio block control bits are used to for Run/Stall the DSP core > while the DAP bits are used for software reset the core. > > The original plan was to use fsl,dsp-ctrl property and to refer the > audiomix bits via syscon interface. This proposal received NACK from > community we shouldn't abuse the syscon interface [1]. > > So remove fsl,dsp-ctrl property for i.MX8MP and use reset control > interface instead. > > [1] https://patchwork.kernel.org/project/imx/patch/20250212085222.107102-6-daniel.baluta@nxp.com/ > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > .../devicetree/bindings/dsp/fsl,dsp.yaml | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml > index ab93ffd3d2e5..b3550c9d12e7 100644 > --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml > +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml > @@ -82,6 +82,9 @@ properties: > description: > Phandle to syscon block which provide access for processor enablement > > + resets: > + maxItems: 1 The DAP core reset is mentioned in the commit message. Why is it missing here? After reading the discussion in [1], I'd expect both the stall and the (core) reset signal to be documented, something like: resets: maxItems: 2 reset-names: items: - const: core - const: stall > + > required: > - compatible > - reg > @@ -164,6 +167,16 @@ allOf: > - const: txdb1 > - const: rxdb0 > - const: rxdb1 > + - if: > + properties: > + compatible: > + contains: > + enum: > + - fsl,imx8mp-dsp > + - fsl,imx8mp-hifi4 > + then: > + required: > + - resets > > additionalProperties: false > > @@ -186,6 +199,7 @@ examples: > }; > - | > #include <dt-bindings/clock/imx8mp-clock.h> > + #include <dt-bindings/reset/imx8mp-reset-audiomix.h> > dsp_reserved: dsp@92400000 { > reg = <0x92400000 0x1000000>; > no-map; > @@ -220,5 +234,5 @@ examples: > <&mu2 3 0>; > memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>, > <&dsp_vdev0vring1>, <&dsp_reserved>; > - fsl,dsp-ctrl = <&audio_blk_ctrl>; Is there nothing else in this range that will have to be controlled by the DSP driver in the future, such as the IMPWIRE register or the XOCDMODE[OCDHALTONRESET] bit? regards Philipp
Hello Philipp, Thanks for your comments! > The DAP core reset is mentioned in the commit message. Why is it > missing here? After reading the discussion in [1], I'd expect both the > stall and the (core) reset signal to be documented, something like: There is no reset controller driver for DAP area yet. We manipulate the bits directly by remapping the DAP address space inside remoteproc driver. See for example: drivers/remoteproc/imx_dsp_rproc.c /* Reset function for DSP on i.MX8MP */ static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv) { » void __iomem *dap = ioremap_wc(IMX8M_DAP_DEBUG, IMX8M_DAP_DEBUG_SIZE); » int pwrctl; » /* Put DSP into reset and stall */ » pwrctl = readl(dap + IMX8M_DAP_PWRCTL); » pwrctl |= IMX8M_PWRCTL_CORERESET; » writel(pwrctl, dap + IMX8M_DAP_PWRCTL); If we agree that this is the right way to go, the next step would be to create a new reset controller driver for DAP area. I want to keep this as a follow up patch in order to not compilate this patch series even more. <snip> > > memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>, > > <&dsp_vdev0vring1>, <&dsp_reserved>; > > - fsl,dsp-ctrl = <&audio_blk_ctrl>; > > Is there nothing else in this range that will have to be controlled by > the DSP driver in the future, such as the IMPWIRE register or the > XOCDMODE[OCDHALTONRESET] bit? We are internally running SOF for couple of years now and we didn't need any of these bits.
Hi Daniel, On Di, 2025-02-25 at 15:41 +0200, Daniel Baluta wrote: > Hello Philipp, > > Thanks for your comments! > > > The DAP core reset is mentioned in the commit message. Why is it > > missing here? After reading the discussion in [1], I'd expect both the > > stall and the (core) reset signal to be documented, something like: > > There is no reset controller driver for DAP area yet. This is about the device tree bindings, not the driver. Even if the driver maps DAP address space with a hard-coded address, ... > We manipulate the bits directly by remapping the DAP address space > inside remoteproc driver. ... which it should not, the bindings could already correctly describe the core reset. I'd just like to make sure that there will be no confusion about the stall "reset" signal, and adding a reset-names property would be an easy way to do it. > See for example: drivers/remoteproc/imx_dsp_rproc.c > > /* Reset function for DSP on i.MX8MP */ > static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv) > { > » void __iomem *dap = ioremap_wc(IMX8M_DAP_DEBUG, > IMX8M_DAP_DEBUG_SIZE); > » int pwrctl; > > » /* Put DSP into reset and stall */ > » pwrctl = readl(dap + IMX8M_DAP_PWRCTL); > » pwrctl |= IMX8M_PWRCTL_CORERESET; > » writel(pwrctl, dap + IMX8M_DAP_PWRCTL); > > > If we agree that this is the right way to go, the next step would be > to create a new reset controller driver for DAP area. > > I want to keep this as a follow up patch in order to not compilate > this patch series even more. I have no issues with adding a reset driver for the DAP region and hooking it up to the DSP driver in a separate series, but the device tree bindings could be correct from the start. > > Is there nothing else in this range that will have to be controlled by > > the DSP driver in the future, such as the IMPWIRE register or the > > XOCDMODE[OCDHALTONRESET] bit? > > We are internally running SOF for couple of years now and we didn't > need any of these bits. Ok. regards Philipp
diff --git a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml index ab93ffd3d2e5..b3550c9d12e7 100644 --- a/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml +++ b/Documentation/devicetree/bindings/dsp/fsl,dsp.yaml @@ -82,6 +82,9 @@ properties: description: Phandle to syscon block which provide access for processor enablement + resets: + maxItems: 1 + required: - compatible - reg @@ -164,6 +167,16 @@ allOf: - const: txdb1 - const: rxdb0 - const: rxdb1 + - if: + properties: + compatible: + contains: + enum: + - fsl,imx8mp-dsp + - fsl,imx8mp-hifi4 + then: + required: + - resets additionalProperties: false @@ -186,6 +199,7 @@ examples: }; - | #include <dt-bindings/clock/imx8mp-clock.h> + #include <dt-bindings/reset/imx8mp-reset-audiomix.h> dsp_reserved: dsp@92400000 { reg = <0x92400000 0x1000000>; no-map; @@ -220,5 +234,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>; };