[U-Boot] [PATCH 0/3] mpc52xx: IPEK01 board support

[U-Boot] [PATCH 0/3] mpc52xx: IPEK01 board support

[Wolfgang Grandegger]

Hello,

this patch add support for the IPEK01 MPC5200 based board.
It requires two patchs for the Fujitsu MB862xx driver:

[PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode
[PATCH 2/3] video: mb862xx: add option VIDEO_FB_16BPP_WORD_SWAP for IPEK01
[PATCH 3/3] mpc52xx: add support for the IPEK01 board

Wolfgang.

[U-Boot] [PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode

[Wolfgang Grandegger]

An embedded and charset-unspecified text was scrubbed...
Name: video-mb862xx-accel.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20091019/6772cf96/attachment.txt 

[U-Boot] [PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode

[Wolfgang Denk]

Dear Wolfgang Grandegger,

In message <20091019111913.427445638@denx.de> you wrote:
> The new IPEK01 board uses the X888RGB mode for the Lime graphics
> controller. For this mode video accelaration does not work. This patch
> makes the accelaration configurable via CONFIG_VIDEO_MB862xx_ACCEL,
> which is enabled for the lwmon5 and the socrates board for backward
> compatibility.

Why would you want to disable it for IPEK01? Accelaration seems to be
a good thing you don't give up if you don't have to, but I
cannot think of reasons why you would have to do without it?


> Index: u-boot-mainline/drivers/video/cfb_console.c
> ===================================================================
> --- u-boot-mainline.orig/drivers/video/cfb_console.c	2009-10-19 13:17:14.582303087 +0200
> +++ u-boot-mainline/drivers/video/cfb_console.c	2009-10-19 13:17:29.406303158 +0200

Please use git-format-patch to create patches.

> --- u-boot-mainline.orig/drivers/video/mb862xx.c	2009-10-19 13:17:14.582303087 +0200
> +++ u-boot-mainline/drivers/video/mb862xx.c	2009-10-19 13:17:17.467553012 +0200
...
> @@ -174,6 +178,14 @@
>  	DE_WR_FIFO (dev->winSizeY << 16 | dev->winSizeX);
>  	/* sync with SW access to framebuffer */
>  	de_wait ();
> +#else
> +	unsigned int i, *p;
> +
> +	i = dev->winSizeX * dev->winSizeY;
> +	p = (unsigned int *)dev->frameAdrs;
> +	while (i--)
> +		*p++ = 0;
> +#endif

Why don't you use memset() here?


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
Use the Force, Luke.

[U-Boot] [PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode

[Wolfgang Grandegger]

Wolfgang Denk wrote:
> Dear Wolfgang Grandegger,
> 
> In message <20091019111913.427445638@denx.de> you wrote:
>> The new IPEK01 board uses the X888RGB mode for the Lime graphics
>> controller. For this mode video accelaration does not work. This patch
>> makes the accelaration configurable via CONFIG_VIDEO_MB862xx_ACCEL,
>> which is enabled for the lwmon5 and the socrates board for backward
>> compatibility.
> 
> Why would you want to disable it for IPEK01? Accelaration seems to be
> a good thing you don't give up if you don't have to, but I
> cannot think of reasons why you would have to do without it?

Because acceleration does work with 16 bpp but *not* with 32 bpp. That's
the reason why we made it configurable. Well, this patch could be
dropped, because the BSP for the IPEK01 posted here uses now 16 bpp as well.

>> Index: u-boot-mainline/drivers/video/cfb_console.c
>> ===================================================================
>> --- u-boot-mainline.orig/drivers/video/cfb_console.c	2009-10-19 13:17:14.582303087 +0200
>> +++ u-boot-mainline/drivers/video/cfb_console.c	2009-10-19 13:17:29.406303158 +0200
> 
> Please use git-format-patch to create patches.

Why? Do you have any problems to apply these patches? I personally
(still) prefer using quilt for patch stack management.

>> --- u-boot-mainline.orig/drivers/video/mb862xx.c	2009-10-19 13:17:14.582303087 +0200
>> +++ u-boot-mainline/drivers/video/mb862xx.c	2009-10-19 13:17:17.467553012 +0200
> ...
>> @@ -174,6 +178,14 @@
>>  	DE_WR_FIFO (dev->winSizeY << 16 | dev->winSizeX);
>>  	/* sync with SW access to framebuffer */
>>  	de_wait ();
>> +#else
>> +	unsigned int i, *p;
>> +
>> +	i = dev->winSizeX * dev->winSizeY;
>> +	p = (unsigned int *)dev->frameAdrs;
>> +	while (i--)
>> +		*p++ = 0;
>> +#endif
> 
> Why don't you use memset() here?

Maybe to ensure that D32 accesses are performed. Anatolij might know?

Wolfgang.

[U-Boot] [PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode

[Wolfgang Denk]

Dear Wolfgang,

In message <4ADC5661.7050508@grandegger.com> you wrote:
>
> >> The new IPEK01 board uses the X888RGB mode for the Lime graphics
> >> controller. For this mode video accelaration does not work. This patch
> >> makes the accelaration configurable via CONFIG_VIDEO_MB862xx_ACCEL,
> >> which is enabled for the lwmon5 and the socrates board for backward
> >> compatibility.
> > 
> > Why would you want to disable it for IPEK01? Accelaration seems to be
> > a good thing you don't give up if you don't have to, but I
> > cannot think of reasons why you would have to do without it?
> 
> Because acceleration does work with 16 bpp but *not* with 32 bpp. That's
> the reason why we made it configurable. Well, this patch could be
> dropped, because the BSP for the IPEK01 posted here uses now 16 bpp as well.

Then please either mention this fact in the commit message (the
current one does not say anything about 16 versus 32 bit mode), or
realy drop the patch.

> >> --- u-boot-mainline.orig/drivers/video/cfb_console.c	2009-10-19 13:17:14.582303087 +0200
> >> +++ u-boot-mainline/drivers/video/cfb_console.c	2009-10-19 13:17:29.406303158 +0200
> > 
> > Please use git-format-patch to create patches.
> 
> Why? Do you have any problems to apply these patches? I personally
> (still) prefer using quilt for patch stack management.

git-format-patch provides index information, which allows for
intelligent merges (i. e. the merge code can then find the patch base
and do a rebase internally). With your patches this is impossible.

Fell free to use quilt or any other tools for your own purposes, but
for patch submission please prepare the patches using
git-format-patch

> >> +#else
> >> +	unsigned int i, *p;
> >> +
> >> +	i = dev->winSizeX * dev->winSizeY;
> >> +	p = (unsigned int *)dev->frameAdrs;
> >> +	while (i--)
> >> +		*p++ = 0;
> >> +#endif
> > 
> > Why don't you use memset() here?
> 
> Maybe to ensure that D32 accesses are performed. Anatolij might know?

How should Anatolij know? It is you who added this code, right?

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
Overdrawn?  But I still have checks left!

[U-Boot] [PATCH 1/3] video: mb862xx: add option CONFIG_VIDEO_MB862xx_ACCEL for X888RGB mode

[Wolfgang Grandegger]

Wolfgang Denk wrote:
> Dear Wolfgang,
> 
> In message <4ADC5661.7050508@grandegger.com> you wrote:
>>>> The new IPEK01 board uses the X888RGB mode for the Lime graphics
>>>> controller. For this mode video accelaration does not work. This patch
>>>> makes the accelaration configurable via CONFIG_VIDEO_MB862xx_ACCEL,
>>>> which is enabled for the lwmon5 and the socrates board for backward
>>>> compatibility.
>>> Why would you want to disable it for IPEK01? Accelaration seems to be
>>> a good thing you don't give up if you don't have to, but I
>>> cannot think of reasons why you would have to do without it?
>> Because acceleration does work with 16 bpp but *not* with 32 bpp. That's
>> the reason why we made it configurable. Well, this patch could be
>> dropped, because the BSP for the IPEK01 posted here uses now 16 bpp as well.
> 
> Then please either mention this fact in the commit message (the
> current one does not say anything about 16 versus 32 bit mode), or
> realy drop the patch.

Well, X888RGB mode is a 32 bpp mode. I leave it up to Anatolij to accept
this patch or not (he is actually the original author).

>>>> --- u-boot-mainline.orig/drivers/video/cfb_console.c	2009-10-19 13:17:14.582303087 +0200
>>>> +++ u-boot-mainline/drivers/video/cfb_console.c	2009-10-19 13:17:29.406303158 +0200
>>> Please use git-format-patch to create patches.
>> Why? Do you have any problems to apply these patches? I personally
>> (still) prefer using quilt for patch stack management.
> 
> git-format-patch provides index information, which allows for
> intelligent merges (i. e. the merge code can then find the patch base
> and do a rebase internally). With your patches this is impossible.
> 
> Fell free to use quilt or any other tools for your own purposes, but
> for patch submission please prepare the patches using
> git-format-patch

OK.

>>>> +#else
>>>> +	unsigned int i, *p;
>>>> +
>>>> +	i = dev->winSizeX * dev->winSizeY;
>>>> +	p = (unsigned int *)dev->frameAdrs;
>>>> +	while (i--)
>>>> +		*p++ = 0;
>>>> +#endif
>>> Why don't you use memset() here?
>> Maybe to ensure that D32 accesses are performed. Anatolij might know?
> 
> How should Anatolij know? It is you who added this code, right?

No, this patch is from Anatolij and he has added his signed-off-by. My
signed-off-by is not correct, strictly speaking. I should have just
added an acked-by or tested-by line. Will change.

Wolfgang.

[U-Boot] [PATCH 2/3] video: mb862xx: add option VIDEO_FB_16BPP_WORD_SWAP for IPEK01

[Wolfgang Grandegger]

An embedded and charset-unspecified text was scrubbed...
Name: video-16bpp-word-swap.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20091019/29ea48fa/attachment.txt 

[U-Boot] [PATCH 2/3] video: mb862xx: add option VIDEO_FB_16BPP_WORD_SWAP for IPEK01

[Wolfgang Denk]

Dear Wolfgang Grandegger,

In message <20091019111913.621578739@denx.de> you wrote:
> In 16 bpp mode, the new IPEK01 board only requires swapping of D16 words
> for D32 accesses due to the diffferent connecting to the GDC bus. This
> patch introduces the configuration option VIDEO_FB_16BPP_WORD_SWAP,
> which should be set for all board using the mb862xx in 16 bpp mode. For
> the IPEK01, VIDEO_FB_16BPP_PIXEL_SWAP should not be set.

I don't see any functional change in this patch - all you do is
renaming VIDEO_FB_16BPP_PIXEL_SWAP into VIDEO_FB_16BPP_WORD_SWAP.

This makes no sense to me.

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
The universe, they said, depended for its operation on the balance of
four forces which they identified as charm,  persuasion,  uncertainty
and bloody-mindedness.      -- Terry Pratchett, "The Light Fantastic"

[U-Boot] [PATCH 2/3] video: mb862xx: add option VIDEO_FB_16BPP_WORD_SWAP for IPEK01

[Wolfgang Grandegger]

Wolfgang Denk wrote:
> Dear Wolfgang Grandegger,
> 
> In message <20091019111913.621578739@denx.de> you wrote:
>> In 16 bpp mode, the new IPEK01 board only requires swapping of D16 words
>> for D32 accesses due to the diffferent connecting to the GDC bus. This
>> patch introduces the configuration option VIDEO_FB_16BPP_WORD_SWAP,
>> which should be set for all board using the mb862xx in 16 bpp mode. For
>> the IPEK01, VIDEO_FB_16BPP_PIXEL_SWAP should not be set.
> 
> I don't see any functional change in this patch - all you do is
> renaming VIDEO_FB_16BPP_PIXEL_SWAP into VIDEO_FB_16BPP_WORD_SWAP.
> 
> This makes no sense to me.

Please have a look to the patched file. VIDEO_FB_16BPP_PIXEL_SWAP is
used in other locations as well. This type of swapping is related to the
way the GDC on the Socrates and lwmo5 board is connected.

Wolfgang.

[U-Boot] [PATCH 2/3] video: mb862xx: add option VIDEO_FB_16BPP_WORD_SWAP for IPEK01

[Wolfgang Denk]

Dear Wolfgang Grandegger,

In message <4ADC56E4.40907@grandegger.com> you wrote:
>
> >> In 16 bpp mode, the new IPEK01 board only requires swapping of D16 words
> >> for D32 accesses due to the diffferent connecting to the GDC bus. This
> >> patch introduces the configuration option VIDEO_FB_16BPP_WORD_SWAP,
> >> which should be set for all board using the mb862xx in 16 bpp mode. For
> >> the IPEK01, VIDEO_FB_16BPP_PIXEL_SWAP should not be set.
> > 
> > I don't see any functional change in this patch - all you do is
> > renaming VIDEO_FB_16BPP_PIXEL_SWAP into VIDEO_FB_16BPP_WORD_SWAP.
> > 
> > This makes no sense to me.
> 
> Please have a look to the patched file. VIDEO_FB_16BPP_PIXEL_SWAP is
> used in other locations as well. This type of swapping is related to the
> way the GDC on the Socrates and lwmo5 board is connected.

I see.

But please add a description of VIDEO_FB_16BPP_PIXEL_SWAP and
VIDEO_FB_16BPP_WORD_SWAP to the README.

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
       There is enough for the need of everyone in this world,
       but not for the greed of everyone.     - Mahatma Gandhi

[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board

[Wolfgang Grandegger]

An embedded and charset-unspecified text was scrubbed...
Name: ipek01-board-support.patch
Url: http://lists.denx.de/pipermail/u-boot/attachments/20091019/14e4b255/attachment.txt 

[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board

[Wolfgang Denk]

Dear Wolfgang Grandegger,

In message <20091019111913.802418590@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@grandegger.com>
> ---
>  Makefile                        |    3 
>  board/ipek01/Makefile           |   50 ++++
>  board/ipek01/config.mk          |   30 ++
>  board/ipek01/ipek01.c           |  374 ++++++++++++++++++++++++++++++++++++
>  board/ipek01/mt46v16m16-75.h    |   37 +++
>  board/ipek01/mt48lc16m16a2-75.h |   43 ++++
>  board/ipek01/u-boot.lds         |  125 ++++++++++++
>  include/configs/ipek01.h        |  411 ++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 1073 insertions(+)
>  create mode 100644 board/ipek01/Makefile
>  create mode 100644 board/ipek01/config.mk
>  create mode 100644 board/ipek01/ipek01.c
>  create mode 100644 board/ipek01/mt46v16m16-75.h
>  create mode 100644 board/ipek01/mt48lc16m16a2-75.h
>  create mode 100644 board/ipek01/u-boot.lds
>  create mode 100644 include/configs/ipek01.h

Entries to MAKEALL and MAINTAINERS are missing.

> Index: u-boot-mainline/Makefile
> ===================================================================
> --- u-boot-mainline.orig/Makefile	2009-10-19 13:17:12.185302922 +0200
> +++ u-boot-mainline/Makefile	2009-10-19 13:17:17.519552635 +0200
> @@ -725,6 +725,9 @@
>  		}
>  	@$(MKCONFIG) -a PM520 ppc mpc5xxx pm520
>  
> +ipek01_config: unconfig
> +	@$(MKCONFIG) -a ipek01 ppc mpc5xxx ipek01
> +

Please keep list ssorted.

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

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

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

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

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

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


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

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

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

> +	"rootpath=/opt/eldk41/ppc_6xx\0"				\

Is this intentional? Or did you just forget to s/41// ?

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

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

> +#define CONFIG_SYS_INIT_RAM_END		MPC5XXX_SRAM_SIZE	/* End of used area in DPRAM */

Line too long. There are more too long lines in this patch. Please
check globally.


> +#undef	CONFIG_IDE_8xx_PCCARD		/* Use IDE with PC Card Adapter */
> +
> +#undef	CONFIG_IDE_8xx_DIRECT		/* Direct IDE	 not supported	*/
> +#undef	CONFIG_IDE_LED			/* LED	 for ide not supported	*/

No need to #undef what is not defined anyway.


Please clean up and resubmit.  Thanks.

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
How does a project get to be a year late?      ... One day at a time.

[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board

[Wolfgang Grandegger]

Wolfgang Denk wrote:
> Dear Wolfgang Grandegger,
> 
> In message <20091019111913.802418590@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@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.

[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board

[Wolfgang Denk]

Dear Wolfgang Grandegger,

In message <4ADC6CC3.6040607@grandegger.com> you wrote:
>
> >> +#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?

I don't think it's dead code on all boards; there are RAMBOOT targts
in the Makefile for some bords (for example digsy_mtc). I just didn't
notice that your board is using this anywhere.


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
My play was a complete success.  The audience was a failure.