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

[Michal Simek]

On 10/29/26 17:27, Borislav Petkov wrote:
> On Thu, Oct 29, 2026 at 05:12:44PM +0100, Michal Simek wrote:
> > With Versal Gen 2 which is still under development I can imagine that we can
> > design it in a way that we say that we won't allow any MMIO interface and
> > only firmware interface will be used and firmware itself will integrate all
> > edacs in the system and export it via the same API. Then we will have only
> > one single driver for the whole platform and it depends only on firmware
> > which functionality is going to be exported to Linux. Which is IMHO model
> > which you would like to see.
>
> I appreciate the intention to make it saner but ARM is insane by definition in
> that regard. You have IP blocks which can be built in in all kinds of
> platforms and sw needs to mimick that. For some EDAC drivers we even have .c
> compilation units which get linked into each platform which supports them.
>
> We would usually do that when you have too many EDAC wrapper drivers loading
> on the system and them needing to talk to each other. Which the subsystem
> hasn't really planned on supporting.
>
> But if the wrappers can be relatively independent, we can do the simple
> thing and do a separate wrapper for each. No biggie.
It is completely independent.

Regarding platform data versus DT which Yazen mentioned. I have played with it 
and pretty much it would look like this from firmware instantiation point of 
view. (active_platform_fw_data - already holds SOC specific hooks that's why 
they can be reused that zynqmp_pm_get_family_info() in edac is not needed.)

+static const struct resource versal_xilsem_res[] = {
+       DEFINE_RES_MEM(0xf2014050, 0xc4),
+};
+
 static const struct platform_fw_data platform_fw_data_versal = {
        .family_code = PM_VERSAL_FAMILY_CODE,
+       .xilsem_name = "versal-xilsem",
+       .xilsem_res = versal_xilsem_res,
 };

 static const struct platform_fw_data platform_fw_data_versal_net = {
        .family_code = PM_VERSAL_NET_FAMILY_CODE,
+       .xilsem_name = "versal-net-xilsem",
+       .xilsem_res = versal_xilsem_res,
 };

+static void zynqmp_firmware_add_xilsem(struct device *dev)
+{
+       const struct platform_fw_data *data = active_platform_fw_data;
+       u32 cfg[4];
+       int ret;
+
+       if (!data->xilsem_name || IS_ERR_OR_NULL(em_dev))
+               return;
+
+       ret = zynqmp_pm_xilsem_read_cfg(0, cfg);
+       if (ret)
+               return;
+
+       xilsem_dev = platform_device_register_resndata(dev, data->xilsem_name,
+                                                      PLATFORM_DEVID_NONE,
+                                                      data->xilsem_res, 1, 
NULL, 0);
+       if (IS_ERR(xilsem_dev))
+               dev_warn(dev, "XilSEM register fail with error %ld\n",
+                        PTR_ERR(xilsem_dev));
+}
+
 static int zynqmp_firmware_probe(struct platform_device *pdev)
 {
        struct device *dev = &pdev->dev;
@@ -2345,6 +2372,8 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
                        dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register 
fail with error\n");
        }

+       zynqmp_firmware_add_xilsem(&pdev->dev);
+
        return of_platform_populate(dev->of_node, NULL, NULL, dev);
 }


Edac driver would have


+static const struct xsem_cfg versal_xsem_cfg = {
+       .sw_event_node_id = VERSAL_EVENT_ERROR_SW_ERR,
+       .cram_ce_mask = XPM_VERSAL_EVENT_ERROR_MASK_XSEM_CRAM_CE_5,
+       .cram_ue_mask = XPM_VERSAL_EVENT_ERROR_MASK_XSEM_CRAM_UE_6,
+       .npi_ue_mask = XPM_VERSAL_EVENT_ERROR_MASK_XSEM_NPI_UE_7,
+};
+
+static const struct xsem_cfg versal_net_xsem_cfg = {
+       .sw_event_node_id = VERSAL_NET_EVENT_ERROR_SW_ERR,
+       .cram_ce_mask = XPM_VERSAL_NET_EVENT_ERROR_MASK_XSEM_CRAM_CE,
+       .cram_ue_mask = XPM_VERSAL_NET_EVENT_ERROR_MASK_XSEM_CRAM_UE,
+       .npi_ue_mask = XPM_VERSAL_NET_EVENT_ERROR_MASK_XSEM_NPI_UE,
+};

+static const struct platform_device_id xsem_edac_id_table[] = {
+       { "versal-xilsem", (kernel_ulong_t)&versal_xsem_cfg },
+       { "versal-net-xilsem", (kernel_ulong_t)&versal_net_xsem_cfg },
+       { }
+};
+MODULE_DEVICE_TABLE(platform, xsem_edac_id_table);
+
 static struct platform_driver xilinx_xsem_edac_driver = {
        .driver = {
                .name = "xilinx-xilsem-edac",
        },
+       .id_table = xsem_edac_id_table,
        .probe = xsem_edac_probe,
        .remove = xsem_edac_remove,
 };

No zynqmp_pm_get_family_info() calls and no DT binding.

Please let me know which way do you prefer.

Thanks,
Michal