| Message ID | 20210422094257.1641396-4-prasanna.vengateshan@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,net-next,1/9] dt-bindings: net: dsa: dt bindings for microchip lan937x | expand |
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(-)