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
>
