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

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Kars,

On Wed, Oct 30, 2019 at 8:11 PM Kars de Jong <jongk@linux-m68k.org> wrote:
> > > 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 ;-).

Fair enough.

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

No, you'd have to ask DaveM. Maybe some users reported problems with
fast transfers...

Cheers,

    Michael