| Message ID | 20250219192102.423850-9-daniel.baluta@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: Add support to Run/Stall DSP via reset API | expand |
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 >
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?
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
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
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.
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 */