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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/12/2025 12:28, Ahmed Tiba 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.

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

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

> +
> +  reg:
> +    minItems: 1

Why is this flexible?

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

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


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

> +
> +  arm,sea-notify:
> +    type: boolean
> +    description:
> +      Set if the platform delivers these errors as Synchronous External Aborts.

This is implied by the compatible, no?

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

> +
> +unevaluatedProperties: false

I do not see any schema referenced.

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

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

Use proper defines.

Best regards,
Krzysztof