Re: [PATCH 1/8] soundwire: bus_type: add master_device/driver support

[Vinod Koul <vkoul@kernel.org>]

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.

-- 
~Vinod