[PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
From: Thor Thayer
Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device model. The SDRAM
controller is using the Memory Controller model.
Each type of ECC is individually configurable.
The SDRAM ECC is a separate Kconfig option because:
1) the SDRAM preparation can take almost 2 seconds on boot and some
customers need a faster boot time.
2) the SDRAM has an ECC initialization dependency on the preloader
which is outside the kernel. It is desirable to be able to turn the
SDRAM on & off separately.
Signed-off-by: Thor Thayer
Signed-off-by: Dinh Nguyen
---
v7: s/of_get_named_gen_pool/of_gen_pool_get
Remove #ifdef for EDAC_DEBUG
Use -ENODEV instead of EPROBE_DEFER
v6: Convert to nested EDAC in device tree. Force L2 cache
on for L2Cache ECC & remove L2 cache syscon for checking
enable bit. Update year in header.
v5: No Change
v4: Change mask defines to use BIT().
Fix comment style to agree with kernel coding style.
Better printk description for read != write in trigger.
Remove SysFS debugging message.
Better dci->mod_name
Move gen_pool pointer assignment to end of function.
Invert logic to reduce indent in ocram depenency check.
Change from dev_err() to edac_printk()
Replace magic numbers with defines & comments.
Improve error injection test.
Change Makefile intermediary name to altr (from alt)
v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
instead of separate files.
v2: Fix L2 dependency comments.
---
drivers/edac/Kconfig | 16 ++
drivers/edac/Makefile | 5 +-
drivers/edac/altera_edac.c | 488 -
3 files changed, 507 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index ef25000..b80b4ad 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -376,6 +376,22 @@ config EDAC_ALTERA_MC
preloader must initialize the SDRAM before loading
the kernel.
+config EDAC_ALTERA_L2C
+ bool "Altera L2 Cache EDAC"
+ depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA
+ select CACHE_L2X0
+ help
+ Support for error detection and correction on the
+ Altera L2 cache Memory for Altera SoCs. This option
+ requires L2 cache so it will force that selection.
+
+config EDAC_ALTERA_OCRAM
+ bool "Altera On-Chip RAM EDAC"
+ depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA && SRAM && GENERIC_ALLOCATOR
+ help
+ Support for error detection and correction on the
+ Altera On-Chip RAM Memory for Altera SoCs.
+
config EDAC_SYNOPSYS
tristate "Synopsys DDR Memory Controller"
depends on EDAC_MM_EDAC && ARCH_ZYNQ
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index dbf53e0..8f1c6fc 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -67,6 +67,9 @@ obj-$(CONFIG_EDAC_OCTEON_L2C) += octeon_edac-l2c.o
obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o
obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o
-obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o
+altr_edac-y:= altera_edac.o
+obj-$(CONFIG_EDAC_ALTERA_MC) += altr_edac.o
+obj-$(CONFIG_EDAC_ALTERA_L2C) += altr_edac.o
+obj-$(CONFIG_EDAC_ALTERA_OCRAM)+= altr_edac.o
obj-$(CONFIG_EDAC_SYNOPSYS)+= synopsys_edac.o
obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 9296409..154ac8c 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -17,8 +17,10 @@
* Adapted from the highbank_mc_edac driver.
*/
+#include
#include
#include
+#include
#include
#include
#include
@@ -34,6 +36,7 @@
#define EDAC_MOD_STR "altera_edac"
#define EDAC_VERSION "1"
+#define EDAC_DEVICE"ALTR_MEM"
static const struct altr_sdram_prv_data c5_data = {
.ecc_ctrl_offset= CV_CTLCFG_OFST,
@@ -75,6 +78,33 @@ static const struct altr_sdram_prv_data a10_data = {
.ue_set_mask= A10_DIAGINT_TDERRA_MASK,
};
+/** EDAC Device Defines **/
+
+/* OCRAM ECC Management Group Defines */
+#define ALTR_MAN_GRP_OCRAM_ECC_OFFSET 0x04
+#define ALTR_OCR_ECC_EN_MASKBIT(0)
+#define ALTR_OCR_ECC_INJS_MASK BIT(1)
+#define ALTR_OCR_ECC_INJD_MASK BIT(2)
+#define ALTR_OCR_ECC_SERR_MASK BIT(3)
+#define ALTR_OCR_ECC_DERR_MASK BIT(4)
+
+/* L2 ECC Management Group Defines */
+#define ALTR_MAN_GRP_L2_ECC_OFFSET 0x00
+#define ALTR_L2_ECC_EN_MASK BIT(0)
+#define ALTR_L2_ECC_INJS_MASK BIT(1)
+#define ALTR_L2_ECC_INJD_MASK BIT(2)
+
+#define ALTR_UE_TRIGGER_CHAR'U' /* Trigger for UE */
+#define ALTR_TRIGGER_READ_WRD_CNT 32/* Line size x 4 */
+#define ALTR_TRIG_OCRAM_BYTE_SIZE
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Tue, Oct 27, 2015 at 03:38:12PM -0500, dingu...@opensource.altera.com wrote:
> From: Thor Thayer
>
> Adding L2 Cache and On-Chip RAM EDAC support for the
> Altera SoCs using the EDAC device model. The SDRAM
> controller is using the Memory Controller model.
>
> Each type of ECC is individually configurable.
>
> The SDRAM ECC is a separate Kconfig option because:
> 1) the SDRAM preparation can take almost 2 seconds on boot and some
> customers need a faster boot time.
> 2) the SDRAM has an ECC initialization dependency on the preloader
> which is outside the kernel. It is desirable to be able to turn the
> SDRAM on & off separately.
>
> Signed-off-by: Thor Thayer
> Signed-off-by: Dinh Nguyen
> ---
> v7: s/of_get_named_gen_pool/of_gen_pool_get
> Remove #ifdef for EDAC_DEBUG
> Use -ENODEV instead of EPROBE_DEFER
>
> v6: Convert to nested EDAC in device tree. Force L2 cache
> on for L2Cache ECC & remove L2 cache syscon for checking
> enable bit. Update year in header.
>
> v5: No Change
>
> v4: Change mask defines to use BIT().
> Fix comment style to agree with kernel coding style.
> Better printk description for read != write in trigger.
> Remove SysFS debugging message.
> Better dci->mod_name
> Move gen_pool pointer assignment to end of function.
> Invert logic to reduce indent in ocram depenency check.
> Change from dev_err() to edac_printk()
> Replace magic numbers with defines & comments.
> Improve error injection test.
> Change Makefile intermediary name to altr (from alt)
>
> v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
> instead of separate files.
>
> v2: Fix L2 dependency comments.
> ---
> drivers/edac/Kconfig | 16 ++
> drivers/edac/Makefile | 5 +-
> drivers/edac/altera_edac.c | 488
> -
> 3 files changed, 507 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index ef25000..b80b4ad 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -376,6 +376,22 @@ config EDAC_ALTERA_MC
> preloader must initialize the SDRAM before loading
> the kernel.
>
> +config EDAC_ALTERA_L2C
> + bool "Altera L2 Cache EDAC"
> + depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA
> + select CACHE_L2X0
> + help
> + Support for error detection and correction on the
> + Altera L2 cache Memory for Altera SoCs. This option
> + requires L2 cache so it will force that selection.
> +
> +config EDAC_ALTERA_OCRAM
> + bool "Altera On-Chip RAM EDAC"
> + depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA && SRAM && GENERIC_ALLOCATOR
> + help
> + Support for error detection and correction on the
> + Altera On-Chip RAM Memory for Altera SoCs.
> +
> config EDAC_SYNOPSYS
> tristate "Synopsys DDR Memory Controller"
> depends on EDAC_MM_EDAC && ARCH_ZYNQ
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index dbf53e0..8f1c6fc 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -67,6 +67,9 @@ obj-$(CONFIG_EDAC_OCTEON_L2C) +=
> octeon_edac-l2c.o
> obj-$(CONFIG_EDAC_OCTEON_LMC)+= octeon_edac-lmc.o
> obj-$(CONFIG_EDAC_OCTEON_PCI)+= octeon_edac-pci.o
>
> -obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o
> +altr_edac-y := altera_edac.o
> +obj-$(CONFIG_EDAC_ALTERA_MC) += altr_edac.o
> +obj-$(CONFIG_EDAC_ALTERA_L2C)+= altr_edac.o
> +obj-$(CONFIG_EDAC_ALTERA_OCRAM) += altr_edac.o
What are those supposed to accomplish?
> obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o
> obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 9296409..154ac8c 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -17,8 +17,10 @@
> * Adapted from the highbank_mc_edac driver.
> */
>
> +#include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -34,6 +36,7 @@
>
> #define EDAC_MOD_STR "altera_edac"
> #define EDAC_VERSION "1"
> +#define EDAC_DEVICE "ALTR_MEM"
Let's simply call it "Altera" - it is more human-friendly.
> static const struct altr_sdram_prv_data c5_data = {
> .ecc_ctrl_offset= CV_CTLCFG_OFST,
> @@ -75,6 +78,33 @@ static const struct altr_sdram_prv_data a10_data = {
> .ue_set_mask= A10_DIAGINT_TDERRA_MASK,
> };
>
> +/** EDAC Device Defines **/
> +
> +/* OCRAM ECC Management Group Defines */
> +#define ALTR_MAN_GRP_OCRAM_ECC_OFFSET 0x04
> +#define ALTR_OCR_ECC_EN_MASKBIT(0)
> +#define ALTR_OCR_ECC_INJS_MASK BIT(1)
> +#define ALTR_OCR_ECC_INJD_MASK BIT(2)
> +#define ALTR_OCR_ECC_SERR_MASK BIT(3)
> +#define ALTR_OCR_ECC_DERR_MASK BIT(
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 11/19/2015 12:34 PM, Borislav Petkov wrote:
> On Tue, Oct 27, 2015 at 03:38:12PM -0500, dingu...@opensource.altera.com
> wrote:
>> From: Thor Thayer
>>
>> Adding L2 Cache and On-Chip RAM EDAC support for the
>> Altera SoCs using the EDAC device model. The SDRAM
>> controller is using the Memory Controller model.
>>
>> Each type of ECC is individually configurable.
>>
>> The SDRAM ECC is a separate Kconfig option because:
>> 1) the SDRAM preparation can take almost 2 seconds on boot and some
>> customers need a faster boot time.
>> 2) the SDRAM has an ECC initialization dependency on the preloader
>> which is outside the kernel. It is desirable to be able to turn the
>> SDRAM on & off separately.
>>
>> Signed-off-by: Thor Thayer
>> Signed-off-by: Dinh Nguyen
>> ---
>> v7: s/of_get_named_gen_pool/of_gen_pool_get
>> Remove #ifdef for EDAC_DEBUG
>> Use -ENODEV instead of EPROBE_DEFER
>>
>> v6: Convert to nested EDAC in device tree. Force L2 cache
>> on for L2Cache ECC & remove L2 cache syscon for checking
>> enable bit. Update year in header.
>>
>> v5: No Change
>>
>> v4: Change mask defines to use BIT().
>> Fix comment style to agree with kernel coding style.
>> Better printk description for read != write in trigger.
>> Remove SysFS debugging message.
>> Better dci->mod_name
>> Move gen_pool pointer assignment to end of function.
>> Invert logic to reduce indent in ocram depenency check.
>> Change from dev_err() to edac_printk()
>> Replace magic numbers with defines & comments.
>> Improve error injection test.
>> Change Makefile intermediary name to altr (from alt)
>>
>> v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
>> instead of separate files.
>>
>> v2: Fix L2 dependency comments.
>> ---
>> drivers/edac/Kconfig | 16 ++
>> drivers/edac/Makefile | 5 +-
>> drivers/edac/altera_edac.c | 488
>> -
>> 3 files changed, 507 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index ef25000..b80b4ad 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
[snip]
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index dbf53e0..8f1c6fc 100644
>> --- a/drivers/edac/Makefile
>> +++ b/drivers/edac/Makefile
>> @@ -67,6 +67,9 @@ obj-$(CONFIG_EDAC_OCTEON_L2C) +=
>> octeon_edac-l2c.o
>> obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o
>> obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o
>>
>> -obj-$(CONFIG_EDAC_ALTERA_MC)+= altera_edac.o
>> +altr_edac-y := altera_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_MC)+= altr_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_L2C) += altr_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_OCRAM) += altr_edac.o
>
> What are those supposed to accomplish?
>
We tried to jam the L2 and OCRAM EDAC functionality in the same
altr_edac.c file. It looks like it might be clean if we split out the L2
and OCRAM functions into their appropriate files(altr_edac_l2.c and
altr_edac_ocram.c). Do you agree?
>> obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o
>> obj-$(CONFIG_EDAC_XGENE)+= xgene_edac.o
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index 9296409..154ac8c 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -17,8 +17,10 @@
>> * Adapted from the highbank_mc_edac driver.
>> */
>>
>> +#include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -34,6 +36,7 @@
>>
>> #define EDAC_MOD_STR"altera_edac"
>> #define EDAC_VERSION"1"
>> +#define EDAC_DEVICE "ALTR_MEM"
>
> Let's simply call it "Altera" - it is more human-friendly.
>
Ok.
>> static const struct altr_sdram_prv_data c5_data = {
>> .ecc_ctrl_offset= CV_CTLCFG_OFST,
>> @@ -75,6 +78,33 @@ static const struct altr_sdram_prv_data a10_data = {
>> .ue_set_mask= A10_DIAGINT_TDERRA_MASK,
>> };
>>
>> +/** EDAC Device Defines **/
>> +
>> +/* OCRAM ECC Management Group Defines */
>> +#define ALTR_MAN_GRP_OCRAM_ECC_OFFSET 0x04
>> +#define ALTR_OCR_ECC_EN_MASKBIT(0)
>> +#define ALTR_OCR_ECC_INJS_MASK BIT(1)
>> +#define ALTR_OCR_ECC_INJD_MASK BIT(2)
>> +#define ALTR_OCR_ECC_SERR_MASK BIT(3)
>> +#define ALTR_OCR_ECC_DERR_MASK BIT(4)
>> +
>> +/* L2 ECC Management Group Defines */
>> +#define ALTR_MAN_GRP_L2_ECC_OFFSET 0x00
>> +#define ALTR_L2_ECC_EN_MASK BIT(0)
>> +#define ALTR_L2_ECC_INJS_MASK BIT(1)
>> +#define ALTR_L2_ECC_INJD_MASK BIT(2)
>
> Single bit masks? You don't need to call them "_MASK" - simply remove
> it.
>
Ok.
>> +#define ALTR_UE_TRIGGER_CHAR'U' /* Trigger for UE */
>> +#define ALTR_TRIGG
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 11:17:29AM -0600, Dinh Nguyen wrote: > We tried to jam the L2 and OCRAM EDAC functionality in the same > altr_edac.c file. It looks like it might be clean if we split out the L2 > and OCRAM functions into their appropriate files(altr_edac_l2.c and > altr_edac_ocram.c). Do you agree? "Clean" in what sense? To me clean is when there's a single compilation unit altera_edac.c which contains all Altera-specific code. > These bindings were in a separate patch that you were not CC'd. I'll > keep you on the entire patch set in the future. Yes, I believe the devicetree definitions need to go hand-in-hand with its user(s). -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/04/2016 01:46 PM, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 11:17:29AM -0600, Dinh Nguyen wrote: >> We tried to jam the L2 and OCRAM EDAC functionality in the same >> altr_edac.c file. It looks like it might be clean if we split out the L2 >> and OCRAM functions into their appropriate files(altr_edac_l2.c and >> altr_edac_ocram.c). Do you agree? > > "Clean" in what sense? To me clean is when there's a single compilation > unit altera_edac.c which contains all Altera-specific code. > altr_edac.c originally added support for SDRAM. Now we're adding support for L2 and OCRAM into the same file by using #ifdef CONFIG_EDAC_ALTERA_OCRAM and CONFIG_EDAC_ALTERA_L2C. So "clean" was to move the l2 and ocram implementation into separate files. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 02:04:08PM -0600, Dinh Nguyen wrote: > altr_edac.c originally added support for SDRAM. Now we're adding support > for L2 and OCRAM into the same file by using #ifdef > CONFIG_EDAC_ALTERA_OCRAM and CONFIG_EDAC_ALTERA_L2C. So "clean" was to > move the l2 and ocram implementation into separate files. So what's wrong with adding those to the same file and using different registration functions? Like xgene_edac does, for example. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/04/2016 02:30 PM, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 02:04:08PM -0600, Dinh Nguyen wrote: >> altr_edac.c originally added support for SDRAM. Now we're adding support >> for L2 and OCRAM into the same file by using #ifdef >> CONFIG_EDAC_ALTERA_OCRAM and CONFIG_EDAC_ALTERA_L2C. So "clean" was to >> move the l2 and ocram implementation into separate files. > > So what's wrong with adding those to the same file and using different > registration functions? Like xgene_edac does, for example. > I don't see a way for the xgene to manually build for each configuration using Kconfig? For SoCFGPA, we would like to keep the option to build for each type of ECC. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 02:46:26PM -0600, Dinh Nguyen wrote: > I don't see a way for the xgene to manually build for each configuration > using Kconfig? For SoCFGPA, we would like to keep the option to build > for each type of ECC. xgene builds everything in and unconditionally. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 02:55:43PM -0600, Dinh Nguyen wrote: > Right. So for us, if we build in SDRAM ECC unconditionally, there is a > requirement with the bootloader to turn on ECC and scrub the memory. Huh, how does a built-in piece of code cause the bootloader to do something?!? And how would the bootloader know what's in the kernel? The bootloader runs first and hands off to the kernel... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/04/2016 02:59 PM, Borislav Petkov wrote: > On Mon, Jan 04, 2016 at 02:46:26PM -0600, Dinh Nguyen wrote: >> I don't see a way for the xgene to manually build for each configuration >> using Kconfig? For SoCFGPA, we would like to keep the option to build >> for each type of ECC. > > xgene builds everything in and unconditionally. > Right. So for us, if we build in SDRAM ECC unconditionally, there is a requirement with the bootloader to turn on ECC and scrub the memory. This can take up to ~2 seconds and some may not want that. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/04/2016 03:07 PM, Borislav Petkov wrote: On Mon, Jan 04, 2016 at 02:55:43PM -0600, Dinh Nguyen wrote: Right. So for us, if we build in SDRAM ECC unconditionally, there is a requirement with the bootloader to turn on ECC and scrub the memory. Huh, how does a built-in piece of code cause the bootloader to do something?!? And how would the bootloader know what's in the kernel? The bootloader runs first and hands off to the kernel... Hi Boris, The decision about ECC or non-ECC SDRAM is made before building the Linux image and must be matched to the appropriate bootloader (ECC or non-ECC). If ECC is desired for SDRAM, the bootloader enables SDRAM ECC and then initializes the memory contents (scrub) before the Linux image is loaded into SDRAM. The ECC syndromes are calculated and stored in SDRAM only when SDRAM ECC is enabled and the SDRAM data is written (in the bootloader case, this is the Linux image). If we suddenly switched ECC on during Linux initialization, we'd be flooded with ECC errors since the ECC syndromes won't match the data for the Linux image. The scrubbing process takes more time to boot which some of our customers don't want. This is what Dinh was referring to. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 03:33:23PM -0600, Thor Thayer wrote: > The decision about ECC or non-ECC SDRAM is made before building the Linux > image and must be matched to the appropriate bootloader (ECC or non-ECC). > > If ECC is desired for SDRAM, the bootloader enables SDRAM ECC and then > initializes the memory contents (scrub) before the Linux image is loaded > into SDRAM. > > The ECC syndromes are calculated and stored in SDRAM only when SDRAM ECC is > enabled and the SDRAM data is written (in the bootloader case, this is the > Linux image). If we suddenly switched ECC on during Linux initialization, > we'd be flooded with ECC errors since the ECC syndromes won't match the data > for the Linux image. > > The scrubbing process takes more time to boot which some of our customers > don't want. This is what Dinh was referring to. So that still doesn't have any effect on what's compiled in the EDAC module, AFAICT. You simply build everything in and depending on whether ECC is enabled or not in the bootloader, altera_edac behaves accordingly. On a system with ECC *not* enabled, it would simply have the SDRAM ECC functionality inactive. This is no different than an x86 system where you enter the BIOS and enable or disable ECC. The EDAC module queries whether ECC has been enabled or not and behaves accordingly. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/04/2016 04:01 PM, Borislav Petkov wrote: On Mon, Jan 04, 2016 at 03:33:23PM -0600, Thor Thayer wrote: The decision about ECC or non-ECC SDRAM is made before building the Linux image and must be matched to the appropriate bootloader (ECC or non-ECC). If ECC is desired for SDRAM, the bootloader enables SDRAM ECC and then initializes the memory contents (scrub) before the Linux image is loaded into SDRAM. The ECC syndromes are calculated and stored in SDRAM only when SDRAM ECC is enabled and the SDRAM data is written (in the bootloader case, this is the Linux image). If we suddenly switched ECC on during Linux initialization, we'd be flooded with ECC errors since the ECC syndromes won't match the data for the Linux image. The scrubbing process takes more time to boot which some of our customers don't want. This is what Dinh was referring to. So that still doesn't have any effect on what's compiled in the EDAC module, AFAICT. You simply build everything in and depending on whether ECC is enabled or not in the bootloader, altera_edac behaves accordingly. On a system with ECC *not* enabled, it would simply have the SDRAM ECC functionality inactive. This is no different than an x86 system where you enter the BIOS and enable or disable ECC. The EDAC module queries whether ECC has been enabled or not and behaves accordingly. OK. I see your point for SDRAM. However, in the case of OCRAM and L2 cache ECC, we want to be able to enable them individually which is what the following does. >> -obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o >> +altr_edac-y := altera_edac.o >> +obj-$(CONFIG_EDAC_ALTERA_MC) += altr_edac.o >> +obj-$(CONFIG_EDAC_ALTERA_L2C) += altr_edac.o >> +obj-$(CONFIG_EDAC_ALTERA_OCRAM) += altr_edac.o > > What are those supposed to accomplish? > and then the defines are also used to conditionally include the L2 or OCRAM ECC functions because everything is in one file. However, the highbank and octeon edacs are split into separate files for L2 which Dinh pointed may be cleaner for individual control. Thanks, Thor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Mon, Jan 04, 2016 at 05:42:40PM -0600, Thor Thayer wrote: > and then the defines are also used to conditionally include the L2 or OCRAM > ECC functions because everything is in one file. So? You don't have to do those funny games in the Makefile. Instead, you have your main CONFIG_EDAC_ALTERA_MC option and all the other CONFIG_* options depend on it. The ifdeffery in altera_edac.c then takes care of what needs to be enabled or not. However(!), your driver is not huge or something. So I still don't understand why you need that split and those additional config options and why not keep it all together in one file. What is the compelling use case for that split and additional complexity? > However, the highbank and octeon edacs are split into separate files for L2 > which Dinh pointed may be cleaner for individual control. I should've nacked that split at the time. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On 01/05/2016 04:58 AM, Borislav Petkov wrote: On Mon, Jan 04, 2016 at 05:42:40PM -0600, Thor Thayer wrote: and then the defines are also used to conditionally include the L2 or OCRAM ECC functions because everything is in one file. So? You don't have to do those funny games in the Makefile. Instead, you have your main CONFIG_EDAC_ALTERA_MC option and all the other CONFIG_* options depend on it. The ifdeffery in altera_edac.c then takes care of what needs to be enabled or not. However(!), your driver is not huge or something. So I still don't understand why you need that split and those additional config options and why not keep it all together in one file. What is the compelling use case for that split and additional complexity? However, the highbank and octeon edacs are split into separate files for L2 which Dinh pointed may be cleaner for individual control. I should've nacked that split at the time. OK, Thanks for the clarification. The CONFIG_EDAC_ALTERA_MC is a little confusing because it refers to the Memory Controller (SDRAM) and uses that in the menu string(Altera SDRAM Memory Controller EDAC). Would it be confusing to rename this CONFIG_EDAC_ALTERA, update the SDRAM code to check the ECC Enable bit instead of this config option and update the string in the menu? I'll keep this as one file and implement your suggested changes and resubmit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
On Tue, Jan 05, 2016 at 02:37:48PM -0600, Thor Thayer wrote: > The CONFIG_EDAC_ALTERA_MC is a little confusing because it refers to the > Memory Controller (SDRAM) and uses that in the menu string(Altera SDRAM > Memory Controller EDAC). > > Would it be confusing to rename this CONFIG_EDAC_ALTERA, update the SDRAM > code to check the ECC Enable bit instead of this config option and update > the string in the menu? Sounds good to me. > I'll keep this as one file and implement your suggested changes and > resubmit. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
