Re: Enabling MSI on the Asus Vintage AH-1

From: John Baldwin <jhb_at_freebsd.org>
Date: Thu, 28 Dec 2006 15:50:54 -0500
On Tuesday 26 December 2006 02:29, Pyun YongHyeon wrote:
> On Mon, Dec 25, 2006 at 11:30:45PM -0500, Scott Long wrote:
>  > Pyun YongHyeon wrote:
>  > >On Sat, Dec 23, 2006 at 09:54:58PM +0000, Bruce M. Simpson wrote:
>  > > > Hi,
>  > > > 
>  > > > It looks like MSI was detected, but not used by the msk(4) driver on 
>  > > the > Asus Vintage AH-1.
>  > > > 
>  > > > This is a uniprocessor Athlon64 system. The PCI bridges on this system 
>  > > > aren't in the MSI blacklist, however, there are several odd messages 
>  > > > regarding a non-default MSI window. Looking at the code suggests it 
>  > > > expects to see the MSI window at 0xfee00000.
>  > > > 
>  > > > BTW: This system's on-board SATA controller stopped working with 
>  > > 6.2-RC, > so I'm using an add-on PCI-e card for SATA to connect the root 
>  > > disk.
>  > > > 
>  > > > pcib0: <ACPI Host-PCI bridge> port 0xcf8-0xcff on acpi0
>  > > > pci0: <ACPI PCI bus> on pcib0
>  > > > pci0: physical bus=0
>  > >
>  > >[...]
>  > >
>  > > > pcib2: <ACPI PCI-PCI bridge> at device 6.0 on pci0
>  > > > pcib2:   secondary bus     2
>  > > > pcib2:   subordinate bus   2
>  > > > pcib2:   I/O decode        0xc000-0xcfff
>  > > > pcib2:   memory decode     0xfe400000-0xfe4fffff
>  > > > pcib2:   no prefetched decode
>  > > > pci2: <ACPI PCI bus> on pcib2
>  > > > pci2: physical bus=2
>  > > > found-> vendor=0x11ab, dev=0x4362, revid=0x19
>  > > >        bus=2, slot=0, func=0
>  > > >        class=02-00-00, hdrtype=0x00, mfdev=0
>  > > >        cmdreg=0x0107, statreg=0x4010, cachelnsz=16 (dwords)
>  > > >        lattimer=0x00 (0 ns), mingnt=0x00 (0 ns), maxlat=0x00 (0 ns)
>  > > >        intpin=a, irq=5
>  > > >        powerspec 2  supports D0 D1 D2 D3  current D0
>  > > >        VPD Ident: Marvell Yukon 88E8053 Gigabit Ethernet Controller
>  > > >        PN: Yukon 88E8053
>  > > >        EC: Rev. 1.9
>  > > >        MN: Marvell
>  > > >        SN: AbCdEfG32a88a
>  > > >        CP: id 1, BAR16, off 0x3cc
>  > > >        RV: 0x24
>  > > >        MSI supports 2 messages, 64 bit
>  > >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  > >I think Yukon II supports one MSI message. But all systems I know
>  > >reported that it supports two MSI messages. This is main reason why
>  > >msk(4) doesn't use MSI ATM. I don't know why Yukon II claims to
>  > >support two MSI messages.(for dual port MAC configuraiton?)
>  > 
>  > My guess is that it advertises 2 messages for the dual-port 
>  > configuration.  The intention is probably that the driver would
>  > run a loop where it allocates a message as it configures each port.
>  > So if you only have a single port, then you only allocate a single
>  > message and ignore the other one.  This looks like it will be hard to
>  > fit into the msk+mskc code that is there right now.
>  > 
>  > >
>  > >You can force to use MSI by assigning 'msic = 1' before calling
>  > >pci_alloc_msi(9) in mskc_attach(). However it wouldn't work if you
>  > >reload the msk(4) again. Other than that it works well with MSI.
>  > 
>  > I don't understand why there would be a failure here.  Can you explain?
>  > 
> 
> Me too.
> 
> I don't remember what message it was but it's somthing like
> a message with two irq numbers for the device on second loading
> and then it failed to allocate interrupt handler.
> 
> mskc0: <Marvell Yukon 88E8053 Gigabit Ethernet> port 0xXXXX-0xYYYY mem
>  0xXXXXXXXX-0xYYYYYYYY irq 19, 256 at device X.Y on pciX
>                        ^^^^^^^^^^^

You need to do the pci_release_msi() after releasing the SYS_RES_IRQ resource.
Also, to support MSI-X parts, you need to alloc your SYS_RES_MEMORY resource
before you call pci_alloc_msi().  The patch below fixes much of this, but
disables MSI for the dual port cards for now since I didn't want to untangle
all the interrupt code to create separate MSI handlers for each port.  You
also have a bug in your task where it seems that if you ifconfig the first
port on a dual port card down, you won't ever handle interrupts for the
second port:

	if (sc_if0 != NULL) {
		ifp0 = sc_if0->msk_ifp;
		if ((ifp0->if_drv_flags & IFF_DRV_RUNNING) == 0)
			goto done;
	}
	if (sc_if1 != NULL) {
		ifp1 = sc_if1->msk_ifp;
		if ((ifp1->if_drv_flags & IFF_DRV_RUNNING) == 0)
			goto done;
	}

Since RUNNING would be clear for ifp0, you would goto done and not handle
any of the events for ifp1.  Anyways, here is the patch I came up with so
far:

Index: if_msk.c
===================================================================
RCS file: /host/cvs/usr/cvs/src/sys/dev/msk/if_msk.c,v
retrieving revision 1.1
diff -u -r1.1 if_msk.c
--- if_msk.c	13 Dec 2006 02:30:11 -0000	1.1
+++ if_msk.c	28 Dec 2006 20:45:57 -0000
_at__at_ -339,16 +339,25 _at__at_
 
 static struct resource_spec msk_res_spec_io[] = {
 	{ SYS_RES_IOPORT,	PCIR_BAR(1),	RF_ACTIVE },
-	{ SYS_RES_IRQ,		0,		RF_ACTIVE | RF_SHAREABLE },
 	{ -1,			0,		0 }
 };
 
 static struct resource_spec msk_res_spec_mem[] = {
 	{ SYS_RES_MEMORY,	PCIR_BAR(0),	RF_ACTIVE },
+	{ -1,			0,		0 }
+};
+
+static struct resource_spec msk_irq_spec_legacy[] = {
 	{ SYS_RES_IRQ,		0,		RF_ACTIVE | RF_SHAREABLE },
 	{ -1,			0,		0 }
 };
 
+static struct resource_spec msk_irq_spec_msi[] = {
+	{ SYS_RES_IRQ,		1,		RF_ACTIVE },
+	{ SYS_RES_IRQ,		2,		RF_ACTIVE },
+	{ -1,			0,		0 }
+};
+
 static int
 msk_miibus_readreg(device_t dev, int phy, int reg)
 {
_at__at_ -1546,25 +1555,7 _at__at_
 	 */
 	pci_enable_busmaster(dev);
 
-	/* Allocate resources */
-	sc->msk_msi = 0;
-	msic = pci_msi_count(dev);
-	if (bootverbose)
-		device_printf(dev, "MSI count : %d\n", msic);
-	/*
-	 * Due to a unknown reason Yukon II reports it can handle two
-	 * messages even if it can handle just one message. Forcing
-	 * to allocate 1 message seems to work but reloading kernel
-	 * module after unloading the driver fails. Only use MSI when
-	 * it reports 1 message until we have better understanding
-	 * for the hardware.
-	 */
-	if (msic == 1 && msi_disable == 0 && pci_alloc_msi(dev, &msic) == 0) {
-		sc->msk_msi = 1;
-		/* Set rid to 1 for SYS_RES_IRQ to use MSI. */
-		msk_res_spec_io[1].rid = 1;
-		msk_res_spec_mem[1].rid = 1;
-	}
+	/* Allocate I/O resource */
 #ifdef MSK_USEIOSPACE
 	sc->msk_res_spec = msk_res_spec_io;
 #else
_at__at_ -1710,6 +1701,36 _at__at_
 		sc->msk_hw_feature = 0;
 	}
 
+	/* Allocate IRQ resources. */
+	msic = pci_msi_count(dev);
+	if (bootverbose)
+		device_printf(dev, "MSI count : %d\n", msic);
+	/*
+	 * The Yukon II reports it can handle two messages, one for each
+	 * possible port.  We go ahead and allocate two messages and only
+	 * setup a handler for both if we have a dual port card.
+	 *
+	 * XXX: I haven't untangled the interrupt handler to handle dual
+	 * port cards with separate MSI messages, so for now I disable MSI
+	 * on dual port cards.
+	 */
+	if (msic == 2 && msi_disable == 0 && sc->msk_num_port == 1 &&
+	    pci_alloc_msi(dev, &msic) == 0) {
+		if (msic == 2) {
+			sc->msk_msi = 1;
+			sc->msk_irq_spec = msk_irq_spec_msi;
+		} else {
+			pci_release_msi(dev);
+			sc->msk_irq_spec = msk_irq_spec_legacy;
+		}
+	}
+
+	error = bus_alloc_resources(dev, sc->msk_irq_spec, sc->msk_irq);
+	if (error) {
+		device_printf(dev, "couldn't allocate IRQ resources\n");
+		goto fail;
+	}
+
 	if ((error = msk_status_dma_alloc(sc)) != 0)
 		goto fail;
 
_at__at_ -1770,8 +1791,8 _at__at_
 	taskqueue_start_threads(&sc->msk_tq, 1, PI_NET, "%s taskq",
 	    device_get_nameunit(sc->msk_dev));
 	/* Hook interrupt last to avoid having to lock softc. */
-	error = bus_setup_intr(dev, sc->msk_res[1], INTR_TYPE_NET |
-	    INTR_MPSAFE | INTR_FAST, msk_intr, sc, &sc->msk_intrhand);
+	error = bus_setup_intr(dev, sc->msk_irq[0], INTR_TYPE_NET |
+	    INTR_MPSAFE | INTR_FAST, msk_intr, sc, &sc->msk_intrhand[0]);
 
 	if (error != 0) {
 		device_printf(dev, "couldn't set up interrupt handler\n");
_at__at_ -1884,10 +1905,15 _at__at_
 		taskqueue_free(sc->msk_tq);
 		sc->msk_tq = NULL;
 	}
-	if (sc->msk_intrhand) {
-		bus_teardown_intr(dev, sc->msk_res[1], sc->msk_intrhand);
-		sc->msk_intrhand = NULL;
+	if (sc->msk_intrhand[0]) {
+		bus_teardown_intr(dev, sc->msk_irq[0], sc->msk_intrhand[0]);
+		sc->msk_intrhand[0] = NULL;
+	}
+	if (sc->msk_intrhand[1]) {
+		bus_teardown_intr(dev, sc->msk_irq[0], sc->msk_intrhand[0]);
+		sc->msk_intrhand[1] = NULL;
 	}
+	bus_release_resources(dev, sc->msk_irq_spec, sc->msk_irq);
 	if (sc->msk_msi)
 		pci_release_msi(dev);
 	bus_release_resources(dev, sc->msk_res_spec, sc->msk_res);
Index: if_mskreg.h
===================================================================
RCS file: /host/cvs/usr/cvs/src/sys/dev/msk/if_mskreg.h,v
retrieving revision 1.1
diff -u -r1.1 if_mskreg.h
--- if_mskreg.h	13 Dec 2006 02:30:11 -0000	1.1
+++ if_mskreg.h	28 Dec 2006 20:45:37 -0000
_at__at_ -2314,9 +2314,11 _at__at_
 
 /* Softc for the Marvell Yukon II controller. */
 struct msk_softc {
-	struct resource		*msk_res[2];	/* I/O and IRQ resources */
+	struct resource		*msk_res[1];	/* I/O resource */
 	struct resource_spec	*msk_res_spec;
-	void			*msk_intrhand;	/* irq handler handle */
+	struct resource		*msk_irq[2];	/* IRQ resources */
+	struct resource_spec	*msk_irq_spec;
+	void			*msk_intrhand[2]; /* irq handler handle */
 	device_t		msk_dev;
 	uint8_t			msk_hw_id;
 	uint8_t			msk_hw_rev;

-- 
John Baldwin
Received on Thu Dec 28 2006 - 20:22:18 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:04 UTC