ChibiOS Free Embedded RTOS

When porting my application to STM32H7, I found that SPI did not work correctly.

I'll just list the things I addressed, and I will attach a patch later:

- I found that the DMEIF interrupt was occasionally being triggered. Since DMEIF is only triggered on a peripheral-to-memory transfer where the MINC bit is cleared, this interrupt can only happen on the transfer to dummyrx. So, it should be ignored, rather than causing the DMA_ERROR_HOOK to be called. (Why have the dummy variables in the first place? This SPI peripheral supports operation in simplex mode.)

- IFCR is being set to 0xFFFFFFFFU in order to clear flags. Documentation states that reserved bits in this register are not to be changed from their reset value, so this should be 0xFF8.

- I use a software-controlled slave select. I am not certain the register configuration is correct for that. I changed the register settings as follows: Clear the SSOE bit, set the SSM bit, set the SSI bit (unsure if necessary, the table embedded in fig. 616 in the reference manual seems to imply so). I am not sure if the ChibiOS driver is intended to support this mode of operation or if it is intended to only support hardware-controlled slave select.

- When debugging, I found that if I put a breakpoint after dmaStreamEnable and before the CSTART bit is set in spi_lld_send, it causes spiSend to get stuck waiting for the interrupt and never return. Changing the order so that CSTART is set prior to dma setup fixes this. I am unsure why this happens - perhaps the SPI peripheral does not like bytes being inserted in the txfifo prior to CSTART being set.

- The CSUSP (suspend request) bit was being set in the dma rx interrupt. Since the SUSPC (suspend clear) bit in the IFCR is only set in spi_lld_start, the driver works for the first transaction or two, and then gets stuck.

Interesting, so this is related viewtopic.php?f=25&t=5329

Giovanni

I think I am able to reproduce the problem now, working on it.

Giovanni

Hi,

I committed in trunk a tentative fix for the CSUSP issue, could you check on your side if this works for you?

About the other points:

jschall wrote: I found that the DMEIF interrupt was occasionally being triggered. Since DMEIF is only triggered on a peripheral-to-memory transfer where the MINC bit is cleared, this interrupt can only happen on the transfer to dummyrx. So, it should be ignored, rather than causing the DMA_ERROR_HOOK to be called. (Why have the dummy variables in the first place? This SPI peripheral supports operation in simplex mode.)

Please verify this again, it smells of some device errata. About simplex mode, it is not used because it would make the driver more complex (a different mode for each API required re-configuring SPI before each operation).

jschall wrote:IFCR is being set to 0xFFFFFFFFU in order to clear flags. Documentation states that reserved bits in this register are not to be changed from their reset value, so this should be 0xFF8.

They write that for all unused bits in all register, some kind of "magical formula". The register is write-only so it makes no sense.

jschall wrote:When debugging, I found that if I put a breakpoint after dmaStreamEnable and before the CSTART bit is set in spi_lld_send, it causes spiSend to get stuck waiting for the interrupt and never return. Changing the order so that CSTART is set prior to dma setup fixes this. I am unsure why this happens - perhaps the SPI peripheral does not like bytes being inserted in the txfifo prior to CSTART being set.

Could you check this again, it could be true but I want to make one fix at time.

Giovanni

I checked out the fresh changes.

After a few exchanges, the driver gets stuck in



with OVR set in IER.

If spi_lld_wait_complete() just resets IFCR, i.e. the line above is commented, the driver appears to work.

The problem is that CSTART should be cleared or being cleared on function entry by the previous CSUSP, the problem is, why is it stuck to one?

I am unable to reproduce this one, are 4 operations sufficient to trigger it? how many are "few" ?

Giovanni

Why CSUSP at all, other than on spiStop? I guess it would have to do with hardware controlled slave select?

Please verify this again, it smells of some device errata.

Does it matter if it actually happens in a given test? It is pretty clear that we should be ignoring that particular error based on the description of it in the RM, and it seems like a possibility if the memory bus is being slammed. I'm ready to just patch this and move on.

They write that for all unused bits in all register, some kind of "magical formula". The register is write-only so it makes no sense.

What is the cost of simply complying with it?

Could you check this again, it could be true but I want to make one fix at time.

Is there potential harm in reordering DMA config and CSTART?

There is a SUSPC bit to clear the SUSP bit (not to be confused to CSUSP, which sets it). Maybe we need to be clearing the SUSP bit?

Giovanni wrote:I am unable to reproduce this one, are 4 operations sufficient to trigger it? how many are "few" ?

It typically happens after hundreds of exchanges on my hardware, I am still trying to find a good way to reproduce it.

jschall wrote:Why CSUSP at all, other than on spiStop? I guess it would have to do with hardware controlled slave select?

Not suspending (plus not calling spi_lld_wait_complete) actually solves all the problems for me (I also use software slave select with SSM and SSI bits set).

jschall wrote:Is there potential harm in reordering DMA config and CSTART?

Where exactly did you put CSTART, e.g. in spi_lld_ignore()? Before dmaStreamSetMemory0()?

Not suspending (plus not calling spi_lld_wait_complete) actually solves all the problems for me (I also use software slave select with SSM and SSI bits set).

For me as well, but I can see why suspending is attractive - it allows the hardware slave select to work, I think. Also spiAbort (which calls spi_lld_abort) is supposed to work, and it uses CSUSP.

Where exactly did you put CSTART, e.g. in spi_lld_ignore()? Before dmaStreamSetMemory0()?

At the top of the function, after the assert, but I don't use ignore at all in my use case so I have not tested it.