[PATCH] tools/mrvl_uart.sh: Remove script

[Robert Marko]

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 <pali at 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 <marcel at ziswiler.com>
> >
> > And you get my tested-by for such patch fixing this as outlined above.
> >
> > Tested-by: Marcel Ziswiler <marcel at 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.