[PATCH 0/3] dwc_eth_qos PHY dt node fixups

[PATCH 0/3] dwc_eth_qos PHY dt node fixups

[Rasmus Villemoes]

I need the dwc_eth_qos to gain a fix similar to what fec_mxc got in
89b5bd54c1a4.

The first patch provides a simplification around the logic for
fetching the phy's address, so there's one less case to worry about.

The second introduces a little helper (it doesn't essentially add any
extra code since it just returns an extra piece of inforamtion that
eth_phy_get_node() already found).

And the third hooks it up in the dwc_eth_qos driver.

Rasmus Villemoes (3):
  net: dwc_eth_qos: remove use of DWC_NET_PHYADDR
  phy: introduce eth_phy_get_node_and_addr()
  net: dwc_eth_qos: set proper DT node for phy device

 drivers/net/dwc_eth_qos.c        | 9 +++++----
 drivers/net/eth-phy-uclass.c     | 9 ++++++++-
 include/configs/imx8mp_evk.h     | 2 --
 include/configs/imx8mp_rsb3720.h | 1 -
 include/eth_phy.h                | 1 +
 5 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/3] net: dwc_eth_qos: remove use of DWC_NET_PHYADDR

[Rasmus Villemoes]

Only two boards in the tree set the macro DWC_NET_PHYADDR. Both have
CONFIG_DM_ETH_PHY=y, so should set the phy address in DT if necessary.

The imx8mp_evk does set the correct address in device tree.

The other board seems to be a copy-paste-adapt from an old
version of the imx8mp_evk config header, given the "#ifdef
CONFIG_DWC_ETH_QOS" block that has been removed from imx8mp_evk header
in commit 127fb454955. Its device tree doesn't even enable (i.e., set
'status = "okay"') the &eqos node. But the other ethernet device,
&fec, does get enabled, and does have a phy sitting at address 4 (and
it also has a corresponding legacy #define CONFIG_FEC_MXC_PHYADDR
4). So I believe it should be completely safe to remove it from there
as well.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/dwc_eth_qos.c        | 3 ---
 include/configs/imx8mp_evk.h     | 2 --
 include/configs/imx8mp_rsb3720.h | 1 -
 3 files changed, 6 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 9d255cf95f..27b3f98e0e 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1081,9 +1081,6 @@ static int eqos_start(struct udevice *dev)
 		int addr = -1;
 #ifdef CONFIG_DM_ETH_PHY
 		addr = eth_phy_get_addr(dev);
-#endif
-#ifdef DWC_NET_PHYADDR
-		addr = DWC_NET_PHYADDR;
 #endif
 		eqos->phy = phy_connect(eqos->mii, addr, dev,
 					eqos->config->interface(dev));
diff --git a/include/configs/imx8mp_evk.h b/include/configs/imx8mp_evk.h
index cc8d65cb54..f846c55d9c 100644
--- a/include/configs/imx8mp_evk.h
+++ b/include/configs/imx8mp_evk.h
@@ -34,8 +34,6 @@
 #define CONFIG_FEC_MXC_PHYADDR          1
 #define FEC_QUIRK_ENET_MAC
 
-#define DWC_NET_PHYADDR			1
-
 #define PHY_ANEG_TIMEOUT 20000
 
 #endif
diff --git a/include/configs/imx8mp_rsb3720.h b/include/configs/imx8mp_rsb3720.h
index c5dd545471..5608491b44 100644
--- a/include/configs/imx8mp_rsb3720.h
+++ b/include/configs/imx8mp_rsb3720.h
@@ -53,7 +53,6 @@
 #define CONFIG_FEC_MXC_PHYADDR          4
 #define FEC_QUIRK_ENET_MAC
 
-#define DWC_NET_PHYADDR			4
 #ifdef CONFIG_DWC_ETH_QOS
 #define CONFIG_SYS_NONCACHED_MEMORY     (1 * SZ_1M)     /* 1M */
 #endif
-- 
2.31.1

[PATCH 2/3] phy: introduce eth_phy_get_node_and_addr()

[Rasmus Villemoes]

Callers often don't just need to know the address of the phy, but also
need to know its DT node so that a subsequent call of phy_config() can
fetch various DT attributes specific to the given phy. Currently,
phy_get_ofnode() usually incorrectly returns a pointer to the node for
the ethernet controller.

This was fixed for fec_mxc in commit 89b5bd54c1a4, but I'm now hitting
the exact same problem with the dwc_eth_qos driver.

I have a ti,dp83867 phy which requires ti,rx-internal-delay and
ti,tx-internal-delay to be specified, which I do, but the phy driver
fails to fetch the values because of the wrong phy_get_ofnode() value.

So generalize eth_phy_get_addr() a bit so that it also optionally
returns the ofnode where it found the reg value.

Follow-up patches will make use of this in dwc_eth_qos, and further
generalize it even further so it can be used from fec_mxc as
well (i.e., add the fixed-link support).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/eth-phy-uclass.c | 9 ++++++++-
 include/eth_phy.h            | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
index 27b77444a0..05e1de8be9 100644
--- a/drivers/net/eth-phy-uclass.c
+++ b/drivers/net/eth-phy-uclass.c
@@ -109,7 +109,7 @@ struct mii_dev *eth_phy_get_mdio_bus(struct udevice *eth_dev)
 	return NULL;
 }
 
-int eth_phy_get_addr(struct udevice *dev)
+int eth_phy_get_node_and_addr(struct udevice *dev, ofnode *phy_node)
 {
 	struct ofnode_phandle_args phandle_args;
 	int reg;
@@ -121,10 +121,17 @@ int eth_phy_get_addr(struct udevice *dev)
 	}
 
 	reg = ofnode_read_u32_default(phandle_args.node, "reg", 0);
+	if (phy_node)
+		*phy_node = phandle_args.node;
 
 	return reg;
 }
 
+int eth_phy_get_addr(struct udevice *dev)
+{
+	return eth_phy_get_node_and_addr(dev, NULL);
+}
+
 /* parsing generic properties of devicetree/bindings/net/ethernet-phy.yaml */
 static int eth_phy_of_to_plat(struct udevice *dev)
 {
diff --git a/include/eth_phy.h b/include/eth_phy.h
index be6c881527..caf85c84a7 100644
--- a/include/eth_phy.h
+++ b/include/eth_phy.h
@@ -14,5 +14,6 @@ int eth_phy_binds_nodes(struct udevice *eth_dev);
 int eth_phy_set_mdio_bus(struct udevice *eth_dev, struct mii_dev *mdio_bus);
 struct mii_dev *eth_phy_get_mdio_bus(struct udevice *eth_dev);
 int eth_phy_get_addr(struct udevice *dev);
+int eth_phy_get_node_and_addr(struct udevice *dev, ofnode *phy_node);
 
 #endif
-- 
2.31.1

[PATCH 3/3] net: dwc_eth_qos: set proper DT node for phy device

[Rasmus Villemoes]

Similar to what was done for the FEC driver in commit
89b5bd54c1a4 (net: fec: Allow the PHY node to be retrieved), make sure
the PHY is associated with the right device tree node, so that phy
specific DT properties is accessible by the phy driver.

This is required on a custom iMX8MP board with a ti,dp83867 phy
sitting in front of the eqos interface.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/dwc_eth_qos.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
index 27b3f98e0e..af35960b42 100644
--- a/drivers/net/dwc_eth_qos.c
+++ b/drivers/net/dwc_eth_qos.c
@@ -1078,9 +1078,11 @@ static int eqos_start(struct udevice *dev)
 	 * don't need to reconnect/reconfigure again
 	 */
 	if (!eqos->phy) {
+		ofnode phy_node = ofnode_null();
 		int addr = -1;
+
 #ifdef CONFIG_DM_ETH_PHY
-		addr = eth_phy_get_addr(dev);
+		addr = eth_phy_get_node_and_addr(dev, &phy_node);
 #endif
 		eqos->phy = phy_connect(eqos->mii, addr, dev,
 					eqos->config->interface(dev));
@@ -1088,6 +1090,8 @@ static int eqos_start(struct udevice *dev)
 			pr_err("phy_connect() failed");
 			goto err_stop_resets;
 		}
+		if (ofnode_valid(phy_node))
+			eqos->phy->node = phy_node;
 
 		if (eqos->max_speed) {
 			ret = phy_set_supported(eqos->phy, eqos->max_speed);
-- 
2.31.1

Re: [PATCH 1/3] net: dwc_eth_qos: remove use of DWC_NET_PHYADDR

[Ramon Fried]

On Thu, May 12, 2022 at 10:34 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Only two boards in the tree set the macro DWC_NET_PHYADDR. Both have
> CONFIG_DM_ETH_PHY=y, so should set the phy address in DT if necessary.
>
> The imx8mp_evk does set the correct address in device tree.
>
> The other board seems to be a copy-paste-adapt from an old
> version of the imx8mp_evk config header, given the "#ifdef
> CONFIG_DWC_ETH_QOS" block that has been removed from imx8mp_evk header
> in commit 127fb454955. Its device tree doesn't even enable (i.e., set
> 'status = "okay"') the &eqos node. But the other ethernet device,
> &fec, does get enabled, and does have a phy sitting at address 4 (and
> it also has a corresponding legacy #define CONFIG_FEC_MXC_PHYADDR
> 4). So I believe it should be completely safe to remove it from there
> as well.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/net/dwc_eth_qos.c        | 3 ---
>  include/configs/imx8mp_evk.h     | 2 --
>  include/configs/imx8mp_rsb3720.h | 1 -
>  3 files changed, 6 deletions(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 9d255cf95f..27b3f98e0e 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1081,9 +1081,6 @@ static int eqos_start(struct udevice *dev)
>                 int addr = -1;
>  #ifdef CONFIG_DM_ETH_PHY
>                 addr = eth_phy_get_addr(dev);
> -#endif
> -#ifdef DWC_NET_PHYADDR
> -               addr = DWC_NET_PHYADDR;
>  #endif
>                 eqos->phy = phy_connect(eqos->mii, addr, dev,
>                                         eqos->config->interface(dev));
> diff --git a/include/configs/imx8mp_evk.h b/include/configs/imx8mp_evk.h
> index cc8d65cb54..f846c55d9c 100644
> --- a/include/configs/imx8mp_evk.h
> +++ b/include/configs/imx8mp_evk.h
> @@ -34,8 +34,6 @@
>  #define CONFIG_FEC_MXC_PHYADDR          1
>  #define FEC_QUIRK_ENET_MAC
>
> -#define DWC_NET_PHYADDR                        1
> -
>  #define PHY_ANEG_TIMEOUT 20000
>
>  #endif
> diff --git a/include/configs/imx8mp_rsb3720.h b/include/configs/imx8mp_rsb3720.h
> index c5dd545471..5608491b44 100644
> --- a/include/configs/imx8mp_rsb3720.h
> +++ b/include/configs/imx8mp_rsb3720.h
> @@ -53,7 +53,6 @@
>  #define CONFIG_FEC_MXC_PHYADDR          4
>  #define FEC_QUIRK_ENET_MAC
>
> -#define DWC_NET_PHYADDR                        4
>  #ifdef CONFIG_DWC_ETH_QOS
>  #define CONFIG_SYS_NONCACHED_MEMORY     (1 * SZ_1M)     /* 1M */
>  #endif
> --
> 2.31.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 3/3] net: dwc_eth_qos: set proper DT node for phy device

[Ramon Fried]

On Thu, May 12, 2022 at 10:35 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Similar to what was done for the FEC driver in commit
> 89b5bd54c1a4 (net: fec: Allow the PHY node to be retrieved), make sure
> the PHY is associated with the right device tree node, so that phy
> specific DT properties is accessible by the phy driver.
>
> This is required on a custom iMX8MP board with a ti,dp83867 phy
> sitting in front of the eqos interface.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/net/dwc_eth_qos.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 27b3f98e0e..af35960b42 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1078,9 +1078,11 @@ static int eqos_start(struct udevice *dev)
>          * don't need to reconnect/reconfigure again
>          */
>         if (!eqos->phy) {
> +               ofnode phy_node = ofnode_null();
>                 int addr = -1;
> +
>  #ifdef CONFIG_DM_ETH_PHY
> -               addr = eth_phy_get_addr(dev);
> +               addr = eth_phy_get_node_and_addr(dev, &phy_node);
>  #endif
>                 eqos->phy = phy_connect(eqos->mii, addr, dev,
>                                         eqos->config->interface(dev));
> @@ -1088,6 +1090,8 @@ static int eqos_start(struct udevice *dev)
>                         pr_err("phy_connect() failed");
>                         goto err_stop_resets;
>                 }
> +               if (ofnode_valid(phy_node))
> +                       eqos->phy->node = phy_node;
>
>                 if (eqos->max_speed) {
>                         ret = phy_set_supported(eqos->phy, eqos->max_speed);
> --
> 2.31.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 0/3] dwc_eth_qos PHY dt node fixups

[Rasmus Villemoes]

On 12/05/2022 09.33, Rasmus Villemoes wrote:
> I need the dwc_eth_qos to gain a fix similar to what fec_mxc got in
> 89b5bd54c1a4.
> 
> The first patch provides a simplification around the logic for
> fetching the phy's address, so there's one less case to worry about.
> 
> The second introduces a little helper (it doesn't essentially add any
> extra code since it just returns an extra piece of inforamtion that
> eth_phy_get_node() already found).
> 
> And the third hooks it up in the dwc_eth_qos driver.

ping... any chance these could get picked up? 1 and 3 has an R-b from
Ramon, and I don't think 2 should be controversial.

Rasmus

Re: [PATCH 1/3] net: dwc_eth_qos: remove use of DWC_NET_PHYADDR

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On Thu, May 12, 2022 at 09:33:07AM +0200, Rasmus Villemoes wrote:

> Only two boards in the tree set the macro DWC_NET_PHYADDR. Both have
> CONFIG_DM_ETH_PHY=y, so should set the phy address in DT if necessary.
> 
> The imx8mp_evk does set the correct address in device tree.
> 
> The other board seems to be a copy-paste-adapt from an old
> version of the imx8mp_evk config header, given the "#ifdef
> CONFIG_DWC_ETH_QOS" block that has been removed from imx8mp_evk header
> in commit 127fb454955. Its device tree doesn't even enable (i.e., set
> 'status = "okay"') the &eqos node. But the other ethernet device,
> &fec, does get enabled, and does have a phy sitting at address 4 (and
> it also has a corresponding legacy #define CONFIG_FEC_MXC_PHYADDR
> 4). So I believe it should be completely safe to remove it from there
> as well.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

As this part of the series is dead code removal, I've made it apply to
top of tree and applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] net: dwc_eth_qos: set proper DT node for phy device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

On Thu, May 12, 2022 at 09:33:09AM +0200, Rasmus Villemoes wrote:

> Similar to what was done for the FEC driver in commit
> 89b5bd54c1a4 (net: fec: Allow the PHY node to be retrieved), make sure
> the PHY is associated with the right device tree node, so that phy
> specific DT properties is accessible by the phy driver.
> 
> This is required on a custom iMX8MP board with a ti,dp83867 phy
> sitting in front of the eqos interface.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> ---
>  drivers/net/dwc_eth_qos.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 27b3f98e0e..af35960b42 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1078,9 +1078,11 @@ static int eqos_start(struct udevice *dev)
>  	 * don't need to reconnect/reconfigure again
>  	 */
>  	if (!eqos->phy) {
> +		ofnode phy_node = ofnode_null();
>  		int addr = -1;
> +
>  #ifdef CONFIG_DM_ETH_PHY
> -		addr = eth_phy_get_addr(dev);
> +		addr = eth_phy_get_node_and_addr(dev, &phy_node);
>  #endif
>  		eqos->phy = phy_connect(eqos->mii, addr, dev,
>  					eqos->config->interface(dev));
> @@ -1088,6 +1090,8 @@ static int eqos_start(struct udevice *dev)
>  			pr_err("phy_connect() failed");
>  			goto err_stop_resets;
>  		}
> +		if (ofnode_valid(phy_node))
> +			eqos->phy->node = phy_node;
>  
>  		if (eqos->max_speed) {
>  			ret = phy_set_supported(eqos->phy, eqos->max_speed);

I'm deferring 2/3 and now for 3/3, given a6acf95508e2 ("net: eqos: add
function to get phy node and address") (which yes, sigh, came after your
series but has been applied already, sorry) I don't see the obvious way
to migrate the changes in without testing. If needed still, can you
please rebase and resend? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/3] net: dwc_eth_qos: set proper DT node for phy device

[Rasmus Villemoes]

On 08/08/2022 21.07, Tom Rini wrote:
> On Thu, May 12, 2022 at 09:33:09AM +0200, Rasmus Villemoes wrote:
> 
>> Similar to what was done for the FEC driver in commit
>> 89b5bd54c1a4 (net: fec: Allow the PHY node to be retrieved), make sure
>> the PHY is associated with the right device tree node, so that phy
>> specific DT properties is accessible by the phy driver.
>>
>> This is required on a custom iMX8MP board with a ti,dp83867 phy
>> sitting in front of the eqos interface.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

> 
> I'm deferring 2/3 and now for 3/3, given a6acf95508e2 ("net: eqos: add
> function to get phy node and address") (which yes, sigh, came after your
> series but has been applied already, sorry) I don't see the obvious way
> to migrate the changes in without testing. If needed still, can you
> please rebase and resend? Thanks.
> 
Well, I have/had other changes on top that I didn't want to send before
this series went in, and those were a bit annoying to rebase. But
a6acf95508e2 does seem to achieve what 2/3+3/3 did, so those can be
dropped. Thanks for applying the other patches.

<moderate grumpiness>

However, we now have one more copy of more or less the exact same code,
so there's certainly an opportunity for somebody to do something like
2/3 and make both fec_mxc.c, dwc_eth_qos.c and probably others use such
a helper.

And on the subject of duplicate functionality with perhaps ever so
slightly different semantics, it's impressive that we have at least

  reset-delay-us/reset-post-delay-us
  reset-assert-us/reset-deassert-us
  phy-reset-duration/phy-reset-post-delay

making it hard to figure out what setting is honored in which contexts.

Oh well.

</moderate grumpiness>

Rasmus