[U-Boot] [PATCH 2/2] EfikaSB: Add preliminary EfikaSB support

[Marek Vasut]

On Thursday, September 22, 2011 04:00:38 PM Stefano Babic wrote:
> On 09/22/2011 02:29 PM, Marek Vasut wrote:
> >> This seems not necessary because CONFIG_MACH_EFIKA* is set at the build
> >> time with the option in boards.cfg. With a correct boards.cfg, we cannot
> >> get this error.
> > 
> > Well once someone adds another efika, he can forget about it. And there's
> > mx53 efika in the works.
> 
> Then there will be a review for the new code. At the moment, this part
> behaves as dead code.

Dead code? it's all used, I don't see your point. To me, it's more readable. 
Hmhm ...
> 
> Do you mean the same board files will be used ? I am not aware about a
> board having two different SOCs. Probably (I say probably, we will see
> whan the patches for a new board will be sent...) we will have a
> different structure, as the MX53 have different setup as the MX51. In
> the same way we have now a mx51evk and mx53evk.

We'll see ... I don't have the board just yet.

> 
> >>> +#ifndef	CONFIG_MACH_EFIKASB
> >> 
> >> It is better to have the check consistent in the file. You mix #ifdef
> >> CONFIG_MACH_EFIKAMX  with #ifndef CONFIG_MACH_EFIKASB, that is the same.
> > 
> > It expresses the intention much better IMO. And see above -- mx53 efika
> > in the works.
> 
> Personally I find confusing if sometimes an #ifdef is used and the next
> time #ifndef with the opposite CONFIG is taken, and both part of code
> are compiled at the same time.
> 
> >> At the moment, the #ifdef seems redundant. You hard-code the efikasb
> >> revision to zero, and then get_efika_rev() is always smaller as
> >> EFIKAMX_BOARD_REV_12. What about to introduce a macro such as board_is()
> >> to increase readability ?
> > 
> > Yes it would, but it'd also increase code size.
> 
> I let you decide.
> 
> >>> +#else
> >>> +	gd->bd->bi_arch_number = MACH_TYPE_MX51_EFIKASB;
> >>> +#endif
> >>> 
> >>>  	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
> >> 
> >> Can we use the new rule to set up the MACH-ID ? You can move the #ifdef
> >> inside config.h and let common code to set it.
> > 
> > Can we do that in a subsequent patch ?
> 
> Surely, you can add a patch to this patchset.

Eventually, yes ... not today though.

> 
> Best regards,
> Stefano Babic