Re: Re: [PATCH v12 4/4] dt-bindings: timer: dwc: Update resets property items

> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The timer driver ignores the newly documented APB interface reset (`presetn`), leading to system hangs if specified.
> --
> 
> --- Patch [4]: [PATCH v12 4/4] dt-bindings: timer: dwc: Update resets property items ---
> commit 0298afcc08fcedc8999619356e00017ff450203d
> Author: Xuyang Dong <[email protected]>
> 
> dt-bindings: timer: dwc: Update resets property items
> 
> This commit updates the snps,dw-apb-timer binding to allow up to two reset
> lines. It specifically documents the timer_N_resetn and presetn reset inputs
> to match the hardware capabilities of the DesignWare IP family.
> 
> > diff --git a/Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml b/Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml
> > index d33c9205a9093..4d30cd01ac65f 100644
> > --- a/Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml
> > +++ b/Documentation/devicetree/bindings/timer/snps,dw-apb-timer.yaml
> > @@ -25,7 +25,10 @@ properties:
> >      maxItems: 1
> >  
> >    resets:
> > -    maxItems: 1
> > +    minItems: 1
> > +    items:
> > +      - description: Timer ticks reference (timer_N_resetn) reset
> > +      - description: APB interface (presetn) reset
> 
> [Severity: High]
> Will this cause a system bus hang if a device tree is updated to specify
> both resets?
> 
> Looking at timer_get_base_and_rate() in
> drivers/clocksource/dw_apb_timer_of.c, the driver unconditionally fetches
> and deasserts only the first reset by passing NULL as the id:
> 
> drivers/clocksource/dw_apb_timer_of.c:timer_get_base_and_rate() {
>     ...
> 	/*
> 	 * Reset the timer if the reset control is available, wiping
> 	 * out the state the firmware may have left it
> 	 */
> 	rstc = of_reset_control_get(np, NULL);
> 	if (!IS_ERR(rstc)) {
> 		reset_control_assert(rstc);
> 		reset_control_deassert(rstc);
> 	}
>     ...
> }
> 
> If the device tree specifies both resets to match this newly added schema
> capability, the second reset (presetn, which controls the APB bus interface)
> will be completely ignored and left asserted.
> 
> Should the driver be updated to explicitly fetch and deassert both resets
> (e.g., via reset_control_bulk_get) before extending the binding, to avoid
> system hangs when memory-mapped registers are accessed?
> 

Hi Sashiko,

In the next version, replace of_reset_control_get() with 
of_reset_control_array_get_optional_exclusive() to ensure all resets 
can be deasserted.

Best regards,
Xuyang Dong

> >  
> >    clocks:
> >      minItems: 1
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4