[U-Boot][PATCH 0/2] Improve fpga cmd usage information

[U-Boot][PATCH 0/2] Improve fpga cmd usage information

[Ibai Erkiaga]

Current usage information is not read friendly specially when all the
options are selected. This series fix some of the issues in the info,
from one side aligment and overlapping issues and from the other the
excess of verbosity on the loads command.


Ibai Erkiaga (2):
  fpga: fix alignment on fpga cmd usage info
  fpga: improve loads usage information

 cmd/fpga.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

-- 
2.42.0.325.g3a06386e31

[U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

[Ibai Erkiaga]

The current implementation generates some alignment issues as well as
some overlapping when all the fpga command options are enabled. The fix
is intended to improve readability of the usage info.

Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>

---

The current output looks like this:
  loadbp        [dev] [address] [size]  Load device from bitstream buffer with partial bitstream(Xilinx only)
Load device from filesystem (FAT by default) (Xilinx only)
  loadfs [dev] [address] [image size] [blocksize] <interface>
        [<dev[:part]>] <filename>
  loadmk [dev] [address]        Load device generated with mkimage
        For loadmk operating on FIT format uImage address must include
        subimage unit name in the form of addr:<subimg_uname>Load encrypted bitstream (Xilinx only)
  loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
        [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
Loads the secure bistreams(authenticated/encrypted/both

The new output lools like this:
loadb  [dev] [address] [size]  Load device from bitstream buffer (Xilinx only)
 loadbp [dev] [address] [size]  Load device from bitstream buffer with partial bitstream (Xilinx only)
 loadfs [dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>
        Load device from filesystem (FAT by default) (Xilinx only)
 loadmk [dev] [address]         Load device generated with mkimage
        NOTE: loadmk operating on FIT format uImage address must include
        subimage unit name in the form of addr:<subimg_uname>
 loads  [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
        [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
        Loads the secure bistreams(authenticated/encrypted/both

---
 cmd/fpga.c | 51 ++++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/cmd/fpga.c b/cmd/fpga.c
index 93f14098ccb..cba38662958 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -411,46 +411,43 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper,
 	   "loadable FPGA image support",
 	   "[operation type] [device number] [image address] [image size]\n"
 	   "fpga operations:\n"
-	   "  dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
-	   "  info\t[dev]\t\t\tlist known device information\n"
-	   "  load\t[dev] [address] [size]\tLoad device from memory buffer\n"
+	   " dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
+	   " info\t[dev]\t\t\tList known device information\n"
+	   " load\t[dev] [address] [size]\tLoad device from memory buffer\n"
 #if defined(CONFIG_CMD_FPGA_LOADP)
-	   "  loadp\t[dev] [address] [size]\t"
+	   " loadp\t[dev] [address] [size]\t"
 	   "Load device from memory buffer with partial bitstream\n"
 #endif
-	   "  loadb\t[dev] [address] [size]\t"
+	   " loadb\t[dev] [address] [size]\t"
 	   "Load device from bitstream buffer (Xilinx only)\n"
 #if defined(CONFIG_CMD_FPGA_LOADBP)
-	   "  loadbp\t[dev] [address] [size]\t"
-	   "Load device from bitstream buffer with partial bitstream"
+	   " loadbp\t[dev] [address] [size]\t"
+	   "Load device from bitstream buffer with partial bitstream "
 	   "(Xilinx only)\n"
 #endif
 #if defined(CONFIG_CMD_FPGA_LOADFS)
-	   "Load device from filesystem (FAT by default) (Xilinx only)\n"
-	   "  loadfs [dev] [address] [image size] [blocksize] <interface>\n"
-	   "        [<dev[:part]>] <filename>\n"
+	   " loadfs\t[dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>\n"
+	   "\tLoad device from filesystem (FAT by default) (Xilinx only)\n"
 #endif
 #if defined(CONFIG_CMD_FPGA_LOADMK)
-	   "  loadmk [dev] [address]\tLoad device generated with mkimage"
+	   " loadmk\t[dev] [address]\t\tLoad device generated with mkimage\n"
 #if defined(CONFIG_FIT)
-	   "\n"
-	   "\tFor loadmk operating on FIT format uImage address must include\n"
-	   "\tsubimage unit name in the form of addr:<subimg_uname>"
+	   "\tNOTE: loadmk operating on FIT format uImage address must include\n"
+	   "\tsubimage unit name in the form of addr:<subimg_uname>\n"
 #endif
 #endif
 #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
-	   "Load encrypted bitstream (Xilinx only)\n"
-	   "  loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
-	   "        [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
-	   "Loads the secure bistreams(authenticated/encrypted/both\n"
-	   "authenticated and encrypted) of [size] from [address].\n"
-	   "The auth-OCM/DDR flag specifies to perform authentication\n"
-	   "in OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
-	   "The enc flag specifies which key to be used for decryption\n"
-	   "0-device key, 1-user key, 2-no encryption.\n"
-	   "The optional Userkey address specifies from which address key\n"
-	   "has to be used for decryption if user key is selected.\n"
-	   "NOTE: the secure bitstream has to be created using Xilinx\n"
-	   "bootgen tool only.\n"
+	   " loads\t[dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
+	   "\t[enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
+	   "\tLoads the secure bistreams(authenticated/encrypted/both\n"
+	   "\tauthenticated and encrypted) of [size] from [address] (Xilinx only)\n"
+	   "\t -The auth-OCM/DDR flag specifies to perform authentication\n"
+	   "\tin OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
+	   "\t -The enc flag specifies which key to be used for decryption\n"
+	   "\t0-device key, 1-user key, 2-no encryption.\n"
+	   "\t -The optional Userkey address specifies from which address key\n"
+	   "\thas to be used for decryption if user key is selected.\n"
+	   "\tNOTE: the secure bitstream has to be created using Xilinx\n"
+	   "\tbootgen tool only.\n"
 #endif
 );
-- 
2.42.0.325.g3a06386e31

[U-Boot][PATCH 2/2] fpga: improve loads usage information

[Ibai Erkiaga]

Current usage information for loads command is too verbose and long for
a command usage prompt. This flag simplifies the text for readability
purposes.

Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
---

The authentication and encryption flag naming is redundant with the
description notes included. Use authflag and encflag names for the
command pattern and use description lines for explanation.

New output looks like this:
loads  [dev] [address] [size] [authflag] [encflag] [Userkey address]
        Load device from memory buffer with secure bistream (authenticated/encrypted/both)(Xilinx only)
         -authflag: 0 for OCM, 1 for DDR, 2 for no authentication (specifies where to perform authentication)
         -encflag: 0 for device key, 1 for user key, 2 for no encryption
         -Userkey address: specifies the address where user key is stored when user key is selected
        NOTE: the secure bitstream has to be created using Xilinx bootgen tool only.

---
 cmd/fpga.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/cmd/fpga.c b/cmd/fpga.c
index cba38662958..cbcbe9c32f1 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -437,17 +437,11 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper,
 #endif
 #endif
 #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
-	   " loads\t[dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
-	   "\t[enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
-	   "\tLoads the secure bistreams(authenticated/encrypted/both\n"
-	   "\tauthenticated and encrypted) of [size] from [address] (Xilinx only)\n"
-	   "\t -The auth-OCM/DDR flag specifies to perform authentication\n"
-	   "\tin OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
-	   "\t -The enc flag specifies which key to be used for decryption\n"
-	   "\t0-device key, 1-user key, 2-no encryption.\n"
-	   "\t -The optional Userkey address specifies from which address key\n"
-	   "\thas to be used for decryption if user key is selected.\n"
-	   "\tNOTE: the secure bitstream has to be created using Xilinx\n"
-	   "\tbootgen tool only.\n"
+	   " loads\t[dev] [address] [size] [authflag] [encflag] [Userkey address]\n"
+	   "\tLoad device from memory buffer with secure bistream (authenticated/encrypted/both)(Xilinx only)\n"
+	   "\t -authflag: 0 for OCM, 1 for DDR, 2 for no authentication (specifies where to perform authentication)\n"
+	   "\t -encflag: 0 for device key, 1 for user key, 2 for no encryption\n"
+	   "\t -Userkey address: specifies the address where user key is stored when user key is selected\n"
+	   "\tNOTE: the secure bitstream has to be created using Xilinx bootgen tool only.\n"
 #endif
 );
-- 
2.42.0.325.g3a06386e31

Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

[Alexander Dahl]

Hello,

Am Wed, Jan 08, 2025 at 02:03:50PM +0000 schrieb Ibai Erkiaga:
> The current implementation generates some alignment issues as well as
> some overlapping when all the fpga command options are enabled. The fix
> is intended to improve readability of the usage info.

Alignment could indeed improved.  One question: the \t is dependent on
the terminal, right?  Would it be better to use spaces instead?  That
would also make the code more readable.

Greets
Alex

> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
> 
> ---
> 
> The current output looks like this:
>   loadbp        [dev] [address] [size]  Load device from bitstream buffer with partial bitstream(Xilinx only)
> Load device from filesystem (FAT by default) (Xilinx only)
>   loadfs [dev] [address] [image size] [blocksize] <interface>
>         [<dev[:part]>] <filename>
>   loadmk [dev] [address]        Load device generated with mkimage
>         For loadmk operating on FIT format uImage address must include
>         subimage unit name in the form of addr:<subimg_uname>Load encrypted bitstream (Xilinx only)
>   loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>         [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
> Loads the secure bistreams(authenticated/encrypted/both
> 
> The new output lools like this:
> loadb  [dev] [address] [size]  Load device from bitstream buffer (Xilinx only)
>  loadbp [dev] [address] [size]  Load device from bitstream buffer with partial bitstream (Xilinx only)
>  loadfs [dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>
>         Load device from filesystem (FAT by default) (Xilinx only)
>  loadmk [dev] [address]         Load device generated with mkimage
>         NOTE: loadmk operating on FIT format uImage address must include
>         subimage unit name in the form of addr:<subimg_uname>
>  loads  [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>         [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
>         Loads the secure bistreams(authenticated/encrypted/both
> 
> ---
>  cmd/fpga.c | 51 ++++++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index 93f14098ccb..cba38662958 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -411,46 +411,43 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper,
>  	   "loadable FPGA image support",
>  	   "[operation type] [device number] [image address] [image size]\n"
>  	   "fpga operations:\n"
> -	   "  dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
> -	   "  info\t[dev]\t\t\tlist known device information\n"
> -	   "  load\t[dev] [address] [size]\tLoad device from memory buffer\n"
> +	   " dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
> +	   " info\t[dev]\t\t\tList known device information\n"
> +	   " load\t[dev] [address] [size]\tLoad device from memory buffer\n"
>  #if defined(CONFIG_CMD_FPGA_LOADP)
> -	   "  loadp\t[dev] [address] [size]\t"
> +	   " loadp\t[dev] [address] [size]\t"
>  	   "Load device from memory buffer with partial bitstream\n"
>  #endif
> -	   "  loadb\t[dev] [address] [size]\t"
> +	   " loadb\t[dev] [address] [size]\t"
>  	   "Load device from bitstream buffer (Xilinx only)\n"
>  #if defined(CONFIG_CMD_FPGA_LOADBP)
> -	   "  loadbp\t[dev] [address] [size]\t"
> -	   "Load device from bitstream buffer with partial bitstream"
> +	   " loadbp\t[dev] [address] [size]\t"
> +	   "Load device from bitstream buffer with partial bitstream "
>  	   "(Xilinx only)\n"
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOADFS)
> -	   "Load device from filesystem (FAT by default) (Xilinx only)\n"
> -	   "  loadfs [dev] [address] [image size] [blocksize] <interface>\n"
> -	   "        [<dev[:part]>] <filename>\n"
> +	   " loadfs\t[dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>\n"
> +	   "\tLoad device from filesystem (FAT by default) (Xilinx only)\n"
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOADMK)
> -	   "  loadmk [dev] [address]\tLoad device generated with mkimage"
> +	   " loadmk\t[dev] [address]\t\tLoad device generated with mkimage\n"
>  #if defined(CONFIG_FIT)
> -	   "\n"
> -	   "\tFor loadmk operating on FIT format uImage address must include\n"
> -	   "\tsubimage unit name in the form of addr:<subimg_uname>"
> +	   "\tNOTE: loadmk operating on FIT format uImage address must include\n"
> +	   "\tsubimage unit name in the form of addr:<subimg_uname>\n"
>  #endif
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
> -	   "Load encrypted bitstream (Xilinx only)\n"
> -	   "  loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
> -	   "        [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
> -	   "Loads the secure bistreams(authenticated/encrypted/both\n"
> -	   "authenticated and encrypted) of [size] from [address].\n"
> -	   "The auth-OCM/DDR flag specifies to perform authentication\n"
> -	   "in OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
> -	   "The enc flag specifies which key to be used for decryption\n"
> -	   "0-device key, 1-user key, 2-no encryption.\n"
> -	   "The optional Userkey address specifies from which address key\n"
> -	   "has to be used for decryption if user key is selected.\n"
> -	   "NOTE: the secure bitstream has to be created using Xilinx\n"
> -	   "bootgen tool only.\n"
> +	   " loads\t[dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
> +	   "\t[enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
> +	   "\tLoads the secure bistreams(authenticated/encrypted/both\n"
> +	   "\tauthenticated and encrypted) of [size] from [address] (Xilinx only)\n"
> +	   "\t -The auth-OCM/DDR flag specifies to perform authentication\n"
> +	   "\tin OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
> +	   "\t -The enc flag specifies which key to be used for decryption\n"
> +	   "\t0-device key, 1-user key, 2-no encryption.\n"
> +	   "\t -The optional Userkey address specifies from which address key\n"
> +	   "\thas to be used for decryption if user key is selected.\n"
> +	   "\tNOTE: the secure bitstream has to be created using Xilinx\n"
> +	   "\tbootgen tool only.\n"
>  #endif
>  );
> -- 
> 2.42.0.325.g3a06386e31
> 

Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

[Erkiaga Elorza, Ibai]

[AMD Official Use Only - AMD Internal Distribution Only]

Agree Alex,

I just used /t to stick as much as possible to the current implementation, but I'm open to change to white spaces if Michal thinks it's better.

Regards
Ibai

________________________________
From: Alexander Dahl <ada@thorsis.com>
Sent: 09 January 2025 07:52
To: Erkiaga Elorza, Ibai <ibai.erkiaga-elorza@amd.com>
Cc: u-boot@lists.denx.de <u-boot@lists.denx.de>; Simek, Michal <michal.simek@amd.com>
Subject: Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Hello,

Am Wed, Jan 08, 2025 at 02:03:50PM +0000 schrieb Ibai Erkiaga:
> The current implementation generates some alignment issues as well as
> some overlapping when all the fpga command options are enabled. The fix
> is intended to improve readability of the usage info.

Alignment could indeed improved.  One question: the \t is dependent on
the terminal, right?  Would it be better to use spaces instead?  That
would also make the code more readable.

Greets
Alex

>
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
>
> ---
>
> The current output looks like this:
>   loadbp        [dev] [address] [size]  Load device from bitstream buffer with partial bitstream(Xilinx only)
> Load device from filesystem (FAT by default) (Xilinx only)
>   loadfs [dev] [address] [image size] [blocksize] <interface>
>         [<dev[:part]>] <filename>
>   loadmk [dev] [address]        Load device generated with mkimage
>         For loadmk operating on FIT format uImage address must include
>         subimage unit name in the form of addr:<subimg_uname>Load encrypted bitstream (Xilinx only)
>   loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>         [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
> Loads the secure bistreams(authenticated/encrypted/both
>
> The new output lools like this:
> loadb  [dev] [address] [size]  Load device from bitstream buffer (Xilinx only)
>  loadbp [dev] [address] [size]  Load device from bitstream buffer with partial bitstream (Xilinx only)
>  loadfs [dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>
>         Load device from filesystem (FAT by default) (Xilinx only)
>  loadmk [dev] [address]         Load device generated with mkimage
>         NOTE: loadmk operating on FIT format uImage address must include
>         subimage unit name in the form of addr:<subimg_uname>
>  loads  [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>         [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
>         Loads the secure bistreams(authenticated/encrypted/both
>
> ---
>  cmd/fpga.c | 51 ++++++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index 93f14098ccb..cba38662958 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -411,46 +411,43 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper,
>          "loadable FPGA image support",
>          "[operation type] [device number] [image address] [image size]\n"
>          "fpga operations:\n"
> -        "  dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
> -        "  info\t[dev]\t\t\tlist known device information\n"
> -        "  load\t[dev] [address] [size]\tLoad device from memory buffer\n"
> +        " dump\t[dev] [address] [size]\tLoad device to memory buffer\n"
> +        " info\t[dev]\t\t\tList known device information\n"
> +        " load\t[dev] [address] [size]\tLoad device from memory buffer\n"
>  #if defined(CONFIG_CMD_FPGA_LOADP)
> -        "  loadp\t[dev] [address] [size]\t"
> +        " loadp\t[dev] [address] [size]\t"
>          "Load device from memory buffer with partial bitstream\n"
>  #endif
> -        "  loadb\t[dev] [address] [size]\t"
> +        " loadb\t[dev] [address] [size]\t"
>          "Load device from bitstream buffer (Xilinx only)\n"
>  #if defined(CONFIG_CMD_FPGA_LOADBP)
> -        "  loadbp\t[dev] [address] [size]\t"
> -        "Load device from bitstream buffer with partial bitstream"
> +        " loadbp\t[dev] [address] [size]\t"
> +        "Load device from bitstream buffer with partial bitstream "
>          "(Xilinx only)\n"
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOADFS)
> -        "Load device from filesystem (FAT by default) (Xilinx only)\n"
> -        "  loadfs [dev] [address] [image size] [blocksize] <interface>\n"
> -        "        [<dev[:part]>] <filename>\n"
> +        " loadfs\t[dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>\n"
> +        "\tLoad device from filesystem (FAT by default) (Xilinx only)\n"
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOADMK)
> -        "  loadmk [dev] [address]\tLoad device generated with mkimage"
> +        " loadmk\t[dev] [address]\t\tLoad device generated with mkimage\n"
>  #if defined(CONFIG_FIT)
> -        "\n"
> -        "\tFor loadmk operating on FIT format uImage address must include\n"
> -        "\tsubimage unit name in the form of addr:<subimg_uname>"
> +        "\tNOTE: loadmk operating on FIT format uImage address must include\n"
> +        "\tsubimage unit name in the form of addr:<subimg_uname>\n"
>  #endif
>  #endif
>  #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
> -        "Load encrypted bitstream (Xilinx only)\n"
> -        "  loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
> -        "        [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
> -        "Loads the secure bistreams(authenticated/encrypted/both\n"
> -        "authenticated and encrypted) of [size] from [address].\n"
> -        "The auth-OCM/DDR flag specifies to perform authentication\n"
> -        "in OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
> -        "The enc flag specifies which key to be used for decryption\n"
> -        "0-device key, 1-user key, 2-no encryption.\n"
> -        "The optional Userkey address specifies from which address key\n"
> -        "has to be used for decryption if user key is selected.\n"
> -        "NOTE: the secure bitstream has to be created using Xilinx\n"
> -        "bootgen tool only.\n"
> +        " loads\t[dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
> +        "\t[enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
> +        "\tLoads the secure bistreams(authenticated/encrypted/both\n"
> +        "\tauthenticated and encrypted) of [size] from [address] (Xilinx only)\n"
> +        "\t -The auth-OCM/DDR flag specifies to perform authentication\n"
> +        "\tin OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
> +        "\t -The enc flag specifies which key to be used for decryption\n"
> +        "\t0-device key, 1-user key, 2-no encryption.\n"
> +        "\t -The optional Userkey address specifies from which address key\n"
> +        "\thas to be used for decryption if user key is selected.\n"
> +        "\tNOTE: the secure bitstream has to be created using Xilinx\n"
> +        "\tbootgen tool only.\n"
>  #endif
>  );
> --
> 2.42.0.325.g3a06386e31
>

Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

[Michal Simek]

On 1/9/25 13:45, Erkiaga Elorza, Ibai wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> 
> Agree Alex,
> 
> I just used /t to stick as much as possible to the current implementation, but 
> I'm open to change to white spaces if Michal thinks it's better.
> 
> Regards
> Ibai
> 
> --------------------------------------------------------------------------------
> *From:* Alexander Dahl <ada@thorsis.com>
> *Sent:* 09 January 2025 07:52
> *To:* Erkiaga Elorza, Ibai <ibai.erkiaga-elorza@amd.com>
> *Cc:* u-boot@lists.denx.de <u-boot@lists.denx.de>; Simek, Michal 
> <michal.simek@amd.com>
> *Subject:* Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info
> Caution: This message originated from an External Source. Use proper caution 
> when opening attachments, clicking links, or responding.
> 
> 
> Hello,
> 
> Am Wed, Jan 08, 2025 at 02:03:50PM +0000 schrieb Ibai Erkiaga:
>> The current implementation generates some alignment issues as well as
>> some overlapping when all the fpga command options are enabled. The fix
>> is intended to improve readability of the usage info.
> 
> Alignment could indeed improved.  One question: the \t is dependent on
> the terminal, right?  Would it be better to use spaces instead?  That
> would also make the code more readable.

Agree. Even other commands are using tabs using spaces would be better.

Thanks,
Michal

Re: [U-Boot][PATCH 2/2] fpga: improve loads usage information

[Michal Simek]

On 1/8/25 15:03, Ibai Erkiaga wrote:
> Current usage information for loads command is too verbose and long for
> a command usage prompt. This flag simplifies the text for readability
> purposes.
> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
> ---
> 
> The authentication and encryption flag naming is redundant with the
> description notes included. Use authflag and encflag names for the
> command pattern and use description lines for explanation.
> 
> New output looks like this:
> loads  [dev] [address] [size] [authflag] [encflag] [Userkey address]
>          Load device from memory buffer with secure bistream (authenticated/encrypted/both)(Xilinx only)
>           -authflag: 0 for OCM, 1 for DDR, 2 for no authentication (specifies where to perform authentication)
>           -encflag: 0 for device key, 1 for user key, 2 for no encryption
>           -Userkey address: specifies the address where user key is stored when user key is selected
>          NOTE: the secure bitstream has to be created using Xilinx bootgen tool only.
> 
> ---
>   cmd/fpga.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index cba38662958..cbcbe9c32f1 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -437,17 +437,11 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga_wrapper,
>   #endif
>   #endif
>   #if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
> -	   " loads\t[dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n"
> -	   "\t[enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n"
> -	   "\tLoads the secure bistreams(authenticated/encrypted/both\n"
> -	   "\tauthenticated and encrypted) of [size] from [address] (Xilinx only)\n"
> -	   "\t -The auth-OCM/DDR flag specifies to perform authentication\n"
> -	   "\tin OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n"
> -	   "\t -The enc flag specifies which key to be used for decryption\n"
> -	   "\t0-device key, 1-user key, 2-no encryption.\n"
> -	   "\t -The optional Userkey address specifies from which address key\n"
> -	   "\thas to be used for decryption if user key is selected.\n"
> -	   "\tNOTE: the secure bitstream has to be created using Xilinx\n"
> -	   "\tbootgen tool only.\n"
> +	   " loads\t[dev] [address] [size] [authflag] [encflag] [Userkey address]\n"
> +	   "\tLoad device from memory buffer with secure bistream (authenticated/encrypted/both)(Xilinx only)\n"
> +	   "\t -authflag: 0 for OCM, 1 for DDR, 2 for no authentication (specifies where to perform authentication)\n"
> +	   "\t -encflag: 0 for device key, 1 for user key, 2 for no encryption\n"
> +	   "\t -Userkey address: specifies the address where user key is stored when user key is selected\n"
> +	   "\tNOTE: the secure bitstream has to be created using Xilinx bootgen tool only.\n"
>   #endif
>   );

When you are on this one I can't see any reason to state that this is Xilinx 
only. Pretty much CONFIG_CMD_FPGA_LOAD_SECURE should depends on FPGA_XILINX and 
then you don't need to state it in help. The same is for that Kconfig symbol.

CONFIG_CMD_FPGA_LOADFS
CONFIG_CMD_FPGA_LOAD_SECURE
and LOADB should have own symbol too which also depends on FPGA_XILINX.

Then you can remove Xilinx only from help too.

Thanks,
Michal

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 4c4ad9d99799..89f0c162145c 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1178,8 +1178,8 @@ config CMD_FPGA_LOADBP
           a partial bitstream.

  config CMD_FPGA_LOADFS
-       bool "fpga loadfs - load bitstream from FAT filesystem (Xilinx only)"
-       depends on CMD_FPGA
+       bool "fpga loadfs - load bitstream from FAT filesystem"
+       depends on CMD_FPGA && FPGA_XILINX
         help
           Supports loading an FPGA device from a FAT filesystem.

@@ -1198,7 +1198,7 @@ config CMD_FPGA_LOADP

  config CMD_FPGA_LOAD_SECURE
         bool "fpga loads - loads secure bitstreams"
-       depends on CMD_FPGA
+       depends on CMD_FPGA && FPGA_XILINX
         select FPGA_LOAD_SECURE
         help
           Enables the fpga loads command which is used to load secure

Re: [U-Boot][PATCH 1/2] fpga: fix alignment on fpga cmd usage info

[Michal Simek]

On 1/8/25 15:03, Ibai Erkiaga wrote:
> The current implementation generates some alignment issues as well as
> some overlapping when all the fpga command options are enabled. The fix
> is intended to improve readability of the usage info.
> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
> 
> ---
> 
> The current output looks like this:
>    loadbp        [dev] [address] [size]  Load device from bitstream buffer with partial bitstream(Xilinx only)
> Load device from filesystem (FAT by default) (Xilinx only)
>    loadfs [dev] [address] [image size] [blocksize] <interface>
>          [<dev[:part]>] <filename>
>    loadmk [dev] [address]        Load device generated with mkimage
>          For loadmk operating on FIT format uImage address must include
>          subimage unit name in the form of addr:<subimg_uname>Load encrypted bitstream (Xilinx only)
>    loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>          [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
> Loads the secure bistreams(authenticated/encrypted/both
> 
> The new output lools like this:
> loadb  [dev] [address] [size]  Load device from bitstream buffer (Xilinx only)
>   loadbp [dev] [address] [size]  Load device from bitstream buffer with partial bitstream (Xilinx only)
>   loadfs [dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>
>          Load device from filesystem (FAT by default) (Xilinx only)
>   loadmk [dev] [address]         Load device generated with mkimage
>          NOTE: loadmk operating on FIT format uImage address must include
>          subimage unit name in the form of addr:<subimg_uname>
>   loads  [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]
>          [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]
>          Loads the secure bistreams(authenticated/encrypted/both

this is not the full look. Full log looks like this.
You can see that some of that lines are longer then 80 that's why they are on 
the next line. I think you should stay inside 80 chars per line.
Second loadp is in front of loadb which looks weird.

There should be description about meaning of [] or <> in help message

[ ] should means optional parameter
< > should means required parameter

Below all of them are optional which is not truth. It is correct in fpga info 
but not in fpga dump. It means I think when you are on this descriptions should 
be also fixed.

Also line
fpga [operation type] [device number] [image address] [image size]
is not covering all commands. It means you should consider if make sense to have 
this line.


Usage:
fpga [operation type] [device number] [image address] [image size]
fpga operations:
  dump	[dev] [address] [size]	Load device to memory buffer
  info	[dev]			List known device information
  load	[dev] [address] [size]	Load device from memory buffer
  loadp	[dev] [address] [size]	Load device from memory buffer with partial bitstream
  loadb	[dev] [address] [size]	Load device from bitstream buffer (Xilinx only)
  loadbp	[dev] [address] [size]	Load device from bitstream buffer with partial 
bitstream (Xilinx only)
  loadfs	[dev] [address] [size] [blocksize] <interface> [<dev[:part]>] <filename>
	Load device from filesystem (FAT by default) (Xilinx only)
  loadmk	[dev] [address]		Load device generated with mkimage
	NOTE: loadmk operating on FIT format uImage address must include
	subimage unit name in the form of addr:<subimg_uname>
  loads	[dev] [address] [size] [authflag] [encflag] [Userkey address]
	Load device from memory buffer with secure bistream 
(authenticated/encrypted/both)(Xilinx only)
	 -authflag: 0 for OCM, 1 for DDR, 2 for no authentication (specifies where to 
perform authentication)
	 -encflag: 0 for device key, 1 for user key, 2 for no encryption
	 -Userkey address: specifies the address where user key is stored when user key 
is selected
	NOTE: the secure bitstream has to be created using Xilinx bootgen tool only.

Thanks,
Michal