[PATCH 09/10] doc: cmd: add documentation for scmi

[Simon Glass]

Hi,

On Tue, 4 Jul 2023 at 03:05, AKASHI Takahiro <takahiro.akashi at linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, Jul 03, 2023 at 02:30:55PM +0100, Simon Glass wrote:
> > Hi ,
> >
> > On Mon, 3 Jul 2023 at 02:19, AKASHI Takahiro <takahiro.akashi at linaro.org> wrote:
> > >
> > > On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > <takahiro.akashi at linaro.org> wrote:
> > > > >
> > > > > This is a help text for scmi command.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > > > > ---
> > > > >  doc/usage/cmd/scmi.rst | 98 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 98 insertions(+)
> > > > >  create mode 100644 doc/usage/cmd/scmi.rst
> > > > >
> > > > > diff --git a/doc/usage/cmd/scmi.rst b/doc/usage/cmd/scmi.rst
> > > > > new file mode 100644
> > > > > index 000000000000..20cdae4b877d
> > > > > --- /dev/null
> > > > > +++ b/doc/usage/cmd/scmi.rst
> > > > > @@ -0,0 +1,98 @@
> > > > > +.. SPDX-License-Identifier: GPL-2.0+:
> > > > > +
> > > > > +scmi command
> > > > > +============
> > > > > +
> > > > > +Synopsis
> > > > > +--------
> > > > > +
> > > > > +::
> > > > > +
> > > > > +    scmi base info
> > > > > +    scmi base perm_dev <agent id> <device id> <flags>
> > > > > +    scmi base perm_proto <agent id> <device id> <command id> <flags>
> > > > > +    scmi base reset <agent id> <flags>
> > > > > +
> > > > > +Description
> > > > > +-----------
> > > > > +
> > > > > +The scmi command is used to access and operate on SCMI server.
> > > > > +
> > > > > +scmi base info
> > > > > +~~~~~~~~~~~~~~
> > > > > +    Show base information about SCMI server and supported protocols
> > > > > +
> > > > > +scmi base perm_dev
> > > > > +~~~~~~~~~~~~~~~~~~
> > > > > +    Allow or deny access permission to the device
> > > > > +
> > > > > +scmi base perm_proto
> > > > > +~~~~~~~~~~~~~~~~~~~~
> > > > > +    Allow or deny access to the protocol on the device
> > > > > +
> > > > > +scmi base reset
> > > > > +~~~~~~~~~~~~~~~
> > > > > +    Reset the existing configurations
> > > > > +
> > > > > +Parameters are used as follows:
> > > > > +
> > > > > +<agent id>
> > > > > +    Agent ID
> > > >
> > > > what is this?
> > >
> > > I thought that the meaning was trivial in SCMI context.
> > > Will change it to "SCMI Agent ID".
> >
> > What is SCMI? Really you should explain and link to information about
> > things you mention in documentation. How else is anyone supposed to
>
> Generally yes, but please note that SCMI and related drivers are already
> there in U-Boot. For instance, see drivers/firmware/scmi/Kconfig.
> I don't think we need any more explanation about what is SCMI everywhere
> the word, "SCMI", appears.
>
> > figure out what you are talking about?
>
> Again, those who don't know about SCMI and if SCMI server is installed
> on their systems will never use this command.
>
> >
> > "SCMI Agent ID" doesn't tell us much. What form does it take? Is it a
> > string? How do you find it out?
>
> While I still believe that What SCMI agent ID means is trivial for
> those who read the SCMI specification, I will give a short description
> about possible values.
>
> > >
> > >
> > > > > +
> > > > > +<device id>
> > > > > +    Device ID
> > > >
> > > > what is this?
> > >
> > > Again, will change it to "SCMI Device ID".
> >
> > That doesn't help much. The purpose of documentation is to explain
> > things for people who don't already know it. We need to try to be as
> > helpful as possible.
>
> The same above.
> Please note that the definition of "device (ID)", except its data type,
> is out of scope of SCMI specification.
> It doesn't describe any means by which values be identified/retrieved.
>
> > >
> > > > > +
> > > > > +<command id>
> > > > > +    Protocol ID, should not be 0x10 (base protocol)
> > > >
> > > > what is this? Please add more detail
> > >
> > > Again, will change it to "SCMI Protocol ID".
> > > I think that users should be familiar with those terms
> > > if they want to use these interfaces.
> >
> > Maybe, but it still isn't clear what it is. A string?
>
> Will add a short description about its data type/format.
>
> > It is confusing
> > that the command ID is also a protocol ID.
>
> Yes, I know, but the confusion exists in SCMI specification.
> It sometimes seems to use both words almost interchangeably, even
> in a single context. For instance, the section 4.2.2.11 of [1]
> says,
>
> 4.2.2.11 BASE_SET_PROTOCOL_PERMISSIONS
>  ...
>  This command BASE_SET_PROTOCOL_PERMISSIONS is used to ...
>  (This "command" has a yet another meaning.)
>
> Parameters
> uint32_t command_id     Bits[31:8] Reserved, must be zero
>                         Bits[7:0] Protocol ID
>
> [1]  https://developer.arm.com/documentation/den0056/e/?lang=en
>
> So using "command_id" for a parameter name and "Protocol ID"
> as a (part of) value is very much aligned with the specification.
>
> That said, I will change "command_id" to "protocol_id" for now.
>
> > >
> > > > > +
> > > > > +<flags>
> > > > > +    Flags to control the action. See SCMI specification for
> > > > > +    defined values.
> > > >
> > > > ?
> > > >
> > > > Please add the flags here, or at the very least provide a URL and page
> > > > number, etc.
> > >
> > > I intentionally avoid providing details here because a set of flags
> > > acceptable to a specific SCMI server may depend on the server
> > > and its implementation version.
> > > The interface on U-Boot is just a wrapper to make a call to SCMI server
> > > via a transport layer and doesn't care what the parameters means.
> > > That said, I agree to referring to a URL to SCMI specification somewhere
> > > in this document.
> >
> > That will help. But also please add that this is a hex value (right?)
>
> Will do.

OK, well do what you can. Documentation which assumes a lot of new
knowledge is not very useful. Linking to docs is good but it still
helps to spell out acronyms at least once in each place, and add a
little summary about what SCMI is for. Linking to docs can be a pain
when the links disappear, also.

Regards,
Simon