On Wed, 17 Jun 2020 at 22:01, Vladimir Oltean olteanv@gmail.com wrote:

Hi Fabio,

On Wed, 17 Jun 2020 at 21:30, Fabio Estevam festevam@gmail.com wrote:

...

Hi,

Now that mx6sabresd board has Ethernet working again with this fix: https://lists.denx.de/pipermail/u-boot/2020-June/416623.html

,I would like to remove the ar8031_phy_fixup() from board/freescale/mx6sabresd/mx6sabresd.c.

Here is what I tested so far: https://pastebin.com/raw/cQW5RmXW

However, I see that in drivers/net/phy/atheros.c the

if (!ofnode_read_u32(node, "qca,clk-out-frequency", &freq)) {

condition is never executed, so qca,clk-out-frequency configuration does not take effect.

Any ideas why this happens?

Thanks

I just tried it out on a board I have. This: printf("%s: found PHY node: %s\n", __func__, ofnode_get_name(node)); prints this: ar803x_of_init: found PHY node: ethernet@2d10000

which is very odd, because it's not the ethernet-phy node, but the node of the parent. It happens because ofnode_valid() is false here:

static inline ofnode phy_get_ofnode(struct phy_device *phydev) { if (ofnode_valid(phydev->node)) return phydev->node; else return dev_ofnode(phydev->dev); }

which again seems to be caused by this commit:

commit eef0b8a930d1a8799b8ebd26e67e27401df6a9f7 Author: Grygorii Strashko grygorii.strashko@ti.com Date: Thu Jul 5 12:02:48 2018 -0500

net: phy: add ofnode node to struct phy_device

Now the UCLASS_ETH device "node" field is owerwritten by some

network drivers in case of Ethernet PHYs which are linked to UCLASS_ETH device using "phy-handle" DT property and when Ethernet PHY driver needs to read some additional information from DT. In such cases following happens (in general):

- network drivers
        priv->phydev = phy_connect(priv->bus, priv->phyaddr, dev,
                                   priv->interface);
        <-- phydev is connected to dev which is UCLASS_ETH device

        if (priv->phy_of_handle > 0)
                dev_set_of_offset(priv->phydev->dev, priv->phy_of_handle);
        <-- phydev->dev->node is overwritten by phy-handle DT node

- PHY driver in .config() callback
        int node = dev_of_offset(dev);
        <-- PHY driver uses overwritten dev->node
        const void *fdt = gd->fdt_blob;

         if (fdtdec_get_bool(fdt, node, "property"))
                ...

As result, UCLASS_ETH device can't be used any more for DT accessing.

This patch adds additional ofnode node field to struct phy_device which can
be set explicitly by network drivers and used by PHY drivers, so
overwriting can be avoided. Also add helper function phy_get_ofnode()
which will check and return phy_device->node or dev_ofnode(phydev->dev) for
backward compatibility with existing drivers.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Didn't yet figure out what to do, though.

-Vladimir

Update: I upgraded tsec to DM_MDIO: https://patchwork.ozlabs.org/project/uboot/patch/20200612151735.49048-4-Zhiq... and it now prints the correct phy node: ar803x_of_init: found PHY node: ethernet-phy@2

Pretty sure that DM_MDIO is also how Michael tested this. What I'm pretty fuzzy about is: do we _need_ DM_MDIO for this to work? Who's more knowledgeable about this stuff around here?

-Vladimir

On Wed, 17 Jun 2020 at 22:22, Tom Rini trini@konsulko.com wrote:

On Wed, Jun 17, 2020 at 10:12:37PM +0300, Vladimir Oltean wrote:

...

On Wed, 17 Jun 2020 at 22:01, Vladimir Oltean olteanv@gmail.com wrote:

...

Hi Fabio,

On Wed, 17 Jun 2020 at 21:30, Fabio Estevam festevam@gmail.com wrote:

...

Hi,

Now that mx6sabresd board has Ethernet working again with this fix: https://lists.denx.de/pipermail/u-boot/2020-June/416623.html

,I would like to remove the ar8031_phy_fixup() from board/freescale/mx6sabresd/mx6sabresd.c.

Here is what I tested so far: https://pastebin.com/raw/cQW5RmXW

However, I see that in drivers/net/phy/atheros.c the

if (!ofnode_read_u32(node, "qca,clk-out-frequency", &freq)) {

condition is never executed, so qca,clk-out-frequency configuration does not take effect.

Any ideas why this happens?

Thanks

I just tried it out on a board I have. This: printf("%s: found PHY node: %s\n", __func__, ofnode_get_name(node)); prints this: ar803x_of_init: found PHY node: ethernet@2d10000

which is very odd, because it's not the ethernet-phy node, but the node of the parent. It happens because ofnode_valid() is false here:

static inline ofnode phy_get_ofnode(struct phy_device *phydev) { if (ofnode_valid(phydev->node)) return phydev->node; else return dev_ofnode(phydev->dev); }

which again seems to be caused by this commit:

commit eef0b8a930d1a8799b8ebd26e67e27401df6a9f7 Author: Grygorii Strashko grygorii.strashko@ti.com Date: Thu Jul 5 12:02:48 2018 -0500

net: phy: add ofnode node to struct phy_device

Now the UCLASS_ETH device "node" field is owerwritten by some

network drivers in case of Ethernet PHYs which are linked to UCLASS_ETH device using "phy-handle" DT property and when Ethernet PHY driver needs to read some additional information from DT. In such cases following happens (in general):

- network drivers
        priv->phydev = phy_connect(priv->bus, priv->phyaddr, dev,
                                   priv->interface);
        <-- phydev is connected to dev which is UCLASS_ETH device

        if (priv->phy_of_handle > 0)
                dev_set_of_offset(priv->phydev->dev, priv->phy_of_handle);
        <-- phydev->dev->node is overwritten by phy-handle DT node

- PHY driver in .config() callback
        int node = dev_of_offset(dev);
        <-- PHY driver uses overwritten dev->node
        const void *fdt = gd->fdt_blob;

         if (fdtdec_get_bool(fdt, node, "property"))
                ...

As result, UCLASS_ETH device can't be used any more for DT accessing.

This patch adds additional ofnode node field to struct phy_device which can
be set explicitly by network drivers and used by PHY drivers, so
overwriting can be avoided. Also add helper function phy_get_ofnode()
which will check and return phy_device->node or dev_ofnode(phydev->dev) for
backward compatibility with existing drivers.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Didn't yet figure out what to do, though.

-Vladimir

Update: I upgraded tsec to DM_MDIO: https://patchwork.ozlabs.org/project/uboot/patch/20200612151735.49048-4-Zhiq... and it now prints the correct phy node: ar803x_of_init: found PHY node: ethernet-phy@2

Pretty sure that DM_MDIO is also how Michael tested this. What I'm pretty fuzzy about is: do we _need_ DM_MDIO for this to work? Who's more knowledgeable about this stuff around here?

Grygorii can you chime in more on your thinking here? Thanks!

-- Tom

Not Grygorii's fault, I'm sure. He just noticed that there wasn't an of node for the PHY, and created one.

Today I learned, with the phy_connect API, you're supposed to populate the phydev node _yourself_. Like zynq_gem does:

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/net/zynq_gem.c#L3...

Too bad almost nobody else does......

With the new dm_eth_phy_connect API, that happens automatically:

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/net/mdio-uclass.c#L172

So, yeah. Fix the Ethernet driver, or switch to DM_MDIO.

-Vladimir

Am 2020-06-17 21:25, schrieb Vladimir Oltean:

On Wed, 17 Jun 2020 at 22:22, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jun 17, 2020 at 10:12:37PM +0300, Vladimir Oltean wrote:

...

On Wed, 17 Jun 2020 at 22:01, Vladimir Oltean olteanv@gmail.com wrote:

...

Hi Fabio,

On Wed, 17 Jun 2020 at 21:30, Fabio Estevam festevam@gmail.com wrote:

...

Hi,

Now that mx6sabresd board has Ethernet working again with this fix: https://lists.denx.de/pipermail/u-boot/2020-June/416623.html

,I would like to remove the ar8031_phy_fixup() from board/freescale/mx6sabresd/mx6sabresd.c.

Here is what I tested so far: https://pastebin.com/raw/cQW5RmXW

However, I see that in drivers/net/phy/atheros.c the

if (!ofnode_read_u32(node, "qca,clk-out-frequency", &freq)) {

condition is never executed, so qca,clk-out-frequency configuration does not take effect.

Any ideas why this happens?

Thanks

I just tried it out on a board I have. This: printf("%s: found PHY node: %s\n", __func__, ofnode_get_name(node)); prints this: ar803x_of_init: found PHY node: ethernet@2d10000

which is very odd, because it's not the ethernet-phy node, but the node of the parent. It happens because ofnode_valid() is false here:

static inline ofnode phy_get_ofnode(struct phy_device *phydev) { if (ofnode_valid(phydev->node)) return phydev->node; else return dev_ofnode(phydev->dev); }

which again seems to be caused by this commit:

commit eef0b8a930d1a8799b8ebd26e67e27401df6a9f7 Author: Grygorii Strashko grygorii.strashko@ti.com Date: Thu Jul 5 12:02:48 2018 -0500

net: phy: add ofnode node to struct phy_device

Now the UCLASS_ETH device "node" field is owerwritten by some

network drivers in case of Ethernet PHYs which are linked to UCLASS_ETH device using "phy-handle" DT property and when Ethernet PHY driver needs to read some additional information from DT. In such cases following happens (in general):

- network drivers
        priv->phydev = phy_connect(priv->bus, priv->phyaddr, dev,
                                   priv->interface);
        <-- phydev is connected to dev which is UCLASS_ETH device

        if (priv->phy_of_handle > 0)
                dev_set_of_offset(priv->phydev->dev, priv->phy_of_handle);
        <-- phydev->dev->node is overwritten by phy-handle DT node

- PHY driver in .config() callback
        int node = dev_of_offset(dev);
        <-- PHY driver uses overwritten dev->node
        const void *fdt = gd->fdt_blob;

         if (fdtdec_get_bool(fdt, node, "property"))
                ...

As result, UCLASS_ETH device can't be used any more for DT accessing.

This patch adds additional ofnode node field to struct phy_device which can
be set explicitly by network drivers and used by PHY drivers, so
overwriting can be avoided. Also add helper function phy_get_ofnode()
which will check and return phy_device->node or dev_ofnode(phydev->dev) for
backward compatibility with existing drivers.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Didn't yet figure out what to do, though.

-Vladimir

Update: I upgraded tsec to DM_MDIO: https://patchwork.ozlabs.org/project/uboot/patch/20200612151735.49048-4-Zhiq... and it now prints the correct phy node: ar803x_of_init: found PHY node: ethernet-phy@2

Pretty sure that DM_MDIO is also how Michael tested this. What I'm pretty fuzzy about is: do we _need_ DM_MDIO for this to work? Who's more knowledgeable about this stuff around here?

Grygorii can you chime in more on your thinking here? Thanks!

-- Tom

Not Grygorii's fault, I'm sure. He just noticed that there wasn't an of node for the PHY, and created one.

Today I learned, with the phy_connect API, you're supposed to populate the phydev node _yourself_. Like zynq_gem does:

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/net/zynq_gem.c#L3...

Too bad almost nobody else does......

Almost nobody ;) https://lists.denx.de/pipermail/u-boot/2019-October/388360.html

-michael

Hi Fabio,

Am 2020-06-17 23:02, schrieb Fabio Estevam:

Hi Michael,

On Wed, Jun 17, 2020 at 5:54 PM Michael Walle michael@walle.cc wrote:

...

Almost nobody ;) https://lists.denx.de/pipermail/u-boot/2019-October/388360.html

Cool, here is what I used: https://pastebin.com/raw/mcKdwwTs

Mine was just a shot in the dark, unfortunately, there wasn't a follow-up on that. until now ;)

That allows me to use 'qca,clk-out-frequency' in the device tree and get rid of the ath8031 board code :-)

nice!

-michael