| Message ID | 1321394453-21076-1-git-send-email-mcarlson@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Nov 16, 2011 at 02:36:18PM -0800, David Miller wrote: > From: "Matt Carlson" <mcarlson@broadcom.com> > Date: Tue, 15 Nov 2011 14:00:53 -0800 > > > Translating between ethtool advertisement settings and MII > > advertisements are common operations for ethernet drivers. This patch > > adds a set of helper functions that implements the conversion. The > > patch then modifies a couple of the drivers to use the new functions. > > > > Signed-off-by: Matt Carlson <mcarlson@broadcom.com> > > Signed-off-by: Michael Chan <mchan@broadcom.com> > > Doesn't build: > > In file included from drivers/net/ethernet/3com/3c59x.c:82:0: > include/linux/mii.h: In function ?ethtool_adv_to_mii_100bt?: > include/linux/mii.h:254:15: error: ?ADVERTISED_10baseT_Half? undeclared (first use in this function) > include/linux/mii.h:254:15: note: each undeclared identifier is reported only once for each function it appears in > include/linux/mii.h:256:15: error: ?ADVERTISED_10baseT_Full? undeclared (first use in this function) > include/linux/mii.h:258:15: error: ?ADVERTISED_100baseT_Half? undeclared (first use in this function) > include/linux/mii.h:260:15: error: ?ADVERTISED_100baseT_Full? undeclared (first use in this function) > include/linux/mii.h:262:15: error: ?ADVERTISED_Pause? undeclared (first use in this function) > include/linux/mii.h:264:15: error: ?ADVERTISED_Asym_Pause? undeclared (first use in this function) I forgot to add '#include <linux\ethtool.h>' to mii.h. I guess for the drivers I enabled, ethtool.h was already included. New patch on its way. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-11-15 at 14:00 -0800, Matt Carlson wrote: > Translating between ethtool advertisement settings and MII > advertisements are common operations for ethernet drivers. This patch > adds a set of helper functions that implements the conversion. The > patch then modifies a couple of the drivers to use the new functions. I like this, but: [....] > --- a/drivers/net/ethernet/broadcom/tg3.c > +++ b/drivers/net/ethernet/broadcom/tg3.c [...] > @@ -4903,23 +4880,19 @@ static int tg3_setup_fiber_mii_phy(struct tg3 *tp, int force_reset) > (tp->phy_flags & TG3_PHYFLG_PARALLEL_DETECT)) { > /* do nothing, just check for link up at the end */ > } else if (tp->link_config.autoneg == AUTONEG_ENABLE) { > - u32 adv, new_adv; > + u32 adv, newadv; > > err |= tg3_readphy(tp, MII_ADVERTISE, &adv); > - new_adv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | > - ADVERTISE_1000XPAUSE | > - ADVERTISE_1000XPSE_ASYM | > - ADVERTISE_SLCT); > - > - new_adv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); > + newadv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | > + ADVERTISE_1000XPAUSE | > + ADVERTISE_1000XPSE_ASYM | > + ADVERTISE_SLCT); Not sure why you're renaming the variable here. > - if (tp->link_config.advertising & ADVERTISED_1000baseT_Half) > - new_adv |= ADVERTISE_1000XHALF; > - if (tp->link_config.advertising & ADVERTISED_1000baseT_Full) > - new_adv |= ADVERTISE_1000XFULL; > + newadv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); > + newadv |= ethtool_adv_to_mii_1000X(tp->link_config.advertising); Doesn't this generate flow control advertising flags twice? Can the link_config.flowctrl and link_config.advertising fields get out of synch? [...] > --- a/include/linux/mii.h > +++ b/include/linux/mii.h > @@ -240,6 +240,171 @@ static inline unsigned int mii_duplex (unsigned int duplex_lock, > } > > /** > + * ethtool_adv_to_mii_100bt > + * @ethadv: the ethtool advertisement settings > + * > + * A small helper function that translates ethtool advertisement > + * settings to phy autonegotiation advertisements for the > + * MII_ADVERTISE register. > + */ > +static inline u32 ethtool_adv_to_mii_100bt(u32 ethadv) [...] > +static inline u32 mii_adv_to_ethtool_100bt(u32 adv) [...] > +static inline u32 ethtool_adv_to_mii_1000T(u32 ethadv) [...] > +static inline u32 mii_adv_to_ethtool_1000T(u32 adv) [...] > +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg? [...] > +static inline u32 mii_lpa_to_ethtool_1000T(u32 lpa) [...] > +static inline u32 ethtool_adv_to_mii_1000X(u32 ethadv) [...] > +static inline u32 mii_adv_to_ethtool_1000X(u32 adv) [...] I'm not convinced about the naming convention for these. Would it not make more sense to name them consistently by register name and signal type: ethtool_adv_to_mii_adv_t mii_adv_to_ethtool_adv_t ethtool_adv_to_mii_ctrl1000_t mii_ctrl1000_to_ethtool_adv_t mii_lpa_to_ethtool_lpa_t mii_stat1000_to_ethtool_lpa_t ethtool_adv_to_mii_adv_x mii_adv_to_ethtool_adv_x Shouldn't there be mii_lpa_to_ethtool_1000X (or mii_lpa_to_ethtool_lpa_x)? Finally, do these need to be inline? Ben.
On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote: > On Tue, 2011-11-15 at 14:00 -0800, Matt Carlson wrote: > > Translating between ethtool advertisement settings and MII > > advertisements are common operations for ethernet drivers. This patch > > adds a set of helper functions that implements the conversion. The > > patch then modifies a couple of the drivers to use the new functions. > > I like this, but: > > [....] > > --- a/drivers/net/ethernet/broadcom/tg3.c > > +++ b/drivers/net/ethernet/broadcom/tg3.c > [...] > > @@ -4903,23 +4880,19 @@ static int tg3_setup_fiber_mii_phy(struct tg3 *tp, int force_reset) > > (tp->phy_flags & TG3_PHYFLG_PARALLEL_DETECT)) { > > /* do nothing, just check for link up at the end */ > > } else if (tp->link_config.autoneg == AUTONEG_ENABLE) { > > - u32 adv, new_adv; > > + u32 adv, newadv; > > > > err |= tg3_readphy(tp, MII_ADVERTISE, &adv); > > - new_adv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | > > - ADVERTISE_1000XPAUSE | > > - ADVERTISE_1000XPSE_ASYM | > > - ADVERTISE_SLCT); > > - > > - new_adv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); > > + newadv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | > > + ADVERTISE_1000XPAUSE | > > + ADVERTISE_1000XPSE_ASYM | > > + ADVERTISE_SLCT); > > Not sure why you're renaming the variable here. 80 column character violations. By eliminating the underscore, I can *just* make it fit. > > - if (tp->link_config.advertising & ADVERTISED_1000baseT_Half) > > - new_adv |= ADVERTISE_1000XHALF; > > - if (tp->link_config.advertising & ADVERTISED_1000baseT_Full) > > - new_adv |= ADVERTISE_1000XFULL; > > + newadv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); > > + newadv |= ethtool_adv_to_mii_1000X(tp->link_config.advertising); > > Doesn't this generate flow control advertising flags twice? Can the > link_config.flowctrl and link_config.advertising fields get out of > synch? The tg3 driver doesn't store flow control settings through the advertising field. I thought about changing the driver in that direction and eliminating the tp->link_config.flowctrl field. That would be a separate patch though. > [...] > > --- a/include/linux/mii.h > > +++ b/include/linux/mii.h > > @@ -240,6 +240,171 @@ static inline unsigned int mii_duplex (unsigned int duplex_lock, > > } > > > > /** > > + * ethtool_adv_to_mii_100bt > > + * @ethadv: the ethtool advertisement settings > > + * > > + * A small helper function that translates ethtool advertisement > > + * settings to phy autonegotiation advertisements for the > > + * MII_ADVERTISE register. > > + */ > > +static inline u32 ethtool_adv_to_mii_100bt(u32 ethadv) > [...] > > +static inline u32 mii_adv_to_ethtool_100bt(u32 adv) > [...] > > +static inline u32 ethtool_adv_to_mii_1000T(u32 ethadv) > [...] > > +static inline u32 mii_adv_to_ethtool_1000T(u32 adv) > [...] > > +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg? You mean, like this? static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa) { u32 result = 0; if (lpa & LPA_LPACK) result |= ADVERTISED_Autoneg; return result | mii_adv_to_ethtool_100bt(lpa); } Yes, that looks like a better implementation. > [...] > > +static inline u32 mii_lpa_to_ethtool_1000T(u32 lpa) > [...] > > +static inline u32 ethtool_adv_to_mii_1000X(u32 ethadv) > [...] > > +static inline u32 mii_adv_to_ethtool_1000X(u32 adv) > [...] > > I'm not convinced about the naming convention for these. Would it not > make more sense to name them consistently by register name and signal > type: > > ethtool_adv_to_mii_adv_t > mii_adv_to_ethtool_adv_t > ethtool_adv_to_mii_ctrl1000_t > mii_ctrl1000_to_ethtool_adv_t > mii_lpa_to_ethtool_lpa_t > mii_stat1000_to_ethtool_lpa_t > ethtool_adv_to_mii_adv_x > mii_adv_to_ethtool_adv_x I don't have a strong preference either way. I'll post the change along with the above modification. > Shouldn't there be mii_lpa_to_ethtool_1000X (or > mii_lpa_to_ethtool_lpa_x)? Yes. You're right. Should it just be a preprocessor definition that points to mii_adv_to_ethtool_1000X()? > Finally, do these need to be inline? I don't have a strong preference here either. Phy code tends to be slower, so there isn't really a strong performance argument. The implementations don't seem to be so large to argue against it though. Would you prefer they not be inlined? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: > > Finally, do these need to be inline? > > I don't have a strong preference here either. Phy code tends to be > slower, so there isn't really a strong performance argument. The > implementations don't seem to be so large to argue against it though. > Would you prefer they not be inlined? > Since we are defining these in .h file, they need to be inline, right? Otherwise multiple source files including the same .h file will have conflict. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: > On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote: [...] > > > +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) > > > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg? > > You mean, like this? > > static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa) > { > u32 result = 0; > > if (lpa & LPA_LPACK) > result |= ADVERTISED_Autoneg; > > return result | mii_adv_to_ethtool_100bt(lpa); > } > > Yes, that looks like a better implementation. Think so. And I think the mii_adv_to_ethtool_* functions should add ADVERTISED_Autoneg unconditionally. But I'm not entirely sure that's right. > > [...] > > > +static inline u32 mii_lpa_to_ethtool_1000T(u32 lpa) > > [...] > > > +static inline u32 ethtool_adv_to_mii_1000X(u32 ethadv) > > [...] > > > +static inline u32 mii_adv_to_ethtool_1000X(u32 adv) > > [...] > > > > I'm not convinced about the naming convention for these. Would it not > > make more sense to name them consistently by register name and signal > > type: > > > > ethtool_adv_to_mii_adv_t > > mii_adv_to_ethtool_adv_t > > ethtool_adv_to_mii_ctrl1000_t > > mii_ctrl1000_to_ethtool_adv_t > > mii_lpa_to_ethtool_lpa_t > > mii_stat1000_to_ethtool_lpa_t > > ethtool_adv_to_mii_adv_x > > mii_adv_to_ethtool_adv_x > > I don't have a strong preference either way. I'll post the change along > with the above modification. > > > Shouldn't there be mii_lpa_to_ethtool_1000X (or > > mii_lpa_to_ethtool_lpa_x)? > > Yes. You're right. Should it just be a preprocessor definition that > points to mii_adv_to_ethtool_1000X()? I think that would need to handle LPA_LPACK as well. > > Finally, do these need to be inline? > > I don't have a strong preference here either. Phy code tends to be > slower, so there isn't really a strong performance argument. The > implementations don't seem to be so large to argue against it though. > Would you prefer they not be inlined? I suppose one of us should measure what difference it makes to code size. Ben.
From: "Michael Chan" <mchan@broadcom.com> Date: Wed, 16 Nov 2011 17:21:32 -0800 > > On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: >> > Finally, do these need to be inline? >> >> I don't have a strong preference here either. Phy code tends to be >> slower, so there isn't really a strong performance argument. The >> implementations don't seem to be so large to argue against it though. >> Would you prefer they not be inlined? >> > > Since we are defining these in .h file, they need to be inline, right? > Otherwise multiple source files including the same .h file will have > conflict. Yes, if you keep them in the header you have to keep them inline. Ben, by suggesting to not inline them, is implicitly saying to put them out in a seperate *.c file somewhere, perhaps net/core/ethtool.c or similar. With appropriate EXPORT_SYMBOL() added. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 16, 2011 at 05:29:42PM -0800, Ben Hutchings wrote: > On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: > > On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote: > [...] > > > > +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) > > > > > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg? > > > > You mean, like this? > > > > static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa) > > { > > u32 result = 0; > > > > if (lpa & LPA_LPACK) > > result |= ADVERTISED_Autoneg; > > > > return result | mii_adv_to_ethtool_100bt(lpa); > > } > > > > Yes, that looks like a better implementation. > > Think so. > > And I think the mii_adv_to_ethtool_* functions should add > ADVERTISED_Autoneg unconditionally. But I'm not entirely sure that's > right. The primary purpose of these functions is to translate the information between two representations. It seems wise to be careful not to add from it or take anything away from it. It is certainly possible to have a valid AN advertisement register configuration, but not have autoneg enabled. Keeping the ADVERTISED_Autoneg out could prevent misuse. Do you agree? > > > Shouldn't there be mii_lpa_to_ethtool_1000X (or > > > mii_lpa_to_ethtool_lpa_x)? > > > > Yes. You're right. Should it just be a preprocessor definition that > > points to mii_adv_to_ethtool_1000X()? > > I think that would need to handle LPA_LPACK as well. I was wondering if that was present in 1000Base-X mode. I didn't see it in my passing glance at a spec. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-11-16 at 20:38 -0500, David Miller wrote: > From: "Michael Chan" <mchan@broadcom.com> > Date: Wed, 16 Nov 2011 17:21:32 -0800 > > > > > On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: > >> > Finally, do these need to be inline? > >> > >> I don't have a strong preference here either. Phy code tends to be > >> slower, so there isn't really a strong performance argument. The > >> implementations don't seem to be so large to argue against it though. > >> Would you prefer they not be inlined? > >> > > > > Since we are defining these in .h file, they need to be inline, right? > > Otherwise multiple source files including the same .h file will have > > conflict. > > Yes, if you keep them in the header you have to keep them inline. > > Ben, by suggesting to not inline them, is implicitly saying to put > them out in a seperate *.c file somewhere, perhaps net/core/ethtool.c > or similar. With appropriate EXPORT_SYMBOL() added. I was thinking of putting them in drivers/net/mii.c, like the other functions declared extern in include/linux/mii.h. That would require 'select MII' in Kconfig for the drivers using them. Ben.
On Wed, 2011-11-16 at 18:06 -0800, Matt Carlson wrote: > On Wed, Nov 16, 2011 at 05:29:42PM -0800, Ben Hutchings wrote: > > On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote: > > > On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote: > > [...] > > > > > +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) > > > > > > > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg? > > > > > > You mean, like this? > > > > > > static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa) > > > { > > > u32 result = 0; > > > > > > if (lpa & LPA_LPACK) > > > result |= ADVERTISED_Autoneg; > > > > > > return result | mii_adv_to_ethtool_100bt(lpa); > > > } > > > > > > Yes, that looks like a better implementation. > > > > Think so. > > > > And I think the mii_adv_to_ethtool_* functions should add > > ADVERTISED_Autoneg unconditionally. But I'm not entirely sure that's > > right. > > The primary purpose of these functions is to translate the information > between two representations. It seems wise to be careful not to add > from it or take anything away from it. It is certainly possible to > have a valid AN advertisement register configuration, but not have > autoneg enabled. Keeping the ADVERTISED_Autoneg out could prevent > misuse. Do you agree? I'm not sure what you mean about misuse. If autoneg is not enabled then the register contents are not used and I don't think the mii_adv_to_ethtool functions should be called at all. But I'm not that bothered either way. > > > > Shouldn't there be mii_lpa_to_ethtool_1000X (or > > > > mii_lpa_to_ethtool_lpa_x)? > > > > > > Yes. You're right. Should it just be a preprocessor definition that > > > points to mii_adv_to_ethtool_1000X()? > > > > I think that would need to handle LPA_LPACK as well. > > I was wondering if that was present in 1000Base-X mode. I didn't see it > in my passing glance at a spec. IEEE 802.3-2005 ยง37.2.1.6 seems to define it similarly to the TP case. Ben.
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c index 32d1f92..e82b981 100644 --- a/drivers/net/ethernet/broadcom/bnx2.c +++ b/drivers/net/ethernet/broadcom/bnx2.c @@ -2064,21 +2064,12 @@ __acquires(&bp->phy_lock) bnx2_read_phy(bp, MII_CTRL1000, &adv1000_reg); adv1000_reg &= PHY_ALL_1000_SPEED; - if (bp->advertising & ADVERTISED_10baseT_Half) - new_adv_reg |= ADVERTISE_10HALF; - if (bp->advertising & ADVERTISED_10baseT_Full) - new_adv_reg |= ADVERTISE_10FULL; - if (bp->advertising & ADVERTISED_100baseT_Half) - new_adv_reg |= ADVERTISE_100HALF; - if (bp->advertising & ADVERTISED_100baseT_Full) - new_adv_reg |= ADVERTISE_100FULL; - if (bp->advertising & ADVERTISED_1000baseT_Full) - new_adv1000_reg |= ADVERTISE_1000FULL; - + new_adv_reg = ethtool_adv_to_mii_100bt(bp->advertising); new_adv_reg |= ADVERTISE_CSMA; - new_adv_reg |= bnx2_phy_get_pause_adv(bp); + new_adv1000_reg |= ethtool_adv_to_mii_1000T(bp->advertising); + if ((adv1000_reg != new_adv1000_reg) || (adv_reg != new_adv_reg) || ((bmcr & BMCR_ANENABLE) == 0)) { diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c index cd36234..b329459 100644 --- a/drivers/net/ethernet/broadcom/tg3.c +++ b/drivers/net/ethernet/broadcom/tg3.c @@ -3594,15 +3594,7 @@ static int tg3_phy_autoneg_cfg(struct tg3 *tp, u32 advertise, u32 flowctrl) u32 val, new_adv; new_adv = ADVERTISE_CSMA; - if (advertise & ADVERTISED_10baseT_Half) - new_adv |= ADVERTISE_10HALF; - if (advertise & ADVERTISED_10baseT_Full) - new_adv |= ADVERTISE_10FULL; - if (advertise & ADVERTISED_100baseT_Half) - new_adv |= ADVERTISE_100HALF; - if (advertise & ADVERTISED_100baseT_Full) - new_adv |= ADVERTISE_100FULL; - + new_adv |= ethtool_adv_to_mii_100bt(advertise); new_adv |= tg3_advert_flowctrl_1000T(flowctrl); err = tg3_writephy(tp, MII_ADVERTISE, new_adv); @@ -3612,11 +3604,7 @@ static int tg3_phy_autoneg_cfg(struct tg3 *tp, u32 advertise, u32 flowctrl) if (tp->phy_flags & TG3_PHYFLG_10_100_ONLY) goto done; - new_adv = 0; - if (advertise & ADVERTISED_1000baseT_Half) - new_adv |= ADVERTISE_1000HALF; - if (advertise & ADVERTISED_1000baseT_Full) - new_adv |= ADVERTISE_1000FULL; + new_adv = ethtool_adv_to_mii_1000T(advertise); if (tp->pci_chip_rev_id == CHIPREV_ID_5701_A0 || tp->pci_chip_rev_id == CHIPREV_ID_5701_B0) @@ -3790,14 +3778,7 @@ static int tg3_copper_is_advertising_all(struct tg3 *tp, u32 mask) { u32 adv_reg, all_mask = 0; - if (mask & ADVERTISED_10baseT_Half) - all_mask |= ADVERTISE_10HALF; - if (mask & ADVERTISED_10baseT_Full) - all_mask |= ADVERTISE_10FULL; - if (mask & ADVERTISED_100baseT_Half) - all_mask |= ADVERTISE_100HALF; - if (mask & ADVERTISED_100baseT_Full) - all_mask |= ADVERTISE_100FULL; + all_mask = ethtool_adv_to_mii_100bt(mask); if (tg3_readphy(tp, MII_ADVERTISE, &adv_reg)) return 0; @@ -3808,11 +3789,7 @@ static int tg3_copper_is_advertising_all(struct tg3 *tp, u32 mask) if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY)) { u32 tg3_ctrl; - all_mask = 0; - if (mask & ADVERTISED_1000baseT_Half) - all_mask |= ADVERTISE_1000HALF; - if (mask & ADVERTISED_1000baseT_Full) - all_mask |= ADVERTISE_1000FULL; + all_mask = ethtool_adv_to_mii_1000T(mask); if (tg3_readphy(tp, MII_CTRL1000, &tg3_ctrl)) return 0; @@ -4903,23 +4880,19 @@ static int tg3_setup_fiber_mii_phy(struct tg3 *tp, int force_reset) (tp->phy_flags & TG3_PHYFLG_PARALLEL_DETECT)) { /* do nothing, just check for link up at the end */ } else if (tp->link_config.autoneg == AUTONEG_ENABLE) { - u32 adv, new_adv; + u32 adv, newadv; err |= tg3_readphy(tp, MII_ADVERTISE, &adv); - new_adv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | - ADVERTISE_1000XPAUSE | - ADVERTISE_1000XPSE_ASYM | - ADVERTISE_SLCT); - - new_adv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); + newadv = adv & ~(ADVERTISE_1000XFULL | ADVERTISE_1000XHALF | + ADVERTISE_1000XPAUSE | + ADVERTISE_1000XPSE_ASYM | + ADVERTISE_SLCT); - if (tp->link_config.advertising & ADVERTISED_1000baseT_Half) - new_adv |= ADVERTISE_1000XHALF; - if (tp->link_config.advertising & ADVERTISED_1000baseT_Full) - new_adv |= ADVERTISE_1000XFULL; + newadv |= tg3_advert_flowctrl_1000X(tp->link_config.flowctrl); + newadv |= ethtool_adv_to_mii_1000X(tp->link_config.advertising); - if ((new_adv != adv) || !(bmcr & BMCR_ANENABLE)) { - tg3_writephy(tp, MII_ADVERTISE, new_adv); + if ((newadv != adv) || !(bmcr & BMCR_ANENABLE)) { + tg3_writephy(tp, MII_ADVERTISE, newadv); bmcr |= BMCR_ANENABLE | BMCR_ANRESTART; tg3_writephy(tp, MII_BMCR, bmcr); diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c index 3ebeb9d..9997be5 100644 --- a/drivers/net/ethernet/sun/niu.c +++ b/drivers/net/ethernet/sun/niu.c @@ -1151,19 +1151,8 @@ static int link_status_mii(struct niu *np, int *link_up_p) supported |= SUPPORTED_1000baseT_Full; lp->supported = supported; - advertising = 0; - if (advert & ADVERTISE_10HALF) - advertising |= ADVERTISED_10baseT_Half; - if (advert & ADVERTISE_10FULL) - advertising |= ADVERTISED_10baseT_Full; - if (advert & ADVERTISE_100HALF) - advertising |= ADVERTISED_100baseT_Half; - if (advert & ADVERTISE_100FULL) - advertising |= ADVERTISED_100baseT_Full; - if (ctrl1000 & ADVERTISE_1000HALF) - advertising |= ADVERTISED_1000baseT_Half; - if (ctrl1000 & ADVERTISE_1000FULL) - advertising |= ADVERTISED_1000baseT_Full; + advertising = mii_adv_to_ethtool_100bt(advert); + advertising |= mii_adv_to_ethtool_1000T(ctrl1000); if (bmcr & BMCR_ANENABLE) { int neg, neg1000; diff --git a/drivers/net/mii.c b/drivers/net/mii.c index c62e781..d0a2962 100644 --- a/drivers/net/mii.c +++ b/drivers/net/mii.c @@ -41,20 +41,8 @@ static u32 mii_get_an(struct mii_if_info *mii, u16 addr) advert = mii->mdio_read(mii->dev, mii->phy_id, addr); if (advert & LPA_LPACK) result |= ADVERTISED_Autoneg; - if (advert & ADVERTISE_10HALF) - result |= ADVERTISED_10baseT_Half; - if (advert & ADVERTISE_10FULL) - result |= ADVERTISED_10baseT_Full; - if (advert & ADVERTISE_100HALF) - result |= ADVERTISED_100baseT_Half; - if (advert & ADVERTISE_100FULL) - result |= ADVERTISED_100baseT_Full; - if (advert & ADVERTISE_PAUSE_CAP) - result |= ADVERTISED_Pause; - if (advert & ADVERTISE_PAUSE_ASYM) - result |= ADVERTISED_Asym_Pause; - - return result; + + return result | mii_adv_to_ethtool_100bt(advert); } /** @@ -104,19 +92,13 @@ int mii_ethtool_gset(struct mii_if_info *mii, struct ethtool_cmd *ecmd) ecmd->autoneg = AUTONEG_ENABLE; ecmd->advertising |= mii_get_an(mii, MII_ADVERTISE); - if (ctrl1000 & ADVERTISE_1000HALF) - ecmd->advertising |= ADVERTISED_1000baseT_Half; - if (ctrl1000 & ADVERTISE_1000FULL) - ecmd->advertising |= ADVERTISED_1000baseT_Full; + if (mii->supports_gmii) + ecmd->advertising |= mii_adv_to_ethtool_1000T(ctrl1000); if (bmsr & BMSR_ANEGCOMPLETE) { ecmd->lp_advertising = mii_get_an(mii, MII_LPA); - if (stat1000 & LPA_1000HALF) - ecmd->lp_advertising |= - ADVERTISED_1000baseT_Half; - if (stat1000 & LPA_1000FULL) - ecmd->lp_advertising |= - ADVERTISED_1000baseT_Full; + ecmd->lp_advertising |= + mii_lpa_to_ethtool_1000T(stat1000); } else { ecmd->lp_advertising = 0; } @@ -204,20 +186,10 @@ int mii_ethtool_sset(struct mii_if_info *mii, struct ethtool_cmd *ecmd) advert2 = mii->mdio_read(dev, mii->phy_id, MII_CTRL1000); tmp2 = advert2 & ~(ADVERTISE_1000HALF | ADVERTISE_1000FULL); } - if (ecmd->advertising & ADVERTISED_10baseT_Half) - tmp |= ADVERTISE_10HALF; - if (ecmd->advertising & ADVERTISED_10baseT_Full) - tmp |= ADVERTISE_10FULL; - if (ecmd->advertising & ADVERTISED_100baseT_Half) - tmp |= ADVERTISE_100HALF; - if (ecmd->advertising & ADVERTISED_100baseT_Full) - tmp |= ADVERTISE_100FULL; - if (mii->supports_gmii) { - if (ecmd->advertising & ADVERTISED_1000baseT_Half) - tmp2 |= ADVERTISE_1000HALF; - if (ecmd->advertising & ADVERTISED_1000baseT_Full) - tmp2 |= ADVERTISE_1000FULL; - } + tmp |= ethtool_adv_to_mii_100bt(ecmd->advertising); + + if (mii->supports_gmii) + tmp2 |= ethtool_adv_to_mii_1000T(ecmd->advertising); if (advert != tmp) { mii->mdio_write(dev, mii->phy_id, MII_ADVERTISE, tmp); mii->advertising = tmp; diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 83a5a5a..edb905f 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -563,20 +563,9 @@ static int genphy_config_advert(struct phy_device *phydev) if (adv < 0) return adv; - adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP | + adv &= ~(ADVERTISE_ALL | ADVERTISE_100BASE4 | ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM); - if (advertise & ADVERTISED_10baseT_Half) - adv |= ADVERTISE_10HALF; - if (advertise & ADVERTISED_10baseT_Full) - adv |= ADVERTISE_10FULL; - if (advertise & ADVERTISED_100baseT_Half) - adv |= ADVERTISE_100HALF; - if (advertise & ADVERTISED_100baseT_Full) - adv |= ADVERTISE_100FULL; - if (advertise & ADVERTISED_Pause) - adv |= ADVERTISE_PAUSE_CAP; - if (advertise & ADVERTISED_Asym_Pause) - adv |= ADVERTISE_PAUSE_ASYM; + adv |= ethtool_adv_to_mii_100bt(advertise); if (adv != oldadv) { err = phy_write(phydev, MII_ADVERTISE, adv); @@ -595,10 +584,7 @@ static int genphy_config_advert(struct phy_device *phydev) return adv; adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF); - if (advertise & SUPPORTED_1000baseT_Half) - adv |= ADVERTISE_1000HALF; - if (advertise & SUPPORTED_1000baseT_Full) - adv |= ADVERTISE_1000FULL; + adv |= ethtool_adv_to_mii_1000T(advertise); if (adv != oldadv) { err = phy_write(phydev, MII_CTRL1000, adv); diff --git a/include/linux/mii.h b/include/linux/mii.h index 2774823..ac49406 100644 --- a/include/linux/mii.h +++ b/include/linux/mii.h @@ -240,6 +240,171 @@ static inline unsigned int mii_duplex (unsigned int duplex_lock, } /** + * ethtool_adv_to_mii_100bt + * @ethadv: the ethtool advertisement settings + * + * A small helper function that translates ethtool advertisement + * settings to phy autonegotiation advertisements for the + * MII_ADVERTISE register. + */ +static inline u32 ethtool_adv_to_mii_100bt(u32 ethadv) +{ + u32 result = 0; + + if (ethadv & ADVERTISED_10baseT_Half) + result |= ADVERTISE_10HALF; + if (ethadv & ADVERTISED_10baseT_Full) + result |= ADVERTISE_10FULL; + if (ethadv & ADVERTISED_100baseT_Half) + result |= ADVERTISE_100HALF; + if (ethadv & ADVERTISED_100baseT_Full) + result |= ADVERTISE_100FULL; + if (ethadv & ADVERTISED_Pause) + result |= ADVERTISE_PAUSE_CAP; + if (ethadv & ADVERTISED_Asym_Pause) + result |= ADVERTISE_PAUSE_ASYM; + + return result; +} + +/** + * mii_adv_to_ethtool_100bt + * @adv: value of the MII_ADVERTISE register + * + * A small helper function that translates MII_ADVERTISE bits + * to ethtool advertisement settings. + */ +static inline u32 mii_adv_to_ethtool_100bt(u32 adv) +{ + u32 result = 0; + + if (adv & ADVERTISE_10HALF) + result |= ADVERTISED_10baseT_Half; + if (adv & ADVERTISE_10FULL) + result |= ADVERTISED_10baseT_Full; + if (adv & ADVERTISE_100HALF) + result |= ADVERTISED_100baseT_Half; + if (adv & ADVERTISE_100FULL) + result |= ADVERTISED_100baseT_Full; + if (adv & ADVERTISE_PAUSE_CAP) + result |= ADVERTISED_Pause; + if (adv & ADVERTISE_PAUSE_ASYM) + result |= ADVERTISED_Asym_Pause; + + return result; +} + +/** + * ethtool_adv_to_mii_1000T + * @ethadv: the ethtool advertisement settings + * + * A small helper function that translates ethtool advertisement + * settings to phy autonegotiation advertisements for the + * MII_CTRL1000 register when in 1000T mode. + */ +static inline u32 ethtool_adv_to_mii_1000T(u32 ethadv) +{ + u32 result = 0; + + if (ethadv & ADVERTISED_1000baseT_Half) + result |= ADVERTISE_1000HALF; + if (ethadv & ADVERTISED_1000baseT_Full) + result |= ADVERTISE_1000FULL; + + return result; +} + +/** + * mii_adv_to_ethtool_1000T + * @adv: value of the MII_CTRL1000 register + * + * A small helper function that translates MII_CTRL1000 + * bits, when in 1000Base-T mode, to ethtool + * advertisement settings. + */ +static inline u32 mii_adv_to_ethtool_1000T(u32 adv) +{ + u32 result = 0; + + if (adv & ADVERTISE_1000HALF) + result |= ADVERTISED_1000baseT_Half; + if (adv & ADVERTISE_1000FULL) + result |= ADVERTISED_1000baseT_Full; + + return result; +} + +#define mii_lpa_to_ethtool_100bt(lpa) mii_adv_to_ethtool_100bt(lpa) + +/** + * mii_lpa_to_ethtool_1000T + * @adv: value of the MII_STAT1000 register + * + * A small helper function that translates MII_STAT1000 + * bits, when in 1000Base-T mode, to ethtool + * advertisement settings. + */ +static inline u32 mii_lpa_to_ethtool_1000T(u32 lpa) +{ + u32 result = 0; + + if (lpa & LPA_1000HALF) + result |= ADVERTISED_1000baseT_Half; + if (lpa & LPA_1000FULL) + result |= ADVERTISED_1000baseT_Full; + + return result; +} + +/** + * ethtool_adv_to_mii_1000X + * @ethadv: the ethtool advertisement settings + * + * A small helper function that translates ethtool advertisement + * settings to phy autonegotiation advertisements for the + * MII_CTRL1000 register when in 1000Base-X mode. + */ +static inline u32 ethtool_adv_to_mii_1000X(u32 ethadv) +{ + u32 result = 0; + + if (ethadv & ADVERTISED_1000baseT_Half) + result |= ADVERTISE_1000XHALF; + if (ethadv & ADVERTISED_1000baseT_Full) + result |= ADVERTISE_1000XFULL; + if (ethadv & ADVERTISED_Pause) + result |= ADVERTISE_1000XPAUSE; + if (ethadv & ADVERTISED_Asym_Pause) + result |= ADVERTISE_1000XPSE_ASYM; + + return result; +} + +/** + * mii_adv_to_ethtool_1000X + * @adv: value of the MII_CTRL1000 register + * + * A small helper function that translates MII_CTRL1000 + * bits, when in 1000Base-X mode, to ethtool + * advertisement settings. + */ +static inline u32 mii_adv_to_ethtool_1000X(u32 adv) +{ + u32 result = 0; + + if (adv & ADVERTISE_1000XHALF) + result |= ADVERTISED_1000baseT_Half; + if (adv & ADVERTISE_1000XFULL) + result |= ADVERTISED_1000baseT_Full; + if (adv & ADVERTISE_1000XPAUSE) + result |= ADVERTISED_Pause; + if (adv & ADVERTISE_1000XPSE_ASYM) + result |= ADVERTISED_Asym_Pause; + + return result; +} + +/** * mii_advertise_flowctrl - get flow control advertisement flags * @cap: Flow control capabilities (FLOW_CTRL_RX, FLOW_CTRL_TX or both) */