[PATCH 0/2] Some esp_scsi updates

[PATCH 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 22 +++++++++++++--------
 drivers/scsi/esp_scsi.h | 44 ++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
enum explaining this.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..b96cbda03d2d 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,18 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

[PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
or Emulex parts. This caused it to be detected as a FAS100A.

Unforunately, this meant the configuration of the CONFIG3 register was
incorrect. This causes data transfer issues with FAST-SCSI targets.

The FSC also has the CONFIG4 register. It can be used to enable a feature
called Active Negation which should always be enabled according to the data
manual.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c | 20 +++++++++++++-------
 drivers/scsi/esp_scsi.h | 25 +++++++++++++++++--------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 4fc3eee3138b..cef1b0cb5ee6 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -243,7 +243,7 @@ static void esp_set_all_config3(struct esp *esp, u8 val)
 /* Reset the ESP chip, _not_ the SCSI bus. */
 static void esp_reset_esp(struct esp *esp)
 {
-	u8 family_code, version;
+	u8 family_code, uid;
 
 	/* Now reset the ESP chip */
 	scsi_esp_cmd(esp, ESP_CMD_RC);
@@ -257,13 +257,17 @@ static void esp_reset_esp(struct esp *esp)
 	 */
 	esp->max_period = ((35 * esp->ccycle) / 1000);
 	if (esp->rev == FAST) {
-		version = esp_read8(ESP_UID);
-		family_code = (version & 0xf8) >> 3;
-		if (family_code == 0x02)
+		uid = esp_read8(ESP_UID);
+		family_code = ESP_FAMILY(uid);
+		if (family_code == ESP_UID_F236)
 			esp->rev = FAS236;
-		else if (family_code == 0x0a)
+		else if (family_code == ESP_UID_HME)
 			esp->rev = FASHME; /* Version is usually '5'. */
-		else
+		else if (family_code == ESP_UID_FSC) {
+			esp->rev = FSC;
+			/* Enable Active Negation */
+			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
+		} else
 			esp->rev = FAS100A;
 		esp->min_period = ((4 * esp->ccycle) / 1000);
 	} else {
@@ -308,7 +312,8 @@ static void esp_reset_esp(struct esp *esp)
 
 	case FAS236:
 	case PCSCSI:
-		/* Fast 236, AM53c974 or HME */
+	case FSC:
+		/* Fast 236, AM53c974, FSC or HME */
 		esp_write8(esp->config2, ESP_CFG2);
 		if (esp->rev == FASHME) {
 			u8 cfg3 = esp->target[0].esp_config3;
@@ -2374,6 +2379,7 @@ static const char *esp_chip_names[] = {
 	"ESP236",
 	"FAS236",
 	"AM53C974",
+	"FSC",
 	"FAS100A",
 	"FAST",
 	"FASHME",
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index b96cbda03d2d..95f2b27e8d6c 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -78,12 +78,14 @@
 #define ESP_CONFIG3_IMS       0x80     /* ID msg chk'ng        (esp/fas236)  */
 #define ESP_CONFIG3_OBPUSH    0x80     /* Push odd-byte to dma (hme)         */
 
-/* ESP config register 4 read-write, found only on am53c974 chips */
-#define ESP_CONFIG4_RADE      0x04     /* Active negation */
-#define ESP_CONFIG4_RAE       0x08     /* Active negation on REQ and ACK */
-#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature */
-#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0 */
-#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1 */
+/* ESP config register 4 read-write, found on am53c974 and FSC chips */
+#define ESP_CONFIG4_BBTE      0x01     /* Back-to-back transfers     (fsc)   */
+#define ESP_CONGIG4_TEST      0x02     /* Transfer counter test mode (fsc)   */
+#define ESP_CONFIG4_RADE      0x04     /* Active negation   (am53c974/fsc)   */
+#define ESP_CONFIG4_RAE       0x08     /* Act. negation REQ/ACK (am53c974)   */
+#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature (am53c974)   */
+#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0    (am53c974)   */
+#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1    (am53c974)   */
 
 #define ESP_CONFIG_GE_12NS    (0)
 #define ESP_CONFIG_GE_25NS    (ESP_CONFIG_GE1)
@@ -209,10 +211,16 @@
 #define ESP_TEST_TS           0x04     /* Tristate test mode */
 
 /* ESP unique ID register read-only, found on fas236+fas100a only */
+#define ESP_UID_REV           0x07     /* ESP revision bitmask */
+#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
+
+#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
+
+/* Values for the ESP family */
 #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
 #define ESP_UID_F236          0x02     /* ESP FAS236   */
-#define ESP_UID_REV           0x07     /* ESP revision */
-#define ESP_UID_FAM           0xf8     /* ESP family   */
+#define ESP_UID_HME           0x0a     /* FAS HME      */
+#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
 
 /* ESP fifo flags register read-only */
 /* Note that the following implies a 16 byte FIFO on the ESP. */
@@ -265,6 +273,7 @@ enum esp_rev {
 	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
 	FAS236,
 	PCSCSI,  /* AM53c974 */
+	FSC,     /* NCR/Symbios Logic FSC */
 	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
 	FAS100A,
 	FAST,
-- 
2.17.1

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

On Tue, 12 Nov 2019, Kars de Jong wrote:

> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.
> 
> Add a comment to the enum explaining this.
> 
> Also, the actual values for the enum fields are irrelevant, so remove the
> explicit values (suggested by Geert Uytterhoeven). This makes adding a new
> field in the middle of the enum easier.
> 
> Finally, move the PCSCSI definition to the right place in the enum. In its
> previous location, at the end of the enum, the wrong values are written to
> the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
> enum explaining this.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>  drivers/scsi/esp_scsi.c |  2 +-
>  drivers/scsi/esp_scsi.h | 19 +++++++++++--------
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..4fc3eee3138b 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
>  	"ESP100A",
>  	"ESP236",
>  	"FAS236",
> +	"AM53C974",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> -	"AM53C974",
>  };
>  
>  static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 91b32f2a1a1b..b96cbda03d2d 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -257,15 +257,18 @@ struct esp_cmd_priv {
>  };
>  #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
>  
> +/* NOTE: this enum is ordered based on chip features! */

Fair enough, that has been overlooked before.

>  enum esp_rev {
> -	ESP100     = 0x00,  /* NCR53C90 - very broken */
> -	ESP100A    = 0x01,  /* NCR53C90A */
> -	ESP236     = 0x02,
> -	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	ESP100,  /* NCR53C90 - very broken */
> +	ESP100A, /* NCR53C90A */
> +	ESP236,
> +	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
> +	FAS236,
> +	PCSCSI,  /* AM53c974 */
> +	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
> +	FAS100A,
> +	FAST,
> +	FASHME,
>  };
>  
>  struct esp_cmd_entry {
> 

FAS100A, FAST and FASHME are below both lines, which is a bit confusing.

In general, I don't like to see comments that merely re-state the explicit 
logic in the algorithm. Such comments add no value.

(At best this redundancy creates a maintenance burden and at worst the 
commentary becomes neglected until it creates contradictions.)

Aside from that:
Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

-- 

Re: [PATCH 2/2] esp_scsi: Add support for FSC chip

[Finn Thain]

On Tue, 12 Nov 2019, Kars de Jong wrote:

> The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
> or Emulex parts. This caused it to be detected as a FAS100A.
> 
> Unforunately, this meant the configuration of the CONFIG3 register was
> incorrect. This causes data transfer issues with FAST-SCSI targets.
> 
> The FSC also has the CONFIG4 register. It can be used to enable a feature
> called Active Negation which should always be enabled according to the data
> manual.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>  drivers/scsi/esp_scsi.c | 20 +++++++++++++-------
>  drivers/scsi/esp_scsi.h | 25 +++++++++++++++++--------
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 4fc3eee3138b..cef1b0cb5ee6 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -243,7 +243,7 @@ static void esp_set_all_config3(struct esp *esp, u8 val)
>  /* Reset the ESP chip, _not_ the SCSI bus. */
>  static void esp_reset_esp(struct esp *esp)
>  {
> -	u8 family_code, version;
> +	u8 family_code, uid;
>  
>  	/* Now reset the ESP chip */
>  	scsi_esp_cmd(esp, ESP_CMD_RC);
> @@ -257,13 +257,17 @@ static void esp_reset_esp(struct esp *esp)
>  	 */
>  	esp->max_period = ((35 * esp->ccycle) / 1000);
>  	if (esp->rev == FAST) {
> -		version = esp_read8(ESP_UID);
> -		family_code = (version & 0xf8) >> 3;
> -		if (family_code == 0x02)
> +		uid = esp_read8(ESP_UID);
> +		family_code = ESP_FAMILY(uid);

The uid temporary isn't needed.

> +		if (family_code == ESP_UID_F236)
>  			esp->rev = FAS236;
> -		else if (family_code == 0x0a)
> +		else if (family_code == ESP_UID_HME)
>  			esp->rev = FASHME; /* Version is usually '5'. */
> -		else
> +		else if (family_code == ESP_UID_FSC) {
> +			esp->rev = FSC;
> +			/* Enable Active Negation */
> +			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
> +		} else
>  			esp->rev = FAS100A;
>  		esp->min_period = ((4 * esp->ccycle) / 1000);
>  	} else {
> @@ -308,7 +312,8 @@ static void esp_reset_esp(struct esp *esp)
>  
>  	case FAS236:
>  	case PCSCSI:
> -		/* Fast 236, AM53c974 or HME */
> +	case FSC:
> +		/* Fast 236, AM53c974, FSC or HME */
>  		esp_write8(esp->config2, ESP_CFG2);
>  		if (esp->rev == FASHME) {
>  			u8 cfg3 = esp->target[0].esp_config3;
> @@ -2374,6 +2379,7 @@ static const char *esp_chip_names[] = {
>  	"ESP236",
>  	"FAS236",
>  	"AM53C974",
> +	"FSC",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index b96cbda03d2d..95f2b27e8d6c 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -78,12 +78,14 @@
>  #define ESP_CONFIG3_IMS       0x80     /* ID msg chk'ng        (esp/fas236)  */
>  #define ESP_CONFIG3_OBPUSH    0x80     /* Push odd-byte to dma (hme)         */
>  
> -/* ESP config register 4 read-write, found only on am53c974 chips */
> -#define ESP_CONFIG4_RADE      0x04     /* Active negation */
> -#define ESP_CONFIG4_RAE       0x08     /* Active negation on REQ and ACK */
> -#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature */
> -#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0 */
> -#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1 */
> +/* ESP config register 4 read-write, found on am53c974 and FSC chips */
> +#define ESP_CONFIG4_BBTE      0x01     /* Back-to-back transfers     (fsc)   */
> +#define ESP_CONGIG4_TEST      0x02     /* Transfer counter test mode (fsc)   */
> +#define ESP_CONFIG4_RADE      0x04     /* Active negation   (am53c974/fsc)   */
> +#define ESP_CONFIG4_RAE       0x08     /* Act. negation REQ/ACK (am53c974)   */
> +#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature (am53c974)   */
> +#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0    (am53c974)   */
> +#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1    (am53c974)   */
>  
>  #define ESP_CONFIG_GE_12NS    (0)
>  #define ESP_CONFIG_GE_25NS    (ESP_CONFIG_GE1)
> @@ -209,10 +211,16 @@
>  #define ESP_TEST_TS           0x04     /* Tristate test mode */
>  
>  /* ESP unique ID register read-only, found on fas236+fas100a only */
> +#define ESP_UID_REV           0x07     /* ESP revision bitmask */

This is unused.

> +#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
> +
> +#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
> +

The ESP_UID_FAM symbol only appears here. I don't think it adds value.

> +/* Values for the ESP family */

I would omit that comment.

>  #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
>  #define ESP_UID_F236          0x02     /* ESP FAS236   */
> -#define ESP_UID_REV           0x07     /* ESP revision */
> -#define ESP_UID_FAM           0xf8     /* ESP family   */
> +#define ESP_UID_HME           0x0a     /* FAS HME      */
> +#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
>  

Is there a distinction between the chip's uid and the chip's family?

-- 

>  /* ESP fifo flags register read-only */
>  /* Note that the following implies a 16 byte FIFO on the ESP. */
> @@ -265,6 +273,7 @@ enum esp_rev {
>  	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
>  	FAS236,
>  	PCSCSI,  /* AM53c974 */
> +	FSC,     /* NCR/Symbios Logic FSC */
>  	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
>  	FAS100A,
>  	FAST,
> 

Re: [PATCH 2/2] esp_scsi: Add support for FSC chip

[Finn Thain]

On Wed, 13 Nov 2019, Finn Thain wrote:

> 
> > +/* Values for the ESP family */
> 
> I would omit that comment.
> 

I see now that you meant "ESP family" in a narrow technical sense. I 
completely missed that. Maybe "Values for the ESP family bits" would make 
that clear.

-- 

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Finn,

Thanks for your review!

Op wo 13 nov. 2019 om 00:07 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Tue, 12 Nov 2019, Kars de Jong wrote:
> > Finally, move the PCSCSI definition to the right place in the enum. In its
> > previous location, at the end of the enum, the wrong values are written to
> > the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
> > enum explaining this.
> >
> >  enum esp_rev {
> > -     ESP100     = 0x00,  /* NCR53C90 - very broken */
> > -     ESP100A    = 0x01,  /* NCR53C90A */
> > -     ESP236     = 0x02,
> > -     FAS236     = 0x03,
> > -     FAS100A    = 0x04,
> > -     FAST       = 0x05,
> > -     FASHME     = 0x06,
> > -     PCSCSI     = 0x07,  /* AM53c974 */
> > +     ESP100,  /* NCR53C90 - very broken */
> > +     ESP100A, /* NCR53C90A */
> > +     ESP236,
> > +     /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
> > +     FAS236,
> > +     PCSCSI,  /* AM53c974 */
> > +     /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
> > +     FAS100A,
> > +     FAST,
> > +     FASHME,
> >  };
> >
> >  struct esp_cmd_entry {
> >
>
> FAS100A, FAST and FASHME are below both lines, which is a bit confusing.

Hmm, you're right. But I don't really know how to solve that. But if
you think the initial comment is enough to trigger people to
investigate the algorithm, I can remove them.

> In general, I don't like to see comments that merely re-state the explicit
> logic in the algorithm. Such comments add no value.
>
> (At best this redundancy creates a maintenance burden and at worst the
> commentary becomes neglected until it creates contradictions.)

Unfortunately the algorithm isn't very obvious here (well, not to me at least).

Kind regards,

Kars.

Re: [PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

Hi Finn,

Thanks for your review!

Op wo 13 nov. 2019 om 00:18 schreef Finn Thain <fthain@telegraphics.com.au>:
>
> On Tue, 12 Nov 2019, Kars de Jong wrote:
> > +#define ESP_UID_REV           0x07     /* ESP revision bitmask */
>
> This is unused.

Yes, but it was already there, I just moved it. I prefer to leave it
in, since it describes the register layout.

> > +#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
> > +
> > +#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
> > +
>
> The ESP_UID_FAM symbol only appears here. I don't think it adds value.

OK, I can just change the macro to:

#define ESP_FAMILY(uid) (((uid) & 0xf8) >> 3)

> > +/* Values for the ESP family */
>
> I would omit that comment.

I will change it to "Values for the ESP family bits" as you suggested
in the next mail.

> >  #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
> >  #define ESP_UID_F236          0x02     /* ESP FAS236   */
> > -#define ESP_UID_REV           0x07     /* ESP revision */
> > -#define ESP_UID_FAM           0xf8     /* ESP family   */
> > +#define ESP_UID_HME           0x0a     /* FAS HME      */
> > +#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
> >
>
> Is there a distinction between the chip's uid and the chip's family?

Yes, the complete UID also includes the revision. The old driver had
cases where the family code was the same but the revision was
different.

Kind regards,

Kars.

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Christoph Hellwig]

On Tue, Nov 12, 2019 at 07:57:09PM +0100, Kars de Jong wrote:
> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.

Yikes.  Wouldn't it make much more sense to have a feature bitmap?

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Christoph!

Op wo 13 nov. 2019 om 15:22 schreef Christoph Hellwig <hch@infradead.org>:
>
> On Tue, Nov 12, 2019 at 07:57:09PM +0100, Kars de Jong wrote:
> > The order of the definitions in the esp_rev enum is important. The values
> > are used in comparisons for chip features.
>
> Yikes.  Wouldn't it make much more sense to have a feature bitmap?

Yes, I mentioned that in my cover letter already, and it was discussed
a bit on the Linux/m68k mailing list.
 It may be hard to get that tested though, most of the users are
legacy architectures with a probably very limited number of users.
But if a mostly review-based process is good enough, I am certainly
willing to change it.

Kind regards,

Kars.

Re: [PATCH 2/2] esp_scsi: Add support for FSC chip

[Finn Thain]

On Wed, 13 Nov 2019, Kars de Jong wrote:

> Op wo 13 nov. 2019 om 00:18 schreef Finn Thain <fthain@telegraphics.com.au>:
> >
> > On Tue, 12 Nov 2019, Kars de Jong wrote:
> > > +#define ESP_UID_REV           0x07     /* ESP revision bitmask */
> >
> > This is unused.
> 
> Yes, but it was already there, I just moved it.
> 

Sure, but if you move dead code, it creates churn which can lead to merge 
conflicts. And such changes still require code review.

Also, you'd lose an opportunity to delete the dead code, which is a pity, 
since that would then require a separate patch.

> I prefer to leave it in, since it describes the register layout.
> 

Well, the driver can't be understood from the code alone. The datasheet 
will always be required reading.

> > > +#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
> > > +
> > > +#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
> > > +
> >
> > The ESP_UID_FAM symbol only appears here. I don't think it adds value.
> 
> OK, I can just change the macro to:
> 
> #define ESP_FAMILY(uid) (((uid) & 0xf8) >> 3)
> 

Now that I understand the relationship between UID and Family, I see why 
you did this.

> > > +/* Values for the ESP family */
> >
> > I would omit that comment.
> 
> I will change it to "Values for the ESP family bits" as you suggested
> in the next mail.
> 

Great.

> > >  #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
> > >  #define ESP_UID_F236          0x02     /* ESP FAS236   */
> > > -#define ESP_UID_REV           0x07     /* ESP revision */
> > > -#define ESP_UID_FAM           0xf8     /* ESP family   */
> > > +#define ESP_UID_HME           0x0a     /* FAS HME      */
> > > +#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
> > >
> >
> > Is there a distinction between the chip's uid and the chip's family?
> 
> Yes, the complete UID also includes the revision. The old driver had 
> cases where the family code was the same but the revision was different.
> 

Makes sense. Thanks for the explanation.

-- 

> Kind regards,
> 
> Kars.
> 

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

On Wed, 13 Nov 2019, Kars de Jong wrote:

> >
> > FAS100A, FAST and FASHME are below both lines, which is a bit 
> > confusing.
> 
> Hmm, you're right. But I don't really know how to solve that. But if you 
> think the initial comment is enough to trigger people to investigate the 
> algorithm, I can remove them.
> 

Yes, please. The initial "NOTE!" that you added is sufficient, IMHO.

-- 

[PATCH v2 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Changes since v1:
- Removed confusing comments from esp_rev enum
- Remove unneeded definitions for UID register
- Remove unneeded local uid variable

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 21 +++++++++++--------
 drivers/scsi/esp_scsi.h | 45 ++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..f764d64e1f25 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,16 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

[PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
or Emulex parts. This caused it to be detected as a FAS100A.

Unforunately, this meant the configuration of the CONFIG3 register was
incorrect. This causes data transfer issues with FAST-SCSI targets.

The FSC also has the CONFIG4 register. It can be used to enable a feature
called Active Negation which should always be enabled according to the data
manual.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c | 19 ++++++++++++-------
 drivers/scsi/esp_scsi.h | 28 ++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 4fc3eee3138b..e887ea3e514a 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -243,7 +243,7 @@ static void esp_set_all_config3(struct esp *esp, u8 val)
 /* Reset the ESP chip, _not_ the SCSI bus. */
 static void esp_reset_esp(struct esp *esp)
 {
-	u8 family_code, version;
+	u8 family_code;
 
 	/* Now reset the ESP chip */
 	scsi_esp_cmd(esp, ESP_CMD_RC);
@@ -257,13 +257,16 @@ static void esp_reset_esp(struct esp *esp)
 	 */
 	esp->max_period = ((35 * esp->ccycle) / 1000);
 	if (esp->rev == FAST) {
-		version = esp_read8(ESP_UID);
-		family_code = (version & 0xf8) >> 3;
-		if (family_code == 0x02)
+		family_code = ESP_FAMILY(esp_read8(ESP_UID));
+		if (family_code == ESP_UID_F236)
 			esp->rev = FAS236;
-		else if (family_code == 0x0a)
+		else if (family_code == ESP_UID_HME)
 			esp->rev = FASHME; /* Version is usually '5'. */
-		else
+		else if (family_code == ESP_UID_FSC) {
+			esp->rev = FSC;
+			/* Enable Active Negation */
+			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
+		} else
 			esp->rev = FAS100A;
 		esp->min_period = ((4 * esp->ccycle) / 1000);
 	} else {
@@ -308,7 +311,8 @@ static void esp_reset_esp(struct esp *esp)
 
 	case FAS236:
 	case PCSCSI:
-		/* Fast 236, AM53c974 or HME */
+	case FSC:
+		/* Fast 236, AM53c974, FSC or HME */
 		esp_write8(esp->config2, ESP_CFG2);
 		if (esp->rev == FASHME) {
 			u8 cfg3 = esp->target[0].esp_config3;
@@ -2374,6 +2378,7 @@ static const char *esp_chip_names[] = {
 	"ESP236",
 	"FAS236",
 	"AM53C974",
+	"FSC",
 	"FAS100A",
 	"FAST",
 	"FASHME",
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index f764d64e1f25..60de32d17d6a 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -78,12 +78,14 @@
 #define ESP_CONFIG3_IMS       0x80     /* ID msg chk'ng        (esp/fas236)  */
 #define ESP_CONFIG3_OBPUSH    0x80     /* Push odd-byte to dma (hme)         */
 
-/* ESP config register 4 read-write, found only on am53c974 chips */
-#define ESP_CONFIG4_RADE      0x04     /* Active negation */
-#define ESP_CONFIG4_RAE       0x08     /* Active negation on REQ and ACK */
-#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature */
-#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0 */
-#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1 */
+/* ESP config register 4 read-write, found on am53c974 and FSC chips */
+#define ESP_CONFIG4_BBTE      0x01     /* Back-to-back transfers     (fsc)   */
+#define ESP_CONGIG4_TEST      0x02     /* Transfer counter test mode (fsc)   */
+#define ESP_CONFIG4_RADE      0x04     /* Active negation   (am53c974/fsc)   */
+#define ESP_CONFIG4_RAE       0x08     /* Act. negation REQ/ACK (am53c974)   */
+#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature (am53c974)   */
+#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0    (am53c974)   */
+#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1    (am53c974)   */
 
 #define ESP_CONFIG_GE_12NS    (0)
 #define ESP_CONFIG_GE_25NS    (ESP_CONFIG_GE1)
@@ -209,10 +211,15 @@
 #define ESP_TEST_TS           0x04     /* Tristate test mode */
 
 /* ESP unique ID register read-only, found on fas236+fas100a only */
+#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
+
+#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
+
+/* Values for the ESP family bits */
 #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
 #define ESP_UID_F236          0x02     /* ESP FAS236   */
-#define ESP_UID_REV           0x07     /* ESP revision */
-#define ESP_UID_FAM           0xf8     /* ESP family   */
+#define ESP_UID_HME           0x0a     /* FAS HME      */
+#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
 
 /* ESP fifo flags register read-only */
 /* Note that the following implies a 16 byte FIFO on the ESP. */
@@ -264,6 +271,11 @@ enum esp_rev {
 	ESP236,
 	FAS236,
 	PCSCSI,  /* AM53c974 */
+<<<<<<< HEAD
+=======
+	FSC,     /* NCR/Symbios Logic FSC */
+	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
+>>>>>>> fb55f6c862d7... esp_scsi: Add support for FSC chip
 	FAS100A,
 	FAST,
 	FASHME,
-- 
2.17.1

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Oops, sorry, forgot to add v2 to the subject.

Op do 14 nov. 2019 om 23:00 schreef Kars de Jong <jongk@linux-m68k.org>:
...

Re: [PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

Bah, ignore this... it still has a merge conflict.

Sorry.

Op do 14 nov. 2019 om 23:00 schreef Kars de Jong <jongk@linux-m68k.org>:

[PATCH v3 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Changes since v1:
- Removed confusing comments from esp_rev enum
- Remove unneeded definitions for UID register
- Remove unneeded local uid variable

Changes since v2:
- Fixed merge conflict in second patch

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 21 +++++++++++++--------
 drivers/scsi/esp_scsi.h | 41 +++++++++++++++++++++++++----------------
 2 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..f764d64e1f25 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,16 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

[PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
or Emulex parts. This caused it to be detected as a FAS100A.

Unforunately, this meant the configuration of the CONFIG3 register was
incorrect. This causes data transfer issues with FAST-SCSI targets.

The FSC also has the CONFIG4 register. It can be used to enable a feature
called Active Negation which should always be enabled according to the data
manual.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c | 19 ++++++++++++-------
 drivers/scsi/esp_scsi.h | 24 ++++++++++++++++--------
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 4fc3eee3138b..e887ea3e514a 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -243,7 +243,7 @@ static void esp_set_all_config3(struct esp *esp, u8 val)
 /* Reset the ESP chip, _not_ the SCSI bus. */
 static void esp_reset_esp(struct esp *esp)
 {
-	u8 family_code, version;
+	u8 family_code;
 
 	/* Now reset the ESP chip */
 	scsi_esp_cmd(esp, ESP_CMD_RC);
@@ -257,13 +257,16 @@ static void esp_reset_esp(struct esp *esp)
 	 */
 	esp->max_period = ((35 * esp->ccycle) / 1000);
 	if (esp->rev == FAST) {
-		version = esp_read8(ESP_UID);
-		family_code = (version & 0xf8) >> 3;
-		if (family_code == 0x02)
+		family_code = ESP_FAMILY(esp_read8(ESP_UID));
+		if (family_code == ESP_UID_F236)
 			esp->rev = FAS236;
-		else if (family_code == 0x0a)
+		else if (family_code == ESP_UID_HME)
 			esp->rev = FASHME; /* Version is usually '5'. */
-		else
+		else if (family_code == ESP_UID_FSC) {
+			esp->rev = FSC;
+			/* Enable Active Negation */
+			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
+		} else
 			esp->rev = FAS100A;
 		esp->min_period = ((4 * esp->ccycle) / 1000);
 	} else {
@@ -308,7 +311,8 @@ static void esp_reset_esp(struct esp *esp)
 
 	case FAS236:
 	case PCSCSI:
-		/* Fast 236, AM53c974 or HME */
+	case FSC:
+		/* Fast 236, AM53c974, FSC or HME */
 		esp_write8(esp->config2, ESP_CFG2);
 		if (esp->rev == FASHME) {
 			u8 cfg3 = esp->target[0].esp_config3;
@@ -2374,6 +2378,7 @@ static const char *esp_chip_names[] = {
 	"ESP236",
 	"FAS236",
 	"AM53C974",
+	"FSC",
 	"FAS100A",
 	"FAST",
 	"FASHME",
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index f764d64e1f25..a673dec1b8a1 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -78,12 +78,14 @@
 #define ESP_CONFIG3_IMS       0x80     /* ID msg chk'ng        (esp/fas236)  */
 #define ESP_CONFIG3_OBPUSH    0x80     /* Push odd-byte to dma (hme)         */
 
-/* ESP config register 4 read-write, found only on am53c974 chips */
-#define ESP_CONFIG4_RADE      0x04     /* Active negation */
-#define ESP_CONFIG4_RAE       0x08     /* Active negation on REQ and ACK */
-#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature */
-#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0 */
-#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1 */
+/* ESP config register 4 read-write, found on am53c974 and FSC chips */
+#define ESP_CONFIG4_BBTE      0x01     /* Back-to-back transfers     (fsc)   */
+#define ESP_CONGIG4_TEST      0x02     /* Transfer counter test mode (fsc)   */
+#define ESP_CONFIG4_RADE      0x04     /* Active negation   (am53c974/fsc)   */
+#define ESP_CONFIG4_RAE       0x08     /* Act. negation REQ/ACK (am53c974)   */
+#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature (am53c974)   */
+#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0    (am53c974)   */
+#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1    (am53c974)   */
 
 #define ESP_CONFIG_GE_12NS    (0)
 #define ESP_CONFIG_GE_25NS    (ESP_CONFIG_GE1)
@@ -209,10 +211,15 @@
 #define ESP_TEST_TS           0x04     /* Tristate test mode */
 
 /* ESP unique ID register read-only, found on fas236+fas100a only */
+#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
+
+#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
+
+/* Values for the ESP family bits */
 #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
 #define ESP_UID_F236          0x02     /* ESP FAS236   */
-#define ESP_UID_REV           0x07     /* ESP revision */
-#define ESP_UID_FAM           0xf8     /* ESP family   */
+#define ESP_UID_HME           0x0a     /* FAS HME      */
+#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
 
 /* ESP fifo flags register read-only */
 /* Note that the following implies a 16 byte FIFO on the ESP. */
@@ -264,6 +271,7 @@ enum esp_rev {
 	ESP236,
 	FAS236,
 	PCSCSI,  /* AM53c974 */
+	FSC,     /* NCR/Symbios Logic FSC */
 	FAS100A,
 	FAST,
 	FASHME,
-- 
2.17.1

Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Finn Thain]

On Thu, 14 Nov 2019, Kars de Jong wrote:

> The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
> or Emulex parts. This caused it to be detected as a FAS100A.
> 
> Unforunately, this meant the configuration of the CONFIG3 register was
> incorrect. This causes data transfer issues with FAST-SCSI targets.
> 
> The FSC also has the CONFIG4 register. It can be used to enable a feature
> called Active Negation which should always be enabled according to the data
> manual.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>

Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

A few observations follow. Not a NAK, just FYI...

> ---
>  drivers/scsi/esp_scsi.c | 19 ++++++++++++-------
>  drivers/scsi/esp_scsi.h | 24 ++++++++++++++++--------
>  2 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 4fc3eee3138b..e887ea3e514a 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -243,7 +243,7 @@ static void esp_set_all_config3(struct esp *esp, u8 val)
>  /* Reset the ESP chip, _not_ the SCSI bus. */
>  static void esp_reset_esp(struct esp *esp)
>  {
> -	u8 family_code, version;
> +	u8 family_code;
>  

This is not the best scope for this variable. You have to touch both lines 
(declaration and initialization) anyway, so you can easily improve this.

>  	/* Now reset the ESP chip */
>  	scsi_esp_cmd(esp, ESP_CMD_RC);
> @@ -257,13 +257,16 @@ static void esp_reset_esp(struct esp *esp)
>  	 */
>  	esp->max_period = ((35 * esp->ccycle) / 1000);
>  	if (esp->rev == FAST) {
> -		version = esp_read8(ESP_UID);
> -		family_code = (version & 0xf8) >> 3;
> -		if (family_code == 0x02)
> +		family_code = ESP_FAMILY(esp_read8(ESP_UID));
> +		if (family_code == ESP_UID_F236)
>  			esp->rev = FAS236;
> -		else if (family_code == 0x0a)
> +		else if (family_code == ESP_UID_HME)
>  			esp->rev = FASHME; /* Version is usually '5'. */
> -		else
> +		else if (family_code == ESP_UID_FSC) {
> +			esp->rev = FSC;
> +			/* Enable Active Negation */
> +			esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
> +		} else
>  			esp->rev = FAS100A;
>  		esp->min_period = ((4 * esp->ccycle) / 1000);
>  	} else {
> @@ -308,7 +311,8 @@ static void esp_reset_esp(struct esp *esp)
>  
>  	case FAS236:
>  	case PCSCSI:
> -		/* Fast 236, AM53c974 or HME */
> +	case FSC:
> +		/* Fast 236, AM53c974, FSC or HME */

This comment merely re-states the logic in the case labels. If you don't 
delete the comment, it has to be maintained along with the case labels. 
Consequently this patch is longer than it needs to be.

>  		esp_write8(esp->config2, ESP_CFG2);
>  		if (esp->rev == FASHME) {
>  			u8 cfg3 = esp->target[0].esp_config3;
> @@ -2374,6 +2378,7 @@ static const char *esp_chip_names[] = {
>  	"ESP236",
>  	"FAS236",
>  	"AM53C974",
> +	"FSC",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index f764d64e1f25..a673dec1b8a1 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -78,12 +78,14 @@
>  #define ESP_CONFIG3_IMS       0x80     /* ID msg chk'ng        (esp/fas236)  */
>  #define ESP_CONFIG3_OBPUSH    0x80     /* Push odd-byte to dma (hme)         */
>  
> -/* ESP config register 4 read-write, found only on am53c974 chips */
> -#define ESP_CONFIG4_RADE      0x04     /* Active negation */
> -#define ESP_CONFIG4_RAE       0x08     /* Active negation on REQ and ACK */
> -#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature */
> -#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0 */
> -#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1 */
> +/* ESP config register 4 read-write, found on am53c974 and FSC chips */
> +#define ESP_CONFIG4_BBTE      0x01     /* Back-to-back transfers     (fsc)   */
> +#define ESP_CONGIG4_TEST      0x02     /* Transfer counter test mode (fsc)   */
> +#define ESP_CONFIG4_RADE      0x04     /* Active negation   (am53c974/fsc)   */
> +#define ESP_CONFIG4_RAE       0x08     /* Act. negation REQ/ACK (am53c974)   */
> +#define ESP_CONFIG4_PWD       0x20     /* Reduced power feature (am53c974)   */
> +#define ESP_CONFIG4_GE0       0x40     /* Glitch eater bit 0    (am53c974)   */
> +#define ESP_CONFIG4_GE1       0x80     /* Glitch eater bit 1    (am53c974)   */
>  

You've added "(FSC)" and "(am53c974)" here, which is fine but you've 
repeated yourself in the comment, "found on am53c974 and FSC chips". The 
DRY principle applies here too (Don't Repeat Yourself).

>  #define ESP_CONFIG_GE_12NS    (0)
>  #define ESP_CONFIG_GE_25NS    (ESP_CONFIG_GE1)
> @@ -209,10 +211,15 @@
>  #define ESP_TEST_TS           0x04     /* Tristate test mode */
>  
>  /* ESP unique ID register read-only, found on fas236+fas100a only */
> +#define ESP_UID_FAM           0xf8     /* ESP family bitmask */
> +
> +#define ESP_FAMILY(uid) (((uid) & ESP_UID_FAM) >> 3)
> +
> +/* Values for the ESP family bits */
>  #define ESP_UID_F100A         0x00     /* ESP FAS100A  */
>  #define ESP_UID_F236          0x02     /* ESP FAS236   */
> -#define ESP_UID_REV           0x07     /* ESP revision */
> -#define ESP_UID_FAM           0xf8     /* ESP family   */
> +#define ESP_UID_HME           0x0a     /* FAS HME      */
> +#define ESP_UID_FSC           0x14     /* NCR/Symbios Logic FSC */
>  
>  /* ESP fifo flags register read-only */
>  /* Note that the following implies a 16 byte FIFO on the ESP. */
> @@ -264,6 +271,7 @@ enum esp_rev {
>  	ESP236,
>  	FAS236,
>  	PCSCSI,  /* AM53c974 */
> +	FSC,     /* NCR/Symbios Logic FSC */

Is there a difference between "FSC" and "NCR/Symbios Logic FSC"? The 
reader has to infer that not all "FSC" chips are equivalent and that the 
brand name is significant. Whereas, there is real value in putting a part 
code here (as in the line above).

-- 

>  	FAS100A,
>  	FAST,
>  	FASHME,
> 

Re: [PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

For simplicity, the entire patch series would normally show the same 
version number (v3). Also, the series would normally be a thread unto 
itself, rather than a sub-thread.

On Thu, 14 Nov 2019, Kars de Jong wrote:

> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.
> 
> Add a comment to the enum explaining this.
> 
> Also, the actual values for the enum fields are irrelevant, so remove the
> explicit values (suggested by Geert Uytterhoeven). This makes adding a new
> field in the middle of the enum easier.
> 
> Finally, move the PCSCSI definition to the right place in the enum. In its
> previous location, at the end of the enum, the wrong values are written to
> the CONFIG3 register when used with FAST-SCSI targets.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>

This is Hannes' code so I'll leave it up to him to review this change.

I presume this is untested on PCSCSI hardware. ISTR that there's an 
emulator for that board somewhere...

-- 

> ---
>  drivers/scsi/esp_scsi.c |  2 +-
>  drivers/scsi/esp_scsi.h | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..4fc3eee3138b 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
>  	"ESP100A",
>  	"ESP236",
>  	"FAS236",
> +	"AM53C974",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> -	"AM53C974",
>  };
>  
>  static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 91b32f2a1a1b..f764d64e1f25 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -257,15 +257,16 @@ struct esp_cmd_priv {
>  };
>  #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
>  
> +/* NOTE: this enum is ordered based on chip features! */
>  enum esp_rev {
> -	ESP100     = 0x00,  /* NCR53C90 - very broken */
> -	ESP100A    = 0x01,  /* NCR53C90A */
> -	ESP236     = 0x02,
> -	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	ESP100,  /* NCR53C90 - very broken */
> +	ESP100A, /* NCR53C90A */
> +	ESP236,
> +	FAS236,
> +	PCSCSI,  /* AM53c974 */
> +	FAS100A,
> +	FAST,
> +	FASHME,
>  };
>  
>  struct esp_cmd_entry {
> 

Re: [PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Finn!

Op vr 15 nov. 2019 om 03:13 schreef Finn Thain <fthain@telegraphics.com.au>:
> For simplicity, the entire patch series would normally show the same
> version number (v3).

OK, thanks.

> Also, the series would normally be a thread unto itself, rather than a sub-thread.

OK, I followed an example from the git-send-email manual ("which
avoids breaking threads to provide a new patch series").
I found the relevant convention in "Submitting patches", I didn't
notice that before. Thanks.

> ...
>
> This is Hannes' code so I'll leave it up to him to review this change.
>
> I presume this is untested on PCSCSI hardware. ISTR that there's an
> emulator for that board somewhere...

Yes, I don't have any, so I could not test it. QEMU emulates it, but
it doesn't care about CONFIG3 etc. at all.

Thanks for your reviews!

Kind regards,

Kars.

Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

Hi Finn,

Thanks again for the review!

Op vr 15 nov. 2019 om 03:09 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Thu, 14 Nov 2019, Kars de Jong wrote:
>
> > The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
> > or Emulex parts. This caused it to be detected as a FAS100A.
> >
> > Unforunately, this meant the configuration of the CONFIG3 register was
> > incorrect. This causes data transfer issues with FAST-SCSI targets.
> >
> > The FSC also has the CONFIG4 register. It can be used to enable a feature
> > called Active Negation which should always be enabled according to the data
> > manual.
> >
> > Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
>
> Reviewed-by: Finn Thain <fthain@telegraphics.com.au>
>
> This is not the best scope for this variable. You have to touch both lines
> (declaration and initialization) anyway, so you can easily improve this.

Good point, I will move it to inside the  "if (esp->rev) == FAST) {" block.

> > -             /* Fast 236, AM53c974 or HME */
> > +     case FSC:
> > +             /* Fast 236, AM53c974, FSC or HME */
>
> This comment merely re-states the logic in the case labels. If you don't
> delete the comment, it has to be maintained along with the case labels.
> Consequently this patch is longer than it needs to be.

Yes, I agree. I'll remove the comment.

>
> You've added "(FSC)" and "(am53c974)" here, which is fine but you've
> repeated yourself in the comment, "found on am53c974 and FSC chips". The
> DRY principle applies here too (Don't Repeat Yourself).

I'll also remove that comment.

> > @@ -264,6 +271,7 @@ enum esp_rev {
> >       ESP236,
> >       FAS236,
> >       PCSCSI,  /* AM53c974 */
> > +     FSC,     /* NCR/Symbios Logic FSC */
>
> Is there a difference between "FSC" and "NCR/Symbios Logic FSC"?

No, the respective data manuals all name these variants as "FSC".

> The reader has to infer that not all "FSC" chips are equivalent and that the
> brand name is significant. Whereas, there is real value in putting a part
> code here (as in the line above).

Agreed, I'll add the part code instead.

Kind regards,

Kars.