Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support

[Mateusz Polchlopek <mateusz.polchlopek@intel.com>]

On 12/3/2024 8:56 AM, Oleksij Rempel wrote:
> Add support for reporting PHY statistics in the DP83TD510 driver. This
> includes cumulative tracking of transmit/receive packet counts, and
> error counts. Implemented functions to update and provide statistics via
> ethtool, with optional polling support enabled through `PHY_POLL_STATS`.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>   drivers/net/phy/dp83td510.c | 98 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
> index 92aa3a2b9744..08d61a6a8c61 100644
> --- a/drivers/net/phy/dp83td510.c
> +++ b/drivers/net/phy/dp83td510.c
> @@ -34,6 +34,24 @@
>   #define DP83TD510E_CTRL_HW_RESET		BIT(15)
>   #define DP83TD510E_CTRL_SW_RESET		BIT(14)
>   
> +#define DP83TD510E_PKT_STAT_1			0x12b
> +#define DP83TD510E_TX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_2			0x12c
> +#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(15, 0)

Shouldn't it be GENMASK(31, 16) ? If not then I think that macro
name is a little bit misleading

> +
> +#define DP83TD510E_PKT_STAT_3			0x12d
> +#define DP83TD510E_TX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_4			0x12e
> +#define DP83TD510E_RX_PKT_CNT_15_0_MASK		GENMASK(15, 0)
> +
> +#define DP83TD510E_PKT_STAT_5			0x12f
> +#define DP83TD510E_RX_PKT_CNT_31_16_MASK	GENMASK(15, 0)

Same as above

> +
> +#define DP83TD510E_PKT_STAT_6			0x130
> +#define DP83TD510E_RX_ERR_PKT_CNT_MASK		GENMASK(15, 0)
> +
>   #define DP83TD510E_AN_STAT_1			0x60c
>   #define DP83TD510E_MASTER_SLAVE_RESOL_FAIL	BIT(15)
>   
> @@ -58,8 +76,16 @@ static const u16 dp83td510_mse_sqi_map[] = {
>   	0x0000  /* 24dB =< SNR */
>   };
>   
> +struct dp83td510_stats {
> +	u64 tx_pkt_cnt;
> +	u64 tx_err_pkt_cnt;
> +	u64 rx_pkt_cnt;
> +	u64 rx_err_pkt_cnt;
> +};
> +
>   struct dp83td510_priv {
>   	bool alcd_test_active;
> +	struct dp83td510_stats stats;
>   };
>   
>   /* Time Domain Reflectometry (TDR) Functionality of DP83TD510 PHY
> @@ -177,6 +203,74 @@ struct dp83td510_priv {
>   #define DP83TD510E_ALCD_COMPLETE			BIT(15)
>   #define DP83TD510E_ALCD_CABLE_LENGTH			GENMASK(10, 0)
>   
> +/**
> + * dp83td510_update_stats - Update the PHY statistics for the DP83TD510 PHY.
> + * @phydev: Pointer to the phy_device structure.
> + *
> + * The function reads the PHY statistics registers and updates the statistics
> + * structure.
> + *
> + * Returns: 0 on success or a negative error code on failure.

Typo, it should be 'Return:' not 'Returns:'

> + */
> +static int dp83td510_update_stats(struct phy_device *phydev)
> +{
> +	struct dp83td510_priv *priv = phydev->priv;
> +	u64 count;
> +	int ret;
> +
> +	/* DP83TD510E_PKT_STAT_1 to DP83TD510E_PKT_STAT_6 registers are cleared
> +	 * after reading them in a sequence. A reading of this register not in
> +	 * sequence will prevent them from being cleared.
> +	 */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_1);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_TX_PKT_CNT_15_0_MASK, ret);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_2);
> +	if (ret < 0)
> +		return ret;
> +	count |= (u64)FIELD_GET(DP83TD510E_TX_PKT_CNT_31_16_MASK, ret) << 16;

Ah... here you do shift. I think it would be better to just define

#define DP83TD510E_TX_PKT_CNT_31_16_MASK	GENMASK(31, 16)

instead of shifting, what do you think ?

> +	ethtool_stat_add(&priv->stats.tx_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_3);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_TX_ERR_PKT_CNT_MASK, ret);
> +	ethtool_stat_add(&priv->stats.tx_err_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_4);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_RX_PKT_CNT_15_0_MASK, ret);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_5);
> +	if (ret < 0)
> +		return ret;
> +	count |= (u64)FIELD_GET(DP83TD510E_RX_PKT_CNT_31_16_MASK, ret) << 16;
> +	ethtool_stat_add(&priv->stats.rx_pkt_cnt, count);
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, DP83TD510E_PKT_STAT_6);
> +	if (ret < 0)
> +		return ret;
> +	count = FIELD_GET(DP83TD510E_RX_ERR_PKT_CNT_MASK, ret);
> +	ethtool_stat_add(&priv->stats.rx_err_pkt_cnt, count);
> +
> +	return 0;
> +}
> +
> +static void dp83td510_get_phy_stats(struct phy_device *phydev,
> +				    struct ethtool_eth_phy_stats *eth_stats,
> +				    struct ethtool_phy_stats *stats)
> +{
> +	struct dp83td510_priv *priv = phydev->priv;
> +
> +	stats->tx_packets = priv->stats.tx_pkt_cnt;
> +	stats->tx_errors = priv->stats.tx_err_pkt_cnt;
> +	stats->rx_packets = priv->stats.rx_pkt_cnt;
> +	stats->rx_errors = priv->stats.rx_err_pkt_cnt;
> +}
> +
>   static int dp83td510_config_intr(struct phy_device *phydev)
>   {
>   	int ret;
> @@ -588,7 +682,7 @@ static struct phy_driver dp83td510_driver[] = {
>   	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
>   	.name		= "TI DP83TD510E",
>   
> -	.flags          = PHY_POLL_CABLE_TEST,
> +	.flags          = PHY_POLL_CABLE_TEST | PHY_POLL_STATS,
>   	.probe		= dp83td510_probe,
>   	.config_aneg	= dp83td510_config_aneg,
>   	.read_status	= dp83td510_read_status,
> @@ -599,6 +693,8 @@ static struct phy_driver dp83td510_driver[] = {
>   	.get_sqi_max	= dp83td510_get_sqi_max,
>   	.cable_test_start = dp83td510_cable_test_start,
>   	.cable_test_get_status = dp83td510_cable_test_get_status,
> +	.get_phy_stats	= dp83td510_get_phy_stats,
> +	.update_stats	= dp83td510_update_stats,
>   
>   	.suspend	= genphy_suspend,
>   	.resume		= genphy_resume,