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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

Hi Fabio,

On Fri, Jul 12, 2019 at 08:37:24AM -0300, Fabio Estevam wrote:
> Hi Manivannan,
> 
> On Thu, Jul 11, 2019 at 3:07 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > This commit adds initial board support for iMXQXP AI_ML board from
> 
> iMXQXP --> i.MX8QXP
> 

Ack.

> 
> > 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
> 

Ack.

> > --- /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...
> 

Oops, this should be "Get and Build the ARM Trusted firmware"

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

Copy paste clutter. Will remove it.

> > +}
> > +
> > +/*
> > + * Board specific reset that is system reset.
> > + */
> 
> You could use single line comment style instead.
> 

Ack.

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

Yeah, will remove it.

> > +CONFIG_ARM=y
> > +CONFIG_SPL_SYS_ICACHE_OFF=y
> > +CONFIG_SPL_SYS_DCACHE_OFF=y
> 
> Why do you turn off the caches?
> 

This also slipped when copying from mek board. Will remove.

> > +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?
> 

Ideally it should but the driver is not matured enough to retrieve address
from DT. Currently it relies on the Kconfig symbol, hence it needs to be
present till the driver is cleaned up.

> > +/* 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.
> 

Actually these defines are not needed. Will remove.

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

Ack.

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

Agree. Initially I thought of not using it since the most of imx8 boards are
not using distro_boot, but anyway it doesn't hurt to do so.

Thanks,
Mani