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