[PATCH 0/3] dwc_eth_qos PHY dt node fixups
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(-)
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
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
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!
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
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);
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
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();#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.
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
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
participants (3)
-
Ramon Fried -
Rasmus Villemoes -
Tom Rini