[PATCH v6 0/5] clk: imx: clk-audiomix: Improvement for audiomix

[PATCH v6 0/5] clk: imx: clk-audiomix: Improvement for audiomix

[Shengjiu Wang]

Some improvement for audiomix driver:
Add reset controller for EARC function
Add CLK_SET_RATE_PARENT flags for clocks
Correct parent clock for earc_phy and audpll clocks.

changes in v6:
- fix type for "correct"
- fix coding style for " * adev"

changes in v5:
- fix miss header issue reported by kernel test robot
- use scoped free

changes in v4:
- use auxiliary device framework for reset controller driver.
- drop syscon and simple-mfd related changes in v3

changes in v3:
- separate reset driver to driver/reset/
- add binding doc for reset driver.
- modify imx8mp.dtsi accordingly

changes in v2:
- add more info in commit messages

Shengjiu Wang (5):
  dt-bindings: clock: imx8mp: Add #reset-cells property
  clk: imx: clk-audiomix: Add reset controller
  reset: imx-aux: Add i.MX auxiliary reset driver
  clk: imx: clk-audiomix: Add CLK_SET_RATE_PARENT flags for clocks
  clk: imx: clk-audiomix: Correct parent clock for earc_phy and audpll

 .../bindings/clock/imx8mp-audiomix.yaml       |   3 +
 drivers/clk/imx/Kconfig                       |   1 +
 drivers/clk/imx/clk-imx8mp-audiomix.c         |  83 ++++++-
 drivers/reset/Kconfig                         |   8 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-imx-aux.c                 | 217 ++++++++++++++++++
 6 files changed, 307 insertions(+), 6 deletions(-)
 create mode 100644 drivers/reset/reset-imx-aux.c

-- 
2.34.1

[PATCH v6 1/5] dt-bindings: clock: imx8mp: Add #reset-cells property

[Shengjiu Wang]

The Audio Block Control contains clock distribution and gating
controls, as well as reset handling to several of the AUDIOMIX
peripherals. Especially the reset controls for Enhanced Audio
Return Channel (EARC) PHY and Controller.

So make Audio Block Control a reset provider for EARC, which
is one of modules in this audio subsystem.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
index 0a6dc1a6e122..6588a17a7d9a 100644
--- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
+++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
@@ -44,6 +44,9 @@ properties:
       ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h
       for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs.
 
+  '#reset-cells':
+    const: 1
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH v6 2/5] clk: imx: clk-audiomix: Add reset controller

[Shengjiu Wang]

Audiomix block control can be a reset controller for
Enhanced Audio Return Channel (EARC), which is one of
modules in this audiomix subsystem.

The reset controller is supported by the auxiliary device
framework.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/clk/imx/Kconfig               |  1 +
 drivers/clk/imx/clk-imx8mp-audiomix.c | 60 +++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
index 6da0fba68225..9edfb030bea9 100644
--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig
@@ -81,6 +81,7 @@ config CLK_IMX8MP
 	tristate "IMX8MP CCM Clock Driver"
 	depends on ARCH_MXC || COMPILE_TEST
 	select MXC_CLK
+	select AUXILIARY_BUS
 	help
 	    Build the driver for i.MX8MP CCM Clock Driver
 
diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
index b381d6f784c8..a3dc2f3606ee 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2022 Marek Vasut <marex@denx.de>
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -13,6 +14,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/slab.h>
 
 #include <dt-bindings/clock/imx8mp-clock.h>
 
@@ -217,6 +219,60 @@ struct clk_imx8mp_audiomix_priv {
 	struct clk_hw_onecell_data clk_data;
 };
 
+#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
+
+static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev)
+{
+	struct auxiliary_device *adev = _adev;
+
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	kfree(adev);
+}
+
+static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev,
+							 struct clk_imx8mp_audiomix_priv *priv)
+{
+	struct auxiliary_device *adev __free(kfree) = NULL;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->name = "reset";
+	adev->dev.parent = dev;
+	adev->dev.release = clk_imx8mp_audiomix_reset_adev_release;
+
+	ret = auxiliary_device_init(adev);
+	if (ret)
+		return ret;
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev,
+					no_free_ptr(adev));
+}
+
+#else /* !CONFIG_RESET_CONTROLLER */
+
+static int clk_imx8mp_audiomix_reset_controller_register(struct clk_imx8mp_audiomix_priv *priv)
+{
+	return 0;
+}
+
+#endif /* !CONFIG_RESET_CONTROLLER */
+
 static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save)
 {
 	struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev);
@@ -337,6 +393,10 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk_register;
 
+	ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv);
+	if (ret)
+		goto err_clk_register;
+
 	pm_runtime_put_sync(dev);
 	return 0;
 
-- 
2.34.1

Re: [PATCH v6 2/5] clk: imx: clk-audiomix: Add reset controller

[Marco Felsch]

On 24-05-30, Shengjiu Wang wrote:
> Audiomix block control can be a reset controller for
> Enhanced Audio Return Channel (EARC), which is one of
> modules in this audiomix subsystem.
> 
> The reset controller is supported by the auxiliary device
> framework.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/clk/imx/Kconfig               |  1 +
>  drivers/clk/imx/clk-imx8mp-audiomix.c | 60 +++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index 6da0fba68225..9edfb030bea9 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -81,6 +81,7 @@ config CLK_IMX8MP
>  	tristate "IMX8MP CCM Clock Driver"
>  	depends on ARCH_MXC || COMPILE_TEST
>  	select MXC_CLK
> +	select AUXILIARY_BUS
>  	help
>  	    Build the driver for i.MX8MP CCM Clock Driver
>  
> diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
> index b381d6f784c8..a3dc2f3606ee 100644
> --- a/drivers/clk/imx/clk-imx8mp-audiomix.c
> +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2022 Marek Vasut <marex@denx.de>
>   */
>  
> +#include <linux/auxiliary_bus.h>
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
> @@ -13,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/slab.h>
>  
>  #include <dt-bindings/clock/imx8mp-clock.h>
>  
> @@ -217,6 +219,60 @@ struct clk_imx8mp_audiomix_priv {
>  	struct clk_hw_onecell_data clk_data;
>  };
>  
> +#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
> +
> +static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev)
> +{
> +	struct auxiliary_device *adev = _adev;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}
> +
> +static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> +	kfree(adev);
> +}
> +
> +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev,
> +							 struct clk_imx8mp_audiomix_priv *priv)
> +{
> +	struct auxiliary_device *adev __free(kfree) = NULL;
> +	int ret;

Since the reset-controller is optional you need to check the existence
of the '#reset-cells' property before you register it.

Regards,
  Marco

> +
> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +	if (!adev)
> +		return -ENOMEM;
> +
> +	adev->name = "reset";
> +	adev->dev.parent = dev;
> +	adev->dev.release = clk_imx8mp_audiomix_reset_adev_release;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret)
> +		return ret;
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev,
> +					no_free_ptr(adev));
> +}
> +
> +#else /* !CONFIG_RESET_CONTROLLER */
> +
> +static int clk_imx8mp_audiomix_reset_controller_register(struct clk_imx8mp_audiomix_priv *priv)
> +{
> +	return 0;
> +}
> +
> +#endif /* !CONFIG_RESET_CONTROLLER */
> +
>  static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save)
>  {
>  	struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev);
> @@ -337,6 +393,10 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_clk_register;
>  
> +	ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv);
> +	if (ret)
> +		goto err_clk_register;
> +
>  	pm_runtime_put_sync(dev);
>  	return 0;
>  
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH v6 2/5] clk: imx: clk-audiomix: Add reset controller

[Shengjiu Wang]

On Thu, May 30, 2024 at 4:51 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> On 24-05-30, Shengjiu Wang wrote:
> > Audiomix block control can be a reset controller for
> > Enhanced Audio Return Channel (EARC), which is one of
> > modules in this audiomix subsystem.
> >
> > The reset controller is supported by the auxiliary device
> > framework.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/clk/imx/Kconfig               |  1 +
> >  drivers/clk/imx/clk-imx8mp-audiomix.c | 60 +++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> > index 6da0fba68225..9edfb030bea9 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -81,6 +81,7 @@ config CLK_IMX8MP
> >       tristate "IMX8MP CCM Clock Driver"
> >       depends on ARCH_MXC || COMPILE_TEST
> >       select MXC_CLK
> > +     select AUXILIARY_BUS
> >       help
> >           Build the driver for i.MX8MP CCM Clock Driver
> >
> > diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > index b381d6f784c8..a3dc2f3606ee 100644
> > --- a/drivers/clk/imx/clk-imx8mp-audiomix.c
> > +++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2022 Marek Vasut <marex@denx.de>
> >   */
> >
> > +#include <linux/auxiliary_bus.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> > @@ -13,6 +14,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> >
> >  #include <dt-bindings/clock/imx8mp-clock.h>
> >
> > @@ -217,6 +219,60 @@ struct clk_imx8mp_audiomix_priv {
> >       struct clk_hw_onecell_data clk_data;
> >  };
> >
> > +#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
> > +
> > +static void clk_imx8mp_audiomix_reset_unregister_adev(void *_adev)
> > +{
> > +     struct auxiliary_device *adev = _adev;
> > +
> > +     auxiliary_device_delete(adev);
> > +     auxiliary_device_uninit(adev);
> > +}
> > +
> > +static void clk_imx8mp_audiomix_reset_adev_release(struct device *dev)
> > +{
> > +     struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > +
> > +     kfree(adev);
> > +}
> > +
> > +static int clk_imx8mp_audiomix_reset_controller_register(struct device *dev,
> > +                                                      struct clk_imx8mp_audiomix_priv *priv)
> > +{
> > +     struct auxiliary_device *adev __free(kfree) = NULL;
> > +     int ret;
>
> Since the reset-controller is optional you need to check the existence
> of the '#reset-cells' property before you register it.
>

ok, will add it.

best regards
Shengjiu Wang

> Regards,
>   Marco
>
> > +
> > +     adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > +     if (!adev)
> > +             return -ENOMEM;
> > +
> > +     adev->name = "reset";
> > +     adev->dev.parent = dev;
> > +     adev->dev.release = clk_imx8mp_audiomix_reset_adev_release;
> > +
> > +     ret = auxiliary_device_init(adev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = auxiliary_device_add(adev);
> > +     if (ret) {
> > +             auxiliary_device_uninit(adev);
> > +             return ret;
> > +     }
> > +
> > +     return devm_add_action_or_reset(dev, clk_imx8mp_audiomix_reset_unregister_adev,
> > +                                     no_free_ptr(adev));
> > +}
> > +
> > +#else /* !CONFIG_RESET_CONTROLLER */
> > +
> > +static int clk_imx8mp_audiomix_reset_controller_register(struct clk_imx8mp_audiomix_priv *priv)
> > +{
> > +     return 0;
> > +}
> > +
> > +#endif /* !CONFIG_RESET_CONTROLLER */
> > +
> >  static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save)
> >  {
> >       struct clk_imx8mp_audiomix_priv *priv = dev_get_drvdata(dev);
> > @@ -337,6 +393,10 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto err_clk_register;
> >
> > +     ret = clk_imx8mp_audiomix_reset_controller_register(dev, priv);
> > +     if (ret)
> > +             goto err_clk_register;
> > +
> >       pm_runtime_put_sync(dev);
> >       return 0;
> >
> > --
> > 2.34.1
> >
> >
> >

[PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Shengjiu Wang]

Add support for the resets on i.MX8MP Audio Block Control module,
which includes the EARC PHY software reset and EARC controller
software reset. The reset controller is created using the auxiliary
device framework and set up in the clock driver.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/reset/Kconfig         |   8 ++
 drivers/reset/Makefile        |   1 +
 drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 drivers/reset/reset-imx-aux.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 7112f5932609..38fdf05b326b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -91,6 +91,14 @@ config RESET_IMX7
 	help
 	  This enables the reset controller driver for i.MX7 SoCs.
 
+config RESET_IMX_AUX
+	tristate "i.MX Auxiliary Reset Driver"
+	depends on CLK_IMX8MP
+	select AUXILIARY_BUS
+	default CLK_IMX8MP
+	help
+	  This enables the auxiliary reset controller driver for i.MX.
+
 config RESET_INTEL_GW
 	bool "Intel Reset Controller Driver"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index fd8b49fa46fc..f078da14c327 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
 obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
+obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
 obj-$(CONFIG_RESET_K210) += reset-k210.o
 obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
new file mode 100644
index 000000000000..61c353abc84e
--- /dev/null
+++ b/drivers/reset/reset-imx-aux.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+/*
+ * The reset does not support the feature and corresponding
+ * values are not valid
+ */
+#define ASSERT_NONE     BIT(0)
+#define DEASSERT_NONE   BIT(1)
+#define STATUS_NONE     BIT(2)
+
+/* When set this function is activated by setting(vs clearing) this bit */
+#define ASSERT_SET      BIT(3)
+#define DEASSERT_SET    BIT(4)
+#define STATUS_SET      BIT(5)
+
+/* The following are the inverse of the above and are added for consistency */
+#define ASSERT_CLEAR    (0 << 3)
+#define DEASSERT_CLEAR  (0 << 4)
+#define STATUS_CLEAR    (0 << 5)
+
+/**
+ * struct imx_reset_ctrl - reset control structure
+ * @assert_offset: reset assert control register offset
+ * @assert_bit: reset assert bit in the reset assert control register
+ * @deassert_offset: reset deassert control register offset
+ * @deassert_bit: reset deassert bit in the reset deassert control register
+ * @status_offset: reset status register offset
+ * @status_bit: reset status bit in the reset status register
+ * @flags: reset flag indicating how the (de)assert and status are handled
+ */
+struct imx_reset_ctrl {
+	u32 assert_offset;
+	u32 assert_bit;
+	u32 deassert_offset;
+	u32 deassert_bit;
+	u32 status_offset;
+	u32 status_bit;
+	u32 flags;
+};
+
+struct imx_reset_data {
+	const struct imx_reset_ctrl *rst_ctrl;
+	size_t rst_ctrl_num;
+};
+
+struct imx_aux_reset_priv {
+	struct reset_controller_dev rcdev;
+	void __iomem *base;
+	const struct imx_reset_data *data;
+};
+
+static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct imx_aux_reset_priv *priv = container_of(rcdev,
+					struct imx_aux_reset_priv, rcdev);
+	const struct imx_reset_data *data = priv->data;
+	void __iomem *reg_addr = priv->base;
+	const struct imx_reset_ctrl *ctrl;
+	unsigned int mask, value, reg;
+
+	if (id >= data->rst_ctrl_num)
+		return -EINVAL;
+
+	ctrl = &data->rst_ctrl[id];
+
+	/* assert not supported for this reset */
+	if (ctrl->flags & ASSERT_NONE)
+		return -EOPNOTSUPP;
+
+	mask = BIT(ctrl->assert_bit);
+	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
+
+	reg = readl(reg_addr + ctrl->assert_offset);
+	writel(reg | value, reg_addr + ctrl->assert_offset);
+
+	return 0;
+}
+
+static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
+				  unsigned long id)
+{
+	struct imx_aux_reset_priv *priv = container_of(rcdev,
+					struct imx_aux_reset_priv, rcdev);
+	const struct imx_reset_data *data = priv->data;
+	void __iomem *reg_addr = priv->base;
+	const struct imx_reset_ctrl *ctrl;
+	unsigned int mask, value, reg;
+
+	if (id >= data->rst_ctrl_num)
+		return -EINVAL;
+
+	ctrl = &data->rst_ctrl[id];
+
+	/* deassert not supported for this reset */
+	if (ctrl->flags & DEASSERT_NONE)
+		return -EOPNOTSUPP;
+
+	mask = BIT(ctrl->deassert_bit);
+	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
+
+	reg = readl(reg_addr + ctrl->deassert_offset);
+	writel(reg | value, reg_addr + ctrl->deassert_offset);
+
+	return 0;
+}
+
+static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct imx_aux_reset_priv *priv = container_of(rcdev,
+					struct imx_aux_reset_priv, rcdev);
+	const struct imx_reset_data *data = priv->data;
+	void __iomem *reg_addr = priv->base;
+	const struct imx_reset_ctrl *ctrl;
+	unsigned int reset_state;
+
+	if (id >= data->rst_ctrl_num)
+		return -EINVAL;
+
+	ctrl = &data->rst_ctrl[id];
+
+	/* status not supported for this reset */
+	if (ctrl->flags & STATUS_NONE)
+		return -EOPNOTSUPP;
+
+	reset_state = readl(reg_addr + ctrl->status_offset);
+
+	return !(reset_state & BIT(ctrl->status_bit)) ==
+		!(ctrl->flags & STATUS_SET);
+}
+
+static const struct reset_control_ops imx_aux_reset_ops = {
+	.assert   = imx_aux_reset_assert,
+	.deassert = imx_aux_reset_deassert,
+	.status	  = imx_aux_reset_status,
+};
+
+static int imx_aux_reset_probe(struct auxiliary_device *adev,
+			       const struct auxiliary_device_id *id)
+{
+	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
+	struct imx_aux_reset_priv *priv;
+	struct device *dev = &adev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->rcdev.owner     = THIS_MODULE;
+	priv->rcdev.nr_resets = data->rst_ctrl_num;
+	priv->rcdev.ops       = &imx_aux_reset_ops;
+	priv->rcdev.of_node   = dev->parent->of_node;
+	priv->rcdev.dev	      = dev;
+	priv->rcdev.of_reset_n_cells = 1;
+	priv->base            = of_iomap(dev->parent->of_node, 0);
+	priv->data            = data;
+
+	return devm_reset_controller_register(dev, &priv->rcdev);
+}
+
+#define EARC  0x200
+
+static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
+	{
+		.assert_offset = EARC,
+		.assert_bit = 0,
+		.deassert_offset = EARC,
+		.deassert_bit = 0,
+		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
+	},
+	{
+		.assert_offset = EARC,
+		.assert_bit = 1,
+		.deassert_offset = EARC,
+		.deassert_bit = 1,
+		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
+	},
+};
+
+static const struct imx_reset_data imx8mp_audiomix_rst_data = {
+	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
+	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
+};
+
+static const struct auxiliary_device_id imx_aux_reset_ids[] = {
+	{
+		.name = "clk_imx8mp_audiomix.reset",
+		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
+
+static struct auxiliary_driver imx_aux_reset_driver = {
+	.probe		= imx_aux_reset_probe,
+	.id_table	= imx_aux_reset_ids,
+};
+
+module_auxiliary_driver(imx_aux_reset_driver);
+
+MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
+MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Marco Felsch]

Hi,

On 24-05-30, Shengjiu Wang wrote:
> Add support for the resets on i.MX8MP Audio Block Control module,
> which includes the EARC PHY software reset and EARC controller
> software reset. The reset controller is created using the auxiliary
> device framework and set up in the clock driver.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/reset/Kconfig         |   8 ++
>  drivers/reset/Makefile        |   1 +
>  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
			    ^
You make use of the auxiliary bus but this isn't a aux driver, it's the
i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
are covered by the AUDIOMIX blk-ctrl.

>  3 files changed, 226 insertions(+)
>  create mode 100644 drivers/reset/reset-imx-aux.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7112f5932609..38fdf05b326b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -91,6 +91,14 @@ config RESET_IMX7
>  	help
>  	  This enables the reset controller driver for i.MX7 SoCs.
>  
> +config RESET_IMX_AUX
> +	tristate "i.MX Auxiliary Reset Driver"
			^
              Same applies here

> +	depends on CLK_IMX8MP
> +	select AUXILIARY_BUS
> +	default CLK_IMX8MP
> +	help
> +	  This enables the auxiliary reset controller driver for i.MX.
> +
>  config RESET_INTEL_GW
>  	bool "Intel Reset Controller Driver"
>  	depends on X86 || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index fd8b49fa46fc..f078da14c327 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
>  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
>  obj-$(CONFIG_RESET_K210) += reset-k210.o
>  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> new file mode 100644
> index 000000000000..61c353abc84e
> --- /dev/null
> +++ b/drivers/reset/reset-imx-aux.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +
> +/*
> + * The reset does not support the feature and corresponding
> + * values are not valid
> + */
> +#define ASSERT_NONE     BIT(0)
> +#define DEASSERT_NONE   BIT(1)
> +#define STATUS_NONE     BIT(2)
> +
> +/* When set this function is activated by setting(vs clearing) this bit */
> +#define ASSERT_SET      BIT(3)
> +#define DEASSERT_SET    BIT(4)
> +#define STATUS_SET      BIT(5)
> +
> +/* The following are the inverse of the above and are added for consistency */
> +#define ASSERT_CLEAR    (0 << 3)
> +#define DEASSERT_CLEAR  (0 << 4)
> +#define STATUS_CLEAR    (0 << 5)
> +
> +/**
> + * struct imx_reset_ctrl - reset control structure
> + * @assert_offset: reset assert control register offset
> + * @assert_bit: reset assert bit in the reset assert control register
> + * @deassert_offset: reset deassert control register offset
> + * @deassert_bit: reset deassert bit in the reset deassert control register
> + * @status_offset: reset status register offset
> + * @status_bit: reset status bit in the reset status register
> + * @flags: reset flag indicating how the (de)assert and status are handled
> + */
> +struct imx_reset_ctrl {
> +	u32 assert_offset;
> +	u32 assert_bit;
> +	u32 deassert_offset;
> +	u32 deassert_bit;
> +	u32 status_offset;
> +	u32 status_bit;
> +	u32 flags;
> +};

Why do we make it this compicated for an simple EARC module reset? I
understand that you want to provide a generic driver which can be
re-used but there is actual no other user and may will get no other user
in the future too. Therefore I would like to keep it simple at the
begin and adapt the code on-demand.

Regards,
  Marco

> +struct imx_reset_data {
> +	const struct imx_reset_ctrl *rst_ctrl;
> +	size_t rst_ctrl_num;
> +};
> +
> +struct imx_aux_reset_priv {
> +	struct reset_controller_dev rcdev;
> +	void __iomem *base;
> +	const struct imx_reset_data *data;
> +};
> +
> +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> +					struct imx_aux_reset_priv, rcdev);
> +	const struct imx_reset_data *data = priv->data;
> +	void __iomem *reg_addr = priv->base;
> +	const struct imx_reset_ctrl *ctrl;
> +	unsigned int mask, value, reg;
> +
> +	if (id >= data->rst_ctrl_num)
> +		return -EINVAL;
> +
> +	ctrl = &data->rst_ctrl[id];
> +
> +	/* assert not supported for this reset */
> +	if (ctrl->flags & ASSERT_NONE)
> +		return -EOPNOTSUPP;
> +
> +	mask = BIT(ctrl->assert_bit);
> +	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> +
> +	reg = readl(reg_addr + ctrl->assert_offset);
> +	writel(reg | value, reg_addr + ctrl->assert_offset);
> +
> +	return 0;
> +}
> +
> +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> +				  unsigned long id)
> +{
> +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> +					struct imx_aux_reset_priv, rcdev);
> +	const struct imx_reset_data *data = priv->data;
> +	void __iomem *reg_addr = priv->base;
> +	const struct imx_reset_ctrl *ctrl;
> +	unsigned int mask, value, reg;
> +
> +	if (id >= data->rst_ctrl_num)
> +		return -EINVAL;
> +
> +	ctrl = &data->rst_ctrl[id];
> +
> +	/* deassert not supported for this reset */
> +	if (ctrl->flags & DEASSERT_NONE)
> +		return -EOPNOTSUPP;
> +
> +	mask = BIT(ctrl->deassert_bit);
> +	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> +
> +	reg = readl(reg_addr + ctrl->deassert_offset);
> +	writel(reg | value, reg_addr + ctrl->deassert_offset);
> +
> +	return 0;
> +}
> +
> +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> +					struct imx_aux_reset_priv, rcdev);
> +	const struct imx_reset_data *data = priv->data;
> +	void __iomem *reg_addr = priv->base;
> +	const struct imx_reset_ctrl *ctrl;
> +	unsigned int reset_state;
> +
> +	if (id >= data->rst_ctrl_num)
> +		return -EINVAL;
> +
> +	ctrl = &data->rst_ctrl[id];
> +
> +	/* status not supported for this reset */
> +	if (ctrl->flags & STATUS_NONE)
> +		return -EOPNOTSUPP;
> +
> +	reset_state = readl(reg_addr + ctrl->status_offset);
> +
> +	return !(reset_state & BIT(ctrl->status_bit)) ==
> +		!(ctrl->flags & STATUS_SET);
> +}
> +
> +static const struct reset_control_ops imx_aux_reset_ops = {
> +	.assert   = imx_aux_reset_assert,
> +	.deassert = imx_aux_reset_deassert,
> +	.status	  = imx_aux_reset_status,
> +};
> +
> +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> +			       const struct auxiliary_device_id *id)
> +{
> +	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> +	struct imx_aux_reset_priv *priv;
> +	struct device *dev = &adev->dev;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->rcdev.owner     = THIS_MODULE;
> +	priv->rcdev.nr_resets = data->rst_ctrl_num;
> +	priv->rcdev.ops       = &imx_aux_reset_ops;
> +	priv->rcdev.of_node   = dev->parent->of_node;
> +	priv->rcdev.dev	      = dev;
> +	priv->rcdev.of_reset_n_cells = 1;
> +	priv->base            = of_iomap(dev->parent->of_node, 0);
> +	priv->data            = data;
> +
> +	return devm_reset_controller_register(dev, &priv->rcdev);
> +}
> +
> +#define EARC  0x200
> +
> +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> +	{
> +		.assert_offset = EARC,
> +		.assert_bit = 0,
> +		.deassert_offset = EARC,
> +		.deassert_bit = 0,
> +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> +	},
> +	{
> +		.assert_offset = EARC,
> +		.assert_bit = 1,
> +		.deassert_offset = EARC,
> +		.deassert_bit = 1,
> +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> +	},
> +};
> +
> +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> +	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
> +	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> +};
> +
> +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> +	{
> +		.name = "clk_imx8mp_audiomix.reset",
> +		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> +
> +static struct auxiliary_driver imx_aux_reset_driver = {
> +	.probe		= imx_aux_reset_probe,
> +	.id_table	= imx_aux_reset_ids,
> +};
> +
> +module_auxiliary_driver(imx_aux_reset_driver);
> +
> +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Frank Li]

On Thu, May 30, 2024 at 11:05:58AM +0200, Marco Felsch wrote:
> Hi,
> 
> On 24-05-30, Shengjiu Wang wrote:
> > Add support for the resets on i.MX8MP Audio Block Control module,
> > which includes the EARC PHY software reset and EARC controller
> > software reset. The reset controller is created using the auxiliary
> > device framework and set up in the clock driver.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/reset/Kconfig         |   8 ++
> >  drivers/reset/Makefile        |   1 +
> >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
> 			    ^
> You make use of the auxiliary bus but this isn't a aux driver, it's the
> i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> are covered by the AUDIOMIX blk-ctrl.
> 
> >  3 files changed, 226 insertions(+)
> >  create mode 100644 drivers/reset/reset-imx-aux.c
> > 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 7112f5932609..38fdf05b326b 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -91,6 +91,14 @@ config RESET_IMX7
> >  	help
> >  	  This enables the reset controller driver for i.MX7 SoCs.
> >  
> > +config RESET_IMX_AUX
> > +	tristate "i.MX Auxiliary Reset Driver"
> 			^
>               Same applies here
> 
> > +	depends on CLK_IMX8MP
> > +	select AUXILIARY_BUS
> > +	default CLK_IMX8MP
> > +	help
> > +	  This enables the auxiliary reset controller driver for i.MX.
> > +
> >  config RESET_INTEL_GW
> >  	bool "Intel Reset Controller Driver"
> >  	depends on X86 || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index fd8b49fa46fc..f078da14c327 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > new file mode 100644
> > index 000000000000..61c353abc84e
> > --- /dev/null
> > +++ b/drivers/reset/reset-imx-aux.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +
> > +/*
> > + * The reset does not support the feature and corresponding
> > + * values are not valid
> > + */
> > +#define ASSERT_NONE     BIT(0)
> > +#define DEASSERT_NONE   BIT(1)
> > +#define STATUS_NONE     BIT(2)
> > +
> > +/* When set this function is activated by setting(vs clearing) this bit */
> > +#define ASSERT_SET      BIT(3)
> > +#define DEASSERT_SET    BIT(4)
> > +#define STATUS_SET      BIT(5)
> > +
> > +/* The following are the inverse of the above and are added for consistency */
> > +#define ASSERT_CLEAR    (0 << 3)
> > +#define DEASSERT_CLEAR  (0 << 4)
> > +#define STATUS_CLEAR    (0 << 5)
> > +
> > +/**
> > + * struct imx_reset_ctrl - reset control structure
> > + * @assert_offset: reset assert control register offset
> > + * @assert_bit: reset assert bit in the reset assert control register
> > + * @deassert_offset: reset deassert control register offset
> > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > + * @status_offset: reset status register offset
> > + * @status_bit: reset status bit in the reset status register
> > + * @flags: reset flag indicating how the (de)assert and status are handled
> > + */
> > +struct imx_reset_ctrl {
> > +	u32 assert_offset;
> > +	u32 assert_bit;
> > +	u32 deassert_offset;
> > +	u32 deassert_bit;
> > +	u32 status_offset;
> > +	u32 status_bit;
> > +	u32 flags;
> > +};
> 
> Why do we make it this compicated for an simple EARC module reset? I
> understand that you want to provide a generic driver which can be
> re-used but there is actual no other user and may will get no other user
> in the future too. Therefore I would like to keep it simple at the
> begin and adapt the code on-demand.

There are many similar cases. such as
https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/pci/controller/dwc/pci-layerscape.c#L251

Previously it use syscon and regmap to a global register space region and
direct operate the register. Now this way will not preferred. It needs
export as reset driver. but actually, it just write some bits. 

We face the similar problem at difference driver when do upstream.

One on going a discussion about sim module reset
https://lore.kernel.org/imx/131e46b1-61d9-41de-a225-853b09c765d1@gmail.com/

We hope an unified and simple method to handle these cases.

Frank

> 
> Regards,
>   Marco
> 
> > +struct imx_reset_data {
> > +	const struct imx_reset_ctrl *rst_ctrl;
> > +	size_t rst_ctrl_num;
> > +};
> > +
> > +struct imx_aux_reset_priv {
> > +	struct reset_controller_dev rcdev;
> > +	void __iomem *base;
> > +	const struct imx_reset_data *data;
> > +};
> > +
> > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > +				unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int mask, value, reg;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* assert not supported for this reset */
> > +	if (ctrl->flags & ASSERT_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	mask = BIT(ctrl->assert_bit);
> > +	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > +
> > +	reg = readl(reg_addr + ctrl->assert_offset);
> > +	writel(reg | value, reg_addr + ctrl->assert_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > +				  unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int mask, value, reg;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* deassert not supported for this reset */
> > +	if (ctrl->flags & DEASSERT_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	mask = BIT(ctrl->deassert_bit);
> > +	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > +
> > +	reg = readl(reg_addr + ctrl->deassert_offset);
> > +	writel(reg | value, reg_addr + ctrl->deassert_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > +				unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int reset_state;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* status not supported for this reset */
> > +	if (ctrl->flags & STATUS_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	reset_state = readl(reg_addr + ctrl->status_offset);
> > +
> > +	return !(reset_state & BIT(ctrl->status_bit)) ==
> > +		!(ctrl->flags & STATUS_SET);
> > +}
> > +
> > +static const struct reset_control_ops imx_aux_reset_ops = {
> > +	.assert   = imx_aux_reset_assert,
> > +	.deassert = imx_aux_reset_deassert,
> > +	.status	  = imx_aux_reset_status,
> > +};
> > +
> > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > +			       const struct auxiliary_device_id *id)
> > +{
> > +	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > +	struct imx_aux_reset_priv *priv;
> > +	struct device *dev = &adev->dev;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->rcdev.owner     = THIS_MODULE;
> > +	priv->rcdev.nr_resets = data->rst_ctrl_num;
> > +	priv->rcdev.ops       = &imx_aux_reset_ops;
> > +	priv->rcdev.of_node   = dev->parent->of_node;
> > +	priv->rcdev.dev	      = dev;
> > +	priv->rcdev.of_reset_n_cells = 1;
> > +	priv->base            = of_iomap(dev->parent->of_node, 0);
> > +	priv->data            = data;
> > +
> > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> > +#define EARC  0x200
> > +
> > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > +	{
> > +		.assert_offset = EARC,
> > +		.assert_bit = 0,
> > +		.deassert_offset = EARC,
> > +		.deassert_bit = 0,
> > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +	},
> > +	{
> > +		.assert_offset = EARC,
> > +		.assert_bit = 1,
> > +		.deassert_offset = EARC,
> > +		.deassert_bit = 1,
> > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +	},
> > +};
> > +
> > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > +	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > +	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > +};
> > +
> > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > +	{
> > +		.name = "clk_imx8mp_audiomix.reset",
> > +		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > +	},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > +
> > +static struct auxiliary_driver imx_aux_reset_driver = {
> > +	.probe		= imx_aux_reset_probe,
> > +	.id_table	= imx_aux_reset_ids,
> > +};
> > +
> > +module_auxiliary_driver(imx_aux_reset_driver);
> > +
> > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.34.1
> > 
> > 
> > 

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Marco Felsch]

On 24-05-30, Frank Li wrote:
> On Thu, May 30, 2024 at 11:05:58AM +0200, Marco Felsch wrote:
> > Hi,
> > 
> > On 24-05-30, Shengjiu Wang wrote:
> > > Add support for the resets on i.MX8MP Audio Block Control module,
> > > which includes the EARC PHY software reset and EARC controller
> > > software reset. The reset controller is created using the auxiliary
> > > device framework and set up in the clock driver.
> > > 
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > >  drivers/reset/Kconfig         |   8 ++
> > >  drivers/reset/Makefile        |   1 +
> > >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
> > 			    ^
> > You make use of the auxiliary bus but this isn't a aux driver, it's the
> > i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> > are covered by the AUDIOMIX blk-ctrl.
> > 
> > >  3 files changed, 226 insertions(+)
> > >  create mode 100644 drivers/reset/reset-imx-aux.c
> > > 
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index 7112f5932609..38fdf05b326b 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -91,6 +91,14 @@ config RESET_IMX7
> > >  	help
> > >  	  This enables the reset controller driver for i.MX7 SoCs.
> > >  
> > > +config RESET_IMX_AUX
> > > +	tristate "i.MX Auxiliary Reset Driver"
> > 			^
> >               Same applies here
> > 
> > > +	depends on CLK_IMX8MP
> > > +	select AUXILIARY_BUS
> > > +	default CLK_IMX8MP
> > > +	help
> > > +	  This enables the auxiliary reset controller driver for i.MX.
> > > +
> > >  config RESET_INTEL_GW
> > >  	bool "Intel Reset Controller Driver"
> > >  	depends on X86 || COMPILE_TEST
> > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > > index fd8b49fa46fc..f078da14c327 100644
> > > --- a/drivers/reset/Makefile
> > > +++ b/drivers/reset/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> > >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> > >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> > >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> > >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> > >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> > >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > > new file mode 100644
> > > index 000000000000..61c353abc84e
> > > --- /dev/null
> > > +++ b/drivers/reset/reset-imx-aux.c
> > > @@ -0,0 +1,217 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright 2024 NXP
> > > + */
> > > +
> > > +#include <linux/auxiliary_bus.h>
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/reset-controller.h>
> > > +
> > > +/*
> > > + * The reset does not support the feature and corresponding
> > > + * values are not valid
> > > + */
> > > +#define ASSERT_NONE     BIT(0)
> > > +#define DEASSERT_NONE   BIT(1)
> > > +#define STATUS_NONE     BIT(2)
> > > +
> > > +/* When set this function is activated by setting(vs clearing) this bit */
> > > +#define ASSERT_SET      BIT(3)
> > > +#define DEASSERT_SET    BIT(4)
> > > +#define STATUS_SET      BIT(5)
> > > +
> > > +/* The following are the inverse of the above and are added for consistency */
> > > +#define ASSERT_CLEAR    (0 << 3)
> > > +#define DEASSERT_CLEAR  (0 << 4)
> > > +#define STATUS_CLEAR    (0 << 5)
> > > +
> > > +/**
> > > + * struct imx_reset_ctrl - reset control structure
> > > + * @assert_offset: reset assert control register offset
> > > + * @assert_bit: reset assert bit in the reset assert control register
> > > + * @deassert_offset: reset deassert control register offset
> > > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > > + * @status_offset: reset status register offset
> > > + * @status_bit: reset status bit in the reset status register
> > > + * @flags: reset flag indicating how the (de)assert and status are handled
> > > + */
> > > +struct imx_reset_ctrl {
> > > +	u32 assert_offset;
> > > +	u32 assert_bit;
> > > +	u32 deassert_offset;
> > > +	u32 deassert_bit;
> > > +	u32 status_offset;
> > > +	u32 status_bit;
> > > +	u32 flags;
> > > +};
> > 
> > Why do we make it this compicated for an simple EARC module reset? I
> > understand that you want to provide a generic driver which can be
> > re-used but there is actual no other user and may will get no other user
> > in the future too. Therefore I would like to keep it simple at the
> > begin and adapt the code on-demand.
> 
> There are many similar cases. such as
> https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/pci/controller/dwc/pci-layerscape.c#L251
> 
> Previously it use syscon and regmap to a global register space region and
> direct operate the register. Now this way will not preferred. It needs
> export as reset driver. but actually, it just write some bits. 

It depends, if your reset-controller is part of an complete different
device like this EARC reset you're right else you can write to the reset
directly within you driver.

> We face the similar problem at difference driver when do upstream.
> 
> One on going a discussion about sim module reset
> https://lore.kernel.org/imx/131e46b1-61d9-41de-a225-853b09c765d1@gmail.com/
> 
> We hope an unified and simple method to handle these cases.

An unified driver for non-unified reset modules? This makes no sense to
me. When it comes to possible quirk handling for different reset modules
your code gets even more complex.

I'm fine with a common code base (driver) if NXP has an common reset
controller IP which is added to several SoC. There should be no common
code base if this isn't the case.

Regards,
  Marco


> 
> Frank
> 
> > 
> > Regards,
> >   Marco
> > 
> > > +struct imx_reset_data {
> > > +	const struct imx_reset_ctrl *rst_ctrl;
> > > +	size_t rst_ctrl_num;
> > > +};
> > > +
> > > +struct imx_aux_reset_priv {
> > > +	struct reset_controller_dev rcdev;
> > > +	void __iomem *base;
> > > +	const struct imx_reset_data *data;
> > > +};
> > > +
> > > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > > +				unsigned long id)
> > > +{
> > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > +					struct imx_aux_reset_priv, rcdev);
> > > +	const struct imx_reset_data *data = priv->data;
> > > +	void __iomem *reg_addr = priv->base;
> > > +	const struct imx_reset_ctrl *ctrl;
> > > +	unsigned int mask, value, reg;
> > > +
> > > +	if (id >= data->rst_ctrl_num)
> > > +		return -EINVAL;
> > > +
> > > +	ctrl = &data->rst_ctrl[id];
> > > +
> > > +	/* assert not supported for this reset */
> > > +	if (ctrl->flags & ASSERT_NONE)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	mask = BIT(ctrl->assert_bit);
> > > +	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > > +
> > > +	reg = readl(reg_addr + ctrl->assert_offset);
> > > +	writel(reg | value, reg_addr + ctrl->assert_offset);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > > +				  unsigned long id)
> > > +{
> > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > +					struct imx_aux_reset_priv, rcdev);
> > > +	const struct imx_reset_data *data = priv->data;
> > > +	void __iomem *reg_addr = priv->base;
> > > +	const struct imx_reset_ctrl *ctrl;
> > > +	unsigned int mask, value, reg;
> > > +
> > > +	if (id >= data->rst_ctrl_num)
> > > +		return -EINVAL;
> > > +
> > > +	ctrl = &data->rst_ctrl[id];
> > > +
> > > +	/* deassert not supported for this reset */
> > > +	if (ctrl->flags & DEASSERT_NONE)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	mask = BIT(ctrl->deassert_bit);
> > > +	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > > +
> > > +	reg = readl(reg_addr + ctrl->deassert_offset);
> > > +	writel(reg | value, reg_addr + ctrl->deassert_offset);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > > +				unsigned long id)
> > > +{
> > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > +					struct imx_aux_reset_priv, rcdev);
> > > +	const struct imx_reset_data *data = priv->data;
> > > +	void __iomem *reg_addr = priv->base;
> > > +	const struct imx_reset_ctrl *ctrl;
> > > +	unsigned int reset_state;
> > > +
> > > +	if (id >= data->rst_ctrl_num)
> > > +		return -EINVAL;
> > > +
> > > +	ctrl = &data->rst_ctrl[id];
> > > +
> > > +	/* status not supported for this reset */
> > > +	if (ctrl->flags & STATUS_NONE)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	reset_state = readl(reg_addr + ctrl->status_offset);
> > > +
> > > +	return !(reset_state & BIT(ctrl->status_bit)) ==
> > > +		!(ctrl->flags & STATUS_SET);
> > > +}
> > > +
> > > +static const struct reset_control_ops imx_aux_reset_ops = {
> > > +	.assert   = imx_aux_reset_assert,
> > > +	.deassert = imx_aux_reset_deassert,
> > > +	.status	  = imx_aux_reset_status,
> > > +};
> > > +
> > > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > > +			       const struct auxiliary_device_id *id)
> > > +{
> > > +	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > > +	struct imx_aux_reset_priv *priv;
> > > +	struct device *dev = &adev->dev;
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	priv->rcdev.owner     = THIS_MODULE;
> > > +	priv->rcdev.nr_resets = data->rst_ctrl_num;
> > > +	priv->rcdev.ops       = &imx_aux_reset_ops;
> > > +	priv->rcdev.of_node   = dev->parent->of_node;
> > > +	priv->rcdev.dev	      = dev;
> > > +	priv->rcdev.of_reset_n_cells = 1;
> > > +	priv->base            = of_iomap(dev->parent->of_node, 0);
> > > +	priv->data            = data;
> > > +
> > > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > > +}
> > > +
> > > +#define EARC  0x200
> > > +
> > > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > > +	{
> > > +		.assert_offset = EARC,
> > > +		.assert_bit = 0,
> > > +		.deassert_offset = EARC,
> > > +		.deassert_bit = 0,
> > > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > +	},
> > > +	{
> > > +		.assert_offset = EARC,
> > > +		.assert_bit = 1,
> > > +		.deassert_offset = EARC,
> > > +		.deassert_bit = 1,
> > > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > +	},
> > > +};
> > > +
> > > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > > +	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > > +	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > > +};
> > > +
> > > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > > +	{
> > > +		.name = "clk_imx8mp_audiomix.reset",
> > > +		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > > +	},
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > > +
> > > +static struct auxiliary_driver imx_aux_reset_driver = {
> > > +	.probe		= imx_aux_reset_probe,
> > > +	.id_table	= imx_aux_reset_ids,
> > > +};
> > > +
> > > +module_auxiliary_driver(imx_aux_reset_driver);
> > > +
> > > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > > +MODULE_LICENSE("GPL");
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > 
> 

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Frank Li]

On Thu, May 30, 2024 at 06:45:10PM +0200, Marco Felsch wrote:
> On 24-05-30, Frank Li wrote:
> > On Thu, May 30, 2024 at 11:05:58AM +0200, Marco Felsch wrote:
> > > Hi,
> > > 
> > > On 24-05-30, Shengjiu Wang wrote:
> > > > Add support for the resets on i.MX8MP Audio Block Control module,
> > > > which includes the EARC PHY software reset and EARC controller
> > > > software reset. The reset controller is created using the auxiliary
> > > > device framework and set up in the clock driver.
> > > > 
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > >  drivers/reset/Kconfig         |   8 ++
> > > >  drivers/reset/Makefile        |   1 +
> > > >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
> > > 			    ^
> > > You make use of the auxiliary bus but this isn't a aux driver, it's the
> > > i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> > > are covered by the AUDIOMIX blk-ctrl.
> > > 
> > > >  3 files changed, 226 insertions(+)
> > > >  create mode 100644 drivers/reset/reset-imx-aux.c
> > > > 
> > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > > index 7112f5932609..38fdf05b326b 100644
> > > > --- a/drivers/reset/Kconfig
> > > > +++ b/drivers/reset/Kconfig
> > > > @@ -91,6 +91,14 @@ config RESET_IMX7
> > > >  	help
> > > >  	  This enables the reset controller driver for i.MX7 SoCs.
> > > >  
> > > > +config RESET_IMX_AUX
> > > > +	tristate "i.MX Auxiliary Reset Driver"
> > > 			^
> > >               Same applies here
> > > 
> > > > +	depends on CLK_IMX8MP
> > > > +	select AUXILIARY_BUS
> > > > +	default CLK_IMX8MP
> > > > +	help
> > > > +	  This enables the auxiliary reset controller driver for i.MX.
> > > > +
> > > >  config RESET_INTEL_GW
> > > >  	bool "Intel Reset Controller Driver"
> > > >  	depends on X86 || COMPILE_TEST
> > > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > > > index fd8b49fa46fc..f078da14c327 100644
> > > > --- a/drivers/reset/Makefile
> > > > +++ b/drivers/reset/Makefile
> > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> > > >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> > > >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> > > >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > > > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> > > >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> > > >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> > > >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > > > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > > > new file mode 100644
> > > > index 000000000000..61c353abc84e
> > > > --- /dev/null
> > > > +++ b/drivers/reset/reset-imx-aux.c
> > > > @@ -0,0 +1,217 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * Copyright 2024 NXP
> > > > + */
> > > > +
> > > > +#include <linux/auxiliary_bus.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/reset-controller.h>
> > > > +
> > > > +/*
> > > > + * The reset does not support the feature and corresponding
> > > > + * values are not valid
> > > > + */
> > > > +#define ASSERT_NONE     BIT(0)
> > > > +#define DEASSERT_NONE   BIT(1)
> > > > +#define STATUS_NONE     BIT(2)
> > > > +
> > > > +/* When set this function is activated by setting(vs clearing) this bit */
> > > > +#define ASSERT_SET      BIT(3)
> > > > +#define DEASSERT_SET    BIT(4)
> > > > +#define STATUS_SET      BIT(5)
> > > > +
> > > > +/* The following are the inverse of the above and are added for consistency */
> > > > +#define ASSERT_CLEAR    (0 << 3)
> > > > +#define DEASSERT_CLEAR  (0 << 4)
> > > > +#define STATUS_CLEAR    (0 << 5)
> > > > +
> > > > +/**
> > > > + * struct imx_reset_ctrl - reset control structure
> > > > + * @assert_offset: reset assert control register offset
> > > > + * @assert_bit: reset assert bit in the reset assert control register
> > > > + * @deassert_offset: reset deassert control register offset
> > > > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > > > + * @status_offset: reset status register offset
> > > > + * @status_bit: reset status bit in the reset status register
> > > > + * @flags: reset flag indicating how the (de)assert and status are handled
> > > > + */
> > > > +struct imx_reset_ctrl {
> > > > +	u32 assert_offset;
> > > > +	u32 assert_bit;
> > > > +	u32 deassert_offset;
> > > > +	u32 deassert_bit;
> > > > +	u32 status_offset;
> > > > +	u32 status_bit;
> > > > +	u32 flags;
> > > > +};
> > > 
> > > Why do we make it this compicated for an simple EARC module reset? I
> > > understand that you want to provide a generic driver which can be
> > > re-used but there is actual no other user and may will get no other user
> > > in the future too. Therefore I would like to keep it simple at the
> > > begin and adapt the code on-demand.
> > 
> > There are many similar cases. such as
> > https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/pci/controller/dwc/pci-layerscape.c#L251
> > 
> > Previously it use syscon and regmap to a global register space region and
> > direct operate the register. Now this way will not preferred. It needs
> > export as reset driver. but actually, it just write some bits. 
> 
> It depends, if your reset-controller is part of an complete different
> device like this EARC reset you're right else you can write to the reset
> directly within you driver.

These reset bits allocated a global register regions. Such as bit0 for
pcie0, bit1 for pci1, bit2 for usb0, bit3 for usb1. Hardware pack these
to misc mmio region.

> 
> > We face the similar problem at difference driver when do upstream.
> > 
> > One on going a discussion about sim module reset
> > https://lore.kernel.org/imx/131e46b1-61d9-41de-a225-853b09c765d1@gmail.com/
> > 
> > We hope an unified and simple method to handle these cases.
> 
> An unified driver for non-unified reset modules? This makes no sense to
> me. When it comes to possible quirk handling for different reset modules
> your code gets even more complex.
> 
> I'm fine with a common code base (driver) if NXP has an common reset
> controller IP which is added to several SoC. There should be no common
> code base if this isn't the case.

Some chip have common reset controller IP. Some chip simple packed reset
bits to one global misc mmio region with other misc control bit, such power
on/off / clock source choose / enable debug features. 

Here we just consider the second case. Anyway we need handle these case at
somewhere. 99% case just write bit<n> to register offset. Only difference
is bit posstion and register offset. If write difference driver for these
just because bit posstion or offset, there will be many duplicated codes
under driver/reset.

For example:
Misc: 0x1000
      bit0: pcie0 reset
      bit2: usb0 reset
      bit3-bit4: pcie clk source select
      bit5-bit6: usb0 clk source select
      bit7: pcie0 phy power on
      bit8: usb0 phy power on.

The difference chip, bit possition will be changed.
How handle this case corporately?

> 
> Regards,
>   Marco
> 
> 
> > 
> > Frank
> > 
> > > 
> > > Regards,
> > >   Marco
> > > 
> > > > +struct imx_reset_data {
> > > > +	const struct imx_reset_ctrl *rst_ctrl;
> > > > +	size_t rst_ctrl_num;
> > > > +};
> > > > +
> > > > +struct imx_aux_reset_priv {
> > > > +	struct reset_controller_dev rcdev;
> > > > +	void __iomem *base;
> > > > +	const struct imx_reset_data *data;
> > > > +};
> > > > +
> > > > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > > > +				unsigned long id)
> > > > +{
> > > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > +					struct imx_aux_reset_priv, rcdev);
> > > > +	const struct imx_reset_data *data = priv->data;
> > > > +	void __iomem *reg_addr = priv->base;
> > > > +	const struct imx_reset_ctrl *ctrl;
> > > > +	unsigned int mask, value, reg;
> > > > +
> > > > +	if (id >= data->rst_ctrl_num)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ctrl = &data->rst_ctrl[id];
> > > > +
> > > > +	/* assert not supported for this reset */
> > > > +	if (ctrl->flags & ASSERT_NONE)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	mask = BIT(ctrl->assert_bit);
> > > > +	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > > > +
> > > > +	reg = readl(reg_addr + ctrl->assert_offset);
> > > > +	writel(reg | value, reg_addr + ctrl->assert_offset);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > > > +				  unsigned long id)
> > > > +{
> > > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > +					struct imx_aux_reset_priv, rcdev);
> > > > +	const struct imx_reset_data *data = priv->data;
> > > > +	void __iomem *reg_addr = priv->base;
> > > > +	const struct imx_reset_ctrl *ctrl;
> > > > +	unsigned int mask, value, reg;
> > > > +
> > > > +	if (id >= data->rst_ctrl_num)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ctrl = &data->rst_ctrl[id];
> > > > +
> > > > +	/* deassert not supported for this reset */
> > > > +	if (ctrl->flags & DEASSERT_NONE)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	mask = BIT(ctrl->deassert_bit);
> > > > +	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > > > +
> > > > +	reg = readl(reg_addr + ctrl->deassert_offset);
> > > > +	writel(reg | value, reg_addr + ctrl->deassert_offset);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > > > +				unsigned long id)
> > > > +{
> > > > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > +					struct imx_aux_reset_priv, rcdev);
> > > > +	const struct imx_reset_data *data = priv->data;
> > > > +	void __iomem *reg_addr = priv->base;
> > > > +	const struct imx_reset_ctrl *ctrl;
> > > > +	unsigned int reset_state;
> > > > +
> > > > +	if (id >= data->rst_ctrl_num)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ctrl = &data->rst_ctrl[id];
> > > > +
> > > > +	/* status not supported for this reset */
> > > > +	if (ctrl->flags & STATUS_NONE)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	reset_state = readl(reg_addr + ctrl->status_offset);
> > > > +
> > > > +	return !(reset_state & BIT(ctrl->status_bit)) ==
> > > > +		!(ctrl->flags & STATUS_SET);
> > > > +}
> > > > +
> > > > +static const struct reset_control_ops imx_aux_reset_ops = {
> > > > +	.assert   = imx_aux_reset_assert,
> > > > +	.deassert = imx_aux_reset_deassert,
> > > > +	.status	  = imx_aux_reset_status,
> > > > +};
> > > > +
> > > > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > > > +			       const struct auxiliary_device_id *id)
> > > > +{
> > > > +	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > > > +	struct imx_aux_reset_priv *priv;
> > > > +	struct device *dev = &adev->dev;
> > > > +
> > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > +	if (!priv)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	priv->rcdev.owner     = THIS_MODULE;
> > > > +	priv->rcdev.nr_resets = data->rst_ctrl_num;
> > > > +	priv->rcdev.ops       = &imx_aux_reset_ops;
> > > > +	priv->rcdev.of_node   = dev->parent->of_node;
> > > > +	priv->rcdev.dev	      = dev;
> > > > +	priv->rcdev.of_reset_n_cells = 1;
> > > > +	priv->base            = of_iomap(dev->parent->of_node, 0);
> > > > +	priv->data            = data;
> > > > +
> > > > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > > > +}
> > > > +
> > > > +#define EARC  0x200
> > > > +
> > > > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > > > +	{
> > > > +		.assert_offset = EARC,
> > > > +		.assert_bit = 0,
> > > > +		.deassert_offset = EARC,
> > > > +		.deassert_bit = 0,
> > > > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > > +	},
> > > > +	{
> > > > +		.assert_offset = EARC,
> > > > +		.assert_bit = 1,
> > > > +		.deassert_offset = EARC,
> > > > +		.deassert_bit = 1,
> > > > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > > +	},
> > > > +};
> > > > +
> > > > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > > > +	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > > > +	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > > > +};
> > > > +
> > > > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > > > +	{
> > > > +		.name = "clk_imx8mp_audiomix.reset",
> > > > +		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > > > +	},
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > > > +
> > > > +static struct auxiliary_driver imx_aux_reset_driver = {
> > > > +	.probe		= imx_aux_reset_probe,
> > > > +	.id_table	= imx_aux_reset_ids,
> > > > +};
> > > > +
> > > > +module_auxiliary_driver(imx_aux_reset_driver);
> > > > +
> > > > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > > > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > > > +MODULE_LICENSE("GPL");
> > > > -- 
> > > > 2.34.1
> > > > 
> > > > 
> > > > 
> > 

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Shengjiu Wang]

On Fri, May 31, 2024 at 2:34 AM Frank Li <Frank.li@nxp.com> wrote:
>
> On Thu, May 30, 2024 at 06:45:10PM +0200, Marco Felsch wrote:
> > On 24-05-30, Frank Li wrote:
> > > On Thu, May 30, 2024 at 11:05:58AM +0200, Marco Felsch wrote:
> > > > Hi,
> > > >
> > > > On 24-05-30, Shengjiu Wang wrote:
> > > > > Add support for the resets on i.MX8MP Audio Block Control module,
> > > > > which includes the EARC PHY software reset and EARC controller
> > > > > software reset. The reset controller is created using the auxiliary
> > > > > device framework and set up in the clock driver.
> > > > >
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > > ---
> > > > >  drivers/reset/Kconfig         |   8 ++
> > > > >  drivers/reset/Makefile        |   1 +
> > > > >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
> > > >                       ^
> > > > You make use of the auxiliary bus but this isn't a aux driver, it's the
> > > > i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> > > > are covered by the AUDIOMIX blk-ctrl.
> > > >
> > > > >  3 files changed, 226 insertions(+)
> > > > >  create mode 100644 drivers/reset/reset-imx-aux.c
> > > > >
> > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > > > index 7112f5932609..38fdf05b326b 100644
> > > > > --- a/drivers/reset/Kconfig
> > > > > +++ b/drivers/reset/Kconfig
> > > > > @@ -91,6 +91,14 @@ config RESET_IMX7
> > > > >         help
> > > > >           This enables the reset controller driver for i.MX7 SoCs.
> > > > >
> > > > > +config RESET_IMX_AUX
> > > > > +       tristate "i.MX Auxiliary Reset Driver"
> > > >                   ^
> > > >               Same applies here
> > > >
> > > > > +       depends on CLK_IMX8MP
> > > > > +       select AUXILIARY_BUS
> > > > > +       default CLK_IMX8MP
> > > > > +       help
> > > > > +         This enables the auxiliary reset controller driver for i.MX.
> > > > > +
> > > > >  config RESET_INTEL_GW
> > > > >         bool "Intel Reset Controller Driver"
> > > > >         depends on X86 || COMPILE_TEST
> > > > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > > > > index fd8b49fa46fc..f078da14c327 100644
> > > > > --- a/drivers/reset/Makefile
> > > > > +++ b/drivers/reset/Makefile
> > > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> > > > >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> > > > >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> > > > >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > > > > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> > > > >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> > > > >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> > > > >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > > > > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > > > > new file mode 100644
> > > > > index 000000000000..61c353abc84e
> > > > > --- /dev/null
> > > > > +++ b/drivers/reset/reset-imx-aux.c
> > > > > @@ -0,0 +1,217 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + * Copyright 2024 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/auxiliary_bus.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_address.h>
> > > > > +#include <linux/of_platform.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/reset-controller.h>
> > > > > +
> > > > > +/*
> > > > > + * The reset does not support the feature and corresponding
> > > > > + * values are not valid
> > > > > + */
> > > > > +#define ASSERT_NONE     BIT(0)
> > > > > +#define DEASSERT_NONE   BIT(1)
> > > > > +#define STATUS_NONE     BIT(2)
> > > > > +
> > > > > +/* When set this function is activated by setting(vs clearing) this bit */
> > > > > +#define ASSERT_SET      BIT(3)
> > > > > +#define DEASSERT_SET    BIT(4)
> > > > > +#define STATUS_SET      BIT(5)
> > > > > +
> > > > > +/* The following are the inverse of the above and are added for consistency */
> > > > > +#define ASSERT_CLEAR    (0 << 3)
> > > > > +#define DEASSERT_CLEAR  (0 << 4)
> > > > > +#define STATUS_CLEAR    (0 << 5)
> > > > > +
> > > > > +/**
> > > > > + * struct imx_reset_ctrl - reset control structure
> > > > > + * @assert_offset: reset assert control register offset
> > > > > + * @assert_bit: reset assert bit in the reset assert control register
> > > > > + * @deassert_offset: reset deassert control register offset
> > > > > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > > > > + * @status_offset: reset status register offset
> > > > > + * @status_bit: reset status bit in the reset status register
> > > > > + * @flags: reset flag indicating how the (de)assert and status are handled
> > > > > + */
> > > > > +struct imx_reset_ctrl {
> > > > > +       u32 assert_offset;
> > > > > +       u32 assert_bit;
> > > > > +       u32 deassert_offset;
> > > > > +       u32 deassert_bit;
> > > > > +       u32 status_offset;
> > > > > +       u32 status_bit;
> > > > > +       u32 flags;
> > > > > +};
> > > >
> > > > Why do we make it this compicated for an simple EARC module reset? I
> > > > understand that you want to provide a generic driver which can be
> > > > re-used but there is actual no other user and may will get no other user
> > > > in the future too. Therefore I would like to keep it simple at the
> > > > begin and adapt the code on-demand.
> > >
> > > There are many similar cases. such as
> > > https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/pci/controller/dwc/pci-layerscape.c#L251
> > >
> > > Previously it use syscon and regmap to a global register space region and
> > > direct operate the register. Now this way will not preferred. It needs
> > > export as reset driver. but actually, it just write some bits.
> >
> > It depends, if your reset-controller is part of an complete different
> > device like this EARC reset you're right else you can write to the reset
> > directly within you driver.
>
> These reset bits allocated a global register regions. Such as bit0 for
> pcie0, bit1 for pci1, bit2 for usb0, bit3 for usb1. Hardware pack these
> to misc mmio region.
>
> >
> > > We face the similar problem at difference driver when do upstream.
> > >
> > > One on going a discussion about sim module reset
> > > https://lore.kernel.org/imx/131e46b1-61d9-41de-a225-853b09c765d1@gmail.com/
> > >
> > > We hope an unified and simple method to handle these cases.
> >
> > An unified driver for non-unified reset modules? This makes no sense to
> > me. When it comes to possible quirk handling for different reset modules
> > your code gets even more complex.
> >
> > I'm fine with a common code base (driver) if NXP has an common reset
> > controller IP which is added to several SoC. There should be no common
> > code base if this isn't the case.
>
> Some chip have common reset controller IP. Some chip simple packed reset
> bits to one global misc mmio region with other misc control bit, such power
> on/off / clock source choose / enable debug features.
>
> Here we just consider the second case. Anyway we need handle these case at
> somewhere. 99% case just write bit<n> to register offset. Only difference
> is bit posstion and register offset. If write difference driver for these
> just because bit posstion or offset, there will be many duplicated codes
> under driver/reset.
>
> For example:
> Misc: 0x1000
>       bit0: pcie0 reset
>       bit2: usb0 reset
>       bit3-bit4: pcie clk source select
>       bit5-bit6: usb0 clk source select
>       bit7: pcie0 phy power on
>       bit8: usb0 phy power on.
>
> The difference chip, bit possition will be changed.
> How handle this case corporately?

What's the conclusion for this topic?
Should be the driver general for other cases or specific
for i.MX8MP AudioMIX?

Best regards
Shengjiu Wang
>
> >
> > Regards,
> >   Marco
> >
> >
> > >
> > > Frank
> > >
> > > >
> > > > Regards,
> > > >   Marco
> > > >
> > > > > +struct imx_reset_data {
> > > > > +       const struct imx_reset_ctrl *rst_ctrl;
> > > > > +       size_t rst_ctrl_num;
> > > > > +};
> > > > > +
> > > > > +struct imx_aux_reset_priv {
> > > > > +       struct reset_controller_dev rcdev;
> > > > > +       void __iomem *base;
> > > > > +       const struct imx_reset_data *data;
> > > > > +};
> > > > > +
> > > > > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > > > > +                               unsigned long id)
> > > > > +{
> > > > > +       struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > > +                                       struct imx_aux_reset_priv, rcdev);
> > > > > +       const struct imx_reset_data *data = priv->data;
> > > > > +       void __iomem *reg_addr = priv->base;
> > > > > +       const struct imx_reset_ctrl *ctrl;
> > > > > +       unsigned int mask, value, reg;
> > > > > +
> > > > > +       if (id >= data->rst_ctrl_num)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       ctrl = &data->rst_ctrl[id];
> > > > > +
> > > > > +       /* assert not supported for this reset */
> > > > > +       if (ctrl->flags & ASSERT_NONE)
> > > > > +               return -EOPNOTSUPP;
> > > > > +
> > > > > +       mask = BIT(ctrl->assert_bit);
> > > > > +       value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > > > > +
> > > > > +       reg = readl(reg_addr + ctrl->assert_offset);
> > > > > +       writel(reg | value, reg_addr + ctrl->assert_offset);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > > > > +                                 unsigned long id)
> > > > > +{
> > > > > +       struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > > +                                       struct imx_aux_reset_priv, rcdev);
> > > > > +       const struct imx_reset_data *data = priv->data;
> > > > > +       void __iomem *reg_addr = priv->base;
> > > > > +       const struct imx_reset_ctrl *ctrl;
> > > > > +       unsigned int mask, value, reg;
> > > > > +
> > > > > +       if (id >= data->rst_ctrl_num)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       ctrl = &data->rst_ctrl[id];
> > > > > +
> > > > > +       /* deassert not supported for this reset */
> > > > > +       if (ctrl->flags & DEASSERT_NONE)
> > > > > +               return -EOPNOTSUPP;
> > > > > +
> > > > > +       mask = BIT(ctrl->deassert_bit);
> > > > > +       value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > > > > +
> > > > > +       reg = readl(reg_addr + ctrl->deassert_offset);
> > > > > +       writel(reg | value, reg_addr + ctrl->deassert_offset);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > > > > +                               unsigned long id)
> > > > > +{
> > > > > +       struct imx_aux_reset_priv *priv = container_of(rcdev,
> > > > > +                                       struct imx_aux_reset_priv, rcdev);
> > > > > +       const struct imx_reset_data *data = priv->data;
> > > > > +       void __iomem *reg_addr = priv->base;
> > > > > +       const struct imx_reset_ctrl *ctrl;
> > > > > +       unsigned int reset_state;
> > > > > +
> > > > > +       if (id >= data->rst_ctrl_num)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       ctrl = &data->rst_ctrl[id];
> > > > > +
> > > > > +       /* status not supported for this reset */
> > > > > +       if (ctrl->flags & STATUS_NONE)
> > > > > +               return -EOPNOTSUPP;
> > > > > +
> > > > > +       reset_state = readl(reg_addr + ctrl->status_offset);
> > > > > +
> > > > > +       return !(reset_state & BIT(ctrl->status_bit)) ==
> > > > > +               !(ctrl->flags & STATUS_SET);
> > > > > +}
> > > > > +
> > > > > +static const struct reset_control_ops imx_aux_reset_ops = {
> > > > > +       .assert   = imx_aux_reset_assert,
> > > > > +       .deassert = imx_aux_reset_deassert,
> > > > > +       .status   = imx_aux_reset_status,
> > > > > +};
> > > > > +
> > > > > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > > > > +                              const struct auxiliary_device_id *id)
> > > > > +{
> > > > > +       struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > > > > +       struct imx_aux_reset_priv *priv;
> > > > > +       struct device *dev = &adev->dev;
> > > > > +
> > > > > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > > +       if (!priv)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       priv->rcdev.owner     = THIS_MODULE;
> > > > > +       priv->rcdev.nr_resets = data->rst_ctrl_num;
> > > > > +       priv->rcdev.ops       = &imx_aux_reset_ops;
> > > > > +       priv->rcdev.of_node   = dev->parent->of_node;
> > > > > +       priv->rcdev.dev       = dev;
> > > > > +       priv->rcdev.of_reset_n_cells = 1;
> > > > > +       priv->base            = of_iomap(dev->parent->of_node, 0);
> > > > > +       priv->data            = data;
> > > > > +
> > > > > +       return devm_reset_controller_register(dev, &priv->rcdev);
> > > > > +}
> > > > > +
> > > > > +#define EARC  0x200
> > > > > +
> > > > > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > > > > +       {
> > > > > +               .assert_offset = EARC,
> > > > > +               .assert_bit = 0,
> > > > > +               .deassert_offset = EARC,
> > > > > +               .deassert_bit = 0,
> > > > > +               .flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > > > +       },
> > > > > +       {
> > > > > +               .assert_offset = EARC,
> > > > > +               .assert_bit = 1,
> > > > > +               .deassert_offset = EARC,
> > > > > +               .deassert_bit = 1,
> > > > > +               .flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > > > > +       },
> > > > > +};
> > > > > +
> > > > > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > > > > +       .rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > > > > +       .rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > > > > +};
> > > > > +
> > > > > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > > > > +       {
> > > > > +               .name = "clk_imx8mp_audiomix.reset",
> > > > > +               .driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > > > > +       },
> > > > > +       { }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > > > > +
> > > > > +static struct auxiliary_driver imx_aux_reset_driver = {
> > > > > +       .probe          = imx_aux_reset_probe,
> > > > > +       .id_table       = imx_aux_reset_ids,
> > > > > +};
> > > > > +
> > > > > +module_auxiliary_driver(imx_aux_reset_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > > > > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > > > > +MODULE_LICENSE("GPL");
> > > > > --
> > > > > 2.34.1
> > > > >
> > > > >
> > > > >
> > >

Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver

[Shengjiu Wang]

On Thu, May 30, 2024 at 5:06 PM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi,
>
> On 24-05-30, Shengjiu Wang wrote:
> > Add support for the resets on i.MX8MP Audio Block Control module,
> > which includes the EARC PHY software reset and EARC controller
> > software reset. The reset controller is created using the auxiliary
> > device framework and set up in the clock driver.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/reset/Kconfig         |   8 ++
> >  drivers/reset/Makefile        |   1 +
> >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
>                             ^
> You make use of the auxiliary bus but this isn't a aux driver, it's the
> i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> are covered by the AUDIOMIX blk-ctrl.

I am confused. According to below discussion:

https://lore.kernel.org/lkml/b86c83a520f0c45a60249468fa92b1de.sboyd@kernel.org/

Stephen and Conor suggested using auxdev.


Best regards
Shengjiu Wang

>
> >  3 files changed, 226 insertions(+)
> >  create mode 100644 drivers/reset/reset-imx-aux.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 7112f5932609..38fdf05b326b 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -91,6 +91,14 @@ config RESET_IMX7
> >       help
> >         This enables the reset controller driver for i.MX7 SoCs.
> >
> > +config RESET_IMX_AUX
> > +     tristate "i.MX Auxiliary Reset Driver"
>                         ^
>               Same applies here
>
> > +     depends on CLK_IMX8MP
> > +     select AUXILIARY_BUS
> > +     default CLK_IMX8MP
> > +     help
> > +       This enables the auxiliary reset controller driver for i.MX.
> > +
> >  config RESET_INTEL_GW
> >       bool "Intel Reset Controller Driver"
> >       depends on X86 || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index fd8b49fa46fc..f078da14c327 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > new file mode 100644
> > index 000000000000..61c353abc84e
> > --- /dev/null
> > +++ b/drivers/reset/reset-imx-aux.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +
> > +/*
> > + * The reset does not support the feature and corresponding
> > + * values are not valid
> > + */
> > +#define ASSERT_NONE     BIT(0)
> > +#define DEASSERT_NONE   BIT(1)
> > +#define STATUS_NONE     BIT(2)
> > +
> > +/* When set this function is activated by setting(vs clearing) this bit */
> > +#define ASSERT_SET      BIT(3)
> > +#define DEASSERT_SET    BIT(4)
> > +#define STATUS_SET      BIT(5)
> > +
> > +/* The following are the inverse of the above and are added for consistency */
> > +#define ASSERT_CLEAR    (0 << 3)
> > +#define DEASSERT_CLEAR  (0 << 4)
> > +#define STATUS_CLEAR    (0 << 5)
> > +
> > +/**
> > + * struct imx_reset_ctrl - reset control structure
> > + * @assert_offset: reset assert control register offset
> > + * @assert_bit: reset assert bit in the reset assert control register
> > + * @deassert_offset: reset deassert control register offset
> > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > + * @status_offset: reset status register offset
> > + * @status_bit: reset status bit in the reset status register
> > + * @flags: reset flag indicating how the (de)assert and status are handled
> > + */
> > +struct imx_reset_ctrl {
> > +     u32 assert_offset;
> > +     u32 assert_bit;
> > +     u32 deassert_offset;
> > +     u32 deassert_bit;
> > +     u32 status_offset;
> > +     u32 status_bit;
> > +     u32 flags;
> > +};
>
> Why do we make it this compicated for an simple EARC module reset? I
> understand that you want to provide a generic driver which can be
> re-used but there is actual no other user and may will get no other user
> in the future too. Therefore I would like to keep it simple at the
> begin and adapt the code on-demand.
>
> Regards,
>   Marco
>
> > +struct imx_reset_data {
> > +     const struct imx_reset_ctrl *rst_ctrl;
> > +     size_t rst_ctrl_num;
> > +};
> > +
> > +struct imx_aux_reset_priv {
> > +     struct reset_controller_dev rcdev;
> > +     void __iomem *base;
> > +     const struct imx_reset_data *data;
> > +};
> > +
> > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > +                             unsigned long id)
> > +{
> > +     struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +                                     struct imx_aux_reset_priv, rcdev);
> > +     const struct imx_reset_data *data = priv->data;
> > +     void __iomem *reg_addr = priv->base;
> > +     const struct imx_reset_ctrl *ctrl;
> > +     unsigned int mask, value, reg;
> > +
> > +     if (id >= data->rst_ctrl_num)
> > +             return -EINVAL;
> > +
> > +     ctrl = &data->rst_ctrl[id];
> > +
> > +     /* assert not supported for this reset */
> > +     if (ctrl->flags & ASSERT_NONE)
> > +             return -EOPNOTSUPP;
> > +
> > +     mask = BIT(ctrl->assert_bit);
> > +     value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > +
> > +     reg = readl(reg_addr + ctrl->assert_offset);
> > +     writel(reg | value, reg_addr + ctrl->assert_offset);
> > +
> > +     return 0;
> > +}
> > +
> > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > +                               unsigned long id)
> > +{
> > +     struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +                                     struct imx_aux_reset_priv, rcdev);
> > +     const struct imx_reset_data *data = priv->data;
> > +     void __iomem *reg_addr = priv->base;
> > +     const struct imx_reset_ctrl *ctrl;
> > +     unsigned int mask, value, reg;
> > +
> > +     if (id >= data->rst_ctrl_num)
> > +             return -EINVAL;
> > +
> > +     ctrl = &data->rst_ctrl[id];
> > +
> > +     /* deassert not supported for this reset */
> > +     if (ctrl->flags & DEASSERT_NONE)
> > +             return -EOPNOTSUPP;
> > +
> > +     mask = BIT(ctrl->deassert_bit);
> > +     value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > +
> > +     reg = readl(reg_addr + ctrl->deassert_offset);
> > +     writel(reg | value, reg_addr + ctrl->deassert_offset);
> > +
> > +     return 0;
> > +}
> > +
> > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > +                             unsigned long id)
> > +{
> > +     struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +                                     struct imx_aux_reset_priv, rcdev);
> > +     const struct imx_reset_data *data = priv->data;
> > +     void __iomem *reg_addr = priv->base;
> > +     const struct imx_reset_ctrl *ctrl;
> > +     unsigned int reset_state;
> > +
> > +     if (id >= data->rst_ctrl_num)
> > +             return -EINVAL;
> > +
> > +     ctrl = &data->rst_ctrl[id];
> > +
> > +     /* status not supported for this reset */
> > +     if (ctrl->flags & STATUS_NONE)
> > +             return -EOPNOTSUPP;
> > +
> > +     reset_state = readl(reg_addr + ctrl->status_offset);
> > +
> > +     return !(reset_state & BIT(ctrl->status_bit)) ==
> > +             !(ctrl->flags & STATUS_SET);
> > +}
> > +
> > +static const struct reset_control_ops imx_aux_reset_ops = {
> > +     .assert   = imx_aux_reset_assert,
> > +     .deassert = imx_aux_reset_deassert,
> > +     .status   = imx_aux_reset_status,
> > +};
> > +
> > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > +                            const struct auxiliary_device_id *id)
> > +{
> > +     struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > +     struct imx_aux_reset_priv *priv;
> > +     struct device *dev = &adev->dev;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->rcdev.owner     = THIS_MODULE;
> > +     priv->rcdev.nr_resets = data->rst_ctrl_num;
> > +     priv->rcdev.ops       = &imx_aux_reset_ops;
> > +     priv->rcdev.of_node   = dev->parent->of_node;
> > +     priv->rcdev.dev       = dev;
> > +     priv->rcdev.of_reset_n_cells = 1;
> > +     priv->base            = of_iomap(dev->parent->of_node, 0);
> > +     priv->data            = data;
> > +
> > +     return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> > +#define EARC  0x200
> > +
> > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > +     {
> > +             .assert_offset = EARC,
> > +             .assert_bit = 0,
> > +             .deassert_offset = EARC,
> > +             .deassert_bit = 0,
> > +             .flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +     },
> > +     {
> > +             .assert_offset = EARC,
> > +             .assert_bit = 1,
> > +             .deassert_offset = EARC,
> > +             .deassert_bit = 1,
> > +             .flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +     },
> > +};
> > +
> > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > +     .rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > +     .rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > +};
> > +
> > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > +     {
> > +             .name = "clk_imx8mp_audiomix.reset",
> > +             .driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > +     },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > +
> > +static struct auxiliary_driver imx_aux_reset_driver = {
> > +     .probe          = imx_aux_reset_probe,
> > +     .id_table       = imx_aux_reset_ids,
> > +};
> > +
> > +module_auxiliary_driver(imx_aux_reset_driver);
> > +
> > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
> >
> >

[PATCH v6 4/5] clk: imx: clk-audiomix: Add CLK_SET_RATE_PARENT flags for clocks

[Shengjiu Wang]

Add CLK_SET_RATE_PARENT flags that when the device driver sets the
child clock rate, parent clock frequency can be refined accordingly.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-imx8mp-audiomix.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
index a3dc2f3606ee..ad6eceb419cc 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -325,12 +325,12 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 	for (i = 0; i < ARRAY_SIZE(sels); i++) {
 		if (sels[i].num_parents == 1) {
 			hw = devm_clk_hw_register_gate_parent_data(dev,
-				sels[i].name, &sels[i].parent, 0,
+				sels[i].name, &sels[i].parent, CLK_SET_RATE_PARENT,
 				base + sels[i].reg, sels[i].shift, 0, NULL);
 		} else {
 			hw = devm_clk_hw_register_mux_parent_data_table(dev,
 				sels[i].name, sels[i].parents,
-				sels[i].num_parents, 0,
+				sels[i].num_parents, CLK_SET_RATE_PARENT,
 				base + sels[i].reg,
 				sels[i].shift, sels[i].width,
 				0, NULL, NULL);
@@ -373,7 +373,8 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 	clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_BYPASS] = hw;
 
 	hw = devm_clk_hw_register_gate(dev, "sai_pll_out", "sai_pll_bypass",
-				       0, base + SAI_PLL_GNRL_CTL, 13,
+				       CLK_SET_RATE_PARENT,
+				       base + SAI_PLL_GNRL_CTL, 13,
 				       0, NULL);
 	if (IS_ERR(hw)) {
 		ret = PTR_ERR(hw);
@@ -382,7 +383,8 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 	clk_hw_data->hws[IMX8MP_CLK_AUDIOMIX_SAI_PLL_OUT] = hw;
 
 	hw = devm_clk_hw_register_fixed_factor(dev, "sai_pll_out_div2",
-					       "sai_pll_out", 0, 1, 2);
+					       "sai_pll_out",
+					       CLK_SET_RATE_PARENT, 1, 2);
 	if (IS_ERR(hw)) {
 		ret = PTR_ERR(hw);
 		goto err_clk_register;
-- 
2.34.1

[PATCH v6 5/5] clk: imx: clk-audiomix: Correct parent clock for earc_phy and audpll

[Shengjiu Wang]

According to Reference Manual of i.MX8MP
The parent clock of "earc_phy" is "sai_pll_out_div2",
The parent clock of "audpll" is "osc_24m".

Add CLK_GATE_PARENT() macro for usage of specifying parent clock.

Fixes: 6cd95f7b151c ("clk: imx: imx8mp: Add audiomix block control")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-imx8mp-audiomix.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
index ad6eceb419cc..d8678b3230e4 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -156,6 +156,15 @@ static const struct clk_parent_data clk_imx8mp_audiomix_pll_bypass_sels[] = {
 		PDM_SEL, 2, 0						\
 	}
 
+#define CLK_GATE_PARENT(gname, cname, pname)						\
+	{								\
+		gname"_cg",						\
+		IMX8MP_CLK_AUDIOMIX_##cname,				\
+		{ .fw_name = pname, .name = pname }, NULL, 1,		\
+		CLKEN0 + 4 * !!(IMX8MP_CLK_AUDIOMIX_##cname / 32),	\
+		1, IMX8MP_CLK_AUDIOMIX_##cname % 32			\
+	}
+
 struct clk_imx8mp_audiomix_sel {
 	const char			*name;
 	int				clkid;
@@ -173,14 +182,14 @@ static struct clk_imx8mp_audiomix_sel sels[] = {
 	CLK_GATE("earc", EARC_IPG),
 	CLK_GATE("ocrama", OCRAMA_IPG),
 	CLK_GATE("aud2htx", AUD2HTX_IPG),
-	CLK_GATE("earc_phy", EARC_PHY),
+	CLK_GATE_PARENT("earc_phy", EARC_PHY, "sai_pll_out_div2"),
 	CLK_GATE("sdma2", SDMA2_ROOT),
 	CLK_GATE("sdma3", SDMA3_ROOT),
 	CLK_GATE("spba2", SPBA2_ROOT),
 	CLK_GATE("dsp", DSP_ROOT),
 	CLK_GATE("dspdbg", DSPDBG_ROOT),
 	CLK_GATE("edma", EDMA_ROOT),
-	CLK_GATE("audpll", AUDPLL_ROOT),
+	CLK_GATE_PARENT("audpll", AUDPLL_ROOT, "osc_24m"),
 	CLK_GATE("mu2", MU2_ROOT),
 	CLK_GATE("mu3", MU3_ROOT),
 	CLK_PDM,
-- 
2.34.1