[U-Boot] [PATCH 2/2 v4] Add support for MINI2440 (s3c2440).

[Wolfgang Denk]

Dear Gabriel Huau,

In message <1335083622-8284-3-git-send-email-contact at huau-gabriel.fr> you wrote:
> ---

Please add some explanations what "MINI2440" is, where to find
documentation, etc.

>  board/friendlyarm/mini2440/Makefile   |   44 +++++++
>  board/friendlyarm/mini2440/mini2440.c |  121 +++++++++++++++++++
>  boards.cfg                            |    1 +
>  include/configs/mini2440.h            |  212 +++++++++++++++++++++++++++++++++
>  4 files changed, 378 insertions(+)
>  create mode 100644 board/friendlyarm/mini2440/Makefile
>  create mode 100644 board/friendlyarm/mini2440/mini2440.c
>  create mode 100644 include/configs/mini2440.h

Entry to MAINTAINERS missing.

...
> +int dram_init(void)
> +{
> +	/*
> +	 * Configuring bus width and timing for bank 0 only
> +	 * Initialize clocks for each bank 0..5
> +	 * Bank 3 and 4 are used for DM9000
> +	 */
> +	__raw_writel(0x221dd120, S3C24X0_MEMCTL_BASE);
> +	__raw_writel(0x700, S3C24X0_MEMCTL_BASE+0x4);  /* Bank0 register */
> +	__raw_writel(0x700, S3C24X0_MEMCTL_BASE+0x8);  /* Bank1 register */
> +	__raw_writel(0x700, S3C24X0_MEMCTL_BASE+0xc);  /* Bank2 register */
> +	__raw_writel(0x1f70, S3C24X0_MEMCTL_BASE+0x10);  /* Bank3 register */
> +	__raw_writel(0x1f70, S3C24X0_MEMCTL_BASE+0x14);  /* Bank4 register */
> +	__raw_writel(0x700, S3C24X0_MEMCTL_BASE+0x18);  /* Bank5 register */

We do not allow base address plus offset notation.  Please use a
proper C struct instead.

> +	gd->ram_size = PHYS_SDRAM_1_SIZE;

Please use get_ram_size() to auto-detect / verify the actual RAM size.

> +/*
> + * High Level Configuration Options
> + */
> +#define CONFIG_ARM920T		1	/* This is an ARM920T Core	*/
> +#define CONFIG_S3C24X0		1	/* in a SAMSUNG S3C2440 SoC */
> +#define CONFIG_S3C2440		1	/* in a SAMSUNG S3C2440 SoC */
> +#define CONFIG_MINI2440		1	/* on a MIN2440 Board       */

Please do not use values in feature-defines.  Please fix globally.

> +/* size in bytes reserved for initial data */
> +#define CONFIG_GBL_DATA_SIZE	128

NAK.  This gets auto-computed.

> +/* everything, incl board info, in Hz */
> +#undef  CONFIG_CLKS_IN_HZ

Please do not undefine what is not defined anyway.  Please fix
globally.

> +/*
> + * When booting from NAND, it is impossible to access the lowest addresses
> + * due to the SteppingStone being in the way. Luckily the NOR doesn't really
> + * care about the highest 16 bits of address, so we set the controlers
> + * registers to go and poke over there, instead.
> + */
> +#define PHYS_FLASH_1			0x0
> +#define CONFIG_SYS_FLASH_BASE	0x0

Urghh... this sounds very much like a serious design issue?



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
backups: always in season, never out of style.