Message-ID: <4c5f438da678097642d9442e5d672a8423ef5804.camel@toradex.com>
Date:   Wed, 23 Mar 2022 10:55:33 +0000
From:   Marcel Ziswiler <marcel.ziswiler@...adex.com>
To:     "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>
CC:     "linux-imx@....com" <linux-imx@....com>,
        "reinhold.mueller@...rion.com" <reinhold.mueller@...rion.com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        "alexander.stein@...tq-group.com" <alexander.stein@...tq-group.com>,
        "olof@...om.net" <olof@...om.net>,
        "tharvey@...eworks.com" <tharvey@...eworks.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "vladimir.oltean@....com" <vladimir.oltean@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "arnd@...db.de" <arnd@...db.de>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "alexandru.marginean@....com" <alexandru.marginean@....com>
Subject: Re: [PATCH v1 3/3] arm64: dts: freescale: add initial support for
 verdin imx8m plus

Hi Laurent

On Wed, 2022-03-23 at 01:02 +0200, Laurent Pinchart wrote:
> Hi Marcel,
> 
> Thank you for the patch.

Thank you for reviewing/testing.

> On Thu, Mar 17, 2022 at 05:01:22PM +0100, Marcel Ziswiler wrote:
> > From: Marcel Ziswiler <marcel.ziswiler@...adex.com>
> > 
> > This patch adds the device tree to support Toradex Verdin iMX8M Plus [1]
> > a computer on module which can be used on different carrier boards.
> > 
> > The module consists of an NXP i.MX 8M Plus family SoC (either i.MX 8M
> > Plus Quad or 8M Plus QuadLite), a PCA9450C PMIC, a Gigabit Ethernet PHY,
> > 1, 2, 4 or 8 GB of LPDDR4 RAM, an eMMC, a TLA2024 ADC, an I2C EEPROM, an
> > RX8130 RTC, an optional I2C temperature sensor plus an optional
> > Bluetooth/Wi-Fi module.
> > 
> > Anything that is not self-contained on the module is disabled by
> > default.
> > 
> > The device tree for the Dahlia includes the module's device tree and
> > enables the supported peripherals of the carrier board.
> > 
> > The device tree for the Verdin Development Board includes the module's
> > device tree as well as the Dahlia one as it is a superset and supports
> > almost all peripherals available.
> > 
> > So far there is no display functionality supported at all but basic
> > console UART, USB host, eMMC and Ethernet functionality work fine.
> > 
> > [1] https://www.toradex.com/computer-on-modules/verdin-arm-family/nxp-imx-8m-plus
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@...adex.com>
> > 
> > ---
> > 
> >  arch/arm64/boot/dts/freescale/Makefile        |    4 +
> >  .../dts/freescale/imx8mp-verdin-dahlia.dtsi   |  125 ++
> >  .../boot/dts/freescale/imx8mp-verdin-dev.dtsi |   44 +
> >  .../imx8mp-verdin-nonwifi-dahlia.dts          |   18 +
> >  .../freescale/imx8mp-verdin-nonwifi-dev.dts   |   18 +
> >  .../dts/freescale/imx8mp-verdin-nonwifi.dtsi  |   54 +
> >  .../freescale/imx8mp-verdin-wifi-dahlia.dts   |   18 +
> >  .../dts/freescale/imx8mp-verdin-wifi-dev.dts  |   18 +
> >  .../dts/freescale/imx8mp-verdin-wifi.dtsi     |   82 +
> >  .../boot/dts/freescale/imx8mp-verdin.dtsi     | 1373 +++++++++++++++++
> >  10 files changed, 1754 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-dahlia.dtsi
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-dev.dtsi
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-nonwifi-dahlia.dts
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-nonwifi-dev.dts
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-nonwifi.dtsi
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-wifi-dahlia.dts
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-wifi-dev.dts
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin-wifi.dtsi
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
> 
> [snip]
> 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-verdin-dahlia.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp-verdin-dahlia.dtsi
> > new file mode 100644
> > index 000000000000..103d97a3b029
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-verdin-dahlia.dtsi
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> > +/*
> > + * Copyright 2022 Toradex
> > + */
> > +
> > +&backlight {
> > +       power-supply = <&reg_3p3v>;
> > +};
> > +
> > +/* Verdin SPI_1 */
> > +&ecspi1 {
> > +       status = "okay";
> > +};
> > +
> > +/* EEPROM on display adapter boards */
> > +&eeprom_display_adapter {
> > +       status = "okay";
> > +};
> > +
> > +/* EEPROM on Verdin Development board */
> > +&eeprom_carrier_board {
> > +       status = "okay";
> > +};
> > +
> > +&eqos {
> > +       status = "okay";
> > +};
> > +
> > +&flexcan1 {
> > +       status = "okay";
> > +};
> > +
> > +&flexcan2 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin QSPI_1 */
> > +&flexspi {
> > +       status = "okay";
> > +};
> > +
> > +/* Current measurement into module VCC */
> > +&hwmon {
> > +       status = "okay";
> > +};
> > +
> > +&hwmon_temp {
> > +       vs-supply = <&reg_1p8v>;
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin I2C_2_DSI */
> > +&i2c2 {
> > +       status = "okay";
> > +};
> > +
> > +&i2c3 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin I2C_1 */
> > +&i2c4 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin PCIE_1 */
> 
> Is this comment needed ? Is it a placeholder for future PCIe support ?
> If so I'd write
> 
> /* TODO: Verdin PCIE_1 */

Yes, exactly, let me do as you suggest.

> > +
> > +/* Verdin PWM_1 */
> > +&pwm1 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin PWM_2 */
> > +&pwm2 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin PWM_3_DSI */
> > +&pwm3 {
> > +       status = "okay";
> > +};
> > +
> > +&reg_usdhc2_vmmc {
> > +       vin-supply = <&reg_3p3v>;
> > +};
> > +
> > +/* VERDIN I2S_1 */
> 
> Same here. By the way, you may want to standardize on Verdin or VERDIN
> and not mix both. These comments apply to the other files too.

Yes, let me also add a TODO in front here. Plus yeah, this capitalized variant just slipped our eyes and I will
change them all to regular Verdin. Thanks for spotting this.

> > +
> > +/* Verdin UART_1 */
> > +&uart1 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin UART_2 */
> > +&uart2 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin UART_3, used as the Linux Console */
> > +&uart3 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin USB_1 */
> > +&usb3_0 {
> > +       status = "okay";
> > +};
> > +
> > +&usb3_phy0 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin USB_2 */
> > +&usb3_1 {
> > +       status = "okay";
> > +};
> > +
> > +&usb3_phy1 {
> > +       status = "okay";
> > +};
> > +
> > +/* Verdin SD_1 */
> > +&usdhc2 {
> > +       status = "okay";
> > +};
> 
> [snip]
> 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-
> > verdin.dtsi
> > new file mode 100644
> > index 000000000000..26d6c2819ee8
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
> > @@ -0,0 +1,1373 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> > +/*
> > + * Copyright 2022 Toradex
> > + */
> > +
> > +#include "dt-bindings/pwm/pwm.h"
> > +#include "imx8mp.dtsi"
> > +
> > +/ {
> > +       chosen {
> > +               stdout-path = &uart3;
> > +       };
> > +
> > +       aliases {
> > +               /* Ethernet aliases to ensure correct MAC addresses */
> > +               ethernet0 = &eqos;
> > +               ethernet1 = &fec;
> 
> On Dahlia the ethernet connector is routed to the eqos if I'm not
> mistaken.

Yes, actually the on-module PHY which is what is routed to the RJ-45 on Dahlia uses the EQOS MAC, correct.

> On my board U-Boot considers this to be the second ethernet
> controller, with the fec being the first one.

Yes, however, U-Boot does use the EQOS one as primary MAC as well. This is just how U-Boot does things. There
is no easy way to change the actual instance numbering. Actually just like on Linux really with the one
difference that U-Boot will always show both independent whether or not your carrier board even has the second
PHY or not. This is due to U-Boot doing lazy loading aka not touch any Ethernet hardware unless you actually
run some network commands.

> The mismatch results in
> the MAC addresses being swapped between eth0 and eth1 when comparing
> U-Boot and Linux.

No, this alias should really be what ensures the correct MAC addresses.

> Am I using a too old boot loader, or should the two
> ethernet controlls be swapped here ?

Yes, I guess you might not be using latest mainline U-Boot. I just verified this on my table again having both
the Dahlia and Verdin Development carrier boards available. Please note that on the Verdin Development Board
the primary Ethernet is also called eth1. Unfortunately, changing this naming is also not entirely trivial. But
it really should have the proper MAC address.

Dahlia

U-Boot 2022.04-rc4-00068-g5f68470d69 (Mar 23 2022 - 10:15:46 +0100)

Verdin iMX8MP # echo $ethaddr
00:14:2d:6c:71:64
=> verified with wireshark

root@...din-imx8mp-07106916:~# ip addr
...
4: eth0: <BROADCAST,MULTICAST,DYNAMIC,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
    link/ether 00:14:2d:6c:71:64 brd ff:ff:ff:ff:ff:ff
    inet 192.168.10.239/24 brd 192.168.10.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::214:2dff:fe6c:7164/64 scope link 
       valid_lft forever preferred_lft forever
...

Verdin Developer Board

U-Boot 2022.04-rc4-00068-g5f68470d69 (Mar 23 2022 - 10:15:46 +0100)

Verdin iMX8MP # echo $ethaddr  
00:14:2d:6c:71:64
=> verified with wireshark

root@...din-imx8mp-07106916:~# uname -a
Linux verdin-imx8mp-07106916 5.17.0-rc8-next-20220317-00003-g695dc7d13c2a #12 SMP PREEMPT Thu Mar 17 16:07:49
CET 2022 aarch64 aarch64 aarch64 GNU/Linux

root@...din-imx8mp-07106916:~# ip addr
...
2: eth0: <NO-CARRIER,BROADCAST,MULTICAST,DYNAMIC,UP> mtu 1500 qdisc mq state DOWN group default qlen 1000
    link/ether 00:14:2d:7c:71:64 brd ff:ff:ff:ff:ff:ff
...
5: eth1: <BROADCAST,MULTICAST,DYNAMIC,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
    link/ether 00:14:2d:6c:71:64 brd ff:ff:ff:ff:ff:ff
    inet 192.168.10.239/24 brd 192.168.10.255 scope global eth1
       valid_lft forever preferred_lft forever
    inet6 fe80::214:2dff:fe6c:7164/64 scope link 
       valid_lft forever preferred_lft forever
...

> > +               rtc0 = &rtc_i2c;
> > +               rtc1 = &snvs_rtc;
> > +       };
> 
> [snip]
> 
> With these issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

Thanks again!

Cheers

Marcel

Powered by blists - more mailing lists