Date: Tue, 19 Mar 2024 20:02:38 +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 Thu, Mar 14, 2024 at 09:18:15PM +0800, Yanteng Si wrote:
> 
> 在 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 are talking about GNETs in this method since it has the
loongson_gnet_ prefix. You are referring to GMAC. I am getting
confused about all of these. Based on the patch 06/11 of this series
you call "Loongson GNET" of all the devices placed on the PCI devices
with PCI ID 0x7a13. PCIe device with ID 0x7a03 is called "Loongson
GMAC". Right?

Anyway no matter whether the PHY is placed externally or inside the
chip. AFAIU as long as you know the interface type between MAC and PHY
it would be better to have it specified.

> > >> +	 * 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.

Then please describe the bug in more details then.

Getting back to the code you implemented here. In the in-situ comment
you say: "We need to use the PS bit to check if the controller's
status is correct and reset PHY if necessary." By calling
phy_restart_aneg() you don't reset the PHY.

Moreover if "PS" flag is set, then the MAC has been pre-configured to
work in the 10/100Mbps mode. Since 1000Mbps speed is requested, the
MAC_CTRL_REG.PS flag will be cleared later in the
stmmac_mac_link_up() method and then phylink_start() shall cause the
link speed re-auto-negotiation. Why do you need the auto-negotiation
started for the default MAC config which will be changed just in a
moment later? All of that seems weird.

Most importantly I have doubts the networking subsystem maintainers
will permit you calling the phy_restart_aneg() method from the MAC
driver code.

> > 
> > >
> > >> +}
> > >> +
> > >>   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) {

it's better to change the order of the operands:
	if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) {

-Serge(y)

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