Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support
From: Pierre-Louis Bossart
Date: Wed Mar 11 2020 - 10:52:24 EST
On 3/11/20 1:36 AM, Vinod Koul wrote:
On 06-03-20, 09:40, Pierre-Louis Bossart wrote:
Why do you need a extra driver for this. Do you have another set of
device object and driver for DSP code? But you do manage that, right?
I am proposing to simplify the device model here and have only one
device (SOF PCI) and driver (SOF PCI driver), which is created by actual
bus (PCI here) as you have in rest of the driver like HDA, DSP etc.
I have already recommended is to make the int-sdw a module which is
invoked by SOF PCI driver code (thereby all code uses SOF PCI device and
SOF PCI driver) directly. The DSP in my time for skl was a separate
module but used the parent objects.
The SOF sdw init (the place where sdw routines are invoked after DSP
load) can call sdw_probe and startup. Based on DSP sequencing you can
call these functions directly without waiting for extra device to be
probed etc.
I feel your flows will be greatly simplified as a result of this.
Not at all, no. This is not a simplification but an extremely invasive
proposal.
The parent-child relationship is extremely useful for power management, and
guarantees that the PCI device remains on while one or more of the masters
are used, and conversely can suspend when all links are idle. I currently
don't need to do anything, it's all taken care of by the framework.
If I have to do all the power management at the PCI device level, then I
will need to keep track of which links are currently active. All these links
are used independently, so it's racy as hell to keep track of the usage when
the pm framework already does so quite elegantly. You really want to use the
pm_runtime_get/put refcount for each master device, not manage them from the
PCI level.
Not at all, you still can call pm_runtime_get/put() calls in sdw module
for PCI device. That doesn't change at all.
Only change is for suspend/resume you have callbacks from PCI driver
rather than pm core.
There are two other related issues that you didn't mention.
the ASoC layer does require a driver with a 'name' for the components
registered with the master device. So if you don't have a driver for the
master device, the DAIs will be associated with the PCI device.
But the ASoC core does make pm_runtime calls on its own,
soc_pcm_open(struct snd_pcm_substream *substream)
{
...
for_each_rtd_components(rtd, i, component)
pm_runtime_get_sync(component->dev);
and if the device that's associated with the DAI is the PCI device, then
that will not result in the relevant master IP being activated, only the PCI
device refcount will be increased - meaning there is no hook that would tell
the PCI layer to turn on a specific link.
What you are recommending would be an all-or-nothing solution with all links
on or all links off, which beats the purpose of having independent
link-level power management.
Why can't you use dai .startup callback for this?
The ASoC core will do pm_runtime calls that will ensure PCI device is
up, DSP firmware downloaded and running.
You can use .startup() to turn on your link and .shutdown to turn off
the link.
There are multiple dais per link, and multiple Slave per link, so we
would have to refcount and track active dais to understand when the link
needs to be turned on/off. It's a duplication of what the pm framework
can do at the device/link level, and will likely introduce race conditions.
Not to mention that we'd need to introduce workqueues to turn the link
off with a delay, with pm_runtime_put_autosuspend() does for free.
Linux is all about frameworks. For power management, we shall use the
power management framework, not reinvent it.