RE: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Sherry Sun]

Hi Robert,

> -----Original Message-----
> From: Robert Richter <rrichter@xxxxxxxxxxx>
> Sent: 2020年4月2日 15:22
> To: Sherry Sun <sherry.sun@xxxxxxx>
> Cc: bp@xxxxxxxxx; mchehab@xxxxxxxxxx; tony.luck@xxxxxxxxx;
> james.morse@xxxxxxx; michal.simek@xxxxxxxxxx;
> manish.narani@xxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Frank Li
> <frank.li@xxxxxxx>
> Subject: Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for
> i.MX8MP
> 
> On 02.04.20 09:20:32, Sherry Sun wrote:
> > Since i.MX8MP use synopsys ddr controller IP, so add edac support for
> > i.MX8MP based on synopsys edac driver. i.MX8MP use LPDDR4 and support
> > interrupts for corrected and uncorrected errors. The main difference
> > between ZynqMP and i.MX8MP ddr controller is the interrupt registers.
> > So add another interrupt handler function, enable/disable interrupt
> > function to distinguish with ZynqMP.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > ---
> >  drivers/edac/synopsys_edac.c | 77
> > +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> > +static void enable_intr_imx8mp(struct synps_edac_priv *priv) {
> > +	int regval;
> > +
> > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > +	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > +
> > +static void disable_intr_imx8mp(struct synps_edac_priv *priv) {
> > +	int regval;
> > +
> > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > +	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > +
> > +/* Interrupt Handler for ECC interrupts on imx8mp platform. */ static
> > +irqreturn_t intr_handler_imx8mp(int irq, void *dev_id) {
> > +	const struct synps_platform_data *p_data;
> > +	struct mem_ctl_info *mci = dev_id;
> > +	struct synps_edac_priv *priv;
> > +	int status, regval;
> > +
> > +	priv = mci->pvt_info;
> > +	p_data = priv->p_data;
> > +
> > +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> > +	if (!(regval & ECC_INTR_MASK))
> > +		return IRQ_NONE;
> > +
> > +	status = p_data->get_error_info(priv);
> > +	if (status)
> > +		return IRQ_NONE;
> > +
> > +	priv->ce_cnt += priv->stat.ce_cnt;
> > +	priv->ue_cnt += priv->stat.ue_cnt;
> > +	handle_error(mci, &priv->stat);
> > +
> > +	edac_dbg(3, "Total error count CE %d UE %d\n",
> > +		 priv->ce_cnt, priv->ue_cnt);
> > +	enable_intr_imx8mp(priv);
> 
> Why do you enable interrupts here?

Because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so here have to re-enable the interrupts.

As said in the commit, the main difference between ZynqMP and i.MX8MP ddr controller is the interrupt registers.
ZynqMP use DDR QOS Interrupt registers,  but i.MX8MP use ECC_CLR_OFST Register(bit8 and bit9) to enable/disable the ce/ue interrupts. 

In zynqmp_get_error_info(), Zynqmp wrote 0 to ECC_CLR_OFST register to clear CE/UE error flags and counts, it has no effect on Zynqmp interrupts. 
But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE interrupt, so need re-enable the interrupts.

Best regards
Sherry Sun

> 
> -Robert
> 
> > +
> > +	return IRQ_HANDLED;
> > +}