Re: [PATCH 1/4] spi: spi-fsl-dspi: Clear completion counter before initiating transfer

[James Clark <james.clark@linaro.org>]

On 10/06/2025 10:31 pm, Vladimir Oltean wrote:
> On Wed, Jun 11, 2025 at 12:01:47AM +0300, Vladimir Oltean wrote:
>> On Tue, Jun 10, 2025 at 04:41:04PM +0100, James Clark wrote:
>>> On 10/06/2025 12:34 pm, Vladimir Oltean wrote:
>>>> On Mon, Jun 09, 2025 at 04:32:38PM +0100, James Clark wrote:
>>>>> In target mode, extra interrupts can be received between the end of a
>>>>> transfer and halting the module if the host continues sending more data.
>>>>
>>>> Presumably you mean not just any extra interrupts can be received, but
>>>> specifically CMDTCF, since that triggers the complete(&dspi->xfer_done)
>>>> call. Other interrupt sources are masked in XSPI mode and should be
>>>> irrelevant.
>>>
>>> Yes complete(&dspi->xfer_done) is called so CMDTCF is set. For example in
>>> one case of underflow I get SPI_SR = 0xca8b0450, which is these flags:
>>>
>>>    TCF, TXRXS, TFUF, TFFF, CMDTCF, RFOF, RFDF, CMDFFF
>>>
>>> Compared to a successful transfer I get 0xc2830330:
>>>
>>>    TCF, TXRXS,       TFFF, CMDTCF,       RFDF, CMDFFF
>>
>> Ok, so my new question would be: if CMDTCF is set, presumably it means a
>> command was transferred. What command was transferred, and who put data
>> in the FIFO for it?
>>
>> Because the answer to the above is AFAIU "no one", I guess the driver
>> should ignore CMDTCF when TFUF (TX FIFO underflow) is set; I consider
>> that to be the logic bug. You are also doing that in patch 4/4, except
>> you still call complete() for some reason. If you don't call complete(),
>> there is no reason to fend against spurious completions.
>>
>> I think I would prefer seeing more deliberate decisions in the driver,
>> it helps if things don't just work by coincidence.
> 
> After thinking some more, I think I agree with your decision.
> 
> If there's a TX FIFO underflow in target mode, presumably there are 2
> cases to handle.
> 
> 1. The underflow occurred in the middle of a large-ish SPI message
>     prepared by the driver, where the driver couldn't refill the TX FIFO
>     fast enough in dspi_interrupt().
> 
> 2. The underflow occurred because the driver had absolutely no SPI
>     message prepared, and yet the host wanted something.
> 
> What changed my mind is that if you don't call complete() on SPI_SR_TFUF
> (like I suggested), then case #1 above will hang. Your proposal is to
> call complete() anyway, but to discard any previous completions,
> associated with case #2, when there's a new message to prepare.
> 
> But I would like you to introduce a comment above the earlier
> reinit_completion() explaining why it is there.

Ok I can do that. I was going to say that a call to complete() would be 
required, sometimes you only get a single interrupt with both CMDTCF and 
TFUF set, rather than two. So it can't be ignored but it seems like 
we've come to the same conclusion.

I did try a few other approaches. One was disabling SPI in the interrupt 
handler on the first completion, but that didn't work because you need 
to wait for stop mode which might hang, and I was still struggling to 
stop the interrupt handler from firing again but I gave up before 
exploring it fully because it didn't feel right.