drivers: spi: Add SPI device support for Apollo3 SoCs

[AlessandroLuo]

This commit adds spi device support for Apollo3 SoCs.

[zephyrbot]

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_ambiq	zephyrproject-rtos/hal_ambiq@df4a986	zephyrproject-rtos/hal_ambiq@5169b60 (main)	zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[RichardSWheatley]

I would change SPIM to SPIC and SPIS to SPIT if we have time.
Otherwise, the code works and looks great!

[AlessandroLuo]

@tbursztyka could you please take another look at this PR?

[tbursztyka]

The logic in this loop does not seem right.

A SPI transaction is not "First I send all, then I read all". If you send 1mb, but the controller has an rx internal buffer of 1kb... you'll get into trouble. I don't know what this LRAM is made of, but I doubt it's limitless.

Also, is calling am_hal_ios_fifo_write() with a NULL as a 2nd parameter going to work? Because it can be NULL. spi_buf->len can be > 0 with spi_buf->tx_buf being NULL. Meaning it's up to the driver (or HAL if it's clever enough) to send dummy bytes instead. And it can be the same in rx so the memcpy below is definitely going to blow up.

Have you tried running tests/drivers/spi/spi_loopback ?

[AlessandroLuo]

Thank you for the comments, let me briefly introduce how our SPIS works.
Ambiq's SPIS (we called it IOS which is I/O Slave) is very specific designed, basically it contains 1KBytes share memory (Local RAM) and the SPI interface, it cannot "send" data out on its own, all operations have to be initiated by the host, so if the slave need to "send" 1mb to the host, we first trigger a GPIO interrupt to the host and then fill as much data to the FIFO, and once the host received the GPIO intrerrupt, it will first read the FIFO counter register FIFOCTR of the slave to know how much data it need to read at that time, after it read the data, the slave will start another iteration to "send" the rest data. In short there must be some kind of handshake before any data transfer.
And Apollo3 SPIS hardware does not support dummy bytes send, that's why I added spi_context_tx_buf_on and spi_context_rx_buf_on check there, NULL buffers should not be passed to HAL.
As our SPIS is actually totally different design compared with SPIM, I don't think it can run the spi_loopback test, we tested the driver with our local testcases.

[tbursztyka]

I missed the fact it is a spiS controller, nevermind. (as well as I mixed up tx/rx_buf_on with tx/rx_on -_-' )

spi_controller_peripheral should be the test to use. (if you have 2 SPI controllers - once acting as the host, the other one as the peripheral - on one board it's quite trivial to set up)

You can't avoid managing the dummy bytes in the driver: if the tx_buf is NULL but tx_len is > 0, it means it should tx dummy bytes of tx_len amount. Or else user code won't be easily portable on your driver. You could allocate a small static array of dummy bytes, and you loop on writing these relevantly to the necessary amount (if the dummy bytes array has a length of 16, if the user needs to tx 17 dummy bytes: it loops twice, once with 16, the next time with 1. For instance.)

Same on rx side, don't you need to "consume" all bytes that have been sent to you? Even the user does not provide any buffer for it I mean. (I does not have to be dummy bytes, it can be any bytes)

the spi buf set is of scatter/gather type, so it can be a complicated setup with various tx/rx, sometimes with no actual buffer but just a length there. That's why I find it weird that you do not loop until it has went through all the set (basically the goto end; in the rx block).

[AlessandroLuo]

I added dummy bytes read/write support, please help to review again, thank you!

[carlescufi]

I would change SPIM to SPIC and SPIS to SPIT if we have time. Otherwise, the code works and looks great!

Indeed, this is in fact a rule:
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-2-inclusive-language

[RichardSWheatley]

I would change SPIM to SPIC and SPIS to SPIT if we have time. Otherwise, the code works and looks great!

Indeed, this is in fact a rule:

https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-2-inclusive-language

So it should be controller/peripheral. Thanks!

[AlessandroLuo]

I would change SPIM to SPIC and SPIS to SPIT if we have time. Otherwise, the code works and looks great!

Indeed, this is in fact a rule:
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-2-inclusive-language

So it should be controller/peripheral. Thanks!

I changed to use controller and device.

[RichardSWheatley]

Should the ambiq,spid.yaml have the spi-controller.yaml or the spi-device.yaml?

[AlessandroLuo]

I created this by reference to what nrf-spis.yaml includes; and changing to spi-device.yaml will cause lots of builld errors, I guess the spi-device.yaml is used for the external device connected to the soc, but not for the soc itself works in device mode.