kwboot: Marvell Dove UART booting

[Pali Rohár]

On Sunday 13 February 2022 22:15:09 Marek Behún wrote:
> On Sun, 13 Feb 2022 20:41:08 +0100
> Pali Rohár <pali at kernel.org> wrote:
> 
> > On Sunday 13 February 2022 17:16:42 Pali Rohár wrote:
> > > On Wednesday 22 December 2021 20:08:56 Tony Dinh wrote:  
> > > > *** Run kwboot
> > > > 
> > > > # kwboot -t -p -B 115200 /dev/ttyUSB0 -D /localdisk/mtd0.t5335z
> > > > Patching image boot signature to UART
> > > > Aligning image header to Xmodem block size
> > > > Waiting 2s and flushing tty
> > > > Sending boot image header (512 bytes)...
> > > >  25 % [....                                                                  ]
> > > > Done
> > > > Sending boot image data (607664 bytes)...
> > > >   0 % [......................................................................]
> > > >   1 % [......................................................................]
> > > >   2 % [......................................................................]
> > > > <snip>
> > > >  95 % [......................................................................]
> > > >  97 % [......................................................................]
> > > >  98 % [..........................................................            ]
> > > > Done
> > > > Finishing transfer
> > > > [Type Ctrl-\ + c to quit]
> > > > 
> > > > *** Hung here! BootROM did not execute the image payload.
> > > > ***
> > > > *** The file mtd0.t5335z is a dd dump from the SPI flash mtd0 with
> > > > *** this command:
> > > > ***     # dd if=/dev/mtd0 of=mtd0.t5335z bs=768k conv=sync
> > > > 
> > > > <End log>
> > > > 
> > > > 
> > > > - Pali's observation:
> > > > 
> > > > It looks like Dove uses kwbimage v0 format with extensions, at
> > > > least according to Function Spec. See 'Binary Code Extension' and
> > > > 'Header Extension'. Currently kwboot and kwbimage supports v0 image only
> > > > with one extension.  
> > > 
> > > I quickly looked at it. Could you try following patch?
> > > 
> > > diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> > > index 74e5d87a4fef..15e83ececc76 100644
> > > --- a/tools/kwbimage.h
> > > +++ b/tools/kwbimage.h
> > > @@ -61,14 +64,46 @@ struct ext_hdr_v0_reg {
> > >  	uint32_t rdata;
> > >  } __packed;
> > >  
> > > -#define EXT_HDR_V0_REG_COUNT ((0x1dc - 0x20) / sizeof(struct ext_hdr_v0_reg))
> > > -
> > > +/* Structure of the extension header, version 0 (Kirkwood, Dove) */
> > >  struct ext_hdr_v0 {
> > > -	uint32_t              offset;
> > > -	uint8_t               reserved[0x20 - sizeof(uint32_t)];
> > > -	struct ext_hdr_v0_reg rcfg[EXT_HDR_V0_REG_COUNT];
> > > -	uint8_t               reserved2[7];
> > > -	uint8_t               checksum;
> > > +	/*
> > > +	 * Beware that extension header offsets specified in 88AP510 Functional
> > > +	 * Specifications are relative to the start of the main header, not to
> > > +	 * the start of the extension header itself.
> > > +	 */
> > > +	uint32_t offset;		/* 0x0-0x3     */
> > > +	uint8_t  rsvd1[8];		/* 0x4-0xB     */
> > > +	uint32_t ddrinitdelay;		/* 0xC-0xF     */
> > > +	uint32_t match_addr;		/* 0x10-0x13   */
> > > +	uint32_t match_mask;		/* 0x14-0x17   */
> > > +	uint32_t match_value;		/* 0x18-0x1B   */
> > > +	uint8_t  ddrwritetype;		/* 0x1C        */
> > > +	uint8_t  ddrresetmpp;		/* 0x1D        */
> > > +	uint8_t  ddrclkenmpp;		/* 0x1E        */
> > > +	uint8_t  ddrmppdelay;		/* 0x1F        */
> > > +	struct ext_hdr_v0_reg rcfg[55]; /* 0x20-0x1D7  */
> > > +	uint8_t  rsvd2[7];		/* 0x1D8-0x1DE */
> > > +	uint8_t  checksum;		/* 0x1DF       */
> > > +} __packed;
> > > +
> > > +/* Structure of the binary code header, version 0 (Dove) */
> > > +struct binext_hdr_v0 {
> > > +	uint32_t match_addr;		/* 0x00-0x03  */
> > > +	uint32_t match_mask;		/* 0x04-0x07  */
> > > +	uint32_t match_value;		/* 0x08-0x0B  */
> > > +	uint32_t offset;		/* 0x0C-0x0F  */
> > > +	uint32_t destaddr;		/* 0x10-0x13  */
> > > +	uint32_t size;			/* 0x14-0x17  */
> > > +	uint32_t execaddr;		/* 0x18-0x1B  */
> > > +	uint32_t param1;		/* 0x1C-0x1F  */
> > > +	uint32_t param2;		/* 0x20-0x23  */
> > > +	uint32_t param3;		/* 0x24-0x27  */
> > > +	uint32_t param4;		/* 0x28-0x2B  */
> > > +	uint8_t  params;		/* 0x2C       */
> > > +	uint8_t  rsvd1;			/* 0x2D       */
> > > +	uint8_t  rsvd2;			/* 0x2E       */
> > > +	uint8_t  checksum;		/* 0x2F       */
> > > +	uint8_t  code[2000];		/* 0x30-0x7FF */
> > >  } __packed;
> > >  
> > >  /* Structure of the main header, version 1 (Armada 370/XP/375/38x/39x) */
> > > @@ -213,8 +248,20 @@ static inline size_t kwbheader_size(const void *header)
> > >  	if (kwbimage_version(header) == 0) {
> > >  		const struct main_hdr_v0 *hdr = header;
> > >  
> > > +		/*
> > > +		 * First extension header starts immediately after the main
> > > +		 * header without any padding. Between extension headers is
> > > +		 * 0x20 byte padding. There is no padding after the last
> > > +		 * extension header. First binary code header starts immediately
> > > +		 * after the last extension header (or immediately after the
> > > +		 * main header if there is no extension header) without any
> > > +		 * padding. There is no padding between binary code headers and
> > > +		 * neither after the last binary code header.
> > > +		 */
> > >  		return sizeof(*hdr) +
> > > -		       hdr->ext ? sizeof(struct ext_hdr_v0) : 0;
> > > +		       hdr->ext * sizeof(struct ext_hdr_v0) +
> > > +		       ((hdr->ext > 1) ? (hdr->ext * 0x20) : 0) +  
> > 
> >                                          ^^^^^^^^^^^^^^^^^
> > Ou.. there is a mistake. It should be: "((hdr->ext - 1) * 0x20)"
> > as number of paddings in between is number of headers minus one.
> 
> In that case the whole ternary operator can be dropped, i.e. instead of
>   ((hdr->ext > 1) ? (hdr->ext * 0x20) : 0)
> you can have
>   ((hdr->ext - 1) * 0x20)
> if I interpret this correctly.

No, it cannot be dropped, with correction it is:

   ((hdr->ext > 1) ? ((hdr->ext - 1) * 0x20) : 0)

When hdr->ext is zero, result must be also 0, not (uint8_t)-1 * 0x20.