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

[Sherry Sun <sherry.sun@nxp.com>]

Hi James,

> -----Original Message-----
> From: James Morse <james.morse@arm.com>
> Sent: 2020年2月27日 1:13
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: bp@alien8.de; mchehab@kernel.org; tony.luck@intel.com;
> rrichter@marvell.com; michal.simek@xilinx.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; robh+dt@kernel.org; mark.rutland@arm.com;
> linux-edac@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com>
> Subject: Re: [PATCH 3/3] EDAC: synopsys: Add edac driver support for
> i.MX8MP
> 
> Hi Sherry,
> 
> On 21/02/2020 06:39, sherry sun wrote:
> > From: Sherry Sun <sherry.sun@nxp.com>
> >
> > 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.
> 
> Same, but different! Is there any more information on how this difference
> comes about?

First of all, thanks for your comments.

The only difference between ZynqMP and i.MX8MP is the interrupt register, because I didn't find DDR QOS Interrupt registers as ZynqMP used in synopsys DDRC Databook, instead, it use ECC Clear Register to enable or disable the ce/ue interrupts. (In synopsys ddrc databook, the name of this register is ECCCTL, maybe it is better to rename the register in this driver form ECCCLR to ECCCTL to avoid misunderstanding, but the description of this register in databook is ECC Clear Register. I'm also confused.)

Actually I have no idea about DDR_QOS_IRQ since I can't find it in synopsys DDRC Databook, I guess it  maybe the private register of ZynqMP?
Or ZynqMP and i.MX8MP use different version of synopsys DDRC Databook? But expect the interrupt register, all the other registers  are the same.

> 
> It looks like the existing users of this driver all used DDR_QOS_IRQ, but yours
> uses DDR_CE_INTR and DDR_UE_INTR. I note you didn't add a new register
> offset, so this must be a feature of the IP that i.MX8MP uses, but others don't.
> 
> Is it possible to describe the feature in the DT instead of quirking based on
> the compatible?
> 
> Ideally someone else with the same configuration in a different SoC should
> be able to use the new parts of this driver without changing the code to quirk
> their platform too.

As talked above, I think i.MX8MP uses the general interrupt feature of the IP since it use the common register ECCCTL. And the DDR_QOS_IRQ maybe the feature of the IP or ZynqMP. But I'm not very sure. So should I still describe it in the DT?

> 
> 
> > diff --git a/drivers/edac/synopsys_edac.c
> > b/drivers/edac/synopsys_edac.c index 2d263382d797..66c801502212
> 100644
> > --- a/drivers/edac/synopsys_edac.c
> > +++ b/drivers/edac/synopsys_edac.c
> 
> > @@ -524,6 +530,54 @@ static void handle_error(struct mem_ctl_info
> > *mci, struct synps_ecc_status *p)
> 
> > +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); }
> 
> I assume these two interrupts are combined as one line. i.e. this driver can't
> race with itself.
> 
> Was this an integration choice? Could someone else use wire the
> DDR_CE_INTR and DDR_UE_INTR interrupts separately so that two CPUs take
> them in parallel?
> 

Actually I enable or disable CE/UE interrupts in one function because I followed the way ZynqMP do.
If it needed, I can separate CE/UE interrupts into different enable or disable functions.

> 
> ECC_CLR_OFST looks mighty like one of these write-only 'clear pending irqs'
> register. I'm surprised you read it, then write to it to enable interrupts... but I
> don't have the documentation!
> 
As explained above, the name of the register is actually ECCCTL, but the description is ECC Clear Register in databook, and it can enable/disable interrupts.

> 
> > +/* 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;
> 
> zynqmp_get_error_info(), which you call via p_data->get_error_info() does
> this too, so this is redundant.
> 

In intr_handler(), when it calls intr_handler_imx8mp(), it uses return intr_handler_imx8mp(), so p_data->get_error_info() will only be executed once.

> 
> > +	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);
> 
> This is the same as the existing intr_handler()...

Same as above, this will be called only once.

> 
> 
> > +	enable_intr_imx8mp(priv);
> 
> Is this because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so now
> you have to re-enable the interrrupts?
> 
> It looks like you are hacking round the problem!

Yes, you are right. Because Zynqmp only use ECC_CLR_OFST register to clear CE/UE error flags and counts, so it has no effect on Zynqmp interrupts when it write 0 to this register. But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE interrupt, so here need re-enable the interrupts.

> 
> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > @@ -541,6 +595,9 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
> >  	priv = mci->pvt_info;
> >  	p_data = priv->p_data;
> >
> > +	if (p_data->quirks & DDR_ECC_IMX8MP)
> > +		return intr_handler_imx8mp(irq, dev_id);
> > +
> >  	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
> >  	regval &= (DDR_QOSCE_MASK | DDR_QOSUE_MASK);
> >  	if (!(regval & ECC_CE_UE_INTR_MASK))
> 
> As this driver has struct synps_platform_data for some function calls (that
> are all the same today), you could add this as one that differs. This would let
> you pass the right one to devm_request_irq() at setup_irq() time. If there is a
> third type of intr_handler, it avoids chaining these quirk/features in the
> intr_handler().

Do you mean I should add a interrupt handler function in synps_platform_data, then use p_data->intr_handler in devm_request_irq() to pass on different interrupt handlers for ZynqMP and i.MX8MP?

> 
> 
> > @@ -817,7 +874,7 @@ static void mc_init(struct mem_ctl_info *mci, struct
> platform_device *pdev)
> >  	platform_set_drvdata(pdev, mci);
> >
> >  	/* Initialize controller capabilities and configuration */
> > -	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
> > +	mci->mtype_cap = MEM_FLAG_LRDDR4 | MEM_FLAG_DDR3 |
> MEM_FLAG_DDR2;
> >  	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
> >  	mci->scrub_cap = SCRUB_HW_SRC;
> >  	mci->scrub_mode = SCRUB_NONE;
> 
> You haven't updated zynq_get_mtype(), is it possible to use the new memory
> type?

I'm not sure if Zynq platform support LPDDR4.
And I don't think it's needed for all the three platform(Zynq/ZynqMP/i.MX8MP) to support the new memory type, mci->mtype_cap only represent the synopsys_edac driver can support this new type(for i.MX8MP part). What do you think?

> 
> 
> I think this would be cleaner if you moved the existing parts of the driver that
> aren't needed when using DDR_CE_INTR and DDR_UE_INTR to the platform
> data as a preparatory patch.
> You can then add support for these interrupts by adding the bits that are
> different.
> This should avoid things like trying to undo what zynq_get_error_info() did.
> Things like this are a maintenance headache.
> 
> As it is, you're hooking in the differences, then working round some of the
> things you didn't want to happen. (e.g. ECC_CLR_OFST being written as zero)
> 

The most part of zynqmp_get_error_info() and intr_handler() functions are same useful for i.MX8MP, so do you mean I should add new functions through platform data instead change the ZynqMP functions?

Best regards
Sherry Sun

> 
> 
> Thanks,
> 
> James