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

[Pierre-Louis Bossart]

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