Re: alc(4): add support for 816x devices (and request for review)
Hello Christos,
Christos Zoulas writes:
Or we can just define IFM_UNKNOWN and use it.
Done. I have added it to if_alcreg.h. According to net/if_media.h that
the max subtype can be 31. I have chosen this value (that does not
conflict with the ones defined in net/if_media.h... If someday there
will be more subtype the 31 used for the local IFM_UNKNOWN may become
problematic! :)).
So IFM_UNKNOWN is used were appropiate as an argument for alc_aspm().
Yes, look for PCI_SUBSYS_ID_REG in /usr/src/sys/dev/pci on how to
get the subvendor/subdevice with pci_conf_read... Perhaps we should
grow convenience functions for those too.
Thank you a lot for this information! I have added it too without the
subvendor part that does not seems useful, please correct me if I am
wrong (FreeBSD's VENDORID_ATHEROS or NetBSD's PCI_VENDOR_ATTANSIC is
ATM the only vendor for all the ethernet cards supported by alc(4)).
I will attach the new patches in this email. Apart the previous two
parts discussed in this email I have also introduced various cosmetic
fix in order to reduce the diff with the present if_alc.c (alc_attach()
variables definitions and initializations).
It would be nice if someone with an alc(4) ethernet card (especially a
813x) can test if I did not broke anything.
After I will receive an ok I will then open a PR attaching these patches
with also various man pages improvements (I will do my best in trying to
avoid wizd(8) invocation ;)).
Ciao,
L.
Index: pcidevs
===
RCS file: /cvsroot/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1211
diff -u -r1.1211 pcidevs
--- pcidevs 14 Jan 2015 15:26:08 - 1.1211
+++ pcidevs 15 Jan 2015 14:50:40 -
@@ -1182,6 +1182,7 @@
product ATTANSIC ETHERNET_100 0x2048 L2 100 Mbit Ethernet Adapter
product ATTANSIC AR8152_B 0x2060 AR8152 v1.1 Fast Ethernet Adapter
product ATTANSIC AR8152_B2 0x2062 AR8152 v2.0 Fast Ethernet Adapter
+product ATTANSIC E2200 0xe091 E2200
/* ATI products */
/* See http://www.x.org/wiki/Radeon%20ASICs */
Index: if_alc.c
===
RCS file: /cvsroot/src/sys/dev/pci/if_alc.c,v
retrieving revision 1.11
diff -u -r1.11 if_alc.c
--- if_alc.c 29 Mar 2014 19:28:24 - 1.11
+++ if_alc.c 15 Jan 2015 14:50:40 -
@@ -95,6 +95,16 @@
Atheros AR8152 v1.1 PCIe Fast Ethernet },
{ PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_AR8152_B2, 6 * 1024,
Atheros AR8152 v2.0 PCIe Fast Ethernet },
+ { PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_AR8161, 9 * 1024,
+ Atheros AR8161 PCIe Gigabit Ethernet },
+ { PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_AR8162, 9 * 1024,
+ Atheros AR8162 PCIe Fast Ethernet },
+ { PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_AR8171, 9 * 1024,
+ Atheros AR8171 PCIe Gigabit Ethernet },
+ { PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_AR8172, 9 * 1024,
+ Atheros AR8172 PCIe Fast Ethernet },
+ { PCI_VENDOR_ATTANSIC, PCI_PRODUCT_ATTANSIC_E2200, 9 * 1024,
+ Killer E2200 Gigabit Ethernet },
{ 0, 0, 0, NULL },
};
@@ -110,14 +120,20 @@
static int alc_mediachange(struct ifnet *);
static void alc_mediastatus(struct ifnet *, struct ifmediareq *);
-static void alc_aspm(struct alc_softc *, int);
+static void alc_aspm(struct alc_softc *, int, int);
+static void alc_aspm_813x(struct alc_softc *, int);
+static void alc_aspm_816x(struct alc_softc *, int);
static void alc_disable_l0s_l1(struct alc_softc *);
static int alc_dma_alloc(struct alc_softc *);
static void alc_dma_free(struct alc_softc *);
+static void alc_dsp_fixup(struct alc_softc *, int);
static int alc_encap(struct alc_softc *, struct mbuf **);
static struct alc_ident *
alc_find_ident(struct pci_attach_args *);
static void alc_get_macaddr(struct alc_softc *);
+static void alc_get_macaddr_813x(struct alc_softc *);
+static void alc_get_macaddr_816x(struct alc_softc *);
+static void alc_get_macaddr_par(struct alc_softc *);
static void alc_init_cmb(struct alc_softc *);
static void alc_init_rr_ring(struct alc_softc *);
static int alc_init_rx_ring(struct alc_softc *, bool);
@@ -125,12 +141,22 @@
static void alc_init_tx_ring(struct alc_softc *);
static int alc_intr(void *);
static void alc_mac_config(struct alc_softc *);
+static uint32_t alc_mii_readreg_813x(struct alc_softc *, int, int);
+static uint32_t alc_mii_readreg_816x(struct alc_softc *, int, int);
+static void alc_mii_writereg_813x(struct alc_softc *, int, int, int);
+static void alc_mii_writereg_816x(struct alc_softc *, int, int, int);
static int alc_miibus_readreg(device_t, int, int);
static void alc_miibus_statchg(struct ifnet *);
static void alc_miibus_writereg(device_t, int, int, int);
+static uint32_t alc_miidbg_readreg(struct alc_softc *, int);
+static void alc_miidbg_writereg(struct alc_softc *, int, int);
+static uint32_t alc_miiext_readreg(struct alc_softc *, int, int);
+static uint32_t alc_miiext_writereg(struct alc_softc *, int, int, int);
static int alc_newbuf(struct alc_softc
Re: alc(4): add support for 816x devices (and request for review)
On Jan 15, 4:18pm, iaml...@gmail.com (Leonardo Taccari) wrote: -- Subject: Re: alc(4): add support for 816x devices (and request for review) | Christos Zoulas writes: | Or we can just define IFM_UNKNOWN and use it. | Done. I have added it to if_alcreg.h. According to net/if_media.h that | the max subtype can be 31. I have chosen this value (that does not | conflict with the ones defined in net/if_media.h... If someday there | will be more subtype the 31 used for the local IFM_UNKNOWN may become | problematic! :)). | | So IFM_UNKNOWN is used were appropiate as an argument for alc_aspm(). Ok. | Yes, look for PCI_SUBSYS_ID_REG in /usr/src/sys/dev/pci on how to | get the subvendor/subdevice with pci_conf_read... Perhaps we should | grow convenience functions for those too. | Thank you a lot for this information! I have added it too without the | subvendor part that does not seems useful, please correct me if I am | wrong (FreeBSD's VENDORID_ATHEROS or NetBSD's PCI_VENDOR_ATTANSIC is | ATM the only vendor for all the ethernet cards supported by alc(4)). I think that SUBSYS_ID_REG returns a 32 bit value that contains both the vendor and the product id, so you need to mask the product id to compare against just the id (look at the macros in pcireg.h). | I will attach the new patches in this email. Apart the previous two | parts discussed in this email I have also introduced various cosmetic | fix in order to reduce the diff with the present if_alc.c (alc_attach() | variables definitions and initializations). LGTM... | It would be nice if someone with an alc(4) ethernet card (especially a | 813x) can test if I did not broke anything. That would be great indeed. | After I will receive an ok I will then open a PR attaching these patches | with also various man pages improvements (I will do my best in trying to | avoid wizd(8) invocation ;)). Ok :-) I will take care of it from there... christos
Re: alc(4): add support for 816x devices (and request for review)
On Jan 14, 5:59pm, iaml...@gmail.com (Leonardo Taccari) wrote: -- Subject: Re: alc(4): add support for 816x devices (and request for review) | | - if (phy != sc-alc_phyaddr) | - return; | - | | is causing your multiple phy issue? | Great catch! I can confirm that adding it to alc_mii_readreg_816x() | function (just after the variables declaration) fixes it (like in | alc_mii_readreg_813x()). Great.. | It is time to give a look to the two REVIEWME where I was unsure... | The first one is: | | + /* REVIEWME: can we access mii-mii_media_active here? */ | + alc_aspm(sc, 1, IFM_SUBTYPE(mii-mii_media_active)); | | FreeBSD instead of IFM_SUBTYPE() passes IFM_UNKNOWN that we do not have. | Can we safetely use IFM_SUBTYPE() here? Or we can just define IFM_UNKNOWN and use it. | The second (and last) part where I am unsure is how to properly handle | the following case: | | + case PCI_PRODUCT_ATTANSIC_AR8161: | + /* | + * REVIEWME: properly handle AR8161 like FreeBSD: | + * if (pci_get_subvendor(dev) == VENDORID_ATHEROS | + *pci_get_subdevice(dev) == 0x0091 sc-alc_rev == 0) | + * sc-alc_flags |= ALC_FLAG_LINK_WAR; | + */ | + /* FALLTHROUGH */ | + case PCI_PRODUCT_ATTANSIC_E2200: | | NetBSD's PCI_PRODUCT_ATTANSIC_AR8161 and FreeBSD's | DEVICEID_ATHEROS_AR8161 are both 0x1091... Can someone help me to | understand how can we handle it? Are the first two comparisons just | redundant and just sc-alc_rev comparison is needed? Yes, look for PCI_SUBSYS_ID_REG in /usr/src/sys/dev/pci on how to get the subvendor/subdevice with pci_conf_read... Perhaps we should grow convenience functions for those too. christos
Re: alc(4): add support for 816x devices (and request for review)
Hello Christos and the entire NetBSD community, Christos Zoulas writes: Looks pretty good, perhaps this: - if (phy != sc-alc_phyaddr) - return; - is causing your multiple phy issue? Great catch! I can confirm that adding it to alc_mii_readreg_816x() function (just after the variables declaration) fixes it (like in alc_mii_readreg_813x()). It is time to give a look to the two REVIEWME where I was unsure... The first one is: + /* REVIEWME: can we access mii-mii_media_active here? */ + alc_aspm(sc, 1, IFM_SUBTYPE(mii-mii_media_active)); FreeBSD instead of IFM_SUBTYPE() passes IFM_UNKNOWN that we do not have. Can we safetely use IFM_SUBTYPE() here? The second (and last) part where I am unsure is how to properly handle the following case: + case PCI_PRODUCT_ATTANSIC_AR8161: + /* +* REVIEWME: properly handle AR8161 like FreeBSD: +* if (pci_get_subvendor(dev) == VENDORID_ATHEROS +*pci_get_subdevice(dev) == 0x0091 sc-alc_rev == 0) +* sc-alc_flags |= ALC_FLAG_LINK_WAR; +*/ + /* FALLTHROUGH */ + case PCI_PRODUCT_ATTANSIC_E2200: NetBSD's PCI_PRODUCT_ATTANSIC_AR8161 and FreeBSD's DEVICEID_ATHEROS_AR8161 are both 0x1091... Can someone help me to understand how can we handle it? Are the first two comparisons just redundant and just sc-alc_rev comparison is needed? Thank you! Ciao, L.
