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

Sibi Sankar
2024-12-05T11:07:44Z

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