[U-Boot] [PATCH v2 2/2] board: Add support for iMXQXP AI_ML board

[Fabio Estevam]

Hi Manivannan,

On Thu, Jul 11, 2019 at 3:07 PM Manivannan Sadhasivam
<manivannan.sadhasivam at linaro.org> wrote:
>
> This commit adds initial board support for iMXQXP AI_ML board from

iMXQXP --> i.MX8QXP


> Einfochips. This board is one of the 96Boards Consumer Edition and AI boards
> of the 96Boards family based on i.MXQXP SoC from NXP/Freescale.

i.MXQXP -> i.MX8QXP

> --- /dev/null
> +++ b/board/einfochips/imx8qxp_ai_ml/README
> @@ -0,0 +1,49 @@
> +U-Boot for the Einfochips i.MX8QXP AI_ML board
> +
> +Quick Start
> +===========
> +
> +- Build the ARM Trusted firmware binary

The first instruction here is to build the ATF...

> +- Get scfw_tcm.bin and ahab-container.img
> +- Build U-Boot
> +- Flash the binary into the SD card
> +- Boot
> +
> +Get and Build the ARM Trusted firmware
> +======================================
> +
> +$ git clone https://source.codeaurora.org/external/imx/imx-atf

and later it is described how to get the ATF.

Looks like the order needs to be inverted.

> +void detail_board_ddr_info(void)
> +{
> +       puts("\nDDR    ");

Is this function really useful as its only purpose is to print "DDR" ?

> +}
> +
> +/*
> + * Board specific reset that is system reset.
> + */

You could use single line comment style instead.

> +#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);

It seems you don't need this function then.

> +CONFIG_ARM=y
> +CONFIG_SPL_SYS_ICACHE_OFF=y
> +CONFIG_SPL_SYS_DCACHE_OFF=y

Why do you turn off the caches?

> +CONFIG_PHY_GIGE=y
> +CONFIG_FEC_MXC_SHARE_MDIO=y
> +CONFIG_FEC_MXC_MDIO_BASE=0x5B040000

Just wondering why CONFIG_FEC_MXC_MDIO_BASE is set in a board config file?

Shouldn't this base address be retrieved from device tree?

> +/* Flat Device Tree Definitions */
> +#define CONFIG_OF_BOARD_SETUP
> +
> +#define CONFIG_FSL_ESDHC
> +#define CONFIG_FSL_USDHC
> +#define CONFIG_SYS_FSL_ESDHC_ADDR       0
> +#define USDHC1_BASE_ADDR                0x5B010000
> +#define USDHC2_BASE_ADDR                0x5B020000

These base addresses should not be needed as they can be retrieved
from device tree.

> +#define CONFIG_ENV_OVERWRITE
> +
> +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
> +
> +/* Initial environment variables */
> +#define CONFIG_EXTRA_ENV_SETTINGS              \
> +       "script=boot.scr\0" \
> +       "image=Image\0" \
> +       "panel=NULL\0" \

If you don't need the variable 'panel', then there is no need to define it.

> +#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"

You may consider to use distro_boot for this community board. It makes
easier for Linux distros to support it.