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@xxxxxxxxxxxxxx>
> > ---
> > 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 |
Attachment:
signature.asc
Description: PGP signature
