diff mbox

[Resend] ARM: pxa27x: fix ac97 controller warm reset code

Message ID 1356728815-14410-1-git-send-email-mikedunn@newsguy.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Dunn Dec. 28, 2012, 9:06 p.m. UTC
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/pxa27x.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

--
1.7.8.6

Comments

Igor Grinberg Dec. 31, 2012, 9:56 a.m. UTC | #1
On 12/28/12 23:06, Mike Dunn wrote:
> 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.

Apparently, it is broken only on boards with no external pullups on these lines.

> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>  arch/arm/mach-pxa/pxa27x.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index 8047ee0..76221e4 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,
> +	MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH),
>  	GPIO113_AC97_nRESET,
> -	GPIO95_GPIO,
> +	MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH),

It was always discouraged to code the above directly.
How about one of the below:
1)
GPIO113_GPIO | MFP_PULL_HIGH,
GPIO95_GPIO  | MFP_PULL_HIGH,

or

2) introduce something like:
GPIO113_AC97_nRESET_GPIO_HIGH
GPIO95_AC97_nRESET_GPIO_HIGH
in the mfp-pxa27x.h file and use them in the structure above.


>  	GPIO95_AC97_nRESET,
>  };
> 
> --
> 1.7.8.6
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Mike Dunn Dec. 31, 2012, 9:04 p.m. UTC | #2
On 12/31/2012 01:56 AM, Igor Grinberg wrote:
> On 12/28/12 23:06, Mike Dunn wrote:


[..]


>>
>>  static unsigned long ac97_reset_config[] = {
>> -	GPIO113_GPIO,
>> +	MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH),
>>  	GPIO113_AC97_nRESET,
>> -	GPIO95_GPIO,
>> +	MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH),
> 
> It was always discouraged to code the above directly.


Had a funny feeling about that when I coded it...  I should have heeded the
feeling instead of being hasty.


> How about one of the below:
> 1)
> GPIO113_GPIO | MFP_PULL_HIGH,
> GPIO95_GPIO  | MFP_PULL_HIGH,
> 
> or
> 
> 2) introduce something like:
> GPIO113_AC97_nRESET_GPIO_HIGH
> GPIO95_AC97_nRESET_GPIO_HIGH
> in the mfp-pxa27x.h file and use them in the structure above.


OK, will implement your suggestion and resubmit.  To me, your first option seems
clearer to the casual reader.

Thanks Igor,
Mike
Igor Grinberg Jan. 1, 2013, 12:40 p.m. UTC | #3
On 12/31/12 23:04, Mike Dunn wrote:
> On 12/31/2012 01:56 AM, Igor Grinberg wrote:
>> On 12/28/12 23:06, Mike Dunn wrote:

[...]

>> How about one of the below:
>> 1)
>> GPIO113_GPIO | MFP_PULL_HIGH,
>> GPIO95_GPIO  | MFP_PULL_HIGH,
>>
>> or
>>
>> 2) introduce something like:
>> GPIO113_AC97_nRESET_GPIO_HIGH
>> GPIO95_AC97_nRESET_GPIO_HIGH
>> in the mfp-pxa27x.h file and use them in the structure above.
> 
> 
> OK, will implement your suggestion and resubmit.  To me, your first option seems
> clearer to the casual reader.

Actually, the second option is more robust, because you actually drive the pin.
diff mbox

Patch

diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 8047ee0..76221e4 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,
+	MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH),
 	GPIO113_AC97_nRESET,
-	GPIO95_GPIO,
+	MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH),
 	GPIO95_AC97_nRESET,
 };