Hi Krzysztof,

-----Original Message-----
From: Krzysztof Kozlowski <redacted>
Sent: Tuesday, August 16, 2022 1:36 PM
To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Rob Herring
[off-list ref]; Krzysztof Kozlowski
[off-list ref]; Michal Simek
[off-list ref]; Borislav Petkov [off-list ref]; Mauro
Carvalho Chehab [off-list ref]; Tony Luck [off-list ref];
James Morse [off-list ref]; Robert Richter [off-list ref]
Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; linux-edac@vger.kernel.org;
saikrishna12468@gmail.com; git (AMD-Xilinx) [off-list ref]; Datta,
Shubhrajyoti [off-list ref]
Subject: Re: [PATCH 2/2] edac: zynqmp_ocm: Add EDAC support for ZynqMP
OCM

On 16/08/2022 10:32, Sai Krishna Potthuri wrote:

quoted

Add EDAC support for Xilinx ZynqMP OCM Controller, this driver reports
CE and UE errors based on the interrupts, and also creates ue/ce sysfs
entries for error injection.

Signed-off-by: Sai Krishna Potthuri <sai.krishna.potthuri@amd.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

A bit confusing SoB order, although sometimes rational. Are you sure about
authorship here?

Yes, I am the author of this file and Shubhrajyoti contributed to this.

quoted

---
 MAINTAINERS                    |   7 +
 drivers/edac/Kconfig           |   9 +
 drivers/edac/Makefile          |   1 +
 drivers/edac/zynqmp_ocm_edac.c | 643
+++++++++++++++++++++++++++++++++
 4 files changed, 660 insertions(+)
 create mode 100644 drivers/edac/zynqmp_ocm_edac.c

diff --git a/MAINTAINERS b/MAINTAINERS index

edc96cdb85e8..cd4c6c90bca3 100644

--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -21692,6 +21692,13 @@ F:

	Documentation/devicetree/bindings/dma/xilinx/xlnx,zynqmp-
dpdma.yaml

quoted

 F:	drivers/dma/xilinx/xilinx_dpdma.c
 F:	include/dt-bindings/dma/xlnx-zynqmp-dpdma.h

+XILINX ZYNQMP OCM EDAC DRIVER
+M:	Shubhrajyoti Datta [off-list ref]
+M:	Sai Krishna Potthuri [off-list ref]
+S:	Maintained
+F:	Documentation/devicetree/bindings/edac/xlnx,zynqmp-ocmc.yaml
+F:	drivers/edac/zynqmp_ocm_edac.c
+
 XILINX ZYNQMP PSGTR PHY DRIVER
 M:	Anurag Kumar Vulisha [off-list ref]
 M:	Laurent Pinchart [off-list ref]

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index

58ab63642e72..fece60f586af 100644

--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig

@@ -539,4 +539,13 @@ config EDAC_DMC520
 	  Support for error detection and correction on the
 	  SoCs with ARM DMC-520 DRAM controller.

+config EDAC_ZYNQMP_OCM
+	tristate "Xilinx ZynqMP OCM Controller"
+	depends on ARCH_ZYNQMP

|| COMPILE_TEST

I will fix in v2.

quoted

+	help
+	  Support for error de

quoted

+/**
+ * zynqmp_ocm_edac_get_eccstate - Return the controller ecc status
+ * @base:	Pointer to the ddr memory controller base address
+ *
+ * Get the ECC enable/disable status for the controller
+ *
+ * Return: ecc status 0/1.
+ */
+static bool zynqmp_ocm_edac_get_eccstate(void __iomem *base) {
+	return readl(base + ECC_CTRL_OFST) & OCM_ECC_ENABLE_MASK; }
+
+static const struct of_device_id zynqmp_ocm_edac_match[] = {
+	{ .compatible = "xlnx,zynqmp-ocmc-1.0"},
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, zynqmp_ocm_edac_match);

This goes to the end. Do not embed static variables in the middle of the code.

I will fix in v2.

quoted

+
+/**
+ * zynqmp_set_ocm_edac_sysfs_attributes - create sysfs attributes
+ * @edac_dev:	Pointer to the edac device struct
+ *
+ * Creates sysfs entries for error injection  */ static void
+zynqmp_set_ocm_edac_sysfs_attributes(struct edac_device_ctl_info
+						*edac_dev)
+{
+	edac_dev->sysfs_attributes = zynqmp_ocm_edac_sysfs_attributes; }
+
+/**
+ * zynqmp_ocm_edac_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Probes a specific controller instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise error code.
+ */

Drop the kerneldoc for probe(). It's obvious and exactly the same
everywhere. You could keep it if you write something different than theh
same message for 1000 other probes.

I will fix in v2.

quoted

+static int zynqmp_ocm_edac_probe(struct platform_device *pdev) {
+	struct zynqmp_ocm_edac_priv *priv;
+	struct edac_device_ctl_info *dci;
+	void __iomem *baseaddr;
+	struct resource *res;
+	int irq, ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	baseaddr = devm_ioremap_resource(&pdev->dev, res);

There is a wrapper for these.

I will fix in v2.

quoted

+	if (IS_ERR(baseaddr))
+		return PTR_ERR(baseaddr);
+
+	if (!zynqmp_ocm_edac_get_eccstate(baseaddr)) {
+		edac_printk(KERN_INFO, EDAC_DEVICE,
+			    "ECC not enabled - Disabling EDAC driver\n");

How do you disable the driver? What if there are two devices - how does this
disables the driver for second device?

Here I am checking the ECC capability of the controller, if ecc is enabled then
i will proceed with the probe otherwise return from here.
If there are two devices, then probe will be called twice, and each device has its
own capabilities.
"Disabling EDAC driver" statement might creating the confusion, i will remove
that statement.

quoted

+		return -ENXIO;
+	}
+
+	dci = edac_device_alloc_ctl_info(sizeof(*priv),

ZYNQMP_OCM_EDAC_STRING,

quoted

+					 1, ZYNQMP_OCM_EDAC_STRING, 1,

0, NULL, 0,

quoted

+					 edac_device_alloc_index());
+	if (!dci) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "Unable to allocate EDAC device\n");

No ENOMEM prints.

I will fix in v2.

quoted

+		return -ENOMEM;
+	}
+
+	priv = dci->pvt_info;
+	platform_set_drvdata(pdev, dci);
+	dci->dev = &pdev->dev;
+	priv->baseaddr = baseaddr;
+	dci->mod_name = pdev->dev.driver->name;
+	dci->ctl_name = ZYNQMP_OCM_EDAC_STRING;
+	dci->dev_name = dev_name(&pdev->dev);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		edac_printk(KERN_ERR, EDAC_DEVICE,
+			    "No irq %d in DT\n", irq);

The same, no need for printks. Core does it.

I will fix in v2.

quoted

+		ret = irq;
+		goto free_dev_ctl;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq,
+			       zynqmp_ocm_edac_intr_handler,
+			       0, dev_name(&pdev->dev), dci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_DEVICE, "Failed to request

Irq\n");

quoted

+		goto free_dev_ctl;
+	}
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IEN_OFST);
+
+	zynqmp_set_ocm_edac_sysfs_attributes(dci);
+	ret = edac_device_add_device(dci);
+	if (ret)
+		goto free_dev_ctl;
+
+	return 0;
+
+free_dev_ctl:
+	edac_device_free_ctl_info(dci);
+
+	return ret;
+}
+
+/**
+ * zynqmp_ocm_edac_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */

Same comment for kerneldoc.

I will fix in v2.

Regards
Sai Krishna

quoted

+static int zynqmp_ocm_edac_remove(struct platform_device *pdev) {
+	struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
+	struct zynqmp_ocm_edac_priv *priv = dci->pvt_info;
+
+	writel(OCM_CEUE_MASK, priv->baseaddr + OCM_IDS_OFST);
+	edac_device_del_device(&pdev->dev);
+	edac_device_free_ctl_info(dci);
+
+	return 0;
+}
+
+static struct platform_driver zynqmp_ocm_edac_driver = {
+	.driver = {
+		   .name = "zynqmp-ocm-edac",
+		   .of_match_table = zynqmp_ocm_edac_match,
+		   },
+	.probe = zynqmp_ocm_edac_probe,
+	.remove = zynqmp_ocm_edac_remove,
+};
+
+module_platform_driver(zynqmp_ocm_edac_driver);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("ZynqMP OCM ECC driver");

MODULE_LICENSE("GPL");


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel