Date: Thu, 14 Mar 2024 21:18:15 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
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


在 2024/3/14 17:43, Yanteng Si 写道:
> 在 2024/2/6 05:55, Serge Semin 写道:
> > 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

>> 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

OK.

> >
> >> 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?
> >
> > GMAC only has a MAC chip inside the chip and needs an external PHY chip; GNET 
> > has the PHY chip inside the chip.
> >> +	 * 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.
> OK,
> > 2. MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.
>
> OK.
>
> if(speed==SPEED_1000){
> /*MAC_CTRL_REG.15 is defined by the GMAC_CONTROL_PS macro.*/
> if(readl(ptr->ioaddr+MAC_CTRL_REG) &(1<<15))
> phy_restart_aneg(ndev->phydev);
> }
>
> > 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?
>
> Due to a bug in the chip's internal PHY, the network is still not working after
> the first self-negotiation, and it needs to be self-negotiated again.
>
> >
> >> +}
> >> +
> >>   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
> OK.
> >
> >>   	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()?
>
> OK,
>
> if (STMMAC_FLAG_DISABLE_FORCE_1000 & priv->plat->flags) {
>
> >
> >> +		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.
>
> OK.
>
>
> Thanks,
>
> Yanteng
>
> >
> > -Serge(y)
> >
> >>   
> >>   struct plat_stmmacenet_data {
> >>   	int bus_id;
> >> -- 
> >> 2.31.4
> >>

Powered by blists - more mailing lists