Re: [PATCH RFC 1/3] dt-bindings: rtc: Add linux,wakeup-states and linux,alarmtimer-priority

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: rtc: OS-specific alarmtimer policy and sleep states in DT
--

commit dc818a3fb414914888d04f5807458bc227693b0c
Author: Igor Velkov <iav@iav.lv>

dt-bindings: rtc: Add linux,wakeup-states and linux,alarmtimer-priority

This commit adds the linux,wakeup-states and linux,alarmtimer-priority
properties to the common RTC Devicetree binding schema to describe per-RTC
sleep-state wakeup capabilities and alarmtimer selection weights, and allows
them in the amlogic,meson-vrtc and nxp,pcf8563 bindings.

> diff --git a/Documentation/devicetree/bindings/rtc/amlogic,meson-vrtc.yaml b/Documentation/devicetree/bindings/rtc/amlogic,meson-vrtc.yaml
> index a89865fa676a6..bc4a1916f6007 100644
> --- a/Documentation/devicetree/bindings/rtc/amlogic,meson-vrtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/amlogic,meson-vrtc.yaml
> @@ -30,6 +30,9 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  linux,alarmtimer-priority: true
> +  linux,wakeup-states: true
> +
>  required:
>    - compatible
>    - reg
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> index 22909a96123e5..5aabeb074c82b 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> @@ -32,6 +32,9 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  linux,alarmtimer-priority: true
> +  linux,wakeup-states: true
> +
>    start-year: true
>    wakeup-source: true
>  
> diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> index efb66df82782f..3b4b8ff90ada0 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> @@ -26,6 +26,35 @@ properties:
>        0: not chargeable
>        1: chargeable
>  
> +  linux,alarmtimer-priority:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Weight of this RTC for waking the system on alarmtimer alarms.
> +      Among the RTCs that can wake the system from the target sleep
> +      state, the one with the highest value is used; on a tie, the
> +      operating system decides. An RTC without this property has
> +      priority 0.
> +
> +  linux,wakeup-states:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description:
> +      System sleep states an alarm of this RTC wakes the system from,
> +      given the board and its firmware. s2idle, shallow and deep are the
> +      states of /sys/power/mem_sleep, hibernate is suspend-to-disk and
> +      off is the powered-off system, and none, given alone, means the
> +      alarm wakes the system from no state. An RTC without this property
> +      is taken to wake the system from every state.
> +    oneOf:
> +      - items:
> +          enum:
> +            - s2idle
> +            - shallow
> +            - deep
> +            - hibernate
> +            - "off"
> +      - items:
> +          - const: none

[Severity: Medium]
Should OS-specific alarmtimer policy weights and Linux /sys/power/mem_sleep
labels be encoded in the Devicetree binding rather than derived by the OS from
hardware properties and compatible strings?

Documentation/devicetree/bindings/writing-bindings.rst notes that bindings
should describe hardware rather than Linux or device driver specifics, and
should not add properties that are already implied by or deducible from the
compatible string.

For linux,alarmtimer-priority, this encodes an arbitrary numeric weight for
the Linux kernel alarmtimer subsystem (kernel/time/alarmtimer.c) into the DT
ABI. Could the kernel instead select the preferred wakeup RTC using hardware
characteristics already known to the RTC drivers (for example, alarm
resolution such as RTC_FEATURE_ALARM_RES_MINUTE in drivers/rtc/rtc-pcf8563.c
versus 1-second resolution, or alarm range), userspace policy, or standard DT
/aliases (rtc0, rtc1)?

For linux,wakeup-states, several of the state definitions appear to conflict
with hardware description or existing properties in rtc.yaml:

1. In Documentation/devicetree/bindings/rtc/amlogic,meson-vrtc.yaml, waking
the system only when the application processors suspend is inherent to the
hardware and firmware interface (a scratch register read by the SCP upon
platform suspend, as implemented in drivers/rtc/rtc-meson-vrtc.c), which is
already deducible from compatible = "amlogic,meson-vrtc" without a new DT
property.

2. The values s2idle, shallow, and deep are Linux sysfs labels from
/sys/power/mem_sleep rather than hardware properties. In particular, s2idle is
a pure Linux software freeze state where any enabled OS interrupt wakes the
system, whereas an RTC without an interrupts property wired to the kernel
cannot wake the system from s2idle. Similarly, on non-ACPI Devicetree
platforms, hibernate uses power-off rather than a distinct hardware sleep
state, and whether an RTC can wake the system from platform suspend or
power-off is determined by interrupt/wakeup routing and power domain or PMIC
wiring.

3. How does linux,wakeup-states interact with the existing wakeup-source
property in Documentation/devicetree/bindings/rtc/rtc.yaml? Specifying
linux,wakeup-states = "none" duplicates the absence of wakeup-source and
allows contradictory combinations such as setting both wakeup-source and
linux,wakeup-states = "none" in nxp,pcf8563.yaml. Conversely, stating that an
RTC without linux,wakeup-states wakes the system from every state (including
hibernate and off) overstates the capability of standard RTCs that cannot wake
a powered-off system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-b4-alarmtimer-rtc-priority-v1-0-e996681b8086@iav.lv?part=1