[PATCH 0/4] soundwire: allow for more than 8 devices, keep IDA for wake-capable devices

[PATCH 0/4] soundwire: allow for more than 8 devices, keep IDA for wake-capable devices

[Bard Liao]

This series suggests a hybrid strategy for device number allocation, where
only wake-capable devices use a system-unique Device Number which will be
used on LunarLake to handle wake-ups using the HDaudio WAKEEN and WAKESTS.

Pierre-Louis Bossart (4):
  soundwire: add enum to control device number allocation
  soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY
  soundwire: extend parameters of new_peripheral_assigned() callback
  soundwire: intel_auxdevice: use SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

 drivers/soundwire/bus.c             | 28 ++++++++++++++++++++++------
 drivers/soundwire/intel_auxdevice.c | 26 ++++++++++++++++++++++----
 include/linux/soundwire/sdw.h       | 24 ++++++++++++++++++++++--
 3 files changed, 66 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/4] soundwire: add enum to control device number allocation

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Commit c60561014257 ("soundwire: bus: allow device number to be unique
at system level") introduced two strategies to allocate device
numbers.

a) a default unconstrained allocation, where each bus can allocate
Device Numbers independently.

b) an ida-based allocation. In this case each Device Number will be
unique at the system-level.

The IDA-based allocation is useful to simplify debug, but it was also
introduced as a prerequisite to deal with the Intel Lunar Lake
hardware programming sequences: the wake-ups have to be handled with a
system-unique SDI address at the HDaudio controller level.

At the time, the restriction introduced by the IDA to 8 devices total
seemed perfectly fine, but recently hardware vendors created
configurations with more than 8 devices.

This patch provides an iso-functionality change, with the allocation
selected with an enum instead of an 'ida_min' value. Follow-up patches
will add a new allocation strategy to allow for more than 8 devices
using information on the type of devices, and only use the IDA-based
allocation for devices capable of generating a wake.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/bus.c             |  4 ++--
 drivers/soundwire/intel_auxdevice.c |  1 +
 include/linux/soundwire/sdw.h       | 16 +++++++++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index b44f8d0affa6..e8c1c55a2a73 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -159,7 +159,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	if (slave->dev_num) { /* clear dev_num if assigned */
 		clear_bit(slave->dev_num, bus->assigned);
-		if (bus->dev_num_ida_min)
+		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
 			ida_free(&sdw_peripheral_ida, slave->dev_num);
 	}
 	list_del_init(&slave->node);
@@ -701,7 +701,7 @@ static int sdw_get_device_num(struct sdw_slave *slave)
 {
 	int bit;
 
-	if (slave->bus->dev_num_ida_min) {
+	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
 		bit = ida_alloc_range(&sdw_peripheral_ida,
 				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
 				      GFP_KERNEL);
diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 0daa6ca9a224..30f3d2ab80fd 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -165,6 +165,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
 	cdns->msg_count = 0;
 
 	bus->link_id = auxdev->id;
+	bus->dev_num_alloc = SDW_DEV_NUM_ALLOC_IDA;
 	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
 	bus->clk_stop_timeout = 1;
 
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index c076a3f879b3..4656d6d0f3bb 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -864,6 +864,17 @@ struct sdw_master_ops {
 	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
 };
 
+/**
+ * enum sdw_dev_num_alloc - Device Number allocation strategies
+ * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
+ * using range [1, 11]
+ * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
+ */
+enum sdw_dev_num_alloc {
+	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
+	SDW_DEV_NUM_ALLOC_IDA,
+};
+
 /**
  * struct sdw_bus - SoundWire bus
  * @dev: Shortcut to &bus->md->dev to avoid changing the entire code.
@@ -895,9 +906,11 @@ struct sdw_master_ops {
  * meaningful if multi_link is set. If set to 1, hardware-based
  * synchronization will be used even if a stream only uses a single
  * SoundWire segment.
+ * @dev_num_alloc: bus-specific device number allocation
  * @dev_num_ida_min: if set, defines the minimum values for the IDA
  * used to allocate system-unique device numbers. This value needs to be
- * identical across all SoundWire bus in the system.
+ * identical across all SoundWire bus in the system. Only used if @sdw_num_alloc
+ * is not default.
  */
 struct sdw_bus {
 	struct device *dev;
@@ -922,6 +935,7 @@ struct sdw_bus {
 	u32 bank_switch_timeout;
 	bool multi_link;
 	int hw_sync_min_links;
+	enum sdw_dev_num_alloc dev_num_alloc;
 	int dev_num_ida_min;
 };
 
-- 
2.25.1

[PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This patch adds a new Device Number allocation strategy, with the IDA
used only for devices that are wake-capable.

"regular" devices such as amplifiers will use Device Numbers
[1..min_ida-1].

"wake-capable" devices such as jack or microphone codecs will use
Device Numbers [min_ida..11].

This hybrid strategy extends the number of supported devices in a
system by only constraining the allocation if required, e.g. in the
case of Intel LunarLake platforms the wake-capable devices are
required to have a unique address to use the HDaudio SDI and HDAudio
WAKEEN/WAKESTS registers.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/bus.c       | 26 +++++++++++++++++++++-----
 include/linux/soundwire/sdw.h |  4 ++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index e8c1c55a2a73..6f465cce8369 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -159,7 +159,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	if (slave->dev_num) { /* clear dev_num if assigned */
 		clear_bit(slave->dev_num, bus->assigned);
-		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
+		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
+		    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
+		     slave->prop.wake_capable))
 			ida_free(&sdw_peripheral_ida, slave->dev_num);
 	}
 	list_del_init(&slave->node);
@@ -699,17 +701,31 @@ EXPORT_SYMBOL(sdw_compare_devid);
 /* called with bus_lock held */
 static int sdw_get_device_num(struct sdw_slave *slave)
 {
+	struct sdw_bus *bus = slave->bus;
 	int bit;
 
-	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
+	if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
+	    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
+	     slave->prop.wake_capable)) {
 		bit = ida_alloc_range(&sdw_peripheral_ida,
-				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
+				      bus->dev_num_ida_min, SDW_MAX_DEVICES,
 				      GFP_KERNEL);
 		if (bit < 0)
 			goto err;
 	} else {
-		bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES);
-		if (bit == SDW_MAX_DEVICES) {
+		int max_devices = SDW_MAX_DEVICES;
+
+		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
+		    !slave->prop.wake_capable) {
+			max_devices = bus->dev_num_ida_min - 1;
+
+			/* range check */
+			if (max_devices < 1 || max_devices > SDW_MAX_DEVICES)
+				return -EINVAL;
+		}
+
+		bit = find_first_zero_bit(bus->assigned, max_devices);
+		if (bit == max_devices) {
 			bit = -ENODEV;
 			goto err;
 		}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 4656d6d0f3bb..8a7541ac735e 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -869,10 +869,14 @@ struct sdw_master_ops {
  * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
  * using range [1, 11]
  * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
+ * @SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY: Hybrid allocation where wake-capable devices rely on
+ * IDA-based allocation and range [ida_min, 11], while regular devices rely on default
+ * allocation in range [1, ida_min - 1]
  */
 enum sdw_dev_num_alloc {
 	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
 	SDW_DEV_NUM_ALLOC_IDA,
+	SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY,
 };
 
 /**
-- 
2.25.1

[PATCH 3/4] soundwire: extend parameters of new_peripheral_assigned() callback

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The parameters are only the bus and the device number, manager ops may
need additional details on the type of peripheral connected, such as
whether it is wake-capable or not.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/bus.c             | 2 +-
 drivers/soundwire/intel_auxdevice.c | 4 +++-
 include/linux/soundwire/sdw.h       | 4 +++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 6f465cce8369..17b9a8bdf234 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -786,7 +786,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
 	slave->dev_num = slave->dev_num_sticky;
 
 	if (bus->ops && bus->ops->new_peripheral_assigned)
-		bus->ops->new_peripheral_assigned(bus, dev_num);
+		bus->ops->new_peripheral_assigned(bus, slave, dev_num);
 
 	return 0;
 }
diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 30f3d2ab80fd..c1df6f014e6b 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -60,7 +60,9 @@ static int generic_post_bank_switch(struct sdw_bus *bus)
 	return sdw->link_res->hw_ops->post_bank_switch(sdw);
 }
 
-static void generic_new_peripheral_assigned(struct sdw_bus *bus, int dev_num)
+static void generic_new_peripheral_assigned(struct sdw_bus *bus,
+					    struct sdw_slave *slave,
+					    int dev_num)
 {
 	struct sdw_cdns *cdns = bus_to_cdns(bus);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 8a7541ac735e..41a856bedf1e 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -861,7 +861,9 @@ struct sdw_master_ops {
 	int (*pre_bank_switch)(struct sdw_bus *bus);
 	int (*post_bank_switch)(struct sdw_bus *bus);
 	u32 (*read_ping_status)(struct sdw_bus *bus);
-	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
+	void (*new_peripheral_assigned)(struct sdw_bus *bus,
+					struct sdw_slave *slave,
+					int dev_num);
 };
 
 /**
-- 
2.25.1

[PATCH 4/4] soundwire: intel_auxdevice: use SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Change the allocation strategy on Intel platforms to use Device Numbers

[1..5]: unconstrained allocation with bus-specific Device Numbers,
typically for amplifiers.

[6..11]: IDA-based system-unique Device Number to be used for
wake-capable devices such as jack codecs or microphone codecs.

These values were chosen based on the typical maximum number of
devices per link given electrical/PHY limitations. This configuration
will e.g. allow for 8 amplifiers on 2 links, and additional devices on
the remaining links.

Example on Dell SDCA device with jack codec, two amplifiers and one
microphone codec: only the jack codec relies on the IDA, others use
the same Device Number 1 on separate links.

rt711-sdca sdw:0:025d:0711:01: signaling enumeration completion for Slave 6
rt1316-sdca sdw:1:025d:1316:01: signaling enumeration completion for Slave 1
rt1316-sdca sdw:2:025d:1316:01: signaling enumeration completion for Slave 1
rt715-sdca sdw:3:025d:0714:01: signaling enumeration completion for Slave 1

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/intel_auxdevice.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index c1df6f014e6b..917edc75ddfb 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -23,8 +23,12 @@
 #include "intel.h"
 #include "intel_auxdevice.h"
 
-/* IDA min selected to avoid conflicts with HDaudio/iDISP SDI values */
-#define INTEL_DEV_NUM_IDA_MIN           4
+/*
+ * IDA min selected to allow for 5 unconstrained devices per link,
+ * and 6 system-unique Device Numbers for wake-capable devices.
+ */
+
+#define INTEL_DEV_NUM_IDA_MIN           6
 
 #define INTEL_MASTER_SUSPEND_DELAY_MS	3000
 
@@ -66,9 +70,20 @@ static void generic_new_peripheral_assigned(struct sdw_bus *bus,
 {
 	struct sdw_cdns *cdns = bus_to_cdns(bus);
 	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	int min_dev = 1;
+	int max_dev = SDW_MAX_DEVICES;
+
+	if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
+		min_dev = INTEL_DEV_NUM_IDA_MIN;
+	} else if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY) {
+		if (slave->prop.wake_capable)
+			min_dev = INTEL_DEV_NUM_IDA_MIN;
+		else
+			max_dev = INTEL_DEV_NUM_IDA_MIN - 1;
+	}
 
 	/* paranoia check, this should never happen */
-	if (dev_num < INTEL_DEV_NUM_IDA_MIN || dev_num > SDW_MAX_DEVICES)  {
+	if (dev_num < min_dev || dev_num > max_dev)  {
 		dev_err(bus->dev, "%s: invalid dev_num %d\n", __func__, dev_num);
 		return;
 	}
@@ -167,7 +182,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
 	cdns->msg_count = 0;
 
 	bus->link_id = auxdev->id;
-	bus->dev_num_alloc = SDW_DEV_NUM_ALLOC_IDA;
+	bus->dev_num_alloc = SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY;
 	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
 	bus->clk_stop_timeout = 1;
 
-- 
2.25.1

Re: [PATCH 1/4] soundwire: add enum to control device number allocation

[Vinod Koul]

On 31-05-23, 11:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Commit c60561014257 ("soundwire: bus: allow device number to be unique
> at system level") introduced two strategies to allocate device
> numbers.
> 
> a) a default unconstrained allocation, where each bus can allocate
> Device Numbers independently.
> 
> b) an ida-based allocation. In this case each Device Number will be
> unique at the system-level.
> 
> The IDA-based allocation is useful to simplify debug, but it was also
> introduced as a prerequisite to deal with the Intel Lunar Lake
> hardware programming sequences: the wake-ups have to be handled with a
> system-unique SDI address at the HDaudio controller level.
> 
> At the time, the restriction introduced by the IDA to 8 devices total
> seemed perfectly fine, but recently hardware vendors created
> configurations with more than 8 devices.
> 
> This patch provides an iso-functionality change, with the allocation
> selected with an enum instead of an 'ida_min' value. Follow-up patches
> will add a new allocation strategy to allow for more than 8 devices
> using information on the type of devices, and only use the IDA-based
> allocation for devices capable of generating a wake.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c             |  4 ++--
>  drivers/soundwire/intel_auxdevice.c |  1 +
>  include/linux/soundwire/sdw.h       | 16 +++++++++++++++-
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index b44f8d0affa6..e8c1c55a2a73 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -159,7 +159,7 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  
>  	if (slave->dev_num) { /* clear dev_num if assigned */
>  		clear_bit(slave->dev_num, bus->assigned);
> -		if (bus->dev_num_ida_min)
> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
>  			ida_free(&sdw_peripheral_ida, slave->dev_num);
>  	}
>  	list_del_init(&slave->node);
> @@ -701,7 +701,7 @@ static int sdw_get_device_num(struct sdw_slave *slave)
>  {
>  	int bit;
>  
> -	if (slave->bus->dev_num_ida_min) {
> +	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
>  		bit = ida_alloc_range(&sdw_peripheral_ida,
>  				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
>  				      GFP_KERNEL);
> diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
> index 0daa6ca9a224..30f3d2ab80fd 100644
> --- a/drivers/soundwire/intel_auxdevice.c
> +++ b/drivers/soundwire/intel_auxdevice.c
> @@ -165,6 +165,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
>  	cdns->msg_count = 0;
>  
>  	bus->link_id = auxdev->id;
> +	bus->dev_num_alloc = SDW_DEV_NUM_ALLOC_IDA;
>  	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
>  	bus->clk_stop_timeout = 1;
>  
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index c076a3f879b3..4656d6d0f3bb 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -864,6 +864,17 @@ struct sdw_master_ops {
>  	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
>  };
>  
> +/**
> + * enum sdw_dev_num_alloc - Device Number allocation strategies
> + * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
> + * using range [1, 11]
> + * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
> + */
> +enum sdw_dev_num_alloc {
> +	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
> +	SDW_DEV_NUM_ALLOC_IDA,

Let default be IDA as 0, am sure we are not setting this field in qcom
or amd controller, lets retain the defaults please

> +};
> +
>  /**
>   * struct sdw_bus - SoundWire bus
>   * @dev: Shortcut to &bus->md->dev to avoid changing the entire code.
> @@ -895,9 +906,11 @@ struct sdw_master_ops {
>   * meaningful if multi_link is set. If set to 1, hardware-based
>   * synchronization will be used even if a stream only uses a single
>   * SoundWire segment.
> + * @dev_num_alloc: bus-specific device number allocation
>   * @dev_num_ida_min: if set, defines the minimum values for the IDA
>   * used to allocate system-unique device numbers. This value needs to be
> - * identical across all SoundWire bus in the system.
> + * identical across all SoundWire bus in the system. Only used if @sdw_num_alloc
> + * is not default.
>   */
>  struct sdw_bus {
>  	struct device *dev;
> @@ -922,6 +935,7 @@ struct sdw_bus {
>  	u32 bank_switch_timeout;
>  	bool multi_link;
>  	int hw_sync_min_links;
> +	enum sdw_dev_num_alloc dev_num_alloc;
>  	int dev_num_ida_min;
>  };
>  
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Vinod Koul]

On 31-05-23, 11:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> This patch adds a new Device Number allocation strategy, with the IDA
> used only for devices that are wake-capable.
> 
> "regular" devices such as amplifiers will use Device Numbers
> [1..min_ida-1].
> 
> "wake-capable" devices such as jack or microphone codecs will use
> Device Numbers [min_ida..11].
> 
> This hybrid strategy extends the number of supported devices in a
> system by only constraining the allocation if required, e.g. in the
> case of Intel LunarLake platforms the wake-capable devices are
> required to have a unique address to use the HDaudio SDI and HDAudio
> WAKEEN/WAKESTS registers.

This seems to be a consequence of Intel hardware decisions, so I guess
best suited place for this is Intel controller, do we really want to
have this in core logic?

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c       | 26 +++++++++++++++++++++-----
>  include/linux/soundwire/sdw.h |  4 ++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index e8c1c55a2a73..6f465cce8369 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -159,7 +159,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  
>  	if (slave->dev_num) { /* clear dev_num if assigned */
>  		clear_bit(slave->dev_num, bus->assigned);
> -		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
> +		    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +		     slave->prop.wake_capable))
>  			ida_free(&sdw_peripheral_ida, slave->dev_num);
>  	}
>  	list_del_init(&slave->node);
> @@ -699,17 +701,31 @@ EXPORT_SYMBOL(sdw_compare_devid);
>  /* called with bus_lock held */
>  static int sdw_get_device_num(struct sdw_slave *slave)
>  {
> +	struct sdw_bus *bus = slave->bus;
>  	int bit;
>  
> -	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
> +	if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
> +	    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +	     slave->prop.wake_capable)) {
>  		bit = ida_alloc_range(&sdw_peripheral_ida,
> -				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
> +				      bus->dev_num_ida_min, SDW_MAX_DEVICES,
>  				      GFP_KERNEL);
>  		if (bit < 0)
>  			goto err;
>  	} else {
> -		bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES);
> -		if (bit == SDW_MAX_DEVICES) {
> +		int max_devices = SDW_MAX_DEVICES;
> +
> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +		    !slave->prop.wake_capable) {
> +			max_devices = bus->dev_num_ida_min - 1;
> +
> +			/* range check */
> +			if (max_devices < 1 || max_devices > SDW_MAX_DEVICES)
> +				return -EINVAL;
> +		}
> +
> +		bit = find_first_zero_bit(bus->assigned, max_devices);
> +		if (bit == max_devices) {
>  			bit = -ENODEV;
>  			goto err;
>  		}
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 4656d6d0f3bb..8a7541ac735e 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -869,10 +869,14 @@ struct sdw_master_ops {
>   * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
>   * using range [1, 11]
>   * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
> + * @SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY: Hybrid allocation where wake-capable devices rely on
> + * IDA-based allocation and range [ida_min, 11], while regular devices rely on default
> + * allocation in range [1, ida_min - 1]
>   */
>  enum sdw_dev_num_alloc {
>  	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
>  	SDW_DEV_NUM_ALLOC_IDA,
> +	SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY,
>  };
>  
>  /**
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH 3/4] soundwire: extend parameters of new_peripheral_assigned() callback

[Vinod Koul]

On 31-05-23, 11:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The parameters are only the bus and the device number, manager ops may
> need additional details on the type of peripheral connected, such as
> whether it is wake-capable or not.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c             | 2 +-
>  drivers/soundwire/intel_auxdevice.c | 4 +++-
>  include/linux/soundwire/sdw.h       | 4 +++-
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 6f465cce8369..17b9a8bdf234 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -786,7 +786,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
>  	slave->dev_num = slave->dev_num_sticky;
>  
>  	if (bus->ops && bus->ops->new_peripheral_assigned)
> -		bus->ops->new_peripheral_assigned(bus, dev_num);
> +		bus->ops->new_peripheral_assigned(bus, slave, dev_num);
>  
>  	return 0;
>  }
> diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
> index 30f3d2ab80fd..c1df6f014e6b 100644
> --- a/drivers/soundwire/intel_auxdevice.c
> +++ b/drivers/soundwire/intel_auxdevice.c
> @@ -60,7 +60,9 @@ static int generic_post_bank_switch(struct sdw_bus *bus)
>  	return sdw->link_res->hw_ops->post_bank_switch(sdw);
>  }
>  
> -static void generic_new_peripheral_assigned(struct sdw_bus *bus, int dev_num)
> +static void generic_new_peripheral_assigned(struct sdw_bus *bus,
> +					    struct sdw_slave *slave,
> +					    int dev_num)
>  {
>  	struct sdw_cdns *cdns = bus_to_cdns(bus);
>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 8a7541ac735e..41a856bedf1e 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -861,7 +861,9 @@ struct sdw_master_ops {
>  	int (*pre_bank_switch)(struct sdw_bus *bus);
>  	int (*post_bank_switch)(struct sdw_bus *bus);
>  	u32 (*read_ping_status)(struct sdw_bus *bus);
> -	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
> +	void (*new_peripheral_assigned)(struct sdw_bus *bus,
> +					struct sdw_slave *slave,

maybe better, drop the bus and pass slave (which contains bus)

-- 
~Vinod

Re: [PATCH 3/4] soundwire: extend parameters of new_peripheral_assigned() callback

[Pierre-Louis Bossart]

>> -static void generic_new_peripheral_assigned(struct sdw_bus *bus, int dev_num)
>> +static void generic_new_peripheral_assigned(struct sdw_bus *bus,
>> +					    struct sdw_slave *slave,
>> +					    int dev_num)
>>  {
>>  	struct sdw_cdns *cdns = bus_to_cdns(bus);
>>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index 8a7541ac735e..41a856bedf1e 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -861,7 +861,9 @@ struct sdw_master_ops {
>>  	int (*pre_bank_switch)(struct sdw_bus *bus);
>>  	int (*post_bank_switch)(struct sdw_bus *bus);
>>  	u32 (*read_ping_status)(struct sdw_bus *bus);
>> -	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
>> +	void (*new_peripheral_assigned)(struct sdw_bus *bus,
>> +					struct sdw_slave *slave,
> 
> maybe better, drop the bus and pass slave (which contains bus)

I kept it for consistency, all callbacks for sdw_master_ops start with
the bus parameter.

Re: [PATCH 1/4] soundwire: add enum to control device number allocation

[Pierre-Louis Bossart]

>> +/**
>> + * enum sdw_dev_num_alloc - Device Number allocation strategies
>> + * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
>> + * using range [1, 11]
>> + * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
>> + */
>> +enum sdw_dev_num_alloc {
>> +	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
>> +	SDW_DEV_NUM_ALLOC_IDA,
> 
> Let default be IDA as 0, am sure we are not setting this field in qcom
> or amd controller, lets retain the defaults please

Not following, QCOM or AMD are NOT using the IDA-based version, so the
default is zero.

Re: [PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Pierre-Louis Bossart]

On 6/8/23 02:06, Vinod Koul wrote:
> On 31-05-23, 11:37, Bard Liao wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> This patch adds a new Device Number allocation strategy, with the IDA
>> used only for devices that are wake-capable.
>>
>> "regular" devices such as amplifiers will use Device Numbers
>> [1..min_ida-1].
>>
>> "wake-capable" devices such as jack or microphone codecs will use
>> Device Numbers [min_ida..11].
>>
>> This hybrid strategy extends the number of supported devices in a
>> system by only constraining the allocation if required, e.g. in the
>> case of Intel LunarLake platforms the wake-capable devices are
>> required to have a unique address to use the HDaudio SDI and HDAudio
>> WAKEEN/WAKESTS registers.
> 
> This seems to be a consequence of Intel hardware decisions, so I guess
> best suited place for this is Intel controller, do we really want to
> have this in core logic?

It's a valid objection.

The reason why I added the alternate strategies in the core logic is
that the IDA and hybrid approach are just software-based with no
specific hardware dependencies. If QCOM or AMD wanted to use the
strategies contributed and tested by Intel, it'd be a two-line change on
their side.

That said, it's likely that at some point *someone* will want to
constrain the device number allocation further, be it with ACPI/DT
properties or reading hardware registers. The device number is a
de-facto priority given the way we scan the PING frames, so some systems
may want to give a higher priority to a specific peripherals.

This would push us to add a master ops callback to control the device
number allocation. It's a bit invasive but that would give the ultimate
flexibility. Reuse between vendors could be possible if 'generic'
callbacks were part of a library to pick from.

I don't really have any objections if this vendor-specific callback was
preferred, it may be a bit early to add this but long-term it's probably
what makes more sense.

I'll go with the flow on suggested recommendations.

>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Rander Wang <rander.wang@intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> ---
>>  drivers/soundwire/bus.c       | 26 +++++++++++++++++++++-----
>>  include/linux/soundwire/sdw.h |  4 ++++
>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index e8c1c55a2a73..6f465cce8369 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -159,7 +159,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
>>  
>>  	if (slave->dev_num) { /* clear dev_num if assigned */
>>  		clear_bit(slave->dev_num, bus->assigned);
>> -		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
>> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
>> +		    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
>> +		     slave->prop.wake_capable))
>>  			ida_free(&sdw_peripheral_ida, slave->dev_num);
>>  	}
>>  	list_del_init(&slave->node);
>> @@ -699,17 +701,31 @@ EXPORT_SYMBOL(sdw_compare_devid);
>>  /* called with bus_lock held */
>>  static int sdw_get_device_num(struct sdw_slave *slave)
>>  {
>> +	struct sdw_bus *bus = slave->bus;
>>  	int bit;
>>  
>> -	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
>> +	if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
>> +	    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
>> +	     slave->prop.wake_capable)) {
>>  		bit = ida_alloc_range(&sdw_peripheral_ida,
>> -				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
>> +				      bus->dev_num_ida_min, SDW_MAX_DEVICES,
>>  				      GFP_KERNEL);
>>  		if (bit < 0)
>>  			goto err;
>>  	} else {
>> -		bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES);
>> -		if (bit == SDW_MAX_DEVICES) {
>> +		int max_devices = SDW_MAX_DEVICES;
>> +
>> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
>> +		    !slave->prop.wake_capable) {
>> +			max_devices = bus->dev_num_ida_min - 1;
>> +
>> +			/* range check */
>> +			if (max_devices < 1 || max_devices > SDW_MAX_DEVICES)
>> +				return -EINVAL;
>> +		}
>> +
>> +		bit = find_first_zero_bit(bus->assigned, max_devices);
>> +		if (bit == max_devices) {
>>  			bit = -ENODEV;
>>  			goto err;
>>  		}
>> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
>> index 4656d6d0f3bb..8a7541ac735e 100644
>> --- a/include/linux/soundwire/sdw.h
>> +++ b/include/linux/soundwire/sdw.h
>> @@ -869,10 +869,14 @@ struct sdw_master_ops {
>>   * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
>>   * using range [1, 11]
>>   * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
>> + * @SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY: Hybrid allocation where wake-capable devices rely on
>> + * IDA-based allocation and range [ida_min, 11], while regular devices rely on default
>> + * allocation in range [1, ida_min - 1]
>>   */
>>  enum sdw_dev_num_alloc {
>>  	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
>>  	SDW_DEV_NUM_ALLOC_IDA,
>> +	SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY,
>>  };
>>  
>>  /**
>> -- 
>> 2.25.1
> 

Re: [PATCH 3/4] soundwire: extend parameters of new_peripheral_assigned() callback

[Vinod Koul]

On 08-06-23, 08:24, Pierre-Louis Bossart wrote:
> 
> >> -static void generic_new_peripheral_assigned(struct sdw_bus *bus, int dev_num)
> >> +static void generic_new_peripheral_assigned(struct sdw_bus *bus,
> >> +					    struct sdw_slave *slave,
> >> +					    int dev_num)
> >>  {
> >>  	struct sdw_cdns *cdns = bus_to_cdns(bus);
> >>  	struct sdw_intel *sdw = cdns_to_intel(cdns);
> >> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> >> index 8a7541ac735e..41a856bedf1e 100644
> >> --- a/include/linux/soundwire/sdw.h
> >> +++ b/include/linux/soundwire/sdw.h
> >> @@ -861,7 +861,9 @@ struct sdw_master_ops {
> >>  	int (*pre_bank_switch)(struct sdw_bus *bus);
> >>  	int (*post_bank_switch)(struct sdw_bus *bus);
> >>  	u32 (*read_ping_status)(struct sdw_bus *bus);
> >> -	void (*new_peripheral_assigned)(struct sdw_bus *bus, int dev_num);
> >> +	void (*new_peripheral_assigned)(struct sdw_bus *bus,
> >> +					struct sdw_slave *slave,
> > 
> > maybe better, drop the bus and pass slave (which contains bus)
> 
> I kept it for consistency, all callbacks for sdw_master_ops start with
> the bus parameter.

That is a valid point, since this is sdw_master_ops() and not slave,
this looks better

-- 
~Vinod

Re: [PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Vinod Koul]

On 08-06-23, 10:09, Pierre-Louis Bossart wrote:
> 
> 
> On 6/8/23 02:06, Vinod Koul wrote:
> > On 31-05-23, 11:37, Bard Liao wrote:
> >> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>
> >> This patch adds a new Device Number allocation strategy, with the IDA
> >> used only for devices that are wake-capable.
> >>
> >> "regular" devices such as amplifiers will use Device Numbers
> >> [1..min_ida-1].
> >>
> >> "wake-capable" devices such as jack or microphone codecs will use
> >> Device Numbers [min_ida..11].
> >>
> >> This hybrid strategy extends the number of supported devices in a
> >> system by only constraining the allocation if required, e.g. in the
> >> case of Intel LunarLake platforms the wake-capable devices are
> >> required to have a unique address to use the HDaudio SDI and HDAudio
> >> WAKEEN/WAKESTS registers.
> > 
> > This seems to be a consequence of Intel hardware decisions, so I guess
> > best suited place for this is Intel controller, do we really want to
> > have this in core logic?
> 
> It's a valid objection.
> 
> The reason why I added the alternate strategies in the core logic is
> that the IDA and hybrid approach are just software-based with no
> specific hardware dependencies. If QCOM or AMD wanted to use the
> strategies contributed and tested by Intel, it'd be a two-line change on
> their side.
> 
> That said, it's likely that at some point *someone* will want to
> constrain the device number allocation further, be it with ACPI/DT
> properties or reading hardware registers. The device number is a
> de-facto priority given the way we scan the PING frames, so some systems
> may want to give a higher priority to a specific peripherals.
> 
> This would push us to add a master ops callback to control the device
> number allocation. It's a bit invasive but that would give the ultimate
> flexibility. Reuse between vendors could be possible if 'generic'
> callbacks were part of a library to pick from.
> 
> I don't really have any objections if this vendor-specific callback was
> preferred, it may be a bit early to add this but long-term it's probably
> what makes more sense.
> 
> I'll go with the flow on suggested recommendations.

Thanks, if it all one of the other two controller start using this, it
would make sense to move it to core then, for now would be better to
have this in specific driver

-- 
~Vinod

Re: [PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

[Pierre-Louis Bossart]

>>> This seems to be a consequence of Intel hardware decisions, so I guess
>>> best suited place for this is Intel controller, do we really want to
>>> have this in core logic?
>>
>> It's a valid objection.
>>
>> The reason why I added the alternate strategies in the core logic is
>> that the IDA and hybrid approach are just software-based with no
>> specific hardware dependencies. If QCOM or AMD wanted to use the
>> strategies contributed and tested by Intel, it'd be a two-line change on
>> their side.
>>
>> That said, it's likely that at some point *someone* will want to
>> constrain the device number allocation further, be it with ACPI/DT
>> properties or reading hardware registers. The device number is a
>> de-facto priority given the way we scan the PING frames, so some systems
>> may want to give a higher priority to a specific peripherals.
>>
>> This would push us to add a master ops callback to control the device
>> number allocation. It's a bit invasive but that would give the ultimate
>> flexibility. Reuse between vendors could be possible if 'generic'
>> callbacks were part of a library to pick from.
>>
>> I don't really have any objections if this vendor-specific callback was
>> preferred, it may be a bit early to add this but long-term it's probably
>> what makes more sense.
>>
>> I'll go with the flow on suggested recommendations.
> 
> Thanks, if it all one of the other two controller start using this, it
> would make sense to move it to core then, for now would be better to
> have this in specific driver

The code is much cleaner indeed that way.

I still have to work on a race condition if the codec driver probe
happens *after* the enumeration. In that case, the properties needed to
decide which allocation to use are not initialized yet.

We may need to either force the codec to re-enumerate with a ForceReset,
or to switch the device number. In theory the latter is straightforward
but there can be additional races if there are interrupts thrown just
before the device number change happens.