diff mbox series

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

Message ID 20200623173546.21870-9-yung-chuan.liao@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series soundwire: intel: revisit SHIM programming | expand

Commit Message

Bard Liao June 23, 2020, 5:35 p.m. UTC
From: Rander Wang <rander.wang@intel.com>

When system is suspended in clock stop mode on intel platforms, both
master and slave are in clock stop mode and soundwire bus is taken
over by a glue hardware. The bus message for jack event is processed
by this glue hardware, which will trigger an interrupt to resume audio
pci device. Then audio pci driver will resume soundwire master and slave,
transfer bus ownership to master, finally slave will report jack event
to master and codec driver is triggered to check jack status.

if a slave has been attached to a bus, the slave->dev_num_sticky
should be non-zero, so we can check this value to skip the
ghost devices defined in ACPI table but not populated in hardware.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel.c      | 48 +++++++++++++++++++++++++++++++++-
 drivers/soundwire/intel.h      |  1 +
 drivers/soundwire/intel_init.c | 22 ++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)

Comments

Vinod Koul June 30, 2020, 4:51 p.m. UTC | #1
On 24-06-20, 01:35, Bard Liao wrote:
> From: Rander Wang <rander.wang@intel.com>
> 
> When system is suspended in clock stop mode on intel platforms, both
> master and slave are in clock stop mode and soundwire bus is taken
> over by a glue hardware. The bus message for jack event is processed
> by this glue hardware, which will trigger an interrupt to resume audio
> pci device. Then audio pci driver will resume soundwire master and slave,
> transfer bus ownership to master, finally slave will report jack event
> to master and codec driver is triggered to check jack status.
> 
> if a slave has been attached to a bus, the slave->dev_num_sticky
> should be non-zero, so we can check this value to skip the
> ghost devices defined in ACPI table but not populated in hardware.
> 
> Signed-off-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/intel.c      | 48 +++++++++++++++++++++++++++++++++-
>  drivers/soundwire/intel.h      |  1 +
>  drivers/soundwire/intel_init.c | 22 ++++++++++++++++
>  3 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 06c553d94890..22d9fd3e34fa 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -13,6 +13,7 @@
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
>  #include <sound/pcm_params.h>
> +#include <linux/pm_runtime.h>
>  #include <sound/soc.h>
>  #include <linux/soundwire/sdw_registers.h>
>  #include <linux/soundwire/sdw.h>
> @@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
>  	return ret;
>  }
>  
> -static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
> +static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)

why drop __maybe?

>  {
>  	void __iomem *shim = sdw->link_res->shim;
>  	unsigned int link_id = sdw->instance;
> @@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +int intel_master_process_wakeen_event(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sdw_intel *sdw;
> +	struct sdw_bus *bus;
> +	struct sdw_slave *slave;
> +	void __iomem *shim;
> +	u16 wake_sts;
> +
> +	sdw = platform_get_drvdata(pdev);
> +	bus = &sdw->cdns.bus;
> +
> +	if (bus->prop.hw_disabled) {
> +		dev_dbg(dev,
> +			"SoundWire master %d is disabled, ignoring\n",
> +			bus->link_id);

single line pls

> +		return 0;
> +	}
> +
> +	shim = sdw->link_res->shim;
> +	wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
> +
> +	if (!(wake_sts & BIT(sdw->instance)))
> +		return 0;
> +
> +	/* disable WAKEEN interrupt ASAP to prevent interrupt flood */
> +	intel_shim_wake(sdw, false);

when & where is this enabled?

> +
> +	/*
> +	 * 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..?
Pierre-Louis Bossart June 30, 2020, 5:18 p.m. UTC | #2
On 6/30/20 11:51 AM, Vinod Koul wrote:
> On 24-06-20, 01:35, Bard Liao wrote:
>> From: Rander Wang <rander.wang@intel.com>
>>
>> When system is suspended in clock stop mode on intel platforms, both
>> master and slave are in clock stop mode and soundwire bus is taken
>> over by a glue hardware. The bus message for jack event is processed
>> by this glue hardware, which will trigger an interrupt to resume audio
>> pci device. Then audio pci driver will resume soundwire master and slave,
>> transfer bus ownership to master, finally slave will report jack event
>> to master and codec driver is triggered to check jack status.
>>
>> if a slave has been attached to a bus, the slave->dev_num_sticky
>> should be non-zero, so we can check this value to skip the
>> ghost devices defined in ACPI table but not populated in hardware.
>>
>> Signed-off-by: Rander Wang <rander.wang@intel.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> ---
>>   drivers/soundwire/intel.c      | 48 +++++++++++++++++++++++++++++++++-
>>   drivers/soundwire/intel.h      |  1 +
>>   drivers/soundwire/intel_init.c | 22 ++++++++++++++++
>>   3 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index 06c553d94890..22d9fd3e34fa 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/io.h>
>>   #include <linux/platform_device.h>
>>   #include <sound/pcm_params.h>
>> +#include <linux/pm_runtime.h>
>>   #include <sound/soc.h>
>>   #include <linux/soundwire/sdw_registers.h>
>>   #include <linux/soundwire/sdw.h>
>> @@ -436,7 +437,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
>>   	return ret;
>>   }
>>   
>> -static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>> +static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
> 
> why drop __maybe?

the __maybe was used in previous patches to avoid throwing 'defined but 
not used' errors.

In this patch this function is actually used so there's no longer a 
reason to keep it.

>>   {
>>   	void __iomem *shim = sdw->link_res->shim;
>>   	unsigned int link_id = sdw->instance;
>> @@ -1337,6 +1338,51 @@ static int intel_master_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +int intel_master_process_wakeen_event(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sdw_intel *sdw;
>> +	struct sdw_bus *bus;
>> +	struct sdw_slave *slave;
>> +	void __iomem *shim;
>> +	u16 wake_sts;
>> +
>> +	sdw = platform_get_drvdata(pdev);
>> +	bus = &sdw->cdns.bus;
>> +
>> +	if (bus->prop.hw_disabled) {
>> +		dev_dbg(dev,
>> +			"SoundWire master %d is disabled, ignoring\n",
>> +			bus->link_id);
> 
> single line pls

ok

>> +		return 0;
>> +	}
>> +
>> +	shim = sdw->link_res->shim;
>> +	wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
>> +
>> +	if (!(wake_sts & BIT(sdw->instance)))
>> +		return 0;
>> +
>> +	/* disable WAKEEN interrupt ASAP to prevent interrupt flood */
>> +	intel_shim_wake(sdw, false);
> 
> when & where is this enabled?

in follow-up patches where the clock-stop mode is enabled.

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

		ret = sdw_cdns_clock_stop(cdns, true);
		if (ret < 0) {
			dev_err(dev, "cannot enable clock stop on suspend\n");
			return ret;
		}

		ret = sdw_cdns_enable_interrupt(cdns, false);
		if (ret < 0) {
			dev_err(dev, "cannot disable interrupts on suspend\n");
			return ret;
		}

		ret = intel_link_power_down(sdw);
		if (ret) {
			dev_err(dev, "Link power down failed: %d", ret);
			return ret;
		}

		intel_shim_wake(sdw, true);

>> +
>> +	/*
>> +	 * 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?
Vinod Koul July 1, 2020, 5:56 a.m. UTC | #3
On 30-06-20, 12:18, Pierre-Louis Bossart wrote:
> > > +		return 0;
> > > +	}
> > > +
> > > +	shim = sdw->link_res->shim;
> > > +	wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
> > > +
> > > +	if (!(wake_sts & BIT(sdw->instance)))
> > > +		return 0;
> > > +
> > > +	/* disable WAKEEN interrupt ASAP to prevent interrupt flood */
> > > +	intel_shim_wake(sdw, false);
> > 
> > when & where is this enabled?
> 
> in follow-up patches where the clock-stop mode is enabled.

ok

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

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?
Pierre-Louis Bossart July 1, 2020, 3:25 p.m. UTC | #4
>>>> +	 * 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?
Vinod Koul July 15, 2020, 4:50 a.m. UTC | #5
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.

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

The point which is not clear to me if why do we need a specific
sequence. Above sequence should work for you, if not I would like to
understand why.

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

If we are worried about Slave device remembering then we should save
state in device context. But i think that slave should *always* perform
runtime resume as a first step in the callback before they would do any
bus/device ops. After all, the callback is wake from low power state

> 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?
>
Pierre-Louis Bossart July 15, 2020, 2:22 p.m. UTC | #6
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
diff mbox series

Patch

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 06c553d94890..22d9fd3e34fa 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -13,6 +13,7 @@ 
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <sound/pcm_params.h>
+#include <linux/pm_runtime.h>
 #include <sound/soc.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/soundwire/sdw.h>
@@ -436,7 +437,7 @@  static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
 	return ret;
 }
 
-static void __maybe_unused intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
+static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 {
 	void __iomem *shim = sdw->link_res->shim;
 	unsigned int link_id = sdw->instance;
@@ -1337,6 +1338,51 @@  static int intel_master_remove(struct platform_device *pdev)
 	return 0;
 }
 
+int intel_master_process_wakeen_event(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sdw_intel *sdw;
+	struct sdw_bus *bus;
+	struct sdw_slave *slave;
+	void __iomem *shim;
+	u16 wake_sts;
+
+	sdw = platform_get_drvdata(pdev);
+	bus = &sdw->cdns.bus;
+
+	if (bus->prop.hw_disabled) {
+		dev_dbg(dev,
+			"SoundWire master %d is disabled, ignoring\n",
+			bus->link_id);
+		return 0;
+	}
+
+	shim = sdw->link_res->shim;
+	wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
+
+	if (!(wake_sts & BIT(sdw->instance)))
+		return 0;
+
+	/* disable WAKEEN interrupt ASAP to prevent interrupt flood */
+	intel_shim_wake(sdw, false);
+
+	/*
+	 * 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);
+	}
+
+	return 0;
+}
+
 static struct platform_driver sdw_intel_drv = {
 	.probe = intel_master_probe,
 	.remove = intel_master_remove,
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index bf127c88eb51..4ea3d262d249 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -47,5 +47,6 @@  struct sdw_intel {
 #define SDW_INTEL_QUIRK_MASK_BUS_DISABLE      BIT(1)
 
 int intel_master_startup(struct platform_device *pdev);
+int intel_master_process_wakeen_event(struct platform_device *pdev);
 
 #endif /* __SDW_INTEL_LOCAL_H */
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 63b3beda443d..eff4e385bb59 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -415,5 +415,27 @@  void sdw_intel_exit(struct sdw_intel_ctx *ctx)
 }
 EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
 
+void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx)
+{
+	struct sdw_intel_link_res *link;
+	u32 link_mask;
+	int i;
+
+	if (!ctx->links)
+		return;
+
+	link = ctx->links;
+	link_mask = ctx->link_mask;
+
+	/* Startup SDW Master devices */
+	for (i = 0; i < ctx->count; i++, link++) {
+		if (!(link_mask & BIT(i)))
+			continue;
+
+		intel_master_process_wakeen_event(link->pdev);
+	}
+}
+EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT);
+
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("Intel Soundwire Init Library");