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

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

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