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

[Sibi Sankar]

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 :|

> > +
> > +PROTOCOL_MESSAGE_ATTRIBUTES
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +message_id: 0x2
> > +protocol_id: 0x80
> > +
> > ++---------------+--------------------------------------------------------------+
> > +|Return values                                                                 |
> > ++------------------+-----------------------------------------------------------+
> > +|Name              |Description                                                |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status      |See ARM SCMI Specification for status code definitions.    |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 attributes |For all message id's the parameter has a value of 0.       |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +QCOM_SCMI_SET_PARAM
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +message_id: 0x10
> > +protocol_id: 0x80
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Parameters                                                                    |
> > ++------------------+-----------------------------------------------------------+
> > +|Name              |Description                                                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 ext_id     |Reserved, must be zero.                                    |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 algo_low   |Lower 32-bit value of the algorithm string.                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 algo_high  |Upper 32-bit value of the algorithm string.                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 param_id   |Serves as the token message id for the algorithm string    |
> > +|                  |and is used to set various parameters supported by it.     |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 buf[]      |Serves as the payload for the specified param_id and       |
> > +|                  |algorithm string pair.                                     |
> > ++------------------+-----------------------------------------------------------+
> > +|Return values                                                                 |
> > ++------------------+-----------------------------------------------------------+
> > +|Name              |Description                                                |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status      |SUCCESS: if the param_id and buf[] is parsed successfully  |
> > +|                  |by the chosen algorithm string.                            |
> > +|                  |NOT_SUPPORTED: if the algorithm string does not have any   |
> > +|                  |matches.                                                   |
> > +|                  |INVALID_PARAMETERS: if the param_id and the buf[] passed   |
> > +|                  |is rejected by the algorithm string.                       |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +QCOM_SCMI_GET_PARAM
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +message_id: 0x11
> > +protocol_id: 0x80
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Parameters                                                                    |
> > ++------------------+-----------------------------------------------------------+
> > +|Name              |Description                                                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 ext_id     |Reserved, must be zero.                                    |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 algo_low   |Lower 32-bit value of the algorithm string.                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 algo_high  |Upper 32-bit value of the algorithm string.                |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 param_id   |Serves as the token message id for the algorithm string.   |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 buf[]      |Serves as the payload and store of value for the specified |
> > +|                  |param_id and algorithm string pair.                        |
> > ++------------------+-----------------------------------------------------------+
> > +|Return values                                                                 |
> > ++------------------+-----------------------------------------------------------+
> > +|Name              |Description                                                |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status      |SUCCESS: if the param_id and buf[] is parsed successfully  |
> > +|                  |by the chosen algorithm string and the result is copied    |
> > +|                  |into buf[].                                                |
> > +|                  |NOT_SUPPORTED: if the algorithm string does not have any   |
> > +|                  |matches.                                                   |
> > +|                  |INVALID_PARAMETERS: if the param_id and the buf[] passed   |
> > +|                  |is rejected by the algorithm string.                       |
> > ++------------------+-----------------------------------------------------------+
>
> ..missed this last time...so you should add here also a field describing
> the reply buf right ? (as it happenns really in your code) something
> like:
>
>   ++------------------+-----------------------------------------------------------+
>   +|uint32 buf[]      |Holds the payload of the result of the query.	 	 |
>   +|                  |					                         |
>   ++------------------+-----------------------------------------------------------+

Thanks will get this added in the next re-spin.

-Sibi

> Thanks,
> Cristian