odd RTL8139 quirk.

odd RTL8139 quirk.

[Dave Jones]

I've just been playing with a model 2 OQO, which has an RTL8139.
It gets detected just fine, though it doesn't actually work..

eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
eth0:  Identified 8139 chip type 'RTL-8139'

The null MAC address being one clue.  Another oddity is that
ethtool reports that there's no link detected, even though there is.
(Enough for it to PXE boot a kernel from at least :)

Futzing with the debug= modparam didn't yield anything extra at all.

Any clues?

	Dave

-- 
http://www.codemonkey.org.uk

Re: odd RTL8139 quirk.

[Jeff Garzik]

Dave Jones wrote:
> I've just been playing with a model 2 OQO, which has an RTL8139.
> It gets detected just fine, though it doesn't actually work..
> 
> eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
> eth0:  Identified 8139 chip type 'RTL-8139'
> 
> The null MAC address being one clue.  Another oddity is that
> ethtool reports that there's no link detected, even though there is.
> (Enough for it to PXE boot a kernel from at least :)
> 
> Futzing with the debug= modparam didn't yield anything extra at all.
> 
> Any clues?

Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
'ifconfig ... up' help?

	Jeff

Re: odd RTL8139 quirk.

[Dave Jones]

On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > > I've just been playing with a model 2 OQO, which has an RTL8139.
 > > It gets detected just fine, though it doesn't actually work..
 > > 
 > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
 > > eth0:  Identified 8139 chip type 'RTL-8139'
 > > 
 > > The null MAC address being one clue.  Another oddity is that
 > > ethtool reports that there's no link detected, even though there is.
 > > (Enough for it to PXE boot a kernel from at least :)
 > > 
 > > Futzing with the debug= modparam didn't yield anything extra at all.
 > > 
 > > Any clues?
 > 
 > Sounds like a broken EEPROM.

Not that it counts for much, but the NIC works fine in the vista install
that it came with.

 >  Does supplying a MAC via ifconfig prior to 'ifconfig ... up' help?

That fails spectacularly with 'eth0: PCI Bus error 0290' after I do the 'up'
Someone suggested that the PHY isn't being detected.
I don't get any of the MII messages from rtl8139_init_one()
Which is odd (to me at least) as ethtool reports MII as a supported port,
(and even has it as the 'current' port).

	Dave

-- 
http://www.codemonkey.org.uk

Re: odd RTL8139 quirk.

[Dave Jones]

On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > > I've just been playing with a model 2 OQO, which has an RTL8139.
 > > It gets detected just fine, though it doesn't actually work..
 > > 
 > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
 > > eth0:  Identified 8139 chip type 'RTL-8139'
 > > 
 > > The null MAC address being one clue.  Another oddity is that
 > > ethtool reports that there's no link detected, even though there is.
 > > (Enough for it to PXE boot a kernel from at least :)
 > > 
 > > Futzing with the debug= modparam didn't yield anything extra at all.
 > > 
 > > Any clues?
 > 
 > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
 > 'ifconfig ... up' help?

Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
Would there be any objection to turning that into a modparam ?
(If we wanted to get really fancy, we could even quirk around it automatically
 when we detect broken hardware).

Thoughts?

	Dave

-- 
http://www.codemonkey.org.uk

Re: odd RTL8139 quirk.

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
>  > Dave Jones wrote:
>  > > I've just been playing with a model 2 OQO, which has an RTL8139.
>  > > It gets detected just fine, though it doesn't actually work..
>  > > 
>  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
>  > > eth0:  Identified 8139 chip type 'RTL-8139'
>  > > 
>  > > The null MAC address being one clue.  Another oddity is that
>  > > ethtool reports that there's no link detected, even though there is.
>  > > (Enough for it to PXE boot a kernel from at least :)
>  > > 
>  > > Futzing with the debug= modparam didn't yield anything extra at all.
>  > > 
>  > > Any clues?
>  > 
>  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
>  > 'ifconfig ... up' help?
> 
> Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
> Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
> Would there be any objection to turning that into a modparam ?
> (If we wanted to get really fancy, we could even quirk around it automatically
>  when we detect broken hardware).

Something like this?  :)

http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html

It did not go upstream but it needed some init-time bug fixing, IIRC. 
Maybe akpm remembers more why my patch sucked... :)

	Jeff

Re: odd RTL8139 quirk.

[Dave Jones]

On Tue, Apr 29, 2008 at 06:04:06PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > > On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
 > >  > Dave Jones wrote:
 > >  > > I've just been playing with a model 2 OQO, which has an RTL8139.
 > >  > > It gets detected just fine, though it doesn't actually work..
 > >  > > 
 > >  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
 > >  > > eth0:  Identified 8139 chip type 'RTL-8139'
 > >  > > 
 > >  > > The null MAC address being one clue.  Another oddity is that
 > >  > > ethtool reports that there's no link detected, even though there is.
 > >  > > (Enough for it to PXE boot a kernel from at least :)
 > >  > > 
 > >  > > Futzing with the debug= modparam didn't yield anything extra at all.
 > >  > > 
 > >  > > Any clues?
 > >  > 
 > >  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
 > >  > 'ifconfig ... up' help?
 > > 
 > > Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
 > > Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
 > > Would there be any objection to turning that into a modparam ?
 > > (If we wanted to get really fancy, we could even quirk around it automatically
 > >  when we detect broken hardware).
 > 
 > Something like this?  :)
 > 
 > http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html

>From a quick eyeball, that would seem that it still requires a choice
to be made whether or not to enable CONFIG_8139TOO_PIO
What I had in mind was to make that whole USE_IO_OPS ifdef
(and related bits) if (modparm==.. instead of #ifdefs.

	Dave

-- 
http://www.codemonkey.org.uk

Re: odd RTL8139 quirk.

[Jeff Garzik]

Dave Jones wrote:
>>From a quick eyeball, that would seem that it still requires a choice
> to be made whether or not to enable CONFIG_8139TOO_PIO
> What I had in mind was to make that whole USE_IO_OPS ifdef
> (and related bits) if (modparm==.. instead of #ifdefs.


The patch is the requisite groundwork to add such a module param... 
iomap support is required to enable runtime rather than compile-time 
MMIO/PIO switching.

e100 does this with the 'use_io' module param.

	Jeff

Re: odd RTL8139 quirk.

[Dave Jones]

On Tue, Apr 29, 2008 at 06:19:46PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > >>From a quick eyeball, that would seem that it still requires a choice
 > > to be made whether or not to enable CONFIG_8139TOO_PIO
 > > What I had in mind was to make that whole USE_IO_OPS ifdef
 > > (and related bits) if (modparm==.. instead of #ifdefs.
 > 
 > 
 > The patch is the requisite groundwork to add such a module param... 
 > iomap support is required to enable runtime rather than compile-time 
 > MMIO/PIO switching.
 > 
 > e100 does this with the 'use_io' module param.

Ah, in which case, I endorse this proposal heartily :)

	Dave

-- 
http://www.codemonkey.org.uk

Re: odd RTL8139 quirk.

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Apr 29, 2008 at 06:19:46PM -0400, Jeff Garzik wrote:
>  > Dave Jones wrote:
>  > >>From a quick eyeball, that would seem that it still requires a choice
>  > > to be made whether or not to enable CONFIG_8139TOO_PIO
>  > > What I had in mind was to make that whole USE_IO_OPS ifdef
>  > > (and related bits) if (modparm==.. instead of #ifdefs.
>  > 
>  > 
>  > The patch is the requisite groundwork to add such a module param... 
>  > iomap support is required to enable runtime rather than compile-time 
>  > MMIO/PIO switching.
>  > 
>  > e100 does this with the 'use_io' module param.
> 
> Ah, in which case, I endorse this proposal heartily :)

Is that endorsement perchance accompanied by the time to resurrect the 
patch?  :)

Also, specifically with your hardware, one would hope we could figure 
out some automatic way for the driver to detect the hardware and 
configure it accordingly.

	Jeff

Re: odd RTL8139 quirk.

[Andrew Morton]

On Tue, 29 Apr 2008 18:04:06 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Dave Jones wrote:
> > On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
> >  > Dave Jones wrote:
> >  > > I've just been playing with a model 2 OQO, which has an RTL8139.
> >  > > It gets detected just fine, though it doesn't actually work..
> >  > > 
> >  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
> >  > > eth0:  Identified 8139 chip type 'RTL-8139'
> >  > > 
> >  > > The null MAC address being one clue.  Another oddity is that
> >  > > ethtool reports that there's no link detected, even though there is.
> >  > > (Enough for it to PXE boot a kernel from at least :)
> >  > > 
> >  > > Futzing with the debug= modparam didn't yield anything extra at all.
> >  > > 
> >  > > Any clues?
> >  > 
> >  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
> >  > 'ifconfig ... up' help?
> > 
> > Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
> > Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
> > Would there be any objection to turning that into a modparam ?
> > (If we wanted to get really fancy, we could even quirk around it automatically
> >  when we detect broken hardware).
> 
> Something like this?  :)
> 
> http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html
> 
> It did not go upstream but it needed some init-time bug fixing, IIRC. 
> Maybe akpm remembers more why my patch sucked... :)

Apart from its From: address you mean? ;)

I can find no record, sorry.

Re: odd RTL8139 quirk.

[Jeff Garzik]

Andrew Morton wrote:
> On Tue, 29 Apr 2008 18:04:06 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Dave Jones wrote:
>>> On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
>>>  > Dave Jones wrote:
>>>  > > I've just been playing with a model 2 OQO, which has an RTL8139.
>>>  > > It gets detected just fine, though it doesn't actually work..
>>>  > > 
>>>  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
>>>  > > eth0:  Identified 8139 chip type 'RTL-8139'
>>>  > > 
>>>  > > The null MAC address being one clue.  Another oddity is that
>>>  > > ethtool reports that there's no link detected, even though there is.
>>>  > > (Enough for it to PXE boot a kernel from at least :)
>>>  > > 
>>>  > > Futzing with the debug= modparam didn't yield anything extra at all.
>>>  > > 
>>>  > > Any clues?
>>>  > 
>>>  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
>>>  > 'ifconfig ... up' help?
>>>
>>> Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
>>> Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
>>> Would there be any objection to turning that into a modparam ?
>>> (If we wanted to get really fancy, we could even quirk around it automatically
>>>  when we detect broken hardware).
>> Something like this?  :)
>>
>> http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html
>>
>> It did not go upstream but it needed some init-time bug fixing, IIRC. 
>> Maybe akpm remembers more why my patch sucked... :)
> 
> Apart from its From: address you mean? ;)
> 
> I can find no record, sorry.

Yeah, it was in -mm for a while (via netdev-2.6.git#ALL most likely), 
and a -mm tester reported that it consistently oops for him, or 
something along those lines.

You continually (and rightly!) pestered me about it, and I withdrew the 
patch from -mm since I didn't have time to futz with it.

	Jeff

Re: odd RTL8139 quirk.

[Stephen Hemminger]

On Wed, 30 Apr 2008 07:13:48 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> > On Tue, 29 Apr 2008 18:04:06 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> Dave Jones wrote:
> >>> On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
> >>>  > Dave Jones wrote:
> >>>  > > I've just been playing with a model 2 OQO, which has an RTL8139.
> >>>  > > It gets detected just fine, though it doesn't actually work..
> >>>  > > 
> >>>  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
> >>>  > > eth0:  Identified 8139 chip type 'RTL-8139'
> >>>  > > 
> >>>  > > The null MAC address being one clue.  Another oddity is that
> >>>  > > ethtool reports that there's no link detected, even though there is.
> >>>  > > (Enough for it to PXE boot a kernel from at least :)
> >>>  > > 
> >>>  > > Futzing with the debug= modparam didn't yield anything extra at all.
> >>>  > > 
> >>>  > > Any clues?
> >>>  > 
> >>>  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
> >>>  > 'ifconfig ... up' help?
> >>>
> >>> Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
> >>> Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
> >>> Would there be any objection to turning that into a modparam ?
> >>> (If we wanted to get really fancy, we could even quirk around it automatically
> >>>  when we detect broken hardware).
> >> Something like this?  :)
> >>
> >> http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html
> >>
> >> It did not go upstream but it needed some init-time bug fixing, IIRC. 
> >> Maybe akpm remembers more why my patch sucked... :)
> > 
> > Apart from its From: address you mean? ;)
> > 
> > I can find no record, sorry.
> 
> Yeah, it was in -mm for a while (via netdev-2.6.git#ALL most likely), 
> and a -mm tester reported that it consistently oops for him, or 
> something along those lines.
> 
> You continually (and rightly!) pestered me about it, and I withdrew the 
> patch from -mm since I didn't have time to futz with it.

Is there any possibility for the driver to discover broken MMIO at runtime?
Could it look for some portion like mac address or HW_REVID being all zeros or all ones?

Re: odd RTL8139 quirk.

[Dave Jones]

On Wed, Apr 30, 2008 at 07:13:48AM -0400, Jeff Garzik wrote:
 > Andrew Morton wrote:
 > > On Tue, 29 Apr 2008 18:04:06 -0400
 > > Jeff Garzik <jeff@garzik.org> wrote:
 > > 
 > >> Dave Jones wrote:
 > >>> On Tue, Apr 29, 2008 at 03:37:41PM -0400, Jeff Garzik wrote:
 > >>>  > Dave Jones wrote:
 > >>>  > > I've just been playing with a model 2 OQO, which has an RTL8139.
 > >>>  > > It gets detected just fine, though it doesn't actually work..
 > >>>  > > 
 > >>>  > > eth0: RealTek RTL8139 at 0xf8830000, 00:00:00:00:00:00, IRQ 18
 > >>>  > > eth0:  Identified 8139 chip type 'RTL-8139'
 > >>>  > > 
 > >>>  > > The null MAC address being one clue.  Another oddity is that
 > >>>  > > ethtool reports that there's no link detected, even though there is.
 > >>>  > > (Enough for it to PXE boot a kernel from at least :)
 > >>>  > > 
 > >>>  > > Futzing with the debug= modparam didn't yield anything extra at all.
 > >>>  > > 
 > >>>  > > Any clues?
 > >>>  > 
 > >>>  > Sounds like a broken EEPROM.  Does supplying a MAC via ifconfig prior to 
 > >>>  > 'ifconfig ... up' help?
 > >>>
 > >>> Ah. This sounds enlightening: http://www.oqotalk.com/index.php/topic,1511.0.html
 > >>> Seems a shame to have to choose PIO vs MMIO for a distro kernel though.
 > >>> Would there be any objection to turning that into a modparam ?
 > >>> (If we wanted to get really fancy, we could even quirk around it automatically
 > >>>  when we detect broken hardware).
 > >> Something like this?  :)
 > >>
 > >> http://www.linux.sgi.com/archives/netdev/2004-11/msg00226.html
 > >>
 > >> It did not go upstream but it needed some init-time bug fixing, IIRC. 
 > >> Maybe akpm remembers more why my patch sucked... :)
 > > 
 > > Apart from its From: address you mean? ;)
 > > 
 > > I can find no record, sorry.
 > 
 > Yeah, it was in -mm for a while (via netdev-2.6.git#ALL most likely), 
 > and a -mm tester reported that it consistently oops for him, or 
 > something along those lines.
 > 
 > You continually (and rightly!) pestered me about it, and I withdrew the 
 > patch from -mm since I didn't have time to futz with it.

So I got back to looking at this problem yesterday.
Turns out the patch you reference above was merged back in 2005 :-)
See commit 22f714b64b55012fa4e0d77132fa82719180f994

On top of this, it turns out to be really easy to make this a runtime
modparam.  I now have the OQO2 up and running with working ethernet.
Patches to follow.

	Dave

-- 
http://www.codemonkey.org.uk

[PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

Make PIO/MMIO a runtime thing via a module parameter.
This is needed to support devices that only work with PIO
without penalising devices that work fine with MMIO in
distro kernels.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 53bd903..09ace39 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -120,11 +120,6 @@
                                  NETIF_MSG_LINK)
 
 
-/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
-#ifdef CONFIG_8139TOO_PIO
-#define USE_IO_OPS 1
-#endif
-
 /* define to 1, 2 or 3 to enable copious debugging info */
 #define RTL8139_DEBUG 0
 
@@ -156,6 +151,9 @@
 static int media[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 static int full_duplex[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 
+/* Whether to use MMIO or PIO. Default to MMIO. */
+static int use_pio;
+
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
@@ -615,6 +613,7 @@ MODULE_DESCRIPTION ("RealTek RTL-8139 Fast Ethernet driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+module_param(use_pio, int, 0);
 module_param(multicast_filter_limit, int, 0);
 module_param_array(media, int, NULL, 0);
 module_param_array(full_duplex, int, NULL, 0);
@@ -710,14 +709,13 @@ static void __rtl8139_cleanup_dev (struct net_device *dev)
 	assert (tp->pci_dev != NULL);
 	pdev = tp->pci_dev;
 
-#ifdef USE_IO_OPS
-	if (tp->mmio_addr)
-		ioport_unmap (tp->mmio_addr);
-#else
-	if (tp->mmio_addr)
-		pci_iounmap (pdev, tp->mmio_addr);
-#endif /* USE_IO_OPS */
-
+	if (use_pio) {
+		if (tp->mmio_addr)
+			ioport_unmap (tp->mmio_addr);
+	} else {
+		if (tp->mmio_addr)
+			pci_iounmap (pdev, tp->mmio_addr);
+	}
 	/* it's ok to call this even if we have no regions to free */
 	pci_release_regions (pdev);
 
@@ -791,32 +789,32 @@ static int __devinit rtl8139_init_board (struct pci_dev *pdev,
 	DPRINTK("PIO region size == 0x%02X\n", pio_len);
 	DPRINTK("MMIO region size == 0x%02lX\n", mmio_len);
 
-#ifdef USE_IO_OPS
-	/* make sure PCI base addr 0 is PIO */
-	if (!(pio_flags & IORESOURCE_IO)) {
-		dev_err(&pdev->dev, "region #0 not a PIO resource, aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-	/* check for weird/broken PCI region reporting */
-	if (pio_len < RTL_MIN_IO_SIZE) {
-		dev_err(&pdev->dev, "Invalid PCI I/O region size(s), aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-#else
-	/* make sure PCI base addr 1 is MMIO */
-	if (!(mmio_flags & IORESOURCE_MEM)) {
-		dev_err(&pdev->dev, "region #1 not an MMIO resource, aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-	if (mmio_len < RTL_MIN_IO_SIZE) {
-		dev_err(&pdev->dev, "Invalid PCI mem region size(s), aborting\n");
-		rc = -ENODEV;
-		goto err_out;
+	if (use_pio) {
+		/* make sure PCI base addr 0 is PIO */
+		if (!(pio_flags & IORESOURCE_IO)) {
+			dev_err(&pdev->dev, "region #0 not a PIO resource, aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+		/* check for weird/broken PCI region reporting */
+		if (pio_len < RTL_MIN_IO_SIZE) {
+			dev_err(&pdev->dev, "Invalid PCI I/O region size(s), aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+	} else {
+		/* make sure PCI base addr 1 is MMIO */
+		if (!(mmio_flags & IORESOURCE_MEM)) {
+			dev_err(&pdev->dev, "region #1 not an MMIO resource, aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+		if (mmio_len < RTL_MIN_IO_SIZE) {
+			dev_err(&pdev->dev, "Invalid PCI mem region size(s), aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
 	}
-#endif
 
 	rc = pci_request_regions (pdev, DRV_NAME);
 	if (rc)
@@ -826,28 +824,28 @@ static int __devinit rtl8139_init_board (struct pci_dev *pdev,
 	/* enable PCI bus-mastering */
 	pci_set_master (pdev);
 
-#ifdef USE_IO_OPS
-	ioaddr = ioport_map(pio_start, pio_len);
-	if (!ioaddr) {
-		dev_err(&pdev->dev, "cannot map PIO, aborting\n");
-		rc = -EIO;
-		goto err_out;
-	}
-	dev->base_addr = pio_start;
-	tp->mmio_addr = ioaddr;
-	tp->regs_len = pio_len;
-#else
-	/* ioremap MMIO region */
-	ioaddr = pci_iomap(pdev, 1, 0);
-	if (ioaddr == NULL) {
-		dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
-		rc = -EIO;
-		goto err_out;
+	if (use_pio) {
+		ioaddr = ioport_map(pio_start, pio_len);
+		if (!ioaddr) {
+			dev_err(&pdev->dev, "cannot map PIO, aborting\n");
+			rc = -EIO;
+			goto err_out;
+		}
+		dev->base_addr = pio_start;
+		tp->mmio_addr = ioaddr;
+		tp->regs_len = pio_len;
+	} else {
+		/* ioremap MMIO region */
+		ioaddr = pci_iomap(pdev, 1, 0);
+		if (ioaddr == NULL) {
+			dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
+			rc = -EIO;
+			goto err_out;
+		}
+		dev->base_addr = (long) ioaddr;
+		tp->mmio_addr = ioaddr;
+		tp->regs_len = mmio_len;
 	}
-	dev->base_addr = (long) ioaddr;
-	tp->mmio_addr = ioaddr;
-	tp->regs_len = mmio_len;
-#endif /* USE_IO_OPS */
 
 	/* Bring old chips out of low-power mode. */
 	RTL_W8 (HltClk, 'R');
@@ -2383,20 +2381,24 @@ static void rtl8139_set_msglevel(struct net_device *dev, u32 datum)
 	np->msg_enable = datum;
 }
 
-/* TODO: we are too slack to do reg dumping for pio, for now */
-#ifdef CONFIG_8139TOO_PIO
-#define rtl8139_get_regs_len	NULL
-#define rtl8139_get_regs	NULL
-#else
 static int rtl8139_get_regs_len(struct net_device *dev)
 {
-	struct rtl8139_private *np = netdev_priv(dev);
+	struct rtl8139_private *np;
+	/* TODO: we are too slack to do reg dumping for pio, for now */
+	if (use_pio)
+		return 0;
+	np = netdev_priv(dev);
 	return np->regs_len;
 }
 
 static void rtl8139_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *regbuf)
 {
-	struct rtl8139_private *np = netdev_priv(dev);
+	struct rtl8139_private *np;
+
+	/* TODO: we are too slack to do reg dumping for pio, for now */
+	if (use_pio)
+		return;
+	np = netdev_priv(dev);
 
 	regs->version = RTL_REGS_VER;
 
@@ -2404,7 +2406,6 @@ static void rtl8139_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 	memcpy_fromio(regbuf, np->mmio_addr, regs->len);
 	spin_unlock_irq(&np->lock);
 }
-#endif /* CONFIG_8139TOO_MMIO */
 
 static int rtl8139_get_sset_count(struct net_device *dev, int sset)
 {
@@ -2610,6 +2611,11 @@ static int __init rtl8139_init_module (void)
 	printk (KERN_INFO RTL8139_DRIVER_NAME "\n");
 #endif
 
+	/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
+#ifdef CONFIG_8139TOO_PIO
+	use_pio = 1;
+#endif
+
 	return pci_register_driver(&rtl8139_pci_driver);
 }
 

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> Make PIO/MMIO a runtime thing via a module parameter.
> This is needed to support devices that only work with PIO
> without penalising devices that work fine with MMIO in
> distro kernels.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

two comments:

1) should use pci_iomap() for both PIO and MMIO

2) modparam should be "use_io" mirroring existing drivers

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Thu, May 29, 2008 at 02:18:45PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > > Make PIO/MMIO a runtime thing via a module parameter.
 > > This is needed to support devices that only work with PIO
 > > without penalising devices that work fine with MMIO in
 > > distro kernels.
 > > 
 > > Signed-off-by: Dave Jones <davej@redhat.com>
 > 
 > two comments:
 > 
 > 1) should use pci_iomap() for both PIO and MMIO
 > 
 > 2) modparam should be "use_io" mirroring existing drivers

Ok, I'll take a stab at those later today (and also your comment
from the other mail).

One other thing: I wasn't sure about my changes to rtl8139_get_regs
and rtl8139_get_regs_len.  Is what I did there safe?
Obviously actually implementing support for PIO reg dumping would
be better, but I think that's beyond the scope of what I'm trying
to do for now.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> On Thu, May 29, 2008 at 02:18:45PM -0400, Jeff Garzik wrote:
>  > Dave Jones wrote:
>  > > Make PIO/MMIO a runtime thing via a module parameter.
>  > > This is needed to support devices that only work with PIO
>  > > without penalising devices that work fine with MMIO in
>  > > distro kernels.
>  > > 
>  > > Signed-off-by: Dave Jones <davej@redhat.com>
>  > 
>  > two comments:
>  > 
>  > 1) should use pci_iomap() for both PIO and MMIO
>  > 
>  > 2) modparam should be "use_io" mirroring existing drivers
> 
> Ok, I'll take a stab at those later today (and also your comment
> from the other mail).
> 
> One other thing: I wasn't sure about my changes to rtl8139_get_regs
> and rtl8139_get_regs_len.  Is what I did there safe?
> Obviously actually implementing support for PIO reg dumping would
> be better, but I think that's beyond the scope of what I'm trying
> to do for now.

Yeah, that should be reasonable

	Jeff

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Thu, May 29, 2008 at 03:01:35PM -0400, Jeff Garzik wrote:
 > Dave Jones wrote:
 > > On Thu, May 29, 2008 at 02:18:45PM -0400, Jeff Garzik wrote:
 > >  > Dave Jones wrote:
 > >  > > Make PIO/MMIO a runtime thing via a module parameter.
 > >  > > This is needed to support devices that only work with PIO
 > >  > > without penalising devices that work fine with MMIO in
 > >  > > distro kernels.
 > >  > > 
 > >  > > Signed-off-by: Dave Jones <davej@redhat.com>
 > >  > 
 > >  > two comments:
 > >  > 
 > >  > 1) should use pci_iomap() for both PIO and MMIO
 > >  > 
 > >  > 2) modparam should be "use_io" mirroring existing drivers
 > > 
 > > Ok, I'll take a stab at those later today (and also your comment
 > > from the other mail).
 > > 
 > > One other thing: I wasn't sure about my changes to rtl8139_get_regs
 > > and rtl8139_get_regs_len.  Is what I did there safe?
 > > Obviously actually implementing support for PIO reg dumping would
 > > be better, but I think that's beyond the scope of what I'm trying
 > > to do for now.
 > 
 > Yeah, that should be reasonable

Finally got back to this.  How does this look?

	Dave



Make PIO/MMIO a runtime thing via a module parameter.
This is needed to support devices that only work with PIO
without penalising devices that work fine with MMIO in
distro kernels.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 53bd903..b620317 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -98,7 +98,6 @@
 #include <linux/compiler.h>
 #include <linux/pci.h>
 #include <linux/init.h>
-#include <linux/ioport.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/rtnetlink.h>
@@ -120,11 +119,6 @@
                                  NETIF_MSG_LINK)
 
 
-/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
-#ifdef CONFIG_8139TOO_PIO
-#define USE_IO_OPS 1
-#endif
-
 /* define to 1, 2 or 3 to enable copious debugging info */
 #define RTL8139_DEBUG 0
 
@@ -156,6 +150,9 @@
 static int media[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 static int full_duplex[MAX_UNITS] = {-1, -1, -1, -1, -1, -1, -1, -1};
 
+/* Whether to use MMIO or PIO. Default to MMIO. */
+static int use_io;
+
 /* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
@@ -615,6 +612,7 @@ MODULE_DESCRIPTION ("RealTek RTL-8139 Fast Ethernet driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+module_param(use_io, int, 0);
 module_param(multicast_filter_limit, int, 0);
 module_param_array(media, int, NULL, 0);
 module_param_array(full_duplex, int, NULL, 0);
@@ -710,13 +708,8 @@ static void __rtl8139_cleanup_dev (struct net_device *dev)
 	assert (tp->pci_dev != NULL);
 	pdev = tp->pci_dev;
 
-#ifdef USE_IO_OPS
-	if (tp->mmio_addr)
-		ioport_unmap (tp->mmio_addr);
-#else
 	if (tp->mmio_addr)
 		pci_iounmap (pdev, tp->mmio_addr);
-#endif /* USE_IO_OPS */
 
 	/* it's ok to call this even if we have no regions to free */
 	pci_release_regions (pdev);
@@ -791,32 +784,32 @@ static int __devinit rtl8139_init_board (struct pci_dev *pdev,
 	DPRINTK("PIO region size == 0x%02X\n", pio_len);
 	DPRINTK("MMIO region size == 0x%02lX\n", mmio_len);
 
-#ifdef USE_IO_OPS
-	/* make sure PCI base addr 0 is PIO */
-	if (!(pio_flags & IORESOURCE_IO)) {
-		dev_err(&pdev->dev, "region #0 not a PIO resource, aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-	/* check for weird/broken PCI region reporting */
-	if (pio_len < RTL_MIN_IO_SIZE) {
-		dev_err(&pdev->dev, "Invalid PCI I/O region size(s), aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-#else
-	/* make sure PCI base addr 1 is MMIO */
-	if (!(mmio_flags & IORESOURCE_MEM)) {
-		dev_err(&pdev->dev, "region #1 not an MMIO resource, aborting\n");
-		rc = -ENODEV;
-		goto err_out;
-	}
-	if (mmio_len < RTL_MIN_IO_SIZE) {
-		dev_err(&pdev->dev, "Invalid PCI mem region size(s), aborting\n");
-		rc = -ENODEV;
-		goto err_out;
+	if (use_io) {
+		/* make sure PCI base addr 0 is PIO */
+		if (!(pio_flags & IORESOURCE_IO)) {
+			dev_err(&pdev->dev, "region #0 not a PIO resource, aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+		/* check for weird/broken PCI region reporting */
+		if (pio_len < RTL_MIN_IO_SIZE) {
+			dev_err(&pdev->dev, "Invalid PCI I/O region size(s), aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+	} else {
+		/* make sure PCI base addr 1 is MMIO */
+		if (!(mmio_flags & IORESOURCE_MEM)) {
+			dev_err(&pdev->dev, "region #1 not an MMIO resource, aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
+		if (mmio_len < RTL_MIN_IO_SIZE) {
+			dev_err(&pdev->dev, "Invalid PCI mem region size(s), aborting\n");
+			rc = -ENODEV;
+			goto err_out;
+		}
 	}
-#endif
 
 	rc = pci_request_regions (pdev, DRV_NAME);
 	if (rc)
@@ -826,28 +819,27 @@ static int __devinit rtl8139_init_board (struct pci_dev *pdev,
 	/* enable PCI bus-mastering */
 	pci_set_master (pdev);
 
-#ifdef USE_IO_OPS
-	ioaddr = ioport_map(pio_start, pio_len);
-	if (!ioaddr) {
-		dev_err(&pdev->dev, "cannot map PIO, aborting\n");
-		rc = -EIO;
-		goto err_out;
-	}
-	dev->base_addr = pio_start;
-	tp->mmio_addr = ioaddr;
-	tp->regs_len = pio_len;
-#else
-	/* ioremap MMIO region */
-	ioaddr = pci_iomap(pdev, 1, 0);
-	if (ioaddr == NULL) {
-		dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
-		rc = -EIO;
-		goto err_out;
+	if (use_io) {
+		ioaddr = pci_iomap(pdev, 0, 0);
+		if (!ioaddr) {
+			dev_err(&pdev->dev, "cannot map PIO, aborting\n");
+			rc = -EIO;
+			goto err_out;
+		}
+		dev->base_addr = pio_start;
+		tp->regs_len = pio_len;
+	} else {
+		/* ioremap MMIO region */
+		ioaddr = pci_iomap(pdev, 1, 0);
+		if (ioaddr == NULL) {
+			dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
+			rc = -EIO;
+			goto err_out;
+		}
+		dev->base_addr = (long) ioaddr;
+		tp->regs_len = mmio_len;
 	}
-	dev->base_addr = (long) ioaddr;
 	tp->mmio_addr = ioaddr;
-	tp->regs_len = mmio_len;
-#endif /* USE_IO_OPS */
 
 	/* Bring old chips out of low-power mode. */
 	RTL_W8 (HltClk, 'R');
@@ -2383,20 +2375,24 @@ static void rtl8139_set_msglevel(struct net_device *dev, u32 datum)
 	np->msg_enable = datum;
 }
 
-/* TODO: we are too slack to do reg dumping for pio, for now */
-#ifdef CONFIG_8139TOO_PIO
-#define rtl8139_get_regs_len	NULL
-#define rtl8139_get_regs	NULL
-#else
 static int rtl8139_get_regs_len(struct net_device *dev)
 {
-	struct rtl8139_private *np = netdev_priv(dev);
+	struct rtl8139_private *np;
+	/* TODO: we are too slack to do reg dumping for pio, for now */
+	if (use_io)
+		return 0;
+	np = netdev_priv(dev);
 	return np->regs_len;
 }
 
 static void rtl8139_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *regbuf)
 {
-	struct rtl8139_private *np = netdev_priv(dev);
+	struct rtl8139_private *np;
+
+	/* TODO: we are too slack to do reg dumping for pio, for now */
+	if (use_io)
+		return;
+	np = netdev_priv(dev);
 
 	regs->version = RTL_REGS_VER;
 
@@ -2404,7 +2400,6 @@ static void rtl8139_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 	memcpy_fromio(regbuf, np->mmio_addr, regs->len);
 	spin_unlock_irq(&np->lock);
 }
-#endif /* CONFIG_8139TOO_MMIO */
 
 static int rtl8139_get_sset_count(struct net_device *dev, int sset)
 {
@@ -2610,6 +2605,11 @@ static int __init rtl8139_init_module (void)
 	printk (KERN_INFO RTL8139_DRIVER_NAME "\n");
 #endif
 
+	/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
+#ifdef CONFIG_8139TOO_PIO
+	use_io = 1;
+#endif
+
 	return pci_register_driver(&rtl8139_pci_driver);
 }
 




-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> On Thu, May 29, 2008 at 03:01:35PM -0400, Jeff Garzik wrote:
>  > Dave Jones wrote:
>  > > On Thu, May 29, 2008 at 02:18:45PM -0400, Jeff Garzik wrote:
>  > >  > Dave Jones wrote:
>  > >  > > Make PIO/MMIO a runtime thing via a module parameter.
>  > >  > > This is needed to support devices that only work with PIO
>  > >  > > without penalising devices that work fine with MMIO in
>  > >  > > distro kernels.
>  > >  > > 
>  > >  > > Signed-off-by: Dave Jones <davej@redhat.com>
>  > >  > 
>  > >  > two comments:
>  > >  > 
>  > >  > 1) should use pci_iomap() for both PIO and MMIO
>  > >  > 
>  > >  > 2) modparam should be "use_io" mirroring existing drivers
>  > > 
>  > > Ok, I'll take a stab at those later today (and also your comment
>  > > from the other mail).
>  > > 
>  > > One other thing: I wasn't sure about my changes to rtl8139_get_regs
>  > > and rtl8139_get_regs_len.  Is what I did there safe?
>  > > Obviously actually implementing support for PIO reg dumping would
>  > > be better, but I think that's beyond the scope of what I'm trying
>  > > to do for now.
>  > 
>  > Yeah, that should be reasonable
> 
> Finally got back to this.  How does this look?
> 
> 	Dave
> 
> 
> 
> Make PIO/MMIO a runtime thing via a module parameter.
> This is needed to support devices that only work with PIO
> without penalising devices that work fine with MMIO in
> distro kernels.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Looks great overall.

Minor nits:

* need module param text description

* [optional] if code not too ugly, change mod param description based on 
CONFIG_8139TOO_PIO to indicate the currently compiled default

* [optional] would prefer CONFIG_8139TOO_PIO be handled at compile time, 
by changing the initialized value

* [extra project] would be highly useful for MMIO to fall back to PIO, 
and vice versa, should any resource be unavailable.  Sometimes, mainly 
with MMIO and broken/weird BIOSen, only the PIO PCI BARs will be filled 
in with useful info.

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Tue, Jul 15, 2008 at 06:18:30PM -0400, Jeff Garzik wrote:
 
 > > Make PIO/MMIO a runtime thing via a module parameter.
 > > This is needed to support devices that only work with PIO
 > > without penalising devices that work fine with MMIO in
 > > distro kernels.
 > > 
 > > Signed-off-by: Dave Jones <davej@redhat.com>
 > 
 > Looks great overall.
 > 
 > Minor nits:
 > 
 > * need module param text description

oops. will fix.
 
 > * [optional] if code not too ugly, change mod param description based on 
 > CONFIG_8139TOO_PIO to indicate the currently compiled default

Not sure of a non-icky way to do this other than ifdefs.
The best I could come up with is.

#ifdef CONFIG_8139TOO_PIO
MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO default=PIO"); 
#else
MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO default=MMIO"); 
#endif

palatable?

 > * [optional] would prefer CONFIG_8139TOO_PIO be handled at compile time, 
 > by changing the initialized value

This bit should be taken care of in rtl8139_init_module() at the bottom..

+       /* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
+#ifdef CONFIG_8139TOO_PIO
+       use_io = 1;
+#endif

 > * [extra project] would be highly useful for MMIO to fall back to PIO, 
 > and vice versa, should any resource be unavailable.  Sometimes, mainly 
 > with MMIO and broken/weird BIOSen, only the PIO PCI BARs will be filled 
 > in with useful info.

Sounds do-able. I'll add it to my rainy-day project list.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Jul 15, 2008 at 06:18:30PM -0400, Jeff Garzik wrote:
>  
>  > > Make PIO/MMIO a runtime thing via a module parameter.
>  > > This is needed to support devices that only work with PIO
>  > > without penalising devices that work fine with MMIO in
>  > > distro kernels.
>  > > 
>  > > Signed-off-by: Dave Jones <davej@redhat.com>
>  > 
>  > Looks great overall.
>  > 
>  > Minor nits:
>  > 
>  > * need module param text description
> 
> oops. will fix.
>  
>  > * [optional] if code not too ugly, change mod param description based on 
>  > CONFIG_8139TOO_PIO to indicate the currently compiled default
> 
> Not sure of a non-icky way to do this other than ifdefs.
> The best I could come up with is.
> 
> #ifdef CONFIG_8139TOO_PIO
> MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO default=PIO"); 
> #else
> MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO default=MMIO"); 
> #endif
> 
> palatable?

best you can do AFAIK, so yes


>  > * [optional] would prefer CONFIG_8139TOO_PIO be handled at compile time, 
>  > by changing the initialized value
> 
> This bit should be taken care of in rtl8139_init_module() at the bottom..
> 
> +       /* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
> +#ifdef CONFIG_8139TOO_PIO
> +       use_io = 1;
> +#endif

Right, I was saying I prefer simply to set the initialized value 
appropriately, and avoid the extra code dollup during module load.


>  > * [extra project] would be highly useful for MMIO to fall back to PIO, 
>  > and vice versa, should any resource be unavailable.  Sometimes, mainly 
>  > with MMIO and broken/weird BIOSen, only the PIO PCI BARs will be filled 
>  > in with useful info.
> 
> Sounds do-able. I'll add it to my rainy-day project list.

Thanks :)  A few users out in the wild will definitely thank you.

	Jeff

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Tue, Jul 15, 2008 at 06:55:42PM -0400, Jeff Garzik wrote:

 > >  > * [optional] would prefer CONFIG_8139TOO_PIO be handled at compile time, 
 > >  > by changing the initialized value
 > > 
 > > This bit should be taken care of in rtl8139_init_module() at the bottom..
 > > 
 > > +       /* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
 > > +#ifdef CONFIG_8139TOO_PIO
 > > +       use_io = 1;
 > > +#endif
 > 
 > Right, I was saying I prefer simply to set the initialized value 
 > appropriately, and avoid the extra code dollup during module load.

Oh, you mean something like..

static int use_io = CONFIG_8139TOO_PIO;

?

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Jul 15, 2008 at 06:55:42PM -0400, Jeff Garzik wrote:
> 
>  > >  > * [optional] would prefer CONFIG_8139TOO_PIO be handled at compile time, 
>  > >  > by changing the initialized value
>  > > 
>  > > This bit should be taken care of in rtl8139_init_module() at the bottom..
>  > > 
>  > > +       /* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
>  > > +#ifdef CONFIG_8139TOO_PIO
>  > > +       use_io = 1;
>  > > +#endif
>  > 
>  > Right, I was saying I prefer simply to set the initialized value 
>  > appropriately, and avoid the extra code dollup during module load.
> 
> Oh, you mean something like..
> 
> static int use_io = CONFIG_8139TOO_PIO;
> 
> ?

Sorry for not being specific.  I would have guessed the uglier

	#ifdef CONFIG_8139TOO_PIO
		static int use_io = 1;
	#else
		static int use_io;
	#endif

since CONFIG_8139TOO_PIO doesn't convert to a 1 or 0 neatly in C AFAIK?


But, overall, yes.  You get the general idea.

	Jeff

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Tue, Jul 15, 2008 at 07:31:16PM -0400, Jeff Garzik wrote:
 > Sorry for not being specific.  I would have guessed the uglier
 > 
 > 	#ifdef CONFIG_8139TOO_PIO
 > 		static int use_io = 1;
 > 	#else
 > 		static int use_io;
 > 	#endif
 > 
 > since CONFIG_8139TOO_PIO doesn't convert to a 1 or 0 neatly in C AFAIK?

Hm, yeah. that only works in the PIO=1 case. When not set, it doesn't
end up in autoconf.h.  I'll go with the fugly ifdefs.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Peter Korsgaard]

>>>>> "Jeff" == Jeff Garzik <jeff@garzik.org> writes:

 >> > * [optional] if code not too ugly, change mod param description
 >> based on  > CONFIG_8139TOO_PIO to indicate the currently compiled
 >> default
 >> 
 >> Not sure of a non-icky way to do this other than ifdefs.
 >> The best I could come up with is.
 >> 
 >> #ifdef CONFIG_8139TOO_PIO
 >> MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO
 >> default=PIO"); #else
 >> MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO
 >> default=MMIO"); #endif
 >> 
 >> palatable?

 Jeff> best you can do AFAIK, so yes

Something like:
#ifdef CONFIG_8139TOO_PIO
#define DEFAULTMODE "PIO"
#else
#define DEFAULTMODE "MMIO"
#endif

MODULE_PARM_DESC(use_io, "PIO/MMIO switch.  0=MMIO 1=PIO default=" DEFAULTMODE);

would be nicer.

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Tue, Jul 15, 2008 at 06:18:30PM -0400, Jeff Garzik wrote:

 > * [extra project] would be highly useful for MMIO to fall back to PIO, 
 > and vice versa, should any resource be unavailable.  Sometimes, mainly 
 > with MMIO and broken/weird BIOSen, only the PIO PCI BARs will be filled 
 > in with useful info.

Hmm, this bit might actually be fairly trivial on top of my other patch..


--- linux-2.6.26.noarch/drivers/net/8139too.c~	2008-07-15 18:49:02.000000000 -0400
+++ linux-2.6.26.noarch/drivers/net/8139too.c	2008-07-15 18:53:07.000000000 -0400
@@ -784,6 +784,7 @@ static int __devinit rtl8139_init_board 
 	DPRINTK("PIO region size == 0x%02X\n", pio_len);
 	DPRINTK("MMIO region size == 0x%02lX\n", mmio_len);
 
+retry:
 	if (use_io) {
 		/* make sure PCI base addr 0 is PIO */
 		if (!(pio_flags & IORESOURCE_IO)) {
@@ -832,9 +833,10 @@ static int __devinit rtl8139_init_board 
 		/* ioremap MMIO region */
 		ioaddr = pci_iomap(pdev, 1, 0);
 		if (ioaddr == NULL) {
-			dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
-			rc = -EIO;
-			goto err_out;
+			dev_err(&pdev->dev, "cannot remap MMIO, trying PIO\n");
+			pci_release_regions(pdev);
+			use_ui = 1;
+			goto retry;
 		}
 		dev->base_addr = (long) ioaddr;
 		tp->regs_len = mmio_len;

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Jul 15, 2008 at 06:18:30PM -0400, Jeff Garzik wrote:
> 
>  > * [extra project] would be highly useful for MMIO to fall back to PIO, 
>  > and vice versa, should any resource be unavailable.  Sometimes, mainly 
>  > with MMIO and broken/weird BIOSen, only the PIO PCI BARs will be filled 
>  > in with useful info.
> 
> Hmm, this bit might actually be fairly trivial on top of my other patch..
> 
> 
> --- linux-2.6.26.noarch/drivers/net/8139too.c~	2008-07-15 18:49:02.000000000 -0400
> +++ linux-2.6.26.noarch/drivers/net/8139too.c	2008-07-15 18:53:07.000000000 -0400
> @@ -784,6 +784,7 @@ static int __devinit rtl8139_init_board 
>  	DPRINTK("PIO region size == 0x%02X\n", pio_len);
>  	DPRINTK("MMIO region size == 0x%02lX\n", mmio_len);
>  
> +retry:
>  	if (use_io) {
>  		/* make sure PCI base addr 0 is PIO */
>  		if (!(pio_flags & IORESOURCE_IO)) {
> @@ -832,9 +833,10 @@ static int __devinit rtl8139_init_board 
>  		/* ioremap MMIO region */
>  		ioaddr = pci_iomap(pdev, 1, 0);
>  		if (ioaddr == NULL) {
> -			dev_err(&pdev->dev, "cannot remap MMIO, aborting\n");
> -			rc = -EIO;
> -			goto err_out;
> +			dev_err(&pdev->dev, "cannot remap MMIO, trying PIO\n");
> +			pci_release_regions(pdev);
> +			use_ui = 1;
> +			goto retry;

use_ui?  Are you sure this is not not an X.org patch?  :)

Yes, seems like that would do the trick quite nicely...

If you would be kind enough to post this as a separate patch from your 
use_io patch (since IMO its a separate logical change), that would be 
helpful.

	Jeff

Re: [PATCH 1/2] 8139too: Make PIO/MMIO a modparam

[Dave Jones]

On Tue, Jul 15, 2008 at 07:03:50PM -0400, Jeff Garzik wrote:
 
 > > +			pci_release_regions(pdev);
 > > +			use_ui = 1;
 > > +			goto retry;
 > 
 > use_ui?  Are you sure this is not not an X.org patch?  :)

heh, who needs compilers ?

 > Yes, seems like that would do the trick quite nicely...
 > 
 > If you would be kind enough to post this as a separate patch from your 
 > use_io patch (since IMO its a separate logical change), that would be 
 > helpful.

Sure, I'll repost the series in a bit.

	Dave

-- 
http://www.codemonkey.org.uk

[PATCH 2/2] 8139too: Make the OQO2 automatically use PIO mode.

[Dave Jones]

The OQO model 2 has an RTL8139 from Atheros that doesn't like MMIO.
Force it to always use polled IO.

Signed-off-by: Dave Jones <davej@redhat.com>

--- linux-2.6.25.noarch/drivers/net/8139too.c~	2008-05-28 21:07:36.000000000 -0400
+++ linux-2.6.25.noarch/drivers/net/8139too.c	2008-05-28 21:19:57.000000000 -0400
@@ -951,6 +951,14 @@ static int __devinit rtl8139_init_one (s
 			   "Use the \"8139cp\" driver for improved performance and stability.\n");
 	}
 
+	if (pdev->vendor == PCI_VENDOR_ID_REALTEK &&
+	    pdev->device == PCI_DEVICE_ID_REALTEK_8139 &&
+	    pdev->subsystem_vendor == PCI_VENDOR_ID_ATHEROS &&
+	    pdev->subsystem_device == PCI_DEVICE_ID_REALTEK_8139) {
+		printk(KERN_INFO "8139too: OQO Model 2 detected. Forcing PIO\n");
+		use_pio = 1;
+	}
+
 	i = rtl8139_init_board (pdev, &dev);
 	if (i < 0)
 		return i;

-- 
http://www.codemonkey.org.uk

Re: [PATCH 2/2] 8139too: Make the OQO2 automatically use PIO mode.

[Jeff Garzik]

Dave Jones wrote:
> The OQO model 2 has an RTL8139 from Atheros that doesn't like MMIO.
> Force it to always use polled IO.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> --- linux-2.6.25.noarch/drivers/net/8139too.c~	2008-05-28 21:07:36.000000000 -0400
> +++ linux-2.6.25.noarch/drivers/net/8139too.c	2008-05-28 21:19:57.000000000 -0400
> @@ -951,6 +951,14 @@ static int __devinit rtl8139_init_one (s
>  			   "Use the \"8139cp\" driver for improved performance and stability.\n");
>  	}
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_REALTEK &&
> +	    pdev->device == PCI_DEVICE_ID_REALTEK_8139 &&
> +	    pdev->subsystem_vendor == PCI_VENDOR_ID_ATHEROS &&
> +	    pdev->subsystem_device == PCI_DEVICE_ID_REALTEK_8139) {
> +		printk(KERN_INFO "8139too: OQO Model 2 detected. Forcing PIO\n");
> +		use_pio = 1;
> +	}
> +

these days we almost never hand-craft PCI ID checks like this...  Add 
this to the pci_device_id table, and create a new RTL8139_PIO entry for 
driver_data that has your desired end result

That way, it is trivial to expand the list simply by updating the PCI ID 
table

Table-driven approaches are really superior for things like this