Hi Paul,
(Apologies to all for botching the patch format ...)
Regarding your suggestion that netpoll be used instead of a dedicated
timer interrupt: no changes to ne.c or 8390p.c are required to use
netpoll, it all works out of the box. All that is needed to use the
driver with netpoll is setting the device interrupt to some source that
can be registered, and enabling CONFIG_NETPOLL. Interrupt rate and hence
throughput is lower with netpoll though, which is why I still prefer the
dedicated timer option.
How much lower? Enough to matter? Implicit in that question is
the assumption that this is largely a hobbyist platform and nobody
is using it in a closet to route gigabytes of traffic.
I'd say about at least double latency. I can try and measure bulk data
rates if it matters. My gut feeling is latency limits data rates even
when say behind a DSL modem for downloads. It sure did when my Falcon
was still hooked up to a university network, uploading and downloading
source and binary packages for Debian/68k.
Of course you're not routing gigabytes of traffic with this (where to -
a PPP connection? :). Whoever wants minimum latency better reach for the
soldering iron and wire up the interrupt line to some suitable input.
Also, the only advantage to modifying ne.c is to allow dumping
the old driver. What is the "remove soon" plan? Any reason
for it to not be synchronous? That would eliminate the Kconfig
churn and the introduction of the _OLD option. Modifying ne.c
and then deciding to keep the old driver because it is "faster"
would make this change pointless.
As soon as eventual changes to ne.c get accepted. If you want us to drop
the old driver in the same patch, fine by me.
diff --git a/drivers/net/ethernet/8390/8390.h
b/drivers/net/ethernet/8390/8390.h
index ef325ff..9416245 100644
--- a/drivers/net/ethernet/8390/8390.h
+++ b/drivers/net/ethernet/8390/8390.h
@@ -32,6 +32,14 @@ extern void ei_poll(struct net_device *dev);
extern void eip_poll(struct net_device *dev);
#endif
+/* Some platforms may need special IRQ flags */
+#if IS_ENABLED(CONFIG_ATARI_ETHERNEC)
+# define EI_IRQ_FLAGS IRQF_SHARED
+#endif
+
+#ifndef EI_IRQ_FLAGS
+# define EI_IRQ_FLAGS 0
+#endif
This seems more klunky than it needs to be. If we assume that anyone
building ne.c on atari is hence trying to drive an ethernec device
than it can just be
#ifdef CONFIG_ATARI
#define EI_IRQ_FLAGS IRQF_SHARED
#else
#define EI_IRQ_FLAGS 0
#endif
Pretty safe assumption - if we further assume no other arch has reason
to resort to such a kludge, we can simplify it this way.
--- a/drivers/net/ethernet/8390/ne.c
+++ b/drivers/net/ethernet/8390/ne.c
@@ -165,7 +165,8 @@ bad_clone_list[] __initdata = {
#if defined(CONFIG_PLAT_MAPPI)
# define DCR_VAL 0x4b
#elif defined(CONFIG_PLAT_OAKS32R) || \
- defined(CONFIG_MACH_TX49XX)
+ defined(CONFIG_MACH_TX49XX) || \
+ IS_ENABLED(CONFIG_ATARI_ETHERNEC)
Rather than use IS_ENABLED on a driver setting, you can follow
the surrounding context and use defined(CONFIG_ATARI) -- i.e.
work off a platform setting.
True as well, point taken. Is the patch acceptable with these changes?
If so, would you be OK with this going through Geert's tree?
Cheers,
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
