Linux-m68k Archive on lore.kernel.org
 help / color / Atom feed
From: Finn Thain <fthain@telegraphics.com.au>
To: Kars de Jong <jongk@linux-m68k.org>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Hannes Reinecke <hare@suse.com>,
	linux-scsi@vger.kernel.org, linux-m68k@vger.kernel.org,
	schmitzmic@gmail.com
Subject: Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip
Date: Fri, 15 Nov 2019 13:09:37 +1100 (AEDT)
Message-ID: <alpine.LNX.2.21.1.1911151212001.9@nippy.intranet> (raw)
In-Reply-To: <20191114222518.2441-3-jongk@linux-m68k.org>


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,
> 

  reply index

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-24 20:56 Amiga PCMCIA network card support Andreas 'count' Kotes
2019-10-25  7:25 ` Kars de Jong
2019-10-25 11:49   ` Andreas 'count' Kotes
2019-10-28  9:19     ` Kars de Jong
2019-10-28 11:08       ` John Paul Adrian Glaubitz
2019-10-28 13:00         ` Kars de Jong
2019-10-28 13:20           ` John Paul Adrian Glaubitz
2019-10-28 15:39             ` ESP SCSI driver (was: Amiga PCMCIA network card support) Kars de Jong
2019-10-28 18:32               ` Michael Schmitz
2019-10-29  9:37                 ` Kars de Jong
2019-10-29 20:20                   ` ESP SCSI driver Michael Schmitz
2019-10-29 22:05                   ` [PATCH] esp_scsi: Add support for FSC chip Kars de Jong
2019-10-30  0:23                     ` Michael Schmitz
2019-10-30  7:11                       ` Kars de Jong
2019-10-30 18:42                         ` Michael Schmitz
2019-10-30  0:31                     ` Finn Thain
2019-10-30  1:06                       ` Michael Schmitz
2019-10-30  7:25                         ` Kars de Jong
2019-10-30  8:45                           ` Geert Uytterhoeven
2019-10-30  9:08                             ` Kars de Jong
2019-10-30 18:34                               ` Michael Schmitz
2019-10-30 18:52                             ` Brad Boyer
2019-10-30  7:22                       ` Kars de Jong
2019-10-30 23:15                         ` Finn Thain
2019-11-12 18:57                     ` [PATCH 0/2] Some esp_scsi updates Kars de Jong
2019-11-12 18:57                       ` [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum Kars de Jong
2019-11-12 23:07                         ` Finn Thain
2019-11-13  8:00                           ` Kars de Jong
2019-11-13 22:25                             ` Finn Thain
2019-11-13 14:22                         ` Christoph Hellwig
2019-11-13 15:03                           ` Kars de Jong
2019-11-12 18:57                       ` [PATCH 2/2] esp_scsi: Add support for FSC chip Kars de Jong
2019-11-12 23:18                         ` Finn Thain
2019-11-12 23:57                           ` Finn Thain
2019-11-13  9:30                           ` Kars de Jong
2019-11-13 22:24                             ` Finn Thain
2019-11-14 21:59                       ` [PATCH v2 0/2] Some esp_scsi updates Kars de Jong
2019-11-14 21:59                         ` [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum Kars de Jong
2019-11-14 22:06                           ` Kars de Jong
2019-11-14 21:59                         ` [PATCH 2/2] esp_scsi: Add support for FSC chip Kars de Jong
2019-11-14 22:07                           ` Kars de Jong
2019-11-14 22:25                         ` [PATCH v3 0/2] Some esp_scsi updates Kars de Jong
2019-11-14 22:25                           ` [PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum Kars de Jong
2019-11-15  2:13                             ` Finn Thain
2019-11-15  7:04                               ` Kars de Jong
2019-11-14 22:25                           ` [PATCH v3 2/2] esp_scsi: Add support for FSC chip Kars de Jong
2019-11-15  2:09                             ` Finn Thain [this message]
2019-11-18 13:27                               ` Kars de Jong
2019-11-09 19:14                   ` [PATCH] zorro_esp: increase maximum dma length to 65536 bytes Kars de Jong
2019-11-09 20:12                     ` James Bottomley
2019-11-10  2:36                       ` Michael Schmitz
2019-11-10  9:01                         ` Kars de Jong
2019-11-10 19:26                           ` Michael Schmitz
2019-11-11  8:47                             ` Kars de Jong
2019-11-10 19:35                         ` James Bottomley
2019-11-12 17:55                           ` [PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane) Kars de Jong
2019-11-12 22:46                             ` Finn Thain
2019-11-13  2:27                             ` Martin K. Petersen
2019-11-12  9:34                         ` [PATCH] zorro_esp: increase maximum dma length to 65536 bytes Kars de Jong
2019-11-09 22:53                     ` Finn Thain
2019-11-10  9:06                       ` Kars de Jong
2019-10-28 23:38               ` ESP SCSI driver (was: Amiga PCMCIA network card support) Finn Thain
2019-10-29 11:52                 ` Kars de Jong
2019-10-29 20:16                   ` ESP SCSI driver Michael Schmitz
2019-10-28 22:31           ` Amiga PCMCIA network card support Finn Thain
2019-10-29  8:56           ` FOSDEM (was: Re: Amiga PCMCIA network card support) Geert Uytterhoeven
2019-10-29  9:13             ` John Paul Adrian Glaubitz
2019-10-28 22:08       ` Amiga PCMCIA network card support Finn Thain
2019-10-29  9:00       ` Geert Uytterhoeven
2019-10-29  9:12         ` John Paul Adrian Glaubitz
2019-10-29  9:14           ` Geert Uytterhoeven
2019-10-29  9:20             ` John Paul Adrian Glaubitz
2019-10-29  9:40         ` Kars de Jong
2019-10-28  6:57   ` Michael Schmitz

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LNX.2.21.1.1911151212001.9@nippy.intranet \
    --to=fthain@telegraphics.com.au \
    --cc=hare@suse.com \
    --cc=jejb@linux.ibm.com \
    --cc=jongk@linux-m68k.org \
    --cc=linux-m68k@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=schmitzmic@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-m68k Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-m68k/0 linux-m68k/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-m68k linux-m68k/ https://lore.kernel.org/linux-m68k \
		linux-m68k@vger.kernel.org linux-m68k@lists.linux-m68k.org
	public-inbox-index linux-m68k

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-m68k


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git