[alsa-devel] [RFC/PATCH] pxa2xx_ac97 and gpio reset line

Hi Mark,
I can't find out if somebody had submitted that kind of patch before. This is a starting point of discussion. I'm not fully convinced by my own code, and so I post it here for advice.
What I'd really like is to be able to choose the PXA AC97 gpio reset (GPIO95 or GPIO113) through platform_data, but I couldn't find a way of doing it. I hope you'll have some kind of idea.
Cheers.
-- Robert

On 19 Feb 2009, at 20:44, Robert Jarzmik robert.jarzmik@free.fr wrote:
Hi Mark,
I can't find out if somebody had submitted that kind of patch before. This is a starting point of discussion. I'm not fully convinced by my own code, and so I post it here for advice.
What I'd really like is to be able to choose the PXA AC97 gpio reset (GPIO95 or GPIO113) through platform_data, but I couldn't find a way of doing it. I hope you'll have some kind of idea.
This is a sensible idea in principle but should be done via platform data for the AC97 driver rather than via Kconfig - it'll need an update to use the platform in ASoC but I'll need to do those anyway.
Cheers.
-- Robert
From 8d4a0c602b85528af2a35fd3f1f1bd8ddca22985 Mon Sep 17 00:00:00 2001 From: Robert Jarzmik robert.jarzmik@free.fr Date: Thu, 19 Feb 2009 21:35:42 +0100 Subject: [PATCH] Allow choice in ac97 gpio reset line
As the PXA series allow 2 gpios to reset the ac97 bus, allow through configuration the definition of the correct gpio which will reset the AC97 bus.
This comes from a silicon defect on the PXA series, where the gpio must be manually controlled in warm reset cases.
Signed-off-by: Robert Jarzmik rjarzmik@free.fr
sound/arm/Kconfig | 17 +++++++++++ sound/arm/pxa2xx-ac97-lib.c | 68 ++++++++++++++++++++++++++++++++++ +++++--- 2 files changed, 80 insertions(+), 5 deletions(-)
diff --git a/sound/arm/Kconfig b/sound/arm/Kconfig index f8e6de4..2749274 100644 --- a/sound/arm/Kconfig +++ b/sound/arm/Kconfig @@ -39,6 +39,23 @@ config SND_PXA2XX_LIB config SND_PXA2XX_LIB_AC97 bool
+choice
- prompt "PXA GPIO line used to reset AC97 bus"
- depends on SND_PXA2XX_LIB_AC97
- help
Choose which GPIO is assigned the AC97 reset function.
Usually board designers use GPIO 113, but GPIO 95 can be used
as well.
+config SND_PXA2XX_LIB_AC97_RESET113
- bool "GPIO113"
+config SND_PXA2XX_LIB_AC97_RESET95
- bool "GPIO95"
+endchoice
config SND_PXA2XX_AC97 tristate "AC97 driver for the Intel PXA2xx chip" depends on ARCH_PXA diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c index 35afd0c..aa522d2 100644 --- a/sound/arm/pxa2xx-ac97-lib.c +++ b/sound/arm/pxa2xx-ac97-lib.c @@ -31,6 +31,8 @@ static DECLARE_WAIT_QUEUE_HEAD(gsr_wq); static volatile long gsr_bits; static struct clk *ac97_clk; static struct clk *ac97conf_clk; +static int is_ac97_resetgpio_95; +static int is_ac97_resetgpio_113;
/*
- Beware PXA27x bugs:
@@ -42,6 +44,55 @@ static struct clk *ac97conf_clk;
- 1 jiffy timeout if interrupt never comes).
*/
+enum {
- RESETGPIO_FORCE_HIGH,
- RESETGPIO_FORCE_LOW,
- RESETGPIO_NORMAL_ALTFUNC
+};
+/**
- set_resetgpio_mode - computes and sets the AC97_RESET gpio mode
on PXA
- @mode: chosen action
- As the PXA CPUs suffer from a AC97 bug, a manual control of the
reset
- line must be done to insure proper work of AC97 reset line.
- This function computes the correct gpio_mode for further use by
reset
- functions, and applied the change through pxa_gpio_mode.
- */
+static void set_resetgpio_mode(int resetgpio_action) +{
- int mode = 0;
- if (is_ac97_resetgpio_113)
switch (resetgpio_action) {
case RESETGPIO_FORCE_LOW:
mode = 113 | GPIO_OUT | GPIO_DFLT_LOW;
break;
case RESETGPIO_FORCE_HIGH:
mode = 113 | GPIO_OUT | GPIO_DFLT_HIGH;
break;
case RESETGPIO_NORMAL_ALTFUNC:
mode = 113 | GPIO_ALT_FN_2_OUT;
break;
};
- if (is_ac97_resetgpio_95)
switch (resetgpio_action) {
case RESETGPIO_FORCE_LOW:
mode = 95 | GPIO_OUT | GPIO_DFLT_LOW;
break;
case RESETGPIO_FORCE_HIGH:
mode = 95 | GPIO_OUT | GPIO_DFLT_HIGH;
break;
case RESETGPIO_NORMAL_ALTFUNC:
mode = 95 | GPIO_ALT_FN_1_OUT;
break;
};
- if (mode)
pxa_gpio_mode(mode);
+}
unsigned short pxa2xx_ac97_read(struct snd_ac97 *ac97, unsigned short reg) { unsigned short val = -1; @@ -137,10 +188,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
/* warm reset broken on Bulverde, so manually keep AC97 reset high */
- pxa_gpio_mode(113 | GPIO_OUT | GPIO_DFLT_HIGH);
- set_resetgpio_mode(RESETGPIO_FORCE_HIGH); udelay(10); GCR |= GCR_WARM_RST;
- pxa_gpio_mode(113 | GPIO_ALT_FN_2_OUT);
- set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC); udelay(500);
}
@@ -308,8 +359,8 @@ int pxa2xx_ac97_hw_resume(void) pxa_gpio_mode(GPIO29_SDATA_IN_AC97_MD); } if (cpu_is_pxa27x()) {
/* Use GPIO 113 as AC97 Reset on Bulverde */
pxa_gpio_mode(113 | GPIO_ALT_FN_2_OUT);
/* Use GPIO 113 or 95 as AC97 Reset on Bulverde */
} clk_enable(ac97_clk); return 0;set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
@@ -321,6 +372,13 @@ int __devinit pxa2xx_ac97_hw_probe(struct platform_device *dev) { int ret;
+#ifdef CONFIG_SND_PXA2XX_LIB_AC97_RESET113
- is_ac97_resetgpio_113 = 1;
+#endif +#ifdef CONFIG_SND_PXA2XX_LIB_AC97_RESET95
- is_ac97_resetgpio_95 = 1;
+#endif
- if (cpu_is_pxa25x() || cpu_is_pxa27x()) { pxa_gpio_mode(GPIO31_SYNC_AC97_MD); pxa_gpio_mode(GPIO30_SDATA_OUT_AC97_MD);
@@ -330,7 +388,7 @@ int __devinit pxa2xx_ac97_hw_probe(struct platform_device *dev)
if (cpu_is_pxa27x()) { /* Use GPIO 113 as AC97 Reset on Bulverde */
pxa_gpio_mode(113 | GPIO_ALT_FN_2_OUT);
set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC); ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK"); if (IS_ERR(ac97conf_clk)) { ret = PTR_ERR(ac97conf_clk);
-- 1.5.6.5

Mark Brown broonie@sirena.org.uk writes:
On 19 Feb 2009, at 20:44, Robert Jarzmik robert.jarzmik@free.fr wrote:
Hi Mark,
I can't find out if somebody had submitted that kind of patch before. This is a starting point of discussion. I'm not fully convinced by my own code, and so I post it here for advice.
What I'd really like is to be able to choose the PXA AC97 gpio reset (GPIO95 or GPIO113) through platform_data, but I couldn't find a way of doing it. I hope you'll have some kind of idea.
This is a sensible idea in principle but should be done via platform data for the AC97 driver rather than via Kconfig - it'll need an update to use the platform in ASoC but I'll need to do those anyway.
Would you like me to help you with this platform data. This GPIO thing is breaking mioa701 GSM chip (not damaging this time :)), so would you like me to cook up the platform data passed to pxa2xx-ac97, and amend my patch accordingly ?
Or have you already coded the beast in your working repository ?
Cheers.
-- Robert
PS: passing that platform data is easy, I should have done it right from the start. I can't see how I missed the simple platform_data in pxa2xx_ac97 probe function ... grrr... Must be the belgian food I tried.

On Sun, Mar 08, 2009 at 07:59:38PM +0100, Robert Jarzmik wrote:
Would you like me to help you with this platform data. This GPIO thing is breaking mioa701 GSM chip (not damaging this time :)), so would you like me to cook up the platform data passed to pxa2xx-ac97, and amend my patch accordingly
I've not done anything with this. I wasn't really intending to do the platform data bit myself, the driver should just default to using the standard setting when no platform data is supplied.
PS: passing that platform data is easy, I should have done it right from the
Well, yes.
start. I can't see how I missed the simple platform_data in pxa2xx_ac97 probe function ... grrr... Must be the belgian food I tried.
Note that at present ASoC does not use the platform device for the AC97 controller at all, it just starts talking to the hardware. That's the bit I was intending to deal with.

Mark Brown broonie@sirena.org.uk writes:
On Sun, Mar 08, 2009 at 07:31:38PM +0000, Mark Brown wrote:
Note that at present ASoC does not use the platform device for the AC97 controller at all, it just starts talking to the hardware. That's the bit I was intending to deal with.
Would you accept that patch instead then, on top of your for-2.6.30 branch ?
Cheers.
-- Robert

On Sat, Mar 14, 2009 at 10:27:41PM +0100, Robert Jarzmik wrote:
As the PXA series allow 2 gpios to reset the ac97 bus, allow through platform data configuration the definition of the correct gpio which will reset the AC97 bus.
This comes from a silicon defect on the PXA series, where the gpio must be manually controlled in warm reset cases.
The changelog should note that this only applies to PXA27x.
+/* AC97 platform_data */ +/**
- struct pxa2xx_ac97_platform_data - pxa ac97 platform data
- @reset_gpio_95: AC97 reset gpio is gpio95
- @reset_gpio_113: AC97 reset gpio is gpio113
- */
+struct pxa2xx_ac97_platform_data {
- unsigned reset_gpio_95:1;
- unsigned reset_gpio_113:1;
+};
Hrm. I'd rather just specify an int here in case we end up needing to cope with other CPUs later. The comments should also say that this data is only required for non-default pins to avoid worrying people with other boards. Parameter checking can reject values we can't cope with.
- As the PXA CPUs suffer from a AC97 bug, a manual control of the reset
- line must be done to insure proper work of AC97 reset line.
Again, this should specify that ths only applies to PXA27x.
- if (!pdata) {
is_ac97_resetgpio_113 = 1;
- } else {
if (pdata->reset_gpio_95)
is_ac97_resetgpio_95 = 1;
if (pdata->reset_gpio_113)
is_ac97_resetgpio_113 = 1;
- }
This should be something more like:
if (pdata) { switch (pdata->reset_gpio) { case 95: case 113: reset_gpio = pdata->reset_gpio; break; case 0: break; default: dev_err(dev, "Invalid reset GPIO %d\n", pdata->reset_gpio); } }
if (cpu_is_pxa27x() && !reset_gpio) reset_gpio = 113; probably also with some handling for not letting this be set on varaints that are OK. That way if we need more platform data for the device we can just zero initialise this field within the platform data.

Mark Brown broonie@sirena.org.uk writes:
<snip> All first comments OK.
Again, this should specify that ths only applies to PXA27x.
- if (!pdata) {
is_ac97_resetgpio_113 = 1;
- } else {
if (pdata->reset_gpio_95)
is_ac97_resetgpio_95 = 1;
if (pdata->reset_gpio_113)
is_ac97_resetgpio_113 = 1;
- }
This should be something more like:
if (pdata) { switch (pdata->reset_gpio) { case 95: case 113: reset_gpio = pdata->reset_gpio; break; case 0: break; default: dev_err(dev, "Invalid reset GPIO %d\n", pdata->reset_gpio); } }
if (cpu_is_pxa27x() && !reset_gpio) reset_gpio = 113;
Here I'll ask for a change.
You assume a reset gpio is _always_ provided on pxa27x CPUs. I do not. There may be faulty boards where no gpio line is dedicated to AC97 reset, or AC97 reset line is commanded by a helper chip.
Please check the new patch if you agree with the changes.
Cheers.
-- Robert

On Sun, Mar 15, 2009 at 12:14:18PM +0100, Robert Jarzmik wrote:
You assume a reset gpio is _always_ provided on pxa27x CPUs. I do not. There may be faulty boards where no gpio line is dedicated to AC97 reset, or AC97 reset line is commanded by a helper chip.
Could you change this to provide an explicit way of specifying that (eg, with -1 as the GPIO)? There are other things we might want platform data for in future - passing data to the AC97 codec, for example - so we can't rely on people not specifying platform data for anything.

Mark Brown broonie@sirena.org.uk writes:
Could you change this to provide an explicit way of specifying that (eg, with -1 as the GPIO)?
Sure.
There are other things we might want platform data for in future - passing data to the AC97 codec, for example - so we can't rely on people not specifying platform data for anything.
I don't understand the "anything". What should be done if pdata is passed with reset_gpio = 0 then ?
Should this be considered as the default case and transformed into 113 ? That sounds a bit buggy, because if someone changes his code to include platform_data support, they'll have to init platform_data anyway, and fill in reset_gpio field.
Should this be handled like a casual gpio (ie. gpio 99) ?
-- Robert

On 15 Mar 2009, at 12:36, Robert Jarzmik robert.jarzmik@free.fr wrote:
I don't understand the "anything". What should be done if pdata is passed with reset_gpio = 0 then ?
Should this be considered as the default case and transformed into 113 ? That sounds a bit buggy, because if someone changes his code to include platform_data support, they'll have to init platform_data anyway, and fill in reset_gpio field.
Yes, that's what I'm asking for. I don't want to make it mandatory because only PXA27x is affected (might be worth naming the field for that actually) and the driver will already be implementing a default behaviour.
Should this be handled like a casual gpio (ie. gpio 99) ?
I'm not sure what you mean by a casual GPIO?

Mark Brown broonie@sirena.org.uk writes:
Yes, that's what I'm asking for. I don't want to make it mandatory because only PXA27x is affected (might be worth naming the field for that actually) and the driver will already be implementing a default behaviour.
OK. It's just I'm not pretty convinced about the naming change. Feel free to change the patch if you want to.
Should this be handled like a casual gpio (ie. gpio 99) ?
I'm not sure what you mean by a casual GPIO?
I meant different from "special" gpios (ie. GPIO95 and GPIO113), which are "special" because they have an alternate function tied to AC97 reset.
Right, so would that one be better ?
Cheers.
-- Robert

On Sun, Mar 15, 2009 at 02:10:54PM +0100, Robert Jarzmik wrote:
Mark Brown broonie@sirena.org.uk writes:
Should this be handled like a casual gpio (ie. gpio 99) ?
I'm not sure what you mean by a casual GPIO?
I meant different from "special" gpios (ie. GPIO95 and GPIO113), which are "special" because they have an alternate function tied to AC97 reset.
Ah, I see - for reference, a more normal term would be something like generic. I didn't mean it was essential support them right now, just that it'd be better to allow for them in the interface in case we need the same workaround again. But yes, this looks good - I've applied it, thanks.
participants (2)
-
Mark Brown
-
Robert Jarzmik