Skip to contentSkip to conversation

net: dsa: ksz: Add MIB counter reading support

PATCH In review Marek Vasut 4 participants 14 messages Dec 7, 2018 – Dec 13, 2018

Subsystems (4)
Networking Network drivers Networking DSA Microchip ksz series Ethernet switch driver
lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH 1/5 net: dsa: ksz: Add MIB counter reading support

From: Tristram Ha <[email protected]>

Add MIB counter reading support to KSZ9477 driver. This makes the MIB counter code more generic by removing the TOTAL_SWITCH_COUNTER_NUM and instead making that configurable per switch model.

Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 drivers/net/dsa/microchip/ksz9477.c    | 129 +++++++++++++++++--------
 drivers/net/dsa/microchip/ksz_common.c | 118 ++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   4 +
 drivers/net/dsa/microchip/ksz_priv.h   |   8 +-
 4 files changed, 214 insertions(+), 45 deletions(-)
drivers/net/dsa/microchip/ksz9477.c +88 -41
index e24dd14ccde77..ace8f2e3c781d 100644
@@ -259,6 +259,74 @@ 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;
+
+	/* retain the flush/freeze bit */
+	ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4, &data);
+	data &= MIB_COUNTER_FLUSH_FREEZE;
+
+	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 = 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)
+{
+	/* enable the port for flush/freeze function */
+	if (freeze)
+		ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
+			     MIB_COUNTER_FLUSH_FREEZE);
+	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, freeze);
+
+	/* disable the port after freeze is done */
+	if (!freeze)
+		ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, 0);
+}
+
+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 */
+	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);
+
+	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 +410,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)
 {

@@ -969,6 +996,17 @@ 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.
+		 */
+		phy->advertising = phy->supported;
+	}
+}
+
 static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
 	u8 data8;

@@ -1143,6 +1181,8 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	/* start switch */
 	ksz_cfg(dev, REG_SW_OPERATION, SW_START, true);
 
+	ksz_init_mib_timer(dev);
+
 	return 0;
 }
 

@@ -1151,6 +1191,7 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.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,

@@ -1276,6 +1317,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) *

@@ -1298,7 +1340,12 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.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,
+	.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,
drivers/net/dsa/microchip/ksz_common.c +118 -0

148 lines not on this page · raw message

drivers/net/dsa/microchip/ksz_common.h +4 -0
index 2dd832de0d526..547859ed39269 100644
@@ -8,12 +8,16 @@
 #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 */
 
 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);
+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,
drivers/net/dsa/microchip/ksz_priv.h +4 -4
index 6dd2ebfd6e12f..bd39c04a0f82f 100644
@@ -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;
 };

@@ -79,8 +78,6 @@ struct ksz_device {
 
 	struct vlan_table *vlan_cache;
 
-	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
 	u8 *txbuf;
 
 	struct ksz_port *ports;

@@ -137,6 +134,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);

@@ -151,6 +150,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);
Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 2/5 net: dsa: ksz: Rename NET_DSA_TAG_KSZ to _KSZ9477

From: Tristram Ha <[email protected]>

Rename the tag Kconfig option and related macros in preparation for addition of new KSZ family switches with different tag formats.

Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 drivers/net/dsa/microchip/Kconfig   | 2 +-
 drivers/net/dsa/microchip/ksz9477.c | 2 +-
 include/net/dsa.h                   | 2 +-
 net/dsa/Kconfig                     | 4 ++++
 net/dsa/dsa.c                       | 4 ++--
 net/dsa/dsa_priv.h                  | 2 +-
 net/dsa/tag_ksz.c                   | 2 +-
 7 files changed, 11 insertions(+), 7 deletions(-)
drivers/net/dsa/microchip/Kconfig +1 -1
index a8caf9249d50f..bea29fde9f3d1 100644
@@ -4,7 +4,7 @@ config NET_DSA_MICROCHIP_KSZ_COMMON
 menuconfig NET_DSA_MICROCHIP_KSZ9477
 	tristate "Microchip KSZ9477 series switch support"
 	depends on NET_DSA
-	select NET_DSA_TAG_KSZ
+	select NET_DSA_TAG_KSZ9477
 	select NET_DSA_MICROCHIP_KSZ_COMMON
 	help
 	  This driver adds support for Microchip KSZ9477 switch chips.
drivers/net/dsa/microchip/ksz9477.c +1 -1
index ace8f2e3c781d..547bfb097dee7 100644
@@ -330,7 +330,7 @@ static void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
 static enum dsa_tag_protocol ksz9477_get_tag_protocol(struct dsa_switch *ds,
 						      int port)
 {
-	return DSA_TAG_PROTO_KSZ;
+	return DSA_TAG_PROTO_KSZ9477;
 }
 
 static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
include/net/dsa.h +1 -1
index 7a03274ea981b..dff6afc22ab1e 100644
@@ -35,7 +35,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_BRCM_PREPEND,
 	DSA_TAG_PROTO_DSA,
 	DSA_TAG_PROTO_EDSA,
-	DSA_TAG_PROTO_KSZ,
+	DSA_TAG_PROTO_KSZ9477,
 	DSA_TAG_PROTO_LAN9303,
 	DSA_TAG_PROTO_MTK,
 	DSA_TAG_PROTO_QCA,
net/dsa/Kconfig +4 -0
index 4183e4ba27a50..8cdf73a31374e 100644
@@ -41,6 +41,10 @@ config NET_DSA_TAG_EDSA
 config NET_DSA_TAG_KSZ
 	bool
 
+config NET_DSA_TAG_KSZ9477
+	bool
+	select NET_DSA_TAG_KSZ
+
 config NET_DSA_TAG_LAN9303
 	bool
net/dsa/dsa.c +2 -2
index 9f3209ff7ffde..4d4a381367d4d 100644
@@ -52,8 +52,8 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = {
 #ifdef CONFIG_NET_DSA_TAG_EDSA
 	[DSA_TAG_PROTO_EDSA] = &edsa_netdev_ops,
 #endif
-#ifdef CONFIG_NET_DSA_TAG_KSZ
-	[DSA_TAG_PROTO_KSZ] = &ksz_netdev_ops,
+#ifdef CONFIG_NET_DSA_TAG_KSZ9477
+	[DSA_TAG_PROTO_KSZ9477] = &ksz9477_netdev_ops,
 #endif
 #ifdef CONFIG_NET_DSA_TAG_LAN9303
 	[DSA_TAG_PROTO_LAN9303] = &lan9303_netdev_ops,
net/dsa/dsa_priv.h +1 -1
index 3964c6f7a7c0d..b48b533294544 100644
@@ -206,7 +206,7 @@ extern const struct dsa_device_ops dsa_netdev_ops;
 extern const struct dsa_device_ops edsa_netdev_ops;
 
 /* tag_ksz.c */
-extern const struct dsa_device_ops ksz_netdev_ops;
+extern const struct dsa_device_ops ksz9477_netdev_ops;
 
 /* tag_lan9303.c */
 extern const struct dsa_device_ops lan9303_netdev_ops;
net/dsa/tag_ksz.c +1 -1
index cad4406d9d4c2..036bc62198f28 100644
@@ -96,7 +96,7 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 }
 
-const struct dsa_device_ops ksz_netdev_ops = {
+const struct dsa_device_ops ksz9477_netdev_ops = {
 	.xmit	= ksz_xmit,
 	.rcv	= ksz_rcv,
 	.overhead = KSZ_INGRESS_TAG_LEN,
Signature · 1 line
2.18.0
author
#2 lore raw

PATCH 3/5 net: dsa: ksz: Factor out common tag code

In reply to Marek Vasut

From: Tristram Ha <[email protected]>

Factor out common code from the tag_ksz , so that the code can be used with other KSZ family switches which use differenly sized tags.

Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 net/dsa/tag_ksz.c | 125 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 35 deletions(-)
net/dsa/tag_ksz.c +90 -35
index 036bc62198f28..d94bad1ab7e53 100644
@@ -14,34 +14,30 @@
 #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)
- */
+struct ksz_tag_ops {
+	void	(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
+	void	(*set_tag)(struct sk_buff *skb, struct net_device *dev);
+};
 
-#define	KSZ_INGRESS_TAG_LEN	2
-#define	KSZ_EGRESS_TAG_LEN	1
+/* Typically only one byte is used for tail tag. */
+#define KSZ_EGRESS_TAG_LEN		1
 
-static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
+/* Frames with following addresse may need to be sent even when the port is
+ * closed.
+ */
+static const u8 special_mult_addr[] = {
+	0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
+};
+
+static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev,
+				struct ksz_tag_ops *ops, int len)
 {
-	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct sk_buff *nskb;
 	int padlen;
-	u8 *tag;
 
 	padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_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 +45,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,34 +66,93 @@ 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 */
+	ops->set_tag(nskb, dev);
 
 	return nskb;
 }
 
 static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt)
+			       struct packet_type *pt, struct ksz_tag_ops *ops)
 {
-	u8 *tag;
-	int source_port;
+	int sp, len;
+	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
 
-	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+	ops->get_tag(tag, &sp, &len);
 
-	source_port = tag[0] & 7;
-
-	skb->dev = dsa_master_find_slave(dev, 0, source_port);
+	skb->dev = dsa_master_find_slave(dev, 0, sp);
 	if (!skb->dev)
 		return NULL;
 
-	pskb_trim_rcsum(skb, skb->len - KSZ_EGRESS_TAG_LEN);
+	pskb_trim_rcsum(skb, skb->len - len);
 
 	return skb;
 }
 
+/*
+ * For Ingress (Host -> KSZ9477), 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 (KSZ9477 -> 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 KSZ9477_INGRESS_TAG_LEN		2
+#define KSZ9477_PTP_TAG_LEN		4
+#define KSZ9477_PTP_TAG_INDICATION	0x80
+
+#define KSZ9477_TAIL_TAG_OVERRIDE	BIT(9)
+#define KSZ9477_TAIL_TAG_LOOKUP		BIT(10)
+
+static void ksz9477_get_tag(u8 *tag, unsigned int *port, unsigned int *len)
+{
+	*port = tag[0] & 7;
+	*len = KSZ_EGRESS_TAG_LEN;
+
+	/* Extra 4-bytes PTP timestamp */
+	if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
+		*len += KSZ9477_PTP_TAG_LEN;
+}
+
+static void ksz9477_set_tag(struct sk_buff *skb, struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	u16 *tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN);
+	u8 *addr = skb_mac_header(skb);
+
+	*tag = 1 << dp->index;
+	if (!memcmp(addr, special_mult_addr, ETH_ALEN))
+		*tag |= KSZ9477_TAIL_TAG_OVERRIDE;
+
+	*tag = cpu_to_be16(*tag);
+}
+
+static struct ksz_tag_ops ksz9477_tag_ops = {
+	.get_tag	= ksz9477_get_tag,
+	.set_tag	= ksz9477_set_tag,
+};
+
+static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	return ksz_xmit(skb, dev, &ksz9477_tag_ops, KSZ9477_INGRESS_TAG_LEN);
+}
+
+static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev,
+				   struct packet_type *pt)
+{
+	return ksz_rcv(skb, dev, pt, &ksz9477_tag_ops);
+}
+
 const struct dsa_device_ops ksz9477_netdev_ops = {
-	.xmit	= ksz_xmit,
-	.rcv	= ksz_rcv,
-	.overhead = KSZ_INGRESS_TAG_LEN,
+	.xmit	= ksz9477_xmit,
+	.rcv	= ksz9477_rcv,
+	.overhead = KSZ9477_INGRESS_TAG_LEN,
 };
Signature · 1 line
2.18.0
maintainer
#3 lore raw

Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code

Quoted 4 lines · Marek Vasut

From: Tristram Ha <[email protected]>

Factor out common code from the tag_ksz , so that the code can be used with other KSZ family switches which use differenly sized tags.

I prefer this implementation over what Tristram recently submitted. It is also what we suggested a while back. However, since then we have had Spectra/meltdown, and we now know a function call through a pointer is expensive. This is the hot path, every frame comes through here, so it is worth taking the time to optimize this. Could you try to remove the ksz_tag_ops structure. xmit looks simple, since it is a tail call, so you can do that in ksz9477_xmit. Receive looks a bit more complex.

I also think for the moment we need it ignore PTP until we have the big picture sorted out. If need be, the code can be refactored yet again. But i don't want PTP holding up getting more switches supported.

Quoted 39 lines
Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 net/dsa/tag_ksz.c | 125 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 35 deletions(-)
net/dsa/tag_ksz.c +5 -16
index 036bc62198f28..d94bad1ab7e53 100644
@@ -14,34 +14,30 @@
 #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)
- */
+struct ksz_tag_ops {
+	void	(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
+	void	(*set_tag)(struct sk_buff *skb, struct net_device *dev);
+};
 
-#define	KSZ_INGRESS_TAG_LEN	2
-#define	KSZ_EGRESS_TAG_LEN	1
+/* Typically only one byte is used for tail tag. */

net/dsa/tag_ksz.c line 31

Quoted 10 lines
net/dsa/tag_ksz.c +8 -1
+#define KSZ_EGRESS_TAG_LEN		1
 
-static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
+/* Frames with following addresse may need to be sent even when the port is
+ * closed.
+ */
+static const u8 special_mult_addr[] = {
+	0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
+};
+

This is new functionality, not part of the refactoring the code. Please add that as a new patch. Also, you can probably make use of is_link_local_ether_addr().

Andrew

#4 lore raw

RE: [PATCH 3/5] net: dsa: ksz: Factor out common tag code

Quoted 15 lines · Marek Vasut
Quoted 4 lines

From: Tristram Ha <[email protected]>

Factor out common code from the tag_ksz , so that the code can be used with other KSZ family switches which use differenly sized tags.

I prefer this implementation over what Tristram recently submitted. It is also what we suggested a while back. However, since then we have had Spectra/meltdown, and we now know a function call through a pointer is expensive. This is the hot path, every frame comes through here, so it is worth taking the time to optimize this. Could you try to remove the ksz_tag_ops structure. xmit looks simple, since it is a tail call, so you can do that in ksz9477_xmit. Receive looks a bit more complex.

Quoted 4 lines

I also think for the moment we need it ignore PTP until we have the big picture sorted out. If need be, the code can be refactored yet again. But i don't want PTP holding up getting more switches supported.

Yes, as you may already know, what Marek submitted was my previous attempt to support different switches in tag_ksz.c.

So this ksz_tag_ops is still not acceptable? As I understand the kernel is using this mechanism all over the places.

What is left is a direct copying of the transmit and receive functions for each new switch tail tag format.

author
#5 lore raw

Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code

Quoted 15 lines · On 12/10/2018 09:32 PM, [email protected] wrote:
Quoted 15 lines · Marek Vasut
Quoted 4 lines

From: Tristram Ha <[email protected]>

Factor out common code from the tag_ksz , so that the code can be used with other KSZ family switches which use differenly sized tags.

I prefer this implementation over what Tristram recently submitted. It is also what we suggested a while back. However, since then we have had Spectra/meltdown, and we now know a function call through a pointer is expensive. This is the hot path, every frame comes through here, so it is worth taking the time to optimize this. Could you try to remove the ksz_tag_ops structure. xmit looks simple, since it is a tail call, so you can do that in ksz9477_xmit. Receive looks a bit more complex.

Quoted 7 lines
Quoted 4 lines

I also think for the moment we need it ignore PTP until we have the big picture sorted out. If need be, the code can be refactored yet again. But i don't want PTP holding up getting more switches supported.

Yes, as you may already know, what Marek submitted was my previous attempt to support different switches in tag_ksz.c.

I adjusted the code quite a bit, please look again.

Quoted 5 lines

So this ksz_tag_ops is still not acceptable? As I understand the kernel is using this mechanism all over the places.

What is left is a direct copying of the transmit and receive functions for each new switch tail tag format.

Maybe a macro would work ? Or some code runtime-patching ?

Signature · 2 lines
Best regards,
Marek Vasut
maintainer
#6 lore raw

Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code

In reply to [email protected]
Quoted 2 lines

So this ksz_tag_ops is still not acceptable? As I understand the kernel is using this mechanism all over the places.

Hi Tristram

It is used all other the place, but generally, not in the hot path, just the control plain.

Quoted 2 lines

What is left is a direct copying of the transmit and receive functions for each new switch tail tag format.

As i said, one of these looks easy to achieve, the other i did not look at too closely, but needs more work.

Andrew

author
#7 lore raw

Re: [PATCH 3/5] net: dsa: ksz: Factor out common tag code

In reply to Andrew Lunn
Quoted 6 lines · Andrew Lunn
Quoted 4 lines · Marek Vasut

From: Tristram Ha <[email protected]>

Factor out common code from the tag_ksz , so that the code can be used with other KSZ family switches which use differenly sized tags.

Quoted 8 lines

I prefer this implementation over what Tristram recently submitted. It is also what we suggested a while back. However, since then we have had Spectra/meltdown, and we now know a function call through a pointer is expensive. This is the hot path, every frame comes through here, so it is worth taking the time to optimize this. Could you try to remove the ksz_tag_ops structure. xmit looks simple, since it is a tail call, so you can do that in ksz9477_xmit. Receive looks a bit more complex.

They're both simple in the end, I'll send V2 once I test it a bit.

Quoted 46 lines

I also think for the moment we need it ignore PTP until we have the big picture sorted out. If need be, the code can be refactored yet again. But i don't want PTP holding up getting more switches supported.

Quoted 41 lines
Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 net/dsa/tag_ksz.c | 125 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 35 deletions(-)
net/dsa/tag_ksz.c +6 -16
index 036bc62198f28..d94bad1ab7e53 100644
@@ -14,34 +14,30 @@
 #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)
- */
+struct ksz_tag_ops {
+	void	(*get_tag)(u8 *tag, unsigned int *port, unsigned int *len);
+	void	(*set_tag)(struct sk_buff *skb, struct net_device *dev);
+};
 
-#define	KSZ_INGRESS_TAG_LEN	2
-#define	KSZ_EGRESS_TAG_LEN	1
+/* Typically only one byte is used for tail tag. */
+#define KSZ_EGRESS_TAG_LEN		1
Quoted 12 lines

net/dsa/tag_ksz.c line 31

Quoted 8 lines
net/dsa/tag_ksz.c +7 -1
-static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
+/* Frames with following addresse may need to be sent even when the port is
+ * closed.
+ */
+static const u8 special_mult_addr[] = {
+	0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
+};
+

This is new functionality, not part of the refactoring the code. Please add that as a new patch. Also, you can probably make use of is_link_local_ether_addr().

Sounds good, will do in V2.

Signature · 2 lines
Best regards,
Marek Vasut
author
#8 lore raw

PATCH 4/5 net: dsa: ksz: Add KSZ8795 tag code

In reply to Marek Vasut

From: Tristram Ha <[email protected]>

Add DSA tag code for Microchip KSZ8795 switch. The switch is simpler and the tag is only 1 byte, instead of 2 as is the case with KSZ9477.

Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 include/net/dsa.h  |  1 +
 net/dsa/Kconfig    |  4 ++++
 net/dsa/dsa.c      |  3 +++
 net/dsa/dsa_priv.h |  1 +
 net/dsa/tag_ksz.c  | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+)
include/net/dsa.h +1 -0
index dff6afc22ab1e..7aee9c33ceb78 100644
@@ -35,6 +35,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_BRCM_PREPEND,
 	DSA_TAG_PROTO_DSA,
 	DSA_TAG_PROTO_EDSA,
+	DSA_TAG_PROTO_KSZ8795,
 	DSA_TAG_PROTO_KSZ9477,
 	DSA_TAG_PROTO_LAN9303,
 	DSA_TAG_PROTO_MTK,
net/dsa/Kconfig +4 -0
index 8cdf73a31374e..0059237d50ec3 100644
@@ -41,6 +41,10 @@ config NET_DSA_TAG_EDSA
 config NET_DSA_TAG_KSZ
 	bool
 
+config NET_DSA_TAG_KSZ8795
+	bool
+	select NET_DSA_TAG_KSZ
+
 config NET_DSA_TAG_KSZ9477
 	bool
 	select NET_DSA_TAG_KSZ
net/dsa/dsa.c +3 -0
index 4d4a381367d4d..bf009192b0d53 100644
@@ -52,6 +52,9 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = {
 #ifdef CONFIG_NET_DSA_TAG_EDSA
 	[DSA_TAG_PROTO_EDSA] = &edsa_netdev_ops,
 #endif
+#ifdef CONFIG_NET_DSA_TAG_KSZ8795
+	[DSA_TAG_PROTO_KSZ8795] = &ksz8795_netdev_ops,
+#endif
 #ifdef CONFIG_NET_DSA_TAG_KSZ9477
 	[DSA_TAG_PROTO_KSZ9477] = &ksz9477_netdev_ops,
 #endif
net/dsa/dsa_priv.h +1 -0
index b48b533294544..903c5118d5cbf 100644
@@ -206,6 +206,7 @@ extern const struct dsa_device_ops dsa_netdev_ops;
 extern const struct dsa_device_ops edsa_netdev_ops;
 
 /* tag_ksz.c */
+extern const struct dsa_device_ops ksz8795_netdev_ops;
 extern const struct dsa_device_ops ksz9477_netdev_ops;
 
 /* tag_lan9303.c */
net/dsa/tag_ksz.c +59 -0

65 lines not on this page · raw message

Signature · 1 line
2.18.0
author
#9 lore raw

PATCH 5/5 net: dsa: ksz: Add Microchip KSZ8795 DSA driver

In reply to Marek Vasut

From: Tristram Ha <[email protected]>

Add Microchip KSZ8795 DSA driver.

Signed-off-by Tristram Ha <[email protected]>
Signed-off-by Marek Vasut <[email protected]>
Cc Vivien Didelot <[email protected]>
Cc Woojung Huh <[email protected]>
Cc David S. Miller <[email protected]>
---
 drivers/net/dsa/microchip/Kconfig       |   17 +
 drivers/net/dsa/microchip/Makefile      |    2 +
 drivers/net/dsa/microchip/ksz8795.c     | 1351 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz8795_reg.h | 1016 +++++++++++++++++
 drivers/net/dsa/microchip/ksz8795_spi.c |  166 +++
 drivers/net/dsa/microchip/ksz_priv.h    |    1 +
 6 files changed, 2553 insertions(+)
 create mode 100644 drivers/net/dsa/microchip/ksz8795.c
 create mode 100644 drivers/net/dsa/microchip/ksz8795_reg.h
 create mode 100644 drivers/net/dsa/microchip/ksz8795_spi.c
drivers/net/dsa/microchip/Kconfig +17 -0
index bea29fde9f3d1..d17aec084c8c7 100644
@@ -14,3 +14,20 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
 	depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
 	help
 	  Select to enable support for registering switches configured through SPI.
+
+menuconfig NET_DSA_MICROCHIP_KSZ8795
+	tristate "Microchip KSZ8795 series switch support"
+	depends on NET_DSA
+	select NET_DSA_TAG_KSZ8795
+	select NET_DSA_MICROCHIP_KSZ_COMMON
+	help
+	  This driver adds support for Microchip KSZ8795 switch chips.
+
+config NET_DSA_MICROCHIP_KSZ8795_SPI
+	tristate "KSZ8795 series SPI connected switch driver"
+	depends on NET_DSA_MICROCHIP_KSZ8795 && SPI
+	help
+	  This driver accesses KSZ8795 chip through SPI.
+
+	  It is required to use the KSZ8795 switch driver as the only access
+	  is through SPI.
drivers/net/dsa/microchip/Makefile +2 -0
index 3142c18b8f573..18ab64172e0bb 100644
@@ -1,3 +1,5 @@
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)	+= ksz_common.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477)		+= ksz9477.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)	+= ksz9477_spi.o
+obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795)		+= ksz8795.o
+obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795_SPI)	+= ksz8795_spi.o
drivers/net/dsa/microchip/ksz8795.c new file +1351 -0

1,351 lines not on this page · raw message

drivers/net/dsa/microchip/ksz8795_reg.h new file +1016 -0

1,016 lines not on this page · raw message

drivers/net/dsa/microchip/ksz8795_spi.c new file +166 -0

166 lines not on this page · raw message

drivers/net/dsa/microchip/ksz_priv.h +1 -0
index bd39c04a0f82f..adafbe7cc17ea 100644
@@ -164,6 +164,7 @@ int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops);
 void ksz_switch_remove(struct ksz_device *dev);
 
+int ksz8795_switch_register(struct ksz_device *dev);
 int ksz9477_switch_register(struct ksz_device *dev);
 
 #endif
Signature · 1 line
2.18.0
maintainer
#10 lore raw

Re: [PATCH 5/5] net: dsa: ksz: Add Microchip KSZ8795 DSA driver

drivers/net/dsa/microchip/ksz8795.c line 263

Quoted 8 lines
drivers/net/dsa/microchip/ksz8795.c new file +8 -0
+static int ksz8795_valid_dyn_entry(struct ksz_device *dev, u8 *data)
+{
+	int timeout = 100;
+
+	do {
+		ksz_read8(dev, REG_IND_DATA_CHECK, data);
+		timeout--;
+	} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);

readx_poll_timeout()?

Quoted 1 line

+static inline void ksz8795_from_vlan(u16 vlan, u8 *fid, u8 *member, u8 *valid)

Please don't use inline in C code, just in headers. Leave the compile to decide if it should be inlined.

drivers/net/dsa/microchip/ksz8795.c line 429

Quoted 6 lines
drivers/net/dsa/microchip/ksz8795.c new file +6 -0
+static void ksz8795_r_vlan_table(struct ksz_device *dev, u16 vid, u16 *vlan)
+{
+	u64 buf;
+	u16 *data = (u16 *)&buf;
+	u16 addr;
+	int index;

The networking code uses reverse christmas tree. So you need to change the order of these declarations, and do the assignment in the body of the function. Please review all the functions.

drivers/net/dsa/microchip/ksz8795.c line 668

Quoted 3 lines
drivers/net/dsa/microchip/ksz8795.c new file +3 -0
+static const u8 stp_multicast_addr[] = {
+	0x01, 0x80, 0xC2, 0x00, 0x00, 0x00
+};

include/linux/etherdevice.h defines eth_stp_addr.

Andrew

maintainer
#11 lore raw
In reply to Marek Vasut

Every patch series should have a header posting with Subject of the form "[PATCH 0/N] ..." explaining what the series does at a high level, how it does it, and why it does it that way.

maintainer
#12 lore raw
In reply to Marek Vasut

drivers/net/dsa/microchip/ksz9477.c static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port, · line 1009

Quoted 11 lines
drivers/net/dsa/microchip/ksz9477.c +11 -0
+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.
+		 */
+		phy->advertising = phy->supported;
+	}
+}
+

This has nothing to do with MIB counters.

Other than that, is this the same as Tristram's recent submission? Maybe once the comments have been addressed, we can merge that version?

Thanks
Andrew

author
#13 lore raw
Quoted 14 lines · Andrew Lunn

drivers/net/dsa/microchip/ksz9477.c static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port, · line 1009

Quoted 11 lines
drivers/net/dsa/microchip/ksz9477.c +11 -0
+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.
+		 */
+		phy->advertising = phy->supported;
+	}
+}
+

This has nothing to do with MIB counters.

Quoted 3 lines

Other than that, is this the same as Tristram's recent submission? Maybe once the comments have been addressed, we can merge that version?

I was not CCed on his recent submission, link ?

Signature · 2 lines
Best regards,
Marek Vasut