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

[Tom Rini]

On Tue, Jan 08, 2013 at 07:58:37PM +0100, Wolfgang Denk wrote:
> Dear Lars,
> 
> In message <1357663926-15937-2-git-send-email-larsi at wh2.tu-dresden.de> you wrote:
> ...
> >  arch/arm/include/asm/arch-am33xx/ddr_defs.h |   18 ++
> >  board/phytec/pcm051/Makefile                |   46 ++++
> >  board/phytec/pcm051/board.c                 |  271 +++++++++++++++++++++++
> >  board/phytec/pcm051/board.h                 |   33 +++
> >  board/phytec/pcm051/mux.c                   |  133 ++++++++++++
> >  boards.cfg                                  |    1 +
> >  include/configs/pcm051.h                    |  308 +++++++++++++++++++++++++++
> 
> Please add an entry to the MAINTAINERS file.
> 
> Also, please run your patch through checkpatch - it complains about a
> number of style errors:
> 
> 	WARNING: Whitespace before semicolon
> 
> > +/* DDR RAM defines */
> > +#define DDR_CLK_MHZ		303
> 
> Is this really correct?  303 ??

Yes, it's really correct.

> > +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...

[snip]
> > +	while (readl(&wdtimer->wdtwwps) != 0x0)
> > +		;
> 
> No timeout?

No.  I argued with Marek about some of these before too.  In short, if
we don't suceed here, there's a catastrophic failure of the hardware.

> > +	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?

> > +			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.

> > +#define CONFIG_ENV_SIZE			(128 << 10)	/* 128 KiB */
> 
> Does this really make sense?  I doubt you will ever need more than 10%
> of this size - so you are just wasting a lot of time for checksumming
> unused memory...

ENV stored in NAND with page size of 128KiB is probably the reason for
this.

> > +#define CONFIG_ENV_IS_NOWHERE
> 
> Really?

Until they add NAND (which just now hit mainline), yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130109/156148d8/attachment.pgp>