[PATCH 0/8] soundwire: remove platform devices, add SOF interfaces

[PATCH 0/8] soundwire: remove platform devices, add SOF interfaces

[Pierre-Louis Bossart]

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

-- 
2.20.1

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

[Pierre-Louis Bossart]

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 */
-- 
2.20.1

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

[Greg KH]

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

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

[Pierre-Louis Bossart]

> 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.

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

[Vinod Koul]

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.
-- 
~Vinod

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

[Pierre-Louis Bossart]

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?

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

[Vinod Koul]

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.

-- 
~Vinod

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

[Pierre-Louis Bossart]

> 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

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

[Greg KH]

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

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

[Vinod Koul]

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.

-- 
~Vinod

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

[Vinod Koul]

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.

-- 
~Vinod 

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

[Pierre-Louis Bossart]

>> 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.

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

[Vinod Koul]

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
-- 
~Vinod

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

[Pierre-Louis Bossart]

>>> 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

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

[Vinod Koul]

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

Why can't you use dai .startup callback for this?

The ASoC core will do pm_runtime calls that will ensure PCI device is
up, DSP firmware downloaded and running.

You can use .startup() to turn on your link and .shutdown to turn off
the link.

-- 
~Vinod

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

[Pierre-Louis Bossart]

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.

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

[Vinod Koul]

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
-- 
~Vinod

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

[Pierre-Louis Bossart]

>>>> 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

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

[Vinod Koul]

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... 

-- 
~Vinod

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

[Pierre-Louis Bossart]

>> 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'?

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

[Vinod Koul]

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.

-- 
~Vinod

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

[Pierre-Louis Bossart]

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.

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

[Vinod Koul]

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.

-- 
~Vinod

[PATCH 2/8] soundwire: intel: transition to sdw_master_device/driver support

[Pierre-Louis Bossart]

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);
-- 
2.20.1

Re: [PATCH 2/8] soundwire: intel: transition to sdw_master_device/driver support

[Greg KH]

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

Re: [PATCH 2/8] soundwire: intel: transition to sdw_master_device/driver support

[Pierre-Louis Bossart]

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;

Re: [PATCH 2/8] soundwire: intel: transition to sdw_master_device/driver support

[Vinod Koul]

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;
>  		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);

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.

-- 
~Vinod

[PATCH 3/8] soundwire: intel_init: add implementation of sdw_intel_enable_irq()

[Pierre-Louis Bossart]

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)
 {
-- 
2.20.1

[PATCH 4/8] soundwire: intel_init: use EXPORT_SYMBOL_NS

[Pierre-Louis Bossart]

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");
-- 
2.20.1

[PATCH 5/8] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

[Pierre-Louis Bossart]

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;
-- 
2.20.1

[PATCH 6/8] soundwire: intel: add helpers for link power down and shim wake

[Pierre-Louis Bossart]

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
  */
-- 
2.20.1

[PATCH 7/8] soundwire: intel: add wake interrupt support

[Pierre-Louis Bossart]

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");
-- 
2.20.1

[PATCH 8/8] soundwire: intel_init: save Slave(s) _ADR info in sdw_intel_ctx

[Pierre-Louis Bossart]

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;
-- 
2.20.1

Re: [PATCH 0/8] soundwire: remove platform devices, add SOF interfaces

[Greg KH]

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 :)