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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250402-acoustic-analytic-guan-d3cda5@sudeepholla>
Date: Wed, 2 Apr 2025 12:46:14 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	linux-kernel@...r.kernel.org, arm-scmi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev,
	devicetree@...r.kernel.org, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v3 1/7] firmware: arm_scmi: imx: Add LMM and CPU
 documentation

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@....com>
> >> 
> >> Add i.MX95 Logical Machine Management and CPU Protocol documentation.
> >> 
> >> Signed-off-by: Peng Fan <peng.fan@....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.


[...]

> >> +|                     |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.

> >> +
> >> +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 ?

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

-- 
Regards,
Sudeep