[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alexandru Marginean]

Current code fails to probe some C45 PHYs that also respond to C22 reads.
This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
previously posted on the u-boot list).
If the PHY ID reads all 0s just ignore it and try the next devad.

Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
---
 drivers/net/phy/phy.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c1c1af9abd..7ccbc4d9da 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
 	while (phy_mask) {
 		int addr = ffs(phy_mask) - 1;
 		int r = get_phy_id(bus, addr, devad, &phy_id);
+
+		/* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
+		 * that return all 0s for C22 reads (like Aquantia AQR112) and
+		 * there are C22 PHYs that return all 0s for C45 reads (like
+		 * Atheros AR8035).
+		 */
+		if (phy_id == 0)
+			return NULL;
+
 		/* If the PHY ID is mostly f's, we didn't find anything */
 		if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
 			is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
-- 
2.17.1

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Ramon Fried]

On Wed, Jun 19, 2019 at 12:07 PM Alexandru Marginean
<alexandru.marginean@nxp.com> wrote:
>
> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> previously posted on the u-boot list).
> If the PHY ID reads all 0s just ignore it and try the next devad.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>  drivers/net/phy/phy.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c1c1af9abd..7ccbc4d9da 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>         while (phy_mask) {
>                 int addr = ffs(phy_mask) - 1;
>                 int r = get_phy_id(bus, addr, devad, &phy_id);
> +
> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> +                * there are C22 PHYs that return all 0s for C45 reads (like
> +                * Atheros AR8035).
> +                */
> +               if (phy_id == 0)
> +                       return NULL;
> +
>                 /* If the PHY ID is mostly f's, we didn't find anything */
>                 if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>                         is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
> --
> 2.17.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

[U-Boot] [PATCH v2] drivers: net: phy: Use Aquantia driver for AQR112, AQR412

[Alexandru Marginean]

adds AQR112 and AQR412 to the list of supported PHYs using existing AQR
code.

Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
---

Changes in v2:
	- Numerical ordering of structure definition and function calls

 drivers/net/phy/aquantia.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index 5c3298d612..465ec2d342 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -461,6 +461,19 @@ struct phy_driver aqr107_driver =
 	.shutdown =en10g_shutdown,
 };
 
+struct phy_driver aqr112_driver =
+	.name =quantia AQR112",
+	.uid =3a1b660,
+	.mask =fffffff0,
+	.features =Y_10G_FEATURES,
+	.mmds =DIO_MMD_PMAPMD | MDIO_MMD_PCS |
+		 MDIO_MMD_PHYXS | MDIO_MMD_AN |
+		 MDIO_MMD_VEND1),
+	.config =quantia_config,
+	.startup =quantia_startup,
+	.shutdown =en10g_shutdown,
+};
+
 struct phy_driver aqr405_driver =
 	.name =quantia AQR405",
 	.uid =3a1b4b2,
@@ -474,6 +487,19 @@ struct phy_driver aqr405_driver =
 	.shutdown =en10g_shutdown,
 };
 
+struct phy_driver aqr412_driver =
+	.name =quantia AQR412",
+	.uid =3a1b710,
+	.mask =fffffff0,
+	.features =Y_10G_FEATURES,
+	.mmds =DIO_MMD_PMAPMD | MDIO_MMD_PCS |
+		 MDIO_MMD_PHYXS | MDIO_MMD_AN |
+		 MDIO_MMD_VEND1),
+	.config =quantia_config,
+	.startup =quantia_startup,
+	.shutdown =en10g_shutdown,
+};
+
 int phy_aquantia_init(void)
 {
 	phy_register(&aq1202_driver);
@@ -481,7 +507,9 @@ int phy_aquantia_init(void)
 	phy_register(&aqr105_driver);
 	phy_register(&aqr106_driver);
 	phy_register(&aqr107_driver);
+	phy_register(&aqr112_driver);
 	phy_register(&aqr405_driver);
+	phy_register(&aqr412_driver);
 
 	return 0;
 }
-- 
2.17.1

[U-Boot] [PATCH v2] drivers: net: phy: Use Aquantia driver for AQR112, AQR412

[Ramon Fried]

On Wed, Jun 19, 2019 at 3:26 PM Alexandru Marginean
<alexandru.marginean@nxp.com> wrote:
>
> adds AQR112 and AQR412 to the list of supported PHYs using existing AQR
> code.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>
> Changes in v2:
>         - Numerical ordering of structure definition and function calls
>
>  drivers/net/phy/aquantia.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
> index 5c3298d612..465ec2d342 100644
> --- a/drivers/net/phy/aquantia.c
> +++ b/drivers/net/phy/aquantia.c
> @@ -461,6 +461,19 @@ struct phy_driver aqr107_driver =
>         .shutdown =en10g_shutdown,
>  };
>
> +struct phy_driver aqr112_driver =
> +       .name =quantia AQR112",
> +       .uid =3a1b660,
> +       .mask =fffffff0,
> +       .features =Y_10G_FEATURES,
> +       .mmds =DIO_MMD_PMAPMD | MDIO_MMD_PCS |
> +                MDIO_MMD_PHYXS | MDIO_MMD_AN |
> +                MDIO_MMD_VEND1),
> +       .config =quantia_config,
> +       .startup =quantia_startup,
> +       .shutdown =en10g_shutdown,
> +};
> +
>  struct phy_driver aqr405_driver =
>         .name =quantia AQR405",
>         .uid =3a1b4b2,
> @@ -474,6 +487,19 @@ struct phy_driver aqr405_driver =
>         .shutdown =en10g_shutdown,
>  };
>
> +struct phy_driver aqr412_driver =
> +       .name =quantia AQR412",
> +       .uid =3a1b710,
> +       .mask =fffffff0,
> +       .features =Y_10G_FEATURES,
> +       .mmds =DIO_MMD_PMAPMD | MDIO_MMD_PCS |
> +                MDIO_MMD_PHYXS | MDIO_MMD_AN |
> +                MDIO_MMD_VEND1),
> +       .config =quantia_config,
> +       .startup =quantia_startup,
> +       .shutdown =en10g_shutdown,
> +};
> +
>  int phy_aquantia_init(void)
>  {
>         phy_register(&aq1202_driver);
> @@ -481,7 +507,9 @@ int phy_aquantia_init(void)
>         phy_register(&aqr105_driver);
>         phy_register(&aqr106_driver);
>         phy_register(&aqr107_driver);
> +       phy_register(&aqr112_driver);
>         phy_register(&aqr405_driver);
> +       phy_register(&aqr412_driver);
>
>         return 0;
>  }
> --
> 2.17.1
>

Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Bin Meng]

Hi Alex,

On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
<alexandru.marginean@nxp.com> wrote:
>
> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> previously posted on the u-boot list).
> If the PHY ID reads all 0s just ignore it and try the next devad.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>  drivers/net/phy/phy.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c1c1af9abd..7ccbc4d9da 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>         while (phy_mask) {
>                 int addr = ffs(phy_mask) - 1;
>                 int r = get_phy_id(bus, addr, devad, &phy_id);
> +
> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs

nits: the multi-line comment block format is wrong

> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> +                * there are C22 PHYs that return all 0s for C45 reads (like
> +                * Atheros AR8035).
> +                */
> +               if (phy_id == 0)
> +                       return NULL;

Should this be "continue"?

> +
>                 /* If the PHY ID is mostly f's, we didn't find anything */
>                 if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>                         is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
> --

Regards,
Bin

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Joe Hershberger]

On Wed, Jun 19, 2019 at 4:07 AM Alexandru Marginean
<alexandru.marginean@nxp.com> wrote:
>
> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> previously posted on the u-boot list).
> If the PHY ID reads all 0s just ignore it and try the next devad.
>
> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> ---
>  drivers/net/phy/phy.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c1c1af9abd..7ccbc4d9da 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>         while (phy_mask) {
>                 int addr = ffs(phy_mask) - 1;
>                 int r = get_phy_id(bus, addr, devad, &phy_id);
> +
> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs

Agreed on the format.

> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> +                * there are C22 PHYs that return all 0s for C45 reads (like
> +                * Atheros AR8035).
> +                */
> +               if (phy_id == 0)
> +                       return NULL;

As for continue or return, this should probably check that phy_id == 0
and r == 0, right?

> +
>                 /* If the PHY ID is mostly f's, we didn't find anything */
>                 if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>                         is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
> --
> 2.17.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

On 7/1/2019 11:15 AM, Bin Meng wrote:
> Hi Alex,
> 
> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> <alexandru.marginean@nxp.com> wrote:
>>
>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>> previously posted on the u-boot list).
>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>
>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>> ---
>>   drivers/net/phy/phy.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index c1c1af9abd..7ccbc4d9da 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>          while (phy_mask) {
>>                  int addr = ffs(phy_mask) - 1;
>>                  int r = get_phy_id(bus, addr, devad, &phy_id);
>> +
>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> 
> nits: the multi-line comment block format is wrong

You're right, I'll fix that.

> 
>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>> +                * Atheros AR8035).
>> +                */
>> +               if (phy_id == 0)
>> +                       return NULL;
> 
> Should this be "continue"?

In case there are C45 and C22 PHYs mixed on the same bus and they are
all flagged in phy_mask?  In general this function shouldn't end up
dealing with multiple PHYs, if it does then it's possible the result
won't the the right one.

If create_phy_by_mask is called from get_phy_device, then we're only
looking for one PHY and if that reads PHY_ID 0 we can just assume we're
not using the correct devad.

create_phy_by_mask can also be called from phy_connect (or some other
place) with phy_mask = 0xffffffff.  The assumption in that case is that
there is one PHY on the given MDIO bus.
If there are several PHYs then we're going into a gray area, the result
isn't explicitly defined.  We could try to probe the PHY with the
smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
0 for at least one devad used.  Or we could keep the current devad and
continue scanning for the next addr, continue instead of return would
achieve that.  I don't really have a strong preference for either, the
message for developers should be to avoid ending up in this situation
altogether.

What do you think?

Thank you!
Alex

> 
>> +
>>                  /* If the PHY ID is mostly f's, we didn't find anything */
>>                  if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>>                          is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
>> --
> 
> Regards,
> Bin
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

Hi Joe,

On 7/2/2019 12:17 AM, Joe Hershberger wrote:
> On Wed, Jun 19, 2019 at 4:07 AM Alexandru Marginean
> <alexandru.marginean@nxp.com> wrote:
>>
>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>> previously posted on the u-boot list).
>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>
>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>> ---
>>   drivers/net/phy/phy.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index c1c1af9abd..7ccbc4d9da 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>          while (phy_mask) {
>>                  int addr = ffs(phy_mask) - 1;
>>                  int r = get_phy_id(bus, addr, devad, &phy_id);
>> +
>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> 
> Agreed on the format.
> 
>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>> +                * Atheros AR8035).
>> +                */
>> +               if (phy_id == 0)
>> +                       return NULL;
> 
> As for continue or return, this should probably check that phy_id == 0
> and r == 0, right?

Right, in case the read over MDIO returns an error code for the missing
PHY we want to continue the loop.  I'll fix this.

Thank you!
Alex

> 
>> +
>>                  /* If the PHY ID is mostly f's, we didn't find anything */
>>                  if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>>                          is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Bin Meng]

Hi Alex,

On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>
> On 7/1/2019 11:15 AM, Bin Meng wrote:
> > Hi Alex,
> >
> > On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> > <alexandru.marginean@nxp.com> wrote:
> >>
> >> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> >> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> >> previously posted on the u-boot list).
> >> If the PHY ID reads all 0s just ignore it and try the next devad.
> >>
> >> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> >> ---
> >>   drivers/net/phy/phy.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >> index c1c1af9abd..7ccbc4d9da 100644
> >> --- a/drivers/net/phy/phy.c
> >> +++ b/drivers/net/phy/phy.c
> >> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
> >>          while (phy_mask) {
> >>                  int addr = ffs(phy_mask) - 1;
> >>                  int r = get_phy_id(bus, addr, devad, &phy_id);
> >> +
> >> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> >
> > nits: the multi-line comment block format is wrong
>
> You're right, I'll fix that.
>
> >
> >> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> >> +                * there are C22 PHYs that return all 0s for C45 reads (like
> >> +                * Atheros AR8035).
> >> +                */
> >> +               if (phy_id == 0)
> >> +                       return NULL;
> >
> > Should this be "continue"?
>
> In case there are C45 and C22 PHYs mixed on the same bus and they are
> all flagged in phy_mask?  In general this function shouldn't end up
> dealing with multiple PHYs, if it does then it's possible the result
> won't the the right one.
>
> If create_phy_by_mask is called from get_phy_device, then we're only
> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
> not using the correct devad.
>
> create_phy_by_mask can also be called from phy_connect (or some other
> place) with phy_mask = 0xffffffff.  The assumption in that case is that
> there is one PHY on the given MDIO bus.

Yes, this is the user scenario that concerns me. But on a shared MDIO
bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
boards did this way. For example, there are multiple eTSEC in the SoC,
and each eTSEC claims to have one MDIO controller, however Freescale
chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
(the first eTSEC).

> If there are several PHYs then we're going into a gray area, the result
> isn't explicitly defined.  We could try to probe the PHY with the

I suspect this function is not being used widely hence not exposing
any known issues?

> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
> 0 for at least one devad used.  Or we could keep the current devad and
> continue scanning for the next addr, continue instead of return would
> achieve that.  I don't really have a strong preference for either, the
> message for developers should be to avoid ending up in this situation
> altogether.
>
> What do you think?

Regards,
Bin

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

Hi Bin,

On 7/3/2019 10:39 AM, Bin Meng wrote:
> Hi Alex,
> 
> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>
>> On 7/1/2019 11:15 AM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
>>> <alexandru.marginean@nxp.com> wrote:
>>>>
>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>>>> previously posted on the u-boot list).
>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>>>
>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>>>> ---
>>>>    drivers/net/phy/phy.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>> index c1c1af9abd..7ccbc4d9da 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>>>           while (phy_mask) {
>>>>                   int addr = ffs(phy_mask) - 1;
>>>>                   int r = get_phy_id(bus, addr, devad, &phy_id);
>>>> +
>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
>>>
>>> nits: the multi-line comment block format is wrong
>>
>> You're right, I'll fix that.
>>
>>>
>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>>>> +                * Atheros AR8035).
>>>> +                */
>>>> +               if (phy_id == 0)
>>>> +                       return NULL;
>>>
>>> Should this be "continue"?
>>
>> In case there are C45 and C22 PHYs mixed on the same bus and they are
>> all flagged in phy_mask?  In general this function shouldn't end up
>> dealing with multiple PHYs, if it does then it's possible the result
>> won't the the right one.
>>
>> If create_phy_by_mask is called from get_phy_device, then we're only
>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
>> not using the correct devad.
>>
>> create_phy_by_mask can also be called from phy_connect (or some other
>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
>> there is one PHY on the given MDIO bus.
> 
> Yes, this is the user scenario that concerns me. But on a shared MDIO
> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
> boards did this way. For example, there are multiple eTSEC in the SoC,
> and each eTSEC claims to have one MDIO controller, however Freescale
> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
> (the first eTSEC).

Virtually all Freescale networking SoCs are like that, each MAC has its
own MDIO controller but those are in fact connected to the SoC internal
PHYs not to the external PHYs on the board.  These SoCs have one or two
MDIOs dedicated to the external PHYs.  If the SoC supports 10G
interfaces then the SoC typically has two external MDIOs, the intent
being that one is used for C22 and the other for C45 PHYs.
Of course MDIO buses with multiple PHYs have to work.  My point is that
one should not end up in this function with a phy_mask with multiple
bits set if the MDIO bus has multiple PHYs connected to it, in that
set-up the caller should know the PHY address up front and set only one
bit in phy_mask.

> 
>> If there are several PHYs then we're going into a gray area, the result
>> isn't explicitly defined.  We could try to probe the PHY with the
> 
> I suspect this function is not being used widely hence not exposing
> any known issues?

At least for qoriq and layerscape Freescale SoCs the PHY address is
known up front before calling this function, precisely because the MDIO
bus is shared, but I'm guessing other SoCs do rely on scanning to get
to the PHYs.

Joe, do you have a preference between return and continue when we hit
a PHY_ID == 0?  Are you OK with continue?

I wonder if it would be useful to scan all IDs in this function, instead
of returning on 1st hit, just to issue a warning if multiple addresses
flagged in phy_mask return valid PHY IDs.  Any thoughts?

Thank you!
Alex


>> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
>> 0 for at least one devad used.  Or we could keep the current devad and
>> continue scanning for the next addr, continue instead of return would
>> achieve that.  I don't really have a strong preference for either, the
>> message for developers should be to avoid ending up in this situation
>> altogether.
>>
>> What do you think?
> 
> Regards,
> Bin
> 

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Joe Hershberger]

On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>
> Hi Bin,
>
> On 7/3/2019 10:39 AM, Bin Meng wrote:
> > Hi Alex,
> >
> > On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>
> >> On 7/1/2019 11:15 AM, Bin Meng wrote:
> >>> Hi Alex,
> >>>
> >>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> >>> <alexandru.marginean@nxp.com> wrote:
> >>>>
> >>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> >>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> >>>> previously posted on the u-boot list).
> >>>> If the PHY ID reads all 0s just ignore it and try the next devad.
> >>>>
> >>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> >>>> ---
> >>>>    drivers/net/phy/phy.c | 9 +++++++++
> >>>>    1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >>>> index c1c1af9abd..7ccbc4d9da 100644
> >>>> --- a/drivers/net/phy/phy.c
> >>>> +++ b/drivers/net/phy/phy.c
> >>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
> >>>>           while (phy_mask) {
> >>>>                   int addr = ffs(phy_mask) - 1;
> >>>>                   int r = get_phy_id(bus, addr, devad, &phy_id);
> >>>> +
> >>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> >>>
> >>> nits: the multi-line comment block format is wrong
> >>
> >> You're right, I'll fix that.
> >>
> >>>
> >>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> >>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
> >>>> +                * Atheros AR8035).
> >>>> +                */
> >>>> +               if (phy_id == 0)
> >>>> +                       return NULL;
> >>>
> >>> Should this be "continue"?
> >>
> >> In case there are C45 and C22 PHYs mixed on the same bus and they are
> >> all flagged in phy_mask?  In general this function shouldn't end up
> >> dealing with multiple PHYs, if it does then it's possible the result
> >> won't the the right one.
> >>
> >> If create_phy_by_mask is called from get_phy_device, then we're only
> >> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
> >> not using the correct devad.
> >>
> >> create_phy_by_mask can also be called from phy_connect (or some other
> >> place) with phy_mask = 0xffffffff.  The assumption in that case is that
> >> there is one PHY on the given MDIO bus.
> >
> > Yes, this is the user scenario that concerns me. But on a shared MDIO
> > bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
> > boards did this way. For example, there are multiple eTSEC in the SoC,
> > and each eTSEC claims to have one MDIO controller, however Freescale
> > chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
> > (the first eTSEC).
>
> Virtually all Freescale networking SoCs are like that, each MAC has its
> own MDIO controller but those are in fact connected to the SoC internal
> PHYs not to the external PHYs on the board.  These SoCs have one or two
> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
> interfaces then the SoC typically has two external MDIOs, the intent
> being that one is used for C22 and the other for C45 PHYs.
> Of course MDIO buses with multiple PHYs have to work.  My point is that
> one should not end up in this function with a phy_mask with multiple
> bits set if the MDIO bus has multiple PHYs connected to it, in that
> set-up the caller should know the PHY address up front and set only one
> bit in phy_mask.
>
> >
> >> If there are several PHYs then we're going into a gray area, the result
> >> isn't explicitly defined.  We could try to probe the PHY with the
> >
> > I suspect this function is not being used widely hence not exposing
> > any known issues?
>
> At least for qoriq and layerscape Freescale SoCs the PHY address is
> known up front before calling this function, precisely because the MDIO
> bus is shared, but I'm guessing other SoCs do rely on scanning to get
> to the PHYs.
>
> Joe, do you have a preference between return and continue when we hit
> a PHY_ID == 0?  Are you OK with continue?
>
> I wonder if it would be useful to scan all IDs in this function, instead
> of returning on 1st hit, just to issue a warning if multiple addresses
> flagged in phy_mask return valid PHY IDs.  Any thoughts?

Scanning all seems like a bit of an independent question. I think the
return vs continue decision at a higher level is a decision about
whether a phy scan should include or ignore C45 phys, correct? Scan
all seems like it applies to all included phys, C45 being included or
not based on the former decision.

Is there a use case anyone is aware of where C45 phys are expected to
be scanned for?

> Thank you!
> Alex
>
>
> >> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
> >> 0 for at least one devad used.  Or we could keep the current devad and
> >> continue scanning for the next addr, continue instead of return would
> >> achieve that.  I don't really have a strong preference for either, the
> >> message for developers should be to avoid ending up in this situation
> >> altogether.
> >>
> >> What do you think?
> >
> > Regards,
> > Bin
> >
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

Hi Joe,

On 7/8/2019 8:08 PM, Joe Hershberger wrote:
> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>
>> Hi Bin,
>>
>> On 7/3/2019 10:39 AM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>
>>>> On 7/1/2019 11:15 AM, Bin Meng wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
>>>>> <alexandru.marginean@nxp.com> wrote:
>>>>>>
>>>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>>>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>>>>>> previously posted on the u-boot list).
>>>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>>>>>
>>>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>>>>>> ---
>>>>>>     drivers/net/phy/phy.c | 9 +++++++++
>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>>>> index c1c1af9abd..7ccbc4d9da 100644
>>>>>> --- a/drivers/net/phy/phy.c
>>>>>> +++ b/drivers/net/phy/phy.c
>>>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>>>>>            while (phy_mask) {
>>>>>>                    int addr = ffs(phy_mask) - 1;
>>>>>>                    int r = get_phy_id(bus, addr, devad, &phy_id);
>>>>>> +
>>>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
>>>>>
>>>>> nits: the multi-line comment block format is wrong
>>>>
>>>> You're right, I'll fix that.
>>>>
>>>>>
>>>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>>>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>>>>>> +                * Atheros AR8035).
>>>>>> +                */
>>>>>> +               if (phy_id == 0)
>>>>>> +                       return NULL;
>>>>>
>>>>> Should this be "continue"?
>>>>
>>>> In case there are C45 and C22 PHYs mixed on the same bus and they are
>>>> all flagged in phy_mask?  In general this function shouldn't end up
>>>> dealing with multiple PHYs, if it does then it's possible the result
>>>> won't the the right one.
>>>>
>>>> If create_phy_by_mask is called from get_phy_device, then we're only
>>>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
>>>> not using the correct devad.
>>>>
>>>> create_phy_by_mask can also be called from phy_connect (or some other
>>>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
>>>> there is one PHY on the given MDIO bus.
>>>
>>> Yes, this is the user scenario that concerns me. But on a shared MDIO
>>> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
>>> boards did this way. For example, there are multiple eTSEC in the SoC,
>>> and each eTSEC claims to have one MDIO controller, however Freescale
>>> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
>>> (the first eTSEC).
>>
>> Virtually all Freescale networking SoCs are like that, each MAC has its
>> own MDIO controller but those are in fact connected to the SoC internal
>> PHYs not to the external PHYs on the board.  These SoCs have one or two
>> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
>> interfaces then the SoC typically has two external MDIOs, the intent
>> being that one is used for C22 and the other for C45 PHYs.
>> Of course MDIO buses with multiple PHYs have to work.  My point is that
>> one should not end up in this function with a phy_mask with multiple
>> bits set if the MDIO bus has multiple PHYs connected to it, in that
>> set-up the caller should know the PHY address up front and set only one
>> bit in phy_mask.
>>
>>>
>>>> If there are several PHYs then we're going into a gray area, the result
>>>> isn't explicitly defined.  We could try to probe the PHY with the
>>>
>>> I suspect this function is not being used widely hence not exposing
>>> any known issues?
>>
>> At least for qoriq and layerscape Freescale SoCs the PHY address is
>> known up front before calling this function, precisely because the MDIO
>> bus is shared, but I'm guessing other SoCs do rely on scanning to get
>> to the PHYs.
>>
>> Joe, do you have a preference between return and continue when we hit
>> a PHY_ID == 0?  Are you OK with continue?
>>
>> I wonder if it would be useful to scan all IDs in this function, instead
>> of returning on 1st hit, just to issue a warning if multiple addresses
>> flagged in phy_mask return valid PHY IDs.  Any thoughts?
> 
> Scanning all seems like a bit of an independent question. I think the
> return vs continue decision at a higher level is a decision about
> whether a phy scan should include or ignore C45 phys, correct? Scan
> all seems like it applies to all included phys, C45 being included or
> not based on the former decision.
> 
> Is there a use case anyone is aware of where C45 phys are expected to
> be scanned for?

Assuming there's just one PHY on the bus that's scanned, and it's a C45
PHY, scanning should actually work (that case works with both continue
and return NULL in fact).

create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
and not find anything else using C22 reads, finally return NULL.
get_phy_device_by_mask will call again with the next devad.  Assuming
the PHY replies to devad 1 or one of the other ones used in
get_phy_device_by_mask, the PHY will be probed as a C45 PHY.

Things aren't as good if there is a bus with multiple PHYs, especially
if they are mixed, C22 with C45.  In that case relying on scanning looks
like a bad idea though.

Alex

>>
>>
>>>> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
>>>> 0 for at least one devad used.  Or we could keep the current devad and
>>>> continue scanning for the next addr, continue instead of return would
>>>> achieve that.  I don't really have a strong preference for either, the
>>>> message for developers should be to avoid ending up in this situation
>>>> altogether.
>>>>
>>>> What do you think?
>>>
>>> Regards,
>>> Bin
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Joe Hershberger]

On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>
> Hi Joe,
>
> On 7/8/2019 8:08 PM, Joe Hershberger wrote:
> > On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>
> >> Hi Bin,
> >>
> >> On 7/3/2019 10:39 AM, Bin Meng wrote:
> >>> Hi Alex,
> >>>
> >>> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>>>
> >>>> On 7/1/2019 11:15 AM, Bin Meng wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> >>>>> <alexandru.marginean@nxp.com> wrote:
> >>>>>>
> >>>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> >>>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> >>>>>> previously posted on the u-boot list).
> >>>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
> >>>>>>
> >>>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> >>>>>> ---
> >>>>>>     drivers/net/phy/phy.c | 9 +++++++++
> >>>>>>     1 file changed, 9 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >>>>>> index c1c1af9abd..7ccbc4d9da 100644
> >>>>>> --- a/drivers/net/phy/phy.c
> >>>>>> +++ b/drivers/net/phy/phy.c
> >>>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
> >>>>>>            while (phy_mask) {
> >>>>>>                    int addr = ffs(phy_mask) - 1;
> >>>>>>                    int r = get_phy_id(bus, addr, devad, &phy_id);
> >>>>>> +
> >>>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> >>>>>
> >>>>> nits: the multi-line comment block format is wrong
> >>>>
> >>>> You're right, I'll fix that.
> >>>>
> >>>>>
> >>>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> >>>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
> >>>>>> +                * Atheros AR8035).
> >>>>>> +                */
> >>>>>> +               if (phy_id == 0)
> >>>>>> +                       return NULL;
> >>>>>
> >>>>> Should this be "continue"?
> >>>>
> >>>> In case there are C45 and C22 PHYs mixed on the same bus and they are
> >>>> all flagged in phy_mask?  In general this function shouldn't end up
> >>>> dealing with multiple PHYs, if it does then it's possible the result
> >>>> won't the the right one.
> >>>>
> >>>> If create_phy_by_mask is called from get_phy_device, then we're only
> >>>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
> >>>> not using the correct devad.
> >>>>
> >>>> create_phy_by_mask can also be called from phy_connect (or some other
> >>>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
> >>>> there is one PHY on the given MDIO bus.
> >>>
> >>> Yes, this is the user scenario that concerns me. But on a shared MDIO
> >>> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
> >>> boards did this way. For example, there are multiple eTSEC in the SoC,
> >>> and each eTSEC claims to have one MDIO controller, however Freescale
> >>> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
> >>> (the first eTSEC).
> >>
> >> Virtually all Freescale networking SoCs are like that, each MAC has its
> >> own MDIO controller but those are in fact connected to the SoC internal
> >> PHYs not to the external PHYs on the board.  These SoCs have one or two
> >> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
> >> interfaces then the SoC typically has two external MDIOs, the intent
> >> being that one is used for C22 and the other for C45 PHYs.
> >> Of course MDIO buses with multiple PHYs have to work.  My point is that
> >> one should not end up in this function with a phy_mask with multiple
> >> bits set if the MDIO bus has multiple PHYs connected to it, in that
> >> set-up the caller should know the PHY address up front and set only one
> >> bit in phy_mask.
> >>
> >>>
> >>>> If there are several PHYs then we're going into a gray area, the result
> >>>> isn't explicitly defined.  We could try to probe the PHY with the
> >>>
> >>> I suspect this function is not being used widely hence not exposing
> >>> any known issues?
> >>
> >> At least for qoriq and layerscape Freescale SoCs the PHY address is
> >> known up front before calling this function, precisely because the MDIO
> >> bus is shared, but I'm guessing other SoCs do rely on scanning to get
> >> to the PHYs.
> >>
> >> Joe, do you have a preference between return and continue when we hit
> >> a PHY_ID == 0?  Are you OK with continue?
> >>
> >> I wonder if it would be useful to scan all IDs in this function, instead
> >> of returning on 1st hit, just to issue a warning if multiple addresses
> >> flagged in phy_mask return valid PHY IDs.  Any thoughts?
> >
> > Scanning all seems like a bit of an independent question. I think the
> > return vs continue decision at a higher level is a decision about
> > whether a phy scan should include or ignore C45 phys, correct? Scan
> > all seems like it applies to all included phys, C45 being included or
> > not based on the former decision.
> >
> > Is there a use case anyone is aware of where C45 phys are expected to
> > be scanned for?
>
> Assuming there's just one PHY on the bus that's scanned, and it's a C45
> PHY, scanning should actually work (that case works with both continue
> and return NULL in fact).
>
> create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
> and not find anything else using C22 reads, finally return NULL.
> get_phy_device_by_mask will call again with the next devad.  Assuming
> the PHY replies to devad 1 or one of the other ones used in
> get_phy_device_by_mask, the PHY will be probed as a C45 PHY.
>
> Things aren't as good if there is a bus with multiple PHYs, especially
> if they are mixed, C22 with C45.  In that case relying on scanning looks
> like a bad idea though.

I agree... if you have more than one phy, on the same MDIO, you had
better have them mapped to MACs explicitly. I'm more curious if there
are use-cases where you would want to ignore a C45 phy. What kind of
performance hit would we take for continuing to scan, just so that we
can warn that the DT should list the phy addrs?

>
> Alex
>
> >>
> >>
> >>>> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
> >>>> 0 for at least one devad used.  Or we could keep the current devad and
> >>>> continue scanning for the next addr, continue instead of return would
> >>>> achieve that.  I don't really have a strong preference for either, the
> >>>> message for developers should be to avoid ending up in this situation
> >>>> altogether.
> >>>>
> >>>> What do you think?
> >>>
> >>> Regards,
> >>> Bin
> >>>
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> https://lists.denx.de/listinfo/u-boot
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

On 7/9/2019 4:25 PM, Joe Hershberger wrote:
> On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>
>> Hi Joe,
>>
>> On 7/8/2019 8:08 PM, Joe Hershberger wrote:
>>> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>> On 7/3/2019 10:39 AM, Bin Meng wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>>>
>>>>>> On 7/1/2019 11:15 AM, Bin Meng wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
>>>>>>> <alexandru.marginean@nxp.com> wrote:
>>>>>>>>
>>>>>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>>>>>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>>>>>>>> previously posted on the u-boot list).
>>>>>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>>>>>>>
>>>>>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>>>>>>>> ---
>>>>>>>>      drivers/net/phy/phy.c | 9 +++++++++
>>>>>>>>      1 file changed, 9 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>>>>>> index c1c1af9abd..7ccbc4d9da 100644
>>>>>>>> --- a/drivers/net/phy/phy.c
>>>>>>>> +++ b/drivers/net/phy/phy.c
>>>>>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>>>>>>>             while (phy_mask) {
>>>>>>>>                     int addr = ffs(phy_mask) - 1;
>>>>>>>>                     int r = get_phy_id(bus, addr, devad, &phy_id);
>>>>>>>> +
>>>>>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
>>>>>>>
>>>>>>> nits: the multi-line comment block format is wrong
>>>>>>
>>>>>> You're right, I'll fix that.
>>>>>>
>>>>>>>
>>>>>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>>>>>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>>>>>>>> +                * Atheros AR8035).
>>>>>>>> +                */
>>>>>>>> +               if (phy_id == 0)
>>>>>>>> +                       return NULL;
>>>>>>>
>>>>>>> Should this be "continue"?
>>>>>>
>>>>>> In case there are C45 and C22 PHYs mixed on the same bus and they are
>>>>>> all flagged in phy_mask?  In general this function shouldn't end up
>>>>>> dealing with multiple PHYs, if it does then it's possible the result
>>>>>> won't the the right one.
>>>>>>
>>>>>> If create_phy_by_mask is called from get_phy_device, then we're only
>>>>>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
>>>>>> not using the correct devad.
>>>>>>
>>>>>> create_phy_by_mask can also be called from phy_connect (or some other
>>>>>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
>>>>>> there is one PHY on the given MDIO bus.
>>>>>
>>>>> Yes, this is the user scenario that concerns me. But on a shared MDIO
>>>>> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
>>>>> boards did this way. For example, there are multiple eTSEC in the SoC,
>>>>> and each eTSEC claims to have one MDIO controller, however Freescale
>>>>> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
>>>>> (the first eTSEC).
>>>>
>>>> Virtually all Freescale networking SoCs are like that, each MAC has its
>>>> own MDIO controller but those are in fact connected to the SoC internal
>>>> PHYs not to the external PHYs on the board.  These SoCs have one or two
>>>> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
>>>> interfaces then the SoC typically has two external MDIOs, the intent
>>>> being that one is used for C22 and the other for C45 PHYs.
>>>> Of course MDIO buses with multiple PHYs have to work.  My point is that
>>>> one should not end up in this function with a phy_mask with multiple
>>>> bits set if the MDIO bus has multiple PHYs connected to it, in that
>>>> set-up the caller should know the PHY address up front and set only one
>>>> bit in phy_mask.
>>>>
>>>>>
>>>>>> If there are several PHYs then we're going into a gray area, the result
>>>>>> isn't explicitly defined.  We could try to probe the PHY with the
>>>>>
>>>>> I suspect this function is not being used widely hence not exposing
>>>>> any known issues?
>>>>
>>>> At least for qoriq and layerscape Freescale SoCs the PHY address is
>>>> known up front before calling this function, precisely because the MDIO
>>>> bus is shared, but I'm guessing other SoCs do rely on scanning to get
>>>> to the PHYs.
>>>>
>>>> Joe, do you have a preference between return and continue when we hit
>>>> a PHY_ID == 0?  Are you OK with continue?
>>>>
>>>> I wonder if it would be useful to scan all IDs in this function, instead
>>>> of returning on 1st hit, just to issue a warning if multiple addresses
>>>> flagged in phy_mask return valid PHY IDs.  Any thoughts?
>>>
>>> Scanning all seems like a bit of an independent question. I think the
>>> return vs continue decision at a higher level is a decision about
>>> whether a phy scan should include or ignore C45 phys, correct? Scan
>>> all seems like it applies to all included phys, C45 being included or
>>> not based on the former decision.
>>>
>>> Is there a use case anyone is aware of where C45 phys are expected to
>>> be scanned for?
>>
>> Assuming there's just one PHY on the bus that's scanned, and it's a C45
>> PHY, scanning should actually work (that case works with both continue
>> and return NULL in fact).
>>
>> create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
>> and not find anything else using C22 reads, finally return NULL.
>> get_phy_device_by_mask will call again with the next devad.  Assuming
>> the PHY replies to devad 1 or one of the other ones used in
>> get_phy_device_by_mask, the PHY will be probed as a C45 PHY.
>>
>> Things aren't as good if there is a bus with multiple PHYs, especially
>> if they are mixed, C22 with C45.  In that case relying on scanning looks
>> like a bad idea though.
> 
> I agree... if you have more than one phy, on the same MDIO, you had
> better have them mapped to MACs explicitly. I'm more curious if there
> are use-cases where you would want to ignore a C45 phy. What kind of
> performance hit would we take for continuing to scan, just so that we
> can warn that the DT should list the phy addrs?

Top of my head, reading all 32 addresses could go up to a few
milliseconds.  This is really only useful for development set-ups
really, for devices in the field it doesn't feel right to scan the
entire bus if it happens that the only PHY on that bus is at a low addr.
Not to mention that no one would see a warning anyway.
So maybe include this as a debug option, or rather just document that
phy_find_by_mask should only be called if there is a single PHY on the
bus, otherwise the result is not guaranteed.


> 
>>
>> Alex
>>
>>>>
>>>>
>>>>>> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
>>>>>> 0 for at least one devad used.  Or we could keep the current devad and
>>>>>> continue scanning for the next addr, continue instead of return would
>>>>>> achieve that.  I don't really have a strong preference for either, the
>>>>>> message for developers should be to avoid ending up in this situation
>>>>>> altogether.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Regards,
>>>>> Bin
>>>>>
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> https://lists.denx.de/listinfo/u-boot
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Joe Hershberger]

On Tue, Jul 9, 2019 at 10:17 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>
> On 7/9/2019 4:25 PM, Joe Hershberger wrote:
> > On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>
> >> Hi Joe,
> >>
> >> On 7/8/2019 8:08 PM, Joe Hershberger wrote:
> >>> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>>>
> >>>> Hi Bin,
> >>>>
> >>>> On 7/3/2019 10:39 AM, Bin Meng wrote:
> >>>>> Hi Alex,
> >>>>>
> >>>>> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
> >>>>>>
> >>>>>> On 7/1/2019 11:15 AM, Bin Meng wrote:
> >>>>>>> Hi Alex,
> >>>>>>>
> >>>>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> >>>>>>> <alexandru.marginean@nxp.com> wrote:
> >>>>>>>>
> >>>>>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> >>>>>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> >>>>>>>> previously posted on the u-boot list).
> >>>>>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/net/phy/phy.c | 9 +++++++++
> >>>>>>>>      1 file changed, 9 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >>>>>>>> index c1c1af9abd..7ccbc4d9da 100644
> >>>>>>>> --- a/drivers/net/phy/phy.c
> >>>>>>>> +++ b/drivers/net/phy/phy.c
> >>>>>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
> >>>>>>>>             while (phy_mask) {
> >>>>>>>>                     int addr = ffs(phy_mask) - 1;
> >>>>>>>>                     int r = get_phy_id(bus, addr, devad, &phy_id);
> >>>>>>>> +
> >>>>>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> >>>>>>>
> >>>>>>> nits: the multi-line comment block format is wrong
> >>>>>>
> >>>>>> You're right, I'll fix that.
> >>>>>>
> >>>>>>>
> >>>>>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> >>>>>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
> >>>>>>>> +                * Atheros AR8035).
> >>>>>>>> +                */
> >>>>>>>> +               if (phy_id == 0)
> >>>>>>>> +                       return NULL;
> >>>>>>>
> >>>>>>> Should this be "continue"?
> >>>>>>
> >>>>>> In case there are C45 and C22 PHYs mixed on the same bus and they are
> >>>>>> all flagged in phy_mask?  In general this function shouldn't end up
> >>>>>> dealing with multiple PHYs, if it does then it's possible the result
> >>>>>> won't the the right one.
> >>>>>>
> >>>>>> If create_phy_by_mask is called from get_phy_device, then we're only
> >>>>>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
> >>>>>> not using the correct devad.
> >>>>>>
> >>>>>> create_phy_by_mask can also be called from phy_connect (or some other
> >>>>>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
> >>>>>> there is one PHY on the given MDIO bus.
> >>>>>
> >>>>> Yes, this is the user scenario that concerns me. But on a shared MDIO
> >>>>> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
> >>>>> boards did this way. For example, there are multiple eTSEC in the SoC,
> >>>>> and each eTSEC claims to have one MDIO controller, however Freescale
> >>>>> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
> >>>>> (the first eTSEC).
> >>>>
> >>>> Virtually all Freescale networking SoCs are like that, each MAC has its
> >>>> own MDIO controller but those are in fact connected to the SoC internal
> >>>> PHYs not to the external PHYs on the board.  These SoCs have one or two
> >>>> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
> >>>> interfaces then the SoC typically has two external MDIOs, the intent
> >>>> being that one is used for C22 and the other for C45 PHYs.
> >>>> Of course MDIO buses with multiple PHYs have to work.  My point is that
> >>>> one should not end up in this function with a phy_mask with multiple
> >>>> bits set if the MDIO bus has multiple PHYs connected to it, in that
> >>>> set-up the caller should know the PHY address up front and set only one
> >>>> bit in phy_mask.
> >>>>
> >>>>>
> >>>>>> If there are several PHYs then we're going into a gray area, the result
> >>>>>> isn't explicitly defined.  We could try to probe the PHY with the
> >>>>>
> >>>>> I suspect this function is not being used widely hence not exposing
> >>>>> any known issues?
> >>>>
> >>>> At least for qoriq and layerscape Freescale SoCs the PHY address is
> >>>> known up front before calling this function, precisely because the MDIO
> >>>> bus is shared, but I'm guessing other SoCs do rely on scanning to get
> >>>> to the PHYs.
> >>>>
> >>>> Joe, do you have a preference between return and continue when we hit
> >>>> a PHY_ID == 0?  Are you OK with continue?
> >>>>
> >>>> I wonder if it would be useful to scan all IDs in this function, instead
> >>>> of returning on 1st hit, just to issue a warning if multiple addresses
> >>>> flagged in phy_mask return valid PHY IDs.  Any thoughts?
> >>>
> >>> Scanning all seems like a bit of an independent question. I think the
> >>> return vs continue decision at a higher level is a decision about
> >>> whether a phy scan should include or ignore C45 phys, correct? Scan
> >>> all seems like it applies to all included phys, C45 being included or
> >>> not based on the former decision.
> >>>
> >>> Is there a use case anyone is aware of where C45 phys are expected to
> >>> be scanned for?
> >>
> >> Assuming there's just one PHY on the bus that's scanned, and it's a C45
> >> PHY, scanning should actually work (that case works with both continue
> >> and return NULL in fact).
> >>
> >> create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
> >> and not find anything else using C22 reads, finally return NULL.
> >> get_phy_device_by_mask will call again with the next devad.  Assuming
> >> the PHY replies to devad 1 or one of the other ones used in
> >> get_phy_device_by_mask, the PHY will be probed as a C45 PHY.
> >>
> >> Things aren't as good if there is a bus with multiple PHYs, especially
> >> if they are mixed, C22 with C45.  In that case relying on scanning looks
> >> like a bad idea though.
> >
> > I agree... if you have more than one phy, on the same MDIO, you had
> > better have them mapped to MACs explicitly. I'm more curious if there
> > are use-cases where you would want to ignore a C45 phy. What kind of
> > performance hit would we take for continuing to scan, just so that we
> > can warn that the DT should list the phy addrs?
>
> Top of my head, reading all 32 addresses could go up to a few
> milliseconds.  This is really only useful for development set-ups
> really, for devices in the field it doesn't feel right to scan the
> entire bus if it happens that the only PHY on that bus is at a low addr.
> Not to mention that no one would see a warning anyway.
> So maybe include this as a debug option, or rather just document that
> phy_find_by_mask should only be called if there is a single PHY on the
> bus, otherwise the result is not guaranteed.

I agree that it doesn't make sense in production. I'm not sure there's
enough value in dev to add this feature. Adding a little documentation
is probably about the right level of work.

> >
> >>
> >> Alex
> >>
> >>>>
> >>>>
> >>>>>> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
> >>>>>> 0 for at least one devad used.  Or we could keep the current devad and
> >>>>>> continue scanning for the next addr, continue instead of return would
> >>>>>> achieve that.  I don't really have a strong preference for either, the
> >>>>>> message for developers should be to avoid ending up in this situation
> >>>>>> altogether.
> >>>>>>
> >>>>>> What do you think?
> >>>>>
> >>>>> Regards,
> >>>>> Bin
> >>>>>
> >>>>
> >>>> _______________________________________________
> >>>> U-Boot mailing list
> >>>> U-Boot at lists.denx.de
> >>>> https://lists.denx.de/listinfo/u-boot
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> https://lists.denx.de/listinfo/u-boot
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Alex Marginean]

On 7/9/2019 7:15 PM, Joe Hershberger wrote:
> On Tue, Jul 9, 2019 at 10:17 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>
>> On 7/9/2019 4:25 PM, Joe Hershberger wrote:
>>> On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>
>>>> Hi Joe,
>>>>
>>>> On 7/8/2019 8:08 PM, Joe Hershberger wrote:
>>>>> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>>>
>>>>>> Hi Bin,
>>>>>>
>>>>>> On 7/3/2019 10:39 AM, Bin Meng wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 7/1/2019 11:15 AM, Bin Meng wrote:
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
>>>>>>>>> <alexandru.marginean@nxp.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Current code fails to probe some C45 PHYs that also respond to C22 reads.
>>>>>>>>>> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
>>>>>>>>>> previously posted on the u-boot list).
>>>>>>>>>> If the PHY ID reads all 0s just ignore it and try the next devad.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alex Marginean <alexm.osslist@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/net/phy/phy.c | 9 +++++++++
>>>>>>>>>>       1 file changed, 9 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>>>>>>>> index c1c1af9abd..7ccbc4d9da 100644
>>>>>>>>>> --- a/drivers/net/phy/phy.c
>>>>>>>>>> +++ b/drivers/net/phy/phy.c
>>>>>>>>>> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
>>>>>>>>>>              while (phy_mask) {
>>>>>>>>>>                      int addr = ffs(phy_mask) - 1;
>>>>>>>>>>                      int r = get_phy_id(bus, addr, devad, &phy_id);
>>>>>>>>>> +
>>>>>>>>>> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
>>>>>>>>>
>>>>>>>>> nits: the multi-line comment block format is wrong
>>>>>>>>
>>>>>>>> You're right, I'll fix that.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
>>>>>>>>>> +                * there are C22 PHYs that return all 0s for C45 reads (like
>>>>>>>>>> +                * Atheros AR8035).
>>>>>>>>>> +                */
>>>>>>>>>> +               if (phy_id == 0)
>>>>>>>>>> +                       return NULL;
>>>>>>>>>
>>>>>>>>> Should this be "continue"?
>>>>>>>>
>>>>>>>> In case there are C45 and C22 PHYs mixed on the same bus and they are
>>>>>>>> all flagged in phy_mask?  In general this function shouldn't end up
>>>>>>>> dealing with multiple PHYs, if it does then it's possible the result
>>>>>>>> won't the the right one.
>>>>>>>>
>>>>>>>> If create_phy_by_mask is called from get_phy_device, then we're only
>>>>>>>> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
>>>>>>>> not using the correct devad.
>>>>>>>>
>>>>>>>> create_phy_by_mask can also be called from phy_connect (or some other
>>>>>>>> place) with phy_mask = 0xffffffff.  The assumption in that case is that
>>>>>>>> there is one PHY on the given MDIO bus.
>>>>>>>
>>>>>>> Yes, this is the user scenario that concerns me. But on a shared MDIO
>>>>>>> bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
>>>>>>> boards did this way. For example, there are multiple eTSEC in the SoC,
>>>>>>> and each eTSEC claims to have one MDIO controller, however Freescale
>>>>>>> chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
>>>>>>> (the first eTSEC).
>>>>>>
>>>>>> Virtually all Freescale networking SoCs are like that, each MAC has its
>>>>>> own MDIO controller but those are in fact connected to the SoC internal
>>>>>> PHYs not to the external PHYs on the board.  These SoCs have one or two
>>>>>> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
>>>>>> interfaces then the SoC typically has two external MDIOs, the intent
>>>>>> being that one is used for C22 and the other for C45 PHYs.
>>>>>> Of course MDIO buses with multiple PHYs have to work.  My point is that
>>>>>> one should not end up in this function with a phy_mask with multiple
>>>>>> bits set if the MDIO bus has multiple PHYs connected to it, in that
>>>>>> set-up the caller should know the PHY address up front and set only one
>>>>>> bit in phy_mask.
>>>>>>
>>>>>>>
>>>>>>>> If there are several PHYs then we're going into a gray area, the result
>>>>>>>> isn't explicitly defined.  We could try to probe the PHY with the
>>>>>>>
>>>>>>> I suspect this function is not being used widely hence not exposing
>>>>>>> any known issues?
>>>>>>
>>>>>> At least for qoriq and layerscape Freescale SoCs the PHY address is
>>>>>> known up front before calling this function, precisely because the MDIO
>>>>>> bus is shared, but I'm guessing other SoCs do rely on scanning to get
>>>>>> to the PHYs.
>>>>>>
>>>>>> Joe, do you have a preference between return and continue when we hit
>>>>>> a PHY_ID == 0?  Are you OK with continue?
>>>>>>
>>>>>> I wonder if it would be useful to scan all IDs in this function, instead
>>>>>> of returning on 1st hit, just to issue a warning if multiple addresses
>>>>>> flagged in phy_mask return valid PHY IDs.  Any thoughts?
>>>>>
>>>>> Scanning all seems like a bit of an independent question. I think the
>>>>> return vs continue decision at a higher level is a decision about
>>>>> whether a phy scan should include or ignore C45 phys, correct? Scan
>>>>> all seems like it applies to all included phys, C45 being included or
>>>>> not based on the former decision.
>>>>>
>>>>> Is there a use case anyone is aware of where C45 phys are expected to
>>>>> be scanned for?
>>>>
>>>> Assuming there's just one PHY on the bus that's scanned, and it's a C45
>>>> PHY, scanning should actually work (that case works with both continue
>>>> and return NULL in fact).
>>>>
>>>> create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
>>>> and not find anything else using C22 reads, finally return NULL.
>>>> get_phy_device_by_mask will call again with the next devad.  Assuming
>>>> the PHY replies to devad 1 or one of the other ones used in
>>>> get_phy_device_by_mask, the PHY will be probed as a C45 PHY.
>>>>
>>>> Things aren't as good if there is a bus with multiple PHYs, especially
>>>> if they are mixed, C22 with C45.  In that case relying on scanning looks
>>>> like a bad idea though.
>>>
>>> I agree... if you have more than one phy, on the same MDIO, you had
>>> better have them mapped to MACs explicitly. I'm more curious if there
>>> are use-cases where you would want to ignore a C45 phy. What kind of
>>> performance hit would we take for continuing to scan, just so that we
>>> can warn that the DT should list the phy addrs?
>>
>> Top of my head, reading all 32 addresses could go up to a few
>> milliseconds.  This is really only useful for development set-ups
>> really, for devices in the field it doesn't feel right to scan the
>> entire bus if it happens that the only PHY on that bus is at a low addr.
>> Not to mention that no one would see a warning anyway.
>> So maybe include this as a debug option, or rather just document that
>> phy_find_by_mask should only be called if there is a single PHY on the
>> bus, otherwise the result is not guaranteed.
> 
> I agree that it doesn't make sense in production. I'm not sure there's
> enough value in dev to add this feature. Adding a little documentation
> is probably about the right level of work.

Sent as a separate patch with comments for PHY APIs.

Thank you!
Alex