| Message ID | 1357499640-13871-2-git-send-email-mikedunn@newsguy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/06/13 21:13, Mike Dunn wrote: > Cold reset on the pxa27x currently fails and > > pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44) > > appears in the kernel log. Through trial-and-error (the pxa270 developer's > manual is mostly incoherent on the topic of ac97 reset), I got cold reset to > complete by setting the WARM_RST bit in the GCR register (and later noticed that > pxa3xx does this for cold reset as well). Also, a timeout loop is needed to > wait for the reset to complete. > > Tested on a palm treo 680 machine. > > Signed-off-by: Mike Dunn <mikedunn@newsguy.com> > --- > sound/arm/pxa2xx-ac97-lib.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c > index 6fc0ae9..1ecd0a66 100644 > --- a/sound/arm/pxa2xx-ac97-lib.c > +++ b/sound/arm/pxa2xx-ac97-lib.c > @@ -148,6 +148,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 */ > > @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) > clk_enable(ac97conf_clk); > udelay(5); > clk_disable(ac97conf_clk); > - GCR = GCR_COLD_RST; > - udelay(50); > + GCR = GCR_COLD_RST | 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); Can we use msleep() instead? May be this will require to change the granularity to 10... > } > #endif >
On 01/07/2013 01:16 AM, Igor Grinberg wrote: > On 01/06/13 21:13, Mike Dunn wrote: [..] >> 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 */ >> >> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) >> clk_enable(ac97conf_clk); >> udelay(5); >> clk_disable(ac97conf_clk); >> - GCR = GCR_COLD_RST; >> - udelay(50); >> + GCR = GCR_COLD_RST | 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); > > Can we use msleep() instead? > May be this will require to change the granularity to 10... Probably. mdelay() is used by similiar code in the same file, so I just stayed consistent. The code runs very infrequently, so I didn't worry about it. Thanks, Mike
On 01/07/13 15:36, Mike Dunn wrote: > On 01/07/2013 01:16 AM, Igor Grinberg wrote: >> On 01/06/13 21:13, Mike Dunn wrote: > > > [..] > > >>> 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 */ >>> >>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) >>> clk_enable(ac97conf_clk); >>> udelay(5); >>> clk_disable(ac97conf_clk); >>> - GCR = GCR_COLD_RST; >>> - udelay(50); >>> + GCR = GCR_COLD_RST | 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); >> >> Can we use msleep() instead? >> May be this will require to change the granularity to 10... > > > Probably. mdelay() is used by similiar code in the same file, so I just stayed > consistent. The code runs very infrequently, so I didn't worry about it. Well, if we can, I think we'd better do, no?
On 01/07/2013 05:57 AM, Igor Grinberg wrote: > On 01/07/13 15:36, Mike Dunn wrote: >> On 01/07/2013 01:16 AM, Igor Grinberg wrote: >>> On 01/06/13 21:13, Mike Dunn wrote: >> >> >> [..] >> >> >>>> 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 */ >>>> >>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) >>>> clk_enable(ac97conf_clk); >>>> udelay(5); >>>> clk_disable(ac97conf_clk); >>>> - GCR = GCR_COLD_RST; >>>> - udelay(50); >>>> + GCR = GCR_COLD_RST | 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); >>> >>> Can we use msleep() instead? >>> May be this will require to change the granularity to 10... >> >> >> Probably. mdelay() is used by similiar code in the same file, so I just stayed >> consistent. The code runs very infrequently, so I didn't worry about it. > > Well, if we can, I think we'd better do, no? If you feel strongly about it, I'll defer to you. But again my arguments are (1) the code runs very infrequently, and (2) very similar code for the other pxa family siblings uses mdelay(). Thanks, Mike
On 01/07/13 16:19, Mike Dunn wrote: > On 01/07/2013 05:57 AM, Igor Grinberg wrote: >> On 01/07/13 15:36, Mike Dunn wrote: >>> On 01/07/2013 01:16 AM, Igor Grinberg wrote: >>>> On 01/06/13 21:13, Mike Dunn wrote: >>> >>> >>> [..] >>> >>> >>>>> 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 */ >>>>> >>>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) >>>>> clk_enable(ac97conf_clk); >>>>> udelay(5); >>>>> clk_disable(ac97conf_clk); >>>>> - GCR = GCR_COLD_RST; >>>>> - udelay(50); >>>>> + GCR = GCR_COLD_RST | 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); >>>> >>>> Can we use msleep() instead? >>>> May be this will require to change the granularity to 10... >>> >>> >>> Probably. mdelay() is used by similiar code in the same file, so I just stayed >>> consistent. The code runs very infrequently, so I didn't worry about it. >> >> Well, if we can, I think we'd better do, no? > > > If you feel strongly about it, I'll defer to you. But again my arguments are > (1) the code runs very infrequently, and (2) very similar code for the other pxa > family siblings uses mdelay(). No, not too strong, but please, don't argument your code with something like: "there are other places where this is done", because it might be done improperly and just go in unnoticed.
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c index 6fc0ae9..1ecd0a66 100644 --- a/sound/arm/pxa2xx-ac97-lib.c +++ b/sound/arm/pxa2xx-ac97-lib.c @@ -148,6 +148,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 */ @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void) clk_enable(ac97conf_clk); udelay(5); clk_disable(ac97conf_clk); - GCR = GCR_COLD_RST; - udelay(50); + GCR = GCR_COLD_RST | 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
Cold reset on the pxa27x currently fails and pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44) appears in the kernel log. Through trial-and-error (the pxa270 developer's manual is mostly incoherent on the topic of ac97 reset), I got cold reset to complete by setting the WARM_RST bit in the GCR register (and later noticed that pxa3xx does this for cold reset as well). Also, a timeout loop is needed to wait for the reset to complete. Tested on a palm treo 680 machine. Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- sound/arm/pxa2xx-ac97-lib.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)