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@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> Reviewed-by: Rander Wang <rander.wang@xxxxxxxxx>
>> Signed-off-by: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
>> ---
>>  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
>