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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The timer driver is not updated to deassert the newly allowed second reset line, which can lead to system hangs.
- [Medium] Missing `reset-names` property prevents accurate hardware representation for partial reset implementations.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver asserts and deasserts the hardware reset before the corresponding clocks are enabled.
--

--- Patch [4]: [PATCH v13 4/5] dt-bindings: timer: dwc: Update resets property items ---
commit 10dcd30e56947333e8bfdc5270bc95e60b327052
Author: Xuyang Dong <dongxuyang@eswincomputing.com>

dt-bindings: timer: dwc: Update resets property items

The DesignWare DW_apb_timers IP has two active-low reset inputs:
presetn resets the register interface logic in the pclk (bus) domain,
and timer_N_resetn resets the counter/timer logic in the timer_N_clk
domain. The existing snps,dw-apb-timer binding only describes one.

Update snps,dw-apb-timer binding to allow up to two reset lines,
matching 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: Medium]
Since the resets array was expanded to allow multiple items, should a
reset-names property be added to allow disambiguation?

Because the schema enforces additionalProperties: false, device trees cannot
currently add names. If a platform only has software control over the APB
reset (presetn) and not the timer reset, it cannot correctly represent the
hardware because array bindings without names require index 0 to be populated
first.

[Severity: High]
Does the driver need to be updated to handle this new reset line?

If a device tree is updated to specify both resets, the timer driver only
fetches and deasserts the first one (index 0):

drivers/clocksource/dw_apb_timer_of.c:timer_get_base_and_rate() {
        ...
        rstc = of_reset_control_get(np, NULL);
        if (!IS_ERR(rstc)) {
                reset_control_assert(rstc);
                reset_control_deassert(rstc);
        }
        ...
}

If a platform requires the second reset (presetn) to be deasserted by the
kernel to access the APB interface, the timer registers will remain
inaccessible. Can this lead to a bus hang or external abort?

[Severity: High]
This is a pre-existing issue, but the driver asserts and deasserts the
hardware reset before the corresponding clocks are enabled:

drivers/clocksource/dw_apb_timer_of.c:timer_get_base_and_rate() {
        ...
        rstc = of_reset_control_get(np, NULL);
        if (!IS_ERR(rstc)) {
                reset_control_assert(rstc);
                reset_control_deassert(rstc);
        }

        /* ... */
        pclk = of_clk_get_by_name(np, "pclk");
        ...
}

The DW APB timer reset signals operate in their respective clock domains.
Toggling a synchronous reset while the associated clock is disabled means
the target flip-flops never receive the clock edges necessary to process
the reset.

Could this leave the timer in an undefined state if the bootloader left
it corrupted with clocks disabled?

>  
>    clocks:
>      minItems: 1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724084153.1473-1-dongxuyang@eswincomputing.com?part=4