Re: [WARNING: ATTACHMENT UNSCANNED]Re: [PATCH RFC net-next 1/2] net: pcs: xpcs: Add special code to operate in Microchip KSZ9477 switch
From: Russell King (Oracle)
Date: Fri Jan 31 2025 - 10:50:13 EST
On Fri, Jan 31, 2025 at 02:36:49PM +0000, Jose Abreu wrote:
> From: Russell King (Oracle) <linux@xxxxxxxxxxxxxxx>
> Date: Thu, Jan 30, 2025 at 11:02:00
>
> > Would it be safe to set these two bits with newer XPCS hardware when
> > programming it for 1000base-X mode, even though documentation e.g.
> > for SJA1105 suggests that these bits do not apply when operating in
> > 1000base-X mode?
>
> It's hard to provide a clear answer because our products can all be modified
> by final customer. I hope this snippet below can help:
>
> "Nothing has changed in "AN control register" ever since at least for a decade.
> Having said that, bit[4] and bit[3] are valid for SGMII mode and not valid
> for 1000BASE-X mode (I don't know why customer says 'serdes' mode.
> There is no such mode in ethernet standard). So, customer shall
> leave this bits at default value of 0. Even if they set to 1, there is no
> impact (as those bits are not used in 1000BASE-X mode)."
Thanks for the reply Jose, that's useful.
Tristram, I think you need to talk to your hardware people to find out
where this requirement to set these two bits comes from as it seems it
isn't a property that comes from Synopsys' IP (I suppose unless your
IP is older than ten years.)
That said, Jose's response indicates that we can set these two bits
with impunity provided another of Synopsys's customers hasn't modified
their integration of XPCS to require these bits to be set to zero. So,
while I think we can do that unconditionally (as per the patch
attached) I think we need a clearer comment to state why it's being
done (and I probably need to now modify the commit message - this was
created before Jose's reply.)
So, I think given the last two patches I've sent, I believe I've
covered both of the issues that you have with XPCS:
1) the need to set bits 4 and 3 in AN control for 1000base-X in KSZ9477
(subject to a better commit message and code comment, which will be
dependent on your research as to where this requirement has come
from.)
2) the lack of MAC_AUTO_SW support in KSZ9477 which can be enabled by
writing DW_XPCS_SGMII_MODE_MAC_MANUAL to xpcs->sgmii_mode.
We now need to work out a way to identify this older IP. I think for
(2) we could potentially do something like (error handling omitted for
clarity):
if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_AUTO) {
xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL,
DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW,
DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW);
ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL);
/* If MAC_AUTO_SW doesn't retain the MAC_AUTO_SW bit, then the
* XPCS implementation does not support this feature, and we
* have to manually configure the BMCR for the link parameters.
*/
if (ret >= 0 && !(ret & DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW))
xpcs->sgmii_mode = DW_XPCS_SGMII_MODE_MAC_MANUAL;
}
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
From: "Russell King (Oracle)" <rmk+kernel@xxxxxxxxxxxxxxx>
Subject: [PATCH net-next] net: xpcs: allow 1000BASE-X to work with older XPCS
IP
Older XPCS IP requires SGMII_LINK and PHY_SIDE_SGMII to be set when
operating in 1000BASE-X mode even though the XPCS is not configured for
SGMII. An example of a device with older XPCS IP is KSZ9477.
We already don't clear these bits if we switch from SGMII to 1000BASE-X
on TXGBE - which would result in 1000BASE-X with the PHY_SIDE_SGMII bit
left set.
It is currently believed to be safe to set both bits on newer IP
without side-effects.
Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
---
drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++--
drivers/net/pcs/pcs-xpcs.h | 1 +
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 29269d5c5f3a..cbd2bdd0fd55 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -745,9 +745,18 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
return ret;
}
- mask = DW_VR_MII_PCS_MODE_MASK;
+ /* Older XPCS IP requires PHY_MODE (bit 3) and SGMII_LINK (but 4) to
+ * be set when operating in 1000BASE-X mode. See page 233
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf
+ * "5.5.9 SGMII AUTO-NEGOTIATION CONTROL REGISTER"
+ */
+ mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_AN_CTRL_SGMII_LINK |
+ DW_VR_MII_TX_CONFIG_MASK;
val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
- DW_VR_MII_PCS_MODE_C37_1000BASEX);
+ DW_VR_MII_PCS_MODE_C37_1000BASEX) |
+ FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
+ DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
+ DW_VR_MII_AN_CTRL_SGMII_LINK;
if (!xpcs->pcs.poll) {
mask |= DW_VR_MII_AN_INTR_EN;
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index be8fab40b012..082933d1d485 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -61,6 +61,7 @@
#define DW_VR_MII_AN_CTRL 0x8001
#define DW_VR_MII_AN_CTRL_8BIT BIT(8)
+#define DW_VR_MII_AN_CTRL_SGMII_LINK BIT(4
#define DW_VR_MII_TX_CONFIG_MASK BIT(3)
#define DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII 0x1
#define DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII 0x0
--
2.30.2