[PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Mike Dunn]

Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
by setting the WARM_RST bit in the GCR register at the end of the cold reset,
and then skipping the warm reset if the link is already up and running.

It appears that setting the WARM_RST bit is a necessary final step during cold
reset.  I think that the PXA25x and PXA3xx may currently be working correctly
because WARM_RST is set within the warm reset routine, and the codec drivers
always follow a cold reset with a warm reset during their initialization. so
this combination effectively completes a cold reset.  This doesn't work on the
PXA27x because its warm reset routine contains additional code for working
around a hardware bug in the warm reset sequence, which causes the reset
sequence to fail.  I only have a PXA27x platform for testing, so this patch only
affects the PXA27x.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

As an aside... I don't understand how always following a cold reset with a warm
reset (as the codec drivers do) makes sense.  Unless I'm mistaken, a warm reset
can only occur on a link that's currently down.  All the timing diagrams I've
seen for reset (warm and cold) show the link down (BITCLK stopped) at the start
of the reset sequence.  The SYNC line is used to effect the warm reset, and SYNC
is already pulsing every frame on a link that's up and running, so it seems
physically impossible to do a warm reset on a link that's currently up.  But I'm
no ac97 expert, so I may just be embarassing myself.  Any insight gratefully
received.  Thanks!

 sound/arm/pxa2xx-ac97-lib.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 6fc0ae9..aa242ff 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -138,6 +138,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
 {
 	gsr_bits = 0;
 
+	/* don't attempt warm reset if link is up */
+	if (GSR & (GSR_SCR | GSR_PCR))
+		return;
+
 	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
 	pxa27x_assert_ac97reset(reset_gpio, 1);
 	udelay(10);
@@ -148,6 +152,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
 
 static inline void pxa_ac97_cold_pxa27x(void)
 {
+	unsigned int timeout;
+
 	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
 	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
 
@@ -159,6 +165,15 @@ static inline void pxa_ac97_cold_pxa27x(void)
 	clk_disable(ac97conf_clk);
 	GCR = GCR_COLD_RST;
 	udelay(50);
+
+	/*
+	 * Setting the WARM_RST bit is a necessary part of a cold reset, at
+	 * least on the buggy and poorly documented PXA27x ac97 controller.
+	 */
+	GCR |= GCR_WARM_RST;
+	timeout = 100;     /* wait for the codec-ready bit to be set */
+	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
+		mdelay(1);
 }
 #endif
 
-- 
1.7.8.6

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

> Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
> developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
> by setting the WARM_RST bit in the GCR register at the end of the cold reset,
> and then skipping the warm reset if the link is already up and running.
>
> It appears that setting the WARM_RST bit is a necessary final step during cold
> reset.  I think that the PXA25x and PXA3xx may currently be working correctly
> because WARM_RST is set within the warm reset routine, and the codec drivers
> always follow a cold reset with a warm reset during their initialization. so
> this combination effectively completes a cold reset.  This doesn't work on the
> PXA27x because its warm reset routine contains additional code for working
> around a hardware bug in the warm reset sequence, which causes the reset
> sequence to fail.  I only have a PXA27x platform for testing, so this patch only
> affects the PXA27x.

Hi Mike,

I'm glad you take over that burden too :)
If you've tested that and if it work, that's great news. I'll test it also to
crosscheck.

This topic was discussed several years ago here :
http://www.spinics.net/lists/alsa-devel/msg12327.html

I think Mark has a much better view on the silicon defect, he will be of much
more help than me.

Cheers.

--
Robert

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Mike Dunn]

On 12/28/2012 10:22 AM, Robert Jarzmik wrote:
> Mike Dunn <mikedunn@newsguy.com> writes:
> 
>> Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
>> developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
>> by setting the WARM_RST bit in the GCR register at the end of the cold reset,
>> and then skipping the warm reset if the link is already up and running.
>>
>> It appears that setting the WARM_RST bit is a necessary final step during cold
>> reset.  I think that the PXA25x and PXA3xx may currently be working correctly
>> because WARM_RST is set within the warm reset routine, and the codec drivers
>> always follow a cold reset with a warm reset during their initialization. so
>> this combination effectively completes a cold reset.  This doesn't work on the
>> PXA27x because its warm reset routine contains additional code for working
>> around a hardware bug in the warm reset sequence, which causes the reset
>> sequence to fail.  I only have a PXA27x platform for testing, so this patch only
>> affects the PXA27x.
> 
> Hi Mike,
> 
> I'm glad you take over that burden too :)


No one else wants to work on this ancient hardware.  It's all Kirkwood this and
Omap that :)  Want to get the kernel fully functional on this old but still
widely available device.


> If you've tested that and if it work, that's great news. I'll test it also to
> crosscheck.


Thanks.  I'd really appreciate that.  Yes, it works for me.


> 
> This topic was discussed several years ago here :
> http://www.spinics.net/lists/alsa-devel/msg12327.html
> 
> I think Mark has a much better view on the silicon defect, he will be of much
> more help than me.


Thanks for the pointer.  I should have done some searches on this myself.  I
would have if I was hitting a wall, but I chanced upon the solution pretty quickly.

Mike

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

> On 12/28/2012 10:22 AM, Robert Jarzmik wrote:
>> Mike Dunn <mikedunn@newsguy.com> writes:
>> 
>>> Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
>>> developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
>>> by setting the WARM_RST bit in the GCR register at the end of the cold reset,
>>> and then skipping the warm reset if the link is already up and running.
>>>
>>> It appears that setting the WARM_RST bit is a necessary final step during cold
>>> reset.  I think that the PXA25x and PXA3xx may currently be working correctly
>>> because WARM_RST is set within the warm reset routine, and the codec drivers
>>> always follow a cold reset with a warm reset during their initialization. so
>>> this combination effectively completes a cold reset.  This doesn't work on the
>>> PXA27x because its warm reset routine contains additional code for working
>>> around a hardware bug in the warm reset sequence, which causes the reset
>>> sequence to fail.  I only have a PXA27x platform for testing, so this patch only
>>> affects the PXA27x.
>> If you've tested that and if it work, that's great news. I'll test it also to
>> crosscheck.
>
>
> Thanks.  I'd really appreciate that.  Yes, it works for me.

Ok, I tested it, through a suspend/resume cycle with a music played, and it
works, and the patch looks correct to me. So :

Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

-- 
Robert

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Mike Dunn]

On 12/30/2012 05:12 AM, Robert Jarzmik wrote:
> Mike Dunn <mikedunn@newsguy.com> writes:
> 
>> On 12/28/2012 10:22 AM, Robert Jarzmik wrote:
>>> Mike Dunn <mikedunn@newsguy.com> writes:
>>>
>>>> Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
>>>> developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
>>>> by setting the WARM_RST bit in the GCR register at the end of the cold reset,
>>>> and then skipping the warm reset if the link is already up and running.
>>>>
>>>> It appears that setting the WARM_RST bit is a necessary final step during cold
>>>> reset.  I think that the PXA25x and PXA3xx may currently be working correctly
>>>> because WARM_RST is set within the warm reset routine, and the codec drivers
>>>> always follow a cold reset with a warm reset during their initialization. so
>>>> this combination effectively completes a cold reset.  This doesn't work on the
>>>> PXA27x because its warm reset routine contains additional code for working
>>>> around a hardware bug in the warm reset sequence, which causes the reset
>>>> sequence to fail.  I only have a PXA27x platform for testing, so this patch only
>>>> affects the PXA27x.
>>> If you've tested that and if it work, that's great news. I'll test it also to
>>> crosscheck.
>>
>>
>> Thanks.  I'd really appreciate that.  Yes, it works for me.
> 
> Ok, I tested it, through a suspend/resume cycle with a music played, and it
> works, and the patch looks correct to me. So :
> 
> Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>


Great!  Many thanks Robert!

Mike

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Mike Dunn]

Please don't apply this patch.  It works, but there is a better way to
accomplish the same goal by fixing another problem and also keeping things
consistent among the pxa siblings.  Patches to follow.

Thanks again Igor.

Mike


On 12/28/2012 07:16 AM, Mike Dunn wrote:
> Currently, ac97 reset is broken on PXA27x.  Through trial-and-error (the pxa270
> developer's manual is mostly incoherent on the topic of ac97 reset), I fixed it
> by setting the WARM_RST bit in the GCR register at the end of the cold reset,
> and then skipping the warm reset if the link is already up and running.
> 
> It appears that setting the WARM_RST bit is a necessary final step during cold
> reset.  I think that the PXA25x and PXA3xx may currently be working correctly
> because WARM_RST is set within the warm reset routine, and the codec drivers
> always follow a cold reset with a warm reset during their initialization. so
> this combination effectively completes a cold reset.  This doesn't work on the
> PXA27x because its warm reset routine contains additional code for working
> around a hardware bug in the warm reset sequence, which causes the reset
> sequence to fail.  I only have a PXA27x platform for testing, so this patch only
> affects the PXA27x.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> 
> As an aside... I don't understand how always following a cold reset with a warm
> reset (as the codec drivers do) makes sense.  Unless I'm mistaken, a warm reset
> can only occur on a link that's currently down.  All the timing diagrams I've
> seen for reset (warm and cold) show the link down (BITCLK stopped) at the start
> of the reset sequence.  The SYNC line is used to effect the warm reset, and SYNC
> is already pulsing every frame on a link that's up and running, so it seems
> physically impossible to do a warm reset on a link that's currently up.  But I'm
> no ac97 expert, so I may just be embarassing myself.  Any insight gratefully
> received.  Thanks!
> 
>  sound/arm/pxa2xx-ac97-lib.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 6fc0ae9..aa242ff 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -138,6 +138,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  {
>  	gsr_bits = 0;
>  
> +	/* don't attempt warm reset if link is up */
> +	if (GSR & (GSR_SCR | GSR_PCR))
> +		return;
> +
>  	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
>  	pxa27x_assert_ac97reset(reset_gpio, 1);
>  	udelay(10);
> @@ -148,6 +152,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  
>  static inline void pxa_ac97_cold_pxa27x(void)
>  {
> +	unsigned int timeout;
> +
>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>  
> @@ -159,6 +165,15 @@ static inline void pxa_ac97_cold_pxa27x(void)
>  	clk_disable(ac97conf_clk);
>  	GCR = GCR_COLD_RST;
>  	udelay(50);
> +
> +	/*
> +	 * Setting the WARM_RST bit is a necessary part of a cold reset, at
> +	 * least on the buggy and poorly documented PXA27x ac97 controller.
> +	 */
> +	GCR |= GCR_WARM_RST;
> +	timeout = 100;     /* wait for the codec-ready bit to be set */
> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
> +		mdelay(1);
>  }
>  #endif
>  

Re: [PATCH] ALSA: pxa2xx: fix ac97 cold reset for pxa27x

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 349 bytes --]

On Sat, Jan 05, 2013 at 09:32:27AM -0800, Mike Dunn wrote:
> Please don't apply this patch.  It works, but there is a better way to
> accomplish the same goal by fixing another problem and also keeping things
> consistent among the pxa siblings.  Patches to follow.

Don't top post...  if I've applied the patch then send an incremental
fix please.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]