Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik <jeff@garzik.org>]

On 01/04/2010 08:30 AM, Russell King wrote:
> On Mon, Jan 04, 2010 at 10:37:56AM +0000, Alan Cox wrote:
>>> 1. There is no way for the 247 to see any configuration settings;
>>>     the only thing it can see are the taskfile reads and writes, and
>>>     the timing of the DMA signals from the 246.
>>>
>>> 2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no
>>>     programming difference between these two modes.  The only way the
>>>     246 can know that UDMA is selected is by looking for the SET
>>>     FEATURES command to the drive.
>>
>> The 2026x certainly snoops SET FEATURES so that would be a reasonable
>> assumption.
>>
>>> I've tried changing the set xfermode code to use a version of
>>> ata_exec_internal() which doesn't return the taskfile, but this makes no
>>> difference to the promise exploding with CRC errors with UDMA writes.
>>> Is it possible to do a similar thing with IDENTIFY?
>>
>> No because you need to know if it worked.
>>
>>> Also, is it possible to get rid of the additional identify and read native
>>> max address commands which seem to be repeated (command register writes
>>> listed):
>>
>> You can turn off Host Protected Area support for this. You could also btw
>> turn *on* host protected area for the IDE stack and see what occurs but I
>> imagine its a red herring anyway. If the snoop is failing then it is more
>> likely to be that the chip has some limitations on the taskfile snooping
>> and perhaps requires that the device register is always written or that
>> the registers are written in a specific order when writing the set
>> features command.
>>
>> Another thing to try if that fails is using a polled set features in case
>> the chip has problems in that area. We've seen a similar bug on some older
>> VIA devices.
>
> Found the problem - getting rid of the read of the alt status register
> after the command has been written fixes the UDMA CRC errors on write:
>
> @@ -676,7 +676,8 @@ void ata_sff_exec_command(struct ata_port *ap, const struct
> ata_taskfile *tf)
>          DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
>
>          iowrite8(tf->command, ap->ioaddr.command_addr);
> -       ata_sff_pause(ap);
> +       ndelay(400);
> +//     ata_sff_pause(ap);
>   }
>   EXPORT_SYMBOL_GPL(ata_sff_exec_command);
>
>
> This rather makes sense.  The PDC20247 handles the UDMA part of the
> protocol.  It has no way to tell the PDC20246 to wait while it suspends
> UDMA, so that a normal register access can take place - the 246 ploughs
> on with the register access without any regard to the state of the 247.
>
> If the drive immediately starts the UDMA protocol after a write to the
> command register (as it probably will for the DMA WRITE command), then
> we'll be accessing the taskfile in the middle of the UDMA setup, which
> can't be good.  It's certainly a violation of the ATA specs.

Well...

Without AltStatus, you would not be able to check and see if a command 
is complete...  pretty much by definition you should able to access that 
at any time.  If BSY=1, none of the other bits are valid, but 
nonetheless it is axiomatic that you cannot determine BSY state without 
reading AltStatus.

It is certainly conceivable that this is a problem on a rare controller 
or two, but in general, AltStatus is how you poll for DMA completion. 
You are allowed to hit it during a DMA transfer.

Well known BIOS code, ATADRVR, the IDE driver and libata all read 
AltStatus __in the middle of a transfer__ in the case of PCI shared 
interrupts, or in the case of interrupt-free polling for DMA completion.

HOWEVER, accessing AltStatus prior to the 400ns post-exec-command period 
is potentially an area of undefined behavior.  Changing this code is 
only a problem for MMIO-based controllers, which use the AltStatus read 
to guarantee that the 400ns delay occurs outside any posted writes.

	Jeff