imx8mp EQOS regression in dwc_eth_qos,c

[Elmar Psilog]

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 at 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