Re: [PATCH v7 2/2] EDAC/versal: Add a Xilinx Versal memory controller driver

[Borislav Petkov <bp@xxxxxxxxx>]

On Wed, Jun 14, 2023 at 09:58:52AM +0530, Shubhrajyoti Datta wrote:
>  MAINTAINERS                          |    7 +
>  drivers/edac/Kconfig                 |   11 +
>  drivers/edac/Makefile                |    1 +
>  drivers/edac/versal_edac.c           | 1065 ++++++++++++++++++++++++++
>  include/linux/firmware/xlnx-zynqmp.h |   10 +
>  5 files changed, 1094 insertions(+)
>  create mode 100644 drivers/edac/versal_edac.c

I've done some changes ontop, see below:

That toggling of interrupts at the end of mc_probe() happens only for
debugfs's sake so they can move inside the ifdef.

Right?

---
diff --git a/MAINTAINERS b/MAINTAINERS
index 4f3514e8116a..569c48368458 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23410,7 +23410,7 @@ M:	Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx>
 M:	Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
 S:	Maintained
 F:	Documentation/devicetree/bindings/memory-controllers/xlnx,versal-ddrmc-edac.yaml
-F:	drivers/edac/xilinx_ddrmc_edac.c
+F:	drivers/edac/versal_edac.c
 
 XILINX WATCHDOG DRIVER
 M:	Srinivas Neeli <srinivas.neeli@xxxxxxx>
diff --git a/drivers/edac/versal_edac.c b/drivers/edac/versal_edac.c
index 4aa073ffa827..07a07641172c 100644
--- a/drivers/edac/versal_edac.c
+++ b/drivers/edac/versal_edac.c
@@ -195,7 +195,7 @@ union edac_info {
 struct ecc_status {
 	union ecc_error_info ceinfo[2];
 	union ecc_error_info ueinfo[2];
-	bool channel;
+	u8 channel;
 	u8 error_type;
 };
 
@@ -637,38 +637,38 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 	init_csrows(mci);
 }
 
-static void enable_intr(struct edac_priv *priv)
+static void disable_all_intr(struct edac_priv *priv)
 {
 	/* Unlock the PCSR registers */
 	writel(PCSR_UNLOCK_VAL, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET);
 
-	/* Enable UE and CE Interrupts to support the interrupt case */
-	writel(XDDR_IRQ_CE_MASK | XDDR_IRQ_UE_MASK,
-	       priv->ddrmc_baseaddr + XDDR_IRQ_EN_OFFSET);
+	writel(XDDR_IRQ_ALL,
+	       priv->ddrmc_baseaddr + XDDR_IRQ_DIS_OFFSET);
+	writel(XDDR_IRQ_ALL,
+	       priv->ddrmc_baseaddr + XDDR_IRQ1_DIS_OFFSET);
 
-	writel(XDDR_IRQ_UE_MASK,
-	       priv->ddrmc_baseaddr + XDDR_IRQ1_EN_OFFSET);
 	/* Lock the PCSR registers */
 	writel(1, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET);
 }
 
-static void disable_all_intr(struct edac_priv *priv)
+#ifdef CONFIG_EDAC_DEBUG
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+static void enable_intr(struct edac_priv *priv)
 {
 	/* Unlock the PCSR registers */
 	writel(PCSR_UNLOCK_VAL, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET);
 
-	writel(XDDR_IRQ_ALL,
-	       priv->ddrmc_baseaddr + XDDR_IRQ_DIS_OFFSET);
-	writel(XDDR_IRQ_ALL,
-	       priv->ddrmc_baseaddr + XDDR_IRQ1_DIS_OFFSET);
+	/* Enable UE and CE Interrupts to support the interrupt case */
+	writel(XDDR_IRQ_CE_MASK | XDDR_IRQ_UE_MASK,
+	       priv->ddrmc_baseaddr + XDDR_IRQ_EN_OFFSET);
 
+	writel(XDDR_IRQ_UE_MASK,
+	       priv->ddrmc_baseaddr + XDDR_IRQ1_EN_OFFSET);
 	/* Lock the PCSR registers */
 	writel(1, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET);
 }
 
-#ifdef CONFIG_EDAC_DEBUG
-#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
-
 /**
  * poison_setup - Update poison registers.
  * @priv:	DDR memory controller private instance data.
@@ -787,7 +787,7 @@ static const struct file_operations xddr_inject_enable_fops = {
 	.llseek = generic_file_llseek,
 };
 
-static void edac_create_debugfs_attributes(struct mem_ctl_info *mci)
+static void create_debugfs_attributes(struct mem_ctl_info *mci)
 {
 	struct edac_priv *priv = mci->pvt_info;
 
@@ -1012,13 +1012,12 @@ static int mc_probe(struct platform_device *pdev)
 		goto del_mc;
 	}
 
-	disable_all_intr(priv);
 #ifdef CONFIG_EDAC_DEBUG
-	edac_create_debugfs_attributes(mci);
-
+	disable_all_intr(priv);
+	create_debugfs_attributes(mci);
 	setup_address_map(priv);
-#endif
 	enable_intr(priv);
+#endif
 	return rc;
 
 del_mc:

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette