[PATCH 0/4] arizona: Improvements to codec DVFS control

[PATCH 0/4] arizona: Improvements to codec DVFS control

[Richard Fitzgerald]

This set of changes cleans up the way the drivers handle
the control of internal subsystem clocks for codecs with
DVFS. This previously relied on the clock control happening
as a side-effect of changing the regulator voltage, rather
than explicitly, which assumed that you were uusing the
built-int regulator. Also DVFS control was being done from
the ADSP driver despite that driver supposedly being codec-
agnostic.
There are other cases on some codecs where DVFS control is
needed so this sequence of patches cleans up the way DVFS
is implemented and adds the extra case of DVFS boost for
higher sample rates on WM8997 and WM5102

Richard Fitzgerald (4):
  mfd: arizona: Export function to control subsystem DVFS
  ASoC: wm_adsp: Move DVFS control into codec driver
  ASoC: arizona: Add DVFS handling for sample rate control
  regulator: arizona-ldo1: Do not control clocking from regulator

 drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
 drivers/regulator/arizona-ldo1.c |    5 --
 include/linux/mfd/arizona/core.h |   12 +++++
 sound/soc/codecs/arizona.c       |   22 +++++++++-
 sound/soc/codecs/wm5102.c        |   47 ++++++++++++++++++++-
 sound/soc/codecs/wm5110.c        |    2 +-
 sound/soc/codecs/wm_adsp.c       |   76 +---------------------------------
 sound/soc/codecs/wm_adsp.h       |   11 +++--
 8 files changed, 170 insertions(+), 89 deletions(-)

-- 
1.7.2.5

[PATCH 0/4] arizona: Improvements to codec DVFS control

[Richard Fitzgerald]

This set of changes cleans up the way the drivers handle
the control of internal subsystem clocks for codecs with
DVFS. This previously relied on the clock control happening
as a side-effect of changing the regulator voltage, rather
than explicitly, which assumed that you were uusing the
built-int regulator. Also DVFS control was being done from
the ADSP driver despite that driver supposedly being codec-
agnostic.
There are other cases on some codecs where DVFS control is
needed so this sequence of patches cleans up the way DVFS
is implemented and adds the extra case of DVFS boost for
higher sample rates on WM8997 and WM5102

Richard Fitzgerald (4):
  mfd: arizona: Export function to control subsystem DVFS
  ASoC: wm_adsp: Move DVFS control into codec driver
  ASoC: arizona: Add DVFS handling for sample rate control
  regulator: arizona-ldo1: Do not control clocking from regulator

 drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
 drivers/regulator/arizona-ldo1.c |    5 --
 include/linux/mfd/arizona/core.h |   12 +++++
 sound/soc/codecs/arizona.c       |   22 +++++++++-
 sound/soc/codecs/wm5102.c        |   47 ++++++++++++++++++++-
 sound/soc/codecs/wm5110.c        |    2 +-
 sound/soc/codecs/wm_adsp.c       |   76 +---------------------------------
 sound/soc/codecs/wm_adsp.h       |   11 +++--
 8 files changed, 170 insertions(+), 89 deletions(-)

-- 
1.7.2.5

[PATCH 1/4] mfd: arizona: Export function to control subsystem DVFS

[Richard Fitzgerald]

Moving this control from being a side-effect of the LDO1
regulator driver to a specific exported function.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/arizona/core.h |   12 +++++
 2 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 58e1fe5..a1b4fe6 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -94,6 +94,89 @@ int arizona_clk32k_disable(struct arizona *arizona)
 }
 EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
 
+int arizona_dvfs_up(struct arizona *arizona, unsigned int mask)
+{
+	unsigned int new_flags;
+	int ret = 0;
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	new_flags = arizona->subsys_max_rq | mask;
+
+	if (arizona->subsys_max_rq != new_flags) {
+		switch (arizona->type) {
+		case WM5102:
+		case WM8997:
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1800000, 1800000);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to raise dcvdd (%u)\n", ret);
+				goto err;
+			}
+
+			ret = regmap_update_bits(arizona->regmap,
+					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+					ARIZONA_SUBSYS_MAX_FREQ, 1);
+			if (ret != 0) {
+				dev_err(arizona->dev,
+					"Failed to enable subsys max (%u)\n",
+					ret);
+				regulator_set_voltage(arizona->dcvdd,
+						      1200000, 1800000);
+				goto err;
+			}
+			break;
+
+		default:
+			break;
+		}
+
+	arizona->subsys_max_rq = new_flags;
+	}
+err:
+	mutex_unlock(&arizona->subsys_max_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_up);
+
+int arizona_dvfs_down(struct arizona *arizona, unsigned int mask)
+{
+	int ret = 0;
+
+	mutex_lock(&arizona->subsys_max_lock);
+
+	arizona->subsys_max_rq &= ~mask;
+
+	if (arizona->subsys_max_rq == 0) {
+		switch (arizona->type) {
+		case WM5102:
+		case WM8997:
+			ret = regmap_update_bits(arizona->regmap,
+					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
+					ARIZONA_SUBSYS_MAX_FREQ, 0);
+			if (ret != 0)
+				dev_err(arizona->dev,
+					"Failed to disable subsys max (%u)\n",
+					ret);
+
+			ret = regulator_set_voltage(arizona->dcvdd,
+						    1200000, 1800000);
+			if (ret != 0)
+				dev_err(arizona->dev,
+					"Failed to lower dcvdd (%u)\n", ret);
+			break;
+
+		default:
+			break;
+		}
+	}
+
+	mutex_unlock(&arizona->subsys_max_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(arizona_dvfs_down);
+
 static irqreturn_t arizona_clkgen_err(int irq, void *data)
 {
 	struct arizona *arizona = data;
@@ -645,6 +728,7 @@ int arizona_dev_init(struct arizona *arizona)
 
 	dev_set_drvdata(arizona->dev, arizona);
 	mutex_init(&arizona->clk_lock);
+	mutex_init(&arizona->subsys_max_lock);
 
 	if (dev_get_platdata(arizona->dev))
 		memcpy(&arizona->pdata, dev_get_platdata(arizona->dev),
diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
index 76564af..b4b8a7e 100644
--- a/include/linux/mfd/arizona/core.h
+++ b/include/linux/mfd/arizona/core.h
@@ -132,11 +132,23 @@ struct arizona {
 	struct mutex clk_lock;
 	int clk32k_ref;
 
+	struct mutex subsys_max_lock;
+	unsigned int subsys_max_rq;
+
 	struct snd_soc_dapm_context *dapm;
 };
 
+#define ARIZONA_DVFS_SR1_RQ          0x00000001
+#define ARIZONA_DVFS_SR2_RQ          0x00000002
+#define ARIZONA_DVFS_SR3_RQ          0x00000004
+#define ARIZONA_DVFS_ASR1_RQ         0x00000010
+#define ARIZONA_DVFS_ASR2_RQ         0x00000020
+#define ARIZONA_DVFS_ADSP1_RQ        0x00010000
+
 int arizona_clk32k_enable(struct arizona *arizona);
 int arizona_clk32k_disable(struct arizona *arizona);
+int arizona_dvfs_up(struct arizona *arizona, unsigned int mask);
+int arizona_dvfs_down(struct arizona *arizona, unsigned int mask);
 
 int arizona_request_irq(struct arizona *arizona, int irq, char *name,
 			irq_handler_t handler, void *data);
-- 
1.7.2.5

[PATCH 2/4] ASoC: wm_adsp: Move DVFS control into codec driver

[Richard Fitzgerald]

In theory the ADSP driver should not need to know
anything about the codec it is part of. But some codecs
need DVFS control based on ADSP clocking speed. This was
being handled by bundling part of the knowledge of this
into the ADSP driver.

This change removes this handling out of the ADSP driver.
A new macro WM_ADSP2_E() takes a callback function to be
called by the preloader widget in place of the default
handler, and this can be used to do codec-specific power
control.

The WM5102 driver has been updated to implement the DVFS.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm5102.c  |   47 ++++++++++++++++++++++++++-
 sound/soc/codecs/wm5110.c  |    2 +-
 sound/soc/codecs/wm_adsp.c |   76 +-------------------------------------------
 sound/soc/codecs/wm_adsp.h |   11 +++---
 4 files changed, 53 insertions(+), 83 deletions(-)

diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c
index dcf1d12..47cc404 100644
--- a/sound/soc/codecs/wm5102.c
+++ b/sound/soc/codecs/wm5102.c
@@ -612,6 +612,49 @@ static int wm5102_sysclk_ev(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int wm5102_adsp_power_ev(struct snd_soc_dapm_widget *w,
+		   struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = w->codec;
+	struct arizona *arizona = dev_get_drvdata(codec->dev->parent);
+	unsigned int v;
+	int ret;
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		ret = regmap_read(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, &v);
+		if (ret != 0) {
+			dev_err(codec->dev,
+				"Failed to read SYSCLK state: %d\n", ret);
+			return -EIO;
+		}
+
+		v = (v & ARIZONA_SYSCLK_FREQ_MASK) >> ARIZONA_SYSCLK_FREQ_SHIFT;
+
+		if (v >= 3) {
+			ret = arizona_dvfs_up(arizona, ARIZONA_DVFS_ADSP1_RQ);
+			if (ret != 0) {
+				dev_err(codec->dev,
+					"Failed to raise DVFS: %d\n", ret);
+				return ret;
+			}
+		}
+		break;
+
+	case SND_SOC_DAPM_PRE_PMD:
+		ret = arizona_dvfs_down(arizona, ARIZONA_DVFS_ADSP1_RQ);
+		if (ret != 0)
+			dev_warn(codec->dev,
+				 "Failed to lower DVFS: %d\n", ret);
+		break;
+
+	default:
+		break;
+	}
+
+	return wm_adsp2_early_event(w, kcontrol, event);
+}
+
 static const char *wm5102_osr_text[] = {
 	"Low power", "Normal", "High performance",
 };
@@ -1300,7 +1343,7 @@ ARIZONA_MUX_WIDGETS(ISRC2DEC2, "ISRC2DEC2"),
 ARIZONA_MUX_WIDGETS(ISRC2INT1, "ISRC2INT1"),
 ARIZONA_MUX_WIDGETS(ISRC2INT2, "ISRC2INT2"),
 
-WM_ADSP2("DSP1", 0),
+WM_ADSP2_E("DSP1", 0, wm5102_adsp_power_ev),
 
 SND_SOC_DAPM_OUTPUT("HPOUT1L"),
 SND_SOC_DAPM_OUTPUT("HPOUT1R"),
@@ -1843,7 +1886,7 @@ static int wm5102_probe(struct platform_device *pdev)
 	wm5102->core.adsp[0].mem = wm5102_dsp1_regions;
 	wm5102->core.adsp[0].num_mems = ARRAY_SIZE(wm5102_dsp1_regions);
 
-	ret = wm_adsp2_init(&wm5102->core.adsp[0], true);
+	ret = wm_adsp2_init(&wm5102->core.adsp[0]);
 	if (ret != 0)
 		return ret;
 
diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c
index 83a7e2f..a5c9f62 100644
--- a/sound/soc/codecs/wm5110.c
+++ b/sound/soc/codecs/wm5110.c
@@ -1683,7 +1683,7 @@ static int wm5110_probe(struct platform_device *pdev)
 		wm5110->core.adsp[i].num_mems
 			= ARRAY_SIZE(wm5110_dsp1_regions);
 
-		ret = wm_adsp2_init(&wm5110->core.adsp[i], false);
+		ret = wm_adsp2_init(&wm5110->core.adsp[i]);
 		if (ret != 0)
 			return ret;
 	}
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 53e3ab5..875a802 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -1539,35 +1539,6 @@ static void wm_adsp2_boot_work(struct work_struct *work)
 		return;
 	}
 
-	if (dsp->dvfs) {
-		ret = regmap_read(dsp->regmap,
-				  dsp->base + ADSP2_CLOCKING, &val);
-		if (ret != 0) {
-			dev_err(dsp->dev, "Failed to read clocking: %d\n", ret);
-			return;
-		}
-
-		if ((val & ADSP2_CLK_SEL_MASK) >= 3) {
-			ret = regulator_enable(dsp->dvfs);
-			if (ret != 0) {
-				dev_err(dsp->dev,
-					"Failed to enable supply: %d\n",
-					ret);
-				return;
-			}
-
-			ret = regulator_set_voltage(dsp->dvfs,
-						    1800000,
-						    1800000);
-			if (ret != 0) {
-				dev_err(dsp->dev,
-					"Failed to raise supply: %d\n",
-					ret);
-				return;
-			}
-		}
-	}
-
 	ret = wm_adsp2_ena(dsp);
 	if (ret != 0)
 		return;
@@ -1668,21 +1639,6 @@ int wm_adsp2_event(struct snd_soc_dapm_widget *w,
 		regmap_write(dsp->regmap, dsp->base + ADSP2_WDMA_CONFIG_2, 0);
 		regmap_write(dsp->regmap, dsp->base + ADSP2_RDMA_CONFIG_1, 0);
 
-		if (dsp->dvfs) {
-			ret = regulator_set_voltage(dsp->dvfs, 1200000,
-						    1800000);
-			if (ret != 0)
-				dev_warn(dsp->dev,
-					 "Failed to lower supply: %d\n",
-					 ret);
-
-			ret = regulator_disable(dsp->dvfs);
-			if (ret != 0)
-				dev_err(dsp->dev,
-					"Failed to enable supply: %d\n",
-					ret);
-		}
-
 		list_for_each_entry(ctl, &dsp->ctl_list, list)
 			ctl->enabled = 0;
 
@@ -1709,7 +1665,7 @@ err:
 }
 EXPORT_SYMBOL_GPL(wm_adsp2_event);
 
-int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs)
+int wm_adsp2_init(struct wm_adsp *adsp)
 {
 	int ret;
 
@@ -1728,36 +1684,6 @@ int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs)
 	INIT_LIST_HEAD(&adsp->ctl_list);
 	INIT_WORK(&adsp->boot_work, wm_adsp2_boot_work);
 
-	if (dvfs) {
-		adsp->dvfs = devm_regulator_get(adsp->dev, "DCVDD");
-		if (IS_ERR(adsp->dvfs)) {
-			ret = PTR_ERR(adsp->dvfs);
-			dev_err(adsp->dev, "Failed to get DCVDD: %d\n", ret);
-			return ret;
-		}
-
-		ret = regulator_enable(adsp->dvfs);
-		if (ret != 0) {
-			dev_err(adsp->dev, "Failed to enable DCVDD: %d\n",
-				ret);
-			return ret;
-		}
-
-		ret = regulator_set_voltage(adsp->dvfs, 1200000, 1800000);
-		if (ret != 0) {
-			dev_err(adsp->dev, "Failed to initialise DVFS: %d\n",
-				ret);
-			return ret;
-		}
-
-		ret = regulator_disable(adsp->dvfs);
-		if (ret != 0) {
-			dev_err(adsp->dev, "Failed to disable DCVDD: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(wm_adsp2_init);
diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h
index a4f6b64..8b208d1 100644
--- a/sound/soc/codecs/wm_adsp.h
+++ b/sound/soc/codecs/wm_adsp.h
@@ -56,8 +56,6 @@ struct wm_adsp {
 	int fw;
 	bool running;
 
-	struct regulator *dvfs;
-
 	struct list_head ctl_list;
 
 	struct work_struct boot_work;
@@ -67,19 +65,22 @@ struct wm_adsp {
 	SND_SOC_DAPM_PGA_E(wname, SND_SOC_NOPM, num, 0, NULL, 0, \
 		wm_adsp1_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD)
 
-#define WM_ADSP2(wname, num) \
+#define WM_ADSP2_E(wname, num, event_fn) \
 {	.id = snd_soc_dapm_dai_link, .name = wname " Preloader", \
-	.reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_early_event, \
+	.reg = SND_SOC_NOPM, .shift = num, .event = event_fn, \
 	.event_flags = SND_SOC_DAPM_PRE_PMU }, \
 {	.id = snd_soc_dapm_out_drv, .name = wname, \
 	.reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_event, \
 	.event_flags = SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD }
 
+#define WM_ADSP2(wname, num) \
+	WM_ADSP2_E(wname, num, wm_adsp2_early_event)
+
 extern const struct snd_kcontrol_new wm_adsp1_fw_controls[];
 extern const struct snd_kcontrol_new wm_adsp2_fw_controls[];
 
 int wm_adsp1_init(struct wm_adsp *adsp);
-int wm_adsp2_init(struct wm_adsp *adsp, bool dvfs);
+int wm_adsp2_init(struct wm_adsp *adsp);
 int wm_adsp1_event(struct snd_soc_dapm_widget *w,
 		   struct snd_kcontrol *kcontrol, int event);
 int wm_adsp2_early_event(struct snd_soc_dapm_widget *w,
-- 
1.7.2.5

[PATCH 3/4] ASoC: arizona: Add DVFS handling for sample rate control

[Richard Fitzgerald]

Some codecs need to boost DVFS for higher sample rates.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/arizona.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c
index 1b14105..7e33d1b 100644
--- a/sound/soc/codecs/arizona.c
+++ b/sound/soc/codecs/arizona.c
@@ -1135,7 +1135,7 @@ static int arizona_hw_params_rate(struct snd_pcm_substream *substream,
 	struct arizona_priv *priv = snd_soc_codec_get_drvdata(codec);
 	struct arizona_dai_priv *dai_priv = &priv->dai[dai->id - 1];
 	int base = dai->driver->base;
-	int i, sr_val;
+	int i, sr_val, ret;
 
 	/*
 	 * We will need to be more flexible than this in future,
@@ -1151,6 +1151,26 @@ static int arizona_hw_params_rate(struct snd_pcm_substream *substream,
 	}
 	sr_val = i;
 
+	switch (priv->arizona->type) {
+	case WM5102:
+	case WM8997:
+		if (arizona_sr_vals[sr_val] >= 88200)
+			ret = arizona_dvfs_up(priv->arizona,
+					      ARIZONA_DVFS_SR1_RQ);
+		else
+			ret = arizona_dvfs_down(priv->arizona,
+						ARIZONA_DVFS_SR1_RQ);
+
+		if (ret != 0) {
+			arizona_aif_err(dai, "Failed to change DVFS %d\n", ret);
+			return ret;
+		}
+		break;
+
+	default:
+		break;
+	}
+
 	switch (dai_priv->clk) {
 	case ARIZONA_CLK_SYSCLK:
 		snd_soc_update_bits(codec, ARIZONA_SAMPLE_RATE_1,
-- 
1.7.2.5

[PATCH 4/4] regulator: arizona-ldo1: Do not control clocking from regulator

[Richard Fitzgerald]

Using the driver for the internal regulator to also control
the clock frequency of blocks inside the codec is an
unexpected side-effect for a regulator, and also means that
the core clocks won't be changed as expected if an external
regulator is used to power the codec.

The clocking control is now handled by the core arizona MFD
driver so can be removed from the LDO1 driver.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 drivers/regulator/arizona-ldo1.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index d3787e1..ea4fbce 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -77,11 +77,6 @@ static int arizona_ldo1_hc_set_voltage_sel(struct regulator_dev *rdev,
 	if (ret != 0)
 		return ret;
 
-	ret = regmap_update_bits(regmap, ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
-				 ARIZONA_SUBSYS_MAX_FREQ, val);
-	if (ret != 0)
-		return ret;
-
 	if (val)
 		return 0;
 
-- 
1.7.2.5

Re: [PATCH 4/4] regulator: arizona-ldo1: Do not control clocking from regulator

[Mark Brown]

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

On Mon, Jun 09, 2014 at 04:04:35PM +0100, Richard Fitzgerald wrote:
> Using the driver for the internal regulator to also control
> the clock frequency of blocks inside the codec is an
> unexpected side-effect for a regulator, and also means that
> the core clocks won't be changed as expected if an external
> regulator is used to power the codec.

IIRC this was deliberately coded in this fashion on advice from the
hardware engineers - there was more going on with that register than
there might at first appear and some actual sync with the LDO.  I
believe there was some different process to follow (possibly just
setting this mode all the time) when using an external regulator, though
it's also possible the hardware guys were just unsure at the time.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] ASoC: wm_adsp: Move DVFS control into codec driver

[Charles Keepax]

On Mon, Jun 09, 2014 at 04:03:14PM +0100, Richard Fitzgerald wrote:
>  
> -#define WM_ADSP2(wname, num) \
> +#define WM_ADSP2_E(wname, num, event_fn) \
>  {	.id = snd_soc_dapm_dai_link, .name = wname " Preloader", \
> -	.reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_early_event, \
> +	.reg = SND_SOC_NOPM, .shift = num, .event = event_fn, \
>  	.event_flags = SND_SOC_DAPM_PRE_PMU }, \

You need to add a SND_SOC_DAPM_PRE_PMD in here otherwise your
wm5102_sysclk_ev won't get called on power down.

Thanks,
Charles

Re: [PATCH 2/4] ASoC: wm_adsp: Move DVFS control into codec driver

[Charles Keepax]

On Mon, Jun 09, 2014 at 04:03:14PM +0100, Richard Fitzgerald wrote:
>  
> -#define WM_ADSP2(wname, num) \
> +#define WM_ADSP2_E(wname, num, event_fn) \
>  {	.id = snd_soc_dapm_dai_link, .name = wname " Preloader", \
> -	.reg = SND_SOC_NOPM, .shift = num, .event = wm_adsp2_early_event, \
> +	.reg = SND_SOC_NOPM, .shift = num, .event = event_fn, \
>  	.event_flags = SND_SOC_DAPM_PRE_PMU }, \

You need to add a SND_SOC_DAPM_PRE_PMD in here otherwise your
wm5102_sysclk_ev won't get called on power down.

Thanks,
Charles

Re: [PATCH 4/4] regulator: arizona-ldo1: Do not control clocking from regulator

[Charles Keepax]

On Mon, Jun 09, 2014 at 07:43:14PM +0100, Mark Brown wrote:
> On Mon, Jun 09, 2014 at 04:04:35PM +0100, Richard Fitzgerald wrote:
> > Using the driver for the internal regulator to also control
> > the clock frequency of blocks inside the codec is an
> > unexpected side-effect for a regulator, and also means that
> > the core clocks won't be changed as expected if an external
> > regulator is used to power the codec.
> 
> IIRC this was deliberately coded in this fashion on advice from the
> hardware engineers - there was more going on with that register than
> there might at first appear and some actual sync with the LDO.  I
> believe there was some different process to follow (possibly just
> setting this mode all the time) when using an external regulator, though
> it's also possible the hardware guys were just unsure at the time.

I have had a good chat with the hardware engineers here and they
are pretty adamant that the only constraint here is that we
should never enable SUBSYS without 1.8V being supplied to the
core.

They also believe that the external case should be handled the
same as the internal one, although admittedly I haven't tested
that personally.

This series needs a slight rebase and fixing up for my comment on
one of the patches, but otherwise I think should be good in my
opinion.

Thanks,
Charles

Re: [PATCH 4/4] regulator: arizona-ldo1: Do not control clocking from regulator

[Charles Keepax]

On Mon, Jun 09, 2014 at 07:43:14PM +0100, Mark Brown wrote:
> On Mon, Jun 09, 2014 at 04:04:35PM +0100, Richard Fitzgerald wrote:
> > Using the driver for the internal regulator to also control
> > the clock frequency of blocks inside the codec is an
> > unexpected side-effect for a regulator, and also means that
> > the core clocks won't be changed as expected if an external
> > regulator is used to power the codec.
> 
> IIRC this was deliberately coded in this fashion on advice from the
> hardware engineers - there was more going on with that register than
> there might at first appear and some actual sync with the LDO.  I
> believe there was some different process to follow (possibly just
> setting this mode all the time) when using an external regulator, though
> it's also possible the hardware guys were just unsure at the time.

I have had a good chat with the hardware engineers here and they
are pretty adamant that the only constraint here is that we
should never enable SUBSYS without 1.8V being supplied to the
core.

They also believe that the external case should be handled the
same as the internal one, although admittedly I haven't tested
that personally.

This series needs a slight rebase and fixing up for my comment on
one of the patches, but otherwise I think should be good in my
opinion.

Thanks,
Charles

Re: [PATCH 4/4] regulator: arizona-ldo1: Do not control clocking from regulator

[Mark Brown]

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

On Wed, Jun 11, 2014 at 11:59:26AM +0100, Charles Keepax wrote:
> On Mon, Jun 09, 2014 at 07:43:14PM +0100, Mark Brown wrote:

> > IIRC this was deliberately coded in this fashion on advice from the
> > hardware engineers - there was more going on with that register than
> > there might at first appear and some actual sync with the LDO.  I
> > believe there was some different process to follow (possibly just
> > setting this mode all the time) when using an external regulator, though
> > it's also possible the hardware guys were just unsure at the time.

> I have had a good chat with the hardware engineers here and they
> are pretty adamant that the only constraint here is that we
> should never enable SUBSYS without 1.8V being supplied to the
> core.

OK, that sounds like the advice when the device was first produced was
not accurate - might be worth checking your datasheets here.  Might also
be worth updating the commit log.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] mfd: arizona: Export function to control subsystem DVFS

[Lee Jones]

On Mon, 09 Jun 2014, Richard Fitzgerald wrote:

> Moving this control from being a side-effect of the LDO1
> regulator driver to a specific exported function.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/arizona/core.h |   12 +++++
>  2 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 58e1fe5..a1b4fe6 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -94,6 +94,89 @@ int arizona_clk32k_disable(struct arizona *arizona)
>  }
>  EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
>  
> +int arizona_dvfs_up(struct arizona *arizona, unsigned int mask)
> +{
> +	unsigned int new_flags;
> +	int ret = 0;
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	new_flags = arizona->subsys_max_rq | mask;

This doesn't look like a mask to me.  It looks like you're setting
flags rather than masking out bits?

> +	if (arizona->subsys_max_rq != new_flags) {
> +		switch (arizona->type) {
> +		case WM5102:
> +		case WM8997:
> +			ret = regulator_set_voltage(arizona->dcvdd,
> +						    1800000, 1800000);
> +			if (ret != 0) {
> +				dev_err(arizona->dev,
> +					"Failed to raise dcvdd (%u)\n", ret);
> +				goto err;
> +			}
> +
> +			ret = regmap_update_bits(arizona->regmap,
> +					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
> +					ARIZONA_SUBSYS_MAX_FREQ, 1);
> +			if (ret != 0) {
> +				dev_err(arizona->dev,
> +					"Failed to enable subsys max (%u)\n",
> +					ret);
> +				regulator_set_voltage(arizona->dcvdd,
> +						      1200000, 1800000);
> +				goto err;
> +			}
> +			break;
> +
> +		default:
> +			break;
> +		}

I don't really get this.  What's the point in passing the mask
parameter - I don't see it being used for anything in this routine? No
matter what is passed in you always just turn on the same regulator.

What am I missing?

> +	arizona->subsys_max_rq = new_flags;

This tabbing is incorrect.
> +	}
> +err:
> +	mutex_unlock(&arizona->subsys_max_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_dvfs_up);
> +
> +int arizona_dvfs_down(struct arizona *arizona, unsigned int mask)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	arizona->subsys_max_rq &= ~mask;
> +
> +	if (arizona->subsys_max_rq == 0) {
> +		switch (arizona->type) {
> +		case WM5102:
> +		case WM8997:
> +			ret = regmap_update_bits(arizona->regmap,
> +					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
> +					ARIZONA_SUBSYS_MAX_FREQ, 0);
> +			if (ret != 0)
> +				dev_err(arizona->dev,
> +					"Failed to disable subsys max (%u)\n",
> +					ret);
> +
> +			ret = regulator_set_voltage(arizona->dcvdd,
> +						    1200000, 1800000);
> +			if (ret != 0)
> +				dev_err(arizona->dev,
> +					"Failed to lower dcvdd (%u)\n", ret);
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&arizona->subsys_max_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_dvfs_down);
> +
>  static irqreturn_t arizona_clkgen_err(int irq, void *data)
>  {
>  	struct arizona *arizona = data;
> @@ -645,6 +728,7 @@ int arizona_dev_init(struct arizona *arizona)
>  
>  	dev_set_drvdata(arizona->dev, arizona);
>  	mutex_init(&arizona->clk_lock);
> +	mutex_init(&arizona->subsys_max_lock);
>  
>  	if (dev_get_platdata(arizona->dev))
>  		memcpy(&arizona->pdata, dev_get_platdata(arizona->dev),
> diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
> index 76564af..b4b8a7e 100644
> --- a/include/linux/mfd/arizona/core.h
> +++ b/include/linux/mfd/arizona/core.h
> @@ -132,11 +132,23 @@ struct arizona {
>  	struct mutex clk_lock;
>  	int clk32k_ref;
>  
> +	struct mutex subsys_max_lock;
> +	unsigned int subsys_max_rq;
> +
>  	struct snd_soc_dapm_context *dapm;
>  };
>  
> +#define ARIZONA_DVFS_SR1_RQ          0x00000001
> +#define ARIZONA_DVFS_SR2_RQ          0x00000002
> +#define ARIZONA_DVFS_SR3_RQ          0x00000004
> +#define ARIZONA_DVFS_ASR1_RQ         0x00000010
> +#define ARIZONA_DVFS_ASR2_RQ         0x00000020
> +#define ARIZONA_DVFS_ADSP1_RQ        0x00010000
> +
>  int arizona_clk32k_enable(struct arizona *arizona);
>  int arizona_clk32k_disable(struct arizona *arizona);
> +int arizona_dvfs_up(struct arizona *arizona, unsigned int mask);
> +int arizona_dvfs_down(struct arizona *arizona, unsigned int mask);
>  
>  int arizona_request_irq(struct arizona *arizona, int irq, char *name,
>  			irq_handler_t handler, void *data);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] mfd: arizona: Export function to control subsystem DVFS

[Mark Brown]

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

On Mon, Jun 16, 2014 at 05:42:42PM +0100, Lee Jones wrote:
> On Mon, 09 Jun 2014, Richard Fitzgerald wrote:

> > +	if (arizona->subsys_max_rq != new_flags) {

> I don't really get this.  What's the point in passing the mask
> parameter - I don't see it being used for anything in this routine? No
> matter what is passed in you always just turn on the same regulator.

> What am I missing?

AFAICT it's a bunch of different independently selectable requests for
the voltage to ramped with any one of them causing it to happen.

I did wonder why this wasn't just done by refcounting, that's the more
normal pattern in the kernel, though I guess it's possible some of them
need different ramps on different devices.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] mfd: arizona: Export function to control subsystem DVFS

[Charles Keepax]

On Mon, Jun 16, 2014 at 05:42:42PM +0100, Lee Jones wrote:
> On Mon, 09 Jun 2014, Richard Fitzgerald wrote:
> 
> > Moving this control from being a side-effect of the LDO1
> > regulator driver to a specific exported function.
> > 
> > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c       |   84 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/arizona/core.h |   12 +++++
> >  2 files changed, 96 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > index 58e1fe5..a1b4fe6 100644
> > --- a/drivers/mfd/arizona-core.c
> > +++ b/drivers/mfd/arizona-core.c
> > @@ -94,6 +94,89 @@ int arizona_clk32k_disable(struct arizona *arizona)
> >  }
> >  EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
> >  
> > +int arizona_dvfs_up(struct arizona *arizona, unsigned int mask)
> > +{
> > +	unsigned int new_flags;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&arizona->subsys_max_lock);
> > +
> > +	new_flags = arizona->subsys_max_rq | mask;
> 
> This doesn't look like a mask to me.  It looks like you're setting
> flags rather than masking out bits?

Richard is on holiday so I will fill in for him. Yeah these are
flags I think mask is just a poorly chosen variable name.

> 
> > +	if (arizona->subsys_max_rq != new_flags) {
> > +		switch (arizona->type) {
> > +		case WM5102:
> > +		case WM8997:
> > +			ret = regulator_set_voltage(arizona->dcvdd,
> > +						    1800000, 1800000);
> > +			if (ret != 0) {
> > +				dev_err(arizona->dev,
> > +					"Failed to raise dcvdd (%u)\n", ret);
> > +				goto err;
> > +			}
> > +
> > +			ret = regmap_update_bits(arizona->regmap,
> > +					ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
> > +					ARIZONA_SUBSYS_MAX_FREQ, 1);
> > +			if (ret != 0) {
> > +				dev_err(arizona->dev,
> > +					"Failed to enable subsys max (%u)\n",
> > +					ret);
> > +				regulator_set_voltage(arizona->dcvdd,
> > +						      1200000, 1800000);
> > +				goto err;
> > +			}
> > +			break;
> > +
> > +		default:
> > +			break;
> > +		}
> 
> I don't really get this.  What's the point in passing the mask
> parameter - I don't see it being used for anything in this routine? No
> matter what is passed in you always just turn on the same regulator.
> 
> What am I missing?

As Mark said each bit represents something that can require the
higher clock rate. Any of the causes being active requires the
higher clock rate.

> 
> > +	arizona->subsys_max_rq = new_flags;
> 
> This tabbing is incorrect.

Will get fixed with the other comments.

Thanks,
Charles

Re: [PATCH 1/4] mfd: arizona: Export function to control subsystem DVFS

[Charles Keepax]

On Mon, Jun 16, 2014 at 05:48:58PM +0100, Mark Brown wrote:
> On Mon, Jun 16, 2014 at 05:42:42PM +0100, Lee Jones wrote:
> > On Mon, 09 Jun 2014, Richard Fitzgerald wrote:
> 
> > > +	if (arizona->subsys_max_rq != new_flags) {
> 
> > I don't really get this.  What's the point in passing the mask
> > parameter - I don't see it being used for anything in this routine? No
> > matter what is passed in you always just turn on the same regulator.
> 
> > What am I missing?
> 
> AFAICT it's a bunch of different independently selectable requests for
> the voltage to ramped with any one of them causing it to happen.
> 
> I did wonder why this wasn't just done by refcounting, that's the more
> normal pattern in the kernel, though I guess it's possible some of them
> need different ramps on different devices.

Currently this is not the case, although it is certainly not
unthinkable that it would occur in future devices.

Thanks,
Charles