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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
On 31-05-23, 11:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
On 6/8/23 02:06, Vinod Koul wrote:
> On 31-05-23, 11:37, Bard Liao wrote:
>> From: Pierre-Louis Bossart <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Rander Wang <[email protected]>
>> Signed-off-by: Bard Liao <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
> >>
> >> 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
>>> 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.