| Message ID | 20220203165046.19218-1-pali@kernel.org |
|---|---|
| State | Accepted |
| Commit | 83a8e27062d2c24e001426c4c35859e133e2f1d5 |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | tools/mrvl_uart.sh: Remove script | expand |
On Thu, 3 Feb 2022 17:50:46 +0100 Pali Rohár <pali@kernel.org> wrote: > There are two tools for sending images over UART to Marvell SoCs: kwboot > and mrvl_uart.sh. kwboot received lot of new features and improvements in > last few months. There is no need to maintain two tools in U-Boot, so > remove old mrvl_uart.sh tool. > > Signed-off-by: Pali Rohár <pali@kernel.org> Reviewed-by: Marek Behún <marek.behun@nic.cz>
Added Kosta to Cc, as he is the author of this script. On 2/3/22 17:50, Pali Rohár wrote: > There are two tools for sending images over UART to Marvell SoCs: kwboot > and mrvl_uart.sh. kwboot received lot of new features and improvements in > last few months. There is no need to maintain two tools in U-Boot, so > remove old mrvl_uart.sh tool. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > tools/mrvl_uart.sh | 119 --------------------------------------------- > 1 file changed, 119 deletions(-) > delete mode 100755 tools/mrvl_uart.sh Kosta, do you see any problems with removing this script? As you might have seen, Pali and Marek did some great work on kwboot in the mean time. Is there anything left in mrvl_uart.sh that kwboot can't handle? Thanks, Stefan > diff --git a/tools/mrvl_uart.sh b/tools/mrvl_uart.sh > deleted file mode 100755 > index a46411fc99fb..000000000000 > --- a/tools/mrvl_uart.sh > +++ /dev/null > @@ -1,119 +0,0 @@ > -#!/bin/bash > -# SPDX-License-Identifier: GPL-2.0 > -# > -###################################################### > -# Copyright (C) 2016 Marvell International Ltd. > -# > -# https://spdx.org/licenses > -# > -# Author: Konstantin Porotchkin kostap@marvell.com > -# > -# Version 0.3 > -# > -# UART recovery downloader for Armada SoCs > -# > -###################################################### > - > -port=$1 > -file=$2 > -speed=$3 > - > -pattern_repeat=1500 > -default_baudrate=115200 > -tmpfile=/tmp/xmodem.pattern > -tools=( dd stty sx minicom ) > - > -case "$3" in > - 2) > - fast_baudrate=230400 > - prefix="\xF2" > - ;; > - 4) > - fast_baudrate=460800 > - prefix="\xF4" > - ;; > - 8) > - fast_baudrate=921600 > - prefix="\xF8" > - ;; > - *) > - fast_baudrate=$default_baudrate > - prefix="\xBB" > -esac > - > -if [[ -z "$port" || -z "$file" ]] > -then > - echo -e "\nMarvell recovery image downloader for Armada SoC family." > - echo -e "Command syntax:" > - echo -e "\t$(basename $0) <port> <file> [2|4|8]" > - echo -e "\tport - serial port the target board is connected to" > - echo -e "\tfile - recovery boot image for target download" > - echo -e "\t2|4|8 - times to increase the default serial port speed by" > - echo -e "For example - load the image over ttyUSB0 @ 460800 baud:" > - echo -e "$(basename $0) /dev/ttyUSB0 /tmp/flash-image.bin 4\n" > - echo -e "=====WARNING=====" > - echo -e "- The speed-up option is not available in SoC families prior to A8K+" > - echo -e "- This utility is not compatible with Armada 37xx SoC family\n" > -fi > - > -# Sanity checks > -if [ -c "$port" ] > -then > - echo -e "Using device connected on serial port \"$port\"" > -else > - echo "Wrong serial port name!" > - exit 1 > -fi > - > -if [ -f "$file" ] > -then > - echo -e "Loading flash image file \"$file\"" > -else > - echo "File $file does not exist!" > - exit 1 > -fi > - > -# Verify required tools installation > -for tool in ${tools[@]} > -do > - toolname=`which $tool` > - if [ -z "$toolname" ] > - then > - echo -e "Missing installation of \"$tool\" --> Exiting" > - exit 1 > - fi > -done > - > - > -echo -e "Recovery will run at $fast_baudrate baud" > -echo -e "========================================" > - > -if [ -f "$tmpfile" ] > -then > - rm -f $tmpfile > -fi > - > -# Send the escape sequence to target board using default debug port speed > -stty -F $port raw ignbrk time 5 $default_baudrate > -counter=0 > -while [ $counter -lt $pattern_repeat ]; do > - echo -n -e "$prefix\x11\x22\x33\x44\x55\x66\x77" >> $tmpfile > - let counter=counter+1 > -done > - > -echo -en "Press the \"Reset\" button on the target board and " > -echo -en "the \"Enter\" key on the host keyboard simultaneously" > -read > -dd if=$tmpfile of=$port &>/dev/null > - > -# Speed up the binary image transfer > -stty -F $port raw ignbrk time 5 $fast_baudrate > -sx -vv $file > $port < $port > -#sx-at91 $port $file > - > -# Return the port to the default speed > -stty -F $port raw ignbrk time 5 $default_baudrate > - > -# Optional - fire up Minicom > -minicom -D $port -b $default_baudrate > - Viele Grüße, Stefan Roese
Hi Stefan et. al Putting Robi and Sergey on CC. On Fri, 2022-02-04 at 06:46 +0100, Stefan Roese wrote: > Added Kosta to Cc, as he is the author of this script. > > On 2/3/22 17:50, Pali Rohár wrote: > > There are two tools for sending images over UART to Marvell SoCs: kwboot > > and mrvl_uart.sh. kwboot received lot of new features and improvements in > > last few months. There is no need to maintain two tools in U-Boot, so > > remove old mrvl_uart.sh tool. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > tools/mrvl_uart.sh | 119 --------------------------------------------- > > 1 file changed, 119 deletions(-) > > delete mode 100755 tools/mrvl_uart.sh > > Kosta, do you see any problems with removing this script? As you might > have seen, Pali and Marek did some great work on kwboot in the mean > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to the MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- firmware/build/a70x0_rb5009/release/flash-image.bin Using device connected on serial port "/dev/ttyUSB3" Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" Recovery will run at 115200 baud ======================================== Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 blocks: Give your local XMODEM receive command now. Bytes Sent:1456384 BPS:7871 Transfer complete Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing something or support for booting 64-bit platforms would yet need to be added. Suggestions welcome. Thanks! > Thanks, > Stefan [1] https://forum.openwrt.org/t/add-support-for-mikrotik-rb5009ug Cheers Marcel > > diff --git a/tools/mrvl_uart.sh b/tools/mrvl_uart.sh > > deleted file mode 100755 > > index a46411fc99fb..000000000000 > > --- a/tools/mrvl_uart.sh > > +++ /dev/null > > @@ -1,119 +0,0 @@ > > -#!/bin/bash > > -# SPDX-License-Identifier: GPL-2.0 > > -# > > -###################################################### > > -# Copyright (C) 2016 Marvell International Ltd. > > -# > > -# https://spdx.org/licenses > > -# > > -# Author: Konstantin Porotchkin kostap@marvell.com > > -# > > -# Version 0.3 > > -# > > -# UART recovery downloader for Armada SoCs > > -# > > -###################################################### > > - > > -port=$1 > > -file=$2 > > -speed=$3 > > - > > -pattern_repeat=1500 > > -default_baudrate=115200 > > -tmpfile=/tmp/xmodem.pattern > > -tools=( dd stty sx minicom ) > > - > > -case "$3" in > > - 2) > > - fast_baudrate=230400 > > - prefix="\xF2" > > - ;; > > - 4) > > - fast_baudrate=460800 > > - prefix="\xF4" > > - ;; > > - 8) > > - fast_baudrate=921600 > > - prefix="\xF8" > > - ;; > > - *) > > - fast_baudrate=$default_baudrate > > - prefix="\xBB" > > -esac > > - > > -if [[ -z "$port" || -z "$file" ]] > > -then > > - echo -e "\nMarvell recovery image downloader for Armada SoC family." > > - echo -e "Command syntax:" > > - echo -e "\t$(basename $0) <port> <file> [2|4|8]" > > - echo -e "\tport - serial port the target board is connected to" > > - echo -e "\tfile - recovery boot image for target download" > > - echo -e "\t2|4|8 - times to increase the default serial port speed by" > > - echo -e "For example - load the image over ttyUSB0 @ 460800 baud:" > > - echo -e "$(basename $0) /dev/ttyUSB0 /tmp/flash-image.bin 4\n" > > - echo -e "=====WARNING=====" > > - echo -e "- The speed-up option is not available in SoC families prior to A8K+" > > - echo -e "- This utility is not compatible with Armada 37xx SoC family\n" > > -fi > > - > > -# Sanity checks > > -if [ -c "$port" ] > > -then > > - echo -e "Using device connected on serial port \"$port\"" > > -else > > - echo "Wrong serial port name!" > > - exit 1 > > -fi > > - > > -if [ -f "$file" ] > > -then > > - echo -e "Loading flash image file \"$file\"" > > -else > > - echo "File $file does not exist!" > > - exit 1 > > -fi > > - > > -# Verify required tools installation > > -for tool in ${tools[@]} > > -do > > - toolname=`which $tool` > > - if [ -z "$toolname" ] > > - then > > - echo -e "Missing installation of \"$tool\" --> Exiting" > > - exit 1 > > - fi > > -done > > - > > - > > -echo -e "Recovery will run at $fast_baudrate baud" > > -echo -e "========================================" > > - > > -if [ -f "$tmpfile" ] > > -then > > - rm -f $tmpfile > > -fi > > - > > -# Send the escape sequence to target board using default debug port speed > > -stty -F $port raw ignbrk time 5 $default_baudrate > > -counter=0 > > -while [ $counter -lt $pattern_repeat ]; do > > - echo -n -e "$prefix\x11\x22\x33\x44\x55\x66\x77" >> $tmpfile > > - let counter=counter+1 > > -done > > - > > -echo -en "Press the \"Reset\" button on the target board and " > > -echo -en "the \"Enter\" key on the host keyboard simultaneously" > > -read > > -dd if=$tmpfile of=$port &>/dev/null > > - > > -# Speed up the binary image transfer > > -stty -F $port raw ignbrk time 5 $fast_baudrate > > -sx -vv $file > $port < $port > > -#sx-at91 $port $file > > - > > -# Return the port to the default speed > > -stty -F $port raw ignbrk time 5 $default_baudrate > > - > > -# Optional - fire up Minicom > > -minicom -D $port -b $default_baudrate > > - > > Viele Grüße, > Stefan Roese
Addendum. On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > [snip] > > Kosta, do you see any problems with removing this script? As you might > > have seen, Pali and Marek did some great work on kwboot in the mean > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to the > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > firmware/build/a70x0_rb5009/release/flash-image.bin > Using device connected on serial port "/dev/ttyUSB3" > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" > Recovery will run at 115200 baud > ======================================== > Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 > blocks: Give your local XMODEM receive command now. > Bytes Sent:1456384 BPS:7871 > > Transfer complete > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing something > or > support for booting 64-bit platforms would yet need to be added. If I patch it as follows it actually starts transferring but does not really get too far. diff --git a/tools/kwboot.c b/tools/kwboot.c index d22e6ea96a..4f40da4f7a 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1500,7 +1500,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) image_ver = kwbimage_version(img); if (image_ver != 0 && image_ver != 1) { fprintf(stderr, "Invalid image header version\n"); - goto err; +fprintf(stderr, "IGNORING\n"); +// goto err; } hdrsz = kwbheader_size(hdr); @@ -1782,7 +1783,8 @@ main(int argc, char **argv) rc = kwboot_img_patch(img, &size, baudrate); if (rc) { fprintf(stderr, "%s: Invalid image.\n", imgpath); - goto out; +fprintf(stderr, "IGNORING\n"); +// goto out; } } ⬢[zim@toolbox Research]$ ./kwboot -b ~/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin /dev/ttyUSB3 kwboot version 2022.01-00018-g666d9a62e0-dirty Invalid image header version IGNORING /var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin: Invalid image. IGNORING Sending boot message. Please reboot the target...| Waiting 2s and flushing tty Sending boot image header (13356800 bytes)... 0 % [......................................................................] [snip: 40 more lines] 2 % [...................................................................++.] 2 % [+++++++++++++++++ xmodem: Bad message > Suggestions welcome. Thanks! > > > Thanks, > > Stefan > > [1] https://forum.openwrt.org/t/add-support-for-mikrotik-rb5009ug > > Cheers > > Marcel [snip]
On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > Addendum. > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > [snip] > > > > Kosta, do you see any problems with removing this script? As you might > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to the > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > firmware/build/a70x0_rb5009/release/flash-image.bin > > Using device connected on serial port "/dev/ttyUSB3" > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" > > Recovery will run at 115200 baud > > ======================================== > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 > > blocks: Give your local XMODEM receive command now. > > Bytes Sent:1456384 BPS:7871 > > > > Transfer complete > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing something > > or > > support for booting 64-bit platforms would yet need to be added. > > If I patch it as follows it actually starts transferring but does not really get too far. 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. This format is not supported by U-Boot as U-Boot does not even build images for this format. You even cannot boot U-Boot directly on those 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project which generate images compatible for those SoCs. So U-Boot does not have any support for those 64-bit images. So you should use TF-A tools which generates these 64-bit Armada bootable images. Probably you could use kwboot just for sending boot pattern and then generic "sx" tool (which is used also by that shell script). And after that kwboot again for terminal support. But this does not verify that image is correct and also may have issues if header part contains executable code which prints something to UART... $ kwboot -b /dev/ttyUSB0 $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 $ kwboot -t /dev/ttyUSB0 > diff --git a/tools/kwboot.c b/tools/kwboot.c > index d22e6ea96a..4f40da4f7a 100644 > --- a/tools/kwboot.c > +++ b/tools/kwboot.c > @@ -1500,7 +1500,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) > image_ver = kwbimage_version(img); > if (image_ver != 0 && image_ver != 1) { > fprintf(stderr, "Invalid image header version\n"); > - goto err; > +fprintf(stderr, "IGNORING\n"); > +// goto err; > } > > hdrsz = kwbheader_size(hdr); > @@ -1782,7 +1783,8 @@ main(int argc, char **argv) > rc = kwboot_img_patch(img, &size, baudrate); > if (rc) { > fprintf(stderr, "%s: Invalid image.\n", imgpath); > - goto out; > +fprintf(stderr, "IGNORING\n"); > +// goto out; > } > } > > > ⬢[zim@toolbox Research]$ ./kwboot -b ~/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin > /dev/ttyUSB3 > kwboot version 2022.01-00018-g666d9a62e0-dirty > Invalid image header version > IGNORING > /var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin: Invalid image. > IGNORING > Sending boot message. Please reboot the target...| > Waiting 2s and flushing tty > Sending boot image header (13356800 bytes)... > 0 % [......................................................................] > > [snip: 40 more lines] > > 2 % [...................................................................++.] > 2 % [+++++++++++++++++ > xmodem: Bad message > > > Suggestions welcome. Thanks! > > > > > Thanks, > > > Stefan > > > > [1] https://forum.openwrt.org/t/add-support-for-mikrotik-rb5009ug > > > > Cheers > > > > Marcel > > [snip]
On Sat, 2022-02-05 at 01:25 +0100, Pali Rohár wrote: > On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > > Addendum. > > > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > > > > [snip] > > > > > > Kosta, do you see any problems with removing this script? As you might > > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to > > > the > > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit > > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: > > > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > > firmware/build/a70x0_rb5009/release/flash-image.bin > > > Using device connected on serial port "/dev/ttyUSB3" > > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" > > > Recovery will run at 115200 baud > > > ======================================== > > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously > > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 > > > blocks: Give your local XMODEM receive command now. > > > Bytes Sent:1456384 BPS:7871 > > > > > > Transfer complete > > > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing > > > something > > > or > > > support for booting 64-bit platforms would yet need to be added. > > > > If I patch it as follows it actually starts transferring but does not really get too far. > > 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. > This format is not supported by U-Boot as U-Boot does not even build > images for this format. You even cannot boot U-Boot directly on those > 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project > which generate images compatible for those SoCs. Yes, we do know that. > So U-Boot does not have any support for those 64-bit images. Yes, U-Boot proper basically has to be combined with TF-A using some external tooling. > So you should use TF-A tools which generates these 64-bit Armada > bootable images. Exactly. > Probably you could use kwboot just for sending boot pattern and then > generic "sx" tool (which is used also by that shell script). And after > that kwboot again for terminal support. But this does not verify that > image is correct and also may have issues if header part contains > executable code which prints something to UART... > > $ kwboot -b /dev/ttyUSB0 Hm, at least kwboot from today's master does not allow -b without also giving it an image. > $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 > $ kwboot -t /dev/ttyUSB0 Remember, it is not that we do not have a solution or do not know how this all works. It is rather that we currently use that mrvl_uart.sh script which this patch is about to remove and Stefan enquired about. Thanks, Pali. [snip]
On Saturday 05 February 2022 01:40:23 Marcel Ziswiler wrote: > On Sat, 2022-02-05 at 01:25 +0100, Pali Rohár wrote: > > On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > > > Addendum. > > > > > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > > > > > > > [snip] > > > > > > > > Kosta, do you see any problems with removing this script? As you might > > > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to > > > > the > > > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit > > > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: > > > > > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > > > firmware/build/a70x0_rb5009/release/flash-image.bin > > > > Using device connected on serial port "/dev/ttyUSB3" > > > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" > > > > Recovery will run at 115200 baud > > > > ======================================== > > > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously > > > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 > > > > blocks: Give your local XMODEM receive command now. > > > > Bytes Sent:1456384 BPS:7871 > > > > > > > > Transfer complete > > > > > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing > > > > something > > > > or > > > > support for booting 64-bit platforms would yet need to be added. > > > > > > If I patch it as follows it actually starts transferring but does not really get too far. > > > > 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. > > This format is not supported by U-Boot as U-Boot does not even build > > images for this format. You even cannot boot U-Boot directly on those > > 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project > > which generate images compatible for those SoCs. > > Yes, we do know that. > > > So U-Boot does not have any support for those 64-bit images. > > Yes, U-Boot proper basically has to be combined with TF-A using some > external tooling. > > > So you should use TF-A tools which generates these 64-bit Armada > > bootable images. > > Exactly. > > > > Probably you could use kwboot just for sending boot pattern and then > > generic "sx" tool (which is used also by that shell script). And after > > that kwboot again for terminal support. But this does not verify that > > image is correct and also may have issues if header part contains > > executable code which prints something to UART... > > > > $ 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? > > $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 > > $ kwboot -t /dev/ttyUSB0 > > Remember, it is not that we do not have a solution or do not know how > this all works. It is rather that we currently use that mrvl_uart.sh > script which this patch is about to remove and Stefan enquired about. I see... But I do not know what is the best option for now. If issue with -b is investigated / fixed then that script can be replaced by two (or three) commands. And I think it is better than maintaining two different tools in U-Boot which do same thing. On the other hand kwboot does not support 64-bit Armada format, which mrvl_uart.sh can handle (but only because it does not do any verification nor does not understand image format), so for this purpose it is better. But because U-Boot does not support 64-bit Armada image format, I do not know if this script should be in U-Boot because whole image building is outside of U-Boot responsibility and... probably this tool should be part of TF-A project instead. So at the end, should be this tool maintained in U-Boot project at all if U-Boot itself does not support nor provide tooling for 64-bit Armada images? Any opinion? > Thanks, Pali. > > [snip]
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: > > > On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > > > > Addendum. > > > > > > > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > > > > > > > > > > [snip] > > > > > > > > > > Kosta, do you see any problems with removing this script? As you might > > > > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > > > > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > > > > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt > > > > > to > > > > > the > > > > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32- > > > > > bit > > > > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as > > > > > follows: > > > > > > > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > > > > firmware/build/a70x0_rb5009/release/flash-image.bin > > > > > Using device connected on serial port "/dev/ttyUSB3" > > > > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash- > > > > > image.bin" > > > > > Recovery will run at 115200 baud > > > > > ======================================== > > > > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously > > > > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, > > > > > 11377 > > > > > blocks: Give your local XMODEM receive command now. > > > > > Bytes Sent:1456384 BPS:7871 > > > > > > > > > > Transfer complete > > > > > > > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing > > > > > something > > > > > or > > > > > support for booting 64-bit platforms would yet need to be added. > > > > > > > > If I patch it as follows it actually starts transferring but does not really get too far. > > > > > > 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. > > > This format is not supported by U-Boot as U-Boot does not even build > > > images for this format. You even cannot boot U-Boot directly on those > > > 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project > > > which generate images compatible for those SoCs. > > > > Yes, we do know that. > > > > > So U-Boot does not have any support for those 64-bit images. > > > > Yes, U-Boot proper basically has to be combined with TF-A using some > > external tooling. > > > > > So you should use TF-A tools which generates these 64-bit Armada > > > bootable images. > > > > Exactly. > > > > > > > Probably you could use kwboot just for sending boot pattern and then > > > generic "sx" tool (which is used also by that shell script). And after > > > that kwboot again for terminal support. But this does not verify that > > > image is correct and also may have issues if header part contains > > > executable code which prints something to UART... > > > > > > $ 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. > > > $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 > > > $ kwboot -t /dev/ttyUSB0 > > > > Remember, it is not that we do not have a solution or do not know how > > this all works. It is rather that we currently use that mrvl_uart.sh > > script which this patch is about to remove and Stefan enquired about. > > I see... > > But I do not know what is the best option for now. If issue with -b is > investigated / fixed then that script can be replaced by two (or three) > commands. And I think it is better than maintaining two different tools > in U-Boot which do same thing. On the other hand kwboot does not support > 64-bit Armada format, which mrvl_uart.sh can handle (but only because it > does not do any verification nor does not understand image format), so > for this purpose it is better. But because U-Boot does not support > 64-bit Armada image format, I do not know if this script should be in > U-Boot because whole image building is outside of U-Boot responsibility > and... probably this tool should be part of TF-A project instead. So at > the end, should be this tool maintained in U-Boot project at all if > U-Boot itself does not support nor provide tooling for 64-bit Armada > images? > > Any opinion? > > > Thanks, Pali. > > > > [snip]
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? Seems that I tested only -b -t combination (as I wanted to see that bootrom correctly start sending NAK xmodem bytes).
Hi, Stefan,
On 2/5/22 01:54, 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: >>> On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: >>>> Addendum. >>>> >>>> On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: >>>> >>>>> >>>> [snip] >>>> >>>>>> Kosta, do you see any problems with removing this script? As you might >>>>>> have seen, Pali and Marek did some great work on kwboot in the mean >>>>>> time. Is there anything left in mrvl_uart.sh that kwboot can't handle? >>>>> >>>>> Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). >>>>> >>>>> Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt to >>>>> the >>>>> MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions 32-bit >>>>> platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as follows: >>>>> >>>>> ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- >>>>> firmware/build/a70x0_rb5009/release/flash-image.bin >>>>> Using device connected on serial port "/dev/ttyUSB3" >>>>> Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash-image.bin" >>>>> Recovery will run at 115200 baud >>>>> ======================================== >>>>> Press the "Reset" button on the target board and the "Enter" key on the host keyboard simultaneously >>>>> Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, 11377 >>>>> blocks: Give your local XMODEM receive command now. >>>>> Bytes Sent:1456384 BPS:7871 >>>>> >>>>> Transfer complete >>>>> >>>>> Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing >>>>> something >>>>> or >>>>> support for booting 64-bit platforms would yet need to be added. >>>> >>>> If I patch it as follows it actually starts transferring but does not really get too far. >>> >>> 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. >>> This format is not supported by U-Boot as U-Boot does not even build >>> images for this format. You even cannot boot U-Boot directly on those >>> 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project >>> which generate images compatible for those SoCs. >> >> Yes, we do know that. >> >>> So U-Boot does not have any support for those 64-bit images. >> >> Yes, U-Boot proper basically has to be combined with TF-A using some >> external tooling. >> >>> So you should use TF-A tools which generates these 64-bit Armada >>> bootable images. >> >> Exactly. >> >> >>> Probably you could use kwboot just for sending boot pattern and then >>> generic "sx" tool (which is used also by that shell script). And after >>> that kwboot again for terminal support. But this does not verify that >>> image is correct and also may have issues if header part contains >>> executable code which prints something to UART... >>> >>> $ 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? > >>> $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 >>> $ kwboot -t /dev/ttyUSB0 >> >> Remember, it is not that we do not have a solution or do not know how >> this all works. It is rather that we currently use that mrvl_uart.sh >> script which this patch is about to remove and Stefan enquired about. > > I see... > > But I do not know what is the best option for now. If issue with -b is > investigated / fixed then that script can be replaced by two (or three) > commands. And I think it is better than maintaining two different tools > in U-Boot which do same thing. On the other hand kwboot does not support > 64-bit Armada format, which mrvl_uart.sh can handle (but only because it > does not do any verification nor does not understand image format), so > for this purpose it is better. But because U-Boot does not support > 64-bit Armada image format, I do not know if this script should be in > U-Boot because whole image building is outside of U-Boot responsibility > and... probably this tool should be part of TF-A project instead. So at > the end, should be this tool maintained in U-Boot project at all if > U-Boot itself does not support nor provide tooling for 64-bit Armada > images? > > Any opinion? Frankly, as long as mrvl_uart.sh has features that kwboot does not support, I'm reluctant to completely remove it. Thanks, Stefan
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@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!
On Sun, 2022-02-06 at 10:03 +0100, Stefan Roese wrote: > On 2/5/22 01:54, 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: > > > > On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > > > > > Addendum. > > > > > > > > > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > Kosta, do you see any problems with removing this script? As you might > > > > > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > > > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > > > > > > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > > > > > > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt > > > > > > to > > > > > > the > > > > > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions > > > > > > 32-bit > > > > > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as > > > > > > follows: > > > > > > > > > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > > > > > firmware/build/a70x0_rb5009/release/flash-image.bin > > > > > > Using device connected on serial port "/dev/ttyUSB3" > > > > > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash- > > > > > > image.bin" > > > > > > Recovery will run at 115200 baud > > > > > > ======================================== > > > > > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard > > > > > > simultaneously > > > > > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, > > > > > > 11377 > > > > > > blocks: Give your local XMODEM receive command now. > > > > > > Bytes Sent:1456384 BPS:7871 > > > > > > > > > > > > Transfer complete > > > > > > > > > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing > > > > > > something > > > > > > or > > > > > > support for booting 64-bit platforms would yet need to be added. > > > > > > > > > > If I patch it as follows it actually starts transferring but does not really get too far. > > > > > > > > 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. > > > > This format is not supported by U-Boot as U-Boot does not even build > > > > images for this format. You even cannot boot U-Boot directly on those > > > > 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project > > > > which generate images compatible for those SoCs. > > > > > > Yes, we do know that. > > > > > > > So U-Boot does not have any support for those 64-bit images. > > > > > > Yes, U-Boot proper basically has to be combined with TF-A using some > > > external tooling. > > > > > > > So you should use TF-A tools which generates these 64-bit Armada > > > > bootable images. > > > > > > Exactly. > > > > > > > > > > Probably you could use kwboot just for sending boot pattern and then > > > > generic "sx" tool (which is used also by that shell script). And after > > > > that kwboot again for terminal support. But this does not verify that > > > > image is correct and also may have issues if header part contains > > > > executable code which prints something to UART... > > > > > > > > $ 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? > > > > > > $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 > > > > $ kwboot -t /dev/ttyUSB0 > > > > > > Remember, it is not that we do not have a solution or do not know how > > > this all works. It is rather that we currently use that mrvl_uart.sh > > > script which this patch is about to remove and Stefan enquired about. > > > > I see... > > > > But I do not know what is the best option for now. If issue with -b is > > investigated / fixed then that script can be replaced by two (or three) > > commands. And I think it is better than maintaining two different tools > > in U-Boot which do same thing. On the other hand kwboot does not support > > 64-bit Armada format, which mrvl_uart.sh can handle (but only because it > > does not do any verification nor does not understand image format), so > > for this purpose it is better. But because U-Boot does not support > > 64-bit Armada image format, I do not know if this script should be in > > U-Boot because whole image building is outside of U-Boot responsibility > > and... probably this tool should be part of TF-A project instead. So at > > the end, should be this tool maintained in U-Boot project at all if > > U-Boot itself does not support nor provide tooling for 64-bit Armada > > images? > > > > Any opinion? > > Frankly, as long as mrvl_uart.sh has features that kwboot does not > support, I'm reluctant to completely remove it. Once "standalone" usage is fixed and properly documented I do recall my objection and am completely fine with removing tools/mrvl_uart.sh. Thanks! > Thanks, > Stefan
Most Kirwood boards work with the current kwboot. And those boards have the BootROM version 1.2.1. For a few that don't, mrvl_uart.sh does not work either. And that is due to the defect in the BootROM version 1.1.1. Thanks! Reviewed-by: Tony Dinh <mibodhi@gmail.com> On Sun, Feb 6, 2022 at 11:27 PM Marcel Ziswiler <marcel@ziswiler.com> wrote: > > On Sun, 2022-02-06 at 10:03 +0100, Stefan Roese wrote: > > On 2/5/22 01:54, 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: > > > > > On Saturday 05 February 2022 01:01:28 Marcel Ziswiler wrote: > > > > > > Addendum. > > > > > > > > > > > > On Sat, 2022-02-05 at 00:43 +0100, Marcel Ziswiler wrote: > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > Kosta, do you see any problems with removing this script? As you might > > > > > > > > have seen, Pali and Marek did some great work on kwboot in the mean > > > > > > > > time. Is there anything left in mrvl_uart.sh that kwboot can't handle? > > > > > > > > > > > > > > Disclaimer: I am not really a Kirkwood developer or at least not yet (;-p). > > > > > > > > > > > > > > Recently, we started playing with mainline U-Boot/Linux kernel as part of an effort to port OpenWrt > > > > > > > to > > > > > > > the > > > > > > > MikroTik RB5009UG [1]. It features an Armada 7040 which is a 64-bit Arm SoC while kwboot mentions > > > > > > > 32-bit > > > > > > > platforms only. Anyway, so far I was able to boot it using the good oldé mrvl_uart.sh script as > > > > > > > follows: > > > > > > > > > > > > > > ⬢[zim@toolbox ~]$ ~/u-boot/tools/mrvl_uart.sh /dev/ttyUSB3 ~/arm-trusted- > > > > > > > firmware/build/a70x0_rb5009/release/flash-image.bin > > > > > > > Using device connected on serial port "/dev/ttyUSB3" > > > > > > > Loading flash image file "/var/home/zim/arm-trusted-firmware/build/a70x0_rb5009/release/flash- > > > > > > > image.bin" > > > > > > > Recovery will run at 115200 baud > > > > > > > ======================================== > > > > > > > Press the "Reset" button on the target board and the "Enter" key on the host keyboard > > > > > > > simultaneously > > > > > > > Sending /var/home/zim/Sources/arm-trusted-firmware.git/build/a70x0_rb5009/release/flash-image.bin, > > > > > > > 11377 > > > > > > > blocks: Give your local XMODEM receive command now. > > > > > > > Bytes Sent:1456384 BPS:7871 > > > > > > > > > > > > > > Transfer complete > > > > > > > > > > > > > > Trying this with kwboot instead I was not successful as of yet. Not sure whether I am just missing > > > > > > > something > > > > > > > or > > > > > > > support for booting 64-bit platforms would yet need to be added. > > > > > > > > > > > > If I patch it as follows it actually starts transferring but does not really get too far. > > > > > > > > > > 64-bit Armada SoCs use different image format than 32-bit Armada SoCs. > > > > > This format is not supported by U-Boot as U-Boot does not even build > > > > > images for this format. You even cannot boot U-Boot directly on those > > > > > 64-bit Armada SoCs. It is TF-A what is booted and it is TF-A project > > > > > which generate images compatible for those SoCs. > > > > > > > > Yes, we do know that. > > > > > > > > > So U-Boot does not have any support for those 64-bit images. > > > > > > > > Yes, U-Boot proper basically has to be combined with TF-A using some > > > > external tooling. > > > > > > > > > So you should use TF-A tools which generates these 64-bit Armada > > > > > bootable images. > > > > > > > > Exactly. > > > > > > > > > > > > > Probably you could use kwboot just for sending boot pattern and then > > > > > generic "sx" tool (which is used also by that shell script). And after > > > > > that kwboot again for terminal support. But this does not verify that > > > > > image is correct and also may have issues if header part contains > > > > > executable code which prints something to UART... > > > > > > > > > > $ 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? > > > > > > > > $ sx flash-image.bin 0<>/dev/ttyUSB0 1>&0 > > > > > $ kwboot -t /dev/ttyUSB0 > > > > > > > > Remember, it is not that we do not have a solution or do not know how > > > > this all works. It is rather that we currently use that mrvl_uart.sh > > > > script which this patch is about to remove and Stefan enquired about. > > > > > > I see... > > > > > > But I do not know what is the best option for now. If issue with -b is > > > investigated / fixed then that script can be replaced by two (or three) > > > commands. And I think it is better than maintaining two different tools > > > in U-Boot which do same thing. On the other hand kwboot does not support > > > 64-bit Armada format, which mrvl_uart.sh can handle (but only because it > > > does not do any verification nor does not understand image format), so > > > for this purpose it is better. But because U-Boot does not support > > > 64-bit Armada image format, I do not know if this script should be in > > > U-Boot because whole image building is outside of U-Boot responsibility > > > and... probably this tool should be part of TF-A project instead. So at > > > the end, should be this tool maintained in U-Boot project at all if > > > U-Boot itself does not support nor provide tooling for 64-bit Armada > > > images? > > > > > > Any opinion? > > > > Frankly, as long as mrvl_uart.sh has features that kwboot does not > > support, I'm reluctant to completely remove it. > > Once "standalone" usage is fixed and properly documented I do recall my objection and am completely fine with > removing tools/mrvl_uart.sh. Thanks! > > > Thanks, > > Stefan
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@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.
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/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@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.
On 2/3/22 17:50, Pali Rohár wrote: > There are two tools for sending images over UART to Marvell SoCs: kwboot > and mrvl_uart.sh. kwboot received lot of new features and improvements in > last few months. There is no need to maintain two tools in U-Boot, so > remove old mrvl_uart.sh tool. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > tools/mrvl_uart.sh | 119 --------------------------------------------- > 1 file changed, 119 deletions(-) > delete mode 100755 tools/mrvl_uart.sh Applied to u-boot-marvell/master Thanks, Stefan > diff --git a/tools/mrvl_uart.sh b/tools/mrvl_uart.sh > deleted file mode 100755 > index a46411fc99fb..000000000000 > --- a/tools/mrvl_uart.sh > +++ /dev/null > @@ -1,119 +0,0 @@ > -#!/bin/bash > -# SPDX-License-Identifier: GPL-2.0 > -# > -###################################################### > -# Copyright (C) 2016 Marvell International Ltd. > -# > -# https://spdx.org/licenses > -# > -# Author: Konstantin Porotchkin kostap@marvell.com > -# > -# Version 0.3 > -# > -# UART recovery downloader for Armada SoCs > -# > -###################################################### > - > -port=$1 > -file=$2 > -speed=$3 > - > -pattern_repeat=1500 > -default_baudrate=115200 > -tmpfile=/tmp/xmodem.pattern > -tools=( dd stty sx minicom ) > - > -case "$3" in > - 2) > - fast_baudrate=230400 > - prefix="\xF2" > - ;; > - 4) > - fast_baudrate=460800 > - prefix="\xF4" > - ;; > - 8) > - fast_baudrate=921600 > - prefix="\xF8" > - ;; > - *) > - fast_baudrate=$default_baudrate > - prefix="\xBB" > -esac > - > -if [[ -z "$port" || -z "$file" ]] > -then > - echo -e "\nMarvell recovery image downloader for Armada SoC family." > - echo -e "Command syntax:" > - echo -e "\t$(basename $0) <port> <file> [2|4|8]" > - echo -e "\tport - serial port the target board is connected to" > - echo -e "\tfile - recovery boot image for target download" > - echo -e "\t2|4|8 - times to increase the default serial port speed by" > - echo -e "For example - load the image over ttyUSB0 @ 460800 baud:" > - echo -e "$(basename $0) /dev/ttyUSB0 /tmp/flash-image.bin 4\n" > - echo -e "=====WARNING=====" > - echo -e "- The speed-up option is not available in SoC families prior to A8K+" > - echo -e "- This utility is not compatible with Armada 37xx SoC family\n" > -fi > - > -# Sanity checks > -if [ -c "$port" ] > -then > - echo -e "Using device connected on serial port \"$port\"" > -else > - echo "Wrong serial port name!" > - exit 1 > -fi > - > -if [ -f "$file" ] > -then > - echo -e "Loading flash image file \"$file\"" > -else > - echo "File $file does not exist!" > - exit 1 > -fi > - > -# Verify required tools installation > -for tool in ${tools[@]} > -do > - toolname=`which $tool` > - if [ -z "$toolname" ] > - then > - echo -e "Missing installation of \"$tool\" --> Exiting" > - exit 1 > - fi > -done > - > - > -echo -e "Recovery will run at $fast_baudrate baud" > -echo -e "========================================" > - > -if [ -f "$tmpfile" ] > -then > - rm -f $tmpfile > -fi > - > -# Send the escape sequence to target board using default debug port speed > -stty -F $port raw ignbrk time 5 $default_baudrate > -counter=0 > -while [ $counter -lt $pattern_repeat ]; do > - echo -n -e "$prefix\x11\x22\x33\x44\x55\x66\x77" >> $tmpfile > - let counter=counter+1 > -done > - > -echo -en "Press the \"Reset\" button on the target board and " > -echo -en "the \"Enter\" key on the host keyboard simultaneously" > -read > -dd if=$tmpfile of=$port &>/dev/null > - > -# Speed up the binary image transfer > -stty -F $port raw ignbrk time 5 $fast_baudrate > -sx -vv $file > $port < $port > -#sx-at91 $port $file > - > -# Return the port to the default speed > -stty -F $port raw ignbrk time 5 $default_baudrate > - > -# Optional - fire up Minicom > -minicom -D $port -b $default_baudrate > - Viele Grüße, Stefan Roese
diff --git a/tools/mrvl_uart.sh b/tools/mrvl_uart.sh deleted file mode 100755 index a46411fc99fb..000000000000 --- a/tools/mrvl_uart.sh +++ /dev/null @@ -1,119 +0,0 @@ -#!/bin/bash -# SPDX-License-Identifier: GPL-2.0 -# -###################################################### -# Copyright (C) 2016 Marvell International Ltd. -# -# https://spdx.org/licenses -# -# Author: Konstantin Porotchkin kostap@marvell.com -# -# Version 0.3 -# -# UART recovery downloader for Armada SoCs -# -###################################################### - -port=$1 -file=$2 -speed=$3 - -pattern_repeat=1500 -default_baudrate=115200 -tmpfile=/tmp/xmodem.pattern -tools=( dd stty sx minicom ) - -case "$3" in - 2) - fast_baudrate=230400 - prefix="\xF2" - ;; - 4) - fast_baudrate=460800 - prefix="\xF4" - ;; - 8) - fast_baudrate=921600 - prefix="\xF8" - ;; - *) - fast_baudrate=$default_baudrate - prefix="\xBB" -esac - -if [[ -z "$port" || -z "$file" ]] -then - echo -e "\nMarvell recovery image downloader for Armada SoC family." - echo -e "Command syntax:" - echo -e "\t$(basename $0) <port> <file> [2|4|8]" - echo -e "\tport - serial port the target board is connected to" - echo -e "\tfile - recovery boot image for target download" - echo -e "\t2|4|8 - times to increase the default serial port speed by" - echo -e "For example - load the image over ttyUSB0 @ 460800 baud:" - echo -e "$(basename $0) /dev/ttyUSB0 /tmp/flash-image.bin 4\n" - echo -e "=====WARNING=====" - echo -e "- The speed-up option is not available in SoC families prior to A8K+" - echo -e "- This utility is not compatible with Armada 37xx SoC family\n" -fi - -# Sanity checks -if [ -c "$port" ] -then - echo -e "Using device connected on serial port \"$port\"" -else - echo "Wrong serial port name!" - exit 1 -fi - -if [ -f "$file" ] -then - echo -e "Loading flash image file \"$file\"" -else - echo "File $file does not exist!" - exit 1 -fi - -# Verify required tools installation -for tool in ${tools[@]} -do - toolname=`which $tool` - if [ -z "$toolname" ] - then - echo -e "Missing installation of \"$tool\" --> Exiting" - exit 1 - fi -done - - -echo -e "Recovery will run at $fast_baudrate baud" -echo -e "========================================" - -if [ -f "$tmpfile" ] -then - rm -f $tmpfile -fi - -# Send the escape sequence to target board using default debug port speed -stty -F $port raw ignbrk time 5 $default_baudrate -counter=0 -while [ $counter -lt $pattern_repeat ]; do - echo -n -e "$prefix\x11\x22\x33\x44\x55\x66\x77" >> $tmpfile - let counter=counter+1 -done - -echo -en "Press the \"Reset\" button on the target board and " -echo -en "the \"Enter\" key on the host keyboard simultaneously" -read -dd if=$tmpfile of=$port &>/dev/null - -# Speed up the binary image transfer -stty -F $port raw ignbrk time 5 $fast_baudrate -sx -vv $file > $port < $port -#sx-at91 $port $file - -# Return the port to the default speed -stty -F $port raw ignbrk time 5 $default_baudrate - -# Optional - fire up Minicom -minicom -D $port -b $default_baudrate -
There are two tools for sending images over UART to Marvell SoCs: kwboot and mrvl_uart.sh. kwboot received lot of new features and improvements in last few months. There is no need to maintain two tools in U-Boot, so remove old mrvl_uart.sh tool. Signed-off-by: Pali Rohár <pali@kernel.org> --- tools/mrvl_uart.sh | 119 --------------------------------------------- 1 file changed, 119 deletions(-) delete mode 100755 tools/mrvl_uart.sh