[PATCH RFC 0/6] net: dsa: microchip: Modify KSZ9477 DSA driver to support different tail tag formats

[PATCH RFC 0/6] net: dsa: microchip: Modify KSZ9477 DSA driver to support different tail tag formats

[Tristram.Ha]

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

This series of patches is to modify the KSZ9477 DSA driver to support
different tail tag formats such that other new KSZ switch drivers can be
added without changing the base tail tagging code.

Tristram Ha (6):
  net: dsa: microchip: Prepare PHY for proper advertisement
  net: dsa: microchip: Add MIB counter reading support
  net: dsa: microchip: Break ksz_priv.h into two files
  net: dsa: microchip: Each switch driver has its own tail tagging
    operations
  net: dsa: microchip: Update tag_ksz.c to access switch driver
  net: dsa: microchip: Add switch offload forwarding support

 drivers/net/dsa/microchip/ksz9477.c    | 218 ++++++++++++++++++++++++++-------
 drivers/net/dsa/microchip/ksz_common.c | 122 +++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |   4 +
 drivers/net/dsa/microchip/ksz_priv.h   |  94 ++------------
 include/linux/dsa/ksz_dsa.h            |  99 +++++++++++++++
 net/dsa/tag_ksz.c                      |  46 ++++---
 6 files changed, 426 insertions(+), 157 deletions(-)
 create mode 100644 include/linux/dsa/ksz_dsa.h

-- 
1.9.1

[PATCH RFC 1/6] 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>
Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 12 ++++++++++++
 drivers/net/dsa/microchip/ksz_common.c | 17 +++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  2 ++
 drivers/net/dsa/microchip/ksz_priv.h   |  2 ++
 4 files changed, 33 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 0684657..98aa25e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -969,6 +969,16 @@ 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)
+{
+	if (port < dev->phy_port_cnt) {
+		/* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
+		 * disable flow control when rate limiting is used.
+		 */
+	}
+}
+
 static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
 	u8 data8;
@@ -1151,6 +1161,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 +1309,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 9705808..39adc57 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -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..206b313 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/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 a38ff08..fcb75a8 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -135,6 +135,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

Re: [PATCH RFC 1/6] net: dsa: microchip: Prepare PHY for proper advertisement

[Andrew Lunn]

On Mon, Dec 03, 2018 at 03:34:52PM -0800, Tristram.Ha@microchip.com wrote:
> 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>
> Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c    | 12 ++++++++++++
>  drivers/net/dsa/microchip/ksz_common.c | 17 +++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.h |  2 ++
>  drivers/net/dsa/microchip/ksz_priv.h   |  2 ++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 0684657..98aa25e 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -969,6 +969,16 @@ 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)
> +{
> +	if (port < dev->phy_port_cnt) {
> +		/* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
> +		 * disable flow control when rate limiting is used.
> +		 */
> +	}

Hi Tristram

Is this meant to be a TODO comment?

What is supposed to happen here is that all forms of pause are disable
by default. The MAC driver needs to enable what it supports by calling
phy_support_sym_pause() or phy_support_asym_pause().

Ah, is this because there is not a real PHY driver?

Thanks
	Andrew

RE: [PATCH RFC 1/6] net: dsa: microchip: Prepare PHY for proper advertisement

[Tristram.Ha]

> > +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> > +			      struct phy_device *phy)
> > +{
> > +	if (port < dev->phy_port_cnt) {
> > +		/* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be
> removed to
> > +		 * disable flow control when rate limiting is used.
> > +		 */
> > +	}
> 
> Hi Tristram
> 
> Is this meant to be a TODO comment?
> 
> What is supposed to happen here is that all forms of pause are disable
> by default. The MAC driver needs to enable what it supports by calling
> phy_support_sym_pause() or phy_support_asym_pause().
> 
> Ah, is this because there is not a real PHY driver?

The kernel has been changed so I am not sure about the current behavior.
I would like to turn on flow control by default.  Before I just assigned
"supported" to "advertising."  I know linkmode_copy is being used now for that.
But last time I checked "advertising" is already the same as "supported."

There will be a situation that flow control should not be turned on as the switch
uses bandwidth control to limit outgoing traffic.

The issue is actually becoming more complex as KSZ9477 has a variant which
does not support gigabit speed, although the same PHY device id is being used.
That means the driver has to fake it by returning a different id and also registering
a different PHY driver to handle that.  Marketing also likes to display the correct chip
name during kernel booting so that users do not get confused.

[PATCH RFC 2/6] 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>
Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 121 ++++++++++++++++++++++-----------
 drivers/net/dsa/microchip/ksz_common.c | 101 +++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   2 +
 drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
 4 files changed, 186 insertions(+), 45 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 98aa25e..bd1ca33 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -259,6 +259,76 @@ 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 timeout;
+	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);
+
+	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");
+		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 +412,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)
 {
@@ -1150,9 +1179,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;
 }
 
@@ -1287,6 +1321,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) *
@@ -1311,6 +1346,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 39adc57..7b8f57b 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,24 @@ 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;
+
+	/* freeze MIB counters if supported */
+	if (dev->dev_ops->freeze_mib)
+		dev->dev_ops->freeze_mib(dev, port, true);
+	mutex_lock(&mib->cnt_mutex);
+	port_r_cnt(dev, port);
+	mutex_unlock(&mib->cnt_mutex);
+	if (dev->dev_ops->freeze_mib)
+		dev->dev_ops->freeze_mib(dev, port, false);
+	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
+}
+
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br)
 {
@@ -255,6 +349,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);
+	dev->dev_ops->port_init_cnt(dev, port);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
@@ -344,6 +439,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 206b313..547859e 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 fcb75a8..1955ea6 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;
 };
@@ -77,8 +77,6 @@ struct ksz_device {
 
 	struct vlan_table *vlan_cache;
 
-	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
 	u8 *txbuf;
 
 	struct ksz_port *ports;
@@ -151,6 +149,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

Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support

[Andrew Lunn]

Hi Tristan

> +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	u32 data;
> +	int timeout;
> +	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);
> +
> +	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);

Could you use readx_poll_timeout() here?

> +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;
> +
> +	/* freeze MIB counters if supported */
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, true);
> +	mutex_lock(&mib->cnt_mutex);
> +	port_r_cnt(dev, port);
> +	mutex_unlock(&mib->cnt_mutex);
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, false);

Should the freeze be protected by the mutex as well?

> +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));

I wonder if this memcpy should also be protected by the mutex. As soon
as the mutex is dropped, the scheduled work could start updating
mib->counters in non-atomic ways?

> +}
> +
>  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>  			 struct net_device *br)
>  {
> @@ -255,6 +349,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);
> +	dev->dev_ops->port_init_cnt(dev, port);

This is probably not the correct place to do this. MIB counters should
not be cleared by an ifdown/ifup cycle. They should only be cleared
when the driver is probed.

RE: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support

[Tristram.Ha]

> > +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> > +			      u64 *cnt)
> > +{
> > +	u32 data;
> > +	int timeout;
> > +	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);
> > +
> > +	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);
> 
> Could you use readx_poll_timeout() here?
> 
> > +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;
> > +
> > +	/* freeze MIB counters if supported */
> > +	if (dev->dev_ops->freeze_mib)
> > +		dev->dev_ops->freeze_mib(dev, port, true);
> > +	mutex_lock(&mib->cnt_mutex);
> > +	port_r_cnt(dev, port);
> > +	mutex_unlock(&mib->cnt_mutex);
> > +	if (dev->dev_ops->freeze_mib)
> > +		dev->dev_ops->freeze_mib(dev, port, false);
> 
> Should the freeze be protected by the mutex as well?
> 
> > +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
> 
> I wonder if this memcpy should also be protected by the mutex. As soon
> as the mutex is dropped, the scheduled work could start updating
> mib->counters in non-atomic ways?
>

I will update as suggested.
 
> > +}
> > +
> >  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
> >  			 struct net_device *br)
> >  {
> > @@ -255,6 +349,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);
> > +	dev->dev_ops->port_init_cnt(dev, port);
> 
> This is probably not the correct place to do this. MIB counters should
> not be cleared by an ifdown/ifup cycle. They should only be cleared
> when the driver is probed.

I wonder what is the official way to clear the counters.  For network debugging
It is good to clear the counters to start fresh to see which frame is not
being sent or received.  Typically the device is reset when it is shutdown as there
are hardware problems.  I would think it is the job of applications like SNMP Manager
to keep track of MIB counters throughout the life of a running system.

Re: [PATCH RFC 2/6] net: dsa: microchip: Add MIB counter reading support

[Andrew Lunn]

> I wonder what is the official way to clear the counters.

I don't think there is one, other than unloading the driver and
loading it again.

	Andrew

[PATCH RFC 3/6] net: dsa: microchip: Break ksz_priv.h into two files

[Tristram.Ha]

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

Break ksz_priv.h into two files: private and public.  The public one is
put in include/linux/dsa/ksz_dsa.h.

This allows the tail tagging code tag_ksz.c to access the switch driver
as the tail tag format can be different when certain switch functions are
enabled.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz_priv.h | 86 +---------------------------------
 include/linux/dsa/ksz_dsa.h          | 90 ++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 84 deletions(-)
 create mode 100644 include/linux/dsa/ksz_dsa.h

diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 1955ea6..5d93822 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0
  *
- * Microchip KSZ series switch common definitions
+ * Microchip KSZ series switch private definitions
  *
  * Copyright (C) 2017-2018 Microchip Technology Inc.
  */
@@ -13,89 +13,7 @@
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
 #include <net/dsa.h>
-
-struct ksz_io_ops;
-
-struct vlan_table {
-	u32 table[3];
-};
-
-struct ksz_port_mib {
-	struct mutex cnt_mutex;		/* structure access */
-	u8 cnt_ptr;
-	u64 *counters;
-};
-
-struct ksz_port {
-	u16 member;
-	u16 vid_member;
-	int stp_state;
-	struct phy_device phydev;
-
-	u32 on:1;			/* port is not disabled by hardware */
-	u32 phy:1;			/* port has a PHY */
-	u32 fiber:1;			/* port is fiber */
-	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;
-};
-
-struct ksz_device {
-	struct dsa_switch *ds;
-	struct ksz_platform_data *pdata;
-	const char *name;
-
-	struct mutex reg_mutex;		/* register access */
-	struct mutex stats_mutex;	/* status access */
-	struct mutex alu_mutex;		/* ALU access */
-	struct mutex vlan_mutex;	/* vlan access */
-	const struct ksz_io_ops *ops;
-	const struct ksz_dev_ops *dev_ops;
-
-	struct device *dev;
-
-	void *priv;
-
-	/* chip specific data */
-	u32 chip_id;
-	int num_vlans;
-	int num_alus;
-	int num_statics;
-	int cpu_port;			/* port connected to CPU */
-	int cpu_ports;			/* port bitmap can be cpu port */
-	int phy_port_cnt;
-	int port_cnt;
-	int reg_mib_cnt;
-	int mib_cnt;
-	int mib_port_cnt;
-	int last_port;			/* ports after that not used */
-	phy_interface_t interface;
-	u32 regs_size;
-
-	struct vlan_table *vlan_cache;
-
-	u8 *txbuf;
-
-	struct ksz_port *ports;
-	struct timer_list mib_read_timer;
-	struct work_struct mib_read;
-	unsigned long mib_read_interval;
-	u16 br_member;
-	u16 member;
-	u16 live_ports;
-	u16 on_ports;			/* ports enabled by DSA */
-	u16 rx_ports;
-	u16 tx_ports;
-	u16 mirror_rx;
-	u16 mirror_tx;
-	u32 features;			/* chip specific features */
-	u32 overrides;			/* chip functions set by user */
-	u16 host_mask;
-	u16 port_mask;
-};
+#include <linux/dsa/ksz_dsa.h>
 
 struct ksz_io_ops {
 	int (*read8)(struct ksz_device *dev, u32 reg, u8 *value);
diff --git a/include/linux/dsa/ksz_dsa.h b/include/linux/dsa/ksz_dsa.h
new file mode 100644
index 0000000..3148cae
--- /dev/null
+++ b/include/linux/dsa/ksz_dsa.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Microchip KSZ series switch common definitions
+ *
+ * Copyright (C) 2017-2018 Microchip Technology Inc.
+ */
+
+#include <linux/phy.h>
+
+struct vlan_table {
+	u32 table[3];
+};
+
+struct ksz_port_mib {
+	struct mutex cnt_mutex;		/* structure access */
+	u8 cnt_ptr;
+	u64 *counters;
+};
+
+struct ksz_port {
+	u16 member;
+	u16 vid_member;
+	int stp_state;
+	struct phy_device phydev;
+
+	u32 on:1;			/* port is not disabled by hardware */
+	u32 phy:1;			/* port has a PHY */
+	u32 fiber:1;			/* port is fiber */
+	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;
+};
+
+struct ksz_device {
+	struct dsa_switch *ds;
+	struct ksz_platform_data *pdata;
+	const char *name;
+
+	struct mutex reg_mutex;		/* register access */
+	struct mutex stats_mutex;	/* status access */
+	struct mutex alu_mutex;		/* ALU access */
+	struct mutex vlan_mutex;	/* vlan access */
+	const struct ksz_io_ops *ops;
+	const struct ksz_dev_ops *dev_ops;
+
+	struct device *dev;
+
+	void *priv;
+
+	/* chip specific data */
+	u32 chip_id;
+	int num_vlans;
+	int num_alus;
+	int num_statics;
+	int cpu_port;			/* port connected to CPU */
+	int cpu_ports;			/* port bitmap can be cpu port */
+	int phy_port_cnt;
+	int port_cnt;
+	int reg_mib_cnt;
+	int mib_cnt;
+	int mib_port_cnt;
+	int last_port;			/* ports after that not used */
+	phy_interface_t interface;
+	u32 regs_size;
+
+	struct vlan_table *vlan_cache;
+
+	u8 *txbuf;
+
+	struct ksz_port *ports;
+	struct timer_list mib_read_timer;
+	struct work_struct mib_read;
+	unsigned long mib_read_interval;
+	u16 br_member;
+	u16 member;
+	u16 live_ports;
+	u16 on_ports;			/* ports enabled by DSA */
+	u16 rx_ports;
+	u16 tx_ports;
+	u16 mirror_rx;
+	u16 mirror_tx;
+	u32 features;			/* chip specific features */
+	u32 overrides;			/* chip functions set by user */
+	u16 host_mask;
+	u16 port_mask;
+};
+
-- 
1.9.1

[PATCH RFC 4/6] net: dsa: microchip: Each switch driver has its own tail tagging operations

[Tristram.Ha]

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

The tail tagging operations are implemented in each switch driver so that
the main tail tagging code tag_ksz.c does not need to be changed after
modification to support that mechanism is made.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 78 +++++++++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.c |  4 +-
 drivers/net/dsa/microchip/ksz_priv.h   |  3 +-
 include/linux/dsa/ksz_dsa.h            |  9 ++++
 4 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index bd1ca33..c690c2b2 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -21,6 +21,14 @@
 #include "ksz_common.h"
 #include "ksz9477_reg.h"
 
+/* features flags */
+#define GBIT_SUPPORT			BIT(0)
+#define NEW_XMII			BIT(1)
+#define IS_9893				BIT(2)
+
+/* overrides flags */
+#define PTP_TAG				BIT(0)
+
 static const struct {
 	int index;
 	char string[ETH_GSTRING_LEN];
@@ -1356,9 +1364,77 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.exit = ksz9477_switch_exit,
 };
 
+/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
+ * ---------------------------------------------------------------------------
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
+ * ---------------------------------------------------------------------------
+ * tag0 : Prioritization (not used now)
+ * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
+ *
+ * For switch with 3 ports only one byte is needed.
+ * When PTP function is enabled additional 4 bytes are needed.
+ *
+ * For Egress (KSZ -> Host), 1 byte is added before FCS.
+ * ---------------------------------------------------------------------------
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
+ * ---------------------------------------------------------------------------
+ * tag0 : zero-based value represents port
+ *	  (eg, 0x00=port1, 0x02=port3, 0x06=port7)
+ *
+ * When PTP function is enabled BIT 7 indicates the received frame is a PTP
+ * message and so there are 4 additional bytes for the receive timestamp.
+ */
+
+static int ksz9477_get_len(struct ksz_device *dev)
+{
+	int len = 1;
+
+	if (!(dev->features & IS_9893))
+		len += 1;
+	if (dev->overrides & PTP_TAG)
+		len += 4;
+	return len;
+}
+
+static int ksz9477_get_tag(struct ksz_device *dev, u8 *tag, int *port)
+{
+	int len = 1;
+
+	if (tag[0] & BIT(7))
+		len += 4;
+	*port = tag[0] & 7;
+	return len;
+}
+
+static void ksz9477_set_tag(struct ksz_device *dev, void *ptr, u8 *addr, int p)
+{
+	if (dev->overrides & PTP_TAG) {
+		u32 *timestamp = (u32 *)ptr;
+
+		*timestamp = 0;
+		ptr = timestamp + 1;
+	}
+	if (dev->features & IS_9893) {
+		u8 *tag = (u8 *)ptr;
+
+		*tag = 1 << p;
+	} else {
+		u16 *tag = (u16 *)ptr;
+
+		*tag = 1 << p;
+		*tag = cpu_to_be16(*tag);
+	}
+}
+
+static const struct ksz_tag_ops ksz9477_tag_ops = {
+	.get_len = ksz9477_get_len,
+	.get_tag = ksz9477_get_tag,
+	.set_tag = ksz9477_set_tag,
+};
+
 int ksz9477_switch_register(struct ksz_device *dev)
 {
-	return ksz_switch_register(dev, &ksz9477_dev_ops);
+	return ksz_switch_register(dev, &ksz9477_dev_ops, &ksz9477_tag_ops);
 }
 EXPORT_SYMBOL(ksz9477_switch_register);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7b8f57b..a72659b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -399,7 +399,8 @@ struct ksz_device *ksz_switch_alloc(struct device *base,
 EXPORT_SYMBOL(ksz_switch_alloc);
 
 int ksz_switch_register(struct ksz_device *dev,
-			const struct ksz_dev_ops *ops)
+			const struct ksz_dev_ops *ops,
+			const struct ksz_tag_ops *tag_ops)
 {
 	int ret;
 
@@ -412,6 +413,7 @@ int ksz_switch_register(struct ksz_device *dev,
 	mutex_init(&dev->vlan_mutex);
 
 	dev->dev_ops = ops;
+	dev->tag_ops = tag_ops;
 
 	if (dev->dev_ops->detect(dev))
 		return -EINVAL;
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 5d93822..d020c1e 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -78,7 +78,8 @@ struct ksz_dev_ops {
 struct ksz_device *ksz_switch_alloc(struct device *base,
 				    const struct ksz_io_ops *ops, void *priv);
 int ksz_switch_register(struct ksz_device *dev,
-			const struct ksz_dev_ops *ops);
+			const struct ksz_dev_ops *ops,
+			const struct ksz_tag_ops *tag_ops);
 void ksz_switch_remove(struct ksz_device *dev);
 
 int ksz9477_switch_register(struct ksz_device *dev);
diff --git a/include/linux/dsa/ksz_dsa.h b/include/linux/dsa/ksz_dsa.h
index 3148cae..3fb2713 100644
--- a/include/linux/dsa/ksz_dsa.h
+++ b/include/linux/dsa/ksz_dsa.h
@@ -7,6 +7,14 @@
 
 #include <linux/phy.h>
 
+struct ksz_device;
+
+struct ksz_tag_ops {
+	int (*get_len)(struct ksz_device *dev);
+	int (*get_tag)(struct ksz_device *dev, u8 *tag, int *port);
+	void (*set_tag)(struct ksz_device *dev, void *ptr, u8 *addr, int p);
+};
+
 struct vlan_table {
 	u32 table[3];
 };
@@ -45,6 +53,7 @@ struct ksz_device {
 	struct mutex vlan_mutex;	/* vlan access */
 	const struct ksz_io_ops *ops;
 	const struct ksz_dev_ops *dev_ops;
+	const struct ksz_tag_ops *tag_ops;
 
 	struct device *dev;
 
-- 
1.9.1

[PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Tristram.Ha]

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

Update tag_ksz.c to access switch driver's tail tagging operations.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 net/dsa/tag_ksz.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 0f62eff..307e58b 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -11,37 +11,25 @@
 #include <linux/etherdevice.h>
 #include <linux/list.h>
 #include <linux/slab.h>
+#include <linux/dsa/ksz_dsa.h>
 #include <net/dsa.h>
 #include "dsa_priv.h"
 
-/* For Ingress (Host -> KSZ), 2 bytes are added before FCS.
- * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
- * ---------------------------------------------------------------------------
- * tag0 : Prioritization (not used now)
- * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
- *
- * For Egress (KSZ -> Host), 1 byte is added before FCS.
- * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
- * ---------------------------------------------------------------------------
- * tag0 : zero-based value represents port
- *	  (eg, 0x00=port1, 0x02=port3, 0x06=port7)
- */
-
-#define	KSZ_INGRESS_TAG_LEN	2
 #define	KSZ_EGRESS_TAG_LEN	1
 
 static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct ksz_device *sw = dp->ds->priv;
 	struct sk_buff *nskb;
+	int len;
 	int padlen;
-	u8 *tag;
+
+	len = sw->tag_ops->get_len(sw);
 
 	padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
 
-	if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
+	if (skb_tailroom(skb) >= padlen + len) {
 		/* Let dsa_slave_xmit() free skb */
 		if (__skb_put_padto(skb, skb->len + padlen, false))
 			return NULL;
@@ -49,7 +37,7 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 		nskb = skb;
 	} else {
 		nskb = alloc_skb(NET_IP_ALIGN + skb->len +
-				 padlen + KSZ_INGRESS_TAG_LEN, GFP_ATOMIC);
+				 padlen + len, GFP_ATOMIC);
 		if (!nskb)
 			return NULL;
 		skb_reserve(nskb, NET_IP_ALIGN);
@@ -70,9 +58,8 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 		consume_skb(skb);
 	}
 
-	tag = skb_put(nskb, KSZ_INGRESS_TAG_LEN);
-	tag[0] = 0;
-	tag[1] = 1 << dp->index; /* destination port */
+	sw->tag_ops->set_tag(sw, skb_put(nskb, len), skb_mac_header(nskb),
+			     dp->index);
 
 	return nskb;
 }
@@ -80,18 +67,27 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 			       struct packet_type *pt)
 {
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch_tree *dst = cpu_dp->dst;
+	struct dsa_switch *ds = dst->ds[0];
+	struct ksz_device *sw;
 	u8 *tag;
+	int len;
 	int source_port;
 
+	if (!ds)
+		return NULL;
+	sw = ds->priv;
+
 	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
 
-	source_port = tag[0] & 7;
+	len = sw->tag_ops->get_tag(sw, tag, &source_port);
 
 	skb->dev = dsa_master_find_slave(dev, 0, source_port);
 	if (!skb->dev)
 		return NULL;
 
-	pskb_trim_rcsum(skb, skb->len - KSZ_EGRESS_TAG_LEN);
+	pskb_trim_rcsum(skb, skb->len - len);
 
 	return skb;
 }
-- 
1.9.1

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Andrew Lunn]

On Mon, Dec 03, 2018 at 03:34:56PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Update tag_ksz.c to access switch driver's tail tagging operations.

Hi Tristram

Humm, i'm not sure we want this, the tagging spit into two places.  I
need to take a closer look at the previous patch, to see why it cannot
be done here.

   Andrew

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Andrew Lunn]

On Wed, Dec 05, 2018 at 07:00:38PM +0100, Andrew Lunn wrote:
> On Mon, Dec 03, 2018 at 03:34:56PM -0800, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <Tristram.Ha@microchip.com>
> > 
> > Update tag_ksz.c to access switch driver's tail tagging operations.
> 
> Hi Tristram
> 
> Humm, i'm not sure we want this, the tagging spit into two places.  I
> need to take a closer look at the previous patch, to see why it cannot
> be done here.

O.K, i think i get what is going on.

I would however implement it differently.

One net/dsa/tag_X.c file can export two dsa_device_ops structures,
allowing you to share common code for the two taggers. You could call
these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE, and the
.get_tag_protocol call would then return the correct one for the
switch.

It might also be possible to merge in tag_trailer, or at least share
some code.

What i don't yet understand is how you are passing PTP information
around. The commit messages need to explain that, since it is not
obvious, and it is the first time we have needed PTP info in a tag
driver.

	Andrew

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Florian Fainelli]

On 12/5/18 10:18 AM, Andrew Lunn wrote:
> On Wed, Dec 05, 2018 at 07:00:38PM +0100, Andrew Lunn wrote:
>> On Mon, Dec 03, 2018 at 03:34:56PM -0800, Tristram.Ha@microchip.com wrote:
>>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>>
>>> Update tag_ksz.c to access switch driver's tail tagging operations.
>>
>> Hi Tristram
>>
>> Humm, i'm not sure we want this, the tagging spit into two places.  I
>> need to take a closer look at the previous patch, to see why it cannot
>> be done here.
> 
> O.K, i think i get what is going on.
> 
> I would however implement it differently.
> 
> One net/dsa/tag_X.c file can export two dsa_device_ops structures,
> allowing you to share common code for the two taggers. You could call
> these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE, and the
> .get_tag_protocol call would then return the correct one for the
> switch.

Agreed, that is what is done by net/dsa/tag_brcm.c because there are two
formats for the Broadcom tag:

- TAG_BRCM: the 4-bytes Broadcom tag is between MAC SA and Ethertype
- TAG_BRCM_PREPEND: the 4-bytes Broadcom tag is before the MAC DA

And the code to process them is basically using relative offsets from
the start of the frame to access correct data.

This is done largely for performance reasons because we have 1/2
Gigabit/secs capable CPU ports and so we want to avoid as little cache
trashing as possible and immediately get the right rcv() function to
process the packets.

> 
> It might also be possible to merge in tag_trailer, or at least share
> some code.
> 
> What i don't yet understand is how you are passing PTP information
> around. The commit messages need to explain that, since it is not
> obvious, and it is the first time we have needed PTP info in a tag
> driver.
> 
> 	Andrew
> 


-- 
Florian

RE: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Tristram.Ha]

> >>> Update tag_ksz.c to access switch driver's tail tagging operations.
> >>
> >> Hi Tristram
> >>
> >> Humm, i'm not sure we want this, the tagging spit into two places.  I
> >> need to take a closer look at the previous patch, to see why it cannot
> >> be done here.
> >
> > O.K, i think i get what is going on.
> >
> > I would however implement it differently.
> >
> > One net/dsa/tag_X.c file can export two dsa_device_ops structures,
> > allowing you to share common code for the two taggers. You could call
> > these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE,
> and the
> > .get_tag_protocol call would then return the correct one for the
> > switch.
> 
> Agreed, that is what is done by net/dsa/tag_brcm.c because there are two
> formats for the Broadcom tag:
> 
> - TAG_BRCM: the 4-bytes Broadcom tag is between MAC SA and Ethertype
> - TAG_BRCM_PREPEND: the 4-bytes Broadcom tag is before the MAC DA
>

I did try to implement this way.  But the other switches do not have the same
format even though the length is the same.  Then I need to change the following
files for any new KSZ switch: include/linux/dsa.h, net/dsa/dsa.c, net/dsa/dsa_priv.h,
and finally net/dsa/tag_ksz.c.

Even then it will not work if Microchip wants to add 1588 PTP capability to the switches.

For KSZ9477 the length of the tail tag changes when the PTP function is enabled.
Typically this function is either enabled or disabled all the time, but if users want to
change that during normal operation to see how the switch behaves, the transmit
function completely stops working correctly.

Older driver implementation is to monitor that register change and adjust the length
dynamically.

Another problem is the tail tag needs to include the timestamp for the 1-step
Pdelay_Resp to have accurate turnaround time when that message is sent out by the
switch.  This will require access to the main switch driver which will keep track of those
PTP messages.

PTP handles transmit timestamp in skb_tx_timestamp, which is typically called after the
frame is sent, so it is too late.  DSA calls dsa_skb_tx_timestamp before sending, but it
only provides a clone to the driver that supports port_txstamp and so the switch driver
may not be able to do anything.
 
> And the code to process them is basically using relative offsets from
> the start of the frame to access correct data.
> 
> This is done largely for performance reasons because we have 1/2
> Gigabit/secs capable CPU ports and so we want to avoid as little cache
> trashing as possible and immediately get the right rcv() function to
> process the packets.
> 

The SoC I used for this driver development actually has problem sending
Gigabit traffic so I do not see the effect of any slowdown, and the updated
MAC driver change for a hardware problem does not help and greatly
degrades the transmit performance.

> >
> > It might also be possible to merge in tag_trailer, or at least share
> > some code.
> >

Actually in previous old DSA implementation I just hijacked this file to
add the tail tag operations without creating a new file like tag_ksz.c.

> > What i don't yet understand is how you are passing PTP information
> > around. The commit messages need to explain that, since it is not
> > obvious, and it is the first time we have needed PTP info in a tag
> > driver.

It seems the official 1588 PTP timestamp API for a PHY driver is only implemented
in only PHY driver, net/phy/dp83640.c, in the whole kernel.  DSA uses similar
mechanism to support 1588 PTP.  In dsa_switch_rcv() the CPU receive function is called
first before dsa_skb_defer_rx_timestamp().  That means the receive tail tag operation
has to be done first to retrieve the receive timestamp so that it can be passed later.

It is probably not good to change the socket buffer length inside the port_rxtstamp
function, and I do not see any other way to insert that transmit timestamp.

A customer has already inquired about implementing 1588 PTP in the DSA driver.  I hope
this mechanism is approved so that I can start doing that.

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Andrew Lunn]

> I did try to implement this way.  But the other switches do not have the same
> format even though the length is the same.  Then I need to change the following
> files for any new KSZ switch: include/linux/dsa.h, net/dsa/dsa.c, net/dsa/dsa_priv.h,
> and finally net/dsa/tag_ksz.c.

You can always add two different tag drivers. They don't have to share
code if it does not make sense.

> Even then it will not work if Microchip wants to add 1588 PTP
> capability to the switches.
> 
> For KSZ9477 the length of the tail tag changes when the PTP function
> is enabled.  Typically this function is either enabled or disabled
> all the time, but if users want to change that during normal
> operation to see how the switch behaves, the transmit function
> completely stops working correctly.

We should figure out how to support PTP. I think that is the main
issue here.

> Older driver implementation is to monitor that register change and adjust the length
> dynamically.
> 
> Another problem is the tail tag needs to include the timestamp for the 1-step
> Pdelay_Resp to have accurate turnaround time when that message is sent out by the
> switch.  This will require access to the main switch driver which will keep track of those
> PTP messages.
> 
> PTP handles transmit timestamp in skb_tx_timestamp, which is typically called after the
> frame is sent, so it is too late.  DSA calls dsa_skb_tx_timestamp before sending, but it
> only provides a clone to the driver that supports port_txstamp and so the switch driver
> may not be able to do anything.

The current design assumes the hardware will insert the PTP timestamp
into the frame using the clock inside the hardware. You then ask it
what timestamp it actually used. 

If i understand you correctly, in your case, software was to provide
the timestamp which then gets inserted into the frame. So you want to
provide this timestamp as late as possible, when the frame reaches the
head of the queue and is about to be sent out the master interface?

> In dsa_switch_rcv() the CPU receive function is called first before
> dsa_skb_defer_rx_timestamp().  That means the receive tail tag
> operation has to be done first to retrieve the receive timestamp so
> that it can be passed later.

What i think you can do is in your tag rx function you can directly
add the timestamp info to the skbuf. The dsa driver function
.port_txtstamp can then always return false.

Your tag function is going to need access to some driver state, but
you should be able to get at that, following pointers, and placing
some of the structures in global headers.

    Andrew

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Richard Cochran]

On Thu, Dec 06, 2018 at 08:00:26PM +0000, Tristram.Ha@microchip.com wrote:
> A customer has already inquired about implementing 1588 PTP in the DSA driver.  I hope
> this mechanism is approved so that I can start doing that.

If you need changes to the PTP core, you had better discuss this with
the PTP maintainer.

Thanks,
Richard

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Pavel Machek]

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

On Thu 2018-12-06 20:00:26, Tristram.Ha@microchip.com wrote:
> > >>> Update tag_ksz.c to access switch driver's tail tagging operations.
> > >>
> > >> Hi Tristram
> > >>
> > >> Humm, i'm not sure we want this, the tagging spit into two places.  I
> > >> need to take a closer look at the previous patch, to see why it cannot
> > >> be done here.
> > >
> > > O.K, i think i get what is going on.
> > >
> > > I would however implement it differently.
> > >
> > > One net/dsa/tag_X.c file can export two dsa_device_ops structures,
> > > allowing you to share common code for the two taggers. You could call
> > > these DSA_TAG_PROTO_KSZ_1_BYTE, and DSA_TAG_PROTO_KSZ_2_BYTE,
> > and the
> > > .get_tag_protocol call would then return the correct one for the
> > > switch.
> > 
> > Agreed, that is what is done by net/dsa/tag_brcm.c because there are two
> > formats for the Broadcom tag:
> > 
> > - TAG_BRCM: the 4-bytes Broadcom tag is between MAC SA and Ethertype
> > - TAG_BRCM_PREPEND: the 4-bytes Broadcom tag is before the MAC DA
> >
> 
> I did try to implement this way.  But the other switches do not have the same
> format even though the length is the same.  Then I need to change the following
> files for any new KSZ switch: include/linux/dsa.h, net/dsa/dsa.c, net/dsa/dsa_priv.h,
> and finally net/dsa/tag_ksz.c.
> 
> Even then it will not work if Microchip wants to add 1588 PTP capability to the switches.
> 
> For KSZ9477 the length of the tail tag changes when the PTP function is enabled.
> Typically this function is either enabled or disabled all the time, but if users want to
> change that during normal operation to see how the switch behaves, the transmit
> function completely stops working correctly.

I'd be careful about locking. Seems like dsa was designed with "tag
format is static", and you want to change it dynamically...
									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 --]

RE: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Tristram.Ha]

> I'd be careful about locking. Seems like dsa was designed with "tag
> format is static", and you want to change it dynamically...

I see there is now a new overhead parameter in the dsa_device_ops structure
and dev_set_mtu is called in master.c.  It does not prevent the tag size to
change dynamically though.  A bigger size can be used instead to make sure the
MAC controller can support it.

In practice I do not think it does anything meaningful.  Most MAC controllers
can transmit and receive more than 1518 bytes but still only advertise 1500
MTU.  It is only when they support jumbo frame the drivers allow increasing
the MTU.  In the case of Atmel MAC controller I only see the mtu size is 1502,
but there is nothing changed inside the driver.

I did find another bug in the Atmel MAC driver concerning this max_mtu
implementation.  It does not affect the DSA operation as the child devices
still have the cap of 1500 MTU, but the main device will have problem running
by itself when MTU is increased.

Re: [PATCH RFC 5/6] net: dsa: microchip: Update tag_ksz.c to access switch driver

[Andrew Lunn]

On Tue, Dec 11, 2018 at 11:59:34PM +0000, Tristram.Ha@microchip.com wrote:
> > I'd be careful about locking. Seems like dsa was designed with "tag
> > format is static", and you want to change it dynamically...
> 
> I see there is now a new overhead parameter in the dsa_device_ops structure
> and dev_set_mtu is called in master.c.  It does not prevent the tag size to
> change dynamically though.  A bigger size can be used instead to make sure the
> MAC controller can support it.
> 
> In practice I do not think it does anything meaningful.  Most MAC controllers
> can transmit and receive more than 1518 bytes but still only advertise 1500
> MTU. 

Hi Tristram

There are a few MAC devices to do enforce 1518. e1000e is one
example. You have to increase the MTU before it will receive DSA
tagged frames. I initially had similar problems with the FEC driver
when i started using that a few years ago. At that time i did not
rallies it was a wide scale problem and just changed the FEC. This
should be a more generic solution.

       Andrew

[PATCH RFC 6/6] net: dsa: microchip: Add switch offload forwarding support

[Tristram.Ha]

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

The flag offload_fwd_mark is set as the switch can forward frames by
itself.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 7 ++++---
 net/dsa/tag_ksz.c                   | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index c690c2b2..ca9199a 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -434,6 +434,7 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 	struct ksz_port *p = &dev->ports[port];
 	u8 data;
 	int member = -1;
+	int forward = dev->member;
 
 	ksz_pread8(dev, port, P_STP_CTRL, &data);
 	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
@@ -501,10 +502,10 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 	}
 
 	/* When topology has changed the function ksz_update_port_member
-	 * should be called to modify port forwarding behavior.  However
-	 * as the offload_fwd_mark indication cannot be reported here
-	 * the switch forwarding function is not enabled.
+	 * should be called to modify port forwarding behavior.
 	 */
+	if (forward != dev->member)
+		ksz_update_port_member(dev, port);
 }
 
 static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 307e58b..50511c2 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -89,6 +89,8 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	pskb_trim_rcsum(skb, skb->len - len);
 
+	skb->offload_fwd_mark = 1;
+
 	return skb;
 }
 
-- 
1.9.1