* net: mdio: support c45 peripherals on c22 only capable mdio controllers
@ 2021-03-31 14:17 Danilo Krummrich
2021-03-31 14:17 ` [PATCH 1/2] net: mdio: rename mii bus probe_capabilities Danilo Krummrich
2021-03-31 14:17 ` [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses Danilo Krummrich
0 siblings, 2 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-03-31 14:17 UTC (permalink / raw)
To: linux, davem, andrew, hkallweit1; +Cc: netdev, linux-kernel, jeremy.linton
This patch series adds support for clause 45 peripherals on busses driven
by an mdio controller that is only capable of clause 22 frame format
messages by indirect bus accesses.
In order to do so we
- change the name of the field probe_capabilities to capabilities in
struct mii_bus to represent the bus capabilities in general (not only
for probing)
- add functions mdiobus_*_mmd() and mdiobus_indirect_mmd() to handle
indirect bus accesses
- let mdiobus_c45_*() functions check the bus capabilities in order to
decide whether a real clause 45 bus transfer or indirect access should
be performed
- use the new functions to simplify existing code a little bit
- and finally allow probing for clause 45 peripherals on busses that are
not capable of the clause 45 frame format
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] net: mdio: rename mii bus probe_capabilities
2021-03-31 14:17 net: mdio: support c45 peripherals on c22 only capable mdio controllers Danilo Krummrich
@ 2021-03-31 14:17 ` Danilo Krummrich
2021-03-31 14:17 ` [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses Danilo Krummrich
1 sibling, 0 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-03-31 14:17 UTC (permalink / raw)
To: linux, davem, andrew, hkallweit1
Cc: netdev, linux-kernel, jeremy.linton, Danilo Krummrich
Rename the probe_capabilities field of struct mii_bus to capabilities.
This field represents the supported frame formats of the mdio controller
backing this bus as by IEEE 802.3 in general. This is not specific to the
probing procedure of the bus.
Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
---
drivers/net/ethernet/freescale/xgmac_mdio.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 2 +-
drivers/net/phy/mdio_bus.c | 2 +-
include/linux/phy.h | 7 +++++--
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index bfa2826c5545..bda04154fca2 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -268,7 +268,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
bus->read = xgmac_mdio_read;
bus->write = xgmac_mdio_write;
bus->parent = &pdev->dev;
- bus->probe_capabilities = MDIOBUS_C22_C45;
+ bus->capabilities = MDIOBUS_C22_C45;
snprintf(bus->id, MII_BUS_ID_SIZE, "%pa", &res->start);
/* Set the PHY base address */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index d64116e0543e..917537731131 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -366,7 +366,7 @@ int stmmac_mdio_register(struct net_device *ndev)
new_bus->name = "stmmac";
if (priv->plat->has_gmac4)
- new_bus->probe_capabilities = MDIOBUS_C22_C45;
+ new_bus->capabilities = MDIOBUS_C22_C45;
if (priv->plat->has_xgmac) {
new_bus->read = &stmmac_xgmac2_mdio_read;
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 823518554079..d03e40a0fbae 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -670,7 +670,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
struct phy_device *phydev = ERR_PTR(-ENODEV);
int err;
- switch (bus->probe_capabilities) {
+ switch (bus->capabilities) {
case MDIOBUS_NO_CAP:
case MDIOBUS_C22:
phydev = get_phy_device(bus, addr, false);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1a12e4436b5b..ba5eb317a471 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -362,13 +362,16 @@ struct mii_bus {
/** @reset_gpiod: Reset GPIO descriptor pointer */
struct gpio_desc *reset_gpiod;
- /** @probe_capabilities: bus capabilities, used for probing */
+ /**
+ * @capabilities: bus capabilities, representing supported frame
+ * formats as by IEEE 802.3
+ */
enum {
MDIOBUS_NO_CAP = 0,
MDIOBUS_C22,
MDIOBUS_C45,
MDIOBUS_C22_C45,
- } probe_capabilities;
+ } capabilities;
/** @shared_lock: protect access to the shared element */
struct mutex shared_lock;
--
2.31.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-03-31 14:17 net: mdio: support c45 peripherals on c22 only capable mdio controllers Danilo Krummrich
2021-03-31 14:17 ` [PATCH 1/2] net: mdio: rename mii bus probe_capabilities Danilo Krummrich
@ 2021-03-31 14:17 ` Danilo Krummrich
2021-03-31 16:27 ` Andrew Lunn
1 sibling, 1 reply; 22+ messages in thread
From: Danilo Krummrich @ 2021-03-31 14:17 UTC (permalink / raw)
To: linux, davem, andrew, hkallweit1
Cc: netdev, linux-kernel, jeremy.linton, Danilo Krummrich
There are still a lot of mdio controllers which don't support the clause
45 frame format as well as drivers for mdio controllers which don't
implement the cause 45 mode of the controller even if natively supported
by the hardware. Therefore it makes sense to support clause 45 peripherals
on busses that support clause 22 transfers only by indirect access.
In order to do so we can use the capabilitiy field of the struct mii_bus
to distinguish between busses that natively support clause 45 and those
who don't. Based on that the mdiobus_c45_*() functions can either issue
a MII_ADDR_C45 flagged request to the bus driver or perform an indirect
access.
The indirect access is performed by the introduced mdiobus_*_mmd()
functions. While performing the indirect access sequence in
mdiobus_indirect_mmd() we check for potential errors occurring in the
sequence, which was not done previously and just assumed to be
successful.
Signed-off-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
---
drivers/net/phy/mdio_bus.c | 265 ++++++++++++++++++++++++++++++++++++-
drivers/net/phy/phy-core.c | 46 ++-----
drivers/net/phy/phy.c | 19 ++-
include/linux/mdio.h | 36 ++---
4 files changed, 298 insertions(+), 68 deletions(-)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index d03e40a0fbae..c80ed65666ac 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
struct phy_device *phydev = ERR_PTR(-ENODEV);
int err;
+ /* In case of NO_CAP and C22 only, we still can try to scan for C45
+ * devices, since indirect access will be used for busses that are not
+ * capable of C45 frame format.
+ */
switch (bus->capabilities) {
case MDIOBUS_NO_CAP:
case MDIOBUS_C22:
- phydev = get_phy_device(bus, addr, false);
- break;
- case MDIOBUS_C45:
- phydev = get_phy_device(bus, addr, true);
- break;
case MDIOBUS_C22_C45:
phydev = get_phy_device(bus, addr, false);
if (IS_ERR(phydev))
phydev = get_phy_device(bus, addr, true);
break;
+ case MDIOBUS_C45:
+ phydev = get_phy_device(bus, addr, true);
+ break;
}
if (IS_ERR(phydev))
@@ -903,6 +905,259 @@ int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
}
EXPORT_SYMBOL(mdiobus_write);
+/**
+ * mdiobus_indirect_mmd - Prepares MMD indirect access
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * Prepares indirect MMD access, such that only the MII_MMD_DATA register is
+ * left to be read or written. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+static int mdiobus_indirect_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum)
+{
+ int err;
+
+ /* Write the desired MMD Devad */
+ err = __mdiobus_write(bus, addr, MII_MMD_CTRL, devad);
+ if (err)
+ goto out;
+
+ /* Write the desired MMD register address */
+ err = __mdiobus_write(bus, addr, MII_MMD_DATA, regnum);
+ if (err)
+ goto out;
+
+ /* Select the Function : DATA with no post increment */
+ err = __mdiobus_write(bus, addr, MII_MMD_CTRL,
+ devad | MII_MMD_CTRL_NOINCR);
+
+out:
+ return err;
+}
+
+/**
+ * __mdiobus_read_mmd - Unlocked version of the mdiobus_read_mmd function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * Read a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_read_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum)
+{
+ int retval;
+
+ retval = mdiobus_indirect_mmd(bus, addr, devad, regnum);
+ if (retval)
+ goto out;
+
+ /* Read the content of the MMD's selected register */
+ retval = __mdiobus_read(bus, addr, MII_MMD_DATA);
+
+out:
+ return retval;
+}
+EXPORT_SYMBOL(__mdiobus_read_mmd);
+
+/**
+ * __mdiobus_write_mmd - Unlocked version of the mdiobus_write_mmd function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * Write a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_write_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum,
+ u16 val)
+{
+ int err;
+
+ err = mdiobus_indirect_mmd(bus, addr, devad, regnum);
+ if (err)
+ goto out;
+
+ /* Write the data into MMD's selected register */
+ err = __mdiobus_write(bus, addr, MII_MMD_DATA, val);
+
+out:
+ return err;
+}
+EXPORT_SYMBOL(__mdiobus_write_mmd);
+
+/**
+ * mdiobus_read_mmd - Convenience function for indirect MMD reads
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_read_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum)
+{
+ int retval;
+
+ mutex_lock(&bus->mdio_lock);
+ retval = __mdiobus_read_mmd(bus, addr, devad, regnum);
+ mutex_unlock(&bus->mdio_lock);
+
+ return retval;
+}
+EXPORT_SYMBOL(mdiobus_read_mmd);
+
+/**
+ * mdiobus_write_mmd - Convenience function for indirect MMD writes
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_write_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum,
+ u16 val)
+{
+ int err;
+
+ mutex_lock(&bus->mdio_lock);
+ err = __mdiobus_write_mmd(bus, addr, devad, regnum, val);
+ mutex_unlock(&bus->mdio_lock);
+
+ return err;
+}
+EXPORT_SYMBOL(mdiobus_write_mmd);
+
+/**
+ * __mdiobus_c45_read - Unlocked version of the mdiobus_c45_read function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * Read a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum)
+{
+ int ret = -EOPNOTSUPP;
+
+ switch (bus->capabilities) {
+ case MDIOBUS_NO_CAP:
+ case MDIOBUS_C22:
+ ret = __mdiobus_read_mmd(bus, addr, devad, regnum);
+ break;
+ case MDIOBUS_C45:
+ case MDIOBUS_C22_C45:
+ ret = __mdiobus_read(bus, addr,
+ mdiobus_c45_addr(devad, regnum));
+ break;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(__mdiobus_c45_read);
+
+/**
+ * __mdiobus_c45_write - Unlocked version of the mdiobus_c45_write function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * Write a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+ u16 val)
+{
+ int ret = -EOPNOTSUPP;
+
+ switch (bus->capabilities) {
+ case MDIOBUS_NO_CAP:
+ case MDIOBUS_C22:
+ ret = __mdiobus_write_mmd(bus, addr, devad, regnum, val);
+ break;
+ case MDIOBUS_C45:
+ case MDIOBUS_C22_C45:
+ ret = __mdiobus_write(bus, addr,
+ mdiobus_c45_addr(devad, regnum), val);
+ break;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(__mdiobus_c45_write);
+
+/**
+ * mdiobus_c45_read - Convenience function for clause 45 reads
+ * The read is either performed by clause 45 frame format or by an indirect
+ * access, depending on the capabilities of the bus.
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum)
+{
+ int retval;
+
+ mutex_lock(&bus->mdio_lock);
+ retval = __mdiobus_c45_read(bus, addr, devad, regnum);
+ mutex_unlock(&bus->mdio_lock);
+
+ return retval;
+}
+EXPORT_SYMBOL(mdiobus_c45_read);
+
+/**
+ * mdiobus_c45_write - Convenience function for clause 45 writes
+ * The write is either performed by clause 45 frame format or by an indirect
+ * access, depending on the capabilities of the bus.
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: the device address
+ * @regnum: register number to read
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+ u16 val)
+{
+ int err;
+
+ mutex_lock(&bus->mdio_lock);
+ err = __mdiobus_c45_write(bus, addr, devad, regnum, val);
+ mutex_unlock(&bus->mdio_lock);
+
+ return err;
+}
+EXPORT_SYMBOL(mdiobus_c45_write);
+
/**
* mdiobus_modify - Convenience function for modifying a given mdio device
* register
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 8d333d3084ed..5f1601e12162 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -442,20 +442,6 @@ int phy_speed_down_core(struct phy_device *phydev)
return __set_linkmode_max_speed(min_common_speed, phydev->advertising);
}
-static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
- u16 regnum)
-{
- /* Write the desired MMD Devad */
- __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
-
- /* Write the desired MMD register address */
- __mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
-
- /* Select the Function : DATA with no post increment */
- __mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
- devad | MII_MMD_CTRL_NOINCR);
-}
-
/**
* __phy_read_mmd - Convenience function for reading a register
* from an MMD on a given PHY.
@@ -472,20 +458,15 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
if (regnum > (u16)~0 || devad > 32)
return -EINVAL;
- if (phydev->drv && phydev->drv->read_mmd) {
+ if (phydev->drv && phydev->drv->read_mmd)
val = phydev->drv->read_mmd(phydev, devad, regnum);
- } else if (phydev->is_c45) {
+ else if (phydev->is_c45)
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);
+ else
+ val = __mdiobus_read_mmd(phydev->mdio.bus, phydev->mdio.addr,
+ devad, regnum);
- /* Read the content of the MMD's selected register */
- val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
- }
return val;
}
EXPORT_SYMBOL(__phy_read_mmd);
@@ -528,22 +509,15 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
if (regnum > (u16)~0 || devad > 32)
return -EINVAL;
- if (phydev->drv && phydev->drv->write_mmd) {
+ if (phydev->drv && phydev->drv->write_mmd)
ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
- } else if (phydev->is_c45) {
+ else if (phydev->is_c45)
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);
-
- /* Write the data into MMD's selected register */
- __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
+ else
+ ret = __mdiobus_write_mmd(phydev->mdio.bus, phydev->mdio.addr,
+ devad, regnum, val);
- ret = 0;
- }
return ret;
}
EXPORT_SYMBOL(__phy_write_mmd);
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fc2e7cb5b2e5..fb07832f378a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -346,20 +346,23 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
if (mdio_phy_id_is_c45(mii_data->phy_id)) {
prtad = mdio_phy_id_prtad(mii_data->phy_id);
devad = mdio_phy_id_devad(mii_data->phy_id);
- devad = mdiobus_c45_addr(devad, mii_data->reg_num);
+
+ mii_data->val_out = mdiobus_c45_read(phydev->mdio.bus,
+ prtad, devad,
+ mii_data->reg_num);
} else {
prtad = mii_data->phy_id;
devad = mii_data->reg_num;
+
+ mii_data->val_out = mdiobus_read(phydev->mdio.bus,
+ prtad, devad);
}
- mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
- devad);
return 0;
case SIOCSMIIREG:
if (mdio_phy_id_is_c45(mii_data->phy_id)) {
prtad = mdio_phy_id_prtad(mii_data->phy_id);
devad = mdio_phy_id_devad(mii_data->phy_id);
- devad = mdiobus_c45_addr(devad, mii_data->reg_num);
} else {
prtad = mii_data->phy_id;
devad = mii_data->reg_num;
@@ -403,7 +406,13 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
}
}
- mdiobus_write(phydev->mdio.bus, prtad, devad, val);
+ if (mdio_phy_id_is_c45(mii_data->phy_id))
+ mii_data->val_out = mdiobus_c45_write(phydev->mdio.bus,
+ prtad, devad,
+ mii_data->reg_num,
+ val);
+ else
+ mdiobus_write(phydev->mdio.bus, prtad, devad, val);
if (prtad == phydev->mdio.addr &&
devad == MII_BMCR &&
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index ffb787d5ebde..7bcd76914154 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -347,35 +347,27 @@ int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
u16 set);
+int __mdiobus_read_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum);
+int __mdiobus_write_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum,
+ u16 val);
+
+int mdiobus_read_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum);
+int mdiobus_write_mmd(struct mii_bus *bus, int addr, u16 devad, u32 regnum,
+ u16 val);
+
static inline u32 mdiobus_c45_addr(int devad, u16 regnum)
{
return MII_ADDR_C45 | devad << MII_DEVADDR_C45_SHIFT | regnum;
}
-static inline int __mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
- u16 regnum)
-{
- return __mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
-}
+int __mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum);
+int __mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+ u16 val);
-static inline int __mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
- u16 regnum, u16 val)
-{
- return __mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum),
- val);
-}
+int mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum);
-static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
- u16 regnum)
-{
- return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
-}
-
-static inline int mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
- u16 regnum, u16 val)
-{
- return mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum), val);
-}
+int mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+ u16 val);
int mdiobus_register_device(struct mdio_device *mdiodev);
int mdiobus_unregister_device(struct mdio_device *mdiodev);
--
2.31.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-03-31 14:17 ` [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses Danilo Krummrich
@ 2021-03-31 16:27 ` Andrew Lunn
2021-03-31 17:58 ` danilokrummrich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Lunn @ 2021-03-31 16:27 UTC (permalink / raw)
To: Danilo Krummrich
Cc: linux, davem, hkallweit1, netdev, linux-kernel, jeremy.linton
> @@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
> struct phy_device *phydev = ERR_PTR(-ENODEV);
> int err;
>
> + /* In case of NO_CAP and C22 only, we still can try to scan for C45
> + * devices, since indirect access will be used for busses that are not
> + * capable of C45 frame format.
> + */
> switch (bus->capabilities) {
> case MDIOBUS_NO_CAP:
> case MDIOBUS_C22:
> - phydev = get_phy_device(bus, addr, false);
> - break;
> - case MDIOBUS_C45:
> - phydev = get_phy_device(bus, addr, true);
> - break;
> case MDIOBUS_C22_C45:
> phydev = get_phy_device(bus, addr, false);
> if (IS_ERR(phydev))
> phydev = get_phy_device(bus, addr, true);
> break;
> + case MDIOBUS_C45:
> + phydev = get_phy_device(bus, addr, true);
> + break;
> }
I think this is going to cause problems.
commit 0231b1a074c672f8c00da00a57144072890d816b
Author: Kevin Hao <haokexin@gmail.com>
Date: Tue Mar 20 09:44:53 2018 +0800
net: phy: realtek: Use the dummy stubs for MMD register access for rtl8211b
The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
("gianfar: Disable EEE autoneg by default"). The reason is that
even though the rtl8211b doesn't support the MMD extended registers
access, it does return some random values if we trying to access
the MMD register via indirect method. This makes it seem that the
EEE is supported by this phy device. And the subsequent writing to
the MMD registers does cause the phy malfunction. So use the dummy
stubs for the MMD register access to fix this issue.
Indirect access to C45 via C22 is not a guaranteed part of C22. So
there are C22 only PHYs which return random junk when you try to use
this access method.
I'm also a bit confused why this is actually needed. PHY drivers which
make use of C45 use the functions phy_read_mmd(), phy_write_mmd().
int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
{
int val;
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 (phydev->is_c45) {
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);
/* Read the content of the MMD's selected register */
val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
}
return val;
}
So if the device is a c45 device, C45 transfers are used, otherwise it
falls back to mmd_phy_indirect(), which is C45 over C22.
Why does this not work for you?
Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-03-31 16:27 ` Andrew Lunn
@ 2021-03-31 17:58 ` danilokrummrich
2021-03-31 18:35 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 22+ messages in thread
From: danilokrummrich @ 2021-03-31 17:58 UTC (permalink / raw)
To: Andrew Lunn; +Cc: linux, davem, hkallweit1, netdev, linux-kernel, jeremy.linton
Hi Andrew,
On 2021-03-31 18:27, Andrew Lunn wrote:
>> @@ -670,19 +670,21 @@ struct phy_device *mdiobus_scan(struct mii_bus
>> *bus, int addr)
>> struct phy_device *phydev = ERR_PTR(-ENODEV);
>> int err;
>>
>> + /* In case of NO_CAP and C22 only, we still can try to scan for C45
>> + * devices, since indirect access will be used for busses that are
>> not
>> + * capable of C45 frame format.
>> + */
>> switch (bus->capabilities) {
>> case MDIOBUS_NO_CAP:
>> case MDIOBUS_C22:
>> - phydev = get_phy_device(bus, addr, false);
>> - break;
>> - case MDIOBUS_C45:
>> - phydev = get_phy_device(bus, addr, true);
>> - break;
>> case MDIOBUS_C22_C45:
>> phydev = get_phy_device(bus, addr, false);
>> if (IS_ERR(phydev))
>> phydev = get_phy_device(bus, addr, true);
>> break;
>> + case MDIOBUS_C45:
>> + phydev = get_phy_device(bus, addr, true);
>> + break;
>> }
>
> I think this is going to cause problems.
Please note that this patch does not change already existing behaviour,
it does
only extend it with the option to drive C45 peripherals from a bus not
being
capable of C45 framing.
For this cited change the only thing happening is that if
get_phy_device()
already failed for probing with is_c45==false (C22 devices) it tries to
probe
with is_c45==true (C45 devices) which then either results into actual
C45 frame
transfers or indirect accesses by calling mdiobus_c45_*() functions.
This is a valid approach, since we made sure that even if the MDIO
controller does
not support C45 framing we can go with indirect accesses.
>
> commit 0231b1a074c672f8c00da00a57144072890d816b
> Author: Kevin Hao <haokexin@gmail.com>
> Date: Tue Mar 20 09:44:53 2018 +0800
>
> net: phy: realtek: Use the dummy stubs for MMD register access for
> rtl8211b
>
> The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
> ("gianfar: Disable EEE autoneg by default"). The reason is that
> even though the rtl8211b doesn't support the MMD extended registers
> access, it does return some random values if we trying to access
> the MMD register via indirect method. This makes it seem that the
> EEE is supported by this phy device. And the subsequent writing to
> the MMD registers does cause the phy malfunction. So use the dummy
> stubs for the MMD register access to fix this issue.
This is something different, here the issue is that an indirect access
does
not work with a PHY being registered with is_c45==false. This is not
related
to this change.
>
> Indirect access to C45 via C22 is not a guaranteed part of C22. So
> there are C22 only PHYs which return random junk when you try to use
> this access method.
For C22 only PHYs nothing will change. If there is not an indirect
access
to a C22 PHY already, then there will not be one by applying this patch
either.
>
> I'm also a bit confused why this is actually needed. PHY drivers which
> make use of C45 use the functions phy_read_mmd(), phy_write_mmd().
I'm looking on it from the perspective of the MDIO controller. If the
controller is not capable of C45 framing this doesn't help.
From only the PHY's capability point of view this is fine, indeed.
>
> int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
> {
> int val;
>
> 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 (phydev->is_c45) {
> 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);
>
> /* Read the content of the MMD's selected register */
> val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
> }
> return val;
> }
>
> So if the device is a c45 device, C45 transfers are used, otherwise it
That's the issue I'm addressing, if the device is a C45 device and the
MDIO
controller is not capable of C45 framing, currently, the device won't be
probed
as a C45 device, because mdiobus_c45_read() will fail. Instead with this
patch
mdiobus_c45_read() can check the bus's capabilities and perform indirect
accesses
in case the MDIO controller is not capable of C45 framing, and therefore
be able
to probe C45 PHYs from a MDIO controller not being capable of C45
framing.
> falls back to mmd_phy_indirect(), which is C45 over C22.
Only if is_c45==false, that's not what I'm addressing. I'm addressing
the case that
is_c45==true, but the MDIO controller does not support C45 framing.
>
> Why does this not work for you?
As explained inline above.
>
> Andrew
Kind regards,
Danilo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-03-31 17:58 ` danilokrummrich
@ 2021-03-31 18:35 ` Russell King - ARM Linux admin
2021-04-01 1:23 ` danilokrummrich
0 siblings, 1 reply; 22+ messages in thread
From: Russell King - ARM Linux admin @ 2021-03-31 18:35 UTC (permalink / raw)
To: danilokrummrich
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummrich@dk-develop.de wrote:
> For this cited change the only thing happening is that if get_phy_device()
> already failed for probing with is_c45==false (C22 devices) it tries to
> probe with is_c45==true (C45 devices) which then either results into actual
> C45 frame transfers or indirect accesses by calling mdiobus_c45_*() functions.
Please explain why and how a PHY may not appear to be present using
C22 frames to read the ID registers, but does appear to be present
when using C22 frames to the C45 indirect registers - and summarise
which PHYs have this behaviour.
It seems very odd that any PHY would only implement C45 indirect
registers in the C22 register space.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-03-31 18:35 ` Russell King - ARM Linux admin
@ 2021-04-01 1:23 ` danilokrummrich
2021-04-01 8:48 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 22+ messages in thread
From: danilokrummrich @ 2021-04-01 1:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On 2021-03-31 20:35, Russell King - ARM Linux admin wrote:
> On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummrich@dk-develop.de
> wrote:
>> For this cited change the only thing happening is that if
>> get_phy_device()
>> already failed for probing with is_c45==false (C22 devices) it tries
>> to
>> probe with is_c45==true (C45 devices) which then either results into
>> actual
>> C45 frame transfers or indirect accesses by calling mdiobus_c45_*()
>> functions.
>
> Please explain why and how a PHY may not appear to be present using
> C22 frames to read the ID registers, but does appear to be present
> when using C22 frames to the C45 indirect registers - and summarise
> which PHYs have this behaviour.
>
> It seems very odd that any PHY would only implement C45 indirect
> registers in the C22 register space.
Honestly, I can't list examples of that case (at least none that have an
upstream driver already). This part of my patch, to fall back to c45 bus
probing when c22 probing does not succeed, is also motivated by the fact
that this behaviour was already introduced with this patch:
commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c
Author: Jeremy Linton <jeremy.linton@arm.com>
Date: Mon Jun 22 20:35:32 2020 +0530
net: phy: Allow mdio buses to auto-probe c45 devices
The mdiobus_scan logic is currently hardcoded to only
work with c22 devices. This works fairly well in most
cases, but its possible that a c45 device doesn't respond
despite being a standard phy. If the parent hardware
is capable, it makes sense to scan for c22 devices before
falling back to c45.
As we want this to reflect the capabilities of the STA,
lets add a field to the mii_bus structure to represent
the capability. That way devices can opt into the extended
scanning. Existing users should continue to default to c22
only scanning as long as they are zero'ing the structure
before use.
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
In this patch i.a. the following lines were added.
+ case MDIOBUS_C22_C45:
+ phydev = get_phy_device(bus, addr, false);
+ if (IS_ERR(phydev))
+ phydev = get_phy_device(bus, addr, true);
+ break;
I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since
with my patch MDIO controllers with those capabilities can handle c45
bus
probing with indirect accesses.
[By the way, I'm unsure if this order for MDIO bus controllers with the
capability MDIOBUS_C22_C45 makes sense, because if we assume that the
majority of c45 PHYs responds well to c22 probing (which I'm convinced
of)
the PHY would still be registered as is_c45==false, which results in the
fact
that even though the underlying bus is capable of real c45 framing only
indirect accessing would be performed. But this is another topic and
unrelated to the patch.]
However, this is not the main motivation of my patch. The main driver is
of_mdiobus_register_phy():
is_c45 = of_device_is_compatible(child,
"ethernet-phy-ieee802.3-c45");
if (!is_c45 && !of_get_phy_id(child, &phy_id))
phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
else
phy = get_phy_device(mdio, addr, is_c45);
In the case a PHY is registered as a c45 compatible PHY in the device
tree,
it is probed in c45 mode and therefore finally mdiobus_c45_read() is
called,
which as by now just expects the underlying MDIO bus controller to be
capable
to do c45 framing and therefore the operation would fail in case it is
not.
Hence, in my opinion it is useful to fall back to indirect accesses in
such a
case to be able to support those PHYs.
There is a similar issue in phy_mii_ioctl(). Let's assume a c45 capable
PHY is
connected to a MDIO bus controller that is not capable of c45 framing.
We can
also assume that it was probed with c22 bus probing, since without this
patch
nothing else is possible.
Now, there might be an ioctl() asking for a c45 transfer by specifying
MDIO_PHY_ID_C45_MASK e.g. in order to access a different MMD's register,
since the PHY is actually capable of c45. Currently, this would result
in
devad = mdiobus_c45_addr(devad, mii_data->reg_num);
mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad, devad);
calls, which would fail, since the bus doesn't support it. Instead
falling back
to indirect access might be the better option. Surely, the userspace
program
could implement the indirect access as well, but I think this way it's
just more
convenient, e.g. "phytool read iface/addr:devad/reg".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-01 1:23 ` danilokrummrich
@ 2021-04-01 8:48 ` Russell King - ARM Linux admin
2021-04-02 1:10 ` Danilo Krummrich
0 siblings, 1 reply; 22+ messages in thread
From: Russell King - ARM Linux admin @ 2021-04-01 8:48 UTC (permalink / raw)
To: danilokrummrich
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Thu, Apr 01, 2021 at 03:23:05AM +0200, danilokrummrich@dk-develop.de wrote:
> On 2021-03-31 20:35, Russell King - ARM Linux admin wrote:
> > On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummrich@dk-develop.de
> > wrote:
> > > For this cited change the only thing happening is that if
> > > get_phy_device()
> > > already failed for probing with is_c45==false (C22 devices) it tries
> > > to
> > > probe with is_c45==true (C45 devices) which then either results into
> > > actual
> > > C45 frame transfers or indirect accesses by calling mdiobus_c45_*()
> > > functions.
> >
> > Please explain why and how a PHY may not appear to be present using
> > C22 frames to read the ID registers, but does appear to be present
> > when using C22 frames to the C45 indirect registers - and summarise
> > which PHYs have this behaviour.
> >
> > It seems very odd that any PHY would only implement C45 indirect
> > registers in the C22 register space.
>
> Honestly, I can't list examples of that case (at least none that have an
> upstream driver already). This part of my patch, to fall back to c45 bus
> probing when c22 probing does not succeed, is also motivated by the fact
> that this behaviour was already introduced with this patch:
So, if I understand what you've just said, you want to make a change to
the kernel to add support for something that you don't need and don't
know that there's any hardware that needs it. Is that correct?
> commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c
> Author: Jeremy Linton <jeremy.linton@arm.com>
> Date: Mon Jun 22 20:35:32 2020 +0530
>
> net: phy: Allow mdio buses to auto-probe c45 devices
>
> The mdiobus_scan logic is currently hardcoded to only
> work with c22 devices. This works fairly well in most
> cases, but its possible that a c45 device doesn't respond
> despite being a standard phy. If the parent hardware
> is capable, it makes sense to scan for c22 devices before
> falling back to c45.
>
> As we want this to reflect the capabilities of the STA,
> lets add a field to the mii_bus structure to represent
> the capability. That way devices can opt into the extended
> scanning. Existing users should continue to default to c22
> only scanning as long as they are zero'ing the structure
> before use.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> In this patch i.a. the following lines were added.
>
> + case MDIOBUS_C22_C45:
> + phydev = get_phy_device(bus, addr, false);
> + if (IS_ERR(phydev))
> + phydev = get_phy_device(bus, addr, true);
> + break;
>
> I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since
> with my patch MDIO controllers with those capabilities can handle c45 bus
> probing with indirect accesses.
If the PHY doesn't respond to C22 accesses but is a C45 PHY, then how
can this work (you seem to have essentially said it doesn't above.)
> [By the way, I'm unsure if this order for MDIO bus controllers with the
> capability MDIOBUS_C22_C45 makes sense, because if we assume that the
> majority of c45 PHYs responds well to c22 probing (which I'm convinced of)
There are some which don't - Clause 45 allows PHYs not to implement
support for Clause 22 accesses.
> the PHY would still be registered as is_c45==false, which results in the
> fact
> that even though the underlying bus is capable of real c45 framing only
> indirect accessing would be performed. But this is another topic and
> unrelated to the patch.]
We make no distinction between IDs found via Clause 22 probing and IDs
found via Clause 45 probing; the PHY driver will match either way. We
don't know of any cases where the IDs are different between these two.
So we still end up with the same driver being matched.
Also note that there are MDIO controllers that support clause 22 and
clause 45, but which require clause 22 to be probed first. Not
everything is MDIO at the bus level anymore - there's PHYs that support
I2C, and the I2C format of a clause 45 read is identical to a clause 22
write.
> In the case a PHY is registered as a c45 compatible PHY in the device tree,
> it is probed in c45 mode and therefore finally mdiobus_c45_read() is
> called,
> which as by now just expects the underlying MDIO bus controller to be
> capable
> to do c45 framing and therefore the operation would fail in case it is not.
> Hence, in my opinion it is useful to fall back to indirect accesses in such
> a
> case to be able to support those PHYs.
Do you actually have a requirement for this?
> There is a similar issue in phy_mii_ioctl(). Let's assume a c45 capable PHY
> is
> connected to a MDIO bus controller that is not capable of c45 framing. We
> can
> also assume that it was probed with c22 bus probing, since without this
> patch
> nothing else is possible.
> Now, there might be an ioctl() asking for a c45 transfer by specifying
> MDIO_PHY_ID_C45_MASK e.g. in order to access a different MMD's register,
> since the PHY is actually capable of c45. Currently, this would result in
>
> devad = mdiobus_c45_addr(devad, mii_data->reg_num);
> mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad, devad);
>
> calls, which would fail, since the bus doesn't support it. Instead falling
> back
> to indirect access might be the better option. Surely, the userspace program
> could implement the indirect access as well, but I think this way it's just
> more
> convenient, e.g. "phytool read iface/addr:devad/reg".
One could also argue this is a feature, and it allows userspace to
know whether C45 cycles are supported or not.
In summary, I think you need to show us that you have a real world
use case for these changes - in other words, a real world PHY setup
that doesn't work today.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-01 8:48 ` Russell King - ARM Linux admin
@ 2021-04-02 1:10 ` Danilo Krummrich
2021-04-02 12:28 ` Andrew Lunn
2021-04-02 12:58 ` Russell King - ARM Linux admin
0 siblings, 2 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-02 1:10 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Apr 01, 2021 at 03:23:05AM +0200, danilokrummrich@dk-develop.de wrote:
> > On 2021-03-31 20:35, Russell King - ARM Linux admin wrote:
> > > On Wed, Mar 31, 2021 at 07:58:33PM +0200, danilokrummrich@dk-develop.de
> > > wrote:
> > > > For this cited change the only thing happening is that if
> > > > get_phy_device()
> > > > already failed for probing with is_c45==false (C22 devices) it tries
> > > > to
> > > > probe with is_c45==true (C45 devices) which then either results into
> > > > actual
> > > > C45 frame transfers or indirect accesses by calling mdiobus_c45_*()
> > > > functions.
> > >
> > > Please explain why and how a PHY may not appear to be present using
> > > C22 frames to read the ID registers, but does appear to be present
> > > when using C22 frames to the C45 indirect registers - and summarise
> > > which PHYs have this behaviour.
> > >
> > > It seems very odd that any PHY would only implement C45 indirect
> > > registers in the C22 register space.
> >
> > Honestly, I can't list examples of that case (at least none that have an
> > upstream driver already). This part of my patch, to fall back to c45 bus
> > probing when c22 probing does not succeed, is also motivated by the fact
> > that this behaviour was already introduced with this patch:
>
> So, if I understand what you've just said, you want to make a change to
> the kernel to add support for something that you don't need and don't
> know that there's any hardware that needs it. Is that correct?
>
No, not at all. As I explained this part of the patch in mdiobus_scan() I did
based on the patch of Jeremy only. It was an indicator for me that there might
be c45 PHYs that don't respond to c22 requests. I interpreted his commit
message in a way that those c45 PHYs are capable of processing c22 requests in
general but implement the indirect registers only, since he said "its possible
that a c45 device doesn't respond despite being a standard phy".
You said that this behaviour would be very odd and I agree. Now, likely I just
misinterpreted this and Jeremy actually tells that the PHYs he's referring to
don't support c22 access at all. In this case we can for sure just forget
about the changes in mdiobus_scan() of this patch. I'll remove them.
Again, just to sort this out, this part of the patch is not it's main purpose.
However, since I implemented the fallback to indirect accesses anyways I
thought it doesn't hurt to consider the case that a PHY implements indirect
access registers only. Anyways, I admit that this is likely pointless and as
said, I'll remove it from the patch.
> > commit 0cc8fecf041d3e5285380da62cc6662bdc942d8c
> > Author: Jeremy Linton <jeremy.linton@arm.com>
> > Date: Mon Jun 22 20:35:32 2020 +0530
> >
> > net: phy: Allow mdio buses to auto-probe c45 devices
> >
> > The mdiobus_scan logic is currently hardcoded to only
> > work with c22 devices. This works fairly well in most
> > cases, but its possible that a c45 device doesn't respond
> > despite being a standard phy. If the parent hardware
> > is capable, it makes sense to scan for c22 devices before
> > falling back to c45.
> >
> > As we want this to reflect the capabilities of the STA,
> > lets add a field to the mii_bus structure to represent
> > the capability. That way devices can opt into the extended
> > scanning. Existing users should continue to default to c22
> > only scanning as long as they are zero'ing the structure
> > before use.
> >
> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > In this patch i.a. the following lines were added.
> >
> > + case MDIOBUS_C22_C45:
> > + phydev = get_phy_device(bus, addr, false);
> > + if (IS_ERR(phydev))
> > + phydev = get_phy_device(bus, addr, true);
> > + break;
> >
> > I'm applying the same logic for MDIOBUS_NO_CAP and MDIOBUS_C22, since
> > with my patch MDIO controllers with those capabilities can handle c45 bus
> > probing with indirect accesses.
>
> If the PHY doesn't respond to C22 accesses but is a C45 PHY, then how
> can this work (you seem to have essentially said it doesn't above.)
>
As stated above likely I wrongly interpreted his commit message as if only the
indirect registers are implemented.
> > [By the way, I'm unsure if this order for MDIO bus controllers with the
> > capability MDIOBUS_C22_C45 makes sense, because if we assume that the
> > majority of c45 PHYs responds well to c22 probing (which I'm convinced of)
>
> There are some which don't - Clause 45 allows PHYs not to implement
> support for Clause 22 accesses.
>
Yes, I agree.
> > the PHY would still be registered as is_c45==false, which results in the
> > fact
> > that even though the underlying bus is capable of real c45 framing only
> > indirect accessing would be performed. But this is another topic and
> > unrelated to the patch.]
>
> We make no distinction between IDs found via Clause 22 probing and IDs
> found via Clause 45 probing; the PHY driver will match either way. We
> don't know of any cases where the IDs are different between these two.
> So we still end up with the same driver being matched.
>
True, I don't say that it doesn't work or the driver is not matched. I just
say that if the c45 capable PHY is already matched with c22 probing (which is
performed first) the struct phy_device is created with is_c45=false. Hence,
phy_*_mmd() functions will perform an indirect access, although a direct
c45 access would be possible, which is less efficient. Admittedly, this is
pretty minor.
> Also note that there are MDIO controllers that support clause 22 and
> clause 45, but which require clause 22 to be probed first. Not
> everything is MDIO at the bus level anymore - there's PHYs that support
> I2C, and the I2C format of a clause 45 read is identical to a clause 22
> write.
>
> > In the case a PHY is registered as a c45 compatible PHY in the device tree,
> > it is probed in c45 mode and therefore finally mdiobus_c45_read() is
> > called,
> > which as by now just expects the underlying MDIO bus controller to be
> > capable
> > to do c45 framing and therefore the operation would fail in case it is not.
> > Hence, in my opinion it is useful to fall back to indirect accesses in such
> > a
> > case to be able to support those PHYs.
>
> Do you actually have a requirement for this?
>
Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that it
should be possible to just register it with the compatible string
"ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this
should result in probing it as c22 PHY and doing indirect accesses through
phy_*_mmd().
> > There is a similar issue in phy_mii_ioctl(). Let's assume a c45 capable PHY
> > is
> > connected to a MDIO bus controller that is not capable of c45 framing. We
> > can
> > also assume that it was probed with c22 bus probing, since without this
> > patch
> > nothing else is possible.
> > Now, there might be an ioctl() asking for a c45 transfer by specifying
> > MDIO_PHY_ID_C45_MASK e.g. in order to access a different MMD's register,
> > since the PHY is actually capable of c45. Currently, this would result in
> >
> > devad = mdiobus_c45_addr(devad, mii_data->reg_num);
> > mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad, devad);
> >
> > calls, which would fail, since the bus doesn't support it. Instead falling
> > back
> > to indirect access might be the better option. Surely, the userspace program
> > could implement the indirect access as well, but I think this way it's just
> > more
> > convenient, e.g. "phytool read iface/addr:devad/reg".
>
> One could also argue this is a feature, and it allows userspace to
> know whether C45 cycles are supported or not.
>
No, if the userspace requests such a transfer although the MDIO controller
does not support real c45 framing the kernel will call mdiobus_c45_addr() to
join the devaddr and and regaddr in one parameter and pass it to
mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing
will not care and just mask/shift the joined value and write it to the
particular register. Obviously, this will result into complete garbage being
read or (even worse) written.
Now, to fix this up we can either check for the bus capabilities in
phy_mii_ioctl(), or even better in the mdiobus_c45_*() functions, and return
a proper error code in case the bus is not capable to do real c45 framing or
we can fall back to indirect accesses, as my patch does. I'd still prefer to
fall back to an indirect access in this case.
What do you prefer?
> In summary, I think you need to show us that you have a real world
> use case for these changes - in other words, a real world PHY setup
> that doesn't work today.
>
My concrete setup would have been the PHY I mentioned above, but if I'm right
with my assumption that I can just use "ethernet-phy-ieee802.3-c22" for it,
I don't have a concreate setup anymore.
However, the kernel provides the option to register arbitrary MDIO drivers with
mdio_driver_register() where a device could support c45 and live on a c22 only
bus. For such devices the fallback to indirect accesses would be useful as well,
since they can't use the existing indirect access functions, because they're
specific to PHYs. However, currently there aren't such drivers in the kernel.
I just want to mention this for completeness.
> Thanks.
>
Thanks to you, Russell!
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-02 1:10 ` Danilo Krummrich
@ 2021-04-02 12:28 ` Andrew Lunn
2021-04-04 18:25 ` Danilo Krummrich
2021-04-02 12:58 ` Russell King - ARM Linux admin
1 sibling, 1 reply; 22+ messages in thread
From: Andrew Lunn @ 2021-04-02 12:28 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
> > Do you actually have a requirement for this?
> >
> Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that it
> should be possible to just register it with the compatible string
> "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this
> should result in probing it as c22 PHY and doing indirect accesses through
> phy_*_mmd().
Hi Danilo
Do you plan to submit a driver for this?
Does this device have an ID in register 2 and 3 in C22 space?
Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-02 1:10 ` Danilo Krummrich
2021-04-02 12:28 ` Andrew Lunn
@ 2021-04-02 12:58 ` Russell King - ARM Linux admin
2021-04-04 19:23 ` Danilo Krummrich
1 sibling, 1 reply; 22+ messages in thread
From: Russell King - ARM Linux admin @ 2021-04-02 12:58 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote:
> On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote:
> > Do you actually have a requirement for this?
> >
> Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that it
> should be possible to just register it with the compatible string
> "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this
> should result in probing it as c22 PHY and doing indirect accesses through
> phy_*_mmd().
Unfortunately, I let my Marvell Extranet access expire last year, so
I can't grab the datasheet for the 88Q2112 to see what Marvell claim
it supports.
> > One could also argue this is a feature, and it allows userspace to
> > know whether C45 cycles are supported or not.
> >
> No, if the userspace requests such a transfer although the MDIO controller
> does not support real c45 framing the kernel will call mdiobus_c45_addr() to
> join the devaddr and and regaddr in one parameter and pass it to
> mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing
> will not care and just mask/shift the joined value and write it to the
> particular register. Obviously, this will result into complete garbage being
> read or (even worse) written.
We have established that MDIO drivers need to reject accesses for
reads/writes that they do not support - this isn't something that
they have historically checked for because it is only recent that
phylib has really started to support clause 45 PHYs.
More modern MDIO drivers check the requested access type and error
out - we need the older MDIO drivers to do the same.
> > In summary, I think you need to show us that you have a real world
> > use case for these changes - in other words, a real world PHY setup
> > that doesn't work today.
> >
> My concrete setup would have been the PHY I mentioned above, but if I'm right
> with my assumption that I can just use "ethernet-phy-ieee802.3-c22" for it,
> I don't have a concreate setup anymore.
>
> However, the kernel provides the option to register arbitrary MDIO drivers with
> mdio_driver_register() where a device could support c45 and live on a c22 only
> bus. For such devices the fallback to indirect accesses would be useful as well,
> since they can't use the existing indirect access functions, because they're
> specific to PHYs. However, currently there aren't such drivers in the kernel.
> I just want to mention this for completeness.
Right, and in such a scenario, I think using the PHY compatible
property that allows you to specify the IDs will do exactly what you
need. Yes, is_c45 will be false, which is exactly what you want in this
case.
The PHY specific C45 driver will still recognise the PHY ID, and bind
to the PHY device. It will then issue the MMD accesses, which will go
through the indirect registers.
So, I don't immediately see any need to change the kernel code for
this. However, you say you don't have this setup anymore, so there's
no way to test this.
I suggest we wait until we do have such a setup where it can be tested
and proven to work, rather than changing the kernel code to try adding
something that we've no way to test - but at the same time where making
those changes has its own risk of causing regressions. Without it being
tested, the cost/benefit ratio is weighed to the change being costly.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-02 12:28 ` Andrew Lunn
@ 2021-04-04 18:25 ` Danilo Krummrich
2022-02-08 16:30 ` Geert Uytterhoeven
0 siblings, 1 reply; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-04 18:25 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
On Fri, Apr 02, 2021 at 02:28:54PM +0200, Andrew Lunn wrote:
> > > Do you actually have a requirement for this?
> > >
> > Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that it
> > should be possible to just register it with the compatible string
> > "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this
> > should result in probing it as c22 PHY and doing indirect accesses through
> > phy_*_mmd().
>
> Hi Danilo
>
> Do you plan to submit a driver for this?
>
Hi Andrew,
Yes, I'll get it ready once I got some spare time.
> Does this device have an ID in register 2 and 3 in C22 space?
>
Currently, I don't have access to the datasheet and I don't remember.
In a couple of days I should have access to the HW again and I will try.
> Andrew
Cheers,
Danilo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-02 12:58 ` Russell King - ARM Linux admin
@ 2021-04-04 19:23 ` Danilo Krummrich
2021-04-05 13:33 ` Andrew Lunn
0 siblings, 1 reply; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-04 19:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote:
> > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote:
> > > One could also argue this is a feature, and it allows userspace to
> > > know whether C45 cycles are supported or not.
> > >
> > No, if the userspace requests such a transfer although the MDIO controller
> > does not support real c45 framing the kernel will call mdiobus_c45_addr() to
> > join the devaddr and and regaddr in one parameter and pass it to
> > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing
> > will not care and just mask/shift the joined value and write it to the
> > particular register. Obviously, this will result into complete garbage being
> > read or (even worse) written.
>
>
> We have established that MDIO drivers need to reject accesses for
> reads/writes that they do not support - this isn't something that
> they have historically checked for because it is only recent that
> phylib has really started to support clause 45 PHYs.
>
I see, that's why you consider it a feature - because it is.
What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to
MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask
for an indirect access in order to save userspace doing the indirect access
itself. A nice side effect would be saving 3 syscalls per request.
> More modern MDIO drivers check the requested access type and error
> out - we need the older MDIO drivers to do the same.
>
So currently every driver should check for the flag MII_ADDR_C45 and report an
error in case it's unsupported.
What do you think about checking the bus' capabilities instead in
mdiobus_c45_*()? This way the check if C45 is supported can even happen before
calling the driver at all. I think that would be a little cleaner than having
two places where information of the bus' capabilities are stored (return value
of read/write functions and the capabilities field).
I think there are not too many drivers setting their capabilities though, but
it should be easy to derive this information from how and if they handle the
MII_ADDR_C45 flag.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-04 19:23 ` Danilo Krummrich
@ 2021-04-05 13:33 ` Andrew Lunn
2021-04-05 18:58 ` Danilo Krummrich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Lunn @ 2021-04-05 13:33 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote:
> On Fri, Apr 02, 2021 at 01:58:58PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Apr 02, 2021 at 03:10:49AM +0200, Danilo Krummrich wrote:
> > > On Thu, Apr 01, 2021 at 09:48:58AM +0100, Russell King - ARM Linux admin wrote:
> > > > One could also argue this is a feature, and it allows userspace to
> > > > know whether C45 cycles are supported or not.
> > > >
> > > No, if the userspace requests such a transfer although the MDIO controller
> > > does not support real c45 framing the kernel will call mdiobus_c45_addr() to
> > > join the devaddr and and regaddr in one parameter and pass it to
> > > mdiobus_read() or mdiobus_write(). A bus driver not supporting c45 framing
> > > will not care and just mask/shift the joined value and write it to the
> > > particular register. Obviously, this will result into complete garbage being
> > > read or (even worse) written.
> >
> >
> > We have established that MDIO drivers need to reject accesses for
> > reads/writes that they do not support - this isn't something that
> > they have historically checked for because it is only recent that
> > phylib has really started to support clause 45 PHYs.
> >
> I see, that's why you consider it a feature - because it is.
> What do you think about adding a flag MDIO_PHY_ID_MMD (or similar) analog to
> MDIO_PHY_ID_C45 for phy_mii_ioctl() to check for, such that userspace can ask
> for an indirect access in order to save userspace doing the indirect access
> itself. A nice side effect would be saving 3 syscalls per request.
We don't care about the performance of this IOCTL interface. It is for
debug only, and you need to be very careful how you use it, because
you can very easily shoot yourself in the foot.
> So currently every driver should check for the flag MII_ADDR_C45 and report an
> error in case it's unsupported.
>
> What do you think about checking the bus' capabilities instead in
> mdiobus_c45_*()? This way the check if C45 is supported can even happen before
> calling the driver at all. I think that would be a little cleaner than having
> two places where information of the bus' capabilities are stored (return value
> of read/write functions and the capabilities field).
>
> I think there are not too many drivers setting their capabilities though, but
> it should be easy to derive this information from how and if they handle the
> MII_ADDR_C45 flag.
I actually don't think anything needs to change. The Marvell PHY
probably probes due to its C22 IDs. The driver will then requests C45
access which automagically get converted into C45 over C22 for your
hardware, but remain C45 access for bus drivers which support C45.
Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 13:33 ` Andrew Lunn
@ 2021-04-05 18:58 ` Danilo Krummrich
2021-04-05 19:27 ` Andrew Lunn
2021-04-05 21:12 ` Russell King - ARM Linux admin
0 siblings, 2 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-05 18:58 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote:
> On Sun, Apr 04, 2021 at 09:23:55PM +0200, Danilo Krummrich wrote:
> > So currently every driver should check for the flag MII_ADDR_C45 and report an
> > error in case it's unsupported.
> >
> > What do you think about checking the bus' capabilities instead in
> > mdiobus_c45_*()? This way the check if C45 is supported can even happen before
> > calling the driver at all. I think that would be a little cleaner than having
> > two places where information of the bus' capabilities are stored (return value
> > of read/write functions and the capabilities field).
> >
> > I think there are not too many drivers setting their capabilities though, but
> > it should be easy to derive this information from how and if they handle the
> > MII_ADDR_C45 flag.
>
> I actually don't think anything needs to change. The Marvell PHY
> probably probes due to its C22 IDs. The driver will then requests C45
> access which automagically get converted into C45 over C22 for your
> hardware, but remain C45 access for bus drivers which support C45.
>
Thanks Andrew - I agree, for the Marvell PHY to work I likly don't need any
change, since I also expect that it will probe with the C22 IDs. I'll try
this soon.
However, this was about something else - Russell wrote:
> > > We have established that MDIO drivers need to reject accesses for
> > > reads/writes that they do not support [..]
The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus
request. In case they don't support it they return -EOPNOTSUPP. So basically,
the bus drivers read/write functions (should) encode the capability of doing
C45 transfers.
I just noted that this is redundant to the bus' capabilities field of
struct mii_bus which also encodes the bus' capabilities of doing C22 and/or C45
transfers.
Now, instead of encoding this information of the bus' capabilities at both
places, I'd propose just checking the mii_bus->capabilities field in the
mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
places where this information is stored. What do you think about that?
> Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 18:58 ` Danilo Krummrich
@ 2021-04-05 19:27 ` Andrew Lunn
2021-04-05 22:30 ` Danilo Krummrich
2021-04-05 21:12 ` Russell King - ARM Linux admin
1 sibling, 1 reply; 22+ messages in thread
From: Andrew Lunn @ 2021-04-05 19:27 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
> Now, instead of encoding this information of the bus' capabilities at both
> places, I'd propose just checking the mii_bus->capabilities field in the
> mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
> places where this information is stored. What do you think about that?
You will need to review all the MDIO bus drivers to make sure they
correctly set the capabilities. There is something like 55 using
of_mdiobus_register() and 45 using mdiobus_register(). So you have 100
drivers to review.
Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 18:58 ` Danilo Krummrich
2021-04-05 19:27 ` Andrew Lunn
@ 2021-04-05 21:12 ` Russell King - ARM Linux admin
2021-04-07 13:26 ` Danilo Krummrich
1 sibling, 1 reply; 22+ messages in thread
From: Russell King - ARM Linux admin @ 2021-04-05 21:12 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
On Mon, Apr 05, 2021 at 08:58:07PM +0200, Danilo Krummrich wrote:
> On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote:
> However, this was about something else - Russell wrote:
> > > > We have established that MDIO drivers need to reject accesses for
> > > > reads/writes that they do not support [..]
> The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus
> request. In case they don't support it they return -EOPNOTSUPP. So basically,
> the bus drivers read/write functions (should) encode the capability of doing
> C45 transfers.
>
> I just noted that this is redundant to the bus' capabilities field of
> struct mii_bus which also encodes the bus' capabilities of doing C22 and/or C45
> transfers.
>
> Now, instead of encoding this information of the bus' capabilities at both
> places, I'd propose just checking the mii_bus->capabilities field in the
> mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
> places where this information is stored. What do you think about that?
It would be good to clean that up, but that's a lot of work given the
number of drivers - not only in terms of making the changes but also
making sure that the changes are as correct as would be sensibly
achievable... then there's the problem of causing regressions by doing
so.
The two ways were introduced at different times to do two different
things: the checking in the read/write methods in each driver was the
first method, which was being added to newer drivers. Then more
recently came the ->capabilities field.
So now we have some drivers that:
- do no checks and don't fill in ->capabilities either (some of which
are likely C22-only.)
- check in their read/write methods for access types they don't support
(e.g. drivers/net/ethernet/marvell/mvmdio.c) and don't fill in
->capabilities. Note, mvmdio supports both C22-only and C45-only
interfaces with no C22-and-C45 interfaces.
- do fill in ->capabilities with MDIOBUS_C22_C45 (and hence have no
checks in their read/write functions).
Sometimes, its best to leave stuff alone... if it ain't broke, don't
make regressions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 19:27 ` Andrew Lunn
@ 2021-04-05 22:30 ` Danilo Krummrich
2021-04-06 7:21 ` Danilo Krummrich
0 siblings, 1 reply; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-05 22:30 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
On Mon, Apr 05, 2021 at 09:27:44PM +0200, Andrew Lunn wrote:
> > Now, instead of encoding this information of the bus' capabilities at both
> > places, I'd propose just checking the mii_bus->capabilities field in the
> > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
> > places where this information is stored. What do you think about that?
>
> You will need to review all the MDIO bus drivers to make sure they
> correctly set the capabilities. There is something like 55 using
> of_mdiobus_register() and 45 using mdiobus_register(). So you have 100
> drivers to review.
Yes, but I think it would be enough to look at the drivers handling the
MII_ADDR_C45 flag, because those are either
- actually capable to do C45 bus transfers or
- do properly return -EOPNOTSUPP.
I counted 27 drivers handling the MII_ADDR_C45 flag. Setting the capabilities
for those should be pretty easy.
The remaining ones, which should be about 73 then, could be left untouched,
because the default capability MDIOBUS_NO_CAP would indicate they can C22 only.
Since they don't handle the MII_ADDR_C45 flag at all, this should be the
correct assumption.
>
> Andrew
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 22:30 ` Danilo Krummrich
@ 2021-04-06 7:21 ` Danilo Krummrich
0 siblings, 0 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-06 7:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, davem, hkallweit1, netdev,
linux-kernel, jeremy.linton
On Tue, Apr 06, 2021 at 12:30:11AM +0200, Danilo Krummrich wrote:
> On Mon, Apr 05, 2021 at 09:27:44PM +0200, Andrew Lunn wrote:
> > > Now, instead of encoding this information of the bus' capabilities at both
> > > places, I'd propose just checking the mii_bus->capabilities field in the
> > > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
> > > places where this information is stored. What do you think about that?
> >
> > You will need to review all the MDIO bus drivers to make sure they
> > correctly set the capabilities. There is something like 55 using
> > of_mdiobus_register() and 45 using mdiobus_register(). So you have 100
> > drivers to review.
> Yes, but I think it would be enough to look at the drivers handling the
> MII_ADDR_C45 flag, because those are either
> - actually capable to do C45 bus transfers or
> - do properly return -EOPNOTSUPP.
>
> I counted 27 drivers handling the MII_ADDR_C45 flag. Setting the capabilities
> for those should be pretty easy.
>
> The remaining ones, which should be about 73 then, could be left untouched,
> because the default capability MDIOBUS_NO_CAP would indicate they can C22 only.
> Since they don't handle the MII_ADDR_C45 flag at all, this should be the
> correct assumption.
Forgot to mention, this would also automatically fixup that userspace C45
requests for those "remaining" drivers results in garbage on the bus. :-)
> >
> > Andrew
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-05 21:12 ` Russell King - ARM Linux admin
@ 2021-04-07 13:26 ` Danilo Krummrich
0 siblings, 0 replies; 22+ messages in thread
From: Danilo Krummrich @ 2021-04-07 13:26 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, davem, hkallweit1, netdev, linux-kernel,
jeremy.linton
I somehow missed this mail..
On Mon, Apr 05, 2021 at 10:12:36PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Apr 05, 2021 at 08:58:07PM +0200, Danilo Krummrich wrote:
> > On Mon, Apr 05, 2021 at 03:33:55PM +0200, Andrew Lunn wrote:
> > However, this was about something else - Russell wrote:
> > > > > We have established that MDIO drivers need to reject accesses for
> > > > > reads/writes that they do not support [..]
> > The MDIO drivers do this by checking the MII_ADDR_C45 flag if it's a C45 bus
> > request. In case they don't support it they return -EOPNOTSUPP. So basically,
> > the bus drivers read/write functions (should) encode the capability of doing
> > C45 transfers.
> >
> > I just noted that this is redundant to the bus' capabilities field of
> > struct mii_bus which also encodes the bus' capabilities of doing C22 and/or C45
> > transfers.
> >
> > Now, instead of encoding this information of the bus' capabilities at both
> > places, I'd propose just checking the mii_bus->capabilities field in the
> > mdiobus_c45_*() functions. IMHO this would be a little cleaner, than having two
> > places where this information is stored. What do you think about that?
>
> It would be good to clean that up, but that's a lot of work given the
> number of drivers - not only in terms of making the changes but also
> making sure that the changes are as correct as would be sensibly
> achievable... then there's the problem of causing regressions by doing
> so.
>
As mentioned in the answer to Andrew, I think it wouldn't be too many as we'd
only need to look for the ones considering MII_ADDR_C45 at all.
> The two ways were introduced at different times to do two different
> things: the checking in the read/write methods in each driver was the
> first method, which was being added to newer drivers. Then more
> recently came the ->capabilities field.
>
> So now we have some drivers that:
> - do no checks and don't fill in ->capabilities either (some of which
> are likely C22-only.)
Guess they could be left untouched completely and simply go for MDIOBUS_NO_CAP
implicitly and therefore C45 requests would be correctly rejected by
mdiobus_c45_*() functions. I think this would be the main advantage apart from
making it more clean.
> - check in their read/write methods for access types they don't support
> (e.g. drivers/net/ethernet/marvell/mvmdio.c) and don't fill in
> ->capabilities. Note, mvmdio supports both C22-only and C45-only
> interfaces with no C22-and-C45 interfaces.
Yes, I think the correct capability could still be easily assigned per instance
within the probe() function:
switch (type) {
case BUS_TYPE_SMI:
bus->capabilities = MDIOBUS_C22;
[...]
break;
case BUS_TYPE_XSMI:
bus->capabilities = MDIOBUS_C45;
[...]
break;
}
> - do fill in ->capabilities with MDIOBUS_C22_C45 (and hence have no
> checks in their read/write functions).
>
Nothing to be done for them.
> Sometimes, its best to leave stuff alone... if it ain't broke, don't
> make regressions.
>
Yes, I can perfectly understand that.
However, maybe I go for a patch anyways, and if so I will also send it. If it's
taken by you in the end is on another piece of paper.
Thanks again!
Danilo
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2021-04-04 18:25 ` Danilo Krummrich
@ 2022-02-08 16:30 ` Geert Uytterhoeven
2022-02-08 22:52 ` Danilo Krummrich
0 siblings, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2022-02-08 16:30 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Andrew Lunn, Russell King - ARM Linux admin, David S. Miller,
Heiner Kallweit, netdev, Linux Kernel Mailing List, Jeremy Linton
Hi Danilo,
On Sun, Apr 4, 2021 at 8:26 PM Danilo Krummrich
<danilokrummrich@dk-develop.de> wrote:
> On Fri, Apr 02, 2021 at 02:28:54PM +0200, Andrew Lunn wrote:
> > > > Do you actually have a requirement for this?
> > > >
> > > Yes, the Marvell 88Q2112 1000Base-T1 PHY. But actually, I just recognize that it
> > > should be possible to just register it with the compatible string
> > > "ethernet-phy-ieee802.3-c22" instead of "ethernet-phy-ieee802.3-c45", this
> > > should result in probing it as c22 PHY and doing indirect accesses through
> > > phy_*_mmd().
> >
> > Hi Danilo
> >
> > Do you plan to submit a driver for this?
>
> Yes, I'll get it ready once I got some spare time.
Did this ever happen? I cannot find such a patch.
> > Does this device have an ID in register 2 and 3 in C22 space?
> >
> Currently, I don't have access to the datasheet and I don't remember.
> In a couple of days I should have access to the HW again and I will try.
I'm asking because I have access to a board with Marvell 88Q2110 PHYs
(ID 0x002b0983).
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses
2022-02-08 16:30 ` Geert Uytterhoeven
@ 2022-02-08 22:52 ` Danilo Krummrich
0 siblings, 0 replies; 22+ messages in thread
From: Danilo Krummrich @ 2022-02-08 22:52 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Russell King - ARM Linux admin, David S. Miller,
Heiner Kallweit, netdev, Linux Kernel Mailing List, Jeremy Linton
Hi Geert,
On Tue, Feb 08, 2022 at 05:30:01PM +0100, Geert Uytterhoeven wrote:
> On Sun, Apr 4, 2021 at 8:26 PM Danilo Krummrich
> <danilokrummrich@dk-develop.de> wrote:
> >
> > Yes, I'll get it ready once I got some spare time.
>
> Did this ever happen? I cannot find such a patch.
>
> I'm asking because I have access to a board with Marvell 88Q2110 PHYs
> (ID 0x002b0983).
No, it happened that I neither got access to the datasheet nor to a board I
could test on again.
I still have some reverse engineered protocol data and I/O traces from a
proprietary driver, which I can provide to you.
However, without a datasheet this might not lead to a proper driver, whereas
when having a proper datasheet this data might be needless.
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
- Danilo
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-02-08 23:02 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-31 14:17 net: mdio: support c45 peripherals on c22 only capable mdio controllers Danilo Krummrich
2021-03-31 14:17 ` [PATCH 1/2] net: mdio: rename mii bus probe_capabilities Danilo Krummrich
2021-03-31 14:17 ` [PATCH 2/2] net: mdio: support c45 peripherals on c22 busses Danilo Krummrich
2021-03-31 16:27 ` Andrew Lunn
2021-03-31 17:58 ` danilokrummrich
2021-03-31 18:35 ` Russell King - ARM Linux admin
2021-04-01 1:23 ` danilokrummrich
2021-04-01 8:48 ` Russell King - ARM Linux admin
2021-04-02 1:10 ` Danilo Krummrich
2021-04-02 12:28 ` Andrew Lunn
2021-04-04 18:25 ` Danilo Krummrich
2022-02-08 16:30 ` Geert Uytterhoeven
2022-02-08 22:52 ` Danilo Krummrich
2021-04-02 12:58 ` Russell King - ARM Linux admin
2021-04-04 19:23 ` Danilo Krummrich
2021-04-05 13:33 ` Andrew Lunn
2021-04-05 18:58 ` Danilo Krummrich
2021-04-05 19:27 ` Andrew Lunn
2021-04-05 22:30 ` Danilo Krummrich
2021-04-06 7:21 ` Danilo Krummrich
2021-04-05 21:12 ` Russell King - ARM Linux admin
2021-04-07 13:26 ` Danilo Krummrich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).