Imx943 change irqsteer driver as native driver #99187

biwenli · 2025-11-11T07:27:34Z

Change irqsteer driver as a zephyr native driver.

There are two irqsteer drivers in mcux sdk(mcux-sdk-ng/drivers/irqsteer and mcux-sdk-ng/drivers/irqsteer_1) and the APIs are not compatible. mcux-sdk-ng/drivers/irqsteer_1 just likes a native irqsteer driver(Refer to the linux irqsteer driver: drivers/irqchip/irq-imx-irqsteer.c). So change it as a native driver directly.

LaurentiuM1234

Some initial comments below.

Most importantly: please don't change anything in the driver unless it's related to the fact that you're dropping the HAL API. This is an important change and by doing so you're making the review harder and easier to miss out the important stuff.

If you really want to make some unrelated changes then please do so in a separate commit with the appropriate justification.

Also, please make sure you don't break bisectability (see comment from binding). Make sure that after you apply each commit, the affected boards will still compile.

dts/bindings/interrupt-controller/nxp,irqsteer-intc.yaml

drivers/interrupt_controller/intc_nxp_irqsteer.c

LaurentiuM1234 · 2025-11-11T10:01:59Z

dts/bindings/interrupt-controller/nxp,irqsteer-intc.yaml

  reg:
    required: true
+
+  irq_offset:


do we still need this?

CC: @yongxu-wang15

biwenli · 2025-11-13T06:17:52Z

Some initial comments below.

Most importantly: please don't change anything in the driver unless it's related to the fact that you're dropping the HAL API. This is an important change and by doing so you're making the review harder and easier to miss out the important stuff.

If you really want to make some unrelated changes then please do so in a separate commit with the appropriate justification.

Also, please make sure you don't break bisectability (see comment from binding). Make sure that after you apply each commit, the affected boards will still compile.

sure, np.

github-actions · 2025-11-17T08:37:20Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	nxp-upstream/hal_nxp@`6d6ab91`	nxp-upstream/hal_nxp@mcux-sdk-ng-drivers-common-fix-build-issue-without-irqsteer-drivers	nxp-upstream/hal_nxp@6d6ab91d..mcux-sdk-ng-drivers-common-fix-build-issue-without-irqsteer-drivers

Additional metadata changed:

Name	URL	Submodules	West cmds	`module.yml`	Blobs
hal_nxp	✏			✏	4x 🆕

⛔ DNM label due to: 1 project with metadata changes and 4 blob changes

Note: This message is automatically posted and updated by the Manifest GitHub Action.

Add irqsteer node for m7_0. Signed-off-by: Biwen Li <[email protected]>

Add irqsteer node for m7_1. Signed-off-by: Biwen Li <[email protected]>

Add edma2 node for lpuart3. Signed-off-by: Biwen Li <[email protected]>

Enable edma2 defaultly. Signed-off-by: Biwen Li <[email protected]>

Add more irqsteer masters. Signed-off-by: Biwen Li <[email protected]>

Add disp_irqsteer node(this is another irqsteer instance). Signed-off-by: Biwen Li <[email protected]>

Sync hal_nxp to fix below build issue, zephyr/modules/hal/nxp/mcux/mcux-sdk-ng/ drivers/common/fsl_common_arm.h:697: undefined reference to IRQSTEER_EnableInterrupt Signed-off-by: Biwen Li <[email protected]>

sonarqubecloud · 2025-11-20T10:33:38Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dbaluta · 2025-11-20T10:35:56Z

This change breaks Sound Open Firmware running on HIFI4 from imx8mp. Will try to do some investigation. Do you have an imx8mp board on your side?

dbaluta · 2025-11-20T10:36:57Z

dts/arm/nxp/nxp_imx943_m33.dtsi


-		cpu0:cpu@0 {
+		cpu0: cpu@0 {
 			device_type = "cpu";


Please don't add random cleanup fixes in the middle of a patch. If you want to do cleanups please add them at the beginning of the PR or create a totally new PR.

Please don't add random cleanup fixes in the middle of a patch. If you want to do cleanups please add them at the beginning of the PR or create a totally new PR.

Okay, I will add them at the begginning of the PR.

dbaluta · 2025-11-20T10:37:54Z

dts/arm/nxp/nxp_imx94x.dtsi


+		flexio1: flexio@425c0000 {
+			compatible = "nxp,flexio";
+			reg = <0x425c0000 DT_SIZE_K(4)>;


The commit description says that you enable edma4 but the code does more than that. E.g enable flexio1 etc

The commit description says that you enable edma4 but the code does more than that. E.g enable flexio1 etc

The flexio is from nxp_imx943_m33.dts, so i move them from nxp_imx943_m33.dts into the common file nxp_imx94x.dtsi

dbaluta · 2025-11-20T10:38:12Z

dts/arm/nxp/nxp_imx94x.dtsi

-		zephyr,memory-attr = <(DT_MEM_ARM(ATTR_MPU_RAM_NOCACHE))>;
+		zephyr,memory-region = "DRAM_M33S_M71_IPC0";
+		zephyr,memory-attr = <DT_MEM_ARM(ATTR_MPU_RAM_NOCACHE)>;
 	};


This doesn't match patch description.

This doesn't match patch description.

Some issues are reported by compliance checks(./scripts/ci/check_compliance.py). So i fix them here.

dbaluta · 2025-11-20T10:38:47Z

dts/arm/nxp/nxp_imx94x.dtsi

-&gpio5{
+&gpio5 {
 	pinmux = <&iomuxc_eth0_txd0_gpio_io_gpio5_io0>,
-		<&iomuxc_eth0_txd1_gpio_io_gpio5_io1>,


Please don't randomly fix whitespaces in the middle of a patch. You can create a separate cleanup PR if you want .

Please don't randomly fix whitespaces in the middle of a patch. You can create a separate cleanup PR if you want .

Fix compliance checks issues.

dbaluta · 2025-11-20T10:40:21Z

dts/arm/nxp/nxp_imx95_m7.dtsi

 				compatible = "nxp,irqsteer-master";
 				reg = <0>;
 				interrupt-controller;
 				#interrupt-cells = <2>;


Please update the commit message with a proper explanation.

Please update the commit message with a proper explanation.

sure, np.

dbaluta · 2025-11-20T11:39:07Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

+/* Register access macros */
+#define IRQSTEER_READ32(base, offset)       sys_read32((base) + (offset))
+#define IRQSTEER_WRITE32(base, offset, val) sys_write32((val), (base) + (offset))
+


I would get read read of this macros because capital letter make code harder to read.

I would get read read of this macros because capital letter make code harder to read.

Sure, np. I will update it(irqsteer_read_reg32).

dbaluta · 2025-11-20T11:42:12Z

drivers/interrupt_controller/intc_nxp_irqsteer.c


-/* used for driver binding */
+/* Driver compatibility */
 #define DT_DRV_COMPAT nxp_irqsteer_intc


The original comment is OK! Why do you need to change it? Driver compatibility means something else.

The original comment is OK! Why do you need to change it? Driver compatibility means something else.

Ok, let's keep the original comment.

dbaluta · 2025-11-20T11:43:27Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

 * @file
- * @brief Driver for NXP's IRQ_STEER IP.
+ * @brief Native driver for NXP's IRQ_STEER IP (without HAL dependency)
 *


Just leave this comment alone, do not modify it. Readers will be confused about this comment and they'll think there is another driver using HAL dependency.

Just leave this comment alone, do not modify it. Readers will be confused about this comment and they'll think there is another driver using HAL dependency.

Ok.

dbaluta · 2025-11-20T11:43:51Z

drivers/interrupt_controller/intc_nxp_irqsteer.c


-LOG_MODULE_REGISTER(nxp_irqstr);
+LOG_MODULE_REGISTER(nxp_irqsteer);



Is there a reason for this change? All the prefixes in this file are still nxp_irqster

Is there a reason for this change? All the prefixes in this file are still nxp_irqster

No special reason. It's okay to keep the original string.

dbaluta · 2025-11-20T11:44:44Z

dts/arm/nxp/nxp_imx95_m7.dtsi

 			compatible = "nxp,irqsteer-intc";
 			reg = <0x44680000 DT_SIZE_K(64)>;
-
 			#size-cells = <0>;


Please dont randomly delete new lines.

Please dont randomly delete new lines.

sure. np.

dbaluta · 2025-11-20T12:40:18Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

@@ -1,12 +1,12 @@
 /*


I'm sorry but I don't understand the commit message.

What does this mean mcux-sdk-ng/drivers/irqsteer_1 just likes a native irqsteer driver?

From my understanding the commit message should look like this:

drivers: intc: irqsteer: Drop calling into NXP HAL Supporting irqsteer using NXP HAL becomes increasingly harder with new boards. For example now there are two incompatible HAL drivers for IRQ steer (mcux-sdk-ng/drivers/irqsteer and mcux-sdk-ng/drivers/irqsteer_1). In order to avoid overcomplicating code and better scaling code for newer boards just drop using the NXP HAL and implement an IRQ Steer native Zephyr driver

I'm sorry but I don't understand the commit message.

What does this mean mcux-sdk-ng/drivers/irqsteer_1 just likes a native irqsteer driver?

From my understanding the commit message should look like this:

drivers: intc: irqsteer: Drop calling into NXP HAL Supporting irqsteer using NXP HAL becomes increasingly harder with new boards. For example now there are two incompatible HAL drivers for IRQ steer (mcux-sdk-ng/drivers/irqsteer and mcux-sdk-ng/drivers/irqsteer_1). In order to avoid overcomplicating code and better scaling code for newer boards just drop using the NXP HAL and implement an IRQ Steer native Zephyr driver

That means that the mcux-sdk-ng/drivers/irqsteer_1 looks like a native irqsteer driver.

Sure, will follow the message.

dbaluta · 2025-11-20T12:42:33Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

+	/* There is one output irq for each group of 64 inputs. */
+	int num_masters;
+	/* One register bit map can represent 32 input interrupts. */
+	int reg_num;


AFAIK IRQSTEER is an configurable IP at hardware integration time so are you sure the groups are always 64? Not clear what is reg_num.

AFAIK IRQSTEER is an configurable IP at hardware integration time so are you sure the groups are always 64? Not clear what is reg_num.

Yes, i'm sure. I have checked all of platforms for the input interrputs per group.

dbaluta · 2025-11-20T12:43:12Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

-static int to_zephyr_irq(uint32_t regmap, uint32_t irq,
+/* Native register operations (replacing HAL functions) */
+
+/**


I would drop any reference to HAL in this driver as we are no longer using it. The only references about HAL should be in the commit message that would justify the removal of NXP API HAL calls.

dbaluta · 2025-11-20T12:45:48Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

+	mask = IRQSTEER_READ32(base, CHAN_MASK(reg_index, reg_num));
+	mask |= BIT(bit);
+	LOG_DBG("base = 0x%x, reg_index = %d, CHAN_MASK(%d, %d) = 0x%x, bit = %d, mask = 0x%x",
+		base, reg_index, reg_index, reg_num, CHAN_MASK(reg_index, reg_num), bit, mask);


I would drop the LOG_DBG it just makes code harder to read. When debugging this the reader can add its own debug prints.

Also, this is not symmetric with the irqsteer_disable_interrupt. So you either add the LOGS to both functions or to none.

dbaluta · 2025-11-20T12:46:37Z

drivers/interrupt_controller/intc_nxp_irqsteer.c

+
+	/* Read status from relevant register index */
+	for (i = 0; i < 2; i++) {
+		if (reg_index == reg_num) {


Try to avoid using magic number as much as possible. What is this 2 representing here?

dbaluta · 2025-11-20T12:50:37Z

dts/bindings/interrupt-controller/nxp,irqsteer-intc.yaml

+    type: int
+    description: |
+      How many second level interrupt numbers are supported by an irqsteer instance.
+      e.g. In i.MX943,


Would reprhase this as:

description: | Total number of second level interrupts supported by an irqsteer instance. Also typo -> conncted -> connected.

dbaluta · 2025-11-20T12:55:26Z

dts/arm/nxp/nxp_imx943_m33.dtsi


 		dtcm: dtcm@20000000 {
 			compatible = "nxp,imx-dtcm";
 			reg = <0x20000000 DT_SIZE_K(256)>;


Please use the commit the message to offer an overview of the irqsteer integration into imx943.

Take this chance to better explain what is irq-offset and num-irqs.

dbaluta · 2025-11-20T12:57:10Z

modules/hal_nxp/mcux/mcux-sdk-ng/drivers/drivers.cmake

 set_variable_ifdef(CONFIG_MCUX_SDIF             CONFIG_MCUX_COMPONENT_driver.sdif)
 set_variable_ifdef(CONFIG_MCUX_XBARA            CONFIG_MCUX_COMPONENT_driver.xbara)
 set_variable_ifdef(CONFIG_MCUX_XBARB            CONFIG_MCUX_COMPONENT_driver.xbarb)
 set_variable_ifdef(CONFIG_QDEC_MCUX             CONFIG_MCUX_COMPONENT_driver.enc)


This could and should be integrated in the commit that drops the HAL NXP calls.

dbaluta · 2025-11-20T13:46:31Z

@biwenli changes are starting to look good. The split per patch starts to be OK.
Few comments:

please do not add whitespace cleanups or code style fixes inside a patch doing other things. Either do it at the beginnning of the PR or create a separate PR.
Please try to add better commit description explaining the hardware configuration that is described by the dts updates
I will need to re-test your PR when ready because as of now SOF on imx8mp no longer boots with this PR.

biwenli · 2025-11-21T08:21:35Z

This change breaks Sound Open Firmware running on HIFI4 from imx8mp. Will try to do some investigation. Do you have an imx8mp board on your side?

Yes, i have. But i never touch SOF. So need your help to debug the issue.

zephyrbot requested review from JiafeiPan, MaureenHelm, Zhiqiang-Hou, andyross, ceolin, dbaluta, dcpleung, dleach02, ithinuel, iuliana-prodan, mmahadevan108, nashif, stephanosio, wearyzen and yangbolu1991 November 11, 2025 07:29

zephyrbot assigned wearyzen Nov 11, 2025

biwenli mentioned this pull request Nov 11, 2025

enable irqsteer for m33 of imx943 #93760

Closed

LaurentiuM1234 reviewed Nov 11, 2025

View reviewed changes

biwenli force-pushed the imx943-change-irqsteer-driver-as-native-driver branch from 900da9d to 4268eb0 Compare November 13, 2025 10:06

zephyrbot requested review from decsny, kartben and tejlmand November 13, 2025 10:08

biwenli force-pushed the imx943-change-irqsteer-driver-as-native-driver branch 3 times, most recently from 121c460 to c1686ee Compare November 13, 2025 10:40

github-actions bot added manifest manifest-hal_nxp DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Nov 17, 2025

biwenli added 7 commits November 20, 2025 19:04

dts: arm: nxp: imx943: m7_0: add irqsteer node

06bb0c5

Add irqsteer node for m7_0. Signed-off-by: Biwen Li <[email protected]>

dts: arm: nxp: imx943: m7_1: add irqsteer node

d15dd49

Add irqsteer node for m7_1. Signed-off-by: Biwen Li <[email protected]>

dts: arm: nxp: imx94x: add edma2 node

6761980

Add edma2 node for lpuart3. Signed-off-by: Biwen Li <[email protected]>

boards: nxp: imx943_evk: m33: enable edma2

b8d1b67

Enable edma2 defaultly. Signed-off-by: Biwen Li <[email protected]>

dts: arm: nxp: imx95: m7: add more masters

7f84ce3

Add more irqsteer masters. Signed-off-by: Biwen Li <[email protected]>

dts: arm: nxp: imx95: m7: add disp_irqsteer node

1fbb9c2

Add disp_irqsteer node(this is another irqsteer instance). Signed-off-by: Biwen Li <[email protected]>

west.yml: sync hal_nxp to fix build issue

9195599

Sync hal_nxp to fix below build issue, zephyr/modules/hal/nxp/mcux/mcux-sdk-ng/ drivers/common/fsl_common_arm.h:697: undefined reference to IRQSTEER_EnableInterrupt Signed-off-by: Biwen Li <[email protected]>

biwenli force-pushed the imx943-change-irqsteer-driver-as-native-driver branch from dbbdc36 to 9195599 Compare November 20, 2025 10:05

dbaluta reviewed Nov 20, 2025

View reviewed changes


		LOG_MODULE_REGISTER(nxp_irqstr);
		LOG_MODULE_REGISTER(nxp_irqsteer);

Imx943 change irqsteer driver as native driver #99187

Are you sure you want to change the base?

Imx943 change irqsteer driver as native driver #99187

Conversation

biwenli commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaurentiuM1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biwenli commented Nov 13, 2025

Uh oh!

github-actions bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 20, 2025

Quality Gate passed

Uh oh!

dbaluta commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

biwenli commented Nov 11, 2025 •

edited

Loading

github-actions bot commented Nov 17, 2025 •

edited

Loading