Re: [PATCH] pata_bf54x: fix BMIDE status register emulation

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 31-12-2011 7:22, Zhang, Sonic wrote:

> Hi Shtylyov,

    I prefer Sergei.
    And please wrap your messages at 80 characters or less.

> After I review the BF54x ATAPI and DMA spec again, I have to NAK your following patch.
> The BF54x ATAPI DMA controller doesn't comply with INF-8038i.

    If you are emulating SFF-8038i register interface (which you are doing by 
implementing "bmdma" set of libata methods -- and bmdma_status() method in 
perticular), you *have to comply*. Either comply, or don't do it at all.

> Interrupts MULTI_DONE_INT, UDMAIN_DONE_INT and UDMAOUT_DONE_INT are generated by BF54x ATAPI host controller. They also indicate the completion of various types of ATAPI transfers.  So do interrupts MULTI_TERM_INT, UDMAIN_TERM_INT and UDMAOUT_TERM_INT, which indicate the device termination of the ATAPI DMA transfer.

    Device can only terminate DMA burst at which point it's up to the host 
controller to decide whether it was the last burst or not. This can only be 
done with the help of the signals external to DMA interfacem, i.e. INTRQ.

> In BF54x hardware reference manual:

> After servicing the interrupt source associated with a bit, the user must
> clear that interrupt source bit. ATA_DEV_INT is the interrupt generated by
> the device. The rest of the interrupts are generated by the host. Either the
> device or the host interrupt can be used by the firmware.

> The PIO_DONE_INT, MULTI_DONE_INT, ULTRA_IN_DONE_INT, and
> ULTRA_OUT_DONE_INT (W1C) bits indicate that interrupts have been
> asserted on completion of various types of transfers.

> The MULTI_TERM_INT (W1C) bit indicates that the interrupt has been
> asserted on device terminate of the multiword DMA transfer.

> The ULTRA_IN_TERM_INT and ULTRA_OUT_TERM_INT (W1C) bits indicate
> that interrupts have been asserted on device termination of ultra DMA in
> or out transfers.

    I have read all that... it doesn't justify your NAK in any way. Just test 
the patch and report the result.

> Sonic

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]
> Sent: Wednesday, December 28, 2011 11:37 PM
> To: linux-ide@vger.kernel.org; jgarzik@pobox.com; Zhang, Sonic
> Subject: [PATCH] pata_bf54x: fix BMIDE status register emulation
>
> The author of this driver clearly wasn't familiar with the BMIDE specification
> (also known as SFF-8038i) when he implemented the bmdma_status() method: first,
> the interrupt bit of the BMIDE status register corresponds to nothing else but
> INTRQ signal (ATAPI_DEV_INT here); second, the error bit is only set if the
> controller encounters issue doing the bus master transfers, not on the DMA burst
> termination interrupts like here (moreover, setting the error bit doesn't cause
> an interrupt).
>
> (The only thing I couldn't figure out is how to flush the FIFO to memory once
> the interrupt happens as required by the mentioned spec.)
>
> Signed-off-by: Sergei Shtylyov<sshtylyov@ru.mvista.com>
>
> ---
> The patch is against the current Linus' tree.
> Sonic, if you still work in Analog Devices, please give this a try.
>
> I looked over the driver, and it left pretty bad impression. In particular,
> I highly doubt that the transfers with more than one S/G item can work. And
> this is after the driver has been in the kernel for 4 years already...
> Unfortunately, I have neither hardware nor much time to work on improving it...
>
>   drivers/ata/pata_bf54x.c |    8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/drivers/ata/pata_bf54x.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/pata_bf54x.c
> +++ linux-2.6/drivers/ata/pata_bf54x.c
> @@ -1153,15 +1153,11 @@ static unsigned char bfin_bmdma_status(s
>   {
>          unsigned char host_stat = 0;
>          void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
> -       unsigned short int_status = ATAPI_GET_INT_STATUS(base);
>
> -       if (ATAPI_GET_STATUS(base)&  (MULTI_XFER_ON|ULTRA_XFER_ON))
> +       if (ATAPI_GET_STATUS(base)&  (MULTI_XFER_ON | ULTRA_XFER_ON))
>                  host_stat |= ATA_DMA_ACTIVE;
> -       if (int_status&  (MULTI_DONE_INT|UDMAIN_DONE_INT|UDMAOUT_DONE_INT|
> -               ATAPI_DEV_INT))
> +       if (ATAPI_GET_INT_STATUS(base)&  ATAPI_DEV_INT)
>                  host_stat |= ATA_DMA_INTR;
> -       if (int_status&  (MULTI_TERM_INT|UDMAIN_TERM_INT|UDMAOUT_TERM_INT))
> -               host_stat |= ATA_DMA_ERR|ATA_DMA_INTR;
>
>          dev_dbg(ap->dev, "ATAPI: host_stat=0x%x\n", host_stat);