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