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

[Sibi Sankar <quic_sibis@quicinc.com>]

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@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.

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