diff mbox

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

Message ID 1294816806-32614-2-git-send-email-hs@denx.de
State Changes Requested
Headers show

Commit Message

Heiko Schocher Jan. 12, 2011, 7:20 a.m. UTC

Comments

Wolfgang Denk Jan. 12, 2011, 7:59 a.m. UTC | #1
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@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
Heiko Schocher Jan. 12, 2011, 9:15 a.m. UTC | #2
Hello Wolfgang,

Wolfgang Denk wrote:
> 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?

Good question ... as I know, there is no possibility to detect the
hardware on an other way then over the RTC ... and I don;t want to
go this way ... what if the RTC is not responding?

>> 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?

The "#endif" is actual on the wrong place ...

>> +/* 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 ?

It is set in arch/powerpc/lib/board.c before calling update_flash_size(),
so this adaption is only needed, if the baseaddr is changing on different
flash sizes, what is valid for the rev5 board ...

>>  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.

Ok, you are right. (In the DTS there are two RTC nodes defined, and this
code removes the not existing one in the dts)

> ft_delete_node() returns int - why do you ignore the return codes?

You are right, add a warning, thanks.

>> +#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?

Yep, add here also a warning.

>>  }
>>  #endif /* defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP) */
>> -
>> -#endif /* CONFIG_CMD_IDE */
> 
> Ah!  So this is a bug fix?

Yep. Should I seperate this in an extra patch?

>> 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@motorola.com.
> 
> Are you sure that Mark wrote any of this code?

No, I just copied this from board/digsy_mtc/is42s16800a-7t.h,
so add my name to it.

>> 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?

Good question, I just used digsy_mtc_rev5, digsy_mtc_rev5_RAMBOOT,
maybe the LOWBOOT case could be deleted.

>> 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?

Hmm.. I have only one flash on the !rev5 board, so I need this
differentiation here, or?

Hmm.. after looking in code, can your proposal work?:
Is flash_info[0].size valid, when the cfi driver detects the flash?

I see in drivers/mtd/cfi_flash.c:

flash_info_t flash_info[CFI_MAX_FLASH_BANKS];   /* FLASH chips info */
[...]
static phys_addr_t __cfi_flash_bank_addr(int i)
{
        return ((phys_addr_t [])CONFIG_SYS_FLASH_BANKS_LIST)[i];
}
[...]
ulong flash_get_size (phys_addr_t base, int banknum)
{
        flash_info_t *info = &flash_info[banknum];
[...]
        info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);

        if (flash_detect_cfi (info, &qry)) {
[...]
unsigned long flash_init (void)
[...]
               if (!flash_detect_legacy(cfi_flash_bank_addr(i), i))
                        flash_get_size(cfi_flash_bank_addr(i), i);

and flash_info[0].size is initialized only in flash_get_size() after
detecting the flash (so, after accessing it with base = 0-flash_info[0].size)...
that must fail, or did I miss something?

bye,
Heiko
Wolfgang Denk Jan. 12, 2011, 9:37 a.m. UTC | #3
Dear Heiko Schocher,

In message <4D2D7122.60909@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?
> 
> Good question ... as I know, there is no possibility to detect the
> hardware on an other way then over the RTC ... and I don;t want to
> go this way ... what if the RTC is not responding?

Maybe you can ask the hardware guys again?

> >> +	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 ?
> 
> It is set in arch/powerpc/lib/board.c before calling update_flash_size(),
> so this adaption is only needed, if the baseaddr is changing on different
> flash sizes, what is valid for the rev5 board ...

It may not be needed, but I think it should compute the same result,
so you should be able to omit the "#if" and the "#endif".

If you should compute a different result, then I'd wonder if the code
is actually correct?

> >> -#endif /* CONFIG_CMD_IDE */
> > 
> > Ah!  So this is a bug fix?
> 
> Yep. Should I seperate this in an extra patch?

At least mention it in the commit message.

> Hmm.. I have only one flash on the !rev5 board, so I need this
> differentiation here, or?

Yes.

> Hmm.. after looking in code, can your proposal work?:
> Is flash_info[0].size valid, when the cfi driver detects the flash?

For the probing, a temporary address can be used.  You set up the
final mapping only after the sizes are knows, similar to what we do
when we have several banks of RAM.


Best regards,

Wolfgang Denk
Detlev Zundel Jan. 12, 2011, 10 a.m. UTC | #4
Hi Wolfgang,

> Dear Heiko Schocher,
>
> In message <4D2D7122.60909@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?
>> 
>> Good question ... as I know, there is no possibility to detect the
>> hardware on an other way then over the RTC ... and I don;t want to
>> go this way ... what if the RTC is not responding?
>
> Maybe you can ask the hardware guys again?

We did this and we are sure that there is no better way.

Cheers
  Detlev
Detlev Zundel Jan. 12, 2011, 10:20 a.m. UTC | #5
Hello Heiko,

[...]

>>> 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?
>
> Good question, I just used digsy_mtc_rev5, digsy_mtc_rev5_RAMBOOT,
> maybe the LOWBOOT case could be deleted.

Yes, the LOWBOOT cases can be removed.  The hardware can not be
configured in a way to use them.

Cheers
  Detlev
Heiko Schocher Jan. 12, 2011, 10:26 a.m. UTC | #6
Hello Wolfgang,

Wolfgang Denk wrote:
> In message <4D2D7122.60909@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?
>> Good question ... as I know, there is no possibility to detect the
>> hardware on an other way then over the RTC ... and I don;t want to
>> go this way ... what if the RTC is not responding?
> 
> Maybe you can ask the hardware guys again?

Ok. done. As Detlev mentioned in the meantime the "LOWBOOT" cases
can be removed, done.

>>>> +	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 ?
>> It is set in arch/powerpc/lib/board.c before calling update_flash_size(),
>> so this adaption is only needed, if the baseaddr is changing on different
>> flash sizes, what is valid for the rev5 board ...
> 
> It may not be needed, but I think it should compute the same result,
> so you should be able to omit the "#if" and the "#endif".
> 
> If you should compute a different result, then I'd wonder if the code
> is actually correct?

Yep, you are right, the "#if ... #endif" is not needed here.

>>>> -#endif /* CONFIG_CMD_IDE */
>>> Ah!  So this is a bug fix?
>> Yep. Should I seperate this in an extra patch?
> 
> At least mention it in the commit message.

Ok, add this to the commit message.

>> Hmm.. I have only one flash on the !rev5 board, so I need this
>> differentiation here, or?
> 
> Yes.
> 
>> Hmm.. after looking in code, can your proposal work?:
>> Is flash_info[0].size valid, when the cfi driver detects the flash?
> 
> For the probing, a temporary address can be used.  You set up the
> final mapping only after the sizes are knows, similar to what we do
> when we have several banks of RAM.

But thats exactly what I do with the defines in include/configs/digsy_mtc.h

#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}

and after detecting the real size through the cfi driver on this
temporary addresses, the flash_info gets updated with the new baseaddr
through the call:

int update_flash_size (int flash_size)
[...]
                if (dev->size) {
                        /* calculate new base addr for this chipselect */
                        base -= dev->size;
[...]
                        /* recalculate the sectoraddr in the cfi driver */
                        size += flash_get_size(base, i);

so after that, flash_info has the right values and I update the chip
selects in board specific code ...

bye,
Heiko
Stefan Roese Jan. 12, 2011, 1:39 p.m. UTC | #7
Hi Wolfgang & Heiko,

On Wednesday 12 January 2011 08:59:20 Wolfgang Denk wrote:
> > 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 we put something like this into this new README, we should make the 
description a bit better. Something like:

--- <snip> ---
The common CFI driver provides this weak default implementation for 
flash_cmd_reset():

void __flash_cmd_reset(flash_info_t *info)
{
        /*
         * We do not yet know what kind of commandset to use, so we issue
         * the reset command in both Intel and AMD variants, in the hope
         * that AMD flash roms ignore the Intel command.
         */
        flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
        flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
}
void flash_cmd_reset(flash_info_t *info)
        __attribute__((weak,alias("__flash_cmd_reset")));


Some flash chips seems to have trouble with this reset sequence. In this case 
the board specific code can override this weak default version with a board 
specific function. For example the digsy_mtc board equipped with the M29W128GH 
from Numonyx needs this version to function properly:

void flash_cmd_reset(flash_info_t *info)
{
        flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
}
--- <snip> ---


Heiko, if nobody objects then please include this version into your next patch 
version. Here my:

Signed-off-by: Stefan Roese <sr@denx.de>

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Wolfgang Denk Jan. 12, 2011, 1:42 p.m. UTC | #8
Dear Heiko Schocher,

In message <4D2D81C3.7010807@denx.de> you wrote:
> 
> > Maybe you can ask the hardware guys again?
> 
> Ok. done. As Detlev mentioned in the meantime the "LOWBOOT" cases
> can be removed, done.

Thanks.

> > For the probing, a temporary address can be used.  You set up the
> > final mapping only after the sizes are knows, similar to what we do
> > when we have several banks of RAM.
> 
> But thats exactly what I do with the defines in include/configs/digsy_mtc.h

Ah, OK. did not notice that.

Best regards,

Wolfgang Denk
diff mbox

Patch

difference to previous board version:
- M29W128GH flash from Numonyx
- SDRAM ISSI IS45S16800 (Option A2 105°C)
- rev5 uses RTC RV-3029-C2
- update cs0 and cs1 baseaddr and length
  depending on the detected flash size.
- added Werner Pfister <Pfister_Werner@intercontrol.de>
  as maintainer for the digsy board variants
- As the M29W128GH needs a special flash_cmd_reset()
  document that in the new file doc/README.cfi.

Signed-off-by: Heiko Schocher <hs@denx.de>
cc: Wolfgang Denk <hs@denx.de>
cc: Stefan Roese <sr@denx.de>
cc: Werner Pfister <Pfister_Werner@intercontrol.de>
---
 MAINTAINERS                    |    4 ++
 board/digsy_mtc/digsy_mtc.c    |  102 +++++++++++++++++++++++++++++++++++++++-
 board/digsy_mtc/is45s16800a2.h |   27 +++++++++++
 boards.cfg                     |    3 +
 doc/README.cfi                 |   15 ++++++
 include/configs/digsy_mtc.h    |   27 +++++++++-
 6 files changed, 173 insertions(+), 5 deletions(-)
 create mode 100644 board/digsy_mtc/is45s16800a2.h
 create mode 100644 doc/README.cfi

diff --git a/MAINTAINERS b/MAINTAINERS
index 553930a..bd18f7b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -340,6 +340,10 @@  Denis Peter <d.peter@mpl.ch>
 	MIP405		PPC4xx
 	PIP405		PPC4xx
 
+Werner Pfister <Pfister_Werner@intercontrol.de>
+	digsy_mtc	mpc5200
+	digsy_mtc_rev5	mpc5200
+
 Kim Phillips <kim.phillips@freescale.com>
 
 	MPC8349EMDS	MPC8349
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
@@ -39,12 +39,29 @@ 
 #include <asm/processor.h>
 #include <asm/io.h>
 #include "eeprom.h"
+#if defined(CONFIG_DIGSY_REV5)
+#include "is45s16800a2.h"
+#include <mtd/cfi_flash.h>
+#else
 #include "is42s16800a-7t.h"
+#endif
+#include <libfdt.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
 extern int usb_cpu_init(void);
 
+#if defined(CONFIG_DIGSY_REV5)
+/*
+ * The M29W128GH needs a specail reset command function,
+ * details see the doc/README.cfi file
+ */
+void flash_cmd_reset(flash_info_t *info)
+{
+	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
+}
+#endif
+
 #ifndef CONFIG_SYS_RAMBOOT
 static void sdram_start(int hi_addr)
 {
@@ -175,6 +192,9 @@  int checkboard(void)
 	char *s = getenv("serial#");
 
 	puts ("Board: InterControl digsyMTC");
+#if defined(CONFIG_DIGSY_REV5)
+	puts (" rev5");
+#endif
 	if (s != NULL) {
 		puts(", ");
 		puts(s);
@@ -305,12 +325,90 @@  void ide_set_reset(int idereset)
 	setbits_be32((void *)MPC5XXX_WU_GPIO_ENABLE, (1 << 25));
 }
 #endif /* CONFIG_IDE_RESET */
+#endif /* CONFIG_CMD_IDE */
 
 #if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
+static int ft_delete_node(void *fdt, const char *compat)
+{
+	int off = -1;
+
+	off = fdt_node_offset_by_compatible(fdt, -1, compat);
+	if (off < 0)
+		return off;
+
+	return(fdt_del_node(fdt, off));
+}
+#if defined(CONFIG_SYS_UPDATE_FLASH_SIZE)
+static int ft_adapt_flash_base(void *blob)
+{
+	flash_info_t	*dev = &flash_info[0];
+	int off;
+	struct fdt_property *prop;
+	int len;
+	u32 *reg, *reg2;
+
+	off = fdt_node_offset_by_compatible(blob, -1, "fsl,mpc5200b-lpb");
+	if (off < 0)
+		return off;
+
+	/* found compatible property */
+	prop = fdt_get_property_w(blob, off, "ranges", &len);
+	if (prop) {
+		reg = reg2 = (u32 *)&prop->data[0];
+
+		reg[2] = dev->start[0];
+		reg[3] = dev->size;
+		fdt_setprop(blob, off, "ranges", reg2, len);
+	}
+
+	return 0;
+}
+
+extern ulong flash_get_size (phys_addr_t base, int banknum);
+
+/* 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
+	return 0;
+}
+#endif /* defined(CONFIG_SYS_UPDATE_FLASH_SIZE) */
+
 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
+#if defined(CONFIG_SYS_UPDATE_FLASH_SIZE)
+	ft_adapt_flash_base(blob);
+#endif
 }
 #endif /* defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP) */
-
-#endif /* CONFIG_CMD_IDE */
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@motorola.com.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#define SDRAM_MODE	0x00CD0000
+#define SDRAM_CONTROL	0x50470000
+#define SDRAM_CONFIG1	0xD2322900
+#define SDRAM_CONFIG2	0x8AD70000
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
 galaxy5200                   powerpc     mpc5xxx     galaxy5200          -              -           galaxy5200:galaxy5200
 galaxy5200_LOWBOOT           powerpc     mpc5xxx     galaxy5200          -              -           galaxy5200:galaxy5200_LOWBOOT
 icecube_5200                 powerpc     mpc5xxx     icecube             -              -           IceCube
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);
+}
+
+see also:
+http://www.mail-archive.com/u-boot@lists.denx.de/msg24368.html
diff --git a/include/configs/digsy_mtc.h b/include/configs/digsy_mtc.h
index d541160..bfbec6a 100644
--- a/include/configs/digsy_mtc.h
+++ b/include/configs/digsy_mtc.h
@@ -249,9 +249,14 @@ 
 /*
  * RTC configuration
  */
+#if defined(CONFIG_DIGSY_REV5)
+#define CONFIG_SYS_I2C_RTC_ADDR	0x56
+#define CONFIG_RTC_RV3029
+#else
 #define CONFIG_RTC_DS1337
 #define CONFIG_SYS_I2C_RTC_ADDR	0x68
 #define CONFIG_SYS_DS1339_TCR_VAL	0xAB	/* diode + 4k resistor */
+#endif
 
 /*
  * Flash configuration
@@ -259,14 +264,24 @@ 
 #define	CONFIG_SYS_FLASH_CFI		1
 #define	CONFIG_FLASH_CFI_DRIVER	1
 
+#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
+
 #define CONFIG_SYS_MAX_FLASH_SECT	256
 #define CONFIG_FLASH_16BIT
 #define CONFIG_SYS_FLASH_CFI_WIDTH FLASH_CFI_16BIT
-#define CONFIG_SYS_FLASH_BANKS_LIST	{ CONFIG_SYS_FLASH_BASE }
+#define CONFIG_SYS_FLASH_SIZE	0x01000000
 #define CONFIG_SYS_FLASH_ERASE_TOUT	240000
 #define CONFIG_SYS_FLASH_WRITE_TOUT	500
 
@@ -409,6 +424,12 @@ 
 #define CONFIG_SYS_CS0_SIZE		CONFIG_SYS_FLASH_SIZE
 #define CONFIG_SYS_CS0_CFG		0x0002DD00
 
+#if defined(CONFIG_DIGSY_REV5)
+#define CONFIG_SYS_CS1_START		CONFIG_SYS_FLASH_BASE_CS1
+#define CONFIG_SYS_CS1_SIZE		CONFIG_SYS_FLASH_SIZE
+#define CONFIG_SYS_CS1_CFG		0x0002DD00
+#endif
+
 #define CONFIG_SYS_CS_BURST		0x00000000
 #define CONFIG_SYS_CS_DEADCYCLE	0x11111111