Messages in this thread Patch in this message | | | Date | Mon, 28 Nov 2022 23:22:55 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net v2 2/2] phy: aquantia: Determine rate adaptation support from registers |
| |
On Mon, Nov 28, 2022 at 02:54:09PM -0500, Sean Anderson wrote: > When autonegotiation completes, the phy interface will be set based on > the global config register for that speed. If the SERDES mode is set to > something which the MAC does not support, then the link will not come > up. To avoid this, validate each combination of interface speed and link > speed which might be configured. This way, we ensure that we only > consider rate adaptation in our advertisement when we can actually use > it. > > The API for get_rate_matching requires that PHY_INTERFACE_MODE_NA be > handled properly. To do this, we adopt a structure similar to > phylink_validate.
Note that this has all but gone away except for a few legacy cases with the advent of the supported_interfaces bitmap.
Also note that phy_get_rate_matching() will not be called by phylink with PHY_INTERFACE_MODE_NA since my recent commit (7642cc28fd37 "net: phylink: fix PHY validation with rate adaption"), and phylink is currently the only user of this interface.
> At the top-level, we either validate a particular > interface speed or all of them. Below that, we validate each combination > of serdes speed and link speed. > > For some firmwares, not all speeds are supported. In this case, the > global config register for that speed will be initialized to zero > (indicating that rate adaptation is not supported). We can detect this > by reading the PMA/PMD speed register to determine which speeds are > supported. This register is read once in probe and cached for later. > > Fixes: 3c42563b3041 ("net: phy: aquantia: Add support for rate matching") > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > This commit should not get backported until it soaks in master for a > while.
You will have to monitor the emails from stable to achieve that - as you have a Fixes tag, that will trigger it to be picked up fairly quicky.
> #define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0 > #define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1 > #define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE 2 > +#define VEND1_GLOBAL_CFG_SERDES_MODE GENMASK(2, 0) > +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI 0 > +#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3 > +#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4 > +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6 > +#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G 7 > > #define VEND1_GLOBAL_RSVD_STAT1 0xc885 > #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID GENMASK(7, 4) > @@ -173,6 +179,7 @@ static const struct aqr107_hw_stat aqr107_hw_stats[] = { > > struct aqr107_priv { > u64 sgmii_stats[AQR107_SGMII_STAT_SZ]; > + int supported_speeds; > }; > > static int aqr107_get_sset_count(struct phy_device *phydev) > @@ -675,13 +682,141 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev) > return 0; > } > > +/** > + * aqr107_rate_adapt_ok_one() - Validate rate adaptation for one configuration > + * @phydev: The phy to act on > + * @serdes_speed: The speed of the serdes (aka the phy interface) > + * @link_speed: The speed of the link > + * > + * This function validates whether rate adaptation will work for a particular > + * combination of @serdes_speed and @link_speed. > + * > + * Return: %true if the global config register for @link_speed is configured for > + * rate adaptation, %true if @link_speed will not be advertised, %false > + * otherwise. > + */ > +static bool aqr107_rate_adapt_ok_one(struct phy_device *phydev, int serdes_speed, > + int link_speed) > +{ > + struct aqr107_priv *priv = phydev->priv; > + int val, speed_bit; > + u32 reg; > + > + phydev_dbg(phydev, "validating link_speed=%d serdes_speed=%d\n", > + link_speed, serdes_speed); > + > + switch (link_speed) { > + case SPEED_10000: > + reg = VEND1_GLOBAL_CFG_10G; > + speed_bit = MDIO_SPEED_10G; > + break; > + case SPEED_5000: > + reg = VEND1_GLOBAL_CFG_5G; > + speed_bit = MDIO_PCS_SPEED_5G; > + break; > + case SPEED_2500: > + reg = VEND1_GLOBAL_CFG_2_5G; > + speed_bit = MDIO_PCS_SPEED_2_5G; > + break; > + case SPEED_1000: > + reg = VEND1_GLOBAL_CFG_1G; > + speed_bit = MDIO_PMA_SPEED_1000; > + break; > + case SPEED_100: > + reg = VEND1_GLOBAL_CFG_100M; > + speed_bit = MDIO_PMA_SPEED_100; > + break; > + case SPEED_10: > + reg = VEND1_GLOBAL_CFG_10M; > + speed_bit = MDIO_PMA_SPEED_10; > + break; > + default: > + WARN_ON_ONCE(1); > + return false; > + } > + > + /* Vacuously OK, since we won't advertise it anyway */ > + if (!(priv->supported_speeds & speed_bit)) > + return true;
This doesn't make any sense. priv->supported_speeds is the set of speeds read from the PMAPMD. The only bits that are valid for this are the MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these two definitions:
#define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */ #define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
Note that they are the same value, yet above, you're testing for bit 6 being clear effectively for both 10M and 2.5G speeds. I suspect this is *not* what you want.
MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1). MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
> + > + val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg); > + if (val < 0) { > + phydev_warn(phydev, "could not read register %x:%.04x (err = %d)\n", > + MDIO_MMD_VEND1, reg, val); > + return false; > + } > + > + phydev_dbg(phydev, "%x:%.04x = %.04x\n", MDIO_MMD_VEND1, reg, val); > + if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) != > + VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE) > + return false; > + > + switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, val)) { > + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G: > + return serdes_speed == SPEED_20000; > + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI: > + return serdes_speed == SPEED_10000; > + case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G: > + return serdes_speed == SPEED_5000; > + case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII: > + return serdes_speed == SPEED_2500; > + case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII: > + return serdes_speed == SPEED_1000; > + default: > + return false; > + } > +} > + > +/** > + * aqr107_rate_adapt_ok() - Validate rate adaptation for an interface speed > + * @phydev: The phy device > + * @speed: The serdes (phy interface) speed > + * > + * This validates whether rate adaptation will work for a particular @speed. > + * All link speeds less than or equal to @speed are validate to ensure they are > + * configured properly. > + * > + * Return: %true if rate adaptation is supported for @speed, %false otherwise. > + */ > +static bool aqr107_rate_adapt_ok(struct phy_device *phydev, int speed) > +{ static int speeds[] = { SPEED_10, SPEED_100, SPEED_1000, SPEED_2500, SPEED_5000, SPEED_10000, }; int i;
for (i = 0; i < ARRAY_SIZE(speeds) && speeds[i] <= speed; i++) if (!aqr107_rate_adapt_ok_one(phydev, speed, speeds[i])) return false;
/* speed must be in speeds[] */ if (i == ARRAY_SIZE(speeds) || speeds[i] != speed) return false;
return true;
would be more concise code?
> +} > + > static int aqr107_get_rate_matching(struct phy_device *phydev, > phy_interface_t iface) > { > - if (iface == PHY_INTERFACE_MODE_10GBASER || > - iface == PHY_INTERFACE_MODE_2500BASEX || > - iface == PHY_INTERFACE_MODE_NA) > + if (iface != PHY_INTERFACE_MODE_NA) { > + if (aqr107_rate_adapt_ok(phydev, > + phy_interface_max_speed(iface))) > + return RATE_MATCH_PAUSE; > + else > + return RATE_MATCH_NONE; > + } > + > + if (aqr107_rate_adapt_ok(phydev, SPEED_10000) || > + aqr107_rate_adapt_ok(phydev, SPEED_2500) || > + aqr107_rate_adapt_ok(phydev, SPEED_1000)) > return RATE_MATCH_PAUSE; > + > return RATE_MATCH_NONE; > } > > @@ -711,10 +846,19 @@ static int aqr107_resume(struct phy_device *phydev) > > static int aqr107_probe(struct phy_device *phydev) > { > - phydev->priv = devm_kzalloc(&phydev->mdio.dev, > - sizeof(struct aqr107_priv), GFP_KERNEL); > - if (!phydev->priv) > + struct aqr107_priv *priv; > + > + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > return -ENOMEM; > + phydev->priv = priv; > + > + priv->supported_speeds = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, > + MDIO_SPEED); > + if (priv->supported_speeds < 0) {
Given the above confusion about the MDIO_SPEED register, I'd suggest this isn't simply named "supported_speeds" but "pmapmd_speeds" to indicate that it's the pmapmd mmd speed register.
> + phydev_err(phydev, "could not determine supported speeds\n"); > + return priv->supported_speeds; > + }; > > return aqr_hwmon_probe(phydev); > } > -- > 2.35.1.1320.gc452695387.dirty > >
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |