Amiga PCMCIA network card support

Amiga PCMCIA network card support

[Andreas 'count' Kotes]

Hello dear list,

I'm new to the party, so please bear with my with stupid questions and
infractions on policy or anything else - I'm willing to learn, and am
receptive to change, please just poke me if needed :)

Replying to a somewhat older (but not too old) mail:

On Fri, 21 Dec 2018, Michael Schmitz wrote:
>+if APNE
>+config APNE100MBIT
>+	bool "PCMCIA NE2000 100MBit support"
>+	default n
>+	depends on NE2000=n && PCMCIA_AXNET=n
>+	depends on PCMCIA_PCNET=n && STNIC=n && ULTRA=n && WD80x3=n
>
>In all honesty, I doubt anyone could ever use the stnic, ultra and wd
>drivers on m68k (Geert?) so these could be omitted as well. Not sure
>pcnet_cs or axnet_cs are possible to use on the Amiga PCMCIA slot, so
>all that remains in practice is the ne driver (which is used on Atari).

what is the underlying problem here anyway?

Looking at http://www.g-mb.de/pcmcia_e.html, there are quite a few cards
supported by cnet.device under AmigaOS, and seem to be working well.

I've got an Apollo RE450CT (at least I think it is - FCC ID: MQ4EC2T)
which works just fine under AmigaOS, but isn't detected by apne.ko at
all - the AmigaOS driver has had the last change to it in August 2007.

One very common card amongst AmigaOS developers with PCMCIA seems to be
the 3Com Etherlink III (3c589), which seems have had a sufficient driver
update in February 2010.

So, the situation is "ok" under AmigaOS ... but Linux support isn't there.

Looking at various forums and mailing lists, people willing to use (or
at least try) to try Linux on an A1200 or A600 seems to fail at this
stage on a regular basis, and some report rather desperate searches for
supported PCMCIA cards on various sites where people trade this kind of
stuff - usually, to no success.

If I look at the amount of people who do report success, consider the
amount of people who talk about this issue at all, and then wildly guess
how many people might have tried and failed and not said a thing, I
think there might be a quite reasonable userbase unlockable by improving
support for PCMCIA cards. The A1200 and A600 are small and sweet boxes,
and companies like Individual Computer are still building accelerators
that make them quite usable for surprising amount of tasks.

Individual Computers also created the RapidRoad extension, which adds
USB support - and AmigaOS software support is also surprisingly good.
I added a Mouse and a Keyboard and they "just worked", same as a USB
network device (ASIX AX88772), which also "just works".

I'd like to work on support for RapidRoad (shouldn't be too much work,
it uses a ST-Ericsson's isp1763 for which Android support already exists
(although probably not for the 8-bit mode it uses), and supporting it
when connected to an X-Surf should afterwards also be possible, if I
read http://wiki.icomp.de/wiki/RapidRoad correct ...

... but, all of this is no fun at all without network support under
Linux :( .. I'll order a GuruNet/Plipbox style device and shall see how
that helps me, but ...

What needs to happen for stable, reliable support for existing PCMCIA
network cards people already have? How can we unblock those possible
Linux users?

cheers from Berlin,

  count

Re: Amiga PCMCIA network card support

[Kars de Jong]

Op do 24 okt. 2019 om 22:56 schreef Andreas 'count' Kotes
<count-vger.kernel.org+linux-
> What needs to happen for stable, reliable support for existing PCMCIA
> network cards people already have? How can we unblock those possible
> Linux users?

That would involve quite a few things. I did most of this back in 2.6
times, and had the standard pcnet_cs, 3c589_cs and serial_cs drivers
working on my Amiga 1200. Some of it was a bit hackish though, changes
to <asm/io.h> were discussed but never implemented. The need to be
able to build a multi-machine kernel (with Atari, Q40 and Amiga
support in a single kernel) was one of the things that interfered (it
could probably be solved by adding an extra level of indirection ;-)).
Then Real Life happened and I didn't touch my Amiga for 10+ years...

1) A proper driver for the PCMCIA slot, including hot plugging and
proper controller reset. This involves rewriting part of the IRQ
handling to make Gayle a separate IRQ controller instead of the hard
coded hack that is used now. That shouldn't be too hard. Support for
setting up different data path widths (IO_DATA_PATH_WIDTH_16 etc.)
will require changes to <asm/io.h>, see point 2.
2) Rework <asm/io.h> so inb()/inw()/inl(), outb()/outw()/outl(),
insb()/insw()/insl() and outsb()/outsw()/outsl() "just work" for
PCMCIA address space without having to create ugly hacks in the
generic drivers. This is not trivial because of the strange memory
mapping of the Amiga PCMCIA slot.
3) ...
4) Profit!

Oh, the 3c589_cs driver needed an ugly hack because some of the
registers just didn't read out properly on the Amiga with 16-bit
access. That issue is also described in the source code for the
AmigaOS driver.

Kind regards,

Kars.

Re: Amiga PCMCIA network card support

[Andreas 'count' Kotes]

Hello Kars,

On 25.10.19 09:25, Kars de Jong wrote:
> Op do 24 okt. 2019 om 22:56 schreef Andreas 'count' Kotes
> <count-vger.kernel.org+linux-
>> What needs to happen for stable, reliable support for existing PCMCIA
>> network cards people already have? How can we unblock those possible
>> Linux users?
> 
> That would involve quite a few things. I did most of this back in 2.6
> times, and had the standard pcnet_cs, 3c589_cs and serial_cs drivers
> working on my Amiga 1200. Some of it was a bit hackish though, changes
> to <asm/io.h> were discussed but never implemented. The need to be
> able to build a multi-machine kernel (with Atari, Q40 and Amiga
> support in a single kernel) was one of the things that interfered (it
> could probably be solved by adding an extra level of indirection ;-)).
> Then Real Life happened and I didn't touch my Amiga for 10+ years...

.. hmm, is this something that's likely to change? ;)

> 1) A proper driver for the PCMCIA slot, including hot plugging and
> proper controller reset. This involves rewriting part of the IRQ
> handling to make Gayle a separate IRQ controller instead of the hard
> coded hack that is used now. That shouldn't be too hard. Support for
> setting up different data path widths (IO_DATA_PATH_WIDTH_16 etc.)
> will require changes to <asm/io.h>, see point 2.
> 2) Rework <asm/io.h> so inb()/inw()/inl(), outb()/outw()/outl(),
> insb()/insw()/insl() and outsb()/outsw()/outsl() "just work" for
> PCMCIA address space without having to create ugly hacks in the
> generic drivers. This is not trivial because of the strange memory
> mapping of the Amiga PCMCIA slot.

ooooof. while neither sounds undoable (and looking at
http://www.ianstedman.co.uk/downloads/A1200FuncSpec.txt and
https://www.amigacoding.com/index.php/Amiga_memory_map), this is way
beyond my limited low-level experience and rusty Amiga coding experience
for me to do (by) myself - but I'd totally be willing to help kick off
and aid any serious effort for that ... or even let myself be mentored
into doing it.

I should also be able to set up an A1200 with a remotely switchable
power socket and remotely accessible Linux serial console and PLIP
network access that boots into alternating kernels to allow remote
development for interested people.

Kars, you sound like you're in Europe - will you be on the Chaos
Communcation Congress in Leipzig this year? Will there be a Linux m68k
table?

I don't know how well bounties (like
https://www.bountysource.com/issues/80706251-m68k-convert-the-backend-to-mode_cc-so-it-can-be-kept-in-future-releases)
work, but I'd certainly willing to chip in with money, time and
resources to get this addressed for good.

I think cleaning up the IRQ handling etc would also be (another?) good
showcase of linux-m68k and the Amiga platform being very much alive,
contrary to what Linus might think :D

all the best from Berlin,

  count

Re: Amiga PCMCIA network card support

[Kars de Jong]

Op vr 25 okt. 2019 om 13:49 schreef Andreas 'count' Kotes
<count-vger.kernel.org+linux-m68k@flatline.de>:
>
> Hello Kars,
>
> On 25.10.19 09:25, Kars de Jong wrote:
> > Op do 24 okt. 2019 om 22:56 schreef Andreas 'count' Kotes
> > <count-vger.kernel.org+linux-
> >> What needs to happen for stable, reliable support for existing PCMCIA
> >> network cards people already have? How can we unblock those possible
> >> Linux users?
> > ...
> > Then Real Life happened and I didn't touch my Amiga for 10+ years...
>
> .. hmm, is this something that's likely to change? ;)

I've dusted it off and powered it on this weekend. It still works (RTC
needs a new battery though), and the 2.6.18 kernel that I modified
many years ago still works fine with the pcnet_cs driver and my
Dynalink L10C card.

I then tried a build of master of Geert's tree, which unfortunately
doesn't fully work due to issues with the Blizzard ESP driver. I also
had similar issues with the last 2.6.2x kernel I tested in 2008(?),
after switching to the new ESP driver.
The drive is identified, set to synchronous 10 MHz mode, some
transfers are done but then it locks up. After the ESP driver runs its
recovery procedure, the bash that I run as init process exits with
code 4.

> Kars, you sound like you're in Europe - will you be on the Chaos
> Communcation Congress in Leipzig this year? Will there be a Linux m68k
> table?

Yes, I'm from Hengelo (the Netherlands), but no, I won't be attending.
I have no idea whether there will be a Linux/m68k presence.

> I don't know how well bounties (like
> https://www.bountysource.com/issues/80706251-m68k-convert-the-backend-to-mode_cc-so-it-can-be-kept-in-future-releases)
> work, but I'd certainly willing to chip in with money, time and
> resources to get this addressed for good.

I don't need money, I need time :-) I looked at the changes I made to
2.6.18, most of it is pretty self contained and should still work on
5.x without too many changes, but the io.h changes are more hairy.
Also, someone once tested this on his A1200 which had a 68030 and I
believe it broke. Something about ioremap() not accepting physical
addresses that were already mapped by a fixed mapping (Zorro II
address space).

> I think cleaning up the IRQ handling etc would also be (another?) good
> showcase of linux-m68k and the Amiga platform being very much alive,
> contrary to what Linus might think :D

Linus knows it's alive, he just seems to wonder why it won't die ;-)
I will try to prepare a patch for the IRQ handling.

Kind regards,

Kars.

Re: Amiga PCMCIA network card support

[John Paul Adrian Glaubitz]

Hi Kars!

On 10/28/19 10:19 AM, Kars de Jong wrote:
>>> Then Real Life happened and I didn't touch my Amiga for 10+ years...
>>
>> .. hmm, is this something that's likely to change? ;)
> 
> I've dusted it off and powered it on this weekend. It still works (RTC
> needs a new battery though), and the 2.6.18 kernel that I modified
> many years ago still works fine with the pcnet_cs driver and my
> Dynalink L10C card.

Cool!

> I then tried a build of master of Geert's tree, which unfortunately
> doesn't fully work due to issues with the Blizzard ESP driver. I also

Could you report these issues? Is it the zorro_esp driver that's causing
problems? Normally the kernel should build fine but if there are issues,
Michael Schmitz would probably be interested to hear about them.

> had similar issues with the last 2.6.2x kernel I tested in 2008(?),
> after switching to the new ESP driver.
> The drive is identified, set to synchronous 10 MHz mode, some
> transfers are done but then it locks up. After the ESP driver runs its
> recovery procedure, the bash that I run as init process exits with
> code 4.

Wait, do you have build issues or do you have issues running the driver?

The new zorro_esp has seen a lot of rewrite. So old bugs might have been
fixed now.

>> Kars, you sound like you're in Europe - will you be on the Chaos
>> Communcation Congress in Leipzig this year? Will there be a Linux m68k
>> table?
> 
> Yes, I'm from Hengelo (the Netherlands), but no, I won't be attending.
> I have no idea whether there will be a Linux/m68k presence.

I don't think there will be any m68k folk at CCC, I'm also not attending
as the event has become too political for my taste.

But I will be attending FOSDEM and Geert is coming as well (at least he
has been to every FOSDEM so far ;)), so we can maybe have a small m68k
meeting. I'm coming with two friends who are also Linux hackers.

>> I don't know how well bounties (like
>> https://www.bountysource.com/issues/80706251-m68k-convert-the-backend-to-mode_cc-so-it-can-be-kept-in-future-releases)
>> work, but I'd certainly willing to chip in with money, time and
>> resources to get this addressed for good.
> 
> I don't need money, I need time :-) I looked at the changes I made to
> 2.6.18, most of it is pretty self contained and should still work on
> 5.x without too many changes, but the io.h changes are more hairy.
> Also, someone once tested this on his A1200 which had a 68030 and I
> believe it broke. Something about ioremap() not accepting physical
> addresses that were already mapped by a fixed mapping (Zorro II
> address space).

Doesn't this depend on the type of accelerator used? I think some of them
cause incompatibilities with the PCMCIA slot, don't they?

>> I think cleaning up the IRQ handling etc would also be (another?) good
>> showcase of linux-m68k and the Amiga platform being very much alive,
>> contrary to what Linus might think :D
> 
> Linus knows it's alive, he just seems to wonder why it won't die ;-)

Because the Amiga community is absolutely die-hard :). This year's Amiga
conference was even extended to two days because of the public interest.

> I will try to prepare a patch for the IRQ handling.

Nice. Thank you! Really cool to see more and more people starting to work
on Linux/m68k again :). This shows that all the effort I put into Debian/m68k
was worth it!

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: Amiga PCMCIA network card support

[Kars de Jong]

Hello!

Op ma 28 okt. 2019 om 12:08 schreef John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de>:
>
> Hi Kars!
> > had similar issues with the last 2.6.2x kernel I tested in 2008(?),
> > after switching to the new ESP driver.
> > The drive is identified, set to synchronous 10 MHz mode, some
> > transfers are done but then it locks up. After the ESP driver runs its
> > recovery procedure, the bash that I run as init process exits with
> > code 4.
>
> Wait, do you have build issues or do you have issues running the driver?

Running. Building was fine.

> The new zorro_esp has seen a lot of rewrite. So old bugs might have been
> fixed now.

Yes, for Mac support etc. Unfortunately it doesn't work for me. For
starters. it does not identify my chip correctly, it says it's a
FAS100A (it's actually a Symbios Logic 53CF94-2 which is more like a
FAS236).
But after I fixed that it still doesn't work.

One difference between the new driver and the old one is its support
for tagged commands. I haven't found an easy way to disable that yet
though.

> But I will be attending FOSDEM and Geert is coming as well (at least he
> has been to every FOSDEM so far ;)), so we can maybe have a small m68k
> meeting. I'm coming with two friends who are also Linux hackers.

Would be cool... Last time I went to something like that was the
Oldenburg meeting of 1998 ;-)

> > Also, someone once tested this on his A1200 which had a 68030 and I
> > believe it broke. Something about ioremap() not accepting physical
> > addresses that were already mapped by a fixed mapping (Zorro II
> > address space).
>
> Doesn't this depend on the type of accelerator used? I think some of them
> cause incompatibilities with the PCMCIA slot, don't they?

I don't think that was the issue. Roman Zippel provided a patch which
fixed it, but his last comment on the matter was

> ... it should have also worked without the patch, so
> there is still some other problem, I'd like to find.
>
> bye, Roman

We never got to that, and Roman seems to have disappeared.

> > I will try to prepare a patch for the IRQ handling.
>
> Nice. Thank you! Really cool to see more and more people starting to work
> on Linux/m68k again :). This shows that all the effort I put into Debian/m68k
> was worth it!

Thanks, I'll try installing a more recent Debian on my Amiga.

Kind regards,

Kars.

Re: Amiga PCMCIA network card support

[John Paul Adrian Glaubitz]

On 10/28/19 2:00 PM, Kars de Jong wrote:
>> The new zorro_esp has seen a lot of rewrite. So old bugs might have been
>> fixed now.
> 
> Yes, for Mac support etc. Unfortunately it doesn't work for me. For
> starters. it does not identify my chip correctly, it says it's a
> FAS100A (it's actually a Symbios Logic 53CF94-2 which is more like a
> FAS236).
> But after I fixed that it still doesn't work.

It works perfectly fine with my Amiga 4000 and the SCSI controller on the
Cyberstorm: http://amiga.resource.cx/exp/cyberstorm1.

Michael actually tested everything on the Amiga 4000.

>> But I will be attending FOSDEM and Geert is coming as well (at least he
>> has been to every FOSDEM so far ;)), so we can maybe have a small m68k
>> meeting. I'm coming with two friends who are also Linux hackers.
> 
> Would be cool... Last time I went to something like that was the
> Oldenburg meeting of 1998 ;-)

Haha.

>>> I will try to prepare a patch for the IRQ handling.
>>
>> Nice. Thank you! Really cool to see more and more people starting to work
>> on Linux/m68k again :). This shows that all the effort I put into Debian/m68k
>> was worth it!
> 
> Thanks, I'll try installing a more recent Debian on my Amiga.

Here's a current image:

> https://cdimage.debian.org/cdimage/ports/2019-10-24/debian-10.0-m68k-NETINST-1.iso

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

ESP SCSI driver (was: Amiga PCMCIA network card support)

[Kars de Jong]

Op ma 28 okt. 2019 om 14:20 schreef John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de>:
>
> On 10/28/19 2:00 PM, Kars de Jong wrote:
> >> The new zorro_esp has seen a lot of rewrite. So old bugs might have been
> >> fixed now.
> >
> > Yes, for Mac support etc. Unfortunately it doesn't work for me. For
> > starters. it does not identify my chip correctly, it says it's a
> > FAS100A (it's actually a Symbios Logic 53CF94-2 which is more like a
> > FAS236).
> > But after I fixed that it still doesn't work.
>
> It works perfectly fine with my Amiga 4000 and the SCSI controller on the
> Cyberstorm: http://amiga.resource.cx/exp/cyberstorm1.
>
> Michael actually tested everything on the Amiga 4000.

It probably uses a different chip. The text and images show it's a
QLogic FAS216 (which is the same as a FAS236, FAS236 is the
differential variant). The type should more accurately be presented as
"FAS2x6" since you can't tell which type it is in software.

Some of the bits in CONFIG3 have completely different meanings between
these chip variants, and because of the misdetection it is quite wrong
for the 53CF9x-2 in my A1200.

I will try out some things this evening, I naively added the FSC to
the end of the enum, but the code does things like "if (esp->rev >=
FAS100A)" which break horribly then.

It looks like it might work if I add my chip type between FAS236 and
FAS100A in the esp_rev enum.

Kind regards,

Kars.

Re: ESP SCSI driver (was: Amiga PCMCIA network card support)

[Michael Schmitz]

Kars,

Don't mess with CONFIG3 - we've tried all that, and in the end had to
use PIO to transfer message bytes. Problems with tagged queueing in
the ESP driver are usually all due to the DMA not transferring single
bytes on Amiga. The additional message byte required to see a reselect
for a tagged command complete remains stuck in the ESP fifo. Can't see
how this would be affected by the driver mis-identifying the chip as
FAS100A.

Regardig mis-detection of the chip revision - can you print the
'version' and 'family_code' variables in the esp->rev == FAST branch
of esp_reset_esp() please?

Can you also send the Zorro device ID data for your board, just so I
know which one of the Blizzard SCSI variants we are talking about
here?

Cheers,

    Michael


On Tue, Oct 29, 2019 at 4:39 AM Kars de Jong <jongk@linux-m68k.org> wrote:
>
> Op ma 28 okt. 2019 om 14:20 schreef John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de>:
> >
> > On 10/28/19 2:00 PM, Kars de Jong wrote:
> > >> The new zorro_esp has seen a lot of rewrite. So old bugs might have been
> > >> fixed now.
> > >
> > > Yes, for Mac support etc. Unfortunately it doesn't work for me. For
> > > starters. it does not identify my chip correctly, it says it's a
> > > FAS100A (it's actually a Symbios Logic 53CF94-2 which is more like a
> > > FAS236).
> > > But after I fixed that it still doesn't work.
> >
> > It works perfectly fine with my Amiga 4000 and the SCSI controller on the
> > Cyberstorm: http://amiga.resource.cx/exp/cyberstorm1.
> >
> > Michael actually tested everything on the Amiga 4000.
>
> It probably uses a different chip. The text and images show it's a
> QLogic FAS216 (which is the same as a FAS236, FAS236 is the
> differential variant). The type should more accurately be presented as
> "FAS2x6" since you can't tell which type it is in software.
>
> Some of the bits in CONFIG3 have completely different meanings between
> these chip variants, and because of the misdetection it is quite wrong
> for the 53CF9x-2 in my A1200.
>
> I will try out some things this evening, I naively added the FSC to
> the end of the enum, but the code does things like "if (esp->rev >=
> FAS100A)" which break horribly then.
>
> It looks like it might work if I add my chip type between FAS236 and
> FAS100A in the esp_rev enum.
>
> Kind regards,
>
> Kars.

Re: ESP SCSI driver (was: Amiga PCMCIA network card support)

[Kars de Jong]

Hi Michael,

Op ma 28 okt. 2019 om 19:32 schreef Michael Schmitz <schmitzmic@gmail.com>:
> Don't mess with CONFIG3 - we've tried all that, and in the end had to
> use PIO to transfer message bytes. Problems with tagged queueing in
> the ESP driver are usually all due to the DMA not transferring single
> bytes on Amiga. The additional message byte required to see a reselect
> for a tagged command complete remains stuck in the ESP fifo. Can't see
> how this would be affected by the driver mis-identifying the chip as
> FAS100A.

And yet that was exactly what caused the issue. It was setting bit 1
of CONFIG3 - ESP_CONFIG3_FAST (FAS100A) instead of bit 3 -
ESP_CONFIG3_FCLK (FAS236 and also FSC). Bit 1 means ESP_CONFIG3_ADMA
on the FSC, and having bit 0 cleared and bit 1 set is actually an
illegal combination on the FSC (marked "Reserved" in the data manual).

> Regardig mis-detection of the chip revision - can you print the
> 'version' and 'family_code' variables in the esp->rev == FAST branch
> of esp_reset_esp() please?

Family code 0x14, version 2. That's what was also used in the old driver.

> >
> > I will try out some things this evening, I naively added the FSC to
> > the end of the enum, but the code does things like "if (esp->rev >=
> > FAS100A)" which break horribly then.
> >
> > It looks like it might work if I add my chip type between FAS236 and
> > FAS100A in the esp_rev enum.

And that indeed fixed it. I ran "fsck -n" on both partitions on the
SCSI disk without issues.
I also want to enable Active Negation and perhaps also "Back-to-Back
Transfer Enable" in CONFIG4 for this chip (see also the data manual
http://www.bitsavers.org/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf
if you are interested, at home I have an even newer one for the
Symbios Logic version).

I will prepare a patch. Oh, I also noticed some issues in the comments
in the driver, I'll see if I can fix those too.

Kind regards,

Kars.

Re: ESP SCSI driver

[Michael Schmitz]

Hi Kars,

On 29/10/19 10:37 PM, Kars de Jong wrote:
> Hi Michael,
>
> Op ma 28 okt. 2019 om 19:32 schreef Michael Schmitz <schmitzmic@gmail.com>:
>> Don't mess with CONFIG3 - we've tried all that, and in the end had to
>> use PIO to transfer message bytes. Problems with tagged queueing in
>> the ESP driver are usually all due to the DMA not transferring single
>> bytes on Amiga. The additional message byte required to see a reselect
>> for a tagged command complete remains stuck in the ESP fifo. Can't see
>> how this would be affected by the driver mis-identifying the chip as
>> FAS100A.
> And yet that was exactly what caused the issue. It was setting bit 1
> of CONFIG3 - ESP_CONFIG3_FAST (FAS100A) instead of bit 3 -
> ESP_CONFIG3_FCLK (FAS236 and also FSC). Bit 1 means ESP_CONFIG3_ADMA

I'd expect chip config settings to go awry if the chip version is not 
detected correctly. What I meant to say, attempts at somehow enabling 
additonal SCSI features in CONFIG3 haven't got us anywhere with the 
reselection interrupt issue.

Your hang may be due to something entirely different yet.

> on the FSC, and having bit 0 cleared and bit 1 set is actually an
> illegal combination on the FSC (marked "Reserved" in the data manual).
>
>> Regardig mis-detection of the chip revision - can you print the
>> 'version' and 'family_code' variables in the esp->rev == FAST branch
>> of esp_reset_esp() please?
> Family code 0x14, version 2. That's what was also used in the old driver.
Family code 0x14 would imply version 0xa2 (or possibly 0xa0). That's 
revision 'fsc' in the old driver?
>
>>> I will try out some things this evening, I naively added the FSC to
>>> the end of the enum, but the code does things like "if (esp->rev >=
>>> FAS100A)" which break horribly then.
>>>
>>> It looks like it might work if I add my chip type between FAS236 and
>>> FAS100A in the esp_rev enum.
> And that indeed fixed it. I ran "fsck -n" on both partitions on the
> SCSI disk without issues.
> I also want to enable Active Negation and perhaps also "Back-to-Back
> Transfer Enable" in CONFIG4 for this chip (see also the data manual
> http://www.bitsavers.org/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf
> if you are interested, at home I have an even newer one for the
> Symbios Logic version).
>
> I will prepare a patch. Oh, I also noticed some issues in the comments
> in the driver, I'll see if I can fix those too.

By all means, go ahead with a patch. My knowledge of the Amiga ESP 
boards is limited to what I could glean from the old driver source. I 
haven't seen an Amiga computer in over a decade.

Cheers,

     Michael


>
> Kind regards,
>
> Kars.

[PATCH] esp_scsi: Add support for FSC chip

[Kars de Jong]

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.

The FSC also has a feature called Active Negation which should always be
enabled according to the data manual. This is done using the CONFIG4
register.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c | 10 ++++++++--
 drivers/scsi/esp_scsi.h | 10 ++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

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);
+		} 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;
@@ -2373,6 +2378,7 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"FSC",
 	"FAS100A",
 	"FAST",
 	"FASHME",
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   */
 
@@ -262,10 +263,11 @@ enum esp_rev {
 	ESP100A    = 0x01,  /* NCR53C90A */
 	ESP236     = 0x02,
 	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	FSC        = 0x04,  /* NCR/Symbios Logic FSC */
+	FAS100A    = 0x05,
+	FAST       = 0x06,
+	FASHME     = 0x07,
+	PCSCSI     = 0x08,  /* AM53c974 */
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

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

[Michael Schmitz]

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.

FWIW, you are welcome to add my Reviewed-by tag (though it won't count 
for much - Finn's may carry more weight).

Cheers,

     Michael


On 30/10/19 11:05 AM, 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.
>
> The FSC also has a feature called Active Negation which should always be
> enabled according to the data manual. This is done using the CONFIG4
> register.
>
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>   drivers/scsi/esp_scsi.c | 10 ++++++++--
>   drivers/scsi/esp_scsi.h | 10 ++++++----
>   2 files changed, 14 insertions(+), 6 deletions(-)
>
> 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.

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).

> @@ -2373,6 +2378,7 @@ static const char *esp_chip_names[] = {
>   	"ESP100A",
>   	"ESP236",
>   	"FAS236",
> +	"FSC",
>   	"FAS100A",
>   	"FAST",
>   	"FASHME",
> 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.
>   
> @@ -262,10 +263,11 @@ enum esp_rev {
>   	ESP100A    = 0x01,  /* NCR53C90A */
>   	ESP236     = 0x02,
>   	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> +	FAS100A    = 0x05,
> +	FAST       = 0x06,
> +	FASHME     = 0x07,
> +	PCSCSI     = 0x08,  /* AM53c974 */
>   };
>   
>   struct esp_cmd_entry {

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

[Kars de Jong]

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.

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

[Michael Schmitz]

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

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

[Finn Thain]

Nice fix!

As Michael mentioned already, you'll need to send this to all the relevant 
recipients:

$ scripts/get_maintainer.pl add-support-for-fsc-chip.patch
"James E.J. Bottomley" <jejb@linux.ibm.com> (maintainer:SCSI SUBSYSTEM)
"Martin K. Petersen" <martin.petersen@oracle.com> (maintainer:SCSI SUBSYSTEM)
linux-scsi@vger.kernel.org (open list:SCSI SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)

A few more suggestions follow.

On Tue, 29 Oct 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.
> 
> The FSC also has a feature called Active Negation which should always be
> enabled according to the data manual. This is done using the CONFIG4
> register.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>  drivers/scsi/esp_scsi.c | 10 ++++++++--
>  drivers/scsi/esp_scsi.h | 10 ++++++----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> 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);

There is a comment in esp_scsi.h which seems to contradict the above 
logic, "ESP config register 4 read-write, found only on am53c974 chips."
I think the comment should be corrected now.

> +		} 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;
> @@ -2373,6 +2378,7 @@ static const char *esp_chip_names[] = {
>  	"ESP100A",
>  	"ESP236",
>  	"FAS236",
> +	"FSC",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> 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   */
>  

I think these should be in numerical order.

> @@ -262,10 +263,11 @@ enum esp_rev {
>  	ESP100A    = 0x01,  /* NCR53C90A */
>  	ESP236     = 0x02,
>  	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> +	FAS100A    = 0x05,
> +	FAST       = 0x06,
> +	FASHME     = 0x07,
> +	PCSCSI     = 0x08,  /* AM53c974 */

I'm guessing that you've placed it here because of the esp->rev >= FAS236 
tests that appear in a couple of places relating to sync transfer period. 
Might be worth mentioning that in the commit log.

(No idea why this list uses hexadecimal, it's not a value from a chip 
register. It would be more readable in decimal, IMHO.)

-- 

>  };
>  
>  struct esp_cmd_entry {
> 

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

[Michael Schmitz]

Hi Finn,

On 30/10/19 1:31 PM, Finn Thain wrote:
> Nice fix!
>
> As Michael mentioned already, you'll need to send this to all the relevant
> recipients:
>
> $ scripts/get_maintainer.pl add-support-for-fsc-chip.patch
> "James E.J. Bottomley" <jejb@linux.ibm.com> (maintainer:SCSI SUBSYSTEM)
> "Martin K. Petersen" <martin.petersen@oracle.com> (maintainer:SCSI SUBSYSTEM)
> linux-scsi@vger.kernel.org (open list:SCSI SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
>
> A few more suggestions follow.
>
> On Tue, 29 Oct 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.
>>
>> The FSC also has a feature called Active Negation which should always be
>> enabled according to the data manual. This is done using the CONFIG4
>> register.
>>
>> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
>> ---
>>   drivers/scsi/esp_scsi.c | 10 ++++++++--
>>   drivers/scsi/esp_scsi.h | 10 ++++++----
>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> 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);
> There is a comment in esp_scsi.h which seems to contradict the above
> logic, "ESP config register 4 read-write, found only on am53c974 chips."
> I think the comment should be corrected now.
>
>> +		} 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;
>> @@ -2373,6 +2378,7 @@ static const char *esp_chip_names[] = {
>>   	"ESP100A",
>>   	"ESP236",
>>   	"FAS236",
>> +	"FSC",
>>   	"FAS100A",
>>   	"FAST",
>>   	"FASHME",
>> 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   */
>>   
> I think these should be in numerical order.
The latter two are bitmasks, so the order makes sense to me.
>
>> @@ -262,10 +263,11 @@ enum esp_rev {
>>   	ESP100A    = 0x01,  /* NCR53C90A */
>>   	ESP236     = 0x02,
>>   	FAS236     = 0x03,
>> -	FAS100A    = 0x04,
>> -	FAST       = 0x05,
>> -	FASHME     = 0x06,
>> -	PCSCSI     = 0x07,  /* AM53c974 */
>> +	FSC        = 0x04,  /* NCR/Symbios Logic FSC */
>> +	FAS100A    = 0x05,
>> +	FAST       = 0x06,
>> +	FASHME     = 0x07,
>> +	PCSCSI     = 0x08,  /* AM53c974 */
> I'm guessing that you've placed it here because of the esp->rev >= FAS236
> tests that appear in a couple of places relating to sync transfer period.
> Might be worth mentioning that in the commit log.

Good catch. I think these tests would need to be changed to esp->rev 
 >=FSC to reflect the fact that the FSC has the same CONFIG3 settings as 
the FAS236.

Cheers,

     Michael

>
> (No idea why this list uses hexadecimal, it's not a value from a chip
> register. It would be more readable in decimal, IMHO.)
>

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

[Kars de Jong]

Hi Michael!

Op wo 30 okt. 2019 om 02:06 schreef Michael Schmitz <schmitzmic@gmail.com>:
> >> @@ -262,10 +263,11 @@ enum esp_rev {
> >>      ESP100A    = 0x01,  /* NCR53C90A */
> >>      ESP236     = 0x02,
> >>      FAS236     = 0x03,
> >> -    FAS100A    = 0x04,
> >> -    FAST       = 0x05,
> >> -    FASHME     = 0x06,
> >> -    PCSCSI     = 0x07,  /* AM53c974 */
> >> +    FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> >> +    FAS100A    = 0x05,
> >> +    FAST       = 0x06,
> >> +    FASHME     = 0x07,
> >> +    PCSCSI     = 0x08,  /* AM53c974 */
> > I'm guessing that you've placed it here because of the esp->rev >= FAS236
> > tests that appear in a couple of places relating to sync transfer period.
> > Might be worth mentioning that in the commit log.
>
> Good catch. I think these tests would need to be changed to esp->rev
>  >=FSC to reflect the fact that the FSC has the same CONFIG3 settings as
> the FAS236.

Um, no, that would break the FAS236. FSC is defined after FAS236. It's
import that its values is lower than FAS100A and greater or equal than
FAS236. And as I wrote in reply to Finn, I think PCSCSI falls in the
same category.

Kind regards,

Kars.

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

[Geert Uytterhoeven]

Hi Kars,

Thanks for your patch!

Initially I wondere how the Apple MESH SCSI (also 53cf94 based) on
my CHRP Longtrail used to work with this, until I realized my Longtrail
died a few years before the new ESP SCSI driver was written ;-)

On Wed, Oct 30, 2019 at 8:26 AM Kars de Jong <jongk@linux-m68k.org> wrote:
> Op wo 30 okt. 2019 om 02:06 schreef Michael Schmitz <schmitzmic@gmail.com>:
> > >> @@ -262,10 +263,11 @@ enum esp_rev {
> > >>      ESP100A    = 0x01,  /* NCR53C90A */
> > >>      ESP236     = 0x02,
> > >>      FAS236     = 0x03,
> > >> -    FAS100A    = 0x04,
> > >> -    FAST       = 0x05,
> > >> -    FASHME     = 0x06,
> > >> -    PCSCSI     = 0x07,  /* AM53c974 */
> > >> +    FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> > >> +    FAS100A    = 0x05,
> > >> +    FAST       = 0x06,
> > >> +    FASHME     = 0x07,
> > >> +    PCSCSI     = 0x08,  /* AM53c974 */

Is anything caring about the actual values?
Doesn't look like that, so you can just drop the values, and let standard C
enum handling assign the numbers. That way you don't have to renumber
when adding a new part.

> > > I'm guessing that you've placed it here because of the esp->rev >= FAS236
> > > tests that appear in a couple of places relating to sync transfer period.
> > > Might be worth mentioning that in the commit log.
> >
> > Good catch. I think these tests would need to be changed to esp->rev
> >  >=FSC to reflect the fact that the FSC has the same CONFIG3 settings as
> > the FAS236.
>
> Um, no, that would break the FAS236. FSC is defined after FAS236. It's
> import that its values is lower than FAS100A and greater or equal than
> FAS236. And as I wrote in reply to Finn, I think PCSCSI falls in the
> same category.

You definitely want to add a comment like "all below use the same CONFIG3
settings", to avoid nasty surprises for future additions (if any).
Using feature bits might be even better, but may not be worthwhile, since
there seems to be only a single "esp->rev > ..." check.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Kars de Jong]

Hi Geert!

Op wo 30 okt. 2019 om 09:45 schreef Geert Uytterhoeven <geert@linux-m68k.org>:
>
> Hi Kars,
>
> Thanks for your patch!
>
> Initially I wondere how the Apple MESH SCSI (also 53cf94 based) on
> my CHRP Longtrail used to work with this, until I realized my Longtrail
> died a few years before the new ESP SCSI driver was written ;-)

Yes, it's been a while :-)

> On Wed, Oct 30, 2019 at 8:26 AM Kars de Jong <jongk@linux-m68k.org> wrote:
> > Op wo 30 okt. 2019 om 02:06 schreef Michael Schmitz <schmitzmic@gmail.com>:
> > > >> @@ -262,10 +263,11 @@ enum esp_rev {
> > > >>      ESP100A    = 0x01,  /* NCR53C90A */
> > > >>      ESP236     = 0x02,
> > > >>      FAS236     = 0x03,
> > > >> -    FAS100A    = 0x04,
> > > >> -    FAST       = 0x05,
> > > >> -    FASHME     = 0x06,
> > > >> -    PCSCSI     = 0x07,  /* AM53c974 */
> > > >> +    FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> > > >> +    FAS100A    = 0x05,
> > > >> +    FAST       = 0x06,
> > > >> +    FASHME     = 0x07,
> > > >> +    PCSCSI     = 0x08,  /* AM53c974 */
>
> Is anything caring about the actual values?
> Doesn't look like that, so you can just drop the values, and let standard C
> enum handling assign the numbers. That way you don't have to renumber
> when adding a new part.

Nice one. That's indeed even better.

> > Um, no, that would break the FAS236. FSC is defined after FAS236. It's
> > import that its values is lower than FAS100A and greater or equal than
> > FAS236. And as I wrote in reply to Finn, I think PCSCSI falls in the
> > same category.
>
> You definitely want to add a comment like "all below use the same CONFIG3
> settings", to avoid nasty surprises for future additions (if any).
> Using feature bits might be even better, but may not be worthwhile, since
> there seems to be only a single "esp->rev > ..." check.

No, there are several more actually:
 * "esp->rev > ESP100A" which basically means "HAS_CONFIG3".
 * "esp->rev < ESP236" which basically means "!HAS_FAST_CLOCK".
 * "esp->rev >= FAS236" which basically means "HAS_FAST_CLOCK"
 * "esp->rev >= FAS100A" which basically means "HAS_OLD_CONFIG3"

So, perhaps having feature bits is not a bad idea at all...

Kind regards,

Kars.

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

[Michael Schmitz]

Hi Kars,

On Wed, Oct 30, 2019 at 10:08 PM Kars de Jong <jongk@linux-m68k.org> wrote:
> > > Um, no, that would break the FAS236. FSC is defined after FAS236. It's
> > > import that its values is lower than FAS100A and greater or equal than
> > > FAS236. And as I wrote in reply to Finn, I think PCSCSI falls in the
> > > same category.

You're right -

> > You definitely want to add a comment like "all below use the same CONFIG3
> > settings", to avoid nasty surprises for future additions (if any).
> > Using feature bits might be even better, but may not be worthwhile, since
> > there seems to be only a single "esp->rev > ..." check.
>
> No, there are several more actually:
>  * "esp->rev > ESP100A" which basically means "HAS_CONFIG3".
>  * "esp->rev < ESP236" which basically means "!HAS_FAST_CLOCK".
>  * "esp->rev >= FAS236" which basically means "HAS_FAST_CLOCK"
>  * "esp->rev >= FAS100A" which basically means "HAS_OLD_CONFIG3"
>
> So, perhaps having feature bits is not a bad idea at all...

We need a few more feature bits (at least HAS_CONFIG2 to differentiate
between ESP100 and ESP100A in esp_get_revision()). This might get hard
in terms of test coverage, but I'd raise that as alternative to adding
FSC to the list or enum in your RFC to linux-scsi.

Cheers,

    Michael
>
> Kind regards,
>
> Kars.

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

[Brad Boyer]

On Wed, Oct 30, 2019 at 09:45:05AM +0100, Geert Uytterhoeven wrote:
> Initially I wondere how the Apple MESH SCSI (also 53cf94 based) on
> my CHRP Longtrail used to work with this, until I realized my Longtrail
> died a few years before the new ESP SCSI driver was written ;-)

I was under the impression that MESH was not ESP based. It always had
(and still has) a completely separate driver. At least on my PowerMac
7600, there were two separate SCSI controllers for internal and
external devices. The external was run by mac53c94 (which still exists
in the source and doesn't use the shared ESP code) while the internal was
run by the mesh driver.

	Brad Boyer
	brad@allandria.com

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

[Kars de Jong]

Hi Finn!

Op wo 30 okt. 2019 om 01:31 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Tue, 29 Oct 2019, Kars de Jong 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);
>
> There is a comment in esp_scsi.h which seems to contradict the above
> logic, "ESP config register 4 read-write, found only on am53c974 chips."
> I think the comment should be corrected now.

Nice catch! I'll change the comment.

> > 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   */
> >

> I think these should be in numerical order.

They're bit masks, but I'll add some white space to separate them and
rearrange it so the values come after the family bit mask.

> > @@ -262,10 +263,11 @@ enum esp_rev {
> >       ESP100A    = 0x01,  /* NCR53C90A */
> >       ESP236     = 0x02,
> >       FAS236     = 0x03,
> > -     FAS100A    = 0x04,
> > -     FAST       = 0x05,
> > -     FASHME     = 0x06,
> > -     PCSCSI     = 0x07,  /* AM53c974 */
> > +     FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> > +     FAS100A    = 0x05,
> > +     FAST       = 0x06,
> > +     FASHME     = 0x07,
> > +     PCSCSI     = 0x08,  /* AM53c974 */
>
> I'm guessing that you've placed it here because of the esp->rev >= FAS236
> tests that appear in a couple of places relating to sync transfer period.
> Might be worth mentioning that in the commit log.

That, and also the checks in the driver that check for esp->rev >=
FAS100A. I think I'll add a comment to the enum so that others don't
have the same issue that I first had.

Actually, I think the PCSCSI value is also at the wrong position in
the list, its CONFIG3 (called CNTLREG3 in the data manual
http://bitsavers.trailing-edge.com/components/amd/_dataSheets/1993_53c974_PCscsi.pdf)
has pretty much the same layout as the FAS236 and FSC, so it would
probably have the same issue as me at high synchronous speeds...

Kind regards,

Kars.

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

[Finn Thain]

On Wed, 30 Oct 2019, Kars de Jong wrote:

> 
> Actually, I think the PCSCSI value is also at the wrong position in the 
> list, its CONFIG3 (called CNTLREG3 in the data manual 
> http://bitsavers.trailing-edge.com/components/amd/_dataSheets/1993_53c974_PCscsi.pdf) 
> has pretty much the same layout as the FAS236 and FSC, so it would 
> probably have the same issue as me at high synchronous speeds...
> 

The PCSCSI driver (drivers/scsi/am53c974.c) is maintained by its author, 
Hannes Reinecke. He would need to review changes to the core driver 
relating to this chip.

-- 

> Kind regards,
> 
> Kars.
> 

[PATCH 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 22 +++++++++++++--------
 drivers/scsi/esp_scsi.h | 44 ++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
enum explaining this.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..b96cbda03d2d 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,18 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

On Tue, 12 Nov 2019, Kars de Jong wrote:

> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.
> 
> Add a comment to the enum explaining this.
> 
> Also, the actual values for the enum fields are irrelevant, so remove the
> explicit values (suggested by Geert Uytterhoeven). This makes adding a new
> field in the middle of the enum easier.
> 
> Finally, move the PCSCSI definition to the right place in the enum. In its
> previous location, at the end of the enum, the wrong values are written to
> the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
> enum explaining this.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>  drivers/scsi/esp_scsi.c |  2 +-
>  drivers/scsi/esp_scsi.h | 19 +++++++++++--------
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..4fc3eee3138b 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
>  	"ESP100A",
>  	"ESP236",
>  	"FAS236",
> +	"AM53C974",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> -	"AM53C974",
>  };
>  
>  static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 91b32f2a1a1b..b96cbda03d2d 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -257,15 +257,18 @@ struct esp_cmd_priv {
>  };
>  #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
>  
> +/* NOTE: this enum is ordered based on chip features! */

Fair enough, that has been overlooked before.

>  enum esp_rev {
> -	ESP100     = 0x00,  /* NCR53C90 - very broken */
> -	ESP100A    = 0x01,  /* NCR53C90A */
> -	ESP236     = 0x02,
> -	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	ESP100,  /* NCR53C90 - very broken */
> +	ESP100A, /* NCR53C90A */
> +	ESP236,
> +	/* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
> +	FAS236,
> +	PCSCSI,  /* AM53c974 */
> +	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
> +	FAS100A,
> +	FAST,
> +	FASHME,
>  };
>  
>  struct esp_cmd_entry {
> 

FAS100A, FAST and FASHME are below both lines, which is a bit confusing.

In general, I don't like to see comments that merely re-state the explicit 
logic in the algorithm. Such comments add no value.

(At best this redundancy creates a maintenance burden and at worst the 
commentary becomes neglected until it creates contradictions.)

Aside from that:
Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

-- 

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Finn,

Thanks for your review!

Op wo 13 nov. 2019 om 00:07 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Tue, 12 Nov 2019, Kars de Jong wrote:
> > Finally, move the PCSCSI definition to the right place in the enum. In its
> > previous location, at the end of the enum, the wrong values are written to
> > the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
> > enum explaining this.
> >
> >  enum esp_rev {
> > -     ESP100     = 0x00,  /* NCR53C90 - very broken */
> > -     ESP100A    = 0x01,  /* NCR53C90A */
> > -     ESP236     = 0x02,
> > -     FAS236     = 0x03,
> > -     FAS100A    = 0x04,
> > -     FAST       = 0x05,
> > -     FASHME     = 0x06,
> > -     PCSCSI     = 0x07,  /* AM53c974 */
> > +     ESP100,  /* NCR53C90 - very broken */
> > +     ESP100A, /* NCR53C90A */
> > +     ESP236,
> > +     /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
> > +     FAS236,
> > +     PCSCSI,  /* AM53c974 */
> > +     /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
> > +     FAS100A,
> > +     FAST,
> > +     FASHME,
> >  };
> >
> >  struct esp_cmd_entry {
> >
>
> FAS100A, FAST and FASHME are below both lines, which is a bit confusing.

Hmm, you're right. But I don't really know how to solve that. But if
you think the initial comment is enough to trigger people to
investigate the algorithm, I can remove them.

> In general, I don't like to see comments that merely re-state the explicit
> logic in the algorithm. Such comments add no value.
>
> (At best this redundancy creates a maintenance burden and at worst the
> commentary becomes neglected until it creates contradictions.)

Unfortunately the algorithm isn't very obvious here (well, not to me at least).

Kind regards,

Kars.

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

On Wed, 13 Nov 2019, Kars de Jong wrote:

> >
> > FAS100A, FAST and FASHME are below both lines, which is a bit 
> > confusing.
> 
> Hmm, you're right. But I don't really know how to solve that. But if you 
> think the initial comment is enough to trigger people to investigate the 
> algorithm, I can remove them.
> 

Yes, please. The initial "NOTE!" that you added is sufficient, IMHO.

-- 

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Christoph Hellwig]

On Tue, Nov 12, 2019 at 07:57:09PM +0100, Kars de Jong wrote:
> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.

Yikes.  Wouldn't it make much more sense to have a feature bitmap?

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Christoph!

Op wo 13 nov. 2019 om 15:22 schreef Christoph Hellwig <hch@infradead.org>:
>
> On Tue, Nov 12, 2019 at 07:57:09PM +0100, Kars de Jong wrote:
> > The order of the definitions in the esp_rev enum is important. The values
> > are used in comparisons for chip features.
>
> Yikes.  Wouldn't it make much more sense to have a feature bitmap?

Yes, I mentioned that in my cover letter already, and it was discussed
a bit on the Linux/m68k mailing list.
 It may be hard to get that tested though, most of the users are
legacy architectures with a probably very limited number of users.
But if a mostly review-based process is good enough, I am certainly
willing to change it.

Kind regards,

Kars.

[PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

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);
+		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,
-- 
2.17.1

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

[Finn Thain]

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?

-- 

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

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

[Finn Thain]

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.

-- 

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

[Kars de Jong]

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.

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

[Finn Thain]

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.

-- 

> Kind regards,
> 
> Kars.
> 

[PATCH v2 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Changes since v1:
- Removed confusing comments from esp_rev enum
- Remove unneeded definitions for UID register
- Remove unneeded local uid variable

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 21 +++++++++++--------
 drivers/scsi/esp_scsi.h | 45 ++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..f764d64e1f25 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,16 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

Re: [PATCH 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Oops, sorry, forgot to add v2 to the subject.

Op do 14 nov. 2019 om 23:00 schreef Kars de Jong <jongk@linux-m68k.org>:
...

[PATCH 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

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 | 19 ++++++++++++-------
 drivers/scsi/esp_scsi.h | 28 ++++++++++++++++++++--------
 2 files changed, 32 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;
 
 	/* 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 */
 		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..60de32d17d6a 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,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,11 @@ enum esp_rev {
 	ESP236,
 	FAS236,
 	PCSCSI,  /* AM53c974 */
+<<<<<<< HEAD
+=======
+	FSC,     /* NCR/Symbios Logic FSC */
+	/* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
+>>>>>>> fb55f6c862d7... esp_scsi: Add support for FSC chip
 	FAS100A,
 	FAST,
 	FASHME,
-- 
2.17.1

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

[Kars de Jong]

Bah, ignore this... it still has a merge conflict.

Sorry.

Op do 14 nov. 2019 om 23:00 schreef Kars de Jong <jongk@linux-m68k.org>:

[PATCH v3 0/2] Some esp_scsi updates

[Kars de Jong]

When trying kernel 5.4.0-rc5 on my Amiga, I experienced data transfer failures
when reading from my FAST-10 SCSI disk. I have a Blizzard 12x0 IV SCSI
controller which uses a Symbios Logic SYM53CF94-2 "FSC" chip.

This used to work with the old generic NCR53C9x driver, so I investigated the
issue.

It turned out to be caused by lacking detection of FSC silicon in the new
driver.

The second patch in this series adds support for the FSC.

When adding support for the chip, I found out the hard way that the esp_rev
enum is ordered (I just added it to the end of the enum).

Then I also discovered that the definition for the PCSCSI chip was in the wrong
place of the enum. It will probably have issues with FAST-10 SCSI targets,
because its CONFIG3 settings are wrong.

The first patch fixes this, and adds comments to the enum to hopefully prevent
this from happening again.

When discussing this on the Linux/m68k mailing list, it was suggested to
perhaps replace the dependency on ordering of the esp_rev enum by feature flags.
I did not implement this for now.

Changes since v1:
- Removed confusing comments from esp_rev enum
- Remove unneeded definitions for UID register
- Remove unneeded local uid variable

Changes since v2:
- Fixed merge conflict in second patch

Kars de Jong (2):
  esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum
  esp_scsi: Add support for FSC chip

 drivers/scsi/esp_scsi.c | 21 +++++++++++++--------
 drivers/scsi/esp_scsi.h | 41 +++++++++++++++++++++++++----------------
 2 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

The order of the definitions in the esp_rev enum is important. The values
are used in comparisons for chip features.

Add a comment to the enum explaining this.

Also, the actual values for the enum fields are irrelevant, so remove the
explicit values (suggested by Geert Uytterhoeven). This makes adding a new
field in the middle of the enum easier.

Finally, move the PCSCSI definition to the right place in the enum. In its
previous location, at the end of the enum, the wrong values are written to
the CONFIG3 register when used with FAST-SCSI targets.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/esp_scsi.c |  2 +-
 drivers/scsi/esp_scsi.h | 17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bb88995a12c7..4fc3eee3138b 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
 	"ESP100A",
 	"ESP236",
 	"FAS236",
+	"AM53C974",
 	"FAS100A",
 	"FAST",
 	"FASHME",
-	"AM53C974",
 };
 
 static struct scsi_transport_template *esp_transport_template;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 91b32f2a1a1b..f764d64e1f25 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -257,15 +257,16 @@ struct esp_cmd_priv {
 };
 #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
 
+/* NOTE: this enum is ordered based on chip features! */
 enum esp_rev {
-	ESP100     = 0x00,  /* NCR53C90 - very broken */
-	ESP100A    = 0x01,  /* NCR53C90A */
-	ESP236     = 0x02,
-	FAS236     = 0x03,
-	FAS100A    = 0x04,
-	FAST       = 0x05,
-	FASHME     = 0x06,
-	PCSCSI     = 0x07,  /* AM53c974 */
+	ESP100,  /* NCR53C90 - very broken */
+	ESP100A, /* NCR53C90A */
+	ESP236,
+	FAS236,
+	PCSCSI,  /* AM53c974 */
+	FAS100A,
+	FAST,
+	FASHME,
 };
 
 struct esp_cmd_entry {
-- 
2.17.1

Re: [PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Finn Thain]

For simplicity, the entire patch series would normally show the same 
version number (v3). Also, the series would normally be a thread unto 
itself, rather than a sub-thread.

On Thu, 14 Nov 2019, Kars de Jong wrote:

> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.
> 
> Add a comment to the enum explaining this.
> 
> Also, the actual values for the enum fields are irrelevant, so remove the
> explicit values (suggested by Geert Uytterhoeven). This makes adding a new
> field in the middle of the enum easier.
> 
> Finally, move the PCSCSI definition to the right place in the enum. In its
> previous location, at the end of the enum, the wrong values are written to
> the CONFIG3 register when used with FAST-SCSI targets.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>

This is Hannes' code so I'll leave it up to him to review this change.

I presume this is untested on PCSCSI hardware. ISTR that there's an 
emulator for that board somewhere...

-- 

> ---
>  drivers/scsi/esp_scsi.c |  2 +-
>  drivers/scsi/esp_scsi.h | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..4fc3eee3138b 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
>  	"ESP100A",
>  	"ESP236",
>  	"FAS236",
> +	"AM53C974",
>  	"FAS100A",
>  	"FAST",
>  	"FASHME",
> -	"AM53C974",
>  };
>  
>  static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 91b32f2a1a1b..f764d64e1f25 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -257,15 +257,16 @@ struct esp_cmd_priv {
>  };
>  #define ESP_CMD_PRIV(CMD)	((struct esp_cmd_priv *)(&(CMD)->SCp))
>  
> +/* NOTE: this enum is ordered based on chip features! */
>  enum esp_rev {
> -	ESP100     = 0x00,  /* NCR53C90 - very broken */
> -	ESP100A    = 0x01,  /* NCR53C90A */
> -	ESP236     = 0x02,
> -	FAS236     = 0x03,
> -	FAS100A    = 0x04,
> -	FAST       = 0x05,
> -	FASHME     = 0x06,
> -	PCSCSI     = 0x07,  /* AM53c974 */
> +	ESP100,  /* NCR53C90 - very broken */
> +	ESP100A, /* NCR53C90A */
> +	ESP236,
> +	FAS236,
> +	PCSCSI,  /* AM53c974 */
> +	FAS100A,
> +	FAST,
> +	FASHME,
>  };
>  
>  struct esp_cmd_entry {
> 

Re: [PATCH v2 1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum

[Kars de Jong]

Hi Finn!

Op vr 15 nov. 2019 om 03:13 schreef Finn Thain <fthain@telegraphics.com.au>:
> For simplicity, the entire patch series would normally show the same
> version number (v3).

OK, thanks.

> Also, the series would normally be a thread unto itself, rather than a sub-thread.

OK, I followed an example from the git-send-email manual ("which
avoids breaking threads to provide a new patch series").
I found the relevant convention in "Submitting patches", I didn't
notice that before. Thanks.

> ...
>
> This is Hannes' code so I'll leave it up to him to review this change.
>
> I presume this is untested on PCSCSI hardware. ISTR that there's an
> emulator for that board somewhere...

Yes, I don't have any, so I could not test it. QEMU emulates it, but
it doesn't care about CONFIG3 etc. at all.

Thanks for your reviews!

Kind regards,

Kars.

[PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

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 | 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;
 
 	/* 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 */
 		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)   */
 
 #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 */
 	FAS100A,
 	FAST,
 	FASHME,
-- 
2.17.1

Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Finn Thain]

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

Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong]

Hi Finn,

Thanks again for the review!

Op vr 15 nov. 2019 om 03:09 schreef Finn Thain <fthain@telegraphics.com.au>:
> 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>
>
> 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.

Good point, I will move it to inside the  "if (esp->rev) == FAST) {" block.

> > -             /* 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.

Yes, I agree. I'll remove the comment.

>
> 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).

I'll also remove that comment.

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

No, the respective data manuals all name these variants as "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).

Agreed, I'll add the part code instead.

Kind regards,

Kars.

[PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

When using this driver on a Blizzard 1260, there were failures whenever
DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a
DMA transfer of 1 byte. This caused the byte at offset 65535 to be
overwritten with 0xff. The Blizzard hardware can't handle single byte DMA
transfers.

Besides this issue, limiting the DMA length to something that is not a
multiple of the page size is very inefficient on most file systems.

It seems this limit was chosen because the DMA transfer counter of the ESP
by default is 16 bits wide, thus limiting the length to 65535 bytes.
However, the value 0 means 65536 bytes, which is handled by the ESP and the
Blizzard just fine. It is also the default maximum used by esp_scsi when
drivers don't provide their own dma_length_limit() function.

Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/zorro_esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index ca8e3abeb2c7..4448567c495d 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
 					u32 dma_len)
 {
-	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
+	return dma_len > (1U << 16) ? (1U << 16) : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
-- 
2.17.1

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[James Bottomley]

On Sat, 2019-11-09 at 20:14 +0100, Kars de Jong wrote:
> When using this driver on a Blizzard 1260, there were failures
> whenever DMA transfers from the SCSI bus to memory of 65535 bytes
> were followed by a DMA transfer of 1 byte. This caused the byte at
> offset 65535 to be overwritten with 0xff. The Blizzard hardware can't
> handle single byte DMA transfers.
> 
> Besides this issue, limiting the DMA length to something that is not
> a multiple of the page size is very inefficient on most file systems.
> 
> It seems this limit was chosen because the DMA transfer counter of
> the ESP by default is 16 bits wide, thus limiting the length to 65535
> bytes. However, the value 0 means 65536 bytes, which is handled by
> the ESP and the Blizzard just fine. It is also the default maximum
> used by esp_scsi when drivers don't provide their own
> dma_length_limit() function.

Have you tested this on any other hardware?  the reason most legacy
hardware would have a setting like this is because they have a two byte
transfer length register and zero doesn't mean 65536.  If this is the
case for any of the cards the zorro_esp drives, it might be better to
lower the max length to 61440 (64k-4k) so the residual is a page.

James

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Michael Schmitz]

James,

Am 10.11.2019 um 09:12 schrieb James Bottomley:
> On Sat, 2019-11-09 at 20:14 +0100, Kars de Jong wrote:
>> When using this driver on a Blizzard 1260, there were failures
>> whenever DMA transfers from the SCSI bus to memory of 65535 bytes
>> were followed by a DMA transfer of 1 byte. This caused the byte at
>> offset 65535 to be overwritten with 0xff. The Blizzard hardware can't
>> handle single byte DMA transfers.
>>
>> Besides this issue, limiting the DMA length to something that is not
>> a multiple of the page size is very inefficient on most file systems.
>>
>> It seems this limit was chosen because the DMA transfer counter of
>> the ESP by default is 16 bits wide, thus limiting the length to 65535
>> bytes. However, the value 0 means 65536 bytes, which is handled by
>> the ESP and the Blizzard just fine. It is also the default maximum
>> used by esp_scsi when drivers don't provide their own
>> dma_length_limit() function.
>
> Have you tested this on any other hardware?  the reason most legacy
> hardware would have a setting like this is because they have a two byte
> transfer length register and zero doesn't mean 65536.  If this is the

The data book for the NCR53FC94/NCR53FC96 (the 'fast' SCSI controller 
used on the board Kars tried) states that with the features enable bit 
clear (no 24 bit DMA counts used), zero does mean 64k indeed. The 
features enable bit is never set by esp_scsi.c. I chose the incorrect 
length limit without realizing this special case for the transfer count. 
and before we found out that 1-byte DMA just won't work.

I need to confirm this from a data book of the older (pre-fast) 
revisions of this chip family. but since as Kars also states, the core 
driver default for the 16 bit transfer size is 64k as well, I very much 
suspect the same behaviour for the older revisions.

All of the old board-specific drivers used a max transfer length of 
0x1000000, only the fastlane driver used 0xfffc. That lower limit might 
be due to a DMA limitation on the fastlane board. We could accommodate 
the different limit for this board by using a board-specific 
dma_length_limit() callback...

> case for any of the cards the zorro_esp drives, it might be better to
> lower the max length to 61440 (64k-4k) so the residual is a page.

For the benefit of keeping the code simple, and avoid retesting the 
fastlane board, that might indeed be the better solution.

Cheers,

	Michael

>
> James
>

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

Hi Michael,

Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>:
> All of the old board-specific drivers used a max transfer length of
> 0x1000000, only the fastlane driver used 0xfffc.

Yes, I also found this when checking the old drivers.

> That lower limit might
> be due to a DMA limitation on the fastlane board. We could accommodate
> the different limit for this board by using a board-specific
> dma_length_limit() callback...

Yes, I think that's the best idea for now. Oktagon also used to have a
different limit but that was never ported to the new ESP core.

> > case for any of the cards the zorro_esp drives, it might be better to
> > lower the max length to 61440 (64k-4k) so the residual is a page.
>
> For the benefit of keeping the code simple, and avoid retesting the
> fastlane board, that might indeed be the better solution.

But it's slower... :-P

Also, I may be adding another board-specific version for the Blizzard
12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in
a later patch.

Kind regards,

Kars.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Michael Schmitz]

Hi Kars,

thanks for your patch!

On 10/11/19 10:01 PM, Kars de Jong wrote:
> Hi Michael,
>
> Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>:
>> All of the old board-specific drivers used a max transfer length of
>> 0x1000000, only the fastlane driver used 0xfffc.
> Yes, I also found this when checking the old drivers.
>
>> That lower limit might
>> be due to a DMA limitation on the fastlane board. We could accommodate
>> the different limit for this board by using a board-specific
>> dma_length_limit() callback...
> Yes, I think that's the best idea for now. Oktagon also used to have a
> different limit but that was never ported to the new ESP core.

I can't remember the details, but as far as I recall it, the Oktagon 
used pseudo-DMA rather than hardware DMA. At the time I started porting 
Zorro ESP drivers to the new core, pseudo-DMA code was available for Mac 
only, and no PIO transfer for data phases at all, so I decided to leave 
that out altogether.

Might be a lot easier now that Finn has moved the PIO support code into 
the core driver. Someone could start with a PIO mode driver and add PDMA 
later.

>>> case for any of the cards the zorro_esp drives, it might be better to
>>> lower the max length to 61440 (64k-4k) so the residual is a page.
>> For the benefit of keeping the code simple, and avoid retesting the
>> fastlane board, that might indeed be the better solution.
> But it's slower... :-P
I wonder what max. transfer size had been used so far, in the majority 
of cases. I hadn't observed this bug in my tests of the ESP driver on 
elgar. So it might not matter so much in practice.
> Also, I may be adding another board-specific version for the Blizzard
> 12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in
> a later patch.

If we can differentiate between the Mark IV board and the Mark II board 
in a reliable way, fine. I can't remember whether I've had a report on 
that ever.

I'd suggest to change the transfer size limit to 60k in the first 
instance, and add board-specific tweaks as needed when you add 24 bit 
DMA support for the Mark IV.

Cheers,

     Michael

>
> Kind regards,
>
> Kars.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

Hi Michael,

Op zo 10 nov. 2019 om 20:26 schreef Michael Schmitz <schmitzmic@gmail.com>:
> >>> case for any of the cards the zorro_esp drives, it might be better to
> >>> lower the max length to 61440 (64k-4k) so the residual is a page.
> >> For the benefit of keeping the code simple, and avoid retesting the
> >> fastlane board, that might indeed be the better solution.
> >
> > But it's slower... :-P
> >
> I wonder what max. transfer size had been used so far, in the majority
> of cases. I hadn't observed this bug in my tests of the ESP driver on
> elgar. So it might not matter so much in practice.

Does Elgar indeed have a Blizzard 2060 as
https://wiki.debian.org/M68k/Autobuilder says?
If it does, it does surprise me that it works, since the DMA engine
appears to be very much like the one
of the Blizzard 1230 (including the >> 1 of the address).
Even when just loading bash on my system, there were many
65535-and-then-1 byte transfers.
It may of course depend on how fragmented your disk is.

> > Also, I may be adding another board-specific version for the Blizzard
> > 12x0 IV to enable 24-bit transfers, like the am53c974 driver does, in
> > a later patch.
>
> If we can differentiate between the Mark IV board and the Mark II board
> in a reliable way, fine. I can't remember whether I've had a report on
> that ever.

They have a different Zorro ID, so that should not be a problem.
By the way, do you remember why you chose to not use the full Zorro
IDs for the various SCSI boards asdefined in zorro_ids.h but only
the manufacturer defines?


Kind regards,

Kars.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[James Bottomley]

On Sun, 2019-11-10 at 15:36 +1300, Michael Schmitz wrote:
> All of the old board-specific drivers used a max transfer length of 
> 0x1000000, only the fastlane driver used 0xfffc. That lower limit
> might be due to a DMA limitation on the fastlane board. We could
> accommodate  the different limit for this board by using a board-
> specific  dma_length_limit() callback...

Sure, that implies the minimum dma burst length is 4 bytes ...

> > case for any of the cards the zorro_esp drives, it might be better
> > to lower the max length to 61440 (64k-4k) so the residual is a
> > page.
> 
> For the benefit of keeping the code simple, and avoid retesting the 
> fastlane board, that might indeed be the better solution.

... which means you don't have to lower the limit by 4k as I suggested,
 because I was being incredibly conservative about what the dma burst
length might be, just use 0xfffc as the default.  I think raising to
0x1000000 for boards which can support it is fine too if you want to
add the extra logic to the driver.

James

[PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Kars de Jong]

When using this driver on a Blizzard 1260, there were failures whenever
DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a
DMA transfer of 1 byte. This caused the byte at offset 65535 to be
overwritten with 0xff. The Blizzard hardware can't handle single byte DMA
transfers.

Besides this issue, limiting the DMA length to something that is not a
multiple of the page size is very inefficient on most file systems.

It seems this limit was chosen because the DMA transfer counter of the ESP
by default is 16 bits wide, thus limiting the length to 65535 bytes.
However, the value 0 means 65536 bytes, which is handled by the ESP and the
Blizzard just fine. It is also the default maximum used by esp_scsi when
drivers don't provide their own dma_length_limit() function.

The limit of 65536 bytes can be used by all boards except the Fastlane. The
old driver used a limit of 65532 bytes (0xfffc), which is reintroduced in
this patch.

Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes")
Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
---
 drivers/scsi/zorro_esp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index ca8e3abeb2c7..a23a8e5794f5 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -218,7 +218,14 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
 					u32 dma_len)
 {
-	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
+	return dma_len > (1U << 16) ? (1U << 16) : dma_len;
+}
+
+static u32 fastlane_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
+					u32 dma_len)
+{
+	/* The old driver used 0xfffc as limit, so do that here too */
+	return dma_len > 0xfffc ? 0xfffc : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -604,7 +611,7 @@ static const struct esp_driver_ops fastlane_esp_ops = {
 	.esp_write8		= zorro_esp_write8,
 	.esp_read8		= zorro_esp_read8,
 	.irq_pending		= fastlane_esp_irq_pending,
-	.dma_length_limit	= zorro_esp_dma_length_limit,
+	.dma_length_limit	= fastlane_esp_dma_length_limit,
 	.reset_dma		= zorro_esp_reset_dma,
 	.dma_drain		= zorro_esp_dma_drain,
 	.dma_invalidate		= fastlane_esp_dma_invalidate,
-- 
2.17.1

Re: [PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Finn Thain]

On Tue, 12 Nov 2019, Kars de Jong wrote:

> When using this driver on a Blizzard 1260, there were failures whenever
> DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a
> DMA transfer of 1 byte. This caused the byte at offset 65535 to be
> overwritten with 0xff. The Blizzard hardware can't handle single byte DMA
> transfers.
> 
> Besides this issue, limiting the DMA length to something that is not a
> multiple of the page size is very inefficient on most file systems.
> 
> It seems this limit was chosen because the DMA transfer counter of the ESP
> by default is 16 bits wide, thus limiting the length to 65535 bytes.
> However, the value 0 means 65536 bytes, which is handled by the ESP and the
> Blizzard just fine. It is also the default maximum used by esp_scsi when
> drivers don't provide their own dma_length_limit() function.
> 
> The limit of 65536 bytes can be used by all boards except the Fastlane. The
> old driver used a limit of 65532 bytes (0xfffc), which is reintroduced in
> this patch.
> 
> Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes")
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>

Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

> ---
>  drivers/scsi/zorro_esp.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> index ca8e3abeb2c7..a23a8e5794f5 100644
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -218,7 +218,14 @@ static int fastlane_esp_irq_pending(struct esp *esp)
>  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
>  					u32 dma_len)
>  {
> -	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
> +	return dma_len > (1U << 16) ? (1U << 16) : dma_len;
> +}
> +
> +static u32 fastlane_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
> +					u32 dma_len)
> +{
> +	/* The old driver used 0xfffc as limit, so do that here too */
> +	return dma_len > 0xfffc ? 0xfffc : dma_len;
>  }
>  
>  static void zorro_esp_reset_dma(struct esp *esp)
> @@ -604,7 +611,7 @@ static const struct esp_driver_ops fastlane_esp_ops = {
>  	.esp_write8		= zorro_esp_write8,
>  	.esp_read8		= zorro_esp_read8,
>  	.irq_pending		= fastlane_esp_irq_pending,
> -	.dma_length_limit	= zorro_esp_dma_length_limit,
> +	.dma_length_limit	= fastlane_esp_dma_length_limit,
>  	.reset_dma		= zorro_esp_reset_dma,
>  	.dma_drain		= zorro_esp_dma_drain,
>  	.dma_invalidate		= fastlane_esp_dma_invalidate,
> 

Re: [PATCH v2] zorro_esp: Limit DMA transfers to 65536 bytes (except on Fastlane)

[Martin K. Petersen]

Kars,

> When using this driver on a Blizzard 1260, there were failures
> whenever DMA transfers from the SCSI bus to memory of 65535 bytes were
> followed by a DMA transfer of 1 byte. This caused the byte at offset
> 65535 to be overwritten with 0xff. The Blizzard hardware can't handle
> single byte DMA transfers.

Applied to 5.5/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

Hi Michael,

Op zo 10 nov. 2019 om 03:36 schreef Michael Schmitz <schmitzmic@gmail.com>:
> I need to confirm this from a data book of the older (pre-fast)
> revisions of this chip family. but since as Kars also states, the core
> driver default for the 16 bit transfer size is 64k as well, I very much
> suspect the same behaviour for the older revisions.

According to ftp://bitsavers.informatik.uni-stuttgart.de/components/ncr/scsi/NCR53C90ab.pdf,
on the NCR 53C90A programming the transfer counter to 0 also means
64k. That's the oldest data book I could find.
But the Zorro driver assumes a fast variant of the chip anyway (the
clock frequency is hard coded to 40 MHz), so this point is a little
moot.

Kind regards,

Kars.

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Finn Thain]

On Sat, 9 Nov 2019, Kars de Jong wrote:

> When using this driver on a Blizzard 1260, there were failures whenever
> DMA transfers from the SCSI bus to memory of 65535 bytes were followed by a
> DMA transfer of 1 byte. This caused the byte at offset 65535 to be
> overwritten with 0xff. The Blizzard hardware can't handle single byte DMA
> transfers.
> 
> Besides this issue, limiting the DMA length to something that is not a
> multiple of the page size is very inefficient on most file systems.
> 

Makes sense. You may want to add,
Fixes: b7ded0e8b0d1 ("scsi: zorro_esp: Limit DMA transfers to 65535 bytes")

> It seems this limit was chosen because the DMA transfer counter of the ESP
> by default is 16 bits wide, thus limiting the length to 65535 bytes.
> However, the value 0 means 65536 bytes, which is handled by the ESP and the
> Blizzard just fine. It is also the default maximum used by esp_scsi when
> drivers don't provide their own dma_length_limit() function.
> 
> Signed-off-by: Kars de Jong <jongk@linux-m68k.org>
> ---
>  drivers/scsi/zorro_esp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> index ca8e3abeb2c7..4448567c495d 100644
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
>  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
>  					u32 dma_len)
>  {
> -	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
> +	return dma_len > (1U << 16) ? (1U << 16) : dma_len;
>  }
>  

Would it be safer to simply remove this code and leave 
esp_driver_ops.dma_length_limit == NULL for all board types?

-- 

>  static void zorro_esp_reset_dma(struct esp *esp)
> 

Re: [PATCH] zorro_esp: increase maximum dma length to 65536 bytes

[Kars de Jong]

Hi Finn,

Op za 9 nov. 2019 om 23:53 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Sat, 9 Nov 2019, Kars de Jong wrote:
> > diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> > index ca8e3abeb2c7..4448567c495d 100644
> > --- a/drivers/scsi/zorro_esp.c
> > +++ b/drivers/scsi/zorro_esp.c
> > @@ -218,7 +218,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
> >  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
> >                                       u32 dma_len)
> >  {
> > -     return dma_len > 0xFFFF ? 0xFFFF : dma_len;
> > +     return dma_len > (1U << 16) ? (1U << 16) : dma_len;
> >  }
> >
>
> Would it be safer to simply remove this code and leave
> esp_driver_ops.dma_length_limit == NULL for all board types?

I have considered that, but that version also imposes unneeded limits
on crossing a 24-bit address boundary. The Zorro boards don't seem to
need this.

Kind regards,

Kars.

Re: ESP SCSI driver (was: Amiga PCMCIA network card support)

[Finn Thain]

On Mon, 28 Oct 2019, Kars de Jong wrote:

> > Michael actually tested everything on the Amiga 4000.
> 
> It probably uses a different chip. The text and images show it's a 
> QLogic FAS216 (which is the same as a FAS236, FAS236 is the differential 
> variant). The type should more accurately be presented as "FAS2x6" since 
> you can't tell which type it is in software.
> 

FWIW, the old driver, removed in commit a2c6ef71364e ("[SCSI] NCR53C9x: 
remove driver", has this:

-               if(family_code == 0x02) {
-                       if ((version & 7) == 2)
-                               esp->erev = fas216;     
-                        else
-                               esp->erev = fas236;

-- 

Re: ESP SCSI driver (was: Amiga PCMCIA network card support)

[Kars de Jong]

Hi Finn,

Op di 29 okt. 2019 om 00:38 schreef Finn Thain <fthain@telegraphics.com.au>:
> On Mon, 28 Oct 2019, Kars de Jong wrote:
> > It probably uses a different chip. The text and images show it's a
> > QLogic FAS216 (which is the same as a FAS236, FAS236 is the differential
> > variant). The type should more accurately be presented as "FAS2x6" since
> > you can't tell which type it is in software.
> >
>
> FWIW, the old driver, removed in commit a2c6ef71364e ("[SCSI] NCR53C9x:
> remove driver", has this:
>
> -               if(family_code == 0x02) {
> -                       if ((version & 7) == 2)
> -                               esp->erev = fas216;
> -                        else
> -                               esp->erev = fas236;

The fas236 type was from the original Sun-only esp driver (I guess the
only Suns with a FAS2x6 had differential variants hence the name). I
got a confirmation for the detection of the FAS216 from someone with a
Cyberstorm board, so that one was added then (including the specific
match on version). Since the NCR53C9x driver was never used on a Sun,
this was never confirmed for the FAS236.

Kind regards,

Kars.

Re: ESP SCSI driver

[Michael Schmitz]

Hi Kars,

On 30/10/19 12:52 AM, Kars de Jong wrote:
> Hi Finn,
>
> Op di 29 okt. 2019 om 00:38 schreef Finn Thain <fthain@telegraphics.com.au>:
>> On Mon, 28 Oct 2019, Kars de Jong wrote:
>>> It probably uses a different chip. The text and images show it's a
>>> QLogic FAS216 (which is the same as a FAS236, FAS236 is the differential
>>> variant). The type should more accurately be presented as "FAS2x6" since
>>> you can't tell which type it is in software.
>>>
>> FWIW, the old driver, removed in commit a2c6ef71364e ("[SCSI] NCR53C9x:
>> remove driver", has this:
>>
>> -               if(family_code == 0x02) {
>> -                       if ((version & 7) == 2)
>> -                               esp->erev = fas216;
>> -                        else
>> -                               esp->erev = fas236;
> The fas236 type was from the original Sun-only esp driver (I guess the
> only Suns with a FAS2x6 had differential variants hence the name). I
> got a confirmation for the detection of the FAS216 from someone with a
> Cyberstorm board, so that one was added then (including the specific
> match on version). Since the NCR53C9x driver was never used on a Sun,
> this was never confirmed for the FAS236.

Do we need to distinguish between fas216 and fas236 at all? From the old 
driver code, they would seem to behave just the same.

Making use of the distinction between fsc and fas236 for enabling active 
negation would not require figuring out the SCSI bus mode (differential 
or single-ended) from my reading of the data book. Enabling back to back 
transfers might requrire more work on the driver.

Cheers,

     Michael


>
> Kind regards,
>
> Kars.

Re: Amiga PCMCIA network card support

[Finn Thain]

On Mon, 28 Oct 2019, Kars de Jong wrote:

> 
> For starters. it does not identify my chip correctly, it says it's a 
> FAS100A (it's actually a Symbios Logic 53CF94-2 which is more like a 
> FAS236). But after I fixed that it still doesn't work.
> 

Can you send the log? I have a script to decode the output from 
esp_dump_cmd_log() which might help here.

> One difference between the new driver and the old one is its support for 
> tagged commands. I haven't found an easy way to disable that yet though.
> 

I pushed some debug patches to my github that might help. 
https://github.com/fthain/linux/commits/esp-debug

There's a patch that adds the option to disable TCQ 
(esp_scsi.esp_use_tcq=0), and one to disable disconnect/reselect etc.

There's also a patch to disable DMA in the zorro_esp driver (requires the 
parameter esp_scsi.esp_disable_sync=1) that might help with debugging the 
FAS236 esp_rev problem.

-- 

FOSDEM (was: Re: Amiga PCMCIA network card support)

[Geert Uytterhoeven]

Hi all,

On Mon, Oct 28, 2019 at 9:27 PM Kars de Jong <jongk@linux-m68k.org> wrote:
> Op ma 28 okt. 2019 om 12:08 schreef John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de>:
> > But I will be attending FOSDEM and Geert is coming as well (at least he
> > has been to every FOSDEM so far ;)), so we can maybe have a small m68k

Yes, I plan to be there.

> > meeting. I'm coming with two friends who are also Linux hackers.
>
> Would be cool... Last time I went to something like that was the
> Oldenburg meeting of 1998 ;-)

Time to broaden your playfield again ;-)

Note that there will be a retro-computing devroom, for which a talk about
e.g. not letting die Linux on m68k may be a suitable topic.

https://fosdem.org/2020/news/2019-10-01-accepted-developer-rooms/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: FOSDEM (was: Re: Amiga PCMCIA network card support)

[John Paul Adrian Glaubitz]

On 10/29/19 9:56 AM, Geert Uytterhoeven wrote:
>> Would be cool... Last time I went to something like that was the
>> Oldenburg meeting of 1998 ;-)
> 
> Time to broaden your playfield again ;-)
> 
> Note that there will be a retro-computing devroom, for which a talk about
> e.g. not letting die Linux on m68k may be a suitable topic.
> 
> https://fosdem.org/2020/news/2019-10-01-accepted-developer-rooms/

Nice. We could also point to the Bountysource campaign for gcc there:

> https://www.bountysource.com/issues/80706251-m68k-convert-the-backend-to-mode_cc-so-it-can-be-kept-in-future-releases

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: Amiga PCMCIA network card support

[Finn Thain]

On Mon, 28 Oct 2019, Kars de Jong wrote:

> Also, someone once tested this on his A1200 which had a 68030 and I 
> believe it broke. Something about ioremap() not accepting physical 
> addresses that were already mapped by a fixed mapping (Zorro II address 
> space).
> 

Interesting! On my to-do list is a task to investigate the reason why 
commit bb6ef7d1e8a6 ("m68k/mac: Don't remap SWIM MMIO region") was needed. 
I think you may have put your finger on that reason...

-- 

Re: Amiga PCMCIA network card support

[Geert Uytterhoeven]

 Hi Kars,

On Mon, Oct 28, 2019 at 8:02 PM Kars de Jong <jongk@linux-m68k.org> wrote:
> Op vr 25 okt. 2019 om 13:49 schreef Andreas 'count' Kotes
> <count-vger.kernel.org+linux-m68k@flatline.de>:
> > On 25.10.19 09:25, Kars de Jong wrote:
> > > Op do 24 okt. 2019 om 22:56 schreef Andreas 'count' Kotes
> > > <count-vger.kernel.org+linux-
> > >> What needs to happen for stable, reliable support for existing PCMCIA
> > >> network cards people already have? How can we unblock those possible
> > >> Linux users?
> > > ...
> > > Then Real Life happened and I didn't touch my Amiga for 10+ years...
> >
> > .. hmm, is this something that's likely to change? ;)
>
> I've dusted it off and powered it on this weekend. It still works (RTC
> needs a new battery though), and the 2.6.18 kernel that I modified
> many years ago still works fine with the pcnet_cs driver and my
> Dynalink L10C card.

You may want to have a close look at the electrolytic capacitors.
A few months ago I had to repair some of the damage done by
a leaking capacitor.  Fortunately I don't need the signals on the
parallel port, but I do use the serial console, and thus had to fix
the eaten RXD trace.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Amiga PCMCIA network card support

[John Paul Adrian Glaubitz]

On 10/29/19 10:00 AM, Geert Uytterhoeven wrote:
>> I've dusted it off and powered it on this weekend. It still works (RTC
>> needs a new battery though), and the 2.6.18 kernel that I modified
>> many years ago still works fine with the pcnet_cs driver and my
>> Dynalink L10C card.
> 
> You may want to have a close look at the electrolytic capacitors.
> A few months ago I had to repair some of the damage done by
> a leaking capacitor.  Fortunately I don't need the signals on the
> parallel port, but I do use the serial console, and thus had to fix
> the eaten RXD trace.

In the worst case, you can just buy a new A1200 board off eBay for 70 Euros:

> https://www.ebay.de/itm/123936094940

Just involves some soldering but people are building new A1200s successfully:

> https://www.a1k.org/forum/index.php?threads/70854/

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: Amiga PCMCIA network card support

[Geert Uytterhoeven]

Hi Adrian,

On Tue, Oct 29, 2019 at 10:12 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On 10/29/19 10:00 AM, Geert Uytterhoeven wrote:
> >> I've dusted it off and powered it on this weekend. It still works (RTC
> >> needs a new battery though), and the 2.6.18 kernel that I modified
> >> many years ago still works fine with the pcnet_cs driver and my
> >> Dynalink L10C card.
> >
> > You may want to have a close look at the electrolytic capacitors.
> > A few months ago I had to repair some of the damage done by
> > a leaking capacitor.  Fortunately I don't need the signals on the
> > parallel port, but I do use the serial console, and thus had to fix
> > the eaten RXD trace.
>
> In the worst case, you can just buy a new A1200 board off eBay for 70 Euros:

Mine is an A4000 ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Amiga PCMCIA network card support

[John Paul Adrian Glaubitz]

Hi!

On 10/29/19 10:14 AM, Geert Uytterhoeven wrote:
> On Tue, Oct 29, 2019 at 10:12 AM John Paul Adrian Glaubitz
> <glaubitz@physik.fu-berlin.de> wrote:
>> On 10/29/19 10:00 AM, Geert Uytterhoeven wrote:
>>>> I've dusted it off and powered it on this weekend. It still works (RTC
>>>> needs a new battery though), and the 2.6.18 kernel that I modified
>>>> many years ago still works fine with the pcnet_cs driver and my
>>>> Dynalink L10C card.
>>>
>>> You may want to have a close look at the electrolytic capacitors.
>>> A few months ago I had to repair some of the damage done by
>>> a leaking capacitor.  Fortunately I don't need the signals on the
>>> parallel port, but I do use the serial console, and thus had to fix
>>> the eaten RXD trace.
>>
>> In the worst case, you can just buy a new A1200 board off eBay for 70 Euros:
> 
> Mine is an A4000 ;-)

Not sure if anyone re-made the A4000 board yet, but you can buy a new A3000 board
already, including AGA:

> http://www.amibay.com/showthread.php?106543-AA3000-Amiga-3000-AGA-motherboard

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: Amiga PCMCIA network card support

[Kars de Jong]

Hi Geert!

Op di 29 okt. 2019 om 10:00 schreef Geert Uytterhoeven <geert@linux-m68k.org>:
>
>  Hi Kars,
>
> On Mon, Oct 28, 2019 at 8:02 PM Kars de Jong <jongk@linux-m68k.org> wrote:
> > I've dusted it off and powered it on this weekend. It still works (RTC
> > needs a new battery though), and the 2.6.18 kernel that I modified
> > many years ago still works fine with the pcnet_cs driver and my
> > Dynalink L10C card.
>
> You may want to have a close look at the electrolytic capacitors.
> A few months ago I had to repair some of the damage done by
> a leaking capacitor.  Fortunately I don't need the signals on the
> parallel port, but I do use the serial console, and thus had to fix
> the eaten RXD trace.

Hmm, good one. I'll check them out.

Thanks,

Kars.

Re: Amiga PCMCIA network card support

[Michael Schmitz]

Kars,

Am 25.10.2019 um 20:25 schrieb Kars de Jong:

> 2) Rework <asm/io.h> so inb()/inw()/inl(), outb()/outw()/outl(),
> insb()/insw()/insl() and outsb()/outsw()/outsl() "just work" for
> PCMCIA address space without having to create ugly hacks in the
> generic drivers. This is not trivial because of the strange memory
> mapping of the Amiga PCMCIA slot.

If you can come up with a way to achieve that (somehow selecting between 
8 bit and 16 bit mode at run time), I'd love to hear about it. I've 
experimented a bit with the ne.c driver to allow for run time selection 
of the correct IO access functions, but never came up with something 
that I'd expect to get past the netdev folks.

> 3) ...
> 4) Profit!
>
> Oh, the 3c589_cs driver needed an ugly hack because some of the
> registers just didn't read out properly on the Amiga with 16-bit
> access. That issue is also described in the source code for the
> AmigaOS driver.

Precisely, and this is the main reason why we can currently compile the 
driver for either 8 bit or 16 bit access, but not both.

@Andreas: I've sent a rough draft of a isp1763 driver to Adrian Glaubitz 
and Michael Karcher a few months ago. Doesn't do anything useful yet, 
and needs someone debugging where it goes wrong carefully.
That was based on 4.18, but shouldn't be hard to get ported to 5.x. I 
can resend that if there's any interest.

Cheers,

	Michael