| Message ID | E1sOz2O-00Gm9W-B7@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: stmmac: dwmac4: fix PCS duplex mode decode | expand |
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) <rmk+kernel@armlinux.org.uk> Fixes: 70523e639bf8c ("drivers: net: stmmac: reworking the PCS code.") Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > --- > 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); 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 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) <rmk+kernel@armlinux.org.uk> 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 <fancer.lancer@gmail.com> -Serge(y) > --- > 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 04:06:54PM +0200, Andrew Lunn wrote: > 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? 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: > > 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? > > 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!)
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: > > 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: > > > > 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: > 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) <rmk+kernel@armlinux.org.uk> > --- > 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"); Thanks Russell for taking care of this change. Can you please add my Reviewed-by: Abhishek Chauhan <quic_abchauha@quicinc.com> on the latest series. Thanks ABC
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");
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) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)