| Message ID | DB904C5425BA6F4E8424B3B51A1414D16D768609F8@NWD2CMBX1.ad.analog.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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);