Re: [PATCH 1/2] soundwire: bus: Don't filter slave alerts

[Pierre-Louis Bossart]

On 1/20/23 04:14, Charles Keepax wrote:
> On Thu, Jan 19, 2023 at 11:27:14AM -0600, Pierre-Louis Bossart wrote:
>> On 1/19/23 10:51, Charles Keepax wrote:
>>> Currently the SoundWire core will loop handling slave alerts but it will
>>> only handle those present when the alert was first raised. This causes
>>> some issues with the Cadence SoundWire IP, which only generates an IRQ
>>> when alert changes state. This means that if a new alert arrives whilst
>>> old alerts are being handled it will not be handled in the currently
>>> loop and then no further alerts will be processed since alert never
>>> changes state to trigger a new IRQ.
>>>
>>> Correct this issue by allowing the core to handle all pending alerts in
>>> the IRQ handling loop. The code will still only loop up to
>>> SDW_READ_INTR_CLEAR_RETRY times, so it shouldn't be possible for it get
>>> completely stuck and if you are generating IRQs faster than you can
>>> handle them you likely have bigger problems anyway.
>>
>> The change makes sense, but it's a bit odd to change the way the
>> interrupts are handled because of a specific design. The bus should be
>> able to deal with various designs, not force a one-size-fits-all policy
>> that may not be quite right in all cases.
>>
>> Could we have a new flag at the bus level that says that peripheral
>> interrupts are not filtered, and set if for the Intel case?
>>
>> We could similarly make the SDW_READ_INTR_CLEAR_RETRY constant
>> bus/platform specific. The SoundWire spec mandates that we re-read the
>> status after clearing the interrupt, but it doesn't say how to deal with
>> recurring interrupts.
> 
> Perhaps I should have phrased the commit message differently
> here. To be honest I am not really convince the old code makes
> a huge amount of sense. So I would prefer not to add a flag
> enabling the weird behaviour.
> 
> I would be of the opinion that there are really two options
> for IRQ handling code like this that make sense:
> 
> 1) Loop until the IRQs are handled, ie. it is the soundwire
> core's responsibility to make sure all the IRQs are handled
> before moving on.
> 
> 2) Just handle the IRQs available when the function is called,
> ie. it is the drivers responsibility to keep calling the core
> until the IRQs are handled.
> 
> That way there is a clearly defined who that is responsible.
> The old code is a weird mix of the two where most of the time
> it is the soundwire core's responsibly to handle recurring
> IRQs unless a new one happens in which case it is the drivers
> responsibilty to recall the core.
> 
> Also the new code will still work for drivers that have level
> IRQs and recall the core, without any modification of those
> drivers. So I don't see what anyone would be gaining from the
> old system.

I think the intent of the 'old code' was the option 2), expect that it's
broken on Intel platforms and not possible because of the hardware design.

I am good with your two suggested options.

> Regarding making the clear retries platform specific that makes
> sense to me but is clearly a separate patch. I will add it onto
> my soundwire todo list.

yes, it's a separate patch indeed.