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

Cristian Marussi
2024-12-04T11:48:38Z

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@quicinc.com>
> ---
>
> 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@quicinc.com>
> +
> +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.

> +
> +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,
Cristian