[U-Boot] [PATCH v4 1/2] RiOTboard: add new board

[Eric Bénard]

Hi Stefano,

Le Sun, 30 Mar 2014 18:20:49 +0200,
Stefano Babic <sbabic at denx.de> a écrit :
> I jump directly to V4 ;-) Sorry for late review, I was not in office
> last days.
> 
> Added Ben in CC. He sent a first version for the Marsboard.
> 
> On 29/03/2014 22:29, Eric Bénard wrote:
> > this board is produced by Embest/Element 14 and is based on i.MX6 Solo
> > The following features are tested :
> > - UART2 (console)
> > - eMMC
> > - SDCard
> > - uSDCard
> > - Ethernet
> > - USB Host (through 4 ports hub)
> > - HDMI output
> > - I2C 1/2/3
> > - LVDS TFT with LCD8000-97C from Embest/Element 14
> > 
> > Boot on eMMC and through USB loader are tested.
> > 
> > For more informations on this board : http://www.riotboard.org/
> > 
> > Signed-off-by: Eric Bénard <eric at eukrea.com>
> > ---
> 
> A general remark. I agree by reading the whole thread about checking at
> runtime which is the running board (you do it getting the cpu type).
> 
> However, you use also a compiler switch mechanism, adding RIOTBOARD or
> MARSBOARD in the boards.cfg. You have implemented two ways to for the
> same thing. This makes in principle your runtime detection useless,
> because you can use #if CONFIG_MARSBOARD instead of "if board_type ==
> BOARD_IS_MARSBOARD)".

True, as said in the log, anyway at the moment the same code can't run
on both boards because of the different CPU (Solo vs Dual - and not Dual
Lite).

> Is it possible to use only the runtime detection ?
> I think the main problem is CONFIG_ENV_IS_*, that is different for the
> two boards. What do you think about it ?
> 
I'll see how we can handle the 2 CONFIG_ENV_IS with runtime detection.

> IMHO you could also squash the two patches together. You add new files,
> and patch 2/2 changes some of them. I think in this case having a single
> patch makes review easier.
> 
OK no problem.

> > +	/* from linux/arch/arm/mach-imx/mach-imx6q.c :
> 
> Codestyle in U-Boot for multiline comments is:
> 
> /*
>  * ...
>  */
> 
OK will fix. FWIW checkpatch doesn't provide any warning concerning
this problem.

> > +int board_video_skip(void)
> > +{
> > ..././//
> We have already discussed in the past about this function. Each board
> (at least, imx6 board) want to have such of them, and code is
> duplicated. What about to factorize it ? I am not against to move it
> into imx-common, if we generally agree, but I would like to avoid to
> duplicate this function for each board.
>
OK, if I understand correctly you want me to factorize it ? ;-)
I'll see what I can do there.

Thanks,
Eric