Skip to contentSkip to conversation

ASoC: mxs-saif: add mclk enable/disable ops

PATCH In review Mans Rullgard 3 participants 3 messages Dec 22, 2016 – Jan 23, 2017

Subsystems (3)
Sound Arm i.MX and Layerscape ASoC
lore
Share & tools
lore

Mans Rullgard

author
#0 lore raw

PATCH v2 1/1 ASoC: mxs-saif: add mclk enable/disable ops

This makes normal clk_enable/disable() calls on mxs_saif_mclk work as expected, i.e. actually turn the mclk output on or (when safe) off. The existing mxs_saif_get/put_mclk() functions are rewritten to use common clk operations on mxs_saif_mclk rather than accessing registers directly.

With these changes mxs-saif can be used together with the simple-card driver.

Signed-off-by Mans Rullgard <[email protected]>

---
v2: add #include <linux/clk-provider.h> needed by some configs

---
 sound/soc/mxs/mxs-saif.c | 144 +++++++++++++++++++++++++++++++----------------
 sound/soc/mxs/mxs-saif.h |   3 +
 2 files changed, 99 insertions(+), 48 deletions(-)
sound/soc/mxs/mxs-saif.c +96 -48
index a002ab892772..7c620399f96b 100644
@@ -204,27 +204,15 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
  */
 int mxs_saif_put_mclk(unsigned int saif_id)
 {
-	struct mxs_saif *saif = mxs_saif[saif_id];
-	u32 stat;
+	struct clk *clk;
 
-	if (!saif)
-		return -EINVAL;
+	clk = clk_get(NULL, "mxs_saif_mclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
-	stat = __raw_readl(saif->base + SAIF_STAT);
-	if (stat & BM_SAIF_STAT_BUSY) {
-		dev_err(saif->dev, "error: busy\n");
-		return -EBUSY;
-	}
+	clk_disable_unprepare(clk);
+	clk_put(clk);
 
-	clk_disable_unprepare(saif->clk);
-
-	/* disable MCLK output */
-	__raw_writel(BM_SAIF_CTRL_CLKGATE,
-		saif->base + SAIF_CTRL + MXS_SET_ADDR);
-	__raw_writel(BM_SAIF_CTRL_RUN,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-
-	saif->mclk_in_use = 0;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mxs_saif_put_mclk);

@@ -239,47 +227,33 @@ int mxs_saif_get_mclk(unsigned int saif_id, unsigned int mclk,
 					unsigned int rate)
 {
 	struct mxs_saif *saif = mxs_saif[saif_id];
-	u32 stat;
 	int ret;
 	struct mxs_saif *master_saif;
+	struct clk *clk;
 
 	if (!saif)
 		return -EINVAL;
 
-	/* Clear Reset */
-	__raw_writel(BM_SAIF_CTRL_SFTRST,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-
-	/* FIXME: need clear clk gate for register r/w */
-	__raw_writel(BM_SAIF_CTRL_CLKGATE,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-
 	master_saif = mxs_saif_get_master(saif);
 	if (saif != master_saif) {
 		dev_err(saif->dev, "can not get mclk from a non-master saif\n");
 		return -EINVAL;
 	}
 
-	stat = __raw_readl(saif->base + SAIF_STAT);
-	if (stat & BM_SAIF_STAT_BUSY) {
-		dev_err(saif->dev, "error: busy\n");
-		return -EBUSY;
-	}
+	clk = clk_get(NULL, "mxs_saif_mclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		goto out;
 
-	saif->mclk_in_use = 1;
 	ret = mxs_saif_set_clk(saif, mclk, rate);
-	if (ret)
-		return ret;
 
-	ret = clk_prepare_enable(saif->clk);
-	if (ret)
-		return ret;
+out:
+	clk_put(clk);
 
-	/* enable MCLK output */
-	__raw_writel(BM_SAIF_CTRL_RUN,
-		saif->base + SAIF_CTRL + MXS_SET_ADDR);
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(mxs_saif_get_mclk);
 

@@ -687,18 +661,92 @@ static irqreturn_t mxs_saif_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+#define to_mxs_saif(c) container_of(c, struct mxs_saif, div_clk.hw)
+
+static int mxs_saif_mclk_enable(struct clk_hw *hw)
+{
+	struct mxs_saif *saif = to_mxs_saif(hw);
+
+	/* Clear Reset */
+	__raw_writel(BM_SAIF_CTRL_SFTRST,
+		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	/* Clear clk gate */
+	__raw_writel(BM_SAIF_CTRL_CLKGATE,
+		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	/* enable MCLK output */
+	__raw_writel(BM_SAIF_CTRL_RUN,
+		saif->base + SAIF_CTRL + MXS_SET_ADDR);
+
+	saif->mclk_in_use = 1;
+
+	return 0;
+}
+
+static void mxs_saif_mclk_disable(struct clk_hw *hw)
+{
+	struct mxs_saif *saif = to_mxs_saif(hw);
+
+	if (!saif->ongoing)
+		__raw_writel(BM_SAIF_CTRL_RUN,
+			     saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	saif->mclk_in_use = 0;
+}
+
+static unsigned long mxs_saif_mclk_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	return clk_divider_ops.recalc_rate(hw, parent_rate);
+}
+
+static long mxs_saif_mclk_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *parent_rate)
+{
+	return clk_divider_ops.round_rate(hw, rate, parent_rate);
+}
+
+static int mxs_saif_mclk_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long parent_rate)
+{
+	return clk_divider_ops.set_rate(hw, rate, parent_rate);
+}
+
+static const struct clk_ops mxs_saif_mclk_ops = {
+	.enable		= mxs_saif_mclk_enable,
+	.disable	= mxs_saif_mclk_disable,
+	.recalc_rate	= mxs_saif_mclk_recalc_rate,
+	.round_rate	= mxs_saif_mclk_round_rate,
+	.set_rate	= mxs_saif_mclk_set_rate,
+};
+
 static int mxs_saif_mclk_init(struct platform_device *pdev)
 {
 	struct mxs_saif *saif = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
+	struct clk_init_data init;
+	struct clk_divider *div;
 	struct clk *clk;
+	const char *parent_name;
 	int ret;
 
-	clk = clk_register_divider(&pdev->dev, "mxs_saif_mclk",
-				   __clk_get_name(saif->clk), 0,
-				   saif->base + SAIF_CTRL,
-				   BP_SAIF_CTRL_BITCLK_MULT_RATE, 3,
-				   0, NULL);
+	parent_name = __clk_get_name(saif->clk);
+
+	init.name = "mxs_saif_mclk";
+	init.ops = &mxs_saif_mclk_ops;
+	init.flags = CLK_GET_RATE_NOCACHE | CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	div = &saif->div_clk;
+	div->reg = saif->base + SAIF_CTRL;
+	div->shift = BP_SAIF_CTRL_BITCLK_MULT_RATE;
+	div->width = 3;
+	div->flags = CLK_DIVIDER_POWER_OF_TWO;
+	div->hw.init = &init;
+
+	clk = clk_register(&pdev->dev, &div->hw);
 	if (IS_ERR(clk)) {
 		ret = PTR_ERR(clk);
 		if (ret == -EEXIST)
sound/soc/mxs/mxs-saif.h +3 -0
index 9a4c0b291b9e..e1bb5cb00ec0 100644
@@ -108,6 +108,7 @@
 
 #define MXS_SAIF_MCLK		0
 
+#include <linux/clk-provider.h>
 #include "mxs-pcm.h"
 
 struct mxs_saif {

@@ -128,6 +129,8 @@ struct mxs_saif {
 		MXS_SAIF_STATE_STOPPED,
 		MXS_SAIF_STATE_RUNNING,
 	} state;
+
+	struct clk_divider div_clk;
 };
 
 extern int mxs_saif_put_mclk(unsigned int saif_id);
Signature · 1 line
2.11.0
#1 lore raw

Hi Mans,

Quoted 9 lines · Mans Rullgard

This makes normal clk_enable/disable() calls on mxs_saif_mclk work as expected, i.e. actually turn the mclk output on or (when safe) off. The existing mxs_saif_get/put_mclk() functions are rewritten to use common clk operations on mxs_saif_mclk rather than accessing registers
directly.

With these changes mxs-saif can be used together with the simple-card driver.

I can confirm that this works for me using the pcm5102a, wm8524 and the wm8731 codec driver. But I was able only to test playback on saif0 for those drivers as I failed to setup the simple-card device tree node both for saif0 and saif1.

However, when using this patch with a platform driver, e.g. mxs-wm8524, instead of the simple-card device tree node configuration, it fails for me. Note, that the platform driver mxs-wm8524 is just a mere copy of the mxs-sgtl5000 driver. The error happens when probing the platform driver at calling mxs_saif_get_mclk(): "failed to get mclk".

As I am testing on a custom board not mainlined yet it is possible that I did something wrong. So maybe someone having access to a mxs-board with a sgtl5000 codec can verify this?

Jörg

Quoted 231 lines
Signed-off-by Mans Rullgard <[email protected]>

---
v2: add #include <linux/clk-provider.h> needed by some configs
---
 sound/soc/mxs/mxs-saif.c | 144 +++++++++++++++++++++++++++++++----
------------
 sound/soc/mxs/mxs-saif.h |   3 +

 2 files changed, 99 insertions(+), 48 deletions(-)
sound/soc/mxs/mxs-saif.c +1 -2
index a002ab892772..7c620399f96b 100644
@@ -204,27 +204,15 @@ static int mxs_saif_set_clk(struct mxs_saif
 *saif,
   */
  int mxs_saif_put_mclk(unsigned int saif_id)
  {
-	struct mxs_saif *saif = mxs_saif[saif_id];
-	u32 stat;
+	struct clk *clk;

sound/soc/mxs/mxs-saif.c +3 -2
-	if (!saif)
-		return -EINVAL;
+	clk = clk_get(NULL, "mxs_saif_mclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);

sound/soc/mxs/mxs-saif.c +2 -5
-	stat = __raw_readl(saif->base + SAIF_STAT);
-	if (stat & BM_SAIF_STAT_BUSY) {
-		dev_err(saif->dev, "error: busy\n");
-		return -EBUSY;
-	}
+	clk_disable_unprepare(clk);
+	clk_put(clk);

sound/soc/mxs/mxs-saif.c +0 -9
-	clk_disable_unprepare(saif->clk);
-
-	/* disable MCLK output */
-	__raw_writel(BM_SAIF_CTRL_CLKGATE,
-		saif->base + SAIF_CTRL + MXS_SET_ADDR);
-	__raw_writel(BM_SAIF_CTRL_RUN,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-
-	saif->mclk_in_use = 0;

  return 0;
 }
 EXPORT_SYMBOL_GPL(mxs_saif_put_mclk);

sound/soc/mxs/mxs-saif.c +1 -1
@@ -239,47 +227,33 @@ int mxs_saif_get_mclk(unsigned int saif_id,
 unsigned int mclk,
  					unsigned int rate)
  {
  	struct mxs_saif *saif = mxs_saif[saif_id];
-	u32 stat;
  	int ret;
  	struct mxs_saif *master_saif;
+	struct clk *clk;


  if (!saif)
  return -EINVAL;

sound/soc/mxs/mxs-saif.c +0 -8
-	/* Clear Reset */
-	__raw_writel(BM_SAIF_CTRL_SFTRST,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-
-	/* FIXME: need clear clk gate for register r/w */
-	__raw_writel(BM_SAIF_CTRL_CLKGATE,
-		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
-

  master_saif = mxs_saif_get_master(saif);
  if (saif != master_saif) {
  dev_err(saif->dev, "can not get mclk from a non- master saif\n");
  return -EINVAL;
  }

sound/soc/mxs/mxs-saif.c +7 -5
-	stat = __raw_readl(saif->base + SAIF_STAT);
-	if (stat & BM_SAIF_STAT_BUSY) {
-		dev_err(saif->dev, "error: busy\n");
-		return -EBUSY;
-	}
+	clk = clk_get(NULL, "mxs_saif_mclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		goto out;

sound/soc/mxs/mxs-saif.c +0 -1
-	saif->mclk_in_use = 1;

  ret = mxs_saif_set_clk(saif, mclk, rate);

sound/soc/mxs/mxs-saif.c +0 -2
-	if (ret)
-		return ret;

sound/soc/mxs/mxs-saif.c +2 -3
-	ret = clk_prepare_enable(saif->clk);
-	if (ret)
-		return ret;
+out:
+	clk_put(clk);

sound/soc/mxs/mxs-saif.c +1 -5
-	/* enable MCLK output */
-	__raw_writel(BM_SAIF_CTRL_RUN,
-		saif->base + SAIF_CTRL + MXS_SET_ADDR);
-
-	return 0;
+	return ret;

 }
 EXPORT_SYMBOL_GPL(mxs_saif_get_mclk);

sound/soc/mxs/mxs-saif.c +0 -0
@@ -687,18 +661,92 @@ static irqreturn_t mxs_saif_irq(int irq, void
 *dev_id)
  	return IRQ_HANDLED;
  }

sound/soc/mxs/mxs-saif.c +36 -0
+#define to_mxs_saif(c) container_of(c, struct mxs_saif, div_clk.hw)
+
+static int mxs_saif_mclk_enable(struct clk_hw *hw)
+{
+	struct mxs_saif *saif = to_mxs_saif(hw);
+
+	/* Clear Reset */
+	__raw_writel(BM_SAIF_CTRL_SFTRST,
+		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	/* Clear clk gate */
+	__raw_writel(BM_SAIF_CTRL_CLKGATE,
+		saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	/* enable MCLK output */
+	__raw_writel(BM_SAIF_CTRL_RUN,
+		saif->base + SAIF_CTRL + MXS_SET_ADDR);
+
+	saif->mclk_in_use = 1;
+
+	return 0;
+}
+
+static void mxs_saif_mclk_disable(struct clk_hw *hw)
+{
+	struct mxs_saif *saif = to_mxs_saif(hw);
+
+	if (!saif->ongoing)
+		__raw_writel(BM_SAIF_CTRL_RUN,
+			     saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	saif->mclk_in_use = 0;
+}
+
+static unsigned long mxs_saif_mclk_recalc_rate(struct clk_hw *hw,
+					       unsigned long

parent_rate)

sound/soc/mxs/mxs-saif.c +5 -0
+{
+	return clk_divider_ops.recalc_rate(hw, parent_rate);
+}
+
+static long mxs_saif_mclk_round_rate(struct clk_hw *hw, unsigned

long rate,

sound/soc/mxs/mxs-saif.c +6 -0
+				     unsigned long *parent_rate)
+{
+	return clk_divider_ops.round_rate(hw, rate, parent_rate);
+}
+
+static int mxs_saif_mclk_set_rate(struct clk_hw *hw, unsigned long

rate,

sound/soc/mxs/mxs-saif.c +13 -0
+				  unsigned long parent_rate)
+{
+	return clk_divider_ops.set_rate(hw, rate, parent_rate);
+}
+
+static const struct clk_ops mxs_saif_mclk_ops = {
+	.enable		= mxs_saif_mclk_enable,
+	.disable	= mxs_saif_mclk_disable,
+	.recalc_rate	= mxs_saif_mclk_recalc_rate,
+	.round_rate	= mxs_saif_mclk_round_rate,
+	.set_rate	= mxs_saif_mclk_set_rate,
+};
+

 static int mxs_saif_mclk_init(struct platform_device *pdev) {
  struct mxs_saif *saif = platform_get_drvdata(pdev); struct device_node *np = pdev->dev.of_node;

sound/soc/mxs/mxs-saif.c +2 -0
+	struct clk_init_data init;
+	struct clk_divider *div;

  struct clk *clk;

sound/soc/mxs/mxs-saif.c +1 -0
+	const char *parent_name;

  int ret;

sound/soc/mxs/mxs-saif.c +16 -5
-	clk = clk_register_divider(&pdev->dev, "mxs_saif_mclk",
-				   __clk_get_name(saif->clk), 0,
-				   saif->base + SAIF_CTRL,
-				   BP_SAIF_CTRL_BITCLK_MULT_RATE, 3,
-				   0, NULL);
+	parent_name = __clk_get_name(saif->clk);
+
+	init.name = "mxs_saif_mclk";
+	init.ops = &mxs_saif_mclk_ops;
+	init.flags = CLK_GET_RATE_NOCACHE | CLK_IS_BASIC;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	div = &saif->div_clk;
+	div->reg = saif->base + SAIF_CTRL;
+	div->shift = BP_SAIF_CTRL_BITCLK_MULT_RATE;
+	div->width = 3;
+	div->flags = CLK_DIVIDER_POWER_OF_TWO;
+	div->hw.init = &init;
+
+	clk = clk_register(&pdev->dev, &div->hw);

  if (IS_ERR(clk)) {
  ret = PTR_ERR(clk);
  if (ret == -EEXIST)

sound/soc/mxs/mxs-saif.h +0 -0
index 9a4c0b291b9e..e1bb5cb00ec0 100644
@@ -108,6 +108,7 @@


 #define MXS_SAIF_MCLK 0

+#include <linux/clk-provider.h>
 #include "mxs-pcm.h"

 struct mxs_saif {

sound/soc/mxs/mxs-saif.h +2 -0
@@ -128,6 +129,8 @@ struct mxs_saif {
  		MXS_SAIF_STATE_STOPPED,
  		MXS_SAIF_STATE_RUNNING,
  	} state;
+
+	struct clk_divider div_clk;
  };
Quoted 2 lines


 extern int mxs_saif_put_mclk(unsigned int saif_id);

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

maintainer
#2 lore raw
In reply to Mans Rullgard

sound/soc/mxs/mxs-saif.c static int mxs_saif_set_clk(struct mxs_saif *saif, · line 211

Quoted 11 lines · Mans Rullgard

int mxs_saif_put_mclk(unsigned int saif_id)
{

sound/soc/mxs/mxs-saif.c +4 -4
-	struct mxs_saif *saif = mxs_saif[saif_id];
-	u32 stat;
+	struct clk *clk;
 
-	if (!saif)
-		return -EINVAL;
+	clk = clk_get(NULL, "mxs_saif_mclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);

So, this *is* an in place refactoring but it's only half done in that we don't have any followup patches that move the clk_get() to device probe where it should be.

sound/soc/mxs/mxs-saif.c static irqreturn_t mxs_saif_irq(int irq, void *dev_id) · line 696

Quoted 10 lines
sound/soc/mxs/mxs-saif.c +10 -0
+static void mxs_saif_mclk_disable(struct clk_hw *hw)
+{
+	struct mxs_saif *saif = to_mxs_saif(hw);
+
+	if (!saif->ongoing)
+		__raw_writel(BM_SAIF_CTRL_RUN,
+			     saif->base + SAIF_CTRL + MXS_CLR_ADDR);
+
+	saif->mclk_in_use = 0;
+}

We silently ignore disables if the clock is in use? That seems error prone. I'd expect us to at least warn in that case.

sound/soc/mxs/mxs-saif.c static irqreturn_t mxs_saif_irq(int irq, void *dev_id) · line 702

Quoted 5 lines
sound/soc/mxs/mxs-saif.c +5 -0
+static unsigned long mxs_saif_mclk_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	return clk_divider_ops.recalc_rate(hw, parent_rate);
+}

Can't we just assign these ops directly? Having to write wrapper functions like this looks like we're doing something wrong here.