Re: [U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
On Thu, Jan 10, 2013 at 08:30:09AM +0100, Lars Poeschel wrote:
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:
[snip]
+ 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.
OK, please also patch board/ti/am335x/board.c to also printf here then.
Other than that, yes, I'd be happy to see you make the other changes you
said. Just partially trying to help explain the SoC to Wolfgang :)
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
Hello Wolfgang,
thank you for your fast review!
On Tuesday 08 January 2013 at 19:58:37, Wolfgang Denk wrote:
+/* DDR RAM defines */
+#define DDR_CLK_MHZ303
Is this really correct? 303 ??
I am quite sure, I read this in a datasheet, but I can not find it anymore. I
set this to 333 now. mtest still works.
...
Do you plan to use this? Otherwise please just omit such dead code.
+#ifdef CONFIG_DRIVER_TI_CPSW
+static void cpsw_control(int enabled)
+{
+ /* VTP can be added here */
+
+ return;
+}
Ditto...
The cpsw driver needs a control function, otherwise the board crashes when
network initializes. On my board this is empty like on am335x_evm.
+#define CONFIG_ENV_OVERWRITE 1
Please do not define values for logical variables like this one;
please fix globally.
Fix globally ? Do you mean, I have to fix that for EVERY board that is in u-
boot, that defines CONFIG_ENV_OVERWRITE with a value to get my patch in?
There are a number of boards doing this wrong!
+#define CONFIG_ENV_IS_NOWHERE
Really?
Uuhm. Yes. At the moment I use this uEnv.txt file on sd-card, as I am not
able to use the NAND yet. The env should go to nand later.
Thanks for the other hints you gave. I will address this and send a version 2
soon.
Regards,
Lars
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
On Tue, Jan 08, 2013 at 07:58:37PM +0100, Wolfgang Denk wrote:
Dear Lars,
In message 1357663926-15937-2-git-send-email-la...@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_MHZ303
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
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
Dear Lars,
In message 1357663926-15937-2-git-send-email-la...@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 ??
+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 ?
+void s_init(void)
+{
+ /* WDT1 is already running when the bootloader gets control
+ * Disable it to avoid random resets
+ */
Incorrect multiline comment style.
+ writel(0x, wdtimer-wdtwspr);
More hard-wired magic values?
+ while (readl(wdtimer-wdtwwps) != 0x0)
+ ;
No timeout?
+ writel(0x, wdtimer-wdtwspr);
More hard-wired magic values?
+ while (readl(wdtimer-wdtwwps) != 0x0)
+ ;
No timeout?
+ while ((readl(uart_base-uartsyssts)
+ UART_CLK_RUNNING_MASK) != UART_CLK_RUNNING_MASK)
+ ;
Braces needed for multiline statement.
+#ifdef CONFIG_BOARD_LATE_INIT
+int board_late_init(void)
+{
+ return 0;
+}
+#endif
Do you plan to use this? Otherwise please just omit such dead code.
+#ifdef CONFIG_DRIVER_TI_CPSW
+static void cpsw_control(int enabled)
+{
+ /* VTP can be added here */
+
+ return;
+}
Ditto...
+ 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.
+ 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.]
+#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...
+#define CONFIG_ENV_OVERWRITE 1
Please do not define values for logical variables like this one;
please fix globally.
+#define CONFIG_ENV_IS_NOWHERE
Really?
Thanks!
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Crash programs fail because they are based on the theory that, with
nine women pregnant, you can get a baby a month. - Wernher von Braun
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
