Re: [PATCH 10/12] dt-bindings: ras: document estatus provider

[Ahmed Tiba <ahmed.tiba@arm.com>]

On 17/12/2025 12:41, Krzysztof Kozlowski wrote:
>> Add a binding for firmware-first CPER providers described via
>> DeviceTree. It covers the shared status block, optional acknowledgment
>> registers, interrupt versus polling modes and the SEA notification
>> flag so non-ACPI platforms can describe their error sources.
>>
>> Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>
>> ---
>>  .../devicetree/bindings/ras/arm,ras-ffh.yaml  | 95 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 96 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/ras/arm,ras-ffh.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/ras/arm,ras-ffh.yaml b/Documentation/devicetree/>bindings/ras/arm,ras-ffh.yaml
>> new file mode 100644
>> index 000000000000..0d2acbf8e8a8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ras/arm,ras-ffh.yaml
>
> What is ras? There is no such directory so some description would be
> useful. Usually you do not get your own directory per binding.

For the next revision I will move the schema under
`Documentation/devicetree/bindings/firmware/` and expand the description to
spell out that Arm Reliability, Availability and Serviceability (RAS) firmware
exposes this FFH CPER provider.

>> @@ -0,0 +1,95 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/ras/arm,ras-ffh.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Arm Firmware-First Handler (FFH) CPER provider
>> +
>> +maintainers:
>> +  - Ahmed Tiba <ahmed.tiba@arm.com>
>> +
>> +description: |
>> +  Some Arm platforms describe a firmware-first error handler that exposes a
>> +  Common Platform Error Record (CPER) buffer directly via DeviceTree. The OS
>> +  maps the buffer to consume the error records, and firmware signals that a new
>> +  record is ready either by asserting an interrupt or by relying on a periodic
>> +  poll. This binding describes the buffer and the associated notification
>
> Do not describe what the binding does. Describe the hardware or firmware.

I'll reword the description so it focuses on the firmware-managed CPER buffer,
optional doorbell register and optional interrupt rather than describing how
the binding is consumed.

>> +  signal. If firmware delivers the error via Synchronous External Abort (SEA),
>> +  the optional sea-notify flag marks the source accordingly.
>> +
>> +properties:
>> +  compatible:
>> +    const: arm,ras-ffh
>
> Again ras - what's that? Your patch or binding must explain that.

That updated description will explicitly expand the Arm RAS acronym so the
compatible string is self-explanatory.

>> +
>> +  reg:
>> +    minItems: 1
>
> Why is this flexible?

I'll keep `reg` describing the CPER status buffer, cap it at two entries, and
document the second entry as the optional doorbell register that some firmware
requires before reusing the buffer.

>> +    items:
>> +      - description: CPER status block exposed by firmware
>> +      - description:
>> +          Optional 32- or 64-bit acknowledgment register. Firmware watches this
>> +          register and expects bit 0 to be written to 1 once the OS consumes the
>> +          status buffer so it can reuse the record.
>> +
>> +  reg-names:
>> +    items:
>> +      - const: status
>> +      - const: ack
>
> Does not match reg.

`reg-names` will remain optional, but when provided the first entry will be
restricted to `"status"` and the second (if present) to `"ack"`, matching the
single optional region permitted in `reg`.

>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description:
>> +      Optional interrupt used to signal that a new status record is ready. If
>> +      omitted, the OS relies on the polling interval property.
>
> What OS is doing should not really matter. Either you have the interrupt
> or not.

I'll trim the wording so it just states that firmware may assert an interrupt
when a new record is ready.

>> +
>> +  poll-interval:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    description:
>> +      Optional polling interval, in milliseconds, for platforms that cannot
>> +      route an interrupt.
>
> That's OS policy, not suitable for binding.

I'll drop `poll-interval` from the binding so the driver can fall back to its
fixed interval when no interrupt is wired.

>> +
>> +  arm,sea-notify:
>> +    type: boolean
>> +    description:
>> +      Set if the platform delivers these errors as Synchronous External Aborts.
>
> This is implied by the compatible, no?

I'll drop `arm,sea-notify` so the compatible alone defines the behaviour.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        poll-interval: false
>> +    then:
>> +      required:
>> +        - interrupts
>> +  - if:
>> +      properties:
>> +        interrupts: false
>> +    then:
>> +      required:
>> +        - poll-interval
>> +  - if:
>> +      properties:
>> +        reg:
>> +          minItems: 2
>> +    then:
>> +      required:
>> +        - reg-names
>
>Drop all this.

I'll drop this block entirely.

>> +
>> +unevaluatedProperties: false
>
> I do not see any schema referenced.

I'll switch to `additionalProperties: false` so we rely solely on the schema
referenced by `$schema` while still rejecting unknown properties.

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    ras-ffh@fe800000 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.> html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).

I'll rename the example node to the generic `error-handler@fe800000` so it
describes the shared error-status block instead of the driver name.

>> +        compatible = "arm,ras-ffh";
>> +        reg = <0xfe800000 0x1000>,
>> +              <0xfe810000 0x4>;
>> +        reg-names = "status", "ack";
>> +        interrupts = <0 32 IRQ_TYPE_LEVEL_HIGH>;
>
> Use proper defines.

I'll also switch the example interrupt listing to `GIC_SPI` and `IRQ_TYPE_*`
macros.



Best regards,
Ahmed