| Message ID | 20191112185710.23988-3-jongk@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some esp_scsi updates | expand |
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?
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. --
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.
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.
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,
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(-)