Loongson's GMAC device takes nearly two seconds to complete DMA reset,
however, the default waiting time for reset is 200 milliseconds

Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
---
 .../net/ethernet/stmicro/stmmac/dwmac-loongson.c    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index bfe6e2d631bd..35639d26256c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev,
 	return 0;
 }
 
+static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr)
+{
+	u32 value = readl(ioaddr + DMA_BUS_MODE);
+
+	value |= DMA_BUS_MODE_SFT_RESET;
+	writel(value, ioaddr + DMA_BUS_MODE);
+
+	return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
+				  !(value & DMA_BUS_MODE_SFT_RESET),
+				  10000, 2000000);
+}
+
 static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct plat_stmmacenet_data *plat;
@@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 
 	plat->bsp_priv = ld;
 	plat->setup = loongson_dwmac_setup;
+	plat->fix_soc_reset = loongson_fix_soc_reset;
 	ld->dev = &pdev->dev;
 	ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
 

base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
-- 
2.43.0

在 1/21/25 16:25, Qunqin Zhao 写道:
> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
> however, the default waiting time for reset is 200 milliseconds
Is only GMAC like this?
>
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac-loongson.c    | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index bfe6e2d631bd..35639d26256c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev,
>   	return 0;
>   }
>   
How about putting a part of the commit message here as a comment?
> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr)


> +{
> +	u32 value = readl(ioaddr + DMA_BUS_MODE);
> +
> +	value |= DMA_BUS_MODE_SFT_RESET;
> +	writel(value, ioaddr + DMA_BUS_MODE);
> +
> +	return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
> +				  !(value & DMA_BUS_MODE_SFT_RESET),
> +				  10000, 2000000);
> +}
> +
>   static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
>   	struct plat_stmmacenet_data *plat;
> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>   
>   	plat->bsp_priv = ld;
>   	plat->setup = loongson_dwmac_setup;
> +	plat->fix_soc_reset = loongson_fix_soc_reset;

If only GMAC needs to be done this way, how about putting it inside the 
loongson_gmac_data()?

Thanks,

Yanteng

在 2025/1/21 下午9:41, Yanteng Si 写道:
>
> 在 1/21/25 16:25, Qunqin Zhao 写道:
>> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
>> however, the default waiting time for reset is 200 milliseconds
> Is only GMAC like this?
At present, this situation has only been found on GMAC.
>>
>> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
>> ---
>>   .../net/ethernet/stmicro/stmmac/dwmac-loongson.c    | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c 
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> index bfe6e2d631bd..35639d26256c 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct 
>> pci_dev *pdev,
>>       return 0;
>>   }
> How about putting a part of the commit message here as a comment?
Sure, will do that.
>> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr)
>
>
>> +{
>> +    u32 value = readl(ioaddr + DMA_BUS_MODE);
>> +
>> +    value |= DMA_BUS_MODE_SFT_RESET;
>> +    writel(value, ioaddr + DMA_BUS_MODE);
>> +
>> +    return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
>> +                  !(value & DMA_BUS_MODE_SFT_RESET),
>> +                  10000, 2000000);
>> +}
>> +
>>   static int loongson_dwmac_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *id)
>>   {
>>       struct plat_stmmacenet_data *plat;
>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev 
>> *pdev, const struct pci_device_id
>>         plat->bsp_priv = ld;
>>       plat->setup = loongson_dwmac_setup;
>> +    plat->fix_soc_reset = loongson_fix_soc_reset;
>
> If only GMAC needs to be done this way, how about putting it inside 
> the loongson_gmac_data()?

Regardless of whether this situation occurs in other devices(like gnet), 
this change will not have any impact on gnet, right?

Thanks.

>
> Thanks,
>
> Yanteng

在 2025/1/22 09:31, Qunqin Zhao 写道:
>
> 在 2025/1/21 下午9:41, Yanteng Si 写道:
>>
>> 在 1/21/25 16:25, Qunqin Zhao 写道:
>>> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
>>> however, the default waiting time for reset is 200 milliseconds
>> Is only GMAC like this?
> At present, this situation has only been found on GMAC.

>>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev 
>>> *pdev, const struct pci_device_id
>>>         plat->bsp_priv = ld;
>>>       plat->setup = loongson_dwmac_setup;
>>> +    plat->fix_soc_reset = loongson_fix_soc_reset;
>>
>> If only GMAC needs to be done this way, how about putting it inside 
>> the loongson_gmac_data()?
>
> Regardless of whether this situation occurs in other devices(like 
> gnet), this change will not have any impact on gnet, right?
>
Yeah，However, it is obvious that there is now a more suitable
place for it. In the Loongson driver, `loongson_gmac_data()`
and `loongson_default_data()` were designed from the beginning.
When GNET support was added later, `loongson_gnet_data()`
was designed. We once made great efforts to extract these codes
from the `probe()` . Are we going to go back to the past?

Of course, I'm not saying that I disagree with you fixing the
GMAC in the `probe()`. I just think it's a bad start. After that,
other people may also put similar code here, and eventually
it will make the `probe` a mess.

If you insist on doing this, please change the function name
to `loongson_gmac_fix_reset()`, just like `loongson_gnet_fix_speed`.


Thanks,
Yanteng

在 2025/1/22 下午4:53, Yanteng Si 写道:
>
>
>
> 在 2025/1/22 09:31, Qunqin Zhao 写道:
>>
>> 在 2025/1/21 下午9:41, Yanteng Si 写道:
>>>
>>> 在 1/21/25 16:25, Qunqin Zhao 写道:
>>>> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
>>>> however, the default waiting time for reset is 200 milliseconds
>>> Is only GMAC like this?
>> At present, this situation has only been found on GMAC.
>
>>>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev 
>>>> *pdev, const struct pci_device_id
>>>>         plat->bsp_priv = ld;
>>>>       plat->setup = loongson_dwmac_setup;
>>>> +    plat->fix_soc_reset = loongson_fix_soc_reset;
>>>
>>> If only GMAC needs to be done this way, how about putting it inside 
>>> the loongson_gmac_data()?
>>
>> Regardless of whether this situation occurs in other devices(like 
>> gnet), this change will not have any impact on gnet, right?
>>
> Yeah，However, it is obvious that there is now a more suitable
> place for it. In the Loongson driver, `loongson_gmac_data()`
> and `loongson_default_data()` were designed from the beginning.
> When GNET support was added later, `loongson_gnet_data()`
> was designed. We once made great efforts to extract these codes
> from the `probe()` . Are we going to go back to the past?
>
> Of course, I'm not saying that I disagree with you fixing the
> GMAC in the `probe()`. I just think it's a bad start. After that,
> other people may also put similar code here, and eventually
> it will make the `probe` a mess.
>
> If you insist on doing this, please change the function name
> to `loongson_gmac_fix_reset()`, just like `loongson_gnet_fix_speed`.

Recently, it is found that GNET may also have a long DMA reset time.  
And the commit

message should be "Loongson's DWMAC device may take nearly two seconds 
to complete DMA reset,
however, the default waiting time for reset is 200 milliseconds".

Thanks.

>
>
> Thanks,
> Yanteng

在 2/6/25 3:22 PM, Qunqin Zhao 写道:
>
> 在 2025/1/22 下午4:53, Yanteng Si 写道:
>>
>>
>>
>> 在 2025/1/22 09:31, Qunqin Zhao 写道:
>>>
>>> 在 2025/1/21 下午9:41, Yanteng Si 写道:
>>>>
>>>> 在 1/21/25 16:25, Qunqin Zhao 写道:
>>>>> Loongson's GMAC device takes nearly two seconds to complete DMA 
>>>>> reset,
>>>>> however, the default waiting time for reset is 200 milliseconds
>>>> Is only GMAC like this?
>>> At present, this situation has only been found on GMAC.
>>
>>>>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev 
>>>>> *pdev, const struct pci_device_id
>>>>>         plat->bsp_priv = ld;
>>>>>       plat->setup = loongson_dwmac_setup;
>>>>> +    plat->fix_soc_reset = loongson_fix_soc_reset;
>>>>
>>>> If only GMAC needs to be done this way, how about putting it inside 
>>>> the loongson_gmac_data()?
>>>
>>> Regardless of whether this situation occurs in other devices(like 
>>> gnet), this change will not have any impact on gnet, right?
>>>
>> Yeah，However, it is obvious that there is now a more suitable
>> place for it. In the Loongson driver, `loongson_gmac_data()`
>> and `loongson_default_data()` were designed from the beginning.
>> When GNET support was added later, `loongson_gnet_data()`
>> was designed. We once made great efforts to extract these codes
>> from the `probe()` . Are we going to go back to the past?
>>
>> Of course, I'm not saying that I disagree with you fixing the
>> GMAC in the `probe()`. I just think it's a bad start. After that,
>> other people may also put similar code here, and eventually
>> it will make the `probe` a mess.
>>
>> If you insist on doing this, please change the function name
>> to `loongson_gmac_fix_reset()`, just like `loongson_gnet_fix_speed`.
>
> Recently, it is found that GNET may also have a long DMA reset time.  
> And the commit

It seems that you haven't tested all the gnet devices. Please test all

the devices before sending v2. Since I've left Loongson, I only have

the 7A2000(gnet) device at hand to assist with the testing.

>
> message should be "Loongson's DWMAC device may take nearly two seconds 
> to complete DMA reset,
> however, the default waiting time for reset is 200 milliseconds".

The function name can refer to loongson_dwmac_setup.

       plat->setup = loongson_dwmac_setup;

+    plat->fix_soc_reset = loongson_dwmac_fix_reset;


Oh, don't forget the necessary comments.


Thanks

Yanteng

Hi, Qunqin,

The patch itself looks good to me, but something can be improved.
1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
2. You lack a "." at the end of the commit message.
3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
to 6.12/6.13.

After that you can add:
Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>


Huacai

On Tue, Jan 21, 2025 at 4:26 PM Qunqin Zhao <zhaoqunqin@loongson.cn> wrote:
>
> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
> however, the default waiting time for reset is 200 milliseconds
>
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-loongson.c    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index bfe6e2d631bd..35639d26256c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev,
>         return 0;
>  }
>
> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr)
> +{
> +       u32 value = readl(ioaddr + DMA_BUS_MODE);
> +
> +       value |= DMA_BUS_MODE_SFT_RESET;
> +       writel(value, ioaddr + DMA_BUS_MODE);
> +
> +       return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
> +                                 !(value & DMA_BUS_MODE_SFT_RESET),
> +                                 10000, 2000000);
> +}
> +
>  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>         struct plat_stmmacenet_data *plat;
> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>
>         plat->bsp_priv = ld;
>         plat->setup = loongson_dwmac_setup;
> +       plat->fix_soc_reset = loongson_fix_soc_reset;
>         ld->dev = &pdev->dev;
>         ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
>
>
> base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
> --
> 2.43.0
>

在 2025/1/21 下午5:29, Huacai Chen 写道:
> Hi, Qunqin,
>
> The patch itself looks good to me, but something can be improved.
> 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
> 2. You lack a "." at the end of the commit message.
> 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
> to 6.12/6.13.
>
> After that you can add:
> Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
OK, Thanks
>
>
> Huacai
>
> On Tue, Jan 21, 2025 at 4:26 PM Qunqin Zhao <zhaoqunqin@loongson.cn> wrote:
>> Loongson's GMAC device takes nearly two seconds to complete DMA reset,
>> however, the default waiting time for reset is 200 milliseconds
>>
>> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
>> ---
>>   .../net/ethernet/stmicro/stmmac/dwmac-loongson.c    | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> index bfe6e2d631bd..35639d26256c 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> @@ -516,6 +516,18 @@ static int loongson_dwmac_acpi_config(struct pci_dev *pdev,
>>          return 0;
>>   }
>>
>> +static int loongson_fix_soc_reset(void *priv, void __iomem *ioaddr)
>> +{
>> +       u32 value = readl(ioaddr + DMA_BUS_MODE);
>> +
>> +       value |= DMA_BUS_MODE_SFT_RESET;
>> +       writel(value, ioaddr + DMA_BUS_MODE);
>> +
>> +       return readl_poll_timeout(ioaddr + DMA_BUS_MODE, value,
>> +                                 !(value & DMA_BUS_MODE_SFT_RESET),
>> +                                 10000, 2000000);
>> +}
>> +
>>   static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>>          struct plat_stmmacenet_data *plat;
>> @@ -566,6 +578,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>
>>          plat->bsp_priv = ld;
>>          plat->setup = loongson_dwmac_setup;
>> +       plat->fix_soc_reset = loongson_fix_soc_reset;
>>          ld->dev = &pdev->dev;
>>          ld->loongson_id = readl(res.addr + GMAC_VERSION) & 0xff;
>>
>>
>> base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
>> --
>> 2.43.0
>>

On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote:
> Hi, Qunqin,
> 
> The patch itself looks good to me, but something can be improved.
> 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
> 2. You lack a "." at the end of the commit message.
> 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
> to 6.12/6.13.

Please don't top post.

Hi, Jakub,

On Wed, Jan 22, 2025 at 2:11 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote:
> > Hi, Qunqin,
> >
> > The patch itself looks good to me, but something can be improved.
> > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
> > 2. You lack a "." at the end of the commit message.
> > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
> > to 6.12/6.13.
>
> Please don't top post.
I know about inline replies, but in this case I agree with the code
itself so I cannot reply after the code.

Huacai

On Wed, 22 Jan 2025 12:09:28 +0800 Huacai Chen wrote:
> Subject: Re: [PATCH] net: stmmac: dwmac-loongson: Add fix_soc_reset function
> 
> On Wed, Jan 22, 2025 at 2:11 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Tue, 21 Jan 2025 17:29:37 +0800 Huacai Chen wrote:  
> > > Hi, Qunqin,
> > >
> > > The patch itself looks good to me, but something can be improved.
> > > 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
> > > 2. You lack a "." at the end of the commit message.
> > > 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
> > > to 6.12/6.13.  
> >
> > Please don't top post.  
> I know about inline replies, but in this case I agree with the code
> itself so I cannot reply after the code.

You're not trying hard enough. The message is Subject, body and code.
Reply in the place where the problem is or where the CC stable should
be added.

在 1/21/25 17:29, Huacai Chen 写道:
> Hi, Qunqin,
>
> The patch itself looks good to me, but something can be improved.
> 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() callback"
> 2. You lack a "." at the end of the commit message.

> 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
> to 6.12/6.13.

Then we also need to have a fixes tag.

Thanks,

Yanteng

在 2025/1/21 下午9:47, Yanteng Si 写道:
>
> 在 1/21/25 17:29, Huacai Chen 写道:
>> Hi, Qunqin,
>>
>> The patch itself looks good to me, but something can be improved.
>> 1. The title can be "net: stmmac: dwmac-loongson: Add fix_soc_reset() 
>> callback"
>> 2. You lack a "." at the end of the commit message.
>
>> 3. Add a "Cc: stable@vger.kernel.org" because it is needed to backport
>> to 6.12/6.13.
>
> Then we also need to have a fixes tag.
OK, thanks.
>
> Thanks,
>
> Yanteng
>