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

[Manish Narani <MNARANI@xilinx.com>]

Hi Boris,

Thanks a lot for the review.

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

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

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

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

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

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

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

I will try to improve the readability.

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

Thanks,
Manish Narani