Message-ID: <f714e017-4016-4230-bb81-1a9cfb749844@loongson.cn>
Date: Wed, 20 Mar 2024 18:42:04 +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/20 01:02, Serge Semin 写道:
> 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?

ls2k is SOC, ls7a ls bridge:


device    type    pci_id    gmac_version
ls7a1000  gmac    7a03      0x37
ls2k1000  gmac    7a03      0x37
ls7a2000  gnet    7a13      0x37
ls2k2000  gnet    7a13      0x10
>
> 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.

I will reply to you tomorrow.


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

OK,


Thanks,

Yanteng

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