From: grygorii.strashko@ti•com (Grygorii Strashko)
To: linux-arm-kernel@lists•infradead.org
Subject: Boot hang regression 3.10.0-rc4 -> 3.10.0
Date: Wed, 10 Jul 2013 15:16:54 +0300 [thread overview]
Message-ID: <51DD50B6.5030604@ti.com> (raw)
In-Reply-To: <20130709194131.GA12523@arwen.pp.htv.fi>
On 07/09/2013 10:41 PM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jul 09, 2013 at 09:59:28PM +0300, Grygorii Strashko wrote:
>>>>> Imagine the device is marked as suspended even though it's fully enabled
>>>>> (it hasn't been suspended by hwmod due to NO_IDLE flag). In that case
>>>>> your context structure is all zeroes (context has never been saved
>>>>> before) then when you call pm_runtime_get_sync() on probe() your
>>>>> ->runtime_resume() will get called, which will restore context,
>>>>> essentially undoing anything which was configured by u-boot.
>>>>
>>>> This could be a problem for drivers which do a save context in ->runtime_suspend()
>>>> but from what I see with omap serial, there is no save context done as part of
>>>> ->runtime_suspend.
>>>
>>> right, because context is "saved" in set_termios. probe() will get
>>> called much before set_termios() has a chance to run, right ?
>>>
>>> Same problem will trigger in that case.
>>>
>>> I still think patch below is necessary
>>>
>>>>> (completely untested, didn't even try to compile, just to illustrate)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>>>>> index 7341eff..d8dca68 100644
>>>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>>>> @@ -2559,6 +2559,12 @@ static void __init _setup_postsetup(struct omap_hwmod *oh)
>>>>> (postsetup_state == _HWMOD_STATE_IDLE)) {
>>>>> oh->_int_flags |= _HWMOD_SKIP_ENABLE;
>>>>> postsetup_state = _HWMOD_STATE_ENABLED;
>>>>> +
>>>>> + /* tell pm_runtime this device is already active */
>>>>> + pm_runtime_set_active(&oh->od->pdev->dev);
>>>>> + } else {
>>>>> + /* tell pm_runtime this device is trully suspended */
>>>>> + pm_runtime_set_suspended(&oh->od->pdev->dev);
>>>>> }
>>>>>
>>>>> if (postsetup_state == _HWMOD_STATE_IDLE)
>>>
>>
>> This will not work - _setup_postsetup() is called from core_initcall
>> level and OMAP devices have not been created at this moment yet
>> (of_platform_populate() is called from
>> customize_machine->init_machine->omap_generic_init() at arch_initcall time)
>
> fair enough, but something *like* that needs to be done. If pm_runtime
> doesn't know the state of the device by the time pm_runtime_enable() is
> called, the wrong assumptions might be made and we will forever have
> such problems as our ->runtime_resume() callback being called when it
> shouldn't.
>
>> More over, I don't recommend to depend on hwmod->od field - it's been
>> created to support OPPs and it's obsolete now in case of DT use.
>
> that's alright, but still we need something similar.
>
> But in any case, if on DT boot that's not used, than *what* uses
> HWMOD_INIT_NO_IDLE during DT boot ?
>
> There's a single place in kernel source which checks if
> HWMOD_INIT_NO_IDLE is set, and that's the place which I patched.
>
> We, certainly, need a way to tell pm_runtime if the device is active or
> suspended by the time we reach our probe() function. Either we assume
> *all* devices are active and we blindly call pm_runtime_set_active() for
> all devices, or we assume *all* devices are suspended as we call
> pm_runtime_set_suspend() for all devices, or we figure out which ones
> are active and which are not and call pm_runtime_set_{active,suspend}()
> conditionally.
>
>> Seems, This issue need to be handled in driver for DT boot use case,
>> possibly cmdline need to be parsed in the same way as it's done in
>> omap_serial_early_init().
>
> so you want *every* single driver to parse their own cmdline ? How big
> would the cmdline become ? This makes no sense.
>
Agree here. Seems there two similar issues:
- this one with omap_serial
- GPIO reset issue http://www.spinics.net/lists/linux-omap/msg84186.html
(similar issue is observed on OMAP4460+TPS6236x)
And I think, that HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET flags can't
be treated as
SoC (IP) specific parameters any more - they should be board specific.
I think, some kind of board_layout {} DT definition need to be created
to solve these issues (just an idea). For example:
Board DTS file:
board_layout {
ti,hwmods-init-no-reset = "gpio1", "uart3";
ti,hwmods-init-no-idle = "uart3";
}
Then above DT parameters can be used in omap_hwmod_setup_all()->_setup()
and omap_device_build_from_dt().
- omap_hwmod_setup_all()->_setup() will configure hwmod's flags
- omap_device_build_from_dt() will check for HWMOD_INIT_NO_IDLE and
call pm_runtime_set_active() if needed (this is the valid place for that)
NOTE. "ti,hwmods-init-no-reset" and "ti,hwmods-init-no-idle" can't be
defined per device, because HWMOD framework will work
(init/reset/setup/idle) with IP even if corresponding IP is not defined
in DT.
More over, board_layout{} section can be used to define some common for
OMAP2+ boards properties or enable/disable PM features, like:
- Setup clksetup/clkshoutdown time for oscillator
- enable/disable OFF-mode
Regards,
-grygorii
next prev parent reply other threads:[~2013-07-10 12:16 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-04 13:25 Boot hang regression 3.10.0-rc4 -> 3.10.0 Mark Jackson
2013-07-04 15:14 ` Mark Jackson
2013-07-04 16:00 ` Mark Jackson
2013-07-05 8:11 ` Bedia, Vaibhav
2013-07-05 11:59 ` Tony Lindgren
2013-07-05 13:20 ` Bedia, Vaibhav
2013-07-05 13:31 ` Bedia, Vaibhav
2013-07-08 11:25 ` Tony Lindgren
2013-07-08 12:16 ` Tony Lindgren
2013-07-08 12:41 ` Rajendra Nayak
2013-07-08 13:10 ` Tony Lindgren
2013-07-08 13:20 ` Rajendra Nayak
2013-07-08 13:25 ` Rajendra Nayak
2013-07-08 13:35 ` Felipe Balbi
2013-07-09 5:33 ` Rajendra Nayak
2013-07-09 6:42 ` Felipe Balbi
2013-07-09 7:19 ` Rajendra Nayak
2013-07-09 7:40 ` Felipe Balbi
2013-07-09 18:59 ` Grygorii Strashko
2013-07-09 19:41 ` Felipe Balbi
2013-07-10 12:16 ` Grygorii Strashko [this message]
2013-07-10 12:25 ` Felipe Balbi
2013-07-10 8:22 ` Kevin Hilman
2013-07-10 12:10 ` Tony Lindgren
2013-07-10 12:27 ` Tony Lindgren
2013-07-10 14:26 ` Tony Lindgren
2013-07-10 16:07 ` Felipe Balbi
2013-07-10 16:11 ` Felipe Balbi
2013-07-11 6:32 ` Tony Lindgren
2013-07-11 9:59 ` Grygorii Strashko
2013-07-12 0:40 ` Suman Anna
2013-07-15 6:44 ` Rajendra Nayak
2013-07-15 10:01 ` Rajendra Nayak
2013-07-15 19:23 ` Suman Anna
2013-07-16 6:30 ` Rajendra Nayak
2013-07-11 9:17 ` Rajendra Nayak
2013-07-11 9:26 ` Felipe Balbi
2013-07-11 10:16 ` [PATCH] arm: omap2plus: unidle devices which are about to probe Felipe Balbi
2013-07-12 11:58 ` Grygorii Strashko
2013-07-12 12:10 ` Felipe Balbi
2013-07-12 12:27 ` Rajendra Nayak
2013-07-13 22:21 ` Kevin Hilman
2013-07-11 9:59 ` Boot hang regression 3.10.0-rc4 -> 3.10.0 Grygorii Strashko
2013-07-16 10:27 ` Grygorii Strashko
2013-07-17 7:10 ` Rajendra Nayak
2013-07-11 6:18 ` Rajendra Nayak
2013-07-11 6:24 ` Tony Lindgren
2013-07-11 9:11 ` Rajendra Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51DD50B6.5030604@ti.com \
--to=grygorii.strashko@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox