[Sorry for the very late follow-up on this series, I simply haven't had
time to look into it. Should be better now.]
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 in the OF case. We must not use C45-over-C22 if the device being
probed isn't a PHY because it involes register writes to standard PHY
registers, which might not be supported in generic PHY devices.
Therefore, we cannot use it in the generic PHY probing code. In the DT
case, we know we are probing a PHY device and thus we can fall back to
C45-over-C22.
The first patches convert the is_c45 property to a new "enum
phy_access_mode". The former property is then derived from the
access mode.
To support the probing for DT, export the prevent_c45_scan logic and
make it a property of the mii bus. Unfortunately, you'd need to stick to
the following flow in every scanning code:
(1) add any c22 phys
(2) scan for broken phys
(3) add any c45 phys using either c45 or c45-over-c22 access
I couldn't find a way to make that generic and move the flow into the
phy core.
Then, a new access method c45-over-c22 is added for get_phy_device() and
phy_{read,write}_mmd(). It is the callers responsibilty to choose the
correct mode. Esp. the generic probing code isn't using c45-over-c22.
c45-over-c22 is then added for the MaxLinear PHYs if it is probed as a
C22 device, which is always the case if there is no compatible =
"ethernet-phy-ieee802.3-c45" in the device tree. The driver will
automatically "promote" the PHY to a C45 one if probed as C22.
The last two patches will then add c45-over-c22 fallback to the DT PHY
registration code. As described above, the probing is split into the
three phases.
FWIW, this now triggers a bug in mscc-miim. Haven't figured out what's
wrong yet, somehow the status register doesn't return busy/pending.
Currently, working around that with a sleep.
net: phy: add error checks in mmd_phy_indirect() and export it
net: phy: get rid of redundant is_c45 information
net: phy: introduce phy_is_c45()
net: phy: replace is_c45 with phy_accces_mode
net: phy: make the "prevent_c45_scan" a property of the MII bus
net: phy: print an info if a broken C45 bus is found
net: phy: add support for C45-over-C22 transfers
net: phy: introduce phy_promote_to_c45()
net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
net: mdio: scan for broken C22 PHYs when probed via OF
Changes since v1:
- major rework to address the problem to not use c45-over-c22 on
bus scanning, see the description above. Except from some preparation
patches there is little left of the original series. The major
difference is that there is now a new transport mode argument and the
c45-over-c22 is not automatically used anymore.
- Link: https://lore.kernel.org/netdev/[email protected]/
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
- Link: https://lore.kernel.org/netdev/[email protected]/
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
- Link: https://lore.kernel.org/netdev/[email protected]/
Signed-off-by: Michael Walle <[email protected]>
---
Michael Walle (10):
net: phy: add error checks in mmd_phy_indirect() and export it
net: phy: get rid of redundant is_c45 information
net: phy: introduce phy_is_c45()
net: phy: replace is_c45 with phy_accces_mode
net: phy: make the "prevent_c45_scan" a property of the MII bus
net: phy: print an info if a broken C45 bus is found
net: phy: add support for C45-over-C22 transfers
net: phy: introduce phy_promote_to_c45()
net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
net: mdio: support C45-over-C22 when probed via OF
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 8 +-
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 4 +-
drivers/net/mdio/fwnode_mdio.c | 19 ++-
drivers/net/mdio/of_mdio.c | 63 +++++++---
drivers/net/phy/bcm84881.c | 2 +-
drivers/net/phy/marvell10g.c | 2 +-
drivers/net/phy/mdio_bus.c | 33 ++---
drivers/net/phy/mxl-gpy.c | 9 +-
drivers/net/phy/nxp-tja11xx.c | 3 +-
drivers/net/phy/phy-core.c | 105 +++++++++++-----
drivers/net/phy/phy.c | 8 +-
drivers/net/phy/phy_device.c | 139 +++++++++++++++++-----
drivers/net/phy/phylink.c | 10 +-
drivers/net/phy/sfp.c | 12 +-
include/linux/phy.h | 48 ++++++--
15 files changed, 331 insertions(+), 134 deletions(-)
---
base-commit: 5e218791dc7b2b57c90d5942ab6bae4bc2ab0ead
change-id: 20230620-feature-c45-over-c22-0a2181babd56
The blacklist will also be used elsewhere in the kernel, e.g. in the
DT scanning code. Make it a property of mii_bus and export the function.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/mdio_bus.c | 17 ++++++++---------
include/linux/phy.h | 5 +++++
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a31eb1204f63..00b25f6803bc 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -613,9 +613,9 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus)
* stomping over the true devices reply, to performing a write to
* themselves which was intended for another device. Now that C22
* devices have been found, see if any of them are bad for C45, and if we
- * should skip the C45 scan.
+ * should prohibit any C45 transactions.
*/
-static bool mdiobus_prevent_c45_scan(struct mii_bus *bus)
+void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus)
{
int i;
@@ -628,10 +628,11 @@ static bool mdiobus_prevent_c45_scan(struct mii_bus *bus)
continue;
oui = phydev->phy_id >> 10;
- if (oui == MICREL_OUI)
- return true;
+ if (oui == MICREL_OUI) {
+ bus->prevent_c45_access = true;
+ break;
+ }
}
- return false;
}
/**
@@ -652,7 +653,6 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
{
struct mdio_device *mdiodev;
struct gpio_desc *gpiod;
- bool prevent_c45_scan;
int i, err;
if (!bus || !bus->name)
@@ -724,9 +724,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
goto error;
}
- prevent_c45_scan = mdiobus_prevent_c45_scan(bus);
-
- if (!prevent_c45_scan && bus->read_c45) {
+ mdiobus_scan_for_broken_c45_access(bus);
+ if (!bus->prevent_c45_access && bus->read_c45) {
err = mdiobus_scan_bus_c45(bus);
if (err)
goto error;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 12679bbd4b91..a7aff91f4eb0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -410,6 +410,9 @@ struct mii_bus {
/** @phy_ignore_ta_mask: PHY addresses to ignore the TA/read failure */
u32 phy_ignore_ta_mask;
+ /** @prevent_c45_access: Don't do any C45 transactions on the bus */
+ unsigned prevent_c45_access:1;
+
/**
* @irq: An array of interrupts, each PHY's interrupt at the index
* matching its address
@@ -462,6 +465,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
struct mii_bus *mdio_find_bus(const char *mdio_name);
struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr);
+void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus);
+
#define PHY_INTERRUPT_DISABLED false
#define PHY_INTERRUPT_ENABLED true
--
2.39.2
If not explitly asked to be probed as a C45 PHY, on a bus which is
capable of doing both C22 and C45 transfers, C45 PHYs are first tried to
be probed as C22 PHYs. To be able to promote the PHY to be a C45 one,
the driver can call phy_promote_to_c45() in its probe function.
This was already done in the mxl-gpy driver by the following snippet:
if (!phy_is_c45(phydev)) {
ret = phy_get_c45_ids(phydev);
if (ret < 0)
return ret;
}
Move that code into the core as it could be used by other drivers, too.
If a PHY is promoted C45-over-C22 access is automatically used as a
fallback if the bus doesn't support C45 transactions.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/mxl-gpy.c | 9 ++++-----
drivers/net/phy/phy_device.c | 36 ++++++++++++++++++++++++++++++------
include/linux/phy.h | 7 ++++++-
3 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 66411e46937b..b7fca1aae1c3 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -281,11 +281,10 @@ static int gpy_probe(struct phy_device *phydev)
int fw_version;
int ret;
- if (!phy_is_c45(phydev)) {
- ret = phy_get_c45_ids(phydev);
- if (ret < 0)
- return ret;
- }
+ /* This might have been probed as a C22 PHY, but this is a C45 PHY */
+ ret = phy_promote_to_c45(phydev);
+ if (ret)
+ return ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 13d5ec7a7c21..3f9041a3ad72 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1085,18 +1085,42 @@ void phy_device_remove(struct phy_device *phydev)
EXPORT_SYMBOL(phy_device_remove);
/**
- * phy_get_c45_ids - Read 802.3-c45 IDs for phy device.
- * @phydev: phy_device structure to read 802.3-c45 IDs
+ * phy_promote_to_c45 - Promote to a C45 PHY
+ * @phydev: phy_device structure
+ *
+ * If a PHY supports both C22 and C45 and it isn't specifically asked to probe
+ * as a C45 PHY it might be probed as a C22 PHY. The driver can call this
+ * function to promote a PHY from C22 to C45.
+ *
+ * Can also be called if a PHY is already a C45 one. In that case it does
+ * nothing.
+ *
+ * If the bus isn't capable of doing C45 transfers, C45-over-C22 access will be
+ * used as a fallback.
*
* Returns zero on success, %-EIO on bus access error, or %-ENODEV if
* the "devices in package" is invalid.
*/
-int phy_get_c45_ids(struct phy_device *phydev)
+int phy_promote_to_c45(struct phy_device *phydev)
{
- return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
- &phydev->c45_ids, false);
+ struct mii_bus *bus = phydev->mdio.bus;
+
+ if (phy_is_c45(phydev))
+ return 0;
+
+ if (dev_of_node(&phydev->mdio.dev))
+ phydev_info(phydev,
+ "Promoting PHY to a C45 one. Please consider using compatible=\"ethernet-phy-ieee802.3-c45\".");
+
+ if (mdiobus_supports_c45(bus))
+ phydev->access_mode = PHY_ACCESS_C45;
+ else
+ phydev->access_mode = PHY_ACCESS_C45_OVER_C22;
+
+ return get_phy_c45_ids(bus, phydev->mdio.addr, &phydev->c45_ids,
+ phydev->access_mode == PHY_ACCESS_C45_OVER_C22);
}
-EXPORT_SYMBOL(phy_get_c45_ids);
+EXPORT_SYMBOL(phy_promote_to_c45);
/**
* phy_find_first - finds the first PHY device on the bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4852651f6326..f9c91766bc47 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -467,6 +467,11 @@ struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr);
void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus);
+static inline bool mdiobus_supports_c45(struct mii_bus *bus)
+{
+ return bus->read_c45 && !bus->prevent_c45_access;
+}
+
#define PHY_INTERRUPT_DISABLED false
#define PHY_INTERRUPT_ENABLED true
@@ -1701,7 +1706,7 @@ static inline int phy_device_register(struct phy_device *phy)
static inline void phy_device_free(struct phy_device *phydev) { }
#endif /* CONFIG_PHYLIB */
void phy_device_remove(struct phy_device *phydev);
-int phy_get_c45_ids(struct phy_device *phydev);
+int phy_promote_to_c45(struct phy_device *phydev);
int phy_init_hw(struct phy_device *phydev);
int phy_suspend(struct phy_device *phydev);
int phy_resume(struct phy_device *phydev);
--
2.39.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.
Add a new transfer mode C45-over-C22. Care has to be taken for
get_phy_device() which is called by either the bus scanning code or the
network device drivers. In the former case, we must not do any
C45-over-C22 accesses because it invokes register writes and we cannot
be sure if the accessed device is a PHY. C45-over-C22 is just defined
for network PHYs, not generic MDIO devices. Therefore, the it is the
callers responsibility to choose the access mode for get_phy_device().
Due to the reasons above, the current scanning code the PHY core cannot
make use of the new transfer mode because we cannot be sure what device
is a PHY. The code will be used for the device tree scanning where we
know which device is a PHY and therefore, C45-over-C22 is safe to use.
A word on the error checking in the MMD access methods: due to legacy
reasons, we cannot check for errors if we do "normal" MMD access to a
C22 PHY without introducing possible regressions. Although, C45-over-C22
is doing essentially the same access, we can do better now and check for
any errors while doing the indirect access.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/phy-core.c | 67 +++++++++++++++++++++++++++-----------------
drivers/net/phy/phy_device.c | 66 +++++++++++++++++++++++++++++++++----------
include/linux/phy.h | 3 ++
3 files changed, 95 insertions(+), 41 deletions(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 5f73d27fe330..57062574ca04 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -575,26 +575,34 @@ EXPORT_SYMBOL(__phy_mmd_indirect);
*/
int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
{
- int val;
+ struct mii_bus *bus = phydev->mdio.bus;
+ int phy_addr = phydev->mdio.addr;
+ bool check_rc = true;
+ int ret;
if (regnum > (u16)~0 || devad > 32)
return -EINVAL;
- if (phydev->drv && phydev->drv->read_mmd) {
- val = phydev->drv->read_mmd(phydev, devad, regnum);
- } else if (phy_is_c45(phydev)) {
- val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
- devad, regnum);
- } else {
- struct mii_bus *bus = phydev->mdio.bus;
- int phy_addr = phydev->mdio.addr;
-
- mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+ if (phydev->drv && phydev->drv->read_mmd)
+ return phydev->drv->read_mmd(phydev, devad, regnum);
+
+ switch (phydev->access_mode) {
+ case PHY_ACCESS_C45:
+ return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
+ case PHY_ACCESS_C22:
+ /* ignore return value for legacy reasons */
+ check_rc = false;
+ fallthrough;
+ case PHY_ACCESS_C45_OVER_C22:
+ ret = mmd_phy_indirect(bus, phy_addr, devad, regnum, check_rc);
+ if (check_rc && ret)
+ return ret;
/* Read the content of the MMD's selected register */
- val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
+ return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
}
- return val;
+
+ return -EOPNOTSUPP;
}
EXPORT_SYMBOL(__phy_read_mmd);
@@ -631,28 +639,35 @@ EXPORT_SYMBOL(phy_read_mmd);
*/
int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
{
+ struct mii_bus *bus = phydev->mdio.bus;
+ int phy_addr = phydev->mdio.addr;
+ bool check_rc = true;
int ret;
if (regnum > (u16)~0 || devad > 32)
return -EINVAL;
- if (phydev->drv && phydev->drv->write_mmd) {
- ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
- } else if (phy_is_c45(phydev)) {
- ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
- devad, regnum, val);
- } else {
- struct mii_bus *bus = phydev->mdio.bus;
- int phy_addr = phydev->mdio.addr;
-
- mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+ if (phydev->drv && phydev->drv->write_mmd)
+ return phydev->drv->write_mmd(phydev, devad, regnum, val);
+
+ switch (phydev->access_mode) {
+ case PHY_ACCESS_C45:
+ return __mdiobus_c45_write(bus, phy_addr, devad, regnum, val);
+ case PHY_ACCESS_C22:
+ check_rc = false;
+ fallthrough;
+ case PHY_ACCESS_C45_OVER_C22:
+ ret = mmd_phy_indirect(bus, phy_addr, devad, regnum, check_rc);
+ if (check_rc && ret)
+ return ret;
/* Write the data into MMD's selected register */
- __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
+ ret = __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
- ret = 0;
+ return check_rc ? ret : 0;
}
- return ret;
+
+ return -EOPNOTSUPP;
}
EXPORT_SYMBOL(__phy_write_mmd);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4fd095282ef7..13d5ec7a7c21 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -715,6 +715,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
}
EXPORT_SYMBOL(phy_device_create);
+static int phy_probe_mmd_read(struct mii_bus *bus, int prtad, int devad,
+ u16 regnum, bool c45_over_c22)
+{
+ int ret;
+
+ if (!c45_over_c22)
+ return mdiobus_c45_read(bus, prtad, devad, regnum);
+
+ mutex_lock(&bus->mdio_lock);
+
+ ret = __phy_mmd_indirect(bus, prtad, devad, regnum);
+ if (ret)
+ goto out;
+
+ ret = __mdiobus_read(bus, prtad, MII_MMD_DATA);
+
+out:
+ mutex_unlock(&bus->mdio_lock);
+
+ return ret;
+}
+
/* phy_c45_probe_present - checks to see if a MMD is present in the package
* @bus: the target MII bus
* @prtad: PHY package address on the MII bus
@@ -726,11 +748,13 @@ EXPORT_SYMBOL(phy_device_create);
* Returns: negative error number on bus access error, zero if no device
* is responding, or positive if a device is present.
*/
-static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
+static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad,
+ bool c45_over_c22)
{
int stat2;
- stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+ stat2 = phy_probe_mmd_read(bus, prtad, devad, MDIO_STAT2,
+ c45_over_c22);
if (stat2 < 0)
return stat2;
@@ -749,16 +773,18 @@ static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
* Returns: 0 on success, -EIO on failure.
*/
static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
- u32 *devices_in_package)
+ u32 *devices_in_package, bool c45_over_c22)
{
int phy_reg;
- phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
+ phy_reg = phy_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS2,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
*devices_in_package = phy_reg << 16;
- phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1);
+ phy_reg = phy_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS1,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
*devices_in_package |= phy_reg;
@@ -780,7 +806,8 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
* the "devices in package" is invalid.
*/
static int get_phy_c45_ids(struct mii_bus *bus, int addr,
- struct phy_c45_device_ids *c45_ids)
+ struct phy_c45_device_ids *c45_ids,
+ bool c45_over_c22)
{
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
u32 devs_in_pkg = 0;
@@ -798,14 +825,16 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
* Some PHYs (88x3310) vendor space is not IEEE802.3
* compliant.
*/
- ret = phy_c45_probe_present(bus, addr, i);
+ ret = phy_c45_probe_present(bus, addr, i,
+ c45_over_c22);
if (ret < 0)
return -EIO;
if (!ret)
continue;
}
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
+ phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
}
@@ -815,7 +844,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
* MMD 0, as some 10G PHYs have zero Devices In package,
* e.g. Cortina CS4315/CS4340 PHY.
*/
- phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
+ phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
@@ -834,7 +864,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
* to ignore these if they do not contain IEEE 802.3
* registers.
*/
- ret = phy_c45_probe_present(bus, addr, i);
+ ret = phy_c45_probe_present(bus, addr, i,
+ c45_over_c22);
if (ret < 0)
return ret;
@@ -842,12 +873,14 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
continue;
}
- phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
+ phy_reg = phy_probe_mmd_read(bus, addr, i, MII_PHYSID1,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
c45_ids->device_ids[i] = phy_reg << 16;
- phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID2);
+ phy_reg = phy_probe_mmd_read(bus, addr, i, MII_PHYSID2,
+ c45_over_c22);
if (phy_reg < 0)
return -EIO;
c45_ids->device_ids[i] |= phy_reg;
@@ -961,7 +994,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
r = get_phy_c22_id(bus, addr, &phy_id);
break;
case PHY_ACCESS_C45:
- r = get_phy_c45_ids(bus, addr, &c45_ids);
+ r = get_phy_c45_ids(bus, addr, &c45_ids, false);
+ break;
+ case PHY_ACCESS_C45_OVER_C22:
+ r = get_phy_c45_ids(bus, addr, &c45_ids, true);
break;
default:
return ERR_PTR(-EIO);
@@ -976,7 +1012,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
* space, if successful, create the C45 PHY device.
*/
if (mode == PHY_ACCESS_C22 && phy_id == 0 && bus->read_c45) {
- r = get_phy_c45_ids(bus, addr, &c45_ids);
+ r = get_phy_c45_ids(bus, addr, &c45_ids, false);
if (!r)
return phy_device_create(bus, addr, phy_id,
PHY_ACCESS_C45, &c45_ids);
@@ -1058,7 +1094,7 @@ EXPORT_SYMBOL(phy_device_remove);
int phy_get_c45_ids(struct phy_device *phydev)
{
return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
- &phydev->c45_ids);
+ &phydev->c45_ids, false);
}
EXPORT_SYMBOL(phy_get_c45_ids);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a7aff91f4eb0..4852651f6326 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -541,10 +541,13 @@ struct macsec_ops;
*
* @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
* @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
+ * @PHY_ACCESS_C45_OVER_C22: indirectly access C45 registers by using by 802.3
+ * c22 MDIO transactions and registers 13 and 14.
*/
enum phy_access_mode {
PHY_ACCESS_C22,
PHY_ACCESS_C45,
+ PHY_ACCESS_C45_OVER_C22,
};
/**
--
2.39.2
If trying to register a C45 PHY on an MDIO bus which isn't capable of
C45 (either because the MDIO controller doesn't support it or because
C45 accesses are prohibited due to faulty C22 PHYs) we can fall back to
the new C45-over-C22 access method.
Signed-off-by: Michael Walle <[email protected]>
---
Please note, that both with the old and the new code compatible =
"ethernet-phy-idNNNN.NNNN" only works for the C22 case. I'm wondering if
compatible = "ethernet-phy-idNNNN.NNNN", "ethernet-phy-ieee802.3-c45
even makes sense because there might be multiple C45 ids. At least it is
an allowed pattern according to the device tree bindings. But with the
current code, the ethernet-phy-idNNNN.NNNN is ignored in the c45 case.
---
drivers/net/mdio/fwnode_mdio.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 972c8932c2fe..fed056d82b4e 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -115,7 +115,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct mii_timestamper *mii_ts = NULL;
struct pse_control *psec = NULL;
struct phy_device *phy;
- bool is_c45;
u32 phy_id;
int rc;
@@ -129,13 +128,19 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
goto clean_pse;
}
- is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
- if (is_c45 || fwnode_get_phy_id(child, &phy_id))
- phy = get_phy_device(bus, addr,
- is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
- else
+ if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45")) {
+ if (mdiobus_supports_c45(bus))
+ phy = get_phy_device(bus, addr, PHY_ACCESS_C45);
+ else
+ phy = get_phy_device(bus, addr,
+ PHY_ACCESS_C45_OVER_C22);
+ } else if (fwnode_get_phy_id(child, &phy_id) == 0) {
phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
NULL);
+ } else {
+ phy = get_phy_device(bus, addr, PHY_ACCESS_C22);
+ }
+
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
goto clean_mii_ts;
--
2.39.2
Fall back to C45-over-C22 when the MDIO bus isn't capable of doing C45
transfers. This might be the case if there are broken PHYs on the bus or
if the MDIO controller cannot do C45 transactions at all.
For this to work, split the PHY registration into three steps, as done
in the generic PHY probing code:
(1) add C22 PHYs
(2) scan for broken C22 PHYs
(3) add C45 PHYs
If step (2) detects a broken PHY, any PHYs will be added with
C45-over-C22 access in step (3). Step (3) also ensures, that
C45-over-C22 is used if C45 access is not supported at all on the bus.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/mdio/of_mdio.c | 63 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 7eb32ebb846d..e9d3cf6b68ee 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -100,6 +100,11 @@ static const struct of_device_id whitelist_phys[] = {
{}
};
+static bool of_mdiobus_child_is_c45_phy(struct device_node *child)
+{
+ return of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
+}
+
/*
* Return true if the child node is for a phy. It must either:
* o Compatible string of "ethernet-phy-idX.X"
@@ -118,7 +123,7 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
if (of_get_phy_id(child, &phy_id) != -EINVAL)
return true;
- if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
+ if (of_mdiobus_child_is_c45_phy(child))
return true;
if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c22"))
@@ -138,6 +143,32 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
}
EXPORT_SYMBOL(of_mdiobus_child_is_phy);
+static int of_mdiobus_register_child(struct mii_bus *mdio,
+ struct device_node *child, bool *scanphys)
+{
+ int addr, rc;
+
+ addr = of_mdio_parse_addr(&mdio->dev, child);
+ if (addr < 0) {
+ *scanphys = true;
+ return 0;
+ }
+
+ if (mdiobus_is_registered_device(mdio, addr))
+ return 0;
+
+ if (of_mdiobus_child_is_phy(child))
+ rc = of_mdiobus_register_phy(mdio, child, addr);
+ else
+ rc = of_mdiobus_register_device(mdio, child, addr);
+
+ if (rc == -ENODEV)
+ dev_err(&mdio->dev, "MDIO device at address %d is missing.\n",
+ addr);
+
+ return rc;
+}
+
/**
* __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
* @mdio: pointer to mii_bus structure
@@ -178,24 +209,26 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
if (rc)
return rc;
- /* Loop over the child nodes and register a phy_device for each phy */
+ /* Loop over the child nodes, skipping C45 PHYs so we can scan for
+ * broken C22 PHYs. The C45 PHYs will be registered afterwards.
+ */
for_each_available_child_of_node(np, child) {
- addr = of_mdio_parse_addr(&mdio->dev, child);
- if (addr < 0) {
- scanphys = true;
+ if (of_mdiobus_child_is_c45_phy(child))
continue;
- }
+ rc = of_mdiobus_register_child(mdio, child, &scanphys);
+ if (rc)
+ goto unregister;
+ }
- if (of_mdiobus_child_is_phy(child))
- rc = of_mdiobus_register_phy(mdio, child, addr);
- else
- rc = of_mdiobus_register_device(mdio, child, addr);
+ /* Some C22 PHYs are broken with C45 transactions. */
+ mdiobus_scan_for_broken_c45_access(mdio);
- if (rc == -ENODEV)
- dev_err(&mdio->dev,
- "MDIO device at address %d is missing.\n",
- addr);
- else if (rc)
+ /* Now add any missing C45 PHYs. If C45 access is not allowed, they
+ * will be registered with C45-over-C22 access.
+ */
+ for_each_available_child_of_node(np, child) {
+ rc = of_mdiobus_register_child(mdio, child, &scanphys);
+ if (rc)
goto unregister;
}
--
2.39.2
On Fri, Jun 23, 2023 at 12:29:16PM +0200, Michael Walle wrote:
...
> @@ -780,7 +806,8 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
> * the "devices in package" is invalid.
> */
> static int get_phy_c45_ids(struct mii_bus *bus, int addr,
> - struct phy_c45_device_ids *c45_ids)
> + struct phy_c45_device_ids *c45_ids,
> + bool c45_over_c22)
Hi Michael,
Please consider adding c45_over_c22 to the kernel doc for get_phy_c45_ids,
which appears a few lines above.
...
On Fri, Jun 23, 2023 at 12:29:19PM +0200, Michael Walle wrote:
...
> @@ -178,24 +209,26 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
> if (rc)
> return rc;
>
> - /* Loop over the child nodes and register a phy_device for each phy */
> + /* Loop over the child nodes, skipping C45 PHYs so we can scan for
> + * broken C22 PHYs. The C45 PHYs will be registered afterwards.
> + */
> for_each_available_child_of_node(np, child) {
> - addr = of_mdio_parse_addr(&mdio->dev, child);
> - if (addr < 0) {
> - scanphys = true;
> + if (of_mdiobus_child_is_c45_phy(child))
> continue;
> - }
> + rc = of_mdiobus_register_child(mdio, child, &scanphys);
> + if (rc)
> + goto unregister;
> + }
>
> - if (of_mdiobus_child_is_phy(child))
> - rc = of_mdiobus_register_phy(mdio, child, addr);
> - else
> - rc = of_mdiobus_register_device(mdio, child, addr);
> + /* Some C22 PHYs are broken with C45 transactions. */
> + mdiobus_scan_for_broken_c45_access(mdio);
Hi Michael,
Unfortunately this seems to cause a build fauilure
for x86_64 allmodconfig.
ERROR: modpost: "mdiobus_scan_for_broken_c45_access" [drivers/net/mdio/of_mdio.ko] undefined!
--
pw-bot: changes-requested
> int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
> {
> - int val;
> + struct mii_bus *bus = phydev->mdio.bus;
> + int phy_addr = phydev->mdio.addr;
> + bool check_rc = true;
> + int ret;
Although __phy_read_mmd() is exported as a GPL symbol, it is not in
fact used outside of this file. I think we can easily change it
signature.
> + switch (phydev->access_mode) {
Have access_mode passed in as a parameter. It then becomes a generic
low level helper.
The function which is really exported and expected to by used by PHY
drivers is:
int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
{
int ret;
phy_lock_mdio_bus(phydev);
ret = __phy_read_mmd(phydev, devad, regnum);
phy_unlock_mdio_bus(phydev);
return ret;
}
EXPORT_SYMBOL(phy_read_mmd);
This can easily pass phydev->access_mode as a parameter.
> +static int phy_probe_mmd_read(struct mii_bus *bus, int prtad, int devad,
> + u16 regnum, bool c45_over_c22)
> +{
What i don't like is this bool c45_over_c22. Why have both the enum
for the three access modes, and this bool. Pass an access mode.
> + int ret;
> +
> + if (!c45_over_c22)
> + return mdiobus_c45_read(bus, prtad, devad, regnum);
> +
> + mutex_lock(&bus->mdio_lock);
> +
> + ret = __phy_mmd_indirect(bus, prtad, devad, regnum);
> + if (ret)
> + goto out;
> +
> + ret = __mdiobus_read(bus, prtad, MII_MMD_DATA);
And then this just uses the generic low level __phy_read_mmd().
Andrew
Am 2023-06-24 22:15, schrieb Andrew Lunn:
>> int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
>> {
>> - int val;
>> + struct mii_bus *bus = phydev->mdio.bus;
>> + int phy_addr = phydev->mdio.addr;
>> + bool check_rc = true;
>> + int ret;
>
> Although __phy_read_mmd() is exported as a GPL symbol, it is not in
> fact used outside of this file. I think we can easily change it
> signature.
>
>> + switch (phydev->access_mode) {
>
> Have access_mode passed in as a parameter. It then becomes a generic
> low level helper.
Are you sure? Why is it a generic helper then? You still need the phydev
parameter. E.g. for the bus, the address and more importantly, for the
phydev->drv->read_mmd op. And in this case, you can't use it for my new
phy_probe_mmd_read() because there is no phydev structure at that time.
Also __phy_read_mmd() is just one of a whole block of (unlocked)
functions
to access the MMDs of a PHY. So, to be consistent you'd have to change
all
the other ones, too. No?
That being said, what about a true generic helper, without the phydev
parameter, which is then called in __phy_{read,write}_mmd()? Where
would that helper belong to? Without the C45-over-C22 I'd have suggested
to put it into mdio_bus.c but C45-over-C22 being a PHY thing, I guess
it should be in phy-core.c.
> The function which is really exported and expected to by used by PHY
> drivers is:
>
> int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
> {
> int ret;
>
> phy_lock_mdio_bus(phydev);
> ret = __phy_read_mmd(phydev, devad, regnum);
> phy_unlock_mdio_bus(phydev);
>
> return ret;
> }
> EXPORT_SYMBOL(phy_read_mmd);
>
> This can easily pass phydev->access_mode as a parameter.
>
>> +static int phy_probe_mmd_read(struct mii_bus *bus, int prtad, int
>> devad,
>> + u16 regnum, bool c45_over_c22)
>> +{
>
> What i don't like is this bool c45_over_c22. Why have both the enum
> for the three access modes, and this bool. Pass an access mode.
Ok, but just to be sure, access mode c22 is then a "return -EINVAL".
>> + int ret;
>> +
>> + if (!c45_over_c22)
>> + return mdiobus_c45_read(bus, prtad, devad, regnum);
>> +
>> + mutex_lock(&bus->mdio_lock);
>> +
>> + ret = __phy_mmd_indirect(bus, prtad, devad, regnum);
>> + if (ret)
>> + goto out;
>> +
>> + ret = __mdiobus_read(bus, prtad, MII_MMD_DATA);
>
> And then this just uses the generic low level __phy_read_mmd().
See above, no there is no *phydev.
-michael
Hi Simon,
Am 2023-06-23 22:48, schrieb Simon Horman:
> On Fri, Jun 23, 2023 at 12:29:19PM +0200, Michael Walle wrote:
>
> ...
>
>> @@ -178,24 +209,26 @@ int __of_mdiobus_register(struct mii_bus *mdio,
>> struct device_node *np,
>> if (rc)
>> return rc;
>>
>> - /* Loop over the child nodes and register a phy_device for each phy
>> */
>> + /* Loop over the child nodes, skipping C45 PHYs so we can scan for
>> + * broken C22 PHYs. The C45 PHYs will be registered afterwards.
>> + */
>> for_each_available_child_of_node(np, child) {
>> - addr = of_mdio_parse_addr(&mdio->dev, child);
>> - if (addr < 0) {
>> - scanphys = true;
>> + if (of_mdiobus_child_is_c45_phy(child))
>> continue;
>> - }
>> + rc = of_mdiobus_register_child(mdio, child, &scanphys);
>> + if (rc)
>> + goto unregister;
>> + }
>>
>> - if (of_mdiobus_child_is_phy(child))
>> - rc = of_mdiobus_register_phy(mdio, child, addr);
>> - else
>> - rc = of_mdiobus_register_device(mdio, child, addr);
>> + /* Some C22 PHYs are broken with C45 transactions. */
>> + mdiobus_scan_for_broken_c45_access(mdio);
>
> Hi Michael,
>
> Unfortunately this seems to cause a build fauilure
> for x86_64 allmodconfig.
>
> ERROR: modpost: "mdiobus_scan_for_broken_c45_access"
> [drivers/net/mdio/of_mdio.ko] undefined!
Oops, sorry. Seems I've forgot to export it. I guess it should
be EXPORT_SYMBOL_GPL().
-michael