ASoC: rt1320: set wake_capable = 0 explicitly by bardliao · Pull Request #5337 · thesofproject/linux

bardliao · 2025-02-25T08:57:37Z

BIOS may set wake_capable = 1 for Windows. But it should be 0 in Linux as we don't use the wake feature at all in Linux.

ranj063 · 2025-02-25T19:44:47Z

sound/soc/codecs/rt1320-sdw.c

 	/* set the timeout values */
 	prop->clk_stop_timeout = 64;

+	/* BIOS may set wake_capable for Windows. But wake_capable is not needed for Linux */


What issue does this fix @bardliao

We meet below error when test with rt1320

soundwire sdw-master-0-2: generic_new_peripheral_assigned: invalid dev_num 1, wake supported 1

It is because that the prop->wake_capable is true, but the codec is not in wake_capable_list[].

It is very likely that "mipi-sdw-wake-up-unavailable" is not set or is set to false in the BIOS. As a result, prop->wake_capable will be true when codec driver read the property.

I checked with Realtek and got the rt1320 doesn't need the wake feature in Linux,
So, set prop->wake_capable = 0 in codec driver makes more sense.

Ok great, can you add that in the commit message that this is what the change fixes?

Commit message updated

but if the rt1320 is capable of wake, then should be in the wake_capable_list[] ?

@bardliao, but how we get to the point of that error in the first place?
The id is assigned via intel_get_device_num_ida() and that should be assigning in the higher range:
https://github.com/thesofproject/linux/blob/topic/sof-dev/drivers/soundwire/intel_auxdevice.c#L261

so in generic_new_peripheral_assigned() the ID should be in the correct range, unless the slave->dev_num_sticky is not 0.

I don't really follow this.

@ujfalusi The issue is that we assign device number when a Peripheral is attached. However, it may happen before read_prop() is called. I.e. There is no guarantee that we will get Peripheral properties before it is attached. At that moment, slave->prop.wake_capable will be 0. And generic_new_peripheral_assigned() will assign a dev_num start with 1.
That's why we add wake_capable_list[] to ensure the dev_num in assigned in the correct range.
The Peripheral will be re-attached when we start a stream, and it will get slave->prop.wake_capable already at this moment. That's why we see the generic_new_peripheral_assigned: invalid dev_num 1, wake supported 1 error.
See e66f91a for the details.

right, so why we don't grow that list with this codec? What is the harm? I cannot relay compute the "rt1320 doesn't need the wake feature in Linux" statement.
Why Linux does not need this? It implies that other OS needs it? What is the harm of marking a codec which can generate wake that it can generate wake even if it will not do that?
In a system will we run out of these IDs? Are OEMs cramp only wake capable devices that we run out of IDs (between 6-11)?

Or is it so that the rt1320 is not capable of wake at all and wrongly marked as such?

right, so why we don't grow that list with this codec? What is the harm?

Because the system-unique device numbers are limited. We can't waste the device number on a codec that is not wake-able. rt1320 is an amp. In theory, we could have up to 4 rt1320 amps (8 speakers) in a device.

I cannot relay compute the "rt1320 doesn't need the wake feature in Linux" statement.

The wake events of rt1320 are power control and ultrasound detection which are disabled in Linux.

Why Linux does not need this? It implies that other OS needs it?

I am not sure about other OS, but not a single OEM ask for the feature in Linux.

What is the harm of marking a codec which can generate wake that it can generate wake even if it will not do that?
In a system will we run out of these IDs? Are OEMs cramp only wake capable devices that we run out of IDs (between 6-11)?

Yes, it is possible we run out of these IDs. We will need to redesign the ID assign mechanism if the IDs are run out of.

Or is it so that the rt1320 is not capable of wake at all and wrongly marked as such?

In chip's point of view, it can support wake events. But I believe that other amps support wake events, too. We didn't add those amps to wake_capable_list[] because it is not needed. Currently, we ID 1 - 5 are reserved for amps (non-wake-able), and ID 6 - 11 are for wake-able codecs. So that we can have up to 5 amps on each SDW bus, and up to 6 wake-able jack/mic codecs. It is sufficient for current audio designs. However, if some amps want to use the 6 - 11 IDs, it is likely that we will run out of the IDs in some audio design especially if the wake-able amp can driver only 1 speaker.

ranj063 · 2025-02-27T15:47:12Z

sound/soc/codecs/rt1320-sdw.c

 	/* set the timeout values */
 	prop->clk_stop_timeout = 64;

+	/* BIOS may set wake_capable for Windows. But wake_capable is not needed for Linux */


@bardliao I dont know if we should mention Windows at all. Can we simply state that the BIOS may set it as wake-capable and leave it at that?

@bardliao , I agree, please remove the reference to Windows, it is not relevant

@ranj063 @ujfalusi done

@bardliao its still there in the comment in the code?

oops. Fixed, too.

"generic_new_peripheral_assigned: invalid dev_num 1, wake supported 1" is reported by our internal CI test. Rt1320's wake feature is not used in Linux and that's why it is not in the wake_capable_list[] list in intel_auxdevice.c. However, BIOS may set it as wake-capable. Overwrite wake_capable to 0 in the codec driver to align with wake_capable_list[]. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ujfalusi · 2025-03-03T07:25:50Z

sound/soc/codecs/rt1320-sdw.c

 	prop->clk_stop_timeout = 64;

+	/* BIOS may set wake_capable. Make sure it is 0 as wake events are disabled. */
+	prop->wake_capable = 0;


the wake_capable_list[] is intel specific (drivers/soundwire/intel_auxdevice.c), the codec driver is not.
I'm not sure how other OSs can handle if the rt1320 is marked by BIOS as wake capable and why we cannot.

The rt1320 can be used on devices sporting other vendor's SoC..

shumingfan · 2025-03-03T10:29:23Z

The rt1320 driver in Linux doesn't enable the wake up source for now.
Hence, the wake_capable can be zero.
Acked-by: shumingf@realtek.com

bardliao requested review from ranj063, shumingfan and ujfalusi February 25, 2025 08:57

ranj063 reviewed Feb 25, 2025

View reviewed changes

bardliao force-pushed the rt1320-wake_capable branch from dd0ce5f to 1f51113 Compare February 26, 2025 05:26

ujfalusi previously approved these changes Feb 27, 2025

View reviewed changes

ranj063 reviewed Feb 27, 2025

View reviewed changes

ujfalusi self-requested a review February 28, 2025 09:25

bardliao force-pushed the rt1320-wake_capable branch from 1f51113 to 68a0c79 Compare March 3, 2025 01:17

bardliao dismissed ujfalusi’s stale review via d49e9f6 March 3, 2025 03:23

bardliao force-pushed the rt1320-wake_capable branch from 68a0c79 to d49e9f6 Compare March 3, 2025 03:23

ujfalusi reviewed Mar 3, 2025

View reviewed changes

shumingfan approved these changes Mar 3, 2025

View reviewed changes

ujfalusi approved these changes Mar 3, 2025

View reviewed changes

ranj063 approved these changes Mar 3, 2025

View reviewed changes

ranj063 merged commit 969fe71 into thesofproject:topic/sof-dev Mar 3, 2025
10 of 15 checks passed

Conversation

bardliao commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shumingfan commented Mar 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants