Broadcom BCM5701 / HP NC6770

[Pyun YongHyeon]

On Mon, Apr 12, 2010 at 10:57:01AM -0700, Pyun YongHyeon wrote:
> On Sun, Apr 11, 2010 at 03:15:16AM -0600, Erich Jenkins, Fuujin Group Ltd wrote:
> > I've been muddling around in src/sys/dev on the old system and the new 
> > system and there appear to be rather major changes to MII and bge, 
> > possibly the whole stack?
> > 
> 
> It was not completely rewritten but many improvements were made.
> 
> > There are a number of things that seem to have been merged with other 
> > parts of the network stack, or perhaps written into the individual 
> > drivers (someone working on the net stack would have to verify that).
> > 
> > For instance, some files called in 5.3-REL seem to have gone away 
> > completely, and in the new (unpatched) version of if_bge.c under 
> > 7.3-REL, calls to these modules are gone:
> > 
> > - #include <vm/vm.h>              /* for vtophys */
> > - #include <vm/pmap.h>            /* for vtophys */
> 
> One of the most significant changes would be bus_dma(9) conversion
> which is required to all drivers to make it work correctly on a
> variety of platforms. bus_dma(9) does not directly use vtophys
> anymore so these headers were nuked.
> 
> > - #include <machine/clock.h>      /* for DELAY */
> > - #include <machine/bus_memio.h>
> > 
> > - #include <dev/pci/pcireg.h> (called but something changed in here)
> > - #include <dev/pci/pcivar.h> (ditto above)
> > 
> 
> No, these headers are still present.
> 
> > It appears that the checksum features have been completely rewritten, 
> 
> Checksum offloading was not completely rewritten but workaround
> for buggy controllers was added.
> 
> > and some of the ring settings have changed. It's interesting that the 
> > driver only fills 256 of the rx rings in the hopes that the cpu is "fast 
> > enough to keep up with the NIC". Would a subroutine here to grab the cpu 
> 
> That magic number 256 is adequate for most cases but it may not be
> enough to handle heavy loads. Internally the controller use fixed
> 512 RX buffers but bge(4) used only half of the buffers to save
> resources. I think you can increase SSLOTS to 512 to get full 512
> RX buffers.
> 
> > clock and count (number of procs/pipelines) be more trouble than it's 
> > worth to "automagically" increase the number of rx rings the driver 
> > fills based on the system in which it's installed?
> > 
> 
> Dynamically increasing number of RX buffers is doable but it would
> add much more code. If there is high demand for that I would just
> increase number of RX buffers to 512. Controller can't be
> configured to have more than 512 RX buffers.
> 
> > Something also changed in pci/pcireg.h and pci/pcivar.h, but I haven't 
> > had the time to hunt down and expand the source tree from the 5.3-REL 
> > branch yet.
> > 
> > I have other machines with copper nics utilizing the bge driver, and 
> > there are no issues at all. Perhaps I'm getting ahead of things, but 
> 
> Yes that is expected one. :-)
> 
> > since this seems to have been broken through several releases, would it 
> > make any sense to split the support between the BCM5701KHB chipset and 
> > the more recent BCM chipset to avoid causing issues with cards/systems 
> > not currently experiencing troubles?
> > 
> 
> I'd like to if I can. Supporting huge number of different
> controllers in single driver is maintenance nightmare. However,
> rewriting some part that require special handling for certain
> controller/revision is too risky because I don't have access to
> most controllers.
> 
> One theory for the issue I got while reading the code is link state
> handling. As I said in previous mail, link state handling for TBI
> is somewhat tricky in bge(4) and driver seemed to rely on periodic
> register access to keep track of link state. I guess polling(4) may
> give different behavior on link state handling as it does not rely
> on interrupts at all. So would you try to use polling(4) and see
> that make any difference on your box?

If polling(4) make it work, try attached patch.
-------------- next part --------------
Index: sys/dev/bge/if_bge.c
===================================================================
--- sys/dev/bge/if_bge.c	(revision 206498)
+++ sys/dev/bge/if_bge.c	(working copy)
@@ -3711,6 +3711,7 @@
 {
 	struct bge_softc *sc = xsc;
 	struct mii_data *mii = NULL;
+	int armintr;
 
 	BGE_LOCK_ASSERT(sc);
 
@@ -3739,18 +3740,16 @@
 		 * link status manually. Here we register pending link event
 		 * and trigger interrupt.
 		 */
+
 #ifdef DEVICE_POLLING
 		/* In polling mode we poll link state in bge_poll(). */
-		if (!(sc->bge_ifp->if_capenable & IFCAP_POLLING))
+		armintr = (sc->bge_ifp->if_capenable & IFCAP_POLLING) == 0;
+#else
+		armintr = 1;
 #endif
-		{
 		sc->bge_link_evt++;
-		if (sc->bge_asicrev == BGE_ASICREV_BCM5700 ||
-		    sc->bge_flags & BGE_FLAG_5788)
+		if (armintr != 0)
 			BGE_SETBIT(sc, BGE_MISC_LOCAL_CTL, BGE_MLC_INTR_SET);
-		else
-			BGE_SETBIT(sc, BGE_HCC_MODE, BGE_HCCMODE_COAL_NOW);
-		}
 	}
 
 	bge_asf_driver_up(sc);