[PATCH v2 1/6] devicetree: Extend synopsys ddrc binding for Xilinx ZynqMP

[PATCH v2 1/6] devicetree: Extend synopsys ddrc binding for Xilinx ZynqMP

[Michal Simek]

Add ZynqMP support to Synopsys memory controller.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- New patch in this series

 .../devicetree/bindings/memory-controllers/synopsys.txt    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
index a43d26d41e04..08c058d5ad76 100644
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
@@ -5,11 +5,23 @@ configuration. The ECC controller corrects one bit error and detects
 two bit errors.
 
 Required properties:
- - compatible: Should be 'xlnx,zynq-ddrc-a05'
+ - compatible: Should be 'xlnx,zynq-ddrc-a05' for Zynq
+	       and 'xlnx,zynqmp-ddrc-2.40a' for ZynqMP
  - reg: Base address and size of the controllers memory area
+ - interrupt-parent: Should be core interrupt controller
+		     valid for ZynqMP DDR Controller
+ - interrupts: Property with a value describing the interrupt number
+	       valid for ZynqMP DDR Controller
 
 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>;
+	};
-- 
1.9.1

[PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Michal Simek]

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

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

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Update commit message
- Update patch subject
- Update kernel-doc description for struct synps_platform_data
- Change synps_platform_data pointer names and update code

 drivers/edac/synopsys_edac.c | 70 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 0c9c59e2b5a3..293380f884fe 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"
 
@@ -95,6 +96,9 @@
 #define SCRUB_MODE_MASK		0x7
 #define SCRUB_MODE_SECDED	0x4
 
+/* DDR ECC Quirks */
+#define DDR_ECC_INTR_SUPPORT		BIT(0)
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -130,6 +134,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,11 +142,29 @@ 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 which returns the current ecc error info
+ * @edac_get_mtype:	function which returns the memory type
+ * @edac_get_dtype:	function which returns the DIMM type
+ * @edac_get_eccstate:	function which returns the ecc enable/disable status
+ * @quirks:		a bitfield of quirks
+ */
+struct synps_platform_data {
+	int (*edac_geterror_info)(void __iomem *base,
+				  struct synps_ecc_status *p);
+	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
@@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci)
 	struct synps_edac_priv *priv = mci->pvt_info;
 	int status;
 
-	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
+	status = priv->p_data->edac_geterror_info(priv->baseaddr,
+						  &priv->stat);
 	if (status)
 		return;
 
@@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
 		for (j = 0; j < csi->nr_channels; j++) {
 			dimm            = csi->channels[j]->dimm;
 			dimm->edac_mode = EDAC_FLAG_SECDED;
-			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
+			dimm->mtype     = priv->p_data->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->dtype     = priv->p_data->edac_get_dtype(
+						priv->baseaddr);
 		}
 	}
 
@@ -423,6 +449,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 +481,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 +518,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,
@@ -486,7 +538,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 	 * 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:
@@ -511,13 +564,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",
-- 
1.9.1

Re: [PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Borislav Petkov]

On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote:
> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> 
> Adds platform specific structures, so that we can add
> different IP support later using quirks.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:

Subject says "2/6" but I can't find 1/6 in my mbox. Nothing in spam
folder either. What's up?

Are you using the --cover-letter option to git format-patch ?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Michal Simek]

On 11.8.2017 11:09, Borislav Petkov wrote:
> On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote:
>> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
>>
>> Adds platform specific structures, so that we can add
>> different IP support later using quirks.
>>
>> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v2:
> 
> Subject says "2/6" but I can't find 1/6 in my mbox. Nothing in spam
> folder either. What's up?

https://lkml.org/lkml/2017/8/7/105
ACK by Rob yesterday.

> 
> Are you using the --cover-letter option to git format-patch ?

I am using patman - u-boot tools for sending patch. It read MAINTAINERS
file and copy right people.
It has option to explicitly CC people. I need to resend this as v3
anyway that's why I will add you there.

Thanks,
Michal

Re: [PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Borislav Petkov]

On Fri, Aug 11, 2017 at 11:16:15AM +0200, Michal Simek wrote:
> https://lkml.org/lkml/2017/8/7/105
> ACK by Rob yesterday.

Ok, pls bounce it to me as I don't have it.

Also, pls refrain from using lkml.org. Simply do:

http://lkml.kernel.org/r/<Message-ID>

> I am using patman - u-boot tools for sending patch. It read MAINTAINERS
> file and copy right people.
> It has option to explicitly CC people. I need to resend this as v3
> anyway that's why I will add you there.

Yes, if you send cross-maintainer changes, please make sure to CC them on all
patches because otherwise stuff like that happens and people start wondering
where is the rest and so on.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Michal Simek]

On 11.8.2017 11:22, Borislav Petkov wrote:
> On Fri, Aug 11, 2017 at 11:16:15AM +0200, Michal Simek wrote:
>> https://lkml.org/lkml/2017/8/7/105
>> ACK by Rob yesterday.
> 
> Ok, pls bounce it to me as I don't have it.

ok.

> 
> Also, pls refrain from using lkml.org. Simply do:
> 
> http://lkml.kernel.org/r/<Message-ID>

Interesting. I have never seen this before.

http://lkml.kernel.org/r/91fd6532076e4c905b5a228d852bba4941c54a28.1502091561.git.michal.simek@xilinx.com

> 
>> I am using patman - u-boot tools for sending patch. It read MAINTAINERS
>> file and copy right people.
>> It has option to explicitly CC people. I need to resend this as v3
>> anyway that's why I will add you there.
> 
> Yes, if you send cross-maintainer changes, please make sure to CC them on all
> patches because otherwise stuff like that happens and people start wondering
> where is the rest and so on.

ok. Anyway do you see something else what I should fix in v3?

Thanks,
Michal

Re: [PATCH v2 2/6] edac: synopsys: Add platform specific structures for ddrc controller

[Borislav Petkov]

On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote:
> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> 
> Adds platform specific structures, so that we can add

"Add platform-specific structures, ..."

i.e., "Do this and do that" formulation. Fix that in the following
patches too pls.

> different IP support later using quirks.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - Update commit message
> - Update patch subject
> - Update kernel-doc description for struct synps_platform_data
> - Change synps_platform_data pointer names and update code
> 
>  drivers/edac/synopsys_edac.c | 70 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 0c9c59e2b5a3..293380f884fe 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"
>  
> @@ -95,6 +96,9 @@
>  #define SCRUB_MODE_MASK		0x7
>  #define SCRUB_MODE_SECDED	0x4
>  
> +/* DDR ECC Quirks */

quirks?

How is reporting ECC with an interrupt a quirk? Or whatever that define
is...

> +#define DDR_ECC_INTR_SUPPORT		BIT(0)
> +
>  /**
>   * struct ecc_error_info - ECC error log information
>   * @row:	Row number
> @@ -130,6 +134,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,11 +142,29 @@ 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 which returns the current ecc error info
> + * @edac_get_mtype:	function which returns the memory type
> + * @edac_get_dtype:	function which returns the DIMM type
> + * @edac_get_eccstate:	function which returns the ecc enable/disable status
> + * @quirks:		a bitfield of quirks
> + */
> +struct synps_platform_data {
> +	int (*edac_geterror_info)(void __iomem *base,
> +				  struct synps_ecc_status *p);
> +	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;

You can just as well drop the "edac_" prefix too - those are
driver-local things and not EDAC-core.

> +};
> +
> +/**
>   * 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
> @@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci)
>  	struct synps_edac_priv *priv = mci->pvt_info;
>  	int status;
>  
> -	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
> +	status = priv->p_data->edac_geterror_info(priv->baseaddr,
> +						  &priv->stat);
>  	if (status)
>  		return;
>  
> @@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
>  		for (j = 0; j < csi->nr_channels; j++) {
>  			dimm            = csi->channels[j]->dimm;
>  			dimm->edac_mode = EDAC_FLAG_SECDED;
> -			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
> +			dimm->mtype     = priv->p_data->edac_get_mtype(
> +						priv->baseaddr);

Always let it stick out, never break on the opening brace. checkpatch
should catch that but it doesn't, for some reason.

>  			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi->nr_channels;
>  			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
> -			dimm->dtype     = synps_edac_get_dtype(priv->baseaddr);
> +			dimm->dtype     = priv->p_data->edac_get_dtype(
> +						priv->baseaddr);
>  		}
>  	}
>  
> @@ -423,6 +449,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,

	.geterror_info	= synps_geterror_info,

and so on looks perfectly fine to me.

> +	.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 +481,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))) {

This patch does more than just adding platform-specific structures and
the commit message is not talking about it.

>  		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
>  		return -ENXIO;
>  	}
> @@ -468,6 +518,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,
> @@ -486,7 +538,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
>  	 * 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);

Ditto.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 3/6] edac: synopsys: Add EDAC ECC support for ZynqMP DDRC

[Michal Simek]

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

This patch adds EDAC ECC support for ZynqMP DDRC IP

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Add binding doc to this series to resolve checkpatch warning
- Rebased on the top of
  https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=for-next
  and resolve conflict caused by "EDAC: Get rid of mci->mod_ver" patch
- Add changes done in previous patch

 drivers/edac/Kconfig         |   2 +-
 drivers/edac/synopsys_edac.c | 305 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 302 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2aeed18..e2f62dda8944 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -445,7 +445,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 293380f884fe..11016cd13a08 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"
@@ -99,6 +100,87 @@
 /* DDR ECC Quirks */
 #define DDR_ECC_INTR_SUPPORT		BIT(0)
 
+/* 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
+
+/* 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
+
+/* 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
+
+/* DDR Memory type defines */
+#define MEM_TYPE_DDR3	0x1
+#define MEM_TYPE_LPDDR3	0x1
+#define MEM_TYPE_DDR2	0x4
+#define MEM_TYPE_DDR4	0x10
+#define MEM_TYPE_LPDDR4	0x10
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -106,6 +188,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;
@@ -113,6 +197,8 @@ struct ecc_error_info {
 	u32 bank;
 	u32 bitpos;
 	u32 data;
+	u32 bankgrpnr;
+	u32 blknr;
 };
 
 /**
@@ -171,7 +257,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(void __iomem *base,
 				    struct synps_ecc_status *p)
@@ -219,6 +305,65 @@ static int synps_edac_geterror_info(void __iomem *base,
 }
 
 /**
+ * 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(void __iomem *base,
+					struct synps_ecc_status *p)
+{
+	u32 regval, clearval = 0;
+
+	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
@@ -255,6 +400,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;
+	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 = priv->p_data->edac_geterror_info(priv->baseaddr,
+						  &priv->stat);
+	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
  *
@@ -310,6 +490,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
  *
@@ -335,6 +549,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
@@ -373,6 +613,32 @@ 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 = MEM_UNKNOWN;
+	u32 memtype;
+
+	memtype = readl(base + CTRL_OFST);
+
+	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;
+
+	return mt;
+}
+
+/**
  * synps_edac_init_csrows - Initialize the cs row data
  * @mci:	Pointer to the edac memory controller instance
  *
@@ -440,8 +706,12 @@ 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);
@@ -457,8 +727,18 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	.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,
+};
+
 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 */ }
 };
 
@@ -478,7 +758,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;
@@ -527,6 +807,23 @@ 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;
+		}
+	}
+
 	rc = edac_mc_add_mc(mci);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC,
-- 
1.9.1

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

[Borislav Petkov]

On Mon, Aug 07, 2017 at 09:39:25AM +0200, Michal Simek wrote:
> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> 
> This patch adds EDAC ECC support for ZynqMP DDRC IP

It does much more and the commit message could talk about it.

> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - Add binding doc to this series to resolve checkpatch warning
> - Rebased on the top of
>   https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=for-next
>   and resolve conflict caused by "EDAC: Get rid of mci->mod_ver" patch
> - Add changes done in previous patch
> 
>  drivers/edac/Kconfig         |   2 +-
>  drivers/edac/synopsys_edac.c | 305 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 302 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 96afb2aeed18..e2f62dda8944 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -445,7 +445,7 @@ config EDAC_ALTERA_SDMMC
>  
>  config EDAC_SYNOPSYS
>  	tristate "Synopsys DDR Memory Controller"
> -	depends on ARCH_ZYNQ
> +	depends on ARCH_ZYNQ || ARM64

This is an unrelated change and it needs a separate patch and a commit
message explaining that you're enabling the driver on arm64 now too.

>  	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 293380f884fe..11016cd13a08 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"
> @@ -99,6 +100,87 @@
>  /* DDR ECC Quirks */
>  #define DDR_ECC_INTR_SUPPORT		BIT(0)
>  
> +/* 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

Some of those are unused. Kill them if they remain unused.

> +
> +/* 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
> +
> +/* 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
> +
> +/* 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
> +
> +/* DDR Memory type defines */
> +#define MEM_TYPE_DDR3	0x1
> +#define MEM_TYPE_LPDDR3	0x1
> +#define MEM_TYPE_DDR2	0x4
> +#define MEM_TYPE_DDR4	0x10
> +#define MEM_TYPE_LPDDR4	0x10
> +
>  /**
>   * struct ecc_error_info - ECC error log information
>   * @row:	Row number
> @@ -106,6 +188,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;
> @@ -113,6 +197,8 @@ struct ecc_error_info {
>  	u32 bank;
>  	u32 bitpos;
>  	u32 data;
> +	u32 bankgrpnr;
> +	u32 blknr;

u32? Can those fit in a smaller integer?

>  };
>  
>  /**
> @@ -171,7 +257,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

So you corrected this to use numbers (1 and 0) which is as arbitrary
change as any...

>   */
>  static int synps_edac_geterror_info(void __iomem *base,
>  				    struct synps_ecc_status *p)
> @@ -219,6 +305,65 @@ static int synps_edac_geterror_info(void __iomem *base,
>  }
>  
>  /**
> + * 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

... and yet copied the old text and didn't change it here. Looks like
this needs making up mind.

> + */
> +static int synps_enh_edac_geterror_info(void __iomem *base,
> +					struct synps_ecc_status *p)

And you have "_edac_" in all those functions which are static and which
only encumbers readability. I think naming scheme like

	get_error_info
	zynq_mp_get_error_info
	...

should be much easier on the eyes.

> +{
> +	u32 regval, clearval = 0;
> +
> +	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);

Align vertically and let it stick out for better readability, like this:

	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);

Ditto.

> +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
> @@ -255,6 +400,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.

s/[iI]sr/ISR/g

> + * 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;
> +	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;

newline.

> +	status = priv->p_data->edac_geterror_info(priv->baseaddr,
> +						  &priv->stat);

Let it stick out.

> +	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
>   *
> @@ -310,6 +490,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.

"... or unknown."

> + */
> +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 - the 80 cols rule is not a hard one.

> +	switch (width) {
> +	case DDRCTL_EWDTH_16:
> +		dt = DEV_X2;

You can save yourself the assignment if you do

		return DEV_X2;

here and below, respectively.

> +		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
>   *
> @@ -335,6 +549,32 @@ static bool synps_edac_get_eccstate(void __iomem *base)
>  }
>  
>  /**
> + * synps_enh_edac_get_eccstate - Return the controller ecc enable/disable status

s/ecc/ECC/g

> + * @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;

Ditto: you don't need the assignment here - just return the boolean value.

> +}
> +
> +/**
>   * synps_edac_get_memsize - reads the size of the attached memory device
>   *
>   * Return: the memory size in bytes
> @@ -373,6 +613,32 @@ 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 = MEM_UNKNOWN;
> +	u32 memtype;
> +
> +	memtype = readl(base + CTRL_OFST);
> +
> +	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;
> +
> +	return mt;

Ditto.

> +}
> +
> +/**
>   * synps_edac_init_csrows - Initialize the cs row data
>   * @mci:	Pointer to the edac memory controller instance
>   *
> @@ -440,8 +706,12 @@ 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);
> @@ -457,8 +727,18 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
>  	.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,
> +};
> +
>  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 */ }
>  };
>  
> @@ -478,7 +758,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;
> @@ -527,6 +807,23 @@ 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;

If you return here, you're leaking 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;
> +		}
> +	}
> +
>  	rc = edac_mc_add_mc(mci);
>  	if (rc) {
>  		edac_printk(KERN_ERR, EDAC_MC,
> -- 
> 1.9.1
> 

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 4/6] edac: synopsys: Add ECC error injection support

[Michal Simek]

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

The ZynqMP DDRC controller has data poisoning support
to inject CE or UE errors. this patch adds this support
using sysfs attributes.

created the following sysfs entries to support this.
-> /sys/devices/system/edac/mc/mc0/inject_data_poison
-> /sys/devices/system/edac/mc/mc0/inject_data_error

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- s/ecc/ECC/ is subject
- fix function parameters alignment
- fix some kernel-doc descriptions

 drivers/edac/synopsys_edac.c | 288 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 287 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 11016cd13a08..4cd9e3d8161c 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -99,6 +99,7 @@
 
 /* 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 */
@@ -174,6 +175,11 @@
 #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	0x1
@@ -181,6 +187,38 @@
 #define MEM_TYPE_DDR4	0x10
 #define MEM_TYPE_LPDDR4	0x10
 
+/* 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	0
+#define DDRC_MSTR_DEV_CONFIG_X8_MASK	1
+#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x10
+#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0X11
+
+/* DDR4 and DDR3 device Row,Column,Bank Mapping */
+#define DDR4_COL_SHIFT		3
+#define DDR4_BANKGRP_SHIFT	13
+#define DDR4_BANK_SHIFT		15
+#define DDR4_ROW_SHIFT		17
+#define DDR4_COL_MASK		0x3FF
+#define DDR4_BANKGRP_MASK	0x3
+#define DDR4_BANK_MASK		0x3
+#define DDR4_ROW_MASK		0x7FFF
+
+#define DDR3_COL_SHIFT		3
+#define DDR3_BANK_SHIFT		13
+#define DDR3_ROW_SHIFT		16
+#define DDR3_COL_MASK		0x3FF
+#define DDR3_BANK_MASK		0x7
+#define DDR3_ROW_MASK		0x3FFF
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -223,6 +261,7 @@ struct synps_ecc_status {
  * @p_data:	Pointer to platform data
  * @ce_cnt:	Correctable Error count
  * @ue_cnt:	Uncorrectable Error count
+ * @poison_addr: Data poison address
  */
 struct synps_edac_priv {
 	void __iomem *baseaddr;
@@ -231,6 +270,7 @@ struct synps_edac_priv {
 	const struct synps_platform_data *p_data;
 	u32 ce_cnt;
 	u32 ue_cnt;
+	ulong poison_addr;
 };
 
 /**
@@ -732,7 +772,8 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	.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,
+	.quirks			= (DDR_ECC_INTR_SUPPORT |
+				   DDR_ECC_DATA_POISON_SUPPORT),
 };
 
 static const struct of_device_id synps_edac_match[] = {
@@ -744,6 +785,240 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 
 MODULE_DEVICE_TABLE(of, synps_edac_match);
 
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+/**
+ * ddr4_poison_setup - update poison registers
+ * @dttype:		Device structure variable
+ * @device_config:	Device configuration
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Update poison registers as per ddr4 mapping
+ */
+static void ddr4_poison_setup(enum dev_type dttype, int device_config,
+			      struct synps_edac_priv *priv)
+{
+	int col, row, bank, bankgrp, regval, shift_val = 0, col_shift;
+
+	/* Check the Configuration of the device */
+	if (device_config & DDRC_MSTR_DEV_CONFIG_X8_MASK) {
+		/* For Full Dq bus */
+		if (dttype == DEV_X8)
+			shift_val = 0;
+		/* For Half Dq bus */
+		else if (dttype == DEV_X4)
+			shift_val = 1;
+		col_shift = 0;
+	} else if (device_config & DDRC_MSTR_DEV_CONFIG_X16_MASK) {
+		if (dttype == DEV_X8)
+			shift_val = 1;
+		else if (dttype == DEV_X4)
+			shift_val = 2;
+		col_shift = 1;
+	}
+
+	col = (priv->poison_addr >> (DDR4_COL_SHIFT -
+				(shift_val - col_shift))) &
+				DDR4_COL_MASK;
+	row = priv->poison_addr >> (DDR4_ROW_SHIFT - shift_val);
+	row &= DDR4_ROW_MASK;
+	bank = priv->poison_addr >> (DDR4_BANK_SHIFT - shift_val);
+	bank &= DDR4_BANK_MASK;
+	bankgrp = (priv->poison_addr >> (DDR4_BANKGRP_SHIFT -
+				(shift_val - col_shift))) &
+				DDR4_BANKGRP_MASK;
+
+	writel(col, priv->baseaddr + ECC_POISON0_OFST);
+	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
+		 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
+}
+
+/**
+ * ddr3_poison_setup - update poison registers
+ * @dttype:		Device structure variable
+ * @device_config:	Device configuration
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Update poison registers as per ddr3 mapping
+ */
+static void ddr3_poison_setup(enum dev_type dttype, int device_config,
+			      struct synps_edac_priv *priv)
+{
+	int col, row, bank, bankgrp, regval, shift_val = 0;
+
+	if (dttype == DEV_X8)
+		/* For Full Dq bus */
+		shift_val = 0;
+	else if (dttype == DEV_X4)
+		/* For Half Dq bus */
+		shift_val = 1;
+
+	col = (priv->poison_addr >> (DDR3_COL_SHIFT - shift_val)) &
+		DDR3_COL_MASK;
+	row = priv->poison_addr >> (DDR3_ROW_SHIFT - shift_val);
+	row &= DDR3_ROW_MASK;
+	bank = priv->poison_addr >> (DDR3_BANK_SHIFT - shift_val);
+	bank &= DDR3_BANK_MASK;
+	bankgrp = 0;
+	writel(col, priv->baseaddr + ECC_POISON0_OFST);
+	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
+			 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
+}
+
+/**
+ * synps_edac_mc_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
+synps_edac_mc_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);
+}
+
+/**
+ * synps_edac_mc_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
+synps_edac_mc_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;
+	int device_config;
+	enum mem_type mttype;
+	enum dev_type dttype;
+
+	mttype = priv->p_data->edac_get_mtype(priv->baseaddr);
+	dttype = priv->p_data->edac_get_dtype(priv->baseaddr);
+	if (kstrtoul(data, 0, &priv->poison_addr))
+		return -EINVAL;
+
+	device_config = readl(priv->baseaddr + CTRL_OFST);
+	device_config = (device_config & DDRC_MSTR_DEV_CONFIG_MASK) >>
+					DDRC_MSTR_DEV_CONFIG_SHIFT;
+	if (mttype == MEM_DDR4)
+		ddr4_poison_setup(dttype, device_config, priv);
+	else if (mttype == MEM_DDR3)
+		ddr3_poison_setup(dttype, device_config, priv);
+
+	return count;
+}
+
+/**
+ * synps_edac_mc_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
+synps_edac_mc_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 ?
+		       "Correctable Error" : "UnCorrectable Error");
+}
+
+/**
+ * synps_edac_mc_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
+synps_edac_mc_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(inject_data_error, 0644,
+	    synps_edac_mc_inject_data_error_show,
+	    synps_edac_mc_inject_data_error_store);
+static DEVICE_ATTR(inject_data_poison, 0644,
+	    synps_edac_mc_inject_data_poison_show,
+	    synps_edac_mc_inject_data_poison_store);
+
+/**
+ * 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.
+ */
+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);
+}
+
 /**
  * synps_edac_mc_probe - Check controller and bind driver
  * @pdev:	Pointer to the platform_device struct
@@ -831,6 +1106,13 @@ 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;
+		}
+	}
 	/*
 	 * Start capturing the correctable and uncorrectable errors. A write of
 	 * 0 starts the counters.
@@ -854,8 +1136,12 @@ 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;
 	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;
-- 
1.9.1

Re: [PATCH v2 4/6] edac: synopsys: Add ECC error injection support

[Borislav Petkov]

On Mon, Aug 07, 2017 at 09:39:26AM +0200, Michal Simek wrote:
> @@ -181,6 +187,38 @@
>  #define MEM_TYPE_DDR4	0x10
>  #define MEM_TYPE_LPDDR4	0x10
>  
> +/* 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	0
> +#define DDRC_MSTR_DEV_CONFIG_X8_MASK	1
> +#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x10
> +#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0X11

Ox

> +
> +/* DDR4 and DDR3 device Row,Column,Bank Mapping */
> +#define DDR4_COL_SHIFT		3
> +#define DDR4_BANKGRP_SHIFT	13
> +#define DDR4_BANK_SHIFT		15
> +#define DDR4_ROW_SHIFT		17
> +#define DDR4_COL_MASK		0x3FF
> +#define DDR4_BANKGRP_MASK	0x3
> +#define DDR4_BANK_MASK		0x3
> +#define DDR4_ROW_MASK		0x7FFF
> +
> +#define DDR3_COL_SHIFT		3
> +#define DDR3_BANK_SHIFT		13
> +#define DDR3_ROW_SHIFT		16
> +#define DDR3_COL_MASK		0x3FF
> +#define DDR3_BANK_MASK		0x7
> +#define DDR3_ROW_MASK		0x3FFF
> +
>  /**
>   * struct ecc_error_info - ECC error log information
>   * @row:	Row number
> @@ -223,6 +261,7 @@ struct synps_ecc_status {
>   * @p_data:	Pointer to platform data
>   * @ce_cnt:	Correctable Error count
>   * @ue_cnt:	Uncorrectable Error count
> + * @poison_addr: Data poison address
>   */
>  struct synps_edac_priv {
>  	void __iomem *baseaddr;
> @@ -231,6 +270,7 @@ struct synps_edac_priv {
>  	const struct synps_platform_data *p_data;
>  	u32 ce_cnt;
>  	u32 ue_cnt;
> +	ulong poison_addr;

unsigned long

Also, this error injection interface needs to go to debugfs and be
behind CONFIG_EDAC_DEBUG. We don't want error injection capability
present on production systems.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 5/6] edac: synopsys: Update ECC error message info

[Michal Simek]

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

This patch updates the ECC error message info
for zynqmp ddrc. added Block number and Bankgroup
in the message info.

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- s/ecc/ECC/ in commit message

 drivers/edac/synopsys_edac.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 4cd9e3d8161c..24490334a4f2 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -418,9 +418,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, "");
@@ -428,9 +436,16 @@ 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, "");
-- 
1.9.1

Re: [PATCH v2 5/6] edac: synopsys: Update ECC error message info

[Borislav Petkov]

On Mon, Aug 07, 2017 at 09:39:27AM +0200, Michal Simek wrote:
> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
> 
> This patch updates the ECC error message info
> for zynqmp ddrc. added Block number and Bankgroup
> in the message info.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - s/ecc/ECC/ in commit message
> 
>  drivers/edac/synopsys_edac.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 4cd9e3d8161c..24490334a4f2 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -418,9 +418,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 (!priv->...)

> +			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, "");
> @@ -428,9 +436,16 @@ 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)

Ditto.

> +			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,

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

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

And above too. Put strings on a single line for easier grepping.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 6/6] edac: synopsys: Enable CE and UE interrupts for ZynqMP DDRC

[Michal Simek]

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

This patch enables Corrected and Uncorrected Error
interrupts for ZynqMP DDRC controller

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Reviewed-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Fix function alignment
- Fix macro indentation

 drivers/edac/synopsys_edac.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 24490334a4f2..f185a28d2db3 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -165,6 +165,8 @@
 #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
@@ -1112,6 +1114,10 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 			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);
@@ -1154,6 +1160,10 @@ static int synps_edac_mc_remove(struct platform_device *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);
-- 
1.9.1

Re: [PATCH v2 1/6] devicetree: Extend synopsys ddrc binding for Xilinx ZynqMP

[Rob Herring]

On Mon, Aug 07, 2017 at 09:39:23AM +0200, Michal Simek wrote:
> Add ZynqMP support to Synopsys memory controller.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - New patch in this series
> 
>  .../devicetree/bindings/memory-controllers/synopsys.txt    | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

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