| Message ID | 20210422094257.1641396-4-prasanna.vengateshan@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: DSA driver support for LAN937x switch | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: woojung.huh@microchip.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 94 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
On Thu, Apr 22, 2021 at 03:12:51PM +0530, Prasanna Vengateshan wrote: > The Microchip LAN937X switches have a tagging protocol which is > very similar to KSZ tagging. So that the implementation is added to > tag_ksz.c and reused common APIs > > Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com> > --- > include/net/dsa.h | 2 ++ > net/dsa/Kconfig | 4 ++-- > net/dsa/tag_ksz.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 507082959aa4..98c1ab6dc4dc 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -50,6 +50,7 @@ struct phylink_link_state; > #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20 > #define DSA_TAG_PROTO_SEVILLE_VALUE 21 > #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22 > +#define DSA_TAG_PROTO_LAN937X_VALUE 23 > > enum dsa_tag_protocol { > DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, > @@ -75,6 +76,7 @@ enum dsa_tag_protocol { > DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, > DSA_TAG_PROTO_OCELOT_8021Q = DSA_TAG_PROTO_OCELOT_8021Q_VALUE, > DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE, > + DSA_TAG_PROTO_LAN937X = DSA_TAG_PROTO_LAN937X_VALUE, > }; > > struct packet_type; > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig > index cbc2bd643ab2..888eb79a85f2 100644 > --- a/net/dsa/Kconfig > +++ b/net/dsa/Kconfig > @@ -97,10 +97,10 @@ config NET_DSA_TAG_MTK > Mediatek switches. > > config NET_DSA_TAG_KSZ > - tristate "Tag driver for Microchip 8795/9477/9893 families of switches" > + tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches" > help > Say Y if you want to enable support for tagging frames for the > - Microchip 8795/9477/9893 families of switches. > + Microchip 8795/937x/9477/9893 families of switches. > > config NET_DSA_TAG_RTL4_A > tristate "Tag driver for Realtek 4 byte protocol A tags" > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c > index 4820dbcedfa2..a67f21bdab8f 100644 > --- a/net/dsa/tag_ksz.c > +++ b/net/dsa/tag_ksz.c > @@ -190,10 +190,68 @@ static const struct dsa_device_ops ksz9893_netdev_ops = { > DSA_TAG_DRIVER(ksz9893_netdev_ops); > MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893); > > +/* For xmit, 2 bytes are added before FCS. > + * --------------------------------------------------------------------------- > + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes) > + * --------------------------------------------------------------------------- > + * tag0 : represents tag override, lookup and valid > + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8) > + * > + * For rcv, 1 byte is added before FCS. > + * --------------------------------------------------------------------------- > + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes) > + * --------------------------------------------------------------------------- > + * tag0 : zero-based value represents port > + * (eg, 0x00=port1, 0x02=port3, 0x07=port8) > + */ > +#define LAN937X_INGRESS_TAG_LEN 2 > + > +#define LAN937X_TAIL_TAG_OVERRIDE BIT(11) I had to look up the datasheet, to see what this is, because "override" can mean many things, not all of them are desirable. Port Blocking Override When set, packets will be sent from the specified port(s) regardless, and any port blocking (see Port Transmit Enable in Port MSTP State Register) is ignored. Do you think you can name it LAN937X_TAIL_TAG_BLOCKING_OVERRIDE? I know it's longer, but it's also more suggestive. > +#define LAN937X_TAIL_TAG_LOOKUP BIT(12) > +#define LAN937X_TAIL_TAG_VALID BIT(13) > +#define LAN937X_TAIL_TAG_PORT_MASK 7 > + > +static struct sk_buff *lan937x_xmit(struct sk_buff *skb, > + struct net_device *dev) > +{ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + __be16 *tag; > + u8 *addr; > + u16 val; > + > + tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN); Here we go again.. why is it called INGRESS_TAG_LEN if it is used during xmit, and only during xmit? > + addr = skb_mac_header(skb); My personal choice would have been: const struct ethhdr *hdr = eth_hdr(skb); if (is_link_local_ether_addr(hdr->h_dest)) > + > + val = BIT(dp->index); > + > + if (is_link_local_ether_addr(addr)) > + val |= LAN937X_TAIL_TAG_OVERRIDE; > + > + /* Tail tag valid bit - This bit should always be set by the CPU*/ > + val |= LAN937X_TAIL_TAG_VALID; > + > + *tag = cpu_to_be16(val); > + > + return skb; > +} > + > +static const struct dsa_device_ops lan937x_netdev_ops = { > + .name = "lan937x", > + .proto = DSA_TAG_PROTO_LAN937X, > + .xmit = lan937x_xmit, > + .rcv = ksz9477_rcv, > + .overhead = LAN937X_INGRESS_TAG_LEN, > + .tail_tag = true, > +}; > + > +DSA_TAG_DRIVER(lan937x_netdev_ops); > +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN937X); > + > static struct dsa_tag_driver *dsa_tag_driver_array[] = { > &DSA_TAG_DRIVER_NAME(ksz8795_netdev_ops), > &DSA_TAG_DRIVER_NAME(ksz9477_netdev_ops), > &DSA_TAG_DRIVER_NAME(ksz9893_netdev_ops), > + &DSA_TAG_DRIVER_NAME(lan937x_netdev_ops), > }; > > module_dsa_tag_drivers(dsa_tag_driver_array); > -- > 2.27.0 >
On Thu, 2021-04-22 at 22:18 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Apr 22, 2021 at 03:12:51PM +0530, Prasanna Vengateshan wrote: > > The Microchip LAN937X switches have a tagging protocol which is > > very similar to KSZ tagging. So that the implementation is added to > > tag_ksz.c and reused common APIs > > > > Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com> > > --- > > include/net/dsa.h | 2 ++ > > net/dsa/Kconfig | 4 ++-- > > net/dsa/tag_ksz.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 62 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/dsa.h b/include/net/dsa.h > > index 507082959aa4..98c1ab6dc4dc 100644 > > --- a/include/net/dsa.h > > +++ b/include/net/dsa.h > > @@ -50,6 +50,7 @@ struct phylink_link_state; > > #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20 > > #define DSA_TAG_PROTO_SEVILLE_VALUE 21 > > #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22 > > +#define DSA_TAG_PROTO_LAN937X_VALUE 23 > > > > enum dsa_tag_protocol { > > DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, > > @@ -75,6 +76,7 @@ enum dsa_tag_protocol { > > DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, > > DSA_TAG_PROTO_OCELOT_8021Q = DSA_TAG_PROTO_OCELOT_8021Q_VALUE, > > DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE, > > + DSA_TAG_PROTO_LAN937X = DSA_TAG_PROTO_LAN937X_VALUE, > > }; > > > > struct packet_type; > > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig > > index cbc2bd643ab2..888eb79a85f2 100644 > > --- a/net/dsa/Kconfig > > +++ b/net/dsa/Kconfig > > @@ -97,10 +97,10 @@ config NET_DSA_TAG_MTK > > Mediatek switches. > > > > config NET_DSA_TAG_KSZ > > - tristate "Tag driver for Microchip 8795/9477/9893 families of > > switches" > > + tristate "Tag driver for Microchip 8795/937x/9477/9893 families of > > switches" > > help > > Say Y if you want to enable support for tagging frames for the > > - Microchip 8795/9477/9893 families of switches. > > + Microchip 8795/937x/9477/9893 families of switches. > > > > config NET_DSA_TAG_RTL4_A > > tristate "Tag driver for Realtek 4 byte protocol A tags" > > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c > > index 4820dbcedfa2..a67f21bdab8f 100644 > > --- a/net/dsa/tag_ksz.c > > +++ b/net/dsa/tag_ksz.c > > @@ -190,10 +190,68 @@ static const struct dsa_device_ops ksz9893_netdev_ops > > = { > > DSA_TAG_DRIVER(ksz9893_netdev_ops); > > MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893); > > > > +/* For xmit, 2 bytes are added before FCS. > > + * ------------------------------------------------------------------------ > > --- > > + * > > DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes) > > + * ------------------------------------------------------------------------ > > --- > > + * tag0 : represents tag override, lookup and valid > > + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8) > > + * > > + * For rcv, 1 byte is added before FCS. > > + * ------------------------------------------------------------------------ > > --- > > + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes) > > + * ------------------------------------------------------------------------ > > --- > > + * tag0 : zero-based value represents port > > + * (eg, 0x00=port1, 0x02=port3, 0x07=port8) > > + */ > > +#define LAN937X_INGRESS_TAG_LEN 2 > > + > > +#define LAN937X_TAIL_TAG_OVERRIDE BIT(11) > > I had to look up the datasheet, to see what this is, because "override" > can mean many things, not all of them are desirable. > > Port Blocking Override > When set, packets will be sent from the specified port(s) regardless, and any > port > blocking (see Port Transmit Enable in Port MSTP State Register) is ignored. > > Do you think you can name it LAN937X_TAIL_TAG_BLOCKING_OVERRIDE? I know > it's longer, but it's also more suggestive. Thanks for reviewing the patch series. Suggestion looks meaningful. Noted for next rev. > > > +#define LAN937X_TAIL_TAG_LOOKUP BIT(12) > > +#define LAN937X_TAIL_TAG_VALID BIT(13) > > +#define LAN937X_TAIL_TAG_PORT_MASK 7 > > + > > +static struct sk_buff *lan937x_xmit(struct sk_buff *skb, > > + struct net_device *dev) > > +{ > > + struct dsa_port *dp = dsa_slave_to_port(dev); > > + __be16 *tag; > > + u8 *addr; > > + u16 val; > > +r > > + tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN); > > Here we go again.. why is it called INGRESS_TAG_LEN if it is used during > xmit, and only during xmit? Definition is ingress to the LAN937x since it has different tag length for ingress and egress of LAN937x. Do you think should it be changed? > > > + addr = skb_mac_header(skb); > > My personal choice would have been: > > const struct ethhdr *hdr = eth_hdr(skb); > > if (is_link_local_ether_addr(hdr->h_dest)) It makes more understandable since h_dest is passed. Noted. > > > > > 2.27.0 > >
> > > +#define LAN937X_TAIL_TAG_LOOKUP BIT(12) > > > +#define LAN937X_TAIL_TAG_VALID BIT(13) > > > +#define LAN937X_TAIL_TAG_PORT_MASK 7 > > > + > > > +static struct sk_buff *lan937x_xmit(struct sk_buff *skb, > > > + struct net_device *dev) > > > +{ > > > + struct dsa_port *dp = dsa_slave_to_port(dev); > > > + __be16 *tag; > > > + u8 *addr; > > > + u16 val; > > > +r > > > + tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN); > > > > Here we go again.. why is it called INGRESS_TAG_LEN if it is used during > > xmit, and only during xmit? > Definition is ingress to the LAN937x since it has different tag length for > ingress and egress of LAN937x. Do you think should it be changed? tag drivers run on Linux, not the switch. So all ingress/egress should be relative to linux, not the switch. You are writing a linux driver here, not firmware running on the switch. Andrew
diff --git a/include/net/dsa.h b/include/net/dsa.h index 507082959aa4..98c1ab6dc4dc 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -50,6 +50,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20 #define DSA_TAG_PROTO_SEVILLE_VALUE 21 #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22 +#define DSA_TAG_PROTO_LAN937X_VALUE 23 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -75,6 +76,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, DSA_TAG_PROTO_OCELOT_8021Q = DSA_TAG_PROTO_OCELOT_8021Q_VALUE, DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE, + DSA_TAG_PROTO_LAN937X = DSA_TAG_PROTO_LAN937X_VALUE, }; struct packet_type; diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index cbc2bd643ab2..888eb79a85f2 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -97,10 +97,10 @@ config NET_DSA_TAG_MTK Mediatek switches. config NET_DSA_TAG_KSZ - tristate "Tag driver for Microchip 8795/9477/9893 families of switches" + tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches" help Say Y if you want to enable support for tagging frames for the - Microchip 8795/9477/9893 families of switches. + Microchip 8795/937x/9477/9893 families of switches. config NET_DSA_TAG_RTL4_A tristate "Tag driver for Realtek 4 byte protocol A tags" diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c index 4820dbcedfa2..a67f21bdab8f 100644 --- a/net/dsa/tag_ksz.c +++ b/net/dsa/tag_ksz.c @@ -190,10 +190,68 @@ static const struct dsa_device_ops ksz9893_netdev_ops = { DSA_TAG_DRIVER(ksz9893_netdev_ops); MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893); +/* For xmit, 2 bytes are added before FCS. + * --------------------------------------------------------------------------- + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes) + * --------------------------------------------------------------------------- + * tag0 : represents tag override, lookup and valid + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8) + * + * For rcv, 1 byte is added before FCS. + * --------------------------------------------------------------------------- + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes) + * --------------------------------------------------------------------------- + * tag0 : zero-based value represents port + * (eg, 0x00=port1, 0x02=port3, 0x07=port8) + */ +#define LAN937X_INGRESS_TAG_LEN 2 + +#define LAN937X_TAIL_TAG_OVERRIDE BIT(11) +#define LAN937X_TAIL_TAG_LOOKUP BIT(12) +#define LAN937X_TAIL_TAG_VALID BIT(13) +#define LAN937X_TAIL_TAG_PORT_MASK 7 + +static struct sk_buff *lan937x_xmit(struct sk_buff *skb, + struct net_device *dev) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + __be16 *tag; + u8 *addr; + u16 val; + + tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN); + addr = skb_mac_header(skb); + + val = BIT(dp->index); + + if (is_link_local_ether_addr(addr)) + val |= LAN937X_TAIL_TAG_OVERRIDE; + + /* Tail tag valid bit - This bit should always be set by the CPU*/ + val |= LAN937X_TAIL_TAG_VALID; + + *tag = cpu_to_be16(val); + + return skb; +} + +static const struct dsa_device_ops lan937x_netdev_ops = { + .name = "lan937x", + .proto = DSA_TAG_PROTO_LAN937X, + .xmit = lan937x_xmit, + .rcv = ksz9477_rcv, + .overhead = LAN937X_INGRESS_TAG_LEN, + .tail_tag = true, +}; + +DSA_TAG_DRIVER(lan937x_netdev_ops); +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN937X); + static struct dsa_tag_driver *dsa_tag_driver_array[] = { &DSA_TAG_DRIVER_NAME(ksz8795_netdev_ops), &DSA_TAG_DRIVER_NAME(ksz9477_netdev_ops), &DSA_TAG_DRIVER_NAME(ksz9893_netdev_ops), + &DSA_TAG_DRIVER_NAME(lan937x_netdev_ops), }; module_dsa_tag_drivers(dsa_tag_driver_array);
The Microchip LAN937X switches have a tagging protocol which is very similar to KSZ tagging. So that the implementation is added to tag_ksz.c and reused common APIs Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com> --- include/net/dsa.h | 2 ++ net/dsa/Kconfig | 4 ++-- net/dsa/tag_ksz.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-)