PATCH 1 - Add the 1BR10 link mode and capability to switch to
BroadR-Reach as a PHY tunable value
PATCH 2 - Add the definitions of LRE registers, necessary to use
BroadR-Reach modes on the BCM5481x PHY
PATCH 3 - Implementation of the BroadR-Reach modes for the Broadcom
PHYs
Changes in v2:
- Divided into multiple patches, removed useless link modes
Changes in v3:
- Fixed uninitialized variable in bcm5481x_config_delay_swap function
Kamil Horák - 2N (3):
net: phy: bcm54811: New link mode for BroadR-Reach
net: phy: bcm54811: Add LRE registers definitions
net: phy: bcm-phy-lib: Implement BroadR-Reach link modes
drivers/net/phy/bcm-phy-lib.c | 122 ++++++++++++
drivers/net/phy/bcm-phy-lib.h | 4 +
drivers/net/phy/broadcom.c | 338 ++++++++++++++++++++++++++++++++--
drivers/net/phy/phy-core.c | 9 +-
include/linux/brcmphy.h | 91 ++++++++-
include/uapi/linux/ethtool.h | 9 +-
net/ethtool/common.c | 7 +
net/ethtool/ioctl.c | 1 +
8 files changed, 560 insertions(+), 21 deletions(-)
--
2.39.2
Introduce new link modes necessary for the BroadR-Reach mode on
bcm5481x PHY by Broadcom and new PHY tunable to choose between
normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
---
drivers/net/phy/phy-core.c | 9 +++++----
include/uapi/linux/ethtool.h | 9 ++++++++-
net/ethtool/common.c | 7 +++++++
net/ethtool/ioctl.c | 1 +
4 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 15f349e5995a..129e223d8985 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -13,10 +13,9 @@
*/
const char *phy_speed_to_str(int speed)
{
- BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
- "Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
- "If a speed or mode has been added please update phy_speed_to_str "
- "and the PHY settings array.\n");
+ BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 103,
+ "Enum ethtool_link_mode_bit_indices and phylib are out of sync. If a speed or mode has been added please update phy_speed_to_str and the PHY settings array.\n"
+ );
switch (speed) {
case SPEED_10:
@@ -265,6 +264,8 @@ static const struct phy_setting settings[] = {
PHY_SETTING( 10, FULL, 10baseT1S_Full ),
PHY_SETTING( 10, HALF, 10baseT1S_Half ),
PHY_SETTING( 10, HALF, 10baseT1S_P2MP_Half ),
+ PHY_SETTING( 10, FULL, 1BR10 ),
+
};
#undef PHY_SETTING
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 041e09c3515d..105432565e6d 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -289,11 +289,18 @@ struct ethtool_tunable {
#define ETHTOOL_PHY_EDPD_NO_TX 0xfffe
#define ETHTOOL_PHY_EDPD_DISABLE 0
+/*
+ * BroadR-Reach Mode Control
+ */
+#define ETHTOOL_PHY_BRR_MODE_ON 1
+#define ETHTOOL_PHY_BRR_MODE_OFF 0
+
enum phy_tunable_id {
ETHTOOL_PHY_ID_UNSPEC,
ETHTOOL_PHY_DOWNSHIFT,
ETHTOOL_PHY_FAST_LINK_DOWN,
ETHTOOL_PHY_EDPD,
+ ETHTOOL_PHY_BRR_MODE,
/*
* Add your fresh new phy tunable attribute above and remember to update
* phy_tunable_strings[] in net/ethtool/common.c
@@ -1845,7 +1852,7 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_10baseT1S_Full_BIT = 99,
ETHTOOL_LINK_MODE_10baseT1S_Half_BIT = 100,
ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT = 101,
-
+ ETHTOOL_LINK_MODE_1BR10_BIT = 102,
/* must be last entry */
__ETHTOOL_LINK_MODE_MASK_NBITS
};
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 6b2a360dcdf0..5e37804958e9 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -98,6 +98,7 @@ phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
[ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift",
[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
[ETHTOOL_PHY_EDPD] = "phy-energy-detect-power-down",
+ [ETHTOOL_PHY_BRR_MODE] = "phy-broadrreach-mode",
};
#define __LINK_MODE_NAME(speed, type, duplex) \
@@ -211,6 +212,7 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
__DEFINE_LINK_MODE_NAME(10, T1S, Full),
__DEFINE_LINK_MODE_NAME(10, T1S, Half),
__DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
+ __DEFINE_SPECIAL_MODE_NAME(1BR10, "1BR10"),
};
static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
@@ -374,6 +376,11 @@ const struct link_mode_info link_mode_params[] = {
__DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
__DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
__DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
+ [ETHTOOL_LINK_MODE_1BR10_BIT] = {
+ .speed = SPEED_10,
+ .lanes = 1,
+ .duplex = DUPLEX_FULL,
+ },
};
static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 5a55270aa86e..9e68c8562fa3 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2722,6 +2722,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
switch (tuna->id) {
case ETHTOOL_PHY_DOWNSHIFT:
case ETHTOOL_PHY_FAST_LINK_DOWN:
+ case ETHTOOL_PHY_BRR_MODE:
if (tuna->len != sizeof(u8) ||
tuna->type_id != ETHTOOL_TUNABLE_U8)
return -EINVAL;
--
2.39.2
Add the definitions of LRE registers for Broadcom BCM5481x PHY
---
include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 90 insertions(+), 1 deletion(-)
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 1394ba302367..9c0b78c1b6fb 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -270,16 +270,105 @@
#define BCM5482_SSD_SGMII_SLAVE 0x15 /* SGMII Slave Register */
#define BCM5482_SSD_SGMII_SLAVE_EN 0x0002 /* Slave mode enable */
#define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
+#define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
+
+/* BroadR-Reach LRE Registers. */
+#define MII_BCM54XX_LRECR 0x00 /* LRE Control Register */
+#define MII_BCM54XX_LRESR 0x01 /* LRE Status Register */
+#define MII_BCM54XX_LREPHYSID1 0x02 /* LRE PHYS ID 1 */
+#define MII_BCM54XX_LREPHYSID2 0x03 /* LRE PHYS ID 2 */
+#define MII_BCM54XX_LREANAA 0x04 /* LDS Auto-Negotiation Advertised Ability */
+#define MII_BCM54XX_LREANAC 0x05 /* LDS Auto-Negotiation Advertised Control */
+#define MII_BCM54XX_LREANPT 0x06 /* LDS Ability Next Page Transmit */
+#define MII_BCM54XX_LRELPA 0x07 /* LDS Link Partner Ability */
+#define MII_BCM54XX_LRELPNPM 0x08 /* LDS Link Partner Next Page Message */
+#define MII_BCM54XX_LRELPNPC 0x09 /* LDS Link Partner Next Page Control */
+#define MII_BCM54XX_LRELDSE 0x0a /* LDS Expansion Register */
+#define MII_BCM54XX_LREES 0x0f /* LRE Extended Status */
+
+/* LRE control register. */
+#define LRECR_RESET 0x8000 /* Reset to default state */
+#define LRECR_LOOPBACK 0x4000 /* Internal Loopback */
+#define LRECR_LDSRES 0x2000 /* Restart LDS Process */
+#define LRECR_LDSEN 0x1000 /* LDS Enable */
+#define LRECR_PDOWN 0x0800 /* Enable low power state */
+#define LRECR_ISOLATE 0x0400 /* Isolate data paths from MII */
+#define LRECR_SPEED100 0x0200 /* Select 100 Mbps */
+#define LRECR_SPEED10 0x0000 /* Select 10 Mbps */
+#define LRECR_4PAIRS 0x0020 /* Select 4 Pairs */
+#define LRECR_2PAIRS 0x0010 /* Select 2 Pairs */
+#define LRECR_1PAIR 0x0000 /* Select 1 Pair */
+#define LRECR_MASTER 0x0008 /* Force Master when LDS disabled */
+#define LRECR_SLAVE 0x0000 /* Force Slave when LDS disabled */
+
+/* LRE status register. */
+#define LRESR_ERCAP 0x0001 /* Ext-reg capability */
+#define LRESR_JCD 0x0002 /* Jabber detected */
+#define LRESR_LSTATUS 0x0004 /* Link status */
+#define LRESR_LDSABILITY 0x0008 /* Can do LDS */
+#define LRESR_8023 0x0010 /* Has IEEE 802.3 Support */
+#define LRESR_LDSCOMPLETE 0x0020 /* LDS Auto-negotiation complete */
+#define LRESR_MFPS 0x0040 /* Can suppress Management Frames Preamble */
+#define LRESR_RESV 0x0080 /* Unused... */
+#define LRESR_ESTATEN 0x0100 /* Extended Status in R15 */
+#define LRESR_10_1PAIR 0x0200 /* Can do 10Mbps 1 Pair */
+#define LRESR_10_2PAIR 0x0400 /* Can do 10Mbps 2 Pairs */
+#define LRESR_100_2PAIR 0x0800 /* Can do 100Mbps 2 Pairs */
+#define LRESR_100_4PAIR 0x1000 /* Can do 100Mbps 4 Pairs */
+#define LRESR_100_1PAIR 0x2000 /* Can do 100Mbps 1 Pair */
+
+/* LDS Auto-Negotiation Advertised Ability. */
+#define LREANAA_PAUSE_ASYM 0x8000 /* Can pause asymmetrically */
+#define LREANAA_PAUSE 0x4000 /* Can pause */
+#define LREANAA_100_1PAIR 0x0020 /* Can do 100Mbps 1 Pair */
+#define LREANAA_100_4PAIR 0x0010 /* Can do 100Mbps 4 Pair */
+#define LREANAA_100_2PAIR 0x0008 /* Can do 100Mbps 2 Pair */
+#define LREANAA_10_2PAIR 0x0004 /* Can do 10Mbps 2 Pair */
+#define LREANAA_10_1PAIR 0x0002 /* Can do 10Mbps 1 Pair */
+
+#define LRE_ADVERTISE_FULL (LREANAA_100_1PAIR | LREANAA_100_4PAIR | \
+ LREANAA_100_2PAIR | LREANAA_10_2PAIR | \
+ LREANAA_10_1PAIR)
+
+#define LRE_ADVERTISE_ALL LRE_ADVERTISE_FULL
+
+/* LDS Link Partner Ability. */
+#define LRELPA_PAUSE_ASYM 0x8000 /* Supports asymmetric pause */
+#define LRELPA_PAUSE 0x4000 /* Supports pause capability */
+#define LRELPA_100_1PAIR 0x0020 /* 100Mbps 1 Pair capable */
+#define LRELPA_100_4PAIR 0x0010 /* 100Mbps 4 Pair capable */
+#define LRELPA_100_2PAIR 0x0008 /* 100Mbps 2 Pair capable */
+#define LRELPA_10_2PAIR 0x0004 /* 10Mbps 2 Pair capable */
+#define LRELPA_10_1PAIR 0x0002 /* 10Mbps 1 Pair capable */
+
+/* LDS Expansion register. */
+#define LDSE_DOWNGRADE 0x8000 /* Can do LDS Speed Downgrade */
+#define LDSE_MASTER 0x4000 /* Master / Slave */
+#define LDSE_PAIRS_MASK 0x3000 /* Pair Count Mask */
+#define LDSE_4PAIRS 0x2000 /* 4 Pairs Connection */
+#define LDSE_2PAIRS 0x1000 /* 2 Pairs Connection */
+#define LDSE_1PAIR 0x0000 /* 1 Pair Connection */
+#define LDSE_CABLEN_MASK 0x0FFF /* Cable Length Mask */
/* BCM54810 Registers */
#define BCM54810_EXP_BROADREACH_LRE_MISC_CTL (MII_BCM54XX_EXP_SEL_ER + 0x90)
#define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0)
#define BCM54810_SHD_CLK_CTL 0x3
#define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9)
+#define BCM54810_SHD_SCR3_TRDDAPD 0x0100
+
+/* BCM54811 Registers */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL (MII_BCM54XX_EXP_SEL_ER + 0x9A)
+/* Access Control Override Enable */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN BIT(15)
+/* Access Control Override Value */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL BIT(14)
+/* Access Control Value */
+#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_VAL BIT(13)
/* BCM54612E Registers */
#define BCM54612E_EXP_SPARE0 (MII_BCM54XX_EXP_SEL_ETC + 0x34)
-#define BCM54612E_LED4_CLK125OUT_EN (1 << 1)
+#define BCM54612E_LED4_CLK125OUT_EN BIT(1)
/* Wake-on-LAN registers */
--
2.39.2
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 <[email protected]>
---
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(-)
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 876f28fd8256..9fa2a20e641f 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -794,6 +794,46 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
return ret;
}
+static int bcm_setup_forced(struct phy_device *phydev)
+{
+ u16 ctl = 0;
+
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ if (phydev->speed == SPEED_100)
+ ctl |= LRECR_SPEED100;
+
+ if (phydev->duplex != DUPLEX_FULL)
+ return -EOPNOTSUPP;
+
+ return phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_SPEED100, ctl);
+}
+
+/**
+ * bcm_linkmode_adv_to_mii_adv_t
+ * @advertising: the linkmode advertisement settings
+ *
+ * A small helper function that translates linkmode advertisement
+ * settings to phy autonegotiation advertisements for the
+ * MII_BCM54XX_LREANAA register.
+ */
+static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
+{
+ u32 result = 0;
+
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_1BR10_BIT, advertising))
+ result |= LREANAA_10_1PAIR;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT, advertising))
+ result |= LREANAA_100_1PAIR;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising))
+ result |= LRELPA_PAUSE;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising))
+ result |= LRELPA_PAUSE_ASYM;
+
+ return result;
+}
+
int bcm_phy_cable_test_start(struct phy_device *phydev)
{
return _bcm_phy_cable_test_start(phydev, false);
@@ -1066,6 +1106,88 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev,
}
EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
+int bcm_setup_master_slave(struct phy_device *phydev)
+{
+ u16 ctl = 0;
+
+ switch (phydev->master_slave_set) {
+ case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+ case MASTER_SLAVE_CFG_MASTER_FORCE:
+ ctl = LRECR_MASTER;
+ break;
+ case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+ case MASTER_SLAVE_CFG_SLAVE_FORCE:
+ break;
+ case MASTER_SLAVE_CFG_UNKNOWN:
+ case MASTER_SLAVE_CFG_UNSUPPORTED:
+ return 0;
+ default:
+ phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+ return -EOPNOTSUPP;
+ }
+
+ return phy_modify_changed(phydev, MII_BCM54XX_LRECR, LRECR_MASTER, ctl);
+}
+EXPORT_SYMBOL_GPL(bcm_setup_master_slave);
+
+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 */
+ 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;
+}
+EXPORT_SYMBOL_GPL(bcm_config_advert);
+
MODULE_DESCRIPTION("Broadcom PHY Library");
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index b52189e45a84..0f6d06c0b7af 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -121,4 +121,8 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
int bcm_phy_led_brightness_set(struct phy_device *phydev,
u8 index, enum led_brightness value);
+int bcm_setup_master_slave(struct phy_device *phydev);
+int bcm_config_aneg(struct phy_device *phydev, bool changed);
+int bcm_config_advert(struct phy_device *phydev);
+
#endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 370e4ed45098..2d8898fd2228 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -553,18 +553,46 @@ static int bcm54810_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
return -EOPNOTSUPP;
}
-static int bcm54811_config_init(struct phy_device *phydev)
+static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
{
- int err, reg;
+ int reg;
- /* Disable BroadR-Reach function. */
reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
- reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
- err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
- reg);
- if (err < 0)
+
+ *data = (reg & BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN) ?
+ ETHTOOL_PHY_BRR_MODE_ON : ETHTOOL_PHY_BRR_MODE_OFF;
+
+ return 0;
+}
+
+static int bcm5481x_set_brrmode(struct phy_device *phydev, u8 on)
+{
+ int reg;
+ int err;
+
+ reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
+
+ if (on)
+ reg |= BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
+ else
+ reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
+
+ err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, reg);
+ if (err)
return err;
+ /* Ensure LRE or IEEE register set is accessed according to the brr on/off,
+ * thus set the override
+ */
+ return bcm_phy_write_exp(phydev, BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL,
+ BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN |
+ on ? 0 : BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL);
+}
+
+static int bcm54811_config_init(struct phy_device *phydev)
+{
+ int err, reg;
+
err = bcm54xx_config_init(phydev);
/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
@@ -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);
}
-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;
- /* 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 bcm5481_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)
+ ret = bcm_config_aneg(phydev, false);
+ 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);
+}
+
+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);
+
+ } 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};
+
+ 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);
+
+ 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;
+}
+
+static int bcm5481x_get_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, void *data)
+{
+ switch (tuna->id) {
+ case ETHTOOL_PHY_BRR_MODE:
+ return bcm5481x_get_brrmode(phydev, data);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int bcm5481x_set_tunable(struct phy_device *phydev,
+ struct ethtool_tunable *tuna, const void *data)
+{
+ int res;
+
+ switch (tuna->id) {
+ case ETHTOOL_PHY_BRR_MODE:
+ res = bcm5481x_set_brrmode(phydev, *(const u8 *)data);
+ if (res >= 0)
+ res = bcm54811_read_abilities(phydev);
+ break;
+ default:
+ res = -EOPNOTSUPP;
+ }
+
+ return res;
+}
+
+static int bcm_read_master_slave(struct phy_device *phydev)
+{
+ int cfg, state;
+ int val;
+
+ /* In BroadR-Reach mode we are always capable of master-slave
+ * and there is no preferred master or slave configuration
+ */
+ phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+ phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+ val = phy_read(phydev, MII_BCM54XX_LRECR);
+ if (val < 0)
+ return val;
+
+ if ((val & LRECR_LDSEN) == 0) {
+ if (val & LRECR_MASTER)
+ cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
+ else
+ cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
+ }
+
+ val = phy_read(phydev, MII_BCM54XX_LRELDSE);
+ if (val < 0)
+ return val;
+
+ if (val & LDSE_MASTER)
+ state = MASTER_SLAVE_STATE_MASTER;
+ else
+ state = MASTER_SLAVE_STATE_SLAVE;
+
+ phydev->master_slave_get = cfg;
+ phydev->master_slave_state = state;
+
+ 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,
+ 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 */
+ 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;
+}
+
+static int bcm_read_status_fixed(struct phy_device *phydev)
+{
+ int lrecr = phy_read(phydev, MII_BCM54XX_LRECR);
+
+ if (lrecr < 0)
+ return lrecr;
+
+ phydev->duplex = DUPLEX_FULL;
+
+ if (lrecr & LRECR_SPEED100)
+ phydev->speed = SPEED_100;
+ else
+ phydev->speed = SPEED_10;
+
+ return 0;
+}
+
+static int bcm54811_read_status(struct phy_device *phydev)
+{
+ int err;
+ u8 brr_mode;
+
+ err = bcm5481x_get_brrmode(phydev, &brr_mode);
+
+ if (err)
+ return err;
+
+ if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON) {
+ /* Get the status in BroadRReach mode just like genphy_read_status
+ * does in normal mode
+ */
+
+ int err, old_link = phydev->link;
+
+ /* Update the link, but return if there was an error
+ * genphy_update_link() functions equally on IEEE and LRE
+ * register set
+ */
+
+ err = genphy_update_link(phydev);
+ if (err)
+ return err;
+
+ /* why bother the PHY if nothing can have changed */
+ if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+ return 0;
+
+ phydev->speed = SPEED_UNKNOWN;
+ phydev->duplex = DUPLEX_UNKNOWN;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ err = bcm_read_master_slave(phydev);
+ if (err < 0)
+ return err;
+
+ /* Read LDS Link Partner Ability */
+ err = bcm_read_lpa(phydev);
+ if (err < 0)
+ return err;
+
+ if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+ phy_resolve_aneg_linkmode(phydev);
+ } else if (phydev->autoneg == AUTONEG_DISABLE) {
+ err = bcm_read_status_fixed(phydev);
+ if (err < 0)
+ return err;
+ }
+ } else {
+ err = genphy_read_status(phydev);
+ }
+
+ return err;
+}
+
static struct phy_driver broadcom_drivers[] = {
{
.phy_id = PHY_ID_BCM5411,
@@ -1212,9 +1516,13 @@ static struct phy_driver broadcom_drivers[] = {
.get_stats = bcm54xx_get_stats,
.probe = bcm54xx_phy_probe,
.config_init = bcm54811_config_init,
- .config_aneg = bcm5481_config_aneg,
+ .config_aneg = bcm54811_config_aneg,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
+ .read_status = bcm54811_read_status,
+ .get_tunable = bcm5481x_get_tunable,
+ .set_tunable = bcm5481x_set_tunable,
+ .get_features = bcm54811_read_abilities,
.suspend = bcm54xx_suspend,
.resume = bcm54xx_resume,
.link_change_notify = bcm54xx_link_change_notify,
--
2.39.2
On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Hor?k - 2N wrote:
> Introduce new link modes necessary for the BroadR-Reach mode on
> bcm5481x PHY by Broadcom and new PHY tunable to choose between
> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
I would of split this into two patches. The reason being, we need the
new link mode. But do we need the tunable? Why don't i just use the
link mode to select it?
ethtool -s eth42 advertise 1BR10
Once you have split this up, you can explain the link mode patch in a
bit more detail. That because the name does not fit 802.3, the normal
macros cannot be used, so everything needs to be hand crafted.
Andrew
---
pw-bot: cr
On Mon, May 06, 2024 at 04:40:14PM +0200, Kamil Hor?k - 2N wrote:
> Add the definitions of LRE registers for Broadcom BCM5481x PHY
> ---
> include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 1394ba302367..9c0b78c1b6fb 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -270,16 +270,105 @@
> #define BCM5482_SSD_SGMII_SLAVE 0x15 /* SGMII Slave Register */
> #define BCM5482_SSD_SGMII_SLAVE_EN 0x0002 /* Slave mode enable */
> #define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
> +#define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
> +
> +/* BroadR-Reach LRE Registers. */
> +#define MII_BCM54XX_LRECR 0x00 /* LRE Control Register */
> +#define MII_BCM54XX_LRESR 0x01 /* LRE Status Register */
> +#define MII_BCM54XX_LREPHYSID1 0x02 /* LRE PHYS ID 1 */
> +#define MII_BCM54XX_LREPHYSID2 0x03 /* LRE PHYS ID 2 */
> +#define MII_BCM54XX_LREANAA 0x04 /* LDS Auto-Negotiation Advertised Ability */
> +#define MII_BCM54XX_LREANAC 0x05 /* LDS Auto-Negotiation Advertised Control */
> +#define MII_BCM54XX_LREANPT 0x06 /* LDS Ability Next Page Transmit */
> +#define MII_BCM54XX_LRELPA 0x07 /* LDS Link Partner Ability */
> +#define MII_BCM54XX_LRELPNPM 0x08 /* LDS Link Partner Next Page Message */
> +#define MII_BCM54XX_LRELPNPC 0x09 /* LDS Link Partner Next Page Control */
> +#define MII_BCM54XX_LRELDSE 0x0a /* LDS Expansion Register */
> +#define MII_BCM54XX_LREES 0x0f /* LRE Extended Status */
Please look at these side by side to standard C22 registers. Which are
different? Is LRECR actually different to BMCR that it needs it own
define? Is LRESR different enought to BMSR that it needs its own
define? Does the ID registers use a different format?
> +/* LRE control register. */
> +#define LRECR_RESET 0x8000 /* Reset to default state */
> +#define LRECR_LOOPBACK 0x4000 /* Internal Loopback */
> +#define LRECR_LDSRES 0x2000 /* Restart LDS Process */
> +#define LRECR_LDSEN 0x1000 /* LDS Enable */
> +#define LRECR_PDOWN 0x0800 /* Enable low power state */
> +#define LRECR_ISOLATE 0x0400 /* Isolate data paths from MII */
> +#define LRECR_SPEED100 0x0200 /* Select 100 Mbps */
> +#define LRECR_SPEED10 0x0000 /* Select 10 Mbps */
> +#define LRECR_4PAIRS 0x0020 /* Select 4 Pairs */
> +#define LRECR_2PAIRS 0x0010 /* Select 2 Pairs */
> +#define LRECR_1PAIR 0x0000 /* Select 1 Pair */
> +#define LRECR_MASTER 0x0008 /* Force Master when LDS disabled */
> +#define LRECR_SLAVE 0x0000 /* Force Slave when LDS disabled */
Reverse the order of these, and then compare them to:
/* Basic mode control register. */
#define BMCR_SPEED1000 0x0040 /* MSB of Speed (1000) */
#define BMCR_CTST 0x0080 /* Collision test */
#define BMCR_FULLDPLX 0x0100 /* Full duplex */
#define BMCR_ANRESTART 0x0200 /* Auto negotiation restart */
#define BMCR_ISOLATE 0x0400 /* Isolate data paths from MII */
#define BMCR_PDOWN 0x0800 /* Enable low power state */
#define BMCR_ANENABLE 0x1000 /* Enable auto negotiation */
#define BMCR_SPEED100 0x2000 /* Select 100Mbps */
#define BMCR_LOOPBACK 0x4000 /* TXD loopback bits */
#define BMCR_RESET 0x8000 /* Reset to default state */
Drop any which are the same, and just add defined for those which are
different.
> +
> +/* LRE status register. */
> +#define LRESR_ERCAP 0x0001 /* Ext-reg capability */
> +#define LRESR_JCD 0x0002 /* Jabber detected */
> +#define LRESR_LSTATUS 0x0004 /* Link status */
> +#define LRESR_LDSABILITY 0x0008 /* Can do LDS */
What does LDS mean? In BMSR this bit is about auto-neg. How does LDS
differ from auto-neg?
Ideally, where the functionality is the same, please use the existing
definitions. It makes it easier for somebody who knows C22 to look at
the code and understand it. And it makes it easier to spot where the
differences actually are.
Andrew
On 5/6/24 07:40, Kamil Horák - 2N wrote:
> Introduce new link modes necessary for the BroadR-Reach mode on
> bcm5481x PHY by Broadcom and new PHY tunable to choose between
> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
Missing Signed-off-by tag.
> ---
> drivers/net/phy/phy-core.c | 9 +++++----
> include/uapi/linux/ethtool.h | 9 ++++++++-
> net/ethtool/common.c | 7 +++++++
> net/ethtool/ioctl.c | 1 +
> 4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 15f349e5995a..129e223d8985 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -13,10 +13,9 @@
> */
> const char *phy_speed_to_str(int speed)
> {
> - BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
> - "Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
> - "If a speed or mode has been added please update phy_speed_to_str "
> - "and the PHY settings array.\n");
> + BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 103,
> + "Enum ethtool_link_mode_bit_indices and phylib are out of sync. If a speed or mode has been added please update phy_speed_to_str and the PHY settings array.\n"
> + );
Unrelated change. I like Andrew's suggestion to key off advertising
1BR10 to enable BroadR-Reach, but let's see what this discussion leads to.
--
Florian
On Mon, May 06, 2024 at 04:40:15PM +0200, Kamil Hor?k - 2N wrote:
> 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 <[email protected]>
> ---
> 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(-)
>
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..9fa2a20e641f 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -794,6 +794,46 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
> return ret;
> }
>
> +static int bcm_setup_forced(struct phy_device *phydev)
> +{
> + u16 ctl = 0;
> +
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + if (phydev->speed == SPEED_100)
> + ctl |= LRECR_SPEED100;
> +
> + if (phydev->duplex != DUPLEX_FULL)
> + return -EOPNOTSUPP;
Is this even possible? I don't actually known, but you don't define a
HALF link mode, so how is this requested?
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.
> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
No inline functions in .c files please, let the compiler decide.
> +int bcm_setup_master_slave(struct phy_device *phydev);
> +int bcm_config_aneg(struct phy_device *phydev, bool changed);
> +int bcm_config_advert(struct phy_device *phydev);
These are all BroadReach specific, so i would put something in there
name to indicate this. Otherwise somebody is going to try to use them
when not appropriate.
Andrew
On 5/6/24 07:40, Kamil Horák - 2N wrote:
> Add the definitions of LRE registers for Broadcom BCM5481x PHY
Missing Signed-off-by tag.
> ---
> include/linux/brcmphy.h | 91 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 1394ba302367..9c0b78c1b6fb 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -270,16 +270,105 @@
> #define BCM5482_SSD_SGMII_SLAVE 0x15 /* SGMII Slave Register */
> #define BCM5482_SSD_SGMII_SLAVE_EN 0x0002 /* Slave mode enable */
> #define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
> +#define BCM5482_SSD_SGMII_SLAVE_AD 0x0001 /* Slave auto-detection */
> +
> +/* BroadR-Reach LRE Registers. */
> +#define MII_BCM54XX_LRECR 0x00 /* LRE Control Register */
> +#define MII_BCM54XX_LRESR 0x01 /* LRE Status Register */
> +#define MII_BCM54XX_LREPHYSID1 0x02 /* LRE PHYS ID 1 */
> +#define MII_BCM54XX_LREPHYSID2 0x03 /* LRE PHYS ID 2 */
> +#define MII_BCM54XX_LREANAA 0x04 /* LDS Auto-Negotiation Advertised Ability */
> +#define MII_BCM54XX_LREANAC 0x05 /* LDS Auto-Negotiation Advertised Control */
> +#define MII_BCM54XX_LREANPT 0x06 /* LDS Ability Next Page Transmit */
> +#define MII_BCM54XX_LRELPA 0x07 /* LDS Link Partner Ability */
> +#define MII_BCM54XX_LRELPNPM 0x08 /* LDS Link Partner Next Page Message */
> +#define MII_BCM54XX_LRELPNPC 0x09 /* LDS Link Partner Next Page Control */
> +#define MII_BCM54XX_LRELDSE 0x0a /* LDS Expansion Register */
> +#define MII_BCM54XX_LREES 0x0f /* LRE Extended Status */
> +
> +/* LRE control register. */
> +#define LRECR_RESET 0x8000 /* Reset to default state */
> +#define LRECR_LOOPBACK 0x4000 /* Internal Loopback */
> +#define LRECR_LDSRES 0x2000 /* Restart LDS Process */
> +#define LRECR_LDSEN 0x1000 /* LDS Enable */
> +#define LRECR_PDOWN 0x0800 /* Enable low power state */
> +#define LRECR_ISOLATE 0x0400 /* Isolate data paths from MII */
> +#define LRECR_SPEED100 0x0200 /* Select 100 Mbps */
> +#define LRECR_SPEED10 0x0000 /* Select 10 Mbps */
> +#define LRECR_4PAIRS 0x0020 /* Select 4 Pairs */
> +#define LRECR_2PAIRS 0x0010 /* Select 2 Pairs */
> +#define LRECR_1PAIR 0x0000 /* Select 1 Pair */
> +#define LRECR_MASTER 0x0008 /* Force Master when LDS disabled */
> +#define LRECR_SLAVE 0x0000 /* Force Slave when LDS disabled */
So previous register had definitions ordered from MSB to LSB...
> +
> +/* LRE status register. */
> +#define LRESR_ERCAP 0x0001 /* Ext-reg capability */
and here we have LSB to MSB, seems like you chose MSB to LSB, so we
should stick to that.
> +#define LRESR_JCD 0x0002 /* Jabber detected */
> +#define LRESR_LSTATUS 0x0004 /* Link status */
> +#define LRESR_LDSABILITY 0x0008 /* Can do LDS */
Comment should be LDS auto-negotiation capable, other comments are fine.
> +#define LRESR_8023 0x0010 /* Has IEEE 802.3 Support */
> +#define LRESR_LDSCOMPLETE 0x0020 /* LDS Auto-negotiation complete */
> +#define LRESR_MFPS 0x0040 /* Can suppress Management Frames Preamble */
> +#define LRESR_RESV 0x0080 /* Unused... */
> +#define LRESR_ESTATEN 0x0100 /* Extended Status in R15 */
> +#define LRESR_10_1PAIR 0x0200 /* Can do 10Mbps 1 Pair */
> +#define LRESR_10_2PAIR 0x0400 /* Can do 10Mbps 2 Pairs */
> +#define LRESR_100_2PAIR 0x0800 /* Can do 100Mbps 2 Pairs */
> +#define LRESR_100_4PAIR 0x1000 /* Can do 100Mbps 4 Pairs */
> +#define LRESR_100_1PAIR 0x2000 /* Can do 100Mbps 1 Pair */
> +
> +/* LDS Auto-Negotiation Advertised Ability. */
> +#define LREANAA_PAUSE_ASYM 0x8000 /* Can pause asymmetrically */
> +#define LREANAA_PAUSE 0x4000 /* Can pause */
> +#define LREANAA_100_1PAIR 0x0020 /* Can do 100Mbps 1 Pair */
> +#define LREANAA_100_4PAIR 0x0010 /* Can do 100Mbps 4 Pair */
> +#define LREANAA_100_2PAIR 0x0008 /* Can do 100Mbps 2 Pair */
> +#define LREANAA_10_2PAIR 0x0004 /* Can do 10Mbps 2 Pair */
> +#define LREANAA_10_1PAIR 0x0002 /* Can do 10Mbps 1 Pair */
> +
> +#define LRE_ADVERTISE_FULL (LREANAA_100_1PAIR | LREANAA_100_4PAIR | \
> + LREANAA_100_2PAIR | LREANAA_10_2PAIR | \
> + LREANAA_10_1PAIR)
> +
> +#define LRE_ADVERTISE_ALL LRE_ADVERTISE_FULL
> +
> +/* LDS Link Partner Ability. */
> +#define LRELPA_PAUSE_ASYM 0x8000 /* Supports asymmetric pause */
> +#define LRELPA_PAUSE 0x4000 /* Supports pause capability */
> +#define LRELPA_100_1PAIR 0x0020 /* 100Mbps 1 Pair capable */
> +#define LRELPA_100_4PAIR 0x0010 /* 100Mbps 4 Pair capable */
> +#define LRELPA_100_2PAIR 0x0008 /* 100Mbps 2 Pair capable */
> +#define LRELPA_10_2PAIR 0x0004 /* 10Mbps 2 Pair capable */
> +#define LRELPA_10_1PAIR 0x0002 /* 10Mbps 1 Pair capable */
> +
> +/* LDS Expansion register. */
> +#define LDSE_DOWNGRADE 0x8000 /* Can do LDS Speed Downgrade */
> +#define LDSE_MASTER 0x4000 /* Master / Slave */
> +#define LDSE_PAIRS_MASK 0x3000 /* Pair Count Mask */
Please define LDSE_PAIRS_SHIFT and make the LDSE_%dPAIRS left shift by 12.
> +#define LDSE_4PAIRS 0x2000 /* 4 Pairs Connection */
> +#define LDSE_2PAIRS 0x1000 /* 2 Pairs Connection */
> +#define LDSE_1PAIR 0x0000 /* 1 Pair Connection */
> +#define LDSE_CABLEN_MASK 0x0FFF /* Cable Length Mask */
>
> /* BCM54810 Registers */
> #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL (MII_BCM54XX_EXP_SEL_ER + 0x90)
> #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN (1 << 0)
> #define BCM54810_SHD_CLK_CTL 0x3
> #define BCM54810_SHD_CLK_CTL_GTXCLK_EN (1 << 9)
> +#define BCM54810_SHD_SCR3_TRDDAPD 0x0100
Unrelated change?
> +
> +/* BCM54811 Registers */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL (MII_BCM54XX_EXP_SEL_ER + 0x9A)
> +/* Access Control Override Enable */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN BIT(15)
> +/* Access Control Override Value */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL BIT(14)
> +/* Access Control Value */
> +#define BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_VAL BIT(13)
This register is not documented in my datasheet, but register 0x0E is, I
am fine with using that one though.
>
> /* BCM54612E Registers */
> #define BCM54612E_EXP_SPARE0 (MII_BCM54XX_EXP_SEL_ETC + 0x34)
> -#define BCM54612E_LED4_CLK125OUT_EN (1 << 1)
> +#define BCM54612E_LED4_CLK125OUT_EN BIT(1)
Unrelated change, and one that is arguably inconsistent with the
definitions you are adding, I would stick with the style you used, since
there are precedents for that in brcmphy.h.
--
Florian
On Mon, May 06, 2024 at 04:40:15PM +0200, Kamil Horák - 2N wrote:
> 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 <[email protected]>
Hi Kamil,
Some minor feedback from my side.
..
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
..
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.
Please consider including a Return: section in the Kernel doc.
Flagged by ./scripts/kernel-doc -Wall -none
> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
..
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
..
> +static int bcm_read_master_slave(struct phy_device *phydev)
> +{
> + int cfg, state;
> + int val;
> +
> + /* In BroadR-Reach mode we are always capable of master-slave
> + * and there is no preferred master or slave configuration
> + */
> + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> + val = phy_read(phydev, MII_BCM54XX_LRECR);
> + if (val < 0)
> + return val;
> +
> + if ((val & LRECR_LDSEN) == 0) {
> + if (val & LRECR_MASTER)
> + cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
> + else
> + cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
> + }
> +
> + val = phy_read(phydev, MII_BCM54XX_LRELDSE);
> + if (val < 0)
> + return val;
> +
> + if (val & LDSE_MASTER)
> + state = MASTER_SLAVE_STATE_MASTER;
> + else
> + state = MASTER_SLAVE_STATE_SLAVE;
> +
> + phydev->master_slave_get = cfg;
Perhaps it is not possible, but it appears that if the
((val & LRECR_LDSEN) == 0) condition above is not met then
cfg is used uninitialised here.
Flagged by Smatch.
> + phydev->master_slave_state = state;
> +
> + return 0;
> +}
..
On 5/6/24 21:14, Andrew Lunn wrote:
> On Mon, May 06, 2024 at 04:40:13PM +0200, Kamil Horák - 2N wrote:
>> Introduce new link modes necessary for the BroadR-Reach mode on
>> bcm5481x PHY by Broadcom and new PHY tunable to choose between
>> normal (IEEE) ethernet and BroadR-Reach modes of the PHY.
> I would of split this into two patches. The reason being, we need the
> new link mode. But do we need the tunable? Why don't i just use the
> link mode to select it?
>
> ethtool -s eth42 advertise 1BR10
Tried to find a way to do the link mode selection this way but the
advertised modes are only applicable when there is auto-negotiation,
which is only partially the case of BCM54811: it only has
auto-negotiation in IEEE mode.
Thus, to avoid choosing between BroadR-Reach and IEEE mode using the PHY
Tunable, we would need something else and I am already running out of
ideas...
Is there any other possibility?
In addition, we would have to check for incompatible link modes selected
to advertise (cannot choose one BRR and one IEEE mode to advertise), or
perhaps the BRR modes would take precedence, if there is any BRR mode
selected to advertise, IEEE modes would be ignored.
>
> Once you have split this up, you can explain the link mode patch in a
> bit more detail. That because the name does not fit 802.3, the normal
> macros cannot be used, so everything needs to be hand crafted.
>
> Andrew
>
> ---
> pw-bot: cr
Kamil