From: khilman@linaro•org (Kevin Hilman)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support
Date: Fri, 09 Aug 2013 13:34:40 -0700 [thread overview]
Message-ID: <87bo5661sv.fsf@kernel.org> (raw)
In-Reply-To: <52051A9B.1060606@ti.com> (Nishanth Menon's message of "Fri, 9 Aug 2013 11:36:43 -0500")
Nishanth Menon <nm@ti•com> writes:
> On 08/09/2013 11:12 AM, Kevin Hilman wrote:
>> Nishanth Menon <nm@ti•com> writes:
>>
>>> On 08/08/2013 06:04 PM, Kevin Hilman wrote:
>>>> Nishanth Menon <nm@ti•com> writes:
>>>>
>>>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote:
>>>>>> Dave Gerlach <d-gerlach@ti•com> writes:
>>>>>>
>>>>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:
>>>>>>>> $subject and patch don't match.
>>>>>>>>
>>>>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:
>>>>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote:
>>>>>>>>>> In reference to
>>>>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver
>>>>>>>>>> bound and which don't.
>>>>>>>>> Does it need to? M3 firmware can pretty much define "I will force
>>>>>>>>> the device into low power state, and if the drivers dont handle
>>>>>>>>> things properly, fix the darned driver". M3 behavior should be
>>>>>>>>> considered as a "hardware" as far as Linux running on MPU is
>>>>>>>>> concerned, and firmware helps change the behavior by accounting for
>>>>>>>>> SoC quirks. *if* we have ability to handle this in the firmware,
>>>>>>>>> there is no need to carry this in Linux.
>>>>>>>>>
>>>>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same
>>>>>>>> comment in the last version. Linux need not know about all such firmware
>>>>>>>> quirks. Also all these M3 specific stuff, should be done somewhere
>>>>>>>> else. Probably having a small M3 driver won't be a bad idea.
>>>>>>>
>>>>>>> I am not opposed to doing it this way and letting the M3 firmware
>>>>>>> handle idling these modules, however the one concern raised in the
>>>>>>> last series is that an approach that does not acknowledge drivers will
>>>>>>> hide driver PM bugs. I suppose as long as I make sure to document that
>>>>>>> the devices are being idled by the M3 firmware this may not be an
>>>>>>> issue. I will look into implementing this.
>>>>>>
>>>>>> No, please don't start idling devices in firmware that are otherwise
>>>>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing
>>>>>> these devices, it should manage their bugs too.
>>>>>
>>>>>>
>>>>>> This is not just about idling devices. This is about handling broken IP
>>>>>> blocks whose power-on reset state does not allow the the powerdomain to
>>>>>> reach its target state. That's just bad hardware design.
>>>>>
>>>>> Right, this is where M3 can help -> provide a consistent state for
>>>>> linux kernel to work with. by the fact that we want to keep majority
>>>>> of the power code inside master CPU, we are just letting M3 help us
>>>>> with nothing major at all..
>>>>
>>>> heh, I would say HW design bugs like this are more than "nothing major
>>>> at all." :)
>>>>
>>>>> tiny stuff like these can help "fix" the hardware design quirks by
>>>>> hiding it behind the firmware and modifying the hardware behavior.
>>>>
>>>> I disagree here. I'm a firmware minimalist, and hiding bugs like this
>>>> in the firmware is wrong when Linux is otherwise managing these devices.
>>>> It also imposes criteria on the firmware of future SoCs that doesn't
>>>> belong there either. IMO, the only stuff the firmware should do is what
>>>> Linux *cannot* do.
>>>>
>>>> Remember, this only needs to happen when there isn't a driver for these
>>>> devices. Should we communicate to the firmware that the OS has no
>>>> driver, so please enable the hack? I think not.
>>>
>>> My view is that the M3 should *ignore* the presence/existence of MPU's
>>> drivers. M3 will do whatever to force the system to go to suspend once
>>> notified - this saves us the prehistoric perpetual trouble when
>>> drivers have bugs (which get exposed in weird usage scenarios) in
>>> production systems, we dont get any hardware help to fix them up while
>>> attempting low power states and system never really hits low power
>>> state. This was always because OMAP and it's derivatives have been
>>> "democratic" in power management - if every hardware block achieves
>>> proper state, then we achieve a system-wide low power state.
>>>
>>>>
>>>>> I know it breaks the purity of role, but as the
>>>>> next evolution, we might want to consider M3 something like an
>>>>> "accelerator" for power management activity.. (not saying it is that
>>>>> fast.. but conceptually).
>>>>
>>>> Yes, it breaks the purity of role, and makes it hard to maintain and
>>>> extend to future SoCs. As a maintainer, that's a red flag. IMO, the
>>>> roles need to be kept clear. The M3 manages some devices and the
>>>> interconnect that MPU/Linux cannot, the rest are managed by Linux.
>>>
>>> suspend is a very controlled state as against cpuidle where driver
>>> knowledge is necessary and in fact mandatory. drivers are supposed to
>>> release their resources - and even though we test the hell out of
>>> them, we do have paths untrodden when it comes to production systems.
>>
>> Since folks don't seem to care about idle for AM33xx (starting with the
>> hw designers, from what I can tell), you have the luxury of thinking
>> only about suspend, where firmware can be heavy handed and force things
>> into submission. Unfortunately, with cpuidle, life is not that easy and
>> you have to have cooperation of the device drivers. Coordinating that
>> with firmware is not so simple, to put it mildly.
>>
>> Any SW/firmware design that does not account for *both* static PM
>> (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term
>> maintainable, and thus ready for mainline IMO. (BTW, this is another
>> theme from previous reviews of this series.)
>
> I completely agree with you. But is'nt the specific suspend state we
> are attempting to achieve on AM335x just tooo expensive latency wise
> for being even considered for cpuidle?
>
> I am sure you recollect the latencies involved in OMAP3 OFF mode Vs
> OMAP4+ OFF mode - which basically kicked out OFF mode from OMAP4
> cpuidle C states? - it was practically useless
>
> in this *specific* power state we are attempting, we do a bunch of i2c
> operations, etc, in short something that cannot even be considered for
> cpuidle.
>
> Considering this, we can consider the same only for suspend path -
> hence allowing firmware to do more here.
>
>
> This does not conflict with cpuidle (which controls MPU) or runtime PM
> (which kicks in once you have drivers active, but if drivers get
> active, we dont need to deal with this crap).
>
> Dont you think this helps the specific case to move this into firmware
> rather than into omap_device?
No, I don't.
That means the firmware design is based on several assumptions about
what Linux can and can't do in idle, and then imposing that on future
Linux designs as well. I dont' buy it.
Kevin
next prev parent reply other threads:[~2013-08-09 20:34 UTC|newest]
Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-06 17:49 [PATCHv3 0/9] ARM: OMAP2+: AM33XX: Add suspend-resume support Dave Gerlach
2013-08-06 17:49 ` [PATCHv3 1/9] memory: emif: Move EMIF register defines to include/linux/ Dave Gerlach
2013-08-08 0:48 ` Russ Dill
2013-08-08 13:35 ` Santosh Shilimkar
2013-08-12 19:32 ` Greg Kroah-Hartman
2013-08-12 19:33 ` Santosh Shilimkar
2013-08-06 17:49 ` [PATCHv3 2/9] ARM: OMAP2+: AM33XX: control: Add some control module registers and APIs Dave Gerlach
2013-08-08 0:52 ` Russ Dill
2013-08-08 13:44 ` Santosh Shilimkar
2013-08-08 16:16 ` Dave Gerlach
2013-08-09 5:11 ` Tony Lindgren
2013-08-09 20:55 ` Dave Gerlach
2013-08-12 7:54 ` Tony Lindgren
2013-08-12 19:17 ` Kevin Hilman
2013-08-12 21:40 ` Dave Gerlach
2013-08-13 14:29 ` Kevin Hilman
2013-08-13 15:08 ` Santosh Shilimkar
2013-08-13 16:19 ` Kevin Hilman
2013-08-13 18:18 ` Santosh Shilimkar
2013-08-13 18:30 ` Russ Dill
2013-08-13 18:40 ` Santosh Shilimkar
2013-08-13 19:11 ` Kevin Hilman
2013-08-14 17:27 ` Suman Anna
2013-08-14 19:16 ` Russ Dill
2013-08-20 23:39 ` Paul Walmsley
2013-08-21 17:32 ` Suman Anna
2013-08-06 17:49 ` [PATCHv3 3/9] ARM: OMAP: DTB: Update IRQ data for WKUP_M3 Dave Gerlach
2013-08-08 0:53 ` Russ Dill
2013-08-08 13:46 ` Santosh Shilimkar
2013-08-06 17:49 ` [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec Dave Gerlach
2013-08-08 2:30 ` Russ Dill
2013-08-08 14:19 ` Santosh Shilimkar
2013-08-08 18:16 ` Kevin Hilman
2013-08-08 19:31 ` Santosh Shilimkar
2013-08-08 20:05 ` Kevin Hilman
2013-08-08 20:11 ` Santosh Shilimkar
2013-08-09 15:11 ` Kevin Hilman
2013-08-09 16:25 ` Dave Gerlach
2013-08-06 17:49 ` [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations Dave Gerlach
2013-08-08 7:02 ` Russ Dill
2013-08-08 14:50 ` Santosh Shilimkar
2013-08-08 15:16 ` Russ Dill
2013-08-08 15:22 ` Santosh Shilimkar
2013-08-08 16:03 ` Russ Dill
2013-08-19 12:54 ` Gururaja Hebbar
2013-08-19 17:51 ` Dave Gerlach
2013-08-06 17:49 ` [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device Dave Gerlach
2013-08-08 7:03 ` Russ Dill
2013-08-08 14:23 ` Santosh Shilimkar
2013-08-08 16:09 ` Dave Gerlach
2013-08-08 18:25 ` Kevin Hilman
2013-08-08 19:49 ` Dave Gerlach
2013-08-06 17:49 ` [PATCHv3 7/9] ARM: OMAP: omap_device: Add APIs to enable and idle hwmods Dave Gerlach
2013-08-08 7:05 ` Russ Dill
2013-08-08 14:26 ` Santosh Shilimkar
2013-08-06 17:49 ` [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support Dave Gerlach
2013-08-07 16:22 ` Nishanth Menon
2013-08-07 18:12 ` Dave Gerlach
2013-08-07 19:16 ` Nishanth Menon
2013-08-08 8:45 ` Russ Dill
2013-08-08 12:26 ` Nishanth Menon
2013-08-08 15:03 ` Santosh Shilimkar
2013-08-08 16:06 ` Dave Gerlach
2013-08-08 16:22 ` Nishanth Menon
2013-08-08 21:14 ` Kevin Hilman
2013-08-08 21:32 ` Nishanth Menon
2013-08-08 23:04 ` Kevin Hilman
2013-08-09 15:11 ` Nishanth Menon
2013-08-09 16:12 ` Kevin Hilman
2013-08-09 16:36 ` Nishanth Menon
2013-08-09 20:34 ` Kevin Hilman [this message]
2013-08-09 21:35 ` Nishanth Menon
2013-08-09 22:28 ` Russ Dill
2013-08-12 16:09 ` Kevin Hilman
2013-08-30 17:29 ` Vaibhav Bedia
2013-08-20 22:48 ` Paul Walmsley
2013-08-23 14:56 ` Dave Gerlach
2013-08-13 7:43 ` Russ Dill
2013-08-13 14:59 ` Kevin Hilman
2013-08-27 21:45 ` Kevin Hilman
2013-08-29 21:41 ` Dave Gerlach
2013-08-29 22:02 ` Kevin Hilman
2013-08-30 17:39 ` Vaibhav Bedia
2013-08-30 21:18 ` Kevin Hilman
2013-08-06 17:49 ` [PATCHv3 9/9] ARM: OMAP2+: AM33XX: Hookup AM33XX PM code into OMAP builds Dave Gerlach
2013-08-08 8:47 ` Russ Dill
2013-08-08 14:53 ` Santosh Shilimkar
2013-08-08 13:31 ` [PATCHv3 0/9] ARM: OMAP2+: AM33XX: Add suspend-resume support Santosh Shilimkar
2013-08-11 11:53 ` Daniel Mack
2013-08-12 18:59 ` Dave Gerlach
2013-08-13 12:39 ` Daniel Mack
2013-08-13 15:33 ` Dave Gerlach
2013-08-13 15:51 ` Daniel Mack
2013-08-19 9:23 ` Gururaja Hebbar
2013-08-19 17:47 ` Dave Gerlach
2013-08-27 20:23 ` Kevin Hilman
2013-08-29 21:30 ` Dave Gerlach
2013-08-29 21:52 ` Kevin Hilman
2013-08-29 22:20 ` Dave Gerlach
2013-08-29 22:20 ` Kevin Hilman
2013-08-29 22:43 ` Russ Dill
2013-08-29 23:02 ` Kevin Hilman
2013-09-03 17:24 ` Dave Gerlach
2013-09-04 15:01 ` Kevin Hilman
2013-09-04 15:12 ` Russ Dill
2013-09-04 15:18 ` Kevin Hilman
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=87bo5661sv.fsf@kernel.org \
--to=khilman@linaro$(echo .)org \
--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