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

[Stefano Babic]

Hi Tapani,


On 30/08/2013 07:07, 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.
> 

I hope I can help you.

>> 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?
>  

Ma main concern iy your patchset is that the tables must be maintained
and duplicated in the source code. However, on the board if a pin is
dedicated to a specific function, it will be dedicated for the same
function even with the other variants of the processor.

To beclearer, I take an example from your patch:

+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),
+};


CSI0_DAT10 is the pad and its function is UART, Does it make sense to
set the pin for dual as MX6_PAD_CSI0_DAT10__UART1_TXD  and for dual as
MX6Q_PAD_CSI0_DAT10__GPIO_5_28 ? Surely not, but your approach makes the
tables hard to maintain. What I am complaining is not the creation of
tables, but the cut&paste work in the code that can generate errors. You
can choose of course another solution for the problem, but what I like
to see is some macro(s) that let this job to the compiler and not to the
board maintainer.

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

Eric sends already a patch:
	http://patchwork.ozlabs.org/patch/270889/

I will apply it, so feel free to use the macros.

> 
>> 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)

As I said, my concern is not relating to the final result (multiple
array in the board file), but how easy is to maintain the code. I would
like to set a pad only once independently from the choosen SOC.

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

With the usage of the precompiler, DCD tables in configuration files are
easy to read as your code. Anyway, if a board uses SPL for setup, there
is no need to let the SOC doing that via DCD tables. And if there would
be a need to move to or from DCD, this can be done by some scripts. We
are not constrained to use imximage syntax in a C file.

> * 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)

Really not - which register is written with "writel(0x001F001F,
MMDC_P1_BASE_ADDR + 0x80c)" ? Only the memory map of all SOCs can
answer. It is easy to read in the imximage.cfg file, because this can be
written as "DATA 4, MX6_MMDC_P1_MPWLDECTRL0, 0x001F001F".

Using C structure in u-boot is a strict rule - if you see, all code is
done in this way. No new code is accepted with base + offset syntax.

> * It is a lot of effort to do struct accessors for huge tables. Both
> of IOMUX and MMDC are large (offsets of 0x800+).

You forget that for iomuxc the job was already done - there is structure
and functions to setup the pinmux.

This is really another reason to use structures: if I write something
into pointer->p1_mpwldectrl0, it is clear for everybody which register
is written. This is not true with writel(0x001F001F, MMDC_P1_BASE_ADDR +
0x80c).

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

Sorry, I see benefits in terms of readability and maintenability of the
source code and it makes easier to add new boards. This is the reason
why there are accessors for iomuxc(), as well as for most SOC's internal
controller.

> 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 is not what I meant - if we see the code, we can recognize the
sequence how the DDR3 must be programmed. We can have a function
realizing the logic (that is, wehich registers in which sequence) must
be written, and taking as arguments the parameters (calibration, and so
on) that for each chip are different. In other words, I would like that
some kind of function will be moved into common code, and not here in
board code.

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

ok - then please add this explanation as comment.

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

Ditto


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
=====================================================================