AACI broken with commit 29a4f2d3

AACI broken with commit 29a4f2d3

[Catalin Marinas]

Hi,

The commit mentioned above adds a writel() to the AC97_POWERDOWN
register which is half-word aligned and causing unaligned exceptions on
a Cortex-A8. Is this register 16-bit only and we should use a writew()
instead?

Thanks.

-- 
Catalin

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Wed, 2010-03-24 at 13:51 +0000, Catalin Marinas wrote:
> The commit mentioned above adds a writel() to the AC97_POWERDOWN
> register which is half-word aligned and causing unaligned exceptions on
> a Cortex-A8. Is this register 16-bit only and we should use a writew()
> instead?

Something like below:


aaci: Use writew() to the AC97_POWERDOWN 16-bit register

From: Catalin Marinas <catalin.marinas@arm.com>

The writel() introduced by commit 29a4f2d3 generates an alignment fault
on ARM.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Philby John <pjohn@in.mvista.com>
Cc: Takashi Iwai <tiwai@suse.de>
---
 sound/arm/aaci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index 656e474..d66d4ff 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
 	struct snd_ac97 *ac97;
 	int ret;
 
-	writel(0, aaci->base + AC97_POWERDOWN);
+	writew(0, aaci->base + AC97_POWERDOWN);
 	/*
 	 * Assert AACIRESET for 2us
 	 */


-- 
Catalin

AACI broken with commit 29a4f2d3

[Will Deacon]

Hi Catalin,

> aaci: Use writew() to the AC97_POWERDOWN 16-bit register
> 
> From: Catalin Marinas <catalin.marinas@arm.com>
> 
> The writel() introduced by commit 29a4f2d3 generates an alignment fault
> on ARM.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Philby John <pjohn@in.mvista.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/arm/aaci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> index 656e474..d66d4ff 100644
> --- a/sound/arm/aaci.c
> +++ b/sound/arm/aaci.c
> @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>  	struct snd_ac97 *ac97;
>  	int ret;
> 
> -	writel(0, aaci->base + AC97_POWERDOWN);
> +	writew(0, aaci->base + AC97_POWERDOWN);
>  	/*
>  	 * Assert AACIRESET for 2us
>  	 */

A writel() looks wrong anyway because even if it could succeed, it would
send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
to me.

Tested-by: Will Deacon <will.deacon@arm.com>

Will

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Wed, 24 Mar 2010 15:18:06 -0000,
Will Deacon wrote:
> 
> Hi Catalin,
> 
> > aaci: Use writew() to the AC97_POWERDOWN 16-bit register
> > 
> > From: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > The writel() introduced by commit 29a4f2d3 generates an alignment fault
> > on ARM.
> > 
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Philby John <pjohn@in.mvista.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/arm/aaci.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> > index 656e474..d66d4ff 100644
> > --- a/sound/arm/aaci.c
> > +++ b/sound/arm/aaci.c
> > @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> >  	struct snd_ac97 *ac97;
> >  	int ret;
> > 
> > -	writel(0, aaci->base + AC97_POWERDOWN);
> > +	writew(0, aaci->base + AC97_POWERDOWN);
> >  	/*
> >  	 * Assert AACIRESET for 2us
> >  	 */
> 
> A writel() looks wrong anyway because even if it could succeed, it would
> send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
> to me.
> 
> Tested-by: Will Deacon <will.deacon@arm.com>

Looking back at the original patch again, I wonder now whether using
AC97_* for the register offset here is really correct.  Usually AC97
registers are accessed indirectly.

Is it just same 0x26, or is this intentional?


thanks,

Takashi

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Thu, Mar 25, 2010 at 12:12:52PM +0100, Takashi Iwai wrote:
> At Wed, 24 Mar 2010 15:18:06 -0000,
> Will Deacon wrote:
> > 
> > Hi Catalin,
> > 
> > > aaci: Use writew() to the AC97_POWERDOWN 16-bit register
> > > 
> > > From: Catalin Marinas <catalin.marinas@arm.com>
> > > 
> > > The writel() introduced by commit 29a4f2d3 generates an alignment fault
> > > on ARM.
> > > 
> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Philby John <pjohn@in.mvista.com>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  sound/arm/aaci.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> > > index 656e474..d66d4ff 100644
> > > --- a/sound/arm/aaci.c
> > > +++ b/sound/arm/aaci.c
> > > @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> > >  	struct snd_ac97 *ac97;
> > >  	int ret;
> > > 
> > > -	writel(0, aaci->base + AC97_POWERDOWN);
> > > +	writew(0, aaci->base + AC97_POWERDOWN);
> > >  	/*
> > >  	 * Assert AACIRESET for 2us
> > >  	 */
> > 
> > A writel() looks wrong anyway because even if it could succeed, it would
> > send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
> > to me.
> > 
> > Tested-by: Will Deacon <will.deacon@arm.com>
> 
> Looking back at the original patch again, I wonder now whether using
> AC97_* for the register offset here is really correct.  Usually AC97
> registers are accessed indirectly.
> 
> Is it just same 0x26, or is this intentional?

The original patch is total crap.

1. You can't access AC97 registers via writel.
2. If the offset is 0x26 (AACI_CSCH2 + AACI_IE + 2), this is writing to
   reserved bits in channel 2's interrupt enable register which has
   nothing to do with the slot 1/2 transmit registers.

The patch could never have been tested in the first place.

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Thu, 25 Mar 2010 11:30:19 +0000,
Russell King - ARM Linux wrote:
> 
> On Thu, Mar 25, 2010 at 12:12:52PM +0100, Takashi Iwai wrote:
> > At Wed, 24 Mar 2010 15:18:06 -0000,
> > Will Deacon wrote:
> > > 
> > > Hi Catalin,
> > > 
> > > > aaci: Use writew() to the AC97_POWERDOWN 16-bit register
> > > > 
> > > > From: Catalin Marinas <catalin.marinas@arm.com>
> > > > 
> > > > The writel() introduced by commit 29a4f2d3 generates an alignment fault
> > > > on ARM.
> > > > 
> > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Philby John <pjohn@in.mvista.com>
> > > > Cc: Takashi Iwai <tiwai@suse.de>
> > > > ---
> > > >  sound/arm/aaci.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> > > > index 656e474..d66d4ff 100644
> > > > --- a/sound/arm/aaci.c
> > > > +++ b/sound/arm/aaci.c
> > > > @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> > > >  	struct snd_ac97 *ac97;
> > > >  	int ret;
> > > > 
> > > > -	writel(0, aaci->base + AC97_POWERDOWN);
> > > > +	writew(0, aaci->base + AC97_POWERDOWN);
> > > >  	/*
> > > >  	 * Assert AACIRESET for 2us
> > > >  	 */
> > > 
> > > A writel() looks wrong anyway because even if it could succeed, it would
> > > send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
> > > to me.
> > > 
> > > Tested-by: Will Deacon <will.deacon@arm.com>
> > 
> > Looking back at the original patch again, I wonder now whether using
> > AC97_* for the register offset here is really correct.  Usually AC97
> > registers are accessed indirectly.
> > 
> > Is it just same 0x26, or is this intentional?
> 
> The original patch is total crap.
> 
> 1. You can't access AC97 registers via writel.
> 2. If the offset is 0x26 (AACI_CSCH2 + AACI_IE + 2), this is writing to
>    reserved bits in channel 2's interrupt enable register which has
>    nothing to do with the slot 1/2 transmit registers.
> 
> The patch could never have been tested in the first place.

Thanks for checking.  Now I wonder why this fixed the original issue
at all.  Obviously something wrong in either testing or problem
analysis.

Anyway, it's better to revert the patch first...


Takashi

AACI broken with commit 29a4f2d3

[Philby John]

On 03/25/2010 05:06 PM, Takashi Iwai wrote:
> At Thu, 25 Mar 2010 11:30:19 +0000,
> Russell King - ARM Linux wrote:
>>
>> On Thu, Mar 25, 2010 at 12:12:52PM +0100, Takashi Iwai wrote:
>>> At Wed, 24 Mar 2010 15:18:06 -0000,
>>> Will Deacon wrote:
>>>>
>>>> Hi Catalin,
>>>>
>>>>> aaci: Use writew() to the AC97_POWERDOWN 16-bit register
>>>>>
>>>>> From: Catalin Marinas<catalin.marinas@arm.com>
>>>>>
>>>>> The writel() introduced by commit 29a4f2d3 generates an alignment fault
>>>>> on ARM.
>>>>>
>>>>> Signed-off-by: Catalin Marinas<catalin.marinas@arm.com>
>>>>> Cc: Philby John<pjohn@in.mvista.com>
>>>>> Cc: Takashi Iwai<tiwai@suse.de>
>>>>> ---
>>>>>   sound/arm/aaci.c |    2 +-
>>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
>>>>> index 656e474..d66d4ff 100644
>>>>> --- a/sound/arm/aaci.c
>>>>> +++ b/sound/arm/aaci.c
>>>>> @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>>>>>   	struct snd_ac97 *ac97;
>>>>>   	int ret;
>>>>>
>>>>> -	writel(0, aaci->base + AC97_POWERDOWN);
>>>>> +	writew(0, aaci->base + AC97_POWERDOWN);
>>>>>   	/*
>>>>>   	 * Assert AACIRESET for 2us
>>>>>   	 */
>>>>
>>>> A writel() looks wrong anyway because even if it could succeed, it would
>>>> send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
>>>> to me.
>>>>
>>>> Tested-by: Will Deacon<will.deacon@arm.com>
>>>
>>> Looking back at the original patch again, I wonder now whether using
>>> AC97_* for the register offset here is really correct.  Usually AC97
>>> registers are accessed indirectly.
>>>
>>> Is it just same 0x26, or is this intentional?
>>
>> The original patch is total crap.
>>
>> 1. You can't access AC97 registers via writel.
>> 2. If the offset is 0x26 (AACI_CSCH2 + AACI_IE + 2), this is writing to
>>     reserved bits in channel 2's interrupt enable register which has
>>     nothing to do with the slot 1/2 transmit registers.
>>
>> The patch could never have been tested in the first place.
>
> Thanks for checking.  Now I wonder why this fixed the original issue
> at all.  Obviously something wrong in either testing or problem
> analysis.
>
> Anyway, it's better to revert the patch first...

This was tested throughly but not for alignment and to verify the fix is 
easy. All you need to do is revert the commit and then type reboot at 
the command prompt. On reboot you will get "aaci-pl041 fpga:04: ac97 
read back fail" failures, rendering audio non-functional on an ARM1176.

The right fix would be to use writew() instead of writel()


Regards,
Philby

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Thu, 2010-03-25 at 11:50 +0000, Philby John wrote:
> On 03/25/2010 05:06 PM, Takashi Iwai wrote:
> > At Thu, 25 Mar 2010 11:30:19 +0000,
> > Russell King - ARM Linux wrote:
> >>
> >> On Thu, Mar 25, 2010 at 12:12:52PM +0100, Takashi Iwai wrote:
> >>> At Wed, 24 Mar 2010 15:18:06 -0000,
> >>> Will Deacon wrote:
> >>>>
> >>>> Hi Catalin,
> >>>>
> >>>>> aaci: Use writew() to the AC97_POWERDOWN 16-bit register
> >>>>>
> >>>>> From: Catalin Marinas<catalin.marinas@arm.com>
> >>>>>
> >>>>> The writel() introduced by commit 29a4f2d3 generates an alignment fault
> >>>>> on ARM.
> >>>>>
> >>>>> Signed-off-by: Catalin Marinas<catalin.marinas@arm.com>
> >>>>> Cc: Philby John<pjohn@in.mvista.com>
> >>>>> Cc: Takashi Iwai<tiwai@suse.de>
> >>>>> ---
> >>>>>   sound/arm/aaci.c |    2 +-
> >>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> >>>>> index 656e474..d66d4ff 100644
> >>>>> --- a/sound/arm/aaci.c
> >>>>> +++ b/sound/arm/aaci.c
> >>>>> @@ -863,7 +863,7 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> >>>>>           struct snd_ac97 *ac97;
> >>>>>           int ret;
> >>>>>
> >>>>> - writel(0, aaci->base + AC97_POWERDOWN);
> >>>>> + writew(0, aaci->base + AC97_POWERDOWN);
> >>>>>           /*
> >>>>>            * Assert AACIRESET for 2us
> >>>>>            */
> >>>>
> >>>> A writel() looks wrong anyway because even if it could succeed, it would
> >>>> send half of its write to AC97_EXTENDED_ID, which sounds suspiciously read-only
> >>>> to me.
> >>>>
> >>>> Tested-by: Will Deacon<will.deacon@arm.com>
> >>>
> >>> Looking back at the original patch again, I wonder now whether using
> >>> AC97_* for the register offset here is really correct.  Usually AC97
> >>> registers are accessed indirectly.
> >>>
> >>> Is it just same 0x26, or is this intentional?
> >>
> >> The original patch is total crap.
> >>
> >> 1. You can't access AC97 registers via writel.
> >> 2. If the offset is 0x26 (AACI_CSCH2 + AACI_IE + 2), this is writing to
> >>     reserved bits in channel 2's interrupt enable register which has
> >>     nothing to do with the slot 1/2 transmit registers.
> >>
> >> The patch could never have been tested in the first place.
> >
> > Thanks for checking.  Now I wonder why this fixed the original issue
> > at all.  Obviously something wrong in either testing or problem
> > analysis.
> >
> > Anyway, it's better to revert the patch first...
> 
> This was tested throughly but not for alignment and to verify the fix is
> easy. All you need to do is revert the commit and then type reboot at
> the command prompt. On reboot you will get "aaci-pl041 fpga:04: ac97
> read back fail" failures, rendering audio non-functional on an ARM1176.

I can confirm that it solves the reset issue on other RealView boards as
well. Whether it's the correct fix I can't tell.

-- 
Catalin

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Thu, Mar 25, 2010 at 12:02:37PM +0000, Catalin Marinas wrote:
> I can confirm that it solves the reset issue on other RealView boards as
> well. Whether it's the correct fix I can't tell.

Something else is going on then.  Whatever, this patch is totally
incorrect and needs to be reverted.  It's not even doing what the
description in the documentation requires.

I quote from page 3-18, which is mentioned in the commit message which
added this code:

  Data transmitted on slot 2 register, AACISL2TX The AACISL2TX register
  is a read/write register. When a write occurs to this register the data
  it contains is sent on the next available frame in slot 2. If a power
  down is required, then data must be written to AACISL1TX location
  address 0x26, which is recorded by the PrimeCell AACI. If the AACISL2TX
  bit 16 is set, then the PrimeCell AACI goes into power down mode.
  Table 3-11 shows the bit assignment of the AACISL2TX register.

And this is definitely NOT what Philby's code is doing.

The original commit is wrong on soo many levels.

AACI broken with commit 29a4f2d3

[Philby John]

On Thu, 2010-03-25 at 12:16 +0000, Russell King - ARM Linux wrote:
> On Thu, Mar 25, 2010 at 12:02:37PM +0000, Catalin Marinas wrote:
> > I can confirm that it solves the reset issue on other RealView boards as
> > well. Whether it's the correct fix I can't tell.
> 
> Something else is going on then.  Whatever, this patch is totally
> incorrect and needs to be reverted.  It's not even doing what the
> description in the documentation requires.
> 
> I quote from page 3-18, which is mentioned in the commit message which
> added this code:
> 
>   Data transmitted on slot 2 register, AACISL2TX The AACISL2TX register
>   is a read/write register. When a write occurs to this register the data
>   it contains is sent on the next available frame in slot 2. If a power
>   down is required, then data must be written to AACISL1TX location
>   address 0x26, which is recorded by the PrimeCell AACI. If the AACISL2TX
>   bit 16 is set, then the PrimeCell AACI goes into power down mode.
>   Table 3-11 shows the bit assignment of the AACISL2TX register.
> 
> And this is definitely NOT what Philby's code is doing.
> 
> The original commit is wrong on soo many levels.

Here is the background work I did previously for the original patch.

Dumped entire AACI registers when reading used to fail. The difference between
a working and non-working probe is confined to just 4 registers.

Working                         Non-Working

0007C000: SL1RX                 00000000: SL1RX
0004E530: SL2RX                 00000000: SL2RX
00000A80: slot flags            00000A02: slot flags
00000000: main flag register    00000002: main flag register

Of which what's interesting are the "slot flags" and the "main flag register".
The slot flag register (AACISLFR) has its 1st bit set to 1, meaning Slot 1
transceiver is busy, when reading vendor/product id (times out after 10 tries).

The manual mandates powering down of the aaci module to attain reset values
(page 3-18, 2-7) by setting AACIRESET. But ofcourse to attain default state its useless
putting them in any clean up routine. That's why I introduced them at probe time. Here is
another way to solve the problem.

>From c10d6111657d881bea53d8559deb7422d0f46583 Mon Sep 17 00:00:00 2001
From: Philby John <pjohn@in.mvista.com>
Date: Fri, 26 Mar 2010 16:41:06 +0530
Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3

The commit 29a4f2d3 uses writel() on the AC97_POWERDOWN register
which is half-word aligned causing unaligned exceptions on a
Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
ac97 read back fail" issue on a soft reset. Reading from 0x26
also clears AACISL1TX/AACISL2TX slots for transmit.

Signed-off-by: Philby John <pjohn@mvista.com>
---
 sound/arm/aaci.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index 656e474..6d677c2 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
 	struct snd_ac97 *ac97;
 	int ret;
 
-	writel(0, aaci->base + AC97_POWERDOWN);
+	/*
+	 * Fix: ac97 read back fail errors by reading
+	 * from Power down register
+	 */
+	readw(aaci->base + 0x26);
 	/*
 	 * Assert AACIRESET for 2us
 	 */
-- 
1.7.0.1

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
> --- a/sound/arm/aaci.c
> +++ b/sound/arm/aaci.c
> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>         struct snd_ac97 *ac97;
>         int ret;
> 
> -       writel(0, aaci->base + AC97_POWERDOWN);
> +       /*
> +        * Fix: ac97 read back fail errors by reading
> +        * from Power down register
> +        */
> +       readw(aaci->base + 0x26);

I still don't understand this. Does aaci->base point to the AACI
registers? There is no register at offset 0x26 but there is one at 0x24
(32-bit AACIIE2).

-- 
Catalin

AACI broken with commit 29a4f2d3

[Philby John]

On 03/26/2010 06:30 PM, Catalin Marinas wrote:
> On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
>> --- a/sound/arm/aaci.c
>> +++ b/sound/arm/aaci.c
>> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>>          struct snd_ac97 *ac97;
>>          int ret;
>>
>> -       writel(0, aaci->base + AC97_POWERDOWN);
>> +       /*
>> +        * Fix: ac97 read back fail errors by reading
>> +        * from Power down register
>> +        */
>> +       readw(aaci->base + 0x26);
>
> I still don't understand this. Does aaci->base point to the AACI
> registers? There is no register at offset 0x26 but there is one at 0x24
> (32-bit AACIIE2).
>

I think there is a register at 0x26 for AACI, except that its not 
defined in aaci.h. References in the manual such as "The AC-link signals
can be placed in low-power mode, when the power down control
and status register (0x26) of the CODEC is programmed to the
appropriate value, both AACIBITCLK and AACISDATAIN are brought to, and 
held at 0.", refer to this register IMHO.

Regards,
Philby

AACI broken with commit 29a4f2d3

[Mark Brown]

On Fri, Mar 26, 2010 at 06:40:46PM +0530, Philby John wrote:

> I think there is a register at 0x26 for AACI, except that its not  
> defined in aaci.h. References in the manual such as "The AC-link signals
> can be placed in low-power mode, when the power down control
> and status register (0x26) of the CODEC is programmed to the
> appropriate value, both AACIBITCLK and AACISDATAIN are brought to, and  
> held at 0.", refer to this register IMHO.

The AC97 bus is clock mastered by the CODEC, register 0x26 here is the
power down register defined by AC97 for the standard CODEC address map,
not the controller.  Low power mode for the link means that the CODEC
stops driving the clock signals on the bus until a warm reset is done.

There is no standard register map for the controller.

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Fri, 2010-03-26 at 13:10 +0000, Philby John wrote:
> On 03/26/2010 06:30 PM, Catalin Marinas wrote:
> > On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
> >> --- a/sound/arm/aaci.c
> >> +++ b/sound/arm/aaci.c
> >> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> >>          struct snd_ac97 *ac97;
> >>          int ret;
> >>
> >> -       writel(0, aaci->base + AC97_POWERDOWN);
> >> +       /*
> >> +        * Fix: ac97 read back fail errors by reading
> >> +        * from Power down register
> >> +        */
> >> +       readw(aaci->base + 0x26);
> >
> > I still don't understand this. Does aaci->base point to the AACI
> > registers? There is no register at offset 0x26 but there is one at 0x24
> > (32-bit AACIIE2).
> >
> 
> I think there is a register at 0x26 for AACI, except that its not
> defined in aaci.h. References in the manual such as "The AC-link signals
> can be placed in low-power mode, when the power down control
> and status register (0x26) of the CODEC is programmed to the
> appropriate value, both AACIBITCLK and AACISDATAIN are brought to, and
> held at 0.", refer to this register IMHO.

But the above says "the power down control and status register (0x26) of
the CODEC". So this refers to the AC97 registers rather than the AACI
registers. Your patch reads from the AACI registers. The AC97 registers
I think are access with aaci_ac97_(read|write) functions.

-- 
Catalin

AACI broken with commit 29a4f2d3

[Philby John]

On 03/26/2010 07:24 PM, Catalin Marinas wrote:
> On Fri, 2010-03-26 at 13:10 +0000, Philby John wrote:
>> On 03/26/2010 06:30 PM, Catalin Marinas wrote:
>>> On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
>>>> --- a/sound/arm/aaci.c
>>>> +++ b/sound/arm/aaci.c
>>>> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>>>>           struct snd_ac97 *ac97;
>>>>           int ret;
>>>>
>>>> -       writel(0, aaci->base + AC97_POWERDOWN);
>>>> +       /*
>>>> +        * Fix: ac97 read back fail errors by reading
>>>> +        * from Power down register
>>>> +        */
>>>> +       readw(aaci->base + 0x26);
>>>
>>> I still don't understand this. Does aaci->base point to the AACI
>>> registers? There is no register at offset 0x26 but there is one at 0x24
>>> (32-bit AACIIE2).
>>>
>>
>> I think there is a register at 0x26 for AACI, except that its not
>> defined in aaci.h. References in the manual such as "The AC-link signals
>> can be placed in low-power mode, when the power down control
>> and status register (0x26) of the CODEC is programmed to the
>> appropriate value, both AACIBITCLK and AACISDATAIN are brought to, and
>> held at 0.", refer to this register IMHO.
>
> But the above says "the power down control and status register (0x26) of
> the CODEC". So this refers to the AC97 registers rather than the AACI
> registers. Your patch reads from the AACI registers. The AC97 registers
> I think are access with aaci_ac97_(read|write) functions.
>

I think its snd_ac97_read(). But they internally again use readl/writel. 
Won't these cause alignment issues again?

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Fri, 2010-03-26 at 14:00 +0000, Philby John wrote:
> On 03/26/2010 07:24 PM, Catalin Marinas wrote:
> > On Fri, 2010-03-26 at 13:10 +0000, Philby John wrote:
> >> On 03/26/2010 06:30 PM, Catalin Marinas wrote:
> >>> On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
> >>>> --- a/sound/arm/aaci.c
> >>>> +++ b/sound/arm/aaci.c
> >>>> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> >>>>           struct snd_ac97 *ac97;
> >>>>           int ret;
> >>>>
> >>>> -       writel(0, aaci->base + AC97_POWERDOWN);
> >>>> +       /*
> >>>> +        * Fix: ac97 read back fail errors by reading
> >>>> +        * from Power down register
> >>>> +        */
> >>>> +       readw(aaci->base + 0x26);
> >>>
> >>> I still don't understand this. Does aaci->base point to the AACI
> >>> registers? There is no register at offset 0x26 but there is one at 0x24
> >>> (32-bit AACIIE2).
> >>>
> >>
> >> I think there is a register at 0x26 for AACI, except that its not
> >> defined in aaci.h. References in the manual such as "The AC-link signals
> >> can be placed in low-power mode, when the power down control
> >> and status register (0x26) of the CODEC is programmed to the
> >> appropriate value, both AACIBITCLK and AACISDATAIN are brought to, and
> >> held at 0.", refer to this register IMHO.
> >
> > But the above says "the power down control and status register (0x26) of
> > the CODEC". So this refers to the AC97 registers rather than the AACI
> > registers. Your patch reads from the AACI registers. The AC97 registers
> > I think are access with aaci_ac97_(read|write) functions.
> >
> 
> I think its snd_ac97_read().

Which calls aaci_ac97_read().

> But they internally again use readl/writel. Won't these cause
> alignment issues again?

The AC97 registers are read by writing the register number (0x26 in this
case) to AACI_SL1TX (32-bit register and correctly aligned) and reading
the result from AACI_SLFR. There is no readl/writel to something with
offset 0x26.

-- 
Catalin

AACI broken with commit 29a4f2d3

[Mark Brown]

On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:

> But the above says "the power down control and status register (0x26) of
> the CODEC". So this refers to the AC97 registers rather than the AACI
> registers. Your patch reads from the AACI registers. The AC97 registers
> I think are access with aaci_ac97_(read|write) functions.

Yes, they are - but note that some CODECs will power up in low power
mode and therefore attempts to read immediately after the controller
probe function starts executing may fail until the controller has issued
a warm reset.

AACI broken with commit 29a4f2d3

[Catalin Marinas]

On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
> 
> > But the above says "the power down control and status register (0x26) of
> > the CODEC". So this refers to the AC97 registers rather than the AACI
> > registers. Your patch reads from the AACI registers. The AC97 registers
> > I think are access with aaci_ac97_(read|write) functions.
> 
> Yes, they are - but note that some CODECs will power up in low power
> mode and therefore attempts to read immediately after the controller
> probe function starts executing may fail until the controller has issued
> a warm reset.

Yes, possibly. But my point is that accessing offset 0x26 in the AACI
register space has nothing to do with the AC97 power register. At offset
0x26 in the AACI register space you find the top part of the AACIIE2
register (if you can even read this as a half-word).

-- 
Catalin

AACI broken with commit 29a4f2d3

[Mark Brown]

On Fri, Mar 26, 2010 at 02:12:14PM +0000, Catalin Marinas wrote:
> On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> > On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:

> > > But the above says "the power down control and status register (0x26) of
> > > the CODEC". So this refers to the AC97 registers rather than the AACI
> > > registers. Your patch reads from the AACI registers. The AC97 registers
> > > I think are access with aaci_ac97_(read|write) functions.

> > Yes, they are - but note that some CODECs will power up in low power
> > mode and therefore attempts to read immediately after the controller
> > probe function starts executing may fail until the controller has issued
> > a warm reset.

> Yes, possibly. But my point is that accessing offset 0x26 in the AACI
> register space has nothing to do with the AC97 power register. At offset
> 0x26 in the AACI register space you find the top part of the AACIIE2
> register (if you can even read this as a half-word).

Oh, absolutely (see my earlier reply).  I just wanted to be clear that
if there was some confusion about actually interacting with the CODEC
register map there's a problem there.

AACI broken with commit 29a4f2d3

[Philby John]

On Fri, 2010-03-26 at 14:12 +0000, Catalin Marinas wrote:
> On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> > On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
> > 
> > > But the above says "the power down control and status register (0x26) of
> > > the CODEC". So this refers to the AC97 registers rather than the AACI
> > > registers. Your patch reads from the AACI registers. The AC97 registers
> > > I think are access with aaci_ac97_(read|write) functions.
> > 
> > Yes, they are - but note that some CODECs will power up in low power
> > mode and therefore attempts to read immediately after the controller
> > probe function starts executing may fail until the controller has issued
> > a warm reset.
> 
> Yes, possibly. But my point is that accessing offset 0x26 in the AACI
> register space has nothing to do with the AC97 power register. At offset
> 0x26 in the AACI register space you find the top part of the AACIIE2
> register (if you can even read this as a half-word).
> 
>From b411099000bbbb9b076168ee98742a36018a67ac Mon Sep 17 00:00:00 2001
From: Philby John <pjohn@in.mvista.com>
Date: Fri, 26 Mar 2010 16:41:06 +0530
Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3

The commit 29a4f2d3 used writel() at offset 0x26 which is
half-word aligned causing unaligned exceptions on a
Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
ac97 read back fail" issue on a soft reset. Reading from any
arbitrary aaci register seems to solve this issue.

Signed-off-by: Philby John <pjohn@mvista.com>
---
 sound/arm/aaci.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index 656e474..91acc9a 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -863,7 +863,6 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
 	struct snd_ac97 *ac97;
 	int ret;
 
-	writel(0, aaci->base + AC97_POWERDOWN);
 	/*
 	 * Assert AACIRESET for 2us
 	 */
@@ -1047,7 +1046,11 @@ static int __devinit aaci_probe(struct amba_device *dev, struct amba_id *id)
 
 	writel(0x1fff, aaci->base + AACI_INTCLR);
 	writel(aaci->maincr, aaci->base + AACI_MAINCR);
-
+	/*
+	 * Fix: ac97 read back fail errors by reading
+	 * from any arbitrary aaci register.
+	 */
+	readl(aaci->base + AACI_CSCH1);
 	ret = aaci_probe_ac97(aaci);
 	if (ret)
 		goto out;
-- 
1.7.0.1

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Fri, 26 Mar 2010 21:37:51 +0530,
Philby John wrote:
> 
> On Fri, 2010-03-26 at 14:12 +0000, Catalin Marinas wrote:
> > On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> > > On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
> > > 
> > > > But the above says "the power down control and status register (0x26) of
> > > > the CODEC". So this refers to the AC97 registers rather than the AACI
> > > > registers. Your patch reads from the AACI registers. The AC97 registers
> > > > I think are access with aaci_ac97_(read|write) functions.
> > > 
> > > Yes, they are - but note that some CODECs will power up in low power
> > > mode and therefore attempts to read immediately after the controller
> > > probe function starts executing may fail until the controller has issued
> > > a warm reset.
> > 
> > Yes, possibly. But my point is that accessing offset 0x26 in the AACI
> > register space has nothing to do with the AC97 power register. At offset
> > 0x26 in the AACI register space you find the top part of the AACIIE2
> > register (if you can even read this as a half-word).
> > 
> >From b411099000bbbb9b076168ee98742a36018a67ac Mon Sep 17 00:00:00 2001
> From: Philby John <pjohn@in.mvista.com>
> Date: Fri, 26 Mar 2010 16:41:06 +0530
> Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3
> 
> The commit 29a4f2d3 used writel() at offset 0x26 which is
> half-word aligned causing unaligned exceptions on a
> Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
> ac97 read back fail" issue on a soft reset. Reading from any
> arbitrary aaci register seems to solve this issue.

Then, isn't this a generic problem like PCI write-posting?


Takashi

> 
> Signed-off-by: Philby John <pjohn@mvista.com>
> ---
>  sound/arm/aaci.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> index 656e474..91acc9a 100644
> --- a/sound/arm/aaci.c
> +++ b/sound/arm/aaci.c
> @@ -863,7 +863,6 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>  	struct snd_ac97 *ac97;
>  	int ret;
>  
> -	writel(0, aaci->base + AC97_POWERDOWN);
>  	/*
>  	 * Assert AACIRESET for 2us
>  	 */
> @@ -1047,7 +1046,11 @@ static int __devinit aaci_probe(struct amba_device *dev, struct amba_id *id)
>  
>  	writel(0x1fff, aaci->base + AACI_INTCLR);
>  	writel(aaci->maincr, aaci->base + AACI_MAINCR);
> -
> +	/*
> +	 * Fix: ac97 read back fail errors by reading
> +	 * from any arbitrary aaci register.
> +	 */
> +	readl(aaci->base + AACI_CSCH1);
>  	ret = aaci_probe_ac97(aaci);
>  	if (ret)
>  		goto out;
> -- 
> 1.7.0.1
> 
> 
> 

AACI broken with commit 29a4f2d3

[Philby John]

On 03/27/2010 02:41 AM, Takashi Iwai wrote:
> At Fri, 26 Mar 2010 21:37:51 +0530,
> Philby John wrote:
>>
>> On Fri, 2010-03-26 at 14:12 +0000, Catalin Marinas wrote:
>>> On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
>>>> On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
>>>>
>>>>> But the above says "the power down control and status register (0x26) of
>>>>> the CODEC". So this refers to the AC97 registers rather than the AACI
>>>>> registers. Your patch reads from the AACI registers. The AC97 registers
>>>>> I think are access with aaci_ac97_(read|write) functions.
>>>>
>>>> Yes, they are - but note that some CODECs will power up in low power
>>>> mode and therefore attempts to read immediately after the controller
>>>> probe function starts executing may fail until the controller has issued
>>>> a warm reset.
>>>
>>> Yes, possibly. But my point is that accessing offset 0x26 in the AACI
>>> register space has nothing to do with the AC97 power register. At offset
>>> 0x26 in the AACI register space you find the top part of the AACIIE2
>>> register (if you can even read this as a half-word).
>>>
>> > From b411099000bbbb9b076168ee98742a36018a67ac Mon Sep 17 00:00:00 2001
>> From: Philby John<pjohn@in.mvista.com>
>> Date: Fri, 26 Mar 2010 16:41:06 +0530
>> Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3
>>
>> The commit 29a4f2d3 used writel() at offset 0x26 which is
>> half-word aligned causing unaligned exceptions on a
>> Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
>> ac97 read back fail" issue on a soft reset. Reading from any
>> arbitrary aaci register seems to solve this issue.
>
> Then, isn't this a generic problem like PCI write-posting?
>

Yes, it does look like. Takashi, do you know of a better way than 
reading from an aaci register?

Regards,
Philby

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Mon, 29 Mar 2010 13:15:22 +0530,
Philby John wrote:
> 
> On 03/27/2010 02:41 AM, Takashi Iwai wrote:
> > At Fri, 26 Mar 2010 21:37:51 +0530,
> > Philby John wrote:
> >>
> >> On Fri, 2010-03-26 at 14:12 +0000, Catalin Marinas wrote:
> >>> On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> >>>> On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
> >>>>
> >>>>> But the above says "the power down control and status register (0x26) of
> >>>>> the CODEC". So this refers to the AC97 registers rather than the AACI
> >>>>> registers. Your patch reads from the AACI registers. The AC97 registers
> >>>>> I think are access with aaci_ac97_(read|write) functions.
> >>>>
> >>>> Yes, they are - but note that some CODECs will power up in low power
> >>>> mode and therefore attempts to read immediately after the controller
> >>>> probe function starts executing may fail until the controller has issued
> >>>> a warm reset.
> >>>
> >>> Yes, possibly. But my point is that accessing offset 0x26 in the AACI
> >>> register space has nothing to do with the AC97 power register. At offset
> >>> 0x26 in the AACI register space you find the top part of the AACIIE2
> >>> register (if you can even read this as a half-word).
> >>>
> >> > From b411099000bbbb9b076168ee98742a36018a67ac Mon Sep 17 00:00:00 2001
> >> From: Philby John<pjohn@in.mvista.com>
> >> Date: Fri, 26 Mar 2010 16:41:06 +0530
> >> Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3
> >>
> >> The commit 29a4f2d3 used writel() at offset 0x26 which is
> >> half-word aligned causing unaligned exceptions on a
> >> Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
> >> ac97 read back fail" issue on a soft reset. Reading from any
> >> arbitrary aaci register seems to solve this issue.
> >
> > Then, isn't this a generic problem like PCI write-posting?
> >
> 
> Yes, it does look like. Takashi, do you know of a better way than 
> reading from an aaci register?

Well, in the case of PCI, you'd need to read the just-written register
value again before any time-sensitive operation such as reset via
re-asserting the register bit.

But, I'm really not sure whether this is the case, too...


Takashi

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Mon, 29 Mar 2010 09:57:57 +0200,
I wrote:
> 
> At Mon, 29 Mar 2010 13:15:22 +0530,
> Philby John wrote:
> > 
> > On 03/27/2010 02:41 AM, Takashi Iwai wrote:
> > > At Fri, 26 Mar 2010 21:37:51 +0530,
> > > Philby John wrote:
> > >>
> > >> On Fri, 2010-03-26 at 14:12 +0000, Catalin Marinas wrote:
> > >>> On Fri, 2010-03-26 at 14:08 +0000, Mark Brown wrote:
> > >>>> On Fri, Mar 26, 2010 at 01:54:45PM +0000, Catalin Marinas wrote:
> > >>>>
> > >>>>> But the above says "the power down control and status register (0x26) of
> > >>>>> the CODEC". So this refers to the AC97 registers rather than the AACI
> > >>>>> registers. Your patch reads from the AACI registers. The AC97 registers
> > >>>>> I think are access with aaci_ac97_(read|write) functions.
> > >>>>
> > >>>> Yes, they are - but note that some CODECs will power up in low power
> > >>>> mode and therefore attempts to read immediately after the controller
> > >>>> probe function starts executing may fail until the controller has issued
> > >>>> a warm reset.
> > >>>
> > >>> Yes, possibly. But my point is that accessing offset 0x26 in the AACI
> > >>> register space has nothing to do with the AC97 power register. At offset
> > >>> 0x26 in the AACI register space you find the top part of the AACIIE2
> > >>> register (if you can even read this as a half-word).
> > >>>
> > >> > From b411099000bbbb9b076168ee98742a36018a67ac Mon Sep 17 00:00:00 2001
> > >> From: Philby John<pjohn@in.mvista.com>
> > >> Date: Fri, 26 Mar 2010 16:41:06 +0530
> > >> Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3
> > >>
> > >> The commit 29a4f2d3 used writel() at offset 0x26 which is
> > >> half-word aligned causing unaligned exceptions on a
> > >> Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
> > >> ac97 read back fail" issue on a soft reset. Reading from any
> > >> arbitrary aaci register seems to solve this issue.
> > >
> > > Then, isn't this a generic problem like PCI write-posting?
> > >
> > 
> > Yes, it does look like. Takashi, do you know of a better way than 
> > reading from an aaci register?
> 
> Well, in the case of PCI, you'd need to read the just-written register
> value again before any time-sensitive operation such as reset via
> re-asserting the register bit.
> 
> But, I'm really not sure whether this is the case, too...

This issue is still pending.
Can anyone give a proper patch with a rational explanation?

Philby's last patch looks OK to apply wrt coding (at least better than
the original one), but it doesn't clarify exactly why.


thanks,

Takashi

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Tue, Apr 06, 2010 at 10:12:09AM +0200, Takashi Iwai wrote:
> This issue is still pending.
> Can anyone give a proper patch with a rational explanation?
> 
> Philby's last patch looks OK to apply wrt coding (at least better than
> the original one), but it doesn't clarify exactly why.

It is technically *incorrect*, which is why I NAK'd it - several times.

I don't know how we can proceed on this; we certainly can not go adding
this positively incorrect change to the kernel.

Maybe Philby would like to take the time to understand the technical
objection to his patch(es), and try to work out a more reasonable
solution.  Short of that, there doesn't appear to be any other way to
proceed - and given that I suggest that we put the issue on the back
burner until someone who is willing to investigate _can_ and is willing
to do so.

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Tue, 6 Apr 2010 18:41:57 +0100,
Russell King - ARM Linux wrote:
> 
> On Tue, Apr 06, 2010 at 10:12:09AM +0200, Takashi Iwai wrote:
> > This issue is still pending.
> > Can anyone give a proper patch with a rational explanation?
> > 
> > Philby's last patch looks OK to apply wrt coding (at least better than
> > the original one), but it doesn't clarify exactly why.
> 
> It is technically *incorrect*, which is why I NAK'd it - several times.

Well, what I meant is his patch below.  It now has nothing to do with
ac97 register or such.

> I don't know how we can proceed on this; we certainly can not go adding
> this positively incorrect change to the kernel.
> 
> Maybe Philby would like to take the time to understand the technical
> objection to his patch(es), and try to work out a more reasonable
> solution.  Short of that, there doesn't appear to be any other way to
> proceed - and given that I suggest that we put the issue on the back
> burner until someone who is willing to investigate _can_ and is willing
> to do so.

Yeah, thus I've been asking now.  I'm fine to revert the patch first.
But, obviously, without some extra accesslike the above, the ac97
reset failed on Philby's system.  IIRC, Catalin confirmed it, too.

The above is a bloody workaround that cures something magically,
indeed.  So I'd like to know what is the real cause there...


thanks,

Takashi

===
From: Philby John <pjohn@in.mvista.com>
Date: Fri, 26 Mar 2010 16:41:06 +0530
Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3

The commit 29a4f2d3 used writel() at offset 0x26 which is
half-word aligned causing unaligned exceptions on a
Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
ac97 read back fail" issue on a soft reset. Reading from any
arbitrary aaci register seems to solve this issue.

Signed-off-by: Philby John <pjohn@mvista.com>
---
 sound/arm/aaci.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
index 656e474..91acc9a 100644
--- a/sound/arm/aaci.c
+++ b/sound/arm/aaci.c
@@ -863,7 +863,6 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
 	struct snd_ac97 *ac97;
 	int ret;
 
-	writel(0, aaci->base + AC97_POWERDOWN);
 	/*
 	 * Assert AACIRESET for 2us
 	 */
@@ -1047,7 +1046,11 @@ static int __devinit aaci_probe(struct amba_device *dev, struct amba_id *id)
 
 	writel(0x1fff, aaci->base + AACI_INTCLR);
 	writel(aaci->maincr, aaci->base + AACI_MAINCR);
-
+	/*
+	 * Fix: ac97 read back fail errors by reading
+	 * from any arbitrary aaci register.
+	 */
+	readl(aaci->base + AACI_CSCH1);
 	ret = aaci_probe_ac97(aaci);
 	if (ret)
 		goto out;
-- 
1.7.0.1

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Tue, Apr 06, 2010 at 08:07:24PM +0200, Takashi Iwai wrote:
>  	writel(0x1fff, aaci->base + AACI_INTCLR);
>  	writel(aaci->maincr, aaci->base + AACI_MAINCR);
> -
> +	/*
> +	 * Fix: ac97 read back fail errors by reading
> +	 * from any arbitrary aaci register.
> +	 */
> +	readl(aaci->base + AACI_CSCH1);

This seems to be acceptable.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

Thanks.

AACI broken with commit 29a4f2d3

[Takashi Iwai]

At Mon, 12 Apr 2010 19:31:48 +0100,
Russell King - ARM Linux wrote:
> 
> On Tue, Apr 06, 2010 at 08:07:24PM +0200, Takashi Iwai wrote:
> >  	writel(0x1fff, aaci->base + AACI_INTCLR);
> >  	writel(aaci->maincr, aaci->base + AACI_MAINCR);
> > -
> > +	/*
> > +	 * Fix: ac97 read back fail errors by reading
> > +	 * from any arbitrary aaci register.
> > +	 */
> > +	readl(aaci->base + AACI_CSCH1);
> 
> This seems to be acceptable.
> 
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

OK, I merged it now.


thanks,

Takashi

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Fri, Mar 26, 2010 at 01:00:10PM +0000, Catalin Marinas wrote:
> On Fri, 2010-03-26 at 11:28 +0000, Philby John wrote:
> > --- a/sound/arm/aaci.c
> > +++ b/sound/arm/aaci.c
> > @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
> >         struct snd_ac97 *ac97;
> >         int ret;
> > 
> > -       writel(0, aaci->base + AC97_POWERDOWN);
> > +       /*
> > +        * Fix: ac97 read back fail errors by reading
> > +        * from Power down register
> > +        */
> > +       readw(aaci->base + 0x26);
> 
> I still don't understand this. Does aaci->base point to the AACI
> registers? There is no register at offset 0x26 but there is one at 0x24
> (32-bit AACIIE2).

I've covered this several times, and I'm getting sick of saying it.
aaci->base is the base address of the AACI.  aaci->base + 0x26 is a
misaligned address to AACI channel 2 interrupt enable register.

That's the fourth time I've said it.

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Fri, Mar 26, 2010 at 04:58:31PM +0530, Philby John wrote:
> On Thu, 2010-03-25 at 12:16 +0000, Russell King - ARM Linux wrote:
> > On Thu, Mar 25, 2010 at 12:02:37PM +0000, Catalin Marinas wrote:
> > > I can confirm that it solves the reset issue on other RealView boards as
> > > well. Whether it's the correct fix I can't tell.
> > 
> > Something else is going on then.  Whatever, this patch is totally
> > incorrect and needs to be reverted.  It's not even doing what the
> > description in the documentation requires.
> > 
> > I quote from page 3-18, which is mentioned in the commit message which
> > added this code:
> > 
> >   Data transmitted on slot 2 register, AACISL2TX The AACISL2TX register
> >   is a read/write register. When a write occurs to this register the data
> >   it contains is sent on the next available frame in slot 2. If a power
> >   down is required, then data must be written to AACISL1TX location
> >   address 0x26, which is recorded by the PrimeCell AACI. If the AACISL2TX
> >   bit 16 is set, then the PrimeCell AACI goes into power down mode.
> >   Table 3-11 shows the bit assignment of the AACISL2TX register.
> > 
> > And this is definitely NOT what Philby's code is doing.
> > 
> > The original commit is wrong on soo many levels.
> 
> Here is the background work I did previously for the original patch.
> 
> Dumped entire AACI registers when reading used to fail. The difference between
> a working and non-working probe is confined to just 4 registers.
> 
> Working                         Non-Working
> 
> 0007C000: SL1RX                 00000000: SL1RX
> 0004E530: SL2RX                 00000000: SL2RX
> 00000A80: slot flags            00000A02: slot flags
> 00000000: main flag register    00000002: main flag register
> 
> Of which what's interesting are the "slot flags" and the "main flag register".
> The slot flag register (AACISLFR) has its 1st bit set to 1, meaning Slot 1
> transceiver is busy, when reading vendor/product id (times out after 10 tries).
> 
> The manual mandates powering down of the aaci module to attain reset values
> (page 3-18, 2-7) by setting AACIRESET. But ofcourse to attain default state its useless
> putting them in any clean up routine. That's why I introduced them at probe time. Here is
> another way to solve the problem.
> 
> >From c10d6111657d881bea53d8559deb7422d0f46583 Mon Sep 17 00:00:00 2001
> From: Philby John <pjohn@in.mvista.com>
> Date: Fri, 26 Mar 2010 16:41:06 +0530
> Subject: [PATCH] Fix alignment faults on ARM Cortex introduced by commit 29a4f2d3
> 
> The commit 29a4f2d3 uses writel() on the AC97_POWERDOWN register
> which is half-word aligned causing unaligned exceptions on a
> Cortex-A8. The original patch solved the "aaci-pl041 fpga:04:
> ac97 read back fail" issue on a soft reset. Reading from 0x26
> also clears AACISL1TX/AACISL2TX slots for transmit.
> 
> Signed-off-by: Philby John <pjohn@mvista.com>
> ---
>  sound/arm/aaci.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c
> index 656e474..6d677c2 100644
> --- a/sound/arm/aaci.c
> +++ b/sound/arm/aaci.c
> @@ -863,7 +863,11 @@ static int __devinit aaci_probe_ac97(struct aaci *aaci)
>  	struct snd_ac97 *ac97;
>  	int ret;
>  
> -	writel(0, aaci->base + AC97_POWERDOWN);
> +	/*
> +	 * Fix: ac97 read back fail errors by reading
> +	 * from Power down register
> +	 */
> +	readw(aaci->base + 0x26);

You are not reading from the power down register.  As I've explained
twice before, 0x26 is part of channel 2 interrupt enable register.

Register 0x26 is the power down register in the AC'97 codec.  This is not
addressable using readw nor writew or any other variant of the Linux MMIO
API.  It can only be read and written through the slot registers.

NAK.

When you've grasped the above fact, then we might be able to proceed on
this issue.  Until then, NAK NAK NAK NAK NAK NAK NAK.

AACI broken with commit 29a4f2d3

[Russell King - ARM Linux]

On Thu, Mar 25, 2010 at 05:20:00PM +0530, Philby John wrote:
> This was tested throughly but not for alignment and to verify the fix is  
> easy. All you need to do is revert the commit and then type reboot at  
> the command prompt. On reboot you will get "aaci-pl041 fpga:04: ac97  
> read back fail" failures, rendering audio non-functional on an ARM1176.

Given the analysis, I can not believe your statement.

> The right fix would be to use writew() instead of writel()

No, you're writing to reserved bits in the interrupt enable register
for a channel we don't use.  There's no way that this is correct, or
even a valid fix.