[U-Boot] [PATCH V3 12/18] arm: mx6: add support for Compulab cm-fx6 CoM
Nikita Kiryanov
nikita at compulab.co.il
Tue Aug 19 17:17:52 CEST 2014
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 at 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
More information about the U-Boot
mailing list