[PATCH v3 0/6] clk: imx: clk-audiomix: Improvement for audiomix

[PATCH v3 0/6] 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
Corrent parent clock for earc_phy and audpll clocks.

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 (6):
  dt-bindings: reset: fsl,imx8mp-audiomix-reset: add bindings
  reset: imx8mp-audiomix: Add AudioMix Block Control reset driver
  dt-bindings: clock: imx8mp: Add reset-controller sub-node
  clk: imx: clk-audiomix: Add CLK_SET_RATE_PARENT flags for clocks
  clk: imx: clk-audiomix: Corrent parent clock for earc_phy and audpll
  arm64: dts: imx8mp: Add reset-controller sub node for audio_blk_ctrl

 .../bindings/clock/imx8mp-audiomix.yaml       |  17 ++-
 .../reset/fsl,imx8mp-audiomix-reset.yaml      |  37 ++++++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |   7 +-
 drivers/clk/imx/clk-imx8mp-audiomix.c         |  23 +++-
 drivers/reset/Kconfig                         |   8 ++
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-imx8mp-audiomix.c         | 117 ++++++++++++++++++
 7 files changed, 201 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx8mp-audiomix-reset.yaml
 create mode 100644 drivers/reset/reset-imx8mp-audiomix.c

-- 
2.34.1

[PATCH v3 1/6] dt-bindings: reset: fsl,imx8mp-audiomix-reset: add bindings

[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.

Add DT-Schema bindings for the reset function in i.MX8MP audiomix
block controller.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../reset/fsl,imx8mp-audiomix-reset.yaml      | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx8mp-audiomix-reset.yaml

diff --git a/Documentation/devicetree/bindings/reset/fsl,imx8mp-audiomix-reset.yaml b/Documentation/devicetree/bindings/reset/fsl,imx8mp-audiomix-reset.yaml
new file mode 100644
index 000000000000..71a10646ab2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/fsl,imx8mp-audiomix-reset.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX8MP AudioMix reset controller
+
+maintainers:
+  - Shengjiu Wang <shengjiu.wang@nxp.com>
+
+description: The reset controller node must be a sub-node of the i.MX8MP
+  AudioMix Block Control node
+
+properties:
+  $nodename:
+    const: reset-controller
+
+  compatible:
+    const: fsl,imx8mp-audiomix-reset
+
+  '#reset-cells':
+    const: 1
+
+required:
+  - compatible
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    reset-controller {
+        compatible = "fsl,imx8mp-audiomix-reset";
+        #reset-cells = <1>;
+    };
-- 
2.34.1

[PATCH v3 2/6] reset: imx8mp-audiomix: Add AudioMix Block Control reset driver

[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 EARC PHY software reset and EARC controller software
reset need to be supported.

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

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 7112f5932609..0e7da0bb0a21 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_IMX8MP_AUDIOMIX
+	tristate "i.MX8MP AudioMix Reset Driver"
+	depends on HAS_IOMEM
+	depends on (ARM64 && ARCH_MXC) || COMPILE_TEST
+	select MFD_SYSCON
+	help
+	  This enables the reset controller driver for i.MX8MP AudioMix.
+
 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..a6796e83900b 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_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.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-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
new file mode 100644
index 000000000000..8ba0d4406b36
--- /dev/null
+++ b/drivers/reset/reset-imx8mp-audiomix.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#define EARC			0x200
+#define EARC_RESET_MASK		0x3
+
+struct imx8mp_audiomix_rst_priv {
+	struct regmap *regmap;
+	struct reset_controller_dev rcdev;
+};
+
+static int imx8mp_audiomix_reset_set(struct reset_controller_dev *rcdev,
+				     unsigned long id, bool assert)
+{
+	struct imx8mp_audiomix_rst_priv *priv = container_of(rcdev,
+				struct imx8mp_audiomix_rst_priv, rcdev);
+	unsigned int mask = BIT(id);
+
+	/* bit = 0 reset, bit = 1 unreset */
+	if (assert)
+		regmap_update_bits(priv->regmap, EARC, mask, 0);
+	else
+		regmap_update_bits(priv->regmap, EARC, mask, mask);
+
+	return 0;
+}
+
+static int imx8mp_audiomix_reset_reset(struct reset_controller_dev *rcdev,
+				       unsigned long id)
+{
+	imx8mp_audiomix_reset_set(rcdev, id, true);
+
+	return imx8mp_audiomix_reset_set(rcdev, id, false);
+}
+
+static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
+					unsigned long id)
+{
+	return imx8mp_audiomix_reset_set(rcdev, id, true);
+}
+
+static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
+					  unsigned long id)
+{
+	return imx8mp_audiomix_reset_set(rcdev, id, false);
+}
+
+static int imx8mp_audiomix_reset_xlate(struct reset_controller_dev *rcdev,
+				       const struct of_phandle_args *reset_spec)
+{
+	unsigned long id = reset_spec->args[0];
+
+	if (!(BIT(id) & EARC_RESET_MASK))
+		return -EINVAL;
+
+	return id;
+}
+
+static const struct reset_control_ops imx8mp_audiomix_reset_ops = {
+	.reset = imx8mp_audiomix_reset_reset,
+	.assert = imx8mp_audiomix_reset_assert,
+	.deassert = imx8mp_audiomix_reset_deassert,
+};
+
+static int imx8mp_audiomix_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
+	struct imx8mp_audiomix_rst_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->rcdev.owner     = THIS_MODULE;
+	priv->rcdev.nr_resets = fls(EARC_RESET_MASK);
+	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
+	priv->rcdev.of_node   = pdev->dev.of_node;
+	priv->rcdev.dev	      = &pdev->dev;
+	priv->rcdev.of_reset_n_cells = 1;
+	priv->rcdev.of_xlate  = imx8mp_audiomix_reset_xlate;
+
+	return devm_reset_controller_register(&pdev->dev, &priv->rcdev);
+}
+
+static const struct of_device_id imx8mp_audiomix_reset_dt_match[] = {
+	{ .compatible = "fsl,imx8mp-audiomix-reset" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx8mp_audiomix_reset_dt_match);
+
+static struct platform_driver imx8mp_audiomix_reset_driver = {
+	.probe	= imx8mp_audiomix_reset_probe,
+	.driver	= {
+		.name = "imx8mp-audiomix-reset",
+		.of_match_table = imx8mp_audiomix_reset_dt_match,
+	},
+};
+module_platform_driver(imx8mp_audiomix_reset_driver);
+
+MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
+MODULE_DESCRIPTION("Freescale i.MX8MP Audio Block Controller reset driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[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 Audio Block Control is a Multi-Function Devices.

Add reset-controller sub node which is a reset provider for EARC.
Add compatible string "syscon", "simple-mfd" which make Audio
Block Control device support reset-controller sub-node.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../bindings/clock/imx8mp-audiomix.yaml         | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
index 0a6dc1a6e122..a403ace4d11f 100644
--- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
+++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
@@ -15,7 +15,10 @@ description: |
 
 properties:
   compatible:
-    const: fsl,imx8mp-audio-blk-ctrl
+    items:
+      - const: fsl,imx8mp-audio-blk-ctrl
+      - const: syscon
+      - const: simple-mfd
 
   reg:
     maxItems: 1
@@ -44,6 +47,11 @@ 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-controller:
+    type: object
+    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
+    description: The child reset devices of AudioMIX Block Control.
+
 required:
   - compatible
   - reg
@@ -60,7 +68,7 @@ examples:
     #include <dt-bindings/clock/imx8mp-clock.h>
 
     clock-controller@30e20000 {
-        compatible = "fsl,imx8mp-audio-blk-ctrl";
+        compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
         reg = <0x30e20000 0x10000>;
         #clock-cells = <1>;
         clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>,
@@ -74,6 +82,11 @@ examples:
                       "sai1", "sai2", "sai3",
                       "sai5", "sai6", "sai7";
         power-domains = <&pgc_audio>;
+
+        reset-controller {
+            compatible = "fsl,imx8mp-audiomix-reset";
+            #reset-cells = <1>;
+        };
     };
 
 ...
-- 
2.34.1

[PATCH v3 4/6] 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>
---
 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 b381d6f784c8..466b5b0d665c 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -269,12 +269,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);
@@ -317,7 +317,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);
@@ -326,7 +327,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 v3 5/6] clk: imx: clk-audiomix: Corrent 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>
---
 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 466b5b0d665c..f4a02ae7e64f 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -154,6 +154,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;
@@ -171,14 +180,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

[PATCH v3 6/6] arm64: dts: imx8mp: Add reset-controller sub node for audio_blk_ctrl

[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

Add reset-controller sub-node for audio_blk_ctrl.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 459c4a54d30e..f94702ad4210 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1565,7 +1565,7 @@ sdma2: dma-controller@30e10000 {
 			};
 
 			audio_blk_ctrl: clock-controller@30e20000 {
-				compatible = "fsl,imx8mp-audio-blk-ctrl";
+				compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
 				reg = <0x30e20000 0x10000>;
 				#clock-cells = <1>;
 				clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>,
@@ -1582,6 +1582,11 @@ audio_blk_ctrl: clock-controller@30e20000 {
 				assigned-clocks = <&clk IMX8MP_AUDIO_PLL1>,
 						  <&clk IMX8MP_AUDIO_PLL2>;
 				assigned-clock-rates = <393216000>, <361267200>;
+
+				audio_blk_ctrl_rst: reset-controller {
+					compatible = "fsl,imx8mp-audiomix-reset";
+					#reset-cells = <1>;
+				};
 			};
 		};
 
-- 
2.34.1

Re: [PATCH v3 2/6] reset: imx8mp-audiomix: Add AudioMix Block Control reset driver

[Frank Li]

On Tue, May 14, 2024 at 05:33:26PM +0800, 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 EARC PHY software reset and EARC controller software
> reset need to be supported.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/reset/Kconfig                 |   8 ++
>  drivers/reset/Makefile                |   1 +
>  drivers/reset/reset-imx8mp-audiomix.c | 117 ++++++++++++++++++++++++++

It is just some bit change. Can you reuse reset-ti-syscon.c ? 

Frank

>  3 files changed, 126 insertions(+)
>  create mode 100644 drivers/reset/reset-imx8mp-audiomix.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7112f5932609..0e7da0bb0a21 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_IMX8MP_AUDIOMIX
> +	tristate "i.MX8MP AudioMix Reset Driver"
> +	depends on HAS_IOMEM
> +	depends on (ARM64 && ARCH_MXC) || COMPILE_TEST
> +	select MFD_SYSCON
> +	help
> +	  This enables the reset controller driver for i.MX8MP AudioMix.
> +
>  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..a6796e83900b 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_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.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-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c
> new file mode 100644
> index 000000000000..8ba0d4406b36
> --- /dev/null
> +++ b/drivers/reset/reset-imx8mp-audiomix.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#define EARC			0x200
> +#define EARC_RESET_MASK		0x3
> +
> +struct imx8mp_audiomix_rst_priv {
> +	struct regmap *regmap;
> +	struct reset_controller_dev rcdev;
> +};
> +
> +static int imx8mp_audiomix_reset_set(struct reset_controller_dev *rcdev,
> +				     unsigned long id, bool assert)
> +{
> +	struct imx8mp_audiomix_rst_priv *priv = container_of(rcdev,
> +				struct imx8mp_audiomix_rst_priv, rcdev);
> +	unsigned int mask = BIT(id);
> +
> +	/* bit = 0 reset, bit = 1 unreset */
> +	if (assert)
> +		regmap_update_bits(priv->regmap, EARC, mask, 0);
> +	else
> +		regmap_update_bits(priv->regmap, EARC, mask, mask);
> +
> +	return 0;
> +}
> +
> +static int imx8mp_audiomix_reset_reset(struct reset_controller_dev *rcdev,
> +				       unsigned long id)
> +{
> +	imx8mp_audiomix_reset_set(rcdev, id, true);
> +
> +	return imx8mp_audiomix_reset_set(rcdev, id, false);
> +}
> +
> +static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
> +					unsigned long id)
> +{
> +	return imx8mp_audiomix_reset_set(rcdev, id, true);
> +}
> +
> +static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
> +					  unsigned long id)
> +{
> +	return imx8mp_audiomix_reset_set(rcdev, id, false);
> +}
> +
> +static int imx8mp_audiomix_reset_xlate(struct reset_controller_dev *rcdev,
> +				       const struct of_phandle_args *reset_spec)
> +{
> +	unsigned long id = reset_spec->args[0];
> +
> +	if (!(BIT(id) & EARC_RESET_MASK))
> +		return -EINVAL;
> +
> +	return id;
> +}
> +
> +static const struct reset_control_ops imx8mp_audiomix_reset_ops = {
> +	.reset = imx8mp_audiomix_reset_reset,
> +	.assert = imx8mp_audiomix_reset_assert,
> +	.deassert = imx8mp_audiomix_reset_deassert,
> +};
> +
> +static int imx8mp_audiomix_reset_probe(struct platform_device *pdev)
> +{
> +	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> +	struct imx8mp_audiomix_rst_priv *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = syscon_node_to_regmap(parent_np);
> +	of_node_put(parent_np);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->rcdev.owner     = THIS_MODULE;
> +	priv->rcdev.nr_resets = fls(EARC_RESET_MASK);
> +	priv->rcdev.ops       = &imx8mp_audiomix_reset_ops;
> +	priv->rcdev.of_node   = pdev->dev.of_node;
> +	priv->rcdev.dev	      = &pdev->dev;
> +	priv->rcdev.of_reset_n_cells = 1;
> +	priv->rcdev.of_xlate  = imx8mp_audiomix_reset_xlate;
> +
> +	return devm_reset_controller_register(&pdev->dev, &priv->rcdev);
> +}
> +
> +static const struct of_device_id imx8mp_audiomix_reset_dt_match[] = {
> +	{ .compatible = "fsl,imx8mp-audiomix-reset" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx8mp_audiomix_reset_dt_match);
> +
> +static struct platform_driver imx8mp_audiomix_reset_driver = {
> +	.probe	= imx8mp_audiomix_reset_probe,
> +	.driver	= {
> +		.name = "imx8mp-audiomix-reset",
> +		.of_match_table = imx8mp_audiomix_reset_dt_match,
> +	},
> +};
> +module_platform_driver(imx8mp_audiomix_reset_driver);
> +
> +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> +MODULE_DESCRIPTION("Freescale i.MX8MP Audio Block Controller reset driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> 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 Audio Block Control is a Multi-Function Devices.
> 
> Add reset-controller sub node which is a reset provider for EARC.
> Add compatible string "syscon", "simple-mfd" which make Audio
> Block Control device support reset-controller sub-node.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  .../bindings/clock/imx8mp-audiomix.yaml         | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> index 0a6dc1a6e122..a403ace4d11f 100644
> --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> @@ -15,7 +15,10 @@ description: |
>  
>  properties:
>    compatible:
> -    const: fsl,imx8mp-audio-blk-ctrl
> +    items:
> +      - const: fsl,imx8mp-audio-blk-ctrl
> +      - const: syscon
> +      - const: simple-mfd
>  
>    reg:
>      maxItems: 1
> @@ -44,6 +47,11 @@ 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-controller:
> +    type: object
> +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> +    description: The child reset devices of AudioMIX Block Control.

Why not just set #reset-cells = <1> in the existing node? IIRC it was
already suggested to you to do that and use auxdev to set up the reset
driver.

Cheers,
Conor.

> +
>  required:
>    - compatible
>    - reg
> @@ -60,7 +68,7 @@ examples:
>      #include <dt-bindings/clock/imx8mp-clock.h>
>  
>      clock-controller@30e20000 {
> -        compatible = "fsl,imx8mp-audio-blk-ctrl";
> +        compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
>          reg = <0x30e20000 0x10000>;
>          #clock-cells = <1>;
>          clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>,
> @@ -74,6 +82,11 @@ examples:
>                        "sai1", "sai2", "sai3",
>                        "sai5", "sai6", "sai7";
>          power-domains = <&pgc_audio>;
> +
> +        reset-controller {
> +            compatible = "fsl,imx8mp-audiomix-reset";
> +            #reset-cells = <1>;
> +        };
>      };
>  
>  ...
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Stephen Boyd]

Quoting Conor Dooley (2024-05-14 11:06:14)
> On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > index 0a6dc1a6e122..a403ace4d11f 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > @@ -15,7 +15,10 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    const: fsl,imx8mp-audio-blk-ctrl
> > +    items:
> > +      - const: fsl,imx8mp-audio-blk-ctrl
> > +      - const: syscon
> > +      - const: simple-mfd
> >  
> >    reg:
> >      maxItems: 1
> > @@ -44,6 +47,11 @@ 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-controller:
> > +    type: object
> > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > +    description: The child reset devices of AudioMIX Block Control.
> 
> Why not just set #reset-cells = <1> in the existing node? IIRC it was
> already suggested to you to do that and use auxdev to set up the reset
> driver.

Yes, do that.

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Shengjiu Wang]

On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Conor Dooley (2024-05-14 11:06:14)
> > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > @@ -15,7 +15,10 @@ description: |
> > >
> > >  properties:
> > >    compatible:
> > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > +    items:
> > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > +      - const: syscon
> > > +      - const: simple-mfd
> > >
> > >    reg:
> > >      maxItems: 1
> > > @@ -44,6 +47,11 @@ 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-controller:
> > > +    type: object
> > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > +    description: The child reset devices of AudioMIX Block Control.
> >
> > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > already suggested to you to do that and use auxdev to set up the reset
> > driver.
>
> Yes, do that.

Can I know why sub nodes can't be used? the relationship of parent and
child devices looks better with sub nodes.

A further question is can I use the reset-ti-syscon? which is a generic reset
device for SoCs.  with it I don't even need to write a new reset device driver.
it is more simple.

Best regards
Shengjiu Wang

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Shengjiu Wang]

On Wed, May 15, 2024 at 10:47 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > @@ -15,7 +15,10 @@ description: |
> > > >
> > > >  properties:
> > > >    compatible:
> > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > +    items:
> > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > +      - const: syscon
> > > > +      - const: simple-mfd
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > +    type: object
> > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > +    description: The child reset devices of AudioMIX Block Control.
> > >
> > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > already suggested to you to do that and use auxdev to set up the reset
> > > driver.
> >
> > Yes, do that.
>
> Can I know why sub nodes can't be used? the relationship of parent and
> child devices looks better with sub nodes.
>
> A further question is can I use the reset-ti-syscon? which is a generic reset
> device for SoCs.  with it I don't even need to write a new reset device driver.
> it is more simple.
>
The document link is:
https://www.kernel.org/doc/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt

Then example is:
examples:
  # Clock Control Module node:
  - |
    #include <dt-bindings/clock/imx8mp-clock.h>
    #include <dt-bindings/reset/ti-syscon.h>

    clock-controller@30e20000 {
        compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
        reg = <0x30e20000 0x10000>;
        #clock-cells = <1>;
        clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>,
                 <&clk IMX8MP_CLK_SAI1>,
                 <&clk IMX8MP_CLK_SAI2>,
                 <&clk IMX8MP_CLK_SAI3>,
                 <&clk IMX8MP_CLK_SAI5>,
                 <&clk IMX8MP_CLK_SAI6>,
                 <&clk IMX8MP_CLK_SAI7>;
        clock-names = "ahb",
                      "sai1", "sai2", "sai3",
                      "sai5", "sai6", "sai7";
        power-domains = <&pgc_audio>;

        reset-controller {
            compatible = "ti,syscon-reset";
            #reset-cells = <1>;
            ti,reset-bits = <
                0x200 0 0x200 0 0 0 (ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE)
                0x200 1 0x200 1 0 0 (ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE)
            >;
        };
    };

Best regards
Shengjiu Wang

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote:
> On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > @@ -15,7 +15,10 @@ description: |
> > > >
> > > >  properties:
> > > >    compatible:
> > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > +    items:
> > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > +      - const: syscon
> > > > +      - const: simple-mfd
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > +    type: object
> > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > +    description: The child reset devices of AudioMIX Block Control.
> > >
> > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > already suggested to you to do that and use auxdev to set up the reset
> > > driver.
> >
> > Yes, do that.
> 
> Can I know why sub nodes can't be used? the relationship of parent and
> child devices looks better with sub nodes.

That's pretty subjective. I don't think it looks better to have a clock
node that is also a syscon with a reset child node as it is rather
inconsistent.
> 
> A further question is can I use the reset-ti-syscon? which is a generic reset
> device for SoCs.  with it I don't even need to write a new reset device driver.
> it is more simple.

That is for a TI SoC. You're working on an imx. I don't think that you
should be using that...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Frank Li]

On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote:
> On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote:
> > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > @@ -15,7 +15,10 @@ description: |
> > > > >
> > > > >  properties:
> > > > >    compatible:
> > > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > > +    items:
> > > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > > +      - const: syscon
> > > > > +      - const: simple-mfd
> > > > >
> > > > >    reg:
> > > > >      maxItems: 1
> > > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > > +    type: object
> > > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > > +    description: The child reset devices of AudioMIX Block Control.
> > > >
> > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > > already suggested to you to do that and use auxdev to set up the reset
> > > > driver.
> > >
> > > Yes, do that.
> > 
> > Can I know why sub nodes can't be used? the relationship of parent and
> > child devices looks better with sub nodes.
> 
> That's pretty subjective. I don't think it looks better to have a clock
> node that is also a syscon with a reset child node as it is rather
> inconsistent.

I think it is multi function device syscon node. it should be like

mfd
{
	clock
	{
		...
	}

	reset
	{
		...
	}
}

clock and reset are difference device node with totally difference's
compatible string.

> > 
> > A further question is can I use the reset-ti-syscon? which is a generic reset
> > device for SoCs.  with it I don't even need to write a new reset device driver.
> > it is more simple.
> 
> That is for a TI SoC. You're working on an imx. I don't think that you
> should be using that...

I think this statement violate the linux basic reuse prinicple. If the
code logic are the same why need duplicate it just because it is difference
company. Of coures, if it is generic enough, it'd better to add a more
generic compatible string.

Frank

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3700 bytes --]

On Wed, May 15, 2024 at 02:28:51PM -0400, Frank Li wrote:
> On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote:
> > On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote:
> > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > @@ -15,7 +15,10 @@ description: |
> > > > > >
> > > > > >  properties:
> > > > > >    compatible:
> > > > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > > > +    items:
> > > > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > > > +      - const: syscon
> > > > > > +      - const: simple-mfd
> > > > > >
> > > > > >    reg:
> > > > > >      maxItems: 1
> > > > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > > > +    type: object
> > > > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > > > +    description: The child reset devices of AudioMIX Block Control.
> > > > >
> > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > > > already suggested to you to do that and use auxdev to set up the reset
> > > > > driver.
> > > >
> > > > Yes, do that.
> > > 
> > > Can I know why sub nodes can't be used? the relationship of parent and
> > > child devices looks better with sub nodes.
> > 
> > That's pretty subjective. I don't think it looks better to have a clock
> > node that is also a syscon with a reset child node as it is rather
> > inconsistent.
> 
> I think it is multi function device syscon node. it should be like
> 
> mfd
> {
> 	clock
> 	{
> 		...
> 	}
> 
> 	reset
> 	{
> 		...
> 	}
> }
> 
> clock and reset are difference device node with totally difference's
> compatible string.

Which is I suspect is gonna require a change to your clock driver,
because the range in the existing clock nodes:
	audio_blk_ctrl: clock-controller@30e20000 {
		compatible = "fsl,imx8mp-audio-blk-ctrl";
		reg = <0x30e20000 0x10000>;
	};
would then have to move to the mfd parent node, and your clock child
would have a reg property that overlaps the reset region. You'd need to
then define a new binding that splits the range in two - obviously
doable, but significantly more work and more disruptive than using an
auxdev.

> > > A further question is can I use the reset-ti-syscon? which is a generic reset
> > > device for SoCs.  with it I don't even need to write a new reset device driver.
> > > it is more simple.
> > 
> > That is for a TI SoC. You're working on an imx. I don't think that you
> > should be using that...
> 
> I think this statement violate the linux basic reuse prinicple. If the
> code logic are the same why need duplicate it just because it is difference
> company. Of coures, if it is generic enough, it'd better to add a more
> generic compatible string.

That's true, but I suspect it only works because only through (ab)use
of the ti,reset-bits property not because you're actually compatible
with TI's reset hardware.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Stephen Boyd]

Quoting Shengjiu Wang (2024-05-14 19:47:57)
> On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > @@ -15,7 +15,10 @@ description: |
> > > >
> > > >  properties:
> > > >    compatible:
> > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > +    items:
> > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > +      - const: syscon
> > > > +      - const: simple-mfd
> > > >
> > > >    reg:
> > > >      maxItems: 1
> > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > +    type: object
> > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > +    description: The child reset devices of AudioMIX Block Control.
> > >
> > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > already suggested to you to do that and use auxdev to set up the reset
> > > driver.
> >
> > Yes, do that.
> 
> Can I know why sub nodes can't be used? the relationship of parent and
> child devices looks better with sub nodes.

Don't make nodes for drivers. Only make nodes for devices. This device
is multi-function, but is still only a single device.

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Frank Li]

On Thu, May 16, 2024 at 06:15:18PM +0100, Conor Dooley wrote:
> On Wed, May 15, 2024 at 02:28:51PM -0400, Frank Li wrote:
> > On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote:
> > > On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote:
> > > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > > Quoting Conor Dooley (2024-05-14 11:06:14)
> > > > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote:
> > > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > > index 0a6dc1a6e122..a403ace4d11f 100644
> > > > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml
> > > > > > > @@ -15,7 +15,10 @@ description: |
> > > > > > >
> > > > > > >  properties:
> > > > > > >    compatible:
> > > > > > > -    const: fsl,imx8mp-audio-blk-ctrl
> > > > > > > +    items:
> > > > > > > +      - const: fsl,imx8mp-audio-blk-ctrl
> > > > > > > +      - const: syscon
> > > > > > > +      - const: simple-mfd
> > > > > > >
> > > > > > >    reg:
> > > > > > >      maxItems: 1
> > > > > > > @@ -44,6 +47,11 @@ 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-controller:
> > > > > > > +    type: object
> > > > > > > +    $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml#
> > > > > > > +    description: The child reset devices of AudioMIX Block Control.
> > > > > >
> > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was
> > > > > > already suggested to you to do that and use auxdev to set up the reset
> > > > > > driver.
> > > > >
> > > > > Yes, do that.
> > > > 
> > > > Can I know why sub nodes can't be used? the relationship of parent and
> > > > child devices looks better with sub nodes.
> > > 
> > > That's pretty subjective. I don't think it looks better to have a clock
> > > node that is also a syscon with a reset child node as it is rather
> > > inconsistent.
> > 
> > I think it is multi function device syscon node. it should be like
> > 
> > mfd
> > {
> > 	clock
> > 	{
> > 		...
> > 	}
> > 
> > 	reset
> > 	{
> > 		...
> > 	}
> > }
> > 
> > clock and reset are difference device node with totally difference's
> > compatible string.
> 
> Which is I suspect is gonna require a change to your clock driver,
> because the range in the existing clock nodes:
> 	audio_blk_ctrl: clock-controller@30e20000 {
> 		compatible = "fsl,imx8mp-audio-blk-ctrl";
> 		reg = <0x30e20000 0x10000>;
> 	};
> would then have to move to the mfd parent node, and your clock child
> would have a reg property that overlaps the reset region. You'd need to
> then define a new binding that splits the range in two - obviously
> doable, but significantly more work and more disruptive than using an
> auxdev.

I am new for auxdev.

according to doc: https://docs.kernel.org/driver-api/auxiliary_bus.html

"key requirement for utilizing the auxiliary bus is that there is no 
dependency on a physical bus, device, register accesses or regmap support. 
These individual devices split from the core cannot live on the platform 
bus as they are not physical devices that are controlled by DT/ACPI."
                ^^^^                                        ^^^

Look like it is easy to register auxdev "reset" devices. But I have a
problem. How to use it by DT phandle?  "reset" devices is service provider.
Some client will use it.

Generally, reset node will used by other devices nodes. like

ABC: reset {
	compatible="simple-reset";
	...
}

other node will use "reset = <&ABC 0>".  If use auxdev, how to get &ABC
in dts file.


> 
> > > > A further question is can I use the reset-ti-syscon? which is a generic reset
> > > > device for SoCs.  with it I don't even need to write a new reset device driver.
> > > > it is more simple.
> > > 
> > > That is for a TI SoC. You're working on an imx. I don't think that you
> > > should be using that...
> > 
> > I think this statement violate the linux basic reuse prinicple. If the
> > code logic are the same why need duplicate it just because it is difference
> > company. Of coures, if it is generic enough, it'd better to add a more
> > generic compatible string.
> 
> That's true, but I suspect it only works because only through (ab)use
> of the ti,reset-bits property not because you're actually compatible
> with TI's reset hardware.

Reset's implement is very simple. Most design is similar in difference
SOC. Just toggle a register bit. If regiser layout is the same, it should
be compatible. this ti driver is suitable for most case. I think call it
as simple-reset-syscon are more reasonable.

> 
> Cheers,
> Conor.

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote:

> Look like it is easy to register auxdev "reset" devices. But I have a
> problem. How to use it by DT phandle?  "reset" devices is service provider.
> Some client will use it.
> 
> Generally, reset node will used by other devices nodes. like
> 
> ABC: reset {
> 	compatible="simple-reset";
> 	...
> }
> 
> other node will use "reset = <&ABC 0>".  If use auxdev, how to get &ABC
> in dts file.

Whether or not you use auxdev or any other method etc, does not matter
in a DT system, the consumer will always have a phandle to the provider
node:

ABC: whatever {
	compatible = "whatever";
	#clock-cells = <...>;
	#reset-cells = <...>;
}

something-else {
	clocks = <&ABC ...>;
	resets = <&ABC ...>;
}

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Frank Li]

On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote:
> On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote:
> 
> > Look like it is easy to register auxdev "reset" devices. But I have a
> > problem. How to use it by DT phandle?  "reset" devices is service provider.
> > Some client will use it.
> > 
> > Generally, reset node will used by other devices nodes. like
> > 
> > ABC: reset {
> > 	compatible="simple-reset";
> > 	...
> > }
> > 
> > other node will use "reset = <&ABC 0>".  If use auxdev, how to get &ABC
> > in dts file.
> 
> Whether or not you use auxdev or any other method etc, does not matter
> in a DT system, the consumer will always have a phandle to the provider
> node:
> 
> ABC: whatever {
> 	compatible = "whatever";
> 	#clock-cells = <...>;
> 	#reset-cells = <...>;
> }
> 
> something-else {
> 	clocks = <&ABC ...>;
> 	resets = <&ABC ...>;
> }


It goes back to old problem, "reset-cells" will be in "clock-controller".

clock-controller@30e20000 {
        compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
        reg = <0x30e20000 0x10000>;
	...
	
	#reset-cells = <...>;
	^^^
    };

If create new "whatever" auxdev bus driver which included two aux devices, 
(clock and reset). 

it will be similar with mfd. Still need change
clock-controller@30e20000 drivers.

"Which is I suspect is gonna require a change to your clock driver,
because the range in the existing clock nodes:
	audio_blk_ctrl: clock-controller@30e20000 {
		compatible = "fsl,imx8mp-audio-blk-ctrl";
		reg = <0x30e20000 0x10000>;
	};
would then have to move to the mfd parent node, and your clock child
would have a reg property that overlaps the reset region. You'd need to
then define a new binding that splits the range in two - obviously
doable, but significantly more work and more disruptive than using an
auxdev."

So I don't know why auxdev will be better than mfd.

A possible benefit may be that Auxdev needn't binding doc for clock and
reset node devices.

Frank




Frank

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

On Fri, May 17, 2024 at 01:10:05PM -0400, Frank Li wrote:
> On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote:
> > On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote:
> > 
> > > Look like it is easy to register auxdev "reset" devices. But I have a
> > > problem. How to use it by DT phandle?  "reset" devices is service provider.
> > > Some client will use it.
> > > 
> > > Generally, reset node will used by other devices nodes. like
> > > 
> > > ABC: reset {
> > > 	compatible="simple-reset";
> > > 	...
> > > }
> > > 
> > > other node will use "reset = <&ABC 0>".  If use auxdev, how to get &ABC
> > > in dts file.
> > 
> > Whether or not you use auxdev or any other method etc, does not matter
> > in a DT system, the consumer will always have a phandle to the provider
> > node:
> > 
> > ABC: whatever {
> > 	compatible = "whatever";
> > 	#clock-cells = <...>;
> > 	#reset-cells = <...>;
> > }
> > 
> > something-else {
> > 	clocks = <&ABC ...>;
> > 	resets = <&ABC ...>;
> > }
> 
> 
> It goes back to old problem, "reset-cells" will be in "clock-controller".
> 
> clock-controller@30e20000 {
>         compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
>         reg = <0x30e20000 0x10000>;
> 	...
> 	
> 	#reset-cells = <...>;
> 	^^^
>     };
> 
> If create new "whatever" auxdev bus driver which included two aux devices, 
> (clock and reset). 
> 
> it will be similar with mfd. Still need change
> clock-controller@30e20000 drivers.
> 
> "Which is I suspect is gonna require a change to your clock driver,
> because the range in the existing clock nodes:
> 	audio_blk_ctrl: clock-controller@30e20000 {
> 		compatible = "fsl,imx8mp-audio-blk-ctrl";
> 		reg = <0x30e20000 0x10000>;
> 	};
> would then have to move to the mfd parent node, and your clock child
> would have a reg property that overlaps the reset region. You'd need to
> then define a new binding that splits the range in two - obviously
> doable, but significantly more work and more disruptive than using an
> auxdev."
> 
> So I don't know why auxdev will be better than mfd.

I think Stephen and I have spent enough time trying to explain why using
auxdev is beneficial here. I, at least, won't be wasting any more of my
(metaphorical) breath.

> A possible benefit may be that Auxdev needn't binding doc for clock and
> reset node devices.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/6] dt-bindings: clock: imx8mp: Add reset-controller sub-node

[Frank Li]

On Fri, May 17, 2024 at 06:13:21PM +0100, Conor Dooley wrote:
> On Fri, May 17, 2024 at 01:10:05PM -0400, Frank Li wrote:
> > On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote:
> > > On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote:
> > > 
> > > > Look like it is easy to register auxdev "reset" devices. But I have a
> > > > problem. How to use it by DT phandle?  "reset" devices is service provider.
> > > > Some client will use it.
> > > > 
> > > > Generally, reset node will used by other devices nodes. like
> > > > 
> > > > ABC: reset {
> > > > 	compatible="simple-reset";
> > > > 	...
> > > > }
> > > > 
> > > > other node will use "reset = <&ABC 0>".  If use auxdev, how to get &ABC
> > > > in dts file.
> > > 
> > > Whether or not you use auxdev or any other method etc, does not matter
> > > in a DT system, the consumer will always have a phandle to the provider
> > > node:
> > > 
> > > ABC: whatever {
> > > 	compatible = "whatever";
> > > 	#clock-cells = <...>;
> > > 	#reset-cells = <...>;
> > > }
> > > 
> > > something-else {
> > > 	clocks = <&ABC ...>;
> > > 	resets = <&ABC ...>;
> > > }
> > 
> > 
> > It goes back to old problem, "reset-cells" will be in "clock-controller".
> > 
> > clock-controller@30e20000 {
> >         compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd";
> >         reg = <0x30e20000 0x10000>;
> > 	...
> > 	
> > 	#reset-cells = <...>;
> > 	^^^
> >     };
> > 
> > If create new "whatever" auxdev bus driver which included two aux devices, 
> > (clock and reset). 
> > 
> > it will be similar with mfd. Still need change
> > clock-controller@30e20000 drivers.
> > 
> > "Which is I suspect is gonna require a change to your clock driver,
> > because the range in the existing clock nodes:
> > 	audio_blk_ctrl: clock-controller@30e20000 {
> > 		compatible = "fsl,imx8mp-audio-blk-ctrl";
> > 		reg = <0x30e20000 0x10000>;
> > 	};
> > would then have to move to the mfd parent node, and your clock child
> > would have a reg property that overlaps the reset region. You'd need to
> > then define a new binding that splits the range in two - obviously
> > doable, but significantly more work and more disruptive than using an
> > auxdev."
> > 
> > So I don't know why auxdev will be better than mfd.
> 
> I think Stephen and I have spent enough time trying to explain why using
> auxdev is beneficial here. I, at least, won't be wasting any more of my
> (metaphorical) breath.

Thanks for your time. After I did more rearch, especially read your
previous patch:
https://lore.kernel.org/linux-clk/20240424-strangle-sharpener-34755c5e6e3e@spud/

I have better view about auxdev.

Frank

> 
> > A possible benefit may be that Auxdev needn't binding doc for clock and
> > reset node devices.
>