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 at arm.com>
>>>>>
>>>>> The writel() introduced by commit 29a4f2d3 generates an alignment fault
>>>>> on ARM.
>>>>>
>>>>> Signed-off-by: Catalin Marinas<catalin.marinas at arm.com>
>>>>> Cc: Philby John<pjohn at in.mvista.com>
>>>>> Cc: Takashi Iwai<tiwai at 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 at 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