diff mbox series

[v4,8/8] board: gw_ventana: enable MV88E61XX DSA support

Message ID 20220928193732.998665-9-tharvey@gateworks.com
State Superseded
Delegated to: Ramon Fried
Headers show
Series Add MV88E6xxx DSA driver and use on gwventana | expand

Commit Message

Tim Harvey Sept. 28, 2022, 7:37 p.m. UTC
Add MV88E61XX DSA support:
 - update dt: U-Boot dsa driver requires different device-tree syntax
   than the linux driver in order to link the dsa ports to the mdio bus.
 - update defconfig
 - replace mv88e61xx_hw_reset weak override with board_phy_config support
   for mv88e61xx configuration that is outside the scope of the DSA driver

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v4:
 - use standard Linux internal MDIO dt structure
 - use PHY_FIXED_ID define
v3:
 - move mdio's mdio@0 subnode
v2: no changes
---
 arch/arm/dts/imx6qdl-gw5904.dtsi        | 31 +++++++++++++++
 board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++----------------
 configs/gwventana_gw5904_defconfig      |  8 ++--
 drivers/net/mv88e6xxx.c                 | 29 ++++++--------
 4 files changed, 65 insertions(+), 53 deletions(-)

Comments

Fabio Estevam Sept. 28, 2022, 7:56 p.m. UTC | #1
Hi Tim,

On Wed, Sep 28, 2022 at 4:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Add MV88E61XX DSA support:
>  - update dt: U-Boot dsa driver requires different device-tree syntax
>    than the linux driver in order to link the dsa ports to the mdio bus.

Shouldn't the U-Boot dts version (imx6qdl-gw5904-u-boot.dtsi) be
updated instead?

Otherwise, this will cause problems when syncing imx6qdl-gw5904.dtsi
with Linux.
Tim Harvey Sept. 28, 2022, 7:59 p.m. UTC | #2
On Wed, Sep 28, 2022 at 12:56 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Wed, Sep 28, 2022 at 4:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > Add MV88E61XX DSA support:
> >  - update dt: U-Boot dsa driver requires different device-tree syntax
> >    than the linux driver in order to link the dsa ports to the mdio bus.
>
> Shouldn't the U-Boot dts version (imx6qdl-gw5904-u-boot.dtsi) be
> updated instead?
>
> Otherwise, this will cause problems when syncing imx6qdl-gw5904.dtsi
> with Linux.

Fabio,

That is one route to go but I'm hoping to update the Linux dt as well
to match so I'm not worried about it at this time.

Best Regards,

Tim
Fabio Estevam Oct. 3, 2022, 5:13 p.m. UTC | #3
Hi Tim,

On Wed, Sep 28, 2022 at 4:37 PM Tim Harvey <tharvey@gateworks.com> wrote:

> diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig
> index 8c6baf0ae5e2..33b57c5a1c5d 100644
> --- a/configs/gwventana_gw5904_defconfig
> +++ b/configs/gwventana_gw5904_defconfig
> @@ -111,11 +111,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y
>  CONFIG_FSL_USDHC=y
>  CONFIG_MTD=y
>  CONFIG_PHYLIB=y
> -CONFIG_MV88E61XX_SWITCH=y
> -CONFIG_MV88E61XX_CPU_PORT=5
> -CONFIG_MV88E61XX_PHY_PORTS=0xf
> -CONFIG_MV88E61XX_FIXED_PORTS=0x0
> +CONFIG_MV88E61XX=y

There is a typo here:

This one should be CONFIG_MV88E6XXX instead.

After fixing this and passing CONFIG_NET_RANDOM_ETHADDR=y to my custom
board defconfig:

 ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
 bootdev       2  [   ]   eth_bootdev           |   |       |--
ethernet@30be0000.bootdev
 mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
 dsa           0  [   ]   mv88e6xxx             |   |           `-- switch@0
 ethernet      1  [   ]   dsa-port              |   |               |-- lan4
 bootdev       3  [   ]   eth_bootdev           |   |               |
 `-- switch@0@0.bootdev
 ethernet      2  [   ]   dsa-port              |   |               |-- lan3
 bootdev       4  [   ]   eth_bootdev           |   |               |
 `-- switch@0@1.bootdev
 ethernet      3  [   ]   dsa-port              |   |               |-- lan2
 bootdev       5  [   ]   eth_bootdev           |   |               |
 `-- switch@0@2.bootdev
 ethernet      4  [   ]   dsa-port              |   |               |-- lan1
 bootdev       6  [   ]   eth_bootdev           |   |               |
 `-- switch@0@3.bootdev
 mdio          1  [ + ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-0
 mdio          2  [   ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-1
 mdio          3  [   ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-2
 mdio          4  [   ]   mv88e6xxx_mdio        |   |
`-- mv88e6xxx-mdio-3

but still not able to get a DHCP address:

u-boot=> dhcp
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4

Thanks
Tim Harvey Oct. 3, 2022, 6:22 p.m. UTC | #4
On Mon, Oct 3, 2022 at 10:13 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Wed, Sep 28, 2022 at 4:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig
> > index 8c6baf0ae5e2..33b57c5a1c5d 100644
> > --- a/configs/gwventana_gw5904_defconfig
> > +++ b/configs/gwventana_gw5904_defconfig
> > @@ -111,11 +111,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y
> >  CONFIG_FSL_USDHC=y
> >  CONFIG_MTD=y
> >  CONFIG_PHYLIB=y
> > -CONFIG_MV88E61XX_SWITCH=y
> > -CONFIG_MV88E61XX_CPU_PORT=5
> > -CONFIG_MV88E61XX_PHY_PORTS=0xf
> > -CONFIG_MV88E61XX_FIXED_PORTS=0x0
> > +CONFIG_MV88E61XX=y
>
> There is a typo here:
>
> This one should be CONFIG_MV88E6XXX instead.

Ooops - thanks for pointing that out. I have another patch on top to
merge two defconfigs that fixed that typo introduced with the v4
series that changed MV88E61XX to MV88E6XXX.

>
> After fixing this and passing CONFIG_NET_RANDOM_ETHADDR=y to my custom
> board defconfig:
>
>  ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
>  bootdev       2  [   ]   eth_bootdev           |   |       |--
> ethernet@30be0000.bootdev
>  mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
>  dsa           0  [   ]   mv88e6xxx             |   |           `-- switch@0
>  ethernet      1  [   ]   dsa-port              |   |               |-- lan4
>  bootdev       3  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@0.bootdev
>  ethernet      2  [   ]   dsa-port              |   |               |-- lan3
>  bootdev       4  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@1.bootdev
>  ethernet      3  [   ]   dsa-port              |   |               |-- lan2
>  bootdev       5  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@2.bootdev
>  ethernet      4  [   ]   dsa-port              |   |               |-- lan1
>  bootdev       6  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@3.bootdev
>  mdio          1  [ + ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-0
>  mdio          2  [   ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-1
>  mdio          3  [   ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-2
>  mdio          4  [   ]   mv88e6xxx_mdio        |   |
> `-- mv88e6xxx-mdio-3
>
> but still not able to get a DHCP address:
>
> u-boot=> dhcp
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> BOOTP broadcast 4

With CONFIG_CMD_NET=y what does 'net list' show?

I have the following (with network attached to lan4 port)
Net:   MV88E61XX eth0: ethernet@2188000, eth1: lan4, eth2: lan3, eth3: lan2, eth
4: lan1
Hit any key to stop autoboot:  0
Ventana > print ethaddr
ethaddr=00:d0:12:f3:f2:f5
Ventana > dm tree
...
 ethernet      0  [ + ]   fecmxc                |   |   |-- ethernet@2188000
 bootdev       0  [   ]   eth_bootdev           |   |   |   |--
ethernet@2188000.bootdev
 mdio          0  [ + ]   fec_mdio              |   |   |   `-- mdio
 dsa           0  [ + ]   mv88e6xxx             |   |   |       `-- switch@0
 ethernet      1  [ + ]   dsa-port              |   |   |           |-- lan4
 bootdev       2  [   ]   eth_bootdev           |   |   |           |
 `-- switch@0@0.bootdev
 ethernet      2  [ + ]   dsa-port              |   |   |           |-- lan3
 bootdev       3  [   ]   eth_bootdev           |   |   |           |
 `-- switch@0@1.bootdev
 ethernet      3  [ + ]   dsa-port              |   |   |           |-- lan2
 bootdev       4  [   ]   eth_bootdev           |   |   |           |
 `-- switch@0@2.bootdev
 ethernet      4  [ + ]   dsa-port              |   |   |           |-- lan1
 bootdev       5  [   ]   eth_bootdev           |   |   |           |
 `-- switch@0@3.bootdev
 mdio          1  [ + ]   mv88e6xxx_mdio        |   |   |
`-- mv88e6xxx-mdio-0
...
Ventana > net list
eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
eth1 : lan4 00:d0:12:f3:f2:f5
eth2 : lan3 00:d0:12:f3:f2:f5
eth3 : lan2 00:d0:12:f3:f2:f5
eth4 : lan1 00:d0:12:f3:f2:f5
Ventana > setenv ethact lan4
Ventana > dhcp
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
DHCP client bound to address 172.24.33.142 (1076 ms)

I do not use CONFIG_NET_RANDOM_ETHADDR and have a single MAC address
allocated to the board.

What board are you using and what does its fec/mdio dt look like?

Best Regards,

Tim
Fabio Estevam Oct. 3, 2022, 6:40 p.m. UTC | #5
Hi Tim,

On Mon, Oct 3, 2022 at 3:22 PM Tim Harvey <tharvey@gateworks.com> wrote:

> With CONFIG_CMD_NET=y what does 'net list' show?
>
> I have the following (with network attached to lan4 port)
> Net:   MV88E61XX eth0: ethernet@2188000, eth1: lan4, eth2: lan3, eth3: lan2, eth
> 4: lan1
> Hit any key to stop autoboot:  0
> Ventana > print ethaddr
> ethaddr=00:d0:12:f3:f2:f5
> Ventana > dm tree
> ...
>  ethernet      0  [ + ]   fecmxc                |   |   |-- ethernet@2188000
>  bootdev       0  [   ]   eth_bootdev           |   |   |   |--
> ethernet@2188000.bootdev
>  mdio          0  [ + ]   fec_mdio              |   |   |   `-- mdio
>  dsa           0  [ + ]   mv88e6xxx             |   |   |       `-- switch@0
>  ethernet      1  [ + ]   dsa-port              |   |   |           |-- lan4
>  bootdev       2  [   ]   eth_bootdev           |   |   |           |
>  `-- switch@0@0.bootdev
>  ethernet      2  [ + ]   dsa-port              |   |   |           |-- lan3
>  bootdev       3  [   ]   eth_bootdev           |   |   |           |
>  `-- switch@0@1.bootdev
>  ethernet      3  [ + ]   dsa-port              |   |   |           |-- lan2
>  bootdev       4  [   ]   eth_bootdev           |   |   |           |
>  `-- switch@0@2.bootdev
>  ethernet      4  [ + ]   dsa-port              |   |   |           |-- lan1
>  bootdev       5  [   ]   eth_bootdev           |   |   |           |
>  `-- switch@0@3.bootdev
>  mdio          1  [ + ]   mv88e6xxx_mdio        |   |   |
> `-- mv88e6xxx-mdio-0
> ...
> Ventana > net list
> eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> eth1 : lan4 00:d0:12:f3:f2:f5
> eth2 : lan3 00:d0:12:f3:f2:f5
> eth3 : lan2 00:d0:12:f3:f2:f5
> eth4 : lan1 00:d0:12:f3:f2:f5
> Ventana > setenv ethact lan4
> Ventana > dhcp
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> DHCP client bound to address 172.24.33.142 (1076 ms)
>
> I do not use CONFIG_NET_RANDOM_ETHADDR and have a single MAC address
> allocated to the board.

Below are the logs:

Net:   MV88E61XX
Warning: ethernet@30be0000 (eth0) using random MAC address - a6:a6:56:1c:28:0a
eth0: ethernet@30be0000
Hit any key to stop autoboot:  0

Your log also shows eth1-eth4. Mine does not.

Unlike your log, dsa-port is not getting probed here:

 ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
 bootdev       2  [   ]   eth_bootdev           |   |       |--
ethernet@30be0000.bootdev
 mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
 dsa           0  [   ]   mv88e6xxx             |   |           `-- switch@0
 ethernet      1  [   ]   dsa-port              |   |               |-- lan4
 bootdev       3  [   ]   eth_bootdev           |   |               |
 `-- switch@0@0.bootdev
 ethernet      2  [   ]   dsa-port              |   |               |-- lan3
 bootdev       4  [   ]   eth_bootdev           |   |               |
 `-- switch@0@1.bootdev
 ethernet      3  [   ]   dsa-port              |   |               |-- lan2
 bootdev       5  [   ]   eth_bootdev           |   |               |
 `-- switch@0@2.bootdev
 ethernet      4  [   ]   dsa-port              |   |               |-- lan1
 bootdev       6  [   ]   eth_bootdev           |   |               |
 `-- switch@0@3.bootdev
 mdio          1  [ + ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-0
 mdio          2  [   ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-1
 mdio          3  [   ]   mv88e6xxx_mdio        |   |
|-- mv88e6xxx-mdio-2
 mdio          4  [   ]   mv88e6xxx_mdio        |   |
`-- mv88e6xxx-mdio-3
 simple_bus    7  [   ]   simple_bus            |   `-- bus@32c00000
 usb           0  [   ]   ehci_mx6              |       `-- usb@32e40000
 phy           0  [   ]   nop_phy               |-- usbphynop1
 regulator     0  [   ]   regulator_fixed       |-- regulator-usdhc2
 sysreset      1  [   ]   wdt_reboot            |-- wdt-reboot
 bootstd       0  [   ]   bootstd_drv           `-- bootstd
 bootmeth      0  [   ]   bootmeth_distro           |-- distro
 bootmeth      1  [   ]   bootmeth_efi              |-- efi
 bootmeth      2  [   ]   bootmeth_pxe              |-- pxe
 bootmeth      3  [   ]   vbe_simple                `-- vbe_simple

u-boot=> net list
eth0 : ethernet@30be0000 a6:a6:56:1c:28:0a active
eth1 : lan4 00:00:00:00:00:00
eth2 : lan3 00:00:00:00:00:00
eth3 : lan2 00:00:00:00:00:00
eth4 : lan1 00:00:00:00:00:00
u-boot=> setenv ethact lan4
u-boot=> dhcp
BOOTP broadcast 1
BOOTP broadcast 2
BOOTP broadcast 3
BOOTP broadcast 4
BOOTP broadcast 5
BOOTP broadcast 6
BOOTP broadcast 7
BOOTP broadcast 8
(IP is never retrieved)

> What board are you using and what does its fec/mdio dt look like?

I am using a custom imx8mn board.

The FEC/MDIO nodes are like this:
https://pastebin.com/raw/0SvLLzxj

Thanks,

Fabio Estevam
Tim Harvey Oct. 3, 2022, 7:09 p.m. UTC | #6
On Mon, Oct 3, 2022 at 11:40 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Mon, Oct 3, 2022 at 3:22 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > With CONFIG_CMD_NET=y what does 'net list' show?
> >
> > I have the following (with network attached to lan4 port)
> > Net:   MV88E61XX eth0: ethernet@2188000, eth1: lan4, eth2: lan3, eth3: lan2, eth
> > 4: lan1
> > Hit any key to stop autoboot:  0
> > Ventana > print ethaddr
> > ethaddr=00:d0:12:f3:f2:f5
> > Ventana > dm tree
> > ...
> >  ethernet      0  [ + ]   fecmxc                |   |   |-- ethernet@2188000
> >  bootdev       0  [   ]   eth_bootdev           |   |   |   |--
> > ethernet@2188000.bootdev
> >  mdio          0  [ + ]   fec_mdio              |   |   |   `-- mdio
> >  dsa           0  [ + ]   mv88e6xxx             |   |   |       `-- switch@0
> >  ethernet      1  [ + ]   dsa-port              |   |   |           |-- lan4
> >  bootdev       2  [   ]   eth_bootdev           |   |   |           |
> >  `-- switch@0@0.bootdev
> >  ethernet      2  [ + ]   dsa-port              |   |   |           |-- lan3
> >  bootdev       3  [   ]   eth_bootdev           |   |   |           |
> >  `-- switch@0@1.bootdev
> >  ethernet      3  [ + ]   dsa-port              |   |   |           |-- lan2
> >  bootdev       4  [   ]   eth_bootdev           |   |   |           |
> >  `-- switch@0@2.bootdev
> >  ethernet      4  [ + ]   dsa-port              |   |   |           |-- lan1
> >  bootdev       5  [   ]   eth_bootdev           |   |   |           |
> >  `-- switch@0@3.bootdev
> >  mdio          1  [ + ]   mv88e6xxx_mdio        |   |   |
> > `-- mv88e6xxx-mdio-0
> > ...
> > Ventana > net list
> > eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> > eth1 : lan4 00:d0:12:f3:f2:f5
> > eth2 : lan3 00:d0:12:f3:f2:f5
> > eth3 : lan2 00:d0:12:f3:f2:f5
> > eth4 : lan1 00:d0:12:f3:f2:f5
> > Ventana > setenv ethact lan4
> > Ventana > dhcp
> > BOOTP broadcast 1
> > BOOTP broadcast 2
> > BOOTP broadcast 3
> > DHCP client bound to address 172.24.33.142 (1076 ms)
> >
> > I do not use CONFIG_NET_RANDOM_ETHADDR and have a single MAC address
> > allocated to the board.
>
> Below are the logs:
>
> Net:   MV88E61XX
> Warning: ethernet@30be0000 (eth0) using random MAC address - a6:a6:56:1c:28:0a
> eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0
>
> Your log also shows eth1-eth4. Mine does not.
>
> Unlike your log, dsa-port is not getting probed here:
>
>  ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
>  bootdev       2  [   ]   eth_bootdev           |   |       |--
> ethernet@30be0000.bootdev
>  mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
>  dsa           0  [   ]   mv88e6xxx             |   |           `-- switch@0
>  ethernet      1  [   ]   dsa-port              |   |               |-- lan4
>  bootdev       3  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@0.bootdev
>  ethernet      2  [   ]   dsa-port              |   |               |-- lan3
>  bootdev       4  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@1.bootdev
>  ethernet      3  [   ]   dsa-port              |   |               |-- lan2
>  bootdev       5  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@2.bootdev
>  ethernet      4  [   ]   dsa-port              |   |               |-- lan1
>  bootdev       6  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@3.bootdev
>  mdio          1  [ + ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-0
>  mdio          2  [   ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-1
>  mdio          3  [   ]   mv88e6xxx_mdio        |   |
> |-- mv88e6xxx-mdio-2
>  mdio          4  [   ]   mv88e6xxx_mdio        |   |
> `-- mv88e6xxx-mdio-3
>  simple_bus    7  [   ]   simple_bus            |   `-- bus@32c00000
>  usb           0  [   ]   ehci_mx6              |       `-- usb@32e40000
>  phy           0  [   ]   nop_phy               |-- usbphynop1
>  regulator     0  [   ]   regulator_fixed       |-- regulator-usdhc2
>  sysreset      1  [   ]   wdt_reboot            |-- wdt-reboot
>  bootstd       0  [   ]   bootstd_drv           `-- bootstd
>  bootmeth      0  [   ]   bootmeth_distro           |-- distro
>  bootmeth      1  [   ]   bootmeth_efi              |-- efi
>  bootmeth      2  [   ]   bootmeth_pxe              |-- pxe
>  bootmeth      3  [   ]   vbe_simple                `-- vbe_simple
>

Right - so it looks like the 'net: fec: add support for DM_MDIO' patch
is not doing its job to probe the mdio bus and ports.

Is CONFIG_DM_MDIO enabled and is dm_fec_bind_mdio returning successfully?

Is CONFIG_DM_ETH_PHY enabled? I don't use that and if I recall that
code never seemed right to me.

Tim

> u-boot=> net list
> eth0 : ethernet@30be0000 a6:a6:56:1c:28:0a active
> eth1 : lan4 00:00:00:00:00:00
> eth2 : lan3 00:00:00:00:00:00
> eth3 : lan2 00:00:00:00:00:00
> eth4 : lan1 00:00:00:00:00:00
> u-boot=> setenv ethact lan4
> u-boot=> dhcp
> BOOTP broadcast 1
> BOOTP broadcast 2
> BOOTP broadcast 3
> BOOTP broadcast 4
> BOOTP broadcast 5
> BOOTP broadcast 6
> BOOTP broadcast 7
> BOOTP broadcast 8
> (IP is never retrieved)
>
> > What board are you using and what does its fec/mdio dt look like?
>
> I am using a custom imx8mn board.
>
> The FEC/MDIO nodes are like this:
> https://pastebin.com/raw/0SvLLzxj
>
> Thanks,
>
> Fabio Estevam
Fabio Estevam Oct. 3, 2022, 10:35 p.m. UTC | #7
On 03/10/2022 16:09, Tim Harvey wrote:

> Right - so it looks like the 'net: fec: add support for DM_MDIO' patch
> is not doing its job to probe the mdio bus and ports.
> 
> Is CONFIG_DM_MDIO enabled and is dm_fec_bind_mdio returning 
> successfully?

Yes, for both.


> Is CONFIG_DM_ETH_PHY enabled? I don't use that and if I recall that
> code never seemed right to me.

CONFIG_DM_ETH_PHY is not enabled.

Regards,

Fabio Estevam
Tim Harvey Oct. 3, 2022, 11:39 p.m. UTC | #8
On Mon, Oct 3, 2022 at 3:35 PM Fabio Estevam <festevam@denx.de> wrote:
>
> On 03/10/2022 16:09, Tim Harvey wrote:
>
> > Right - so it looks like the 'net: fec: add support for DM_MDIO' patch
> > is not doing its job to probe the mdio bus and ports.
> >
> > Is CONFIG_DM_MDIO enabled and is dm_fec_bind_mdio returning
> > successfully?
>
> Yes, for both.
>
>
> > Is CONFIG_DM_ETH_PHY enabled? I don't use that and if I recall that
> > code never seemed right to me.
>
> CONFIG_DM_ETH_PHY is not enabled.
>

Fabio,

I'm not sure why your configuration is behaving differently and think
you'll have to instrument code to help understand it.

Here is the chain of events I see on my imx6q fec <-> mv88e6085 configuration:
fecmxc_probe() calls dm_fec_bind_mdio() which finds the mdio node in
dt and binds the 'fec_mdio' driver to it then probes that driver. When
it binds via device_bind_driver_to_node() it should end up calling
dsa_post_bind() to bind to the dsa_port driver. Can you verify that
you have DM_DSA enabled and that dsa_post_bind() gets called?

You can enable debug for dsa-uclass.c and perhaps that will explain
more. In dsa_post_bind() there is a loop over pdata->num_ports which
calls device_bind_driver_to_node() to bind the node to
DSA_PORT_CHILD_DRV_NAME - I imagine that isn't happening due to one of
the checks prior to this failing silently.

Tim
Fabio Estevam Oct. 3, 2022, 11:46 p.m. UTC | #9
Hi Tim,

On Mon, Oct 3, 2022 at 8:39 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Fabio,
>
> I'm not sure why your configuration is behaving differently and think
> you'll have to instrument code to help understand it.
>
> Here is the chain of events I see on my imx6q fec <-> mv88e6085 configuration:

The switch I am using is a mv88e6320.

I instrumented the code a bit and I notice that I am not able to read
the switch ID.

mv88e6xxx_get_switch_id() fails because mv88e6xxx_port_read() returns 0xffff.

I am investigating this failure.

Thanks
Fabio Estevam Oct. 4, 2022, 12:14 a.m. UTC | #10
On Mon, Oct 3, 2022 at 8:46 PM Fabio Estevam <festevam@gmail.com> wrote:

> The switch I am using is a mv88e6320.
>
> I instrumented the code a bit and I notice that I am not able to read
> the switch ID.
>
> mv88e6xxx_get_switch_id() fails because mv88e6xxx_port_read() returns 0xffff.

This was caused by the wrong description of the Ethernet switch reset.

With this fixed, dm tree looks a bit better:

 ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
 bootdev       2  [   ]   eth_bootdev           |   |       |--
ethernet@30be0000.bootdev
 mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
 dsa           0  [ + ]   mv88e6xxx             |   |           `-- switch@0
 ethernet      1  [   ]   dsa-port              |   |               |-- lan0
 bootdev       3  [   ]   eth_bootdev           |   |               |
 `-- switch@0@0.bootdev
 ethernet      2  [   ]   dsa-port              |   |               |-- lan1
 bootdev       4  [   ]   eth_bootdev           |   |               |
 `-- switch@0@1.bootdev
 ethernet      3  [ + ]   dsa-port              |   |               |-- lan3
 bootdev       5  [   ]   eth_bootdev           |   |               |
 `-- switch@0@3.bootdev
 mdio          1  [ + ]   mv88e6xxx_mdio        |   |
`-- mv88e6xxx-mdio-0

Boot log:
Net:   MV88E61XX
Warning: ethernet@30be0000 (eth0) using random MAC address - 36:6f:7f:0c:61:14
eth0: ethernet@30be0000Could not get PHY for mv88e6xxx-mdio-0: addr 0
Could not get PHY for mv88e6xxx-mdio-0: addr 1
, eth3: lan3

u-boot=> net list
eth0 : ethernet@30be0000 36:6f:7f:0c:61:14 active
eth1 : lan0 00:00:00:00:00:00
eth2 : lan1 00:00:00:00:00:00
eth3 : lan3 36:6f:7f:0c:61:14

but still not able to get DHCP IP.
Tim Harvey Oct. 4, 2022, 12:23 a.m. UTC | #11
On Mon, Oct 3, 2022 at 5:15 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Mon, Oct 3, 2022 at 8:46 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> > The switch I am using is a mv88e6320.
> >
> > I instrumented the code a bit and I notice that I am not able to read
> > the switch ID.
> >
> > mv88e6xxx_get_switch_id() fails because mv88e6xxx_port_read() returns 0xffff.
>
> This was caused by the wrong description of the Ethernet switch reset.

Seems like something that needs an error print? Which driver this?

>
> With this fixed, dm tree looks a bit better:
>
>  ethernet      0  [ + ]   fecmxc                |   |   `-- ethernet@30be0000
>  bootdev       2  [   ]   eth_bootdev           |   |       |--
> ethernet@30be0000.bootdev
>  mdio          0  [ + ]   fec_mdio              |   |       `-- mdio
>  dsa           0  [ + ]   mv88e6xxx             |   |           `-- switch@0
>  ethernet      1  [   ]   dsa-port              |   |               |-- lan0
>  bootdev       3  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@0.bootdev
>  ethernet      2  [   ]   dsa-port              |   |               |-- lan1
>  bootdev       4  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@1.bootdev
>  ethernet      3  [ + ]   dsa-port              |   |               |-- lan3
>  bootdev       5  [   ]   eth_bootdev           |   |               |
>  `-- switch@0@3.bootdev
>  mdio          1  [ + ]   mv88e6xxx_mdio        |   |
> `-- mv88e6xxx-mdio-0
>
> Boot log:
> Net:   MV88E61XX
> Warning: ethernet@30be0000 (eth0) using random MAC address - 36:6f:7f:0c:61:14
> eth0: ethernet@30be0000Could not get PHY for mv88e6xxx-mdio-0: addr 0
> Could not get PHY for mv88e6xxx-mdio-0: addr 1
> , eth3: lan3

This doesn't look correct - why could it not get the phy for lan1 and
lan2 yet lan3 was fine? I would assume a DT port/reg mismatch.

>
> u-boot=> net list
> eth0 : ethernet@30be0000 36:6f:7f:0c:61:14 active
> eth1 : lan0 00:00:00:00:00:00
> eth2 : lan1 00:00:00:00:00:00
> eth3 : lan3 36:6f:7f:0c:61:14
>
> but still not able to get DHCP IP.

Where are you setting your active port? By default eth0 is active and
thats the cpu uplink. If you have a network connection on lan3 port
you would need to 'setenv ethact lan3'.

Tim
Fabio Estevam Oct. 4, 2022, 12:39 a.m. UTC | #12
Hi Tim,

On Mon, Oct 3, 2022 at 9:23 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Where are you setting your active port? By default eth0 is active and
> thats the cpu uplink. If you have a network connection on lan3 port
> you would need to 'setenv ethact lan3'.

It works now: both 'dhcp' and 'tftp' commands are functional.

Thanks a lot for your work on this and for your help!

If you send a v5 with the CONFIG_MV88E6XXX=y typo fixed,
feel free to add for the whole series:

Reviewed-by: Fabio Estevam <festevam@denx.de>
Tim Harvey Oct. 4, 2022, 3:26 p.m. UTC | #13
On Mon, Oct 3, 2022 at 5:39 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Mon, Oct 3, 2022 at 9:23 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > Where are you setting your active port? By default eth0 is active and
> > thats the cpu uplink. If you have a network connection on lan3 port
> > you would need to 'setenv ethact lan3'.
>
> It works now: both 'dhcp' and 'tftp' commands are functional.
>
> Thanks a lot for your work on this and for your help!
>
> If you send a v5 with the CONFIG_MV88E6XXX=y typo fixed,
> feel free to add for the whole series:
>
> Reviewed-by: Fabio Estevam <festevam@denx.de>

Fabio,

Thanks for testing this. I'll submit a v5 shortly with your rb tag and
support for 6320:
--- a/drivers/net/mv88e6xxx.c
+++ b/drivers/net/mv88e6xxx.c
@@ -176,6 +176,7 @@
 #define PORT_SWITCH_ID_6220            0x2200
 #define PORT_SWITCH_ID_6240            0x2400
 #define PORT_SWITCH_ID_6250            0x2500
+#define PORT_SWITCH_ID_6320            0x1150
 #define PORT_SWITCH_ID_6352            0x3520

 struct mv88e6xxx_priv {
@@ -792,6 +793,7 @@ static int mv88e6xxx_probe(struct udevice *dev)
        case PORT_SWITCH_ID_6071:
        case PORT_SWITCH_ID_6220:
        case PORT_SWITCH_ID_6250:
+       case PORT_SWITCH_ID_6320:
                priv->port_count = 7;
                priv->phy_ctrl1_en_det_shift = 14;
                priv->phy_ctrl1_en_det_width = 1;


Was there an error path you found in drivers/net/mv88e6xxx.c that
should print an error to explain a failed probe for better future
troubleshooting or was that failure somewhere else? It sounds like
your switch was held in reset such that register reads were failing.

Best Regards,

Tim
Fabio Estevam Oct. 4, 2022, 4:25 p.m. UTC | #14
Hi Tim,

On Tue, Oct 4, 2022 at 12:26 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Fabio,
>
> Thanks for testing this. I'll submit a v5 shortly with your rb tag and
> support for 6320:

Thanks for including the 6320 model!

> Was there an error path you found in drivers/net/mv88e6xxx.c that
> should print an error to explain a failed probe for better future
> troubleshooting or was that failure somewhere else? It sounds like
> your switch was held in reset such that register reads were failing.

Yes, that was the reason for the register read failure.

Initially, I had described the reset GPIO inside using reset-gpios inside
the switch node, just like described in the Linux binding document:
Documentation/devicetree/bindings/net/dsa/marvell.txt

It did not work, so I moved it to phy-reset-gpios inside the fec node.

I don't think this is a blocker for your v5 series. We can improve it
later so that
the binding from Linux could also work in U-Boot.

Thanks
Fabio Estevam Oct. 4, 2022, 4:47 p.m. UTC | #15
Hi Tim,

On Tue, Oct 4, 2022 at 1:25 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Tue, Oct 4, 2022 at 12:26 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > Fabio,
> >
> > Thanks for testing this. I'll submit a v5 shortly with your rb tag and
> > support for 6320:
>
> Thanks for including the 6320 model!
>
> > Was there an error path you found in drivers/net/mv88e6xxx.c that
> > should print an error to explain a failed probe for better future
> > troubleshooting or was that failure somewhere else? It sounds like

I forgot to reply to this one. Yes, it would help if we print an error like
this:

--- a/drivers/net/mv88e6xxx.c
+++ b/drivers/net/mv88e6xxx.c
@@ -410,8 +410,10 @@ static int mv88e6xxx_get_switch_id(struct udevice *dev)
        int res;

        res = mv88e6xxx_port_read(dev, 0, PORT_REG_SWITCH_ID);
-       if (res < 0)
+       if (res < 0) {
+               dev_err(dev, "Failed to read switch ID: %d\n", res);
                return res;
+       }
        return res & 0xfff0;
 }

Thanks
Tim Harvey Oct. 4, 2022, 4:50 p.m. UTC | #16
On Tue, Oct 4, 2022 at 9:47 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Tue, Oct 4, 2022 at 1:25 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Tim,
> >
> > On Tue, Oct 4, 2022 at 12:26 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > > Fabio,
> > >
> > > Thanks for testing this. I'll submit a v5 shortly with your rb tag and
> > > support for 6320:
> >
> > Thanks for including the 6320 model!
> >
> > > Was there an error path you found in drivers/net/mv88e6xxx.c that
> > > should print an error to explain a failed probe for better future
> > > troubleshooting or was that failure somewhere else? It sounds like
>
> I forgot to reply to this one. Yes, it would help if we print an error like
> this:
>
> --- a/drivers/net/mv88e6xxx.c
> +++ b/drivers/net/mv88e6xxx.c
> @@ -410,8 +410,10 @@ static int mv88e6xxx_get_switch_id(struct udevice *dev)
>         int res;
>
>         res = mv88e6xxx_port_read(dev, 0, PORT_REG_SWITCH_ID);
> -       if (res < 0)
> +       if (res < 0) {
> +               dev_err(dev, "Failed to read switch ID: %d\n", res);
>                 return res;
> +       }
>         return res & 0xfff0;
>  }
>
> Thanks

Fabio,

I just sent a v5 before seeing this. I think all we are waiting for on
this series is for Vladimir to ok the dt changes as I believe I have
finally done the right thing there now.

If I need to do a v6 I'll add this in, otherwise I'll send it after it
gets accepted and merged.

Best Regards,

Tim
Fabio Estevam Oct. 4, 2022, 4:52 p.m. UTC | #17
Hi Tim,

On 04/10/2022 13:50, Tim Harvey wrote:

> Fabio,
> 
> I just sent a v5 before seeing this. I think all we are waiting for on
> this series is for Vladimir to ok the dt changes as I believe I have
> finally done the right thing there now.
> 
> If I need to do a v6 I'll add this in, otherwise I'll send it after it
> gets accepted and merged.

That's totally fine. Thanks!
diff mbox series

Patch

diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi
index 612b6e068e28..487dd7648274 100644
--- a/arch/arm/dts/imx6qdl-gw5904.dtsi
+++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
@@ -212,6 +212,27 @@ 
 			compatible = "marvell,mv88e6085";
 			reg = <0>;
 
+			mdio {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sw_phy0: ethernet-phy@0 {
+					reg = <0x0>;
+				};
+
+				sw_phy1: ethernet-phy@1 {
+					reg = <0x1>;
+				};
+
+				sw_phy2: ethernet-phy@2 {
+					reg = <0x2>;
+				};
+
+				sw_phy3: ethernet-phy@3 {
+					reg = <0x3>;
+				};
+			};
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -219,27 +240,37 @@ 
 				port@0 {
 					reg = <0>;
 					label = "lan4";
+					phy-handle = <&sw_phy0>;
 				};
 
 				port@1 {
 					reg = <1>;
 					label = "lan3";
+					phy-handle = <&sw_phy1>;
 				};
 
 				port@2 {
 					reg = <2>;
 					label = "lan2";
+					phy-handle = <&sw_phy2>;
 				};
 
 				port@3 {
 					reg = <3>;
 					label = "lan1";
+					phy-handle = <&sw_phy3>;
 				};
 
 				port@5 {
 					reg = <5>;
 					label = "cpu";
 					ethernet = <&fec>;
+					phy-mode = "rgmii-id";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+					};
 				};
 			};
 		};
diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c
index 99f52b9953e2..1c3da2c82707 100644
--- a/board/gateworks/gw_ventana/gw_ventana.c
+++ b/board/gateworks/gw_ventana/gw_ventana.c
@@ -83,44 +83,30 @@  int board_phy_config(struct phy_device *phydev)
 		break;
 	}
 
+	/* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
+	if (phydev->phy_id == PHY_FIXED_ID &&
+	    (board_type == GW5904 || board_type == GW5909)) {
+		struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
+
+		puts("MV88E61XX ");
+		/* GPIO[0] output CLK125 for RGMII_REFCLK */
+		bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe);
+		bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
+
+		/* Port 0-3 LED configuration: Table 80/82 */
+		/* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
+		bus->write(bus, 0x10, 0, 0x16, 0x8088);
+		bus->write(bus, 0x11, 0, 0x16, 0x8088);
+		bus->write(bus, 0x12, 0, 0x16, 0x8088);
+		bus->write(bus, 0x13, 0, 0x16, 0x8088);
+	}
+
 	if (phydev->drv->config)
 		phydev->drv->config(phydev);
 
 	return 0;
 }
 
-#ifdef CONFIG_MV88E61XX_SWITCH
-int mv88e61xx_hw_reset(struct phy_device *phydev)
-{
-	struct mii_dev *bus = phydev->bus;
-
-	/* GPIO[0] output, CLK125 */
-	debug("enabling RGMII_REFCLK\n");
-	bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
-		   0x1a /*MV_SCRATCH_MISC*/,
-		   (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
-	bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
-		   0x1a /*MV_SCRATCH_MISC*/,
-		   (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
-
-	/* RGMII delay - Physical Control register bit[15:14] */
-	debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT);
-	/* forced 1000mbps full-duplex link */
-	bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
-	phydev->autoneg = AUTONEG_DISABLE;
-	phydev->speed = SPEED_1000;
-	phydev->duplex = DUPLEX_FULL;
-
-	/* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
-	bus->write(bus, 0x10, 0, 0x16, 0x8088);
-	bus->write(bus, 0x11, 0, 0x16, 0x8088);
-	bus->write(bus, 0x12, 0, 0x16, 0x8088);
-	bus->write(bus, 0x13, 0, 0x16, 0x8088);
-
-	return 0;
-}
-#endif // CONFIG_MV88E61XX_SWITCH
-
 #if defined(CONFIG_VIDEO_IPUV3)
 static void enable_hdmi(struct display_info_t const *dev)
 {
diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig
index 8c6baf0ae5e2..33b57c5a1c5d 100644
--- a/configs/gwventana_gw5904_defconfig
+++ b/configs/gwventana_gw5904_defconfig
@@ -111,11 +111,11 @@  CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_FSL_USDHC=y
 CONFIG_MTD=y
 CONFIG_PHYLIB=y
-CONFIG_MV88E61XX_SWITCH=y
-CONFIG_MV88E61XX_CPU_PORT=5
-CONFIG_MV88E61XX_PHY_PORTS=0xf
-CONFIG_MV88E61XX_FIXED_PORTS=0x0
+CONFIG_MV88E61XX=y
+CONFIG_PHY_FIXED=y
+CONFIG_DM_ETH=y
 CONFIG_DM_MDIO=y
+CONFIG_DM_DSA=y
 CONFIG_E1000=y
 CONFIG_FEC_MXC=y
 CONFIG_MII=y
diff --git a/drivers/net/mv88e6xxx.c b/drivers/net/mv88e6xxx.c
index a3a3f6cf4acc..6064bb73257b 100644
--- a/drivers/net/mv88e6xxx.c
+++ b/drivers/net/mv88e6xxx.c
@@ -727,31 +727,26 @@  static const struct dsa_ops mv88e6xxx_dsa_ops = {
 static int mv88e6xxx_probe_mdio(struct udevice *dev)
 {
 	struct udevice *mdev;
-	ofnode node, mdios;
 	const char *name;
+	ofnode node;
 	int ret;
 
-	/* bind phy ports of mdios child node to mv88e6xxx_mdio device */
-	mdios = dev_read_subnode(dev, "mdios");
-	if (!ofnode_valid(mdios))
+	/* bind phy ports of mdio child node to mv88e6xxx_mdio device */
+	node = dev_read_subnode(dev, "mdio");
+	if (!ofnode_valid(node))
 		return 0;
 
-	ofnode_for_each_subnode(node, mdios) {
-		name = ofnode_get_name(node);
-		ret = device_bind_driver_to_node(dev,
-						 "mv88e6xxx_mdio",
-						 name, node, NULL);
-		if (ret) {
-			dev_err(dev, "failed to bind %s: %d\n", name, ret);
-			continue;
-		}
-
+	name = ofnode_get_name(node);
+	ret = device_bind_driver_to_node(dev,
+					 "mv88e6xxx_mdio",
+					 name, node, NULL);
+	if (ret) {
+		dev_err(dev, "failed to bind %s: %d\n", name, ret);
+	} else {
 		/* need to probe it as there is no compatible to do so */
 		ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdev);
-		if (ret) {
+		if (ret)
 			dev_err(dev, "failed to probe %s: %d\n", name, ret);
-			continue;
-		}
 	}
 
 	return ret;