| Message ID | E1tffRT-003Z5u-CY@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: xpcs: cleanups and partial support for KSZ9477 | expand |
> -----Original Message----- > From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle) > Sent: Wednesday, February 5, 2025 5:28 AM > To: Tristram Ha - C24268 <Tristram.Ha@microchip.com> > Cc: Vladimir Oltean <olteanv@gmail.com>; UNGLinuxDriver > <UNGLinuxDriver@microchip.com>; Woojung Huh - C21699 > <Woojung.Huh@microchip.com>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit > <hkallweit1@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; netdev@vger.kernel.org > Subject: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older > XPCS IP > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content > is safe > > 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@armlinux.org.uk> > --- > 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 1eba0c583f16..d522e4a5a138 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -774,9 +774,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/ProductDo > cuments/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 96117bd9e2b6..f0ddd93c7a22 100644 > --- a/drivers/net/pcs/pcs-xpcs.h > +++ b/drivers/net/pcs/pcs-xpcs.h > @@ -73,6 +73,7 @@ > > /* VR_MII_AN_CTRL */ > #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 > -- Tested-by: Tristram Ha <tristram.ha@microchip.com>
On Wed, Feb 05, 2025 at 01:27:47PM +0000, Russell King (Oracle) wrote: > 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. Is there a confirmation written down somewhere that a transition from SGMII to 1000Base-X was explicitly tested? I have to remain a bit skeptical and say that although the code is indeed like this, it doesn't mean by itself there are no unintended side effects. > 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@armlinux.org.uk> > --- > 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 1eba0c583f16..d522e4a5a138 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -774,9 +774,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 ~~~ bit > + * 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; I do believe that this is the kind of patch one would write when the hardware is completely a black box. But when we have Microchip engineers here with a channel open towards their hardware design who can help clarify where the requirement comes from, that just isn't the case. So I wouldn't rush with this. Plus, it isn't even the most conservative way in which a (supposedly) integration-specific requirement is fulfilled in the common Synopsys driver. If one integration makes vendor-specific choices about these bits, I wouldn't assume that no other vendors made contradictory choices. I don't want to say too much before Tristram comes with a statement from Microchip hardware design, but _if_ it turns out to be a KSZ9477 specific requirement, it still seems safer to only enable this based (at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his other patch set, if not based on something stronger (a conditional describing some functional behavior, rather than a specific hardware IP).
On Mon, Feb 10, 2025 at 01:05:55PM +0200, Vladimir Oltean wrote: > On Wed, Feb 05, 2025 at 01:27:47PM +0000, Russell King (Oracle) wrote: > > 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. > > Is there a confirmation written down somewhere that a transition from > SGMII to 1000Base-X was explicitly tested? I have to remain a bit > skeptical and say that although the code is indeed like this, it > doesn't mean by itself there are no unintended side effects. > > > 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@armlinux.org.uk> > > --- > > 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 1eba0c583f16..d522e4a5a138 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -774,9 +774,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 > ~~~ > bit > > > + * 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; > > I do believe that this is the kind of patch one would write when the > hardware is completely a black box. But when we have Microchip engineers > here with a channel open towards their hardware design who can help > clarify where the requirement comes from, that just isn't the case. > So I wouldn't rush with this. > > Plus, it isn't even the most conservative way in which a (supposedly) > integration-specific requirement is fulfilled in the common Synopsys > driver. If one integration makes vendor-specific choices about these > bits, I wouldn't assume that no other vendors made contradictory choices. > > I don't want to say too much before Tristram comes with a statement from > Microchip hardware design, but _if_ it turns out to be a KSZ9477 > specific requirement, it still seems safer to only enable this based > (at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his > other patch set, if not based on something stronger (a conditional > describing some functional behavior, rather than a specific hardware IP). So Jose's public reassurance means nothing?
On Mon, Feb 10, 2025 at 11:49:21AM +0000, Russell King (Oracle) wrote: > On Mon, Feb 10, 2025 at 01:05:55PM +0200, Vladimir Oltean wrote: > > I do believe that this is the kind of patch one would write when the > > hardware is completely a black box. But when we have Microchip engineers > > here with a channel open towards their hardware design who can help > > clarify where the requirement comes from, that just isn't the case. > > So I wouldn't rush with this. > > > > Plus, it isn't even the most conservative way in which a (supposedly) > > integration-specific requirement is fulfilled in the common Synopsys > > driver. If one integration makes vendor-specific choices about these > > bits, I wouldn't assume that no other vendors made contradictory choices. > > > > I don't want to say too much before Tristram comes with a statement from > > Microchip hardware design, but _if_ it turns out to be a KSZ9477 > > specific requirement, it still seems safer to only enable this based > > (at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his > > other patch set, if not based on something stronger (a conditional > > describing some functional behavior, rather than a specific hardware IP). > > So Jose's public reassurance means nothing? [ for context to all readers, _this_ public reassurance: https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@DM4PR12MB5088.namprd12.prod.outlook.com/ ] Yup, this is what I'm saying. He basically said that it's outside of Synopsys control how these bits are used in the final integration. And if so, it's also naturally outside of vendor X's (Microchip) control how vendor Y adds integration-specific logic to Synopsys-undefined bits for 1000Base-X mode. Thus, the only thing I'm saying is that it isn't the safest thing we can do, in Linux, to assume that no other integration has added a contradictory vendor-defined behavior for these bits. It's an assumption we aren't even _forced_ to make, so why risk it?
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 1eba0c583f16..d522e4a5a138 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -774,9 +774,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 96117bd9e2b6..f0ddd93c7a22 100644 --- a/drivers/net/pcs/pcs-xpcs.h +++ b/drivers/net/pcs/pcs-xpcs.h @@ -73,6 +73,7 @@ /* VR_MII_AN_CTRL */ #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
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@armlinux.org.uk> --- drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++-- drivers/net/pcs/pcs-xpcs.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-)