[PATCH 0/3] soundwire: allocate device_number with IDA

[PATCH 0/3] soundwire: allocate device_number with IDA

[Bard Liao]

The device_number is currently allocated in the scope of each bus and does
not need to be unique at the system level.
This leads e.g. on Dell devices with three or four Device1 on different
bus segments. To make the device_number unique at the system level, and
unified with the HDaudio/iDISP SDI values, this series allocates the
dev_number with an IDA restricted between 4 and 11 (inclusive).

Pierre-Louis Bossart (3):
  soundwire: bus: rename sdw_ida as sdw_bus_ida
  soundwire: bus: allow device number to be unique at system level
  soundwire: intel: set dev_num_ida_min

 drivers/soundwire/bus.c       | 29 ++++++++++++++++++++---------
 drivers/soundwire/intel.c     |  4 ++++
 include/linux/soundwire/sdw.h |  4 ++++
 3 files changed, 28 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/3] soundwire: bus: rename sdw_ida as sdw_bus_ida

[Bard Liao]

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

To avoid confusions with follow-up patches using a IDA mechanism for
peripheral 'device number' allocation, rename sdw_ida as sdw_bus_ida.

Pure rename, no functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 8d4000664fa3..37638c20c804 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -11,11 +11,11 @@
 #include "bus.h"
 #include "sysfs_local.h"
 
-static DEFINE_IDA(sdw_ida);
+static DEFINE_IDA(sdw_bus_ida);
 
 static int sdw_get_id(struct sdw_bus *bus)
 {
-	int rc = ida_alloc(&sdw_ida, GFP_KERNEL);
+	int rc = ida_alloc(&sdw_bus_ida, GFP_KERNEL);
 
 	if (rc < 0)
 		return rc;
@@ -179,7 +179,7 @@ void sdw_bus_master_delete(struct sdw_bus *bus)
 	sdw_master_device_del(bus);
 
 	sdw_bus_debugfs_exit(bus);
-	ida_free(&sdw_ida, bus->id);
+	ida_free(&sdw_bus_ida, bus->id);
 }
 EXPORT_SYMBOL(sdw_bus_master_delete);
 
-- 
2.25.1

[PATCH 2/3] soundwire: bus: allow device number to be unique at system level

[Bard Liao]

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

The SoundWire specification allows the device number to be allocated
at will. When a system includes multiple SoundWire links, the device
number scope is limited to the link to which the device is attached.

However, for integration/debug it can be convenient to have a unique
device number across the system. This patch adds a 'dev_num_ida_min'
field at the bus level, which when set will be used to allocate an
IDA.

The allocation happens when a hardware device reports as ATTACHED. If
any error happens during the enumeration, the allocated IDA is not
freed - the device number will be reused if/when the device re-joins
the bus. The IDA is only freed when the Linux device is unregistered.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@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       | 23 +++++++++++++++++------
 include/linux/soundwire/sdw.h |  4 ++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 37638c20c804..8970f8560766 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -12,6 +12,7 @@
 #include "sysfs_local.h"
 
 static DEFINE_IDA(sdw_bus_ida);
+static DEFINE_IDA(sdw_peripheral_ida);
 
 static int sdw_get_id(struct sdw_bus *bus)
 {
@@ -157,9 +158,11 @@ static int sdw_delete_slave(struct device *dev, void *data)
 
 	mutex_lock(&bus->bus_lock);
 
-	if (slave->dev_num) /* clear dev_num if assigned */
+	if (slave->dev_num) { /* clear dev_num if assigned */
 		clear_bit(slave->dev_num, bus->assigned);
-
+		if (bus->dev_num_ida_min)
+			ida_free(&sdw_peripheral_ida, slave->dev_num);
+	}
 	list_del_init(&slave->node);
 	mutex_unlock(&bus->bus_lock);
 
@@ -639,10 +642,18 @@ static int sdw_get_device_num(struct sdw_slave *slave)
 {
 	int bit;
 
-	bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES);
-	if (bit == SDW_MAX_DEVICES) {
-		bit = -ENODEV;
-		goto err;
+	if (slave->bus->dev_num_ida_min) {
+		bit = ida_alloc_range(&sdw_peripheral_ida,
+				      slave->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) {
+			bit = -ENODEV;
+			goto err;
+		}
 	}
 
 	/*
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 39058c841469..a2b31d25ea27 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -889,6 +889,9 @@ 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_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.
  */
 struct sdw_bus {
 	struct device *dev;
@@ -913,6 +916,7 @@ struct sdw_bus {
 	u32 bank_switch_timeout;
 	bool multi_link;
 	int hw_sync_min_links;
+	int dev_num_ida_min;
 };
 
 int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
-- 
2.25.1

[PATCH 3/3] soundwire: intel: set dev_num_ida_min

[Bard Liao]

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

The allowed values for SoundWire device numbers are between 1 and 11
(inclusive). HDaudio/iDISP codecs typically use SDI values 0..3
(inclusive). To allow for a unique peripheral SDI/dev_number across
HDaudio and SoundWire buses, we set the minimum base to 4. This still
allows for 8 SoundWire peripherals in the system, currently more than
needed in actual products.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@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.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 89d1d0d021fc..3e372599a8c3 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -22,6 +22,9 @@
 #include "bus.h"
 #include "intel.h"
 
+/* IDA min selected to avoid conflicts with HDaudio/iDISP SDI values */
+#define INTEL_DEV_NUM_IDA_MIN           4
+
 #define INTEL_MASTER_SUSPEND_DELAY_MS	3000
 #define INTEL_MASTER_RESET_ITERATIONS	10
 
@@ -1307,6 +1310,7 @@ static int intel_link_probe(struct auxiliary_device *auxdev,
 	cdns->msg_count = 0;
 
 	bus->link_id = auxdev->id;
+	bus->dev_num_ida_min = INTEL_DEV_NUM_IDA_MIN;
 
 	sdw_cdns_probe(cdns);
 
-- 
2.25.1

Re: [PATCH 0/3] soundwire: allocate device_number with IDA

[Vinod Koul]

On 23-08-22, 12:50, Bard Liao wrote:
> The device_number is currently allocated in the scope of each bus and does
> not need to be unique at the system level.
> This leads e.g. on Dell devices with three or four Device1 on different
> bus segments. 

And how is that an issue that should be fixed?

> To make the device_number unique at the system level, and

Why should we do that...
> unified with the HDaudio/iDISP SDI values, this series allocates the
> dev_number with an IDA restricted between 4 and 11 (inclusive).

Does this not place an artificial restriction that a system can have
only 12 devices if we have unique device

Perhaps a better way would be to use dus:dev combination while dealing
with a device always..??

-- 
~Vinod

Re: [PATCH 0/3] soundwire: allocate device_number with IDA

[Pierre-Louis Bossart]

Hi Vinod,

>> The device_number is currently allocated in the scope of each bus and does
>> not need to be unique at the system level.
>> This leads e.g. on Dell devices with three or four Device1 on different
>> bus segments. 
> 
> And how is that an issue that should be fixed?

1. this makes things simpler for debug. You can look at the logs and see
what the device is without having to look at the link. It's been much
easier to track complicated issues when each device is tracked with a
unique system level number.

2. Intel hardware has a requirement that the device number be unique for
all links managed by  a given controller. This patch prepares that
transition. I cannot disclose more details at the moment, you will have
to trust me on this one.

>> To make the device_number unique at the system level, and
> 
> Why should we do that...
>> unified with the HDaudio/iDISP SDI values, this series allocates the
>> dev_number with an IDA restricted between 4 and 11 (inclusive).
> 
> Does this not place an artificial restriction that a system can have
> only 12 devices if we have unique device

It does indeed, but it simplifies debug and it allows future hardware to
be supported.

In practice, the most we've seen on Intel platforms is 4 devices at the
system level. Even if the SoundWire spec does allow for 11 devices per
link, it's way over-engineered due to capacitive load and signal
integrity issues. We've seen an absolute maximum of 4 devices on a
single link in test rigs, and that was never deployed. Two devices per
link were also only used in prototypes which never hit the market.

At any rate, this in an opt-in solution, not a requirement for non-Intel
platforms at all. Others can keep using as many devices as they wish,
within the bounds allowed by the standard.

Note that the device_number creates a de-facto priority, and it would be
perfectly acceptable for some platforms to tweak the current first-come
first-serve allocation to improve interrupt response time on each link,
etc. The standard says nothing about how the device numbers should be
allocated, only that they be unique on each link. In other words, you
should expect additional changes to the existing way of allocating
device numbers.

> Perhaps a better way would be to use dus:dev combination while dealing
> with a device always..??

That is not compatible with Intel hardware requirements. This wasn't a
decision where Bard and I were consulted, it is what it is.

Does this clarify things?
Thanks
-Pierre

Re: [PATCH 0/3] soundwire: allocate device_number with IDA

[Vinod Koul]

On 23-08-22, 12:50, Bard Liao wrote:
> The device_number is currently allocated in the scope of each bus and does
> not need to be unique at the system level.
> This leads e.g. on Dell devices with three or four Device1 on different
> bus segments. To make the device_number unique at the system level, and
> unified with the HDaudio/iDISP SDI values, this series allocates the
> dev_number with an IDA restricted between 4 and 11 (inclusive).

Applied, thanks

-- 
~Vinod

[PATCH 0/3] soundwire: allocate device_number with IDA

[Bard Liao]

The device_number is currently allocated in the scope of each bus and does
not need to be unique at the system level.
This leads e.g. on Dell devices with three or four Device1 on different
bus segments. To make the device_number unique at the system level, and
unified with the HDaudio/iDISP SDI values, this PRs allocates the
dev_number with an IDA restricted between 4 and 11 (inclusive).

Pierre-Louis Bossart (3):
  soundwire: bus: rename sdw_ida as sdw_bus_ida
  soundwire: bus: allow device number to be unique at system level
  soundwire: intel: set dev_num_ida_min

 drivers/soundwire/bus.c       | 29 ++++++++++++++++++++---------
 drivers/soundwire/intel.c     |  4 ++++
 include/linux/soundwire/sdw.h |  4 ++++
 3 files changed, 28 insertions(+), 9 deletions(-)

-- 
2.25.1