[RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Sylwester Nawrocki]

This patch adds handling of the CODEC's external MCLK{1,2} clocks
needed for board configurations where these clocks are not always on
oscillators.
The 32k source MCLKn clock is basically kept permanently enabled while
the other MCLKn clock is being gated in the MFD's runtime suspend/resume
callbacks.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
I'm not sure it's a correct approach hence only an RFC patch.
---
 drivers/mfd/arizona-core.c       | 105 ++++++++++++++++++++++++++++++++-------
 include/linux/mfd/arizona/core.h |   9 ++++
 2 files changed, 96 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index e4f97b3..a733b61 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -10,6 +10,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
@@ -49,7 +50,15 @@ int arizona_clk32k_enable(struct arizona *arizona)
 		case ARIZONA_32KZ_MCLK1:
 			ret = pm_runtime_get_sync(arizona->dev);
 			if (ret != 0)
-				goto out;
+				goto err_ref;
+			ret = clk_prepare_enable(arizona->mclk[ARIZONA_MCLK1]);
+			if (ret != 0)
+				goto err_pm;
+			break;
+		case ARIZONA_32KZ_MCLK2:
+			ret = clk_prepare_enable(arizona->mclk[ARIZONA_MCLK2]);
+			if (ret != 0)
+				goto err_ref;
 			break;
 		}
 
@@ -58,7 +67,9 @@ int arizona_clk32k_enable(struct arizona *arizona)
 					 ARIZONA_CLK_32K_ENA);
 	}
 
-out:
+err_pm:
+	pm_runtime_put_sync(arizona->dev);
+err_ref:
 	if (ret != 0)
 		arizona->clk32k_ref--;
 
@@ -83,6 +94,10 @@ int arizona_clk32k_disable(struct arizona *arizona)
 		switch (arizona->pdata.clk32k_src) {
 		case ARIZONA_32KZ_MCLK1:
 			pm_runtime_put_sync(arizona->dev);
+			clk_disable_unprepare(arizona->mclk[ARIZONA_MCLK1]);
+			break;
+		case ARIZONA_32KZ_MCLK2:
+			clk_disable_unprepare(arizona->mclk[ARIZONA_MCLK2]);
 			break;
 		}
 	}
@@ -93,6 +108,34 @@ int arizona_clk32k_disable(struct arizona *arizona)
 }
 EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
 
+/* Enable all external MCLKn clocks except the 32k source clock */
+int arizona_mclk_enable(struct arizona *arizona)
+{
+	int ret;
+
+	if (arizona->pdata.clk32k_src != ARIZONA_32KZ_MCLK1) {
+		ret = clk_prepare_enable(arizona->mclk[ARIZONA_MCLK1]);
+		if (ret != 0)
+			return ret;
+	}
+	if (arizona->pdata.clk32k_src != ARIZONA_32KZ_MCLK2) {
+		ret = clk_prepare_enable(arizona->mclk[ARIZONA_MCLK2]);
+		if (ret != 0)
+			return ret;
+	}
+	return 0;
+}
+
+/* Disable all external MCLKn clocks except the 32k source clock */
+void arizona_mclk_disable(struct arizona *arizona)
+{
+	if (arizona->pdata.clk32k_src != ARIZONA_32KZ_MCLK1)
+		clk_disable_unprepare(arizona->mclk[ARIZONA_MCLK1]);
+
+	if (arizona->pdata.clk32k_src != ARIZONA_32KZ_MCLK2)
+		clk_disable_unprepare(arizona->mclk[ARIZONA_MCLK2]);
+}
+
 static irqreturn_t arizona_clkgen_err(int irq, void *data)
 {
 	struct arizona *arizona = data;
@@ -533,6 +576,12 @@ static int arizona_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	ret = arizona_mclk_enable(arizona);
+	if (ret != 0) {
+		dev_err(dev, "Failed to enable mclk: %d\n", ret);
+		goto err_reg;
+	}
+
 	if (arizona->has_fully_powered_off) {
 		arizona_disable_reset(arizona);
 		enable_irq(arizona->irq);
@@ -546,14 +595,14 @@ static int arizona_runtime_resume(struct device *dev)
 		if (arizona->external_dcvdd) {
 			ret = arizona_connect_dcvdd(arizona);
 			if (ret != 0)
-				goto err;
+				goto err_clk;
 		}
 
 		ret = wm5102_patch(arizona);
 		if (ret != 0) {
 			dev_err(arizona->dev, "Failed to apply patch: %d\n",
 				ret);
-			goto err;
+			goto err_clk;
 		}
 
 		ret = wm5102_apply_hardware_patch(arizona);
@@ -561,19 +610,19 @@ static int arizona_runtime_resume(struct device *dev)
 			dev_err(arizona->dev,
 				"Failed to apply hardware patch: %d\n",
 				ret);
-			goto err;
+			goto err_clk;
 		}
 		break;
 	case WM5110:
 	case WM8280:
 		ret = arizona_wait_for_boot(arizona);
 		if (ret)
-			goto err;
+			goto err_clk;
 
 		if (arizona->external_dcvdd) {
 			ret = arizona_connect_dcvdd(arizona);
 			if (ret != 0)
-				goto err;
+				goto err_clk;
 		} else {
 			/*
 			 * As this is only called for the internal regulator
@@ -586,7 +635,7 @@ static int arizona_runtime_resume(struct device *dev)
 				dev_err(arizona->dev,
 					"Failed to set resume voltage: %d\n",
 					ret);
-				goto err;
+				goto err_clk;
 			}
 		}
 
@@ -595,24 +644,24 @@ static int arizona_runtime_resume(struct device *dev)
 			dev_err(arizona->dev,
 				"Failed to re-apply sleep patch: %d\n",
 				ret);
-			goto err;
+			goto err_clk;
 		}
 		break;
 	case WM1831:
 	case CS47L24:
 		ret = arizona_wait_for_boot(arizona);
 		if (ret != 0)
-			goto err;
+			goto err_clk;
 		break;
 	default:
 		ret = arizona_wait_for_boot(arizona);
 		if (ret != 0)
-			goto err;
+			goto err_clk;
 
 		if (arizona->external_dcvdd) {
 			ret = arizona_connect_dcvdd(arizona);
 			if (ret != 0)
-				goto err;
+				goto err_clk;
 		}
 		break;
 	}
@@ -620,12 +669,14 @@ static int arizona_runtime_resume(struct device *dev)
 	ret = regcache_sync(arizona->regmap);
 	if (ret != 0) {
 		dev_err(arizona->dev, "Failed to restore register cache\n");
-		goto err;
+		goto err_clk;
 	}
 
 	return 0;
 
-err:
+err_clk:
+	arizona_mclk_disable(arizona);
+err_reg:
 	regcache_cache_only(arizona->regmap, true);
 	regulator_disable(arizona->dcvdd);
 	return ret;
@@ -707,6 +758,7 @@ static int arizona_runtime_suspend(struct device *dev)
 	regcache_cache_only(arizona->regmap, true);
 	regcache_mark_dirty(arizona->regmap);
 	regulator_disable(arizona->dcvdd);
+	arizona_mclk_disable(arizona);
 
 	/* Allow us to completely power down if no jack detection */
 	if (!jd_active) {
@@ -1000,6 +1052,7 @@ static const struct mfd_cell wm8998_devs[] = {
 
 int arizona_dev_init(struct arizona *arizona)
 {
+	const char * const mclk_name[] = { "mclk1", "mclk2" };
 	struct device *dev = arizona->dev;
 	const char *type_name = NULL;
 	unsigned int reg, val, mask;
@@ -1016,6 +1069,15 @@ int arizona_dev_init(struct arizona *arizona)
 	else
 		arizona_of_get_core_pdata(arizona);
 
+	for (i = 0; i < ARRAY_SIZE(arizona->mclk); i++) {
+		arizona->mclk[i] = devm_clk_get(arizona->dev, mclk_name[i]);
+		if (IS_ERR(arizona->mclk[i])) {
+			dev_info(arizona->dev, "Failed to get mclk%d: %ld\n",
+				 i, PTR_ERR(arizona->mclk[i]));
+			arizona->mclk[i] = NULL;
+		}
+	}
+
 	regcache_cache_only(arizona->regmap, true);
 
 	switch (arizona->type) {
@@ -1101,6 +1163,16 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_enable;
 	}
 
+	/* Chip default */
+	if (!arizona->pdata.clk32k_src)
+		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
+
+	ret = arizona_mclk_enable(arizona);
+	if (ret != 0) {
+		dev_err(dev, "Failed to enable mclk: %d\n", ret);
+		goto err_enable;
+	}
+
 	arizona_disable_reset(arizona);
 
 	regcache_cache_only(arizona->regmap, false);
@@ -1332,10 +1404,6 @@ int arizona_dev_init(struct arizona *arizona)
 			     arizona->pdata.gpio_defaults[i]);
 	}
 
-	/* Chip default */
-	if (!arizona->pdata.clk32k_src)
-		arizona->pdata.clk32k_src = ARIZONA_32KZ_MCLK2;
-
 	switch (arizona->pdata.clk32k_src) {
 	case ARIZONA_32KZ_MCLK1:
 	case ARIZONA_32KZ_MCLK2:
@@ -1490,6 +1558,7 @@ err_pm:
 	pm_runtime_disable(arizona->dev);
 err_reset:
 	arizona_enable_reset(arizona);
+	arizona_mclk_disable(arizona);
 	regulator_disable(arizona->dcvdd);
 err_enable:
 	regulator_bulk_disable(arizona->num_core_supplies,
diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
index 58ab4c0..b9909bb 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -13,6 +13,7 @@
 #ifndef _WM_ARIZONA_CORE_H
 #define _WM_ARIZONA_CORE_H
 
+#include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/notifier.h>
 #include <linux/regmap.h>
@@ -21,6 +22,12 @@
 
 #define ARIZONA_MAX_CORE_SUPPLIES 2
 
+enum {
+	ARIZONA_MCLK1,
+	ARIZONA_MCLK2,
+	ARIZONA_NUM_MCLK
+};
+
 enum arizona_type {
 	WM5102 = 1,
 	WM5110 = 2,
@@ -139,6 +146,8 @@ struct arizona {
 	struct mutex clk_lock;
 	int clk32k_ref;
 
+	struct clk *mclk[ARIZONA_NUM_MCLK];
+
 	bool ctrlif_error;
 
 	struct snd_soc_dapm_context *dapm;
-- 
1.9.1

Re: [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Fri, Aug 19, 2016 at 07:17:16PM +0200, Sylwester Nawrocki wrote:
> This patch adds handling of the CODEC's external MCLK{1,2} clocks
> needed for board configurations where these clocks are not always on
> oscillators.
> The 32k source MCLKn clock is basically kept permanently enabled while
> the other MCLKn clock is being gated in the MFD's runtime suspend/resume
> callbacks.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> I'm not sure it's a correct approach hence only an RFC patch.
> ---

Yeah I am not sure this is quite the correct approach, there are
quite a few corner cases that would not be covered well here. For
example an internally divided down 32k in which case both the 32k
and MCLK would come from the same pin, or using the 32k to feed
an FLL in which case we are trying to enable MCLK1 unnecessarily.

I think we could request the 32k clock source from this part
of the code, but without implementing clock drivers for the
chips internal clocking I think the main MCLK would need to be
requested from the machine driver for now.

On that note, I have been working on a patch chain that adds an
actual clock driver for the chip unfortunately this has been
delayed somewhat due to issues interfacing SPI backed clocks to
the clock framework. Krzysztof Kozlowski has sent a series that
appears to resolve these issues for me so hopefully once the
clock guys have had a look at that I can send my clock driver.
My current implementation mostly just implements the 32k clock
but we can start building that out to include the SYSCLKs and
FLLs etc. fairly quickly. The best thing might be to wait for
that and then build additional features onto that.

Let me know if you want to get an early look at that code.

Thanks,
Charles

Re: [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Fri, Aug 19, 2016 at 07:17:16PM +0200, Sylwester Nawrocki wrote:
> This patch adds handling of the CODEC's external MCLK{1,2} clocks
> needed for board configurations where these clocks are not always on
> oscillators.
> The 32k source MCLKn clock is basically kept permanently enabled while
> the other MCLKn clock is being gated in the MFD's runtime suspend/resume
> callbacks.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> I'm not sure it's a correct approach hence only an RFC patch.
> ---

Yeah I am not sure this is quite the correct approach, there are
quite a few corner cases that would not be covered well here. For
example an internally divided down 32k in which case both the 32k
and MCLK would come from the same pin, or using the 32k to feed
an FLL in which case we are trying to enable MCLK1 unnecessarily.

I think we could request the 32k clock source from this part
of the code, but without implementing clock drivers for the
chips internal clocking I think the main MCLK would need to be
requested from the machine driver for now.

On that note, I have been working on a patch chain that adds an
actual clock driver for the chip unfortunately this has been
delayed somewhat due to issues interfacing SPI backed clocks to
the clock framework. Krzysztof Kozlowski has sent a series that
appears to resolve these issues for me so hopefully once the
clock guys have had a look at that I can send my clock driver.
My current implementation mostly just implements the 32k clock
but we can start building that out to include the SYSCLKs and
FLLs etc. fairly quickly. The best thing might be to wait for
that and then build additional features onto that.

Let me know if you want to get an early look at that code.

Thanks,
Charles

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Sylwester Nawrocki]

On 08/22/2016 11:22 AM, Charles Keepax wrote:
> Yeah I am not sure this is quite the correct approach, there are
> quite a few corner cases that would not be covered well here. For
> example an internally divided down 32k in which case both the 32k
> and MCLK would come from the same pin, or using the 32k to feed
> an FLL in which case we are trying to enable MCLK1 unnecessarily.
> 
> I think we could request the 32k clock source from this part
> of the code, but without implementing clock drivers for the
> chips internal clocking I think the main MCLK would need to be
> requested from the machine driver for now.
> 
> On that note, I have been working on a patch chain that adds an
> actual clock driver for the chip unfortunately this has been
> delayed somewhat due to issues interfacing SPI backed clocks to
> the clock framework. Krzysztof Kozlowski has sent a series that
> appears to resolve these issues for me so hopefully once the
> clock guys have had a look at that I can send my clock driver.
> My current implementation mostly just implements the 32k clock
> but we can start building that out to include the SYSCLKs and
> FLLs etc. fairly quickly. The best thing might be to wait for
> that and then build additional features onto that.
> 
> Let me know if you want to get an early look at that code.

Thanks for the feedback.  I would really like to avoid touching
this code and messing up anything in code shared by multiple
CODECs.  You certainly know it much better than than I do.

I got suggestion from Mark not to request the main MCLK clock
in the machine driver.  But even if gating of that clock was
added to the CODEC driver I would need to get hold of it in
the machine driver to get rate of MCLK.  So I thought about
exporting a helper from the MFD for requesting MCLK clock or
specifying MCLK clock back in the sound DT node.

IIUC, you are suggesting to leave the 32k parts of the patch 
and just drop the MCLK part?

If we provided an interface like:

struct clk * arizona_get_mclk(struct arizona *arizona, int id);
void arizona_put_mclk(struct clk *clk);

the machine driver would need to get hold of struct arizona*,
which is not that straightforward if we are given on CODEC
of_node (it can be a child of I2S or SPI bus).

Then how about specifying MCLK in the sound node and using
regular devm_clk_get()?

Regarding the clock locking patches, I think it needs some more 
discussion and should rather be merged early in the development
cycle to have good exposure in -next as it's quite an invasive
change.

I'd be happy to look at your code, if only to have a better 
overview and to avoid interfering with you work.

Anyway, my main goal is only to get the tm2_wm5110 sound card
upstream, with as little casualties as possible;)

--
Thanks, 
Sylwester

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Mark Brown]

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

On Mon, Aug 22, 2016 at 07:22:47PM +0200, Sylwester Nawrocki wrote:

> I got suggestion from Mark not to request the main MCLK clock
> in the machine driver.  But even if gating of that clock was
> added to the CODEC driver I would need to get hold of it in
> the machine driver to get rate of MCLK.  So I thought about
> exporting a helper from the MFD for requesting MCLK clock or
> specifying MCLK clock back in the sound DT node.

No, the immediate suggestion is more about the requesting bit than the
management bit - once the DT is there it's set in stone.

> If we provided an interface like:

> struct clk * arizona_get_mclk(struct arizona *arizona, int id);
> void arizona_put_mclk(struct clk *clk);

> the machine driver would need to get hold of struct arizona*,
> which is not that straightforward if we are given on CODEC
> of_node (it can be a child of I2S or SPI bus).

We could add an interface for the machine driver to retrieve a clock
from the CODEC as a transition measure...

> Then how about specifying MCLK in the sound node and using
> regular devm_clk_get()?

This is what we're trying to avoid?

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

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Mon, Aug 22, 2016 at 07:22:47PM +0200, Sylwester Nawrocki wrote:
> On 08/22/2016 11:22 AM, Charles Keepax wrote:
> > Yeah I am not sure this is quite the correct approach, there are
> > quite a few corner cases that would not be covered well here. For
> > example an internally divided down 32k in which case both the 32k
> > and MCLK would come from the same pin, or using the 32k to feed
> > an FLL in which case we are trying to enable MCLK1 unnecessarily.
> > 
> > I think we could request the 32k clock source from this part
> > of the code, but without implementing clock drivers for the
> > chips internal clocking I think the main MCLK would need to be
> > requested from the machine driver for now.
> > 
> > On that note, I have been working on a patch chain that adds an
> > actual clock driver for the chip unfortunately this has been
> > delayed somewhat due to issues interfacing SPI backed clocks to
> > the clock framework. Krzysztof Kozlowski has sent a series that
> > appears to resolve these issues for me so hopefully once the
> > clock guys have had a look at that I can send my clock driver.
> > My current implementation mostly just implements the 32k clock
> > but we can start building that out to include the SYSCLKs and
> > FLLs etc. fairly quickly. The best thing might be to wait for
> > that and then build additional features onto that.
> > 
> > Let me know if you want to get an early look at that code.
> 
> Thanks for the feedback.  I would really like to avoid touching
> this code and messing up anything in code shared by multiple
> CODECs.  You certainly know it much better than than I do.
> 
> I got suggestion from Mark not to request the main MCLK clock
> in the machine driver.  But even if gating of that clock was
> added to the CODEC driver I would need to get hold of it in
> the machine driver to get rate of MCLK.  So I thought about
> exporting a helper from the MFD for requesting MCLK clock or
> specifying MCLK clock back in the sound DT node.
> 
> IIUC, you are suggesting to leave the 32k parts of the patch 
> and just drop the MCLK part?
> 

I would certainly be ok with that it is very similar to what my
patch does but done from the MFD driver rather than moving things
out into a clock driver. Although it looks like in this case we
need to solve both clocks for it to be useful to you.

> If we provided an interface like:
> 
> struct clk * arizona_get_mclk(struct arizona *arizona, int id);
> void arizona_put_mclk(struct clk *clk);
> 
> the machine driver would need to get hold of struct arizona*,
> which is not that straightforward if we are given on CODEC
> of_node (it can be a child of I2S or SPI bus).
> 
> Then how about specifying MCLK in the sound node and using
> regular devm_clk_get()?
> 

Yeah I think we want to avoid specifying the MCLK in the sound
node as it really is a clock the codec consumes so should be
defined there in the DT.

> Regarding the clock locking patches, I think it needs some more 
> discussion and should rather be merged early in the development
> cycle to have good exposure in -next as it's quite an invasive
> change.
> 

I would agree there, and I am not sure how clear it is what the
clock guys will make of the approach yet as well. Which might
cause issues if we want to merge something now.

> I'd be happy to look at your code, if only to have a better 
> overview and to avoid interfering with you work.
> 

I am afraid I haven't managed to get time today but I will fire
you an email with my current work in progress stuff, hopefully
tomorrow.

> Anyway, my main goal is only to get the tm2_wm5110 sound card
> upstream, with as little casualties as possible;)

Apologies it seems I missed another version of this on the
mailing list, please do feel free to add me or the
patches@opensource.wolfsonmicro.com list as a direct CC on
any patches using our CODECs, I am always more than happy to look
over things and feel bad when I miss stuff. I will have a look
at the latest version of the patch.

I think we should be able to do something requesting the 32k
approximately as your existing patch here does, but then
requesting the main MCLK from the set_pll and set_sysclk
handlers. Eventually I would like the internal SYSCLK and FLLs
represented in the clock framework, so I want to have a quick
think about how that would migrate over. Let me see what I can
come up with here.

Thanks,
Charles

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Mon, Aug 22, 2016 at 07:22:47PM +0200, Sylwester Nawrocki wrote:
> On 08/22/2016 11:22 AM, Charles Keepax wrote:
> > Yeah I am not sure this is quite the correct approach, there are
> > quite a few corner cases that would not be covered well here. For
> > example an internally divided down 32k in which case both the 32k
> > and MCLK would come from the same pin, or using the 32k to feed
> > an FLL in which case we are trying to enable MCLK1 unnecessarily.
> > 
> > I think we could request the 32k clock source from this part
> > of the code, but without implementing clock drivers for the
> > chips internal clocking I think the main MCLK would need to be
> > requested from the machine driver for now.
> > 
> > On that note, I have been working on a patch chain that adds an
> > actual clock driver for the chip unfortunately this has been
> > delayed somewhat due to issues interfacing SPI backed clocks to
> > the clock framework. Krzysztof Kozlowski has sent a series that
> > appears to resolve these issues for me so hopefully once the
> > clock guys have had a look at that I can send my clock driver.
> > My current implementation mostly just implements the 32k clock
> > but we can start building that out to include the SYSCLKs and
> > FLLs etc. fairly quickly. The best thing might be to wait for
> > that and then build additional features onto that.
> > 
> > Let me know if you want to get an early look at that code.
> 
> Thanks for the feedback.  I would really like to avoid touching
> this code and messing up anything in code shared by multiple
> CODECs.  You certainly know it much better than than I do.
> 
> I got suggestion from Mark not to request the main MCLK clock
> in the machine driver.  But even if gating of that clock was
> added to the CODEC driver I would need to get hold of it in
> the machine driver to get rate of MCLK.  So I thought about
> exporting a helper from the MFD for requesting MCLK clock or
> specifying MCLK clock back in the sound DT node.
> 
> IIUC, you are suggesting to leave the 32k parts of the patch 
> and just drop the MCLK part?
> 

I would certainly be ok with that it is very similar to what my
patch does but done from the MFD driver rather than moving things
out into a clock driver. Although it looks like in this case we
need to solve both clocks for it to be useful to you.

> If we provided an interface like:
> 
> struct clk * arizona_get_mclk(struct arizona *arizona, int id);
> void arizona_put_mclk(struct clk *clk);
> 
> the machine driver would need to get hold of struct arizona*,
> which is not that straightforward if we are given on CODEC
> of_node (it can be a child of I2S or SPI bus).
> 
> Then how about specifying MCLK in the sound node and using
> regular devm_clk_get()?
> 

Yeah I think we want to avoid specifying the MCLK in the sound
node as it really is a clock the codec consumes so should be
defined there in the DT.

> Regarding the clock locking patches, I think it needs some more 
> discussion and should rather be merged early in the development
> cycle to have good exposure in -next as it's quite an invasive
> change.
> 

I would agree there, and I am not sure how clear it is what the
clock guys will make of the approach yet as well. Which might
cause issues if we want to merge something now.

> I'd be happy to look at your code, if only to have a better 
> overview and to avoid interfering with you work.
> 

I am afraid I haven't managed to get time today but I will fire
you an email with my current work in progress stuff, hopefully
tomorrow.

> Anyway, my main goal is only to get the tm2_wm5110 sound card
> upstream, with as little casualties as possible;)

Apologies it seems I missed another version of this on the
mailing list, please do feel free to add me or the
patches@opensource.wolfsonmicro.com list as a direct CC on
any patches using our CODECs, I am always more than happy to look
over things and feel bad when I miss stuff. I will have a look
at the latest version of the patch.

I think we should be able to do something requesting the 32k
approximately as your existing patch here does, but then
requesting the main MCLK from the set_pll and set_sysclk
handlers. Eventually I would like the internal SYSCLK and FLLs
represented in the clock framework, so I want to have a quick
think about how that would migrate over. Let me see what I can
come up with here.

Thanks,
Charles

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Sylwester Nawrocki]

On 08/22/2016 07:41 PM, Mark Brown wrote:
> On Mon, Aug 22, 2016 at 07:22:47PM +0200, Sylwester Nawrocki wrote:
> 
>> If we provided an interface like:
> 
>> struct clk * arizona_get_mclk(struct arizona *arizona, int id);
>> void arizona_put_mclk(struct clk *clk);
> 
>> the machine driver would need to get hold of struct arizona*,
>> which is not that straightforward if we are given only CODEC
>> of_node (it can be a child of I2C or SPI bus).
> 
> We could add an interface for the machine driver to retrieve a clock
> from the CODEC as a transition measure...

That works for me. Is the long term plan to control the CODEC's clocks
through the common clock API in machine drivers and phase out calls
like snd_soc_codec_set_pll(), snd_soc_codec_set_sysclk()?

>> Then how about specifying MCLK in the sound node and using
>> regular devm_clk_get()?
> 
> This is what we're trying to avoid?

Indeed, sorry, I just started getting confused of what are acceptable
tradeoffs here.

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Sylwester Nawrocki]

On 08/23/2016 06:28 PM, Charles Keepax wrote:
> Apologies it seems I missed another version of this on the
> mailing list, please do feel free to add me or the
> patches@opensource.wolfsonmicro.com list as a direct CC on
> any patches using our CODECs, I am always more than happy to look
> over things and feel bad when I miss stuff. I will have a look
> at the latest version of the patch.

Actually I should have copied you on the last iteration after
addressing your review comments, sorry about this omission.

> I think we should be able to do something requesting the 32k
> approximately as your existing patch here does, but then
> requesting the main MCLK from the set_pll and set_sysclk
> handlers. Eventually I would like the internal SYSCLK and FLLs
> represented in the clock framework, so I want to have a quick
> think about how that would migrate over. Let me see what I can
> come up with here.

Sounds good, I assume you mean enabling/disabling from the set_pll
and set_syclk handlers. Do you find any issues with that? I think
the CODEC should ensure external MCLK is enabled when setting FLL,
otherwise it will not lock?
Presumably arizona_set_fll() and arizona_set_sysclk() are places
where it should be checked what the root source clock is and enable
it if it's not yet enabled?

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Thu, Aug 25, 2016 at 12:18:47PM +0200, Sylwester Nawrocki wrote:
> On 08/23/2016 06:28 PM, Charles Keepax wrote:
> > I think we should be able to do something requesting the 32k
> > approximately as your existing patch here does, but then
> > requesting the main MCLK from the set_pll and set_sysclk
> > handlers. Eventually I would like the internal SYSCLK and FLLs
> > represented in the clock framework, so I want to have a quick
> > think about how that would migrate over. Let me see what I can
> > come up with here.
> 
> Sounds good, I assume you mean enabling/disabling from the set_pll
> and set_syclk handlers. Do you find any issues with that? I think
> the CODEC should ensure external MCLK is enabled when setting FLL,
> otherwise it will not lock?
> Presumably arizona_set_fll() and arizona_set_sysclk() are places
> where it should be checked what the root source clock is and enable
> it if it's not yet enabled?

Yes so the MCLK needs to be available before we start the FLL, so
my thinking was we would enable the clock that corresponds
to the source for the FLL in arizona_set_fll before we start
enabling the FLL.

The direct MCLK would require a little more work but we could
probably enable the clock in this case from wm5110_sysclk_ev.

I have sent you through a copy of my prototype clock patches you
can have a look at. I am going to have a bit of a look over this
today and hopefully will be able to get back to you with more
concrete thoughts later today.

Thanks,
Charles

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Charles Keepax]

On Thu, Aug 25, 2016 at 12:18:47PM +0200, Sylwester Nawrocki wrote:
> On 08/23/2016 06:28 PM, Charles Keepax wrote:
> > I think we should be able to do something requesting the 32k
> > approximately as your existing patch here does, but then
> > requesting the main MCLK from the set_pll and set_sysclk
> > handlers. Eventually I would like the internal SYSCLK and FLLs
> > represented in the clock framework, so I want to have a quick
> > think about how that would migrate over. Let me see what I can
> > come up with here.
> 
> Sounds good, I assume you mean enabling/disabling from the set_pll
> and set_syclk handlers. Do you find any issues with that? I think
> the CODEC should ensure external MCLK is enabled when setting FLL,
> otherwise it will not lock?
> Presumably arizona_set_fll() and arizona_set_sysclk() are places
> where it should be checked what the root source clock is and enable
> it if it's not yet enabled?

Yes so the MCLK needs to be available before we start the FLL, so
my thinking was we would enable the clock that corresponds
to the source for the FLL in arizona_set_fll before we start
enabling the FLL.

The direct MCLK would require a little more work but we could
probably enable the clock in this case from wm5110_sysclk_ev.

I have sent you through a copy of my prototype clock patches you
can have a look at. I am going to have a bit of a look over this
today and hopefully will be able to get back to you with more
concrete thoughts later today.

Thanks,
Charles

Re: [alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks

[Sylwester Nawrocki]

On 08/25/2016 03:02 PM, Charles Keepax wrote:
> Yes so the MCLK needs to be available before we start the FLL, so
> my thinking was we would enable the clock that corresponds
> to the source for the FLL in arizona_set_fll before we start
> enabling the FLL.
> 
> The direct MCLK would require a little more work but we could
> probably enable the clock in this case from wm5110_sysclk_ev.
> 
> I have sent you through a copy of my prototype clock patches you
> can have a look at. I am going to have a bit of a look over this
> today and hopefully will be able to get back to you with more
> concrete thoughts later today.

Thanks, I will have a closer look at the patches.

I have hard coded the MCLK frequency in the tm2_wm511 machine driver
as its fixed anyway and now if the MCLK gating was taken care of by
the CODEC we wouldn't need at all to get at the MCLK clocks in the
machine driver.