| Message ID | 20220508224848.2384723-4-hauke@hauke-m.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support | expand |
> The switch does not support per port MTU setting, but only a MRU > setting. Implement this by setting the MTU on the CPU port. > > Without this patch the MRU was always set to 1536, not it is set by the > DSA subsystem and the user scan change it. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index be64cfdeccc7..f9b690251155 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port, > return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask); > } > > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, > + int new_mtu) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct realtek_priv *priv = ds->priv; > + int length; > + > + /* When a new MTU is set, DSA always set the CPU port's MTU to the > + * largest MTU of the slave ports. Because the switch only has a global > + * RX length register, only allowing CPU port here is enough. > + */ > + if (!dsa_is_cpu_port(ds, port)) > + return 0; > + > + length = new_mtu + ETH_HLEN + ETH_FCS_LEN; > + length += dp->tag_ops->needed_headroom; > + length += dp->tag_ops->needed_tailroom; Isn't it better to keep that within the driver? No matter the tag position, it will be either 4 (RTL8365MB_CPU_FORMAT_4BYTES) or 8 (RTL8365MB_CPU_FORMAT_8BYTES) bytes. You can retrieve that from priv->chip_data->cpu->format, but the driver will probably never support RTL8365MB_CPU_FORMAT_4BYTES. Until someone does implement the 4-bytes tag (for some mysterious reason), I believe we could simply use a constant here (using a proper new macro). > + > + if (length > RTL8365MB_CFG0_MAX_LEN_MASK) > + return -EINVAL; > + > + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > + RTL8365MB_CFG0_MAX_LEN_MASK, > + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, > + length)); > +} > + > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port) > +{ > + return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8; What is this magic 8? RTL8_4_TAG_LEN? > +} > + > static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port, > u32 offset, u32 length, u64 *mibvalue) > { > @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > p->index = i; > } > > - /* Set maximum packet length to 1536 bytes */ > - ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > - RTL8365MB_CFG0_MAX_LEN_MASK, > - FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536)); > - if (ret) > - goto out_teardown_irq; > - > if (priv->setup_interface) { > ret = priv->setup_interface(ds); > if (ret) { > @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .phy_read = rtl8365mb_dsa_phy_read, > .phy_write = rtl8365mb_dsa_phy_write, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > -- > 2.30.2 >
> > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, > > + int new_mtu) > > +{ > > + struct dsa_port *dp = dsa_to_port(ds, port); > > + struct realtek_priv *priv = ds->priv; > > + int length; > > + > > + /* When a new MTU is set, DSA always set the CPU port's MTU to the > > + * largest MTU of the slave ports. Because the switch only has a global > > + * RX length register, only allowing CPU port here is enough. > > + */ > > + if (!dsa_is_cpu_port(ds, port)) > > + return 0; > > + > > + length = new_mtu + ETH_HLEN + ETH_FCS_LEN; > > + length += dp->tag_ops->needed_headroom; > > + length += dp->tag_ops->needed_tailroom; > > Isn't it better to keep that within the driver? No matter the tag > position, it will be either 4 (RTL8365MB_CPU_FORMAT_4BYTES) or 8 > (RTL8365MB_CPU_FORMAT_8BYTES) bytes. You can retrieve that from > priv->chip_data->cpu->format, but the driver will probably never > support RTL8365MB_CPU_FORMAT_4BYTES. Until someone does implement the > 4-bytes tag (for some mysterious reason), I believe we could simply > use a constant here (using a proper new macro). Another option is to simply always use the bigger header length. I doubt there are many people actually using jumbo frames, and do they really care about 0x3FFF-4, vs 0x3FFF-8? > > + > > + if (length > RTL8365MB_CFG0_MAX_LEN_MASK) > > + return -EINVAL; > > + > > + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > > + RTL8365MB_CFG0_MAX_LEN_MASK, > > + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, > > + length)); > > +} > > + > > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port) > > +{ > > + return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8; > > What is this magic 8? RTL8_4_TAG_LEN? There are some DSA headers in include/linux/dsa, probably a new one should be added with this RTL8_4_TAG_LEN. Andrew
On Mon, May 09, 2022 at 12:48:47AM +0200, Hauke Mehrtens wrote: > The switch does not support per port MTU setting, but only a MRU > setting. Implement this by setting the MTU on the CPU port. > > Without this patch the MRU was always set to 1536, not it is set by the s/not/now/ > DSA subsystem and the user scan change it. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index be64cfdeccc7..f9b690251155 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port, > return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask); > } > > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, > + int new_mtu) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct realtek_priv *priv = ds->priv; > + int length; > + > + /* When a new MTU is set, DSA always set the CPU port's MTU to the s/set/sets/ > + * largest MTU of the slave ports. Because the switch only has a global > + * RX length register, only allowing CPU port here is enough. > + */ > + if (!dsa_is_cpu_port(ds, port)) > + return 0; > + > + length = new_mtu + ETH_HLEN + ETH_FCS_LEN; > + length += dp->tag_ops->needed_headroom; > + length += dp->tag_ops->needed_tailroom; > + > + if (length > RTL8365MB_CFG0_MAX_LEN_MASK) > + return -EINVAL; > + > + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > + RTL8365MB_CFG0_MAX_LEN_MASK, > + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, > + length)); > +} > + > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port) > +{ > + return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8; Is this 8 because of the Realtek CPU tag length? Luiz and Andrew had some good comments regarding this. Otherwise I think the patch is OK. > +} > + > static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port, > u32 offset, u32 length, u64 *mibvalue) > { > @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > p->index = i; > } > > - /* Set maximum packet length to 1536 bytes */ > - ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > - RTL8365MB_CFG0_MAX_LEN_MASK, > - FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536)); > - if (ret) > - goto out_teardown_irq; > - > if (priv->setup_interface) { > ret = priv->setup_interface(ds); > if (ret) { > @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .phy_read = rtl8365mb_dsa_phy_read, > .phy_write = rtl8365mb_dsa_phy_write, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > -- > 2.30.2 >
On Mon, May 09, 2022 at 12:48:47AM +0200, Hauke Mehrtens wrote: > The switch does not support per port MTU setting, but only a MRU > setting. Implement this by setting the MTU on the CPU port. Could you also please set ds->mtu_enforcement_ingress, for no other reason than to have the dev->mtu of all other bridge ports automatically updated when the user changes one of them? > > Without this patch the MRU was always set to 1536, not it is set by the > DSA subsystem and the user scan change it. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index be64cfdeccc7..f9b690251155 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port, > return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask); > } > > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, > + int new_mtu) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct realtek_priv *priv = ds->priv; > + int length; > + > + /* When a new MTU is set, DSA always set the CPU port's MTU to the > + * largest MTU of the slave ports. Because the switch only has a global > + * RX length register, only allowing CPU port here is enough. > + */ > + if (!dsa_is_cpu_port(ds, port)) das_port_is_cpu(dp) > + return 0; > + > + length = new_mtu + ETH_HLEN + ETH_FCS_LEN; > + length += dp->tag_ops->needed_headroom; > + length += dp->tag_ops->needed_tailroom; > + > + if (length > RTL8365MB_CFG0_MAX_LEN_MASK) > + return -EINVAL; > + > + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > + RTL8365MB_CFG0_MAX_LEN_MASK, > + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, > + length)); > +} > + > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port) > +{ > + return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8; > +} > + > static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port, > u32 offset, u32 length, u64 *mibvalue) > { > @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > p->index = i; > } > > - /* Set maximum packet length to 1536 bytes */ > - ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, > - RTL8365MB_CFG0_MAX_LEN_MASK, > - FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536)); > - if (ret) > - goto out_teardown_irq; > - > if (priv->setup_interface) { > ret = priv->setup_interface(ds); > if (ret) { > @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .phy_read = rtl8365mb_dsa_phy_read, > .phy_write = rtl8365mb_dsa_phy_write, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > + .port_change_mtu = rtl8365mb_port_change_mtu, > + .port_max_mtu = rtl8365mb_port_max_mtu, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > .get_sset_count = rtl8365mb_get_sset_count, > -- > 2.30.2 >
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index be64cfdeccc7..f9b690251155 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port, return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask); } +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port, + int new_mtu) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct realtek_priv *priv = ds->priv; + int length; + + /* When a new MTU is set, DSA always set the CPU port's MTU to the + * largest MTU of the slave ports. Because the switch only has a global + * RX length register, only allowing CPU port here is enough. + */ + if (!dsa_is_cpu_port(ds, port)) + return 0; + + length = new_mtu + ETH_HLEN + ETH_FCS_LEN; + length += dp->tag_ops->needed_headroom; + length += dp->tag_ops->needed_tailroom; + + if (length > RTL8365MB_CFG0_MAX_LEN_MASK) + return -EINVAL; + + return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, + RTL8365MB_CFG0_MAX_LEN_MASK, + FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, + length)); +} + +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port) +{ + return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8; +} + static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port, u32 offset, u32 length, u64 *mibvalue) { @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds) p->index = i; } - /* Set maximum packet length to 1536 bytes */ - ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG, - RTL8365MB_CFG0_MAX_LEN_MASK, - FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536)); - if (ret) - goto out_teardown_irq; - if (priv->setup_interface) { ret = priv->setup_interface(ds); if (ret) { @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, .port_stp_state_set = rtl8365mb_port_stp_state_set, + .port_change_mtu = rtl8365mb_port_change_mtu, + .port_max_mtu = rtl8365mb_port_max_mtu, .get_strings = rtl8365mb_get_strings, .get_ethtool_stats = rtl8365mb_get_ethtool_stats, .get_sset_count = rtl8365mb_get_sset_count, @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { .phy_read = rtl8365mb_dsa_phy_read, .phy_write = rtl8365mb_dsa_phy_write, .port_stp_state_set = rtl8365mb_port_stp_state_set, + .port_change_mtu = rtl8365mb_port_change_mtu, + .port_max_mtu = rtl8365mb_port_max_mtu, .get_strings = rtl8365mb_get_strings, .get_ethtool_stats = rtl8365mb_get_ethtool_stats, .get_sset_count = rtl8365mb_get_sset_count,
The switch does not support per port MTU setting, but only a MRU setting. Implement this by setting the MTU on the CPU port. Without this patch the MRU was always set to 1536, not it is set by the DSA subsystem and the user scan change it. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 7 deletions(-)