net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver

All of lore.kernel.org
 help / color / mirror / Atom feed

* net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver
@ 2017-04-05  9:20 Juergen Borleis
  2017-04-05  9:20 ` [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format Juergen Borleis
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Juergen Borleis @ 2017-04-05  9:20 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem

The LAN9303 is a three port 10/100 ethernet switch with integrated phys
for the two external ethernet ports. The third port is an RMII/MII
interface to a host master network interface (e.g. fixed link).

While the LAN9303 device itself supports offload packet processing, this
driver does not make use of it yet. This driver just configures the device
to provide two separate network interfaces (which is the default state of
a DSA device).

Please note: the "MDIO managed mode" driver part isn't tested yet. I have
used and tested the "I2C managed mode" only.

Comments are welcome.

jb

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format
  2017-04-05  9:20 net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver Juergen Borleis
@ 2017-04-05  9:20 ` Juergen Borleis
  2017-04-05 17:10   ` Andrew Lunn
  2017-04-05  9:20 ` [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303 Juergen Borleis
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-05  9:20 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem

To define the outgoing port and to discover the incoming port a regular
VLAN tag is used by the LAN9303. But its VID meaning is 'special'.

This tag handler/filter depends on some hardware features which must be
enabled in the device to provide and make use of this special VLAN tag
to control the destination and the source of an ethernet packet.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 include/net/dsa.h     |   1 +
 net/dsa/Kconfig       |   3 +
 net/dsa/Makefile      |   1 +
 net/dsa/dsa.c         |   3 +
 net/dsa/dsa_priv.h    |   3 +
 net/dsa/tag_lan9303.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 166 insertions(+)
 create mode 100644 net/dsa/tag_lan9303.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4e13e695f0251..4fb1f2b086b05 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -31,6 +31,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_EDSA,
 	DSA_TAG_PROTO_BRCM,
 	DSA_TAG_PROTO_QCA,
+	DSA_TAG_PROTO_LAN9303,
 	DSA_TAG_LAST,		/* MUST BE LAST */
 };
 
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 9649238eef404..22c8bd69ff71c 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -31,4 +31,7 @@ config NET_DSA_TAG_TRAILER
 config NET_DSA_TAG_QCA
 	bool
 
+config NET_DSA_TAG_LAN9303
+	bool
+
 endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 31d343796251d..aafc74f2cb193 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -8,3 +8,4 @@ dsa_core-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
 dsa_core-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
+dsa_core-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index b6d4f6a23f06c..f93f78de23af3 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -53,6 +53,9 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = {
 #ifdef CONFIG_NET_DSA_TAG_QCA
 	[DSA_TAG_PROTO_QCA] = &qca_netdev_ops,
 #endif
+#ifdef CONFIG_NET_DSA_TAG_LAN9303
+	[DSA_TAG_PROTO_LAN9303] = &lan9303_netdev_ops,
+#endif
 	[DSA_TAG_PROTO_NONE] = &none_ops,
 };
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 0706a511244e9..a54cfc8aefa83 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -85,4 +85,7 @@ extern const struct dsa_device_ops brcm_netdev_ops;
 /* tag_qca.c */
 extern const struct dsa_device_ops qca_netdev_ops;
 
+/* tag_lan9303.c */
+extern const struct dsa_device_ops lan9303_netdev_ops;
+
 #endif
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
new file mode 100644
index 0000000000000..ad04c6d447f77
--- /dev/null
+++ b/net/dsa/tag_lan9303.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <jbe@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/etherdevice.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include "dsa_priv.h"
+
+/* To define the outgoing port and to discover the incoming port a regular
+ * VLAN tag is used by the LAN9303. But its VID meaning is 'special':
+ *
+ *       Dest MAC       Src MAC        TAG    Type
+ * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 | 1 2 |...
+ *                                |<------->|
+ * TAG:
+ *    |<------------->|
+ *    |  1  2 | 3  4  |
+ *      TPID    VID
+ *     0x8100
+ *
+ * VID bit 3 indicates a request for an ALR lookup.
+ *
+ * If VID bit 3 is zero, then bits 0 and 1 specify the destination port
+ * (0, 1, 2) or broadcast (3) or the source port (1, 2).
+ *
+ * VID bit 4 is used to specify if the STP port state should be overridden.
+ * Required when no forwarding between the external ports should happen.
+ */
+
+#define LAN9303_TAG_LEN 4
+
+static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	u16 *lan9303_tag;
+
+	/* insert a special VLAN tag between the MAC addresses
+	 * and the current ethertype field.
+	 */
+	if (skb_cow_head(skb, LAN9303_TAG_LEN) < 0) {
+		dev_dbg(&dev->dev,
+			"Cannot make room for the special tag. Dropping packet\n");
+		goto out_free;
+	}
+
+	/* provide 'LAN9303_TAG_LEN' bytes additional space */
+	skb_push(skb, LAN9303_TAG_LEN);
+
+	/* make room between MACs and Ether-Type */
+	memmove(skb->data, skb->data + LAN9303_TAG_LEN, 2 * ETH_ALEN);
+
+	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
+	lan9303_tag[0] = htons(ETH_P_8021Q);
+	lan9303_tag[1] = htons(p->dp->index | BIT(4));
+
+	return skb;
+out_free:
+	kfree_skb(skb);
+	return NULL;
+}
+
+static int lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
+		       struct packet_type *pt, struct net_device *orig_dev)
+{
+	u16 *lan9303_tag;
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds = dst->ds[0];
+	unsigned int source_port;
+
+	if (unlikely(!dst)) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet, due to missing switch tree device\n");
+		goto out_drop;
+	}
+
+	if (unlikely(!ds)) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet, due to missing DSA switch device\n");
+		goto out_drop;
+	}
+
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (!skb) {
+		dev_warn_ratelimited(&dev->dev, "Cannot post-process skb: unshareable\n");
+		goto out;
+	}
+
+	if (unlikely(!pskb_may_pull(skb, 2 + 2))) {
+		dev_warn_ratelimited(&dev->dev,
+				     "Dropping packet, cannot pull\n");
+		goto out_drop;
+	}
+
+	/* '->data' points into the middle of our special VLAN tag information:
+	 *
+	 * ~ MAC src   | 0x81 | 0x00 | 0xyy | 0xzz | ether type
+	 *                           ^
+	 *                        ->data
+	 */
+	lan9303_tag = (u16 *)(skb->data - 2);
+
+	if (lan9303_tag[0] != htons(0x8100)) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid VLAN marker\n");
+		goto out_drop;
+	}
+
+	source_port = ntohs(lan9303_tag[1]) & 0x3;
+
+	if (source_port >= DSA_MAX_PORTS) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid source port\n");
+		goto out_drop;
+	}
+
+	if (!ds->ports[source_port].netdev) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid netdev or device\n");
+		goto out_drop;
+	}
+
+	/* remove the special VLAN tag between the MAC addresses
+	 * and the current ethertype field.
+	 */
+	skb_pull_rcsum(skb, 2 + 2);
+	memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
+		2 * ETH_ALEN);
+	skb_push(skb, ETH_HLEN);
+
+	/* forward the packet to the dedicated interface */
+	skb->dev = ds->ports[source_port].netdev;
+	skb->pkt_type = PACKET_HOST;
+	skb->protocol = eth_type_trans(skb, skb->dev);
+
+	skb->dev->stats.rx_packets++;
+	skb->dev->stats.rx_bytes += skb->len;
+
+	netif_receive_skb(skb);
+
+	return 0;
+out_drop:
+	kfree_skb(skb);
+out:
+	return 0;
+}
+
+const struct dsa_device_ops lan9303_netdev_ops = {
+	.xmit = lan9303_xmit,
+	.rcv = lan9303_rcv,
+};
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-05  9:20 net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver Juergen Borleis
  2017-04-05  9:20 ` [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format Juergen Borleis
@ 2017-04-05  9:20 ` Juergen Borleis
  2017-04-05 18:12   ` Andrew Lunn
  2017-04-05  9:20 ` [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support Juergen Borleis
  2017-04-05  9:20 ` [PATCH 4/4] net: dsa: LAN9303: add MDIO " Juergen Borleis
  3 siblings, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-05  9:20 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem

The SMSC/Microchip LAN9303 is an ethernet switch device with one CPU port
and two external ethernet ports with built-in phys.

This driver uses the DSA framework, but is currently only capable of
separating the two external ports. There is no offload support yet.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 drivers/net/phy/lan9303-core.c | 924 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/lan9303.h      |  21 +
 2 files changed, 945 insertions(+)
 create mode 100644 drivers/net/phy/lan9303-core.c
 create mode 100644 drivers/net/phy/lan9303.h

diff --git a/drivers/net/phy/lan9303-core.c b/drivers/net/phy/lan9303-core.c
new file mode 100644
index 0000000000000..1c4698f0e13dc
--- /dev/null
+++ b/drivers/net/phy/lan9303-core.c
@@ -0,0 +1,924 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regmap.h>
+#include <linux/mutex.h>
+
+#include "lan9303.h"
+
+#define LAN9303_CHIP_REV 0x14
+#define LAN9303_IRQ_CFG 0x15
+# define LAN9303_IRQ_CFG_IRQ_ENABLE BIT(8)
+# define LAN9303_IRQ_CFG_IRQ_POL BIT(4)
+# define LAN9303_IRQ_CFG_IRQ_TYPE BIT(0)
+#define LAN9303_INT_STS 0x16
+# define LAN9303_INT_STS_PHY_INT2 BIT(27)
+# define LAN9303_INT_STS_PHY_INT1 BIT(26)
+#define LAN9303_INT_EN 0x17
+# define LAN9303_INT_EN_PHY_INT2_EN BIT(27)
+# define LAN9303_INT_EN_PHY_INT1_EN BIT(26)
+#define LAN9303_HW_CFG 0x1D
+# define LAN9303_HW_CFG_READY BIT(27)
+# define LAN9303_HW_CFG_AMDX_EN_PORT2 BIT(26)
+# define LAN9303_HW_CFG_AMDX_EN_PORT1 BIT(25)
+#define LAN9303_PMI_DATA 0x29
+#define LAN9303_PMI_ACCESS 0x2A
+# define LAN9303_PMI_ACCESS_MII_BUSY BIT(0)
+# define LAN9303_PMI_ACCESS_MII_WRITE BIT(1)
+#define LAN9303_MANUAL_FC_1 0x68
+#define LAN9303_MANUAL_FC_2 0x69
+#define LAN9303_MANUAL_FC_0 0x6a
+#define LAN9303_SWITCH_CSR_DATA 0x6b
+#define LAN9303_SWITCH_CSR_CMD 0x6c
+#define LAN9303_SWITCH_CSR_CMD_BUSY BIT(31)
+#define LAN9303_SWITCH_CSR_CMD_RW BIT(30)
+#define LAN9303_SWITCH_CSR_CMD_LANES (BIT(19) | BIT(18) | BIT(17) | BIT(16))
+#define LAN9303_VIRT_PHY_BASE 0x70
+#define LAN9303_VIRT_SPECIAL_CTRL 0x77
+
+#define LAN9303_SW_DEV_ID 0x0000
+#define LAN9303_SW_RESET 0x0001
+#define LAN9303_SW_RESET_RESET BIT(0)
+#define LAN9303_SW_IMR 0x0004
+#define LAN9303_SW_IPR 0x0005
+#define LAN9303_MAC_VER_ID_0 0x0400
+#define LAN9303_MAC_RX_CFG_0 0x0401
+#define LAN9303_MAC_RX_UNDSZE_CNT_0 0x0410
+#define LAN9303_MAC_RX_64_CNT_0 0x0411
+#define LAN9303_MAC_RX_127_CNT_0 0x0412
+#define LAN9303_MAC_RX_255_CNT_0 0x413
+#define LAN9303_MAC_RX_511_CNT_0 0x0414
+#define LAN9303_MAC_RX_1023_CNT_0 0x0415
+#define LAN9303_MAC_RX_MAX_CNT_0 0x0416
+#define LAN9303_MAC_RX_OVRSZE_CNT_0 0x0417
+#define LAN9303_MAC_RX_PKTOK_CNT_0 0x0418
+#define LAN9303_MAC_RX_CRCERR_CNT_0 0x0419
+#define LAN9303_MAC_RX_MULCST_CNT_0 0x041a
+#define LAN9303_MAC_RX_BRDCST_CNT_0 0x041b
+#define LAN9303_MAC_RX_PAUSE_CNT_0 0x041c
+#define LAN9303_MAC_RX_FRAG_CNT_0 0x041d
+#define LAN9303_MAC_RX_JABB_CNT_0 0x041e
+#define LAN9303_MAC_RX_ALIGN_CNT_0 0x041f
+#define LAN9303_MAC_RX_PKTLEN_CNT_0 0x0420
+#define LAN9303_MAC_RX_GOODPKTLEN_CNT_0 0x0421
+#define LAN9303_MAC_RX_SYMBL_CNT_0 0x0422
+#define LAN9303_MAC_RX_CTLFRM_CNT_0 0x0423
+
+#define LAN9303_MAC_TX_CFG_0 0x0440
+#define LAN9303_MAC_TX_DEFER_CNT_0 0x0451
+#define LAN9303_MAC_TX_PAUSE_CNT_0 0x0452
+#define LAN9303_MAC_TX_PKTOK_CNT_0 0x0453
+#define LAN9303_MAC_TX_64_CNT_0 0x0454
+#define LAN9303_MAC_TX_127_CNT_0 0x0455
+#define LAN9303_MAC_TX_255_CNT_0 0x0456
+#define LAN9303_MAC_TX_511_CNT_0 0x0457
+#define LAN9303_MAC_TX_1023_CNT_0 0x0458
+#define LAN9303_MAC_TX_MAX_CNT_0 0x0459
+#define LAN9303_MAC_TX_UNDSZE_CNT_0 0x045a
+#define LAN9303_MAC_TX_PKTLEN_CNT_0 0x045c
+#define LAN9303_MAC_TX_BRDCST_CNT_0 0x045d
+#define LAN9303_MAC_TX_MULCST_CNT_0 0x045e
+#define LAN9303_MAC_TX_LATECOL_0 0x045f
+#define LAN9303_MAC_TX_EXCOL_CNT_0 0x0460
+#define LAN9303_MAC_TX_SNGLECOL_CNT_0 0x0461
+#define LAN9303_MAC_TX_MULTICOL_CNT_0 0x0462
+#define LAN9303_MAC_TX_TOTALCOL_CNT_0 0x0463
+
+#define LAN9303_MAC_VER_ID_1 0x0800
+#define LAN9303_MAC_RX_CFG_1 0x0801
+#define LAN9303_MAC_TX_CFG_1 0x0840
+#define LAN9303_MAC_VER_ID_2 0x0c00
+#define LAN9303_MAC_RX_CFG_2 0x0c01
+#define LAN9303_MAC_TX_CFG_2 0x0c40
+#define LAN9303_SWE_ALR_CMD 0x1800
+#define LAN9303_SWE_VLAN_CMD 0x180b
+# define LAN9303_SWE_VLAN_CMD_RNW BIT(5)
+# define LAN9303_SWE_VLAN_CMD_PVIDNVLAN BIT(4)
+#define LAN9303_SWE_VLAN_WR_DATA 0x180c
+#define LAN9303_SWE_VLAN_RD_DATA 0x180e
+# define LAN9303_SWE_VLAN_MEMBER_PORT2 BIT(17)
+# define LAN9303_SWE_VLAN_UNTAG_PORT2 BIT(16)
+# define LAN9303_SWE_VLAN_MEMBER_PORT1 BIT(15)
+# define LAN9303_SWE_VLAN_UNTAG_PORT1 BIT(14)
+# define LAN9303_SWE_VLAN_MEMBER_PORT0 BIT(13)
+# define LAN9303_SWE_VLAN_UNTAG_PORT0 BIT(12)
+#define LAN9303_SWE_VLAN_CMD_STS 0x1810
+#define LAN9303_SWE_GLB_INGRESS_CFG 0x1840
+#define LAN9303_SWE_PORT_STATE 0x1843
+# define LAN9303_SWE_PORT_STATE_FORWARDING_PORT2 (0)
+# define LAN9303_SWE_PORT_STATE_LEARNING_PORT2 BIT(5)
+# define LAN9303_SWE_PORT_STATE_BLOCKING_PORT2 BIT(4)
+# define LAN9303_SWE_PORT_STATE_FORWARDING_PORT1 (0)
+# define LAN9303_SWE_PORT_STATE_LEARNING_PORT1 BIT(3)
+# define LAN9303_SWE_PORT_STATE_BLOCKING_PORT1 BIT(2)
+# define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
+# define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
+# define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+#define LAN9303_SWE_PORT_MIRROR 0x1846
+# define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
+# define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
+# define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT1 BIT(6)
+# define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT0 BIT(5)
+# define LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT2 BIT(4)
+# define LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT1 BIT(3)
+# define LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT0 BIT(2)
+# define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
+# define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
+#define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
+#define LAN9303_BM_CFG 0x1c00
+#define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c
+# define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16))
+# define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
+# define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
+
+static const struct regmap_range lan9303_valid_regs[] = {
+	regmap_reg_range(0x14, 0x17), /* misc, interrupt */
+	regmap_reg_range(0x19, 0x19), /* endian test */
+	regmap_reg_range(0x1d, 0x1d), /* hardware config */
+	regmap_reg_range(0x23, 0x24), /* general purpose timer */
+	regmap_reg_range(0x27, 0x27), /* counter */
+	regmap_reg_range(0x29, 0x2a), /* PMI index regs */
+	regmap_reg_range(0x68, 0x6a), /* flow control */
+	regmap_reg_range(0x6b, 0x6c), /* switch fabric indirect regs */
+	regmap_reg_range(0x6d, 0x6f), /* misc */
+	regmap_reg_range(0x70, 0x77), /* virtual phy */
+	regmap_reg_range(0x78, 0x7a), /* GPIO */
+	regmap_reg_range(0x7c, 0x7e), /* MAC & reset */
+	regmap_reg_range(0x80, 0xb7), /* switch fabric direct regs */
+};
+
+static const struct regmap_range lan9303_reserved_ranges[] = {
+	regmap_reg_range(0x00, 0x13),
+	regmap_reg_range(0x18, 0x18),
+	regmap_reg_range(0x1a, 0x1c),
+	regmap_reg_range(0x1e, 0x22),
+	regmap_reg_range(0x25, 0x26),
+	regmap_reg_range(0x28, 0x28),
+	regmap_reg_range(0x2b, 0x67),
+	regmap_reg_range(0x7b, 0x7b),
+	regmap_reg_range(0x7f, 0x7f),
+	regmap_reg_range(0xb8, 0xff),
+};
+
+const struct regmap_access_table lan9303_register_set = {
+	.yes_ranges = lan9303_valid_regs,
+	.n_yes_ranges = ARRAY_SIZE(lan9303_valid_regs),
+	.no_ranges = lan9303_reserved_ranges,
+	.n_no_ranges = ARRAY_SIZE(lan9303_reserved_ranges),
+};
+
+static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
+{
+	int ret;
+
+	/* we can lose arbitration for the I2C case, because the device
+	 * tries to detect and read an external EEPROM after reset and acts as
+	 * a master on the shared I2C bus itself. This conflicts with our
+	 * attempts to access the device as a slave at the same moment.
+	 */
+	do {
+		ret = regmap_read(regmap, offset, reg);
+		if (ret == -EAGAIN)
+			msleep(500);
+	} while (ret == -EAGAIN);
+
+	return ret;
+}
+
+/* virtual phy registers must be read mapped */
+static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
+{
+	int ret;
+	u32 val;
+
+	if (regnum > 7)
+		return -EINVAL;
+
+	ret = lan9303_read(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, &val);
+	if (ret != 0)
+		return ret;
+
+	return val & 0xffff;
+}
+
+static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
+{
+	if (regnum > 7)
+		return -EINVAL;
+
+	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
+}
+
+static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
+{
+	int ret;
+	u32 reg;
+
+	/* transfer completed? */
+	do {
+		ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
+		if (ret != 0)
+			return ret;
+	} while (reg & LAN9303_PMI_ACCESS_MII_BUSY);
+
+	return 0;
+}
+
+static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
+{
+	int ret;
+	u32 val;
+
+	val = ((unsigned int)addr) << 11; /* setup phy ID */
+	val |= ((unsigned int)regnum) << 6;
+
+	mutex_lock(&chip->indirect_mutex);
+
+	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	if (ret != 0)
+		goto on_error;
+
+	/* start the MII read cycle */
+	ret = regmap_write(chip->regmap, LAN9303_PMI_ACCESS, val);
+	if (ret != 0)
+		goto on_error;
+
+	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	if (ret != 0)
+		goto on_error;
+
+	/* read the result of this operation */
+	ret = lan9303_read(chip->regmap, LAN9303_PMI_DATA, &val);
+	if (ret != 0)
+		goto on_error;
+
+	mutex_unlock(&chip->indirect_mutex);
+
+	return val & 0xffff;
+
+on_error:
+	mutex_unlock(&chip->indirect_mutex);
+	return ret;
+}
+
+static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
+				 unsigned int val)
+{
+	int ret;
+	u32 reg;
+
+	reg = ((unsigned int)addr) << 11; /* setup phy ID */
+	reg |= ((unsigned int)regnum) << 6;
+	reg |= LAN9303_PMI_ACCESS_MII_WRITE;
+
+	mutex_lock(&chip->indirect_mutex);
+
+	ret = lan9303_port_phy_reg_wait_for_completion(chip);
+	if (ret != 0)
+		goto on_error;
+
+	/* write the data first... */
+	ret = regmap_write(chip->regmap, LAN9303_PMI_DATA, val);
+	if (ret != 0)
+		goto on_error;
+
+	/* ...then start the MII write cycle */
+	ret = regmap_write(chip->regmap, LAN9303_PMI_ACCESS, reg);
+
+on_error:
+	mutex_unlock(&chip->indirect_mutex);
+	return ret;
+}
+
+static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
+{
+	int ret;
+	u32 reg;
+
+	/* transfer completed? */
+	do {
+		ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
+		if (ret) {
+			dev_err(chip->dev,
+				"Failed to read csr command status: %d\n",
+				ret);
+			return ret;
+		}
+	} while (reg & LAN9303_SWITCH_CSR_CMD_BUSY);
+
+	return 0;
+}
+
+static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
+{
+	u32 reg;
+	int ret;
+
+	reg = regnum;
+	reg |= LAN9303_SWITCH_CSR_CMD_LANES;
+	reg |= LAN9303_SWITCH_CSR_CMD_BUSY;
+
+	mutex_lock(&chip->indirect_mutex);
+
+	ret = lan9303_switch_wait_for_completion(chip);
+	if (ret)
+		goto on_error;
+
+	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_DATA, val);
+	if (ret) {
+		dev_err(chip->dev, "Failed to write csr data reg: %d\n", ret);
+		goto on_error;
+	}
+
+	/* trigger write */
+	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_CMD, reg);
+	if (ret)
+		dev_err(chip->dev, "Failed to write csr command reg: %d\n",
+			ret);
+
+on_error:
+	mutex_unlock(&chip->indirect_mutex);
+	return ret;
+}
+
+static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
+{
+	u32 reg;
+	int ret;
+
+	reg = regnum;
+	reg |= LAN9303_SWITCH_CSR_CMD_LANES;
+	reg |= LAN9303_SWITCH_CSR_CMD_RW;
+	reg |= LAN9303_SWITCH_CSR_CMD_BUSY;
+
+	mutex_lock(&chip->indirect_mutex);
+
+	ret = lan9303_switch_wait_for_completion(chip);
+	if (ret)
+		goto on_error;
+
+	/* trigger read */
+	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_CMD, reg);
+	if (ret) {
+		dev_err(chip->dev, "Failed to write csr command reg: %d\n",
+			ret);
+		goto on_error;
+	}
+
+	ret = lan9303_switch_wait_for_completion(chip);
+	if (ret)
+		goto on_error;
+
+	ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_DATA, val);
+	if (ret)
+		dev_err(chip->dev, "Failed to read csr data reg: %d\n",
+			ret);
+on_error:
+	mutex_unlock(&chip->indirect_mutex);
+	return ret;
+}
+
+static int lan9303_detect_phy_ids(struct lan9303 *chip)
+{
+	int reg;
+
+	/* depending on the 'phy_addr_sel_strap' setting, the three phys are
+	 * using IDs 0-1-2 or IDs 1-2-3. We cannot read back the
+	 * 'phy_addr_sel_strap' setting directly, so we need a test, which
+	 * configuration is active:
+	 * Register 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
+	 * and the IDs are 0-1-2, else it contains something different from
+	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
+	 */
+	reg = lan9303_port_phy_reg_read(chip, 3, 18);
+	if (reg < 0) {
+		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
+		return reg;
+	}
+
+	if (reg != 0)
+		chip->phy_addr_sel_strap = 1;
+	else
+		chip->phy_addr_sel_strap = 0;
+
+	dev_dbg(chip->dev, "Phy configuration '%s' detected\n",
+		chip->phy_addr_sel_strap ? "1-2-3" : "0-1-2");
+
+	return 0;
+}
+
+static void lan9303_report_virt_phy_config(struct lan9303 *chip,
+					   unsigned int reg)
+{
+	switch ((reg >> 8) & 0x03) {
+	case 0:
+		dev_info(chip->dev, "Virtual phy in 'MII MAC mode'\n");
+		break;
+	case 1:
+		dev_info(chip->dev, "Virtual phy in 'MII PHY mode'\n");
+		break;
+	case 2:
+		dev_info(chip->dev, "Virtual phy in 'RMII PHY mode'\n");
+		break;
+	case 3:
+		dev_err(chip->dev, "Invalid virtual phy mode\n");
+		break;
+	}
+
+	if (reg & BIT(6))
+		dev_info(chip->dev, "RMII clock is an output\n");
+	else
+		dev_info(chip->dev, "RMII clock is an input\n");
+}
+
+/* stop processing packets at all ports */
+static int lan9303_disable_switching(struct lan9303 *chip)
+{
+	int ret;
+
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_0, 0x02);
+	if (ret)
+		return ret;
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1, 0x02);
+	if (ret)
+		return ret;
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2, 0x02);
+	if (ret)
+		return ret;
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_0, 0x56);
+	if (ret)
+		return ret;
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1, 0x56);
+	if (ret)
+		return ret;
+	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2, 0x56);
+}
+
+/* start processing packets at CPU port */
+static int lan9303_enable_switching(struct lan9303 *chip)
+{
+	int ret;
+
+	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_0, 0x03);
+	if (ret)
+		return ret;
+
+	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_0, 0x57);
+}
+
+/* We want a special working switch:
+ * - do not route between port 1 and 2
+ * - route everything from port 1 to port 0
+ * - route everything from port 2 to port 0
+ * - route special tagged packets from port 0 to port 1 *or* port 2
+ */
+static int lan9303_separate_ports(struct lan9303 *chip)
+{
+	int ret;
+
+	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR,
+				LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT0 |
+				LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT1 |
+				LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT2 |
+				LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING |
+				LAN9303_SWE_PORT_MIRROR_SNIFF_ALL);
+	if (ret)
+		return ret;
+
+	/* enable defining the destination port via special VLAN tagging
+	 * for port 0
+	 */
+	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
+				       0x03);
+	if (ret)
+		return ret;
+
+	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
+	 * able to discover their source port
+	 */
+	ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
+			LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
+	if (ret)
+		return ret;
+
+	/* prevent port 1 and 2 from forwarding packets by their own */
+	return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
+				LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
+				LAN9303_SWE_PORT_STATE_BLOCKING_PORT1 |
+				LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
+}
+
+static int lan9303_handle_reset(struct lan9303 *chip)
+{
+	if (!chip->reset_gpio)
+		return 0;
+
+	gpiod_export_link(chip->dev, "reset", chip->reset_gpio);
+
+	if (chip->reset_duration != 0)
+		msleep(chip->reset_duration);
+
+	/* release (deassert) reset and activate the device */
+	gpiod_set_value_cansleep(chip->reset_gpio, 0);
+
+	return 0;
+}
+
+static int lan9303_check_device(struct lan9303 *chip)
+{
+	int ret;
+	u32 reg;
+
+	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read chip revision register: %d\n",
+			ret);
+#ifdef DEBUG
+		if (!chip->reset_gpio) {
+			dev_warn(chip->dev,
+				 "Maybe failed due to missing reset GPIO\n");
+		}
+#endif
+		return ret;
+	}
+
+	if ((reg >> 16) != 0x9303) {
+		dev_err(chip->dev, "Invalid chip found: %X\n", reg >> 16);
+		return ret;
+	}
+
+	/* The default state of the LAN9303 device is to forward packets between
+	 * all ports (if not configured differently by an external EEPROM).
+	 * The initial state of a DSA device must be forwarding packets only
+	 * between the external and the internal ports and no forwarding
+	 * between the external ports. In preparation we stop packet handling
+	 * at all for now until the LAN9303 device is re-programmed accordingly.
+	 */
+	ret = lan9303_disable_switching(chip);
+	if (ret)
+		dev_warn(chip->dev, "failed to disable switching %d\n", ret);
+
+	dev_info(chip->dev, "Found LAN9303 rev. %u\n", reg & 0xffff);
+
+	ret = lan9303_detect_phy_ids(chip);
+	if (ret) {
+		dev_err(chip->dev, "failed to discover phy bootstrap setup: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = lan9303_read(chip->regmap, LAN9303_HW_CFG, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to detect hardware configuration %d\n",
+			ret);
+		return ret;
+	}
+
+	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT1)
+		dev_info(chip->dev, "Port 1 auto-dmx enabled\n");
+	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT2)
+		dev_info(chip->dev, "Port 2 auto-dmx enabled\n");
+
+	ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+	if (ret < 0)
+		dev_err(chip->dev, "failed to read virtual phy configuration %d\n",
+			ret);
+	else
+		lan9303_report_virt_phy_config(chip, reg);
+
+	ret = lan9303_read_switch_reg(chip, LAN9303_SW_DEV_ID, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read switch device ID %d\n", ret);
+		return ret;
+	}
+	dev_info(chip->dev, "Switch device: %x, version %u, revision %u found\n",
+		 (reg >> 16) & 0xff, (reg >> 8) & 0xff, reg & 0xff);
+
+	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_0, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read switch MAC 0 ID %d\n", ret);
+		return ret;
+	}
+	dev_info(chip->dev, "MAC 0 device: %x, version %u, revision %u found\n",
+		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
+
+	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_1, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read switch MAC 1 ID %d\n", ret);
+		return ret;
+	}
+	dev_info(chip->dev, "MAC 1 device: %x, version %u, revision %u found\n",
+		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
+
+	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_2, &reg);
+	if (ret) {
+		dev_err(chip->dev, "failed to read switch MAC 2 ID %d\n", ret);
+		return ret;
+	}
+	dev_info(chip->dev, "MAC 2 device: %x, version %u, revision %u found\n",
+		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
+
+	return 0;
+}
+
+/* ---------------------------- DSA -----------------------------------*/
+
+static enum dsa_tag_protocol lan9303_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_LAN9303;
+}
+
+static int lan9303_setup(struct dsa_switch *ds)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	int ret;
+
+	/* Make sure that port 0 is the cpu port */
+	if (!dsa_is_cpu_port(ds, 0)) {
+		dev_err(chip->dev, "port 0 is not the CPU port\n");
+		return -EINVAL;
+	}
+
+	ret = lan9303_separate_ports(chip);
+	if (ret)
+		dev_err(chip->dev, "failed to separate ports %d\n", ret);
+
+	ret = lan9303_enable_switching(chip);
+	if (ret)
+		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
+
+	return 0;
+}
+
+struct lan9303_mib_desc {
+	unsigned int offset; /* offset of first MAC */
+	const char *name;
+};
+
+static const struct lan9303_mib_desc lan9303_mib[] = {
+	{ .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", },
+	{ .offset = LAN9303_MAC_RX_PAUSE_CNT_0, .name = "RxPause", },
+	{ .offset = LAN9303_MAC_RX_MULCST_CNT_0, .name = "RxMulti", },
+	{ .offset = LAN9303_MAC_RX_PKTOK_CNT_0, .name = "RxOk", },
+	{ .offset = LAN9303_MAC_RX_CRCERR_CNT_0, .name = "RxCrcErr", },
+	{ .offset = LAN9303_MAC_RX_ALIGN_CNT_0, .name = "RxAlignErr", },
+	{ .offset = LAN9303_MAC_RX_JABB_CNT_0, .name = "RxJabber", },
+	{ .offset = LAN9303_MAC_RX_FRAG_CNT_0, .name = "RxFragment", },
+	{ .offset = LAN9303_MAC_RX_64_CNT_0, .name = "Rx64Byte", },
+	{ .offset = LAN9303_MAC_RX_127_CNT_0, .name = "Rx128Byte", },
+	{ .offset = LAN9303_MAC_RX_255_CNT_0, .name = "Rx256Byte", },
+	{ .offset = LAN9303_MAC_RX_511_CNT_0, .name = "Rx512Byte", },
+	{ .offset = LAN9303_MAC_RX_1023_CNT_0, .name = "Rx1024Byte", },
+	{ .offset = LAN9303_MAC_RX_MAX_CNT_0, .name = "RxMaxByte", },
+	{ .offset = LAN9303_MAC_RX_PKTLEN_CNT_0, .name = "RxByteCnt", },
+	{ .offset = LAN9303_MAC_RX_SYMBL_CNT_0, .name = "RxSymbolCnt", },
+	{ .offset = LAN9303_MAC_RX_CTLFRM_CNT_0, .name = "RxCfs", },
+	{ .offset = LAN9303_MAC_RX_OVRSZE_CNT_0, .name = "RxOverFlow", },
+	{ .offset = LAN9303_MAC_TX_UNDSZE_CNT_0, .name = "TxShort", },
+	{ .offset = LAN9303_MAC_TX_BRDCST_CNT_0, .name = "TxBroad", },
+	{ .offset = LAN9303_MAC_TX_PAUSE_CNT_0, .name = "TxPause", },
+	{ .offset = LAN9303_MAC_TX_MULCST_CNT_0, .name = "TxMulti", },
+	{ .offset = LAN9303_MAC_RX_UNDSZE_CNT_0, .name = "TxUnderRun", },
+	{ .offset = LAN9303_MAC_TX_64_CNT_0, .name = "Tx64Byte", },
+	{ .offset = LAN9303_MAC_TX_127_CNT_0, .name = "Tx128Byte", },
+	{ .offset = LAN9303_MAC_TX_255_CNT_0, .name = "Tx256Byte", },
+	{ .offset = LAN9303_MAC_TX_511_CNT_0, .name = "Tx512Byte", },
+	{ .offset = LAN9303_MAC_TX_1023_CNT_0, .name = "Tx1024Byte", },
+	{ .offset = LAN9303_MAC_TX_MAX_CNT_0, .name = "TxMaxByte", },
+	{ .offset = LAN9303_MAC_TX_PKTLEN_CNT_0, .name = "TxByteCnt", },
+	{ .offset = LAN9303_MAC_TX_PKTOK_CNT_0, .name = "TxOk", },
+	{ .offset = LAN9303_MAC_TX_TOTALCOL_CNT_0, .name = "TxCollision", },
+	{ .offset = LAN9303_MAC_TX_MULTICOL_CNT_0, .name = "TxMultiCol", },
+	{ .offset = LAN9303_MAC_TX_SNGLECOL_CNT_0, .name = "TxSingleCol", },
+	{ .offset = LAN9303_MAC_TX_EXCOL_CNT_0, .name = "TxExcCol", },
+	{ .offset = LAN9303_MAC_TX_DEFER_CNT_0, .name = "TxDefer", },
+	{ .offset = LAN9303_MAC_TX_LATECOL_0, .name = "TxLateCol", },
+};
+
+static void lan9303_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
+{
+	unsigned int u;
+
+	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
+		strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
+			ETH_GSTRING_LEN);
+	}
+}
+
+static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	int phy_base = chip->phy_addr_sel_strap;
+
+	if (phy == phy_base)
+		return lan9303_virt_phy_reg_read(chip, regnum);
+	if (phy > phy_base + 2)
+		return -ENODEV;
+
+	return lan9303_port_phy_reg_read(chip, phy, regnum);
+}
+
+static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
+			     u16 val)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	int phy_base = chip->phy_addr_sel_strap;
+
+	if (phy == phy_base)
+		return lan9303_virt_phy_reg_write(chip, regnum, val);
+	if (phy > phy_base + 2)
+		return -ENODEV;
+
+	return lan9303_phy_reg_write(chip, phy, regnum, val);
+}
+
+static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
+				      uint64_t *data)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	u32 reg;
+	unsigned int u, poff;
+	int ret;
+
+	dev_dbg(chip->dev, "%s called\n", __func__);
+
+	poff = port * 0x400;
+
+	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
+		ret = lan9303_read_switch_reg(chip,
+					      lan9303_mib[u].offset + poff,
+					      &reg);
+		if (ret != 0)
+			dev_warn(chip->dev, "Reading status reg %u failed\n",
+				 lan9303_mib[u].offset + poff);
+		data[u] = reg;
+	}
+}
+
+/* ethtool function used to query the number of statistics items */
+static int lan9303_get_sset_count(struct dsa_switch *ds)
+{
+	return ARRAY_SIZE(lan9303_mib);
+}
+
+/* power on the given port */
+static int lan9303_port_enable(struct dsa_switch *ds, int port,
+			       struct phy_device *phy)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	int rc;
+
+	/* enable internal data handling */
+	switch (port) {
+	case 1:
+		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1, 0x03);
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
+					       0x57);
+		break;
+	case 2:
+		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2, 0x03);
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2,
+					       0x57);
+		break;
+	default:
+		dev_dbg(chip->dev, "Error: request to power up invalid port %d\n",
+			port);
+		return -ENODEV;
+	}
+
+	return rc;
+}
+
+static void lan9303_port_disable(struct dsa_switch *ds, int port,
+				 struct phy_device *phy)
+{
+	struct lan9303 *chip = ds_to_lan9303(ds);
+	int rc;
+
+	/* disable internal data handling */
+	switch (port) {
+	case 1:
+		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
+					   0, BIT(14) | BIT(11));
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1,
+					       0x02);
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
+					       0x56);
+		break;
+	case 2:
+		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
+					   0, BIT(14) | BIT(11));
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2,
+					       0x02);
+		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2,
+					       0x56);
+		break;
+	default:
+		dev_dbg(chip->dev, "Error: request to power down invalid port %d\n",
+			port);
+	}
+}
+
+static struct dsa_switch_ops lan9303_switch_ops = {
+	.get_tag_protocol = lan9303_get_tag_protocol,
+	.setup = lan9303_setup,
+	.get_strings = lan9303_get_strings,
+	.phy_read = lan9303_phy_read,
+	.phy_write = lan9303_phy_write,
+	.get_ethtool_stats = lan9303_get_ethtool_stats,
+	.get_sset_count = lan9303_get_sset_count,
+	.port_enable = lan9303_port_enable,
+	.port_disable = lan9303_port_disable,
+};
+
+static int lan9303_register_phys(struct lan9303 *chip)
+{
+	chip->ds.priv = chip;
+	chip->ds.dev = chip->dev;
+	chip->ds.ops = &lan9303_switch_ops;
+	chip->ds.phys_mii_mask = chip->phy_addr_sel_strap ? 0xe : 0x7;
+
+	return dsa_register_switch(&chip->ds, chip->dev);
+}
+
+static void lan9303_probe_reset_gpio(struct lan9303 *chip,
+				     struct device_node *np)
+{
+	chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "phy-reset",
+						   GPIOD_OUT_LOW);
+
+	if (!chip->reset_gpio) {
+		dev_dbg(chip->dev, "No reset GPIO defined\n");
+		return;
+	}
+
+	chip->reset_duration = 200;
+
+	if (np) {
+		of_property_read_u32(np, "phy-reset-duration",
+				     &chip->reset_duration);
+	} else {
+		dev_dbg(chip->dev, "reset duration defaults to 200 ms\n");
+	}
+
+	/* A sane reset duration should not be longer than 1s */
+	if (chip->reset_duration > 1000)
+		chip->reset_duration = 1000;
+}
+
+int lan9303_probe(struct lan9303 *chip, struct device_node *np)
+{
+	int ret;
+
+	mutex_init(&chip->indirect_mutex);
+
+	lan9303_probe_reset_gpio(chip, np);
+
+	ret = lan9303_handle_reset(chip);
+	if (ret != 0)
+		return ret;
+
+	ret = lan9303_check_device(chip);
+	if (ret != 0)
+		return ret;
+
+	ret = lan9303_register_phys(chip);
+	if (ret != 0)
+		return ret;
+
+	return 0;
+}
+
+int lan9303_remove(struct lan9303 *chip)
+{
+	int rc;
+
+	rc = lan9303_disable_switching(chip);
+	if (rc != 0)
+		dev_warn(chip->dev, "shutting down failed\n");
+
+	dsa_unregister_switch(&chip->ds);
+
+	/* assert reset to the whole device to prevent it from doing anything */
+	gpiod_set_value_cansleep(chip->reset_gpio, 1);
+	gpiod_unexport(chip->reset_gpio);
+
+	return 0;
+}
+
+MODULE_AUTHOR("Juergen Borleis <kernel@pengutronix.de>");
+MODULE_DESCRIPTION("Driver for SMSC/Microchip LAN9303 three port ethernet switch");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/phy/lan9303.h b/drivers/net/phy/lan9303.h
new file mode 100644
index 0000000000000..cfb1c5d5fef51
--- /dev/null
+++ b/drivers/net/phy/lan9303.h
@@ -0,0 +1,21 @@
+#include <linux/regmap.h>
+#include <linux/device.h>
+#include <net/dsa.h>
+
+struct lan9303 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *irq_data;
+	struct gpio_desc *reset_gpio;
+	u32 reset_duration; /* in [ms] */
+	bool phy_addr_sel_strap;
+	struct dsa_switch ds;
+	struct mutex indirect_mutex; /* protect indexed register access */
+};
+
+extern const struct regmap_access_table lan9303_register_set;
+
+#define ds_to_lan9303(dsasw) container_of(dsasw, struct lan9303, ds)
+
+int lan9303_probe(struct lan9303 *chip, struct device_node *np);
+int lan9303_remove(struct lan9303 *chip);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support
  2017-04-05  9:20 net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver Juergen Borleis
  2017-04-05  9:20 ` [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format Juergen Borleis
  2017-04-05  9:20 ` [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303 Juergen Borleis
@ 2017-04-05  9:20 ` Juergen Borleis
  2017-04-05 18:21   ` Andrew Lunn
  2017-04-05  9:20 ` [PATCH 4/4] net: dsa: LAN9303: add MDIO " Juergen Borleis
  3 siblings, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-05  9:20 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem

In this mode the switch device and the internal phys will be managed via
I2C interface. The MDIO interface is still supported, but for the
(emulated) CPU port only.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 .../devicetree/bindings/net/dsa/lan9303.txt        |  74 ++++++++++++++
 drivers/net/phy/Kconfig                            |  17 ++++
 drivers/net/phy/Makefile                           |   5 +
 drivers/net/phy/lan9303_i2c.c                      | 109 +++++++++++++++++++++
 4 files changed, 205 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt
 create mode 100644 drivers/net/phy/lan9303_i2c.c

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
new file mode 100644
index 0000000000000..2c8a466065a27
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -0,0 +1,74 @@
+SMSC/MicroChip LAN9303 three port ethernet switch
+-------------------------------------------------
+
+Required properties:
+
+- compatible: should be "smsc,lan9303"
+- #size-cells: must be 0
+- #address-cells: must be 1
+
+Optional properties:
+
+- phy-reset-gpios: GPIO to be used to reset the whole device, always low active
+- phy-reset-duration: reset duration, defaults to 200 ms
+
+Subnodes:
+
+The integrated switch subnode should be specified according to the binding
+described in dsa/dsa.txt. The CPU port of this switch is always port 0.
+
+Example:
+
+I2C managed mode:
+
+	master: masterdevice@X {
+		phy-handle = <&ethphy>;
+		status = "okay";
+
+		mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			ethphy: ethernet-phy@0 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <0>;
+				max-speed = <100>;
+			};
+		};
+
+	};
+
+	switch: switch@a {
+		compatible = "smsc,lan9303";
+		reg = <0xa>;
+		status = "okay";
+		interrupts-extended = <&gpio2 7 IRQ_TYPE_LEVEL_LOW>;
+		phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
+		phy-reset-duration = <200>;
+
+		dsa,member = <0 0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 { /* RMII fixed link to master */
+				reg = <0>;
+				label = "cpu";
+				ethernet = <&master>;
+				max-speed = <100>;
+			};
+
+			port@1 { /* external port 1 */
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <1>;
+				label = "lan1;
+			};
+
+			port@2 { /* external port 2 */
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <2>;
+				label = "lan2";
+			};
+		};
+	};
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 8dbd59baa34d5..acbc73adbf8c3 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -345,3 +345,20 @@ endif # PHYLIB
 config MICREL_KS8995MA
 	tristate "Micrel KS8995MA 5-ports 10/100 managed Ethernet switch"
 	depends on SPI
+
+config SMSC_LAN9303
+	tristate "SMSC LAN9303 3-ports 10/100 ethernet switch"
+	depends on NET_DSA
+	select NET_DSA_TAG_LAN9303
+	help
+	  This module provides a driver for SMSC LAN9303 3 port ethernet
+	  switch.
+
+config SMSC_LAN9303_I2C
+	bool "I2C managed mode"
+	depends on SMSC_LAN9303
+	depends on I2C && OF
+	select REGMAP_I2C
+	help
+	  Provide access functions if the SMSC LAN9303 is configured for I2C
+	  managed mode.
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 407b0b601ea82..4313adec2e8b6 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -7,6 +7,10 @@ libphy-$(CONFIG_LED_TRIGGER_PHY)	+= phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)		+= libphy.o
 
+lan9303-objs-y := lan9303-core.o
+lan9303-objs-$(CONFIG_SMSC_LAN9303_I2C) += lan9303_i2c.o
+lan9303-objs := $(lan9303-objs-y)
+
 obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
 obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
@@ -52,6 +56,7 @@ obj-$(CONFIG_NATIONAL_PHY)	+= national.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_SMSC_PHY)		+= smsc.o
+obj-$(CONFIG_SMSC_LAN9303)	+= lan9303.o
 obj-$(CONFIG_STE10XP)		+= ste10Xp.o
 obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
 obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
diff --git a/drivers/net/phy/lan9303_i2c.c b/drivers/net/phy/lan9303_i2c.c
new file mode 100644
index 0000000000000..d8d0e592ccd8d
--- /dev/null
+++ b/drivers/net/phy/lan9303_i2c.c
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+
+#include "lan9303.h"
+
+struct lan9303_i2c {
+	struct i2c_client *device;
+	struct lan9303 chip;
+};
+
+static const struct regmap_config lan9303_i2c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 1,
+	.can_multi_write = true,
+	.max_register = 0x0ff, /* address bits 0..1 are not used */
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+
+	.volatile_table = &lan9303_register_set,
+	.wr_table = &lan9303_register_set,
+	.rd_table = &lan9303_register_set,
+
+	.cache_type = REGCACHE_NONE,
+};
+
+static int lan9303_i2c_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct lan9303_i2c *sw_dev;
+	int ret;
+
+	sw_dev = devm_kzalloc(&client->dev, sizeof(struct lan9303_i2c),
+			      GFP_KERNEL);
+	if (!sw_dev)
+		return -ENOMEM;
+
+	sw_dev->chip.regmap = devm_regmap_init_i2c(client,
+						&lan9303_i2c_regmap_config);
+	if (IS_ERR(sw_dev->chip.regmap)) {
+		ret = PTR_ERR(sw_dev->chip.regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* link forward and backward */
+	sw_dev->device = client;
+	i2c_set_clientdata(client, sw_dev);
+	sw_dev->chip.dev = &client->dev;
+
+	ret = lan9303_probe(&sw_dev->chip, client->dev.of_node);
+	if (ret != 0)
+		return ret;
+
+	dev_info(&client->dev, "LAN9303 I2C driver loaded successfully\n");
+
+	return 0;
+}
+
+static int lan9303_i2c_remove(struct i2c_client *client)
+{
+	struct lan9303_i2c *sw_dev;
+
+	sw_dev = i2c_get_clientdata(client);
+	if (!sw_dev)
+		return -ENODEV;
+
+	return lan9303_remove(&sw_dev->chip);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct i2c_device_id lan9303_i2c_id[] = {
+	{ "lan9303", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, lan9303_i2c_id);
+
+static const struct of_device_id lan9303_i2c_of_match[] = {
+	{ .compatible = "smsc,lan9303", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, lan9303_i2c_of_match);
+
+static struct i2c_driver lan9303_i2c_driver = {
+	.driver = {
+		.name = "LAN9303_I2C",
+		.of_match_table = of_match_ptr(lan9303_i2c_of_match),
+	},
+	.probe = lan9303_i2c_probe,
+	.remove = lan9303_i2c_remove,
+	.id_table = lan9303_i2c_id,
+};
+module_i2c_driver(lan9303_i2c_driver);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/4] net: dsa: LAN9303: add MDIO managed mode support
  2017-04-05  9:20 net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver Juergen Borleis
                   ` (2 preceding siblings ...)
  2017-04-05  9:20 ` [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support Juergen Borleis
@ 2017-04-05  9:20 ` Juergen Borleis
  2017-04-05 19:32   ` Andrew Lunn
  3 siblings, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-05  9:20 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, f.fainelli, kernel, andrew, vivien.didelot, davem

When the LAN9303 device is in MDIO manged mode, all register accesse must
be done via MDIO.

Please note: this code is *untested* yet due to the absence of such
configured hardware. It is based on a patch of Stefan Roese from 2014.

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---
 .../devicetree/bindings/net/dsa/lan9303.txt        |   4 +
 drivers/net/phy/Kconfig                            |   8 ++
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/lan9303_mdio.c                     | 144 +++++++++++++++++++++
 4 files changed, 157 insertions(+)
 create mode 100644 drivers/net/phy/lan9303_mdio.c

diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
index 2c8a466065a27..fa2672400cd58 100644
--- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt
+++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
@@ -72,3 +72,7 @@ I2C managed mode:
 			};
 		};
 	};
+
+MDIO managed mode:
+
+	*TODO*
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index acbc73adbf8c3..dfff1794d4f7d 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -362,3 +362,11 @@ config SMSC_LAN9303_I2C
 	help
 	  Provide access functions if the SMSC LAN9303 is configured for I2C
 	  managed mode.
+
+config SMSC_LAN9303_MDIO
+	bool "MDIO managed mode"
+	depends on SMSC_LAN9303
+	depends on OF_MDIO
+	help
+	  Provide access functions if the SMSC LAN9303 is configured for MDIO
+	  managed mode.
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 4313adec2e8b6..30a528e084ea5 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PHYLIB)		+= libphy.o
 
 lan9303-objs-y := lan9303-core.o
 lan9303-objs-$(CONFIG_SMSC_LAN9303_I2C) += lan9303_i2c.o
+lan9303-objs-$(CONFIG_SMSC_LAN9303_MDIO) += lan9303_mdio.o
 lan9303-objs := $(lan9303-objs-y)
 
 obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
diff --git a/drivers/net/phy/lan9303_mdio.c b/drivers/net/phy/lan9303_mdio.c
new file mode 100644
index 0000000000000..8230b4e215f42
--- /dev/null
+++ b/drivers/net/phy/lan9303_mdio.c
@@ -0,0 +1,144 @@
+/*
+ * Copyright (C) 2017 Pengutronix, Juergen Borleis <kernel@pengutronix.de>
+ *
+ * Partially based on a patch from
+ * Copyright (c) 2014 Stefan Roese <sr@denx.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#include "lan9303.h"
+
+/* Generate phy-addr and -reg from the input address */
+#define PHY_ADDR(x) ((((x) >> 6) + 0x10) & 0x1f)
+#define PHY_REG(x) (((x) >> 1) & 0x1f)
+
+struct lan9303_mdio {
+	struct mdio_device *device;
+	struct lan9303 chip;
+};
+
+static void lan9303_mdio_real_write(struct mdio_device *mdio, int reg, u16 val)
+{
+	mdio->bus->write(mdio->bus, PHY_ADDR(reg), PHY_REG(reg), val);
+}
+
+static int lan9303_mdio_write(void *ctx, uint32_t reg, uint32_t val)
+{
+	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+
+	mutex_lock(&sw_dev->device->bus->mdio_lock);
+	lan9303_mdio_real_write(sw_dev->device, reg, val & 0xffff);
+	lan9303_mdio_real_write(sw_dev->device, reg + 2, (val >> 16) & 0xffff);
+	mutex_unlock(&sw_dev->device->bus->mdio_lock);
+
+	return 0;
+}
+
+static u16 lan9303_mdio_real_read(struct mdio_device *mdio, int reg)
+{
+	return mdio->bus->read(mdio->bus, PHY_ADDR(reg), PHY_REG(reg));
+}
+
+static int lan9303_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
+{
+	struct lan9303_mdio *sw_dev = (struct lan9303_mdio *)ctx;
+
+	mutex_lock(&sw_dev->device->bus->mdio_lock);
+	*val = lan9303_mdio_real_read(sw_dev->device, reg);
+	*val |= (lan9303_mdio_real_read(sw_dev->device, reg + 2) << 16);
+	mutex_unlock(&sw_dev->device->bus->mdio_lock);
+
+	return 0;
+}
+
+static const struct regmap_config lan9303_mdio_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 1,
+	.can_multi_write = true,
+	.max_register = 0x0ff, /* address bits 0..1 are not used */
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+
+	.volatile_table = &lan9303_register_set,
+	.wr_table = &lan9303_register_set,
+	.rd_table = &lan9303_register_set,
+
+	.reg_read = lan9303_mdio_read,
+	.reg_write = lan9303_mdio_write,
+
+	.cache_type = REGCACHE_NONE,
+};
+
+static int lan9303_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct lan9303_mdio *sw_dev;
+	int ret;
+
+	sw_dev = devm_kzalloc(&mdiodev->dev, sizeof(struct lan9303_mdio),
+			      GFP_KERNEL);
+	if (!sw_dev)
+		return -ENOMEM;
+
+	sw_dev->chip.regmap = devm_regmap_init(&mdiodev->dev, NULL, sw_dev,
+						&lan9303_mdio_regmap_config);
+	if (IS_ERR(sw_dev->chip.regmap)) {
+		ret = PTR_ERR(sw_dev->chip.regmap);
+		dev_err(&mdiodev->dev, "regmap init failed: %d\n", ret);
+		return ret;
+	}
+
+	/* link forward and backward */
+	sw_dev->device = mdiodev;
+	dev_set_drvdata(&mdiodev->dev, sw_dev);
+	sw_dev->chip.dev = &mdiodev->dev;
+
+	ret = lan9303_probe(&sw_dev->chip, mdiodev->dev.of_node);
+	if (ret != 0)
+		return ret;
+
+	dev_info(&mdiodev->dev, "LAN9303 MDIO driver loaded successfully\n");
+
+	return 0;
+}
+
+static void lan9303_mdio_remove(struct mdio_device *mdiodev)
+{
+	struct lan9303_mdio *sw_dev = dev_get_drvdata(&mdiodev->dev);
+
+	if (!sw_dev)
+		return;
+
+	lan9303_remove(&sw_dev->chip);
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct of_device_id lan9303_mdio_of_match[] = {
+	{ .compatible = "smsc,lan9303" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, lan9303_mdio_of_match);
+
+static struct mdio_driver lan9303_mdio_driver = {
+	.mdiodrv.driver = {
+		.name = "LAN9303_MDIO",
+		.of_match_table = of_match_ptr(lan9303_mdio_of_match),
+	},
+	.probe  = lan9303_mdio_probe,
+	.remove = lan9303_mdio_remove,
+};
+mdio_module_driver(lan9303_mdio_driver);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format
  2017-04-05  9:20 ` [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format Juergen Borleis
@ 2017-04-05 17:10   ` Andrew Lunn
  2017-04-06 13:36     ` Juergen Borleis
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2017-04-05 17:10 UTC (permalink / raw)
  To: Juergen Borleis
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

On Wed, Apr 05, 2017 at 11:20:21AM +0200, Juergen Borleis wrote:
> To define the outgoing port and to discover the incoming port a regular
> VLAN tag is used by the LAN9303. But its VID meaning is 'special'.
> 
> This tag handler/filter depends on some hardware features which must be
> enabled in the device to provide and make use of this special VLAN tag
> to control the destination and the source of an ethernet packet.
> 
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> ---
>  include/net/dsa.h     |   1 +
>  net/dsa/Kconfig       |   3 +
>  net/dsa/Makefile      |   1 +
>  net/dsa/dsa.c         |   3 +
>  net/dsa/dsa_priv.h    |   3 +
>  net/dsa/tag_lan9303.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 166 insertions(+)
>  create mode 100644 net/dsa/tag_lan9303.c
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 4e13e695f0251..4fb1f2b086b05 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -31,6 +31,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_EDSA,
>  	DSA_TAG_PROTO_BRCM,
>  	DSA_TAG_PROTO_QCA,
> +	DSA_TAG_PROTO_LAN9303,
>  	DSA_TAG_LAST,		/* MUST BE LAST */
>  };
>  
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 9649238eef404..22c8bd69ff71c 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -31,4 +31,7 @@ config NET_DSA_TAG_TRAILER
>  config NET_DSA_TAG_QCA
>  	bool
>  
> +config NET_DSA_TAG_LAN9303
> +	bool
> +
>  endif
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 31d343796251d..aafc74f2cb193 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -8,3 +8,4 @@ dsa_core-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
>  dsa_core-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
>  dsa_core-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
>  dsa_core-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o
> +dsa_core-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index b6d4f6a23f06c..f93f78de23af3 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -53,6 +53,9 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = {
>  #ifdef CONFIG_NET_DSA_TAG_QCA
>  	[DSA_TAG_PROTO_QCA] = &qca_netdev_ops,
>  #endif
> +#ifdef CONFIG_NET_DSA_TAG_LAN9303
> +	[DSA_TAG_PROTO_LAN9303] = &lan9303_netdev_ops,
> +#endif
>  	[DSA_TAG_PROTO_NONE] = &none_ops,
>  };
>  
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 0706a511244e9..a54cfc8aefa83 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -85,4 +85,7 @@ extern const struct dsa_device_ops brcm_netdev_ops;
>  /* tag_qca.c */
>  extern const struct dsa_device_ops qca_netdev_ops;
>  
> +/* tag_lan9303.c */
> +extern const struct dsa_device_ops lan9303_netdev_ops;
> +
>  #endif
> diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
> new file mode 100644
> index 0000000000000..ad04c6d447f77
> --- /dev/null
> +++ b/net/dsa/tag_lan9303.c
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright (C) 2017 Pengutronix, Juergen Borleis <jbe@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +#include "dsa_priv.h"
> +
> +/* To define the outgoing port and to discover the incoming port a regular
> + * VLAN tag is used by the LAN9303. But its VID meaning is 'special':
> + *
> + *       Dest MAC       Src MAC        TAG    Type
> + * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 | 1 2 |...
> + *                                |<------->|
> + * TAG:
> + *    |<------------->|
> + *    |  1  2 | 3  4  |
> + *      TPID    VID
> + *     0x8100
> + *
> + * VID bit 3 indicates a request for an ALR lookup.
> + *
> + * If VID bit 3 is zero, then bits 0 and 1 specify the destination port
> + * (0, 1, 2) or broadcast (3) or the source port (1, 2).
> + *
> + * VID bit 4 is used to specify if the STP port state should be overridden.
> + * Required when no forwarding between the external ports should happen.
> + */

Hi Juergen

Nice comment, thanks.

> +
> +#define LAN9303_TAG_LEN 4
> +
> +static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
> +{

...

> +	/* make room between MACs and Ether-Type */
> +	memmove(skb->data, skb->data + LAN9303_TAG_LEN, 2 * ETH_ALEN);
> +
> +	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
> +	lan9303_tag[0] = htons(ETH_P_8021Q);
> +	lan9303_tag[1] = htons(p->dp->index | BIT(4));

> +static int lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
> +		       struct packet_type *pt, struct net_device *orig_dev)
> +{
> +	u16 *lan9303_tag;
> +	struct dsa_switch_tree *dst = dev->dsa_ptr;
> +	struct dsa_switch *ds = dst->ds[0];
> +	unsigned int source_port;
> +
> +	if (unlikely(!dst)) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet, due to missing switch tree device\n");
> +		goto out_drop;
> +	}

By the time you get here, you have already dereferenced dst, in order
to get ds. So this is pointless.

> +	lan9303_tag = (u16 *)(skb->data - 2);
> +
> +	if (lan9303_tag[0] != htons(0x8100)) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid VLAN marker\n");
> +		goto out_drop;
> +	}

In the transmit function, you use ETH_P_8021Q, please do so here as
well.

> +	source_port = ntohs(lan9303_tag[1]) & 0x3;
> +
> +	if (source_port >= DSA_MAX_PORTS) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid source port\n");
> +		goto out_drop;
> +	}

You can be more specific here. For this hardware, > 3 is invalid and
should be dropped. If we later add other chips which have more ports,
we can relax this check then.

> +	/* remove the special VLAN tag between the MAC addresses
> +	 * and the current ethertype field.
> +	 */
> +	skb_pull_rcsum(skb, 2 + 2);
> +	memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
> +		2 * ETH_ALEN);
> +	skb_push(skb, ETH_HLEN);

Do you need to do anything with the checksum here? Other tagging
protocols do.

	  Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-05  9:20 ` [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303 Juergen Borleis
@ 2017-04-05 18:12   ` Andrew Lunn
  2017-04-06 10:18     ` Juergen Borleis
  2017-04-06 10:39     ` Juergen Borleis
  0 siblings, 2 replies; 18+ messages in thread
From: Andrew Lunn @ 2017-04-05 18:12 UTC (permalink / raw)
  To: Juergen Borleis
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

On Wed, Apr 05, 2017 at 11:20:22AM +0200, Juergen Borleis wrote:
> The SMSC/Microchip LAN9303 is an ethernet switch device with one CPU port
> and two external ethernet ports with built-in phys.
> 
> This driver uses the DSA framework, but is currently only capable of
> separating the two external ports. There is no offload support yet.
> 
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> ---
>  drivers/net/phy/lan9303-core.c | 924 +++++++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/lan9303.h      |  21 +

drivers/net/dsa please.

One general comment. I'm assuming parts of this code comes from
openwrt swconfig. In swconfig, i think they consider switches to be
part of the PHY layer. In DSA, switches are switches, PHYs are
PHYs. So the naming in this driver needs to reflect this.

> +static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
> +{
> +	int ret;
> +
> +	/* we can lose arbitration for the I2C case, because the device
> +	 * tries to detect and read an external EEPROM after reset and acts as
> +	 * a master on the shared I2C bus itself. This conflicts with our
> +	 * attempts to access the device as a slave at the same moment.
> +	 */
> +	do {
> +		ret = regmap_read(regmap, offset, reg);
> +		if (ret == -EAGAIN)
> +			msleep(500);
> +	} while (ret == -EAGAIN);

Please limit the number of retires, and return -EIO if you don't get
access.

> +/* virtual phy registers must be read mapped */
> +static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
> +{
> +	int ret;
> +	u32 val;
> +
> +	if (regnum > 7)
> +		return -EINVAL;
> +
> +	ret = lan9303_read(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, &val);
> +	if (ret != 0)
> +		return ret;

Here, and everywhere else, please just use (ret)

> +
> +	return val & 0xffff;
> +}
> +
> +static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
> +{
> +	if (regnum > 7)
> +		return -EINVAL;
> +
> +	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);

Does this virtual PHY use the usual 10/100/1000 PHY registers?


> +}
> +
> +static int lan9303_port_phy_reg_wait_for_completion(struct lan9303 *chip)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	/* transfer completed? */
> +	do {
> +		ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
> +		if (ret != 0)
> +			return ret;
> +	} while (reg & LAN9303_PMI_ACCESS_MII_BUSY);

Again, no endless looping please. Abort after a while.

> +
> +	return 0;
> +}
> +
> +static int lan9303_port_phy_reg_read(struct lan9303 *chip, int addr, int regnum)
> +{
> +	int ret;
> +	u32 val;
> +
> +	val = ((unsigned int)addr) << 11; /* setup phy ID */
> +	val |= ((unsigned int)regnum) << 6;
> +
> +	mutex_lock(&chip->indirect_mutex);
> +
> +	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	/* start the MII read cycle */
> +	ret = regmap_write(chip->regmap, LAN9303_PMI_ACCESS, val);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	/* read the result of this operation */
> +	ret = lan9303_read(chip->regmap, LAN9303_PMI_DATA, &val);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	mutex_unlock(&chip->indirect_mutex);
> +
> +	return val & 0xffff;
> +
> +on_error:
> +	mutex_unlock(&chip->indirect_mutex);
> +	return ret;
> +}
> +
> +static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
> +				 unsigned int val)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	reg = ((unsigned int)addr) << 11; /* setup phy ID */

Within Linux, PHY ID means the contents of PHY registers 2 and 3. It
would be good not to confuse things by using a different meaning.

> +	reg |= ((unsigned int)regnum) << 6;
> +	reg |= LAN9303_PMI_ACCESS_MII_WRITE;
> +
> +	mutex_lock(&chip->indirect_mutex);
> +
> +	ret = lan9303_port_phy_reg_wait_for_completion(chip);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	/* write the data first... */
> +	ret = regmap_write(chip->regmap, LAN9303_PMI_DATA, val);
> +	if (ret != 0)
> +		goto on_error;
> +
> +	/* ...then start the MII write cycle */
> +	ret = regmap_write(chip->regmap, LAN9303_PMI_ACCESS, reg);
> +
> +on_error:
> +	mutex_unlock(&chip->indirect_mutex);
> +	return ret;
> +}
> +
> +static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	/* transfer completed? */
> +	do {
> +		ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
> +		if (ret) {
> +			dev_err(chip->dev,
> +				"Failed to read csr command status: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	} while (reg & LAN9303_SWITCH_CSR_CMD_BUSY);

More endless looping...

> +
> +	return 0;
> +}
> +
> +static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	reg = regnum;
> +	reg |= LAN9303_SWITCH_CSR_CMD_LANES;
> +	reg |= LAN9303_SWITCH_CSR_CMD_BUSY;
> +
> +	mutex_lock(&chip->indirect_mutex);
> +
> +	ret = lan9303_switch_wait_for_completion(chip);
> +	if (ret)
> +		goto on_error;
> +
> +	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_DATA, val);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to write csr data reg: %d\n", ret);
> +		goto on_error;
> +	}
> +
> +	/* trigger write */
> +	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_CMD, reg);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to write csr command reg: %d\n",
> +			ret);
> +
> +on_error:
> +	mutex_unlock(&chip->indirect_mutex);
> +	return ret;
> +}
> +
> +static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	reg = regnum;
> +	reg |= LAN9303_SWITCH_CSR_CMD_LANES;
> +	reg |= LAN9303_SWITCH_CSR_CMD_RW;
> +	reg |= LAN9303_SWITCH_CSR_CMD_BUSY;
> +
> +	mutex_lock(&chip->indirect_mutex);
> +
> +	ret = lan9303_switch_wait_for_completion(chip);
> +	if (ret)
> +		goto on_error;
> +
> +	/* trigger read */
> +	ret = regmap_write(chip->regmap, LAN9303_SWITCH_CSR_CMD, reg);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to write csr command reg: %d\n",
> +			ret);
> +		goto on_error;
> +	}
> +
> +	ret = lan9303_switch_wait_for_completion(chip);
> +	if (ret)
> +		goto on_error;
> +
> +	ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_DATA, val);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to read csr data reg: %d\n",
> +			ret);
> +on_error:
> +	mutex_unlock(&chip->indirect_mutex);
> +	return ret;
> +}
> +
> +static int lan9303_detect_phy_ids(struct lan9303 *chip)

This is another example of phy_ids having a different meaning to
normal. lan9303_detect_phy_addrs()?

> +{
> +	int reg;
> +
> +	/* depending on the 'phy_addr_sel_strap' setting, the three phys are
> +	 * using IDs 0-1-2 or IDs 1-2-3. We cannot read back the
> +	 * 'phy_addr_sel_strap' setting directly, so we need a test, which
> +	 * configuration is active:
> +	 * Register 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
> +	 * and the IDs are 0-1-2, else it contains something different from
> +	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
> +	 */
> +	reg = lan9303_port_phy_reg_read(chip, 3, 18);

#defines for 3 and 18?

> +	if (reg < 0) {
> +		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
> +		return reg;
> +	}
> +
> +	if (reg != 0)
> +		chip->phy_addr_sel_strap = 1;
> +	else
> +		chip->phy_addr_sel_strap = 0;
> +
> +	dev_dbg(chip->dev, "Phy configuration '%s' detected\n",
> +		chip->phy_addr_sel_strap ? "1-2-3" : "0-1-2");
> +
> +	return 0;
> +}
> +
> +static void lan9303_report_virt_phy_config(struct lan9303 *chip,
> +					   unsigned int reg)
> +{
> +	switch ((reg >> 8) & 0x03) {
> +	case 0:
> +		dev_info(chip->dev, "Virtual phy in 'MII MAC mode'\n");
> +		break;
> +	case 1:
> +		dev_info(chip->dev, "Virtual phy in 'MII PHY mode'\n");
> +		break;
> +	case 2:
> +		dev_info(chip->dev, "Virtual phy in 'RMII PHY mode'\n");
> +		break;
> +	case 3:
> +		dev_err(chip->dev, "Invalid virtual phy mode\n");
> +		break;
> +	}
> +
> +	if (reg & BIT(6))
> +		dev_info(chip->dev, "RMII clock is an output\n");
> +	else
> +		dev_info(chip->dev, "RMII clock is an input\n");
> +}
> +
> +/* stop processing packets at all ports */
> +static int lan9303_disable_switching(struct lan9303 *chip)

switching normally means allowing packets to go from port to port,
based on address learning. Is that what is being disabled here? Or are
you just disabling ports so no frames at all pass through?

> +{
> +	int ret;
> +
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_0, 0x02);

#defines for these magic numbers.

> +	if (ret)
> +		return ret;
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1, 0x02);
> +	if (ret)
> +		return ret;
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2, 0x02);
> +	if (ret)
> +		return ret;
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_0, 0x56);
> +	if (ret)
> +		return ret;
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1, 0x56);
> +	if (ret)
> +		return ret;
> +	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2, 0x56);
> +}
> +
> +/* start processing packets at CPU port */
> +static int lan9303_enable_switching(struct lan9303 *chip)
> +{
> +	int ret;
> +
> +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_0, 0x03);
> +	if (ret)
> +		return ret;
> +
> +	return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_0, 0x57);
> +}
> +
> +/* We want a special working switch:
> + * - do not route between port 1 and 2

route generally refers to layer 3, IP routing. Forward is the more
common term for layer 2, but it can also be used at L3, so is still a
bit ambiguous.

> + * - route everything from port 1 to port 0
> + * - route everything from port 2 to port 0
> + * - route special tagged packets from port 0 to port 1 *or* port 2
> + */
> +static int lan9303_separate_ports(struct lan9303 *chip)
> +{
> +	int ret;
> +
> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR,
> +				LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT0 |
> +				LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT1 |
> +				LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT2 |
> +				LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING |
> +				LAN9303_SWE_PORT_MIRROR_SNIFF_ALL);
> +	if (ret)
> +		return ret;
> +
> +	/* enable defining the destination port via special VLAN tagging
> +	 * for port 0
> +	 */
> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> +				       0x03);
> +	if (ret)
> +		return ret;
> +
> +	/* tag incoming packets at port 1 and 2 on their way to port 0 to be
> +	 * able to discover their source port
> +	 */
> +	ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
> +			LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
> +	if (ret)
> +		return ret;
> +
> +	/* prevent port 1 and 2 from forwarding packets by their own */
> +	return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
> +				LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
> +				LAN9303_SWE_PORT_STATE_BLOCKING_PORT1 |
> +				LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
> +}
> +
> +static int lan9303_handle_reset(struct lan9303 *chip)
> +{
> +	if (!chip->reset_gpio)
> +		return 0;
> +
> +	gpiod_export_link(chip->dev, "reset", chip->reset_gpio);

Why do this? Are you expecting userspace to reset the switch?

> +
> +	if (chip->reset_duration != 0)
> +		msleep(chip->reset_duration);
> +
> +	/* release (deassert) reset and activate the device */
> +	gpiod_set_value_cansleep(chip->reset_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int lan9303_check_device(struct lan9303 *chip)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> +			ret);
> +#ifdef DEBUG
> +		if (!chip->reset_gpio) {
> +			dev_warn(chip->dev,
> +				 "Maybe failed due to missing reset GPIO\n");
> +		}
> +#endif

No #ifdef please. Either this should be mandatory, and you should of
failed in the probe function, or it is optional, and then there is no
need to warn.

> +		return ret;
> +	}
> +
> +	if ((reg >> 16) != 0x9303) {

#define for the ID?

> +		dev_err(chip->dev, "Invalid chip found: %X\n", reg >> 16);
> +		return ret;
> +	}
> +
> +	/* The default state of the LAN9303 device is to forward packets between
> +	 * all ports (if not configured differently by an external EEPROM).
> +	 * The initial state of a DSA device must be forwarding packets only
> +	 * between the external and the internal ports and no forwarding
> +	 * between the external ports. In preparation we stop packet handling
> +	 * at all for now until the LAN9303 device is re-programmed accordingly.
> +	 */
> +	ret = lan9303_disable_switching(chip);
> +	if (ret)
> +		dev_warn(chip->dev, "failed to disable switching %d\n", ret);
> +
> +	dev_info(chip->dev, "Found LAN9303 rev. %u\n", reg & 0xffff);
> +
> +	ret = lan9303_detect_phy_ids(chip);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to discover phy bootstrap setup: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = lan9303_read(chip->regmap, LAN9303_HW_CFG, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to detect hardware configuration %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT1)
> +		dev_info(chip->dev, "Port 1 auto-dmx enabled\n");
> +	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT2)
> +		dev_info(chip->dev, "Port 2 auto-dmx enabled\n");

What is dmx?

> +
> +	ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
> +	if (ret < 0)
> +		dev_err(chip->dev, "failed to read virtual phy configuration %d\n",
> +			ret);
> +	else
> +		lan9303_report_virt_phy_config(chip, reg);
> +
> +	ret = lan9303_read_switch_reg(chip, LAN9303_SW_DEV_ID, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read switch device ID %d\n", ret);
> +		return ret;
> +	}
> +	dev_info(chip->dev, "Switch device: %x, version %u, revision %u found\n",
> +		 (reg >> 16) & 0xff, (reg >> 8) & 0xff, reg & 0xff);
> +
> +	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_0, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read switch MAC 0 ID %d\n", ret);
> +		return ret;
> +	}
> +	dev_info(chip->dev, "MAC 0 device: %x, version %u, revision %u found\n",
> +		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
> +
> +	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_1, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read switch MAC 1 ID %d\n", ret);
> +		return ret;
> +	}
> +	dev_info(chip->dev, "MAC 1 device: %x, version %u, revision %u found\n",
> +		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
> +
> +	ret = lan9303_read_switch_reg(chip, LAN9303_MAC_VER_ID_2, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read switch MAC 2 ID %d\n", ret);
> +		return ret;
> +	}
> +	dev_info(chip->dev, "MAC 2 device: %x, version %u, revision %u found\n",
> +		 (reg >> 8) & 0xf, (reg >> 4) & 0xf, reg & 0xf);
> +

We are spamming the log with lots of information here. Do we need it
all?

> +	return 0;
> +}
> +
> +/* ---------------------------- DSA -----------------------------------*/
> +
> +static enum dsa_tag_protocol lan9303_get_tag_protocol(struct dsa_switch *ds)
> +{
> +	return DSA_TAG_PROTO_LAN9303;
> +}
> +
> +static int lan9303_setup(struct dsa_switch *ds)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	int ret;
> +
> +	/* Make sure that port 0 is the cpu port */
> +	if (!dsa_is_cpu_port(ds, 0)) {
> +		dev_err(chip->dev, "port 0 is not the CPU port\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = lan9303_separate_ports(chip);
> +	if (ret)
> +		dev_err(chip->dev, "failed to separate ports %d\n", ret);
> +
> +	ret = lan9303_enable_switching(chip);
> +	if (ret)
> +		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
> +
> +	return 0;
> +}
> +
> +struct lan9303_mib_desc {
> +	unsigned int offset; /* offset of first MAC */
> +	const char *name;
> +};
> +
> +static const struct lan9303_mib_desc lan9303_mib[] = {
> +	{ .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", },
> +	{ .offset = LAN9303_MAC_RX_PAUSE_CNT_0, .name = "RxPause", },
> +	{ .offset = LAN9303_MAC_RX_MULCST_CNT_0, .name = "RxMulti", },
> +	{ .offset = LAN9303_MAC_RX_PKTOK_CNT_0, .name = "RxOk", },
> +	{ .offset = LAN9303_MAC_RX_CRCERR_CNT_0, .name = "RxCrcErr", },
> +	{ .offset = LAN9303_MAC_RX_ALIGN_CNT_0, .name = "RxAlignErr", },
> +	{ .offset = LAN9303_MAC_RX_JABB_CNT_0, .name = "RxJabber", },
> +	{ .offset = LAN9303_MAC_RX_FRAG_CNT_0, .name = "RxFragment", },
> +	{ .offset = LAN9303_MAC_RX_64_CNT_0, .name = "Rx64Byte", },
> +	{ .offset = LAN9303_MAC_RX_127_CNT_0, .name = "Rx128Byte", },
> +	{ .offset = LAN9303_MAC_RX_255_CNT_0, .name = "Rx256Byte", },
> +	{ .offset = LAN9303_MAC_RX_511_CNT_0, .name = "Rx512Byte", },
> +	{ .offset = LAN9303_MAC_RX_1023_CNT_0, .name = "Rx1024Byte", },
> +	{ .offset = LAN9303_MAC_RX_MAX_CNT_0, .name = "RxMaxByte", },
> +	{ .offset = LAN9303_MAC_RX_PKTLEN_CNT_0, .name = "RxByteCnt", },
> +	{ .offset = LAN9303_MAC_RX_SYMBL_CNT_0, .name = "RxSymbolCnt", },
> +	{ .offset = LAN9303_MAC_RX_CTLFRM_CNT_0, .name = "RxCfs", },
> +	{ .offset = LAN9303_MAC_RX_OVRSZE_CNT_0, .name = "RxOverFlow", },
> +	{ .offset = LAN9303_MAC_TX_UNDSZE_CNT_0, .name = "TxShort", },
> +	{ .offset = LAN9303_MAC_TX_BRDCST_CNT_0, .name = "TxBroad", },
> +	{ .offset = LAN9303_MAC_TX_PAUSE_CNT_0, .name = "TxPause", },
> +	{ .offset = LAN9303_MAC_TX_MULCST_CNT_0, .name = "TxMulti", },
> +	{ .offset = LAN9303_MAC_RX_UNDSZE_CNT_0, .name = "TxUnderRun", },
> +	{ .offset = LAN9303_MAC_TX_64_CNT_0, .name = "Tx64Byte", },
> +	{ .offset = LAN9303_MAC_TX_127_CNT_0, .name = "Tx128Byte", },
> +	{ .offset = LAN9303_MAC_TX_255_CNT_0, .name = "Tx256Byte", },
> +	{ .offset = LAN9303_MAC_TX_511_CNT_0, .name = "Tx512Byte", },
> +	{ .offset = LAN9303_MAC_TX_1023_CNT_0, .name = "Tx1024Byte", },
> +	{ .offset = LAN9303_MAC_TX_MAX_CNT_0, .name = "TxMaxByte", },
> +	{ .offset = LAN9303_MAC_TX_PKTLEN_CNT_0, .name = "TxByteCnt", },
> +	{ .offset = LAN9303_MAC_TX_PKTOK_CNT_0, .name = "TxOk", },
> +	{ .offset = LAN9303_MAC_TX_TOTALCOL_CNT_0, .name = "TxCollision", },
> +	{ .offset = LAN9303_MAC_TX_MULTICOL_CNT_0, .name = "TxMultiCol", },
> +	{ .offset = LAN9303_MAC_TX_SNGLECOL_CNT_0, .name = "TxSingleCol", },
> +	{ .offset = LAN9303_MAC_TX_EXCOL_CNT_0, .name = "TxExcCol", },
> +	{ .offset = LAN9303_MAC_TX_DEFER_CNT_0, .name = "TxDefer", },
> +	{ .offset = LAN9303_MAC_TX_LATECOL_0, .name = "TxLateCol", },
> +};
> +
> +static void lan9303_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
> +{
> +	unsigned int u;
> +
> +	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> +		strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
> +			ETH_GSTRING_LEN);
> +	}
> +}
> +
> +static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	int phy_base = chip->phy_addr_sel_strap;
> +
> +	if (phy == phy_base)
> +		return lan9303_virt_phy_reg_read(chip, regnum);
> +	if (phy > phy_base + 2)
> +		return -ENODEV;
> +
> +	return lan9303_port_phy_reg_read(chip, phy, regnum);
> +}

PHY functions in the middle of stats functions? Maybe move them later?

> +
> +static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> +			     u16 val)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	int phy_base = chip->phy_addr_sel_strap;
> +
> +	if (phy == phy_base)
> +		return lan9303_virt_phy_reg_write(chip, regnum, val);
> +	if (phy > phy_base + 2)
> +		return -ENODEV;
> +
> +	return lan9303_phy_reg_write(chip, phy, regnum, val);

Does the MDIO bus go to the outside world? Could there be external
PHYs?

> +}
> +
> +static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> +				      uint64_t *data)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	u32 reg;
> +	unsigned int u, poff;
> +	int ret;
> +
> +	dev_dbg(chip->dev, "%s called\n", __func__);

I think this can be removed.

> +
> +	poff = port * 0x400;
> +
> +	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> +		ret = lan9303_read_switch_reg(chip,
> +					      lan9303_mib[u].offset + poff,
> +					      &reg);
> +		if (ret != 0)
> +			dev_warn(chip->dev, "Reading status reg %u failed\n",
> +				 lan9303_mib[u].offset + poff);
> +		data[u] = reg;
> +	}
> +}
> +
> +/* ethtool function used to query the number of statistics items */
> +static int lan9303_get_sset_count(struct dsa_switch *ds)
> +{
> +	return ARRAY_SIZE(lan9303_mib);
> +}
> +
> +/* power on the given port */
> +static int lan9303_port_enable(struct dsa_switch *ds, int port,
> +			       struct phy_device *phy)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	int rc;

Please be consistent and use ret.

> +
> +	/* enable internal data handling */
> +	switch (port) {
> +	case 1:
> +		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1, 0x03);
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> +					       0x57);
> +		break;
> +	case 2:
> +		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2, 0x03);
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2,
> +					       0x57);
> +		break;
> +	default:
> +		dev_dbg(chip->dev, "Error: request to power up invalid port %d\n",
> +			port);
> +		return -ENODEV;
> +	}
> +
> +	return rc;
> +}
> +
> +static void lan9303_port_disable(struct dsa_switch *ds, int port,
> +				 struct phy_device *phy)
> +{
> +	struct lan9303 *chip = ds_to_lan9303(ds);
> +	int rc;
> +
> +	/* disable internal data handling */
> +	switch (port) {
> +	case 1:
> +		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> +					   0, BIT(14) | BIT(11));
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1,
> +					       0x02);
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> +					       0x56);

It seems odd that port_enable does not touch the PHY, but port_disable
does. What is this doing?

> +		break;
> +	case 2:
> +		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 2,
> +					   0, BIT(14) | BIT(11));
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2,
> +					       0x02);
> +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2,
> +					       0x56);
> +		break;
> +	default:
> +		dev_dbg(chip->dev, "Error: request to power down invalid port %d\n",
> +			port);
> +	}
> +}
> +
> +static struct dsa_switch_ops lan9303_switch_ops = {
> +	.get_tag_protocol = lan9303_get_tag_protocol,
> +	.setup = lan9303_setup,
> +	.get_strings = lan9303_get_strings,
> +	.phy_read = lan9303_phy_read,
> +	.phy_write = lan9303_phy_write,
> +	.get_ethtool_stats = lan9303_get_ethtool_stats,
> +	.get_sset_count = lan9303_get_sset_count,
> +	.port_enable = lan9303_port_enable,
> +	.port_disable = lan9303_port_disable,
> +};
> +
> +static int lan9303_register_phys(struct lan9303 *chip)

This one place where the switch is being called a phy.

> +{
> +	chip->ds.priv = chip;
> +	chip->ds.dev = chip->dev;
> +	chip->ds.ops = &lan9303_switch_ops;
> +	chip->ds.phys_mii_mask = chip->phy_addr_sel_strap ? 0xe : 0x7;
> +
> +	return dsa_register_switch(&chip->ds, chip->dev);
> +}
> +
> +static void lan9303_probe_reset_gpio(struct lan9303 *chip,
> +				     struct device_node *np)
> +{
> +	chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "phy-reset",

This is a reset for the switch, not a PHY in the switch i think. We
have established a bit of a standard in DSA drivers to just call this
"reset".

> +						   GPIOD_OUT_LOW);
> +
> +	if (!chip->reset_gpio) {
> +		dev_dbg(chip->dev, "No reset GPIO defined\n");
> +		return;
> +	}
> +
> +	chip->reset_duration = 200;
> +
> +	if (np) {
> +		of_property_read_u32(np, "phy-reset-duration",
> +				     &chip->reset_duration);
> +	} else {
> +		dev_dbg(chip->dev, "reset duration defaults to 200 ms\n");
> +	}
> +
> +	/* A sane reset duration should not be longer than 1s */
> +	if (chip->reset_duration > 1000)
> +		chip->reset_duration = 1000;
> +}
> +
> +int lan9303_probe(struct lan9303 *chip, struct device_node *np)
> +{
> +	int ret;
> +
> +	mutex_init(&chip->indirect_mutex);
> +
> +	lan9303_probe_reset_gpio(chip, np);
> +
> +	ret = lan9303_handle_reset(chip);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = lan9303_check_device(chip);
> +	if (ret != 0)
> +		return ret;
> +
> +	ret = lan9303_register_phys(chip);
> +	if (ret != 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +int lan9303_remove(struct lan9303 *chip)
> +{
> +	int rc;
> +
> +	rc = lan9303_disable_switching(chip);
> +	if (rc != 0)
> +		dev_warn(chip->dev, "shutting down failed\n");
> +
> +	dsa_unregister_switch(&chip->ds);
> +
> +	/* assert reset to the whole device to prevent it from doing anything */
> +	gpiod_set_value_cansleep(chip->reset_gpio, 1);
> +	gpiod_unexport(chip->reset_gpio);
> +
> +	return 0;
> +}
> +
> +MODULE_AUTHOR("Juergen Borleis <kernel@pengutronix.de>");
> +MODULE_DESCRIPTION("Driver for SMSC/Microchip LAN9303 three port ethernet switch");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/phy/lan9303.h b/drivers/net/phy/lan9303.h
> new file mode 100644
> index 0000000000000..cfb1c5d5fef51
> --- /dev/null
> +++ b/drivers/net/phy/lan9303.h
> @@ -0,0 +1,21 @@
> +#include <linux/regmap.h>
> +#include <linux/device.h>
> +#include <net/dsa.h>
> +
> +struct lan9303 {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct gpio_desc *reset_gpio;
> +	u32 reset_duration; /* in [ms] */
> +	bool phy_addr_sel_strap;
> +	struct dsa_switch ds;
> +	struct mutex indirect_mutex; /* protect indexed register access */
> +};
> +
> +extern const struct regmap_access_table lan9303_register_set;
> +
> +#define ds_to_lan9303(dsasw) container_of(dsasw, struct lan9303, ds)
> +
> +int lan9303_probe(struct lan9303 *chip, struct device_node *np);
> +int lan9303_remove(struct lan9303 *chip);
> -- 
> 2.11.0
> 

  Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support
  2017-04-05  9:20 ` [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support Juergen Borleis
@ 2017-04-05 18:21   ` Andrew Lunn
  2017-04-06 13:46     ` Juergen Borleis
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2017-04-05 18:21 UTC (permalink / raw)
  To: Juergen Borleis
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

On Wed, Apr 05, 2017 at 11:20:23AM +0200, Juergen Borleis wrote:
> In this mode the switch device and the internal phys will be managed via
> I2C interface. The MDIO interface is still supported, but for the
> (emulated) CPU port only.
> 
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> ---
>  .../devicetree/bindings/net/dsa/lan9303.txt        |  74 ++++++++++++++
>  drivers/net/phy/Kconfig                            |  17 ++++
>  drivers/net/phy/Makefile                           |   5 +
>  drivers/net/phy/lan9303_i2c.c                      | 109 +++++++++++++++++++++
>  4 files changed, 205 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt
>  create mode 100644 drivers/net/phy/lan9303_i2c.c
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> new file mode 100644
> index 0000000000000..2c8a466065a27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> @@ -0,0 +1,74 @@
> +SMSC/MicroChip LAN9303 three port ethernet switch
> +-------------------------------------------------
> +
> +Required properties:
> +
> +- compatible: should be "smsc,lan9303"
> +- #size-cells: must be 0
> +- #address-cells: must be 1
> +
> +Optional properties:
> +
> +- phy-reset-gpios: GPIO to be used to reset the whole device, always low active
> +- phy-reset-duration: reset duration, defaults to 200 ms

It is good to state the unit, ms.

> +
> +Subnodes:
> +
> +The integrated switch subnode should be specified according to the binding
> +described in dsa/dsa.txt. The CPU port of this switch is always port 0.
> +
> +Example:
> +
> +I2C managed mode:
> +
> +	master: masterdevice@X {
> +		phy-handle = <&ethphy>;
> +		status = "okay";
> +
> +		mdio {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ethphy: ethernet-phy@0 {
> +				compatible = "ethernet-phy-ieee802.3-c22";
> +				reg = <0>;
> +				max-speed = <100>;
> +			};

If there is RMII between the CPU interface and the switch, why is this
PHY needed?

> +		};
> +
> +	};
> +
> +	switch: switch@a {
> +		compatible = "smsc,lan9303";
> +		reg = <0xa>;
> +		status = "okay";
> +		interrupts-extended = <&gpio2 7 IRQ_TYPE_LEVEL_LOW>;

This interrupt is not in the binding documentation, or the code.

> +		phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
> +		phy-reset-duration = <200>;
> +
> +		dsa,member = <0 0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 { /* RMII fixed link to master */
> +				reg = <0>;
> +				label = "cpu";
> +				ethernet = <&master>;
> +				max-speed = <100>;

max-speed does not do anything i think, since there is no adjust_link
function.

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] net: dsa: LAN9303: add MDIO managed mode support
  2017-04-05  9:20 ` [PATCH 4/4] net: dsa: LAN9303: add MDIO " Juergen Borleis
@ 2017-04-05 19:32   ` Andrew Lunn
  2017-04-06 13:53     ` Florian Fainelli
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2017-04-05 19:32 UTC (permalink / raw)
  To: Juergen Borleis
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

> +static const struct of_device_id lan9303_mdio_of_match[] = {
> +	{ .compatible = "smsc,lan9303" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, lan9303_mdio_of_match);

If i'm reading this right, i think you have the same compatible string
for both the i2c and the mdio driver. Does that work? I've no idea.

     Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-05 18:12   ` Andrew Lunn
@ 2017-04-06 10:18     ` Juergen Borleis
  2017-04-06 11:59       ` Andrew Lunn
  2017-04-06 10:39     ` Juergen Borleis
  1 sibling, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-06 10:18 UTC (permalink / raw)
  To: kernel
  Cc: Andrew Lunn, f.fainelli, vivien.didelot, netdev, linux-kernel, davem

Hi Andrew,

v2 of the patches will follow.

On Wednesday 05 April 2017 20:12:32 Andrew Lunn wrote:
> [...]
> > +	do {
> > +		ret = regmap_read(regmap, offset, reg);
> > +		if (ret == -EAGAIN)
> > +			msleep(500);
> > +	} while (ret == -EAGAIN);
>
> Please limit the number of retires, and return -EIO if you don't get
> access.

Done in v2.

> > +/* virtual phy registers must be read mapped */
> > +static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
> > +{
> > +	int ret;
> > +	u32 val;
> > +
> > +	if (regnum > 7)
> > +		return -EINVAL;
> > +
> > +	ret = lan9303_read(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, &val);
> > +	if (ret != 0) 
> > +		return ret;
>
> Here, and everywhere else, please just use (ret)

Done in v2.

> [...]
> > +static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
> > +{ 
> > +	if (regnum > 7)
> > +		return -EINVAL;
> > +
> > +	return regmap_write(chip->regmap, LAN9303_VIRT_PHY_BASE + regnum, val);
>
> Does this virtual PHY use the usual 10/100/1000 PHY registers?

Yes. Registers 0...6

> [...]
> > +	do {
> > +		ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
> > +		if (ret != 0)
> > +			return ret;
> > +	} while (reg & LAN9303_PMI_ACCESS_MII_BUSY);
>
> Again, no endless looping please. Abort after a while.

Done in v2.

> [...]
> > +static int lan9303_phy_reg_write(struct lan9303 *chip, int addr, int regnum,
> > +				 unsigned int val) 
> > +{
> > +	int ret;
> > +	u32 reg;
> > +
> > +	reg = ((unsigned int)addr) << 11; /* setup phy ID */
>
> Within Linux, PHY ID means the contents of PHY registers 2 and 3. It
> would be good not to confuse things by using a different meaning.

Renamed in v2.

> [...]
> > +	/* transfer completed? */
> > +	do {
> > +		ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
> > +		if (ret) {
> > +			dev_err(chip->dev,
> > +				"Failed to read csr command status: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	} while (reg & LAN9303_SWITCH_CSR_CMD_BUSY);
>
> More endless looping...

More done in v2 :)

> [...]
> > +static int lan9303_detect_phy_ids(struct lan9303 *chip)
>
> This is another example of phy_ids having a different meaning to
> normal. lan9303_detect_phy_addrs()?

Renamed in v2.

> > +{
> > +	int reg;
> > +
> > +	/* depending on the 'phy_addr_sel_strap' setting, the three phys are
> > +	 * using IDs 0-1-2 or IDs 1-2-3. We cannot read back the
> > +	 * 'phy_addr_sel_strap' setting directly, so we need a test, which
> > +	 * configuration is active:
> > +	 * Register 18 of phy 3 reads as 0x0000, if 'phy_addr_sel_strap' is 0
> > +	 * and the IDs are 0-1-2, else it contains something different from
> > +	 * 0x0000, which means 'phy_addr_sel_strap' is 1 and the IDs are 1-2-3.
> > +	 */ 
> > +	reg = lan9303_port_phy_reg_read(chip, 3, 18);
>
> #defines for 3 and 18?

Done in v2.

>
> > +	if (reg < 0) {
> > +		dev_err(chip->dev, "Failed to detect phy config: %d\n", reg);
> > +		return reg;
> > +	}
> > +
> > +	if (reg != 0)
> > +		chip->phy_addr_sel_strap = 1;
> > +	else
> > +		chip->phy_addr_sel_strap = 0;
> > +
> > +	dev_dbg(chip->dev, "Phy configuration '%s' detected\n",
> > +		chip->phy_addr_sel_strap ? "1-2-3" : "0-1-2");
> > +
> > +	return 0;
> > +}
> > +
> > +static void lan9303_report_virt_phy_config(struct lan9303 *chip,
> > +					   unsigned int reg)
> > +{
> > +	switch ((reg >> 8) & 0x03) {
> > +	case 0:
> > +		dev_info(chip->dev, "Virtual phy in 'MII MAC mode'\n");
> > +		break;
> > +	case 1:
> > +		dev_info(chip->dev, "Virtual phy in 'MII PHY mode'\n");
> > +		break;
> > +	case 2:
> > +		dev_info(chip->dev, "Virtual phy in 'RMII PHY mode'\n");
> > +		break;
> > +	case 3:
> > +		dev_err(chip->dev, "Invalid virtual phy mode\n");
> > +		break;
> > +	}
> > +
> > +	if (reg & BIT(6))
> > +		dev_info(chip->dev, "RMII clock is an output\n");
> > +	else
> > +		dev_info(chip->dev, "RMII clock is an input\n");
> > +}
> > +
> > +/* stop processing packets at all ports */
> > +static int lan9303_disable_switching(struct lan9303 *chip)
>
> switching normally means allowing packets to go from port to port,
> based on address learning. Is that what is being disabled here? Or are
> you just disabling ports so no frames at all pass through?

Yes. The device defaults to learning mode and starts to switch packages
immediately.

> > +{
> > +	int ret;
> > +
> > +	ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_0, 0x02);
>
> #defines for these magic numbers.

I refactored this routines in v2.

> [...]
> > +
> > +/* We want a special working switch:
> > + * - do not route between port 1 and 2
>
> route generally refers to layer 3, IP routing. Forward is the more
> common term for layer 2, but it can also be used at L3, so is still a
> bit ambiguous.

I'm not a network expert. In v2 I use "forwarding" instead.

> [...]
> > +static int lan9303_handle_reset(struct lan9303 *chip)
> > +{
> > +	if (!chip->reset_gpio)
> > +		return 0;
> > +
> > +	gpiod_export_link(chip->dev, "reset", chip->reset_gpio);
>
> Why do this? Are you expecting userspace to reset the switch?

Leftover from development. Removed in v2.

> [...]
> > +#ifdef DEBUG
> > +		if (!chip->reset_gpio) {
> > +			dev_warn(chip->dev,
> > +				 "Maybe failed due to missing reset GPIO\n");
> > +		}
> > +#endif
>
> No #ifdef please. Either this should be mandatory, and you should of
> failed in the probe function, or it is optional, and then there is no
> need to warn.

Done in v2.

> [...]
> > +	if ((reg >> 16) != 0x9303) {
>
> #define for the ID?

Done in v2.

> [...]
> > +	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT1)
> > +		dev_info(chip->dev, "Port 1 auto-dmx enabled\n");
> > +	if (reg & LAN9303_HW_CFG_AMDX_EN_PORT2)
> > +		dev_info(chip->dev, "Port 2 auto-dmx enabled\n");
>
> What is dmx?

Typo...

> [...]
> We are spamming the log with lots of information here. Do we need it
> all?

Leftover from development, removed in v2.

> [...]
> > +static int lan9303_phy_read(struct dsa_switch *ds, int phy, int regnum)
> > +{ 
> > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > +	int phy_base = chip->phy_addr_sel_strap;
> > +
> > +	if (phy == phy_base)
> > +		return lan9303_virt_phy_reg_read(chip, regnum);
> > +	if (phy > phy_base + 2)
> > +		return -ENODEV;
> > +
> > +	return lan9303_port_phy_reg_read(chip, phy, regnum);
> > +}
>
> PHY functions in the middle of stats functions? Maybe move them later?

Done in v2.

> > +
> > +static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> > +			          u16 val) 
> > +{
> > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > +	int phy_base = chip->phy_addr_sel_strap;
> > +
> > +	if (phy == phy_base)
> > +		return lan9303_virt_phy_reg_write(chip, regnum, val);
> > +	if (phy > phy_base + 2)
> > +		return -ENODEV;
> > +
> > +	return lan9303_phy_reg_write(chip, phy, regnum, val);
>
> Does the MDIO bus go to the outside world? Could there be external
> PHYs?

???? This device includes two phys (at port 1 and 2) and these functions are
called to detect their state.

> [...]
> > +	dev_dbg(chip->dev, "%s called\n", __func__);
>
> I think this can be removed.

Done in v2.

> [...]
> > +/* power on the given port */
> > +static int lan9303_port_enable(struct dsa_switch *ds, int port,
> > +			       struct phy_device *phy)
> > +{
> > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > +	int rc;
>
> Please be consistent and use ret.

Changed by refactoring the whole function in v2.

> > +	/* enable internal data handling */
> > +	switch (port) {
> > +	case 1:
> > +		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1, 0x03);
> > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> > +					       0x57);
> > +		break;
> > +	case 2:
> > +		rc = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_2, 0x03);
> > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_2,
> > +					       0x57);
> > +		break;
> > +	default:
> > +		dev_dbg(chip->dev, "Error: request to power up invalid port %d\n",
> > +			port);
> > +		return -ENODEV;
> > +	}
> > +
> > +	return rc;
> > +}
> > +
> > +static void lan9303_port_disable(struct dsa_switch *ds, int port,
> > +				 struct phy_device *phy)
> > +{
> > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > +	int rc;
> > +
> > +	/* disable internal data handling */
> > +	switch (port) {
> > +	case 1:
> > +		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> > +					   0, BIT(14) | BIT(11));
> > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1,
> > +					       0x02);
> > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> > +					       0x56);
>
> It seems odd that port_enable does not touch the PHY, but port_disable
> does. What is this doing?

My experience is, the framework powers up the phys by its own in conjunction
with calling lan9303_port_enable(), but do not power down them in conjunction
with calling lan9303_port_disable(). In v2 I do not touch the phy anymore.

> [...]
> > +static int lan9303_register_phys(struct lan9303 *chip)
>
> This one place where the switch is being called a phy.

Renamed in v2.

> [...]
> > +static void lan9303_probe_reset_gpio(struct lan9303 *chip,
> > +				     struct device_node *np)
> > +{
> > +	chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "phy-reset",
>
> This is a reset for the switch, not a PHY in the switch i think. We
> have established a bit of a standard in DSA drivers to just call this
> "reset".

Renamed in v2.

Thanks
Juergen

-- 
Pengutronix e.K.                             | Juergen Borleis             |
Linux Solutions for Science and Industry     | Phone: +49-5121-206917-5128 |
Peiner Str. 6-8, 31137 Hildesheim, Germany   | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686             | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-05 18:12   ` Andrew Lunn
  2017-04-06 10:18     ` Juergen Borleis
@ 2017-04-06 10:39     ` Juergen Borleis
  1 sibling, 0 replies; 18+ messages in thread
From: Juergen Borleis @ 2017-04-06 10:39 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

Hi Andrew,

On Wednesday 05 April 2017 20:12:32 Andrew Lunn wrote:
> [...]
> >  drivers/net/phy/lan9303-core.c | 924 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/phy/lan9303.h      |  21 +
>
> drivers/net/dsa please.

Done in v2.

> One general comment. I'm assuming parts of this code comes from
> openwrt swconfig. In swconfig, i think they consider switches to be
> part of the PHY layer. In DSA, switches are switches, PHYs are
> PHYs. So the naming in this driver needs to reflect this.

And the LAN9303 is a switch with built-in PHYs :)

Juergen

-- 
Pengutronix e.K.                             | Juergen Borleis             |
Industrial Linux Solutions                   | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-06 10:18     ` Juergen Borleis
@ 2017-04-06 11:59       ` Andrew Lunn
  2017-04-06 13:42         ` Juergen Borleis
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2017-04-06 11:59 UTC (permalink / raw)
  To: Juergen Borleis
  Cc: kernel, f.fainelli, vivien.didelot, netdev, linux-kernel, davem

> > > +static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
> > > +			          u16 val) 
> > > +{
> > > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > > +	int phy_base = chip->phy_addr_sel_strap;
> > > +
> > > +	if (phy == phy_base)
> > > +		return lan9303_virt_phy_reg_write(chip, regnum, val);
> > > +	if (phy > phy_base + 2)
> > > +		return -ENODEV;
> > > +
> > > +	return lan9303_phy_reg_write(chip, phy, regnum, val);
> >
> > Does the MDIO bus go to the outside world? Could there be external
> > PHYs?
> 
> ???? This device includes two phys (at port 1 and 2) and these functions are
> called to detect their state.

Some switches have the MDIO bus available on pins. It is then possible
to connect additional PHYs on the MDIO bus. If their is an external
MDIO bus, you should remove the test for phy > phy_base + 2, and allow
the full range of 32. 

> > > +static void lan9303_port_disable(struct dsa_switch *ds, int port,
> > > +				 struct phy_device *phy)
> > > +{
> > > +	struct lan9303 *chip = ds_to_lan9303(ds);
> > > +	int rc;
> > > +
> > > +	/* disable internal data handling */
> > > +	switch (port) {
> > > +	case 1:
> > > +		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> > > +					   0, BIT(14) | BIT(11));
> > > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1,
> > > +					       0x02);
> > > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> > > +					       0x56);
> >
> > It seems odd that port_enable does not touch the PHY, but port_disable
> > does. What is this doing?
> 
> My experience is, the framework powers up the phys by its own in conjunction
> with calling lan9303_port_enable(), but do not power down them in conjunction
> with calling lan9303_port_disable(). In v2 I do not touch the phy anymore.

So this is touching the BMCR_PDOWN bit? Using the #define would of
helped explain this.

       Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format
  2017-04-05 17:10   ` Andrew Lunn
@ 2017-04-06 13:36     ` Juergen Borleis
  0 siblings, 0 replies; 18+ messages in thread
From: Juergen Borleis @ 2017-04-06 13:36 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

Hi Andrew,

On Wednesday 05 April 2017 19:10:01 Andrew Lunn wrote:
> [...]
> > +static int lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
> > +		       struct packet_type *pt, struct net_device *orig_dev)
> > +{
> > +	u16 *lan9303_tag;
> > +	struct dsa_switch_tree *dst = dev->dsa_ptr;
> > +	struct dsa_switch *ds = dst->ds[0];
> > +	unsigned int source_port;
> > +
> > +	if (unlikely(!dst)) {
> > +		dev_warn_ratelimited(&dev->dev, "Dropping packet, due to missing switch tree device\n");
> > +		goto out_drop; 
> > +	}
>
> By the time you get here, you have already dereferenced dst, in order
> to get ds. So this is pointless.

Ups! Thanks! Fixed in v2.

> > +	lan9303_tag = (u16 *)(skb->data - 2);
> > +
> > +	if (lan9303_tag[0] != htons(0x8100)) {
> > +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid VLAN marker\n");
> > +		goto out_drop; 
> > +	}
>
> In the transmit function, you use ETH_P_8021Q, please do so here as
> well.

Done in v2.

> > +	source_port = ntohs(lan9303_tag[1]) & 0x3;
> > +
> > +	if (source_port >= DSA_MAX_PORTS) {
> > +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid source port\n");
> > +		goto out_drop; 
> > +	}
>
> You can be more specific here. For this hardware, > 3 is invalid and
> should be dropped. If we later add other chips which have more ports,
> we can relax this check then.

Done in v2.

> > +	/* remove the special VLAN tag between the MAC addresses
> > +	 * and the current ethertype field.
> > +	 */
> > +	skb_pull_rcsum(skb, 2 + 2);
> > +	memmove(skb->data - ETH_HLEN, skb->data - (ETH_HLEN + LAN9303_TAG_LEN),
> > +		2 * ETH_ALEN); 
> > +	skb_push(skb, ETH_HLEN);
>
> Do you need to do anything with the checksum here? Other tagging
> protocols do.

Don't know exactly what you are meaning here. For me it does the same like
the qca_tag_rcv() function in tag_qca.c does.

Juergen

-- 
Pengutronix e.K.                             | Juergen Borleis             |
Industrial Linux Solutions                   | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
  2017-04-06 11:59       ` Andrew Lunn
@ 2017-04-06 13:42         ` Juergen Borleis
  0 siblings, 0 replies; 18+ messages in thread
From: Juergen Borleis @ 2017-04-06 13:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: kernel, f.fainelli, vivien.didelot, netdev, linux-kernel, davem

Hi Andrew,

On Thursday 06 April 2017 13:59:00 Andrew Lunn wrote:
> [...]
> > > Does the MDIO bus go to the outside world? Could there be external
> > > PHYs?
> >
> > ???? This device includes two phys (at port 1 and 2) and these
> > functions are called to detect their state.
>
> Some switches have the MDIO bus available on pins. It is then possible
> to connect additional PHYs on the MDIO bus. If their is an external
> MDIO bus, you should remove the test for phy > phy_base + 2, and allow
> the full range of 32.

Hmm, not sure. You can run this device without a master port and use an 
additional external PHY instead. In this case there is an external MDIO 
available to this external PHY. But I don't know if I can reach this MDIO 
in the same way like the internal MDIO to the built-in PHYs.

> [...]
> > > > +		rc = lan9303_phy_reg_write(chip, chip->phy_addr_sel_strap + 1,
> > > > +					   0, BIT(14) | BIT(11));
> > > > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_1,
> > > > +					       0x02);
> > > > +		rc += lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_1,
> > > > +					       0x56);
> > >
> > > It seems odd that port_enable does not touch the PHY, but
> > > port_disable does. What is this doing?
> >
> > My experience is, the framework powers up the phys by its own in
> > conjunction with calling lan9303_port_enable(), but do not power down
> > them in conjunction with calling lan9303_port_disable(). In v2 I do not
> > touch the phy anymore.
>
> So this is touching the BMCR_PDOWN bit? Using the #define would of
> helped explain this.

Okay.

Juergen

-- 
Pengutronix e.K.                             | Juergen Borleis             |
Industrial Linux Solutions                   | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support
  2017-04-05 18:21   ` Andrew Lunn
@ 2017-04-06 13:46     ` Juergen Borleis
  2017-04-06 13:52       ` Florian Fainelli
  0 siblings, 1 reply; 18+ messages in thread
From: Juergen Borleis @ 2017-04-06 13:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, linux-kernel, f.fainelli, kernel, vivien.didelot, davem

Hi Andrew,

On Wednesday 05 April 2017 20:21:55 Andrew Lunn wrote:
> [...]
> > +SMSC/MicroChip LAN9303 three port ethernet switch
> > +-------------------------------------------------
> > +
> > +Required properties:
> > +
> > +- compatible: should be "smsc,lan9303"
> > +- #size-cells: must be 0
> > +- #address-cells: must be 1
> > +
> > +Optional properties:
> > +
> > +- phy-reset-gpios: GPIO to be used to reset the whole device, always low active
> > +- phy-reset-duration: reset duration, defaults to 200 ms 
>
> It is good to state the unit, ms.

Done in v2.

> [...]
> > +	master: masterdevice@X {
> > +		phy-handle = <&ethphy>;
> > +		status = "okay";
> > +
> > +		mdio {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			ethphy: ethernet-phy@0 {
> > +				compatible = "ethernet-phy-ieee802.3-c22";
> > +				reg = <0>;
> > +				max-speed = <100>;
> > +			};
>
> If there is RMII between the CPU interface and the switch, why is this
> PHY needed?

Tested and changed to "fixed-link" in v2.

> [...]
> > +	switch: switch@a {
> > +		compatible = "smsc,lan9303";
> > +		reg = <0xa>;
> > +		status = "okay";
> > +		interrupts-extended = <&gpio2 7 IRQ_TYPE_LEVEL_LOW>;
>
> This interrupt is not in the binding documentation, or the code.

Leftover from development. Removed in v2.

> > +		phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
> > +		phy-reset-duration = <200>;
> > +
> > +		dsa,member = <0 0>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 { /* RMII fixed link to master */
> > +				reg = <0>;
> > +				label = "cpu";
> > +				ethernet = <&master>;
> > +				max-speed = <100>;
>
> max-speed does not do anything i think, since there is no adjust_link
> function.

Removed in v2.

Thanks.

Juergen

-- 
Pengutronix e.K.                             | Juergen Borleis             |
Industrial Linux Solutions                   | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support
  2017-04-06 13:46     ` Juergen Borleis
@ 2017-04-06 13:52       ` Florian Fainelli
  0 siblings, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2017-04-06 13:52 UTC (permalink / raw)
  To: Juergen Borleis, Andrew Lunn
  Cc: netdev, linux-kernel, kernel, vivien.didelot, davem


On 04/06/2017 06:46 AM, Juergen Borleis wrote:
>>> +		phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
>>> +		phy-reset-duration = <200>;
>>> +
>>> +		dsa,member = <0 0>;
>>> +
>>> +		ports {
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +
>>> +			port@0 { /* RMII fixed link to master */
>>> +				reg = <0>;
>>> +				label = "cpu";
>>> +				ethernet = <&master>;
>>> +				max-speed = <100>;
>>
>> max-speed does not do anything i think, since there is no adjust_link
>> function.

Since port 0 is the CPU port, we expect to find a fixed PHY for it, so
yes, this won't do much because fixed-link already appropriately limits
the speed.
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] net: dsa: LAN9303: add MDIO managed mode support
  2017-04-05 19:32   ` Andrew Lunn
@ 2017-04-06 13:53     ` Florian Fainelli
  2017-04-06 14:25       ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2017-04-06 13:53 UTC (permalink / raw)
  To: Andrew Lunn, Juergen Borleis
  Cc: netdev, linux-kernel, kernel, vivien.didelot, davem



On 04/05/2017 12:32 PM, Andrew Lunn wrote:
>> +static const struct of_device_id lan9303_mdio_of_match[] = {
>> +	{ .compatible = "smsc,lan9303" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, lan9303_mdio_of_match);
> 
> If i'm reading this right, i think you have the same compatible string
> for both the i2c and the mdio driver. Does that work? I've no idea.

This should not be a problem, actually, this does seem like the right
thing to do. The bus topology would solve the Linux device creation such
that you would probe either via mdio or i2c (or both) but through the
appropriate drivers' probe functions.
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] net: dsa: LAN9303: add MDIO managed mode support
  2017-04-06 13:53     ` Florian Fainelli
@ 2017-04-06 14:25       ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2017-04-06 14:25 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Juergen Borleis, netdev, linux-kernel, kernel, vivien.didelot, davem

On Thu, Apr 06, 2017 at 06:53:10AM -0700, Florian Fainelli wrote:
> 
> 
> On 04/05/2017 12:32 PM, Andrew Lunn wrote:
> >> +static const struct of_device_id lan9303_mdio_of_match[] = {
> >> +	{ .compatible = "smsc,lan9303" },
> >> +	{ /* sentinel */ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, lan9303_mdio_of_match);
> > 
> > If i'm reading this right, i think you have the same compatible string
> > for both the i2c and the mdio driver. Does that work? I've no idea.
> 
> This should not be a problem, actually, this does seem like the right
> thing to do. The bus topology would solve the Linux device creation such
> that you would probe either via mdio or i2c (or both) but through the
> appropriate drivers' probe functions.

Hi Florian

I was more thinking about what happens when there is a call out to
userspace to find a kernel module to load to driver this hardware.  Is
the compatible string used for this? Is the subsystem string also used
when finding the correct kernel module?

	 Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2017-04-06 14:26 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05  9:20 net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver Juergen Borleis
2017-04-05  9:20 ` [PATCH 1/4] net: dsa: add support for the SMSC-LAN9303 tagging format Juergen Borleis
2017-04-05 17:10   ` Andrew Lunn
2017-04-06 13:36     ` Juergen Borleis
2017-04-05  9:20 ` [PATCH 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303 Juergen Borleis
2017-04-05 18:12   ` Andrew Lunn
2017-04-06 10:18     ` Juergen Borleis
2017-04-06 11:59       ` Andrew Lunn
2017-04-06 13:42         ` Juergen Borleis
2017-04-06 10:39     ` Juergen Borleis
2017-04-05  9:20 ` [PATCH 3/4] net: dsa: LAN9303: add I2C managed mode support Juergen Borleis
2017-04-05 18:21   ` Andrew Lunn
2017-04-06 13:46     ` Juergen Borleis
2017-04-06 13:52       ` Florian Fainelli
2017-04-05  9:20 ` [PATCH 4/4] net: dsa: LAN9303: add MDIO " Juergen Borleis
2017-04-05 19:32   ` Andrew Lunn
2017-04-06 13:53     ` Florian Fainelli
2017-04-06 14:25       ` Andrew Lunn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.