Message-ID: <YjpVlmEGewIGE3WR@pendragon.ideasonboard.com>
Date:   Wed, 23 Mar 2022 01:02:46 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Marcel Ziswiler <marcel@...wiler.com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        Alex Marginean <alexandru.marginean@....com>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Arnd Bergmann <arnd@...db.de>,
        Fabio Estevam <festevam@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Olof Johansson <olof@...om.net>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Reinhold Mueller <reinhold.mueller@...rion.com>,
        Rob Herring <robh+dt@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>,
        Tim Harvey <tharvey@...eworks.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 3/3] arm64: dts: freescale: add initial support for
 verdin imx8m plus

Hi Marcel,

Thank you for the patch.

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 */

> +
> +/* 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.

> +
> +/* 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. On my board U-Boot considers this to be the second ethernet
controller, with the fec being the first one. The mismatch results in
the MAC addresses being swapped between eth0 and eth1 when comparing
U-Boot and Linux. Am I using a too old boot loader, or should the two
ethernet controlls be swapped here ?

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

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists