Date: Thu, 28 Dec 2006 15:50:54 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org, pyunyh@gmail.com Cc: "Bruce M. Simpson" <bms@freebsd.org> Subject: Re: Enabling MSI on the Asus Vintage AH-1 Message-ID: <200612281550.55581.jhb@freebsd.org> In-Reply-To: <20061226072929.GC994@cdnetworks.co.kr> References: <20061212020023.GA9698@cdnetworks.co.kr> <4590A575.3090403@samsco.org> <20061226072929.GC994@cdnetworks.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
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
@@ -339,16 +339,25 @@
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)
{
@@ -1546,25 +1555,7 @@
*/
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
@@ -1710,6 +1701,36 @@
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;
@@ -1770,8 +1791,8 @@
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");
@@ -1884,10 +1905,15 @@
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
@@ -2314,9 +2314,11 @@
/* 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200612281550.55581.jhb>