Re: [EXTERNAL] Re: [PATCH v1 RESEND] SoundWire: Allow Prepare command for Simplified_CP_SM

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

>>> Are you referring to the DPN_IntStat::PortReady bit? IOW do the errors during
>>> prepare happen because the interrupt is not generated?
>>> If that is the case, then you should be aware of the thread "soundwire:
>>> stream: Prepare ports in parallel to reduce stream start latency", where it was
>>> agreed that the use of the PortReady interrupt is broken. We do need to move
>>> to a different code where we have a polling loop on the
>>> DPN_PrepareStatus::NotFinished bits.
>>
>> Thanks for the info.
>>
>>> So is the problem in the interrupt generation, or is the problem with the 'Not
>>> Finished' bits?
>> Both, while using Full CP_SM on a simplified CP_SM device:
>> * First problem: the interrupt will not get generated on a simple CP_SM device.
>> * Second problem: even after timeout waiting for interrupt with huge delays,
>>    we can't rely on 'Not Finished' bits, causing timeout errors.
>>    In the current code for full CP_SM, 'Not Finished' = 0 is a good response even
>>    with timeout.
>>
> That's because it _IS_ a good response. If the part is reporting
> NotFinished=0, then it's good to go and we don't need to treat it as a
> timeout. The timeout is caused by the deadlock that Pierre mentioned.
> Full CP_SM will always timeout because that code is holding the bus
> lock, which blocks the interrupt handler.
> 
> Ignoring the timeout if NotFinished=0 was a workaround until I found
> time to fix the interrupt handling. Otherwise full CP_SM would never
> work.
> 
> We've now decided to replace that code with a polling loop instead of
> using interrupts.

IOW the suggestion would be to use Richard's patch and force the drive to use a regular state machine.
see https://github.com/thesofproject/linux/pull/5649

>>> If that works then an alternate solution to this patch would be to quirk all TI
>>> devices to use the regular state machine, and use the polling loop as
>>> suggested by Richard Fitzgerald.
>>> I think it's worth exploring what happens if the PortReady interrupt is not
>>> used. If that's broken as well, then your solution would work but needs
>>> more/better comments indeed.
>> Currently, we have one device which needs this workaround.
>> Initially, it looked like a generic solution to me. However, if it makes sense,
>> I can implement sdw_slave_ops->port_prep to make custom changes
>> for the required devices only, handle any errors and drop this patch.
>> Please suggest.

But if you want to keep following the CP_SM state machine, then you could indeed add the required write in a port_prep() routine.
That'd fine as well, you'd be relying on what the core SoundWire stream management provides - namely a means to do device-specific stuff before/after port setup.