[PATCH net-next] net: phy: aquantia: add SGMII statistics

[PATCH net-next] net: phy: aquantia: add SGMII statistics

[Heiner Kallweit]

The AQR107 family has SGMII statistics counters. Let's expose them to
ethtool. To interpret the counters correctly one has to be aware that
rx on SGMII side is tx on ethernet side. The counters are populated
by the chip in 100Mbps/1Gbps mode only.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_main.c | 106 +++++++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index be5204a1f..8ab4c78dd 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -69,6 +69,18 @@
 #define MDIO_AN_RX_VEND_STAT3			0xe832
 #define MDIO_AN_RX_VEND_STAT3_AFR		BIT(0)
 
+/* MDIO_MMD_C22EXT */
+#define MDIO_C22EXT_STAT_SGMII_RX_GOOD_FRAMES		0xd292
+#define MDIO_C22EXT_STAT_SGMII_RX_BAD_FRAMES		0xd294
+#define MDIO_C22EXT_STAT_SGMII_RX_FALSE_CARRIER		0xd297
+#define MDIO_C22EXT_STAT_SGMII_TX_GOOD_FRAMES		0xd313
+#define MDIO_C22EXT_STAT_SGMII_TX_BAD_FRAMES		0xd315
+#define MDIO_C22EXT_STAT_SGMII_TX_FALSE_CARRIER		0xd317
+#define MDIO_C22EXT_STAT_SGMII_TX_COLLISIONS		0xd318
+#define MDIO_C22EXT_STAT_SGMII_TX_LINE_COLLISIONS	0xd319
+#define MDIO_C22EXT_STAT_SGMII_TX_FRAME_ALIGN_ERR	0xd31a
+#define MDIO_C22EXT_STAT_SGMII_TX_RUNT_FRAMES		0xd31b
+
 /* Vendor specific 1, MDIO_MMD_VEND1 */
 #define VEND1_GLOBAL_FW_ID			0x0020
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
@@ -108,6 +120,79 @@
 #define VEND1_GLOBAL_INT_VEND_MASK_GLOBAL2	BIT(1)
 #define VEND1_GLOBAL_INT_VEND_MASK_GLOBAL3	BIT(0)
 
+struct aqr107_hw_stat {
+	const char *name;
+	int reg;
+	int size;
+};
+
+#define SGMII_STAT(n, r, s) { n, MDIO_C22EXT_STAT_SGMII_ ## r, s }
+static const struct aqr107_hw_stat aqr107_hw_stats[] = {
+	SGMII_STAT("sgmii_rx_good_frames",	    RX_GOOD_FRAMES,	26),
+	SGMII_STAT("sgmii_rx_bad_frames",	    RX_BAD_FRAMES,	26),
+	SGMII_STAT("sgmii_rx_false_carrier_events", RX_FALSE_CARRIER,	 8),
+	SGMII_STAT("sgmii_tx_good_frames",	    TX_GOOD_FRAMES,	26),
+	SGMII_STAT("sgmii_tx_bad_frames",	    TX_BAD_FRAMES,	26),
+	SGMII_STAT("sgmii_tx_false_carrier_events", TX_FALSE_CARRIER,	 8),
+	SGMII_STAT("sgmii_tx_collisions",	    TX_COLLISIONS,	 8),
+	SGMII_STAT("sgmii_tx_line_collisions",	    TX_LINE_COLLISIONS,	 8),
+	SGMII_STAT("sgmii_tx_frame_alignment_err",  TX_FRAME_ALIGN_ERR,	16),
+	SGMII_STAT("sgmii_tx_runt_frames",	    TX_RUNT_FRAMES,	22),
+};
+
+static int aqr107_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(aqr107_hw_stats);
+}
+
+static void aqr107_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(aqr107_hw_stats); i++)
+		strscpy(data + i * ETH_GSTRING_LEN, aqr107_hw_stats[i].name,
+			ETH_GSTRING_LEN);
+}
+
+static u64 aqr107_get_stat(struct phy_device *phydev, int index)
+{
+	const struct aqr107_hw_stat *stat = aqr107_hw_stats + index;
+	int len_l = min(stat->size, 16);
+	int len_h = stat->size - len_l;
+	u64 ret;
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg);
+	if (val < 0) {
+		phydev_err(phydev, "Reading HW Statistics failed\n");
+		return 0;
+	}
+
+	ret = val & GENMASK(len_l - 1, 0);
+	if (len_h) {
+		val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg + 1);
+		if (val < 0) {
+			phydev_err(phydev, "Reading HW Statistics failed\n");
+			return 0;
+		}
+		ret += (val & GENMASK(len_h - 1, 0)) << 16;
+	}
+
+	return ret;
+}
+
+static void aqr107_get_stats(struct phy_device *phydev,
+			     struct ethtool_stats *stats, u64 *data)
+{
+	u64 *pstats = phydev->priv;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(aqr107_hw_stats); i++) {
+		*pstats += aqr107_get_stat(phydev, i);
+		*data++ = *pstats++;
+	}
+}
+
 static int aqr_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;
@@ -490,6 +575,17 @@ static int aqr107_resume(struct phy_device *phydev)
 				  MDIO_CTRL1_LPOWER);
 }
 
+static int aqr107_probe(struct phy_device *phydev)
+{
+	phydev->priv = devm_kcalloc(&phydev->mdio.dev,
+				    ARRAY_SIZE(aqr107_hw_stats),
+				    sizeof(u64), GFP_KERNEL);
+	if (!phydev->priv)
+		return -ENOMEM;
+
+	return aqr_hwmon_probe(phydev);
+}
+
 static struct phy_driver aqr_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -536,7 +632,7 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQR107",
 	.aneg_done	= genphy_c45_aneg_done,
 	.get_features	= genphy_c45_pma_read_abilities,
-	.probe		= aqr_hwmon_probe,
+	.probe		= aqr107_probe,
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -546,6 +642,9 @@ static struct phy_driver aqr_driver[] = {
 	.set_tunable    = aqr107_set_tunable,
 	.suspend	= aqr107_suspend,
 	.resume		= aqr107_resume,
+	.get_sset_count	= aqr107_get_sset_count,
+	.get_strings	= aqr107_get_strings,
+	.get_stats	= aqr107_get_stats,
 	.link_change_notify = aqr107_link_change_notify,
 },
 {
@@ -553,7 +652,7 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQCS109",
 	.aneg_done	= genphy_c45_aneg_done,
 	.get_features	= genphy_c45_pma_read_abilities,
-	.probe		= aqr_hwmon_probe,
+	.probe		= aqr107_probe,
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
@@ -563,6 +662,9 @@ static struct phy_driver aqr_driver[] = {
 	.set_tunable    = aqr107_set_tunable,
 	.suspend	= aqr107_suspend,
 	.resume		= aqr107_resume,
+	.get_sset_count	= aqr107_get_sset_count,
+	.get_strings	= aqr107_get_strings,
+	.get_stats	= aqr107_get_stats,
 	.link_change_notify = aqr107_link_change_notify,
 },
 {
-- 
2.21.0

Re: [PATCH net-next] net: phy: aquantia: add SGMII statistics

[Andrew Lunn]

> +static u64 aqr107_get_stat(struct phy_device *phydev, int index)
> +{
> +	const struct aqr107_hw_stat *stat = aqr107_hw_stats + index;
> +	int len_l = min(stat->size, 16);
> +	int len_h = stat->size - len_l;
> +	u64 ret;
> +	int val;
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg);
> +	if (val < 0) {
> +		phydev_err(phydev, "Reading HW Statistics failed\n");
> +		return 0;
> +	}
> +
> +	ret = val & GENMASK(len_l - 1, 0);
> +	if (len_h) {
> +		val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg + 1);
> +		if (val < 0) {
> +			phydev_err(phydev, "Reading HW Statistics failed\n");
> +			return 0;

Hi Heiner

When things go wrong, it seems to be reasonably normal to return
U64_MAX, not zero. It is such a large value that is raises questions,
where as 0 might be considered a correct value, not an error.

> +static void aqr107_get_stats(struct phy_device *phydev,
> +			     struct ethtool_stats *stats, u64 *data)
> +{
> +	u64 *pstats = phydev->priv;

This seems like a trap waiting for somebody to fall into.

It would be more future proof to define a struct which just contains
an array. 

   Andrew

Re: [PATCH net-next] net: phy: aquantia: add SGMII statistics

[Heiner Kallweit]

On 31.03.2019 16:45, Andrew Lunn wrote:
>> +static u64 aqr107_get_stat(struct phy_device *phydev, int index)
>> +{
>> +	const struct aqr107_hw_stat *stat = aqr107_hw_stats + index;
>> +	int len_l = min(stat->size, 16);
>> +	int len_h = stat->size - len_l;
>> +	u64 ret;
>> +	int val;
>> +
>> +	val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg);
>> +	if (val < 0) {
>> +		phydev_err(phydev, "Reading HW Statistics failed\n");
>> +		return 0;
>> +	}
>> +
>> +	ret = val & GENMASK(len_l - 1, 0);
>> +	if (len_h) {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg + 1);
>> +		if (val < 0) {
>> +			phydev_err(phydev, "Reading HW Statistics failed\n");
>> +			return 0;
> 
> Hi Heiner
> 
> When things go wrong, it seems to be reasonably normal to return
> U64_MAX, not zero. It is such a large value that is raises questions,
> where as 0 might be considered a correct value, not an error.
> 
>> +static void aqr107_get_stats(struct phy_device *phydev,
>> +			     struct ethtool_stats *stats, u64 *data)
>> +{
>> +	u64 *pstats = phydev->priv;
> 
> This seems like a trap waiting for somebody to fall into.
> 
> It would be more future proof to define a struct which just contains
> an array. 
> 
Both good points. I'll cook a v2.

>    Andrew
> 
Heiner