[PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[Lucas Stach]

100BASE-T1 is the automotive ethernet standard 802.3bw-2015. Currently
we don't detect any valid modes for PHYs, which only support this
standard. Add support to detect the common 100Mbit full-duplex mode.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/phy/phy_device.c | 2 ++
 include/uapi/linux/mii.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 67f25ac29025..8ef48b38d97b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1607,6 +1607,8 @@ int genphy_config_init(struct phy_device *phydev)
 		if (val < 0)
 			return val;
 
+		if (val & ESTATUS_100T1_FULL)
+			features |= SUPPORTED_100baseT_Full;
 		if (val & ESTATUS_1000_TFULL)
 			features |= SUPPORTED_1000baseT_Full;
 		if (val & ESTATUS_1000_THALF)
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index b5c2fdcf23fd..eb5cc45d23fb 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -121,6 +121,7 @@
 #define EXPANSION_MFAULTS	0x0010	/* Multiple faults detected    */
 #define EXPANSION_RESV		0xffe0	/* Unused...                   */
 
+#define ESTATUS_100T1_FULL	0x0080	/* Can do 100BASE-T1 Full      */
 #define ESTATUS_1000_TFULL	0x2000	/* Can do 1000BT Full          */
 #define ESTATUS_1000_THALF	0x1000	/* Can do 1000BT Half          */
 
-- 
2.11.0

[PATCH 2/3] net: phy: select sensible mode for non-autoneg PHYs on startup

[Lucas Stach]

Init speed and duplex to unknown, so phy_lookup_setting() knows that
it should select the mode only based on the PHY allowed link modes.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/phy/phy-core.c   | 5 +++++
 drivers/net/phy/phy_device.c | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 21f75ae244b3..149a4bab1e6f 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -149,6 +149,11 @@ phy_lookup_setting(int speed, int duplex, const unsigned long *mask,
 	const struct phy_setting *p, *match = NULL, *last = NULL;
 	int i;
 
+	if (!exact && speed == SPEED_UNKNOWN)
+		speed = INT_MAX;
+	if (!exact && duplex == DUPLEX_UNKNOWN)
+		duplex = DUPLEX_FULL;
+
 	for (i = 0, p = settings; i < ARRAY_SIZE(settings); i++, p++) {
 		if (p->bit < maxbit && test_bit(p->bit, mask)) {
 			last = p;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8ef48b38d97b..35278282259a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1785,6 +1785,8 @@ static int phy_probe(struct device *dev)
 	phydev->supported = phydrv->features;
 	of_set_phy_supported(phydev);
 	phydev->advertising = phydev->supported;
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
 
 	/* Get the EEE modes we want to prohibit. We will ask
 	 * the PHY stop advertising these mode later on
-- 
2.11.0

[PATCH 3/3] net: phy: sanitize autoneg in phy_start_aneg_priv

[Lucas Stach]

phy_sanitize_settings() is only called when autonegotiation has been
explicitly disabled. This breaks PHYs without any autonegotiation
capability, as the check for the capability happens inside this function.

Move the check out to the caller, so it is properly applied for those
PHYs.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/phy/phy.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2b1e67bc1e73..433d859b6955 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -226,10 +226,6 @@ static void phy_sanitize_settings(struct phy_device *phydev)
 	const struct phy_setting *setting;
 	u32 features = phydev->supported;
 
-	/* Sanitize settings based on PHY capabilities */
-	if ((features & SUPPORTED_Autoneg) == 0)
-		phydev->autoneg = AUTONEG_DISABLE;
-
 	setting = phy_find_valid(phydev->speed, phydev->duplex, features);
 	if (setting) {
 		phydev->speed = setting->speed;
@@ -487,6 +483,10 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
 
 	mutex_lock(&phydev->lock);
 
+	/* Sanitize settings based on PHY capabilities */
+	if ((phydev->supported & SUPPORTED_Autoneg) == 0)
+		phydev->autoneg = AUTONEG_DISABLE;
+
 	if (AUTONEG_DISABLE == phydev->autoneg)
 		phy_sanitize_settings(phydev);
 
-- 
2.11.0

Re: [PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[Andrew Lunn]

On Wed, Dec 13, 2017 at 06:37:49PM +0100, Lucas Stach wrote:
> 100BASE-T1 is the automotive ethernet standard 802.3bw-2015. Currently
> we don't detect any valid modes for PHYs, which only support this
> standard. Add support to detect the common 100Mbit full-duplex mode.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/net/phy/phy_device.c | 2 ++
>  include/uapi/linux/mii.h     | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 67f25ac29025..8ef48b38d97b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1607,6 +1607,8 @@ int genphy_config_init(struct phy_device *phydev)
>  		if (val < 0)
>  			return val;
>  
> +		if (val & ESTATUS_100T1_FULL)
> +			features |= SUPPORTED_100baseT_Full;

Hi Lucas

Why did you decide to do this, and not add a SUPPORTED_100baseT1?

Could a device support both 100-BASE-T and 100-BASE-T1?  If at some
point we need to differentiate between them, it is going to be
hard. Especially since this is part of the kernel ABI.

      Andrew

Re: [PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[Lucas Stach]

Hi Andrew,

Am Mittwoch, den 13.12.2017, 21:11 +0100 schrieb Andrew Lunn:
> On Wed, Dec 13, 2017 at 06:37:49PM +0100, Lucas Stach wrote:
> > 100BASE-T1 is the automotive ethernet standard 802.3bw-2015.
> > Currently
> > we don't detect any valid modes for PHYs, which only support this
> > standard. Add support to detect the common 100Mbit full-duplex
> > mode.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/net/phy/phy_device.c | 2 ++
> >  include/uapi/linux/mii.h     | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phy_device.c
> > b/drivers/net/phy/phy_device.c
> > index 67f25ac29025..8ef48b38d97b 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1607,6 +1607,8 @@ int genphy_config_init(struct phy_device
> > *phydev)
> >  		if (val < 0)
> >  			return val;
> >  
> > +		if (val & ESTATUS_100T1_FULL)
> > +			features |= SUPPORTED_100baseT_Full;
> 
> Hi Lucas
> 
> Why did you decide to do this, and not add a SUPPORTED_100baseT1?
> 
> Could a device support both 100-BASE-T and 100-BASE-T1?  If at some
> point we need to differentiate between them, it is going to be
> hard. Especially since this is part of the kernel ABI.

Networking and especially PHY isn't really my primary area of
expertise, so excuse my ignorance. My reasoning was that we don't
differentiate between 100BASE-T2 and 100BASE-T4 in the kernel today, so
I thought it was fine to handle T1 the same way.

There are PHYs that can both do regular 100/1000 MBit Ethernet and
100BASE-T1, but definitely not at the same time or over the same
electrical wiring. 100BASE-T1 is really different in that it uses
capacitive coupling, instead of magnetic like on regular Ethernet. So
it is really a board level decision what gets used and is not something
I would expect to change at runtime.

I'll leave it to your judgment if this patch seems fine with the above
information in mind. Happy to rework if needed.

Regards,
Lucas

Re: [PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[Andrew Lunn]

> > Hi Lucas
> > 
> > Why did you decide to do this, and not add a SUPPORTED_100baseT1?
> > 
> > Could a device support both 100-BASE-T and 100-BASE-T1?  If at some
> > point we need to differentiate between them, it is going to be
> > hard. Especially since this is part of the kernel ABI.
> 
> Networking and especially PHY isn't really my primary area of
> expertise, so excuse my ignorance. My reasoning was that we don't
> differentiate between 100BASE-T2 and 100BASE-T4 in the kernel today, so
> I thought it was fine to handle T1 the same way.
> 
> There are PHYs that can both do regular 100/1000 MBit Ethernet and
> 100BASE-T1, but definitely not at the same time or over the same
> electrical wiring. 100BASE-T1 is really different in that it uses
> capacitive coupling, instead of magnetic like on regular Ethernet. So
> it is really a board level decision what gets used and is not something
> I would expect to change at runtime.

Hi Lucus

http://www.marvell.com/docs/automotive/assets/marvell-automotive-ethernet-88Q5050-product-brief-2017-07.pdf

This is a Marvell 8-port switch. It appears it can switch some of its
ports between T1, TX, xMII, GMII and SGMII.

So maybe an end device is fixed to 100BASE-T1, but it looks like
switches could be more flexible.

So i think we should be able to differentiate between T1 and TX.
We might also need an PHY_INTERFACE_MODE_100BASE_T1.

Florian, what do you think?

   Andrew

Re: [PATCH 1/3] net: phy: add support to detect 100BASE-T1 capability

[Lucas Stach]

Am Donnerstag, den 14.12.2017, 10:46 +0100 schrieb Andrew Lunn:
> > > Hi Lucas
> > > 
> > > Why did you decide to do this, and not add a SUPPORTED_100baseT1?
> > > 
> > > Could a device support both 100-BASE-T and 100-BASE-T1?  If at
> > > some
> > > point we need to differentiate between them, it is going to be
> > > hard. Especially since this is part of the kernel ABI.
> > 
> > Networking and especially PHY isn't really my primary area of
> > expertise, so excuse my ignorance. My reasoning was that we don't
> > differentiate between 100BASE-T2 and 100BASE-T4 in the kernel
> > today, so
> > I thought it was fine to handle T1 the same way.
> > 
> > There are PHYs that can both do regular 100/1000 MBit Ethernet and
> > 100BASE-T1, but definitely not at the same time or over the same
> > electrical wiring. 100BASE-T1 is really different in that it uses
> > capacitive coupling, instead of magnetic like on regular Ethernet.
> > So
> > it is really a board level decision what gets used and is not
> > something
> > I would expect to change at runtime.
> 
> Hi Lucus
> 
> http://www.marvell.com/docs/automotive/assets/marvell-automotive-ethe
> rnet-88Q5050-product-brief-2017-07.pdf
> 
> This is a Marvell 8-port switch. It appears it can switch some of its
> ports between T1, TX, xMII, GMII and SGMII.
> 
> So maybe an end device is fixed to 100BASE-T1, but it looks like
> switches could be more flexible.

If you need this for the configuration of the switch in userspace, then
yes I agree that we should be able to differentiate between TX and T1. 

I'll just note that even while you can switch the PHY mode it won't
make much sense at runtime, as you won't be able to connect T1 to a
switch port that has standard Ethernet magnetics at this PHY port.

> So i think we should be able to differentiate between T1 and TX.
> We might also need an PHY_INTERFACE_MODE_100BASE_T1.

At least the PHYs I've looked at expose regular RGMII or (R)MII to the
MAC.

Again, if you need this for switch configuration, I'm happy to add it.

Regards,
Lucas