[U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x

[Lars Poeschel]

Hi Wolfgang, hi Tom,

as I almost have the changes requested by Wolfgang in place, you two can choose, which version I should resubmit. ;)

Am 09.01.2013 um 20:34 schrieb Tom Rini:

>>> +static void rtc32k_enable(void)
>>> +{
>>> +	struct rtc_regs *rtc = (struct rtc_regs *)AM335X_RTC_BASE;
>>> +
>>> +	/*
>>> +	 * Unlock the RTC's registers.  For more details please see the
>>> +	 * RTC_SS section of the TRM.  In order to unlock we need to
>>> +	 * write these specific values (keys) in this order.
>>> +	 */
>>> +	writel(0x83e70b13, &rtc->kick0r);
>>> +	writel(0x95a4f1e0, &rtc->kick1r);
>> 
>> These magic numbers should probbly be moved to some header file ?
> 
> This is a copy/paste from the am335x board.c file.  I specifically did a
> long comment to explain what / where these values come from because that
> (to me) is more helpful than
> #define AM33X_RTC_KICK0R_KEY 0x...
> #define AM33X_RTC_KICK1R_KEY 0x...

davinci does it this way. I would prefer Tom's variant having the values right in place.

>>> +	if (!eth_getenv_enetaddr("ethaddr", mac_addr)) {
>>> +		debug("<ethaddr> not set. Reading from E-fuse\n");
>> 
>> This should be a printf().  Any such automatic changes are always
>> worth to be told explicitly.
> 
> This is the normal case of asking the hardware what MAC it shipped with.
> Why do we want to tell the user the expected has happened?

Here I'd prefer the printf variant, because for a common user it is not obivous, that it is reading the MAC from efuse. So this message is helpful.

>>> +			goto try_usbether;
>> ...
>>> +#endif
>>> +try_usbether:
>> 
>> Did you ever try building without CONFIG_DRIVER_TI_CPSW set?  I bet
>> this causes a few compiler warnings. [Hint: You define a label, but
>> don't use it anywhere.]
> 
> It's a valid question if this board uses the CPSW eth or not, yes.

Yes, the board uses CPSW, but here I would prefer having the try_usbether label inside the ifdef.

>>> +#define CONFIG_ENV_IS_NOWHERE
>> 
>> Really?
> 
> Until they add NAND (which just now hit mainline), yes.

This is a very valuable information, thanks! 

Regards,
Lars