[RFC/PATCH] pxa2xx_ac97 and gpio reset line

[RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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

>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 */
+		set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
 	}
 	clk_enable(ac97_clk);
 	return 0;
@@ -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

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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 */
> +        set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
>    }
>    clk_enable(ac97_clk);
>    return 0;
> @@ -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
>
>

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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.

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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.

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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.

...I also meant to say, hopefully tomorrow.

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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

>From ec5b385ddd3ec86f14310be1cba361307c4c1026 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 v2] Allow choice in ac97 gpio reset line

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.

Signed-off-by: Robert Jarzmik <rjarzmik@free.fr>
---
 include/sound/pxa2xx-lib.h  |   11 +++++++
 sound/arm/pxa2xx-ac97-lib.c |   71 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/include/sound/pxa2xx-lib.h b/include/sound/pxa2xx-lib.h
index 2fd3d25..01c2e4a 100644
--- a/include/sound/pxa2xx-lib.h
+++ b/include/sound/pxa2xx-lib.h
@@ -42,4 +42,15 @@ extern int pxa2xx_ac97_hw_resume(void);
 extern int pxa2xx_ac97_hw_probe(struct platform_device *dev);
 extern void pxa2xx_ac97_hw_remove(struct platform_device *dev);
 
+/* 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;
+};
+
 #endif
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 35afd0c..acf2e5a 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 */
+		set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
 	}
 	clk_enable(ac97_clk);
 	return 0;
@@ -320,6 +371,16 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_resume);
 int __devinit pxa2xx_ac97_hw_probe(struct platform_device *dev)
 {
 	int ret;
+	struct pxa2xx_ac97_platform_data *pdata = dev->dev.platform_data;
+
+	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;
+	}
 
 	if (cpu_is_pxa25x() || cpu_is_pxa27x()) {
 		pxa_gpio_mode(GPIO31_SYNC_AC97_MD);
@@ -330,7 +391,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

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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.

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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

>From 630a1b73898b1eeffc07062c4a6d4e845c33e022 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 v2] Allow choice in ac97 gpio reset line

As the PXA27x 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 PXA27x series, where
the gpio must be manually controlled in warm reset cases.

Signed-off-by: Robert Jarzmik <rjarzmik@free.fr>
---
 include/sound/pxa2xx-lib.h  |   14 +++++++++
 sound/arm/pxa2xx-ac97-lib.c |   68 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/include/sound/pxa2xx-lib.h b/include/sound/pxa2xx-lib.h
index 2fd3d25..9ee0f19 100644
--- a/include/sound/pxa2xx-lib.h
+++ b/include/sound/pxa2xx-lib.h
@@ -42,4 +42,18 @@ extern int pxa2xx_ac97_hw_resume(void);
 extern int pxa2xx_ac97_hw_probe(struct platform_device *dev);
 extern void pxa2xx_ac97_hw_remove(struct platform_device *dev);
 
+/* AC97 platform_data */
+/**
+ * struct pxa2xx_ac97_platform_data - pxa ac97 platform data
+ * @reset_gpio: AC97 reset gpio (normally gpio113 or gpio95)
+ *
+ * Platform data should only be specified for pxa27x CPUs where a silicon bug
+ * prevents correct operation of the reset line. If not specified, the default
+ * behaviour is to consider gpio 113 as the AC97 reset line, which is the
+ * default on most boards.
+ */
+struct pxa2xx_ac97_platform_data {
+	int reset_gpio;
+};
+
 #endif
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 35afd0c..1df4b0b 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -31,6 +31,7 @@ static DECLARE_WAIT_QUEUE_HEAD(gsr_wq);
 static volatile long gsr_bits;
 static struct clk *ac97_clk;
 static struct clk *ac97conf_clk;
+static int reset_gpio;
 
 /*
  * Beware PXA27x bugs:
@@ -42,6 +43,45 @@ 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 PXA27x 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 (reset_gpio)
+		switch (resetgpio_action) {
+		case RESETGPIO_NORMAL_ALTFUNC:
+			if (reset_gpio == 113)
+				mode = 113 | GPIO_OUT | GPIO_DFLT_LOW;
+			if (reset_gpio == 95)
+				mode = 95 | GPIO_ALT_FN_1_OUT;
+			break;
+		case RESETGPIO_FORCE_LOW:
+			mode = reset_gpio | GPIO_OUT | GPIO_DFLT_LOW;
+			break;
+		case RESETGPIO_FORCE_HIGH:
+			mode = reset_gpio | GPIO_OUT | GPIO_DFLT_HIGH;
+			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 +177,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 +348,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 */
+		set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
 	}
 	clk_enable(ac97_clk);
 	return 0;
@@ -320,6 +360,24 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_resume);
 int __devinit pxa2xx_ac97_hw_probe(struct platform_device *dev)
 {
 	int ret;
+	struct pxa2xx_ac97_platform_data *pdata = dev->dev.platform_data;
+
+	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);
+		}
+	} else {
+		if (cpu_is_pxa27x())
+			reset_gpio = 113;
+	}
 
 	if (cpu_is_pxa25x() || cpu_is_pxa27x()) {
 		pxa_gpio_mode(GPIO31_SYNC_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

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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.

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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?

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Robert Jarzmik]

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

>From 3fba866a0d339508576ef4bbb3f90a5a85235313 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 v3] Allow choice in ac97 gpio reset line

As the PXA27x 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 PXA27x series, where
the gpio must be manually controlled in warm reset cases.

Signed-off-by: Robert Jarzmik <rjarzmik@free.fr>
---
 include/sound/pxa2xx-lib.h  |   15 +++++++++
 sound/arm/pxa2xx-ac97-lib.c |   71 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/include/sound/pxa2xx-lib.h b/include/sound/pxa2xx-lib.h
index 2fd3d25..2c894b6 100644
--- a/include/sound/pxa2xx-lib.h
+++ b/include/sound/pxa2xx-lib.h
@@ -42,4 +42,19 @@ extern int pxa2xx_ac97_hw_resume(void);
 extern int pxa2xx_ac97_hw_probe(struct platform_device *dev);
 extern void pxa2xx_ac97_hw_remove(struct platform_device *dev);
 
+/* AC97 platform_data */
+/**
+ * struct pxa2xx_ac97_platform_data - pxa ac97 platform data
+ * @reset_gpio: AC97 reset gpio (normally gpio113 or gpio95)
+ *              a -1 value means no gpio will be used for reset
+ *
+ * Platform data should only be specified for pxa27x CPUs where a silicon bug
+ * prevents correct operation of the reset line. If not specified, the default
+ * behaviour is to consider gpio 113 as the AC97 reset line, which is the
+ * default on most boards.
+ */
+struct pxa2xx_ac97_platform_data {
+	int reset_gpio;
+};
+
 #endif
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 35afd0c..d721ea7 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -31,6 +31,7 @@ static DECLARE_WAIT_QUEUE_HEAD(gsr_wq);
 static volatile long gsr_bits;
 static struct clk *ac97_clk;
 static struct clk *ac97conf_clk;
+static int reset_gpio;
 
 /*
  * Beware PXA27x bugs:
@@ -42,6 +43,45 @@ 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 PXA27x 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 (reset_gpio)
+		switch (resetgpio_action) {
+		case RESETGPIO_NORMAL_ALTFUNC:
+			if (reset_gpio == 113)
+				mode = 113 | GPIO_OUT | GPIO_DFLT_LOW;
+			if (reset_gpio == 95)
+				mode = 95 | GPIO_ALT_FN_1_OUT;
+			break;
+		case RESETGPIO_FORCE_LOW:
+			mode = reset_gpio | GPIO_OUT | GPIO_DFLT_LOW;
+			break;
+		case RESETGPIO_FORCE_HIGH:
+			mode = reset_gpio | GPIO_OUT | GPIO_DFLT_HIGH;
+			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 +177,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 +348,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 */
+		set_resetgpio_mode(RESETGPIO_NORMAL_ALTFUNC);
 	}
 	clk_enable(ac97_clk);
 	return 0;
@@ -320,6 +360,27 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_resume);
 int __devinit pxa2xx_ac97_hw_probe(struct platform_device *dev)
 {
 	int ret;
+	struct pxa2xx_ac97_platform_data *pdata = dev->dev.platform_data;
+
+	if (pdata) {
+		switch (pdata->reset_gpio) {
+		case 95:
+		case 113:
+			reset_gpio = pdata->reset_gpio;
+			break;
+		case 0:
+			reset_gpio = 113;
+			break;
+		case -1:
+			break;
+		default:
+			dev_err(dev, "Invalid reset GPIO %d\n",
+				pdata->reset_gpio);
+		}
+	} else {
+		if (cpu_is_pxa27x())
+			reset_gpio = 113;
+	}
 
 	if (cpu_is_pxa25x() || cpu_is_pxa27x()) {
 		pxa_gpio_mode(GPIO31_SYNC_AC97_MD);
@@ -330,7 +391,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

Re: [RFC/PATCH] pxa2xx_ac97 and gpio reset line

[Mark Brown]

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.