[PATCH] tools/mrvl_uart.sh: Remove script

[PATCH] tools/mrvl_uart.sh: Remove script

[Pali Rohár]

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

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
-
-- 
2.20.1

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marek Behún]

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>

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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]

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Pali Rohár]

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]

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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]

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Pali Rohár]

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]

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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]

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Pali Rohár]

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).

Re: [EXT] Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Kostya Porotchkin]

Hi, Stefan,

________________________________
From: Stefan Roese <sr@denx.de>
Sent: Friday, February 4, 2022 07:46
To: Pali Rohár <pali@kernel.org>; Marek Behún <marek.behun@nic.cz>
Cc: u-boot@lists.denx.de <u-boot@lists.denx.de>; Kostya Porotchkin <kostap@marvell.com>
Subject: [EXT] Re: [PATCH] tools/mrvl_uart.sh: Remove script

External Email

----------------------------------------------------------------------
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?

I have no objections. If it works with other utility, it's good enough to me.

Regards
Kosta

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://urldefense.proofpoint.com/v2/url?u=https-3A__spdx.org_licenses&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=-N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o&m=-xB-ulRTo6jmziJtjKO-uZIA3fRrVF1eO7iCic2512bsXq1dncHj7MZfGwgeHf5b&s=woGkTBcqlZAvwkBGvYPkCgAtlak70RHo6ojWfMA5tgA&e=
> -#
> -# 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

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Stefan Roese]

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

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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!

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Marcel Ziswiler]

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

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Tony Dinh]

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

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Pali Rohár]

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.

Re: [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@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.

Re: [PATCH] tools/mrvl_uart.sh: Remove script

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de