Re: [PATCH 8/9] soundwire: intel: add wake interrupt support

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

On 7/14/20 11:50 PM, Vinod Koul wrote:
> On 01-07-20, 10:25, Pierre-Louis Bossart wrote:
>>
>>>>>> +	 * wake up master and slave so that slave can notify master
>>>>>> +	 * the wakeen event and let codec driver check codec status
>>>>>> +	 */
>>>>>> +	list_for_each_entry(slave, &bus->slaves, node) {
>>>>>> +		/*
>>>>>> +		 * discard devices that are defined in ACPI tables but
>>>>>> +		 * not physically present and devices that cannot
>>>>>> +		 * generate wakes
>>>>>> +		 */
>>>>>> +		if (slave->dev_num_sticky && slave->prop.wake_capable)
>>>>>> +			pm_request_resume(&slave->dev);
>>>>>
>>>>> Hmmm, shouldn't slave do this? would it not make sense to notify the
>>>>> slave thru callback and then slave decides to resume or not..?
>>>>
>>>> In this mode, the bus is clock-stop mode, and events are detected with level
>>>> detector tied to PCI events. The master and slave devices are all in
>>>> pm_runtime suspended states. The codec cannot make any decisions on its own
>>>> since the bus is stopped, it needs to first resume, which assumes that the
>>>> master resumes first and the enumeration re-done before it can access any of
>>>> its registers.
>>>>
>>>> By looping through the list of devices that can generate events, you end-up
>>>> first forcing the master to resume, and then each slave resumes and can
>>>> check who generated the event and what happened while suspended. if the
>>>> codec didn't generate the event it will go back to suspended mode after the
>>>> usual timeout.
>>>>
>>>> We can add a callback but that callback would only be used for Intel
>>>> solutions, but internally it would only do a pm_request_resume() since the
>>>> codec cannot make any decisions before first resuming. In other words, it
>>>> would be an Intel-specific callback that is implemented using generic resume
>>>> operations. It's probably better to keep this in Intel-specific code, no?
>>>
>>> I do not like the idea that a device would be woken up, that kind of
>>> defeats the whole idea behind the runtime pm. Waking up a device to
>>> check the events is a generic sdw concept, I don't see that as Intel
>>> specific one.
>>
>> In this case, this in an Intel-specific mode that's beyond what MIPI
>> defined. This is not the traditional in-band SoundWire wake defined in the
>> SoundWire specification. The bus is completely down, the master IP is
>> powered-off and all context lost. There is still the ability for a Slave
>> device to throw a wake as defined by MIPI in clock-stop-mode1, but this is
>> handled with a separate level detector and the wake detection is handled by
>> the PCI subsystem. On a wake, the master IP needs to be powered-up, the
>> entire bus needs to be restarted and the Slave devices re-enumerated.
> 
> Right and I would expect that Slave device would do runtime_get_sync()
> first thing in the callback. That would ensure that the Master is
> powered up, Slave is powered up, all enumeration is complete. This is
> more standard way to deal with this, we expect devices to be so we
> low powered or off so cannot do read/write unless we resume.

As shared privately with you, we don't need to deal with device PM or 
add a callback, it's enough to resume the master, which will indirectly 
restart the bus and result in devices being resumed when they report 
their interrupt status. We'll share the update from [1] in the v2.

[1] https://github.com/thesofproject/linux/pull/2247