OMAP: l3: fixes and cleanup

PATCH New Omar Ramirez Luna 2 participants 10 messages Mar 29, 2011 – Mar 31, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #0 lore raw

cover OMAP: l3: fixes and cleanup

Based on the comments received for the first patch:
OMAP3: l3: fix for "irq 10: nobody cared" message[1], and quick skimming through the code.

Although there are still parenthesis that are not needed because of operator precedence, they were left for clarity.

E.g.:
(error >> 32) & 0xffff;

clear = (L3_AGENT_STATUS_CLEAR_IA << int_type) |

        L3_AGENT_STATUS_CLEAR_TA;

---
[1] https://patchwork.kernel.org/patch/665141/

Omar Ramirez Luna (5):
OMAP3: l3: fix for "irq 10: nobody cared" message
OMAP3: l3: fix omap3_l3_probe error path
OMAP3: l3: minor cleanup for error message, parenthesis and extra
lines
OMAP4: l3: fix omap4_l3_probe error path
OMAP4: l3: minor cleanup for parenthesis and extra spaces

 arch/arm/mach-omap2/omap_l3_noc.c |   51 ++++++++++++++++--------------------
 arch/arm/mach-omap2/omap_l3_smx.c |   46 ++++++++++++---------------------
 2 files changed, 40 insertions(+), 57 deletions(-)

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #1 lore raw

PATCH 1/5 OMAP3: l3: fix for "irq 10: nobody cared" message

If an error occurs in the L3 on any other initiator than MPU, the interrupt goes unhandled given that the 'base' register was calculated with the initialized err_source value (which coincidentally points to MPU) and not with the actual source of the error.

Removed parenthesis that are not needed for the touched lines.

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 arch/arm/mach-omap2/omap_l3_smx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/omap_l3_smx.c +2 -2

index 5f2da75..4321e79 100644
@@ -196,11 +196,11 @@ static irqreturn_t omap3_l3_app_irq(int irq, void *_l3)
 		/* No timeout error for debug sources */
 	}
 
-	base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source)));
-
 	/* identify the error source */
 	for (err_source = 0; !(status & (1 << err_source)); err_source++)
 									;
+
+	base = l3->rt + *(omap3_l3_bases[int_type] + err_source);
 	error = omap3_l3_readll(base, L3_ERROR_LOG);
 
 	if (error) {

Signature · 1 line

1.7.1

Santosh Shilimkar

2011-03-30 06:27 UTC #2 lore raw

PATCH 1/5 OMAP3: l3: fix for "irq 10: nobody cared" message

Quoted 10 lines · Omar Ramirez Luna

If an error occurs in the L3 on any other initiator than MPU, the interrupt goes unhandled given that the 'base' register was calculated with the initialized err_source value (which coincidentally points to MPU) and not with the actual source of the error.
Removed parenthesis that are not needed for the touched lines.
Signed-off-by Omar Ramirez Luna<[email protected]>
---

Acked-by Santosh Shilimkar <[email protected]>

Quoted 21 lines

  arch/arm/mach-omap2/omap_l3_smx.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/omap_l3_smx.c +2 -2

index 5f2da75..4321e79 100644
@@ -196,11 +196,11 @@ static irqreturn_t omap3_l3_app_irq(int irq, void *_l3)
  		/* No timeout error for debug sources */
  	}
 
-	base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source)));
-
  	/* identify the error source */
  	for (err_source = 0; !(status&  (1<<  err_source)); err_source++)
  									;
+
+	base = l3->rt + *(omap3_l3_bases[int_type] + err_source);
  	error = omap3_l3_readll(base, L3_ERROR_LOG);
 
  	if (error) {

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #3 lore raw

PATCH 2/5 OMAP3: l3: fix omap3_l3_probe error path

In reply to Omar Ramirez Luna

Add missing free_irq and remove an empty goto label.

Safe to assume that if we reached the end point of execution without errors, then return value is 0, so replacing instead another goto.

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 arch/arm/mach-omap2/omap_l3_smx.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/omap_l3_smx.c +10 ~~-13~~

index 4321e79..32961b1 100644
@@ -228,10 +228,8 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 	int                     ret;
 
 	l3 = kzalloc(sizeof(*l3), GFP_KERNEL);
-	if (!l3) {
-		ret = -ENOMEM;
-		goto err0;
-	}
+	if (!l3)
+		return -ENOMEM;
 
 	platform_set_drvdata(pdev, l3);

@@ -239,13 +237,13 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 	if (!res) {
 		dev_err(&pdev->dev, "couldn't find resource\n");
 		ret = -ENODEV;
-		goto err1;
+		goto err0;
 	}
 	l3->rt = ioremap(res->start, resource_size(res));
 	if (!(l3->rt)) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
-		goto err2;
+		goto err0;
 	}
 
 	l3->debug_irq = platform_get_irq(pdev, 0);

@@ -254,7 +252,7 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 		"l3-debug-irq", l3);
 	if (ret) {
 		dev_err(&pdev->dev, "couldn't request debug irq\n");
-		goto err3;
+		goto err1;
 	}
 
 	l3->app_irq = platform_get_irq(pdev, 1);

@@ -264,18 +262,17 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 
 	if (ret) {
 		dev_err(&pdev->dev, "couldn't request app irq\n");
-		goto err4;
+		goto err2;
 	}
 
-	goto err0;
+	return 0;
 
-err4:
-err3:
-	iounmap(l3->rt);
 err2:
+	free_irq(l3->debug_irq, l3);
 err1:
-	kfree(l3);
+	iounmap(l3->rt);
 err0:
+	kfree(l3);
 	return ret;
 }

Signature · 1 line

1.7.1

Santosh Shilimkar

2011-03-30 06:36 UTC #4 lore raw

PATCH 2/5 OMAP3: l3: fix omap3_l3_probe error path

Quoted 8 lines · Omar Ramirez Luna

Add missing free_irq and remove an empty goto label.
Safe to assume that if we reached the end point of execution without errors, then return value is 0, so replacing instead another goto.
Signed-off-by Omar Ramirez Luna<[email protected]>
---

The free_irq() change looks good. Other changes are not necessary though because its is personnel choice to return from one place or from every error path.

In both forms ...

Acked-by Santosh Shilimkar <[email protected]>

Quoted 69 lines

  arch/arm/mach-omap2/omap_l3_smx.c |   23 ++++++++++-------------
  1 files changed, 10 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/omap_l3_smx.c +10 ~~-13~~

index 4321e79..32961b1 100644
@@ -228,10 +228,8 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
  	int                     ret;
 
  	l3 = kzalloc(sizeof(*l3), GFP_KERNEL);
-	if (!l3) {
-		ret = -ENOMEM;
-		goto err0;
-	}
+	if (!l3)
+		return -ENOMEM;
 
  	platform_set_drvdata(pdev, l3);

@@ -239,13 +237,13 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
  	if (!res) {
  		dev_err(&pdev->dev, "couldn't find resource\n");
  		ret = -ENODEV;
-		goto err1;
+		goto err0;
  	}
  	l3->rt = ioremap(res->start, resource_size(res));
  	if (!(l3->rt)) {
  		dev_err(&pdev->dev, "ioremap failed\n");
  		ret = -ENOMEM;
-		goto err2;
+		goto err0;
  	}
 
  	l3->debug_irq = platform_get_irq(pdev, 0);

@@ -254,7 +252,7 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
  		"l3-debug-irq", l3);
  	if (ret) {
  		dev_err(&pdev->dev, "couldn't request debug irq\n");
-		goto err3;
+		goto err1;
  	}
 
  	l3->app_irq = platform_get_irq(pdev, 1);

@@ -264,18 +262,17 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 
  	if (ret) {
  		dev_err(&pdev->dev, "couldn't request app irq\n");
-		goto err4;
+		goto err2;
  	}
 
-	goto err0;
+	return 0;
 
-err4:
-err3:
-	iounmap(l3->rt);
  err2:
+	free_irq(l3->debug_irq, l3);
  err1:
-	kfree(l3);
+	iounmap(l3->rt);
  err0:
+	kfree(l3);
  	return ret;
  }

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #5 lore raw

PATCH 3/5 OMAP3: l3: minor cleanup for error message, parenthesis and extra lines

In reply to Omar Ramirez Luna

Removing extra lines and unneeded parenthesis.

While at it collapsing an if statement to detect the type of error based on the irq received; also fixing the message printed on error.

So now: looks like:
"No Error Error seen..." "No Error seen..."
"In-Band Error Error seen..." "In-Band Error seen..."

Other messages are self explanatory, no "Error" added to them. (E.g.: "Address Hole seen...")

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 arch/arm/mach-omap2/omap_l3_smx.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

arch/arm/mach-omap2/omap_l3_smx.c +5 ~~-14~~

index 32961b1..873c0e3 100644
@@ -155,7 +155,7 @@ static irqreturn_t omap3_l3_block_irq(struct omap3_l3 *l3,
 	u8                      multi = error & L3_ERROR_LOG_MULTI;
 	u32			address = omap3_l3_decode_addr(error_addr);
 
-	WARN(true, "%s Error seen by %s %s at address %x\n",
+	WARN(true, "%s seen by %s %s at address %x\n",
 				 omap3_l3_code_string(code),
 			  omap3_l3_initiator_string(initid),
 			     multi ? "Multiple Errors" : "",

@@ -167,21 +167,15 @@ static irqreturn_t omap3_l3_block_irq(struct omap3_l3 *l3,
 static irqreturn_t omap3_l3_app_irq(int irq, void *_l3)
 {
 	struct omap3_l3         *l3 = _l3;
-
 	u64                     status, clear;
 	u64                     error;
 	u64			error_addr;
 	u64			err_source = 0;
 	void			__iomem *base;
 	int			int_type;
-
 	irqreturn_t             ret = IRQ_NONE;
 
-	if (irq == l3->app_irq)
-		int_type = L3_APPLICATION_ERROR;
-	else
-		int_type = L3_DEBUG_ERROR;
-
+	int_type = irq == l3->app_irq ? L3_APPLICATION_ERROR : L3_DEBUG_ERROR;
 	if (!int_type) {
 		status = omap3_l3_readll(l3->rt, L3_SI_FLAG_STATUS_0);
 		/*

@@ -202,7 +196,6 @@ static irqreturn_t omap3_l3_app_irq(int irq, void *_l3)
 
 	base = l3->rt + *(omap3_l3_bases[int_type] + err_source);
 	error = omap3_l3_readll(base, L3_ERROR_LOG);
-
 	if (error) {
 		error_addr = omap3_l3_readll(base, L3_ERROR_LOG_ADDR);

@@ -210,9 +203,8 @@ static irqreturn_t omap3_l3_app_irq(int irq, void *_l3)
 	}
 
 	/* Clear the status register */
-	clear = ((L3_AGENT_STATUS_CLEAR_IA << int_type) |
-		 (L3_AGENT_STATUS_CLEAR_TA));
-
+	clear = (L3_AGENT_STATUS_CLEAR_IA << int_type) |
+		L3_AGENT_STATUS_CLEAR_TA;
 	omap3_l3_writell(base, L3_AGENT_STATUS, clear);
 
 	/* clear the error log register */

@@ -240,7 +232,7 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 		goto err0;
 	}
 	l3->rt = ioremap(res->start, resource_size(res));
-	if (!(l3->rt)) {
+	if (!l3->rt) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
 		goto err0;

@@ -259,7 +251,6 @@ static int __init omap3_l3_probe(struct platform_device *pdev)
 	ret = request_irq(l3->app_irq, omap3_l3_app_irq,
 		IRQF_DISABLED | IRQF_TRIGGER_RISING,
 		"l3-app-irq", l3);
-
 	if (ret) {
 		dev_err(&pdev->dev, "couldn't request app irq\n");
 		goto err2;

Signature · 1 line

1.7.1

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #6 lore raw

PATCH 4/5 OMAP4: l3: fix omap4_l3_probe error path

In reply to Omar Ramirez Luna

If kzalloc fails driver shouldn't continue with a NULL pointer.

Add missing free_irq and remove empty goto labels.

Safe to assume that if we reached the end point of execution without errors, then return value is 0, so replacing instead another goto.

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 arch/arm/mach-omap2/omap_l3_noc.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

arch/arm/mach-omap2/omap_l3_noc.c +16 ~~-18~~

index 82632c2..def3de8 100644
@@ -132,49 +132,49 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 
 	l3 = kzalloc(sizeof(*l3), GFP_KERNEL);
 	if (!l3)
-		ret = -ENOMEM;
+		return -ENOMEM;
 
 	platform_set_drvdata(pdev, l3);
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "couldn't find resource 0\n");
 		ret = -ENODEV;
-		goto err1;
+		goto err0;
 	}
 
 	l3->l3_base[0] = ioremap(res->start, resource_size(res));
 	if (!(l3->l3_base[0])) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
-		goto err2;
+		goto err0;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	if (!res) {
 		dev_err(&pdev->dev, "couldn't find resource 1\n");
 		ret = -ENODEV;
-		goto err3;
+		goto err1;
 	}
 
 	l3->l3_base[1] = ioremap(res->start, resource_size(res));
 	if (!(l3->l3_base[1])) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
-		goto err4;
+		goto err1;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
 	if (!res) {
 		dev_err(&pdev->dev, "couldn't find resource 2\n");
 		ret = -ENODEV;
-		goto err5;
+		goto err2;
 	}
 
 	l3->l3_base[2] = ioremap(res->start, resource_size(res));
 	if (!(l3->l3_base[2])) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
-		goto err6;
+		goto err2;
 	}
 
 	/*

@@ -187,7 +187,7 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 	if (ret) {
 		pr_crit("L3: request_irq failed to register for 0x%x\n",
 					 OMAP44XX_IRQ_L3_DBG);
-		goto err7;
+		goto err3;
 	}
 	l3->debug_irq = irq;

@@ -198,24 +198,22 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 	if (ret) {
 		pr_crit("L3: request_irq failed to register for 0x%x\n",
 					 OMAP44XX_IRQ_L3_APP);
-		goto err8;
+		goto err4;
 	}
 	l3->app_irq = irq;
 
-	goto err0;
-err8:
-err7:
-	iounmap(l3->l3_base[2]);
-err6:
-err5:
-	iounmap(l3->l3_base[1]);
+	return 0;
+
 err4:
+	free_irq(l3->debug_irq, l3);
 err3:
-	iounmap(l3->l3_base[0]);
+	iounmap(l3->l3_base[2]);
 err2:
+	iounmap(l3->l3_base[1]);
 err1:
-	kfree(l3);
+	iounmap(l3->l3_base[0]);
 err0:
+	kfree(l3);
 	return ret;
 }

Signature · 1 line

1.7.1

Omar Ramirez Luna

author

2011-03-29 17:20 UTC #7 lore raw

PATCH 5/5 OMAP4: l3: minor cleanup for parenthesis and extra spaces

In reply to Omar Ramirez Luna

Removing unneeded parenthesis and two consecutive spaces around:

	std_err_main =  readl(std_err_main_addr);
			^^

While at it collapsing an if statement to detect the type of error based on the irq received.

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 arch/arm/mach-omap2/omap_l3_noc.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

arch/arm/mach-omap2/omap_l3_noc.c +7 ~~-10~~

index def3de8..7b9f190 100644
@@ -63,10 +63,7 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3)
 	char *source_name;
 
 	/* Get the Type of interrupt */
-	if (irq == l3->app_irq)
-		inttype = L3_APPLICATION_ERROR;
-	else
-		inttype = L3_DEBUG_ERROR;
+	inttype = irq == l3->app_irq ? L3_APPLICATION_ERROR : L3_DEBUG_ERROR;
 
 	for (i = 0; i < L3_MODULES; i++) {
 		/*

@@ -84,10 +81,10 @@ static irqreturn_t l3_interrupt_handler(int irq, void *_l3)
 
 			err_src = j;
 			/* Read the stderrlog_main_source from clk domain */
-			std_err_main_addr = base + (*(l3_targ[i] + err_src));
-			std_err_main =  readl(std_err_main_addr);
+			std_err_main_addr = base + *(l3_targ[i] + err_src);
+			std_err_main = readl(std_err_main_addr);
 
-			switch ((std_err_main & CUSTOM_ERROR)) {
+			switch (std_err_main & CUSTOM_ERROR) {
 			case STANDARD_ERROR:
 				source_name =
 				l3_targ_stderrlog_main_name[i][err_src];

@@ -143,7 +140,7 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 	}
 
 	l3->l3_base[0] = ioremap(res->start, resource_size(res));
-	if (!(l3->l3_base[0])) {
+	if (!l3->l3_base[0]) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
 		goto err0;

@@ -157,7 +154,7 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 	}
 
 	l3->l3_base[1] = ioremap(res->start, resource_size(res));
-	if (!(l3->l3_base[1])) {
+	if (!l3->l3_base[1]) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
 		goto err1;

@@ -171,7 +168,7 @@ static int __init omap4_l3_probe(struct platform_device *pdev)
 	}
 
 	l3->l3_base[2] = ioremap(res->start, resource_size(res));
-	if (!(l3->l3_base[2])) {
+	if (!l3->l3_base[2]) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
 		goto err2;

Signature · 1 line

1.7.1

Santosh Shilimkar

2011-03-30 06:45 UTC #8 lore raw

cover OMAP: l3: fixes and cleanup

In reply to Omar Ramirez Luna

Omar,

Quoted 16 lines · Omar Ramirez Luna

Based on the comments received for the first patch:
OMAP3: l3: fix for "irq 10: nobody cared" message[1], and quick skimming through the code.
Although there are still parenthesis that are not needed because of operator precedence, they were left for clarity.
E.g.:
(error>> 32)& 0xffff;
clear = (L3_AGENT_STATUS_CLEAR_IA<< int_type) |
         L3_AGENT_STATUS_CLEAR_TA;
---
[1] https://patchwork.kernel.org/patch/665141/

Quoted 8 lines

Omar Ramirez Luna (5):
OMAP3: l3: fix for "irq 10: nobody cared" message
OMAP3: l3: fix omap3_l3_probe error path
OMAP3: l3: minor cleanup for error message, parenthesis and extra
lines
OMAP4: l3: fix omap4_l3_probe error path
OMAP4: l3: minor cleanup for parenthesis and extra spaces

Thanks for the cleanup. I have reviewed the series and it looks good to me.
I would suggest to fold similar changes like,
- PATCH 2/5 and PATCH 4/5 into one patch
- PATCH 3/5 and PATCH 5/5 into one patch

With this update you can add my ack for this series

Quoted 4 lines

  arch/arm/mach-omap2/omap_l3_noc.c |   51 ++++++++++++++++--------------------
  arch/arm/mach-omap2/omap_l3_smx.c |   46 ++++++++++++---------------------
  2 files changed, 40 insertions(+), 57 deletions(-)

Ramirez Luna, Omar

author

2011-03-31 22:05 UTC #9 lore raw

cover OMAP: l3: fixes and cleanup

Quoted 7 lines · Santosh Shilimkar

Quoted 7 lines
? OMAP3: l3: fix for "irq 10: nobody cared" message
? OMAP3: l3: fix omap3_l3_probe error path
? OMAP3: l3: minor cleanup for error message, parenthesis and extra ? ? lines
? OMAP4: l3: fix omap4_l3_probe error path
? OMAP4: l3: minor cleanup for parenthesis and extra spaces

Quoted 7 lines

Thanks for the cleanup. I have reviewed the series and it looks good to me.
I would suggest to fold similar changes like,
- PATCH 2/5 and PATCH 4/5 into one patch
- PATCH 3/5 and PATCH 5/5 into one patch
With this update you can add my ack for this series

Ok, will do, thanks for the comments.

Regards,

Omar