[PATCH 0/2] Add 100BaseT1 and 1000BaseT1

[PATCH 0/2] Add 100BaseT1 and 1000BaseT1

[Andrew Lunn]

Import the latest ethtool.h and add two new links modes.

Andrew Lunn (2):
  ethtool: sync ethtool-copy.h with linux-next from 30/05/2019
  ethtool: Add 100BaseT1 and 1000BaseT1 link modes

 ethtool-copy.h | 7 ++++++-
 ethtool.c      | 6 ++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] ethtool: sync ethtool-copy.h with linux-next from 30/05/2019

[Andrew Lunn]

Sync ethtool-copy.h with linux-next from 22/05/2019. This provides
access to the new link modes for 100BaseT1 and 1000BaseT1.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 ethtool-copy.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 92ab10d65fc9..ad16e8f9c290 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1481,6 +1481,8 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
 	ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT	 = 65,
 	ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT	 = 66,
+	ETHTOOL_LINK_MODE_100baseT1_Full_BIT             = 67,
+	ETHTOOL_LINK_MODE_1000baseT1_Full_BIT            = 68,
 
 	/* must be last entry */
 	__ETHTOOL_LINK_MODE_MASK_NBITS
@@ -1597,7 +1599,7 @@ enum ethtool_link_mode_bit_indices {
 
 static __inline__ int ethtool_validate_speed(__u32 speed)
 {
-	return speed <= INT_MAX || speed == SPEED_UNKNOWN;
+	return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;
 }
 
 /* Duplex, half or full. */
@@ -1710,6 +1712,9 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
 #define ETH_MODULE_SFF_8436		0x4
 #define ETH_MODULE_SFF_8436_LEN		256
 
+#define ETH_MODULE_SFF_8636_MAX_LEN	640
+#define ETH_MODULE_SFF_8436_MAX_LEN	640
+
 /* Reset flags */
 /* The reset() operation must clear the flags for the components which
  * were actually reset.  On successful return, the flags indicate the
-- 
2.20.1

[PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Andrew Lunn]

The kernel can now indicate if the PHY supports operating over a
single pair at 100Mbps or 1000Mbps.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 ethtool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 66a907edd97b..35158939e04c 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -494,8 +494,10 @@ static void init_global_link_mode_masks(void)
 		ETHTOOL_LINK_MODE_10baseT_Full_BIT,
 		ETHTOOL_LINK_MODE_100baseT_Half_BIT,
 		ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+		ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
 		ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
 		ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+		ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
 		ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
 		ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
 		ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
@@ -634,10 +636,14 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
 		  "100baseT/Half" },
 		{ 1, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
 		  "100baseT/Full" },
+		{ 1, ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+		  "100baseT1/Full" },
 		{ 0, ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
 		  "1000baseT/Half" },
 		{ 1, ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
 		  "1000baseT/Full" },
+		{ 1, ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+		  "1000baseT1/Full" },
 		{ 0, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
 		  "1000baseKX/Full" },
 		{ 0, ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
-- 
2.20.1

Re: [PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Michal Kubecek]

On Thu, May 30, 2019 at 08:06:16PM +0200, Andrew Lunn wrote:
> The kernel can now indicate if the PHY supports operating over a
> single pair at 100Mbps or 1000Mbps.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  ethtool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 66a907edd97b..35158939e04c 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -494,8 +494,10 @@ static void init_global_link_mode_masks(void)
>  		ETHTOOL_LINK_MODE_10baseT_Full_BIT,
>  		ETHTOOL_LINK_MODE_100baseT_Half_BIT,
>  		ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +		ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
>  		ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>  		ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +		ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
>  		ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
>  		ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
>  		ETHTOOL_LINK_MODE_10000baseT_Full_BIT,

The only place where the all_advertised_modes_bits[] array is used is

	ethtool_link_mode_zero(all_advertised_modes);
	ethtool_link_mode_zero(all_advertised_flags);
	for (i = 0; i < ARRAY_SIZE(all_advertised_modes_bits); ++i) {
		ethtool_link_mode_set_bit(all_advertised_modes_bits[i],
					  all_advertised_modes);
		ethtool_link_mode_set_bit(all_advertised_modes_bits[i],
					  all_advertised_flags);
	}

so the order does not really matter. I would prefer to have the elements
ordered the same way as in enum ethtool_link_mode_bit_indices so that
it's easier to check if something is missing.

> @@ -634,10 +636,14 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
>  		  "100baseT/Half" },
>  		{ 1, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>  		  "100baseT/Full" },
> +		{ 1, ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +		  "100baseT1/Full" },
>  		{ 0, ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
>  		  "1000baseT/Half" },
>  		{ 1, ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>  		  "1000baseT/Full" },
> +		{ 1, ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
> +		  "1000baseT1/Full" },
>  		{ 0, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
>  		  "1000baseKX/Full" },
>  		{ 0, ETHTOOL_LINK_MODE_2500baseX_Full_BIT,

Does it mean that we could end up with lines like

                                100baseT/Half 100baseT/Full 100baseT1/Full
                                1000baseT/Full 1000baseT1/Full

if there is a NIC supporting both T and T1? (I'm not sure if it's
possible - but if not, there is no need for setting same_line.) It would
be probably confusing for users as modes on the same line always were
half/full duplex variants of the same.

You should also add the new modes to ethtool.8.in.

Michal

Re: [PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Andrew Lunn]

> > @@ -634,10 +636,14 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
> >  		  "100baseT/Half" },
> >  		{ 1, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> >  		  "100baseT/Full" },
> > +		{ 1, ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> > +		  "100baseT1/Full" },
> >  		{ 0, ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> >  		  "1000baseT/Half" },
> >  		{ 1, ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> >  		  "1000baseT/Full" },
> > +		{ 1, ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
> > +		  "1000baseT1/Full" },
> >  		{ 0, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> >  		  "1000baseKX/Full" },
> >  		{ 0, ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> 
> Does it mean that we could end up with lines like
> 
>                                 100baseT/Half 100baseT/Full 100baseT1/Full
>                                 1000baseT/Full 1000baseT1/Full
> 
> if there is a NIC supporting both T and T1?

Hi Michal

In theory, it is possible for a PHY to support both plain T and
T1. And a 1000BaseT could also implement 1000BaseT2 and 1000BaseT1.
I've not yet seen an actual PHY which does this though.

> It would
> be probably confusing for users as modes on the same line always were
> half/full duplex variants of the same.

I can clear the same_line flag.
 
> You should also add the new modes to ethtool.8.in.

Yes, will do.

     Andrew

Re: [PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Lucas Stach]

Am Freitag, den 31.05.2019, 13:59 +0200 schrieb Andrew Lunn:
> > > @@ -634,10 +636,14 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
> > > > > >  		  "100baseT/Half" },
> > > > > >  		{ 1, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > > > > >  		  "100baseT/Full" },
> > > > > > +		{ 1, ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> > > > > > +		  "100baseT1/Full" },
> > > > > >  		{ 0, ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> > > > > >  		  "1000baseT/Half" },
> > > > > >  		{ 1, ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > > > > >  		  "1000baseT/Full" },
> > > > > > +		{ 1, ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
> > > > > > +		  "1000baseT1/Full" },
> > > > > >  		{ 0, ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
> > > > > >  		  "1000baseKX/Full" },
> > >  		{ 0, ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
> > 
> > Does it mean that we could end up with lines like
> > 
> >                                 100baseT/Half 100baseT/Full 100baseT1/Full
> >                                 1000baseT/Full 1000baseT1/Full
> > 
> > if there is a NIC supporting both T and T1?
> 
> Hi Michal
> 
> In theory, it is possible for a PHY to support both plain T and
> T1.

That's not just theory. The Broadcom BCM54811 PHY supports both
100/1000baseT, as well as 100baseT1.

Regards,
Lucas

Re: [PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Andrew Lunn]

> That's not just theory. The Broadcom BCM54811 PHY supports both
> 100/1000baseT, as well as 100baseT1.

Hi Lucus

There does not appear to be a driver for it, which is why i've not
seen it, nor have we had this conversation before.

Do you have a driver to submit?

   Andrew

Re: [PATCH 2/2] ethtool: Add 100BaseT1 and 1000BaseT1 link modes

[Lucas Stach]

Am Freitag, den 31.05.2019, 14:30 +0200 schrieb Andrew Lunn:
> > That's not just theory. The Broadcom BCM54811 PHY supports both
> > 100/1000baseT, as well as 100baseT1.
> 
> Hi Lucus
> 
> There does not appear to be a driver for it, which is why i've not
> seen it, nor have we had this conversation before.
> 
> Do you have a driver to submit?

No, we've looked at this chip in the past for a project, but never
ended up doing anything with it. But the datasheet is quite clear that
the PHY supports both modes.

It was just a drive-by comment, as something clicked in my mind there.

Regards,
Lucas