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

[Kars de Jong <jongk@linux-m68k.org>]

Hi Michael,

Thanks for the review!

Op wo 30 okt. 2019 om 01:23 schreef Michael Schmitz <schmitzmic@gmail.com>:
>
> 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@vger.kernel.org. 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.