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

[Daniel Stodden]

On Wed, 2012-04-25 at 02:08 +0200, Luka Perkov wrote:
> 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 ... ?

Good point. I used to have a couple more options which I later
discarded, right now they'd all fit on a single line anyway.

Fixed.

> > +.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)

Huh?

I don't know u-boot's build system very well, but normally CONFIG items
are employed to keep the target lean. Where the best you could ask from
down-config'd host toolset build would be to not accidentally miss sth?
This is all host code.

Is building that conditionally really best current practice? If so,
yeah, we sure can change that.

But would appreciate a clarification.

> >  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?

Yup. :)

> ...
> 
> > +
> > +/*
> > + * 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?

Yup, removed.

Cheers,
Daniel