Provide a helper to determine if the PHY has a C45 register space. This
is a preparation patch to remove the is_c45 field. No functional change.
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
- rename to phy_has_c45_registers()
---
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 4 ++--
drivers/net/phy/bcm84881.c | 2 +-
drivers/net/phy/marvell10g.c | 2 +-
drivers/net/phy/mxl-gpy.c | 2 +-
drivers/net/phy/phy-core.c | 4 ++--
drivers/net/phy/phy.c | 8 +++++---
drivers/net/phy/phy_device.c | 6 +++---
drivers/net/phy/phylink.c | 12 +++++++-----
include/linux/phy.h | 5 +++++
9 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index b54f3706fb97..387d24f195aa 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -916,7 +916,7 @@ static void hns_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
hns_nic_test_strs[MAC_INTERNALLOOP_MAC]);
ethtool_sprintf(&buff,
hns_nic_test_strs[MAC_INTERNALLOOP_SERDES]);
- if ((netdev->phydev) && (!netdev->phydev->is_c45))
+ if (netdev->phydev && !phy_has_c45_registers(netdev->phydev))
ethtool_sprintf(&buff,
hns_nic_test_strs[MAC_INTERNALLOOP_PHY]);
@@ -976,7 +976,7 @@ static int hns_get_sset_count(struct net_device *netdev, int stringset)
if (priv->ae_handle->phy_if == PHY_INTERFACE_MODE_XGMII)
cnt--;
- if ((!netdev->phydev) || (netdev->phydev->is_c45))
+ if (!netdev->phydev || phy_has_c45_registers(netdev->phydev))
cnt--;
return cnt;
diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..857344260230 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -47,7 +47,7 @@ static int bcm84881_probe(struct phy_device *phydev)
/* This driver requires PMAPMD and AN blocks */
const u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
- if (!phydev->is_c45 ||
+ if (!phy_has_c45_registers(phydev) ||
(phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
return -ENODEV;
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 55d9d7acc32e..d1c12843462f 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -499,7 +499,7 @@ static int mv3310_probe(struct phy_device *phydev)
u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
int ret;
- if (!phydev->is_c45 ||
+ if (!phy_has_c45_registers(phydev) ||
(phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
return -ENODEV;
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index ea1073adc5a1..b4ddb9a003d9 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -281,7 +281,7 @@ static int gpy_probe(struct phy_device *phydev)
int fw_version;
int ret;
- if (!phydev->is_c45) {
+ if (!phy_has_c45_registers(phydev)) {
ret = phy_get_c45_ids(phydev);
if (ret < 0)
return ret;
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index a64186dc53f8..686a57d56885 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
if (phydev->drv && phydev->drv->read_mmd) {
val = phydev->drv->read_mmd(phydev, devad, regnum);
- } else if (phydev->is_c45) {
+ } else if (phy_has_c45_registers(phydev)) {
val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
devad, regnum);
} else {
@@ -612,7 +612,7 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
if (phydev->drv && phydev->drv->write_mmd) {
ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
- } else if (phydev->is_c45) {
+ } else if (phy_has_c45_registers(phydev)) {
ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
devad, regnum, val);
} else {
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index bdf00b2b2c1d..c6059ba8b9e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -182,7 +182,8 @@ int phy_restart_aneg(struct phy_device *phydev)
{
int ret;
- if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+ if (phy_has_c45_registers(phydev) &&
+ !(phydev->c45_ids.devices_in_package & BIT(0)))
ret = genphy_c45_restart_aneg(phydev);
else
ret = genphy_restart_aneg(phydev);
@@ -203,7 +204,7 @@ int phy_aneg_done(struct phy_device *phydev)
{
if (phydev->drv && phydev->drv->aneg_done)
return phydev->drv->aneg_done(phydev);
- else if (phydev->is_c45)
+ else if (phy_has_c45_registers(phydev))
return genphy_c45_aneg_done(phydev);
else
return genphy_aneg_done(phydev);
@@ -896,7 +897,8 @@ int phy_config_aneg(struct phy_device *phydev)
/* Clause 45 PHYs that don't implement Clause 22 registers are not
* allowed to call genphy_config_aneg()
*/
- if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+ if (phy_has_c45_registers(phydev) &&
+ !(phydev->c45_ids.devices_in_package & BIT(0)))
return genphy_c45_config_aneg(phydev);
return genphy_config_aneg(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 226d5507c865..44968ea447fc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -533,7 +533,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
if (phydrv->match_phy_device)
return phydrv->match_phy_device(phydev);
- if (phydev->is_c45) {
+ if (phy_has_c45_registers(phydev)) {
for (i = 1; i < num_ids; i++) {
if (phydev->c45_ids.device_ids[i] == 0xffffffff)
continue;
@@ -1452,7 +1452,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
* exist, and we should use the genphy driver.
*/
if (!d->driver) {
- if (phydev->is_c45)
+ if (phy_has_c45_registers(phydev))
d->driver = &genphy_c45_driver.mdiodrv.driver;
else
d->driver = &genphy_driver.mdiodrv.driver;
@@ -3227,7 +3227,7 @@ static int phy_probe(struct device *dev)
}
else if (phydrv->get_features)
err = phydrv->get_features(phydev);
- else if (phydev->is_c45)
+ else if (phy_has_c45_registers(phydev))
err = genphy_c45_pma_read_abilities(phydev);
else
err = genphy_read_abilities(phydev);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index d0aaa5cad853..54fde8252079 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1726,7 +1726,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
* against all interface modes, which may lead to more ethtool link
* modes being advertised than are actually supported.
*/
- if (phy->is_c45 && config.rate_matching == RATE_MATCH_NONE &&
+ if (phy_has_c45_registers(phy) &&
+ config.rate_matching == RATE_MATCH_NONE &&
interface != PHY_INTERFACE_MODE_RXAUI &&
interface != PHY_INTERFACE_MODE_XAUI &&
interface != PHY_INTERFACE_MODE_USXGMII)
@@ -2673,7 +2674,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
reg);
}
- if (phydev->is_c45) {
+ if (phy_has_c45_registers(phydev)) {
switch (reg) {
case MII_BMCR:
case MII_BMSR:
@@ -2715,7 +2716,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
reg, val);
}
- if (phydev->is_c45) {
+ if (phy_has_c45_registers(phydev)) {
switch (reg) {
case MII_BMCR:
case MII_BMSR:
@@ -3188,8 +3189,9 @@ static void phylink_sfp_link_up(void *upstream)
*/
static bool phylink_phy_no_inband(struct phy_device *phy)
{
- return phy->is_c45 && phy_id_compare(phy->c45_ids.device_ids[1],
- 0xae025150, 0xfffffff0);
+ return phy_has_c45_registers(phy) &&
+ phy_id_compare(phy->c45_ids.device_ids[1],
+ 0xae025150, 0xfffffff0);
}
static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 11c1e91563d4..fdb3774e99fc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -766,6 +766,11 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
return container_of(to_mdio_device(dev), struct phy_device, mdio);
}
+static inline bool phy_has_c45_registers(struct phy_device *phydev)
+{
+ return phydev->is_c45;
+}
+
/**
* struct phy_tdr_config - Configuration of a TDR raw test
*
--
2.39.2On Wed, Jul 12, 2023 at 05:07:02PM +0200, Michael Walle wrote:
> Provide a helper to determine if the PHY has a C45 register space. This
> is a preparation patch to remove the is_c45 field. No functional change.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
On Wed, Jul 12, 2023 at 05:07:02PM +0200, Michael Walle wrote:
> Provide a helper to determine if the PHY has a C45 register space. This
> is a preparation patch to remove the is_c45 field. No functional change.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> v3:
> - rename to phy_has_c45_registers()
> ---
> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 4 ++--
> drivers/net/phy/bcm84881.c | 2 +-
> drivers/net/phy/marvell10g.c | 2 +-
> drivers/net/phy/mxl-gpy.c | 2 +-
> drivers/net/phy/phy-core.c | 4 ++--
> drivers/net/phy/phy.c | 8 +++++---
> drivers/net/phy/phy_device.c | 6 +++---
> drivers/net/phy/phylink.c | 12 +++++++-----
> include/linux/phy.h | 5 +++++
> 9 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> index b54f3706fb97..387d24f195aa 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> @@ -916,7 +916,7 @@ static void hns_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
> hns_nic_test_strs[MAC_INTERNALLOOP_MAC]);
> ethtool_sprintf(&buff,
> hns_nic_test_strs[MAC_INTERNALLOOP_SERDES]);
> - if ((netdev->phydev) && (!netdev->phydev->is_c45))
> + if (netdev->phydev && !phy_has_c45_registers(netdev->phydev))
> ethtool_sprintf(&buff,
> hns_nic_test_strs[MAC_INTERNALLOOP_PHY]);
>
> @@ -976,7 +976,7 @@ static int hns_get_sset_count(struct net_device *netdev, int stringset)
> if (priv->ae_handle->phy_if == PHY_INTERFACE_MODE_XGMII)
> cnt--;
>
> - if ((!netdev->phydev) || (netdev->phydev->is_c45))
> + if (!netdev->phydev || phy_has_c45_registers(netdev->phydev))
> cnt--;
These two look to be phy_has_c22_registers(). Probably. It calls
phy_loopback() and it seems to assume that only works for C22
registers, because of the genphy_loopback() call.
>
> return cnt;
> diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
> index 9717a1626f3f..857344260230 100644
> --- a/drivers/net/phy/bcm84881.c
> +++ b/drivers/net/phy/bcm84881.c
> @@ -47,7 +47,7 @@ static int bcm84881_probe(struct phy_device *phydev)
> /* This driver requires PMAPMD and AN blocks */
> const u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
>
> - if (!phydev->is_c45 ||
> + if (!phy_has_c45_registers(phydev) ||
> (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
> return -ENODEV;
The comment indicates it. phy_has_c45_registers() is correct, and how
you access them does not matter.
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 55d9d7acc32e..d1c12843462f 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -499,7 +499,7 @@ static int mv3310_probe(struct phy_device *phydev)
> u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
> int ret;
>
> - if (!phydev->is_c45 ||
> + if (!phy_has_c45_registers(phydev) ||
> (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
> return -ENODEV;
Correct, we need the C45 registers. How we access them does not
matter.
> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index ea1073adc5a1..b4ddb9a003d9 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c
> @@ -281,7 +281,7 @@ static int gpy_probe(struct phy_device *phydev)
> int fw_version;
> int ret;
>
> - if (!phydev->is_c45) {
> + if (!phy_has_c45_registers(phydev)) {
> ret = phy_get_c45_ids(phydev);
> if (ret < 0)
> return ret;
This is i think the oddball case which gits a patch of its own
later. This is about, did we discover the PHY via C22. If so, we need
to promote to C45. So i actually think this should be expressed
something like
if (phy_has_c22_registers(phydev) &&
!phy_has_c45_registers(phydev))
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index a64186dc53f8..686a57d56885 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
>
> if (phydev->drv && phydev->drv->read_mmd) {
> val = phydev->drv->read_mmd(phydev, devad, regnum);
> - } else if (phydev->is_c45) {
> + } else if (phy_has_c45_registers(phydev)) {
This i would say should be
phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
on the bus, and if not, fall back to C45 over C22.
> val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
> devad, regnum);
> } else {
> @@ -612,7 +612,7 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
>
> if (phydev->drv && phydev->drv->write_mmd) {
> ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
> - } else if (phydev->is_c45) {
> + } else if (phy_has_c45_registers(phydev)) {
same as above.
> ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
> devad, regnum, val);
> } else {
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index bdf00b2b2c1d..c6059ba8b9e6 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -182,7 +182,8 @@ int phy_restart_aneg(struct phy_device *phydev)
> {
> int ret;
>
> - if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
> + if (phy_has_c45_registers(phydev) &&
> + !(phydev->c45_ids.devices_in_package & BIT(0)))
> ret = genphy_c45_restart_aneg(phydev);
> else
> ret = genphy_restart_aneg(phydev);
Correct.
> @@ -203,7 +204,7 @@ int phy_aneg_done(struct phy_device *phydev)
> {
> if (phydev->drv && phydev->drv->aneg_done)
> return phydev->drv->aneg_done(phydev);
> - else if (phydev->is_c45)
> + else if (phy_has_c45_registers(phydev))
> return genphy_c45_aneg_done(phydev);
> else
> return genphy_aneg_done(phydev);
Correct.
> @@ -896,7 +897,8 @@ int phy_config_aneg(struct phy_device *phydev)
> /* Clause 45 PHYs that don't implement Clause 22 registers are not
> * allowed to call genphy_config_aneg()
> */
> - if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
> + if (phy_has_c45_registers(phydev) &&
> + !(phydev->c45_ids.devices_in_package & BIT(0)))
> return genphy_c45_config_aneg(phydev);
Correct.
> return genphy_config_aneg(phydev);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 226d5507c865..44968ea447fc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -533,7 +533,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
> if (phydrv->match_phy_device)
> return phydrv->match_phy_device(phydev);
>
> - if (phydev->is_c45) {
> + if (phy_has_c45_registers(phydev)) {
> for (i = 1; i < num_ids; i++) {
> if (phydev->c45_ids.device_ids[i] == 0xffffffff)
> continue;
Correct, do we have C45 ID registers, and does not matter how we got
them.
> @@ -1452,7 +1452,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
> * exist, and we should use the genphy driver.
> */
> if (!d->driver) {
> - if (phydev->is_c45)
> + if (phy_has_c45_registers(phydev))
> d->driver = &genphy_c45_driver.mdiodrv.driver;
> else
> d->driver = &genphy_driver.mdiodrv.driver;
Correct.
> @@ -3227,7 +3227,7 @@ static int phy_probe(struct device *dev)
> }
> else if (phydrv->get_features)
> err = phydrv->get_features(phydev);
> - else if (phydev->is_c45)
> + else if (phy_has_c45_registers(phydev))
> err = genphy_c45_pma_read_abilities(phydev);
> else
> err = genphy_read_abilities(phydev);
Correct.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d0aaa5cad853..54fde8252079 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1726,7 +1726,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
> * against all interface modes, which may lead to more ethtool link
> * modes being advertised than are actually supported.
> */
> - if (phy->is_c45 && config.rate_matching == RATE_MATCH_NONE &&
> + if (phy_has_c45_registers(phy) &&
> + config.rate_matching == RATE_MATCH_NONE &&
> interface != PHY_INTERFACE_MODE_RXAUI &&
> interface != PHY_INTERFACE_MODE_XAUI &&
> interface != PHY_INTERFACE_MODE_USXGMII)
I would say correct. The comment above this is:
/* Clause 45 PHYs may switch their Serdes lane between, e.g. 10GBASE-R,
* 5GBASE-R, 2500BASE-X and SGMII if they are not using rate matching.
I suppose a C22 PHY could as well, but none currently do, and actually
implementing anything faster than 1G without C45 is pretty difficult.
> @@ -2673,7 +2674,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
> reg);
> }
>
> - if (phydev->is_c45) {
> + if (phy_has_c45_registers(phydev)) {
> switch (reg) {
> case MII_BMCR:
> case MII_BMSR:
This one is less clear. It ends up doing:
return mdiobus_c45_read(pl->phydev->mdio.bus, prtad, devad,
reg);
which is a C45 bus transaction. So i would actually say this should be
phy_has_c45_transfers(phydev). And maybe at some point in the future
this gets changed to use a higher level function which can do either
C45 or C45 over C22, and then the condition changed to
phy_has_c45_registers(phydev).
> @@ -2715,7 +2716,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
> reg, val);
> }
>
> - if (phydev->is_c45) {
> + if (phy_has_c45_registers(phydev)) {
> switch (reg) {
> case MII_BMCR:
> case MII_BMSR:
This is the same as the previous.
> @@ -3188,8 +3189,9 @@ static void phylink_sfp_link_up(void *upstream)
> */
> static bool phylink_phy_no_inband(struct phy_device *phy)
> {
> - return phy->is_c45 && phy_id_compare(phy->c45_ids.device_ids[1],
> - 0xae025150, 0xfffffff0);
> + return phy_has_c45_registers(phy) &&
> + phy_id_compare(phy->c45_ids.device_ids[1],
> + 0xae025150, 0xfffffff0);
> }
Correct, we are looking at a C45 ID.
> static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 11c1e91563d4..fdb3774e99fc 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -766,6 +766,11 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
> return container_of(to_mdio_device(dev), struct phy_device, mdio);
> }
>
> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
> +{
> + return phydev->is_c45;
> +}
And this is where it gets interesting. I think as a first step, you
should implement the four functions:
phy_has_c22_registers()
phy_has_c45_registers()
phy_has_c22_transfers()
phy_has_c45_transfers()
based on this. So there is initially no functional change.
You can then change the implementation of _transfers() based on what
the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
been found.
Then change the implementation of _registers() based on the results of
probing for the ID registers.
That should give us a basis for a clean separation between register
spaces and bus transaction, and then adding C45 over C22 should be
more obviously correct.
Andrew
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index a64186dc53f8..686a57d56885 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device *phydev, int
>> devad, u32 regnum)
>>
>> if (phydev->drv && phydev->drv->read_mmd) {
>> val = phydev->drv->read_mmd(phydev, devad, regnum);
>> - } else if (phydev->is_c45) {
>> + } else if (phy_has_c45_registers(phydev)) {
>
> This i would say should be
>
> phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
> on the bus, and if not, fall back to C45 over C22.
Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
I've have a similar helper introduced in 9/11:
static inline bool mdiobus_supports_c45(struct mii_bus *bus)
{
return bus->read_c45 && !bus->prevent_c45_access;
}
>> static int phylink_sfp_connect_phy(void *upstream, struct phy_device
>> *phy)
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 11c1e91563d4..fdb3774e99fc 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -766,6 +766,11 @@ static inline struct phy_device
>> *to_phy_device(const struct device *dev)
>> return container_of(to_mdio_device(dev), struct phy_device, mdio);
>> }
>>
>> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
>> +{
>> + return phydev->is_c45;
>> +}
>
> And this is where it gets interesting. I think as a first step, you
> should implement the four functions:
>
> phy_has_c22_registers()
> phy_has_c45_registers()
> phy_has_c22_transfers()
> phy_has_c45_transfers()
>
> based on this. So there is initially no functional change.
>
>
> You can then change the implementation of _transfers() based on what
> the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
> been found.
See above. Shouldn't it be mdiobus_...() then?
> Then change the implementation of _registers() based on the results of
> probing for the ID registers.
So this is where I cannot follow. Right now there is
(1) probing via bus scan
(2) probing via DT (or maybe also ACPI)
With (1) you we have scan_c22(), so if successful,
phy_has_c22_registers()
will return true, right? But it's not that clear for
phy_has_c45_registers(), because sometimes we prevent that scan. So
the PHY might have c45 but we don't know.
For (2) we don't even do a c22 scan if we know if its a C45 PHY (or the
other way around). I'm not sure we can reliably tell (at the end of this
series) if a phy has c22 register, c45 registers or both.
-michael
> That should give us a basis for a clean separation between register
> spaces and bus transaction, and then adding C45 over C22 should be
> more obviously correct.
>
> Andrew
Hi Andrew,
Am 2023-07-19 09:11, schrieb Michael Walle:
>>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>>> index a64186dc53f8..686a57d56885 100644
>>> --- a/drivers/net/phy/phy-core.c
>>> +++ b/drivers/net/phy/phy-core.c
>>> @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device *phydev, int
>>> devad, u32 regnum)
>>>
>>> if (phydev->drv && phydev->drv->read_mmd) {
>>> val = phydev->drv->read_mmd(phydev, devad, regnum);
>>> - } else if (phydev->is_c45) {
>>> + } else if (phy_has_c45_registers(phydev)) {
>>
>> This i would say should be
>>
>> phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
>> on the bus, and if not, fall back to C45 over C22.
>
> Shouldn't this then be a bus property? I.e.
> mdiobus_has_c45_transfers().
> I've have a similar helper introduced in 9/11:
>
> static inline bool mdiobus_supports_c45(struct mii_bus *bus)
> {
> return bus->read_c45 && !bus->prevent_c45_access;
> }
>
>>> static int phylink_sfp_connect_phy(void *upstream, struct phy_device
>>> *phy)
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 11c1e91563d4..fdb3774e99fc 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -766,6 +766,11 @@ static inline struct phy_device
>>> *to_phy_device(const struct device *dev)
>>> return container_of(to_mdio_device(dev), struct phy_device, mdio);
>>> }
>>>
>>> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
>>> +{
>>> + return phydev->is_c45;
>>> +}
>>
>> And this is where it gets interesting. I think as a first step, you
>> should implement the four functions:
>>
>> phy_has_c22_registers()
>> phy_has_c45_registers()
>> phy_has_c22_transfers()
>> phy_has_c45_transfers()
>>
>> based on this. So there is initially no functional change.
>>
>>
>> You can then change the implementation of _transfers() based on what
>> the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
>> been found.
>
> See above. Shouldn't it be mdiobus_...() then?
>
>> Then change the implementation of _registers() based on the results of
>> probing for the ID registers.
>
> So this is where I cannot follow. Right now there is
> (1) probing via bus scan
> (2) probing via DT (or maybe also ACPI)
>
> With (1) you we have scan_c22(), so if successful,
> phy_has_c22_registers()
> will return true, right? But it's not that clear for
> phy_has_c45_registers(), because sometimes we prevent that scan. So
> the PHY might have c45 but we don't know.
>
> For (2) we don't even do a c22 scan if we know if its a C45 PHY (or the
> other way around). I'm not sure we can reliably tell (at the end of
> this
> series) if a phy has c22 register, c45 registers or both.
Any news here?
-michael
On Tue, Aug 01, 2023 at 04:47:04PM +0200, Michael Walle wrote:
> Hi Andrew,
>
> Am 2023-07-19 09:11, schrieb Michael Walle:
> > > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> > > > index a64186dc53f8..686a57d56885 100644
> > > > --- a/drivers/net/phy/phy-core.c
> > > > +++ b/drivers/net/phy/phy-core.c
> > > > @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device
> > > > *phydev, int devad, u32 regnum)
> > > >
> > > > if (phydev->drv && phydev->drv->read_mmd) {
> > > > val = phydev->drv->read_mmd(phydev, devad, regnum);
> > > > - } else if (phydev->is_c45) {
> > > > + } else if (phy_has_c45_registers(phydev)) {
> > >
> > > This i would say should be
> > >
> > > phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
> > > on the bus, and if not, fall back to C45 over C22.
> >
> > Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
> > I've have a similar helper introduced in 9/11:
> >
> > static inline bool mdiobus_supports_c45(struct mii_bus *bus)
> > {
> > return bus->read_c45 && !bus->prevent_c45_access;
> > }
In the case of the above (the code in __phy_read_mmd()), I wouldn't
at least initially change the test there.
phydev->is_c45 will only be true if we probed the PHY using clause
45 accesses. Thus, it will be set if "the bus supports clause 45
accesses" _and_ "the PHY responds to those accesses".
Changing that to only "the bus supports clause 45 accesses" means
that a PHY supporting only clause 22 access with indirect clause
45 access then fails if it's used with a bus that supports both
clause 22 and clause 45 accesses.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi,
>> > > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> > > > index a64186dc53f8..686a57d56885 100644
>> > > > --- a/drivers/net/phy/phy-core.c
>> > > > +++ b/drivers/net/phy/phy-core.c
>> > > > @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device
>> > > > *phydev, int devad, u32 regnum)
>> > > >
>> > > > if (phydev->drv && phydev->drv->read_mmd) {
>> > > > val = phydev->drv->read_mmd(phydev, devad, regnum);
>> > > > - } else if (phydev->is_c45) {
>> > > > + } else if (phy_has_c45_registers(phydev)) {
>> > >
>> > > This i would say should be
>> > >
>> > > phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
>> > > on the bus, and if not, fall back to C45 over C22.
>> >
>> > Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
>> > I've have a similar helper introduced in 9/11:
>> >
>> > static inline bool mdiobus_supports_c45(struct mii_bus *bus)
>> > {
>> > return bus->read_c45 && !bus->prevent_c45_access;
>> > }
>
> In the case of the above (the code in __phy_read_mmd()), I wouldn't
> at least initially change the test there.
>
> phydev->is_c45 will only be true if we probed the PHY using clause
> 45 accesses. Thus, it will be set if "the bus supports clause 45
> accesses" _and_ "the PHY responds to those accesses".
>
> Changing that to only "the bus supports clause 45 accesses" means
> that a PHY supporting only clause 22 access with indirect clause
> 45 access then fails if it's used with a bus that supports both
> clause 22 and clause 45 accesses.
Yeah of course. It was more about the naming, but I just realized
that with mdiobus_supports_c45() you can't access the original
"is_c45" property of the PHY. So maybe this patch needs to be split
into two to get rid of .is_c45:
First a mechanical one:
phy_has_c45_registers() {
return phydev->is_c45;
}
phy_has_c22_registers() {
return !phydev->is_c45;
}
For all the places Andrew said it's correct. Leave all the
other uses of .is_c45 as is for now and rework them in a
later patch to use mdiobus_supports_{c22,c45}().
-michael
On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
> > In the case of the above (the code in __phy_read_mmd()), I wouldn't
> > at least initially change the test there.
> >
> > phydev->is_c45 will only be true if we probed the PHY using clause
> > 45 accesses. Thus, it will be set if "the bus supports clause 45
> > accesses" _and_ "the PHY responds to those accesses".
> >
> > Changing that to only "the bus supports clause 45 accesses" means
> > that a PHY supporting only clause 22 access with indirect clause
> > 45 access then fails if it's used with a bus that supports both
> > clause 22 and clause 45 accesses.
>
> Yeah of course. It was more about the naming, but I just realized
> that with mdiobus_supports_c45() you can't access the original
> "is_c45" property of the PHY. So maybe this patch needs to be split
> into two to get rid of .is_c45:
>
> First a mechanical one:
> phy_has_c45_registers() {
> return phydev->is_c45;
> }
Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
suggested "_transfers" instead - because a PHY can have C45 registers
that are accessible via the indirect registers in C22 space.
I'd go one further:
static bool phy_supports_c45_transfers(struct phy_device *phydev)
{
return phydev->is_c45;
}
Since that covers that (a) the bus needs to support C45 transfers and
(b) the PHY also needs to respond to C45 transfers.
If we want to truly know whether a clause 22 PHY has clause 45
registers, that's difficult to answer, because then you're into the
realms of "does this PHY implement the indirect access method" and
we haven't been keeping track of that for the PHYs we have drivers
for - many will do, but it's optional in clause 22. The problem is
that when it's not implemented, the registers could be serving some
other function.
> phy_has_c22_registers() {
> return !phydev->is_c45;
> }
The reverse is not true, as clause 45 PHYs can also support clause 22
registers - from 802.3:
"For cases where a single entity combines Clause 45 MMDs with Clause 22
registers, then the Clause 22 registers may be accessed using the Clause
45 electrical interface and the Clause 22 management frame structure."
"Bit 5.0 is used to indicate that Clause 22 functionality has been
implemented within a Clause 45 electrical interface device."
Therefore, this would more accurately describe when Clause 22 registers
are present for a PHY:
static bool phy_has_c22_registers(struct phy_device *phydev)
{
/* If we probed the PHY without clause 45 accesses, then by
* definition, clause 22 registers must be present.
*/
if (!phydev->is_c45)
return true;
/* If we probed the PHY with clause 45 accesses, clause 22
* registers may be present if bit 0 in the Devices-in-pacakge
* register pair is set.
*/
return phydev->c45_ids.devices_in_package & BIT(0);
}
Note that this doesn't take account of whether the bus supports clause
22 register access - there are a number of MDIO buses that do not
support such accesses, and they may be coupled with a PHY that does
support clause 22 registers.
I'm aware of a SFP with a Realtek PHY on that falls into this exact
case, and getting that working is progressing at the moment.
> For all the places Andrew said it's correct. Leave all the
> other uses of .is_c45 as is for now and rework them in a
> later patch to use mdiobus_supports_{c22,c45}().
For the two cases in marvell10g and bcm84881, the test there for
is_c45 is purely to determine "was this ID found on a PHY supporting
clause 45 access" - however, in both cases, a check is made for MMDs
present in devices_in_package which will fail if the PHY wasn't
discovered in clause 45 mode.
Note that 88x3310 does not support clause 22 access. I forget whether
bcm84881 does or not.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
>> > In the case of the above (the code in __phy_read_mmd()), I wouldn't
>> > at least initially change the test there.
>> >
>> > phydev->is_c45 will only be true if we probed the PHY using clause
>> > 45 accesses. Thus, it will be set if "the bus supports clause 45
>> > accesses" _and_ "the PHY responds to those accesses".
>> >
>> > Changing that to only "the bus supports clause 45 accesses" means
>> > that a PHY supporting only clause 22 access with indirect clause
>> > 45 access then fails if it's used with a bus that supports both
>> > clause 22 and clause 45 accesses.
>>
>> Yeah of course. It was more about the naming, but I just realized
>> that with mdiobus_supports_c45() you can't access the original
>> "is_c45" property of the PHY. So maybe this patch needs to be split
>> into two to get rid of .is_c45:
>>
>> First a mechanical one:
>> phy_has_c45_registers() {
>> return phydev->is_c45;
>> }
>
> Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> suggested "_transfers" instead - because a PHY can have C45 registers
> that are accessible via the indirect registers in C22 space.
I'm confused now. Andrew suggested to split it into four different
functions:
phy_has_c22_registers()
phy_has_c45_registers()
phy_has_c22_transfers()
phy_has_c45_transfers()
Without a functional change. That is, either return phydev->is_c45
or the inverse.
You seem to suggest to use either
phy_supports_c45_transfers() or
phy_has_c22_registers()
I'm not sure how to continue now.
> I'd go one further:
>
> static bool phy_supports_c45_transfers(struct phy_device *phydev)
> {
> return phydev->is_c45;
> }
>
> Since that covers that (a) the bus needs to support C45 transfers and
> (b) the PHY also needs to respond to C45 transfers.
>
> If we want to truly know whether a clause 22 PHY has clause 45
> registers, that's difficult to answer, because then you're into the
> realms of "does this PHY implement the indirect access method" and
> we haven't been keeping track of that for the PHYs we have drivers
> for - many will do, but it's optional in clause 22. The problem is
> that when it's not implemented, the registers could be serving some
> other function.
>
>> phy_has_c22_registers() {
>> return !phydev->is_c45;
>> }
>
> The reverse is not true, as clause 45 PHYs can also support clause 22
> registers - from 802.3:
>
> "For cases where a single entity combines Clause 45 MMDs with Clause
> 22
> registers, then the Clause 22 registers may be accessed using the
> Clause
> 45 electrical interface and the Clause 22 management frame structure."
>
> "Bit 5.0 is used to indicate that Clause 22 functionality has been
> implemented within a Clause 45 electrical interface device."
>
> Therefore, this would more accurately describe when Clause 22 registers
> are present for a PHY:
>
> static bool phy_has_c22_registers(struct phy_device *phydev)
> {
> /* If we probed the PHY without clause 45 accesses, then by
> * definition, clause 22 registers must be present.
> */
> if (!phydev->is_c45)
> return true;
>
> /* If we probed the PHY with clause 45 accesses, clause 22
> * registers may be present if bit 0 in the Devices-in-pacakge
> * register pair is set.
> */
> return phydev->c45_ids.devices_in_package & BIT(0);
> }
>
> Note that this doesn't take account of whether the bus supports clause
> 22 register access - there are a number of MDIO buses that do not
> support such accesses, and they may be coupled with a PHY that does
> support clause 22 registers.
>
> I'm aware of a SFP with a Realtek PHY on that falls into this exact
> case, and getting that working is progressing at the moment.
>
>> For all the places Andrew said it's correct. Leave all the
>> other uses of .is_c45 as is for now and rework them in a
>> later patch to use mdiobus_supports_{c22,c45}().
>
> For the two cases in marvell10g and bcm84881, the test there for
> is_c45 is purely to determine "was this ID found on a PHY supporting
> clause 45 access" - however, in both cases, a check is made for MMDs
> present in devices_in_package which will fail if the PHY wasn't
> discovered in clause 45 mode.
>
> Note that 88x3310 does not support clause 22 access. I forget whether
> bcm84881 does or not.
So a simple "phydev->is_c45" should be enough? Why do you test
for the MMD presence bits?
-michael
> I'm confused now. Andrew suggested to split it into four different > functions: > > phy_has_c22_registers() > phy_has_c45_registers() > phy_has_c22_transfers() > phy_has_c45_transfers() > > Without a functional change. That is, either return phydev->is_c45 > or the inverse. Without a functional change at this step of introducing the four functions. Then later really implement them to do what the name implies. Doing it in steps helps with bisect when it breaks something. It could also be that not all four are needed, or not all four are possible. But the four express the full combinations of transfers and registers. Andrew
On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote: > > I'm confused now. Andrew suggested to split it into four different > > functions: > > > > phy_has_c22_registers() > > phy_has_c45_registers() > > phy_has_c22_transfers() > > phy_has_c45_transfers() > > > > Without a functional change. That is, either return phydev->is_c45 > > or the inverse. > > Without a functional change at this step of introducing the four > functions. Then later really implement them to do what the name > implies. Doing it in steps helps with bisect when it breaks something. > > It could also be that not all four are needed, or not all four are > possible. But the four express the full combinations of transfers and > registers. I'm left wondering how you think that phy_has_c45_registers() can be implemented in a useful way, because I'm at a total loss... -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > > I'm confused now. Andrew suggested to split it into four different
> > > functions:
> > >
> > > phy_has_c22_registers()
> > > phy_has_c45_registers()
> > > phy_has_c22_transfers()
> > > phy_has_c45_transfers()
> > >
> > > Without a functional change. That is, either return phydev->is_c45
> > > or the inverse.
> >
> > Without a functional change at this step of introducing the four
> > functions. Then later really implement them to do what the name
> > implies. Doing it in steps helps with bisect when it breaks something.
> >
> > It could also be that not all four are needed, or not all four are
> > possible. But the four express the full combinations of transfers and
> > registers.
>
> I'm left wondering how you think that phy_has_c45_registers() can
> be implemented in a useful way, because I'm at a total loss...
get_phy_c22_id() looks at the ID registers, and see if its mostly
Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be
get_phy_c22_id() != -ENODEV. And you can extend that with the logic
you quoted from the standard. get_phy_c45_ids() also returns -ENODEV
if there are no devices in the package. So phy_has_c45_registers()
could be get_phy_c45_id() != -ENODEV.
This is heuristics, not something explicitly from the standard, but
the basic idea of looking for not mostly Fs has been used for a long
time.
Andrew
On Thu, Aug 03, 2023 at 12:21:03AM +0200, Andrew Lunn wrote: > On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote: > > On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote: > > > > I'm confused now. Andrew suggested to split it into four different > > > > functions: > > > > > > > > phy_has_c22_registers() > > > > phy_has_c45_registers() > > > > phy_has_c22_transfers() > > > > phy_has_c45_transfers() > > > > > > > > Without a functional change. That is, either return phydev->is_c45 > > > > or the inverse. > > > > > > Without a functional change at this step of introducing the four > > > functions. Then later really implement them to do what the name > > > implies. Doing it in steps helps with bisect when it breaks something. > > > > > > It could also be that not all four are needed, or not all four are > > > possible. But the four express the full combinations of transfers and > > > registers. > > > > I'm left wondering how you think that phy_has_c45_registers() can > > be implemented in a useful way, because I'm at a total loss... > > get_phy_c22_id() looks at the ID registers, and see if its mostly > Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be > get_phy_c22_id() != -ENODEV. And you can extend that with the logic > you quoted from the standard. get_phy_c45_ids() also returns -ENODEV > if there are no devices in the package. So phy_has_c45_registers() > could be get_phy_c45_id() != -ENODEV. No, this is not correct, because get_phy_c45_id() uses mdiobus c45 reads - if the bus doesn't support c45 transfers, then get_phy_c45_id() fails. This doesn't tell us whether the PHY has clause 45 registers at all, it just tells us that we probed the PHY successfully using clause 45 accesses. The fact of the matter is that there is no way in the current phylib code (and probably no way to probe it either) to determine whether a PHY has clause 45 registers or not. To see why, take for example the AR8035. This clearly has clause 45 registers, because it has MMDs 3 and 7 implemented. However, these are only accessible through the indirect access method via clause 22 cycles. So, does it make sense for AR8035 to return false from phy_has_c45_registers() ? No it doesn't, because it clearly does have clause 45 registers! The same can be said about Marvell 88e151x PHYs. And probably many other clause 22 PHYs. Hence why I disagree with your suggestion. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi, > Hence why I disagree with your suggestion. So how can we make progress here? I tried Russells suggestion using phy_supports_c45_transfers() and phy_has_c22_registers(). You can find a possible v4 of this series on my github [1]. Some uses of .is_c45 seem to be to test whether the c45_ids are valid. E.g. from the phy.h: * @c45_ids: 802.3-c45 Device Identifiers if is_c45. But if you test if a bit is set, you can skip that. For the opposite test you'd need to have some kind of indication whether c45_ids was populated in the first place. If you convert all these uses of is_c45, there will only be a handful of uses left. And replacing these by phy_supports_c45_transfers() sounds sane to me except for for these two: (1) drivers/net/ethernet/hisilicon/hns/hns_ethtool.c (2) drivers/net/phy/mxl-gpy.c (2) will eventually replaced by my phy_promote_to_c45(). I might reorder the patches, so I'll get rid of the .is_c45 use there before the conversion to phy_supports_c45_transfers(). But honestly, for (1) I don't have any idea whats going on. If I look at the very first commit, it seems that the is_c45 property is used to distinguish loopback handling between a gigabit and a 10g PHY (?). Btw, Russell, I've noticed that phy_restart_aneg() and phy_config_aneg() will test for c22 registers, but phy_aneg_done() tests just for .is_c45. Is that correct? -michael [1] https://github.com/mwalle/linux/tree/feature-c45-over-c22-v4 [2] https://git.kernel.org/torvalds/c/b5996f11ea5496d0445078f47d22c987888ed467
On Wed, Aug 02, 2023 at 05:33:20PM +0200, Michael Walle wrote:
> Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> > On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
> > > > In the case of the above (the code in __phy_read_mmd()), I wouldn't
> > > > at least initially change the test there.
> > > >
> > > > phydev->is_c45 will only be true if we probed the PHY using clause
> > > > 45 accesses. Thus, it will be set if "the bus supports clause 45
> > > > accesses" _and_ "the PHY responds to those accesses".
> > > >
> > > > Changing that to only "the bus supports clause 45 accesses" means
> > > > that a PHY supporting only clause 22 access with indirect clause
> > > > 45 access then fails if it's used with a bus that supports both
> > > > clause 22 and clause 45 accesses.
> > >
> > > Yeah of course. It was more about the naming, but I just realized
> > > that with mdiobus_supports_c45() you can't access the original
> > > "is_c45" property of the PHY. So maybe this patch needs to be split
> > > into two to get rid of .is_c45:
> > >
> > > First a mechanical one:
> > > phy_has_c45_registers() {
> > > return phydev->is_c45;
> > > }
> >
> > Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> > suggested "_transfers" instead - because a PHY can have C45 registers
> > that are accessible via the indirect registers in C22 space.
>
> I'm confused now. Andrew suggested to split it into four different
> functions:
>
> phy_has_c22_registers()
> phy_has_c45_registers()
> phy_has_c22_transfers()
> phy_has_c45_transfers()
Honestly, I don't think we can come up with tests that satisfy all of
these. Particularly the question whether a PHY has c45 registers or
not is a difficult one, as there is no sane way to determine that with
a clause 22 PHY.
I'm also not sure what use the c22 transfers one would be, since if a
PHY doesn't have c22 registers, then that's probably all we need to
know.
> Without a functional change. That is, either return phydev->is_c45
> or the inverse.
I think I've already explained why !phydev->is_c45 can't be interpeted
as a PHY having C22 registers, but let me restate. It is _entirely_
possible for a PHY to have C45 registers _and_ C22 registers, and
that is indicated by bit 0 of the devices-in-package field.
>
> You seem to suggest to use either
> phy_supports_c45_transfers() or
> phy_has_c22_registers()
>
> I'm not sure how to continue now.
>
> > I'd go one further:
> >
> > static bool phy_supports_c45_transfers(struct phy_device *phydev)
> > {
> > return phydev->is_c45;
> > }
> >
> > Since that covers that (a) the bus needs to support C45 transfers and
> > (b) the PHY also needs to respond to C45 transfers.
> >
> > If we want to truly know whether a clause 22 PHY has clause 45
> > registers, that's difficult to answer, because then you're into the
> > realms of "does this PHY implement the indirect access method" and
> > we haven't been keeping track of that for the PHYs we have drivers
> > for - many will do, but it's optional in clause 22. The problem is
> > that when it's not implemented, the registers could be serving some
> > other function.
> >
> > > phy_has_c22_registers() {
> > > return !phydev->is_c45;
> > > }
> >
> > The reverse is not true, as clause 45 PHYs can also support clause 22
> > registers - from 802.3:
> >
> > "For cases where a single entity combines Clause 45 MMDs with Clause
> > 22
> > registers, then the Clause 22 registers may be accessed using the
> > Clause
> > 45 electrical interface and the Clause 22 management frame structure."
> >
> > "Bit 5.0 is used to indicate that Clause 22 functionality has been
> > implemented within a Clause 45 electrical interface device."
> >
> > Therefore, this would more accurately describe when Clause 22 registers
> > are present for a PHY:
> >
> > static bool phy_has_c22_registers(struct phy_device *phydev)
> > {
> > /* If we probed the PHY without clause 45 accesses, then by
> > * definition, clause 22 registers must be present.
> > */
> > if (!phydev->is_c45)
> > return true;
> >
> > /* If we probed the PHY with clause 45 accesses, clause 22
> > * registers may be present if bit 0 in the Devices-in-pacakge
> > * register pair is set.
> > */
> > return phydev->c45_ids.devices_in_package & BIT(0);
> > }
> >
> > Note that this doesn't take account of whether the bus supports clause
> > 22 register access - there are a number of MDIO buses that do not
> > support such accesses, and they may be coupled with a PHY that does
> > support clause 22 registers.
> >
> > I'm aware of a SFP with a Realtek PHY on that falls into this exact
> > case, and getting that working is progressing at the moment.
> >
> > > For all the places Andrew said it's correct. Leave all the
> > > other uses of .is_c45 as is for now and rework them in a
> > > later patch to use mdiobus_supports_{c22,c45}().
> >
> > For the two cases in marvell10g and bcm84881, the test there for
> > is_c45 is purely to determine "was this ID found on a PHY supporting
> > clause 45 access" - however, in both cases, a check is made for MMDs
> > present in devices_in_package which will fail if the PHY wasn't
> > discovered in clause 45 mode.
> >
> > Note that 88x3310 does not support clause 22 access. I forget whether
> > bcm84881 does or not.
>
> So a simple "phydev->is_c45" should be enough? Why do you test
> for the MMD presence bits?
Okay, so if quoting the bits from IEEE 802.3 doesn't provide sufficient
explanation, I'm at a loss what would...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Am 2023-08-02 18:06, schrieb Russell King (Oracle):
> On Wed, Aug 02, 2023 at 05:33:20PM +0200, Michael Walle wrote:
>> Am 2023-08-01 17:57, schrieb Russell King (Oracle):
>> > On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
>> > > > In the case of the above (the code in __phy_read_mmd()), I wouldn't
>> > > > at least initially change the test there.
>> > > >
>> > > > phydev->is_c45 will only be true if we probed the PHY using clause
>> > > > 45 accesses. Thus, it will be set if "the bus supports clause 45
>> > > > accesses" _and_ "the PHY responds to those accesses".
>> > > >
>> > > > Changing that to only "the bus supports clause 45 accesses" means
>> > > > that a PHY supporting only clause 22 access with indirect clause
>> > > > 45 access then fails if it's used with a bus that supports both
>> > > > clause 22 and clause 45 accesses.
>> > >
>> > > Yeah of course. It was more about the naming, but I just realized
>> > > that with mdiobus_supports_c45() you can't access the original
>> > > "is_c45" property of the PHY. So maybe this patch needs to be split
>> > > into two to get rid of .is_c45:
>> > >
>> > > First a mechanical one:
>> > > phy_has_c45_registers() {
>> > > return phydev->is_c45;
>> > > }
>> >
>> > Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
>> > suggested "_transfers" instead - because a PHY can have C45 registers
>> > that are accessible via the indirect registers in C22 space.
>>
>> I'm confused now. Andrew suggested to split it into four different
>> functions:
>>
>> phy_has_c22_registers()
>> phy_has_c45_registers()
>> phy_has_c22_transfers()
>> phy_has_c45_transfers()
>
> Honestly, I don't think we can come up with tests that satisfy all of
> these. Particularly the question whether a PHY has c45 registers or
> not is a difficult one, as there is no sane way to determine that with
> a clause 22 PHY.
And that is exactly the one which Andrew suggested to use for almost all
cases. It doesn't make sense to me to introduce that function if it
can't
really be made to return a correct value (in a later patch).
> I'm also not sure what use the c22 transfers one would be, since if a
> PHY doesn't have c22 registers, then that's probably all we need to
> know.
>
>> Without a functional change. That is, either return phydev->is_c45
>> or the inverse.
>
> I think I've already explained why !phydev->is_c45 can't be interpeted
> as a PHY having C22 registers, but let me restate. It is _entirely_
> possible for a PHY to have C45 registers _and_ C22 registers, and
> that is indicated by bit 0 of the devices-in-package field.
>
>> You seem to suggest to use either
>> phy_supports_c45_transfers() or
>> phy_has_c22_registers()
>>
>> I'm not sure how to continue now.
>>
>> > I'd go one further:
>> >
>> > static bool phy_supports_c45_transfers(struct phy_device *phydev)
>> > {
>> > return phydev->is_c45;
>> > }
>> >
>> > Since that covers that (a) the bus needs to support C45 transfers and
>> > (b) the PHY also needs to respond to C45 transfers.
>> >
>> > If we want to truly know whether a clause 22 PHY has clause 45
>> > registers, that's difficult to answer, because then you're into the
>> > realms of "does this PHY implement the indirect access method" and
>> > we haven't been keeping track of that for the PHYs we have drivers
>> > for - many will do, but it's optional in clause 22. The problem is
>> > that when it's not implemented, the registers could be serving some
>> > other function.
>> >
>> > > phy_has_c22_registers() {
>> > > return !phydev->is_c45;
>> > > }
>> >
>> > The reverse is not true, as clause 45 PHYs can also support clause 22
>> > registers - from 802.3:
>> >
>> > "For cases where a single entity combines Clause 45 MMDs with Clause
>> > 22
>> > registers, then the Clause 22 registers may be accessed using the
>> > Clause
>> > 45 electrical interface and the Clause 22 management frame structure."
>> >
>> > "Bit 5.0 is used to indicate that Clause 22 functionality has been
>> > implemented within a Clause 45 electrical interface device."
>> >
>> > Therefore, this would more accurately describe when Clause 22 registers
>> > are present for a PHY:
>> >
>> > static bool phy_has_c22_registers(struct phy_device *phydev)
>> > {
>> > /* If we probed the PHY without clause 45 accesses, then by
>> > * definition, clause 22 registers must be present.
>> > */
>> > if (!phydev->is_c45)
>> > return true;
>> >
>> > /* If we probed the PHY with clause 45 accesses, clause 22
>> > * registers may be present if bit 0 in the Devices-in-pacakge
>> > * register pair is set.
>> > */
>> > return phydev->c45_ids.devices_in_package & BIT(0);
>> > }
>> >
>> > Note that this doesn't take account of whether the bus supports clause
>> > 22 register access - there are a number of MDIO buses that do not
>> > support such accesses, and they may be coupled with a PHY that does
>> > support clause 22 registers.
>> >
>> > I'm aware of a SFP with a Realtek PHY on that falls into this exact
>> > case, and getting that working is progressing at the moment.
>> >
>> > > For all the places Andrew said it's correct. Leave all the
>> > > other uses of .is_c45 as is for now and rework them in a
>> > > later patch to use mdiobus_supports_{c22,c45}().
>> >
>> > For the two cases in marvell10g and bcm84881, the test there for
>> > is_c45 is purely to determine "was this ID found on a PHY supporting
>> > clause 45 access" - however, in both cases, a check is made for MMDs
>> > present in devices_in_package which will fail if the PHY wasn't
>> > discovered in clause 45 mode.
>> >
>> > Note that 88x3310 does not support clause 22 access. I forget whether
>> > bcm84881 does or not.
>>
>> So a simple "phydev->is_c45" should be enough? Why do you test
>> for the MMD presence bits?
>
> Okay, so if quoting the bits from IEEE 802.3 doesn't provide sufficient
> explanation, I'm at a loss what would...
I'm talking about
u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
if (!phydev->is_c45 ||
(phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
return -ENODEV;
How should that look like after this series?
On Wed, Aug 02, 2023 at 07:11:27PM +0200, Michael Walle wrote: > I'm talking about > > u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN; > if (!phydev->is_c45 || > (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask) > return -ENODEV; > > How should that look like after this series? In the case of the marvell10g driver, at least the 88x3310 can be accessed via clause 45 bus cycles _or_ the clause 45 indirect register access via clause 22. I'm not sure what the clause 22 registers would contain, whether they contain valid values with the exception of the clause 45 indirect access registers because there's not enough information in the documentation, and I can't access the clause 22 registers on real hardware. Another issue is this PHY has two different ID values. One is 0x002b 0x09aa, the other is 0x0141 0x0daa. One or other of these values appears in the MMDs - in other words, they are not consistently used. This means it's impossible to guess what value may be in the clause 22 ID registers for this PHY. However, given the way phylib works, the above effectively ensures that we detected the PHY using clause 45 accesses, and then goes on to verify that we do indeed have the PMAPMD MMD and the AN MMD. Effectively, it ensures that get_phy_c45_ids() was used to detect the device. So, in effect, this code is ensuring that we discovered the PHY using clause 45 accesses, and that at a minimum the PHY also indicates that the PMAPMD and AN MMDs are implemented. For the bcm84881 driver, it's a similar situation - that's only ever been used with a bus that supports _only_ clause 45 accesses. Even less idea whether the PHY could respond to clause 22 accesses as there is no information available on the PHY. So, I'd suggest both of these are the same - returns -ENODEV if the bus doesn't support clause 45 transfers or if the two MMDs are not indicated. That said, it _can_ be simplified down to just testing the devices_in_package member, because that will only be non-zero if we successfully probed the PHY via some accessible way to the clause 45 registers. So: if ((phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask) return -ENODEV; is probably entirely sufficient in both cases. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2026 Red Hat, Inc.