| Message ID | 20230710205900.52894-2-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add a driver for the Marvell 88Q2110 PHY | expand |
On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote: > This patch adds the link modes for the 1000BASE-T1 Ethernet PHYs. It > supports 100BASE-T1/1000BASE-T1 in full duplex mode. So far I could not > find a 1000BASE-T1 PHY that also supports 10BASE-T1, so this mode is not > added. Is this actually needed? Ideally you want to extend genphy_c45_pma_read_abilities() to look in the PHY registers and determine what the PHY can do. You should only use .features if it is impossible to determine the PHY abilities by reading registers. Andrew
Hi Andrew, On Mon, Jul 10, 2023 at 11:10:17PM +0200, Andrew Lunn wrote: > On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote: > > This patch adds the link modes for the 1000BASE-T1 Ethernet PHYs. It > > supports 100BASE-T1/1000BASE-T1 in full duplex mode. So far I could not > > find a 1000BASE-T1 PHY that also supports 10BASE-T1, so this mode is not > > added. > > Is this actually needed? Ideally you want to extend > genphy_c45_pma_read_abilities() to look in the PHY registers and > determine what the PHY can do. You should only use .features if it is > impossible to determine the PHY abilities by reading registers. Unfortunately the MDIO_PMA_EXTABLE register does not work on this PHY. It will not signalize that it is BT1 capable (MDIO_PMA_EXTABLE_BT1). I tested it again (should be 0x0800): root@verdin-imx8mp-14777637:~# ./phytool read eth0/7:1/11 0x0020 The PHY documentation does not list this register at all. Even though the MDIO_PMA_PMD_BT1 exists and works correctly: root@verdin-imx8mp-14777637:~# ./phytool read eth0/7:1/18 0x0003 This register is documented in the datasheet again. So unfortunately I think I have to force the features, or do you see another possibility? Regards, Stefan
On Thu, Jul 13, 2023 at 04:10:21PM +0200, Stefan Eichenberger wrote: > Hi Andrew, > > On Mon, Jul 10, 2023 at 11:10:17PM +0200, Andrew Lunn wrote: > > On Mon, Jul 10, 2023 at 10:58:57PM +0200, Stefan Eichenberger wrote: > > > This patch adds the link modes for the 1000BASE-T1 Ethernet PHYs. It > > > supports 100BASE-T1/1000BASE-T1 in full duplex mode. So far I could not > > > find a 1000BASE-T1 PHY that also supports 10BASE-T1, so this mode is not > > > added. > > > > Is this actually needed? Ideally you want to extend > > genphy_c45_pma_read_abilities() to look in the PHY registers and > > determine what the PHY can do. You should only use .features if it is > > impossible to determine the PHY abilities by reading registers. > > Unfortunately the MDIO_PMA_EXTABLE register does not work on this PHY. > It will not signalize that it is BT1 capable (MDIO_PMA_EXTABLE_BT1). I > tested it again (should be 0x0800): ** * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities * @phydev: target phy_device struct */ static bool genphy_c45_baset1_able(struct phy_device *phydev) { int val; if (phydev->pma_extable == -ENODATA) { val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE); if (val < 0) return false; phydev->pma_extable = val; } return !!(phydev->pma_extable & MDIO_PMA_EXTABLE_BT1); } This is rather odd, but might help you. You already have a workaround in mv88q2xxx_config_init(). Have you tried adding a get_features() callback with sets phydev->pma_extable to the correct value, and then calls genphy_c45_pma_read_abilities()? Please also report the bug in the PHY to Marvell. Maybe a later revision might have it fixed. Andrew
On Thu, Jul 13, 2023 at 05:18:02PM +0200, Andrew Lunn wrote: > > ** > * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities > * @phydev: target phy_device struct > */ > static bool genphy_c45_baset1_able(struct phy_device *phydev) > { > int val; > > if (phydev->pma_extable == -ENODATA) { > val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_EXTABLE); > if (val < 0) > return false; > > phydev->pma_extable = val; > } > > return !!(phydev->pma_extable & MDIO_PMA_EXTABLE_BT1); > } > > This is rather odd, but might help you. You already have a workaround > in mv88q2xxx_config_init(). Have you tried adding a get_features() > callback with sets phydev->pma_extable to the correct value, and then > calls genphy_c45_pma_read_abilities()? > > Please also report the bug in the PHY to Marvell. Maybe a later > revision might have it fixed. Thanks for the suggestion. Unfortunately, genphy_c45_pma_read_abilities() directly reads from the PHY registers. Therefore, I can't use the pma_extable. But probably I can just set the missing features in get_features for this PHY. I will see what I can do here. Thanks, Stefan
> Thanks for the suggestion. Unfortunately, > genphy_c45_pma_read_abilities() directly reads from the PHY registers. Please consider refactoring the bits of genphy_c45_pma_read_abilities() you need into a helper. You can then call the helper directly from your driver. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 0c2014accba7d..acc8950f08cfa 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -50,6 +50,9 @@ EXPORT_SYMBOL_GPL(phy_basic_t1_features); __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1s_p2mp_features) __ro_after_init; EXPORT_SYMBOL_GPL(phy_basic_t1s_p2mp_features); +__ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_t1_features) __ro_after_init; +EXPORT_SYMBOL_GPL(phy_gbit_t1_features); + __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init; EXPORT_SYMBOL_GPL(phy_gbit_features); @@ -109,6 +112,13 @@ const int phy_basic_t1s_p2mp_features_array[2] = { }; EXPORT_SYMBOL_GPL(phy_basic_t1s_p2mp_features_array); +const int phy_gbit_t1_features_array[3] = { + ETHTOOL_LINK_MODE_TP_BIT, + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, +}; +EXPORT_SYMBOL_GPL(phy_gbit_t1_features_array); + const int phy_gbit_features_array[2] = { ETHTOOL_LINK_MODE_1000baseT_Half_BIT, ETHTOOL_LINK_MODE_1000baseT_Full_BIT, @@ -165,6 +175,10 @@ static void features_init(void) linkmode_set_bit_array(phy_basic_t1s_p2mp_features_array, ARRAY_SIZE(phy_basic_t1s_p2mp_features_array), phy_basic_t1s_p2mp_features); + /* 1000 full, TP */ + linkmode_set_bit_array(phy_gbit_t1_features_array, + ARRAY_SIZE(phy_gbit_t1_features_array), + phy_gbit_t1_features); /* 10/100 half/full + 1000 half/full */ linkmode_set_bit_array(phy_basic_ports_array, diff --git a/include/linux/phy.h b/include/linux/phy.h index 11c1e91563d47..6c71f10e0f7f0 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -47,6 +47,7 @@ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1s_p2mp_features) __ro_after_init; +extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_t1_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_fibre_features) __ro_after_init; extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_all_ports_features) __ro_after_init; @@ -58,6 +59,7 @@ extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_eee_cap1_features) __ro_after_init; #define PHY_BASIC_FEATURES ((unsigned long *)&phy_basic_features) #define PHY_BASIC_T1_FEATURES ((unsigned long *)&phy_basic_t1_features) #define PHY_BASIC_T1S_P2MP_FEATURES ((unsigned long *)&phy_basic_t1s_p2mp_features) +#define PHY_GBIT_T1_FEATURES ((unsigned long *)&phy_gbit_t1_features) #define PHY_GBIT_FEATURES ((unsigned long *)&phy_gbit_features) #define PHY_GBIT_FIBRE_FEATURES ((unsigned long *)&phy_gbit_fibre_features) #define PHY_GBIT_ALL_PORTS_FEATURES ((unsigned long *)&phy_gbit_all_ports_features)
This patch adds the link modes for the 1000BASE-T1 Ethernet PHYs. It supports 100BASE-T1/1000BASE-T1 in full duplex mode. So far I could not find a 1000BASE-T1 PHY that also supports 10BASE-T1, so this mode is not added. Signed-off-by: Stefan Eichenberger <eichest@gmail.com> --- drivers/net/phy/phy_device.c | 14 ++++++++++++++ include/linux/phy.h | 2 ++ 2 files changed, 16 insertions(+)