Re: [PATCH 2/2] soundwire: Intel: stop sdw clock in system suspend

[Pierre-Louis Bossart]

>>>> --- a/drivers/soundwire/intel_auxdevice.c
>>>> +++ b/drivers/soundwire/intel_auxdevice.c
>>>> @@ -669,7 +669,8 @@ static int __maybe_unused intel_suspend(struct
>>> device *dev)
>>>>           return 0;
>>>>       }
>>>>
>>>> -    ret = sdw_intel_stop_bus(sdw, false);
>>>> +    /* No need to keep the SoundWire clock active in system suspend */
>>>> +    ret = sdw_intel_stop_bus(sdw, true);
>>>
>>> erm, are you sure about this change?
>>>
>>> What this does is stop the SoundWire clock before entering system suspend.
>>> Is this needed? The whole point of system suspend is that the host will stop
>>> operating completely.
> 
> Not necessarily. There are multiple types of system suspend, like
> suspend-to-RAM. The system doesn't always shut down.

Let's be clear: all types of system suspend do not use the clock stop mode, at least on Intel platforms.

The clock stop mode is ONLY used in the pm_runtime suspend.

The main reason for this is that the host cannot keep a detector alive to restart the bus at the request of a peripheral.
As a result, the jack detection only works in pm_runtime suspend and NOT in system suspend. It's been the default since we introduced support of clock stop...

Note that in case where the peripheral is not capable of generating a wake, the clock stop is not used either. No need to keep that detector alive on the host if it's never going to see a wake.

> Also depends on the OS: Android seems to use system suspend as a deep
> sleep state.
> 
>>
>> Yes, that's what we need. The purpose is to inform the Peripheral to go
>> into the deserved clock stop mode before system suspend.

Sorry but this isn't what this patch does. 

I think you are confusing power savings in clock stop mode with the ability for the codec to enter a lower power in system suspend.

If you do need the ability to put the peripheral in a lower power mode, it's perfectly acceptable, but that can be done in the system suspend routine of the peripheral driver. You do not need or want to piggy-back on the clock stop mechanism which isn't used in system suspend.

>>> There is in theory no need to stop the clock because the clock restart capability
>>> will not be used.
> 
> As Bard says, the peripherals should be informed that the clock will be
> stopped. They at least should have the opportunity to act on that. And
> they could still be powered across one of the lighter forms of system
> suspend.

Again it'd be fine to reach a lower power for the peripheral or keep some power rails active, but that's orthogonal to the clock stop.

>>> Think for example of jack detection, it's supported in pm_runtime suspend
>>> with the SoundWire interrupt capability, but not in system suspend.
>>
>> An example that we need clock stop1 in system suspend is to keep the amp's
>> firmware in system suspend. In other words, we don't want to reload the
>> firmware after system resume

If you want to use lower power modes in system suspend, that's fine. The suspend/resume routines can be modified to introduce lower power modes on the peripheral side.
I am not disagreeing with the concept of lowering power consumption in system suspend, just on the mechanisms that you trying to use.

But keeping the context on the peripheral is one bridge too far I am afraid. Intel platforms perform a bus reset in all cases, including in clock-stop modes, that was the hardware guidance. That means that no matter what you do prior to pm_runtime and system suspend the peripheral WILL lose context.

To rehash the different points:
- The only significant difference between pm_runtime and system suspend is the wake detection capability which is only supported in the former case. 
- On Intel platforms, context is lost on the peripheral in all resume routines because of the bus reset, the latency on resume is pretty much the same in both cases
- The peripheral driver can introduce a lower-power mode in system suspend by changing the system suspend routine.