[U-Boot] [PATCH] Add AMD 29LV160DB FLASH support to SBC-2410A board.

[Wolfgang Denk]

Dear Daniel,

In message <7909dbcf0812310223i53d16c52s208f2c704a4f6717 at mail.gmail.com> you wrote:
> 
> I'm not sure if this is the proper way to submit a patch - please feel
> free to let me know what needs to be changed, if anything.

It is the proper way, but there are some problems with your patch.

> 
>     Daniel Schuler
> 
> ===================

Such explanations should follow the commit message, i., e. please put
these below the "---" line, not above it.

> The current SBC-2410A board from Embest ships with 2MiB of NOR FLASH,
> for which CONFIG_AMD_LV160 has been defined in include/configs/sbc2410x.h

Instead of extending the proprietary flash driver for this board, you
should drop it completely and  use  the  CFI  flash  driver  instead.
Needed changes are small, and you will get rid of such chip dependen-
cies.

> Also, CONFIG_ENV_ADDR has been made more generic by making it depend on
> CONFIG_ENV_SIZE, so that only the latter needs to be changed to modify
> the size of the environment.

You should configure the environment such  that  it  works  with  all
types  of flash chips, so you can use the same binary image of U-Boot
on all boards.


Your patch includes also other changes, which are not mentioned in the
commit message, and which should (as they are unrelated) be split off
into a separate patch.

> In board/sbc2410x/flash.c, the appropriate #elif and case statements have
> been added to support this chip.
> 
> Signed-off-by: Daniel Schuler <dbschuler at gmail.com>
> ---
>  board/sbc2410x/flash.c     |    6 ++++++

I really recommend to get rid of this file. Use
drivers/mtd/cfi_flash.c instead.

> --- a/include/configs/sbc2410x.h
> +++ b/include/configs/sbc2410x.h
> @@ -139,7 +139,7 @@
> 
>  #define	CONFIG_SYS_LOAD_ADDR		0x33000000	/* default load address	*/
> 
> -/* the PWM TImer 4 uses a counter of 15625 for 10 ms, so we need */
> +/* the PWM Timer 4 uses a counter of 15625 for 10 ms, so we need */

Unrelated change; should be separate patch.

>  /* it to wrap 100 times (total 1562500) to get 1 sec. */
>  #define	CONFIG_SYS_HZ			1562500
> 
> @@ -173,29 +173,35 @@
>   */
>  /* #define CONFIG_AMD_LV400	1	/\* uncomment this if you have a LV400
> flash *\/ */
> 
> -#define CONFIG_AMD_LV800	1	/* uncomment this if you have a LV800 flash */
> +/* #define CONFIG_AMD_LV800	1	/\* uncomment this if you have a LV800
> flash *\/ */
> +
> +#define CONFIG_AMD_LV160	1	/* uncomment this if you have a LV160 flash */

I recommend to make the configuration independent of the actual flash
chips.

>  #define CONFIG_SYS_MAX_FLASH_BANKS	1	/* max number of memory banks */
> 
> +#ifdef CONFIG_AMD_LV160
> +#define PHYS_FLASH_SIZE		0x00200000 /* 2MB */
> +#define CONFIG_SYS_MAX_FLASH_SECT	(35)     /* max number of sectors
> on one chip */
^^^^^^^^^^^^^^^^

and fix your mailer. Your patch is line-wrapped, so it is useless
anyway.

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
"...all the  good  computer  designs  are  bootlegged;  the  formally
planned  products,  if  they  are built at all, are dogs!" - David E.
Lundstrom, "A Few Good Men From Univac", MIT Press, 1987