On Tue, Jan 08, 2013 at 07:58:37PM +0100, Wolfgang Denk wrote:
Dear Lars,
In message 1357663926-15937-2-git-send-email-larsi@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.