[PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Niklas Schnelle]

Hi Geert,

This is a follow up in my ongoing effort of making inb()/outb() and
similar I/O port accessors compile-time optional. Previously I sent this
as a treewide series titled "treewide: Remove I/O port accessors for
HAS_IOPORT=n" with the latest being its 5th version[0]. With a significant
subset of patches merged I've changed over to per-subsystem series. These
series are stand alone and should be merged via the relevant tree such
that with all subsystems complete we can follow this up with the final
patch that will make the I/O port accessors compile-time optional.

The current state of the full series with changes to the remaining
subsystems and the aforementioned final patch can be found for your
convenience on my git.kernel.org tree in the has_ioport_v6 branch[1] with
signed tags. As for compile-time vs runtime see Linus' reply to my first
attempt[2].

Thanks,
Niklas

[0] https://lore.kernel.org/all/20230522105049.1467313-1-schnelle@linux.ibm.com/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=has_ioport_v6
[2] https://lore.kernel.org/lkml/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/

Niklas Schnelle (1):
  m68k: Let GENERIC_IOMAP depend on HAS_IOPORT

 arch/m68k/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.40.1

[PATCH 1/1] m68k: Let GENERIC_IOMAP depend on HAS_IOPORT

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
compile time. With that choosing dynamically between I/O port and MMIO
access via GNERIC_IOMAP will not work. So only select GENERIC_IOMAP when
HAS_IOPORT is selected.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 arch/m68k/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 6ffa29585194..6ef282f329ee 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -18,7 +18,7 @@ config M68K
 	select DMA_DIRECT_REMAP if M68K_NONCOHERENT_DMA && !COLDFIRE
 	select GENERIC_ATOMIC64
 	select GENERIC_CPU_DEVICES
-	select GENERIC_IOMAP
+	select GENERIC_IOMAP if HAS_IOPORT
 	select GENERIC_IRQ_SHOW
 	select GENERIC_LIB_ASHLDI3
 	select GENERIC_LIB_ASHRDI3
-- 
2.40.1

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Michael Schmitz]

Niklas,

how do you propose we handle legacy drivers that do depend on 
inb()/outb() functions (_not_ actual ISA I/O) on architectures that map 
inb()/outb() to MMIO functions?

(In my case, that's at least ne.c - Geert ought to have a better 
overview what else does use inb()/outb() on m68k)

Cheers,

     Michael

On 4/04/24 01:28, Niklas Schnelle wrote:
> Hi Geert,
>
> This is a follow up in my ongoing effort of making inb()/outb() and
> similar I/O port accessors compile-time optional. Previously I sent this
> as a treewide series titled "treewide: Remove I/O port accessors for
> HAS_IOPORT=n" with the latest being its 5th version[0]. With a significant
> subset of patches merged I've changed over to per-subsystem series. These
> series are stand alone and should be merged via the relevant tree such
> that with all subsystems complete we can follow this up with the final
> patch that will make the I/O port accessors compile-time optional.
>
> The current state of the full series with changes to the remaining
> subsystems and the aforementioned final patch can be found for your
> convenience on my git.kernel.org tree in the has_ioport_v6 branch[1] with
> signed tags. As for compile-time vs runtime see Linus' reply to my first
> attempt[2].
>
> Thanks,
> Niklas
>
> [0] https://lore.kernel.org/all/20230522105049.1467313-1-schnelle@linux.ibm.com/
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=has_ioport_v6
> [2] https://lore.kernel.org/lkml/CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com/
>
> Niklas Schnelle (1):
>    m68k: Let GENERIC_IOMAP depend on HAS_IOPORT
>
>   arch/m68k/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
> Niklas,
>
> how do you propose we handle legacy drivers that do depend on 
> inb()/outb() functions (_not_ actual ISA I/O) on architectures that map 
> inb()/outb() to MMIO functions?
>
> (In my case, that's at least ne.c - Geert ought to have a better 
> overview what else does use inb()/outb() on m68k)

If a machine provides an inb()/outb() set of operations that
is actually used, it should set HAS_IOPORT.

For the Q40, it may be better in the long run to change the
drivers to just use MMIO directly though.

    Arnd

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Geert Uytterhoeven]

Hi Arnd,

On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
> > how do you propose we handle legacy drivers that do depend on
> > inb()/outb() functions (_not_ actual ISA I/O) on architectures that map
> > inb()/outb() to MMIO functions?
> >
> > (In my case, that's at least ne.c - Geert ought to have a better
> > overview what else does use inb()/outb() on m68k)
>
> If a machine provides an inb()/outb() set of operations that
> is actually used, it should set HAS_IOPORT.
>
> For the Q40, it may be better in the long run to change the
> drivers to just use MMIO directly though.

Q40 uses ISA.

Michael is worried about non-ISA drivers using inb() and friends.
At some point in time (i.e. eons ago), we were told it was better to
use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
pointers...

Anyway, I don't think we have many users of inb() and friends left, and
I assume the bots should have detected any/most remaining users in Niklas'
branch...

arch/m68k/include/asm/floppy.h on Sun-3x might be the only offender?

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 0/1] m68k: Handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Fri, Apr 5, 2024, at 12:16, Geert Uytterhoeven wrote:
> On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
>>
>> For the Q40, it may be better in the long run to change the
>> drivers to just use MMIO directly though.
>
> Q40 uses ISA.

Ah, indeed. I got confused by  the NE2000 example as that
contains "depends on (ISA || (Q40 && m)", which would have
indicated that it's not actually using CONFIG_ISA.

> Michael is worried about non-ISA drivers using inb() and friends.
> At some point in time (i.e. eons ago), we were told it was better to
> use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
> pointers...

It's definitely still better to use an abstraction layer for MMIO
accesses using inline asm instructions than open-coding the
volatile pointer dereferences. Over time we have gotten better
at defining which of the available abstractions should be used
for a given bus, so inb()/outb() is now only really used for
things derived from ISA in some form, including e.g. PCI and LPC.

> Anyway, I don't think we have many users of inb() and friends left, and
> I assume the bots should have detected any/most remaining users in Niklas'
> branch...
>
> arch/m68k/include/asm/floppy.h on Sun-3x might be the only offender?

Could be. I think we can leave this one to whoever tries to get
sun3x floppy support working, it's been marked broken for a while
(see below). If there are any others, they will cause pretty
obvious build failures once inb()/outb() are removed from the
build, and they should be trivial to fix then.

     Arnd

commit f1e0f28a85001f4faa3ea930fcf201933f42340e
Author: akpm <akpm>
Date:   Mon Jan 19 18:31:30 2004 +0000

    [PATCH] M68k floppy selection
    
    From: Geert Uytterhoeven <geert@linux-m68k.org>
    
    Floppy: On m68k, PC-style floppies are used on Q40/Q60 and Sun-3x only. Sun-3x
    floppy is currently broken (needs I/O abstractions)
    
    BKrev: 400c2282G1O-TsH5FiwzPbOorftQhg

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 2dce1d2699a9..32fdec34568e 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -6,7 +6,7 @@ menu "Block devices"
 
 config BLK_DEV_FD
        tristate "Normal floppy disk support"
-       depends on !X86_PC9800 && !ARCH_S390
+       depends on (!X86_PC9800 && !ARCH_S390 && !M68K) || Q40 || (SUN3X && BROKEN)
        ---help---
          If you want to use the floppy disk drive(s) of your PC under Linux,
          say Y. Information about this driver, especially important for IBM

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Michael Schmitz]

Hi Geert,

Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
> Hi Arnd,
>
> On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
>>> how do you propose we handle legacy drivers that do depend on
>>> inb()/outb() functions (_not_ actual ISA I/O) on architectures that map
>>> inb()/outb() to MMIO functions?
>>>
>>> (In my case, that's at least ne.c - Geert ought to have a better
>>> overview what else does use inb()/outb() on m68k)
>>
>> If a machine provides an inb()/outb() set of operations that
>> is actually used, it should set HAS_IOPORT.
>>
>> For the Q40, it may be better in the long run to change the
>> drivers to just use MMIO directly though.
>
> Q40 uses ISA.
>
> Michael is worried about non-ISA drivers using inb() and friends.
> At some point in time (i.e. eons ago), we were told it was better to
> use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
> pointers...
>
> Anyway, I don't think we have many users of inb() and friends left, and
> I assume the bots should have detected any/most remaining users in Niklas'
> branch...

All the 8390 based ethernet drivers still use inb() and friends.

That is the main reason for the terrible hacks in 
arch/m68k/include/asm/io_mm.h ...

The last time I tried to add support for a different PCMCIA ethernet 
adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't 
getting anywhere with the netdev crowd. That was ages ago, and I doubt 
their enthusiasm for a rewrite of the 8390 code base to avoid using 
inb() on MMIO architectures will be any better now.

Just saying ...

	Michael

>
> arch/m68k/include/asm/floppy.h on Sun-3x might be the only offender?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Fri, Apr 5, 2024, at 20:36, Michael Schmitz wrote:
> Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
>> On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
>>> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
>>>> how do you propose we handle legacy drivers that do depend on
>>>> inb()/outb() functions (_not_ actual ISA I/O) on architectures that map
>>>> inb()/outb() to MMIO functions?
>>>>
>>>> (In my case, that's at least ne.c - Geert ought to have a better
>>>> overview what else does use inb()/outb() on m68k)
>>>
>>> If a machine provides an inb()/outb() set of operations that
>>> is actually used, it should set HAS_IOPORT.
>>>
>>> For the Q40, it may be better in the long run to change the
>>> drivers to just use MMIO directly though.
>>
>> Q40 uses ISA.
>>
>> Michael is worried about non-ISA drivers using inb() and friends.
>> At some point in time (i.e. eons ago), we were told it was better to
>> use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
>> pointers...
>>
>> Anyway, I don't think we have many users of inb() and friends left, and
>> I assume the bots should have detected any/most remaining users in Niklas'
>> branch...
>
> All the 8390 based ethernet drivers still use inb() and friends.
>
> That is the main reason for the terrible hacks in 
> arch/m68k/include/asm/io_mm.h ...
>
> The last time I tried to add support for a different PCMCIA ethernet 
> adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't 
> getting anywhere with the netdev crowd. That was ages ago, and I doubt 
> their enthusiasm for a rewrite of the 8390 code base to avoid using 
> inb() on MMIO architectures will be any better now.

From what I can see, there is already an abstraction layer in
these drivers that is used by all m68k drivers except apne:

$ git grep -w 'define\sei_inb'
drivers/net/ethernet/8390/8390.h:#define ei_inb(_p)     inb(_p)
drivers/net/ethernet/8390/8390p.c:#define ei_inb(_p)    inb(_p)
drivers/net/ethernet/8390/ax88796.c:#define ei_inb(_a) readb(ax_convert_addr(_a))
drivers/net/ethernet/8390/etherh.c:#define ei_inb(_p)    readb((void __iomem *)_p)
drivers/net/ethernet/8390/hydra.c:#define ei_inb(port)   in_8(port)
drivers/net/ethernet/8390/mac8390.c:#define ei_inb(port)        in_8(port)
drivers/net/ethernet/8390/mcf8390.c:#define     ei_inb  ei_inb
drivers/net/ethernet/8390/xsurf100.c:#define ei_inb(_a) z_readb(ax_convert_addr(_a))
drivers/net/ethernet/8390/zorro8390.c:#define ei_inb(port)              in_8(port)

Can't apne.c just do the same here? The patch below didn't
take that long to come up with, but I may be missing something
here of course.

    Arnd

8<---
From 5dd43e612a52adf499b1ea3d33e3b2b45894d275 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 5 Apr 2024 21:47:51 +0200
Subject: [PATCH] net: apne: convert to lib8390

The apne driver still uses the ISA-style inb()/outb() wappers through the
8390.c helper module, which won't work in the future.

Change it to include lib8390.c like all the other m68k variants of this
driver do, so it can have custom MMIO abstractions.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/net/ethernet/8390/Makefile b/drivers/net/ethernet/8390/Makefile
index 85c83c566ec6..ec1b325da4e4 100644
--- a/drivers/net/ethernet/8390/Makefile
+++ b/drivers/net/ethernet/8390/Makefile
@@ -4,7 +4,7 @@
 #
 
 obj-$(CONFIG_MAC8390) += mac8390.o
-obj-$(CONFIG_APNE) += apne.o 8390.o
+obj-$(CONFIG_APNE) += apne.o
 obj-$(CONFIG_ARM_ETHERH) += etherh.o
 obj-$(CONFIG_AX88796) += ax88796.o
 obj-$(CONFIG_HYDRA) += hydra.o
diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
index 828edca8d30c..ea3747723b3c 100644
--- a/drivers/net/ethernet/8390/apne.c
+++ b/drivers/net/ethernet/8390/apne.c
@@ -41,7 +41,15 @@
 #include <asm/amigayle.h>
 #include <asm/amipcmcia.h>
 
-#include "8390.h"
+#define ei_inb(port)            in_8(port)
+#define ei_outb(val, port)      out_8(port, val)
+#define ei_inb_p(port)          in_8(port)
+#define ei_outb_p(val, port)    out_8(port, val)
+
+static const char version[] =
+    "apne.c:v1.1 7/10/98 Alain Malek (Alain.Malek@cryogen.ch)\n";
+
+#include "lib8390.c"
 
 /* ---- No user-serviceable parts below ---- */
 
@@ -105,14 +113,21 @@ static int init_pcmcia(void);
 #define MANUAL_HWADDR5 0x9a
 */
 
-static const char version[] =
-    "apne.c:v1.1 7/10/98 Alain Malek (Alain.Malek@cryogen.ch)\n";
-
 static int apne_owned;	/* signal if card already owned */
 
-static u32 apne_msg_enable;
-module_param_named(msg_enable, apne_msg_enable, uint, 0444);
-MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
+static const struct net_device_ops apne_netdev_ops = {
+	.ndo_open		= __ei_open,
+	.ndo_stop		= __ei_close,
+	.ndo_start_xmit		= __ei_start_xmit,
+	.ndo_tx_timeout		= __ei_tx_timeout,
+	.ndo_get_stats		= __ei_get_stats,
+	.ndo_set_rx_mode	= __ei_set_multicast_list,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_set_mac_address	= eth_mac_addr,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller	= __ei_poll,
+#endif
+};
 
 static struct net_device * __init apne_probe(void)
 {
@@ -141,11 +156,11 @@ static struct net_device * __init apne_probe(void)
 		return ERR_PTR(-ENODEV);
 	}
 
-	dev = alloc_ei_netdev();
+	dev = ____alloc_ei_netdev(0);
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 	ei_local = netdev_priv(dev);
-	ei_local->msg_enable = apne_msg_enable;
+	ei_local->msg_enable = msg_enable;
 
 	/* disable pcmcia irq for readtuple */
 	pcmcia_disable_irq();
@@ -203,7 +218,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
 #endif
     static unsigned version_printed;
 
-    if ((apne_msg_enable & NETIF_MSG_DRV) && (version_printed++ == 0))
+    if ((msg_enable & NETIF_MSG_DRV) && (version_printed++ == 0))
 		netdev_info(dev, version);
 
     netdev_info(dev, "PCMCIA NE*000 ethercard probe");
@@ -309,7 +324,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
 
     dev->base_addr = ioaddr;
     dev->irq = IRQ_AMIGA_PORTS;
-    dev->netdev_ops = &ei_netdev_ops;
+    dev->netdev_ops = &apne_netdev_ops;
 
     /* Install the Interrupt handler */
     i = request_irq(dev->irq, apne_interrupt, IRQF_SHARED, DRV_NAME, dev);
@@ -333,7 +348,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
     ei_status.block_output = &apne_block_output;
     ei_status.get_8390_hdr = &apne_get_8390_hdr;
 
-    NS8390_init(dev, 0);
+    __NS8390_init(dev, 0);
 
     pcmcia_ack_int(pcmcia_get_intreq());		/* ack PCMCIA int req */
     pcmcia_enable_irq();
@@ -513,7 +528,7 @@ apne_block_output(struct net_device *dev, int count,
 	if (time_after(jiffies, dma_start + 2*HZ/100)) {	/* 20ms */
 		netdev_warn(dev, "timeout waiting for Tx RDC.\n");
 		apne_reset_8390(dev);
-		NS8390_init(dev,1);
+		__NS8390_init(dev,1);
 		break;
 	}
 
@@ -534,10 +549,10 @@ static irqreturn_t apne_interrupt(int irq, void *dev_id)
         pcmcia_ack_int(pcmcia_intreq);
         return IRQ_NONE;
     }
-    if (apne_msg_enable & NETIF_MSG_INTR)
+    if (msg_enable & NETIF_MSG_INTR)
 	pr_debug("pcmcia intreq = %x\n", pcmcia_intreq);
     pcmcia_disable_irq();			/* to get rid of the sti() within ei_interrupt */
-    ei_interrupt(irq, dev_id);
+    __ei_interrupt(irq, dev_id);
     pcmcia_ack_int(pcmcia_get_intreq());
     pcmcia_enable_irq();
     return IRQ_HANDLED;

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Michael Schmitz]

Hi Arnd,

thanks for your suggestions!

Am 06.04.2024 um 09:13 schrieb Arnd Bergmann:
> On Fri, Apr 5, 2024, at 20:36, Michael Schmitz wrote:
>> Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
>>> On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
>>>> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
>>>>> how do you propose we handle legacy drivers that do depend on
>>>>> inb()/outb() functions (_not_ actual ISA I/O) on architectures that map
>>>>> inb()/outb() to MMIO functions?
>>>>>
>>>>> (In my case, that's at least ne.c - Geert ought to have a better
>>>>> overview what else does use inb()/outb() on m68k)
>>>>
>>>> If a machine provides an inb()/outb() set of operations that
>>>> is actually used, it should set HAS_IOPORT.
>>>>
>>>> For the Q40, it may be better in the long run to change the
>>>> drivers to just use MMIO directly though.
>>>
>>> Q40 uses ISA.
>>>
>>> Michael is worried about non-ISA drivers using inb() and friends.
>>> At some point in time (i.e. eons ago), we were told it was better to
>>> use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
>>> pointers...
>>>
>>> Anyway, I don't think we have many users of inb() and friends left, and
>>> I assume the bots should have detected any/most remaining users in Niklas'
>>> branch...
>>
>> All the 8390 based ethernet drivers still use inb() and friends.
>>
>> That is the main reason for the terrible hacks in
>> arch/m68k/include/asm/io_mm.h ...
>>
>> The last time I tried to add support for a different PCMCIA ethernet
>> adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't
>> getting anywhere with the netdev crowd. That was ages ago, and I doubt
>> their enthusiasm for a rewrite of the 8390 code base to avoid using
>> inb() on MMIO architectures will be any better now.
>
> From what I can see, there is already an abstraction layer in
> these drivers that is used by all m68k drivers except apne:

As well as ne ... which uses the 8390p.c helper.

>
> $ git grep -w 'define\sei_inb'
> drivers/net/ethernet/8390/8390.h:#define ei_inb(_p)     inb(_p)
> drivers/net/ethernet/8390/8390p.c:#define ei_inb(_p)    inb(_p)
> drivers/net/ethernet/8390/ax88796.c:#define ei_inb(_a) readb(ax_convert_addr(_a))
> drivers/net/ethernet/8390/etherh.c:#define ei_inb(_p)    readb((void __iomem *)_p)
> drivers/net/ethernet/8390/hydra.c:#define ei_inb(port)   in_8(port)
> drivers/net/ethernet/8390/mac8390.c:#define ei_inb(port)        in_8(port)
> drivers/net/ethernet/8390/mcf8390.c:#define     ei_inb  ei_inb
> drivers/net/ethernet/8390/xsurf100.c:#define ei_inb(_a) z_readb(ax_convert_addr(_a))
> drivers/net/ethernet/8390/zorro8390.c:#define ei_inb(port)              in_8(port)
>
> Can't apne.c just do the same here? The patch below didn't
> take that long to come up with, but I may be missing something
> here of course.

The address translation from ISA IO ports to MMIO addresses needs to be 
added as well (in_8() does not use address translation on m68k). apne.c 
also uses inw() which does have a different address translation yet, but 
that's only for data transfer from the ring buffers and can be handled 
entirely inside apne.c.

Now that is all limited to m68k. I might be able to submit a patch, but 
I cannot test any of this.

ne.c needs the same treatment as far as I can see, and I could actually 
test that one (on Atari, not actually on a PC ISA card). I'll see what I 
can come up with.

I might well be missing something else here - as I said, it's been a few 
years since I worked on the apne driver, and experimented with IO 
abstractions in that context. The problem has always been making sure 
drivers shared by different m68k platforms need only be built once and 
still work on e.g. Q40 and Atari.

You've given me something to work with, thanks again!

Cheers,

	Michael

>
>     Arnd
>
> 8<---
> From 5dd43e612a52adf499b1ea3d33e3b2b45894d275 Mon Sep 17 00:00:00 2001
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Fri, 5 Apr 2024 21:47:51 +0200
> Subject: [PATCH] net: apne: convert to lib8390
>
> The apne driver still uses the ISA-style inb()/outb() wappers through the
> 8390.c helper module, which won't work in the future.
>
> Change it to include lib8390.c like all the other m68k variants of this
> driver do, so it can have custom MMIO abstractions.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/net/ethernet/8390/Makefile b/drivers/net/ethernet/8390/Makefile
> index 85c83c566ec6..ec1b325da4e4 100644
> --- a/drivers/net/ethernet/8390/Makefile
> +++ b/drivers/net/ethernet/8390/Makefile
> @@ -4,7 +4,7 @@
>  #
>
>  obj-$(CONFIG_MAC8390) += mac8390.o
> -obj-$(CONFIG_APNE) += apne.o 8390.o
> +obj-$(CONFIG_APNE) += apne.o
>  obj-$(CONFIG_ARM_ETHERH) += etherh.o
>  obj-$(CONFIG_AX88796) += ax88796.o
>  obj-$(CONFIG_HYDRA) += hydra.o
> diff --git a/drivers/net/ethernet/8390/apne.c b/drivers/net/ethernet/8390/apne.c
> index 828edca8d30c..ea3747723b3c 100644
> --- a/drivers/net/ethernet/8390/apne.c
> +++ b/drivers/net/ethernet/8390/apne.c
> @@ -41,7 +41,15 @@
>  #include <asm/amigayle.h>
>  #include <asm/amipcmcia.h>
>
> -#include "8390.h"
> +#define ei_inb(port)            in_8(port)
> +#define ei_outb(val, port)      out_8(port, val)
> +#define ei_inb_p(port)          in_8(port)
> +#define ei_outb_p(val, port)    out_8(port, val)
> +
> +static const char version[] =
> +    "apne.c:v1.1 7/10/98 Alain Malek (Alain.Malek@cryogen.ch)\n";
> +
> +#include "lib8390.c"
>
>  /* ---- No user-serviceable parts below ---- */
>
> @@ -105,14 +113,21 @@ static int init_pcmcia(void);
>  #define MANUAL_HWADDR5 0x9a
>  */
>
> -static const char version[] =
> -    "apne.c:v1.1 7/10/98 Alain Malek (Alain.Malek@cryogen.ch)\n";
> -
>  static int apne_owned;	/* signal if card already owned */
>
> -static u32 apne_msg_enable;
> -module_param_named(msg_enable, apne_msg_enable, uint, 0444);
> -MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
> +static const struct net_device_ops apne_netdev_ops = {
> +	.ndo_open		= __ei_open,
> +	.ndo_stop		= __ei_close,
> +	.ndo_start_xmit		= __ei_start_xmit,
> +	.ndo_tx_timeout		= __ei_tx_timeout,
> +	.ndo_get_stats		= __ei_get_stats,
> +	.ndo_set_rx_mode	= __ei_set_multicast_list,
> +	.ndo_validate_addr	= eth_validate_addr,
> +	.ndo_set_mac_address	= eth_mac_addr,
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +	.ndo_poll_controller	= __ei_poll,
> +#endif
> +};
>
>  static struct net_device * __init apne_probe(void)
>  {
> @@ -141,11 +156,11 @@ static struct net_device * __init apne_probe(void)
>  		return ERR_PTR(-ENODEV);
>  	}
>
> -	dev = alloc_ei_netdev();
> +	dev = ____alloc_ei_netdev(0);
>  	if (!dev)
>  		return ERR_PTR(-ENOMEM);
>  	ei_local = netdev_priv(dev);
> -	ei_local->msg_enable = apne_msg_enable;
> +	ei_local->msg_enable = msg_enable;
>
>  	/* disable pcmcia irq for readtuple */
>  	pcmcia_disable_irq();
> @@ -203,7 +218,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
>  #endif
>      static unsigned version_printed;
>
> -    if ((apne_msg_enable & NETIF_MSG_DRV) && (version_printed++ == 0))
> +    if ((msg_enable & NETIF_MSG_DRV) && (version_printed++ == 0))
>  		netdev_info(dev, version);
>
>      netdev_info(dev, "PCMCIA NE*000 ethercard probe");
> @@ -309,7 +324,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
>
>      dev->base_addr = ioaddr;
>      dev->irq = IRQ_AMIGA_PORTS;
> -    dev->netdev_ops = &ei_netdev_ops;
> +    dev->netdev_ops = &apne_netdev_ops;
>
>      /* Install the Interrupt handler */
>      i = request_irq(dev->irq, apne_interrupt, IRQF_SHARED, DRV_NAME, dev);
> @@ -333,7 +348,7 @@ static int __init apne_probe1(struct net_device *dev, int ioaddr)
>      ei_status.block_output = &apne_block_output;
>      ei_status.get_8390_hdr = &apne_get_8390_hdr;
>
> -    NS8390_init(dev, 0);
> +    __NS8390_init(dev, 0);
>
>      pcmcia_ack_int(pcmcia_get_intreq());		/* ack PCMCIA int req */
>      pcmcia_enable_irq();
> @@ -513,7 +528,7 @@ apne_block_output(struct net_device *dev, int count,
>  	if (time_after(jiffies, dma_start + 2*HZ/100)) {	/* 20ms */
>  		netdev_warn(dev, "timeout waiting for Tx RDC.\n");
>  		apne_reset_8390(dev);
> -		NS8390_init(dev,1);
> +		__NS8390_init(dev,1);
>  		break;
>  	}
>
> @@ -534,10 +549,10 @@ static irqreturn_t apne_interrupt(int irq, void *dev_id)
>          pcmcia_ack_int(pcmcia_intreq);
>          return IRQ_NONE;
>      }
> -    if (apne_msg_enable & NETIF_MSG_INTR)
> +    if (msg_enable & NETIF_MSG_INTR)
>  	pr_debug("pcmcia intreq = %x\n", pcmcia_intreq);
>      pcmcia_disable_irq();			/* to get rid of the sti() within ei_interrupt */
> -    ei_interrupt(irq, dev_id);
> +    __ei_interrupt(irq, dev_id);
>      pcmcia_ack_int(pcmcia_get_intreq());
>      pcmcia_enable_irq();
>      return IRQ_HANDLED;
>

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Sat, Apr 6, 2024, at 03:14, Michael Schmitz wrote:
> Am 06.04.2024 um 09:13 schrieb Arnd Bergmann:
>> On Fri, Apr 5, 2024, at 20:36, Michael Schmitz wrote:
>>> Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
>>> The last time I tried to add support for a different PCMCIA ethernet
>>> adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't
>>> getting anywhere with the netdev crowd. That was ages ago, and I doubt
>>> their enthusiasm for a rewrite of the 8390 code base to avoid using
>>> inb() on MMIO architectures will be any better now.
>>
>> From what I can see, there is already an abstraction layer in
>> these drivers that is used by all m68k drivers except apne:
>
> As well as ne ... which uses the 8390p.c helper.

Is there any machine using ne.c that doesn't set HAS_IOPORT though?
Q40 and ATARI_ETHERNEC both have custom inb()/outb(), so
those are not affected by the change.

>> $ git grep -w 'define\sei_inb'
>> drivers/net/ethernet/8390/8390.h:#define ei_inb(_p)     inb(_p)
>> drivers/net/ethernet/8390/8390p.c:#define ei_inb(_p)    inb(_p)
>> drivers/net/ethernet/8390/ax88796.c:#define ei_inb(_a) readb(ax_convert_addr(_a))
>> drivers/net/ethernet/8390/etherh.c:#define ei_inb(_p)    readb((void __iomem *)_p)
>> drivers/net/ethernet/8390/hydra.c:#define ei_inb(port)   in_8(port)
>> drivers/net/ethernet/8390/mac8390.c:#define ei_inb(port)        in_8(port)
>> drivers/net/ethernet/8390/mcf8390.c:#define     ei_inb  ei_inb
>> drivers/net/ethernet/8390/xsurf100.c:#define ei_inb(_a) z_readb(ax_convert_addr(_a))
>> drivers/net/ethernet/8390/zorro8390.c:#define ei_inb(port)              in_8(port)
>>
>> Can't apne.c just do the same here? The patch below didn't
>> take that long to come up with, but I may be missing something
>> here of course.
>
> The address translation from ISA IO ports to MMIO addresses needs to be 
> added as well (in_8() does not use address translation on m68k).

Indeed, I totally missed that bit.

> apne.c also uses inw() which does have a different address translation
> yet, but that's only for data transfer from the ring buffers and can
> be handled entirely inside apne.c.

and this as well.

> Now that is all limited to m68k. I might be able to submit a patch, but 
> I cannot test any of this.
>
> ne.c needs the same treatment as far as I can see, and I could actually 
> test that one (on Atari, not actually on a PC ISA card). I'll see what I 
> can come up with.

ATARI_ROM_ISA turns on HAS_IOPORT, so this one doesn't need any
immediate changes as a result of Niklas's series. I see now that
the apne driver doesn't actually need changes either since
AMIGA_PCMCIA turns on ISA.

I don't think there is an easy way to rework ne.c to avoid
inb()/outb(), but you could consider splitting the atari
support out into a separate module the same way as apne.c
to make it use the atari operations directly.

> I might well be missing something else here - as I said, it's been a few 
> years since I worked on the apne driver, and experimented with IO 
> abstractions in that context. The problem has always been making sure 
> drivers shared by different m68k platforms need only be built once and 
> still work on e.g. Q40 and Atari.

Do you know of any other ISA style drivers that are used with the
amiga pcmcia or the atari rom I/O operations, aside from the 8390
family? If this is the only one using it, it does sound like this
could be simplified a lot by just making those two not share the
object code with the ISA-style ne.c.

      Arnd

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Michael Schmitz]

Hi Arnd,

Am 07.04.2024 um 03:27 schrieb Arnd Bergmann:
> On Sat, Apr 6, 2024, at 03:14, Michael Schmitz wrote:
>> Am 06.04.2024 um 09:13 schrieb Arnd Bergmann:
>>> On Fri, Apr 5, 2024, at 20:36, Michael Schmitz wrote:
>>>> Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
>>>> The last time I tried to add support for a different PCMCIA ethernet
>>>> adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't
>>>> getting anywhere with the netdev crowd. That was ages ago, and I doubt
>>>> their enthusiasm for a rewrite of the 8390 code base to avoid using
>>>> inb() on MMIO architectures will be any better now.
>>>
>>> From what I can see, there is already an abstraction layer in
>>> these drivers that is used by all m68k drivers except apne:
>>
>> As well as ne ... which uses the 8390p.c helper.
>
> Is there any machine using ne.c that doesn't set HAS_IOPORT though?
> Q40 and ATARI_ETHERNEC both have custom inb()/outb(), so
> those are not affected by the change.

Thanks for clarifying - I had been left with the impression that 
inb()/outb() would only be retained for architectures that have not only 
the ISA bus but inb/outb processor instructions.

>> ne.c needs the same treatment as far as I can see, and I could actually
>> test that one (on Atari, not actually on a PC ISA card). I'll see what I
>> can come up with.
>
> ATARI_ROM_ISA turns on HAS_IOPORT, so this one doesn't need any
> immediate changes as a result of Niklas's series. I see now that
> the apne driver doesn't actually need changes either since
> AMIGA_PCMCIA turns on ISA.

Your patch would not actually be too hard to get right as apne only 
needs unconditional address translation (unless we want to take this 
opportunity to introduce 100 Mbit support; I need to dig out my old 
patches for that). But perhaps leave that for later

> I don't think there is an easy way to rework ne.c to avoid
> inb()/outb(), but you could consider splitting the atari
> support out into a separate module the same way as apne.c
> to make it use the atari operations directly.

We used to have atari_ethernec.c for that. AFAIR the netdev maintainers 
asked for us to use ne.c instead. They had suggestions around making IO 
abstractions more flexible for apne.c a while back. I need to revisit that.

>> I might well be missing something else here - as I said, it's been a few
>> years since I worked on the apne driver, and experimented with IO
>> abstractions in that context. The problem has always been making sure
>> drivers shared by different m68k platforms need only be built once and
>> still work on e.g. Q40 and Atari.
>
> Do you know of any other ISA style drivers that are used with the
> amiga pcmcia or the atari rom I/O operations, aside from the 8390
> family? If this is the only one using it, it does sound like this
> could be simplified a lot by just making those two not share the
> object code with the ISA-style ne.c.

No network drivers that I can remember, but the same ROM I/O is used by 
the isp116x-hcd USB driver. That one uses the defines from asm/raw_io.h 
directly though, not inb()/outb().

I don't think any other PCMCIA cards were ever supported on Amiga.

Cheers,

	Michael

>
>       Arnd
>

Re: [PATCH 0/1] m68k: Handle HAS_IOPORT dependencies

[Geert Uytterhoeven]

On Fri, Apr 5, 2024 at 8:36 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 05.04.2024 um 23:16 schrieb Geert Uytterhoeven:
> > On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >> On Wed, Apr 3, 2024, at 20:11, Michael Schmitz wrote:
> >>> how do you propose we handle legacy drivers that do depend on
> >>> inb()/outb() functions (_not_ actual ISA I/O) on architectures that map
> >>> inb()/outb() to MMIO functions?
> >>>
> >>> (In my case, that's at least ne.c - Geert ought to have a better
> >>> overview what else does use inb()/outb() on m68k)
> >>
> >> If a machine provides an inb()/outb() set of operations that
> >> is actually used, it should set HAS_IOPORT.
> >>
> >> For the Q40, it may be better in the long run to change the
> >> drivers to just use MMIO directly though.
> >
> > Q40 uses ISA.
> >
> > Michael is worried about non-ISA drivers using inb() and friends.
> > At some point in time (i.e. eons ago), we were told it was better to
> > use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
> > pointers...
> >
> > Anyway, I don't think we have many users of inb() and friends left, and
> > I assume the bots should have detected any/most remaining users in Niklas'
> > branch...
>
> All the 8390 based ethernet drivers still use inb() and friends.
>
> That is the main reason for the terrible hacks in
> arch/m68k/include/asm/io_mm.h ...
>
> The last time I tried to add support for a different PCMCIA ethernet
> adapter to apne.c _without_ adding to the hacks in io_mm.h, I wasn't
> getting anywhere with the netdev crowd. That was ages ago, and I doubt
> their enthusiasm for a rewrite of the 8390 code base to avoid using
> inb() on MMIO architectures will be any better now.

As Arnd already discovered later in this thread, AMIGA_PCMCIA
(sort of) selects ISA, so APNE can be considered a "real" ISA. driver,
so it can keep on using inb() and friends.

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 0/1] m68k: Handle HAS_IOPORT dependencies

[Geert Uytterhoeven]

Hi Arnd,

On Fri, Apr 5, 2024 at 1:26 PM Arnd Bergmann <arnd@kernel.org> wrote:
> On Fri, Apr 5, 2024, at 12:16, Geert Uytterhoeven wrote:
> > On Wed, Apr 3, 2024 at 8:35 PM Arnd Bergmann <arnd@kernel.org> wrote:
> > Michael is worried about non-ISA drivers using inb() and friends.
> > At some point in time (i.e. eons ago), we were told it was better to
> > use in[bwl]()/read[bwl]() instead of directly dereferencing volatile
> > pointers...
>
> It's definitely still better to use an abstraction layer for MMIO
> accesses using inline asm instructions than open-coding the
> volatile pointer dereferences. Over time we have gotten better
> at defining which of the available abstractions should be used
> for a given bus, so inb()/outb() is now only really used for
> things derived from ISA in some form, including e.g. PCI and LPC.

Indeed. Nowadays the consensus is that readl() and friends should
always perform little-endian accesses, thus implying byte-swapping on
big-endian platforms, but most of the m68k drivers were written long
before that enlightenment...

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