[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board
Wolfgang Grandegger
wg at grandegger.com
Mon Oct 19 15:42:27 CEST 2009
Wolfgang Denk wrote:
> Dear Wolfgang Grandegger,
>
> In message <20091019111913.802418590 at denx.de> you wrote:
>> This patch adds support for the board IPEK01 based on the MPC5200.
>> The Futjitsu Lime graphics controller is configured in 16 bpp mode.
>>
>> Signed-off-by: Wolfgang Grandegger <wg at grandegger.com>
[...]
>> Index: u-boot-mainline/board/ipek01/ipek01.c
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ u-boot-mainline/board/ipek01/ipek01.c 2009-10-19 13:17:17.527552105 +0200
> ...
>> +#ifndef CONFIG_SYS_RAMBOOT
>
> Is RAM-Boot actually a asupported mode of operation on this board, or
> are you just copuy & pasting dead code here?
If it's really dead code I'm going to prepare and send a patch removing
it for all boards. Would that be fine?
>> +static void sdram_start (int hi_addr)
>> +{
>> + long hi_addr_bit = hi_addr ? 0x01000000 : 0;
>> +
>> + /* unlock mode register */
>> + *(vu_long *) MPC5XXX_SDRAM_CTRL =
>> + SDRAM_CONTROL | 0x80000000 | hi_addr_bit;
>> + __asm__ volatile ("sync");
>
> Please use I/O accessors to write device registers (please fix
> globally).
Well, I borrowed known to work code from other boards. Unfortunately
there are no better examples (yet). Will fix, of course.
> ...
>> +phys_size_t initdram (int board_type)
>> +{
>> + ulong dramsize = 0;
>> + ulong dramsize2 = 0;
>> + uint svr, pvr;
>> +#ifndef CONFIG_SYS_RAMBOOT
>> + ulong test1, test2;
>> +
>> + /* setup SDRAM chip selects */
>> + *(vu_long *) MPC5XXX_SDRAM_CS0CFG = 0x0000001e; /* 2G at 0x0 */
>> + *(vu_long *) MPC5XXX_SDRAM_CS1CFG = 0x00000000; /* disabled */
>
> It might make sense to #define some constants in the board config
> file.
>
>> +
>> + /* setup config registers */
>> + *(vu_long *) MPC5XXX_SDRAM_CONFIG1 = SDRAM_CONFIG1;
>> + *(vu_long *) MPC5XXX_SDRAM_CONFIG2 = SDRAM_CONFIG2;
>> + __asm__ volatile ("sync");
>> +
>> +#if SDRAM_DDR
>
> I consider this bad style. In U-Boot, we usually use #ifdef, but this
> collides with your #define SDRAM_DDR 0 versus 1 below. I recommend to
> clean this up.
OK.
>> + /*
>> + * On MPC5200B we need to set the special configuration delay in the
>> + * DDR controller. Please refer to Freescale's AN3221 "MPC5200B SDRAM
>> + * Initialization and Configuration", 3.3.1 SDelay--MBAR + 0x0190:
>> + *
>> + * "The SDelay should be written to a value of 0x00000004. It is
>> + * required to account for changes caused by normal wafer processing
>> + * parameters."
>> + */
>> + svr = get_svr ();
>> + pvr = get_pvr ();
>> + if ((SVR_MJREV (svr) >= 2) && (PVR_MAJ (pvr) == 1) &&
>> + (PVR_MIN (pvr) == 4)) {
>> + *(vu_long *) MPC5XXX_SDRAM_SDELAY = 0x04;
>> + __asm__ volatile ("sync");
>> + }
>
> Is this test really needed? Are there versions of this board with
> pre-Rev. B silicon?
I would guess no. I have to check with the board provider.
>> +#if defined (CONFIG_CMD_IDE) && defined (CONFIG_IDE_RESET)
>> +
>> +void init_ide_reset (void)
>> +{
>> + debug ("init_ide_reset\n");
>> +}
>> +
>> +void ide_set_reset (int idereset)
>> +{
>> + debug ("ide_reset(%d)\n", idereset);
>> +}
>> +#endif /* defined (CONFIG_SYS_CMD_IDE) && defined (CONFIG_IDE_RESET) */
>
> This is just dead code. Please get rid of it.
>
>> +#ifdef CONFIG_VIDEO
>> +#define DISPLAY_WIDTH 800
>> +#define DISPLAY_HEIGHT 600
>
> This should go to the board config file.
OK.
>> +#define CONFIG_SYS_LIME_SRST (CONFIG_SYS_LIME_BASE + 0x01FC002C)
>> +#define CONFIG_SYS_LIME_CCF (CONFIG_SYS_LIME_BASE + 0x01FC0038)
>> +#define CONFIG_SYS_LIME_MMR (CONFIG_SYS_LIME_BASE + 0x01FCFFFC)
>> +/* Lime clock frequency */
>> +#define CONFIG_SYS_LIME_CLK 0x90000 /* geo 166MHz other 133MHz */
>> +/* SDRAM parameter */
>> +#define CONFIG_SYS_LIME_MMR_VALUE 0x41c767e3
>
> Please do not use base register plus offset. Use a proper C struct to
> describe the device registers.
I think this should be done for the mb862xx video driver first, which
does still use base register plus offset. But it would be nice if the
mb862xx video driver would make the register access functions public,
which could then be used here.
>> +#define CONFIG_SYS_LIME_CID (CONFIG_SYS_LIME_BASE + 0x01FC00F0)
>> +#define CONFIG_SYS_LIME_REV (CONFIG_SYS_LIME_BASE + 0x01FF8084)
>
> Ditto.
>
>> +int lime_probe (void)
>> +{
>> + uint reg;
>> +
>> + /* Try to access GDC ID/Revision registers */
>> + reg = in_be32 ((void *)CONFIG_SYS_LIME_CID);
>> + reg = in_be32 ((void *)CONFIG_SYS_LIME_CID);
>> + if (reg == 0x303) {
>> + reg = in_be32 ((void *)CONFIG_SYS_LIME_REV);
>> + reg = in_be32 ((void *)CONFIG_SYS_LIME_REV);
>> + reg = ((reg & ~0xff) == 0x20050100) ? 1 : 0;
>
> Please see above - using a proper C struct we can get rid of these
> casts.
>
>> +#if defined(CONFIG_CONSOLE_EXTRA_INFO)
>> +/*
>> + * Return text to be printed besides the logo.
>> + */
>> +void video_get_info_str (int line_number, char *info)
>> +{
>> + if (line_number == 1) {
>> + strcpy (info, " Board: IPEK01");
>> + } else {
>> + info[0] = '\0';
>> + }
>
> Please use TABs for indentation. And no braces are needed here.
Oops.
>> Index: u-boot-mainline/board/ipek01/mt46v16m16-75.h
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ u-boot-mainline/board/ipek01/mt46v16m16-75.h 2009-10-19 13:17:17.529553509 +0200
>
> Seems we are adding the 8th copy of this file?
>
> Can you please move this to a common place? Thanks.
>
>> +#define SDRAM_DDR 1 /* is DDR */
>
> Dangerous. See somment above.
>
>> Index: u-boot-mainline/board/ipek01/mt48lc16m16a2-75.h
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ u-boot-mainline/board/ipek01/mt48lc16m16a2-75.h 2009-10-19 13:17:17.530552255 +0200
>
> Seems we are adding the 9th copy of this file?
>
> Can you please move this to a common place? Thanks.
>
>> +#define SDRAM_DDR 0 /* is SDR */
>
> Very dangerous. See somment above.
I think only DDR-RAM is used. I will cleanup.
>> Index: u-boot-mainline/board/ipek01/u-boot.lds
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ u-boot-mainline/board/ipek01/u-boot.lds 2009-10-19 13:17:17.540334101 +0200
>
> Does this board really need a private linker script? I don't think so.
>
>> Index: u-boot-mainline/include/configs/ipek01.h
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ u-boot-mainline/include/configs/ipek01.h 2009-10-19 13:17:17.541552432 +0200
> ...
>> +#define CONFIG_MPC5200_DDR 1 /* ... use DDR RAM */
>
> Seems to be redundant with the SDRAM_DDR above. Please decide for one
> solution.
>
>
>> +#define CONFIG_CMD_DATE /* support for RTC, date/time...*/
>> +#define CONFIG_CMD_DHCP /* DHCP Support */
>> +#define CONFIG_CMD_FAT /* FAT support */
>> +#define CONFIG_CMD_I2C /* I2C serial bus support */
>> +#define CONFIG_CMD_IRQ /* irqinfo */
>> +#define CONFIG_CMD_IDE /* IDE harddisk support */
>> +#define CONFIG_CMD_MII /* MII support */
>> +#define CONFIG_CMD_PCI /* pciinfo */
>> +#define CONFIG_CMD_USB /* USB Support */
>
> Maybe you could sort this list? Thanks.
>
>> +#if (TEXT_BASE == 0xFC000000) /* Boot low */
>> +# define CONFIG_SYS_LOWBOOT 1
>> +#endif
>
> Needed or dead code?
I have to check.
>> + "rootpath=/opt/eldk41/ppc_6xx\0" \
>
> Is this intentional? Or did you just forget to s/41// ?
My mistake!
>
>> + "loadaddr=300000\0" \
>
> This is pretty low; be careful when your kernel grows a bit...
>
>> +#define OF_CPU "PowerPC,5200 at 0"
>> +#define OF_SOC "soc5200 at f0000000"
>> +#define OF_TBCLK (bd->bi_busfreq / 4)
>> +#define OF_STDOUT_PATH "/soc5200 at f0000000/serial at 2000"
>
> Is all this really needed?
Have to check.
>> +/* Disk-On-Chip currently not supported by U-Boot */
>> +#define CONFIG_SYS_DOC_BASE 0xE0000000
>> +#define CONFIG_SYS_DOC_SIZE 0x00100000
>
> Dead code? Please remove.
Linux may want to use Disk-On-Chip. But I have to check if it's present
on the board.
I will fix the other issues as well.
Wolfgang.
More information about the U-Boot
mailing list