EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
Subsystems (6)
PATCH v8 1/4 EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support
From: Thor Thayer <[email protected]>
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.
---
v8: Remove MASK from single bit mask names.
s/altr,edac/altr,socfpga-ecc-manager
Use debugfs instead of sysfs.
Add chip family name to match string.
Fix header year.
Fix build dependencies & change commit accordingly. s/CONFIG_EDAC_ALTERA_MC/CONFIG_EDAC_ALTERA
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 | 26 ++- drivers/edac/Makefile | 2 +- drivers/edac/altera_edac.c | 484 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 504 insertions(+), 8 deletions(-)
drivers/edac/Kconfig +21 -5
@@ -367,14 +367,30 @@ config EDAC_OCTEON_PCI Support for error detection and correction on the Cavium Octeon family of SOCs. -config EDAC_ALTERA_MC - bool "Altera SDRAM Memory Controller EDAC" +config EDAC_ALTERA + bool "Altera SOCFPGA ECC" depends on EDAC_MM_EDAC=y && ARCH_SOCFPGA help Support for error detection and correction on the - Altera SDRAM memory controller. Note that the - preloader must initialize the SDRAM before loading - the kernel. + Altera SOCs. This must be selected for SDRAM ECC. + Note that the preloader must initialize the SDRAM + before loading the kernel. + +config EDAC_ALTERA_L2C + bool "Altera L2 Cache ECC" + depends on EDAC_ALTERA=y + 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 ECC" + depends on EDAC_ALTERA=y && 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"
drivers/edac/Makefile +1 -1
@@ -67,6 +67,6 @@ 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 +obj-$(CONFIG_EDAC_ALTERA) += altera_edac.o obj-$(CONFIG_EDAC_SYNOPSYS) += synopsys_edac.o obj-$(CONFIG_EDAC_XGENE) += xgene_edac.o
drivers/edac/altera_edac.c +482 -2
513 lines not on this page · raw message
Signature · 1 line
1.7.9.5
PATCH v8 2/4 ARM: dts: Add Altera L2 Cache and OCRAM EDAC entries
From: Thor Thayer <[email protected]>
Adding the device tree entries and bindings needed to support the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html
---
v8: Fix node names to include chip family and use ecc manager
to better describe the driver. Rename socfpga-edac.txt to socfpga-eccmgr.txt.
v7: No Change
v6: Change to nested EDAC device nodes based on community
feedback. Remove L2 syscon. Use consolidated binding.
v3-5: No Change
v2: Remove OCRAM declaration and reference prior patch.
--- .../bindings/arm/altera/socfpga-eccmgr.txt | 49 ++++++++++++++++++++ arch/arm/boot/dts/socfpga.dtsi | 20 ++++++++ 2 files changed, 69 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +49 -0
@@ -0,0 +1,49 @@ +Altera SoCFPGA ECC Manager +This driver uses the EDAC framework to implement the SOCFPGA ECC Manager. +The ECC Manager counts and corrects single bit errors and counts/handles +double bit errors which are uncorrectable. + +Required Properties: +- compatible : Should be "altr,socfpga-ecc-manager" +- #address-cells: must be 1 +- #size-cells: must be 1 +- ranges : standard definition, should translate from local addresses + +Subcomponents: + +L2 Cache ECC +Required Properties: +- compatible : Should be "altr,socfpga-l2-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +On Chip RAM ECC +Required Properties: +- compatible : Should be "altr,socfpga-ocram-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- iram : phandle to On-Chip RAM definition. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +Example: + + eccmgr: eccmgr at 0xffd08140 { + compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc at ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc at ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + };
arch/arm/boot/dts/socfpga.dtsi +20 -0
@@ -656,6 +656,26 @@ status = "disabled"; }; + eccmgr: eccmgr at 0xffd08140 { + compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc at ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc at ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + }; + L2: l2-cache at fffef000 { compatible = "arm,pl310-cache"; reg = <0xfffef000 0x1000>;
Signature · 1 line
1.7.9.5
Re: [PATCHv8 2/4] ARM: dts: Add Altera L2 Cache and OCRAM EDAC entries
Quoted 18 lines · tthayer at opensource.altera.com
From: Thor Thayer <[email protected]>
Adding the device tree entries and bindings needed to support the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.htmlSigned-off-by Thor Thayer <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>---
v8: Fix node names to include chip family and use ecc manager
to better describe the driver. Rename socfpga-edac.txt to socfpga-eccmgr.txt.
v7: No Change
v6: Change to nested EDAC device nodes based on community
feedback. Remove L2 syscon. Use consolidated binding.
v3-5: No Change
v2: Remove OCRAM declaration and reference prior patch.
Quoted 5 lines
--- .../bindings/arm/altera/socfpga-eccmgr.txt | 49 ++++++++++++++++++++ arch/arm/boot/dts/socfpga.dtsi | 20 ++++++++ 2 files changed, 69 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
A couple of nits, otherwise:
Quoted 28 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +21
-0@@ -0,0 +1,49 @@ +Altera SoCFPGA ECC Manager +This driver uses the EDAC framework to implement the SOCFPGA ECC Manager. +The ECC Manager counts and corrects single bit errors and counts/handles +double bit errors which are uncorrectable. + +Required Properties: +- compatible : Should be "altr,socfpga-ecc-manager" +- #address-cells: must be 1 +- #size-cells: must be 1 +- ranges : standard definition, should translate from local addresses + +Subcomponents: + +L2 Cache ECC +Required Properties: +- compatible : Should be "altr,socfpga-l2-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +On Chip RAM ECC
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt line 31
Quoted 10 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +10
-0+Required Properties: +- compatible : Should be "altr,socfpga-ocram-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- iram : phandle to On-Chip RAM definition. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +Example: + + eccmgr: eccmgr at 0xffd08140 {
drop the '0x'
Quoted 23 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +18
-0+ compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc at ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc at ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + };arch/arm/boot/dts/socfpga.dtsi +0
-0
arch/arm/boot/dts/socfpga.dtsi line 659
Quoted 4 lines
arch/arm/boot/dts/socfpga.dtsi +1
-0@@ -656,6 +656,26 @@ status = "disabled"; }; + eccmgr: eccmgr at 0xffd08140 {
and here.
Quoted 30 lines
arch/arm/boot/dts/socfpga.dtsi +19
-0+ compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc at ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc at ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + }; + L2: l2-cache at fffef000 { compatible = "arm,pl310-cache"; reg = <0xfffef000 0x1000>;Signature · 7 lines
1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCHv8 2/4] ARM: dts: Add Altera L2 Cache and OCRAM EDAC entries
Quoted 25 lines · Rob Herring
Quoted 18 lines · On Thu, Jan 21, 2016 at 11:34:26AM -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
Adding the device tree entries and bindings needed to support the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.htmlSigned-off-by Thor Thayer <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>---
v8: Fix node names to include chip family and use ecc manager
to better describe the driver. Rename socfpga-edac.txt to socfpga-eccmgr.txt.
v7: No Change
v6: Change to nested EDAC device nodes based on community
feedback. Remove L2 syscon. Use consolidated binding.
v3-5: No Change
v2: Remove OCRAM declaration and reference prior patch.Quoted 5 lines
--- .../bindings/arm/altera/socfpga-eccmgr.txt | 49 ++++++++++++++++++++ arch/arm/boot/dts/socfpga.dtsi | 20 ++++++++ 2 files changed, 69 insertions(+)create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
Quoted 4 lines
A couple of nits, otherwise:
Acked-by Rob Herring <[email protected]>
Great! I will make these changes for my next version that fixes some coding issue. Thank you for reviewing.
Quoted 101 lines
Quoted 28 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +21
-0@@ -0,0 +1,49 @@ +Altera SoCFPGA ECC Manager +This driver uses the EDAC framework to implement the SOCFPGA ECC Manager. +The ECC Manager counts and corrects single bit errors and counts/handles +double bit errors which are uncorrectable. + +Required Properties: +- compatible : Should be "altr,socfpga-ecc-manager" +- #address-cells: must be 1 +- #size-cells: must be 1 +- ranges : standard definition, should translate from local addresses + +Subcomponents: + +L2 Cache ECC +Required Properties: +- compatible : Should be "altr,socfpga-l2-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +On Chip RAM ECCDocumentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt line 31
Quoted 10 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +10
-0+Required Properties: +- compatible : Should be "altr,socfpga-ocram-ecc" +- reg : Address and size for ECC error interrupt clear registers. +- iram : phandle to On-Chip RAM definition. +- interrupts : Should be single bit error interrupt, then double bit error + interrupt. Note the rising edge type. + +Example: + + eccmgr: eccmgr@0xffd08140 {drop the '0x'
Quoted 23 lines
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt new file +18
-0+ compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc@ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc@ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + };arch/arm/boot/dts/socfpga.dtsi +0
-0arch/arm/boot/dts/socfpga.dtsi line 659
Quoted 4 lines
arch/arm/boot/dts/socfpga.dtsi +1
-0@@ -656,6 +656,26 @@ status = "disabled"; }; + eccmgr: eccmgr@0xffd08140 {and here.
Quoted 30 lines
arch/arm/boot/dts/socfpga.dtsi +19
-1+ compatible = "altr,socfpga-ecc-manager"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + l2-ecc@ffd08140 { + compatible = "altr,socfpga-l2-ecc"; + reg = <0xffd08140 0x4>; + interrupts = <0 36 1>, <0 37 1>; + }; + + ocram-ecc@ffd08144 { + compatible = "altr,socfpga-ocram-ecc"; + reg = <0xffd08144 0x4>; + iram = <&ocram>; + interrupts = <0 178 1>, <0 179 1>; + }; + }; + L2: l2-cache@fffef000 { compatible = "arm,pl310-cache"; reg = <0xfffef000 0x1000>; --1.7.9.5
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v8 3/4 ARM: socfpga: enable L2 cache ECC on startup
In reply to [email protected]From: Dinh Nguyen <[email protected]>
This patch enables the ECC for L2 cache on machine startup. The ECC has to be enabled before data is stored in memory otherwise the ECC will fail on reads.
---
v8: Address community suggestions for strings. Fix string based on
maintainer feedback. Update year in header.
v7: unmap locally scoped mapped_l2_edac_addr and add of_node_put(np)
v6: Remove pr_debug() & update year in header.
--- arch/arm/mach-socfpga/Makefile | 1 + arch/arm/mach-socfpga/core.h | 1 + arch/arm/mach-socfpga/l2_cache.c | 43 ++++++++++++++++++++++++++++++++++++++ arch/arm/mach-socfpga/socfpga.c | 2 ++ 4 files changed, 47 insertions(+) create mode 100644 arch/arm/mach-socfpga/l2_cache.c
arch/arm/mach-socfpga/Makefile +1 -0
@@ -5,3 +5,4 @@ obj-y := socfpga.o obj-$(CONFIG_SMP) += headsmp.o platsmp.o obj-$(CONFIG_SOCFPGA_SUSPEND) += pm.o self-refresh.o +obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
arch/arm/mach-socfpga/core.h +1 -0
@@ -36,6 +36,7 @@extern void socfpga_init_clocks (void );extern void socfpga_sysmgr_init (void ); +void socfpga_init_l2_ecc (void );extern void __iomem *sys_manager_base_addr;extern void __iomem *rst_manager_base_addr;
arch/arm/mach-socfpga/l2_cache.c new file +43 -0
43 lines not on this page · raw message
arch/arm/mach-socfpga/socfpga.c +2 -0
@@ -59,6 +59,8 @@ static void __init socfpga_init_irq(void) {irqchip_init ();socfpga_sysmgr_init (); +if (IS_ENABLED (CONFIG_EDAC_ALTERA_L2C )) +socfpga_init_l2_ecc (); }static void socfpga_cyclone5_restart (enum reboot_mode mode,const char *cmd)
Signature · 1 line
1.7.9.5
PATCH v8 4/4 ARM: socfpga: Enable OCRAM ECC on startup
In reply to [email protected]From: Dinh Nguyen <[email protected]>
This patch enables the ECC for On-Chip RAM on machine startup. The ECC has to be enabled before data is stored in memory otherwise the ECC will fail on reads.
---
v8: Address community comments on strings. Fix match strings based
on maintainer feedback. Update year in header.
v7: enable OCRAM ECC during platform init
v6: Implement OCRAM discovery changes from community. Add
of_node_put(). Remove be32_to_cpup(). Use __init() which allows removal of .init_machine(). Update year in header.
--- arch/arm/mach-socfpga/Makefile | 1 + arch/arm/mach-socfpga/core.h | 1 + arch/arm/mach-socfpga/ocram.c | 50 +++++++++++++++++++++++++++++++++++++++ arch/arm/mach-socfpga/socfpga.c | 3 +++ 4 files changed, 55 insertions(+) create mode 100644 arch/arm/mach-socfpga/ocram.c
arch/arm/mach-socfpga/Makefile +1 -0
@@ -6,3 +6,4 @@ obj-y := socfpga.o obj-$(CONFIG_SMP) += headsmp.o platsmp.o obj-$(CONFIG_SOCFPGA_SUSPEND) += pm.o self-refresh.o obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o +obj-$(CONFIG_EDAC_ALTERA_OCRAM) += ocram.o
arch/arm/mach-socfpga/core.h +1 -0
@@ -37,6 +37,7 @@extern void socfpga_init_clocks (void );extern void socfpga_sysmgr_init (void );void socfpga_init_l2_ecc (void ); +void socfpga_init_ocram_ecc (void );extern void __iomem *sys_manager_base_addr;extern void __iomem *rst_manager_base_addr;
arch/arm/mach-socfpga/ocram.c new file +50 -0
50 lines not on this page · raw message
arch/arm/mach-socfpga/socfpga.c +3 -0
@@ -61,6 +61,9 @@ static void __init socfpga_init_irq(void)socfpga_sysmgr_init ();if (IS_ENABLED (CONFIG_EDAC_ALTERA_L2C ))socfpga_init_l2_ecc (); + +if (IS_ENABLED (CONFIG_EDAC_ALTERA_OCRAM )) +socfpga_init_ocram_ecc (); }static void socfpga_cyclone5_restart (enum reboot_mode mode,const char *cmd)
Signature · 1 line
1.7.9.5
Hi Thor,
Quoted 10 lines · tthayer at opensource.altera.com
From: Thor Thayer <[email protected]>
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.
Signed-off-by Thor Thayer <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>
You are sending a change authored by yourself for review, but you add Dinh's SoB, what's his role here?
See Documentation/SubmittingPatches "Sign your work".
[snip]
Quoted 19 lines
drivers/edac/altera_edac.c +19
-0+/* +* altr_edac_device_probe() +* This is a generic EDAC device driver that will support +* various Altera memory devices such as the L2 cache ECC and +* OCRAM ECC as well as the memories for other peripherals. +* Module specific initialization is done by passing the +* function index in the device tree. +*/ +static int altr_edac_device_probe (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci; +struct altr_edac_device_dev *drvdata; +struct resource *r; +int res =0 ; +struct device_node *np = pdev->dev.of_node; +char *ecc_name = (char *)np->name; +static int dev_instance; +struct dentry *debugfs; +
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 745
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+if (!devres_open_group (&pdev->dev,NULL ,GFP_KERNEL )) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to open devm\n" ); +return -ENOMEM ; + } + + r =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!r) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to get mem resource\n" );
Missing devres_release_group(&pdev->dev, NULL) on error path.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 752
Quoted 7 lines
drivers/edac/altera_edac.c +7
-0+return -ENODEV ; + } + +if (!devm_request_mem_region (&pdev->dev, r->start,resource_size (r), +dev_name (&pdev->dev))) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error requesting mem region\n" , ecc_name);
See above.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 762
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+return -EBUSY ; + } + + dci =edac_device_alloc_ctl_info (sizeof (*drvdata), ecc_name, +1 , ecc_name,1 ,0 ,NULL ,0 , + dev_instance++); + +if (!dci) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s: Unable to allocate EDAC device\n" , ecc_name);
See above.
Quoted 88 lines
drivers/edac/altera_edac.c +88
-0+return -ENOMEM ; + } + + drvdata = dci->pvt_info; + dci->dev = &pdev->dev; +platform_set_drvdata (pdev, dci); + drvdata->edac_dev_name = ecc_name; + + drvdata->base =devm_ioremap (&pdev->dev, r->start,resource_size (r)); +if (!drvdata->base) + goto err; + +/* Get driver specific data for this EDAC device */ + drvdata->data =of_match_node (altr_edac_device_of_match, np)->data; + +/* Check specific dependencies for the module */ +if (drvdata->data->setup) { + res = drvdata->data->setup (pdev, drvdata->base); +if (res <0 ) + goto err; + } + + drvdata->sb_irq =platform_get_irq (pdev,0 ); + res =devm_request_irq (&pdev->dev, drvdata->sb_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + drvdata->db_irq =platform_get_irq (pdev,1 ); + res =devm_request_irq (&pdev->dev, drvdata->db_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + dci->mod_name ="Altera ECC Manager" ; + dci->dev_name = drvdata->edac_dev_name; + + debugfs =edac_debugfs_create_dir (ecc_name); +if (debugfs) +altr_create_edacdev_dbgfs (dci, drvdata->data, debugfs); + +if (edac_device_add_device (dci)) + goto err; + +devres_close_group (&pdev->dev,NULL ); + +return 0 ; +err: +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error setting up EDAC device: %d\n" , ecc_name, res); +devres_release_group (&pdev->dev,NULL ); +edac_device_free_ctl_info (dci); + +return res; +} + +static int altr_edac_device_remove (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci =platform_get_drvdata (pdev); + +edac_device_del_device (&pdev->dev); +edac_device_free_ctl_info (dci); + +return 0 ; +} + +static struct platform_driver altr_edac_device_driver = { + .probe = altr_edac_device_probe, + .remove = altr_edac_device_remove, + .driver = { + .name ="altr_edac_device" , + .of_match_table = altr_edac_device_of_match, + }, +}; +module_platform_driver (altr_edac_device_driver); + +/*********************** OCRAM EDAC Device Functions *********************/ + +#ifdef CONFIG_EDAC_ALTERA_OCRAM + +static void *ocram_alloc_mem (size_t size,void **other) +{ +struct device_node *np; +struct gen_pool *gp; +void *sram_addr; +
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 860
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+ np =of_find_compatible_node (NULL ,NULL ,"altr,socfpga-ocram-ecc" ); +if (!np) +return NULL ; + + gp =of_gen_pool_get (np,"iram" ,0 ); +if (!gp) { +of_node_put (np); +return NULL ; + } +of_node_put (np);
gp = of_gen_pool_get(np, "iram", 0); of_node_put(np); if (!gp) return NULL;
version is better.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 866
Quoted 6 lines
drivers/edac/altera_edac.c +6
-0+ + sram_addr = (void *)gen_pool_alloc (gp, size /sizeof (size_t )); +if (!sram_addr) +return NULL ; + +memset (sram_addr,0 , size);
Potential memory corruption, you allocate (size / sizeof(size_t)) bytes and then write size bytes.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 876
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+wmb ();/* Ensure data is written out */ + + *other = gp;/* Remember this handle for freeing later */ + +return sram_addr; +} + +static void ocram_free_mem (void *p,size_t size,void *other) +{ +gen_pool_free ((struct gen_pool *)other, (u32 )p, size /sizeof (size_t ));
See a comment above.
Review
Quoted 2 lines
(quoted diff) +2
-0+} +
--
With best wishes,
Vladimir
Hi Vladimir,
Quoted 14 lines · Vladimir Zapolskiy
Hi Thor,
Quoted 10 lines · On 21.01.2016 19:34, [email protected] wrote:
From: Thor Thayer <[email protected]>
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.
Signed-off-by Thor Thayer <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>
Quoted 6 lines
You are sending a change authored by yourself for review, but you add Dinh's SoB, what's his role here?
See
Documentation/SubmittingPatches"Sign your work".[snip]
While I was working in a different group at Altera last year, Dinh submitted the previous patch revision so I kept his signed off by for continuity. I'll just use myself in the future. Thank you for clarifying.
Quoted 24 lines
Quoted 24 lines
drivers/edac/altera_edac.c +23
-0+/* +* altr_edac_device_probe() +* This is a generic EDAC device driver that will support +* various Altera memory devices such as the L2 cache ECC and +* OCRAM ECC as well as the memories for other peripherals. +* Module specific initialization is done by passing the +* function index in the device tree. +*/ +static int altr_edac_device_probe (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci; +struct altr_edac_device_dev *drvdata; +struct resource *r; +int res =0 ; +struct device_node *np = pdev->dev.of_node; +char *ecc_name = (char *)np->name; +static int dev_instance; +struct dentry *debugfs; + +if (!devres_open_group (&pdev->dev,NULL ,GFP_KERNEL )) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to open devm\n" ); +return -ENOMEM ;
Quoted 9 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 745
Quoted 6 lines
drivers/edac/altera_edac.c +6
-0+ } + + r =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!r) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to get mem resource\n" );Missing
devres_release_group(&pdev->dev, NULL)on error path.
Yes. Thank you.
Quoted 127 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 752
Quoted 7 lines
drivers/edac/altera_edac.c +7
-0+return -ENODEV ; + } + +if (!devm_request_mem_region (&pdev->dev, r->start,resource_size (r), +dev_name (&pdev->dev))) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error requesting mem region\n" , ecc_name);See above.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 762
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+return -EBUSY ; + } + + dci =edac_device_alloc_ctl_info (sizeof (*drvdata), ecc_name, +1 , ecc_name,1 ,0 ,NULL ,0 , + dev_instance++); + +if (!dci) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s: Unable to allocate EDAC device\n" , ecc_name);See above.
Quoted 88 lines
drivers/edac/altera_edac.c +88
-0+return -ENOMEM ; + } + + drvdata = dci->pvt_info; + dci->dev = &pdev->dev; +platform_set_drvdata (pdev, dci); + drvdata->edac_dev_name = ecc_name; + + drvdata->base =devm_ioremap (&pdev->dev, r->start,resource_size (r)); +if (!drvdata->base) + goto err; + +/* Get driver specific data for this EDAC device */ + drvdata->data =of_match_node (altr_edac_device_of_match, np)->data; + +/* Check specific dependencies for the module */ +if (drvdata->data->setup) { + res = drvdata->data->setup (pdev, drvdata->base); +if (res <0 ) + goto err; + } + + drvdata->sb_irq =platform_get_irq (pdev,0 ); + res =devm_request_irq (&pdev->dev, drvdata->sb_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + drvdata->db_irq =platform_get_irq (pdev,1 ); + res =devm_request_irq (&pdev->dev, drvdata->db_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + dci->mod_name ="Altera ECC Manager" ; + dci->dev_name = drvdata->edac_dev_name; + + debugfs =edac_debugfs_create_dir (ecc_name); +if (debugfs) +altr_create_edacdev_dbgfs (dci, drvdata->data, debugfs); + +if (edac_device_add_device (dci)) + goto err; + +devres_close_group (&pdev->dev,NULL ); + +return 0 ; +err: +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error setting up EDAC device: %d\n" , ecc_name, res); +devres_release_group (&pdev->dev,NULL ); +edac_device_free_ctl_info (dci); + +return res; +} + +static int altr_edac_device_remove (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci =platform_get_drvdata (pdev); + +edac_device_del_device (&pdev->dev); +edac_device_free_ctl_info (dci); + +return 0 ; +} + +static struct platform_driver altr_edac_device_driver = { + .probe = altr_edac_device_probe, + .remove = altr_edac_device_remove, + .driver = { + .name ="altr_edac_device" , + .of_match_table = altr_edac_device_of_match, + }, +}; +module_platform_driver (altr_edac_device_driver); + +/*********************** OCRAM EDAC Device Functions *********************/ + +#ifdef CONFIG_EDAC_ALTERA_OCRAM + +static void *ocram_alloc_mem (size_t size,void **other) +{ +struct device_node *np; +struct gen_pool *gp; +void *sram_addr; +drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 860
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+ np =of_find_compatible_node (NULL ,NULL ,"altr,socfpga-ocram-ecc" ); +if (!np) +return NULL ; + + gp =of_gen_pool_get (np,"iram" ,0 ); +if (!gp) { +of_node_put (np); +return NULL ; + } +of_node_put (np);gp = of_gen_pool_get(np, "iram", 0); of_node_put(np); if (!gp) return NULL;
Quoted 2 lines
version is better.
I'm sorry, can you elaborate? Are you saying I should return something other than NULL?
Quoted 10 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 866
Quoted 6 lines
drivers/edac/altera_edac.c +6
-0+ + sram_addr = (void *)gen_pool_alloc (gp, size /sizeof (size_t )); +if (!sram_addr) +return NULL ; + +memset (sram_addr,0 , size);Potential memory corruption, you allocate (size /
sizeof(size_t)) bytes and then write size bytes.
Yes, good catch. I thought I'd fixed this but missed it when I came back.
Quoted 13 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 876
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+wmb ();/* Ensure data is written out */ + + *other = gp;/* Remember this handle for freeing later */ + +return sram_addr; +} + +static void ocram_free_mem (void *p,size_t size,void *other) +{ +gen_pool_free ((struct gen_pool *)other, (u32 )p, size /sizeof (size_t ));See a comment above.
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+} +--
With best wishes,
Vladimir
Thank you for reviewing.
Hi Thor,
Quoted 21 lines · Thor Thayer
Hi Vladimir,
Quoted 17 lines · Vladimir Zapolskiy
Hi Thor,
Quoted 10 lines · tthayer at opensource.altera.com
From: Thor Thayer <[email protected]>
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.
Signed-off-by Thor Thayer <[email protected]>Signed-off-by Dinh Nguyen <[email protected]>You are sending a change authored by yourself for review, but you add Dinh's SoB, what's his role here?
Quoted 7 lines
Quoted 3 lines
See
Documentation/SubmittingPatches"Sign your work".[snip]
While I was working in a different group at Altera last year, Dinh submitted the previous patch revision so I kept his signed off by for continuity. I'll just use myself in the future. Thank you for clarifying.
it sounds like the author of the original change is Dinh, but if you agreed about authorship transfer, then "From: Thor Thayer" statement should be correct, but in any case your SoB should follow Dinh's SoB, if you decide to keep the latter one.
This consideration may apply to the other changes in the changeset as well.
Quoted 162 lines
Quoted 24 lines
Quoted 24 lines
drivers/edac/altera_edac.c +23
-0+/* +* altr_edac_device_probe() +* This is a generic EDAC device driver that will support +* various Altera memory devices such as the L2 cache ECC and +* OCRAM ECC as well as the memories for other peripherals. +* Module specific initialization is done by passing the +* function index in the device tree. +*/ +static int altr_edac_device_probe (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci; +struct altr_edac_device_dev *drvdata; +struct resource *r; +int res =0 ; +struct device_node *np = pdev->dev.of_node; +char *ecc_name = (char *)np->name; +static int dev_instance; +struct dentry *debugfs; + +if (!devres_open_group (&pdev->dev,NULL ,GFP_KERNEL )) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to open devm\n" ); +return -ENOMEM ;Quoted 9 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 745
Quoted 6 lines
drivers/edac/altera_edac.c +6
-0+ } + + r =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!r) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"Unable to get mem resource\n" );Missing
devres_release_group(&pdev->dev, NULL)on error path.Yes. Thank you.
Quoted 127 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 752
Quoted 7 lines
drivers/edac/altera_edac.c +7
-0+return -ENODEV ; + } + +if (!devm_request_mem_region (&pdev->dev, r->start,resource_size (r), +dev_name (&pdev->dev))) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error requesting mem region\n" , ecc_name);See above.
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 762
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+return -EBUSY ; + } + + dci =edac_device_alloc_ctl_info (sizeof (*drvdata), ecc_name, +1 , ecc_name,1 ,0 ,NULL ,0 , + dev_instance++); + +if (!dci) { +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s: Unable to allocate EDAC device\n" , ecc_name);See above.
Quoted 88 lines
drivers/edac/altera_edac.c +88
-0+return -ENOMEM ; + } + + drvdata = dci->pvt_info; + dci->dev = &pdev->dev; +platform_set_drvdata (pdev, dci); + drvdata->edac_dev_name = ecc_name; + + drvdata->base =devm_ioremap (&pdev->dev, r->start,resource_size (r)); +if (!drvdata->base) + goto err; + +/* Get driver specific data for this EDAC device */ + drvdata->data =of_match_node (altr_edac_device_of_match, np)->data; + +/* Check specific dependencies for the module */ +if (drvdata->data->setup) { + res = drvdata->data->setup (pdev, drvdata->base); +if (res <0 ) + goto err; + } + + drvdata->sb_irq =platform_get_irq (pdev,0 ); + res =devm_request_irq (&pdev->dev, drvdata->sb_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + drvdata->db_irq =platform_get_irq (pdev,1 ); + res =devm_request_irq (&pdev->dev, drvdata->db_irq, + altr_edac_device_handler, +0 ,dev_name (&pdev->dev), dci); +if (res <0 ) + goto err; + + dci->mod_name ="Altera ECC Manager" ; + dci->dev_name = drvdata->edac_dev_name; + + debugfs =edac_debugfs_create_dir (ecc_name); +if (debugfs) +altr_create_edacdev_dbgfs (dci, drvdata->data, debugfs); + +if (edac_device_add_device (dci)) + goto err; + +devres_close_group (&pdev->dev,NULL ); + +return 0 ; +err: +edac_printk (KERN_ERR ,EDAC_DEVICE , +"%s:Error setting up EDAC device: %d\n" , ecc_name, res); +devres_release_group (&pdev->dev,NULL ); +edac_device_free_ctl_info (dci); + +return res; +} + +static int altr_edac_device_remove (struct platform_device *pdev) +{ +struct edac_device_ctl_info *dci =platform_get_drvdata (pdev); + +edac_device_del_device (&pdev->dev); +edac_device_free_ctl_info (dci); + +return 0 ; +} + +static struct platform_driver altr_edac_device_driver = { + .probe = altr_edac_device_probe, + .remove = altr_edac_device_remove, + .driver = { + .name ="altr_edac_device" , + .of_match_table = altr_edac_device_of_match, + }, +}; +module_platform_driver (altr_edac_device_driver); + +/*********************** OCRAM EDAC Device Functions *********************/ + +#ifdef CONFIG_EDAC_ALTERA_OCRAM + +static void *ocram_alloc_mem (size_t size,void **other) +{ +struct device_node *np; +struct gen_pool *gp; +void *sram_addr; +drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 860
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+ np =of_find_compatible_node (NULL ,NULL ,"altr,socfpga-ocram-ecc" ); +if (!np) +return NULL ; + + gp =of_gen_pool_get (np,"iram" ,0 ); +if (!gp) { +of_node_put (np); +return NULL ; + } +of_node_put (np);gp = of_gen_pool_get(np, "iram", 0); of_node_put(np); if (!gp) return NULL;
Quoted 5 lines
Quoted 2 lines
version is better.
I'm sorry, can you elaborate? Are you saying I should return something other than NULL?
Here I propose to do a tiny code optimization, 4 lines vs yours 6 lines of code without any functional difference.
Quoted 26 lines
Quoted 10 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 866
Quoted 6 lines
drivers/edac/altera_edac.c +6
-0+ + sram_addr = (void *)gen_pool_alloc (gp, size /sizeof (size_t )); +if (!sram_addr) +return NULL ; + +memset (sram_addr,0 , size);Potential memory corruption, you allocate (size /
sizeof(size_t)) bytes and then write size bytes.Yes, good catch. I thought I'd fixed this but missed it when I came back.
Quoted 13 lines
drivers/edac/altera_edac.c static struct platform_driver altr_sdram_edac_driver = { · line 876
Quoted 10 lines
drivers/edac/altera_edac.c +10
-0+wmb ();/* Ensure data is written out */ + + *other = gp;/* Remember this handle for freeing later */ + +return sram_addr; +} + +static void ocram_free_mem (void *p,size_t size,void *other) +{ +gen_pool_free ((struct gen_pool *)other, (u32 )p, size /sizeof (size_t ));See a comment above.
Quoted 7 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+} +Thank you for reviewing.
No problem :)
--
With best wishes,
Vladimir
Borislav Petkov
maintainerQuoted 6 lines · Vladimir Zapolskiy
it sounds like the author of the original change is Dinh, but if you agreed about authorship transfer, then "From: Thor Thayer" statement should be correct, but in any case your SoB should follow Dinh's SoB, if you decide to keep the latter one.
This consideration may apply to the other changes in the changeset as well.
So the patch author should be in the From:
If Thor has changed the original patch considerably, then you Thor and Dinh could decide amongst each other who should be the author.
If Thor becomes the author and lands in From:, then the commit message could state something like "based on original work from Dinh" or "Originally-from: Dinh" and so on. "git log" has some examples.
The SOB chain shows who handled the patch on its way upstream. So in this case, it should be:
SOB: Dinh (if From: is Dinh - otherwise Originally-by:)
SOB: Thor
SOB: Boris
if I'm going to pick it up and send it to Linus.
Ok?
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Quoted 20 lines · Borislav Petkov
Quoted 6 lines · Vladimir Zapolskiy
it sounds like the author of the original change is Dinh, but if you agreed about authorship transfer, then "From: Thor Thayer" statement should be correct, but in any case your SoB should follow Dinh's SoB, if you decide to keep the latter one.
This consideration may apply to the other changes in the changeset as well.
So the patch author should be in the From:
If Thor has changed the original patch considerably, then you Thor and Dinh could decide amongst each other who should be the author.
If Thor becomes the author and lands in From:, then the commit message could state something like "based on original work from Dinh" or "Originally-from: Dinh" and so on. "git log" has some examples.
The SOB chain shows who handled the patch on its way upstream. So in this case, it should be:
Quoted 8 lines
SOB: Dinh (if From: is Dinh - otherwise Originally-by:)
SOB: Thor
SOB: Borisif I'm going to pick it up and send it to Linus.
Ok?
OK. Thank you for the explanation, it is much clearer now. I apologize for creating this confusion.
I was the original author for the previous versions of patches (1-6) but since I was out for a portion of last year, Dinh picked up version 7 and submitted it.
Since I'm taking it back over, Dinh and I agreed that I'll be the only signoff from now on. Additionally, I'll make sure I'm the From: going forward.
Thor