Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
> So, I have to apologize again here. > I guess I was an idiot/n00b about this. Not a problem. If it is not something you have come across before, you can easily miss the significance. So you just need to modify the ordering and you are good to go. Please add a comment in the code about this latching. We don't want somebody changing the ordering and breaking it. Thanks Andrew
Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
On Tue, 2019-08-13 at 08:48 +0300, Alexandru Ardelean wrote:
> On Mon, 2019-08-12 at 16:33 +0200, Andrew Lunn wrote:
> > [External]
> >
> > > +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> > > +struct adin_hw_stat *stat,
> > > +u32 *val)
> > > +{
> > > + int ret;
> > > +
> > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val = (ret & 0x);
> > > +
> > > + if (stat->reg2 == 0)
> > > + return 0;
> > > +
> > > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val <<= 16;
> > > + *val |= (ret & 0x);
> > > +
> > > + return 0;
> > > +}
> >
> > It still looks like you have not dealt with overflow from the LSB into
> > the MSB between the two reads.
>
> Apologies for forgetting to address this.
> I did not intentionally leave it out; this item got lost after V1 [which had
> the most remarks].
> Changelog V1 -> V2 was quite bulky, and I did not look at V1 remarks after I
> finished V2.
>
> Thanks for snippet.
So, I have to apologize again here.
I guess I was an idiot/n00b about this.
The PHY stats do support snapshot, and I sync-ed with someone from the
chip-team to confirm.
Also, from the datasheet[1] (page 29 - FRAME GENERATOR AND CHECKER - 5th
paragraph):
--
The frame checker counts the number of CRC errors and these
are reported in the receive error counter register (RxErrCnt
register, address 0x0014). To ensure synchronization between
the frame checker error counter and frame checker frame counters,
all of the counters are latched once the receive error counter
register is read. Hence when using the frame checker, the
receive error counter should be read first and then all the other
frame counters and error counters should be read. A latched copy
of the receive frame counter register is available in the
FcFrmCntH and FcFrmCntL registers (register addresses 0x1E.0x940A
and 0x1E.0x940B).
-
Then in the description of these regs, it mentions (repeteadly):
-
This register is a latched copy of bits 31:16 of the 32-bit
receive frame counter register. When the receive error counter
(RxErrCnt register address 0x0014) is read, the receive
frame
counter register is latched. A copy of the receive frame counter
register is latched when RxErrCnt is read so that
the error count
and receive frame count are synchronized
-
I'll re-spin this with the rename of the strings, and maybe do a minor polish
of the code.
Thanks & sorry for the noise/trouble
Alex
[1]
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
>
> > do {
> > hi1 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > if (hi1 < 0)
> > return hi1;
> >
> > low = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > if (low < 0)
> > return low;
> >
> > hi2 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > if (hi2 < 0)
> > return hi1;
> > } while (hi1 != hi2)
> >
> > return low | (hi << 16);
> >
> > Andrew
Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
On Mon, 2019-08-12 at 16:26 +0200, Andrew Lunn wrote:
> [External]
>
> > +/* Named just like in the datasheet */
> > +static struct adin_hw_stat adin_hw_stats[] = {
> > + { "RxErrCnt", 0x0014, },
> > + { "MseA", 0x8402, 0, true },
> > + { "MseB", 0x8403, 0, true },
> > + { "MseC", 0x8404, 0, true },
> > + { "MseD", 0x8405, 0, true },
> > + { "FcFrmCnt", 0x940A, 0x940B }, /* FcFrmCntH + FcFrmCntL */
> > + { "FcLenErrCnt",0x940C },
> > + { "FcAlgnErrCnt", 0x940D },
> > + { "FcSymbErrCnt", 0x940E },
> > + { "FcOszCnt", 0x940F },
> > + { "FcUszCnt", 0x9410 },
> > + { "FcOddCnt", 0x9411 },
> > + { "FcOddPreCnt",0x9412 },
> > + { "FcDribbleBitsCnt", 0x9413 },
> > + { "FcFalseCarrierCnt", 0x9414 },
>
> I see some value in using the names from the datasheet. However, i
> found it quite hard to now what these counters represent given there
> current name. What is Mse? How does MseA differ from MseB? You have up
> to ETH_GSTRING_LEN characters, so maybe longer names would be better?
I'll expand the names.
Regarding MseA/B/C/D, I'll admit I am also a bit fuzzy about them.
They describe link-quality settings, and the values have some meaning to the
chip guys [when I talked witht them about
it], but I did not insist on getting a deep explanation about them [and what
their values represent].
I guess for this PHY driver, we could drop them, and if they're needed they can
be accessed via phytool, and if they're
really needed, I can try to add them later with more complete detail [about
them and their use/value].
I included them here, because they are listed in the error-counter register
"group" [in the datasheet], and I inertially
added them.
>
>Andrew
Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
On Mon, 2019-08-12 at 16:33 +0200, Andrew Lunn wrote:
> [External]
>
> > +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> > + struct adin_hw_stat *stat,
> > + u32 *val)
> > +{
> > + int ret;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = (ret & 0x);
> > +
> > + if (stat->reg2 == 0)
> > + return 0;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val <<= 16;
> > + *val |= (ret & 0x);
> > +
> > + return 0;
> > +}
>
> It still looks like you have not dealt with overflow from the LSB into
> the MSB between the two reads.
Apologies for forgetting to address this.
I did not intentionally leave it out; this item got lost after V1 [which had
the most remarks].
Changelog V1 -> V2 was quite bulky, and I did not look at V1 remarks after I
finished V2.
Thanks for snippet.
>
> do {
> hi1 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> if (hi1 < 0)
> return hi1;
>
> low = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> if (low < 0)
> return low;
>
> hi2 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> if (hi2 < 0)
> return hi1;
> } while (hi1 != hi2)
>
> return low | (hi << 16);
>
> Andrew
Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
> +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> +struct adin_hw_stat *stat,
> +u32 *val)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> + if (ret < 0)
> + return ret;
> +
> + *val = (ret & 0x);
> +
> + if (stat->reg2 == 0)
> + return 0;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> + if (ret < 0)
> + return ret;
> +
> + *val <<= 16;
> + *val |= (ret & 0x);
> +
> + return 0;
> +}
It still looks like you have not dealt with overflow from the LSB into
the MSB between the two reads.
do {
hi1 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
if (hi1 < 0)
return hi1;
low = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
if (low < 0)
return low;
hi2 = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
if (hi2 < 0)
return hi1;
} while (hi1 != hi2)
return low | (hi << 16);
Andrew
Re: [PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
> +/* Named just like in the datasheet */
> +static struct adin_hw_stat adin_hw_stats[] = {
> + { "RxErrCnt", 0x0014, },
> + { "MseA", 0x8402, 0, true },
> + { "MseB", 0x8403, 0, true },
> + { "MseC", 0x8404, 0, true },
> + { "MseD", 0x8405, 0, true },
> + { "FcFrmCnt", 0x940A, 0x940B }, /* FcFrmCntH + FcFrmCntL */
> + { "FcLenErrCnt",0x940C },
> + { "FcAlgnErrCnt", 0x940D },
> + { "FcSymbErrCnt", 0x940E },
> + { "FcOszCnt", 0x940F },
> + { "FcUszCnt", 0x9410 },
> + { "FcOddCnt", 0x9411 },
> + { "FcOddPreCnt",0x9412 },
> + { "FcDribbleBitsCnt", 0x9413 },
> + { "FcFalseCarrierCnt", 0x9414 },
I see some value in using the names from the datasheet. However, i
found it quite hard to now what these counters represent given there
current name. What is Mse? How does MseA differ from MseB? You have up
to ETH_GSTRING_LEN characters, so maybe longer names would be better?
Andrew
[PATCH v4 13/14] net: phy: adin: add ethtool get_stats support
This change implements retrieving all the error counters from the PHY.
The PHY supports several error counters/stats. The `Mean Square Errors`
status values are only valie when a link is established, and shouldn't be
accumulated. These values characterize the quality of a signal.
The rest of the error counters are self-clearing on read.
Most of them are reports from the Frame Checker engine that the PHY has.
Not retrieving the `LPI Wake Error Count Register` here, since that is used
by the PHY framework to check for any EEE errors. And that register is
self-clearing when read (as per IEEE spec).
Signed-off-by: Alexandru Ardelean
---
drivers/net/phy/adin.c | 109 +
1 file changed, 109 insertions(+)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index e4afa8c2bec7..3ab15a585c1b 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -152,12 +152,40 @@ static struct adin_clause45_mmd_map
adin_clause45_mmd_map[] = {
{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,ADIN1300_LPI_WAKE_ERR_CNT_REG },
};
+struct adin_hw_stat {
+ const char *string;
+ u16 reg1;
+ u16 reg2;
+ bool do_not_accumulate;
+};
+
+/* Named just like in the datasheet */
+static struct adin_hw_stat adin_hw_stats[] = {
+ { "RxErrCnt", 0x0014, },
+ { "MseA", 0x8402, 0, true },
+ { "MseB", 0x8403, 0, true },
+ { "MseC", 0x8404, 0, true },
+ { "MseD", 0x8405, 0, true },
+ { "FcFrmCnt", 0x940A, 0x940B }, /* FcFrmCntH + FcFrmCntL */
+ { "FcLenErrCnt",0x940C },
+ { "FcAlgnErrCnt", 0x940D },
+ { "FcSymbErrCnt", 0x940E },
+ { "FcOszCnt", 0x940F },
+ { "FcUszCnt", 0x9410 },
+ { "FcOddCnt", 0x9411 },
+ { "FcOddPreCnt",0x9412 },
+ { "FcDribbleBitsCnt", 0x9413 },
+ { "FcFalseCarrierCnt", 0x9414 },
+};
+
/**
* struct adin_priv - ADIN PHY driver private data
* edpd_enabledtrue if Energy Detect Powerdown mode is enabled
+ * stats statistic counters for the PHY
*/
struct adin_priv {
booledpd_enabled;
+ u64 stats[ARRAY_SIZE(adin_hw_stats)];
};
static int adin_lookup_reg_value(const struct adin_cfg_reg_map *tbl, int cfg)
@@ -590,6 +618,81 @@ static int adin_reset(struct phy_device *phydev)
return adin_subsytem_soft_reset(phydev);
}
+static int adin_get_sset_count(struct phy_device *phydev)
+{
+ return ARRAY_SIZE(adin_hw_stats);
+}
+
+static void adin_get_strings(struct phy_device *phydev, u8 *data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
+ strlcpy([i * ETH_GSTRING_LEN],
+ adin_hw_stats[i].string, ETH_GSTRING_LEN);
+ }
+}
+
+static int adin_read_mmd_stat_regs(struct phy_device *phydev,
+ struct adin_hw_stat *stat,
+ u32 *val)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
+ if (ret < 0)
+ return ret;
+
+ *val = (ret & 0x);
+
+ if (stat->reg2 == 0)
+ return 0;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
+ if (ret < 0)
+ return ret;
+
+ *val <<= 16;
+ *val |= (ret & 0x);
+
+ return 0;
+}
+
+static u64 adin_get_stat(struct phy_device *phydev, int i)
+{
+ struct adin_hw_stat *stat = _hw_stats[i];
+ struct adin_priv *priv = phydev->priv;
+ u32 val;
+ int ret;
+
+ if (stat->reg1 > 0x1f) {
+ ret = adin_read_mmd_stat_regs(phydev, stat, );
+ if (ret < 0)
+ return (u64)(~0);
+ } else {
+ ret = phy_read(phydev, stat->reg1);
+ if (ret < 0)
+ return (u64)(~0);
+ val = (ret & 0x);
+ }
+
+ if (stat->do_not_accumulate)
+ priv->stats[i] = val;
+ else
+ priv->stats[i] += val;
+
+ return priv->stats[i];
+}
+
+static void adin_get_stats(struct phy_device *phydev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++)
+ data[i] = adin_get_stat(phydev, i);
+}
+
static int adin_probe(struct phy_device *phydev)
{
struct device *dev = >mdio.dev;
@@ -618,6 +721,9 @@ static struct phy_driver adin_driver[] = {
.set_tunable= adin_set_tunable,
.ack_interrupt = adin_phy_ack_intr,
.config_intr= adin_phy_config_intr,
+ .get_sset_count = adin_get_sset_count,
+ .get_strings= adin_get_strings,
+ .get_stats = adin_get_stats,
