[RFC net-next 0/2] Introduce MDIO probe order C45 over C22

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Hence, we introduce MDIOBUS_CAP_C45_C22 which allow us to probe using
Clause-45, if it fails, we then proceed to try with Clause-22.

[1]: https://lkml.org/lkml/2021/3/18/584


Wong Vee Khee (2):
  net: phy: allow mdio bus to probe for c45 devices before c22
  net: stmmac: allow gmac4 to probe for c45 devices before c22

 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 2 +-
 include/linux/phy.h                               | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

On Tue, May 25, 2021 at 01:58:03PM +0800, Wong Vee Khee wrote:

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Is it possible to identify it is a Marvell PHY? Do any of the other
C22 registers return anything unique? I'm wondering if adding
.match_phy_device to genphy would work to identify it is a Marvell PHY
and not bind to it. Or we can turn it around, make the
.match_phy_device specifically look for the fixed-link device by
putting a magic number in one of the vendor registers.

    Andrew

On Tue, May 25, 2021 at 03:34:34PM +0200, Andrew Lunn wrote:

On Tue, May 25, 2021 at 01:58:03PM +0800, Wong Vee Khee wrote:

quoted

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Is it possible to identify it is a Marvell PHY? Do any of the other
C22 registers return anything unique? I'm wondering if adding
.match_phy_device to genphy would work to identify it is a Marvell PHY
and not bind to it. Or we can turn it around, make the
.match_phy_device specifically look for the fixed-link device by
putting a magic number in one of the vendor registers.

I checked the Marvell and did not see any unique register values.
Also, since get_phy_c22_id() returns a *phy_id== 0, it is not bind to
any PHY driver, so I don't think adding the match_phy_device check in
getphy would help.

 VK

On Tue, Jun 01, 2021 at 06:47:34PM +0800, Wong Vee Khee wrote:

On Tue, May 25, 2021 at 03:34:34PM +0200, Andrew Lunn wrote:

quoted

On Tue, May 25, 2021 at 01:58:03PM +0800, Wong Vee Khee wrote:

quoted

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Is it possible to identify it is a Marvell PHY? Do any of the other
C22 registers return anything unique? I'm wondering if adding
.match_phy_device to genphy would work to identify it is a Marvell PHY
and not bind to it. Or we can turn it around, make the
.match_phy_device specifically look for the fixed-link device by
putting a magic number in one of the vendor registers.

I checked the Marvell and did not see any unique register values.
Also, since get_phy_c22_id() returns a *phy_id== 0, it is not bind to
any PHY driver, so I don't think adding the match_phy_device check in
getphy would help.

It has a Marvell ID in C45 space. So maybe we need to special case for
ID 0. If we get that, go look in C45 space. If we find a valid ID, use
it. If we get EOPNOTSUP because the MDIO bus is not C45 capable, or we
don't find a vendor ID in C45 space, keep with id == 0 and load
genphy?

The other option is consider the PHY broken, and require that you put
the correct ID in DT as the compatible string. The correct driver will
then be loaded, based on the compatible string, rather than what is
found by probing.

      Andrew

On Tue, Jun 01, 2021 at 03:04:51PM +0200, Andrew Lunn wrote:

On Tue, Jun 01, 2021 at 06:47:34PM +0800, Wong Vee Khee wrote:

quoted

On Tue, May 25, 2021 at 03:34:34PM +0200, Andrew Lunn wrote:

quoted

On Tue, May 25, 2021 at 01:58:03PM +0800, Wong Vee Khee wrote:

quoted

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Is it possible to identify it is a Marvell PHY? Do any of the other
C22 registers return anything unique? I'm wondering if adding
.match_phy_device to genphy would work to identify it is a Marvell PHY
and not bind to it. Or we can turn it around, make the
.match_phy_device specifically look for the fixed-link device by
putting a magic number in one of the vendor registers.

I checked the Marvell and did not see any unique register values.
Also, since get_phy_c22_id() returns a *phy_id== 0, it is not bind to
any PHY driver, so I don't think adding the match_phy_device check in
getphy would help.

It has a Marvell ID in C45 space. So maybe we need to special case for
ID 0. If we get that, go look in C45 space. If we find a valid ID, use
it. If we get EOPNOTSUP because the MDIO bus is not C45 capable, or we
don't find a vendor ID in C45 space, keep with id == 0 and load
genphy?

Make sense for me.
Let me what you think of adding the checks in *get_phy_device():

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..ad9a87fadea1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -862,11 +862,21 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        c45_ids.mmds_present = 0;
        memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));

-       if (is_c45)
+       if (is_c45) {
                r = get_phy_c45_ids(bus, addr, &c45_ids);
-       else
+       } else {
                r = get_phy_c22_id(bus, addr, &phy_id);

+               if (phy_id == 0) {
+                       r = get_phy_c45_ids(bus, addr, &c45_ids);
+                       if (r == -ENOTSUPP || r == -ENODEV)
+                               return 0;
+                       else
+                               return phy_device_create(bus, addr, phy_id,
+                                                        true, &c45_ids);
+               }
+       }
+
        if (r)
                return ERR_PTR(r);

The other option is consider the PHY broken, and require that you put
the correct ID in DT as the compatible string. The correct driver will
then be loaded, based on the compatible string, rather than what is
found by probing.

Unfortunately all Intel platforms (ElkhartLake/TigerLake/AlderLake) are non-DT.


VK

On Tue, Jun 01, 2021 at 11:44:23PM +0800, Wong Vee Khee wrote:

This bit is not correct. I said 'or we don't find a vendor ID in C45
space, keep with id == 0'. We need to keep backwards compatibility. If
get_phy_c22_id() did not return an error we should create a device
with phy_id 0, if get_phy_c45_ids() returns an error.

     Andrew

On Wed, Jun 02, 2021 at 12:21:58AM +0200, Andrew Lunn wrote:

On Tue, Jun 01, 2021 at 11:44:23PM +0800, Wong Vee Khee wrote:

quoted

On Tue, Jun 01, 2021 at 03:04:51PM +0200, Andrew Lunn wrote:

quoted

On Tue, Jun 01, 2021 at 06:47:34PM +0800, Wong Vee Khee wrote:

quoted

On Tue, May 25, 2021 at 03:34:34PM +0200, Andrew Lunn wrote:

quoted

On Tue, May 25, 2021 at 01:58:03PM +0800, Wong Vee Khee wrote:

quoted

Synopsys MAC controller is capable of pairing with external PHY devices
that accessible via Clause-22 and Clause-45.

There is a problem when it is paired with Marvell 88E2110 which returns
PHY ID of 0 using get_phy_c22_id(). We can add this check in that
function, but this will break swphy, as swphy_reg_reg() return 0. [1]

Is it possible to identify it is a Marvell PHY? Do any of the other
C22 registers return anything unique? I'm wondering if adding
.match_phy_device to genphy would work to identify it is a Marvell PHY
and not bind to it. Or we can turn it around, make the
.match_phy_device specifically look for the fixed-link device by
putting a magic number in one of the vendor registers.

I checked the Marvell and did not see any unique register values.
Also, since get_phy_c22_id() returns a *phy_id== 0, it is not bind to
any PHY driver, so I don't think adding the match_phy_device check in
getphy would help.

It has a Marvell ID in C45 space. So maybe we need to special case for
ID 0. If we get that, go look in C45 space. If we find a valid ID, use
it. If we get EOPNOTSUP because the MDIO bus is not C45 capable, or we
don't find a vendor ID in C45 space, keep with id == 0 and load
genphy?

Make sense for me.
Let me what you think of adding the checks in *get_phy_device():

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..ad9a87fadea1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -862,11 +862,21 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        c45_ids.mmds_present = 0;
        memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));

-       if (is_c45)
+       if (is_c45) {
                r = get_phy_c45_ids(bus, addr, &c45_ids);
-       else
+       } else {
                r = get_phy_c22_id(bus, addr, &phy_id);

+               if (phy_id == 0) {
+                       r = get_phy_c45_ids(bus, addr, &c45_ids);
+                       if (r == -ENOTSUPP || r == -ENODEV)
+                               return 0;

This bit is not correct. I said 'or we don't find a vendor ID in C45
space, keep with id == 0'. We need to keep backwards compatibility. If
get_phy_c22_id() did not return an error we should create a device
with phy_id 0, if get_phy_c45_ids() returns an error.

Yeah, you're right. Thanks for pointing that out. It should be:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..73bfde770f2d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -862,11 +862,22 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        c45_ids.mmds_present = 0;
        memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));

-       if (is_c45)
+       if (is_c45) {
                r = get_phy_c45_ids(bus, addr, &c45_ids);
-       else
+       } else {
                r = get_phy_c22_id(bus, addr, &phy_id);

+               if (phy_id == 0) {
+                       r = get_phy_c45_ids(bus, addr, &c45_ids);
+                       if (r == -ENOTSUPP || r == -ENODEV)
+                               return phy_device_create(bus, addr, phy_id,
+                                                        false, &c45_ids);
+                       else
+                               return phy_device_create(bus, addr, phy_id,
+                                                        true, &c45_ids);
+               }
+       }
+
        if (r)
                return ERR_PTR(r);

VK 

Still not correct. Think about when get_phy_c22_id() returns an
error. Walk through all the different code paths and check they do the
right thing. It is actually a lot more complex than what is shown
here. Think about all the different types of PHYs and all the
different types of MDIO bus drivers.

      Andrew

On Wed, Jun 02, 2021 at 04:19:43AM +0200, Andrew Lunn wrote:

quoted

Yeah, you're right. Thanks for pointing that out. It should be:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..73bfde770f2d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -862,11 +862,22 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        c45_ids.mmds_present = 0;
        memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));

-       if (is_c45)
+       if (is_c45) {
                r = get_phy_c45_ids(bus, addr, &c45_ids);
-       else
+       } else {
                r = get_phy_c22_id(bus, addr, &phy_id);

+               if (phy_id == 0) {
+                       r = get_phy_c45_ids(bus, addr, &c45_ids);
+                       if (r == -ENOTSUPP || r == -ENODEV)
+                               return phy_device_create(bus, addr, phy_id,
+                                                        false, &c45_ids);
+                       else
+                               return phy_device_create(bus, addr, phy_id,
+                                                        true, &c45_ids);

Still not correct. Think about when get_phy_c22_id() returns an
error. Walk through all the different code paths and check they do the
right thing. It is actually a lot more complex than what is shown
here. Think about all the different types of PHYs and all the
different types of MDIO bus drivers.

I took a look at how most ethernet drivers implement their "bus->read"
function. Most of them either return -EIO or -ENODEV.

I think it safe to drop the return error type when we try with C45 access:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..282d16fdf6e1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -870,6 +870,18 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        if (r)
                return ERR_PTR(r);

+       /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
+        * of 0 when probed using get_phy_c22_id() with no error. Proceed to
+        * probe with C45 to see if we're able to get a valid PHY ID in the C45
+        * space, if successful, create the C45 PHY device.
+        */
+       if ((!is_c45) && (phy_id == 0)) {
+               r = get_phy_c45_ids(bus, addr, &c45_ids);
+               if (!r)
+                       return phy_device_create(bus, addr, phy_id,
+                                                true, &c45_ids);
+       }
+
        return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);

With this implementation, it should have handled all four scenarios listed
in the table below:

    *------------------*------------*------------*
    | get_phy_c22_id() |  phy_id==0 |   Handled  |
    |   return error   |            |            |
    *------------------*------------*------------*
    |     false        |    false   |   true     |
    *------------------*------------*------------*
    |     false        |    true    |   true     |
    *------------------*------------*------------*
    |     true         |    false   |   true     |
    *------------------*------------*------------*
    |     true         |    true    |   true     |
    *------------------*------------*------------*


VK

This is getting better. But look at for example
drivers/net/mdio/mdio-bcm-unimac.c. What will happen when you ask it
to do get_phy_c45_ids()?

   Andrew

On Wed, Jun 02, 2021 at 05:03:52PM +0200, Andrew Lunn wrote:

quoted

I took a look at how most ethernet drivers implement their "bus->read"
function. Most of them either return -EIO or -ENODEV.

I think it safe to drop the return error type when we try with C45 access:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..282d16fdf6e1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -870,6 +870,18 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        if (r)
                return ERR_PTR(r);

+       /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
+        * of 0 when probed using get_phy_c22_id() with no error. Proceed to
+        * probe with C45 to see if we're able to get a valid PHY ID in the C45
+        * space, if successful, create the C45 PHY device.
+        */
+       if ((!is_c45) && (phy_id == 0)) {
+               r = get_phy_c45_ids(bus, addr, &c45_ids);
+               if (!r)
+                       return phy_device_create(bus, addr, phy_id,
+                                                true, &c45_ids);
+       }

This is getting better. But look at for example
drivers/net/mdio/mdio-bcm-unimac.c. What will happen when you ask it
to do get_phy_c45_ids()?

I will add an additional check for bus->probe_capabilities. This will ensure
that only a MDIO bus that is capable for C45 access will go for the 'try getting
PHY ID from C45 space' approach. Currently, only Freescale's QorIQ 10G MDIO
Controller driver and STMMAC driver has a bus->probe_capabilities of > MDIOBUS_C45.
So, I would say with this additional checking, it would not break most of the drivers:-

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..460c0866ac84 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -870,6 +870,19 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        if (r)
                return ERR_PTR(r);

+       /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
+        * of 0 when probed using get_phy_c22_id() with no error. Proceed to
+        * probe with C45 to see if we're able to get a valid PHY ID in the C45
+        * space, if successful, create the C45 PHY device.
+        */
+       if ((!is_c45) && (phy_id == 0) &&
+            (bus->probe_capabilities >= MDIOBUS_C45)) {
+               r = get_phy_c45_ids(bus, addr, &c45_ids);
+               if (!r)
+                       return phy_device_create(bus, addr, phy_id,
+                                                true, &c45_ids);
+       }
+
        return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);

  VK

Hi Andrew,

On Thu, Jun 03, 2021 at 07:51:55AM +0800, Wong Vee Khee wrote:

Can you take a look at the latest implementation and provide
feedback?

VK

Can you take a look at the latest implementation and provide
feedback?

I think we are close, so please submit a proper patch.

       Andrew

On Sat, Jun 05, 2021 at 08:36:37PM +0200, Andrew Lunn wrote:

quoted

Can you take a look at the latest implementation and provide
feedback?

I think we are close, so please submit a proper patch.

Will do. Thanks Andrew!

 VK