diff mbox series

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

Message ID 1582267156-20189-4-git-send-email-sherry.sun@nxp.com (mailing list archive)
State New, archived
Headers show
Series Add edac driver for i.MX8MP based on synopsys edac driver | expand

Commit Message

Sherry Sun Feb. 21, 2020, 6:39 a.m. UTC 
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.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/edac/synopsys_edac.c | 77 +++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

Comments

James Morse Feb. 26, 2020, 5:12 p.m. UTC | #1 
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?

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.


> 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?


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!


> +/* 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.


> +	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()...


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


> +
> +	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().


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



Thanks,

James
Sherry Sun Feb. 27, 2020, 5 a.m. UTC | #2 
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
diff mbox series

Patch

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
@@ -101,6 +101,7 @@ 
 /* DDR ECC Quirks */
 #define DDR_ECC_INTR_SUPPORT		BIT(0)
 #define DDR_ECC_DATA_POISON_SUPPORT	BIT(1)
+#define DDR_ECC_IMX8MP			BIT(2)
 
 /* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
 /* ECC Configuration Registers */
@@ -266,6 +267,11 @@ 
 
 #define RANK_B0_BASE			6
 
+/* ECCCTL UE/CE Interrupt enable/disable for IMX8MP*/
+#define DDR_CE_INTR_EN_MASK			0x100
+#define DDR_UE_INTR_EN_MASK			0x200
+#define ECC_INTR_MASK				0x10100
+
 /**
  * struct ecc_error_info - ECC error log information.
  * @row:	Row number.
@@ -524,6 +530,54 @@  static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 	memset(p, 0, sizeof(*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);
+}
+
+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);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * intr_handler - Interrupt Handler for ECC interrupts.
  * @irq:        IRQ number.
@@ -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))
@@ -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;
@@ -842,6 +899,9 @@  static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 static void enable_intr(struct synps_edac_priv *priv)
 {
 	/* Enable UE/CE Interrupts */
+	if (priv->p_data->quirks & DDR_ECC_IMX8MP)
+		return enable_intr_imx8mp(priv);
+
 	writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
 }
@@ -849,6 +909,9 @@  static void enable_intr(struct synps_edac_priv *priv)
 static void disable_intr(struct synps_edac_priv *priv)
 {
 	/* Disable UE/CE Interrupts */
+	if (priv->p_data->quirks & DDR_ECC_IMX8MP)
+		return disable_intr_imx8mp(priv);
+
 	writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
 }
@@ -898,6 +961,14 @@  static const struct synps_platform_data zynqmp_edac_def = {
 			  ),
 };
 
+static const struct synps_platform_data imx8mp_edac_def = {
+	.get_error_info	= zynqmp_get_error_info,
+	.get_mtype	= zynqmp_get_mtype,
+	.get_dtype	= zynqmp_get_dtype,
+	.get_ecc_state	= zynqmp_get_ecc_state,
+	.quirks         = (DDR_ECC_INTR_SUPPORT | DDR_ECC_IMX8MP),
+};
+
 static const struct of_device_id synps_edac_match[] = {
 	{
 		.compatible = "xlnx,zynq-ddrc-a05",
@@ -907,6 +978,10 @@  static const struct of_device_id synps_edac_match[] = {
 		.compatible = "xlnx,zynqmp-ddrc-2.40a",
 		.data = (void *)&zynqmp_edac_def
 	},
+	{
+		.compatible = "fsl,imx8mp-ddrc",
+		.data = (void *)&imx8mp_edac_def
+	},
 	{
 		/* end of table */
 	}