Re: [PATCH v3 1/7] firmware: arm_scmi: imx: Add LMM and CPU documentation

Cristian Marussi
2025-04-01T14:52:17Z

On Mon, Mar 03, 2025 at 10:53:22AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Add i.MX95 Logical Machine Management and CPU Protocol documentation.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 801 ++++++++++++++++++++++++
> 1 file changed, 801 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> index b2dfd6c46ca2f5f12f0475c24cb54c060e9fa421..74326bf2ea8586282a735713e0ab7eb90ccce8ff 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst
> @@ -32,6 +32,501 @@ port, and deploy the SM on supported processors.
> The SM implements an interface compliant with the Arm SCMI Specification
> with additional vendor specific extensions.
>
> +SCMI_LMM: System Control and Management Logical Machine Management Vendor Protocol
> +==================================================================================
> +
> +This protocol is intended for boot, shutdown, and reset of other logical
> +machines (LM). It is usually used to allow one LM(e.g. OSPM) to manage
> +another LM which is usually an offload or accelerator engine.. Notifications
> +from this protocol can also be used to manage a communication link to another
> +LM. The LMM protocol provides functions to:
> +
> +- Describe the protocol version.
> +- Discover implementation attributes.
> +- Discover the LMs defined in the system.
> +- Boot an LM.
> +- Shutdown an LM (gracefully or forcibly).
> +- Reset an LM (gracefully or forcibly).
> +- Wake an LM from suspend.
> +- Suspend an LM (gracefully).
> +- Read boot/shutdown/reset information for an LM.
> +- Get notifications when an LM boots or shuts down (e.g. LM[X] requested
> + notification of LM[Y] boots or shuts down, when LM[Y] boots or shuts down,
> + SCMI firmware will send notification to LM[X]).
> +
> +'Graceful' means asking LM itself to shutdown/reset/etc (e.g. sending
> +notification to Linux, Then Linux reboots or powers down itself). It is async
> +command that the SUCCESS of the command just means the command successfully
> +return, not means reboot/reset successfully finished.
> +'Forceful' means the SM will force shutdown/reset/etc the LM. It is sync
> +command that the SUCCESS of the command means the LM has been successfully
> +shutdown/reset/etc.
> +If the commands not have Graceful/Forceful flag settings, such as WAKE, SUSEND,
> +it is async command.
> +

Hi,

> +Commands:
> +_________
> +
> +PROTOCOL_VERSION
> +~~~~~~~~~~~~~~~~
> +
> +message_id: 0x0
> +protocol_id: 0x80
> +This command is mandatory.
> +

Good that you added the mandatory/optional description..

> ++---------------+--------------------------------------------------------------+
> +|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
> +This command is mandatory.
> +
> ++------------------+-----------------------------------------------------------+
> +|Return values |
> ++------------------+-----------------------------------------------------------+
> +|Name |Description |
> ++------------------+-----------------------------------------------------------+
> +|int32 status | See ARM SCMI Specification for status code definitions. |
> ++------------------+-----------------------------------------------------------+
> +|uint32 attributes |Protocol attributes: |
> +| |Bits[31:8] Reserved, must be zero. |
> +| |Bits[7:0] Number of Logical Machines |
> ++------------------+-----------------------------------------------------------+
> +
> +PROTOCOL_MESSAGE_ATTRIBUTES
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +message_id: 0x2
> +protocol_id: 0x80
> +This command is mandatory.
> +
> ++------------------+-----------------------------------------------------------+
> +|Return values |
> ++------------------+-----------------------------------------------------------+
> +|Name |Description |
> ++------------------+-----------------------------------------------------------+
> +|int32 status |SUCCESS: in case the message is implemented and available |
> +| |to use. |
> +| |NOT_FOUND: if the message identified by message_id is |
> +| |invalid or not implemented |
> ++------------------+-----------------------------------------------------------+
> +|uint32 attributes |Flags that are associated with a specific function in the |
> +| |protocol. For all functions in this protocol, this |
> +| |parameter has a value of 0 |
> ++------------------+-----------------------------------------------------------+
> +
> +LMM_ATTRIBUTES
> +~~~~~~~~~~~~~~
> +
> +message_id: 0x3
> +protocol_id: 0x80
> +This command is mandatory.
> +
> ++------------------+-----------------------------------------------------------+
> +|Parameters |
> ++------------------+-----------------------------------------------------------+
> +|Name |Description |
> ++------------------+-----------------------------------------------------------+
> +|uint32 lmid |ID of the Logical Machine |
> ++------------------+-----------------------------------------------------------+
> +|Return values |
> ++------------------+-----------------------------------------------------------+
> +|Name |Description |
> ++------------------+-----------------------------------------------------------+
> +|int32 status |SUCCESS: if valid attributes are returned. |
> +| |NOT_FOUND: if lmId not points to a valid logical machine. |
> +| |DENIED: if the agent does not have permission to get info |
> +| |for the LM specified by lmid. |

..mmmmm... regardig this next field...

> ++------------------+-----------------------------------------------------------+
> +|uint32 lmid |Identifier of the LM whose identification is requested. |
> +| |This field is: Populated with the lmId of the calling |
> +| |agent, when the lmId parameter passed via the function is |
> +| |0xFFFFFFFF. Identical to the lmId field passed via the |
> +| |calling parameters, in all other cases |

In V2 there was an issue with the description and you told me

>> ++------------------+-----------------------------------------------------------+
> >> +|Parameters |
> >> ++------------------+-----------------------------------------------------------+
> >> +|Name |Description |
> >> ++------------------+-----------------------------------------------------------+
> >> +|uint32 lmid |ID of the Logical Machine |
> >> ++------------------+-----------------------------------------------------------+
> >> +|Return values |
> >> ++------------------+-----------------------------------------------------------+
> >> +|Name |Description |
> >> ++------------------+-----------------------------------------------------------+
> >> +|int32 status |SUCCESS: if valid attributes are returned. |
> >> +| |NOT_FOUND: if lmId not points to a valid logical machine. |
> >> +| |DENIED: if the agent does not have permission to get info |
> >> +| |for the LM specified by lmid. |
> >> ++------------------+-----------------------------------------------------------+
> >> +|uint32 attributes | Bits[31:8] reserved. |
> >> +| | Bits[7:0] Number of Logical Machines. |
> >
> >...BUT this returns again the number of LMs while asking the attributes
> >of a specific LM ? .... is it a typo or what ? ...if it is just as a
> >sort of placeholder for when you'll have really LM's attributes to show,
> >consider that once this is documented and supported in this version of
> >your vendor protocol it will be needed to be kept and maintained...maybe
> >better just to declare this as zero in this version of the protocol if
> >you dont really have anything for this command in this version...(like
> >many times are defined the attributes fields in PROTOCOL_MESSAGE_ATTRIBUTES
> >above, if you really know you could want/need this command in the
> >future...is it used now ?
>
> My bad. This should be updated with below
> +------------------+-----------------------------------------------------------+
> |uint32 attributes | Bits[31:0] reserved. must be zero |
> +------------------+-----------------------------------------------------------+
> |uint32 state | Current state of the LM |
> +------------------+-----------------------------------------------------------+
> |uint32 errStatus | Last error status recorded |
> +------------------+-----------------------------------------------------------+
> |char name[16] | A NULL terminated ASCII string with the LM name, of up |
> | | to 16 bytes |
> +------------------+-----------------------------------------------------------+

...so it seems to me that the above lmid is a new addition that was not
mentioned ... bad cut&paste ? it does NOT seem to make so much
sense...or I am missing something

Other than this, LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian