[PATCH] ARM: PXA27x: fix workaround for AC97 reset

[PATCH] ARM: PXA27x: fix workaround for AC97 reset

[Igor Grinberg]

Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
A bug in the controller's warm reset functionality requires that the MFP
used by the controller as the AC97_RESET_n line be temporarily
reconfigured as a GPIO (AF0) and manually held high for the duration of
the warm reset cycle.

The workaround was broken long ago by commit fb1bf8cd
([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
The commit above changed the original workaround code in a way that
changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
This way, the GPIO state was left input (and undriven) and only worked
for boards with external pullup on the line.

Fix the above breakage by actually configurring the GPIO for output high
in case of reset assert and returning it to default state
(AF2 and GPDR - input) in case of reset deassert.

Reported-by: Mike Dunn <mikedunn@newsguy.com>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: stable at vger.kernel.org
---
Mike, this should work and without the GPIO not requested warning.
Can you test this, please?

 arch/arm/mach-pxa/devices.c |   13 ++++++++++++-
 arch/arm/mach-pxa/pxa27x.c  |    5 +++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index daa86d3..eaa7282 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c
@@ -5,6 +5,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/spi/pxa2xx_spi.h>
 #include <linux/i2c/pxa-i2c.h>
+#include <linux/gpio.h>
 
 #include <mach/udc.h>
 #include <linux/platform_data/usb-pxa3xx-ulpi.h>
@@ -488,7 +489,17 @@ struct platform_device pxa_device_ac97 = {
 
 void __init pxa_set_ac97_info(pxa2xx_audio_ops_t *ops)
 {
-	pxa_register_device(&pxa_device_ac97, ops);
+	int err = 0;
+
+	if (ops && gpio_is_valid(ops->reset_gpio)) {
+		err = gpio_request_one(ops->reset_gpio, GPIOF_IN, "ac97 rst");
+		if (err)
+			pr_err("%s: Failed requesting GPIO%d (ac97 rst): %d",
+			       __func__, ops->reset_gpio, err);
+	}
+
+	if (!err)
+		pxa_register_device(&pxa_device_ac97, ops);
 }
 
 #ifdef CONFIG_PXA25x
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 8047ee0..bb7e7c3 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -62,6 +62,11 @@ void pxa27x_assert_ac97reset(int reset_gpio, int on)
 	if (reset_gpio == 95)
 		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
 				       &ac97_reset_config[3], 1);
+
+	if (on)
+		gpio_direction_output(reset_gpio, 1);
+	else	/* to be on the safe side, return the GPDR to input */
+		gpio_direction_input(reset_gpio);
 }
 EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
 
-- 
1.7.3.4

[PATCH] ARM: PXA27x: fix workaround for AC97 reset

[Robert Jarzmik]

Igor Grinberg <grinberg@compulab.co.il> writes:

> Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
> A bug in the controller's warm reset functionality requires that the MFP
> used by the controller as the AC97_RESET_n line be temporarily
> reconfigured as a GPIO (AF0) and manually held high for the duration of
> the warm reset cycle.
>
> The workaround was broken long ago by commit fb1bf8cd
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
> The commit above changed the original workaround code in a way that
> changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
> This way, the GPIO state was left input (and undriven) and only worked
> for boards with external pullup on the line.
>
> Fix the above breakage by actually configurring the GPIO for output high
> in case of reset assert and returning it to default state
> (AF2 and GPDR - input) in case of reset deassert.

NAK.
If you return GPIO95 to AF-input, the GPIO95 will be "KP_DKIN<2>", ie. you'll
map a keyboard input to a AC97 reset.
As I said in a mail before, don't put a AC97 reset line as an input pin, this is
not a sensible patch.

Cheers.

--
Robert

[PATCH] ARM: PXA27x: fix workaround for AC97 reset

[Igor Grinberg]

On 01/06/13 16:07, Robert Jarzmik wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
> 
>> Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
>> A bug in the controller's warm reset functionality requires that the MFP
>> used by the controller as the AC97_RESET_n line be temporarily
>> reconfigured as a GPIO (AF0) and manually held high for the duration of
>> the warm reset cycle.
>>
>> The workaround was broken long ago by commit fb1bf8cd
>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
>> The commit above changed the original workaround code in a way that
>> changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
>> This way, the GPIO state was left input (and undriven) and only worked
>> for boards with external pullup on the line.
>>
>> Fix the above breakage by actually configurring the GPIO for output high
>> in case of reset assert and returning it to default state
>> (AF2 and GPDR - input) in case of reset deassert.
> 
> NAK.
> If you return GPIO95 to AF-input, the GPIO95 will be "KP_DKIN<2>", ie. you'll
> map a keyboard input to a AC97 reset.

I'm sorry, I don't understand what do you mean by "AF-input"?
For GPIO95 AF1 means AC97_nRESET and GPDR settings should not meter at all.
So setting GPDR for GPIO95/113 to input is only a safety measure.

> As I said in a mail before, don't put a AC97 reset line as an input pin, this is
> not a sensible patch.

Hmmm... I've opened the PXA27x dev manual once again in several years.
Ok. I was mislead by all the newer SoC where direction pins do not count
unless the MFP in AF-GPIO. As I've remembered now, it is not true for PXA27x.

This is what the manual says:
"Each pin can be programmed as an output, an input, or as bidirectional
for certain alternate functions (that override the value programmed in the
GPIO direction registers)."

So, you're right and GPDR does meter for AC97_nRESET.
I'll send a v2.

Thanks!

-- 
Regards,
Igor.

[PATCH v2] ARM: PXA27x: fix workaround for AC97 reset

[Igor Grinberg]

Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
A bug in the controller's warm reset functionality requires that the MFP
used by the controller as the AC97_RESET_n line be temporarily
reconfigured as a GPIO (AF0) and manually held high for the duration of
the warm reset cycle.

The workaround was broken long ago by commit fb1bf8cd
([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
The commit above changed the original workaround code in a way that
changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
This way, the GPIO state was left input (and undriven) and only worked
for boards with external pullup on the line.

Fix the above breakage by actually configurring the GPIO for output high
in case of reset assert and returning it to default state
(AF1 - for GPIO95 and AF2 - for GPIO113) in case of reset deassert.

Reported-by: Mike Dunn <mikedunn@newsguy.com>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
Cc: Mike Dunn <mikedunn@newsguy.com>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: stable at vger.kernel.org
---
v2:	Do not change the GPDR to input on reset deassert, as AC97
	controller will not drive it (thanks Robert).
	Also, this time really send to Mike.

Mike, Robert, can you test this one on boards with GPIO113 and GPIO95?
Thanks!

 arch/arm/mach-pxa/devices.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index daa86d3..03db2ce 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c
@@ -5,6 +5,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/spi/pxa2xx_spi.h>
 #include <linux/i2c/pxa-i2c.h>
+#include <linux/gpio.h>
 
 #include <mach/udc.h>
 #include <linux/platform_data/usb-pxa3xx-ulpi.h>
@@ -488,7 +489,18 @@ struct platform_device pxa_device_ac97 = {
 
 void __init pxa_set_ac97_info(pxa2xx_audio_ops_t *ops)
 {
-	pxa_register_device(&pxa_device_ac97, ops);
+	int err = 0;
+
+	if (ops && gpio_is_valid(ops->reset_gpio)) {
+		err = gpio_request_one(ops->reset_gpio, GPIOF_INIT_HIGH,
+				       "ac97 rst");
+		if (err)
+			pr_err("%s: Failed requesting GPIO%d (ac97 rst): %d",
+			       __func__, ops->reset_gpio, err);
+	}
+
+	if (!err)
+		pxa_register_device(&pxa_device_ac97, ops);
 }
 
 #ifdef CONFIG_PXA25x
-- 
1.7.3.4

[PATCH v2] ARM: PXA27x: fix workaround for AC97 reset

[Mike Dunn]

Whoops, I just submitted some patches that clash with this.  Sorry, didn't see
this one.  I'll test this, and will resubmit my patches to work with this one if
necessary.  Your code placement is probably better.

Thanks,
Mike


On 01/06/2013 08:48 AM, Igor Grinberg wrote:
> Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
> A bug in the controller's warm reset functionality requires that the MFP
> used by the controller as the AC97_RESET_n line be temporarily
> reconfigured as a GPIO (AF0) and manually held high for the duration of
> the warm reset cycle.
> 
> The workaround was broken long ago by commit fb1bf8cd
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
> The commit above changed the original workaround code in a way that
> changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
> This way, the GPIO state was left input (and undriven) and only worked
> for boards with external pullup on the line.
> 
> Fix the above breakage by actually configurring the GPIO for output high
> in case of reset assert and returning it to default state
> (AF1 - for GPIO95 and AF2 - for GPIO113) in case of reset deassert.
> 
> Reported-by: Mike Dunn <mikedunn@newsguy.com>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Mike Dunn <mikedunn@newsguy.com>
> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: stable at vger.kernel.org
> ---
> v2:	Do not change the GPDR to input on reset deassert, as AC97
> 	controller will not drive it (thanks Robert).
> 	Also, this time really send to Mike.
> 
> Mike, Robert, can you test this one on boards with GPIO113 and GPIO95?
> Thanks!
> 
>  arch/arm/mach-pxa/devices.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
> index daa86d3..03db2ce 100644
> --- a/arch/arm/mach-pxa/devices.c
> +++ b/arch/arm/mach-pxa/devices.c
> @@ -5,6 +5,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/spi/pxa2xx_spi.h>
>  #include <linux/i2c/pxa-i2c.h>
> +#include <linux/gpio.h>
>  
>  #include <mach/udc.h>
>  #include <linux/platform_data/usb-pxa3xx-ulpi.h>
> @@ -488,7 +489,18 @@ struct platform_device pxa_device_ac97 = {
>  
>  void __init pxa_set_ac97_info(pxa2xx_audio_ops_t *ops)
>  {
> -	pxa_register_device(&pxa_device_ac97, ops);
> +	int err = 0;
> +
> +	if (ops && gpio_is_valid(ops->reset_gpio)) {
> +		err = gpio_request_one(ops->reset_gpio, GPIOF_INIT_HIGH,
> +				       "ac97 rst");
> +		if (err)
> +			pr_err("%s: Failed requesting GPIO%d (ac97 rst): %d",
> +			       __func__, ops->reset_gpio, err);
> +	}
> +
> +	if (!err)
> +		pxa_register_device(&pxa_device_ac97, ops);
>  }
>  
>  #ifdef CONFIG_PXA25x

[PATCH v2] ARM: PXA27x: fix workaround for AC97 reset

[Mike Dunn]

On 01/06/2013 08:48 AM, Igor Grinberg wrote:
> Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
> A bug in the controller's warm reset functionality requires that the MFP
> used by the controller as the AC97_RESET_n line be temporarily
> reconfigured as a GPIO (AF0) and manually held high for the duration of
> the warm reset cycle.
> 
> The workaround was broken long ago by commit fb1bf8cd
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
> The commit above changed the original workaround code in a way that
> changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
> This way, the GPIO state was left input (and undriven) and only worked
> for boards with external pullup on the line.
> 
> Fix the above breakage by actually configurring the GPIO for output high
> in case of reset assert and returning it to default state
> (AF1 - for GPIO95 and AF2 - for GPIO113) in case of reset deassert.


Hi Igor,

I pulled patches 2, 3, and 4 from my patch set (patch #1 fixes an unrelated
problem with ac97 cold reset) and replaced it with this, and warm reset works.
My pxa270 uses gpio 95 for reset, BTW.  I don't have a platform that uses 113.

Honestly, I don't understand why it works :)  The pxa27x_assert_ac97reset()
function calls pxa2xx_mfp_config() with an input direction configuration when it
switches to gpio (AF0), so I don't see how the line remains an output driven
high.  I think you tried to explain that in the email exchange, but I'm still
confused.  I'm looking at __mfp_config_gpio(), and it looks like it's setting
the GPDR register.

[..]


>  
>  void __init pxa_set_ac97_info(pxa2xx_audio_ops_t *ops)
>  {
> -	pxa_register_device(&pxa_device_ac97, ops);
> +	int err = 0;
> +
> +	if (ops && gpio_is_valid(ops->reset_gpio)) {
> +		err = gpio_request_one(ops->reset_gpio, GPIOF_INIT_HIGH,
> +				       "ac97 rst");
> +		if (err)
> +			pr_err("%s: Failed requesting GPIO%d (ac97 rst): %d",
> +			       __func__, ops->reset_gpio, err);
> +	}
> +
> +	if (!err)
> +		pxa_register_device(&pxa_device_ac97, ops);
>  }
>  
>  #ifdef CONFIG_PXA25x


On further thought, I'm not sure about doing this here.  This file is for all
pxa's, no?  The gpio only needs to be obtained for pxa270.  And even in that
case, the ac97 controller peripheral may not be used if no codec is attached, in
which case the pins are free to be used for any kind of gpio.  It might be
better to have the ac97 driver request the gpio.

And a more general complaint: the functionality is very obscure.  (Heck, I still
can't figure it out! :)

Thanks Igor,
Mike

[PATCH v2] ARM: PXA27x: fix workaround for AC97 reset

[Igor Grinberg]

On 01/07/13 00:19, Mike Dunn wrote:
> On 01/06/2013 08:48 AM, Igor Grinberg wrote:
>> Fix the workaround to a hardware bug in the AC97 controller of PXA27x.
>> A bug in the controller's warm reset functionality requires that the MFP
>> used by the controller as the AC97_RESET_n line be temporarily
>> reconfigured as a GPIO (AF0) and manually held high for the duration of
>> the warm reset cycle.
>>
>> The workaround was broken long ago by commit fb1bf8cd
>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset()).
>> The commit above changed the original workaround code in a way that
>> changed the MFP to AF0 (GPIO), but forgot to drive the GPIO high output.
>> This way, the GPIO state was left input (and undriven) and only worked
>> for boards with external pullup on the line.
>>
>> Fix the above breakage by actually configurring the GPIO for output high
>> in case of reset assert and returning it to default state
>> (AF1 - for GPIO95 and AF2 - for GPIO113) in case of reset deassert.
> 
> 
> Hi Igor,
> 
> I pulled patches 2, 3, and 4 from my patch set (patch #1 fixes an unrelated
> problem with ac97 cold reset) and replaced it with this, and warm reset works.
> My pxa270 uses gpio 95 for reset, BTW.  I don't have a platform that uses 113.

Thanks!

> 
> Honestly, I don't understand why it works :)  The pxa27x_assert_ac97reset()
> function calls pxa2xx_mfp_config() with an input direction configuration when it
> switches to gpio (AF0), so I don't see how the line remains an output driven
> high.  I think you tried to explain that in the email exchange, but I'm still
> confused.  I'm looking at __mfp_config_gpio(), and it looks like it's setting
> the GPDR register.

Yeah, I see your concern now...
Probably, we still need a combination of both: like in v1 of my patch,
just without changing the direction to input.

> 
> [..]
> 
> 
>>  
>>  void __init pxa_set_ac97_info(pxa2xx_audio_ops_t *ops)
>>  {
>> -	pxa_register_device(&pxa_device_ac97, ops);
>> +	int err = 0;
>> +
>> +	if (ops && gpio_is_valid(ops->reset_gpio)) {
>> +		err = gpio_request_one(ops->reset_gpio, GPIOF_INIT_HIGH,
>> +				       "ac97 rst");
>> +		if (err)
>> +			pr_err("%s: Failed requesting GPIO%d (ac97 rst): %d",
>> +			       __func__, ops->reset_gpio, err);
>> +	}
>> +
>> +	if (!err)
>> +		pxa_register_device(&pxa_device_ac97, ops);
>>  }
>>  
>>  #ifdef CONFIG_PXA25x
> 
> 
> On further thought, I'm not sure about doing this here.  This file is for all
> pxa's, no?  The gpio only needs to be obtained for pxa270. 

The only problem I can see here, is that the ops->reset_gpio
may be uninitialized ( = 0 ) and that will be a problem as 0 is a valid GPIO,
so there are several options here:
1) Change all the users of pxa_set_ac97_info() that provide non-NULL ops pointer
   to initialize the reset_gpio to -EINVAL (preferable)
2) We can check cpu_is_pxa27x().
3) We can check that provided GPIO is 95 or 113.

> And even in that
> case, the ac97 controller peripheral may not be used if no codec is attached, in
> which case the pins are free to be used for any kind of gpio. 

In such case you should not call pxa_set_ac97_info(), right?

> It might be
> better to have the ac97 driver request the gpio.

Thought about that, but this means we request the GPIO in the driver but
configure it (gpio_direction_out()) in the arch code - this is a bit awkward.

I would go for implementing 1), unless we have no problem requesting the GPIO
in the driver and driving it in the arch code...

-- 
Regards,
Igor.