Re: [PATCH v3 1/4] dt-bindings: edac: Add bindings for Xilinx Versal XilSEM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Adding a third mandatory item to `reg` and `reg-names` breaks backward compatibility for existing device trees, causing schema validation and driver probe failures.
- [Medium] Unrelated XilSEM hardware registers are incorrectly merged into the DDR Memory Controller (`xlnx,versal-ddrmc`) device tree node, violating DT design principles.
--

commit 4759b2e3e1d7757d4a16075824d7a25fd86d0c73
Author: Rama devi Veggalam <rama.devi.veggalam@amd.com>

dt-bindings: edac: Add bindings for Xilinx Versal XilSEM

Update versal edac device tree bindings for Versal Soft Error Mitigation
(XilSEM).

> diff --git a/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml b/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
> index 12f8e9f350bcf..568d2af7de81a 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
[ ... ]
> @@ -23,11 +37,13 @@ properties:
>      items:
>        - description: DDR Memory Controller registers
>        - description: NOC registers corresponding to DDR Memory Controller
> +      - description: SEM RTCA Controller registers

[Severity: Medium]
Is it correct to merge unrelated XilSEM hardware registers into the DDR Memory
Controller device tree node?

The device tree describes an inaccurate hardware topology by grouping physically
separate register bases under a single compatible string simply because a single
EDAC driver handles both. Should XilSEM be represented by its own distinct
device tree node?

[Severity: Critical]
Does adding this new required register break backward compatibility with older
device trees?

Because this adds a third mandatory item to the list without specifying
minItems: 2, older device trees lacking the "semrtca" register will fail
schema validation.

Additionally, does the driver unconditionally require this new register in
mc_probe(), causing probes to fail on existing systems? If the driver code
looks for the new register:

drivers/edac/versal_edac.c:mc_probe() {
    ...
    sem_baseaddr = devm_platform_ioremap_resource_byname(pdev, "semrtca");
    if (IS_ERR(sem_baseaddr))
        return PTR_ERR(sem_baseaddr);
    ...
}

This will return an error when the DT node lacks the semrtca register, leading
to immediate probe failure on existing setups.

>  
>    reg-names:
>      items:
>        - const: base
>        - const: noc
> +      - const: semrtca

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624212545.2850787-1-rama.devi.veggalam@amd.com?part=1