[alsa-devel] SoC pxa2xx-ac97 + wm9705 + touchscreen suspend/resume
Trying Linus' latest (as of this morning) out on a PXA255 platform with a WM9705 codec reveals a number of problems:
1. when suspend occurs, we turn the AC97 link off by setting the GCR_ACLINK_OFF bit, and stopping the functional units clock.
On resume, we reconfigure the pins and re-enable the clock. Unfortunately, this results in GCR being left with zeros, and GSR remains all zeros, and the codec is inaccessible, with lots of warnings from the read/write functions scrolling by:
pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0) ...
Setting GCR to '2' (to release cold reset) using devmem2 starts things moving again, shutting up this warning.
2. maybe as a result of the above problem, the wm9705 touchscreen driver doesn't reinitialize, causing loss of touchscreen. Unbinding and re-binding the driver restores the touchscreen, but with a very long lag between touching the screen and it being registered.
I bring (2) up because I notice that the resume actions in (1) are deferred. Given that codecs have shared functions (such as touchscreens) need to access the codec from their own resume functions, how can this deferral be safe?
Liam - this problem has been noticed on a NetbookPro (I'm sure you remember that platform...)
On Mon, Mar 30, 2009 at 11:37:20PM +0100, Russell King - ARM Linux wrote:
- when suspend occurs, we turn the AC97 link off by setting the
GCR_ACLINK_OFF bit, and stopping the functional units clock.
Setting GCR to '2' (to release cold reset) using devmem2 starts things moving again, shutting up this warning.
Hrm, I suspect this is a result of the second issue.
- maybe as a result of the above problem, the wm9705 touchscreen
driver doesn't reinitialize, causing loss of touchscreen. Unbinding and re-binding the driver restores the touchscreen, but with a very long lag between touching the screen and it being registered.
I bring (2) up because I notice that the resume actions in (1) are deferred. Given that codecs have shared functions (such as touchscreens) need to access the codec from their own resume functions, how can this deferral be safe?
Other multi-function devices shouldn't have this problem since they will not be relying on ASoC to resume their control interface (most likely they will be using MFD to share the device). It's not safe for AC97 devices, though.
We only really need the deferral for non-AC97 devices - it's there since some I2C buses are very slow and non-AC97 codecs often have large numbers of registers to restore and require delays to bring the codec up cleanly leading to a substantial impact on overall resume time.
Could you let me know if the patch below works, please? I've not fully tested it myself yet.
On Tue, Mar 31, 2009 at 11:30:48AM +0100, Mark Brown wrote:
On Mon, Mar 30, 2009 at 11:37:20PM +0100, Russell King - ARM Linux wrote:
- when suspend occurs, we turn the AC97 link off by setting the
GCR_ACLINK_OFF bit, and stopping the functional units clock.
Setting GCR to '2' (to release cold reset) using devmem2 starts things moving again, shutting up this warning.
Hrm, I suspect this is a result of the second issue.
- maybe as a result of the above problem, the wm9705 touchscreen
driver doesn't reinitialize, causing loss of touchscreen. Unbinding and re-binding the driver restores the touchscreen, but with a very long lag between touching the screen and it being registered.
I bring (2) up because I notice that the resume actions in (1) are deferred. Given that codecs have shared functions (such as touchscreens) need to access the codec from their own resume functions, how can this deferral be safe?
Other multi-function devices shouldn't have this problem since they will not be relying on ASoC to resume their control interface (most likely they will be using MFD to share the device). It's not safe for AC97 devices, though.
We only really need the deferral for non-AC97 devices - it's there since some I2C buses are very slow and non-AC97 codecs often have large numbers of registers to restore and require delays to bring the codec up cleanly leading to a substantial impact on overall resume time.
Could you let me know if the patch below works, please? I've not fully tested it myself yet.
I don't think this is the complete story. Sometime between 2.6.27 and 2.6.29, the structure below /sys/devices/platform/soc-audio changed (the ac97 codec moved to the top level.)
This clearly isn't right. We want to resume the host side of the link first, followed by the AC97 codec, followed by any sub drivers next. The way to guarantee that is to ensure that the parenting of the devices is correct.
In other words, this structure:
/sys/devices/platform/soc-audio/ac97-device/sub-devices-eg-touchscreen
I'm not sure where wm9705-ts currently appears in the device tree (I need to resume the device, and sort out its resume quirks so that I can see the sysfs layout... but I'm absolutely sure that the ac97 device appears at /sys/devices/platform/ which is definitely wrong.
On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
I don't think this is the complete story. Sometime between 2.6.27 and 2.6.29, the structure below /sys/devices/platform/soc-audio changed (the ac97 codec moved to the top level.)
Oh, are you using the ac97.c codec driver? With a normal ASoC codec driver the AC97 codec is not visible as a separate device in the device tree - if you could point me to your machine driver that'd be helpful.
I'll take a look at ac97.c tomorrow; there have been some problems found with the way it registers in that timeframe so it's likely that one of the fixes there has caused the regression. The way it uses the generic ALSA AC97 code means that it needs some special casing.
As a workaround I'd suggest using the generic PXA AC97 support in sound/arm rather than an ASoC machine driver. Switching your machine driver to use the WM9705 codec driver should also avoid the problems.
On Tue, Mar 31, 2009 at 08:15:40PM +0100, Mark Brown wrote:
On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
I don't think this is the complete story. Sometime between 2.6.27 and 2.6.29, the structure below /sys/devices/platform/soc-audio changed (the ac97 codec moved to the top level.)
Oh, are you using the ac97.c codec driver?
No, I'm using the wm9705 driver.
With a normal ASoC codec driver the AC97 codec is not visible as a separate device in the device tree - if you could point me to your machine driver that'd be helpful.
Not true - the codec appears as /sys/devices/0-0:WM9705 rather than a sub-device of the AC97 controller. Below this there are two sub-devices:
/sys/devices/0-0:WM9705/wm97xx-battery /sys/devices/0-0:WM9705/wm97xx-touch
The wm97xx-ts touchscreen driver binds to the 0-0:WM9705 device, so clearly unless 0-0:WM9705 is a child of the AC97 controller, things aren't going to be guaranteed to be resumed in the right order.
On Thu, Apr 02, 2009 at 12:55:41PM +0100, Russell King - ARM Linux wrote:
On Tue, Mar 31, 2009 at 08:15:40PM +0100, Mark Brown wrote:
With a normal ASoC codec driver the AC97 codec is not visible as a separate device in the device tree - if you could point me to your machine driver that'd be helpful.
Not true - the codec appears as /sys/devices/0-0:WM9705 rather than a sub-device of the AC97 controller. Below this there are two sub-devices:
Ah, I see what you mean - as you say that's actually the device for the touch functionalty of the device rather than for the codec part which is handled in soc-audio.
/sys/devices/0-0:WM9705/wm97xx-battery /sys/devices/0-0:WM9705/wm97xx-touch
The wm97xx-ts touchscreen driver binds to the 0-0:WM9705 device, so clearly unless 0-0:WM9705 is a child of the AC97 controller, things aren't going to be guaranteed to be resumed in the right order.
Right, the second patch I sent yesterday parents the device properly so the ordering should now be OK there. Did you try using the generic AC97 driver? The code for handling the controller hardware is now shared between the ASoC and that - if it fails with both that'd point at the controller support.
On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
I don't think this is the complete story. Sometime between 2.6.27 and 2.6.29, the structure below /sys/devices/platform/soc-audio changed (the ac97 codec moved to the top level.)
I've reproduced and fixed this for the ASoC native driver case (patch below); looking at the code and history I can't immediately see how this ever worked properly - the parent appears to have been NULL ever since ASoC was merged into mainline. I can't reproduce this for the ac97.c case, the code looks correct and the behaviour at runtime also appears correct in my tests.
I also haven't manage to reproduce the ordering issues with the register writes with or without my fixes so I can't be 100% confident that this will fix the problem you're seeing. If you could post your machine driver that'd be helpful.
I'm not sure where wm9705-ts currently appears in the device tree (I need to resume the device, and sort out its resume quirks so that I can see the sysfs layout... but I'm absolutely sure that the ac97 device appears at /sys/devices/platform/ which is definitely wrong.
Note that there *is* a device for the AC97 controller visible in the device tree but that currently it plays no role in suspend and resume when ASoC is being used, it exists only to tell ASoC that the AC97 device has probed. Hopefully by 2.6.31 there will be internal synchronisation within ASoC to do the suspend and resume of the entire ASoC card over multiple parent buses when the first and last component get called.
On Wed, Apr 01, 2009 at 08:27:10PM +0100, Mark Brown wrote:
On Tue, Mar 31, 2009 at 07:37:55PM +0100, Russell King - ARM Linux wrote:
I don't think this is the complete story. Sometime between 2.6.27 and 2.6.29, the structure below /sys/devices/platform/soc-audio changed (the ac97 codec moved to the top level.)
I also haven't manage to reproduce the ordering issues with the register writes with or without my fixes so I can't be 100% confident that this will fix the problem you're seeing. If you could post your machine driver that'd be helpful.
My machine driver is very simple and doesn't implement any suspend/resume callbacks (that's what other things like soc-audio are doing, right?)
I've tried both your patches, and I still have to manually write to GCR to bring the bus out of reset. I'll investigate more in the coming days.
On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:
My machine driver is very simple and doesn't implement any suspend/resume callbacks (that's what other things like soc-audio are doing, right?)
Yes, that's normal - you should only need to implement suspend and resume callbacks for the machine if there is extra hardware on the board that is handled by the machine driver.
I've tried both your patches, and I still have to manually write to GCR to bring the bus out of reset. I'll investigate more in the coming days.
Which PXA variant is the system using?
On Thu, Apr 02, 2009 at 01:18:03PM +0100, Mark Brown wrote:
On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:
I've tried both your patches, and I still have to manually write to GCR to bring the bus out of reset. I'll investigate more in the coming days.
Which PXA variant is the system using?
PXA255.
On Thu, Apr 02, 2009 at 01:38:44PM +0100, Russell King - ARM Linux wrote:
On Thu, Apr 02, 2009 at 01:18:03PM +0100, Mark Brown wrote:
On Thu, Apr 02, 2009 at 01:13:48PM +0100, Russell King - ARM Linux wrote:
I've tried both your patches, and I still have to manually write to GCR to bring the bus out of reset. I'll investigate more in the coming days.
Which PXA variant is the system using?
PXA255.
Right, adding some printk's, which include into pxa2xx-ac97's reset functions reveals...
soc_resume: entry soc_resume_deferred: entry pxa2xx_ac97_resume: soc_resume_deferred: exit soc_resume: exit wm97xx_resume: entry pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=120 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=76 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=78 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=80 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=82 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=84 GSR=0x0) pxa2xx_ac97_write: write error (ac97_reg=86 GSR=0x0) wm97xx_resume: exit
So, basically, we're starting to access the AC97 bus without first initializing the controller. This can't work on _any_ PXA platform.
On any reset, GCR is initialized by the hardware to zero. That means that the PXA AC97 hardware is held in reset state, and the codec is held in cold reset.
On resume or probe, we need to take the hardware out of cold reset.
That happens in the probe just fine because one of the first actions that the core ALSA AC97 code does is to issue a cold reset - see sound/pci/ac97/ac97_codec.c:snd_ac97_mixer() where it calls the bus reset() method.
And here we have the problem... using Ian's wm9705 driver, it doesn't implement any suspend/resume ops. I assume Ian's e-series platforms aren't suspend-able.
Having added the suspend/resume functions to the driver in the same manner as soc/codecs/ac97.c:
static int wm9705_soc_suspend(struct platform_device *pdev, pm_message_t msg) { struct snd_soc_device *socdev = platform_get_drvdata(pdev); snd_ac97_suspend(socdev->card->codec->ac97); return 0; }
static int wm9705_soc_resume(struct platform_device *pdev) { struct snd_soc_device *socdev = platform_get_drvdata(pdev); snd_ac97_resume(socdev->card->codec->ac97); return 0; }
I now find that the board crashes inexplicably on suspend... because... Argh, Ian, what pile of mess have you created in wm9705?
There's *no* AC97 initialization in there. Whereas ac97.c does:
/* add codec as bus device for standard ac97 */ ret = snd_ac97_bus(codec->card, 0, &soc_ac97_ops, NULL, &ac97_bus); if (ret < 0) goto bus_err;
memset(&ac97_template, 0, sizeof(struct snd_ac97_template)); ret = snd_ac97_mixer(ac97_bus, &ac97_template, &codec->ac97); if (ret < 0) goto bus_err;
You do:
ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0);
which doesn't actually give us an AC97 codec device - it only kmallocs some memory for them.
Okay, I'm going to shelve this project for at least the next three months now, and will return to it when (and if) the ASoC codec situation improves.
On Thu, Apr 02, 2009 at 02:31:24PM +0100, Russell King - ARM Linux wrote:
So, basically, we're starting to access the AC97 bus without first initializing the controller. This can't work on _any_ PXA platform.
Ah, that explains it - yes, this just isn't going to do suspend/resume as-is (but see the patch below).
That happens in the probe just fine because one of the first actions that the core ALSA AC97 code does is to issue a cold reset - see sound/pci/ac97/ac97_codec.c:snd_ac97_mixer() where it calls the bus reset() method.
The core ALSA AC97 code shouldn't be getting involved for ASoC-specific CODEC drivers. An ASoC-specific driver should be doing whatever is required to the CODEC directly - one of the reasons for having an ASoC-specific driver is to allow more detailed management of the device than can be supported within the standard ALSA AC97 framework.
ac97.c calls into the standard ALSA AC97 code to provide an adaption layer which allows the generic AC97 support to be used with an ASoC AC97 controller driver for devices which don't need or want any additional support.
I now find that the board crashes inexplicably on suspend... because... Argh, Ian, what pile of mess have you created in wm9705?
You do:
ret = snd_soc_new_ac97_codec(codec, &soc_ac97_ops, 0);
which doesn't actually give us an AC97 codec device - it only kmallocs some memory for them.
That's what an ASoC AC97 driver should do - it is responsible for bringing up the link by issuing a reset operation (which the wm9705 driver is doing) and doing any other setup required. The ASoC core will then go and register the AC97 bus device for child devices like the
Okay, I'm going to shelve this project for at least the next three months now, and will return to it when (and if) the ASoC codec situation improves.
Before doing that could you please try the patch below to wm9705.c implementing suspend and resume operations? It's completely speculative, I don't have a WM9705 test system to hand but I'm moderately confident
On Thu, Apr 02, 2009 at 03:12:01PM +0100, Mark Brown wrote:
Before doing that could you please try the patch below to wm9705.c implementing suspend and resume operations? It's completely speculative, I don't have a WM9705 test system to hand but I'm moderately confident
No real change - since I got rid of the debug printk's I scattered in last time around, I don't know if this patch has changed anything.
It's actually worse though - using my devmem2 hack no longer restores things to a functional state. devmem2 says the control register is now set as:
Value at address 0x4050000C: 0xC0002
but I'm still seeing lots of:
pxa2xx_ac97_write: write error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0) pxa2xx_ac97_read: read error (ac97_reg=118 GSR=0x0) ...
messages. Before, setting the control register to '2' (thereby releasing the cold reset) caused things to start working again.
On Thu, Apr 02, 2009 at 03:40:17PM +0100, Russell King - ARM Linux wrote:
It's actually worse though - using my devmem2 hack no longer restores things to a functional state. devmem2 says the control register is now set as:
Gah, I see one problem with the patch I sent. The one below *might* do better if you have time to test it. In any case, thanks a lot for your help here - I'm fairly confident that the remaining problems you're seeing are due to the lack of suspend and resume operations in the WM9705 driver.
On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:
On Thu, Apr 02, 2009 at 03:40:17PM +0100, Russell King - ARM Linux wrote:
It's actually worse though - using my devmem2 hack no longer restores things to a functional state. devmem2 says the control register is now set as:
Gah, I see one problem with the patch I sent. The one below *might* do better if you have time to test it. In any case, thanks a lot for your help here - I'm fairly confident that the remaining problems you're seeing are due to the lack of suspend and resume operations in the WM9705 driver.
From eeec4f57eb9d8f5f45d9536319e0eb130cc3ba28 Mon Sep 17 00:00:00 2001
From: Mark Brown broonie@opensource.wolfsonmicro.com Date: Thu, 2 Apr 2009 15:49:41 +0100 Subject: [PATCH] ASoC: Implement suspend and resume operations for WM9705
Compile tested only.
I beg to disagree with that statement...
sound/soc/codecs/wm9705.c: In function `wm9705_soc_suspend': sound/soc/codecs/wm9705.c:326: error: `AC97_POWERDONW' undeclared (first use in this function) sound/soc/codecs/wm9705.c:326: error: (Each undeclared identifier is reported only once sound/soc/codecs/wm9705.c:326: error: for each function it appears in.) sound/soc/codecs/wm9705.c: At top level: sound/soc/codecs/wm9705.c:445: warning: initialization from incompatible pointer type
Fixing these results in something which can be built, and along with your other two for the soc-core resume and the ac97 device parent, the complaints from the touchscreen driver no longer appear.
Thanks.
On Thu, Apr 02, 2009 at 04:09:22PM +0100, Russell King - ARM Linux wrote:
On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:
Compile tested only.
I beg to disagree with that statement...
Sorry about that - spot the use of git commit --amend.
Fixing these results in something which can be built, and along with your other two for the soc-core resume and the ac97 device parent, the complaints from the touchscreen driver no longer appear.
Excellent - I'll push the fixed patches to Takashi when he's back from vacation. Thanks again for your work tracking this down.
On Thu, Apr 02, 2009 at 04:22:34PM +0100, Mark Brown wrote:
On Thu, Apr 02, 2009 at 04:09:22PM +0100, Russell King - ARM Linux wrote:
On Thu, Apr 02, 2009 at 03:53:30PM +0100, Mark Brown wrote:
Compile tested only.
I beg to disagree with that statement...
Sorry about that - spot the use of git commit --amend.
Fixing these results in something which can be built, and along with your other two for the soc-core resume and the ac97 device parent, the complaints from the touchscreen driver no longer appear.
Excellent - I'll push the fixed patches to Takashi when he's back from vacation. Thanks again for your work tracking this down.
I notice that the fixes were merged, minus one:
sound/soc/codecs/wm9705.c: At top level: sound/soc/codecs/wm9705.c:445: warning: initialization from incompatible pointer type
so you might find this trivial patch useful.
Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk
diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c index 6e23a81..c2d1a7a 100644 --- a/sound/soc/codecs/wm9705.c +++ b/sound/soc/codecs/wm9705.c @@ -318,7 +318,7 @@ static int wm9705_reset(struct snd_soc_codec *codec) }
#ifdef CONFIG_PM -static int wm9705_soc_suspend(struct platform_device *pdev) +static int wm9705_soc_suspend(struct platform_device *pdev, pm_message_t msg) { struct snd_soc_device *socdev = platform_get_drvdata(pdev); struct snd_soc_codec *codec = socdev->card->codec;
Russell King - ARM Linux wrote:
And here we have the problem... using Ian's wm9705 driver, it doesn't implement any suspend/resume ops. I assume Ian's e-series platforms aren't suspend-able.
Correct - at present.
I now find that the board crashes inexplicably on suspend... because... Argh, Ian, what pile of mess have you created in wm9705?
Um no. Its correct. The core handles the init.
Anyhow, I see that you and Mark have tracked this down, nice work.
Nice to know that my codec driver has come in useful beyond my own e-series platform.
-Ian
On Mon, 2009-03-30 at 23:37 +0100, Russell King - ARM Linux wrote:
Liam - this problem has been noticed on a NetbookPro (I'm sure you remember that platform...)
I do indeed ;)
Mark, unfortunately my netbook is dead otherwise I could have tested.
Liam
Russell King - ARM Linux wrote:
Trying Linus' latest (as of this morning) out on a PXA255 platform with a WM9705 codec reveals a number of problems:
are you using my wm9705 soc codec, or something else?
participants (4)
-
Ian Molton -
Liam Girdwood -
Mark Brown -
Russell King - ARM Linux