[U-Boot] [PATCH] Add support for TechNexion edm1-cf-imx6 SoM

[Tapani Utriainen]

Stefano,

Thank you for reviewing this patch, and for the constructive comments. 
Most of your comments are taken on board, and we will re-submit updated patches 
in the near future.

On some things we probably need some clarification, see inlined responses
to some of your questions.

> > ---
> >  MAINTAINERS                                     |   4 +
> >  arch/arm/include/asm/arch-mx6/spl.h             |  19 +
> >  board/technexion/edm_cf_imx6/Makefile           |  26 +
> >  board/technexion/edm_cf_imx6/README             |  30 +
> >  board/technexion/edm_cf_imx6/clocks.cfg         |  44 ++
> >  board/technexion/edm_cf_imx6/edm_cf_imx6.c      | 801 ++++++++++++++++++++++++
> >  board/technexion/edm_cf_imx6/edm_cf_imx6_pins.h |  44 ++
> >  board/technexion/edm_cf_imx6/imximage.cfg       |  17 +
> >  boards.cfg                                      |   1 +
> >  include/configs/edm_cf_imx6.h                   | 140 +++++
> >  10 files changed, 1126 insertions(+)
> >  create mode 100644 arch/arm/include/asm/arch-mx6/spl.h
> >  create mode 100644 board/technexion/edm_cf_imx6/Makefile
> >  create mode 100644 board/technexion/edm_cf_imx6/README
> >  create mode 100644 board/technexion/edm_cf_imx6/clocks.cfg
> >  create mode 100644 board/technexion/edm_cf_imx6/edm_cf_imx6.c
> >  create mode 100644 board/technexion/edm_cf_imx6/edm_cf_imx6_pins.h
> >  create mode 100644 board/technexion/edm_cf_imx6/imximage.cfg
> >  create mode 100644 include/configs/edm_cf_imx6.h
> > 
> 
> The patch should be split into separate patches, and each of them fixes
> a specific issue. From our previous discussion, we agree about:
> 
> - we need to clean up conflicts in pad definitions - see Eric's answer.
> - we need a way to simply defines pins for the different SOC variations.
> Eric and Troy have already proposed a schema adding tables *only* into
> the board file, and the generation of the table is quite automatic.
> Let's say, if I am a board maintainer and I want to add support for a
> board having all iMX6 variations, I would like to define only once which
> pins I need, without replicating for each SOC variant.
> - the same should be done with DDR and clocks, if necessary.
> 
> After these preparation patches, there should be a patch preparing i.MX6
> for SPL - changes in i.MX6 common code should go here.
> 
> Last, there will be a patch on top of the rest adding support to your board.
>

Understand. However, as far as I can tell Troy's suggestion is still
creating the pad setup compile time for one cpu. 
Is there something obvious we are missing?
 
> 
> Please change all license text according to the new rule with
> SPDX-License-Identifier.
>
Will do.
 
> 
> Maybe you should add some comments explaining that this is your
> decision, not due to the SOC. Only the first dd is mandatory by iMX
> (offset is 0x400 in flash). For u-boot, you have decided to put it into
> raw data exactly after SPL and not into a filesystem.
> 
> > +
> > +Only raw mmc boot has been verified to work.
> 
> The phrase is misleading, and let me think the other ways do not work. I
> assume that you tested only raw mmc, so please rephrase to explain this.
>
Not using a filesystem is by choice. We'll clarify on that 
(or maybe better, enable FAT)

...

> > +
> > +static inline void setup_boot_device(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;
> > +	uint bt_mem_mmc = (soc_sbmr & 0x00001000) >> 12;
> > +
> > +	switch (bt_mem_ctl) {
> > +	case 0x0:
> > +		if (bt_mem_type)
> > +			boot_dev = ONE_NAND_BOOT;
> > +		else
> > +			boot_dev = WEIM_NOR_BOOT;
> > +		break;
> > +	case 0x2:
> > +			boot_dev = SATA_BOOT;
> > +		break;
> > +	case 0x3:
> > +		if (bt_mem_type)
> > +			boot_dev = I2C_BOOT;
> > +		else
> > +			boot_dev = SPI_NOR_BOOT;
> > +		break;
> > +	case 0x4:
> > +	case 0x5:
> > +		if (bt_mem_mmc)
> > +			boot_dev = SD0_BOOT;
> > +		else
> > +			boot_dev = SD1_BOOT;
> > +		break;
> > +	case 0x6:
> > +	case 0x7:
> > +		boot_dev = MMC_BOOT;
> > +		break;
> > +	case 0x8 ... 0xf:
> > +		boot_dev = NAND_BOOT;
> > +		break;
> > +	default:
> > +		boot_dev = UNKNOWN_BOOT;
> > +		break;
> > +	}
> > +}
> > 
> 
> Let's say: boot device is not board dependent, and the required SPL
> function should be moved into general code (imx_common or
> arch/arm/cpu/armv7/mx6).
> 

Will do. In the first patch attempt we deliberately tried not to touch
common imx6 code.

> +
> > +int dram_init(void)
> > +{
> > +	uint cpurev, imxtype;
> > +	u32 sdram_size;
> > +
> > +	cpurev = get_cpu_rev();
> > +	imxtype = (cpurev & 0xFF000) >> 12;
> 
> I am expecting to have a global function getting the cputype, with
> macros of the type is_cpu_mx6q() (I see you use it later) to help us the
> usage. Not here in board code.
> 

Added.

> > +static iomux_v3_cfg_t const edmdl_uart1_pads[] = {
> > +	MX6DL_PAD_CSI0_DAT10__UART1_TXD | MUX_PAD_CTRL(UART_PAD_CTRL),
> > +	MX6DL_PAD_CSI0_DAT11__UART1_RXD | MUX_PAD_CTRL(UART_PAD_CTRL),
> > +};
> > +
> > +static iomux_v3_cfg_t const edmq_uart1_pads[] = {
> > +	MX6Q_PAD_CSI0_DAT10__UART1_TXD | MUX_PAD_CTRL(UART_PAD_CTRL),
> > +	MX6Q_PAD_CSI0_DAT11__UART1_RXD | MUX_PAD_CTRL(UART_PAD_CTRL),
> > +};
> > +
> > +static iomux_v3_cfg_t const edmdl_usdhc1_pads[] = {
> > +	MX6DL_PAD_SD1_CLK__USDHC1_CLK   | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6DL_PAD_SD1_CMD__USDHC1_CMD   | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6DL_PAD_SD1_DAT0__USDHC1_DAT0 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6DL_PAD_SD1_DAT1__USDHC1_DAT1 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6DL_PAD_SD1_DAT2__USDHC1_DAT2 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6DL_PAD_SD1_DAT3__USDHC1_DAT3 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	/* Card detect */
> > +	MX6DL_PAD_GPIO_2__GPIO_1_2      | MUX_PAD_CTRL(NO_PAD_CTRL),
> > +};
> > +
> > +static iomux_v3_cfg_t const edmq_usdhc1_pads[] = {
> > +	MX6Q_PAD_SD1_CLK__USDHC1_CLK   | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6Q_PAD_SD1_CMD__USDHC1_CMD   | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6Q_PAD_SD1_DAT0__USDHC1_DAT0 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6Q_PAD_SD1_DAT1__USDHC1_DAT1 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6Q_PAD_SD1_DAT2__USDHC1_DAT2 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	MX6Q_PAD_SD1_DAT3__USDHC1_DAT3 | MUX_PAD_CTRL(USDHC_PAD_CTRL),
> > +	/* Card detect */
> > +	MX6Q_PAD_GPIO_2__GPIO_1_2      | MUX_PAD_CTRL(NO_PAD_CTRL),
> > +};
> 
> I do not like this solution: this is a bare duplication of the pads, and
> it is prone to errors. The definitions of the table must be in some way
> automatic. I want to define a pin: if SD1_CLK is used for MMC, this does
> not depend from the SOC variant.
>

TBH, I hate this solution as well. But guess we heard Eric's cries for mercy, 
and did it the way Boundary (and several kernel board files) do it -- rather 
than the way we did it in the Wandboard kernel.

> What about Troy's solution ?
> 
Did not apply? And still had a mess of arrays in the board file. Or what we
have *is* the multi-cpu variant of Troy's approach...?
(With both CPU type macros expanded, manually)

> 
> Let's say : this does not scale well. For each peripheral we have
> exactly the same code. The if..then..else should be hidden in some way,
> using macros to select the right table.
> 
...
> 
> As you see, we can have a lot of some code.
> 
...
> 
> Again here.
>
Beating a dead horse. I happily killed that mess in the WB kernel once already.

> > +
> > +int board_early_init_f(void)
> > +{
> > +	setup_iomux_uart();
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Do not overwrite the console
> > + * Use always serial for U-Boot console
> > + */
> > +int overwrite_console(void)
> > +{
> > +	return 1;
> > +}
> 
> I have not seen CONFIG_VIDEO in your configuration file. Is this dead code ?
>
Yes, we cut away the splash screen support from the submitted patch.
(Our original patch was against another u-boot version, and we tried to cut out 
everything non-essential or thoroughly tested).

Will cut even more :-)
 
> > +#if defined(CONFIG_SPL_BUILD)
> > +void board_init_f(ulong dummy)
> > +{
> > +	/* Set the stack pointer. */
> > +	asm volatile("mov sp, %0\n" : : "r"(CONFIG_SPL_STACK));
> > +
> > +	/* Clear the BSS. */
> > +	memset(__bss_start, 0, __bss_end - __bss_start);
> > +
> > +	/* Set global data pointer. */
> > +	gd = &gdata;
> > +
> > +	arch_cpu_init();
> > +	board_early_init_f();
> > +	timer_init();
> > +	preloader_console_init();
> > +
> > +	board_init_r(NULL, 0);
> > +}
> 
> None of this stuff is board specific - we need to factorize this, making
> available for all i.MX6 boards. I will say, for i.MX5, too.
>
Will try.
 
> > +
> > +static void spl_dram_init_mx6solo_512mb(void)
> > +{
> > +	/* DDR3 initialization based on the MX6Solo Auto Reference Design (ARD) */
> > +	/* DDR IO TYPE */
> > +	writel(0x000c0000, IOMUXC_BASE_ADDR + 0x774);
> > +	writel(0x00000000, IOMUXC_BASE_ADDR + 0x754);
> > +	/* Clock */
> > +	writel(0x00000030, IOMUXC_BASE_ADDR + 0x4ac);
> > +	writel(0x00000030, IOMUXC_BASE_ADDR + 0x4b0);
> > +	/* Address */

[ .... long list of writels ... ]

> > +	writel(0x04008032, MMDC_P0_BASE_ADDR + 0x01c);
> > +	writel(0x00008033, MMDC_P0_BASE_ADDR + 0x01c);
> > +	writel(0x00048031, MMDC_P0_BASE_ADDR + 0x01c);
> > +	writel(0x07208030, MMDC_P0_BASE_ADDR + 0x01c);
> > +	writel(0x04008040, MMDC_P0_BASE_ADDR + 0x01c);
> > +	writel(0x00005800, MMDC_P0_BASE_ADDR + 0x020);
> > +	writel(0x00011117, MMDC_P0_BASE_ADDR + 0x818);
> > +	writel(0x00011117, MMDC_P1_BASE_ADDR + 0x818);
> > +	writel(0x0002556d, MMDC_P0_BASE_ADDR + 0x004);
> > +	writel(0x00011006, MMDC_P1_BASE_ADDR + 0x004);
> > +	writel(0x00000000, MMDC_P0_BASE_ADDR + 0x01c);
> > +}
> 
> Really I do not like this solution. First, you should accessor to set
> the iomux, without using base address + offset. And of course, access to
> the ram controller should be done in the same way using a C structure,
> not offsets.
> 
> Then the problem is similar to the pads. I will propose we have a
> general function, and the values of board specific parameters (32
> against 64 bit size, calibration registers, and so on), and start the
> ddr procedure. The functions here do the same on different registers.
> 

We agree that the does does not look pretty. But there needs to be some 
clarification. 

However, using this has some benefits:
* It is easier to convert from (and compare to) DCD tables 
* Easier to look things up in the TRM (base + offset are easier to find
in a long list of registers sorted by offset, than a name)
* It is a lot of effort to do struct accessors for huge tables. Both
of IOMUX and MMDC are large (offsets of 0x800+). Having struct
accessors would take quite long to enter manually (two tables of 500+ entries 
each, and different between cpu types). This would be hours, if not a day of
braindead work without any tangible benefit.

I could make those tables of { offset, value } and do the writels using
a for-loop, but that would just mariginally improve on the ugliness.

> > +u32 spl_boot_device(void)
> > +{
> > +	puts("Boot Device: ");
> > +	switch (get_boot_device()) {
> > +	case SD0_BOOT:
> > +		printf("SD0\n");
> > +		return BOOT_DEVICE_MMC1;
> > +	case SD1_BOOT:
> > +		printf("SD1\n");
> > +		return BOOT_DEVICE_MMC2;
> > +	case MMC_BOOT:
> > +		printf("MMC\n");
> > +		return BOOT_DEVICE_MMC2;
> > +	case NAND_BOOT:
> > +		printf("NAND\n");
> > +		return BOOT_DEVICE_NAND;
> > +	case UNKNOWN_BOOT:
> > +	default:
> > +		printf("UNKNOWN\n");
> > +		return BOOT_DEVICE_NONE;
> > +	}
> > +}
> 
> This is also common code.
> 
Yes.

> > +
> > +u32 spl_boot_mode(void)
> > +{
> > +	switch (spl_boot_device()) {
> > +	case BOOT_DEVICE_MMC1:
> > +	case BOOT_DEVICE_MMC2:
> > +	case BOOT_DEVICE_MMC2_2:
> > +#ifdef CONFIG_SPL_FAT_SUPPORT
> > +		return MMCSD_MODE_FAT;
> > +#else
> > +		return MMCSD_MODE_RAW;
> > +#endif
> > +		break;
> > +        case BOOT_DEVICE_NAND:
> > +	default:
> > +		puts("spl: ERROR:  unsupported device\n");
> > +		hang();
> > +	}
> > +}
> > +
> > +void reset_cpu(ulong addr)
> > +{
> > +
> > +}
> 
> reset_cpu for imx should activate the watchdog, see
> drivers/watchdog/imx_watchdog.c
>
Ok.
 
> > diff --git a/boards.cfg b/boards.cfg
> > index 79d6cd8..b7d66ff 100644
> > --- a/boards.cfg
> > +++ b/boards.cfg
> > @@ -288,6 +288,7 @@ nitrogen6s1g                 arm         armv7       nitrogen6x          boundar
> >  wandboard_dl		     arm	 armv7	     wandboard		 -		mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6dl.cfg,MX6DL,DDR_MB=1024
> >  wandboard_quad		     arm	 armv7	     wandboard		 -		mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q,DDR_MB=2048
> >  wandboard_solo		     arm	 armv7	     wandboard		 -		mx6 wandboard:IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6s.cfg,MX6S,DDR_MB=512
> > +edm_cf_imx6                  arm         armv7       edm_cf_imx6         technexion     mx6 edm_cf_imx6:IMX_CONFIG=board/technexion/edm_cf_imx6/imximage.cfg,SPL
> 
> Why CONFIG_SPL is not set into the configuration file ? Is there a
> version without SPL ?
> 
There used to be... yes, we'll fix that.

> Please maintain the list sorted.
>
No problem.

... 
> > +
> > +#define MACH_TYPE_EDM_CF_IMX6		4257
> > +#define CONFIG_MACH_TYPE		MACH_TYPE_EDM_CF_IMX6
> 
> if MACH_TYPE_EDM_CF_IMX6 is used only here, better:
> 
> #define CONFIG_MACH_TYPE		4257
>
Yes, guess we tried to mimic wandboard.
 
> > +
> > +#define CONFIG_CMDLINE_TAG
> > +#define CONFIG_SETUP_MEMORY_TAGS
> > +#define CONFIG_REVISION_TAG
> > +

> 
> > +#ifdef CONFIG_SPL
> > +#define CONFIG_SPL_FRAMEWORK
> > +#define CONFIG_SPL_TEXT_BASE          0x00908400
> 
> Ok - SPL goes into IRAM, that is good. Can you explain me the value of
> the 0x8400 offset ?
> 
The available IRAM starts at 907000 and we need space for the IVT tables.

> > +#define CONFIG_SPL_PAD_TO 0x400
> > +#define CONFIG_SPL_START_S_PATH     "arch/arm/cpu/armv7"
> > +#define CONFIG_SPL_STACK	0x0091FFB8
> 
> Maybe better set it with some size - where is coming this value ?
> 

From page 393 in the imx6solo TRM. That is Freescale's recommended initial 
stack top.

regards,

//Tapani