diff mbox series

[2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY

Message ID 20230531033736.792464-3-yung-chuan.liao@linux.intel.com
State New
Headers show
Series soundwire: allow for more than 8 devices, keep IDA for wake-capable devices | expand

Commit Message

Liao, Bard May 31, 2023, 3:37 a.m. UTC 
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(-)

Comments

Vinod Koul June 8, 2023, 7:06 a.m. UTC | #1 
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
Pierre-Louis Bossart June 8, 2023, 3:09 p.m. UTC | #2 
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
>
diff mbox series

Patch

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,
 };
 
 /**