Re: [PATCH v4] imx: support i.MX8QM DMSSE20 a1 board

["oliver.graute@kococonnector.com" <oliver.graute@gmail.com>]

On 13/07/22, Marcel Ziswiler wrote:
> Hi Oliver
> 
> On Tue, 2022-07-12 at 12:14 +0200, Oliver Graute wrote:
> > Add i.MX8QM DMSSE20 a1 board support
> > 
> > U-Boot 2022.07-00033-g2a5cf8e9e7 (Jul 12 2022 - 11:29:05 +0200)
> > 
> > Model: Advantech iMX8QM DMSSE20
> > Board: DMS-SE20A1 8GB
> > Build: SCFW 549b1e18, SECO-FW c9de51c0, ATF 5782363
> > Boot:  USB
> > DRAM:  8 GiB
> > Core:  100 devices, 19 uclasses, devicetree: separate
> > MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
> > Loading Environment from MMC... OK
> > In:    serial@5a060000
> > Out:   serial@5a060000
> > Err:   serial@5a060000
> > Net:   eth0: ethernet@5b040000
> > Warning: ethernet@5b050000 (eth1) using random MAC address - aa:a2:be:5b:81:4a
> > , eth1: ethernet@5b050000
> > Hit any key to stop autoboot:  0
> > 
> > Signed-off-by: Oliver Graute <oliver.graute@kococonnector.com>
> > ---
> >  Changes for v4
> >  -update atf fw version
> >  -update seco fw version
> >  -update scfw version
> >  -move CONFIG_IMX_SMMU to imx8qm_dmsse20a1_defconfig
> >  -move CONFIG_LOADADDR to imx8qm_dmsse20a1_defconfig
> >  -move CONFIG_SYS_LOAD_ADDR to imx8qm_dmsse20a1_defconfig
> >  -move CONFIG_SYS_MALLOC_LEN to imx8qm_dmsse20a1_defconfig
> >  -move CONFIG_SYS_MMC_IMG_LOAD_PART to imx8qm_dmsse20a1_defconfig
> >  -move CONFIG_FSL_USDHC to imx8qm_dmsse20a1_defconfig
> >  -replaced CONFIG_SPL_MMC_SUPPORT with CONFIG_SPL_MMC
> >  -replaced CONFIG_SPL_SERIAL_SUPPORT with CONFIG_SPL_SERIAL
> >  -drop CONFIG_FEC_XCV_TYPE
> > 
> > Changes for v3
> >  -Remove addr parameter from reset_cpu()
> >  -moved some configs into defconfig
> > 
> > Changes for v2
> >  -replaced bd_t with struct bd_info
> >  -added missing DTS in MAINTAINERS
> >  -replaced README with imx8qm-dmsse20-a1.rst
> >  -move CMD_FUSE to Kconfig
> >  -removed fdt_high
> >  -added i2c support
> >  -added rtc support
> > 
> >  arch/arm/dts/Makefile                         |   1 +
> >  arch/arm/dts/imx8qm-dmsse20-a1.dts            | 407 ++++++++++++++++++
> >  arch/arm/mach-imx/imx8/Kconfig                |   7 +
> >  board/advantech/imx8qm_dmsse20_a1/Kconfig     |  17 +
> >  board/advantech/imx8qm_dmsse20_a1/MAINTAINERS |   7 +
> >  board/advantech/imx8qm_dmsse20_a1/Makefile    |   8 +
> >  .../imx8qm_dmsse20_a1/imx8qm_dmsse20_a1.c     | 188 ++++++++
> >  .../advantech/imx8qm_dmsse20_a1/imximage.cfg  |  21 +
> >  board/advantech/imx8qm_dmsse20_a1/spl.c       | 224 ++++++++++
> >  common/Kconfig                                |   2 +-
> >  configs/imx8qm_dmsse20a1_defconfig            | 105 +++++
> >  doc/board/advantech/imx8qm-dmsse20-a1.rst     |  59 +++
> >  doc/board/advantech/index.rst                 |   1 +
> >  include/configs/imx8qm_dmsse20.h              | 164 +++++++
> >  14 files changed, 1210 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm/dts/imx8qm-dmsse20-a1.dts
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/Kconfig
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/MAINTAINERS
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/Makefile
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/imx8qm_dmsse20_a1.c
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/imximage.cfg
> >  create mode 100644 board/advantech/imx8qm_dmsse20_a1/spl.c
> >  create mode 100644 configs/imx8qm_dmsse20a1_defconfig
> >  create mode 100644 doc/board/advantech/imx8qm-dmsse20-a1.rst
> >  create mode 100644 include/configs/imx8qm_dmsse20.h
> > 
> > diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> > index a7e0d9f6c0..50bc76289f 100644
> > --- a/arch/arm/dts/Makefile
> > +++ b/arch/arm/dts/Makefile
> > @@ -920,6 +920,7 @@ dtb-$(CONFIG_ARCH_IMX8) += \
> >         fsl-imx8qm-apalis.dtb \
> >         fsl-imx8qm-mek.dtb \
> >         imx8qm-cgtqmx8.dtb \
> > +       imx8qm-dmsse20-a1.dtb \
> >         imx8qm-rom7720-a1.dtb \
> >         fsl-imx8qxp-ai_ml.dtb \
> >         fsl-imx8qxp-colibri.dtb \
> > diff --git a/arch/arm/dts/imx8qm-dmsse20-a1.dts b/arch/arm/dts/imx8qm-dmsse20-a1.dts
> > new file mode 100644
> > index 0000000000..81ef7fb2ce
> > --- /dev/null
> > +++ b/arch/arm/dts/imx8qm-dmsse20-a1.dts
> > @@ -0,0 +1,407 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> For device trees it is usually advisable to use the following license:
> 
> GPL-2.0-or-later OR MIT
> 
> Anyway, please use at least latest SPDX notation being GPL-2.0-or-later rather than GPL-2.0+.

ok 

> 
> > +/*
> > + * Copyright 2017 NXP
> 
> That also seems bogus to me.

ok

> 
> > + */
> > +
> > +/dts-v1/;
> > +
> > +/* First 128KB is for PSCI ATF. */
> > +/memreserve/ 0x80000000 0x00020000;
> > +
> > +#include "fsl-imx8qm.dtsi"
> > +
> > +/ {
> > +       model = "Advantech iMX8QM DMSSE20";
> > +       compatible = "fsl,imx8qm-mek", "fsl,imx8qm";
> > +
> > +       aliases {
> > +          mmc0 = &usdhc1;
> > +          mmc2 = &usdhc3;
> > +       };
> > +
> > +       chosen {
> > +               bootargs = "console=ttyLP0,115200 earlycon=lpuart32,0x5a060000,115200";
> 
> This stuff is completely downstream bogus.

I'am confused here because I see such statements in a lot of device
trees.  

> 
> > +               stdout-path = &lpuart0;
> > +       };
> > +
> > +       regulators {
> 
> That grouping is also bogus.

What do you mean exactly?
> 
> > +               compatible = "simple-bus";
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               reg_usb_otg1_vbus: regulator@0 {
> > +                       compatible = "regulator-fixed";
> > +                       reg = <0>;
> 
> Using any such is also bogus (same with above @0 notation, of course).

I'am confused here too. What is the right notation now?

> 
> > +                       regulator-name = "usb_otg1_vbus";
> > +                       regulator-min-microvolt = <5000000>;
> > +                       regulator-max-microvolt = <5000000>;
> > +                       gpio = <&gpio4 3 GPIO_ACTIVE_HIGH>;
> > +                       enable-active-high;
> > +               };
> > +
> > +               reg_usdhc2_vmmc: usdhc2_vmmc {
> > +                       compatible = "regulator-fixed";
> > +                       regulator-name = "sw-3p3-sd1";
> > +                       regulator-min-microvolt = <3300000>;
> > +                       regulator-max-microvolt = <3300000>;
> > +                       gpio = <&gpio4 7 GPIO_ACTIVE_HIGH>;
> > +                       enable-active-high;
> > +               };
> > +
> > +               busfreq {
> > +                       status = "disabled";
> > +               };
> > +       };
> > +};
> > +
> > +&iomuxc {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_hog_1>;
> > +
> > +       imx8qm-mek {
> > +               pinctrl_hog_1: hoggrp-1 {
> 
> That notation vs. below without dash could also be cleaned up.

ok will fix

snip

> > +                       >;
> > +               };
> > +
> > +               pinctrl_rtc_mc_8803: rtc_mc_8803_grp{
> 
> Underscores in node names. Ugh!

ok will fix
> 
> > +                       fsl,pins = <
> > +                               SC_P_SIM0_POWER_EN_DMA_I2C3_SDA         0xc600004c
> > +                               SC_P_SIM0_PD_DMA_I2C3_SCL               0xc600004c
> > +                       >;
> > +               };
> > +
> > +               pinctrl_usdhc1: usdhc1grp {
> > +                       fsl,pins = <
> > +                               SC_P_EMMC0_CLK_CONN_EMMC0_CLK           0x06000041
> > +                               SC_P_EMMC0_CMD_CONN_EMMC0_CMD           0x00000021
> > +                               SC_P_EMMC0_DATA0_CONN_EMMC0_DATA0       0x00000021
> > +                               SC_P_EMMC0_DATA1_CONN_EMMC0_DATA1       0x00000021
> > +                               SC_P_EMMC0_DATA2_CONN_EMMC0_DATA2       0x00000021
> > +                               SC_P_EMMC0_DATA3_CONN_EMMC0_DATA3       0x00000021
> > +                               SC_P_EMMC0_DATA4_CONN_EMMC0_DATA4       0x00000021
> > +                               SC_P_EMMC0_DATA5_CONN_EMMC0_DATA5       0x00000021
> > +                               SC_P_EMMC0_DATA6_CONN_EMMC0_DATA6       0x00000021
> > +                               SC_P_EMMC0_DATA7_CONN_EMMC0_DATA7       0x00000021
> > +                               SC_P_EMMC0_STROBE_CONN_EMMC0_STROBE     0x06000041
> > +                               SC_P_EMMC0_RESET_B_CONN_EMMC0_RESET_B   0x00000021
> > +                       >;
> > +               };
> > +
> > +               pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
> 
> I believe this speed denomination is the very only place dashes would be used. See e.g. here
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mp-evk.dts#n578

thx for this hint
will fix it
> 
> > +       pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>;
> > +       pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;
> > +       pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>;
> > +       bus-width = <4>;
> > +       cd-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>;
> > +       wp-gpios = <&gpio5 21 GPIO_ACTIVE_HIGH>;
> > +       vmmc-supply = <&reg_usdhc2_vmmc>;
> 
> I would sort those alphabetically.

will fix it
> 
> > +       status = "okay";
> > +};
> > +
> > +&usdhc3 {
> > +       pinctrl-names = "default","state_100mhz", "state_200mhz";
> > +       pinctrl-0 = <&pinctrl_usdhc3>, <&pinctrl_usdhc3_gpio>;
> > +       pinctrl-1 = <&pinctrl_usdhc3_100mhz>, <&pinctrl_usdhc3_gpio>;
> > +       pinctrl-2 = <&pinctrl_usdhc3_200mhz>, <&pinctrl_usdhc3_gpio>;
> > +       bus-width = <4>;
> > +       cd-gpios = <&gpio4 12 GPIO_ACTIVE_LOW>;
> > +       wp-gpios = <&gpio4 11 GPIO_ACTIVE_HIGH>;
> > +       no-1-8-v;
> 
> Ditto.

ok
> 
> > +       status = "okay";
> > +};
> > +
> > +&fec1 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_fec1>;
> > +       phy-mode = "rgmii-id";
> > +       phy-handle = <&ethphy0>;
> > +       fsl,ar8031-phy-fixup;
> > +       fsl,magic-packet;
> 
> D.

ok
> 
> > +       status = "okay";
> > +
> > +       mdio {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               ethphy0: ethernet-phy@4 {
> > +                       compatible = "ethernet-phy-ieee802.3-c22";
> > +                       reg = <4>;
> > +               };
> > +       };
> > +};
> > +
> > +&fec2 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_fec2>;
> > +       phy-mode = "rgmii-id";
> > +       phy-handle = <&ethphy1>;
> > +       fsl,ar8031-phy-fixup;
> > +       fsl,magic-packet;
> > +       status = "okay";
> > +       fsl,mii-exclusive;
> 
> That actually is just downstream only NXP bogus.

you mean the `fsl,mii-exclusive`? so I should just drop it?

> 
> > new file mode 100644
> > index 0000000000..262ffcd683
> > --- /dev/null
> > +++ b/board/advantech/imx8qm_dmsse20_a1/Makefile
> > @@ -0,0 +1,8 @@
> > +#
> > +# Copyright 2017 NXP
> 
> D.

ok

> 
> > +#
> > +# SPDX-License-Identifier:     GPL-2.0+
> 
> D.

ok
> 
> > +#
> > +
> > +obj-y += imx8qm_dmsse20_a1.o
> > +obj-$(CONFIG_SPL_BUILD) += spl.o
> > diff --git a/board/advantech/imx8qm_dmsse20_a1/imx8qm_dmsse20_a1.c
> > b/board/advantech/imx8qm_dmsse20_a1/imx8qm_dmsse20_a1.c
> > new file mode 100644
> > index 0000000000..c3bc26f80d
> > --- /dev/null
> > +++ b/board/advantech/imx8qm_dmsse20_a1/imx8qm_dmsse20_a1.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> D.

ok
> 
> > +/*
> > + * Copyright 2017-2018 NXP
> > + * Copyright (C) 2020 Oliver Graute <oliver.graute@kococonnector.com>
> 
> How the time flies.


ok
> 
> > index 0000000000..e324c7ca37
> > --- /dev/null
> > +++ b/board/advantech/imx8qm_dmsse20_a1/imximage.cfg
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier:    GPL-2.0+ */
> 
> D.

ok
> 
> > +/*
> > + * Copyright 2018 NXP
> 
> D.

ok
> 
> > index 0000000000..06bb049c3a
> > --- /dev/null
> > +++ b/board/advantech/imx8qm_dmsse20_a1/spl.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> D.

ok

> 
> > +/*
> > + * Copyright 2017-2018 NXP
> 
> D.

ok
> 
> > +
> > +#endif /* __IMX8QM_DMSSE20_H */
> 
> Otherwise looks great. Keep up the good work!
> 

thx for the review

> Cheers
> 
> Marcel

Best Regards,

Oliver