diff mbox series

net: phy: dp83867: perform soft reset and retain established link

Message ID 20210324010006.32576-1-praneeth@ti.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: phy: dp83867: perform soft reset and retain established link | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 3 maintainers not CCed: hkallweit1@gmail.com linux@armlinux.org.uk andrew@lunn.ch
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Bajjuri, Praneeth March 24, 2021, 1 a.m. UTC 
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(-)

Comments

Andrew Lunn March 24, 2021, 12:32 p.m. UTC | #1 
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
Andrew Lunn March 31, 2021, 4:35 p.m. UTC | #2 
>     > 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
Modi, Geet March 31, 2021, 8:36 p.m. UTC | #3 
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
Bajjuri, Praneeth May 28, 2021, 4:32 p.m. UTC | #4 
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
>
Andrew Lunn May 28, 2021, 4:52 p.m. UTC | #5 
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
Andrew Lunn June 1, 2021, 7:18 p.m. UTC | #6 
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
Modi, Geet June 1, 2021, 11:52 p.m. UTC | #7 
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
Bajjuri, Praneeth June 10, 2021, 12:47 a.m. UTC | #8 
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
>
diff mbox series

Patch

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;