[U-Boot] [PATCH 1/1] MX5:MX53: support for freescale MX53LOCO board

[Jason Liu]

Hi, Wolfgang,

2011/4/14 Wolfgang Denk <wd at denx.de>:
> Dear Jason Liu,
>
> In message <1298283316-3443-1-git-send-email-r64343 at freescale.com> you wrote:
>> This patch add initial support for freescale MX53LOCO board.
>> Network(FEC),SD/MMC, UART have been supported by this patch.
>>
>> Signed-off-by: Jason Liu <r64343 at freescale.com>
> ...
>> +int dram_init(void)
>> +{
>> +     gd->ram_size = get_ram_size((volatile void *)CONFIG_SYS_SDRAM_BASE,
>> +                             PHYS_SDRAM_1_SIZE);
>> +
>> +     return 0;
>> +}
>> +void dram_init_banksize(void)
>> +{
>> +     gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
>> +     gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
>> +
>> +     gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
>> +     gd->bd->bi_dram[1].size = PHYS_SDRAM_2_SIZE;
>> +}
>
> This is apparently wrong: in dram_init() you run get_ram_size() only
> over the first bank of memory, but dram_init_banksize() says you have
> two of them ??

Yes, I have already thought about it before I submit patches. The fact is that
there is there are two DDR chips connected to CS0 and CS1 of MX53 on loco
board, but each DDR chip is 512MB, but CS0 and CS1 can support up to 1GB,
which means the memory space is not continuous, when set:
gd->ram_size = get_ram_size((volatile void *)CONFIG_SYS_SDRAM_BASE,
                          PHYS_SDRAM_1_SIZE + PHYS_SDRAM_2_SIZE);
it will return the same as:

gd->ram_size = get_ram_size((volatile void *)CONFIG_SYS_SDRAM_BASE,
                            PHYS_SDRAM_1_SIZE);


I think the gd->ram_size is just used for uboot, but
dram_init_banksize will tell
linux kernel about the real memory layout.

If you still think that I really need fix it, I will do it.

>
>
>> +int board_mmc_getcd(u8 *cd, struct mmc *mmc)
>> +{
>> +     struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
>> +
>> +     if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR)
>> +             *cd = mxc_gpio_get(77); /*GPIO3_13*/
>> +     else
>> +             *cd = mxc_gpio_get(75); /*GPIO3_11*/
>
> Is this just data or a device?  If the latter should be the case you
> mustuse I/O accessors instead.

It's data not a device here, so no need use I/O accessors.

>
> ...
>> +     puts("Board: MX53LOCO [");
>> +
>> +     cause = src_regs->srsr;
>> +     switch (cause) {
>> +     case 0x0001:
>> +             printf("POR");
>> +             break;
>> +     case 0x0009:
>> +             printf("RST");
>> +             break;
>> +     case 0x0010:
>> +     case 0x0011:
>> +             printf("WDOG");
>> +             break;
>> +     default:
>> +             printf("unknown");
>> +     }
>
> This has been discussed before: this code should be factored out into
> common code.

Yes, I think I can send out another clean up patch for it to fix all
the boards including
mx31/35/51/53 later.

>
> ...
>> +#define CONFIG_CMDLINE_TAG           1       /* enable passing of ATAGs */
>> +#define CONFIG_REVISION_TAG          1
>> +#define CONFIG_SETUP_MEMORY_TAGS     1
>> +#define CONFIG_INITRD_TAG            1
>
> Please remove the '1' from all #defines that just enable features,
> i.  e. where no specific numeric value is needed.

Yes, I can send out another clean up patch to fix all the i.mx board config file

>
>
>> +#define CONFIG_SYS_MEMTEST_START       0x70000000
>> +#define CONFIG_SYS_MEMTEST_END         0x10000
>
> Has this actually been tested?

Yes, I have tested with mtest command.

BTW, this patch set has been send out 2 months ago and Stefano has
send out pull request
to Albert and Albert has pulled in already, do you agree I send out
another patch to address
your comments? Thanks for your consideration.

BR,
Jason

>
> 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
> People seldom know what they want until you give them what  they  ask
> for.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>