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(" 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
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(" 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
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(" 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
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
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(" 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
[U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x
From: Lars Poeschel The board is named pcm051 and has this hardware: SOC: TI AM3359 DDR3-RAM: 2x MT41J256M8HX-15EIT:D 512MiB ETH 1: LAN8710AI SPI-Flash: W25Q64BVSSIG RTC: RV-4162-C7 I2C-EEPROM: CAT32WC32 NAND: MT29F4G08_VFPGA63 PMIC: TPS65910A3 LCD Supported: UART 1 MMC/SD ETH 1 USB I2C SPI Not yet supported: NAND RTC LCD Signed-off-by: Lars Poeschel --- 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 +++ 7 files changed, 810 insertions(+) create mode 100644 board/phytec/pcm051/Makefile create mode 100644 board/phytec/pcm051/board.c create mode 100644 board/phytec/pcm051/board.h create mode 100644 board/phytec/pcm051/mux.c create mode 100644 include/configs/pcm051.h diff --git a/arch/arm/include/asm/arch-am33xx/ddr_defs.h b/arch/arm/include/asm/arch-am33xx/ddr_defs.h index 8e69fb6..f95b332 100644 --- a/arch/arm/include/asm/arch-am33xx/ddr_defs.h +++ b/arch/arm/include/asm/arch-am33xx/ddr_defs.h @@ -65,6 +65,24 @@ #define MT41J128MJT125_PHY_FIFO_WE 0x100 #define MT41J128MJT125_IOCTRL_VALUE0x18B +/* Micron MT41J256M8HX-15E */ +#define MT41J256M8HX15E_EMIF_READ_LATENCY 0x06 +#define MT41J256M8HX15E_EMIF_TIM1 0x0888A39B +#define MT41J256M8HX15E_EMIF_TIM2 0x26337FDA +#define MT41J256M8HX15E_EMIF_TIM3 0x501F830F +#define MT41J256M8HX15E_EMIF_SDCFG 0x61C04B32 +#define MT41J256M8HX15E_EMIF_SDREF 0x093B +#define MT41J256M8HX15E_ZQ_CFG 0x50074BE4 +#define MT41J256M8HX15E_DLL_LOCK_DIFF 0x1 +#define MT41J256M8HX15E_RATIO 0x40 +#define MT41J256M8HX15E_INVERT_CLKOUT 0x1 +#define MT41J256M8HX15E_RD_DQS 0x3B +#define MT41J256M8HX15E_WR_DQS 0x85 +#define MT41J256M8HX15E_PHY_WR_DATA0xC1 +#define MT41J256M8HX15E_PHY_FIFO_WE0x100 +#define MT41J256M8HX15E_IOCTRL_VALUE 0x18B + + /** * Configure SDRAM */ diff --git a/board/phytec/pcm051/Makefile b/board/phytec/pcm051/Makefile new file mode 100644 index 000..67a87a1 --- /dev/null +++ b/board/phytec/pcm051/Makefile @@ -0,0 +1,46 @@ +# +# Makefile +# +# Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/ +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed "as is" WITHOUT ANY WARRANTY of any +# kind, whether express or implied; without even the implied warranty +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# + +include $(TOPDIR)/config.mk + +LIB= $(obj)lib$(BOARD).o + +ifdef CONFIG_SPL_BUILD +COBJS := mux.o +endif + +COBJS += board.o +SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) +SOBJS := $(addprefix $(obj),$(SOBJS)) + +$(LIB):$(obj).depend $(OBJS) $(SOBJS) + $(call cmd_link_o_target, $(OBJS) $(SOBJS)) + +clean: + rm -f $(SOBJS) $(OBJS) + +distclean: clean + rm -f $(LIB) core *.bak $(obj).depend + +# + +# defines $(obj).depend target +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +# diff --git a/board/phytec/pcm051/board.c b/board/phytec/pcm051/board.c new file mode 100644 index 000..d2fb60d --- /dev/null +++ b/board/phytec/pcm051/board.c @@ -0,0 +1,271 @@ +/* + * board.c + * + * Board functions for Phytec phyCORE-AM335x (pcm051) based boards + * + * Copyright (C) 2013 Lemonage Software GmbH + * Author Lars Poeschel + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR /PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "board.h" + +DECLARE_GLOBAL_DATA_PTR; + +static struct wd_timer *wdtimer = (struct wd_timer *)WDT_
