[U-Boot] [PATCH][v2] powerpc/CoreNet: add tool to support pbl image build.

[Wolfgang Denk]

Dear Shaohui Xie,

In message <1338979010-950-1-git-send-email-Shaohui.Xie at freescale.com> you wrote:
> Provides a tool to build boot Image for PBL(Pre boot loader) which is
> used on Freescale CoreNet SoCs, PBL can be used to load some instructions
> and/or data for pre-initialization. The default output image is u-boot.pbl,
> for more details please refer to doc/README.pblimage.
...
> --- /dev/null
> +++ b/board/freescale/corenet_ds/config.mk
> @@ -0,0 +1,26 @@
...
> +ifeq ($(CONFIG_RAMBOOT_PBL), y)
> +CONFIG_PBL_CONFIG = $(SRCTREE)/$(CONFIG_BOARDDIR)/pblimage.cfg
> +ALL-y += $(obj)u-boot.pbl
> +endif

I want to get rid if config.mk files to the extend possible - and here
I see no need for a new one.

Please dump this.

> diff --git a/board/freescale/corenet_ds/pblimage.cfg b/board/freescale/corenet_ds/pblimage.cfg
> new file mode 100644
> index 0000000..01003ce
> --- /dev/null
> +++ b/board/freescale/corenet_ds/pblimage.cfg
> @@ -0,0 +1,60 @@
...
> +#64 bytes RCW data for P4080, replace it when building image
> +#for P3041DS or P5020DS.
> +4c580000 00000000 18185218 0000cccc
> +40464000 3c3c2000 58000000 61000000
> +00000000 00000000 00000000 008b6000
> +00000000 00000000 00000000 00000000

You don;t expect us to edit this fiule just when building for another
target?  Please find a solution that works without editing source
files.

> diff --git a/common/image.c b/common/image.c
> index 202c8a1..afc7155 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -147,6 +147,7 @@ static const table_entry_t uimage_type[] = {
>  	{	IH_TYPE_SCRIPT,     "script",	  "Script",		},
>  	{	IH_TYPE_STANDALONE, "standalone", "Standalone Program", },
>  	{	IH_TYPE_UBLIMAGE,   "ublimage",   "Davinci UBL image",},
> +	{	IH_TYPE_PBLIMAGE,   "pblimage",   "Freescale PBL Boot Image",},

Please keep/make list sorted.

> --- /dev/null
> +++ b/doc/README.pblimage
> @@ -0,0 +1,138 @@
> +------------------------------------------------------------------
> +Freescale PBL(pre-boot loader) Boot Image generation using mkimage
> +------------------------------------------------------------------
> +
> +This document describes the U-Boot feature as it
> +is implemented for the Freescale CoreNet SoC.

Is it also implemented for other SoCs, differently?  If not, then this
is redundant and should be removed.

> +The CoreNet SoC's can boot directly from eSPI FLASH, SD/MMC and
> +NAND, etc. For more details refer section 5 Pre-boot loader
> +specifications.

section 5 of which document?  Provide title and URL.

> +   For eSPI boot:
> +	To build the eSPI boot image for P4080DS:
> +	make P4080DS_SPIFLASH
> +
> +	To build the eSPI boot image for P3041DS:
> +	make P3041DS_SPIFLASH
> +
> +	To build the eSPI boot image for P5020DS:
> +	make P5020DS_SPIFLASH

Come on, this doesn't scale.  Why not simply:

	make <name>_SPIFLASH

?

> +2. Command below provided a way to re-build the PBL boot image if the
> +configuration file needes to be modified while the u-boot.bin does not
> +need to be re-built.

Fix spelling and grammar.

Also, such a command should never be necessary - the "make" system
should catch any such situations automatically.  If it doesn't, then
please fix this.

> +Board specific configuration file specifications:
> +------------------------------------------------
> +1. This file must present in the $(BOARDDIR) and the name should be
> +	pblimage.cfg (since this is used in Makefile)

WHo knows what $(BOARDDIR) might be? s/should/must/

> diff --git a/tools/Makefile b/tools/Makefile
> index 64bcc4d..26f4927 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -90,6 +90,7 @@ OBJ_FILES-$(CONFIG_CMD_LOADS) += img2srec.o
>  OBJ_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes.o
>  NOPED_OBJ_FILES-y += aisimage.o
>  NOPED_OBJ_FILES-y += kwbimage.o
> +NOPED_OBJ_FILES-y += pblimage.o
>  NOPED_OBJ_FILES-y += imximage.o
>  NOPED_OBJ_FILES-y += omapimage.o
>  NOPED_OBJ_FILES-y += mkenvimage.o
> @@ -203,6 +204,7 @@ $(obj)mkimage$(SFX):	$(obj)aisimage.o \
>  			$(obj)image.o \
>  			$(obj)imximage.o \
>  			$(obj)kwbimage.o \
> +			$(obj)pblimage.o \
>  			$(obj)md5.o \
>  			$(obj)mkimage.o \
>  			$(obj)os_support.o \

Should we make this code conditional?  Most users will never need it.

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
When a woman marries again it is because she detested her first  hus-
band.  When  a  man  marries again, it is because he adored his first
wife.                                                  -- Oscar Wilde