[PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Suman Anna]

The hwmod init sequence involves initializing and idling all the
hwmods during bootup. If a module class has sysconfig, the init
sequence utilizes the module register base for performing any
sysc configuration.

The module address space is being removed from hwmod database and
retrieved from the <reg> property of the corresponding DT node.
If a hwmod does not have its corresponding DT node defined and the
memory address space is not defined in the corresponding
omap_hwmod_ocp_if, then the module register target address space
would be NULL and any sysc programming would result in a NULL
pointer dereference and a kernel boot hang.

Handle this scenario by checking for a valid module address space
during the _init of each hwmod, and leaving it in the registered
state if no module register address base is defined in either of
the hwmod data or the DT data.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
This patch helps break the dependencies between hwmod entries and
corresponding DT entries (especially on OMAP5, where most of the
address spaces are already cleaned up and the current data files
have minimal entries) and fixes any boot issues due to missing
addresses. See for reference,
http://marc.info/?t=138005421400003&r=1&w=2

Tested on BeagleXM, Panda4, BeagleBone Black and Panda5 using
Tero's v7 clk DT patches and Benoit's for-3.13/dts on top of
3.12-rc3

 arch/arm/mach-omap2/omap_hwmod.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index d9ee0ff..1d9edb3 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2361,21 +2361,23 @@ static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
  * Cache the virtual address used by the MPU to access this IP block's
  * registers.  This address is needed early so the OCP registers that
  * are part of the device's address space can be ioremapped properly.
- * No return value.
+ *
+ * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and
+ * -ENOMEM on absent or invalid register target address space.
  */
-static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
+static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 {
 	struct omap_hwmod_addr_space *mem;
 	void __iomem *va_start = NULL;
 	struct device_node *np;
 
 	if (!oh)
-		return;
+		return -EINVAL;
 
 	_save_mpu_port_index(oh);
 
 	if (oh->_int_flags & _HWMOD_NO_MPU_PORT)
-		return;
+		return -ENOMEM;
 
 	mem = _find_mpu_rt_addr_space(oh);
 	if (!mem) {
@@ -2384,7 +2386,7 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 
 		/* Extract the IO space from device tree blob */
 		if (!of_have_populated_dt())
-			return;
+			return -ENOMEM;
 
 		np = of_dev_hwmod_lookup(of_find_node_by_name(NULL, "ocp"), oh);
 		if (np)
@@ -2395,13 +2397,14 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 
 	if (!va_start) {
 		pr_err("omap_hwmod: %s: Could not ioremap\n", oh->name);
-		return;
+		return -ENOMEM;
 	}
 
 	pr_debug("omap_hwmod: %s: MPU register target at va %p\n",
 		 oh->name, va_start);
 
 	oh->_mpu_rt_va = va_start;
+	return 0;
 }
 
 /**
@@ -2414,8 +2417,8 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
  * registered at this point.  This is the first of two phases for
  * hwmod initialization.  Code called here does not touch any hardware
  * registers, it simply prepares internal data structures.  Returns 0
- * upon success or if the hwmod isn't registered, or -EINVAL upon
- * failure.
+ * upon success or if the hwmod isn't registered or if the hwmod's
+ * address space is not defined, or -EINVAL upon failure.
  */
 static int __init _init(struct omap_hwmod *oh, void *data)
 {
@@ -2424,8 +2427,14 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 	if (oh->_state != _HWMOD_STATE_REGISTERED)
 		return 0;
 
-	if (oh->class->sysc)
-		_init_mpu_rt_base(oh, NULL);
+	if (oh->class->sysc) {
+		r = _init_mpu_rt_base(oh, NULL);
+		if (r < 0) {
+			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",
+				oh->name);
+			return 0;
+		}
+	}
 
 	r = _init_clocks(oh, NULL);
 	if (r < 0) {
-- 
1.8.4

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Santosh Shilimkar]

On Thursday 03 October 2013 12:59 PM, Suman Anna wrote:
> The hwmod init sequence involves initializing and idling all the
> hwmods during bootup. If a module class has sysconfig, the init
> sequence utilizes the module register base for performing any
> sysc configuration.
> 
> The module address space is being removed from hwmod database and
> retrieved from the <reg> property of the corresponding DT node.
> If a hwmod does not have its corresponding DT node defined and the
> memory address space is not defined in the corresponding
> omap_hwmod_ocp_if, then the module register target address space
> would be NULL and any sysc programming would result in a NULL
> pointer dereference and a kernel boot hang.
> 
> Handle this scenario by checking for a valid module address space
> during the _init of each hwmod, and leaving it in the registered
> state if no module register address base is defined in either of
> the hwmod data or the DT data.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> This patch helps break the dependencies between hwmod entries and
> corresponding DT entries (especially on OMAP5, where most of the
> address spaces are already cleaned up and the current data files
> have minimal entries) and fixes any boot issues due to missing
> addresses. See for reference,
> http://marc.info/?t=138005421400003&r=1&w=2
> 
> Tested on BeagleXM, Panda4, BeagleBone Black and Panda5 using
> Tero's v7 clk DT patches and Benoit's for-3.13/dts on top of
> 3.12-rc3
> 
Good to break that dts/hwmod dependency.
FWIW,
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Nishanth Menon]

On 10/03/2013 11:59 AM, Suman Anna wrote:
> The hwmod init sequence involves initializing and idling all the
> hwmods during bootup. If a module class has sysconfig, the init
> sequence utilizes the module register base for performing any
> sysc configuration.
> 
> The module address space is being removed from hwmod database and
> retrieved from the <reg> property of the corresponding DT node.
> If a hwmod does not have its corresponding DT node defined and the
> memory address space is not defined in the corresponding
> omap_hwmod_ocp_if, then the module register target address space
> would be NULL and any sysc programming would result in a NULL
> pointer dereference and a kernel boot hang.
> 
> Handle this scenario by checking for a valid module address space
> during the _init of each hwmod, and leaving it in the registered
> state if no module register address base is defined in either of
> the hwmod data or the DT data.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> This patch helps break the dependencies between hwmod entries and
> corresponding DT entries (especially on OMAP5, where most of the
> address spaces are already cleaned up and the current data files
> have minimal entries) and fixes any boot issues due to missing
> addresses. See for reference,
> http://marc.info/?t=138005421400003&r=1&w=2
> 
> Tested on BeagleXM, Panda4, BeagleBone Black and Panda5 using
> Tero's v7 clk DT patches and Benoit's for-3.13/dts on top of
> 3.12-rc3
> 
>  arch/arm/mach-omap2/omap_hwmod.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)

Mandatory to have this patch for OMAP5uEVM to boot after Tero's v7 [1]
series is merged else the delta between dts and hwmod entries cause
OMAP5 platforms to croak and die - at least worked around as seen in
[2] :(

Tested-by: Nishanth Menon <nm@ti.com>

[1] http://marc.info/?t=138009899400001&r=1&w=2
[2] OMAP5uEVM: http://pastebin.com/jtEMwTY5

-- 
Regards,
Nishanth Menon

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Suman Anna]

On 10/07/2013 04:19 PM, Nishanth Menon wrote:
> On 10/03/2013 11:59 AM, Suman Anna wrote:
>> The hwmod init sequence involves initializing and idling all the
>> hwmods during bootup. If a module class has sysconfig, the init
>> sequence utilizes the module register base for performing any
>> sysc configuration.
>>
>> The module address space is being removed from hwmod database and
>> retrieved from the <reg> property of the corresponding DT node.
>> If a hwmod does not have its corresponding DT node defined and the
>> memory address space is not defined in the corresponding
>> omap_hwmod_ocp_if, then the module register target address space
>> would be NULL and any sysc programming would result in a NULL
>> pointer dereference and a kernel boot hang.
>>
>> Handle this scenario by checking for a valid module address space
>> during the _init of each hwmod, and leaving it in the registered
>> state if no module register address base is defined in either of
>> the hwmod data or the DT data.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> This patch helps break the dependencies between hwmod entries and
>> corresponding DT entries (especially on OMAP5, where most of the
>> address spaces are already cleaned up and the current data files
>> have minimal entries) and fixes any boot issues due to missing
>> addresses. See for reference,
>> http://marc.info/?t=138005421400003&r=1&w=2
>>
>> Tested on BeagleXM, Panda4, BeagleBone Black and Panda5 using
>> Tero's v7 clk DT patches and Benoit's for-3.13/dts on top of
>> 3.12-rc3
>>
>>  arch/arm/mach-omap2/omap_hwmod.c | 29 +++++++++++++++++++----------
>>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> Mandatory to have this patch for OMAP5uEVM to boot after Tero's v7 [1]
> series is merged else the delta between dts and hwmod entries cause
> OMAP5 platforms to croak and die - at least worked around as seen in
> [2] :(
> 
> Tested-by: Nishanth Menon <nm@ti.com>
> 
> [1] http://marc.info/?t=138009899400001&r=1&w=2
> [2] OMAP5uEVM: http://pastebin.com/jtEMwTY5
> 

This patch should also help Paul to be able to pick up the OMAP5
hwspinlock hwmod data [3] and would yield a similar warning without
its corresponding DT node as in Nishant's log [2]. Obviously, both
hwmod and DT need to be present for the associated driver to work, but
atleast it doesn't hang the boot.

regards
Suman

[3] http://marc.info/?l=linux-omap&m=138055666108306&w=2

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Tony Lindgren]

* Suman Anna <s-anna@ti.com> [131003 10:07]:
> The hwmod init sequence involves initializing and idling all the
> hwmods during bootup. If a module class has sysconfig, the init
> sequence utilizes the module register base for performing any
> sysc configuration.
> 
> The module address space is being removed from hwmod database and
> retrieved from the <reg> property of the corresponding DT node.
> If a hwmod does not have its corresponding DT node defined and the
> memory address space is not defined in the corresponding
> omap_hwmod_ocp_if, then the module register target address space
> would be NULL and any sysc programming would result in a NULL
> pointer dereference and a kernel boot hang.

Hmm so is this needed as a fix for the -rcy cycle?
 
Other than that looks OK to me, Paul should queue or ack this one:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Suman Anna]

Tony,

On 10/08/2013 01:09 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [131003 10:07]:
>> The hwmod init sequence involves initializing and idling all the
>> hwmods during bootup. If a module class has sysconfig, the init
>> sequence utilizes the module register base for performing any
>> sysc configuration.
>>
>> The module address space is being removed from hwmod database and
>> retrieved from the <reg> property of the corresponding DT node.
>> If a hwmod does not have its corresponding DT node defined and the
>> memory address space is not defined in the corresponding
>> omap_hwmod_ocp_if, then the module register target address space
>> would be NULL and any sysc programming would result in a NULL
>> pointer dereference and a kernel boot hang.
> 
> Hmm so is this needed as a fix for the -rcy cycle?

This is a dependency for a successful OMAP5 boot with Tero's clock
patches (just the clock patches won't help), but those are not in
3.12-rcy anyway. I will leave it upto you and Paul, it will be one less
patch that people need to worry about for booting OMAP5 if it can make
the -rcy cycle.

regards
Suman

>  
> Other than that looks OK to me, Paul should queue or ack this one:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Paul Walmsley]

Hi

On Thu, 3 Oct 2013, Suman Anna wrote:

> The hwmod init sequence involves initializing and idling all the
> hwmods during bootup. If a module class has sysconfig, the init
> sequence utilizes the module register base for performing any
> sysc configuration.

thanks for doing this.  A few comments:

1. The patch adds a warning when checked with 'checkpatch.pl --strict':

CHECK: Alignment should match open parenthesis
#109: FILE: arch/arm/mach-omap2/omap_hwmod.c:2434:
+			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",
+				oh->name);

The issue has been fixed in in the local copy, but please don't forget to 
ensure that your patches don't generate any warnings[*] from 
'checkpatch.pl --strict' before sending them.

[*] Well, with the possible exception of 80-column violations when 
necessary.

2. -ENOMEM isn't the right error code to use here - it connotates "out of 
memory" rather than "missing address space".  -ENXIO seems better, so I've 
changed the patch to use that instead.


The patch has been queued provisionally, pending the outcome of testing.


- Paul

Re: [PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Suman Anna]

Hi Paul,

On 10/09/2013 12:35 AM, Paul Walmsley wrote:
> Hi
> 
> On Thu, 3 Oct 2013, Suman Anna wrote:
> 
>> The hwmod init sequence involves initializing and idling all the
>> hwmods during bootup. If a module class has sysconfig, the init
>> sequence utilizes the module register base for performing any
>> sysc configuration.
> 
> thanks for doing this.  A few comments:
> 
> 1. The patch adds a warning when checked with 'checkpatch.pl --strict':
> 
> CHECK: Alignment should match open parenthesis
> #109: FILE: arch/arm/mach-omap2/omap_hwmod.c:2434:
> +			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",
> +				oh->name);
> 
> The issue has been fixed in in the local copy, but please don't forget to 
> ensure that your patches don't generate any warnings[*] from 
> 'checkpatch.pl --strict' before sending them.

I have not been using the --strict option, but will change my patch
sanity checking to use it going forward.

> 
> [*] Well, with the possible exception of 80-column violations when 
> necessary.
> 
> 2. -ENOMEM isn't the right error code to use here - it connotates "out of 
> memory" rather than "missing address space".  -ENXIO seems better, so I've 
> changed the patch to use that instead.

OK, thanks for fixing it up.

regards
Suman

[PATCH] ARM: OMAP2+: hwmod: check for module address space during init

[Paul Walmsley]

Commit 6423d6df1440a8acfc2f375d7cbc4cd66c2e6910 upstream.  Applies on
v3.12.  This fixes boot-time crashes on AM335x-based boards:

http://marc.info/?l=linux-omap&m=138495738502704&w=2

...

The hwmod init sequence involves initializing and idling all the
hwmods during bootup. If a module class has sysconfig, the init
sequence utilizes the module register base for performing any
sysc configuration.

The module address space is being removed from hwmod database and
retrieved from the <reg> property of the corresponding DT node.
If a hwmod does not have its corresponding DT node defined and the
memory address space is not defined in the corresponding
omap_hwmod_ocp_if, then the module register target address space
would be NULL and any sysc programming would result in a NULL
pointer dereference and a kernel boot hang.

Handle this scenario by checking for a valid module address space
during the _init of each hwmod, and leaving it in the registered
state if no module register address base is defined in either of
the hwmod data or the DT data.

Signed-off-by: Suman Anna <s-anna@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Nishanth Menon <nm@ti.com>
Acked-by: Tony Lindgren <tony@atomide.com>
[paul@pwsan.com: use -ENXIO rather than -ENOMEM to indicate a missing address
 space error; fixed checkpatch.pl problem]
Cc: Stefan Roese <stefan.roese@gmail.com>
Cc: Rajendra Nayak <rnayak@ti.com>
[paul@pwsan.com: added -stable notes]
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index d9ee0ff094d4..3d5db8c83b3c 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2361,21 +2361,23 @@ static struct device_node *of_dev_hwmod_lookup(struct device_node *np,
  * Cache the virtual address used by the MPU to access this IP block's
  * registers.  This address is needed early so the OCP registers that
  * are part of the device's address space can be ioremapped properly.
- * No return value.
+ *
+ * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and
+ * -ENXIO on absent or invalid register target address space.
  */
-static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
+static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 {
 	struct omap_hwmod_addr_space *mem;
 	void __iomem *va_start = NULL;
 	struct device_node *np;
 
 	if (!oh)
-		return;
+		return -EINVAL;
 
 	_save_mpu_port_index(oh);
 
 	if (oh->_int_flags & _HWMOD_NO_MPU_PORT)
-		return;
+		return -ENXIO;
 
 	mem = _find_mpu_rt_addr_space(oh);
 	if (!mem) {
@@ -2384,7 +2386,7 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 
 		/* Extract the IO space from device tree blob */
 		if (!of_have_populated_dt())
-			return;
+			return -ENXIO;
 
 		np = of_dev_hwmod_lookup(of_find_node_by_name(NULL, "ocp"), oh);
 		if (np)
@@ -2395,13 +2397,14 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
 
 	if (!va_start) {
 		pr_err("omap_hwmod: %s: Could not ioremap\n", oh->name);
-		return;
+		return -ENXIO;
 	}
 
 	pr_debug("omap_hwmod: %s: MPU register target at va %p\n",
 		 oh->name, va_start);
 
 	oh->_mpu_rt_va = va_start;
+	return 0;
 }
 
 /**
@@ -2414,8 +2417,8 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data)
  * registered at this point.  This is the first of two phases for
  * hwmod initialization.  Code called here does not touch any hardware
  * registers, it simply prepares internal data structures.  Returns 0
- * upon success or if the hwmod isn't registered, or -EINVAL upon
- * failure.
+ * upon success or if the hwmod isn't registered or if the hwmod's
+ * address space is not defined, or -EINVAL upon failure.
  */
 static int __init _init(struct omap_hwmod *oh, void *data)
 {
@@ -2424,8 +2427,14 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 	if (oh->_state != _HWMOD_STATE_REGISTERED)
 		return 0;
 
-	if (oh->class->sysc)
-		_init_mpu_rt_base(oh, NULL);
+	if (oh->class->sysc) {
+		r = _init_mpu_rt_base(oh, NULL);
+		if (r < 0) {
+			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",
+			     oh->name);
+			return 0;
+		}
+	}
 
 	r = _init_clocks(oh, NULL);
 	if (r < 0) {
-- 
1.8.4.3