[PATCH net] net: stmmac: dwmac4: fix PCS duplex mode decode

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

Thanks to Abhishek Chauhan for providing a response on this issue.

Signed-off-by: Russell King (Oracle) <redacted>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index b25774d69195..26d837554a2d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c

@@ -791,7 +791,7 @@ static void dwmac4_phystatus(void __iomem *ioaddr, struct stmmac_extra_stats *x)
 		else
 			x->pcs_speed = SPEED_10;
 
-		x->pcs_duplex = (status & GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK);
+		x->pcs_duplex = (status & GMAC_PHYIF_CTRLSTATUS_LNKMOD);
 
 		pr_info("Link is Up - %d/%s\n", (int)x->pcs_speed,
 			x->pcs_duplex ? "Full" : "Half");

-- 
2.30.2

On Wed, Jul 03, 2024 at 01:24:40PM +0100, Russell King (Oracle) wrote:

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

This appears to be the only use of
GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK. Maybe it should be removed after
this change?

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

    Andrew

On Wed, Jul 03, 2024 at 01:24:40PM +0100, Russell King (Oracle) wrote:

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

Thanks to Abhishek Chauhan for providing a response on this issue.

Signed-off-by: Russell King (Oracle) <redacted>

Right. The PHYIF_CTRLSTATUS CSR has the link status bits defined
starting from 16-bit position. So the method has improperly detected
duplex mode. Bit zero is occupied with the TC-flag (transmit RGMII,
SMII, SGMII configs) which is never set and which means the method
always reported the half-duplex link mode.

Reviewed-by: Serge Semin <redacted>

-Serge(y)

On Wed, Jul 03, 2024 at 01:24:40PM GMT, Russell King (Oracle) wrote:

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

Thanks to Abhishek Chauhan for providing a response on this issue.

Signed-off-by: Russell King (Oracle) <redacted>

Fixes: 70523e639bf8c ("drivers: net: stmmac: reworking the PCS code.")
Reviewed-by: Andrew Halaney <redacted>

Since GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK is confusing and unused I think
removing it altogether would be a good call.

Thanks,
Andrew

On Wed, Jul 03, 2024 at 04:06:54PM +0200, Andrew Lunn wrote:

On Wed, Jul 03, 2024 at 01:24:40PM +0100, Russell King (Oracle) wrote:

quoted

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

This appears to be the only use of
GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK. Maybe it should be removed after
this change?

There is a PCS-refactoring work initiated by Russell, which besides of
other things may eventually imply dropping this macro:
https://lore.kernel.org/netdev/20240624132802.14238-6-fancer.lancer@gmail.com/
So unless it is strongly required I guess there is no much need in
respinning this patch or implementing additional one for now.

-Serge(y)

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

    Andrew

On Wed, Jul 03, 2024 at 06:07:54PM +0300, Serge Semin wrote:

On Wed, Jul 03, 2024 at 04:06:54PM +0200, Andrew Lunn wrote:

quoted

On Wed, Jul 03, 2024 at 01:24:40PM +0100, Russell King (Oracle) wrote:

quoted

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

This appears to be the only use of
GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK. Maybe it should be removed after
this change?

There is a PCS-refactoring work initiated by Russell, which besides of
other things may eventually imply dropping this macro:
https://lore.kernel.org/netdev/20240624132802.14238-6-fancer.lancer@gmail.com/
So unless it is strongly required I guess there is no much need in
respinning this patch or implementing additional one for now.

Nevertheless, a respin is worth doing with Andrew's suggested change
because this patch will impact the refactoring work even without that
change. We might as well have a complete patch for this change.

(Besides, I've already incorporated Andrew's feedback!)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Wed, Jul 03, 2024 at 04:29:24PM +0100, Russell King (Oracle) wrote:

On Wed, Jul 03, 2024 at 06:07:54PM +0300, Serge Semin wrote:

quoted

On Wed, Jul 03, 2024 at 04:06:54PM +0200, Andrew Lunn wrote:

quoted

On Wed, Jul 03, 2024 at 01:24:40PM +0100, Russell King (Oracle) wrote:

quoted

dwmac4 was decoding the duplex mode from the GMAC_PHYIF_CONTROL_STATUS
register incorrectly, using GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1)
rather than GMAC_PHYIF_CTRLSTATUS_LNKMOD (bit 16). Fix this.

This appears to be the only use of
GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK. Maybe it should be removed after
this change?

There is a PCS-refactoring work initiated by Russell, which besides of
other things may eventually imply dropping this macro:
https://lore.kernel.org/netdev/20240624132802.14238-6-fancer.lancer@gmail.com/
So unless it is strongly required I guess there is no much need in
respinning this patch or implementing additional one for now.

Nevertheless, a respin is worth doing with Andrew's suggested change
because this patch will impact the refactoring work even without that
change. We might as well have a complete patch for this change.

(Besides, I've already incorporated Andrew's feedback!)

Ok. I just noted that that the respinning wasn't required due to the
same change implied by another patchset. But since you have already
implemented the change to make the patch more complete, then it's even
better than waiting for our STMMAC PCS discussions over.

-Serge(y)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On 7/3/2024 5:24 AM, Russell King (Oracle) wrote:

Thanks Russell for taking care of this change. 

Can you please add my Reviewed-by: Abhishek Chauhan [off-list ref] on the latest series. 

Thanks 
ABC