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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>> 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.

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.

	if ((clock_stop_quirks & SDW_INTEL_CLK_STOP_BUS_RESET) ||
	    !clock_stop_quirks) {

		/* resume parent first */
		ret = pm_request_resume(dev);
		if (ret < 0)
			dev_err(dev, "%s: pm_request_resume failed: %d\n", __func__, ret);

		/*
		 * resume all children next.
		 * if there are no children on this link,
		 * this is a no-op
		 */
		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);

		if (ret < 0)
			dev_err(dev, "%s: intel_resume_child_device failed: %d\n", __func__,
ret);
	}