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@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...
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 |
