* [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding @ 2017-05-30 10:44 John Crispin 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: John Crispin @ 2017-05-30 10:44 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Sean Wang Cc: netdev, linux-kernel, John Crispin, Rob Herring, devicetree Extend the DSA binding documentation, adding the new property required when there is more than one CPU port attached to the switch. Cc: Rob Herring <robh+dt@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: John Crispin <john@phrozen.org> --- Documentation/devicetree/bindings/net/dsa/dsa.txt | 61 ++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt index cfe8f64eca4f..c164eb38ccc5 100644 --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt @@ -55,6 +55,11 @@ A user port has the following optional property: - label : Describes the label associated with this port, which will become the netdev name. +- cpu : Option for non "cpu"/"dsa" ports. A phandle to a + "cpu" port, which will be used for passing packets + from this port to the host. If not present, the first + "cpu" port will be used. + Port child nodes may also contain the following optional standardised properties, described in binding documents: @@ -71,7 +76,7 @@ properties, described in binding documents: Documentation/devicetree/bindings/net/fixed-link.txt for details. -Example +Examples The following example shows three switches on three MDIO busses, linked into one DSA cluster. @@ -264,6 +269,60 @@ linked into one DSA cluster. }; }; +The following example shows a switch that has two cpu ports each connecting +to a different MAC. + +&mdio0 { + switch@0 { + compatible = "mediatek,mt7530"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + label = "lan0"; + cpu = <&cpu_port1>; + }; + + port@1 { + reg = <1>; + label = "lan1"; + cpu = <&cpu_port1>; + }; + + port@2 { + reg = <2>; + label = "lan2"; + cpu = <&cpu_port1>; + }; + + port@3 { + reg = <3>; + label = "wan"; + cpu = <&cpu_port2>; + }; + + cpu_port2: port@5 { + reg = <5>; + label = "cpu"; + ethernet = <&gmac2>; + phy-mode = "trgmii"; + }; + + cpu_port1: port@6 { + reg = <6>; + label = "cpu"; + ethernet = <&gmac1>; + phy-mode = "trgmii"; + }; + }; + }; +}; + Deprecated Binding ------------------ -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 10:44 [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding John Crispin @ 2017-05-30 10:44 ` John Crispin 2017-05-30 15:38 ` kbuild test robot ` (2 more replies) 2017-05-30 10:44 ` [PATCH V2 3/3] net-next: dsa: mt7530: " John Crispin 2017-05-30 21:32 ` [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding Florian Fainelli 2 siblings, 3 replies; 19+ messages in thread From: John Crispin @ 2017-05-30 10:44 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Sean Wang Cc: netdev, linux-kernel, John Crispin Some boards have two CPU interfaces connected to the switch, e.g. WiFi access points, with 1 port labeled WAN, 4 ports labeled lan1-lan4, and two port connected to the SoC. This patch extends DSA to allows both CPU ports to be used. The "cpu" node in the DSA tree can now have a phandle to the host interface it connects to. Each user port can have a phandle to a cpu port which should be used for traffic between the port and the CPU. Thus simple load sharing over the two CPU ports can be achieved. Signed-off-by: John Crispin <john@phrozen.org> --- include/net/dsa.h | 21 ++++++++++++++++++++- net/dsa/dsa2.c | 35 +++++++++++++++++++++++++++++++---- net/dsa/dsa_priv.h | 1 + net/dsa/slave.c | 26 ++++++++++++++++---------- 4 files changed, 68 insertions(+), 15 deletions(-) diff --git a/include/net/dsa.h b/include/net/dsa.h index c0e567c0c824..d2994bd2c507 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -186,6 +186,8 @@ struct dsa_port { u8 stp_state; struct net_device *bridge_dev; struct devlink_port devlink_port; + struct net_device *ethernet; + int upstream; }; struct dsa_switch { @@ -251,7 +253,7 @@ struct dsa_switch { static inline bool dsa_is_cpu_port(struct dsa_switch *ds, int p) { - return ds->dst->cpu_dp == &ds->ports[p]; + return !!(ds->cpu_port_mask & (1 << p)); } static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p) @@ -269,6 +271,11 @@ static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p) return ds->enabled_port_mask & (1 << p) && ds->ports[p].netdev; } +static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int p) +{ + return dsa_is_cpu_port(ds, p) || dsa_is_dsa_port(ds, p); +} + static inline u8 dsa_upstream_port(struct dsa_switch *ds) { struct dsa_switch_tree *dst = ds->dst; @@ -285,6 +292,18 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds) return ds->rtable[dst->cpu_dp->ds->index]; } +static inline u8 dsa_port_upstream_port(struct dsa_switch *ds, int port) +{ + /* + * If this port has a specific upstream cpu port, use it, + * otherwise use the switch default. + */ + if (ds->ports[port].upstream) + return ds->ports[port].upstream; + else + return dsa_upstream_port(ds); +} + struct dsa_switch_ops { /* * Legacy probing. diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 4301f52e4f5a..8b13aa735c40 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -253,8 +253,6 @@ static int dsa_cpu_port_apply(struct dsa_port *port, u32 index, return err; } - ds->cpu_port_mask |= BIT(index); - memset(&ds->ports[index].devlink_port, 0, sizeof(ds->ports[index].devlink_port)); err = devlink_port_register(ds->devlink, &ds->ports[index].devlink_port, @@ -269,6 +267,10 @@ static void dsa_cpu_port_unapply(struct dsa_port *port, u32 index, dsa_cpu_dsa_destroy(port); ds->cpu_port_mask &= ~BIT(index); + if (ds->ports[index].ethernet) { + dev_put(ds->ports[index].ethernet); + ds->ports[index].ethernet = NULL; + } } static int dsa_user_port_apply(struct dsa_port *port, u32 index, @@ -530,6 +532,29 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, dst->rcv = dst->tag_ops->rcv; + dev_hold(ethernet_dev); + ds->ports[index].ethernet = ethernet_dev; + ds->cpu_port_mask |= BIT(index); + + return 0; +} + +static int dsa_user_parse(struct device_node *port, u32 index, + struct dsa_switch *ds) +{ + struct device_node *cpu_port; + const unsigned int *cpu_port_reg; + int cpu_port_index; + + cpu_port = of_parse_phandle(port, "cpu", 0); + if (cpu_port) { + cpu_port_reg = of_get_property(cpu_port, "reg", NULL); + if (!cpu_port_reg) + return -EINVAL; + cpu_port_index = be32_to_cpup(cpu_port_reg); + ds->ports[index].upstream = cpu_port_index; + } + return 0; } @@ -544,11 +569,13 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds) if (!dsa_port_is_valid(port)) continue; - if (dsa_port_is_cpu(port)) { + if (dsa_port_is_cpu(port)) err = dsa_cpu_parse(port, index, dst, ds); + else if (dsa_is_normal_port(port)) + err = dsa_user_parse(port->dn, index, ds); + if (err) return err; - } } pr_info("DSA: switch %d %d parsed\n", dst->tree, ds->index); diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index c1d4180651af..91fdc16befb2 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -78,6 +78,7 @@ struct dsa_slave_priv { /* DSA port data, such as switch, port index, etc. */ struct dsa_port *dp; + struct net_device *master; /* * The phylib phy_device pointer for the PHY connected diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 887e26695519..45f17f35ced1 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -66,7 +66,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); - return p->dp->ds->dst->master_netdev->ifindex; + return p->master->ifindex; } static inline bool dsa_port_is_bridged(struct dsa_port *dp) @@ -77,7 +77,7 @@ static inline bool dsa_port_is_bridged(struct dsa_port *dp) static int dsa_slave_open(struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); - struct net_device *master = p->dp->ds->dst->master_netdev; + struct net_device *master = p->master; struct dsa_switch *ds = p->dp->ds; u8 stp_state = dsa_port_is_bridged(p->dp) ? BR_STATE_BLOCKING : BR_STATE_FORWARDING; @@ -132,7 +132,7 @@ static int dsa_slave_open(struct net_device *dev) static int dsa_slave_close(struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); - struct net_device *master = p->dp->ds->dst->master_netdev; + struct net_device *master = p->master; struct dsa_switch *ds = p->dp->ds; if (p->phy) @@ -159,7 +159,7 @@ static int dsa_slave_close(struct net_device *dev) static void dsa_slave_change_rx_flags(struct net_device *dev, int change) { struct dsa_slave_priv *p = netdev_priv(dev); - struct net_device *master = p->dp->ds->dst->master_netdev; + struct net_device *master = p->master; if (change & IFF_ALLMULTI) dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1); @@ -170,7 +170,7 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change) static void dsa_slave_set_rx_mode(struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); - struct net_device *master = p->dp->ds->dst->master_netdev; + struct net_device *master = p->master; dev_mc_sync(master, dev); dev_uc_sync(master, dev); @@ -179,7 +179,7 @@ static void dsa_slave_set_rx_mode(struct net_device *dev) static int dsa_slave_set_mac_address(struct net_device *dev, void *a) { struct dsa_slave_priv *p = netdev_priv(dev); - struct net_device *master = p->dp->ds->dst->master_netdev; + struct net_device *master = p->master; struct sockaddr *addr = a; int err; @@ -376,7 +376,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) /* Queue the SKB for transmission on the parent interface, but * do not modify its EtherType */ - nskb->dev = p->dp->ds->dst->master_netdev; + nskb->dev = p->master; dev_queue_xmit(nskb); return NETDEV_TX_OK; @@ -685,7 +685,7 @@ static int dsa_slave_netpoll_setup(struct net_device *dev, { struct dsa_slave_priv *p = netdev_priv(dev); struct dsa_switch *ds = p->dp->ds; - struct net_device *master = ds->dst->master_netdev; + struct net_device *master = p->master; struct netpoll *netpoll; int err = 0; @@ -1140,11 +1140,16 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, struct net_device *master; struct net_device *slave_dev; struct dsa_slave_priv *p; + int port_cpu = ds->ports[port].upstream; int ret; - master = ds->dst->master_netdev; - if (ds->master_netdev) + if (port_cpu && ds->ports[port_cpu].ethernet) + master = ds->ports[port_cpu].ethernet; + else if (ds->master_netdev) master = ds->master_netdev; + else + master = ds->dst->master_netdev; + master->dsa_ptr = (void *)ds->dst; slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name, NET_NAME_UNKNOWN, ether_setup); @@ -1173,6 +1178,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, p->dp = &ds->ports[port]; INIT_LIST_HEAD(&p->mall_tc_list); p->xmit = dst->tag_ops->xmit; + p->master = master; p->old_pause = -1; p->old_link = -1; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin @ 2017-05-30 15:38 ` kbuild test robot 2017-05-30 18:37 ` John Crispin 2017-05-30 19:45 ` Vivien Didelot 2017-05-30 22:56 ` Florian Fainelli 2 siblings, 1 reply; 19+ messages in thread From: kbuild test robot @ 2017-05-30 15:38 UTC (permalink / raw) To: John Crispin Cc: kbuild-all, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Sean Wang, netdev, linux-kernel, John Crispin [-- Attachment #1: Type: text/plain, Size: 6633 bytes --] Hi John, [auto build test ERROR on net-next/master] [also build test ERROR on next-20170530] [cannot apply to v4.12-rc3] [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/John-Crispin/Documentation-devicetree-add-multiple-cpu-port-DSA-binding/20170530-224954 config: x86_64-randconfig-x014-201722 (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 error/warnings (new ones prefixed by >>): In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: net//dsa/dsa2.c: In function 'dsa_ds_parse': >> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:160:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:160:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: declared here static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:160:42: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:160:42: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: declared here static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:171:16: note: in definition of macro '__trace_if' ______r = !!(cond); \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ In file included from include/linux/ioport.h:12:0, from include/linux/device.h:16, from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' else if (dsa_is_normal_port(port)) ^ include/linux/compiler.h:171:16: note: in definition of macro '__trace_if' ______r = !!(cond); \ ^~~~ >> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' else if (dsa_is_normal_port(port)) ^~ In file included from net//dsa/dsa_priv.h:17:0, from net//dsa/dsa2.c:22: include/net/dsa.h:264:20: note: declared here static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/dsa_is_normal_port +574 net//dsa/dsa2.c 568 port = &ds->ports[index]; 569 if (!dsa_port_is_valid(port)) 570 continue; 571 572 if (dsa_port_is_cpu(port)) 573 err = dsa_cpu_parse(port, index, dst, ds); > 574 else if (dsa_is_normal_port(port)) 575 err = dsa_user_parse(port->dn, index, ds); 576 577 if (err) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 28681 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 15:38 ` kbuild test robot @ 2017-05-30 18:37 ` John Crispin 2017-05-30 19:15 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: John Crispin @ 2017-05-30 18:37 UTC (permalink / raw) To: kbuild test robot Cc: kbuild-all, Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Sean Wang, netdev, linux-kernel Hi, the patch series is based on net-next from 12 hours ago and works fine on that tree. I compile and runtime tested it quite intensively on various boards John On 30/05/17 17:38, kbuild test robot wrote: > Hi John, > > [auto build test ERROR on net-next/master] > [also build test ERROR on next-20170530] > [cannot apply to v4.12-rc3] > [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/John-Crispin/Documentation-devicetree-add-multiple-cpu-port-DSA-binding/20170530-224954 > config: x86_64-randconfig-x014-201722 (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 error/warnings (new ones prefixed by >>): > > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: > net//dsa/dsa2.c: In function 'dsa_ds_parse': >>> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:160:30: note: in definition of macro '__trace_if' > if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: >>> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:160:30: note: in definition of macro '__trace_if' > if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: declared here > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: >>> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:160:42: note: in definition of macro '__trace_if' > if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: >>> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:160:42: note: in definition of macro '__trace_if' > if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: declared here > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: >>> net//dsa/dsa2.c:574:31: error: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [-Werror=incompatible-pointer-types] > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:171:16: note: in definition of macro '__trace_if' > ______r = !!(cond); \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > In file included from include/linux/ioport.h:12:0, > from include/linux/device.h:16, > from net//dsa/dsa2.c:13: >>> net//dsa/dsa2.c:574:12: error: too few arguments to function 'dsa_is_normal_port' > else if (dsa_is_normal_port(port)) > ^ > include/linux/compiler.h:171:16: note: in definition of macro '__trace_if' > ______r = !!(cond); \ > ^~~~ >>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' > else if (dsa_is_normal_port(port)) > ^~ > In file included from net//dsa/dsa_priv.h:17:0, > from net//dsa/dsa2.c:22: > include/net/dsa.h:264:20: note: declared here > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^~~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors > > vim +/dsa_is_normal_port +574 net//dsa/dsa2.c > > 568 port = &ds->ports[index]; > 569 if (!dsa_port_is_valid(port)) > 570 continue; > 571 > 572 if (dsa_port_is_cpu(port)) > 573 err = dsa_cpu_parse(port, index, dst, ds); > > 574 else if (dsa_is_normal_port(port)) > 575 err = dsa_user_parse(port->dn, index, ds); > 576 > 577 if (err) > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 18:37 ` John Crispin @ 2017-05-30 19:15 ` Florian Fainelli 2017-05-30 19:23 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-30 19:15 UTC (permalink / raw) To: John Crispin, kbuild test robot Cc: kbuild-all, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel Hi John, On 05/30/2017 11:37 AM, John Crispin wrote: > Hi, > > the patch series is based on net-next from 12 hours ago and works fine > on that tree. I compile and runtime tested it quite intensively on > various boards The warning is legit though: 572 if (dsa_port_is_cpu(port)) 573 err = dsa_cpu_parse(port, index, dst, ds); > 574 else if (dsa_is_normal_port(port)) 575 err = dsa_user_parse(port->dn, index, ds); and after applying your patches I also met it: net/dsa/dsa2.c: In function 'dsa_ds_parse': net/dsa/dsa2.c:574:3: warning: passing argument 1 of 'dsa_is_normal_port' from incompatible pointer type [enabled by default] else if (dsa_is_normal_port(port)) ^ In file included from net/dsa/dsa_priv.h:17:0, from net/dsa/dsa2.c:22: ./include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but argument is of type 'struct dsa_port *' static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^ net/dsa/dsa2.c:574:3: error: too few arguments to function 'dsa_is_normal_port' else if (dsa_is_normal_port(port)) ^ In file included from net/dsa/dsa_priv.h:17:0, from net/dsa/dsa2.c:22: ./include/net/dsa.h:264:20: note: declared here static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) ^ CC net/bridge/br_stp.o scripts/Makefile.build:302: recipe for target 'net/dsa/dsa2.o' failed make[4]: *** [net/dsa/dsa2.o] Error 1 scripts/Makefile.build:561: recipe for target 'net/dsa' failed make[3]: *** [net/dsa] Error 2 make[3]: *** Waiting for unfinished jobs.... We need something like this, I have some comments on your patches that I will send shortly. Thanks! diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 8b13aa735c40..124c5acfa123 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -539,14 +539,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, return 0; } -static int dsa_user_parse(struct device_node *port, u32 index, +static int dsa_user_parse(struct dsa_port *port, u32 index, struct dsa_switch *ds) { struct device_node *cpu_port; const unsigned int *cpu_port_reg; int cpu_port_index; - cpu_port = of_parse_phandle(port, "cpu", 0); + cpu_port = of_parse_phandle(port->dn, "cpu", 0); if (cpu_port) { cpu_port_reg = of_get_property(cpu_port, "reg", NULL); if (!cpu_port_reg) @@ -572,7 +572,7 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds) if (dsa_port_is_cpu(port)) err = dsa_cpu_parse(port, index, dst, ds); else if (dsa_is_normal_port(port)) - err = dsa_user_parse(port->dn, index, ds); + err = dsa_user_parse(port, index, ds); if (err) return err; > > John > > > On 30/05/17 17:38, kbuild test robot wrote: >> Hi John, >> >> [auto build test ERROR on net-next/master] >> [also build test ERROR on next-20170530] >> [cannot apply to v4.12-rc3] >> [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/John-Crispin/Documentation-devicetree-add-multiple-cpu-port-DSA-binding/20170530-224954 >> >> config: x86_64-randconfig-x014-201722 (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 error/warnings (new ones prefixed by >>): >> >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >> net//dsa/dsa2.c: In function 'dsa_ds_parse': >>>> net//dsa/dsa2.c:574:31: error: passing argument 1 of >>>> 'dsa_is_normal_port' from incompatible pointer type >>>> [-Werror=incompatible-pointer-types] >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:160:30: note: in definition of macro >> '__trace_if' >> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but >> argument is of type 'struct dsa_port *' >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >>>> net//dsa/dsa2.c:574:12: error: too few arguments to function >>>> 'dsa_is_normal_port' >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:160:30: note: in definition of macro >> '__trace_if' >> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: declared here >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >>>> net//dsa/dsa2.c:574:31: error: passing argument 1 of >>>> 'dsa_is_normal_port' from incompatible pointer type >>>> [-Werror=incompatible-pointer-types] >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:160:42: note: in definition of macro >> '__trace_if' >> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but >> argument is of type 'struct dsa_port *' >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >>>> net//dsa/dsa2.c:574:12: error: too few arguments to function >>>> 'dsa_is_normal_port' >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:160:42: note: in definition of macro >> '__trace_if' >> if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: declared here >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >>>> net//dsa/dsa2.c:574:31: error: passing argument 1 of >>>> 'dsa_is_normal_port' from incompatible pointer type >>>> [-Werror=incompatible-pointer-types] >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:171:16: note: in definition of macro >> '__trace_if' >> ______r = !!(cond); \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but >> argument is of type 'struct dsa_port *' >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> In file included from include/linux/ioport.h:12:0, >> from include/linux/device.h:16, >> from net//dsa/dsa2.c:13: >>>> net//dsa/dsa2.c:574:12: error: too few arguments to function >>>> 'dsa_is_normal_port' >> else if (dsa_is_normal_port(port)) >> ^ >> include/linux/compiler.h:171:16: note: in definition of macro >> '__trace_if' >> ______r = !!(cond); \ >> ^~~~ >>>> net//dsa/dsa2.c:574:8: note: in expansion of macro 'if' >> else if (dsa_is_normal_port(port)) >> ^~ >> In file included from net//dsa/dsa_priv.h:17:0, >> from net//dsa/dsa2.c:22: >> include/net/dsa.h:264:20: note: declared here >> static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) >> ^~~~~~~~~~~~~~~~~~ >> cc1: some warnings being treated as errors >> >> vim +/dsa_is_normal_port +574 net//dsa/dsa2.c >> >> 568 port = &ds->ports[index]; >> 569 if (!dsa_port_is_valid(port)) >> 570 continue; >> 571 >> 572 if (dsa_port_is_cpu(port)) >> 573 err = dsa_cpu_parse(port, index, dst, ds); >> > 574 else if (dsa_is_normal_port(port)) >> 575 err = dsa_user_parse(port->dn, index, ds); >> 576 >> 577 if (err) >> >> --- >> 0-DAY kernel test infrastructure Open Source Technology >> Center >> https://lists.01.org/pipermail/kbuild-all Intel >> Corporation > -- Florian ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 19:15 ` Florian Fainelli @ 2017-05-30 19:23 ` Florian Fainelli 0 siblings, 0 replies; 19+ messages in thread From: Florian Fainelli @ 2017-05-30 19:23 UTC (permalink / raw) To: John Crispin, kbuild test robot Cc: kbuild-all, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel On 05/30/2017 12:15 PM, Florian Fainelli wrote: > Hi John, > > On 05/30/2017 11:37 AM, John Crispin wrote: >> Hi, >> >> the patch series is based on net-next from 12 hours ago and works fine >> on that tree. I compile and runtime tested it quite intensively on >> various boards > > The warning is legit though: > > 572 if (dsa_port_is_cpu(port)) > 573 err = dsa_cpu_parse(port, index, dst, ds); > > 574 else if (dsa_is_normal_port(port)) > 575 err = dsa_user_parse(port->dn, index, ds); > > and after applying your patches I also met it: > > net/dsa/dsa2.c: In function 'dsa_ds_parse': > net/dsa/dsa2.c:574:3: warning: passing argument 1 of > 'dsa_is_normal_port' from incompatible pointer type [enabled by default] > else if (dsa_is_normal_port(port)) > ^ > In file included from net/dsa/dsa_priv.h:17:0, > from net/dsa/dsa2.c:22: > ./include/net/dsa.h:264:20: note: expected 'struct dsa_switch *' but > argument is of type 'struct dsa_port *' > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^ > net/dsa/dsa2.c:574:3: error: too few arguments to function > 'dsa_is_normal_port' > else if (dsa_is_normal_port(port)) > ^ > In file included from net/dsa/dsa_priv.h:17:0, > from net/dsa/dsa2.c:22: > ./include/net/dsa.h:264:20: note: declared here > static inline bool dsa_is_normal_port(struct dsa_switch *ds, int p) > ^ > CC net/bridge/br_stp.o > scripts/Makefile.build:302: recipe for target 'net/dsa/dsa2.o' failed > make[4]: *** [net/dsa/dsa2.o] Error 1 > scripts/Makefile.build:561: recipe for target 'net/dsa' failed > make[3]: *** [net/dsa] Error 2 > make[3]: *** Waiting for unfinished jobs.... > > We need something like this, I have some comments on your patches that I > will send shortly. Thanks! > > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index 8b13aa735c40..124c5acfa123 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -539,14 +539,14 @@ static int dsa_cpu_parse(struct dsa_port *port, > u32 index, > return 0; > } > > -static int dsa_user_parse(struct device_node *port, u32 index, > +static int dsa_user_parse(struct dsa_port *port, u32 index, > struct dsa_switch *ds) > { > struct device_node *cpu_port; > const unsigned int *cpu_port_reg; > int cpu_port_index; > > - cpu_port = of_parse_phandle(port, "cpu", 0); > + cpu_port = of_parse_phandle(port->dn, "cpu", 0); > if (cpu_port) { > cpu_port_reg = of_get_property(cpu_port, "reg", NULL); > if (!cpu_port_reg) > @@ -572,7 +572,7 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, > struct dsa_switch *ds) > if (dsa_port_is_cpu(port)) > err = dsa_cpu_parse(port, index, dst, ds); > else if (dsa_is_normal_port(port)) > - err = dsa_user_parse(port->dn, index, ds); > + err = dsa_user_parse(port, index, ds); > > if (err) > return err; here is a version that builds, I missed one hunk: diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 8b13aa735c40..d71a2a610340 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -539,14 +539,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, return 0; } -static int dsa_user_parse(struct device_node *port, u32 index, +static int dsa_user_parse(struct dsa_port *port, u32 index, struct dsa_switch *ds) { struct device_node *cpu_port; const unsigned int *cpu_port_reg; int cpu_port_index; - cpu_port = of_parse_phandle(port, "cpu", 0); + cpu_port = of_parse_phandle(port->dn, "cpu", 0); if (cpu_port) { cpu_port_reg = of_get_property(cpu_port, "reg", NULL); if (!cpu_port_reg) @@ -571,8 +571,8 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds) if (dsa_port_is_cpu(port)) err = dsa_cpu_parse(port, index, dst, ds); - else if (dsa_is_normal_port(port)) - err = dsa_user_parse(port->dn, index, ds); + else if (dsa_is_normal_port(ds, index)) + err = dsa_user_parse(port, index, ds); if (err) return err; -- Florian ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin 2017-05-30 15:38 ` kbuild test robot @ 2017-05-30 19:45 ` Vivien Didelot 2017-05-30 19:50 ` Vivien Didelot 2017-05-30 22:56 ` Florian Fainelli 2 siblings, 1 reply; 19+ messages in thread From: Vivien Didelot @ 2017-05-30 19:45 UTC (permalink / raw) To: John Crispin, Andrew Lunn, Florian Fainelli, David S . Miller, Sean Wang Cc: netdev, linux-kernel, John Crispin Hi John, John Crispin <john@phrozen.org> writes: > +static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int p) > +{ > + return dsa_is_cpu_port(ds, p) || dsa_is_dsa_port(ds, p); > +} This looks confusing to me. What DSA calls an "upstream" port for the moment is the port which goes to the CPU interface. CPU0 (eth0) | | sw0 sw1 sw2 [p0 p1 p2]--[p0 p1 p2]--[p0 p1 p2] | | | | eth1 eth2 eth3 eth4 So in the example above, sw1p0 is an upstream port, but sw1p2 is not. This is why dsa_upstream_port makes use of ds->rtable. > @@ -1140,11 +1140,16 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, > struct net_device *master; > struct net_device *slave_dev; > struct dsa_slave_priv *p; > + int port_cpu = ds->ports[port].upstream; ds->ports[port] is p->dp. > int ret; > > - master = ds->dst->master_netdev; > - if (ds->master_netdev) > + if (port_cpu && ds->ports[port_cpu].ethernet) 0 is a valid port index for a CPU, e.g. Marvell 88E6390. > + master = ds->ports[port_cpu].ethernet; > + else if (ds->master_netdev) > master = ds->master_netdev; > + else > + master = ds->dst->master_netdev; > + master->dsa_ptr = (void *)ds->dst; > > slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name, > NET_NAME_UNKNOWN, ether_setup); > @@ -1173,6 +1178,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, > p->dp = &ds->ports[port]; > INIT_LIST_HEAD(&p->mall_tc_list); > p->xmit = dst->tag_ops->xmit; > + p->master = master; I'm a bit confused why we need all these references to net devices. We now have ds->master_netdev, dst->master_netdev, dp->ethernet and p->master... Wouldn't it be simpler if we only had dp->ethernet (or whichever more explicit name) for the conduit interface used to send/receive frames? Maybe I am missing something, in which case I'm sorry in advance. Thanks, Vivien ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 19:45 ` Vivien Didelot @ 2017-05-30 19:50 ` Vivien Didelot 0 siblings, 0 replies; 19+ messages in thread From: Vivien Didelot @ 2017-05-30 19:50 UTC (permalink / raw) To: John Crispin, Andrew Lunn, Florian Fainelli, David S . Miller, Sean Wang Cc: netdev, linux-kernel, John Crispin Hi John, Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes: >> + int port_cpu = ds->ports[port].upstream; > > ds->ports[port] is p->dp. I misread this part, p is not yet allocated in that chunk, please ignore this one comment ;-) Thanks, Vivien ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin 2017-05-30 15:38 ` kbuild test robot 2017-05-30 19:45 ` Vivien Didelot @ 2017-05-30 22:56 ` Florian Fainelli 2017-05-31 0:06 ` Andrew Lunn 2 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-30 22:56 UTC (permalink / raw) To: John Crispin, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang, jiri, idosch Cc: netdev, linux-kernel +Jiri, Ido, On 05/30/2017 03:44 AM, John Crispin wrote: > Some boards have two CPU interfaces connected to the switch, e.g. WiFi > access points, with 1 port labeled WAN, 4 ports labeled lan1-lan4, and > two port connected to the SoC. > > This patch extends DSA to allows both CPU ports to be used. The "cpu" > node in the DSA tree can now have a phandle to the host interface it > connects to. Each user port can have a phandle to a cpu port which > should be used for traffic between the port and the CPU. Thus simple > load sharing over the two CPU ports can be achieved. Part of the problem here is that: - we have the initial state where we only allow the user-ports to be talking to the CPU port, now we have more than one CPU port, so which one do we choose? You have proposed a Device Tree change for that, and while this works, we are going beyond pure HW description and we are already describing a desired policy, not ideal - past the initial setup, if we start creating bridge devices and so on, we have no way to tell: group Ports 0-3 together and send traffic to CPU port 0, then let Port 5 alone and send traffic to CPU port 1, that's a DSA-only problem though, because we still have the CPU port(s) as independent network interfaces. Right now, we cannot enslave master network devices into the bridge when a tagging protocol is used (see 8db0a2ee2c6302a1dcbcdb93cb731dfc6c0cdb5e) so we cannot quite associate a bridge device with its underlying CPU port. I suppose we could revise that commit and let the enslaving happen in order for the switch drivers to "learn" which CPU/master network device is being added to the bridge, and just not register the RX handler for that interface. Now, that would still force the user to configure two bridges in order to properly steer traffic towards the requested ports but it would allow us to be very flexible (which is probably desired here) in how ports are grouped together. Does that make sense? > > Signed-off-by: John Crispin <john@phrozen.org> > --- > include/net/dsa.h | 21 ++++++++++++++++++++- > net/dsa/dsa2.c | 35 +++++++++++++++++++++++++++++++---- > net/dsa/dsa_priv.h | 1 + > net/dsa/slave.c | 26 ++++++++++++++++---------- > 4 files changed, 68 insertions(+), 15 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index c0e567c0c824..d2994bd2c507 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -186,6 +186,8 @@ struct dsa_port { > u8 stp_state; > struct net_device *bridge_dev; > struct devlink_port devlink_port; > + struct net_device *ethernet; > + int upstream; > }; > > struct dsa_switch { > @@ -251,7 +253,7 @@ struct dsa_switch { > > static inline bool dsa_is_cpu_port(struct dsa_switch *ds, int p) > { > - return ds->dst->cpu_dp == &ds->ports[p]; > + return !!(ds->cpu_port_mask & (1 << p)); > } > > static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p) > @@ -269,6 +271,11 @@ static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p) > return ds->enabled_port_mask & (1 << p) && ds->ports[p].netdev; > } > > +static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int p) > +{ > + return dsa_is_cpu_port(ds, p) || dsa_is_dsa_port(ds, p); > +} > + > static inline u8 dsa_upstream_port(struct dsa_switch *ds) > { > struct dsa_switch_tree *dst = ds->dst; > @@ -285,6 +292,18 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds) > return ds->rtable[dst->cpu_dp->ds->index]; > } > > +static inline u8 dsa_port_upstream_port(struct dsa_switch *ds, int port) > +{ > + /* > + * If this port has a specific upstream cpu port, use it, > + * otherwise use the switch default. > + */ > + if (ds->ports[port].upstream) > + return ds->ports[port].upstream; > + else > + return dsa_upstream_port(ds); > +} > + > struct dsa_switch_ops { > /* > * Legacy probing. > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index 4301f52e4f5a..8b13aa735c40 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -253,8 +253,6 @@ static int dsa_cpu_port_apply(struct dsa_port *port, u32 index, > return err; > } > > - ds->cpu_port_mask |= BIT(index); > - > memset(&ds->ports[index].devlink_port, 0, > sizeof(ds->ports[index].devlink_port)); > err = devlink_port_register(ds->devlink, &ds->ports[index].devlink_port, > @@ -269,6 +267,10 @@ static void dsa_cpu_port_unapply(struct dsa_port *port, u32 index, > dsa_cpu_dsa_destroy(port); > ds->cpu_port_mask &= ~BIT(index); > > + if (ds->ports[index].ethernet) { > + dev_put(ds->ports[index].ethernet); > + ds->ports[index].ethernet = NULL; > + } > } > > static int dsa_user_port_apply(struct dsa_port *port, u32 index, > @@ -530,6 +532,29 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, > > dst->rcv = dst->tag_ops->rcv; > > + dev_hold(ethernet_dev); > + ds->ports[index].ethernet = ethernet_dev; > + ds->cpu_port_mask |= BIT(index); > + > + return 0; > +} > + > +static int dsa_user_parse(struct device_node *port, u32 index, > + struct dsa_switch *ds) > +{ > + struct device_node *cpu_port; > + const unsigned int *cpu_port_reg; > + int cpu_port_index; > + > + cpu_port = of_parse_phandle(port, "cpu", 0); > + if (cpu_port) { > + cpu_port_reg = of_get_property(cpu_port, "reg", NULL); > + if (!cpu_port_reg) > + return -EINVAL; > + cpu_port_index = be32_to_cpup(cpu_port_reg); > + ds->ports[index].upstream = cpu_port_index; > + } > + > return 0; > } > > @@ -544,11 +569,13 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds) > if (!dsa_port_is_valid(port)) > continue; > > - if (dsa_port_is_cpu(port)) { > + if (dsa_port_is_cpu(port)) > err = dsa_cpu_parse(port, index, dst, ds); > + else if (dsa_is_normal_port(port)) > + err = dsa_user_parse(port->dn, index, ds); > + > if (err) > return err; > - } > } > > pr_info("DSA: switch %d %d parsed\n", dst->tree, ds->index); > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index c1d4180651af..91fdc16befb2 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -78,6 +78,7 @@ struct dsa_slave_priv { > > /* DSA port data, such as switch, port index, etc. */ > struct dsa_port *dp; > + struct net_device *master; > > /* > * The phylib phy_device pointer for the PHY connected > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 887e26695519..45f17f35ced1 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -66,7 +66,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > > - return p->dp->ds->dst->master_netdev->ifindex; > + return p->master->ifindex; > } > > static inline bool dsa_port_is_bridged(struct dsa_port *dp) > @@ -77,7 +77,7 @@ static inline bool dsa_port_is_bridged(struct dsa_port *dp) > static int dsa_slave_open(struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > - struct net_device *master = p->dp->ds->dst->master_netdev; > + struct net_device *master = p->master; > struct dsa_switch *ds = p->dp->ds; > u8 stp_state = dsa_port_is_bridged(p->dp) ? > BR_STATE_BLOCKING : BR_STATE_FORWARDING; > @@ -132,7 +132,7 @@ static int dsa_slave_open(struct net_device *dev) > static int dsa_slave_close(struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > - struct net_device *master = p->dp->ds->dst->master_netdev; > + struct net_device *master = p->master; > struct dsa_switch *ds = p->dp->ds; > > if (p->phy) > @@ -159,7 +159,7 @@ static int dsa_slave_close(struct net_device *dev) > static void dsa_slave_change_rx_flags(struct net_device *dev, int change) > { > struct dsa_slave_priv *p = netdev_priv(dev); > - struct net_device *master = p->dp->ds->dst->master_netdev; > + struct net_device *master = p->master; > > if (change & IFF_ALLMULTI) > dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1); > @@ -170,7 +170,7 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change) > static void dsa_slave_set_rx_mode(struct net_device *dev) > { > struct dsa_slave_priv *p = netdev_priv(dev); > - struct net_device *master = p->dp->ds->dst->master_netdev; > + struct net_device *master = p->master; > > dev_mc_sync(master, dev); > dev_uc_sync(master, dev); > @@ -179,7 +179,7 @@ static void dsa_slave_set_rx_mode(struct net_device *dev) > static int dsa_slave_set_mac_address(struct net_device *dev, void *a) > { > struct dsa_slave_priv *p = netdev_priv(dev); > - struct net_device *master = p->dp->ds->dst->master_netdev; > + struct net_device *master = p->master; > struct sockaddr *addr = a; > int err; > > @@ -376,7 +376,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) > /* Queue the SKB for transmission on the parent interface, but > * do not modify its EtherType > */ > - nskb->dev = p->dp->ds->dst->master_netdev; > + nskb->dev = p->master; > dev_queue_xmit(nskb); > > return NETDEV_TX_OK; > @@ -685,7 +685,7 @@ static int dsa_slave_netpoll_setup(struct net_device *dev, > { > struct dsa_slave_priv *p = netdev_priv(dev); > struct dsa_switch *ds = p->dp->ds; > - struct net_device *master = ds->dst->master_netdev; > + struct net_device *master = p->master; > struct netpoll *netpoll; > int err = 0; > > @@ -1140,11 +1140,16 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, > struct net_device *master; > struct net_device *slave_dev; > struct dsa_slave_priv *p; > + int port_cpu = ds->ports[port].upstream; > int ret; > > - master = ds->dst->master_netdev; > - if (ds->master_netdev) > + if (port_cpu && ds->ports[port_cpu].ethernet) > + master = ds->ports[port_cpu].ethernet; > + else if (ds->master_netdev) > master = ds->master_netdev; > + else > + master = ds->dst->master_netdev; > + master->dsa_ptr = (void *)ds->dst; > > slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name, > NET_NAME_UNKNOWN, ether_setup); > @@ -1173,6 +1178,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, > p->dp = &ds->ports[port]; > INIT_LIST_HEAD(&p->mall_tc_list); > p->xmit = dst->tag_ops->xmit; > + p->master = master; > > p->old_pause = -1; > p->old_link = -1; > -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-30 22:56 ` Florian Fainelli @ 2017-05-31 0:06 ` Andrew Lunn 2017-05-31 0:16 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2017-05-31 0:06 UTC (permalink / raw) To: Florian Fainelli Cc: John Crispin, Vivien Didelot, David S . Miller, Sean Wang, jiri, idosch, netdev, linux-kernel > - past the initial setup, if we start creating bridge devices and so on, > we have no way to tell: group Ports 0-3 together and send traffic to CPU > port 0, then let Port 5 alone and send traffic to CPU port 1, that's a > DSA-only problem though, because we still have the CPU port(s) as > independent network interfaces. What is the problem here? Frames come out the master interface, get untagged and passed to the slave interface and go upto the bridge. It should all just work. Same in the reverse direction. In order to make best use of the extra bandwidth of having two cpu ports, i probably want the user ports reasonably evenly distributed between the CPU ports. Dedicating one CPU port to one user port is probably sub-optimal. How many people have 1Gbps Fibre to the home, which could fully utilise a one-to-one mapping for the WAN port? > Now, that would still force the user to configure two bridges in order > to properly steer traffic towards the requested ports but it would allow > us to be very flexible (which is probably desired here) in how ports are > grouped together. We want a sensible default, spreading the slave ports evenly over the CPU ports. We could add a devlink command to change the defaults at runtime. Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-31 0:06 ` Andrew Lunn @ 2017-05-31 0:16 ` Florian Fainelli 2017-05-31 0:52 ` Andrew Lunn 0 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-31 0:16 UTC (permalink / raw) To: Andrew Lunn Cc: John Crispin, Vivien Didelot, David S . Miller, Sean Wang, jiri, idosch, netdev, linux-kernel On 05/30/2017 05:06 PM, Andrew Lunn wrote: >> - past the initial setup, if we start creating bridge devices and so on, >> we have no way to tell: group Ports 0-3 together and send traffic to CPU >> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a >> DSA-only problem though, because we still have the CPU port(s) as >> independent network interfaces. > > What is the problem here? Frames come out the master interface, get > untagged and passed to the slave interface and go upto the bridge. It > should all just work. Same in the reverse direction. The problem is really that is you have multiple CPU ports, how do you define which one gets all the traffic by default? Ascending order of port number? Descending order? > > In order to make best use of the extra bandwidth of having two cpu > ports, i probably want the user ports reasonably evenly distributed > between the CPU ports. Dedicating one CPU port to one user port is > probably sub-optimal. How many people have 1Gbps Fibre to the home, > which could fully utilise a one-to-one mapping for the WAN port? I actually tend to think that most use cases our there are in the order of dedicating one CPU port to one corresponding switch port (user facing, or internal) in order to provided guaranteed bandwidth for that port. But as an user, I want to choose how the grouping is going to work, and right now, I cannot, unless this is hardcoded in Device Tree, which sounds both wrong and inadequate. > >> Now, that would still force the user to configure two bridges in order >> to properly steer traffic towards the requested ports but it would allow >> us to be very flexible (which is probably desired here) in how ports are >> grouped together. > > We want a sensible default, spreading the slave ports evenly over the > CPU ports. We could add a devlink command to change the defaults at > runtime. Sensible default is fine for the first time boot, but we should let users be entirely flexible in how they want their user-facing ports to map to a CPU port as you say, and IMHO using separate bridges to configure that is a possible way to go since there is already knowledge in the bridge join/leave code in DSA that already knows the dwnstream/user-facing ports, but does not yet know about CPU ports. Code speaks better, so let me see if I can cook something to illustrate this. Thanks! -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 2/3] net-next: dsa: add multi cpu port support 2017-05-31 0:16 ` Florian Fainelli @ 2017-05-31 0:52 ` Andrew Lunn 0 siblings, 0 replies; 19+ messages in thread From: Andrew Lunn @ 2017-05-31 0:52 UTC (permalink / raw) To: Florian Fainelli Cc: John Crispin, Vivien Didelot, David S . Miller, Sean Wang, jiri, idosch, netdev, linux-kernel On Tue, May 30, 2017 at 05:16:27PM -0700, Florian Fainelli wrote: > On 05/30/2017 05:06 PM, Andrew Lunn wrote: > >> - past the initial setup, if we start creating bridge devices and so on, > >> we have no way to tell: group Ports 0-3 together and send traffic to CPU > >> port 0, then let Port 5 alone and send traffic to CPU port 1, that's a > >> DSA-only problem though, because we still have the CPU port(s) as > >> independent network interfaces. > > > > What is the problem here? Frames come out the master interface, get > > untagged and passed to the slave interface and go upto the bridge. It > > should all just work. Same in the reverse direction. > > The problem is really that is you have multiple CPU ports, how do you > define which one gets all the traffic by default? Ascending order of > port number? Descending order? I would probably default to round robin when allocating user ports to CPU ports. That probably gives you the best default. > I actually tend to think that most use cases our there are in the order > of dedicating one CPU port to one corresponding switch port (user > facing, or internal) in order to provided guaranteed bandwidth for that > port. Which is generally a waste of bandwidth. Best case, i get 40Mbps Internet access. Meaning 960Mbps of a dedicated cpu port would be wasted. > But as an user, I want to choose how the grouping is going to > work, and right now, I cannot, unless this is hardcoded in Device Tree, > which sounds both wrong and inadequate. So how about round-robin default, and then devlink to move a user port to a specific cpu port? We also need to watch out for asymmetry. I think newer marvell chips don't support egress to multiple CPU ports. Ingress to the switch i think is unlimited. The older chips are more flexible in this respect. So we need some degree of flexibility here. Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 3/3] net-next: dsa: mt7530: add multi cpu port support 2017-05-30 10:44 [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding John Crispin 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin @ 2017-05-30 10:44 ` John Crispin 2017-05-30 21:32 ` [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding Florian Fainelli 2 siblings, 0 replies; 19+ messages in thread From: John Crispin @ 2017-05-30 10:44 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller, Sean Wang Cc: netdev, linux-kernel, John Crispin MT7530 switches have 2 CPU ports. Inside an MT7623a these are connected to GMAC1 and GMAC2. The code currently has the CPU hard coded to 6. Change this to using the new dsa_port_upstream_port() api. In case port 5 is not setup as a cpu port, we configure the RGMII passthrough mode allowing GMAC2 to connect to an external PHY. Signed-off-by: John Crispin <john@phrozen.org> --- drivers/net/dsa/mt7530.c | 45 +++++++++++++++++++++++++++++---------------- drivers/net/dsa/mt7530.h | 1 - 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 4d2f45153ede..370e0833474b 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -632,6 +632,9 @@ static int mt7530_cpu_port_enable(struct mt7530_priv *priv, int port) { + u8 port_mask = 0; + int i; + /* Enable Mediatek header mode on the cpu port */ mt7530_write(priv, MT7530_PVC_P(port), PORT_SPEC_TAG); @@ -648,8 +651,12 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv, /* CPU port gets connected to all user ports of * the switch */ + for (i = 0; i < MT7530_NUM_PORTS; i++) + if ((priv->ds->enabled_port_mask & BIT(i)) && + (dsa_port_upstream_port(priv->ds, i) == port)) + port_mask |= BIT(i); mt7530_write(priv, MT7530_PCR_P(port), - PCR_MATRIX(priv->ds->enabled_port_mask)); + PCR_MATRIX(port_mask)); return 0; } @@ -659,6 +666,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy) { struct mt7530_priv *priv = ds->priv; + u8 upstream = dsa_port_upstream_port(ds, port); mutex_lock(&priv->reg_mutex); @@ -669,7 +677,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port, * restore the port matrix if the port is the member of a certain * bridge. */ - priv->ports[port].pm |= PCR_MATRIX(BIT(MT7530_CPU_PORT)); + priv->ports[port].pm |= PCR_MATRIX(BIT(upstream)); priv->ports[port].enable = true; mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK, priv->ports[port].pm); @@ -732,7 +740,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *bridge) { struct mt7530_priv *priv = ds->priv; - u32 port_bitmap = BIT(MT7530_CPU_PORT); + u8 upstream = dsa_port_upstream_port(ds, port); + u32 port_bitmap = BIT(upstream); int i; mutex_lock(&priv->reg_mutex); @@ -770,6 +779,7 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *bridge) { struct mt7530_priv *priv = ds->priv; + u8 upstream = dsa_port_upstream_port(ds, port); int i; mutex_lock(&priv->reg_mutex); @@ -794,8 +804,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port, */ if (priv->ports[port].enable) mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK, - PCR_MATRIX(BIT(MT7530_CPU_PORT))); - priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT)); + PCR_MATRIX(BIT(upstream))); + priv->ports[port].pm = PCR_MATRIX(BIT(upstream)); mutex_unlock(&priv->reg_mutex); } @@ -892,15 +902,7 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port, static enum dsa_tag_protocol mtk_get_tag_protocol(struct dsa_switch *ds) { - struct mt7530_priv *priv = ds->priv; - - if (!dsa_is_cpu_port(ds, MT7530_CPU_PORT)) { - dev_warn(priv->dev, - "port not matched with tagging CPU port\n"); - return DSA_TAG_PROTO_NONE; - } else { - return DSA_TAG_PROTO_MTK; - } + return DSA_TAG_PROTO_MTK; } static int @@ -971,10 +973,21 @@ mt7530_setup(struct dsa_switch *ds) SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST | SYS_CTRL_REG_RST); - /* Enable Port 6 only; P5 as GMAC5 which currently is not supported */ + /* Enable Port 6. Port 5 is setup in passthrough mode if it is not a CPU + * port + */ val = mt7530_read(priv, MT7530_MHWTRAP); - val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS; + val &= ~MHWTRAP_P5_DIS & ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS; val |= MHWTRAP_MANUAL; + if (!dsa_is_cpu_port(ds, 5)) { + val |= MHWTRAP_P5_DIS; + val |= MHWTRAP_P5_MAC_SEL; + val |= MHWTRAP_P5_RGMII_MODE; + } else if (0) { + val &= ~MHWTRAP_P5_DIS; + val &= ~MHWTRAP_P5_MAC_SEL; + val &= ~MHWTRAP_P5_RGMII_MODE; + } mt7530_write(priv, MT7530_MHWTRAP, val); /* Enable and reset MIB counters */ diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index b83d76b99802..728e0c3a8883 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -15,7 +15,6 @@ #define __MT7530_H #define MT7530_NUM_PORTS 7 -#define MT7530_CPU_PORT 6 #define MT7530_NUM_FDB_RECORDS 2048 #define NUM_TRGMII_CTRL 5 -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-05-30 10:44 [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding John Crispin 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin 2017-05-30 10:44 ` [PATCH V2 3/3] net-next: dsa: mt7530: " John Crispin @ 2017-05-30 21:32 ` Florian Fainelli 2017-06-07 21:10 ` Rob Herring 2 siblings, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-05-30 21:32 UTC (permalink / raw) To: John Crispin, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang Cc: netdev, linux-kernel, Rob Herring, devicetree On 05/30/2017 03:44 AM, John Crispin wrote: > Extend the DSA binding documentation, adding the new property required > when there is more than one CPU port attached to the switch. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: John Crispin <john@phrozen.org> > --- > Documentation/devicetree/bindings/net/dsa/dsa.txt | 61 ++++++++++++++++++++++- > 1 file changed, 60 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt > index cfe8f64eca4f..c164eb38ccc5 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt > @@ -55,6 +55,11 @@ A user port has the following optional property: > - label : Describes the label associated with this port, which > will become the netdev name. > > +- cpu : Option for non "cpu"/"dsa" ports. A phandle to a > + "cpu" port, which will be used for passing packets > + from this port to the host. If not present, the first > + "cpu" port will be used. So this option essentially allow us to "partition" the switch between vectors of ports and their upstream/CPU port. While using Device Tree is an obvious choice for making the initial partitioning, it seems like we are missing a configuration mechanism whereby we can properly assign ports to a specific upstream CPU port. Let's move the actual discussion into patch 2 in order not to pollute the DT maintainers' inbox. > + > Port child nodes may also contain the following optional standardised > properties, described in binding documents: > > @@ -71,7 +76,7 @@ properties, described in binding documents: > Documentation/devicetree/bindings/net/fixed-link.txt > for details. > > -Example > +Examples > > The following example shows three switches on three MDIO busses, > linked into one DSA cluster. > @@ -264,6 +269,60 @@ linked into one DSA cluster. > }; > }; > > +The following example shows a switch that has two cpu ports each connecting > +to a different MAC. > + > +&mdio0 { > + switch@0 { > + compatible = "mediatek,mt7530"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + label = "lan0"; > + cpu = <&cpu_port1>; > + }; > + > + port@1 { > + reg = <1>; > + label = "lan1"; > + cpu = <&cpu_port1>; > + }; > + > + port@2 { > + reg = <2>; > + label = "lan2"; > + cpu = <&cpu_port1>; > + }; > + > + port@3 { > + reg = <3>; > + label = "wan"; > + cpu = <&cpu_port2>; > + }; > + > + cpu_port2: port@5 { > + reg = <5>; > + label = "cpu"; > + ethernet = <&gmac2>; > + phy-mode = "trgmii"; > + }; > + > + cpu_port1: port@6 { > + reg = <6>; > + label = "cpu"; > + ethernet = <&gmac1>; > + phy-mode = "trgmii"; > + }; > + }; > + }; > +}; > + > Deprecated Binding > ------------------ > > -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-05-30 21:32 ` [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding Florian Fainelli @ 2017-06-07 21:10 ` Rob Herring 2017-06-07 21:35 ` Florian Fainelli 2017-06-07 21:42 ` Andrew Lunn 0 siblings, 2 replies; 19+ messages in thread From: Rob Herring @ 2017-06-07 21:10 UTC (permalink / raw) To: Florian Fainelli Cc: John Crispin, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel, devicetree On Tue, May 30, 2017 at 02:32:29PM -0700, Florian Fainelli wrote: > On 05/30/2017 03:44 AM, John Crispin wrote: > > Extend the DSA binding documentation, adding the new property required > > when there is more than one CPU port attached to the switch. > > > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: devicetree@vger.kernel.org > > Signed-off-by: John Crispin <john@phrozen.org> > > --- > > Documentation/devicetree/bindings/net/dsa/dsa.txt | 61 ++++++++++++++++++++++- > > 1 file changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt > > index cfe8f64eca4f..c164eb38ccc5 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt > > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt > > @@ -55,6 +55,11 @@ A user port has the following optional property: > > - label : Describes the label associated with this port, which > > will become the netdev name. > > > > +- cpu : Option for non "cpu"/"dsa" ports. A phandle to a > > + "cpu" port, which will be used for passing packets > > + from this port to the host. If not present, the first > > + "cpu" port will be used. > > So this option essentially allow us to "partition" the switch between > vectors of ports and their upstream/CPU port. Could this be more generic? This is basically saying route all packets on this port to another port. Maybe there's some usecase to route to non-cpu ports? > While using Device Tree is an obvious choice for making the initial > partitioning, it seems like we are missing a configuration mechanism > whereby we can properly assign ports to a specific upstream CPU port. What determines how things are routed/partitioned? If it is purely user choice then I don't think this should be in DT. > Let's move the actual discussion into patch 2 in order not to pollute > the DT maintainers' inbox. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-06-07 21:10 ` Rob Herring @ 2017-06-07 21:35 ` Florian Fainelli 2017-06-08 19:31 ` Andrew Lunn 2017-06-07 21:42 ` Andrew Lunn 1 sibling, 1 reply; 19+ messages in thread From: Florian Fainelli @ 2017-06-07 21:35 UTC (permalink / raw) To: Rob Herring Cc: John Crispin, Andrew Lunn, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel, devicetree On 06/07/2017 02:10 PM, Rob Herring wrote: > On Tue, May 30, 2017 at 02:32:29PM -0700, Florian Fainelli wrote: >> On 05/30/2017 03:44 AM, John Crispin wrote: >>> Extend the DSA binding documentation, adding the new property required >>> when there is more than one CPU port attached to the switch. >>> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: devicetree@vger.kernel.org >>> Signed-off-by: John Crispin <john@phrozen.org> >>> --- >>> Documentation/devicetree/bindings/net/dsa/dsa.txt | 61 ++++++++++++++++++++++- >>> 1 file changed, 60 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt >>> index cfe8f64eca4f..c164eb38ccc5 100644 >>> --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt >>> +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt >>> @@ -55,6 +55,11 @@ A user port has the following optional property: >>> - label : Describes the label associated with this port, which >>> will become the netdev name. >>> >>> +- cpu : Option for non "cpu"/"dsa" ports. A phandle to a >>> + "cpu" port, which will be used for passing packets >>> + from this port to the host. If not present, the first >>> + "cpu" port will be used. >> >> So this option essentially allow us to "partition" the switch between >> vectors of ports and their upstream/CPU port. > > Could this be more generic? This is basically saying route all packets > on this port to another port. Maybe there's some usecase to route to > non-cpu ports? Yes, we absolutely want it to be more generic. The "problem" is that before an user has a chance to come in and type some commands that reconfigure the switch, there is a default state in which we need to assume one particular CPU/management port is going to be used to receive traffic. This can be pushed as a driver decision as to which of the multiple CPU ports is the most suitable IMHO. > >> While using Device Tree is an obvious choice for making the initial >> partitioning, it seems like we are missing a configuration mechanism >> whereby we can properly assign ports to a specific upstream CPU port. > > What determines how things are routed/partitioned? If it is purely user > choice then I don't think this should be in DT. Right now we don't have any mechanism, and statically doing this from Device Tree is too inflexible. I have been working on a parallel path where we use the bridge (which is already accelerated when there is a switch) in order to define groups of ports, the idea would be do to e.g: brctl addbr br-lan brctl addbr br-lan eth0 brctl addbr br-lan lan1 ... brctl addbr br-lan lan4 brctl addbr br-wan brctl addbr br-wan eth1 brctl addbr br-wan wan Assuming that lan1-lan4 are your LAN ports, and wan is your WAN port and you have two CPU ports. Until we configure these bridges though, we would be in the same state that we currently are, which is that traffic is only possible between lan1 <-> eth0, lan2 <-> eth0 and so on (every port is isolated from each other). -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-06-07 21:35 ` Florian Fainelli @ 2017-06-08 19:31 ` Andrew Lunn 2017-06-08 19:57 ` Florian Fainelli 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2017-06-08 19:31 UTC (permalink / raw) To: Florian Fainelli Cc: Rob Herring, John Crispin, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel, devicetree > Right now we don't have any mechanism, and statically doing this from > Device Tree is too inflexible. I have been working on a parallel path > where we use the bridge (which is already accelerated when there is a > switch) in order to define groups of ports, the idea would be do to e.g: > > brctl addbr br-lan > brctl addbr br-lan eth0 > brctl addbr br-lan lan1 > ... > brctl addbr br-lan lan4 > > brctl addbr br-wan > brctl addbr br-wan eth1 > brctl addbr br-wan wan > > Assuming that lan1-lan4 are your LAN ports, and wan is your WAN port and > you have two CPU ports. Hi Florian I don't like this, on multiple levels. My wan port typically never has more than 40Mbps of traffic on it. So dedicating a whole 1Gbps ethernet to it makes no sense. I want to share eth1 bandwidth with the wan port, and some of the other ports. Meaning i would have to add eth1 to br-lan as well as br-wan. Does the bridge allow that? And what sort of hacks do you have to allow a port to be added to a bridge, but not used by the bridge? And what is the point of br-wan? It only has one real port in it. So i'm adding per-packet overhead which i don't need, just in order to signal to the hardware how it should statically route cpu traffic for a port. Now say i have one of the bigger marvell switches, with 11 ports, in an industrial application. I setup 3 or 4 bridges. I then need to add eth0 and eth1 to two different bridges. And say i use some ports without a bridge. How do i configure them? And how do i dump the current mapping? For me, this is the wrong architecture. What CPU port is used should be a port property, not a bridge property. I think we should use devlink. Add commands to dump the current mapping, and set it. Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-06-08 19:31 ` Andrew Lunn @ 2017-06-08 19:57 ` Florian Fainelli 0 siblings, 0 replies; 19+ messages in thread From: Florian Fainelli @ 2017-06-08 19:57 UTC (permalink / raw) To: Andrew Lunn Cc: Rob Herring, John Crispin, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel, devicetree On 06/08/2017 12:31 PM, Andrew Lunn wrote: >> Right now we don't have any mechanism, and statically doing this from >> Device Tree is too inflexible. I have been working on a parallel path >> where we use the bridge (which is already accelerated when there is a >> switch) in order to define groups of ports, the idea would be do to e.g: >> >> brctl addbr br-lan >> brctl addbr br-lan eth0 >> brctl addbr br-lan lan1 >> ... >> brctl addbr br-lan lan4 >> >> brctl addbr br-wan >> brctl addbr br-wan eth1 >> brctl addbr br-wan wan >> >> Assuming that lan1-lan4 are your LAN ports, and wan is your WAN port and >> you have two CPU ports. > > Hi Florian > > I don't like this, on multiple levels. > > My wan port typically never has more than 40Mbps of traffic on it. So > dedicating a whole 1Gbps ethernet to it makes no sense. I want to > share eth1 bandwidth with the wan port, and some of the other > ports. Meaning i would have to add eth1 to br-lan as well as > br-wan. Does the bridge allow that? And what sort of hacks do you have > to allow a port to be added to a bridge, but not used by the bridge? This is AN example of how to configure a port grouping based on John's example and use case, not THE only example ;) The idea is obviously that you can define association between user-facing ports and CPU ports as you see fit, the idea is to use bridge to do that association because that's already what controls VLAN membership and forwarding. > > And what is the point of br-wan? It only has one real port in it. So > i'm adding per-packet overhead which i don't need, just in order to > signal to the hardware how it should statically route cpu traffic for > a port. No, it has two ports in it, adding eth1 is necessary do define the VLAN membership and forwarding rules, when eth1 is added we resolve the CPU port this corresponds to in the switch and we use that to define the forwarding decision between ports. The overhead per-packet is extremely limited because the first thing br_handle_frame() will do is see that eth1 is a DSA master network device and pass packets back up the stack for processing through dst->rcv(). > > Now say i have one of the bigger marvell switches, with 11 ports, in > an industrial application. I setup 3 or 4 bridges. I then need to add > eth0 and eth1 to two different bridges. And say i use some ports> without a bridge. How do i configure them? If you don't add your conduit interface to the bridge, then the default CPU interface (which could be left to the driver to decide which one is relevant) gets used and things work as expected. When you add a DSA master network device to the bridge, only then do we use that information to refine the forwarding decision and map to the appropriate port vectors. Doing this becomes necessary if you create a second (or more) bridge to isolate a group of ports. > > And how do i dump the current mapping? You look at both (or more) bridges' members, what's wrong or even any different from what happens today? > > For me, this is the wrong architecture. What CPU port is used should > be a port property, not a bridge property. I think we should use > devlink. Add commands to dump the current mapping, and set it. In premise I agree, although just like we need a bridge today to configure VLAN memberships, it did not seem to me like a big stretch to use a bridge to configure which CPU port you want. -- Florian ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding 2017-06-07 21:10 ` Rob Herring 2017-06-07 21:35 ` Florian Fainelli @ 2017-06-07 21:42 ` Andrew Lunn 1 sibling, 0 replies; 19+ messages in thread From: Andrew Lunn @ 2017-06-07 21:42 UTC (permalink / raw) To: Rob Herring Cc: Florian Fainelli, John Crispin, Vivien Didelot, David S . Miller, Sean Wang, netdev, linux-kernel, devicetree On Wed, Jun 07, 2017 at 04:10:02PM -0500, Rob Herring wrote: > On Tue, May 30, 2017 at 02:32:29PM -0700, Florian Fainelli wrote: > > On 05/30/2017 03:44 AM, John Crispin wrote: > > > Extend the DSA binding documentation, adding the new property required > > > when there is more than one CPU port attached to the switch. > > > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: devicetree@vger.kernel.org > > > Signed-off-by: John Crispin <john@phrozen.org> > > > --- > > > Documentation/devicetree/bindings/net/dsa/dsa.txt | 61 ++++++++++++++++++++++- > > > 1 file changed, 60 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt > > > index cfe8f64eca4f..c164eb38ccc5 100644 > > > --- a/Documentation/devicetree/bindings/net/dsa/dsa.txt > > > +++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt > > > @@ -55,6 +55,11 @@ A user port has the following optional property: > > > - label : Describes the label associated with this port, which > > > will become the netdev name. > > > > > > +- cpu : Option for non "cpu"/"dsa" ports. A phandle to a > > > + "cpu" port, which will be used for passing packets > > > + from this port to the host. If not present, the first > > > + "cpu" port will be used. > > > > So this option essentially allow us to "partition" the switch between > > vectors of ports and their upstream/CPU port. > > Could this be more generic? This is basically saying route all packets > on this port to another port. Hi Rob No, it is not saying that. The CPU port of the switch is special. It is used by the switch for frames it does not know what to do with. e.g, it has not learned the destination MAC address, it is an IGMP management packet, etc. Or the MAC address is that of the CPU, or the CPU needs to bridge it out another interface, e.g. a L2 VPN. The switch will add an additional header indicating what the ingress port was, and pass it to the CPU via this port. There is a presentation Florian, Vivien and I made at netdev 2.1 earlier this year which talks about this. If you want to mirror all packets from one port to another, you can use tc and the mirror action. > > While using Device Tree is an obvious choice for making the initial > > partitioning, it seems like we are missing a configuration mechanism > > whereby we can properly assign ports to a specific upstream CPU port. > > What determines how things are routed/partitioned? If it is purely user > choice then I don't think this should be in DT. > > Let's move the actual discussion into patch 2 in order not to pollute > > the DT maintainers' inbox. We are aiming to load balance traffic to/from the CPU and the switch. The ports of a switch can very in characteristics. Sometimes two are able to do 10Gbps, while others are just 1Gbps. So it would make sense to give those higher speed ports a bigger fraction of the available CPU bandwidth, etc. This binding gives us the option to start with a sensible default for a typical application of the hardware. For something like a WiFi box, this is probably sufficient. However, there is also a lot of usage of DSA in industrial application, and more flexibility is needed. For that we probably need a user API of some sort which allows the defaults in the device tree to be modified to a specific user case. Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-06-08 19:57 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-30 10:44 [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding John Crispin 2017-05-30 10:44 ` [PATCH V2 2/3] net-next: dsa: add multi cpu port support John Crispin 2017-05-30 15:38 ` kbuild test robot 2017-05-30 18:37 ` John Crispin 2017-05-30 19:15 ` Florian Fainelli 2017-05-30 19:23 ` Florian Fainelli 2017-05-30 19:45 ` Vivien Didelot 2017-05-30 19:50 ` Vivien Didelot 2017-05-30 22:56 ` Florian Fainelli 2017-05-31 0:06 ` Andrew Lunn 2017-05-31 0:16 ` Florian Fainelli 2017-05-31 0:52 ` Andrew Lunn 2017-05-30 10:44 ` [PATCH V2 3/3] net-next: dsa: mt7530: " John Crispin 2017-05-30 21:32 ` [PATCH V2 1/3] Documentation: devicetree: add multiple cpu port DSA binding Florian Fainelli 2017-06-07 21:10 ` Rob Herring 2017-06-07 21:35 ` Florian Fainelli 2017-06-08 19:31 ` Andrew Lunn 2017-06-08 19:57 ` Florian Fainelli 2017-06-07 21:42 ` Andrew Lunn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).