[PATCH v2 0/4] clk: imx: clk-audiomix: Improvement for audiomix

[PATCH v2 0/4] 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 v2:
- add more info in commit messages.

Shengjiu Wang (4):
  dt-bindings: clock: imx8mp: Add #reset-cells property
  clk: imx: clk-audiomix: Add reset controller
  clk: imx: clk-audiomix: Add CLK_SET_RATE_PARENT flags for clocks
  clk: imx: clk-audiomix: Corrent parent clock for earc_phy and audpll

 .../bindings/clock/imx8mp-audiomix.yaml       |   3 +
 drivers/clk/imx/clk-imx8mp-audiomix.c         | 105 +++++++++++++++++-
 2 files changed, 102 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] 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>
---
 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

Re: [PATCH v2 1/4] dt-bindings: clock: imx8mp: Add #reset-cells property

[Krzysztof Kozlowski]

On 09/05/2024 03:43, 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 make Audio Block Control a reset provider for EARC, which
> is one of modules in this audio subsystem.
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 2/4] 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 EARC PHY software reset and EARC controller software
reset need to be supported.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/clk/imx/clk-imx8mp-audiomix.c | 82 +++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mp-audiomix.c b/drivers/clk/imx/clk-imx8mp-audiomix.c
index b381d6f784c8..e84365cef71b 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset-controller.h>
 
 #include <dt-bindings/clock/imx8mp-clock.h>
 
@@ -35,6 +36,8 @@
 #define SAI_PLL_MNIT_CTL	0x410
 #define IPG_LP_CTRL		0x504
 
+#define EARC_RESET_MASK		0x3
+
 #define SAIn_MCLK1_PARENT(n)						\
 static const struct clk_parent_data					\
 clk_imx8mp_audiomix_sai##n##_mclk1_parents[] = {			\
@@ -212,6 +215,7 @@ static const u16 audiomix_regs[] = {
 struct clk_imx8mp_audiomix_priv {
 	void __iomem *base;
 	u32 regs_save[ARRAY_SIZE(audiomix_regs)];
+	struct reset_controller_dev rcdev;
 
 	/* Must be last */
 	struct clk_hw_onecell_data clk_data;
@@ -232,6 +236,80 @@ static void clk_imx8mp_audiomix_save_restore(struct device *dev, bool save)
 	}
 }
 
+static int clk_imx8mp_audiomix_reset_set(struct reset_controller_dev *rcdev,
+					 unsigned long id, bool assert)
+{
+	struct clk_imx8mp_audiomix_priv *drvdata = container_of(rcdev,
+				struct clk_imx8mp_audiomix_priv, rcdev);
+	unsigned int mask = BIT(id);
+	u32 reg;
+
+	pm_runtime_get_sync(rcdev->dev);
+
+	/* bit = 0 reset, bit = 1 unreset */
+	reg = readl(drvdata->base + EARC);
+	if (assert)
+		writel(reg & ~mask, drvdata->base + EARC);
+	else
+		writel(reg | mask, drvdata->base + EARC);
+
+	pm_runtime_put_sync(rcdev->dev);
+
+	return 0;
+}
+
+static int clk_imx8mp_audiomix_reset_reset(struct reset_controller_dev *rcdev,
+					   unsigned long id)
+{
+	clk_imx8mp_audiomix_reset_set(rcdev, id, true);
+
+	return clk_imx8mp_audiomix_reset_set(rcdev, id, false);
+}
+
+static int clk_imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev,
+					    unsigned long id)
+{
+	return clk_imx8mp_audiomix_reset_set(rcdev, id, true);
+}
+
+static int clk_imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev,
+					      unsigned long id)
+{
+	return clk_imx8mp_audiomix_reset_set(rcdev, id, false);
+}
+
+static int clk_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 clk_imx8mp_audiomix_reset_ops = {
+	.reset		= clk_imx8mp_audiomix_reset_reset,
+	.assert		= clk_imx8mp_audiomix_reset_assert,
+	.deassert	= clk_imx8mp_audiomix_reset_deassert,
+};
+
+static int clk_imx8mp_audiomix_register_reset_controller(struct device *dev)
+{
+	struct clk_imx8mp_audiomix_priv *drvdata = dev_get_drvdata(dev);
+
+	drvdata->rcdev.owner     = THIS_MODULE;
+	drvdata->rcdev.nr_resets = fls(EARC_RESET_MASK);
+	drvdata->rcdev.ops       = &clk_imx8mp_audiomix_reset_ops;
+	drvdata->rcdev.of_node   = dev->of_node;
+	drvdata->rcdev.dev	 = dev;
+	drvdata->rcdev.of_reset_n_cells = 1;
+	drvdata->rcdev.of_xlate  = clk_imx8mp_audiomix_reset_xlate;
+
+	return devm_reset_controller_register(dev, &drvdata->rcdev);
+}
+
 static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 {
 	struct clk_imx8mp_audiomix_priv *priv;
@@ -337,6 +415,10 @@ static int clk_imx8mp_audiomix_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk_register;
 
+	ret = clk_imx8mp_audiomix_register_reset_controller(dev);
+	if (ret)
+		goto err_clk_register;
+
 	pm_runtime_put_sync(dev);
 	return 0;
 
-- 
2.34.1

Re: [PATCH v2 2/4] clk: imx: clk-audiomix: Add reset controller

[Stephen Boyd]

Quoting Shengjiu Wang (2024-05-08 18:43:56)
> 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.

Can you move this to drivers/reset and use auxiliary device APIs to do
that? The idea would be to have reset maintainers review reset code.

Re: [PATCH v2 2/4] clk: imx: clk-audiomix: Add reset controller

[Shengjiu Wang]

On Fri, May 10, 2024 at 6:04 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Shengjiu Wang (2024-05-08 18:43:56)
> > 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.
>
> Can you move this to drivers/reset and use auxiliary device APIs to do
> that? The idea would be to have reset maintainers review reset code.

Thanks for your comments.

This is a minor reset control only for XCVR devices, two reset bits
are accessed.

If we move to an auxiliary device,  we need to define a new header file
and a new driver, which will bring more code size and complexity.

So is it necessary to separate it to another auxiliary driver/device?

And add Philipp Zabel in loop for review.

Best Regards
Shengjiu Wang

Re: [PATCH v2 2/4] clk: imx: clk-audiomix: Add reset controller

[Shengjiu Wang]

On Fri, May 10, 2024 at 12:02 PM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Fri, May 10, 2024 at 6:04 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Shengjiu Wang (2024-05-08 18:43:56)
> > > 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.
> >
> > Can you move this to drivers/reset and use auxiliary device APIs to do
> > that? The idea would be to have reset maintainers review reset code.
>
> Thanks for your comments.
>
> This is a minor reset control only for XCVR devices, two reset bits
> are accessed.
>
> If we move to an auxiliary device,  we need to define a new header file
> and a new driver, which will bring more code size and complexity.
>
> So is it necessary to separate it to another auxiliary driver/device?
>
> And add Philipp Zabel in loop for review.
>

I will use syscon and simple-mfd to separate the reset function to
a new driver,  which will be a child node of the audiomix device.

Best regards
Shengjiu Wang

[PATCH v2 3/4] 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 e84365cef71b..6ed44cd5001b 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -347,12 +347,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);
@@ -395,7 +395,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);
@@ -404,7 +405,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 v2 4/4] 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 6ed44cd5001b..11a29bd46a9e 100644
--- a/drivers/clk/imx/clk-imx8mp-audiomix.c
+++ b/drivers/clk/imx/clk-imx8mp-audiomix.c
@@ -157,6 +157,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;
@@ -174,14 +183,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