[PATCH 3/4] soundwire: intel: exit clock stop mode on system suspend

[Pierre-Louis Bossart]

On 8/6/21 8:31 AM, Vinod Koul wrote:
> On 02-08-21, 11:28, Pierre-Louis Bossart wrote:
>>
>>
>>
>>>> 1. In above you are calling resume of child devices first and then intel
>>>> device, which sounds reverse, should you not resume intel device first
>>>> and then child (codec devices) ?
>>>>
>>>> 2. What about when resume is invoked by the core for the child devices.
>>>> That would be called in the PM resume flow, so why do it here?
>>>
>>> I realize it's a complicated sequence, it took us multiple phases to get
>>> it right. There are multiple layers between power domain, bus and driver.
>>>
>>> The .prepare phase happens before the system suspend phase. Unlike
>>> suspend, which progresses from children to parents, the .prepare is
>>> handled parent first.
>>>
>>> When we do a request_resume of the child device, by construction that
>>> also resumes the parent. In other words, if we have multiple codecs on a
>>> link, the first iteration of device_for_each_child() will already resume
>>> the parent and the first device, and the second iteration will only
>>> resume the second device.
>>>
>>> What this step does is make sure than when the codec .suspend routine is
>>> invoked, the entire bus is already back to full power. I did check
>>> privately with Rafael (CC:ed) if this sequence was legit.
>>>
>>> We did consider modifying the system suspend callback in codec drivers,
>>> so that we would do a pm_runtime resume(). This is functionally
>>> equivalent to what we are suggesting here, but we decided not to do so
>>> for two main reasons
>>>
>>> a) lots of code changes across all codecs for an Intel-specific issue
>>>
>>> b) we would need to add a flag so that codec drivers would know in which
>>> Intel-specific clock-stop mode the bus was configured. That's not so
>>> good either.
>>>
>>> It seemed simpler to use to add this .prepare step and test on the Intel
>>> clock stop mode before doing a pm_runtime_resume for all codecs.
> 
> Ack, the code looks neat. But glancing at it, reader might get confused
> about the sequencing done here.. It is not very obvious, so consider
> adding this to changelog or driver comments. It will be helpful

Yep, even in internal reviews this was far from straightforward to
explain. I added comments but I can certainly try to explain more.

>>
>> Note that we could invert the two parts and do a parent resume first,
>> and a loop for all children second. It's completely equivalent, but
>> might be less convoluted to understand without any implicit behavior
>> assumed.
> 
> Agree, it would be redundant as PM core would take care of it. maybe
> add a comment so that it is explicit

Will add comments as well.

Note that I have another lead to further improve suspend-resume, running
stress tests on thousands of cycles atm. I'll wait until we have more
results to resubmit this series.

Thanks for the reviews!