[alsa-devel] [RFC PATCH] mfd: arizona: Add gating of external MCLKn clocks
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;
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
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
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?
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.
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
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?
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
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.
participants (3)
-
Charles Keepax -
Mark Brown -
Sylwester Nawrocki