| Message ID | 20220107053723.21341-1-luizluca@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [1/3] ramips: ethernet: ralink: mt7620 jumbo frame support | expand |
Hi, can I get `Tested-by:` for this series? Thanks. Luiz Angelo Daros de Luca <luizluca@gmail.com> [2022-01-07 02:37:21]: > mt7620 can forward jumbo frames. The fe_change_mtu() was already > compatible except for the GDM_FWD_CFG address. > > An MTU greater than 1500 is required to use DSA tags with a stacked > switch chip. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > .../files/drivers/net/ethernet/ralink/mtk_eth_soc.c | 13 ++++++++++--- > .../files/drivers/net/ethernet/ralink/soc_mt7620.c | 3 ++- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > index 8b57a3cc9a..be2ee6ba7f 100644 > --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > @@ -1458,6 +1458,13 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > struct fe_priv *priv = netdev_priv(dev); > int frag_size, old_mtu; > u32 fwd_cfg; > + u32 fwd_reg; > + > +#ifdef CONFIG_SOC_MT7620 > + fwd_reg = MT7620A_GDMA1_FWD_CFG; > +#else > + fwd_reg = FE_GDMA1_FWD_CFG; > +#endif > > old_mtu = dev->mtu; > dev->mtu = new_mtu; > @@ -1482,7 +1489,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > > fe_stop(dev); > if (!IS_ENABLED(CONFIG_SOC_MT7621)) { > - fwd_cfg = fe_r32(FE_GDMA1_FWD_CFG); > + fwd_cfg = fe_r32(fwd_reg); > if (new_mtu <= ETH_DATA_LEN) { > fwd_cfg &= ~FE_GDM1_JMB_EN; > } else { > @@ -1491,7 +1498,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > fwd_cfg |= (DIV_ROUND_UP(frag_size, 1024) << > FE_GDM1_JMB_LEN_SHIFT) | FE_GDM1_JMB_EN; > } > - fe_w32(fwd_cfg, FE_GDMA1_FWD_CFG); > + fe_w32(fwd_cfg, fwd_reg); > } > > return fe_open(dev); > @@ -1610,7 +1617,7 @@ static int fe_probe(struct platform_device *pdev) > > > if (IS_ENABLED(CONFIG_SOC_MT7620)) > - netdev->max_mtu = 1508; > + netdev->max_mtu = 2048; > if (IS_ENABLED(CONFIG_SOC_MT7621)) > netdev->max_mtu = 2048; > > diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > index 42685eebc3..8c43e6d78f 100644 > --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > @@ -345,7 +345,8 @@ static void mt7620_init_data(struct fe_soc_data *data, > struct fe_priv *priv = netdev_priv(netdev); > > priv->flags = FE_FLAG_PADDING_64B | FE_FLAG_RX_2B_OFFSET | > - FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH; > + FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH | > + FE_FLAG_JUMBO_FRAME; > > netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM | > NETIF_F_HW_VLAN_CTAG_TX;
Petr Štetiar <ynezz@true.cz> wrote: > Hi, can I get `Tested-by:` for this series? Thanks. It simple a) don't apply to master tree; b) not working - MAX_RX_LENGTH constant not touched, GSW_REG_GMACCR not tweaked for accepting jumbo frames. > Luiz Angelo Daros de Luca <luizluca@gmail.com> [2022-01-07 02:37:21]: > > mt7620 can forward jumbo frames. The fe_change_mtu() was already > > compatible except for the GDM_FWD_CFG address. > > > > An MTU greater than 1500 is required to use DSA tags with a stacked > > switch chip. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > .../files/drivers/net/ethernet/ralink/mtk_eth_soc.c | 13 ++++++++++--- > > .../files/drivers/net/ethernet/ralink/soc_mt7620.c | 3 ++- > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > > index 8b57a3cc9a..be2ee6ba7f 100644 > > --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > > +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c > > @@ -1458,6 +1458,13 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > > struct fe_priv *priv = netdev_priv(dev); > > int frag_size, old_mtu; > > u32 fwd_cfg; > > + u32 fwd_reg; > > + > > +#ifdef CONFIG_SOC_MT7620 > > + fwd_reg = MT7620A_GDMA1_FWD_CFG; > > +#else > > + fwd_reg = FE_GDMA1_FWD_CFG; > > +#endif > > > > old_mtu = dev->mtu; > > dev->mtu = new_mtu; > > @@ -1482,7 +1489,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > > > > fe_stop(dev); > > if (!IS_ENABLED(CONFIG_SOC_MT7621)) { > > - fwd_cfg = fe_r32(FE_GDMA1_FWD_CFG); > > + fwd_cfg = fe_r32(fwd_reg); > > if (new_mtu <= ETH_DATA_LEN) { > > fwd_cfg &= ~FE_GDM1_JMB_EN; > > } else { > > @@ -1491,7 +1498,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) > > fwd_cfg |= (DIV_ROUND_UP(frag_size, 1024) << > > FE_GDM1_JMB_LEN_SHIFT) | FE_GDM1_JMB_EN; > > } > > - fe_w32(fwd_cfg, FE_GDMA1_FWD_CFG); > > + fe_w32(fwd_cfg, fwd_reg); > > } > > > > return fe_open(dev); > > @@ -1610,7 +1617,7 @@ static int fe_probe(struct platform_device *pdev) > > > > > > if (IS_ENABLED(CONFIG_SOC_MT7620)) > > - netdev->max_mtu = 1508; > > + netdev->max_mtu = 2048; > > if (IS_ENABLED(CONFIG_SOC_MT7621)) > > netdev->max_mtu = 2048; This chunk is missing in master tree. And never present. > > diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > > index 42685eebc3..8c43e6d78f 100644 > > --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > > +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c > > @@ -345,7 +345,8 @@ static void mt7620_init_data(struct fe_soc_data *data, > > struct fe_priv *priv = netdev_priv(netdev); > > > > priv->flags = FE_FLAG_PADDING_64B | FE_FLAG_RX_2B_OFFSET | > > - FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH; > > + FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH | > > + FE_FLAG_JUMBO_FRAME; > > > > netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM | > > NETIF_F_HW_VLAN_CTAG_TX;
Hi Andrey, > It simple a) don't apply to master tree; b) not working - MAX_RX_LENGTH constant not > touched, GSW_REG_GMACCR not tweaked for accepting jumbo frames. a) Sorry, I missed your answer and thanks for the review. Yes, it was not applying to master. It was based on another patch deeper in my tree. I fixed that a long time ago but I postponed the fix as I didn't see any interest from ML (I was wrong). I was focusing on upstream patches (realtek dsa switches merged into 5.18) and postponed the ramips fixes for when they would be needed. b) MAX_RX_LENGTH seems to be used only for keeping the buffer at least over (MAX_RX_LENGTH - FE_RX_ETH_HLEN) and it does not care when mtu is actually even bigger than (MAX_RX_LENGTH - FE_RX_ETH_HLEN). See: target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c: static inline int fe_max_frag_size(int mtu) { /* make sure buf_size will be at least MAX_RX_LENGTH */ if (mtu + FE_RX_ETH_HLEN < MAX_RX_LENGTH) mtu = MAX_RX_LENGTH - FE_RX_ETH_HLEN; return SKB_DATA_ALIGN(FE_RX_HLEN + mtu) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); } static inline int fe_max_buf_size(int frag_size) { int buf_size = frag_size - NET_SKB_PAD - NET_IP_ALIGN - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); BUG_ON(buf_size < MAX_RX_LENGTH); return buf_size; } Does this logic need to be updated somehow for jumbo or even non jumbo frames? c) MAX_RX_JUMBO and MAX_RX_PKT_LEN in GSW_REG_GMACCR are switch regs while I touched only ethernet frame engine regs. For me, the switch was happily forwarding frames with 8 extra bytes (DSA tag). In my device, the internal switch was being used as a transparent switch (no vlan), connecting to an external switch (RTL8367S) through the RGMII interface. Maybe the small increase was still falling into an accepted frame size range (for the switch), the RGMII interface might have a less restricted access or disabling vlans also makes the switch more tolerant to larger frames. The FastEthernet ports are not used in my device so extra bytes might need to come from the RTL8367S DSA switch, which currently does not support increasing the MTU of slave ports. I cannot easily generate incoming frames larger than 1508. I wonder if it is the ethernet driver's responsibility to increase the switch frame size or if it is the switch driver (swconfig or DSA). It might not only affect the CPU port but any traffic between ports. I'm not very comfortable touching a piece of code I cannot really test. If a dev does have access to a mt7620 device that actually uses its ports, it might be easier for that dev to go ahead and fix it himself. The patch is as simple as: #ifdef CONFIG_SOC_MT7620 if (<frame size calculated from mtu> > 1536) { // set MAX_RX_JUMBO as 2k GSW_REG_GMACCR |= 0x2 << 2 // set MAX_RX_PKT_LEN as MAX_RX_JUMBO GSW_REG_GMACCR |= 0x3 } #endif It might be inserted into the same place fe_max_frag_size is called in fe_change_mtu() but I'm not sure how it would interact with other non MT7621 SoCs. I wonder why MT7621 does not seem to bother the extra bytes. Anyway, the patch is a step further and it is still required and enough for an external DSA switch. The extra GSW_REG_GMACCR tweaks, if needed, might be added in the future. I'll wait a couple of days to settle this thread and resubmit a master-compatible v2 (just a minor fix you pointed out) with an extra patch for disabling checksum offload when the DSA tag is not Mediatek. Regards, Luiz
Once upon a time, Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > > Hi Andrey, > > > It simple a) don't apply to master tree; b) not working - MAX_RX_LENGTH constant not > > touched, GSW_REG_GMACCR not tweaked for accepting jumbo frames. > > a) Sorry, I missed your answer and thanks for the review. Yes, it was > not applying to master. It was based on another patch deeper in my > tree. I fixed that a long time ago but I postponed the fix as I didn't > see any interest from ML (I was wrong). I was focusing on upstream > patches (realtek dsa switches merged into 5.18) and postponed the > ramips fixes for when they would be needed. > > b) MAX_RX_LENGTH seems to be used only for keeping the buffer at least > over (MAX_RX_LENGTH - FE_RX_ETH_HLEN) and it does not care when mtu is > actually even bigger than (MAX_RX_LENGTH - FE_RX_ETH_HLEN). See: MAX_RX_LENGTH affects buffers for DMA transfers. > target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c: > static inline int fe_max_frag_size(int mtu) > { > /* make sure buf_size will be at least MAX_RX_LENGTH */ > if (mtu + FE_RX_ETH_HLEN < MAX_RX_LENGTH) > mtu = MAX_RX_LENGTH - FE_RX_ETH_HLEN; > > return SKB_DATA_ALIGN(FE_RX_HLEN + mtu) + > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > } > > static inline int fe_max_buf_size(int frag_size) > { > int buf_size = frag_size - NET_SKB_PAD - NET_IP_ALIGN - > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > BUG_ON(buf_size < MAX_RX_LENGTH); > return buf_size; > } > > Does this logic need to be updated somehow for jumbo or even non jumbo frames? No. Only maybe remove BUG_ON(). > c) MAX_RX_JUMBO and MAX_RX_PKT_LEN in GSW_REG_GMACCR are switch regs > while I touched only ethernet frame engine regs. There are two parts - FE and switch. And both need to be properly programmed. > For me, the switch was happily forwarding frames with 8 extra bytes (DSA tag). In my > device, the internal switch was being used as a transparent switch (no > vlan), connecting to an external switch (RTL8367S) through the RGMII > interface. Maybe the small increase was still falling into an accepted > frame size range (for the switch), the RGMII interface might have a > less restricted access or disabling vlans also makes the switch more > tolerant to larger frames. The FastEthernet ports are not used in my > device so extra bytes might need to come from the RTL8367S DSA switch, > which currently does not support increasing the MTU of slave ports. I > cannot easily generate incoming frames larger than 1508. I have hardware with an internal switch. And your patch is not working for it. > I wonder if it is the ethernet driver's responsibility to increase the > switch frame size or if it is the switch driver (swconfig or DSA). It > might not only affect the CPU port but any traffic between ports. I'm > not very comfortable touching a piece of code I cannot really test. If > a dev does have access to a mt7620 device that actually uses its > ports, it might be easier for that dev to go ahead and fix it himself. > The patch is as simple as: > > #ifdef CONFIG_SOC_MT7620 > if (<frame size calculated from mtu> > 1536) { > // set MAX_RX_JUMBO as 2k > GSW_REG_GMACCR |= 0x2 << 2 > // set MAX_RX_PKT_LEN as MAX_RX_JUMBO > GSW_REG_GMACCR |= 0x3 > } > #endif > It might be inserted into the same place fe_max_frag_size is called in > fe_change_mtu() but I'm not sure how it would interact with other non > MT7621 SoCs. > I wonder why MT7621 does not seem to bother the extra bytes. Anyway, > the patch is a step further and it is still required and enough for an > external DSA switch. > The extra GSW_REG_GMACCR tweaks, if needed, might be added in the > future. I'll wait a couple of days to settle this thread and resubmit > a master-compatible v2 (just a minor fix you pointed out) with an > extra patch for disabling checksum offload when the DSA tag is not > Mediatek. Attached patch tested on real MT7620 with internal switch and it is able to operate with a 2048-bytes frame (included single VLAN tag).
> > > > Hi Andrey, > > > > > It simple a) don't apply to master tree; b) not working - MAX_RX_LENGTH constant not > > > touched, GSW_REG_GMACCR not tweaked for accepting jumbo frames. > > > > a) Sorry, I missed your answer and thanks for the review. Yes, it was > > not applying to master. It was based on another patch deeper in my > > tree. I fixed that a long time ago but I postponed the fix as I didn't > > see any interest from ML (I was wrong). I was focusing on upstream > > patches (realtek dsa switches merged into 5.18) and postponed the > > ramips fixes for when they would be needed. > > > > b) MAX_RX_LENGTH seems to be used only for keeping the buffer at least > > over (MAX_RX_LENGTH - FE_RX_ETH_HLEN) and it does not care when mtu is > > actually even bigger than (MAX_RX_LENGTH - FE_RX_ETH_HLEN). See: > > MAX_RX_LENGTH affects buffers for DMA transfers. Is it because when you unconditionally enable jumbo frames in switch, you need to prepare the ethernet driver to receive a larger frame, even when mtu is 1500 or smaller? > > > c) MAX_RX_JUMBO and MAX_RX_PKT_LEN in GSW_REG_GMACCR are switch regs > > while I touched only ethernet frame engine regs. > > There are two parts - FE and switch. And both need to be properly programmed. > > Attached patch tested on real MT7620 with internal switch and it is > able to operate with a 2048-bytes frame (included single VLAN tag). Are you ignoring CONFIG_SOC_MT7621 support for this driver? (it is indeed upstream for recent kernels). The patch worked as expected! Thanks a lot! Do you plan to submit it? Tested-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c index 8b57a3cc9a..be2ee6ba7f 100644 --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c @@ -1458,6 +1458,13 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) struct fe_priv *priv = netdev_priv(dev); int frag_size, old_mtu; u32 fwd_cfg; + u32 fwd_reg; + +#ifdef CONFIG_SOC_MT7620 + fwd_reg = MT7620A_GDMA1_FWD_CFG; +#else + fwd_reg = FE_GDMA1_FWD_CFG; +#endif old_mtu = dev->mtu; dev->mtu = new_mtu; @@ -1482,7 +1489,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) fe_stop(dev); if (!IS_ENABLED(CONFIG_SOC_MT7621)) { - fwd_cfg = fe_r32(FE_GDMA1_FWD_CFG); + fwd_cfg = fe_r32(fwd_reg); if (new_mtu <= ETH_DATA_LEN) { fwd_cfg &= ~FE_GDM1_JMB_EN; } else { @@ -1491,7 +1498,7 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu) fwd_cfg |= (DIV_ROUND_UP(frag_size, 1024) << FE_GDM1_JMB_LEN_SHIFT) | FE_GDM1_JMB_EN; } - fe_w32(fwd_cfg, FE_GDMA1_FWD_CFG); + fe_w32(fwd_cfg, fwd_reg); } return fe_open(dev); @@ -1610,7 +1617,7 @@ static int fe_probe(struct platform_device *pdev) if (IS_ENABLED(CONFIG_SOC_MT7620)) - netdev->max_mtu = 1508; + netdev->max_mtu = 2048; if (IS_ENABLED(CONFIG_SOC_MT7621)) netdev->max_mtu = 2048; diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c index 42685eebc3..8c43e6d78f 100644 --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/soc_mt7620.c @@ -345,7 +345,8 @@ static void mt7620_init_data(struct fe_soc_data *data, struct fe_priv *priv = netdev_priv(netdev); priv->flags = FE_FLAG_PADDING_64B | FE_FLAG_RX_2B_OFFSET | - FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH; + FE_FLAG_RX_SG_DMA | FE_FLAG_HAS_SWITCH | + FE_FLAG_JUMBO_FRAME; netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_TX;
mt7620 can forward jumbo frames. The fe_change_mtu() was already compatible except for the GDM_FWD_CFG address. An MTU greater than 1500 is required to use DSA tags with a stacked switch chip. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- .../files/drivers/net/ethernet/ralink/mtk_eth_soc.c | 13 ++++++++++--- .../files/drivers/net/ethernet/ralink/soc_mt7620.c | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-)