[U-Boot] [PATCH v2 8/8] lpc32xx: add support for board work_92105

[Stefan Roese <sr@denx.de>]

Hi Albert,

On 13.02.2015 12:08, Albert ARIBAUD wrote:
>>> +++ b/board/work-microwave/work_92105/README
>
>>> +This board has SPL support, and uses the LPC32XX boot image format.
>>> +Once the U-Boot target "work_92105" is built, the following two files
>>> +can be flashed:
>>> +
>>> +	spl/lpc32xx-spl.bin at offset 0x0
>>> +	u-boot.bin at offset 0x40000
>>
>> Really u-boot.bin? Why don't you use u-boot.img with the header / CRC
>> instead? This is commonly used by other SPL platforms.
>
> u-boot.bin (headerless) is what the old process used, so I went for the
> least change path, but of course u-boot.img works too. I'll fix the
> text to mention u-boot.img and just note that u-boot.bin would work too
> but would not be checked for integrity.

Good. Thanks.

> (plus I need to fix the SPL image name anyway, since I changed it
> when switching to mkimage.)
>
>> And wouldn't it be nice to combine (concatenate) those two images into
>> one image that can be flashed to the board? There are already some
>> Makefile rules for doing this.
>
> Actually it would have to concatenate two (redundant) copies of the SPL
> image then one of U-Boot, bu yes, you're right. I'll look into it --
> and add notes to this effect in the board README file.

Thanks.

>>> +++ b/include/configs/work_92105.h
>
>>> +#define CONFIG_ETHADDR			00:12:B4:00:AF:FE
>>> +#define	CONFIG_OVERWRITE_ETHADDR_ONCE
>>
>> We usually don't allow MAC addresses to be set in the board config
>> files. Is this really needed?
>
> If CONFIG_ETH.*ADDR are not to be allowed, then maybe ./README should
> be updated to reflect this, and the more than 08 existing definitions
> in include/configs should be fixed. :)

Yes. We've been less strict with code reviews a few years ago. Thats 
most likely when those defines "slipped in".

> This is what was in the boards's default config initially. I'd rather
> leave it there in case the actual board provisioning process assumes
> the board to have this eth MAC address on very first boot. I can add a
> big red blinking comment though.

Okay.

Thanks,
Stefan