Re: [PATCH V5 1/2] firmware: arm_scmi: Add QCOM Generic Vendor Protocol documentation

[Sudeep Holla]

On Thu, Dec 05, 2024 at 04:37:28PM +0530, Sibi Sankar wrote:
> 
> 
> On 12/4/24 17:18, Cristian Marussi wrote:
> > On Fri, Nov 15, 2024 at 06:45:14AM +0530, Sibi Sankar wrote:
> > > Add QCOM System Control Management Interface (SCMI) Generic Vendor
> > > Extensions Protocol documentation.
> > > 
> > > Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> > > ---
> > > 
> > > v4:
> > > * Update the protol attributes doc with more information. [Cristian]
> > > 
> > >   .../arm_scmi/vendors/qcom/qcom_generic.rst    | 211 ++++++++++++++++++
> > >   1 file changed, 211 insertions(+)
> > >   create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
> > > 
> > > diff --git a/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst b/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
> > > new file mode 100644
> > > index 000000000000..141bc932e30f
> > > --- /dev/null
> > > +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
> > > @@ -0,0 +1,211 @@
> > > +.. SPDX-License-Identifier: GPL-2.0
> > > +.. include:: <isonum.txt>
> > > +
> > > +===============================================================================
> > > +QCOM System Control and Management Interface(SCMI) Vendor Protocols Extension
> > > +===============================================================================
> > > +
> > > +:Copyright: |copy| 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> > > +
> > > +:Author: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> > > +
> > > +SCMI_GENERIC: System Control and Management Interface QCOM Generic Vendor Protocol
> > > +==================================================================================
> > > +
> > > +This protocol is intended as a generic way of exposing a number of Qualcomm
> > > +SoC specific features through a mixture of pre-determined algorithm string and
> > > +param_id pairs hosted on the SCMI controller. It implements an interface compliant
> > > +with the Arm SCMI Specification with additional vendor specific commands as
> > > +detailed below.
> > > +
> > > +Commands:
> > > +_________
> > > +
> > > +PROTOCOL_VERSION
> > > +~~~~~~~~~~~~~~~~
> > > +
> > > +message_id: 0x0
> > > +protocol_id: 0x80
> > > +
> > > ++---------------+--------------------------------------------------------------+
> > > +|Return values                                                                 |
> > > ++---------------+--------------------------------------------------------------+
> > > +|Name           |Description                                                   |
> > > ++---------------+--------------------------------------------------------------+
> > > +|int32 status   |See ARM SCMI Specification for status code definitions.       |
> > > ++---------------+--------------------------------------------------------------+
> > > +|uint32 version |For this revision of the specification, this value must be    |
> > > +|               |0x10000.                                                      |
> > > ++---------------+--------------------------------------------------------------+
> > > +
> > > +PROTOCOL_ATTRIBUTES
> > > +~~~~~~~~~~~~~~~~~~~
> > > +
> > > +message_id: 0x1
> > > +protocol_id: 0x80
> > > +
> > > ++---------------+--------------------------------------------------------------+
> > > +|Return values                                                                 |
> > > ++------------------+-----------------------------------------------------------+
> > > +|Name              |Description                                                |
> > > ++------------------+-----------------------------------------------------------+
> > > +|int32 status      |See ARM SCMI Specification for status code definitions.    |
> > > ++------------------+-----------------------------------------------------------+
> > > +|uint32 attributes |Bits[31:16] Reserved, must be to 0.                        |
> > > +|                  |Bits[15:8] Number of agents in the system                  |
> > > +|                  |Bits[7:0] Number of vendor protocols in the system         |
> > > ++------------------+-----------------------------------------------------------+
> > 
> > Thanks of clarifing this....may I ask why number of agents is reported
> > here too given that it is already exposed by Base protocol ?
> > 
> > Not really arguing about this so much, but you will end up having to maintain this
> > on 2 different protocols fw side...or are they not 'agents' in the SCMI meaning ?
> > 
> > Anyway, I'm fine with it, even though you dont seem to use this
> > anywhere.
> 
> We don't use it anywhere and it looks like it was just put together
> so that this protocol is compliant to the spec :|

Interesting, I need to dig and check if that is needed to be compliant.
Even if it is required, please add a text to say it must match what std.
BASE protocol response(IOW it just can't be random here as you don't use
it anymore). We may decide to match and warn if required at all.

-- 
Regards,
Sudeep