[PATCH] pata_bf54x: fix BMIDE status register emulation

[PATCH] pata_bf54x: fix BMIDE status register emulation

[Sergei Shtylyov]

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);
 

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

[Sergei Shtylyov]

Hello.

On 12/28/2011 06:36 PM, Sergei Shtylyov wrote:

> 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

    Forgot to say that burst terminations are happening on the IDE side.

> (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>

WBR, Sergei

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

[Zhang, Sonic]

Acked-by: Sonic Zhang <sonic.zhang@analog.com>


SG list was not implemented in pata_bf54x driver from the beginning, because ATAPI controller on BF54x support maximum 131070 bytes in one BMDMA transfer. The size in sg list buffers may exceeds this limitation easily. Do you know a way to set the maximum total buffer size in a sg list?


Static struct scsi_host_template bfin_sht = {
        ATA_BASE_SHT(DRV_NAME),
        .sg_tablesize                   = SG_NONE,
        .dma_boundary           = ATA_DMA_BOUNDARY,
};


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);

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

[Sergei Shtylyov]

Hello.

On 12/29/2011 02:31 PM, Zhang, Sonic wrote:

> Acked-by: Sonic Zhang<sonic.zhang@analog.com>


> SG list was not implemented in pata_bf54x driver from the beginning, because ATAPI controller on BF54x support maximum 131070 bytes in one BMDMA transfer.

    So what? You can chain transfers in software, using "done" interrupts I think...

> The size in sg list buffers may exceeds this limitation easily. Do you know a way to set the maximum total buffer size in a sg list?

    Maybe thru 'max_sectors' field in scsi_host_template? Though sectors can be 
of different size...

> Static struct scsi_host_template bfin_sht = {
>          ATA_BASE_SHT(DRV_NAME),
>          .sg_tablesize                   = SG_NONE,
>          .dma_boundary           = ATA_DMA_BOUNDARY,
> };

    Ah, I have overlooked this. Anyway 'dma_boundary' should be twice more than 
ATA_DMA_BOUNDARY.

> 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...

WBR, Sergei

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

[Zhang, Sonic]

Hi,

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@mvista.com]
Sent: Thursday, December 29, 2011 10:10 PM
To: Zhang, Sonic
Cc: Sergei Shtylyov; linux-ide@vger.kernel.org; jgarzik@pobox.com
Subject: Re: [PATCH] pata_bf54x: fix BMIDE status register emulation

Hello.

On 12/29/2011 02:31 PM, Zhang, Sonic wrote:

> Acked-by: Sonic Zhang<sonic.zhang@analog.com>


> SG list was not implemented in pata_bf54x driver from the beginning, because ATAPI controller on BF54x support maximum 131070 bytes in one BMDMA transfer.

    So what? You can chain transfers in software, using "done" interrupts I think...

Sonic> I just figure out that this can be solved by setting the max sg_tablesize to 4.

> The size in sg list buffers may exceeds this limitation easily. Do you know a way to set the maximum total buffer size in a sg list?

    Maybe thru 'max_sectors' field in scsi_host_template? Though sectors can be
of different size...

Sonic> max_sectors doesn't affect the max size of a sg list.

> Static struct scsi_host_template bfin_sht = {
>          ATA_BASE_SHT(DRV_NAME),
>          .sg_tablesize                   = SG_NONE,
>          .dma_boundary           = ATA_DMA_BOUNDARY,
> };

    Ah, I have overlooked this. Anyway 'dma_boundary' should be twice more than
ATA_DMA_BOUNDARY.

Sonic> I don't find this boundary limit in the spec of the DMA controller for BF54x ATAPI device. So, it can be removed.

> 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...

WBR, Sergei

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

[Sergei Shtylyov]

Hello.

On 30-12-2011 10:07, Zhang, Sonic wrote:

>> SG list was not implemented in pata_bf54x driver from the beginning, because ATAPI controller on BF54x support maximum 131070 bytes in one BMDMA transfer.

>      So what? You can chain transfers in software, using "done" interrupts I think...

> Sonic>  I just figure out that this can be solved by setting the max sg_tablesize to 4.

    Why to 4?

>> The size in sg list buffers may exceeds this limitation easily. Do you know a way to set the maximum total buffer size in a sg list?

>      Maybe thru 'max_sectors' field in scsi_host_template? Though sectors can be
> of different size...

> Sonic>  max_sectors doesn't affect the max size of a sg list.

>> Static struct scsi_host_template bfin_sht = {
>>           ATA_BASE_SHT(DRV_NAME),
>>           .sg_tablesize                   = SG_NONE,
>>           .dma_boundary           = ATA_DMA_BOUNDARY,
>> };

>      Ah, I have overlooked this. Anyway 'dma_boundary' should be twice more than
> ATA_DMA_BOUNDARY.

> Sonic>  I don't find this boundary limit in the spec of the DMA controller for BF54x ATAPI device.

    You just mentioned that ATAPI controller can transfer only 128KB in one go.

> So, it can be removed.

>> 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...

WBR, Sergei

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

[Zhang, Sonic]

Hi,

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@mvista.com]
Sent: Friday, December 30, 2011 6:00 PM
To: Zhang, Sonic
Cc: Sergei Shtylyov; linux-ide@vger.kernel.org; jgarzik@pobox.com
Subject: Re: [PATCH] pata_bf54x: fix BMIDE status register emulation

Hello.

On 30-12-2011 10:07, Zhang, Sonic wrote:

>> SG list was not implemented in pata_bf54x driver from the beginning, because ATAPI controller on BF54x support maximum 131070 bytes in one BMDMA transfer.

>      So what? You can chain transfers in software, using "done" interrupts I think...

> Sonic>  I just figure out that this can be solved by setting the max sg_tablesize to 4.

    Why to 4?

Sonic> The test result shows max size of all sg list buffers may exceed 13170 if sg_tablesize is bigger than 4.

>> The size in sg list buffers may exceeds this limitation easily. Do you know a way to set the maximum total buffer size in a sg list?

>      Maybe thru 'max_sectors' field in scsi_host_template? Though sectors can be
> of different size...

> Sonic>  max_sectors doesn't affect the max size of a sg list.

>> Static struct scsi_host_template bfin_sht = {
>>           ATA_BASE_SHT(DRV_NAME),
>>           .sg_tablesize                   = SG_NONE,
>>           .dma_boundary           = ATA_DMA_BOUNDARY,
>> };

>      Ah, I have overlooked this. Anyway 'dma_boundary' should be twice more than
> ATA_DMA_BOUNDARY.

> Sonic>  I don't find this boundary limit in the spec of the DMA controller for BF54x ATAPI device.

    You just mentioned that ATAPI controller can transfer only 128KB in one go.

Sonic> OK. I think I miss understand the meaning of dma_boundary. It should be the max length of one DMA buffer. That means ATA_DMA_BOUNDARY(0xFFFF) is correct for bf54x DMA controller. While 128KB limit is the max length of one data transfer of the ATAPI controller. In bf54x, the DMA controller is not part of the ATAPI controller. And bf54x ATAPI DMA doesn't comply with INF-8038i.


> So, it can be removed.

>> 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...

WBR, Sergei

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

[Zhang, Sonic]

Hi Shtylyov,

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.

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.

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.



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);

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

[Sergei Shtylyov]

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);

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

[Zhang, Sonic]

Hi Serge,

The MULTI_DONE_INT, UDMAIN_DONE_INT and UDMAOUT_DONE_INT are triggered independent of the ATAPI_DEV_INT, although they bind to the same IRQ on bf548. With your patch, these interrupts are ignored and results in kernel error "unhandled IRQ".

If you insist the BMDMA emulation on bf548 should comply with INF-8038i. I would propose the following patch to disable all BF548 ATAPI specific interrupts.


Sonic


---
 drivers/ata/pata_bf54x.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index bd987bb..9711c2a 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -418,14 +418,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
                                        (tcyc_tdvs<<8 | tdvs));
                                ATAPI_SET_ULTRA_TIM_2(base, (tmli<<8 | tss));
                                ATAPI_SET_ULTRA_TIM_3(base, (trp<<8 | tzah));
-
-                               /* Enable host ATAPI Untra DMA interrupts */
-                               ATAPI_SET_INT_MASK(base,
-                                       ATAPI_GET_INT_MASK(base)
-                                       | UDMAIN_DONE_MASK
-                                       | UDMAOUT_DONE_MASK
-                                       | UDMAIN_TERM_MASK
-                                       | UDMAOUT_TERM_MASK);
                        }
                }
        }
@@ -470,10 +462,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
                        ATAPI_SET_MULTI_TIM_0(base, (tm<<8 | td));
                        ATAPI_SET_MULTI_TIM_1(base, (tkr<<8 | tkw));
                        ATAPI_SET_MULTI_TIM_2(base, (teoc<<8 | th));
-
-                       /* Enable host ATAPI Multi DMA interrupts */
-                       ATAPI_SET_INT_MASK(base, ATAPI_GET_INT_MASK(base)
-                               | MULTI_DONE_MASK | MULTI_TERM_MASK);
                        SSYNC();
                }
        }
@@ -1155,13 +1143,10 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
        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);

--
1.7.0.4



-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@mvista.com]
Sent: Saturday, December 31, 2011 11:06 PM
To: Zhang, Sonic
Cc: Sergei Shtylyov; linux-ide@vger.kernel.org; jgarzik@pobox.com
Subject: Re: [PATCH] pata_bf54x: fix BMIDE status register emulation

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);

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

[Sergei Shtylyov]

Hello.

On 04-01-2012 10:04, Zhang, Sonic wrote:

> Hi Serge,

> The MULTI_DONE_INT, UDMAIN_DONE_INT and UDMAOUT_DONE_INT are triggered independent
> of the ATAPI_DEV_INT, although they bind to the same IRQ on bf548. With your patch,
 > these interrupts are ignored and results in kernel error "unhandled IRQ".

    Ah, indeed...

> If you insist the BMDMA emulation on bf548 should comply with INF-8038i.

    It's not that I insist. It clearly follows from libata implemeting 
bmdma_status() method.

> I would propose the following patch to disable all BF548 ATAPI specific interrupts.

    Yes, I agree -- they don't seem needed. And that will simplify the driver 
too. I'll recast the patch and add your signoff if you agree.

> Sonic

> ---
>   drivers/ata/pata_bf54x.c |   19 ++-----------------
>   1 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
> index bd987bb..9711c2a 100644
> --- a/drivers/ata/pata_bf54x.c
> +++ b/drivers/ata/pata_bf54x.c
> @@ -418,14 +418,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>                                          (tcyc_tdvs<<8 | tdvs));
>                                  ATAPI_SET_ULTRA_TIM_2(base, (tmli<<8 | tss));
>                                  ATAPI_SET_ULTRA_TIM_3(base, (trp<<8 | tzah));
> -
> -                               /* Enable host ATAPI Untra DMA interrupts */
> -                               ATAPI_SET_INT_MASK(base,
> -                                       ATAPI_GET_INT_MASK(base)
> -                                       | UDMAIN_DONE_MASK
> -                                       | UDMAOUT_DONE_MASK
> -                                       | UDMAIN_TERM_MASK
> -                                       | UDMAOUT_TERM_MASK);
>                          }
>                  }
>          }
> @@ -470,10 +462,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>                          ATAPI_SET_MULTI_TIM_0(base, (tm<<8 | td));
>                          ATAPI_SET_MULTI_TIM_1(base, (tkr<<8 | tkw));
>                          ATAPI_SET_MULTI_TIM_2(base, (teoc<<8 | th));
> -
> -                       /* Enable host ATAPI Multi DMA interrupts */
> -                       ATAPI_SET_INT_MASK(base, ATAPI_GET_INT_MASK(base)
> -                               | MULTI_DONE_MASK | MULTI_TERM_MASK);
>                          SSYNC();
>                  }
>          }
> @@ -1155,13 +1143,10 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
>          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);
>
> --
> 1.7.0.4

WBR, Sergei

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

[Sonic Zhang]

On Wed, Jan 4, 2012 at 9:21 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
>
> On 04-01-2012 10:04, Zhang, Sonic wrote:
>
>> Hi Serge,
>
>
>> The MULTI_DONE_INT, UDMAIN_DONE_INT and UDMAOUT_DONE_INT are triggered
>> independent
>> of the ATAPI_DEV_INT, although they bind to the same IRQ on bf548. With
>> your patch,
>
>> these interrupts are ignored and results in kernel error "unhandled IRQ".
>
>   Ah, indeed...
>
>
>> If you insist the BMDMA emulation on bf548 should comply with INF-8038i.
>
>
>   It's not that I insist. It clearly follows from libata implemeting
> bmdma_status() method.
>
>
>> I would propose the following patch to disable all BF548 ATAPI specific
>> interrupts.
>
>
>   Yes, I agree -- they don't seem needed. And that will simplify the driver
> too. I'll recast the patch and add your signoff if you agree.

No problem.

Sonic

>
>> Sonic
>
>
>> ---
>>  drivers/ata/pata_bf54x.c |   19 ++-----------------
>>  1 files changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
>> index bd987bb..9711c2a 100644
>> --- a/drivers/ata/pata_bf54x.c
>> +++ b/drivers/ata/pata_bf54x.c
>> @@ -418,14 +418,6 @@ static void bfin_set_dmamode(struct ata_port *ap,
>> struct ata_device *adev)
>>                                         (tcyc_tdvs<<8 | tdvs));
>>                                 ATAPI_SET_ULTRA_TIM_2(base, (tmli<<8 |
>> tss));
>>                                 ATAPI_SET_ULTRA_TIM_3(base, (trp<<8 |
>> tzah));
>> -
>> -                               /* Enable host ATAPI Untra DMA interrupts
>> */
>> -                               ATAPI_SET_INT_MASK(base,
>> -                                       ATAPI_GET_INT_MASK(base)
>> -                                       | UDMAIN_DONE_MASK
>> -                                       | UDMAOUT_DONE_MASK
>> -                                       | UDMAIN_TERM_MASK
>> -                                       | UDMAOUT_TERM_MASK);
>>                         }
>>                 }
>>         }
>> @@ -470,10 +462,6 @@ static void bfin_set_dmamode(struct ata_port *ap,
>> struct ata_device *adev)
>>                         ATAPI_SET_MULTI_TIM_0(base, (tm<<8 | td));
>>                         ATAPI_SET_MULTI_TIM_1(base, (tkr<<8 | tkw));
>>                         ATAPI_SET_MULTI_TIM_2(base, (teoc<<8 | th));
>> -
>> -                       /* Enable host ATAPI Multi DMA interrupts */
>> -                       ATAPI_SET_INT_MASK(base, ATAPI_GET_INT_MASK(base)
>> -                               | MULTI_DONE_MASK | MULTI_TERM_MASK);
>>                         SSYNC();
>>                 }
>>         }
>> @@ -1155,13 +1143,10 @@ static unsigned char bfin_bmdma_status(struct
>> ata_port *ap)
>>         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);
>>
>> --
>> 1.7.0.4
>
>
> WBR, Sergei
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sergei Shtylyov]

Hello.

On 01/04/2012 09:04 AM, Zhang, Sonic wrote:

> The MULTI_DONE_INT, UDMAIN_DONE_INT and UDMAOUT_DONE_INT are triggered independent of the ATAPI_DEV_INT, although they bind to the same IRQ on bf548. With your patch, these interrupts are ignored and results in kernel error "unhandled IRQ".

> If you insist the BMDMA emulation on bf548 should comply with INF-8038i. I would propose the following patch to disable all BF548 ATAPI specific interrupts.

> Sonic


> ---
>   drivers/ata/pata_bf54x.c |   19 ++-----------------
>   1 files changed, 2 insertions(+), 17 deletions(-)

> diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
> index bd987bb..9711c2a 100644
> --- a/drivers/ata/pata_bf54x.c
> +++ b/drivers/ata/pata_bf54x.c
> @@ -418,14 +418,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>                                          (tcyc_tdvs<<8 | tdvs));
>                                  ATAPI_SET_ULTRA_TIM_2(base, (tmli<<8 | tss));
>                                  ATAPI_SET_ULTRA_TIM_3(base, (trp<<8 | tzah));
> -
> -                               /* Enable host ATAPI Untra DMA interrupts */
> -                               ATAPI_SET_INT_MASK(base,
> -                                       ATAPI_GET_INT_MASK(base)
> -                                       | UDMAIN_DONE_MASK
> -                                       | UDMAOUT_DONE_MASK
> -                                       | UDMAIN_TERM_MASK
> -                                       | UDMAOUT_TERM_MASK);
>                          }
>                  }
>          }
> @@ -470,10 +462,6 @@ static void bfin_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>                          ATAPI_SET_MULTI_TIM_0(base, (tm<<8 | td));
>                          ATAPI_SET_MULTI_TIM_1(base, (tkr<<8 | tkw));
>                          ATAPI_SET_MULTI_TIM_2(base, (teoc<<8 | th));
> -
> -                       /* Enable host ATAPI Multi DMA interrupts */
> -                       ATAPI_SET_INT_MASK(base, ATAPI_GET_INT_MASK(base)
> -                               | MULTI_DONE_MASK | MULTI_TERM_MASK);
>                          SSYNC();
>                  }
>          }
> @@ -1155,13 +1143,10 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
>          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);
>

    Unfortunately your patch is white space damaged, with tabs converted to 
spaces. I'll reconstruct it...

MBR, Sergei

[PATCH v2] pata_bf54x: fix BMIDE status register emulation

[Sergei Shtylyov]

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 IDE DMA
burst termination interrupts like here (moreover, setting the error bit doesn't
cause an interrupt).  We now need to disable all those unused interrupts...

(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>
Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>

 drivers/ata/pata_bf54x.c |   20 ++------------------
 1 file changed, 2 insertions(+), 18 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
@@ -418,14 +418,6 @@ static void bfin_set_dmamode(struct ata_
 					(tcyc_tdvs<<8 | tdvs));
 				ATAPI_SET_ULTRA_TIM_2(base, (tmli<<8 | tss));
 				ATAPI_SET_ULTRA_TIM_3(base, (trp<<8 | tzah));
-
-				/* Enable host ATAPI Untra DMA interrupts */
-				ATAPI_SET_INT_MASK(base,
-					ATAPI_GET_INT_MASK(base)
-					| UDMAIN_DONE_MASK
-					| UDMAOUT_DONE_MASK
-					| UDMAIN_TERM_MASK
-					| UDMAOUT_TERM_MASK);
 			}
 		}
 	}
@@ -470,10 +462,6 @@ static void bfin_set_dmamode(struct ata_
 			ATAPI_SET_MULTI_TIM_0(base, (tm<<8 | td));
 			ATAPI_SET_MULTI_TIM_1(base, (tkr<<8 | tkw));
 			ATAPI_SET_MULTI_TIM_2(base, (teoc<<8 | th));
-
-			/* Enable host ATAPI Multi DMA interrupts */
-			ATAPI_SET_INT_MASK(base, ATAPI_GET_INT_MASK(base)
-				| MULTI_DONE_MASK | MULTI_TERM_MASK);
 			SSYNC();
 		}
 	}
@@ -1153,15 +1141,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);