[PATCH 37/37] arm: imx: add i.MX8ULP EVK support

[Fabio Estevam]

Hi Peng,

Thanks for submitting this series.

On Mon, Apr 12, 2021 at 8:43 AM Peng Fan (OSS) <peng.fan at oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan at nxp.com>
>
> Add i.MX8ULP EVK basic support, support SD/I2C/ENET/LPUART
>
> Note: upower API currently not included.
>
> Signed-off-by: Peng Fan <peng.fan at nxp.com>
> ---
>  arch/arm/dts/imx8ulp-emulator-u-boot.dtsi   |   32 +
>  arch/arm/dts/imx8ulp-emulator.dts           |   93 +

These emulator-related files can be dropped as they are only for
internal NXP usage.

>  arch/arm/dts/imx8ulp-evk-u-boot.dtsi        |   32 +
>  arch/arm/dts/imx8ulp-evk.dts                |  204 +++
>  arch/arm/mach-imx/imx8ulp/Kconfig           |    7 +
>  board/freescale/imx8ulp_evk/Kconfig         |   14 +
>  board/freescale/imx8ulp_evk/MAINTAINERS     |    6 +
>  board/freescale/imx8ulp_evk/Makefile        |    7 +
>  board/freescale/imx8ulp_evk/ddr_init.c      |  207 +++
>  board/freescale/imx8ulp_evk/imx8ulp_evk.c   |   67 +
>  board/freescale/imx8ulp_evk/lpddr4_timing.c | 1696 +++++++++++++++++++
>  board/freescale/imx8ulp_evk/spl.c           |  146 ++
>  configs/imx8ulp_evk_defconfig               |  103 ++
>  include/configs/imx8ulp_evk.h               |  108 ++
>  14 files changed, 2722 insertions(+)
>  create mode 100644 arch/arm/dts/imx8ulp-emulator-u-boot.dtsi
>  create mode 100644 arch/arm/dts/imx8ulp-emulator.dts
>  create mode 100644 arch/arm/dts/imx8ulp-evk-u-boot.dtsi
>  create mode 100644 arch/arm/dts/imx8ulp-evk.dts
>  create mode 100644 board/freescale/imx8ulp_evk/Kconfig
>  create mode 100644 board/freescale/imx8ulp_evk/MAINTAINERS
>  create mode 100644 board/freescale/imx8ulp_evk/Makefile
>  create mode 100644 board/freescale/imx8ulp_evk/ddr_init.c
>  create mode 100644 board/freescale/imx8ulp_evk/imx8ulp_evk.c
>  create mode 100644 board/freescale/imx8ulp_evk/lpddr4_timing.c
>  create mode 100644 board/freescale/imx8ulp_evk/spl.c

Please add a readme file that explains how the final binary is
generated, the various required firmware files, etc.

I understand that some of the git repo's may not be public at the
moment, but you could put a "TBD" in the git URL for now.

Better to put the read me now, so that when someone wants to test
this, the build instructions are available.

> +       /* Check PLL lock status */
> +       lock_0 = readl(DENALI_PHY_1564) & 0xffff;
> +       lock_1 = (readl(DENALI_PHY_1564) >> 16) & 0xffff;
> +       lock_2 = readl(DENALI_PHY_1565) & 0xffff;
> +
> +       if ((lock_0 & 0x3) != 0x3 || (lock_1 & 0x3) != 0x3 || (lock_2 & 0x3) != 0x3) {
> +               printf("De-Skew PLL failed to lock\n");
> +               printf("lock_0=0x%x, lock_1=0x%x, lock_2=0x%x\n", lock_0, lock_1, lock_2);
> +               return -1;

-1 is not an appropriate return value. What about returning -ETIMEDOUT?

Also, shouldn't you wait for some time period until the lock the PLL
gets locked?

> +void enable_bypass_modeenable_bypass_mode(void)

This function is not used anywhere. Please remove it.


> +int board_early_init_f(void)
> +{
> +       return 0;
> +}

Better drop this function and its Kconfig option if you don't need it.

> +
> +int board_late_init(void)
> +{
> +       return 0;

Better drop this function and its Kconfig option if you don't need it.

> +}
> diff --git a/board/freescale/imx8ulp_evk/lpddr4_timing.c b/board/freescale/imx8ulp_evk/lpddr4_timing.c
> new file mode 100644
> index 0000000000..e827f01cd7

>
> +static void xrdc_configure_mda(void)
> +{
> +       ulong xrdc_base = 0x292f0000, off;
> +       u32 i = 0;

No need to initialize i to 0.

> +
> +       /* Set MDA3-5 for PXP, ENET, CAAM to DID 1*/
> +       for (i = 3; i <= 5; i++) {
> +               off = 0x800 + i * 0x20;
> +               writel(0x200000A1, xrdc_base + off);
> +               writel(0xA00000A1, xrdc_base + off);
> +       }


> +++ b/configs/imx8ulp_evk_defconfig
> @@ -0,0 +1,103 @@
> +CONFIG_ARM=y
> +CONFIG_ARCH_IMX8ULP=y
> +CONFIG_SYS_TEXT_BASE=0x80200000
> +CONFIG_SYS_MALLOC_F_LEN=0x2000
> +CONFIG_DM_GPIO=y
> +CONFIG_TARGET_IMX8ULP_EVK=y
> +CONFIG_SPL_SERIAL_SUPPORT=y
> +CONFIG_NR_DRAM_BANKS=1
> +CONFIG_SPL=y
> +CONFIG_SPL_TEXT_BASE=0x22020000
> +CONFIG_SPL_LOAD_IMX_CONTAINER=y
> +CONFIG_FIT=y
> +CONFIG_FIT_VERBOSE=y
> +CONFIG_BOOTDELAY=0

This is not developer-friendly. Please remove

> +CONFIG_DEFAULT_FDT_FILE="imx8ulp-evk.dtb"
> +CONFIG_SPL_BOARD_INIT=y
> +CONFIG_SPL_SEPARATE_BSS=y
> +#CONFIG_SPL_RAM_SUPPORT=y
> +#CONFIG_SPL_RAM_DEVICE=y
> +#CONFIG_SPL_MMC_SUPPORT=y

This is not the correct way to disable a Kconfig symbol.

Please re-generate this using make savedefconfig.

 +CONFIG_SPL_SYS_ICACHE_OFF=y
> +CONFIG_SPL_SYS_DCACHE_OFF=y

Why are the caches turned off?

> +CONFIG_SPL_DRIVERS_MISC_SUPPORT=y
> +CONFIG_MISC=y
> +
> +CONFIG_CMD_I2C=y
> +CONFIG_CMD_FUSE=y
> +CONFIG_CMD_GPIO=y
> +CONFIG_CMD_MEMTEST=y
> +CONFIG_CMD_EXT2=y
> +CONFIG_CMD_EXT4=y
> +CONFIG_CMD_EXT4_WRITE=y
> +CONFIG_CMD_FAT=y
> +CONFIG_CMD_CACHE=y
> +CONFIG_CMD_REGULATOR=y
> +CONFIG_BAUDRATE=115200
> +
> +CONFIG_DM_REGULATOR=y
> +CONFIG_DM_REGULATOR_FIXED=y
> +CONFIG_DM_REGULATOR_GPIO=y

> +#define CONFIG_SERIAL_TAG

Do you need this?

> +#define CONFIG_REMAKE_ELF
> +
> +#define CONFIG_BOARD_EARLY_INIT_F
> +#define CONFIG_BOARD_LATE_INIT

This is a Kconfig symbol now.

> +
> +/* ENET Config */
> +#if defined(CONFIG_FEC_MXC)
> +#define CONFIG_ETHPRIME                 "FEC"
> +#define PHY_ANEG_TIMEOUT               20000
> +
> +#define CONFIG_FEC_XCV_TYPE            RMII
> +#define CONFIG_FEC_MXC_PHYADDR         1
> +#define FEC_QUIRK_ENET_MAC
> +
> +#define IMX_FEC_BASE                   0x29950000

With DM you don't need these.