thesofproject
SoundWire: Call Prepare command for Simplified_CP_SM
As defined in the MIPI SoundWire specification v1-2 for Simplified Channel Prepare State Machine (Simplified_CP_SM)
- Figure 114 for the Simplified_CP_SM in the specification shows the "Ready" state (NF=0, P=1) that can be reached via "Prepare0 OR Prepare1" transitions. The diagram itself show "Prepare0 OR Prepare1" would be provided to the device.
- Table 115 (Stimulus to the Channel Prepare State Machine) in the specification clarifies that both Prepare0 and Prepare1 are read-only/"write-ignored" bits for Simplified_CP_SM.
So, the “write-ignored” in informative section indicate it would be device’s choice to ignore. If device is not making any use of Prepare0/Prepare1 input from Host controller it can ignore. Given in TI device implementation we make full use of Prepare0/Prepare1, the prepare command should be sent.
The fix ensures compliance with the specification while maintaining proper functionality of dataport operations as per the spec. The commit ensures that even when using "simplified-channelprepare-sm", the channel prepare ("DPN_PrepareCtrl") function is properly called as mandated by the specification.
Can one of the admins verify this patch?
reply test this please to run this test once
The change looks good to me. However, there is a typo in the commit message. The Channel Prepare State Machine in the spec is Figure 141 not 114 What do you think? @shumingfan @charleskeepax
Yeah we should probably test this, seems a bit weird to be writing the prepares for a simple device.
If the device requires Prepare, it should not be Simplified_CP_SM. Why don't you report your peripheral as Full CP_SM, and then Prepare=1 will be written?
The problem I see with this pull request is that it would be valid to remove this code, according to the SoundWire specification.
I think it at least needs a comment in the code to say that writing Prepare=1 on Simplified_CP_SM is a workaround for some peripherals. So someone changing the code in future knows that it is a workaround that must not be removed.
Hi @rfvirgil Thanks for the review.
If the device requires Prepare, it should not be Simplified_CP_SM. Why don't you report your peripheral as Full CP_SM, and then Prepare=1 will be written?
The problem I see with this pull request is that it would be valid to remove this code, according to the SoundWire specification.
I'd like to clarify the implementation based on the MIPI SoundWire specification v1-2 from TI's interpretation. According to Figure 141 in the specification, even in Simplified_CP_SM, the "Ready" state (NF=0, P=1) must be reached via "Prepare0 OR Prepare1" transitions. While Table 115 notes that Prepare0/Prepare1 are "Read-only / write-ignored" for Simplified_CP_SM, this doesn't mean the command shouldn't be sent per our interpretation - it indicates that devices may ignore the specific value written. The "write-ignored" designation means the peripheral can choose how to handle the value, but the host still needs to send the Prepare command to trigger the state transition.
In our TI device implementation, we utilize the Prepare0/Prepare1 input.
Please correct me if there is some misunderstanding here.
I think it at least needs a comment in the code to say that writing Prepare=1 on Simplified_CP_SM is a workaround for some peripherals. So someone changing the code in future knows that it is a workaround that must not be removed.
I'll add a clear comment explaining that this implementation follows the specification requirement for state transitions in Simplified_CP_SM, noting that while the values may be "write-ignored" by some peripherals, the command itself is still necessary for proper state machine operation.
While Table 115 notes that Prepare0/Prepare1 are "Read-only / write-ignored" for Simplified_CP_SM, this doesn't mean the command shouldn't be sent per our interpretation - it indicates that devices may ignore the specific value written.
I think this is the bit Richard is somewhat skeptical about. It seems very reasonable to interpret the "Read-only / write-ignored" as the host may write the prepare bits, making your change allowed by the specification I think. It seems very hard, however, to interpret that as the host shall write the prepare bits, which appears to be how your commit message is worded.
Would be good to understand why the device can't just use the full prepare state machine as Richard asks, if it needs the prepare signals that would seem the obvious fix. I guess it doesn't update the NF flag properly?
From the Cirrus side, testing this with our hardware it seems fine, but would be good to hear back from Realtek as well, since it is quite a scary change.
Would be good to understand why the device can't just use the full prepare state machine as Richard asks, if it needs the prepare signals that would seem the obvious fix. I guess it doesn't update the NF flag properly?
We have a complex situation as the same ACPI table is used between Windows and Linux OSes. Based on our previous experiment, for a "Simplified CP_SM" device, Windows driver expects the "Alert" message for port prepare status which is absent in case devices implementing "Simplified CP_SM" and hence we saw one of the functionality not working when the ACPI table is configured as "Full CP_SM"
Hmm... yes it is a requirement that an IRQ is raised on the prep/deprep, so I guess that does force the use of the simplified state machine. Well as noted the change doesn't affect our devices so its ok with us, would still be good to hear back from Realtek on if they are ok with this, as I think they might use the simple process more than we do.
Hmm... yes it is a requirement that an IRQ is raised on the prep/deprep, so I guess that does force the use of the simplified state machine. Well as noted the change doesn't affect our devices so its ok with us, would still be good to hear back from Realtek on if they are ok with this, as I think they might use the simple process more than we do.
I tested this PR with some codecs. It seems fine.
Thank you @bardliao @shumingfan @charleskeepax, @rfvirgil for review/testing. I have [1] updated the commit message. [2] Added a comment in the code as suggested.
Hello All, I have tested this patch with ALC712-VB codec. I have observed Channel prepare timeout results in port prepare failures during headphone playback audio use case. If i revert this patch, no port prepare timeouts are observed.
ch_prep_timeout_2212_logs_rtk.txt Attached logs. @shumingfan : Could you please help to comment?
I referred old patch which is part of patch series shared by TI team, where below changes are missing .
-
} else { -
/* Some device return error for the prepare command, -
* ignore the error for Simplified CP_SM -
*/ -
ret = 0;
}
Will retest with updated patch . @shumingfan : Thanks for pointing out.