Skip to content

Conversation

@yvesll
Copy link
Contributor

@yvesll yvesll commented Aug 5, 2025

Added NXP's EDAC implementation based on EIM and ERM

@github-actions
Copy link

github-actions bot commented Aug 5, 2025

Hello @yvesll, 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
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

Looks like a very good first PR. But can you explain why this Devicetree structure with 4(+?) nodes was chosen?

@yvesll
Copy link
Contributor Author

yvesll commented Aug 9, 2025

Looks like a very good first PR. But can you explain why this Devicetree structure with 4(+?) nodes was chosen?

You are correct, the virtual edac node and independent eim channel binding are redundant here.
I will only keep erm for error reporting and eim for error injection.

@yvesll yvesll force-pushed the add-nxp-edac-driver branch from e694c25 to be74c15 Compare August 11, 2025 07:27
@sonarqubecloud
Copy link

@Holt-Sun
Copy link
Contributor

Quality Gate passed

Please fix the issue in "Quality Gate passed".

@yvesll yvesll force-pushed the add-nxp-edac-driver branch from be74c15 to 815fb39 Compare September 17, 2025 01:50
@zephyrbot zephyrbot requested a review from decsny September 17, 2025 01:51
@yvesll yvesll force-pushed the add-nxp-edac-driver branch from 815fb39 to 971b938 Compare September 18, 2025 07:32
@yvesll yvesll force-pushed the add-nxp-edac-driver branch from 971b938 to 57d53c8 Compare September 19, 2025 09:10
@zephyrbot zephyrbot requested a review from decsny September 19, 2025 09:11
@decsny
Copy link
Member

decsny commented Oct 23, 2025

@yvesll you might need to rebase to get past some twister bug

@yvesll
Copy link
Contributor Author

yvesll commented Oct 23, 2025

@yvesll you might need to rebase to get past some twister bug

Sure, I will do this together with the support for MCXE31

/* Callback data provided to function passed to notify_cb_set */
struct edac_nxp_callback_data {
/* Number of corrected errors */
uint8_t corr_err_count;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the corr_err_count could exceed 255, then should use uint32_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not, ERM only use 8 bit for error counter.


static void edac_nxp_irq_0(const struct device *dev)
{
IRQ_CONNECT(DT_INST_IRQ_BY_IDX(0, 0, irq), DT_INST_IRQ_BY_IDX(0, 0, priority), edac_nxp_isr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcode the irq to 2. Considering the update for all IRQ numbers

Suggested change
IRQ_CONNECT(DT_INST_IRQ_BY_IDX(0, 0, irq), DT_INST_IRQ_BY_IDX(0, 0, priority), edac_nxp_isr,
#include <zephyr/sys/util.h>
static void edac_nxp_irq_0(const struct device *dev)
{
IRQ_CONNECT(DT_INST_IRQ_BY_IDX(0, 0, irq), DT_INST_IRQ_BY_IDX(0, 0, priority), edac_nxp_isr,
DEVICE_DT_INST_GET(0), 0);
irq_enable(DT_INST_IRQ_BY_IDX(0, 0, irq));
IRQ_CONNECT(DT_INST_IRQ_BY_IDX(0, 1, irq), DT_INST_IRQ_BY_IDX(0, 1, priority), edac_nxp_isr,
DEVICE_DT_INST_GET(0), 0);
irq_enable(DT_INST_IRQ_BY_IDX(0, 1, irq));
}
#define ERM_IRQ_CONNECT_IDX(idx, n) \
do { \
IRQ_CONNECT(DT_INST_IRQ_BY_IDX(n, idx, irq), \
DT_INST_IRQ_BY_IDX(n, idx, priority), \
edac_nxp_isr, DEVICE_DT_INST_GET(n), 0); \
irq_enable(DT_INST_IRQ_BY_IDX(n, idx, irq)); \
} while (0)
static void edac_nxp_irq_0(const struct device *dev)
{
ARG_UNUSED(dev);
UTIL_LISTIFY(DT_INST_NUM_IRQS(0), ERM_IRQ_CONNECT_IDX, 0);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No platform has more than one erm/eim instance now. It only requires one instance as 31 channels is really enough for MCU.

Add edac driver for NXP's ERM and EIM peripherals. It can inject ECC
error to specific channel within EIM and then report the error address,
syndrome and count within ERM.

Signed-off-by: Yves Wang <zhengjia.wang@nxp.com>
Add eim, erm and edac instance for frdm_mcxa153, frdm_mcxn236,
frdm_mcxn947, frdm_mcxe247 and frdm_mcxe31b.

Signed-off-by: Yves Wang <zhengjia.wang@nxp.com>
Decouple the edac sample with IBECC.

Signed-off-by: Yves Wang <zhengjia.wang@nxp.com>
Update the console log in readme to make it aligned with the actual
output

Signed-off-by: Yves Wang <zhengjia.wang@nxp.com>
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants