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

[Laurentiu Mihalcea <laurentiumihalcea111@gmail.com>]

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