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