On 10/22/24 15:52, Cristian Marussi wrote:
On Mon, Oct 07, 2024 at 11:40:20AM +0530, Sibi Sankar wrote:
Add QCOM System Control Management Interface (SCMI) Generic Vendor
Extensions Protocol documentation.
Hi Sibi,
a few remarks down below.
Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
---
.../arm_scmi/vendors/qcom/qcom_generic.rst | 210 ++++++++++++++++++
1 file changed, 210 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..1ee6dabaac23
--- /dev/null
+++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom_generic.rst
@@ -0,0 +1,210 @@
+.. 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[8] Set to 1. |
+| |Bits[0] Set to 1. |
++------------------+-----------------------------------------------------------+
Mmmm, this does not explain so much what are those bits and what values
they can indeed assume :P ...
lol, after a lot of rooting around figured out they
return the number of vendor protocols available in
the system :/
Not really sure if it's adding any useful info.
+
+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. |
++------------------+-----------------------------------------------------------+
And what abot the size of this payload ? .. so you are relying on the
fact that the transport will add the total message length at that layer
and so the server can determine where the valid payload ends...
...it means the server will have some expectations about the payload length
based on the param_id and will check against the received transport-advertised
message-length, am I right ?
...BUT what if you end up with multiple versions of this protocol in the future,
with varying payload lengths for the same param_id...REMEMEBER that the server
cannot know which version of a protocol the client is running (while the client
can see what the server runs) UNLESS you implement also NEGOTIATE_PROTOCOL_VERSION
for this protocol...
...so without an explicit length nor the NEGOTIATE_PROTOCOL_VERSION you wont be
able in the future, server-side, to be sure if you are assumnig the right payload
length for the right version that the client is speaking...so at the end you
wont be able to support multiple versions of the protocol even if the Kernel
can support all of those versions...do you see what I mean ?
I think that would be advisable to implement NEGOTIATE_PROTOCOL_VERSION
if you dont want to carry an explicit size in the message for this payload...
...or am I missing something ?
ack makes sense but we planned to make sure that the sub-strings
used maintain abi for all their messages but like you said having
way to negotiate protocol version will be nice to have. Will make
sure something along the lines get implemented with the next major
version upgrade.
-Sibi
+|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. |
++------------------+-----------------------------------------------------------+
Similarly, no payload length means you will have to code some builtin
check to verify the length of the message that you have received against
the specific version that the server is running...this is NOT so
problematic here as in the _SET above since the client/agent DOES know which
protocol version the server is running...
...it is a bit odd, but indeed similar to other variable sized SCMI messages in
standard protocols that sports optional fields in the reply, for which, similarly
you have to check the version of the protocol to desume the size of the message
based on the presence or not of some fields...
+
+QCOM_SCMI_START_ACTIVITY
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+message_id: 0x12
+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 generally used to start the activity performed by |
+| |the algorithm string. |
++------------------+-----------------------------------------------------------+
+|uint32 buf[] |Serves as the payload for the specified param_id and |
+| |algorithm string pair. |
++------------------+-----------------------------------------------------------+
+|Return values |
++------------------+-----------------------------------------------------------+
+|Name |Description |
++------------------+-----------------------------------------------------------+
+|int32 status |SUCCESS: if the activity performed by the algorithm string |
+| |starts successfully. |
+| |NOT_SUPPORTED: if the algorithm string does not have any. |
+| |matches or if the activity is already running. |
++------------------+-----------------------------------------------------------+
+
Same consideration as above...being a SET-like operation with a variable
sized field in the request AND no explicit payload length, you will have
to derive the size from the message length BUT since you doint even have
implemented NEGOTIATE_PROTOCOL_VERSION in the future any kind of check
will become impossibe server side if you will have multiple protocols
with varying sizes for buf depending on the protocol version
+QCOM_SCMI_STOP_ACTIVITY
+~~~~~~~~~~~~~~~~~~~~~~~
+
+message_id: 0x13
+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 generally used to stop the activity performed by |
+| |the algorithm string. |
++------------------+-----------------------------------------------------------+
+|uint32 buf[] |Serves as the payload for the specified param_id and |
+| |algorithm string pair. |
++------------------+-----------------------------------------------------------+
Same.
+|Return values |
++------------------+-----------------------------------------------------------+
+|Name |Description |
++------------------+-----------------------------------------------------------+
+|int32 status |SUCCESS: if the activity performed by the algorithm string |
+| |stops successfully. |
+| |NOT_SUPPORTED: if the algorithm string does not have any |
+| |matches or if the activity isn't running. |
++------------------+-----------------------------------------------------------+
Thanks,
Cristian
