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

Peng Fan
2025-04-02T15:02:25Z

Hi Sudeep,

On Wed, Apr 02, 2025 at 12:46:14PM +0100, Sudeep Holla wrote:
>On Wed, Apr 02, 2025 at 08:35:03PM +0800, Peng Fan wrote:
>> Hi Sudeep,
>>
>> Thanks for reviewing the patch.
>>
>> For comments that I am not very clear, I marked with [TODO] for easily
>> jump to.
>>
>> On Tue, Apr 01, 2025 at 03:15:46PM +0100, Sudeep Holla wrote:
>> >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
>
>> >> +
>> >> +PROTOCOL_MESSAGE_ATTRIBUTES
>> >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >> +
>> >> +message_id: 0x2
>> >> +protocol_id: 0x80
>> >> +This command is mandatory.
>> >> +
>> >
>> >For completeness add parameters here for message_id as in the spec as it is
>> >referred in the returned value and seems incomplete without it.
>>
>> [TODO]
>> Sorry, I may not get your point here. You mean below format?
>>
>> +------------------+-----------------------------------------------------------+
>> |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 |
>>
>> message_id is not put in the table, but it is list above just below
>> the protocol name. I would prefer to keep current layout and align with
>> the MISC and BBM protocol.
>>
>
>I meant why is the input parameter message_id not described in the table,
>but is referred in the return values. For completeness, just add it even
>though it may match the SCMI spec in terms of input parameter.

I will add below only for PROTOCOL_MESSAGE_ATTRIBUTES which refer message_id
in the return values. Please raise if you have concern.

+------------------+-----------------------------------------------------------+
|Parameters |
+------------------+-----------------------------------------------------------+
|Name |Description |
+------------------+-----------------------------------------------------------+
|uint32 message_id |ID of the message, const: 0x2 |
+------------------+-----------------------------------------------------------+
|Return values |

...

>
>
>[...]
>
>> >> +| |Bit[23] Valid err ID: |
>> >> +| |Set to 1 if the error ID field is valid. |
>> >> +| |Set to 0 if the error ID field is not valid. |
>> >> +| |Bits[22:8] Error ID(Agent ID of the system). |
>> >> +| |Bit[7:0] Reason(WDOG, POR, FCCU and etc) |
>> >
>> >Is there a mapping for this ?
>>
>> I will add a note in V4:
>> See the SRESR register description in the System Reset Controller (SRC) section
>> in SoC reference mannual.
>>
>
>A reference would be good here then. I would be hard to imagine what it means
>otherwise.

Understood.

>
>> >> +
>> >> +LMM_RESET_VECTOR_SET
>> >> +~~~~~~~~~~~~~~~~~~~~
>> >> +
>> >> +message_id: 0xC
>> >> +protocol_id: 0x80
>> >> +This command is mandatory.
>> >> +
>> >
>> >I can't recall if I had asked this before. How is this different from
>> >CPU_RESET_VECTOR_SET ? Why do you need this ? Why can't you use
>> >CPU_RESET_VECTOR_SET with an additional LMM_* command.
>> >
>> >I am sure there is a valid reason. If so please document the same.
>>
>> CPU_RESET_VECTOR_SET is for cases that M7 and A55 in the same LM.
>> LMM_RESET_VECTOR_SET is for cases that M7 and A55 in different LM.
>> M7 LM is under control of A55 LM
>>
>
>That still doesn't answer my question. I was asking why do you need this
>extra interface ? If LMM_RESET_VECTOR_SET can take both cpu id and LM id,
>it can be used even for cpus within same LM with current LM ID. Why the
>need for separate interface ?

For this one, I need check with firmware owner. loop you in
internal mail.

Thanks,
Peng

>
>Other than these 2, I am fine with your response on all other comments.
>
>--
>Regards,
>Sudeep
>