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

[Nishanth Menon]

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?
[...]

-- 
Regards,
Nishanth Menon