Re: [PATCH v4 1/3] dt-bindings: edac: xlnx,versal-ddrmc: Add xlnx,versal-xilsem compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: edac: xlnx,versal-xilsem firmware service modeled as DDRMC MMIO node
--

commit 5dae9ede166dd1a988e3766f5755cc780a87b1f7
Author: Rama devi Veggalam <rama.devi.veggalam@amd.com>

dt-bindings: edac: xlnx,versal-ddrmc: Add xlnx,versal-xilsem compatible

This commit adds the xlnx,versal-xilsem compatible string to the Versal
memory controller binding in
Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
and gates the reg, reg-names, and interrupts constraints per compatible
string using allOf/if-then blocks.

> 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..b5dc32065e58d 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
> @@ -4,25 +4,35 @@
>  $id: http://devicetree.org/schemas/memory-controllers/xlnx,versal-ddrmc-edac.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Xilinx Versal DDRMC (Integrated DDR Memory Controller)
> +title: Xilinx Versal DDRMC (Integrated DDR Memory Controller) and XilSEM EDAC
>  
>  maintainers:
>    - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
>    - Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
> +  - Rama Devi Veggalam <rama.devi.veggalam@amd.com>
>  
>  description:
>    The integrated DDR Memory Controllers (DDRMCs) support both DDR4 and LPDDR4/
>    4X memory interfaces. Versal DDR memory controller has an optional ECC support
>    which correct single bit ECC errors and detect double bit ECC errors.
>  
> +  Xilinx Versal Soft Error Mitigation (XilSEM) is part of the Platform Loader
> +  and Manager (PLM) which is loaded into and runs on the Platform Management
> +  Controller (PMC). XilSEM is responsible for detecting and optionally
> +  correcting soft errors in Configuration Memory of Versal. Whenever an error
> +  is detected, be it correctable or uncorrectable, XilSEM notifies the errors
> +  to user applications via the firmware's event notification interface,
> +  rather than a devicetree-described interrupt.
> +
>  properties:
>    compatible:
> -    const: xlnx,versal-ddrmc
> +    enum:
> +      - xlnx,versal-ddrmc
> +      - xlnx,versal-xilsem
>  
>    reg:
> -    items:
> -      - description: DDR Memory Controller registers
> -      - description: NOC registers corresponding to DDR Memory Controller
> +    minItems: 1
> +    maxItems: 2
>  
>    reg-names:
>      items:
> @@ -35,8 +45,39 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - reg-names
> -  - interrupts
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: xlnx,versal-ddrmc
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: DDR Memory Controller registers
> +            - description: NOC registers corresponding to DDR Memory Controller
> +        reg-names:
> +          items:
> +            - const: base
> +            - const: noc
> +      required:
> +        - reg-names
> +        - interrupts
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: xlnx,versal-xilsem
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: XilSEM (Soft Error Mitigation) register region
> +        reg-names: false
> +        interrupts: false
>  
>  additionalProperties: false
>  
> @@ -55,3 +96,12 @@ examples:
>          interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
>        };
>      };
> +  - |
> +    bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      edac@f2014050 {
> +          compatible = "xlnx,versal-xilsem";
> +          reg = <0x0 0xf2014050 0x0 0xc4>;
> +      };
> +    };

[Severity: Medium]
Is Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
the right place to model xlnx,versal-xilsem when a devicetree defines this
node for xsem_edac_probe() in drivers/edac/versal_edac.c?

In Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml,
the description section notes that XilSem is part of the Platform Loader and
Manager running on the Platform Management Controller and notifies error
events via the firmware event notification interface rather than a
devicetree-described interrupt. In include/linux/firmware/xlnx-zynqmp.h,
enum pm_module_id defines XilSem as a firmware module:

enum pm_module_id {
	PM_MODULE_ID = 0x0,
	XPM_MODULE_ID = 0x2,
	XSEM_MODULE_ID = 0x3,
	TF_A_MODULE_ID = 0xa,
};

and its control and query operations in drivers/firmware/xilinx/zynqmp-sem.c
are performed through firmware calls.

In the examples section above, edac@f2014050 uses reg = <0x0 0xf2014050 0x0 0xc4>.
As shown in struct xsem_edac_priv in drivers/edac/xsem_edac.h and in
xsem_edac_probe() in drivers/edac/versal_edac.c (which maps resource 0 as
plmrtca_baseaddr), 0xf2014050 is a shared-memory offset within the firmware
Run-Time Configuration Area at 0xf2014000 in controller sram rather than a
dedicated hardware peripheral register block.

In addition, because XilSem monitors configuration memory and registers
rather than acting as a memory controller, it shares no hardware properties
with xlnx,versal-ddrmc. This forces the allOf section in
Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
to split reg, reg-names, and interrupts into mutually exclusive if/then
branches solely because the drivers were combined in
drivers/edac/versal_edac.c.

Should this firmware service instead be represented under
xlnx,versal-firmware or instantiated as a child platform device by the
firmware driver (similar to xlnx_event_manager in
drivers/firmware/xilinx/zynqmp.c), rather than modeling a firmware
shared-memory offset as a memory controller bus node in devicetree?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1792587442-2794880-1-git-send-email-rama.devi.veggalam@amd.com?part=1