| Message ID | 20220715215954.1449214-9-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dpaa: Convert to phylink | expand |
> +/** > + * phy_interface_speed() - get the speed of a phy interface > + * @interface: phy interface mode defined by &typedef phy_interface_t > + * @link_speed: the speed of the link > + * > + * Some phy interfaces modes adapt to the speed of the underlying link (such as > + * by duplicating data or changing the clock rate). Others, however, are fixed > + * at a particular rate. Determine the speed of a phy interface mode for a > + * particular link speed. > + * > + * Return: The speed of @interface > + */ > +static int phy_interface_speed(phy_interface_t interface, int link_speed) > +{ > + switch (interface) { > + case PHY_INTERFACE_MODE_100BASEX: > + return SPEED_100; > + > + case PHY_INTERFACE_MODE_TBI: > + case PHY_INTERFACE_MODE_MOCA: > + case PHY_INTERFACE_MODE_RTBI: > + case PHY_INTERFACE_MODE_1000BASEX: > + case PHY_INTERFACE_MODE_1000BASEKX: > + case PHY_INTERFACE_MODE_TRGMII: > + return SPEED_1000; > + > + case PHY_INTERFACE_MODE_2500BASEX: > + return SPEED_2500; > + > + case PHY_INTERFACE_MODE_5GBASER: > + return SPEED_5000; > + > + case PHY_INTERFACE_MODE_XGMII: > + case PHY_INTERFACE_MODE_RXAUI: > + case PHY_INTERFACE_MODE_XAUI: > + case PHY_INTERFACE_MODE_10GBASER: > + case PHY_INTERFACE_MODE_10GKR: > + return SPEED_10000; > + > + case PHY_INTERFACE_MODE_25GBASER: > + return SPEED_25000; > + > + case PHY_INTERFACE_MODE_XLGMII: > + return SPEED_40000; > + > + case PHY_INTERFACE_MODE_USXGMII: > + case PHY_INTERFACE_MODE_RGMII_TXID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + case PHY_INTERFACE_MODE_RGMII_ID: > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_QSGMII: > + case PHY_INTERFACE_MODE_SGMII: > + case PHY_INTERFACE_MODE_GMII: > + case PHY_INTERFACE_MODE_REVRMII: > + case PHY_INTERFACE_MODE_RMII: > + case PHY_INTERFACE_MODE_SMII: > + case PHY_INTERFACE_MODE_REVMII: > + case PHY_INTERFACE_MODE_MII: > + case PHY_INTERFACE_MODE_INTERNAL: > + return link_speed; > + > + case PHY_INTERFACE_MODE_NA: > + case PHY_INTERFACE_MODE_MAX: > + break; > + } > + > + return SPEED_UNKNOWN; This seem error prone when new PHY_INTERFACE_MODES are added. I would prefer a WARN_ON_ONCE() in the default: so we get to know about such problems. I'm also wondering if we need a sanity check here. I've seen quite a few boards a Fast Ethernet MAC, but a 1G PHY because they are cheap. In such cases, the MAC is supposed to call phy_set_max_speed() to indicate it can only do 100Mbs. PHY_INTERFACE_MODE_MII but a link_speed of 1G is clearly wrong. Are there other cases where we could have a link speed faster than what the interface mode allows? Bike shedding a bit, but would it be better to use host_side_speed and line_side_speed? When you say link_speed, which link are your referring to? Since we are talking about the different sides of the PHY doing different speeds, the naming does need to be clear. Andrew
On 7/16/22 4:06 PM, Andrew Lunn wrote: >> +/** >> + * phy_interface_speed() - get the speed of a phy interface >> + * @interface: phy interface mode defined by &typedef phy_interface_t >> + * @link_speed: the speed of the link >> + * >> + * Some phy interfaces modes adapt to the speed of the underlying link (such as >> + * by duplicating data or changing the clock rate). Others, however, are fixed >> + * at a particular rate. Determine the speed of a phy interface mode for a >> + * particular link speed. >> + * >> + * Return: The speed of @interface >> + */ >> +static int phy_interface_speed(phy_interface_t interface, int link_speed) >> +{ >> + switch (interface) { >> + case PHY_INTERFACE_MODE_100BASEX: >> + return SPEED_100; >> + >> + case PHY_INTERFACE_MODE_TBI: >> + case PHY_INTERFACE_MODE_MOCA: >> + case PHY_INTERFACE_MODE_RTBI: >> + case PHY_INTERFACE_MODE_1000BASEX: >> + case PHY_INTERFACE_MODE_1000BASEKX: >> + case PHY_INTERFACE_MODE_TRGMII: >> + return SPEED_1000; >> + >> + case PHY_INTERFACE_MODE_2500BASEX: >> + return SPEED_2500; >> + >> + case PHY_INTERFACE_MODE_5GBASER: >> + return SPEED_5000; >> + >> + case PHY_INTERFACE_MODE_XGMII: >> + case PHY_INTERFACE_MODE_RXAUI: >> + case PHY_INTERFACE_MODE_XAUI: >> + case PHY_INTERFACE_MODE_10GBASER: >> + case PHY_INTERFACE_MODE_10GKR: >> + return SPEED_10000; >> + >> + case PHY_INTERFACE_MODE_25GBASER: >> + return SPEED_25000; >> + >> + case PHY_INTERFACE_MODE_XLGMII: >> + return SPEED_40000; >> + >> + case PHY_INTERFACE_MODE_USXGMII: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII: >> + case PHY_INTERFACE_MODE_QSGMII: >> + case PHY_INTERFACE_MODE_SGMII: >> + case PHY_INTERFACE_MODE_GMII: >> + case PHY_INTERFACE_MODE_REVRMII: >> + case PHY_INTERFACE_MODE_RMII: >> + case PHY_INTERFACE_MODE_SMII: >> + case PHY_INTERFACE_MODE_REVMII: >> + case PHY_INTERFACE_MODE_MII: >> + case PHY_INTERFACE_MODE_INTERNAL: >> + return link_speed; >> + >> + case PHY_INTERFACE_MODE_NA: >> + case PHY_INTERFACE_MODE_MAX: >> + break; >> + } >> + >> + return SPEED_UNKNOWN; > > This seem error prone when new PHY_INTERFACE_MODES are added. I would > prefer a WARN_ON_ONCE() in the default: so we get to know about such > problems. Actually, this is the reason I did not add a default: clause to the switch (and instead listed everything out). If a new interface mode is added, there will be a warning (as I discovered when preparing this patch). I can still add a warning here if you'd like; the return there should effectively be dead code. > I'm also wondering if we need a sanity check here. I've seen quite a > few boards a Fast Ethernet MAC, but a 1G PHY because they are > cheap. In such cases, the MAC is supposed to call phy_set_max_speed() > to indicate it can only do 100Mbs. PHY_INTERFACE_MODE_MII but a > link_speed of 1G is clearly wrong. Are there other cases where we > could have a link speed faster than what the interface mode allows? AFAIK the phy must report SPEED_100 here, since many MACs set their configuration based on the resolved speed. So if a phy reported SPEED_1000 then the MAC would be confused. > Bike shedding a bit, but would it be better to use host_side_speed and > line_side_speed? When you say link_speed, which link are your > referring to? Since we are talking about the different sides of the > PHY doing different speeds, the naming does need to be clear. When I say "link" I mean the thing that the PMD speaks. That is, one of the ethtool link mode bits. I am thinking of a topology like MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy The way it has been done up to now, the phy interface mode and the link mode have the same speed. For some MIIs, (such as MII or GMII) this is actually the case, since the data clock changes depending on the data speed. For others (SGMII/USXGMII) the data is repeated, but the clock rate stays the same. In particular, the MAC doesn't care what the actual link speed is, just what configuration it has to use (so it selects the right clock etc). The exception to the above is when you have no phy (such as for 1000BASE-X): MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD All of the phy interface modes which can be used this way are "non-adaptive." That is, in the above case they have a fixed speed. That said, I would like to keep the "phy interface mode speed" named "speed" so I don't have to write up a semantic patch to rename it in all the drivers. --- One thing I thought about is that it might be better to set this based on the phy adaptation as well. Something like static void phylink_set_speed(struct phylink_link_state *state) { if (state->rate_adaptation == RATE_ADAPT_NONE) { state->speed = state->link_speed; return; } switch (state->interface) { case PHY_INTERFACE_MODE_REVRMII: case PHY_INTERFACE_MODE_RMII: case PHY_INTERFACE_MODE_SMII: case PHY_INTERFACE_MODE_REVMII: case PHY_INTERFACE_MODE_MII: case PHY_INTERFACE_MODE_100BASEX: state->speed = SPEED_100; return; case PHY_INTERFACE_MODE_RGMII_TXID: case PHY_INTERFACE_MODE_RGMII_RXID: case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_QSGMII: case PHY_INTERFACE_MODE_SGMII: case PHY_INTERFACE_MODE_GMII: case PHY_INTERFACE_MODE_TBI: case PHY_INTERFACE_MODE_MOCA: case PHY_INTERFACE_MODE_RTBI: case PHY_INTERFACE_MODE_1000BASEX: case PHY_INTERFACE_MODE_1000BASEKX: case PHY_INTERFACE_MODE_TRGMII: state->speed = SPEED_1000; return; case PHY_INTERFACE_MODE_2500BASEX: state->speed = SPEED_2500; return; case PHY_INTERFACE_MODE_5GBASER: state->speed = SPEED_5000; return; case PHY_INTERFACE_MODE_USXGMII: case PHY_INTERFACE_MODE_XGMII: case PHY_INTERFACE_MODE_RXAUI: case PHY_INTERFACE_MODE_XAUI: case PHY_INTERFACE_MODE_10GBASER: case PHY_INTERFACE_MODE_10GKR: state->speed = SPEED_10000; return; case PHY_INTERFACE_MODE_25GBASER: state->speed = SPEED_25000; return; case PHY_INTERFACE_MODE_XLGMII: state->speed = SPEED_40000; return; case PHY_INTERFACE_MODE_INTERNAL: state->speed = link_speed; return; case PHY_INTERFACE_MODE_NA: case PHY_INTERFACE_MODE_MAX: state->speed = SPEED_UNKNOWN; return; } WARN(); } The reason being that this would allow for rate adaptation for "rate adapting" phy interface modes such as MII. This would be necessary for things like RATE_ADAPT_CRS modes like 10PASS-TS which always use 100M MII, but have a variable link speed. --Sean
> > This seem error prone when new PHY_INTERFACE_MODES are added. I would > > prefer a WARN_ON_ONCE() in the default: so we get to know about such > > problems. > > Actually, this is the reason I did not add a default: clause to the > switch (and instead listed everything out). If a new interface mode is > added, there will be a warning (as I discovered when preparing this > patch). Ah, the compiler produces a warning. O.K. that is good. Better than an WARN_ON_ONCE at runtime. > > Bike shedding a bit, but would it be better to use host_side_speed and > > line_side_speed? When you say link_speed, which link are your > > referring to? Since we are talking about the different sides of the > > PHY doing different speeds, the naming does need to be clear. > When I say "link" I mean the thing that the PMD speaks. That is, one of > the ethtool link mode bits. I am thinking of a topology like > > > MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy > > The way it has been done up to now, the phy interface mode and the link > mode have the same speed. For some MIIs, (such as MII or GMII) this is > actually the case, since the data clock changes depending on the data > speed. For others (SGMII/USXGMII) the data is repeated, but the clock > rate stays the same. In particular, the MAC doesn't care what the actual > link speed is, just what configuration it has to use (so it selects the > right clock etc). > > The exception to the above is when you have no phy (such as for > 1000BASE-X): > > MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD > > All of the phy interface modes which can be used this way are > "non-adaptive." That is, in the above case they have a fixed speed. > > That said, I would like to keep the "phy interface mode speed" named > "speed" so I don't have to write up a semantic patch to rename it in all > the drivers. So you want phydev->speed to be the host side speed. That leaves the line side speed as a new variable, so it can be called line_side_speed? I just find link_speed ambiguous, and line_side_speed less so. The documentation for phydev->speed needs updating to make it clear it is the host side speed. Andrew
On 7/16/22 9:26 PM, Andrew Lunn wrote: >> > This seem error prone when new PHY_INTERFACE_MODES are added. I would >> > prefer a WARN_ON_ONCE() in the default: so we get to know about such >> > problems. >> >> Actually, this is the reason I did not add a default: clause to the >> switch (and instead listed everything out). If a new interface mode is >> added, there will be a warning (as I discovered when preparing this >> patch). > > Ah, the compiler produces a warning. O.K. that is good. Better than an > WARN_ON_ONCE at runtime. > >> > Bike shedding a bit, but would it be better to use host_side_speed and >> > line_side_speed? When you say link_speed, which link are your >> > referring to? Since we are talking about the different sides of the >> > PHY doing different speeds, the naming does need to be clear. >> When I say "link" I mean the thing that the PMD speaks. That is, one of >> the ethtool link mode bits. I am thinking of a topology like >> >> >> MAC (+PCS) <-- phy interface mode (MII) --> phy <-- link mode --> far-end phy >> >> The way it has been done up to now, the phy interface mode and the link >> mode have the same speed. For some MIIs, (such as MII or GMII) this is >> actually the case, since the data clock changes depending on the data >> speed. For others (SGMII/USXGMII) the data is repeated, but the clock >> rate stays the same. In particular, the MAC doesn't care what the actual >> link speed is, just what configuration it has to use (so it selects the >> right clock etc). >> >> The exception to the above is when you have no phy (such as for >> 1000BASE-X): >> >> MAC (+PCS) <-- MDI --> PMD <-- link mode --> far-end PMD >> >> All of the phy interface modes which can be used this way are >> "non-adaptive." That is, in the above case they have a fixed speed. >> >> That said, I would like to keep the "phy interface mode speed" named >> "speed" so I don't have to write up a semantic patch to rename it in all >> the drivers. > > So you want phydev->speed to be the host side speed. That leaves the > line side speed as a new variable, so it can be called line_side_speed? > > I just find link_speed ambiguous, and line_side_speed less so. I would rather use something with "link" to match up with ETHTOOL_LINK_MODE_*. Ideally "speed" would be something like "interface_speed" to match up with PHY_INTERFACE_MODE_*. > The documentation for phydev->speed needs updating to make it clear it > is the host side speed. OK --Sean
On Sat, Jul 16, 2022 at 10:06:01PM +0200, Andrew Lunn wrote: > This seem error prone when new PHY_INTERFACE_MODES are added. I would > prefer a WARN_ON_ONCE() in the default: so we get to know about such > problems. > > I'm also wondering if we need a sanity check here. I've seen quite a > few boards a Fast Ethernet MAC, but a 1G PHY because they are > cheap. In such cases, the MAC is supposed to call phy_set_max_speed() > to indicate it can only do 100Mbs. PHY_INTERFACE_MODE_MII but a > link_speed of 1G is clearly wrong. Are there other cases where we > could have a link speed faster than what the interface mode allows? Currently, phylink will deal with that situation - the MAC will report that it only supports 10/100, and when the PHY is brought up, the supported/advertisement masks will be restricted to those speeds. > Bike shedding a bit, but would it be better to use host_side_speed and > line_side_speed? When you say link_speed, which link are your > referring to? Since we are talking about the different sides of the > PHY doing different speeds, the naming does need to be clear. Yes, we definitely need that clarification. I am rather worried that we have drivers using ->speed today in their mac_config and we're redefining what that means in this patch. Also, the value that we pass to the *_link_up() calls appears to be the phy <-> (pcs|mac) speed not the media speed. It's also ->speed and ->duplex that we report to the user in the "Link is Up" message, which will be confusing if it always says 10G despite the media link being e.g. 100M.
On 7/18/22 12:06 PM, Russell King (Oracle) wrote: > On Sat, Jul 16, 2022 at 10:06:01PM +0200, Andrew Lunn wrote: >> This seem error prone when new PHY_INTERFACE_MODES are added. I would >> prefer a WARN_ON_ONCE() in the default: so we get to know about such >> problems. >> >> I'm also wondering if we need a sanity check here. I've seen quite a >> few boards a Fast Ethernet MAC, but a 1G PHY because they are >> cheap. In such cases, the MAC is supposed to call phy_set_max_speed() >> to indicate it can only do 100Mbs. PHY_INTERFACE_MODE_MII but a >> link_speed of 1G is clearly wrong. Are there other cases where we >> could have a link speed faster than what the interface mode allows? > > Currently, phylink will deal with that situation - the MAC will report > that it only supports 10/100, and when the PHY is brought up, the > supported/advertisement masks will be restricted to those speeds. > >> Bike shedding a bit, but would it be better to use host_side_speed and >> line_side_speed? When you say link_speed, which link are your >> referring to? Since we are talking about the different sides of the >> PHY doing different speeds, the naming does need to be clear. > > Yes, we definitely need that clarification. > > I am rather worried that we have drivers using ->speed today in their > mac_config and we're redefining what that means in this patch. Well, kind of. Previously, interface speed was defined to be link speed, and both were just "speed". The MAC driver doesn't really care what the link speed is if there is a phy, just how fast the phy interface mode speed is. > Also, > the value that we pass to the *_link_up() calls appears to be the > phy <-> (pcs|mac) speed not the media speed. This is by design, to avoid breaking existing drivers. > It's also ->speed and > ->duplex that we report to the user in the "Link is Up" message, > which will be confusing if it always says 10G despite the media link > being e.g. 100M. > Ah, I should probably change that message as well. The ethtool stuff is already updated by this patch to report the link speed. --Sean
> > I am rather worried that we have drivers using ->speed today in their > > mac_config and we're redefining what that means in this patch. > > Well, kind of. Previously, interface speed was defined to be link speed, > and both were just "speed". The MAC driver doesn't really care what the > link speed is if there is a phy, just how fast the phy interface mode > speed is. I'm not sure that is true. At least for SGMII, the MAC is passed the line side speed, which can be 10, 100, or 1G. The PHY interface mode speed is fixed at 1G, since it is SGMII, but the MAC needs to know if it needs to repeat symbols because the line side speed is lower than the host side speed. Andrew
On 7/18/22 1:28 PM, Andrew Lunn wrote: >> > I am rather worried that we have drivers using ->speed today in their >> > mac_config and we're redefining what that means in this patch. >> >> Well, kind of. Previously, interface speed was defined to be link speed, >> and both were just "speed". The MAC driver doesn't really care what the >> link speed is if there is a phy, just how fast the phy interface mode >> speed is. > > I'm not sure that is true. At least for SGMII, the MAC is passed the > line side speed, which can be 10, 100, or 1G. The PHY interface mode > speed is fixed at 1G, since it is SGMII, but the MAC needs to know if > it needs to repeat symbols because the line side speed is lower than > the host side speed. Right. In this case the phy interface speed is changing, so the MAC needs to know about it. I suppose a more precise definition would be something like > The data rate of the media-independent interface between the MAC and > the phy, without taking into account protocol overhead or flow control, > but including encoding overhead. as opposed to the link speed which is > The data rate of the medium between the local device and the link > partner, without ... --Sean
On Mon, Jul 18, 2022 at 07:28:58PM +0200, Andrew Lunn wrote: > > > I am rather worried that we have drivers using ->speed today in their > > > mac_config and we're redefining what that means in this patch. > > > > Well, kind of. Previously, interface speed was defined to be link speed, > > and both were just "speed". The MAC driver doesn't really care what the > > link speed is if there is a phy, just how fast the phy interface mode > > speed is. > > I'm not sure that is true. At least for SGMII, the MAC is passed the > line side speed, which can be 10, 100, or 1G. The PHY interface mode > speed is fixed at 1G, since it is SGMII, but the MAC needs to know if > it needs to repeat symbols because the line side speed is lower than > the host side speed. ... and passing the SGMII link speed (1G) will break a lot of stuff where the MAC/PCS may need to know the media speed to do the right number of symbol replication. So I don't think we can get away with just saying ->speed is the link speed which will prevent drivers breaking. I don't think it's that simple. Like everything with phylink, all the drivers need to be looked at and tweaked with every damn change, which makes phylink development painfully slow.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index b08716fe22c1..a952cdc7c96e 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -296,6 +296,75 @@ static void phylink_caps_to_linkmodes(unsigned long *linkmodes, } } +/** + * phy_interface_speed() - get the speed of a phy interface + * @interface: phy interface mode defined by &typedef phy_interface_t + * @link_speed: the speed of the link + * + * Some phy interfaces modes adapt to the speed of the underlying link (such as + * by duplicating data or changing the clock rate). Others, however, are fixed + * at a particular rate. Determine the speed of a phy interface mode for a + * particular link speed. + * + * Return: The speed of @interface + */ +static int phy_interface_speed(phy_interface_t interface, int link_speed) +{ + switch (interface) { + case PHY_INTERFACE_MODE_100BASEX: + return SPEED_100; + + case PHY_INTERFACE_MODE_TBI: + case PHY_INTERFACE_MODE_MOCA: + case PHY_INTERFACE_MODE_RTBI: + case PHY_INTERFACE_MODE_1000BASEX: + case PHY_INTERFACE_MODE_1000BASEKX: + case PHY_INTERFACE_MODE_TRGMII: + return SPEED_1000; + + case PHY_INTERFACE_MODE_2500BASEX: + return SPEED_2500; + + case PHY_INTERFACE_MODE_5GBASER: + return SPEED_5000; + + case PHY_INTERFACE_MODE_XGMII: + case PHY_INTERFACE_MODE_RXAUI: + case PHY_INTERFACE_MODE_XAUI: + case PHY_INTERFACE_MODE_10GBASER: + case PHY_INTERFACE_MODE_10GKR: + return SPEED_10000; + + case PHY_INTERFACE_MODE_25GBASER: + return SPEED_25000; + + case PHY_INTERFACE_MODE_XLGMII: + return SPEED_40000; + + case PHY_INTERFACE_MODE_USXGMII: + case PHY_INTERFACE_MODE_RGMII_TXID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_QSGMII: + case PHY_INTERFACE_MODE_SGMII: + case PHY_INTERFACE_MODE_GMII: + case PHY_INTERFACE_MODE_REVRMII: + case PHY_INTERFACE_MODE_RMII: + case PHY_INTERFACE_MODE_SMII: + case PHY_INTERFACE_MODE_REVMII: + case PHY_INTERFACE_MODE_MII: + case PHY_INTERFACE_MODE_INTERNAL: + return link_speed; + + case PHY_INTERFACE_MODE_NA: + case PHY_INTERFACE_MODE_MAX: + break; + } + + return SPEED_UNKNOWN; +} + /** * phylink_get_linkmodes() - get acceptable link modes * @linkmodes: ethtool linkmode mask (must be already initialised) @@ -515,7 +584,7 @@ static int phylink_parse_fixedlink(struct phylink *pl, if (fixed_node) { ret = fwnode_property_read_u32(fixed_node, "speed", &speed); - pl->link_config.speed = speed; + pl->link_config.link_speed = speed; pl->link_config.duplex = DUPLEX_HALF; if (fwnode_property_read_bool(fixed_node, "full-duplex")) @@ -559,7 +628,7 @@ static int phylink_parse_fixedlink(struct phylink *pl, if (!ret) { pl->link_config.duplex = prop[1] ? DUPLEX_FULL : DUPLEX_HALF; - pl->link_config.speed = prop[2]; + pl->link_config.link_speed = prop[2]; if (prop[3]) __set_bit(ETHTOOL_LINK_MODE_Pause_BIT, pl->link_config.lp_advertising); @@ -569,11 +638,13 @@ static int phylink_parse_fixedlink(struct phylink *pl, } } - if (pl->link_config.speed > SPEED_1000 && + if (pl->link_config.link_speed > SPEED_1000 && pl->link_config.duplex != DUPLEX_FULL) phylink_warn(pl, "fixed link specifies half duplex for %dMbps link?\n", - pl->link_config.speed); + pl->link_config.link_speed); + pl->link_config.speed = phy_interface_speed(pl->link_config.interface, + pl->link_config.link_speed); bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS); linkmode_copy(pl->link_config.advertising, pl->supported); phylink_validate(pl, pl->supported, &pl->link_config); @@ -1270,7 +1341,8 @@ struct phylink *phylink_create(struct phylink_config *config, pl->link_port = PORT_MII; pl->link_config.interface = iface; pl->link_config.pause = MLO_PAUSE_AN; - pl->link_config.speed = SPEED_UNKNOWN; + pl->link_config.link_speed = SPEED_UNKNOWN; + pl->link_config.speed = phy_interface_speed(iface, SPEED_UNKNOWN); pl->link_config.duplex = DUPLEX_UNKNOWN; pl->link_config.an_enabled = true; pl->mac_ops = mac_ops; @@ -1335,7 +1407,9 @@ static void phylink_phy_change(struct phy_device *phydev, bool up) phy_get_pause(phydev, &tx_pause, &rx_pause); mutex_lock(&pl->state_mutex); - pl->phy_state.speed = phydev->speed; + pl->phy_state.link_speed = phydev->speed; + pl->phy_state.speed = phy_interface_speed(phydev->interface, + phydev->speed); pl->phy_state.duplex = phydev->duplex; pl->phy_state.pause = MLO_PAUSE_NONE; if (tx_pause) @@ -1413,7 +1487,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, pl->phydev = phy; pl->phy_state.interface = interface; pl->phy_state.pause = MLO_PAUSE_NONE; - pl->phy_state.speed = SPEED_UNKNOWN; + pl->phy_state.link_speed = SPEED_UNKNOWN; + pl->phy_state.speed = phy_interface_speed(interface, SPEED_UNKNOWN); pl->phy_state.duplex = DUPLEX_UNKNOWN; linkmode_copy(pl->supported, supported); linkmode_copy(pl->link_config.advertising, config.advertising); @@ -1857,7 +1932,7 @@ static void phylink_get_ksettings(const struct phylink_link_state *state, { phylink_merge_link_mode(kset->link_modes.advertising, state->advertising); linkmode_copy(kset->link_modes.lp_advertising, state->lp_advertising); - kset->base.speed = state->speed; + kset->base.speed = state->link_speed; kset->base.duplex = state->duplex; kset->base.autoneg = state->an_enabled ? AUTONEG_ENABLE : AUTONEG_DISABLE; @@ -1974,13 +2049,13 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, * If the link parameters match, accept them but do nothing. */ if (pl->cur_link_an_mode == MLO_AN_FIXED) { - if (s->speed != pl->link_config.speed || + if (s->speed != pl->link_config.link_speed || s->duplex != pl->link_config.duplex) return -EINVAL; return 0; } - config.speed = s->speed; + config.link_speed = s->speed; config.duplex = s->duplex; break; @@ -1996,7 +2071,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, return 0; } - config.speed = SPEED_UNKNOWN; + config.link_speed = SPEED_UNKNOWN; config.duplex = DUPLEX_UNKNOWN; break; @@ -2046,7 +2121,10 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, if (config.an_enabled && phylink_is_empty_linkmode(config.advertising)) return -EINVAL; + config.speed = phy_interface_speed(config.interface, config.link_speed); + mutex_lock(&pl->state_mutex); + pl->link_config.link_speed = config.link_speed; pl->link_config.speed = config.speed; pl->link_config.duplex = config.duplex; pl->link_config.an_enabled = config.an_enabled; @@ -2291,7 +2369,7 @@ static int phylink_mii_emul_read(unsigned int reg, int val; fs.link = state->link; - fs.speed = state->speed; + fs.speed = state->link_speed; fs.duplex = state->duplex; fs.pause = test_bit(ETHTOOL_LINK_MODE_Pause_BIT, lpa); fs.asym_pause = test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, lpa); @@ -2588,7 +2666,8 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode, memset(&config, 0, sizeof(config)); linkmode_copy(config.advertising, advertising); config.interface = PHY_INTERFACE_MODE_NA; - config.speed = SPEED_UNKNOWN; + config.link_speed = SPEED_UNKNOWN; + config.speed = PHY_INTERFACE_MODE_NA; config.duplex = DUPLEX_UNKNOWN; config.pause = MLO_PAUSE_AN; config.an_enabled = pl->link_config.an_enabled; diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 6d06896fc20d..30e3fbe19fb4 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -56,7 +56,10 @@ static inline bool phylink_autoneg_inband(unsigned int mode) * @lp_advertising: ethtool bitmask containing link partner advertised link * modes * @interface: link &typedef phy_interface_t mode - * @speed: link speed, one of the SPEED_* constants. + * @speed: interface speed, one of the SPEED_* constants. If + * @rate_adaptation is being performed, this will be different from + * @link_speed. + * @link_speed: link speed, one of the SPEED_* constants. * @duplex: link duplex mode, one of DUPLEX_* constants. * @pause: link pause state, described by MLO_PAUSE_* constants. * @link: true if the link is up. @@ -68,6 +71,7 @@ struct phylink_link_state { __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising); phy_interface_t interface; int speed; + int link_speed; int duplex; int pause; unsigned int link:1;
This adds support for cases when the link speed differs from the speed of the phy interface mode. Such cases can occur when some kind of rate adaptation is occurring. The interface speed is used for the link state speed member. This is done for compatibility with existing drivers. For example, if the link mode is 1000BASE-T, and the phy interface mode is XGMII, then the MAC will expect SPEED_10000 for speed (which is what would have been passed previously). On the other hand, external APIs use link_speed instead of speed. This is so the speed reported to userspace matches the resolved link mode. phy_interface_speed assumes that certain interfaces adapt to the link rate and others do not. Generally, interface speed adaptation occurs by changing the clock rate (such as for MII), or by repeating symbols (such as for SGMII). This assumptation precludes using rate adaptation for interfaces which already adapt their speed. For example, a phy which performed rate adaptation with GMII (keeping the frequency at 125MHz) would not be supported. Although speed is one of the more prominent ways the link mode can differ from the phy interface mode, they can also differ in duplex. With pause-based rate adaptation, both the interface and link must be full duplex. However, with CRS-based rate adaptation, the interface must be half duplex, but the link mode may be full duplex. This can occur with 10PASS-TS and 2BASE-TL. In these cases, ethtool will report the "wrong" duplex. To fix this, a similar process could be performed for duplex. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- Changes in v3: - New drivers/net/phy/phylink.c | 105 +++++++++++++++++++++++++++++++++----- include/linux/phylink.h | 6 ++- 2 files changed, 97 insertions(+), 14 deletions(-)