[PATCH v4 0/3] i3c: dw: Add apb reset support

[PATCH v4 0/3] i3c: dw: Add apb reset support

[Jisheng Zhang]

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(+)

-- 
2.53.0

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

[Jisheng Zhang]

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 <jszhang@kernel.org>
---
 .../devicetree/bindings/i3c/snps,dw-i3c-master.yaml       | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index e803457d3f55..613dce7757bc 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -35,6 +35,14 @@ properties:
       - const: core
       - const: apb
 
+  resets:
+    items:
+      - description: Reset signal
+
+  reset-names:
+    items:
+      - const: core_rst
+
   interrupts:
     maxItems: 1
 
-- 
2.53.0

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

[Krzysztof Kozlowski]

On 25/05/2026 16:00, Jisheng Zhang wrote:
> The "core_rst" reset support has been in the code from day1, but the

<Driver requests the "core_rst" since day1 (commit foo bar)....>

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

> 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 <krzysztof.kozlowski@oss.qualcomm.com>


Best regards,
Krzysztof

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

[Jisheng Zhang]

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

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
index 613dce7757bc..2575442b28ff 100644
--- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
@@ -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
-- 
2.53.0

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

[sashiko-bot]

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 <jszhang@kernel.org>

dt-bindings: i3c: dw: Add apb reset

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

> diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> index 613dce7757bc3..2575442b28ff4 100644
> --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> @@ -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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525140018.19598-1-jszhang@kernel.org?part=2

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

[Krzysztof Kozlowski]

On 25/05/2026 16:00, Jisheng Zhang wrote:
> 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.

> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> index 613dce7757bc..2575442b28ff 100644
> --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> @@ -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

>  
>    interrupts:
>      maxItems: 1


Best regards,
Krzysztof

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

[Jisheng Zhang]

On Mon, May 25, 2026 at 05:25:45PM +0200, Krzysztof Kozlowski wrote:
> On 25/05/2026 16:00, Jisheng Zhang wrote:
> > 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.

> 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?

> 
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> > index 613dce7757bc..2575442b28ff 100644
> > --- a/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> > +++ b/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.yaml
> > @@ -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
> 
> >  
> >    interrupts:
> >      maxItems: 1
> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 27/05/2026 06:18, Jisheng Zhang wrote:
> On Mon, May 25, 2026 at 05:25:45PM +0200, Krzysztof Kozlowski wrote:
>> On 25/05/2026 16:00, Jisheng Zhang wrote:
>>> 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.

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

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

[Jisheng Zhang]

On Wed, May 27, 2026 at 09:53:16AM +0200, Krzysztof Kozlowski wrote:
> On 27/05/2026 06:18, Jisheng Zhang wrote:
> > On Mon, May 25, 2026 at 05:25:45PM +0200, Krzysztof Kozlowski wrote:
> >> On 25/05/2026 16:00, Jisheng Zhang wrote:
> >>> 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.

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.

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

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.


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

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

[Krzysztof Kozlowski]

On 30/05/2026 14:58, Jisheng Zhang wrote:
> On Wed, May 27, 2026 at 09:53:16AM +0200, Krzysztof Kozlowski wrote:
>> On 27/05/2026 06:18, Jisheng Zhang wrote:
>>> On Mon, May 25, 2026 at 05:25:45PM +0200, Krzysztof Kozlowski wrote:
>>>> On 25/05/2026 16:00, Jisheng Zhang wrote:
>>>>> 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.
> 
> 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.
> 
>>
>>>
>>>> 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.
> 
> 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

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

[Krzysztof Kozlowski]

On 03/06/2026 16:36, Krzysztof Kozlowski wrote:
> On 30/05/2026 14:58, Jisheng Zhang wrote:
>> On Wed, May 27, 2026 at 09:53:16AM +0200, Krzysztof Kozlowski wrote:
>>> On 27/05/2026 06:18, Jisheng Zhang wrote:
>>>> On Mon, May 25, 2026 at 05:25:45PM +0200, Krzysztof Kozlowski wrote:
>>>>> On 25/05/2026 16:00, Jisheng Zhang wrote:
>>>>>> 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.
>>
>> 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.
>>
>>>
>>>>
>>>>> 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.
>>
>> 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.

> 
> Your are changing an already accepted binding.


Best regards,
Krzysztof

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

[Jisheng Zhang]

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

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/i3c/master/dw-i3c-master.c | 7 +++++++
 drivers/i3c/master/dw-i3c-master.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 655693a2187e..9de54d584bc3 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -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);
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index c5cb695c16ab..a4ba60043288 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -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];
-- 
2.53.0

Re: [PATCH v4 3/3] i3c: dw: Add apb reset support

[sashiko-bot]

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 <jszhang@kernel.org>
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.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..03efd46db8d0c 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -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?

>  
>  	dw_i3c_master_set_intr_regs(master);
>  	dw_i3c_master_restore_timing_regs(master);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525140018.19598-1-jszhang@kernel.org?part=3