mac_esp: fix PIO mode
PATCH 1/1 mac_esp: fix PIO mode
The mac_esp PIO algorithm no longer works in 2.6.31 and crashes my Centris 660av. So here's a better one.
One of the SCSI drives I tested still doesn't like the PIO mode and fails with "esp: esp0: Reconnect IRQ2 timeout" (the same drive works fine in PDMA mode).
This failure happens when esp_reconnect_with_tag() tries to read in two tag bytes but the chip only provides one (0x20). I don't know what causes this. I decided not to waste any more time trying to fix it because the best solution is to rip out the PIO mode altogether and use the DMA engine.
--- drivers/scsi/esp_scsi.c | 11 ++--- drivers/scsi/mac_esp.c | 97 ++++++++++++++++++++++++------------------------ 2 files changed, 55 insertions(+), 53 deletions(-)
drivers/scsi/mac_esp.c +49 -46
@@ -22,7 +22,6 @@#include <asm/irq.h> #include <asm/dma.h> -#include <asm/macints.h> #include <asm/macintosh.h>
@@ -278,24 +277,27 @@ static void mac_esp_send_pdma_cmd(struct* Programmed IO routines follow. */ -static inline int mac_esp_wait_for_fifo (struct esp *esp) +static inline unsigned int mac_esp_wait_for_fifo (struct esp *esp) {int i =500000 ;do { -if (esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ) -return 0 ; +unsigned int fbytes =esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ; + +if (fbytes) +return fbytes;udelay (2 ); }while (--i); printk(KERN_ERR PFX "FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS)); -return 1 ; +return 0 ; }static inline int mac_esp_wait_for_intr (struct esp *esp) { +struct mac_esp_priv *mep =MAC_ESP_GET_PRIV (esp);int i =500000 ; do {
@@ -307,6 +309,7 @@ static inline int mac_esp_wait_for_intr( }while (--i); printk(KERN_ERR PFX "IRQ timeout (sreg %02x)\n", esp->sreg); + mep->error =1 ;return 1 ; }
@@ -346,11 +349,10 @@ static inline int mac_esp_wait_for_intr(static void mac_esp_send_pio_cmd (struct esp *esp,u32 addr,u32 esp_count,u32 dma_count,int write,u8 cmd) { -unsigned long flags;struct mac_esp_priv *mep =MAC_ESP_GET_PRIV (esp);u8 *fifo = esp->regs +ESP_FDATA *16 ; -local_irq_save (flags); +disable_irq (esp->host->irq); cmd &= ~ESP_CMD_DMA ; mep->error =0 ;
@@ -358,11 +360,35 @@ static void mac_esp_send_pio_cmd(structif (write) {scsi_esp_cmd (esp, cmd); -if (!mac_esp_wait_for_intr (esp)) { -if (mac_esp_wait_for_fifo (esp)) - esp_count =0 ; - }else { - esp_count =0 ; +while (1 ) { +unsigned int n; + + n =mac_esp_wait_for_fifo (esp); +if (!n) +break ; + +if (n > esp_count) + n = esp_count; + esp_count -= n; + +MAC_ESP_PIO_LOOP ("%2@,%0@+" , n); + +if (!esp_count) +break ; + +if (mac_esp_wait_for_intr (esp)) +break ; + +if (((esp->sreg &ESP_STAT_PMASK ) !=ESP_DIP ) && + ((esp->sreg &ESP_STAT_PMASK ) !=ESP_MIP )) +break ; + + esp->ireg =esp_read8 (ESP_INTRPT ); +if ((esp->ireg & (ESP_INTR_DC |ESP_INTR_BSERV )) != +ESP_INTR_BSERV ) +break ; + +scsi_esp_cmd (esp,ESP_CMD_TI ); } }else {scsi_esp_cmd (esp,ESP_CMD_FLUSH );
@@ -373,47 +399,24 @@ static void mac_esp_send_pio_cmd(structMAC_ESP_PIO_LOOP ("%0@+,%2@" , esp_count);scsi_esp_cmd (esp, cmd); - } - -while (esp_count) { -unsigned int n; - -if (mac_esp_wait_for_intr (esp)) { - mep->error =1 ; -break ; - } - -if (esp->sreg &ESP_STAT_SPAM ) { - printk(KERN_ERR PFX "gross error\n"); - mep->error =1 ; -break ; - } - n =esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ; - -if (write) { -if (n > esp_count) - n = esp_count; - esp_count -= n; - -MAC_ESP_PIO_LOOP ("%2@,%0@+" , n); +while (esp_count) { +unsigned int n; -if ((esp->sreg &ESP_STAT_PMASK ) ==ESP_STATP ) +if (mac_esp_wait_for_intr (esp))break ; -if (esp_count) { - esp->ireg =esp_read8 (ESP_INTRPT ); -if (esp->ireg &ESP_INTR_DC ) -break ; +if (((esp->sreg &ESP_STAT_PMASK ) !=ESP_DOP ) && + ((esp->sreg &ESP_STAT_PMASK ) !=ESP_MOP )) +break ; -scsi_esp_cmd (esp,ESP_CMD_TI ); - } - }else { esp->ireg =esp_read8 (ESP_INTRPT ); -if (esp->ireg &ESP_INTR_DC ) +if ((esp->ireg & (ESP_INTR_DC |ESP_INTR_BSERV )) != +ESP_INTR_BSERV )break ; - n =MAC_ESP_FIFO_SIZE - n; + n =MAC_ESP_FIFO_SIZE - + (esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES );if (n > esp_count) n = esp_count;
@@ -428,7 +431,7 @@ static void mac_esp_send_pio_cmd(struct } } -local_irq_restore (flags); +enable_irq (esp->host->irq); }static int mac_esp_irq_pending (struct esp *esp)
drivers/scsi/esp_scsi.c +5 -6
@@ -2405,12 +2405,6 @@ static int esp_slave_configure(struct scstruct esp_target_data *tp = &esp->target[dev->id];int goal_tags, queue_depth; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { -/* Bypass async domain validation */ - dev->ppr =0 ; - dev->sdtr =0 ; - } - goal_tags =0 ;if (dev->tagged_supported) {
@@ -2433,6 +2427,11 @@ static int esp_slave_configure(struct sc } tp->flags |=ESP_TGT_DISCONNECT ; +if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { + dev->wdtr =spi_support_wide (dev->sdev_target) =0 ; + dev->sdtr =spi_support_sync (dev->sdev_target) =0 ; + } +if (!spi_initial_dv (dev->sdev_target))spi_dv_device (dev);
From: Finn Thain <[email protected]>
Date: Mon, 23 Nov 2009 14:57:42 +1100 (EST)
Quoted 8 lines
The
mac_espPIO algorithm no longer works in 2.6.31 and crashes my Centris 660av. So here's a better one.One of the SCSI drives I tested still doesn't like the PIO mode and fails with "esp: esp0: Reconnect IRQ2 timeout" (the same drive works fine in PDMA mode).
Quoted 7 lines
This failure happens when
esp_reconnect_with_tag()tries to read in two tag bytes but the chip only provides one (0x20). I don't know what causes this. I decided not to waste any more time trying to fix it because the best solution is to rip out the PIO mode altogether and use the DMA engine.Signed-off-by Finn Thain <[email protected]>
Can you explain why the esp_slave_configure() part of your patch is necessary?
Quoted 19 lines
Index:
linux-2.6.31/drivers/scsi/esp_scsi.c
===================================================================drivers/scsi/esp_scsi.c +0
-6@@ -2405,12 +2405,6 @@ static int esp_slave_configure(struct scstruct esp_target_data *tp = &esp->target[dev->id];int goal_tags, queue_depth; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { -/* Bypass async domain validation */ - dev->ppr =0 ; - dev->sdtr =0 ; - } - goal_tags =0 ;if (dev->tagged_supported) {@@ -2433,6 +2427,11 @@ static int esp_slave_configure(struct sc }
drivers/scsi/esp_scsi.c static int esp_slave_configure(struct sc · line 2437
Quoted 10 lines
drivers/scsi/esp_scsi.c +5
-0tp->flags |=ESP_TGT_DISCONNECT ; +if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { + dev->wdtr =spi_support_wide (dev->sdev_target) =0 ; + dev->sdtr =spi_support_sync (dev->sdev_target) =0 ; + } +if (!spi_initial_dv (dev->sdev_target))spi_dv_device (dev);
That part...
Thanks.
On Wed, 2 Dec 2009, David Miller wrote:
...
Quoted 25 lines
Can you explain why the
esp_slave_configure()part of your patch is necessary?Quoted 21 lines
Index:
linux-2.6.31/drivers/scsi/esp_scsi.c
===================================================================drivers/scsi/esp_scsi.c +0
-6@@ -2405,12 +2405,6 @@ static int esp_slave_configure(struct scstruct esp_target_data *tp = &esp->target[dev->id];int goal_tags, queue_depth; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { -/* Bypass async domain validation */ - dev->ppr =0 ; - dev->sdtr =0 ; - } - goal_tags =0 ;if (dev->tagged_supported) {@@ -2433,6 +2427,11 @@ static int esp_slave_configure(struct sc } tp->flags |=ESP_TGT_DISCONNECT ;
Quoted 8 lines
drivers/scsi/esp_scsi.c static int esp_slave_configure(struct sc · line 2437
Quoted 8 lines
drivers/scsi/esp_scsi.c +5
-0+if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { + dev->wdtr =spi_support_wide (dev->sdev_target) =0 ; + dev->sdtr =spi_support_sync (dev->sdev_target) =0 ; + } +if (!spi_initial_dv (dev->sdev_target))spi_dv_device (dev);
The aim is that domain validation will not test for sync when we know it can't work (in PIO mode). This is the result:
mac_esp: using PIO for controller 0
esp: esp0, regs[50f18000:(null)] irq[19]
esp: esp0 is a ESP236, 25 MHz (ccf=5), SCSI ID 7
scsi0 : esp
scsi 0:0:6:0: Direct-Access QUANTUM LPS540S 590S PQ: 0 ANSI: 2 CCS
target0:0:6: Beginning Domain Validation
target0:0:6: Ending Domain Validation
sd 0:0:6:0: [sda] 1057616 512-byte logical blocks: (541 MB/516 MiB)
sd 0:0:6:0: [sda] Write Protect is off
sd 0:0:6:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Whereas, without the esp_slave_configure() code, I get this:
scsi0 : esp
scsi 0:0:6:0: Direct-Access QUANTUM LPS540S 590S PQ: 0 ANSI: 2 CCS
target0:0:6: Beginning Domain Validation
target0:0:6: asynchronousmac_esp: FIFO is empty (sreg 81)
esp: esp0: DMA length is zero!
esp: esp0: cur adr[010eb082] len[00000000]
target0:0:6: Domain Validation detected failure, dropping back target0:0:6: asynchronousmac_esp: FIFO is empty (sreg 81)
esp: esp0: DMA length is zero!
esp: esp0: cur adr[010eb082] len[00000000]
target0:0:6: Domain Validation detected failure, dropping back target0:0:6: asynchronousmac_esp: FIFO is empty (sreg 81)
esp: esp0: DMA length is zero!
esp: esp0: cur adr[010eb082] len[00000000]
target0:0:6: Domain Validation detected failure, dropping back target0:0:6: asynchronousmac_esp: FIFO is empty (sreg 81)
esp: esp0: DMA length is zero!
esp: esp0: cur adr[010eb082] len[00000000]
target0:0:6: Domain Validation detected failure, dropping back target0:0:6: asynchronousmac_esp: FIFO is empty (sreg 81)
esp: esp0: DMA length is zero!
esp: esp0: cur adr[010eb082] len[00000000]
target0:0:6: Domain Validation Failure, dropping back to Asynchronous target0:0:6: Domain Validation skipping write tests
target0:0:6: Ending Domain Validation
sd 0:0:6:0: [sda] 1057616 512-byte logical blocks: (541 MB/516 MiB)
sd 0:0:6:0: [sda] Write Protect is off
sd 0:0:6:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Which takes longer but gives the same result. The CDROM drive is worse however:
scsi0 : esp
scsi 0:0:4:0: CD-ROM TOSHIBA CD-ROM XM-5401TA 1036 PQ: 0 ANSI: 2
target0:0:4: Beginning Domain Validation
target0:0:4: asynchronousmac_esp: FIFO is empty (sreg 01)
esp: esp0: Aborting command [010dc940:12]
esp: esp0: Current command [010dc940:12]
esp: esp0: Active command [010dc940:12]
esp: esp0: Dumping command log
esp: esp0: ent[27] CMD val[01] sreg[87] seqreg[01] sreg2[00] ireg[10] ss[00] event[06]
esp: esp0: ent[28] CMD val[10] sreg[87] seqreg[01] sreg2[00] ireg[10] ss[00] event[06]
esp: esp0: ent[29] CMD val[12] sreg[87] seqreg[01] sreg2[00] ireg[08] ss[00] event[06]
esp: esp0: ent[30] EVENT val[0d] sreg[87] seqreg[01] sreg2[00] ireg[08] ss[00] event[06]
...
So the esp driver then retries the aborted command a few times, with delays. Multiply those retries by the five domain validation iterations. Then, IIRC, the CDROM finally gets offlined at the end of that slow process.
I did a quick test, and this works too:
@@ -2405,12 +2405,6 @@ static int esp_slave_configure(struct sc
struct esp_target_data *tp = &esp->target[dev->id]; int goal_tags, queue_depth;
- if (esp->flags & ESP_FLAG_DISABLE_SYNC) {
- /* Bypass async domain validation */
- dev->ppr = 0;
- dev->sdtr = 0;
- }
-
goal_tags = 0;
if (dev->tagged_supported) {
@@ -2433,6 +2427,9 @@ static int esp_slave_configure(struct sc
}
tp->flags |= ESP_TGT_DISCONNECT;
+ if (esp->flags & ESP_FLAG_DISABLE_SYNC)
+ spi_support_sync(dev->sdev_target) = 0;
+
if (!spi_initial_dv(dev->sdev_target))
spi_dv_device(dev);
I will resubmit the patch like so if you wish. Or you could go ahead and make the change.
Finn
Quoted 28 lines · Finn Thain
On Wed, 2 Dec 2009, David Miller wrote:
...
Quoted 25 lines
Can you explain why the
esp_slave_configure()part of your patch is necessary?Quoted 21 lines
Index:
linux-2.6.31/drivers/scsi/esp_scsi.c
===================================================================drivers/scsi/esp_scsi.c +0
-6@@ -2405,12 +2405,6 @@ static int esp_slave_configure(struct scstruct esp_target_data *tp = &esp->target[dev->id];int goal_tags, queue_depth; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { -/* Bypass async domain validation */ - dev->ppr =0 ; - dev->sdtr =0 ; - } - goal_tags =0 ;if (dev->tagged_supported) {@@ -2433,6 +2427,11 @@ static int esp_slave_configure(struct sc } tp->flags |=ESP_TGT_DISCONNECT ;
Quoted 11 lines
Quoted 8 lines
drivers/scsi/esp_scsi.c static int esp_slave_configure(struct sc · line 2437
Quoted 8 lines
drivers/scsi/esp_scsi.c +5
-0+if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { + dev->wdtr =spi_support_wide (dev->sdev_target) =0 ; + dev->sdtr =spi_support_sync (dev->sdev_target) =0 ; + } +if (!spi_initial_dv (dev->sdev_target))spi_dv_device (dev);The aim is that domain validation will not test for sync when we know it can't work (in PIO mode). This is the result:
So this isn't the correct way to handle the problem. You're altering the disk capability flags to trick the transport class. What you want to be doing is updating esp_set_period so it won't go under whatever you deem to be a correct period setting (or if it's really async then it won't set the period at all).
James
PATCH 1/1 mac_esp: fix PIO mode, take 2
The mac_esp PIO algorithm no longer works in 2.6.31 and crashes my Centris 660av. So here's a better one.
Also, force async with esp_set_offset() rather than esp_slave_configure().
One of the SCSI drives I tested still doesn't like the PIO mode and fails with "esp: esp0: Reconnect IRQ2 timeout" (the same drive works fine in PDMA mode).
This failure happens when esp_reconnect_with_tag() tries to read in two tag bytes but the chip only provides one (0x20). I don't know what causes this. I decided not to waste any more time trying to fix it because the best solution is to rip out the PIO mode altogether and use the DMA engine.
--- drivers/scsi/esp_scsi.c | 14 ++----- drivers/scsi/mac_esp.c | 95 ++++++++++++++++++++++++------------------------ 2 files changed, 53 insertions(+), 56 deletions(-)
drivers/scsi/mac_esp.c +49 -46
@@ -22,7 +22,6 @@#include <asm/irq.h> #include <asm/dma.h> -#include <asm/macints.h> #include <asm/macintosh.h>
@@ -278,24 +277,27 @@ static void mac_esp_send_pdma_cmd(struct* Programmed IO routines follow. */ -static inline int mac_esp_wait_for_fifo (struct esp *esp) +static inline unsigned int mac_esp_wait_for_fifo (struct esp *esp) {int i =500000 ;do { -if (esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ) -return 0 ; +unsigned int fbytes =esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ; + +if (fbytes) +return fbytes;udelay (2 ); }while (--i); printk(KERN_ERR PFX "FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS)); -return 1 ; +return 0 ; }static inline int mac_esp_wait_for_intr (struct esp *esp) { +struct mac_esp_priv *mep =MAC_ESP_GET_PRIV (esp);int i =500000 ; do {
@@ -307,6 +309,7 @@ static inline int mac_esp_wait_for_intr( }while (--i); printk(KERN_ERR PFX "IRQ timeout (sreg %02x)\n", esp->sreg); + mep->error =1 ;return 1 ; }
@@ -346,11 +349,10 @@ static inline int mac_esp_wait_for_intr(static void mac_esp_send_pio_cmd (struct esp *esp,u32 addr,u32 esp_count,u32 dma_count,int write,u8 cmd) { -unsigned long flags;struct mac_esp_priv *mep =MAC_ESP_GET_PRIV (esp);u8 *fifo = esp->regs +ESP_FDATA *16 ; -local_irq_save (flags); +disable_irq (esp->host->irq); cmd &= ~ESP_CMD_DMA ; mep->error =0 ;
@@ -358,11 +360,35 @@ static void mac_esp_send_pio_cmd(structif (write) {scsi_esp_cmd (esp, cmd); -if (!mac_esp_wait_for_intr (esp)) { -if (mac_esp_wait_for_fifo (esp)) - esp_count =0 ; - }else { - esp_count =0 ; +while (1 ) { +unsigned int n; + + n =mac_esp_wait_for_fifo (esp); +if (!n) +break ; + +if (n > esp_count) + n = esp_count; + esp_count -= n; + +MAC_ESP_PIO_LOOP ("%2@,%0@+" , n); + +if (!esp_count) +break ; + +if (mac_esp_wait_for_intr (esp)) +break ; + +if (((esp->sreg &ESP_STAT_PMASK ) !=ESP_DIP ) && + ((esp->sreg &ESP_STAT_PMASK ) !=ESP_MIP )) +break ; + + esp->ireg =esp_read8 (ESP_INTRPT ); +if ((esp->ireg & (ESP_INTR_DC |ESP_INTR_BSERV )) != +ESP_INTR_BSERV ) +break ; + +scsi_esp_cmd (esp,ESP_CMD_TI ); } }else {scsi_esp_cmd (esp,ESP_CMD_FLUSH );
@@ -373,47 +399,24 @@ static void mac_esp_send_pio_cmd(structMAC_ESP_PIO_LOOP ("%0@+,%2@" , esp_count);scsi_esp_cmd (esp, cmd); - } - -while (esp_count) { -unsigned int n; - -if (mac_esp_wait_for_intr (esp)) { - mep->error =1 ; -break ; - } - -if (esp->sreg &ESP_STAT_SPAM ) { - printk(KERN_ERR PFX "gross error\n"); - mep->error =1 ; -break ; - } - n =esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES ; - -if (write) { -if (n > esp_count) - n = esp_count; - esp_count -= n; - -MAC_ESP_PIO_LOOP ("%2@,%0@+" , n); +while (esp_count) { +unsigned int n; -if ((esp->sreg &ESP_STAT_PMASK ) ==ESP_STATP ) +if (mac_esp_wait_for_intr (esp))break ; -if (esp_count) { - esp->ireg =esp_read8 (ESP_INTRPT ); -if (esp->ireg &ESP_INTR_DC ) -break ; +if (((esp->sreg &ESP_STAT_PMASK ) !=ESP_DOP ) && + ((esp->sreg &ESP_STAT_PMASK ) !=ESP_MOP )) +break ; -scsi_esp_cmd (esp,ESP_CMD_TI ); - } - }else { esp->ireg =esp_read8 (ESP_INTRPT ); -if (esp->ireg &ESP_INTR_DC ) +if ((esp->ireg & (ESP_INTR_DC |ESP_INTR_BSERV )) != +ESP_INTR_BSERV )break ; - n =MAC_ESP_FIFO_SIZE - n; + n =MAC_ESP_FIFO_SIZE - + (esp_read8 (ESP_FFLAGS ) &ESP_FF_FBYTES );if (n > esp_count) n = esp_count;
@@ -428,7 +431,7 @@ static void mac_esp_send_pio_cmd(struct } } -local_irq_restore (flags); +enable_irq (esp->host->irq); }static int mac_esp_irq_pending (struct esp *esp)
drivers/scsi/esp_scsi.c +4 -10
@@ -1449,9 +1449,6 @@ static void esp_msgin_sdtr(struct esp *eif (offset >15 ) goto do_reject; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) - offset =0 ; -if (offset) {int one_clock;
@@ -2405,12 +2402,6 @@ static int esp_slave_configure(struct scstruct esp_target_data *tp = &esp->target[dev->id];int goal_tags, queue_depth; -if (esp->flags &ESP_FLAG_DISABLE_SYNC ) { -/* Bypass async domain validation */ - dev->ppr =0 ; - dev->sdtr =0 ; - } - goal_tags =0 ;if (dev->tagged_supported) {
@@ -2660,7 +2651,10 @@ static void esp_set_offset(struct scsi_tstruct esp *esp =shost_priv (host);struct esp_target_data *tp = &esp->target[target->id]; - tp->nego_goal_offset = offset; +if (esp->flags &ESP_FLAG_DISABLE_SYNC ) + tp->nego_goal_offset =0 ; +else + tp->nego_goal_offset = offset; tp->flags |=ESP_TGT_CHECK_NEGO ; }