[U-Boot] [PATCH 4/4] ARM: atmel: add sama5d3xek support

Bo Shen
2013-03-05T02:03:57Z

Hi Tom,

On 3/4/2013 23:14, Tom Rini wrote:
> On Thu, Feb 28, 2013 at 03:00:47PM +0800, Bo Shen wrote:
>
>> Add sama5d3xek support with following feature
>> - boot from NAND flash, PMECC support, 4bit ECC @ 512 bytes sector
>> - boot from SPI flash support
>> - boot from SD card support
>> - LCD support
>> - EMAC support
>> - USB support
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>
> Some minor comments:
>
> [snip]
>> + if (cpu_is_sama5d3())
>> + switch (extension_id) {
>> + case ARCH_EXID_SAMA5D31:
>> + return CONFIG_SYS_AT91_D31_CPU_NAME;
>> + case ARCH_EXID_SAMA5D33:
>> + return CONFIG_SYS_AT91_D33_CPU_NAME;
>> + case ARCH_EXID_SAMA5D34:
>> + return CONFIG_SYS_AT91_D34_CPU_NAME;
>> + case ARCH_EXID_SAMA5D35:
>> + return CONFIG_SYS_AT91_D35_CPU_NAME;
>> + default:
>> + return CONFIG_SYS_AT91_UNKNOWN_CPU;
>
> These aren't configurable, and are used once. Just put the strings
> here.

OK, I will use strings here directly here in next version.

>
>> @@ -0,0 +1,268 @@
>> +/*
>> + * Configuation settings for the SAMA5D3xEK board.
> [snip]
>> +#undef CONFIG_USE_IRQ /* we don't need IRQ/FIQ stuff */
>> +
>> +#undef CONFIG_CMDLINE_TAG /* enable passing of ATAGs */
>> +#undef CONFIG_SETUP_MEMORY_TAGS
>> +#undef CONFIG_INITRD_TAG
>
> Just leave these, and the other #undef's out.

You mean I need not to #undef these, because these are not defined, am I
right?

>> +/*
>> + * Command line configuration.
>> + */
>> +#include <config_cmd_default.h>
>> +#undef CONFIG_CMD_FPGA
>> +#undef CONFIG_CMD_IMI
>> +#undef CONFIG_CMD_IMLS
>> +#undef CONFIG_CMD_AUTOSCRIPT
>> +#undef CONFIG_CMD_LOADS
>
> These are fine to leave in 'tho.

These are no useful for us.
I will consider remove unneeded #undef

>> +#ifdef CONFIG_USE_IRQ
>> +#error CONFIG_USE_IRQ not supported
>> +#endif
>
> Just drop that part.

Ok, I will drop this in next version.

> And please check things with checkpatch.pl, I
> thought I saw a '#define<tab>FOO' in there. Thanks!

I have checked this patch with checkpatch.pl, and get no errors and no
warnings.

Best Regards,
Bo Shen