| Message ID | 20240808130833.2083875-2-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/3] ethtool: Add new result codes for TDR diagnostics | expand |
On 08.08.2024 15:08:32, Oleksij Rempel wrote: > Introduce helper functions specific to Open Alliance diagnostics, > integrating them into the PHY framework. Currently, these helpers > are limited to 1000BaseT1 specific TDR functionality. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/Makefile | 2 +- > drivers/net/phy/open_alliance_helpers.c | 70 +++++++++++++++++++++++++ > include/linux/open_alliance_helpers.h | 47 +++++++++++++++++ > 3 files changed, 118 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/phy/open_alliance_helpers.c > create mode 100644 include/linux/open_alliance_helpers.h > > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 202ed7f450da6..8a46a04af01a5 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -2,7 +2,7 @@ > # Makefile for Linux PHY drivers > > libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \ > - linkmode.o > + linkmode.o open_alliance_helpers.o > mdio-bus-y += mdio_bus.o mdio_device.o > > ifdef CONFIG_MDIO_DEVICE > diff --git a/drivers/net/phy/open_alliance_helpers.c b/drivers/net/phy/open_alliance_helpers.c > new file mode 100644 > index 0000000000000..eac1004c065ae > --- /dev/null > +++ b/drivers/net/phy/open_alliance_helpers.c > @@ -0,0 +1,70 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * open_alliance_helpers.c - OPEN Alliance specific PHY diagnostic helpers > + * > + * This file contains helper functions for implementing advanced diagnostic > + * features as specified by the OPEN Alliance for automotive Ethernet PHYs. > + * These helpers include functionality for Time Delay Reflection (TDR), dynamic > + * channel quality assessment, and other PHY diagnostics. > + * > + * For more information on the specifications, refer to the OPEN Alliance > + * documentation: https://opensig.org/automotive-ethernet-specifications/ > + */ > + > +#include <linux/ethtool_netlink.h> > +#include <linux/open_alliance_helpers.h> > + > +/** > + * oa_1000bt1_get_ethtool_cable_result_code - Convert TDR status to ethtool > + * result code > + * @reg_value: Value read from the TDR register > + * > + * This function takes a register value from the HDD.TDR register and converts > + * the TDR status to the corresponding ethtool cable test result code. > + * > + * Return: The appropriate ethtool result code based on the TDR status > + */ > +int oa_1000bt1_get_ethtool_cable_result_code(u16 reg_value) > +{ > + u8 tdr_status = (reg_value & OA_1000BT1_HDD_TDR_STATUS_MASK) >> 4; > + u8 dist_val = (reg_value & OA_1000BT1_HDD_TDR_DISTANCE_MASK) >> 8; can you make use of FIELD_GET() here and in the other functions? Marc
On Thu, Aug 08, 2024 at 03:08:32PM +0200, Oleksij Rempel wrote: > Introduce helper functions specific to Open Alliance diagnostics, > integrating them into the PHY framework. Currently, these helpers > are limited to 1000BaseT1 specific TDR functionality. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/Makefile | 2 +- > drivers/net/phy/open_alliance_helpers.c | 70 +++++++++++++++++++++++++ > include/linux/open_alliance_helpers.h | 47 +++++++++++++++++ > 3 files changed, 118 insertions(+), 1 deletion(-) > create mode 100644 drivers/net/phy/open_alliance_helpers.c > create mode 100644 include/linux/open_alliance_helpers.h > > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 202ed7f450da6..8a46a04af01a5 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -2,7 +2,7 @@ > # Makefile for Linux PHY drivers > > libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \ > - linkmode.o > + linkmode.o open_alliance_helpers.o Given that most PHY drivers aren't open alliance, would it be better to have a config symbol that can be selected by PHY drivers that need it? Thanks.
On Thu, Aug 08, 2024 at 03:08:32PM +0200, Oleksij Rempel wrote: > Introduce helper functions specific to Open Alliance diagnostics, > integrating them into the PHY framework. Currently, these helpers > are limited to 1000BaseT1 specific TDR functionality. Thanks for generalising this code. > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/Makefile | 2 +- > drivers/net/phy/open_alliance_helpers.c | 70 +++++++++++++++++++++++++ > include/linux/open_alliance_helpers.h | 47 +++++++++++++++++ We don't have any PHY drivers outside of drivers/net/phy, and i don't see any reason why we should allow them anywhere else. So please consider moving the header file into that directory, rather than having it global. > ifdef CONFIG_MDIO_DEVICE > diff --git a/drivers/net/phy/open_alliance_helpers.c b/drivers/net/phy/open_alliance_helpers.c > new file mode 100644 > index 0000000000000..eac1004c065ae > --- /dev/null > +++ b/drivers/net/phy/open_alliance_helpers.c > @@ -0,0 +1,70 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * open_alliance_helpers.c - OPEN Alliance specific PHY diagnostic helpers > + * > + * This file contains helper functions for implementing advanced diagnostic > + * features as specified by the OPEN Alliance for automotive Ethernet PHYs. > + * These helpers include functionality for Time Delay Reflection (TDR), dynamic > + * channel quality assessment, and other PHY diagnostics. > + * > + * For more information on the specifications, refer to the OPEN Alliance > + * documentation: https://opensig.org/automotive-ethernet-specifications/ Please could you give a reference to the exact standard. I think this is "Advanced diagnostic features for 1000BASE-T1 automotive Ethernet PHYs TC12 - advanced PHY features" ? The standard seem open, so you could include a URL: https://opensig.org/wp-content/uploads/2024/03/Advanced_PHY_features_for_automotive_Ethernet_v2.0_fin.pdf Andrew
On Thu, Aug 08, 2024 at 03:54:06PM +0200, Andrew Lunn wrote: > Please could you give a reference to the exact standard. I think this > is "Advanced diagnostic features for 1000BASE-T1 automotive Ethernet > PHYs TC12 - advanced PHY features" ? > > The standard seem open, so you could include a URL: > > https://opensig.org/wp-content/uploads/2024/03/Advanced_PHY_features_for_automotive_Ethernet_v2.0_fin.pdf I already started to implement other diagnostic features supported by the TI DP83TG720 PHY. For example following can be implemented too: 6.3 Link quality – start-up time and link losses (LQ) 6.3.1 Link training time (LTT) 6.3.2 Local receiver time (LRT) 6.3.3 Remote receiver time (RRT) 6.3.4 Link Failures and Losses (LFL) 6.3.5 Communication ready status (COM) 6.4 Polarity Detection and Correction (POL) 6.4.1 Polarity Detection (DET) 6.4.2 Polarity Correction (COR) What is the best way to proceed with them? Export them over phy-statistics interface or extending the netlink interface? Regards, Oleksij
On Fri, Aug 09, 2024 at 07:17:50AM +0200, Oleksij Rempel wrote: > On Thu, Aug 08, 2024 at 03:54:06PM +0200, Andrew Lunn wrote: > > > Please could you give a reference to the exact standard. I think this > > is "Advanced diagnostic features for 1000BASE-T1 automotive Ethernet > > PHYs TC12 - advanced PHY features" ? > > > > The standard seem open, so you could include a URL: > > > > https://opensig.org/wp-content/uploads/2024/03/Advanced_PHY_features_for_automotive_Ethernet_v2.0_fin.pdf > > I already started to implement other diagnostic features supported by the > TI DP83TG720 PHY. For example following can be implemented too: > 6.3 Link quality – start-up time and link losses (LQ) > 6.3.1 Link training time (LTT) > 6.3.2 Local receiver time (LRT) > 6.3.3 Remote receiver time (RRT) These three are the time it takes for some action. Not really a statistic in the normal netdev sense, since it does not count up. But they are kind of statistics, so it is probably not abusing statistics too much, so maybe O.K. > 6.3.4 Link Failures and Losses (LFL) This is a count, so does fit statistics. > 6.3.5 Communication ready status (COM) Similar to the BMSR link bit. Do it add anything useful? > 6.4 Polarity Detection and Correction (POL) > 6.4.1 Polarity Detection (DET) > 6.4.2 Polarity Correction (COR) Could these be mapped to ETH_TP_MDI* ? Andrew
On Fri, Aug 09, 2024 at 04:00:53PM +0200, Andrew Lunn wrote: > On Fri, Aug 09, 2024 at 07:17:50AM +0200, Oleksij Rempel wrote: > > On Thu, Aug 08, 2024 at 03:54:06PM +0200, Andrew Lunn wrote: > > > > > Please could you give a reference to the exact standard. I think this > > > is "Advanced diagnostic features for 1000BASE-T1 automotive Ethernet > > > PHYs TC12 - advanced PHY features" ? > > > > > > The standard seem open, so you could include a URL: > > > > > > https://opensig.org/wp-content/uploads/2024/03/Advanced_PHY_features_for_automotive_Ethernet_v2.0_fin.pdf > > > > I already started to implement other diagnostic features supported by the > > TI DP83TG720 PHY. For example following can be implemented too: > > 6.3 Link quality – start-up time and link losses (LQ) > > 6.3.1 Link training time (LTT) > > 6.3.2 Local receiver time (LRT) > > 6.3.3 Remote receiver time (RRT) > > These three are the time it takes for some action. Not really a > statistic in the normal netdev sense, since it does not count up. But > they are kind of statistics, so it is probably not abusing statistics > too much, so maybe O.K. > > > 6.3.4 Link Failures and Losses (LFL) > > This is a count, so does fit statistics. > > > 6.3.5 Communication ready status (COM) > > Similar to the BMSR link bit. Do it add anything useful? Probably. I can leave it for now > > 6.4 Polarity Detection and Correction (POL) > > 6.4.1 Polarity Detection (DET) > > 6.4.2 Polarity Correction (COR) > > Could these be mapped to ETH_TP_MDI* ? Yes, but it will look confusing in the user space. To make better representation in ethtool we will probably need a new port type. For example instead of PORT_TP it will be PORT_STP (single twiste pair) or PORT_SPE (single pair ethernet). What do you think? Beside, there are some not standard specific fail indicators. It can show RGMII and SGMII specific errors. For example R/S_GMII FIFO full/empty errors. If i see it correctly, it will not drop MDI link, so I won't be able to return a link fail reason for this case. Regards, Oleksij
> > > 6.4 Polarity Detection and Correction (POL) > > > 6.4.1 Polarity Detection (DET) > > > 6.4.2 Polarity Correction (COR) > > > > Could these be mapped to ETH_TP_MDI* ? > > Yes, but it will look confusing in the user space. To make better > representation in ethtool we will probably need a new port type. For > example instead of PORT_TP it will be PORT_STP (single twiste pair) or > PORT_SPE (single pair ethernet). What do you think? Thinking about this some more.... The Marvell PHYs can indicate per pair if the polarity is wrong and has been corrected, for 10BaseT and 1000BaseT. The datasheet indicates that for 100BaseTX polarity does not matter. So i agree, MDI crossover is the wrong way to indicate this. We should add polarity indication, for up to 4 pairs. Andrew
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 202ed7f450da6..8a46a04af01a5 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -2,7 +2,7 @@ # Makefile for Linux PHY drivers libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \ - linkmode.o + linkmode.o open_alliance_helpers.o mdio-bus-y += mdio_bus.o mdio_device.o ifdef CONFIG_MDIO_DEVICE diff --git a/drivers/net/phy/open_alliance_helpers.c b/drivers/net/phy/open_alliance_helpers.c new file mode 100644 index 0000000000000..eac1004c065ae --- /dev/null +++ b/drivers/net/phy/open_alliance_helpers.c @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * open_alliance_helpers.c - OPEN Alliance specific PHY diagnostic helpers + * + * This file contains helper functions for implementing advanced diagnostic + * features as specified by the OPEN Alliance for automotive Ethernet PHYs. + * These helpers include functionality for Time Delay Reflection (TDR), dynamic + * channel quality assessment, and other PHY diagnostics. + * + * For more information on the specifications, refer to the OPEN Alliance + * documentation: https://opensig.org/automotive-ethernet-specifications/ + */ + +#include <linux/ethtool_netlink.h> +#include <linux/open_alliance_helpers.h> + +/** + * oa_1000bt1_get_ethtool_cable_result_code - Convert TDR status to ethtool + * result code + * @reg_value: Value read from the TDR register + * + * This function takes a register value from the HDD.TDR register and converts + * the TDR status to the corresponding ethtool cable test result code. + * + * Return: The appropriate ethtool result code based on the TDR status + */ +int oa_1000bt1_get_ethtool_cable_result_code(u16 reg_value) +{ + u8 tdr_status = (reg_value & OA_1000BT1_HDD_TDR_STATUS_MASK) >> 4; + u8 dist_val = (reg_value & OA_1000BT1_HDD_TDR_DISTANCE_MASK) >> 8; + + switch (tdr_status) { + case OA_1000BT1_HDD_TDR_STATUS_CABLE_OK: + return ETHTOOL_A_CABLE_RESULT_CODE_OK; + case OA_1000BT1_HDD_TDR_STATUS_OPEN: + return ETHTOOL_A_CABLE_RESULT_CODE_OPEN; + case OA_1000BT1_HDD_TDR_STATUS_SHORT: + return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT; + case OA_1000BT1_HDD_TDR_STATUS_NOISE: + return ETHTOOL_A_CABLE_RESULT_CODE_NOISE; + default: + if (dist_val == OA_1000BT1_HDD_TDR_DISTANCE_RESOLUTION_NOT_POSSIBLE) + return ETHTOOL_A_CABLE_RESULT_CODE_RESOLUTION_NOT_POSSIBLE; + return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC; + } +} + +/** + * oa_1000bt1_get_tdr_distance - Get distance to the main fault from TDR + * register value + * @reg_value: Value read from the TDR register + * + * This function takes a register value from the HDD.TDR register and extracts + * the distance to the main fault detected by the TDR feature. The distance is + * measured in centimeters and ranges from 0 to 3100 centimeters. If the + * distance is not available (0x3f), the function returns -ERANGE. + * + * Return: The distance to the main fault in centimeters, or -ERANGE if the + * resolution is not possible. + */ +int oa_1000bt1_get_tdr_distance(u16 reg_value) +{ + u8 dist_val = (reg_value & OA_1000BT1_HDD_TDR_DISTANCE_MASK) >> 8; + + if (dist_val == OA_1000BT1_HDD_TDR_DISTANCE_RESOLUTION_NOT_POSSIBLE) + return -ERANGE; + + return dist_val * 100; +} + diff --git a/include/linux/open_alliance_helpers.h b/include/linux/open_alliance_helpers.h new file mode 100644 index 0000000000000..8b7d97bc6f186 --- /dev/null +++ b/include/linux/open_alliance_helpers.h @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef OPEN_ALLIANCE_HELPERS_H +#define OPEN_ALLIANCE_HELPERS_H + +/* + * These defines reflect the TDR (Time Delay Reflection) diagnostic feature + * for 1000BASE-T1 automotive Ethernet PHYs as specified by the OPEN Alliance. + * + * The register values are part of the HDD.TDR register, which provides + * information about the cable status and faults. The exact register offset + * is device-specific and should be provided by the driver. + */ +#define OA_1000BT1_HDD_TDR_ACTIVATION_MASK GENMASK(1, 0) +#define OA_1000BT1_HDD_TDR_ACTIVATION_OFF 1 +#define OA_1000BT1_HDD_TDR_ACTIVATION_ON 2 + +#define OA_1000BT1_HDD_TDR_STATUS_MASK GENMASK(7, 4) +#define OA_1000BT1_HDD_TDR_STATUS_SHORT 3 +#define OA_1000BT1_HDD_TDR_STATUS_OPEN 6 +#define OA_1000BT1_HDD_TDR_STATUS_NOISE 5 +#define OA_1000BT1_HDD_TDR_STATUS_CABLE_OK 7 +#define OA_1000BT1_HDD_TDR_STATUS_TEST_IN_PROGRESS 8 +#define OA_1000BT1_HDD_TDR_STATUS_TEST_NOT_POSSIBLE 13 + +/* + * OA_1000BT1_HDD_TDR_DISTANCE_MASK: + * This mask is used to extract the distance to the first/main fault + * detected by the TDR feature. Each bit represents an approximate distance + * of 1 meter, ranging from 0 to 31 meters. The exact interpretation of the + * bits may vary, but generally: + * 000000 = no error + * 000001 = error about 0-1m away + * 000010 = error between 1-2m away + * ... + * 011111 = error about 30-31m away + * 111111 = resolution not possible / out of distance + */ +#define OA_1000BT1_HDD_TDR_DISTANCE_MASK GENMASK(13, 8) +#define OA_1000BT1_HDD_TDR_DISTANCE_NO_ERROR 0 +#define OA_1000BT1_HDD_TDR_DISTANCE_RESOLUTION_NOT_POSSIBLE 0x3f + +int oa_1000bt1_get_ethtool_cable_result_code(u16 reg_value); +int oa_1000bt1_get_tdr_distance(u16 reg_value); + +#endif /* OPEN_ALLIANCE_HELPERS_H */ +
Introduce helper functions specific to Open Alliance diagnostics, integrating them into the PHY framework. Currently, these helpers are limited to 1000BaseT1 specific TDR functionality. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/Makefile | 2 +- drivers/net/phy/open_alliance_helpers.c | 70 +++++++++++++++++++++++++ include/linux/open_alliance_helpers.h | 47 +++++++++++++++++ 3 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 drivers/net/phy/open_alliance_helpers.c create mode 100644 include/linux/open_alliance_helpers.h