[PATCH net] net: ngbe: Fix phy mode set to external phy

[PATCH net] net: ngbe: Fix phy mode set to external phy

[Mengyuan Lou]

When use rgmmi to attach to external phy, set
PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
And it is does matter to internal phy.

Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
 drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
index ba33a57b42c2..be99ef5833da 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
@@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx)
 	ret = phy_connect_direct(wx->netdev,
 				 wx->phydev,
 				 ngbe_handle_link_change,
-				 PHY_INTERFACE_MODE_RGMII_ID);
+				 PHY_INTERFACE_MODE_RGMII_RXID);
 	if (ret) {
 		wx_err(wx, "PHY connect failed.\n");
 		return ret;
-- 
2.43.2

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[Przemek Kitszel]

On 8/6/24 10:25, Mengyuan Lou wrote:
> When use rgmmi to attach to external phy, set
> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
> And it is does matter to internal phy.
> 

  107│  * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent 
interface
  108│  * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
  109│  * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
  110│  * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay

Your change effectively disables Internal Tx delay, but your commit
message does not tell about that. It also does not tell about why,
nor what is wrong in current behavior.

> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")

This commit indeed has introduced the line you are changing,
but without explanation, this is not a bugfix.

> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> ---
>   drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> index ba33a57b42c2..be99ef5833da 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> @@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx)
>   	ret = phy_connect_direct(wx->netdev,
>   				 wx->phydev,
>   				 ngbe_handle_link_change,
> -				 PHY_INTERFACE_MODE_RGMII_ID);
> +				 PHY_INTERFACE_MODE_RGMII_RXID);
>   	if (ret) {
>   		wx_err(wx, "PHY connect failed.\n");
>   		return ret;

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[mengyuanlou]

> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道：
> 
> On 8/6/24 10:25, Mengyuan Lou wrote:
>> When use rgmmi to attach to external phy, set
>> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
>> And it is does matter to internal phy.
> 
> 107│  * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface
> 108│  * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
> 109│  * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
> 110│  * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay
> 
> Your change effectively disables Internal Tx delay, but your commit
> message does not tell about that. It also does not tell about why,
> nor what is wrong in current behavior.
> 

I will add it, when wangxun em Nics are used as a Mac to attach to external phy.
We should disable tx delay.


>> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
> 

Fixes: bc2426d74aa3 ("net: ngbe: convert phylib to phylink")

I just want to fix it both in a1cf597b99a7 and bc2426d74aa3 commits.
How can I do it.

> This commit indeed has introduced the line you are changing,
> but without explanation, this is not a bugfix.
> 
>> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
>> ---
>>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> index ba33a57b42c2..be99ef5833da 100644
>> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> @@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx)
>>   ret = phy_connect_direct(wx->netdev,
>>   wx->phydev,
>>   ngbe_handle_link_change,
>> - PHY_INTERFACE_MODE_RGMII_ID);
>> + PHY_INTERFACE_MODE_RGMII_RXID);
>>   if (ret) {
>>   wx_err(wx, "PHY connect failed.\n");
>>   return ret;
> 

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[Andrew Lunn]

On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote:
> 
> 
> > 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道：
> > 
> > On 8/6/24 10:25, Mengyuan Lou wrote:
> >> When use rgmmi to attach to external phy, set
> >> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
> >> And it is does matter to internal phy.
> > 
> > 107│  * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface
> > 108│  * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
> > 109│  * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
> > 110│  * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay
> > 
> > Your change effectively disables Internal Tx delay, but your commit
> > message does not tell about that. It also does not tell about why,
> > nor what is wrong in current behavior.
> > 
> 
> I will add it, when wangxun em Nics are used as a Mac to attach to external phy.
> We should disable tx delay.

Why should you disable TX delay?

What is providing that delay? Something needs to add a 2ns delay. Does
the PCB have an extra long clock line?

    Andrew

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[mengyuanlou]

> 2024年8月7日 20:58，Andrew Lunn <andrew@lunn.ch> 写道：
> 
> On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote:
>> 
>> 
>>> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道：
>>> 
>>> On 8/6/24 10:25, Mengyuan Lou wrote:
>>>> When use rgmmi to attach to external phy, set
>>>> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
>>>> And it is does matter to internal phy.
>>> 
>>> 107│  * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface
>>> 108│  * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
>>> 109│  * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
>>> 110│  * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay
>>> 
>>> Your change effectively disables Internal Tx delay, but your commit
>>> message does not tell about that. It also does not tell about why,
>>> nor what is wrong in current behavior.
>>> 
>> 
>> I will add it, when wangxun em Nics are used as a Mac to attach to external phy.
>> We should disable tx delay.
> 
> Why should you disable TX delay?
> 
> What is providing that delay? Something needs to add a 2ns delay. Does
> the PCB have an extra long clock line?
> 

Mac only has add the Tx delay，and it can not be modified.

So just disable TX delay in PHY.

Mengyuan Lou 

>    Andrew

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[Andrew Lunn]

On Thu, Aug 08, 2024 at 03:23:58PM +0800, mengyuanlou@net-swift.com wrote:
> 
> 
> > 2024年8月7日 20:58，Andrew Lunn <andrew@lunn.ch> 写道：
> > 
> > On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote:
> >> 
> >> 
> >>> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道：
> >>> 
> >>> On 8/6/24 10:25, Mengyuan Lou wrote:
> >>>> When use rgmmi to attach to external phy, set
> >>>> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
> >>>> And it is does matter to internal phy.
> >>> 
> >>> 107│  * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface
> >>> 108│  * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay
> >>> 109│  * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay
> >>> 110│  * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay
> >>> 
> >>> Your change effectively disables Internal Tx delay, but your commit
> >>> message does not tell about that. It also does not tell about why,
> >>> nor what is wrong in current behavior.
> >>> 
> >> 
> >> I will add it, when wangxun em Nics are used as a Mac to attach to external phy.
> >> We should disable tx delay.
> > 
> > Why should you disable TX delay?
> > 
> > What is providing that delay? Something needs to add a 2ns delay. Does
> > the PCB have an extra long clock line?
> > 
> 
> Mac only has add the Tx delay，and it can not be modified.
> 
> So just disable TX delay in PHY.

So slowly we are starting to understand the problem....

You need to document all this in the justification of the patch. This
asymmetric setup is also very unusual, so you should add a comment in
the code explaining it.

For Linux in general, we let the PHY add the delays, and if the MAC
can add delays, we generally don't make use of that ability. There are
a few exceptions, because there are a few PHY which lack the
capabilities to add delays. Anybody with a general Linux PHY
background are going to look at your code and question it, because it
is very odd. Hence the need for a comment.

	Andrew

[PATCH net] net: ngbe: Fix phy mode set to external phy

[Mengyuan Lou]

The MAC only has add the TX delay and it can not be modified.
MAC and PHY are both set the TX delay cause transmission problems.
So just disable TX delay in PHY, when use rgmii to attach to
external phy, set PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
And it is does not matter to internal phy.

Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
v2:
-Add a comment for the code modification.
-Add the problem in commit messages.
v1:
https://lore.kernel.org/netdev/C1587837D62D1BC0+20240806082520.29193-1-mengyuanlou@net-swift.com/

 drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
index ba33a57b42c2..0876b2e810c0 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
@@ -215,10 +215,14 @@ int ngbe_phy_connect(struct wx *wx)
 {
 	int ret;
 
+	/* The MAC only has add the Tx delay and it can not be modified.
+	 * So just disable TX delay in PHY, and it is does not matter to
+	 * internal phy.
+	 */
 	ret = phy_connect_direct(wx->netdev,
 				 wx->phydev,
 				 ngbe_handle_link_change,
-				 PHY_INTERFACE_MODE_RGMII_ID);
+				 PHY_INTERFACE_MODE_RGMII_RXID);
 	if (ret) {
 		wx_err(wx, "PHY connect failed.\n");
 		return ret;
-- 
2.43.2

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[Paolo Abeni]

On 8/12/24 12:30, Mengyuan Lou wrote:
> The MAC only has add the TX delay and it can not be modified.
> MAC and PHY are both set the TX delay cause transmission problems.
> So just disable TX delay in PHY, when use rgmii to attach to
> external phy, set PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
> And it is does not matter to internal phy.
> 
> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> ---
> v2:
> -Add a comment for the code modification.
> -Add the problem in commit messages.
> v1:
> https://lore.kernel.org/netdev/C1587837D62D1BC0+20240806082520.29193-1-mengyuanlou@net-swift.com/
> 
>   drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> index ba33a57b42c2..0876b2e810c0 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
> @@ -215,10 +215,14 @@ int ngbe_phy_connect(struct wx *wx)
>   {
>   	int ret;
>   
> +	/* The MAC only has add the Tx delay and it can not be modified.
> +	 * So just disable TX delay in PHY, and it is does not matter to
> +	 * internal phy.
> +	 */
>   	ret = phy_connect_direct(wx->netdev,
>   				 wx->phydev,
>   				 ngbe_handle_link_change,
> -				 PHY_INTERFACE_MODE_RGMII_ID);
> +				 PHY_INTERFACE_MODE_RGMII_RXID);
>   	if (ret) {
>   		wx_err(wx, "PHY connect failed.\n");
>   		return ret;

Does not apply cleanly to net since:

commit bc2426d74aa35cd8ec9c97a253ef57c2c5cd730c
Author: Jiawen Wu <jiawenwu@trustnetic.com>
Date:   Wed Jan 3 10:08:49 2024 +0800

     net: ngbe: convert phylib to phylink

Please rebase

Thanks,

Paolo

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[mengyuanlou]

> 2024年8月15日 18:15，Paolo Abeni <pabeni@redhat.com> 写道：
> 
> On 8/12/24 12:30, Mengyuan Lou wrote:
>> The MAC only has add the TX delay and it can not be modified.
>> MAC and PHY are both set the TX delay cause transmission problems.
>> So just disable TX delay in PHY, when use rgmii to attach to
>> external phy, set PHY_INTERFACE_MODE_RGMII_RXID to phy drivers.
>> And it is does not matter to internal phy.
>> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")
>> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
>> ---
>> v2:
>> -Add a comment for the code modification.
>> -Add the problem in commit messages.
>> v1:
>> https://lore.kernel.org/netdev/C1587837D62D1BC0+20240806082520.29193-1-mengyuanlou@net-swift.com/
>>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> index ba33a57b42c2..0876b2e810c0 100644
>> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
>> @@ -215,10 +215,14 @@ int ngbe_phy_connect(struct wx *wx)
>>  {
>>   int ret;
>>  + /* The MAC only has add the Tx delay and it can not be modified.
>> +  * So just disable TX delay in PHY, and it is does not matter to
>> +  * internal phy.
>> +  */
>>   ret = phy_connect_direct(wx->netdev,
>>    wx->phydev,
>>    ngbe_handle_link_change,
>> -  PHY_INTERFACE_MODE_RGMII_ID);
>> +  PHY_INTERFACE_MODE_RGMII_RXID);
>>   if (ret) {
>>   wx_err(wx, "PHY connect failed.\n");
>>   return ret;
> 
> Does not apply cleanly to net since:
> 
> commit bc2426d74aa35cd8ec9c97a253ef57c2c5cd730c
> Author: Jiawen Wu <jiawenwu@trustnetic.com>
> Date:   Wed Jan 3 10:08:49 2024 +0800
> 
>    net: ngbe: convert phylib to phylink
> 
> Please rebase

I Know there has something changed in this commit.

If I want to send this patch to fix the code between this two commits.
> commit: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")

> commit: bc2426d74aa3 (“net: ngbe: convert phylib to phylink")

Should I add the stable tag flag like 6.6 or do something else.

Thanks,
Mengyuan Lou.

Re: [PATCH net] net: ngbe: Fix phy mode set to external phy

[Jakub Kicinski]

On Fri, 16 Aug 2024 14:11:27 +0800 mengyuanlou@net-swift.com wrote:
> > Does not apply cleanly to net since:
> > 
> > commit bc2426d74aa35cd8ec9c97a253ef57c2c5cd730c
> > Author: Jiawen Wu <jiawenwu@trustnetic.com>
> > Date:   Wed Jan 3 10:08:49 2024 +0800
> > 
> >    net: ngbe: convert phylib to phylink
> > 
> > Please rebase  
> 
> I Know there has something changed in this commit.
> 
> If I want to send this patch to fix the code between this two commits.
> > commit: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.")  
> 
> > commit: bc2426d74aa3 (“net: ngbe: convert phylib to phylink")  
> 
> Should I add the stable tag flag like 6.6 or do something else.

If I understand the question correctly the answer is: don't worry about
stable right now.

Add the CC: stable@.. tag, and fix the code as it is in net/main.
Once the commit reaches Linus you will get a notification from 
the stable team that the patch has reached them and either does 
or does not apply. At which point you can provide them with a patch
adjusted to what's needed in LTS kernels.