From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 19 Apr 2013 13:58:24 +0200
Subject: [U-Boot] [PATCH 6/6] arm: mx5: Add support for DENX M53EVK
In-Reply-To: <20130419055211.8EB56200566@gemini.denx.de>
References: <1366344655-8535-1-git-send-email-marex@denx.de>
	<1366344655-8535-6-git-send-email-marex@denx.de>
	<20130419055211.8EB56200566@gemini.denx.de>
Message-ID: <201304191358.25181.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Wolfgang Denk,

> Dear Marek Vasut,
> 
> In message <1366344655-8535-6-git-send-email-marex@denx.de> you wrote:
> ...
> 
> > +#define CONFIG_CMD_DHCP
> > +#define CONFIG_CMD_EXT2
> > +#define CONFIG_CMD_FAT
> > +#define CONFIG_CMD_FAT
> 
> One CONFIG_CMD_FAT should be enough.

Indeed, it might be too fat now.

[...]
> 
> > +#define CONFIG_CMDLINE_TAG
> > +#define CONFIG_INITRD_TAG
> > +#define CONFIG_SETUP_MEMORY_TAGS
> 
> I think we support only DT enabled kernels, so do we really need
> these?

I do need those to boot ancient FSL kernel (for that Android 4.1.2 for MX53 with 
working graphics acceleration I'm cooking here).

> > +#define	CONFIG_BOOTFILE		"uImage"
> 
> Please make this "m53evk/uImage" as usual.
> 
> Don't we need a DT file as well?

OK

> > +#define	CONFIG_BOOTARGS		"console=ttymxc1,115200"
> > +#define	CONFIG_BOOTCOMMAND	"run bootcmd_net"
> 
> Where is "bootcmd_net" defined?
> 
> I doubt that this is actually a working environment.

Right.

> > +#define CONFIG_LOADADDR		0x70800000
> > +#define CONFIG_SYS_LOAD_ADDR	CONFIG_LOADADDR
> 
> What do we need CONFIG_LOADADDR for?

For this stuff, the "loadaddr" env variable. I have kinda idea to kill this and 
unify it to CONFIG_SYS_LOAD_ADDR all around, what do you think?

include/env_default.h:#ifdef    CONFIG_LOADADDR
include/env_default.h:  "loadaddr="     __stringify(CONFIG_LOADADDR)    "\0"

> > +/*
> > + * The 0x1000 offset must be present, otherwise board won't boot.
> > + * The value (IRAM_BASE_ADDR) must be spelled out, otherwise linker
> > won't link. + */
> > +#define CONFIG_SPL_TEXT_BASE		0x70008000
> > +#define CONFIG_SPL_PAD_TO		0x8000
> > +#define CONFIG_SPL_STACK		0x70004000
> > +#define CONFIG_SPL_LIBCOMMON_SUPPORT
> > +#define CONFIG_SPL_LIBGENERIC_SUPPORT
> > +#define CONFIG_SPL_SERIAL_SUPPORT
> > +#define CONFIG_SPL_GPIO_SUPPORT
> 
> The comment above does not relate to the definitions here.  Is it
> misplaced, or not correct?

Damn, remnant of my experimentation.