[U-Boot] [PATCH v3] imx: support i.MX8QM ROM 7720 a1 board

Tue Sep 3 01:22:22 UTC 2019

> Subject: Re: [PATCH v3] imx: support i.MX8QM ROM 7720 a1 board
> 
> On 02/09/19, Peng Fan wrote:
> > > Subject: [PATCH v3] imx: support i.MX8QM ROM 7720 a1 board
> > >
> > > +echo "ATF_LOAD_ADDR=$ATF_LOAD_ADDR" >&2 echo
> > > +"BL33_LOAD_ADDR=$BL33_LOAD_ADDR" >&2
> > > +
> >
> > This is no needed.
> >
> 
> ok I`ll remove it
> 
> > > +config SYS_VENDOR
> > > +	default "freescale"
> >
> > This board is not from nxp(freescale). Please correct vendor.
> 
> yes vendor is advantech, I'll change that in next version Then I need to move
> all files into the board/advantech folder?

Yes, please.

> >
> > > +Copy the following binaries to iMX8QM folder:
> > > +
> > > +$ cp imx-atf/build/imx8qm/release/bl31.bin .
> > > +$ cp u-boot/u-boot.bin .
> > > +
> > > +Copy the following firmwares U-Boot folder :
> > > +
> > > +$ cp firmware-imx-8.0/firmware/seco/mx8qm-ahab-container.img .
> > > +$ cp imx-sc-firmware-1.1/mx8qm-val-scfw-tcm.bin scfw_tcm.bin
> > > +
> > > +$ make SOC=iMX8QM flash_linux_m4
> >
> > flash_linux_m4 is not a valid build target in U-Boot.
> 
> yes indeed but this is the build target in the imx-mkimage repo. Which I
> currently need to build the complete bootstream with ATF and scu.

flash_linux_m4 is imx-mkimage target, not U-Boot target.
U-Boot already mkimage already has i.MX8QM support.

> 
> > > SC_PM_PW_MODE_ON);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = sc_pm_set_clock_rate(-1, SC_R_UART_0, 2, &rate);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Enable UART0 clock root */
> > > +	ret = sc_pm_clock_enable(-1, SC_R_UART_0, 2, true, false);
> > > +	if (ret)
> > > +		return ret;
> >
> > Please use sc_pm_setup_uart
> 
> ok
> >
> > > +
> > > +	setup_iomux_uart();
> > > +
> > > +	sc_pm_set_resource_power_mode(-1, SC_R_DC_0,
> > > SC_PM_PW_MODE_ON);
> >
> > Why power up DC_0? Should not kernel do it by itself?
> 
> Without this change the kernel is stucked during the boot. So currently the
> kernel don't power up DC_0. So I would like to keep that until it is fixed in the
> kernel.

Then, please add a comment here.

> 
> >
> > > +#if IS_ENABLED(CONFIG_DM_GPIO)
> > > +static void board_gpio_init(void)
> > > +{
> > > +	/* TODO */
> > > +}
> > > +#else
> > > +static inline void board_gpio_init(void) {} #endif
> >
> > If do nothing, please drop.
> 
> ok
> >
> > > +
> > > +#if IS_ENABLED(CONFIG_FEC_MXC)
> > > +#include <miiphy.h>
> > > +
> > > +int board_phy_config(struct phy_device *phydev) { #ifdef
> > > +CONFIG_FEC_ENABLE_MAX7322
> > > +	u8 value;
> > > +
> > > +	/* This is needed to drive the pads to 1.8V instead of 1.5V */
> > > +	i2c_set_bus_num(CONFIG_MAX7322_I2C_BUS);
> > > +
> > > +	if (!i2c_probe(CONFIG_MAX7322_I2C_ADDR)) {
> > > +		/* Write 0x1 to enable O0 output, this device has no addr */
> > > +		/* hence addr length is 0 */
> > > +		value = 0x1;
> > > +		if (i2c_write(CONFIG_MAX7322_I2C_ADDR, 0, 0, &value, 1))
> > > +			printf("MAX7322 write failed\n");
> > > +	} else {
> > > +		printf("MAX7322 Not found\n");
> > > +	}
> > > +	mdelay(1);
> >
> > Please use DM_I2C.
> 
> I don't get what you mean here.

Enable CONFIG_DM_I2C and use dm_i2c_x api.

> 
> > > +#ifdef CONFIG_OF_BOARD_SETUP
> > > +int ft_board_setup(void *blob, bd_t *bd) {
> > > +	return 0;
> > > +}
> > > +#endif
> >
> > Drop it do nothing.
> 
> I got some linking erros if I try to remove that ft_board_setup.

Drop CONFIG_OF_BOARD_SETUP if you not need it.

> 
> > > +int board_mmc_getcd(struct mmc *mmc) {
> > > +	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
> > > +	int ret = 0;
> > > +
> > > +	switch (cfg->esdhc_base) {
> > > +	case USDHC1_BASE_ADDR:
> > > +		ret = 1;
> > > +		break;
> > > +	case USDHC2_BASE_ADDR:
> > > +		ret = !gpio_get_value(USDHC1_CD_GPIO);
> > > +		break;
> > > +	case USDHC3_BASE_ADDR:
> > > +		ret = !gpio_get_value(USDHC2_CD_GPIO);
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> >
> > SPL_DM_MMC should help.
> 
> you mean I should turn on CONFIG_SPL_DM_MMC=y ?

Yes.

> >
> > > +
> > > +#endif /* CONFIG_FSL_ESDHC */
> > > +
> > > +void spl_dram_init(void)
> > > +{
> > > +	/* do nothing for now */
> > > +}
> >
> > Drop
> >
> > > +CONFIG_SYS_MALLOC_F_LEN=0x4000
> >
> > This might not large enough, check how i.MX8QM MEK does in upstream.

Sorry. 0x4000 is ok.

> 
> should I use CONFIG_SYS_MALLOC_F_LEN=0x2000 ?
> 
> > > +CONFIG_DM_PCA953X=y
> > > +CONFIG_DM_I2C=y
> > > +CONFIG_SYS_I2C_IMX_LPI2C=y
> > > +CONFIG_I2C_MUX=y
> > > +CONFIG_I2C_MUX_PCA954x=y
> > > +CONFIG_MISC=y
> > > +CONFIG_DM_MMC=y
> > > +CONFIG_FSL_ESDHC_IMX=y
> > > +CONFIG_PHYLIB=y
> > > +CONFIG_PHY_ADDR_ENABLE=y
> > > +CONFIG_PHY_ATHEROS=y
> > > +CONFIG_DM_ETH=y
> > > +CONFIG_PHY_GIGE=y
> > > +CONFIG_FEC_MXC_SHARE_MDIO=y
> > > +CONFIG_FEC_MXC_MDIO_BASE=0x5B040000
> > > +CONFIG_FEC_MXC=y
> > > +CONFIG_MII=y
> > > +CONFIG_PINCTRL=y
> > > +CONFIG_SPL_PINCTRL=y
> > > +CONFIG_PINCTRL_IMX8=y
> > > +CONFIG_POWER_DOMAIN=y
> > > +CONFIG_IMX8_POWER_DOMAIN=y
> > > +CONFIG_DM_REGULATOR=y
> > > +CONFIG_SPL_DM_REGULATOR=y
> > > +CONFIG_DM_REGULATOR_FIXED=y
> > > +CONFIG_DM_REGULATOR_GPIO=y
> > > +CONFIG_SPL_DM_REGULATOR_GPIO=y
> > > +CONFIG_DM_SERIAL=y
> > > +CONFIG_FSL_LPUART=y
> > > +CONFIG_SPL_TINY_MEMSET=y
> > > +# CONFIG_EFI_LOADER is not set
> > > +CONFIG_ARCH_MISC_INIT
> > > +CONFIG_NET_RANDOM_ETHADDR=y
> > > diff --git a/include/configs/imx8qm_rom7720.h
> > > b/include/configs/imx8qm_rom7720.h
> > > new file mode 100644
> > > index 0000000000..a5c869b94e
> > > --- /dev/null
> > > +++ b/include/configs/imx8qm_rom7720.h
> >
> > Please check how i.MX8QM MEK does in upstream.
> 
> I don't get what you mean here.
> 
> >
> > > +#define CONFIG_FSL_USDHC
> > > +#define CONFIG_SYS_FSL_ESDHC_ADDR	0
> > > +#define USDHC1_BASE_ADDR		0x5B010000
> > > +#define USDHC2_BASE_ADDR		0x5B020000
> > > +#define USDHC3_BASE_ADDR		0x5B030000
> >
> > Not needed.
> 
> if I remove that above Boot is stuck at:
> Loading Environment from MMC...

This should not be needed, I think. Did you enable DM_MMC?

> 
> 
> > > +#ifdef CONFIG_AHAB_BOOT
> > > +#define AHAB_ENV "sec_boot=yes\0"
> > > +#else
> > > +#define AHAB_ENV "sec_boot=no\0"
> >
> > AHAB not supported currently in upstream.
> 
> ok, do you know if I can do hab4 instead with the imx8qm?

Upstream u-boot has not added ahab feature, NXP downstream
support ahab.

Regards,
Peng.

> 
> very thx for your feedback.
> 
> Best regards,
> 
> Oliver