Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes
From: Christophe JAILLET
Date: Mon May 06 2024 - 16:49:13 EST
Le 06/05/2024 à 16:40, Kamil Horák - 2N a écrit :
Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
Create set of functions alternative to IEEE 802.3 to handle configuration
of these modes on compatible Broadcom PHYs.
Signed-off-by: Kamil Horák - 2N <kamilh@xxxxxxxx>
---
drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
drivers/net/phy/bcm-phy-lib.h | 4 +
drivers/net/phy/broadcom.c | 338 ++++++++++++++++++++++++++++++++--
3 files changed, 449 insertions(+), 15 deletions(-)
Hi,
a few nitpicks below, should it help.
..
+int bcm_config_aneg(struct phy_device *phydev, bool changed)
+{
+ int err;
+
+ if (genphy_config_eee_advert(phydev))
+ changed = true;
+
+ err = bcm_setup_master_slave(phydev);
+ if (err < 0)
+ return err;
+ else if (err)
+ changed = true;
+
+ if (phydev->autoneg != AUTONEG_ENABLE)
+ return bcm_setup_forced(phydev);
+
+ err = bcm_config_advert(phydev);
+ if (err < 0) /* error */
Nitpick: the comment could be removed, IMHO (otherwise maybe it should
be added a few lines above too)
+ return err;
+ else if (err)
+ changed = true;
+
+ return genphy_check_and_restart_aneg(phydev, changed);
+}
+EXPORT_SYMBOL_GPL(bcm_config_aneg);
+
+/**
+ * bcm_config_advert - sanitize and advertise auto-negotiation parameters
+ * @phydev: target phy_device struct
+ *
+ * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
+ * after sanitizing the values to make sure we only advertise
+ * what is supported. Returns < 0 on error, 0 if the PHY's advertisement
+ * hasn't changed, and > 0 if it has changed.
+ */
+int bcm_config_advert(struct phy_device *phydev)
+{
+ int err;
+ u32 adv;
+
+ /* Only allow advertising what this PHY supports */
+ linkmode_and(phydev->advertising, phydev->advertising,
+ phydev->supported);
+
+ adv = bcm_linkmode_adv_to_mii_adv_t(phydev->advertising);
+
+ /* Setup BroadR-Reach mode advertisement */
+ err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
+ LRE_ADVERTISE_ALL | LREANAA_PAUSE |
+ LREANAA_PAUSE_ASYM, adv);
+
+ if (err < 0)
+ return err;
+
+ return err > 0 ? 1 : 0;
Nitpick: Could be: return err;
(at this point it can be only 0 or 1 anyway)
+}
+EXPORT_SYMBOL_GPL(bcm_config_advert);
..
@@ -576,18 +604,16 @@ static int bcm54811_config_init(struct phy_device *phydev)
return err;
}
- return err;
+ /* Configure BroadR-Reach function. */
+ return bcm5481x_set_brrmode(phydev, ETHTOOL_PHY_BRR_MODE_OFF);
Nitpick: 2 spaces before "bcm5481x_set_brrmode"
}
-static int bcm5481_config_aneg(struct phy_device *phydev)
+static int bcm5481x_config_delay_swap(struct phy_device *phydev)
{
struct device_node *np = phydev->mdio.dev.of_node;
- int ret;
-
- /* Aneg firstly. */
- ret = genphy_config_aneg(phydev);
+ int ret = 0;
I think that ret can be left un-initialized and use return 0; at the end
of the function.
- /* Then we can set up the delay. */
+ /* Set up the delay. */
bcm54xx_config_clock_delay(phydev);
if (of_property_read_bool(np, "enet-phy-lane-swap")) {
@@ -601,6 +627,56 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
return ret;
}
..
+static int bcm54811_config_aneg(struct phy_device *phydev)
+{
+ int ret;
+ u8 brr_mode;
+
+ /* Aneg firstly. */
+ ret = bcm5481x_get_brrmode(phydev, &brr_mode);
+ if (ret)
+ return ret;
+
+ if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+ /* BCM54811 is only capable of autonegotiation in IEEE mode */
+ if (phydev->autoneg)
+ return -EOPNOTSUPP;
+
+ ret = bcm_config_aneg(phydev, false);
+
Nitpick: unneeded new line
+ } else {
+ ret = genphy_config_aneg(phydev);
+ }
+
+ if (ret)
+ return ret;
+
+ /* Then we can set up the delay and swap. */
+ return bcm5481x_config_delay_swap(phydev);
+}
+
struct bcm54616s_phy_priv {
bool mode_1000bx_en;
};
@@ -1062,6 +1138,234 @@ static void bcm54xx_link_change_notify(struct phy_device *phydev)
bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_EXP08, ret);
}
+static int bcm54811_read_abilities(struct phy_device *phydev)
+{
+ int val, err;
+ int i;
+ static const int modes_array[] = {ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+ ETHTOOL_LINK_MODE_1BR10_BIT,
+ ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_100baseT_Half_BIT,
+ ETHTOOL_LINK_MODE_10baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseT_Half_BIT};
+
Nitpick: unneeded new line. Or maybe brr_mode could be defined above
this struct?
Should there be an extra space after { and before }?
(see br_bits below)
+ u8 brr_mode;
+
+ for (i = 0; i < ARRAY_SIZE(modes_array); i++)
+ linkmode_clear_bit(modes_array[i], phydev->supported);
+
+ err = bcm5481x_get_brrmode(phydev, &brr_mode);
+
Nitpick: unneeded new line
+ if (err)
+ return err;
+
+ if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+ linkmode_set_bit_array(phy_basic_ports_array,
+ ARRAY_SIZE(phy_basic_ports_array),
+ phydev->supported);
+
+ val = phy_read(phydev, MII_BCM54XX_LRESR);
+ if (val < 0)
+ return val;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+ phydev->supported, 1);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+ phydev->supported,
+ val & LRESR_100_1PAIR);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
+ phydev->supported,
+ val & LRESR_10_1PAIR);
+ } else {
+ return genphy_read_abilities(phydev);
+ }
+
+ return err;
Nitpick: return 0; ?
+}
..
+/* Read LDS Link Partner Ability in BroadR-Reach mode */
+static int bcm_read_lpa(struct phy_device *phydev)
+{
+ int i, lrelpa;
+
+ if (phydev->autoneg != AUTONEG_ENABLE) {
+ if (!phydev->autoneg_complete) {
+ /* aneg not yet done, reset all relevant bits */
+ static int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,
Nitpick: add const? (but maybe the line would get too long)
+ ETHTOOL_LINK_MODE_Pause_BIT,
+ ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ ETHTOOL_LINK_MODE_1BR10_BIT,
+ ETHTOOL_LINK_MODE_100baseT1_Full_BIT };
+ for (i = 0; i < ARRAY_SIZE(br_bits); i++)
+ linkmode_clear_bit(br_bits[i], phydev->lp_advertising);
+
+ return 0;
+ }
+
+ /* Long-Distance-Signalling Link Partner Ability */
Typo? Signaling?
+ lrelpa = phy_read(phydev, MII_BCM54XX_LRELPA);
+ if (lrelpa < 0)
+ return lrelpa;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+ phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1BR10_BIT,
+ phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);
+ } else {
+ linkmode_zero(phydev->lp_advertising);
+ }
+
+ return 0;
+}
..
CJ