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.gra...@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 = <®_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 = <ðphy0>;
> > + 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 = <ðphy1>;
> > + 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.gra...@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
