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)
+
+#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)
+
+#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.
+ */
+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;
+ 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,
--
2.39.5On Tue, Dec 03, 2024 at 08:56:20AM +0100, 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) > + > +#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) > + > +#define DP83TD510E_PKT_STAT_6 0x130 > +#define DP83TD510E_RX_ERR_PKT_CNT_MASK GENMASK(15, 0) I'm not sure I like this pattern of _MASK here. Why not call these registers e.g. DP83TD510E_RX_PKT_CNT_31_16 ? Given that the full register value is used, I don't see the need for _MASK and the FIELD_GET()s, which just add extra complexity to the code and reduce readability. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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,
On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
>
>
> 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
Yes, the name may be a bit misleading...
[...]
> > + */
> > +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)
No. This would not be the same.
The current code takes the lower 16 bit of "ret" and shifts it left 16
bits.
As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.
DP83TD510E_PKT_STAT_1 gives 0x????aaaa
DP83TD510E_PKT_STAT_2 gives 0x????bbbb
count will be 0xbbbbaaaa
This raises another question: Are these values latched?
If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
unlatched MMIO busses you first read the upper part, then the lower,
then the upper again and loop if the value of the upper part changed in
between. Not sure how much overhead this means for the slow busses.
Consult the doc of the chip if you can read both in one go and if the
chip latches these values for that access mode.
> instead of shifting, what do you think ?
nope - If you don't want to shift, you can use a combination of
FIELD_GET() (to extract the relevant 16 bits) and FIELD_PREP() to shift.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On Thu, Dec 05, 2024 at 10:01:10AM +0100, Marc Kleine-Budde wrote:
> On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
> >
> >
> > 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
>
> Yes, the name may be a bit misleading...
The naming is done according to the chip datasheet. This is preferable
way to name defines.
> [...]
>
> > > + */
> > > +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.
> > > + */
this comment is relevant for the following question by Marc.
> > > + 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)
>
> No. This would not be the same.
>
> The current code takes the lower 16 bit of "ret" and shifts it left 16
> bits.
>
> As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
> 16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.
>
> DP83TD510E_PKT_STAT_1 gives 0x????aaaa
> DP83TD510E_PKT_STAT_2 gives 0x????bbbb
>
> count will be 0xbbbbaaaa
>
> This raises another question: Are these values latched?
>
> If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
> unlatched MMIO busses you first read the upper part, then the lower,
> then the upper again and loop if the value of the upper part changed in
> between. Not sure how much overhead this means for the slow busses.
>
> Consult the doc of the chip if you can read both in one go and if the
> chip latches these values for that access mode.
It is not documented, what is documented is that PKT_STAT_1 to
PKT_STAT_3 should be read in sequence to trigger auto clear function of
this registers. If chip do not latches these values, we will have
additional problem - some counts will be lost in the PKT_STAT_1/2 till we with
PKT_STAT_3 will be done.
With other words, I'll do more testing and add corresponding comments in
the code..
--
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 |
On 12/5/2024 10:01 AM, Marc Kleine-Budde wrote:
> On 05.12.2024 09:43:34, Mateusz Polchlopek wrote:
>>
>>
>> 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
>
> Yes, the name may be a bit misleading...
>
> [...]
>
>>> + */
>>> +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)
>
> No. This would not be the same.
>
> The current code takes the lower 16 bit of "ret" and shifts it left 16
> bits.
>
> As far as I understand the code DP83TD510E_PKT_STAT_1 contain the lower
> 16 bits, while DP83TD510E_PKT_STAT_2 contain the upper 16 bits.
>
> DP83TD510E_PKT_STAT_1 gives 0x????aaaa
> DP83TD510E_PKT_STAT_2 gives 0x????bbbb
>
> count will be 0xbbbbaaaa
>
> This raises another question: Are these values latched?
>
> If not you can get funny results if DP83TD510E_PKT_STAT_1 rolls over. On
> unlatched MMIO busses you first read the upper part, then the lower,
> then the upper again and loop if the value of the upper part changed in
> between. Not sure how much overhead this means for the slow busses.
>
> Consult the doc of the chip if you can read both in one go and if the
> chip latches these values for that access mode.
>
>> instead of shifting, what do you think ?
>
> nope - If you don't want to shift, you can use a combination of
> FIELD_GET() (to extract the relevant 16 bits) and FIELD_PREP() to shift.
>
> regards,
> Marc
>
Okay, thanks Marc for an explanation! Now I understand it better
© 2016 - 2026 Red Hat, Inc.