[PATCHv2 0/3] Altera EDAC Fix & Cleanup Patches

[PATCHv2 0/3] Altera EDAC Fix & Cleanup Patches

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

This patchset includes a fix for the Stratix10 IRQ regmap
and cleanup of the driver as it has evolved.

Patch 2 and 3 were accepted into the edac-for-next branch [1]
but the fix should be included. Version 2 of the patches
rebase on top of the fix.

This patchset should replace the current patches[1].

[1] https://lore.kernel.org/linux-edac/1573156890-26891-1-git-send-email-thor.thayer@linux.intel.com/

Meng Li (1):
  EDAC/altera: Use fast register IO for S10 IRQs

Thor Thayer (2):
  EDAC/altera: Cleanup the ECC Manager
  EDAC/altera: Use the Altera System Manager driver

 drivers/edac/altera_edac.c | 152 +++------------------------------------------
 1 file changed, 9 insertions(+), 143 deletions(-)

-- 
2.7.4

[PATCHv2 1/3] EDAC/altera: Use fast register IO for S10 IRQs

[thor.thayer]

From: Meng Li <Meng.Li@windriver.com>

When an irq occurs in altera edac driver, regmap_xxx() is invoked
in atomic context. Regmap must indicate register IO is fast so
that a spinlock is used instead of a mutex to avoid sleeping
in atomic context.

Fixes mutex-lock error
   lock_acquire+0xfc/0x288
   __mutex_lock+0x8c/0x808
   mutex_lock_nested+0x3c/0x50
   regmap_lock_mutex+0x24/0x30
   regmap_write+0x40/0x78
   a10_eccmgr_irq_unmask+0x34/0x40
   unmask_irq.part.0+0x30/0x50
   irq_enable+0x74/0x80
   __irq_startup+0x80/0xa8
   irq_startup+0x70/0x150
   __setup_irq+0x650/0x6d0
   request_threaded_irq+0xe4/0x180
   devm_request_threaded_irq+0x7c/0xf0
   altr_sdram_probe+0x2c4/0x600
<snip>

Upstream fix pending [1] (common code uses fast mode)
[1] https://lkml.org/lkml/2019/11/7/1014

Fixes: 3dab6bd52687 ("EDAC, altera: Add support for Stratix10 SDRAM EDAC")
Cc: stable@vger.kernel.org
Reported-by: Meng Li <Meng.Li@windriver.com>
Signed-off-by: Meng Li <Meng.Li@windriver.com>
Reviewed-by: Thor Thayer <thor.thayer@linux.intel.com>
---
v2 Change Author to Meng Li & Reviewed-by: Thor Thayer
---
 drivers/edac/altera_edac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index fbda4b876afd..0be3d1b17f03 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -560,6 +560,7 @@ static const struct regmap_config s10_sdram_regmap_cfg = {
 	.reg_write = s10_protected_reg_write,
 	.use_single_read = true,
 	.use_single_write = true,
+	.fast_io = true,
 };
 
 /************** </Stratix10 EDAC Memory Controller Functions> ***********/
-- 
2.7.4

Re: [PATCHv2 1/3] EDAC/altera: Use fast register IO for S10 IRQs

[Borislav Petkov]

On Thu, Nov 21, 2019 at 12:30:46PM -0600, thor.thayer@linux.intel.com wrote:
> From: Meng Li <Meng.Li@windriver.com>
> 
> When an irq occurs in altera edac driver, regmap_xxx() is invoked
> in atomic context. Regmap must indicate register IO is fast so
> that a spinlock is used instead of a mutex to avoid sleeping
> in atomic context.
> 
> Fixes mutex-lock error
>    lock_acquire+0xfc/0x288
>    __mutex_lock+0x8c/0x808
>    mutex_lock_nested+0x3c/0x50
>    regmap_lock_mutex+0x24/0x30
>    regmap_write+0x40/0x78
>    a10_eccmgr_irq_unmask+0x34/0x40
>    unmask_irq.part.0+0x30/0x50
>    irq_enable+0x74/0x80
>    __irq_startup+0x80/0xa8
>    irq_startup+0x70/0x150
>    __setup_irq+0x650/0x6d0
>    request_threaded_irq+0xe4/0x180
>    devm_request_threaded_irq+0x7c/0xf0
>    altr_sdram_probe+0x2c4/0x600
> <snip>
> 
> Upstream fix pending [1] (common code uses fast mode)
> [1] https://lkml.org/lkml/2019/11/7/1014
> 
> Fixes: 3dab6bd52687 ("EDAC, altera: Add support for Stratix10 SDRAM EDAC")
> Cc: stable@vger.kernel.org
> Reported-by: Meng Li <Meng.Li@windriver.com>
> Signed-off-by: Meng Li <Meng.Li@windriver.com>
> Reviewed-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
> v2 Change Author to Meng Li & Reviewed-by: Thor Thayer

You don't absolutely need to have Reviewed-by: you, when you send
someone else's patch. The fact that you send it, kinda implies you've
reviewed it. I sure hope so, at least :-)

What the patch must have is your SOB unterneath. I'll fix that up when
applying.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCHv2 2/3] EDAC/altera: Cleanup the ECC Manager

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Cleanup the ECC Manager peripheral test in probe function as suggested
by James. Remove the check for Stratix10.

Suggested-by: James Morse <james.morse@arm.com>
Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
V2 No change
---
 drivers/edac/altera_edac.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 0be3d1b17f03..3e86cf327ad0 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1014,11 +1014,6 @@ static int socfpga_is_a10(void)
 	return of_machine_is_compatible("altr,socfpga-arria10");
 }
 
-static int socfpga_is_s10(void)
-{
-	return of_machine_is_compatible("altr,socfpga-stratix10");
-}
-
 static __init int __maybe_unused
 altr_init_a10_ecc_block(struct device_node *np, u32 irq_mask,
 			u32 ecc_ctrl_en_mask, bool dual_port)
@@ -1126,9 +1121,6 @@ static int __init __maybe_unused altr_init_a10_ecc_device_type(char *compat)
 	int irq;
 	struct device_node *child, *np;
 
-	if (!socfpga_is_a10() && !socfpga_is_s10())
-		return -ENODEV;
-
 	np = of_find_compatible_node(NULL, NULL,
 				     "altr,socfpga-a10-ecc-manager");
 	if (!np) {
@@ -2271,18 +2263,7 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
 		if (!of_device_is_available(child))
 			continue;
 
-		if (of_device_is_compatible(child, "altr,socfpga-a10-l2-ecc") || 
-		    of_device_is_compatible(child, "altr,socfpga-a10-ocram-ecc") ||
-		    of_device_is_compatible(child, "altr,socfpga-eth-mac-ecc") ||
-		    of_device_is_compatible(child, "altr,socfpga-nand-ecc") ||
-		    of_device_is_compatible(child, "altr,socfpga-dma-ecc") ||
-		    of_device_is_compatible(child, "altr,socfpga-usb-ecc") ||
-		    of_device_is_compatible(child, "altr,socfpga-qspi-ecc") ||
-#ifdef CONFIG_EDAC_ALTERA_SDRAM
-		    of_device_is_compatible(child, "altr,sdram-edac-s10") ||
-#endif
-		    of_device_is_compatible(child, "altr,socfpga-sdmmc-ecc"))
-
+		if (of_match_node(altr_edac_a10_device_of_match, child))
 			altr_edac_a10_device_add(edac, child);
 
 #ifdef CONFIG_EDAC_ALTERA_SDRAM
-- 
2.7.4

[PATCHv2 3/3] EDAC/altera: Use the Altera System Manager driver

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

Simplify the EDAC file by using the Altera System Manager driver that
abstracts the differences between ARM32 and ARM64. Also allows the
removal of the Arria10 test function since this is handled by the System
Manager driver.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
V2 Rebase on top of fast IO fix.
---
 drivers/edac/altera_edac.c | 132 +++------------------------------------------
 1 file changed, 8 insertions(+), 124 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 3e86cf327ad0..e91cf1147a4e 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/kernel.h>
+#include <linux/mfd/altera-sysmgr.h>
 #include <linux/mfd/syscon.h>
 #include <linux/notifier.h>
 #include <linux/of_address.h>
@@ -275,7 +276,6 @@ static int a10_unmask_irq(struct platform_device *pdev, u32 mask)
 	return ret;
 }
 
-static int socfpga_is_a10(void);
 static int altr_sdram_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *id;
@@ -399,7 +399,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
 		goto err;
 
 	/* Only the Arria10 has separate IRQs */
-	if (socfpga_is_a10()) {
+	if (of_machine_is_compatible("altr,socfpga-arria10")) {
 		/* Arria10 specific initialization */
 		res = a10_init(mc_vbase);
 		if (res < 0)
@@ -502,69 +502,6 @@ module_platform_driver(altr_sdram_edac_driver);
 
 #endif	/* CONFIG_EDAC_ALTERA_SDRAM */
 
-/**************** Stratix 10 EDAC Memory Controller Functions ************/
-
-/**
- * s10_protected_reg_write
- * Write to a protected SMC register.
- * @context: Not used.
- * @reg: Address of register
- * @value: Value to write
- * Return: INTEL_SIP_SMC_STATUS_OK (0) on success
- *	   INTEL_SIP_SMC_REG_ERROR on error
- *	   INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION if not supported
- */
-static int s10_protected_reg_write(void *context, unsigned int reg,
-				   unsigned int val)
-{
-	struct arm_smccc_res result;
-	unsigned long offset = (unsigned long)context;
-
-	arm_smccc_smc(INTEL_SIP_SMC_REG_WRITE, offset + reg, val, 0, 0,
-		      0, 0, 0, &result);
-
-	return (int)result.a0;
-}
-
-/**
- * s10_protected_reg_read
- * Read the status of a protected SMC register
- * @context: Not used.
- * @reg: Address of register
- * @value: Value read.
- * Return: INTEL_SIP_SMC_STATUS_OK (0) on success
- *	   INTEL_SIP_SMC_REG_ERROR on error
- *	   INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION if not supported
- */
-static int s10_protected_reg_read(void *context, unsigned int reg,
-				  unsigned int *val)
-{
-	struct arm_smccc_res result;
-	unsigned long offset = (unsigned long)context;
-
-	arm_smccc_smc(INTEL_SIP_SMC_REG_READ, offset + reg, 0, 0, 0,
-		      0, 0, 0, &result);
-
-	*val = (unsigned int)result.a1;
-
-	return (int)result.a0;
-}
-
-static const struct regmap_config s10_sdram_regmap_cfg = {
-	.name = "s10_ddr",
-	.reg_bits = 32,
-	.reg_stride = 4,
-	.val_bits = 32,
-	.max_register = 0xffd12228,
-	.reg_read = s10_protected_reg_read,
-	.reg_write = s10_protected_reg_write,
-	.use_single_read = true,
-	.use_single_write = true,
-	.fast_io = true,
-};
-
-/************** </Stratix10 EDAC Memory Controller Functions> ***********/
-
 /************************* EDAC Parent Probe *************************/
 
 static const struct of_device_id altr_edac_device_of_match[];
@@ -1009,11 +946,6 @@ static int __maybe_unused altr_init_memory_port(void __iomem *ioaddr, int port)
 	return ret;
 }
 
-static int socfpga_is_a10(void)
-{
-	return of_machine_is_compatible("altr,socfpga-arria10");
-}
-
 static __init int __maybe_unused
 altr_init_a10_ecc_block(struct device_node *np, u32 irq_mask,
 			u32 ecc_ctrl_en_mask, bool dual_port)
@@ -1029,34 +961,10 @@ altr_init_a10_ecc_block(struct device_node *np, u32 irq_mask,
 	/* Get the ECC Manager - parent of the device EDACs */
 	np_eccmgr = of_get_parent(np);
 
-	if (socfpga_is_a10()) {
-		ecc_mgr_map = syscon_regmap_lookup_by_phandle(np_eccmgr,
-							      "altr,sysmgr-syscon");
-	} else {
-		struct device_node *sysmgr_np;
-		struct resource res;
-		uintptr_t base;
-
-		sysmgr_np = of_parse_phandle(np_eccmgr,
-					     "altr,sysmgr-syscon", 0);
-		if (!sysmgr_np) {
-			edac_printk(KERN_ERR, EDAC_DEVICE,
-				    "Unable to find altr,sysmgr-syscon\n");
-			return -ENODEV;
-		}
+	ecc_mgr_map =
+		altr_sysmgr_regmap_lookup_by_phandle(np_eccmgr,
+						     "altr,sysmgr-syscon");
 
-		if (of_address_to_resource(sysmgr_np, 0, &res)) {
-			of_node_put(sysmgr_np);
-			return -ENOMEM;
-		}
-
-		/* Need physical address for SMCC call */
-		base = res.start;
-
-		ecc_mgr_map = regmap_init(NULL, NULL, (void *)base,
-					  &s10_sdram_regmap_cfg);
-		of_node_put(sysmgr_np);
-	}
 	of_node_put(np_eccmgr);
 	if (IS_ERR(ecc_mgr_map)) {
 		edac_printk(KERN_ERR, EDAC_DEVICE,
@@ -2171,33 +2079,9 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, edac);
 	INIT_LIST_HEAD(&edac->a10_ecc_devices);
 
-	if (socfpga_is_a10()) {
-		edac->ecc_mgr_map =
-			syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
-							"altr,sysmgr-syscon");
-	} else {
-		struct device_node *sysmgr_np;
-		struct resource res;
-		uintptr_t base;
-
-		sysmgr_np = of_parse_phandle(pdev->dev.of_node,
-					     "altr,sysmgr-syscon", 0);
-		if (!sysmgr_np) {
-			edac_printk(KERN_ERR, EDAC_DEVICE,
-				    "Unable to find altr,sysmgr-syscon\n");
-			return -ENODEV;
-		}
-
-		if (of_address_to_resource(sysmgr_np, 0, &res))
-			return -ENOMEM;
-
-		/* Need physical address for SMCC call */
-		base = res.start;
-
-		edac->ecc_mgr_map = devm_regmap_init(&pdev->dev, NULL,
-						     (void *)base,
-						     &s10_sdram_regmap_cfg);
-	}
+	edac->ecc_mgr_map =
+		altr_sysmgr_regmap_lookup_by_phandle(pdev->dev.of_node,
+						     "altr,sysmgr-syscon");
 
 	if (IS_ERR(edac->ecc_mgr_map)) {
 		edac_printk(KERN_ERR, EDAC_DEVICE,
-- 
2.7.4

Re: [PATCHv2 0/3] Altera EDAC Fix & Cleanup Patches

[Borislav Petkov]

On Thu, Nov 21, 2019 at 12:30:45PM -0600, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> This patchset includes a fix for the Stratix10 IRQ regmap
> and cleanup of the driver as it has evolved.
> 
> Patch 2 and 3 were accepted into the edac-for-next branch [1]
> but the fix should be included. Version 2 of the patches
> rebase on top of the fix.
> 
> This patchset should replace the current patches[1].
> 
> [1] https://lore.kernel.org/linux-edac/1573156890-26891-1-git-send-email-thor.thayer@linux.intel.com/
> 
> Meng Li (1):
>   EDAC/altera: Use fast register IO for S10 IRQs
> 
> Thor Thayer (2):
>   EDAC/altera: Cleanup the ECC Manager
>   EDAC/altera: Use the Altera System Manager driver
> 
>  drivers/edac/altera_edac.c | 152 +++------------------------------------------
>  1 file changed, 9 insertions(+), 143 deletions(-)
> 
> -- 

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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