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

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

[Maxime Chevallier]

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

[PATCH net 1/2] net: phylink: report correct max speed for QUSGMII

[Maxime Chevallier]

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

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

[Maxime Chevallier]

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

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

[Russell King (Oracle)]

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!

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

[Maxime Chevallier]

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.
> 

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

[Maxime Chevallier]

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

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

[Russell King (Oracle)]

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!

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

[Maxime Chevallier]

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

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

[Russell King (Oracle)]

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!

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

[Maxime Chevallier]

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

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

[Simon Horman]

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