Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

["Pali Rohár" <pali@kernel.org>]

On Wednesday 27 October 2021 17:08:35 Marek Behún wrote:
> On Wed, 27 Oct 2021 16:10:21 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> > On Wednesday 27 October 2021 15:52:47 Pali Rohár wrote:
> > > On Wednesday 27 October 2021 07:09:42 Stefan Roese wrote:  
> > > > On 26.10.21 20:48, Pali Rohár wrote:  
> > > > > On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:  
> > > > > > On 26.10.21 14:40, Pali Rohár wrote:  
> > > > > > > My another guess there could be a problem is usage of stack. Maybe it is
> > > > > > > possible that register with stack pointer is not initialized after the
> > > > > > > full transfer when going to execute main image. Same arm baudrate change
> > > > > > > code is used after the SPL and before main U-Boot, and the first
> > > > > > > instruction is "push". Maybe modifying the arm code to not use stack
> > > > > > > when switching the baudrate back to 115200 could also help. I will look
> > > > > > > at it.  
> > > > > > 
> > > > > > Thanks.  
> > > > > 
> > > > > Here is dirty hack patch which completely disable calling code for
> > > > > changing baudrate back to 115200 on ARM side:
> > > > > 
> > > > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > > > index 5f4ff673972e..00d58a239c71 100644
> > > > > --- a/tools/kwboot.c
> > > > > +++ b/tools/kwboot.c
> > > > > @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
> > > > >   		return rc;
> > > > >   	if (baudrate) {
> > > > > -		char buf[sizeof(kwb_baud_magic)];
> > > > > -
> > > > > -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > > -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > > -		if (rc)
> > > > > -			return rc;
> > > > > -
> > > > > -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > > -			errno = EPROTO;
> > > > > -			return -1;
> > > > > -		}
> > > > > +//		char buf[sizeof(kwb_baud_magic)];
> > > > > +//
> > > > > +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > > +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > > +//		if (rc)
> > > > > +//			return rc;
> > > > > +//
> > > > > +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > > +//			errno = EPROTO;
> > > > > +//			return -1;
> > > > > +//		}
> > > > >   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
> > > > >   		rc = kwboot_tty_change_baudrate(tty, 115200);
> > > > > @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> > > > >   		 * This code is appended after the data part of the image and
> > > > >   		 * execaddr is changed to execute this code before U-Boot proper.
> > > > >   		 */
> > > > > -		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > > -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > > > > +//		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > > +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);  
> > > > 
> > > > I do have this here in my version as well:
> > > > 
> > > > 		/* Update the 32-bit data checksum */
> > > > 		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> > > > 
> > > > 		/* recompute header size */
> > > > 		hdrsz = kwbheader_size(hdr);
> > > > 
> > > > So I'm using the newer version, just to be sure.  
> > > 
> > > Ok, I probably generated diff against older version, but you have
> > > figured out how to apply it.
> > >   
> > > > >   		/* recompute header size */
> > > > >   		hdrsz = kwbheader_size(hdr);
> > > > > 
> > > > > As main U-Boot binary on ARM resets UART, it means that baudrate is
> > > > > properly set to 115200. Probably beginning of the U-Boot output could be
> > > > > lost but at least console should start.
> > > > > 
> > > > > Could you try this patch if it starts working now?  
> > > > 
> > > > Okay, applied this patch and and booting with different baudrates works
> > > > on this board again (tested with 230400):
> > > > 
> > > >  96 %
> > > > [......................................................................]
> > > >  98 %
> > > > [......................................................................]
> > > >  99 % [................       ]
> > > > Done
> > > > Finishing transfer
> > > > 
> > > > Changing baudrate back to 115200 Bd
> > > > 
> > > > [Type Ctrl-\ + c to quit]
> > > > 
> > > > 
> > > > U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)
> > > > 
> > > > SoC:   MV78260-B0 at 1333 MHz
> > > > I2C:   ready
> > > > DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> > > > Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> > > > Bytes, erase size 256 KiB, total 16 MiB
> > > > OK
> > > > Model: Marvell Armada XP theadorable
> > > > ...
> > > > 
> > > > 
> > > > Thanks,
> > > > Stefan  
> > > 
> > > Perfect! So it really looks like that issue is in the code which resets
> > > baudrate back to the value 115200.
> > > 
> > > I have there another diff which removes usage of the stack in code which
> > > resets baudrate back to default value:
> > > 
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index b56c9a0c8104..8f0e50501398 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
> > >  	memcpy(code, kwboot_baud_code, codesz - 8);
> > >  	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
> > >  	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
> > > +
> > > +	if (!pre) {  
> > 
> > Ou, there is a mistake, it should be "if (pre) {"
> > 
> > > +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
> > > +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
> > > +	}
> 
> This fixes it for me as well, for that one Omnia which didn't work
> which I was debugging a few days ago.
> 
> I guess this can't be done in the binhdr? So we need 2 different
> versions? I don't quite like this ad-hoc change, but I also don't
> like two copies with just a small change between them...
> 
> Marek

I will prepare cleanup / proper patch with one (configurable) version.
Just I need to know if this change fixes issue also for AXP.