[PATCH v4 4/5] sf: doc: Add documentation for the 'sf' command

[Simon Glass]

Hi Heinrich,

On Sat, 13 Nov 2021 at 04:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 9/19/21 23:49, Simon Glass wrote:
> > This command is fairly complicated so documentation is useful.
> > Unfortunately I an not sure how the MTD side of things works and cannot
> > find information about that.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> >
> > Acked-by: Pratyush Yadav <p.yadav at ti.com>
> > ---
> >
> > Changes in v4:
> > - Split out the 'const' change into a separate patch
> > - Show the 'sf probe' output in the examples
> >
> > Changes in v2:
> > - Many fixes as suggested by Heinrich
> >
> >   doc/usage/index.rst |   1 +
> >   doc/usage/sf.rst    | 245 ++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 246 insertions(+)
> >   create mode 100644 doc/usage/sf.rst
> >
> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> > index 356f2a56181..9a7b12b7c54 100644
> > --- a/doc/usage/index.rst
> > +++ b/doc/usage/index.rst
> > @@ -43,6 +43,7 @@ Shell commands
> >      qfw
> >      reset
> >      sbi
> > +   sf
>
> Please, keep this list in alphabetical order.
>
> >      scp03
> >      setexpr
> >      size
> > diff --git a/doc/usage/sf.rst b/doc/usage/sf.rst
> > new file mode 100644
> > index 00000000000..71bd1be5175
> > --- /dev/null
> > +++ b/doc/usage/sf.rst
> > @@ -0,0 +1,245 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +sf command
> > +==========
> > +
> > +Synopis
> > +-------
> > +
> > +::
> > +
> > +    sf probe [[[<bus>:]<cs>] [<hz> [<mode>]]]
> > +    sf read <addr> <offset>|<partition> <len>
> > +    sf write <addr> <offset>|<partition> <len>
> > +    sf erase <offset>|<partition> <len>
> > +    sf update <addr> <offset>|<partition> <len>
> > +    sf protect lock|unlock <sector> <len>
> > +    sf test <offset>|<partition> <len>
> > +
> > +Description
> > +-----------
> > +
> > +The *sf* command is used to access SPI flash, supporting read/write/erase and
> > +a few other functions.
> > +
> > +Probe
> > +-----
> > +
> > +The flash must first be probed with *sf probe* before any of the other
> > +subcommands can be used. All of the parameters are optional:
> > +
> > +bus
> > +     SPI bus number containing the SPI-flash chip, e.g. 0. If you don't know
> > +     the number, you can use 'dm uclass' to see all the spi devices,
> > +     and check the value for 'seq' for each one (here 0 and 2)::
>
> I would have expected the 'spi' command to have an info sub-command like
> the other subsystems. But that is missing.
>
> > +
> > +        uclass 89: spi
> > +        0     spi at 0 @ 05484960, seq 0
> > +        1     spi at 1 @ 05484b40, seq 2
> > +
> > +cs
> > +     SPI chip-select to use for the chip. This is often 0 and can be omitted,
> > +     but in some cases multiple slaves are attached to a SPI controller,
> > +     selected by a chip-select line for each one.
> > +
> > +hz
> > +     Speed of the SPI bus in hertz. This normally defaults to 100000, i.e.
> > +     100KHz, which is very slow. Note that if the device exists in the
> > +     device tree, there might be a speed provided there, in which case this
> > +     setting is ignored.
> > +
> > +mode
> > +     SPI mode to use:
> > +
> > +     =====  ================
> > +     Mode   Meaning
> > +     =====  ================
> > +     0      CPOL=0, CPHA=0
> > +     1      CPOL=0, CPHA=1
> > +     2      CPOL=1, CPHA=0
> > +     3      CPOL=1, CPHA=1
> > +     =====  ================
> > +
> > +     Clock phase (CPHA) 0 means that data is transferred (sampled) on the
> > +     first clock edge; 1 means the second.
> > +
> > +     Clock polarity (CPOL) controls the idle state of the clock, 0 for low,
> > +     1 for high.
> > +     The active state is the opposite of idle.
> > +
> > +     You may find this `SPI documentation`_ useful.
> > +
> > +Parameters for other subcommands (described below) are as follows:
>
> I would not expect parameters for other sub-commands in chapter "Probe".
>
> Please put all parameters into a separate section "Parameters". This
> makes navigation easier.

This series was sent back in April and is now at version 5, after
multiple rounds of changes. This version alone sat here for nearly two
months. Who will want to write documentation in U-Boot if this is the
process?

I don't disagree with most of your comments, just the timing, although
the 'spi info' thing is highly debatable, as you cannot memory-map SPI
itself, only SPI flash.

The common.h header removal suffered a similar fate and we have never
resolved that, now 18 months later.

So please, let's get something in and move forward.

Regards,
Simon