[PATCH RFC v2 0/9] Clause 45 PHY probing cleanups

[PATCH RFC v2 0/9] Clause 45 PHY probing cleanups

[Russell King - ARM Linux admin]

Hi,

This is version 2 of my proposal to expand our Clause 45 PHY probing.
This series does not change the functionality beyond probing further
MMDs.

The first four patches clean up get_phy_device() and called functions,
updating the kernel doc, adding information about the various error
return values.

This is not against net-next nor net trees, but against my own private
tree, but I'm posting it to serve as an illustration of what I think
should be done - I knocked this up this morning.

I haven't tested the new changes in version 2 yet beyond compile
testing.

Given the proximity of the merge window, this *isn't* code I'd like to
see merged into net-next - it's way too risky at this point.  So, we
have time to consider our options.

If we want to start scanning for Clause 45 PHYs like we do for Clause
22 PHYs, we definitely need to have indications from the MDIO drivers
that they support Clause 45 accesses, or all MDIO drivers audited to
add the necessary rejection; many of them do not explicitly reject a
request to perform Clause 45 accesses, and will just try and fit the
unmasked register address into their registers, potentially setting
invalid bits when writing their registers.

Changes from v2:
- Further cleanups to get_phy_c45_ids(), get_phy_c22_id() and
  get_phy_device(), with kerneldoc updates to better describe what
  is going on, and what the error return codes signify.

- Only read status register 2 to detect device presence for the
  two vendor MMDs which we know are a potential problem on 88x3310
  PHYs.  We can expand to also check MMDs 1 through 6 if necessary,
  but that would be a behaviour change beyond what this series is
  trying to do.

Unaddressed issues:
- Reading zero from PHY ID registers - OUI 00:00:00 is allocated to
  Xerox Corporation, but it's unlikely that there is a PHY out there
  validly using this OUI. However, I believe that we do know that
  there are PHYs with zero PHY ID registers (in DSA switches, Andrew?)

- mmds_present - I have a patch on top of this which clears the vendor
  MMDs if the devices-present field in status register 2 indicates
  not-present.  We may wish to do this for MMDs 1 through 6 as well
  which have status register 2, but that comes with some risk.

Discussion points:
- drivers/net/phy is becoming quite large, do we want to split it
  into separate subdirectories for PHY drivers, MDIO drivers, and
  core code?
- I have a patch that splits the "genphy" clause 22 code like I did
  with the clause 45 code, which will need refreshing before I
  submit - do my fellow phylib maintainers think that's a good move?

 drivers/net/phy/phy-c45.c    |   4 +-
 drivers/net/phy/phy_device.c | 159 ++++++++++++++++++++++++++++---------------
 drivers/net/phy/phylink.c    |   8 +--
 include/linux/phy.h          |   8 ++-
 4 files changed, 117 insertions(+), 62 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

[PATCH RFC v2 1/9] net: phy: clean up cortina workaround

[Russell King]

Move the Cortina PHY workaround out of the "devices in package" loop;
it doesn't need to be in there as the control flow will terminate the
loop once we enter the workaround irrespective of the workaround's
outcome. The workaround is triggered by the ID being mostly 1's, which
will in any case terminate the loop.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d14c4ba24a90..e04284c4ebf8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -718,23 +718,21 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
 		if (phy_reg < 0)
 			return -EIO;
+	}
+
+	if ((*devs & 0x1fffffff) == 0x1fffffff) {
+		/* If mostly Fs, there is no device there, then let's probe
+		 * MMD 0, as some 10G PHYs have zero Devices In package,
+		 * e.g. Cortina CS4315/CS4340 PHY.
+		 */
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
+		if (phy_reg < 0)
+			return -EIO;
 
+		/* no device there, let's get out of here */
 		if ((*devs & 0x1fffffff) == 0x1fffffff) {
-			/*  If mostly Fs, there is no device there,
-			 *  then let's continue to probe more, as some
-			 *  10G PHYs have zero Devices In package,
-			 *  e.g. Cortina CS4315/CS4340 PHY.
-			 */
-			phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
-			if (phy_reg < 0)
-				return -EIO;
-			/* no device there, let's get out of here */
-			if ((*devs & 0x1fffffff) == 0x1fffffff) {
-				*phy_id = 0xffffffff;
-				return 0;
-			} else {
-				break;
-			}
+			*phy_id = 0xffffffff;
+			return 0;
 		}
 	}
 
-- 
2.20.1

[PATCH RFC v2 2/9] net: phy: clean up PHY ID reading

[Russell King]

Rearrange the code to read the PHY IDs, so we don't call get_phy_id()
only to immediately call get_phy_c45_ids().  Move that logic into
get_phy_device(), which results in better readability.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e04284c4ebf8..0d6b6ca66216 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -756,29 +756,18 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 }
 
 /**
- * get_phy_id - reads the specified addr for its ID.
+ * get_phy_c22_id - reads the specified addr for its clause 22 ID.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
  * @phy_id: where to store the ID retrieved.
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
- * @c45_ids: where to store the c45 ID information.
- *
- * Description: In the case of a 802.3-c22 PHY, reads the ID registers
- *   of the PHY at @addr on the @bus, stores it in @phy_id and returns
- *   zero on success.
- *
- *   In the case of a 802.3-c45 PHY, get_phy_c45_ids() is invoked, and
- *   its return value is in turn returned.
  *
+ * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus.
+ * Return the PHY ID read from the PHY in @phy_id on successful access.
  */
-static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
-		      bool is_c45, struct phy_c45_device_ids *c45_ids)
+static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 {
 	int phy_reg;
 
-	if (is_c45)
-		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
-
 	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
 	if (phy_reg < 0) {
@@ -817,7 +806,11 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	c45_ids.devices_in_package = 0;
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
-	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
+	if (is_c45)
+		r = get_phy_c45_ids(bus, addr, &phy_id, &c45_ids);
+	else
+		r = get_phy_c22_id(bus, addr, &phy_id);
+
 	if (r)
 		return ERR_PTR(r);
 
-- 
2.20.1

Re: [PATCH RFC v2 2/9] net: phy: clean up PHY ID reading

[Florian Fainelli]

On 5/27/2020 3:33 AM, Russell King wrote:
> Rearrange the code to read the PHY IDs, so we don't call get_phy_id()
> only to immediately call get_phy_c45_ids().  Move that logic into
> get_phy_device(), which results in better readability.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 3/9] net: phy: clean up get_phy_c45_ids() failure handling

[Russell King]

When we decide that a PHY is not present, we do not need to go through
the hoops of setting *phy_id to 0xffffffff, and then return zero to
make get_phy_device() fail - we can return -ENODEV which will have the
same effect.

Doing so means we no longer have to pass a pointer to phy_id in, and
we can then clean up the clause 22 path in a similar way.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0d6b6ca66216..53b914ee5031 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -695,16 +695,16 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
  * get_phy_c45_ids - reads the specified addr for its 802.3-c45 IDs.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
- * @phy_id: where to store the ID retrieved.
  * @c45_ids: where to store the c45 ID information.
  *
- *   If the PHY devices-in-package appears to be valid, it and the
- *   corresponding identifiers are stored in @c45_ids, zero is stored
- *   in @phy_id.  Otherwise 0xffffffff is stored in @phy_id.  Returns
- *   zero on success.
+ * Read the PHY "devices in package". If this appears to be valid, read
+ * the PHY identifiers for each device. Return the "devices in package"
+ * and identifiers in @c45_ids.
  *
+ * Returns zero on success, %-EIO on bus access error, or %-ENODEV if
+ * the "devices in package" is invalid.
  */
-static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
+static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 			   struct phy_c45_device_ids *c45_ids)
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
@@ -730,10 +730,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			return -EIO;
 
 		/* no device there, let's get out of here */
-		if ((*devs & 0x1fffffff) == 0x1fffffff) {
-			*phy_id = 0xffffffff;
-			return 0;
-		}
+		if ((*devs & 0x1fffffff) == 0x1fffffff)
+			return -ENODEV;
 	}
 
 	/* Now probe Device Identifiers for each device present. */
@@ -751,7 +749,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			return -EIO;
 		c45_ids->device_ids[i] |= phy_reg;
 	}
-	*phy_id = 0;
+
 	return 0;
 }
 
@@ -807,7 +805,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
 	if (is_c45)
-		r = get_phy_c45_ids(bus, addr, &phy_id, &c45_ids);
+		r = get_phy_c45_ids(bus, addr, &c45_ids);
 	else
 		r = get_phy_c22_id(bus, addr, &phy_id);
 
-- 
2.20.1

Re: [PATCH RFC v2 3/9] net: phy: clean up get_phy_c45_ids() failure handling

[Florian Fainelli]

On 5/27/2020 3:33 AM, Russell King wrote:
> When we decide that a PHY is not present, we do not need to go through
> the hoops of setting *phy_id to 0xffffffff, and then return zero to
> make get_phy_device() fail - we can return -ENODEV which will have the
> same effect.
> 
> Doing so means we no longer have to pass a pointer to phy_id in, and
> we can then clean up the clause 22 path in a similar way.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 4/9] net: phy: clean up get_phy_c22_id() invalid ID handling

[Russell King]

Move the ID check from get_phy_device() into get_phy_c22_id(), which
simplifies get_phy_device(). The ID reading functions are now
responsible for indicating whether they found a PHY or not via their
return code - they must return -ENODEV when a PHY is not present.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 53b914ee5031..424e608cec21 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -759,8 +759,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
  * @addr: PHY address on the MII bus
  * @phy_id: where to store the ID retrieved.
  *
- * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus.
- * Return the PHY ID read from the PHY in @phy_id on successful access.
+ * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus,
+ * placing it in @phy_id. Return zero on successful read and the ID is
+ * valid, %-EIO on bus access error, or %-ENODEV if no device responds
+ * or invalid ID.
  */
 static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 {
@@ -782,6 +784,10 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 
 	*phy_id |= phy_reg;
 
+	/* If the phy_id is mostly Fs, there is no device there */
+	if ((*phy_id & 0x1fffffff) == 0x1fffffff)
+		return -ENODEV;
+
 	return 0;
 }
 
@@ -812,10 +818,6 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is mostly Fs, there is no device there */
-	if ((phy_id & 0x1fffffff) == 0x1fffffff)
-		return ERR_PTR(-ENODEV);
-
 	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);
-- 
2.20.1

Re: [PATCH RFC v2 4/9] net: phy: clean up get_phy_c22_id() invalid ID handling

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> Move the ID check from get_phy_device() into get_phy_c22_id(), which
> simplifies get_phy_device(). The ID reading functions are now
> responsible for indicating whether they found a PHY or not via their
> return code - they must return -ENODEV when a PHY is not present.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 5/9] net: phy: reword get_phy_device() kerneldoc

[Russell King]

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 424e608cec21..bc20ee01b31d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -798,8 +798,17 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
  * @addr: PHY address on the MII bus
  * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
  *
- * Description: Reads the ID registers of the PHY at @addr on the
- *   @bus, then allocates and returns the phy_device to represent it.
+ * Probe for a PHY at @addr on @bus.
+ *
+ * When probing for a clause 22 PHY, then read the ID registers. If we find
+ * a valid ID, allocate and return a &struct phy_device.
+ *
+ * When probing for a clause 45 PHY, read the "devices in package" registers.
+ * If the "devices in package" appears valid, read the ID registers for each
+ * MMD, allocate and return a &struct phy_device.
+ *
+ * Returns an allocated &struct phy_device on success, %-ENODEV if there is
+ * no PHY present, or %-EIO on bus access error.
  */
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-- 
2.20.1

Re: [PATCH RFC v2 5/9] net: phy: reword get_phy_device() kerneldoc

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 6/9] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King]

Add support for probing MMDs above 7 for a valid devices-in-package
specifier, but only probe the vendor MMDs for this if they also report
that there the device is present in status register 2.  This avoids
issues where the MMD is implemented, but does not provide IEEE 802.3
compliant registers (such as the MV88X3310 PHY.)

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 40 ++++++++++++++++++++++++++++++++++--
 include/linux/phy.h          |  2 ++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bc20ee01b31d..79f01cfec774 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 }
 EXPORT_SYMBOL(phy_device_create);
 
+/* phy_c45_probe_present - checks to see if a MMD is present in the package
+ * @bus: the target MII bus
+ * @prtad: PHY package address on the MII bus
+ * @devad: PHY device (MMD) address
+ *
+ * Read the MDIO_STAT2 register, and check whether a device is responding
+ * at this address.
+ *
+ * Returns: negative error number on bus access error, zero if no device
+ * is responding, or positive if a device is present.
+ */
+static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
+{
+	int stat2;
+
+	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+	if (stat2 < 0)
+		return stat2;
+
+	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
+}
+
 /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
@@ -709,12 +731,26 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
 	u32 *devs = &c45_ids->devices_in_package;
-	int i, phy_reg;
+	int i, ret, phy_reg;
 
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
 	 */
-	for (i = 1; i < num_ids && *devs == 0; i++) {
+	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
+		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
+			/* Check that there is a device present at this
+			 * address before reading the devices-in-package
+			 * register to avoid reading garbage from the PHY.
+			 * Some PHYs (88x3310) vendor space is not IEEE802.3
+			 * compliant.
+			 */
+			ret = phy_c45_probe_present(bus, addr, i);
+			if (ret < 0)
+				return -EIO;
+
+			if (!ret)
+				continue;
+		}
 		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
 		if (phy_reg < 0)
 			return -EIO;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9b7c46cf14d3..41c046545354 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -350,6 +350,8 @@ enum phy_state {
 	PHY_NOLINK,
 };
 
+#define MDIO_MMD_NUM 32
+
 /**
  * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
  * @devices_in_package: Bit vector of devices present.
-- 
2.20.1

Re: [PATCH RFC v2 6/9] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> Add support for probing MMDs above 7 for a valid devices-in-package
> specifier, but only probe the vendor MMDs for this if they also report
> that there the device is present in status register 2.  This avoids
> issues where the MMD is implemented, but does not provide IEEE 802.3
> compliant registers (such as the MV88X3310 PHY.)
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH RFC v2 6/9] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Calvin Johnson]

Hi Russell,

On Wed, May 27, 2020 at 11:34:11AM +0100, Russell King wrote:
> Add support for probing MMDs above 7 for a valid devices-in-package
> specifier, but only probe the vendor MMDs for this if they also report
> that there the device is present in status register 2.  This avoids
> issues where the MMD is implemented, but does not provide IEEE 802.3
> compliant registers (such as the MV88X3310 PHY.)

While this patch looks good to me, commit message doesn't seem to align
with the code changes as it is dealing with MMD at addresses 30 & 31.
Can you please clarify?

Thanks
Calvin

> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phy_device.c | 40 ++++++++++++++++++++++++++++++++++--
>  include/linux/phy.h          |  2 ++
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index bc20ee01b31d..79f01cfec774 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  }
>  EXPORT_SYMBOL(phy_device_create);
>  
> +/* phy_c45_probe_present - checks to see if a MMD is present in the package
> + * @bus: the target MII bus
> + * @prtad: PHY package address on the MII bus
> + * @devad: PHY device (MMD) address
> + *
> + * Read the MDIO_STAT2 register, and check whether a device is responding
> + * at this address.
> + *
> + * Returns: negative error number on bus access error, zero if no device
> + * is responding, or positive if a device is present.
> + */
> +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
> +{
> +	int stat2;
> +
> +	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
> +	if (stat2 < 0)
> +		return stat2;
> +
> +	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
> +}
> +
>  /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
>   * @bus: the target MII bus
>   * @addr: PHY address on the MII bus
> @@ -709,12 +731,26 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>  {
>  	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
>  	u32 *devs = &c45_ids->devices_in_package;
> -	int i, phy_reg;
> +	int i, ret, phy_reg;
>  
>  	/* Find first non-zero Devices In package. Device zero is reserved
>  	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>  	 */
> -	for (i = 1; i < num_ids && *devs == 0; i++) {
> +	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> +		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
> +			/* Check that there is a device present at this
> +			 * address before reading the devices-in-package
> +			 * register to avoid reading garbage from the PHY.
> +			 * Some PHYs (88x3310) vendor space is not IEEE802.3
> +			 * compliant.
> +			 */
> +			ret = phy_c45_probe_present(bus, addr, i);
> +			if (ret < 0)
> +				return -EIO;
> +
> +			if (!ret)
> +				continue;
> +		}
>  		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
>  		if (phy_reg < 0)
>  			return -EIO;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 9b7c46cf14d3..41c046545354 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -350,6 +350,8 @@ enum phy_state {
>  	PHY_NOLINK,
>  };
>  
> +#define MDIO_MMD_NUM 32
> +
>  /**
>   * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
>   * @devices_in_package: Bit vector of devices present.
> -- 
> 2.20.1
> 

Re: [PATCH RFC v2 6/9] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King - ARM Linux admin]

On Wed, Jun 10, 2020 at 09:46:33PM +0530, Calvin Johnson wrote:
> Hi Russell,
> 
> On Wed, May 27, 2020 at 11:34:11AM +0100, Russell King wrote:
> > Add support for probing MMDs above 7 for a valid devices-in-package
> > specifier, but only probe the vendor MMDs for this if they also report
> > that there the device is present in status register 2.  This avoids
> > issues where the MMD is implemented, but does not provide IEEE 802.3
> > compliant registers (such as the MV88X3310 PHY.)
> 
> While this patch looks good to me, commit message doesn't seem to align
> with the code changes as it is dealing with MMD at addresses 30 & 31.
> Can you please clarify?

IEEE 802.3 does not define the "device-is-present" two bits in register
8 for all MMDs - it is only present for MMDs 1, 2, 3, 4, 5, 30 and 31.
None of the other MMDs, even those that have been recently defined (at
least in IEEE 802.3-2018) have these bits.

Hence, we can't use them except on the MMDs where they are defined to
be present.

I considered also checking them in MMDs 1, 2, 3, 4, 5, but decided that
the risk of regression was too high for this patch; that's something
which could be added in a separate patch though, to avoid having to
revert the entire thing if a regression is found at a later date.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up

Re: [PATCH RFC v2 6/9] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Calvin Johnson]

On Wed, Jun 10, 2020 at 05:34:17PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jun 10, 2020 at 09:46:33PM +0530, Calvin Johnson wrote:
> > Hi Russell,
> > 
> > On Wed, May 27, 2020 at 11:34:11AM +0100, Russell King wrote:
> > > Add support for probing MMDs above 7 for a valid devices-in-package
> > > specifier, but only probe the vendor MMDs for this if they also report
> > > that there the device is present in status register 2.  This avoids
> > > issues where the MMD is implemented, but does not provide IEEE 802.3
> > > compliant registers (such as the MV88X3310 PHY.)
> > 
> > While this patch looks good to me, commit message doesn't seem to align
> > with the code changes as it is dealing with MMD at addresses 30 & 31.
> > Can you please clarify?
> 
> IEEE 802.3 does not define the "device-is-present" two bits in register
> 8 for all MMDs - it is only present for MMDs 1, 2, 3, 4, 5, 30 and 31.
> None of the other MMDs, even those that have been recently defined (at
> least in IEEE 802.3-2018) have these bits.
> 
> Hence, we can't use them except on the MMDs where they are defined to
> be present.
> 
> I considered also checking them in MMDs 1, 2, 3, 4, 5, but decided that
> the risk of regression was too high for this patch; that's something
> which could be added in a separate patch though, to avoid having to
> revert the entire thing if a regression is found at a later date.
 
It makes sense to me now.

Thanks
Calvin

[PATCH RFC v2 7/9] net: phy: set devices_in_package only after validation

[Russell King]

Only set the devices_in_package to a non-zero value if we find a valid
value for this field, so we avoid leaving it set to e.g. 0x1fffffff.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 79f01cfec774..82a223225901 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 			   struct phy_c45_device_ids *c45_ids)
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
-	u32 *devs = &c45_ids->devices_in_package;
+	u32 devs_in_pkg = 0;
 	int i, ret, phy_reg;
 
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
 	 */
-	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
+	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
 		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
 			/* Check that there is a device present at this
 			 * address before reading the devices-in-package
@@ -751,28 +751,28 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 			if (!ret)
 				continue;
 		}
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 	}
 
-	if ((*devs & 0x1fffffff) == 0x1fffffff) {
+	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
 		/* If mostly Fs, there is no device there, then let's probe
 		 * MMD 0, as some 10G PHYs have zero Devices In package,
 		 * e.g. Cortina CS4315/CS4340 PHY.
 		 */
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 
 		/* no device there, let's get out of here */
-		if ((*devs & 0x1fffffff) == 0x1fffffff)
+		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff)
 			return -ENODEV;
 	}
 
 	/* Now probe Device Identifiers for each device present. */
 	for (i = 1; i < num_ids; i++) {
-		if (!(c45_ids->devices_in_package & (1 << i)))
+		if (!(devs_in_pkg & (1 << i)))
 			continue;
 
 		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
@@ -786,6 +786,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 		c45_ids->device_ids[i] |= phy_reg;
 	}
 
+	c45_ids->devices_in_package = devs_in_pkg;
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH RFC v2 7/9] net: phy: set devices_in_package only after validation

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> Only set the devices_in_package to a non-zero value if we find a valid
> value for this field, so we avoid leaving it set to e.g. 0x1fffffff.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 8/9] net: phy: split devices_in_package

[Russell King]

We have two competing requirements for the devices_in_package field.
We want to use it as a bit array indicating which MMDs are present, but
we also want to know if the Clause 22 registers are present.

Since "devices in package" is a term used in the 802.3 specification,
keep this as the as-specified values read from the PHY, and introduce
a new member "mmds_present" to indicate which MMDs are actually
present in the PHY, derived from the "devices in package" value.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-c45.c    | 4 ++--
 drivers/net/phy/phy_device.c | 6 +++---
 drivers/net/phy/phylink.c    | 8 ++++----
 include/linux/phy.h          | 4 +++-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 8cd952950a75..4b5805e2bd0e 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
 	int val, devad;
 	bool link = true;
 
-	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
+	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
 		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
 		if (val < 0)
 			return val;
@@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
 	int val;
 
 	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
-	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
+	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
 		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 82a223225901..c90409b00e94 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 		return -EIO;
 	*devices_in_package |= phy_reg;
 
-	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
-	*devices_in_package &= ~BIT(0);
-
 	return 0;
 }
 
@@ -787,6 +784,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 	}
 
 	c45_ids->devices_in_package = devs_in_pkg;
+	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+	c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
 
 	return 0;
 }
@@ -855,6 +854,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	int r;
 
 	c45_ids.devices_in_package = 0;
+	c45_ids.mmds_present = 0;
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
 	if (is_c45)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 26bfb80027bd..3d26881157d6 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 		case MII_BMSR:
 		case MII_PHYSID1:
 		case MII_PHYSID2:
-			devad = __ffs(phydev->c45_ids.devices_in_package);
+			devad = __ffs(phydev->c45_ids.mmds_present);
 			break;
 		case MII_ADVERTISE:
 		case MII_LPA:
-			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
+			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
 				return -EINVAL;
 			devad = MDIO_MMD_AN;
 			if (reg == MII_ADVERTISE)
@@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 		case MII_BMSR:
 		case MII_PHYSID1:
 		case MII_PHYSID2:
-			devad = __ffs(phydev->c45_ids.devices_in_package);
+			devad = __ffs(phydev->c45_ids.mmds_present);
 			break;
 		case MII_ADVERTISE:
 		case MII_LPA:
-			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
+			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
 				return -EINVAL;
 			devad = MDIO_MMD_AN;
 			if (reg == MII_ADVERTISE)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 41c046545354..0d41c710339a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -354,11 +354,13 @@ enum phy_state {
 
 /**
  * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
- * @devices_in_package: Bit vector of devices present.
+ * @devices_in_package: IEEE 802.3 devices in package register value.
+ * @mmds_present: bit vector of MMDs present.
  * @device_ids: The device identifer for each present device.
  */
 struct phy_c45_device_ids {
 	u32 devices_in_package;
+	u32 mmds_present;
 	u32 device_ids[8];
 };
 
-- 
2.20.1

Re: [PATCH RFC v2 8/9] net: phy: split devices_in_package

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> We have two competing requirements for the devices_in_package field.
> We want to use it as a bit array indicating which MMDs are present, but
> we also want to know if the Clause 22 registers are present.
> 
> Since "devices in package" is a term used in the 802.3 specification,
> keep this as the as-specified values read from the PHY, and introduce
> a new member "mmds_present" to indicate which MMDs are actually
> present in the PHY, derived from the "devices in package" value.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC v2 9/9] net: phy: read MMD ID from all present MMDs

[Russell King]

Expand the device_ids[] array to allow all MMD IDs to be read rather
than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
register reports that a device really is present here for these new
devices to maintain compatibility with our current behaviour.

88X3310 PHY vendor MMDs do are marked as present in the
devices_in_package, but do not contain IEE 802.3 compatible register
sets in their lower space.  This avoids reading incorrect values as MMD
identifiers.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 13 +++++++++++++
 include/linux/phy.h          |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c90409b00e94..ebd3306610a4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -772,6 +772,19 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 		if (!(devs_in_pkg & (1 << i)))
 			continue;
 
+		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
+			/* Probe the "Device Present" bits for the vendor MMDs
+			 * to ignore these if they do not contain IEEE 802.3
+			 * registers.
+			 */
+			ret = phy_c45_probe_present(bus, addr, i);
+			if (ret < 0)
+				return ret;
+
+			if (!ret)
+				continue;
+		}
+
 		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
 		if (phy_reg < 0)
 			return -EIO;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0d41c710339a..3325dd8fb9ac 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -361,7 +361,7 @@ enum phy_state {
 struct phy_c45_device_ids {
 	u32 devices_in_package;
 	u32 mmds_present;
-	u32 device_ids[8];
+	u32 device_ids[MDIO_MMD_NUM];
 };
 
 struct macsec_context;
-- 
2.20.1

Re: [PATCH RFC v2 9/9] net: phy: read MMD ID from all present MMDs

[Florian Fainelli]

On 5/27/2020 3:34 AM, Russell King wrote:
> Expand the device_ids[] array to allow all MMD IDs to be read rather
> than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
> register reports that a device really is present here for these new
> devices to maintain compatibility with our current behaviour.
> 
> 88X3310 PHY vendor MMDs do are marked as present in the
> devices_in_package, but do not contain IEE 802.3 compatible register
> sets in their lower space.  This avoids reading incorrect values as MMD
> identifiers.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian