On 23/01/2023 16:38, Pierre-Louis Bossart wrote:
On 1/23/23 10:08, Richard Fitzgerald wrote:
On 23/01/2023 15:50, Pierre-Louis Bossart wrote:
On 1/23/23 08:53, Charles Keepax wrote:
On Fri, Jan 20, 2023 at 10:20:50AM -0600, Pierre-Louis Bossart wrote:
On 1/20/23 03:59, Charles Keepax wrote:
On Thu, Jan 19, 2023 at 11:12:04AM -0600, Pierre-Louis Bossart wrote:
There should be an explanation and something checking that both
are not
used concurrently.
I will try to expand the explanation a litte, but I dont see any
reason to block calling both handlers, no ill effects would come
for a driver having both and it is useful if any soundwire
specific steps are needed that arn't on other control buses.
I think it's problematic if the peripheral tries to wake-up the manager
from clock-stop with both an in-band wake (i.e. drive the data line
high) and a separate GPIO-based interrupt. It's asking for trouble
IMHO.
We spent hours in the MIPI team to make sure there were no races
between
the manager-initiated restarts and peripheral-initiated restarts,
adding
a 3rd mechanism in the mix gives me a migraine already.
Apologies but I am struggling see why this has any bearing on
the case of a device that does both an in-band and out-of-band
wake. The code we are adding in this patch will only be called in the
in-band case. handle_nested_irq doesn't do any hardware magic or
schedule any threads, it just calls a function that was provided
when the client called request_threaded_irq. The only guarantee
of atomicity you have on the interrupt_callback is sdw_dev_lock
and that is being held across both calls after the patch.
Could you be a little more specific on what you mean by this
represents a 3rd mechanism, to me this isn't a new mechanism just
an extra callback? Say for example this patch added an
interrupt_callback_early to sdw_slave_ops that is called just
before interrupt_callback.
Well, the main concern is exiting the clock-stop. That is handled by the
manager and could be done
a) as the result of the framework deciding that something needs to be
done (typically as a result of user/applications starting a stream)
b) by the device with an in-band wake in case of e.g. jack detection or
acoustic events detected
c) same as b) but with a separate out-of-band interrupt.
I'd like to make sure b) and c) are mutually-exclusive options, and that
the device will not throw BOTH an in-band wake and an external interrupt.
Why would it be a problem if the device did (b) and (c)?
(c) is completely invisible to the SoundWire core and not something
that it has to handle. The handler for an out-of-band interrupt must
call pm_runtime_get_sync() or pm_runtime_resume_and_get() and that
would wake its own driver and the host controller.
The Intel hardware has a power optimization for the clock-stop, which
leads to different paths to wake the system. The SoundWire IP can deal
with the data line staying high, but in the optimized mode the wakes are
signaled as DSP interrupts at a higher level. That's why we added this
intel_link_process_wakeen_event() function called from
hda_dsp_interrupt_thread().
So yes on paper everything would work nicely, but that's asking for
trouble with races left and right. In other words, unless you have a
Wake up from a hard INT is simply a runtime_resume of the codec driver.
That is no different from ASoC runtime resuming the driver to perform
some audio activity, or to access a volatile register. An event caused
a runtime-resume - the driver and the host controller must resume.
The Intel code _must_ be able to safely wakeup from clock-stop if
something runtime-resumes the codec driver. ASoC relies on that, and
pm_runtime would be broken if that doesn't work.
very good reason for using two wake-up mechanisms, pick a single one.
(a) and (c) are very similar in that all the exit is handled by
pm_runtime so I am not worried too much. I do worry about paths that
were never tested and never planned for.
