Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: firmware: scmi: add optional crc #86347

Merged

Conversation

AndreHeinemans-NXP
Copy link
Contributor

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.

Copy link

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

Copy link
Collaborator

@LaurentiuM1234 LaurentiuM1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yangbolu1991
Copy link
Contributor

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.

@zephyrbot zephyrbot added the platform: NXP Drivers NXP Semiconductors, drivers label Feb 27, 2025
@AndreHeinemans-NXP AndreHeinemans-NXP force-pushed the add-crc-to-scmi branch 4 times, most recently from 21efab8 to dea3547 Compare February 27, 2025 15:27
@dbaluta
Copy link
Collaborator

dbaluta commented Feb 28, 2025

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
Copy link
Contributor Author

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

PetervdPerk-NXP
PetervdPerk-NXP previously approved these changes Mar 3, 2025
@dbaluta
Copy link
Collaborator

dbaluta commented Mar 3, 2025

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
bperseghetti previously approved these changes Mar 3, 2025
@bperseghetti
Copy link
Member

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.

Copy link
Contributor

@JiafeiPan JiafeiPan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@LaurentiuM1234 LaurentiuM1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AndreHeinemans-NXP
Copy link
Contributor Author

@JiafeiPan, @LaurentiuM1234,

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

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>
@kartben kartben added this to the v4.2.0 milestone Mar 6, 2025
@carlescufi carlescufi merged commit da3d029 into zephyrproject-rtos:main Mar 7, 2025
23 checks passed
Copy link

github-actions bot commented Mar 7, 2025

Hi @AndreHeinemans-NXP!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet