| Message ID | 20200710120851.28984-6-o.rempel@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | add cable test support for ksz8081 and ksz8873 | expand |
On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote: > This patch support for cable test for the ksz886x switches and the > ksz8081 PHY. > > The patch was tested on a KSZ8873RLL switch with following results: > > - port 1: > - cannot detect any distance > - provides inverted values > (Errata: DS80000830A: "LinkMD does not work on Port 1", > http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf) > - Reports "short" on open or ok. > - Reports "ok" on short. > > - port 2: > - can detect distance > - can detect open on each wire of pair A (wire 1 and 2) > - can detect open only on one wire of pair B (only wire 3) > - can detect short between wires of a pair (wires 1 + 2 or 3 + 6) > - short between pairs is detected as open. > For example short between wires 2 + 3 is detected as open. > > In order to work around the errata for port 1, the ksz8795 switch driver > should be extended to provide proper device tree support for the related > PHY nodes. So we can set a DT property to mark the port 1 as affected by > the errata. Hi Oleksij Do the PHY register read/writes pass through the DSA driver for the 8873? I was wondering if the switch could intercept reads/writes on port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more robust solution than DT properties, which are going to get forgotten. Andrew
On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote: > This patch support for cable test for the ksz886x switches and the > ksz8081 PHY. > > The patch was tested on a KSZ8873RLL switch with following results: > > - port 1: > - cannot detect any distance > - provides inverted values > (Errata: DS80000830A: "LinkMD does not work on Port 1", > http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf) > - Reports "short" on open or ok. > - Reports "ok" on short. > > - port 2: > - can detect distance > - can detect open on each wire of pair A (wire 1 and 2) > - can detect open only on one wire of pair B (only wire 3) > - can detect short between wires of a pair (wires 1 + 2 or 3 + 6) > - short between pairs is detected as open. > For example short between wires 2 + 3 is detected as open. > > In order to work around the errata for port 1, the ksz8795 switch driver > should be extended to provide proper device tree support for the related > PHY nodes. So we can set a DT property to mark the port 1 as affected by > the errata. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sat, Jul 11, 2020 at 08:29:12PM +0200, Andrew Lunn wrote: > On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote: > > This patch support for cable test for the ksz886x switches and the > > ksz8081 PHY. > > > > The patch was tested on a KSZ8873RLL switch with following results: > > > > - port 1: > > - cannot detect any distance > > - provides inverted values > > (Errata: DS80000830A: "LinkMD does not work on Port 1", > > http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf) > > - Reports "short" on open or ok. > > - Reports "ok" on short. > > > > - port 2: > > - can detect distance > > - can detect open on each wire of pair A (wire 1 and 2) > > - can detect open only on one wire of pair B (only wire 3) > > - can detect short between wires of a pair (wires 1 + 2 or 3 + 6) > > - short between pairs is detected as open. > > For example short between wires 2 + 3 is detected as open. > > > > In order to work around the errata for port 1, the ksz8795 switch driver > > should be extended to provide proper device tree support for the related > > PHY nodes. So we can set a DT property to mark the port 1 as affected by > > the errata. Hi Andrew, > Hi Oleksij > > Do the PHY register read/writes pass through the DSA driver for the > 8873? I was wondering if the switch could intercept reads/writes on > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more > robust solution than DT properties, which are going to get forgotten. Yes, it was my first idea as well. But this switch allows direct MDIO access to the PHYs and this PHY driver could be used without DSA driver. Not sure if we should support both variants? Beside, the Port 1 need at least one more quirk. The pause souport is announced but is not working. Should we some how clear Puase bit in the PHY and tell PHY framework to not use it? What is the best way to do it? Regards, Oleksij
On Mon, Jul 13, 2020 at 06:11:30AM +0200, Oleksij Rempel wrote: > On Sat, Jul 11, 2020 at 08:29:12PM +0200, Andrew Lunn wrote: > > On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote: > > > This patch support for cable test for the ksz886x switches and the > > > ksz8081 PHY. > > > > > > The patch was tested on a KSZ8873RLL switch with following results: > > > > > > - port 1: > > > - cannot detect any distance > > > - provides inverted values > > > (Errata: DS80000830A: "LinkMD does not work on Port 1", > > > http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf) > > > - Reports "short" on open or ok. > > > - Reports "ok" on short. > > > > > > - port 2: > > > - can detect distance > > > - can detect open on each wire of pair A (wire 1 and 2) > > > - can detect open only on one wire of pair B (only wire 3) > > > - can detect short between wires of a pair (wires 1 + 2 or 3 + 6) > > > - short between pairs is detected as open. > > > For example short between wires 2 + 3 is detected as open. > > > > > > In order to work around the errata for port 1, the ksz8795 switch driver > > > should be extended to provide proper device tree support for the related > > > PHY nodes. So we can set a DT property to mark the port 1 as affected by > > > the errata. > > Hi Andrew, > > > Hi Oleksij > > > > Do the PHY register read/writes pass through the DSA driver for the > > 8873? I was wondering if the switch could intercept reads/writes on > > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more > > robust solution than DT properties, which are going to get forgotten. > > Yes, it was my first idea as well. But this switch allows direct MDIO > access to the PHYs and this PHY driver could be used without DSA driver. > Not sure if we should support both variants? > > Beside, the Port 1 need at least one more quirk. The pause souport is > announced but is not working. Should we some how clear Puase bit in the PHY and > tell PHY framework to not use it? What is the best way to do it? On other hand, if adding this quirks in to switch driver is acceptable way, i'll be happy with this as well. Regards, Oleksij
> > Hi Oleksij > > > > Do the PHY register read/writes pass through the DSA driver for the > > 8873? I was wondering if the switch could intercept reads/writes on > > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more > > robust solution than DT properties, which are going to get forgotten. > > Yes, it was my first idea as well. But this switch allows direct MDIO > access to the PHYs and this PHY driver could be used without DSA driver. > Not sure if we should support both variants? > > Beside, the Port 1 need at least one more quirk. The pause souport is > announced but is not working. Should we some how clear Puase bit in the PHY and > tell PHY framework to not use it? What is the best way to do it? It all seems rather odd, the way one PHY is messed up, but the other works. Does this PHY exist as a standalone device, not integrated into the switch? Do the same erratas apply to such a standalone device? If the issues are just limited to integrated PHYs, there is maybe something you can do via DSA: in slave.c: static int dsa_slave_phy_setup(struct net_device *slave_dev) { ... if (ds->ops->get_phy_flags) phy_flags = ds->ops->get_phy_flags(ds, dp->index); ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags); It is either B53 or SF2 which uses this, i forget which. flags get or'ed into phydev->dev_flags. These are device specific flags. So you could define a bit to represent this errata. And then in the PHY driver do whatever needs to be done when you see the flag set for a specific PHY. If Pause is broken, then yes, it would be good to remove the Pause from the available features, and return an error if requested to use it. Andrew
On Mon, Jul 13, 2020 at 05:17:19PM +0200, Andrew Lunn wrote: > > > Hi Oleksij > > > > > > Do the PHY register read/writes pass through the DSA driver for the > > > 8873? I was wondering if the switch could intercept reads/writes on > > > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more > > > robust solution than DT properties, which are going to get forgotten. > > > > Yes, it was my first idea as well. But this switch allows direct MDIO > > access to the PHYs and this PHY driver could be used without DSA driver. > > Not sure if we should support both variants? > > > > Beside, the Port 1 need at least one more quirk. The pause souport is > > announced but is not working. Should we some how clear Puase bit in the PHY and > > tell PHY framework to not use it? What is the best way to do it? > > It all seems rather odd, the way one PHY is messed up, but the other > works. Does this PHY exist as a standalone device, not integrated into > the switch? Do the same erratas apply to such a standalone device? I found multiple microchip devices with same PHYid: KSZ8463, KSZ8851 KSZ8463 - switch. Would be covered by DSA driver KSZ8851 - single MAC device with PHY. Supported by ethernet/micrel driver. This erratum is not documented for other devices. So it may exist or not. > If the issues are just limited to integrated PHYs, there is maybe > something you can do via DSA: > > in slave.c: > > static int dsa_slave_phy_setup(struct net_device *slave_dev) > { > ... > if (ds->ops->get_phy_flags) > phy_flags = ds->ops->get_phy_flags(ds, dp->index); > > ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags); > > It is either B53 or SF2 which uses this, i forget which. flags get > or'ed into phydev->dev_flags. These are device specific flags. So you > could define a bit to represent this errata. And then in the PHY > driver do whatever needs to be done when you see the flag set for a > specific PHY. > > If Pause is broken, then yes, it would be good to remove the Pause > from the available features, and return an error if requested to use > it. OK. So, i'll cover both errata with separate flags? Set flags in the DSA driver and apply workarounds in the PHY. ACK? Regards, Oleksij
> OK. So, i'll cover both errata with separate flags? Set flags in the DSA > driver and apply workarounds in the PHY. ACK? Yes. Assume the issues are limited to just the first PHY in this switch. If there are discrete PHYs with the same issue, we can come up with a different way to identify them. Andrew
On Tue, Jul 14, 2020 at 03:12:40PM +0200, Andrew Lunn wrote: > > OK. So, i'll cover both errata with separate flags? Set flags in the DSA > > driver and apply workarounds in the PHY. ACK? > > Yes. Assume the issues are limited to just the first PHY in this > switch. If there are discrete PHYs with the same issue, we can come up > with a different way to identify them. I assume, it is not the blocker for the current patch set? Regards, Oleksij
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 221ba661645f..0c86ab2e483e 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -20,6 +20,7 @@ */ #include <linux/bitfield.h> +#include <linux/ethtool_netlink.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/phy.h> @@ -62,6 +63,18 @@ #define KSZPHY_INTCS_ALL (KSZPHY_INTCS_LINK_UP |\ KSZPHY_INTCS_LINK_DOWN) +/* LinkMD Control/Status */ +#define KSZ8081_LMD 0x1d +#define KSZ8081_LMD_ENABLE_TEST BIT(15) +#define KSZ8081_LMD_STAT_NORMAL 0 +#define KSZ8081_LMD_STAT_OPEN 1 +#define KSZ8081_LMD_STAT_SHORT 2 +#define KSZ8081_LMD_STAT_FAIL 3 +#define KSZ8081_LMD_STAT_MASK GENMASK(14, 13) +/* Short cable (<10 meter) has been detected by LinkMD */ +#define KSZ8081_LMD_SHORT_INDICATOR BIT(12) +#define KSZ8081_LMD_DELTA_TIME_MASK GENMASK(8, 0) + /* PHY Control 1 */ #define MII_KSZPHY_CTRL_1 0x1e #define KSZ8081_CTRL1_MDIX_STAT BIT(4) @@ -1358,6 +1371,164 @@ static int kszphy_probe(struct phy_device *phydev) return 0; } +static int ksz886x_cable_test_start(struct phy_device *phydev) +{ + /* If autoneg is enabled, we won't be able to test cross pair + * short. In this case, the PHY will "detect" a link and + * confuse the internal state machine - disable auto neg here. + * If autoneg is disabled, we should set the speed to 10mbit. + */ + return phy_clear_bits(phydev, MII_BMCR, BMCR_ANENABLE | BMCR_SPEED100); +} + +static int ksz886x_cable_test_result_trans(u16 status) +{ + switch (FIELD_GET(KSZ8081_LMD_STAT_MASK, status)) { + case KSZ8081_LMD_STAT_NORMAL: + return ETHTOOL_A_CABLE_RESULT_CODE_OK; + case KSZ8081_LMD_STAT_SHORT: + return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT; + case KSZ8081_LMD_STAT_OPEN: + return ETHTOOL_A_CABLE_RESULT_CODE_OPEN; + case KSZ8081_LMD_STAT_FAIL: + /* fall through */ + default: + return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC; + } +} + +static bool ksz886x_cable_test_failed(u16 status) +{ + return FIELD_GET(KSZ8081_LMD_STAT_MASK, status) == + KSZ8081_LMD_STAT_FAIL; +} + +static bool ksz886x_cable_test_fault_length_valid(u16 status) +{ + switch (FIELD_GET(KSZ8081_LMD_STAT_MASK, status)) { + case KSZ8081_LMD_STAT_OPEN: + /* fall through */ + case KSZ8081_LMD_STAT_SHORT: + return true; + } + return false; +} + +static int ksz886x_cable_test_fault_length(u16 status) +{ + int dt; + + /* According to the data sheet the distance to the fault is + * DELTA_TIME * 0.4 meters. + */ + dt = FIELD_GET(KSZ8081_LMD_DELTA_TIME_MASK, status); + + return (dt * 400) / 10; +} + +static int ksz886x_cable_test_wait_for_completion(struct phy_device *phydev) +{ + int val, ret; + + ret = phy_read_poll_timeout(phydev, KSZ8081_LMD, val, + !(val & KSZ8081_LMD_ENABLE_TEST), + 30000, 100000, true); + + return ret < 0 ? ret : 0; +} + +static int ksz886x_cable_test_one_pair(struct phy_device *phydev, int pair) +{ + static const int ethtool_pair[] = { + ETHTOOL_A_CABLE_PAIR_A, + ETHTOOL_A_CABLE_PAIR_B, + }; + int ret, val, mdix; + + /* There is no way to choice the pair, like we do one ksz9031. + * We can workaround this limitation by using the MDI-X functionality. + */ + if (pair == 0) + mdix = ETH_TP_MDI; + else + mdix = ETH_TP_MDI_X; + + switch (phydev->phy_id & MICREL_PHY_ID_MASK) { + case PHY_ID_KSZ8081: + ret = ksz8081_config_mdix(phydev, mdix); + break; + case PHY_ID_KSZ886X: + ret = ksz886x_config_mdix(phydev, mdix); + break; + default: + ret = -ENODEV; + } + + if (ret) + return ret; + + /* Now we are ready to fire. This command will send a 100ns pulse + * to the pair. + */ + ret = phy_write(phydev, KSZ8081_LMD, KSZ8081_LMD_ENABLE_TEST); + if (ret) + return ret; + + ret = ksz886x_cable_test_wait_for_completion(phydev); + if (ret) + return ret; + + val = phy_read(phydev, KSZ8081_LMD); + if (val < 0) + return val; + + if (ksz886x_cable_test_failed(val)) + return -EAGAIN; + + ret = ethnl_cable_test_result(phydev, ethtool_pair[pair], + ksz886x_cable_test_result_trans(val)); + if (ret) + return ret; + + if (!ksz886x_cable_test_fault_length_valid(val)) + return 0; + + return ethnl_cable_test_fault_length(phydev, ethtool_pair[pair], + ksz886x_cable_test_fault_length(val)); +} + +static int ksz886x_cable_test_get_status(struct phy_device *phydev, + bool *finished) +{ + unsigned long pair_mask = 0x3; + int retries = 20; + int pair, ret; + + *finished = false; + + /* Try harder if link partner is active */ + while (pair_mask && retries--) { + for_each_set_bit(pair, &pair_mask, 4) { + ret = ksz886x_cable_test_one_pair(phydev, pair); + if (ret == -EAGAIN) + continue; + if (ret < 0) + return ret; + clear_bit(pair, &pair_mask); + } + /* If link partner is in autonegotiation mode it will send 2ms + * of FLPs with at least 6ms of silence. + * Add 2ms sleep to have better chances to hit this silence. + */ + if (pair_mask) + msleep(2); + } + + *finished = true; + + return 0; +} + static struct phy_driver ksphy_driver[] = { { .phy_id = PHY_ID_KS8737, @@ -1477,6 +1648,8 @@ static struct phy_driver ksphy_driver[] = { .get_stats = kszphy_get_stats, .suspend = kszphy_suspend, .resume = kszphy_resume, + .cable_test_start = ksz886x_cable_test_start, + .cable_test_get_status = ksz886x_cable_test_get_status, }, { .phy_id = PHY_ID_KSZ8061, .name = "Micrel KSZ8061", @@ -1558,6 +1731,8 @@ static struct phy_driver ksphy_driver[] = { .read_status = ksz886x_read_status, .suspend = genphy_suspend, .resume = ksz886x_resume, + .cable_test_start = ksz886x_cable_test_start, + .cable_test_get_status = ksz886x_cable_test_get_status, }, { .name = "Micrel KSZ87XX Switch", /* PHY_BASIC_FEATURES */
This patch support for cable test for the ksz886x switches and the ksz8081 PHY. The patch was tested on a KSZ8873RLL switch with following results: - port 1: - cannot detect any distance - provides inverted values (Errata: DS80000830A: "LinkMD does not work on Port 1", http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf) - Reports "short" on open or ok. - Reports "ok" on short. - port 2: - can detect distance - can detect open on each wire of pair A (wire 1 and 2) - can detect open only on one wire of pair B (only wire 3) - can detect short between wires of a pair (wires 1 + 2 or 3 + 6) - short between pairs is detected as open. For example short between wires 2 + 3 is detected as open. In order to work around the errata for port 1, the ksz8795 switch driver should be extended to provide proper device tree support for the related PHY nodes. So we can set a DT property to mark the port 1 as affected by the errata. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/micrel.c | 175 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+)