[PATCH] net: phy: dp83867: perform soft reset and retain established link

[PATCH] net: phy: dp83867: perform soft reset and retain established link

[praneeth]

From: Praneeth Bajjuri <praneeth@ti.com>

Current logic is performing hard reset and causing the programmed
registers to be wiped out.

as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
8.6.26 Control Register (CTRL)
do SW_RESTART to perform a reset not including the registers and is
acceptable to do this if a link is already present.

Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
Signed-off-by: Geet Modi <geet.modi@ti.com>
---
 drivers/net/phy/dp83867.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 9bd9a5c0b1db..29ab49e894db 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -826,7 +826,7 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 {
 	int err;
 
-	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
+	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
 	if (err < 0)
 		return err;
 
-- 
2.17.1

Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Andrew Lunn]

On Tue, Mar 23, 2021 at 08:00:06PM -0500, praneeth@ti.com wrote:
> From: Praneeth Bajjuri <praneeth@ti.com>
> 
> Current logic is performing hard reset and causing the programmed
> registers to be wiped out.
> 
> as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> 8.6.26 Control Register (CTRL)
> do SW_RESTART to perform a reset not including the registers and is
> acceptable to do this if a link is already present.

I don't see any code here to determine if the like is present. What if
the cable is not plugged in?

> @@ -826,7 +826,7 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>  {
>  	int err;
>  
> -	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
> +	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
>  	if (err < 0)
>  		return err;

The code continues

       usleep_range(10, 20);

        /* After reset FORCE_LINK_GOOD bit is set. Although the
         * default value should be unset. Disable FORCE_LINK_GOOD
         * for the phy to work properly.
         */
        return phy_modify(phydev, MII_DP83867_PHYCTRL,
                         DP83867_PHYCR_FORCE_LINK_GOOD, 0);
}

Do you still need to clear the FORCE_LINK_GOOD bit after a restart?

   Andrew

Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Andrew Lunn]

>     > as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> 
>     > 8.6.26 Control Register (CTRL)
>     > do SW_RESTART to perform a reset not including the registers and is
>     > acceptable to do this if a link is already present.
> 
>  
> 
>     I don't see any code here to determine if the like is present. What if
>     the cable is not plugged in?
> 
>     This API is primarily used for reset. Link Status is checked thru different
> register. This shall not impact the cable plug in/out. With this change, it
> will align with DP83822 driver API.

So why is there the comment:

>     >                                            and is
>     > acceptable to do this if a link is already present.

That kind of says, it is not acceptable to do this if the link is not
present. Which is why i'm asking.

	 Andrew

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Modi, Geet]

Hi Andrew,

If Link is present, the current reset will reset the registers including the link and Host will need to re-program all the registers. With the change proposed, it will reset the state machine while keeping the register content.

My understanding is that below API is called to perform the soft reset and not complete wipe of PHY. Do you agree ?


.soft_reset	= dp83867_phy_reset,


Regards,
Geet



See below difference between the two reset from DP83867 datasheet.

8.5.5.3 Global Software Reset
A global software reset is accomplished by setting bit 15 of register CTRL (address 0x001F) to 1. This bit resets
all the internal circuits in the PHY including IEEE-defined registers and all the extended registers. The global
software reset resets the device such that all registers are reset to default values and the hardware configuration
values are maintained.
8.5.5.4 Global Software Restart
A global software restart is accomplished by setting bit 14 of register CTRL (0x001F) to 1. This action resets all
the PHY circuits except the registers in the Register File.




In DP83822 linux, the API is calling the software reset and we are aligning DP83867 implementaiton with DP83822.


Regards,
Geet



On 3/31/21, 9:35 AM, "Andrew Lunn" <andrew@lunn.ch> wrote:

    >     > as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
    > 
    >     > 8.6.26 Control Register (CTRL)
    >     > do SW_RESTART to perform a reset not including the registers and is
    >     > acceptable to do this if a link is already present.
    > 
    >  
    > 
    >     I don't see any code here to determine if the like is present. What if
    >     the cable is not plugged in?
    > 
    >     This API is primarily used for reset. Link Status is checked thru different
    > register. This shall not impact the cable plug in/out. With this change, it
    > will align with DP83822 driver API.

    So why is there the comment:

    >     >                                            and is
    >     > acceptable to do this if a link is already present.

    That kind of says, it is not acceptable to do this if the link is not
    present. Which is why i'm asking.

    	 Andrew

Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Bajjuri, Praneeth]

Hi Andrew,

On 3/31/2021 11:35 AM, Andrew Lunn wrote:
>>      > as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
>>
>>      > 8.6.26 Control Register (CTRL)
>>      > do SW_RESTART to perform a reset not including the registers and is
>>      > acceptable to do this if a link is already present.
>>
>>   
>>
>>      I don't see any code here to determine if the like is present. What if
>>      the cable is not plugged in?
>>
>>      This API is primarily used for reset. Link Status is checked thru different
>> register. This shall not impact the cable plug in/out. With this change, it
>> will align with DP83822 driver API.
> 
> So why is there the comment:
> 
>>      >                                            and is
>>      > acceptable to do this if a link is already present.
> 
> That kind of says, it is not acceptable to do this if the link is not
> present. Which is why i'm asking.

Does the feedback from Geet help in clarity you requested.
Ref:
https://lore.kernel.org/netdev/4838EA12-7BF4-4FF2-8305-7446C3498DDF@ti.com/

> 
> 	 Andrew
> 

Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Andrew Lunn]

On Fri, May 28, 2021 at 11:32:15AM -0500, Bajjuri, Praneeth wrote:
> Hi Andrew,
> 
> On 3/31/2021 11:35 AM, Andrew Lunn wrote:
> > >      > as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> > > 
> > >      > 8.6.26 Control Register (CTRL)
> > >      > do SW_RESTART to perform a reset not including the registers and is
> > >      > acceptable to do this if a link is already present.
> > > 
> > > 
> > >      I don't see any code here to determine if the like is present. What if
> > >      the cable is not plugged in?
> > > 
> > >      This API is primarily used for reset. Link Status is checked thru different
> > > register. This shall not impact the cable plug in/out. With this change, it
> > > will align with DP83822 driver API.
> > 
> > So why is there the comment:
> > 
> > >      >                                            and is
> > >      > acceptable to do this if a link is already present.
> > 
> > That kind of says, it is not acceptable to do this if the link is not
> > present. Which is why i'm asking.
> 
> Does the feedback from Geet help in clarity you requested.
> Ref:
> https://lore.kernel.org/netdev/4838EA12-7BF4-4FF2-8305-7446C3498DDF@ti.com/

Not really.

>                                                        and is
> > >      > acceptable to do this if a link is already present.

There needs to be something to either:

1) Ensure there is link, so we known we are within acceptable
behaviour.

2) Document what happens when there is no link, meaning we do
something which is not acceptable. Is the magic smoke going to be
released? Does the link die until the next reboot? Or despite it being
unacceptable, nothing really happens, and it is not a problem?

	      Andrew

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Andrew Lunn]

On Tue, Jun 01, 2021 at 07:01:04PM +0000, Modi, Geet wrote:
> Hello Andrew,
> 
>  
> 
> Please let me know if you have additional questions/clarifications to approve
> below change request.
> 
>  
> 
> Regards,
> Geet
> 
>  
> 
>  
> 
> From: Geet Modi <geet.modi@ti.com>
> Date: Friday, May 28, 2021 at 10:10 AM
> To: Andrew Lunn <andrew@lunn.ch>, "Bajjuri, Praneeth" <praneeth@ti.com>
> Cc: "David S . Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
> "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
> "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
> Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform
> soft reset and retain established link

So this all seems to boil down to, it does not matter if it is
acceptable or not, you are going to do it. So please just remove that
part of the comment. It has no value.

	 Andrew

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Modi, Geet]

Praneeth,

Can you please help edit the comment and resubmit for approval ?

Regards,
Geet


On 6/1/21, 12:18 PM, "Andrew Lunn" <andrew@lunn.ch> wrote:

    On Tue, Jun 01, 2021 at 07:01:04PM +0000, Modi, Geet wrote:
    > Hello Andrew,
    > 
    >  
    > 
    > Please let me know if you have additional questions/clarifications to approve
    > below change request.
    > 
    >  
    > 
    > Regards,
    > Geet
    > 
    >  
    > 
    >  
    > 
    > From: Geet Modi <geet.modi@ti.com>
    > Date: Friday, May 28, 2021 at 10:10 AM
    > To: Andrew Lunn <andrew@lunn.ch>, "Bajjuri, Praneeth" <praneeth@ti.com>
    > Cc: "David S . Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
    > "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
    > "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
    > Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform
    > soft reset and retain established link

    So this all seems to boil down to, it does not matter if it is
    acceptable or not, you are going to do it. So please just remove that
    part of the comment. It has no value.

    	 Andrew

[PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[praneeth]

From: Praneeth Bajjuri <praneeth@ti.com>

Current logic is performing hard reset and causing the programmed
registers to be wiped out.

as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
8.6.26 Control Register (CTRL)

do SW_RESTART to perform a reset not including the registers,
If performed when link is already present,
it will drop the link and trigger re-auto negotiation.

Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
Signed-off-by: Geet Modi <geet.modi@ti.com>
---
 drivers/net/phy/dp83867.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 9bd9a5c0b1db..6bbc81ad295f 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -826,16 +826,12 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 {
 	int err;
 
-	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
+	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
 	if (err < 0)
 		return err;
 
 	usleep_range(10, 20);
 
-	/* After reset FORCE_LINK_GOOD bit is set. Although the
-	 * default value should be unset. Disable FORCE_LINK_GOOD
-	 * for the phy to work properly.
-	 */
 	return phy_modify(phydev, MII_DP83867_PHYCTRL,
 			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
 }
-- 
2.17.1

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform soft reset and retain established link

[Bajjuri, Praneeth]

Andrew,

On 6/1/2021 2:18 PM, Andrew Lunn wrote:
> On Tue, Jun 01, 2021 at 07:01:04PM +0000, Modi, Geet wrote:
>> Hello Andrew,
>>
>>   
>>
>> Please let me know if you have additional questions/clarifications to approve
>> below change request.
>>
>>   
>>
>> Regards,
>> Geet
>>
>>   
>>
>>   
>>
>> From: Geet Modi <geet.modi@ti.com>
>> Date: Friday, May 28, 2021 at 10:10 AM
>> To: Andrew Lunn <andrew@lunn.ch>, "Bajjuri, Praneeth" <praneeth@ti.com>
>> Cc: "David S . Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
>> "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
>> "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
>> Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH] net: phy: dp83867: perform
>> soft reset and retain established link
> 
> So this all seems to boil down to, it does not matter if it is
> acceptable or not, you are going to do it. So please just remove that
> part of the comment. It has no value.

Sent v2 addressing comment as per your suggestion.
Ref: https://lore.kernel.org/patchwork/patch/1444281/

Thanks
Praneeth

> 
> 	 Andrew
> 

Re: [PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[Andrew Lunn]

On Wed, Jun 09, 2021 at 07:43:42PM -0500, praneeth@ti.com wrote:
> From: Praneeth Bajjuri <praneeth@ti.com>
> 
> Current logic is performing hard reset and causing the programmed
> registers to be wiped out.
> 
> as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> 8.6.26 Control Register (CTRL)
> 
> do SW_RESTART to perform a reset not including the registers,
> If performed when link is already present,
> it will drop the link and trigger re-auto negotiation.
> 
> Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
> Signed-off-by: Geet Modi <geet.modi@ti.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[Johannes Pointner]

Hello,

On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jun 09, 2021 at 07:43:42PM -0500, praneeth@ti.com wrote:
> > From: Praneeth Bajjuri <praneeth@ti.com>
> >
> > Current logic is performing hard reset and causing the programmed
> > registers to be wiped out.
> >
> > as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> > 8.6.26 Control Register (CTRL)
> >
> > do SW_RESTART to perform a reset not including the registers,
> > If performed when link is already present,
> > it will drop the link and trigger re-auto negotiation.
> >
> > Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
> > Signed-off-by: Geet Modi <geet.modi@ti.com>
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
>     Andrew

I reported a few days ago an issue with the DP83822 which I think is
caused by a similar change.
https://lore.kernel.org/netdev/CAHvQdo2yzJC89K74c_CZFjPydDQ5i22w36XPR5tKVv_W8a2vcg@mail.gmail.com/
In my case I can't get an link after this change, reverting it fixes
the problem for me.

Hannes

Re: [PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[Bajjuri, Praneeth]

Hannes,

On 6/10/2021 12:53 AM, Johannes Pointner wrote:
> Hello,
> 
> On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Wed, Jun 09, 2021 at 07:43:42PM -0500, praneeth@ti.com wrote:
>>> From: Praneeth Bajjuri <praneeth@ti.com>
>>>
>>> Current logic is performing hard reset and causing the programmed
>>> registers to be wiped out.
>>>
>>> as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
>>> 8.6.26 Control Register (CTRL)
>>>
>>> do SW_RESTART to perform a reset not including the registers,
>>> If performed when link is already present,
>>> it will drop the link and trigger re-auto negotiation.
>>>
>>> Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
>>> Signed-off-by: Geet Modi <geet.modi@ti.com>
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>
>>      Andrew
> 
> I reported a few days ago an issue with the DP83822 which I think is
> caused by a similar change.
> https://lore.kernel.org/netdev/CAHvQdo2yzJC89K74c_CZFjPydDQ5i22w36XPR5tKVv_W8a2vcg@mail.gmail.com/
> In my case I can't get an link after this change, reverting it fixes
> the problem for me.

Are you saying that instead of reset if sw_restart is done as per this 
patch, there is no issue?

> 
> Hannes
> 

Re: [PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Wed, 9 Jun 2021 19:43:42 -0500 you wrote:
> From: Praneeth Bajjuri <praneeth@ti.com>
> 
> Current logic is performing hard reset and causing the programmed
> registers to be wiped out.
> 
> as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> 8.6.26 Control Register (CTRL)
> 
> [...]

Here is the summary with links:
  - [v2] net: phy: dp83867: perform soft reset and retain established link
    https://git.kernel.org/netdev/net/c/da9ef50f545f

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2] net: phy: dp83867: perform soft reset and retain established link

[Johannes Pointner]

Hello Praneeth,

On Fri, Jun 11, 2021 at 7:05 PM Bajjuri, Praneeth <praneeth@ti.com> wrote:
>
> Hannes,
>
> On 6/10/2021 12:53 AM, Johannes Pointner wrote:
> > Hello,
> >
> > On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> On Wed, Jun 09, 2021 at 07:43:42PM -0500, praneeth@ti.com wrote:
> >>> From: Praneeth Bajjuri <praneeth@ti.com>
> >>>
> >>> Current logic is performing hard reset and causing the programmed
> >>> registers to be wiped out.
> >>>
> >>> as per datasheet: https://www.ti.com/lit/ds/symlink/dp83867cr.pdf
> >>> 8.6.26 Control Register (CTRL)
> >>>
> >>> do SW_RESTART to perform a reset not including the registers,
> >>> If performed when link is already present,
> >>> it will drop the link and trigger re-auto negotiation.
> >>>
> >>> Signed-off-by: Praneeth Bajjuri <praneeth@ti.com>
> >>> Signed-off-by: Geet Modi <geet.modi@ti.com>
> >>
> >> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >>
> >>      Andrew
> >
> > I reported a few days ago an issue with the DP83822 which I think is
> > caused by a similar change.
> > https://lore.kernel.org/netdev/CAHvQdo2yzJC89K74c_CZFjPydDQ5i22w36XPR5tKVv_W8a2vcg@mail.gmail.com/
> > In my case I can't get an link after this change, reverting it fixes
> > the problem for me.
>
> Are you saying that instead of reset if sw_restart is done as per this
> patch, there is no issue?
In my case(DP83822 connected to an i.MX6) if the digital(SW) restart
is used (Bit 14) I have the issue that I can' get a link.
ip addr shows:
1: lo: <LOOPBACK> mtu 65536 qdisc noop qlen 1000
   link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast qlen 1000
   link/ether 00:60:65:54:32:10 brd ff:ff:ff:ff:ff:ff

If I revert this back to using the SW reset (Bit 15) it works again.

Regards,
Hannes