On Fri, May 24, 2013 at 15:48 +0200, David Imhoff wrote:
> Mike Belopuhov wrote on 2013-05-23 21:55:
> >On Thu, May 23, 2013 at 11:04 +0200, David Imhoff wrote:
> >>The problem seems to be in the Auto polling of the mac link state.
> >>ifconfig shows the correct link state, but the BGE_STS_LINK bit in
> >>the driver is never set. Debugging the interrupt routine I found the
> >>driver never receives a link state changed attention, instead the
> >>last interrupt indicates a auto-poll error and bit 0 of register
> >>BGE_AUTOPOLL_STS is set.
> >>Looking at the FreeBSD and Linux drivers I found that they don't use
> >>auto-polling. Also the programmers guide of the BCM5718 family(page
> >>204) doesn't describe the auto-polling method anymore, as was done
> >>in the PG of other BCM57xx.
> >I agree that this might be a better solution than what I had:
> >setting the BGE_STS_LINK_EVT flag back in bge_link_upd to force
> >the bge_intr into running it every time untill it gets the link;
> >and it seems to work.
>
> I also tried fixing auto-polling mode, but it kept giving me a lot
> of bogus interrupts(few hundred) after bringing the link up. Using
> the LNKRDY signal just gives 2 interrupts.
>
> >>So I disabled Auto-polling for the BCM5718 family. This made all
> >>ports work on my Dell R320(BCM5720) and BCM5719. My BCM5719 still
> >>doesn't fully work because now i run into bge_watchdog reset's when
> >>sending big packet, the same issue reported by Robert Young on 5
> >>April.
> >>
> >Right, it seems like the TX completion interrupts never arrive
> >for large packets. Forcing txeof doesn't help. In fact I can
> >see some funny things going on here: for small ping packets 1
> >out of 1 tx buffer descriptors contain an mbuf to scratch, for
> >170 byte packets 1 out of 2 tx bd's contain an mbuf, for 250
> >byte packets 1 out of 3 tx bd's contain an mbuf, for 500 byte
> >packets TX completion interrupt never arrives.
>
> In my setup the problem seems RX related. Sending a 1450
> byte ping to a non-existing host using a forced arp entry works.
> But when i do the same on the remote host and run tcpdump on
> the BCM5719 interface, the controller crashes and the
> watchdog will trigger when trying to send a (small) packet.
>
> My steps to reproduce:
> - connect bge host to a remote host using a hub or direct link
> - on bge host:
> # ifconfig bge0 192.168.1.1
> # tcpdump -n -i bge0
> - on remote host:
> # ifconfig ??? 192.168.1.2
> # arp -s 192.168.1.4 02:11:11:11:11:14
> # ping -c 1 192.168.1.4
> # ping -s 1450 -c 5 192.168.1.4
> - on bge host, tcpdump shows only 1 packet received
> - on bge host: ping -c 1 192.168.1.2
> - wait a bit, and notice the watchdog reset
>
Hmm, rx is screwed up for sure. take a look at "-s 200": it won't
lock up, but messages are received incorrectly, part of the payload
gets duplicated.
So it looks like that chip gets hung when it's receiving, but since
we don't transmit anything and don't reset the timer watchdog fires.
> >I had a suspicion that this is because ASF mode is turned on
> >and we fail to share a port with IPMI firmware, but right now
> >I'm more inclined to believe that this is just another TX bug.
>
> Both of my BCM5719 cards are PCI/E expansion cards, so no port
> sharing on them.
>
It confirms my suspicion that it's not IPMI related.
> >>I also noticed if_bge.c line 2293:
> >>BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL|10<<16);
> >>Does any one have a clue why the (10<<16) is there? it changes the
> >>MI clock. But why? and why OR 0xA with the default 0xC, instead of
> >>0x2? FreeBSD and Linux use the default of 0xC for the MI Clock.
> >>
> >I think it's a remnant of the original code and it should be
> >BGE_MIMODE_BASE (as in "the other frequency").
>
> But since BGE_MIMODE_BASE is the default value after reset and
> OR'ing the value wouldn't be correct, I suggest to just remove
> the '|(10<<16)'.
>
I'm not OR'ing it in, I'm overwriting the value.
> >Please inspect my diff below; I believe it's more complete.
> >diff --git sys/dev/pci/if_bge.c sys/dev/pci/if_bge.c
> >index 1d37192..d357443 100644
> >--- sys/dev/pci/if_bge.c
> >+++ sys/dev/pci/if_bge.c
> >@@ -1055,10 +1055,22 @@ bge_miibus_statchg(struct device *dev)
> > (mii->mii_media_active & IFM_ETH_FMASK) != sc->bge_flowflags) {
> > sc->bge_flowflags = mii->mii_media_active & IFM_ETH_FMASK;
> > mii->mii_media_active &= ~IFM_ETH_FMASK;
> > }
> >+ if (!BGE_STS_BIT(sc, BGE_STS_LINK) &&
> >+ mii->mii_media_status & IFM_ACTIVE &&
> >+ IFM_SUBTYPE(mii->mii_media_active) != IFM_NONE)
> >+ BGE_STS_SETBIT(sc, BGE_STS_LINK);
> >+ else if (BGE_STS_BIT(sc, BGE_STS_LINK) &&
> >+ (!(mii->mii_media_status & IFM_ACTIVE) ||
> >+ IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE))
> >+ BGE_STS_CLRBIT(sc, BGE_STS_LINK);
> >+
> >+ if (!BGE_STS_BIT(sc, BGE_STS_LINK))
> >+ return;
> >+
> > /* Set the port mode (MII/GMII) to match the link speed. */
> > mac_mode = CSR_READ_4(sc, BGE_MAC_MODE) &
> > ~(BGE_MACMODE_PORTMODE | BGE_MACMODE_HALF_DUPLEX);
> > tx_mode = CSR_READ_4(sc, BGE_TX_MODE);
> > rx_mode = CSR_READ_4(sc, BGE_RX_MODE);
> >@@ -1773,11 +1785,11 @@ int
> > bge_blockinit(struct bge_softc *sc)
> > {
> > volatile struct bge_rcb *rcb;
> > vaddr_t rcb_addr;
> > bge_hostaddr taddr;
> >- u_int32_t dmactl, val;
> >+ u_int32_t dmactl, mimode, val;
> > int i, limit;
> >
> > /*
> > * Initialize the memory window pointer register so that
> > * we can access the first 32K of internal NIC RAM. This will
> >@@ -2369,12 +2381,20 @@ bge_blockinit(struct bge_softc *sc)
> >
> > /* Enable PHY auto polling (for MII/GMII only) */
> > if (sc->bge_flags & BGE_PHY_FIBER_TBI) {
> > CSR_WRITE_4(sc, BGE_MI_STS, BGE_MISTS_LINK);
> > } else {
> >- BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL);
> >- BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL|10<<16);
> >+ if ((sc->bge_flags & BGE_CPMU_PRESENT) != 0)
> >+ mimode = BGE_MIMODE_500KHZ_CONST;
> >+ else
> >+ mimode = BGE_MIMODE_BASE;
>
> Line 2643 already sets the 500KHZ Const clock.
That chunk should be removed. It collided with other changes
and didn't make it into the diff by mistake.
> This also changes the "Use Short Preamble" flag and "PHY Address"
> which are not 0 by default.
>
I've followed FreeBSD here. If you inspect the Linux driver you'll
find the same:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/broadcom/tg3.c#n16149
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/broadcom/tg3.c#n1479
I guess we need to think about it a bit more.
Tests will help though.
> >+ if (BGE_IS_5700_FAMILY(sc) ||
> >+ BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5705) {
> >+ mimode |= BGE_MIMODE_AUTOPOLL;
> >+ BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL);
> >+ }
>
> I didn't dare to change the behaviour for non BCM5718 family
> cards since this might break compatibility with cards that are
> currently working. I don't know what the official policy of
> OpenBSD is for these kind of changes.
>
For bge I'd guess that FreeBSD is an upstream, yet we don't sync
with it very often. But anyways this surely have to be tested.
> >+ CSR_WRITE_4(sc, BGE_MI_MODE, mimode);
> > if (BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5700)
>
> FreeBSD excludes the BGE_CHIPID_BCM5700_B2 chip here, so
> maybe we should also do so.
>
I guess we can do that.
> > CSR_WRITE_4(sc, BGE_MAC_EVT_ENB,
> > BGE_EVTENB_MI_INTERRUPT);
> > }
> >@@ -4515,10 +4535,17 @@ bge_link_upd(struct bge_softc *sc)
> > else if (BGE_STS_BIT(sc, BGE_STS_LINK) &&
> > (!(mii->mii_media_status & IFM_ACTIVE) ||
> > IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE))
> > BGE_STS_CLRBIT(sc, BGE_STS_LINK);
> > }
> >+ } else {
> >+ /*
> >+ * For controllers that call mii_tick, we have to poll
> >+ * link status.
> >+ */
>
> I wonder if it's possible that a interrupt is triggered during a
> bge_miibus_writereg/bge_miibus_readreg on a non-APE firmware? In that
> case this code path would also be taken on cards that use auto-polling.
>
This code is executed under IPL_NET, so no, it's not possible.
> Also do we still have to check for broken chips that have the
> BGE_STATFLAG_LINKSTATE_CHANGED always set?
>
FreeBSD has changed that check to just "if statusword is set".
I don't know which are this chips.
> >+ mii_pollstat(mii);
> >+ bge_miibus_statchg(&sc->bge_dev);
> > }
> >
> > /* Clear the attention */
> > CSR_WRITE_4(sc, BGE_MAC_STS, BGE_MACSTAT_SYNC_CHANGED|
> > BGE_MACSTAT_CFG_CHANGED|BGE_MACSTAT_MI_COMPLETE|
>
> Attached below is a new version of your patch with my comments
> applied.
>
> Kind regards,
>
> David
>
diff --git sys/dev/pci/if_bge.c sys/dev/pci/if_bge.c
index 1d37192..846c09c 100644
--- sys/dev/pci/if_bge.c
+++ sys/dev/pci/if_bge.c
@@ -1055,10 +1055,22 @@ bge_miibus_statchg(struct device *dev)
(mii->mii_media_active & IFM_ETH_FMASK) != sc->bge_flowflags) {
sc->bge_flowflags = mii->mii_media_active & IFM_ETH_FMASK;
mii->mii_media_active &= ~IFM_ETH_FMASK;
}
+ if (!BGE_STS_BIT(sc, BGE_STS_LINK) &&
+ mii->mii_media_status & IFM_ACTIVE &&
+ IFM_SUBTYPE(mii->mii_media_active) != IFM_NONE)
+ BGE_STS_SETBIT(sc, BGE_STS_LINK);
+ else if (BGE_STS_BIT(sc, BGE_STS_LINK) &&
+ (!(mii->mii_media_status & IFM_ACTIVE) ||
+ IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE))
+ BGE_STS_CLRBIT(sc, BGE_STS_LINK);
+
+ if (!BGE_STS_BIT(sc, BGE_STS_LINK))
+ return;
+
/* Set the port mode (MII/GMII) to match the link speed. */
mac_mode = CSR_READ_4(sc, BGE_MAC_MODE) &
~(BGE_MACMODE_PORTMODE | BGE_MACMODE_HALF_DUPLEX);
tx_mode = CSR_READ_4(sc, BGE_TX_MODE);
rx_mode = CSR_READ_4(sc, BGE_RX_MODE);
@@ -1773,11 +1785,11 @@ int
bge_blockinit(struct bge_softc *sc)
{
volatile struct bge_rcb *rcb;
vaddr_t rcb_addr;
bge_hostaddr taddr;
- u_int32_t dmactl, val;
+ u_int32_t dmactl, mimode, val;
int i, limit;
/*
* Initialize the memory window pointer register so that
* we can access the first 32K of internal NIC RAM. This will
@@ -2369,13 +2381,22 @@ bge_blockinit(struct bge_softc *sc)
/* Enable PHY auto polling (for MII/GMII only) */
if (sc->bge_flags & BGE_PHY_FIBER_TBI) {
CSR_WRITE_4(sc, BGE_MI_STS, BGE_MISTS_LINK);
} else {
- BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL);
- BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL|10<<16);
- if (BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5700)
+ if ((sc->bge_flags & BGE_CPMU_PRESENT) != 0)
+ mimode = BGE_MIMODE_500KHZ_CONST;
+ else
+ mimode = BGE_MIMODE_BASE;
+ if (BGE_IS_5700_FAMILY(sc) ||
+ BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5705) {
+ mimode |= BGE_MIMODE_AUTOPOLL;
+ BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL);
+ }
+ CSR_WRITE_4(sc, BGE_MI_MODE, mimode);
+ if (BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5700 &&
+ sc->bge_chipid != BGE_CHIPID_BCM5700_B2)
CSR_WRITE_4(sc, BGE_MAC_EVT_ENB,
BGE_EVTENB_MI_INTERRUPT);
}
/*
@@ -2719,13 +2740,10 @@ bge_attach(struct device *parent, struct device *self,
void *aux)
BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5761 ||
BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5785 ||
BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57780)
sc->bge_flags |= BGE_CPMU_PRESENT;
- if ((sc->bge_flags & BGE_CPMU_PRESENT) != 0)
- BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_500KHZ_CONST);
-
/* Try to reset the chip. */
DPRINTFN(5, ("bge_reset\n"));
bge_sig_pre_reset(sc, BGE_RESET_START);
bge_reset(sc);
@@ -4489,15 +4507,10 @@ bge_link_upd(struct bge_softc *sc)
BGE_STS_CLRBIT(sc, BGE_STS_LINK);
ifp->if_link_state = LINK_STATE_DOWN;
if_link_state_change(ifp);
ifp->if_baudrate = 0;
}
- /*
- * Discard link events for MII/GMII cards if MI auto-polling disabled.
- * This should not happen since mii callouts are locked now, but
- * we keep this check for debug.
- */
} else if (BGE_STS_BIT(sc, BGE_STS_AUTOPOLL)) {
/*
* Some broken BCM chips have BGE_STATFLAG_LINKSTATE_CHANGED bit
* in status word always set. Workaround this bug by reading
* PHY link status directly.
@@ -4515,10 +4528,17 @@ bge_link_upd(struct bge_softc *sc)
else if (BGE_STS_BIT(sc, BGE_STS_LINK) &&
(!(mii->mii_media_status & IFM_ACTIVE) ||
IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE))
BGE_STS_CLRBIT(sc, BGE_STS_LINK);
}
+ } else {
+ /*
+ * For controllers that call mii_tick, we have to poll
+ * link status.
+ */
+ mii_pollstat(mii);
+ bge_miibus_statchg(&sc->bge_dev);
}
/* Clear the attention */
CSR_WRITE_4(sc, BGE_MAC_STS, BGE_MACSTAT_SYNC_CHANGED|
BGE_MACSTAT_CFG_CHANGED|BGE_MACSTAT_MI_COMPLETE|
