Hi Michael,
Thanks for the review!
Op wo 30 okt. 2019 om 01:23 schreef Michael Schmitz <schmitzmic@xxxxxxxxx>:
Hi Kars,
thanks for your patch. Comments inline below. Note that in order to get
this patch integrated into the next release, you should resend it to
linux-scsi@xxxxxxxxxxxxxxx. The SCSI maintainers won't pick it up here.
I know, I first wanted you guys to have a look at it before involving them.
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..6b34a5764de5 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -263,7 +263,11 @@ static void esp_reset_esp(struct esp *esp)
esp->rev = FAS236;
else if (family_code == 0x0a)
esp->rev = FASHME; /* Version is usually '5'. */
- else
+ else if (family_code == 0x14) {
+ esp->rev = FSC;
+ /* Enable Active Negation */
+ esp_write8(ESP_CONFIG4_RADE, ESP_CFG4);
Please move that into the below section ...
+ } 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;
... here, where chip-specific config bits are otherwise set. Just to
keep with driver style.
Yes, I considered that, but the setting of CONFIG4 for the PCSCSI is
also done immediately after it is detected (lines 272-284). So in a
way, I kept to the driver style as well ;-).
Also consider whether esp->radelay can be set shorter than the default
96 with active negation enabled (couldn't make sense of the data book on
that point - may well be unrelated to active negation).
The old driver used 16 for all the FAST variants except FAS100A. I
also tried to get that from the data manual but there doesn't seem to
be any info about it. I never had any issues with having it set to 16.
16 means no delay for REQ/ACK de-assertion and 1/2 clock delay for
REQ/ACK assertion.
The new driver uses 96, which means 1/2 clock delay for REQ/ACK
de-assertion and 1 clock delay for REQ/ACK assertion (when using
FASTCLK in CONFIG3).
Why was 96 chosen for the new driver, do you know?
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..b60ea3e5e0eb 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -211,6 +211,7 @@
/* ESP unique ID register read-only, found on fas236+fas100a only */
#define ESP_UID_F100A 0x00 /* ESP FAS100A */
#define ESP_UID_F236 0x02 /* ESP FAS236 */
+#define ESP_UID_FSC 0x14 /* NCR/Symbios Logic FSC */
#define ESP_UID_REV 0x07 /* ESP revision */
#define ESP_UID_FAM 0xf8 /* ESP family */
Can't find these used anywhere in the code ... I suppose they could be
removed, or the detection code above rewritten to make use of these macros.
Yes, I noticed that too... I prefer the latter.
Thanks,
Kars.
