Date: Wed, 20 Mar 2024 13:10:04 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Yanteng Si <siyanteng@...ngson.cn>, andrew@...n.ch, 
	hkallweit1@...il.com, peppe.cavallaro@...com, alexandre.torgue@...s.st.com, 
	joabreu@...opsys.com, Jose.Abreu@...opsys.com, chenhuacai@...ngson.cn, 
	guyinggang@...ngson.cn, netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 09/11] net: stmmac: dwmac-loongson: Fix half
 duplex

On Thu, Mar 14, 2024 at 09:08:41PM +0800, Yanteng Si wrote:
> 
> 在 2024/3/13 18:21, Russell King (Oracle) 写道:
> > On Wed, Mar 13, 2024 at 05:24:52PM +0800, Yanteng Si wrote:
> > > 在 2024/2/6 06:06, Serge Semin 写道:
> > > > On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote:
> > > > > On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote:
> > > > > > Current GNET does not support half duplex mode.
> > > > > > 
> > > > > > Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> > > > > > Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> > > > > > Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> > > > > > ---
> > > > > >    drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++-
> > > > > >    drivers/net/ethernet/stmicro/stmmac/stmmac_main.c    |  3 ++-
> > > > > >    include/linux/stmmac.h                               |  1 +
> > > > > >    3 files changed, 13 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > > > index 264c4c198d5a..1753a3c46b77 100644
> > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > > > @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev,
> > > > > >    				struct stmmac_resources *res,
> > > > > >    				struct device_node *np)
> > > > > >    {
> > > > > > -	if (pdev->revision == 0x00 || pdev->revision == 0x01)
> > > > > > +	switch (pdev->revision) {
> > > > > > +	case 0x00:
> > > > > > +		plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 |
> > > > > > +			       STMMAC_FLAG_DISABLE_HALF_DUPLEX;
> > > > > > +		break;
> > > > > > +	case 0x01:
> > > > > >    		plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000;
> > > > > > +		break;
> > > > > > +	default:
> > > > > > +		break;
> > > > > > +	}
> > > > > Move this change into the patch
> > > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support
> > > > > 
> > > > > >    	return 0;
> > > > > >    }
> > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > > index 5617b40abbe4..3aa862269eb0 100644
> > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > > @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev)
> > > > > >    static void stmmac_set_half_duplex(struct stmmac_priv *priv)
> > > > > >    {
> > > > > >    	/* Half-Duplex can only work with single tx queue */
> > > > > > -	if (priv->plat->tx_queues_to_use > 1)
> > > > > > +	if (priv->plat->tx_queues_to_use > 1 ||
> > > > > > +	    (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags))
> > > > > >    		priv->phylink_config.mac_capabilities &=
> > > > > >    			~(MAC_10HD | MAC_100HD | MAC_1000HD);
> > > > > >    	else
> > > > > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> > > > > > index 2810361e4048..197f6f914104 100644
> > > > > > --- a/include/linux/stmmac.h
> > > > > > +++ b/include/linux/stmmac.h
> > > > > > @@ -222,6 +222,7 @@ struct dwmac4_addrs {
> > > > > >    #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING	BIT(11)
> > > > > >    #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY	BIT(12)
> > > > > >    #define STMMAC_FLAG_DISABLE_FORCE_1000	BIT(13)
> > > > > > +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX	BIT(14)
> > > > > Place the patch with this change before
> > > > > [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support
> > > > > as a pre-requisite/preparation patch. Don't forget a thorough
> > > > > description of what is wrong with the GNET Half-Duplex mode.
> > > > BTW what about re-defining the stmmac_ops.phylink_get_caps() callback
> > > > instead of adding fixup flags in this patch and in the next one?
> > > ok.
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > index ac1b48ff7199..b57e1325ce62 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem
> > > *ioaddr,
> > >       return 0;
> > >   }
> 
> Hi Russell,
> 
> > > +static void loongson_phylink_get_caps(struct stmmac_priv *priv)
> > > +{
> > > +    priv->phylink_config.mac_capabilities = (MAC_10FD |
> > > +        MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD);
> > Why is this so complicated? It would be silly if the _full duplex_
> > definitions also defined the _half duplex_ bits. This should be just:
> > 
> > 	priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD |
> > 						MAC_1000FD;
> 
> Yes, you are right. Our gnet device (7a2000) does not support half-duplex,
> while the gnet device (2k2000) does.
> 
> I plan to use PCI IDand IP CORE as the condition to separate full-duplex and
> half-duplex.
> 
> > 
> > if that is all you support. Do you not support any pause modes (they
> > would need to be included as well here.)
> 
> I have tested it and our gnet device supports MAC_ASYM_PAUSE and
> MAC_SYM_PAUSE, but gmac does not. I will fix this in the patch v9.
> 
> This is also easy to do because all GMAC devices have the same PCI ID.
> 
> > 

> > As to this approach, I don't think it's a good model to override the
> > stmmac MAC operations. Instead, I would suggest that a better approach
> > would be for the platform to provide its capabilities to the stmmac
> > core code (maybe a new member in stmmac_priv) which, when set, is used
> > to reduce the capabilities provided to phylink via
> > priv->phylink_config.mac_capabilities.
> > 
> > Why? The driver has several components that are involved in the
> > overall capabilities, and the capabilities of the system is the
> > logical subset of all these capabilities. One component should not
> > be setting capabilities that a different component doesn't support.

In general you are right for sure - it's better to avoid one part
setting capabilities and another part unsetting them at least from the
readability and maintainability point of view. But in this case we've
already got implemented a ready-to-use internal interface
stmmac_ops::phylink_get_caps() which can be used to extend/reduce the
capabilities field based on the particular MAC abilities. Moreover
it's called right from the component setting the capabilities. Are you
saying that the callback is supposed to be utilized for extending the
capabilities only?

If you insist on not overriding the stmmac_ops::phylink_get_caps()
anyway then please explain what is the principal difference
between the next two code snippets:
	/* Get the MAC specific capabilities */
        stmmac_mac_phylink_get_caps(priv);
and
	priv->phylink_config.mac_capabilities &= ~priv->plat->mac_caps_mask;
in the MAC-capabilities update implementation? Do you think the later
approach would be more descriptive? If so then would the
callback-based approach almost equally descriptive if the callback
name was, suppose, stmmac_mac_phylink_set_caps() or similar?

In anyway I am sure the approach suggested in the initial patch of
this thread isn't good since it motivates the developers to implement
more-and-more DW MAC-specific platform capabilities flags fixing
another flags, which makes the generic code even more complicated
than it already is with endless if-else-plat-flags statements.

-Serge(y)

> 
> Hi Serge,
> 
> It seems to be going back again, what do you think?
> 
> Thanks,
> 
> Yanteng
> 
> > 
> 
> 

Powered by blists - more mailing lists