[PATCH 4/6] ncr5380: Resolve various static checker warnings

[PATCH 4/6] ncr5380: Resolve various static checker warnings

[Finn Thain]

Avoid various warnings from "make C=1" by annotating a couple of
unlock-then-lock sequences, replacing a zero with NULL and
correcting some type casts.

Also avoid a warning from "make W=1" by adding braces.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
These are the warning messages referred to above:

drivers/scsi/NCR5380.c:
warning: context imbalance in 'NCR5380_select' - unexpected unlock
warning: context imbalance in 'NCR5380_information_transfer' - unexpected unlock

drivers/scsi/atari_scsi.c:816:39: warning: Using plain integer as NULL pointer

drivers/scsi/mac_scsi.c:157:46: warning: incorrect type in initializer (different address spaces)
drivers/scsi/mac_scsi.c:157:46: expected unsigned char *s
drivers/scsi/mac_scsi.c:157:46:    got unsigned char [noderef] [usertype] <asn:2>*

drivers/scsi/mac_scsi.c:260:46: warning: incorrect type in initializer (different address spaces)
drivers/scsi/mac_scsi.c:260:46: expected unsigned char *d
drivers/scsi/mac_scsi.c:260:46:    got unsigned char [noderef] [usertype] <asn:2>*

drivers/scsi/mac_scsi.c:384:22: warning: incorrect type in assignment (different address spaces)
drivers/scsi/mac_scsi.c:384:22: expected unsigned char [noderef] [usertype] <asn:2>*io
drivers/scsi/mac_scsi.c:384:22:    got void *<noident>

drivers/scsi/mac_scsi.c:387:35: warning: incorrect type in assignment (different address spaces)
drivers/scsi/mac_scsi.c:387:35: expected unsigned char [noderef] [usertype] <asn:2>*pdma_io
drivers/scsi/mac_scsi.c:387:35:    got void *<noident>

drivers/scsi/NCR5380.c: In function 'maybe_release_dma_irq':
drivers/scsi/NCR5380.c:626:36: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
   NCR5380_release_dma_irq(instance);

Avoiding these messages doesn't fix any bugs but may improve the
signal/noise ratio of static checker output.
---
 drivers/scsi/NCR5380.c    | 5 ++++-
 drivers/scsi/atari_scsi.c | 2 +-
 drivers/scsi/mac_scsi.c   | 8 ++++----
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index f29b407..518d101 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -591,8 +591,9 @@ static inline void maybe_release_dma_irq(struct Scsi_Host *instance)
 	    list_empty(&hostdata->unissued) &&
 	    list_empty(&hostdata->autosense) &&
 	    !hostdata->connected &&
-	    !hostdata->selecting)
+	    !hostdata->selecting) {
 		NCR5380_release_dma_irq(instance);
+	}
 }
 
 /**
@@ -931,6 +932,7 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, void *dev_id)
 
 static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
                                         struct scsi_cmnd *cmd)
+	__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
 	unsigned char tmp[3], phase;
@@ -1623,6 +1625,7 @@ static int NCR5380_transfer_dma(struct Scsi_Host *instance,
  */
 
 static void NCR5380_information_transfer(struct Scsi_Host *instance)
+	__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
 	unsigned char msgout = NOP;
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 2b6eb7c..b2ffab65 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -782,7 +782,7 @@ static int __init atari_scsi_probe(struct platform_device *pdev)
 			return -ENOMEM;
 		}
 		atari_dma_phys_buffer = atari_stram_to_phys(atari_dma_buffer);
-		atari_dma_orig_addr = 0;
+		atari_dma_orig_addr = NULL;
 	}
 
 	instance = scsi_host_alloc(&atari_scsi_template,
diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index ccb68d1..196acc7 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -154,7 +154,7 @@ __asm__ __volatile__					\
 static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
                                 unsigned char *dst, int len)
 {
-	unsigned char *s = hostdata->pdma_io + (INPUT_DATA_REG << 4);
+	u8 __iomem *s = hostdata->pdma_io + (INPUT_DATA_REG << 4);
 	unsigned char *d = dst;
 	int n = len;
 	int transferred;
@@ -257,7 +257,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
                                  unsigned char *src, int len)
 {
 	unsigned char *s = src;
-	unsigned char *d = hostdata->pdma_io + (OUTPUT_DATA_REG << 4);
+	u8 __iomem *d = hostdata->pdma_io + (OUTPUT_DATA_REG << 4);
 	int n = len;
 	int transferred;
 
@@ -381,10 +381,10 @@ static int __init mac_scsi_probe(struct platform_device *pdev)
 
 	hostdata = shost_priv(instance);
 	hostdata->base = pio_mem->start;
-	hostdata->io = (void *)pio_mem->start;
+	hostdata->io = (u8 __iomem *)pio_mem->start;
 
 	if (pdma_mem && setup_use_pdma)
-		hostdata->pdma_io = (void *)pdma_mem->start;
+		hostdata->pdma_io = (u8 __iomem *)pdma_mem->start;
 	else
 		host_flags |= FLAG_NO_PSEUDO_DMA;
 
-- 
2.10.2

[PATCH 2/6] ncr5380: Clean up dead code and redundant macro usage

[Finn Thain]

Remove dead code inside #if 0 conditionals.

Remove the #ifdef __KERNEL__ test, since NCR5380.h has no definitions
that relate to userspace code.

Remove two redundant macro definitions which were overlooked in
commit e9db3198e08b ("sun3_scsi: Adopt NCR5380.c core driver").

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.h    |  7 -------
 drivers/scsi/atari_scsi.c | 31 -------------------------------
 drivers/scsi/sun3_scsi.c  |  3 ---
 3 files changed, 41 deletions(-)

diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index e61d9f9..d78f0957 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -166,11 +166,7 @@
 #define CSR_SCSI_BUF_RDY       0x02	/* ro  SCSI buffer read */
 #define CSR_GATED_53C80_IRQ    0x01	/* ro  Last block xferred */
 
-#if 0
-#define CSR_BASE CSR_SCSI_BUFF_INTR | CSR_53C80_INTR
-#else
 #define CSR_BASE CSR_53C80_INTR
-#endif
 
 /* Note : PHASE_* macros are based on the values of the STATUS register */
 #define PHASE_MASK 	(SR_MSG | SR_CD | SR_IO)
@@ -229,8 +225,6 @@ struct NCR5380_hostdata {
 	char info[168];				/* Host banner message */
 };
 
-#ifdef __KERNEL__
-
 struct NCR5380_cmd {
 	struct list_head list;
 };
@@ -323,5 +317,4 @@ static inline int NCR5380_dma_residual_none(struct NCR5380_hostdata *hostdata)
 	return 0;
 }
 
-#endif				/* __KERNEL__ */
 #endif				/* NCR5380_H */
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 105b353..2b6eb7c 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -178,37 +178,6 @@ static int scsi_dma_is_ignored_buserr(unsigned char dma_stat)
 }
 
 
-#if 0
-/* Dead code... wasn't called anyway :-) and causes some trouble, because at
- * end-of-DMA, both SCSI ints are triggered simultaneously, so the NCR int has
- * to clear the DMA int pending bit before it allows other level 6 interrupts.
- */
-static void scsi_dma_buserr(int irq, void *dummy)
-{
-	unsigned char dma_stat = tt_scsi_dma.dma_ctrl;
-
-	/* Don't do anything if a NCR interrupt is pending. Probably it's just
-	 * masked... */
-	if (atari_irq_pending(IRQ_TT_MFP_SCSI))
-		return;
-
-	printk("Bad SCSI DMA interrupt! dma_addr=0x%08lx dma_stat=%02x dma_cnt=%08lx\n",
-	       SCSI_DMA_READ_P(dma_addr), dma_stat, SCSI_DMA_READ_P(dma_cnt));
-	if (dma_stat & 0x80) {
-		if (!scsi_dma_is_ignored_buserr(dma_stat))
-			printk("SCSI DMA bus error -- bad DMA programming!\n");
-	} else {
-		/* Under normal circumstances we never should get to this point,
-		 * since both interrupts are triggered simultaneously and the 5380
-		 * int has higher priority. When this irq is handled, that DMA
-		 * interrupt is cleared. So a warning message is printed here.
-		 */
-		printk("SCSI DMA intr ?? -- this shouldn't happen!\n");
-	}
-}
-#endif
-
-
 static irqreturn_t scsi_tt_intr(int irq, void *dev)
 {
 	struct Scsi_Host *instance = dev;
diff --git a/drivers/scsi/sun3_scsi.c b/drivers/scsi/sun3_scsi.c
index 88db699..166f466 100644
--- a/drivers/scsi/sun3_scsi.c
+++ b/drivers/scsi/sun3_scsi.c
@@ -56,9 +56,6 @@
 #define NCR5380_dma_send_setup          sun3scsi_dma_count
 #define NCR5380_dma_residual            sun3scsi_dma_residual
 
-#define NCR5380_acquire_dma_irq(instance)    (1)
-#define NCR5380_release_dma_irq(instance)
-
 #include "NCR5380.h"
 
 
-- 
2.10.2

[PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

This series removes some unused code and related comments,
addresses the warnings generated by 'make W=1' and 'make C=1'
and fixes a theoretical bug in the bus reset method in atari_scsi.

There's also a patch to add a missing error check during target
selection. The only target I tested was a QUANTUM DAYTONA514S disk
as that's all I have access to right now. Some testing with other
targets would be prudent.

Michael, Ondrej, can I get you to review/test please?


Finn Thain (6):
  ncr5380: Shorten host info string by removing unused option macros
  ncr5380: Clean up dead code and redundant macro usage
  ncr5380: Reduce #include files
  ncr5380: Resolve various static checker warnings
  ncr5380: Improve target selection robustness
  atari_scsi: Reset DMA during bus reset only under ST-DMA lock

 drivers/scsi/NCR5380.c    |  64 ++++++++++-------------------
 drivers/scsi/NCR5380.h    |  17 +-------
 drivers/scsi/atari_scsi.c |  36 ++--------------
 drivers/scsi/g_NCR5380.c  |  45 +++++++++++++++++++-
 drivers/scsi/g_NCR5380.h  |  56 -------------------------
 drivers/scsi/mac_scsi.c   |   8 ++--
 drivers/scsi/sun3_scsi.c  |  83 +++++++++++++++++++++++++++++++++++--
 drivers/scsi/sun3_scsi.h  | 102 ----------------------------------------------
 8 files changed, 152 insertions(+), 259 deletions(-)
 delete mode 100644 drivers/scsi/g_NCR5380.h
 delete mode 100644 drivers/scsi/sun3_scsi.h

-- 
2.10.2

[PATCH 6/6] atari_scsi: Reset DMA during bus reset only under ST-DMA lock

[Finn Thain]

The atari_scsi driver should not access Falcon DMA chip registers
unless it has acquired exclusive access to that chip. If the driver
doesn't have exclusive access then there's no need for a DMA reset
as there are no scsi commands in progress.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/atari_scsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index b2ffab65..f792420 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -682,7 +682,8 @@ static int atari_scsi_bus_reset(struct scsi_cmnd *cmd)
 	if (IS_A_TT()) {
 		tt_scsi_dma.dma_ctrl = 0;
 	} else {
-		st_dma.dma_mode_status = 0x90;
+		if (stdma_is_locked_by(scsi_falcon_intr))
+			st_dma.dma_mode_status = 0x90;
 		atari_dma_active = 0;
 		atari_dma_orig_addr = NULL;
 	}
-- 
2.10.2

[PATCH 5/6] ncr5380: Improve target selection robustness

[Finn Thain]

Handle timeout or bus phase change errors that could occur when
sending the IDENTIFY message.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 518d101..acc3344 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1165,8 +1165,16 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	data = tmp;
 	phase = PHASE_MSGOUT;
 	NCR5380_transfer_pio(instance, &phase, &len, &data);
+	if (len) {
+		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
+		cmd->result = DID_ERROR << 16;
+		complete_cmd(instance, cmd);
+		dsprintk(NDEBUG_SELECTION, instance, "IDENTIFY message transfer failed\n");
+		cmd = NULL;
+		goto out;
+	}
+
 	dsprintk(NDEBUG_SELECTION, instance, "nexus established.\n");
-	/* XXX need to handle errors here */
 
 	hostdata->connected = cmd;
 	hostdata->busy[cmd->device->id] |= 1 << cmd->device->lun;
-- 
2.10.2

[PATCH 1/6] ncr5380: Shorten host info string by removing unused option macros

[Finn Thain]

The DIFFERENTIAL and PARITY option macros are unused: no supported
hardware uses differential signalling and the core driver never
implemented parity checking. These options just waste space in the host
info string.

While we are here, fix a typo in the NCR5380_info() kernel-doc comment.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.c | 49 +++++++++----------------------------------------
 drivers/scsi/NCR5380.h | 10 +---------
 2 files changed, 10 insertions(+), 49 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 4f5ca79..f29b407 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -96,17 +96,6 @@
  * of chips.  To use it, you write an architecture specific functions
  * and macros and include this file in your driver.
  *
- * These macros control options :
- * AUTOSENSE - if defined, REQUEST SENSE will be performed automatically
- * for commands that return with a CHECK CONDITION status.
- *
- * DIFFERENTIAL - if defined, NCR53c81 chips will use external differential
- * transceivers.
- *
- * PSEUDO_DMA - if defined, PSEUDO DMA is used during the data transfer phases.
- *
- * REAL_DMA - if defined, REAL DMA is used during the data transfer phases.
- *
  * These macros MUST be defined :
  *
  * NCR5380_read(register)  - read from the specified register
@@ -347,7 +336,7 @@ static void NCR5380_print_phase(struct Scsi_Host *instance)
 #endif
 
 /**
- * NCR58380_info - report driver and host information
+ * NCR5380_info - report driver and host information
  * @instance: relevant scsi host instance
  *
  * For use as the host template info() handler.
@@ -360,33 +349,6 @@ static const char *NCR5380_info(struct Scsi_Host *instance)
 	return hostdata->info;
 }
 
-static void prepare_info(struct Scsi_Host *instance)
-{
-	struct NCR5380_hostdata *hostdata = shost_priv(instance);
-
-	snprintf(hostdata->info, sizeof(hostdata->info),
-	         "%s, irq %d, "
-		 "io_port 0x%lx, base 0x%lx, "
-	         "can_queue %d, cmd_per_lun %d, "
-	         "sg_tablesize %d, this_id %d, "
-	         "flags { %s%s%s}, "
-	         "options { %s} ",
-	         instance->hostt->name, instance->irq,
-		 hostdata->io_port, hostdata->base,
-	         instance->can_queue, instance->cmd_per_lun,
-	         instance->sg_tablesize, instance->this_id,
-	         hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
-	         hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
-	         hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY "  : "",
-#ifdef DIFFERENTIAL
-	         "DIFFERENTIAL "
-#endif
-#ifdef PARITY
-	         "PARITY "
-#endif
-	         "");
-}
-
 /**
  * NCR5380_init - initialise an NCR5380
  * @instance: adapter to configure
@@ -436,7 +398,14 @@ static int NCR5380_init(struct Scsi_Host *instance, int flags)
 	if (!hostdata->work_q)
 		return -ENOMEM;
 
-	prepare_info(instance);
+	snprintf(hostdata->info, sizeof(hostdata->info),
+		"%s, irq %d, io_port 0x%lx, base 0x%lx, can_queue %d, cmd_per_lun %d, sg_tablesize %d, this_id %d, flags { %s%s%s}",
+		instance->hostt->name, instance->irq, hostdata->io_port,
+		hostdata->base, instance->can_queue, instance->cmd_per_lun,
+		instance->sg_tablesize, instance->this_id,
+		hostdata->flags & FLAG_DMA_FIXUP     ? "DMA_FIXUP "     : "",
+		hostdata->flags & FLAG_NO_PSEUDO_DMA ? "NO_PSEUDO_DMA " : "",
+		hostdata->flags & FLAG_TOSHIBA_DELAY ? "TOSHIBA_DELAY " : "");
 
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 	NCR5380_write(MODE_REG, MR_BASE);
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 51a3567..e61d9f9 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -81,11 +81,7 @@
 #define ICR_ASSERT_ATN		0x02	/* rw Set to assert ATN */
 #define ICR_ASSERT_DATA		0x01	/* rw SCSI_DATA_REG is asserted */
 
-#ifdef DIFFERENTIAL
-#define ICR_BASE		ICR_DIFF_ENABLE
-#else
 #define ICR_BASE		0
-#endif
 
 #define MODE_REG		2
 /*
@@ -102,11 +98,7 @@
 #define MR_DMA_MODE		0x02	/* rw DMA / pseudo DMA mode */
 #define MR_ARBITRATE		0x01	/* rw start arbitration */
 
-#ifdef PARITY
-#define MR_BASE			MR_ENABLE_PAR_CHECK
-#else
 #define MR_BASE			0
-#endif
 
 #define TARGET_COMMAND_REG	3
 #define TCR_LAST_BYTE_SENT	0x80	/* ro DMA done */
@@ -234,7 +226,7 @@ struct NCR5380_hostdata {
 	unsigned char id_higher_mask;		/* All bits above id_mask */
 	unsigned char last_message;		/* Last Message Out */
 	unsigned long region_size;		/* Size of address/port range */
-	char info[256];
+	char info[168];				/* Host banner message */
 };
 
 #ifdef __KERNEL__
-- 
2.10.2

[PATCH 3/6] ncr5380: Reduce #include files

[Finn Thain]

The NCR5380 wrapper drivers don't export symbols or declarations and
don't actually need separate header files. Most of these header files
were removed already; only sun3_scsi.h and g_NCR5380.h remain.

Move the remaining definitions to the corresponding .c files to improve
readability and proximity. The #defines which influence the #included
core driver are no longer mixed up with unrelated #defines and #includes.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/g_NCR5380.c |  45 ++++++++++++++++++++-
 drivers/scsi/g_NCR5380.h |  56 --------------------------
 drivers/scsi/sun3_scsi.c |  80 ++++++++++++++++++++++++++++++++++++-
 drivers/scsi/sun3_scsi.h | 102 -----------------------------------------------
 4 files changed, 122 insertions(+), 161 deletions(-)
 delete mode 100644 drivers/scsi/g_NCR5380.h
 delete mode 100644 drivers/scsi/sun3_scsi.h

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6f9665d..67c8dac 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -26,14 +26,55 @@
 #include <linux/blkdev.h>
 #include <linux/module.h>
 #include <scsi/scsi_host.h>
-#include "g_NCR5380.h"
-#include "NCR5380.h"
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/isa.h>
 #include <linux/pnp.h>
 #include <linux/interrupt.h>
 
+/* Definitions for the core NCR5380 driver. */
+
+#define NCR5380_read(reg) \
+	ioread8(hostdata->io + hostdata->offset + (reg))
+#define NCR5380_write(reg, value) \
+	iowrite8(value, hostdata->io + hostdata->offset + (reg))
+
+#define NCR5380_implementation_fields \
+	int offset; \
+	int c400_ctl_status; \
+	int c400_blk_cnt; \
+	int c400_host_buf; \
+	int io_width
+
+#define NCR5380_dma_xfer_len            generic_NCR5380_dma_xfer_len
+#define NCR5380_dma_recv_setup          generic_NCR5380_pread
+#define NCR5380_dma_send_setup          generic_NCR5380_pwrite
+#define NCR5380_dma_residual            NCR5380_dma_residual_none
+
+#define NCR5380_intr                    generic_NCR5380_intr
+#define NCR5380_queue_command           generic_NCR5380_queue_command
+#define NCR5380_abort                   generic_NCR5380_abort
+#define NCR5380_bus_reset               generic_NCR5380_bus_reset
+#define NCR5380_info                    generic_NCR5380_info
+
+#define NCR5380_io_delay(x)             udelay(x)
+
+#include "NCR5380.h"
+
+#define DRV_MODULE_NAME "g_NCR5380"
+
+#define NCR53C400_mem_base 0x3880
+#define NCR53C400_host_buffer 0x3900
+#define NCR53C400_region_size 0x3a00
+
+#define BOARD_NCR5380 0
+#define BOARD_NCR53C400 1
+#define BOARD_NCR53C400A 2
+#define BOARD_DTC3181E 3
+#define BOARD_HP_C2502 4
+
+#define IRQ_AUTO 254
+
 #define MAX_CARDS 8
 
 /* old-style parameters for compatibility */
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
deleted file mode 100644
index 81b22d9..0000000
--- a/drivers/scsi/g_NCR5380.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * Generic Generic NCR5380 driver defines
- *
- * Copyright 1993, Drew Eckhardt
- *	Visionary Computing
- *	(Unix and Linux consulting and custom programming)
- *	drew@colorado.edu
- *      +1 (303) 440-4894
- *
- * NCR53C400 extensions (c) 1994,1995,1996, Kevin Lentin
- *    K.Lentin@cs.monash.edu.au
- */
-
-#ifndef GENERIC_NCR5380_H
-#define GENERIC_NCR5380_H
-
-#define DRV_MODULE_NAME "g_NCR5380"
-
-#define NCR5380_read(reg) \
-	ioread8(hostdata->io + hostdata->offset + (reg))
-#define NCR5380_write(reg, value) \
-	iowrite8(value, hostdata->io + hostdata->offset + (reg))
-
-#define NCR5380_implementation_fields \
-	int offset; \
-	int c400_ctl_status; \
-	int c400_blk_cnt; \
-	int c400_host_buf; \
-	int io_width;
-
-#define NCR53C400_mem_base 0x3880
-#define NCR53C400_host_buffer 0x3900
-#define NCR53C400_region_size 0x3a00
-
-#define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
-#define NCR5380_dma_recv_setup		generic_NCR5380_pread
-#define NCR5380_dma_send_setup		generic_NCR5380_pwrite
-#define NCR5380_dma_residual		NCR5380_dma_residual_none
-
-#define NCR5380_intr generic_NCR5380_intr
-#define NCR5380_queue_command generic_NCR5380_queue_command
-#define NCR5380_abort generic_NCR5380_abort
-#define NCR5380_bus_reset generic_NCR5380_bus_reset
-#define NCR5380_info generic_NCR5380_info
-
-#define NCR5380_io_delay(x)		udelay(x)
-
-#define BOARD_NCR5380	0
-#define BOARD_NCR53C400	1
-#define BOARD_NCR53C400A 2
-#define BOARD_DTC3181E	3
-#define BOARD_HP_C2502	4
-
-#define IRQ_AUTO	254
-
-#endif /* GENERIC_NCR5380_H */
diff --git a/drivers/scsi/sun3_scsi.c b/drivers/scsi/sun3_scsi.c
index 166f466..14cebaf 100644
--- a/drivers/scsi/sun3_scsi.c
+++ b/drivers/scsi/sun3_scsi.c
@@ -34,7 +34,6 @@
 #include <asm/dvma.h>
 
 #include <scsi/scsi_host.h>
-#include "sun3_scsi.h"
 
 /* minimum number of bytes to do dma on */
 #define DMA_MIN_SIZE                    129
@@ -58,6 +57,85 @@
 
 #include "NCR5380.h"
 
+/* dma regs start at regbase + 8, directly after the NCR regs */
+struct sun3_dma_regs {
+	unsigned short dma_addr_hi; /* vme only */
+	unsigned short dma_addr_lo; /* vme only */
+	unsigned short dma_count_hi; /* vme only */
+	unsigned short dma_count_lo; /* vme only */
+	unsigned short udc_data; /* udc dma data reg (obio only) */
+	unsigned short udc_addr; /* uda dma addr reg (obio only) */
+	unsigned short fifo_data; /* fifo data reg,
+	                           * holds extra byte on odd dma reads
+	                           */
+	unsigned short fifo_count;
+	unsigned short csr; /* control/status reg */
+	unsigned short bpack_hi; /* vme only */
+	unsigned short bpack_lo; /* vme only */
+	unsigned short ivect; /* vme only */
+	unsigned short fifo_count_hi; /* vme only */
+};
+
+/* ucd chip specific regs - live in dvma space */
+struct sun3_udc_regs {
+	unsigned short rsel; /* select regs to load */
+	unsigned short addr_hi; /* high word of addr */
+	unsigned short addr_lo; /* low word */
+	unsigned short count; /* words to be xfer'd */
+	unsigned short mode_hi; /* high word of channel mode */
+	unsigned short mode_lo; /* low word of channel mode */
+};
+
+/* addresses of the udc registers */
+#define UDC_MODE 0x38
+#define UDC_CSR 0x2e /* command/status */
+#define UDC_CHN_HI 0x26 /* chain high word */
+#define UDC_CHN_LO 0x22 /* chain lo word */
+#define UDC_CURA_HI 0x1a /* cur reg A high */
+#define UDC_CURA_LO 0x0a /* cur reg A low */
+#define UDC_CURB_HI 0x12 /* cur reg B high */
+#define UDC_CURB_LO 0x02 /* cur reg B low */
+#define UDC_MODE_HI 0x56 /* mode reg high */
+#define UDC_MODE_LO 0x52 /* mode reg low */
+#define UDC_COUNT 0x32 /* words to xfer */
+
+/* some udc commands */
+#define UDC_RESET 0
+#define UDC_CHN_START 0xa0 /* start chain */
+#define UDC_INT_ENABLE 0x32 /* channel 1 int on */
+
+/* udc mode words */
+#define UDC_MODE_HIWORD 0x40
+#define UDC_MODE_LSEND 0xc2
+#define UDC_MODE_LRECV 0xd2
+
+/* udc reg selections */
+#define UDC_RSEL_SEND 0x282
+#define UDC_RSEL_RECV 0x182
+
+/* bits in csr reg */
+#define CSR_DMA_ACTIVE 0x8000
+#define CSR_DMA_CONFLICT 0x4000
+#define CSR_DMA_BUSERR 0x2000
+
+#define CSR_FIFO_EMPTY 0x400 /* fifo flushed? */
+#define CSR_SDB_INT 0x200 /* sbc interrupt pending */
+#define CSR_DMA_INT 0x100 /* dma interrupt pending */
+
+#define CSR_LEFT 0xc0
+#define CSR_LEFT_3 0xc0
+#define CSR_LEFT_2 0x80
+#define CSR_LEFT_1 0x40
+#define CSR_PACK_ENABLE 0x20
+
+#define CSR_DMA_ENABLE 0x10
+
+#define CSR_SEND 0x8 /* 1 = send  0 = recv */
+#define CSR_FIFO 0x2 /* reset fifo */
+#define CSR_INTR 0x4 /* interrupt enable */
+#define CSR_SCSI 0x1
+
+#define VME_DATA24 0x3d00
 
 extern int sun3_map_test(unsigned long, char *);
 
diff --git a/drivers/scsi/sun3_scsi.h b/drivers/scsi/sun3_scsi.h
deleted file mode 100644
index d22745f..0000000
--- a/drivers/scsi/sun3_scsi.h
+++ /dev/null
@@ -1,102 +0,0 @@
-/*
- * Sun3 SCSI stuff by Erik Verbruggen (erik@bigmama.xtdnet.nl)
- *
- * Sun3 DMA additions by Sam Creasey (sammy@sammy.net)
- *
- * Adapted from mac_scsinew.h:
- */
-/*
- * Cumana Generic NCR5380 driver defines
- *
- * Copyright 1993, Drew Eckhardt
- *	Visionary Computing
- *	(Unix and Linux consulting and custom programming)
- *	drew@colorado.edu
- *      +1 (303) 440-4894
- */
-
-#ifndef SUN3_SCSI_H
-#define SUN3_SCSI_H
-
-/* additional registers - mainly DMA control regs */
-/* these start at regbase + 8 -- directly after the NCR regs */
-struct sun3_dma_regs {
-	unsigned short dma_addr_hi; /* vme only */
-	unsigned short dma_addr_lo; /* vme only */
-	unsigned short dma_count_hi; /* vme only */
-	unsigned short dma_count_lo; /* vme only */
-	unsigned short udc_data; /* udc dma data reg (obio only) */
-	unsigned short udc_addr; /* uda dma addr reg (obio only) */
-	unsigned short fifo_data; /* fifo data reg, holds extra byte on
-				     odd dma reads */
-	unsigned short fifo_count; 
-	unsigned short csr; /* control/status reg */
-	unsigned short bpack_hi; /* vme only */
-	unsigned short bpack_lo; /* vme only */
-	unsigned short ivect; /* vme only */
-	unsigned short fifo_count_hi; /* vme only */
-};
-
-/* ucd chip specific regs - live in dvma space */
-struct sun3_udc_regs {
-     unsigned short rsel; /* select regs to load */
-     unsigned short addr_hi; /* high word of addr */
-     unsigned short addr_lo; /* low word */
-     unsigned short count; /* words to be xfer'd */
-     unsigned short mode_hi; /* high word of channel mode */
-     unsigned short mode_lo; /* low word of channel mode */
-};
-
-/* addresses of the udc registers */
-#define UDC_MODE 0x38 
-#define UDC_CSR 0x2e /* command/status */
-#define UDC_CHN_HI 0x26 /* chain high word */
-#define UDC_CHN_LO 0x22 /* chain lo word */
-#define UDC_CURA_HI 0x1a /* cur reg A high */
-#define UDC_CURA_LO 0x0a /* cur reg A low */
-#define UDC_CURB_HI 0x12 /* cur reg B high */
-#define UDC_CURB_LO 0x02 /* cur reg B low */
-#define UDC_MODE_HI 0x56 /* mode reg high */
-#define UDC_MODE_LO 0x52 /* mode reg low */
-#define UDC_COUNT 0x32 /* words to xfer */
-
-/* some udc commands */
-#define UDC_RESET 0
-#define UDC_CHN_START 0xa0 /* start chain */
-#define UDC_INT_ENABLE 0x32 /* channel 1 int on */
-
-/* udc mode words */
-#define UDC_MODE_HIWORD 0x40
-#define UDC_MODE_LSEND 0xc2
-#define UDC_MODE_LRECV 0xd2
-
-/* udc reg selections */
-#define UDC_RSEL_SEND 0x282
-#define UDC_RSEL_RECV 0x182
-
-/* bits in csr reg */
-#define CSR_DMA_ACTIVE 0x8000
-#define CSR_DMA_CONFLICT 0x4000
-#define CSR_DMA_BUSERR 0x2000
-
-#define CSR_FIFO_EMPTY 0x400 /* fifo flushed? */
-#define CSR_SDB_INT 0x200 /* sbc interrupt pending */
-#define CSR_DMA_INT 0x100 /* dma interrupt pending */
-
-#define CSR_LEFT 0xc0
-#define CSR_LEFT_3 0xc0
-#define CSR_LEFT_2 0x80
-#define CSR_LEFT_1 0x40
-#define CSR_PACK_ENABLE 0x20
-
-#define CSR_DMA_ENABLE 0x10
-
-#define CSR_SEND 0x8 /* 1 = send  0 = recv */
-#define CSR_FIFO 0x2 /* reset fifo */
-#define CSR_INTR 0x4 /* interrupt enable */
-#define CSR_SCSI 0x1 
-
-#define VME_DATA24 0x3d00
-
-#endif /* SUN3_SCSI_H */
-
-- 
2.10.2

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Martin K. Petersen]

>>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:

Finn> Michael, Ondrej, can I get you to review/test please?

Pretty please!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Michael Schmitz]

Martin, Finn,

I'll get to that on the weekend - Auckland Anniversary Day coming up so
plenty of time.

Cheers,

	Michael

Am 26.01.2017 um 12:25 schrieb Martin K. Petersen:
>>>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:
> 
> Finn> Michael, Ondrej, can I get you to review/test please?
> 
> Pretty please!
> 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Martin K. Petersen]

>>>>> "Michael" == Michael Schmitz <schmitzmic@gmail.com> writes:

Michael> Martin, Finn, I'll get to that on the weekend - Auckland
Michael> Anniversary Day coming up so plenty of time.

Great, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Monday 16 January 2017 00:50:57 Finn Thain wrote:
> This series removes some unused code and related comments,
> addresses the warnings generated by 'make W=1' and 'make C=1'
> and fixes a theoretical bug in the bus reset method in atari_scsi.
>
> There's also a patch to add a missing error check during target
> selection. The only target I tested was a QUANTUM DAYTONA514S disk
> as that's all I have access to right now. Some testing with other
> targets would be prudent.
>
> Michael, Ondrej, can I get you to review/test please?

Tested on HP C2502 (53C400A chip), Canon FG2-5202 (53C400 chip), DTC-3181L 
(DTCT-436P chip) and MS-PNR (53C400A chip) ISA cards - everything works fine!

Targets tested:
QUANTUM  LP240S GM240S01X
IBM      DORS-32160
IBM      0663L12

Thanks.

Tested-by: Ondrej Zary <linux@rainbow-software.org>

-- 
Ondrej Zary

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Sat, 28 Jan 2017, Ondrej Zary wrote:

> On Monday 16 January 2017 00:50:57 Finn Thain wrote:
> > This series removes some unused code and related comments, addresses 
> > the warnings generated by 'make W=1' and 'make C=1' and fixes a 
> > theoretical bug in the bus reset method in atari_scsi.
> >
> > There's also a patch to add a missing error check during target 
> > selection. The only target I tested was a QUANTUM DAYTONA514S disk as 
> > that's all I have access to right now. Some testing with other targets 
> > would be prudent.
> >
> > Michael, Ondrej, can I get you to review/test please?
> 
> Tested on HP C2502 (53C400A chip), Canon FG2-5202 (53C400 chip), 
> DTC-3181L (DTCT-436P chip) and MS-PNR (53C400A chip) ISA cards - 
> everything works fine!
> 
> Targets tested:
> QUANTUM  LP240S GM240S01X
> IBM      DORS-32160
> IBM      0663L12
> 
> Thanks.
> 
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> 

Very helpful. Thank you, Ondrej.

-- 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Michael Schmitz]

Hi Finn,


Am 16.01.2017 um 12:50 schrieb Finn Thain:
> This series removes some unused code and related comments,
> addresses the warnings generated by 'make W=1' and 'make C=1'
> and fixes a theoretical bug in the bus reset method in atari_scsi.
> 
> There's also a patch to add a missing error check during target
> selection. The only target I tested was a QUANTUM DAYTONA514S disk
> as that's all I have access to right now. Some testing with other
> targets would be prudent.
> 
> Michael, Ondrej, can I get you to review/test please?

No regressions during testing on my Falcon both using default settings,
i.e. can_queue=1 or internal queueing (can_queue=4, cmd_per_lun=2), and
the legacy IDE driver.

I've seen a bus reset executed when reloading the module (bus jammed for
some reason apparently), so that code path has been tested as well.

No change in the weird behaviour of my SCSI-SATA adapter, but perhaps
that wasn't unexpected.

Targets: Disks Seagate ST34520W and IBM DORS-32160W, CD-ROM Plextor PX-40TS.

Tested-by: Michael Schmitz <schmitzmic@gmail.com>

> 
> 
> Finn Thain (6):
>   ncr5380: Shorten host info string by removing unused option macros
>   ncr5380: Clean up dead code and redundant macro usage
>   ncr5380: Reduce #include files
>   ncr5380: Resolve various static checker warnings
>   ncr5380: Improve target selection robustness
>   atari_scsi: Reset DMA during bus reset only under ST-DMA lock
> 
>  drivers/scsi/NCR5380.c    |  64 ++++++++++-------------------
>  drivers/scsi/NCR5380.h    |  17 +-------
>  drivers/scsi/atari_scsi.c |  36 ++--------------
>  drivers/scsi/g_NCR5380.c  |  45 +++++++++++++++++++-
>  drivers/scsi/g_NCR5380.h  |  56 -------------------------
>  drivers/scsi/mac_scsi.c   |   8 ++--
>  drivers/scsi/sun3_scsi.c  |  83 +++++++++++++++++++++++++++++++++++--
>  drivers/scsi/sun3_scsi.h  | 102 ----------------------------------------------
>  8 files changed, 152 insertions(+), 259 deletions(-)
>  delete mode 100644 drivers/scsi/g_NCR5380.h
>  delete mode 100644 drivers/scsi/sun3_scsi.h
> 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Mon, 30 Jan 2017, Michael Schmitz wrote:

> > Michael, Ondrej, can I get you to review/test please?
> 
> No regressions during testing on my Falcon both using default settings, 
> i.e. can_queue=1 or internal queueing (can_queue=4, cmd_per_lun=2), and 
> the legacy IDE driver.
> 
> I've seen a bus reset executed when reloading the module (bus jammed for 
> some reason apparently), so that code path has been tested as well.
> 
> No change in the weird behaviour of my SCSI-SATA adapter, but perhaps 
> that wasn't unexpected.
> 

Well, I guess we eliminated one possibility. Could be a bus termination 
issue perhaps?

> Targets: Disks Seagate ST34520W and IBM DORS-32160W, CD-ROM Plextor 
> PX-40TS.
> 
> Tested-by: Michael Schmitz <schmitzmic@gmail.com>

Thanks, Michael.

-- 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Sunday 29 January 2017 02:05:02 Finn Thain wrote:
> On Sat, 28 Jan 2017, Ondrej Zary wrote:
> > On Monday 16 January 2017 00:50:57 Finn Thain wrote:
> > > This series removes some unused code and related comments, addresses
> > > the warnings generated by 'make W=1' and 'make C=1' and fixes a
> > > theoretical bug in the bus reset method in atari_scsi.
> > >
> > > There's also a patch to add a missing error check during target
> > > selection. The only target I tested was a QUANTUM DAYTONA514S disk as
> > > that's all I have access to right now. Some testing with other targets
> > > would be prudent.
> > >
> > > Michael, Ondrej, can I get you to review/test please?
> >
> > Tested on HP C2502 (53C400A chip), Canon FG2-5202 (53C400 chip),
> > DTC-3181L (DTCT-436P chip) and MS-PNR (53C400A chip) ISA cards -
> > everything works fine!
> >
> > Targets tested:
> > QUANTUM  LP240S GM240S01X
> > IBM      DORS-32160
> > IBM      0663L12
> >
> > Thanks.
> >
> > Tested-by: Ondrej Zary <linux@rainbow-software.org>
>
> Very helpful. Thank you, Ondrej.

Also tested two CD-ROM drives and they didn't work (machine hangs). They
didn't work before and looks like they never worked with PDMA.

The fundamental problem of PDMA is that it either transfers all required data
or it breaks horribly. Even when I added timeouts to the possibly infinite
loops (to avoid hangs), the chip remained in some bad state. Sometimes the
53C80 gated IRQ check triggers. This can be hopefully fixed but there is a HW
limitation: if less than 128 bytes were read from SCSI device, they get lost
in chip buffer (the 128B buffers don't swap until full).

Fortunately, most of the requests for too much data are bogus. The problem is
that generic_NCR5380_dma_xfer_len() uses cmd->transfersize which seems to be
wrong. All other NCR5380 drivers use cmd->SCp.this_residual.

This is also why rescan-scsi-bus hangs (cmd->transfersize is 8192 but
cmd->SCp.this_residual is only 96).

This quick fix allows CD-ROM and also rescan-scsi-bus to work.
But it's not complete. Seems that we need:
 - fix pread and pwrite to terminate gracefully
 - something like atari_scsi_dma_xfer_len to allow DMA only for block commands

@@ -588,22 +619,19 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                         struct scsi_cmnd *cmd)
 {
-       int transfersize = cmd->transfersize;
+       int transfersize = cmd->SCp.this_residual;

        if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
                return 0;

-       /* Limit transfers to 32K, for xx400 & xx406
-        * pseudoDMA that transfers in 128 bytes blocks.
-        */
-       if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
-           !(cmd->SCp.this_residual % transfersize))
-               transfersize = 32 * 1024;
-
        /* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
        if (transfersize % 128)
                transfersize = 0;

+       /* Limit transfers to 32K */
+       if (transfersize > 32 * 1024)
+               transfersize = 32 * 1024;
+
        return transfersize;
 }




-- 
Ondrej Zary

g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Mon, 30 Jan 2017, Ondrej Zary wrote:

> 
> Also tested two CD-ROM drives and they didn't work (machine hangs). They 
> didn't work before and looks like they never worked with PDMA.
> 
> The fundamental problem of PDMA is that it either transfers all required 
> data or it breaks horribly. Even when I added timeouts to the possibly 
> infinite loops (to avoid hangs), the chip remained in some bad state. 
> Sometimes the 53C80 gated IRQ check triggers. This can be hopefully 
> fixed but there is a HW limitation: if less than 128 bytes were read 
> from SCSI device, they get lost in chip buffer (the 128B buffers don't 
> swap until full).
> 

The core driver allows wrapper drivers to inhibit PDMA e.g. for inbound 
transfers smaller than 128 bytes. This is used by atari_scsi and sun3_scsi 
(as you probably realize). For example, atari_scsi now uses 
cmd->sc_data_direction to find out the direction of the DMA transfer.

> Fortunately, most of the requests for too much data are bogus. The 
> problem is that generic_NCR5380_dma_xfer_len() uses cmd->transfersize 
> which seems to be wrong. All other NCR5380 drivers use 
> cmd->SCp.this_residual.
> 

Yes, this always looked wrong to me. Much of the logic in 
generic_NCR5380_dma_xfer_len() looks suspicious but I see you've addressed 
this below. Thanks for looking into this.

> This is also why rescan-scsi-bus hangs (cmd->transfersize is 8192 but 
> cmd->SCp.this_residual is only 96).
> 

Great, that resolves that mystery.

> This quick fix allows CD-ROM and also rescan-scsi-bus to work.
> But it's not complete. Seems that we need:
>  - fix pread and pwrite to terminate gracefully

mac_scsi.c probably offers a reasonable example of a PDMA transfer 
algorithm that is resiliant to timeouts.

See also the cmd->device->borken logic in the core driver. This way, 
generic_NCR5380_pread() or generic_NCR5380_pwrite() can inhibit further 
PDMA to that device if need be.

>  - something like atari_scsi_dma_xfer_len to allow DMA only for block 
>    commands
> 

Are you trying to figure out which commands are going to disconnect during 
a transfer? This is really a function of the firmware in the target; there 
are no good heuristics AFAICT, so the PDMA algorithm has to be robust. 
mac_scsi has to cope with this too.

Does the problem go away when you assign no IRQ? When instance->irq == 
NO_IRQ, the core driver will inhibit disconnect privileges.

> @@ -588,22 +619,19 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
>                                          struct scsi_cmnd *cmd)
>  {
> -       int transfersize = cmd->transfersize;
> +       int transfersize = cmd->SCp.this_residual;
> 
>         if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
>                 return 0;
> 
> -       /* Limit transfers to 32K, for xx400 & xx406
> -        * pseudoDMA that transfers in 128 bytes blocks.
> -        */
> -       if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
> -           !(cmd->SCp.this_residual % transfersize))
> -               transfersize = 32 * 1024;
> -
>         /* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
>         if (transfersize % 128)
>                 transfersize = 0;
> 
> +       /* Limit transfers to 32K */
> +       if (transfersize > 32 * 1024)
> +               transfersize = 32 * 1024;
> +
>         return transfersize;

I would prefer to see,

#define G_NCR5380_DMA_MAX_SIZE	32768

...

	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);

Thanks.

-- 

>  }
> 
> 
> 
> 
> 

Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Martin K. Petersen]

>>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:

Finn> This series removes some unused code and related comments,
Finn> addresses the warnings generated by 'make W=1' and 'make C=1' and
Finn> fixes a theoretical bug in the bus reset method in atari_scsi.

Finn> There's also a patch to add a missing error check during target
Finn> selection. The only target I tested was a QUANTUM DAYTONA514S disk
Finn> as that's all I have access to right now. Some testing with other
Finn> targets would be prudent.

Applied to 4.11/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
[...]
> Are you trying to figure out which commands are going to disconnect during
> a transfer? This is really a function of the firmware in the target; there
> are no good heuristics AFAICT, so the PDMA algorithm has to be robust.
> mac_scsi has to cope with this too.
>
> Does the problem go away when you assign no IRQ? When instance->irq ==
> NO_IRQ, the core driver will inhibit disconnect privileges.

Yes, it seems to run fine with IRQ/disconnect disabled.
With IRQ enabled, "dd if=/dev/sr0 of=anything" stops after a while. I get 
gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.

When I enable interrupts during PDMA (like the removed UNSAFE macro did), the 
problems go away. I see an IRQ after each pread call.
(had to disable "no 53C80 gated irq after transfer" and "no end dma signal" 
messages to reduce log spam)

-- 
Ondrej Zary

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Thu, 16 Feb 2017, Ondrej Zary wrote:

> On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> [...]
> > Are you trying to figure out which commands are going to disconnect 
> > during a transfer? This is really a function of the firmware in the 
> > target; there are no good heuristics AFAICT, so the PDMA algorithm has 
> > to be robust. mac_scsi has to cope with this too.
> >
> > Does the problem go away when you assign no IRQ? When instance->irq == 
> > NO_IRQ, the core driver will inhibit disconnect privileges.
> 
> Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ 
> enabled, "dd if=/dev/sr0 of=anything" stops after a while.

When you say "stops", do you mean an infinite loop in 
generic_NCR5380_pread or does the loop complete (which would presumably 
leave the target stuck in DATA IN phase, and a scsi command timeout would 
probably follow after 30 seconds...)

> I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> 

You can use NCR5380_print() to get a decoded register dump.

When I decode the above values I get,

BASR   = 0x10 = BASR_IRQ
MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO

Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a 
phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN, 
which shows that the target has given up on the DATA IN phase and is 
presumably trying to send a DISCONNECT message.

> When I enable interrupts during PDMA (like the removed UNSAFE macro 
> did), the problems go away. I see an IRQ after each pread call.

You shouldn't need an interrupt, because NCR5380_dma_complete() gets 
called regardless. AFAICT, the only difference is the timing, which 
becomes less predictable. Calling spinlock_irq_restore() before the 
transfer seems to create a race condition between hostdata->dma_len store 
and load.

I think that the pread call has not yet returned when the interrupt fires 
and NCR5380_dma_complete() is called. Hence hostdata->dma_len has not yet 
been initialized. So when NCR5380_dma_complete() is called by the 
interrupt handler, SCp.this_residual will not change at all because 
hostdata->dma_len is still zero.

> (had to disable "no 53C80 gated irq after transfer" and "no end dma 
> signal" messages to reduce log spam)
> 

That may provide a quick way to detect the failed PDMA transfer, at least 
for testing purposes. There may be a more conclusive test for a partial 
transfer.

We could to implement something like macscsi_dma_residual() to take care 
of a failed PDMA transfer. That way, the failure can be taken into account 
when NCR5380_dma_complete() is called at the end of the transfer.

See patch below for example. It should confirm the theory above though it 
really needs some timeouts added (NCR5380_poll_politely()). And perhaps we 
could do something more clever than retry indefinitely, though we can 
still rely on the command timeout.

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6f9665d..75cfaf3 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -497,15 +497,17 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 		blocks--;
 	}
 
+	hostdata->pdma_residual = 0;
+
 	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
-		printk("53C400r: no 53C80 gated irq after transfer");
+		hostdata->pdma_residual = hostdata->dma_len;
 
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
 		;
 
 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		hostdata->pdma_residual = hostdata->dma_len;
 		
 	return 0;
 }
@@ -576,12 +578,14 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 		/* FIXME - no timeout */
 	}
 
-	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
-		printk(KERN_ERR "53C400w: no end dma signal\n");
-	}
+	hostdata->pdma_residual = 0;
+
+	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
+		hostdata->pdma_residual = hostdata->dma_len;
 
 	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
 		; 	// TIMEOUT
+
 	return 0;
 }
 
@@ -607,6 +611,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
 	return transfersize;
 }
 
+static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
+{
+	return hostdata->pdma_residual;
+}
+
 /*
  *	Include the NCR5380 core code that we build our driver around	
  */
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 81b22d9..08c91b7 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -26,7 +26,8 @@
 	int c400_ctl_status; \
 	int c400_blk_cnt; \
 	int c400_host_buf; \
-	int io_width;
+	int io_width; \
+	int pdma_residual;
 
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
@@ -35,7 +36,7 @@
 #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
 #define NCR5380_dma_recv_setup		generic_NCR5380_pread
 #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
-#define NCR5380_dma_residual		NCR5380_dma_residual_none
+#define NCR5380_dma_residual		generic_NCR5380_dma_residual
 
 #define NCR5380_intr generic_NCR5380_intr
 #define NCR5380_queue_command generic_NCR5380_queue_command

-- 

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > [...]
> >
> > > Are you trying to figure out which commands are going to disconnect
> > > during a transfer? This is really a function of the firmware in the
> > > target; there are no good heuristics AFAICT, so the PDMA algorithm has
> > > to be robust. mac_scsi has to cope with this too.
> > >
> > > Does the problem go away when you assign no IRQ? When instance->irq ==
> > > NO_IRQ, the core driver will inhibit disconnect privileges.
> >
> > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ
> > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
>
> When you say "stops", do you mean an infinite loop in
> generic_NCR5380_pread or does the loop complete (which would presumably
> leave the target stuck in DATA IN phase, and a scsi command timeout would
> probably follow after 30 seconds...)

I've added timeouts to the possibly-infinite loops. It times out waiting for 
the host buffer to become ready. Then everything breaks.
Now I found why: pread() returns without waiting for the 53C80 registers to be 
ready. Adding the wait allows to continue in PIO mode with "tag#0 switching 
to slow handshake".

> > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
>
> You can use NCR5380_print() to get a decoded register dump.
>
> When I decode the above values I get,
>
> BASR   = 0x10 = BASR_IRQ
> MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
>
> Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a
> phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN,
> which shows that the target has given up on the DATA IN phase and is
> presumably trying to send a DISCONNECT message.

Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors. When the 
2nd sector is not ready in the drive internal buffer, the drive probably 
disconnects which breaks the fragile pdma transfer. When the transfersize is 
limited to 2048 bytes, the problem goes away.

The problem also went away with enabled interrupts because I had some debug 
printks added which were slowing down the transfer enough for the drive (SONY 
CDU-55S) to keep up and not disconnect. Got the same result by inserting 
udelay(100) after each 128 byte block trasnferred in pread().

> > When I enable interrupts during PDMA (like the removed UNSAFE macro
> > did), the problems go away. I see an IRQ after each pread call.
>
> You shouldn't need an interrupt, because NCR5380_dma_complete() gets
> called regardless. AFAICT, the only difference is the timing, which
> becomes less predictable. Calling spinlock_irq_restore() before the
> transfer seems to create a race condition between hostdata->dma_len store
> and load.
>
> I think that the pread call has not yet returned when the interrupt fires
> and NCR5380_dma_complete() is called. Hence hostdata->dma_len has not yet
> been initialized. So when NCR5380_dma_complete() is called by the
> interrupt handler, SCp.this_residual will not change at all because
> hostdata->dma_len is still zero.
>
> > (had to disable "no 53C80 gated irq after transfer" and "no end dma
> > signal" messages to reduce log spam)
>
> That may provide a quick way to detect the failed PDMA transfer, at least
> for testing purposes. There may be a more conclusive test for a partial
> transfer.
>
> We could to implement something like macscsi_dma_residual() to take care
> of a failed PDMA transfer. That way, the failure can be taken into account
> when NCR5380_dma_complete() is called at the end of the transfer.
>
> See patch below for example. It should confirm the theory above though it
> really needs some timeouts added (NCR5380_poll_politely()). And perhaps we
> could do something more clever than retry indefinitely, though we can
> still rely on the command timeout.

Thanks for idea

> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6f9665d..75cfaf3 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -497,15 +497,17 @@ static inline int generic_NCR5380_pread(struct
> NCR5380_hostdata *hostdata, blocks--;
>  	}
>
> +	hostdata->pdma_residual = 0;
> +
>  	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
> -		printk("53C400r: no 53C80 gated irq after transfer");
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
>  		;
>
>  	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> -		printk(KERN_ERR "53C400r: no end dma signal\n");
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	return 0;
>  }
> @@ -576,12 +578,14 @@ static inline int generic_NCR5380_pwrite(struct
> NCR5380_hostdata *hostdata, /* FIXME - no timeout */
>  	}
>
> -	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
> -		printk(KERN_ERR "53C400w: no end dma signal\n");
> -	}
> +	hostdata->pdma_residual = 0;
> +
> +	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> +		hostdata->pdma_residual = hostdata->dma_len;
>
>  	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
>  		; 	// TIMEOUT
> +
>  	return 0;
>  }
>
> @@ -607,6 +611,11 @@ static int generic_NCR5380_dma_xfer_len(struct
> NCR5380_hostdata *hostdata, return transfersize;
>  }
>
> +static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
> +{
> +	return hostdata->pdma_residual;
> +}
> +
>  /*
>   *	Include the NCR5380 core code that we build our driver around
>   */
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 81b22d9..08c91b7 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -26,7 +26,8 @@
>  	int c400_ctl_status; \
>  	int c400_blk_cnt; \
>  	int c400_host_buf; \
> -	int io_width;
> +	int io_width; \
> +	int pdma_residual;
>
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
> @@ -35,7 +36,7 @@
>  #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
>  #define NCR5380_dma_recv_setup		generic_NCR5380_pread
>  #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
> -#define NCR5380_dma_residual		NCR5380_dma_residual_none
> +#define NCR5380_dma_residual		generic_NCR5380_dma_residual
>
>  #define NCR5380_intr generic_NCR5380_intr
>  #define NCR5380_queue_command generic_NCR5380_queue_command


-- 
Ondrej Zary

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Sat, 18 Feb 2017, Ondrej Zary wrote:

> On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> > On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > > [...]
> > >
> > > > Are you trying to figure out which commands are going to 
> > > > disconnect during a transfer? This is really a function of the 
> > > > firmware in the target; there are no good heuristics AFAICT, so 
> > > > the PDMA algorithm has to be robust. mac_scsi has to cope with 
> > > > this too.
> > > >
> > > > Does the problem go away when you assign no IRQ? When 
> > > > instance->irq == NO_IRQ, the core driver will inhibit disconnect 
> > > > privileges.
> > >
> > > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ 
> > > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
> >
> > When you say "stops", do you mean an infinite loop in 
> > generic_NCR5380_pread or does the loop complete (which would 
> > presumably leave the target stuck in DATA IN phase, and a scsi command 
> > timeout would probably follow after 30 seconds...)
> 
> I've added timeouts to the possibly-infinite loops. It times out waiting 
> for the host buffer to become ready.

In mac_scsi you'll find that the PDMA loop exploits the 15ms poll_politely 
time limit to give the target device time to catch up. You might want to 
do something similar.

> Then everything breaks. Now I found why: pread() returns without waiting 
> for the 53C80 registers to be ready.

Ouch! You can't return control to the core driver when the 53C80 core is 
unavailable. That would need special handling: the core driver would have 
to fail the scsi command and reset the device (and bus), based on the 
result you return from NCR5380_dma_recv_setup/NCR5380_dma_send_setup.

> Adding the wait allows to continue in PIO mode with "tag#0 switching to 
> slow handshake".
> 

I don't think this is the code path you want. The target isn't really 
broken. But yes, we could use PIO as a slow workaround for fragile PDMA 
logic.

> > > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> >
> > You can use NCR5380_print() to get a decoded register dump.
> >
> > When I decode the above values I get,
> >
> > BASR   = 0x10 = BASR_IRQ
> > MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> > STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
> >
> > Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a 
> > phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN, 
> > which shows that the target has given up on the DATA IN phase and is 
> > presumably trying to send a DISCONNECT message.
> 
> Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors. 
> When the 2nd sector is not ready in the drive internal buffer, the drive 
> probably disconnects which breaks the fragile pdma transfer. When the 
> transfersize is limited to 2048 bytes, the problem goes away.
> 

OK.

> The problem also went away with enabled interrupts because I had some 
> debug printks added which were slowing down the transfer enough for the 
> drive (SONY CDU-55S) to keep up and not disconnect. Got the same result 
> by inserting udelay(100) after each 128 byte block trasnferred in 
> pread().
> 

Well, yeah, if you change the timing and omit to mention that, as well as 
changing the spinlock_irq_save/restore pairs, then it's going to be 
difficult for me to make sense of your results. Anyway, I'm glad that you 
got to the bottom of this mystery.

> > > When I enable interrupts during PDMA (like the removed UNSAFE macro 
> > > did), the problems go away. I see an IRQ after each pread call.
> >

-- 

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Ondrej Zary]

On Sunday 19 February 2017 00:27:55 Finn Thain wrote:
> On Sat, 18 Feb 2017, Ondrej Zary wrote:
> > On Friday 17 February 2017 23:38:12 Finn Thain wrote:
> > > On Thu, 16 Feb 2017, Ondrej Zary wrote:
> > > > On Tuesday 31 January 2017 02:31:45 Finn Thain wrote:
> > > > [...]
> > > >
> > > > > Are you trying to figure out which commands are going to
> > > > > disconnect during a transfer? This is really a function of the
> > > > > firmware in the target; there are no good heuristics AFAICT, so
> > > > > the PDMA algorithm has to be robust. mac_scsi has to cope with
> > > > > this too.
> > > > >
> > > > > Does the problem go away when you assign no IRQ? When
> > > > > instance->irq == NO_IRQ, the core driver will inhibit disconnect
> > > > > privileges.
> > > >
> > > > Yes, it seems to run fine with IRQ/disconnect disabled. With IRQ
> > > > enabled, "dd if=/dev/sr0 of=anything" stops after a while.
> > >
> > > When you say "stops", do you mean an infinite loop in
> > > generic_NCR5380_pread or does the loop complete (which would
> > > presumably leave the target stuck in DATA IN phase, and a scsi command
> > > timeout would probably follow after 30 seconds...)
> >
> > I've added timeouts to the possibly-infinite loops. It times out waiting
> > for the host buffer to become ready.
>
> In mac_scsi you'll find that the PDMA loop exploits the 15ms poll_politely
> time limit to give the target device time to catch up. You might want to
> do something similar.
>
> > Then everything breaks. Now I found why: pread() returns without waiting
> > for the 53C80 registers to be ready.
>
> Ouch! You can't return control to the core driver when the 53C80 core is
> unavailable. That would need special handling: the core driver would have
> to fail the scsi command and reset the device (and bus), based on the
> result you return from NCR5380_dma_recv_setup/NCR5380_dma_send_setup.
>
> > Adding the wait allows to continue in PIO mode with "tag#0 switching to
> > slow handshake".
>
> I don't think this is the code path you want. The target isn't really
> broken. But yes, we could use PIO as a slow workaround for fragile PDMA
> logic.

Yes, we don't want that.

> > > > I get gated 53C80 IRQ, BASR=0x10, MODE=0x0e, STATUS=0x7c.
> > >
> > > You can use NCR5380_print() to get a decoded register dump.
> > >
> > > When I decode the above values I get,
> > >
> > > BASR   = 0x10 = BASR_IRQ
> > > MODE   = 0x0e = MR_ENABLE_EOP_INTR | MR_MONITOR_BSY | MR_DMA_MODE
> > > STATUS = 0x7c = SR_BSY | SR_REQ | SR_MSG | SR_CD | SR_IO
> > >
> > > Since BASR_PHASE_MATCH is not set, the interrupt is almost certainly a
> > > phase mismatch. The new phase is SR_MSG | SR_CD | SR_IO = PHASE_MSGIN,
> > > which shows that the target has given up on the DATA IN phase and is
> > > presumably trying to send a DISCONNECT message.
> >
> > Looks you're right. The transfersize is 4096, i.e. 2 CD-ROM sectors.
> > When the 2nd sector is not ready in the drive internal buffer, the drive
> > probably disconnects which breaks the fragile pdma transfer. When the
> > transfersize is limited to 2048 bytes, the problem goes away.
>
> OK.
>
> > The problem also went away with enabled interrupts because I had some
> > debug printks added which were slowing down the transfer enough for the
> > drive (SONY CDU-55S) to keep up and not disconnect. Got the same result
> > by inserting udelay(100) after each 128 byte block trasnferred in
> > pread().
>
> Well, yeah, if you change the timing and omit to mention that, as well as
> changing the spinlock_irq_save/restore pairs, then it's going to be
> difficult for me to make sense of your results. Anyway, I'm glad that you
> got to the bottom of this mystery.
>
> > > > When I enable interrupts during PDMA (like the removed UNSAFE macro
> > > > did), the problems go away. I see an IRQ after each pread call.

These two patches are enough to make PDMA work with CD-ROM drives on
53C400(A), with IRQ enabled or not. They even improve transfer rates with
HDDs because of bigger block size.

But DTC436 breaks with CDU-55S, immediately after modprobe.
1) Seems that IRQ timing is slightly different so I rewrote the wait for
the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some
data to be transferred but
2) DTC436 likes to hang (return only 0xff from all registers - like it's
not even present on the bus) on repeating CSR register reads and maybe some
other actions too. This problem already appeared before in pwrite() where
I added the udelay(4) workaround. Now I added udelay(100) to the waits but
it still crashes sometimes (randomly after tenths or hundreds of MB).

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6f9665d..9d0cfd4 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 	return 0;
 }
 
+#define G_NCR5380_DMA_MAX_SIZE	32768
 static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                         struct scsi_cmnd *cmd)
 {
-	int transfersize = cmd->transfersize;
+	int transfersize = cmd->SCp.this_residual;
 
 	if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
 		return 0;
 
-	/* Limit transfers to 32K, for xx400 & xx406
-	 * pseudoDMA that transfers in 128 bytes blocks.
-	 */
-	if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
-	    !(cmd->SCp.this_residual % transfersize))
-		transfersize = 32 * 1024;
-
 	/* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
 	if (transfersize % 128)
 		transfersize = 0;
 
-	return transfersize;
+	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
 }
 
 /*

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 9d0cfd4..797115e 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -453,15 +453,15 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 	int blocks = len / 128;
 	int start = 0;
 
+	hostdata->pdma_residual = 0;
+
 	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
 	NCR5380_write(hostdata->c400_blk_cnt, blocks);
 	while (1) {
 		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
 			break;
-		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
-			printk(KERN_ERR "53C400r: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
-			return -1;
-		}
+		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
+			goto out_wait;
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; /* FIXME - no timeout */
 
@@ -499,13 +499,13 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
 
 	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
 		printk("53C400r: no 53C80 gated irq after transfer");
-
+out_wait:
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
 		;
 
 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		hostdata->pdma_residual = blocks * 128;
 		
 	return 0;
 }
@@ -526,13 +526,13 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 	int blocks = len / 128;
 	int start = 0;
 
+	hostdata->pdma_residual = 0;
+
 	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
 	NCR5380_write(hostdata->c400_blk_cnt, blocks);
 	while (1) {
-		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
-			printk(KERN_ERR "53C400w: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
-			return -1;
-		}
+		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
+			goto out_wait;
 
 		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
 			break;
@@ -569,16 +569,15 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
 		start += 128;
 		blocks--;
 	}
-
+out_wait:
 	/* wait for 53C80 registers to be available */
 	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)) {
 		udelay(4); /* DTC436 chip hangs without this */
 		/* FIXME - no timeout */
 	}
 
-	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
-		printk(KERN_ERR "53C400w: no end dma signal\n");
-	}
+	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
+		hostdata->pdma_residual = blocks * 128;
 
 	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
 		; 	// TIMEOUT
@@ -601,6 +600,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
 	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
 }
 
+static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
+{
+	return hostdata->pdma_residual;
+}
+
 /*
  *	Include the NCR5380 core code that we build our driver around	
  */
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 81b22d9..08c91b7 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -26,7 +26,8 @@
 	int c400_ctl_status; \
 	int c400_blk_cnt; \
 	int c400_host_buf; \
-	int io_width;
+	int io_width; \
+	int pdma_residual;
 
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
@@ -35,7 +36,7 @@
 #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
 #define NCR5380_dma_recv_setup		generic_NCR5380_pread
 #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
-#define NCR5380_dma_residual		NCR5380_dma_residual_none
+#define NCR5380_dma_residual		generic_NCR5380_dma_residual
 
 #define NCR5380_intr generic_NCR5380_intr
 #define NCR5380_queue_command generic_NCR5380_queue_command


-- 
Ondrej Zary

Re: g_NCR5380 PDMA, was Re: [PATCH 0/6] ncr5380: Miscellaneous minor patches

[Finn Thain]

On Sun, 19 Feb 2017, Ondrej Zary wrote:

> 
> These two patches are enough to make PDMA work with CD-ROM drives on 
> 53C400(A), with IRQ enabled or not. They even improve transfer rates 
> with HDDs because of bigger block size.
> 

Nice! This is a great improvement.

> But DTC436 breaks with CDU-55S, immediately after modprobe.
> 1) Seems that IRQ timing is slightly different so I rewrote the wait for
> the host buffer to include check for CSR_GATED_53C80_IRQ. This allows some
> data to be transferred but
> 2) DTC436 likes to hang (return only 0xff from all registers - like it's
> not even present on the bus) on repeating CSR register reads and maybe some
> other actions too. This problem already appeared before in pwrite() where
> I added the udelay(4) workaround. Now I added udelay(100) to the waits but
> it still crashes sometimes (randomly after tenths or hundreds of MB).
> 

That is not a regression, right? If there are no regressions, I'd like to 
merge this patch (with some minor tweaks, rebased on the latest driver).

The problem with these Domex chips is that the manufacturer doesn't 
respond to repeated requests for information.

If you want to pursue this bug, when working on undocumented proprietary 
hardware from manufacturers like this one, what I recommend is to look for 
clues in the code for alternative implementations (NetBSD etc) and try 
wiggling parameters until the behaviour changes (e.g. reduce the transfer 
size to 128 bytes, introduce delays at key places. Probably the same sort 
of thing you already tried.) Snooping the device register accessess made 
by a DOS/Windows driver is another approach.

> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6f9665d..9d0cfd4 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -585,26 +585,20 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  	return 0;
>  }
>  
> +#define G_NCR5380_DMA_MAX_SIZE	32768

Please put this at the top with the other definitions. (As you will 
recall, the macro definitions were moved around in the latest driver.)

>  static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
>                                          struct scsi_cmnd *cmd)
>  {
> -	int transfersize = cmd->transfersize;
> +	int transfersize = cmd->SCp.this_residual;
>  
>  	if (hostdata->flags & FLAG_NO_PSEUDO_DMA)
>  		return 0;
>  
> -	/* Limit transfers to 32K, for xx400 & xx406
> -	 * pseudoDMA that transfers in 128 bytes blocks.
> -	 */
> -	if (transfersize > 32 * 1024 && cmd->SCp.this_residual &&
> -	    !(cmd->SCp.this_residual % transfersize))
> -		transfersize = 32 * 1024;
> -
>  	/* 53C400 datasheet: non-modulo-128-byte transfers should use PIO */
>  	if (transfersize % 128)
>  		transfersize = 0;
>  
> -	return transfersize;
> +	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
>  }
>  
>  /*
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 9d0cfd4..797115e 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -453,15 +453,15 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
>  	int blocks = len / 128;
>  	int start = 0;
>  
> +	hostdata->pdma_residual = 0;
> +
>  	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE | CSR_TRANS_DIR);
>  	NCR5380_write(hostdata->c400_blk_cnt, blocks);
>  	while (1) {
>  		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
>  			break;
> -		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
> -			printk(KERN_ERR "53C400r: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
> -			return -1;
> -		}
> +		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
> +			goto out_wait;

This algorithm basically says that, if the IRQ is late (or missing) we 
will go through the main loop, information transfer and DMA setup steps 
again. That's fine, but it means that we have to exit the pread/pwrite 
routines with the chip in a suitable state for back-to-back PDMA 
transfers.

NCR5380_dma_complete() takes care of things for the 53C80 core, but maybe 
the DTC436 logic might need a bit more work (?) For example, if we exit 
with a partially filled buffer, will the buffer switching logic break? The 
53C400 datasheet mentions a "Resume Transfer Register" which makes me a 
bit suspicious about chip state after a DISCONNECT... just a thought.

>  		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
>  			; /* FIXME - no timeout */
>  
> @@ -499,13 +499,13 @@ static inline int generic_NCR5380_pread(struct NCR5380_hostdata *hostdata,
>  
>  	if (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ))
>  		printk("53C400r: no 53C80 gated irq after transfer");
> -
> +out_wait:
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG))
>  		;
>  
>  	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> -		printk(KERN_ERR "53C400r: no end dma signal\n");
> +		hostdata->pdma_residual = blocks * 128;


Can we unconditionally assign pdma_residual? For example,

 	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
-		printk(KERN_ERR "53C400r: no end dma signal\n");
+		pr_err("%s: No end dma signal (%d/%d)\n", __func__, start, len);

+	hostdata->pdma_residual = len - start;
 
 	return 0;
 }

Since the new 'goto' can cause the while loop to terminate early, it is 
easier to reason about driver behaviour if this assignment is not 
conditional on the End of DMA signal.

> @@ -526,13 +526,13 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  	int blocks = len / 128;
>  	int start = 0;
>  
> +	hostdata->pdma_residual = 0;
> +
>  	NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
>  	NCR5380_write(hostdata->c400_blk_cnt, blocks);
>  	while (1) {
> -		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ) {
> -			printk(KERN_ERR "53C400w: Got 53C80_IRQ start=%d, blocks=%d\n", start, blocks);
> -			return -1;
> -		}
> +		if (NCR5380_read(hostdata->c400_ctl_status) & CSR_GATED_53C80_IRQ)
> +			goto out_wait;
>  
>  		if (NCR5380_read(hostdata->c400_blk_cnt) == 0)
>  			break;
> @@ -569,16 +569,15 @@ static inline int generic_NCR5380_pwrite(struct NCR5380_hostdata *hostdata,
>  		start += 128;
>  		blocks--;
>  	}
> -
> +out_wait:
>  	/* wait for 53C80 registers to be available */
>  	while (!(NCR5380_read(hostdata->c400_ctl_status) & CSR_53C80_REG)) {
>  		udelay(4); /* DTC436 chip hangs without this */
>  		/* FIXME - no timeout */
>  	}
>  
> -	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER)) {
> -		printk(KERN_ERR "53C400w: no end dma signal\n");
> -	}
> +	if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_END_DMA_TRANSFER))
> +		hostdata->pdma_residual = blocks * 128;
>  

Same here.


>  	while (!(NCR5380_read(TARGET_COMMAND_REG) & TCR_LAST_BYTE_SENT))
>  		; 	// TIMEOUT
> @@ -601,6 +600,11 @@ static int generic_NCR5380_dma_xfer_len(struct NCR5380_hostdata *hostdata,
>  	return min(transfersize, G_NCR5380_DMA_MAX_SIZE);
>  }
>  
> +static int generic_NCR5380_dma_residual(struct NCR5380_hostdata *hostdata)
> +{
> +	return hostdata->pdma_residual;
> +}
> +
>  /*
>   *	Include the NCR5380 core code that we build our driver around	
>   */
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 81b22d9..08c91b7 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -26,7 +26,8 @@
>  	int c400_ctl_status; \
>  	int c400_blk_cnt; \
>  	int c400_host_buf; \
> -	int io_width;
> +	int io_width; \
> +	int pdma_residual;
>  
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
> @@ -35,7 +36,7 @@
>  #define NCR5380_dma_xfer_len		generic_NCR5380_dma_xfer_len
>  #define NCR5380_dma_recv_setup		generic_NCR5380_pread
>  #define NCR5380_dma_send_setup		generic_NCR5380_pwrite
> -#define NCR5380_dma_residual		NCR5380_dma_residual_none
> +#define NCR5380_dma_residual		generic_NCR5380_dma_residual
>  
>  #define NCR5380_intr generic_NCR5380_intr
>  #define NCR5380_queue_command generic_NCR5380_queue_command
> 
> 
> 

Thanks.

--