LinuxLists.cc - [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg

2023-06-08 16:03:54

Subject: [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg

As reported here [1], there are some inconsistencies for the Q-USGMII
mode speeds and configuration. The first patch in this fixup series
makes so that we correctly report the max speed of 1Gbps for this mode.

The second patch uses a dedicated helper to decode the control word.
This is necessary as although USGMII control words are close to USXGMII,
they don't support the same speeds.

Thanks,

Maxime

[1] : https://lore.kernel.org/netdev/[email protected]/

Maxime Chevallier (2):
net: phylink: report correct max speed for QUSGMII
net: phylink: use a dedicated helper to parse usgmii control word

drivers/net/phy/phylink.c | 41 +++++++++++++++++++++++++++++++++++++--
include/uapi/linux/mdio.h | 3 +++
2 files changed, 42 insertions(+), 2 deletions(-)

--
2.40.1

2023-06-08 16:04:45

by Maxime Chevallier

[permalink] [raw]

Subject: [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word

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 <[email protected]>
---
drivers/net/phy/phylink.c | 39 ++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/mdio.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)

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__ */
--
2.40.1

2023-06-08 16:07:48

by Maxime Chevallier

[permalink] [raw]

Subject: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

2023-06-08 16:48:39

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

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.

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

2023-06-08 16:51:48

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word

On Thu, Jun 08, 2023 at 06:34:14PM +0200, Maxime Chevallier wrote:
> 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

Seems to be a duplicate patch?

Please see my comments on the other submission of this patch.

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

2023-06-08 17:28:09

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

Hi Russell,

On Thu, 8 Jun 2023 17:32:52 +0100
"Russell King (Oracle)" <[email protected]> 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

2023-06-08 17:28:11

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word

Hi Russell,

On Thu, 8 Jun 2023 17:33:46 +0100
"Russell King (Oracle)" <[email protected]> wrote:

> On Thu, Jun 08, 2023 at 06:34:14PM +0200, Maxime Chevallier wrote:
> > 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
>
> Seems to be a duplicate patch?

Heh indeed, I fixed my commit title at the last minute and forgot to
cleanup my outgoing folder... Sorry about that

> Please see my comments on the other submission of this patch.
>

2023-06-08 17:58:33

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

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)" <[email protected]> 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.

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

2023-06-08 17:58:51

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

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

2023-06-09 07:45:39

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word

Hi Russell,

On Thu, 8 Jun 2023 18:30:41 +0100
"Russell King (Oracle)" <[email protected]> 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)" <[email protected]> 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