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

[Peng Fan <peng.fan@oss.nxp.com>]

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
>