[v1] net: phy: dp83822: Fix reset pin definitions

net: phy: dp83822: Fix reset pin definitions

Posted by Michel Alex 1 month, 1 week ago

The SW_RESET definition was incorrectly assigned to bit 14, which is the
Digital Restart bit according to the datasheet. This commit corrects
SW_RESET to bit 15 and assigns DIG_RESTART to bit 14 as per the
datasheet specifications.

The SW_RESET define is only used in the phy_reset function, which fully
re-initializes the PHY after the reset is performed. The change in the
bit definitions should not have any negative impact on the functionality
of the PHY.

Cc: mailto:stable@vger.kernel.org
Signed-off-by: Alex Michel <mailto:alex.michel@wiedemann-group.com>
---
 drivers/net/phy/dp83822.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index fc247f479257..3ab64e04a01c 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -45,8 +45,8 @@
 /* Control Register 2 bits */
 #define DP83822_FX_ENABLE	BIT(14)
 
-#define DP83822_HW_RESET	BIT(15)
-#define DP83822_SW_RESET	BIT(14)
+#define DP83822_SW_RESET	BIT(15)
+#define DP83822_DIG_RESTART	BIT(14)
 
 /* PHY STS bits */
 #define DP83822_PHYSTS_DUPLEX			BIT(2)
-- 
2.43.0

Re: net: phy: dp83822: Fix reset pin definitions

Posted by Maxime Chevallier 1 month, 1 week ago

Hi,

On Wed, 16 Oct 2024 09:56:34 +0000
Michel Alex <Alex.Michel@wiedemann-group.com> wrote:

> The SW_RESET definition was incorrectly assigned to bit 14, which is the
> Digital Restart bit according to the datasheet. This commit corrects
> SW_RESET to bit 15 and assigns DIG_RESTART to bit 14 as per the
> datasheet specifications.
> 
> The SW_RESET define is only used in the phy_reset function, which fully
> re-initializes the PHY after the reset is performed. The change in the
> bit definitions should not have any negative impact on the functionality
> of the PHY.
> 
> Cc: mailto:stable@vger.kernel.org
> Signed-off-by: Alex Michel <mailto:alex.michel@wiedemann-group.com>

Thanks for the patch ! When submitting a patch for inclusion through
the net subsystem, you need to format your patch so that you indicate
whether the patch is aimed towards net-next (new features) or net
(bugfixes). More information can be found here :

https://www.kernel.org/doc/Documentation/process/maintainer-netdev.rst

You can use the --subject-prefix="PATCH net" option to git format-patch
when generating the patch.

It seems to me that this is indeed a bug, which has been reported
before :

https://lore.kernel.org/netdev/CAHvQdo2yzJC89K74c_CZFjPydDQ5i22w36XPR5tKVv_W8a2vcg@mail.gmail.com/

You would therefore need a Fixes tag pinpointing the commit that
introduced the issue :

Fixes: 5dc39fd5ef35 ("net: phy: DP83822: Add ability to advertise Fiber connection")

I don't have a board with that PHY to test it, it seems that issue of
the wrong bit being set during reset was introduced when Fiber support
for this PHY was added, it's unclear if the change was on purpose or
not and if changing this would break the boards that relies on straps
to detect that they are using Fiber :/

Best regards,

Maxime

[PATCH net v2] net: phy: dp83822: Fix reset pin definitions

Posted by Michel Alex 1 month, 1 week ago

This change fixes a rare issue where the PHY fails to detect a link
due to incorrect reset behavior.

The SW_RESET definition was incorrectly assigned to bit 14, which is the
Digital Restart bit according to the datasheet. This commit corrects
SW_RESET to bit 15 and assigns DIG_RESTART to bit 14 as per the
datasheet specifications.

The SW_RESET define is only used in the phy_reset function, which fully
re-initializes the PHY after the reset is performed. The change in the
bit definitions should not have any negative impact on the functionality
of the PHY.

v2:
- added Fixes tag
- improved commit message

Cc: stable@vger.kernel.org
Fixes: 5dc39fd5ef35 ("net: phy: DP83822: Add ability to advertise Fiber connection")
Signed-off-by: Alex Michel <alex.michel@wiedemann-group.com>
---
 drivers/net/phy/dp83822.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index fc247f479257..3ab64e04a01c 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -45,8 +45,8 @@
 /* Control Register 2 bits */
 #define DP83822_FX_ENABLE	BIT(14)
 
-#define DP83822_HW_RESET	BIT(15)
-#define DP83822_SW_RESET	BIT(14)
+#define DP83822_SW_RESET	BIT(15)
+#define DP83822_DIG_RESTART	BIT(14)
 
 /* PHY STS bits */
 #define DP83822_PHYSTS_DUPLEX			BIT(2)
-- 
2.43.0

Re: [PATCH net v2] net: phy: dp83822: Fix reset pin definitions

Posted by Andrew Lunn 1 month, 1 week ago

On Wed, Oct 16, 2024 at 12:11:15PM +0000, Michel Alex wrote:
> This change fixes a rare issue where the PHY fails to detect a link
> due to incorrect reset behavior.
> 
> The SW_RESET definition was incorrectly assigned to bit 14, which is the
> Digital Restart bit according to the datasheet. This commit corrects
> SW_RESET to bit 15 and assigns DIG_RESTART to bit 14 as per the
> datasheet specifications.
> 
> The SW_RESET define is only used in the phy_reset function, which fully
> re-initializes the PHY after the reset is performed. The change in the
> bit definitions should not have any negative impact on the functionality
> of the PHY.
> 
> v2:
> - added Fixes tag
> - improved commit message

> 
> Cc: stable@vger.kernel.org
> Fixes: 5dc39fd5ef35 ("net: phy: DP83822: Add ability to advertise Fiber connection")
> Signed-off-by: Alex Michel <alex.michel@wiedemann-group.com>

Please create a new thread for each new patch submission. The
machinary testing patches does not understand it when you just add a
new version to an existing thread.


    Andrew

---
pw-bot: cr

Re: [PATCH net v2] net: phy: dp83822: Fix reset pin definitions

Posted by Andrew Lunn 1 month, 1 week ago

On Wed, Oct 16, 2024 at 12:11:15PM +0000, Michel Alex wrote:
> This change fixes a rare issue where the PHY fails to detect a link
> due to incorrect reset behavior.
> 
> The SW_RESET definition was incorrectly assigned to bit 14, which is the
> Digital Restart bit according to the datasheet. This commit corrects
> SW_RESET to bit 15 and assigns DIG_RESTART to bit 14 as per the
> datasheet specifications.
> 
> The SW_RESET define is only used in the phy_reset function, which fully
> re-initializes the PHY after the reset is performed. The change in the
> bit definitions should not have any negative impact on the functionality
> of the PHY.
> 
> v2:
> - added Fixes tag
> - improved commit message
> 
> Cc: stable@vger.kernel.org
> Fixes: 5dc39fd5ef35 ("net: phy: DP83822: Add ability to advertise Fiber connection")
> Signed-off-by: Alex Michel <alex.michel@wiedemann-group.com>

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

    Andrew