Messages in this thread |  | | Date | Thu, 5 Dec 2024 11:32:10 +0100 | | Subject | Re: [PATCH net-next v1 6/7] phy: dp83td510: add statistics support | | From | Mateusz Polchlopek <> |
| |
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
|  |