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

[PATCH 0/5] 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.

Patches 1-4 apply to reset-imx8mp-audiomix (should go via reset
controller tree)
 * refactor code, introduce active_low configuration option and then
   add support to Run/Stall DSP.

Patch 5 applies to imx_dsp_rproc driver (should go via REMOTEPROC tree)
 * use reset controller API to control the DSP instead of directly
   touching the audiomix registers via a syscon.

Using reset controller API was suggested by Frank Li and Krzysztof
Kozlowski in the following discussion thread:

https://patchwork.kernel.org/project/imx/patch/20241210125338.104959-6-daniel.baluta@nxp.com/

Daniel Baluta (5):
  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

 drivers/remoteproc/imx_dsp_rproc.c    | 25 +++++---
 drivers/remoteproc/imx_rproc.h        |  2 +
 drivers/reset/reset-imx8mp-audiomix.c | 86 ++++++++++++++++++++-------
 3 files changed, 83 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/5] 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>
---
 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 2/5] 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 | 53 ++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 1fe21980a66c..6b1666c4e069 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -12,7 +12,30 @@
 #include <linux/reset-controller.h>
 
 #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
-#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
+#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
+#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
+
+#define IMX8MP_AUDIOMIX_EARC		0
+#define IMX8MP_AUDIOMIX_EARC_PHY	1
+
+#define IMX8MP_AUDIOMIX_RESET_NUM	2
+
+struct imx8mp_reset_map {
+	unsigned int offset;
+	unsigned int mask;
+};
+
+static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
+	[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 +53,18 @@ 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);
+	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
+		return -EINVAL;
+
+	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 +75,18 @@ 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);
+	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
+		return -EINVAL;
+
+	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 +111,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 = IMX8MP_AUDIOMIX_RESET_NUM;
 	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
 	priv->rcdev.of_node   = dev->parent->of_node;
 	priv->rcdev.dev	      = dev;
-- 
2.25.1

[PATCH 3/5] 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 configuratin option.

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

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 6b1666c4e069..8cc0a6b58cbc 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -23,16 +23,19 @@
 struct imx8mp_reset_map {
 	unsigned int offset;
 	unsigned int mask;
+	bool active_low;
 };
 
 static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
 	[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,
 	},
 
 };
@@ -48,48 +51,46 @@ 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;
 
 	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
 		return -EINVAL;
 
 	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;
-
-	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
-		return -EINVAL;
-
-	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 4/5] 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
index 8cc0a6b58cbc..ee56d52a7278 100644
--- a/drivers/reset/reset-imx8mp-audiomix.c
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -15,10 +15,14 @@
 #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
 #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
 
+#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
+#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	0x20
+
 #define IMX8MP_AUDIOMIX_EARC		0
 #define IMX8MP_AUDIOMIX_EARC_PHY	1
+#define IMX8MP_AUDIOMIX_DSP		2
 
-#define IMX8MP_AUDIOMIX_RESET_NUM	2
+#define IMX8MP_AUDIOMIX_RESET_NUM	3
 
 struct imx8mp_reset_map {
 	unsigned int offset;
@@ -37,7 +41,11 @@ static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
 		.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 5/5] 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>
---
 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..2b97e4d0bb9e 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_optional_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 1/5] reset: imx8mp-audiomix: Add prefix for internal macro

[Philipp Zabel]

On Di, 2025-02-18 at 10:57 +0200, Daniel Baluta wrote:
> 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>

regards
Philipp

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

[Philipp Zabel]

Hi Daniel,

On Di, 2025-02-18 at 10:57 +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.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 53 ++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 1fe21980a66c..6b1666c4e069 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -12,7 +12,30 @@
>  #include <linux/reset-controller.h>
>  
>  #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
> -#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
> +#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
> +#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2

Will any of the reset controls manipulate multiple bits at once?
I'd use BIT(0) and BIT(1) here.

> +
> +#define IMX8MP_AUDIOMIX_EARC		0
> +#define IMX8MP_AUDIOMIX_EARC_PHY	1
> +
> +#define IMX8MP_AUDIOMIX_RESET_NUM	2
> +
> +struct imx8mp_reset_map {
> +	unsigned int offset;
> +	unsigned int mask;
> +};
> +
> +static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {

If you make this reset_map[], drop IMX8MP_AUDIOMIX_RESET_NUM, and use
.nr_resets = ARRAY_SIZE(reset_map) below, followup patches that add new
bits will be simplified.

> +	[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,
> +	},
> +

Please drop this empty line.

> +};
>  
>  struct imx8mp_audiomix_reset {
>  	struct reset_controller_dev rcdev;
> @@ -30,13 +53,18 @@ 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);
> +	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)

Whitespace error. But also, this check is not necessary.
The reset core will not return reset controls that fail the same check
in of_reset_simple_xlate(), so we can never get here with the wrong id
if rcdev.nr_resets correctly is set to ARRAY_SIZE(reset_map).

> +		return -EINVAL;
> +
> +	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 +75,18 @@ 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);
> +	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
> +		return -EINVAL;

Same as above.

> +
> +	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 +111,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 = IMX8MP_AUDIOMIX_RESET_NUM;

Could use ARRAY_SIZE(reset_map) here.

regards
Philipp

Re: [PATCH 3/5] reset: imx8mp-audiomix: Introduce active_low configuration option

[Philipp Zabel]

On Di, 2025-02-18 at 10:57 +0200, Daniel Baluta wrote:
> 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 configuratin option.
                            ^
                            missing 'o'.

> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 45 ++++++++++++++-------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 6b1666c4e069..8cc0a6b58cbc 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -23,16 +23,19 @@
>  struct imx8mp_reset_map {
>  	unsigned int offset;
>  	unsigned int mask;
> +	bool active_low;
>  };
>  
>  static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
>  	[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,
>  	},
>  
>  };
> @@ -48,48 +51,46 @@ 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;

Nitpick, I would make active_low bool, like assert. Otherwise,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 4/5] reset: imx8mp-audiomix: Add support for DSP run/stall

[Philipp Zabel]

On Di, 2025-02-18 at 10:57 +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>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 8cc0a6b58cbc..ee56d52a7278 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -15,10 +15,14 @@
>  #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
>  #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
>  
> +#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
> +#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	0x20
> +
>  #define IMX8MP_AUDIOMIX_EARC		0
>  #define IMX8MP_AUDIOMIX_EARC_PHY	1
> +#define IMX8MP_AUDIOMIX_DSP		2
>  
> -#define IMX8MP_AUDIOMIX_RESET_NUM	2
> +#define IMX8MP_AUDIOMIX_RESET_NUM	3

See patch 2, this could be removed.

>  
>  struct imx8mp_reset_map {
>  	unsigned int offset;
> @@ -37,7 +41,11 @@ static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
>  		.mask	= IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK,
>  		.active_low = true,
>  	},
> -

This belongs in patch 2.

> +	[IMX8MP_AUDIOMIX_DSP] = {
> +		.offset	= IMX8MP_AUDIOMIX_DSP_OFFSET,
> +		.mask	= IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK,
> +		.active_low = false,
> +	},
>  };
>  
>  struct imx8mp_audiomix_reset {

Otherwise,
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

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

[Philipp Zabel]

On Di, 2025-02-18 at 10:57 +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>
> ---
>  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..2b97e4d0bb9e 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_optional_exclusive(dev, NULL);

Is this optional on purpose? There is no mention of it in the commit
message. Where is this resets property documented in the dt-bindings?

regards
Philipp

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

[Peng Fan]

On Tue, Feb 18, 2025 at 10:57:09AM +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.
>
>Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>---
> drivers/reset/reset-imx8mp-audiomix.c | 53 ++++++++++++++++++++++-----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
>index 1fe21980a66c..6b1666c4e069 100644
>--- a/drivers/reset/reset-imx8mp-audiomix.c
>+++ b/drivers/reset/reset-imx8mp-audiomix.c
>@@ -12,7 +12,30 @@
> #include <linux/reset-controller.h>
> 
> #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
>-#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
>+#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
>+#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
>+
>+#define IMX8MP_AUDIOMIX_EARC		0
>+#define IMX8MP_AUDIOMIX_EARC_PHY	1

Should this two lines be put in dt-bindings?

>+
>+#define IMX8MP_AUDIOMIX_RESET_NUM	2
>+
>+struct imx8mp_reset_map {
>+	unsigned int offset;
>+	unsigned int mask;
>+};
>+
>+static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
>+	[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,
>+	},
>+

blank line

>+};
> 

Regards,
Peng

Re: [PATCH 3/5] reset: imx8mp-audiomix: Introduce active_low configuration option

[Peng Fan]

On Tue, Feb 18, 2025 at 10:57:10AM +0200, Daniel Baluta wrote:
>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 configuratin option.
>
>Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>---
> drivers/reset/reset-imx8mp-audiomix.c | 45 ++++++++++++++-------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
>index 6b1666c4e069..8cc0a6b58cbc 100644
>--- a/drivers/reset/reset-imx8mp-audiomix.c
>+++ b/drivers/reset/reset-imx8mp-audiomix.c
>@@ -23,16 +23,19 @@
> struct imx8mp_reset_map {
> 	unsigned int offset;
> 	unsigned int mask;
>+	bool active_low;

How about using u32 flags?
Then it will be easy to extend to add new FLAG in future in case.

If you tend to use bool, also ok to me:

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 4/5] reset: imx8mp-audiomix: Add support for DSP run/stall

[Peng Fan]

On Tue, Feb 18, 2025 at 10:57:11AM +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>
>---
> drivers/reset/reset-imx8mp-audiomix.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
>index 8cc0a6b58cbc..ee56d52a7278 100644
>--- a/drivers/reset/reset-imx8mp-audiomix.c
>+++ b/drivers/reset/reset-imx8mp-audiomix.c
>@@ -15,10 +15,14 @@
> #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
> #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
> 
>+#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
>+#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	0x20
>+
> #define IMX8MP_AUDIOMIX_EARC		0
> #define IMX8MP_AUDIOMIX_EARC_PHY	1
>+#define IMX8MP_AUDIOMIX_DSP		2

Move this to dt-binding?

Regards,
Peng

Re: [PATCH 1/5] reset: imx8mp-audiomix: Add prefix for internal macro

[Frank Li]

On Tue, Feb 18, 2025 at 10:57:08AM +0200, Daniel Baluta wrote:
> This adds IMX8MP_AUDIOMIX_ prefix to internal macros
> in order to show that specific macros are related to
> audiomix.

nit: warp at 75 chars

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Daniel Baluta <daniel.baluta@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
>

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

[Frank Li]

On Tue, Feb 18, 2025 at 10:57:09AM +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.

Nit: wrap at 75 chars

>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 53 ++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 1fe21980a66c..6b1666c4e069 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -12,7 +12,30 @@
>  #include <linux/reset-controller.h>
>
>  #define IMX8MP_AUDIOMIX_EARC_OFFSET		0x200
> -#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x3
> +#define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
> +#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
> +
> +#define IMX8MP_AUDIOMIX_EARC		0
> +#define IMX8MP_AUDIOMIX_EARC_PHY	1


Does dt binding need such macro?

If not, you can use enum.
If yes, create binding header file for it.

Frank
> +
> +#define IMX8MP_AUDIOMIX_RESET_NUM	2
> +
> +struct imx8mp_reset_map {
> +	unsigned int offset;
> +	unsigned int mask;
> +};
> +
> +static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
> +	[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 +53,18 @@ 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);
> +	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
> +		return -EINVAL;
> +
> +	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 +75,18 @@ 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);
> +	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
> +		return -EINVAL;
> +
> +	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 +111,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 = IMX8MP_AUDIOMIX_RESET_NUM;
>  	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
>  	priv->rcdev.of_node   = dev->parent->of_node;
>  	priv->rcdev.dev	      = dev;
> --
> 2.25.1
>

Re: [PATCH 3/5] reset: imx8mp-audiomix: Introduce active_low configuration option

[Frank Li]

On Tue, Feb 18, 2025 at 10:30:21AM +0100, Philipp Zabel wrote:
> On Di, 2025-02-18 at 10:57 +0200, Daniel Baluta wrote:
> > 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 configuratin option.
>                             ^
>                             missing 'o'.

run ./scripts/checkpatch.pl -g HEAD --strict --codespell

Frank
>
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  drivers/reset/reset-imx8mp-audiomix.c | 45 ++++++++++++++-------------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> > index 6b1666c4e069..8cc0a6b58cbc 100644
> > --- a/drivers/reset/reset-imx8mp-audiomix.c
> > +++ b/drivers/reset/reset-imx8mp-audiomix.c
> > @@ -23,16 +23,19 @@
> >  struct imx8mp_reset_map {
> >  	unsigned int offset;
> >  	unsigned int mask;
> > +	bool active_low;
> >  };
> >
> >  static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
> >  	[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,
> >  	},
> >
> >  };
> > @@ -48,48 +51,46 @@ 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;
>
> Nitpick, I would make active_low bool, like assert. Otherwise,
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> regards
> Philipp

Re: [PATCH 3/5] reset: imx8mp-audiomix: Introduce active_low configuration option

[Frank Li]

On Tue, Feb 18, 2025 at 10:57:10AM +0200, Daniel Baluta wrote:
> 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 configuratin option.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/reset/reset-imx8mp-audiomix.c | 45 ++++++++++++++-------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 6b1666c4e069..8cc0a6b58cbc 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -23,16 +23,19 @@
>  struct imx8mp_reset_map {
>  	unsigned int offset;
>  	unsigned int mask;
> +	bool active_low;
>  };
>
>  static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
>  	[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,
>  	},
>
>  };
> @@ -48,48 +51,46 @@ 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;
>
>  	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
>  		return -EINVAL;
>
>  	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;
> -
> -	if (id >=  IMX8MP_AUDIOMIX_RESET_NUM)
> -		return -EINVAL;
> -
> -	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
>

Re: [PATCH 4/5] reset: imx8mp-audiomix: Add support for DSP run/stall

[Frank Li]

On Tue, Feb 18, 2025 at 10:57:11AM +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 | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> index 8cc0a6b58cbc..ee56d52a7278 100644
> --- a/drivers/reset/reset-imx8mp-audiomix.c
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -15,10 +15,14 @@
>  #define IMX8MP_AUDIOMIX_EARC_RESET_MASK		0x1
>  #define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK	0x2
>
> +#define IMX8MP_AUDIOMIX_DSP_OFFSET		0x108
> +#define IMX8MP_AUDIOMIX_DSP_RUNSTALL_MASK	0x20
> +
>  #define IMX8MP_AUDIOMIX_EARC		0
>  #define IMX8MP_AUDIOMIX_EARC_PHY	1
> +#define IMX8MP_AUDIOMIX_DSP		2
>
> -#define IMX8MP_AUDIOMIX_RESET_NUM	2
> +#define IMX8MP_AUDIOMIX_RESET_NUM	3
>
>  struct imx8mp_reset_map {
>  	unsigned int offset;
> @@ -37,7 +41,11 @@ static const struct imx8mp_reset_map reset_map[IMX8MP_AUDIOMIX_RESET_NUM] = {
>  		.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 5/5] imx_dsp_rproc: Use reset controller API to control the DSP

[Peng Fan]

On Tue, Feb 18, 2025 at 10:57:12AM +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>

LGTM: Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 1/5] reset: imx8mp-audiomix: Add prefix for internal macro

[Peng Fan]

On Tue, Feb 18, 2025 at 10:57:08AM +0200, Daniel Baluta wrote:
>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: Peng Fan <peng.fan@nxp.com>

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

[Daniel Baluta]

On Tue, Feb 18, 2025 at 5:56 PM Frank Li <Frank.li@nxp.com> wrote:
>
> On Tue, Feb 18, 2025 at 10:57:09AM +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.
>
> Nit: wrap at 75 chars
>
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> > ---
> >  drivers/reset/reset-imx8mp-audiomix.c | 53 ++++++++++++++++++++++-----
> >  1 file changed, 43 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/reset/reset-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> > index 1fe21980a66c..6b1666c4e069 100644
> > --- a/drivers/reset/reset-imx8mp-audiomix.c
> > +++ b/drivers/reset/reset-imx8mp-audiomix.c
> > @@ -12,7 +12,30 @@
> >  #include <linux/reset-controller.h>
> >
> >  #define IMX8MP_AUDIOMIX_EARC_OFFSET          0x200
> > -#define IMX8MP_AUDIOMIX_EARC_RESET_MASK              0x3
> > +#define IMX8MP_AUDIOMIX_EARC_RESET_MASK              0x1
> > +#define IMX8MP_AUDIOMIX_EARC_PHY_RESET_MASK  0x2
> > +
> > +#define IMX8MP_AUDIOMIX_EARC         0
> > +#define IMX8MP_AUDIOMIX_EARC_PHY     1
>
>
> Does dt binding need such macro?
>
> If not, you can use enum.
> If yes, create binding header file for it.

Thanks Philipp, Peng and Frank for your comments. All are valid.

Will fix them in v2.

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

[Daniel Baluta]

> > +     case IMX_RPROC_RESET_CONTROLLER:
> > +             priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
>
> Is this optional on purpose? There is no mention of it in the commit
> message. Where is this resets property documented in the dt-bindings?

For this particular case on imx8mp the reset control shouldn't be optional
because the DSP won't really start without it.

I was thinking for future boards that might have a reset but which is
is not mandatory.

Will document the resets in v2.