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

[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",

[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

[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.

[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.

[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