[U-Boot] [PATCH] kwboot: Boot Marvell Kirkwood SoCs over a serial link.

[Luka Perkov]

Hi Daniel,

I want to say that this works on my kirkwood board ib62x0. Nice work.

I have only a few suggestions...

On Mon, Apr 23, 2012 at 11:07:21PM -0700, Daniel Stodden wrote:
> +.SH NAME
> +kwboot \- Boot Marvell Kirkwood SoCs over a serial link.
> +.SH SYNOPSIS
> +.B kwboot
> +.RB [ "-b \fIimage\fP" ]
> +.RB [ "-t" ]
> +.RB [...]

Why not put here -p and all other stuff instad ... ?

> +.BI "\-p"
> +In combination with \fB-b\fP, patches the header in \fIimage\fP prior
> +to upload, to "UART boot" type.
> +
> +This option attempts on-the-fly conversion of some none-UART image
> +types, such as images which were originally formatted to be stored in
> +flash memory.
> +
> +Conversion is performed in memory. The contents of \fIimage\fP will
> +not be altered.

I really like this -p feature :)

> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -68,6 +68,7 @@ BIN_FILES-$(CONFIG_CMD_LOADS) += img2srec$(SFX)
>  BIN_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes$(SFX)
>  BIN_FILES-y += mkenvimage$(SFX)
>  BIN_FILES-y += mkimage$(SFX)
> +BIN_FILES-y += kwboot$(SFX)

Why not use something like:

BIN_FILES-$(CONFIG_KIRKWOOD) += kwboot$(SFX)

>  NOPED_OBJ_FILES-y += os_support.o
>  OBJ_FILES-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1.o
>  NOPED_OBJ_FILES-y += ublimage.o
> +OBJ_FILES-y += kwboot.o

The same question here.

> +$(obj)kwboot$(SFX): $(obj)kwboot.o
> +	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
> +	$(HOSTSTRIP) $@
> +

And here.

> index 0000000..fbec8b6
> --- /dev/null
> +++ b/tools/kwboot.c
> @@ -0,0 +1,750 @@
> +/*
> + * Boot a Marvell Kirkwood SoC, with Xmodem over UART0.
> + *
> + * (c) 2012 Daniel Stodden <daniel.stodden at gmail.com>
> + *
> + * References: marvell.com, "88F6180, 88F6190, 88F6192, and 88F6281
> + *   Integrated Controller: Functional Specifications" December 2,
> + *   2008. Chapter 24.2 "BootROM Firmware".

     ^^
Are the spaces here on purpose?

...

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "Linux"
> + * c-basic-offset: 8
> + * tab-width: 8
> + * indent-tabs-mode: t
> + * End:
> + */

Why not remove that completely?

Regards,
Luka