Re: kern/46961: Please support BCM57762 Ethernet arapter (Apple's Thunderbolt Ethernet Adapter)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

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.
 
 > --- 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.
 (existing "BGE_ASICREV_BCM57765" is "0x57785" anyway)
 
 > @@ -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?
 
 
 > @@ -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?
 
 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;
 ---
 
 Izumi Tsutsui