Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	ranjani.sridharan@linux.intel.com, hui.wang@canonical.com,
	broonie@kernel.org, srinivas.kandagatla@linaro.org,
	jank@cadence.com, mengdong.lin@intel.com,
	slawomir.blauciak@intel.com, sanyog.r.kale@intel.com,
	Bard Liao <yung-chuan.liao@linux.intel.com>,
	rander.wang@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 8/9] soundwire: intel: add wake interrupt support
Date: Wed, 1 Jul 2020 11:26:54 +0530
Message-ID: <20200701055654.GW2599@vkoul-mobl> (raw)
In-Reply-To: <a69aa5d0-613a-6ef6-9263-378b1e99770a@linux.intel.com>

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?

-- 
~Vinod

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 17:35 [PATCH 0/9] soundwire: intel: revisit SHIM programming Bard Liao
2020-06-23 17:35 ` [PATCH 1/9] soundwire: intel: reuse code for wait loops to set/clear bits Bard Liao
2020-06-23 17:35 ` [PATCH 2/9] soundwire: intel: revisit SHIM programming sequences Bard Liao
2020-06-23 17:35 ` [PATCH 3/9] soundwire: intel: introduce a helper to arm link synchronization Bard Liao
2020-06-23 17:35 ` [PATCH 4/9] soundwire: intel: introduce helper for " Bard Liao
2020-06-23 17:35 ` [PATCH 5/9] soundwire: intel_init: add implementation of sdw_intel_enable_irq() Bard Liao
2020-06-23 17:35 ` [PATCH 6/9] soundwire: intel_init: use EXPORT_SYMBOL_NS Bard Liao
2020-06-23 17:35 ` [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads Bard Liao
2020-06-30 16:24   ` Vinod Koul
2020-06-30 16:46     ` Pierre-Louis Bossart
2020-07-01  5:42       ` Vinod Koul
2020-07-02  7:35         ` Liao, Bard
2020-07-02 15:01           ` Pierre-Louis Bossart
2020-06-23 17:35 ` [PATCH 8/9] soundwire: intel: add wake interrupt support Bard Liao
2020-06-30 16:51   ` Vinod Koul
2020-06-30 17:18     ` Pierre-Louis Bossart
2020-07-01  5:56       ` Vinod Koul [this message]
2020-07-01 15:25         ` Pierre-Louis Bossart
2020-06-23 17:35 ` [PATCH 9/9] Soundwire: intel_init: save Slave(s) _ADR info in sdw_intel_ctx Bard Liao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200701055654.GW2599@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=bard.liao@intel.com \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hui.wang@canonical.com \
    --cc=jank@cadence.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mengdong.lin@intel.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=rander.wang@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=sanyog.r.kale@intel.com \
    --cc=slawomir.blauciak@intel.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=tiwai@suse.de \
    --cc=yung-chuan.liao@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git