[U-Boot] [PATCH v11 1/6] arm: add Faraday ARMv5TE cores support

[Kuo-Jung Su]

2014-03-26 14:47 GMT+08:00 Wolfgang Denk <wd at denx.de>:
> Dear Kuo-Jung Su,
>
> In message <1395813799-3672-2-git-send-email-dantesu at gmail.com> you wrote:
>> From: Kuo-Jung Su <dantesu at faraday-tech.com>
>>
>> Here is the list of verified cores:
>>
>> 1. FA606TE (ARMv5TE, no mmu)
>> 2. FA626TE (ARMv5TE)
> ...
>> diff --git a/include/configs/faraday-common.h b/include/configs/faraday-common.h
>> new file mode 100644
>> index 0000000..546c0a9
>> --- /dev/null
>> +++ b/include/configs/faraday-common.h
> ...
>> +#ifndef CONFIG_ETHADDR
>> +#define CONFIG_ETHADDR              00:41:71:00:00:50
>> +#endif
>> +
>> +#ifndef CONFIG_IPADDR
>> +#define CONFIG_IPADDR               10.0.0.192
>> +#endif
>> +
>> +#ifndef CONFIG_NETMASK
>> +#define CONFIG_NETMASK              255.255.255.0
>> +#endif
>> +
>> +#ifndef CONFIG_SERVERIP
>> +#define CONFIG_SERVERIP             10.0.0.128
>> +#endif
>
> We do not allow such static network configuration.  Especially
> assigning the same MAC address to all devices is deadly.  Also,
> the address is not an officially assigned nor a local one.
> Please remove all this code.
>

Got it, thanks

>> +# endif
>> +# ifndef CONFIG_G_DNL_VENDOR_NUM
>> +# define CONFIG_G_DNL_VENDOR_NUM    0x1d50  /* OpenMoko */
>> +# endif
>
> This looks wrong to me?
>
>> +# ifndef CONFIG_G_DNL_PRODUCT_NUM
>> +# define CONFIG_G_DNL_PRODUCT_NUM   0x5119
>> +# endif
>
> Is this a valid ID?
>

By stealing the OpenMoko's vendor id & product id, I could directly use the
OpenMoko dfu-util without any modifications.

And unfortunately we don't have a registered OUI for Faraday Technology, so
I'll remove this ID in the next release.

>> +/* Console I/O Buffer Size */
>> +#define CONFIG_SYS_CBSIZE           256
>> +
>> +/* Max number of command args */
>> +#define CONFIG_SYS_MAXARGS          32
>
> You use a large number of args with a tiny console buffer?  This looks
> suspicious.  Please check.
>

Got it, thanks

>> +#define CONFIG_CMD_AUTOSCRIPT   /* support autoscript */
>
> This has been removed years ago.  Please use CONFIG_CMD_SOURCE
> instead.
>

Got it, thanks



-- 
Best wishes,
Kuo-Jung Su