| Message ID | 20210422094257.1641396-1-prasanna.vengateshan@microchip.com |
|---|---|
| Headers | show |
| Series | net: dsa: microchip: DSA driver support for LAN937x switch | expand |
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, Apr 22, 2021 at 03:12:53PM +0530, Prasanna Vengateshan wrote: > Support for phylink_validate() and reused KSZ commmon API for > phylink_mac_link_down() operation > > Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com> > --- > drivers/net/dsa/microchip/lan937x_main.c | 42 ++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c > index 944c6f4d6d60..93c392081423 100644 > --- a/drivers/net/dsa/microchip/lan937x_main.c > +++ b/drivers/net/dsa/microchip/lan937x_main.c > @@ -312,6 +312,46 @@ static int lan937x_get_max_mtu(struct dsa_switch *ds, int port) > return FR_MAX_SIZE; > } > > +static void lan937x_phylink_validate(struct dsa_switch *ds, int port, > + unsigned long *supported, > + struct phylink_link_state *state) > +{ > + struct ksz_device *dev = ds->priv; > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > + > + if (phy_interface_mode_is_rgmii(state->interface) || > + state->interface == PHY_INTERFACE_MODE_SGMII || > + state->interface == PHY_INTERFACE_MODE_RMII || > + state->interface == PHY_INTERFACE_MODE_MII || > + lan937x_is_internal_100BTX_phy_port(dev, port)) { > + phylink_set(mask, 10baseT_Half); > + phylink_set(mask, 10baseT_Full); > + phylink_set(mask, 100baseT_Half); > + phylink_set(mask, 100baseT_Full); > + phylink_set(mask, Autoneg); > + phylink_set_port_modes(mask); > + phylink_set(mask, Pause); > + phylink_set(mask, Asym_Pause); Why do you advertise pause if you don't react to it (I commented on the previous patch that you force flow control off)? And the Microchip KSZ DSA phylink integration is the strangest I've ever seen, bar none. Do your RGMII ports work at 10/100/1000 Mbps? If yes, how? Do your SGMII ports work in fixed-link, at 10/100/1000? How about with a PHY that expects in-band autoneg? > + } > + > + /* For RGMII & SGMII interfaces */ > + if (phy_interface_mode_is_rgmii(state->interface) || > + state->interface == PHY_INTERFACE_MODE_SGMII) { > + phylink_set(mask, 1000baseT_Full); > + } > + > + /* For T1 PHY */ > + if (lan937x_is_internal_t1_phy_port(dev, port)) { > + phylink_set(mask, 100baseT1_Full); > + phylink_set_port_modes(mask); > + } > + > + bitmap_and(supported, supported, mask, > + __ETHTOOL_LINK_MODE_MASK_NBITS); > + bitmap_and(state->advertising, state->advertising, mask, > + __ETHTOOL_LINK_MODE_MASK_NBITS); > +} > + > static int lan937x_port_pre_bridge_flags(struct dsa_switch *ds, int port, > struct switchdev_brport_flags flags, > struct netlink_ext_ack *extack) > @@ -340,6 +380,8 @@ const struct dsa_switch_ops lan937x_switch_ops = { > .port_fast_age = ksz_port_fast_age, > .port_max_mtu = lan937x_get_max_mtu, > .port_change_mtu = lan937x_change_mtu, > + .phylink_validate = lan937x_phylink_validate, > + .phylink_mac_link_down = ksz_mac_link_down, > }; > > int lan937x_switch_register(struct ksz_device *dev) > -- > 2.27.0 > Fundamentally, what is the purpose of this patch?
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