[U-Boot] [PATCH 2/2] mpc5200, digsy_mtc: add support for rev5 board version
Heiko Schocher
hs at denx.de
Wed Jan 12 10:15:14 CET 2011
Hello Wolfgang,
Wolfgang Denk wrote:
> In message <1294816806-32614-2-git-send-email-hs at 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 at 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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
More information about the U-Boot
mailing list