driver/edac/fsl_ddr: Add support of little endian

Two symbols are missing if mpc85xx_edac driver is compiled as module.

Signed-off-by York Sun <[email protected]>

Quoted 3 lines · York Sun

Two symbols are missing if mpc85xx_edac driver is compiled as module.

Signed-off-by York Sun <[email protected]>

Good catch! One comment below.

Reviewed-by Andrew Donnellan <[email protected]>

arch/powerpc/kernel/pci-common.c int early_find_capability(struct pci_controller *hose, int bus, int devfn, · line 1589

Quoted 7 lines

/*
* Reads the interrupt pin to determine if interrupt is use by card.

arch/powerpc/kernel/pci-common.c +1 -0

@@ -1585,6 +1586,7 @@ int early_find_capability(struct pci_controller *hose, int bus, int devfn,
 {
 	return pci_bus_find_capability(fake_pci_bus(hose, bus), devfn, cap);
 }
+EXPORT_SYMBOL(early_find_capability);

It'd be nicer for this to be renamed as "pci_early_find_capability" or something like that with a "namespace", I think.

I will rename it if I respin this patch for any reason. Otherwise, I will send out another patch to rename it after merging.

Two symbols are missing if mpc85xx_edac driver is compiled as module.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: Change subject tag
v2: no change

 arch/powerpc/kernel/pci-common.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/kernel/pci-common.c +1 -0

index 0f7a60f..86bc484 100644
@@ -226,6 +226,7 @@ struct pci_controller* pci_find_hose_for_OF_device(struct device_node* node)
 	}
 	return NULL;
 }
+EXPORT_SYMBOL(pci_find_hose_for_OF_device);
 
 /*
  * Reads the interrupt pin to determine if interrupt is use by card.

York Sun <[email protected]> writes:=0A=
=0A=

arch/powerpc/kernel/pci-=

Quoted 13 lines

Two symbols are missing if mpc85xx_edac driver is compiled as module.=0A= =0A=

Signed-off-by York Sun <[email protected]>=0A=

=0A=
---=0A=
Change log=0A=
v3: Change subject tag=0A=
v2: no change=0A=
=0A=
arch/powerpc/kernel/pci-common.c | 2 ++=0A=

 1 file changed, 2 insertions(+)=0A=

=0A=

arch/powerpc/kernel/pci-= renamed from arch/powerpc/kernel/pci-common.c +0 -0

arch/powerpc/kernel/pci-common.c=0A= struct pci_controller* pci_find_hose_for_OF_device(s=

Quoted 4 lines

index 0f7a60f..86bc484 100644=0A=

arch/powerpc/kernel/pci-common.c=0A= +0 -0

@@ -226,6 +226,7 @@ struct pci_controller* pci_find_hose_for_OF_device(s=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h=

Quoted 8 lines

arch/powerpc/kernel/pci-common.c=0A= +1 -0

 	}=0A=
 	return NULL;=0A=
 }=0A=
+EXPORT_SYMBOL(pci_find_hose_for_OF_device);=0A=

=0A=
/*=0A=
* Reads the interrupt pin to determine if interrupt is use by card.=0A=

arch/powerpc/kernel/pci-common.c=0A= +0 -0

@@ -1585,6 +1586,7 @@ int early_find_capability(struct pci_controller *h=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h= · line 1587

Quoted 2 lines

arch/powerpc/kernel/pci-common.c=0A= +0 -0

 {=0A=
 	return pci_bus_find_capability(fake_pci_bus(hose, bus), devfn, cap);=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h= · line 1589

Quoted 2 lines

arch/powerpc/kernel/pci-common.c=0A= +1 -0

 }=0A=
+EXPORT_SYMBOL(early_find_capability);=0A=

=0A=
Does the driver really need to use these routines? They're meant for use=

early in boot, before PCI is setup.=0A=
=0A=
AFAICS this is just a regular driver, so when it's probed the PCI=0A= devices should have already been scanned. In which case pci_get_device()=

could work couldn't it? (I see other edac drivers doing that).=0A= =0A=

I don't have deep knowledge of this driver. What I am trying is to separate the common DDR part and share it with ARM platforms. Along the way, I found the compiling error if build a module. If exposing these functions becomes a concern, I can live without it.

Quoted 4 lines · york sun

I don't have deep knowledge of this driver. What I am trying is to separate the common DDR part and share it with ARM platforms. Along the way, I found the compiling error if build a module. If exposing these functions becomes a concern, I can live without it.

Perhaps you or Johannes could fix this properly to use pci_get_device() as the rest of the EDAC drivers do, instead of exporting core PCI functions...

On Fri, Aug 05, 2016 at 04:26:26AM +0000, york sun wrote:=0A=

Quoted 2 lines

I don't have deep knowledge of this driver. What I am trying is to=0A= separate the common DDR part and share it with ARM platforms. Along the=

Quoted 2 lines

way, I found the compiling error if build a module. If exposing these=0A= functions becomes a concern, I can live without it.=0A=

=0A=
Perhaps you or Johannes could fix this properly to use pci_get_device()=

as the rest of the EDAC drivers do, instead of exporting core PCI=0A= functions...=0A=
=0A=

York Sun <[email protected]> writes:=0A=
=0A=

arch/powerpc/kernel/pci-=

Quoted 13 lines

Two symbols are missing if mpc85xx_edac driver is compiled as module.=0A= =0A=

Signed-off-by York Sun <[email protected]>=0A=

=0A=
---=0A=
Change log=0A=
v3: Change subject tag=0A=
v2: no change=0A=
=0A=
arch/powerpc/kernel/pci-common.c | 2 ++=0A=

 1 file changed, 2 insertions(+)=0A=

=0A=

arch/powerpc/kernel/pci-= renamed from arch/powerpc/kernel/pci-common.c +0 -0

arch/powerpc/kernel/pci-common.c=0A= struct pci_controller* pci_find_hose_for_OF_device(s=

Quoted 4 lines

index 0f7a60f..86bc484 100644=0A=

arch/powerpc/kernel/pci-common.c=0A= +0 -0

@@ -226,6 +226,7 @@ struct pci_controller* pci_find_hose_for_OF_device(s=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h=

Quoted 8 lines

arch/powerpc/kernel/pci-common.c=0A= +1 -0

 	}=0A=
 	return NULL;=0A=
 }=0A=
+EXPORT_SYMBOL(pci_find_hose_for_OF_device);=0A=

=0A=
/*=0A=
* Reads the interrupt pin to determine if interrupt is use by card.=0A=

arch/powerpc/kernel/pci-common.c=0A= +0 -0

@@ -1585,6 +1586,7 @@ int early_find_capability(struct pci_controller *h=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h= · line 1587

Quoted 2 lines

arch/powerpc/kernel/pci-common.c=0A= +0 -0

 {=0A=
 	return pci_bus_find_capability(fake_pci_bus(hose, bus), devfn, cap);=

arch/powerpc/kernel/pci-common.c=0A= int early_find_capability(struct pci_controller *h= · line 1589

Quoted 2 lines

arch/powerpc/kernel/pci-common.c=0A= +1 -0

 }=0A=
+EXPORT_SYMBOL(early_find_capability);=0A=

=0A=
Does the driver really need to use these routines? They're meant for use=

early in boot, before PCI is setup.=0A=
=0A=
AFAICS this is just a regular driver, so when it's probed the PCI=0A= devices should have already been scanned. In which case pci_get_device()=

could work couldn't it? (I see other edac drivers doing that).=0A=

Quoted 4 lines · Michael Ellerman

Does the driver really need to use these routines? They're meant for use early in boot, before PCI is setup.

Quoted 3 lines

AFAICS this is just a regular driver, so when it's probed the PCI devices should have already been scanned. In which case pci_get_device() could work couldn't it? (I see other edac drivers doing that).

I am trying to fix this but need some help. We are dealing with PCIe controller here. Does it have a bus number assigned at this point? If yes, how can I find it? I seem not able to find out where the platform_data is filled as well. Can someone kindly point it out to me?

On Fri, 2016-08-05 at 20:29 +0000, york sun wrote:=0A=

Quoted 3 lines

On 08/04/2016 08:43 PM, Michael Ellerman wrote:=0A=

Quoted 2 lines

=0A=
Does the driver really need to use these routines? They're meant for us=

Quoted 4 lines

Quoted 4 lines

early in boot, before PCI is setup.=0A=
=0A=
AFAICS this is just a regular driver, so when it's probed the PCI=0A= devices should have already been scanned. In which case pci_get_device(=

Quoted 5 lines

Quoted 1 line

could work couldn't it? (I see other edac drivers doing that).=0A=

I am trying to fix this but need some help. We are dealing with PCIe=0A= controller here. Does it have a bus number assigned at this point? If=0A= yes, how can I find it? I seem not able to find out where the=0A= platform_data is filled as well. Can someone kindly point it out to me?=

=0A=
=0A=
The platform data comes from add_err_dev() in arch/powerpc/sysdev/fsl_pci=

=0A=

Quoted 19 lines · Scott Wood

Quoted 16 lines · york sun

Quoted 10 lines · Michael Ellerman

Does the driver really need to use these routines? They're meant for use
early in boot, before PCI is setup.

AFAICS this is just a regular driver, so when it's probed the PCI devices should have already been scanned. In which case pci_get_device()
could work couldn't it? (I see other edac drivers doing that).

I am trying to fix this but need some help. We are dealing with PCIe controller here. Does it have a bus number assigned at this point? If yes, how can I find it? I seem not able to find out where the platform_data is filled as well. Can someone kindly point it out to me?

Quoted 3 lines

The platform data comes from add_err_dev() in
arch/powerpc/sysdev/fsl_pci.c.

Thanks, Scott.

When add_err_dev() is called, pci is not scanned, is using early_find_capability() justified?

On e500v1, read fault exception enable (RFXE) controls whether assertion of core_fault_in causes a machine check interrupt. Assertion of core_fault_in can result from uncorrectable data error, such as an L2 multibit ECC error. It can also occur from a system error if logic on the integrated device signals a fault for nonfatal errors. RFXE bit is cleared out of reset, and should be left clear for normal operation. Assertion of core_fault_in does not cause a machine check.

RFXE is set specifically for RIO (Rapid IO) and PCI for book E to catch the errors by machine check. With this bit set, EDAC driver can't get the interrupt in case of uncorrectable error. So this bit is cleared in favor of EDAC. However, the benefit of catching such uncorrectable error doesn't outweight the other errors which may hang the system. Beside, e500v2 has different errors maksed by RFXE, and e500mc doesn't support this bit. It is more reasonable to leave RFXE as is in EDAC driver, and leave the uncorrectable errors triggering machine check for e500v1.

Quoted 9 lines · York Sun

On e500v1, read fault exception enable (RFXE) controls whether assertion of core_fault_in causes a machine check interrupt. Assertion of core_fault_in can result from uncorrectable data error, such as an L2 multibit ECC error. It can also occur from a system error if logic on the integrated device signals a fault for nonfatal errors. RFXE bit is cleared out of reset, and should be left clear for normal operation. Assertion of core_fault_in does not cause a machine check.

Quoted 9 lines

RFXE is set specifically for RIO (Rapid IO) and PCI for book E to catch the errors by machine check. With this bit set, EDAC driver can't get the interrupt in case of uncorrectable error. So this bit is cleared in favor of EDAC. However, the benefit of catching such uncorrectable error doesn't outweight the other errors which may hang the system. Beside, e500v2 has different errors maksed by RFXE, and e500mc doesn't support this bit. It is more reasonable to leave RFXE as is in EDAC driver, and leave the uncorrectable errors triggering machine check for e500v1.

Very nice, thanks for expanding it!

Two final remarks:

- please use a spell checker

- now, what happens if you leave RFXE clear and mpc85xx_edac gets the error? Is it going to do proper error handling of the uncorrectable error or are we better off handling the error in the #MC interrupt handler?

IOW, is mpc85xx_edac well equipped to handle those multibit errors or should we leave the current setting as is?

RFXE is cleared by default. So for most SoCs, this is not even a concern at all. But for e500v1, when RIO or PCI are used, this bit is set specifically to catch an error by machine check (see commit 4e0e3435). This is not the uncorrectable error from DDR. We will be better off to let this error happen.

Quoted 5 lines · york sun

RFXE is cleared by default. So for most SoCs, this is not even a concern at all. But for e500v1, when RIO or PCI are used, this bit is set specifically to catch an error by machine check (see commit 4e0e3435). This is not the uncorrectable error from DDR. We will be better off to let this error happen.

So I'm reading this: "With this bit set, EDAC driver can't get the interrupt in case of uncorrectable error. So this bit is cleared in favor of EDAC."

AFAIU, it means, RFXE bit remains clear so EDAC will get the interrupt for the uncorrectable error (UE). So on those !e500v1 systems, EDAC be handling those UEs.

Am I close?

If so, can EDAC handle the UE?

Yes, for most SoCs RFXE remains cleared. Uncorrectable errors are handled by EDAC.

Quoted 2 lines · york sun

Yes, for most SoCs RFXE remains cleared. Uncorrectable errors are handled by EDAC.

And how is mpc85_xxx EDAC handling them?

mpc85xx_mc_check() only reports them.

And now to get to my original question: is it *enough* to report uncorrectable errors on those platforms or do they need more sophisticated error handling in order to disable data corruption?

It is uncorrectable. DDR controller can only report the error. I don't believe EDAC driver can do more. For the same reason I said we can leave RXFE as is, even for e500v1 case (with RIO or PCI is enabled). Nothing can be done with uncorrectable error.

Quoted 10 lines · Borislav Petkov

Quoted 4 lines · york sun

It is uncorrectable. DDR controller can only report the error. I don't believe EDAC driver can do more. For the same reason I said we can leave RXFE as is, even for e500v1 case (with RIO or PCI is enabled). Nothing can be done with uncorrectable error.

Of course it can: it can panic the machine so that it doesn't corrupt data on secondary storage. You might consider whether this is a better course of action instead of only reporting the error.

Boris,

My opinion is the error shouldn't happen at the first place. It usually means wrong configuration or physical error on the hardware. An EDAC driver can report these errors. How far you want to go on the error handling is up for discussion. I don't think we should include those improvement in this patch set.

I want to add this, normally uncorrectable errors don't trigger machine check on e500v1. RXFE controls different interrupt on e500v2. e500mc doesn't support RXFE. Together with the reason I explained, I believe EDAC driver shouldn't change RXFE.

I hope I made it clear.

Quoted 4 lines · york sun

It is uncorrectable. DDR controller can only report the error. I don't believe EDAC driver can do more. For the same reason I said we can leave RXFE as is, even for e500v1 case (with RIO or PCI is enabled). Nothing can be done with uncorrectable error.

Of course it can: it can panic the machine so that it doesn't corrupt data on secondary storage. You might consider whether this is a better course of action instead of only reporting the error.

The mpc85xx compatible DDR controllers are used on ARM-based SoCs. Separate the DDR part from mpc85xx EDAC driver and prepare to support both architecture.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: Fix compiling errors and warnings caused by patch ordering
v2: Reordered patch
Separate FSL DDR commont code as shared object, not another driver This patch is generated with "git format-patch -M40 -C40" to show copy-and-delete.

 drivers/edac/Makefile                           |   5 +-
 drivers/edac/{mpc85xx_edac.c => fsl_ddr_edac.c} | 699 +-----------------------
 drivers/edac/{mpc85xx_edac.h => fsl_ddr_edac.h} | 106 +---
 drivers/edac/mpc85xx_edac.c                     | 559 +------------------
 drivers/edac/mpc85xx_edac.h                     |  66 ---
 5 files changed, 22 insertions(+), 1413 deletions(-)
 copy drivers/edac/{mpc85xx_edac.c => fsl_ddr_edac.c} (43%)
 copy drivers/edac/{mpc85xx_edac.h => fsl_ddr_edac.h} (43%)

drivers/edac/Makefile +4 -1

index f9e4a3e..ee047a4 100644
@@ -50,7 +50,10 @@ amd64_edac_mod-$(CONFIG_EDAC_AMD64_ERROR_INJECTION) += amd64_edac_inj.o
 obj-$(CONFIG_EDAC_AMD64)		+= amd64_edac_mod.o
 
 obj-$(CONFIG_EDAC_PASEMI)		+= pasemi_edac.o
-obj-$(CONFIG_EDAC_MPC85XX)		+= mpc85xx_edac.o
+
+mpc85xx_edac_mod-y			:= fsl_ddr_edac.o mpc85xx_edac.o
+obj-$(CONFIG_EDAC_MPC85XX)		+= mpc85xx_edac_mod.o
+
 obj-$(CONFIG_EDAC_MV64X60)		+= mv64x60_edac.o
 obj-$(CONFIG_EDAC_CELL)			+= cell_edac.o
 obj-$(CONFIG_EDAC_PPC4XX)		+= ppc4xx_edac.o

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +0 -0

similarity index 43%
copy from drivers/edac/mpc85xx_edac.c
copy to drivers/edac/fsl_ddr_edac.c
index c0b0951..280797e 100644

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +0 -0

  *
  * Parts Copyrighted (c) 2013 by Freescale Semiconductor, Inc.
  *

@@ -20,33 +20,20 @@
 #include <linux/edac.h>
 #include <linux/smp.h>
 #include <linux/gfp.h>

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +0 -14

-static int edac_dev_idx;
-#ifdef CONFIG_PCI
-static int edac_pci_idx;
-#endif
 static int edac_mc_idx;
 
 static u32 orig_ddr_err_disable;
 static u32 orig_ddr_err_sbe;
 
-/*
- * PCI Err defines
- */
-#ifdef CONFIG_PCI
-static u32 orig_pci_err_cap_dr;
-static u32 orig_pci_err_en;
-#endif
-
-static u32 orig_l2_err_disable;
-
 /************************ MC SYSFS parts ***********************************/
 
 #define to_mci(k) container_of(k, struct mem_ctl_info, dev)

Quoted 47 lines · York Sun

The mpc85xx compatible DDR controllers are used on ARM-based SoCs. Separate the DDR part from mpc85xx EDAC driver and prepare to support both architecture.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: Fix compiling errors and warnings caused by patch ordering
v2: Reordered patch
Separate FSL DDR commont code as shared object, not another driver This patch is generated with "git format-patch -M40 -C40" to show copy-and-delete.

 drivers/edac/Makefile                           |   5 +-
 drivers/edac/{mpc85xx_edac.c => fsl_ddr_edac.c} | 699 +-----------------------
 drivers/edac/{mpc85xx_edac.h => fsl_ddr_edac.h} | 106 +---
 drivers/edac/mpc85xx_edac.c                     | 559 +------------------
 drivers/edac/mpc85xx_edac.h                     |  66 ---
 5 files changed, 22 insertions(+), 1413 deletions(-)
 copy drivers/edac/{mpc85xx_edac.c => fsl_ddr_edac.c} (43%)
 copy drivers/edac/{mpc85xx_edac.h => fsl_ddr_edac.h} (43%)

drivers/edac/Makefile +4 -1

index f9e4a3e..ee047a4 100644
@@ -50,7 +50,10 @@ amd64_edac_mod-$(CONFIG_EDAC_AMD64_ERROR_INJECTION) += amd64_edac_inj.o
 obj-$(CONFIG_EDAC_AMD64)		+= amd64_edac_mod.o
 
 obj-$(CONFIG_EDAC_PASEMI)		+= pasemi_edac.o
-obj-$(CONFIG_EDAC_MPC85XX)		+= mpc85xx_edac.o
+
+mpc85xx_edac_mod-y			:= fsl_ddr_edac.o mpc85xx_edac.o
+obj-$(CONFIG_EDAC_MPC85XX)		+= mpc85xx_edac_mod.o
+
 obj-$(CONFIG_EDAC_MV64X60)		+= mv64x60_edac.o
 obj-$(CONFIG_EDAC_CELL)			+= cell_edac.o
 obj-$(CONFIG_EDAC_PPC4XX)		+= ppc4xx_edac.o

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +0 -0

similarity index 43%
copy from drivers/edac/mpc85xx_edac.c
copy to drivers/edac/fsl_ddr_edac.c
index c0b0951..280797e 100644

drivers/edac/fsl_ddr_edac.c line 2

Quoted 3 lines

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +1 -1

@@ -1,5 +1,5 @@
 /*
- * Freescale MPC85xx Memory Controller kernel module
+ * Freescale Memory Controller kernel modul

This should say something like "Layerscape SoC blabla... Originally split out from mpc85xx_edac EDAC driver."

Ditto for the header below.

Quoted 10 lines

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +0 -1

  *
  * Parts Copyrighted (c) 2013 by Freescale Semiconductor, Inc.
  *

@@ -20,33 +20,20 @@
 #include <linux/edac.h>
 #include <linux/smp.h>
 #include <linux/gfp.h>
-#include <linux/fsl/edac.h>
 

drivers/edac/fsl_ddr_edac.c line 30

Quoted 8 lines

drivers/edac/fsl_ddr_edac.c renamed from drivers/edac/mpc85xx_edac.c +3 -1

 #include <linux/of_platform.h>
 #include <linux/of_device.h>
 #include "edac_module.h"
 #include "edac_core.h"
-#include "mpc85xx_edac.h"
+#include "fsl_ddr_edac.h"
+
+#define EDAC_MOD_STR	"FSL_DDR_EDAC"

Lowercase please.

Use generic names for macros, variables and functions.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: Absort changes from previous patch after reording
v2: Separated from "House cleaning" patch of v1

 drivers/edac/fsl_ddr_edac.c | 153 ++++++++++++++++++++++----------------------
 drivers/edac/fsl_ddr_edac.h |  43 ++++++-------
 drivers/edac/mpc85xx_edac.c |   4 +-
 3 files changed, 98 insertions(+), 102 deletions(-)

drivers/edac/fsl_ddr_edac.h +7 -20

index 94c6907..556bac5 100644
@@ -12,7 +12,7 @@
 #ifndef _FSL_DDR_EDAC_H_
 #define _FSL_DDR_EDAC_H_
 
-#define mpc85xx_mc_printk(mci, level, fmt, arg...) \
+#define fsl_ddr_mc_printk(mci, level, fmt, arg...) \
 	edac_mc_chipset_printk(mci, level, "FSL_DDR", fmt, ##arg)
 
 /*

@@ -20,26 +20,23 @@
  */
 
 /* DDR_SDRAM_CFG */
-#define MPC85XX_MC_DDR_SDRAM_CFG	0x0110
-#define MPC85XX_MC_CS_BNDS_0		0x0000
-#define MPC85XX_MC_CS_BNDS_1		0x0008
-#define MPC85XX_MC_CS_BNDS_2		0x0010
-#define MPC85XX_MC_CS_BNDS_3		0x0018
-#define MPC85XX_MC_CS_BNDS_OFS		0x0008
+#define MC_DDR_SDRAM_CFG	0x0110
+#define MC_CS_BNDS_0		0x0000
+#define MC_CS_BNDS_OFS		0x0008
 
-#define MPC85XX_MC_DATA_ERR_INJECT_HI	0x0e00
-#define MPC85XX_MC_DATA_ERR_INJECT_LO	0x0e04
-#define MPC85XX_MC_ECC_ERR_INJECT	0x0e08
-#define MPC85XX_MC_CAPTURE_DATA_HI	0x0e20
-#define MPC85XX_MC_CAPTURE_DATA_LO	0x0e24
-#define MPC85XX_MC_CAPTURE_ECC		0x0e28
-#define MPC85XX_MC_ERR_DETECT		0x0e40
-#define MPC85XX_MC_ERR_DISABLE		0x0e44
-#define MPC85XX_MC_ERR_INT_EN		0x0e48
-#define MPC85XX_MC_CAPTURE_ATRIBUTES	0x0e4c
-#define MPC85XX_MC_CAPTURE_ADDRESS	0x0e50
-#define MPC85XX_MC_CAPTURE_EXT_ADDRESS	0x0e54
-#define MPC85XX_MC_ERR_SBE		0x0e58
+#define MC_DATA_ERR_INJECT_HI	0x0e00
+#define MC_DATA_ERR_INJECT_LO	0x0e04
+#define MC_ECC_ERR_INJECT	0x0e08

Quoted 13 lines · York Sun

Use generic names for macros, variables and functions.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: Absort changes from previous patch after reording
v2: Separated from "House cleaning" patch of v1

 drivers/edac/fsl_ddr_edac.c | 153 ++++++++++++++++++++++----------------------
 drivers/edac/fsl_ddr_edac.h |  43 ++++++-------
 drivers/edac/mpc85xx_edac.c |   4 +-
 3 files changed, 98 insertions(+), 102 deletions(-)

...

Quoted 49 lines

drivers/edac/fsl_ddr_edac.h +13 -20

index 94c6907..556bac5 100644
@@ -12,7 +12,7 @@
 #ifndef _FSL_DDR_EDAC_H_
 #define _FSL_DDR_EDAC_H_
 
-#define mpc85xx_mc_printk(mci, level, fmt, arg...) \
+#define fsl_ddr_mc_printk(mci, level, fmt, arg...) \
 	edac_mc_chipset_printk(mci, level, "FSL_DDR", fmt, ##arg)
 
 /*

@@ -20,26 +20,23 @@
  */
 
 /* DDR_SDRAM_CFG */
-#define MPC85XX_MC_DDR_SDRAM_CFG	0x0110
-#define MPC85XX_MC_CS_BNDS_0		0x0000
-#define MPC85XX_MC_CS_BNDS_1		0x0008
-#define MPC85XX_MC_CS_BNDS_2		0x0010
-#define MPC85XX_MC_CS_BNDS_3		0x0018
-#define MPC85XX_MC_CS_BNDS_OFS		0x0008
+#define MC_DDR_SDRAM_CFG	0x0110
+#define MC_CS_BNDS_0		0x0000
+#define MC_CS_BNDS_OFS		0x0008
 
-#define MPC85XX_MC_DATA_ERR_INJECT_HI	0x0e00
-#define MPC85XX_MC_DATA_ERR_INJECT_LO	0x0e04
-#define MPC85XX_MC_ECC_ERR_INJECT	0x0e08
-#define MPC85XX_MC_CAPTURE_DATA_HI	0x0e20
-#define MPC85XX_MC_CAPTURE_DATA_LO	0x0e24
-#define MPC85XX_MC_CAPTURE_ECC		0x0e28
-#define MPC85XX_MC_ERR_DETECT		0x0e40
-#define MPC85XX_MC_ERR_DISABLE		0x0e44
-#define MPC85XX_MC_ERR_INT_EN		0x0e48
-#define MPC85XX_MC_CAPTURE_ATRIBUTES	0x0e4c
-#define MPC85XX_MC_CAPTURE_ADDRESS	0x0e50
-#define MPC85XX_MC_CAPTURE_EXT_ADDRESS	0x0e54
-#define MPC85XX_MC_ERR_SBE		0x0e58
+#define MC_DATA_ERR_INJECT_HI	0x0e00
+#define MC_DATA_ERR_INJECT_LO	0x0e04
+#define MC_ECC_ERR_INJECT	0x0e08
+#define MC_CAPTURE_DATA_HI	0x0e20
+#define MC_CAPTURE_DATA_LO	0x0e24
+#define MC_CAPTURE_ECC		0x0e28
+#define MC_ERR_DETECT		0x0e40
+#define MC_ERR_DISABLE		0x0e44
+#define MC_ERR_INT_EN		0x0e48

drivers/edac/fsl_ddr_edac.h line 39

Quoted 4 lines

drivers/edac/fsl_ddr_edac.h +4 -0

+#define MC_CAPTURE_ATRIBUTES	0x0e4c
+#define MC_CAPTURE_ADDRESS	0x0e50
+#define MC_CAPTURE_EXT_ADDRESS	0x0e54
+#define MC_ERR_SBE		0x0e58

You should choose one name prefix for all driver symbols and stick to it. Either "FSL" or "LSC" for "Layerscape" or whatever but please be consistent.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: no change

 drivers/edac/fsl_ddr_edac.c | 12 ++++++++++--
 drivers/edac/fsl_ddr_edac.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

drivers/edac/fsl_ddr_edac.c +0 -0

index 60761c0..88ecf7d 100644

drivers/edac/fsl_ddr_edac.c +6 -0

 			mtype = MEM_RDDR3;
 			break;
+		case DSC_SDTYPE_DDR4:
+			mtype = MEM_RDDR4;
+			break;
 		default:
 			mtype = MEM_UNKNOWN;
 			break;

@@ -391,6 +394,9 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		case DSC_SDTYPE_DDR3:
 			mtype = MEM_DDR3;
 			break;
+		case DSC_SDTYPE_DDR4:
+			mtype = MEM_DDR4;
+			break;
 		default:
 			mtype = MEM_UNKNOWN;
 			break;

drivers/edac/fsl_ddr_edac.c +1 -0

+			 MEM_FLAG_DDR4 | MEM_FLAG_RDDR4;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
 	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->mod_name = EDAC_MOD_STR;

drivers/edac/fsl_ddr_edac.h +1 -0

index 556bac5..c7b7dbf 100644
@@ -50,6 +50,7 @@
 #define DSC_SDTYPE_DDR		0x02000000
 #define DSC_SDTYPE_DDR2		0x03000000
 #define DSC_SDTYPE_DDR3		0x07000000
+#define DSC_SDTYPE_DDR4		0x05000000
 #define DSC_X32_EN	0x00000020
 
 /* Err_Int_En */

Signature · 1 line

2.7.4

On Thu, Aug 04, 2016 at 03:58:32PM -0700, York Sun wrote:

<--- Missing commit message.

Quoted 11 lines

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: no change

 drivers/edac/fsl_ddr_edac.c | 12 ++++++++++--
 drivers/edac/fsl_ddr_edac.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

drivers/edac/fsl_ddr_edac.c static void fsl_ddr_init_csrows(struct mem_ctl_info *mci) · line 376

Quoted 6 lines

drivers/edac/fsl_ddr_edac.c +0 -0

index 60761c0..88ecf7d 100644
@@ -376,6 +376,9 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		case DSC_SDTYPE_DDR3:

Btw, those DSC_SDTYPE_* defines are used only here to map to the MEM_* ones. You can just as well use the naked numbers here and drop the DSC_SDTYPE* ones as it is clear what the naked numbers mean based on how they're being used.

Quoted 22 lines

drivers/edac/fsl_ddr_edac.c +6 -0

 			mtype = MEM_RDDR3;
 			break;
+		case DSC_SDTYPE_DDR4:
+			mtype = MEM_RDDR4;
+			break;
 		default:
 			mtype = MEM_UNKNOWN;
 			break;

@@ -391,6 +394,9 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		case DSC_SDTYPE_DDR3:
 			mtype = MEM_DDR3;
 			break;
+		case DSC_SDTYPE_DDR4:
+			mtype = MEM_DDR4;
+			break;
 		default:
 			mtype = MEM_UNKNOWN;
 			break;

@@ -495,8 +501,10 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 	}
 

drivers/edac/fsl_ddr_edac.c int fsl_ddr_mc_err_probe(struct platform_device *op) · line 506

Quoted 6 lines

drivers/edac/fsl_ddr_edac.c +3 -2

 	edac_dbg(3, "init mci\n");
-	mci->mtype_cap = MEM_FLAG_RDDR | MEM_FLAG_RDDR2 |
-	    MEM_FLAG_DDR | MEM_FLAG_DDR2;
+	mci->mtype_cap = MEM_FLAG_DDR | MEM_FLAG_RDDR |
+			 MEM_FLAG_DDR2 | MEM_FLAG_RDDR2 |
+			 MEM_FLAG_DDR3 | MEM_FLAG_RDDR3 |

DDR3 is silently added too, you can talk about that in the commit message, for example.

Get endianness from device tree. Both big endian and little endian are supported. Default to big endian for backward compatibility to MPC85xx.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Separated from "Add support for ARM-based SoCs" patch

 .../fsl/ddr.txt}                                   |  6 ++
 drivers/edac/fsl_ddr_edac.c                        | 83 ++++++++++++++--------
 2 files changed, 58 insertions(+), 31 deletions(-)
 rename Documentation/devicetree/bindings/{powerpc/fsl/mem-ctrlr.txt => memory-controllers/fsl/ddr.txt} (74%)

Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt renamed from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt +5 -0

similarity index 74%
rename from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt
rename to Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt
index f87856f..62623f9 100644
@@ -7,6 +7,10 @@ Properties:
 		  "fsl,qoriq-memory-controller".
 - reg		: Address and size of DDR controller registers
 - interrupts	: Error interrupt of DDR controller
+- little-endian	: Specifies little-endian access to registers
+- big-endian	: Specifies big-endian access to registers
+
+If both little-endian and big-endian are omitted, big-endian will be used.
 
 Example 1:
 

@@ -14,6 +18,7 @@ Example 1:
 		compatible = "fsl,bsc9132-memory-controller";
 		reg = <0x2000 0x1000>;
 		interrupts = <16 2 1 8>;
+		big-endian;
 	};
 
 

drivers/edac/fsl_ddr_edac.c +0 -0

index 88ecf7d..b1b7924 100644

drivers/edac/fsl_ddr_edac.c +8 -8

 	res = of_address_to_resource(op->dev.of_node, 0, &r);
 	if (res) {
 		pr_err("%s: Unable to get resource for MC err regs\n",

@@ -492,7 +512,7 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 		goto err;
 	}
 
-	sdram_ctl = in_be32(pdata->mc_vbase + MC_DDR_SDRAM_CFG);
+	sdram_ctl = ddr_in32(pdata->mc_vbase + MC_DDR_SDRAM_CFG);
 	if (!(sdram_ctl & DSC_ECC_EN)) {
 		/* no ECC */
 		pr_warn("%s: No ECC DIMMs discovered\n", __func__);

@@ -520,11 +540,11 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 
 	/* store the original error disable bits */
 	orig_ddr_err_disable =
-	    in_be32(pdata->mc_vbase + MC_ERR_DISABLE);
-	out_be32(pdata->mc_vbase + MC_ERR_DISABLE, 0);
+	    ddr_in32(pdata->mc_vbase + MC_ERR_DISABLE);
+	ddr_out32(pdata->mc_vbase + MC_ERR_DISABLE, 0);
 
 	/* clear all error bits */
-	out_be32(pdata->mc_vbase + MC_ERR_DETECT, ~0);
+	ddr_out32(pdata->mc_vbase + MC_ERR_DETECT, ~0);
 
 	if (edac_mc_add_mc_with_groups(mci, fsl_ddr_dev_groups)) {
 		edac_dbg(3, "failed edac_mc_add_mc()\n");

@@ -532,15 +552,15 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 	}
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
-		out_be32(pdata->mc_vbase + MC_ERR_INT_EN,
-			 DDR_EIE_MBEE | DDR_EIE_SBEE);
+		ddr_out32(pdata->mc_vbase + MC_ERR_INT_EN,
+			  DDR_EIE_MBEE | DDR_EIE_SBEE);
 
 		/* store the original error management threshold */
-		orig_ddr_err_sbe = in_be32(pdata->mc_vbase +
+		orig_ddr_err_sbe = ddr_in32(pdata->mc_vbase +
 					   MC_ERR_SBE) & 0xff0000;
 
 		/* set threshold to 1 error per interrupt */
-		out_be32(pdata->mc_vbase + MC_ERR_SBE, 0x10000);
+		ddr_out32(pdata->mc_vbase + MC_ERR_SBE, 0x10000);
 
 		/* register interrupts */
 		pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);

Quoted 43 lines · York Sun

Get endianness from device tree. Both big endian and little endian are supported. Default to big endian for backward compatibility to MPC85xx.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Separated from "Add support for ARM-based SoCs" patch

 .../fsl/ddr.txt}                                   |  6 ++
 drivers/edac/fsl_ddr_edac.c                        | 83 ++++++++++++++--------
 2 files changed, 58 insertions(+), 31 deletions(-)
 rename Documentation/devicetree/bindings/{powerpc/fsl/mem-ctrlr.txt => memory-controllers/fsl/ddr.txt} (74%)

Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt renamed from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt +5 -0

similarity index 74%
rename from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt
rename to Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt
index f87856f..62623f9 100644
@@ -7,6 +7,10 @@ Properties:
 		  "fsl,qoriq-memory-controller".
 - reg		: Address and size of DDR controller registers
 - interrupts	: Error interrupt of DDR controller
+- little-endian	: Specifies little-endian access to registers
+- big-endian	: Specifies big-endian access to registers
+
+If both little-endian and big-endian are omitted, big-endian will be used.
 
 Example 1:
 

@@ -14,6 +18,7 @@ Example 1:
 		compatible = "fsl,bsc9132-memory-controller";
 		reg = <0x2000 0x1000>;
 		interrupts = <16 2 1 8>;
+		big-endian;
 	};
 
 

Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt Example 2: · line 33

Quoted 5 lines

Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt renamed from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt +1 -0

@@ -24,4 +29,5 @@ Example 2:
 				"fsl,qoriq-memory-controller";
 		reg = <0x8000 0x1000>;
 		interrupts = <16 2 1 23>;
+		big-endian;
 	};

This needs an ACK from DT maintainers. They're on CC.

Quoted 4 lines

drivers/edac/fsl_ddr_edac.c +0 -0

index 88ecf7d..b1b7924 100644

...

drivers/edac/fsl_ddr_edac.c int fsl_ddr_mc_err_probe(struct platform_device *op) · line 491

Quoted 10 lines

drivers/edac/fsl_ddr_edac.c +6 -0

@@ -470,6 +483,13 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 	mci->ctl_name = pdata->name;
 	mci->dev_name = pdata->name;
 
+	if (of_find_property(op->dev.of_node, "little-endian", NULL))
+		little_endian = true;
+	else if (of_find_property(op->dev.of_node, "big-endian", NULL))
+		little_endian = false;
+	else
+		little_endian = false;

What happened here?

Considering little_endian is static, this should suffice for its initialization:

	if (of_find_property(op->dev.of_node, "little-endian", NULL))
		little_endian = true;

Quoted 47 lines

drivers/edac/fsl_ddr_edac.c +8 -8

 	res = of_address_to_resource(op->dev.of_node, 0, &r);
 	if (res) {
 		pr_err("%s: Unable to get resource for MC err regs\n",

@@ -492,7 +512,7 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 		goto err;
 	}
 
-	sdram_ctl = in_be32(pdata->mc_vbase + MC_DDR_SDRAM_CFG);
+	sdram_ctl = ddr_in32(pdata->mc_vbase + MC_DDR_SDRAM_CFG);
 	if (!(sdram_ctl & DSC_ECC_EN)) {
 		/* no ECC */
 		pr_warn("%s: No ECC DIMMs discovered\n", __func__);

@@ -520,11 +540,11 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 
 	/* store the original error disable bits */
 	orig_ddr_err_disable =
-	    in_be32(pdata->mc_vbase + MC_ERR_DISABLE);
-	out_be32(pdata->mc_vbase + MC_ERR_DISABLE, 0);
+	    ddr_in32(pdata->mc_vbase + MC_ERR_DISABLE);
+	ddr_out32(pdata->mc_vbase + MC_ERR_DISABLE, 0);
 
 	/* clear all error bits */
-	out_be32(pdata->mc_vbase + MC_ERR_DETECT, ~0);
+	ddr_out32(pdata->mc_vbase + MC_ERR_DETECT, ~0);
 
 	if (edac_mc_add_mc_with_groups(mci, fsl_ddr_dev_groups)) {
 		edac_dbg(3, "failed edac_mc_add_mc()\n");

@@ -532,15 +552,15 @@ int fsl_ddr_mc_err_probe(struct platform_device *op)
 	}
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
-		out_be32(pdata->mc_vbase + MC_ERR_INT_EN,
-			 DDR_EIE_MBEE | DDR_EIE_SBEE);
+		ddr_out32(pdata->mc_vbase + MC_ERR_INT_EN,
+			  DDR_EIE_MBEE | DDR_EIE_SBEE);
 
 		/* store the original error management threshold */
-		orig_ddr_err_sbe = in_be32(pdata->mc_vbase +
+		orig_ddr_err_sbe = ddr_in32(pdata->mc_vbase +
 					   MC_ERR_SBE) & 0xff0000;
 
 		/* set threshold to 1 error per interrupt */
-		out_be32(pdata->mc_vbase + MC_ERR_SBE, 0x10000);
+		ddr_out32(pdata->mc_vbase + MC_ERR_SBE, 0x10000);
 

drivers/edac/fsl_ddr_edac.c err: · line 596

Quoted 7 lines

drivers/edac/fsl_ddr_edac.c +1 -0

 		/* register interrupts */
 		pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);

@@ -573,6 +593,7 @@ err:
 	edac_mc_free(mci);
 	return res;
 }
+EXPORT_SYMBOL_GPL(fsl_ddr_mc_err_probe);

What's that export for?

Get endianness from device tree. Both big endian and little endian are supported. Default to big endian for backward compatibility to MPC85xx.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Separated from "Add support for ARM-based SoCs" patch

 .../fsl/ddr.txt}                                   |  6 ++
 drivers/edac/fsl_ddr_edac.c                        | 83 ++++++++++++++--------
 2 files changed, 58 insertions(+), 31 deletions(-)
 rename Documentation/devicetree/bindings/{powerpc/fsl/mem-ctrlr.txt => memory-controllers/fsl/ddr.txt} (74%)

Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt renamed from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt +0 -0

similarity index 74%
rename from Documentation/devicetree/bindings/powerpc/fsl/mem-ctrlr.txt
rename to Documentation/devicetree/bindings/memory-controllers/fsl/ddr.txt
index f87856f..62623f9 100644

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67
++++++++++++++++++++++++++++++++++ 7 files changed, 92 insertions(+), 12
deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

Quoted 10 lines · York Sun

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

Quoted 11 lines

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67
++++++++++++++++++++++++++++++++++ 7 files changed, 92 insertions(+), 12
deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

This only adds EDAC support for ARM64. What would be necessary for ARM support, e.g. LS1021A?

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67 ++++++++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+), 12 deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

arch/arm64/Kconfig.platforms +1 -0

index 7ef1d05..185a215 100644
@@ -41,6 +41,7 @@ config ARCH_EXYNOS
 
 config ARCH_LAYERSCAPE
 	bool "ARMv8 based Freescale Layerscape SoC family"
+	select EDAC_SUPPORT
 	help
 	  This enables support for the Freescale Layerscape SoC family.
 

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +5 -6

similarity index 79%
copy from arch/arm/include/asm/edac.h
copy to arch/arm64/include/asm/edac.h
index 5189fa8..36a226c 100644
@@ -18,16 +18,15 @@
 #define ASM_EDAC_H
 /*
  * ECC atomic, DMA, SMP and interrupt safe scrub function.
- * Implements the per arch edac_atomic_scrub() that EDAC use for software
+ * Implements the per arch atomic_scrub() that EDAC use for software
  * ECC scrubbing.  It reads memory and then writes back the original
  * value, allowing the hardware to detect and correct memory errors.
  */
-
-static inline void edac_atomic_scrub(void *va, u32 size)
+static inline void atomic_scrub(void *va, u32 size)
 {
-#if __LINUX_ARM_ARCH__ >= 6
-	unsigned int *virt_addr = va;
-	unsigned int temp, temp2;
+	unsigned long *virt_addr = va;
+	unsigned long temp;
+	unsigned int temp2;
 	unsigned int i;
 
 	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +1 -2

 			: "=&r"(temp), "=&r"(temp2)
 			: "r"(virt_addr)
-			: "cc");
+			: "memory");
 	}
-#endif
 }
 
 #endif

arch/arm64/include/asm/irq.h +0 -0

index b77197d..d09c008 100644

Quoted 61 lines · On Thu, 4 Aug 2016 15:58:35 -0700 York Sun <[email protected]> wrote:

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67 ++++++++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+), 12 deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

arch/arm64/Kconfig.platforms +1 -0

index 7ef1d05..185a215 100644
@@ -41,6 +41,7 @@ config ARCH_EXYNOS
 
 config ARCH_LAYERSCAPE
 	bool "ARMv8 based Freescale Layerscape SoC family"
+	select EDAC_SUPPORT
 	help
 	  This enables support for the Freescale Layerscape SoC family.
 

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +5 -6

similarity index 79%
copy from arch/arm/include/asm/edac.h
copy to arch/arm64/include/asm/edac.h
index 5189fa8..36a226c 100644
@@ -18,16 +18,15 @@
 #define ASM_EDAC_H
 /*
  * ECC atomic, DMA, SMP and interrupt safe scrub function.
- * Implements the per arch edac_atomic_scrub() that EDAC use for software
+ * Implements the per arch atomic_scrub() that EDAC use for software
  * ECC scrubbing.  It reads memory and then writes back the original
  * value, allowing the hardware to detect and correct memory errors.
  */
-
-static inline void edac_atomic_scrub(void *va, u32 size)
+static inline void atomic_scrub(void *va, u32 size)
 {
-#if __LINUX_ARM_ARCH__ >= 6
-	unsigned int *virt_addr = va;
-	unsigned int temp, temp2;
+	unsigned long *virt_addr = va;
+	unsigned long temp;
+	unsigned int temp2;
 	unsigned int i;
 

arch/arm64/include/asm/edac.h static inline void edac_atomic_scrub(void *va, u32 size) · line 39

Quoted 12 lines

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +3 -4

 	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {

@@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
 		 * so we are interrupt, DMA and SMP safe.
 		 */
 		__asm__ __volatile__("\n"
-			"1:	ldrex	%0, [%2]\n"
-			"	strex	%1, %0, [%2]\n"
-			"	teq	%1, #0\n"
-			"	bne	1b\n"
+			"1:	ldxr	%0, [%2]\n"
+			"	stxr	%w1, %0, [%2]\n"
+			"	cbnz	%w1, 1b\n"

Hiding architecture code in a driver, are we? Hmmm.

Also, how can this be safe if you have non cache-coherent DMA going on?

Quoted 15 lines

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +1 -2

 			: "=&r"(temp), "=&r"(temp2)
 			: "r"(virt_addr)
-			: "cc");
+			: "memory");
 	}
-#endif
 }
 
 #endif

arch/arm64/include/asm/irq.h +0 -0

index b77197d..d09c008 100644

arch/arm64/include/asm/irq.h line 16

Quoted 6 lines

arch/arm64/include/asm/irq.h +3 -0

@@ -11,6 +11,10 @@
 #include <asm-generic/irq.h>
 #include <asm/thread_info.h>
 
+#ifndef NO_IRQ
+#define NO_IRQ	((unsigned int)(-1))
+#endif

NAK.

NO_IRQ shouldn't exist at all, and if it had to exist, it would have the value 0. This really is a leftover of a distant past that has no real relevance on recent architectures.

Quoted 64 lines · Marc Zyngier

Quoted 61 lines · On Thu, 4 Aug 2016 15:58:35 -0700 York Sun <[email protected]> wrote:

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67 ++++++++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+), 12 deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

arch/arm64/Kconfig.platforms +1 -0

index 7ef1d05..185a215 100644
@@ -41,6 +41,7 @@ config ARCH_EXYNOS
 
 config ARCH_LAYERSCAPE
 	bool "ARMv8 based Freescale Layerscape SoC family"
+	select EDAC_SUPPORT
 	help
 	  This enables support for the Freescale Layerscape SoC family.
 

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +5 -6

similarity index 79%
copy from arch/arm/include/asm/edac.h
copy to arch/arm64/include/asm/edac.h
index 5189fa8..36a226c 100644
@@ -18,16 +18,15 @@
 #define ASM_EDAC_H
 /*
  * ECC atomic, DMA, SMP and interrupt safe scrub function.
- * Implements the per arch edac_atomic_scrub() that EDAC use for software
+ * Implements the per arch atomic_scrub() that EDAC use for software
  * ECC scrubbing.  It reads memory and then writes back the original
  * value, allowing the hardware to detect and correct memory errors.
  */
-
-static inline void edac_atomic_scrub(void *va, u32 size)
+static inline void atomic_scrub(void *va, u32 size)
 {
-#if __LINUX_ARM_ARCH__ >= 6
-	unsigned int *virt_addr = va;
-	unsigned int temp, temp2;
+	unsigned long *virt_addr = va;
+	unsigned long temp;
+	unsigned int temp2;
 	unsigned int i;
 

Quoted 14 lines

arch/arm64/include/asm/edac.h static inline void edac_atomic_scrub(void *va, u32 size) · line 39

Quoted 12 lines

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +3 -4

 	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {

@@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
 		 * so we are interrupt, DMA and SMP safe.
 		 */
 		__asm__ __volatile__("\n"
-			"1:	ldrex	%0, [%2]\n"
-			"	strex	%1, %0, [%2]\n"
-			"	teq	%1, #0\n"
-			"	bne	1b\n"
+			"1:	ldxr	%0, [%2]\n"
+			"	stxr	%w1, %0, [%2]\n"
+			"	cbnz	%w1, 1b\n"

Hiding architecture code in a driver, are we? Hmmm.

This adds atomic_scrub() for arm64. Arm already has this function. Are you suggesting to separate this part as an individual patch?

Quoted 2 lines

Also, how can this be safe if you have non cache-coherent DMA going on?

I am clueless on this topic. What's your suggestion.

Quoted 80 lines · york sun

Quoted 64 lines · Marc Zyngier

Quoted 61 lines · On Thu, 4 Aug 2016 15:58:35 -0700 York Sun <[email protected]> wrote:

Add DDR EDAC for ARM-based compatible controllers. Both big-endian and little-endian are supported.

Signed-off-by York Sun <[email protected]>

---
Change log
v3: no change
v2: Create new driver using shared DDR object

 arch/arm64/Kconfig.platforms           |  1 +
 arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
 arch/arm64/include/asm/irq.h           |  4 ++
 drivers/edac/Kconfig                   |  7 ++++
 drivers/edac/Makefile                  |  3 ++
 drivers/edac/fsl_ddr_edac.c            |  1 +
 drivers/edac/layerscape_edac.c         | 67 ++++++++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+), 12 deletions(-)
 copy arch/{arm => arm64}/include/asm/edac.h (79%)
 create mode 100644 drivers/edac/layerscape_edac.c

arch/arm64/Kconfig.platforms +1 -0

index 7ef1d05..185a215 100644
@@ -41,6 +41,7 @@ config ARCH_EXYNOS
 
 config ARCH_LAYERSCAPE
 	bool "ARMv8 based Freescale Layerscape SoC family"
+	select EDAC_SUPPORT
 	help
 	  This enables support for the Freescale Layerscape SoC family.
 

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +5 -6

similarity index 79%
copy from arch/arm/include/asm/edac.h
copy to arch/arm64/include/asm/edac.h
index 5189fa8..36a226c 100644
@@ -18,16 +18,15 @@
 #define ASM_EDAC_H
 /*
  * ECC atomic, DMA, SMP and interrupt safe scrub function.
- * Implements the per arch edac_atomic_scrub() that EDAC use for software
+ * Implements the per arch atomic_scrub() that EDAC use for software
  * ECC scrubbing.  It reads memory and then writes back the original
  * value, allowing the hardware to detect and correct memory errors.
  */
-
-static inline void edac_atomic_scrub(void *va, u32 size)
+static inline void atomic_scrub(void *va, u32 size)
 {
-#if __LINUX_ARM_ARCH__ >= 6
-	unsigned int *virt_addr = va;
-	unsigned int temp, temp2;
+	unsigned long *virt_addr = va;
+	unsigned long temp;
+	unsigned int temp2;
 	unsigned int i;
 

Quoted 14 lines

arch/arm64/include/asm/edac.h static inline void edac_atomic_scrub(void *va, u32 size) · line 39

Quoted 12 lines

arch/arm64/include/asm/edac.h renamed from arch/arm/include/asm/edac.h +3 -4

 	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {

@@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
 		 * so we are interrupt, DMA and SMP safe.
 		 */
 		__asm__ __volatile__("\n"
-			"1:	ldrex	%0, [%2]\n"
-			"	strex	%1, %0, [%2]\n"
-			"	teq	%1, #0\n"
-			"	bne	1b\n"
+			"1:	ldxr	%0, [%2]\n"
+			"	stxr	%w1, %0, [%2]\n"
+			"	cbnz	%w1, 1b\n"

Hiding architecture code in a driver, are we? Hmmm.

Quoted 6 lines

This adds atomic_scrub() for arm64. Arm already has this function. Are you suggesting to separate this part as an individual patch?

Quoted 2 lines

Also, how can this be safe if you have non cache-coherent DMA going on?

I am clueless on this topic. What's your suggestion.

We've discussed this in the past for ARMv7 and the bottom line is that it's not safe to scrub memory backing non-coherent DMA buffers. You will likely end up with data corruption if you do that.

The two options are:

(1) Restrict scrubbing to non-DMA memory, or
(2) Only enable scrubbing if the system is completely cache-coherent

The problem with (1) is that we can't detect that, particularly when the streaming DMA API is in use.

The problem with (2) is that you can have a coherent system that decides dynamically to drop in and out of coherency as a result of, e.g. PCI NoSnoop transactions.