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!
