i3c: dw: Add apb reset support

PATCH In review Jisheng Zhang 3 participants 13 messages May 25 – Jun 3

Subsystems (3)

Device tree bindings I3C I3C driver for synopsys DesignWare

lore

Share & tools

lore

Jisheng Zhang

author

2026-05-25 14:19 UTC #0 lore raw

v4 cover i3c: dw: Add apb reset support

Add support of apb reset which is to reset the APB interface. The first patch is to document the exisiting reset dt-binding. 2nd patch is to add apb reset dt-binding. The last patch is to add apb reset support.

NOTE: to align with "core_rst", the new added apb reset is named as "apb_rst", IOW, the reset names: "core_rst" + "apb_rst". I can modify it if maintainers prefer "core_rst" + "apb"

Since v3:
- back to v2, I.E keep the "_rst" suffix, since removing it is an ABI breakage.

Since v2:
- remove "_rst" suffix

Since v1:
- add dt-binding

Jisheng Zhang (3):
dt-bindings: i3c: dw: Describe core reset
dt-bindings: i3c: dw: Add apb reset
i3c: dw: Add apb reset support

 .../devicetree/bindings/i3c/snps,dw-i3c-master.yaml    | 10 ++++++++++
 drivers/i3c/master/dw-i3c-master.c                     |  7 +++++++
 drivers/i3c/master/dw-i3c-master.h                     |  1 +
 3 files changed, 18 insertions(+)

Signature · 1 line

2.53.0

Jisheng Zhang

author

2026-05-25 14:19 UTC #1 lore raw

PATCH v4 1/3 dt-bindings: i3c: dw: Describe core reset

The "core_rst" reset support has been in the code from day1, but the dt-binding doesn't exist. Add dt-binding to describe reset property.

Then why not remove the "_rst" suffix? Krzysztof pointed out "Cleanup of name is not really worth affecting users. core_rst is not the best name but it is not incorrect, either."

Signed-off-by Jisheng Zhang <[email protected]>

---
 .../devicetree/bindings/i3c/snps,dw-i3c-master.yaml       | 8 ++++++++
 1 file changed, 8 insertions(+)

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +8 -0

index e803457d3f55..613dce7757bc 100644
@@ -35,6 +35,14 @@ properties:
       - const: core
       - const: apb
 
+  resets:
+    items:
+      - description: Reset signal
+
+  reset-names:
+    items:
+      - const: core_rst
+
   interrupts:
     maxItems: 1

Signature · 1 line

2.53.0

Krzysztof Kozlowski

Reviewed-by on v4 1/3

2026-05-25 15:23 UTC #2 lore raw

Re: [PATCH v4 1/3] dt-bindings: i3c: dw: Describe core reset

Quoted 1 line · Jisheng Zhang

The "core_rst" reset support has been in the code from day1, but the

Reset cannot be supported and it is about ABI implemented by the driver.

Quoted 7 lines

dt-binding doesn't exist. Add dt-binding to describe reset property.
Then why not remove the "_rst" suffix? Krzysztof pointed out "Cleanup of name is not really worth affecting users. core_rst is not the best name but it is not incorrect, either."

This drop, irrelevant. You can provide Link: to previous discussions, OTOH.

With above improved:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Jisheng Zhang

author

2026-05-25 14:19 UTC #3 lore raw

PATCH v4 2/3 dt-bindings: i3c: dw: Add apb reset

In reply to Jisheng Zhang

Add dt-binding for support of apb reset which is to reset the APB interface.

Signed-off-by Jisheng Zhang <[email protected]>

---
 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
 1 file changed, 2 insertions(+)

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +2 -0

index 613dce7757bc..2575442b28ff 100644
@@ -38,10 +38,12 @@ properties:
   resets:
     items:
       - description: Reset signal
+      - description: APB interface reset signal
 
   reset-names:
     items:
       - const: core_rst
+      - const: apb_rst
 
   interrupts:
     maxItems: 1

Signature · 1 line

2.53.0

[email protected]

AI review

2026-05-25 14:36 UTC #4 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Adding a second item to `resets` and `reset-names` without specifying `minItems: 1` makes the…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Adding a second item to resets and reset-names without specifying minItems: 1 makes the new reset mandatory, breaking validation for device trees that only provide the core reset. --

commit 620ece5c39191636270140b0c99ea8ddfa402c6f
Author: Jisheng Zhang <[email protected]>

dt-bindings: i3c: dw: Add apb reset

Add dt-binding for support of apb reset which is to reset the APB interface.

Quoted 5 lines

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +0 -0
index 613dce7757bc3..2575442b28ff4 100644

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml properties: · line 46

Quoted 9 lines

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +2 -0

@@ -38,10 +38,12 @@ properties:
   resets:
     items:
       - description: Reset signal
+      - description: APB interface reset signal
 
   reset-names:
     items:
       - const: core_rst
+      - const: apb_rst

[Severity: Medium]
Does adding a second item to resets and reset-names without minItems: 1 make the new reset mandatory?

In dt-schema, when items is defined as a list, the minItems and maxItems properties implicitly default to the number of items in the list. By adding apb_rst as a second item without specifying minItems: 1, the binding now strictly requires exactly two items whenever the resets property is present.

This breaks backward compatibility for existing device trees that supply only core_rst, causing them to fail make dtbs_check. To fix this schema issue,
minItems: 1 must be explicitly added to both resets and reset-names.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Krzysztof Kozlowski

2026-05-25 15:25 UTC #5 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

In reply to Jisheng Zhang

Quoted 2 lines · Jisheng Zhang

Add dt-binding for support of apb reset which is to reset the APB interface.

And this is ABI break, so you must explain WHY breaking ABI is worth doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.

Quoted 11 lines

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +0 -0
index 613dce7757bc..2575442b28ff 100644

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml properties: · line 46

Quoted 9 lines

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +2 -0

@@ -38,10 +38,12 @@ properties:
   resets:
     items:
       - description: Reset signal
+      - description: APB interface reset signal
 
   reset-names:
     items:
       - const: core_rst
+      - const: apb_rst

apb

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml properties: · line 49

Quoted 3 lines

Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +0 -0
   interrupts:
     maxItems: 1

Best regards,
Krzysztof

Jisheng Zhang

author

2026-05-27 04:37 UTC #6 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Quoted 5 lines · Krzysztof Kozlowski

Quoted 2 lines · Jisheng Zhang
Add dt-binding for support of apb reset which is to reset the APB interface.
And this is ABI break, so you must explain WHY breaking ABI is worth

This just adds an optional apb reset, it doesn't break any exisiting ABI. Kindly let me know whether adding new optional binding is also an ABI break.

Quoted 3 lines

doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.

I'm preparing one of synaptics SoCs support to uptream, it needs this apb reset signal for i3c. So you mean I delay this series until the SoC upstream series come, right?

Quoted 31 lines

Quoted 12 lines
Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +0 -0
index 613dce7757bc..2575442b28ff 100644
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml properties: · line 46
Quoted 9 lines
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +2 -0
@@ -38,10 +38,12 @@ properties:
   resets:
     items:
       - description: Reset signal
+      - description: APB interface reset signal
 
   reset-names:
     items:
       - const: core_rst
+      - const: apb_rst
apb
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml properties: · line 49
Quoted 3 lines
Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml +0 -0
   interrupts:
     maxItems: 1
Best regards,
Krzysztof

Krzysztof Kozlowski

2026-05-27 07:53 UTC #7 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Quoted 10 lines · Jisheng Zhang

Quoted 5 lines · Krzysztof Kozlowski
Quoted 2 lines · Jisheng Zhang
Add dt-binding for support of apb reset which is to reset the APB interface.
And this is ABI break, so you must explain WHY breaking ABI is worth
This just adds an optional apb reset, it doesn't break any exisiting ABI. Kindly let me know whether adding new optional binding is also an ABI break.

It does. One reset was before. Now all devices must have two resets. Clear ABI impact.

Quoted 8 lines

Quoted 4 lines
doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.
I'm preparing one of synaptics SoCs support to uptream, it needs this apb reset signal for i3c. So you mean I delay this series until the SoC upstream series come, right?

No. You just sent patch which basically says that Altera has two resets, without any explanation of that.

Write explicit patches with answers WHY you re doing and WHY its impact is correct.

Best regards,
Krzysztof

Jisheng Zhang

author

2026-05-30 13:17 UTC #8 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Quoted 6 lines · Krzysztof Kozlowski

Quoted 5 lines · Jisheng Zhang
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Jisheng Zhang
Add dt-binding for support of apb reset which is to reset the APB interface.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
And this is ABI break, so you must explain WHY breaking ABI is worth
This just adds an optional apb reset, it doesn't break any exisiting ABI. Kindly let me know whether adding new optional binding is also an ABI break.
It does. One reset was before. Now all devices must have two resets. Clear ABI impact.

I checked the dts with only one reset by manual dt_binding_check and dtbs_check, it still work, so I'm not sure why those dts "must have two resets". Kindly let me know what's wrong.

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.

Quoted 6 lines

Quoted 3 lines
I'm preparing one of synaptics SoCs support to uptream, it needs this apb reset signal for i3c. So you mean I delay this series until the SoC upstream series come, right?
No. You just sent patch which basically says that Altera has two resets, without any explanation of that.

I guess you mixed other series/patches with mine here, no? I didn't mention Altera at all ;) What my series mentioned is adding apb reset which is to reset the APB interface.

Quoted 7 lines

Write explicit patches with answers WHY you re doing and WHY its impact is correct.
Best regards,
Krzysztof

Krzysztof Kozlowski

2026-06-03 14:36 UTC #9 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Quoted 30 lines · Jisheng Zhang

Quoted 6 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Jisheng Zhang
Add dt-binding for support of apb reset which is to reset the APB interface.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
And this is ABI break, so you must explain WHY breaking ABI is worth
This just adds an optional apb reset, it doesn't break any exisiting ABI. Kindly let me know whether adding new optional binding is also an ABI break.
It does. One reset was before. Now all devices must have two resets. Clear ABI impact.
I checked the dts with only one reset by manual dt_binding_check and dtbs_check, it still work, so I'm not sure why those dts "must have two resets". Kindly let me know what's wrong.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.
I'm preparing one of synaptics SoCs support to uptream, it needs this apb reset signal for i3c. So you mean I delay this series until the SoC upstream series come, right?

Quoted 7 lines

Quoted 2 lines
No. You just sent patch which basically says that Altera has two resets, without any explanation of that.
I guess you mixed other series/patches with mine here, no? I didn't mention Altera at all ;) What my series mentioned is adding apb reset which is to reset the APB interface.

Read the binding. Altera is already there. I do not talk about other patches.

Your are changing an already accepted binding.

Best regards,
Krzysztof

Krzysztof Kozlowski

2026-06-03 14:38 UTC #10 lore raw

Re: [PATCH v4 2/3] dt-bindings: i3c: dw: Add apb reset

Quoted 34 lines · Krzysztof Kozlowski

Quoted 33 lines · Jisheng Zhang
Quoted 6 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang
Quoted 4 lines · Krzysztof Kozlowski
Quoted 2 lines · Jisheng Zhang
Add dt-binding for support of apb reset which is to reset the APB interface.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
And this is ABI break, so you must explain WHY breaking ABI is worth
This just adds an optional apb reset, it doesn't break any exisiting ABI. Kindly let me know whether adding new optional binding is also an ABI break.
It does. One reset was before. Now all devices must have two resets. Clear ABI impact.
I checked the dts with only one reset by manual dt_binding_check and dtbs_check, it still work, so I'm not sure why those dts "must have two resets". Kindly let me know what's wrong.
Quoted 12 lines
Quoted 9 lines
Quoted 5 lines
doing that or what is the impact. Additionally you should explain which devices have it. Does Altera have it? You really lack explanation WHY you are doing it and which hardware you exactly describe.
I'm preparing one of synaptics SoCs support to uptream, it needs this apb reset signal for i3c. So you mean I delay this series until the SoC upstream series come, right?
No. You just sent patch which basically says that Altera has two resets, without any explanation of that.

Quoted 8 lines

Quoted 4 lines
I guess you mixed other series/patches with mine here, no? I didn't mention Altera at all ;) What my series mentioned is adding apb reset which is to reset the APB interface.
Read the binding. Altera is already there. I do not talk about other patches.

... and then read exactly what your patch (not commit) is saying. It says among things that Altera has two resets, while commit is completely silent on that.

Quoted 2 lines

Your are changing an already accepted binding.

Best regards,
Krzysztof

Jisheng Zhang

author

2026-05-25 14:19 UTC #11 lore raw

PATCH v4 3/3 i3c: dw: Add apb reset support

In reply to Jisheng Zhang

Add support of apb reset which is to reset the APB interface.

Signed-off-by Jisheng Zhang <[email protected]>

---
 drivers/i3c/master/dw-i3c-master.c | 7 +++++++
 drivers/i3c/master/dw-i3c-master.h | 1 +
 2 files changed, 8 insertions(+)

drivers/i3c/master/dw-i3c-master.c +7 -0

index 655693a2187e..9de54d584bc3 100644
@@ -1591,6 +1591,11 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	if (IS_ERR(master->core_rst))
 		return PTR_ERR(master->core_rst);
 
+	master->apb_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
+									       "apb_rst");
+	if (IS_ERR(master->apb_rst))
+		return PTR_ERR(master->apb_rst);
+
 	spin_lock_init(&master->xferqueue.lock);
 	INIT_LIST_HEAD(&master->xferqueue.list);

@@ -1765,6 +1770,7 @@ static int __maybe_unused dw_i3c_master_runtime_suspend(struct device *dev)
 	dw_i3c_master_disable(master);
 
 	reset_control_assert(master->core_rst);
+	reset_control_assert(master->apb_rst);
 	dw_i3c_master_disable_clks(master);
 	pinctrl_pm_select_sleep_state(dev);
 	return 0;

@@ -1777,6 +1783,7 @@ static int __maybe_unused dw_i3c_master_runtime_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 	dw_i3c_master_enable_clks(master);
 	reset_control_deassert(master->core_rst);
+	reset_control_deassert(master->apb_rst);
 
 	dw_i3c_master_set_intr_regs(master);
 	dw_i3c_master_restore_timing_regs(master);

drivers/i3c/master/dw-i3c-master.h +1 -0

index c5cb695c16ab..a4ba60043288 100644
@@ -37,6 +37,7 @@ struct dw_i3c_master {
 	struct dw_i3c_master_caps caps;
 	void __iomem *regs;
 	struct reset_control *core_rst;
+	struct reset_control *apb_rst;
 	struct clk *core_clk;
 	struct clk *pclk;
 	char version[5];

Signature · 1 line

2.53.0

[email protected]

AI review

2026-05-25 14:49 UTC #12 lore raw

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Asymmetric reset assertion/deassertion order for APB and Core resets violates hardware sequencing rules.

Pre-existing issues:
- [High] Missing error handling for clock enablement and reset deassertion in runtime resume leads to potential kernel panics. --

commit f0e43b6549a854c56bb22e329a26e4ad0bcd7e1c
Author: Jisheng Zhang <[email protected]>
Subject: i3c: dw: Add apb reset support

This commit adds support for an APB interface reset in the DesignWare I3C master driver. It fetches the optional reset during probe and updates the runtime power management callbacks to assert and deassert it alongside the core reset.

Quoted 4 lines

drivers/i3c/master/dw-i3c-master.c +0 -0
index a7593d6efac5a..03efd46db8d0c 100644

[ ... ]

drivers/i3c/master/dw-i3c-master.c static int __maybe_unused dw_i3c_master_runtime_resume(struct device *dev) · line 1805

Quoted 13 lines

drivers/i3c/master/dw-i3c-master.c +2 -0

@@ -1784,6 +1789,7 @@ static int __maybe_unused dw_i3c_master_runtime_suspend(struct device *dev)
 	dw_i3c_master_disable(master);
 
 	reset_control_assert(master->core_rst);
+	reset_control_assert(master->apb_rst);
 	dw_i3c_master_disable_clks(master);
 	pinctrl_pm_select_sleep_state(dev);
 	return 0;

@@ -1796,6 +1802,7 @@ static int __maybe_unused dw_i3c_master_runtime_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 	dw_i3c_master_enable_clks(master);
 	reset_control_deassert(master->core_rst);
+	reset_control_deassert(master->apb_rst);

[Severity: High]
Should the reset deassertion order in resume be reversed to follow standard hardware sequencing?

Currently, the resets are asserted in the suspend path as core_rst followed by apb_rst, and they are deasserted in the resume path in the exact same order. Standard practice usually requires a last-in, first-out sequence.

Additionally, dw_i3c_common_probe() requests these resets in core_rst then apb_rst order. This means the automatic devres teardown will assert them as apb_rst then core_rst, which is the opposite of the manual runtime suspend logic introduced here.

Could this asymmetric sequencing cause bus hangs or IP initialization failures if a core is released from reset while its APB bus interface remains in reset?

[Severity: High]
This is a pre-existing issue, but this patch extends the pattern to the newly added apb_rst. Are we missing error handling for the clock enablement and reset deassertion?

The return values from dw_i3c_master_enable_clks() and both calls to reset_control_deassert() are being discarded. If any of these operations fail, the driver will proceed to configure the hardware by calling dw_i3c_master_set_intr_regs().

Would it be safer to check these return values and return an error code to the PM framework to avoid potential synchronous external aborts when accessing inactive IP registers?

Quoted 3 lines

drivers/i3c/master/dw-i3c-master.c +0 -0

 	dw_i3c_master_set_intr_regs(master);
 	dw_i3c_master_restore_timing_regs(master);

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3