Re: [PATCH net-next v8 09/11] net: stmmac: dwmac-loongson: Fix half duplex

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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@loongson.cn>
> > > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
> > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.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;
>  }
> 
> +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;

if that is all you support. Do you not support any pause modes (they
would need to be included as well here.)

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!