* [PATCH V2 0/2] EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver
@ 2022-04-21 1:53 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 0/2] EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver
@ 2022-04-21 1:53 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR
2022-04-21 1:53 ` Sherry Sun
@ 2022-04-21 1:53 ` Sherry Sun
-1 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR
@ 2022-04-21 1:53 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
2022-04-21 1:53 ` Sherry Sun
@ 2022-04-21 1:53 ` Sherry Sun
-1 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
@ 2022-04-21 1:53 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-21 1:53 UTC (permalink / raw)
To: michal.simek, bp, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric
Cc: linux-edac, linux-kernel, linux-arm-kernel, linux-imx
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR
2022-04-21 1:53 ` Sherry Sun
@ 2022-04-21 8:58 ` Borislav Petkov
-1 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-21 8:58 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
linux-imx
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR
@ 2022-04-21 8:58 ` Borislav Petkov
0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-21 8:58 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
linux-imx
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
2022-04-21 1:53 ` Sherry Sun
@ 2022-04-21 9:06 ` Borislav Petkov
-1 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-21 9:06 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
@ 2022-04-21 9:06 ` Borislav Petkov
0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-21 9:06 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
2022-04-21 9:06 ` Borislav Petkov
@ 2022-04-23 7:03 ` Sherry Sun
-1 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-23 7:03 UTC (permalink / raw)
To: Borislav Petkov
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
> -----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&data=05%7C01%7Csherry.sun%40nxp.com%7C91d3a08c4e0
> d43aeac9108da23764aec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637861288219637795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=wnCvrZJ2%2FZKd%2Ff2X5xptEg7zfDqy5sLmyDCq8Rh3QB
> M%3D&reserved=0
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
@ 2022-04-23 7:03 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-23 7:03 UTC (permalink / raw)
To: Borislav Petkov
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
> -----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&data=05%7C01%7Csherry.sun%40nxp.com%7C91d3a08c4e0
> d43aeac9108da23764aec%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637861288219637795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=wnCvrZJ2%2FZKd%2Ff2X5xptEg7zfDqy5sLmyDCq8Rh3QB
> M%3D&reserved=0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
2022-04-23 7:03 ` Sherry Sun
@ 2022-04-26 14:58 ` Borislav Petkov
-1 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-26 14:58 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
@ 2022-04-26 14:58 ` Borislav Petkov
0 siblings, 0 replies; 16+ messages in thread
From: Borislav Petkov @ 2022-04-26 14:58 UTC (permalink / raw)
To: Sherry Sun
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
2022-04-26 14:58 ` Borislav Petkov
@ 2022-04-27 1:40 ` Sherry Sun
-1 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-27 1:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
> 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.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler for V3.X Synopsys EDAC DDR
@ 2022-04-27 1:40 ` Sherry Sun
0 siblings, 0 replies; 16+ messages in thread
From: Sherry Sun @ 2022-04-27 1:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: michal.simek, Shubhrajyoti.datta, mchehab, tony.luck,
james.morse, rric, linux-edac, linux-kernel, linux-arm-kernel,
dl-linux-imx
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-04-27 1:42 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 1:53 [PATCH V2 0/2] EDAC: synopsys: fix some bugs in V3.X Synopsys EDAC DDR driver Sherry Sun
2022-04-21 1:53 ` Sherry Sun
2022-04-21 1:53 ` [PATCH V2 1/2] EDAC: synopsys: Add disable_intr support for V3.X Synopsys EDAC DDR Sherry Sun
2022-04-21 1:53 ` Sherry Sun
2022-04-21 8:58 ` Borislav Petkov
2022-04-21 8:58 ` Borislav Petkov
2022-04-21 1:53 ` [PATCH V2 2/2] EDAC: synopsys: re-enable the interrupts in intr_handler " Sherry Sun
2022-04-21 1:53 ` Sherry Sun
2022-04-21 9:06 ` Borislav Petkov
2022-04-21 9:06 ` Borislav Petkov
2022-04-23 7:03 ` Sherry Sun
2022-04-23 7:03 ` Sherry Sun
2022-04-26 14:58 ` Borislav Petkov
2022-04-26 14:58 ` Borislav Petkov
2022-04-27 1:40 ` Sherry Sun
2022-04-27 1:40 ` Sherry Sun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.