drivers: firmware: scmi: add optional crc #86347

AndreHeinemans-NXP · 2025-02-26T09:55:27Z

An option is added to validate incomming and outgoing SCMI messages on shared memory.

This is needed especially for the imx95_evk/mimx9596/m7 which is used in combination with imx-sm on the M33 core. The imx-sm software uses CRC on SCMI by default. It will be easier to start with this target because it works with the sdcard image that comes with the evk.

github-actions · 2025-02-26T09:56:12Z

Hello @AndreHeinemans-NXP, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

drivers/firmware/scmi/Kconfig

dbaluta · 2025-02-26T14:27:27Z

boards/nxp/imx95_evk/imx95_evk_mimx9596_m7_ddr_defconfig

@@ -17,6 +17,7 @@ CONFIG_CONSOLE=y
 CONFIG_MBOX=y
 CONFIG_MBOX_INIT_PRIORITY=0
 CONFIG_ARM_SCMI=y
+CONFIG_ARM_SCMI_SHMEM_USE_CRC=y



This will enable mandatory CRC validation for all applications on imx95_m7 using ddr config. So this will break any application which doesn't encode the CRC.

e.g sound open firmware: https://github.com/thesofproject/sof/blob/main/app/boards/imx95_evk_mimx9596_m7_ddr.conf

I don't think we should add this kind of checks in the scmi core itself.

By default the imx-sm does enable CRC, hence this PR to make Zephyr actually work with the "Hello world" example.
See imx-sm source
https://github.com/nxp-imx/imx-sm/blob/a07928b4a69e092667c3c2dc61adc36b5eaaf836/configs/mx95evk.cfg#L335

It is not mandatory since you can override this setting in the application. All applications who do not need it only have to add 'CONFIG_ARM_SCMI_SHMEM_USE_CRC=n' to their config.

The reason that I enabled it by default on the imx95_evk is that system-manager (imx-sm) also has it enabled by default.

That's unreasonable to ask applications to know about CONFIG_ARM_SCMI_SHMEM_USE_CRC and disable it.
I think your particular application should explicitly enable it.

AFAIK I understand your first end goal is to make default "Hello World" sample to work on M7 with the default imx-sm config.

Looks like we have a conflict here. Allow me some time to better understand the context and then decide on a solution that works for everyone.

PRs are blocked for 4.2 so we do have some time until we can merge this.

LaurentiuM1234

Enabling CRC by default in Zephyr effectively breaks all upstream applications counting on the fact that it's disabled (e.g: SOF, CC: @yangbolu1991 this might affect you as well). Apart from that, you're enabling a feature which you arguably don't need for your application.

IMO this can be solved by updating the documentation (i.e: https://docs.zephyrproject.org/latest/boards/nxp/imx95_evk/doc/index.html) to tell people which configuration requires CRC and which doesn't. I can just as fine run "Hello, world" using ALT config without changing anything on Zephyr-side.

LaurentiuM1234 · 2025-02-26T15:06:07Z

drivers/firmware/scmi/shmem.c

@@ -96,6 +97,14 @@ int scmi_shmem_read_message(const struct device *shmem, struct scmi_message *msg
 		return -EINVAL;
 	}

+#ifdef CONFIG_ARM_SCMI_SHMEM_USE_CRC
+	/* crc match? */
+	if (layout->res1[1] != crc32_ieee((const uint8_t *)&layout->msg_hdr, layout->len)) {


Please don't introduce vendor-specific processing inside the SCMI core. If you need to do some additional processing inside the shared memory read/write functions I'd suggest adding 2 new functions: scmi_shmem_vendor_write_message() and scmi_shmem_vendor_read_message() and calling them from the core. If the vendor needs extra processing they should implement those functions in drivers/firmware/scmi/vendor/<vendor_name>/shmem.c.

Just saying but it was never working to begin with.......

Does this even get actively tested with CI at all?

Also recompiling imx-sm is a bit of pain anyhow.

@LaurentiuM1234
I added the nxp specific code in the driver/firmware/scmi/nxp/shmem.c file which only contains the functions scmi_shmem_vendor_validate_message() and scmi_shmem_vendor_prepare_message(). This file will only be built when CONFIG_ARM_SCMI_SHMEM_VENDOR_NXP is configured and will override the existing empty functions.

yangbolu1991 · 2025-02-27T07:39:10Z

For i.mx95, crc was disabled in mx95alt.cfg, which used for m7 tests.
nxp-imx/imx-sm@4d53d16
We had also uploaded imx95 boot firmwares to nxp.com. We are supporting spsdk runner to support west build for bootable firmware. See,
#80507

But I'm not sure if there are any cases requiring CRC. If so, I think it should be documented.
Thanks.

dbaluta · 2025-02-28T07:55:05Z

For i.mx95, crc was disabled in mx95alt.cfg, which used for m7 tests. nxp-imx/imx-sm@4d53d16 We had also uploaded imx95 boot firmwares to nxp.com. We are supporting spsdk runner to support west build for bootable firmware. See, #80507

But I'm not sure if there are any cases requiring CRC. If so, I think it should be documented. Thanks.

@AndreHeinemans-NXP @PetervdPerk-NXP Would this work for you? Looks like CRC is disabled with newer configurations.

AndreHeinemans-NXP · 2025-02-28T10:19:04Z

@dbaluta, @LaurentiuM1234 ,
I changed the default to no CRC by disabling CONFIG_ARM_SCMI_SHMEM_VENDOR_NXP. And also added a note in the rst so users know about it and can enable it if needed.

dbaluta · 2025-03-03T11:30:11Z

In principle, I agree with this PR. It is clean enough to not touch the SCMI core and by default the check is disabled and only activated in specific scenarios.

Will need to get an Ack from @LaurentiuM1234 and we can merge it.

bperseghetti · 2025-03-03T16:05:21Z

Thanks this patch allows me to use Zephyr on the IMX95-EVK, I tried before but couldn't even get hello world to show up till this patch.

JiafeiPan

Actually NXP uses two of the SCMI SMT “implementation defined” words, it is res1[2] in the code, the first one holds the status that indicate if CRC is being used and which algorithm is used, and the second one is the CRC. so we need to check first word firstly and then decide use CRC or not, and also decide which CRC algorithm is used. so that the driver will be compatible for all SM firmware with CRC enabled or not.

LaurentiuM1234

as @JiafeiPan pointed out, res1[0] holds the CRC type (algorithm) used and if CRC is enabled. We can use this to dynamically check if CRC is enabled/disabled and which algorithm to use. This way, we can have the CRC NXP vendor extension enabled by default, thus allowing people to use whatever SM configuration they want.

LaurentiuM1234 · 2025-03-04T11:17:57Z

drivers/firmware/scmi/nxp/shmem.c

+
+LOG_MODULE_REGISTER(arm_scmi_shmem_nxp);
+
+int scmi_shmem_vendor_validate_message(const struct scmi_shmem_layout *layout)


maybe we can use a more generic name for these functions? e.g: scmi_shmem_vendor_write_message and scmi_shmem_vendor_read_message. We don't really know what other vendors do with the implementation-defined bits.

LaurentiuM1234 · 2025-03-04T11:21:29Z

drivers/firmware/scmi/nxp/shmem.c

+void scmi_shmem_vendor_prepare_message(struct scmi_shmem_layout *layout)
+{
+#ifdef CONFIG_ARM_SCMI_SHMEM_USE_CRC
+	layout->res1[1] = crc32_ieee((const uint8_t *)&layout->msg_hdr, layout->len);


CRC32 is one of the CRCs supported by SM. You'll most likely have to check the value of res1[0] to see which CRC type SM expects.

Only supporting CRC32 right now is alright but at least check the value of res1[0] and return an error code if SM expects an unsupported CRC type?

LaurentiuM1234 · 2025-03-04T11:28:51Z

drivers/firmware/scmi/Kconfig

+	help
+	  When enabled, additional SCMI features specific to NXP will be available
+
+config ARM_SCMI_SHMEM_USE_CRC


maybe put vendor-specific configurations inside their own Kconfigs? e.g: drivers/firmware/scmi/nxp/Kconfig

LaurentiuM1234 · 2025-03-04T11:36:01Z

drivers/firmware/scmi/Kconfig

@@ -48,6 +48,19 @@ config ARM_SCMI_SHMEM_INIT_PRIORITY
 	help
 	  SCMI SHMEM driver device initialization priority.

+config ARM_SCMI_SHMEM_VENDOR_NXP


I'd suggest making this more generic: CONFIG_ARM_SCMI_NXP_VENDOR_EXTENSIONS. Using this, we can have a distributed approach with our Kconfig and CMakeLists.txt files (i.e: each vendor can have its own Kconfig with their own vendor-specific configurations and a CMakeLists.txt file for adding source files based on their own configurations).

LaurentiuM1234 · 2025-03-04T11:39:38Z

drivers/firmware/scmi/shmem_internal.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+struct scmi_shmem_layout {


this whole header file can probably be moved to ./include/zephyr/drivers/firmware/scmi/shmem.h. This way, we can avoid introducing another header.

An option is added to allow vendor specific processing at scmi_shmem_write_message() and scmi_shmem_read_message(). Additionally code has been added specific to NXP which has some extended validation features. Signed-off-by: Andre Heinemans <andre.heinemans@nxp.com>

AndreHeinemans-NXP · 2025-03-04T15:27:55Z

@JiafeiPan, @LaurentiuM1234,

Thanks for pointing this out. It's a perfect solution. I reworked all the comments.

zephyrbot added the platform: NXP MPU label Feb 26, 2025

zephyrbot requested review from dbaluta, dleach02, iuliana-prodan and JiafeiPan February 26, 2025 09:56

zephyrbot assigned JiafeiPan Feb 26, 2025

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch from de53e5e to 2a78a31 Compare February 26, 2025 10:19

iuliana-prodan reviewed Feb 26, 2025

View reviewed changes

drivers/firmware/scmi/Kconfig Outdated Show resolved Hide resolved

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch from c4326d1 to 0d87b9d Compare February 26, 2025 13:44

dbaluta reviewed Feb 26, 2025

View reviewed changes

LaurentiuM1234 reviewed Feb 26, 2025

View reviewed changes

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch from 0d87b9d to 4eada1f Compare February 27, 2025 13:04

zephyrbot added the platform: NXP Drivers NXP Semiconductors, drivers label Feb 27, 2025

zephyrbot requested review from decsny, manuargue, mmahadevan108 and Raymond0225 February 27, 2025 13:04

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch 4 times, most recently from 21efab8 to dea3547 Compare February 27, 2025 15:27

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch from dea3547 to 51d08a8 Compare February 28, 2025 10:05

PetervdPerk-NXP previously approved these changes Mar 3, 2025

View reviewed changes

PetervdPerk-NXP requested review from iuliana-prodan, LaurentiuM1234 and dbaluta March 3, 2025 08:38

bperseghetti previously approved these changes Mar 3, 2025

View reviewed changes

JiafeiPan requested changes Mar 4, 2025

View reviewed changes

LaurentiuM1234 reviewed Mar 4, 2025

View reviewed changes

AndreHeinemans-NXP dismissed stale reviews from bperseghetti and PetervdPerk-NXP via 01c58ac March 4, 2025 15:26

AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch from 51d08a8 to 01c58ac Compare March 4, 2025 15:26

AndreHeinemans-NXP requested a review from JiafeiPan March 4, 2025 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: firmware: scmi: add optional crc #86347

drivers: firmware: scmi: add optional crc #86347

AndreHeinemans-NXP commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

dbaluta Feb 26, 2025

PetervdPerk-NXP Feb 26, 2025

AndreHeinemans-NXP Feb 26, 2025

dbaluta Feb 26, 2025 •

edited

Loading

LaurentiuM1234 left a comment

LaurentiuM1234 Feb 26, 2025

PetervdPerk-NXP Feb 26, 2025 •

edited

Loading

AndreHeinemans-NXP Feb 27, 2025

yangbolu1991 commented Feb 27, 2025

dbaluta commented Feb 28, 2025

AndreHeinemans-NXP commented Feb 28, 2025

dbaluta commented Mar 3, 2025

bperseghetti commented Mar 3, 2025

JiafeiPan left a comment

LaurentiuM1234 left a comment

LaurentiuM1234 Mar 4, 2025

LaurentiuM1234 Mar 4, 2025

LaurentiuM1234 Mar 4, 2025

LaurentiuM1234 Mar 4, 2025

LaurentiuM1234 Mar 4, 2025

AndreHeinemans-NXP commented Mar 4, 2025


		LOG_MODULE_REGISTER(arm_scmi_shmem_nxp);

		int scmi_shmem_vendor_validate_message(const struct scmi_shmem_layout *layout)

drivers: firmware: scmi: add optional crc #86347

Are you sure you want to change the base?

drivers: firmware: scmi: add optional crc #86347

Conversation

AndreHeinemans-NXP commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

LaurentiuM1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetervdPerk-NXP Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangbolu1991 commented Feb 27, 2025

dbaluta commented Feb 28, 2025

AndreHeinemans-NXP commented Feb 28, 2025

dbaluta commented Mar 3, 2025

bperseghetti commented Mar 3, 2025

JiafeiPan left a comment

Choose a reason for hiding this comment

LaurentiuM1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreHeinemans-NXP commented Mar 4, 2025

dbaluta Feb 26, 2025 •

edited

Loading

PetervdPerk-NXP Feb 26, 2025 •

edited

Loading