netdev - Re: [PATCH net-next v8 08/11] net: stmmac: dwmac-loongson: Fix MAC speed for GNET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fg46ykzlyhw7vszgfaxkfkqe5la77clj2vcyrxo6f2irjod3gq@xdrlg4h7hzbu>
Date: Tue, 6 Feb 2024 00:55:25 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com, 
	chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 08/11] net: stmmac: dwmac-loongson: Fix MAC
 speed for GNET

On Tue, Jan 30, 2024 at 04:48:20PM +0800, Yanteng Si wrote:
> Current GNET on LS7A only supports ANE when speed is
> set to 1000M.

If so you need to merge it into the patch
[PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support

> 
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 19 +++++++++++++++++++
>  .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  6 ++++++
>  include/linux/stmmac.h                        |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index 60d0a122d7c9..264c4c198d5a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -344,6 +344,21 @@ static struct stmmac_pci_info loongson_gmac_pci_info = {
>  	.config = loongson_gmac_config,
>  };
>  
> +static void loongson_gnet_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> +{
> +	struct loongson_data *ld = (struct loongson_data *)priv;
> +	struct net_device *ndev = dev_get_drvdata(ld->dev);
> +	struct stmmac_priv *ptr = netdev_priv(ndev);
> +

> +	/* The controller and PHY don't work well together.

So there _is_ a PHY. What is the interface between MAC and PHY then?

> +	 * We need to use the PS bit to check if the controller's status
> +	 * is correct and reset PHY if necessary.
> +	 */

> +	if (speed == SPEED_1000)
> +		if (readl(ptr->ioaddr + MAC_CTRL_REG) & (1 << 15) /* PS */)
> +			phy_restart_aneg(ndev->phydev);

1. Please add curly braces for the outer if-statement.
2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.
3. How is the AN-restart helps? PHY-reset is done in
stmmac_init_phy()->phylink_connect_phy()->... a bit earlier than
this is called in the framework of the stmmac_mac_link_up() callback.
Wouldn't that restart AN too?

> +}
> +
>  static struct mac_device_info *loongson_setup(void *apriv)
>  {
>  	struct stmmac_priv *priv = apriv;
> @@ -401,6 +416,7 @@ static int loongson_gnet_data(struct pci_dev *pdev,
>  	plat->phy_interface = PHY_INTERFACE_MODE_INTERNAL;
>  
>  	plat->bsp_priv = &pdev->dev;
> +	plat->fix_mac_speed = loongson_gnet_fix_speed;
>  
>  	plat->dma_cfg->pbl = 32;
>  	plat->dma_cfg->pblx8 = true;
> @@ -416,6 +432,9 @@ static int loongson_gnet_config(struct pci_dev *pdev,
>  				struct stmmac_resources *res,
>  				struct device_node *np)
>  {

> +	if (pdev->revision == 0x00 || pdev->revision == 0x01)
> +		plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000;
> +

This should be in 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_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 42d27b97dd1d..31068fbc23c9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev,
>  		return 0;
>  	}
>  

> +	if (FIELD_GET(STMMAC_FLAG_DISABLE_FORCE_1000, priv->plat->flags)) {

FIELD_GET()?

> +		if (cmd->base.speed == SPEED_1000 &&
> +		    cmd->base.autoneg != AUTONEG_ENABLE)
> +			return -EOPNOTSUPP;
> +	}
> +
>  	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
>  }
>  
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index dee5ad6e48c5..2810361e4048 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -221,6 +221,7 @@ struct dwmac4_addrs {
>  #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI		BIT(10)
>  #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)

Detach the change introducing the STMMAC_FLAG_DISABLE_FORCE_1000 flag
into a separate patch a place it before
[PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support
as a pre-requisite/preparation patch.
Don't forget a _detailed_ description of why it's necessary, what is
wrong with GNET so 1G speed doesn't work without AN.

-Serge(y)

>  
>  struct plat_stmmacenet_data {
>  	int bus_id;
> -- 
> 2.31.4
>