[PATCH net-next v2 0/5] net-next: dsa: add Mediatek MT7530 support

[PATCH net-next v2 0/5] net-next: dsa: add Mediatek MT7530 support

[sean.wang-NuS5LvNUpcJWk0Htik3J/w]

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
Mediatek router platforms such as MT7623A or MT7623N which includes 7-port
Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY. Among these ports,
The port from 0 to 4 are the user ports connecting with the remote devices
while the port 5 and 6 are the CPU ports connecting into Mediatek Ethernet
GMAC.

The patch series integrated Mediatek MT7530 into DSA support which
includes the most of the essential callbacks such as tag insertion for
port distinguishing, port control, bridge offloading, STP setup and
ethtool operations to allow DSA to model each user port into independently
standalone netdevice as the other DSA driver had done.

Changes since v1:
- rebased into 4.11-rc1
- refined binding document including below five items 
- changed the type of mediatek,mcm into bool
- used reset controller binding for MCM reset and removed "mediatek,ethsys"
  property from binding
- reused CPU port's ethernet Phandle instead of creating new one and removed
  "mediatek,ethernet" property from binding
- aligned naming for GPIO reset with dsa/marvell.txt
- added phy-mode as required property child nodes within ports container
- handled gpio reset with devm_gpiod_* API
- refined comment words
- removed condition for CDM setting since the setup looks both fine for all cases
- allowed of_find_net_device_by_node() working with pointing the device node into
  real netdev instance
- fixed Kbuild warnings

Sean Wang (5):
  dt-bindings: net: dsa: add Mediatek MT7530 binding
  net-next: dsa: add Mediatek tag RX/TX handler
  net-next: ethernet: mediatek: add CDM able to recognize the tag for
    DSA
  net-next: ethernet: mediatek: add device_node of GMAC pointing into
    the netdev instance
  net-next: dsa: add dsa support for Mediatek MT7530 switch

 .../devicetree/bindings/net/dsa/mt7530.txt         |   92 ++
 drivers/net/dsa/Kconfig                            |    8 +
 drivers/net/dsa/Makefile                           |    2 +-
 drivers/net/dsa/mt7530.c                           | 1172 ++++++++++++++++++++
 drivers/net/dsa/mt7530.h                           |  382 +++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.c        |    8 +
 drivers/net/ethernet/mediatek/mtk_eth_soc.h        |    4 +
 include/net/dsa.h                                  |    1 +
 net/dsa/Kconfig                                    |    2 +
 net/dsa/Makefile                                   |    1 +
 net/dsa/dsa.c                                      |    3 +
 net/dsa/dsa_priv.h                                 |    3 +
 net/dsa/tag_mtk.c                                  |  117 ++
 13 files changed, 1794 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
 create mode 100644 drivers/net/dsa/mt7530.c
 create mode 100644 drivers/net/dsa/mt7530.h
 create mode 100644 net/dsa/tag_mtk.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v2 1/5] dt-bindings: net: dsa: add Mediatek MT7530 binding

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add device-tree binding for Mediatek MT7530 switch.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/net/dsa/mt7530.txt         | 92 ++++++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
new file mode 100644
index 0000000..a9bc27b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -0,0 +1,92 @@
+Mediatek MT7530 Ethernet switch
+================================
+
+Required properties:
+
+- compatible: Must be compatible = "mediatek,mt7530";
+- #address-cells: Must be 1.
+- #size-cells: Must be 0.
+- mediatek,mcm: Boolean; if defined, indicates that either MT7530 is the part
+	on multi-chip module belong to MT7623A has or the remotely standalone
+	chip as the function MT7623N reference board provided for.
+- core-supply: Phandle to the regulator node necessary for the core power.
+- io-supply: Phandle to the regulator node necessary for the I/O power.
+	See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
+	for details for the regulator setup on these boards.
+
+If the property mediatek,mcm isn't defined, following property is required
+
+- reset-gpios: Should be a gpio specifier for a reset line.
+
+Else, following properties are required
+
+- resets : Phandle pointing to the system reset controller with
+	line index for the ethsys.
+- reset-names : Should be set to "mcm".
+
+Required properties for the child nodes within ports container:
+
+- reg: Port address described must be 6 for CPU port and from 0 to 5 for
+	user ports.
+- phy-mode: String, must be either "trgmii" or "rgmii" for port labeled
+	 "cpu".
+
+See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional
+required, optional properties and how the integrated switch subnodes must
+be specified.
+
+Example:
+
+	&mdio0 {
+		switch@0 {
+			compatible = "mediatek,mt7530";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+
+			core-supply = <&mt6323_vpa_reg>;
+			io-supply = <&mt6323_vemc3v3_reg>;
+			reset-gpios = <&pio 33 0>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+				port@0 {
+					reg = <0>;
+					label = "lan0";
+				};
+
+				port@1 {
+					reg = <1>;
+					label = "lan1";
+				};
+
+				port@2 {
+					reg = <2>;
+					label = "lan2";
+				};
+
+				port@3 {
+					reg = <3>;
+					label = "lan3";
+				};
+
+				port@4 {
+					reg = <4>;
+					label = "wan";
+				};
+
+				port@6 {
+					reg = <6>;
+					label = "cpu";
+					ethernet = <&gmac0>;
+					phy-mode = "trgmii";
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+					};
+				};
+			};
+		};
+	};
-- 
1.9.1

Re: [PATCH net-next v2 1/5] dt-bindings: net: dsa: add Mediatek MT7530 binding

[Rob Herring]

On Tue, Mar 21, 2017 at 05:35:06PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> Add device-tree binding for Mediatek MT7530 switch.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  .../devicetree/bindings/net/dsa/mt7530.txt         | 92 ++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

[PATCH net-next v2 2/5] net-next: dsa: add Mediatek tag RX/TX handler

[sean.wang-NuS5LvNUpcJWk0Htik3J/w]

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Add the support for the 4-bytes tag for DSA port distinguishing inserted
allowing receiving and transmitting the packet via the particular port.
The tag is being added after the source MAC address in the ethernet
header.

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 include/net/dsa.h  |   1 +
 net/dsa/Kconfig    |   2 +
 net/dsa/Makefile   |   1 +
 net/dsa/dsa.c      |   3 ++
 net/dsa/dsa_priv.h |   3 ++
 net/dsa/tag_mtk.c  | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 127 insertions(+)
 create mode 100644 net/dsa/tag_mtk.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4e13e69..3276547 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_MTK,
 	DSA_TAG_LAST,		/* MUST BE LAST */
 };
 
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 9649238..d78789b 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -31,4 +31,6 @@ config NET_DSA_TAG_TRAILER
 config NET_DSA_TAG_QCA
 	bool
 
+config NET_DSA_TAG_MTK
+	bool
 endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 31d3437..9b1d478 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_MTK) += tag_mtk.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index b6d4f6a..617f736 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -53,6 +53,9 @@ static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb,
 #ifdef CONFIG_NET_DSA_TAG_QCA
 	[DSA_TAG_PROTO_QCA] = &qca_netdev_ops,
 #endif
+#ifdef CONFIG_NET_DSA_TAG_MTK
+	[DSA_TAG_PROTO_MTK] = &mtk_netdev_ops,
+#endif
 	[DSA_TAG_PROTO_NONE] = &none_ops,
 };
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 0706a51..2a31399 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -85,4 +85,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 /* tag_qca.c */
 extern const struct dsa_device_ops qca_netdev_ops;
 
+/* tag_mtk.c */
+extern const struct dsa_device_ops mtk_netdev_ops;
+
 #endif
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
new file mode 100644
index 0000000..833a9d6
--- /dev/null
+++ b/net/dsa/tag_mtk.c
@@ -0,0 +1,117 @@
+/*
+ * Mediatek DSA Tag support
+ * Copyright (C) 2017 Landen Chao <landen.chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *		      Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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 "dsa_priv.h"
+
+#define MTK_HDR_LEN		4
+#define MTK_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
+#define MTK_HDR_XMIT_DP_BIT_MASK	GENMASK(5, 0)
+
+static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
+				    struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	u8 *mtk_tag;
+
+	if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
+		goto out_free;
+
+	skb_push(skb, MTK_HDR_LEN);
+
+	memmove(skb->data, skb->data + MTK_HDR_LEN, 2 * ETH_ALEN);
+
+	/* Build the tag after the MAC Source Address */
+	mtk_tag = skb->data + 2 * ETH_ALEN;
+	mtk_tag[0] = 0;
+	mtk_tag[1] = (1 << p->dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
+	mtk_tag[2] = 0;
+	mtk_tag[3] = 0;
+
+	return skb;
+
+out_free:
+	kfree_skb(skb);
+	return NULL;
+}
+
+static int mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
+		       struct packet_type *pt, struct net_device *orig_dev)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds;
+	int port;
+	__be16 *phdr, hdr;
+
+	if (unlikely(!dst))
+		goto out_drop;
+
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (!skb)
+		goto out;
+
+	if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
+		goto out_drop;
+
+	/* The MTK header is added by the switch between src addr
+	 * and ethertype at this point, skb->data points to 2 bytes
+	 * after src addr so header should be 2 bytes right before.
+	 */
+	phdr = (__be16 *)(skb->data - 2);
+	hdr = ntohs(*phdr);
+
+	/* Remove MTK tag and recalculate checksum. */
+	skb_pull_rcsum(skb, MTK_HDR_LEN);
+
+	memmove(skb->data - ETH_HLEN,
+		skb->data - ETH_HLEN - MTK_HDR_LEN,
+		2 * ETH_ALEN);
+
+	/* This protocol doesn't support cascading multiple
+	 * switches so it's safe to assume the switch is first
+	 * in the tree.
+	 */
+	ds = dst->ds[0];
+	if (!ds)
+		goto out_drop;
+
+	/* Get source port information */
+	port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);
+	if (!ds->ports[port].netdev)
+		goto out_drop;
+
+	/* Update skb & forward the frame accordingly */
+	skb_push(skb, ETH_HLEN);
+
+	skb->pkt_type = PACKET_HOST;
+	skb->dev = ds->ports[port].netdev;
+	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 mtk_netdev_ops = {
+	.xmit	= mtk_tag_xmit,
+	.rcv	= mtk_tag_rcv,
+};
-- 
1.9.1

[PATCH net-next v2 3/5] net-next: ethernet: mediatek: add CDM able to recognize the tag for DSA

[sean.wang-NuS5LvNUpcJWk0Htik3J/w]

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

The patch adds the setup for allowing CDM can recognize these packets with
carrying port-distinguishing tag. Otherwise, these tagging packets will be
handled incorrectly by CDM. The setup is working out for general untag
packets as well.

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 6 ++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 9e75768..c21ed99 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1846,6 +1846,12 @@ static int mtk_hw_init(struct mtk_eth *eth)
 	/* GE2, Force 1000M/FD, FC ON */
 	mtk_w32(eth, MAC_MCR_FIXED_LINK, MTK_MAC_MCR(1));
 
+	/* Indicates CDM to parse the MTK special tag from CPU
+	 * which also is working out for untag packets.
+	 */
+	val = mtk_r32(eth, MTK_CDMQ_IG_CTRL);
+	mtk_w32(eth, val | MTK_CDMQ_STAG_EN, MTK_CDMQ_IG_CTRL);
+
 	/* Enable RX VLan Offloading */
 	mtk_w32(eth, 1, MTK_CDMP_EG_CTRL);
 
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 99b1c8e..996024d 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -70,6 +70,10 @@
 /* Frame Engine Interrupt Grouping Register */
 #define MTK_FE_INT_GRP		0x20
 
+/* CDMP Ingress Control Register */
+#define MTK_CDMQ_IG_CTRL	0x1400
+#define MTK_CDMQ_STAG_EN	BIT(0)
+
 /* CDMP Exgress Control Register */
 #define MTK_CDMP_EG_CTRL	0x404
 
-- 
1.9.1

Re: [PATCH net-next v2 3/5] net-next: ethernet: mediatek: add CDM able to recognize the tag for DSA

[Andrew Lunn]

On Tue, Mar 21, 2017 at 05:35:08PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> The patch adds the setup for allowing CDM can recognize these packets with
> carrying port-distinguishing tag. Otherwise, these tagging packets will be
> handled incorrectly by CDM. The setup is working out for general untag
> packets as well.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

    Andrew

Re: [PATCH net-next v2 3/5] net-next: ethernet: mediatek: add CDM able to recognize the tag for DSA

[Florian Fainelli]

On 03/21/2017 02:35 AM, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> The patch adds the setup for allowing CDM can recognize these packets with
> carrying port-distinguishing tag. Otherwise, these tagging packets will be
> handled incorrectly by CDM. The setup is working out for general untag
> packets as well.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v2 4/5] net-next: ethernet: mediatek: add device_node of GMAC pointing into the netdev instance

[sean.wang-NuS5LvNUpcJWk0Htik3J/w]

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

the patch adds the setup of the corresponding device node of GMAC into the
netdev instance which could allow other modules such as DSA to find the
instance through the node in dt-bindings using of_find_net_device_by_node()
call.

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index c21ed99..84b09a4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2323,6 +2323,8 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 	eth->netdev[id]->ethtool_ops = &mtk_ethtool_ops;
 
 	eth->netdev[id]->irq = eth->irq[0];
+	eth->netdev[id]->dev.of_node = np;
+
 	return 0;
 
 free_netdev:
-- 
1.9.1

Re: [PATCH net-next v2 4/5] net-next: ethernet: mediatek: add device_node of GMAC pointing into the netdev instance

[Andrew Lunn]

On Tue, Mar 21, 2017 at 05:35:09PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> the patch adds the setup of the corresponding device node of GMAC into the
> netdev instance which could allow other modules such as DSA to find the
> instance through the node in dt-bindings using of_find_net_device_by_node()
> call.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index c21ed99..84b09a4 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -2323,6 +2323,8 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
>  	eth->netdev[id]->ethtool_ops = &mtk_ethtool_ops;
>  
>  	eth->netdev[id]->irq = eth->irq[0];
> +	eth->netdev[id]->dev.of_node = np;
> +

Humm, O.K. This is not obvious, until you look at of_dev_node_match()
in net-sysfs.c.

Most Ethernet drivers don't set netdev.dev.of_node. But they do call
SET_NETDEV_DEV(), which sets netdev.dev.parent.

of_dev_node_match() first looks at netdev.dev.parent->of_node, and if that
does not match, then looks at netdev.dev.of_node.

For the mtk Ethernet driver, the parent is not going to work, because
of the sub devices. So netdev.dev.of_node does need to be set.

Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

    Andrew


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 4/5] net-next: ethernet: mediatek: add device_node of GMAC pointing into the netdev instance

[Florian Fainelli]

On 03/21/2017 02:35 AM, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> the patch adds the setup of the corresponding device node of GMAC into the
> netdev instance which could allow other modules such as DSA to find the
> instance through the node in dt-bindings using of_find_net_device_by_node()
> call.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[sean.wang-NuS5LvNUpcJWk0Htik3J/w]

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
Mediatek router platforms such as MT7623A or MT7623N platform which
includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
Among these ports, The port from 0 to 4 are the user ports connecting
with the remote devices while the port 5 and 6 are the CPU ports
connecting into Mediatek Ethernet GMAC.

For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
through either the TRGMII or RGMII which could be controlled by phy-mode
in the dt-bindings to specify which mode is preferred to use. And for
port 5, only RGMII can be specified. However, currently, only port 6 is
being supported in this DSA driver.

The driver is made with the reference to qca8k and other existing DSA
driver. The most of the essential callbacks of the DSA are already
support in the driver, including tag insert for user port distinguishing,
port control, bridge offloading, STP setup and ethtool operation to allow
DSA to model each user port into a standalone netdevice as the other DSA
driver had done.

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/net/dsa/Kconfig  |    8 +
 drivers/net/dsa/Makefile |    2 +-
 drivers/net/dsa/mt7530.c | 1172 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mt7530.h |  382 +++++++++++++++
 4 files changed, 1563 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/mt7530.c
 create mode 100644 drivers/net/dsa/mt7530.h

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 0659846..5b322b4 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -34,4 +34,12 @@ config NET_DSA_QCA8K
 	  This enables support for the Qualcomm Atheros QCA8K Ethernet
 	  switch chips.
 
+config NET_DSA_MT7530
+	tristate "Mediatek MT7530 Ethernet switch support"
+	depends on NET_DSA
+	select NET_DSA_TAG_MTK
+	---help---
+	  This enables support for the Mediatek MT7530 Ethernet switch
+	  chip.
+
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index a3c9416..8e629c1 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -2,6 +2,6 @@ obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_BCM_SF2)	+= bcm-sf2.o
 bcm-sf2-objs			:= bcm_sf2.o bcm_sf2_cfp.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
-
+obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
 obj-y				+= b53/
 obj-y				+= mv88e6xxx/
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
new file mode 100644
index 0000000..99e5485
--- /dev/null
+++ b/drivers/net/dsa/mt7530.c
@@ -0,0 +1,1172 @@
+/*
+ * Mediatek MT7530 DSA Switch driver
+ * Copyright (C) 2017 Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *
+ * 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/if_bridge.h>
+#include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/of_gpio.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/of_platform.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <net/dsa.h>
+#include <net/switchdev.h>
+
+#include "mt7530.h"
+
+/* String, offset, and register size in bytes if different from 4 bytes */
+static const struct mt7530_mib_desc mt7530_mib[] = {
+	MIB_DESC(1, 0x00, "TxDrop"),
+	MIB_DESC(1, 0x04, "TxCrcErr"),
+	MIB_DESC(1, 0x08, "TxUnicast"),
+	MIB_DESC(1, 0x0c, "TxMulticast"),
+	MIB_DESC(1, 0x10, "TxBroadcast"),
+	MIB_DESC(1, 0x14, "TxCollision"),
+	MIB_DESC(1, 0x18, "TxSingleCollision"),
+	MIB_DESC(1, 0x1c, "TxMultipleCollision"),
+	MIB_DESC(1, 0x20, "TxDeferred"),
+	MIB_DESC(1, 0x24, "TxLateCollision"),
+	MIB_DESC(1, 0x28, "TxExcessiveCollistion"),
+	MIB_DESC(1, 0x2c, "TxPause"),
+	MIB_DESC(1, 0x30, "TxPktSz64"),
+	MIB_DESC(1, 0x34, "TxPktSz65To127"),
+	MIB_DESC(1, 0x38, "TxPktSz128To255"),
+	MIB_DESC(1, 0x3c, "TxPktSz256To511"),
+	MIB_DESC(1, 0x40, "TxPktSz512To1023"),
+	MIB_DESC(1, 0x44, "Tx1024ToMax"),
+	MIB_DESC(2, 0x48, "TxBytes"),
+	MIB_DESC(1, 0x60, "RxDrop"),
+	MIB_DESC(1, 0x64, "RxFiltering"),
+	MIB_DESC(1, 0x6c, "RxMulticast"),
+	MIB_DESC(1, 0x70, "RxBroadcast"),
+	MIB_DESC(1, 0x74, "RxAlignErr"),
+	MIB_DESC(1, 0x78, "RxCrcErr"),
+	MIB_DESC(1, 0x7c, "RxUnderSizeErr"),
+	MIB_DESC(1, 0x80, "RxFragErr"),
+	MIB_DESC(1, 0x84, "RxOverSzErr"),
+	MIB_DESC(1, 0x88, "RxJabberErr"),
+	MIB_DESC(1, 0x8c, "RxPause"),
+	MIB_DESC(1, 0x90, "RxPktSz64"),
+	MIB_DESC(1, 0x94, "RxPktSz65To127"),
+	MIB_DESC(1, 0x98, "RxPktSz128To255"),
+	MIB_DESC(1, 0x9c, "RxPktSz256To511"),
+	MIB_DESC(1, 0xa0, "RxPktSz512To1023"),
+	MIB_DESC(1, 0xa4, "RxPktSz1024ToMax"),
+	MIB_DESC(2, 0xa8, "RxBytes"),
+	MIB_DESC(1, 0xb0, "RxCtrlDrop"),
+	MIB_DESC(1, 0xb4, "RxIngressDrop"),
+	MIB_DESC(1, 0xb8, "RxArlDrop"),
+};
+
+static int
+mt7623_trgmii_write(struct mt7530_priv *priv,  u32 reg, u32 val)
+{
+	int ret;
+
+	ret =  regmap_write(priv->ethernet, TRGMII_BASE(reg), val);
+	if (ret < 0)
+		dev_err(priv->dev,
+			"failed to priv write register\n");
+	return ret;
+}
+
+static u32
+mt7623_trgmii_read(struct mt7530_priv *priv, u32 reg)
+{
+	int ret;
+	u32 val;
+
+	ret = regmap_read(priv->ethernet, TRGMII_BASE(reg), &val);
+	if (ret < 0) {
+		dev_err(priv->dev,
+			"failed to priv read register\n");
+		return ret;
+	}
+
+	return val;
+}
+
+static void
+mt7623_trgmii_rmw(struct mt7530_priv *priv, u32 reg,
+		  u32 mask, u32 set)
+{
+	u32 val;
+
+	val = mt7623_trgmii_read(priv, reg);
+	val &= ~mask;
+	val |= set;
+	mt7623_trgmii_write(priv, reg, val);
+}
+
+static void
+mt7623_trgmii_set(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	mt7623_trgmii_rmw(priv, reg, 0, val);
+}
+
+static void
+mt7623_trgmii_clear(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	mt7623_trgmii_rmw(priv, reg, val, 0);
+}
+
+static int
+core_read_mmd_indirect(struct mt7530_priv *priv, int prtad, int devad)
+{
+	struct mii_bus *bus = priv->bus;
+	int value, ret;
+
+	/* Write the desired MMD Devad */
+	ret = bus->write(bus, 0, MII_MMD_CTRL, devad);
+	if (ret < 0)
+		goto err;
+
+	/* Write the desired MMD register address */
+	ret = bus->write(bus, 0, MII_MMD_DATA, prtad);
+	if (ret < 0)
+		goto err;
+
+	/* Select the Function : DATA with no post increment */
+	ret = bus->write(bus, 0, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
+	if (ret < 0)
+		goto err;
+
+	/* Read the content of the MMD's selected register */
+	value = bus->read(bus, 0, MII_MMD_DATA);
+
+	return value;
+err:
+	dev_err(&bus->dev,  "failed to read mmd register\n");
+
+	return ret;
+}
+
+static int
+core_write_mmd_indirect(struct mt7530_priv *priv, int prtad,
+			int devad, u32 data)
+{
+	struct mii_bus *bus = priv->bus;
+	int ret;
+
+	/* Write the desired MMD Devad */
+	ret = bus->write(bus, 0, MII_MMD_CTRL, devad);
+	if (ret < 0)
+		goto err;
+
+	/* Write the desired MMD register address */
+	ret = bus->write(bus, 0, MII_MMD_DATA, prtad);
+	if (ret < 0)
+		goto err;
+
+	/* Select the Function : DATA with no post increment */
+	ret = bus->write(bus, 0, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
+	if (ret < 0)
+		goto err;
+
+	/* Write the data into MMD's selected register */
+	ret = bus->write(bus, 0, MII_MMD_DATA, data);
+err:
+	if (ret < 0)
+		dev_err(&bus->dev,
+			"failed to write mmd register\n");
+	return ret;
+}
+
+static void
+core_write(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	struct mii_bus *bus = priv->bus;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	core_write_mmd_indirect(priv, reg, MDIO_MMD_VEND2, val);
+
+	mutex_unlock(&bus->mdio_lock);
+}
+
+static void
+core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
+{
+	struct mii_bus *bus = priv->bus;
+	u32 val;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	val = core_read_mmd_indirect(priv, reg, MDIO_MMD_VEND2);
+	val &= ~mask;
+	val |= set;
+	core_write_mmd_indirect(priv, reg, MDIO_MMD_VEND2, val);
+
+	mutex_unlock(&bus->mdio_lock);
+}
+
+static void
+core_set(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	core_rmw(priv, reg, 0, val);
+}
+
+static void
+core_clear(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	core_rmw(priv, reg, val, 0);
+}
+
+static int
+mt7530_mii_write(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	struct mii_bus *bus = priv->bus;
+	u16 page, r, lo, hi;
+	int ret;
+
+	page = (reg >> 6) & 0x3ff;
+	r  = (reg >> 2) & 0xf;
+	lo = val & 0xffff;
+	hi = val >> 16;
+
+	/* MT7530 uses 31 as the pseudo port */
+	ret = bus->write(bus, 0x1f, 0x1f, page);
+	if (ret < 0)
+		goto err;
+
+	ret = bus->write(bus, 0x1f, r,  lo);
+	if (ret < 0)
+		goto err;
+
+	ret = bus->write(bus, 0x1f, 0x10, hi);
+err:
+	if (ret < 0)
+		dev_err(&bus->dev,
+			"failed to write mt7530 register\n");
+	return ret;
+}
+
+static u32
+mt7530_mii_read(struct mt7530_priv *priv, u32 reg)
+{
+	struct mii_bus *bus = priv->bus;
+	u16 page, r, lo, hi;
+	int ret;
+
+	page = (reg >> 6) & 0x3ff;
+	r = (reg >> 2) & 0xf;
+
+	/* MT7530 uses 31 as the pseudo port */
+	ret = bus->write(bus, 0x1f, 0x1f, page);
+	if (ret < 0) {
+		dev_err(&bus->dev,
+			"failed to read mt7530 register\n");
+		return ret;
+	}
+
+	lo = bus->read(bus, 0x1f, r);
+	hi = bus->read(bus, 0x1f, 0x10);
+
+	return (hi << 16) | (lo & 0xffff);
+}
+
+static void
+mt7530_write(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	struct mii_bus *bus = priv->bus;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	mt7530_mii_write(priv, reg, val);
+
+	mutex_unlock(&bus->mdio_lock);
+}
+
+static u32
+mt7530_read(struct mt7530_priv *priv, u32 reg)
+{
+	struct mii_bus *bus = priv->bus;
+	u32 val;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	val = mt7530_mii_read(priv, reg);
+
+	mutex_unlock(&bus->mdio_lock);
+
+	return val;
+}
+
+static void
+mt7530_rmw(struct mt7530_priv *priv, u32 reg,
+	   u32 mask, u32 set)
+{
+	struct mii_bus *bus = priv->bus;
+	u32 val;
+
+	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	val = mt7530_mii_read(priv, reg);
+	val &= ~mask;
+	val |= set;
+	mt7530_mii_write(priv, reg, val);
+
+	mutex_unlock(&bus->mdio_lock);
+}
+
+static void
+mt7530_set(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	mt7530_rmw(priv, reg, 0, val);
+}
+
+static void
+mt7530_clear(struct mt7530_priv *priv, u32 reg, u32 val)
+{
+	mt7530_rmw(priv, reg, val, 0);
+}
+
+static int
+mt7530_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
+{
+	struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
+
+	/* BIT(15) is used as indication for pseudo registers
+	 * which would be translated into the general MDIO
+	 * access to leverage the unique regmap sys interface.
+	 */
+	if (reg & BIT(15))
+		*val = mdiobus_read_nested(priv->bus,
+					   (reg & 0xf00) >> 8,
+					   (reg & 0xff) >> 2);
+	else
+		*val = mt7530_read(priv, reg);
+
+	return 0;
+}
+
+static int
+mt7530_regmap_write(void *ctx, uint32_t reg, uint32_t val)
+{
+	struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
+
+	if (reg & BIT(15))
+		mdiobus_write_nested(priv->bus,
+				     (reg & 0xf00) >> 8,
+				     (reg & 0xff) >> 2, val);
+	else
+		mt7530_write(priv, reg, val);
+
+	return 0;
+}
+
+static const struct regmap_range mt7530_readable_ranges[] = {
+	regmap_reg_range(0x0000, 0x00ac), /* Global control */
+	regmap_reg_range(0x2000, 0x202c), /* Port Control - P0 */
+	regmap_reg_range(0x2100, 0x212c), /* Port Control - P1 */
+	regmap_reg_range(0x2200, 0x222c), /* Port Control - P2 */
+	regmap_reg_range(0x2300, 0x232c), /* Port Control - P3 */
+	regmap_reg_range(0x2400, 0x242c), /* Port Control - P4 */
+	regmap_reg_range(0x2500, 0x252c), /* Port Control - P5 */
+	regmap_reg_range(0x2600, 0x262c), /* Port Control - P6 */
+	regmap_reg_range(0x30e0, 0x30f8), /* Port MAC - SYS */
+	regmap_reg_range(0x3000, 0x3014), /* Port MAC - P0 */
+	regmap_reg_range(0x3100, 0x3114), /* Port MAC - P1 */
+	regmap_reg_range(0x3200, 0x3214), /* Port MAC - P2*/
+	regmap_reg_range(0x3300, 0x3314), /* Port MAC - P3*/
+	regmap_reg_range(0x3400, 0x3414), /* Port MAC - P4 */
+	regmap_reg_range(0x3500, 0x3514), /* Port MAC - P5 */
+	regmap_reg_range(0x3600, 0x3614), /* Port MAC - P6 */
+	regmap_reg_range(0x4000, 0x40d4), /* MIB - P0 */
+	regmap_reg_range(0x4100, 0x41d4), /* MIB - P1 */
+	regmap_reg_range(0x4200, 0x42d4), /* MIB - P2 */
+	regmap_reg_range(0x4300, 0x43d4), /* MIB - P3 */
+	regmap_reg_range(0x4400, 0x44d4), /* MIB - P4 */
+	regmap_reg_range(0x4500, 0x45d4), /* MIB - P5 */
+	regmap_reg_range(0x4600, 0x46d4), /* MIB - P6 */
+	regmap_reg_range(0x4fe0, 0x4ff4), /* SYS */
+	regmap_reg_range(0x7000, 0x700c), /* SYS 2 */
+	regmap_reg_range(0x7018, 0x7028), /* SYS 3 */
+	regmap_reg_range(0x7800, 0x7830), /* SYS 4 */
+	regmap_reg_range(0x7a00, 0x7a7c), /* TRGMII */
+	regmap_reg_range(0x8000, 0x8078), /* Psedo address for Phy - P0 */
+	regmap_reg_range(0x8100, 0x8178), /* Psedo address for Phy - P1 */
+	regmap_reg_range(0x8200, 0x8278), /* Psedo address for Phy - P2 */
+	regmap_reg_range(0x8300, 0x8378), /* Psedo address for Phy - P3 */
+	regmap_reg_range(0x8400, 0x8478), /* Psedo address for Phy - P4 */
+};
+
+static const struct regmap_access_table mt7530_readable_table = {
+	.yes_ranges = mt7530_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(mt7530_readable_ranges),
+};
+
+static struct regmap_config mt7530_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0x8478,
+	.reg_read = mt7530_regmap_read,
+	.reg_write = mt7530_regmap_write,
+	.rd_table = &mt7530_readable_table,
+};
+
+static int
+mt7530_fdb_cmd(struct mt7530_priv *priv, enum mt7530_fdb_cmd cmd, u32 *rsp)
+{
+	u32 reg;
+	int ret;
+
+	/* Set the command operating upon the MAC address entries */
+	reg = ATC_BUSY | ATC_MAT(0) | cmd;
+	mt7530_write(priv, MT7530_ATC, reg);
+
+	/* Wait for completion */
+	ret = wait_condition_timeout(
+		!(mt7530_read(priv, MT7530_ATC) & ATC_BUSY), 20);
+	if (ret < 0) {
+		dev_err(priv->dev, "cmd = %x timeout\n", cmd);
+		return -EIO;
+	}
+
+	/* Additional sanity for read command if the specified
+	 * entry is invalid
+	 */
+	reg = mt7530_read(priv, MT7530_ATC);
+	if ((cmd == MT7530_FDB_READ) && (reg & ATC_INVALID))
+		return -EINVAL;
+
+	if (rsp)
+		*rsp = reg;
+
+	return 0;
+}
+
+static void
+mt7530_fdb_read(struct mt7530_priv *priv, struct mt7530_fdb *fdb)
+{
+	u32 reg[3];
+	int i;
+
+	/* Read from ARL table into an array */
+	for (i = 0; i < 3; i++) {
+		reg[i] = mt7530_read(priv, MT7530_TSRA1 + (i * 4));
+
+		dev_dbg(priv->dev, "%s(%d) reg[%d]=0x%x\n",
+			__func__, __LINE__, i, reg[i]);
+	}
+
+	/* vid - 11:0 on reg[1] */
+	fdb->vid = (reg[1] >> 0) & 0xfff;
+	/* aging - 31:24 on reg[2] */
+	fdb->aging = (reg[2] >> 24) & 0xff;
+	/* portmask - 11:4 on reg[2] */
+	fdb->port_mask = (reg[2] >> 4) & 0xff;
+	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
+	fdb->mac[0] = (reg[0] >> 24) & 0xff;
+	fdb->mac[1] = (reg[0] >> 16) & 0xff;
+	fdb->mac[2] = (reg[0] >>  8) & 0xff;
+	fdb->mac[3] = (reg[0] >>  0) & 0xff;
+	fdb->mac[4] = (reg[1] >> 24) & 0xff;
+	fdb->mac[5] = (reg[1] >> 16) & 0xff;
+	/* noarp - 3:2 on reg[2] */
+	fdb->noarp = ((reg[2] >> 2) & 0x3) == STATIC_ENT;
+}
+
+static void
+mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
+		 u8 port_mask, const u8 *mac,
+		 u8 aging, u8 type)
+{
+	u32 reg[3] = { 0 };
+	int i;
+
+	/* vid - 11:0 on reg[1] */
+	reg[1] |= (vid & 0xfff) << 0;
+	/* aging - 31:25 on reg[2] */
+	reg[2] |= (aging & 0xff) << 24;
+	/* portmask - 11:4 on reg[2] */
+	reg[2] |= (port_mask & 0xff) << 4;
+	/* type - 3 indicate that entry is static wouldn't
+	 * be aged out and 0 specified as erasing an entry
+	 */
+	reg[2] |= (type & 0x3) << 2;
+	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
+	reg[1] |= mac[5] << 16;
+	reg[1] |= mac[4] << 24;
+	reg[0] |= mac[3] << 0;
+	reg[0] |= mac[2] << 8;
+	reg[0] |= mac[1] << 16;
+	reg[0] |= mac[0] << 24;
+
+	/* Wrirte array into the ARL table */
+	for (i = 0; i < 3; i++)
+		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
+}
+
+static int
+mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u32 ncpo1, ssc_delta, trgint, i;
+
+	switch (mode) {
+	case PHY_INTERFACE_MODE_RGMII:
+		trgint = 0;
+		ncpo1 = 0x0c80;
+		ssc_delta = 0x87;
+		break;
+	case PHY_INTERFACE_MODE_TRGMII:
+		trgint = 1;
+		ncpo1 = 0x1400;
+		ssc_delta = 0x57;
+		break;
+	default:
+		pr_err("xMII mode %d not supported\n", mode);
+		return -EINVAL;
+	}
+
+	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+		   P6_INTF_MODE(trgint));
+
+	/* Lower Tx Driving */
+	for (i = 0 ; i < 6 ; i++)
+		mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
+			     TD_DM_DRVP(8) | TD_DM_DRVN(8));
+
+	/* Setup MT7530 core clock */
+	if (!trgint) {
+		/* Disable MT7530 core clock */
+		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
+
+		/* Disable MT7530 PLL, since phy_device has not yet been
+		 * created when this function is called. So we provide
+		 * core_write_mmd_indirect to complete this function
+		 */
+		core_write_mmd_indirect(priv,
+					CORE_GSWPLL_GRP1,
+					MDIO_MMD_VEND2,
+					0);
+
+		/* Setup MT7530 core clock into 500Mhz */
+		core_write(priv, CORE_GSWPLL_GRP2,
+			   RG_GSWPLL_POSDIV_500M(1) |
+			   RG_GSWPLL_FBKDIV_500M(25));
+
+		/* Enable MT7530 PLL */
+		core_write(priv, CORE_GSWPLL_GRP1,
+			   RG_GSWPLL_EN_PRE |
+			   RG_GSWPLL_POSDIV_200M(2) |
+			   RG_GSWPLL_FBKDIV_200M(32));
+
+		/* Enable MT7530 core clock */
+		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
+	}
+
+	/* Setup the MT7530 TRGMII Tx Clock */
+	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
+	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
+	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
+	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP4,
+		   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
+		   RG_SYSPLL_BIAS_LPF_EN);
+	core_write(priv, CORE_PLL_GROUP2,
+		   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
+		   RG_SYSPLL_POSDIV(1));
+	core_write(priv, CORE_PLL_GROUP7,
+		   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
+		   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
+	core_set(priv, CORE_TRGMII_GSW_CLK_CG,
+		 REG_GSWCK_EN | REG_TRGMIICK_EN);
+
+	if (!trgint)
+		for (i = 0 ; i < 5 ; i++)
+			mt7530_rmw(priv, MT7530_TRGMII_RD(i),
+				   RD_TAP_MASK, RD_TAP(16));
+	else
+		mt7623_trgmii_set(priv, GSW_INTF_MODE, INTF_MODE_TRGMII);
+
+	return 0;
+}
+
+static int
+mt7623_pad_clk_setup(struct dsa_switch *ds)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int i;
+
+	for (i = 0 ; i < 6; i++)
+		mt7623_trgmii_write(priv, GSW_TRGMII_TD_ODT(i),
+				    TD_DM_DRVP(8) | TD_DM_DRVN(8));
+
+	mt7623_trgmii_set(priv, GSW_TRGMII_RCK_CTRL, RX_RST | RXC_DQSISEL);
+	mt7623_trgmii_clear(priv, GSW_TRGMII_RCK_CTRL, RX_RST);
+
+	return 0;
+}
+
+static void
+mt7530_mib_reset(struct dsa_switch *ds)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_FLUSH);
+	mt7530_write(priv, MT7530_MIB_CCR, CCR_MIB_ACTIVATE);
+}
+
+static void
+mt7530_port_set_status(struct dsa_switch *ds, int port, int enable)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u32 mask = PMCR_TX_EN | PMCR_RX_EN;
+
+	mutex_lock(&priv->reg_mutex);
+	if (enable)
+		mt7530_set(priv, MT7530_PMCR_P(port), mask);
+	else
+		mt7530_clear(priv, MT7530_PMCR_P(port), mask);
+	mutex_unlock(&priv->reg_mutex);
+}
+
+static int
+mt7530_setup(struct dsa_switch *ds)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int ret, i, phy_mode;
+	u8  cpup_mask = 0;
+	u32 id, val;
+	struct regmap *regmap;
+	struct device_node *dn;
+
+	/* Make sure that cpu port specfied on the dt is appropriate */
+	if (!dsa_is_cpu_port(ds, MT7530_CPU_PORT)) {
+		dev_err(priv->dev, "port not matched with the CPU port\n");
+		return -EINVAL;
+	}
+
+	/* The parent node of master_netdev which holds the common system
+	 * controller also is the container for two GMACs nodes representing
+	 * as two netdev instances.
+	 */
+	dn = ds->master_netdev->dev.of_node->parent;
+	priv->ethernet = syscon_node_to_regmap(dn);
+	if (IS_ERR(priv->ethernet))
+		return PTR_ERR(priv->ethernet);
+
+	regmap = devm_regmap_init(ds->dev, NULL, priv,
+				  &mt7530_regmap_config);
+	if (IS_ERR(regmap))
+		dev_warn(priv->dev, "phy regmap initialization failed");
+
+	phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
+	if (phy_mode < 0) {
+		dev_err(priv->dev, "Can't find phy-mode for master device\n");
+		return phy_mode;
+	}
+	dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);
+
+	regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
+	ret = regulator_enable(priv->core_pwr);
+	if (ret < 0) {
+		dev_err(priv->dev,
+			"Failed to enable core power: %d\n", ret);
+		return ret;
+	}
+
+	regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
+	ret = regulator_enable(priv->io_pwr);
+	if (ret < 0) {
+		dev_err(priv->dev, "Failed to enable io pwr: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* Reset whole chip through gpio pin or memory-mapped registers for
+	 * different type of hardware
+	 */
+	if (priv->mcm) {
+		reset_control_assert(priv->rstc);
+		usleep_range(1000, 1100);
+		reset_control_deassert(priv->rstc);
+	} else {
+		gpiod_set_value_cansleep(priv->reset, 0);
+		usleep_range(1000, 1100);
+		gpiod_set_value_cansleep(priv->reset, 1);
+	}
+
+	/* Wait until the reset completion */
+	ret = wait_condition_timeout(mt7530_read(priv, MT7530_HWTRAP) != 0,
+				     1000);
+	if (ret < 0) {
+		dev_err(priv->dev, "reset timeout\n");
+		return ret;
+	}
+
+	id = mt7530_read(priv, MT7530_CREV);
+	id >>= CHIP_NAME_SHIFT;
+	if (id != MT7530_ID)
+		return -ENODEV;
+
+	/* Reset the switch through internal reset */
+	mt7530_write(priv, MT7530_SYS_CTRL,
+		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
+		     SYS_CTRL_REG_RST);
+
+	/* Enable Port 6 only, P5 as GMAC5 which currently is not supported */
+	val = mt7530_read(priv, MT7530_MHWTRAP);
+	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
+	val |= MHWTRAP_MANUAL;
+	mt7530_write(priv, MT7530_MHWTRAP, val);
+
+	ret = mt7530_pad_clk_setup(ds, phy_mode);
+	if (ret < 0)
+		return ret;
+
+	/* Enable and reset MIB counters */
+	mt7530_mib_reset(ds);
+
+	/* Disable forwarding by default on all ports */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		mt7530_write(priv, MT7530_PCR_P(i), PCR_MATRIX_INIT);
+
+	mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
+
+	/* Fabric setup for the cpu port */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		if (dsa_is_cpu_port(ds, i)) {
+			/* Enable Mediatek header mode on the cpu port */
+			mt7530_write(priv, MT7530_PVC_P(i),
+				     PORT_SPEC_TAG);
+
+			/* Setup the MAC by default for the cpu port */
+			mt7530_write(priv, MT7530_PMCR_P(i), PMCR_CPUP_LINK);
+
+			/* Disable auto learning on the cpu port */
+			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
+
+			/* Unknown unicast frame fordwarding to the cpu port */
+			mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(i)));
+
+			/* CPU port gets connected to all user ports of
+			 * the switch
+			 */
+			mt7530_write(priv, MT7530_PCR_P(i),
+				     PCR_MATRIX(ds->enabled_port_mask));
+
+			cpup_mask |= BIT(i);
+		}
+
+	/* Fabric setup for the all user ports */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		if (ds->enabled_port_mask & BIT(i)) {
+			/* Setup the MAC by default for all user ports */
+			mt7530_write(priv, MT7530_PMCR_P(i),
+				     PMCR_USERP_LINK);
+
+			/* The user port gets connected to the cpu port only */
+			mt7530_write(priv, MT7530_PCR_P(i),
+				     PCR_MATRIX(cpup_mask));
+		}
+
+	/* Flush the FDB table */
+	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Setup RX circuit, relevant PAD and driving on the host which must
+	 * be placed after the setup on the device side is all finished.
+	 */
+	ret = mt7623_pad_clk_setup(ds);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int mt7530_phy_read(struct dsa_switch *ds, int port, int regnum)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return mdiobus_read_nested(priv->bus, port, regnum);
+}
+
+int mt7530_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
+{
+	struct mt7530_priv *priv = ds->priv;
+
+	return mdiobus_write_nested(priv->bus, port, regnum, val);
+}
+
+static void
+mt7530_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++)
+		strncpy(data + i * ETH_GSTRING_LEN, mt7530_mib[i].name,
+			ETH_GSTRING_LEN);
+}
+
+static void
+mt7530_get_ethtool_stats(struct dsa_switch *ds, int port,
+			 uint64_t *data)
+{
+	struct mt7530_priv *priv = ds->priv;
+	const struct mt7530_mib_desc *mib;
+	u32 reg, i;
+	u64 hi;
+
+	for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++) {
+		mib = &mt7530_mib[i];
+		reg = MT7530_PORT_MIB_COUNTER(port) + mib->offset;
+
+		data[i] = mt7530_read(priv, reg);
+		if (mib->size == 2) {
+			hi = mt7530_read(priv, reg + 4);
+			data[i] |= hi << 32;
+		}
+	}
+}
+
+static int
+mt7530_get_sset_count(struct dsa_switch *ds)
+{
+	return ARRAY_SIZE(mt7530_mib);
+}
+
+static int
+mt7530_port_enable(struct dsa_switch *ds, int port,
+		   struct phy_device *phy)
+{
+	mt7530_port_set_status(ds, port, 1);
+
+	return 0;
+}
+
+static void
+mt7530_port_disable(struct dsa_switch *ds, int port,
+		    struct phy_device *phy)
+{
+	mt7530_port_set_status(ds, port, 0);
+}
+
+static void
+mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u32 stp_state;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+		stp_state = MT7530_STP_DISABLED;
+		break;
+	case BR_STATE_BLOCKING:
+		stp_state = MT7530_STP_BLOCKING;
+		break;
+	case BR_STATE_LISTENING:
+		stp_state = MT7530_STP_LISTENING;
+		break;
+	case BR_STATE_LEARNING:
+		stp_state = MT7530_STP_LEARNING;
+		break;
+	case BR_STATE_FORWARDING:
+	default:
+		stp_state = MT7530_STP_FORWARDING;
+		break;
+	}
+
+	mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, stp_state);
+}
+
+static int
+mt7530_port_bridge_join(struct dsa_switch *ds, int port,
+			struct net_device *bridge)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int port_bitmap = BIT(MT7530_CPU_PORT);
+	int i;
+
+	mutex_lock(&priv->reg_mutex);
+
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		if (ds->enabled_port_mask & BIT(i)) {
+			if (ds->ports[i].bridge_dev != bridge)
+				continue;
+			/* Add this port to the port maxtrix of the
+			 * other ports in the bridge
+			 */
+			mt7530_set(priv, MT7530_PCR_P(i),
+				   PCR_MATRIX(BIT(port)));
+
+			if (i != port)
+				port_bitmap |= BIT(i);
+		}
+
+	/* Add all other ports to this port matrix */
+	mt7530_rmw(priv, MT7530_PCR_P(port),
+		   PCR_MATRIX_MASK, PCR_MATRIX(port_bitmap));
+
+	mutex_unlock(&priv->reg_mutex);
+
+	return 0;
+}
+
+static void
+mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
+			 struct net_device *bridge)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int i;
+
+	mutex_lock(&priv->reg_mutex);
+
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		if (ds->enabled_port_mask & BIT(i)) {
+			if (ds->ports[i].bridge_dev != bridge)
+				continue;
+			/* Remove this port from the port maxtrix
+			 * of the other ports in the bridge.
+			 */
+			mt7530_clear(priv, MT7530_PCR_P(i),
+				     PCR_MATRIX(BIT(port)));
+		}
+
+	/* Set the cpu port to be the only one in the port matrix of
+	 * this port.
+	 */
+	mt7530_rmw(priv, MT7530_PCR_P(port),
+		   PCR_MATRIX_MASK,
+		   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
+
+	mutex_unlock(&priv->reg_mutex);
+}
+
+static int
+mt7530_port_fdb_prepare(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_port_fdb *fdb,
+			struct switchdev_trans *trans)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int ret;
+
+	/* Because auto-learned entries shares the same FDB table.
+	 * an entry is reserved with no port_mask to make sure fdb_add
+	 * is called while the entry is still available.
+	 */
+	mutex_lock(&priv->reg_mutex);
+	mt7530_fdb_write(priv, fdb->vid, 0, fdb->addr, -1, STATIC_ENT);
+	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static void
+mt7530_port_fdb_add(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_fdb *fdb,
+		    struct switchdev_trans *trans)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u8 port_mask = BIT(port);
+
+	mutex_lock(&priv->reg_mutex);
+	mt7530_fdb_write(priv, fdb->vid, port_mask, fdb->addr, -1, STATIC_ENT);
+	mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
+	mutex_unlock(&priv->reg_mutex);
+}
+
+static int
+mt7530_port_fdb_del(struct dsa_switch *ds, int port,
+		    const struct switchdev_obj_port_fdb *fdb)
+{
+	struct mt7530_priv *priv = ds->priv;
+	int ret;
+	u8 port_mask = BIT(port);
+
+	mutex_lock(&priv->reg_mutex);
+	mt7530_fdb_write(priv, fdb->vid, port_mask, fdb->addr, -1, STATIC_EMP);
+	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
+	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
+}
+
+static int
+mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
+		     struct switchdev_obj_port_fdb *fdb,
+		     int (*cb)(struct switchdev_obj *obj))
+{
+	struct mt7530_priv *priv = ds->priv;
+	struct mt7530_fdb _fdb = { 0 };
+	int cnt = MT7530_NUM_FDB_RECORDS;
+	int ret = 0;
+	u32 rsp = 0;
+
+	mutex_lock(&priv->reg_mutex);
+
+	ret = mt7530_fdb_cmd(priv, MT7530_FDB_START, &rsp);
+	if (ret < 0)
+		goto err;
+
+	do {
+		if (rsp & ATC_SRCH_HIT) {
+			mt7530_fdb_read(priv, &_fdb);
+			if (_fdb.port_mask & BIT(port)) {
+				ether_addr_copy(fdb->addr, _fdb.mac);
+				fdb->vid = _fdb.vid;
+				fdb->ndm_state = _fdb.noarp ?
+						NUD_NOARP : NUD_REACHABLE;
+				ret = cb(&fdb->obj);
+				if (ret < 0)
+					break;
+			}
+		}
+	} while (--cnt &&
+		 !(rsp & ATC_SRCH_END) &&
+		 !mt7530_fdb_cmd(priv, MT7530_FDB_NEXT, &rsp));
+err:
+	mutex_unlock(&priv->reg_mutex);
+
+	return 0;
+}
+
+static enum dsa_tag_protocol
+mtk_get_tag_protocol(struct dsa_switch *ds)
+{
+	return DSA_TAG_PROTO_MTK;
+}
+
+static struct dsa_switch_ops mt7530_switch_ops = {
+	.get_tag_protocol	= mtk_get_tag_protocol,
+	.setup			= mt7530_setup,
+	.get_strings		= mt7530_get_strings,
+	.phy_read		= mt7530_phy_read,
+	.phy_write		= mt7530_phy_write,
+	.get_ethtool_stats	= mt7530_get_ethtool_stats,
+	.get_sset_count		= mt7530_get_sset_count,
+	.port_enable		= mt7530_port_enable,
+	.port_disable		= mt7530_port_disable,
+	.port_stp_state_set	= mt7530_stp_state_set,
+	.port_bridge_join	= mt7530_port_bridge_join,
+	.port_bridge_leave	= mt7530_port_bridge_leave,
+	.port_fdb_prepare	= mt7530_port_fdb_prepare,
+	.port_fdb_add		= mt7530_port_fdb_add,
+	.port_fdb_del		= mt7530_port_fdb_del,
+	.port_fdb_dump		= mt7530_port_fdb_dump,
+};
+
+static int
+mt7530_probe(struct mdio_device *mdiodev)
+{
+	struct mt7530_priv *priv;
+	struct device_node *dn;
+
+	dn = mdiodev->dev.of_node;
+
+	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ds = dsa_switch_alloc(&mdiodev->dev, DSA_MAX_PORTS);
+	if (!priv->ds)
+		return -ENOMEM;
+
+	/* Use medatek,mcm property to distinguish hardware type that would
+	 * casues a little bit differences on power-on sequence.
+	 */
+	priv->mcm = of_property_read_bool(dn, "mediatek,mcm");
+	if (priv->mcm) {
+		dev_info(&mdiodev->dev, "MT7530 adapts as multi-chip module\n");
+
+		priv->rstc = devm_reset_control_get(&mdiodev->dev, "mcm");
+		if (IS_ERR(priv->rstc)) {
+			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
+			return PTR_ERR(priv->rstc);
+		}
+	}
+
+	priv->core_pwr = devm_regulator_get(&mdiodev->dev, "core");
+	if (IS_ERR(priv->core_pwr))
+		return PTR_ERR(priv->core_pwr);
+
+	priv->io_pwr = devm_regulator_get(&mdiodev->dev, "io");
+	if (IS_ERR(priv->io_pwr))
+		return PTR_ERR(priv->io_pwr);
+
+	/* Not MCM that indicates switch works as the remote standalone
+	 * integrated circuit so the GPIO pin would be used to complete
+	 * the reset, otherwise memory-mapped register accessing used
+	 * through syscon provides in the case of MCM.
+	 */
+	if (!priv->mcm) {
+		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
+						      GPIOD_OUT_LOW);
+		if (IS_ERR(priv->reset)) {
+			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
+			return PTR_ERR(priv->reset);
+		}
+	}
+
+	priv->bus = mdiodev->bus;
+	priv->dev = &mdiodev->dev;
+	priv->ds->priv = priv;
+	priv->ds->ops = &mt7530_switch_ops;
+	mutex_init(&priv->reg_mutex);
+	dev_set_drvdata(&mdiodev->dev, priv);
+
+	return dsa_register_switch(priv->ds, &mdiodev->dev);
+}
+
+static void
+mt7530_remove(struct mdio_device *mdiodev)
+{
+	struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
+	int ret = 0;
+
+	ret = regulator_disable(priv->core_pwr);
+	if (ret < 0)
+		dev_err(priv->dev,
+			"Failed to disable core power: %d\n", ret);
+
+	ret = regulator_disable(priv->io_pwr);
+	if (ret < 0)
+		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
+			ret);
+
+	dsa_unregister_switch(priv->ds);
+	mutex_destroy(&priv->reg_mutex);
+}
+
+static const struct of_device_id mt7530_of_match[] = {
+	{ .compatible = "mediatek,mt7530" },
+	{ /* sentinel */ },
+};
+
+static struct mdio_driver mt7530_mdio_driver = {
+	.probe  = mt7530_probe,
+	.remove = mt7530_remove,
+	.mdiodrv.driver = {
+		.name = "mt7530",
+		.of_match_table = mt7530_of_match,
+	},
+};
+
+mdio_module_driver(mt7530_mdio_driver);
+
+MODULE_AUTHOR("Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("Driver for Mediatek MT7530 Switch");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:mediatek-mt7530");
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
new file mode 100644
index 0000000..ba1e1cd
--- /dev/null
+++ b/drivers/net/dsa/mt7530.h
@@ -0,0 +1,382 @@
+/*
+ * Copyright (C) 2017 Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *
+ * 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.
+ */
+
+#ifndef __MT7530_H
+#define __MT7530_H
+
+#define MT7530_NUM_PORTS		7
+#define MT7530_CPU_PORT			6
+#define MT7530_NUM_FDB_RECORDS		2048
+
+#define TRGMII_BASE(x)			(0x10000 + (x))
+
+/* Registers to ethsys access */
+#define ETHSYS_CLKCFG0			0x2c
+#define  ETHSYS_TRGMII_CLK_SEL362_5	BIT(11)
+
+#define SYSC_REG_RSTCTRL		0x34
+#define  RESET_MCM			BIT(2)
+
+/* Registers to mac forward conrol for unknown frames */
+#define MT7530_MFC			0x10
+#define  BC_FFP(x)			(((x) & 0xff) << 24)
+#define  UNM_FFP(x)			(((x) & 0xff) << 16)
+#define  UNU_FFP(x)			(((x) & 0xff) << 8)
+#define  UNU_FFP_MASK			UNU_FFP(~0)
+
+/* Registers for address table access */
+
+#define MT7530_ATA1			0x74
+#define  STATIC_EMP			0
+#define  STATIC_ENT			3
+#define MT7530_ATA2			0x78
+
+/* Register for address table write data */
+#define MT7530_ATWD			0x7c
+
+/* Register for address table control */
+#define MT7530_ATC			0x80
+#define  ATC_HASH			(((x) & 0xfff) << 16)
+#define  ATC_BUSY			BIT(15)
+#define  ATC_SRCH_END			BIT(14)
+#define  ATC_SRCH_HIT			BIT(13)
+#define  ATC_INVALID			BIT(12)
+#define  ATC_MAT(x)			(((x) & 0xf) << 8)
+#define  ATC_MAT_MACTAB			ATC_MAT(0)
+
+enum mt7530_fdb_cmd {
+	MT7530_FDB_READ	= 0,
+	MT7530_FDB_WRITE = 1,
+	MT7530_FDB_FLUSH = 2,
+	MT7530_FDB_START = 4,
+	MT7530_FDB_NEXT = 5,
+};
+
+/* Registers for table search read address */
+#define MT7530_TSRA1			0x84
+#define MT7530_TSRA2			0x88
+#define MT7530_ATRD			0x8C
+
+/* Register for vlan table control */
+#define MT7530_VTCR			0x90
+#define  VTCR_BUSY			BIT(31)
+#define  VTCR_FUNC			(((x) & 0xf) << 12)
+#define  VTCR_FUNC_RD_VID		0x1
+#define  VTCR_FUNC_WR_VID		0x2
+#define  VTCR_FUNC_INV_VID		0x3
+#define  VTCR_FUNC_VAL_VID		0x4
+#define  VTCR_VID			((x) & 0xfff)
+
+/* Register for setup vlan and acl write data */
+#define MT7530_VAWD1			0x94
+#define  PORT_STAG			BIT(31)
+#define  IVL_MAC			BIT(30)
+#define  PORT_MEM(x)			(((x) & 0xff) << 16)
+#define  VALID				BIT(1)
+
+#define MT7530_VAWD2			0x98
+
+/* Register for port STP state control */
+#define MT7530_SSP_P(x)			(0x2000 + ((x) * 0x100))
+#define  FID_PST(x)			((x) & 0x3)
+#define  FID_PST_MASK			FID_PST(0x3)
+
+enum mt7530_stp_state {
+	MT7530_STP_DISABLED = 0,
+	MT7530_STP_BLOCKING = 1,
+	MT7530_STP_LISTENING = 1,
+	MT7530_STP_LEARNING = 2,
+	MT7530_STP_FORWARDING  = 3
+};
+
+/* Registor for port control */
+#define MT7530_PCR_P(x)			(0x2004 + ((x) * 0x100))
+#define  PORT_VLAN(x)			((x) & 0x3)
+#define  PCR_MATRIX(x)			(((x) & 0xff) << 16)
+#define  PORT_PRI(x)			(((x) & 0x7) << 24)
+#define  EG_TAG(x)			(((x) & 0x3) << 28)
+#define  PCR_MATRIX_MASK		PCR_MATRIX(0xff)
+#define  PCR_MATRIX_INIT		PCR_MATRIX(0)
+
+/* Register for port security control */
+#define MT7530_PSC_P(x)			(0x200c + ((x) * 0x100))
+#define  SA_DIS				BIT(4)
+
+/* Register for port vlan control */
+#define MT7530_PVC_P(x)			(0x2010 + ((x) * 0x100))
+#define  PORT_SPEC_TAG			BIT(5)
+#define  VLAN_ATTR(x)			(((x) & 0x3) << 6)
+#define  STAG_VPID			(((x) & 0xffff) << 16)
+
+/* Register for port port-and-protocol based vlan 1 control */
+#define MT7530_PPBV1_P(x)		(0x2014 + ((x) * 0x100))
+
+/* Register for port MAC control register */
+#define MT7530_PMCR_P(x)		(0x3000 + ((x) * 0x100))
+#define  PMCR_IFG_XMIT(x)		(((x) & 0x3) << 18)
+#define  PMCR_MAC_MODE			BIT(16)
+#define  PMCR_FORCE_MODE		BIT(15)
+#define  PMCR_TX_EN			BIT(14)
+#define  PMCR_RX_EN			BIT(13)
+#define  PMCR_BACKOFF_EN		BIT(9)
+#define  PMCR_BACKPR_EN			BIT(8)
+#define  PMCR_TX_FC_EN			BIT(5)
+#define  PMCR_RX_FC_EN			BIT(4)
+#define  PMCR_FORCE_SPEED_1000		BIT(3)
+#define  PMCR_FORCE_FDX			BIT(1)
+#define  PMCR_FORCE_LNK			BIT(0)
+#define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
+					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
+					 PMCR_TX_EN | PMCR_RX_EN | \
+					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
+#define  PMCR_CPUP_LINK			(PMCR_COMMON_LINK | PMCR_FORCE_MODE | \
+					 PMCR_FORCE_SPEED_1000 | \
+					 PMCR_FORCE_FDX | \
+					 PMCR_FORCE_LNK)
+#define  PMCR_USERP_LINK		PMCR_COMMON_LINK
+#define  PMCR_FIXED_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
+					 PMCR_FORCE_MODE | PMCR_TX_EN | \
+					 PMCR_RX_EN | PMCR_BACKPR_EN | \
+					 PMCR_BACKOFF_EN | \
+					 PMCR_FORCE_SPEED_1000 | \
+					 PMCR_FORCE_FDX | \
+					 PMCR_FORCE_LNK)
+#define PMCR_FIXED_LINK_FC		(PMCR_FIXED_LINK | \
+					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
+
+#define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
+
+/* Register for MIB */
+#define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)
+#define MT7530_MIB_CCR			0x4fe0
+#define  CCR_MIB_ENABLE			BIT(31)
+#define  CCR_RX_OCT_CNT_GOOD		BIT(7)
+#define  CCR_RX_OCT_CNT_BAD		BIT(6)
+#define  CCR_TX_OCT_CNT_GOOD		BIT(5)
+#define  CCR_TX_OCT_CNT_BAD		BIT(4)
+#define  CCR_MIB_FLUSH			(CCR_RX_OCT_CNT_GOOD | \
+					 CCR_RX_OCT_CNT_BAD | \
+					 CCR_TX_OCT_CNT_GOOD | \
+					 CCR_TX_OCT_CNT_BAD)
+#define  CCR_MIB_ACTIVATE		(CCR_MIB_ENABLE | \
+					 CCR_RX_OCT_CNT_GOOD | \
+					 CCR_RX_OCT_CNT_BAD | \
+					 CCR_TX_OCT_CNT_GOOD | \
+					 CCR_TX_OCT_CNT_BAD)
+/* Register for system reset */
+#define MT7530_SYS_CTRL			0x7000
+#define  SYS_CTRL_PHY_RST		BIT(2)
+#define  SYS_CTRL_SW_RST		BIT(1)
+#define  SYS_CTRL_REG_RST		BIT(0)
+
+/* Register for hw trap status */
+#define MT7530_HWTRAP			0x7800
+
+/* Register for hw trap modification */
+#define MT7530_MHWTRAP			0x7804
+#define  MHWTRAP_MANUAL			BIT(16)
+#define  MHWTRAP_P5_MAC_SEL		BIT(13)
+#define  MHWTRAP_P6_DIS			BIT(8)
+#define  MHWTRAP_P5_RGMII_MODE		BIT(7)
+#define  MHWTRAP_P5_DIS			BIT(6)
+#define  MHWTRAP_PHY_ACCESS		BIT(5)
+
+/* Regiser for TOP signal control */
+#define MT7530_TOP_SIG_CTRL		0x7808
+#define  TOP_SIG_CTRL_NORMAL		(BIT(17) | BIT(16))
+
+#define MT7530_IO_DRV_CR		0x7810
+#define  P5_IO_CLK_DRV(x)		((x) & 0x3)
+#define  P5_IO_DATA_DRV(x)		(((x) & 0x3) << 4)
+
+#define MT7530_P6ECR			0x7830
+#define  P6_INTF_MODE_MASK		0x3
+#define  P6_INTF_MODE(x)		((x) & 0x3)
+
+/* Registers for TRGMII on the both side */
+#define MT7530_TRGMII_RCK_CTRL		0x7a00
+#define GSW_TRGMII_RCK_CTRL		0x300
+#define  RX_RST				BIT(31)
+#define  RXC_DQSISEL			BIT(30)
+#define  DQSI1_TAP_MASK			(0x7f << 8)
+#define  DQSI0_TAP_MASK			0x7f
+#define  DQSI1_TAP(x)			(((x) & 0x7f) << 8)
+#define  DQSI0_TAP(x)			((x) & 0x7f)
+
+#define MT7530_TRGMII_RCK_RTT		0x7a04
+#define GSW_TRGMII_RCK_RTT		0x304
+#define  DQS1_GATE			BIT(31)
+#define  DQS0_GATE			BIT(30)
+
+#define MT7530_TRGMII_RD(x)		(0x7a10 + (x) * 8)
+#define GSW_TRGMII_RD(x)		(0x310 + (x) * 8)
+#define  BSLIP_EN			BIT(31)
+#define  EDGE_CHK			BIT(30)
+#define  RD_TAP_MASK			0x7f
+#define  RD_TAP(x)			((x) & 0x7f)
+
+#define GSW_TRGMII_TXCTRL		0x340
+#define MT7530_TRGMII_TXCTRL		0x7a40
+#define  TRAIN_TXEN			BIT(31)
+#define  TXC_INV			BIT(30)
+#define  TX_RST				BIT(28)
+
+#define MT7530_TRGMII_TD_ODT(i)		(0x7a54 + 8 * (i))
+#define GSW_TRGMII_TD_ODT(i)		(0x354 + 8 * (i))
+#define  TD_DM_DRVP(x)			((x) & 0xf)
+#define  TD_DM_DRVN(x)			(((x) & 0xf) << 4)
+
+#define GSW_INTF_MODE			0x390
+#define  INTF_MODE_TRGMII		BIT(1)
+
+#define MT7530_TRGMII_TCK_CTRL		0x7a78
+#define  TCK_TAP(x)			(((x) & 0xf) << 8)
+
+#define MT7530_P5RGMIIRXCR		0x7b00
+#define  CSR_RGMII_EDGE_ALIGN		BIT(8)
+#define  CSR_RGMII_RXC_0DEG_CFG(x)	((x) & 0xf)
+
+#define MT7530_P5RGMIITXCR		0x7b04
+#define  CSR_RGMII_TXC_CFG(x)		((x) & 0x1f)
+
+#define MT7530_CREV			0x7ffc
+#define  CHIP_NAME_SHIFT		16
+#define  MT7530_ID			0x7530
+
+/* Registers for core PLL access through mmd indirect */
+#define CORE_PLL_GROUP2			0x401
+#define  RG_SYSPLL_EN_NORMAL		BIT(15)
+#define  RG_SYSPLL_VODEN		BIT(14)
+#define  RG_SYSPLL_LF			BIT(13)
+#define  RG_SYSPLL_RST_DLY(x)		(((x) & 0x3) << 12)
+#define  RG_SYSPLL_LVROD_EN		BIT(10)
+#define  RG_SYSPLL_PREDIV(x)		(((x) & 0x3) << 8)
+#define  RG_SYSPLL_POSDIV(x)		(((x) & 0x3) << 5)
+#define  RG_SYSPLL_FBKSEL		BIT(4)
+#define  RT_SYSPLL_EN_AFE_OLT		BIT(0)
+
+#define CORE_PLL_GROUP4			0x403
+#define  RG_SYSPLL_DDSFBK_EN		BIT(12)
+#define  RG_SYSPLL_BIAS_EN		BIT(11)
+#define  RG_SYSPLL_BIAS_LPF_EN		BIT(10)
+
+#define CORE_PLL_GROUP5			0x404
+#define  RG_LCDDS_PCW_NCPO1(x)		((x) & 0xffff)
+
+#define CORE_PLL_GROUP6			0x405
+#define  RG_LCDDS_PCW_NCPO0(x)		((x) & 0xffff)
+
+#define CORE_PLL_GROUP7			0x406
+#define  RG_LCDDS_PWDB			BIT(15)
+#define  RG_LCDDS_ISO_EN		BIT(13)
+#define  RG_LCCDS_C(x)			(((x) & 0x7) << 4)
+#define  RG_LCDDS_PCW_NCPO_CHG		BIT(3)
+
+#define CORE_PLL_GROUP10		0x409
+#define  RG_LCDDS_SSC_DELTA(x)		((x) & 0xfff)
+
+#define CORE_PLL_GROUP11		0x40a
+#define  RG_LCDDS_SSC_DELTA1(x)		((x) & 0xfff)
+
+#define CORE_GSWPLL_GRP1		0x40d
+#define  RG_GSWPLL_PREDIV(x)		(((x) & 0x3) << 14)
+#define  RG_GSWPLL_POSDIV_200M(x)	(((x) & 0x3) << 12)
+#define  RG_GSWPLL_EN_PRE		BIT(11)
+#define  RG_GSWPLL_FBKSEL		BIT(10)
+#define  RG_GSWPLL_BP			BIT(9)
+#define  RG_GSWPLL_BR			BIT(8)
+#define  RG_GSWPLL_FBKDIV_200M(x)	((x) & 0xff)
+
+#define CORE_GSWPLL_GRP2		0x40e
+#define  RG_GSWPLL_POSDIV_500M(x)	(((x) & 0x3) << 8)
+#define  RG_GSWPLL_FBKDIV_500M(x)	((x) & 0xff)
+
+#define CORE_TRGMII_GSW_CLK_CG		0x410
+#define  REG_GSWCK_EN			BIT(0)
+#define  REG_TRGMIICK_EN		BIT(1)
+
+#define wait_condition_timeout(condition, timeout)			\
+({									\
+	long __ret = 0;							\
+	unsigned long toj;						\
+	toj = jiffies + msecs_to_jiffies(timeout);			\
+									\
+	for ( ; !(condition) && !time_after_eq(jiffies, toj) ; )	\
+		cond_resched();						\
+									\
+	if (time_after_eq(jiffies, toj))				\
+		__ret = -ETIMEDOUT;					\
+	__ret;								\
+})
+
+#define MIB_DESC(_s, _o, _n)	\
+	{			\
+		.size = (_s),	\
+		.offset = (_o),	\
+		.name = (_n),	\
+	}
+
+struct mt7530_mib_desc {
+	unsigned int size;
+	unsigned int offset;
+	const char *name;
+};
+
+struct mt7530_fdb {
+	u16 vid;
+	u8 port_mask;
+	u8 aging;
+	u8 mac[6];
+	bool noarp;
+};
+
+/* struct mt7530_priv -	This is the main datasructure for holding the state
+ *			of the driver
+ * @dev:		The device pointer
+ * @ds:			The pointer to the dsa core structure
+ * @bus:		The bus used for the device and built-in PHY
+ * @ethsys:		The regmap used for enabling the necessary PLL
+ * @ethernet:		The regmap used for access TRGMII-based registers
+ * @core_pwr:		The power supplied into the core
+ * @io_pwr:		The power supplied into the I/O
+ * @mcm:		Flag for distinguishing if standalone IC or module
+ *			coupling
+ * @reset:		The descriptor for GPIO line tied to its reset pin
+ * @phy_mode:		The xMII for cpu port used
+ * @ports:		Holding the state amongs ports
+ * @reg_mutex:		The lock for protecting among process accessing
+ *			registers
+ */
+struct mt7530_priv {
+	struct device		*dev;
+	struct dsa_switch	*ds;
+	struct mii_bus		*bus;
+	struct reset_control	*rstc;
+	struct regmap		*ethernet;
+	struct regulator	*core_pwr;
+	struct regulator	*io_pwr;
+	bool			mcm;
+	struct gpio_desc	*reset;
+	int phy_mode;
+
+	/* protect among processes for registers access*/
+	struct mutex reg_mutex;
+};
+
+struct mt7530_hw_stats {
+	const char	*string;
+	u16		reg;
+	u8		sizeof_stat;
+};
+
+#endif /* __MT7530_H */
-- 
1.9.1

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Andrew Lunn]

> +static int
> +core_read_mmd_indirect(struct mt7530_priv *priv, int prtad, int devad)
> +{
> +	struct mii_bus *bus = priv->bus;
> +	int value, ret;
> +
> +	/* Write the desired MMD Devad */
> +	ret = bus->write(bus, 0, MII_MMD_CTRL, devad);
> +	if (ret < 0)
> +		goto err;
> +
> +	/* Write the desired MMD register address */
> +	ret = bus->write(bus, 0, MII_MMD_DATA, prtad);
> +	if (ret < 0)
> +		goto err;
> +
> +	/* Select the Function : DATA with no post increment */
> +	ret = bus->write(bus, 0, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
> +	if (ret < 0)
> +		goto err;

Hi Sean

This appear to be a copy of mmd_phy_indirect(). There are patches from
Russell King which made this a public function. Once Russell patches
make net-next, it would be nice to use mmd_phy_indirect(). But that
might be as a follow up patch, rather than now, depending on the order
of acceptance of the patches.

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Florian Fainelli]

On 03/21/2017 02:35 AM, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> 
> MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
> Mediatek router platforms such as MT7623A or MT7623N platform which
> includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
> Among these ports, The port from 0 to 4 are the user ports connecting
> with the remote devices while the port 5 and 6 are the CPU ports
> connecting into Mediatek Ethernet GMAC.
> 
> For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
> through either the TRGMII or RGMII which could be controlled by phy-mode
> in the dt-bindings to specify which mode is preferred to use. And for
> port 5, only RGMII can be specified. However, currently, only port 6 is
> being supported in this DSA driver.
> 
> The driver is made with the reference to qca8k and other existing DSA
> driver. The most of the essential callbacks of the DSA are already
> support in the driver, including tag insert for user port distinguishing,
> port control, bridge offloading, STP setup and ethtool operation to allow
> DSA to model each user port into a standalone netdevice as the other DSA
> driver had done.

Overall, this looks pretty nice and clean, a few comments below

> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---

> +static void
> +mt7530_fdb_read(struct mt7530_priv *priv, struct mt7530_fdb *fdb)
> +{
> +	u32 reg[3];
> +	int i;
> +
> +	/* Read from ARL table into an array */
> +	for (i = 0; i < 3; i++) {
> +		reg[i] = mt7530_read(priv, MT7530_TSRA1 + (i * 4));
> +
> +		dev_dbg(priv->dev, "%s(%d) reg[%d]=0x%x\n",
> +			__func__, __LINE__, i, reg[i]);
> +	}
> +
> +	/* vid - 11:0 on reg[1] */
> +	fdb->vid = (reg[1] >> 0) & 0xfff;
> +	/* aging - 31:24 on reg[2] */
> +	fdb->aging = (reg[2] >> 24) & 0xff;
> +	/* portmask - 11:4 on reg[2] */
> +	fdb->port_mask = (reg[2] >> 4) & 0xff;
> +	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> +	fdb->mac[0] = (reg[0] >> 24) & 0xff;
> +	fdb->mac[1] = (reg[0] >> 16) & 0xff;
> +	fdb->mac[2] = (reg[0] >>  8) & 0xff;
> +	fdb->mac[3] = (reg[0] >>  0) & 0xff;
> +	fdb->mac[4] = (reg[1] >> 24) & 0xff;
> +	fdb->mac[5] = (reg[1] >> 16) & 0xff;
> +	/* noarp - 3:2 on reg[2] */
> +	fdb->noarp = ((reg[2] >> 2) & 0x3) == STATIC_ENT;

Could you add some definitions for the bits and masks that you are
shifting here?

> +}
> +
> +static void
> +mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> +		 u8 port_mask, const u8 *mac,
> +		 u8 aging, u8 type)
> +{
> +	u32 reg[3] = { 0 };
> +	int i;
> +
> +	/* vid - 11:0 on reg[1] */
> +	reg[1] |= (vid & 0xfff) << 0;
> +	/* aging - 31:25 on reg[2] */
> +	reg[2] |= (aging & 0xff) << 24;
> +	/* portmask - 11:4 on reg[2] */
> +	reg[2] |= (port_mask & 0xff) << 4;
> +	/* type - 3 indicate that entry is static wouldn't
> +	 * be aged out and 0 specified as erasing an entry
> +	 */
> +	reg[2] |= (type & 0x3) << 2;
> +	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> +	reg[1] |= mac[5] << 16;
> +	reg[1] |= mac[4] << 24;
> +	reg[0] |= mac[3] << 0;
> +	reg[0] |= mac[2] << 8;
> +	reg[0] |= mac[1] << 16;
> +	reg[0] |= mac[0] << 24;
> +
> +	/* Wrirte array into the ARL table */
> +	for (i = 0; i < 3; i++)
> +		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
> +}

Same here.

> +
> +static int
> +mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 ncpo1, ssc_delta, trgint, i;
> +
> +	switch (mode) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		trgint = 0;
> +		ncpo1 = 0x0c80;
> +		ssc_delta = 0x87;
> +		break;
> +	case PHY_INTERFACE_MODE_TRGMII:
> +		trgint = 1;
> +		ncpo1 = 0x1400;
> +		ssc_delta = 0x57;
> +		break;
> +	default:
> +		pr_err("xMII mode %d not supported\n", mode);
> +		return -EINVAL;
> +	}

You may be able to move this to an adjust_link callback that the PHY
library would call when the PHY gets setup and the port is finally used,
as opposed to doing this upfront during driver initialization.


> +mt7530_setup(struct dsa_switch *ds)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	int ret, i, phy_mode;
> +	u8  cpup_mask = 0;
> +	u32 id, val;
> +	struct regmap *regmap;
> +	struct device_node *dn;
> +
> +	/* Make sure that cpu port specfied on the dt is appropriate */
> +	if (!dsa_is_cpu_port(ds, MT7530_CPU_PORT)) {
> +		dev_err(priv->dev, "port not matched with the CPU port\n");
> +		return -EINVAL;
> +	}

This is kind of a hard error, in that case, a sensible thing to do could
be issue a warning to the user telling that the configuration does not
permit the use of Mediatek tags. Your get_tag_protocol() function could
then return DSA_TAG_PROTO_NONE here instead of DSA_TAG_PROTO_MTK. It
would still allow an user to utilize the switch, and we would know what
is wrong with the configuration/board setup though.


> +
> +	/* The parent node of master_netdev which holds the common system
> +	 * controller also is the container for two GMACs nodes representing
> +	 * as two netdev instances.
> +	 */
> +	dn = ds->master_netdev->dev.of_node->parent;
> +	priv->ethernet = syscon_node_to_regmap(dn);
> +	if (IS_ERR(priv->ethernet))
> +		return PTR_ERR(priv->ethernet);
> +
> +	regmap = devm_regmap_init(ds->dev, NULL, priv,
> +				  &mt7530_regmap_config);
> +	if (IS_ERR(regmap))
> +		dev_warn(priv->dev, "phy regmap initialization failed");
> +
> +	phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> +	if (phy_mode < 0) {
> +		dev_err(priv->dev, "Can't find phy-mode for master device\n");
> +		return phy_mode;
> +	}
> +	dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);

An adjust_link() callback which has a proper PHY device structure would
be more appropriate to look up the phy_mode rather than doing this here.

[snip]

> +	mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> +
> +	/* Fabric setup for the cpu port */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)

Are not you missing an opening parenthesis here in the for() statement?

> +		if (dsa_is_cpu_port(ds, i)) {
> +			/* Enable Mediatek header mode on the cpu port */
> +			mt7530_write(priv, MT7530_PVC_P(i),
> +				     PORT_SPEC_TAG);
> +
> +			/* Setup the MAC by default for the cpu port */
> +			mt7530_write(priv, MT7530_PMCR_P(i), PMCR_CPUP_LINK);
> +
> +			/* Disable auto learning on the cpu port */
> +			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
> +
> +			/* Unknown unicast frame fordwarding to the cpu port */
> +			mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(i)));
> +
> +			/* CPU port gets connected to all user ports of
> +			 * the switch
> +			 */
> +			mt7530_write(priv, MT7530_PCR_P(i),
> +				     PCR_MATRIX(ds->enabled_port_mask));
> +
> +			cpup_mask |= BIT(i);
> +		}
> +
> +	/* Fabric setup for the all user ports */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> +		if (ds->enabled_port_mask & BIT(i)) {
> +			/* Setup the MAC by default for all user ports */
> +			mt7530_write(priv, MT7530_PMCR_P(i),
> +				     PMCR_USERP_LINK);
> +
> +			/* The user port gets connected to the cpu port only */
> +			mt7530_write(priv, MT7530_PCR_P(i),
> +				     PCR_MATRIX(cpup_mask));
> +		}

This should be moved to the port_enable() function.

[snip]

> +#define wait_condition_timeout(condition, timeout)			\
> +({									\
> +	long __ret = 0;							\
> +	unsigned long toj;						\
> +	toj = jiffies + msecs_to_jiffies(timeout);			\
> +									\
> +	for ( ; !(condition) && !time_after_eq(jiffies, toj) ; )	\
> +		cond_resched();						\
> +									\
> +	if (time_after_eq(jiffies, toj))				\
> +		__ret = -ETIMEDOUT;					\
> +	__ret;								\
> +})

Can you use read*_poll_timeout() instead of this?


> +/* struct mt7530_priv -	This is the main datasructure for holding the state
> + *			of the driver
> + * @dev:		The device pointer
> + * @ds:			The pointer to the dsa core structure
> + * @bus:		The bus used for the device and built-in PHY
> + * @ethsys:		The regmap used for enabling the necessary PLL
> + * @ethernet:		The regmap used for access TRGMII-based registers
> + * @core_pwr:		The power supplied into the core
> + * @io_pwr:		The power supplied into the I/O
> + * @mcm:		Flag for distinguishing if standalone IC or module
> + *			coupling
> + * @reset:		The descriptor for GPIO line tied to its reset pin
> + * @phy_mode:		The xMII for cpu port used
> + * @ports:		Holding the state amongs ports
> + * @reg_mutex:		The lock for protecting among process accessing
> + *			registers
> + */

Kudos for using kernel doc here.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Sean Wang]

Hi Florian,

Thank for taking your time on reviewing. Add comment as inline.

On Wed, 2017-03-22 at 11:39 -0700, Florian Fainelli wrote:
> On 03/21/2017 02:35 AM, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > 
> > MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
> > Mediatek router platforms such as MT7623A or MT7623N platform which
> > includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
> > Among these ports, The port from 0 to 4 are the user ports connecting
> > with the remote devices while the port 5 and 6 are the CPU ports
> > connecting into Mediatek Ethernet GMAC.
> > 
> > For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
> > through either the TRGMII or RGMII which could be controlled by phy-mode
> > in the dt-bindings to specify which mode is preferred to use. And for
> > port 5, only RGMII can be specified. However, currently, only port 6 is
> > being supported in this DSA driver.
> > 
> > The driver is made with the reference to qca8k and other existing DSA
> > driver. The most of the essential callbacks of the DSA are already
> > support in the driver, including tag insert for user port distinguishing,
> > port control, bridge offloading, STP setup and ethtool operation to allow
> > DSA to model each user port into a standalone netdevice as the other DSA
> > driver had done.
> 
> Overall, this looks pretty nice and clean, a few comments below
> 
> > 
> > Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Landen Chao <Landen.Chao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> 
> > +static void
> > +mt7530_fdb_read(struct mt7530_priv *priv, struct mt7530_fdb *fdb)
> > +{
> > +	u32 reg[3];
> > +	int i;
> > +
> > +	/* Read from ARL table into an array */
> > +	for (i = 0; i < 3; i++) {
> > +		reg[i] = mt7530_read(priv, MT7530_TSRA1 + (i * 4));
> > +
> > +		dev_dbg(priv->dev, "%s(%d) reg[%d]=0x%x\n",
> > +			__func__, __LINE__, i, reg[i]);
> > +	}
> > +
> > +	/* vid - 11:0 on reg[1] */
> > +	fdb->vid = (reg[1] >> 0) & 0xfff;
> > +	/* aging - 31:24 on reg[2] */
> > +	fdb->aging = (reg[2] >> 24) & 0xff;
> > +	/* portmask - 11:4 on reg[2] */
> > +	fdb->port_mask = (reg[2] >> 4) & 0xff;
> > +	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> > +	fdb->mac[0] = (reg[0] >> 24) & 0xff;
> > +	fdb->mac[1] = (reg[0] >> 16) & 0xff;
> > +	fdb->mac[2] = (reg[0] >>  8) & 0xff;
> > +	fdb->mac[3] = (reg[0] >>  0) & 0xff;
> > +	fdb->mac[4] = (reg[1] >> 24) & 0xff;
> > +	fdb->mac[5] = (reg[1] >> 16) & 0xff;
> > +	/* noarp - 3:2 on reg[2] */
> > +	fdb->noarp = ((reg[2] >> 2) & 0x3) == STATIC_ENT;
> 
> Could you add some definitions for the bits and masks that you are
> shifting here?
> 

Okay, I'll make into proper macro for readability  

> > +}
> > +
> > +static void
> > +mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> > +		 u8 port_mask, const u8 *mac,
> > +		 u8 aging, u8 type)
> > +{
> > +	u32 reg[3] = { 0 };
> > +	int i;
> > +
> > +	/* vid - 11:0 on reg[1] */
> > +	reg[1] |= (vid & 0xfff) << 0;
> > +	/* aging - 31:25 on reg[2] */
> > +	reg[2] |= (aging & 0xff) << 24;
> > +	/* portmask - 11:4 on reg[2] */
> > +	reg[2] |= (port_mask & 0xff) << 4;
> > +	/* type - 3 indicate that entry is static wouldn't
> > +	 * be aged out and 0 specified as erasing an entry
> > +	 */
> > +	reg[2] |= (type & 0x3) << 2;
> > +	/* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> > +	reg[1] |= mac[5] << 16;
> > +	reg[1] |= mac[4] << 24;
> > +	reg[0] |= mac[3] << 0;
> > +	reg[0] |= mac[2] << 8;
> > +	reg[0] |= mac[1] << 16;
> > +	reg[0] |= mac[0] << 24;
> > +
> > +	/* Wrirte array into the ARL table */
> > +	for (i = 0; i < 3; i++)
> > +		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
> > +}
> 
> Same here.
> 

As above. I will improve them.


> > +
> > +static int
> > +mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> > +{
> > +	struct mt7530_priv *priv = ds->priv;
> > +	u32 ncpo1, ssc_delta, trgint, i;
> > +
> > +	switch (mode) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +		trgint = 0;
> > +		ncpo1 = 0x0c80;
> > +		ssc_delta = 0x87;
> > +		break;
> > +	case PHY_INTERFACE_MODE_TRGMII:
> > +		trgint = 1;
> > +		ncpo1 = 0x1400;
> > +		ssc_delta = 0x57;
> > +		break;
> > +	default:
> > +		pr_err("xMII mode %d not supported\n", mode);
> > +		return -EINVAL;
> > +	}
> 
> You may be able to move this to an adjust_link callback that the PHY
> library would call when the PHY gets setup and the port is finally used,
> as opposed to doing this upfront during driver initialization.
> 


Good point. i will follow up


> 
> > +mt7530_setup(struct dsa_switch *ds)
> > +{
> > +	struct mt7530_priv *priv = ds->priv;
> > +	int ret, i, phy_mode;
> > +	u8  cpup_mask = 0;
> > +	u32 id, val;
> > +	struct regmap *regmap;
> > +	struct device_node *dn;
> > +
> > +	/* Make sure that cpu port specfied on the dt is appropriate */
> > +	if (!dsa_is_cpu_port(ds, MT7530_CPU_PORT)) {
> > +		dev_err(priv->dev, "port not matched with the CPU port\n");
> > +		return -EINVAL;
> > +	}
> 
> This is kind of a hard error, in that case, a sensible thing to do could
> be issue a warning to the user telling that the configuration does not
> permit the use of Mediatek tags. Your get_tag_protocol() function could
> then return DSA_TAG_PROTO_NONE here instead of DSA_TAG_PROTO_MTK. It
> would still allow an user to utilize the switch, and we would know what
> is wrong with the configuration/board setup though.
> 

I seems making a hard requirement.

I will improve it with showing warning message if the number of cpu port
is not right.

It at least allows the other user ports can work in case cpu port
doesn't work.


> > +
> > +	/* The parent node of master_netdev which holds the common system
> > +	 * controller also is the container for two GMACs nodes representing
> > +	 * as two netdev instances.
> > +	 */
> > +	dn = ds->master_netdev->dev.of_node->parent;
> > +	priv->ethernet = syscon_node_to_regmap(dn);
> > +	if (IS_ERR(priv->ethernet))
> > +		return PTR_ERR(priv->ethernet);
> > +
> > +	regmap = devm_regmap_init(ds->dev, NULL, priv,
> > +				  &mt7530_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		dev_warn(priv->dev, "phy regmap initialization failed");
> > +
> > +	phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> > +	if (phy_mode < 0) {
> > +		dev_err(priv->dev, "Can't find phy-mode for master device\n");
> > +		return phy_mode;
> > +	}
> > +	dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);
> 
> An adjust_link() callback which has a proper PHY device structure would
> be more appropriate to look up the phy_mode rather than doing this here.
> 

As above. I will follow up 

> [snip]
> 
> > +	mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> > +
> > +	/* Fabric setup for the cpu port */
> > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> 
> Are not you missing an opening parenthesis here in the for() statement?
> 

It is missing because the following is single statement. 
I admits it is better to add the parenthesis for readability, so I will
add it.

> > +		if (dsa_is_cpu_port(ds, i)) {
> > +			/* Enable Mediatek header mode on the cpu port */
> > +			mt7530_write(priv, MT7530_PVC_P(i),
> > +				     PORT_SPEC_TAG);
> > +
> > +			/* Setup the MAC by default for the cpu port */
> > +			mt7530_write(priv, MT7530_PMCR_P(i), PMCR_CPUP_LINK);
> > +
> > +			/* Disable auto learning on the cpu port */
> > +			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
> > +
> > +			/* Unknown unicast frame fordwarding to the cpu port */
> > +			mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(i)));
> > +
> > +			/* CPU port gets connected to all user ports of
> > +			 * the switch
> > +			 */
> > +			mt7530_write(priv, MT7530_PCR_P(i),
> > +				     PCR_MATRIX(ds->enabled_port_mask));
> > +
> > +			cpup_mask |= BIT(i);
> > +		}
> > +
> > +	/* Fabric setup for the all user ports */
> > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > +		if (ds->enabled_port_mask & BIT(i)) {
> > +			/* Setup the MAC by default for all user ports */
> > +			mt7530_write(priv, MT7530_PMCR_P(i),
> > +				     PMCR_USERP_LINK);
> > +
> > +			/* The user port gets connected to the cpu port only */
> > +			mt7530_write(priv, MT7530_PCR_P(i),
> > +				     PCR_MATRIX(cpup_mask));
> > +		}
> 
> This should be moved to the port_enable() function.
> 

okay. i will follow up.

> [snip]
*_poll_timeout
> > +#define wait_condition_timeout(condition, timeout)			\
> > +({									\
> > +	long __ret = 0;							\
> > +	unsigned long toj;						\
> > +	toj = jiffies + msecs_to_jiffies(timeout);			\
> > +									\
> > +	for ( ; !(condition) && !time_after_eq(jiffies, toj) ; )	\
> > +		cond_resched();						\
> > +									\
> > +	if (time_after_eq(jiffies, toj))				\
> > +		__ret = -ETIMEDOUT;					\
> > +	__ret;								\
> > +})
> 
> Can you use read*_poll_timeout() instead of this?
> 

I will try to reuse them

the function pointer for op in *_poll_timeout seems only accepting
only single argument for address. which implies I need creating another
function for mt7530_readX() used by *_poll_timeout and keeping another
static pointer to mt7530 internal for mt7530_readX() to work.


> > +/* struct mt7530_priv -	This is the main datasructure for holding the state
> > + *			of the driver
> > + * @dev:		The device pointer
> > + * @ds:			The pointer to the dsa core structure
> > + * @bus:		The bus used for the device and built-in PHY
> > + * @ethsys:		The regmap used for enabling the necessary PLL
> > + * @ethernet:		The regmap used for access TRGMII-based registers
> > + * @core_pwr:		The power supplied into the core
> > + * @io_pwr:		The power supplied into the I/O
> > + * @mcm:		Flag for distinguishing if standalone IC or module
> > + *			coupling
> > + * @reset:		The descriptor for GPIO line tied to its reset pin
> > + * @phy_mode:		The xMII for cpu port used
> > + * @ports:		Holding the state amongs ports
> > + * @reg_mutex:		The lock for protecting among process accessing
> > + *			registers
> > + */
> 
> Kudos for using kernel doc here.

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Andrew Lunn]

> +static int
> +mt7623_trgmii_write(struct mt7530_priv *priv,  u32 reg, u32 val)
> +{
> +	int ret;
> +
> +	ret =  regmap_write(priv->ethernet, TRGMII_BASE(reg), val);
> +	if (ret < 0)
> +		dev_err(priv->dev,
> +			"failed to priv write register\n");
> +	return ret;
> +}
> +
> +static u32
> +mt7623_trgmii_read(struct mt7530_priv *priv, u32 reg)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(priv->ethernet, TRGMII_BASE(reg), &val);
> +	if (ret < 0) {
> +		dev_err(priv->dev,
> +			"failed to priv read register\n");
> +		return ret;
> +	}
> +
> +	return val;
> +}

Hi Sean

These appear to be the only two accessors which use the regmap.

> +static int
> +mt7530_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> +{
> +	struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
> +
> +	/* BIT(15) is used as indication for pseudo registers
> +	 * which would be translated into the general MDIO
> +	 * access to leverage the unique regmap sys interface.
> +	 */
> +	if (reg & BIT(15))
> +		*val = mdiobus_read_nested(priv->bus,
> +					   (reg & 0xf00) >> 8,
> +					   (reg & 0xff) >> 2);
> +	else
> +		*val = mt7530_read(priv, reg);
> +
> +	return 0;
> +}

.....

> +static const struct regmap_range mt7530_readable_ranges[] = {
> +	regmap_reg_range(0x0000, 0x00ac), /* Global control */
> +	regmap_reg_range(0x2000, 0x202c), /* Port Control - P0 */
> +	regmap_reg_range(0x2100, 0x212c), /* Port Control - P1 */
> +	regmap_reg_range(0x2200, 0x222c), /* Port Control - P2 */
> +	regmap_reg_range(0x2300, 0x232c), /* Port Control - P3 */
> +	regmap_reg_range(0x2400, 0x242c), /* Port Control - P4 */
> +	regmap_reg_range(0x2500, 0x252c), /* Port Control - P5 */
> +	regmap_reg_range(0x2600, 0x262c), /* Port Control - P6 */
> +	regmap_reg_range(0x30e0, 0x30f8), /* Port MAC - SYS */
> +	regmap_reg_range(0x3000, 0x3014), /* Port MAC - P0 */
> +	regmap_reg_range(0x3100, 0x3114), /* Port MAC - P1 */
> +	regmap_reg_range(0x3200, 0x3214), /* Port MAC - P2*/
> +	regmap_reg_range(0x3300, 0x3314), /* Port MAC - P3*/
> +	regmap_reg_range(0x3400, 0x3414), /* Port MAC - P4 */
> +	regmap_reg_range(0x3500, 0x3514), /* Port MAC - P5 */
> +	regmap_reg_range(0x3600, 0x3614), /* Port MAC - P6 */
> +	regmap_reg_range(0x4000, 0x40d4), /* MIB - P0 */
> +	regmap_reg_range(0x4100, 0x41d4), /* MIB - P1 */
> +	regmap_reg_range(0x4200, 0x42d4), /* MIB - P2 */
> +	regmap_reg_range(0x4300, 0x43d4), /* MIB - P3 */
> +	regmap_reg_range(0x4400, 0x44d4), /* MIB - P4 */
> +	regmap_reg_range(0x4500, 0x45d4), /* MIB - P5 */
> +	regmap_reg_range(0x4600, 0x46d4), /* MIB - P6 */
> +	regmap_reg_range(0x4fe0, 0x4ff4), /* SYS */
> +	regmap_reg_range(0x7000, 0x700c), /* SYS 2 */
> +	regmap_reg_range(0x7018, 0x7028), /* SYS 3 */
> +	regmap_reg_range(0x7800, 0x7830), /* SYS 4 */
> +	regmap_reg_range(0x7a00, 0x7a7c), /* TRGMII */
> +	regmap_reg_range(0x8000, 0x8078), /* Psedo address for Phy - P0 */
> +	regmap_reg_range(0x8100, 0x8178), /* Psedo address for Phy - P1 */
> +	regmap_reg_range(0x8200, 0x8278), /* Psedo address for Phy - P2 */
> +	regmap_reg_range(0x8300, 0x8378), /* Psedo address for Phy - P3 */
> +	regmap_reg_range(0x8400, 0x8478), /* Psedo address for Phy - P4 */
> +};

It looks like your regmap accessor are only used for 0x7a00 to 0x7a7c.

It is not clear why you even bother with a regmap. If you have it, why
not use it for all registers within the regmap?

    Andrew

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Sean Wang]

Hi Andrew,

The purpose for the regmap table registered is to 

provide a way which helps us to look up a specific 

register on the switch through regmap-debugfs.


And not all ranges of register is defined

so I only include the meaningful ones in a sparse way 

for the table.

	Sean


On Thu, 2017-03-23 at 08:22 +0100, Andrew Lunn wrote:
> > +static int
> > +mt7623_trgmii_write(struct mt7530_priv *priv,  u32 reg, u32 val)
> > +{
> > +	int ret;
> > +
> > +	ret =  regmap_write(priv->ethernet, TRGMII_BASE(reg), val);
> > +	if (ret < 0)
> > +		dev_err(priv->dev,
> > +			"failed to priv write register\n");
> > +	return ret;
> > +}
> > +
> > +static u32
> > +mt7623_trgmii_read(struct mt7530_priv *priv, u32 reg)
> > +{
> > +	int ret;
> > +	u32 val;
> > +
> > +	ret = regmap_read(priv->ethernet, TRGMII_BASE(reg), &val);
> > +	if (ret < 0) {
> > +		dev_err(priv->dev,
> > +			"failed to priv read register\n");
> > +		return ret;
> > +	}
> > +
> > +	return val;
> > +}
> 
> Hi Sean
> 
> These appear to be the only two accessors which use the regmap.
> 
> > +static int
> > +mt7530_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > +{
> > +	struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
> > +
> > +	/* BIT(15) is used as indication for pseudo registers
> > +	 * which would be translated into the general MDIO
> > +	 * access to leverage the unique regmap sys interface.
> > +	 */
> > +	if (reg & BIT(15))
> > +		*val = mdiobus_read_nested(priv->bus,
> > +					   (reg & 0xf00) >> 8,
> > +					   (reg & 0xff) >> 2);
> > +	else
> > +		*val = mt7530_read(priv, reg);
> > +
> > +	return 0;
> > +}
> 
> .....
> 
> > +static const struct regmap_range mt7530_readable_ranges[] = {
> > +	regmap_reg_range(0x0000, 0x00ac), /* Global control */
> > +	regmap_reg_range(0x2000, 0x202c), /* Port Control - P0 */
> > +	regmap_reg_range(0x2100, 0x212c), /* Port Control - P1 */
> > +	regmap_reg_range(0x2200, 0x222c), /* Port Control - P2 */
> > +	regmap_reg_range(0x2300, 0x232c), /* Port Control - P3 */
> > +	regmap_reg_range(0x2400, 0x242c), /* Port Control - P4 */
> > +	regmap_reg_range(0x2500, 0x252c), /* Port Control - P5 */
> > +	regmap_reg_range(0x2600, 0x262c), /* Port Control - P6 */
> > +	regmap_reg_range(0x30e0, 0x30f8), /* Port MAC - SYS */
> > +	regmap_reg_range(0x3000, 0x3014), /* Port MAC - P0 */
> > +	regmap_reg_range(0x3100, 0x3114), /* Port MAC - P1 */
> > +	regmap_reg_range(0x3200, 0x3214), /* Port MAC - P2*/
> > +	regmap_reg_range(0x3300, 0x3314), /* Port MAC - P3*/
> > +	regmap_reg_range(0x3400, 0x3414), /* Port MAC - P4 */
> > +	regmap_reg_range(0x3500, 0x3514), /* Port MAC - P5 */
> > +	regmap_reg_range(0x3600, 0x3614), /* Port MAC - P6 */
> > +	regmap_reg_range(0x4000, 0x40d4), /* MIB - P0 */
> > +	regmap_reg_range(0x4100, 0x41d4), /* MIB - P1 */
> > +	regmap_reg_range(0x4200, 0x42d4), /* MIB - P2 */
> > +	regmap_reg_range(0x4300, 0x43d4), /* MIB - P3 */
> > +	regmap_reg_range(0x4400, 0x44d4), /* MIB - P4 */
> > +	regmap_reg_range(0x4500, 0x45d4), /* MIB - P5 */
> > +	regmap_reg_range(0x4600, 0x46d4), /* MIB - P6 */
> > +	regmap_reg_range(0x4fe0, 0x4ff4), /* SYS */
> > +	regmap_reg_range(0x7000, 0x700c), /* SYS 2 */
> > +	regmap_reg_range(0x7018, 0x7028), /* SYS 3 */
> > +	regmap_reg_range(0x7800, 0x7830), /* SYS 4 */
> > +	regmap_reg_range(0x7a00, 0x7a7c), /* TRGMII */
> > +	regmap_reg_range(0x8000, 0x8078), /* Psedo address for Phy - P0 */
> > +	regmap_reg_range(0x8100, 0x8178), /* Psedo address for Phy - P1 */
> > +	regmap_reg_range(0x8200, 0x8278), /* Psedo address for Phy - P2 */
> > +	regmap_reg_range(0x8300, 0x8378), /* Psedo address for Phy - P3 */
> > +	regmap_reg_range(0x8400, 0x8478), /* Psedo address for Phy - P4 */
> > +};
> 
> It looks like your regmap accessor are only used for 0x7a00 to 0x7a7c.
> 
> It is not clear why you even bother with a regmap. If you have it, why
> not use it for all registers within the regmap?
> 
>     Andrew

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 09:06, Sean Wang wrote:
> Hi Andrew,
> 
> The purpose for the regmap table registered is to 
> 
> provide a way which helps us to look up a specific 
> 
> register on the switch through regmap-debugfs.
> 
> 
> And not all ranges of register is defined
> 
> so I only include the meaningful ones in a sparse way 
> 
> for the table.
I think in that case it might be nice to make regmap support optional in
order to avoid pulling in bloat on platforms that don't need it.

- Felix


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[John Crispin]

On 23/03/17 15:09, Felix Fietkau wrote:
> On 2017-03-23 09:06, Sean Wang wrote:
>> Hi Andrew,
>>
>> The purpose for the regmap table registered is to
>>
>> provide a way which helps us to look up a specific
>>
>> register on the switch through regmap-debugfs.
>>
>>
>> And not all ranges of register is defined
>>
>> so I only include the meaningful ones in a sparse way
>>
>> for the table.
> I think in that case it might be nice to make regmap support optional in
> order to avoid pulling in bloat on platforms that don't need it.
>
> - Felix
>
The 2 relevant platforms are mips/ralink and arm/mediatek. both require 
regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth 
driver which is a prereq for mt7530. so regmap cannot be optional here.

     John


>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 15:25, John Crispin wrote:
> 
> 
> On 23/03/17 15:09, Felix Fietkau wrote:
>> On 2017-03-23 09:06, Sean Wang wrote:
>>> Hi Andrew,
>>>
>>> The purpose for the regmap table registered is to
>>>
>>> provide a way which helps us to look up a specific
>>>
>>> register on the switch through regmap-debugfs.
>>>
>>>
>>> And not all ranges of register is defined
>>>
>>> so I only include the meaningful ones in a sparse way
>>>
>>> for the table.
>> I think in that case it might be nice to make regmap support optional in
>> order to avoid pulling in bloat on platforms that don't need it.
>>
>> - Felix
>>
> The 2 relevant platforms are mips/ralink and arm/mediatek. both require 
> regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth 
> driver which is a prereq for mt7530. so regmap cannot be optional here.
Makes sense, thanks.

- Felix

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Andrew Lunn]

On Thu, Mar 23, 2017 at 04:06:56PM +0800, Sean Wang wrote:
> Hi Andrew,
> 
> The purpose for the regmap table registered is to 
> 
> provide a way which helps us to look up a specific 
> 
> register on the switch through regmap-debugfs.
> 
> 
> And not all ranges of register is defined
> 
> so I only include the meaningful ones in a sparse way 
> 
> for the table.

Many of there registers can be dumped using existing tools.  You can
dump the port registers using ethtool -r, if you implement
get_regs/get_regs_len in your driver. mii-tool can dump the PHY
registers.

What you cannot see are global registers, so i can understand the
usage of regmap-debugfs. However, i have a hard time with you only
actually using the regmap for get/set for one small subset of
registers. Either you need to use regmap to get/set all registers, or
you remove it from the mainline driver, and keep it as a private patch
which you use for your development work. For the Marvell driver we
have an out of tree patch which exports a log of information via
debugfs.

	Andrew

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Andrew Lunn]

On Tue, Mar 21, 2017 at 05:35:10PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:

Hi Sean

> +	/* Lower Tx Driving */
> +	for (i = 0 ; i < 6 ; i++)

Could MT7530_CPU_PORT be used here? 

> +		mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
> +			     TD_DM_DRVP(8) | TD_DM_DRVN(8));
> +
> +	/* Setup MT7530 core clock */
> +	if (!trgint) {
> +		/* Disable MT7530 core clock */
> +		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> +
> +		/* Disable MT7530 PLL, since phy_device has not yet been
> +		 * created when this function is called. So we provide
> +		 * core_write_mmd_indirect to complete this function
> +		 */
> +		core_write_mmd_indirect(priv,
> +					CORE_GSWPLL_GRP1,
> +					MDIO_MMD_VEND2,
> +					0);
> +
> +		/* Setup MT7530 core clock into 500Mhz */
> +		core_write(priv, CORE_GSWPLL_GRP2,
> +			   RG_GSWPLL_POSDIV_500M(1) |
> +			   RG_GSWPLL_FBKDIV_500M(25));
> +
> +		/* Enable MT7530 PLL */
> +		core_write(priv, CORE_GSWPLL_GRP1,
> +			   RG_GSWPLL_EN_PRE |
> +			   RG_GSWPLL_POSDIV_200M(2) |
> +			   RG_GSWPLL_FBKDIV_200M(32));
> +
> +		/* Enable MT7530 core clock */
> +		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> +	}
> +
> +	/* Setup the MT7530 TRGMII Tx Clock */
> +	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> +	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> +	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> +	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> +	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> +	core_write(priv, CORE_PLL_GROUP4,
> +		   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
> +		   RG_SYSPLL_BIAS_LPF_EN);
> +	core_write(priv, CORE_PLL_GROUP2,
> +		   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
> +		   RG_SYSPLL_POSDIV(1));
> +	core_write(priv, CORE_PLL_GROUP7,
> +		   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
> +		   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> +	core_set(priv, CORE_TRGMII_GSW_CLK_CG,
> +		 REG_GSWCK_EN | REG_TRGMIICK_EN);
> +
> +	if (!trgint)
> +		for (i = 0 ; i < 5 ; i++)

Why only 5 here? All other similar loops are to 6.  Replacing 5 with a
#define might help make this more readable.

> +			mt7530_rmw(priv, MT7530_TRGMII_RD(i),
> +				   RD_TAP_MASK, RD_TAP(16));
> +	else
> +		mt7623_trgmii_set(priv, GSW_INTF_MODE, INTF_MODE_TRGMII);
> +
> +	return 0;
> +}
> +
> +static int
> +mt7623_pad_clk_setup(struct dsa_switch *ds)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	int i;
> +
> +	for (i = 0 ; i < 6; i++)

MT7530_CPU_PORT?

> +
> +/* Registers to mac forward conrol for unknown frames */

/conrol/control

> +
> +/* Registor for port control */

Register

> +/* Regiser for TOP signal control */

Register

> +/* struct mt7530_priv -	This is the main datasructure for holding the state

data structure

> + *			of the driver
> + * @dev:		The device pointer
> + * @ds:			The pointer to the dsa core structure
> + * @bus:		The bus used for the device and built-in PHY
> + * @ethsys:		The regmap used for enabling the necessary PLL
> + * @ethernet:		The regmap used for access TRGMII-based registers
> + * @core_pwr:		The power supplied into the core
> + * @io_pwr:		The power supplied into the I/O
> + * @mcm:		Flag for distinguishing if standalone IC or module
> + *			coupling
> + * @reset:		The descriptor for GPIO line tied to its reset pin
> + * @phy_mode:		The xMII for cpu port used
> + * @ports:		Holding the state amongs ports

among

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Sean Wang]

Hi Andrew 


On Fri, 2017-03-24 at 15:19 +0100, Andrew Lunn wrote:
> On Tue, Mar 21, 2017 at 05:35:10PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> 
> Hi Sean
> 
> > +	/* Lower Tx Driving */
> > +	for (i = 0 ; i < 6 ; i++)
> 
> Could MT7530_CPU_PORT be used here? 
> 

I should create meaningful definition instead of avoiding hard coding 

where 6 should be corrected into 5 which is meant for the number for

control register to configure TRGMII path


> > +		mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
> > +			     TD_DM_DRVP(8) | TD_DM_DRVN(8));
> > +
> > +	/* Setup MT7530 core clock */
> > +	if (!trgint) {
> > +		/* Disable MT7530 core clock */
> > +		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > +
> > +		/* Disable MT7530 PLL, since phy_device has not yet been
> > +		 * created when this function is called. So we provide
> > +		 * core_write_mmd_indirect to complete this function
> > +		 */
> > +		core_write_mmd_indirect(priv,
> > +					CORE_GSWPLL_GRP1,
> > +					MDIO_MMD_VEND2,
> > +					0);
> > +
> > +		/* Setup MT7530 core clock into 500Mhz */
> > +		core_write(priv, CORE_GSWPLL_GRP2,
> > +			   RG_GSWPLL_POSDIV_500M(1) |
> > +			   RG_GSWPLL_FBKDIV_500M(25));
> > +
> > +		/* Enable MT7530 PLL */
> > +		core_write(priv, CORE_GSWPLL_GRP1,
> > +			   RG_GSWPLL_EN_PRE |
> > +			   RG_GSWPLL_POSDIV_200M(2) |
> > +			   RG_GSWPLL_FBKDIV_200M(32));
> > +
> > +		/* Enable MT7530 core clock */
> > +		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > +	}
> > +
> > +	/* Setup the MT7530 TRGMII Tx Clock */
> > +	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > +	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> > +	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> > +	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> > +	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> > +	core_write(priv, CORE_PLL_GROUP4,
> > +		   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
> > +		   RG_SYSPLL_BIAS_LPF_EN);
> > +	core_write(priv, CORE_PLL_GROUP2,
> > +		   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
> > +		   RG_SYSPLL_POSDIV(1));
> > +	core_write(priv, CORE_PLL_GROUP7,
> > +		   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
> > +		   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> > +	core_set(priv, CORE_TRGMII_GSW_CLK_CG,
> > +		 REG_GSWCK_EN | REG_TRGMIICK_EN);
> > +
> > +	if (!trgint)
> > +		for (i = 0 ; i < 5 ; i++)
> 
> Why only 5 here? All other similar loops are to 6.  Replacing 5 with a
> #define might help make this more readable.

the same as above, it should be corrected

> > +			mt7530_rmw(priv, MT7530_TRGMII_RD(i),
> > +				   RD_TAP_MASK, RD_TAP(16));
> > +	else
> > +		mt7623_trgmii_set(priv, GSW_INTF_MODE, INTF_MODE_TRGMII);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +mt7623_pad_clk_setup(struct dsa_switch *ds)
> > +{
> > +	struct mt7530_priv *priv = ds->priv;
> > +	int i;
> > +
> > +	for (i = 0 ; i < 6; i++)
> 
> MT7530_CPU_PORT?


the same as above , it should be corrected
> > +
> > +/* Registers to mac forward conrol for unknown frames */
> 
> /conrol/control
> 

will fix it up
> > +
> > +/* Registor for port control */
> 
> Register
> 

will fix it up
> > +/* Regiser for TOP signal control */
> 
> Register
> 

will fix it up
> > +/* struct mt7530_priv -	This is the main datasructure for holding the state
> 
> data structure
> 
> > + *			of the driver
> > + * @dev:		The device pointer
> > + * @ds:			The pointer to the dsa core structure
> > + * @bus:		The bus used for the device and built-in PHY
> > + * @ethsys:		The regmap used for enabling the necessary PLL
> > + * @ethernet:		The regmap used for access TRGMII-based registers
> > + * @core_pwr:		The power supplied into the core
> > + * @io_pwr:		The power supplied into the I/O
> > + * @mcm:		Flag for distinguishing if standalone IC or module
> > + *			coupling
> > + * @reset:		The descriptor for GPIO line tied to its reset pin
> > + * @phy_mode:		The xMII for cpu port used
> > + * @ports:		Holding the state amongs ports
> 
> among

will fix it up

thank for your careful reviewing again!!



> 	Andrew


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Andrew Lunn]

Hi Sean

> +	regmap = devm_regmap_init(ds->dev, NULL, priv,
> +				  &mt7530_regmap_config);
> +	if (IS_ERR(regmap))
> +		dev_warn(priv->dev, "phy regmap initialization failed");
> +

Shouldn't this be a fatal error? If you keep going when there is an
error, what happens when you actually try to use the regmap?

> +	phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> +	if (phy_mode < 0) {
> +		dev_err(priv->dev, "Can't find phy-mode for master device\n");
> +		return phy_mode;
> +	}
> +	dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);

dev_dbg? 

> +
> +	id = mt7530_read(priv, MT7530_CREV);
> +	id >>= CHIP_NAME_SHIFT;
> +	if (id != MT7530_ID)
> +		return -ENODEV;

It might be helpful to say what ID has been found, if it is not the
supported ID.

	Andrew

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Sean Wang]

Hi Andrew,

Add comment as below inline


On Fri, 2017-03-24 at 15:02 +0100, Andrew Lunn wrote:
> Hi Sean
> 
> > +	regmap = devm_regmap_init(ds->dev, NULL, priv,
> > +				  &mt7530_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		dev_warn(priv->dev, "phy regmap initialization failed");
> > +
> 
> Shouldn't this be a fatal error? If you keep going when there is an
> error, what happens when you actually try to use the regmap?
> 

Initial thought is that it is just for debugging purpose so if it fails,
it should break any functionality of switch and only have implication as
a warning message. 

Anyway, i will remove it in the next one since it seems better being
kept in private place as you suggested in the previous mail.  

> > +	phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> > +	if (phy_mode < 0) {
> > +		dev_err(priv->dev, "Can't find phy-mode for master device\n");
> > +		return phy_mode;
> > +	}
> > +	dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);
> 
> dev_dbg? 
> 

Sorry. i forgot turned it into dev_dbg

> > +
> > +	id = mt7530_read(priv, MT7530_CREV);
> > +	id >>= CHIP_NAME_SHIFT;
> > +	if (id != MT7530_ID)
> > +		return -ENODEV;
> 
> It might be helpful to say what ID has been found, if it is not the
> supported ID.


I will fix it up to make users know what was going on 

thanks for taking your time on those reviewing again!


> 	Andrew