Dear Heiko Schocher,
In message <1294816806-32614-2-git-send-email...@denx.de> you wrote:
>
Global question: do we really need an CONFIG_DIGSY_REV5? Is there not
a way to determine the revision by probing the hardware? For example,
the RTC's show up at different addresses on the bus - but ther emight
be even easier ways?
> diff --git a/board/digsy_mtc/digsy_mtc.c b/board/digsy_mtc/digsy_mtc.c
> index cc6087b..2b0c574 100644
> --- a/board/digsy_mtc/digsy_mtc.c
> +++ b/board/digsy_mtc/digsy_mtc.c
....
> #endif /* CONFIG_IDE_RESET */
> +#endif /* CONFIG_CMD_IDE */
This looks wrong to me. You did not add a matching "#if" or
"#ifdef" anywhere?
> +/* Update the Flash Baseaddr settings */
> +int update_flash_size (int flash_size)
> +{
> + volatile struct mpc5xxx_mmap_ctl *mm =
> + (struct mpc5xxx_mmap_ctl *) CONFIG_SYS_MBAR;
> + flash_info_t *dev;
> + int i;
> + int size = 0;
> + unsigned long base = 0x0;
> + u32 *cs_reg = (u32 *)&mm->cs0_start;
> +
> + for (i = 0; i < 2; i++) {
> + dev = &flash_info[i];
> +
> + if (dev->size) {
> + /* calculate new base addr for this chipselect */
> + base -= dev->size;
> + out_be32(cs_reg, START_REG(base));
> + cs_reg++;
> + out_be32(cs_reg, STOP_REG(base, dev->size));
> + cs_reg++;
> + /* recalculate the sectoraddr in the cfi driver */
> + size += flash_get_size(base, i);
> + }
> + }
> +#if defined(CONFIG_DIGSY_REV5)
> + gd->bd->bi_flashstart = base;
> +#endif
Why is this #if needed? Why not always set bi_flashstart ?
> void ft_board_setup(void *blob, bd_t *bd)
> {
> ft_cpu_setup(blob, bd);
> + /* remove RTC */
> +#if defined(CONFIG_DIGSY_REV5)
> + ft_delete_node(blob, "dallas,ds1339");
> +#else
> + ft_delete_node(blob, "mc,rv3029c2");
> +#endif
You should add a comment here what you are doing, and why.
ft_delete_node() returns int - why do you ignore the return codes?
> +#if defined(CONFIG_SYS_UPDATE_FLASH_SIZE)
> + ft_adapt_flash_base(blob);
> +#endif
ft_adapt_flash_base() returns int - why do you ignore the return code?
> }
> #endif /* defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP) */
> -
> -#endif /* CONFIG_CMD_IDE */
Ah! So this is a bug fix?
> diff --git a/board/digsy_mtc/is45s16800a2.h b/board/digsy_mtc/is45s16800a2.h
> new file mode 100644
> index 0000000..029e6cd
> --- /dev/null
> +++ b/board/digsy_mtc/is45s16800a2.h
> @@ -0,0 +1,27 @@
> +/*
> + * (C) Copyright 2004-2009
> + * Mark Jonas, Freescale Semiconductor, mark.jo...@motorola.com.
Are you sure that Mark wrote any of this code?
> diff --git a/boards.cfg b/boards.cfg
> index 94b8745..9e1fc14 100644
> --- a/boards.cfg
> +++ b/boards.cfg
> @@ -241,6 +241,9 @@ cm5200 powerpc mpc5xxx
> digsy_mtc powerpc mpc5xxx digsy_mtc
> digsy_mtc_LOWBOOT powerpc mpc5xxx digsy_mtc -
> - digsy_mtc:SYS_TEXT_BASE=0xFF000000
> digsy_mtc_RAMBOOT powerpc mpc5xxx digsy_mtc -
> - digsy_mtc:SYS_TEXT_BASE=0x00100000
> +digsy_mtc_rev5 powerpc mpc5xxx digsy_mtc -
> - digsy_mtc:DIGSY_REV5
> +digsy_mtc_rev5_LOWBOOT powerpc mpc5xxx digsy_mtc -
> - digsy_mtc:SYS_TEXT_BASE=0xFF000000,DIGSY_REV5
> +digsy_mtc_rev5_RAMBOOT powerpc mpc5xxx digsy_mtc -
> - digsy_mtc:SYS_TEXT_BASE=0x00100000,DIGSY_REV5
Do we really need all these 6 configurations for the digsy_mtc board?
Are all of them actively being used?
> diff --git a/doc/README.cfi b/doc/README.cfi
> new file mode 100644
> index 0000000..fa35108
> --- /dev/null
> +++ b/doc/README.cfi
> @@ -0,0 +1,15 @@
> +known issues:
> +
> +using M29W128GH from Numonyx:
> +
> +You need to add a board specific flash_cmd_reset() function
> +for this chip to work correctly. Something like this should
> +work (tested on the digsy_mtc board):
> +
> +void flash_cmd_reset(flash_info_t *info)
> +{
> + flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
> +}
Stefan, can you please send an explicit ACK for this part?
> +#if defined(CONFIG_DIGSY_REV5)
> +#define CONFIG_SYS_FLASH_BASE 0xFE000000
> +#define CONFIG_SYS_FLASH_BASE_CS1 0xFC000000
> +#define CONFIG_SYS_MAX_FLASH_BANKS 2
> +#define CONFIG_SYS_FLASH_BANKS_LIST { CONFIG_SYS_FLASH_BASE_CS1, \
> + CONFIG_SYS_FLASH_BASE}
> +#define CONFIG_SYS_UPDATE_FLASH_SIZE
> +#define CONFIG_FDT_FIXUP_NOR_FLASH_SIZE
> +#else
> #define CONFIG_SYS_FLASH_BASE 0xFF000000
> -#define CONFIG_SYS_FLASH_SIZE 0x01000000
> -
> #define CONFIG_SYS_MAX_FLASH_BANKS 1
> +#define CONFIG_SYS_FLASH_BANKS_LIST { CONFIG_SYS_FLASH_BASE }
> +#endif
Is this really needed? I understand you map the flash at the end of
the address space? I used to use code like this in similar
situations:
#define CONFIG_SYS_FLASH_BASE (0-flash_info[0].size)
This will auto-adjust depending on the actual size of the flash, and
avoids all these ifdef's. Maybe you can do something similar?
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
"The good Christian should beware of mathematicians and all those who
make empty prophecies. The danger already exists that mathematicians
have made a covenant with the devil to darken the spirit and confine
man in the bonds of Hell." - Saint Augustine
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
