After the c22 and c45 access split is finally merged. This can now be
posted again. The old version can be found here:
https://lore.kernel.org/netdev/[email protected]/
Although all the discussion was here:
https://lore.kernel.org/netdev/[email protected]/
The goal here is to get the GYP215 and LAN8814 running on the Microchip
LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
LAN8814 has a bug which makes it impossible to use C45 on that bus.
Fortunately, it was the intention of the GPY215 driver to be used on a C22
bus. But I think this could have never really worked, because the
phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will
fail.
Introduce C45-over-C22 support and use it if the MDIO bus doesn't support
C45. Also enable it when a PHY is promoted from C22 to C45.
Changes since RFC v2:
- Reased to latest net-next
- new check_rc argument in mmd_phy_indirect() to retain old behavior
- determine bus capabilities by bus->read and bus->read_c45
- always set phydev->c45_over_c22 if PHY is promoted
Changes since RFC v1:
- use __phy_mmd_indirect() in mdiobus_probe_mmd_read()
- add new properties has_c45 c45_over_c22 (and remove is_c45)
- drop MDIOBUS_NO_CAP handling, Andrew is preparing a series to
add probe_capabilities to mark all C45 capable MDIO bus drivers
Michael Walle (5):
net: phy: add error checks in mmd_phy_indirect() and export it
net: phy: support indirect c45 access in get_phy_c45_ids()
net: phy: add support for C45-over-C22 transfers
phy: net: introduce phy_promote_to_c45()
net: phy: mxl-gpy: remove unneeded ops
.../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 | 33 +-------
drivers/net/phy/phy-core.c | 48 ++++++++---
drivers/net/phy/phy.c | 6 +-
drivers/net/phy/phy_device.c | 80 ++++++++++++++++---
drivers/net/phy/phylink.c | 8 +-
include/linux/phy.h | 12 ++-
9 files changed, 128 insertions(+), 67 deletions(-)
--
2.30.2
If an MDIO bus is only capable of doing C22 transfers we can use
indirect accesses to C45 registers over C22 registers. This was already
the intention of the GPY215 driver. The author described their use case
as follows:
Our product supports both C22 and C45.
In the real system, we found C22 was used by customers (with indirect
access to C45 registers when necessary).
In its probe function phy_get_c45_ids() is called but this will always
do C45 accesses and thus will fail on a C22-only bus. With the current
code we only have the is_c45 property which is used to indicate a C45
PHY but also used to choose the transfer mode. With C45-over-C22 we need
to split these two properties.
Drop the is_c45 and instead introduce two new properties, has_c45 and
c45_over_c22. has_c45 is set to true if this is a C45 PHY and
c45_over_c22 is true if we need to do indirect accesses using the C22
registers.
c45_over_c22 will always be set by just looking at the bus capabilities.
It will be set to true if a bus is C22-only, regardless if the PHY would
support indirect access. Firstly, it is a reasonable assumption that C45
PHYs will support this access and secondly, there is really not much we
can do otherwise.
Signed-off-by: Michael Walle <[email protected]>
---
.../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 | 6 ++---
drivers/net/phy/phy_device.c | 25 ++++++++++++++++---
drivers/net/phy/phylink.c | 8 +++---
include/linux/phy.h | 8 +++---
9 files changed, 40 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index b54f3706fb97..e34e1510e6a6 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 && !netdev->phydev->has_c45)
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 || netdev->phydev->has_c45)
cnt--;
return cnt;
diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..d9131d5284c1 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 (!phydev->has_c45 ||
(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 383a9c9f36e5..27a52c11ad75 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 (!phydev->has_c45 ||
(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 e5972b4ef6e8..e86aea4381c2 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 (!phydev->has_c45) {
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 c9c92b95ace2..5e4a54f0a3dc 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -584,7 +584,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 (phydev->has_c45 && !phydev->c45_over_c22) {
val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
devad, regnum);
} else {
@@ -640,7 +640,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 (phydev->has_c45 && !phydev->c45_over_c22) {
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 3378ca4f49b6..121aaae6b2f8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -169,7 +169,7 @@ int phy_restart_aneg(struct phy_device *phydev)
{
int ret;
- if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+ if (phydev->has_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
ret = genphy_c45_restart_aneg(phydev);
else
ret = genphy_restart_aneg(phydev);
@@ -190,7 +190,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 (phydev->has_c45)
return genphy_c45_aneg_done(phydev);
else
return genphy_aneg_done(phydev);
@@ -883,7 +883,7 @@ 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 (phydev->has_c45 && !(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 9777a7fd180a..d5ea034cde98 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -516,7 +516,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 (phydev->has_c45) {
for (i = 1; i < num_ids; i++) {
if (phydev->c45_ids.device_ids[i] == 0xffffffff)
continue;
@@ -648,7 +648,24 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
dev->autoneg = AUTONEG_ENABLE;
dev->pma_extable = -ENODATA;
- dev->is_c45 = is_c45;
+
+ /* Depending on the bus capabilities, we have to use C45-over-C22
+ * register access. We have the following cases:
+ *
+ * 1) bus can only do C45.
+ * 2) bus can only do C22.
+ * 3) bus can do C22 and C45.
+ *
+ * 1) and 3) are easy, because we can just use C45 transfers. For 2) we
+ * don't have any other choice but to use C22 transfers. Even if the
+ * PHY wouldn't support it we cannot do any better.
+ *
+ * Set this for C22 PHYs, too, because the PHY driver might promote it
+ * to C45.
+ */
+ dev->c45_over_c22 = bus->read && !bus->read_c45;
+
+ dev->has_c45 = is_c45;
dev->phy_id = phy_id;
if (c45_ids)
dev->c45_ids = *c45_ids;
@@ -1456,7 +1473,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 (phydev->has_c45)
d->driver = &genphy_c45_driver.mdiodrv.driver;
else
d->driver = &genphy_driver.mdiodrv.driver;
@@ -3115,7 +3132,7 @@ static int phy_probe(struct device *dev)
linkmode_copy(phydev->supported, phydrv->features);
else if (phydrv->get_features)
err = phydrv->get_features(phydev);
- else if (phydev->is_c45)
+ else if (phydev->has_c45)
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 319790221d7f..63bec523a211 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1642,7 +1642,7 @@ 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 && config.rate_matching == RATE_MATCH_NONE &&
interface != PHY_INTERFACE_MODE_RXAUI &&
interface != PHY_INTERFACE_MODE_XAUI &&
interface != PHY_INTERFACE_MODE_USXGMII)
@@ -2584,7 +2584,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
reg);
}
- if (phydev->is_c45) {
+ if (phydev->has_c45 && !phydev->c45_over_c22) {
switch (reg) {
case MII_BMCR:
case MII_BMSR:
@@ -2626,7 +2626,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
reg, val);
}
- if (phydev->is_c45) {
+ if (phydev->has_c45 && !phydev->c45_over_c22) {
switch (reg) {
case MII_BMCR:
case MII_BMSR:
@@ -3101,7 +3101,7 @@ static void phylink_sfp_link_up(void *upstream)
*/
static bool phylink_phy_no_inband(struct phy_device *phy)
{
- return phy->is_c45 &&
+ return phy->has_c45 &&
(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f7a5e110f95c..4c02c468a24b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -532,8 +532,9 @@ struct macsec_ops;
* @devlink: Create a link between phy dev and mac dev, if the external phy
* used by current mac interface is managed by another mac interface.
* @phy_id: UID for this device found during discovery
- * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
- * @is_c45: Set to true if this PHY uses clause 45 addressing.
+ * @c45_ids: 802.3-c45 Device Identifiers if has_c45.
+ * @has_c45: Set to true if this PHY has clause 45 address space.
+ * @c45_over_c22: Set to true if c45-over-c22 addressing is used.
* @is_internal: Set to true if this PHY is internal to a MAC.
* @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
* @is_gigabit_capable: Set to true if PHY supports 1000Mbps
@@ -625,7 +626,8 @@ struct phy_device {
u32 phy_id;
struct phy_c45_device_ids c45_ids;
- unsigned is_c45:1;
+ unsigned has_c45:1;
+ unsigned c45_over_c22:1;
unsigned is_internal:1;
unsigned is_pseudo_fixed_link:1;
unsigned is_gigabit_capable:1;
--
2.30.2
On Fri, Jan 20, 2023 at 11:40:06PM +0100, Michael Walle wrote:
> After the c22 and c45 access split is finally merged. This can now be
> posted again. The old version can be found here:
> https://lore.kernel.org/netdev/[email protected]/
> Although all the discussion was here:
> https://lore.kernel.org/netdev/[email protected]/
>
> The goal here is to get the GYP215 and LAN8814 running on the Microchip
> LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
> LAN8814 has a bug which makes it impossible to use C45 on that bus.
> Fortunately, it was the intention of the GPY215 driver to be used on a C22
> bus. But I think this could have never really worked, because the
> phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will
> fail.
>
> Introduce C45-over-C22 support and use it if the MDIO bus doesn't support
> C45. Also enable it when a PHY is promoted from C22 to C45.
I see this breaking up into two problems.
1) Scanning the bus and finding device, be it by C22, C45, or C45 over C22.
2) Allowing drivers to access C45 register spaces, without caring if
it is C45 transfers or C45 over C22.
For scanning the bus we currently have:
if (bus->read) {
err = mdiobus_scan_bus_c22(bus);
if (err)
goto error;
}
prevent_c45_scan = mdiobus_prevent_c45_scan(bus);
if (!prevent_c45_scan && bus->read_c45) {
err = mdiobus_scan_bus_c45(bus);
if (err)
goto error;
}
I think we should be adding something like:
else {
if (bus->read) {
err = mdiobus_scan_bus_c45_over_c22(bus);
if (err)
goto error;
}
}
That makes the top level pretty obvious what is going on.
But i think we need some more cleanup lower down. We now have a clean
separation in MDIO bus drivers between C22 bus transactions and C45
transactions bus. But further up it is less clear. PHY drivers should
be using phy_read_mmd()/phy_write_mmd() etc, which means access the
C45 address space, but says nothing about what bus transactions to
use. So that is also quite clean.
The problem is in the middle. get_phy_c45_devs_in_pkg() uses
mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus
transaction, or access the C45 address space? I would say it means
perform a C45 bus transaction. It does not take a phydev, so we are
below the concept of PHYs, and so C45 over C22 does not exist at this
level.
So i think we need to review all calls to
mdiobus_c45_read/mdiobus_c45_write() etc and see if they mean C45 bus
transaction or C45 address space. Those meaning address space should
be changed to phy_read_mmd()/phy_write_mmd().
get_phy_device(), get_phy_c45_devs_in_pkg(), get_phy_c45_ids(),
phy_c45_probe_present() however do not deal with phydev, so cannot use
phy_read_mmd()/phy_write_mmd(). They probably need the bool is_c45
replaced with an enum indicating what sort of bus transaction should
be performed. Depending on that value, they can call
mdiobus_c45_read() or mmd_phy_indirect() and __mdiobus_read().
I don't have time at the moment, but i would like to dig more into
phydev->is_c45. has_c45 makes sense to indicate it has c45 address
space. But we need to see if it is every used to indicate to use c45
transactions. But it is clear we need a new member to indicate if C45
or C45 over C22 should be performed, and this should be set by how the
PHY was found in the first place.
Andrew
On Mon, Jan 23, 2023 at 07:03:18PM +0100, Andrew Lunn wrote:
> On Fri, Jan 20, 2023 at 11:40:06PM +0100, Michael Walle wrote:
> > After the c22 and c45 access split is finally merged. This can now be
> > posted again. The old version can be found here:
> > https://lore.kernel.org/netdev/[email protected]/
> > Although all the discussion was here:
> > https://lore.kernel.org/netdev/[email protected]/
> >
> > The goal here is to get the GYP215 and LAN8814 running on the Microchip
> > LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
> > LAN8814 has a bug which makes it impossible to use C45 on that bus.
> > Fortunately, it was the intention of the GPY215 driver to be used on a C22
> > bus. But I think this could have never really worked, because the
> > phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will
> > fail.
> >
> > Introduce C45-over-C22 support and use it if the MDIO bus doesn't support
> > C45. Also enable it when a PHY is promoted from C22 to C45.
>
> I see this breaking up into two problems.
>
> 1) Scanning the bus and finding device, be it by C22, C45, or C45 over C22.
>
> 2) Allowing drivers to access C45 register spaces, without caring if
> it is C45 transfers or C45 over C22.
>
> For scanning the bus we currently have:
>
>
> if (bus->read) {
> err = mdiobus_scan_bus_c22(bus);
> if (err)
> goto error;
> }
>
> prevent_c45_scan = mdiobus_prevent_c45_scan(bus);
>
> if (!prevent_c45_scan && bus->read_c45) {
> err = mdiobus_scan_bus_c45(bus);
> if (err)
> goto error;
> }
>
> I think we should be adding something like:
>
> else {
> if (bus->read) {
> err = mdiobus_scan_bus_c45_over_c22(bus);
> if (err)
> goto error;
> }
> }
>
> That makes the top level pretty obvious what is going on.
>
> But i think we need some more cleanup lower down. We now have a clean
> separation in MDIO bus drivers between C22 bus transactions and C45
> transactions bus. But further up it is less clear. PHY drivers should
> be using phy_read_mmd()/phy_write_mmd() etc, which means access the
> C45 address space, but says nothing about what bus transactions to
> use. So that is also quite clean.
>
> The problem is in the middle. get_phy_c45_devs_in_pkg() uses
> mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus
> transaction, or access the C45 address space? I would say it means
> perform a C45 bus transaction. It does not take a phydev, so we are
> below the concept of PHYs, and so C45 over C22 does not exist at this
> level.
C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking
at the PHY C45-over-C22 registers unless we know for certain that the
C22 device we are accessing is a PHY, otherwise we could be writing
into e.g. a switch register or something else.
So, the mdiobus_* API should be the raw bus API. If we want C45 bus
cycles then mdiobus_c45_*() is the API that gives us that, vs C22 bus
cycles through the non-C45 API.
C45-over-C22 being a PHY thing is something that should be handled by
phylib, and currently is. The phylib accessors there will use C45 or
C45-over-C22 as appropriate.
The problem comes with PHYs that maybe don't expose C22 ID registers
but do have C45-over-C22. These aren't detectable without probing
using the C45-over-C22 PHY protocol, but doing that gratuitously will
end up writing values to e.g. switch registers and disrupting their
operation. So I regard that as a very dangerous thing to be doing.
Given that, it seems that such a case could not be automatically
probed, and thus must be described in firmware.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking
> at the PHY C45-over-C22 registers unless we know for certain that the
> C22 device we are accessing is a PHY, otherwise we could be writing
> into e.g. a switch register or something else.
Humm, yes. Good point.
> The problem comes with PHYs that maybe don't expose C22 ID registers
> but do have C45-over-C22.
>
> Given that, it seems that such a case could not be automatically
> probed, and thus must be described in firmware.
We already have the compatible:
- const: ethernet-phy-ieee802.3-c45
description: PHYs that implement IEEE802.3 clause 45
But it is not clear what that actually means. Does it mean it has c45
registers, or does it mean it supports C45 bus transactions?
If we have that compatible, we could probe first using C45 and if that
fails, or is not supported by the bus master, probe using C45 over
C22. That seems safe. For Michael use case, the results of
mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so
we know not to perform the C45 scan, and go direct to C45 over C22.
Andrew
> - const: ethernet-phy-ieee802.3-c45
> description: PHYs that implement IEEE802.3 clause 45
>
> But it is not clear what that actually means. Does it mean it has c45
> registers, or does it mean it supports C45 bus transactions?
PHYs which support C45 access but don't have C45 registers aren't
a thing I presume - or doesn't make any sense, right?
PHYs which have C45 registers but don't support C45 access exists.
e.g. the EEE registers of C22 PHYs. But they are C22 PHYs.
So I'd say if you have compatible = "ethernet-phy-ieee802.3-c45",
it is a PHY with C45 registers _and_ which are accessible by
C45 transactions. But they might or might not support C22 access.
But I think thats pretty irrelevant because you always do C45 if
you can. You cannot do C45 if:
(1) the mdio bus doesn't support C45
(2) you have a broken C22 phy on the mdio bus
In both cases, if the PHY doesn't support C22-over-C45 you are
screwed. Therefore, if you have either (1) or (2) we should always
fall back to C22-over-C45.
> If we have that compatible, we could probe first using C45 and if that
> fails, or is not supported by the bus master, probe using C45 over
> C22. That seems safe. For Michael use case, the results of
> mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so
> we know not to perform the C45 scan, and go direct to C45 over C22.
So you are talking about DT, in which case there is no auto probing.
See phy_mask in the dt code. Only PHYs in the device tree are probed.
(unless of course there is no reg property.. then there is some
obscure auto scanning). So if you want a C45 PHY you'd have to
have that compatible in any case.
Btw. I still don't know how you can get a C45 PHY instance without
a device tree, except if there is a C45 only bus or the PHY doesn't
respond on C22 ids. Maybe I'm missing something here..
-michael
On Tue, Jan 24, 2023 at 01:35:48AM +0100, Michael Walle wrote:
> > - const: ethernet-phy-ieee802.3-c45
> > description: PHYs that implement IEEE802.3 clause 45
> >
> > But it is not clear what that actually means. Does it mean it has c45
> > registers, or does it mean it supports C45 bus transactions?
>
> PHYs which support C45 access but don't have C45 registers aren't
> a thing I presume - or doesn't make any sense, right?
>
> PHYs which have C45 registers but don't support C45 access exists.
> e.g. the EEE registers of C22 PHYs. But they are C22 PHYs.
I wonder if there are any C22 PHYs which only allow access to EEE via
C45 transactions. That would be pretty broken...
To some extent, i'm still looking at everything and trying to decide
if the current definitions/documentation make it clear if means C45
transfers or registers. And the documentation in DT is ambiguous, but
as you point out, it probably means registers, not transactions.
> So I'd say if you have compatible = "ethernet-phy-ieee802.3-c45",
> it is a PHY with C45 registers _and_ which are accessible by
> C45 transactions. But they might or might not support C22 access.
> But I think thats pretty irrelevant because you always do C45 if
> you can. You cannot do C45 if:
> (1) the mdio bus doesn't support C45
> (2) you have a broken C22 phy on the mdio bus
>
> In both cases, if the PHY doesn't support C22-over-C45 you are
> screwed. Therefore, if you have either (1) or (2) we should always
> fall back to C22-over-C45.
>
> > If we have that compatible, we could probe first using C45 and if that
> > fails, or is not supported by the bus master, probe using C45 over
> > C22. That seems safe. For Michael use case, the results of
> > mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so
> > we know not to perform the C45 scan, and go direct to C45 over C22.
>
> So you are talking about DT, in which case there is no auto probing.
> See phy_mask in the dt code. Only PHYs in the device tree are probed.
> (unless of course there is no reg property.. then there is some
> obscure auto scanning). So if you want a C45 PHY you'd have to
> have that compatible in any case.
>
> Btw. I still don't know how you can get a C45 PHY instance without
> a device tree, except if there is a C45 only bus or the PHY doesn't
> respond on C22 ids. Maybe I'm missing something here..
In the DT case, you are probably correct. But there are a number of
MDIO busses which don't come from DT. Those are typically PCIe or USB
devices. Those do get scanned, and my recent changes should mean they
first get scanned using C22 and then C45. DSA switches also typically
don't have a MDIO node in DT, it is assumed there is a 1:1 mapping
between port number and address on the MDIO bus. But as you said, it
would require that they don't respond to C22, or the bus master does
not support C22, which does actually exist from Marvell at least.
In the none DT case, we probably cannot easily do anything about
C22-over-C45, because as Russell pointed out, it is potentially a
destructive process doing a scan. We want some indication we do expect
a PHY to be there. And "ethernet-phy-ieee802.3-c45" would do that.
Andrew
>> The problem is in the middle. get_phy_c45_devs_in_pkg() uses
>> mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus
>> transaction, or access the C45 address space? I would say it means
>> perform a C45 bus transaction. It does not take a phydev, so we are
>> below the concept of PHYs, and so C45 over C22 does not exist at this
>> level.
>
> C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking
> at the PHY C45-over-C22 registers unless we know for certain that the
> C22 device we are accessing is a PHY, otherwise we could be writing
> into e.g. a switch register or something else.
>
> So, the mdiobus_* API should be the raw bus API. If we want C45 bus
> cycles then mdiobus_c45_*() is the API that gives us that, vs C22 bus
> cycles through the non-C45 API.
>
> C45-over-C22 being a PHY thing is something that should be handled by
> phylib, and currently is. The phylib accessors there will use C45 or
> C45-over-C22 as appropriate.
I think the crux is get_phy_device(). It is used for two different
cases:
(1) to scan the mdio bus
(2) to add a c45 phy, i.e. in the DT/fwnode case
For (1) we must not use indirect access. And for (2) we know for
a fact that it must be a PHY and thus we can (and have to) fall back
to c45-over-c22.
Btw. for the DT case, it seems we need yet another property
to indicate broken MDIO busses.
-michael
> Btw. for the DT case, it seems we need yet another property
> to indicate broken MDIO busses.
I would prefer to avoid that. I would suggest you do what i did for
the none DT case. First probe using C22 for all devices known in DT.
Then call mdiobus_prevent_c45_scan() which will determine if any of
the found devices are FUBAR and will break C45. Then do a second probe
using C45 and/or C45 over C22 for those devices in DT with the c45
compatible.
Andrew
Am 2023-01-24 22:03, schrieb Andrew Lunn:
>> Btw. for the DT case, it seems we need yet another property
>> to indicate broken MDIO busses.
>
> I would prefer to avoid that. I would suggest you do what i did for
> the none DT case. First probe using C22 for all devices known in DT.
> Then call mdiobus_prevent_c45_scan() which will determine if any of
> the found devices are FUBAR and will break C45. Then do a second probe
> using C45 and/or C45 over C22 for those devices in DT with the c45
> compatible.
I tried that yesterday. Have a look at of_mdiobus_register() [1].
There the device tree is walked and each PHY with a reg property
is probed. Afterwards, if there was a node without a reg property,
the bus is scanned for the missing PHYs. If we would just probe c22
first, the order of the auto scanning might change, if there is a
c45 phy in between two c22 phys. I was thinking to just ignore the
case that the autoscan would discover a broken PHY.
(1) scan c22
(2) scan c45 (maybe using c45-over-c22)
(3) do the autoscan
-michael
[1]
https://elixir.bootlin.com/linux/v6.2-rc5/source/drivers/net/mdio/of_mdio.c#L149
>
> Andrew
On Tue, Jan 24, 2023 at 10:20:33PM +0100, Michael Walle wrote:
> Am 2023-01-24 22:03, schrieb Andrew Lunn:
> > > Btw. for the DT case, it seems we need yet another property
> > > to indicate broken MDIO busses.
> >
> > I would prefer to avoid that. I would suggest you do what i did for
> > the none DT case. First probe using C22 for all devices known in DT.
> > Then call mdiobus_prevent_c45_scan() which will determine if any of
> > the found devices are FUBAR and will break C45. Then do a second probe
> > using C45 and/or C45 over C22 for those devices in DT with the c45
> > compatible.
>
> I tried that yesterday. Have a look at of_mdiobus_register() [1].
> There the device tree is walked and each PHY with a reg property
> is probed. Afterwards, if there was a node without a reg property,
> the bus is scanned for the missing PHYs. If we would just probe c22
> first, the order of the auto scanning might change, if there is a
> c45 phy in between two c22 phys. I was thinking to just ignore the
> case that the autoscan would discover a broken PHY.
I think it is pretty rare to not have a reg value. The DT lint tools
will complain about that, etc. So any examples are likely to be old
boards. And old board are a lot less likely to have C45 PHYs. So there
is a corner case left unhandled, but it seems pretty unlikely. So i
agree, lets address it if anybody reports issues. But please mention
it in the commit message, just i can somebody does a git bisect, etc.
Andrew