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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+117 −9
Open

drivers: firmware: scmi: add optional crc #86347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions boards/nxp/imx95_evk/imx95_evk_mimx9596_m7_ddr_defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ CONFIG_CONSOLE=y
CONFIG_MBOX=y
CONFIG_MBOX_INIT_PRIORITY=0
CONFIG_ARM_SCMI=y
CONFIG_ARM_SCMI_NXP_VENDOR_EXTENSIONS=y

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@dbaluta dbaluta Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

# kernel-related configurations
CONFIG_XIP=n
1 change: 1 addition & 0 deletions boards/nxp/imx95_evk/imx95_evk_mimx9596_m7_defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ CONFIG_XIP=y
CONFIG_MBOX=y
CONFIG_MBOX_INIT_PRIORITY=0
CONFIG_ARM_SCMI=y
CONFIG_ARM_SCMI_NXP_VENDOR_EXTENSIONS=y
2 changes: 2 additions & 0 deletions drivers/firmware/scmi/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ zephyr_library_sources_ifdef(CONFIG_ARM_SCMI_SHMEM shmem.c)
zephyr_library_sources_ifdef(CONFIG_ARM_SCMI_CLK_HELPERS clk.c)
zephyr_library_sources_ifdef(CONFIG_ARM_SCMI_PINCTRL_HELPERS pinctrl.c)
zephyr_library_sources_ifdef(CONFIG_ARM_SCMI_POWER_DOMAIN_HELPERS power.c)

add_subdirectory_ifdef(CONFIG_ARM_SCMI nxp)
2 changes: 2 additions & 0 deletions drivers/firmware/scmi/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,6 @@ config ARM_SCMI_TRANSPORT_INIT_PRIORITY
help
SCMI transport driver device initialization priority.

source "drivers/firmware/scmi/nxp/Kconfig"

endif # ARM_SCMI
5 changes: 5 additions & 0 deletions drivers/firmware/scmi/nxp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# SPDX-License-Identifier: Apache-2.0

zephyr_library()

zephyr_library_sources_ifdef(CONFIG_ARM_SCMI_NXP_VENDOR_EXTENSIONS shmem.c)
8 changes: 8 additions & 0 deletions drivers/firmware/scmi/nxp/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Copyright 2025 NXP
# SPDX-License-Identifier: Apache-2.0

config ARM_SCMI_NXP_VENDOR_EXTENSIONS
bool "Allow NXP specific SCMI features"
select CRC
help
When enabled, additional SCMI features specific to NXP will be available
50 changes: 50 additions & 0 deletions drivers/firmware/scmi/nxp/shmem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2025 NXP
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/drivers/firmware/scmi/shmem.h>
#include <zephyr/sys/crc.h>
#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(arm_scmi_shmem_nxp);

#define SMT_CRC_NONE 0U
#define SMT_CRC_XOR 1U /* Unsupported */
#define SMT_CRC_J1850 2U /* Unsupported */
#define SMT_CRC_CRC32 3U

int scmi_shmem_vendor_read_message(const struct scmi_shmem_layout *layout)
{
uint32_t validation_type = layout->res1[0];

if (validation_type == SMT_CRC_CRC32) {
if (layout->res1[1] != crc32_ieee((const uint8_t *)&layout->msg_hdr, layout->len)) {
LOG_ERR("bad message crc");
return -EBADMSG;
}
} else if (validation_type == SMT_CRC_NONE) {
/* do nothing */
} else {
LOG_ERR("invalid validation type 0x%x", validation_type);
return -EINVAL;
}

return 0;
}

int scmi_shmem_vendor_write_message(struct scmi_shmem_layout *layout)
{
uint32_t validation_type = layout->res1[0];

if (validation_type == SMT_CRC_CRC32) {
layout->res1[1] = crc32_ieee((const uint8_t *)&layout->msg_hdr, layout->len);
} else if (validation_type == SMT_CRC_NONE) {
/* do nothing */
} else {
LOG_ERR("invalid validation type 0x%x", validation_type);
return -EINVAL;
}

return 0;
}
28 changes: 19 additions & 9 deletions drivers/firmware/scmi/shmem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,6 @@ struct scmi_shmem_data {
mm_reg_t regmap;
};

struct scmi_shmem_layout {
volatile uint32_t res0;
volatile uint32_t chan_status;
volatile uint32_t res1[2];
volatile uint32_t chan_flags;
volatile uint32_t len;
volatile uint32_t msg_hdr;
};

int scmi_shmem_get_channel_status(const struct device *dev, uint32_t *status)
{
struct scmi_shmem_data *data;
Expand All @@ -57,6 +48,16 @@ static void scmi_shmem_memcpy(mm_reg_t dst, mm_reg_t src, uint32_t bytes)
}
}

__weak int scmi_shmem_vendor_read_message(const struct scmi_shmem_layout *layout)
{
return 0;
}

__weak int scmi_shmem_vendor_write_message(struct scmi_shmem_layout *layout)
{
return 0;
}

int scmi_shmem_read_message(const struct device *shmem, struct scmi_message *msg)
{
struct scmi_shmem_layout *layout;
Expand Down Expand Up @@ -96,6 +97,11 @@ int scmi_shmem_read_message(const struct device *shmem, struct scmi_message *msg
return -EINVAL;
}

if (scmi_shmem_vendor_read_message(layout) < 0) {
LOG_ERR("vendor specific validation failed");
return -EINVAL;
}

if (msg->content) {
scmi_shmem_memcpy(POINTER_TO_UINT(msg->content),
data->regmap + sizeof(*layout), msg->len);
Expand Down Expand Up @@ -139,6 +145,10 @@ int scmi_shmem_write_message(const struct device *shmem, struct scmi_message *ms
POINTER_TO_UINT(msg->content), msg->len);
}

if (scmi_shmem_vendor_write_message(layout) < 0) {
return -EINVAL;
}

/* done, mark channel as busy and proceed */
layout->chan_status &= ~SCMI_SHMEM_CHAN_STATUS_BUSY_BIT;

Expand Down
29 changes: 29 additions & 0 deletions include/zephyr/drivers/firmware/scmi/shmem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@
#define SCMI_SHMEM_CHAN_STATUS_BUSY_BIT BIT(0)
#define SCMI_SHMEM_CHAN_FLAG_IRQ_BIT BIT(0)

struct scmi_shmem_layout {
volatile uint32_t res0;
volatile uint32_t chan_status;
volatile uint32_t res1[2];
volatile uint32_t chan_flags;
volatile uint32_t len;
volatile uint32_t msg_hdr;
};

struct scmi_message;

/**
Expand Down Expand Up @@ -64,4 +73,24 @@ void scmi_shmem_update_flags(const struct device *shmem,
*/
uint32_t scmi_shmem_channel_status(const struct device *shmem);

/**
* @brief Process vendor specific features when writing message
*
* @param layout layout of the message
*
* @retval 0 if successful
* @retval negative errno if failure
*/
int scmi_shmem_vendor_write_message(struct scmi_shmem_layout *layout);

/**
* @brief Process vendor specific features when reading message
*
* @param layout layout of the message
*
* @retval 0 if successful
* @retval negative errno if failure
*/
int scmi_shmem_vendor_read_message(const struct scmi_shmem_layout *layout);

#endif /* _INCLUDE_ZEPHYR_DRIVERS_FIRMWARE_SCMI_SHMEM_H_ */
Loading