[PATCH v1 net-next 0/4] net: dsa: microchip: add MIB counters support

[PATCH v1 net-next 0/4] net: dsa: microchip: add MIB counters support

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

This series of patches is to modify the KSZ9477 DSA driver to read MIB
counters periodically to avoid overflow.

v1
- Use readx_poll_time
- Do not clear MIB counters when port is enabled
- Do not advertise 1000 half-duplex mode when port is enabled
- Do not use freeze function as MIB counters may miss counts

Tristram Ha (4):
  net: dsa: microchip: prepare PHY for proper advertisement
  net: dsa: microchip: add MIB counter reading support
  net: dsa: microchip: use readx_poll_time for polling
  net: dsa: microchip: remove unnecessary include headers

 drivers/net/dsa/microchip/ksz9477.c    | 249 ++++++++++++++++++---------------
 drivers/net/dsa/microchip/ksz_common.c | 115 ++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |   6 +-
 drivers/net/dsa/microchip/ksz_priv.h   |  11 +-
 4 files changed, 264 insertions(+), 117 deletions(-)

-- 
1.9.1

[PATCH v1 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Prepare PHY for proper advertisement and get link status for the port.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 17 ++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.c | 19 ++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  4 +++-
 drivers/net/dsa/microchip/ksz_priv.h   |  4 +++-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 89ed059..0fdb22d 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 switch driver main logic
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -965,6 +965,19 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
+static void ksz9477_phy_setup(struct ksz_device *dev, int port,
+			      struct phy_device *phy)
+{
+	/* ETHTOOL_LINK_MODE_Pause_BIT and ETHTOOL_LINK_MODE_Asym_Pause_BIT
+	 * can be removed to disable flow control when rate limiting is used.
+	 */
+	if (port < dev->phy_port_cnt) {
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(phy,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+	}
+}
+
 static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
 	u8 data8;
@@ -1151,6 +1164,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	.setup			= ksz9477_setup,
 	.phy_read		= ksz9477_phy_read16,
 	.phy_write		= ksz9477_phy_write16,
+	.adjust_link		= ksz_adjust_link,
 	.port_enable		= ksz_enable_port,
 	.port_disable		= ksz_disable_port,
 	.get_strings		= ksz9477_get_strings,
@@ -1298,6 +1312,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
+	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.shutdown = ksz9477_reset_switch,
 	.detect = ksz9477_switch_detect,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8a5111f..a57bda7 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2,7 +2,7 @@
 /*
  * Microchip switch driver main logic
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -61,6 +61,22 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
 }
 EXPORT_SYMBOL_GPL(ksz_phy_write16);
 
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p = &dev->ports[port];
+
+	if (phydev->link) {
+		dev->live_ports |= (1 << port) & dev->on_ports;
+	} else if (p->phydev.link) {
+		p->link_just_down = 1;
+		dev->live_ports &= ~(1 << port);
+	}
+	p->phydev = *phydev;
+}
+EXPORT_SYMBOL_GPL(ksz_adjust_link);
+
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
 {
 	struct ksz_device *dev = ds->priv;
@@ -238,6 +254,7 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
+	dev->dev_ops->phy_setup(dev, port, phy);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 2dd832d..6d49319 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0
  * Microchip switch driver common header
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_COMMON_H
@@ -13,6 +13,8 @@
 
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
 int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev);
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br);
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 60b4901..0fdc58b 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -2,7 +2,7 @@
  *
  * Microchip KSZ series switch common definitions
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_PRIV_H
@@ -137,6 +137,8 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
+	void (*phy_setup)(struct ksz_device *dev, int port,
+			  struct phy_device *phy);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 	void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-- 
1.9.1

[PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Add MIB counter reading support.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 139 +++++++++++++++++++++++----------
 drivers/net/dsa/microchip/ksz_common.c |  96 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   2 +
 drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
 4 files changed, 198 insertions(+), 46 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 0fdb22d..4502e13 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -10,6 +10,7 @@
 #include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/iopoll.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
@@ -18,8 +19,8 @@
 #include <net/switchdev.h>
 
 #include "ksz_priv.h"
-#include "ksz_common.h"
 #include "ksz9477_reg.h"
+#include "ksz_common.h"
 
 static const struct {
 	int index;
@@ -92,6 +93,27 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, addr, data);
 }
 
+#define read8_op(addr)	\
+({ \
+	u8 data; \
+	ksz_read8(dev, addr, &data); \
+	data; \
+})
+
+#define read32_op(addr)	\
+({ \
+	u32 data; \
+	ksz_read32(dev, addr, &data); \
+	data; \
+})
+
+#define pread32_op(addr)	\
+({ \
+	u32 data; \
+	ksz_pread32(dev, port, addr, &data); \
+	data; \
+})
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32 waiton,
 					int timeout)
 {
@@ -259,6 +281,70 @@ static int ksz9477_reset_switch(struct ksz_device *dev)
 	return 0;
 }
 
+static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
+			      u64 *cnt)
+{
+	u32 data;
+	int ret;
+	struct ksz_port *p = &dev->ports[port];
+
+	/* retain the flush/freeze bit */
+	data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+	data |= MIB_COUNTER_READ;
+	data |= (addr << MIB_COUNTER_INDEX_S);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
+
+	ret = readx_poll_timeout(pread32_op, REG_PORT_MIB_CTRL_STAT__4, data,
+				 !(data & MIB_COUNTER_READ), 10, 1000);
+
+	/* failed to read MIB. get out of loop */
+	if (ret < 0) {
+		dev_dbg(dev->dev, "Failed to get MIB\n");
+		return;
+	}
+
+	/* count resets upon read */
+	ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
+	*cnt += data;
+}
+
+static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
+			      u64 *dropped, u64 *cnt)
+{
+	addr = ksz9477_mib_names[addr].index;
+	ksz9477_r_mib_cnt(dev, port, addr, cnt);
+}
+
+static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
+{
+	struct ksz_port *p = &dev->ports[port];
+	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+
+	/* enable/disable the port for flush/freeze function */
+	mutex_lock(&p->mib.cnt_mutex);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, val);
+
+	/* used by MIB counter reading code to know freeze is enabled */
+	p->freeze = freeze;
+	mutex_unlock(&p->mib.cnt_mutex);
+}
+
+static void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
+{
+	struct ksz_port_mib *mib = &dev->ports[port].mib;
+
+	/* flush all enabled port MIB counters */
+	mutex_lock(&mib->cnt_mutex);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
+		     MIB_COUNTER_FLUSH_FREEZE);
+	ksz_write8(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FLUSH);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, 0);
+	mutex_unlock(&mib->cnt_mutex);
+
+	mib->cnt_ptr = 0;
+	memset(mib->counters, 0, dev->mib_cnt * sizeof(u64));
+}
+
 static enum dsa_tag_protocol ksz9477_get_tag_protocol(struct dsa_switch *ds,
 						      int port)
 {
@@ -342,47 +428,6 @@ static void ksz9477_get_strings(struct dsa_switch *ds, int port,
 	}
 }
 
-static void ksz_get_ethtool_stats(struct dsa_switch *ds, int port,
-				  uint64_t *buf)
-{
-	struct ksz_device *dev = ds->priv;
-	int i;
-	u32 data;
-	int timeout;
-
-	mutex_lock(&dev->stats_mutex);
-
-	for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {
-		data = MIB_COUNTER_READ;
-		data |= ((ksz9477_mib_names[i].index & 0xFF) <<
-			MIB_COUNTER_INDEX_S);
-		ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
-
-		timeout = 1000;
-		do {
-			ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
-				    &data);
-			usleep_range(1, 10);
-			if (!(data & MIB_COUNTER_READ))
-				break;
-		} while (timeout-- > 0);
-
-		/* failed to read MIB. get out of loop */
-		if (!timeout) {
-			dev_dbg(dev->dev, "Failed to get MIB\n");
-			break;
-		}
-
-		/* count resets upon read */
-		ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
-
-		dev->mib_value[i] += (uint64_t)data;
-		buf[i] = dev->mib_value[i];
-	}
-
-	mutex_unlock(&dev->stats_mutex);
-}
-
 static void ksz9477_cfg_port_member(struct ksz_device *dev, int port,
 				    u8 member)
 {
@@ -1153,9 +1198,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	/* queue based egress rate limit */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_5, SW_OUT_RATE_LIMIT_QUEUE_BASED, true);
 
+	/* enable global MIB counter freeze function */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
+
 	/* start switch */
 	ksz_cfg(dev, REG_SW_OPERATION, SW_START, true);
 
+	ksz_init_mib_timer(dev);
+
 	return 0;
 }
 
@@ -1290,6 +1340,7 @@ static int ksz9477_switch_init(struct ksz_device *dev)
 	if (!dev->ports)
 		return -ENOMEM;
 	for (i = 0; i < dev->mib_port_cnt; i++) {
+		mutex_init(&dev->ports[i].mib.cnt_mutex);
 		dev->ports[i].mib.counters =
 			devm_kzalloc(dev->dev,
 				     sizeof(u64) *
@@ -1314,6 +1365,10 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
 	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
+	.r_mib_cnt = ksz9477_r_mib_cnt,
+	.r_mib_pkt = ksz9477_r_mib_pkt,
+	.freeze_mib = ksz9477_freeze_mib,
+	.port_init_cnt = ksz9477_port_init_cnt,
 	.shutdown = ksz9477_reset_switch,
 	.detect = ksz9477_switch_detect,
 	.init = ksz9477_switch_init,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index a57bda7..62d4344 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -40,6 +40,82 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
 }
 EXPORT_SYMBOL_GPL(ksz_update_port_member);
 
+static void port_r_cnt(struct ksz_device *dev, int port)
+{
+	struct ksz_port_mib *mib = &dev->ports[port].mib;
+	u64 *dropped;
+
+	/* Some ports may not have MIB counters before SWITCH_COUNTER_NUM. */
+	while (mib->cnt_ptr < dev->reg_mib_cnt) {
+		dev->dev_ops->r_mib_cnt(dev, port, mib->cnt_ptr,
+					&mib->counters[mib->cnt_ptr]);
+		++mib->cnt_ptr;
+	}
+
+	/* last one in storage */
+	dropped = &mib->counters[dev->mib_cnt];
+
+	/* Some ports may not have MIB counters after SWITCH_COUNTER_NUM. */
+	while (mib->cnt_ptr < dev->mib_cnt) {
+		dev->dev_ops->r_mib_pkt(dev, port, mib->cnt_ptr,
+					dropped, &mib->counters[mib->cnt_ptr]);
+		++mib->cnt_ptr;
+	}
+	mib->cnt_ptr = 0;
+}
+
+static void ksz_mib_read_work(struct work_struct *work)
+{
+	struct ksz_device *dev =
+		container_of(work, struct ksz_device, mib_read);
+	struct ksz_port *p;
+	struct ksz_port_mib *mib;
+	int i;
+
+	for (i = 0; i < dev->mib_port_cnt; i++) {
+		p = &dev->ports[i];
+		if (!p->on)
+			continue;
+		mib = &p->mib;
+		mutex_lock(&mib->cnt_mutex);
+
+		/* read only dropped counters when link is not up */
+		if (p->link_just_down)
+			p->link_just_down = 0;
+		else if (!p->phydev.link)
+			mib->cnt_ptr = dev->reg_mib_cnt;
+		port_r_cnt(dev, i);
+		mutex_unlock(&mib->cnt_mutex);
+	}
+}
+
+static void mib_monitor(struct timer_list *t)
+{
+	struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
+
+	mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
+	schedule_work(&dev->mib_read);
+}
+
+void ksz_init_mib_timer(struct ksz_device *dev)
+{
+	int i;
+
+	/* Read MIB counters every 30 seconds to avoid overflow. */
+	dev->mib_read_interval = msecs_to_jiffies(30000);
+
+	INIT_WORK(&dev->mib_read, ksz_mib_read_work);
+	timer_setup(&dev->mib_read_timer, mib_monitor, 0);
+
+	for (i = 0; i < dev->mib_port_cnt; i++)
+		dev->dev_ops->port_init_cnt(dev, i);
+
+	/* Start the timer 2 seconds later. */
+	dev->mib_read_timer.expires = jiffies + msecs_to_jiffies(2000);
+	add_timer(&dev->mib_read_timer);
+}
+EXPORT_SYMBOL_GPL(ksz_init_mib_timer);
+
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg)
 {
 	struct ksz_device *dev = ds->priv;
@@ -88,6 +164,20 @@ int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
 }
 EXPORT_SYMBOL_GPL(ksz_sset_count);
 
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+
+	mutex_lock(&mib->cnt_mutex);
+	port_r_cnt(dev, port);
+	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
+	mutex_unlock(&mib->cnt_mutex);
+}
+EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
+
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br)
 {
@@ -355,6 +445,12 @@ int ksz_switch_register(struct ksz_device *dev,
 
 void ksz_switch_remove(struct ksz_device *dev)
 {
+	/* timer started */
+	if (dev->mib_read_timer.expires) {
+		del_timer_sync(&dev->mib_read_timer);
+		flush_work(&dev->mib_read);
+	}
+
 	dev->dev_ops->exit(dev);
 	dsa_unregister_switch(dev->ds);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 6d49319..f5f5fd8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -8,6 +8,7 @@
 #define __KSZ_COMMON_H
 
 void ksz_update_port_member(struct ksz_device *dev, int port);
+void ksz_init_mib_timer(struct ksz_device *dev);
 
 /* Common DSA access functions */
 
@@ -16,6 +17,7 @@
 void ksz_adjust_link(struct dsa_switch *ds, int port,
 		     struct phy_device *phydev);
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br);
 void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 0fdc58b..a35bb34 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -14,8 +14,6 @@
 #include <linux/etherdevice.h>
 #include <net/dsa.h>
 
-#include "ksz9477_reg.h"
-
 struct ksz_io_ops;
 
 struct vlan_table {
@@ -23,6 +21,7 @@ struct vlan_table {
 };
 
 struct ksz_port_mib {
+	struct mutex cnt_mutex;		/* structure access */
 	u8 cnt_ptr;
 	u64 *counters;
 };
@@ -39,6 +38,7 @@ struct ksz_port {
 	u32 sgmii:1;			/* port is SGMII */
 	u32 force:1;
 	u32 link_just_down:1;		/* link just goes down */
+	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
 };
@@ -79,8 +79,6 @@ struct ksz_device {
 
 	struct vlan_table *vlan_cache;
 
-	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
 	u8 *txbuf;
 
 	struct ksz_port *ports;
@@ -153,6 +151,7 @@ struct ksz_dev_ops {
 			  u64 *cnt);
 	void (*r_mib_pkt)(struct ksz_device *dev, int port, u16 addr,
 			  u64 *dropped, u64 *cnt);
+	void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze);
 	void (*port_init_cnt)(struct ksz_device *dev, int port);
 	int (*shutdown)(struct ksz_device *dev);
 	int (*detect)(struct ksz_device *dev);
-- 
1.9.1

[PATCH v1 net-next 3/4] net: dsa: microchip: use readx_poll_time for polling

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Replace register polling functions using timeout with readx_poll_time call.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 91 +++++++++++--------------------------
 1 file changed, 27 insertions(+), 64 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4502e13..8391b9e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -114,28 +114,11 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 	data; \
 })
 
-static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32 waiton,
-					int timeout)
-{
-	u8 data;
-
-	do {
-		ksz_read8(dev, REG_SW_VLAN_CTRL, &data);
-		if (!(data & waiton))
-			break;
-		usleep_range(1, 10);
-	} while (timeout-- > 0);
-
-	if (timeout <= 0)
-		return -ETIMEDOUT;
-
-	return 0;
-}
-
 static int ksz9477_get_vlan_table(struct ksz_device *dev, u16 vid,
 				  u32 *vlan_table)
 {
 	int ret;
+	u8 data;
 
 	mutex_lock(&dev->vlan_mutex);
 
@@ -143,7 +126,8 @@ static int ksz9477_get_vlan_table(struct ksz_device *dev, u16 vid,
 	ksz_write8(dev, REG_SW_VLAN_CTRL, VLAN_READ | VLAN_START);
 
 	/* wait to be cleared */
-	ret = ksz9477_wait_vlan_ctrl_ready(dev, VLAN_START, 1000);
+	ret = readx_poll_timeout(read8_op, REG_SW_VLAN_CTRL, data,
+				 !(data & VLAN_START), 10, 1000);
 	if (ret < 0) {
 		dev_dbg(dev->dev, "Failed to read vlan table\n");
 		goto exit;
@@ -165,6 +149,7 @@ static int ksz9477_set_vlan_table(struct ksz_device *dev, u16 vid,
 				  u32 *vlan_table)
 {
 	int ret;
+	u8 data;
 
 	mutex_lock(&dev->vlan_mutex);
 
@@ -176,7 +161,8 @@ static int ksz9477_set_vlan_table(struct ksz_device *dev, u16 vid,
 	ksz_write8(dev, REG_SW_VLAN_CTRL, VLAN_START | VLAN_WRITE);
 
 	/* wait to be cleared */
-	ret = ksz9477_wait_vlan_ctrl_ready(dev, VLAN_START, 1000);
+	ret = readx_poll_timeout(read8_op, REG_SW_VLAN_CTRL, data,
+				 !(data & VLAN_START), 10, 1000);
 	if (ret < 0) {
 		dev_dbg(dev->dev, "Failed to write vlan table\n");
 		goto exit;
@@ -211,42 +197,6 @@ static void ksz9477_write_table(struct ksz_device *dev, u32 *table)
 	ksz_write32(dev, REG_SW_ALU_VAL_D, table[3]);
 }
 
-static int ksz9477_wait_alu_ready(struct ksz_device *dev, u32 waiton,
-				  int timeout)
-{
-	u32 data;
-
-	do {
-		ksz_read32(dev, REG_SW_ALU_CTRL__4, &data);
-		if (!(data & waiton))
-			break;
-		usleep_range(1, 10);
-	} while (timeout-- > 0);
-
-	if (timeout <= 0)
-		return -ETIMEDOUT;
-
-	return 0;
-}
-
-static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev, u32 waiton,
-				      int timeout)
-{
-	u32 data;
-
-	do {
-		ksz_read32(dev, REG_SW_ALU_STAT_CTRL__4, &data);
-		if (!(data & waiton))
-			break;
-		usleep_range(1, 10);
-	} while (timeout-- > 0);
-
-	if (timeout <= 0)
-		return -ETIMEDOUT;
-
-	return 0;
-}
-
 static int ksz9477_reset_switch(struct ksz_device *dev)
 {
 	u8 data8;
@@ -649,7 +599,8 @@ static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_READ | ALU_START);
 
 	/* wait to be finished */
-	ret = ksz9477_wait_alu_ready(dev, ALU_START, 1000);
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_CTRL__4, data,
+				 !(data & ALU_START), 10, 1000);
 	if (ret < 0) {
 		dev_dbg(dev->dev, "Failed to read ALU\n");
 		goto exit;
@@ -673,7 +624,8 @@ static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_WRITE | ALU_START);
 
 	/* wait to be finished */
-	ret = ksz9477_wait_alu_ready(dev, ALU_START, 1000);
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_CTRL__4, data,
+				 !(data & ALU_START), 10, 1000);
 	if (ret < 0)
 		dev_dbg(dev->dev, "Failed to write ALU\n");
 
@@ -706,7 +658,8 @@ static int ksz9477_port_fdb_del(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_READ | ALU_START);
 
 	/* wait to be finished */
-	ret = ksz9477_wait_alu_ready(dev, ALU_START, 1000);
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_CTRL__4, data,
+				 !(data & ALU_START), 10, 1000);
 	if (ret < 0) {
 		dev_dbg(dev->dev, "Failed to read ALU\n");
 		goto exit;
@@ -740,7 +693,8 @@ static int ksz9477_port_fdb_del(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_WRITE | ALU_START);
 
 	/* wait to be finished */
-	ret = ksz9477_wait_alu_ready(dev, ALU_START, 1000);
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_CTRL__4, data,
+				 !(data & ALU_START), 10, 1000);
 	if (ret < 0)
 		dev_dbg(dev->dev, "Failed to write ALU\n");
 
@@ -832,6 +786,7 @@ static void ksz9477_port_mdb_add(struct dsa_switch *ds, int port,
 	u32 static_table[4];
 	u32 data;
 	int index;
+	int ret;
 	u32 mac_hi, mac_lo;
 
 	mac_hi = ((mdb->addr[0] << 8) | mdb->addr[1]);
@@ -847,7 +802,10 @@ static void ksz9477_port_mdb_add(struct dsa_switch *ds, int port,
 		ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
 
 		/* wait to be finished */
-		if (ksz9477_wait_alu_sta_ready(dev, ALU_STAT_START, 1000) < 0) {
+		ret = readx_poll_timeout(read32_op, REG_SW_ALU_STAT_CTRL__4,
+					 data, !(data & ALU_STAT_START),
+					 10, 1000);
+		if (ret < 0) {
 			dev_dbg(dev->dev, "Failed to read ALU STATIC\n");
 			goto exit;
 		}
@@ -888,7 +846,9 @@ static void ksz9477_port_mdb_add(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
 
 	/* wait to be finished */
-	if (ksz9477_wait_alu_sta_ready(dev, ALU_STAT_START, 1000) < 0)
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_STAT_CTRL__4, data,
+				 !(data & ALU_STAT_START), 10, 1000);
+	if (ret < 0)
 		dev_dbg(dev->dev, "Failed to read ALU STATIC\n");
 
 exit:
@@ -918,7 +878,9 @@ static int ksz9477_port_mdb_del(struct dsa_switch *ds, int port,
 		ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
 
 		/* wait to be finished */
-		ret = ksz9477_wait_alu_sta_ready(dev, ALU_STAT_START, 1000);
+		ret = readx_poll_timeout(read32_op, REG_SW_ALU_STAT_CTRL__4,
+					 data, !(data & ALU_STAT_START),
+					 10, 1000);
 		if (ret < 0) {
 			dev_dbg(dev->dev, "Failed to read ALU STATIC\n");
 			goto exit;
@@ -960,7 +922,8 @@ static int ksz9477_port_mdb_del(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_STAT_CTRL__4, data);
 
 	/* wait to be finished */
-	ret = ksz9477_wait_alu_sta_ready(dev, ALU_STAT_START, 1000);
+	ret = readx_poll_timeout(read32_op, REG_SW_ALU_STAT_CTRL__4, data,
+				 !(data & ALU_STAT_START), 10, 1000);
 	if (ret < 0)
 		dev_dbg(dev->dev, "Failed to read ALU STATIC\n");
 
-- 
1.9.1

[PATCH v1 net-next 4/4] net: dsa: microchip: remove unnecessary include headers

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Remove unnecessary header include lines.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8391b9e..7c51edd 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -5,15 +5,11 @@
  * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
-#include <linux/delay.h>
-#include <linux/export.h>
-#include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/iopoll.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
-#include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
-- 
1.9.1

Re: [PATCH v1 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Andrew Lunn]

> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> +			      struct phy_device *phy)
> +{
> +	/* ETHTOOL_LINK_MODE_Pause_BIT and ETHTOOL_LINK_MODE_Asym_Pause_BIT
> +	 * can be removed to disable flow control when rate limiting is used.
> +	 */
> +	if (port < dev->phy_port_cnt) {
> +		/* The MAC actually cannot run in 1000 half-duplex mode. */
> +		phy_remove_link_mode(phy,
> +				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> +	}

Hi Tristram

The comment about pause does not seem to match the code.

> +}
> +
>  static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
>  	u8 data8;
> @@ -1151,6 +1164,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
>  	.setup			= ksz9477_setup,
>  	.phy_read		= ksz9477_phy_read16,
>  	.phy_write		= ksz9477_phy_write16,
> +	.adjust_link		= ksz_adjust_link,
>  	.port_enable		= ksz_enable_port,
>  	.port_disable		= ksz_disable_port,
>  	.get_strings		= ksz9477_get_strings,
> @@ -1298,6 +1312,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
>  	.get_port_addr = ksz9477_get_port_addr,
>  	.cfg_port_member = ksz9477_cfg_port_member,
>  	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
> +	.phy_setup = ksz9477_phy_setup,
>  	.port_setup = ksz9477_port_setup,
>  	.shutdown = ksz9477_reset_switch,
>  	.detect = ksz9477_switch_detect,
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8a5111f..a57bda7 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip switch driver main logic
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
>  #include <linux/delay.h>
> @@ -61,6 +61,22 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  }
>  EXPORT_SYMBOL_GPL(ksz_phy_write16);
>  
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (phydev->link) {
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	} else if (p->phydev.link) {
> +		p->link_just_down = 1;
> +		dev->live_ports &= ~(1 << port);
> +	}

It is not very easy to understand what on_ports, live_ports and
link_just_down are all about. Could you simplify this, and make the
code symmetric? ksz_enable_port does not touch any of these, but
ksz_disable_port does?

grep live_ports *
ksz9477.c:		dev->live_ports = dev->host_mask;
ksz9477.c:				  dev->live_ports |= (1 << port);
ksz_common.c:				  dev->live_ports &= ~(1 << port);
ksz_priv.h:				  u16 live_ports;

So live_ports is not actually used for anything.

> +	p->phydev = *phydev;

This last statement seems like it belongs in phy_setup, which gets
called as part of port_enable.

Re: [PATCH v1 net-next 3/4] net: dsa: microchip: use readx_poll_time for polling

[Andrew Lunn]

On Thu, Feb 07, 2019 at 08:07:08PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Replace register polling functions using timeout with readx_poll_time call.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 91 +++++++++++--------------------------
>  1 file changed, 27 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 4502e13..8391b9e 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -114,28 +114,11 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
>  	data; \
>  })
>  
> -static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32 waiton,
> -					int timeout)
> -{
> -	u8 data;
> -
> -	do {
> -		ksz_read8(dev, REG_SW_VLAN_CTRL, &data);
> -		if (!(data & waiton))
> -			break;
> -		usleep_range(1, 10);
> -	} while (timeout-- > 0);
> -
> -	if (timeout <= 0)
> -		return -ETIMEDOUT;
> -
> -	return 0;

Hi Tristram

I think it would be better to keep ksz9477_wait_vlan_ctrl_ready(),
ksz9477_wait_alu_ready() etc, and change there implementation to use
readx_poll_timeout(). The function names act as better documentation
for what we are waiting for than the parameters being passed to
readx_poll_timeout().

	Andrew

Re: [PATCH v1 net-next 4/4] net: dsa: microchip: remove unnecessary include headers

[Andrew Lunn]

On Thu, Feb 07, 2019 at 08:07:09PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Remove unnecessary header include lines.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Andrew Lunn]

On Thu, Feb 07, 2019 at 08:07:07PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add MIB counter reading support.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c    | 139 +++++++++++++++++++++++----------
>  drivers/net/dsa/microchip/ksz_common.c |  96 +++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.h |   2 +
>  drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
>  4 files changed, 198 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 0fdb22d..4502e13 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -10,6 +10,7 @@
>  #include <linux/gpio.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/iopoll.h>
>  #include <linux/platform_data/microchip-ksz.h>
>  #include <linux/phy.h>
>  #include <linux/etherdevice.h>
> @@ -18,8 +19,8 @@
>  #include <net/switchdev.h>
>  
>  #include "ksz_priv.h"
> -#include "ksz_common.h"
>  #include "ksz9477_reg.h"
> +#include "ksz_common.h"
>  
>  static const struct {
>  	int index;
> @@ -92,6 +93,27 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
>  	ksz_write32(dev, addr, data);
>  }
>  
> +#define read8_op(addr)	\
> +({ \
> +	u8 data; \
> +	ksz_read8(dev, addr, &data); \
> +	data; \
> +})
> +
> +#define read32_op(addr)	\
> +({ \
> +	u32 data; \
> +	ksz_read32(dev, addr, &data); \
> +	data; \
> +})

These two are not used. Please remove them.

> +
> +#define pread32_op(addr)	\
> +({ \
> +	u32 data; \
> +	ksz_pread32(dev, port, addr, &data); \
> +	data; \
> +})


It works, but it is not nice, and it makes assumptions about how
readx_poll_timeout is implemented.

> +	ret = readx_poll_timeout(pread32_op, REG_PORT_MIB_CTRL_STAT__4, data,
> +				 !(data & MIB_COUNTER_READ), 10, 1000);

The macro is only used one, and addr is fixed,
REG_PORT_MIB_CTRL_STAT__4. So you can at least replace addr with port,
and rename the macro pread32_stat(port).


> +	/* failed to read MIB. get out of loop */
> +	if (ret < 0) {
> +		dev_dbg(dev->dev, "Failed to get MIB\n");
> +		return;
> +	}
> +
> +	/* count resets upon read */
> +	ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
> +	*cnt += data;
> +}
> +
> +static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *dropped, u64 *cnt)
> +{
> +	addr = ksz9477_mib_names[addr].index;
> +	ksz9477_r_mib_cnt(dev, port, addr, cnt);
> +}
> +
> +static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
> +{
> +	struct ksz_port *p = &dev->ports[port];
> +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;

Reverse Christmas tree.

> +
> +	/* enable/disable the port for flush/freeze function */
> +	mutex_lock(&p->mib.cnt_mutex);
> +	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, val);
> +
> +	/* used by MIB counter reading code to know freeze is enabled */
> +	p->freeze = freeze;
> +	mutex_unlock(&p->mib.cnt_mutex);
> +}


> +static void ksz_mib_read_work(struct work_struct *work)
> +{
> +	struct ksz_device *dev =
> +		container_of(work, struct ksz_device, mib_read);
> +	struct ksz_port *p;
> +	struct ksz_port_mib *mib;
> +	int i;
> +
> +	for (i = 0; i < dev->mib_port_cnt; i++) {
> +		p = &dev->ports[i];
> +		if (!p->on)
> +			continue;
> +		mib = &p->mib;
> +		mutex_lock(&mib->cnt_mutex);
> +
> +		/* read only dropped counters when link is not up */
> +		if (p->link_just_down)
> +			p->link_just_down = 0;
> +		else if (!p->phydev.link)
> +			mib->cnt_ptr = dev->reg_mib_cnt;

This link_just_down stuff is not clear at all. Why can the drop
counters not be read when the link is up?

> +		port_r_cnt(dev, i);
> +		mutex_unlock(&mib->cnt_mutex);
> +	}
> +}

RE: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Tristram.Ha]

> > +static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool
> freeze)
> > +{
> > +	struct ksz_port *p = &dev->ports[port];
> > +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> 
> Reverse Christmas tree.

There was a checkpatch.pl patch in 2016 that tried to check this, but it was never accepted?
 
> > +		/* read only dropped counters when link is not up */
> > +		if (p->link_just_down)
> > +			p->link_just_down = 0;
> > +		else if (!p->phydev.link)
> > +			mib->cnt_ptr = dev->reg_mib_cnt;
> 
> This link_just_down stuff is not clear at all. Why can the drop
> counters not be read when the link is up?

All of the MIB counters, except some that may be marked by driver, do not get updated when the link is down, so it is a waste of time to read them.

My intention is the driver eventually reads the MIB counters at least every second or faster so that the ethtool API called to show MIB counters gets them from memory rather than starting a read operation.  For now that API is called from user space with the ethtool utility, so it may not be called too often and too fast.  But theoretically that API can be called from a program continually.

For simple switches that do not need to do anything special the MIB read operation does not cause any issue except CPU load, for more complicate switches that need to do some background operations too many read operation can affect some critical functions.

RE: [PATCH v1 net-next 3/4] net: dsa: microchip: use readx_poll_time for polling

[Tristram.Ha]

> > -static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32
> waiton,
> > -					int timeout)
> > -{
> > -	u8 data;
> > -
> > -	do {
> > -		ksz_read8(dev, REG_SW_VLAN_CTRL, &data);
> > -		if (!(data & waiton))
> > -			break;
> > -		usleep_range(1, 10);
> > -	} while (timeout-- > 0);
> > -
> > -	if (timeout <= 0)
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> 
> Hi Tristram
> 
> I think it would be better to keep ksz9477_wait_vlan_ctrl_ready(),
> ksz9477_wait_alu_ready() etc, and change there implementation to use
> readx_poll_timeout(). The function names act as better documentation
> for what we are waiting for than the parameters being passed to
> readx_poll_timeout().

The macro readx_poll_timeout() was suggested when the MIB counter read timeout code was implemented.  It is a little awkward to use with ksz_read as the macro was designed to use with readb, readw, and readl call.

Maybe the whole macro can be copied and modified to accommodate ksz_read?

In the spirit of always updating the driver with better code the ksz9477_wait_ functions are also replaced to use readx_poll_timeout().  If the function code is just replaced with a macro inside there is not much point to declare the code as a function.

I think I just leave the old code as is.

Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Andrew Lunn]

> All of the MIB counters, except some that may be marked by driver,
> do not get updated when the link is down, so it is a waste of time
> to read them.

Hi Tristram

O.K, so make this clear in the code. Maybe rather than having this
link_just_down, have the adjust link callback update the cached
values for all counters? 

> My intention is the driver eventually reads the MIB counters at
> least every second or faster so that the ethtool API called to show
> MIB counters gets them from memory rather than starting a read
> operation.

The user expects to see the current counters, not some cached values.
For me it is O.K. to frequently read the counters to prevent wrap
around, but each ethtool call should update the counters before
returning them to user space.

> For simple switches that do not need to do anything special the MIB
> read operation does not cause any issue except CPU load, for more
> complicate switches that need to do some background operations too
> many read operation can affect some critical functions.

Sounds like a bad design of the switch, if reading statistics from it
can upset its operation. You might want to consider rate limiting the
ethtool call.

	Andrew

RE: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Florian Fainelli]

On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
>> > +static void ksz9477_freeze_mib(struct ksz_device *dev, int port,
>bool
>> freeze)
>> > +{
>> > +	struct ksz_port *p = &dev->ports[port];
>> > +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
>> 
>> Reverse Christmas tree.
>
>There was a checkpatch.pl patch in 2016 that tried to check this, but
>it was never accepted?
> 
>> > +		/* read only dropped counters when link is not up */
>> > +		if (p->link_just_down)
>> > +			p->link_just_down = 0;
>> > +		else if (!p->phydev.link)
>> > +			mib->cnt_ptr = dev->reg_mib_cnt;
>> 
>> This link_just_down stuff is not clear at all. Why can the drop
>> counters not be read when the link is up?
>
>All of the MIB counters, except some that may be marked by driver, do
>not get updated when the link is down, so it is a waste of time to read
>them.

Can you use netif_running() to determine that condition? Maintaining your own set of variables when the PHY state machine should already determine the link state sounds redundant if not error prone.

>
>My intention is the driver eventually reads the MIB counters at least
>every second or faster so that the ethtool API called to show MIB
>counters gets them from memory rather than starting a read operation. 
>For now that API is called from user space with the ethtool utility, so
>it may not be called too often and too fast.  But theoretically that
>API can be called from a program continually.
>
>For simple switches that do not need to do anything special the MIB
>read operation does not cause any issue except CPU load, for more
>complicate switches that need to do some background operations too many
>read operation can affect some critical functions.

Some switches have a MIB autocast feature taking a snapshot which AFAIR is internally implemented as a fast read register with no contention on other registers internally, do you have something similar?
-- 
Florian

RE: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Tristram.Ha]

> >> > +		/* read only dropped counters when link is not up */
> >> > +		if (p->link_just_down)
> >> > +			p->link_just_down = 0;
> >> > +		else if (!p->phydev.link)
> >> > +			mib->cnt_ptr = dev->reg_mib_cnt;
> >>
> >> This link_just_down stuff is not clear at all. Why can the drop
> >> counters not be read when the link is up?
> >
> >All of the MIB counters, except some that may be marked by driver, do
> >not get updated when the link is down, so it is a waste of time to read
> >them.
> 
> Can you use netif_running() to determine that condition? Maintaining your
> own set of variables when the PHY state machine should already determine
> the link state sounds redundant if not error prone.
> 

The driver can store the PHY device pointer passed to it when the port is enabled.  But I am a little worried that pointer can be changed or completely gone as it is out of control of the driver.

> >My intention is the driver eventually reads the MIB counters at least
> >every second or faster so that the ethtool API called to show MIB
> >counters gets them from memory rather than starting a read operation.
> >For now that API is called from user space with the ethtool utility, so
> >it may not be called too often and too fast.  But theoretically that
> >API can be called from a program continually.
> >
> >For simple switches that do not need to do anything special the MIB
> >read operation does not cause any issue except CPU load, for more
> >complicate switches that need to do some background operations too many
> >read operation can affect some critical functions.
> 
> Some switches have a MIB autocast feature taking a snapshot which AFAIR is
> internally implemented as a fast read register with no contention on other
> registers internally, do you have something similar?

There is no such function in the switch.  Every MIB counter read has to go through a single SPI transfer using indirect access.  There are no table-like stored values that a single SPI transfer can retrieve all.

For i2C the access is even slower, but then I do not expect this access mechanism is used when the switch can do more complex things.

Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Florian Fainelli]

On 2/14/19 11:26 AM, Tristram.Ha@microchip.com wrote:
>>>>> +		/* read only dropped counters when link is not up */
>>>>> +		if (p->link_just_down)
>>>>> +			p->link_just_down = 0;
>>>>> +		else if (!p->phydev.link)
>>>>> +			mib->cnt_ptr = dev->reg_mib_cnt;
>>>>
>>>> This link_just_down stuff is not clear at all. Why can the drop
>>>> counters not be read when the link is up?
>>>
>>> All of the MIB counters, except some that may be marked by driver, do
>>> not get updated when the link is down, so it is a waste of time to read
>>> them.
>>
>> Can you use netif_running() to determine that condition? Maintaining your
>> own set of variables when the PHY state machine should already determine
>> the link state sounds redundant if not error prone.
>>
> 
> The driver can store the PHY device pointer passed to it when the port is enabled.  But I am a little worried that pointer can be changed or completely gone as it is out of control of the driver.

The per-port network device is accessible from dp->slave so you can do
netif_running(dp->slave) from your driver, what are you talking about here?

> 
>>> My intention is the driver eventually reads the MIB counters at least
>>> every second or faster so that the ethtool API called to show MIB
>>> counters gets them from memory rather than starting a read operation.
>>> For now that API is called from user space with the ethtool utility, so
>>> it may not be called too often and too fast.  But theoretically that
>>> API can be called from a program continually.
>>>
>>> For simple switches that do not need to do anything special the MIB
>>> read operation does not cause any issue except CPU load, for more
>>> complicate switches that need to do some background operations too many
>>> read operation can affect some critical functions.
>>
>> Some switches have a MIB autocast feature taking a snapshot which AFAIR is
>> internally implemented as a fast read register with no contention on other
>> registers internally, do you have something similar?
> 
> There is no such function in the switch.  Every MIB counter read has to go through a single SPI transfer using indirect access.  There are no table-like stored values that a single SPI transfer can retrieve all.
> 
> For i2C the access is even slower, but then I do not expect this access mechanism is used when the switch can do more complex things.
> 

Is that something you are considering to change for future designs?
-- 
Florian

Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Joe Perches]

On Tue, 2019-02-12 at 19:51 -0800, Florian Fainelli wrote:
> On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
> > > > +static void ksz9477_freeze_mib(struct ksz_device *dev, int port,
> > > > +				bool freeze)
> > > > +{
> > > > +	struct ksz_port *p = &dev->ports[port];
> > > > +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> > > 
> > > Reverse Christmas tree.
> > 
> > There was a checkpatch.pl patch in 2016 that tried to check this, but
> > it was never accepted?

https://lore.kernel.org/patchwork/patch/732076/

While I still think use of reverse christmas tree is misguided,
there were some disagreements about what form it really is:

Is this reverse christmas tree?

   void foo(void)
   {
   	int	aa;
   	int	a = 1;

or is

   void foo(void)
   {
   	int	a = 1;
   	int	aa;

IMHO: neither really helps to visually find or scan for
automatics so the whole concept isn't particularly useful.

Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Wed 2019-02-20 15:49:25, Joe Perches wrote:
> On Tue, 2019-02-12 at 19:51 -0800, Florian Fainelli wrote:
> > On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
> > > > > +static void ksz9477_freeze_mib(struct ksz_device *dev, int port,
> > > > > +				bool freeze)
> > > > > +{
> > > > > +	struct ksz_port *p = &dev->ports[port];
> > > > > +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> > > > 
> > > > Reverse Christmas tree.
> > > 
> > > There was a checkpatch.pl patch in 2016 that tried to check this, but
> > > it was never accepted?
> 
> https://lore.kernel.org/patchwork/patch/732076/
> 
> While I still think use of reverse christmas tree is misguided,
> there were some disagreements about what form it really is:

Agreed about "misguided".

Often there's more logical order, like "self" pointer first... which
this patch follows.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]