| Message ID | d142b909d0600b67b9ceadc767c4177be216f5bd.1720512888.git.0x1207@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: refactor FPE for gmac4 and xgmac | expand |
On Tue, Jul 09, 2024 at 04:21:19PM +0800, Furong Xu wrote: > The FPE support for xgmac is incomplete, drop it temporarily. > Once FPE implementation is refactored, xgmac support will be added. This is a pretty unusual thing to do. What does the current implementation do? Is there enough for it to actually work? If i was doing a git bisect and landed on this patch, could i find my networking is broken? More normal is to build a new implementation by the side, and then swap to it. Andrew
Hi Andrew, Furong, On Tue, Jul 09, 2024 at 03:16:35PM +0200, Andrew Lunn wrote: > On Tue, Jul 09, 2024 at 04:21:19PM +0800, Furong Xu wrote: > > The FPE support for xgmac is incomplete, drop it temporarily. > > Once FPE implementation is refactored, xgmac support will be added. > > This is a pretty unusual thing to do. What does the current > implementation do? Is there enough for it to actually work? If i was > doing a git bisect and landed on this patch, could i find my > networking is broken? > > More normal is to build a new implementation by the side, and then > swap to it. > > Andrew > There were 2 earlier attempts from Jianheng Zhang @ Synopsys to add FPE support to new hardware. I told him that the #1 priority should be to move the stmmac driver over to the new standard API which uses ethtool + tc. https://lore.kernel.org/netdev/CY5PR12MB63726FED738099761A9B81E7BF8FA@CY5PR12MB6372.namprd12.prod.outlook.com/ https://lore.kernel.org/netdev/CY5PR12MB63727C24923AE855CFF0D425BF8EA@CY5PR12MB6372.namprd12.prod.outlook.com/ I'm not sure what happened in the meantime. Jianheng must have faced some issue, because he never came back. I did comment this at the time: | Even this very patch is slightly strange - it is not brand new hardware | support, but it fills in some more FPE ops in dwxlgmac2_ops - when | dwxgmac3_fpe_configure() was already there. So this suggests the | existing support was incomplete. How complete is it now? No way to tell. | There is a selftest to tell, but we can't run it because the driver | doesn't integrate with those kernel APIs. So it is relatively known that the support is incomplete. But I still think we should push for more reviewer insight into this driver by having access to a selftest to get a clearer picture of how it behaves. For that, we need the compliance to the common API. Otherwise, I completely agree to the idea that any development should be done incrementally on top of whatever already exists, instead of putting a curtain on and then taking it back off.
Hi Vladimir On Tue, 9 Jul 2024 20:10:18 +0300, Vladimir Oltean <olteanv@gmail.com> wrote: > Hi Andrew, Furong, > > On Tue, Jul 09, 2024 at 03:16:35PM +0200, Andrew Lunn wrote: > > On Tue, Jul 09, 2024 at 04:21:19PM +0800, Furong Xu wrote: > > > The FPE support for xgmac is incomplete, drop it temporarily. > > > Once FPE implementation is refactored, xgmac support will be added. > > > > This is a pretty unusual thing to do. What does the current > > implementation do? Is there enough for it to actually work? If i was > > doing a git bisect and landed on this patch, could i find my > > networking is broken? > > > > More normal is to build a new implementation by the side, and then > > swap to it. > > > > Andrew > > > > There were 2 earlier attempts from Jianheng Zhang @ Synopsys to add FPE > support to new hardware. > > I told him that the #1 priority should be to move the stmmac driver over > to the new standard API which uses ethtool + tc. > https://lore.kernel.org/netdev/CY5PR12MB63726FED738099761A9B81E7BF8FA@CY5PR12MB6372.namprd12.prod.outlook.com/ > https://lore.kernel.org/netdev/CY5PR12MB63727C24923AE855CFF0D425BF8EA@CY5PR12MB6372.namprd12.prod.outlook.com/ > > I'm not sure what happened in the meantime. Jianheng must have faced > some issue, because he never came back. > > I did comment this at the time: > > | Even this very patch is slightly strange - it is not brand new hardware > | support, but it fills in some more FPE ops in dwxlgmac2_ops - when > | dwxgmac3_fpe_configure() was already there. So this suggests the > | existing support was incomplete. How complete is it now? No way to tell. > | There is a selftest to tell, but we can't run it because the driver > | doesn't integrate with those kernel APIs. > > So it is relatively known that the support is incomplete. But I still > think we should push for more reviewer insight into this driver by > having access to a selftest to get a clearer picture of how it behaves. > For that, we need the compliance to the common API. > After some searching and learning about your commits for FPE using the generic framework, I think it is clear enough to me to implement the new standard driver interface which uses ethtool + tc, and then the refactor of low level FPE function can follow. Thanks.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h index 6a2c7d22df1e..917796293c26 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h @@ -193,8 +193,6 @@ #define XGMAC_MDIO_ADDR 0x00000200 #define XGMAC_MDIO_DATA 0x00000204 #define XGMAC_MDIO_C22P 0x00000220 -#define XGMAC_FPE_CTRL_STS 0x00000280 -#define XGMAC_EFPE BIT(0) #define XGMAC_ADDRx_HIGH(x) (0x00000300 + (x) * 0x8) #define XGMAC_ADDR_MAX 32 #define XGMAC_AE BIT(31) diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c index 6a987cf598e4..e5bc3957041e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c @@ -1505,31 +1505,6 @@ static void dwxgmac2_set_arp_offload(struct mac_device_info *hw, bool en, writel(value, ioaddr + XGMAC_RX_CONFIG); } -static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg, - u32 num_txq, - u32 num_rxq, bool enable) -{ - u32 value; - - if (!enable) { - value = readl(ioaddr + XGMAC_FPE_CTRL_STS); - - value &= ~XGMAC_EFPE; - - writel(value, ioaddr + XGMAC_FPE_CTRL_STS); - return; - } - - value = readl(ioaddr + XGMAC_RXQ_CTRL1); - value &= ~XGMAC_RQ; - value |= (num_rxq - 1) << XGMAC_RQ_SHIFT; - writel(value, ioaddr + XGMAC_RXQ_CTRL1); - - value = readl(ioaddr + XGMAC_FPE_CTRL_STS); - value |= XGMAC_EFPE; - writel(value, ioaddr + XGMAC_FPE_CTRL_STS); -} - const struct stmmac_ops dwxgmac210_ops = { .core_init = dwxgmac2_core_init, .set_mac = dwxgmac2_set_mac, @@ -1570,7 +1545,6 @@ const struct stmmac_ops dwxgmac210_ops = { .config_l3_filter = dwxgmac2_config_l3_filter, .config_l4_filter = dwxgmac2_config_l4_filter, .set_arp_offload = dwxgmac2_set_arp_offload, - .fpe_configure = dwxgmac3_fpe_configure, }; static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode, @@ -1627,7 +1601,6 @@ const struct stmmac_ops dwxlgmac2_ops = { .config_l3_filter = dwxgmac2_config_l3_filter, .config_l4_filter = dwxgmac2_config_l4_filter, .set_arp_offload = dwxgmac2_set_arp_offload, - .fpe_configure = dwxgmac3_fpe_configure, }; int dwxgmac2_setup(struct stmmac_priv *priv)
The FPE support for xgmac is incomplete, drop it temporarily. Once FPE implementation is refactored, xgmac support will be added. Signed-off-by: Furong Xu <0x1207@gmail.com> --- .../net/ethernet/stmicro/stmmac/dwxgmac2.h | 2 -- .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 27 ------------------- 2 files changed, 29 deletions(-)