[PATCH 0/5] OMAP: l3: fixes and cleanup

[PATCH 0/5] OMAP: l3: fixes and cleanup

[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/

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(-)

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

[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 <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_l3_smx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_l3_smx.c b/arch/arm/mach-omap2/omap_l3_smx.c
index 5f2da75..4321e79 100644
--- a/arch/arm/mach-omap2/omap_l3_smx.c
+++ b/arch/arm/mach-omap2/omap_l3_smx.c
@@ -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) {
-- 
1.7.1

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

[Santosh Shilimkar]

On 3/29/2011 10:50 PM, Omar Ramirez Luna wrote:
> 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<omar.ramirez@ti.com>
> ---

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

>   arch/arm/mach-omap2/omap_l3_smx.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_l3_smx.c b/arch/arm/mach-omap2/omap_l3_smx.c
> index 5f2da75..4321e79 100644
> --- a/arch/arm/mach-omap2/omap_l3_smx.c
> +++ b/arch/arm/mach-omap2/omap_l3_smx.c
> @@ -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) {

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

[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 <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_l3_smx.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_l3_smx.c b/arch/arm/mach-omap2/omap_l3_smx.c
index 4321e79..32961b1 100644
--- a/arch/arm/mach-omap2/omap_l3_smx.c
+++ b/arch/arm/mach-omap2/omap_l3_smx.c
@@ -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;
 }
 
-- 
1.7.1

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

[Santosh Shilimkar]

On 3/29/2011 10:50 PM, Omar Ramirez Luna wrote:
> 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<omar.ramirez@ti.com>
> ---

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 <santosh.shilimkar@ti.com>


>   arch/arm/mach-omap2/omap_l3_smx.c |   23 ++++++++++-------------
>   1 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_l3_smx.c b/arch/arm/mach-omap2/omap_l3_smx.c
> index 4321e79..32961b1 100644
> --- a/arch/arm/mach-omap2/omap_l3_smx.c
> +++ b/arch/arm/mach-omap2/omap_l3_smx.c
> @@ -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;
>   }
>

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

[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 <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_l3_smx.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_l3_smx.c b/arch/arm/mach-omap2/omap_l3_smx.c
index 32961b1..873c0e3 100644
--- a/arch/arm/mach-omap2/omap_l3_smx.c
+++ b/arch/arm/mach-omap2/omap_l3_smx.c
@@ -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;
-- 
1.7.1

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

[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 <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_l3_noc.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_l3_noc.c b/arch/arm/mach-omap2/omap_l3_noc.c
index 82632c2..def3de8 100644
--- a/arch/arm/mach-omap2/omap_l3_noc.c
+++ b/arch/arm/mach-omap2/omap_l3_noc.c
@@ -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;
 }
 
-- 
1.7.1

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

[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 <omar.ramirez@ti.com>
---
 arch/arm/mach-omap2/omap_l3_noc.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_l3_noc.c b/arch/arm/mach-omap2/omap_l3_noc.c
index def3de8..7b9f190 100644
--- a/arch/arm/mach-omap2/omap_l3_noc.c
+++ b/arch/arm/mach-omap2/omap_l3_noc.c
@@ -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;
-- 
1.7.1

[PATCH 0/5] OMAP: l3: fixes and cleanup

[Santosh Shilimkar]

Omar,

On 3/29/2011 10:50 PM, Omar Ramirez Luna wrote:
> 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
>
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

>   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(-)
>

[PATCH 0/5] OMAP: l3: fixes and cleanup

[Ramirez Luna, Omar]

On Wed, Mar 30, 2011 at 1:45 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>> ? 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

Ok, will do, thanks for the comments.

Regards,

Omar