Re: [PATCH v4 11/11] net: stmmac: dwmac-loongson: Add GNET support

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

On Tue, Aug 22, 2023 at 05:41:19PM +0800, Feiyang Chen wrote:
> @@ -192,6 +192,86 @@ static struct stmmac_pci_info loongson_gmac_pci_info = {
>  	.config = loongson_gmac_config,
>  };
>  
> +static void loongson_gnet_fix_speed(void *priv, unsigned int speed)
> +{

What tree are these patches against? They don't look like the net-next
tree, since net-next has changed the prototype for the fix_mac_speed
method. Also, it would be good for it to be called
"loongson_gnet_fix_mac_speed" so that grepping for "fix_mac_speed"
finds not only the callsite but also the method definitions too.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a98bcd797720..fa4d7b90c5fa 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1242,7 +1242,8 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
>  	}
>  
>  	/* Half-Duplex can only work with single queue */
> -	if (priv->plat->tx_queues_to_use > 1)
> +	if (priv->plat->tx_queues_to_use > 1 ||
> +	    FIELD_GET(DWEGMAC_DISABLE_HALF_DUPLEX, priv->plat->dwegmac_flags))
>  		priv->phylink_config.mac_capabilities &=
>  			~(MAC_10HD | MAC_100HD | MAC_1000HD);

There have been a number of shortcomings of stmmac's setup of the
phylink MAC capabilities recently, and I think it's getting to the
point where we need someone to bite the bullet and do something
about it.

Constantly extending conditions such as the above doesn't make a
lot of sense.

In patch 9, you've added:

+       priv->phylink_config.mac_capabilities = MAC_10 | MAC_100;
+       if (!FIELD_GET(DWEGMAC_DISABLE_FLOW_CONTROL, priv->plat->dwegmac_flags))
+               priv->phylink_config.mac_capabilities |=
+                       MAC_ASYM_PAUSE | MAC_SYM_PAUSE;

The only two capabilities that are unconditional are MAC_10FD and
MAC_100FD. Everything else is conditional on something.

I'm thinking two things:

1) the stmmac platform implementations should be responsible for
   initialising priv->phylink_config.mac_capabilities

2) phylink may need a function:
   phylink_set_max_speed(struct phylink_config *config, u32 max_speed);
   which does similar to phy_set_max_speed(), but operates on
   mac_capabilities. This can then be used after calling the platform
   setup of priv->phylink_config.mac_capabilities to limit the
   speed. I'm not entirely sure that this is required though. I don't
   think it's required for dwmac-intel, but would possibly be needed
   for stmmac_platform since there's a "max-speed" property in DT.

What I'm basically saying is... let's not make the setup of
mac_capabilities any more convoluted than it already is by adding
new flags which only exist to set or clear other bits.

Also, let's try to stick to positive logic where possible!

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