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

[Heiko Schocher <hs@denx.de>]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany