Skip to contentSkip to conversation

mac_esp, zorro_esp, esp_scsi: Various improvements

PATCH New Finn Thain 1 participant 7 messages Oct 16, 2018 SCSI

lore
Share & tools
lore

Finn Thain

author
#0 lore raw

v3 cover mac_esp, zorro_esp, esp_scsi: Various improvements

This series has fixes and cleanup for mac_esp, zorro_esp and the core esp_scsi driver.

These improvements include elimination of duplicated code temporarily introduced for zorro_esp.

Finn Thain (6):
zorro_esp: Limit DMA transfers to 65535 bytes
esp_scsi: Track residual for PIO transfers
esp_scsi: Grant disconnect privilege for untagged commands
esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD
esp_scsi: De-duplicate PIO routines
esp_scsi: Optimize PIO loops

 drivers/scsi/Kconfig     |   5 +
 drivers/scsi/esp_scsi.c  | 197 +++++++++++++++++++++++++-------
 drivers/scsi/esp_scsi.h  |  11 +-
 drivers/scsi/mac_esp.c   | 171 +---------------------------
 drivers/scsi/zorro_esp.c | 240 ++++++---------------------------------
 5 files changed, 208 insertions(+), 416 deletions(-)
Signature · 1 line
2.18.1
author
#1 lore raw

PATCH v3 1/6 zorro_esp: Limit DMA transfers to 65535 bytes

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so the chip's Transfer Counter register is only 16 bits wide (not 24). A larger transfer cannot work and will theoretically result in a failed command and a "DMA length is zero" error.

Fixes 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro NCR53C9x boards")
Cc Michael Schmitz <[email protected]>
---
 drivers/scsi/zorro_esp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/scsi/zorro_esp.c +1 -7
index bb70882e6b56..be79127db594 100644
@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
 					u32 dma_len)
 {
-	return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
+	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }
@@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */
@@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */
@@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */
Signature · 1 line
2.18.1
author
#2 lore raw

PATCH v3 2/6 esp_scsi: Track residual for PIO transfers

In reply to Finn Thain

If a target disconnects during a PIO data transfer the command may fail when the target reconnects:

scsi host1: DMA length is zero!
scsi host1: cur adr[04380000] len[00000000]

The scsi bus is then reset. This happens because the residual reached zero before the transfer was completed.

The usual residual calculation relies on the Transfer Count registers. That works for DMA transfers but not for PIO transfers. Fix the problem by storing the PIO transfer residual and using that to correctly calculate bytes_sent.

Fixes 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")

---
Changed since v2:
- Declare send_cmd_residual as u32 instead of int.

---
 drivers/scsi/esp_scsi.c | 1 +
 drivers/scsi/esp_scsi.h | 2 ++
 drivers/scsi/mac_esp.c  | 2 ++
 3 files changed, 5 insertions(+)
drivers/scsi/esp_scsi.c +1 -0
index c3fc34b9964d..9e5d3f7d29ae 100644
@@ -1338,6 +1338,7 @@ static int esp_data_bytes_sent(struct esp *esp, struct esp_cmd_entry *ent,
 
 	bytes_sent = esp->data_dma_len;
 	bytes_sent -= ecount;
+	bytes_sent -= esp->send_cmd_residual;
 
 	/*
 	 * The am53c974 has a DMA 'pecularity'. The doc states:
drivers/scsi/esp_scsi.h +2 -0
index 8163dca2071b..a77772777a30 100644
@@ -540,6 +540,8 @@ struct esp {
 
 	void			*dma;
 	int			dmarev;
+
+	u32			send_cmd_residual;
 };
 
 /* A front-end driver for the ESP chip should do the following in
drivers/scsi/mac_esp.c +2 -0
index eb551f3cc471..71879f2207e0 100644
@@ -427,6 +427,8 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			scsi_esp_cmd(esp, ESP_CMD_TI);
 		}
 	}
+
+	esp->send_cmd_residual = esp_count;
 }
 
 static int mac_esp_irq_pending(struct esp *esp)
Signature · 1 line
2.18.1
author
#3 lore raw

PATCH v3 3/6 esp_scsi: Grant disconnect privilege for untagged commands

In reply to Finn Thain

A SCSI device is not granted disconnect privilege by an esp_scsi host unless that device has its simple_tags flag set. However, a device may support disconnect/reselect and not support command queueing. Allow such devices to disconnect and thereby improve bus utilization.

Drop the redundant 'lp' check. The mid-layer invokes .slave_alloc and .slave_destroy in such a way that we may rely on scmd->device->hostdata for as long as scmd belongs to the low-level driver.

---
Changed since v2:
- Drop redundant 'lp' check.

---
 drivers/scsi/esp_scsi.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
drivers/scsi/esp_scsi.c +1 -10
index 9e5d3f7d29ae..5b24aa21a797 100644
@@ -717,7 +717,6 @@ static struct esp_cmd_entry *find_and_prep_issuable_command(struct esp *esp)
 static void esp_maybe_execute_command(struct esp *esp)
 {
 	struct esp_target_data *tp;
-	struct esp_lun_data *lp;
 	struct scsi_device *dev;
 	struct scsi_cmnd *cmd;
 	struct esp_cmd_entry *ent;
@@ -743,7 +742,6 @@ static void esp_maybe_execute_command(struct esp *esp)
 	tgt = dev->id;
 	lun = dev->lun;
 	tp = &esp->target[tgt];
-	lp = dev->hostdata;
 
 	list_move(&ent->list, &esp->active_cmds);
 
@@ -799,14 +797,7 @@ static void esp_maybe_execute_command(struct esp *esp)
 	}
 
 build_identify:
-	/* If we don't have a lun-data struct yet, we're probing
-	 * so do not disconnect.  Also, do not disconnect unless
-	 * we have a tag on this command.
-	 */
-	if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
-		*p++ = IDENTIFY(1, lun);
-	else
-		*p++ = IDENTIFY(0, lun);
+	*p++ = IDENTIFY(tp->flags & ESP_TGT_DISCONNECT, lun);
 
 	if (ent->tag[0] && esp->rev == ESP100) {
 		/* ESP100 lacks select w/atn3 command, use select
Signature · 1 line
2.18.1
author
#4 lore raw

PATCH v3 4/6 esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD

In reply to Finn Thain

The concept of a 'slow command' as it appears in esp_scsi is confusing because it could refer to an ESP command or a SCSI command. It turns out that it refers to a particular ESP select command which the driver also tracks as 'ESP_SELECT_MSGOUT'. For readability, it is better to use the terminology from the datasheets.

The global ESP_FLAG_DOING_SLOWCMD flag is redundant anyway, as it can be inferred from esp->select_state. Remove the ESP_FLAG_DOING_SLOWCMD cruft and just use a boolean local variable.

---
 drivers/scsi/esp_scsi.c | 57 +++++++++++++++++------------------------
 drivers/scsi/esp_scsi.h |  3 +--
 2 files changed, 24 insertions(+), 36 deletions(-)
drivers/scsi/esp_scsi.c +23 -34
index 5b24aa21a797..989ed594451d 100644
@@ -478,17 +478,6 @@ static void esp_restore_pointers(struct esp *esp, struct esp_cmd_entry *ent)
 	spriv->tot_residue = ent->saved_tot_residue;
 }
 
-static void esp_check_command_len(struct esp *esp, struct scsi_cmnd *cmd)
-{
-	if (cmd->cmd_len == 6 ||
-	    cmd->cmd_len == 10 ||
-	    cmd->cmd_len == 12) {
-		esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
-	} else {
-		esp->flags |= ESP_FLAG_DOING_SLOWCMD;
-	}
-}
-
 static void esp_write_tgt_config3(struct esp *esp, int tgt)
 {
 	if (esp->rev > ESP100A) {
@@ -720,6 +709,7 @@ static void esp_maybe_execute_command(struct esp *esp)
 	struct scsi_device *dev;
 	struct scsi_cmnd *cmd;
 	struct esp_cmd_entry *ent;
+	bool select_and_stop = false;
 	int tgt, lun, i;
 	u32 val, start_cmd;
 	u8 *p;
@@ -750,7 +740,8 @@ static void esp_maybe_execute_command(struct esp *esp)
 	esp_map_dma(esp, cmd);
 	esp_save_pointers(esp, ent);
 
-	esp_check_command_len(esp, cmd);
+	if (!(cmd->cmd_len == 6 || cmd->cmd_len == 10 || cmd->cmd_len == 12))
+		select_and_stop = true;
 
 	p = esp->command_block;
 
@@ -791,9 +782,9 @@ static void esp_maybe_execute_command(struct esp *esp)
 			tp->flags &= ~ESP_TGT_CHECK_NEGO;
 		}
 
-		/* Process it like a slow command.  */
-		if (tp->flags & (ESP_TGT_NEGO_WIDE | ESP_TGT_NEGO_SYNC))
-			esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+		/* If there are multiple message bytes, use Select and Stop */
+		if (esp->msg_out_len)
+			select_and_stop = true;
 	}
 
 build_identify:
@@ -803,23 +794,10 @@ static void esp_maybe_execute_command(struct esp *esp)
 		/* ESP100 lacks select w/atn3 command, use select
 		 * and stop instead.
 		 */
-		esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+		select_and_stop = true;
 	}
 
-	if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
-		start_cmd = ESP_CMD_SELA;
-		if (ent->tag[0]) {
-			*p++ = ent->tag[0];
-			*p++ = ent->tag[1];
-
-			start_cmd = ESP_CMD_SA3;
-		}
-
-		for (i = 0; i < cmd->cmd_len; i++)
-			*p++ = cmd->cmnd[i];
-
-		esp->select_state = ESP_SELECT_BASIC;
-	} else {
+	if (select_and_stop) {
 		esp->cmd_bytes_left = cmd->cmd_len;
 		esp->cmd_bytes_ptr = &cmd->cmnd[0];
 
@@ -834,6 +812,19 @@ static void esp_maybe_execute_command(struct esp *esp)
 
 		start_cmd = ESP_CMD_SELAS;
 		esp->select_state = ESP_SELECT_MSGOUT;
+	} else {
+		start_cmd = ESP_CMD_SELA;
+		if (ent->tag[0]) {
+			*p++ = ent->tag[0];
+			*p++ = ent->tag[1];
+
+			start_cmd = ESP_CMD_SA3;
+		}
+
+		for (i = 0; i < cmd->cmd_len; i++)
+			*p++ = cmd->cmnd[i];
+
+		esp->select_state = ESP_SELECT_BASIC;
 	}
 	val = tgt;
 	if (esp->rev == FASHME)
@@ -1243,7 +1234,6 @@ static int esp_finish_select(struct esp *esp)
 			esp_unmap_dma(esp, cmd);
 			esp_free_lun_tag(ent, cmd->device->hostdata);
 			tp->flags &= ~(ESP_TGT_NEGO_SYNC | ESP_TGT_NEGO_WIDE);
-			esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
 			esp->cmd_bytes_ptr = NULL;
 			esp->cmd_bytes_left = 0;
 		} else {
@@ -1294,9 +1284,8 @@ static int esp_finish_select(struct esp *esp)
 				esp_flush_fifo(esp);
 		}
 
-		/* If we are doing a slow command, negotiation, etc.
-		 * we'll do the right thing as we transition to the
-		 * next phase.
+		/* If we are doing a Select And Stop command, negotiation, etc.
+		 * we'll do the right thing as we transition to the next phase.
 		 */
 		esp_event(esp, ESP_EVENT_CHECK_PHASE);
 		return 0;
drivers/scsi/esp_scsi.h +1 -2
index a77772777a30..9113812f94ba 100644
@@ -490,7 +490,6 @@ struct esp {
 	u32			flags;
 #define ESP_FLAG_DIFFERENTIAL	0x00000001
 #define ESP_FLAG_RESETTING	0x00000002
-#define ESP_FLAG_DOING_SLOWCMD	0x00000004
 #define ESP_FLAG_WIDE_CAPABLE	0x00000008
 #define ESP_FLAG_QUICKIRQ_CHECK	0x00000010
 #define ESP_FLAG_DISABLE_SYNC	0x00000020
@@ -532,7 +531,7 @@ struct esp {
 	u32			min_period;
 	u32			radelay;
 
-	/* Slow command state.  */
+	/* ESP_CMD_SELAS command state */
 	u8			*cmd_bytes_ptr;
 	int			cmd_bytes_left;
 
Signature · 1 line
2.18.1
author
#5 lore raw

PATCH v3 5/6 esp_scsi: De-duplicate PIO routines

In reply to Finn Thain

As a temporary measure, the code to implement PIO transfers was duplicated in zorro_esp and mac_esp. Now that it has stabilized move the common code into the core driver but don't build it unless needed.

This replaces the inline assembler with more portable writesb() calls. Optimizing the m68k writesb() implementation is a separate patch.

---
Changed since v1:
- Use shost_printk() instead of pr_err().
- Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.
Changed since v2:
- Omit "default n" from Kconfig patch.
- Use conventional formatting for ESP sreg values in shost_printk().
- Collect struct members relating to esp_send_pio_cmd().

---
 drivers/scsi/Kconfig     |   5 +
 drivers/scsi/esp_scsi.c  | 128 +++++++++++++++++++++
 drivers/scsi/esp_scsi.h  |   6 +
 drivers/scsi/mac_esp.c   | 173 +----------------------------
 drivers/scsi/zorro_esp.c | 232 ++++++---------------------------------
 5 files changed, 179 insertions(+), 365 deletions(-)
drivers/scsi/Kconfig +5 -0
index 35c909bbf8ba..d2edf0f8beb7 100644
@@ -42,6 +42,9 @@ config SCSI_DMA
 	bool
 	default n
 
+config SCSI_ESP_PIO
+	bool
+
 config SCSI_NETLINK
 	bool
 	default	n
@@ -1355,6 +1358,7 @@ config SCSI_ZORRO_ESP
 	tristate "Zorro ESP SCSI support"
 	depends on ZORRO && SCSI
 	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
 	help
 	  Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
 	  expansion boards for the Amiga.
@@ -1397,6 +1401,7 @@ config SCSI_MAC_ESP
 	tristate "Macintosh NCR53c9[46] SCSI"
 	depends on MAC && SCSI
 	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
 	help
 	  This is the NCR 53c9x SCSI controller found on most of the 68040
 	  based Macintoshes.
drivers/scsi/esp_scsi.c +128 -0

131 lines not on this page · raw message

drivers/scsi/esp_scsi.h +6 -0
index 9113812f94ba..cff84d52eb9c 100644
@@ -540,6 +540,9 @@ struct esp {
 	void			*dma;
 	int			dmarev;
 
+	/* These are used by esp_send_pio_cmd() */
+	u8 __iomem		*fifo_reg;
+	int			send_cmd_error;
 	u32			send_cmd_residual;
 };
 
@@ -581,4 +584,7 @@ extern void scsi_esp_unregister(struct esp *);
 extern irqreturn_t scsi_esp_intr(int, void *);
 extern void scsi_esp_cmd(struct esp *, u8);
 
+extern void esp_send_pio_cmd(struct esp *esp, u32 dma_addr, u32 esp_count,
+			     u32 dma_count, int write, u8 cmd);
+
 #endif /* !(_ESP_SCSI_H) */
drivers/scsi/mac_esp.c +6 -167

225 lines not on this page · raw message

drivers/scsi/zorro_esp.c +34 -198

343 lines not on this page · raw message

Signature · 1 line
2.18.1
author
#6 lore raw

PATCH v3 6/6 esp_scsi: Optimize PIO loops

In reply to Finn Thain

Avoid function calls in the inner PIO loops. On a Centris 660av this improves throughput for sequential read transfers by about 40% and sequential write by about 10%.

Unfortunately it is not possible to have methods like .esp_write8 placed inline so this is always going to be slow, even with LTO.

---
Changed since v1:
- Don't touch the scsi_esp_cmd(esp, ESP_CMD_FLUSH) as it's outside the loop.

---
 drivers/scsi/esp_scsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/scsi/esp_scsi.c +6 -6
index 0f0d486be486..c3077c064522 100644
@@ -2783,7 +2783,7 @@ static inline unsigned int esp_wait_for_fifo(struct esp *esp)
 		if (fbytes)
 			return fbytes;
 
-		udelay(2);
+		udelay(1);
 	} while (--i);
 
 	shost_printk(KERN_ERR, esp->host, "FIFO is empty. sreg [%02x]\n",
@@ -2800,7 +2800,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
 		if (esp->sreg & ESP_STAT_INTR)
 			return 0;
 
-		udelay(2);
+		udelay(1);
 	} while (--i);
 
 	shost_printk(KERN_ERR, esp->host, "IRQ timeout. sreg [%02x]\n",
@@ -2828,7 +2828,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			if (!esp_wait_for_fifo(esp))
 				break;
 
-			*dst++ = esp_read8(ESP_FDATA);
+			*dst++ = readb(esp->fifo_reg);
 			--esp_count;
 
 			if (!esp_count)
@@ -2849,9 +2849,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			}
 
 			if (phase == ESP_MIP)
-				scsi_esp_cmd(esp, ESP_CMD_MOK);
+				esp_write8(ESP_CMD_MOK, ESP_CMD);
 
-			scsi_esp_cmd(esp, ESP_CMD_TI);
+			esp_write8(ESP_CMD_TI, ESP_CMD);
 		}
 	} else {
 		unsigned int n = ESP_FIFO_SIZE;
@@ -2891,7 +2891,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			src += n;
 			esp_count -= n;
 
-			scsi_esp_cmd(esp, ESP_CMD_TI);
+			esp_write8(ESP_CMD_TI, ESP_CMD);
 		}
 	}
 
Signature · 1 line
2.18.1