[PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[Charles Keepax]

On the wm5102 the register patches are applied manually, rather than by
the regmap core. This application is wrapped in calls to
regcache_cache_bypass. However, this is dangerous as other threads may
be accessing the hardware at the same time as the pm_runtime operations
and if they do so during the period whilst cache_bypass is enabled those
writes will miss the cache when they shouldn't.

As the cache_bypass is not strictly necessary for applying the patches
(the device is always powered on whilst it is applied) remove the calls
to regcache_cache_bypass to avoid this situation.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c  |    4 ----
 drivers/mfd/wm5102-tables.c |   10 +++-------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index a45aab9..1c3ae57 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -251,8 +251,6 @@ static int arizona_apply_hardware_patch(struct arizona* arizona)
 	unsigned int fll, sysclk;
 	int ret, err;
 
-	regcache_cache_bypass(arizona->regmap, true);
-
 	/* Cache existing FLL and SYSCLK settings */
 	ret = regmap_read(arizona->regmap, ARIZONA_FLL1_CONTROL_1, &fll);
 	if (ret != 0) {
@@ -322,8 +320,6 @@ err_fll:
 			err);
 	}
 
-	regcache_cache_bypass(arizona->regmap, false);
-
 	if (ret != 0)
 		return ret;
 	else
diff --git a/drivers/mfd/wm5102-tables.c b/drivers/mfd/wm5102-tables.c
index 187ee86..b9556bc 100644
--- a/drivers/mfd/wm5102-tables.c
+++ b/drivers/mfd/wm5102-tables.c
@@ -81,7 +81,7 @@ static const struct reg_default wm5102_revb_patch[] = {
 int wm5102_patch(struct arizona *arizona)
 {
 	const struct reg_default *wm5102_patch;
-	int ret = 0;
+	int ret;
 	int i, patch_size;
 
 	switch (arizona->rev) {
@@ -93,21 +93,17 @@ int wm5102_patch(struct arizona *arizona)
 		patch_size = ARRAY_SIZE(wm5102_revb_patch);
 	}
 
-	regcache_cache_bypass(arizona->regmap, true);
-
 	for (i = 0; i < patch_size; i++) {
 		ret = regmap_write(arizona->regmap, wm5102_patch[i].reg,
 				   wm5102_patch[i].def);
 		if (ret != 0) {
 			dev_err(arizona->dev, "Failed to write %x = %x: %d\n",
 				wm5102_patch[i].reg, wm5102_patch[i].def, ret);
-			goto out;
+			return ret;
 		}
 	}
 
-out:
-	regcache_cache_bypass(arizona->regmap, false);
-	return ret;
+	return 0;
 }
 
 static const struct regmap_irq wm5102_aod_irqs[ARIZONA_NUM_IRQ] = {
-- 
1.7.2.5

Re: [PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[Mark Brown]

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

On Thu, Feb 20, 2014 at 04:26:48PM +0000, Charles Keepax wrote:

> As the cache_bypass is not strictly necessary for applying the patches
> (the device is always powered on whilst it is applied) remove the calls
> to regcache_cache_bypass to avoid this situation.

That's not why the cache bypass is there, the cache bypass is there to
make sure we don't cache the writes and hence reapply them while
restoring the cache.  This would cause breakage with writes which change
register defaults (where it would overwrite previously cached writes) or
need to be done in that specific sequence.  A bypassed write goes
straight to the hardware so it can't work if the device is powered off.

Given that this is happening on resume I expect you can achieve
sufficient exclusion by making sure that everything that writes to the
device holds a runtim PM reference which you ought to be doing anyway,
the PM infrastructure makes sure that only one resume can run at once.
However I expect that's going to be annoying with ASoC where you don't
really want to boot the device just to change controls (but perhaps with
autosuspend it's not too bad, most likely the device will be getting
powered up shortly anyway if userspace is fiddling with the controls).

Being able to use the regular patch infrastructure would of course be
nicer but you're going to need to add a callback to allow you to run the
hardware patch in that case.

You probably also want to be applying the patch using async writes to
improve performance but that's separate and just an optimisation.

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

Re: [PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[Charles Keepax]

On Fri, Feb 21, 2014 at 08:30:58AM +0900, Mark Brown wrote:
> On Thu, Feb 20, 2014 at 04:26:48PM +0000, Charles Keepax wrote:
> 
> > As the cache_bypass is not strictly necessary for applying the patches
> > (the device is always powered on whilst it is applied) remove the calls
> > to regcache_cache_bypass to avoid this situation.
> 
> That's not why the cache bypass is there, the cache bypass is there to
> make sure we don't cache the writes and hence reapply them while
> restoring the cache.  This would cause breakage with writes which change
> register defaults (where it would overwrite previously cached writes) or
> need to be done in that specific sequence.  A bypassed write goes
> straight to the hardware so it can't work if the device is powered off.

I guess there are two parts here applying the hardware patch and
the manual application of the register patch. Applying the
hardware patch restores registers once it is finished anyway, and
the actual patch is applied by the write sequencer so will go
straight to the hardware. So that really doesn't need the
cache_bypass.

Good point on the manual patch application it could potentially
clobber some user setup. Although the current register patch
should not cause any issues on this front.

> Given that this is happening on resume I expect you can achieve
> sufficient exclusion by making sure that everything that writes to the
> device holds a runtim PM reference which you ought to be doing anyway,
> the PM infrastructure makes sure that only one resume can run at once.
> However I expect that's going to be annoying with ASoC where you don't
> really want to boot the device just to change controls (but perhaps with
> autosuspend it's not too bad, most likely the device will be getting
> powered up shortly anyway if userspace is fiddling with the controls).

Yeah I had considered this and I agree it should work but I had
decided against it because of the powering up for each control
write.

> Being able to use the regular patch infrastructure would of course be
> nicer but you're going to need to add a callback to allow you to run the
> hardware patch in that case.

I had considered adding bypassed versions of regmap_write and
read. That way you could ensure that the bypass was only active
whilst the regmap lock was held and avoid the problem that way.
What do you think to that idea?

> You probably also want to be applying the patch using async writes to
> improve performance but that's separate and just an optimisation.

Good point I shall have a look at this.

Thanks,
Charles

Re: [PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[Charles Keepax]

On Fri, Feb 21, 2014 at 09:38:55AM +0000, Charles Keepax wrote:
> On Fri, Feb 21, 2014 at 08:30:58AM +0900, Mark Brown wrote:
> > On Thu, Feb 20, 2014 at 04:26:48PM +0000, Charles Keepax wrote:
> > 
> > > As the cache_bypass is not strictly necessary for applying the patches
> > > (the device is always powered on whilst it is applied) remove the calls
> > > to regcache_cache_bypass to avoid this situation.
> > 
> 
> Good point on the manual patch application it could potentially
> clobber some user setup. Although the current register patch
> should not cause any issues on this front.

Except for the update to the noise gate control register that I
missed :-(

Thanks,
Charles

Re: [PATCH] mfd: wm5102: Remove cache_bypass from manual register patching

[Mark Brown]

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

On Fri, Feb 21, 2014 at 09:38:55AM +0000, Charles Keepax wrote:

> I guess there are two parts here applying the hardware patch and
> the manual application of the register patch. Applying the
> hardware patch restores registers once it is finished anyway, and
> the actual patch is applied by the write sequencer so will go
> straight to the hardware. So that really doesn't need the
> cache_bypass.

Yeah, it's only the manually applied bit that cares.

> > Being able to use the regular patch infrastructure would of course be
> > nicer but you're going to need to add a callback to allow you to run the
> > hardware patch in that case.

> I had considered adding bypassed versions of regmap_write and
> read. That way you could ensure that the bypass was only active
> whilst the regmap lock was held and avoid the problem that way.
> What do you think to that idea?

I think that's a reasonable idea, though I see you actually did
something slightly different (which is sensible and in fact there
already).

Thinking about this stuff there's also some ideas I discussed with
Dimitris for the DSP memories where if it were possible to mark only a
section of the register map as cache only the DSP download could be sped
up a little by making the DSP memories cache only during download and
then syncing at the end to do the actual writes.  That way coefficient
writes that change defaults in the firmware would get coalesced and
possibly some writes to adjacent blocks could be combined too.  Probably
not a massive win but it'd be nice.

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