[PATCH v4 0/4] EDAC: Enhancements to Synopsys EDAC driver

[PATCH v4 0/4] EDAC: Enhancements to Synopsys EDAC driver

[Manish Narani]

This patch series enhances the current EDAC driver to support different
platforms.This series adds support for ZynqMP DDRC controller in synopsys
EDAC driver. This series also adds Device tree properties and relevant
binding documentation.

Changes in v2:
	- Moved checking of DDR_ECC_INTR_SUPPORT from (1/4) to (3/4) as it is
	  a feature of ZynqMP DDRC
	- The Binding Documentation in (2/4) is modified as per the review
	  comments

Changes in v3:
	- The commit message in (2/4) is modified (Synopsys EDAC Driver -->
	  ZynqMP DDRC)

Changes in v4:
	- Updated the commit message in (1/4)
	- Renamed function pointer names removing 'synps_' in (1/4)
	- Shortened unnecessary long lines as per the review comment on (1/4)

Manish Narani (4):
  edac: synps: Add platform specific structures for ddrc controller
  dt: bindings: Document ZynqMP DDRC in Synopsys documentation
  edac: synopsys: Add EDAC ECC support for ZynqMP DDRC
  arm64: zynqmp: Add DDRC node

 .../bindings/memory-controllers/synopsys.txt       |  27 +-
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi             |   7 +
 drivers/edac/Kconfig                               |   2 +-
 drivers/edac/synopsys_edac.c                       | 945 ++++++++++++++++++++-
 4 files changed, 941 insertions(+), 40 deletions(-)

-- 
2.1.1

[PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc controller

[Manish Narani]

Add platform specific structures, so that we can add different IP
support later using quirks.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/edac/synopsys_edac.c | 83 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 18 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 0c9c59e..b3c54e7 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -22,6 +22,7 @@
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 #include "edac_module.h"
 
@@ -130,6 +131,7 @@ struct synps_ecc_status {
  * @baseaddr:	Base address of the DDR controller
  * @message:	Buffer for framing the event specific info
  * @stat:	ECC status information
+ * @p_data:	Pointer to platform data
  * @ce_cnt:	Correctable Error count
  * @ue_cnt:	Uncorrectable Error count
  */
@@ -137,24 +139,47 @@ struct synps_edac_priv {
 	void __iomem *baseaddr;
 	char message[SYNPS_EDAC_MSG_SIZE];
 	struct synps_ecc_status stat;
+	const struct synps_platform_data *p_data;
 	u32 ce_cnt;
 	u32 ue_cnt;
 };
 
 /**
+ * struct synps_platform_data -  synps platform data structure
+ * @edac_geterror_info:	function pointer to synps edac error info
+ * @edac_get_mtype:	function pointer to synps edac mtype
+ * @edac_get_dtype:	function pointer to synps edac dtype
+ * @edac_get_eccstate:	function pointer to synps edac eccstate
+ * @quirks:		to differentiate IPs
+ */
+struct synps_platform_data {
+	int (*edac_geterror_info)(struct synps_edac_priv *priv);
+	enum mem_type (*edac_get_mtype)(const void __iomem *base);
+	enum dev_type (*edac_get_dtype)(const void __iomem *base);
+	bool (*edac_get_eccstate)(void __iomem *base);
+	int quirks;
+};
+
+/**
  * synps_edac_geterror_info - Get the current ecc error info
- * @base:	Pointer to the base address of the ddr memory controller
- * @p:		Pointer to the synopsys ecc status structure
+ * @priv:	Pointer to DDR memory controller private instance data
  *
  * Determines there is any ecc error or not
  *
  * Return: one if there is no error otherwise returns zero
  */
-static int synps_edac_geterror_info(void __iomem *base,
-				    struct synps_ecc_status *p)
+static int synps_edac_geterror_info(struct synps_edac_priv *priv)
 {
+	void __iomem *base;
+	struct synps_ecc_status *p;
 	u32 regval, clearval = 0;
 
+	if (!priv)
+		return 1;
+
+	base = priv->baseaddr;
+	p = &priv->stat;
+
 	regval = readl(base + STAT_OFST);
 	if (!regval)
 		return 1;
@@ -240,9 +265,10 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
 static void synps_edac_check(struct mem_ctl_info *mci)
 {
 	struct synps_edac_priv *priv = mci->pvt_info;
+	const struct synps_platform_data *p_data = priv->p_data;
 	int status;
 
-	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
+	status = p_data->edac_geterror_info(priv);
 	if (status)
 		return;
 
@@ -362,6 +388,7 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
 	struct csrow_info *csi;
 	struct dimm_info *dimm;
 	struct synps_edac_priv *priv = mci->pvt_info;
+	const struct synps_platform_data *p_data = priv->p_data;
 	u32 size;
 	int row, j;
 
@@ -370,12 +397,13 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
 		size = synps_edac_get_memsize();
 
 		for (j = 0; j < csi->nr_channels; j++) {
-			dimm            = csi->channels[j]->dimm;
+			dimm = csi->channels[j]->dimm;
 			dimm->edac_mode = EDAC_FLAG_SECDED;
-			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
-			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi->nr_channels;
-			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
-			dimm->dtype     = synps_edac_get_dtype(priv->baseaddr);
+			dimm->mtype = p_data->edac_get_mtype(priv->baseaddr);
+			dimm->nr_pages = (size >> PAGE_SHIFT) /
+						csi->nr_channels;
+			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
+			dimm->dtype = p_data->edac_get_dtype(priv->baseaddr);
 		}
 	}
 
@@ -423,6 +451,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	return status;
 }
 
+static const struct synps_platform_data zynq_edac_def = {
+	.edac_geterror_info	= synps_edac_geterror_info,
+	.edac_get_mtype		= synps_edac_get_mtype,
+	.edac_get_dtype		= synps_edac_get_dtype,
+	.edac_get_eccstate	= synps_edac_get_eccstate,
+	.quirks			= 0,
+};
+
+static const struct of_device_id synps_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
 /**
  * synps_edac_mc_probe - Check controller and bind driver
  * @pdev:	Pointer to the platform_device struct
@@ -440,13 +483,22 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 	int rc;
 	struct resource *res;
 	void __iomem *baseaddr;
+	const struct of_device_id *match;
+	const struct synps_platform_data *p_data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	baseaddr = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(baseaddr))
 		return PTR_ERR(baseaddr);
 
-	if (!synps_edac_get_eccstate(baseaddr)) {
+	match = of_match_node(synps_edac_match, pdev->dev.of_node);
+	if (!match && !match->data) {
+		dev_err(&pdev->dev, "of_match_node() failed\n");
+		return -EINVAL;
+	}
+
+	p_data = (struct synps_platform_data *)match->data;
+	if (!(p_data->edac_get_eccstate(baseaddr))) {
 		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
 		return -ENXIO;
 	}
@@ -468,6 +520,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 
 	priv = mci->pvt_info;
 	priv->baseaddr = baseaddr;
+	priv->p_data = match->data;
+
 	rc = synps_edac_mc_init(mci, pdev);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC,
@@ -511,13 +565,6 @@ static int synps_edac_mc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id synps_edac_match[] = {
-	{ .compatible = "xlnx,zynq-ddrc-a05", },
-	{ /* end of table */ }
-};
-
-MODULE_DEVICE_TABLE(of, synps_edac_match);
-
 static struct platform_driver synps_edac_mc_driver = {
 	.driver = {
 		   .name = "synopsys-edac",
-- 
2.1.1

RE: [PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc controller

[Manish Narani]

Ping.
 
> -----Original Message-----
> From: Manish Narani [mailto:manish.narani@xilinx.com]
> Sent: Saturday, August 4, 2018 2:56 PM
> To: robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Michal Simek <michals@xilinx.com>; bp@alien8.de;
> mchehab@kernel.org; mdf@kernel.org; Edgar Iglesias <edgari@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Naga Sureshkumar Relli
> <nagasure@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>;
> stefan.krsmanovic@aggios.com
> Cc: Srinivas Goud <sgoud@xilinx.com>; Anirudha Sarangi
> <anirudh@xilinx.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> edac@vger.kernel.org; Manish Narani <MNARANI@xilinx.com>
> Subject: [PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc
> controller
> 
> Add platform specific structures, so that we can add different IP support later
> using quirks.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/edac/synopsys_edac.c | 83 ++++++++++++++++++++++++++++++++++--
> --------
>  1 file changed, 65 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index
> 0c9c59e..b3c54e7 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -22,6 +22,7 @@
>  #include <linux/edac.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
> 
>  #include "edac_module.h"
> 
> @@ -130,6 +131,7 @@ struct synps_ecc_status {
>   * @baseaddr:	Base address of the DDR controller
>   * @message:	Buffer for framing the event specific info
>   * @stat:	ECC status information
> + * @p_data:	Pointer to platform data
>   * @ce_cnt:	Correctable Error count
>   * @ue_cnt:	Uncorrectable Error count
>   */
> @@ -137,24 +139,47 @@ struct synps_edac_priv {
>  	void __iomem *baseaddr;
>  	char message[SYNPS_EDAC_MSG_SIZE];
>  	struct synps_ecc_status stat;
> +	const struct synps_platform_data *p_data;
>  	u32 ce_cnt;
>  	u32 ue_cnt;
>  };
> 
>  /**
> + * struct synps_platform_data -  synps platform data structure
> + * @edac_geterror_info:	function pointer to synps edac error info
> + * @edac_get_mtype:	function pointer to synps edac mtype
> + * @edac_get_dtype:	function pointer to synps edac dtype
> + * @edac_get_eccstate:	function pointer to synps edac eccstate
> + * @quirks:		to differentiate IPs
> + */
> +struct synps_platform_data {
> +	int (*edac_geterror_info)(struct synps_edac_priv *priv);
> +	enum mem_type (*edac_get_mtype)(const void __iomem *base);
> +	enum dev_type (*edac_get_dtype)(const void __iomem *base);
> +	bool (*edac_get_eccstate)(void __iomem *base);
> +	int quirks;
> +};
> +
> +/**
>   * synps_edac_geterror_info - Get the current ecc error info
> - * @base:	Pointer to the base address of the ddr memory controller
> - * @p:		Pointer to the synopsys ecc status structure
> + * @priv:	Pointer to DDR memory controller private instance data
>   *
>   * Determines there is any ecc error or not
>   *
>   * Return: one if there is no error otherwise returns zero
>   */
> -static int synps_edac_geterror_info(void __iomem *base,
> -				    struct synps_ecc_status *p)
> +static int synps_edac_geterror_info(struct synps_edac_priv *priv)
>  {
> +	void __iomem *base;
> +	struct synps_ecc_status *p;
>  	u32 regval, clearval = 0;
> 
> +	if (!priv)
> +		return 1;
> +
> +	base = priv->baseaddr;
> +	p = &priv->stat;
> +
>  	regval = readl(base + STAT_OFST);
>  	if (!regval)
>  		return 1;
> @@ -240,9 +265,10 @@ static void synps_edac_handle_error(struct
> mem_ctl_info *mci,  static void synps_edac_check(struct mem_ctl_info *mci)  {
>  	struct synps_edac_priv *priv = mci->pvt_info;
> +	const struct synps_platform_data *p_data = priv->p_data;
>  	int status;
> 
> -	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
> +	status = p_data->edac_geterror_info(priv);
>  	if (status)
>  		return;
> 
> @@ -362,6 +388,7 @@ static int synps_edac_init_csrows(struct mem_ctl_info
> *mci)
>  	struct csrow_info *csi;
>  	struct dimm_info *dimm;
>  	struct synps_edac_priv *priv = mci->pvt_info;
> +	const struct synps_platform_data *p_data = priv->p_data;
>  	u32 size;
>  	int row, j;
> 
> @@ -370,12 +397,13 @@ static int synps_edac_init_csrows(struct
> mem_ctl_info *mci)
>  		size = synps_edac_get_memsize();
> 
>  		for (j = 0; j < csi->nr_channels; j++) {
> -			dimm            = csi->channels[j]->dimm;
> +			dimm = csi->channels[j]->dimm;
>  			dimm->edac_mode = EDAC_FLAG_SECDED;
> -			dimm->mtype     = synps_edac_get_mtype(priv-
> >baseaddr);
> -			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi-
> >nr_channels;
> -			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
> -			dimm->dtype     = synps_edac_get_dtype(priv-
> >baseaddr);
> +			dimm->mtype = p_data->edac_get_mtype(priv-
> >baseaddr);
> +			dimm->nr_pages = (size >> PAGE_SHIFT) /
> +						csi->nr_channels;
> +			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
> +			dimm->dtype = p_data->edac_get_dtype(priv-
> >baseaddr);
>  		}
>  	}
> 
> @@ -423,6 +451,21 @@ static int synps_edac_mc_init(struct mem_ctl_info
> *mci,
>  	return status;
>  }
> 
> +static const struct synps_platform_data zynq_edac_def = {
> +	.edac_geterror_info	= synps_edac_geterror_info,
> +	.edac_get_mtype		= synps_edac_get_mtype,
> +	.edac_get_dtype		= synps_edac_get_dtype,
> +	.edac_get_eccstate	= synps_edac_get_eccstate,
> +	.quirks			= 0,
> +};
> +
> +static const struct of_device_id synps_edac_match[] = {
> +	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
> +	{ /* end of table */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, synps_edac_match);
> +
>  /**
>   * synps_edac_mc_probe - Check controller and bind driver
>   * @pdev:	Pointer to the platform_device struct
> @@ -440,13 +483,22 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
>  	int rc;
>  	struct resource *res;
>  	void __iomem *baseaddr;
> +	const struct of_device_id *match;
> +	const struct synps_platform_data *p_data;
> 
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	baseaddr = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(baseaddr))
>  		return PTR_ERR(baseaddr);
> 
> -	if (!synps_edac_get_eccstate(baseaddr)) {
> +	match = of_match_node(synps_edac_match, pdev->dev.of_node);
> +	if (!match && !match->data) {
> +		dev_err(&pdev->dev, "of_match_node() failed\n");
> +		return -EINVAL;
> +	}
> +
> +	p_data = (struct synps_platform_data *)match->data;
> +	if (!(p_data->edac_get_eccstate(baseaddr))) {
>  		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
>  		return -ENXIO;
>  	}
> @@ -468,6 +520,8 @@ static int synps_edac_mc_probe(struct platform_device
> *pdev)
> 
>  	priv = mci->pvt_info;
>  	priv->baseaddr = baseaddr;
> +	priv->p_data = match->data;
> +
>  	rc = synps_edac_mc_init(mci, pdev);
>  	if (rc) {
>  		edac_printk(KERN_ERR, EDAC_MC,
> @@ -511,13 +565,6 @@ static int synps_edac_mc_remove(struct
> platform_device *pdev)
>  	return 0;
>  }
> 
> -static const struct of_device_id synps_edac_match[] = {
> -	{ .compatible = "xlnx,zynq-ddrc-a05", },
> -	{ /* end of table */ }
> -};
> -
> -MODULE_DEVICE_TABLE(of, synps_edac_match);
> -
>  static struct platform_driver synps_edac_mc_driver = {
>  	.driver = {
>  		   .name = "synopsys-edac",
> --
> 2.1.1

Re: [PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc controller

[Borislav Petkov]

On Sat, Aug 18, 2018 at 10:11:54AM +0000, Manish Narani wrote:
> Ping.

First of all, one ping is enough. If your patchset contained, say, 30
patches, were you really going to send a ping for each one of them?!?!?!

Secondly, you do know that we have merge window right now, don't you?

And during the merge window, people are busy with merge window testing
and sending stuff which is ready, upstream - not reviewing new code
which has absolutely no chance of going in now?

Jeez.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc controller

[Borislav Petkov]

On Sat, Aug 04, 2018 at 02:55:32PM +0530, Manish Narani wrote:
> Add platform specific structures, so that we can add different IP
> support later using quirks.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/edac/synopsys_edac.c | 83 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 65 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 0c9c59e..b3c54e7 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -22,6 +22,7 @@
>  #include <linux/edac.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
>  
>  #include "edac_module.h"
>  
> @@ -130,6 +131,7 @@ struct synps_ecc_status {
>   * @baseaddr:	Base address of the DDR controller
>   * @message:	Buffer for framing the event specific info
>   * @stat:	ECC status information
> + * @p_data:	Pointer to platform data
>   * @ce_cnt:	Correctable Error count
>   * @ue_cnt:	Uncorrectable Error count
>   */
> @@ -137,24 +139,47 @@ struct synps_edac_priv {
>  	void __iomem *baseaddr;
>  	char message[SYNPS_EDAC_MSG_SIZE];
>  	struct synps_ecc_status stat;
> +	const struct synps_platform_data *p_data;
>  	u32 ce_cnt;
>  	u32 ue_cnt;
>  };
>  
>  /**
> + * struct synps_platform_data -  synps platform data structure
> + * @edac_geterror_info:	function pointer to synps edac error info
> + * @edac_get_mtype:	function pointer to synps edac mtype
> + * @edac_get_dtype:	function pointer to synps edac dtype
> + * @edac_get_eccstate:	function pointer to synps edac eccstate
> + * @quirks:		to differentiate IPs
> + */
> +struct synps_platform_data {
> +	int (*edac_geterror_info)(struct synps_edac_priv *priv);
> +	enum mem_type (*edac_get_mtype)(const void __iomem *base);
> +	enum dev_type (*edac_get_dtype)(const void __iomem *base);
> +	bool (*edac_get_eccstate)(void __iomem *base);
> +	int quirks;
> +};
> +
> +/**
>   * synps_edac_geterror_info - Get the current ecc error info
> - * @base:	Pointer to the base address of the ddr memory controller
> - * @p:		Pointer to the synopsys ecc status structure
> + * @priv:	Pointer to DDR memory controller private instance data
>   *
>   * Determines there is any ecc error or not
>   *
>   * Return: one if there is no error otherwise returns zero
>   */
> -static int synps_edac_geterror_info(void __iomem *base,
> -				    struct synps_ecc_status *p)
> +static int synps_edac_geterror_info(struct synps_edac_priv *priv)
>  {
> +	void __iomem *base;
> +	struct synps_ecc_status *p;
>  	u32 regval, clearval = 0;
>  
> +	if (!priv)
> +		return 1;
> +
> +	base = priv->baseaddr;
> +	p = &priv->stat;
> +
>  	regval = readl(base + STAT_OFST);
>  	if (!regval)
>  		return 1;
> @@ -240,9 +265,10 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
>  static void synps_edac_check(struct mem_ctl_info *mci)
>  {
>  	struct synps_edac_priv *priv = mci->pvt_info;
> +	const struct synps_platform_data *p_data = priv->p_data;
>  	int status;
>  
> -	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
> +	status = p_data->edac_geterror_info(priv);
>  	if (status)
>  		return;
>  
> @@ -362,6 +388,7 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
>  	struct csrow_info *csi;
>  	struct dimm_info *dimm;
>  	struct synps_edac_priv *priv = mci->pvt_info;
> +	const struct synps_platform_data *p_data = priv->p_data;
>  	u32 size;
>  	int row, j;
>  
> @@ -370,12 +397,13 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
>  		size = synps_edac_get_memsize();
>  
>  		for (j = 0; j < csi->nr_channels; j++) {
> -			dimm            = csi->channels[j]->dimm;
> +			dimm = csi->channels[j]->dimm;
>  			dimm->edac_mode = EDAC_FLAG_SECDED;
> -			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
> -			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi->nr_channels;
> -			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
> -			dimm->dtype     = synps_edac_get_dtype(priv->baseaddr);
> +			dimm->mtype = p_data->edac_get_mtype(priv->baseaddr);
> +			dimm->nr_pages = (size >> PAGE_SHIFT) /
> +						csi->nr_channels;

Why do you have to break that line? Just let it stick out.

And why can't you keep the nice vertical alignment on the '=' signs for
better readability?

> +			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
> +			dimm->dtype = p_data->edac_get_dtype(priv->baseaddr);
>  		}
>  	}
>  
> @@ -423,6 +451,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
>  	return status;
>  }
>  
> +static const struct synps_platform_data zynq_edac_def = {
> +	.edac_geterror_info	= synps_edac_geterror_info,
> +	.edac_get_mtype		= synps_edac_get_mtype,
> +	.edac_get_dtype		= synps_edac_get_dtype,
> +	.edac_get_eccstate	= synps_edac_get_eccstate,
> +	.quirks			= 0,

... like you've done here, for example.

> +};
> +
> +static const struct of_device_id synps_edac_match[] = {
> +	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
> +	{ /* end of table */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, synps_edac_match);
> +
>  /**
>   * synps_edac_mc_probe - Check controller and bind driver
>   * @pdev:	Pointer to the platform_device struct
> @@ -440,13 +483,22 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  	int rc;
>  	struct resource *res;
>  	void __iomem *baseaddr;
> +	const struct of_device_id *match;
> +	const struct synps_platform_data *p_data;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	baseaddr = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(baseaddr))
>  		return PTR_ERR(baseaddr);
>  
> -	if (!synps_edac_get_eccstate(baseaddr)) {
> +	match = of_match_node(synps_edac_match, pdev->dev.of_node);
> +	if (!match && !match->data) {
> +		dev_err(&pdev->dev, "of_match_node() failed\n");

That error message is not really helpful.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH v4 1/4] edac: synps: Add platform specific structures for ddrc controller

[Manish Narani]

Hi Boris,

Thank you so much for the review.

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, August 21, 2018 6:37 PM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Michal Simek <michals@xilinx.com>;
> mchehab@kernel.org; mdf@kernel.org; Edgar Iglesias <edgari@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Naga Sureshkumar Relli
> <nagasure@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>;
> stefan.krsmanovic@aggios.com; Srinivas Goud <sgoud@xilinx.com>; Anirudha
> Sarangi <anirudh@xilinx.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> edac@vger.kernel.org
> Subject: Re: [PATCH v4 1/4] edac: synps: Add platform specific structures for
> ddrc controller
> 
> On Sat, Aug 04, 2018 at 02:55:32PM +0530, Manish Narani wrote:
> > Add platform specific structures, so that we can add different IP
> > support later using quirks.
> >
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> >  drivers/edac/synopsys_edac.c | 83
> > ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 65 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/edac/synopsys_edac.c
> > b/drivers/edac/synopsys_edac.c index 0c9c59e..b3c54e7 100644
> > --- a/drivers/edac/synopsys_edac.c
> > +++ b/drivers/edac/synopsys_edac.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/edac.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/of.h>
> >
> >  #include "edac_module.h"
> >
> > @@ -130,6 +131,7 @@ struct synps_ecc_status {
> >   * @baseaddr:	Base address of the DDR controller
> >   * @message:	Buffer for framing the event specific info
> >   * @stat:	ECC status information
> > + * @p_data:	Pointer to platform data
> >   * @ce_cnt:	Correctable Error count
> >   * @ue_cnt:	Uncorrectable Error count
> >   */
> > @@ -137,24 +139,47 @@ struct synps_edac_priv {
> >  	void __iomem *baseaddr;
> >  	char message[SYNPS_EDAC_MSG_SIZE];
> >  	struct synps_ecc_status stat;
> > +	const struct synps_platform_data *p_data;
> >  	u32 ce_cnt;
> >  	u32 ue_cnt;
> >  };
> >
> >  /**
> > + * struct synps_platform_data -  synps platform data structure
> > + * @edac_geterror_info:	function pointer to synps edac error info
> > + * @edac_get_mtype:	function pointer to synps edac mtype
> > + * @edac_get_dtype:	function pointer to synps edac dtype
> > + * @edac_get_eccstate:	function pointer to synps edac eccstate
> > + * @quirks:		to differentiate IPs
> > + */
> > +struct synps_platform_data {
> > +	int (*edac_geterror_info)(struct synps_edac_priv *priv);
> > +	enum mem_type (*edac_get_mtype)(const void __iomem *base);
> > +	enum dev_type (*edac_get_dtype)(const void __iomem *base);
> > +	bool (*edac_get_eccstate)(void __iomem *base);
> > +	int quirks;
> > +};
> > +
> > +/**
> >   * synps_edac_geterror_info - Get the current ecc error info
> > - * @base:	Pointer to the base address of the ddr memory controller
> > - * @p:		Pointer to the synopsys ecc status structure
> > + * @priv:	Pointer to DDR memory controller private instance data
> >   *
> >   * Determines there is any ecc error or not
> >   *
> >   * Return: one if there is no error otherwise returns zero
> >   */
> > -static int synps_edac_geterror_info(void __iomem *base,
> > -				    struct synps_ecc_status *p)
> > +static int synps_edac_geterror_info(struct synps_edac_priv *priv)
> >  {
> > +	void __iomem *base;
> > +	struct synps_ecc_status *p;
> >  	u32 regval, clearval = 0;
> >
> > +	if (!priv)
> > +		return 1;
> > +
> > +	base = priv->baseaddr;
> > +	p = &priv->stat;
> > +
> >  	regval = readl(base + STAT_OFST);
> >  	if (!regval)
> >  		return 1;
> > @@ -240,9 +265,10 @@ static void synps_edac_handle_error(struct
> > mem_ctl_info *mci,  static void synps_edac_check(struct mem_ctl_info
> > *mci)  {
> >  	struct synps_edac_priv *priv = mci->pvt_info;
> > +	const struct synps_platform_data *p_data = priv->p_data;
> >  	int status;
> >
> > -	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
> > +	status = p_data->edac_geterror_info(priv);
> >  	if (status)
> >  		return;
> >
> > @@ -362,6 +388,7 @@ static int synps_edac_init_csrows(struct
> mem_ctl_info *mci)
> >  	struct csrow_info *csi;
> >  	struct dimm_info *dimm;
> >  	struct synps_edac_priv *priv = mci->pvt_info;
> > +	const struct synps_platform_data *p_data = priv->p_data;
> >  	u32 size;
> >  	int row, j;
> >
> > @@ -370,12 +397,13 @@ static int synps_edac_init_csrows(struct
> mem_ctl_info *mci)
> >  		size = synps_edac_get_memsize();
> >
> >  		for (j = 0; j < csi->nr_channels; j++) {
> > -			dimm            = csi->channels[j]->dimm;
> > +			dimm = csi->channels[j]->dimm;
> >  			dimm->edac_mode = EDAC_FLAG_SECDED;
> > -			dimm->mtype     = synps_edac_get_mtype(priv-
> >baseaddr);
> > -			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi-
> >nr_channels;
> > -			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
> > -			dimm->dtype     = synps_edac_get_dtype(priv-
> >baseaddr);
> > +			dimm->mtype = p_data->edac_get_mtype(priv-
> >baseaddr);
> > +			dimm->nr_pages = (size >> PAGE_SHIFT) /
> > +						csi->nr_channels;
> 
> Why do you have to break that line? Just let it stick out.[] 
Okay. I will update this in v5.

> 
> And why can't you keep the nice vertical alignment on the '=' signs for better
> readability?
Okay. This will bring checkpatch warning (above 80 lines), but for better readability, I will update this in v5.

> 
> > +			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
> > +			dimm->dtype = p_data->edac_get_dtype(priv-
> >baseaddr);
> >  		}
> >  	}
> >
> > @@ -423,6 +451,21 @@ static int synps_edac_mc_init(struct mem_ctl_info
> *mci,
> >  	return status;
> >  }
> >
> > +static const struct synps_platform_data zynq_edac_def = {
> > +	.edac_geterror_info	= synps_edac_geterror_info,
> > +	.edac_get_mtype		= synps_edac_get_mtype,
> > +	.edac_get_dtype		= synps_edac_get_dtype,
> > +	.edac_get_eccstate	= synps_edac_get_eccstate,
> > +	.quirks			= 0,
> 
> ... like you've done here, for example.
> 
> > +};
> > +
> > +static const struct of_device_id synps_edac_match[] = {
> > +	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
> > +	{ /* end of table */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, synps_edac_match);
> > +
> >  /**
> >   * synps_edac_mc_probe - Check controller and bind driver
> >   * @pdev:	Pointer to the platform_device struct
> > @@ -440,13 +483,22 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> >  	int rc;
> >  	struct resource *res;
> >  	void __iomem *baseaddr;
> > +	const struct of_device_id *match;
> > +	const struct synps_platform_data *p_data;
> >
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	baseaddr = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(baseaddr))
> >  		return PTR_ERR(baseaddr);
> >
> > -	if (!synps_edac_get_eccstate(baseaddr)) {
> > +	match = of_match_node(synps_edac_match, pdev->dev.of_node);
> > +	if (!match && !match->data) {
> > +		dev_err(&pdev->dev, "of_match_node() failed\n");
> 
> That error message is not really helpful.
Okay. I will update that part of code.

Thanks,
Manish Narani

[PATCH v4 2/4] dt: bindings: Document ZynqMP DDRC in Synopsys documentation

[Manish Narani]

This patch adds information of ZynqMP DDRC which reports the single bit
errors that are corrected and the double bit errors that are detected.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 .../bindings/memory-controllers/synopsys.txt       | 27 ++++++++++++++++++----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
index a43d26d..9d32762 100644
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
@@ -1,15 +1,32 @@
 Binding for Synopsys IntelliDDR Multi Protocol Memory Controller
 
-This controller has an optional ECC support in half-bus width (16-bit)
-configuration. The ECC controller corrects one bit error and detects
-two bit errors.
+The ZynqMP DDR ECC controller has an optional ECC support in 64-bit and 32-bit
+bus width configurations.
+
+The Zynq DDR ECC controller has an optional ECC support in half-bus width
+(16-bit) configuration.
+
+These both ECC controllers correct single bit ECC errors and detect double bit
+ECC errors.
 
 Required properties:
- - compatible: Should be 'xlnx,zynq-ddrc-a05'
- - reg: Base address and size of the controllers memory area
+ - compatible: One of:
+	- 'xlnx,zynq-ddrc-a05' : Zynq DDR ECC controller
+	- 'xlnx,zynqmp-ddrc-2.40a' : ZynqMP DDR ECC controller
+ - reg: Should contain DDR controller registers location and length.
+
+Required properties for "xlnx,zynqmp-ddrc-2.40a":
+ - interrupts: Property with a value describing the interrupt number.
 
 Example:
 	memory-controller@f8006000 {
 		compatible = "xlnx,zynq-ddrc-a05";
 		reg = <0xf8006000 0x1000>;
 	};
+
+	mc: memory-controller@fd070000 {
+		compatible = "xlnx,zynqmp-ddrc-2.40a";
+		reg = <0x0 0xfd070000 0x0 0x30000>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 112 4>;
+	};
-- 
2.1.1

Re: [PATCH v4 2/4] dt: bindings: Document ZynqMP DDRC in Synopsys documentation

[Rob Herring]

On Sat, Aug 04, 2018 at 02:55:33PM +0530, Manish Narani wrote:
> This patch adds information of ZynqMP DDRC which reports the single bit
> errors that are corrected and the double bit errors that are detected.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  .../bindings/memory-controllers/synopsys.txt       | 27 ++++++++++++++++++----
>  1 file changed, 22 insertions(+), 5 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v4 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Manish Narani]

Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC Error
Injection in ZynqMP. The corrected and uncorrected error interrupts
support is added. The Row, Column, Bank, Bank Group and Rank bits
positions are determined via Address Map registers of Synopsys DDRC.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/edac/Kconfig         |   2 +-
 drivers/edac/synopsys_edac.c | 864 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 848 insertions(+), 18 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 57304b2..b1fc7a16 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -441,7 +441,7 @@ config EDAC_ALTERA_SDMMC
 
 config EDAC_SYNOPSYS
 	tristate "Synopsys DDR Memory Controller"
-	depends on ARCH_ZYNQ
+	depends on ARCH_ZYNQ || ARM64
 	help
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index b3c54e7..82f276b 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -22,6 +22,7 @@
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/interrupt.h>
 #include <linux/of.h>
 
 #include "edac_module.h"
@@ -96,6 +97,170 @@
 #define SCRUB_MODE_MASK		0x7
 #define SCRUB_MODE_SECDED	0x4
 
+/* DDR ECC Quirks */
+#define DDR_ECC_INTR_SUPPORT	BIT(0)
+
+/* DDR ECC Quirks */
+#define DDR_ECC_INTR_SUPPORT	BIT(0)
+#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
+
+/* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
+/* ECC Configuration Registers */
+#define ECC_CFG0_OFST	0x70
+#define ECC_CFG1_OFST	0x74
+
+/* ECC Status Register */
+#define ECC_STAT_OFST	0x78
+
+/* ECC Clear Register */
+#define ECC_CLR_OFST	0x7C
+
+/* ECC Error count Register */
+#define ECC_ERRCNT_OFST	0x80
+
+/* ECC Corrected Error Address Register */
+#define ECC_CEADDR0_OFST	0x84
+#define ECC_CEADDR1_OFST	0x88
+
+/* ECC Syndrome Registers */
+#define ECC_CSYND0_OFST	0x8C
+#define ECC_CSYND1_OFST	0x90
+#define ECC_CSYND2_OFST	0x94
+
+/* ECC Bit Mask0 Address Register */
+#define ECC_BITMASK0_OFST	0x98
+#define ECC_BITMASK1_OFST	0x9C
+#define ECC_BITMASK2_OFST	0xA0
+
+/* ECC UnCorrected Error Address Register */
+#define ECC_UEADDR0_OFST	0xA4
+#define ECC_UEADDR1_OFST	0xA8
+
+/* ECC Syndrome Registers */
+#define ECC_UESYND0_OFST	0xAC
+#define ECC_UESYND1_OFST	0xB0
+#define ECC_UESYND2_OFST	0xB4
+
+/* ECC Poison Address Reg */
+#define ECC_POISON0_OFST	0xB8
+#define ECC_POISON1_OFST	0xBC
+
+#define ECC_ADDRMAP0_OFFSET	0x200
+
+/* Control register bitfield definitions */
+#define ECC_CTRL_BUSWIDTH_MASK	0x3000
+#define ECC_CTRL_BUSWIDTH_SHIFT	12
+#define ECC_CTRL_CLR_CE_ERRCNT	BIT(2)
+#define ECC_CTRL_CLR_UE_ERRCNT	BIT(3)
+
+/* DDR Control Register width definitions  */
+#define DDRCTL_EWDTH_16		2
+#define DDRCTL_EWDTH_32		1
+#define DDRCTL_EWDTH_64		0
+
+/* ECC status register definitions */
+#define ECC_STAT_UECNT_MASK	0xF0000
+#define ECC_STAT_UECNT_SHIFT	16
+#define ECC_STAT_CECNT_MASK	0xF00
+#define ECC_STAT_CECNT_SHIFT	8
+#define ECC_STAT_BITNUM_MASK	0x7F
+
+/* DDR QOS Interrupt register definitions */
+#define DDR_QOS_IRQ_STAT_OFST	0x20200
+#define DDR_QOSUE_MASK		0x4
+#define	DDR_QOSCE_MASK		0x2
+#define	ECC_CE_UE_INTR_MASK	0x6
+#define DDR_QOS_IRQ_EN_OFST     0x20208
+#define DDR_QOS_IRQ_DB_OFST	0x2020C
+
+/* ECC Corrected Error Register Mask and Shifts*/
+#define ECC_CEADDR0_RW_MASK	0x3FFFF
+#define ECC_CEADDR0_RNK_MASK	BIT(24)
+#define ECC_CEADDR1_BNKGRP_MASK	0x3000000
+#define ECC_CEADDR1_BNKNR_MASK	0x70000
+#define ECC_CEADDR1_BLKNR_MASK	0xFFF
+#define ECC_CEADDR1_BNKGRP_SHIFT	24
+#define ECC_CEADDR1_BNKNR_SHIFT	16
+
+/* ECC Poison register shifts */
+#define ECC_POISON0_RANK_SHIFT 24
+#define ECC_POISON1_BANKGRP_SHIFT 28
+#define ECC_POISON1_BANKNR_SHIFT 24
+
+/* DDR Memory type defines */
+#define MEM_TYPE_DDR3		0x1
+#define MEM_TYPE_LPDDR3		0x8
+#define MEM_TYPE_DDR2		0x4
+#define MEM_TYPE_DDR4		0x10
+#define MEM_TYPE_LPDDR4		0x20
+
+/* DDRC Software control register */
+#define DDRC_SWCTL 0x320
+
+/* DDRC ECC CE & UE poison mask */
+#define ECC_CEPOISON_MASK 0x3
+#define ECC_UEPOISON_MASK 0x1
+
+/* DDRC Device config masks */
+#define DDRC_MSTR_DEV_CONFIG_MASK 0xC0000000
+#define DDRC_MSTR_DEV_CONFIG_SHIFT	30
+#define DDRC_MSTR_DEV_CONFIG_X4_MASK	0x0
+#define DDRC_MSTR_DEV_CONFIG_X8_MASK	0x1
+#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x2
+#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0x3
+
+#define DDR_MAX_ROW_SHIFT	18
+#define DDR_MAX_COL_SHIFT	14
+#define DDR_MAX_BANK_SHIFT	3
+#define DDR_MAX_BANKGRP_SHIFT	2
+
+#define ROW_MAX_VAL_MASK	0xF
+#define COL_MAX_VAL_MASK	0xF
+#define BANK_MAX_VAL_MASK	0x1F
+#define BANKGRP_MAX_VAL_MASK	0x1F
+#define RANK_MAX_VAL_MASK	0x1F
+
+#define ROW_B0_BASE		6
+#define ROW_B1_BASE		7
+#define ROW_B2_BASE		8
+#define ROW_B3_BASE		9
+#define ROW_B4_BASE		10
+#define ROW_B5_BASE		11
+#define ROW_B6_BASE		12
+#define ROW_B7_BASE		13
+#define ROW_B8_BASE		14
+#define ROW_B9_BASE		15
+#define ROW_B10_BASE		16
+#define ROW_B11_BASE		17
+#define ROW_B12_BASE		18
+#define ROW_B13_BASE		19
+#define ROW_B14_BASE		20
+#define ROW_B15_BASE		21
+#define ROW_B16_BASE		22
+#define ROW_B17_BASE		23
+
+#define COL_B2_BASE		2
+#define COL_B3_BASE		3
+#define COL_B4_BASE		4
+#define COL_B5_BASE		5
+#define COL_B6_BASE		6
+#define COL_B7_BASE		7
+#define COL_B8_BASE		8
+#define COL_B9_BASE		9
+#define COL_B10_BASE		10
+#define COL_B11_BASE		11
+#define COL_B12_BASE		12
+#define COL_B13_BASE		13
+
+#define BANK_B0_BASE		2
+#define BANK_B1_BASE		3
+#define BANK_B2_BASE		4
+
+#define BANKGRP_B0_BASE		2
+#define BANKGRP_B1_BASE		3
+
+#define RANK_B0_BASE		6
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -103,6 +268,8 @@
  * @bank:	Bank number
  * @bitpos:	Bit position
  * @data:	Data causing the error
+ * @bankgrpnr:	Bank group number
+ * @blknr:	Block number
  */
 struct ecc_error_info {
 	u32 row;
@@ -110,6 +277,8 @@ struct ecc_error_info {
 	u32 bank;
 	u32 bitpos;
 	u32 data;
+	u32 bankgrpnr;
+	u32 blknr;
 };
 
 /**
@@ -128,12 +297,18 @@ struct synps_ecc_status {
 
 /**
  * struct synps_edac_priv - DDR memory controller private instance data
- * @baseaddr:	Base address of the DDR controller
- * @message:	Buffer for framing the event specific info
- * @stat:	ECC status information
- * @p_data:	Pointer to platform data
- * @ce_cnt:	Correctable Error count
- * @ue_cnt:	Uncorrectable Error count
+ * @baseaddr:		Base address of the DDR controller
+ * @message:		Buffer for framing the event specific info
+ * @stat:		ECC status information
+ * @p_data:		Pointer to platform data
+ * @ce_cnt:		Correctable Error count
+ * @ue_cnt:		Uncorrectable Error count
+ * @poison_addr:	Data poison address
+ * @row_shift:		Value of row shifts
+ * @col_shift:		Value of col shifts
+ * @bank_shift:		Value of bank shifts
+ * @bankgrp_shift:	Value of bank group shifts
+ * @rank_shift:		Value of rank shifts
  */
 struct synps_edac_priv {
 	void __iomem *baseaddr;
@@ -142,6 +317,12 @@ struct synps_edac_priv {
 	const struct synps_platform_data *p_data;
 	u32 ce_cnt;
 	u32 ue_cnt;
+	ulong poison_addr;
+	u32 row_shift[18];
+	u32 col_shift[14];
+	u32 bank_shift[3];
+	u32 bankgrp_shift[2];
+	u32 rank_shift[1];
 };
 
 /**
@@ -166,7 +347,7 @@ struct synps_platform_data {
  *
  * Determines there is any ecc error or not
  *
- * Return: one if there is no error otherwise returns zero
+ * Return: 1 if there is no error otherwise returns 0
  */
 static int synps_edac_geterror_info(struct synps_edac_priv *priv)
 {
@@ -221,6 +402,72 @@ static int synps_edac_geterror_info(struct synps_edac_priv *priv)
 }
 
 /**
+ * synps_enh_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @p:		Pointer to the synopsys ecc status structure
+ *
+ * Determines there is any ecc error or not
+ *
+ * Return: one if there is no error otherwise returns zero
+ */
+static int synps_enh_edac_geterror_info(struct synps_edac_priv *priv)
+{
+	void __iomem *base;
+	struct synps_ecc_status *p;
+	u32 regval, clearval = 0;
+
+	if (!priv)
+		return 1;
+
+	base = priv->baseaddr;
+	p = &priv->stat;
+
+	regval = readl(base + ECC_STAT_OFST);
+	if (!regval)
+		return 1;
+
+	p->ce_cnt = (regval & ECC_STAT_CECNT_MASK) >> ECC_STAT_CECNT_SHIFT;
+	p->ue_cnt = (regval & ECC_STAT_UECNT_MASK) >> ECC_STAT_UECNT_SHIFT;
+	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
+
+	regval = readl(base + ECC_CEADDR0_OFST);
+	if (!(p->ce_cnt))
+		goto ue_err;
+
+	p->ceinfo.row = (regval & ECC_CEADDR0_RW_MASK);
+	regval = readl(base + ECC_CEADDR1_OFST);
+	p->ceinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
+					ECC_CEADDR1_BNKNR_SHIFT;
+	p->ceinfo.bankgrpnr = (regval &	ECC_CEADDR1_BNKGRP_MASK) >>
+					ECC_CEADDR1_BNKGRP_SHIFT;
+	p->ceinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
+	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
+	edac_dbg(3, "ce bit position: %d data: %d\n", p->ceinfo.bitpos,
+		 p->ceinfo.data);
+
+ue_err:
+	regval = readl(base + ECC_UEADDR0_OFST);
+	if (!(p->ue_cnt))
+		goto out;
+
+	p->ueinfo.row = (regval & ECC_CEADDR0_RW_MASK);
+	regval = readl(base + ECC_UEADDR1_OFST);
+	p->ueinfo.bankgrpnr = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
+					ECC_CEADDR1_BNKGRP_SHIFT;
+	p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
+					ECC_CEADDR1_BNKNR_SHIFT;
+	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
+	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
+out:
+	clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
+	clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
+	writel(clearval, base + ECC_CLR_OFST);
+	writel(0x0, base + ECC_CLR_OFST);
+
+	return 0;
+}
+
+/**
  * synps_edac_handle_error - Handle controller error types CE and UE
  * @mci:	Pointer to the edac memory controller instance
  * @p:		Pointer to the synopsys ecc status structure
@@ -235,9 +482,17 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
 
 	if (p->ce_cnt) {
 		pinf = &p->ceinfo;
-		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
-			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
-			 "CE", pinf->row, pinf->bank, pinf->col);
+		if (priv->p_data->quirks == 0)
+			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+				 "CE", pinf->row, pinf->bank, pinf->col);
+		else
+			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+				 "DDR ECC error type :%s Row %d Bank %d Col %d "
+				 "BankGroup Number %d Block Number %d",
+				 "CE", pinf->row, pinf->bank, pinf->col,
+				 pinf->bankgrpnr, pinf->blknr);
+
 		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
 				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
 				     priv->message, "");
@@ -245,9 +500,17 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
 
 	if (p->ue_cnt) {
 		pinf = &p->ueinfo;
-		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
-			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
-			 "UE", pinf->row, pinf->bank, pinf->col);
+		if (priv->p_data->quirks == 0)
+			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+				"UE", pinf->row, pinf->bank, pinf->col);
+		else
+			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+				 "DDR ECC error type :%s Row %d Bank %d Col %d "
+				 "BankGroup Number %d Block Number %d",
+				 "UE", pinf->row, pinf->bank, pinf->col,
+				 pinf->bankgrpnr, pinf->blknr);
+
 		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
 				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
 				     priv->message, "");
@@ -257,6 +520,41 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
 }
 
 /**
+ * synps_edac_intr_handler - synps edac isr
+ * @irq:        irq number
+ * @dev_id:     device id poniter
+ *
+ * This is the Isr routine called by edac core interrupt thread.
+ * Used to check and post ECC errors.
+ *
+ * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise
+ */
+static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct synps_edac_priv *priv = mci->pvt_info;
+	const struct synps_platform_data *p_data = priv->p_data;
+	int status, regval;
+
+	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST) &
+			(DDR_QOSCE_MASK | DDR_QOSUE_MASK);
+	if (!(regval & ECC_CE_UE_INTR_MASK))
+		return IRQ_NONE;
+	status = p_data->edac_geterror_info(priv);
+	if (status)
+		return IRQ_NONE;
+
+	priv->ce_cnt += priv->stat.ce_cnt;
+	priv->ue_cnt += priv->stat.ue_cnt;
+	synps_edac_handle_error(mci, &priv->stat);
+
+	edac_dbg(3, "Total error count ce %d ue %d\n",
+		 priv->ce_cnt, priv->ue_cnt);
+	writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+	return IRQ_HANDLED;
+}
+
+/**
  * synps_edac_check - Check controller for ECC errors
  * @mci:	Pointer to the edac memory controller instance
  *
@@ -312,6 +610,40 @@ static enum dev_type synps_edac_get_dtype(const void __iomem *base)
 }
 
 /**
+ * synps_enh_edac_get_dtype - Return the controller memory width
+ * @base:	Pointer to the ddr memory controller base address
+ *
+ * Get the EDAC device type width appropriate for the current controller
+ * configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type synps_enh_edac_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + CTRL_OFST);
+	width = (width & ECC_CTRL_BUSWIDTH_MASK) >>
+		ECC_CTRL_BUSWIDTH_SHIFT;
+	switch (width) {
+	case DDRCTL_EWDTH_16:
+		dt = DEV_X2;
+		break;
+	case DDRCTL_EWDTH_32:
+		dt = DEV_X4;
+		break;
+	case DDRCTL_EWDTH_64:
+		dt = DEV_X8;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
  * synps_edac_get_eccstate - Return the controller ecc enable/disable status
  * @base:	Pointer to the ddr memory controller base address
  *
@@ -337,6 +669,32 @@ static bool synps_edac_get_eccstate(void __iomem *base)
 }
 
 /**
+ * synps_enh_edac_get_eccstate - Return the controller ecc enable/disable status
+ * @base:	Pointer to the ddr memory controller base address
+ *
+ * Get the ECC enable/disable status for the controller
+ *
+ * Return: a ecc status boolean i.e true/false - enabled/disabled.
+ */
+static bool synps_enh_edac_get_eccstate(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 ecctype;
+	bool state = false;
+
+	dt = synps_enh_edac_get_dtype(base);
+	if (dt == DEV_UNKNOWN)
+		return state;
+
+	ecctype = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
+	if ((ecctype == SCRUB_MODE_SECDED) &&
+	    ((dt == DEV_X2) || (dt == DEV_X4) || (dt == DEV_X8)))
+		state = true;
+
+	return state;
+}
+
+/**
  * synps_edac_get_memsize - reads the size of the attached memory device
  *
  * Return: the memory size in bytes
@@ -375,6 +733,35 @@ static enum mem_type synps_edac_get_mtype(const void __iomem *base)
 }
 
 /**
+ * synps_enh_edac_get_mtype - Returns controller memory type
+ * @base:	pointer to the synopsys ecc status structure
+ *
+ * Get the EDAC memory type appropriate for the current controller
+ * configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type synps_enh_edac_get_mtype(const void __iomem *base)
+{
+	enum mem_type mt;
+	u32 memtype;
+
+	memtype = readl(base + CTRL_OFST);
+
+	mt = MEM_UNKNOWN;
+	if ((memtype & MEM_TYPE_DDR3) || (memtype & MEM_TYPE_LPDDR3))
+		mt = MEM_DDR3;
+	else if (memtype & MEM_TYPE_DDR2)
+		mt = MEM_RDDR2;
+	else if ((memtype & MEM_TYPE_LPDDR4) || (memtype & MEM_TYPE_DDR4))
+		mt = MEM_DDR4;
+	else
+		mt = MEM_EMPTY;
+
+	return mt;
+}
+
+/**
  * synps_edac_init_csrows - Initialize the cs row data
  * @mci:	Pointer to the edac memory controller instance
  *
@@ -442,8 +829,13 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	mci->dev_name = SYNPS_EDAC_MOD_STRING;
 	mci->mod_name = SYNPS_EDAC_MOD_VER;
 
-	edac_op_state = EDAC_OPSTATE_POLL;
-	mci->edac_check = synps_edac_check;
+	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
+		edac_op_state = EDAC_OPSTATE_INT;
+	} else {
+		edac_op_state = EDAC_OPSTATE_POLL;
+		mci->edac_check = synps_edac_check;
+	}
+
 	mci->ctl_page_to_phys = NULL;
 
 	status = synps_edac_init_csrows(mci);
@@ -459,13 +851,409 @@ static const struct synps_platform_data zynq_edac_def = {
 	.quirks			= 0,
 };
 
+static const struct synps_platform_data zynqmp_enh_edac_def = {
+	.edac_geterror_info	= synps_enh_edac_geterror_info,
+	.edac_get_mtype		= synps_enh_edac_get_mtype,
+	.edac_get_dtype		= synps_enh_edac_get_dtype,
+	.edac_get_eccstate	= synps_enh_edac_get_eccstate,
+	.quirks			= (DDR_ECC_INTR_SUPPORT |
+				   DDR_ECC_DATA_POISON_SUPPORT),
+};
+
 static const struct of_device_id synps_edac_match[] = {
 	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
+	{ .compatible = "xlnx,zynqmp-ddrc-2.40a",
+				.data = (void *)&zynqmp_enh_edac_def},
 	{ /* end of table */ }
 };
 
 MODULE_DEVICE_TABLE(of, synps_edac_match);
 
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+/**
+ * ddr_poison_setup -	update poison registers
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Update poison registers as per ddr mapping
+ * Return: none.
+ */
+static void ddr_poison_setup(struct synps_edac_priv *priv)
+{
+	int col = 0, row = 0, bank = 0, bankgrp = 0, rank = 0, regval;
+	int index;
+	ulong hif_addr = 0;
+
+	hif_addr = priv->poison_addr >> 3;
+
+	for (index = 0; index < DDR_MAX_ROW_SHIFT; index++) {
+		if (priv->row_shift[index])
+			row |= (((hif_addr >> priv->row_shift[index]) &
+						BIT(0)) << index);
+		else
+			break;
+	}
+
+	for (index = 0; index < DDR_MAX_COL_SHIFT; index++) {
+		if (priv->col_shift[index] || index < 3)
+			col |= (((hif_addr >> priv->col_shift[index]) &
+						BIT(0)) << index);
+		else
+			break;
+	}
+
+	for (index = 0; index < DDR_MAX_BANK_SHIFT; index++) {
+		if (priv->bank_shift[index])
+			bank |= (((hif_addr >> priv->bank_shift[index]) &
+						BIT(0)) << index);
+		else
+			break;
+	}
+
+	for (index = 0; index < DDR_MAX_BANKGRP_SHIFT; index++) {
+		if (priv->bankgrp_shift[index])
+			bankgrp |= (((hif_addr >> priv->bankgrp_shift[index])
+						& BIT(0)) << index);
+		else
+			break;
+	}
+
+	if (priv->rank_shift[0])
+		rank = (hif_addr >> priv->rank_shift[0]) & BIT(0);
+
+	regval = (rank << ECC_POISON0_RANK_SHIFT) | col;
+	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
+	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
+			 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
+}
+
+/**
+ * inject_data_error_show - Get Poison0 & 1 register contents
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ *
+ * Get the Poison0 and Poison1 register contents
+ * Return: Number of bytes copied.
+ */
+static ssize_t inject_data_error_show(struct device *dev,
+					      struct device_attribute *mattr,
+					      char *data)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	return sprintf(data, "Poison0 Addr: 0x%08x\n\rPoison1 Addr: 0x%08x\n\r"
+			"Error injection Address: 0x%lx\n\r",
+			readl(priv->baseaddr + ECC_POISON0_OFST),
+			readl(priv->baseaddr + ECC_POISON1_OFST),
+			priv->poison_addr);
+}
+
+/**
+ * inject_data_error_store - Configure Poison0 Poison1 registers
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Configures the Poison0 and Poison1 register contents as per user given
+ * address
+ * Return: Number of bytes copied.
+ */
+static ssize_t inject_data_error_store(struct device *dev,
+					       struct device_attribute *mattr,
+					       const char *data, size_t count)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	if (kstrtoul(data, 0, &priv->poison_addr))
+		return -EINVAL;
+
+	ddr_poison_setup(priv);
+
+	return count;
+}
+
+/**
+ * inject_data_poison_show - Shows type of Data poison
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ *
+ * Shows the type of Error injection enabled, either UE or CE
+ * Return: Number of bytes copied.
+ */
+static ssize_t inject_data_poison_show(struct device *dev,
+					      struct device_attribute *mattr,
+					      char *data)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	return sprintf(data, "Data Poisoning: %s\n\r",
+			(((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) == 0x3)
+			? ("Correctable Error") : ("UnCorrectable Error"));
+}
+
+/**
+ * inject_data_poison_store - Enbles Data poison CE/UE
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Enables the CE or UE Data poison
+ * Return: Number of bytes copied.
+ */
+static ssize_t inject_data_poison_store(struct device *dev,
+					       struct device_attribute *mattr,
+					       const char *data, size_t count)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	writel(0, priv->baseaddr + DDRC_SWCTL);
+	if (strncmp(data, "CE", 2) == 0)
+		writel(ECC_CEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+	else
+		writel(ECC_UEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+	writel(1, priv->baseaddr + DDRC_SWCTL);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(inject_data_error);
+static DEVICE_ATTR_RW(inject_data_poison);
+
+/**
+ * synps_edac_create_sysfs_attributes - Create sysfs entries
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * Create sysfs attributes for injecting ECC errors using data poison.
+ *
+ * Return: 0 if sysfs creation was successful, else return negative error code.
+ */
+static int synps_edac_create_sysfs_attributes(struct mem_ctl_info *mci)
+{
+	int rc;
+
+	rc = device_create_file(&mci->dev, &dev_attr_inject_data_error);
+	if (rc < 0)
+		return rc;
+	rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison);
+	if (rc < 0)
+		return rc;
+	return 0;
+}
+
+/**
+ * synps_edac_remove_sysfs_attributes - Removes sysfs entries
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * Removes sysfs attributes.
+ *
+ * Return: none.
+ */
+static void synps_edac_remove_sysfs_attributes(struct mem_ctl_info *mci)
+{
+	device_remove_file(&mci->dev, &dev_attr_inject_data_error);
+	device_remove_file(&mci->dev, &dev_attr_inject_data_poison);
+}
+
+/**
+ * setup_address_map -	Set Address Map by querying ADDRMAP registers
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Set Address Map by querying ADDRMAP registers
+ * Return: none.
+ */
+static void setup_address_map(struct synps_edac_priv *priv)
+{
+	u32 addrmap[12], addrmap_row_b2_10;
+	int index;
+	u32 width, memtype;
+
+	memtype = readl(priv->baseaddr + CTRL_OFST);
+	width = (memtype & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
+
+	for (index = 0; index < 12; index++) {
+		u32 addrmap_offset;
+
+		addrmap_offset = ECC_ADDRMAP0_OFFSET + (index * 4);
+		addrmap[index] = readl(priv->baseaddr + addrmap_offset);
+	}
+
+	priv->row_shift[0] = (addrmap[5] & ROW_MAX_VAL_MASK) + ROW_B0_BASE;
+	priv->row_shift[1] = ((addrmap[5] >> 8) &
+			ROW_MAX_VAL_MASK) + ROW_B1_BASE;
+
+	addrmap_row_b2_10 = (addrmap[5] >> 16) & ROW_MAX_VAL_MASK;
+	if (addrmap_row_b2_10 != ROW_MAX_VAL_MASK) {
+		for (index = 2; index < 11; index++)
+			priv->row_shift[index] = addrmap_row_b2_10 +
+				index + ROW_B0_BASE;
+
+	} else {
+		priv->row_shift[2] = (addrmap[9] &
+				ROW_MAX_VAL_MASK) + ROW_B2_BASE;
+		priv->row_shift[3] = ((addrmap[9] >> 8) &
+				ROW_MAX_VAL_MASK) + ROW_B3_BASE;
+		priv->row_shift[4] = ((addrmap[9] >> 16) &
+				ROW_MAX_VAL_MASK) + ROW_B4_BASE;
+		priv->row_shift[5] = ((addrmap[9] >> 24) &
+				ROW_MAX_VAL_MASK) + ROW_B5_BASE;
+		priv->row_shift[6] = (addrmap[10] &
+				ROW_MAX_VAL_MASK) + ROW_B6_BASE;
+		priv->row_shift[7] = ((addrmap[10] >> 8) &
+				ROW_MAX_VAL_MASK) + ROW_B7_BASE;
+		priv->row_shift[8] = ((addrmap[10] >> 16) &
+				ROW_MAX_VAL_MASK) + ROW_B8_BASE;
+		priv->row_shift[9] = ((addrmap[10] >> 24) &
+				ROW_MAX_VAL_MASK) + ROW_B9_BASE;
+		priv->row_shift[10] = (addrmap[11] &
+				ROW_MAX_VAL_MASK) + ROW_B10_BASE;
+	}
+
+	priv->row_shift[11] = (((addrmap[5] >> 24) & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[5] >> 24) &
+				ROW_MAX_VAL_MASK) + ROW_B11_BASE);
+	priv->row_shift[12] = ((addrmap[6] & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[6] &
+				ROW_MAX_VAL_MASK) + ROW_B12_BASE);
+	priv->row_shift[13] = (((addrmap[6] >> 8) & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 8) &
+				ROW_MAX_VAL_MASK) + ROW_B13_BASE);
+	priv->row_shift[14] = (((addrmap[6] >> 16) & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 16) &
+				ROW_MAX_VAL_MASK) + ROW_B14_BASE);
+	priv->row_shift[15] = (((addrmap[6] >> 24) & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 24) &
+				ROW_MAX_VAL_MASK) + ROW_B15_BASE);
+	priv->row_shift[16] = ((addrmap[7] & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[7] &
+				ROW_MAX_VAL_MASK) + ROW_B16_BASE);
+	priv->row_shift[17] = (((addrmap[7] >> 8) & ROW_MAX_VAL_MASK) ==
+				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[7] >> 8) &
+				ROW_MAX_VAL_MASK) + ROW_B17_BASE);
+
+	priv->col_shift[0] = 0;
+	priv->col_shift[1] = 1;
+	priv->col_shift[2] = (addrmap[2] & COL_MAX_VAL_MASK) + COL_B2_BASE;
+	priv->col_shift[3] = ((addrmap[2] >> 8) &
+			COL_MAX_VAL_MASK) + COL_B3_BASE;
+	priv->col_shift[4] = (((addrmap[2] >> 16) & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 16) &
+					COL_MAX_VAL_MASK) + COL_B4_BASE);
+	priv->col_shift[5] = (((addrmap[2] >> 24) & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 24) &
+					COL_MAX_VAL_MASK) + COL_B5_BASE);
+	priv->col_shift[6] = ((addrmap[3] & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : ((addrmap[3] &
+					COL_MAX_VAL_MASK) + COL_B6_BASE);
+	priv->col_shift[7] = (((addrmap[3] >> 8) & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 8) &
+					COL_MAX_VAL_MASK) + COL_B7_BASE);
+	priv->col_shift[8] = (((addrmap[3] >> 16) & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 16) &
+					COL_MAX_VAL_MASK) + COL_B8_BASE);
+	priv->col_shift[9] = (((addrmap[3] >> 24) & COL_MAX_VAL_MASK) ==
+			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 24) &
+					COL_MAX_VAL_MASK) + COL_B9_BASE);
+	if (width == DDRCTL_EWDTH_64) {
+		if (memtype & MEM_TYPE_LPDDR3) {
+			priv->col_shift[10] = ((addrmap[4] &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				((addrmap[4] & COL_MAX_VAL_MASK) +
+				 COL_B10_BASE);
+			priv->col_shift[11] = (((addrmap[4] >> 8) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK) +
+				 COL_B11_BASE);
+		} else {
+			priv->col_shift[11] = ((addrmap[4] &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				((addrmap[4] & COL_MAX_VAL_MASK) +
+				 COL_B10_BASE);
+			priv->col_shift[13] = (((addrmap[4] >> 8) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK) +
+				 COL_B11_BASE);
+		}
+	} else if (width == DDRCTL_EWDTH_32) {
+		if (memtype & MEM_TYPE_LPDDR3) {
+			priv->col_shift[10] = (((addrmap[3] >> 24) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
+				 COL_B9_BASE);
+			priv->col_shift[11] = ((addrmap[4] &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				((addrmap[4] & COL_MAX_VAL_MASK) +
+				 COL_B10_BASE);
+		} else {
+			priv->col_shift[11] = (((addrmap[3] >> 24) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
+				 COL_B9_BASE);
+			priv->col_shift[13] = ((addrmap[4] &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				((addrmap[4] & COL_MAX_VAL_MASK) +
+				 COL_B10_BASE);
+		}
+	} else {
+		if (memtype & MEM_TYPE_LPDDR3) {
+			priv->col_shift[10] = (((addrmap[3] >> 16) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK) +
+				 COL_B8_BASE);
+			priv->col_shift[11] = (((addrmap[3] >> 24) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
+				 COL_B9_BASE);
+			priv->col_shift[13] = ((addrmap[4] &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				((addrmap[4] & COL_MAX_VAL_MASK) +
+				 COL_B10_BASE);
+		} else {
+			priv->col_shift[11] = (((addrmap[3] >> 16) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK) +
+				 COL_B8_BASE);
+			priv->col_shift[13] = (((addrmap[3] >> 24) &
+				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
+				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
+				 COL_B9_BASE);
+		}
+	}
+
+	if (width) {
+		for (index = 9; index > width; index--) {
+			priv->col_shift[index] = priv->col_shift[index - width];
+			priv->col_shift[index - width] = 0;
+		}
+	}
+
+	priv->bank_shift[0] = (addrmap[1] & BANK_MAX_VAL_MASK) + BANK_B0_BASE;
+	priv->bank_shift[1] = ((addrmap[1] >> 8) &
+				BANK_MAX_VAL_MASK) + BANK_B1_BASE;
+	priv->bank_shift[2] = (((addrmap[1] >> 16) &
+				BANK_MAX_VAL_MASK) == BANK_MAX_VAL_MASK) ? 0 :
+				(((addrmap[1] >> 16) & BANK_MAX_VAL_MASK) +
+				 BANK_B2_BASE);
+
+	priv->bankgrp_shift[0] = (addrmap[8] &
+				BANKGRP_MAX_VAL_MASK) + BANKGRP_B0_BASE;
+	priv->bankgrp_shift[1] = (((addrmap[8] >> 8) & BANKGRP_MAX_VAL_MASK) ==
+				BANKGRP_MAX_VAL_MASK) ? 0 : (((addrmap[8] >> 8)
+				& BANKGRP_MAX_VAL_MASK) + BANKGRP_B1_BASE);
+
+	priv->rank_shift[0] = ((addrmap[0] & RANK_MAX_VAL_MASK) ==
+				RANK_MAX_VAL_MASK) ? 0 : ((addrmap[0] &
+				RANK_MAX_VAL_MASK) + RANK_B0_BASE);
+}
+
 /**
  * synps_edac_mc_probe - Check controller and bind driver
  * @pdev:	Pointer to the platform_device struct
@@ -480,7 +1268,7 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[2];
 	struct synps_edac_priv *priv;
-	int rc;
+	int rc, irq, status;
 	struct resource *res;
 	void __iomem *baseaddr;
 	const struct of_device_id *match;
@@ -529,6 +1317,27 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 		goto free_edac_mc;
 	}
 
+	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0) {
+			edac_printk(KERN_ERR, EDAC_MC,
+					"No irq %d in DT\n", irq);
+			return -ENODEV;
+		}
+
+		status = devm_request_irq(&pdev->dev, irq,
+			synps_edac_intr_handler,
+			0, dev_name(&pdev->dev), mci);
+		if (status < 0) {
+			edac_printk(KERN_ERR, EDAC_MC, "Failed to request Irq\n");
+			goto free_edac_mc;
+		}
+
+		/* Enable UE/CE Interrupts */
+		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
+			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
+	}
+
 	rc = edac_mc_add_mc(mci);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC,
@@ -536,11 +1345,24 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 		goto free_edac_mc;
 	}
 
+	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT) {
+		if (synps_edac_create_sysfs_attributes(mci)) {
+			edac_printk(KERN_ERR, EDAC_MC,
+					"Failed to create sysfs entries\n");
+			goto free_edac_mc;
+		}
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "xlnx,zynqmp-ddrc-2.40a"))
+		setup_address_map(priv);
+
 	/*
 	 * Start capturing the correctable and uncorrectable errors. A write of
 	 * 0 starts the counters.
 	 */
-	writel(0x0, baseaddr + ECC_CTRL_OFST);
+	if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
+		writel(0x0, baseaddr + ECC_CTRL_OFST);
 	return rc;
 
 free_edac_mc:
@@ -558,8 +1380,16 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 static int synps_edac_mc_remove(struct platform_device *pdev)
 {
 	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+	struct synps_edac_priv *priv;
 
+	priv = mci->pvt_info;
+	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
+		/* Disable UE/CE Interrupts */
+		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
+			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
 	edac_mc_del_mc(&pdev->dev);
+	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT)
+		synps_edac_remove_sysfs_attributes(mci);
 	edac_mc_free(mci);
 
 	return 0;
-- 
2.1.1

Re: [PATCH v4 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Borislav Petkov]

On Sat, Aug 04, 2018 at 02:55:34PM +0530, Manish Narani wrote:
> Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC Error
> Injection in ZynqMP. The corrected and uncorrected error interrupts
> support is added. The Row, Column, Bank, Bank Group and Rank bits
> positions are determined via Address Map registers of Synopsys DDRC.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/edac/Kconfig         |   2 +-
>  drivers/edac/synopsys_edac.c | 864 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 848 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 57304b2..b1fc7a16 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -441,7 +441,7 @@ config EDAC_ALTERA_SDMMC
>  
>  config EDAC_SYNOPSYS
>  	tristate "Synopsys DDR Memory Controller"
> -	depends on ARCH_ZYNQ
> +	depends on ARCH_ZYNQ || ARM64
>  	help
>  	  Support for error detection and correction on the Synopsys DDR
>  	  memory controller.
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index b3c54e7..82f276b 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -22,6 +22,7 @@
>  #include <linux/edac.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/interrupt.h>
>  #include <linux/of.h>
>  
>  #include "edac_module.h"
> @@ -96,6 +97,170 @@
>  #define SCRUB_MODE_MASK		0x7
>  #define SCRUB_MODE_SECDED	0x4
>  
> +/* DDR ECC Quirks */
> +#define DDR_ECC_INTR_SUPPORT	BIT(0)
> +
> +/* DDR ECC Quirks */
> +#define DDR_ECC_INTR_SUPPORT	BIT(0)

This is repeated.

> +#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
> +
> +/* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
> +/* ECC Configuration Registers */
> +#define ECC_CFG0_OFST	0x70
> +#define ECC_CFG1_OFST	0x74
> +
> +/* ECC Status Register */
> +#define ECC_STAT_OFST	0x78
> +
> +/* ECC Clear Register */
> +#define ECC_CLR_OFST	0x7C
> +
> +/* ECC Error count Register */
> +#define ECC_ERRCNT_OFST	0x80

Do you not see how the rest of the defines' values are aligned
vertically in this file?

> +
> +/* ECC Corrected Error Address Register */
> +#define ECC_CEADDR0_OFST	0x84
> +#define ECC_CEADDR1_OFST	0x88
> +
> +/* ECC Syndrome Registers */
> +#define ECC_CSYND0_OFST	0x8C
> +#define ECC_CSYND1_OFST	0x90
> +#define ECC_CSYND2_OFST	0x94
> +
> +/* ECC Bit Mask0 Address Register */
> +#define ECC_BITMASK0_OFST	0x98
> +#define ECC_BITMASK1_OFST	0x9C
> +#define ECC_BITMASK2_OFST	0xA0
> +
> +/* ECC UnCorrected Error Address Register */
> +#define ECC_UEADDR0_OFST	0xA4
> +#define ECC_UEADDR1_OFST	0xA8
> +
> +/* ECC Syndrome Registers */
> +#define ECC_UESYND0_OFST	0xAC
> +#define ECC_UESYND1_OFST	0xB0
> +#define ECC_UESYND2_OFST	0xB4
> +
> +/* ECC Poison Address Reg */
> +#define ECC_POISON0_OFST	0xB8
> +#define ECC_POISON1_OFST	0xBC
> +
> +#define ECC_ADDRMAP0_OFFSET	0x200
> +
> +/* Control register bitfield definitions */
> +#define ECC_CTRL_BUSWIDTH_MASK	0x3000
> +#define ECC_CTRL_BUSWIDTH_SHIFT	12
> +#define ECC_CTRL_CLR_CE_ERRCNT	BIT(2)
> +#define ECC_CTRL_CLR_UE_ERRCNT	BIT(3)
> +
> +/* DDR Control Register width definitions  */
> +#define DDRCTL_EWDTH_16		2
> +#define DDRCTL_EWDTH_32		1
> +#define DDRCTL_EWDTH_64		0
> +
> +/* ECC status register definitions */
> +#define ECC_STAT_UECNT_MASK	0xF0000
> +#define ECC_STAT_UECNT_SHIFT	16
> +#define ECC_STAT_CECNT_MASK	0xF00
> +#define ECC_STAT_CECNT_SHIFT	8
> +#define ECC_STAT_BITNUM_MASK	0x7F
> +
> +/* DDR QOS Interrupt register definitions */
> +#define DDR_QOS_IRQ_STAT_OFST	0x20200
> +#define DDR_QOSUE_MASK		0x4
> +#define	DDR_QOSCE_MASK		0x2
> +#define	ECC_CE_UE_INTR_MASK	0x6
> +#define DDR_QOS_IRQ_EN_OFST     0x20208
> +#define DDR_QOS_IRQ_DB_OFST	0x2020C
> +
> +/* ECC Corrected Error Register Mask and Shifts*/
> +#define ECC_CEADDR0_RW_MASK	0x3FFFF
> +#define ECC_CEADDR0_RNK_MASK	BIT(24)
> +#define ECC_CEADDR1_BNKGRP_MASK	0x3000000
> +#define ECC_CEADDR1_BNKNR_MASK	0x70000
> +#define ECC_CEADDR1_BLKNR_MASK	0xFFF
> +#define ECC_CEADDR1_BNKGRP_SHIFT	24
> +#define ECC_CEADDR1_BNKNR_SHIFT	16
> +
> +/* ECC Poison register shifts */
> +#define ECC_POISON0_RANK_SHIFT 24
> +#define ECC_POISON1_BANKGRP_SHIFT 28
> +#define ECC_POISON1_BANKNR_SHIFT 24
> +
> +/* DDR Memory type defines */
> +#define MEM_TYPE_DDR3		0x1
> +#define MEM_TYPE_LPDDR3		0x8
> +#define MEM_TYPE_DDR2		0x4
> +#define MEM_TYPE_DDR4		0x10
> +#define MEM_TYPE_LPDDR4		0x20
> +
> +/* DDRC Software control register */
> +#define DDRC_SWCTL 0x320
> +
> +/* DDRC ECC CE & UE poison mask */
> +#define ECC_CEPOISON_MASK 0x3
> +#define ECC_UEPOISON_MASK 0x1
> +
> +/* DDRC Device config masks */
> +#define DDRC_MSTR_DEV_CONFIG_MASK 0xC0000000
> +#define DDRC_MSTR_DEV_CONFIG_SHIFT	30
> +#define DDRC_MSTR_DEV_CONFIG_X4_MASK	0x0
> +#define DDRC_MSTR_DEV_CONFIG_X8_MASK	0x1
> +#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x2
> +#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0x3

Those are too long and thus violate the vertical alignment - so you can shorten
them by doing, for example, s/CONFIG_MASK/CFG_MSK/

> +
> +#define DDR_MAX_ROW_SHIFT	18
> +#define DDR_MAX_COL_SHIFT	14
> +#define DDR_MAX_BANK_SHIFT	3
> +#define DDR_MAX_BANKGRP_SHIFT	2
> +
> +#define ROW_MAX_VAL_MASK	0xF
> +#define COL_MAX_VAL_MASK	0xF
> +#define BANK_MAX_VAL_MASK	0x1F
> +#define BANKGRP_MAX_VAL_MASK	0x1F
> +#define RANK_MAX_VAL_MASK	0x1F
> +
> +#define ROW_B0_BASE		6
> +#define ROW_B1_BASE		7
> +#define ROW_B2_BASE		8
> +#define ROW_B3_BASE		9
> +#define ROW_B4_BASE		10
> +#define ROW_B5_BASE		11
> +#define ROW_B6_BASE		12
> +#define ROW_B7_BASE		13
> +#define ROW_B8_BASE		14
> +#define ROW_B9_BASE		15
> +#define ROW_B10_BASE		16
> +#define ROW_B11_BASE		17
> +#define ROW_B12_BASE		18
> +#define ROW_B13_BASE		19
> +#define ROW_B14_BASE		20
> +#define ROW_B15_BASE		21
> +#define ROW_B16_BASE		22
> +#define ROW_B17_BASE		23
> +
> +#define COL_B2_BASE		2
> +#define COL_B3_BASE		3
> +#define COL_B4_BASE		4
> +#define COL_B5_BASE		5
> +#define COL_B6_BASE		6
> +#define COL_B7_BASE		7
> +#define COL_B8_BASE		8
> +#define COL_B9_BASE		9
> +#define COL_B10_BASE		10
> +#define COL_B11_BASE		11
> +#define COL_B12_BASE		12
> +#define COL_B13_BASE		13
> +
> +#define BANK_B0_BASE		2
> +#define BANK_B1_BASE		3
> +#define BANK_B2_BASE		4
> +
> +#define BANKGRP_B0_BASE		2
> +#define BANKGRP_B1_BASE		3
> +
> +#define RANK_B0_BASE		6
> +
>  /**
>   * struct ecc_error_info - ECC error log information
>   * @row:	Row number
> @@ -103,6 +268,8 @@
>   * @bank:	Bank number
>   * @bitpos:	Bit position
>   * @data:	Data causing the error
> + * @bankgrpnr:	Bank group number
> + * @blknr:	Block number
>   */
>  struct ecc_error_info {
>  	u32 row;
> @@ -110,6 +277,8 @@ struct ecc_error_info {
>  	u32 bank;
>  	u32 bitpos;
>  	u32 data;
> +	u32 bankgrpnr;
> +	u32 blknr;
>  };
>  
>  /**
> @@ -128,12 +297,18 @@ struct synps_ecc_status {
>  
>  /**
>   * struct synps_edac_priv - DDR memory controller private instance data
> - * @baseaddr:	Base address of the DDR controller
> - * @message:	Buffer for framing the event specific info
> - * @stat:	ECC status information
> - * @p_data:	Pointer to platform data
> - * @ce_cnt:	Correctable Error count
> - * @ue_cnt:	Uncorrectable Error count
> + * @baseaddr:		Base address of the DDR controller
> + * @message:		Buffer for framing the event specific info
> + * @stat:		ECC status information
> + * @p_data:		Pointer to platform data
> + * @ce_cnt:		Correctable Error count
> + * @ue_cnt:		Uncorrectable Error count
> + * @poison_addr:	Data poison address
> + * @row_shift:		Value of row shifts
> + * @col_shift:		Value of col shifts
> + * @bank_shift:		Value of bank shifts
> + * @bankgrp_shift:	Value of bank group shifts
> + * @rank_shift:		Value of rank shifts

We know it is a value - explaining what those shifts are would be much
more helpful.

>   */
>  struct synps_edac_priv {
>  	void __iomem *baseaddr;
> @@ -142,6 +317,12 @@ struct synps_edac_priv {
>  	const struct synps_platform_data *p_data;
>  	u32 ce_cnt;
>  	u32 ue_cnt;
> +	ulong poison_addr;
> +	u32 row_shift[18];
> +	u32 col_shift[14];
> +	u32 bank_shift[3];
> +	u32 bankgrp_shift[2];
> +	u32 rank_shift[1];
>  };
>  
>  /**
> @@ -166,7 +347,7 @@ struct synps_platform_data {
>   *
>   * Determines there is any ecc error or not
>   *
> - * Return: one if there is no error otherwise returns zero
> + * Return: 1 if there is no error otherwise returns 0
>   */
>  static int synps_edac_geterror_info(struct synps_edac_priv *priv)
>  {
> @@ -221,6 +402,72 @@ static int synps_edac_geterror_info(struct synps_edac_priv *priv)
>  }
>  
>  /**
> + * synps_enh_edac_geterror_info - Get the current ecc error info
> + * @base:	Pointer to the base address of the ddr memory controller
> + * @p:		Pointer to the synopsys ecc status structure
> + *
> + * Determines there is any ecc error or not
> + *
> + * Return: one if there is no error otherwise returns zero
> + */

That's one sloppy comment - the function arguments are wrong and the
description text needs to have correct english sentences ending with a
fullstop.

> +static int synps_enh_edac_geterror_info(struct synps_edac_priv *priv)

What is that prefix "synps_enh_edac" even supposed to mean? Especially
since this is a static function which doesn't need any prefixing...
Check your other functions too.

> +{
> +	void __iomem *base;
> +	struct synps_ecc_status *p;
> +	u32 regval, clearval = 0;
> +
> +	if (!priv)
> +		return 1;
> +
> +	base = priv->baseaddr;
> +	p = &priv->stat;
> +
> +	regval = readl(base + ECC_STAT_OFST);
> +	if (!regval)
> +		return 1;
> +
> +	p->ce_cnt = (regval & ECC_STAT_CECNT_MASK) >> ECC_STAT_CECNT_SHIFT;
> +	p->ue_cnt = (regval & ECC_STAT_UECNT_MASK) >> ECC_STAT_UECNT_SHIFT;
> +	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
> +
> +	regval = readl(base + ECC_CEADDR0_OFST);
> +	if (!(p->ce_cnt))
> +		goto ue_err;
> +
> +	p->ceinfo.row = (regval & ECC_CEADDR0_RW_MASK);
> +	regval = readl(base + ECC_CEADDR1_OFST);
> +	p->ceinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
> +					ECC_CEADDR1_BNKNR_SHIFT;
> +	p->ceinfo.bankgrpnr = (regval &	ECC_CEADDR1_BNKGRP_MASK) >>
> +					ECC_CEADDR1_BNKGRP_SHIFT;
> +	p->ceinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
> +	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
> +	edac_dbg(3, "ce bit position: %d data: %d\n", p->ceinfo.bitpos,
> +		 p->ceinfo.data);
> +
> +ue_err:
> +	regval = readl(base + ECC_UEADDR0_OFST);
> +	if (!(p->ue_cnt))
> +		goto out;
> +
> +	p->ueinfo.row = (regval & ECC_CEADDR0_RW_MASK);
> +	regval = readl(base + ECC_UEADDR1_OFST);
> +	p->ueinfo.bankgrpnr = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
> +					ECC_CEADDR1_BNKGRP_SHIFT;
> +	p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
> +					ECC_CEADDR1_BNKNR_SHIFT;
> +	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
> +	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
> +out:
> +	clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
> +	clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
> +	writel(clearval, base + ECC_CLR_OFST);
> +	writel(0x0, base + ECC_CLR_OFST);
> +
> +	return 0;
> +}
> +
> +/**
>   * synps_edac_handle_error - Handle controller error types CE and UE
>   * @mci:	Pointer to the edac memory controller instance
>   * @p:		Pointer to the synopsys ecc status structure
> @@ -235,9 +482,17 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
>  
>  	if (p->ce_cnt) {
>  		pinf = &p->ceinfo;
> -		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> -			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> -			 "CE", pinf->row, pinf->bank, pinf->col);
> +		if (priv->p_data->quirks == 0)

		if (!...

> +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> +				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> +				 "CE", pinf->row, pinf->bank, pinf->col);
> +		else
> +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> +				 "DDR ECC error type :%s Row %d Bank %d Col %d "
> +				 "BankGroup Number %d Block Number %d",
> +				 "CE", pinf->row, pinf->bank, pinf->col,
> +				 pinf->bankgrpnr, pinf->blknr);
> +

Pls integrate checkpatch.pl into your patch creation workflow - some of
the warnings do make sense:

WARNING: quoted string split across lines
#380: FILE: drivers/edac/synopsys_edac.c:492:
+                                "DDR ECC error type :%s Row %d Bank %d Col %d "
+                                "BankGroup Number %d Block Number %d",

WARNING: quoted string split across lines
#401: FILE: drivers/edac/synopsys_edac.c:510:
+                                "DDR ECC error type :%s Row %d Bank %d Col %d "
+                                "BankGroup Number %d Block Number %d",

total: 0 errors, 2 warnings, 1005 lines checked


>  		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
>  				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
>  				     priv->message, "");
> @@ -245,9 +500,17 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
>  
>  	if (p->ue_cnt) {
>  		pinf = &p->ueinfo;
> -		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> -			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> -			 "UE", pinf->row, pinf->bank, pinf->col);
> +		if (priv->p_data->quirks == 0)

		if (!...

> +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> +				 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> +				"UE", pinf->row, pinf->bank, pinf->col);
> +		else
> +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> +				 "DDR ECC error type :%s Row %d Bank %d Col %d "
> +				 "BankGroup Number %d Block Number %d",
> +				 "UE", pinf->row, pinf->bank, pinf->col,
> +				 pinf->bankgrpnr, pinf->blknr);
> +
>  		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
>  				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
>  				     priv->message, "");
> @@ -257,6 +520,41 @@ static void synps_edac_handle_error(struct mem_ctl_info *mci,
>  }
>  
>  /**
> + * synps_edac_intr_handler - synps edac isr

Either explain in plain english what this is or don't write a comment at
all. "isr" is not helping.

Also, it is EDAC and not "edac" - check your whole patch for lowercased
abbreviations like "ddr", "ecc", ..., and correct them.

> + * @irq:        irq number
> + * @dev_id:     device id poniter

Also, check your patch for spelling errors: "poniter" is some new type
of pony or?

:)

> + *
> + * This is the Isr routine called by edac core interrupt thread.
> + * Used to check and post ECC errors.
> + *
> + * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise

All those sentences need to end with a fullstop. Check your whole patch.

> + */
> +static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id)
> +{
> +	struct mem_ctl_info *mci = dev_id;
> +	struct synps_edac_priv *priv = mci->pvt_info;
> +	const struct synps_platform_data *p_data = priv->p_data;
> +	int status, regval;
> +
> +	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST) &
> +			(DDR_QOSCE_MASK | DDR_QOSUE_MASK);

Do the & in a second line:

	regval &= ...

for better readability.

> +	if (!(regval & ECC_CE_UE_INTR_MASK))
> +		return IRQ_NONE;

<----- newline here.

> +	status = p_data->edac_geterror_info(priv);
> +	if (status)
> +		return IRQ_NONE;
> +
> +	priv->ce_cnt += priv->stat.ce_cnt;
> +	priv->ue_cnt += priv->stat.ue_cnt;
> +	synps_edac_handle_error(mci, &priv->stat);
> +
> +	edac_dbg(3, "Total error count ce %d ue %d\n",
> +		 priv->ce_cnt, priv->ue_cnt);
> +	writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
> +	return IRQ_HANDLED;
> +}
> +
> +/**
>   * synps_edac_check - Check controller for ECC errors
>   * @mci:	Pointer to the edac memory controller instance
>   *
> @@ -312,6 +610,40 @@ static enum dev_type synps_edac_get_dtype(const void __iomem *base)
>  }
>  
>  /**
> + * synps_enh_edac_get_dtype - Return the controller memory width
> + * @base:	Pointer to the ddr memory controller base address
> + *
> + * Get the EDAC device type width appropriate for the current controller
> + * configuration.
> + *
> + * Return: a device type width enumeration.
> + */
> +static enum dev_type synps_enh_edac_get_dtype(const void __iomem *base)
> +{
> +	enum dev_type dt;
> +	u32 width;
> +
> +	width = readl(base + CTRL_OFST);
> +	width = (width & ECC_CTRL_BUSWIDTH_MASK) >>
> +		ECC_CTRL_BUSWIDTH_SHIFT;

Let it stick out instead of breaking it this way.

> +	switch (width) {
> +	case DDRCTL_EWDTH_16:
> +		dt = DEV_X2;
> +		break;
> +	case DDRCTL_EWDTH_32:
> +		dt = DEV_X4;
> +		break;
> +	case DDRCTL_EWDTH_64:
> +		dt = DEV_X8;
> +		break;
> +	default:
> +		dt = DEV_UNKNOWN;
> +	}
> +
> +	return dt;
> +}
> +
> +/**
>   * synps_edac_get_eccstate - Return the controller ecc enable/disable status
>   * @base:	Pointer to the ddr memory controller base address
>   *
> @@ -337,6 +669,32 @@ static bool synps_edac_get_eccstate(void __iomem *base)
>  }
>  
>  /**
> + * synps_enh_edac_get_eccstate - Return the controller ecc enable/disable status
> + * @base:	Pointer to the ddr memory controller base address
> + *
> + * Get the ECC enable/disable status for the controller
> + *
> + * Return: a ecc status boolean i.e true/false - enabled/disabled.
> + */
> +static bool synps_enh_edac_get_eccstate(void __iomem *base)
> +{
> +	enum dev_type dt;
> +	u32 ecctype;
> +	bool state = false;

You don't need state at all, just do...

> +
> +	dt = synps_enh_edac_get_dtype(base);
> +	if (dt == DEV_UNKNOWN)
> +		return state;

		return false;

> +
> +	ecctype = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
> +	if ((ecctype == SCRUB_MODE_SECDED) &&
> +	    ((dt == DEV_X2) || (dt == DEV_X4) || (dt == DEV_X8)))
> +		state = true;

		return true;

> +
> +	return state;

	return false;

> +}
> +
> +/**
>   * synps_edac_get_memsize - reads the size of the attached memory device
>   *
>   * Return: the memory size in bytes
> @@ -375,6 +733,35 @@ static enum mem_type synps_edac_get_mtype(const void __iomem *base)
>  }
>  
>  /**
> + * synps_enh_edac_get_mtype - Returns controller memory type
> + * @base:	pointer to the synopsys ecc status structure
> + *
> + * Get the EDAC memory type appropriate for the current controller
> + * configuration.
> + *
> + * Return: a memory type enumeration.
> + */
> +static enum mem_type synps_enh_edac_get_mtype(const void __iomem *base)
> +{
> +	enum mem_type mt;
> +	u32 memtype;
> +
> +	memtype = readl(base + CTRL_OFST);
> +
> +	mt = MEM_UNKNOWN;

You don't need this default assignment...

> +	if ((memtype & MEM_TYPE_DDR3) || (memtype & MEM_TYPE_LPDDR3))
> +		mt = MEM_DDR3;
> +	else if (memtype & MEM_TYPE_DDR2)
> +		mt = MEM_RDDR2;
> +	else if ((memtype & MEM_TYPE_LPDDR4) || (memtype & MEM_TYPE_DDR4))
> +		mt = MEM_DDR4;
> +	else
> +		mt = MEM_EMPTY;

... if you do the catch-all here.

> +
> +	return mt;
> +}
> +
> +/**
>   * synps_edac_init_csrows - Initialize the cs row data
>   * @mci:	Pointer to the edac memory controller instance
>   *
> @@ -442,8 +829,13 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
>  	mci->dev_name = SYNPS_EDAC_MOD_STRING;
>  	mci->mod_name = SYNPS_EDAC_MOD_VER;
>  
> -	edac_op_state = EDAC_OPSTATE_POLL;
> -	mci->edac_check = synps_edac_check;
> +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
> +		edac_op_state = EDAC_OPSTATE_INT;
> +	} else {
> +		edac_op_state = EDAC_OPSTATE_POLL;
> +		mci->edac_check = synps_edac_check;
> +	}
> +
>  	mci->ctl_page_to_phys = NULL;
>  
>  	status = synps_edac_init_csrows(mci);
> @@ -459,13 +851,409 @@ static const struct synps_platform_data zynq_edac_def = {
>  	.quirks			= 0,
>  };
>  
> +static const struct synps_platform_data zynqmp_enh_edac_def = {
> +	.edac_geterror_info	= synps_enh_edac_geterror_info,
> +	.edac_get_mtype		= synps_enh_edac_get_mtype,
> +	.edac_get_dtype		= synps_enh_edac_get_dtype,
> +	.edac_get_eccstate	= synps_enh_edac_get_eccstate,
> +	.quirks			= (DDR_ECC_INTR_SUPPORT |
> +				   DDR_ECC_DATA_POISON_SUPPORT),
> +};
> +
>  static const struct of_device_id synps_edac_match[] = {
>  	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
> +	{ .compatible = "xlnx,zynqmp-ddrc-2.40a",
> +				.data = (void *)&zynqmp_enh_edac_def},

Align those struct members vertically like this:

static const struct of_device_id synps_edac_match[] = {
        {
                .compatible = "xlnx,zynq-ddrc-a05",
                .data = (void *)&zynq_edac_def
        },
        {
                .compatible = "xlnx,zynqmp-ddrc-2.40a",
                .data = (void *)&zynqmp_enh_edac_def
        },
        { /* end of table */ }
};

>  MODULE_DEVICE_TABLE(of, synps_edac_match);
>  
> +#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
> +
> +/**
> + * ddr_poison_setup -	update poison registers
> + * @priv:		Pointer to synps_edac_priv struct
> + *
> + * Update poison registers as per ddr mapping
> + * Return: none.
> + */
> +static void ddr_poison_setup(struct synps_edac_priv *priv)
> +{
> +	int col = 0, row = 0, bank = 0, bankgrp = 0, rank = 0, regval;
> +	int index;
> +	ulong hif_addr = 0;
> +
> +	hif_addr = priv->poison_addr >> 3;
> +
> +	for (index = 0; index < DDR_MAX_ROW_SHIFT; index++) {
> +		if (priv->row_shift[index])
> +			row |= (((hif_addr >> priv->row_shift[index]) &
> +						BIT(0)) << index);
> +		else
> +			break;
> +	}
> +
> +	for (index = 0; index < DDR_MAX_COL_SHIFT; index++) {
> +		if (priv->col_shift[index] || index < 3)
> +			col |= (((hif_addr >> priv->col_shift[index]) &
> +						BIT(0)) << index);
> +		else
> +			break;
> +	}
> +
> +	for (index = 0; index < DDR_MAX_BANK_SHIFT; index++) {
> +		if (priv->bank_shift[index])
> +			bank |= (((hif_addr >> priv->bank_shift[index]) &
> +						BIT(0)) << index);
> +		else
> +			break;
> +	}
> +
> +	for (index = 0; index < DDR_MAX_BANKGRP_SHIFT; index++) {
> +		if (priv->bankgrp_shift[index])
> +			bankgrp |= (((hif_addr >> priv->bankgrp_shift[index])
> +						& BIT(0)) << index);
> +		else
> +			break;
> +	}
> +
> +	if (priv->rank_shift[0])
> +		rank = (hif_addr >> priv->rank_shift[0]) & BIT(0);
> +
> +	regval = (rank << ECC_POISON0_RANK_SHIFT) | col;
> +	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
> +	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
> +			 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
> +	writel(regval, priv->baseaddr + ECC_POISON1_OFST);

Those 4 lines could use some readability improvements.

> +}
> +
> +/**
> + * inject_data_error_show - Get Poison0 & 1 register contents
> + * @dev:	Pointer to the device struct
> + * @mattr:	Pointer to device attributes
> + * @data:	Pointer to user data

That "pointer to" is kinda silly. Just explain what those things are.
Ditto for the rest of the driver.

> + *
> + * Get the Poison0 and Poison1 register contents
> + * Return: Number of bytes copied.
> + */
> +static ssize_t inject_data_error_show(struct device *dev,
> +					      struct device_attribute *mattr,
> +					      char *data)

Align arguments on the opening brace.

More importantly, you need to put all that injection functionality
behind CONFIG_EDAC_DEBUG - you don't want to expose the injection
capabilities on a production machine.

> +{
> +	struct mem_ctl_info *mci = to_mci(dev);
> +	struct synps_edac_priv *priv = mci->pvt_info;
> +
> +	return sprintf(data, "Poison0 Addr: 0x%08x\n\rPoison1 Addr: 0x%08x\n\r"
> +			"Error injection Address: 0x%lx\n\r",
> +			readl(priv->baseaddr + ECC_POISON0_OFST),
> +			readl(priv->baseaddr + ECC_POISON1_OFST),
> +			priv->poison_addr);
> +}
> +
> +/**
> + * inject_data_error_store - Configure Poison0 Poison1 registers
> + * @dev:	Pointer to the device struct
> + * @mattr:	Pointer to device attributes
> + * @data:	Pointer to user data
> + * @count:	read the size bytes from buffer
> + *
> + * Configures the Poison0 and Poison1 register contents as per user given
> + * address
> + * Return: Number of bytes copied.
> + */
> +static ssize_t inject_data_error_store(struct device *dev,
> +					       struct device_attribute *mattr,
> +					       const char *data, size_t count)
> +{
> +	struct mem_ctl_info *mci = to_mci(dev);
> +	struct synps_edac_priv *priv = mci->pvt_info;
> +
> +	if (kstrtoul(data, 0, &priv->poison_addr))
> +		return -EINVAL;
> +
> +	ddr_poison_setup(priv);
> +
> +	return count;
> +}
> +
> +/**
> + * inject_data_poison_show - Shows type of Data poison
> + * @dev:	Pointer to the device struct
> + * @mattr:	Pointer to device attributes
> + * @data:	Pointer to user data
> + *
> + * Shows the type of Error injection enabled, either UE or CE
> + * Return: Number of bytes copied.
> + */
> +static ssize_t inject_data_poison_show(struct device *dev,
> +					      struct device_attribute *mattr,
> +					      char *data)
> +{
> +	struct mem_ctl_info *mci = to_mci(dev);
> +	struct synps_edac_priv *priv = mci->pvt_info;
> +
> +	return sprintf(data, "Data Poisoning: %s\n\r",
> +			(((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) == 0x3)
> +			? ("Correctable Error") : ("UnCorrectable Error"));
> +}
> +
> +/**
> + * inject_data_poison_store - Enbles Data poison CE/UE
> + * @dev:	Pointer to the device struct
> + * @mattr:	Pointer to device attributes
> + * @data:	Pointer to user data
> + * @count:	read the size bytes from buffer
> + *
> + * Enables the CE or UE Data poison
> + * Return: Number of bytes copied.
> + */
> +static ssize_t inject_data_poison_store(struct device *dev,
> +					       struct device_attribute *mattr,
> +					       const char *data, size_t count)
> +{
> +	struct mem_ctl_info *mci = to_mci(dev);
> +	struct synps_edac_priv *priv = mci->pvt_info;
> +
> +	writel(0, priv->baseaddr + DDRC_SWCTL);
> +	if (strncmp(data, "CE", 2) == 0)
> +		writel(ECC_CEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
> +	else
> +		writel(ECC_UEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
> +	writel(1, priv->baseaddr + DDRC_SWCTL);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(inject_data_error);
> +static DEVICE_ATTR_RW(inject_data_poison);
> +
> +/**
> + * synps_edac_create_sysfs_attributes - Create sysfs entries
> + * @mci:	Pointer to the edac memory controller instance
> + *
> + * Create sysfs attributes for injecting ECC errors using data poison.
> + *
> + * Return: 0 if sysfs creation was successful, else return negative error code.
> + */
> +static int synps_edac_create_sysfs_attributes(struct mem_ctl_info *mci)
> +{
> +	int rc;
> +
> +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_error);
> +	if (rc < 0)
> +		return rc;
> +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison);
> +	if (rc < 0)
> +		return rc;
> +	return 0;
> +}
> +
> +/**
> + * synps_edac_remove_sysfs_attributes - Removes sysfs entries
> + * @mci:	Pointer to the edac memory controller instance
> + *
> + * Removes sysfs attributes.
> + *
> + * Return: none.
> + */
> +static void synps_edac_remove_sysfs_attributes(struct mem_ctl_info *mci)
> +{
> +	device_remove_file(&mci->dev, &dev_attr_inject_data_error);
> +	device_remove_file(&mci->dev, &dev_attr_inject_data_poison);
> +}
> +
> +/**
> + * setup_address_map -	Set Address Map by querying ADDRMAP registers
> + * @priv:		Pointer to synps_edac_priv struct
> + *
> + * Set Address Map by querying ADDRMAP registers
> + * Return: none.
> + */
> +static void setup_address_map(struct synps_edac_priv *priv)
> +{
> +	u32 addrmap[12], addrmap_row_b2_10;
> +	int index;
> +	u32 width, memtype;
> +
> +	memtype = readl(priv->baseaddr + CTRL_OFST);
> +	width = (memtype & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
> +
> +	for (index = 0; index < 12; index++) {
> +		u32 addrmap_offset;
> +
> +		addrmap_offset = ECC_ADDRMAP0_OFFSET + (index * 4);
> +		addrmap[index] = readl(priv->baseaddr + addrmap_offset);
> +	}
> +
> +	priv->row_shift[0] = (addrmap[5] & ROW_MAX_VAL_MASK) + ROW_B0_BASE;
> +	priv->row_shift[1] = ((addrmap[5] >> 8) &
> +			ROW_MAX_VAL_MASK) + ROW_B1_BASE;
> +
> +	addrmap_row_b2_10 = (addrmap[5] >> 16) & ROW_MAX_VAL_MASK;
> +	if (addrmap_row_b2_10 != ROW_MAX_VAL_MASK) {
> +		for (index = 2; index < 11; index++)
> +			priv->row_shift[index] = addrmap_row_b2_10 +
> +				index + ROW_B0_BASE;
> +
> +	} else {
> +		priv->row_shift[2] = (addrmap[9] &
> +				ROW_MAX_VAL_MASK) + ROW_B2_BASE;
> +		priv->row_shift[3] = ((addrmap[9] >> 8) &
> +				ROW_MAX_VAL_MASK) + ROW_B3_BASE;
> +		priv->row_shift[4] = ((addrmap[9] >> 16) &
> +				ROW_MAX_VAL_MASK) + ROW_B4_BASE;
> +		priv->row_shift[5] = ((addrmap[9] >> 24) &
> +				ROW_MAX_VAL_MASK) + ROW_B5_BASE;
> +		priv->row_shift[6] = (addrmap[10] &
> +				ROW_MAX_VAL_MASK) + ROW_B6_BASE;
> +		priv->row_shift[7] = ((addrmap[10] >> 8) &
> +				ROW_MAX_VAL_MASK) + ROW_B7_BASE;
> +		priv->row_shift[8] = ((addrmap[10] >> 16) &
> +				ROW_MAX_VAL_MASK) + ROW_B8_BASE;
> +		priv->row_shift[9] = ((addrmap[10] >> 24) &
> +				ROW_MAX_VAL_MASK) + ROW_B9_BASE;
> +		priv->row_shift[10] = (addrmap[11] &
> +				ROW_MAX_VAL_MASK) + ROW_B10_BASE;
> +	}
> +
> +	priv->row_shift[11] = (((addrmap[5] >> 24) & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[5] >> 24) &
> +				ROW_MAX_VAL_MASK) + ROW_B11_BASE);
> +	priv->row_shift[12] = ((addrmap[6] & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[6] &
> +				ROW_MAX_VAL_MASK) + ROW_B12_BASE);
> +	priv->row_shift[13] = (((addrmap[6] >> 8) & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 8) &
> +				ROW_MAX_VAL_MASK) + ROW_B13_BASE);
> +	priv->row_shift[14] = (((addrmap[6] >> 16) & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 16) &
> +				ROW_MAX_VAL_MASK) + ROW_B14_BASE);
> +	priv->row_shift[15] = (((addrmap[6] >> 24) & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >> 24) &
> +				ROW_MAX_VAL_MASK) + ROW_B15_BASE);
> +	priv->row_shift[16] = ((addrmap[7] & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[7] &
> +				ROW_MAX_VAL_MASK) + ROW_B16_BASE);
> +	priv->row_shift[17] = (((addrmap[7] >> 8) & ROW_MAX_VAL_MASK) ==
> +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[7] >> 8) &
> +				ROW_MAX_VAL_MASK) + ROW_B17_BASE);
> +
> +	priv->col_shift[0] = 0;
> +	priv->col_shift[1] = 1;
> +	priv->col_shift[2] = (addrmap[2] & COL_MAX_VAL_MASK) + COL_B2_BASE;
> +	priv->col_shift[3] = ((addrmap[2] >> 8) &
> +			COL_MAX_VAL_MASK) + COL_B3_BASE;
> +	priv->col_shift[4] = (((addrmap[2] >> 16) & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 16) &
> +					COL_MAX_VAL_MASK) + COL_B4_BASE);
> +	priv->col_shift[5] = (((addrmap[2] >> 24) & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 24) &
> +					COL_MAX_VAL_MASK) + COL_B5_BASE);
> +	priv->col_shift[6] = ((addrmap[3] & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : ((addrmap[3] &
> +					COL_MAX_VAL_MASK) + COL_B6_BASE);
> +	priv->col_shift[7] = (((addrmap[3] >> 8) & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 8) &
> +					COL_MAX_VAL_MASK) + COL_B7_BASE);
> +	priv->col_shift[8] = (((addrmap[3] >> 16) & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 16) &
> +					COL_MAX_VAL_MASK) + COL_B8_BASE);
> +	priv->col_shift[9] = (((addrmap[3] >> 24) & COL_MAX_VAL_MASK) ==
> +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 24) &
> +					COL_MAX_VAL_MASK) + COL_B9_BASE);
> +	if (width == DDRCTL_EWDTH_64) {
> +		if (memtype & MEM_TYPE_LPDDR3) {
> +			priv->col_shift[10] = ((addrmap[4] &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				((addrmap[4] & COL_MAX_VAL_MASK) +
> +				 COL_B10_BASE);
> +			priv->col_shift[11] = (((addrmap[4] >> 8) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK) +
> +				 COL_B11_BASE);
> +		} else {
> +			priv->col_shift[11] = ((addrmap[4] &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				((addrmap[4] & COL_MAX_VAL_MASK) +
> +				 COL_B10_BASE);
> +			priv->col_shift[13] = (((addrmap[4] >> 8) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK) +
> +				 COL_B11_BASE);
> +		}
> +	} else if (width == DDRCTL_EWDTH_32) {
> +		if (memtype & MEM_TYPE_LPDDR3) {
> +			priv->col_shift[10] = (((addrmap[3] >> 24) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
> +				 COL_B9_BASE);
> +			priv->col_shift[11] = ((addrmap[4] &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				((addrmap[4] & COL_MAX_VAL_MASK) +
> +				 COL_B10_BASE);
> +		} else {
> +			priv->col_shift[11] = (((addrmap[3] >> 24) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
> +				 COL_B9_BASE);
> +			priv->col_shift[13] = ((addrmap[4] &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				((addrmap[4] & COL_MAX_VAL_MASK) +
> +				 COL_B10_BASE);
> +		}
> +	} else {
> +		if (memtype & MEM_TYPE_LPDDR3) {
> +			priv->col_shift[10] = (((addrmap[3] >> 16) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK) +
> +				 COL_B8_BASE);
> +			priv->col_shift[11] = (((addrmap[3] >> 24) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
> +				 COL_B9_BASE);
> +			priv->col_shift[13] = ((addrmap[4] &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				((addrmap[4] & COL_MAX_VAL_MASK) +
> +				 COL_B10_BASE);
> +		} else {
> +			priv->col_shift[11] = (((addrmap[3] >> 16) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK) +
> +				 COL_B8_BASE);
> +			priv->col_shift[13] = (((addrmap[3] >> 24) &
> +				COL_MAX_VAL_MASK) == COL_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK) +
> +				 COL_B9_BASE);
> +		}
> +	}
> +
> +	if (width) {
> +		for (index = 9; index > width; index--) {
> +			priv->col_shift[index] = priv->col_shift[index - width];
> +			priv->col_shift[index - width] = 0;
> +		}
> +	}
> +
> +	priv->bank_shift[0] = (addrmap[1] & BANK_MAX_VAL_MASK) + BANK_B0_BASE;
> +	priv->bank_shift[1] = ((addrmap[1] >> 8) &
> +				BANK_MAX_VAL_MASK) + BANK_B1_BASE;
> +	priv->bank_shift[2] = (((addrmap[1] >> 16) &
> +				BANK_MAX_VAL_MASK) == BANK_MAX_VAL_MASK) ? 0 :
> +				(((addrmap[1] >> 16) & BANK_MAX_VAL_MASK) +
> +				 BANK_B2_BASE);
> +
> +	priv->bankgrp_shift[0] = (addrmap[8] &
> +				BANKGRP_MAX_VAL_MASK) + BANKGRP_B0_BASE;
> +	priv->bankgrp_shift[1] = (((addrmap[8] >> 8) & BANKGRP_MAX_VAL_MASK) ==
> +				BANKGRP_MAX_VAL_MASK) ? 0 : (((addrmap[8] >> 8)
> +				& BANKGRP_MAX_VAL_MASK) + BANKGRP_B1_BASE);
> +
> +	priv->rank_shift[0] = ((addrmap[0] & RANK_MAX_VAL_MASK) ==
> +				RANK_MAX_VAL_MASK) ? 0 : ((addrmap[0] &
> +				RANK_MAX_VAL_MASK) + RANK_B0_BASE);
> +}

The assignments in that whole function are really unreadable. You could
define macros maybe and hide all the details in them as the assignments
look pretty regular...

> +
>  /**
>   * synps_edac_mc_probe - Check controller and bind driver
>   * @pdev:	Pointer to the platform_device struct
> @@ -480,7 +1268,7 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  	struct mem_ctl_info *mci;
>  	struct edac_mc_layer layers[2];
>  	struct synps_edac_priv *priv;
> -	int rc;
> +	int rc, irq, status;
>  	struct resource *res;
>  	void __iomem *baseaddr;
>  	const struct of_device_id *match;
> @@ -529,6 +1317,27 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  		goto free_edac_mc;
>  	}
>  
> +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
> +		irq = platform_get_irq(pdev, 0);
> +		if (irq < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +					"No irq %d in DT\n", irq);
> +			return -ENODEV;

			goto free_edac_mc;

otherwise you leak memory.

> +		}
> +
> +		status = devm_request_irq(&pdev->dev, irq,
> +			synps_edac_intr_handler,
> +			0, dev_name(&pdev->dev), mci);
> +		if (status < 0) {
> +			edac_printk(KERN_ERR, EDAC_MC, "Failed to request Irq\n");
> +			goto free_edac_mc;
> +		}
> +
> +		/* Enable UE/CE Interrupts */
> +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> +			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
> +	}
> +
>  	rc = edac_mc_add_mc(mci);
>  	if (rc) {
>  		edac_printk(KERN_ERR, EDAC_MC,
> @@ -536,11 +1345,24 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  		goto free_edac_mc;
>  	}
>  
> +	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT) {
> +		if (synps_edac_create_sysfs_attributes(mci)) {
> +			edac_printk(KERN_ERR, EDAC_MC,
> +					"Failed to create sysfs entries\n");
> +			goto free_edac_mc;
> +		}
> +	}
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "xlnx,zynqmp-ddrc-2.40a"))
> +		setup_address_map(priv);
> +
>  	/*
>  	 * Start capturing the correctable and uncorrectable errors. A write of
>  	 * 0 starts the counters.
>  	 */
> -	writel(0x0, baseaddr + ECC_CTRL_OFST);
> +	if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
> +		writel(0x0, baseaddr + ECC_CTRL_OFST);
>  	return rc;
>  
>  free_edac_mc:
> @@ -558,8 +1380,16 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  static int synps_edac_mc_remove(struct platform_device *pdev)
>  {
>  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> +	struct synps_edac_priv *priv;
>  
> +	priv = mci->pvt_info;
> +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
> +		/* Disable UE/CE Interrupts */
> +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> +			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
>  	edac_mc_del_mc(&pdev->dev);
> +	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT)
> +		synps_edac_remove_sysfs_attributes(mci);
>  	edac_mc_free(mci);
>  
>  	return 0;
> -- 
> 2.1.1
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH v4 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Manish Narani]

Hi Boris,

Thanks a lot for the review.

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, August 21, 2018 6:40 PM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Michal Simek <michals@xilinx.com>;
> mchehab@kernel.org; mdf@kernel.org; Edgar Iglesias <edgari@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Naga Sureshkumar Relli
> <nagasure@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>;
> stefan.krsmanovic@aggios.com; Srinivas Goud <sgoud@xilinx.com>; Anirudha
> Sarangi <anirudh@xilinx.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> edac@vger.kernel.org
> Subject: Re: [PATCH v4 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP
> DDRC
> 
> On Sat, Aug 04, 2018 at 02:55:34PM +0530, Manish Narani wrote:
> > Add EDAC ECC support for ZynqMP DDRC IP. Also add support for ECC
> > Error Injection in ZynqMP. The corrected and uncorrected error
> > interrupts support is added. The Row, Column, Bank, Bank Group and
> > Rank bits positions are determined via Address Map registers of Synopsys
> DDRC.
> >
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> >  drivers/edac/Kconfig         |   2 +-
> >  drivers/edac/synopsys_edac.c | 864
> > ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 848 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index
> > 57304b2..b1fc7a16 100644
> > --- a/drivers/edac/Kconfig
> > +++ b/drivers/edac/Kconfig
> > @@ -441,7 +441,7 @@ config EDAC_ALTERA_SDMMC
> >
> >  config EDAC_SYNOPSYS
> >  	tristate "Synopsys DDR Memory Controller"
> > -	depends on ARCH_ZYNQ
> > +	depends on ARCH_ZYNQ || ARM64
> >  	help
> >  	  Support for error detection and correction on the Synopsys DDR
> >  	  memory controller.
> > diff --git a/drivers/edac/synopsys_edac.c
> > b/drivers/edac/synopsys_edac.c index b3c54e7..82f276b 100644
> > --- a/drivers/edac/synopsys_edac.c
> > +++ b/drivers/edac/synopsys_edac.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/edac.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/of.h>
> >
> >  #include "edac_module.h"
> > @@ -96,6 +97,170 @@
> >  #define SCRUB_MODE_MASK		0x7
> >  #define SCRUB_MODE_SECDED	0x4
> >
> > +/* DDR ECC Quirks */
> > +#define DDR_ECC_INTR_SUPPORT	BIT(0)
> > +
> > +/* DDR ECC Quirks */
> > +#define DDR_ECC_INTR_SUPPORT	BIT(0)
> 
> This is repeated.
I missed that. I will update the same in v5.

> 
> > +#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
> > +
> > +/* ZynqMP Enhanced DDR memory controller registers that are relevant
> > +to ECC */
> > +/* ECC Configuration Registers */
> > +#define ECC_CFG0_OFST	0x70
> > +#define ECC_CFG1_OFST	0x74
> > +
> > +/* ECC Status Register */
> > +#define ECC_STAT_OFST	0x78
> > +
> > +/* ECC Clear Register */
> > +#define ECC_CLR_OFST	0x7C
> > +
> > +/* ECC Error count Register */
> > +#define ECC_ERRCNT_OFST	0x80
> 
> Do you not see how the rest of the defines' values are aligned vertically in this
> file?
Will work in this and update the same.
> 
> > +
> > +/* ECC Corrected Error Address Register */
> > +#define ECC_CEADDR0_OFST	0x84
> > +#define ECC_CEADDR1_OFST	0x88
> > +
> > +/* ECC Syndrome Registers */
> > +#define ECC_CSYND0_OFST	0x8C
> > +#define ECC_CSYND1_OFST	0x90
> > +#define ECC_CSYND2_OFST	0x94
> > +
> > +/* ECC Bit Mask0 Address Register */
> > +#define ECC_BITMASK0_OFST	0x98
> > +#define ECC_BITMASK1_OFST	0x9C
> > +#define ECC_BITMASK2_OFST	0xA0
> > +
> > +/* ECC UnCorrected Error Address Register */
> > +#define ECC_UEADDR0_OFST	0xA4
> > +#define ECC_UEADDR1_OFST	0xA8
> > +
> > +/* ECC Syndrome Registers */
> > +#define ECC_UESYND0_OFST	0xAC
> > +#define ECC_UESYND1_OFST	0xB0
> > +#define ECC_UESYND2_OFST	0xB4
> > +
> > +/* ECC Poison Address Reg */
> > +#define ECC_POISON0_OFST	0xB8
> > +#define ECC_POISON1_OFST	0xBC
> > +
> > +#define ECC_ADDRMAP0_OFFSET	0x200
> > +
> > +/* Control register bitfield definitions */
> > +#define ECC_CTRL_BUSWIDTH_MASK	0x3000
> > +#define ECC_CTRL_BUSWIDTH_SHIFT	12
> > +#define ECC_CTRL_CLR_CE_ERRCNT	BIT(2)
> > +#define ECC_CTRL_CLR_UE_ERRCNT	BIT(3)
> > +
> > +/* DDR Control Register width definitions  */
> > +#define DDRCTL_EWDTH_16		2
> > +#define DDRCTL_EWDTH_32		1
> > +#define DDRCTL_EWDTH_64		0
> > +
> > +/* ECC status register definitions */
> > +#define ECC_STAT_UECNT_MASK	0xF0000
> > +#define ECC_STAT_UECNT_SHIFT	16
> > +#define ECC_STAT_CECNT_MASK	0xF00
> > +#define ECC_STAT_CECNT_SHIFT	8
> > +#define ECC_STAT_BITNUM_MASK	0x7F
> > +
> > +/* DDR QOS Interrupt register definitions */
> > +#define DDR_QOS_IRQ_STAT_OFST	0x20200
> > +#define DDR_QOSUE_MASK		0x4
> > +#define	DDR_QOSCE_MASK		0x2
> > +#define	ECC_CE_UE_INTR_MASK	0x6
> > +#define DDR_QOS_IRQ_EN_OFST     0x20208
> > +#define DDR_QOS_IRQ_DB_OFST	0x2020C
> > +
> > +/* ECC Corrected Error Register Mask and Shifts*/
> > +#define ECC_CEADDR0_RW_MASK	0x3FFFF
> > +#define ECC_CEADDR0_RNK_MASK	BIT(24)
> > +#define ECC_CEADDR1_BNKGRP_MASK	0x3000000
> > +#define ECC_CEADDR1_BNKNR_MASK	0x70000
> > +#define ECC_CEADDR1_BLKNR_MASK	0xFFF
> > +#define ECC_CEADDR1_BNKGRP_SHIFT	24
> > +#define ECC_CEADDR1_BNKNR_SHIFT	16
> > +
> > +/* ECC Poison register shifts */
> > +#define ECC_POISON0_RANK_SHIFT 24
> > +#define ECC_POISON1_BANKGRP_SHIFT 28
> > +#define ECC_POISON1_BANKNR_SHIFT 24
> > +
> > +/* DDR Memory type defines */
> > +#define MEM_TYPE_DDR3		0x1
> > +#define MEM_TYPE_LPDDR3		0x8
> > +#define MEM_TYPE_DDR2		0x4
> > +#define MEM_TYPE_DDR4		0x10
> > +#define MEM_TYPE_LPDDR4		0x20
> > +
> > +/* DDRC Software control register */
> > +#define DDRC_SWCTL 0x320
> > +
> > +/* DDRC ECC CE & UE poison mask */
> > +#define ECC_CEPOISON_MASK 0x3
> > +#define ECC_UEPOISON_MASK 0x1
> > +
> > +/* DDRC Device config masks */
> > +#define DDRC_MSTR_DEV_CONFIG_MASK 0xC0000000
> > +#define DDRC_MSTR_DEV_CONFIG_SHIFT	30
> > +#define DDRC_MSTR_DEV_CONFIG_X4_MASK	0x0
> > +#define DDRC_MSTR_DEV_CONFIG_X8_MASK	0x1
> > +#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x2
> > +#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0x3
> 
> Those are too long and thus violate the vertical alignment - so you can shorten
> them by doing, for example, s/CONFIG_MASK/CFG_MSK/
Fair enough. Will update it in v5.
> 
> > +
> > +#define DDR_MAX_ROW_SHIFT	18
> > +#define DDR_MAX_COL_SHIFT	14
> > +#define DDR_MAX_BANK_SHIFT	3
> > +#define DDR_MAX_BANKGRP_SHIFT	2
> > +
> > +#define ROW_MAX_VAL_MASK	0xF
> > +#define COL_MAX_VAL_MASK	0xF
> > +#define BANK_MAX_VAL_MASK	0x1F
> > +#define BANKGRP_MAX_VAL_MASK	0x1F
> > +#define RANK_MAX_VAL_MASK	0x1F
> > +
> > +#define ROW_B0_BASE		6
> > +#define ROW_B1_BASE		7
> > +#define ROW_B2_BASE		8
> > +#define ROW_B3_BASE		9
> > +#define ROW_B4_BASE		10
> > +#define ROW_B5_BASE		11
> > +#define ROW_B6_BASE		12
> > +#define ROW_B7_BASE		13
> > +#define ROW_B8_BASE		14
> > +#define ROW_B9_BASE		15
> > +#define ROW_B10_BASE		16
> > +#define ROW_B11_BASE		17
> > +#define ROW_B12_BASE		18
> > +#define ROW_B13_BASE		19
> > +#define ROW_B14_BASE		20
> > +#define ROW_B15_BASE		21
> > +#define ROW_B16_BASE		22
> > +#define ROW_B17_BASE		23
> > +
> > +#define COL_B2_BASE		2
> > +#define COL_B3_BASE		3
> > +#define COL_B4_BASE		4
> > +#define COL_B5_BASE		5
> > +#define COL_B6_BASE		6
> > +#define COL_B7_BASE		7
> > +#define COL_B8_BASE		8
> > +#define COL_B9_BASE		9
> > +#define COL_B10_BASE		10
> > +#define COL_B11_BASE		11
> > +#define COL_B12_BASE		12
> > +#define COL_B13_BASE		13
> > +
> > +#define BANK_B0_BASE		2
> > +#define BANK_B1_BASE		3
> > +#define BANK_B2_BASE		4
> > +
> > +#define BANKGRP_B0_BASE		2
> > +#define BANKGRP_B1_BASE		3
> > +
> > +#define RANK_B0_BASE		6
> > +
> >  /**
> >   * struct ecc_error_info - ECC error log information
> >   * @row:	Row number
> > @@ -103,6 +268,8 @@
> >   * @bank:	Bank number
> >   * @bitpos:	Bit position
> >   * @data:	Data causing the error
> > + * @bankgrpnr:	Bank group number
> > + * @blknr:	Block number
> >   */
> >  struct ecc_error_info {
> >  	u32 row;
> > @@ -110,6 +277,8 @@ struct ecc_error_info {
> >  	u32 bank;
> >  	u32 bitpos;
> >  	u32 data;
> > +	u32 bankgrpnr;
> > +	u32 blknr;
> >  };
> >
> >  /**
> > @@ -128,12 +297,18 @@ struct synps_ecc_status {
> >
> >  /**
> >   * struct synps_edac_priv - DDR memory controller private instance data
> > - * @baseaddr:	Base address of the DDR controller
> > - * @message:	Buffer for framing the event specific info
> > - * @stat:	ECC status information
> > - * @p_data:	Pointer to platform data
> > - * @ce_cnt:	Correctable Error count
> > - * @ue_cnt:	Uncorrectable Error count
> > + * @baseaddr:		Base address of the DDR controller
> > + * @message:		Buffer for framing the event specific info
> > + * @stat:		ECC status information
> > + * @p_data:		Pointer to platform data
> > + * @ce_cnt:		Correctable Error count
> > + * @ue_cnt:		Uncorrectable Error count
> > + * @poison_addr:	Data poison address
> > + * @row_shift:		Value of row shifts
> > + * @col_shift:		Value of col shifts
> > + * @bank_shift:		Value of bank shifts
> > + * @bankgrp_shift:	Value of bank group shifts
> > + * @rank_shift:		Value of rank shifts
> 
> We know it is a value - explaining what those shifts are would be much more
> helpful.
Will update the description in v5.

> 
> >   */
> >  struct synps_edac_priv {
> >  	void __iomem *baseaddr;
> > @@ -142,6 +317,12 @@ struct synps_edac_priv {
> >  	const struct synps_platform_data *p_data;
> >  	u32 ce_cnt;
> >  	u32 ue_cnt;
> > +	ulong poison_addr;
> > +	u32 row_shift[18];
> > +	u32 col_shift[14];
> > +	u32 bank_shift[3];
> > +	u32 bankgrp_shift[2];
> > +	u32 rank_shift[1];
> >  };
> >
> >  /**
> > @@ -166,7 +347,7 @@ struct synps_platform_data {
> >   *
> >   * Determines there is any ecc error or not
> >   *
> > - * Return: one if there is no error otherwise returns zero
> > + * Return: 1 if there is no error otherwise returns 0
> >   */
> >  static int synps_edac_geterror_info(struct synps_edac_priv *priv)  {
> > @@ -221,6 +402,72 @@ static int synps_edac_geterror_info(struct
> > synps_edac_priv *priv)  }
> >
> >  /**
> > + * synps_enh_edac_geterror_info - Get the current ecc error info
> > + * @base:	Pointer to the base address of the ddr memory controller
> > + * @p:		Pointer to the synopsys ecc status structure
> > + *
> > + * Determines there is any ecc error or not
> > + *
> > + * Return: one if there is no error otherwise returns zero */
> 
> That's one sloppy comment - the function arguments are wrong and the
> description text needs to have correct english sentences ending with a fullstop.
Okay.
> 
> > +static int synps_enh_edac_geterror_info(struct synps_edac_priv *priv)
> 
> What is that prefix "synps_enh_edac" even supposed to mean? Especially since
> this is a static function which doesn't need any prefixing...
> Check your other functions too.

There are 2 different versions of Synopsys DDR controllers which are handled through this driver. One function is  synps_edac_geterror_info() (used for DDRC in Zynq) and the other is synps_enh_edac_geterror_info() (used for DDRC in ZynqMP). The prefixes are there to identify them. I could remove 'synps_enh' from the prefix and keep 'zynq' and 'zynqmp' to differentiate. Please suggest.

> 
> > +{
> > +	void __iomem *base;
> > +	struct synps_ecc_status *p;
> > +	u32 regval, clearval = 0;
> > +
> > +	if (!priv)
> > +		return 1;
> > +
> > +	base = priv->baseaddr;
> > +	p = &priv->stat;
> > +
> > +	regval = readl(base + ECC_STAT_OFST);
> > +	if (!regval)
> > +		return 1;
> > +
> > +	p->ce_cnt = (regval & ECC_STAT_CECNT_MASK) >>
> ECC_STAT_CECNT_SHIFT;
> > +	p->ue_cnt = (regval & ECC_STAT_UECNT_MASK) >>
> ECC_STAT_UECNT_SHIFT;
> > +	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
> > +
> > +	regval = readl(base + ECC_CEADDR0_OFST);
> > +	if (!(p->ce_cnt))
> > +		goto ue_err;
> > +
> > +	p->ceinfo.row = (regval & ECC_CEADDR0_RW_MASK);
> > +	regval = readl(base + ECC_CEADDR1_OFST);
> > +	p->ceinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
> > +					ECC_CEADDR1_BNKNR_SHIFT;
> > +	p->ceinfo.bankgrpnr = (regval &	ECC_CEADDR1_BNKGRP_MASK) >>
> > +					ECC_CEADDR1_BNKGRP_SHIFT;
> > +	p->ceinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
> > +	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
> > +	edac_dbg(3, "ce bit position: %d data: %d\n", p->ceinfo.bitpos,
> > +		 p->ceinfo.data);
> > +
> > +ue_err:
> > +	regval = readl(base + ECC_UEADDR0_OFST);
> > +	if (!(p->ue_cnt))
> > +		goto out;
> > +
> > +	p->ueinfo.row = (regval & ECC_CEADDR0_RW_MASK);
> > +	regval = readl(base + ECC_UEADDR1_OFST);
> > +	p->ueinfo.bankgrpnr = (regval & ECC_CEADDR1_BNKGRP_MASK) >>
> > +					ECC_CEADDR1_BNKGRP_SHIFT;
> > +	p->ueinfo.bank = (regval & ECC_CEADDR1_BNKNR_MASK) >>
> > +					ECC_CEADDR1_BNKNR_SHIFT;
> > +	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
> > +	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
> > +out:
> > +	clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
> > +	clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
> > +	writel(clearval, base + ECC_CLR_OFST);
> > +	writel(0x0, base + ECC_CLR_OFST);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> >   * synps_edac_handle_error - Handle controller error types CE and UE
> >   * @mci:	Pointer to the edac memory controller instance
> >   * @p:		Pointer to the synopsys ecc status structure
> > @@ -235,9 +482,17 @@ static void synps_edac_handle_error(struct
> > mem_ctl_info *mci,
> >
> >  	if (p->ce_cnt) {
> >  		pinf = &p->ceinfo;
> > -		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > -			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> > -			 "CE", pinf->row, pinf->bank, pinf->col);
> > +		if (priv->p_data->quirks == 0)
> 
> 		if (!...
> 
> > +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > +				 "DDR ECC error type :%s Row %d Bank %d Col
> %d ",
> > +				 "CE", pinf->row, pinf->bank, pinf->col);
> > +		else
> > +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > +				 "DDR ECC error type :%s Row %d Bank %d Col
> %d "
> > +				 "BankGroup Number %d Block Number %d",
> > +				 "CE", pinf->row, pinf->bank, pinf->col,
> > +				 pinf->bankgrpnr, pinf->blknr);
> > +
> 
> Pls integrate checkpatch.pl into your patch creation workflow - some of the
> warnings do make sense:
> 
> WARNING: quoted string split across lines
> #380: FILE: drivers/edac/synopsys_edac.c:492:
> +                                "DDR ECC error type :%s Row %d Bank %d Col %d "
> +                                "BankGroup Number %d Block Number %d",
> 
> WARNING: quoted string split across lines
> #401: FILE: drivers/edac/synopsys_edac.c:510:
> +                                "DDR ECC error type :%s Row %d Bank %d Col %d "
> +                                "BankGroup Number %d Block Number %d",
> 
> total: 0 errors, 2 warnings, 1005 lines checked
Okay. I will update this.
> 
> 
> >  		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> >  				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
> >  				     priv->message, "");
> > @@ -245,9 +500,17 @@ static void synps_edac_handle_error(struct
> > mem_ctl_info *mci,
> >
> >  	if (p->ue_cnt) {
> >  		pinf = &p->ueinfo;
> > -		snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > -			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
> > -			 "UE", pinf->row, pinf->bank, pinf->col);
> > +		if (priv->p_data->quirks == 0)
> 
> 		if (!...
> 
> > +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > +				 "DDR ECC error type :%s Row %d Bank %d Col
> %d ",
> > +				"UE", pinf->row, pinf->bank, pinf->col);
> > +		else
> > +			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
> > +				 "DDR ECC error type :%s Row %d Bank %d Col
> %d "
> > +				 "BankGroup Number %d Block Number %d",
> > +				 "UE", pinf->row, pinf->bank, pinf->col,
> > +				 pinf->bankgrpnr, pinf->blknr);
> > +
> >  		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> >  				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
> >  				     priv->message, "");
> > @@ -257,6 +520,41 @@ static void synps_edac_handle_error(struct
> > mem_ctl_info *mci,  }
> >
> >  /**
> > + * synps_edac_intr_handler - synps edac isr
> 
> Either explain in plain english what this is or don't write a comment at all. "isr"
> is not helping.
> 
> Also, it is EDAC and not "edac" - check your whole patch for lowercased
> abbreviations like "ddr", "ecc", ..., and correct them.
> 
> > + * @irq:        irq number
> > + * @dev_id:     device id poniter
> 
> Also, check your patch for spelling errors: "poniter" is some new type of pony
> or?
> 
> :)
Okay. I will update this.
> 
> > + *
> > + * This is the Isr routine called by edac core interrupt thread.
> > + * Used to check and post ECC errors.
> > + *
> > + * Return: IRQ_NONE, if interrupt not set or IRQ_HANDLED otherwise
> 
> All those sentences need to end with a fullstop. Check your whole patch.
> 
> > + */
> > +static irqreturn_t synps_edac_intr_handler(int irq, void *dev_id) {
> > +	struct mem_ctl_info *mci = dev_id;
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +	const struct synps_platform_data *p_data = priv->p_data;
> > +	int status, regval;
> > +
> > +	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST) &
> > +			(DDR_QOSCE_MASK | DDR_QOSUE_MASK);
> 
> Do the & in a second line:
> 
> 	regval &= ...
> 
> for better readability.
Sure. Will do this.
> 
> > +	if (!(regval & ECC_CE_UE_INTR_MASK))
> > +		return IRQ_NONE;
> 
> <----- newline here.
> 
> > +	status = p_data->edac_geterror_info(priv);
> > +	if (status)
> > +		return IRQ_NONE;
> > +
> > +	priv->ce_cnt += priv->stat.ce_cnt;
> > +	priv->ue_cnt += priv->stat.ue_cnt;
> > +	synps_edac_handle_error(mci, &priv->stat);
> > +
> > +	edac_dbg(3, "Total error count ce %d ue %d\n",
> > +		 priv->ce_cnt, priv->ue_cnt);
> > +	writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/**
> >   * synps_edac_check - Check controller for ECC errors
> >   * @mci:	Pointer to the edac memory controller instance
> >   *
> > @@ -312,6 +610,40 @@ static enum dev_type synps_edac_get_dtype(const
> > void __iomem *base)  }
> >
> >  /**
> > + * synps_enh_edac_get_dtype - Return the controller memory width
> > + * @base:	Pointer to the ddr memory controller base address
> > + *
> > + * Get the EDAC device type width appropriate for the current
> > +controller
> > + * configuration.
> > + *
> > + * Return: a device type width enumeration.
> > + */
> > +static enum dev_type synps_enh_edac_get_dtype(const void __iomem
> > +*base) {
> > +	enum dev_type dt;
> > +	u32 width;
> > +
> > +	width = readl(base + CTRL_OFST);
> > +	width = (width & ECC_CTRL_BUSWIDTH_MASK) >>
> > +		ECC_CTRL_BUSWIDTH_SHIFT;
> 
> Let it stick out instead of breaking it this way.
Okay. I will keep a single line.
> 
> > +	switch (width) {
> > +	case DDRCTL_EWDTH_16:
> > +		dt = DEV_X2;
> > +		break;
> > +	case DDRCTL_EWDTH_32:
> > +		dt = DEV_X4;
> > +		break;
> > +	case DDRCTL_EWDTH_64:
> > +		dt = DEV_X8;
> > +		break;
> > +	default:
> > +		dt = DEV_UNKNOWN;
> > +	}
> > +
> > +	return dt;
> > +}
> > +
> > +/**
> >   * synps_edac_get_eccstate - Return the controller ecc enable/disable status
> >   * @base:	Pointer to the ddr memory controller base address
> >   *
> > @@ -337,6 +669,32 @@ static bool synps_edac_get_eccstate(void __iomem
> > *base)  }
> >
> >  /**
> > + * synps_enh_edac_get_eccstate - Return the controller ecc enable/disable
> status
> > + * @base:	Pointer to the ddr memory controller base address
> > + *
> > + * Get the ECC enable/disable status for the controller
> > + *
> > + * Return: a ecc status boolean i.e true/false - enabled/disabled.
> > + */
> > +static bool synps_enh_edac_get_eccstate(void __iomem *base) {
> > +	enum dev_type dt;
> > +	u32 ecctype;
> > +	bool state = false;
> 
> You don't need state at all, just do...
> 
> > +
> > +	dt = synps_enh_edac_get_dtype(base);
> > +	if (dt == DEV_UNKNOWN)
> > +		return state;
> 
> 		return false;
> 
> > +
> > +	ecctype = readl(base + ECC_CFG0_OFST) & SCRUB_MODE_MASK;
> > +	if ((ecctype == SCRUB_MODE_SECDED) &&
> > +	    ((dt == DEV_X2) || (dt == DEV_X4) || (dt == DEV_X8)))
> > +		state = true;
> 
> 		return true;
> 
> > +
> > +	return state;
> 
> 	return false;
Okay.

> 
> > +}
> > +
> > +/**
> >   * synps_edac_get_memsize - reads the size of the attached memory device
> >   *
> >   * Return: the memory size in bytes
> > @@ -375,6 +733,35 @@ static enum mem_type
> synps_edac_get_mtype(const
> > void __iomem *base)  }
> >
> >  /**
> > + * synps_enh_edac_get_mtype - Returns controller memory type
> > + * @base:	pointer to the synopsys ecc status structure
> > + *
> > + * Get the EDAC memory type appropriate for the current controller
> > + * configuration.
> > + *
> > + * Return: a memory type enumeration.
> > + */
> > +static enum mem_type synps_enh_edac_get_mtype(const void __iomem
> > +*base) {
> > +	enum mem_type mt;
> > +	u32 memtype;
> > +
> > +	memtype = readl(base + CTRL_OFST);
> > +
> > +	mt = MEM_UNKNOWN;
> 
> You don't need this default assignment...
Yes. Will update this.
> 
> > +	if ((memtype & MEM_TYPE_DDR3) || (memtype &
> MEM_TYPE_LPDDR3))
> > +		mt = MEM_DDR3;
> > +	else if (memtype & MEM_TYPE_DDR2)
> > +		mt = MEM_RDDR2;
> > +	else if ((memtype & MEM_TYPE_LPDDR4) || (memtype &
> MEM_TYPE_DDR4))
> > +		mt = MEM_DDR4;
> > +	else
> > +		mt = MEM_EMPTY;
> 
> ... if you do the catch-all here.
> 
> > +
> > +	return mt;
> > +}
> > +
> > +/**
> >   * synps_edac_init_csrows - Initialize the cs row data
> >   * @mci:	Pointer to the edac memory controller instance
> >   *
> > @@ -442,8 +829,13 @@ static int synps_edac_mc_init(struct mem_ctl_info
> *mci,
> >  	mci->dev_name = SYNPS_EDAC_MOD_STRING;
> >  	mci->mod_name = SYNPS_EDAC_MOD_VER;
> >
> > -	edac_op_state = EDAC_OPSTATE_POLL;
> > -	mci->edac_check = synps_edac_check;
> > +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
> > +		edac_op_state = EDAC_OPSTATE_INT;
> > +	} else {
> > +		edac_op_state = EDAC_OPSTATE_POLL;
> > +		mci->edac_check = synps_edac_check;
> > +	}
> > +
> >  	mci->ctl_page_to_phys = NULL;
> >
> >  	status = synps_edac_init_csrows(mci); @@ -459,13 +851,409 @@
> static
> > const struct synps_platform_data zynq_edac_def = {
> >  	.quirks			= 0,
> >  };
> >
> > +static const struct synps_platform_data zynqmp_enh_edac_def = {
> > +	.edac_geterror_info	= synps_enh_edac_geterror_info,
> > +	.edac_get_mtype		= synps_enh_edac_get_mtype,
> > +	.edac_get_dtype		= synps_enh_edac_get_dtype,
> > +	.edac_get_eccstate	= synps_enh_edac_get_eccstate,
> > +	.quirks			= (DDR_ECC_INTR_SUPPORT |
> > +				   DDR_ECC_DATA_POISON_SUPPORT),
> > +};
> > +
> >  static const struct of_device_id synps_edac_match[] = {
> >  	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def
> > },
> > +	{ .compatible = "xlnx,zynqmp-ddrc-2.40a",
> > +				.data = (void *)&zynqmp_enh_edac_def},
> 
> Align those struct members vertically like this:
> 
> static const struct of_device_id synps_edac_match[] = {
>         {
>                 .compatible = "xlnx,zynq-ddrc-a05",
>                 .data = (void *)&zynq_edac_def
>         },
>         {
>                 .compatible = "xlnx,zynqmp-ddrc-2.40a",
>                 .data = (void *)&zynqmp_enh_edac_def
>         },
>         { /* end of table */ }
> };
Seems clean. Will update this.
> 
> >  MODULE_DEVICE_TABLE(of, synps_edac_match);
> >
> > +#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
> > +
> > +/**
> > + * ddr_poison_setup -	update poison registers
> > + * @priv:		Pointer to synps_edac_priv struct
> > + *
> > + * Update poison registers as per ddr mapping
> > + * Return: none.
> > + */
> > +static void ddr_poison_setup(struct synps_edac_priv *priv) {
> > +	int col = 0, row = 0, bank = 0, bankgrp = 0, rank = 0, regval;
> > +	int index;
> > +	ulong hif_addr = 0;
> > +
> > +	hif_addr = priv->poison_addr >> 3;
> > +
> > +	for (index = 0; index < DDR_MAX_ROW_SHIFT; index++) {
> > +		if (priv->row_shift[index])
> > +			row |= (((hif_addr >> priv->row_shift[index]) &
> > +						BIT(0)) << index);
> > +		else
> > +			break;
> > +	}
> > +
> > +	for (index = 0; index < DDR_MAX_COL_SHIFT; index++) {
> > +		if (priv->col_shift[index] || index < 3)
> > +			col |= (((hif_addr >> priv->col_shift[index]) &
> > +						BIT(0)) << index);
> > +		else
> > +			break;
> > +	}
> > +
> > +	for (index = 0; index < DDR_MAX_BANK_SHIFT; index++) {
> > +		if (priv->bank_shift[index])
> > +			bank |= (((hif_addr >> priv->bank_shift[index]) &
> > +						BIT(0)) << index);
> > +		else
> > +			break;
> > +	}
> > +
> > +	for (index = 0; index < DDR_MAX_BANKGRP_SHIFT; index++) {
> > +		if (priv->bankgrp_shift[index])
> > +			bankgrp |= (((hif_addr >> priv->bankgrp_shift[index])
> > +						& BIT(0)) << index);
> > +		else
> > +			break;
> > +	}
> > +
> > +	if (priv->rank_shift[0])
> > +		rank = (hif_addr >> priv->rank_shift[0]) & BIT(0);
> > +
> > +	regval = (rank << ECC_POISON0_RANK_SHIFT) | col;
> > +	writel(regval, priv->baseaddr + ECC_POISON0_OFST);
> > +	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
> > +			 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
> > +	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
> 
> Those 4 lines could use some readability improvements.
Okay. I will try to do the same.
> 
> > +}
> > +
> > +/**
> > + * inject_data_error_show - Get Poison0 & 1 register contents
> > + * @dev:	Pointer to the device struct
> > + * @mattr:	Pointer to device attributes
> > + * @data:	Pointer to user data
> 
> That "pointer to" is kinda silly. Just explain what those things are.
> Ditto for the rest of the driver.
Okay.
> 
> > + *
> > + * Get the Poison0 and Poison1 register contents
> > + * Return: Number of bytes copied.
> > + */
> > +static ssize_t inject_data_error_show(struct device *dev,
> > +					      struct device_attribute *mattr,
> > +					      char *data)
> 
> Align arguments on the opening brace.
> 
> More importantly, you need to put all that injection functionality behind
> CONFIG_EDAC_DEBUG - you don't want to expose the injection capabilities on
> a production machine.
I agree. I will update the same by keeping the above mentioned macro.

> 
> > +{
> > +	struct mem_ctl_info *mci = to_mci(dev);
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +
> > +	return sprintf(data, "Poison0 Addr: 0x%08x\n\rPoison1 Addr:
> 0x%08x\n\r"
> > +			"Error injection Address: 0x%lx\n\r",
> > +			readl(priv->baseaddr + ECC_POISON0_OFST),
> > +			readl(priv->baseaddr + ECC_POISON1_OFST),
> > +			priv->poison_addr);
> > +}
> > +
> > +/**
> > + * inject_data_error_store - Configure Poison0 Poison1 registers
> > + * @dev:	Pointer to the device struct
> > + * @mattr:	Pointer to device attributes
> > + * @data:	Pointer to user data
> > + * @count:	read the size bytes from buffer
> > + *
> > + * Configures the Poison0 and Poison1 register contents as per user
> > +given
> > + * address
> > + * Return: Number of bytes copied.
> > + */
> > +static ssize_t inject_data_error_store(struct device *dev,
> > +					       struct device_attribute *mattr,
> > +					       const char *data, size_t count) {
> > +	struct mem_ctl_info *mci = to_mci(dev);
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +
> > +	if (kstrtoul(data, 0, &priv->poison_addr))
> > +		return -EINVAL;
> > +
> > +	ddr_poison_setup(priv);
> > +
> > +	return count;
> > +}
> > +
> > +/**
> > + * inject_data_poison_show - Shows type of Data poison
> > + * @dev:	Pointer to the device struct
> > + * @mattr:	Pointer to device attributes
> > + * @data:	Pointer to user data
> > + *
> > + * Shows the type of Error injection enabled, either UE or CE
> > + * Return: Number of bytes copied.
> > + */
> > +static ssize_t inject_data_poison_show(struct device *dev,
> > +					      struct device_attribute *mattr,
> > +					      char *data)
> > +{
> > +	struct mem_ctl_info *mci = to_mci(dev);
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +
> > +	return sprintf(data, "Data Poisoning: %s\n\r",
> > +			(((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) ==
> 0x3)
> > +			? ("Correctable Error") : ("UnCorrectable Error")); }
> > +
> > +/**
> > + * inject_data_poison_store - Enbles Data poison CE/UE
> > + * @dev:	Pointer to the device struct
> > + * @mattr:	Pointer to device attributes
> > + * @data:	Pointer to user data
> > + * @count:	read the size bytes from buffer
> > + *
> > + * Enables the CE or UE Data poison
> > + * Return: Number of bytes copied.
> > + */
> > +static ssize_t inject_data_poison_store(struct device *dev,
> > +					       struct device_attribute *mattr,
> > +					       const char *data, size_t count) {
> > +	struct mem_ctl_info *mci = to_mci(dev);
> > +	struct synps_edac_priv *priv = mci->pvt_info;
> > +
> > +	writel(0, priv->baseaddr + DDRC_SWCTL);
> > +	if (strncmp(data, "CE", 2) == 0)
> > +		writel(ECC_CEPOISON_MASK, priv->baseaddr +
> ECC_CFG1_OFST);
> > +	else
> > +		writel(ECC_UEPOISON_MASK, priv->baseaddr +
> ECC_CFG1_OFST);
> > +	writel(1, priv->baseaddr + DDRC_SWCTL);
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(inject_data_error);
> > +static DEVICE_ATTR_RW(inject_data_poison);
> > +
> > +/**
> > + * synps_edac_create_sysfs_attributes - Create sysfs entries
> > + * @mci:	Pointer to the edac memory controller instance
> > + *
> > + * Create sysfs attributes for injecting ECC errors using data poison.
> > + *
> > + * Return: 0 if sysfs creation was successful, else return negative error code.
> > + */
> > +static int synps_edac_create_sysfs_attributes(struct mem_ctl_info
> > +*mci) {
> > +	int rc;
> > +
> > +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_error);
> > +	if (rc < 0)
> > +		return rc;
> > +	rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison);
> > +	if (rc < 0)
> > +		return rc;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * synps_edac_remove_sysfs_attributes - Removes sysfs entries
> > + * @mci:	Pointer to the edac memory controller instance
> > + *
> > + * Removes sysfs attributes.
> > + *
> > + * Return: none.
> > + */
> > +static void synps_edac_remove_sysfs_attributes(struct mem_ctl_info
> > +*mci) {
> > +	device_remove_file(&mci->dev, &dev_attr_inject_data_error);
> > +	device_remove_file(&mci->dev, &dev_attr_inject_data_poison); }
> > +
> > +/**
> > + * setup_address_map -	Set Address Map by querying ADDRMAP
> registers
> > + * @priv:		Pointer to synps_edac_priv struct
> > + *
> > + * Set Address Map by querying ADDRMAP registers
> > + * Return: none.
> > + */
> > +static void setup_address_map(struct synps_edac_priv *priv) {
> > +	u32 addrmap[12], addrmap_row_b2_10;
> > +	int index;
> > +	u32 width, memtype;
> > +
> > +	memtype = readl(priv->baseaddr + CTRL_OFST);
> > +	width = (memtype & ECC_CTRL_BUSWIDTH_MASK) >>
> > +ECC_CTRL_BUSWIDTH_SHIFT;
> > +
> > +	for (index = 0; index < 12; index++) {
> > +		u32 addrmap_offset;
> > +
> > +		addrmap_offset = ECC_ADDRMAP0_OFFSET + (index * 4);
> > +		addrmap[index] = readl(priv->baseaddr + addrmap_offset);
> > +	}
> > +
> > +	priv->row_shift[0] = (addrmap[5] & ROW_MAX_VAL_MASK) +
> ROW_B0_BASE;
> > +	priv->row_shift[1] = ((addrmap[5] >> 8) &
> > +			ROW_MAX_VAL_MASK) + ROW_B1_BASE;
> > +
> > +	addrmap_row_b2_10 = (addrmap[5] >> 16) & ROW_MAX_VAL_MASK;
> > +	if (addrmap_row_b2_10 != ROW_MAX_VAL_MASK) {
> > +		for (index = 2; index < 11; index++)
> > +			priv->row_shift[index] = addrmap_row_b2_10 +
> > +				index + ROW_B0_BASE;
> > +
> > +	} else {
> > +		priv->row_shift[2] = (addrmap[9] &
> > +				ROW_MAX_VAL_MASK) + ROW_B2_BASE;
> > +		priv->row_shift[3] = ((addrmap[9] >> 8) &
> > +				ROW_MAX_VAL_MASK) + ROW_B3_BASE;
> > +		priv->row_shift[4] = ((addrmap[9] >> 16) &
> > +				ROW_MAX_VAL_MASK) + ROW_B4_BASE;
> > +		priv->row_shift[5] = ((addrmap[9] >> 24) &
> > +				ROW_MAX_VAL_MASK) + ROW_B5_BASE;
> > +		priv->row_shift[6] = (addrmap[10] &
> > +				ROW_MAX_VAL_MASK) + ROW_B6_BASE;
> > +		priv->row_shift[7] = ((addrmap[10] >> 8) &
> > +				ROW_MAX_VAL_MASK) + ROW_B7_BASE;
> > +		priv->row_shift[8] = ((addrmap[10] >> 16) &
> > +				ROW_MAX_VAL_MASK) + ROW_B8_BASE;
> > +		priv->row_shift[9] = ((addrmap[10] >> 24) &
> > +				ROW_MAX_VAL_MASK) + ROW_B9_BASE;
> > +		priv->row_shift[10] = (addrmap[11] &
> > +				ROW_MAX_VAL_MASK) + ROW_B10_BASE;
> > +	}
> > +
> > +	priv->row_shift[11] = (((addrmap[5] >> 24) & ROW_MAX_VAL_MASK)
> ==
> > +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[5] >>
> 24) &
> > +				ROW_MAX_VAL_MASK) + ROW_B11_BASE);
> > +	priv->row_shift[12] = ((addrmap[6] & ROW_MAX_VAL_MASK) ==
> > +				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[6] &
> > +				ROW_MAX_VAL_MASK) + ROW_B12_BASE);
> > +	priv->row_shift[13] = (((addrmap[6] >> 8) & ROW_MAX_VAL_MASK) ==
> > +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >>
> 8) &
> > +				ROW_MAX_VAL_MASK) + ROW_B13_BASE);
> > +	priv->row_shift[14] = (((addrmap[6] >> 16) & ROW_MAX_VAL_MASK)
> ==
> > +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >>
> 16) &
> > +				ROW_MAX_VAL_MASK) + ROW_B14_BASE);
> > +	priv->row_shift[15] = (((addrmap[6] >> 24) & ROW_MAX_VAL_MASK)
> ==
> > +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[6] >>
> 24) &
> > +				ROW_MAX_VAL_MASK) + ROW_B15_BASE);
> > +	priv->row_shift[16] = ((addrmap[7] & ROW_MAX_VAL_MASK) ==
> > +				ROW_MAX_VAL_MASK) ? 0 : ((addrmap[7] &
> > +				ROW_MAX_VAL_MASK) + ROW_B16_BASE);
> > +	priv->row_shift[17] = (((addrmap[7] >> 8) & ROW_MAX_VAL_MASK) ==
> > +				ROW_MAX_VAL_MASK) ? 0 : (((addrmap[7] >>
> 8) &
> > +				ROW_MAX_VAL_MASK) + ROW_B17_BASE);
> > +
> > +	priv->col_shift[0] = 0;
> > +	priv->col_shift[1] = 1;
> > +	priv->col_shift[2] = (addrmap[2] & COL_MAX_VAL_MASK) +
> COL_B2_BASE;
> > +	priv->col_shift[3] = ((addrmap[2] >> 8) &
> > +			COL_MAX_VAL_MASK) + COL_B3_BASE;
> > +	priv->col_shift[4] = (((addrmap[2] >> 16) & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 16) &
> > +					COL_MAX_VAL_MASK) +
> COL_B4_BASE);
> > +	priv->col_shift[5] = (((addrmap[2] >> 24) & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[2] >> 24) &
> > +					COL_MAX_VAL_MASK) +
> COL_B5_BASE);
> > +	priv->col_shift[6] = ((addrmap[3] & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : ((addrmap[3] &
> > +					COL_MAX_VAL_MASK) +
> COL_B6_BASE);
> > +	priv->col_shift[7] = (((addrmap[3] >> 8) & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 8) &
> > +					COL_MAX_VAL_MASK) +
> COL_B7_BASE);
> > +	priv->col_shift[8] = (((addrmap[3] >> 16) & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 16) &
> > +					COL_MAX_VAL_MASK) +
> COL_B8_BASE);
> > +	priv->col_shift[9] = (((addrmap[3] >> 24) & COL_MAX_VAL_MASK) ==
> > +			COL_MAX_VAL_MASK) ? 0 : (((addrmap[3] >> 24) &
> > +					COL_MAX_VAL_MASK) +
> COL_B9_BASE);
> > +	if (width == DDRCTL_EWDTH_64) {
> > +		if (memtype & MEM_TYPE_LPDDR3) {
> > +			priv->col_shift[10] = ((addrmap[4] &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				((addrmap[4] & COL_MAX_VAL_MASK) +
> > +				 COL_B10_BASE);
> > +			priv->col_shift[11] = (((addrmap[4] >> 8) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B11_BASE);
> > +		} else {
> > +			priv->col_shift[11] = ((addrmap[4] &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				((addrmap[4] & COL_MAX_VAL_MASK) +
> > +				 COL_B10_BASE);
> > +			priv->col_shift[13] = (((addrmap[4] >> 8) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[4] >> 8) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B11_BASE);
> > +		}
> > +	} else if (width == DDRCTL_EWDTH_32) {
> > +		if (memtype & MEM_TYPE_LPDDR3) {
> > +			priv->col_shift[10] = (((addrmap[3] >> 24) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B9_BASE);
> > +			priv->col_shift[11] = ((addrmap[4] &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				((addrmap[4] & COL_MAX_VAL_MASK) +
> > +				 COL_B10_BASE);
> > +		} else {
> > +			priv->col_shift[11] = (((addrmap[3] >> 24) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B9_BASE);
> > +			priv->col_shift[13] = ((addrmap[4] &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				((addrmap[4] & COL_MAX_VAL_MASK) +
> > +				 COL_B10_BASE);
> > +		}
> > +	} else {
> > +		if (memtype & MEM_TYPE_LPDDR3) {
> > +			priv->col_shift[10] = (((addrmap[3] >> 16) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B8_BASE);
> > +			priv->col_shift[11] = (((addrmap[3] >> 24) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B9_BASE);
> > +			priv->col_shift[13] = ((addrmap[4] &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				((addrmap[4] & COL_MAX_VAL_MASK) +
> > +				 COL_B10_BASE);
> > +		} else {
> > +			priv->col_shift[11] = (((addrmap[3] >> 16) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 16) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B8_BASE);
> > +			priv->col_shift[13] = (((addrmap[3] >> 24) &
> > +				COL_MAX_VAL_MASK) ==
> COL_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[3] >> 24) & COL_MAX_VAL_MASK)
> +
> > +				 COL_B9_BASE);
> > +		}
> > +	}
> > +
> > +	if (width) {
> > +		for (index = 9; index > width; index--) {
> > +			priv->col_shift[index] = priv->col_shift[index - width];
> > +			priv->col_shift[index - width] = 0;
> > +		}
> > +	}
> > +
> > +	priv->bank_shift[0] = (addrmap[1] & BANK_MAX_VAL_MASK) +
> BANK_B0_BASE;
> > +	priv->bank_shift[1] = ((addrmap[1] >> 8) &
> > +				BANK_MAX_VAL_MASK) + BANK_B1_BASE;
> > +	priv->bank_shift[2] = (((addrmap[1] >> 16) &
> > +				BANK_MAX_VAL_MASK) ==
> BANK_MAX_VAL_MASK) ? 0 :
> > +				(((addrmap[1] >> 16) &
> BANK_MAX_VAL_MASK) +
> > +				 BANK_B2_BASE);
> > +
> > +	priv->bankgrp_shift[0] = (addrmap[8] &
> > +				BANKGRP_MAX_VAL_MASK) +
> BANKGRP_B0_BASE;
> > +	priv->bankgrp_shift[1] = (((addrmap[8] >> 8) &
> BANKGRP_MAX_VAL_MASK) ==
> > +				BANKGRP_MAX_VAL_MASK) ? 0 :
> (((addrmap[8] >> 8)
> > +				& BANKGRP_MAX_VAL_MASK) +
> BANKGRP_B1_BASE);
> > +
> > +	priv->rank_shift[0] = ((addrmap[0] & RANK_MAX_VAL_MASK) ==
> > +				RANK_MAX_VAL_MASK) ? 0 : ((addrmap[0] &
> > +				RANK_MAX_VAL_MASK) + RANK_B0_BASE); }
> 
> The assignments in that whole function are really unreadable. You could define
> macros maybe and hide all the details in them as the assignments look pretty
> regular...

I will try to improve the readability.

> 
> > +
> >  /**
> >   * synps_edac_mc_probe - Check controller and bind driver
> >   * @pdev:	Pointer to the platform_device struct
> > @@ -480,7 +1268,7 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> >  	struct mem_ctl_info *mci;
> >  	struct edac_mc_layer layers[2];
> >  	struct synps_edac_priv *priv;
> > -	int rc;
> > +	int rc, irq, status;
> >  	struct resource *res;
> >  	void __iomem *baseaddr;
> >  	const struct of_device_id *match;
> > @@ -529,6 +1317,27 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> >  		goto free_edac_mc;
> >  	}
> >
> > +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
> > +		irq = platform_get_irq(pdev, 0);
> > +		if (irq < 0) {
> > +			edac_printk(KERN_ERR, EDAC_MC,
> > +					"No irq %d in DT\n", irq);
> > +			return -ENODEV;
> 
> 			goto free_edac_mc;
> 
> otherwise you leak memory.
Okay.
> 
> > +		}
> > +
> > +		status = devm_request_irq(&pdev->dev, irq,
> > +			synps_edac_intr_handler,
> > +			0, dev_name(&pdev->dev), mci);
> > +		if (status < 0) {
> > +			edac_printk(KERN_ERR, EDAC_MC, "Failed to request
> Irq\n");
> > +			goto free_edac_mc;
> > +		}
> > +
> > +		/* Enable UE/CE Interrupts */
> > +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > +			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
> > +	}
> > +
> >  	rc = edac_mc_add_mc(mci);
> >  	if (rc) {
> >  		edac_printk(KERN_ERR, EDAC_MC,
> > @@ -536,11 +1345,24 @@ static int synps_edac_mc_probe(struct
> platform_device *pdev)
> >  		goto free_edac_mc;
> >  	}
> >
> > +	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT) {
> > +		if (synps_edac_create_sysfs_attributes(mci)) {
> > +			edac_printk(KERN_ERR, EDAC_MC,
> > +					"Failed to create sysfs entries\n");
> > +			goto free_edac_mc;
> > +		}
> > +	}
> > +
> > +	if (of_device_is_compatible(pdev->dev.of_node,
> > +				    "xlnx,zynqmp-ddrc-2.40a"))
> > +		setup_address_map(priv);
> > +
> >  	/*
> >  	 * Start capturing the correctable and uncorrectable errors. A write of
> >  	 * 0 starts the counters.
> >  	 */
> > -	writel(0x0, baseaddr + ECC_CTRL_OFST);
> > +	if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
> > +		writel(0x0, baseaddr + ECC_CTRL_OFST);
> >  	return rc;
> >
> >  free_edac_mc:
> > @@ -558,8 +1380,16 @@ static int synps_edac_mc_probe(struct
> > platform_device *pdev)  static int synps_edac_mc_remove(struct
> > platform_device *pdev)  {
> >  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> > +	struct synps_edac_priv *priv;
> >
> > +	priv = mci->pvt_info;
> > +	if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)
> > +		/* Disable UE/CE Interrupts */
> > +		writel((DDR_QOSUE_MASK | DDR_QOSCE_MASK),
> > +			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
> >  	edac_mc_del_mc(&pdev->dev);
> > +	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT)
> > +		synps_edac_remove_sysfs_attributes(mci);
> >  	edac_mc_free(mci);
> >
> >  	return 0;
> > --
> > 2.1.1
> >

Thanks,
Manish Narani

Re: [PATCH v4 3/4] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Borislav Petkov]

On Wed, Aug 22, 2018 at 01:02:40PM +0000, Manish Narani wrote:
>  I could remove 'synps_enh' from the prefix and keep 'zynq' and
> 'zynqmp' to differentiate. Please suggest.

Makes sense and it is shorter, sure.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH v4 4/4] arm64: zynqmp: Add DDRC node

[Manish Narani]

This patch adds ddrc memory controller node in dts. The size mentioned
in dts is 0x30000, because we need to access DDR_QOS INTR registers
located at fd090208 from this driver.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index a091e6f..7d6a3cf 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -355,6 +355,13 @@
 			xlnx,bus-width = <64>;
 		};
 
+		mc: memory-controller@fd070000 {
+			compatible = "xlnx,zynqmp-ddrc-2.40a";
+			reg = <0x0 0xfd070000 0x0 0x30000>;
+			interrupt-parent = <&gic>;
+			interrupts = <0 112 4>;
+		};
+
 		gem0: ethernet@ff0b0000 {
 			compatible = "cdns,zynqmp-gem", "cdns,gem";
 			status = "disabled";
-- 
2.1.1

RE: [PATCH v4 4/4] arm64: zynqmp: Add DDRC node

[Manish Narani]

Ping.

> -----Original Message-----
> From: Manish Narani [mailto:manish.narani@xilinx.com]
> Sent: Saturday, August 4, 2018 2:56 PM
> To: robh+dt@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; Michal Simek <michals@xilinx.com>; bp@alien8.de;
> mchehab@kernel.org; mdf@kernel.org; Edgar Iglesias <edgari@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Naga Sureshkumar Relli
> <nagasure@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>;
> stefan.krsmanovic@aggios.com
> Cc: Srinivas Goud <sgoud@xilinx.com>; Anirudha Sarangi
> <anirudh@xilinx.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> edac@vger.kernel.org; Manish Narani <MNARANI@xilinx.com>
> Subject: [PATCH v4 4/4] arm64: zynqmp: Add DDRC node
> 
> This patch adds ddrc memory controller node in dts. The size mentioned in dts
> is 0x30000, because we need to access DDR_QOS INTR registers located at
> fd090208 from this driver.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index a091e6f..7d6a3cf 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -355,6 +355,13 @@
>  			xlnx,bus-width = <64>;
>  		};
> 
> +		mc: memory-controller@fd070000 {
> +			compatible = "xlnx,zynqmp-ddrc-2.40a";
> +			reg = <0x0 0xfd070000 0x0 0x30000>;
> +			interrupt-parent = <&gic>;
> +			interrupts = <0 112 4>;
> +		};
> +
>  		gem0: ethernet@ff0b0000 {
>  			compatible = "cdns,zynqmp-gem", "cdns,gem";
>  			status = "disabled";
> --
> 2.1.1

Re: [PATCH v4 0/4] EDAC: Enhancements to Synopsys EDAC driver

[Borislav Petkov]

On Sat, Aug 04, 2018 at 02:55:31PM +0530, Manish Narani wrote:
> This patch series enhances the current EDAC driver to support different

This one, your 2/4 and 4/4 still says "this patch". Hurried too much?
How about slowing down and looking at them with a critical eye?

Also, how about waiting for a week before resending your patches so that
you give people chance to review them too?

While you wait, please read Documentation/process/submitting-patches.rst
- it'll help you with the submission modalities.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH v4 0/4] EDAC: Enhancements to Synopsys EDAC driver

[Manish Narani]

Hi Boris,


> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Saturday, August 4, 2018 10:03 PM
> Subject: Re: [PATCH v4 0/4] EDAC: Enhancements to Synopsys EDAC driver
> 
> On Sat, Aug 04, 2018 at 02:55:31PM +0530, Manish Narani wrote:
> > This patch series enhances the current EDAC driver to support
> > different
> 
> This one, your 2/4 and 4/4 still says "this patch". Hurried too much?
> How about slowing down and looking at them with a critical eye?
> 
> Also, how about waiting for a week before resending your patches so that you
> give people chance to review them too?
> 
> While you wait, please read Documentation/process/submitting-patches.rst
> - it'll help you with the submission modalities.

Yes, I missed it. Sorry for that. I will surely wait for around a week for others to review.
Thanks for the guidance.

Thanks,
Manish Narani