New network driver for Amiga X-Surf 100 (m68k)

From: Michael Karcher <[email protected]>

Call ax_mii_init in ax_open(), and unregister/remove mdiobus resources in ax_close().

This is needed to be able to unload the module, as the module is busy while the MII bus is attached.

Signed-off-by Michael Karcher <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

---
 drivers/net/ethernet/8390/ax88796.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

drivers/net/ethernet/8390/ax88796.c +0 -0

index 2a256aa..f7b8911 100644

drivers/net/ethernet/8390/ax88796.c +1 -0

+
 /* device private data */
 
 struct ax_device {

Quoted 20 lines · Michael Schmitz

From: Michael Karcher <[email protected]>

Call ax_mii_init in ax_open(), and unregister/remove mdiobus resources in ax_close().

This is needed to be able to unload the module, as the module is busy while the MII bus is attached.

Signed-off-by Michael Karcher <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

---
 drivers/net/ethernet/8390/ax88796.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

drivers/net/ethernet/8390/ax88796.c +0 -0

index 2a256aa..f7b8911 100644
@@ -79,6 +79,8 @@
 

drivers/net/ethernet/8390/ax88796.c line 82

Quoted 3 lines

drivers/net/ethernet/8390/ax88796.c +1 -0

 static u32 ax_msg_enable;
 
+static int ax_mii_init(struct net_device *dev);

Hi Michael

We try to avoid forward declarations. Please can you move ax_mii_init() so this is not needed.

Quoted 8 lines

drivers/net/ethernet/8390/ax88796.c +1 -0

+
 /* device private data */
 
 struct ax_device {

@@ -396,6 +398,10 @@ static int ax_open(struct net_device *dev)
 
      netdev_dbg(dev, "open\n");
 

drivers/net/ethernet/8390/ax88796.c static int ax_open(struct net_device *dev) · line 407

Quoted 7 lines

drivers/net/ethernet/8390/ax88796.c +4 -0

+     ret = ax_mii_init(dev);
+     if (ret)
+             goto failed_request_irq;
+
      ret = request_irq(dev->irq, ax_ei_interrupt, ax->irqflags,
                        dev->name, dev);
      if (ret)

You are missing some cleanup on error at the end of ax_open(). It was also missing before.

Andrew

From: John Paul Adrian Glaubitz <[email protected]>

This complements the fix in 82533ad9a1c that removed the free_irq

call in the error path of probe, to also not call free_irq when remove is called to revert the effects of probe.

Signed-off-by Michael Karcher <[email protected]>

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16]
[cannot apply to net-next/master net/master v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Michael-Schmitz/New-network-driver-for-Amiga-X-Surf-100-m68k/20180417-141150
config: arm-samsung (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm

All warnings (new ones prefixed by >>):

In file included from arch/arm/mach-s3c24xx/mach-anubis.c:42:0:

Quoted 1 line

Quoted 1 line

include/net/ax88796.h:35:11: warning: 'struct sk_buff' declared inside parameter list will not be visible outside of this definition or declaration

       struct sk_buff *skb, int ring_offset);
              ^~~~~~~

vim +35 include/net/ax88796.h

20
21 struct ax_plat_data {
22 unsigned int flags;
23 unsigned char wordlength; /* 1 or 2 */
24 unsigned char dcr_val; /* default value for DCR */
25 unsigned char rcr_val; /* default value for RCR */
26 unsigned char gpoc_val; /* default value for GPOC */
27 u32 *reg_offsets; /* register offsets */
28 u8 *mac_addr; /* MAC addr (only used when
29 AXFLG_MAC_FROMPLATFORM is used */
30
31 /* uses default ax88796 buffer if set to NULL */
32 void (*block_output)(struct net_device *dev, int count,
33 const unsigned char *buf, int star_page);
34 void (*block_input)(struct net_device *dev, int count,

Quoted 1 line

35 struct sk_buff *skb, int ring_offset);

    36  };
    37

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

I think this is a false positive - we're encouraged to provide the full parameter list for functions, so the sreuct sk_buff* can't be avoided.

Quoted 3 lines · Michael Schmitz

I think this is a false positive - we're encouraged to provide the full parameter list for functions, so the sreuct sk_buff* can't be avoided.

Hi Michael

How is <linux/skbuff.h> being included?

You probably want to build using the .config file and see.

Andrew

I think this is a false positive - we're encouraged to provide the full parameter list for functions, so the sreuct sk_buff* can't be avoided.

Cheers,

Michael

Quoted 41 lines · kbuild test robot

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16]
[cannot apply to net-next/master net/master v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Michael-Schmitz/New-network-driver-for-Amiga-X-Surf-100-m68k/20180417-141150
config: arm-samsung (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm

All warnings (new ones prefixed by >>):

In file included from arch/arm/mach-s3c24xx/mach-anubis.c:42:0:

Quoted 1 line

Quoted 1 line

include/net/ax88796.h:35:11: warning: 'struct sk_buff' declared inside parameter list will not be visible outside of this definition or declaration

       struct sk_buff *skb, int ring_offset);
              ^~~~~~~

vim +35 include/net/ax88796.h

20
21 struct ax_plat_data {
22 unsigned int flags;
23 unsigned char wordlength; /* 1 or 2 */
24 unsigned char dcr_val; /* default value for DCR */
25 unsigned char rcr_val; /* default value for RCR */
26 unsigned char gpoc_val; /* default value for GPOC */
27 u32 *reg_offsets; /* register offsets */
28 u8 *mac_addr; /* MAC addr (only used when
29 AXFLG_MAC_FROMPLATFORM is used */
30
31 /* uses default ax88796 buffer if set to NULL */
32 void (*block_output)(struct net_device *dev, int count,
33 const unsigned char *buf, int star_page);
34 void (*block_input)(struct net_device *dev, int count,

Quoted 7 lines

Quoted 1 line

35 struct sk_buff *skb, int ring_offset);

    36  };
    37

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Michael Schmitz

I think this is a false positive - we're encouraged to provide the full parameter list for functions, so the sreuct sk_buff* can't be avoided.

I don't think it's a false positive. I think ax88796.h would need to #include <linux/skbuff.h>.

You may be able to get away with a forward declaration, as in, struct skbuff;
but I'm not sure about that. I would have to build mach-anubis.c to check.

But why do you need to pass an skbuff pointer here? xs100_block_input() only accesses skb->data.

BTW, this patch has an unrelated whitespace change.

--

Quoted 51 lines

Cheers,

Michael

Quoted 45 lines · kbuild test robot

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16]
[cannot apply to net-next/master net/master v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Michael-Schmitz/New-network-driver-for-Amiga-X-Surf-100-m68k/20180417-141150
config: arm-samsung (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm

All warnings (new ones prefixed by >>):

In file included from arch/arm/mach-s3c24xx/mach-anubis.c:42:0:

Quoted 1 line

Quoted 1 line

include/net/ax88796.h:35:11: warning: 'struct sk_buff' declared inside parameter list will not be visible outside of this definition or declaration

       struct sk_buff *skb, int ring_offset);
              ^~~~~~~

vim +35 include/net/ax88796.h

20
21 struct ax_plat_data {
22 unsigned int flags;
23 unsigned char wordlength; /* 1 or 2 */
24 unsigned char dcr_val; /* default value for DCR */
25 unsigned char rcr_val; /* default value for RCR */
26 unsigned char gpoc_val; /* default value for GPOC */
27 u32 *reg_offsets; /* register offsets */
28 u8 *mac_addr; /* MAC addr (only used when
29 AXFLG_MAC_FROMPLATFORM is used */
30
31 /* uses default ax88796 buffer if set to NULL */
32 void (*block_output)(struct net_device *dev, int count,
33 const unsigned char *buf, int star_page);
34 void (*block_input)(struct net_device *dev, int count,

Quoted 1 line

35 struct sk_buff *skb, int ring_offset);

    36  };
    37

Quoted 8 lines

Quoted 3 lines

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: John Paul Adrian Glaubitz <[email protected]>

The AX88796B as installed on the X-Surf-100 does not recognize a MII reset request if the previous write to the MII control register also was a reset request. So a dummy write to the control register makes the soft reset in the PHY initialization code work.

Signed-off-by Michael Karcher <[email protected]>

---
 drivers/net/ethernet/8390/ax88796.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/net/ethernet/8390/ax88796.c +0 -0

index 6af9aca..a2f9a09 100644

Quoted 20 lines · Michael Schmitz

From: John Paul Adrian Glaubitz <[email protected]>

The AX88796B as installed on the X-Surf-100 does not recognize a MII reset request if the previous write to the MII control register also was a reset request. So a dummy write to the control register makes the soft reset in the PHY initialization code work.

Signed-off-by Michael Karcher <[email protected]>

---
 drivers/net/ethernet/8390/ax88796.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/net/ethernet/8390/ax88796.c +0 -0

index 6af9aca..a2f9a09 100644
@@ -374,6 +374,10 @@ static int ax_mii_probe(struct net_device *dev)
              return -ENODEV;
      }
 

drivers/net/ethernet/8390/ax88796.c static int ax_mii_probe(struct net_device *dev) · line 380

Quoted 4 lines

drivers/net/ethernet/8390/ax88796.c +4 -0

+     /* write a non-reset pattern to the control register to
+      * re-arm the reset request detection logic (needed on AX88796B)
+      */
+     phy_write(phy_dev, MII_BMCR, 0);

This should really be fixed in the PHY driver, not the MAC.

Andrew

Quoted 1 line

This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have a few side effects on more commonly used hardware, perhaps?

Quoted 4 lines

Quoted 1 line

This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have a few side effects on more commonly used hardware, perhaps?

Hi Michael

What PHY driver is used?

In the driver you can implement a .soft_reset function which first does the dummy write, and then uses genphy_soft_reset() to do the actual reset.

[Andrew, sorry for the dup. I did hit reply-to-auhor instead of reply-to-all first.]

Quoted 8 lines · Andrew Lunn

Quoted 4 lines

Quoted 1 line

This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have a few side effects on more commonly used hardware, perhaps?

Hi Michael

What PHY driver is used?

The ax88796b comes with its own integrated (buggy) PHY needing this workaround. This PHY has its own ID which is not known by Linux, so it is using the genphy driver as fallback.

Quoted 3 lines

In the driver you can implement a .soft_reset function which first does the dummy write, and then uses genphy_soft_reset() to do the actual reset.

We could do that - but I dont't see the point in creating a PHY driver that is only ever used by this MAC driver, just to add a single line to the genphy driver. If the same PHY might be used with a different MAC, you definitely would have a point there, though.

Quoted 16 lines · Michael Karcher

[Andrew, sorry for the dup. I did hit reply-to-auhor instead of reply-to-all first.]

Quoted 8 lines · Andrew Lunn

Quoted 4 lines

Quoted 1 line

This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have a few side effects on more commonly used hardware, perhaps?

Hi Michael

What PHY driver is used?

The ax88796b comes with its own integrated (buggy) PHY needing this workaround. This PHY has its own ID which is not known by Linux, so it is using the genphy driver as fallback.

Quoted 7 lines

Quoted 3 lines

In the driver you can implement a .soft_reset function which first does the dummy write, and then uses genphy_soft_reset() to do the actual reset.

We could do that - but I dont't see the point in creating a PHY driver that is only ever used by this MAC driver, just to add a single line to the genphy driver. If the same PHY might be used with a different MAC, you definitely would have a point there, though.

Hi Michael

We try to keep the core code clean, and put all workarounds for buggy hardware in drivers specific to them. It just helps keep the core code maintainable.

I would prefer a driver specific to this PHY with the workaround. But lets see what Florian says.

Quoted 26 lines · Andrew Lunn

Quoted 16 lines · Michael Karcher

[Andrew, sorry for the dup. I did hit reply-to-auhor instead of reply-to-all first.]

Quoted 8 lines · Andrew Lunn

Quoted 4 lines

Quoted 1 line

This should really be fixed in the PHY driver, not the MAC.

OK - do you want this separate, or as part of this series? Might have a few side effects on more commonly used hardware, perhaps?

Hi Michael

What PHY driver is used?

The ax88796b comes with its own integrated (buggy) PHY needing this workaround. This PHY has its own ID which is not known by Linux, so it is using the genphy driver as fallback.

Quoted 7 lines

Quoted 3 lines

In the driver you can implement a .soft_reset function which first does the dummy write, and then uses genphy_soft_reset() to do the actual reset.

We could do that - but I dont't see the point in creating a PHY driver that is only ever used by this MAC driver, just to add a single line to the genphy driver. If the same PHY might be used with a different MAC, you definitely would have a point there, though.

Quoted 8 lines

Hi Michael

We try to keep the core code clean, and put all workarounds for buggy hardware in drivers specific to them. It just helps keep the core code maintainable.

I would prefer a driver specific to this PHY with the workaround. But lets see what Florian says.

If you are already using the generic PHY driver, coming up with a custom one that only overrides the soft_reset and/or config_init callback is really not that much work, and as Andrew says, it helps make things clearer and properly isolated. As far as where to place that driver, you can either create a new file under drivers/net/phy/* or you can even register a phy_driver instance from within ax88796 if that makes it any clearer.

FWIW, there are plenty of examples where there is a PHY driver used by a single MAC, and that is perfectly fine, because the abstraction is still preserved.
--
Florian

Add platform device driver to populate the ax88796 platform data from information provided by the XSurf100 zorro device driver. This driver will have to be loaded before loading the ax88796 module, or compiled as built-in.

Signed-off-by Michael Karcher <[email protected]>

drivers/net/ethernet/8390/xsurf100.c new file +12 -0

+/* Hard reset the card. This used to pause for the same period that a
+ * 8390 reset command required, but that shouldn't be necessary.
+ */
+static void ax_reset_8390(struct net_device *dev)
+{
+       struct ei_device *ei_local = netdev_priv(dev);
+       unsigned long reset_start_time = jiffies;
+       void __iomem *addr = (void __iomem *)dev->base_addr;
+
+       netif_dbg(ei_local, hw, dev, "resetting the 8390 t=%ld...\n", jiffies);
+
+       ei_outb(ei_inb(addr + NE_RESET), addr + NE_RESET);

Hi Michael,

Thanks for your patch!

Quoted 4 lines · Michael Schmitz

Add platform device driver to populate the ax88796 platform data from information provided by the XSurf100 zorro device driver. This driver will have to be loaded before loading the ax88796 module, or compiled as built-in.

Is that really true? The platform device should be probed when both the device and driver have been registered, but order shouldn't matter.

Quoted 2 lines

Signed-off-by Michael Karcher <[email protected]>

Missing "From: Michael Karcher ..."?

Quoted 7 lines

Signed-off-by Michael Schmitz <[email protected]>

drivers/net/ethernet/8390/Kconfig +0 -0

@@ -30,7 +30,7 @@ config PCMCIA_AXNET
 

drivers/net/ethernet/8390/Kconfig config PCMCIA_AXNET · line 33

Quoted 4 lines

drivers/net/ethernet/8390/Kconfig +1 -1

 config AX88796
        tristate "ASIX AX88796 NE2000 clone support"
-       depends on (ARM || MIPS || SUPERH)
+       depends on (ARM || MIPS || SUPERH || AMIGA)

s/AMIGA/ZORRO/, for consistency with the below.

drivers/net/ethernet/8390/Kconfig config AX88796_93CX6 · line 51

Quoted 12 lines

drivers/net/ethernet/8390/Kconfig +4 -0

        select CRC32
        select PHYLIB
        select MDIO_BITBANG

@@ -45,6 +45,18 @@ config AX88796_93CX6
        ---help---
          Select this if your platform comes with an external 93CX6 eeprom.
 
+config XSURF100
+       tristate "Amiga XSurf 100 AX88796/NE2000 clone support"
+       depends on ZORRO
+       depends on AX88796

It's a bit unfortunate the user has to enable _two_ config options to enable this driver.

I see two solutions for that:

1) Hide the XSURF100 symbol, so it gets enabled automatically if AX88796 is enabled on a Zorro bus system:

config XSURF100
tristate
depends on ZORRO
default AX88796

2) Hide the AX88796 symbol, and let it be selected by XSURF100:

    config AX88796
            tristate "ASIX AX88796 NE2000 clone support" if !ZORRO
            depends on ARM || MIPS || SUPERH || ZORRO
            ...

    config XSURF100
            tristate "Amiga XSurf 100 AX88796/NE2000 clone support"
            depends on ZORRO
            select AX88796

drivers/net/ethernet/8390/xsurf100.c line 9

Quoted 12 lines

drivers/net/ethernet/8390/xsurf100.c new file +9 -0

@@ -0,0 +1,411 @@
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/zorro.h>
+#include <net/ax88796.h>
+#include <asm/amigaints.h>
+
+#define ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF100 \
+               ZORRO_ID(INDIVIDUAL_COMPUTERS, 0x64, 0)

Another long define to get rid of? ;-)

Quoted 12 lines

drivers/net/ethernet/8390/xsurf100.c new file +12 -0

+/* Hard reset the card. This used to pause for the same period that a
+ * 8390 reset command required, but that shouldn't be necessary.
+ */
+static void ax_reset_8390(struct net_device *dev)
+{
+       struct ei_device *ei_local = netdev_priv(dev);
+       unsigned long reset_start_time = jiffies;
+       void __iomem *addr = (void __iomem *)dev->base_addr;
+
+       netif_dbg(ei_local, hw, dev, "resetting the 8390 t=%ld...\n", jiffies);
+
+       ei_outb(ei_inb(addr + NE_RESET), addr + NE_RESET);

drivers/net/ethernet/8390/xsurf100.c line 75

Quoted 10 lines

drivers/net/ethernet/8390/xsurf100.c new file +10 -0

+
+       ei_local->txing = 0;
+       ei_local->dmaing = 0;
+
+       /* This check _should_not_ be necessary, omit eventually. */
+       while ((ei_inb(addr + EN0_ISR) & ENISR_RESET) == 0) {
+               if (time_after(jiffies, reset_start_time + 2 * HZ / 100)) {
+                       netdev_warn(dev, "%s: did not complete.\n", __func__);
+                       break;
+               }

cpu_relax()?

How long does this usually take? If > 1 ms, you can use e.g. msleep(1) instead of cpu_relax().

Quoted 5 lines

drivers/net/ethernet/8390/xsurf100.c new file +4 -0

+       }
+
+       ei_outb(ENISR_RESET, addr + EN0_ISR);   /* Ack intr. */
+}

drivers/net/ethernet/8390/xsurf100.c line 195

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c new file +4 -0

+       if (ei_local->dmaing) {
+               netdev_err(dev,
+                          "DMAing conflict in %s "
+                          "[DMAstat:%d][irqlock:%d].\n",

Please don't split error messages, as that makes it more difficult to grep for them.

Quoted 7 lines

drivers/net/ethernet/8390/xsurf100.c new file +6 -0

+                          __func__,
+                          ei_local->dmaing, ei_local->irqlock);
+               return;
 
+static int xsurf100_probe(struct zorro_dev *zdev,
+                         const struct zorro_device_id *ent)
+{

drivers/net/ethernet/8390/xsurf100.c line 323

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c new file +4 -0

+       /* error handling for ioremap regs */
+       if (!ax88796_data.base_regs) {
+               dev_err(&zdev->dev, "Cannot ioremap area %p (registers)\n",
+                       (void *)zdev->resource.start);

Please use %pR to format struct resource.
Documentation/core-api/printk-formats.rst

drivers/net/ethernet/8390/xsurf100.c line 335

Quoted 5 lines

drivers/net/ethernet/8390/xsurf100.c new file +4 -0

+       /* error handling for ioremap data */
+       if (!ax88796_data.data_area) {
+               dev_err(&zdev->dev, "Cannot ioremap area %p (32-bit access)\n",
+                       (void *)zdev->resource.start + XS100_8390_DATA32_BASE);

%pR

drivers/net/ethernet/8390/xsurf100.c line 382

Quoted 8 lines

drivers/net/ethernet/8390/xsurf100.c new file +7 -0

+static void xsurf100_remove(struct zorro_dev *zdev)
+{
+       struct platform_device *pdev;
+       struct xsurf100_ax_plat_data *xs100;
+
+       pdev = zorro_get_drvdata(zdev);
+       xs100 = dev_get_platdata(&pdev->dev);

struct platform_device *pdev = pdev = zorro_get_drvdata(zdev); struct xsurf100_ax_plat_data *xs100 = dev_get_platdata(&pdev->dev);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Quoted 18 lines · Geert Uytterhoeven

Quoted 16 lines · Michael Schmitz

Add platform device driver to populate the ax88796 platform data from information provided by the XSurf100 zorro device driver. This driver will have to be loaded before loading the ax88796 module, or compiled as built-in.

drivers/net/ethernet/8390/xsurf100.c new file +7 -0

 
+static int xsurf100_probe(struct zorro_dev *zdev,
+                         const struct zorro_device_id *ent)
+{
 
+       /* error handling for ioremap regs */
+       if (!ax88796_data.base_regs) {
+               dev_err(&zdev->dev, "Cannot ioremap area %p (registers)\n",
+                       (void *)zdev->resource.start);

Quoted 2 lines

Please use %pR to format struct resource.
Documentation/core-api/printk-formats.rst

The driver uses ioremap to map two subsections of the mem resource for two different purposes - control register access, and ring buffer access. The output of %pR may be misleading here (wrong size), and even more so below.

Quoted 6 lines

drivers/net/ethernet/8390/xsurf100.c line 335

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c new file +4 -0

+       /* error handling for ioremap data */
+       if (!ax88796_data.data_area) {
+               dev_err(&zdev->dev, "Cannot ioremap area %p (32-bit access)\n",
+                       (void *)zdev->resource.start + XS100_8390_DATA32_BASE);

%pR

I've added the offset into the mem resource here to clarify what we've tried to map.

Quoted 6 lines

drivers/net/ethernet/8390/xsurf100.c new file +3 -0

@@ -0,0 +1,411 @@
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>

drivers/net/ethernet/8390/xsurf100.c line 9

Quoted 6 lines

drivers/net/ethernet/8390/xsurf100.c new file +6 -0

+#include <linux/zorro.h>
+#include <net/ax88796.h>
+#include <asm/amigaints.h>
+
+#define ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF100 \
+               ZORRO_ID(INDIVIDUAL_COMPUTERS, 0x64, 0)

Another long define to get rid of? ;-)

This patch series adds support for the Individual Computers X-Surf 100 network card for m68k Amiga, a network adapter based on the AX88796 chip set.

The driver was originally written for kernel version 3.19 by Michael Karcher (see CC:), and adapted to 4.16 for submission to netdev by me. Questions regarding motivation for some of the changes are probably best directed at Michael Karcher.

The driver has been tested by Adrian <[email protected]> who will send his Tested-by tag separately.

A few changes to the ax88796 driver were required:
- to read the MAC address, some setup of the ax99796 chip must be done,
- attach to the MII bus only on device open to allow module unloading,
- allow to supersede ax_block_input/ax_block_output by card-specific optimized code,
- use an optional interrupt status callback to allow easier sharing of the card interrupt,
- set IRQF_SHARED if platform IRQ resource is marked shareable,

The Asix Electronix PHY used on the X-Surf 100 is buggy, and causes the software reset to hang if the previous command sent to the PHY was also a soft reset. This bug requires addition of a PHY driver for Asix PHYs to provide a fixed .soft_reset function, included in this series.

Some additional cleanup:
- do not attempt to free IRQ in ax_remove (complements 82533ad9a1c),
- clear platform drvdata on probe fail and module remove.

Changes since v1:

Raised in review by Andrew Lunn:
- move MII code around to avoid need for forward declaration
- combine patches 2 and 7 to add cleanup in error path

Changes since v2:

- corrected authorship attribution to Michael Karcher

Suggested by Geert Uytterhoeven:
- use ei_local->reset_8390() instead of duplicating ax_reset_8390()
- use %pR to format struct resource pointers
- assign pdev and xs100 pointers in declaration
- don't split error messages
- change Kconfig logic to only require XSURF100 set on Amiga

Suggested by Andrew Lunn:
- add COMPILE_TEST to ax88796 Kconfig options
- use new Asix PHY driver for X-Surf 100

Suggested by Andrew Lunn/Finn Thain:
- declare struct sk_buff in ax88796.h
- correct whitespace error in ax88796.h

This series' patches, in order:

1/9 net: phy: new Asix Electronics PHY driver
2/9 net: ax88796: Fix MAC address reading
3/9 net: ax88796: Attach MII bus only when open
4/9 net: ax88796: Do not free IRQ in ax_remove() (already freed in ax_close()). 5/9 net: ax88796: Add block_input/output hooks to ax_plat_data
6/9 net: ax88796: add interrupt status callback to platform data
7/9 net: ax88796: set IRQF_SHARED flag when IRQ resource is marked as shareable 8/9 net: ax88796: release platform device drvdata on probe error and module remove 9/9 net: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

 drivers/net/ethernet/8390/Kconfig    |   17 ++-
 drivers/net/ethernet/8390/Makefile   |    1 +
 drivers/net/ethernet/8390/ax88796.c  |  228 ++++++++++++--------
 drivers/net/ethernet/8390/xsurf100.c |  381 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/Kconfig              |    6 +
 drivers/net/phy/Makefile             |    1 +
 drivers/net/phy/asix.c               |   65 ++++++
 drivers/net/phy/phy_device.c         |    3 +-
 include/linux/phy.h                  |    1 +
 include/net/ax88796.h                |   14 ++
 10 files changed, 621 insertions(+), 96 deletions(-)

Cheers,

Michael

All,

just noticed belatedly that the Makefile hunk of patch 9 does no longer apply cleanly in 4.17-rc1, sorry. My series was based on 4.16. I'll resend that one, OK?

Cheers,

Michael

Quoted 5 lines

1/9 net: phy: new Asix Electronics PHY driver
2/9 net: ax88796: Fix MAC address reading
3/9 net: ax88796: Attach MII bus only when open
4/9 net: ax88796: Do not free IRQ in ax_remove() (already freed in ax_close()). 5/9 net: ax88796: Add block_input/output hooks to ax_plat_data

Quoted 4 lines

6/9 net: ax88796: add interrupt status callback to platform data
7/9 net: ax88796: set IRQF_SHARED flag when IRQ resource is marked as shareable 8/9 net: ax88796: release platform device drvdata on probe error and module remove 9/9 net: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

Quoted 16 lines

 drivers/net/ethernet/8390/Kconfig    |   17 ++-
 drivers/net/ethernet/8390/Makefile   |    1 +
 drivers/net/ethernet/8390/ax88796.c  |  228 ++++++++++++--------
 drivers/net/ethernet/8390/xsurf100.c |  381 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/Kconfig              |    6 +
 drivers/net/phy/Makefile             |    1 +
 drivers/net/phy/asix.c               |   65 ++++++
 drivers/net/phy/phy_device.c         |    3 +-
 include/linux/phy.h                  |    1 +
 include/net/ax88796.h                |   14 ++
 10 files changed, 621 insertions(+), 96 deletions(-)

Cheers,

Michael

Quoted 5 lines

Quoted 5 lines

1/9 net: phy: new Asix Electronics PHY driver
2/9 net: ax88796: Fix MAC address reading
3/9 net: ax88796: Attach MII bus only when open
4/9 net: ax88796: Do not free IRQ in ax_remove() (already freed in ax_close()). 5/9 net: ax88796: Add block_input/output hooks to ax_plat_data

I found that git am rejects this one, though 'patch' applies it with fuzz.

Quoted 5 lines

Quoted 5 lines

6/9 net: ax88796: add interrupt status callback to platform data
7/9 net: ax88796: set IRQF_SHARED flag when IRQ resource is marked as shareable 8/9 net: ax88796: release platform device drvdata on probe error and module remove 9/9 net: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

git am rejected this one and also complained about trailing whitespace.

I'd rebase on v4.17-rc1 and also run checkpatch over the results.

--

Quoted 16 lines

Quoted 16 lines

 drivers/net/ethernet/8390/Kconfig    |   17 ++-
 drivers/net/ethernet/8390/Makefile   |    1 +
 drivers/net/ethernet/8390/ax88796.c  |  228 ++++++++++++--------
 drivers/net/ethernet/8390/xsurf100.c |  381 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/Kconfig              |    6 +
 drivers/net/phy/Makefile             |    1 +
 drivers/net/phy/asix.c               |   65 ++++++
 drivers/net/phy/phy_device.c         |    3 +-
 include/linux/phy.h                  |    1 +
 include/net/ax88796.h                |   14 ++
 10 files changed, 621 insertions(+), 96 deletions(-)

Cheers,

Michael

All,

just noticed belatedly that the Makefile hunk of patch 9 does no longer apply cleanly in 4.17-rc1, sorry. My series was based on 4.16. I'll resend that one, OK?

Quoted 5 lines · Michael Schmitz

All,

just noticed belatedly that the Makefile hunk of patch 9 does no longer apply cleanly in 4.17-rc1, sorry. My series was based on 4.16. I'll resend that one, OK?

Hi Michael

You should be based on DaveM net-next tree:

git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git

Please also have "net-next" in the patch subject. See Documentation/networking/netdev-FAQ.txt

Andrew

[This is a resend of my v3 series which was based on the wrong version and tree. Only substantial change is to Asix AX99796B PHY driver.]

This patch series adds support for the Individual Computers X-Surf 100 network card for m68k Amiga, a network adapter based on the AX88796 chip set.

The driver was originally written for kernel version 3.19 by Michael Karcher (see CC:), and adapted to 4.16+ for submission to netdev by me. Questions regarding motivation for some of the changes are probably best directed at Michael Karcher.

The driver has been tested by Adrian <[email protected]> who will send his Tested-by tag separately.

A few changes to the ax88796 driver were required:
- to read the MAC address, some setup of the ax99796 chip must be done,
- attach to the MII bus only on device open to allow module unloading,
- allow to supersede ax_block_input/ax_block_output by card-specific optimized code,
- use an optional interrupt status callback to allow easier sharing of the card interrupt,
- set IRQF_SHARED if platform IRQ resource is marked shareable

The Asix Electronix PHY used on the X-Surf 100 is buggy, and causes the software reset to hang if the previous command sent to the PHY was also a soft reset. This bug requires addition of a PHY driver for Asix PHYs to provide a fixed .soft_reset function, included in this series.

Some additional cleanup:
- do not attempt to free IRQ in ax_remove (complements 82533ad9a1c),
- clear platform drvdata on probe fail and module remove.

Changes since v1:

Raised in review by Andrew Lunn:
- move MII code around to avoid need for forward declaration,
- combine patches 2 and 7 to add cleanup in error path

Changes since v2:

- corrected authorship attribution to Michael Karcher

Suggested by Geert Uytterhoeven:
- use ei_local->reset_8390() instead of duplicating ax_reset_8390(),
- use %pR to format struct resource pointers,
- assign pdev and xs100 pointers in declaration,
- don't split error messages,
- change Kconfig logic to only require XSURF100 set on Amiga

Suggested by Andrew Lunn:
- add COMPILE_TEST to ax88796 Kconfig options,
- use new Asix PHY driver for X-Surf 100

Suggested by Andrew Lunn/Finn Thain:
- declare struct sk_buff in ax88796.h,
- correct whitespace error in ax88796.h

Changes since v3:

- various checkpatch cleanup

Andrew Lunn:
- don't duplicate genphy_soft_reset in Asix PHY driver, just call genphy_soft_reset after writing zero to control register

This series' patches, in order:

1/9 net-next: phy: new Asix Electronics PHY driver
2/9 net-next: ax88796: Fix MAC address reading
3/9 net-next: ax88796: Attach MII bus only when open
4/9 net-next: ax88796: Do not free IRQ in ax_remove() (already freed in ax_close()). 5/9 net-next: ax88796: Add block_input/output hooks to ax_plat_data
6/9 net-next: ax88796: add interrupt status callback to platform data
7/9 net-next: ax88796: set IRQF_SHARED flag when IRQ resource is marked as shareable 8/9 net-next: ax88796: release platform device drvdata on probe error and module remove 9/9 net-next: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

 drivers/net/ethernet/8390/Kconfig    |   17 ++-
 drivers/net/ethernet/8390/Makefile   |    1 +
 drivers/net/ethernet/8390/ax88796.c  |  228 ++++++++++++--------
 drivers/net/ethernet/8390/xsurf100.c |  382 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/Kconfig              |    6 +
 drivers/net/phy/Makefile             |    1 +
 drivers/net/phy/asix.c               |   63 ++++++
 include/net/ax88796.h                |   14 ++
 8 files changed, 617 insertions(+), 95 deletions(-)

Cheers,

Michael

This patch series adds support for the Individual Computers X-Surf 100 network card for m68k Amiga, a network adapter based on the AX88796 chip set.

From: Michael Schmitz <[email protected]>
Date: Thu, 19 Apr 2018 14:05:17 +1200

Quoted 2 lines

This patch series adds support for the Individual Computers X-Surf 100 network card for m68k Amiga, a network adapter based on the AX88796 chip set.

Series applied, thank you.

The Asix Electronics PHY found on the X-Surf 100 Amiga Zorro network card by Individual Computers is buggy, and needs the reset bit toggled as workaround to make a PHY soft reset succeed.

Add workaround driver just for this special case.

Suggested in xsurf100 patch series review by Andrew Lunn <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

The Asix Electronics PHY found on the X-Surf 100 Amiga Zorro network card by Individual Computers is buggy, and needs the reset bit toggled as workaround to make a PHY soft reset succeed.

Add workaround driver just for this special case.

Suggested in xsurf100 patch series review by Andrew Lunn <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

drivers/net/phy/asix.c new file +1 -0

@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0

Michael,

Quoted 10 lines · Michael Schmitz

drivers/net/phy/asix.c new file +7 -0

@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for Asix PHYs
+ *
+ * Author: Michael Schmitz <[email protected]>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the

drivers/net/phy/asix.c line 11

Quoted 4 lines

drivers/net/phy/asix.c new file +4 -0

+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */

This license information is broken. The SPDX license identifier and the boiler plate text are contradicting. The SPDX id is GPL v2 only and the boiler plate says v2 or later.

Quoted 13 lines · Thomas Gleixner

Michael,

Quoted 9 lines · Michael Schmitz

drivers/net/phy/asix.c new file +6 -0

@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for Asix PHYs
+ *
+ * Author: Michael Schmitz <[email protected]>
+ *
+ * This program is free software; you can redistribute  it and/or modify it

Quoted 8 lines

drivers/net/phy/asix.c line 11

Quoted 5 lines

drivers/net/phy/asix.c new file +5 -0

+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */

This license information is broken. The SPDX license identifier and the boiler plate text are contradicting. The SPDX id is GPL v2 only and the boiler plate says v2 or later.

Hi Thomas

Please see:

https://www.spinics.net/lists/netdev/msg544312.html

The first two patches are simple SPDX converstions. Then it gets interesting trying to sort out license inconsistencies.

Andrew

From: Michael Karcher <[email protected]>

Add platform device driver to populate the ax88796 platform data from information provided by the XSurf100 zorro device driver. The ax88796 module will be loaded through this module's probe function.

Signed-off-by Michael Karcher <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

drivers/net/ethernet/8390/xsurf100.c new file +1 -0

 
+#define __NS8390_init ax_NS8390_init

+#include "lib8390.c"

drivers/net/ethernet/8390/xsurf100.c new file +3 -0

+static void xs100_block_output(struct net_device *dev, int count,
+                              const unsigned char *buf, const int start_page)
+{

Hi Michael,

Quoted 8 lines · Michael Schmitz

From: Michael Karcher <[email protected]>

Add platform device driver to populate the ax88796 platform data from information provided by the XSurf100 zorro device driver. The ax88796 module will be loaded through this module's probe function.

Signed-off-by Michael Karcher <[email protected]>

Signed-off-by Michael Schmitz <[email protected]>

This is now commit 861928f4e60e826c ("net-next: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)").

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c new file +1 -0

 
+#define __NS8390_init ax_NS8390_init

[...]

Quoted 1 line

+#include "lib8390.c"

drivers/net/ethernet/8390/lib8390.c:202: warning: ‘__ei_open’ defined but not used
drivers/net/ethernet/8390/lib8390.c:231: warning: ‘__ei_close’ defined but not used
drivers/net/ethernet/8390/lib8390.c:255: warning: ‘__ei_tx_timeout’ defined but not used drivers/net/ethernet/8390/lib8390.c:302: warning: ‘__ei_start_xmit’ defined but not used drivers/net/ethernet/8390/lib8390.c:510: warning: ‘__ei_poll’ defined but not used
drivers/net/ethernet/8390/lib8390.c:851: warning: ‘__ei_get_stats’ defined but not used drivers/net/ethernet/8390/lib8390.c:951: warning:
‘__ei_set_multicast_list’ defined but not used drivers/net/ethernet/8390/lib8390.c:989: warning:
‘____alloc_ei_netdev’ defined but not used

So I was wondering: why is this file included, as XSURF100 selects AX88796, while ax88796.c includes lib8390.c anyway?

Apparently lib8390.c is included for register definitions (provided by 8390.h, and can easily be fixed), and for the __NS8390_init() implementation, called below.

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c new file +3 -0

+static void xs100_block_output(struct net_device *dev, int count,
+                              const unsigned char *buf, const int start_page)
+{

[...]

drivers/net/ethernet/8390/xsurf100.c line 242

Quoted 12 lines

drivers/net/ethernet/8390/xsurf100.c new file +12 -0

+       while ((ei_inb(nic_base + EN0_ISR) & ENISR_RDC) == 0) {
+               if (jiffies - dma_start > 2 * HZ / 100) {       /* 20ms */
+                       netdev_warn(dev, "timeout waiting for Tx RDC.\n");
+                       ei_local->reset_8390(dev);
+                       ax_NS8390_init(dev, 1);
+                       break;
+               }
+       }
+
+       ei_outb(ENISR_RDC, nic_base + EN0_ISR); /* Ack intr. */
+       ei_local->dmaing &= ~0x01;
+}

Can we get rid of the inclusion of lib8390.c, and the related warnings? Perhaps ax88796.c can export its ax_NS8390_init(), iff the implementation is identical? Or is there a better solution?

Thanks!

Gr{oetje,eeting}s,

Geert

Am 08.06.2018 um 02:36 schrieb Geert Uytterhoeven:

Quoted 7 lines

So I was wondering: why is this file included, as XSURF100 selects AX88796,
while ax88796.c includes lib8390.c anyway?

Apparently lib8390.c is included for register definitions (provided by 8390.h, and can easily be fixed), and for the __NS8390_init() implementation, called below.

Mostly the latter.

Quoted 8 lines

Quoted 5 lines

(quoted diff) +2 -0

+static void xs100_block_output(struct net_device *dev, int count,
+                              const unsigned char *buf, const int

start_page)
+{

[...]

Quoted 6 lines

Quoted 6 lines

(quoted diff) +2 -0

+       while ((ei_inb(nic_base + EN0_ISR) & ENISR_RDC) == 0) {
+               if (jiffies - dma_start > 2 * HZ / 100) {       /* 20ms

*/
+ netdev_warn(dev, "timeout waiting for Tx
RDC.\n");
+ ei_local->reset_8390(dev);

Quoted 8 lines

drivers/net/ethernet/8390/xsurf100.c line 242

Quoted 8 lines

drivers/net/ethernet/8390/xsurf100.c new file +8 -0

+                       ax_NS8390_init(dev, 1);
+                       break;
+               }
+       }
+
+       ei_outb(ENISR_RDC, nic_base + EN0_ISR); /* Ack intr. */
+       ei_local->dmaing &= ~0x01;
+}

Let me add my 2 cents as main author of that code:

actually, the the block_input / block_output functions were the reason I included the lib8390.c file. Except for xs100_write / xs100_read, they are a verbatim copy from ax88796.c I'm not that enthusiastic about that idea anymore, but did not get around to improve it. I added a customization point to ax88796 for a custom block_input / block_output, because the 8390 core already provides that customization point. What I really need is a customization point just for the 8390-remote-DMA-via-MMIO functions (i.e. xs100_write, xs100_read) instead of the whole block transfer core that also sets up the remote DMA engine and tries to re-initialize the card in case of unexplained problems.

This should get rid of
- xs100_block_output
- xs100_block_input (which has the calls to ax_reset_8390 and
ax_NS8390_init)
- ax_reset_8390
- and thus the include of lib8390.c (which should be included only by
ax88796.c, not by xsurf100.c)

Regards,
Michael Karcher

Hi Michael,

Quoted 6 lines

actually, the the block_input / block_output functions were the reason I included the lib8390.c file. Except for xs100_write / xs100_read, they are
a verbatim copy from ax88796.c I'm not that enthusiastic about that idea anymore, but did not get around to improve it. I added a customization point to ax88796 for a custom block_input / block_output, because the

Quoted 8 lines

8390
core already provides that customization point. What I really need is a customization point just for the 8390-remote-DMA-via-MMIO functions (i.e.
xs100_write, xs100_read) instead of the whole block transfer core that also sets up the remote DMA engine and tries to re-initialize the card in
case of unexplained problems.

OK, so essentially change

         if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

to something like

         if (ax->block_read) {
		ax->block_read(dev, buf, count);
	} else if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

and populate ax->block_read() and ax_block_write() from platform data, instead of substituting ax_block_input() / ax_block_output() wholesale?

I must be missing something here.

Adds one test for
ax->block_read on the critical path but we already have the test for ei_local->word16 there. May need rearranging the tests so the majority of ax88796 users isn't impacted.

Anyway, your code, your call.

Quoted 40 lines · Michael Schmitz

Hi Michael,

Quoted 6 lines

actually, the the block_input / block_output functions were the reason I included the lib8390.c file. Except for xs100_write / xs100_read, they are
a verbatim copy from ax88796.c I'm not that enthusiastic about that idea anymore, but did not get around to improve it. I added a customization point to ax88796 for a custom block_input / block_output, because the

Quoted 8 lines

8390
core already provides that customization point. What I really need is a customization point just for the 8390-remote-DMA-via-MMIO functions (i.e.
xs100_write, xs100_read) instead of the whole block transfer core that also sets up the remote DMA engine and tries to re-initialize the card in
case of unexplained problems.

OK, so essentially change

         if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

to something like

         if (ax->block_read) {
		ax->block_read(dev, buf, count);
	} else if (ei_local->word16) {
                 ioread16_rep(nic_base + NE_DATAPORT, buf, count >> 1);
                 if (count & 0x01)
                         buf[count-1] = ei_inb(nic_base + NE_DATAPORT);

         } else {
                 ioread8_rep(nic_base + NE_DATAPORT, buf, count);
         }

Quoted 2 lines

and populate ax->block_read() and ax_block_write() from platform data, instead of substituting ax_block_input() / ax_block_output() wholesale?

That's basically how I think the whole lib8390.c story should in fact be tackled. Less code duplication, no second include of lib8390 and constrain xsurf100.c to the pieces that make this piece of hardware unique.

Quoted 1 line

I must be missing something here.

I don't think so.

Quoted 4 lines

Adds one test for
ax->block_read on the critical path but we already have the test for ei_local->word16 there. May need rearranging the tests so the majority of ax88796 users isn't impacted.

Rearranging sounds like a good idea. As I understand, the only valid rearrangement is putting it inside the 16-bit branch, because the xs100 uses 16-bit transfers and needs the extra byte for odd counts. The code

checks word16 at the beginning of xs100_block_output for that. This has the advantage of not hurting users of the 8 bit interface, which might be the slowest users of the ax88796, but comes at the cost of not being able to customize the block_input/block_output for 8-bit users. As this "cost" is not a problem now (no one can customize block_input/block_output currently), lets put the block_read check into the word16 block. You might want to name the member block_read16 instead of just block_read to convey the information, that it is only used if word16 is set.

Quoted 1 line

Anyway, your code, your call.

On the other hand: Your polishing, your call. Thank you for your work on gettting the code in good shape for merging.

Kind regards,
Michael Karcher

The block I/O code for the new X-Surf 100 ax88796 driver needs ax_NS8390_init() for error fixup in its block_output function.

Export this function so we can lose the lib8380.c include in the X-Surf 100 driver.

Signed-off-by Michael Schmitz <[email protected]>

Now that ax88796.c exports the ax_NS8390_init() symbol, we can include 8390.h instead of lib8390.c, avoiding duplication of that function and killing a few compile warnings in the bargain.

Signed-off-by Michael Schmitz <[email protected]>

Hi Michael,

Quoted 6 lines · Michael Schmitz

Now that ax88796.c exports the ax_NS8390_init() symbol, we can include 8390.h instead of lib8390.c, avoiding duplication of that function and killing a few compile warnings in the bargain.

Signed-off-by Michael Schmitz <[email protected]>

Thanks for your patch!

Quoted 6 lines

drivers/net/ethernet/8390/xsurf100.c +0 -0

@@ -33,8 +33,6 @@
 #define HW_CHIPID              0x70
 #define HW_SCRATCH             0x78
 

drivers/net/ethernet/8390/xsurf100.c static void reg_write16(void __iomem *base, u16 reg, u16 val)

Quoted 6 lines

drivers/net/ethernet/8390/xsurf100.c +0 -2

-#define __NS8390_init ax_NS8390_init
-
 /* force unsigned long back to 'void __iomem *' */
 #define ax_convert_addr(_a) ((void __force __iomem *)(_a))
 

@@ -80,12 +78,10 @@ static void reg_write16(void __iomem *base, u16 reg, u16 val)

This doesn't apply against net-next, which doesn't have reg_write16() (yet?).

Apart from that, your patch looks fine to me.

Gr{oetje,eeting}s,

Geert

The block I/O code for the new X-Surf 100 ax88796 driver needs ax_NS8390_init() for error fixup in its block_output function.

Export this static function through the ax_NS8390_reinit() wrapper so we can lose the lib8380.c include in the X-Surf 100 driver.

Fixes 861928f4e60e826c ("net-next: New ax88796 platform

driver for Amiga X-Surf 100 Zorro board (m68k)")

Signed-off-by Michael Schmitz <[email protected]>

Now that ax88796.c exports the ax_NS8390_reinit() symbol, we can include 8390.h instead of lib8390.c, avoiding duplication of that function and killing a few compile warnings in the bargain.

Fixes 861928f4e60e826c ("net-next: New ax88796 platform

driver for Amiga X-Surf 100 Zorro board (m68k)")

Signed-off-by Michael Schmitz <[email protected]>

Now that ax88796.c exports the ax_NS8390_reinit() symbol, we can include 8390.h instead of lib8390.c, avoiding duplication of that function and killing a few compile warnings in the bargain.

Fixes 861928f4e60e826c ("net-next: New ax88796 platform

driver for Amiga X-Surf 100 Zorro board (m68k)")

Signed-off-by Michael Schmitz <[email protected]>

---
 drivers/net/ethernet/8390/xsurf100.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

drivers/net/ethernet/8390/xsurf100.c +0 -2

index e2c9638..1c3e8d1 100644
@@ -22,8 +22,6 @@
 #define XS100_8390_DATA_WRITE32_BASE 0x0C80
 #define XS100_8390_DATA_AREA_SIZE 0x80
 
-#define __NS8390_init ax_NS8390_init
-
 /* force unsigned long back to 'void __iomem *' */
 #define ax_convert_addr(_a) ((void __force __iomem *)(_a))
 

Quoted 8 lines

Now that ax88796.c exports the ax_NS8390_reinit() symbol, we can include 8390.h instead of lib8390.c, avoiding duplication of that function and killing a few compile warnings in the bargain.

Fixes 861928f4e60e826c ("net-next: New ax88796 platform

driver for Amiga X-Surf 100 Zorro board (m68k)")

Quoted 4 lines

Signed-off-by Michael Schmitz <[email protected]>

---
 drivers/net/ethernet/8390/xsurf100.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

Geert noticed that a patch I just sent is similar to this one. Since I assume you have verified this version works, it would be nice if you could resend both patches.

Alternatively, I can include them in my series if you like.

Reviewed-by Arnd Bergmann <[email protected]

Quoted 17 lines

drivers/net/ethernet/8390/xsurf100.c +0 -2

index e2c9638..1c3e8d1 100644
@@ -22,8 +22,6 @@
 #define XS100_8390_DATA_WRITE32_BASE 0x0C80
 #define XS100_8390_DATA_AREA_SIZE 0x80
 
-#define __NS8390_init ax_NS8390_init
-
 /* force unsigned long back to 'void __iomem *' */
 #define ax_convert_addr(_a) ((void __force __iomem *)(_a))
 

@@ -42,10 +40,11 @@
 /* Ensure we have our RCR base value */
 #define AX88796_PLATFORM
 

drivers/net/ethernet/8390/xsurf100.c line 44

Quoted 4 lines

drivers/net/ethernet/8390/xsurf100.c +2 -2

-static unsigned char version[] =
-               "ax88796.c: Copyright 2005,2007 Simtec Electronics\n";
+#define NS8390_CORE
+#include "8390.h"

I don't see that #define being referenced anywhere, can that be dropped?

Arnd

Which reminds me to check how far we ever got with testing the XSurf500 driver that's still stuck in my tree.

Quoted 1 line

Alternatively, I can include them in my series if you like.

Please do that - I haven't followed net-next for over a year and don't have a current enough tree to rebase this on.

Quoted 6 lines · Michael Schmitz

Which reminds me to check how far we ever got with testing the XSurf500 driver that's still stuck in my tree.

Quoted 1 line

Alternatively, I can include them in my series if you like.

Please do that - I haven't followed net-next for over a year and don't have a current enough tree to rebase this on.

Ok, done. I'll send the series once net-next opens for 5.14.

Arnd

The Asix Electronics PHY found on the X-Surf 100 Amiga Zorro network card by Individual Computers is buggy, and needs the reset bit toggled as workaround to make a PHY soft reset succed.

drivers/net/phy/asix.c new file +15 -0

+
+/**
+ * asix_soft_reset - software reset the PHY via BMCR_RESET bit
+ * @phydev: target phy_device struct
+ *
+ * Description: Perform a software PHY reset using the standard
+ * BMCR_RESET bit and poll for the reset bit to be cleared.
+ * Toggle BMCR_RESET bit off to accomodate broken PHY implementations
+ * such as used on the Individual Computers' X-Surf 100 Zorro card.
+ *
+ * Returns: 0 on success, < 0 on failure
+ */
+static int asix_soft_reset(struct phy_device *phydev)
+{
+	int ret;

Quoted 15 lines

drivers/net/phy/asix.c new file +15 -0

+
+/**
+ * asix_soft_reset - software reset the PHY via BMCR_RESET bit
+ * @phydev: target phy_device struct
+ *
+ * Description: Perform a software PHY reset using the standard
+ * BMCR_RESET bit and poll for the reset bit to be cleared.
+ * Toggle BMCR_RESET bit off to accomodate broken PHY implementations
+ * such as used on the Individual Computers' X-Surf 100 Zorro card.
+ *
+ * Returns: 0 on success, < 0 on failure
+ */
+static int asix_soft_reset(struct phy_device *phydev)
+{
+     int ret;

drivers/net/phy/asix.c line 48

Quoted 10 lines

drivers/net/phy/asix.c new file +10 -0

+
+     /* Asix PHY won't reset unless reset bit toggles */
+     ret = phy_write(phydev, MII_BMCR, 0);
+     if (ret < 0)
+             return ret;
+
+     phy_write(phydev, MII_BMCR, BMCR_RESET);
+
+     return phy_poll_reset(phydev);
+}

Why not simply:

static int asix_soft_reset(struct phy_device *phydev)

{
        int ret;

        /* Asix PHY won't reset unless reset bit toggles */
        ret = phy_write(phydev, MII_BMCR, 0);
        if (ret < 0)
                return ret;

        return genphy_soft_reset(phydev);
}

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html