imx8mp EQOS regression in dwc_eth_qos,c
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);
returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Thank you and best regards, Elmar
DTS should be correct &eqos { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_eqos>; phy-mode = "rgmii-id"; status = "okay";
// fixed-link = <0 1 1000 0 0>; // old - just for test fixed-link { speed = <1000>; full-duplex; }; };
Adding Marek, who has sent some EQOS patches recently.
On Mon, Feb 13, 2023 at 6:02 PM Elmar Psilog epsi@gmx.de wrote:
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Thank you and best regards, Elmar
DTS should be correct &eqos { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_eqos>; phy-mode = "rgmii-id"; status = "okay";
// fixed-link = <0 1 1000 0 0>; // old - just for test fixed-link { speed = <1000>; full-duplex; }; };
On 2/13/23 22:04, Fabio Estevam wrote:
Adding Marek, who has sent some EQOS patches recently.
On Mon, Feb 13, 2023 at 6:02 PM Elmar Psilog epsi@gmx.de wrote:
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Try this patch (needs CONFIG_FIXED_PHY=y) :
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index d488bd0c288..592af53b352 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -791,9 +791,21 @@ static int eqos_start(struct udevice *dev) */ if (!eqos->phy) { int addr = -1; - addr = eqos_get_phy_addr(eqos, dev); - eqos->phy = phy_connect(eqos->mii, addr, dev, - eqos->config->interface(dev)); + ofnode fixed_node; + + if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev), + "fixed-link"); + if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + } + + if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev, + eqos->config->interface(dev)); + } + if (!eqos->phy) { pr_err("phy_connect() failed"); goto err_stop_resets;
Am 13.02.23 um 22:20 schrieb Marek Vasut:
On 2/13/23 22:04, Fabio Estevam wrote:
Adding Marek, who has sent some EQOS patches recently.
On Mon, Feb 13, 2023 at 6:02 PM Elmar Psilog epsi@gmx.de wrote:
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);
returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Try this patch (needs CONFIG_FIXED_PHY=y) :
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index d488bd0c288..592af53b352 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -791,9 +791,21 @@ static int eqos_start(struct udevice *dev) */ if (!eqos->phy) { int addr = -1; - addr = eqos_get_phy_addr(eqos, dev); - eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ ofnode fixed_node;
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev),
- "fixed-link");
+ if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + }
+ if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ }
if (!eqos->phy) { pr_err("phy_connect() failed"); goto err_stop_resets;
Thanks Fabio for forwarding and Marek for the quick patch! Don't get this exactly. Bit confused about the closing "}" in if(IS_ENABLED) seems to be missed (or an else path)? Where exactly should this be? From the insertion it looks like you mean:
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev), + "fixed-link"); + if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + } + } + if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev, eqos->config->interface(dev)); + } ....
But that doesn't solve the issue. The magic line
eqos->phy->node = eqos->phy_of_node;
will still executed.
Likely a different issue but connected to EQOS: I am wondering why the clocks for ethernet look so much different if FEC is not configured. Shouldn't be the case, right? At least imx8mp.dtsi doesn't tell about any 24MHz clock. Also counter is "0"? Well, EQOS works with uncommenting the line above and this clock, but why?
clk_dump returns on NXP EVK board with FEC configured:
266666666 1 | | |-- sys_pll1_266m 266666666 0 | | | |-- nand_usdhc_bus 266666666 4 | | | `-- enet_axi 266666666 2 | | | |-- enet1_root_clk 266666666 2 | | | `-- sim_enet_root_clk ... 1000000000 1 | | `-- sys_pll2_bypass 1000000000 3 | | `-- sys_pll2_out 50000000 2 | | |-- sys_pll2_50m 50000000 2 | | | `-- enet_phy_ref 100000000 2 | | |-- sys_pll2_100m 100000000 2 | | | `-- enet_timer 125000000 2 | | |-- sys_pll2_125m 125000000 2 | | | `-- enet_ref 166666666 0 | | |-- sys_pll2_166m
and without FEC configured:
24000000 0 | |-- enet_axi 24000000 0 | | |-- enet1_root_clk 24000000 0 | | `-- sim_enet_root_clk ... 24000000 0 | |-- enet_ref 24000000 0 | |-- enet_timer 24000000 0 | |-- enet_phy_ref
Best regards, Elmar
On 2/14/23 18:31, Elmar Psilog wrote:
Am 13.02.23 um 22:20 schrieb Marek Vasut:
On 2/13/23 22:04, Fabio Estevam wrote:
Adding Marek, who has sent some EQOS patches recently.
On Mon, Feb 13, 2023 at 6:02 PM Elmar Psilog epsi@gmx.de wrote:
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);
returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Try this patch (needs CONFIG_FIXED_PHY=y) :
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index d488bd0c288..592af53b352 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -791,9 +791,21 @@ static int eqos_start(struct udevice *dev) */ if (!eqos->phy) { int addr = -1; - addr = eqos_get_phy_addr(eqos, dev); - eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ ofnode fixed_node;
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev),
- "fixed-link");
+ if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + }
+ if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ }
if (!eqos->phy) { pr_err("phy_connect() failed"); goto err_stop_resets;
Thanks Fabio for forwarding and Marek for the quick patch! Don't get this exactly. Bit confused about the closing "}" in if(IS_ENABLED) seems to be missed (or an else path)? Where exactly should this be? From the insertion it looks like you mean:
Yes, the { shouldn't be at the end of 'if (ofnode_valid(fixed_node)) {' .
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev), + "fixed-link"); + if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev));
Add this here (*) too (see below):
eqos->phy_of_node = fixed_node;
+ }
This works too.
+ } + if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev, eqos->config->interface(dev)); + } ....
But that doesn't solve the issue. The magic line
eqos->phy->node = eqos->phy_of_node;
will still executed.
See (*) above , does that help ?
Likely a different issue but connected to EQOS: I am wondering why the clocks for ethernet look so much different if FEC is not configured. Shouldn't be the case, right? At least imx8mp.dtsi doesn't tell about any 24MHz clock. Also counter is "0"? Well, EQOS works with uncommenting the line above and this clock, but why?
Each device is probed on first use, so the clock are left unconfigured until the device is actually used (e.g. for ethernet transfer). Note that there is a huge series which overhauls the EQoS and FEC clock on the ML, see:
[PATCH v3 01/14] clk: imx8mp: Add EQoS MAC clock
Am 15.02.23 um 01:19 schrieb Marek Vasut:
On 2/14/23 18:31, Elmar Psilog wrote:
Am 13.02.23 um 22:20 schrieb Marek Vasut:
On 2/13/23 22:04, Fabio Estevam wrote:
Adding Marek, who has sent some EQOS patches recently.
On Mon, Feb 13, 2023 at 6:02 PM Elmar Psilog epsi@gmx.de wrote:
Hello, Think I found a regression in EQOS driver with fixed-phy. Maybe someone with a imx8mp board might check that use case to confirm? That would be great. While ethernet was working in v2022.04 a "ping" in v2023.01 returns
ERROR: no/invalid <fixed-link> property! invalid speed 0 eqos_adjust_link() failed: -22 FAILED
although devicetree/hardware kept unchanged. This happens because in file fixed.c in in function fixedphy_config() the call
val = ofnode_read_u32_default(node, "speed", 0);
returns 0 instead of 1000 and also the duplex is not set. Found that in file/function dwc_eth_qos.c / eqos_start() the line
eqos->phy->node = eqos->phy_of_node;
is responsible for losing the information. Don't know what magic happens here - so I can't fix it - I just followed the data. So all works well and even the parsing of old and new fixed-link devicetree works til that line. After that I don't get speed anymore. Maybe you can have a look at this?
Try this patch (needs CONFIG_FIXED_PHY=y) :
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index d488bd0c288..592af53b352 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -791,9 +791,21 @@ static int eqos_start(struct udevice *dev) */ if (!eqos->phy) { int addr = -1; - addr = eqos_get_phy_addr(eqos, dev); - eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ ofnode fixed_node;
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev),
- "fixed-link");
+ if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + }
+ if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev,
- eqos->config->interface(dev));
+ }
if (!eqos->phy) { pr_err("phy_connect() failed"); goto err_stop_resets;
Thanks Fabio for forwarding and Marek for the quick patch! Don't get this exactly. Bit confused about the closing "}" in if(IS_ENABLED) seems to be missed (or an else path)? Where exactly should this be? From the insertion it looks like you mean:
Yes, the { shouldn't be at the end of 'if (ofnode_valid(fixed_node)) {' .
+ if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev), + "fixed-link"); + if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev));
Add this here (*) too (see below):
eqos->phy_of_node = fixed_node;
+ }
This works too.
+ } + if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev, eqos->config->interface(dev)); + } ....
But that doesn't solve the issue. The magic line
eqos->phy->node = eqos->phy_of_node;
will still executed.
See (*) above , does that help ?
Likely a different issue but connected to EQOS: I am wondering why the clocks for ethernet look so much different if FEC is not configured. Shouldn't be the case, right? At least imx8mp.dtsi doesn't tell about any 24MHz clock. Also counter is "0"? Well, EQOS works with uncommenting the line above and this clock, but why?
Each device is probed on first use, so the clock are left unconfigured until the device is actually used (e.g. for ethernet transfer). Note that there is a huge series which overhauls the EQoS and FEC clock on the ML, see:
[PATCH v3 01/14] clk: imx8mp: Add EQoS MAC clock
Marek, great! Can confirm that this patch (just to be sure attached complete below) does it's job.
My concern with the clock is, that what is done in u-boot seems to have an effect in linux/kernel too. Not all clocks seems to be reconfigured in kernel. I'll try the upcoming 2023.04 release next.
Again, big thanks.
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index afc47b56ff..10915d8e47 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -785,9 +785,21 @@ static int eqos_start(struct udevice *dev) */ if (!eqos->phy) { int addr = -1; - addr = eqos_get_phy_addr(eqos, dev); - eqos->phy = phy_connect(eqos->mii, addr, dev, - eqos->config->interface(dev)); + ofnode fixed_node; + + if (IS_ENABLED(CONFIG_PHY_FIXED)) { + fixed_node = ofnode_find_subnode(dev_ofnode(dev), + "fixed-link"); + if (ofnode_valid(fixed_node)) { + eqos->phy = fixed_phy_create(dev_ofnode(dev)); + eqos->phy_of_node = fixed_node; + } + } + if (!eqos->phy) { + addr = eqos_get_phy_addr(eqos, dev); + eqos->phy = phy_connect(eqos->mii, addr, dev, eqos->config->interface(dev)); + } + if (!eqos->phy) { pr_err("phy_connect() failed"); goto err_stop_resets;
Hi Elmar,
On Wed, Feb 15, 2023 at 3:02 AM Elmar Psilog epsi@gmx.de wrote:
Marek, great! Can confirm that this patch (just to be sure attached complete below) does it's job.
Care to submit a formal patch to the list?
It would be great to have this problem fixed in 2023.04.
participants (3)
-
Elmar Psilog -
Fabio Estevam -
Marek Vasut