Re: [PATCH 05/15] doc: sandbox: Add docs for the sb command

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 10/28/24 13:47, Simon Glass wrote:
> This command has a few small features, so document it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   doc/usage/cmd/sb.rst | 54 ++++++++++++++++++++++++++++++++++++++++++++
>   doc/usage/index.rst  |  1 +
>   2 files changed, 55 insertions(+)
>   create mode 100644 doc/usage/cmd/sb.rst
>
> diff --git a/doc/usage/cmd/sb.rst b/doc/usage/cmd/sb.rst
> new file mode 100644
> index 00000000000..6f54f9d9eb7
> --- /dev/null
> +++ b/doc/usage/cmd/sb.rst
> @@ -0,0 +1,54 @@
> +.. SPDX-License-Identifier: GPL-2.0+

Please, use a valid SPDX identifier. See here

https://spdx.org/licenses/GPL-2.0-or-later.html

> +
> +.. index::
> +   single: sbi (command)

The sbi command is used to show the status of the RISC-V SBI firmware.
You wanted to describe the sb command?

> +
> +sbi command

ditto

> +===========
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    sb handoff
> +    sb state
> +
> +Description
> +-----------
> +
> +The *sb* command is used to display information about sandbox's internal
> +operation. See :doc:`/arch/sandbox/index` for more information.

The command can only show states and not any operation.

%s/operation/operational state/ ?

> +
> +sb handoff
> +~~~~~~~~~~
> +
> +This shows information about any handoff information received from SPL. If
> +U-Boot is started from an SPL build, it shows a valid magic number.
> +
> +sb state
> +~~~~~~~~
> +
> +This shows basic information about the sandbox state, currently just the
> +command-line with which sandbox was started.
> +
> +Example
> +-------
> +
> +This shows checking for the presence of SPL-handoff information. For this to
> +work, ``u-boot-spl`` must be run, with build that enables ``CONFIG_SPL``, such
> +as ``sandbox_spl``::
> +
> +    => sb handoff
> +    SPL handoff magic 14f93c7b

As a user I would have no clue what this magic is used for. This needs
some explanation. Why should I care for some random number?

Is this really sandbox only information?

I would be much more interested what information is passed in general
from SPL.

> +
> +This shows output from the *sb state* subcommand::
> +
> +    => sb state
> +    Arguments:
> +    /tmp/b/sandbox/u-boot -D

Please, remove /tmp/b/sandbox. I would expect the average user to invoke
the sandbox from the build directory:

./u-boot -D

> +
> +Configuration
> +-------------
> +
> +The *sb handoff* command is only supported if CONFIG_HANDOFF is enabled.

What enables the sb command?

Why do we need two sub-commands to show so little information?

It would be much easier to simply type 'sb' and get all the information.

If you there is so little information of interest, why don't you simply
implement arch_print_bdinfo() like you did on x86?

Best regards

Heinrich

> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index db71711c393..24b2d2637b1 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -103,6 +103,7 @@ Shell commands
>      cmd/reset
>      cmd/rng
>      cmd/saves
> +   cmd/sb
>      cmd/sbi
>      cmd/scmi
>      cmd/scp03