[PATCH net-next v3 0/3] Add Statistics Support for DP83TG720 PHY

[PATCH net-next v3 0/3] Add Statistics Support for DP83TG720 PHY

[Oleksij Rempel]

Hi all,

This patch series primarily focuses on adding statistics support for the
DP83TG720 PHY driver. Along the way, to achieve this goal, I introduced
defines for various statistics to ensure they are reusable across
different PHY drivers.

Changes are tracked in separate patches.

Oleksij Rempel (3):
  phy: open_alliance_helpers: Add defines for link quality metrics
  phy: Add defines for standardized PHY generic counters
  phy: dp83tg720: Add statistics support

 drivers/net/phy/dp83tg720.c             | 324 ++++++++++++++++++++++++
 drivers/net/phy/open_alliance_helpers.h |  14 +
 include/linux/phy.h                     |   6 +
 3 files changed, 344 insertions(+)

--
2.39.2

[PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Oleksij Rempel]

Introduce a set of defines for link quality (LQ) related metrics in the
Open Alliance helpers. These metrics include:

- `oa_lq_lfl_esd_event_count`: Number of ESD events detected by the Link
  Failures and Losses (LFL).
- `oa_lq_link_training_time`: Time required to establish a link.
- `oa_lq_remote_receiver_time`: Time required until the remote receiver
  signals that it is locked.
- `oa_lq_local_receiver_time`: Time required until the local receiver is
  locked.
- `oa_lq_lfl_link_loss_count`: Number of link losses.
- `oa_lq_lfl_link_failure_count`: Number of link failures that do not
  cause a link loss.

These standardized defines will be used by PHY drivers to report these
statistics.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/open_alliance_helpers.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/open_alliance_helpers.h b/drivers/net/phy/open_alliance_helpers.h
index 8b7d97bc6f186..f8b392671e20d 100644
--- a/drivers/net/phy/open_alliance_helpers.h
+++ b/drivers/net/phy/open_alliance_helpers.h
@@ -3,6 +3,20 @@
 #ifndef OPEN_ALLIANCE_HELPERS_H
 #define OPEN_ALLIANCE_HELPERS_H
 
+/* Link quality (LQ) related metrics */
+/* The number of ESD events detected by the Link Failures and Losses (LFL) */
+#define OA_LQ_LFL_ESD_EVENT_COUNT		"oa_lq_lfl_esd_event_count"
+/* Time required to establish a link */
+#define OA_LQ_LINK_TRAINING_TIME		"oa_lq_link_training_time"
+/* Time required until the remote receiver is signaling that it is locked */
+#define OA_LQ_REMOTE_RECEIVER_TIME		"oa_lq_remote_receiver_time"
+/* Time required until the local receiver is locked */
+#define OA_LQ_LOCAL_RECEIVER_TIME		"oa_lq_local_receiver_time"
+/* Number of link losses */
+#define OA_LQ_LFL_LINK_LOSS_COUNT		"oa_lq_lfl_link_loss_count"
+/* Number of link failures causing NOT a link loss */
+#define OA_LQ_LFL_LINK_FAILURE_COUNT		"oa_lq_lfl_link_failure_count"
+
 /*
  * These defines reflect the TDR (Time Delay Reflection) diagnostic feature
  * for 1000BASE-T1 automotive Ethernet PHYs as specified by the OPEN Alliance.
-- 
2.39.2

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Andrew Lunn]

On Thu, Aug 22, 2024 at 01:59:37PM +0200, Oleksij Rempel wrote:
> Introduce a set of defines for link quality (LQ) related metrics in the
> Open Alliance helpers. These metrics include:
> 
> - `oa_lq_lfl_esd_event_count`: Number of ESD events detected by the Link
>   Failures and Losses (LFL).
> - `oa_lq_link_training_time`: Time required to establish a link.
> - `oa_lq_remote_receiver_time`: Time required until the remote receiver
>   signals that it is locked.
> - `oa_lq_local_receiver_time`: Time required until the local receiver is
>   locked.
> - `oa_lq_lfl_link_loss_count`: Number of link losses.
> - `oa_lq_lfl_link_failure_count`: Number of link failures that do not
>   cause a link loss.
> 
> These standardized defines will be used by PHY drivers to report these
> statistics.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Jakub Kicinski]

On Thu, 22 Aug 2024 13:59:37 +0200 Oleksij Rempel wrote:
> Introduce a set of defines for link quality (LQ) related metrics in the
> Open Alliance helpers. These metrics include:
> 
> - `oa_lq_lfl_esd_event_count`: Number of ESD events detected by the Link
>   Failures and Losses (LFL).
> - `oa_lq_link_training_time`: Time required to establish a link.
> - `oa_lq_remote_receiver_time`: Time required until the remote receiver
>   signals that it is locked.
> - `oa_lq_local_receiver_time`: Time required until the local receiver is
>   locked.
> - `oa_lq_lfl_link_loss_count`: Number of link losses.
> - `oa_lq_lfl_link_failure_count`: Number of link failures that do not
>   cause a link loss.
> 
> These standardized defines will be used by PHY drivers to report these
> statistics.

If these are defined by a standard why not report them as structured
data? Like we report ethtool_eth_mac_stats, ethtool_eth_ctrl_stats,
ethtool_rmon_stats etc.?

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Andrew Lunn]

On Mon, Aug 26, 2024 at 09:32:17AM -0700, Jakub Kicinski wrote:
> On Thu, 22 Aug 2024 13:59:37 +0200 Oleksij Rempel wrote:
> > Introduce a set of defines for link quality (LQ) related metrics in the
> > Open Alliance helpers. These metrics include:
> > 
> > - `oa_lq_lfl_esd_event_count`: Number of ESD events detected by the Link
> >   Failures and Losses (LFL).
> > - `oa_lq_link_training_time`: Time required to establish a link.
> > - `oa_lq_remote_receiver_time`: Time required until the remote receiver
> >   signals that it is locked.
> > - `oa_lq_local_receiver_time`: Time required until the local receiver is
> >   locked.
> > - `oa_lq_lfl_link_loss_count`: Number of link losses.
> > - `oa_lq_lfl_link_failure_count`: Number of link failures that do not
> >   cause a link loss.
> > 
> > These standardized defines will be used by PHY drivers to report these
> > statistics.
> 
> If these are defined by a standard why not report them as structured
> data? Like we report ethtool_eth_mac_stats, ethtool_eth_ctrl_stats,
> ethtool_rmon_stats etc.?

We could do, but we have no infrastructure for this at the
moment. These are PHY statistics, not MAC statistics. We don't have
all the ethool_op infrastructure, etc. We also need to think about
which PHY do we want the statics from, the bootlin code for multiple
PHYs etc.

I will leave it up to Oleksij, but it would neatly avoid different
vendors returning the same stats with different names.

	Andrew

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Jakub Kicinski]

On Mon, 26 Aug 2024 19:12:52 +0200 Andrew Lunn wrote:
> > If these are defined by a standard why not report them as structured
> > data? Like we report ethtool_eth_mac_stats, ethtool_eth_ctrl_stats,
> > ethtool_rmon_stats etc.?  
> 
> We could do, but we have no infrastructure for this at the
> moment. These are PHY statistics, not MAC statistics.
> We don't have all the ethool_op infrastructure, etc.

This appears to not be a concern when calling phy_ops->get_sset_count()
You know this code better than me, but I can't think of any big 'infra'
that we'd need. ethtool code can just call phy_ops, the rest is likely
a repeat of the "MAC"/ethtool_ops stats.

> We also need to think about which PHY do we want the statics from,
> the bootlin code for multiple PHYs etc.

True, that said I'd rather we added a new group for the well-defined
PHY stats without supporting multi-PHY, than let the additional
considerations prevent us from making progress. ioctl stats are
strictly worse.

I'm sorry to pick on this particular series, but the structured ethtool
stats have been around for 3 years. Feels like it's time to fill the
gaps on the PHY side.

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Oleksij Rempel]

Hi Jakub,

On Mon, Aug 26, 2024 at 12:57:19PM -0700, Jakub Kicinski wrote:
> On Mon, 26 Aug 2024 19:12:52 +0200 Andrew Lunn wrote:
> > > If these are defined by a standard why not report them as structured
> > > data? Like we report ethtool_eth_mac_stats, ethtool_eth_ctrl_stats,
> > > ethtool_rmon_stats etc.?  
> > 
> > We could do, but we have no infrastructure for this at the
> > moment. These are PHY statistics, not MAC statistics.
> > We don't have all the ethool_op infrastructure, etc.
> 
> This appears to not be a concern when calling phy_ops->get_sset_count()
> You know this code better than me, but I can't think of any big 'infra'
> that we'd need. ethtool code can just call phy_ops, the rest is likely
> a repeat of the "MAC"/ethtool_ops stats.
> 
> > We also need to think about which PHY do we want the statics from,
> > the bootlin code for multiple PHYs etc.
> 
> True, that said I'd rather we added a new group for the well-defined
> PHY stats without supporting multi-PHY, than let the additional
> considerations prevent us from making progress. ioctl stats are
> strictly worse.
> 
> I'm sorry to pick on this particular series, but the structured ethtool
> stats have been around for 3 years. Feels like it's time to fill the
> gaps on the PHY side.

I completely agree with you, but I currently don't have additional
budget for this project.

What might help is a diagnostic concept that I can present to my
customers to seek sponsorship for implementing various interfaces based
on their relevance and priority for different projects.

Since I haven't seen an existing concept from the end product or
component vendors, I suggest starting this within the Linux Kernel
NetDev community.

I'll send my current thoughts in a separate email

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Jakub Kicinski]

On Tue, 27 Aug 2024 06:51:27 +0200 Oleksij Rempel wrote:
> I completely agree with you, but I currently don't have additional
> budget for this project.

Is this a legit reason not to do something relatively simple?
Especially that we're talking about uAPI, once we go down
the string path I presume they will stick around forever.

IDK. Additional opinions welcome...

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Oleksij Rempel]

On Tue, Aug 27, 2024 at 11:33:00AM -0700, Jakub Kicinski wrote:
> On Tue, 27 Aug 2024 06:51:27 +0200 Oleksij Rempel wrote:
> > I completely agree with you, but I currently don't have additional
> > budget for this project.
> 
> Is this a legit reason not to do something relatively simple?

Due to the nature of my work in a consulting company, my time is scheduled
across multiple customers. For the 10BaseT1 PHY, I had 2 days budgeted left,
which allowed me to implement some extra diagnostics. This was simple,
predictable, and within the scope of the original task.

However, now that the budget for this task and customer has been used up, any
additional work would require a full process:
- I would need to sell the idea to the customer.
- The new task would need to be prioritized.
- It would then be scheduled, which could happen this year, next year, or
  possibly never.

A similar situation occurred with the EEE implementation. I started with a
simple fix for Atheros PHY's SmartEEE, but it led to reworking the entire EEE
infrastructure in the kernel. Once the budget was exhausted, I couldn’t
continue with SmartEEE for Atheros PHYs. These are the risks inherent to
consulting work. I still see it as not wasted time, because we have a better
EEE infrastructure now.

Considering that you've requested a change to the uAPI, the work has now become
more predictable. I can plan for it within the task and update the required
time budget accordingly. However, it's worth noting that while this work is
manageable, the time spent on this particular task could be seen as somewhat
wasted from a budget perspective, as it wasn't part of the original scope.

> Especially that we're talking about uAPI, once we go down
> the string path I presume they will stick around forever.

Yes, I agree with it. I just needed this feedback as early as possible.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Jakub Kicinski]

On Wed, 28 Aug 2024 06:50:46 +0200 Oleksij Rempel wrote:
> Considering that you've requested a change to the uAPI, the work has now become
> more predictable. I can plan for it within the task and update the required
> time budget accordingly. However, it's worth noting that while this work is
> manageable, the time spent on this particular task could be seen as somewhat
> wasted from a budget perspective, as it wasn't part of the original scope.

I can probably take a stab at the kernel side, since I know the code
already shouldn't take me more more than an hour. Would that help?
You'd still need to retest, fix bugs. And go thru review.. so all
the not-so-fun parts

> > Especially that we're talking about uAPI, once we go down
> > the string path I presume they will stick around forever.  
> 
> Yes, I agree with it. I just needed this feedback as early as possible.

Andrew? Do you want to decide? :)

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Andrew Lunn]

On Wed, Aug 28, 2024 at 01:34:28PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 06:50:46 +0200 Oleksij Rempel wrote:
> > Considering that you've requested a change to the uAPI, the work has now become
> > more predictable. I can plan for it within the task and update the required
> > time budget accordingly. However, it's worth noting that while this work is
> > manageable, the time spent on this particular task could be seen as somewhat
> > wasted from a budget perspective, as it wasn't part of the original scope.
> 
> I can probably take a stab at the kernel side, since I know the code
> already shouldn't take me more more than an hour. Would that help?
> You'd still need to retest, fix bugs. And go thru review.. so all
> the not-so-fun parts
> 
> > > Especially that we're talking about uAPI, once we go down
> > > the string path I presume they will stick around forever.  
> > 
> > Yes, I agree with it. I just needed this feedback as early as possible.
> 
> Andrew? Do you want to decide? :)

I agree about avoiding free test strings. Something more structures
would be good.

I can definitely help out with review, but i don't have any time at
the moment for writing code.

	Andrew

Re: [PATCH net-next v3 1/3] phy: open_alliance_helpers: Add defines for link quality metrics

[Oleksij Rempel]

On Wed, Aug 28, 2024 at 01:34:28PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 06:50:46 +0200 Oleksij Rempel wrote:
> > Considering that you've requested a change to the uAPI, the work has now become
> > more predictable. I can plan for it within the task and update the required
> > time budget accordingly. However, it's worth noting that while this work is
> > manageable, the time spent on this particular task could be seen as somewhat
> > wasted from a budget perspective, as it wasn't part of the original scope.
> 
> I can probably take a stab at the kernel side, since I know the code
> already shouldn't take me more more than an hour. Would that help?

Ack. This will be a great help.

> You'd still need to retest, fix bugs. And go thru review.. so all
> the not-so-fun parts

Sure :)

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v3 2/3] phy: Add defines for standardized PHY generic counters

[Oleksij Rempel]

Introduce a set of defines for generic PHY-specific counters.
These defines provide standardized names for commonly
tracked statistics across different PHY drivers, ensuring consistency in
how these metrics are reported:

- `PHY_TX_PKT_COUNT`: Transmit packet count.
- `PHY_RX_PKT_COUNT`: Receive packet count.
- `PHY_TX_ERR_COUNT`: Transmit error count.
- `PHY_RX_ERR_COUNT`: Receive error count.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6b7d40d49129d..65fd56ca8cb39 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2209,4 +2209,10 @@ module_exit(phy_module_exit)
 bool phy_driver_is_genphy(struct phy_device *phydev);
 bool phy_driver_is_genphy_10g(struct phy_device *phydev);
 
+/* Defines for PHY specific counters */
+#define PHY_TX_PKT_COUNT	"tx_pkt_cnt"
+#define PHY_RX_PKT_COUNT	"rx_pkt_cnt"
+#define PHY_TX_ERR_COUNT	"tx_err_cnt"
+#define PHY_RX_ERR_COUNT	"rx_err_cnt"
+
 #endif /* __PHY_H */
-- 
2.39.2

Re: [PATCH net-next v3 2/3] phy: Add defines for standardized PHY generic counters

[Andrew Lunn]

On Thu, Aug 22, 2024 at 01:59:38PM +0200, Oleksij Rempel wrote:
> Introduce a set of defines for generic PHY-specific counters.
> These defines provide standardized names for commonly
> tracked statistics across different PHY drivers, ensuring consistency in
> how these metrics are reported:
> 
> - `PHY_TX_PKT_COUNT`: Transmit packet count.
> - `PHY_RX_PKT_COUNT`: Receive packet count.
> - `PHY_TX_ERR_COUNT`: Transmit error count.
> - `PHY_RX_ERR_COUNT`: Receive error count.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v3 3/3] phy: dp83tg720: Add statistics support

[Oleksij Rempel]

Introduce statistics support for the DP83TG720 PHY driver, enabling
detailed monitoring and reporting of link quality and packet-related
metrics.

To avoid double reading of certain registers, the implementation caches
all relevant register values in a single operation. This approach
ensures accurate and consistent data retrieval, particularly for
registers that clear upon reading or require special handling.

Some of the statistics, such as link training times, do not increment
and therefore require special handling during the extraction process.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v3:
- update comment for dp83tg720_get_stats()
changes v2:
- use ethtool_puts()
- use defines for statistic names
---
 drivers/net/phy/dp83tg720.c | 324 ++++++++++++++++++++++++++++++++++++
 1 file changed, 324 insertions(+)

diff --git a/drivers/net/phy/dp83tg720.c b/drivers/net/phy/dp83tg720.c
index 0ef4d7dba0656..43d7febdfd772 100644
--- a/drivers/net/phy/dp83tg720.c
+++ b/drivers/net/phy/dp83tg720.c
@@ -51,6 +51,20 @@
 /* Register 0x0405: Unknown Register */
 #define DP83TG720S_UNKNOWN_0405			0x405
 
+#define DP83TG720S_A2D_REG_66			0x442
+#define DP83TG720S_ESD_EVENT_COUNT_MASK		GENMASK(14, 9)
+
+#define DP83TG720S_LINK_QUAL_1			0x543
+#define DP83TG720S_LINK_TRAINING_TIME_MASK	GENMASK(7, 0)
+
+#define DP83TG720S_LINK_QUAL_2			0x544
+#define DP83TG720S_REMOTE_RECEIVER_TIME_MASK	GENMASK(15, 8)
+#define DP83TG720S_LOCAL_RECEIVER_TIME_MASK	GENMASK(7, 0)
+
+#define DP83TG720S_LINK_QUAL_3			0x547
+#define DP83TG720S_LINK_LOSS_CNT_MASK		GENMASK(15, 10)
+#define DP83TG720S_LINK_FAIL_CNT_MASK		GENMASK(9, 0)
+
 /* Register 0x0576: TDR Master Link Down Control */
 #define DP83TG720S_TDR_MASTER_LINK_DOWN		0x576
 
@@ -60,6 +74,24 @@
 /* In RGMII mode, Enable or disable the internal delay for TXD */
 #define DP83TG720S_RGMII_TX_CLK_SEL		BIT(0)
 
+#define DP83TG720S_PKT_STAT_1			0x639
+#define DP83TG720S_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_2			0x63A
+#define DP83TG720S_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_3			0x63B
+#define DP83TG720S_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_4			0x63C
+#define DP83TG720S_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_5			0x63D
+#define DP83TG720S_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)
+
+#define DP83TG720S_PKT_STAT_6			0x63E
+#define DP83TG720S_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
+
 /* Register 0x083F: Unknown Register */
 #define DP83TG720S_UNKNOWN_083F			0x83f
 
@@ -69,6 +101,280 @@
 
 #define DP83TG720_SQI_MAX			7
 
+struct dp83tg720_cache_reg {
+	u16 mmd;
+	u16 reg;
+};
+
+#define DP83TG720_FLAG_COUNTER BIT(0)
+
+struct dp83tg720_hw_stat {
+	const char *string;
+	u8 cache_index1;
+	u8 cache_index2;  /* If a statistic spans multiple registers */
+	u32 mask1;
+	u32 mask2;        /* Mask for the second register */
+	u8 shift1;
+	u8 shift2;        /* Shift for the second register */
+	u8 flags;
+};
+
+enum dp83tg720_cache_reg_idx {
+	DP83TG720_CACHE_A2D_REG_66 = 0,
+	DP83TG720_CACHE_LINK_QUAL_1,
+	DP83TG720_CACHE_LINK_QUAL_2,
+	DP83TG720_CACHE_LINK_QUAL_3,
+	DP83TG720_CACHE_PKT_STAT_1,
+	DP83TG720_CACHE_PKT_STAT_2,
+	DP83TG720_CACHE_PKT_STAT_3,
+	DP83TG720_CACHE_PKT_STAT_4,
+	DP83TG720_CACHE_PKT_STAT_5,
+	DP83TG720_CACHE_PKT_STAT_6,
+
+	DP83TG720_CACHE_EMPTY,
+};
+
+static const struct dp83tg720_cache_reg dp83tg720_cache_regs[] = {
+	[DP83TG720_CACHE_A2D_REG_66] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_A2D_REG_66,
+	},
+	[DP83TG720_CACHE_LINK_QUAL_1] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_LINK_QUAL_1,
+	},
+	[DP83TG720_CACHE_LINK_QUAL_2] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_LINK_QUAL_2,
+	},
+	[DP83TG720_CACHE_LINK_QUAL_3] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_LINK_QUAL_3,
+	},
+	[DP83TG720_CACHE_PKT_STAT_1] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_1,
+	},
+	[DP83TG720_CACHE_PKT_STAT_2] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_2,
+	},
+	[DP83TG720_CACHE_PKT_STAT_3] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_3,
+	},
+	[DP83TG720_CACHE_PKT_STAT_4] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_4,
+	},
+	[DP83TG720_CACHE_PKT_STAT_5] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_5,
+	},
+	[DP83TG720_CACHE_PKT_STAT_6] = {
+		.mmd = MDIO_MMD_VEND2,
+		.reg = DP83TG720S_PKT_STAT_6,
+	},
+};
+
+static const struct dp83tg720_hw_stat dp83tg720_hw_stats[] = {
+	{
+		.string = OA_LQ_LFL_ESD_EVENT_COUNT,
+		.cache_index1 = DP83TG720_CACHE_A2D_REG_66,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_ESD_EVENT_COUNT_MASK,
+		.shift1 = 9,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = OA_LQ_LINK_TRAINING_TIME,
+		.cache_index1 = DP83TG720_CACHE_LINK_QUAL_1,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_LINK_TRAINING_TIME_MASK,
+	},
+	{
+		.string = OA_LQ_REMOTE_RECEIVER_TIME,
+		.cache_index1 = DP83TG720_CACHE_LINK_QUAL_2,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_REMOTE_RECEIVER_TIME_MASK,
+		.shift1 = 8,
+	},
+	{
+		.string = OA_LQ_LOCAL_RECEIVER_TIME,
+		.cache_index1 = DP83TG720_CACHE_LINK_QUAL_2,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_LOCAL_RECEIVER_TIME_MASK,
+	},
+	{
+		.string = OA_LQ_LFL_LINK_LOSS_COUNT,
+		.cache_index1 = DP83TG720_CACHE_LINK_QUAL_3,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_LINK_LOSS_CNT_MASK,
+		.shift1 = 10,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = OA_LQ_LFL_LINK_FAILURE_COUNT,
+		.cache_index1 = DP83TG720_CACHE_LINK_QUAL_3,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_LINK_FAIL_CNT_MASK,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = PHY_TX_PKT_COUNT,
+		.cache_index1 = DP83TG720_CACHE_PKT_STAT_1,
+		.cache_index2 = DP83TG720_CACHE_PKT_STAT_2,
+		.mask1 = DP83TG720S_TX_PKT_CNT_15_0_MASK,
+		.mask2 = DP83TG720S_TX_PKT_CNT_31_16_MASK,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = PHY_TX_ERR_COUNT,
+		.cache_index1 = DP83TG720_CACHE_PKT_STAT_3,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_TX_ERR_PKT_CNT_MASK,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = PHY_RX_PKT_COUNT,
+		.cache_index1 = DP83TG720_CACHE_PKT_STAT_4,
+		.cache_index2 = DP83TG720_CACHE_PKT_STAT_5,
+		.mask1 = DP83TG720S_RX_PKT_CNT_15_0_MASK,
+		.mask2 = DP83TG720S_RX_PKT_CNT_31_16_MASK,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+	{
+		.string = PHY_RX_ERR_COUNT,
+		.cache_index1 = DP83TG720_CACHE_PKT_STAT_6,
+		.cache_index2 = DP83TG720_CACHE_EMPTY,
+		.mask1 = DP83TG720S_RX_ERR_PKT_CNT_MASK,
+		.flags = DP83TG720_FLAG_COUNTER,
+	},
+};
+
+struct dp83tg720_priv {
+	u64 stats[ARRAY_SIZE(dp83tg720_hw_stats)];
+	u16 reg_cache[ARRAY_SIZE(dp83tg720_cache_regs)];
+};
+
+/**
+ * dp83tg720_cache_reg_values - Cache register values to avoid clearing counters.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * Reads and caches the values of all relevant registers.
+ *
+ * Returns: 0 on success, a negative error code on failure.
+ */
+static int dp83tg720_cache_reg_values(struct phy_device *phydev)
+{
+	struct dp83tg720_priv *priv = phydev->priv;
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(dp83tg720_cache_regs); i++) {
+		const struct dp83tg720_cache_reg *cache_reg =
+			&dp83tg720_cache_regs[i];
+
+		ret = phy_read_mmd(phydev, cache_reg->mmd, cache_reg->reg);
+		if (ret < 0)
+			return ret;
+
+		priv->reg_cache[i] = (u16)ret;
+	}
+
+	return 0;
+}
+
+/**
+ * dp83tg720_extract_stat_value - Extract specific statistic value from cache.
+ * @phydev: Pointer to the phy_device structure.
+ * @i: Index of the statistic in the dp83tg720_hw_stats array.
+ *
+ * Extracts the specific statistic value from the cached register values.
+ *
+ * Returns: The extracted statistic value.
+ */
+static u64 dp83tg720_extract_stat_value(struct phy_device *phydev, int i)
+{
+	const struct dp83tg720_hw_stat *stat = &dp83tg720_hw_stats[i];
+	struct dp83tg720_priv *priv = phydev->priv;
+	u32 val1, val2 = 0;
+
+	val1 = (priv->reg_cache[stat->cache_index1] & stat->mask1);
+	val1 >>= stat->shift1;
+	if (stat->cache_index2 != DP83TG720_CACHE_EMPTY) {
+		val2 = (priv->reg_cache[stat->cache_index2] & stat->mask2);
+		val2 >>= stat->shift2;
+	}
+
+	if (stat->flags & DP83TG720_FLAG_COUNTER)
+		priv->stats[i] += val1 | (val2 << 16);
+	else
+		priv->stats[i] = val1 | (val2 << 16);
+
+	return priv->stats[i];
+}
+
+/**
+ * dp83tg720_get_sset_count - Get the number of statistics sets.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * Returns: The number of statistics sets.
+ */
+static int dp83tg720_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(dp83tg720_hw_stats);
+}
+
+/**
+ * dp83tg720_get_strings - Get the strings for the statistics.
+ * @phydev: Pointer to the phy_device structure.
+ * @data: Pointer to the buffer where the strings will be stored.
+ *
+ * Fills the buffer with the strings for the statistics
+ */
+static void dp83tg720_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp83tg720_hw_stats); i++)
+		ethtool_puts(&data, dp83tg720_hw_stats[i].string);
+}
+
+/**
+ * dp83tg720_get_stats - Get the statistics values.
+ * @phydev: Pointer to the phy_device structure.
+ * @stats: Pointer to the ethtool_stats structure.
+ * @data: Pointer to the buffer where the statistics values will be stored.
+ */
+static void dp83tg720_get_stats(struct phy_device *phydev,
+				struct ethtool_stats *stats, u64 *data)
+{
+	int i, j = 0;
+
+	dp83tg720_cache_reg_values(phydev);
+
+	for (i = 0; i < ARRAY_SIZE(dp83tg720_hw_stats); i++) {
+		data[j] = dp83tg720_extract_stat_value(phydev, i);
+		j++;
+	}
+}
+
+/**
+ * dp83tg720_update_stats - Update the statistics values.
+ * @phydev: Pointer to the phy_device structure.
+ *
+ * Updates the statistics values.
+ */
+static void dp83tg720_update_stats(struct phy_device *phydev)
+{
+	int i;
+
+	dp83tg720_cache_reg_values(phydev);
+
+	for (i = 0; i < ARRAY_SIZE(dp83tg720_hw_stats); i++)
+		dp83tg720_extract_stat_value(phydev, i);
+}
+
 /**
  * dp83tg720_cable_test_start - Start the cable test for the DP83TG720 PHY.
  * @phydev: Pointer to the phy_device structure.
@@ -208,6 +514,7 @@ static int dp83tg720_read_status(struct phy_device *phydev)
 	u16 phy_sts;
 	int ret;
 
+	dp83tg720_update_stats(phydev);
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
 
@@ -341,12 +648,26 @@ static int dp83tg720_config_init(struct phy_device *phydev)
 	return genphy_c45_pma_baset1_read_master_slave(phydev);
 }
 
+static int dp83tg720_probe(struct phy_device *phydev)
+{
+	struct dp83tg720_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static struct phy_driver dp83tg720_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(DP83TG720S_PHY_ID),
 	.name		= "TI DP83TG720S",
 
 	.flags          = PHY_POLL_CABLE_TEST,
+	.probe		= dp83tg720_probe,
 	.config_aneg	= dp83tg720_config_aneg,
 	.read_status	= dp83tg720_read_status,
 	.get_features	= genphy_c45_pma_read_ext_abilities,
@@ -355,6 +676,9 @@ static struct phy_driver dp83tg720_driver[] = {
 	.get_sqi_max	= dp83tg720_get_sqi_max,
 	.cable_test_start = dp83tg720_cable_test_start,
 	.cable_test_get_status = dp83tg720_cable_test_get_status,
+	.get_sset_count = dp83tg720_get_sset_count,
+	.get_strings	= dp83tg720_get_strings,
+	.get_stats	= dp83tg720_get_stats,
 
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.39.2

Re: [PATCH net-next v3 3/3] phy: dp83tg720: Add statistics support

[Andrew Lunn]

On Thu, Aug 22, 2024 at 01:59:39PM +0200, Oleksij Rempel wrote:
> Introduce statistics support for the DP83TG720 PHY driver, enabling
> detailed monitoring and reporting of link quality and packet-related
> metrics.
> 
> To avoid double reading of certain registers, the implementation caches
> all relevant register values in a single operation. This approach
> ensures accurate and consistent data retrieval, particularly for
> registers that clear upon reading or require special handling.
> 
> Some of the statistics, such as link training times, do not increment
> and therefore require special handling during the extraction process.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew