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

Commit Message

Bajjuri, Praneeth June 10, 2021, 12:43 a.m. UTC

From: Praneeth Bajjuri <[e-mail]>

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 <[e-mail]>
Signed-off-by: Geet Modi <[e-mail]>
---
 drivers/net/phy/dp83867.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Andrew Lunn June 10, 2021, 4:07 a.m. UTC | #1

On Wed, Jun 09, 2021 at 07:43:42PM -0500, [e-mail] wrote:
> From: Praneeth Bajjuri <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Geet Modi <[e-mail]>

Reviewed-by: Andrew Lunn <[e-mail]>

    Andrew

Johannes Pointner June 10, 2021, 5:53 a.m. UTC | #2

Hello,

On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <[e-mail]> wrote:
>
> On Wed, Jun 09, 2021 at 07:43:42PM -0500, [e-mail] wrote:
> > From: Praneeth Bajjuri <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Geet Modi <[e-mail]>
>
> Reviewed-by: Andrew Lunn <[e-mail]>
>
>     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/[e-mail]/
In my case I can't get an link after this change, reverting it fixes
the problem for me.

Hannes

Bajjuri, Praneeth June 11, 2021, 5:05 p.m. UTC | #3

Hannes,

On 6/10/2021 12:53 AM, Johannes Pointner wrote:
> Hello,
> 
> On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <[e-mail]> wrote:
>>
>> On Wed, Jun 09, 2021 at 07:43:42PM -0500, [e-mail] wrote:
>>> From: Praneeth Bajjuri <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Geet Modi <[e-mail]>
>>
>> Reviewed-by: Andrew Lunn <[e-mail]>
>>
>>      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/[e-mail]/
> 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
>

[e-mail] June 11, 2021, 5:20 p.m. UTC | #4

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 <[e-mail]>
> 
> 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

Johannes Pointner June 14, 2021, 8:17 a.m. UTC | #5

Hello Praneeth,

On Fri, Jun 11, 2021 at 7:05 PM Bajjuri, Praneeth <[e-mail]> wrote:
>
> Hannes,
>
> On 6/10/2021 12:53 AM, Johannes Pointner wrote:
> > Hello,
> >
> > On Thu, Jun 10, 2021 at 6:10 AM Andrew Lunn <[e-mail]> wrote:
> >>
> >> On Wed, Jun 09, 2021 at 07:43:42PM -0500, [e-mail] wrote:
> >>> From: Praneeth Bajjuri <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Signed-off-by: Geet Modi <[e-mail]>
> >>
> >> Reviewed-by: Andrew Lunn <[e-mail]>
> >>
> >>      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/[e-mail]/
> > 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

Patch

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);
 }