* Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word Maxime Chevallier
@ 2023-06-08 16:32 ` Russell King (Oracle)
2023-06-08 17:53 ` Maxime Chevallier
2023-06-08 17:45 ` Simon Horman
1 sibling, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2023-06-08 16:32 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word Maxime Chevallier
@ 2023-06-08 16:33 ` Russell King (Oracle)
2023-06-08 17:54 ` Maxime Chevallier
0 siblings, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2023-06-08 16:33 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg
@ 2023-06-08 16:34 Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 1/2] net: phylink: report correct max speed for QUSGMII Maxime Chevallier
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 16:34 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
linux-arm-kernel, Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver,
Vladimir Oltean
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/ZHnd+6FUO77XFJvQ@shell.armlinux.org.uk/
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net 1/2] net: phylink: report correct max speed for QUSGMII
2023-06-08 16:34 [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg Maxime Chevallier
@ 2023-06-08 16:34 ` Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word Maxime Chevallier
2 siblings, 0 replies; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 16:34 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
linux-arm-kernel, Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver,
Vladimir Oltean
Q-USGMII is the quad port version of USGMII, and supports a max speed of
1Gbps on each line. Make so that phylink_interface_max_speed() reports
this information correctly.
Fixes: ae0e4bb2a0e0 ("net: phylink: Adjust link settings based on rate matching")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/phy/phylink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index b4831110003c..809e6d5216dc 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -188,6 +188,7 @@ static int phylink_interface_max_speed(phy_interface_t interface)
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_QUSGMII:
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_GMII:
return SPEED_1000;
@@ -204,7 +205,6 @@ static int phylink_interface_max_speed(phy_interface_t interface)
case PHY_INTERFACE_MODE_10GBASER:
case PHY_INTERFACE_MODE_10GKR:
case PHY_INTERFACE_MODE_USXGMII:
- case PHY_INTERFACE_MODE_QUSGMII:
return SPEED_10000;
case PHY_INTERFACE_MODE_25GBASER:
--
2.40.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word
2023-06-08 16:34 [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 1/2] net: phylink: report correct max speed for QUSGMII Maxime Chevallier
@ 2023-06-08 16:34 ` Maxime Chevallier
2023-06-08 16:33 ` Russell King (Oracle)
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word Maxime Chevallier
2 siblings, 1 reply; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 16:34 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
linux-arm-kernel, Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver,
Vladimir Oltean
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 16:34 [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 1/2] net: phylink: report correct max speed for QUSGMII Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word Maxime Chevallier
@ 2023-06-08 16:34 ` Maxime Chevallier
2023-06-08 16:32 ` Russell King (Oracle)
2023-06-08 17:45 ` Simon Horman
2 siblings, 2 replies; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 16:34 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, thomas.petazzoni,
Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
linux-arm-kernel, Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver,
Vladimir Oltean
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 17:53 ` Maxime Chevallier
@ 2023-06-08 17:30 ` Russell King (Oracle)
2023-06-09 8:09 ` Maxime Chevallier
0 siblings, 1 reply; 11+ messages in thread
From: Russell King (Oracle) @ 2023-06-08 17:30 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word Maxime Chevallier
2023-06-08 16:32 ` Russell King (Oracle)
@ 2023-06-08 17:45 ` Simon Horman
1 sibling, 0 replies; 11+ messages in thread
From: Simon Horman @ 2023-06-08 17:45 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, Russell King,
linux-arm-kernel, Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver,
Vladimir Oltean
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 16:32 ` Russell King (Oracle)
@ 2023-06-08 17:53 ` Maxime Chevallier
2023-06-08 17:30 ` Russell King (Oracle)
0 siblings, 1 reply; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 17:53 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word
2023-06-08 16:33 ` Russell King (Oracle)
@ 2023-06-08 17:54 ` Maxime Chevallier
0 siblings, 0 replies; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-08 17:54 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
Hi Russell,
On Thu, 8 Jun 2023 17:33:46 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> 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.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word
2023-06-08 17:30 ` Russell King (Oracle)
@ 2023-06-09 8:09 ` Maxime Chevallier
0 siblings, 0 replies; 11+ messages in thread
From: Maxime Chevallier @ 2023-06-09 8:09 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: davem, netdev, linux-kernel, thomas.petazzoni, Andrew Lunn,
Florian Fainelli, Heiner Kallweit, linux-arm-kernel,
Horatiu.Vultur, Allan.Nielsen, UNGLinuxDriver, Vladimir Oltean
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-06-09 7:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-08 16:34 [PATCH net 0/2] fixes for Q-USGMII speeds and autoneg Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 1/2] net: phylink: report correct max speed for QUSGMII Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use a dedicated helper to parse usgmii control word Maxime Chevallier
2023-06-08 16:33 ` Russell King (Oracle)
2023-06-08 17:54 ` Maxime Chevallier
2023-06-08 16:34 ` [PATCH net 2/2] net: phylink: use USXGMII control-word format to parse Q-USGMII word Maxime Chevallier
2023-06-08 16:32 ` Russell King (Oracle)
2023-06-08 17:53 ` Maxime Chevallier
2023-06-08 17:30 ` Russell King (Oracle)
2023-06-09 8:09 ` Maxime Chevallier
2023-06-08 17:45 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).