From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 511ECC433EF for ; Wed, 27 Oct 2021 15:13:44 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 806B5604E9 for ; Wed, 27 Oct 2021 15:13:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 806B5604E9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AA8CE82D6F; Wed, 27 Oct 2021 17:13:41 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="lkQRqjjI"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id ED4D38347C; Wed, 27 Oct 2021 17:13:39 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4540D82D6F for ; Wed, 27 Oct 2021 17:13:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: by mail.kernel.org (Postfix) with ESMTPSA id B5664604E9; Wed, 27 Oct 2021 15:13:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1635347614; bh=Ol6KBr3+yaoNYE4nSBewMaJ02RkEsukzobBQmbeAlwI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lkQRqjjIxwvtTp+5JvMlTv/6DGJr6g42M+vzkmM44cGZJFMkzx704J47wtH9VxkDs r7ocqTTHomdzEZ6pOG/DkcJzQkYLwDC2sTtNQzzmlsb52j5a4tSAgk3VzDTULeArBh u34sa1OeXtJy+rELPJByUgfwm62XlhpPiIrhKWVBs5LAAbHYtoagdeSHzO0LI+sVTM OpvsssrUoKnv1dXq7amOxBDLwTx3luA/rCaMnW9Ahj5HzpixHDPf27G6quAPEogmWr jG53mT46xN+nFPhN7SEEy7AMcdIfJn+I6T0bNiXLgu9OEoQg/XEV07ZHTjBp/fSYFN +q6qeUvdw/sGA== Received: by pali.im (Postfix) id 5F53C894; Wed, 27 Oct 2021 17:13:31 +0200 (CEST) Date: Wed, 27 Oct 2021 17:13:31 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Marek =?utf-8?B?QmVow7pu?= Cc: Stefan Roese , u-boot@lists.denx.de, Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements Message-ID: <20211027151331.yss64d25vexffli5@pali> References: <1d9e63ba-bb86-8a86-d7f0-02fdf6e1da52@denx.de> <20211026090620.eilcjqkc33wavm5b@pali> <872e26ac-77ff-f19b-b3c9-debff1af58b0@denx.de> <20211026124048.lprqsoqow63cwqom@pali> <20211026184824.3g2bup2cwpnxk6a3@pali> <2eae7502-2fb9-cefd-57e8-f14786a626bc@denx.de> <20211027135247.kusaph47hrvieoiq@pali> <20211027141021.kgtby7g3dukmf225@pali> <20211027170835.7c584981@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20211027170835.7c584981@thinkpad> User-Agent: NeoMutt/20180716 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Wednesday 27 October 2021 17:08:35 Marek Behún wrote: > On Wed, 27 Oct 2021 16:10:21 +0200 > Pali Rohár 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.