| Message ID | 20230608163415.511762-4-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
On Thu, Jun 08, 2023 at 06:34:15PM +0200, Maxime Chevallier wrote: > diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h > index 256b463e47a6..1d20a9082507 100644 > --- a/include/uapi/linux/mdio.h > +++ b/include/uapi/linux/mdio.h > @@ -444,4 +444,7 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) > #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ > #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ > > +/* Usgmii control word is based on Usxgmii, masking away 2.5, 5 and 10Gbps */ > +#define MDIO_USGMII_SPD_MASK 0x0600 This isn't correct: 11:9 Speed: Bit 11, 10, 9: 1 1 1 to 011 = Reserved 0 1 0 = 1000 Mbps: 1000BASE-TX, 1000BASE-X 0 0 1 = 100 Mbps: 100BASE-TX, 100BASE-FX 0 0 0 = 10 Mbps: 10BASET, 10BASE2, 10BASE5 If we only look at bits 10 and 9, then we're interpreting the reserved combinations as valid as well.
On Thu, Jun 08, 2023 at 07:53:30PM +0200, Maxime Chevallier wrote: > Hi Russell, > > On Thu, 8 Jun 2023 17:32:52 +0100 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Thu, Jun 08, 2023 at 06:34:15PM +0200, Maxime Chevallier wrote: > > > diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h > > > index 256b463e47a6..1d20a9082507 100644 > > > --- a/include/uapi/linux/mdio.h > > > +++ b/include/uapi/linux/mdio.h > > > @@ -444,4 +444,7 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) > > > #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ > > > #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ > > > > > > +/* Usgmii control word is based on Usxgmii, masking away 2.5, 5 and 10Gbps */ > > > +#define MDIO_USGMII_SPD_MASK 0x0600 > > > > This isn't correct: > > > > 11:9 Speed: Bit 11, 10, 9: > > 1 1 1 to 011 = Reserved > > 0 1 0 = 1000 Mbps: 1000BASE-TX, 1000BASE-X > > 0 0 1 = 100 Mbps: 100BASE-TX, 100BASE-FX > > 0 0 0 = 10 Mbps: 10BASET, 10BASE2, 10BASE5 > > > > If we only look at bits 10 and 9, then we're interpreting the reserved > > combinations as valid as well. > > That's why I rewrote the decoding helper instead of simply masking away > the extra bit, so that we exclude the 0 1 1 combination ( 10G speed ). I don't think you've understood my comment properly. Here is what the code is doing: +#define MDIO_USGMII_SPD_MASK 0x0600 + switch (lpa & MDIO_USGMII_SPD_MASK) { #define MDIO_USXGMII_10 0x0000 /* 10Mbps */ + case MDIO_USXGMII_10: + state->speed = SPEED_10; + break; #define MDIO_USXGMII_100 0x0200 /* 100Mbps */ + case MDIO_USXGMII_100: + state->speed = SPEED_100; + break; #define MDIO_USXGMII_1000 0x0400 /* 1000Mbps */ + case MDIO_USXGMII_1000: + state->speed = SPEED_1000; + break; + default: + state->link = false; + return; + } So, this will decode bits 11:9 as: 000 10Mbps 001 100Mbps 010 1000Mgps 011 link = false 100 10Mbps 101 100Mbps 110 1000Mbps 111 link = false Whereas, USGMII says the last four are all reserved. Why does this happen? Because the mask is defined as: +#define MDIO_USGMII_SPD_MASK 0x0600 which only covers bits 10 and 9, masking off bit 11. However, bit 11 is _still_ part of the field, and if it's set, then it is a "reserved" speed. We should not be just ignoring bit 11. I hope that helps to clarify.
On Thu, Jun 08, 2023 at 06:34:15PM +0200, Maxime Chevallier wrote: ... > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 809e6d5216dc..730f8860d2a6 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -3298,6 +3298,41 @@ void phylink_decode_usxgmii_word(struct phylink_link_state *state, > } > EXPORT_SYMBOL_GPL(phylink_decode_usxgmii_word); > > +/** > + * phylink_decode_usgmii_word() - decode the USGMII word from a MAC PCS > + * @state: a pointer to a struct phylink_link_state. > + * @lpa: a 16 bit value which stores the USGMII auto-negotiation word > + * > + * Helper for MAC PCS supporting the USGMII protocol and the auto-negotiation > + * code word. Decode the USGMII code word and populate the corresponding fields > + * (speed, duplex) into the phylink_link_state structure. The structure for this > + * word is the same as the USXGMII word, expect it only supports speeds up to > + * 1Gbps. > + */ > +static void phylink_decode_usgmii_word(struct phylink_link_state *state, > + uint16_t lpa) Hi Maxime, a minor nit from my side: the indentation of the line above should line up with the inside of the opening parentheses on the previous line. static void phylink_decode_usgmii_word(struct phylink_link_state *state, uint16_t lpa) ... As I see there is feedback from Russell, of a more substantial nature, and it looks will be a v2, I'll mark this as changes requested in patchwork. pw-bot: cr
Hi Russell, On Thu, 8 Jun 2023 17:32:52 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Jun 08, 2023 at 06:34:15PM +0200, Maxime Chevallier wrote: > > diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h > > index 256b463e47a6..1d20a9082507 100644 > > --- a/include/uapi/linux/mdio.h > > +++ b/include/uapi/linux/mdio.h > > @@ -444,4 +444,7 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) > > #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ > > #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ > > > > +/* Usgmii control word is based on Usxgmii, masking away 2.5, 5 and 10Gbps */ > > +#define MDIO_USGMII_SPD_MASK 0x0600 > > This isn't correct: > > 11:9 Speed: Bit 11, 10, 9: > 1 1 1 to 011 = Reserved > 0 1 0 = 1000 Mbps: 1000BASE-TX, 1000BASE-X > 0 0 1 = 100 Mbps: 100BASE-TX, 100BASE-FX > 0 0 0 = 10 Mbps: 10BASET, 10BASE2, 10BASE5 > > If we only look at bits 10 and 9, then we're interpreting the reserved > combinations as valid as well. That's why I rewrote the decoding helper instead of simply masking away the extra bit, so that we exclude the 0 1 1 combination ( 10G speed ). The comment is wrong though. This patch set needs more coffee, Best regards, Maxime
Hi Russell, On Thu, 8 Jun 2023 18:30:41 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Thu, Jun 08, 2023 at 07:53:30PM +0200, Maxime Chevallier wrote: > > Hi Russell, > > > > On Thu, 8 Jun 2023 17:32:52 +0100 > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > > > On Thu, Jun 08, 2023 at 06:34:15PM +0200, Maxime Chevallier wrote: > > > > diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h > > > > index 256b463e47a6..1d20a9082507 100644 > > > > --- a/include/uapi/linux/mdio.h > > > > +++ b/include/uapi/linux/mdio.h > > > > @@ -444,4 +444,7 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) > > > > #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ > > > > #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ > > > > > > > > +/* Usgmii control word is based on Usxgmii, masking away 2.5, 5 and 10Gbps */ > > > > +#define MDIO_USGMII_SPD_MASK 0x0600 > > > > > > This isn't correct: > > > > > > 11:9 Speed: Bit 11, 10, 9: > > > 1 1 1 to 011 = Reserved > > > 0 1 0 = 1000 Mbps: 1000BASE-TX, 1000BASE-X > > > 0 0 1 = 100 Mbps: 100BASE-TX, 100BASE-FX > > > 0 0 0 = 10 Mbps: 10BASET, 10BASE2, 10BASE5 > > > > > > If we only look at bits 10 and 9, then we're interpreting the reserved > > > combinations as valid as well. > > > > That's why I rewrote the decoding helper instead of simply masking away > > the extra bit, so that we exclude the 0 1 1 combination ( 10G speed ). > > I don't think you've understood my comment properly. Here is what > the code is doing: Indeed :( thanks for the clarifications. > +#define MDIO_USGMII_SPD_MASK 0x0600 > + switch (lpa & MDIO_USGMII_SPD_MASK) { > #define MDIO_USXGMII_10 0x0000 /* 10Mbps */ > + case MDIO_USXGMII_10: > + state->speed = SPEED_10; > + break; > #define MDIO_USXGMII_100 0x0200 /* 100Mbps */ > + case MDIO_USXGMII_100: > + state->speed = SPEED_100; > + break; > #define MDIO_USXGMII_1000 0x0400 /* 1000Mbps */ > + case MDIO_USXGMII_1000: > + state->speed = SPEED_1000; > + break; > + default: > + state->link = false; > + return; > + } > > So, this will decode bits 11:9 as: > > 000 10Mbps > 001 100Mbps > 010 1000Mgps > 011 link = false > 100 10Mbps > 101 100Mbps > 110 1000Mbps > 111 link = false > > Whereas, USGMII says the last four are all reserved. Why does this > happen? Because the mask is defined as: > > +#define MDIO_USGMII_SPD_MASK 0x0600 > > which only covers bits 10 and 9, masking off bit 11. However, bit 11 > is _still_ part of the field, and if it's set, then it is a "reserved" > speed. We should not be just ignoring bit 11. > > I hope that helps to clarify. > Indeed, thanks... I'll respin with a proper implementation this time. Thanks for spotting this Maxime
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 809e6d5216dc..730f8860d2a6 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -3298,6 +3298,41 @@ void phylink_decode_usxgmii_word(struct phylink_link_state *state, } EXPORT_SYMBOL_GPL(phylink_decode_usxgmii_word); +/** + * phylink_decode_usgmii_word() - decode the USGMII word from a MAC PCS + * @state: a pointer to a struct phylink_link_state. + * @lpa: a 16 bit value which stores the USGMII auto-negotiation word + * + * Helper for MAC PCS supporting the USGMII protocol and the auto-negotiation + * code word. Decode the USGMII code word and populate the corresponding fields + * (speed, duplex) into the phylink_link_state structure. The structure for this + * word is the same as the USXGMII word, expect it only supports speeds up to + * 1Gbps. + */ +static void phylink_decode_usgmii_word(struct phylink_link_state *state, + uint16_t lpa) +{ + switch (lpa & MDIO_USGMII_SPD_MASK) { + case MDIO_USXGMII_10: + state->speed = SPEED_10; + break; + case MDIO_USXGMII_100: + state->speed = SPEED_100; + break; + case MDIO_USXGMII_1000: + state->speed = SPEED_1000; + break; + default: + state->link = false; + return; + } + + if (lpa & MDIO_USXGMII_FULL_DUPLEX) + state->duplex = DUPLEX_FULL; + else + state->duplex = DUPLEX_HALF; +} + /** * phylink_mii_c22_pcs_decode_state() - Decode MAC PCS state from MII registers * @state: a pointer to a &struct phylink_link_state. @@ -3335,9 +3370,11 @@ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, case PHY_INTERFACE_MODE_SGMII: case PHY_INTERFACE_MODE_QSGMII: - case PHY_INTERFACE_MODE_QUSGMII: phylink_decode_sgmii_word(state, lpa); break; + case PHY_INTERFACE_MODE_QUSGMII: + phylink_decode_usgmii_word(state, lpa); + break; default: state->link = false; diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h index 256b463e47a6..1d20a9082507 100644 --- a/include/uapi/linux/mdio.h +++ b/include/uapi/linux/mdio.h @@ -444,4 +444,7 @@ static inline __u16 mdio_phy_id_c45(int prtad, int devad) #define MDIO_USXGMII_5000FULL 0x1a00 /* 5000Mbps full-duplex */ #define MDIO_USXGMII_LINK 0x8000 /* PHY link with copper-side partner */ +/* Usgmii control word is based on Usxgmii, masking away 2.5, 5 and 10Gbps */ +#define MDIO_USGMII_SPD_MASK 0x0600 + #endif /* _UAPI__LINUX_MDIO_H__ */
Q-USGMII is a derivative of USGMII, that uses a specific formatting for the control word. The layout is close to the USXGMII control word, but doesn't support speeds over 1Gbps. Use a dedicated decoding logic for the USGMII control word, re-using USXGMII definitions with a custom mask and only considering 10/100/1000 speeds Fixes: 5e61fe157a27 ("net: phy: Introduce QUSGMII PHY mode") Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/phy/phylink.c | 39 ++++++++++++++++++++++++++++++++++++++- include/uapi/linux/mdio.h | 3 +++ 2 files changed, 41 insertions(+), 1 deletion(-)