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

[Eric Nelson]

Hi Tapani,

On 08/29/2013 10:07 PM, Tapani Utriainen wrote:
>
> 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.
>
>>  <snip>
>>
>> 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?
>
Troy's patch-set defines some macros, then includes "pads.h" either
once (for single architecture) or twice (for multi-arch images):

	#ifdef CONFIG_MX6Q
	#include "pads.h"
	#endif
	#if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S)
	#define FOR_DL_SOLO
	#include "pads.h"
	#endif

This allows pads.h to define the set of pads once, and build
either one or two sets of tables.

This allows you to define the pads in one place, easing maintenance
but requires the use of formalized naming of tables using
MX6NAME() macro when defining and referencing per-architecture
data.

<snip>

>>> +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)
>
Right.

There is no way around having multiple tables. The "Troy" approach
just makes it easier to create and maintain.

>>> +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.
>

It seems to me that there should be a way to define the memory
configuration data once and use it both in the imximage file
to produce a DCD for a single-architecture binary or to produce
a table for the multi-arch approach.

If you look at the structure of boundary/nitrogen6q.cfg and
variants, you'll see that we pulled out the common parts (ddr-setup.cfg
and clocks.cfg) and that the memory timings for each configuration
and processor variant are separated into the files

	1066mhz_4x128mx16.cfg
	...
	800mhz_2x256mx16.cfg

With some slight additional macro-fu, we should be able to do
both with the same structure.

Regards,


Eric