Re: [U-Boot] [PATCH V2 51/51] imx: add i.MX8MN DDR4 board support

Subject: Re: [U-Boot] [PATCH V2 51/51] imx: add i.MX8MN DDR4 board support

[...]

...

MX8MM_IOMUXC_NAND_CE1_B_USDHC3_STROBE 0x190

Those changes looks identical. Why do you need to change those lines? (Wrong formatting) ?

Wrongly added. Drop in V3.

...

>;

+};

[...]

...

diff --git a/arch/arm/dts/imx8mn-ddr4-evk.dts b/arch/arm/dts/imx8mn-ddr4-evk.dts new file mode 100644 index 0000000000..9b2c1727a8 --- /dev/null +++ b/arch/arm/dts/imx8mn-ddr4-evk.dts

DTS files shall be added in a separate commit, to avoid one very large one when you add the board.

Fix in V3.

...

@@ -0,0 +1,221 @@

• { 0x3d400498, 0x03ff0000 },
• { 0x3d40049c, 0x01000e00 },
• { 0x3d4004a0, 0x03ff0000 },

+};

Isn't there auto training code for i.MX8 as it is for i.MX6Q?

For example: https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-imx/mx6 /ddr.c https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-imx/ddr mc-vf610-calibration.c

The DDR FW will do auto training. The DDR controller is complicated and different from i.MX6.

...

+/* PHY Initialize Configuration */ +struct dram_cfg_param ddr_ddrphy_cfg[] = { +};

[....]

...

The list of binary data is quite impressive for i.MX8

There are many many many registers in the DDRC in i.MX8M needs to configured.

...

+struct dram_fsp_msg ddr_dram_fsp_msg[] = {

[..]

...

+{

• struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
• imx_iomux_v3_setup_multiple_pads(wdog_pads,

Shouldn't those pins be configured via DTS/DM? As fair as I see this is the imx8mn_evk.c file (which seems not to be the SPL).

DM WDOG is not enabled here. Just use reset_cpu to do external reset

...

ARRAY_SIZE(wdog_pads)); +

• set_wdog_reset(wdog);
• imx_iomux_v3_setup_multiple_pads(uart_pads,

ARRAY_SIZE(uart_pads)); +

• init_uart_clk(1);

This also seems like not DM/DTS code.

We not enable DM UART.

...

• return 0;

+}

+#ifdef CONFIG_BOARD_POSTCLK_INIT +int board_postclk_init(void) +{

• /* TODO */
• return 0;

+} +#endif

+int dram_init(void) +{

• /* rom_pointer[1] contains the size of TEE occupies */
• if (rom_pointer[1])

• gd->ram_size = PHYS_SDRAM_SIZE - rom_pointer[1];
  

• else

• gd->ram_size = PHYS_SDRAM_SIZE;
  

• return 0;

+}

+#ifdef CONFIG_OF_BOARD_SETUP +int ft_board_setup(void *blob, bd_t *bd) +{

• return 0;

+} +#endif

+int board_init(void) +{

• return 0;

+}

+int board_mmc_get_env_dev(int devno) +{

• return devno - 1;

+}

Shouldn't the number of mmc device be get via alias defined in DTS (or assigned automatically)?

What is the purpose of this function?

mmc_get_env_dev will call this function. devno is the controller index inside SoC, however not every controller is enabled in a board.

...

+int board_late_init(void) +{ +#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG

• env_set("board_name", "DDR4 EVK");
• env_set("board_rev", "iMX8MN");

+#endif

• return 0;

+} diff --git a/board/freescale/imx8mn_evk/spl.c b/board/freescale/imx8mn_evk/spl.c new file mode 100644 index 0000000000..aa5f37fde0 --- /dev/null +++ b/board/freescale/imx8mn_evk/spl.c @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2019 NXP
• */

+#include <common.h> +#include <spl.h> +#include <asm/io.h> +#include <errno.h> +#include <asm/arch/imx8mn_pins.h> +#include <asm/arch/sys_proto.h> +#include <asm/arch/clock.h> +#include <asm/gpio.h> +#include <asm/arch/ddr.h> +#include <asm/mach-imx/boot_mode.h> +#include <asm/mach-imx/gpio.h> +#include <asm/mach-imx/iomux-v3.h> +#include <asm/mach-imx/mxc_i2c.h>

+#include <dm/uclass.h> +#include <dm/device.h> +#include <dm/uclass-internal.h> +#include <dm/device-internal.h>

+DECLARE_GLOBAL_DATA_PTR;

+int spl_board_boot_device(enum boot_device boot_dev_spl) +{

• return BOOT_DEVICE_BOOTROM;

+}

+void spl_dram_init(void) +{

• ddr_init(&dram_timing);

+}

+void spl_board_init(void) +{

• struct udevice *dev;
• puts("Normal Boot\n");
• uclass_find_first_device(UCLASS_CLK, &dev);
• for (; dev; uclass_find_next_device(&dev)) {

• if (device_probe(dev))
  

• 	continue;
  

Is this correct that in the SPL you call probe on all UCLASS_CLK devices registered for i.MX8?

Any issue you forseen?

...

• }

+}

+#ifdef CONFIG_SPL_LOAD_FIT +int board_fit_config_name_match(const char *name) +{

• /* Just empty function now - can't decide what to choose */
• debug("%s: %s\n", __func__, name);
• return 0;

+} +#endif

+void board_init_f(ulong dummy) +{

• int ret;
• arch_cpu_init();
• init_uart_clk(1);
• board_early_init_f();
• timer_init();
• preloader_console_init();
• /* Clear the BSS. */
• memset(__bss_start, 0, __bss_end - __bss_start);
• ret = spl_init();
• if (ret) {

• debug("spl_init() failed: %d\n", ret);
  

• hang();
  

• }
• enable_tzc380();
• /* DDR initialization */
• spl_dram_init();
• board_init_r(NULL, 0);

+} diff --git a/configs/imx8mn_ddr4_evk_defconfig b/configs/imx8mn_ddr4_evk_defconfig new file mode 100644 index 0000000000..01e10fc427 --- /dev/null +++ b/configs/imx8mn_ddr4_evk_defconfig @@ -0,0 +1,53 @@ +CONFIG_ARM=y +CONFIG_ARCH_IMX8M=y +CONFIG_SYS_TEXT_BASE=0x40200000 +CONFIG_SPL_GPIO_SUPPORT=y +CONFIG_SPL_LIBCOMMON_SUPPORT=y +CONFIG_SPL_LIBGENERIC_SUPPORT=y +CONFIG_SYS_MALLOC_F_LEN=0x6000 +CONFIG_TARGET_IMX8MN_EVK=y +CONFIG_SPL_SERIAL_SUPPORT=y +CONFIG_SPL=y +CONFIG_IMX_ROMAPI_LOADADDR=0x48000000 +CONFIG_FIT=y +CONFIG_FIT_EXTERNAL_OFFSET=0x3000 +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"

+CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=arch/arm/mach-imx/imx8m /imximage-8mn-ddr4.cfg"

...

+CONFIG_DEFAULT_FDT_FILE="fsl-imx8mn-ddr4-evk.dtb" +CONFIG_SPL_TEXT_BASE=0x912000 +CONFIG_SPL_BOARD_INIT=y +CONFIG_SPL_BOOTROM_SUPPORT=y +CONFIG_SPL_SEPARATE_BSS=y +CONFIG_SPL_I2C_SUPPORT=y +CONFIG_HUSH_PARSER=y +CONFIG_CMD_CLK=y +CONFIG_CMD_GPIO=y +CONFIG_CMD_I2C=y +CONFIG_CMD_CACHE=y +CONFIG_CMD_REGULATOR=y +CONFIG_CMD_EXT2=y +CONFIG_CMD_EXT4=y +CONFIG_CMD_EXT4_WRITE=y +CONFIG_CMD_FAT=y +CONFIG_OF_CONTROL=y +CONFIG_SPL_OF_CONTROL=y +CONFIG_DEFAULT_DEVICE_TREE="imx8mm-evk" +CONFIG_SPL_DM=y +CONFIG_SPL_CLK=y +CONFIG_CLK_IMX8MN=y +CONFIG_DM_GPIO=y +CONFIG_MXC_GPIO=y +CONFIG_DM_I2C=y +CONFIG_SYS_I2C_MXC=y +CONFIG_DM_MMC=y +CONFIG_PHYLIB=y +CONFIG_DM_ETH=y +CONFIG_PINCTRL=y +CONFIG_SPL_PINCTRL=y +CONFIG_PINCTRL_IMX8M=y +CONFIG_DM_REGULATOR=y +CONFIG_DM_REGULATOR_FIXED=y +CONFIG_DM_REGULATOR_GPIO=y +CONFIG_MXC_UART=y +CONFIG_DM_THERMAL=y

You could also from the beginning enable CONFIG_WDT and CONFIG_SYSRESET (as i.MX6 already uses it).

I'll try.

...

diff --git a/include/configs/imx8mn_evk.h b/include/configs/imx8mn_evk.h new file mode 100644 index 0000000000..6ec8a2e362 --- /dev/null +++ b/include/configs/imx8mn_evk.h @@ -0,0 +1,198 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright 2018 NXP
• */

+#ifndef __IMX8MN_EVK_H +#define __IMX8MN_EVK_H

+#include <linux/sizes.h> +#include <asm/arch/imx-regs.h>

+#ifdef CONFIG_SECURE_BOOT +#define CONFIG_CSF_SIZE 0x2000 /* 8K region */ +#endif

+#define CONFIG_SPL_MAX_SIZE (148 * 1024) +#define CONFIG_SYS_MONITOR_LEN (512 * 1024) +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR +#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x300 +#define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 1 +#define CONFIG_SYS_UBOOT_BASE \

• (QSPI0_AMBA_BASE +

CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR *

...

+#ifdef CONFIG_SPL_BUILD +/*#define CONFIG_ENABLE_DDR_TRAINING_DEBUG*/ +#define CONFIG_SPL_WATCHDOG_SUPPORT +#define CONFIG_SPL_DRIVERS_MISC_SUPPORT +#define CONFIG_SPL_POWER_SUPPORT +#define CONFIG_SPL_LDSCRIPT "arch/arm/cpu/armv8/u-boot-spl.lds" +#define CONFIG_SPL_STACK 0x95fff0 +#define CONFIG_SPL_BSS_START_ADDR 0x00950000 +#define CONFIG_SPL_BSS_MAX_SIZE 0x2000 /* 8 KB */

Please fix globally -> replace 0x2000 -> SZ_8K from include/linux/sizes.h

Fix in V3.

...

+#define CONFIG_SYS_SPL_MALLOC_START 0x42200000 +#define CONFIG_SYS_SPL_MALLOC_SIZE 0x80000 /* 64 KB */ +#define CONFIG_SYS_ICACHE_OFF +#define CONFIG_SYS_DCACHE_OFF

+/* malloc f used before GD_FLG_FULL_MALLOC_INIT set */ +#define CONFIG_MALLOC_F_ADDR 0x00940000

+/* For RAW image gives a error info not panic */ +#define CONFIG_SPL_ABORT_ON_RAW_IMAGE

+#undef CONFIG_DM_MMC +#undef CONFIG_DM_PMIC +#undef CONFIG_DM_PMIC_PFUZE100

+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG

+#endif

+#define CONFIG_REMAKE_ELF

+#define CONFIG_BOARD_EARLY_INIT_F +#define CONFIG_BOARD_POSTCLK_INIT +#define CONFIG_BOARD_LATE_INIT

+/* Flat Device Tree Definitions */ +#define CONFIG_OF_BOARD_SETUP

+#undef CONFIG_CMD_EXPORTENV +#undef CONFIG_CMD_IMPORTENV +#undef CONFIG_CMD_IMLS

+#undef CONFIG_CMD_CRC32 +#undef CONFIG_BOOTM_NETBSD

+/* Initial environment variables */ +#define CONFIG_EXTRA_ENV_SETTINGS \

• "script=boot.scr\0" \
• "image=Image\0" \
• "console=ttymxc1,115200

earlycon=ec_imx6q,0x30890000,115200\0" \

• "fdt_addr=0x43000000\0" \
• "fdt_high=0xffffffffffffffff\0" \
• "boot_fdt=try\0" \
• "fdt_file=" CONFIG_DEFAULT_FDT_FILE "\0" \
• "initrd_addr=0x43800000\0" \
• "initrd_high=0xffffffffffffffff\0" \
• "mmcdev="__stringify(CONFIG_SYS_MMC_ENV_DEV)"\0" \
• "mmcpart=" __stringify(CONFIG_SYS_MMC_IMG_LOAD_PART) "\0" \
• "mmcroot=" CONFIG_MMCROOT " rootwait rw\0" \
• "mmcautodetect=yes\0" \
• "mmcargs=setenv bootargs console=${console}

root=${mmcroot}\0 " \

• "loadbootscript=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr}

${script};\0" \

• "bootscript=echo Running bootscript from mmc ...; " \

• "source\0" \
  

• "loadimage=fatload mmc ${mmcdev}:${mmcpart} ${loadaddr}

${image}\0" \

• "loadfdt=fatload mmc ${mmcdev}:${mmcpart} ${fdt_addr}

${fdt_file}\0" \

• "mmcboot=echo Booting from mmc ...; " \

• "run mmcargs; " \
  

• "if test ${boot_fdt} = yes || test ${boot_fdt} =
  

try; then " \

• 	"if run loadfdt; then " \
  

• 		"booti ${loadaddr} - ${fdt_addr}; " \
  

Please correct me if I'm wrong, but why do you use 'booti' (and presumably boot Image?).

Please use fitImage (in think that it is mature enough to be used for new ports - other imx SoCs use it already for some time);

It can be called as: bootm ${loadaddr}#conf@${fdt_conf}

No. We not switch to FIT when uboot booting kernel.

...

• 	"else " \
  

• 		"echo WARN: Cannot load the DT; " \
  

• 	"fi; " \
  

• "else " \
  

• 	"echo wait for boot; " \
  

• "fi;\0" \
  

• "netargs=setenv bootargs console=${console} " \

• "root=/dev/nfs " \
  

• "ip=dhcp nfsroot=${serverip}:${nfsroot},v3,tcp\0" \
  

• "netboot=echo Booting from net ...; " \

• "run netargs;  " \
  

• "if test ${ip_dyn} = yes; then " \
  

• 	"setenv get_cmd dhcp; " \
  

• "else " \
  

• 	"setenv get_cmd tftp; " \
  

• "fi; " \
  

• "${get_cmd} ${loadaddr} ${image}; " \
  

• "if test ${boot_fdt} = yes || test ${boot_fdt} =
  

try; then " \

• 	"if ${get_cmd} ${fdt_addr} ${fdt_file}; then
  

" \

• 		"booti ${loadaddr} - ${fdt_addr}; " \
  

• 	"else " \
  

• 		"echo WARN: Cannot load the DT; " \
  

• 	"fi; " \
  

• "else " \
  

• 	"booti; " \
  

• "fi;\0"
  

+#define CONFIG_BOOTCOMMAND \

•   "mmc dev ${mmcdev}; if mmc rescan; then " \
  

•    "if run loadbootscript; then " \
  

• 	   "run bootscript; " \
  

•    "else " \
  

• 	   "if run loadimage; then " \
  

• 		   "run mmcboot; " \
  

• 	   "else run netboot; " \
  

• 	   "fi; " \
  

•    "fi; " \
  

•   "else booti ${loadaddr} - ${fdt_addr}; fi"
  

+/* Link Definitions */ +#define CONFIG_LOADADDR 0x40480000

+#define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR

+#define CONFIG_SYS_INIT_RAM_ADDR 0x40000000 +#define CONFIG_SYS_INIT_RAM_SIZE 0x80000 +#define CONFIG_SYS_INIT_SP_OFFSET \

• (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE)

+#define CONFIG_SYS_INIT_SP_ADDR \

• (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET)

+#define CONFIG_ENV_OVERWRITE +#if defined(CONFIG_ENV_IS_IN_MMC) +#define CONFIG_ENV_OFFSET (64 * SZ_64K) +#endif +#define CONFIG_ENV_SIZE 0x1000 +#define CONFIG_SYS_MMC_ENV_DEV 0 /* USDHC2 */ +#define CONFIG_MMCROOT "/dev/mmcblk1p2" /* USDHC2 */ + +/* Size of malloc() pool */ +#define CONFIG_SYS_MALLOC_LEN SZ_32M

+#define CONFIG_SYS_SDRAM_BASE 0x40000000 +#define PHYS_SDRAM 0x40000000 +#define PHYS_SDRAM_SIZE 0x80000000 /* 2GB DDR */ + +#define CONFIG_SYS_MEMTEST_START PHYS_SDRAM +#define CONFIG_SYS_MEMTEST_END

(CONFIG_SYS_MEMTEST_START +

...

(PHYS_SDRAM_SIZE >> 1)) + +#define CONFIG_BAUDRATE 115200

+#define CONFIG_MXC_UART_BASE UART2_BASE_ADDR

+/* Monitor Command Prompt */ +#undef CONFIG_SYS_PROMPT +#define CONFIG_SYS_PROMPT "u-boot=> " +#define CONFIG_SYS_PROMPT_HUSH_PS2 "> " +#define CONFIG_SYS_CBSIZE 2048 +#define CONFIG_SYS_MAXARGS 64 +#define CONFIG_SYS_BARGSIZE CONFIG_SYS_CBSIZE +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \

• 			sizeof(CONFIG_SYS_PROMPT) +
  

+#define CONFIG_IMX_BOOTAUX

Has the i.MX8 the use case for IMX_BOOTAUX - if not please remove it.

i.MX8M use bootaux to kick M4

...

+/* USDHC */ +#define CONFIG_CMD_MMC +#define CONFIG_FSL_ESDHC +#define CONFIG_FSL_USDHC

+#define CONFIG_SYS_FSL_USDHC_NUM 2 +#define CONFIG_SYS_FSL_ESDHC_ADDR 0

Some of the above are in Kconfig.

...

+#define CONFIG_SUPPORT_EMMC_BOOT /* eMMC specific */ +#define CONFIG_SYS_MMC_IMG_LOAD_PART 1

+#define CONFIG_CMD_FUSE

+#define CONFIG_SYS_I2C_MXC_I2C1 /* enable I2C bus 1 */ +#define CONFIG_SYS_I2C_MXC_I2C2 /* enable I2C bus 2 */ +#define CONFIG_SYS_I2C_MXC_I2C3 /* enable I2C bus 3 */

...

+#define CONFIG_SYS_I2C_SPEED 100000

Those #defines (I2C related) are already supported in Kconfig. Please use them.

Please review above defines and move to Kconfig.

Sure.

Thanks, Peng.

...

+#define CONFIG_OF_SYSTEM_SETUP +#endif

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de