On Fri, Feb 21, 2025 at 3:20 AM Laurentiu Mihalcea
<laurentiumihalcea111@xxxxxxxxx> 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@xxxxxxx>
> >>> Reviewed-by: Peng Fan <peng.fan@xxxxxxx>
> >>> ---
> >>> 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.
