[U-Boot] [PATCH 5/5] MX5:MX53: add initial support for MX53EVK board

[Stefano Babic]

On 12/17/2010 04:05 AM, Jason Liu wrote:
> There is pretty much different with I.MX51 ROM,  we will not use DCD
> data file to config the
> DDR script since ROM has the DCD size limitation and use the advance
> feature of what we
> called plug-in,  the plug-in code must be in the first 2K of MMC card
> from 0x400 offset, that's
> why we need put this code section before start.S. The plug-in code
> will be called by boot ROM
> to do DDR init first and copy u-boot to DDR and jump to _start  to run it.

As I am not understanding what you mean as "plugin", it seems to me you
need some code able to set-up RAM and copy the u-boot code. This method
looks like very similar to other SoCs, where a first stage boot loader
is needed or when we boot from NAND.

I do not find any reasonable argument to follow the methods you are
proposing and to link statically this code to u-boot. In U-Boot we have
already two mechanisms for do this initial setup, one using special
images (with .kwb and .imx images when the RBL of the SoC is able to do
the whole job), the second one with a first stage bootloader as nand_spl
does and as Albert has already suggested.

> Does anyone of expert like you and Albert and Wolfgang tell me how to
> do it? It's very important for FSL
> coming many SOCs support, since we will use plug-in feature of boot ROM.

I am really not understanding if this "plugin" feature is really
different as what we have with other SoCs or when we boot from NAND on
other SOCs. As I can up now understand, it is similar to other
approaches we have support already and I do not see a reason to
implement a new one.

Is there some released Freescale's documentation to allow to me to
better understand what you are meaning ?

> Yes, we need write it in assembly. Please see the comments above.
> We can't put it into board_init or board_early_init_f, because it does
> DDR init.
> It *must* run in IRAM which load by ROM first.

Running in IRAM is not a sufficient reason to do in this way. If I am
not wrong, we could reach the same behavior on a i.MX51, too, if we set
the app_code_jump in the header to point to an IRAM address. Then even
the i.MX51 RBL should copy the code from storage to IRAM and then jump
to this address, isnt it ?

> I can't catch your mean. The function just do i2c iomux setup, anything wrong?

My concern is related to the fact that the controllers are normally
enumerated as "port numbers" and you make the selection based on the
base address, that could be confusing. I see an approach of the type:

static void setup_i2c(unsigned int port_number)
{

	switch (port_number) {
		0:
			.....
			break;
		1:

	.........
}


more intuitive to understand.

>>> +     /* Set core voltage VDDGP to 1.05V for 800MHZ */
>>> +     buf[0] = 0x45;
>>> +     buf[1] = 0x4a;
>>> +     buf[2] = 0x52;
>>
>> Please remove all fixed constants in the file and replaced them with
>> named constants, defined in a header file. Check vision2.c as reference.
>> This board uses the MC13892 PMIC controller, and ./include/mc13892.h
>> contains all required defines.
> 
> OK,

As further comment: there is already a pmic driver in u-boot
(fsl_pmic.c), that hides the underlying communication between processor
and pmic itself. At the moment, only spi is implemented. It is better to
expand the fsl_pmic code to support i2c, too, and to use the accessors
pmic_reg_read() and pmic_reg_write() to access to the pmic internal
registers in the board related code.

> No, this is not the case like MX53. The power of FEC is default on of
> this board.
> We need change power before we can run CPU at full speed.

Ok, understood.

>>> +#define CONFIG_SYS_I2C_PORT             I2C2_BASE_ADDR
>>
>> As stated before: port means an enumeration value (0,1,..N), and it is
>> set to a physical address.
> 
> Yes, I will fix it.

This comment is related to the previous one for the setup_i2c()
function. Sorry if I was not clear enough. As you describe the define
CONFIG_SYS_I2C_PORT, this should be a port number (0,1,..N) and the
setup_i2() should accept as parameter this enumeration value, not a base
address.

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-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================