Date:   Fri, 18 Jun 2021 20:06:19 +1200
From:   Michael Schmitz <schmitzmic@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Finn Thain <fthain@...ux-m68k.org>,
        Linux/m68k <linux-m68k@...r.kernel.org>,
        ALeX Kazik <alex@...ik.de>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support
 to apne.c driver

Hi Geert,

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.

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

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

Cheers,

	Michael

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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
>

Powered by blists - more mailing lists