[U-Boot] [PATCH V3 12/18] arm: mx6: add support for Compulab cm-fx6 CoM

[Nikita Kiryanov <nikita@compulab.co.il>]

Hi Igor,

On 13/08/14 15:55, Igor Grinberg wrote:
>> +#ifdef CONFIG_FSL_ESDHC
>> +int board_mmc_init(bd_t *bis)
>> +{
>> +	int i;
>> +
>> +	cm_fx6_set_usdhc_iomux();
>> +	for (i = 0; i < CONFIG_SYS_FSL_USDHC_NUM; i++) {
>> +		usdhc_cfg[i].sdhc_clk = mxc_get_clock(usdhc_clk[i]);
>> +		usdhc_cfg[i].max_bus_width = 4;
>> +		fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
>> +		enable_usdhc_clk(1, i);
>
> Why does the board code needs to handle the mmc clocks?
> Can't this be handled in the common code?

It (probably) can, but it currently isn't. If we were to change this
I would prefer it to be done outside of this patch series.

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +int board_init(void)
>> +{
>> +	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
>> +	return 0;
>> +}
>> +
>> +int checkboard(void)
>> +{
>> +	puts("Board: CM-FX6\n");
>> +	return 0;
>> +}
>> +
>> +static ulong bank1_size;
>> +static ulong bank2_size;
>> +
>> +void dram_init_banksize(void)
>> +{
>> +	gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
>> +	gd->bd->bi_dram[0].size = bank1_size;
>> +	gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
>> +	gd->bd->bi_dram[1].size = bank2_size;
>> +}
>> +
>> +int dram_init(void)
>> +{
>> +	gd->ram_size = imx_ddr_size();
>> +	switch (gd->ram_size) {
>> +	case 0x10000000: /* DDR_16BIT_256MB */
>> +		bank1_size = 0x10000000;
>> +		bank2_size = 0;
>> +		break;
>> +	case 0x20000000: /* DDR_32BIT_512MB */
>> +		bank1_size = 0x20000000;
>> +		bank2_size = 0;
>> +		break;
>> +	case 0x40000000:
>> +		if (is_cpu_type(MXC_CPU_MX6SOLO)) { /* DDR_32BIT_1GB */
>> +			bank1_size = 0x20000000;
>> +			bank2_size = 0x20000000;
>> +		} else { /* DDR_64BIT_1GB */
>> +			bank1_size = 0x40000000;
>> +			bank2_size = 0;
>
> You don't need to init bank2_size to zero in all above cases.

Actually, I'm going to refactor and eliminate these variables, because
I see that bss is not yet ready at this stage in the boot.

>
>> +		}
>> +		break;
>> +	case 0x80000000: /* DDR_64BIT_2GB */
>> +		bank1_size = 0x40000000;
>> +		bank2_size = 0x40000000;
>> +		break;
>> +	case 0xF0000000: /* DDR_64BIT_4GB */
>> +		bank1_size = 0x70000000;
>> +		bank2_size = 0x7FF00000;
>> +		gd->ram_size -= 0x100000;
>> +		break;
>> +	default:
>> +		printf("ERROR: Unsupported DRAM size 0x%lx\n", gd->ram_size);
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +u32 get_board_rev(void)
>> +{
>> +	return 100;
>
> Hmmm... cl_eeprom_get_board_rev()?

There's no I2C support in this patch.
I'll remove this function and reintroduce it later per your suggestion
in a different patch.

>> +static __maybe_unused enum mxc_clock usdhc_clk[3] = {
>> +	MXC_ESDHC_CLK,
>> +	MXC_ESDHC2_CLK,
>> +	MXC_ESDHC3_CLK,
>> +};
>
> Why do you need the above structures defined here in .h file?
> Can they live in .c file that is using them?
> I think cm_fx6.c is the appropriate place for these.

I'll move them to cm_fx6.c. The code reuse was minimal anyway..

>> +static void spl_mx6s_dram_setup_iomux(void)
>> +{
>> +	struct mx6sdl_iomux_ddr_regs ddr_iomux;
>> +	struct mx6sdl_iomux_grp_regs grp_iomux;
>> +
>> +	ddr_iomux.dram_sdqs0	= 0x00000038;
>> +	ddr_iomux.dram_sdqs1	= 0x00000038;
>> +	ddr_iomux.dram_sdqs2	= 0x00000038;
>> +	ddr_iomux.dram_sdqs3	= 0x00000038;
>> +	ddr_iomux.dram_sdqs4	= 0x00000038;
>> +	ddr_iomux.dram_sdqs5	= 0x00000038;
>> +	ddr_iomux.dram_sdqs6	= 0x00000038;
>> +	ddr_iomux.dram_sdqs7	= 0x00000038;
>> +	ddr_iomux.dram_dqm0	= 0x00000038;
>> +	ddr_iomux.dram_dqm1	= 0x00000038;
>> +	ddr_iomux.dram_dqm2	= 0x00000038;
>> +	ddr_iomux.dram_dqm3	= 0x00000038;
>> +	ddr_iomux.dram_dqm4	= 0x00000038;
>> +	ddr_iomux.dram_dqm5	= 0x00000038;
>> +	ddr_iomux.dram_dqm6	= 0x00000038;
>> +	ddr_iomux.dram_dqm7	= 0x00000038;
>> +	ddr_iomux.dram_cas	= 0x00000038;
>> +	ddr_iomux.dram_ras	= 0x00000038;
>> +	ddr_iomux.dram_sdclk_0	= 0x00000038;
>> +	ddr_iomux.dram_sdclk_1	= 0x00000038;
>> +	ddr_iomux.dram_sdcke0	= 0x00003000;
>> +	ddr_iomux.dram_sdcke1	= 0x00003000;
>> +	/*
>> +	 * Below DRAM_RESET[DDR_SEL] = 0 which is incorrect according to
>> +	 * Freescale QRM, but this is exactly the value used by the automatic
>> +	 * calibration script and it works also in all our tests, so we leave
>> +	 * it as is at this point.
>> +	 */
>> +	ddr_iomux.dram_reset	= 0x00000038;
>> +	ddr_iomux.dram_sdba2	= 0x00000000;
>> +	ddr_iomux.dram_sdodt0	= 0x00000038;
>> +	ddr_iomux.dram_sdodt1	= 0x00000038;
>> +	grp_iomux.grp_b0ds	= 0x00000038;
>> +	grp_iomux.grp_b1ds	= 0x00000038;
>> +	grp_iomux.grp_b2ds	= 0x00000038;
>> +	grp_iomux.grp_b3ds	= 0x00000038;
>> +	grp_iomux.grp_b4ds	= 0x00000038;
>> +	grp_iomux.grp_b5ds	= 0x00000038;
>> +	grp_iomux.grp_b6ds	= 0x00000038;
>> +	grp_iomux.grp_b7ds	= 0x00000038;
>> +	grp_iomux.grp_addds	= 0x00000038;
>> +	grp_iomux.grp_ddrmode_ctl = 0x00020000;
>> +	grp_iomux.grp_ddrpke	= 0x00000000;
>> +	grp_iomux.grp_ddrmode	= 0x00020000;
>> +	grp_iomux.grp_ctlds	= 0x00000038;
>> +	grp_iomux.grp_ddr_type	= 0x000C0000;
>
> Hmmm... Can we do the above initializations statically?

Yes.

> I mean, define the structures outside of the function and initialize
> them statically, and then only pass the structures to the below
> function. Moreover, this way, you will not need this function at all,
> but call the below from cm_fx6_spl_dram_init().

Will do (here and below)..

>
>> +	mx6sdl_dram_iocfg(64, &ddr_iomux, &grp_iomux);
>> +}
>> +
>> +static void spl_mx6q_dram_setup_iomux(void)
>> +{
>> +	struct mx6dq_iomux_ddr_regs ddr_iomux;
>> +	struct mx6dq_iomux_grp_regs grp_iomux;
>> +

[..]

>> +	mx6dq_dram_iocfg(64, &ddr_iomux, &grp_iomux);
>> +}
>> +
>> +static void spl_mx6s_dram_init(enum ddr_config dram_config, int reset)
>
> I think we have bool type in U-Boot, right?
> If so, it would be more descriptive to have bool reset.

OK

>
>> +{
>> +	struct mx6_mmdc_calibration calib;
>> +	struct mx6_ddr_sysinfo sysinfo;
>> +	struct mx6_ddr3_cfg ddr3_cfg;
>> +
>> +	if (reset)
>> +		((struct mmdc_p_regs *)MX6_MMDC_P0_MDCTL)->mdmisc = 2;
>> +
>> +	calib.p0_mpwldectrl0	= 0x005B0061;
>> +	calib.p0_mpwldectrl1	= 0x004F0055;


[..]

>> +
>> +static int cm_fx6_spl_dram_init(void)
>> +{
>> +	u32 cpurev, imxtype;
>> +	unsigned long bank1_size, bank2_size;
>> +
>> +	cpurev = get_cpu_rev();
>> +	imxtype = (cpurev & 0xFF000) >> 12;
>
> I would expect here at least cpu_type() usage instead of open coding.
> Or may be to spare the above construct, it is worth introducing
> a kind of get_cpu_type()? like:
>
> #define get_cpu_type() (cpu_type(get_cpu_rev()))
>
> in arch/arm/include/asm/arch-mx6/sys_proto.h
> And then you can use get_imx_cpu_type() inside the switch below.
> What do you think?

Good idea. Will add it.

>
>> +
>> +	switch (imxtype) {
>> +	case MXC_CPU_MX6SOLO:
>> +		spl_mx6s_dram_setup_iomux();
>> +
>> +		spl_mx6s_dram_init(DDR_32BIT_1GB, 0);
>> +		bank1_size = get_ram_size((long int *)PHYS_SDRAM_1, 0x80000000);
>> +		if (bank1_size == 0x40000000)
>> +			return 0;
>> +
>> +		if (bank1_size == 0x20000000) {
>> +			spl_mx6s_dram_init(DDR_32BIT_512MB, 1);
>> +			return 0;
>> +		}
>> +
>> +		spl_mx6s_dram_init(DDR_16BIT_256MB, 1);
>> +		bank1_size = get_ram_size((long int *)PHYS_SDRAM_1, 0x80000000);
>> +		if (bank1_size == 0x10000000)
>> +			return 0;
>> +
>> +		break;
>> +	case MXC_CPU_MX6D:
>> +	case MXC_CPU_MX6Q:
>> +		spl_mx6q_dram_setup_iomux();
>> +
>> +		spl_mx6q_dram_init(DDR_64BIT_4GB, 0);
>> +		bank1_size = get_ram_size((long int *)PHYS_SDRAM_1, 0x80000000);
>> +		if (bank1_size == 0x80000000)
>> +			return 0;
>> +
>> +		if (bank1_size == 0x40000000) {
>> +			bank2_size = get_ram_size((long int *)PHYS_SDRAM_2,
>> +								0x80000000);
>> +			if (bank2_size == 0x40000000) {
>> +				/* Don't do a full reset here */
>> +				spl_mx6q_dram_init(DDR_64BIT_2GB, 0);
>> +			} else {
>> +				spl_mx6q_dram_init(DDR_64BIT_1GB, 1);
>> +			}
>> +
>> +			return 0;
>> +		}
>> +
>> +		spl_mx6q_dram_init(DDR_32BIT_512MB, 1);
>> +		bank1_size = get_ram_size((long int *)PHYS_SDRAM_1, 0x80000000);
>> +		if (bank1_size == 0x20000000)
>> +			return 0;
>> +
>> +		spl_mx6q_dram_init(DDR_16BIT_256MB, 1);
>> +		bank1_size = get_ram_size((long int *)PHYS_SDRAM_1, 0x80000000);
>> +		if (bank1_size == 0x10000000)
>> +			return 0;
>> +
>> +		break;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +static iomux_v3_cfg_t const uart4_pads[] = {
>> +	IOMUX_PADS(PAD_KEY_COL0__UART4_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
>> +	IOMUX_PADS(PAD_KEY_ROW0__UART4_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
>> +};
>> +
>> +static void cm_fx6_setup_uart(void)
>> +{
>> +	SETUP_IOMUX_PADS(uart4_pads);
>> +	enable_uart_clk(1);
>> +}
>> +
>> +#ifdef CONFIG_SPL_SPI_SUPPORT
>> +static void cm_fx6_setup_ecspi(void)
>> +{
>> +	enable_cspi_clock(1, 0);
>> +	cm_fx6_set_ecspi_iomux();
>
> It does not really meter, but it will lok better if the sequence
> will be the same as for uart: mux and then clock...

Can do..

>
>> +}
>> +#else
>> +static void cm_fx6_setup_ecspi(void) { }
>> +#endif
>> +
>> +void board_init_f(ulong dummy)
>> +{
>> +	gd = &gdata;
>> +	enable_usdhc_clk(1, 2);
>
> can this be done inside board_mmc_init() or even in a common location
> like fsl_esdhc_initialize()?
>
>> +	arch_cpu_init();
>> +	timer_init();
>> +	cm_fx6_setup_ecspi();
>> +	cm_fx6_setup_uart();
>> +	get_clocks();
>> +	preloader_console_init();
>> +	gpio_direction_output(CM_FX6_GREEN_LED, 1);
>> +	if (cm_fx6_spl_dram_init()) {
>> +		puts("!!!ERROR!!! DRAM detection failed!!!\n");
>> +		hang();
>
> Hmmm... why hang? may be reset the cpu/board and try again?

I prefer the hang because it seems safer to me; better to not
boot than boot with bad RAM.

>
>> +	}
>> +
>> +	memset(__bss_start, 0, __bss_end - __bss_start);
>> +	board_init_r(NULL, 0);
>> +}
>> +
>> +void spl_board_init(void)
>> +{
>> +	uint soc_sbmr = readl(SRC_BASE_ADDR + 0x4);
>> +	uint bt_mem_ctl = (soc_sbmr & 0x000000FF) >> 4;
>> +	uint bt_mem_type = (soc_sbmr & 0x00000008) >> 3;
>> +
>> +	if (bt_mem_ctl == 0x3 && !bt_mem_type)
>> +		puts("Booting from SPI flash\n");
>> +	else if (bt_mem_ctl == 0x4 || bt_mem_ctl == 0x5)
>> +		puts("Booting from MMC\n");
>> +	else
>> +		puts("Unknown boot device\n");
>
> Can we call spl_boot_device() here instead?

Sure

>
>> +}
>> +
>> +#ifdef CONFIG_SPL_MMC_SUPPORT
>> +int board_mmc_init(bd_t *bis)
>> +{
>> +	cm_fx6_set_usdhc_iomux();
>> +
>> +	usdhc_cfg[2].sdhc_clk = mxc_get_clock(usdhc_clk[2]);
>> +	usdhc_cfg[2].max_bus_width = 4;
>
> You use only one member of that array...
> It is not likely to change.
> Can we just define one instance on the stack and
> hardcode its initialization?
> This will eliminate the need for sharing the same definition
> of that array between SPL and U-Boot and thus make things simpler.

Agreed.

>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>> new file mode 100644
>> index 0000000..b877b65
>> --- /dev/null
>> +++ b/include/configs/cm_fx6.h
>> @@ -0,0 +1,211 @@
>> +/*
>> + * Config file for Compulab CM-FX6 board
>> + *
>> + * Copyright (C) 2014, Compulab Ltd - http://compulab.co.il/
>> + *
>> + * Author: Nikita Kiryanov <nikita@compulab.co.il>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#ifndef __CONFIG_CM_FX6_H
>> +#define __CONFIG_CM_FX6_H
>> +
>> +#include <asm/arch/imx-regs.h>
>> +#include <config_distro_defaults.h>
>> +
>> +#define CONFIG_SYS_L2CACHE_OFF
>
> Hmmm... Is there a problem using cache on i.MX6 currently?
>

This define can be removed.

>
>> +#define CONFIG_EXTRA_ENV_SETTINGS \
>> +	"kernel=uImage-cm-fx6\0" \
>> +	"autoload=no\0" \
>> +	"loadaddr=0x10800000\0" \
>> +	"fdtaddr=0x11000000\0" \
>> +	"console=ttymxc3,115200\0" \
>> +	"ethprime=FEC0\0" \
>> +	"bootscr=boot.scr\0" \
>> +	"bootm_low=18000000\0" \
>> +	"video_hdmi=mxcfb0:dev=hdmi,1920x1080M-32 at 50,if=RGB32\0" \
>> +	"video_dvi=mxcfb0:dev=dvi,1280x800M-32 at 50,if=RGB32\0" \
>> +	"fdtfile=cm-fx6.dtb\0" \
>> +	"doboot=bootm ${loadaddr}\0" \
>> +	"loadfdt=false\0" \
>> +	"setboottypez=setenv kernel zImage-cm-fx6;" \
>> +		"setenv doboot bootz ${loadaddr} - ${fdtaddr};" \
>> +		"setenv loadfdt true;\0" \
>> +	"setboottypem=setenv kernel uImage-cm-fx6;" \
>> +		"setenv doboot bootm ${loadaddr};" \
>> +		"setenv loadfdt false;\0"\
>> +	"run_eboot=echo Starting EBOOT ...; "\
>> +		"mmc dev ${mmcdev} && " \
>> +		"mmc rescan && mmc read 10042000 a 400 && go 10042000\0" \
>> +	"mmcdev=2\0" \
>> +	"mmcroot=/dev/mmcblk0p2 rw rootwait\0" \
>> +	"loadmmcbootscript=fatload mmc ${mmcdev} ${loadaddr} ${bootscr}\0" \
>
> Can we switch to use load instead of fatload?
>

Yes