From mboxrd@z Thu Jan  1 00:00:00 1970
From: Steve Rae <srae@broadcom.com>
Date: Mon, 21 Jul 2014 17:02:37 -0700
Subject: [U-Boot] [PATCH 4/4] arm: add Cygnus and NSP boards
In-Reply-To: <20140720075434.48C31380316@gemini.denx.de>
References: <1405733854-20194-1-git-send-email-srae@broadcom.com>
	<1405733854-20194-5-git-send-email-srae@broadcom.com>
	<20140720075434.48C31380316@gemini.denx.de>
Message-ID: <53CDAA1D.2070709@broadcom.com>
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



On 14-07-20 12:54 AM, Wolfgang Denk wrote:
> Dear Steve Rae,
>
> In message <1405733854-20194-5-git-send-email-srae@broadcom.com> you wrote:
>>
>> The bcm_ep board configuration is used by a number of boards
>> including Cygnus and NSP.
>> Add builds for the bcm958300k and the bcm958622hr boards.
> ...
>> +/* uArchitecture specifics */
> ...
>> +#define CONFIG_CONS_INDEX		3
>> +#define CONFIG_SYS_NS16550_COM3		0x18023000
>
> Is the console inex really architecture specific and identical for all
> boards based on this?  I would expect to find this in the board config
> header.
>
Yes -- the bootrom code always uses 3 for this architecture

>> +/* uArchitecture specifics */
> ...
>> +#define CONFIG_CONS_INDEX		1
>> +#define CONFIG_SYS_NS16550_COM1		0x18000300
>
> Ditto here.
Ditto -- the bootrom code always uses 1 for this architecture

>
> ...
>> +	/* Address of boot parameters passed to kernel
>> +	 * Use default offset 0x100
>> +	 */
>
> Incorrect multiline comment style.  Please check (and fix, if needed)
> globally.
will fix in [v2]

>
>> +/*
>> + * misc_init_r - miscellaneous platform dependent initializations
>> + */
>> +int misc_init_r(void)
>> +{
>> +	return 0;
>> +}
>
> It makes no sense to add an empty function here.  Just do not define
> CONFIG_MISC_INIT_R
will fix in [v2]

>
>
>> +#define CONFIG_SYS_MALLOC_LEN		SZ_4M	/* see armv7/start.S. */
>> +#define CONFIG_STACKSIZE		SZ_256K
>
> Please do not use the SZ_ defines.  They are deprecated.
will fix in [v2]

>
>> +/* Init functions */
>> +#define CONFIG_MISC_INIT_R	/* board's misc_init_r function */
>
> Unused, so remove.
will fix in [v2]
>
>> +#define CONFIG_ENV_SIZE			0x10000
>
> Do you really, really need 64 kB of environmnt?  I doubt that.
will fix in [v2]

>
>
> Best regards,
>
> Wolfgang Denk
>