| Message ID | 20250212-imx-lmm-cpu-v2-0-3aee005968c1@nxp.com |
|---|---|
| Headers | show |
| Series | firmware: scmi/imx: Add i.MX95 LMM/CPU Protocol | expand |
On Wed, Feb 12, 2025 at 03:40:23PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Add i.MX95 Logical Machine Management and CPU Protocol documentation. > Hi, a few comments below. > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 737 ++++++++++++++++++++++++ > 1 file changed, 737 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > index b2dfd6c46ca2f5f12f0475c24cb54c060e9fa421..78a09cd8102becd5584d28bdef18df2d77fb7e7c 100644 > --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > @@ -32,6 +32,461 @@ port, and deploy the SM on supported processors. > The SM implements an interface compliant with the Arm SCMI Specification > with additional vendor specific extensions. > In general I noticed that there is NOT specified anywhere if all the following commands are mandatory or optional in your protocol: would be better to be specified, as it is I will assume that are all mandatory. > +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). > + > +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 |Protocol attributes: | > +| |Bits[31:8] Reserved, must be zero. | > +| |Bits[7:0] Number of Logical Machines | ...so this states how many LMs you have...ok... > ++------------------+-----------------------------------------------------------+ > + > +PROTOCOL_MESSAGE_ATTRIBUTES > +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +message_id: 0x2 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|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 > + > ++------------------+-----------------------------------------------------------+ > +|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 ? > ++------------------+-----------------------------------------------------------+ > + > +LMM_BOOT > +~~~~~~~~ > + > +message_id: 0x4 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully booted. | ...this and other commands below that cause a transition are a bit ambiguos in their description IMHO...does this mean that on reception of a SUCCESS the LM identified by lmid has successfully completed the boot ? ...or, as I suppose, this being an immmediate command, SUCCESS means something like 'boot succesfully started', NOT that when this SUCCESS reply comes back the LM has booted...also becuse I can see a lot of notifications defined down below to signal the completion of such operations... ..if this is the case please clarify... ...if this is NOT the case and this is intended to return synchronoulsy when the required operation has completed, even though such machines are maybe (?) minimal/tiny compute systems, can you guarantee that they can boot/shutdown etc in a reasonably short time not to hog the channel for too long (I mean well before the configured transport timeout)... > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_RESET > +~~~~~~~~~ > + > +message_id: 0x5 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|uint32 flags |Reset flags: | > +| |Bits[31:1] Reserved, must be zero. | > +| |Bit[0] Graceful request: | > +| |Set to 1 if the request is a graceful request. | > +| |Set to 0 if the request is a forceful request. | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully resets. | Same ... does this simply mean LM reset issued successfully ? > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_SHUTDOWN > +~~~~~~~~~~~~ > + > +message_id: 0x6 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|uint32 flags |Reset flags: | > +| |Bits[31:1] Reserved, must be zero. | > +| |Bit[0] Graceful request: | > +| |Set to 1 if the request is a graceful request. | > +| |Set to 0 if the request is a forceful request. | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully shutdowns. | Same...I suppose the shutdown has been sucessfully issued and could be still in progress...if this is not the case I fear this immediate command will potentially hog the channel for too long.... > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_WAKE > +~~~~~~~~ > + > +message_id: 0x7 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully wakes. | Same...I suppose you can know when an LM is fully woken only from the dedicated notification receipt... > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_SUSPEND > +~~~~~~~~~~~ > + > +message_id: 0x8 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully suspends. | Same..I suppose the SCMI server will know when the suspend for this LM is effective and will send a notification, so this is again only an indication that the suspend process has started... > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + In general, you could also think to make the bahvour of all this operation configurable like teh RESET protocol does...so you could ask for an LMM op to complete synchronously OR instead let it happen asynchnously in the background after the command-request has completed and then wait for one of the existing notification... ...again if this is already the expected behaviour please clarifiy the sync/async kind of execution that is expected when you issue such commands > +LMM_NOTIFY > +~~~~~~~~~~ > + > +message_id: 0x9 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|uint32 flags |Notification flags: | > +| |Bits[31:3] Reserved, must be zero. | > +| |Bit[3] Wake (resume) notification: | > +| |Set to 1 to send notification. | > +| |Set to 0 if no notification. | > +| |Bit[2] Suspend (sleep) notification: | > +| |Set to 1 to send notification. | > +| |Set to 0 if no notification. | > +| |Bit[1] Shutdown (off) notification: | > +| |Set to 1 to send notification. | > +| |Set to 0 if no notification. | > +| |Bit[0] Boot (on) notification: | > +| |Set to 1 to send notification. | > +| |Set to 0 if no notification | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if the notification state successfully updated. | > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if input attributes flag specifies | > +| |unsupported or invalid configurations. | > +| |DENIED: if the agent does not have permission to request | > +| |the notification. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_RESET_REASON > +~~~~~~~~~~~~~~~~ > + > +message_id: 0xA > +protocol_id: 0x80 What is this...a bit of explanation would be fine given that this command is not straightforward to understand like the previous boot/suspend etc... > + > ++---------------------+--------------------------------------------------------+ > +|Parameters | > ++---------------------+--------------------------------------------------------+ > +|Name |Description | > ++---------------------+--------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++---------------------+--------------------------------------------------------+ > +|Return values | > ++---------------------+--------------------------------------------------------+ > +|Name |Description | > ++---------------------+--------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully suspends. | ? typo ? > +| |NOT_FOUND: if lmId not points to a valid logical machine| > +| |DENIED: if the agent does not have permission to request| > +| |the reset reason. | > ++---------------------+--------------------------------------------------------+ > +|uint32 bootflags |Boot reason flags. This parameter has the format: | > +| |Bits[31] Valid. | > +| |Set to 1 if the entire reason is valid. | > +| |Set to 0 if the entire reason is not valid. | > +| |Bits[30:29] Reserved, must be zero. | > +| |Bit[28] Valid origin: | > +| |Set to 1 if the origin field is valid. | > +| |Set to 0 if the origin field is not valid. | > +| |Bits[27:24] Origin. | Are this origin values defined anywhere ? > +| |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. | And this errors ID ? which value should I expect here...and to what this errors refer to... > +| |Bits[22:8] Error ID. | > +| |Bit[7:0] Reason(WDOG, POR, FCCU and etc) | > ++---------------------+--------------------------------------------------------+ > +|uint32 shutdownflags |Shutdown reason flags. This parameter has the format: | > +| |Bits[31] Valid. | > +| |Set to 1 if the entire reason is valid. | > +| |Set to 0 if the entire reason is not valid. | > +| |Bits[30:29] Number of valid extended info words. | > +| |Bit[28] Valid origin: | > +| |Set to 1 if the origin field is valid. | > +| |Set to 0 if the origin field is not valid. | Ditto. > +| |Bits[27:24] Origin. | > +| |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. | Ditto. > +| |Bit[7:0] Reason | Ditto. > ++---------------------+--------------------------------------------------------+ > +|uint32 extinfo[0,20] |Array of extended info words(e.g. fault pc) | > ++---------------------+--------------------------------------------------------+ ..so what does this field represent....is this a fixed sized array ? seems more likely to be dynamically sized in range [0,20] AFAICU... ...now...if it is dynamically sized: + there should be a size field declaring the size of the returned answer, if not you should rely on the transport size to know where the message ends (unless if hard-coded based on the above flags...in that case explain and document) + you cannot assume that such a potentially big reply reply fit into a single message due to possible limitations of the transport sizes...IOW you should think about a 'multi-part' message like many exist in SCMI where the reply reports returned+remaining entries... ..how can yo be sure to fit in any transport...this message has currently a potential max payload reply of 96 bytes ... > + > +LMM_POWER_ON > +~~~~~~~~~~~~ > + > +message_id: 0xB > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully powers on. | Beside the same consideration as above regarding what this SUCCESS mean...what is meant to be the difference between LMM_POWER_ON and LMM_BOOT ? > +| |NOT_FOUND: if lmId not points to a valid logical machine. | > +| |INVALID_PARAMETERS: if lmId is same as the caller. | > +| |DENIED: if the agent does not have permission to manage the| > +| |the LM specified by lmid. | > ++------------------+-----------------------------------------------------------+ > + > +LMM_RESET_VECTOR_SET > +~~~~~~~~~~~~~~~~~~~~ > + > +message_id: 0xC > +protocol_id: 0x80 > + > ++-----------------------+------------------------------------------------------+ > +|Parameters | > ++-----------------------+------------------------------------------------------+ > +|Name |Description | > ++-----------------------+------------------------------------------------------+ > +|uint32 lmid |ID of the Logical Machine | > ++-----------------------+------------------------------------------------------+ > +|uint32 cpuid |ID of the CPU inside the LM | > ++-----------------------+------------------------------------------------------+ > +|uint32 flags |Reset vector flags | > +| |Bits[31:1] Reserved, must be zero. | > +| |Bit[0] Table flag. | > +| |Set to 1 if vector is the vector table base address | So what is this when Bit[0] is set to zero instead ? > ++-----------------------+------------------------------------------------------+ > +|uint32 resetVectorLow |Lower vector | > ++-----------------------+------------------------------------------------------+ > +|uint32 resetVectorHigh |Higher vector | > ++-----------------------+------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if LM successfully powers on. | ...typo ?... > +| |NOT_FOUND: if lmId not points to a valid logical machine, | > +| |or cpuId is not valid. > +| |INVALID_PARAMETERS: if reset vector is invalid. | > +| |DENIED: if the agent does not have permission to set the | > +| |the reset vector for the CPU in the LM. | > ++------------------+-----------------------------------------------------------+ > + > +NEGOTIATE_PROTOCOL_VERSION > +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +message_id: 0x10 > +protocol_id: 0x80 > + > ++--------------------+---------------------------------------------------------+ > +|Parameters | > ++--------------------+---------------------------------------------------------+ > +|Name |Description | > ++--------------------+---------------------------------------------------------+ > +|uint32 version |The negotiated protocol version the agent intends to use | > ++--------------------+---------------------------------------------------------+ > +|Return values | > ++--------------------+---------------------------------------------------------+ > +|Name |Description | > ++--------------------+---------------------------------------------------------+ > +|int32 status |SUCCESS: if the negotiated protocol version is supported | > +| |by the platform. All commands, responses, and | > +| |notifications post successful return of this command must| > +| |comply with the negotiated version. | > +| |NOT_SUPPORTED: if the protocol version is not supported. | > ++--------------------+---------------------------------------------------------+ > + > +Notifications > +_____________ > + > +LMM_EVENT > +~~~~~~~~~ > + > +message_id: 0x0 > +protocol_id: 0x80 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 lmid |Identifier for the LM that caused the transition. | > ++------------------+-----------------------------------------------------------+ > +|uint32 eventlm |Identifier for the LM the event is for. | So this is the LM that the event refer to ? i.e. this LM eventlm is the one that is booting/resetting/shutdown ? if this is the case, maybe better as: "Identifier of the LM this event refers to." > ++------------------+-----------------------------------------------------------+ > +|uint32 flags |LM events: | > +| |Bits[31:3] Reserved, must be zero. | > +| |Bit[3] Wake (resume) event: | > +| |1 LM has awakened. | > +| |0 not a wake event. | > +| |Bit[2] Suspend (sleep) event: | > +| |1 LM has suspended. | > +| |0 not a suspend event. | > +| |Bit[1] Shutdown (off) event: | > +| |1 LM has shutdown. | > +| |0 not a shutdown event. | > +| |Bit[0] Boot (on) event: | > +| |1 LM has booted. | > +| |0 not a boot event. | > ++------------------+-----------------------------------------------------------+ > + > SCMI_BBM: System Control and Management BBM Vendor Protocol > ============================================================== > > @@ -436,6 +891,288 @@ protocol_id: 0x81 > | |0 no button change detected. | > +------------------+-----------------------------------------------------------+ > > +SCMI_CPU: System Control and Management CPU Vendor Protocol > +============================================================== > + > +This protocol allows an agent to start or stop a CPU. It is used to manage > +auxiliary CPUs in an LM (e.g. additional cores in an AP cluster or > +Cortex-M cores). > +Note: For cores in AP cluster, ATF will use CPU protocol to handle them. So this is sort of a PSCI-by-proxy in which you ask the server to turn on/off some of the LM CPUs ? First question would be is not enough to just ask for a specific LM to be booted with LMM ? or add to the LMM protocol a way to specifiy the 'level' of boot that you ask for an LM...I may miss many details but it seems to me a bit odd that you are allowd to ask, from the agent that could have just requested an LM to boot to have some CPUs ON or OFF... ...and specify the boot/resume address vectors here to use when turning ON... Maybe a bit more of explanation of the rationale and expectations here could help understanding the differet needs from LMM > + > +The CPU protocol provides functions to: > + > +- Describe the protocol version. > +- Discover implementation attributes. > +- Discover the CPUs defined in the system. > +- Start a CPU. > +- Stop a CPU. > +- Set the boot and resume addresses for a CPU. > +- Set the sleep mode of a CPU. > +- Configure wake-up sources for a CPU. > +- Configure power domain reactions (LPM mode and retention mask) for a CPU. > +- The CPU IDs can be found in the CPU section of the SoC DEVICE: SM Device > + Interface. They can also be found in the SoC RM. See the CPU Mode Control > + (CMC) list in General Power Controller (GPC) section. > + > +CPU settings are not aggregated and setting their state is normally exclusive > +to one client. > + > +Commands: > +_________ > + > +PROTOCOL_VERSION > +~~~~~~~~~~~~~~~~ > + > +message_id: 0x0 > +protocol_id: 0x82 > + > ++---------------+--------------------------------------------------------------+ > +|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: 0x82 > + > ++---------------+--------------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status | See ARM SCMI Specification for status code definitions. | > ++------------------+-----------------------------------------------------------+ > +|uint32 attributes |Protocol attributes: | > +| |Bits[31:16] Reserved, must be zero. | > +| |Bits[15:0] Number of CPUs | > ++------------------+-----------------------------------------------------------+ > + > +PROTOCOL_MESSAGE_ATTRIBUTES > +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +message_id: 0x2 > +protocol_id: 0x82 > + > ++---------------+--------------------------------------------------------------+ > +|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 | > ++------------------+-----------------------------------------------------------+ > + > +CPU_ATTRIBUTES > +~~~~~~~~~~~~~~ > + > +message_id: 0x4 > +protocol_id: 0x82 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if valid attributes are returned successfully. | > +| |NOT_FOUND: if the cpuid is not valid. | > ++------------------+-----------------------------------------------------------+ > +|uint32 attributes |Bits[31:0] Reserved, must be zero | > ++------------------+-----------------------------------------------------------+ > +|char name[16] |NULL terminated ASCII string with CPU name up to 16 bytes | > ++------------------+-----------------------------------------------------------+ > + > +CPU_START > +~~~~~~~~~ > + > +message_id: 0x4 > +protocol_id: 0x82 > + Any constraint to call this only after having successfully called RESET_VECROR_SET ? > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if the cpu is started successfully. | > +| |NOT_FOUND: if cpuid is not valid. | > +| |DENIED: the calling agent is not allowed to start this CPU.| > ++------------------+-----------------------------------------------------------+ > + > +CPU_STOP > +~~~~~~~~ > + > +message_id: 0x5 > +protocol_id: 0x82 > + > ++------------------+-----------------------------------------------------------+ > +|Parameters | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++------------------+-----------------------------------------------------------+ > +|Return values | > ++------------------+-----------------------------------------------------------+ > +|Name |Description | > ++------------------+-----------------------------------------------------------+ > +|int32 status |SUCCESS: if the cpu is started successfully. | > +| |NOT_FOUND: if cpuid is not valid. | > +| |DENIED: the calling agent is not allowed to stop this CPU. | > ++------------------+-----------------------------------------------------------+ > + > +CPU_RESET_VECTOR_SET > +~~~~~~~~~~~~~~~~~~~~ > + > +message_id: 0x6 > +protocol_id: 0x82 > + > ++----------------------+-------------------------------------------------------+ > +|Parameters | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++----------------------+-------------------------------------------------------+ > +|uint32 flags |Reset vector flags: | > +| |Bit[31] Resume flag. | > +| |Set to 1 to update the reset vector used on resume. | > +| |Bit[30] Boot flag. | > +| |Set to 1 to update the reset vector used for boot. | > +| |Bits[29:1] Reserved, must be zero. | > +| |Bit[0] Table flag. | > +| |Set to 1 if vector is the vector table base address. | > ++----------------------+-------------------------------------------------------+ > +|uint32 resetVectorLow |Lower vector: | > +| |If bit[0] of flags is 0, the lower 32 bits of the | > +| |physical address where the CPU should execute from on | > +| |reset. If bit[0] of flags is 1, the lower 32 bits of | > +| |the vector table base address | > ++----------------------+-------------------------------------------------------+ > +|uint32 resetVectorhigh|Upper vector: | > +| |If bit[0] of flags is 0, the upper 32 bits of the | > +| |physical address where the CPU should execute from on | > +| |reset. If bit[0] of flags is 1, the upper 32 bits of | > +| |the vector table base address | > ++----------------------+-------------------------------------------------------+ > +|Return values | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|int32 status |SUCCESS: if the CPU reset vector is set successfully. | > +| |NOT_FOUND: if cpuId does not point to a valid CPU. | > +| |INVALID_PARAMETERS: the requested vector type is not | > +| |supported by this CPU. | > +| |DENIED: the calling agent is not allowed to set the | > +| |reset vector of this CPU | > ++----------------------+-------------------------------------------------------+ > + > +CPU_SLEEP_MODE_SET > +~~~~~~~~~~~~~~~~~~ > +message_id: 0x7 > +protocol_id: 0x82 > + > ++----------------------+-------------------------------------------------------+ > +|Parameters | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++----------------------+-------------------------------------------------------+ > +|uint32 flags |Sleep mode flags: | > +| |Bits[31:1] Reserved, must be zero. | > +| |Bit[0] IRQ mux: | > +| |If set to 1 the wakeup mux source is the GIC, else if 0| > +| |then the GPC | What is the GPC in this context ? > ++----------------------+-------------------------------------------------------+ > +|uint32 sleepmode |target sleep mode | What values can this assume ? Is there any predefined sleep value here ? > ++----------------------+-------------------------------------------------------+ > +|Return values | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|int32 status |SUCCESS: if the CPU sleep mode is set successfully. | > +| |NOT_FOUND: if cpuId does not point to a valid CPU. | > +| |INVALID_PARAMETERS: the sleepmode or flags is invalid. | > +| |DENIED: the calling agent is not allowed to configure | > +| |the CPU | > ++----------------------+-------------------------------------------------------+ > + > +CPU_INFO_GET > +~~~~~~~~~~~~ > +message_id: 0xC > +protocol_id: 0x82 > + > ++----------------------+-------------------------------------------------------+ > +|Parameters | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|uint32 cpuid |Identifier for the CPU | > ++----------------------+-------------------------------------------------------+ > +|Return values | > ++----------------------+-------------------------------------------------------+ > +|Name |Description | > ++----------------------+-------------------------------------------------------+ > +|int32 status |SUCCESS: if valid attributes are returned successfully.| > +| |NOT_FOUND: if the cpuid is not valid. | > ++----------------------+-------------------------------------------------------+ > +|uint32 runmode |Run mode for the CPU | What are the possible runmode values ? > ++----------------------+-------------------------------------------------------+ > +|uint32 sleepmode |Sleep mode for the CPU | > ++----------------------+-------------------------------------------------------+ > +|uint32 resetvectorlow |Reset vector low 32 bits for the CPU | > ++----------------------+-------------------------------------------------------+ > +|uint32 resetvecothigh |Reset vector high 32 bits for the CPU | > ++----------------------+-------------------------------------------------------+ > + > +NEGOTIATE_PROTOCOL_VERSION > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > +message_id: 0x10 > +protocol_id: 0x82 > + > ++--------------------+---------------------------------------------------------+ > +|Parameters | > ++--------------------+---------------------------------------------------------+ > +|Name |Description | > ++--------------------+---------------------------------------------------------+ > +|uint32 version |The negotiated protocol version the agent intends to use | > ++--------------------+---------------------------------------------------------+ > +|Return values | > ++--------------------+---------------------------------------------------------+ > +|Name |Description | > ++--------------------+---------------------------------------------------------+ > +|int32 status |SUCCESS: if the negotiated protocol version is supported | > +| |by the platform. All commands, responses, and | > +| |notifications post successful return of this command must| > +| |comply with the negotiated version. | > +| |NOT_SUPPORTED: if the protocol version is not supported. | > ++--------------------+---------------------------------------------------------+ > + > SCMI_MISC: System Control and Management MISC Vendor Protocol > ================================================================ > Thanks, Cristian
On Wed, Feb 12, 2025 at 03:40:25PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Add Logical Machine Management(LMM) protocol which is intended for boot, > shutdown, and reset of other logical machines (LM). It is usually used to > allow one LM to manager another used as an offload or accelerator engine. > Hi, > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/arm_scmi/vendors/imx/Kconfig | 11 + > drivers/firmware/arm_scmi/vendors/imx/Makefile | 1 + > drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c | 270 +++++++++++++++++++++ > include/linux/scmi_imx_protocol.h | 31 +++ > 4 files changed, 313 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig > index a01bf5e47301d2f93c9bfc7eebc77e083ea4ed75..1a936fc87d2350e2a21bccd45dfbeebfa3b90286 100644 > --- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig > +++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig > @@ -12,6 +12,17 @@ config IMX_SCMI_BBM_EXT > To compile this driver as a module, choose M here: the > module will be called imx-sm-bbm. > > +config IMX_SCMI_LMM_EXT > + tristate "i.MX SCMI LMM EXTENSION" > + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) > + default y if ARCH_MXC > + help > + This enables i.MX System Logical Machine Protocol to > + manage Logical Machines boot, shutdown and etc. > + > + To compile this driver as a module, choose M here: the > + module will be called imx-sm-lmm. > + > config IMX_SCMI_MISC_EXT > tristate "i.MX SCMI MISC EXTENSION" > depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) > diff --git a/drivers/firmware/arm_scmi/vendors/imx/Makefile b/drivers/firmware/arm_scmi/vendors/imx/Makefile > index d3ee6d5449244a4f5cdf6abcf1845f312c512325..f39a99ccaf9af757475e8b112d224669444d7ddc 100644 > --- a/drivers/firmware/arm_scmi/vendors/imx/Makefile > +++ b/drivers/firmware/arm_scmi/vendors/imx/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o > +obj-$(CONFIG_IMX_SCMI_LMM_EXT) += imx-sm-lmm.o > obj-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o > diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c > new file mode 100644 > index 0000000000000000000000000000000000000000..4b9211df2329623fae0400039db91cb2b98cded1 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System control and Management Interface (SCMI) NXP LMM Protocol > + * > + * Copyright 2025 NXP > + */ > + > +#include <linux/bits.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/scmi_protocol.h> > +#include <linux/scmi_imx_protocol.h> > + > +#include "../../protocols.h" > +#include "../../notify.h" > + > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000 > + > +enum scmi_imx_lmm_protocol_cmd { > + SCMI_IMX_LMM_ATTRIBUTES = 0x3, > + SCMI_IMX_LMM_BOOT = 0x4, > + SCMI_IMX_LMM_RESET = 0x5, > + SCMI_IMX_LMM_SHUTDOWN = 0x6, > + SCMI_IMX_LMM_WAKE = 0x7, > + SCMI_IMX_LMM_SUSPEND = 0x8, > + SCMI_IMX_LMM_NOTIFY = 0x9, > + SCMI_IMX_LMM_RESET_REASON = 0xA, > + SCMI_IMX_LMM_POWER_ON = 0xB, > + SCMI_IMX_LMM_RESET_VECTOR_SET = 0xC, > +}; > + > +struct scmi_imx_lmm_priv { > + u32 nr_lmm; > +}; > + > +#define SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(x) (((x) & 0xFFU)) > +struct scmi_msg_imx_lmm_protocol_attributes { > + __le32 attributes; > +}; > + > +struct scmi_msg_imx_lmm_attributes_out { > + __le32 lmid; > + __le32 attributes; > + __le32 state; > + __le32 errstatus; > + u8 name[LMM_MAX_NAME]; > +}; > + > +struct scmi_imx_lmm_reset_vector_set_in { > + __le32 lmid; > + __le32 cpuid; > +#define SCMI_LMM_VEC_FLAGS_TABLE BIT(0) > + __le32 flags; > + __le32 resetvectorlow; > + __le32 resetvectorhigh; > +}; > + > +struct scmi_imx_lmm_shutdown_in { > + __le32 lmid; > + __le32 flags; > +}; > + > +static int scmi_imx_lmm_validate_lmid(const struct scmi_protocol_handle *ph, u32 lmid) > +{ > + struct scmi_imx_lmm_priv *priv = ph->get_priv(ph); > + > + if (lmid >= priv->nr_lmm) > + return -EINVAL; > + > + return 0; > +} > + > +static int scmi_imx_lmm_attributes(const struct scmi_protocol_handle *ph, > + u32 lmid, struct scmi_imx_lmm_info *info) > +{ > + struct scmi_msg_imx_lmm_attributes_out *out; > + struct scmi_xfer *t; > + int ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_ATTRIBUTES, sizeof(u32), 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(lmid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + out = t->rx.buf; > + info->lmid = le32_to_cpu(out->lmid); > + info->state = le32_to_cpu(out->state); > + info->errstatus = le32_to_cpu(out->errstatus); > + strscpy(info->name, out->name); > + dev_dbg(ph->dev, "i.MX LMM: Logical Machine(%d), name: %s\n", > + info->lmid, out->name); > + } else { > + dev_err(ph->dev, "i.MX LMM: Failed to get info of Logical Machine(%u)\n", lmid); > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_lmm_boot(const struct scmi_protocol_handle *ph, u32 lmid) > +{ > + struct scmi_xfer *t; > + int ret; > + > + ret = scmi_imx_lmm_validate_lmid(ph, lmid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_BOOT, sizeof(u32), > + 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(lmid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + ..this.... > +static int scmi_imx_lmm_power_on(const struct scmi_protocol_handle *ph, u32 lmid) > +{ > + struct scmi_xfer *t; > + int ret; > + > + ret = scmi_imx_lmm_validate_lmid(ph, lmid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_POWER_ON, sizeof(u32), > + 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(lmid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} ...can be refatcored to use one common workhorse fcuntion sued by both ops... > + > +static int scmi_imx_lmm_reset_vector_set(const struct scmi_protocol_handle *ph, > + u32 lmid, u32 cpuid, u32 flags, u64 vector) > +{ > + struct scmi_imx_lmm_reset_vector_set_in *in; > + struct scmi_xfer *t; > + int ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_RESET_VECTOR_SET, sizeof(*in), > + 0, &t); > + if (ret) > + return ret; > + > + in = t->tx.buf; > + in->lmid = cpu_to_le32(lmid); > + in->cpuid = cpu_to_le32(cpuid); > + in->flags = cpu_to_le32(flags); ...bitfields in a flag field must not be enianity converted...only eventually subfields representing numbers (liek you did above)... ..I feel FIELD_PREP should be fine...or even BIT(0) really given what these flags represents... ...again check issues with smatch.... > + in->resetvectorlow = cpu_to_le32(lower_32_bits(vector)); > + in->resetvectorhigh = cpu_to_le32(upper_32_bits(vector)); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_lmm_shutdown(const struct scmi_protocol_handle *ph, u32 lmid, > + u32 flags) > +{ > + struct scmi_imx_lmm_shutdown_in *in; > + struct scmi_xfer *t; > + int ret; > + > + ret = scmi_imx_lmm_validate_lmid(ph, lmid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_SHUTDOWN, sizeof(*in), > + 0, &t); > + if (ret) > + return ret; > + > + in = t->tx.buf; > + in->lmid = cpu_to_le32(lmid); > + in->flags = cpu_to_le32(flags); Same here, smatch would complain if you remove straight away this cpu_to_le32(), BUT this is a bitfield and does NOT contain any longer-than-2 number value that needs to be endianess manipulated...you just have to be able to set the required BIT 0 and 1, whitouth pissing off smatch :P > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static const struct scmi_imx_lmm_proto_ops scmi_imx_lmm_proto_ops = { > + .lmm_boot = scmi_imx_lmm_boot, > + .lmm_info = scmi_imx_lmm_attributes, > + .lmm_power_on = scmi_imx_lmm_power_on, > + .lmm_reset_vector_set = scmi_imx_lmm_reset_vector_set, > + .lmm_shutdown = scmi_imx_lmm_shutdown, > +}; > + > +static int scmi_imx_lmm_protocol_attributes_get(const struct scmi_protocol_handle *ph, > + struct scmi_imx_lmm_priv *priv) > +{ > + struct scmi_msg_imx_lmm_protocol_attributes *attr; > + struct scmi_xfer *t; > + int ret; > + > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, > + sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + priv->nr_lmm = SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(le32_to_cpu(attr->attributes)); This seems wrong to me...you have an 8bit field to extract from an attribute field...so I would at first cut the byte out and then convert to LE, NOT the other way around like you are doing (i.e.: le32_to_cpu((attr->attributes & 0xFF))). Even BETTER to use: le32_get_bits((x), GENMASK(7, 0)) ...the thing is, being a single byte you really SHOULD NOT need to address any endianity concern (i.e. FIELD_GET(GENMASK(7, 0), (x))), BUT I fear that if you dont use some of the le32_ accessors smatch/sparse will complain since the message field is, correctly, declared as __le32. So le32_get_bits is the way to go (bit check with the static analyzer :P) > + dev_info(ph->dev, "i.MX LMM: %d Logical Machines\n", > + priv->nr_lmm); > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_lmm_protocol_init(const struct scmi_protocol_handle *ph) > +{ > + struct scmi_imx_lmm_priv *info; > + u32 version; > + int ret; > + > + ret = ph->xops->version_get(ph, &version); > + if (ret) > + return ret; > + > + dev_info(ph->dev, "NXP SM LMM Version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + info = devm_kzalloc(ph->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + ret = scmi_imx_lmm_protocol_attributes_get(ph, info); > + if (ret) > + return ret; > + > + return ph->set_priv(ph, info, version); > +} > + > +static const struct scmi_protocol scmi_imx_lmm = { > + .id = SCMI_PROTOCOL_IMX_LMM, > + .owner = THIS_MODULE, > + .instance_init = &scmi_imx_lmm_protocol_init, > + .ops = &scmi_imx_lmm_proto_ops, > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, > + .vendor_id = SCMI_IMX_VENDOR, > + .sub_vendor_id = SCMI_IMX_SUBVENDOR, > +}; > +module_scmi_protocol(scmi_imx_lmm); > + Should this also be added: MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_LMM) "-" SCMI_IMX_VENDOR); ...after this went in: commit d4cc8912cbff4990940b33cc61a9b09ddaee9704 Author: Cristian Marussi <cristian.marussi@arm.com> Date: Mon Dec 9 16:49:56 2024 +0000 firmware: arm_scmi: Add module aliases to i.MX vendor protocol ...I think is in v6.14-rc already to rebae on... > +MODULE_DESCRIPTION("i.MX SCMI LMM driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/scmi_imx_protocol.h b/include/linux/scmi_imx_protocol.h > index 53b356a26414279f4aaaa8287c32209ed1ad57b4..07779c36ef9a126907e26e304a8feca16fd60ab2 100644 > --- a/include/linux/scmi_imx_protocol.h > +++ b/include/linux/scmi_imx_protocol.h > @@ -11,8 +11,10 @@ > #include <linux/bitfield.h> > #include <linux/device.h> > #include <linux/notifier.h> > +#include <linux/scmi_protocol.h> > #include <linux/types.h> > > +#define SCMI_PROTOCOL_IMX_LMM 0x80 > #define SCMI_PROTOCOL_IMX_BBM 0x81 > #define SCMI_PROTOCOL_IMX_MISC 0x84 > > @@ -57,4 +59,33 @@ struct scmi_imx_misc_proto_ops { > int (*misc_ctrl_req_notify)(const struct scmi_protocol_handle *ph, > u32 ctrl_id, u32 evt_id, u32 flags); > }; > + > +#define LMM_ID_DISCOVER 0xFFFFFFFFU What is this ? Documented anywhere ? > +#define LMM_MAX_NAME 16 > + > +enum scmi_imx_lmm_state { > + LMM_STATE_LM_OFF, > + LMM_STATE_LM_ON, > + LMM_STATE_LM_SUSPEND, > + LMM_STATE_LM_POWERED, > +}; > + > +struct scmi_imx_lmm_info { > + u32 lmid; > + enum scmi_imx_lmm_state state; > + u32 errstatus; > + u8 name[LMM_MAX_NAME]; > +}; > + > +struct scmi_imx_lmm_proto_ops { > + int (*lmm_boot)(const struct scmi_protocol_handle *ph, u32 lmid); > + int (*lmm_info)(const struct scmi_protocol_handle *ph, u32 lmid, > + struct scmi_imx_lmm_info *info); > + int (*lmm_power_on)(const struct scmi_protocol_handle *ph, u32 lmid); > + int (*lmm_reset_vector_set)(const struct scmi_protocol_handle *ph, > + u32 lmid, u32 cpuid, u32 flags, u64 vector); > + int (*lmm_shutdown)(const struct scmi_protocol_handle *ph, u32 lmid, > + u32 flags); > +}; > + > #endif > Thanks, Cristian
On Wed, Feb 12, 2025 at 03:40:26PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > This protocol allows an agent to start, stop a CPU or set reset vector. It > is used to manage auxiliary CPUs in an LM (e.g. additional cores in an AP > cluster). > Hi, > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/arm_scmi/vendors/imx/Kconfig | 11 + > drivers/firmware/arm_scmi/vendors/imx/Makefile | 1 + > drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c | 287 +++++++++++++++++++++ > include/linux/scmi_imx_protocol.h | 10 + > 4 files changed, 309 insertions(+) > [snip] > + > +struct scmi_imx_cpu_info_get_out { > +#define CPU_RUN_MODE_START 0 > +#define CPU_RUN_MODE_HOLD 1 > +#define CPU_RUN_MODE_STOP 2 > +#define CPU_RUN_MODE_SLEEP 3 > + __le32 runmode; > + __le32 sleepmode; > + __le32 resetvectorlow; > + __le32 resetvectorhigh; > +}; > + > +static int scmi_imx_cpu_validate_cpuid(const struct scmi_protocol_handle *ph, > + u32 cpuid) > +{ > + struct scmi_imx_cpu_info *info = ph->get_priv(ph); > + > + if (cpuid >= info->nr_cpu) > + return -EINVAL; > + > + return 0; > +} > + > +static int scmi_imx_cpu_start(const struct scmi_protocol_handle *ph, u32 cpuid) > +{ > + struct scmi_xfer *t; > + int ret; > + > + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_START, sizeof(u32), > + 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(cpuid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_cpu_stop(const struct scmi_protocol_handle *ph, u32 cpuid) > +{ > + struct scmi_xfer *t; > + int ret; > + > + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_STOP, sizeof(u32), > + 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(cpuid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + Please refactor and unify this 2 seemingly identical start/stop funcs by defining a common helper... > +static int scmi_imx_cpu_reset_vector_set(const struct scmi_protocol_handle *ph, > + u32 cpuid, u64 vector, bool start, > + bool boot, bool resume) > +{ > + struct scmi_imx_cpu_reset_vector_set_in *in; > + struct scmi_xfer *t; > + int ret; > + u32 flags; > + > + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_RESET_VECTOR_SET, sizeof(*in), > + 0, &t); > + if (ret) > + return ret; > + > + in = t->tx.buf; > + in->cpuid = cpu_to_le32(cpuid); > + flags = start ? CPU_VEC_FLAGS_START : 0; > + flags |= boot ? CPU_VEC_FLAGS_BOOT : 0; > + flags |= resume ? CPU_VEC_FLAGS_RESUME : 0; > + in->flags = cpu_to_le32(flags); ...you should just avoid the endianess helper given that is a bitfield (I cannot exclude that there are other places where we wrongly endianIZE bitfields...) and I think that the best way to do it without cause smatch to cry would be to use le32_encode_bits() which should just produce the desired flags into an __le32 le32_encode_bits and friends are used throughout the code base and added https://patchwork.ozlabs.org/project/ubuntu-kernel/patch/20190606034255.2192-2-aaron.ma@canonical.com/ which seems to be the best (and only) documentation :P > + in->resetvectorlow = cpu_to_le32(lower_32_bits(vector)); > + in->resetvectorhigh = cpu_to_le32(upper_32_bits(vector)); > + ret = ph->xops->do_xfer(ph, t); > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_cpu_started(const struct scmi_protocol_handle *ph, u32 cpuid, > + bool *started) > +{ > + struct scmi_imx_cpu_info_get_out *out; > + struct scmi_xfer *t; > + u32 mode; > + int ret; > + maybe overlay paranoid...but if (!started) return -EINVAL; ...up to you, if you feel paranoid too > + *started = false; > + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); > + if (ret) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_INFO_GET, sizeof(u32), > + 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(cpuid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + out = t->rx.buf; > + mode = le32_to_cpu(out->runmode); > + if ((mode == CPU_RUN_MODE_START) || (mode == CPU_RUN_MODE_SLEEP)) > + *started = true; > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static const struct scmi_imx_cpu_proto_ops scmi_imx_cpu_proto_ops = { > + .cpu_reset_vector_set = scmi_imx_cpu_reset_vector_set, > + .cpu_start = scmi_imx_cpu_start, > + .cpu_started = scmi_imx_cpu_started, > + .cpu_stop = scmi_imx_cpu_stop, > +}; > + > +static int scmi_imx_cpu_protocol_attributes_get(const struct scmi_protocol_handle *ph, > + struct scmi_imx_cpu_info *info) > +{ > + struct scmi_msg_imx_cpu_protocol_attributes *attr; > + struct scmi_xfer *t; > + int ret; > + > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, > + sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + info->nr_cpu = SCMI_IMX_CPU_PROTO_ATTR_NUM_CPUS(attr->attributes); le32_get_bits(attr->attributes, GENMASK()) > + dev_info(ph->dev, "i.MX SM CPU: %d cpus\n", > + info->nr_cpu); > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_cpu_attributes_get(const struct scmi_protocol_handle *ph, > + u32 cpuid) > +{ > + struct scmi_msg_imx_cpu_attributes_out *out; > + char name[SCMI_SHORT_NAME_MAX_SIZE] = {'\0'}; > + struct scmi_xfer *t; > + int ret; > + > + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_ATTRIBUTES, sizeof(u32), 0, &t); > + if (ret) > + return ret; > + > + put_unaligned_le32(cpuid, t->tx.buf); > + ret = ph->xops->do_xfer(ph, t); > + if (!ret) { > + out = t->rx.buf; > + strscpy(name, out->name, SCMI_SHORT_NAME_MAX_SIZE); > + dev_info(ph->dev, "i.MX CPU: name: %s\n", name); > + } else { > + dev_err(ph->dev, "i.MX cpu: Failed to get info of cpu(%u)\n", cpuid); > + } > + > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int scmi_imx_cpu_protocol_init(const struct scmi_protocol_handle *ph) > +{ > + struct scmi_imx_cpu_info *info; > + u32 version; > + int ret, i; > + > + ret = ph->xops->version_get(ph, &version); > + if (ret) > + return ret; > + > + dev_info(ph->dev, "NXP SM CPU Protocol Version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + info = devm_kzalloc(ph->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + ret = scmi_imx_cpu_protocol_attributes_get(ph, info); > + if (ret) > + return ret; > + > + for (i = 0; i < info->nr_cpu; i++) { > + ret = scmi_imx_cpu_attributes_get(ph, i); > + if (ret) > + return ret; > + } > + > + return ph->set_priv(ph, info, version); > +} > + > +static const struct scmi_protocol scmi_imx_cpu = { > + .id = SCMI_PROTOCOL_IMX_CPU, > + .owner = THIS_MODULE, > + .instance_init = &scmi_imx_cpu_protocol_init, > + .ops = &scmi_imx_cpu_proto_ops, > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, > + .vendor_id = SCMI_IMX_VENDOR, > + .sub_vendor_id = SCMI_IMX_SUBVENDOR, > +}; > +module_scmi_protocol(scmi_imx_cpu); similarly as LMM... MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_CPU) "-" SCMI_IMX_VENDOR); Thanks Cristian
Hi Cristian, On Mon, Feb 24, 2025 at 12:28:34PM +0000, Cristian Marussi wrote: >On Wed, Feb 12, 2025 at 03:40:23PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> Add i.MX95 Logical Machine Management and CPU Protocol documentation. >> > >Hi, > >a few comments below. > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 737 ++++++++++++++++++++++++ >> 1 file changed, 737 insertions(+) >> >> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst >> index b2dfd6c46ca2f5f12f0475c24cb54c060e9fa421..78a09cd8102becd5584d28bdef18df2d77fb7e7c 100644 >> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst >> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst >> @@ -32,6 +32,461 @@ port, and deploy the SM on supported processors. >> The SM implements an interface compliant with the Arm SCMI Specification >> with additional vendor specific extensions. >> > >In general I noticed that there is NOT specified anywhere if all the >following commands are mandatory or optional in your protocol: would be >better to be specified, as it is I will assume that are all mandatory. The Doc from [1] does not specify optional or mandatory, I need check with firmware owner to see whether this could be added. [1]https://github.com/nxp-imx/imx-sm > >> +SCMI_LMM: System Control and Management Logical Machine Management Vendor Protocol >> +================================================================================== >> + >> + [...] >> +PROTOCOL_ATTRIBUTES >> +~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x1 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|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 | > >...so this states how many LMs you have...ok... > >> ++------------------+-----------------------------------------------------------+ >> + >> +PROTOCOL_MESSAGE_ATTRIBUTES >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x2 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|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 >> + >> ++------------------+-----------------------------------------------------------+ >> +|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 | +------------------+-----------------------------------------------------------+ > >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_BOOT >> +~~~~~~~~ >> + >> +message_id: 0x4 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully booted. | > >...this and other commands below that cause a transition are a bit >ambiguos in their description IMHO...does this mean that on reception of >a SUCCESS the LM identified by lmid has successfully completed the boot ? > >...or, as I suppose, this being an immmediate command, SUCCESS means something >like 'boot succesfully started', NOT that when this SUCCESS reply comes back the >LM has booted...also becuse I can see a lot of notifications defined down below to >signal the completion of such operations... > >..if this is the case please clarify... right. This means the LM has been boot successfully started. Not means LM fully function up. I will updated this with "SUCCESS: if LM boots successfully started" > >...if this is NOT the case and this is intended to return synchronoulsy >when the required operation has completed, even though such machines are >maybe (?) minimal/tiny compute systems, can you guarantee that they can boot/shutdown >etc in a reasonably short time not to hog the channel for too long (I mean >well before the configured transport timeout)... > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_RESET >> +~~~~~~~~~ >> + >> +message_id: 0x5 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 flags |Reset flags: | >> +| |Bits[31:1] Reserved, must be zero. | >> +| |Bit[0] Graceful request: | >> +| |Set to 1 if the request is a graceful request. | >> +| |Set to 0 if the request is a forceful request. | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully resets. | > >Same ... does this simply mean LM reset issued successfully ? For graceful reset, platform will issue a notification to agent and just return, agent will do the reset, as linux reboot. For forceful reset, platform will do the force reset of agent, this is longer time. I will update with "The LMM RESET command finished successfully in graceful reset or LM successfully resets in forceful reset" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_SHUTDOWN >> +~~~~~~~~~~~~ >> + >> +message_id: 0x6 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 flags |Reset flags: | >> +| |Bits[31:1] Reserved, must be zero. | >> +| |Bit[0] Graceful request: | >> +| |Set to 1 if the request is a graceful request. | >> +| |Set to 0 if the request is a forceful request. | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully shutdowns. | > >Same...I suppose the shutdown has been sucessfully issued and could be >still in progress...if this is not the case I fear this immediate >command will potentially hog the channel for too long.... For graceful shutdown, platform will issue a notification to agent and just return, agent will do shutdown. For forceful shutdown, platform will do the force shutdown of agent, this is longer time. I will update with "The LMM shutdown command finished successfully in graceful request or LM successfully shutdown in forceful request" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_WAKE >> +~~~~~~~~ >> + >> +message_id: 0x7 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully wakes. | > >Same...I suppose you can know when an LM is fully woken only from the >dedicated notification receipt... I will updated this with "SUCCESS: if LM wake command successfully returns" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_SUSPEND >> +~~~~~~~~~~~ >> + >> +message_id: 0x8 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully suspends. | > >Same..I suppose the SCMI server will know when the suspend for this LM is >effective and will send a notification, so this is again only an indication >that the suspend process has started... Right. I will updated this with "SUCCESS: if LM suspend command successfully returns" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + > >In general, you could also think to make the bahvour of all this >operation configurable like teh RESET protocol does...so you could ask >for an LMM op to complete synchronously OR instead let it happen >asynchnously in the background after the command-request has completed >and then wait for one of the existing notification... > >...again if this is already the expected behaviour please clarifiy the sync/async >kind of execution that is expected when you issue such commands Yes, for graceful, it is async; for forceful, it is sync. If command not has graceful/forceful, such as lm wake/suspend, it is async > >> +LMM_NOTIFY >> +~~~~~~~~~~ >> + >> +message_id: 0x9 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 flags |Notification flags: | >> +| |Bits[31:3] Reserved, must be zero. | >> +| |Bit[3] Wake (resume) notification: | >> +| |Set to 1 to send notification. | >> +| |Set to 0 if no notification. | >> +| |Bit[2] Suspend (sleep) notification: | >> +| |Set to 1 to send notification. | >> +| |Set to 0 if no notification. | >> +| |Bit[1] Shutdown (off) notification: | >> +| |Set to 1 to send notification. | >> +| |Set to 0 if no notification. | >> +| |Bit[0] Boot (on) notification: | >> +| |Set to 1 to send notification. | >> +| |Set to 0 if no notification | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if the notification state successfully updated. | >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if input attributes flag specifies | >> +| |unsupported or invalid configurations. | >> +| |DENIED: if the agent does not have permission to request | >> +| |the notification. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_RESET_REASON >> +~~~~~~~~~~~~~~~~ >> + >> +message_id: 0xA >> +protocol_id: 0x80 > >What is this...a bit of explanation would be fine given that this >command is not straightforward to understand like the previous >boot/suspend etc... This function returns the boot/shutdown/reset reason for an LM. Such as POR, WDOG and etc. > >> + >> ++---------------------+--------------------------------------------------------+ >> +|Parameters | >> ++---------------------+--------------------------------------------------------+ >> +|Name |Description | >> ++---------------------+--------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++---------------------+--------------------------------------------------------+ >> +|Return values | >> ++---------------------+--------------------------------------------------------+ >> +|Name |Description | >> ++---------------------+--------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully suspends. | > >? typo ? Sorry. This should be "the reset reason of the LM successfully updated" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine| >> +| |DENIED: if the agent does not have permission to request| >> +| |the reset reason. | >> ++---------------------+--------------------------------------------------------+ >> +|uint32 bootflags |Boot reason flags. This parameter has the format: | >> +| |Bits[31] Valid. | >> +| |Set to 1 if the entire reason is valid. | >> +| |Set to 0 if the entire reason is not valid. | >> +| |Bits[30:29] Reserved, must be zero. | >> +| |Bit[28] Valid origin: | >> +| |Set to 1 if the origin field is valid. | >> +| |Set to 0 if the origin field is not valid. | >> +| |Bits[27:24] Origin. | > >Are this origin values defined anywhere ? It varies in the current imx-sm implementation, there is no spec defining this, it could be a LMID or BBM register value or else. > >> +| |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. | > >And this errors ID ? which value should I expect here...and to what this >errors refer to... > >> +| |Bits[22:8] Error ID. | There is not doc defining this. It just varies, It could be BBM button, agentID, faultID or a peripheral ID or else. >> +| |Bit[7:0] Reason(WDOG, POR, FCCU and etc) | >> ++---------------------+--------------------------------------------------------+ >> +|uint32 shutdownflags |Shutdown reason flags. This parameter has the format: | >> +| |Bits[31] Valid. | >> +| |Set to 1 if the entire reason is valid. | >> +| |Set to 0 if the entire reason is not valid. | >> +| |Bits[30:29] Number of valid extended info words. | >> +| |Bit[28] Valid origin: | >> +| |Set to 1 if the origin field is valid. | >> +| |Set to 0 if the origin field is not valid. | > >Ditto. >> +| |Bits[27:24] Origin. | >> +| |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. | >Ditto. >> +| |Bit[7:0] Reason | >Ditto. I will check imx-sm code and add more info for upper all three, but there is no documentation and it varies in imx-sm code repo. >> ++---------------------+--------------------------------------------------------+ >> +|uint32 extinfo[0,20] |Array of extended info words(e.g. fault pc) | >> ++---------------------+--------------------------------------------------------+ > >..so what does this field represent....is this a fixed sized array ? seems more likely >to be dynamically sized in range [0,20] AFAICU... This should be updated with "extinfo[3]". The length has been speicified in "Bits[30:29] Number of valid extended info words." > >...now...if it is dynamically sized: > >+ there should be a size field declaring the size of the returned >answer, if not you should rely on the transport size to know where the >message ends (unless if hard-coded based on the above flags...in that >case explain and document) already specified in shutdownflags BITS[30:29] > >+ you cannot assume that such a potentially big reply reply fit into a >single message due to possible limitations of the transport >sizes...IOW you should think about a 'multi-part' message like many >exist in SCMI where the reply reports returned+remaining entries... >..how can yo be sure to fit in any transport...this message has >currently a potential max payload reply of 96 bytes ... > >> + >> +LMM_POWER_ON >> +~~~~~~~~~~~~ >> + >> +message_id: 0xB >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully powers on. | > >Beside the same consideration as above regarding what this SUCCESS >mean...what is meant to be the difference between LMM_POWER_ON and LMM_BOOT ? POWER ON, it is just power up, not means the LMM is kicked out to boot. BOOT, means LMM is kicked out to boot For example, CPU has power and reset release function. power on, means power up the CPU boot, means reset release to let CPU run. Especially for Cortex-M core, the TCM is in same power domain as Cortex-M. before booting the core, need power up the core to loading images to TCM, then boot the core. > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | >> +| |DENIED: if the agent does not have permission to manage the| >> +| |the LM specified by lmid. | >> ++------------------+-----------------------------------------------------------+ >> + >> +LMM_RESET_VECTOR_SET >> +~~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0xC >> +protocol_id: 0x80 >> + >> ++-----------------------+------------------------------------------------------+ >> +|Parameters | >> ++-----------------------+------------------------------------------------------+ >> +|Name |Description | >> ++-----------------------+------------------------------------------------------+ >> +|uint32 lmid |ID of the Logical Machine | >> ++-----------------------+------------------------------------------------------+ >> +|uint32 cpuid |ID of the CPU inside the LM | >> ++-----------------------+------------------------------------------------------+ >> +|uint32 flags |Reset vector flags | >> +| |Bits[31:1] Reserved, must be zero. | >> +| |Bit[0] Table flag. | >> +| |Set to 1 if vector is the vector table base address | > >So what is this when Bit[0] is set to zero instead ? Per current imx-sm, this flags is not used as of now, just a reserved flag for future extension. So 0 or 1 does not matter here. I will update with Bits[31:0] Reserved. > >> ++-----------------------+------------------------------------------------------+ >> +|uint32 resetVectorLow |Lower vector | >> ++-----------------------+------------------------------------------------------+ >> +|uint32 resetVectorHigh |Higher vector | >> ++-----------------------+------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if LM successfully powers on. | > >...typo ?... "If reset vector is set successfully" > >> +| |NOT_FOUND: if lmId not points to a valid logical machine, | >> +| |or cpuId is not valid. >> +| |INVALID_PARAMETERS: if reset vector is invalid. | >> +| |DENIED: if the agent does not have permission to set the | >> +| |the reset vector for the CPU in the LM. | >> ++------------------+-----------------------------------------------------------+ >> + >> +NEGOTIATE_PROTOCOL_VERSION >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x10 >> +protocol_id: 0x80 >> + >> ++--------------------+---------------------------------------------------------+ >> +|Parameters | >> ++--------------------+---------------------------------------------------------+ >> +|Name |Description | >> ++--------------------+---------------------------------------------------------+ >> +|uint32 version |The negotiated protocol version the agent intends to use | >> ++--------------------+---------------------------------------------------------+ >> +|Return values | >> ++--------------------+---------------------------------------------------------+ >> +|Name |Description | >> ++--------------------+---------------------------------------------------------+ >> +|int32 status |SUCCESS: if the negotiated protocol version is supported | >> +| |by the platform. All commands, responses, and | >> +| |notifications post successful return of this command must| >> +| |comply with the negotiated version. | >> +| |NOT_SUPPORTED: if the protocol version is not supported. | >> ++--------------------+---------------------------------------------------------+ >> + >> +Notifications >> +_____________ >> + >> +LMM_EVENT >> +~~~~~~~~~ >> + >> +message_id: 0x0 >> +protocol_id: 0x80 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 lmid |Identifier for the LM that caused the transition. | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 eventlm |Identifier for the LM the event is for. | > >So this is the LM that the event refer to ? i.e. this LM eventlm is the >one that is booting/resetting/shutdown ? >if this is the case, maybe better as: > > "Identifier of the LM this event refers to." Thanks, will update as you suggest. > >> ++------------------+-----------------------------------------------------------+ >> +|uint32 flags |LM events: | >> +| |Bits[31:3] Reserved, must be zero. | >> +| |Bit[3] Wake (resume) event: | >> +| |1 LM has awakened. | >> +| |0 not a wake event. | >> +| |Bit[2] Suspend (sleep) event: | >> +| |1 LM has suspended. | >> +| |0 not a suspend event. | >> +| |Bit[1] Shutdown (off) event: | >> +| |1 LM has shutdown. | >> +| |0 not a shutdown event. | >> +| |Bit[0] Boot (on) event: | >> +| |1 LM has booted. | >> +| |0 not a boot event. | >> ++------------------+-----------------------------------------------------------+ >> + >> SCMI_BBM: System Control and Management BBM Vendor Protocol >> ============================================================== >> >> @@ -436,6 +891,288 @@ protocol_id: 0x81 >> | |0 no button change detected. | >> +------------------+-----------------------------------------------------------+ >> >> +SCMI_CPU: System Control and Management CPU Vendor Protocol >> +============================================================== >> + >> +This protocol allows an agent to start or stop a CPU. It is used to manage >> +auxiliary CPUs in an LM (e.g. additional cores in an AP cluster or >> +Cortex-M cores). >> +Note: For cores in AP cluster, ATF will use CPU protocol to handle them. > >So this is sort of a PSCI-by-proxy in which you ask the server to turn >on/off some of the LM CPUs ? Yes. A55 cores are in one LM, so in ATF one A55 use cpu protocol to manage other A55 cores. > >First question would be is not enough to just ask for a specific LM to >be booted with LMM ? or add to the LMM protocol a way to specifiy the LMM is a large scope, CPU could be inside LMM. >'level' of boot that you ask for an LM...I may miss many details but it >seems to me a bit odd that you are allowd to ask, from the agent that >could have just requested an LM to boot to have some CPUs ON or OFF... >...and specify the boot/resume address vectors here to use when turning >ON... There is case the M7 and A55 are in the same LM, so in such case, if A55 core needs to manage M7, need use CPU protocol. If A55 and M7 are in two separate LMs, A55 needs use LMM protocol to manage M7 LM. > >Maybe a bit more of explanation of the rationale and expectations here >could help understanding the differet needs from LMM There are some internal discussion between NXP and ARM if you search your mail "NXP-ARM SCMI OEM extension". I could not post them all here. > >> + >> +The CPU protocol provides functions to: >> + >> +- Describe the protocol version. >> +- Discover implementation attributes. >> +- Discover the CPUs defined in the system. >> +- Start a CPU. >> +- Stop a CPU. >> +- Set the boot and resume addresses for a CPU. >> +- Set the sleep mode of a CPU. >> +- Configure wake-up sources for a CPU. >> +- Configure power domain reactions (LPM mode and retention mask) for a CPU. >> +- The CPU IDs can be found in the CPU section of the SoC DEVICE: SM Device >> + Interface. They can also be found in the SoC RM. See the CPU Mode Control >> + (CMC) list in General Power Controller (GPC) section. >> + >> +CPU settings are not aggregated and setting their state is normally exclusive >> +to one client. >> + >> +Commands: >> +_________ >> + >> +PROTOCOL_VERSION >> +~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x0 >> +protocol_id: 0x82 >> + >> ++---------------+--------------------------------------------------------------+ >> +|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: 0x82 >> + >> ++---------------+--------------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status | See ARM SCMI Specification for status code definitions. | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 attributes |Protocol attributes: | >> +| |Bits[31:16] Reserved, must be zero. | >> +| |Bits[15:0] Number of CPUs | >> ++------------------+-----------------------------------------------------------+ >> + >> +PROTOCOL_MESSAGE_ATTRIBUTES >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x2 >> +protocol_id: 0x82 >> + >> ++---------------+--------------------------------------------------------------+ >> +|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 | >> ++------------------+-----------------------------------------------------------+ >> + >> +CPU_ATTRIBUTES >> +~~~~~~~~~~~~~~ >> + >> +message_id: 0x4 >> +protocol_id: 0x82 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if valid attributes are returned successfully. | >> +| |NOT_FOUND: if the cpuid is not valid. | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 attributes |Bits[31:0] Reserved, must be zero | >> ++------------------+-----------------------------------------------------------+ >> +|char name[16] |NULL terminated ASCII string with CPU name up to 16 bytes | >> ++------------------+-----------------------------------------------------------+ >> + >> +CPU_START >> +~~~~~~~~~ >> + >> +message_id: 0x4 >> +protocol_id: 0x82 >> + > >Any constraint to call this only after having successfully called RESET_VECROR_SET ? No constraint, if not called RESET_VECROR_SET, it will use default value 0. > >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if the cpu is started successfully. | >> +| |NOT_FOUND: if cpuid is not valid. | >> +| |DENIED: the calling agent is not allowed to start this CPU.| >> ++------------------+-----------------------------------------------------------+ >> + >> +CPU_STOP >> +~~~~~~~~ >> + >> +message_id: 0x5 >> +protocol_id: 0x82 >> + >> ++------------------+-----------------------------------------------------------+ >> +|Parameters | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++------------------+-----------------------------------------------------------+ >> +|Return values | >> ++------------------+-----------------------------------------------------------+ >> +|Name |Description | >> ++------------------+-----------------------------------------------------------+ >> +|int32 status |SUCCESS: if the cpu is started successfully. | >> +| |NOT_FOUND: if cpuid is not valid. | >> +| |DENIED: the calling agent is not allowed to stop this CPU. | >> ++------------------+-----------------------------------------------------------+ >> + >> +CPU_RESET_VECTOR_SET >> +~~~~~~~~~~~~~~~~~~~~ >> + >> +message_id: 0x6 >> +protocol_id: 0x82 >> + >> ++----------------------+-------------------------------------------------------+ >> +|Parameters | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 flags |Reset vector flags: | >> +| |Bit[31] Resume flag. | >> +| |Set to 1 to update the reset vector used on resume. | >> +| |Bit[30] Boot flag. | >> +| |Set to 1 to update the reset vector used for boot. | >> +| |Bits[29:1] Reserved, must be zero. | >> +| |Bit[0] Table flag. | >> +| |Set to 1 if vector is the vector table base address. | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 resetVectorLow |Lower vector: | >> +| |If bit[0] of flags is 0, the lower 32 bits of the | >> +| |physical address where the CPU should execute from on | >> +| |reset. If bit[0] of flags is 1, the lower 32 bits of | >> +| |the vector table base address | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 resetVectorhigh|Upper vector: | >> +| |If bit[0] of flags is 0, the upper 32 bits of the | >> +| |physical address where the CPU should execute from on | >> +| |reset. If bit[0] of flags is 1, the upper 32 bits of | >> +| |the vector table base address | >> ++----------------------+-------------------------------------------------------+ >> +|Return values | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|int32 status |SUCCESS: if the CPU reset vector is set successfully. | >> +| |NOT_FOUND: if cpuId does not point to a valid CPU. | >> +| |INVALID_PARAMETERS: the requested vector type is not | >> +| |supported by this CPU. | >> +| |DENIED: the calling agent is not allowed to set the | >> +| |reset vector of this CPU | >> ++----------------------+-------------------------------------------------------+ >> + >> +CPU_SLEEP_MODE_SET >> +~~~~~~~~~~~~~~~~~~ >> +message_id: 0x7 >> +protocol_id: 0x82 >> + >> ++----------------------+-------------------------------------------------------+ >> +|Parameters | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 flags |Sleep mode flags: | >> +| |Bits[31:1] Reserved, must be zero. | >> +| |Bit[0] IRQ mux: | >> +| |If set to 1 the wakeup mux source is the GIC, else if 0| >> +| |then the GPC | > >What is the GPC in this context ? General Power Controller. > >> ++----------------------+-------------------------------------------------------+ >> +|uint32 sleepmode |target sleep mode | > >What values can this assume ? Is there any predefined sleep value here ? From imx-sm, the define is hardware related. I could list in next version. #define CPU_SLEEP_MODE_RUN 0U #define CPU_SLEEP_MODE_WAIT 1U #define CPU_SLEEP_MODE_STOP 2U #define CPU_SLEEP_MODE_SUSPEND 3U RUN, WAIT, STOP, SUSPEND is GPC concept. it means when wfi, the GPC will do what action. > >> ++----------------------+-------------------------------------------------------+ >> +|Return values | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|int32 status |SUCCESS: if the CPU sleep mode is set successfully. | >> +| |NOT_FOUND: if cpuId does not point to a valid CPU. | >> +| |INVALID_PARAMETERS: the sleepmode or flags is invalid. | >> +| |DENIED: the calling agent is not allowed to configure | >> +| |the CPU | >> ++----------------------+-------------------------------------------------------+ >> + >> +CPU_INFO_GET >> +~~~~~~~~~~~~ >> +message_id: 0xC >> +protocol_id: 0x82 >> + >> ++----------------------+-------------------------------------------------------+ >> +|Parameters | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 cpuid |Identifier for the CPU | >> ++----------------------+-------------------------------------------------------+ >> +|Return values | >> ++----------------------+-------------------------------------------------------+ >> +|Name |Description | >> ++----------------------+-------------------------------------------------------+ >> +|int32 status |SUCCESS: if valid attributes are returned successfully.| >> +| |NOT_FOUND: if the cpuid is not valid. | >> ++----------------------+-------------------------------------------------------+ >> +|uint32 runmode |Run mode for the CPU | > >What are the possible runmode values ? #define CPU_RUN_MODE_START 0U // start cpu #define CPU_RUN_MODE_HOLD 1U // power up and hold cpu #define CPU_RUN_MODE_STOP 2U // reset and hold cpu #define CPU_RUN_MODE_SLEEP 3U // in cpuidle Appreciate for detailed reviewing on the documentation. Thanks, Peng
On Mon, Feb 24, 2025 at 03:54:19PM +0000, Cristian Marussi wrote: >On Wed, Feb 12, 2025 at 03:40:25PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> Add Logical Machine Management(LMM) protocol which is intended for boot, >> shutdown, and reset of other logical machines (LM). It is usually used to >> allow one LM to manager another used as an offload or accelerator engine. >> > >Hi, > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/firmware/arm_scmi/vendors/imx/Kconfig | 11 + >> drivers/firmware/arm_scmi/vendors/imx/Makefile | 1 + >> drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c | 270 +++++++++++++++++++++ >> include/linux/scmi_imx_protocol.h | 31 +++ >> 4 files changed, 313 insertions(+) >> >> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig >> index a01bf5e47301d2f93c9bfc7eebc77e083ea4ed75..1a936fc87d2350e2a21bccd45dfbeebfa3b90286 100644 >> --- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig >> +++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig >> @@ -12,6 +12,17 @@ config IMX_SCMI_BBM_EXT >> To compile this driver as a module, choose M here: the >> module will be called imx-sm-bbm. >> >> +config IMX_SCMI_LMM_EXT >> + tristate "i.MX SCMI LMM EXTENSION" >> + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) >> + default y if ARCH_MXC >> + help >> + This enables i.MX System Logical Machine Protocol to >> + manage Logical Machines boot, shutdown and etc. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called imx-sm-lmm. >> + >> config IMX_SCMI_MISC_EXT >> tristate "i.MX SCMI MISC EXTENSION" >> depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) >> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Makefile b/drivers/firmware/arm_scmi/vendors/imx/Makefile >> index d3ee6d5449244a4f5cdf6abcf1845f312c512325..f39a99ccaf9af757475e8b112d224669444d7ddc 100644 >> --- a/drivers/firmware/arm_scmi/vendors/imx/Makefile >> +++ b/drivers/firmware/arm_scmi/vendors/imx/Makefile >> @@ -1,3 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> obj-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o >> +obj-$(CONFIG_IMX_SCMI_LMM_EXT) += imx-sm-lmm.o >> obj-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o >> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c >> new file mode 100644 >> index 0000000000000000000000000000000000000000..4b9211df2329623fae0400039db91cb2b98cded1 >> --- /dev/null >> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c >> @@ -0,0 +1,270 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * System control and Management Interface (SCMI) NXP LMM Protocol >> + * >> + * Copyright 2025 NXP >> + */ >> + >> +#include <linux/bits.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/scmi_protocol.h> >> +#include <linux/scmi_imx_protocol.h> >> + >> +#include "../../protocols.h" >> +#include "../../notify.h" >> + >> +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000 >> + >> +enum scmi_imx_lmm_protocol_cmd { >> + SCMI_IMX_LMM_ATTRIBUTES = 0x3, >> + SCMI_IMX_LMM_BOOT = 0x4, >> + SCMI_IMX_LMM_RESET = 0x5, >> + SCMI_IMX_LMM_SHUTDOWN = 0x6, >> + SCMI_IMX_LMM_WAKE = 0x7, >> + SCMI_IMX_LMM_SUSPEND = 0x8, >> + SCMI_IMX_LMM_NOTIFY = 0x9, >> + SCMI_IMX_LMM_RESET_REASON = 0xA, >> + SCMI_IMX_LMM_POWER_ON = 0xB, >> + SCMI_IMX_LMM_RESET_VECTOR_SET = 0xC, >> +}; >> + >> +struct scmi_imx_lmm_priv { >> + u32 nr_lmm; >> +}; >> + >> +#define SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(x) (((x) & 0xFFU)) >> +struct scmi_msg_imx_lmm_protocol_attributes { >> + __le32 attributes; >> +}; >> + >> +struct scmi_msg_imx_lmm_attributes_out { >> + __le32 lmid; >> + __le32 attributes; >> + __le32 state; >> + __le32 errstatus; >> + u8 name[LMM_MAX_NAME]; >> +}; >> + >> +struct scmi_imx_lmm_reset_vector_set_in { >> + __le32 lmid; >> + __le32 cpuid; >> +#define SCMI_LMM_VEC_FLAGS_TABLE BIT(0) >> + __le32 flags; >> + __le32 resetvectorlow; >> + __le32 resetvectorhigh; >> +}; >> + >> +struct scmi_imx_lmm_shutdown_in { >> + __le32 lmid; >> + __le32 flags; >> +}; >> + >> +static int scmi_imx_lmm_validate_lmid(const struct scmi_protocol_handle *ph, u32 lmid) >> +{ >> + struct scmi_imx_lmm_priv *priv = ph->get_priv(ph); >> + >> + if (lmid >= priv->nr_lmm) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static int scmi_imx_lmm_attributes(const struct scmi_protocol_handle *ph, >> + u32 lmid, struct scmi_imx_lmm_info *info) >> +{ >> + struct scmi_msg_imx_lmm_attributes_out *out; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_ATTRIBUTES, sizeof(u32), 0, &t); >> + if (ret) >> + return ret; >> + >> + put_unaligned_le32(lmid, t->tx.buf); >> + ret = ph->xops->do_xfer(ph, t); >> + if (!ret) { >> + out = t->rx.buf; >> + info->lmid = le32_to_cpu(out->lmid); >> + info->state = le32_to_cpu(out->state); >> + info->errstatus = le32_to_cpu(out->errstatus); >> + strscpy(info->name, out->name); >> + dev_dbg(ph->dev, "i.MX LMM: Logical Machine(%d), name: %s\n", >> + info->lmid, out->name); >> + } else { >> + dev_err(ph->dev, "i.MX LMM: Failed to get info of Logical Machine(%u)\n", lmid); >> + } >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_lmm_boot(const struct scmi_protocol_handle *ph, u32 lmid) >> +{ >> + struct scmi_xfer *t; ... >> + > >..this.... > >> +static int scmi_imx_lmm_power_on(const struct scmi_protocol_handle *ph, u32 lmid) >> +{ >> + ... >> + return ret; >> +} > >...can be refatcored to use one common workhorse fcuntion sued by both >ops... ok. I will merge power on and boot into one function. > >> + >> +static int scmi_imx_lmm_reset_vector_set(const struct scmi_protocol_handle *ph, >> + u32 lmid, u32 cpuid, u32 flags, u64 vector) >> +{ >> + struct scmi_imx_lmm_reset_vector_set_in *in; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_RESET_VECTOR_SET, sizeof(*in), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + in = t->tx.buf; >> + in->lmid = cpu_to_le32(lmid); >> + in->cpuid = cpu_to_le32(cpuid); >> + in->flags = cpu_to_le32(flags); > >...bitfields in a flag field must not be enianity converted...only >eventually subfields representing numbers (liek you did above)... > >..I feel FIELD_PREP should be fine...or even BIT(0) really given what >these flags represents... I will mark flags as reserved as updated doc, because this flag is not used as of now. > >...again check issues with smatch.... yes, I will try this. > >> + in->resetvectorlow = cpu_to_le32(lower_32_bits(vector)); >> + in->resetvectorhigh = cpu_to_le32(upper_32_bits(vector)); >> + ret = ph->xops->do_xfer(ph, t); >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_lmm_shutdown(const struct scmi_protocol_handle *ph, u32 lmid, >> + u32 flags) >> +{ >> + struct scmi_imx_lmm_shutdown_in *in; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = scmi_imx_lmm_validate_lmid(ph, lmid); >> + if (ret) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_LMM_SHUTDOWN, sizeof(*in), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + in = t->tx.buf; >> + in->lmid = cpu_to_le32(lmid); >> + in->flags = cpu_to_le32(flags); > >Same here, smatch would complain if you remove straight away this >cpu_to_le32(), BUT this is a bitfield and does NOT contain any >longer-than-2 number value that needs to be endianess manipulated...you >just have to be able to set the required BIT 0 and 1, whitouth pissing >off smatch :P Only BIT0 used as of now. I will update as you suggest. > >> + ret = ph->xops->do_xfer(ph, t); >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static const struct scmi_imx_lmm_proto_ops scmi_imx_lmm_proto_ops = { >> + .lmm_boot = scmi_imx_lmm_boot, >> + .lmm_info = scmi_imx_lmm_attributes, >> + .lmm_power_on = scmi_imx_lmm_power_on, >> + .lmm_reset_vector_set = scmi_imx_lmm_reset_vector_set, >> + .lmm_shutdown = scmi_imx_lmm_shutdown, >> +}; >> + >> +static int scmi_imx_lmm_protocol_attributes_get(const struct scmi_protocol_handle *ph, >> + struct scmi_imx_lmm_priv *priv) >> +{ >> + struct scmi_msg_imx_lmm_protocol_attributes *attr; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, >> + sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + attr = t->rx.buf; >> + >> + ret = ph->xops->do_xfer(ph, t); >> + if (!ret) { >> + priv->nr_lmm = SCMI_IMX_LMM_PROTO_ATTR_NUM_LM(le32_to_cpu(attr->attributes)); > >This seems wrong to me...you have an 8bit field to extract from an >attribute field...so I would at first cut the byte out and then convert >to LE, NOT the other way around like you are doing >(i.e.: le32_to_cpu((attr->attributes & 0xFF))). > >Even BETTER to use: > > le32_get_bits((x), GENMASK(7, 0)) > >...the thing is, being a single byte you really SHOULD NOT need to address >any endianity concern (i.e. FIELD_GET(GENMASK(7, 0), (x))), BUT I fear that >if you dont use some of the le32_ accessors smatch/sparse will complain since >the message field is, correctly, declared as __le32. > >So le32_get_bits is the way to go (bit check with the static analyzer :P) Yup, fix in next version. > >> + dev_info(ph->dev, "i.MX LMM: %d Logical Machines\n", >> + priv->nr_lmm); >> + } >> + ... >> +}; >> +module_scmi_protocol(scmi_imx_lmm); >> + > > >Should this also be added: > >MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_LMM) "-" SCMI_IMX_VENDOR); > >...after this went in: fix in next version. > >> }; .... >> + >> +#define LMM_ID_DISCOVER 0xFFFFFFFFU > >What is this ? Documented anywhere ? LMM_ATTRIBUTES if ID is 0xFFFFFFFF, it will return current lmid. I will update in doc patch. >
On Mon, Feb 24, 2025 at 06:10:48PM +0000, Cristian Marussi wrote: >On Wed, Feb 12, 2025 at 03:40:26PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> This protocol allows an agent to start, stop a CPU or set reset vector. It >> is used to manage auxiliary CPUs in an LM (e.g. additional cores in an AP >> cluster). >> > >Hi, > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> drivers/firmware/arm_scmi/vendors/imx/Kconfig | 11 + >> drivers/firmware/arm_scmi/vendors/imx/Makefile | 1 + >> drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c | 287 +++++++++++++++++++++ >> include/linux/scmi_imx_protocol.h | 10 + >> 4 files changed, 309 insertions(+) >> > >[snip] > >> + >> +struct scmi_imx_cpu_info_get_out { >> +#define CPU_RUN_MODE_START 0 >> +#define CPU_RUN_MODE_HOLD 1 >> +#define CPU_RUN_MODE_STOP 2 >> +#define CPU_RUN_MODE_SLEEP 3 >> + __le32 runmode; >> + __le32 sleepmode; >> + __le32 resetvectorlow; >> + __le32 resetvectorhigh; >> +}; >> + >> +static int scmi_imx_cpu_validate_cpuid(const struct scmi_protocol_handle *ph, >> + u32 cpuid) >> +{ >> + struct scmi_imx_cpu_info *info = ph->get_priv(ph); >> + >> + if (cpuid >= info->nr_cpu) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static int scmi_imx_cpu_start(const struct scmi_protocol_handle *ph, u32 cpuid) >> +{ >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); >> + if (ret) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_START, sizeof(u32), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + put_unaligned_le32(cpuid, t->tx.buf); >> + ret = ph->xops->do_xfer(ph, t); >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_cpu_stop(const struct scmi_protocol_handle *ph, u32 cpuid) >> +{ >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); >> + if (ret) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_STOP, sizeof(u32), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + put_unaligned_le32(cpuid, t->tx.buf); >> + ret = ph->xops->do_xfer(ph, t); >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + > >Please refactor and unify this 2 seemingly identical start/stop funcs by >defining a common helper... ok. fix in next version. > >> +static int scmi_imx_cpu_reset_vector_set(const struct scmi_protocol_handle *ph, >> + u32 cpuid, u64 vector, bool start, >> + bool boot, bool resume) >> +{ >> + struct scmi_imx_cpu_reset_vector_set_in *in; >> + struct scmi_xfer *t; >> + int ret; >> + u32 flags; >> + >> + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); >> + if (ret) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_RESET_VECTOR_SET, sizeof(*in), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + in = t->tx.buf; >> + in->cpuid = cpu_to_le32(cpuid); >> + flags = start ? CPU_VEC_FLAGS_START : 0; >> + flags |= boot ? CPU_VEC_FLAGS_BOOT : 0; >> + flags |= resume ? CPU_VEC_FLAGS_RESUME : 0; >> + in->flags = cpu_to_le32(flags); > >...you should just avoid the endianess helper given that is a bitfield (I cannot >exclude that there are other places where we wrongly endianIZE bitfields...) and >I think that the best way to do it without cause smatch to cry would be >to use le32_encode_bits() which should just produce the desired flags >into an __le32 > >le32_encode_bits and friends are used throughout the code base and added > >https://patchwork.ozlabs.org/project/ubuntu-kernel/patch/20190606034255.2192-2-aaron.ma@canonical.com/ > >which seems to be the best (and only) documentation :P Thanks for the pointer, I will fix in next version as you suggest. > >> + in->resetvectorlow = cpu_to_le32(lower_32_bits(vector)); >> + in->resetvectorhigh = cpu_to_le32(upper_32_bits(vector)); >> + ret = ph->xops->do_xfer(ph, t); >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_cpu_started(const struct scmi_protocol_handle *ph, u32 cpuid, >> + bool *started) >> +{ >> + struct scmi_imx_cpu_info_get_out *out; >> + struct scmi_xfer *t; >> + u32 mode; >> + int ret; >> + > >maybe overlay paranoid...but > > if (!started) > return -EINVAL; > >...up to you, if you feel paranoid too > >> + *started = false; >> + ret = scmi_imx_cpu_validate_cpuid(ph, cpuid); >> + if (ret) >> + return ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_INFO_GET, sizeof(u32), >> + 0, &t); >> + if (ret) >> + return ret; >> + >> + put_unaligned_le32(cpuid, t->tx.buf); >> + ret = ph->xops->do_xfer(ph, t); >> + if (!ret) { >> + out = t->rx.buf; >> + mode = le32_to_cpu(out->runmode); >> + if ((mode == CPU_RUN_MODE_START) || (mode == CPU_RUN_MODE_SLEEP)) >> + *started = true; >> + } >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static const struct scmi_imx_cpu_proto_ops scmi_imx_cpu_proto_ops = { >> + .cpu_reset_vector_set = scmi_imx_cpu_reset_vector_set, >> + .cpu_start = scmi_imx_cpu_start, >> + .cpu_started = scmi_imx_cpu_started, >> + .cpu_stop = scmi_imx_cpu_stop, >> +}; >> + >> +static int scmi_imx_cpu_protocol_attributes_get(const struct scmi_protocol_handle *ph, >> + struct scmi_imx_cpu_info *info) >> +{ >> + struct scmi_msg_imx_cpu_protocol_attributes *attr; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0, >> + sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + attr = t->rx.buf; >> + >> + ret = ph->xops->do_xfer(ph, t); >> + if (!ret) { >> + info->nr_cpu = SCMI_IMX_CPU_PROTO_ATTR_NUM_CPUS(attr->attributes); > > > le32_get_bits(attr->attributes, GENMASK()) Fix in next version. > >> + dev_info(ph->dev, "i.MX SM CPU: %d cpus\n", >> + info->nr_cpu); >> + } >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_cpu_attributes_get(const struct scmi_protocol_handle *ph, >> + u32 cpuid) >> +{ >> + struct scmi_msg_imx_cpu_attributes_out *out; >> + char name[SCMI_SHORT_NAME_MAX_SIZE] = {'\0'}; >> + struct scmi_xfer *t; >> + int ret; >> + >> + ret = ph->xops->xfer_get_init(ph, SCMI_IMX_CPU_ATTRIBUTES, sizeof(u32), 0, &t); >> + if (ret) >> + return ret; >> + >> + put_unaligned_le32(cpuid, t->tx.buf); >> + ret = ph->xops->do_xfer(ph, t); >> + if (!ret) { >> + out = t->rx.buf; >> + strscpy(name, out->name, SCMI_SHORT_NAME_MAX_SIZE); >> + dev_info(ph->dev, "i.MX CPU: name: %s\n", name); >> + } else { >> + dev_err(ph->dev, "i.MX cpu: Failed to get info of cpu(%u)\n", cpuid); >> + } >> + >> + ph->xops->xfer_put(ph, t); >> + >> + return ret; >> +} >> + >> +static int scmi_imx_cpu_protocol_init(const struct scmi_protocol_handle *ph) >> +{ >> + struct scmi_imx_cpu_info *info; >> + u32 version; >> + int ret, i; >> + >> + ret = ph->xops->version_get(ph, &version); >> + if (ret) >> + return ret; >> + >> + dev_info(ph->dev, "NXP SM CPU Protocol Version %d.%d\n", >> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); >> + >> + info = devm_kzalloc(ph->dev, sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return -ENOMEM; >> + >> + ret = scmi_imx_cpu_protocol_attributes_get(ph, info); >> + if (ret) >> + return ret; >> + >> + for (i = 0; i < info->nr_cpu; i++) { >> + ret = scmi_imx_cpu_attributes_get(ph, i); >> + if (ret) >> + return ret; >> + } >> + >> + return ph->set_priv(ph, info, version); >> +} >> + >> +static const struct scmi_protocol scmi_imx_cpu = { >> + .id = SCMI_PROTOCOL_IMX_CPU, >> + .owner = THIS_MODULE, >> + .instance_init = &scmi_imx_cpu_protocol_init, >> + .ops = &scmi_imx_cpu_proto_ops, >> + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, >> + .vendor_id = SCMI_IMX_VENDOR, >> + .sub_vendor_id = SCMI_IMX_SUBVENDOR, >> +}; >> +module_scmi_protocol(scmi_imx_cpu); > >similarly as LMM... > >MODULE_ALIAS("scmi-protocol-" __stringify(SCMI_PROTOCOL_IMX_CPU) "-" SCMI_IMX_VENDOR); Fix in next version Appreciate again for reviewing the large patchset. Thanks, Peng > >Thanks >Cristian >
On Wed, Feb 12, 2025 at 03:40:27PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The i.MX95 System manager exports SCMI LMM protocol for linux to manage > Logical Machines. The driver is to use the LMM Protocol interface to > boot, shutdown a LM. > Hi, > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/imx/Kconfig | 11 +++++ > drivers/firmware/imx/Makefile | 1 + > drivers/firmware/imx/sm-lmm.c | 98 +++++++++++++++++++++++++++++++++++++++++ > include/linux/firmware/imx/sm.h | 33 ++++++++++++++ > 4 files changed, 143 insertions(+) > > diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig > index 907cd149c40a8b5f1b14c53e1c315ff4a28f32ac..c3e344d6ecc645df1f0e3ee8078934c47f347fd7 100644 > --- a/drivers/firmware/imx/Kconfig > +++ b/drivers/firmware/imx/Kconfig > @@ -23,6 +23,17 @@ config IMX_SCU > This driver manages the IPC interface between host CPU and the > SCU firmware running on M4. > > +config IMX_SCMI_LMM_DRV > + tristate "IMX SCMI LMM Protocol driver" > + depends on IMX_SCMI_LMM_EXT || COMPILE_TEST > + default y if ARCH_MXC > + help > + The System Controller Management Interface firmware (SCMI FW) is > + a low-level system function which runs on a dedicated Cortex-M > + core that could provide Logical Machine management features. > + > + This driver can also be built as a module. > + > config IMX_SCMI_MISC_DRV > tristate "IMX SCMI MISC Protocol driver" > default y if ARCH_MXC > diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile > index 8d046c341be878bb6dd1e6277992ff66ae90e292..7762855d2a771169d4f1867d27e0d51be7c9ad03 100644 > --- a/drivers/firmware/imx/Makefile > +++ b/drivers/firmware/imx/Makefile > @@ -2,3 +2,4 @@ > obj-$(CONFIG_IMX_DSP) += imx-dsp.o > obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o > obj-${CONFIG_IMX_SCMI_MISC_DRV} += sm-misc.o > +obj-${CONFIG_IMX_SCMI_LMM_DRV} += sm-lmm.o > diff --git a/drivers/firmware/imx/sm-lmm.c b/drivers/firmware/imx/sm-lmm.c > new file mode 100644 > index 0000000000000000000000000000000000000000..ed9650a8cb7ca878874e8609f0a5c83b5e46204a > --- /dev/null > +++ b/drivers/firmware/imx/sm-lmm.c > @@ -0,0 +1,98 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2025 NXP > + */ > + > +#include <linux/firmware/imx/sm.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/scmi_protocol.h> > +#include <linux/scmi_imx_protocol.h> > + > +static const struct scmi_imx_lmm_proto_ops *imx_lmm_ops; > +static struct scmi_protocol_handle *ph; > + > +int scmi_imx_lmm_boot(u32 lmid) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_lmm_ops->lmm_boot(ph, lmid); > +}; > +EXPORT_SYMBOL(scmi_imx_lmm_boot); ...cant you unify this... > + > +int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + if (!info) > + return -EINVAL; > + > + return imx_lmm_ops->lmm_info(ph, lmid, info); > +}; > +EXPORT_SYMBOL(scmi_imx_lmm_info); > + > +int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_lmm_ops->lmm_reset_vector_set(ph, lmid, cpuid, flags, vector); > +} > +EXPORT_SYMBOL(scmi_imx_lmm_reset_vector_set); > + > +int scmi_imx_lmm_power_on(u32 lmid) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_lmm_ops->lmm_power_on(ph, lmid); > +}; > +EXPORT_SYMBOL(scmi_imx_lmm_power_on); ...this... > + > +int scmi_imx_lmm_shutdown(u32 lmid, u32 flags) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_lmm_ops->lmm_shutdown(ph, lmid, flags); > +}; > +EXPORT_SYMBOL(scmi_imx_lmm_shutdown); > + ...and this into some common operation that accepts parameters to specifiy what to do...like (just making up name) scmi_imx_operation(lmid, flags, <OP>) ...so exporting only ONE symbol ? > +static int scmi_imx_lmm_probe(struct scmi_device *sdev) > +{ > + const struct scmi_handle *handle = sdev->handle; > + > + if (!handle) > + return -ENODEV; > + > + if (imx_lmm_ops) { > + dev_err(&sdev->dev, "lmm already initialized\n"); > + return -EEXIST; > + } > + > + imx_lmm_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_LMM, &ph); > + if (IS_ERR(imx_lmm_ops)) > + return PTR_ERR(imx_lmm_ops); > + > + return 0; > +} > + > +static const struct scmi_device_id scmi_id_table[] = { > + { SCMI_PROTOCOL_IMX_LMM, "imx-lmm" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(scmi, scmi_id_table); > + > +static struct scmi_driver scmi_imx_lmm_driver = { > + .name = "scmi-imx-lmm", > + .probe = scmi_imx_lmm_probe, > + .id_table = scmi_id_table, > +}; > +module_scmi_driver(scmi_imx_lmm_driver); > + > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_DESCRIPTION("IMX SM LMM driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h > index 9b85a3f028d1b0a5287b453eb3ad8412a363fe6c..e02b7b558afb6f430f6fbeeaf3ee1f59feea3c1b 100644 > --- a/include/linux/firmware/imx/sm.h > +++ b/include/linux/firmware/imx/sm.h > @@ -8,6 +8,7 @@ > > #include <linux/bitfield.h> > #include <linux/errno.h> > +#include <linux/scmi_imx_protocol.h> > #include <linux/types.h> > > #define SCMI_IMX_CTRL_PDM_CLK_SEL 0 /* AON PDM clock sel */ > @@ -20,4 +21,36 @@ > int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val); > int scmi_imx_misc_ctrl_set(u32 id, u32 val); > This down below seems fine to me (maybe simoplfied as stated above) and I guess we should see if we dont hit some weird Kconfig combination that breaks some builds on some combination of modules/builtin: in the past I vaguely remember Arnd posting the build error...and in fact the above scmi_imx_misc_ctrl_set/get have NO ifdeffery and aer handeld in that reversed way in which the protocol depends on the driver config IMX_SCMI_MISC_EXT tristate "i.MX SCMI MISC EXTENSION" depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) depends on IMX_SCMI_MISC_DRV default y if ARCH_MXC ...which weird but was needed by the build bots combinations....(I think)...so just a heads up that you could have to deal with that again... > +#if IS_ENABLED(CONFIG_IMX_SCMI_LMM_DRV) || IS_ENABLED(CONFIG_COMPILE_TEST) > +int scmi_imx_lmm_boot(u32 lmid); > +int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info); > +int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector); > +int scmi_imx_lmm_power_on(u32 lmid); > +int scmi_imx_lmm_shutdown(u32 lmid, u32 flags); > +#else > +static inline int scmi_imx_lmm_boot(u32 lmid) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_lmm_reset_vector_set(u32 lmid, u32 cpuid, u32 flags, u64 vector) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_lmm_power_on(u32 lmid) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_lmm_shutdown(u32 lmid, u32 flags) > +{ > + return -EOPNOTSUPP; > +} > +#endif > #endif > Other thean the above unification: Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
On Wed, Feb 12, 2025 at 03:40:28PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The i.MX95 System manager exports SCMI CPU protocol for linux to manage > cpu cores. The driver is to use the cpu Protocol interface to > start, stop a cpu cores (eg, M7). > Hi, > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/imx/Kconfig | 11 +++++ > drivers/firmware/imx/Makefile | 1 + > drivers/firmware/imx/sm-cpu.c | 91 +++++++++++++++++++++++++++++++++++++++++ > include/linux/firmware/imx/sm.h | 29 +++++++++++++ > 4 files changed, 132 insertions(+) > > diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig > index c3e344d6ecc645df1f0e3ee8078934c47f347fd7..91c753921dffbe16ced8c10565d44c15b66b2797 100644 > --- a/drivers/firmware/imx/Kconfig > +++ b/drivers/firmware/imx/Kconfig > @@ -23,6 +23,17 @@ config IMX_SCU > This driver manages the IPC interface between host CPU and the > SCU firmware running on M4. > > +config IMX_SCMI_CPU_DRV > + tristate "IMX SCMI CPU Protocol driver" > + depends on IMX_SCMI_CPU_EXT || COMPILE_TEST > + default y if ARCH_MXC > + help > + The System Controller Management Interface firmware (SCMI FW) is > + a low-level system function which runs on a dedicated Cortex-M > + core that could provide cpu management features. > + > + This driver can also be built as a module. > + > config IMX_SCMI_LMM_DRV > tristate "IMX SCMI LMM Protocol driver" > depends on IMX_SCMI_LMM_EXT || COMPILE_TEST > diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile > index 7762855d2a771169d4f1867d27e0d51be7c9ad03..3bbaffa6e3478112638ed031375602389f18ef09 100644 > --- a/drivers/firmware/imx/Makefile > +++ b/drivers/firmware/imx/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_IMX_DSP) += imx-dsp.o > obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o > +obj-${CONFIG_IMX_SCMI_CPU_DRV} += sm-cpu.o > obj-${CONFIG_IMX_SCMI_MISC_DRV} += sm-misc.o > obj-${CONFIG_IMX_SCMI_LMM_DRV} += sm-lmm.o > diff --git a/drivers/firmware/imx/sm-cpu.c b/drivers/firmware/imx/sm-cpu.c > new file mode 100644 > index 0000000000000000000000000000000000000000..1ce694a34b22843db5c1697ecb33c0479edb2ed9 > --- /dev/null > +++ b/drivers/firmware/imx/sm-cpu.c > @@ -0,0 +1,91 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2025 NXP > + */ > + > +#include <linux/firmware/imx/sm.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/scmi_protocol.h> > +#include <linux/scmi_imx_protocol.h> > + > +static const struct scmi_imx_cpu_proto_ops *imx_cpu_ops; > +static struct scmi_protocol_handle *ph; > + > +int scmi_imx_cpu_reset_vector_set(u32 cpuid, u64 vector, bool start, bool boot, > + bool resume) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_cpu_ops->cpu_reset_vector_set(ph, cpuid, vector, start, > + boot, resume); > +} > +EXPORT_SYMBOL(scmi_imx_cpu_reset_vector_set); > + > +int scmi_imx_cpu_start(u32 cpuid) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_cpu_ops->cpu_start(ph, cpuid); > +}; > +EXPORT_SYMBOL(scmi_imx_cpu_start); > + ...same as in LMM...this... > +int scmi_imx_cpu_started(u32 cpuid, bool *started) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + if (!started) > + return -EINVAL; > + > + return imx_cpu_ops->cpu_started(ph, cpuid, started); > +}; > +EXPORT_SYMBOL(scmi_imx_cpu_started); > + > +int scmi_imx_cpu_stop(u32 cpuid) > +{ > + if (!ph) > + return -EPROBE_DEFER; > + > + return imx_cpu_ops->cpu_stop(ph, cpuid); > +}; > +EXPORT_SYMBOL(scmi_imx_cpu_stop); > + ...and this can be just one with ONLY one symbol exported... > +static int scmi_imx_cpu_probe(struct scmi_device *sdev) > +{ > + const struct scmi_handle *handle = sdev->handle; > + > + if (!handle) > + return -ENODEV; > + > + if (imx_cpu_ops) { > + dev_err(&sdev->dev, "sm cpu already initialized\n"); > + return -EEXIST; > + } > + > + imx_cpu_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_CPU, &ph); > + if (IS_ERR(imx_cpu_ops)) > + return PTR_ERR(imx_cpu_ops); > + > + return 0; > +} > + > +static const struct scmi_device_id scmi_id_table[] = { > + { SCMI_PROTOCOL_IMX_CPU, "imx-cpu" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(scmi, scmi_id_table); > + > +static struct scmi_driver scmi_imx_cpu_driver = { > + .name = "scmi-imx-cpu", > + .probe = scmi_imx_cpu_probe, > + .id_table = scmi_id_table, > +}; > +module_scmi_driver(scmi_imx_cpu_driver); > + > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_DESCRIPTION("IMX SM CPU driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h > index e02b7b558afb6f430f6fbeeaf3ee1f59feea3c1b..0eff427e5ba2cb3f93c26f7d055c346a1d1433f0 100644 > --- a/include/linux/firmware/imx/sm.h > +++ b/include/linux/firmware/imx/sm.h > @@ -21,6 +21,35 @@ > int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val); > int scmi_imx_misc_ctrl_set(u32 id, u32 val); > Same kind of heads up here too.. > +#if IS_ENABLED(CONFIG_IMX_SCMI_CPU_DRV) || IS_ENABLED(CONFIG_COMPILE_TEST) > +int scmi_imx_cpu_start(u32 cpuid); > +int scmi_imx_cpu_started(u32 cpuid, bool *started); > +int scmi_imx_cpu_stop(u32 cpuid); > +int scmi_imx_cpu_reset_vector_set(u32 cpuid, u64 vector, bool start, bool boot, > + bool resume); > +#else > +static inline int scmi_imx_cpu_start(u32 cpuid) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_cpu_started(u32 cpuid, bool *started) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_cpu_stop(u32 cpuid) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int scmi_imx_cpu_reset_vector_set(u32 cpuid, u64 vector, > + bool start, bool boot, bool resume) > +{ > + return -EOPNOTSUPP; > +} > +#endif > + > #if IS_ENABLED(CONFIG_IMX_SCMI_LMM_DRV) || IS_ENABLED(CONFIG_COMPILE_TEST) > int scmi_imx_lmm_boot(u32 lmid); > int scmi_imx_lmm_info(u32 lmid, struct scmi_imx_lmm_info *info); > Other than this: Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
On Tue, Feb 25, 2025 at 05:07:09PM +0800, Peng Fan wrote: > Hi Cristian, > > On Mon, Feb 24, 2025 at 12:28:34PM +0000, Cristian Marussi wrote: > >On Wed, Feb 12, 2025 at 03:40:23PM +0800, Peng Fan (OSS) wrote: > >> From: Peng Fan <peng.fan@nxp.com> > >> > >> Add i.MX95 Logical Machine Management and CPU Protocol documentation. > >> > > > >Hi, > > > >a few comments below. > > > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> > >> --- > >> drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 737 ++++++++++++++++++++++++ > >> 1 file changed, 737 insertions(+) > >> > >> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > >> index b2dfd6c46ca2f5f12f0475c24cb54c060e9fa421..78a09cd8102becd5584d28bdef18df2d77fb7e7c 100644 > >> --- a/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > >> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx95.rst > >> @@ -32,6 +32,461 @@ port, and deploy the SM on supported processors. > >> The SM implements an interface compliant with the Arm SCMI Specification > >> with additional vendor specific extensions. > >> > > > >In general I noticed that there is NOT specified anywhere if all the > >following commands are mandatory or optional in your protocol: would be > >better to be specified, as it is I will assume that are all mandatory. > > The Doc from [1] does not specify optional or mandatory, I need check > with firmware owner to see whether this could be added. > > [1]https://github.com/nxp-imx/imx-sm > Ok...just specify for each command what is the expectation. > > > >> +SCMI_LMM: System Control and Management Logical Machine Management Vendor Protocol > >> +================================================================================== > >> + > >> + > [...] > >> +PROTOCOL_ATTRIBUTES > >> +~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x1 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|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 | > > > >...so this states how many LMs you have...ok... > > > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +PROTOCOL_MESSAGE_ATTRIBUTES > >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x2 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|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 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|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 | > +------------------+-----------------------------------------------------------+ > ok > > > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_BOOT > >> +~~~~~~~~ > >> + > >> +message_id: 0x4 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully booted. | > > > >...this and other commands below that cause a transition are a bit > >ambiguos in their description IMHO...does this mean that on reception of > >a SUCCESS the LM identified by lmid has successfully completed the boot ? > > > >...or, as I suppose, this being an immmediate command, SUCCESS means something > >like 'boot succesfully started', NOT that when this SUCCESS reply comes back the > >LM has booted...also becuse I can see a lot of notifications defined down below to > >signal the completion of such operations... > > > >..if this is the case please clarify... > > right. This means the LM has been boot successfully started. Not means > LM fully function up. > > I will updated this with "SUCCESS: if LM boots successfully started" > ok > > > > >...if this is NOT the case and this is intended to return synchronoulsy > >when the required operation has completed, even though such machines are > >maybe (?) minimal/tiny compute systems, can you guarantee that they can boot/shutdown > >etc in a reasonably short time not to hog the channel for too long (I mean > >well before the configured transport timeout)... > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_RESET > >> +~~~~~~~~~ > >> + > >> +message_id: 0x5 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 flags |Reset flags: | > >> +| |Bits[31:1] Reserved, must be zero. | > >> +| |Bit[0] Graceful request: | > >> +| |Set to 1 if the request is a graceful request. | > >> +| |Set to 0 if the request is a forceful request. | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully resets. | > > > >Same ... does this simply mean LM reset issued successfully ? > > For graceful reset, platform will issue a notification to agent and just return, > agent will do the reset, as linux reboot. > For forceful reset, platform will do the force reset of agent, this is longer > time. > > I will update with "The LMM RESET command finished successfully in graceful reset > or LM successfully resets in forceful reset" > ok > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_SHUTDOWN > >> +~~~~~~~~~~~~ > >> + > >> +message_id: 0x6 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 flags |Reset flags: | > >> +| |Bits[31:1] Reserved, must be zero. | > >> +| |Bit[0] Graceful request: | > >> +| |Set to 1 if the request is a graceful request. | > >> +| |Set to 0 if the request is a forceful request. | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully shutdowns. | > > > >Same...I suppose the shutdown has been sucessfully issued and could be > >still in progress...if this is not the case I fear this immediate > >command will potentially hog the channel for too long.... > > For graceful shutdown, platform will issue a notification to agent and just return, > agent will do shutdown. > For forceful shutdown, platform will do the force shutdown of agent, this is longer > time. > > I will update with "The LMM shutdown command finished successfully in graceful request > or LM successfully shutdown in forceful request" > Yes please, in general for all this cases you should clearly state if the action is expected to be completed sunchronously with the immediate response of asynchronously and signaled by an optional notification... ..I think it is important to clarify the expectations since you really do NOT use SCMI async commands (with DELAYED RESPONSES) but a mix of sync immediate commands and later notification on completion (AFAIU)...so better to be clear all over this commnds.. > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_WAKE > >> +~~~~~~~~ > >> + > >> +message_id: 0x7 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully wakes. | > > > >Same...I suppose you can know when an LM is fully woken only from the > >dedicated notification receipt... > > I will updated this with "SUCCESS: if LM wake command successfully returns" > ok > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_SUSPEND > >> +~~~~~~~~~~~ > >> + > >> +message_id: 0x8 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully suspends. | > > > >Same..I suppose the SCMI server will know when the suspend for this LM is > >effective and will send a notification, so this is again only an indication > >that the suspend process has started... > > Right. > I will updated this with "SUCCESS: if LM suspend command successfully returns" > ok > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > > > >In general, you could also think to make the bahvour of all this > >operation configurable like teh RESET protocol does...so you could ask > >for an LMM op to complete synchronously OR instead let it happen > >asynchnously in the background after the command-request has completed > >and then wait for one of the existing notification... > > > >...again if this is already the expected behaviour please clarifiy the sync/async > >kind of execution that is expected when you issue such commands > > Yes, for graceful, it is async; for forceful, it is sync. > If command not has graceful/forceful, such as lm wake/suspend, it is async > Yes please specify this sync vs async completion as specified above and the expected optional notifs > > > >> +LMM_NOTIFY > >> +~~~~~~~~~~ > >> + > >> +message_id: 0x9 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 flags |Notification flags: | > >> +| |Bits[31:3] Reserved, must be zero. | > >> +| |Bit[3] Wake (resume) notification: | > >> +| |Set to 1 to send notification. | > >> +| |Set to 0 if no notification. | > >> +| |Bit[2] Suspend (sleep) notification: | > >> +| |Set to 1 to send notification. | > >> +| |Set to 0 if no notification. | > >> +| |Bit[1] Shutdown (off) notification: | > >> +| |Set to 1 to send notification. | > >> +| |Set to 0 if no notification. | > >> +| |Bit[0] Boot (on) notification: | > >> +| |Set to 1 to send notification. | > >> +| |Set to 0 if no notification | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the notification state successfully updated. | > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if input attributes flag specifies | > >> +| |unsupported or invalid configurations. | > >> +| |DENIED: if the agent does not have permission to request | > >> +| |the notification. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_RESET_REASON > >> +~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0xA > >> +protocol_id: 0x80 > > > >What is this...a bit of explanation would be fine given that this > >command is not straightforward to understand like the previous > >boot/suspend etc... > > This function returns the boot/shutdown/reset reason for an LM. > > Such as POR, WDOG and etc. > > > > >> + > >> ++---------------------+--------------------------------------------------------+ > >> +|Parameters | > >> ++---------------------+--------------------------------------------------------+ > >> +|Name |Description | > >> ++---------------------+--------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++---------------------+--------------------------------------------------------+ > >> +|Return values | > >> ++---------------------+--------------------------------------------------------+ > >> +|Name |Description | > >> ++---------------------+--------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully suspends. | > > > >? typo ? > > Sorry. This should be "the reset reason of the LM successfully updated" > > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine| > >> +| |DENIED: if the agent does not have permission to request| > >> +| |the reset reason. | > >> ++---------------------+--------------------------------------------------------+ > >> +|uint32 bootflags |Boot reason flags. This parameter has the format: | > >> +| |Bits[31] Valid. | > >> +| |Set to 1 if the entire reason is valid. | > >> +| |Set to 0 if the entire reason is not valid. | > >> +| |Bits[30:29] Reserved, must be zero. | > >> +| |Bit[28] Valid origin: | > >> +| |Set to 1 if the origin field is valid. | > >> +| |Set to 0 if the origin field is not valid. | > >> +| |Bits[27:24] Origin. | > > > >Are this origin values defined anywhere ? > > It varies in the current imx-sm implementation, there is no spec > defining this, it could be a LMID or BBM register value or else. > O_o ... ok... always a bit concerned about opaque stuff...not strong opinion as of now... > > > >> +| |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. | > > > >And this errors ID ? which value should I expect here...and to what this > >errors refer to... > > > >> +| |Bits[22:8] Error ID. | > > There is not doc defining this. It just varies, > It could be BBM button, agentID, faultID or a peripheral ID or else. ok > > >> +| |Bit[7:0] Reason(WDOG, POR, FCCU and etc) | > >> ++---------------------+--------------------------------------------------------+ > >> +|uint32 shutdownflags |Shutdown reason flags. This parameter has the format: | > >> +| |Bits[31] Valid. | > >> +| |Set to 1 if the entire reason is valid. | > >> +| |Set to 0 if the entire reason is not valid. | > >> +| |Bits[30:29] Number of valid extended info words. | > >> +| |Bit[28] Valid origin: | > >> +| |Set to 1 if the origin field is valid. | > >> +| |Set to 0 if the origin field is not valid. | > > > >Ditto. > >> +| |Bits[27:24] Origin. | > >> +| |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. | > >Ditto. > >> +| |Bit[7:0] Reason | > >Ditto. > > I will check imx-sm code and add more info for upper all three, > but there is no documentation and it varies in imx-sm code repo. > ok > >> ++---------------------+--------------------------------------------------------+ > >> +|uint32 extinfo[0,20] |Array of extended info words(e.g. fault pc) | > >> ++---------------------+--------------------------------------------------------+ > > > >..so what does this field represent....is this a fixed sized array ? seems more likely > >to be dynamically sized in range [0,20] AFAICU... > > This should be updated with "extinfo[3]". The length has been speicified in > "Bits[30:29] Number of valid extended info words." Ah ok ... much better thanks for clarifying > > > > >...now...if it is dynamically sized: > > > >+ there should be a size field declaring the size of the returned > >answer, if not you should rely on the transport size to know where the > >message ends (unless if hard-coded based on the above flags...in that > >case explain and document) > > already specified in shutdownflags BITS[30:29] > ok > > > >+ you cannot assume that such a potentially big reply reply fit into a > >single message due to possible limitations of the transport > >sizes...IOW you should think about a 'multi-part' message like many > >exist in SCMI where the reply reports returned+remaining entries... > >..how can yo be sure to fit in any transport...this message has > >currently a potential max payload reply of 96 bytes ... > > > >> + > >> +LMM_POWER_ON > >> +~~~~~~~~~~~~ > >> + > >> +message_id: 0xB > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully powers on. | > > > >Beside the same consideration as above regarding what this SUCCESS > >mean...what is meant to be the difference between LMM_POWER_ON and LMM_BOOT ? > > POWER ON, it is just power up, not means the LMM is kicked out to boot. > BOOT, means LMM is kicked out to boot > > For example, CPU has power and reset release function. > power on, means power up the CPU > boot, means reset release to let CPU run. yep, got that, but I was just wondering what was the purpose to have this distinction from the SCMI agent perspective > > Especially for Cortex-M core, the TCM is in same power domain > as Cortex-M. before booting the core, need power up the core to loading > images to TCM, then boot the core. > but it makes sense now a bit more....thanks for clarifying... > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine. | > >> +| |INVALID_PARAMETERS: if lmId is same as the caller. | > >> +| |DENIED: if the agent does not have permission to manage the| > >> +| |the LM specified by lmid. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +LMM_RESET_VECTOR_SET > >> +~~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0xC > >> +protocol_id: 0x80 > >> + > >> ++-----------------------+------------------------------------------------------+ > >> +|Parameters | > >> ++-----------------------+------------------------------------------------------+ > >> +|Name |Description | > >> ++-----------------------+------------------------------------------------------+ > >> +|uint32 lmid |ID of the Logical Machine | > >> ++-----------------------+------------------------------------------------------+ > >> +|uint32 cpuid |ID of the CPU inside the LM | > >> ++-----------------------+------------------------------------------------------+ > >> +|uint32 flags |Reset vector flags | > >> +| |Bits[31:1] Reserved, must be zero. | > >> +| |Bit[0] Table flag. | > >> +| |Set to 1 if vector is the vector table base address | > > > >So what is this when Bit[0] is set to zero instead ? > > Per current imx-sm, this flags is not used as of now, just a reserved > flag for future extension. So 0 or 1 does not matter here. > I will update with Bits[31:0] Reserved. ok > > > > >> ++-----------------------+------------------------------------------------------+ > >> +|uint32 resetVectorLow |Lower vector | > >> ++-----------------------+------------------------------------------------------+ > >> +|uint32 resetVectorHigh |Higher vector | > >> ++-----------------------+------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if LM successfully powers on. | > > > >...typo ?... > > "If reset vector is set successfully" > > > > >> +| |NOT_FOUND: if lmId not points to a valid logical machine, | > >> +| |or cpuId is not valid. > >> +| |INVALID_PARAMETERS: if reset vector is invalid. | > >> +| |DENIED: if the agent does not have permission to set the | > >> +| |the reset vector for the CPU in the LM. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +NEGOTIATE_PROTOCOL_VERSION > >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x10 > >> +protocol_id: 0x80 > >> + > >> ++--------------------+---------------------------------------------------------+ > >> +|Parameters | > >> ++--------------------+---------------------------------------------------------+ > >> +|Name |Description | > >> ++--------------------+---------------------------------------------------------+ > >> +|uint32 version |The negotiated protocol version the agent intends to use | > >> ++--------------------+---------------------------------------------------------+ > >> +|Return values | > >> ++--------------------+---------------------------------------------------------+ > >> +|Name |Description | > >> ++--------------------+---------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the negotiated protocol version is supported | > >> +| |by the platform. All commands, responses, and | > >> +| |notifications post successful return of this command must| > >> +| |comply with the negotiated version. | > >> +| |NOT_SUPPORTED: if the protocol version is not supported. | > >> ++--------------------+---------------------------------------------------------+ > >> + > >> +Notifications > >> +_____________ > >> + > >> +LMM_EVENT > >> +~~~~~~~~~ > >> + > >> +message_id: 0x0 > >> +protocol_id: 0x80 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 lmid |Identifier for the LM that caused the transition. | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 eventlm |Identifier for the LM the event is for. | > > > >So this is the LM that the event refer to ? i.e. this LM eventlm is the > >one that is booting/resetting/shutdown ? > >if this is the case, maybe better as: > > > > "Identifier of the LM this event refers to." > > Thanks, will update as you suggest. > ok > > > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 flags |LM events: | > >> +| |Bits[31:3] Reserved, must be zero. | > >> +| |Bit[3] Wake (resume) event: | > >> +| |1 LM has awakened. | > >> +| |0 not a wake event. | > >> +| |Bit[2] Suspend (sleep) event: | > >> +| |1 LM has suspended. | > >> +| |0 not a suspend event. | > >> +| |Bit[1] Shutdown (off) event: | > >> +| |1 LM has shutdown. | > >> +| |0 not a shutdown event. | > >> +| |Bit[0] Boot (on) event: | > >> +| |1 LM has booted. | > >> +| |0 not a boot event. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> SCMI_BBM: System Control and Management BBM Vendor Protocol > >> ============================================================== > >> > >> @@ -436,6 +891,288 @@ protocol_id: 0x81 > >> | |0 no button change detected. | > >> +------------------+-----------------------------------------------------------+ > >> > >> +SCMI_CPU: System Control and Management CPU Vendor Protocol > >> +============================================================== > >> + > >> +This protocol allows an agent to start or stop a CPU. It is used to manage > >> +auxiliary CPUs in an LM (e.g. additional cores in an AP cluster or > >> +Cortex-M cores). > >> +Note: For cores in AP cluster, ATF will use CPU protocol to handle them. > > > >So this is sort of a PSCI-by-proxy in which you ask the server to turn > >on/off some of the LM CPUs ? > > Yes. A55 cores are in one LM, so in ATF one A55 use cpu protocol to manage other > A55 cores. > > > > >First question would be is not enough to just ask for a specific LM to > >be booted with LMM ? or add to the LMM protocol a way to specifiy the > > LMM is a large scope, CPU could be inside LMM. > > >'level' of boot that you ask for an LM...I may miss many details but it > >seems to me a bit odd that you are allowd to ask, from the agent that > >could have just requested an LM to boot to have some CPUs ON or OFF... > >...and specify the boot/resume address vectors here to use when turning > >ON... > > There is case the M7 and A55 are in the same LM, so in such case, if A55 core > needs to manage M7, need use CPU protocol. > > If A55 and M7 are in two separate LMs, A55 needs use LMM protocol to manage > M7 LM. > this is a sort of remoteproc-like via SCMI > > > >Maybe a bit more of explanation of the rationale and expectations here > >could help understanding the differet needs from LMM > > There are some internal discussion between NXP and ARM if you search > your mail "NXP-ARM SCMI OEM extension". > > I could not post them all here. > ok > > > >> + > >> +The CPU protocol provides functions to: > >> + > >> +- Describe the protocol version. > >> +- Discover implementation attributes. > >> +- Discover the CPUs defined in the system. > >> +- Start a CPU. > >> +- Stop a CPU. > >> +- Set the boot and resume addresses for a CPU. > >> +- Set the sleep mode of a CPU. > >> +- Configure wake-up sources for a CPU. > >> +- Configure power domain reactions (LPM mode and retention mask) for a CPU. > >> +- The CPU IDs can be found in the CPU section of the SoC DEVICE: SM Device > >> + Interface. They can also be found in the SoC RM. See the CPU Mode Control > >> + (CMC) list in General Power Controller (GPC) section. > >> + > >> +CPU settings are not aggregated and setting their state is normally exclusive > >> +to one client. > >> + > >> +Commands: > >> +_________ > >> + > >> +PROTOCOL_VERSION > >> +~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x0 > >> +protocol_id: 0x82 > >> + > >> ++---------------+--------------------------------------------------------------+ > >> +|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: 0x82 > >> + > >> ++---------------+--------------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status | See ARM SCMI Specification for status code definitions. | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 attributes |Protocol attributes: | > >> +| |Bits[31:16] Reserved, must be zero. | > >> +| |Bits[15:0] Number of CPUs | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +PROTOCOL_MESSAGE_ATTRIBUTES > >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x2 > >> +protocol_id: 0x82 > >> + > >> ++---------------+--------------------------------------------------------------+ > >> +|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 | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +CPU_ATTRIBUTES > >> +~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x4 > >> +protocol_id: 0x82 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if valid attributes are returned successfully. | > >> +| |NOT_FOUND: if the cpuid is not valid. | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 attributes |Bits[31:0] Reserved, must be zero | > >> ++------------------+-----------------------------------------------------------+ > >> +|char name[16] |NULL terminated ASCII string with CPU name up to 16 bytes | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +CPU_START > >> +~~~~~~~~~ > >> + > >> +message_id: 0x4 > >> +protocol_id: 0x82 > >> + > > > >Any constraint to call this only after having successfully called RESET_VECROR_SET ? > > No constraint, if not called RESET_VECROR_SET, it will use default value 0. > > > > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the cpu is started successfully. | > >> +| |NOT_FOUND: if cpuid is not valid. | > >> +| |DENIED: the calling agent is not allowed to start this CPU.| > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +CPU_STOP > >> +~~~~~~~~ > >> + > >> +message_id: 0x5 > >> +protocol_id: 0x82 > >> + > >> ++------------------+-----------------------------------------------------------+ > >> +|Parameters | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++------------------+-----------------------------------------------------------+ > >> +|Return values | > >> ++------------------+-----------------------------------------------------------+ > >> +|Name |Description | > >> ++------------------+-----------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the cpu is started successfully. | > >> +| |NOT_FOUND: if cpuid is not valid. | > >> +| |DENIED: the calling agent is not allowed to stop this CPU. | > >> ++------------------+-----------------------------------------------------------+ > >> + > >> +CPU_RESET_VECTOR_SET > >> +~~~~~~~~~~~~~~~~~~~~ > >> + > >> +message_id: 0x6 > >> +protocol_id: 0x82 > >> + > >> ++----------------------+-------------------------------------------------------+ > >> +|Parameters | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 flags |Reset vector flags: | > >> +| |Bit[31] Resume flag. | > >> +| |Set to 1 to update the reset vector used on resume. | > >> +| |Bit[30] Boot flag. | > >> +| |Set to 1 to update the reset vector used for boot. | > >> +| |Bits[29:1] Reserved, must be zero. | > >> +| |Bit[0] Table flag. | > >> +| |Set to 1 if vector is the vector table base address. | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 resetVectorLow |Lower vector: | > >> +| |If bit[0] of flags is 0, the lower 32 bits of the | > >> +| |physical address where the CPU should execute from on | > >> +| |reset. If bit[0] of flags is 1, the lower 32 bits of | > >> +| |the vector table base address | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 resetVectorhigh|Upper vector: | > >> +| |If bit[0] of flags is 0, the upper 32 bits of the | > >> +| |physical address where the CPU should execute from on | > >> +| |reset. If bit[0] of flags is 1, the upper 32 bits of | > >> +| |the vector table base address | > >> ++----------------------+-------------------------------------------------------+ > >> +|Return values | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the CPU reset vector is set successfully. | > >> +| |NOT_FOUND: if cpuId does not point to a valid CPU. | > >> +| |INVALID_PARAMETERS: the requested vector type is not | > >> +| |supported by this CPU. | > >> +| |DENIED: the calling agent is not allowed to set the | > >> +| |reset vector of this CPU | > >> ++----------------------+-------------------------------------------------------+ > >> + > >> +CPU_SLEEP_MODE_SET > >> +~~~~~~~~~~~~~~~~~~ > >> +message_id: 0x7 > >> +protocol_id: 0x82 > >> + > >> ++----------------------+-------------------------------------------------------+ > >> +|Parameters | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 flags |Sleep mode flags: | > >> +| |Bits[31:1] Reserved, must be zero. | > >> +| |Bit[0] IRQ mux: | > >> +| |If set to 1 the wakeup mux source is the GIC, else if 0| > >> +| |then the GPC | > > > >What is the GPC in this context ? > > General Power Controller. > > > > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 sleepmode |target sleep mode | > > > >What values can this assume ? Is there any predefined sleep value here ? > > From imx-sm, the define is hardware related. I could list in next version. > > #define CPU_SLEEP_MODE_RUN 0U > #define CPU_SLEEP_MODE_WAIT 1U > #define CPU_SLEEP_MODE_STOP 2U > #define CPU_SLEEP_MODE_SUSPEND 3U > > RUN, WAIT, STOP, SUSPEND is GPC concept. it means when wfi, the GPC will > do what action. > > > > >> ++----------------------+-------------------------------------------------------+ > >> +|Return values | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|int32 status |SUCCESS: if the CPU sleep mode is set successfully. | > >> +| |NOT_FOUND: if cpuId does not point to a valid CPU. | > >> +| |INVALID_PARAMETERS: the sleepmode or flags is invalid. | > >> +| |DENIED: the calling agent is not allowed to configure | > >> +| |the CPU | > >> ++----------------------+-------------------------------------------------------+ > >> + > >> +CPU_INFO_GET > >> +~~~~~~~~~~~~ > >> +message_id: 0xC > >> +protocol_id: 0x82 > >> + > >> ++----------------------+-------------------------------------------------------+ > >> +|Parameters | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 cpuid |Identifier for the CPU | > >> ++----------------------+-------------------------------------------------------+ > >> +|Return values | > >> ++----------------------+-------------------------------------------------------+ > >> +|Name |Description | > >> ++----------------------+-------------------------------------------------------+ > >> +|int32 status |SUCCESS: if valid attributes are returned successfully.| > >> +| |NOT_FOUND: if the cpuid is not valid. | > >> ++----------------------+-------------------------------------------------------+ > >> +|uint32 runmode |Run mode for the CPU | > > > >What are the possible runmode values ? > > #define CPU_RUN_MODE_START 0U // start cpu > #define CPU_RUN_MODE_HOLD 1U // power up and hold cpu > #define CPU_RUN_MODE_STOP 2U // reset and hold cpu > #define CPU_RUN_MODE_SLEEP 3U // in cpuidle > > Appreciate for detailed reviewing on the documentation. > Thanks, Cristian
i.MX95 System Manager(SM) implements Logical Machine Management(LMM) and CPU protocol to manage Logical Machine(LM) and CPUs(eg, M7). To manage M7 in a separate LM or in same LM as Linux itself. LMM APIs and CPU APIs are needed. When M7 is in LM 'lm-m7', and this LM is managable by 'linux-lm', linux could use LMM_BOOT, LMM_SHUTDOWN and etc to manage 'lm-m7'. If in same LM, use CPU_START, CPU_STOP, CPU_RESET_VECTOR_SET and etc to manage M7. Both LMM/CPU APIs will be used by remoteproc driver. The remoteproc patchset will be posted out after this patchset gets reviewed or in good shape per Maitainer's view. Build pass with COMPILE_TEST Tested with remoteproc on i.MX95 Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Changes in v2: - Add dt-bindings patch 2 - Add a maintainer entry with patch 7 - Update doc to address various questions and make it clear - Use strscpy to use scmi server returned string - Drop extra blank line - Shrink scmi_imx_cpu_attributes_get function args. - Typo fixes - Add LMM_RESET_VECTOR_SET - Link to v1: https://lore.kernel.org/r/20250121-imx-lmm-cpu-v1-0-0eab7e073e4e@nxp.com --- Peng Fan (7): firmware: arm_scmi: imx: Add LMM and CPU documentation dt-bindings: firmware: Add i.MX95 SCMI LMM and CPU protocol firmware: arm_scmi: imx: Add i.MX95 LMM protocol firmware: arm_scmi: imx: Add i.MX95 CPU Protocol firmware: imx: Add i.MX95 SCMI LMM driver firmware: imx: Add i.MX95 SCMI CPU driver MAINTAINERS: add entry for i.MX SCMI extensions .../bindings/firmware/nxp,imx95-scmi.yaml | 16 + MAINTAINERS | 9 + drivers/firmware/arm_scmi/vendors/imx/Kconfig | 22 + drivers/firmware/arm_scmi/vendors/imx/Makefile | 2 + drivers/firmware/arm_scmi/vendors/imx/imx-sm-cpu.c | 287 ++++++++ drivers/firmware/arm_scmi/vendors/imx/imx-sm-lmm.c | 270 ++++++++ drivers/firmware/arm_scmi/vendors/imx/imx95.rst | 737 +++++++++++++++++++++ drivers/firmware/imx/Kconfig | 22 + drivers/firmware/imx/Makefile | 2 + drivers/firmware/imx/sm-cpu.c | 91 +++ drivers/firmware/imx/sm-lmm.c | 98 +++ include/linux/firmware/imx/sm.h | 62 ++ include/linux/scmi_imx_protocol.h | 41 ++ 13 files changed, 1659 insertions(+) --- base-commit: c4661e026ccea82ebec71e4b9013c9a4f9f4e232 change-id: 20250120-imx-lmm-cpu-418daaa257e2 Best regards,