| Message ID | 20200227223206.5020-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: remove platform devices, add SOF interfaces | expand |
On Thu, Feb 27, 2020 at 04:31:59PM -0600, Pierre-Louis Bossart wrote: > In the existing SoundWire code, Master Devices are not explicitly > represented - only SoundWire Slave Devices are exposed (the use of > capital letters follows the SoundWire specification conventions). > > The SoundWire Master Device provides the clock, synchronization > information and command/control channels. When multiple links are > supported, a Controller may expose more than one Master Device; they > are typically embedded inside a larger audio cluster (be it in an > SOC/chipset or an external audio codec), and we need to describe it > using the Linux device and driver model. This will allow for > configuration functions to account for external dependencies such as > power rails, clock sources or wake-up mechanisms. This transition will > also allow for better sysfs support without the reference count issues > mentioned in the initial reviews. > > In this patch, we convert the existing code to use an explicit > sdw_slave_type, then define new objects (sdw_master_device and > sdw_master_driver). > > A parent (such as the Intel audio controller or its equivalent on > Qualcomm devices) would use sdw_master_device_add() to create the > device, passing a driver name as a parameter. The master device would > be released when device_unregister() is invoked by the parent. > > Note that since there is no standard for the Master host-facing > interface, so the bus matching relies on a simple string matching (as > previously done with platform devices). > > The 'Master Device' driver exposes callbacks for > probe/startup/shutdown/remove/process_wake. The startup and process > wake need to be called by the parent directly (using wrappers), while > the probe/shutdown/remove are handled by the SoundWire bus core upon > device creation and release. > > Additional callbacks will be added in the future for e.g. autonomous > clock stop modes. > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/Makefile | 2 +- > drivers/soundwire/bus_type.c | 141 +++++++++++++++++++++++++++-- > drivers/soundwire/master.c | 100 ++++++++++++++++++++ > drivers/soundwire/slave.c | 7 +- > include/linux/soundwire/sdw.h | 76 ++++++++++++++++ > include/linux/soundwire/sdw_type.h | 36 +++++++- > 6 files changed, 351 insertions(+), 11 deletions(-) > create mode 100644 drivers/soundwire/master.c As you are adding new sysfs files here, is there a follow-on patch for Documentation/ABI/ updates? thanks, greg k-h
> As you are adding new sysfs files here, is there a follow-on patch for > Documentation/ABI/ updates? Yes that's the plan. The original patches for sysfs were submitted as an RFC: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/148699.html Most of the sysfs entries would mirror the value of _DSD properties, but that's still very useful to check platform integration issues. Some of the DSDT blocks are overwritten at run-time depending on BIOS menu selections, so it's impossible to figure out what the firmware exposes to the OS just by looking at the DSDT contents extracted with acpica tools.
On 27-02-20, 16:31, Pierre-Louis Bossart wrote: > In the existing SoundWire code, Master Devices are not explicitly > represented - only SoundWire Slave Devices are exposed (the use of > capital letters follows the SoundWire specification conventions). > > The SoundWire Master Device provides the clock, synchronization > information and command/control channels. When multiple links are > supported, a Controller may expose more than one Master Device; they > are typically embedded inside a larger audio cluster (be it in an > SOC/chipset or an external audio codec), and we need to describe it > using the Linux device and driver model. This will allow for > configuration functions to account for external dependencies such as > power rails, clock sources or wake-up mechanisms. This transition will I dont not see that as a soundwire issue. The external dependencies should be handled as any device would do in Linux kernel with subsystem specific things for soundwire mechanisms like wake-up Intel has a big controller with HDA, DSP and Soundwire clubbed together, I dont think we should burden the susbstem due to hw design > also allow for better sysfs support without the reference count issues > mentioned in the initial reviews. > > In this patch, we convert the existing code to use an explicit > sdw_slave_type, then define new objects (sdw_master_device and > sdw_master_driver). Thanks for sdw_master_device, that is required and fully agreed upon. What is not agreed is the sdw_master_driver. We do not need that. As we have discussed your proposal with Greg and aligned (quoting that here) on following device model for Intel and ARM: > - For DT cases we will have: > -> soundwire DT device (soundwire DT driver) > -> soundwire master device > -> soundwire slave device (slave drivers) > - For Intel case, you would have: > -> HDA PCI device (SOF driver + soundwire module) > -> soundwire master device > -> soundwire slave device (slave drivers) But you have gone ahead and kept the sdw_master_driver which does not fit into rest of the world except Intel. I think I am okay with rest of proposal, except this one, so can you remove this and we can make progress. This issue is lingering since Oct! > A parent (such as the Intel audio controller or its equivalent on > Qualcomm devices) would use sdw_master_device_add() to create the > device, passing a driver name as a parameter. The master device would > be released when device_unregister() is invoked by the parent. We already have a DT driver for soundwire master! We dont need another layer which does not add value! > Note that since there is no standard for the Master host-facing > interface, so the bus matching relies on a simple string matching (as > previously done with platform devices). > > The 'Master Device' driver exposes callbacks for > probe/startup/shutdown/remove/process_wake. The startup and process > wake need to be called by the parent directly (using wrappers), while > the probe/shutdown/remove are handled by the SoundWire bus core upon > device creation and release. these are added to handle intel DSP and sequencing issue, rest of the world does not have these issues and does not needs them! > Additional callbacks will be added in the future for e.g. autonomous > clock stop modes. Yes these would be required, these can be added in sdw_master_device too, I dont see them requiring a dummy driver layer.. > @@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev) > slave->probed = true; > complete(&slave->probe_complete); > > - dev_dbg(dev, "probe complete\n"); > - This does not seem to belong to this patch. > +struct device_type sdw_master_type = { > + .name = "soundwire_master", > + .release = sdw_master_device_release, > +}; > + > +struct sdw_master_device > +*sdw_master_device_add(const char *master_name, > + struct device *parent, > + struct fwnode_handle *fwnode, > + int link_id, > + void *pdata) > +{ > + struct sdw_master_device *md; > + int ret; > + > + md = kzalloc(sizeof(*md), GFP_KERNEL); > + if (!md) > + return ERR_PTR(-ENOMEM); > + > + md->link_id = link_id; > + md->pdata = pdata; > + md->master_name = master_name; should we not allocate the memory here for master_name? > + > + init_completion(&md->probe_complete); > + > + md->dev.parent = parent; > + md->dev.fwnode = fwnode; > + md->dev.bus = &sdw_bus_type; > + md->dev.type = &sdw_master_type; > + md->dev.dma_mask = md->dev.parent->dma_mask; > + dev_set_name(&md->dev, "sdw-master-%d", md->link_id); why do we need master_name if we are setting this here? > + > + ret = device_register(&md->dev); > + if (ret) { > + dev_err(parent, "Failed to add master: ret %d\n", ret); > + /* > + * On err, don't free but drop ref as this will be freed > + * when release method is invoked. > + */ > + put_device(&md->dev); > + return ERR_PTR(-ENOMEM); ENOMEM? > +int sdw_master_device_startup(struct sdw_master_device *md) > +{ > + struct sdw_master_driver *mdrv; > + struct device *dev; > + int ret = 0; > + > + if (IS_ERR_OR_NULL(md)) > + return -EINVAL; > + > + dev = &md->dev; > + mdrv = drv_to_sdw_master_driver(dev->driver); > + > + if (mdrv && mdrv->startup) > + ret = mdrv->startup(md); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(sdw_master_device_startup); who invokes this and when, can you add kernel-doc style documentation to all APIs exported > +int sdw_master_device_process_wake_event(struct sdw_master_device *md) > +{ > + struct sdw_master_driver *mdrv; > + struct device *dev; > + int ret = 0; > + > + if (IS_ERR_OR_NULL(md)) > + return -EINVAL; > + > + dev = &md->dev; > + mdrv = drv_to_sdw_master_driver(dev->driver); > + > + if (mdrv && mdrv->process_wake_event) > + ret = mdrv->process_wake_event(md); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event); Documentation required > +/** > + * struct sdw_master_device - SoundWire 'Master Device' representation > + * > + * @dev: Linux device for this Master > + * @master_name: Linux driver name > + * @driver: Linux driver for this Master (set by SoundWire core during probe) > + * @probe_complete: used by parent if synchronous probe behavior is needed > + * @link_id: link index as defined by MIPI DisCo specification > + * @pm_runtime_suspended: flag to restore pm_runtime state after system resume > + * @pdata: private data typically provided with sdw_master_device_add() > + */ > + > +struct sdw_master_device { > + struct device dev; > + const char *master_name; > + struct sdw_master_driver *driver; > + struct completion probe_complete; > + int link_id; > + bool pm_runtime_suspended; why not use runtime_pm apis like pm_runtime_suspended() > +/** > + * sdw_master_device_add() - create a Linux Master Device representation. > + * > + * @master_name: Linux driver name > + * @parent: the parent Linux device (e.g. a PCI device) > + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent) > + * @link_id: link index as defined by MIPI DisCo specification > + * @pdata: private data (e.g. register base, offsets, platform quirks, etc). > + */ > +struct sdw_master_device > +*sdw_master_device_add(const char *master_name, > + struct device *parent, > + struct fwnode_handle *fwnode, > + int link_id, > + void *pdata); > + > +/** > + * sdw_master_device_startup() - startup hardware > + * > + * @md: Linux Soundwire master device Please add more useful comments like when this API would be invoked and what shall be expected outcome > + */ > +int sdw_master_device_startup(struct sdw_master_device *md); > + > +/** > + * sdw_master_device_process_wake_event() - handle external wake > + * event, e.g. handled at the PCI level > + * > + * @md: Linux Soundwire master device > + */ > +int sdw_master_device_process_wake_event(struct sdw_master_device *md); > + If you look at existing headers the documentation is in C files for APIs, so can you move them over. When adding stuff please look at the rest of the code as an example.
On 3/2/20 11:41 PM, Vinod Koul wrote: > On 27-02-20, 16:31, Pierre-Louis Bossart wrote: >> In the existing SoundWire code, Master Devices are not explicitly >> represented - only SoundWire Slave Devices are exposed (the use of >> capital letters follows the SoundWire specification conventions). >> >> The SoundWire Master Device provides the clock, synchronization >> information and command/control channels. When multiple links are >> supported, a Controller may expose more than one Master Device; they >> are typically embedded inside a larger audio cluster (be it in an >> SOC/chipset or an external audio codec), and we need to describe it >> using the Linux device and driver model. This will allow for >> configuration functions to account for external dependencies such as >> power rails, clock sources or wake-up mechanisms. This transition will > > I dont not see that as a soundwire issue. The external dependencies > should be handled as any device would do in Linux kernel with subsystem > specific things for soundwire mechanisms like wake-up There is nothing in the SoundWire specification that tells how a controller/master should interface with the rest of the SoC. There are two ways of handling wake-ups a. 'normal' wakes handled internally by the master - that's what the regular clock stop does. b. PCI-based wakes. In that case, the wake is not detected at the SoundWire level - the IP is power-gated -, but comes from the PCI subsystem based on a level detector and needs to be notified to the SoundWire master. That's what I did. Now you can claim that this case b) doesn't belong in the drivers/soundwire code, in which case my answer will be to move all the Intel controller code in sound/soc/sof/intel. I have no choice but to implement what's needed on the hardware I have. > > > Intel has a big controller with HDA, DSP and Soundwire clubbed together, > I dont think we should burden the susbstem due to hw design > >> also allow for better sysfs support without the reference count issues >> mentioned in the initial reviews. >> >> In this patch, we convert the existing code to use an explicit >> sdw_slave_type, then define new objects (sdw_master_device and >> sdw_master_driver). > > Thanks for sdw_master_device, that is required and fully agreed upon. > What is not agreed is the sdw_master_driver. We do not need that. I will respectfully ask that you have a conversation with Greg on this one. These patches were reviewed as 'sane', I am not going to go back on this without an agreement on directions. > > As we have discussed your proposal with Greg and aligned (quoting that > here) on following device model for Intel and ARM: > >> - For DT cases we will have: >> -> soundwire DT device (soundwire DT driver) >> -> soundwire master device >> -> soundwire slave device (slave drivers) >> - For Intel case, you would have: >> -> HDA PCI device (SOF driver + soundwire module) >> -> soundwire master device >> -> soundwire slave device (slave drivers) > > But you have gone ahead and kept the sdw_master_driver which does not fit > into rest of the world except Intel. I followed Greg's guidance. There was nothing in the thread with Greg that hinted as a required change. If you don't agree with Greg, please talk with him. > > I think I am okay with rest of proposal, except this one, so can you > remove this and we can make progress. This issue is lingering since Oct! Yes indeed, and we just circled once more. >> A parent (such as the Intel audio controller or its equivalent on >> Qualcomm devices) would use sdw_master_device_add() to create the >> device, passing a driver name as a parameter. The master device would >> be released when device_unregister() is invoked by the parent. > > We already have a DT driver for soundwire master! We dont need another > layer which does not add value! Talk with Greg please. > >> Note that since there is no standard for the Master host-facing >> interface, so the bus matching relies on a simple string matching (as >> previously done with platform devices). >> >> The 'Master Device' driver exposes callbacks for >> probe/startup/shutdown/remove/process_wake. The startup and process >> wake need to be called by the parent directly (using wrappers), while >> the probe/shutdown/remove are handled by the SoundWire bus core upon >> device creation and release. > > these are added to handle intel DSP and sequencing issue, rest of the > world does not have these issues and does not needs them! They are not required, don't use them if you don't need them? What's wrong with this approach? >> Additional callbacks will be added in the future for e.g. autonomous >> clock stop modes. > > Yes these would be required, these can be added in sdw_master_device > too, I dont see them requiring a dummy driver layer.. Again talk with Greg. >> @@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev) >> slave->probed = true; >> complete(&slave->probe_complete); >> >> - dev_dbg(dev, "probe complete\n"); >> - > > This does not seem to belong to this patch. > >> +struct device_type sdw_master_type = { >> + .name = "soundwire_master", >> + .release = sdw_master_device_release, >> +}; >> + >> +struct sdw_master_device >> +*sdw_master_device_add(const char *master_name, >> + struct device *parent, >> + struct fwnode_handle *fwnode, >> + int link_id, >> + void *pdata) >> +{ >> + struct sdw_master_device *md; >> + int ret; >> + >> + md = kzalloc(sizeof(*md), GFP_KERNEL); >> + if (!md) >> + return ERR_PTR(-ENOMEM); >> + >> + md->link_id = link_id; >> + md->pdata = pdata; >> + md->master_name = master_name; > > should we not allocate the memory here for master_name? > >> + >> + init_completion(&md->probe_complete); >> + >> + md->dev.parent = parent; >> + md->dev.fwnode = fwnode; >> + md->dev.bus = &sdw_bus_type; >> + md->dev.type = &sdw_master_type; >> + md->dev.dma_mask = md->dev.parent->dma_mask; >> + dev_set_name(&md->dev, "sdw-master-%d", md->link_id); > > why do we need master_name if we are setting this here? for the matching needed to find a driver. > >> + >> + ret = device_register(&md->dev); >> + if (ret) { >> + dev_err(parent, "Failed to add master: ret %d\n", ret); >> + /* >> + * On err, don't free but drop ref as this will be freed >> + * when release method is invoked. >> + */ >> + put_device(&md->dev); >> + return ERR_PTR(-ENOMEM); > > ENOMEM? no, this function returns a pointer. > >> +int sdw_master_device_startup(struct sdw_master_device *md) >> +{ >> + struct sdw_master_driver *mdrv; >> + struct device *dev; >> + int ret = 0; >> + >> + if (IS_ERR_OR_NULL(md)) >> + return -EINVAL; >> + >> + dev = &md->dev; >> + mdrv = drv_to_sdw_master_driver(dev->driver); >> + >> + if (mdrv && mdrv->startup) >> + ret = mdrv->startup(md); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(sdw_master_device_startup); > > who invokes this and when, can you add kernel-doc style documentation to > all APIs exported ok > >> +int sdw_master_device_process_wake_event(struct sdw_master_device *md) >> +{ >> + struct sdw_master_driver *mdrv; >> + struct device *dev; >> + int ret = 0; >> + >> + if (IS_ERR_OR_NULL(md)) >> + return -EINVAL; >> + >> + dev = &md->dev; >> + mdrv = drv_to_sdw_master_driver(dev->driver); >> + >> + if (mdrv && mdrv->process_wake_event) >> + ret = mdrv->process_wake_event(md); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event); > > Documentation required ok > >> +/** >> + * struct sdw_master_device - SoundWire 'Master Device' representation >> + * >> + * @dev: Linux device for this Master >> + * @master_name: Linux driver name >> + * @driver: Linux driver for this Master (set by SoundWire core during probe) >> + * @probe_complete: used by parent if synchronous probe behavior is needed >> + * @link_id: link index as defined by MIPI DisCo specification >> + * @pm_runtime_suspended: flag to restore pm_runtime state after system resume >> + * @pdata: private data typically provided with sdw_master_device_add() >> + */ >> + >> +struct sdw_master_device { >> + struct device dev; >> + const char *master_name; >> + struct sdw_master_driver *driver; >> + struct completion probe_complete; >> + int link_id; >> + bool pm_runtime_suspended; > > why not use runtime_pm apis like pm_runtime_suspended() that's exactly what we do, we store the value on pm_runtime_suspended() and use it on resume. > >> +/** >> + * sdw_master_device_add() - create a Linux Master Device representation. >> + * >> + * @master_name: Linux driver name >> + * @parent: the parent Linux device (e.g. a PCI device) >> + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent) >> + * @link_id: link index as defined by MIPI DisCo specification >> + * @pdata: private data (e.g. register base, offsets, platform quirks, etc). >> + */ >> +struct sdw_master_device >> +*sdw_master_device_add(const char *master_name, >> + struct device *parent, >> + struct fwnode_handle *fwnode, >> + int link_id, >> + void *pdata); >> + >> +/** >> + * sdw_master_device_startup() - startup hardware >> + * >> + * @md: Linux Soundwire master device > > Please add more useful comments like when this API would be invoked and > what shall be expected outcome Huh, that's pretty much self-explanatory? device_add() adds a device and if there's a driver for it the probe will be called. Nothing fancy or earth-shattering. > >> + */ >> +int sdw_master_device_startup(struct sdw_master_device *md); >> + >> +/** >> + * sdw_master_device_process_wake_event() - handle external wake >> + * event, e.g. handled at the PCI level >> + * >> + * @md: Linux Soundwire master device >> + */ >> +int sdw_master_device_process_wake_event(struct sdw_master_device *md); >> + > > If you look at existing headers the documentation is in C files for > APIs, so can you move them over. > > When adding stuff please look at the rest of the code as an example. isn't it more clear when the prototypes come with the documentation, rather than the kernel doc stuff be buried in pages of code?
On 03-03-20, 09:23, Pierre-Louis Bossart wrote: > On 3/2/20 11:41 PM, Vinod Koul wrote: > > On 27-02-20, 16:31, Pierre-Louis Bossart wrote: > > > In the existing SoundWire code, Master Devices are not explicitly > > > represented - only SoundWire Slave Devices are exposed (the use of > > > capital letters follows the SoundWire specification conventions). > > > > > > The SoundWire Master Device provides the clock, synchronization > > > information and command/control channels. When multiple links are > > > supported, a Controller may expose more than one Master Device; they > > > are typically embedded inside a larger audio cluster (be it in an > > > SOC/chipset or an external audio codec), and we need to describe it > > > using the Linux device and driver model. This will allow for > > > configuration functions to account for external dependencies such as > > > power rails, clock sources or wake-up mechanisms. This transition will > > > > I dont not see that as a soundwire issue. The external dependencies > > should be handled as any device would do in Linux kernel with subsystem > > specific things for soundwire mechanisms like wake-up > > There is nothing in the SoundWire specification that tells how a > controller/master should interface with the rest of the SoC. > > There are two ways of handling wake-ups > a. 'normal' wakes handled internally by the master - that's what the regular > clock stop does. > b. PCI-based wakes. In that case, the wake is not detected at the SoundWire > level - the IP is power-gated -, but comes from the PCI subsystem based on a > level detector and needs to be notified to the SoundWire master. That's what > I did. > > Now you can claim that this case b) doesn't belong in the drivers/soundwire > code, in which case my answer will be to move all the Intel controller code > in sound/soc/sof/intel. I have no choice but to implement what's needed on > the hardware I have. Did you read the full para I replied? Let me quote that again.. > > I dont not see that as a soundwire issue. The external dependencies > > should be handled as any device would do in Linux kernel So things which have external dependencies is first part like you said clock, etc.. They are generic ones and should be handled with typical kernel mechanisms available.. > > with subsystem > > specific things for soundwire mechanisms like wake-up And you didn't even read this where i have said subsystem specific things for soundwire mechanisms like **wake-up** So that means I am okay with have wakeup mechanisms for sdw within subsystem. And you want to move code, feel free to do that with my NAK! Please read emails completely before screaming away! Not a great way to start the conversation > > Intel has a big controller with HDA, DSP and Soundwire clubbed together, > > I dont think we should burden the susbstem due to hw design > > > > > also allow for better sysfs support without the reference count issues > > > mentioned in the initial reviews. > > > > > > In this patch, we convert the existing code to use an explicit > > > sdw_slave_type, then define new objects (sdw_master_device and > > > sdw_master_driver). > > > > Thanks for sdw_master_device, that is required and fully agreed upon. > > What is not agreed is the sdw_master_driver. We do not need that. > > I will respectfully ask that you have a conversation with Greg on this one. > These patches were reviewed as 'sane', I am not going to go back on this > without an agreement on directions. > > > > > As we have discussed your proposal with Greg and aligned (quoting that > > here) on following device model for Intel and ARM: > > > > > - For DT cases we will have: > > > -> soundwire DT device (soundwire DT driver) > > > -> soundwire master device > > > -> soundwire slave device (slave drivers) > > > - For Intel case, you would have: > > > -> HDA PCI device (SOF driver + soundwire module) > > > -> soundwire master device > > > -> soundwire slave device (slave drivers) > > > > But you have gone ahead and kept the sdw_master_driver which does not fit > > into rest of the world except Intel. > > I followed Greg's guidance. There was nothing in the thread with Greg that > hinted as a required change. > If you don't agree with Greg, please talk with him. Were the above lines agreed or not? Do you see driver for master devices or not? Greg was okay with as well as these patches but I am not okay with the driver part for master, so I would like to see that removed. Different reviewers can have different reasons.. I have given bunch of reasons here, BUT I have not seen a single technical reason why this cannot be done. > > I think I am okay with rest of proposal, except this one, so can you > > remove this and we can make progress. This issue is lingering since Oct! > > Yes indeed, and we just circled once more. > > > > A parent (such as the Intel audio controller or its equivalent on > > > Qualcomm devices) would use sdw_master_device_add() to create the > > > device, passing a driver name as a parameter. The master device would > > > be released when device_unregister() is invoked by the parent. > > > > We already have a DT driver for soundwire master! We dont need another > > layer which does not add value! > > Talk with Greg please. > > > > > > Note that since there is no standard for the Master host-facing > > > interface, so the bus matching relies on a simple string matching (as > > > previously done with platform devices). > > > > > > The 'Master Device' driver exposes callbacks for > > > probe/startup/shutdown/remove/process_wake. The startup and process > > > wake need to be called by the parent directly (using wrappers), while > > > the probe/shutdown/remove are handled by the SoundWire bus core upon > > > device creation and release. > > > > these are added to handle intel DSP and sequencing issue, rest of the > > world does not have these issues and does not needs them! > > They are not required, don't use them if you don't need them? > What's wrong with this approach? I would like to see everyone use similar mechanism, Can you give me reasons why you absolutely need master_driver? I don't want to see vendor specific mechanisms in subsystem unless it is an absolute must have. This case doesn't fall into that category. As I have told you couple of times earlier, you can remove the sdw_master_driver and call the functions directly, that solves your problem but somehow i see reluctance on that. Can you tell me technical reason for not doing that > > > Additional callbacks will be added in the future for e.g. autonomous > > > clock stop modes. > > > > Yes these would be required, these can be added in sdw_master_device > > too, I dont see them requiring a dummy driver layer.. > > Again talk with Greg. > > > > @@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev) > > > slave->probed = true; > > > complete(&slave->probe_complete); > > > - dev_dbg(dev, "probe complete\n"); > > > - > > > > This does not seem to belong to this patch. This was not answered > > > > > +struct device_type sdw_master_type = { > > > + .name = "soundwire_master", > > > + .release = sdw_master_device_release, > > > +}; > > > + > > > +struct sdw_master_device > > > +*sdw_master_device_add(const char *master_name, > > > + struct device *parent, > > > + struct fwnode_handle *fwnode, > > > + int link_id, > > > + void *pdata) > > > +{ > > > + struct sdw_master_device *md; > > > + int ret; > > > + > > > + md = kzalloc(sizeof(*md), GFP_KERNEL); > > > + if (!md) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + md->link_id = link_id; > > > + md->pdata = pdata; > > > + md->master_name = master_name; > > > > should we not allocate the memory here for master_name? And this wasn't either! > > > > > + > > > + init_completion(&md->probe_complete); > > > + > > > + md->dev.parent = parent; > > > + md->dev.fwnode = fwnode; > > > + md->dev.bus = &sdw_bus_type; > > > + md->dev.type = &sdw_master_type; > > > + md->dev.dma_mask = md->dev.parent->dma_mask; > > > + dev_set_name(&md->dev, "sdw-master-%d", md->link_id); > > > > why do we need master_name if we are setting this here? > > for the matching needed to find a driver. > > > > > > + > > > + ret = device_register(&md->dev); > > > + if (ret) { > > > + dev_err(parent, "Failed to add master: ret %d\n", ret); > > > + /* > > > + * On err, don't free but drop ref as this will be freed > > > + * when release method is invoked. > > > + */ > > > + put_device(&md->dev); > > > + return ERR_PTR(-ENOMEM); > > > > ENOMEM? > > no, this function returns a pointer. Yes but why should it be ENOMEM? That is incorrect and you are hiding the actual reason.. It should be: return ERR_PTR(ret) > > > +int sdw_master_device_startup(struct sdw_master_device *md) > > > +{ > > > + struct sdw_master_driver *mdrv; > > > + struct device *dev; > > > + int ret = 0; > > > + > > > + if (IS_ERR_OR_NULL(md)) > > > + return -EINVAL; > > > + > > > + dev = &md->dev; > > > + mdrv = drv_to_sdw_master_driver(dev->driver); > > > + > > > + if (mdrv && mdrv->startup) > > > + ret = mdrv->startup(md); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(sdw_master_device_startup); > > > > who invokes this and when, can you add kernel-doc style documentation to > > all APIs exported > > ok > > > > > > +int sdw_master_device_process_wake_event(struct sdw_master_device *md) > > > +{ > > > + struct sdw_master_driver *mdrv; > > > + struct device *dev; > > > + int ret = 0; > > > + > > > + if (IS_ERR_OR_NULL(md)) > > > + return -EINVAL; > > > + > > > + dev = &md->dev; > > > + mdrv = drv_to_sdw_master_driver(dev->driver); > > > + > > > + if (mdrv && mdrv->process_wake_event) > > > + ret = mdrv->process_wake_event(md); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event); > > > > Documentation required > > ok > > > > > > +/** > > > + * struct sdw_master_device - SoundWire 'Master Device' representation > > > + * > > > + * @dev: Linux device for this Master > > > + * @master_name: Linux driver name > > > + * @driver: Linux driver for this Master (set by SoundWire core during probe) > > > + * @probe_complete: used by parent if synchronous probe behavior is needed > > > + * @link_id: link index as defined by MIPI DisCo specification > > > + * @pm_runtime_suspended: flag to restore pm_runtime state after system resume > > > + * @pdata: private data typically provided with sdw_master_device_add() > > > + */ > > > + > > > +struct sdw_master_device { > > > + struct device dev; > > > + const char *master_name; > > > + struct sdw_master_driver *driver; > > > + struct completion probe_complete; > > > + int link_id; > > > + bool pm_runtime_suspended; > > > > why not use runtime_pm apis like pm_runtime_suspended() > > that's exactly what we do, we store the value on pm_runtime_suspended() and > use it on resume. Why store when you can query? > > > +/** > > > + * sdw_master_device_add() - create a Linux Master Device representation. > > > + * > > > + * @master_name: Linux driver name > > > + * @parent: the parent Linux device (e.g. a PCI device) > > > + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent) > > > + * @link_id: link index as defined by MIPI DisCo specification > > > + * @pdata: private data (e.g. register base, offsets, platform quirks, etc). > > > + */ > > > +struct sdw_master_device > > > +*sdw_master_device_add(const char *master_name, > > > + struct device *parent, > > > + struct fwnode_handle *fwnode, > > > + int link_id, > > > + void *pdata); > > > + > > > +/** > > > + * sdw_master_device_startup() - startup hardware > > > + * > > > + * @md: Linux Soundwire master device > > > > Please add more useful comments like when this API would be invoked and > > what shall be expected outcome > > Huh, that's pretty much self-explanatory? device_add() adds a device and if > there's a driver for it the probe will be called. Nothing fancy or > earth-shattering. As I have asked before, pls document when this API would be invoked, at the time hw is ready or before.., before providing clock/power.., before/after bus init..? This cannot be called anyplace and needs to be called before/after certain steps and that needs to be documented! > > > + */ > > > +int sdw_master_device_startup(struct sdw_master_device *md); > > > + > > > +/** > > > + * sdw_master_device_process_wake_event() - handle external wake > > > + * event, e.g. handled at the PCI level > > > + * > > > + * @md: Linux Soundwire master device > > > + */ > > > +int sdw_master_device_process_wake_event(struct sdw_master_device *md); > > > + > > > > If you look at existing headers the documentation is in C files for > > APIs, so can you move them over. > > > > When adding stuff please look at the rest of the code as an example. > > isn't it more clear when the prototypes come with the documentation, rather > than the kernel doc stuff be buried in pages of code? Not to me, I want to see these in the code and compare! As I have said when adding stuff please look at the rest of the code as an example, everywhere else in the subsystem.
> Were the above lines agreed or not? Do you see driver for master devices > or not? Greg was okay with as well as these patches but I am not okay > with the driver part for master, so I would like to see that removed. > > Different reviewers can have different reasons.. I have given bunch of > reasons here, BUT I have not seen a single technical reason why this > cannot be done. With all due respect, I consider Greg as THE reviewer for device/driver questions. Your earlier proposal to use platform devices was rejected by Greg, and we've lost an entire month in the process, so I am somewhat dubious on your proposal not to use a driver. If you want a technical objection, let me restate what I already mentioned: If you look at the hierarchy, we have PCI device -> PCI driver soundwire_master_device0 soundwire_slave(s) -> codec driver ... soundwire_master_deviceN soundwire_slave(s) -> codec driver You have not explained how I could possibly deal with power management without having a driver for the master_device(s). The pm_ops need to be inserted in a driver structure, which means we need a driver. And if we need a driver, then we might as well have a real driver with .probe .remove support, driver_register(), etc. I really don't see what's broken or unnecessary with these patches. I would also kindly ask that you stop using exclamation marks and what I consider as hostile language. I've asked you multiple times, it's not professional, sorry. Regards -Pierre
On Wed, Mar 04, 2020 at 09:17:07AM -0600, Pierre-Louis Bossart wrote: > > > > Were the above lines agreed or not? Do you see driver for master devices > > or not? Greg was okay with as well as these patches but I am not okay > > with the driver part for master, so I would like to see that removed. > > > > Different reviewers can have different reasons.. I have given bunch of > > reasons here, BUT I have not seen a single technical reason why this > > cannot be done. > > With all due respect, I consider Greg as THE reviewer for device/driver > questions. Your earlier proposal to use platform devices was rejected by > Greg, and we've lost an entire month in the process, so I am somewhat > dubious on your proposal not to use a driver. > > If you want a technical objection, let me restate what I already mentioned: > > If you look at the hierarchy, we have > > PCI device -> PCI driver > soundwire_master_device0 > soundwire_slave(s) -> codec driver > ... > soundwire_master_deviceN > soundwire_slave(s) -> codec driver > > You have not explained how I could possibly deal with power management > without having a driver for the master_device(s). The pm_ops need to be > inserted in a driver structure, which means we need a driver. And if we need > a driver, then we might as well have a real driver with .probe .remove > support, driver_register(), etc. To weigh in here, yes, you need such a "device" here as it isn't the PCI device that you can use, you need your own. Just like most other busses have this (USB has host controller drivers as one example, that create the "root bus" device that all other USB devices hang off of.) This "controller device" should hang off of the hardware device be it a platform/PCI/i2c/spi/serial/whatever type of controller. That's why it is needed. > I really don't see what's broken or unnecessary with these patches. The "wait until something else happens" does seem a bit hacky, odds are that's not really needed if you are using the driver model correctly, but soundwire is "odd" in places so maybe that is necessary, I'll defer to you two on that mess :) thanks, greg k-h
On 04-03-20, 09:17, Pierre-Louis Bossart wrote: > > > > Were the above lines agreed or not? Do you see driver for master devices > > or not? Greg was okay with as well as these patches but I am not okay > > with the driver part for master, so I would like to see that removed. > > > > Different reviewers can have different reasons.. I have given bunch of > > reasons here, BUT I have not seen a single technical reason why this > > cannot be done. > > With all due respect, I consider Greg as THE reviewer for device/driver > questions. Your earlier proposal to use platform devices was rejected by > Greg, and we've lost an entire month in the process, so I am somewhat > dubious on your proposal not to use a driver. > > If you want a technical objection, let me restate what I already mentioned: > > If you look at the hierarchy, we have > > PCI device -> PCI driver > soundwire_master_device0 > soundwire_slave(s) -> codec driver > ... > soundwire_master_deviceN > soundwire_slave(s) -> codec driver > > You have not explained how I could possibly deal with power management > without having a driver for the master_device(s). The pm_ops need to be > inserted in a driver structure, which means we need a driver. And if we need > a driver, then we might as well have a real driver with .probe .remove > support, driver_register(), etc. Please read the emails sent to you completely, including the reply on 2nd patch of this series. I think i am repeating this 3rd or 4th time now. Am going to repeat this info here to help move things. 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. Second issue of PM: You do manage the DSP PM right? Similar way. So here I would expect you to add functions/callbacks from SOF driver to this module and call PM routines from SOF PM routines allowing you to suspend/resume. Similar way DSP used to be managed. Something like: .sdw_suspend .sdw_resume functions/callbacks which will do sdw specific pm configurations. You do not need module specific pm_ops, you can do the required steps in callbacks from SOF driver Bonus, this can be tuned and called at the specific places in DSP suspend/resume flow, which is something I suspect you would want. For places which need dev/driver objects like sdw dai's please pass the SOF PCI dev object. Is there any other technical reason left unexplored/unexplained? > I really don't see what's broken or unnecessary with these patches. Adding a layer for Intel in common code is unnecessary IMO. As demonstrated above you can use the intel specific callback to do the same task in intel specific way. I would very much prefer that approach to solve this We definitely need a sdw_master_device for everyone, but I don't believe we need a sdw_master_device for Intel or anyone else. > I would also kindly ask that you stop using exclamation marks and what I > consider as hostile language. I've asked you multiple times, it's not > professional, sorry. oops, i would apologise for that. I seem to have a habit of using that which indicates my surprise and not hostile language, maybe it is cultural thing, but I would try to refrain. thanks for letting me know.
On 04-03-20, 17:28, Greg KH wrote: > On Wed, Mar 04, 2020 at 09:17:07AM -0600, Pierre-Louis Bossart wrote: > > > > > > > Were the above lines agreed or not? Do you see driver for master devices > > > or not? Greg was okay with as well as these patches but I am not okay > > > with the driver part for master, so I would like to see that removed. > > > > > > Different reviewers can have different reasons.. I have given bunch of > > > reasons here, BUT I have not seen a single technical reason why this > > > cannot be done. > > > > With all due respect, I consider Greg as THE reviewer for device/driver > > questions. Your earlier proposal to use platform devices was rejected by > > Greg, and we've lost an entire month in the process, so I am somewhat > > dubious on your proposal not to use a driver. > > > > If you want a technical objection, let me restate what I already mentioned: > > > > If you look at the hierarchy, we have > > > > PCI device -> PCI driver > > soundwire_master_device0 > > soundwire_slave(s) -> codec driver > > ... > > soundwire_master_deviceN > > soundwire_slave(s) -> codec driver > > > > You have not explained how I could possibly deal with power management > > without having a driver for the master_device(s). The pm_ops need to be > > inserted in a driver structure, which means we need a driver. And if we need > > a driver, then we might as well have a real driver with .probe .remove > > support, driver_register(), etc. > > To weigh in here, yes, you need such a "device" here as it isn't the PCI > device that you can use, you need your own. Just like most other busses > have this (USB has host controller drivers as one example, that create > the "root bus" device that all other USB devices hang off of.) This > "controller device" should hang off of the hardware device be it a > platform/PCI/i2c/spi/serial/whatever type of controller. That's why it > is needed. > > > I really don't see what's broken or unnecessary with these patches. > > The "wait until something else happens" does seem a bit hacky, odds are > that's not really needed if you are using the driver model correctly, > but soundwire is "odd" in places so maybe that is necessary, I'll defer > to you two on that mess :) I have no issues with adding the root bus device, so we really need the sdw_master_device. That really was a miss from me. If Pierre remove the driver parts from this set, I would merge the rest in a heartbeat. But in the mix we dont want to add a new driver which is dummy. The PCI/DT device will have a driver which is something to be used here. For Qualcomm controller, we get a DT device and driver and that does the job, it doesn't need one more driver and probe routine. If Pierre had a PCI device for SDW controller, he would not be asking for this, since Intel has a complex device, and he would like to split the functions, the need of a new driver arises. But the whole subsystem should not be burdened for that. It can be managed without that and is really an Intel issue not a subsystem one.
>> If you want a technical objection, let me restate what I already mentioned: >> >> If you look at the hierarchy, we have >> >> PCI device -> PCI driver >> soundwire_master_device0 >> soundwire_slave(s) -> codec driver >> ... >> soundwire_master_deviceN >> soundwire_slave(s) -> codec driver >> >> You have not explained how I could possibly deal with power management >> without having a driver for the master_device(s). The pm_ops need to be >> inserted in a driver structure, which means we need a driver. And if we need >> a driver, then we might as well have a real driver with .probe .remove >> support, driver_register(), etc. > > Please read the emails sent to you completely, including the reply on > 2nd patch of this series. I think i am repeating this 3rd or 4th time > now. Am going to repeat this info here to help move things. > > 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. I might add that I don't see the logic in having pm support at the Slave device level, but not at the master, and again at the PCI level. It's just simpler to handle pm at each level rather that fudge layers. I would also remind you that the solution you developed while at Intel did in fact use the parent-child relationship for power management, and I remember very clearly having discussions with you on this. I don't see why replacing platform devices by master devices should change this design choice. > Second issue of PM: > You do manage the DSP PM right? Similar way. > So here I would expect you to add functions/callbacks from SOF driver to > this module and call PM routines from SOF PM routines allowing you to > suspend/resume. Similar way DSP used to be managed. Something like: > .sdw_suspend .sdw_resume functions/callbacks which will do sdw specific > pm configurations. You do not need module specific pm_ops, you can do > the required steps in callbacks from SOF driver > > Bonus, this can be tuned and called at the specific places in DSP > suspend/resume flow, which is something I suspect you would want. No. The links can only be resumed when the DSP is fully powered. We've tried all sorts of optimizations already and worked with the hardware team on this. > > For places which need dev/driver objects like sdw dai's please pass the > SOF PCI dev object. > > Is there any other technical reason left unexplored/unexplained? > >> I really don't see what's broken or unnecessary with these patches. > > Adding a layer for Intel in common code is unnecessary IMO. As > demonstrated above you can use the intel specific callback to do the > same task in intel specific way. I would very much prefer that approach > to solve this > > We definitely need a sdw_master_device for everyone, but I don't believe > we need a sdw_master_device for Intel or anyone else. I will flip the argument: you can implement a lightweight master driver in no time. All you need is to move the code you currently have in the platform device .probe() to the master_device .probe(). Big deal, the overhead is negligible - and you don't need to add pm_ops if you don't need to. I would add that you cannot possibly compare the two implementations. Qualcomm has an extremely simple SoundWire link optimized for 2 PDM amplifiers connected to a SLIMbus codec, with a fixed bit allocation. There is currently no power management for this link. Intel has 4 links running in parallel and synchronized in hardware, with complicated power management, different flavors of clock-stop - some not controlled by the driver but by DSP firmware - , 5 hardware configurations (more coming) and 6 third-party devices (more coming). You've got to give us some slack here, and leave us handle power management in the simplest possible way.
On 05-03-20, 06:46, Pierre-Louis Bossart wrote: > > > > If you want a technical objection, let me restate what I already mentioned: > > > > > > If you look at the hierarchy, we have > > > > > > PCI device -> PCI driver > > > soundwire_master_device0 > > > soundwire_slave(s) -> codec driver > > > ... > > > soundwire_master_deviceN > > > soundwire_slave(s) -> codec driver > > > > > > You have not explained how I could possibly deal with power management > > > without having a driver for the master_device(s). The pm_ops need to be > > > inserted in a driver structure, which means we need a driver. And if we need > > > a driver, then we might as well have a real driver with .probe .remove > > > support, driver_register(), etc. > > > > Please read the emails sent to you completely, including the reply on > > 2nd patch of this series. I think i am repeating this 3rd or 4th time > > now. Am going to repeat this info here to help move things. > > > > 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. > I might add that I don't see the logic in having pm support at the Slave > device level, but not at the master, and again at the PCI level. It's just > simpler to handle pm at each level rather that fudge layers. > > I would also remind you that the solution you developed while at Intel did > in fact use the parent-child relationship for power management, and I > remember very clearly having discussions with you on this. I don't see why > replacing platform devices by master devices should change this design > choice. That was with the premise of a platform device, since that is no longer the case, we have to adapt. But you still have PCI->master dev->slave relation and actually not much changes wrt that. You still need to enable pm for master device. Only change in that master_dev will not have pm_ops. Again, it is same for i2c/spi where we have pci|of dev -> adapter dev -> i2c dev. > > Second issue of PM: > > You do manage the DSP PM right? Similar way. > > So here I would expect you to add functions/callbacks from SOF driver to > > this module and call PM routines from SOF PM routines allowing you to > > suspend/resume. Similar way DSP used to be managed. Something like: > > .sdw_suspend .sdw_resume functions/callbacks which will do sdw specific > > pm configurations. You do not need module specific pm_ops, you can do > > the required steps in callbacks from SOF driver > > > > Bonus, this can be tuned and called at the specific places in DSP > > suspend/resume flow, which is something I suspect you would want. > > No. The links can only be resumed when the DSP is fully powered. We've tried > all sorts of optimizations already and worked with the hardware team on > this. And you can call links _exactly_ when DSP is up and additional optimizations applied. You are not reliant on core sequencing. > > For places which need dev/driver objects like sdw dai's please pass the > > SOF PCI dev object. > > > > Is there any other technical reason left unexplored/unexplained? > > > > > I really don't see what's broken or unnecessary with these patches. > > > > Adding a layer for Intel in common code is unnecessary IMO. As > > demonstrated above you can use the intel specific callback to do the > > same task in intel specific way. I would very much prefer that approach > > to solve this > > > > We definitely need a sdw_master_device for everyone, but I don't believe > > we need a sdw_master_device for Intel or anyone else. > > I will flip the argument: you can implement a lightweight master driver in > no time. All you need is to move the code you currently have in the platform > device .probe() to the master_device .probe(). Big deal, the overhead is > negligible - and you don't need to add pm_ops if you don't need to. And in that case will you have use for sdw_master_driver? > I would add that you cannot possibly compare the two implementations. > > Qualcomm has an extremely simple SoundWire link optimized for 2 PDM > amplifiers connected to a SLIMbus codec, with a fixed bit allocation. There > is currently no power management for this link. Not upstream but planned to be implemented. And assumption above may not be true always esp future chipsets. More support will come eventually but none of that warrants the need of a sdw_master_driver. > Intel has 4 links running in parallel and synchronized in hardware, with > complicated power management, different flavors of clock-stop - some not > controlled by the driver but by DSP firmware - , 5 hardware configurations > (more coming) and 6 third-party devices (more coming). Number of links are inconsequential to soundwire. For us it is just an instance. > You've got to give us some slack here, and leave us handle power management > in the simplest possible way. I still do not agree that it will cause additional complexities for you. Thanks
>>> 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. Given these limitations, I am not willing to change directions on power management. We have a tried-and-tested solution, backed by months of validation, and you are sending down an unproven path with your suggestion. So what are the options? a) stay with the current approach and platform devices. Greg's vetoed this so we can move to the next one. b) use a solution similar to what we suggested back in October, and very similar to the GreyBus host device, which creates a master device but did not require a full-blown master_driver, it only uses the name and pm_ops fields of the raw driver structure, which is all we really need. the basic usage from the PCI layer was struct driver { .name = "my-driver", .pm_ops = &my_ops, } my_driver; md = sdw_master_device_add(&my_driver, parent, fw_node, link_id) and all the rest is platform-specific/optional. sdw_intel_master_device_init(md); allocations and call to sdw_bus_master_add() sdw_intel_master_device_startup(md); hardware enablement sdw_intel_master_device_wake_process(md). deal with wake information coming from PCI layer. We liked this solution since it was as simple as can be, but you rejected it on the grounds that the probe/init should be handled "by the core" to quote your own words, but looking back it may be the best solution for all. There is no additional overhead, and it deals with both the ALSA name requirement and lets us us power management. If you don't have power management at the link level you don't have to use the pm_ops. c) use the proposal in this patch with a more elaborate driver handling. Yes it requires a full-blown driver with callbacks but it addresses your prior feedback that the core handles the probe/remove operations. All these solutions are proven to work. Pick one. If you want to suggest another, then please provide a pseudo API and address the non-negotiable requirement of independent link-level power management. Thanks -Pierre
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.
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.
On 11-03-20, 09:44, Pierre-Louis Bossart wrote: > 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. Yes sure, that seems to be the cost unfortunately. While it might feel I am blocking but the real block here is the hw design which gives you a monolith whereas it should have been different devices. If you have a 'device' for sdw or a standalone controller we would not be debating this.. > Linux is all about frameworks. For power management, we shall use the power > management framework, not reinvent it. This reminds me, please talk to Mika and Rafael, they had similar problems with lpss etc and IIRC they were working on splices to solve this.. Its been some time (few years now) so maybe they have a solution.. Thanks
>>>> 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. > > Yes sure, that seems to be the cost unfortunately. While it might feel I > am blocking but the real block here is the hw design which gives you a > monolith whereas it should have been different devices. If you have a > 'device' for sdw or a standalone controller we would not be debating > this.. The hardware is what it is. The ACPI spec is what it is. I am just pragmatic and making platforms work with that's available *today*, and I don't have time or interest in revisiting what might have been. >> Linux is all about frameworks. For power management, we shall use the power >> management framework, not reinvent it. > > This reminds me, please talk to Mika and Rafael, they had similar > problems with lpss etc and IIRC they were working on splices to solve > this.. Its been some time (few years now) so maybe they have a > solution.. We've been discussing this since October, I don't really have any appetite for looking into new concepts when the existing framework just does what we need. It's really down to your objection to the use of 'struct driver'... For ASoC support we only need the .name and .pm_ops, so there's really no possible path forward otherwise. Like I said, we have 3 options a) stay with platform devices for now. You will need to have a conversation with Greg on this. b) use a minimal sdw_master_device with a minimal 'struct driver' use. c) use a more elaborate solution suggested in this patchset and yes that means the Qualcomm driver would need to change a bit. Pick one or suggest something that is implementable. The first version of the patches was provided in October, the last RFC was provided on January 31, time's up. At the moment you are preventing ASoC integration from moving forward. Thanks -Pierre
On 13-03-20, 11:54, Pierre-Louis Bossart wrote: > > > > > > 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. > > > > Yes sure, that seems to be the cost unfortunately. While it might feel I > > am blocking but the real block here is the hw design which gives you a > > monolith whereas it should have been different devices. If you have a > > 'device' for sdw or a standalone controller we would not be debating > > this.. > > The hardware is what it is. The ACPI spec is what it is. > > I am just pragmatic and making platforms work with that's available *today*, > and I don't have time or interest in revisiting what might have been. > > > > Linux is all about frameworks. For power management, we shall use the power > > > management framework, not reinvent it. > > > > This reminds me, please talk to Mika and Rafael, they had similar > > problems with lpss etc and IIRC they were working on splices to solve > > this.. Its been some time (few years now) so maybe they have a > > solution.. > > We've been discussing this since October, I don't really have any appetite > for looking into new concepts when the existing framework just does what we > need. yes they do but add an intrusive platform specific change into soundwire core, something I would not like to add. You should really be willing to talk to your colleagues to see if there is something you can reuse. > It's really down to your objection to the use of 'struct driver'... For ASoC > support we only need the .name and .pm_ops, so there's really no possible > path forward otherwise. It means that we cannot have a solution which is Intel specific into core. If you has a standalone controller you do not need this. > Like I said, we have 3 options Repeating the already discussed doesn't help. I have already told you the constraint to work is not to add Intel specific change into core. I have already said that expect the driver part I dont have objections to rest of this series and am ready to merge > a) stay with platform devices for now. You will need to have a conversation > with Greg on this. > > b) use a minimal sdw_master_device with a minimal 'struct driver' use. > > c) use a more elaborate solution suggested in this patchset and yes that > means the Qualcomm driver would need to change a bit. > > Pick one or suggest something that is implementable. The first version of > the patches was provided in October, the last RFC was provided on January > 31, time's up. At the moment you are preventing ASoC integration from moving > forward. In opensource review we go back and forth and we debate and come to a common conclusion. Choosing a specific set of solutions and constraining yourself to pick one does not help. I have only _one_ constraint no platform specific change in core. If that is satisfied I will go with it. Sorry but this is non-negotiable for me. Ask yourself, do you need this intrusive core change if you had this exact same controller(s) but only as standalone one...
>> It's really down to your objection to the use of 'struct driver'... For ASoC >> support we only need the .name and .pm_ops, so there's really no possible >> path forward otherwise. > > It means that we cannot have a solution which is Intel specific into > core. If you has a standalone controller you do not need this. A 'struct driver' is not Intel-specific, sorry. >> Like I said, we have 3 options > > Repeating the already discussed doesn't help. I have already told you the > constraint to work is not to add Intel specific change into core. > > I have already said that expect the driver part I dont have objections > to rest of this series and am ready to merge > >> a) stay with platform devices for now. You will need to have a conversation >> with Greg on this. >> >> b) use a minimal sdw_master_device with a minimal 'struct driver' use. >> >> c) use a more elaborate solution suggested in this patchset and yes that >> means the Qualcomm driver would need to change a bit. >> >> Pick one or suggest something that is implementable. The first version of >> the patches was provided in October, the last RFC was provided on January >> 31, time's up. At the moment you are preventing ASoC integration from moving >> forward. > > In opensource review we go back and forth and we debate and come to a > common conclusion. Choosing a specific set of solutions and constraining > yourself to pick one does not help. First off, the ask to move away from platform devices came from Greg. Not me. All I did here was suggest solutions, one reviewed by Greg as 'sane' and 'nice work'. Greg essentially wrote the book on devices/drivers so his review means I am not completely senile just yet. You pushed back with two proposals that don't account for power management and the driver name required for ASoC. That was on top on another suggestion to use platform devices that was shot down by Greg himself with language I can't quote here. Please re-read my words: my ask was "Pick one or suggest something that is implementable." You don't pick one and don't suggest anything implementable either, so there's really not much I can do, can I? I don't have a solution without a 'struct driver', and you don't want it. The only short-term path forward I see is to ask Greg to agree to keep the platform devices for now. > I have only _one_ constraint no platform specific change in core. If that > is satisfied I will go with it. Sorry but this is non-negotiable for me. How is a 'struct driver' platform specific? > Ask yourself, do you need this intrusive core change if you had this > exact same controller(s) but only as standalone one... That would really not change anything. There would be some sort of ID (PCI or something else) for the controller and multiple masters below. The ACPI/DisCo spec does not account for masters so they would have to be created by hand. Again how is a 'struct driver' an 'intrusive change'?
On 16-03-20, 14:15, Pierre-Louis Bossart wrote: > > > > > It's really down to your objection to the use of 'struct driver'... For ASoC > > > support we only need the .name and .pm_ops, so there's really no possible > > > path forward otherwise. > > > > It means that we cannot have a solution which is Intel specific into > > core. If you has a standalone controller you do not need this. > > A 'struct driver' is not Intel-specific, sorry. We are discussing 'struct sdw_master_driver'. Please be very specific in you replies and do not use incorrect terminology which confuses people. Sorry a 'struct sdw_master_driver' IMHO is. As I have said it is not needed if you have standalone controller even in Intel case, and rest of the world. > > > Like I said, we have 3 options > > > > Repeating the already discussed doesn't help. I have already told you the > > constraint to work is not to add Intel specific change into core. > > > > I have already said that expect the driver part I dont have objections > > to rest of this series and am ready to merge > > > > > a) stay with platform devices for now. You will need to have a conversation > > > with Greg on this. > > > > > > b) use a minimal sdw_master_device with a minimal 'struct driver' use. > > > > > > c) use a more elaborate solution suggested in this patchset and yes that > > > means the Qualcomm driver would need to change a bit. > > > > > > Pick one or suggest something that is implementable. The first version of > > > the patches was provided in October, the last RFC was provided on January > > > 31, time's up. At the moment you are preventing ASoC integration from moving > > > forward. > > > > In opensource review we go back and forth and we debate and come to a > > common conclusion. Choosing a specific set of solutions and constraining > > yourself to pick one does not help. > > First off, the ask to move away from platform devices came from Greg. Not > me. All I did here was suggest solutions, one reviewed by Greg as 'sane' and > 'nice work'. Greg essentially wrote the book on devices/drivers so his > review means I am not completely senile just yet. > > You pushed back with two proposals that don't account for power management > and the driver name required for ASoC. That was on top on another suggestion > to use platform devices that was shot down by Greg himself with language I > can't quote here. > > Please re-read my words: my ask was "Pick one or suggest something that is > implementable." IMO the path I suggested is implementable.. > > You don't pick one and don't suggest anything implementable either, so > there's really not much I can do, can I? I don't have a solution without a > 'struct driver', and you don't want it. > > The only short-term path forward I see is to ask Greg to agree to keep the > platform devices for now. And I guess you didn't talk to your Intel colleagues... Please talk to them on how they did it. > > > I have only _one_ constraint no platform specific change in core. If that > > is satisfied I will go with it. Sorry but this is non-negotiable for me. > > How is a 'struct driver' platform specific? > > > Ask yourself, do you need this intrusive core change if you had this > > exact same controller(s) but only as standalone one... > > That would really not change anything. There would be some sort of ID (PCI > or something else) for the controller and multiple masters below. The If it is single master? If it is multiple master with different ACPI ID for each master? Would you still need 'struct sdw_master_driver'? > ACPI/DisCo spec does not account for masters so they would have to be > created by hand. > > Again how is a 'struct driver' an 'intrusive change'? Again do not confuse by using incorrect terminology. Again 'struct sdw_master_driver' for a specific Intel hw configuration is.
On 3/20/20 10:33 AM, Vinod Koul wrote: > On 16-03-20, 14:15, Pierre-Louis Bossart wrote: >> >> >>>> It's really down to your objection to the use of 'struct driver'... For ASoC >>>> support we only need the .name and .pm_ops, so there's really no possible >>>> path forward otherwise. >>> >>> It means that we cannot have a solution which is Intel specific into >>> core. If you has a standalone controller you do not need this. >> >> A 'struct driver' is not Intel-specific, sorry. > > We are discussing 'struct sdw_master_driver'. Please be very specific in > you replies and do not use incorrect terminology which confuses people. > > Sorry a 'struct sdw_master_driver' IMHO is. As I have said it is not > needed if you have standalone controller even in Intel case, and rest of > the world. You're splitting hair without providing a solution. Please see the series [PATCH 0/5] soundwire: add sdw_master_device support on Qualcomm platforms This solution was tested on Qualcomm platforms, that doesn't require this sdw_master_driver to be used, so your objections are now invalid.
On 20-03-20, 11:36, Pierre-Louis Bossart wrote: > > > On 3/20/20 10:33 AM, Vinod Koul wrote: > > On 16-03-20, 14:15, Pierre-Louis Bossart wrote: > > > > > > > > > > > It's really down to your objection to the use of 'struct driver'... For ASoC > > > > > support we only need the .name and .pm_ops, so there's really no possible > > > > > path forward otherwise. > > > > > > > > It means that we cannot have a solution which is Intel specific into > > > > core. If you has a standalone controller you do not need this. > > > > > > A 'struct driver' is not Intel-specific, sorry. > > > > We are discussing 'struct sdw_master_driver'. Please be very specific in > > you replies and do not use incorrect terminology which confuses people. > > > > Sorry a 'struct sdw_master_driver' IMHO is. As I have said it is not > > needed if you have standalone controller even in Intel case, and rest of > > the world. > > You're splitting hair without providing a solution. > > Please see the series [PATCH 0/5] soundwire: add sdw_master_device support > on Qualcomm platforms > > This solution was tested on Qualcomm platforms, that doesn't require this > sdw_master_driver to be used, so your objections are now invalid. I have given you a solution which you dont like. I have asked you to talk to your colleagues at Intel, I have not heard back. I cant do anymore than this. testing on QC boards doesnt make sense, the contention is 'sdw_master_driver' which doesnt get used. I have said earlier, will say again, if you drop this piece I am ready to apply the rest of the patches.
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index e2cdff990e9f..7319918e0aec 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -4,7 +4,7 @@ # #Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o +soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o ifdef CONFIG_DEBUG_FS diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 17f096dd6806..e610f1d3b840 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -33,13 +33,33 @@ sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv) static int sdw_bus_match(struct device *dev, struct device_driver *ddrv) { - struct sdw_slave *slave = dev_to_sdw_dev(dev); - struct sdw_driver *drv = drv_to_sdw_driver(ddrv); + struct sdw_slave *slave; + struct sdw_driver *drv; + struct sdw_master_device *md; + struct sdw_master_driver *mdrv; + int ret = 0; - return !!sdw_get_device_id(slave, drv); + if (is_sdw_slave(dev)) { + slave = dev_to_sdw_dev(dev); + drv = drv_to_sdw_driver(ddrv); + + ret = !!sdw_get_device_id(slave, drv); + } else { + md = dev_to_sdw_master_device(dev); + mdrv = drv_to_sdw_master_driver(ddrv); + + /* + * we don't have any hardware information so + * match with a hopefully unique string + */ + ret = !strncmp(md->master_name, mdrv->driver.name, + strlen(md->master_name)); + } + return ret; } -int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) +static int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, + size_t size) { /* modalias is sdw:m<mfg_id>p<part_id> */ @@ -47,12 +67,31 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size) slave->id.mfg_id, slave->id.part_id); } +static int sdw_master_modalias(const struct sdw_master_device *md, + char *buf, size_t size) +{ + /* modalias is sdw:<string> since we don't have any hardware info */ + + return snprintf(buf, size, "sdw:%s\n", + md->master_name); +} + static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) { - struct sdw_slave *slave = dev_to_sdw_dev(dev); + struct sdw_slave *slave; + struct sdw_master_device *md; char modalias[32]; - sdw_slave_modalias(slave, modalias, sizeof(modalias)); + if (is_sdw_slave(dev)) { + slave = dev_to_sdw_dev(dev); + + sdw_slave_modalias(slave, modalias, sizeof(modalias)); + + } else { + md = dev_to_sdw_master_device(dev); + + sdw_master_modalias(md, modalias, sizeof(modalias)); + } if (add_uevent_var(env, "MODALIAS=%s", modalias)) return -ENOMEM; @@ -113,8 +152,6 @@ static int sdw_drv_probe(struct device *dev) slave->probed = true; complete(&slave->probe_complete); - dev_dbg(dev, "probe complete\n"); - return 0; } @@ -181,6 +218,94 @@ void sdw_unregister_driver(struct sdw_driver *drv) } EXPORT_SYMBOL_GPL(sdw_unregister_driver); +static int sdw_master_drv_probe(struct device *dev) +{ + struct sdw_master_device *md = dev_to_sdw_master_device(dev); + struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver); + int ret; + + /* + * attach to power domain but don't turn on (last arg) + */ + ret = dev_pm_domain_attach(dev, false); + if (ret) + return ret; + + ret = mdrv->probe(md, md->pdata); + if (ret) { + dev_err(dev, "Probe of %s failed: %d\n", + mdrv->driver.name, ret); + dev_pm_domain_detach(dev, false); + return ret; + } + + return 0; +} + +static int sdw_master_drv_remove(struct device *dev) +{ + struct sdw_master_device *md = dev_to_sdw_master_device(dev); + struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver); + int ret = 0; + + if (mdrv->remove) + ret = mdrv->remove(md); + + dev_pm_domain_detach(dev, false); + + return ret; +} + +static void sdw_master_drv_shutdown(struct device *dev) +{ + struct sdw_master_device *md = dev_to_sdw_master_device(dev); + struct sdw_master_driver *mdrv = drv_to_sdw_master_driver(dev->driver); + + if (mdrv->shutdown) + mdrv->shutdown(md); +} + +/** + * __sdw_register_master_driver() - register a SoundWire Master driver + * @mdrv: 'Master driver' to register + * @owner: owning module/driver + * + * Return: zero on success, else a negative error code. + */ +int __sdw_register_master_driver(struct sdw_master_driver *mdrv, + struct module *owner) +{ + mdrv->driver.bus = &sdw_bus_type; + + if (!mdrv->probe) { + pr_err("driver %s didn't provide SDW probe routine\n", + mdrv->driver.name); + return -EINVAL; + } + + mdrv->driver.owner = owner; + mdrv->driver.probe = sdw_master_drv_probe; + + if (mdrv->remove) + mdrv->driver.remove = sdw_master_drv_remove; + + if (mdrv->shutdown) + mdrv->driver.shutdown = sdw_master_drv_shutdown; + + return driver_register(&mdrv->driver); +} +EXPORT_SYMBOL_GPL(__sdw_register_master_driver); + +/** + * sdw_unregister_master_driver() - unregisters the SoundWire Master driver + * @mdrv: driver to unregister + */ +void sdw_unregister_master_driver(struct sdw_master_driver *mdrv) +{ + driver_unregister(&mdrv->driver); +} +EXPORT_SYMBOL_GPL(sdw_unregister_master_driver); + static int __init sdw_bus_init(void) { sdw_debugfs_init(); diff --git a/drivers/soundwire/master.c b/drivers/soundwire/master.c new file mode 100644 index 000000000000..1f3c376327f9 --- /dev/null +++ b/drivers/soundwire/master.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: (GPL-2.0) +// Copyright(c) 2019-2020 Intel Corporation. + +#include <linux/device.h> +#include <linux/acpi.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_type.h> +#include "bus.h" + +static void sdw_master_device_release(struct device *dev) +{ + struct sdw_master_device *md = dev_to_sdw_master_device(dev); + + kfree(md); +} + +struct device_type sdw_master_type = { + .name = "soundwire_master", + .release = sdw_master_device_release, +}; + +struct sdw_master_device +*sdw_master_device_add(const char *master_name, + struct device *parent, + struct fwnode_handle *fwnode, + int link_id, + void *pdata) +{ + struct sdw_master_device *md; + int ret; + + md = kzalloc(sizeof(*md), GFP_KERNEL); + if (!md) + return ERR_PTR(-ENOMEM); + + md->link_id = link_id; + md->pdata = pdata; + md->master_name = master_name; + + init_completion(&md->probe_complete); + + md->dev.parent = parent; + md->dev.fwnode = fwnode; + md->dev.bus = &sdw_bus_type; + md->dev.type = &sdw_master_type; + md->dev.dma_mask = md->dev.parent->dma_mask; + dev_set_name(&md->dev, "sdw-master-%d", md->link_id); + + ret = device_register(&md->dev); + if (ret) { + dev_err(parent, "Failed to add master: ret %d\n", ret); + /* + * On err, don't free but drop ref as this will be freed + * when release method is invoked. + */ + put_device(&md->dev); + return ERR_PTR(-ENOMEM); + } + + return md; +} +EXPORT_SYMBOL_GPL(sdw_master_device_add); + +int sdw_master_device_startup(struct sdw_master_device *md) +{ + struct sdw_master_driver *mdrv; + struct device *dev; + int ret = 0; + + if (IS_ERR_OR_NULL(md)) + return -EINVAL; + + dev = &md->dev; + mdrv = drv_to_sdw_master_driver(dev->driver); + + if (mdrv && mdrv->startup) + ret = mdrv->startup(md); + + return ret; +} +EXPORT_SYMBOL_GPL(sdw_master_device_startup); + +int sdw_master_device_process_wake_event(struct sdw_master_device *md) +{ + struct sdw_master_driver *mdrv; + struct device *dev; + int ret = 0; + + if (IS_ERR_OR_NULL(md)) + return -EINVAL; + + dev = &md->dev; + mdrv = drv_to_sdw_master_driver(dev->driver); + + if (mdrv && mdrv->process_wake_event) + ret = mdrv->process_wake_event(md); + + return ret; +} +EXPORT_SYMBOL_GPL(sdw_master_device_process_wake_event); diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index aace57fae7f8..7ca4f2d9bfa6 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -14,6 +14,11 @@ static void sdw_slave_release(struct device *dev) kfree(slave); } +struct device_type sdw_slave_type = { + .name = "sdw_slave", + .release = sdw_slave_release, +}; + static int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id, struct fwnode_handle *fwnode) { @@ -41,9 +46,9 @@ static int sdw_slave_add(struct sdw_bus *bus, id->class_id, id->unique_id); } - slave->dev.release = sdw_slave_release; slave->dev.bus = &sdw_bus_type; slave->dev.of_node = of_node_get(to_of_node(fwnode)); + slave->dev.type = &sdw_slave_type; slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; init_completion(&slave->enumeration_complete); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ee349a4c5349..a64fde620ae6 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -630,6 +630,31 @@ struct sdw_slave { #define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev) +/** + * struct sdw_master_device - SoundWire 'Master Device' representation + * + * @dev: Linux device for this Master + * @master_name: Linux driver name + * @driver: Linux driver for this Master (set by SoundWire core during probe) + * @probe_complete: used by parent if synchronous probe behavior is needed + * @link_id: link index as defined by MIPI DisCo specification + * @pm_runtime_suspended: flag to restore pm_runtime state after system resume + * @pdata: private data typically provided with sdw_master_device_add() + */ + +struct sdw_master_device { + struct device dev; + const char *master_name; + struct sdw_master_driver *driver; + struct completion probe_complete; + int link_id; + bool pm_runtime_suspended; + void *pdata; +}; + +#define dev_to_sdw_master_device(d) \ + container_of(d, struct sdw_master_device, dev) + struct sdw_driver { const char *name; @@ -644,6 +669,26 @@ struct sdw_driver { struct device_driver driver; }; +/** + * struct sdw_master_driver - SoundWire 'Master Device' driver + * + * @probe: initializations and allocation (hardware may not be enabled yet) + * @startup: initialization handled after the hardware is enabled, all + * clock/power dependencies are available (optional) + * @shutdown: cleanups before hardware is disabled (optional) + * @remove: free all remaining resources + * @process_wake_event: handle external wake (optional) + * @driver: baseline structure used for name/PM hooks. + */ +struct sdw_master_driver { + int (*probe)(struct sdw_master_device *md, void *link_ctx); + int (*startup)(struct sdw_master_device *md); + int (*shutdown)(struct sdw_master_device *md); + int (*remove)(struct sdw_master_device *md); + int (*process_wake_event)(struct sdw_master_device *md); + struct device_driver driver; +}; + #define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \ { .mfg_id = (_mfg_id), .part_id = (_part_id), \ .driver_data = (unsigned long)(_drv_data) } @@ -833,6 +878,37 @@ struct sdw_bus { int sdw_add_bus_master(struct sdw_bus *bus); void sdw_delete_bus_master(struct sdw_bus *bus); +/** + * sdw_master_device_add() - create a Linux Master Device representation. + * + * @master_name: Linux driver name + * @parent: the parent Linux device (e.g. a PCI device) + * @fwnode: the parent fwnode (e.g. an ACPI companion device to the parent) + * @link_id: link index as defined by MIPI DisCo specification + * @pdata: private data (e.g. register base, offsets, platform quirks, etc). + */ +struct sdw_master_device +*sdw_master_device_add(const char *master_name, + struct device *parent, + struct fwnode_handle *fwnode, + int link_id, + void *pdata); + +/** + * sdw_master_device_startup() - startup hardware + * + * @md: Linux Soundwire master device + */ +int sdw_master_device_startup(struct sdw_master_device *md); + +/** + * sdw_master_device_process_wake_event() - handle external wake + * event, e.g. handled at the PCI level + * + * @md: Linux Soundwire master device + */ +int sdw_master_device_process_wake_event(struct sdw_master_device *md); + /** * sdw_port_config: Master or Slave Port configuration * diff --git a/include/linux/soundwire/sdw_type.h b/include/linux/soundwire/sdw_type.h index aaa7f4267c14..331ba58bda27 100644 --- a/include/linux/soundwire/sdw_type.h +++ b/include/linux/soundwire/sdw_type.h @@ -5,16 +5,36 @@ #define __SOUNDWIRE_TYPES_H extern struct bus_type sdw_bus_type; +extern struct device_type sdw_slave_type; +extern struct device_type sdw_master_type; + +static inline int is_sdw_slave(const struct device *dev) +{ + return dev->type == &sdw_slave_type; +} #define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver) #define sdw_register_driver(drv) \ __sdw_register_driver(drv, THIS_MODULE) +static inline int is_sdw_master_device(const struct device *dev) +{ + return dev->type == &sdw_master_type; +} + +#define drv_to_sdw_master_driver(_drv) \ + container_of(_drv, struct sdw_master_driver, driver) + +#define sdw_register_master_driver(drv) \ + __sdw_register_master_driver(drv, THIS_MODULE) + int __sdw_register_driver(struct sdw_driver *drv, struct module *owner); void sdw_unregister_driver(struct sdw_driver *drv); -int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size); +int __sdw_register_master_driver(struct sdw_master_driver *mdrv, + struct module *owner); +void sdw_unregister_master_driver(struct sdw_master_driver *mdrv); /** * module_sdw_driver() - Helper macro for registering a Soundwire driver @@ -27,4 +47,18 @@ int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size); #define module_sdw_driver(__sdw_driver) \ module_driver(__sdw_driver, sdw_register_driver, \ sdw_unregister_driver) + +/** + * module_sdw_master_driver() - Helper macro for registering a Soundwire + * Master driver + * @__sdw_master_driver: soundwire Master driver struct + * + * Helper macro for Soundwire Master drivers which do not do anything special in + * module init/exit. This eliminates a lot of boilerplate. Each module may only + * use this macro once, and calling it replaces module_init() and module_exit() + */ +#define module_sdw_master_driver(__sdw_master_driver) \ + module_driver(__sdw_master_driver, sdw_register_master_driver, \ + sdw_unregister_master_driver) + #endif /* __SOUNDWIRE_TYPES_H */