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

[Michal Simek <michal.simek@amd.com>]

Hi,

On 10/28/26 05:48, Veggalam, Rama Devi wrote:
> AMD General
> 
> Hi Yazen,
> 
> I am evaluating both approaches and will get back to you.
> 
> Thanks,
> Rama
> 
>> -----Original Message-----
>> From: Ghannam, Yazen <Yazen.Ghannam@amd.com>
>> Sent: Tuesday, October 27, 2026 2:09 AM
>> To: Rob Herring <robh@kernel.org>
>> Cc: Veggalam, Rama Devi <rama.devi.veggalam@amd.com>; bp@alien8.de;
>> tony.luck@intel.com; Simek, Michal <michal.simek@amd.com>;
>> krzk+dt@kernel.org; conor+dt@kernel.org; linux-kernel@vger.kernel.org; linux-
>> edac@vger.kernel.org; devicetree@vger.kernel.org; james.morse@arm.com;
>> mchehab@kernel.org; rric@kernel.org; git (AMD-Xilinx) <git@amd.com>
>> Subject: Re: [PATCH v4 1/3] dt-bindings: edac: xlnx,versal-ddrmc: Add
>> xlnx,versal-xilsem compatible
>>
>> On Thu, Oct 22, 2026 at 09:37:30AM -0500, Rob Herring wrote:
>>> On Wed, Oct 21, 2026 at 06:27:20PM +0530, Rama devi Veggalam wrote:
>>>> Add the "xlnx,versal-xilsem" compatible string for the Versal XilSEM
>>>> (Xilinx Soft Error Mitigation) hardware block.
>>>>
>>>> XilSEM runs as part of the Platform Loader and Manager (PLM) on the
>>>> PMC and detects (and optionally corrects) soft errors in Versal
>>>> Configuration Memory, notifying error events to user applications.
>>>>
>>>> The new compatible string only needs a single register region and
>>>> does not use reg-names or interrupts, so the schema gates these
>>>> requirements per compatible string.
>>>>
>>>> Signed-off-by: Rama devi Veggalam <rama.devi.veggalam@amd.com>
>>>> ---
>>>> Changes in v4:
>>>> - Added a new "xlnx,versal-xilsem" compatible string to the existing
>>>>    xlnx,versal-ddrmc-edac.yaml binding, gated via allOf/if-then so
>>>>    DDRMC and XilSEM requirements (reg, reg-names, interrupts) are
>>>>    validated independently per compatible string.
>>>>
>>>> Changes in v3:
>>>> - Merged XilSEM edac with Versal Edac
>>>>
>>>> Changes in v2:
>>>> - Changed "xlnx,versal-xilsem-edac" to constant
>>>> - Removed "compatible: in required section
>>>> - Removed "|" in description
>>>> - Removed "items" in compatible
>>>> - Fixed indentation in examples
>>>> - Updated title and description
>>>> ---
>>>>   .../memory-controllers/xlnx,versal-ddrmc-edac.yaml | 64
>>>> +++++++++++++++++++---
>>>>   1 file changed, 57 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-d
>>>> drmc-edac.yaml
>>>> b/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-d
>>>> drmc-edac.yaml
>>>> index 12f8e9f..b5dc320 100644
>>>> ---
>>>> a/Documentation/devicetree/bindings/memory-controllers/xlnx,versal-d
>>>> drmc-edac.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/xlnx,vers
>>>> +++ al-ddrmc-edac.yaml
>>>> @@ -4,25 +4,35 @@
>>>>   $id:
>>>> http://devicetree.org/schemas/memory-controllers/xlnx,versal-ddrmc-e
>>>> dac.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
>>>
>>> These 2 devices have absolutely 0 properties in common. It makes no
>>> sense to share a binding doc.
>>>
>>
>> Right, and maybe DT isn't needed at all?
>>
>> One option in the Sashiko feedback is to use a platform driver.
>>
>> IIUC, here's a summary:
>> 1) Drop this DT patch.
>> 2) Have ZYNQMP_FIRMWARE create a platform driver for XilSEM.
>> 3) Have the Versal EDAC module use the XilSEM platform driver.
>>
>> Rama, what do you think?
Let me jump to it and start that discussion again
https://lore.kernel.org/all/20250814102909.GBaJ26dZzvSs9usTKm@fat_crate.local/

"EDAC drivers have support for multiple hw RAS components in a single driver.

The stuff you wanna drive is still part of Versal, right?

So what's the problem with adding it to versal_edac?"

versal_edac/versalnet_edac and drivers for memory controller.
Xilsem is not for memory controller. It's purpose is to check CRAM 
(Configuration RAM, which holds the programmable logic configuration) and NOC 
NPIs (Network On Chip Programming Interface).

Please correct me if I am wrong but today edac core only checks that there 
should be only one MC edac but there could be multiple EDAC devices.
XILSEM edac device is present on Versal and Versal NET SOCs.
https://docs.amd.com/r/en-US/oslib_rm/Tool-Configuration-for-XilSEM
(Actually also on Versal Gen 2 but firmware interface is going to be different).

I don't think we should try to integrate EDAC memory controller with EDAC device 
because their usage is independent of each other.


Second part regarding platform_driver with DT or not.

XILSEM has two configurations depending on how many SLRs (Super Logic Regions) 
are used. Low level firmware (PLM - Platform Loader and Manager, the platform 
firmware) is configured different based on amount of SLRs.

- In case of one SLR MMIO access is used.
- If there are multiple SLRs (master and slaves) the only way
how to access data is via firmware interface where SLR ID is passed.
Also there is no need to have MMIO.

If I go back to platform_driver data and instantiation.

There are definitely 2 ways how to bind the driver
- current DT description
- or passing physical address via platform_device_register_resndata() from 
firmware driver, without DT.

Both ways are valid and would work. Which one do you prefer?

Thanks,
Michal