[U-Boot] [PATCH 2/2] mpc5200, digsy_mtc: add support for rev5 board version

[Wolfgang Denk <wd@denx.de>]

Dear Heiko Schocher,

In message <1294816806-32614-2-git-send-email-hs@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.jonas at 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: wd at 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