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

[kbuild test robot]

Hi Sean,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.11-rc2 next-20170310]
[cannot apply to net-next/master net/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/dt-bindings-net-dsa-add-Mediatek-MT7530-binding/20170315-083834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/net/dsa/mt7530.c: In function 'mt7530_probe':
   drivers/net/dsa/mt7530.c:1076:27: warning: unused variable 'mdio' 
[-Wunused-variable]
 struct device_node *dn, *mdio;
  ^~~~
   drivers/net/dsa/mt7530.c: In function 'mt7530_remove':
>> drivers/net/dsa/mt7530.c:1173:9: warning: 'return' with a value, in function 
>> returning void
 return ret;
^~~
   drivers/net/dsa/mt7530.c:1153:1: note: declared here
mt7530_remove(struct mdio_device *mdiodev)
^

vim +/return +1173 drivers/net/dsa/mt7530.c

  1070  };
  1071  
  1072  static int
  1073  mt7530_probe(struct mdio_device *mdiodev)
  1074  {
  1075  struct mt7530_priv *priv;
> 1076  struct device_node *dn, *mdio;
  1077  int ret;
  1078  const char *pm;
  1079  
  1080  dn = mdiodev->dev.of_node;
  1081  
  1082  priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
  1083  if (!priv)
  1084  return -ENOMEM;
  1085  
  1086  priv->ds = devm_kzalloc(>dev, sizeof(*priv->ds), 
GFP_KERNEL);
  1087  if (!priv->ds)
  1088  return -ENOMEM;
  1089  
  1090  /* Use medatek,mcm property to distinguish hardware type that 
would
  1091   * casues a little bit differences on power-on sequence.
  1092   */
  1093  ret = of_property_read_string(dn, "mediatek,mcm", );
  1094  if (!ret && !strcasecmp(pm, "enabled")) {
  1095  priv->mcm = true;
  1096  dev_info(>dev, "MT7530 adapts as multi-chip 
module\n");
  1097  }
  1098  
  1099  priv->core_pwr = devm_regulator_get(>dev, "core");
  1100  if (IS_ERR(priv->core_pwr))
  1101  return PTR_ERR(priv->core_pwr);
  1102  
  1103  priv->io_pwr = devm_regulator_get(>dev, "io");
  1104  if (IS_ERR(priv->io_pwr))
  1105  return PTR_ERR(priv->io_pwr);
  1106  
  1107  /* MT7530 shares the certain address space with Mediatek 
Ethernet
  1108   * driver for controling TRGMII. Here we create syscon regmap 
for
  1109   * access and control these parameters up on TRGMII.
  1110   */
    priv->ethsys = syscon_regmap_lookup_by_phandle(dn,
  1112 
"mediatek,ethsys");
  1113  if (IS_ERR(priv->ethsys))
  1114  return PTR_ERR(priv->ethsys);
  1115  
  1116  priv->ethernet = syscon_regmap_lookup_by_phandle(dn,
  1117 
"mediatek,ethernet");
  1118  if (IS_ERR(priv->ethernet))
  1119  return PTR_ERR(priv->ethernet);
  1120  
  1121  /* Not MCM that indicates switch works as the remote standalone
  1122   * integrated circuit so the GPIO pin would be used to complete
  1123   * the reset, otherwise memory-mapped register accessing used
  1124   * through syscon provides in the case of MCM.
  1125   */
  1126  if (!priv->mcm) {
  1127  priv->reset = of_get_named_gpio(dn, 
"mediatek,reset-pin", 0);
  1128  if (!gpio_is_valid(priv->reset))
  1129  return priv->reset;
  1130  
  1131  ret = devm_gpio_request_one(>dev,
  1132  priv->reset, 
GPIOF_OUT_INIT_LOW,
  1133  "mediatek,reset-pin");
  1134  if (ret < 0) {
  1135  dev_err(>dev,
  1136  "fail to devm_gpio_request reset\n");
  1137  return ret;
  1138  }
  1139  }
  1140  
  1141  priv->bus = mdiodev->bus;
  1142  priv->dev = >dev;
  1143  priv->ds->priv = priv;
  1144  priv->ds->dev = >dev;
  1145  priv->ds->ops = _switch_ops;
  1146  mutex_init(>reg_mutex);
  1147  dev_set_drvdata(>dev, priv);
  1148  
  1149  return dsa_register_switch(priv->ds, priv->ds->dev->of_node);
  1150  }
  1151  
  1152  static void
  1153  mt7530_remove(struct mdio_device *mdiodev)
  1154  {
  1155  struct 

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

[kbuild test robot]

Hi Sean,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.11-rc2 next-20170310]
[cannot apply to net-next/master net/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/dt-bindings-net-dsa-add-Mediatek-MT7530-binding/20170315-083834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from drivers/net/dsa/mt7530.c:25:0:
   drivers/net/dsa/mt7530.c: In function 'mt7530_setup':
>> drivers/net/dsa/mt7530.c:699:19: warning: large integer implicitly truncated 
>> to unsigned type [-Woverflow]
   RESET_MCM, ~RESET_MCM);
  ^
   include/linux/regmap.h:70:42: note: in definition of macro 
'regmap_update_bits'
 regmap_update_bits_base(map, reg, mask, val, NULL, false, false)
 ^~~
   drivers/net/dsa/mt7530.c: In function 'mt7530_probe':
   drivers/net/dsa/mt7530.c:1076:27: warning: unused variable 'mdio' 
[-Wunused-variable]
 struct device_node *dn, *mdio;
  ^~~~
   drivers/net/dsa/mt7530.c: In function 'mt7530_remove':
   drivers/net/dsa/mt7530.c:1173:9: warning: 'return' with a value, in function 
returning void
 return ret;
^~~
   drivers/net/dsa/mt7530.c:1153:1: note: declared here
mt7530_remove(struct mdio_device *mdiodev)
^

vim +699 drivers/net/dsa/mt7530.c

   683  ret = regulator_enable(priv->io_pwr);
   684  if (ret < 0) {
   685  dev_err(priv->dev, "Failed to enable io pwr: %d\n",
   686  ret);
   687  return ret;
   688  }
   689  
   690  /* Reset whole chip through gpio pin or
   691   * memory-mapped registers for different
   692   * type of hardware
   693   */
   694  if (priv->mcm) {
   695  regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
   696 RESET_MCM, RESET_MCM);
   697  usleep_range(1000, 1100);
   698  regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
 > 699 RESET_MCM, ~RESET_MCM);
   700  } else {
   701  gpio_direction_output(priv->reset, 0);
   702  usleep_range(1000, 1100);
   703  gpio_set_value(priv->reset, 1);
   704  }
   705  
   706  /* Wait until the reset completion */
   707  ret = wait_condition_timeout(mt7530_read(priv, MT7530_HWTRAP) 
!= 0,

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

[Sean Wang]

On Tue, 2017-03-14 at 00:11 +0100, Andrew Lunn wrote:
> > +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;
> > +
> > +   /* 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;
> > +   }
> > +
> > +   regmap = devm_regmap_init(ds->dev, NULL, priv,
> > + _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);
> 
> Hi Sean
> 
> It is not documented in the binding that a phy-mode is mandatory for
> the cpu port.
> 
> Andrew

Hi Andrew,

thanks for your reviewing. I'll also add the missing part into the next
one. 
Sean

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

[Andrew Lunn]

> +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;
> +
> + /* 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;
> + }
> +
> + regmap = devm_regmap_init(ds->dev, NULL, priv,
> +   _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);

Hi Sean

It is not documented in the binding that a phy-mode is mandatory for
the cpu port.

Andrew

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

[Andrew Lunn]

Hi Sean

Just looking at the GPIO handling at the moment.

> + /* Reset whole chip through gpio pin or
> +  * memory-mapped registers for different
> +  * type of hardware
> +  */
> + if (priv->mcm) {
> + regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
> +RESET_MCM, RESET_MCM);
> + usleep_range(1000, 1100);
> + regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
> +RESET_MCM, ~RESET_MCM);
> + } else {
> + gpio_direction_output(priv->reset, 0);
> + usleep_range(1000, 1100);
> + gpio_set_value(priv->reset, 1);
> + }



> + /* 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 = of_get_named_gpio(dn, "mediatek,reset-pin", 0);
> + if (!gpio_is_valid(priv->reset))
> + return priv->reset;
> +
> + ret = devm_gpio_request_one(>dev,
> + priv->reset, GPIOF_OUT_INIT_LOW,
> + "mediatek,reset-pin");
> + if (ret < 0) {
> + dev_err(>dev,
> + "fail to devm_gpio_request reset\n");
> + return ret;
> + }
> + }

You are not handling the flags part of the GPIO binding. It is better
to use devm_gpiod_ API calls, which will handle the active low flags
for you.

Andrew

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

[sean.wang]

From: Sean Wang 

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 
Signed-off-by: Landen Chao 
---
 drivers/net/dsa/Kconfig  |8 +
 drivers/net/dsa/Makefile |2 +-
 drivers/net/dsa/mt7530.c | 1195 ++
 drivers/net/dsa/mt7530.h |  387 +++
 4 files changed, 1591 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 8346e4f..84e734e 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -1,6 +1,6 @@
 obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
 obj-$(CONFIG_NET_DSA_BCM_SF2)  += bcm_sf2.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 000..e316b9b
--- /dev/null
+++ b/drivers/net/dsa/mt7530.c
@@ -0,0 +1,1195 @@
+/*
+ * Mediatek MT7530 DSA Switch driver
+ * Copyright (C) 2017 Sean Wang 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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,