Re: [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Michael,

On Fri, Jun 18, 2021 at 10:06 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
Am 18.06.2021 um 19:16 schrieb Geert Uytterhoeven:
+#ifdef CONFIG_APNE100MBIT
+    if (apne_100_mbit)
+            isa_type = ISA_TYPE_AG16;
+#endif
+
I think isa_type has to be assigned unconditionally otherwise it can't be
reset for 10 mbit cards. Therefore, the AMIGAHW_PRESENT(PCMCIA) logic in
arch/m68k/kernel/setup_mm.c probably should move here.

Good catch! I am uncertain though as to whether replacing a 100 Mbit
card by a 10 Mbit one at run time is a common use case (or even
possible, given constraints of the Amiga PCMCIA interface?), but it
ought to work even if rarely used.

Given it's PCMCIA, I guess that's a possibility.
Furthermore, always setting isa_type means the user can recover from
a mistake by unloading the module, and modprobe'ing again with the
correct parameter.
For the builtin-case, that needs a s/0444/0644/ change, though.

How does re-probing after a card change for a builtin driver work?
Changing the permission bits is a minor issue.

Oh right, this driver predates the driver framework, and doesn't support
PCMCIA hotplug.  So auto-unregister on removal doesn't work.
Even using unbind/bind in sysfs won't work.

So rmmod/modprobe is the only thing that has a chance to work...

The comment there says isa_type must be set as early as possible, so I'd
rather leave that alone, and add an 'else' clause here.

This of course raise the question whether we ought to move the entire
isa_type handling into arch code instead - make it a generic
amiga_pcmcia_16bit option settable via sysfs. There may be other 16 bit
cards that require the same treatment, and duplicating PCMCIA mode
switching all over the place could be avoided. Opinions?

Indeed.

The only downside I can see is that setting isa_type needs to be done
ahead of modprobe, through sysfs. That might be a little error prone.

Still, can we autodetect in the driver?

Guess we'll have to find out how the 16 bit cards behave if first poked
in 8 bit mode, attempting to force a reset of the 8390 chip, and
switching to 16 bit mode if this fails. That's normally done in
apne_probe1() which runs after init_pcmcia(), so we can't rely on the
result of a 8390 reset autoprobe to do the PCMCIA software reset there.

The 8390 reset part does not rely on anything else in apne_probe1(), so
that code can be lifted out of apne_probe1() and run early in
apne_probe() (after the check for an inserted PCMCIA card). I'll try and
prepare a patch for Alex to test that method.

I'm wondering how this is handled on PCs with PCMCIA, or if there
really is something special about Amiga PCMCIA hardware...

What's special about Amiga PCMCIA hardware is that the card reset isn't
connected for those 16 bit cards, so pcmcia_reset() does not work.

I was mostly thinking about the difference between 8-bit and 16-bit
accesses.

Whether the software reset workaround hurts for 8 bit cards is something
I don't know and cannot test. But

And I'd really like to get rid of the CONFIG_APNE100MBIT option,
i.e. always include the support, if possible.

I can't see why that wouldn't be possible - the only downside is that we
force MULTI_ISA=1 always for Amiga, and lose the optimizations done for
MUTLI_ISA=0 in io_mm.h. Unless we autoprobe, we can use isa_type to
guard against running a software reset on 8 bit cards ...

The latter sounds like a neat trick...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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



[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux