[PATCH 0/8] soundwire: remove platform devices, add SOF interfaces
The first two patches were already reviewed by Greg KH in an earlier RFC. Since I only cleaned-up the error handling flow and removed an unnecessary wrapper, I took the liberty of adding Greg's Reviewed-by tag for these two patches.
The rest of the patches implement the interfaces required for the SOF driver (interrupts handled with a single handler, PCI wakes and sharing of _ADR information to select a machine driver).
When this patchset is merged, a tag will need to be shared with Mark Brown so that we can provide the SOF patches to the ASoC tree and enable SoundWire in builds for Intel platforms.
Bard Liao (2): soundwire: intel/cadence: merge Soundwire interrupt handlers/threads soundwire: intel_init: save Slave(s) _ADR info in sdw_intel_ctx
Pierre-Louis Bossart (5): soundwire: bus_type: add master_device/driver support soundwire: intel: transition to sdw_master_device/driver support soundwire: intel_init: add implementation of sdw_intel_enable_irq() soundwire: intel_init: use EXPORT_SYMBOL_NS soundwire: intel: add helpers for link power down and shim wake
Rander Wang (1): soundwire: intel: add wake interrupt support
drivers/soundwire/Makefile | 2 +- drivers/soundwire/bus_type.c | 141 ++++++++++- drivers/soundwire/cadence_master.c | 18 +- drivers/soundwire/cadence_master.h | 4 + drivers/soundwire/intel.c | 182 +++++++++++--- drivers/soundwire/intel.h | 12 +- drivers/soundwire/intel_init.c | 365 +++++++++++++++++++++++------ drivers/soundwire/master.c | 100 ++++++++ drivers/soundwire/slave.c | 7 +- include/linux/soundwire/sdw.h | 76 ++++++ include/linux/soundwire/sdw_type.h | 36 ++- 11 files changed, 819 insertions(+), 124 deletions(-) create mode 100644 drivers/soundwire/master.c
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
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 */
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, 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.
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.
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.
Use sdw_master_device and driver instead of platform devices
In addition, rather than a plain-vanilla init/exit, this patch provides 3 steps in the initialization (ACPI scan, probe, startup) which makes it easier to detect platform support for SoundWire, allocate required resources as early as possible, and conversely help make the startup() callback lighter-weight with only hardware register setup.
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 97 ++++++----- drivers/soundwire/intel.h | 8 +- drivers/soundwire/intel_init.c | 284 +++++++++++++++++++++++++-------- 3 files changed, 286 insertions(+), 103 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index a327669c757b..5a583382cec4 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -11,12 +11,12 @@ #include <linux/module.h> #include <linux/interrupt.h> #include <linux/io.h> -#include <linux/platform_device.h> #include <sound/pcm_params.h> #include <sound/soc.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_intel.h> +#include <linux/soundwire/sdw_type.h> #include "cadence_master.h" #include "bus.h" #include "intel.h" @@ -92,8 +92,6 @@ #define SDW_ALH_STRMZCFG_DMAT GENMASK(7, 0) #define SDW_ALH_STRMZCFG_CHN GENMASK(19, 16)
-#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) - enum intel_pdi_type { INTEL_PDI_IN = 0, INTEL_PDI_OUT = 1, @@ -1083,24 +1081,23 @@ static int intel_init(struct sdw_intel *sdw) /* * probe and init */ -static int intel_probe(struct platform_device *pdev) +static int intel_master_probe(struct sdw_master_device *md, void *link_ctx) { - struct sdw_cdns_stream_config config; struct sdw_intel *sdw; int ret;
- sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL); + sdw = devm_kzalloc(&md->dev, sizeof(*sdw), GFP_KERNEL); if (!sdw) return -ENOMEM;
- sdw->instance = pdev->id; - sdw->link_res = dev_get_platdata(&pdev->dev); - sdw->cdns.dev = &pdev->dev; + sdw->instance = md->link_id; + sdw->link_res = link_ctx; + sdw->cdns.dev = &md->dev; sdw->cdns.registers = sdw->link_res->registers; - sdw->cdns.instance = sdw->instance; + sdw->cdns.instance = md->link_id; sdw->cdns.msg_count = 0; - sdw->cdns.bus.dev = &pdev->dev; - sdw->cdns.bus.link_id = pdev->id; + sdw->cdns.bus.dev = &md->dev; + sdw->cdns.bus.link_id = md->link_id;
sdw_cdns_probe(&sdw->cdns);
@@ -1108,16 +1105,52 @@ static int intel_probe(struct platform_device *pdev) sdw_intel_ops.read_prop = intel_prop_read; sdw->cdns.bus.ops = &sdw_intel_ops;
- platform_set_drvdata(pdev, sdw); + md->pdata = sdw; + + /* set driver data, accessed by snd_soc_dai_set_drvdata() */ + dev_set_drvdata(&md->dev, &sdw->cdns);
ret = sdw_add_bus_master(&sdw->cdns.bus); if (ret) { - dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret); + dev_err(&md->dev, "sdw_add_bus_master fail: %d\n", ret); return ret; }
+ if (sdw->cdns.bus.prop.hw_disabled) + dev_info(&md->dev, + "SoundWire master %d is disabled, will be ignored\n", + sdw->cdns.bus.link_id); + + /* Acquire IRQ */ + ret = request_threaded_irq(sdw->link_res->irq, + sdw_cdns_irq, sdw_cdns_thread, + IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); + if (ret < 0) { + dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", + sdw->link_res->irq); + goto err_init; + } + + complete(&md->probe_complete); + + return 0; + +err_init: + sdw_delete_bus_master(&sdw->cdns.bus); + return ret; +} + +static int intel_master_startup(struct sdw_master_device *md) +{ + struct sdw_cdns_stream_config config; + struct sdw_intel *sdw; + int ret; + + sdw = md->pdata; + if (sdw->cdns.bus.prop.hw_disabled) { - dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n", + dev_info(&md->dev, + "SoundWire master %d is disabled, ignoring\n", sdw->cdns.bus.link_id); return 0; } @@ -1135,16 +1168,6 @@ static int intel_probe(struct platform_device *pdev)
intel_pdi_ch_update(sdw);
- /* Acquire IRQ */ - ret = request_threaded_irq(sdw->link_res->irq, - sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); - if (ret < 0) { - dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", - sdw->link_res->irq); - goto err_init; - } - ret = sdw_cdns_enable_interrupt(&sdw->cdns, true); if (ret < 0) { dev_err(sdw->cdns.dev, "cannot enable interrupts\n"); @@ -1171,17 +1194,17 @@ static int intel_probe(struct platform_device *pdev)
err_interrupt: sdw_cdns_enable_interrupt(&sdw->cdns, false); - free_irq(sdw->link_res->irq, sdw); err_init: + free_irq(sdw->link_res->irq, sdw); sdw_delete_bus_master(&sdw->cdns.bus); return ret; }
-static int intel_remove(struct platform_device *pdev) +static int intel_master_remove(struct sdw_master_device *md) { struct sdw_intel *sdw;
- sdw = platform_get_drvdata(pdev); + sdw = md->pdata;
if (!sdw->cdns.bus.prop.hw_disabled) { intel_debugfs_exit(sdw); @@ -1194,17 +1217,19 @@ static int intel_remove(struct platform_device *pdev) return 0; }
-static struct platform_driver sdw_intel_drv = { - .probe = intel_probe, - .remove = intel_remove, - .driver = { - .name = "int-sdw",
+static struct sdw_master_driver intel_sdw_driver = { + .driver = { + .name = "intel-master", + .owner = THIS_MODULE, + .bus = &sdw_bus_type, }, + .probe = intel_master_probe, + .startup = intel_master_startup, + .remove = intel_master_remove, }; - -module_platform_driver(sdw_intel_drv); +module_sdw_master_driver(intel_sdw_driver);
MODULE_LICENSE("Dual BSD/GPL"); -MODULE_ALIAS("platform:int-sdw"); +MODULE_ALIAS("sdw:intel-master"); MODULE_DESCRIPTION("Intel Soundwire Master Driver"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 38b7c125fb10..795d6213eba5 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -7,7 +7,7 @@ /** * struct sdw_intel_link_res - Soundwire Intel link resource structure, * typically populated by the controller driver. - * @pdev: platform_device + * @md: master device * @mmio_base: mmio base of SoundWire registers * @registers: Link IO registers base * @shim: Audio shim pointer @@ -17,7 +17,7 @@ * @dev: device implementing hw_params and free callbacks */ struct sdw_intel_link_res { - struct platform_device *pdev; + struct sdw_master_device *md; void __iomem *mmio_base; /* not strictly needed, useful for debug */ void __iomem *registers; void __iomem *shim; @@ -27,4 +27,8 @@ struct sdw_intel_link_res { struct device *dev; };
+#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) + +#define SDW_INTEL_MASTER_PROBE_TIMEOUT 2000 + #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 4b769409f6f8..a6ff7d2ae8be 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -11,7 +11,7 @@ #include <linux/export.h> #include <linux/io.h> #include <linux/module.h> -#include <linux/platform_device.h> +#include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_intel.h> #include "intel.h"
@@ -23,22 +23,51 @@ #define SDW_LINK_BASE 0x30000 #define SDW_LINK_SIZE 0x10000
-static int link_mask; -module_param_named(sdw_link_mask, link_mask, int, 0444); +static int ctrl_link_mask; +module_param_named(sdw_link_mask, ctrl_link_mask, int, 0444); MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)");
-static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) +static bool is_link_enabled(struct fwnode_handle *fw_node, int i) +{ + struct fwnode_handle *link; + char name[32]; + u32 quirk_mask = 0; + + /* Find master handle */ + snprintf(name, sizeof(name), + "mipi-sdw-link-%d-subproperties", i); + + link = fwnode_get_named_child_node(fw_node, name); + if (!link) + return false; + + fwnode_property_read_u32(link, + "intel-quirk-mask", + &quirk_mask); + + if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE) + return false; + + return true; +} + +static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) { struct sdw_intel_link_res *link = ctx->links; + u32 link_mask; int i;
if (!link) return 0;
- for (i = 0; i < ctx->count; i++) { - if (link->pdev) - platform_device_unregister(link->pdev); - link++; + link_mask = ctx->link_mask; + + for (i = 0; i < ctx->count; i++, link++) { + if (link_mask && !(link_mask & BIT(i))) + continue; + + if (!IS_ERR_OR_NULL(link->md)) + device_unregister(&link->md->dev); }
kfree(ctx->links); @@ -47,112 +76,208 @@ static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) return 0; }
-static struct sdw_intel_ctx -*sdw_intel_add_controller(struct sdw_intel_res *res) +static int +sdw_intel_scan_controller(struct sdw_intel_acpi_info *info) { - struct platform_device_info pdevinfo; - struct platform_device *pdev; - struct sdw_intel_link_res *link; - struct sdw_intel_ctx *ctx; struct acpi_device *adev; int ret, i; u8 count; - u32 caps;
- if (acpi_bus_get_device(res->handle, &adev)) - return NULL; + if (acpi_bus_get_device(info->handle, &adev)) + return -EINVAL;
/* Found controller, find links supported */ count = 0; ret = fwnode_property_read_u8_array(acpi_fwnode_handle(adev), "mipi-sdw-master-count", &count, 1);
- /* Don't fail on error, continue and use hw value */ + /* + * In theory we could check the number of links supported in + * hardware, but in that step we cannot assume SoundWire IP is + * powered. + * + * In addition, if the BIOS doesn't even provide this + * 'master-count' property then all the inits based on link + * masks will fail as well. + * + * We will check the hardware capabilities in the startup() step + */ + if (ret) { dev_err(&adev->dev, "Failed to read mipi-sdw-master-count: %d\n", ret); - count = SDW_MAX_LINKS; + return -EINVAL; }
- /* Check SNDWLCAP.LCOUNT */ - caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); - caps &= GENMASK(2, 0); - - /* Check HW supported vs property value and use min of two */ - count = min_t(u8, caps, count); - /* Check count is within bounds */ if (count > SDW_MAX_LINKS) { dev_err(&adev->dev, "Link count %d exceeds max %d\n", count, SDW_MAX_LINKS); - return NULL; + return -EINVAL; } else if (!count) { dev_warn(&adev->dev, "No SoundWire links detected\n"); - return NULL; + return -EINVAL; } + dev_dbg(&adev->dev, "ACPI reports %d SDW Link devices\n", count);
+ info->count = count; + + for (i = 0; i < count; i++) { + if (ctrl_link_mask && !(ctrl_link_mask & BIT(i))) { + dev_dbg(&adev->dev, + "Link %d masked, will not be enabled\n", i); + continue; + } + + if (!is_link_enabled(acpi_fwnode_handle(adev), i)) { + dev_dbg(&adev->dev, + "Link %d not selected in firmware\n", i); + continue; + } + + info->link_mask |= BIT(i); + } + + return 0; +} + +static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res) +{ + struct sdw_intel_link_res *link; + struct sdw_intel_ctx *ctx; + struct acpi_device *adev; + struct sdw_master_device *md; + unsigned long time; + u32 link_mask; + int count; + int i; + + if (!res) + return NULL; + + if (acpi_bus_get_device(res->handle, &adev)) + return NULL; + + if (!res->count) + return NULL; + + count = res->count; dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL;
- ctx->count = count; - ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL); + ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL); if (!ctx->links) goto link_err;
+ ctx->count = count; + ctx->mmio_base = res->mmio_base; + ctx->link_mask = res->link_mask; + ctx->handle = res->handle; + link = ctx->links; + link_mask = ctx->link_mask;
/* Create SDW Master devices */ - for (i = 0; i < count; i++) { - if (link_mask && !(link_mask & BIT(i))) { - dev_dbg(&adev->dev, - "Link %d masked, will not be enabled\n", i); - link++; + for (i = 0; i < count; i++, link++) { + if (link_mask && !(link_mask & BIT(i))) continue; - }
+ link->mmio_base = res->mmio_base; link->registers = res->mmio_base + SDW_LINK_BASE - + (SDW_LINK_SIZE * i); + + (SDW_LINK_SIZE * i); link->shim = res->mmio_base + SDW_SHIM_BASE; link->alh = res->mmio_base + SDW_ALH_BASE; - + link->irq = res->irq; link->ops = res->ops; link->dev = res->dev; + link->clock_stop_quirks = res->clock_stop_quirks;
- memset(&pdevinfo, 0, sizeof(pdevinfo)); + md = sdw_master_device_add("intel-master", + res->parent, + acpi_fwnode_handle(adev), + i, + link);
- pdevinfo.parent = res->parent; - pdevinfo.name = "int-sdw"; - pdevinfo.id = i; - pdevinfo.fwnode = acpi_fwnode_handle(adev); + if (IS_ERR(md)) { + dev_err(&adev->dev, "Could not create link %d\n", i); + goto err; + }
- pdev = platform_device_register_full(&pdevinfo); - if (IS_ERR(pdev)) { - dev_err(&adev->dev, - "platform device creation failed: %ld\n", - PTR_ERR(pdev)); - goto pdev_err; + /* + * we need to wait for the bus to be probed so that we + * can report ACPI information to the upper layers + */ + time = wait_for_completion_timeout(&md->probe_complete, + msecs_to_jiffies(SDW_INTEL_MASTER_PROBE_TIMEOUT)); + if (!time) { + dev_err(&adev->dev, "Master %d probe timed out\n", i); + goto err; }
- link->pdev = pdev; - link++; + link->md = md; }
return ctx;
-pdev_err: - sdw_intel_cleanup_pdev(ctx); +err: + ctx->count = i; + sdw_intel_cleanup(ctx); link_err: kfree(ctx); return NULL; }
+static int +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) +{ + struct acpi_device *adev; + struct sdw_intel_link_res *link; + struct sdw_master_device *md; + u32 caps; + u32 link_mask; + int i; + + if (acpi_bus_get_device(ctx->handle, &adev)) + return -EINVAL; + + /* Check SNDWLCAP.LCOUNT */ + caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); + caps &= GENMASK(2, 0); + + /* Check HW supported vs property value */ + if (caps < ctx->count) { + dev_err(&adev->dev, + "BIOS master count is larger than hardware capabilities\n"); + return -EINVAL; + } + + if (!ctx->links) + return -EINVAL; + + link = ctx->links; + link_mask = ctx->link_mask; + + /* Startup SDW Master devices */ + for (i = 0; i < ctx->count; i++, link++) { + if (link_mask && !(link_mask & BIT(i))) + continue; + + md = link->md; + + sdw_master_device_startup(md); + } + + return 0; +} + static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, void *cdata, void **return_value) { - struct sdw_intel_res *res = cdata; + struct sdw_intel_acpi_info *info = cdata; struct acpi_device *adev; acpi_status status; u64 adr; @@ -166,7 +291,7 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, return AE_NOT_FOUND; }
- res->handle = handle; + info->handle = handle;
/* * On some Intel platforms, multiple children of the HDAS @@ -183,36 +308,65 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, }
/** - * sdw_intel_init() - SoundWire Intel init routine + * sdw_intel_acpi_scan() - SoundWire Intel init routine * @parent_handle: ACPI parent handle - * @res: resource data + * @info: description of what firmware/DSDT tables expose * - * This scans the namespace and creates SoundWire link controller devices - * based on the info queried. + * This scans the namespace and queries firmware to figure out which + * links to enable. A follow-up use of sdw_intel_probe() and + * sdw_intel_startup() is required for creation of devices and bus + * startup */ -void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res) +int sdw_intel_acpi_scan(acpi_handle *parent_handle, + struct sdw_intel_acpi_info *info) { acpi_status status;
status = acpi_walk_namespace(ACPI_TYPE_DEVICE, parent_handle, 1, sdw_intel_acpi_cb, - NULL, res, NULL); + NULL, info, NULL); if (ACPI_FAILURE(status)) - return NULL; + return -ENODEV;
- return sdw_intel_add_controller(res); + return sdw_intel_scan_controller(info); } +EXPORT_SYMBOL(sdw_intel_acpi_scan);
+/** + * sdw_intel_probe() - SoundWire Intel probe routine + * @res: resource data + * + * This creates SoundWire Master and Slave devices below the controller. + * All the information necessary is stored in the context, and the res + * argument pointer can be freed after this step. + */ +struct sdw_intel_ctx +*sdw_intel_probe(struct sdw_intel_res *res) +{ + return sdw_intel_probe_controller(res); +} +EXPORT_SYMBOL(sdw_intel_probe); + +/** + * sdw_intel_startup() - SoundWire Intel startup + * @ctx: SoundWire context allocated in the probe + * + */ +int sdw_intel_startup(struct sdw_intel_ctx *ctx) +{ + return sdw_intel_startup_controller(ctx); +} +EXPORT_SYMBOL(sdw_intel_startup); /** * sdw_intel_exit() - SoundWire Intel exit - * @arg: callback context + * @ctx: SoundWire context allocated in the probe * * Delete the controller instances created and cleanup */ void sdw_intel_exit(struct sdw_intel_ctx *ctx) { - sdw_intel_cleanup_pdev(ctx); + sdw_intel_cleanup(ctx); kfree(ctx); } EXPORT_SYMBOL(sdw_intel_exit);
On Thu, Feb 27, 2020 at 04:32:00PM -0600, Pierre-Louis Bossart wrote:
+static struct sdw_master_driver intel_sdw_driver = {
- .driver = {
.name = "intel-master",.owner = THIS_MODULE,
Nit, setting .owner isn't needed anymore, as the core code handles this for you, right?
thanks,
greg k-h
On 2/28/20 1:34 AM, Greg KH wrote:
On Thu, Feb 27, 2020 at 04:32:00PM -0600, Pierre-Louis Bossart wrote:
+static struct sdw_master_driver intel_sdw_driver = {
- .driver = {
.name = "intel-master",.owner = THIS_MODULE,Nit, setting .owner isn't needed anymore, as the core code handles this for you, right?
Ah, yes indeed, the following code takes care of it. Thanks for spotting this!
I can remove this assignment in a follow-up patch if that's alright with Vinod.
+#define sdw_register_master_driver(drv) \ + __sdw_register_master_driver(drv, THIS_MODULE)
+/** + * __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;
On 27-02-20, 16:32, Pierre-Louis Bossart wrote:
+static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res) +{
struct sdw_intel_link_res *link;
struct sdw_intel_ctx *ctx;
struct acpi_device *adev;
struct sdw_master_device *md;
unsigned long time;
u32 link_mask;
int count;
int i;
if (!res)
return NULL;if (acpi_bus_get_device(res->handle, &adev))
return NULL;if (!res->count)
return NULL;count = res->count; dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL;
- ctx->count = count;
- ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);
ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL); if (!ctx->links) goto link_err;
ctx->count = count;
ctx->mmio_base = res->mmio_base;
ctx->link_mask = res->link_mask;
ctx->handle = res->handle;
link = ctx->links;
link_mask = ctx->link_mask;
/* Create SDW Master devices */
- for (i = 0; i < count; i++) {
if (link_mask && !(link_mask & BIT(i))) {dev_dbg(&adev->dev,"Link %d masked, will not be enabled\n", i);link++;
- for (i = 0; i < count; i++, link++) {
if (link_mask && !(link_mask & BIT(i))) continue;
}
link->mmio_base = res->mmio_base;
+ (SDW_LINK_SIZE * i);
+ (SDW_LINK_SIZE * i);
link->irq = res->irq;link->clock_stop_quirks = res->clock_stop_quirks;
memset(&pdevinfo, 0, sizeof(pdevinfo));
md = sdw_master_device_add("intel-master",res->parent,acpi_fwnode_handle(adev),i,link);
So we add the device in intel layer
pdevinfo.parent = res->parent;pdevinfo.name = "int-sdw";pdevinfo.id = i;pdevinfo.fwnode = acpi_fwnode_handle(adev);
if (IS_ERR(md)) {dev_err(&adev->dev, "Could not create link %d\n", i);goto err;}
pdev = platform_device_register_full(&pdevinfo);if (IS_ERR(pdev)) {dev_err(&adev->dev,"platform device creation failed: %ld\n",PTR_ERR(pdev));goto pdev_err;
/** we need to wait for the bus to be probed so that we* can report ACPI information to the upper layers*/time = wait_for_completion_timeout(&md->probe_complete,msecs_to_jiffies(SDW_INTEL_MASTER_PROBE_TIMEOUT));
Then wait for probe to complete..
+static int +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) +{
- struct acpi_device *adev;
- struct sdw_intel_link_res *link;
- struct sdw_master_device *md;
- u32 caps;
- u32 link_mask;
- int i;
- if (acpi_bus_get_device(ctx->handle, &adev))
return -EINVAL;- /* Check SNDWLCAP.LCOUNT */
- caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP);
- caps &= GENMASK(2, 0);
- /* Check HW supported vs property value */
- if (caps < ctx->count) {
dev_err(&adev->dev,"BIOS master count is larger than hardware capabilities\n");return -EINVAL;- }
- if (!ctx->links)
return -EINVAL;- link = ctx->links;
- link_mask = ctx->link_mask;
- /* Startup SDW Master devices */
- for (i = 0; i < ctx->count; i++, link++) {
if (link_mask && !(link_mask & BIT(i)))continue;md = link->md;sdw_master_device_startup(md);
And finally start the device.
If i look at bigger picture:
PCI SOF driver scans the capabilities and finds SoundWire supported: - Invokes sdw_intel_probe_controller() - This adds sdw_master_device and waits till the probe is complete. - Invokes sdw_intel_startup_controller() - It starts up the controller by calling sdw_master_device_startup()
Now, I guess at the peril of repeating myself again:
Why not do:
- PCI SOF driver scans the capabilities and finds SoundWire supported: - Invokes sdw_intel_probe_controller() - This adds sdw_master_device - Does rest of device init and context init - Invoked sdw_intel_startup_controller() - Calls sdw_master_device_startup() to start
You will get more fine grained control of the sequencing, no need to wait for dummy probe to be over. The device for these would be parent PCI SOF device and driver PCI SOF driver.
So in summary I do not see a reason for even Intel to have a dummy soundwire_master_driver.
This function is required to enable all interrupts across all links.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel_init.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index a6ff7d2ae8be..88ad2a1637ad 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -141,6 +141,30 @@ sdw_intel_scan_controller(struct sdw_intel_acpi_info *info) return 0; }
+#define HDA_DSP_REG_ADSPIC2 (0x10) +#define HDA_DSP_REG_ADSPIS2 (0x14) +#define HDA_DSP_REG_ADSPIC2_SNDW BIT(5) + +/** + * sdw_intel_enable_irq() - enable/disable Intel SoundWire IRQ + * @mmio_base: The mmio base of the control register + * @enable: true if enable + */ +void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable) +{ + u32 val; + + val = readl(mmio_base + HDA_DSP_REG_ADSPIC2); + + if (enable) + val |= HDA_DSP_REG_ADSPIC2_SNDW; + else + val &= ~HDA_DSP_REG_ADSPIC2_SNDW; + + writel(val, mmio_base + HDA_DSP_REG_ADSPIC2); +} +EXPORT_SYMBOL(sdw_intel_enable_irq); + static struct sdw_intel_ctx *sdw_intel_probe_controller(struct sdw_intel_res *res) {
Make sure all symbols in this soundwire-intel-init module are exported with a namespace.
The MODULE_IMPORT_NS will be used in Intel/SOF HDaudio modules to be posted in a separate series.
Namespaces are only introduced for the Intel parts of the SoundWire code at this time, in future patches we should also add namespaces for Cadence parts and the SoundWire core.
Suggested-by: Greg KH gregkh@linuxfoundation.org Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel_init.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 88ad2a1637ad..01c8b390887b 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -163,7 +163,7 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable)
writel(val, mmio_base + HDA_DSP_REG_ADSPIC2); } -EXPORT_SYMBOL(sdw_intel_enable_irq); +EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
static struct sdw_intel_ctx *sdw_intel_probe_controller(struct sdw_intel_res *res) @@ -355,7 +355,7 @@ int sdw_intel_acpi_scan(acpi_handle *parent_handle,
return sdw_intel_scan_controller(info); } -EXPORT_SYMBOL(sdw_intel_acpi_scan); +EXPORT_SYMBOL_NS(sdw_intel_acpi_scan, SOUNDWIRE_INTEL_INIT);
/** * sdw_intel_probe() - SoundWire Intel probe routine @@ -370,7 +370,7 @@ struct sdw_intel_ctx { return sdw_intel_probe_controller(res); } -EXPORT_SYMBOL(sdw_intel_probe); +EXPORT_SYMBOL_NS(sdw_intel_probe, SOUNDWIRE_INTEL_INIT);
/** * sdw_intel_startup() - SoundWire Intel startup @@ -381,7 +381,7 @@ int sdw_intel_startup(struct sdw_intel_ctx *ctx) { return sdw_intel_startup_controller(ctx); } -EXPORT_SYMBOL(sdw_intel_startup); +EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT); /** * sdw_intel_exit() - SoundWire Intel exit * @ctx: SoundWire context allocated in the probe @@ -393,7 +393,7 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx) sdw_intel_cleanup(ctx); kfree(ctx); } -EXPORT_SYMBOL(sdw_intel_exit); +EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_DESCRIPTION("Intel Soundwire Init Library");
From: Bard Liao yung-chuan.liao@linux.intel.com
The existing code uses one pair of interrupt handler/thread per link but at the hardware level the interrupt is shared. This works fine for legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled Interrupt) mode, likely due to edges being lost.
This patch unifies interrupt handling for all links. The dedicated handler is removed since we use a common one for all shared interrupt sources, and the thread function takes care of dealing with interrupt sources. This partition follows the model used for the SOF IPC on HDaudio platforms, where similar timeout issues were noticed and doing all the interrupt handling/clearing in the thread improved reliability/stability.
Validation results with 4 links active in parallel show a night-and-day improvement with no timeouts noticed even during stress tests. Latency and quality of service are not affected by the change - mostly because events on a SoundWire link are throttled by the bus frame rate (typically 8..48kHz).
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 18 ++++++++++-------- drivers/soundwire/cadence_master.h | 4 ++++ drivers/soundwire/intel.c | 17 +---------------- drivers/soundwire/intel.h | 4 ++++ drivers/soundwire/intel_init.c | 20 +++++++++++++++++++- 5 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 9bec270d0fa4..bd868746ad22 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -17,6 +17,7 @@ #include <linux/soundwire/sdw.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <linux/workqueue.h> #include "bus.h" #include "cadence_master.h"
@@ -776,7 +777,7 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) CDNS_MCP_INT_SLAVE_MASK, 0);
int_status &= ~CDNS_MCP_INT_SLAVE_MASK; - ret = IRQ_WAKE_THREAD; + schedule_work(&cdns->work); }
cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status); @@ -785,13 +786,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) EXPORT_SYMBOL(sdw_cdns_irq);
/** - * sdw_cdns_thread() - Cadence irq thread handler - * @irq: irq number - * @dev_id: irq context + * To update slave status in a work since we will need to handle + * other interrupts eg. CDNS_MCP_INT_RX_WL during the update slave + * process. + * @work: cdns worker thread */ -irqreturn_t sdw_cdns_thread(int irq, void *dev_id) +static void cdns_update_slave_status_work(struct work_struct *work) { - struct sdw_cdns *cdns = dev_id; + struct sdw_cdns *cdns = + container_of(work, struct sdw_cdns, work); u32 slave0, slave1;
dev_dbg_ratelimited(cdns->dev, "Slave status change\n"); @@ -808,9 +811,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id) cdns_updatel(cdns, CDNS_MCP_INTMASK, CDNS_MCP_INT_SLAVE_MASK, CDNS_MCP_INT_SLAVE_MASK);
- return IRQ_HANDLED; } -EXPORT_SYMBOL(sdw_cdns_thread);
/* * init routines @@ -1226,6 +1227,7 @@ int sdw_cdns_probe(struct sdw_cdns *cdns) init_completion(&cdns->tx_complete); cdns->bus.port_ops = &cdns_port_ops;
+ INIT_WORK(&cdns->work, cdns_update_slave_status_work); return 0; } EXPORT_SYMBOL(sdw_cdns_probe); diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 2de1b2493ffc..ce68ffe757fd 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -126,6 +126,10 @@ struct sdw_cdns {
bool link_up; unsigned int msg_count; + + struct work_struct work; + + struct list_head list; };
#define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 5a583382cec4..d5533564195a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1098,6 +1098,7 @@ static int intel_master_probe(struct sdw_master_device *md, void *link_ctx) sdw->cdns.msg_count = 0; sdw->cdns.bus.dev = &md->dev; sdw->cdns.bus.link_id = md->link_id; + sdw->link_res->cdns = &sdw->cdns;
sdw_cdns_probe(&sdw->cdns);
@@ -1121,23 +1122,9 @@ static int intel_master_probe(struct sdw_master_device *md, void *link_ctx) "SoundWire master %d is disabled, will be ignored\n", sdw->cdns.bus.link_id);
- /* Acquire IRQ */ - ret = request_threaded_irq(sdw->link_res->irq, - sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); - if (ret < 0) { - dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", - sdw->link_res->irq); - goto err_init; - } - complete(&md->probe_complete);
return 0; - -err_init: - sdw_delete_bus_master(&sdw->cdns.bus); - return ret; }
static int intel_master_startup(struct sdw_master_device *md) @@ -1195,7 +1182,6 @@ static int intel_master_startup(struct sdw_master_device *md) err_interrupt: sdw_cdns_enable_interrupt(&sdw->cdns, false); err_init: - free_irq(sdw->link_res->irq, sdw); sdw_delete_bus_master(&sdw->cdns.bus); return ret; } @@ -1209,7 +1195,6 @@ static int intel_master_remove(struct sdw_master_device *md) if (!sdw->cdns.bus.prop.hw_disabled) { intel_debugfs_exit(sdw); sdw_cdns_enable_interrupt(&sdw->cdns, false); - free_irq(sdw->link_res->irq, sdw); snd_soc_unregister_component(sdw->cdns.dev); } sdw_delete_bus_master(&sdw->cdns.bus); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 795d6213eba5..2027ad5d0e8a 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -15,6 +15,8 @@ * @irq: Interrupt line * @ops: Shim callback ops * @dev: device implementing hw_params and free callbacks + * @cdns: Cadence master descriptor + * @list: used to walk-through all masters exposed by the same controller */ struct sdw_intel_link_res { struct sdw_master_device *md; @@ -25,6 +27,8 @@ struct sdw_intel_link_res { int irq; const struct sdw_intel_ops *ops; struct device *dev; + struct sdw_cdns *cdns; + struct list_head list; };
#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 01c8b390887b..954b21b4712d 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -9,10 +9,12 @@
#include <linux/acpi.h> #include <linux/export.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_intel.h> +#include "cadence_master.h" #include "intel.h"
#define SDW_LINK_TYPE 4 /* from Intel ACPI documentation */ @@ -165,6 +167,19 @@ void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable) } EXPORT_SYMBOL_NS(sdw_intel_enable_irq, SOUNDWIRE_INTEL_INIT);
+irqreturn_t sdw_intel_thread(int irq, void *dev_id) +{ + struct sdw_intel_ctx *ctx = dev_id; + struct sdw_intel_link_res *link; + + list_for_each_entry(link, &ctx->link_list, list) + sdw_cdns_irq(irq, link->cdns); + + sdw_intel_enable_irq(ctx->mmio_base, true); + return IRQ_HANDLED; +} +EXPORT_SYMBOL(sdw_intel_thread); + static struct sdw_intel_ctx *sdw_intel_probe_controller(struct sdw_intel_res *res) { @@ -205,6 +220,8 @@ static struct sdw_intel_ctx link = ctx->links; link_mask = ctx->link_mask;
+ INIT_LIST_HEAD(&ctx->link_list); + /* Create SDW Master devices */ for (i = 0; i < count; i++, link++) { if (link_mask && !(link_mask & BIT(i))) @@ -215,7 +232,6 @@ static struct sdw_intel_ctx + (SDW_LINK_SIZE * i); link->shim = res->mmio_base + SDW_SHIM_BASE; link->alh = res->mmio_base + SDW_ALH_BASE; - link->irq = res->irq; link->ops = res->ops; link->dev = res->dev; link->clock_stop_quirks = res->clock_stop_quirks; @@ -243,6 +259,8 @@ static struct sdw_intel_ctx }
link->md = md; + + list_add_tail(&link->list, &ctx->link_list); }
return ctx;
These routines are required for power management
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index d5533564195a..eafa2016c76d 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -362,6 +362,59 @@ static int intel_shim_init(struct sdw_intel *sdw) return ret; }
+static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable) +{ + void __iomem *shim = sdw->link_res->shim; + unsigned int link_id = sdw->instance; + u16 wake_en, wake_sts; + + if (wake_enable) { + /* Enable the wakeup */ + intel_writew(shim, SDW_SHIM_WAKEEN, + (SDW_SHIM_WAKEEN_ENABLE << link_id)); + } else { + /* Disable the wake up interrupt */ + wake_en = intel_readw(shim, SDW_SHIM_WAKEEN); + wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id); + intel_writew(shim, SDW_SHIM_WAKEEN, wake_en); + + /* Clear wake status */ + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS); + wake_sts |= (SDW_SHIM_WAKEEN_ENABLE << link_id); + intel_writew(shim, SDW_SHIM_WAKESTS_STATUS, wake_sts); + } +} + +static int intel_link_power_down(struct sdw_intel *sdw) +{ + int link_control, spa_mask, cpa_mask, ret; + unsigned int link_id = sdw->instance; + void __iomem *shim = sdw->link_res->shim; + u16 ioctl; + + /* Glue logic */ + ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id)); + ioctl |= SDW_SHIM_IOCTL_BKE; + ioctl |= SDW_SHIM_IOCTL_COE; + intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl); + + ioctl &= ~(SDW_SHIM_IOCTL_MIF); + intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl); + + /* Link power down sequence */ + link_control = intel_readl(shim, SDW_SHIM_LCTL); + spa_mask = ~(SDW_SHIM_LCTL_SPA << link_id); + cpa_mask = (SDW_SHIM_LCTL_CPA << link_id); + link_control &= spa_mask; + + ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask); + if (ret < 0) + return ret; + + sdw->cdns.link_up = false; + return 0; +} + /* * PDI routines */
From: Rander Wang rander.wang@intel.com
When system is suspended in clock stop mode on intel platforms, both master and slave are in clock stop mode and soundwire bus is taken over by a glue hardware. The bus message for jack event is processed by this glue hardware, which will trigger an interrupt to resume audio pci device. Then audio pci driver will resume soundwire master and slave, transfer bus ownership to master, finally slave will report jack event to master and codec driver is triggered to check jack status.
Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 45 ++++++++++++++++++++++++++++++++++ drivers/soundwire/intel_init.c | 12 +++++++++ 2 files changed, 57 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index eafa2016c76d..4cc6c857dd09 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1255,6 +1255,50 @@ static int intel_master_remove(struct sdw_master_device *md) return 0; }
+static int intel_master_process_wakeen_event(struct sdw_master_device *md) +{ + struct sdw_intel *sdw; + struct sdw_slave *slave; + struct sdw_bus *bus; + void __iomem *shim; + u16 wake_sts; + + sdw = md->pdata; + + if (sdw->cdns.bus.prop.hw_disabled) { + dev_info(&md->dev, + "SoundWire master %d is disabled, ignoring\n", + sdw->cdns.bus.link_id); + return 0; + } + + shim = sdw->link_res->shim; + wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS); + + if (!(wake_sts & BIT(sdw->instance))) + return 0; + + /* disable WAKEEN interrupt ASAP to prevent interrupt flood */ + intel_shim_wake(sdw, false); + + bus = &sdw->cdns.bus; + + /* + * wake up master and slave so that slave can notify master + * the wakeen event and let codec driver check codec status + */ + list_for_each_entry(slave, &bus->slaves, node) { + if (slave->prop.wake_capable) { + if (slave->status != SDW_SLAVE_ATTACHED && + slave->status != SDW_SLAVE_ALERT) + continue; + + pm_request_resume(&slave->dev); + } + } + + return 0; +}
static struct sdw_master_driver intel_sdw_driver = { .driver = { @@ -1265,6 +1309,7 @@ static struct sdw_master_driver intel_sdw_driver = { .probe = intel_master_probe, .startup = intel_master_startup, .remove = intel_master_remove, + .process_wake_event = intel_master_process_wakeen_event, }; module_sdw_master_driver(intel_sdw_driver);
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 954b21b4712d..91ec91127f2a 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -413,5 +413,17 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx) } EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT);
+void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx) +{ + struct sdw_intel_link_res *link; + + if (!ctx->links) + return; + + list_for_each_entry(link, &ctx->link_list, list) + sdw_master_device_process_wake_event(link->md); +} +EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT); + MODULE_LICENSE("Dual BSD/GPL"); MODULE_DESCRIPTION("Intel Soundwire Init Library");
From: Bard Liao yung-chuan.liao@linux.intel.com
Save ACPI information in context so that we can match machine driver with sdw _ADR matching tables.
Signed-off-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel_init.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 91ec91127f2a..60d6d844ee4a 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -74,6 +74,8 @@ static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
kfree(ctx->links); ctx->links = NULL; + kfree(ctx->ids); + ctx->ids = NULL;
return 0; } @@ -188,7 +190,11 @@ static struct sdw_intel_ctx struct acpi_device *adev; struct sdw_master_device *md; unsigned long time; + struct sdw_slave *slave; + struct list_head *node; + struct sdw_bus *bus; u32 link_mask; + int num_slaves = 0; int count; int i;
@@ -261,6 +267,25 @@ static struct sdw_intel_ctx link->md = md;
list_add_tail(&link->list, &ctx->link_list); + bus = &link->cdns->bus; + /* Calculate number of slaves */ + list_for_each(node, &bus->slaves) + num_slaves++; + } + + ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL); + if (!ctx->ids) + goto err; + + ctx->num_slaves = num_slaves; + i = 0; + list_for_each_entry(link, &ctx->link_list, list) { + bus = &link->cdns->bus; + list_for_each_entry(slave, &bus->slaves, node) { + ctx->ids[i].id = slave->id; + ctx->ids[i].link_id = bus->link_id; + i++; + } }
return ctx;
On Thu, Feb 27, 2020 at 04:31:58PM -0600, Pierre-Louis Bossart wrote:
The first two patches were already reviewed by Greg KH in an earlier RFC. Since I only cleaned-up the error handling flow and removed an unnecessary wrapper, I took the liberty of adding Greg's Reviewed-by tag for these two patches.
That is fine, they still look sane to me :)
participants (3)
-
Greg KH -
Pierre-Louis Bossart -
Vinod Koul