Message-ID: <81a33798-0667-9a4f-9916-eb157ad5514d@linux.intel.com>
Date:   Wed, 1 Jul 2020 10:25:02 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Bard Liao <yung-chuan.liao@...ux.intel.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
        jank@...ence.com, srinivas.kandagatla@...aro.org,
        rander.wang@...ux.intel.com, ranjani.sridharan@...ux.intel.com,
        hui.wang@...onical.com, sanyog.r.kale@...el.com,
        slawomir.blauciak@...el.com, mengdong.lin@...el.com,
        bard.liao@...el.com
Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support


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

We trigger that sequence with a loop that calls pm_request_resume() for 
all Slave devices that are present and exposed in their properties that 
they are wake-capable. As you rightly said in your comments, this will 
result a nice wake-up handled by the pm_runtime framework in the right 
sequence (DSP first, then SoundWire master then Slave devices).

You will see in follow-up patches that the master IP can be configured 
in different clock stop modes, depending on the needs (power/latency 
compromise typically). When the more traditional SoundWire wake is used, 
we do not use this sequence at all.

> I would like to see a generic callback for the devices and let devices
> do the resume part, that is standard operating principle when we deal
> with suspended devices. If the device thinks they need to resume, they
> will do the runtime resume, check the status and sleep if not
> applicable. Since we have set the parents correctly, any resume
> operation for slaves would wake master up as well...
> 
> I do not see a need for intel driver to resume slave devices here, or
> did I miss something?

Yes, the part "If the device thinks they need to resume" is not quite 
right. The Slave device cannot access its registers before fully 
resuming, which in this case means a re-enumeration, so cannot "think" 
or make a decision on its own. That's the reason why we force them to 
resume, since it's the first step they would need to do anyways, and 
then if they don't have anything to do they go back to idle after a 
timeout. I invite you to see the suspend/resume sequences in codec 
drivers where you will see regmap access moves to cache-only on suspend 
and full access restored on resume, along with a hardware sync.

I see your point and I really think we are talking about a similar 
sequence, but we simplified your idea since there's no possibility of 
making a decision before Slave devices resume.

The only optimization we did here is that we only resume Slave devices 
than can generate a wake, and filter out the 'ghost' devices that are 
described in ACPI tables but don't physically exist.

Does this help?

Powered by blists - more mailing lists