* [PATCH net-next v4 0/3] Arrow SpeedChips XRS700x DSA Driver
@ 2021-01-13 14:59 George McCollister
2021-01-13 14:59 ` [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer George McCollister
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: George McCollister @ 2021-01-13 14:59 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: Rob Herring, David S . Miller, netdev, devicetree, George McCollister
This series adds a DSA driver for the Arrow SpeedChips XRS 7000 series
of HSR/PRP gigabit switch chips.
The chips use Flexibilis IP.
More information can be found here:
https://www.flexibilis.com/products/speedchips-xrs7000/
The switches have up to three RGMII ports and one MII port and are
managed via mdio or i2c. They use a one byte trailing tag to identify
the switch port when in managed mode so I've added a tag driver which
implements this.
This series contains minimal DSA functionality which may be built upon
in future patches. The ultimate goal is to add HSR and PRP
(IEC 62439-3 Clause 5 & 4) offloading with integration into net/hsr.
Changes since v1:
* Use central TX reallocation in tag driver. (Andrew Lunn)
* Code style fixes. (Andrew Lunn, Vladimir Oltean)
* Code simplifications. (Andrew Lunn, Vladimir Oltean)
* Verify detected switch matches compatible. (Andrew Lunn)
* Add inbound policy to allow BPDUs. (Andrew Lunn)
* Move files into their own subdir. (Vladimir Oltean)
* Automate regmap field allocation. (Vladimir Oltean)
* Move setting link speed to .mac_link_up. (Vladimir Oltean)
* Use different compatible strings for e/f variants.
Changes since v2:
* Export constant xrs700x_info symbols. (Jakub Kicinski)
* Report stats via .get_stats64. (Jakub Kicinski, Vladimir Oltean)
* Use a 3 second polling rate for counters.
Changes since v3:
* Builds against net-next now that get_stats64 commit has been merged.
* Don't show status in devicetree examples. (Rob Herring)
* Use ethernet-port(s) in devicetree examples. (Rob Herring)
* Use strscpy() instead of strlcpy().
George McCollister (3):
dsa: add support for Arrow XRS700x tag trailer
net: dsa: add Arrow SpeedChips XRS700x driver
dt-bindings: net: dsa: add bindings for xrs700x switches
.../devicetree/bindings/net/dsa/arrow,xrs700x.yaml | 73 +++
drivers/net/dsa/Kconfig | 2 +
drivers/net/dsa/Makefile | 1 +
drivers/net/dsa/xrs700x/Kconfig | 26 +
drivers/net/dsa/xrs700x/Makefile | 4 +
drivers/net/dsa/xrs700x/xrs700x.c | 629 +++++++++++++++++++++
drivers/net/dsa/xrs700x/xrs700x.h | 42 ++
drivers/net/dsa/xrs700x/xrs700x_i2c.c | 150 +++++
drivers/net/dsa/xrs700x/xrs700x_mdio.c | 162 ++++++
drivers/net/dsa/xrs700x/xrs700x_reg.h | 205 +++++++
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +
net/dsa/Makefile | 1 +
net/dsa/tag_xrs700x.c | 67 +++
14 files changed, 1370 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml
create mode 100644 drivers/net/dsa/xrs700x/Kconfig
create mode 100644 drivers/net/dsa/xrs700x/Makefile
create mode 100644 drivers/net/dsa/xrs700x/xrs700x.c
create mode 100644 drivers/net/dsa/xrs700x/xrs700x.h
create mode 100644 drivers/net/dsa/xrs700x/xrs700x_i2c.c
create mode 100644 drivers/net/dsa/xrs700x/xrs700x_mdio.c
create mode 100644 drivers/net/dsa/xrs700x/xrs700x_reg.h
create mode 100644 net/dsa/tag_xrs700x.c
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer 2021-01-13 14:59 [PATCH net-next v4 0/3] Arrow SpeedChips XRS700x DSA Driver George McCollister @ 2021-01-13 14:59 ` George McCollister 2021-01-14 1:05 ` Vladimir Oltean 2021-01-13 14:59 ` [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 3/3] dt-bindings: net: dsa: add bindings for xrs700x switches George McCollister 2 siblings, 1 reply; 16+ messages in thread From: George McCollister @ 2021-01-13 14:59 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean Cc: Rob Herring, David S . Miller, netdev, devicetree, George McCollister Add support for Arrow SpeedChips XRS700x single byte tag trailer. This is modeled on tag_trailer.c which works in a similar way. Signed-off-by: George McCollister <george.mccollister@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> --- include/net/dsa.h | 2 ++ net/dsa/Kconfig | 6 +++++ net/dsa/Makefile | 1 + net/dsa/tag_xrs700x.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 net/dsa/tag_xrs700x.c diff --git a/include/net/dsa.h b/include/net/dsa.h index c3485ba6c312..74b5bf835657 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -46,6 +46,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_AR9331_VALUE 16 #define DSA_TAG_PROTO_RTL4_A_VALUE 17 #define DSA_TAG_PROTO_HELLCREEK_VALUE 18 +#define DSA_TAG_PROTO_XRS700X_VALUE 19 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -67,6 +68,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE, DSA_TAG_PROTO_RTL4_A = DSA_TAG_PROTO_RTL4_A_VALUE, DSA_TAG_PROTO_HELLCREEK = DSA_TAG_PROTO_HELLCREEK_VALUE, + DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, }; struct packet_type; diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index dfecd7b22fd7..2d226a5c085f 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -139,4 +139,10 @@ config NET_DSA_TAG_TRAILER Say Y or M if you want to enable support for tagging frames at with a trailed. e.g. Marvell 88E6060. +config NET_DSA_TAG_XRS700X + tristate "Tag driver for XRS700x switches" + help + Say Y or M if you want to enable support for tagging frames for + Arrow SpeedChips XRS700x switches that use a single byte tag trailer. + endif diff --git a/net/dsa/Makefile b/net/dsa/Makefile index 0fb2b75a7ae3..92cea2132241 100644 --- a/net/dsa/Makefile +++ b/net/dsa/Makefile @@ -18,3 +18,4 @@ obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o +obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c new file mode 100644 index 000000000000..4ee7c260a8a9 --- /dev/null +++ b/net/dsa/tag_xrs700x.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * XRS700x tag format handling + * Copyright (c) 2008-2009 Marvell Semiconductor + * Copyright (c) 2020 NovaTech LLC + */ + +#include <linux/etherdevice.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/bitops.h> + +#include "dsa_priv.h" + +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + u8 *trailer; + + trailer = skb_put(skb, 1); + trailer[0] = BIT(dp->index); + + return skb; +} + +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, + struct packet_type *pt) +{ + int source_port; + u8 *trailer; + + if (skb_linearize(skb)) + return NULL; + + trailer = skb_tail_pointer(skb) - 1; + + source_port = ffs((int)trailer[0]) - 1; + + if (source_port < 0) + return NULL; + + skb->dev = dsa_master_find_slave(dev, 0, source_port); + if (!skb->dev) + return NULL; + + if (pskb_trim_rcsum(skb, skb->len - 1)) + return NULL; + + /* Frame is forwarded by hardware, don't forward in software. */ + skb->offload_fwd_mark = 1; + + return skb; +} + +static const struct dsa_device_ops xrs700x_netdev_ops = { + .name = "xrs700x", + .proto = DSA_TAG_PROTO_XRS700X, + .xmit = xrs700x_xmit, + .rcv = xrs700x_rcv, + .overhead = 1, + .tail_tag = true, +}; + +MODULE_LICENSE("GPL"); +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_XRS700X); + +module_dsa_tag_driver(xrs700x_netdev_ops); -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer 2021-01-13 14:59 ` [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer George McCollister @ 2021-01-14 1:05 ` Vladimir Oltean 2021-01-14 1:48 ` Andrew Lunn 2021-01-14 15:03 ` George McCollister 0 siblings, 2 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 1:05 UTC (permalink / raw) To: George McCollister Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, devicetree On Wed, Jan 13, 2021 at 08:59:20AM -0600, George McCollister wrote: > Add support for Arrow SpeedChips XRS700x single byte tag trailer. This > is modeled on tag_trailer.c which works in a similar way. > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> A few comments below. > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > new file mode 100644 > index 000000000000..4ee7c260a8a9 > --- /dev/null > +++ b/net/dsa/tag_xrs700x.c > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * XRS700x tag format handling > + * Copyright (c) 2008-2009 Marvell Semiconductor Why does Marvell get copyright? > + * Copyright (c) 2020 NovaTech LLC > + */ > + > +#include <linux/etherdevice.h> > +#include <linux/list.h> > +#include <linux/slab.h> These 3 includes are not needed. You can probably remove them later though, if there is no other reason to resend. > +#include <linux/bitops.h> > + > +#include "dsa_priv.h" > + > +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + u8 *trailer; > + > + trailer = skb_put(skb, 1); > + trailer[0] = BIT(dp->index); > + > + return skb; > +} > + > +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > + struct packet_type *pt) > +{ > + int source_port; > + u8 *trailer; > + > + if (skb_linearize(skb)) > + return NULL; We've been through this, there should be no reason to linearize an skb for a one-byte tail tag.. > + > + trailer = skb_tail_pointer(skb) - 1; > + > + source_port = ffs((int)trailer[0]) - 1; > + > + if (source_port < 0) > + return NULL; > + > + skb->dev = dsa_master_find_slave(dev, 0, source_port); > + if (!skb->dev) > + return NULL; > + > + if (pskb_trim_rcsum(skb, skb->len - 1)) > + return NULL; > + > + /* Frame is forwarded by hardware, don't forward in software. */ > + skb->offload_fwd_mark = 1; > + > + return skb; > +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer 2021-01-14 1:05 ` Vladimir Oltean @ 2021-01-14 1:48 ` Andrew Lunn 2021-01-14 15:03 ` George McCollister 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2021-01-14 1:48 UTC (permalink / raw) To: Vladimir Oltean Cc: George McCollister, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, devicetree On Thu, Jan 14, 2021 at 03:05:19AM +0200, Vladimir Oltean wrote: > On Wed, Jan 13, 2021 at 08:59:20AM -0600, George McCollister wrote: > > Add support for Arrow SpeedChips XRS700x single byte tag trailer. This > > is modeled on tag_trailer.c which works in a similar way. > > > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > A few comments below. > > > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > > new file mode 100644 > > index 000000000000..4ee7c260a8a9 > > --- /dev/null > > +++ b/net/dsa/tag_xrs700x.c > > @@ -0,0 +1,67 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * XRS700x tag format handling > > + * Copyright (c) 2008-2009 Marvell Semiconductor > > Why does Marvell get copyright? Probably because it started life as tag_trailer.c? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer 2021-01-14 1:05 ` Vladimir Oltean 2021-01-14 1:48 ` Andrew Lunn @ 2021-01-14 15:03 ` George McCollister 1 sibling, 0 replies; 16+ messages in thread From: George McCollister @ 2021-01-14 15:03 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Wed, Jan 13, 2021 at 7:05 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > +++ b/net/dsa/tag_xrs700x.c > > @@ -0,0 +1,67 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * XRS700x tag format handling > > + * Copyright (c) 2008-2009 Marvell Semiconductor > > Why does Marvell get copyright? What Andrew said. I started with tag_trailer.c since it is quite similar and it seemed wrong to remove the copyright. > > > + * Copyright (c) 2020 NovaTech LLC > > + */ > > + > > +#include <linux/etherdevice.h> > > +#include <linux/list.h> > > +#include <linux/slab.h> > > These 3 includes are not needed. You can probably remove them later > though, if there is no other reason to resend. Removed. > > > +#include <linux/bitops.h> > > + > > +#include "dsa_priv.h" > > + > > +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) > > +{ > > + struct dsa_port *dp = dsa_slave_to_port(dev); > > + u8 *trailer; > > + > > + trailer = skb_put(skb, 1); > > + trailer[0] = BIT(dp->index); > > + > > + return skb; > > +} > > + > > +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > > + struct packet_type *pt) > > +{ > > + int source_port; > > + u8 *trailer; > > + > > + if (skb_linearize(skb)) > > + return NULL; > > We've been through this, there should be no reason to linearize an skb > for a one-byte tail tag.. Sorry about this. You and Andrew started discussing it and I guess I got distracted fixing the other issues. Removed. I'll retest after making other changes to patches in the series but based on what you've said it should be fine without it. > > > + > > + trailer = skb_tail_pointer(skb) - 1; > > + > > + source_port = ffs((int)trailer[0]) - 1; > > + > > + if (source_port < 0) > > + return NULL; > > + > > + skb->dev = dsa_master_find_slave(dev, 0, source_port); > > + if (!skb->dev) > > + return NULL; > > + > > + if (pskb_trim_rcsum(skb, skb->len - 1)) > > + return NULL; > > + > > + /* Frame is forwarded by hardware, don't forward in software. */ > > + skb->offload_fwd_mark = 1; > > + > > + return skb; > > +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-13 14:59 [PATCH net-next v4 0/3] Arrow SpeedChips XRS700x DSA Driver George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer George McCollister @ 2021-01-13 14:59 ` George McCollister 2021-01-14 1:56 ` Vladimir Oltean 2021-01-14 17:28 ` Florian Fainelli 2021-01-13 14:59 ` [PATCH net-next v4 3/3] dt-bindings: net: dsa: add bindings for xrs700x switches George McCollister 2 siblings, 2 replies; 16+ messages in thread From: George McCollister @ 2021-01-13 14:59 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean Cc: Rob Herring, David S . Miller, netdev, devicetree, George McCollister Add a driver with initial support for the Arrow SpeedChips XRS7000 series of gigabit Ethernet switch chips which are typically used in critical networking applications. The switches have up to three RGMII ports and one RMII port. Management to the switches can be performed over i2c or mdio. Support for advanced features such as PTP and HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and may be added at a later date. Signed-off-by: George McCollister <george.mccollister@gmail.com> --- drivers/net/dsa/Kconfig | 2 + drivers/net/dsa/Makefile | 1 + drivers/net/dsa/xrs700x/Kconfig | 26 ++ drivers/net/dsa/xrs700x/Makefile | 4 + drivers/net/dsa/xrs700x/xrs700x.c | 629 +++++++++++++++++++++++++++++++++ drivers/net/dsa/xrs700x/xrs700x.h | 42 +++ drivers/net/dsa/xrs700x/xrs700x_i2c.c | 150 ++++++++ drivers/net/dsa/xrs700x/xrs700x_mdio.c | 162 +++++++++ drivers/net/dsa/xrs700x/xrs700x_reg.h | 205 +++++++++++ 9 files changed, 1221 insertions(+) create mode 100644 drivers/net/dsa/xrs700x/Kconfig create mode 100644 drivers/net/dsa/xrs700x/Makefile create mode 100644 drivers/net/dsa/xrs700x/xrs700x.c create mode 100644 drivers/net/dsa/xrs700x/xrs700x.h create mode 100644 drivers/net/dsa/xrs700x/xrs700x_i2c.c create mode 100644 drivers/net/dsa/xrs700x/xrs700x_mdio.c create mode 100644 drivers/net/dsa/xrs700x/xrs700x_reg.h diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig index f6a0488589fc..3af373e90806 100644 --- a/drivers/net/dsa/Kconfig +++ b/drivers/net/dsa/Kconfig @@ -60,6 +60,8 @@ source "drivers/net/dsa/qca/Kconfig" source "drivers/net/dsa/sja1105/Kconfig" +source "drivers/net/dsa/xrs700x/Kconfig" + config NET_DSA_QCA8K tristate "Qualcomm Atheros QCA8K Ethernet switch family support" depends on NET_DSA diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile index a84adb140a04..f3598c040994 100644 --- a/drivers/net/dsa/Makefile +++ b/drivers/net/dsa/Makefile @@ -24,3 +24,4 @@ obj-y += mv88e6xxx/ obj-y += ocelot/ obj-y += qca/ obj-y += sja1105/ +obj-y += xrs700x/ diff --git a/drivers/net/dsa/xrs700x/Kconfig b/drivers/net/dsa/xrs700x/Kconfig new file mode 100644 index 000000000000..d10a4dce1676 --- /dev/null +++ b/drivers/net/dsa/xrs700x/Kconfig @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: GPL-2.0-only +config NET_DSA_XRS700X + tristate + depends on NET_DSA + select NET_DSA_TAG_XRS700X + select REGMAP + help + This enables support for Arrow SpeedChips XRS7003/7004 gigabit + Ethernet switches. + +config NET_DSA_XRS700X_I2C + tristate "Arrow XRS7000X series switch in I2C mode" + depends on NET_DSA && I2C + select NET_DSA_XRS700X + select REGMAP_I2C + help + Enable I2C support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet + switches. + +config NET_DSA_XRS700X_MDIO + tristate "Arrow XRS7000X series switch in MDIO mode" + depends on NET_DSA + select NET_DSA_XRS700X + help + Enable MDIO support for Arrow SpeedChips XRS7003/7004 gigabit Ethernet + switches. diff --git a/drivers/net/dsa/xrs700x/Makefile b/drivers/net/dsa/xrs700x/Makefile new file mode 100644 index 000000000000..51a3a7d9296a --- /dev/null +++ b/drivers/net/dsa/xrs700x/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_NET_DSA_XRS700X) += xrs700x.o +obj-$(CONFIG_NET_DSA_XRS700X_I2C) += xrs700x_i2c.o +obj-$(CONFIG_NET_DSA_XRS700X_MDIO) += xrs700x_mdio.o diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c new file mode 100644 index 000000000000..cb1e711881cb --- /dev/null +++ b/drivers/net/dsa/xrs700x/xrs700x.c @@ -0,0 +1,629 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 NovaTech LLC + * George McCollister <george.mccollister@gmail.com> + */ + +#include <net/dsa.h> +#include <linux/if_bridge.h> +#include <linux/of_device.h> +#include "xrs700x.h" +#include "xrs700x_reg.h" + +#define XRS700X_MIB_INTERVAL msecs_to_jiffies(3000) + +#define XRS7003E_ID 0x100 +#define XRS7003F_ID 0x101 +#define XRS7004E_ID 0x200 +#define XRS7004F_ID 0x201 + +const struct xrs700x_info xrs7003e_info = {XRS7003E_ID, "XRS7003E", 3}; +EXPORT_SYMBOL(xrs7003e_info); + +const struct xrs700x_info xrs7003f_info = {XRS7003F_ID, "XRS7003F", 3}; +EXPORT_SYMBOL(xrs7003f_info); + +const struct xrs700x_info xrs7004e_info = {XRS7004E_ID, "XRS7004E", 4}; +EXPORT_SYMBOL(xrs7004e_info); + +const struct xrs700x_info xrs7004f_info = {XRS7004F_ID, "XRS7004F", 4}; +EXPORT_SYMBOL(xrs7004f_info); + +struct xrs700x_regfield { + struct reg_field rf; + struct regmap_field **rmf; +}; + +struct xrs700x_mib { + unsigned int offset; + const char *name; + int stats64_offset; +}; + +#define XRS700X_MIB_ETHTOOL_ONLY(o, n) {o, n, -1} +#define XRS700X_MIB(o, n, m) {o, n, offsetof(struct rtnl_link_stats64, m)} + +static const struct xrs700x_mib xrs700x_mibs[] = { + XRS700X_MIB(XRS_RX_GOOD_OCTETS_L, "rx_good_octets", rx_bytes), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_BAD_OCTETS_L, "rx_bad_octets"), + XRS700X_MIB(XRS_RX_UNICAST_L, "rx_unicast", rx_packets), + XRS700X_MIB(XRS_RX_BROADCAST_L, "rx_broadcast", rx_packets), + XRS700X_MIB(XRS_RX_MULTICAST_L, "rx_multicast", multicast), + XRS700X_MIB(XRS_RX_UNDERSIZE_L, "rx_undersize", rx_length_errors), + XRS700X_MIB(XRS_RX_FRAGMENTS_L, "rx_fragments", rx_length_errors), + XRS700X_MIB(XRS_RX_OVERSIZE_L, "rx_oversize", rx_length_errors), + XRS700X_MIB(XRS_RX_JABBER_L, "rx_jabber", rx_length_errors), + XRS700X_MIB(XRS_RX_ERR_L, "rx_err", rx_errors), + XRS700X_MIB(XRS_RX_CRC_L, "rx_crc", rx_crc_errors), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_64_L, "rx_64"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_65_127_L, "rx_65_127"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_128_255_L, "rx_128_255"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_256_511_L, "rx_256_511"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_512_1023_L, "rx_512_1023"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_1024_1536_L, "rx_1024_1536"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_HSR_PRP_L, "rx_hsr_prp"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_WRONGLAN_L, "rx_wronglan"), + XRS700X_MIB_ETHTOOL_ONLY(XRS_RX_DUPLICATE_L, "rx_duplicate"), + XRS700X_MIB(XRS_TX_OCTETS_L, "tx_octets", tx_bytes), + XRS700X_MIB(XRS_TX_UNICAST_L, "tx_unicast", tx_packets), + XRS700X_MIB(XRS_TX_BROADCAST_L, "tx_broadcast", tx_packets), + XRS700X_MIB(XRS_TX_MULTICAST_L, "tx_multicast", tx_packets), + XRS700X_MIB_ETHTOOL_ONLY(XRS_TX_HSR_PRP_L, "tx_hsr_prp"), + XRS700X_MIB(XRS_PRIQ_DROP_L, "priq_drop", tx_dropped), + XRS700X_MIB(XRS_EARLY_DROP_L, "early_drop", tx_dropped), +}; + +static void xrs700x_get_strings(struct dsa_switch *ds, int port, + u32 stringset, uint8_t *data) +{ + int i; + + if (stringset != ETH_SS_STATS) + return; + + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) { + strscpy(data, xrs700x_mibs[i].name, ETH_GSTRING_LEN); + data += ETH_GSTRING_LEN; + } +} + +static int xrs700x_get_sset_count(struct dsa_switch *ds, int port, int sset) +{ + if (sset != ETH_SS_STATS) + return -EOPNOTSUPP; + + return ARRAY_SIZE(xrs700x_mibs); +} + +static void xrs700x_read_port_counters(struct xrs700x *priv, int port) +{ + struct xrs700x_port *p = &priv->ports[port]; + struct rtnl_link_stats64 stats; + int i; + + memset(&stats, 0, sizeof(stats)); + + mutex_lock(&p->mib_mutex); + + /* Capture counter values */ + regmap_write(priv->regmap, XRS_CNT_CTRL(port), 1); + + for (i = 0; i < ARRAY_SIZE(xrs700x_mibs); i++) { + unsigned int high = 0, low = 0, reg; + + reg = xrs700x_mibs[i].offset + XRS_PORT_OFFSET * port; + regmap_read(priv->regmap, reg, &low); + regmap_read(priv->regmap, reg + 2, &high); + + p->mib_data[i] += (high << 16) | low; + + if (xrs700x_mibs[i].stats64_offset >= 0) { + u8 *s = (u8 *)&stats + xrs700x_mibs[i].stats64_offset; + *(u64 *)s += p->mib_data[i]; + } + } + + /* multicast must be added to rx_packets (which already includes + * unicast and broadcast) + */ + stats.rx_packets += stats.multicast; + + u64_stats_update_begin(&p->syncp); + p->stats64 = stats; + u64_stats_update_end(&p->syncp); + + mutex_unlock(&p->mib_mutex); +} + +static void xrs700x_mib_work(struct work_struct *work) +{ + struct xrs700x *priv = container_of(work, struct xrs700x, + mib_work.work); + int i; + + for (i = 0; i < priv->ds->num_ports; i++) + xrs700x_read_port_counters(priv, i); + + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL); +} + +static void xrs700x_get_ethtool_stats(struct dsa_switch *ds, int port, + uint64_t *data) +{ + struct xrs700x *priv = ds->priv; + struct xrs700x_port *p = &priv->ports[port]; + + xrs700x_read_port_counters(priv, port); + + mutex_lock(&p->mib_mutex); + memcpy(data, p->mib_data, sizeof(*data) * ARRAY_SIZE(xrs700x_mibs)); + mutex_unlock(&p->mib_mutex); +} + +static void xrs700x_get_stats64(struct dsa_switch *ds, int port, + struct rtnl_link_stats64 *s) +{ + struct xrs700x *priv = ds->priv; + struct xrs700x_port *p = &priv->ports[port]; + unsigned int start; + + do { + start = u64_stats_fetch_begin(&p->syncp); + *s = p->stats64; + } while (u64_stats_fetch_retry(&p->syncp, start)); +} + +static int xrs700x_setup_regmap_range(struct xrs700x *priv) +{ + struct xrs700x_regfield regfields[] = { + { + .rf = REG_FIELD_ID(XRS_PORT_STATE(0), 0, 1, + priv->ds->num_ports, + XRS_PORT_OFFSET), + .rmf = &priv->ps_forward + }, + { + .rf = REG_FIELD_ID(XRS_PORT_STATE(0), 2, 3, + priv->ds->num_ports, + XRS_PORT_OFFSET), + .rmf = &priv->ps_management + }, + { + .rf = REG_FIELD_ID(XRS_PORT_STATE(0), 4, 9, + priv->ds->num_ports, + XRS_PORT_OFFSET), + .rmf = &priv->ps_sel_speed + }, + { + .rf = REG_FIELD_ID(XRS_PORT_STATE(0), 10, 11, + priv->ds->num_ports, + XRS_PORT_OFFSET), + .rmf = &priv->ps_cur_speed + } + }; + int i = 0; + + for (; i < ARRAY_SIZE(regfields); i++) { + *regfields[i].rmf = devm_regmap_field_alloc(priv->dev, + priv->regmap, + regfields[i].rf); + if (IS_ERR(*regfields[i].rmf)) + return PTR_ERR(*regfields[i].rmf); + } + + return 0; +} + +static enum dsa_tag_protocol xrs700x_get_tag_protocol(struct dsa_switch *ds, + int port, + enum dsa_tag_protocol m) +{ + return DSA_TAG_PROTO_XRS700X; +} + +static int xrs700x_reset(struct dsa_switch *ds) +{ + struct xrs700x *priv = ds->priv; + unsigned int val; + int ret; + + ret = regmap_write(priv->regmap, XRS_GENERAL, XRS_GENERAL_RESET); + if (ret) + goto error; + + ret = regmap_read_poll_timeout(priv->regmap, XRS_GENERAL, + val, !(val & XRS_GENERAL_RESET), + 10, 1000); +error: + if (ret) { + dev_err_ratelimited(priv->dev, "error resetting switch: %d\n", + ret); + } + + return ret; +} + +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port, + u8 state) +{ + struct xrs700x *priv = ds->priv; + unsigned int bpdus = 1; + unsigned int val; + + switch (state) { + case BR_STATE_DISABLED: + bpdus = 0; + fallthrough; + case BR_STATE_BLOCKING: + case BR_STATE_LISTENING: + val = XRS_PORT_DISABLED; + break; + case BR_STATE_LEARNING: + val = XRS_PORT_LEARNING; + break; + case BR_STATE_FORWARDING: + val = XRS_PORT_FORWARDING; + break; + default: + dev_err(ds->dev, "invalid STP state: %d\n", state); + return; + } + + regmap_fields_write(priv->ps_forward, port, val); + + /* Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf() + * which allows BPDU forwarding to the CPU port when the front facing + * port is in disabled/learning state. + */ + regmap_update_bits(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 1, bpdus); + + dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n", + __func__, port, state, val); +} + +/* Add an inbound policy filter which matches the BPDU destination MAC + * and forwards to the CPU port. Leave the policy disabled, it will be + * enabled as needed. + */ +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port) +{ + struct xrs700x *priv = ds->priv; + unsigned int val = 0; + int i = 0; + int ret; + + /* Compare all 48 bits of the destination MAC address. */ + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2); + if (ret) + return ret; + + /* match BPDU destination 01:80:c2:00:00:00 */ + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0); + if (ret) + return ret; + + /* Mirror BPDU to CPU port */ + for (; i < ds->num_ports; i++) { + if (dsa_is_cpu_port(ds, i)) + val |= BIT(i); + } + + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_FWD_MIRROR(port, 0), val); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_FWD_ALLOW(port, 0), 0); + if (ret) + return ret; + + return 0; +} + +static int xrs700x_port_setup(struct dsa_switch *ds, int port) +{ + bool cpu_port = dsa_is_cpu_port(ds, port); + struct xrs700x *priv = ds->priv; + unsigned int val = 0; + int ret, i; + + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED); + + /* Disable forwarding to non-CPU ports */ + for (i = 0; i < ds->num_ports; i++) { + if (!dsa_is_cpu_port(ds, i)) + val |= BIT(i); + } + + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val); + if (ret) + return ret; + + val = cpu_port ? XRS_PORT_MODE_MANAGEMENT : XRS_PORT_MODE_NORMAL; + ret = regmap_fields_write(priv->ps_management, port, val); + if (ret) + return ret; + + if (!cpu_port) { + ret = xrs700x_port_add_bpdu_ipf(ds, port); + if (ret) + return ret; + } + + return 0; +} + +static int xrs700x_setup(struct dsa_switch *ds) +{ + struct xrs700x *priv = ds->priv; + int ret, i; + + ret = xrs700x_reset(ds); + if (ret) + return ret; + + for (i = 0; i < ds->num_ports; i++) { + ret = xrs700x_port_setup(ds, i); + if (ret) + return ret; + } + + schedule_delayed_work(&priv->mib_work, XRS700X_MIB_INTERVAL); + + return 0; +} + +static void xrs700x_teardown(struct dsa_switch *ds) +{ + struct xrs700x *priv = ds->priv; + + cancel_delayed_work_sync(&priv->mib_work); +} + +static void xrs700x_phylink_validate(struct dsa_switch *ds, int port, + unsigned long *supported, + struct phylink_link_state *state) +{ + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; + + switch (port) { + case 0: + break; + case 1: + case 2: + case 3: + phylink_set(mask, 1000baseT_Full); + break; + default: + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); + dev_err(ds->dev, "Unsupported port: %i\n", port); + return; + } + + phylink_set_port_modes(mask); + + /* The switch only supports full duplex. */ + phylink_set(mask, 10baseT_Full); + phylink_set(mask, 100baseT_Full); + + bitmap_and(supported, supported, mask, + __ETHTOOL_LINK_MODE_MASK_NBITS); + bitmap_and(state->advertising, state->advertising, mask, + __ETHTOOL_LINK_MODE_MASK_NBITS); +} + +static void xrs700x_mac_link_up(struct dsa_switch *ds, int port, + unsigned int mode, phy_interface_t interface, + struct phy_device *phydev, + int speed, int duplex, + bool tx_pause, bool rx_pause) +{ + struct xrs700x *priv = ds->priv; + unsigned int val; + + switch (speed) { + case SPEED_1000: + val = XRS_PORT_SPEED_1000; + break; + case SPEED_100: + val = XRS_PORT_SPEED_100; + break; + case SPEED_10: + val = XRS_PORT_SPEED_10; + break; + default: + return; + } + + regmap_fields_write(priv->ps_sel_speed, port, val); + + dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n", + __func__, port, mode, speed); +} + +static int xrs700x_bridge_common(struct dsa_switch *ds, int port, + struct net_device *bridge, bool join) +{ + unsigned int i, cpu_mask = 0, mask = 0; + struct xrs700x *priv = ds->priv; + int ret; + + for (i = 0; i < ds->num_ports; i++) { + if (dsa_is_cpu_port(ds, i)) + continue; + + cpu_mask |= BIT(i); + + if (dsa_to_port(ds, i)->bridge_dev == bridge) + continue; + + mask |= BIT(i); + } + + for (i = 0; i < ds->num_ports; i++) { + if (dsa_to_port(ds, i)->bridge_dev != bridge) + continue; + + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask); + if (ret) + return ret; + } + + if (!join) { + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), + cpu_mask); + if (ret) + return ret; + } + + return 0; +} + +static int xrs700x_bridge_join(struct dsa_switch *ds, int port, + struct net_device *bridge) +{ + return xrs700x_bridge_common(ds, port, bridge, true); +} + +static void xrs700x_bridge_leave(struct dsa_switch *ds, int port, + struct net_device *bridge) +{ + xrs700x_bridge_common(ds, port, bridge, false); +} + +static const struct dsa_switch_ops xrs700x_ops = { + .get_tag_protocol = xrs700x_get_tag_protocol, + .setup = xrs700x_setup, + .teardown = xrs700x_teardown, + .port_stp_state_set = xrs700x_port_stp_state_set, + .phylink_validate = xrs700x_phylink_validate, + .phylink_mac_link_up = xrs700x_mac_link_up, + .get_strings = xrs700x_get_strings, + .get_sset_count = xrs700x_get_sset_count, + .get_ethtool_stats = xrs700x_get_ethtool_stats, + .get_stats64 = xrs700x_get_stats64, + .port_bridge_join = xrs700x_bridge_join, + .port_bridge_leave = xrs700x_bridge_leave, +}; + +static int xrs700x_detect(struct xrs700x *dev) +{ + const struct xrs700x_info *info; + unsigned int id; + int ret; + + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id); + if (ret) { + dev_err(dev->dev, "error %d while reading switch id.\n", + ret); + return ret; + } + + info = of_device_get_match_data(dev->dev); + if (!info) + return -EINVAL; + + if (info->id == id) { + dev->ds->num_ports = info->num_ports; + dev_info(dev->dev, "%s detected.\n", info->name); + return 0; + } + + dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n", + info->id, id); + + return -ENODEV; +} + +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv) +{ + struct dsa_switch *ds; + struct xrs700x *dev; + + ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL); + if (!ds) + return NULL; + + ds->dev = base; + + dev = devm_kzalloc(base, sizeof(*dev), GFP_KERNEL); + if (!dev) + return NULL; + + INIT_DELAYED_WORK(&dev->mib_work, xrs700x_mib_work); + + ds->ops = &xrs700x_ops; + ds->priv = dev; + dev->dev = base; + + dev->ds = ds; + dev->priv = priv; + + return dev; +} +EXPORT_SYMBOL(xrs700x_switch_alloc); + +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port) +{ + struct xrs700x_port *p = &dev->ports[port]; + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs); + + p->mib_data = devm_kzalloc(dev->dev, mib_size, GFP_KERNEL); + if (!p->mib_data) + return -ENOMEM; + + mutex_init(&p->mib_mutex); + u64_stats_init(&p->syncp); + + return 0; +} + +int xrs700x_switch_register(struct xrs700x *dev) +{ + int ret; + int i; + + ret = xrs700x_detect(dev); + if (ret) + return ret; + + ret = xrs700x_setup_regmap_range(dev); + if (ret) + return ret; + + dev->ports = devm_kzalloc(dev->dev, + sizeof(*dev->ports) * dev->ds->num_ports, + GFP_KERNEL); + if (!dev->ports) + return -ENOMEM; + + for (i = 0; i < dev->ds->num_ports; i++) { + ret = xrs700x_alloc_port_mib(dev, i); + if (ret) + return ret; + } + + ret = dsa_register_switch(dev->ds); + + return ret; +} +EXPORT_SYMBOL(xrs700x_switch_register); + +void xrs700x_switch_remove(struct xrs700x *dev) +{ + cancel_delayed_work_sync(&dev->mib_work); + + dsa_unregister_switch(dev->ds); +} +EXPORT_SYMBOL(xrs700x_switch_remove); + +MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>"); +MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/net/dsa/xrs700x/xrs700x.h b/drivers/net/dsa/xrs700x/xrs700x.h new file mode 100644 index 000000000000..831f94941eae --- /dev/null +++ b/drivers/net/dsa/xrs700x/xrs700x.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#include <linux/device.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/workqueue.h> +#include <linux/u64_stats_sync.h> +#include <uapi/linux/if_link.h> + +struct xrs700x_info { + unsigned int id; + const char *name; + size_t num_ports; +}; + +extern const struct xrs700x_info xrs7003e_info; +extern const struct xrs700x_info xrs7003f_info; +extern const struct xrs700x_info xrs7004e_info; +extern const struct xrs700x_info xrs7004f_info; + +struct xrs700x_port { + struct mutex mib_mutex; /* protects mib_data */ + uint64_t *mib_data; + struct rtnl_link_stats64 stats64; + struct u64_stats_sync syncp; +}; + +struct xrs700x { + struct dsa_switch *ds; + struct device *dev; + void *priv; + struct regmap *regmap; + struct regmap_field *ps_forward; + struct regmap_field *ps_management; + struct regmap_field *ps_sel_speed; + struct regmap_field *ps_cur_speed; + struct delayed_work mib_work; + struct xrs700x_port *ports; +}; + +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv); +int xrs700x_switch_register(struct xrs700x *dev); +void xrs700x_switch_remove(struct xrs700x *dev); diff --git a/drivers/net/dsa/xrs700x/xrs700x_i2c.c b/drivers/net/dsa/xrs700x/xrs700x_i2c.c new file mode 100644 index 000000000000..fe64a7757f9e --- /dev/null +++ b/drivers/net/dsa/xrs700x/xrs700x_i2c.c @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 NovaTech LLC + * George McCollister <george.mccollister@gmail.com> + */ + +#include <linux/bits.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include "xrs700x.h" +#include "xrs700x_reg.h" + +static int xrs700x_i2c_reg_read(void *context, unsigned int reg, + unsigned int *val) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + unsigned char buf[4]; + int ret; + + buf[0] = reg >> 23 & 0xff; + buf[1] = reg >> 15 & 0xff; + buf[2] = reg >> 7 & 0xff; + buf[3] = (reg & 0x7f) << 1; + + ret = i2c_master_send(i2c, buf, sizeof(buf)); + if (ret < 0) { + dev_err(dev, "xrs i2c_master_send returned %d\n", ret); + return ret; + } + + ret = i2c_master_recv(i2c, buf, 2); + if (ret < 0) { + dev_err(dev, "xrs i2c_master_recv returned %d\n", ret); + return ret; + } + + *val = buf[0] << 8 | buf[1]; + + return 0; +} + +static int xrs700x_i2c_reg_write(void *context, unsigned int reg, + unsigned int val) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + unsigned char buf[6]; + int ret; + + buf[0] = reg >> 23 & 0xff; + buf[1] = reg >> 15 & 0xff; + buf[2] = reg >> 7 & 0xff; + buf[3] = (reg & 0x7f) << 1 | 1; + buf[4] = val >> 8 & 0xff; + buf[5] = val & 0xff; + + ret = i2c_master_send(i2c, buf, sizeof(buf)); + if (ret < 0) { + dev_err(dev, "xrs i2c_master_send returned %d\n", ret); + return ret; + } + + return 0; +} + +static const struct regmap_config xrs700x_i2c_regmap_config = { + .val_bits = 16, + .reg_stride = 2, + .reg_bits = 32, + .pad_bits = 0, + .write_flag_mask = 0, + .read_flag_mask = 0, + .reg_read = xrs700x_i2c_reg_read, + .reg_write = xrs700x_i2c_reg_write, + .max_register = 0, + .cache_type = REGCACHE_NONE, + .reg_format_endian = REGMAP_ENDIAN_BIG, + .val_format_endian = REGMAP_ENDIAN_BIG +}; + +static int xrs700x_i2c_probe(struct i2c_client *i2c, + const struct i2c_device_id *i2c_id) +{ + struct xrs700x *dev; + int ret; + + dev = xrs700x_switch_alloc(&i2c->dev, i2c); + if (!dev) + return -ENOMEM; + + dev->regmap = devm_regmap_init(&i2c->dev, NULL, &i2c->dev, + &xrs700x_i2c_regmap_config); + if (IS_ERR(dev->regmap)) { + ret = PTR_ERR(dev->regmap); + dev_err(&i2c->dev, "Failed to initialize regmap: %d\n", ret); + return ret; + } + + ret = xrs700x_switch_register(dev); + + /* Main DSA driver may not be started yet. */ + if (ret) + return ret; + + i2c_set_clientdata(i2c, dev); + + return 0; +} + +static int xrs700x_i2c_remove(struct i2c_client *i2c) +{ + struct xrs700x *dev = i2c_get_clientdata(i2c); + + xrs700x_switch_remove(dev); + + return 0; +} + +static const struct i2c_device_id xrs700x_i2c_id[] = { + { "xrs700x-switch", 0 }, + {}, +}; + +MODULE_DEVICE_TABLE(i2c, xrs700x_i2c_id); + +static const struct of_device_id xrs700x_i2c_dt_ids[] = { + { .compatible = "arrow,xrs7003e", .data = &xrs7003e_info }, + { .compatible = "arrow,xrs7003f", .data = &xrs7003f_info }, + { .compatible = "arrow,xrs7004e", .data = &xrs7004e_info }, + { .compatible = "arrow,xrs7004f", .data = &xrs7004f_info }, + {}, +}; +MODULE_DEVICE_TABLE(of, xrs700x_i2c_dt_ids); + +static struct i2c_driver xrs700x_i2c_driver = { + .driver = { + .name = "xrs700x-i2c", + .of_match_table = of_match_ptr(xrs700x_i2c_dt_ids), + }, + .probe = xrs700x_i2c_probe, + .remove = xrs700x_i2c_remove, + .id_table = xrs700x_i2c_id, +}; + +module_i2c_driver(xrs700x_i2c_driver); + +MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>"); +MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA I2C driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c new file mode 100644 index 000000000000..4fa6cc8f871c --- /dev/null +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 NovaTech LLC + * George McCollister <george.mccollister@gmail.com> + */ + +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/mdio.h> +#include <linux/module.h> +#include <linux/phy.h> +#include "xrs700x.h" +#include "xrs700x_reg.h" + +#define XRS_MDIO_IBA0 0x10 +#define XRS_MDIO_IBA1 0x11 +#define XRS_MDIO_IBD 0x14 + +#define XRS_IB_READ 0x0 +#define XRS_IB_WRITE 0x1 + +static int xrs700x_mdio_reg_read(void *context, unsigned int reg, + unsigned int *val) +{ + struct mdio_device *mdiodev = context; + struct device *dev = &mdiodev->dev; + u16 uval; + int ret; + + uval = (u16)FIELD_GET(GENMASK(31, 16), reg); + + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); + return ret; + } + + uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ); + + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); + return ret; + } + + ret = mdiobus_read(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_read returned %d\n", ret); + return ret; + } + + *val = (unsigned int)ret; + + return 0; +} + +static int xrs700x_mdio_reg_write(void *context, unsigned int reg, + unsigned int val) +{ + struct mdio_device *mdiodev = context; + struct device *dev = &mdiodev->dev; + u16 uval; + int ret; + + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD, (u16)val); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); + return ret; + } + + uval = (u16)FIELD_GET(GENMASK(31, 16), reg); + + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); + return ret; + } + + uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_WRITE); + + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval); + if (ret < 0) { + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); + return ret; + } + + return 0; +} + +static const struct regmap_config xrs700x_mdio_regmap_config = { + .val_bits = 16, + .reg_stride = 2, + .reg_bits = 32, + .pad_bits = 0, + .write_flag_mask = 0, + .read_flag_mask = 0, + .reg_read = xrs700x_mdio_reg_read, + .reg_write = xrs700x_mdio_reg_write, + .max_register = XRS_VLAN(MAX_VLAN), + .cache_type = REGCACHE_NONE, + .reg_format_endian = REGMAP_ENDIAN_BIG, + .val_format_endian = REGMAP_ENDIAN_BIG +}; + +static int xrs700x_mdio_probe(struct mdio_device *mdiodev) +{ + struct xrs700x *dev; + int ret; + + dev = xrs700x_switch_alloc(&mdiodev->dev, mdiodev); + if (!dev) + return -ENOMEM; + + dev->regmap = devm_regmap_init(&mdiodev->dev, NULL, mdiodev, + &xrs700x_mdio_regmap_config); + if (IS_ERR(dev->regmap)) { + ret = PTR_ERR(dev->regmap); + dev_err(&mdiodev->dev, "Failed to initialize regmap: %d\n", ret); + return ret; + } + + ret = xrs700x_switch_register(dev); + + /* Main DSA driver may not be started yet. */ + if (ret) + return ret; + + dev_set_drvdata(&mdiodev->dev, dev); + + return 0; +} + +static void xrs700x_mdio_remove(struct mdio_device *mdiodev) +{ + struct xrs700x *dev = dev_get_drvdata(&mdiodev->dev); + + xrs700x_switch_remove(dev); +} + +static const struct of_device_id xrs700x_mdio_dt_ids[] = { + { .compatible = "arrow,xrs7003e", .data = &xrs7003e_info }, + { .compatible = "arrow,xrs7003f", .data = &xrs7003f_info }, + { .compatible = "arrow,xrs7004e", .data = &xrs7004e_info }, + { .compatible = "arrow,xrs7004f", .data = &xrs7004f_info }, + {}, +}; +MODULE_DEVICE_TABLE(of, xrs700x_mdio_dt_ids); + +static struct mdio_driver xrs700x_mdio_driver = { + .mdiodrv.driver = { + .name = "xrs700x-mdio", + .of_match_table = xrs700x_mdio_dt_ids, + }, + .probe = xrs700x_mdio_probe, + .remove = xrs700x_mdio_remove, +}; + +mdio_module_driver(xrs700x_mdio_driver); + +MODULE_AUTHOR("George McCollister <george.mccollister@gmail.com>"); +MODULE_DESCRIPTION("Arrow SpeedChips XRS700x DSA MDIO driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/net/dsa/xrs700x/xrs700x_reg.h b/drivers/net/dsa/xrs700x/xrs700x_reg.h new file mode 100644 index 000000000000..d162d0b84bac --- /dev/null +++ b/drivers/net/dsa/xrs700x/xrs700x_reg.h @@ -0,0 +1,205 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +/* Register Base Addresses */ +#define XRS_DEVICE_ID_BASE 0x0 +#define XRS_GPIO_BASE 0x10000 +#define XRS_PORT_OFFSET 0x10000 +#define XRS_PORT_BASE(x) (0x200000 + XRS_PORT_OFFSET * (x)) +#define XRS_RTC_BASE 0x280000 +#define XRS_TS_OFFSET 0x8000 +#define XRS_TS_BASE(x) (0x290000 + XRS_TS_OFFSET * (x)) +#define XRS_SWITCH_CONF_BASE 0x300000 + +/* Device Identification Registers */ +#define XRS_DEV_ID0 (XRS_DEVICE_ID_BASE + 0) +#define XRS_DEV_ID1 (XRS_DEVICE_ID_BASE + 2) +#define XRS_INT_ID0 (XRS_DEVICE_ID_BASE + 4) +#define XRS_INT_ID1 (XRS_DEVICE_ID_BASE + 6) +#define XRS_REV_ID (XRS_DEVICE_ID_BASE + 8) + +/* GPIO Registers */ +#define XRS_CONFIG0 (XRS_GPIO_BASE + 0x1000) +#define XRS_INPUT_STATUS0 (XRS_GPIO_BASE + 0x1002) +#define XRS_CONFIG1 (XRS_GPIO_BASE + 0x1004) +#define XRS_INPUT_STATUS1 (XRS_GPIO_BASE + 0x1006) +#define XRS_CONFIG2 (XRS_GPIO_BASE + 0x1008) +#define XRS_INPUT_STATUS2 (XRS_GPIO_BASE + 0x100a) + +/* Port Configuration Registers */ +#define XRS_PORT_GEN_BASE(x) (XRS_PORT_BASE(x) + 0x0) +#define XRS_PORT_HSR_BASE(x) (XRS_PORT_BASE(x) + 0x2000) +#define XRS_PORT_PTP_BASE(x) (XRS_PORT_BASE(x) + 0x4000) +#define XRS_PORT_CNT_BASE(x) (XRS_PORT_BASE(x) + 0x6000) +#define XRS_PORT_IPO_BASE(x) (XRS_PORT_BASE(x) + 0x8000) + +/* Port Configuration Registers - General and State */ +#define XRS_PORT_STATE(x) (XRS_PORT_GEN_BASE(x) + 0x0) +#define XRS_PORT_FORWARDING 0 +#define XRS_PORT_LEARNING 1 +#define XRS_PORT_DISABLED 2 +#define XRS_PORT_MODE_NORMAL 0 +#define XRS_PORT_MODE_MANAGEMENT 1 +#define XRS_PORT_SPEED_1000 0x12 +#define XRS_PORT_SPEED_100 0x20 +#define XRS_PORT_SPEED_10 0x30 +#define XRS_PORT_VLAN(x) (XRS_PORT_GEN_BASE(x) + 0x10) +#define XRS_PORT_VLAN0_MAPPING(x) (XRS_PORT_GEN_BASE(x) + 0x12) +#define XRS_PORT_FWD_MASK(x) (XRS_PORT_GEN_BASE(x) + 0x14) +#define XRS_PORT_VLAN_PRIO(x) (XRS_PORT_GEN_BASE(x) + 0x16) + +/* Port Configuration Registers - HSR/PRP */ +#define XRS_HSR_CFG(x) (XRS_PORT_HSR_BASE(x) + 0x0) + +/* Port Configuration Registers - PTP */ +#define XRS_PTP_RX_SYNC_DELAY_NS_LO(x) (XRS_PORT_PTP_BASE(x) + 0x2) +#define XRS_PTP_RX_SYNC_DELAY_NS_HI(x) (XRS_PORT_PTP_BASE(x) + 0x4) +#define XRS_PTP_RX_EVENT_DELAY_NS(x) (XRS_PORT_PTP_BASE(x) + 0xa) +#define XRS_PTP_TX_EVENT_DELAY_NS(x) (XRS_PORT_PTP_BASE(x) + 0x12) + +/* Port Configuration Registers - Counter */ +#define XRS_CNT_CTRL(x) (XRS_PORT_CNT_BASE(x) + 0x0) +#define XRS_RX_GOOD_OCTETS_L (XRS_PORT_CNT_BASE(0) + 0x200) +#define XRS_RX_GOOD_OCTETS_H (XRS_PORT_CNT_BASE(0) + 0x202) +#define XRS_RX_BAD_OCTETS_L (XRS_PORT_CNT_BASE(0) + 0x204) +#define XRS_RX_BAD_OCTETS_H (XRS_PORT_CNT_BASE(0) + 0x206) +#define XRS_RX_UNICAST_L (XRS_PORT_CNT_BASE(0) + 0x208) +#define XRS_RX_UNICAST_H (XRS_PORT_CNT_BASE(0) + 0x20a) +#define XRS_RX_BROADCAST_L (XRS_PORT_CNT_BASE(0) + 0x20c) +#define XRS_RX_BROADCAST_H (XRS_PORT_CNT_BASE(0) + 0x20e) +#define XRS_RX_MULTICAST_L (XRS_PORT_CNT_BASE(0) + 0x210) +#define XRS_RX_MULTICAST_H (XRS_PORT_CNT_BASE(0) + 0x212) +#define XRS_RX_UNDERSIZE_L (XRS_PORT_CNT_BASE(0) + 0x214) +#define XRS_RX_UNDERSIZE_H (XRS_PORT_CNT_BASE(0) + 0x216) +#define XRS_RX_FRAGMENTS_L (XRS_PORT_CNT_BASE(0) + 0x218) +#define XRS_RX_FRAGMENTS_H (XRS_PORT_CNT_BASE(0) + 0x21a) +#define XRS_RX_OVERSIZE_L (XRS_PORT_CNT_BASE(0) + 0x21c) +#define XRS_RX_OVERSIZE_H (XRS_PORT_CNT_BASE(0) + 0x21e) +#define XRS_RX_JABBER_L (XRS_PORT_CNT_BASE(0) + 0x220) +#define XRS_RX_JABBER_H (XRS_PORT_CNT_BASE(0) + 0x222) +#define XRS_RX_ERR_L (XRS_PORT_CNT_BASE(0) + 0x224) +#define XRS_RX_ERR_H (XRS_PORT_CNT_BASE(0) + 0x226) +#define XRS_RX_CRC_L (XRS_PORT_CNT_BASE(0) + 0x228) +#define XRS_RX_CRC_H (XRS_PORT_CNT_BASE(0) + 0x22a) +#define XRS_RX_64_L (XRS_PORT_CNT_BASE(0) + 0x22c) +#define XRS_RX_64_H (XRS_PORT_CNT_BASE(0) + 0x22e) +#define XRS_RX_65_127_L (XRS_PORT_CNT_BASE(0) + 0x230) +#define XRS_RX_65_127_H (XRS_PORT_CNT_BASE(0) + 0x232) +#define XRS_RX_128_255_L (XRS_PORT_CNT_BASE(0) + 0x234) +#define XRS_RX_128_255_H (XRS_PORT_CNT_BASE(0) + 0x236) +#define XRS_RX_256_511_L (XRS_PORT_CNT_BASE(0) + 0x238) +#define XRS_RX_256_511_H (XRS_PORT_CNT_BASE(0) + 0x23a) +#define XRS_RX_512_1023_L (XRS_PORT_CNT_BASE(0) + 0x23c) +#define XRS_RX_512_1023_H (XRS_PORT_CNT_BASE(0) + 0x23e) +#define XRS_RX_1024_1536_L (XRS_PORT_CNT_BASE(0) + 0x240) +#define XRS_RX_1024_1536_H (XRS_PORT_CNT_BASE(0) + 0x242) +#define XRS_RX_HSR_PRP_L (XRS_PORT_CNT_BASE(0) + 0x244) +#define XRS_RX_HSR_PRP_H (XRS_PORT_CNT_BASE(0) + 0x246) +#define XRS_RX_WRONGLAN_L (XRS_PORT_CNT_BASE(0) + 0x248) +#define XRS_RX_WRONGLAN_H (XRS_PORT_CNT_BASE(0) + 0x24a) +#define XRS_RX_DUPLICATE_L (XRS_PORT_CNT_BASE(0) + 0x24c) +#define XRS_RX_DUPLICATE_H (XRS_PORT_CNT_BASE(0) + 0x24e) +#define XRS_TX_OCTETS_L (XRS_PORT_CNT_BASE(0) + 0x280) +#define XRS_TX_OCTETS_H (XRS_PORT_CNT_BASE(0) + 0x282) +#define XRS_TX_UNICAST_L (XRS_PORT_CNT_BASE(0) + 0x284) +#define XRS_TX_UNICAST_H (XRS_PORT_CNT_BASE(0) + 0x286) +#define XRS_TX_BROADCAST_L (XRS_PORT_CNT_BASE(0) + 0x288) +#define XRS_TX_BROADCAST_H (XRS_PORT_CNT_BASE(0) + 0x28a) +#define XRS_TX_MULTICAST_L (XRS_PORT_CNT_BASE(0) + 0x28c) +#define XRS_TX_MULTICAST_H (XRS_PORT_CNT_BASE(0) + 0x28e) +#define XRS_TX_HSR_PRP_L (XRS_PORT_CNT_BASE(0) + 0x290) +#define XRS_TX_HSR_PRP_H (XRS_PORT_CNT_BASE(0) + 0x292) +#define XRS_PRIQ_DROP_L (XRS_PORT_CNT_BASE(0) + 0x2c0) +#define XRS_PRIQ_DROP_H (XRS_PORT_CNT_BASE(0) + 0x2c2) +#define XRS_EARLY_DROP_L (XRS_PORT_CNT_BASE(0) + 0x2c4) +#define XRS_EARLY_DROP_H (XRS_PORT_CNT_BASE(0) + 0x2c6) + +/* Port Configuration Registers - Inbound Policy 0 - 15 */ +#define XRS_ETH_ADDR_CFG(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0x0) +#define XRS_ETH_ADDR_FWD_ALLOW(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0x2) +#define XRS_ETH_ADDR_FWD_MIRROR(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0x4) +#define XRS_ETH_ADDR_0(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0x8) +#define XRS_ETH_ADDR_1(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0xa) +#define XRS_ETH_ADDR_2(x, p) (XRS_PORT_IPO_BASE(x) + \ + (p) * 0x20 + 0xc) + +/* RTC Registers */ +#define XRS_CUR_NSEC0 (XRS_RTC_BASE + 0x1004) +#define XRS_CUR_NSEC1 (XRS_RTC_BASE + 0x1006) +#define XRS_CUR_SEC0 (XRS_RTC_BASE + 0x1008) +#define XRS_CUR_SEC1 (XRS_RTC_BASE + 0x100a) +#define XRS_CUR_SEC2 (XRS_RTC_BASE + 0x100c) +#define XRS_TIME_CC0 (XRS_RTC_BASE + 0x1010) +#define XRS_TIME_CC1 (XRS_RTC_BASE + 0x1012) +#define XRS_TIME_CC2 (XRS_RTC_BASE + 0x1014) +#define XRS_STEP_SIZE0 (XRS_RTC_BASE + 0x1020) +#define XRS_STEP_SIZE1 (XRS_RTC_BASE + 0x1022) +#define XRS_STEP_SIZE2 (XRS_RTC_BASE + 0x1024) +#define XRS_ADJUST_NSEC0 (XRS_RTC_BASE + 0x1034) +#define XRS_ADJUST_NSEC1 (XRS_RTC_BASE + 0x1036) +#define XRS_ADJUST_SEC0 (XRS_RTC_BASE + 0x1038) +#define XRS_ADJUST_SEC1 (XRS_RTC_BASE + 0x103a) +#define XRS_ADJUST_SEC2 (XRS_RTC_BASE + 0x103c) +#define XRS_TIME_CMD (XRS_RTC_BASE + 0x1040) + +/* Time Stamper Registers */ +#define XRS_TS_CTRL(x) (XRS_TS_BASE(x) + 0x1000) +#define XRS_TS_INT_MASK(x) (XRS_TS_BASE(x) + 0x1008) +#define XRS_TS_INT_STATUS(x) (XRS_TS_BASE(x) + 0x1010) +#define XRS_TS_NSEC0(x) (XRS_TS_BASE(x) + 0x1104) +#define XRS_TS_NSEC1(x) (XRS_TS_BASE(x) + 0x1106) +#define XRS_TS_SEC0(x) (XRS_TS_BASE(x) + 0x1108) +#define XRS_TS_SEC1(x) (XRS_TS_BASE(x) + 0x110a) +#define XRS_TS_SEC2(x) (XRS_TS_BASE(x) + 0x110c) +#define XRS_PNCT0(x) (XRS_TS_BASE(x) + 0x1110) +#define XRS_PNCT1(x) (XRS_TS_BASE(x) + 0x1112) + +/* Switch Configuration Registers */ +#define XRS_SWITCH_GEN_BASE (XRS_SWITCH_CONF_BASE + 0x0) +#define XRS_SWITCH_TS_BASE (XRS_SWITCH_CONF_BASE + 0x2000) +#define XRS_SWITCH_VLAN_BASE (XRS_SWITCH_CONF_BASE + 0x4000) + +/* Switch Configuration Registers - General */ +#define XRS_GENERAL (XRS_SWITCH_GEN_BASE + 0x10) +#define XRS_GENERAL_TIME_TRAILER BIT(9) +#define XRS_GENERAL_MOD_SYNC BIT(10) +#define XRS_GENERAL_CUT_THRU BIT(13) +#define XRS_GENERAL_CLR_MAC_TBL BIT(14) +#define XRS_GENERAL_RESET BIT(15) +#define XRS_MT_CLEAR_MASK (XRS_SWITCH_GEN_BASE + 0x12) +#define XRS_ADDRESS_AGING (XRS_SWITCH_GEN_BASE + 0x20) +#define XRS_TS_CTRL_TX (XRS_SWITCH_GEN_BASE + 0x28) +#define XRS_TS_CTRL_RX (XRS_SWITCH_GEN_BASE + 0x2a) +#define XRS_INT_MASK (XRS_SWITCH_GEN_BASE + 0x2c) +#define XRS_INT_STATUS (XRS_SWITCH_GEN_BASE + 0x2e) +#define XRS_MAC_TABLE0 (XRS_SWITCH_GEN_BASE + 0x200) +#define XRS_MAC_TABLE1 (XRS_SWITCH_GEN_BASE + 0x202) +#define XRS_MAC_TABLE2 (XRS_SWITCH_GEN_BASE + 0x204) +#define XRS_MAC_TABLE3 (XRS_SWITCH_GEN_BASE + 0x206) + +/* Switch Configuration Registers - Frame Timestamp */ +#define XRS_TX_TS_NS_LO(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + 0x0) +#define XRS_TX_TS_NS_HI(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + 0x2) +#define XRS_TX_TS_S_LO(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + 0x4) +#define XRS_TX_TS_S_HI(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + 0x6) +#define XRS_TX_TS_HDR(t, h) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x2 * (h) + 0xe) +#define XRS_RX_TS_NS_LO(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x200) +#define XRS_RX_TS_NS_HI(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x202) +#define XRS_RX_TS_S_LO(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x204) +#define XRS_RX_TS_S_HI(t) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x206) +#define XRS_RX_TS_HDR(t, h) (XRS_SWITCH_TS_BASE + 0x80 * (t) + \ + 0x2 * (h) + 0xe) + +/* Switch Configuration Registers - VLAN */ +#define XRS_VLAN(v) (XRS_SWITCH_VLAN_BASE + 0x2 * (v)) + +#define MAX_VLAN 4095 -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-13 14:59 ` [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver George McCollister @ 2021-01-14 1:56 ` Vladimir Oltean 2021-01-14 16:53 ` George McCollister 2021-01-14 17:28 ` Florian Fainelli 1 sibling, 1 reply; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 1:56 UTC (permalink / raw) To: George McCollister Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, devicetree On Wed, Jan 13, 2021 at 08:59:21AM -0600, George McCollister wrote: > Add a driver with initial support for the Arrow SpeedChips XRS7000 > series of gigabit Ethernet switch chips which are typically used in > critical networking applications. > > The switches have up to three RGMII ports and one RMII port. > Management to the switches can be performed over i2c or mdio. > > Support for advanced features such as PTP and > HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and > may be added at a later date. > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > --- Some non-exhaustive feedback below. > +static void xrs700x_teardown(struct dsa_switch *ds) > +{ > + struct xrs700x *priv = ds->priv; > + > + cancel_delayed_work_sync(&priv->mib_work); > +} > + > +static void xrs700x_mac_link_up(struct dsa_switch *ds, int port, > + unsigned int mode, phy_interface_t interface, > + struct phy_device *phydev, > + int speed, int duplex, > + bool tx_pause, bool rx_pause) > +{ > + struct xrs700x *priv = ds->priv; > + unsigned int val; > + > + switch (speed) { > + case SPEED_1000: > + val = XRS_PORT_SPEED_1000; > + break; > + case SPEED_100: > + val = XRS_PORT_SPEED_100; > + break; > + case SPEED_10: > + val = XRS_PORT_SPEED_10; > + break; > + default: > + return; > + } > + > + regmap_fields_write(priv->ps_sel_speed, port, val); > + > + dev_dbg_ratelimited(priv->dev, "%s: port: %d mode: %u speed: %u\n", > + __func__, port, mode, speed); > +} What PHY interface types does the switch support as of this patch? No RGMII delay configuration needed? > + > +static int xrs700x_bridge_common(struct dsa_switch *ds, int port, > + struct net_device *bridge, bool join) > +{ > + unsigned int i, cpu_mask = 0, mask = 0; > + struct xrs700x *priv = ds->priv; > + int ret; > + > + for (i = 0; i < ds->num_ports; i++) { > + if (dsa_is_cpu_port(ds, i)) > + continue; > + > + cpu_mask |= BIT(i); > + > + if (dsa_to_port(ds, i)->bridge_dev == bridge) > + continue; > + > + mask |= BIT(i); > + } > + > + for (i = 0; i < ds->num_ports; i++) { > + if (dsa_to_port(ds, i)->bridge_dev != bridge) > + continue; > + > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask); Maybe it would be worth mentioning in a comment that PORT_FWD_MASK's encoding is "1 = Disable forwarding to the port", otherwise this is confusing. > + if (ret) > + return ret; > + } > + > + if (!join) { > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), > + cpu_mask); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +static int xrs700x_detect(struct xrs700x *dev) > +{ > + const struct xrs700x_info *info; > + unsigned int id; > + int ret; > + > + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id); > + if (ret) { > + dev_err(dev->dev, "error %d while reading switch id.\n", > + ret); > + return ret; > + } > + > + info = of_device_get_match_data(dev->dev); > + if (!info) > + return -EINVAL; > + > + if (info->id == id) { > + dev->ds->num_ports = info->num_ports; > + dev_info(dev->dev, "%s detected.\n", info->name); > + return 0; > + } > + > + dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n", > + info->id, id); I've been there too, not the smartest of decisions in the long run. See commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead of DT one on mismatch") if you want a sneak preview of how this is going to feel two years from now. If you can detect the device id you're probably better off with a single compatible string. > + > + return -ENODEV; > +} > + > +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port) > +{ > + struct xrs700x_port *p = &dev->ports[port]; > + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs); Reverse Christmas tree ordering... sorry. > +int xrs700x_switch_register(struct xrs700x *dev) > +{ > + int ret; > + int i; > + > + ret = xrs700x_detect(dev); > + if (ret) > + return ret; > + > + ret = xrs700x_setup_regmap_range(dev); > + if (ret) > + return ret; > + > + dev->ports = devm_kzalloc(dev->dev, > + sizeof(*dev->ports) * dev->ds->num_ports, > + GFP_KERNEL); devm_kcalloc? > + if (!dev->ports) > + return -ENOMEM; > + > + for (i = 0; i < dev->ds->num_ports; i++) { > + ret = xrs700x_alloc_port_mib(dev, i); > + if (ret) > + return ret; > + } > + > + ret = dsa_register_switch(dev->ds); > + > + return ret; return dsa_register_switch > +} > +EXPORT_SYMBOL(xrs700x_switch_register); > + > +void xrs700x_switch_remove(struct xrs700x *dev) > +{ > + cancel_delayed_work_sync(&dev->mib_work); Is it not enough that this is called from xrs700x_teardown too, which is in the call path of dsa_unregister_switch below? > + > + dsa_unregister_switch(dev->ds); > +} > +EXPORT_SYMBOL(xrs700x_switch_remove); > diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > new file mode 100644 > index 000000000000..4fa6cc8f871c > --- /dev/null > +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > +static int xrs700x_mdio_reg_read(void *context, unsigned int reg, > + unsigned int *val) > +{ > + struct mdio_device *mdiodev = context; > + struct device *dev = &mdiodev->dev; > + u16 uval; > + int ret; > + > + uval = (u16)FIELD_GET(GENMASK(31, 16), reg); > + > + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval); > + if (ret < 0) { > + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); > + return ret; > + } > + > + uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ); What happened to bit 0 of "reg"? > + > + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval); > + if (ret < 0) { > + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); > + return ret; > + } > + > + ret = mdiobus_read(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD); > + if (ret < 0) { > + dev_err(dev, "xrs mdiobus_read returned %d\n", ret); > + return ret; > + } > + > + *val = (unsigned int)ret; > + > + return 0; > +} > +static int xrs700x_mdio_probe(struct mdio_device *mdiodev) > +{ > + struct xrs700x *dev; May boil down to preference too, but I don't believe "dev" is a happy name to give to a driver private data structure. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 1:56 ` Vladimir Oltean @ 2021-01-14 16:53 ` George McCollister 2021-01-14 18:12 ` Andrew Lunn 2021-01-14 18:32 ` Vladimir Oltean 0 siblings, 2 replies; 16+ messages in thread From: George McCollister @ 2021-01-14 16:53 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Wed, Jan 13, 2021 at 7:57 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > What PHY interface types does the switch support as of this patch? > No RGMII delay configuration needed? > Port 0: RMII Port 1-3: RGMII For RGMII the documentation states: "PCB is required to add 1.5 ns to 2.0 ns more delay to the clock line than the other lines, unless the other end (PHY) has configurable RX clock delay." > > + > > +static int xrs700x_bridge_common(struct dsa_switch *ds, int port, > > + struct net_device *bridge, bool join) > > +{ > > + unsigned int i, cpu_mask = 0, mask = 0; > > + struct xrs700x *priv = ds->priv; > > + int ret; > > + > > + for (i = 0; i < ds->num_ports; i++) { > > + if (dsa_is_cpu_port(ds, i)) > > + continue; > > + > > + cpu_mask |= BIT(i); > > + > > + if (dsa_to_port(ds, i)->bridge_dev == bridge) > > + continue; > > + > > + mask |= BIT(i); > > + } > > + > > + for (i = 0; i < ds->num_ports; i++) { > > + if (dsa_to_port(ds, i)->bridge_dev != bridge) > > + continue; > > + > > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask); > > Maybe it would be worth mentioning in a comment that PORT_FWD_MASK's > encoding is "1 = Disable forwarding to the port", otherwise this is > confusing. Okay, done. > > > + if (ret) > > + return ret; > > + } > > + > > + if (!join) { > > + ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), > > + cpu_mask); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > > +static int xrs700x_detect(struct xrs700x *dev) > > +{ > > + const struct xrs700x_info *info; > > + unsigned int id; > > + int ret; > > + > > + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id); > > + if (ret) { > > + dev_err(dev->dev, "error %d while reading switch id.\n", > > + ret); > > + return ret; > > + } > > + > > + info = of_device_get_match_data(dev->dev); > > + if (!info) > > + return -EINVAL; > > + > > + if (info->id == id) { > > + dev->ds->num_ports = info->num_ports; > > + dev_info(dev->dev, "%s detected.\n", info->name); > > + return 0; > > + } > > + > > + dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n", > > + info->id, id); > > I've been there too, not the smartest of decisions in the long run. See > commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead > of DT one on mismatch") if you want a sneak preview of how this is going > to feel two years from now. If you can detect the device id you're > probably better off with a single compatible string. Previously Andrew said: "Either you need to verify the compatible from day one so it is not wrong, or you just use a single compatible "arrow,xrs700x", which cannot be wrong." I did it the first way he suggested, if you would have replied at that time to use a single that's the way I would have done it that way. If you two can agree I should change it to a single string I'd be happy to do so. In my case I need 3 RGMII and only one of the package types will fit on the board so there's no risk of changing to one of the existing parts. Perhaps this could be an issue if a new part is added in the future or on someone else's design. > > > + > > + return -ENODEV; > > +} > > + > > +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port) > > +{ > > + struct xrs700x_port *p = &dev->ports[port]; > > + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs); > > Reverse Christmas tree ordering... sorry. The second line uses p so that won't work. I'll change the function to use devm_kcalloc like you recommended below and just get rid of mib_size. > > > +int xrs700x_switch_register(struct xrs700x *dev) > > +{ > > + int ret; > > + int i; > > + > > + ret = xrs700x_detect(dev); > > + if (ret) > > + return ret; > > + > > + ret = xrs700x_setup_regmap_range(dev); > > + if (ret) > > + return ret; > > + > > + dev->ports = devm_kzalloc(dev->dev, > > + sizeof(*dev->ports) * dev->ds->num_ports, > > + GFP_KERNEL); > > devm_kcalloc? Ok, done. > > > + if (!dev->ports) > > + return -ENOMEM; > > + > > + for (i = 0; i < dev->ds->num_ports; i++) { > > + ret = xrs700x_alloc_port_mib(dev, i); > > + if (ret) > > + return ret; > > + } > > + > > + ret = dsa_register_switch(dev->ds); > > + > > + return ret; > > return dsa_register_switch > > > +} > > +EXPORT_SYMBOL(xrs700x_switch_register); > > + > > +void xrs700x_switch_remove(struct xrs700x *dev) > > +{ > > + cancel_delayed_work_sync(&dev->mib_work); > > Is it not enough that this is called from xrs700x_teardown too, which is > in the call path of dsa_unregister_switch below? yeah, looks like it. I'll remove this. > > > + > > + dsa_unregister_switch(dev->ds); > > +} > > +EXPORT_SYMBOL(xrs700x_switch_remove); > > diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > > new file mode 100644 > > index 000000000000..4fa6cc8f871c > > --- /dev/null > > +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > > +static int xrs700x_mdio_reg_read(void *context, unsigned int reg, > > + unsigned int *val) > > +{ > > + struct mdio_device *mdiodev = context; > > + struct device *dev = &mdiodev->dev; > > + u16 uval; > > + int ret; > > + > > + uval = (u16)FIELD_GET(GENMASK(31, 16), reg); > > + > > + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval); > > + if (ret < 0) { > > + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); > > + return ret; > > + } > > + > > + uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ); > > What happened to bit 0 of "reg"? From the datasheet: "Bits 15-1 of the address on the internal bus to where data is written or from where data is read. Address bit 0 is always 0 (because of 16 bit registers)." reg_stride is set to 2. "The register address stride. Valid register addresses are a multiple of this value." > > > + > > + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA0, uval); > > + if (ret < 0) { > > + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); > > + return ret; > > + } > > + > > + ret = mdiobus_read(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBD); > > + if (ret < 0) { > > + dev_err(dev, "xrs mdiobus_read returned %d\n", ret); > > + return ret; > > + } > > + > > + *val = (unsigned int)ret; > > + > > + return 0; > > +} > > > +static int xrs700x_mdio_probe(struct mdio_device *mdiodev) > > +{ > > + struct xrs700x *dev; > > May boil down to preference too, but I don't believe "dev" is a happy > name to give to a driver private data structure. There are other drivers in the subsystem that do this. If there was a consistent pattern followed in the subsystem I would have followed it. Trust me I was a bit frustrated with home much time I spent going through multiple drivers trying to determine the best practices for organization, naming, etc. If it's a big let me know and I'll change it. Thanks, George ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 16:53 ` George McCollister @ 2021-01-14 18:12 ` Andrew Lunn 2021-01-14 18:32 ` Vladimir Oltean 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2021-01-14 18:12 UTC (permalink / raw) To: George McCollister Cc: Vladimir Oltean, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... > > > +static int xrs700x_detect(struct xrs700x *dev) > > > +{ > > > + const struct xrs700x_info *info; > > > + unsigned int id; > > > + int ret; > > > + > > > + ret = regmap_read(dev->regmap, XRS_DEV_ID0, &id); > > > + if (ret) { > > > + dev_err(dev->dev, "error %d while reading switch id.\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + info = of_device_get_match_data(dev->dev); > > > + if (!info) > > > + return -EINVAL; > > > + > > > + if (info->id == id) { > > > + dev->ds->num_ports = info->num_ports; > > > + dev_info(dev->dev, "%s detected.\n", info->name); > > > + return 0; > > > + } > > > + > > > + dev_err(dev->dev, "expected switch id 0x%x but found 0x%x.\n", > > > + info->id, id); > > > > I've been there too, not the smartest of decisions in the long run. See > > commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead > > of DT one on mismatch") if you want a sneak preview of how this is going > > to feel two years from now. If you can detect the device id you're > > probably better off with a single compatible string. > > Previously Andrew said: > "Either you need to verify the compatible from day one so it is not > wrong, or you just use a single compatible "arrow,xrs700x", which > cannot be wrong." > > I did it the first way he suggested, if you would have replied at that > time to use a single that's the way I would have done it that way. > > If you two can agree I should change it to a single string I'd be > happy to do so. I'm happy both ways. Marvell uses just on compatible, and has worked fine. Other drivers have specific compatible strings, and enforce the match, and that has also worked fine. So it is really up to you. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 16:53 ` George McCollister 2021-01-14 18:12 ` Andrew Lunn @ 2021-01-14 18:32 ` Vladimir Oltean 2021-01-14 18:47 ` George McCollister 1 sibling, 1 reply; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 18:32 UTC (permalink / raw) To: George McCollister Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Thu, Jan 14, 2021 at 10:53:16AM -0600, George McCollister wrote: > On Wed, Jan 13, 2021 at 7:57 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > What PHY interface types does the switch support as of this patch? > > No RGMII delay configuration needed? > > > > Port 0: RMII > Port 1-3: RGMII > > For RGMII the documentation states: > "PCB is required to add 1.5 ns to 2.0 ns more delay to the clock line > than the other lines, unless the other end (PHY) has configurable RX > clock delay." Ok, didn't notice that. > > I've been there too, not the smartest of decisions in the long run. See > > commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead > > of DT one on mismatch") if you want a sneak preview of how this is going > > to feel two years from now. If you can detect the device id you're > > probably better off with a single compatible string. > > Previously Andrew said: > "Either you need to verify the compatible from day one so it is not > wrong, or you just use a single compatible "arrow,xrs700x", which > cannot be wrong." > > I did it the first way he suggested, if you would have replied at that > time to use a single that's the way I would have done it that way. > > If you two can agree I should change it to a single string I'd be > happy to do so. In my case I need 3 RGMII and only one of the package > types will fit on the board so there's no risk of changing to one of > the existing parts. Perhaps this could be an issue if a new part is > added in the future or on someone else's design. Ok, if the parts are not pin-compatible I guess the range of potential issues to deal with may be lower. Don't get me wrong, I don't have a strong opinion. I'm fine if you ignore this comment and keep the specific compatibles, I think this is what the Open Firmware document recommends anyway. > > > +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port) > > > +{ > > > + struct xrs700x_port *p = &dev->ports[port]; > > > + size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs); > > > > Reverse Christmas tree ordering... sorry. > > The second line uses p so that won't work. I'll change the function to > use devm_kcalloc like you recommended below and just get rid of > mib_size. Yes, if you can get rid of it, that works. Generally when somebody says "reverse xmas tree" and it's obvious that there are data dependencies between variables, what they mean to request is: struct xrs700x_port *p = &dev->ports[port]; size_t mib_size; mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs); > > > diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > > > new file mode 100644 > > > index 000000000000..4fa6cc8f871c > > > --- /dev/null > > > +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c > > > +static int xrs700x_mdio_reg_read(void *context, unsigned int reg, > > > + unsigned int *val) > > > +{ > > > + struct mdio_device *mdiodev = context; > > > + struct device *dev = &mdiodev->dev; > > > + u16 uval; > > > + int ret; > > > + > > > + uval = (u16)FIELD_GET(GENMASK(31, 16), reg); > > > + > > > + ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval); > > > + if (ret < 0) { > > > + dev_err(dev, "xrs mdiobus_write returned %d\n", ret); > > > + return ret; > > > + } > > > + > > > + uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ); > > > > What happened to bit 0 of "reg"? > > From the datasheet: > "Bits 15-1 of the address on the internal bus to where data is written > or from where data is read. Address bit 0 is always 0 (because of 16 > bit registers)." > > reg_stride is set to 2. > "The register address stride. Valid register addresses are a multiple > of this value." Ok, clear now. > > May boil down to preference too, but I don't believe "dev" is a happy > > name to give to a driver private data structure. > > There are other drivers in the subsystem that do this. If there was a > consistent pattern followed in the subsystem I would have followed it. > Trust me I was a bit frustrated with home much time I spent going > through multiple drivers trying to determine the best practices for > organization, naming, etc. > If it's a big let me know and I'll change it. Funny that you are complaining about consistency in other drivers, because if I count correctly, out of a total of 22 occurrences of struct xrs700x variables in yours, 13 are named priv and 9 are named dev. So you are not even consistent with yourself. But it's not a major issue either way. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 18:32 ` Vladimir Oltean @ 2021-01-14 18:47 ` George McCollister 2021-01-14 19:00 ` Vladimir Oltean 0 siblings, 1 reply; 16+ messages in thread From: George McCollister @ 2021-01-14 18:47 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Thu, Jan 14, 2021 at 12:32 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > May boil down to preference too, but I don't believe "dev" is a happy > > > name to give to a driver private data structure. > > > > There are other drivers in the subsystem that do this. If there was a > > consistent pattern followed in the subsystem I would have followed it. > > Trust me I was a bit frustrated with home much time I spent going > > through multiple drivers trying to determine the best practices for > > organization, naming, etc. > > If it's a big let me know and I'll change it. > > Funny that you are complaining about consistency in other drivers, > because if I count correctly, out of a total of 22 occurrences of > struct xrs700x variables in yours, 13 are named priv and 9 are named > dev. So you are not even consistent with yourself. But it's not a major > issue either way. Touché. This ended up happening because I followed the pattern used by different drivers in different places. Specifically ksz was using regmap to work on multiple buses but wasn't a very clean example for much else. I'll just change it to priv everywhere. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 18:47 ` George McCollister @ 2021-01-14 19:00 ` Vladimir Oltean 0 siblings, 0 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 19:00 UTC (permalink / raw) To: George McCollister Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Thu, Jan 14, 2021 at 12:47:46PM -0600, George McCollister wrote: > On Thu, Jan 14, 2021 at 12:32 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > May boil down to preference too, but I don't believe "dev" is a happy > > > > name to give to a driver private data structure. > > > > > > There are other drivers in the subsystem that do this. If there was a > > > consistent pattern followed in the subsystem I would have followed it. > > > Trust me I was a bit frustrated with home much time I spent going > > > through multiple drivers trying to determine the best practices for > > > organization, naming, etc. > > > If it's a big let me know and I'll change it. > > > > Funny that you are complaining about consistency in other drivers, > > because if I count correctly, out of a total of 22 occurrences of > > struct xrs700x variables in yours, 13 are named priv and 9 are named > > dev. So you are not even consistent with yourself. But it's not a major > > issue either way. > > Touché. This ended up happening because I followed the pattern used by > different drivers in different places. Specifically ksz was using > regmap to work on multiple buses but wasn't a very clean example for > much else. > I'll just change it to priv everywhere. Don't worry, I know you copied from the micrel ksz driver, I made sure to complain there as well: https://lkml.org/lkml/2020/11/12/1344 It's a pretty bad driver to copy from, by the way. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-13 14:59 ` [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver George McCollister 2021-01-14 1:56 ` Vladimir Oltean @ 2021-01-14 17:28 ` Florian Fainelli 2021-01-14 18:35 ` George McCollister 1 sibling, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2021-01-14 17:28 UTC (permalink / raw) To: George McCollister, Andrew Lunn, Vivien Didelot, Vladimir Oltean Cc: Rob Herring, David S . Miller, netdev, devicetree On 1/13/21 6:59 AM, George McCollister wrote: > Add a driver with initial support for the Arrow SpeedChips XRS7000 > series of gigabit Ethernet switch chips which are typically used in > critical networking applications. > > The switches have up to three RGMII ports and one RMII port. > Management to the switches can be performed over i2c or mdio. > > Support for advanced features such as PTP and > HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and > may be added at a later date. > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > --- [snip] This looks ready to me, just a few nits and suggestions. > +/* Add an inbound policy filter which matches the BPDU destination MAC > + * and forwards to the CPU port. Leave the policy disabled, it will be > + * enabled as needed. > + */ > +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port) > +{ > + struct xrs700x *priv = ds->priv; > + unsigned int val = 0; > + int i = 0; > + int ret; > + > + /* Compare all 48 bits of the destination MAC address. */ > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2); > + if (ret) > + return ret; > + > + /* match BPDU destination 01:80:c2:00:00:00 */ > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001); > + if (ret) > + return ret; > + > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2); > + if (ret) > + return ret; > + > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0); > + if (ret) > + return ret; Not that this is likely to change, but you could write this as a for loop and use eth_stp_addr from include/linux/etherdevice.h. [snip] > +static int xrs700x_port_setup(struct dsa_switch *ds, int port) > +{ > + bool cpu_port = dsa_is_cpu_port(ds, port); > + struct xrs700x *priv = ds->priv; > + unsigned int val = 0; > + int ret, i; > + > + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED); It looks like we should be standardizing at some point on having switch drivers do just the global configuration in the ->setup() callback and have the core call the ->port_disable() for each port except the CPU/DSA ports, and finally let the actual port configuration bet done in ->port_enable(). What you have is fine for now and easy to change in the future. > +int xrs700x_switch_register(struct xrs700x *dev) > +{ > + int ret; > + int i; > + > + ret = xrs700x_detect(dev); > + if (ret) > + return ret; > + > + ret = xrs700x_setup_regmap_range(dev); > + if (ret) > + return ret; > + > + dev->ports = devm_kzalloc(dev->dev, > + sizeof(*dev->ports) * dev->ds->num_ports, > + GFP_KERNEL); > + if (!dev->ports) > + return -ENOMEM; > + > + for (i = 0; i < dev->ds->num_ports; i++) { > + ret = xrs700x_alloc_port_mib(dev, i); > + if (ret) > + return ret; Nothing frees up the successfully allocated p->mib_data[] in case of errors so you would be leaking here. [snip] > + > + /* Main DSA driver may not be started yet. */ > + if (ret) > + return ret; > + > + i2c_set_clientdata(i2c, dev); I would be tempted to move this before "publishing" the device, probably does not harm though, likewise for the MDIO stub. [snip] > +/* Switch Configuration Registers - VLAN */ > +#define XRS_VLAN(v) (XRS_SWITCH_VLAN_BASE + 0x2 * (v)) > + > +#define MAX_VLAN 4095 Can you use VLAN_N_VID - 1 here from include/linux/if_vlan.h? -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 17:28 ` Florian Fainelli @ 2021-01-14 18:35 ` George McCollister 2021-01-14 18:37 ` Florian Fainelli 0 siblings, 1 reply; 16+ messages in thread From: George McCollister @ 2021-01-14 18:35 UTC (permalink / raw) To: Florian Fainelli Cc: Andrew Lunn, Vivien Didelot, Vladimir Oltean, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On Thu, Jan 14, 2021 at 11:28 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 1/13/21 6:59 AM, George McCollister wrote: > > Add a driver with initial support for the Arrow SpeedChips XRS7000 > > series of gigabit Ethernet switch chips which are typically used in > > critical networking applications. > > > > The switches have up to three RGMII ports and one RMII port. > > Management to the switches can be performed over i2c or mdio. > > > > Support for advanced features such as PTP and > > HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and > > may be added at a later date. > > > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > > --- > [snip] > > This looks ready to me, just a few nits and suggestions. > > > > +/* Add an inbound policy filter which matches the BPDU destination MAC > > + * and forwards to the CPU port. Leave the policy disabled, it will be > > + * enabled as needed. > > + */ > > +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port) > > +{ > > + struct xrs700x *priv = ds->priv; > > + unsigned int val = 0; > > + int i = 0; > > + int ret; > > + > > + /* Compare all 48 bits of the destination MAC address. */ > > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2); > > + if (ret) > > + return ret; > > + > > + /* match BPDU destination 01:80:c2:00:00:00 */ > > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0); > > + if (ret) > > + return ret; > > Not that this is likely to change, but you could write this as a for > loop and use eth_stp_addr from include/linux/etherdevice.h. Okay, changed to a loop using eth_stp_addr. I'll retest STP to make sure I didn't break it (this would be my luck towards the end of a patch series). > > [snip] > > > +static int xrs700x_port_setup(struct dsa_switch *ds, int port) > > +{ > > + bool cpu_port = dsa_is_cpu_port(ds, port); > > + struct xrs700x *priv = ds->priv; > > + unsigned int val = 0; > > + int ret, i; > > + > > + xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED); > > It looks like we should be standardizing at some point on having switch > drivers do just the global configuration in the ->setup() callback and > have the core call the ->port_disable() for each port except the CPU/DSA > ports, and finally let the actual port configuration bet done in > ->port_enable(). What you have is fine for now and easy to change in the > future. Sounds good. > > > +int xrs700x_switch_register(struct xrs700x *dev) > > +{ > > + int ret; > > + int i; > > + > > + ret = xrs700x_detect(dev); > > + if (ret) > > + return ret; > > + > > + ret = xrs700x_setup_regmap_range(dev); > > + if (ret) > > + return ret; > > + > > + dev->ports = devm_kzalloc(dev->dev, > > + sizeof(*dev->ports) * dev->ds->num_ports, > > + GFP_KERNEL); > > + if (!dev->ports) > > + return -ENOMEM; > > + > > + for (i = 0; i < dev->ds->num_ports; i++) { > > + ret = xrs700x_alloc_port_mib(dev, i); > > + if (ret) > > + return ret; > > Nothing frees up the successfully allocated p->mib_data[] in case of > errors so you would be leaking here. In case of an error probe will end up returning an error and the memory will be free'd since it was allocated with a devm_ function, won't it? > > [snip] > > > + > > + /* Main DSA driver may not be started yet. */ > > + if (ret) > > + return ret; > > + > > + i2c_set_clientdata(i2c, dev); > > I would be tempted to move this before "publishing" the device, probably > does not harm though, likewise for the MDIO stub. Okay, I moved it. > > [snip] > > > +/* Switch Configuration Registers - VLAN */ > > +#define XRS_VLAN(v) (XRS_SWITCH_VLAN_BASE + 0x2 * (v)) > > + > > +#define MAX_VLAN 4095 > > Can you use VLAN_N_VID - 1 here from include/linux/if_vlan.h? Sure. > -- > Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver 2021-01-14 18:35 ` George McCollister @ 2021-01-14 18:37 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2021-01-14 18:37 UTC (permalink / raw) To: George McCollister Cc: Andrew Lunn, Vivien Didelot, Vladimir Oltean, Rob Herring, David S . Miller, netdev, open list:OPEN FIRMWARE AND... On 1/14/21 10:35 AM, George McCollister wrote: >> Nothing frees up the successfully allocated p->mib_data[] in case of >> errors so you would be leaking here. > > In case of an error probe will end up returning an error and the > memory will be free'd since it was allocated with a devm_ function, > won't it? Sorry completely missed that xrs700x_alloc_port_mib() used a devm_kzalloc(), this is fine. -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net-next v4 3/3] dt-bindings: net: dsa: add bindings for xrs700x switches 2021-01-13 14:59 [PATCH net-next v4 0/3] Arrow SpeedChips XRS700x DSA Driver George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver George McCollister @ 2021-01-13 14:59 ` George McCollister 2 siblings, 0 replies; 16+ messages in thread From: George McCollister @ 2021-01-13 14:59 UTC (permalink / raw) To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean Cc: Rob Herring, David S . Miller, netdev, devicetree, George McCollister Add documentation and an example for Arrow SpeedChips XRS7000 Series single chip Ethernet switches. Signed-off-by: George McCollister <george.mccollister@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> --- .../devicetree/bindings/net/dsa/arrow,xrs700x.yaml | 73 ++++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml diff --git a/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml new file mode 100644 index 000000000000..3f01b65f3b22 --- /dev/null +++ b/Documentation/devicetree/bindings/net/dsa/arrow,xrs700x.yaml @@ -0,0 +1,73 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/dsa/arrow,xrs700x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Arrow SpeedChips XRS7000 Series Switch Device Tree Bindings + +allOf: + - $ref: dsa.yaml# + +maintainers: + - George McCollister <george.mccollister@gmail.com> + +description: + The Arrow SpeedChips XRS7000 Series of single chip gigabit Ethernet switches + are designed for critical networking applications. They have up to three + RGMII ports and one RMII port and are managed via i2c or mdio. + +properties: + compatible: + oneOf: + - enum: + - arrow,xrs7003e + - arrow,xrs7003f + - arrow,xrs7004e + - arrow,xrs7004f + + reg: + maxItems: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + switch@8 { + compatible = "arrow,xrs7004e"; + reg = <0x8>; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + ethernet-port@1 { + reg = <1>; + label = "lan0"; + phy-handle = <&swphy0>; + phy-mode = "rgmii-id"; + }; + ethernet-port@2 { + reg = <2>; + label = "lan1"; + phy-handle = <&swphy1>; + phy-mode = "rgmii-id"; + }; + ethernet-port@3 { + reg = <3>; + label = "cpu"; + ethernet = <&fec1>; + fixed-link { + speed = <1000>; + full-duplex; + }; + }; + }; + }; + }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-14 19:00 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 14:59 [PATCH net-next v4 0/3] Arrow SpeedChips XRS700x DSA Driver George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer George McCollister 2021-01-14 1:05 ` Vladimir Oltean 2021-01-14 1:48 ` Andrew Lunn 2021-01-14 15:03 ` George McCollister 2021-01-13 14:59 ` [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver George McCollister 2021-01-14 1:56 ` Vladimir Oltean 2021-01-14 16:53 ` George McCollister 2021-01-14 18:12 ` Andrew Lunn 2021-01-14 18:32 ` Vladimir Oltean 2021-01-14 18:47 ` George McCollister 2021-01-14 19:00 ` Vladimir Oltean 2021-01-14 17:28 ` Florian Fainelli 2021-01-14 18:35 ` George McCollister 2021-01-14 18:37 ` Florian Fainelli 2021-01-13 14:59 ` [PATCH net-next v4 3/3] dt-bindings: net: dsa: add bindings for xrs700x switches George McCollister
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).