From: Michael Schmitz <schmitzmic@gmail.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Finn Thain <fthain@linux-m68k.org>,
Linux/m68k <linux-m68k@vger.kernel.org>,
ALeX Kazik <alex@kazik.de>, netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver
Date: Fri, 18 Jun 2021 20:06:19 +1200 [thread overview]
Message-ID: <da54e915-c142-a69b-757f-6a6419f173fa@gmail.com> (raw)
In-Reply-To: <CAMuHMdVGe1EutOVpw3-R=25xG0p2rWd65cB2mqM-imXWYjLtXw@mail.gmail.com>
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@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
>
next prev parent reply other threads:[~2021-06-18 8:06 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 5:28 [PATCH v3 0/2] Add APNE PCMCIA 100 Mbit support Michael Schmitz
2021-06-17 5:28 ` [PATCH v3 1/2] m68k: io_mm.h - add APNE 100 MBit support Michael Schmitz
2021-06-17 6:58 ` Finn Thain
2021-06-17 21:42 ` Michael Schmitz
2021-06-17 5:28 ` [PATCH net-next v3 2/2] net/8390: apne.c - add 100 Mbit support to apne.c driver Michael Schmitz
2021-06-17 6:51 ` Finn Thain
2021-06-17 19:33 ` Michael Schmitz
2021-06-18 7:16 ` Geert Uytterhoeven
2021-06-18 8:06 ` Michael Schmitz [this message]
2021-06-18 8:13 ` Geert Uytterhoeven
2021-06-18 8:28 ` Michael Schmitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=da54e915-c142-a69b-757f-6a6419f173fa@gmail.com \
--to=schmitzmic@gmail.com \
--cc=alex@kazik.de \
--cc=fthain@linux-m68k.org \
--cc=geert@linux-m68k.org \
--cc=linux-m68k@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.