Hi Pali, Sorry for the late reply.
As Marcel pointed out, we were relying on this script as kwboot just wasn't working. But if it can replace mrvl_uart.sh then I don't have an issue with dropping it after it gets fixed. Regards, Robert On Mon, 7 Feb 2022 at 10:02, Pali Rohár <p...@kernel.org> wrote: > > On Monday 07 February 2022 08:20:54 Marcel Ziswiler wrote: > > On Sat, 2022-02-05 at 15:54 +0100, Pali Rohár wrote: > > > On Saturday 05 February 2022 03:07:00 Marcel Ziswiler wrote: > > > > On Sat, 2022-02-05 at 01:54 +0100, Pali Rohár wrote: > > > > > On Saturday 05 February 2022 01:40:23 Marcel Ziswiler wrote: > > > > > > On Sat, 2022-02-05 at 01:25 +0100, Pali Rohár wrote: > > > > > > > $ kwboot -b /dev/ttyUSB0 > > > > > > > > > > > > Hm, at least kwboot from today's master does not allow -b without > > > > > > also > > > > > > giving it an image. > > > > > > > > > > This commit is part of master branch and added support for it: > > > > > https://source.denx.de/u-boot/u-boot/-/commit/c513fe47dca24de87a904ce7d71cfd8a390e1154 > > > > > > > > > > If it does not work then there is some bug which should be fixed. I > > > > > have > > > > > tested it and it works on my setup. > > > > > > > > > > Can you check if you have that commit to ensure that we are not going > > > > > to > > > > > test / debug older version? > > > > > > > > Yes, sure. I debugged it a little and I believe I found the issue. I > > > > guess the intention was for it to be > > > > run > > > > giving it a dash '-' as image argument for skipping. (Even though that > > > > usually would mean using stdin). > > > > Anyway, > > > > it fails as it then uses such dash as ttypath because optind did not > > > > get incremented. The following fixes > > > > it > > > > for me: > > > > > > > > diff --git a/tools/kwboot.c b/tools/kwboot.c > > > > index 2684f0e75a..d490c026e9 100644 > > > > --- a/tools/kwboot.c > > > > +++ b/tools/kwboot.c > > > > @@ -1768,7 +1768,8 @@ main(int argc, char **argv) > > > > if (prev_optind == optind) > > > > goto usage; > > > > if (argv[optind] && argv[optind][0] != '-') > > > > - imgpath = argv[optind++]; > > > > + imgpath = argv[optind]; > > > > + optind++; > > > > break; > > > > > > > > case 'D': > > > > > > > > Let me know if that is indeed the intention and I can send a proper fix > > > > for it. Maybe I can also update the > > > > documentation/usage in that respect. Thanks again. > > > > > > Now I see where is the issue. Your fix is incorrect because it breaks > > > other usage (e.g. specifying other options). > > > > > > The issue here is that argv[optind] is used also when it is the last > > > option on the command line -- which is not getopt() at all, as it is > > > parsed after the getopt() processing. > > > > > > So I think that correct fix should be something like this: > > > > > > bootmsg = kwboot_msg_boot; > > > if (prev_optind == optind) > > > goto usage; > > > - if (argv[optind] && argv[optind][0] != '-') > > > + if (optind < argc-1 && argv[optind] && > > > argv[optind][0] != '-') > > > imgpath = argv[optind++]; > > > break; > > > > > > > > > This should fix usage of all "kwboot -b /dev/tty", "kwboot -b -t /dev/tty" > > > and "kwboot -b image.kwb /dev/tty" usage. > > > > > > Could you test it? > > > > Yes, this indeed works fine now. > > > > With that, and if we properly document such "standalone" usage, I would be > > fine with dropping > > tools/mrvl_uart.sh. So you can get my reviewed-by for this one. > > > > Reviewed-by: Marcel Ziswiler <mar...@ziswiler.com> > > > > And you get my tested-by for such patch fixing this as outlined above. > > > > Tested-by: Marcel Ziswiler <mar...@ziswiler.com> > > > > > Seems that I tested only -b -t combination (as I wanted to see that > > > bootrom correctly start sending NAK xmodem bytes). > > > > Yep, now I got you. Thanks! > > Ok, thank you for testing, I will send a proper patch to ML.