[U-Boot] [PATCH] Added support for the Guntermann & Drunck PowerPC 440 EP/GR ETX module.
Wolfgang Denk
wd at denx.de
Thu Nov 27 16:48:34 CET 2008
Dear Dirk Eibach,
In message <1227789406-8800-1-git-send-email-eibach at gdsys.de> you wrote:
>
> Subject: Added support for the Guntermann & Drunck PowerPC 440 EP/GR ETX module.
Please keep the subject well under 70 characters so it makes a good
title line for the commit message.
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -133,6 +133,7 @@ Jon Diekema <jon.diekema at smiths-aerospace.com>
> Dirk Eibach <eibach at gdsys.de>
>
> neo PPC405EP
> + gdppc440etx PPC440EP/GR
Please keep lists sorted.
> diff --git a/board/gdsys/gdppc440etx/gdppc440etx.c b/board/gdsys/gdppc440etx/gdppc440etx.c
> new file mode 100644
> index 0000000..8903b61
> --- /dev/null
> +++ b/board/gdsys/gdppc440etx/gdppc440etx.c
> @@ -0,0 +1,494 @@
> +/*
> + * (C) Copyright 2008
> + * Dirk Eibach, Guntermann & Drunck GmbH, eibach at gdsys.de
> + *
You claim exclusive copyright for this file. However, large parts of
this file are verbatim copies from elsewhere. For example, the PCI
code has extensively been "borrowed" from other boards. This is
perfectly legal, but please make sure to correctly attribute such
code. Otherwise the (legal) copying into (illegal) stealing.
> + /*clear tmrclk divisor */
> + *(unsigned char *)(CONFIG_SYS_BCSR_BASE | 0x04) = 0x00;
> +
> + /*enable ethernet */
> + *(unsigned char *)(CONFIG_SYS_BCSR_BASE | 0x08) = 0xf0;
> +
> + /*get rid of flash write protect */
> + *(unsigned char *)(CONFIG_SYS_BCSR_BASE | 0x07) = 0x00;
Please use I/O accessor functions/macros to write peripherals.
> +int misc_init_r (void)
> +{
> + uint pbcr;
> + int size_val = 0;
> +
> + /* Re-do sizing to get full correct info */
> + mtdcr(ebccfga, pb0cr);
> + pbcr = mfdcr(ebccfgd);
> + switch (gd->bd->bi_flashsize) {
> + case 1 << 20:
> + size_val = 0;
> + break;
> + case 2 << 20:
> + size_val = 1;
> + break;
> + case 4 << 20:
> + size_val = 2;
> + break;
> + case 8 << 20:
> + size_val = 3;
> + break;
> + case 16 << 20:
> + size_val = 4;
> + break;
> + case 32 << 20:
> + size_val = 5;
> + break;
> + case 64 << 20:
> + size_val = 6;
> + break;
> + case 128 << 20:
> + size_val = 7;
> + break;
> + }
How about providing a "default" entry?
And - wouldn't it be easier to drop the while switch and use
uint sz = (gd->bd->bi_flashsize >> 20);
for (size_val=0; (sz & 1) == 0; ++size_val)
sz >>= 1;
instead?
> + pbcr = (pbcr & 0x0001ffff) | gd->bd->bi_flashstart | (size_val << 17);
...or combine the >> 20 above with the << 17 here ?
> + printf("Board: GDPPC440ETX - Guntermann & Drunck PPC440EP/GR ETX-module");
> +
> + rev = in_8((void *)(CONFIG_SYS_BCSR_BASE + 0));
> + val = in_8((void *)(CONFIG_SYS_BCSR_BASE + 5)) & CONFIG_SYS_BCSR5_PCI66EN;
Line length?
> +/*************************************************************************
> + * initdram -- doesn't use serial presence detect.
> + *
> + * Assumes: 256 MB, ECC, non-registered
Do not hard-code sizes. Use aut-sizing, please.
> +void sdram_tr1_set(int ram_address, int* tr1_value)
> +{
> + int i;
> + int j, k;
> + volatile unsigned int* ram_pointer = (unsigned int*)ram_address;
> + int first_good = -1, last_bad = 0x1ff;
> +
> + unsigned long test[NUM_TRIES] = {
> + 0x00000000, 0x00000000, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0x00000000, 0x00000000, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0x00000000, 0x00000000,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0x00000000, 0x00000000,
> + 0xAAAAAAAA, 0xAAAAAAAA, 0x55555555, 0x55555555,
> + 0xAAAAAAAA, 0xAAAAAAAA, 0x55555555, 0x55555555,
> + 0x55555555, 0x55555555, 0xAAAAAAAA, 0xAAAAAAAA,
> + 0x55555555, 0x55555555, 0xAAAAAAAA, 0xAAAAAAAA,
> + 0xA5A5A5A5, 0xA5A5A5A5, 0x5A5A5A5A, 0x5A5A5A5A,
> + 0xA5A5A5A5, 0xA5A5A5A5, 0x5A5A5A5A, 0x5A5A5A5A,
> + 0x5A5A5A5A, 0x5A5A5A5A, 0xA5A5A5A5, 0xA5A5A5A5,
> + 0x5A5A5A5A, 0x5A5A5A5A, 0xA5A5A5A5, 0xA5A5A5A5,
> + 0xAA55AA55, 0xAA55AA55, 0x55AA55AA, 0x55AA55AA,
> + 0xAA55AA55, 0xAA55AA55, 0x55AA55AA, 0x55AA55AA,
> + 0x55AA55AA, 0x55AA55AA, 0xAA55AA55, 0xAA55AA55,
> + 0x55AA55AA, 0x55AA55AA, 0xAA55AA55, 0xAA55AA55 };
> +
> + /* go through all possible SDRAM0_TR1[RDCT] values */
> + for (i=0; i<=0x1ff; i++) {
> + /* set the current value for TR1 */
> + mtsdram(mem_tr1, (0x80800800 | i));
> +
> + /* write values */
> + for (j=0; j<NUM_TRIES; j++) {
> + ram_pointer[j] = test[j];
> +
> + /* clear any cache at ram location */
> + __asm__("dcbf 0,%0": :"r" (&ram_pointer[j]));
> + }
> +
> + /* read values back */
> + for (j=0; j<NUM_TRIES; j++) {
> + for (k=0; k<NUM_READS; k++) {
> + /* clear any cache at ram location */
> + __asm__("dcbf 0,%0": :"r" (&ram_pointer[j]));
> +
> + if (ram_pointer[j] != test[j])
> + break;
> + }
> +
> + /* read error */
> + if (k != NUM_READS) {
> + break;
> + }
> + }
Please do not implement yet another RAM test in U-Boot. We already
have enough of them. If you really want to test your RAM, that is. For
auto-sizing, you should use memsize() instead.
> +#if defined(CONFIG_PCI)
> +int is_pci_host(struct pci_controller *hose)
> +{
> + /* Bamboo is always configured as host. */
> + return (1);
> +}
> +#endif /* defined(CONFIG_PCI) */
Bamboo. Aha. That's where the code was copied from ;-)
You might want to fix that, though.
> +
> +/*************************************************************************
> + * hw_watchdog_reset
> + *
> + * This routine is called to reset (keep alive) the watchdog timer
> + *
> + ************************************************************************/
> +#if defined(CONFIG_HW_WATCHDOG)
> +void hw_watchdog_reset(void)
> +{
> +
> +}
> +#endif
Please do not add dead code - either you have a WD, then it needs real
reset code, or you don't, then omit this all together.
> diff --git a/board/gdsys/gdppc440etx/init.S b/board/gdsys/gdppc440etx/init.S
> new file mode 100644
> index 0000000..f938236
> --- /dev/null
> +++ b/board/gdsys/gdppc440etx/init.S
> @@ -0,0 +1,112 @@
> +/*
> +*
> +* See file CREDITS for list of people who contributed to this
> +* project.
Copyright entry missing.
> + /*
> + * BOOT_CS (FLASH) must be first. Before relocation SA_I can be off to use the
> + * speed up boot process. It is patched after relocation to enable SA_I
> + */
> + tlbentry( CONFIG_SYS_BOOT_BASE_ADDR, SZ_256M, CONFIG_SYS_BOOT_BASE_ADDR, 0, AC_R|AC_W|AC_X|SA_G/*|SA_I*/)
> +
> + /* TLB-entry for init-ram in dcache (SA_I must be turned off!) */
> + tlbentry( CONFIG_SYS_INIT_RAM_ADDR, SZ_64K, CONFIG_SYS_INIT_RAM_ADDR, 0, AC_R|AC_W|AC_X|SA_G )
> +
> + tlbentry( CONFIG_SYS_SDRAM_BASE, SZ_256M, CONFIG_SYS_SDRAM_BASE, 0, AC_R|AC_W|AC_X|SA_G|SA_I )
> + tlbentry( CONFIG_SYS_PCI_BASE, SZ_256M, CONFIG_SYS_PCI_BASE, 0, AC_R|AC_W|SA_G|SA_I )
> + tlbentry( CONFIG_SYS_NVRAM_BASE_ADDR, SZ_256M, CONFIG_SYS_NVRAM_BASE_ADDR, 0, AC_R|AC_W|AC_X|SA_W|SA_I )
> +
> + /* PCI */
> + tlbentry( CONFIG_SYS_PCI_MEMBASE, SZ_256M, CONFIG_SYS_PCI_MEMBASE, 0, AC_R|AC_W|SA_G|SA_I )
> + tlbentry( CONFIG_SYS_PCI_MEMBASE1, SZ_256M, CONFIG_SYS_PCI_MEMBASE1, 0, AC_R|AC_W|SA_G|SA_I )
> + tlbentry( CONFIG_SYS_PCI_MEMBASE2, SZ_256M, CONFIG_SYS_PCI_MEMBASE2, 0, AC_R|AC_W|SA_G|SA_I )
> + tlbentry( CONFIG_SYS_PCI_MEMBASE3, SZ_256M, CONFIG_SYS_PCI_MEMBASE3, 0, AC_R|AC_W|SA_G|SA_I )
Lines too long.
> diff --git a/common/Makefile b/common/Makefile
> index 6484b23..a04c3d9 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -70,7 +70,7 @@ COBJS-$(CONFIG_CMD_BOOTLDR) += cmd_bootldr.o
> COBJS-$(CONFIG_CMD_CACHE) += cmd_cache.o
> COBJS-$(CONFIG_CMD_CONSOLE) += cmd_console.o
> COBJS-$(CONFIG_CMD_CPLBINFO) += cmd_cplbinfo.o
> -COBJS-$(CONFIG_CMD_DATE) += cmd_date.o
> +#COBJS-$(CONFIG_CMD_DATE) += cmd_date.o
Grrrrghhh...
NEVER make any such changes to global files.
Why don;t you just remove the CONFIG_CMD_DATE from your board config
file if you don't want to have this feature????
> diff --git a/include/configs/gdppc440etx.h b/include/configs/gdppc440etx.h
> new file mode 100644
> index 0000000..d41a98a
> --- /dev/null
> +++ b/include/configs/gdppc440etx.h
...
> +#define CONFIG_BOARD_EARLY_INIT_F 1 /* Call board_early_init_f */
> +#define CONFIG_MISC_INIT_R 1 /* call misc_init_r() */
> +#if 0
> +#define CONFIG_BOARD_RESET 1 /* call board_reset() */
> +#endif
Please do not add dead code.
> +/*-----------------------------------------------------------------------
> + * Base addresses -- Note these are effective addresses where the
> + * actual resources get mapped (not physical addresses)
> + *----------------------------------------------------------------------*/
> +#define CONFIG_SYS_FLASH_BASE 0xfc000000 /* start of FLASH */
> +#define CONFIG_SYS_PCI_MEMBASE 0xa0000000 /* mapped pci memory*/
Lines too long (whole file).
> +#define CONFIG_SYS_PCI_SUBSYS_VENDORID 0x10e8 /* AMCC */
> +#define CONFIG_SYS_PCI_SUBSYS_ID 0xcafe /* Whatever */
Whetever? This doesn't look like valid settings to me...
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: wd at denx.de
Intel's new motto: United we stand. Divided we fall!
More information about the U-Boot
mailing list