[U-Boot] [PATCH v2 1/2] Add README for the "Falcon" mode

[Andreas Bießmann]

Dear Stefano Babic,

On 13.11.2012 12:11, Stefano Babic wrote:
> Simple howto to add support to a board
> for booting the kernel from SPL ("Falcon" mode).
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> ---
> Changes in v2:
> - spelling, language fixes (Andreas Biessman)
> - rewrite some unclear sentences
> - drop CONFIG_SPL_OS_BOOT_KEY
> - make example with twister more exhaustive
> 
>  doc/README.falcon |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 doc/README.falcon
> 

<snip>

> +Configuration
> +----------------------------
> +CONFIG_CMD_SPL		Enable the "spl export" command.
> +			The command "spl export" is then available in U-Boot
> +			mode
> +CONFIG_SPL_OS_BOOT	Activate Falcon mode.
> +			A board should implement the following functions:

I think reordering this list to have the required functions directly
after the colon would be helpful. Alternatively add a pointer to the
list of functions after the list of defines.

> +
> +CONFIG_SYS_SPL_ARGS_ADDR	Address in RAM where the parameters must be
> +				copied by SPL.
> +				In most cases, it is <start_of_ram> + 0x100
> +
> +CONFIG_SYS_NAND_SPL_KERNEL_OFFS	Offset in NAND where the kernel is stored
> +
> +CONFIG_CMD_SPL_NAND_OFS	Offset in NAND where the parameters area was saved.
> +
> +CONFIG_CMD_SPL_WRITE_SIZE 	Size of the parameters area to be copied
> +
> +Function that a board must implement
> +------------------------------------
> +
> +void spl_board_prepare_for_linux(void) : optional
> +	Called from SPL before starting the kernel
> +
> +spl_start_uboot() : required
> +		Returns "0" if SPL starts the kernel, "1" if U-Boot
> +		must be started.
> +
> +
> +Using spl command
> +-----------------
> +
> +spl - SPL configuration
> +
> +Usage:
> +
> +spl export <img=atags|fdt> [kernel_addr] [fdt_addr if <img> = fdt] - export a kernel parameter image
> +
> +img		: "atags" or "fdt"
> +kernel_addr	: kernel is loaded as part of the boot process, but it is not started.
> +		  This is the address where a kernel image is stored.
> +fdt_addr	: in case of fdt, the address of the device tree.
> +
> +
> +The spl puts its result at a self gained position. The position is defined at compile
> +time or when generating the uImage but not at command line for 'spl export'
> +(see spl_export(): gd->bd->bi_boot_params vs. images.ft_addr).

I think you got me wrong by my last remarks about the usage() of 'spl
export'.


First of all there is presumably a typo in the descriptive text.

The current code has this usage():
---8<---
	"export <img=atags|fdt> [kernel_addr] [initrd_addr] "
	"[fdt_addr if <img> = fdt] - export a kernel parameter image\n"
	"\t initrd_img can be set to \"-\" if fdt_addr without initrd img is"
	"used")
--->8---
The arguments say something about 'initrd_addr' but the descriptive text
has 'initrd_img'. I think (but do not know since I have not used the spl
command so often) that the 'initrd_img' should be written 'initrd_addr'
(cc Simon Schwarz for that).

The second thing in my last review of your patch was maybe shortly
described. You did write there the correct usage() of 'spl export' but
later on add a more descriptive text about the parameters of 'spl
export'. In these descriptive text you did write 'init_addr' instead of
'initrd_addr' - that was my comment about.
Additionally I thought your descriptive text is way better than the
current descriptive text of 'spl export', so I asked to add your
description to the command.

Now the main reason for my writing here ;)
Your usage() of 'spl export' some lines above is wrong cause it is
missing the 'initrd_addr' parameter.

<snip example>

Best regards

Andreas Bießmann