Re: Boot hang regression 3.10.0-rc4 -> 3.10.0

[Rajendra Nayak <rnayak@ti.com>]

On Monday 15 July 2013 12:14 PM, Rajendra Nayak wrote:
> On Friday 12 July 2013 06:10 AM, Suman Anna wrote:
>> On 07/11/2013 04:59 AM, Grygorii Strashko wrote:
>>> Hi,
>>>
>>> On 07/11/2013 09:32 AM, Tony Lindgren wrote:
>>>> * Felipe Balbi <balbi@ti.com> [130710 09:18]:
>>>>>
>>>>> On Wed, Jul 10, 2013 at 07:07:04PM +0300, Felipe Balbi wrote:
>>>>>> how about something like below ? It makes omap_device/hwmod and
>>>>>> pm_runtime agree on the initial state of the device and will prevent
>>>>>> ->runtime_resume() from being called on first pm_runtime_get*() done
>>>>>> during probe.
>>>>>>
>>>>>> This is similar to what PCI bus does (if you look at pci_pm_init()).
>>>>>>
>>>>>> commit 59108a500b4ab4b1a5102648a3360276dbf7df6f
>>>>>> Author: Felipe Balbi <balbi@ti.com>
>>>>>> Date:   Wed Jul 10 18:50:16 2013 +0300
>>>>>>
>>>>>>      arm: omap2plus: unidle devices which are about to probe
>>>>>>
>>>>>>      in order to make HWMOD and pm_runtime agree on the
>>>>>>      initial state of the device, we will unidle the device
>>>>>>      and call pm_runtime_set_active() to tell pm_runtime
>>>>>>      that the device is really active.
>>> Don't think that it's good idea (
>>> I've checked some driver's and think this patch will enable some devices
>>> unpredictably:
>>> - hwspinlock
>>> - mailbox
>>> - iommu
>>> - ipu
>>> All above devices need to be enabled on demand only (no
>>> pm_runtime_get*() calls in probe). More over, some of them have very
>>> specific enabling sequence - like ipu).
>>>
>>> May be Summan can say more on that.
>>
>> Indeed, this is a problem for any of the slave processor devices.
>> mailbox and iommu would be slaves to the remoteproc and the drivers have
>> a specific sequence of bringing up a processor. The current
>> hwmod/omap_device code is such that these devices will be left in reset
>> and the driver code use the omap_device_(de)assert_hardreset API
>> together with omap_device_enable code to bring up the devices. The
>> remoteproc driver also needs to assert the resets (there are other
>> problems associated with using omap_device_idle for remoteproc and
>> iommu) for bringing up the devices after a suspend sequence. hwspinlock
>> and mailbox may get away since they are in CORE domain, but definitely
>> an issue for iommu and remoteproc. I would think that this would also
>> affect other compute devices like IVAHD, ISS, SGX.
> 
> Today, for these IPs I guess hwmod waits for the resets to be de-asserted, right?
> 
>         /*
>          * If an IP block contains HW reset lines and all of them are
>          * asserted, we let integration code associated with that
>          * block handle the enable.  We've received very little
>          * information on what those driver authors need, and until
>          * detailed information is provided and the driver code is
>          * posted to the public lists, this is probably the best we
>          * can do.
>          */
>         if (_are_all_hardreset_lines_asserted(oh))
>                 return 0;
> 
> What if this information is send back to omap_device() through a return value
> so omap_device() knows about this too, so it avoids marking the omap device as
> enabled? Wouldn't that fix the issue?

I meant something like this..

>From 2fbea0dde0f72897089ef2e8e441b5e5bd6ea967 Mon Sep 17 00:00:00 2001
From: Rajendra Nayak <rnayak@ti.com>
Date: Mon, 15 Jul 2013 15:23:07 +0530
Subject: [PATCH] ARM: OMAP2+: Make omap_device aware of hwmod failing to
 enable/idle/shutdown the hwmods

For IP blocks (mainly processors) which have hard reset lines, hwmod avoids
enable/idle/shutdown operations as long as all the hard reset lines are
kept asserted. However it does not return an error back to the caller (in some
cases the omap_device layer) to communicate back the failure to operate on the
hwmod.

Fix this by making _enable()/_idle()/_shutdown() all return an error in such
cases, and also fix the omap_device layer to look at the return values coming
from hwmod operations before doing a omap_device level state transition.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_device.c |   20 ++++++++++----------
 arch/arm/mach-omap2/omap_hwmod.c  |    6 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 5cc9287..e89244b 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -210,13 +210,12 @@ static int _omap_device_notifier_call(struct notifier_block *nb,
  */
 static int _omap_device_enable_hwmods(struct omap_device *od)
 {
-	int i;
+	int i, ret = 0;
 
 	for (i = 0; i < od->hwmods_cnt; i++)
-		omap_hwmod_enable(od->hwmods[i]);
+		ret |= omap_hwmod_enable(od->hwmods[i]);
 
-	/* XXX pass along return value here? */
-	return 0;
+	return ret;
 }
 
 /**
@@ -227,13 +226,12 @@ static int _omap_device_enable_hwmods(struct omap_device *od)
  */
 static int _omap_device_idle_hwmods(struct omap_device *od)
 {
-	int i;
+	int i, ret = 0;
 
 	for (i = 0; i < od->hwmods_cnt; i++)
-		omap_hwmod_idle(od->hwmods[i]);
+		ret |= omap_hwmod_idle(od->hwmods[i]);
 
-	/* XXX pass along return value here? */
-	return 0;
+	return ret;
 }
 
 /* Public functions for use by core code */
@@ -697,7 +695,8 @@ int omap_device_enable(struct platform_device *pdev)
 
 	ret = _omap_device_enable_hwmods(od);
 
-	od->_state = OMAP_DEVICE_STATE_ENABLED;
+	if (!ret)
+		od->_state = OMAP_DEVICE_STATE_ENABLED;
 
 	return ret;
 }
@@ -727,7 +726,8 @@ int omap_device_idle(struct platform_device *pdev)
 
 	ret = _omap_device_idle_hwmods(od);
 
-	od->_state = OMAP_DEVICE_STATE_IDLE;
+	if (!ret)
+		od->_state = OMAP_DEVICE_STATE_IDLE;
 
 	return ret;
 }
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 7341eff..07fd2a8 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2133,7 +2133,7 @@ static int _enable(struct omap_hwmod *oh)
 	 * can do.
 	 */
 	if (_are_all_hardreset_lines_asserted(oh))
-		return 0;
+		return -EPERM;
 
 	/* Mux pins for device runtime if populated */
 	if (oh->mux && (!oh->mux->enabled ||
@@ -2222,7 +2222,7 @@ static int _idle(struct omap_hwmod *oh)
 	}
 
 	if (_are_all_hardreset_lines_asserted(oh))
-		return 0;
+		return -EPERM;
 
 	if (oh->class->sysc)
 		_idle_sysc(oh);
@@ -2276,7 +2276,7 @@ static int _shutdown(struct omap_hwmod *oh)
 	}
 
 	if (_are_all_hardreset_lines_asserted(oh))
-		return 0;
+		return -EPERM;
 
 	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
 
-- 
1.7.9.5