NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/46961: Please support BCM57762 Ethernet arapter (Apple's Thunderbolt Ethernet Adapter)
The following reply was made to PR kern/46961; it has been noted by GNATS.
From: Ryo ONODERA <ryo_on%yk.rim.or.jp@localhost>
To: gnats-bugs%NetBSD.org@localhost, tsutsui%ceres.dti.ne.jp@localhost
Cc:
Subject: Re: kern/46961: Please support BCM57762 Ethernet arapter (Apple's
Thunderbolt Ethernet Adapter)
Date: Sun, 16 Sep 2012 22:52:03 +0900 (JST)
Hi,
Thanks for your review!
I have add the replies below.
I have update my patches.
And I have forgotten to include PHY part.
Please see new patches.
http://www.netbsd.org/~ryoon/120916a-bcm57762.diff
From: Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>, Date: Sun, 16 Sep
2012 10:25:02 +0000 (UTC)
> The following reply was made to PR kern/46961; it has been noted by GNATS.
>
> From: Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>
> To: gnats-bugs%NetBSD.org@localhost
> Cc: kern-bug-people%NetBSD.org@localhost, tsutsui%ceres.dti.ne.jp@localhost
> Subject: Re: kern/46961: Please support BCM57762 Ethernet arapter (Apple's
> Thunderbolt
> Ethernet Adapter)
> Date: Sun, 16 Sep 2012 19:21:46 +0900
>
> > --- net/if_ether.h 19 May 2010 20:41:59 -0000 1.58
> > +++ net/if_ether.h 16 Sep 2012 00:45:23 -0000
>
> > +#define ETHER_MAX_DIX_LEN 1536 /* Maximum DIX frame length */
>
> What does the "Maximum DIX frame length" mean?
>
> With a quick glance, OpenBSD's cvs log doesn't mention about it
> http://www.openbsd.org/cgi-bin/cvsweb/src/sys/netinet/if_ether.h#rev1.34
> and I can't find description that mentions 1536 bytes around DIX Ethernet.
>
> If there is no specification about the value, it's better
> to add local macro into if_bge.c rather than MI if_ether.h.
I think DIX Ethernet's max length is 1526.
I have no idea about 1536.
Anyway I will make ETHER_MAX_DIX_LEN bge local.
> > --- dev/pci/if_bge.c 22 Jul 2012 14:33:01 -0000 1.201
> > +++ dev/pci/if_bge.c 16 Sep 2012 00:45:23 -0000
>
> > @@ -564,6 +564,10 @@
> > "Broadcom BCM57761 Fast Ethernet",
> > },
> > { PCI_VENDOR_BROADCOM,
> > + PCI_PRODUCT_BROADCOM_BCM57762,
> > + "Broadcom BCM57762 Gigabit Ethernet",
> > + },
> > + { PCI_VENDOR_BROADCOM,
> > PCI_PRODUCT_BROADCOM_BCM57765,
> > "Broadcom BCM57765 Fast Ethernet",
> > },
> > @@ -728,6 +732,7 @@
> > { BGE_ASICREV_BCM57780, "unknown BCM57780" },
> > { BGE_ASICREV_BCM5717, "unknown BCM5717" },
> > { BGE_ASICREV_BCM57765, "unknown BCM57765" },
> > + { BGE_ASICREV_BCM57766, "unknown BCM57766" },
> >
> > { 0, NULL }
> > };
>
> Your bge is BCM57762, but has BCM57766 ASICREV?
> Where does the name come from?
>
> Even if the actual value of BGE_ASICREV_BCM57766 is "0x57766",
> it's less confusing to define it as "BGE_ASICREV_BCM57762"
> for readers if it's actually returned by 57762.
Linux's tg3 identifies 0x57766 as ASIC_REV_57766.
See
http://lxr.free-electrons.com/source/drivers/net/ethernet/broadcom/tg3.c#L13653
.
> (existing "BGE_ASICREV_BCM57765" is "0x57785" anyway)
In Linux's tg3 0x57785 is ASIC_REV_57765.
In my opinion, 0x57766 should be BGE_ASICREV_57766, due to consistency
between OSes.
> > @@ -1979,14 +2000,10 @@
>
> > } else {
> > - CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_READDMA_LOWAT, 0x0);
> > - CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_MACRX_LOWAT, 0x10);
> > - CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_HIWAT, 0x60);
> > + CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_READDMA_LOWAT, 0x0);
> > + CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_MACRX_LOWAT, 0x10);
> > + CSR_WRITE_4(sc, BGE_BMAN_MBUFPOOL_HIWAT, 0x60);
> > }
> > #endif
>
> Wrong and unnecessary indent changes?
Yes.
> > @@ -2031,7 +2048,12 @@
> > /* Step 41: Initialize the standard RX ring control block */
> > rcb = &sc->bge_rdata->bge_info.bge_std_rx_rcb;
> > BGE_HOSTADDR(rcb->bge_hostaddr, BGE_RING_DMA_ADDR(sc, bge_rx_std_ring));
> > - if (BGE_IS_5705_PLUS(sc))
> > + if (BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5717 ||
> > + BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57765 ||
> > + BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57766)
> > + rcb->bge_maxlen_flags = (BGE_RCB_MAXLEN_FLAGS(512, 0) |
> > + (ETHER_MAX_DIX_LEN << 2));
> > + else if (BGE_IS_5705_PLUS(sc))
> > rcb->bge_maxlen_flags = BGE_RCB_MAXLEN_FLAGS(512, 0);
> > else
> > rcb->bge_maxlen_flags =
>
> Doesn't your 57762 work without this (ETHER_MAX_DIX_LEN << 2) change?
ETHER_MAX_DIX_LEN << 2 is essential.
But 1626 << 2 is also acceptable.
> OpenBSD log says it was taken from Linux driver,
> http://www.openbsd.org/cgi-bin/cvsweb/src/sys/dev/pci/if_bge.c#rev1.28
> but probably we should check specification of the
> BGE_RX_STD_RCB_MAXLEN_FLAGS register...
>
> > @@ -2904,10 +2930,14 @@
> > "setting short Tx thresholds\n");
> > }
> >
> > - if (BGE_IS_5705_PLUS(sc))
> > - sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT_5705;
> > - else
> > +
> > + if (BGE_IS_5700_FAMILY(sc) ||
> > + BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5717 ||
> > + BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57765 ||
> > + BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57766)
> > sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT;
> > + else
> > + sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT_5705;
> >
> > /* Set up ifnet structure */
> > ifp = &sc->ethercom.ec_if;
>
> It could be problematic (i.e. hard to confirm) to change default
> sc->bge_return_ring_cnt value.
>
> How about this one?
> ---
> if (BGE_IS_5705_PLUS(sc) {
> if (BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM5717 ||
> BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57765 ||
> BGE_ASICREV(sc->bge_chipid) == BGE_ASICREV_BCM57766)
> sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT;
> else
> sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT_5705;
> } else
> sc->bge_return_ring_cnt = BGE_RETURN_RING_CNT;
This works well.
> ---
>
> Izumi Tsutsui
>
>
>
--
Ryo ONODERA // ryo_on%yk.rim.or.jp@localhost
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Home |
Main Index |
Thread Index |
Old Index