Hi Michael,

Thanks for the patch. Looks mostly good, some small remarks inside.

On Mon, Feb 20, 2023 at 10:06:14AM +0100, Michael Kopfensteiner wrote:
> The Variscite DT8MCustomBoard is an eval board for the several Variscite
> SOMs of their "DART" product line. This commit adds support for that
> baseboard in combination with a DART-MX8M-PLUS SOM. The commit contains
> an adapted version of the vendors device tree [1] and the vendors DDR
> timings, taken from Variscite's public U-Boot sources [2].
> 
> Both files have been slightly changed to integrate well with barebox.
> 
> The boardsupport added with this commit does not yet support every
> feature of the DT8MCustomBoard. Yet it already supports all basic
> necessities to make use of barebox.
> 
> [1] 
> https://github.com/varigit/linux-imx/tree/3f94f35bda827e8aa06beadb10c77358cfb6dad9
> [2] 
> https://github.com/varigit/uboot-imx/tree/7cad2ff68a508c71c572151a85bc786711bab969
> 
> Signed-off-by: Michael Kopfensteiner <[email protected]>
> ---
>  arch/arm/boards/Makefile                      |    1 +
>  .../variscite-dt8mcustomboard-imx8mp/Makefile |    5 +
>  .../variscite-dt8mcustomboard-imx8mp/board.c  |   91 ++
>  .../init/automount                            |   19 +
>  .../flash-header-imx8mp-dart.imxcfg           |    7 +
>  .../lowlevel.c                                |  138 ++
>  .../lpddr4-timing.c                           | 1128 +++++++++++++++++
>  arch/arm/configs/imx_v8_defconfig             |    2 +
>  ...variscite_dt8mcustomboard_imx8mp_defconfig |  143 +++
>  arch/arm/dts/Makefile                         |    1 +
>  .../dts/imx8mp-var-dart-dt8mcustomboard.dts   |  680 ++++++++++
>  arch/arm/dts/imx8mp-var-dart.dtsi             |  395 ++++++
>  arch/arm/mach-imx/Kconfig                     |   10 +
>  images/Makefile.imx                           |    5 +
>  14 files changed, 2625 insertions(+)
>  create mode 100644 arch/arm/boards/variscite-dt8mcustomboard-imx8mp/Makefile
>  create mode 100644 arch/arm/boards/variscite-dt8mcustomboard-imx8mp/board.c
>  create mode 100644 
> arch/arm/boards/variscite-dt8mcustomboard-imx8mp/defaultenv-variscite-imx8mp-dart/init/automount
>  create mode 100644 
> arch/arm/boards/variscite-dt8mcustomboard-imx8mp/flash-header-imx8mp-dart.imxcfg
>  create mode 100644 
> arch/arm/boards/variscite-dt8mcustomboard-imx8mp/lowlevel.c
>  create mode 100644 
> arch/arm/boards/variscite-dt8mcustomboard-imx8mp/lpddr4-timing.c
>  create mode 100644 
> arch/arm/configs/variscite_dt8mcustomboard_imx8mp_defconfig
>  create mode 100644 arch/arm/dts/imx8mp-var-dart-dt8mcustomboard.dts
>  create mode 100644 arch/arm/dts/imx8mp-var-dart.dtsi
> 
> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
> index f92094d15b..671f07c7bc 100644
> --- a/arch/arm/boards/Makefile
> +++ b/arch/arm/boards/Makefile
> @@ -200,3 +200,4 @@ obj-$(CONFIG_MACH_RK3568_EVB)                     += 
> rockchip-rk3568-evb/
>  obj-$(CONFIG_MACH_RK3568_BPI_R2PRO)                  += 
> rockchip-rk3568-bpi-r2pro/
>  obj-$(CONFIG_MACH_PINE64_QUARTZ64)           += pine64-quartz64/
>  obj-$(CONFIG_MACH_RADXA_ROCK3)                       += radxa-rock3/
> +obj-$(CONFIG_MACH_VARISCITE_DT8MCUSTOMBOARD_IMX8MP)  += 
> variscite-dt8mcustomboard-imx8mp/
> diff --git a/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/Makefile 
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/Makefile
> new file mode 100644
> index 0000000000..c7bf14d146
> --- /dev/null
> +++ b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-y += board.o
> +lwl-y += lowlevel.o lpddr4-timing.o
> +bbenv-y += defaultenv-variscite-imx8mp-dart
> diff --git a/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/board.c 
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/board.c
> new file mode 100644
> index 0000000000..a6ef1bb7c9
> --- /dev/null
> +++ b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/board.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Michael Kopfensteiner, VAHLE Automation GmbH
> + */
> +
> +#include <asm/memory.h>
> +#include <bootsource.h>
> +#include <common.h>
> +#include <deep-probe.h>
> +#include <init.h>
> +#include <linux/phy.h>
> +#include <linux/sizes.h>
> +#include <mach/bbu.h>
> +#include <mach/iomux-mx8mp.h>
> +#include <gpio.h>
> +#include <envfs.h>
> +
> +#define PHY_ID_ADIN1300              0x0283bc30
> +#define PHY_ID_MODEL_MASK    0xfffffff0
> +
> +/*
> + * This fixup is necessary to properly configure the ADIN1300
> + * PHY on the SOM to properly communicate using RGMII.
> + * This fixup disables the PHY's internal 2ns RGMII receive clock
> + * delay. Without this configuration change, the system will
> + * be able to send Ethernet packages, but the MAC won't receive
> + * any response packages.
> + *
> + * This fixup is specific to the ADIN1300 PHY. This implementation
> + * was ported from Variscite's U-Boot sources.
> + */
> +static int phy_fixup_adin1300(struct phy_device *dev) {
> +     int ret;
> +
> +     pr_debug("BOARD: applying PHY fixup for ADIN1300\n");
> +
> +     ret = mdiobus_write(dev->bus, dev->addr, 0x0010, 0xFF23);
> +     if (ret) {
> +             pr_warn("ADIN1300 PHY fixup: failed to write EXT_REG_PTR\n");
> +             return ret;
> +     }
> +
> +     ret = mdiobus_write(dev->bus, dev->addr, 0x0011, 0x0E01);
> +     if (ret)  {
> +             pr_warn("ADIN1300 PHY fixup: failed to write EXT_REG_DATA\n");
> +             return ret;
> +     }
> +
> +     return 0;
> +}
> +
> +static int var_imx8mp_dart_cb_probe(struct device_d *dev)
> +{
> +     int emmc_bbu_flag = 0;
> +     int sd_bbu_flag = 0;
> +
> +     phy_register_fixup(PHY_ANY_ID, PHY_ID_ADIN1300, PHY_ID_MODEL_MASK, 
> phy_fixup_adin1300);
> +
> +     if (bootsource_get() == BOOTSOURCE_MMC) {
> +             if (bootsource_get_instance() == 2) {
> +                     of_device_enable_path("/chosen/environment-emmc");
> +                     emmc_bbu_flag = BBU_HANDLER_FLAG_DEFAULT;
> +             } else {
> +                     of_device_enable_path("/chosen/environment-sd");
> +                     sd_bbu_flag = BBU_HANDLER_FLAG_DEFAULT;
> +             }
> +     } else {
> +             of_device_enable_path("/chosen/environment-emmc");
> +             emmc_bbu_flag = BBU_HANDLER_FLAG_DEFAULT;
> +     }

This is equivalent to:

        if (bootsource_get() == BOOTSOURCE_MMC && bootsource_get_instance() != 
2) {
                of_device_enable_path("/chosen/environment-sd");
                sd_bbu_flag = BBU_HANDLER_FLAG_DEFAULT;
        } else {
                of_device_enable_path("/chosen/environment-emmc");
                emmc_bbu_flag = BBU_HANDLER_FLAG_DEFAULT;
        }

This has a bit less duplication. Rather than excluding the non-eMMC
devices I think you should include the right one, i.e. write
"bootsource_get_instance() == x"

> +
> +     imx8m_bbu_internal_mmc_register_handler("SD", "/dev/mmc1.barebox", 
> sd_bbu_flag);
> +     imx8m_bbu_internal_mmcboot_register_handler("eMMC", "/dev/mmc2", 
> emmc_bbu_flag);
> +
> +     defaultenv_append_directory(defaultenv_variscite_imx8mp_dart);
> +
> +     return 0;
> +}
> +
> +static const struct of_device_id var_imx8mp_dart_cb_of_match[] = {
> +     { .compatible = "variscite,imx8mp-var-dart" },
> +     { /* Sentinel */ }
> +};
> +BAREBOX_DEEP_PROBE_ENABLE(var_imx8mp_dart_cb_of_match);
> +
> +static struct driver_d var_imx8mp_dart_cb_board_driver = {
> +     .name = "board-var-imx8mp-dart-cb",
> +     .probe = var_imx8mp_dart_cb_probe,
> +     .of_compatible = var_imx8mp_dart_cb_of_match,
> +};
> +coredevice_platform_driver(var_imx8mp_dart_cb_board_driver);
> diff --git 
> a/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/defaultenv-variscite-imx8mp-dart/init/automount
>  
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/defaultenv-variscite-imx8mp-dart/init/automount
> new file mode 100644
> index 0000000000..4c084a9fb8
> --- /dev/null
> +++ 
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/defaultenv-variscite-imx8mp-dart/init/automount
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +
> +# automount tftp server
> +
> +mkdir -p /mnt/tftp
> +automount /mnt/tftp 'ifup -a1 && mount -t tftp $global.net.server /mnt/tftp'
> +
> +# automount nfs server's nfsroot
> +
> +mkdir -p /mnt/nfs
> +automount /mnt/nfs 'ifup -a1 && mount -t nfs 
> ${global.net.server}:/home/${global.user}/nfsroot/${global.hostname} /mnt/nfs'
> +
> +# detect and (automatically) prepare automounts for the internal eMMC
> +detect mmc2
> +
> +# FAT on usb disk example
> +
> +#mkdir -p /mnt/fat
> +#automount -d /mnt/fat 'usb && [ -e /dev/disk0.0 ] && mount /dev/disk0.0 
> /mnt/fat'

Is this file needed? What's the difference to the file provided in
defaultenv/defaultenv-2-base/init/automount?

> diff --git 
> a/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/flash-header-imx8mp-dart.imxcfg
>  
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/flash-header-imx8mp-dart.imxcfg
> new file mode 100644
> index 0000000000..2b104edfb7
> --- /dev/null
> +++ 
> b/arch/arm/boards/variscite-dt8mcustomboard-imx8mp/flash-header-imx8mp-dart.imxcfg
> +     imx8mp_setup_pad(MX8MP_PAD_I2C1_SDA__I2C1_SDA | I2C_PAD_CTRL);
> +
> +     imx8mm_early_clock_init();
> +     imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_I2C1);
> +
> +     i2c = imx8m_i2c_early_init(IOMEM(MX8MP_I2C1_BASE_ADDR));
> +
> +     pmic_configure(i2c, 0x25, pca9450_cfg, ARRAY_SIZE(pca9450_cfg));
> +}
> +
> +extern struct dram_timing_info dram_timing;

Please add some board specific prefix to the name. Otherwise we'll get
linker errors when more than one board is compiled in.

> +
> +static void start_atf(void)
> +{
> +     /*
> +      * If we are in EL3 we are running for the first time and need to
> +      * initialize the DRAM and run TF-A (BL31). The TF-A will then jump
> +      * to DRAM in EL2.
> +      */
> +     if (current_el() != 3)
> +             return;
> +
> +     power_init_board();
> +
> +     imx8mp_ddr_init(&dram_timing, DRAM_TYPE_LPDDR4);
> +
> +     imx8mp_load_and_start_image_via_tfa();
> +}
> +
> +static __noreturn noinline void variscite_imx8mp_dart_cb_start(void)
> +{
> +     if (IS_ENABLED(CONFIG_DEBUG_LL))
> +         setup_uart();

setup_uart() calls pbl_set_putc() which makes it useful for regular use,
not only CONFIG_DEBUG_LL. I would just call this unconditionally.

> +
> +     start_atf();
> +
> +     /*
> +      * Standard entry we hit once we initialized both DDR and ATF
> +      */
> +     imx8mp_barebox_entry(__dtb_z_imx8mp_var_dart_dt8mcustomboard_start);
> +}
> +
> +/*
> + * Power-on execution flow of nxp_imx8mp_vardart_start() might not be
> + * obvious for a very first read, so here's, hopefully helpful,
> + * summary:
> + *
> + * 1. MaskROM uploads PBL into OCRAM and that's where this function is
> + *    executed for the first time. At entry the exception level is EL3.
> + *
> + * 2. DDR is initialized and the image is loaded from storage into DRAM. The 
> PBL
> + *    part is copied from OCRAM to the TF-A return address in DRAM.
> + *
> + * 3. TF-A is executed and exits into the PBL code in DRAM. TF-A has taken us
> + *    from EL3 to EL2.
> + *
> + * 4. Standard barebox boot flow continues
> + */
> +ENTRY_FUNCTION(start_variscite_imx8mp_dart, r0, r1, r2)
> +{
> +     imx8mp_cpu_lowlevel_init();
> +     relocate_to_current_adr();
> +     setup_c();
> +
> +     IMD_USED_OF(imx8mp_var_dart_dt8mcustomboard);
> +
> +     variscite_imx8mp_dart_cb_start();
> +}
> diff --git a/arch/arm/configs/variscite_dt8mcustomboard_imx8mp_defconfig 
> b/arch/arm/configs/variscite_dt8mcustomboard_imx8mp_defconfig

No board specific defconfigs please.

> diff --git a/arch/arm/dts/imx8mp-var-dart-dt8mcustomboard.dts 
> b/arch/arm/dts/imx8mp-var-dart-dt8mcustomboard.dts
> new file mode 100644
> index 0000000000..704289aa0b
> --- /dev/null
> +++ b/arch/arm/dts/imx8mp-var-dart-dt8mcustomboard.dts
> @@ -0,0 +1,680 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2019 NXP
> + * Copyright 2020-2021 Variscite Ltd.
> + * Copyright 2023 VAHLE Automation GmbH
> + */
> +
> +#include "imx8mp-var-dart.dtsi"
> +
> +/ {
> +     model = "Variscite DART-MX8M-PLUS on DT8MCustomBoard 2.x";
> +
> +     // use the memory controller instead
> +     /delete-node/ memory@40000000;

Better don't add this node to the dtsi file. Is the memory size detected
correctly? The dtsi file describes 6 GiB of memory which is rather
unusual. Is that what your board has?

> +&edacmc {
> +     status = "okay";
> +};

You can drop this. It's already enabled in imx8mp.dtsi.

> +&i2c2 {
> +     clock-frequency = <400000>;
> +     pinctrl-names = "default", "gpio";
> +     pinctrl-0 = <&pinctrl_i2c2>;
> +     pinctrl-1 = <&pinctrl_i2c2_gpio>;
> +     scl-gpios = <&gpio5 16 GPIO_ACTIVE_HIGH>;
> +     sda-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> +     status = "okay";
> +
> +     typec@3d {
> +             compatible = "nxp,ptn5150";
> +             pinctrl-names = "default";
> +             pinctrl-0 = <&pinctrl_extcon>;
> +             reg = <0x3d>;
> +             interrupt-parent = <&gpio1>;
> +             interrupts = <10 IRQ_TYPE_LEVEL_HIGH>;
> +             irq-is-id-quirk;
> +             status ="okay";

status = "okay" is unnecessary for newly added nodes as it's the default.

> +
> +             port {
> +                     typec_dr_sw: endpoint {
> +                             remote-endpoint = <&usb3_drd_sw>;
> +                     };
> +             };
> +     };
> +
> +     /* DS1337 RTC module */
> +     rtc@68 {
> +             compatible = "dallas,ds1337";
> +             reg = <0x68>;
> +             pinctrl-names = "default";
> +             pinctrl-0 = <&pinctrl_rtc>;
> +             interrupt-parent = <&gpio1>;
> +             interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> +             wakeup-source;
> +             status = "okay";

dito

> +     };
> +
> +     /* Capacitive touch controller */
> +     ft5x06_ts: ft5x06_ts@38 {
> +             compatible = "edt,edt-ft5206";
> +             reg = <0x38>;
> +             pinctrl-names = "default";
> +             pinctrl-0 = <&pinctrl_captouch>;
> +             reset-gpios = <&pca6408_2 4 GPIO_ACTIVE_LOW>;
> +             interrupt-parent = <&gpio1>;
> +             interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
> +             touchscreen-size-x = <800>;
> +             touchscreen-size-y = <480>;
> +             touchscreen-inverted-x;
> +             touchscreen-inverted-y;
> +             wakeup-source;
> +             status = "okay";

dito, found in other places as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Reply via email to