[U-Boot] [PATCH v3 7/8] imx6: SPL support for iMX6 SabreSD

[Stefano Babic <sbabic@denx.de>]

Hi John,

On 08/11/2014 22:27, John Tobias wrote:
> This patch will enable the support for SPL on iMX6 SabreSD.
> It tested on SD2 and SD3 mmc port.
> ---
>  board/freescale/mx6sabresd/mx6sabresd.c | 211 +++++++++++++++++++++++++++++++-
>  1 file changed, 209 insertions(+), 2 deletions(-)
> 
> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
> index 3d81fff..f443337 100644
> --- a/board/freescale/mx6sabresd/mx6sabresd.c
> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
> @@ -55,8 +55,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  int dram_init(void)
>  {
> -	gd->ram_size = get_ram_size((void *)PHYS_SDRAM, PHYS_SDRAM_SIZE);
> -
> +	gd->ram_size = imx_ddr_size();
>  	return 0;
>  }
>  
> @@ -607,3 +606,211 @@ int checkboard(void)
>  	puts("Board: MX6-SabreSD\n");
>  	return 0;
>  }
> +
> +#ifdef CONFIG_SPL_BUILD
> +#include <spl.h>
> +#include <libfdt.h>
> +
> +#define BOOT_CFG	0x20D8004
> +#define __REG(x)        (*((volatile u32 *)(x)))
> +
> +struct fsl_esdhc_cfg spl_usdhc_cfg;
> +/*
> + * Got it from mx6q_4x_mt41j128.cfg file
> + */
> +void set_mt41j128_ddr(void)
> +{
> +	__REG(0x020e05a8) = 0x00000028;
> +	__REG(0x020e05b0) = 0x00000028;
> +	__REG(0x020e0524) = 0x00000028;
> +	__REG(0x020e051c) = 0x00000028;
> +
> +	__REG(0x020e0518) = 0x00000028;
> +	__REG(0x020e050c) = 0x00000028;
> +	__REG(0x020e05b8) = 0x00000028;
> +	__REG(0x020e05c0) = 0x00000028;
> +
> +	__REG(0x020e05ac) = 0x00000028;
> +	__REG(0x020e05b4) = 0x00000028;
> +	__REG(0x020e0528) = 0x00000028;
> +	__REG(0x020e0520) = 0x00000028;
> +
> +	__REG(0x020e0514) = 0x00000028;
> +	__REG(0x020e0510) = 0x00000028;
> +	__REG(0x020e05bc) = 0x00000028;
> +	__REG(0x020e05c4) = 0x00000028;
> +
> +	__REG(0x020e056c) = 0x00000030;
> +	__REG(0x020e0578) = 0x00000030;
> +	__REG(0x020e0588) = 0x00000030;
> +	__REG(0x020e0594) = 0x00000030;
> +
> +	__REG(0x020e057c) = 0x00000030;
> +	__REG(0x020e0590) = 0x00000030;
> +	__REG(0x020e0598) = 0x00000030;
> +	__REG(0x020e058c) = 0x00000000;
> +
> +	__REG(0x020e059c) = 0x00003030;
> +	__REG(0x020e05a0) = 0x00003030;
> +	__REG(0x020e0784) = 0x00000028;
> +	__REG(0x020e0788) = 0x00000028;
> +
> +	__REG(0x020e0794) = 0x00000028;
> +	__REG(0x020e079c) = 0x00000028;
> +	__REG(0x020e07a0) = 0x00000028;
> +	__REG(0x020e07a4) = 0x00000028;
> +
> +	__REG(0x020e07a8) = 0x00000028;
> +	__REG(0x020e0748) = 0x00000028;
> +	__REG(0x020e074c) = 0x00000030;
> +	__REG(0x020e0750) = 0x00020000;
> +
> +	__REG(0x020e0758) = 0x00000000;
> +	__REG(0x020e0774) = 0x00020000;
> +	__REG(0x020e078c) = 0x00000030;
> +	__REG(0x020e0798) = 0x000C0000;
> +
> +	__REG(0x021b081c) = 0x33333333;
> +	__REG(0x021b0820) = 0x33333333;
> +	__REG(0x021b0824) = 0x33333333;
> +	__REG(0x021b0828) = 0x33333333;
> +
> +	__REG(0x021b481c) = 0x33333333;
> +	__REG(0x021b4820) = 0x33333333;
> +	__REG(0x021b4824) = 0x33333333;
> +	__REG(0x021b4828) = 0x33333333;
> +
> +	__REG(0x021b0018) = 0x00001740;
> +
> +	__REG(0x021b001c) = 0x00008000;
> +	__REG(0x021b000c) = 0x8A8F7975;
> +	__REG(0x021b0010) = 0xFF538E64;
> +	__REG(0x021b0014) = 0x01FF00DB;
> +	__REG(0x021b002c) = 0x000026D2;
> +
> +	__REG(0x021b0030) = 0x008F0E21;
> +	__REG(0x021b0008) = 0x09444040;
> +	__REG(0x021b0004) = 0x00020036;
> +	__REG(0x021b0040) = 0x00000047;
> +	__REG(0x021b0000) = 0x841A0000;
> +
> +	__REG(0x021b001c) = 0x04088032;
> +	__REG(0x021b001c) = 0x00008033;
> +	__REG(0x021b001c) = 0x00428031;
> +	__REG(0x021b001c) = 0x09408030;
> +
> +	__REG(0x021b001c) = 0x04008040;
> +	__REG(0x021b0800) = 0xA1380003;
> +	__REG(0x021b0020) = 0x00005800;
> +	__REG(0x021b0818) = 0x00000007;
> +	__REG(0x021b4818) = 0x00000007;
> +
> +	/* Calibration values based on ARD and 528MHz */
> +	__REG(0x021b083c) = 0x434B0358;
> +	__REG(0x021b0840) = 0x033D033C;
> +	__REG(0x021b483c) = 0x03520362;
> +	__REG(0x021b4840) = 0x03480318;
> +	__REG(0x021b0848) = 0x41383A3C;
> +	__REG(0x021b4848) = 0x3F3C374A;
> +	__REG(0x021b0850) = 0x42434444;
> +	__REG(0x021b4850) = 0x4932473A;
> +
> +	__REG(0x021b080c) = 0x001F001F;
> +	__REG(0x021b0810) = 0x001F001F;
> +
> +	__REG(0x021b480c) = 0x001F001F;
> +	__REG(0x021b4810) = 0x001F001F;
> +
> +	__REG(0x021b08b8) = 0x00000800;
> +	__REG(0x021b48b8) = 0x00000800;
> +
> +	__REG(0x021b0404) = 0x00011006;
> +	__REG(0x021b0004) = 0x00025576;
> +
> +	__REG(0x021b001c) = 0x00000000;
> +
> +	__REG(0x020c4068) = 0x00C03F3F;
> +	__REG(0x020c406c) = 0x0030FC00;
> +	__REG(0x020c4070) = 0x0FFFC000;
> +	__REG(0x020c4074) = 0x3FF00000;
> +	__REG(0x020c4078) = 0x00FFF300;
> +	__REG(0x020c407c) = 0x0F0000C3;
> +	__REG(0x020c4080) = 0x000003FF;
> +}
> +

This cannot be accepted. Firstly, you cannot use fixed offset. The code
you propose here is very difficult to maintain.

Then, please take a look at the i.MX6 boards that are already supporting
SPL (ventana and novena). There is a logical separation between iomux,
calibration, and so on. You have to provide tables, and you should then
call the common functions:

        mx6dq_dram_iocfg();
        mx6_dram_cfg();

Again, please take a look at the boards I mentioned.

> +/*
> + * This section require the differentiation
> + * between iMX6 Sabre Families.
> + * But for now, it will configure only for
> + * SabreSD.
> + */
> +static void spl_dram_init(void)
> +{
> +	set_mt41j128_ddr();
> +}
> +
> +int spl_board_mmc_init(bd_t *bis)
> +{
> +	unsigned reg = readl(BOOT_CFG);
> +	/*
> +	 * Upon reading BOOT_CFG register the following map is done:
> +	 * (U-boot device node)    (Physical Port)
> +	 * 0x840                   SD2
> +	 * 0x1040                  SD3
> +	 * 0x1840                  eMMC
> +	 */

You add a new entry point here (spl_board_mmc_init), but why cannot you
do this in the functions already supplied by SPL ? Why not in board_init_f ?

> +	switch (reg) {
> +	case 0x840:
> +		imx_iomux_v3_setup_multiple_pads(
> +			usdhc2_pads, ARRAY_SIZE(usdhc2_pads));
> +		spl_usdhc_cfg.esdhc_base = USDHC2_BASE_ADDR;
> +		spl_usdhc_cfg.sdhc_clk = mxc_get_clock(MXC_ESDHC2_CLK);
> +		gd->arch.sdhc_clk = spl_usdhc_cfg.sdhc_clk;
> +		break;
> +	case 0x1040:
> +		imx_iomux_v3_setup_multiple_pads(
> +			usdhc3_pads, ARRAY_SIZE(usdhc3_pads));
> +		spl_usdhc_cfg.esdhc_base = USDHC3_BASE_ADDR;
> +		spl_usdhc_cfg.sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
> +		gd->arch.sdhc_clk = spl_usdhc_cfg.sdhc_clk;
> +		break;
> +	case 0x1840:
> +		imx_iomux_v3_setup_multiple_pads(
> +			usdhc4_pads, ARRAY_SIZE(usdhc4_pads));
> +		spl_usdhc_cfg.esdhc_base = USDHC4_BASE_ADDR;
> +		spl_usdhc_cfg.sdhc_clk = mxc_get_clock(MXC_ESDHC4_CLK);
> +		gd->arch.sdhc_clk = spl_usdhc_cfg.sdhc_clk;
> +		break;
> +	}
> +
> +	return fsl_esdhc_initialize(bis, &spl_usdhc_cfg);
> +}
> +
> +void board_init_f(ulong dummy)
> +{
> +	/* setup AIPS and disable watchdog */
> +	arch_cpu_init();
> +
> +	/* iomux and setup of i2c */
> +	board_early_init_f();

??

A "early" function is generally called by common code at a different
point. It makes no sense to call it inside board_init_f()

> +
> +	/* setup GP timer */
> +	timer_init();
> +

Then I am expecting iomux setuup---

> +	/* UART clocks enabled and gd valid - init serial console */
> +	preloader_console_init();
> +
> +	/* DDR initialization */
> +	spl_dram_init();

and here calling the already supplied functions.

> +
> +	/* Clear the BSS. */
> +	memset(__bss_start, 0, __bss_end - __bss_start);
> +
> +	/* load/boot image from boot device */
> +	board_init_r(NULL, 0);
> +}
> +
> +void reset_cpu(ulong addr)
> +{
> +}
> +#endif
> 

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================