[PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

[Kevin Hilman]

Nishanth Menon <nm at ti.com> writes:

> On 08/09/2013 11:12 AM, Kevin Hilman wrote:
>> Nishanth Menon <nm at ti.com> writes:
>>
>>> On 08/08/2013 06:04 PM, Kevin Hilman wrote:
>>>> Nishanth Menon <nm at ti.com> writes:
>>>>
>>>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote:
>>>>>> Dave Gerlach <d-gerlach at 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