[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Mike Dunn]

Change log:
v2: Heed Igor's admonishment to define a macro instead of using MFP_CFG_OUT
    directly.  Thanks.

This patch fixes some code that implements a work-around to a hardware bug in
the ac97 controller on the 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 generic output gpio (AF0) and manually
held high for the duration of the warm reset cycle.  This is what was done in
the original code, but it was broken long ago by commit
fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
which changed the mfp to a GPIO input instead of a high output.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
 arch/arm/mach-pxa/pxa27x.c                  |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
index a611ad3..8281e17 100644
--- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
+++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
@@ -463,6 +463,9 @@
 	GPIO76_LCD_PCLK,	\
 	GPIO77_LCD_BIAS
 
+/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
+#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
+#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
 
 extern int keypad_set_wake(unsigned int on);
 #endif /* __ASM_ARCH_MFP_PXA27X_H */
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 8047ee0..616cb87 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -47,9 +47,9 @@ void pxa27x_clear_otgph(void)
 EXPORT_SYMBOL(pxa27x_clear_otgph);
 
 static unsigned long ac97_reset_config[] = {
-	GPIO113_GPIO,
+	GPIO113_AC97_nRESET_GPIO_HIGH,
 	GPIO113_AC97_nRESET,
-	GPIO95_GPIO,
+	GPIO95_AC97_nRESET_GPIO_HIGH,
 	GPIO95_AC97_nRESET,
 };
 
-- 
1.7.8.6

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Igor Grinberg]

On 01/03/13 16:39, Mike Dunn wrote:
> Change log:
> v2: Heed Igor's admonishment to define a macro instead of using MFP_CFG_OUT
>     directly.  Thanks.

Usually, the change log is not useful after applying the patch, so
it should go below the scissors line ("---").

> 
> This patch fixes some code that implements a work-around to a hardware bug in
> the ac97 controller on the 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 generic output gpio (AF0) and manually
> held high for the duration of the warm reset cycle.  This is what was done in
> the original code, but it was broken long ago by commit
> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
> which changed the mfp to a GPIO input instead of a high output.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

Thanks!

Acked-by: Igor Grinberg <grinberg@compulab.co.il>

> ---
>  arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
>  arch/arm/mach-pxa/pxa27x.c                  |    4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> index a611ad3..8281e17 100644
> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> @@ -463,6 +463,9 @@
>  	GPIO76_LCD_PCLK,	\
>  	GPIO77_LCD_BIAS
>  
> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
>  
>  extern int keypad_set_wake(unsigned int on);
>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index 8047ee0..616cb87 100644
> --- a/arch/arm/mach-pxa/pxa27x.c
> +++ b/arch/arm/mach-pxa/pxa27x.c
> @@ -47,9 +47,9 @@ void pxa27x_clear_otgph(void)
>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>  
>  static unsigned long ac97_reset_config[] = {
> -	GPIO113_GPIO,
> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>  	GPIO113_AC97_nRESET,
> -	GPIO95_GPIO,
> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>  	GPIO95_AC97_nRESET,
>  };
>  

-- 
Regards,
Igor.

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Igor Grinberg]

On 01/04/13 08:50, Igor Grinberg wrote:
> On 01/03/13 16:39, Mike Dunn wrote:
>> Change log:
>> v2: Heed Igor's admonishment to define a macro instead of using MFP_CFG_OUT
>>     directly.  Thanks.
> 
> Usually, the change log is not useful after applying the patch, so
> it should go below the scissors line ("---").
> 
>>
>> This patch fixes some code that implements a work-around to a hardware bug in
>> the ac97 controller on the 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 generic output gpio (AF0) and manually
>> held high for the duration of the warm reset cycle.  This is what was done in
>> the original code, but it was broken long ago by commit
>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>> which changed the mfp to a GPIO input instead of a high output.

Looking at this one more time...
fb1bf8cd13 ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
also removed the call to pxa_gpio_mode() function which effectively set
AF to GPIO and configured the GPIO to output high.
(Later b1d9bf1d ([ARM] pxa: remove pxa_gpio_mode() and files) removed the
pxa_gpio_mode() function)

See below...

>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> 
> Thanks!
> 
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> 
>> ---
>>  arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
>>  arch/arm/mach-pxa/pxa27x.c                  |    4 ++--
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> index a611ad3..8281e17 100644
>> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> @@ -463,6 +463,9 @@
>>  	GPIO76_LCD_PCLK,	\
>>  	GPIO77_LCD_BIAS
>>  
>> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
>> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
>> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)

The DRIVE_HIGH does not really configures the GPIO to output high, but
only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
low power modes.
This means, that by doing the above, you just configure the MFP for GPIO output,
but do not assign it a value, so it gets driven with some undefined value.
This is not safe.

Can you please, check if the attached patch below does the job?

>>  
>>  extern int keypad_set_wake(unsigned int on);
>>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
>> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
>> index 8047ee0..616cb87 100644
>> --- a/arch/arm/mach-pxa/pxa27x.c
>> +++ b/arch/arm/mach-pxa/pxa27x.c
>> @@ -47,9 +47,9 @@ void pxa27x_clear_otgph(void)
>>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>>  
>>  static unsigned long ac97_reset_config[] = {
>> -	GPIO113_GPIO,
>> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>>  	GPIO113_AC97_nRESET,
>> -	GPIO95_GPIO,
>> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>>  	GPIO95_AC97_nRESET,
>>  };
>>  
> 

-- 
Regards,
Igor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pxa-ac97-reset-fix.patch
Type: text/x-patch
Size: 497 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130104/8826e3af/attachment-0001.bin>

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Mike Dunn]

On 01/04/2013 02:31 AM, Igor Grinberg wrote:

[..]


> Looking at this one more time...
> fb1bf8cd13 ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
> also removed the call to pxa_gpio_mode() function which effectively set
> AF to GPIO and configured the GPIO to output high.
> (Later b1d9bf1d ([ARM] pxa: remove pxa_gpio_mode() and files) removed the
> pxa_gpio_mode() function)
> 
> See below...

>
>>>
>>>  extern int keypad_set_wake(unsigned int on);
>>>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
>>> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
>>> index 8047ee0..616cb87 100644
>>> --- a/arch/arm/mach-pxa/pxa27x.c
>>> +++ b/arch/arm/mach-pxa/pxa27x.c
>>> @@ -47,9 +47,9 @@ void pxa27x_clear_otgph(void)
>>>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>>>
>>>  static unsigned long ac97_reset_config[] = {
>>> -	GPIO113_GPIO,
>>> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>>>  	GPIO113_AC97_nRESET,
>>> -	GPIO95_GPIO,
>>> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>>>  	GPIO95_AC97_nRESET,
>>>  };
>>>
>>
>


>
>>>
>>>  extern int keypad_set_wake(unsigned int on);
>>>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
>>> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
>>> index 8047ee0..616cb87 100644
>>> --- a/arch/arm/mach-pxa/pxa27x.c
>>> +++ b/arch/arm/mach-pxa/pxa27x.c
>>> @@ -47,9 +47,9 @@ void pxa27x_clear_otgph(void)
>>>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>>>
>>>  static unsigned long ac97_reset_config[] = {
>>> -	GPIO113_GPIO,
>>> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>>>  	GPIO113_AC97_nRESET,
>>> -	GPIO95_GPIO,
>>> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>>>  	GPIO95_AC97_nRESET,
>>>  };
>>>
>>
>



[..]


> The DRIVE_HIGH does not really configures the GPIO to output high, but
> only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
> low power modes.
> This means, that by doing the above, you just configure the MFP for GPIO output,
> but do not assign it a value, so it gets driven with some undefined value.
> This is not safe.
> 
> Can you please, check if the attached patch below does the job?	


Yes, it does! (With a warning from gpiolib becaus gpio_request() was not called).

In a different patch, I skip the ac97 warm reset if the link is up (which it
always is when called), so honestly, I didn't functionally test my patch.
Normally I wouldn't submit a patch without testing, but clearly the code was
already broken anyway, and the change looked straightforward (danger!).

With your patch and my other patch removed, the warm reset does not fail, as before.

I am in your debt!  I will submit a new version of this patch soon.

Thanks again,
Mike

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Robert Jarzmik]

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

> On 01/04/13 08:50, Igor Grinberg wrote:
>> On 01/03/13 16:39, Mike Dunn wrote:
>>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
>>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>>> which changed the mfp to a GPIO input instead of a high output.
>
> Looking at this one more time...
> fb1bf8cd13 ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
> also removed the call to pxa_gpio_mode() function which effectively set
> AF to GPIO and configured the GPIO to output high.
> (Later b1d9bf1d ([ARM] pxa: remove pxa_gpio_mode() and files) removed the
> pxa_gpio_mode() function)
>
> See below...
> The DRIVE_HIGH does not really configures the GPIO to output high, but
> only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
> low power modes.
> This means, that by doing the above, you just configure the MFP for GPIO output,
> but do not assign it a value, so it gets driven with some undefined value.
> This is not safe.
>
> Can you please, check if the attached patch below does the job?

This is not the original behaviour before commit
fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.

The original behaviour was :
 - on = 1 => set GPIO as output GPIO, set to 1
 - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
 IP.

If you don't set the alternate function, the GCR register usage for reset is
useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?

And a question for Mike : when you made your tests, was it you intended
behaviour to never set ac97 alternate function and use direct output GPIO
setting with gpio_set_value(, X) ? Or am I missing something here ?

Cheers.

--
Robert

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Mike Dunn]

On 01/04/2013 12:34 PM, Robert Jarzmik wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
>>
>> See below...
>> The DRIVE_HIGH does not really configures the GPIO to output high, but
>> only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
>> low power modes.
>> This means, that by doing the above, you just configure the MFP for GPIO output,
>> but do not assign it a value, so it gets driven with some undefined value.
>> This is not safe.
>>
>> Can you please, check if the attached patch below does the job?
> 
> This is not the original behaviour before commit
> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
> 
> The original behaviour was :
>  - on = 1 => set GPIO as output GPIO, set to 1
>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>  IP.
> 
> If you don't set the alternate function, the GCR register usage for reset is
> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?
> 
> And a question for Mike : when you made your tests, was it you intended
> behaviour to never set ac97 alternate function and use direct output GPIO
> setting with gpio_set_value(, X) ? Or am I missing something here ?


The original behavior: at the start of warm reset, reconfigure the mfp from alt
fn AC97_RESET_n to generic gpio output, driven high.  Then, when warm reset
completes, change the pin back to the ac97 alt fn.  As Igor pointed out, my
patch just reconfigured the pin as an generic output gpio, but did not actually
drive it high.

The unfortunate name and prototype given to the function
pxa27x_assert_ac97reset() by the commit back in 2010 does not help clarify things.

Am *I* missing anything?  I'm still looking into this, but with Igor's suggested
patch, warm reset no longer fails.

Thanks,
Mike

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Igor Grinberg]

On 01/04/13 22:34, Robert Jarzmik wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
> 
>> On 01/04/13 08:50, Igor Grinberg wrote:
>>> On 01/03/13 16:39, Mike Dunn wrote:
>>>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
>>>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>>>> which changed the mfp to a GPIO input instead of a high output.
>>
>> Looking at this one more time...
>> fb1bf8cd13 ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>> also removed the call to pxa_gpio_mode() function which effectively set
>> AF to GPIO and configured the GPIO to output high.
>> (Later b1d9bf1d ([ARM] pxa: remove pxa_gpio_mode() and files) removed the
>> pxa_gpio_mode() function)
>>
>> See below...
>> The DRIVE_HIGH does not really configures the GPIO to output high, but
>> only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
>> low power modes.
>> This means, that by doing the above, you just configure the MFP for GPIO output,
>> but do not assign it a value, so it gets driven with some undefined value.
>> This is not safe.
>>
>> Can you please, check if the attached patch below does the job?
> 
> This is not the original behaviour before commit
> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
> 
> The original behaviour was :
>  - on = 1 => set GPIO as output GPIO, set to 1
>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>  IP.
> 
> If you don't set the alternate function, the GCR register usage for reset is
> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?

Well, I've made a quick patch for Mike to test if this works and
since it works I will submit a proper one.

To your question about setting the direction,
I'd like us to be on a safe side and not drive the pin if AF is not GPIO.
Although it should not meter and changing the AF to ac97reset should do the job,
but just to be on the safe side, as I think GPDR/GPCR/GPSR settings are preserved
even if you change the AF to something other than GPIO.


-- 
Regards,
Igor.

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Robert Jarzmik]

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

> On 01/04/13 22:34, Robert Jarzmik wrote:
>> This is not the original behaviour before commit
>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
>> 
>> The original behaviour was :
>>  - on = 1 => set GPIO as output GPIO, set to 1
>>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>>  IP.
>> 
>> If you don't set the alternate function, the GCR register usage for reset is
>> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?
>
> Well, I've made a quick patch for Mike to test if this works and
> since it works I will submit a proper one.
Mmm...

> To your question about setting the direction,
> I'd like us to be on a safe side and not drive the pin if AF is not GPIO.
> Although it should not meter and changing the AF to ac97reset should do the job,
> but just to be on the safe side, as I think GPDR/GPCR/GPSR settings are preserved
> even if you change the AF to something other than GPIO.

I think I was not clear enough.

My point is that if you program the GPIO as input, then the PXA will not drive
the pin anymore. If you have a resistor doing the "pullup", everything will work
(as I assume is the case in your board). If not, the AC97 codec (wm9713 and
co...) could have its reset line asserted (ie. #RESET line), and *this* bothers me.

So changing the AC97 #RESET line to "GPIO input" is incorrect, and I don't agree
with the patch.

Cheers.

-- 
Robert

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Igor Grinberg]

On 01/06/13 16:00, Robert Jarzmik wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
> 
>> On 01/04/13 22:34, Robert Jarzmik wrote:
>>> This is not the original behaviour before commit
>>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
>>>
>>> The original behaviour was :
>>>  - on = 1 => set GPIO as output GPIO, set to 1
>>>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>>>  IP.
>>>
>>> If you don't set the alternate function, the GCR register usage for reset is
>>> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?
>>
>> Well, I've made a quick patch for Mike to test if this works and
>> since it works I will submit a proper one.
> Mmm...
> 
>> To your question about setting the direction,
>> I'd like us to be on a safe side and not drive the pin if AF is not GPIO.
>> Although it should not meter and changing the AF to ac97reset should do the job,
>> but just to be on the safe side, as I think GPDR/GPCR/GPSR settings are preserved
>> even if you change the AF to something other than GPIO.
> 
> I think I was not clear enough.

Definitely...

> 
> My point is that if you program the GPIO as input, then the PXA will not drive
> the pin anymore.

Not exactly. The MFP will not be driven by the GPIO subsystem,
but it will be driven by the AC97 controller, as the for
GPIO95 - AF1 means AC97_nRESET and for GPIO113 - AF2 also means AC97_nRESET.
No problem here - it is driven as it used to be in the original code.


> If you have a resistor doing the "pullup", everything will work
> (as I assume is the case in your board). If not, the AC97 codec (wm9713 and
> co...) could have its reset line asserted (ie. #RESET line), and *this* bothers me.

That is what the original workaround did.
Should not bother you, instead, can you test it?

> 
> So changing the AC97 #RESET line to "GPIO input" is incorrect, and I don't agree
> with the patch.

It is not incorrect, it just does not have any effect. Please, test it.

-- 
Regards,
Igor.

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Mike Dunn]

On 01/06/2013 06:00 AM, Robert Jarzmik wrote:
> Igor Grinberg <grinberg@compulab.co.il> writes:
> 
>> On 01/04/13 22:34, Robert Jarzmik wrote:
>>> This is not the original behaviour before commit
>>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
>>>
>>> The original behaviour was :
>>>  - on = 1 => set GPIO as output GPIO, set to 1
>>>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>>>  IP.
>>>
>>> If you don't set the alternate function, the GCR register usage for reset is
>>> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?
>>
>> Well, I've made a quick patch for Mike to test if this works and
>> since it works I will submit a proper one.
> Mmm...
> 
>> To your question about setting the direction,
>> I'd like us to be on a safe side and not drive the pin if AF is not GPIO.
>> Although it should not meter and changing the AF to ac97reset should do the job,
>> but just to be on the safe side, as I think GPDR/GPCR/GPSR settings are preserved
>> even if you change the AF to something other than GPIO.
> 
> I think I was not clear enough.
> 
> My point is that if you program the GPIO as input, then the PXA will not drive
> the pin anymore. If you have a resistor doing the "pullup", everything will work
> (as I assume is the case in your board). If not, the AC97 codec (wm9713 and
> co...) could have its reset line asserted (ie. #RESET line), and *this* bothers me.
> 
> So changing the AC97 #RESET line to "GPIO input" is incorrect, and I don't agree
> with the patch.


I believe the extra lines in Igor's patch just effectively set the direction but
not the alt fn setting, which shouldn't do anything after the pin was configured
for the alt fn.  But in fact, I skipped those lines in the patch I prepared.  I
was too happy about discovering the reason warm reset continued to fail to
quibble :) Thanks again Igor.

Another change I made to Igor's patch is to drive the line (effectively set GPSR
bit) before switching from the ac97_reset alt fn to gpio / AF0.  Will CC you on
the patches.  They work well; warm and cold reset.  Yay!

Mike

[PATCH v2] ARM: pxa27x: fix ac97 controller warm reset code

[Mike Dunn]

On 01/05/2013 11:11 PM, Igor Grinberg wrote:
> On 01/04/13 22:34, Robert Jarzmik wrote:
>> Igor Grinberg <grinberg@compulab.co.il> writes:
>>
>>> On 01/04/13 08:50, Igor Grinberg wrote:
>>>> On 01/03/13 16:39, Mike Dunn wrote:
>>>>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
>>>>> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>>>>> which changed the mfp to a GPIO input instead of a high output.
>>>
>>> Looking at this one more time...
>>> fb1bf8cd13 ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
>>> also removed the call to pxa_gpio_mode() function which effectively set
>>> AF to GPIO and configured the GPIO to output high.
>>> (Later b1d9bf1d ([ARM] pxa: remove pxa_gpio_mode() and files) removed the
>>> pxa_gpio_mode() function)
>>>
>>> See below...
>>> The DRIVE_HIGH does not really configures the GPIO to output high, but
>>> only sets the MFP_LPM_DRIVE_HIGH bit which in turn is only effective in
>>> low power modes.
>>> This means, that by doing the above, you just configure the MFP for GPIO output,
>>> but do not assign it a value, so it gets driven with some undefined value.
>>> This is not safe.
>>>
>>> Can you please, check if the attached patch below does the job?
>>
>> This is not the original behaviour before commit
>> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6.
>>
>> The original behaviour was :
>>  - on = 1 => set GPIO as output GPIO, set to 1
>>  - on = 0 => set GPIO to the alternate function ac97reset, driven by PXA2xx AC97
>>  IP.
>>
>> If you don't set the alternate function, the GCR register usage for reset is
>> useless, isn't it ? So why do you set the GPIO as "input" with on == 0 ?
> 
> Well, I've made a quick patch for Mike to test if this works and
> since it works I will submit a proper one.
> 
> To your question about setting the direction,
> I'd like us to be on a safe side and not drive the pin if AF is not GPIO.


I just finished testing a patch that only sets the level and direction if
switching from the ac97 alt fn to gpio, and it works fine.


> Although it should not meter and changing the AF to ac97reset should do the job,
> but just to be on the safe side, as I think GPDR/GPCR/GPSR settings are preserved
> even if you change the AF to something other than GPIO.


Yes, I think this is the case, but for simplicity my patch sets GPDR and GPSR
every time the function is called.

Thanks,
Mike