[PATCH V2 0/2] EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver

[PATCH V2 0/2] EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver

[Sherry Sun]

The two patches fix some issues for V3.X Synopsys EDAC DDR in synopsys_edac.c.
For the details, please check the patch commit log. This has been verified on
i.MX8MP platform.

Sherry Sun (2):
  EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR
  EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X
    Synopsys EDAC DDR

 drivers/edac/synopsys_edac.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR

[Sherry Sun]

V3.X Synopsys EDAC DDR doesn't have the QOS Interrupt register, need
to change to use the ECC Clear Register to disable the interrupts.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in V2:
1. Add the Reviewed-by and Acked-by tag.
---
 drivers/edac/synopsys_edac.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 40b1abeca856..88a481043d4c 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -865,8 +865,11 @@ static void enable_intr(struct synps_edac_priv *priv)
 static void disable_intr(struct synps_edac_priv *priv)
 {
 	/* Disable UE/CE Interrupts */
-	writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
-			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
+	if (priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR)
+		writel(0x0, priv->baseaddr + ECC_CLR_OFST);
+	else
+		writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
+		       priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
 }
 
 static int setup_irq(struct mem_ctl_info *mci,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR

[Borislav Petkov]

> Subject: Re: [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR

EDAC tree subject prefixes are of the format:

EDAC/<driver name>: Bla...

where "Bla starts with a capital letter".

What is "disable_intr support"?

I can figure it out when looking at the code but you need to make this
human-readable. For example

"Disable the error interrupt on Synopsys EDAC v3.x hardware"

Also, is it all caps V3.X or is it simply v3.x?

On Thu, Apr 21, 2022 at 09:53:12AM +0800, Sherry Sun wrote:
> V3.X Synopsys EDAC DDR doesn't have the QOS Interrupt register, need
> to change to use the ECC Clear Register to disable the interrupts.

Not "need to change" but "change".

From Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

Please have a look at that document.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR

[Sherry Sun]

Since zynqmp_get_error_info() is called during CE/UE interrupt, at the
end of zynqmp_get_error_info(), it wirtes 0 to ECC_CLR_OFST, which cause
the CE/UE interrupts of V3.X Synopsys EDAC DDR been disabled, then the
interrupt handler will be called only once, so need to re-enable the
interrupts at the end of intr_handler for V3.X Synopsys EDAC DDR.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in V2:
1. Add the Reviewed-by and Acked-by tag.
2. Add the newline as suggested by Michal.
---
 drivers/edac/synopsys_edac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 88a481043d4c..ae1cf02a92f5 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 	memset(p, 0, sizeof(*p));
 }
 
+static void enable_intr(struct synps_edac_priv *priv);
+
 /**
  * intr_handler - Interrupt Handler for ECC interrupts.
  * @irq:        IRQ number.
@@ -568,6 +570,9 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
 	/* v3.0 of the controller does not have this register */
 	if (!(priv->p_data->quirks & DDR_ECC_INTR_SELF_CLEAR))
 		writel(regval, priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
+	else
+		enable_intr(priv);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR

[Borislav Petkov]

On Thu, Apr 21, 2022 at 09:53:13AM +0800, Sherry Sun wrote:
> Since zynqmp_get_error_info() is called during CE/UE interrupt, at the

This also needs to be made human-readable: for example,
"zynqmp_get_error_info() reads the error information from the registers
when an interrupt for a {un-,}correctable error is raised."

> end of zynqmp_get_error_info(), it wirtes 0 to ECC_CLR_OFST, which cause

Unknown word [wirtes] in commit message.
Suggestions: ['writes',

Please introduce a spellchecker into your patch creation workflow.

> the CE/UE interrupts of V3.X Synopsys EDAC DDR been disabled, then the

"which disables the error interrupts" - make it simple - no need for the
V3.X marketing bla.

> interrupt handler will be called only once, so need to re-enable the

"Therefore, reenable the error interrupt line ..."

> interrupts at the end of intr_handler for V3.X Synopsys EDAC DDR.

I think you're catching my drift: our commit messages need to be
understandable and when read months, years from now, still to make
sense.

> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
> Changes in V2:
> 1. Add the Reviewed-by and Acked-by tag.
> 2. Add the newline as suggested by Michal.
> ---
>  drivers/edac/synopsys_edac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 88a481043d4c..ae1cf02a92f5 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
>  	memset(p, 0, sizeof(*p));
>  }
>  
> +static void enable_intr(struct synps_edac_priv *priv);

Why the forward declaration?

Why not simply move {enable,disable}_intr() upwards in that file?

Also, for both fixes: do you want them backported in stable kernels?

I think you do because they look like you'd want that v3.x support to
work with older kernels too.

If so, read the section about "Fixes:" in
Documentation/process/submitting-patches.rst

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR

[Sherry Sun]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: 2022年4月21日 17:07
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: michal.simek@xilinx.com; Shubhrajyoti.datta@xilinx.com;
> mchehab@kernel.org; tony.luck@intel.com; james.morse@arm.com;
> rric@kernel.org; linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in
> intr_handler for V3.X Synopsys EDAC DDR
> 
> On Thu, Apr 21, 2022 at 09:53:13AM +0800, Sherry Sun wrote:
> > Since zynqmp_get_error_info() is called during CE/UE interrupt, at the
> 
> This also needs to be made human-readable: for example,
> "zynqmp_get_error_info() reads the error information from the registers
> when an interrupt for a {un-,}correctable error is raised."
> 
> > end of zynqmp_get_error_info(), it wirtes 0 to ECC_CLR_OFST, which
> > cause
> 
> Unknown word [wirtes] in commit message.
> Suggestions: ['writes',
> 
> Please introduce a spellchecker into your patch creation workflow.
> 
> > the CE/UE interrupts of V3.X Synopsys EDAC DDR been disabled, then the
> 
> "which disables the error interrupts" - make it simple - no need for the V3.X
> marketing bla.
> 
> > interrupt handler will be called only once, so need to re-enable the
> 
> "Therefore, reenable the error interrupt line ..."
> 
> > interrupts at the end of intr_handler for V3.X Synopsys EDAC DDR.
> 
> I think you're catching my drift: our commit messages need to be
> understandable and when read months, years from now, still to make sense.

Hi Borislav, thanks for the detailed suggestions on this patchset, will improve them in V3.

> 
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Reviewed-by: Shubhrajyoti Datta <Shubhrajyoti.datta@xilinx.com>
> > Acked-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> > Changes in V2:
> > 1. Add the Reviewed-by and Acked-by tag.
> > 2. Add the newline as suggested by Michal.
> > ---
> >  drivers/edac/synopsys_edac.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/edac/synopsys_edac.c
> > b/drivers/edac/synopsys_edac.c index 88a481043d4c..ae1cf02a92f5
> 100644
> > --- a/drivers/edac/synopsys_edac.c
> > +++ b/drivers/edac/synopsys_edac.c
> > @@ -527,6 +527,8 @@ static void handle_error(struct mem_ctl_info *mci,
> struct synps_ecc_status *p)
> >  	memset(p, 0, sizeof(*p));
> >  }
> >
> > +static void enable_intr(struct synps_edac_priv *priv);
> 
> Why the forward declaration?
> 
> Why not simply move {enable,disable}_intr() upwards in that file?

I wanted minimal code changes here, but if you think it's better to simply move {enable,disable}_intr() upwards, I will do that way in V3.

> 
> Also, for both fixes: do you want them backported in stable kernels?
> 
> I think you do because they look like you'd want that v3.x support to work
> with older kernels too.
> 
> If so, read the section about "Fixes:" in Documentation/process/submitting-
> patches.rst

My fix patches are based on Dinh's patch: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"), as this patch was introduced since L5.17, it's quite new, so I think we don't need to backport them to the stable kernels. Thanks~

Best regards
Sherry

> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeopl
> e.kernel.org%2Ftglx%2Fnotes-about-
> netiquette&amp;data=05%7C01%7Csherry.sun%40nxp.com%7C91d3a08c4e0
> d43aeac9108da23764aec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637861288219637795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&amp;sdata=wnCvrZJ2%2FZKd%2Ff2X5xptEg7zfDqy5sLmyDCq8Rh3QB
> M%3D&amp;reserved=0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR

[Borislav Petkov]

On Sat, Apr 23, 2022 at 07:03:45AM +0000, Sherry Sun wrote:
> My fix patches are based on Dinh's patch: f7824ded4149
> ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"),
> as this patch was introduced since L5.17, it's quite new, so I think
> we don't need to backport them to the stable kernels.

What do you mean, quite new?

There are 5.17.x stable releases so they will need those fixes too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR

[Sherry Sun]

> On Sat, Apr 23, 2022 at 07:03:45AM +0000, Sherry Sun wrote:
> > My fix patches are based on Dinh's patch: f7824ded4149
> > ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR"),
> > as this patch was introduced since L5.17, it's quite new, so I think
> > we don't need to backport them to the stable kernels.
> 
> What do you mean, quite new?
> 
> There are 5.17.x stable releases so they will need those fixes too.

Hi Borislav, got it, so we can add the fix tag for the two patches. I will send V4.
Fixes: f7824ded4149 ("EDAC/synopsys: Add support for version 3 of the Synopsys EDAC DDR")

Best regards
Sherry

> 
> --
> Regards/Gruss,
>     Boris.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel