drivers: spi: Add support for half-duplex (3-wire) SPI #69634
Conversation
zephyrbot
added
area: SPI
ThreeEights
force-pushed
the
rpi-pico-half-duplex-spi
branch
2 times, most recently
from
3e7d204
to
2fc7abb
Compare
|
Drat - the tx/rx counts in the 3-wire logic were wrong. Corrected. |
drivers/spi/spi_rpi_pico_pio.pio
Outdated
| @@ -0,0 +1,78 @@ | |||
| ; Copyright (c) 2023 Stephen Boylan <stephen.boylan@beechwoods.com> | |||
There was a problem hiding this comment.
Strictly speaking, no; at this time, this file is essentially documentation. We went back and forth on including this file. This is the original source for generating the embedded binary PIO code, but can't be used directly in builds without the ability to use pioasm in the build process. If it's the consensus of reviewers that it doesn't make sense to include this file now, I'll remove it.
There was a problem hiding this comment.
This file should be deleted, as it is not possible to include unnecessary files other than documents.
It seems that the code is already embedded in the C source as a comment, but it might be better to write it in the form of the original code here.
There was a problem hiding this comment.
@ThreeEights
Please remove this file.
ThreeEights
force-pushed
the
rpi-pico-half-duplex-spi
branch
from
2fc7abb
to
c2d61d7
Compare
drivers/spi/spi_rpi_pico_pio.c
Outdated
| #if defined(CONFIG_SPI_RPI_PICO_PIO_32_BIT) | ||
| case 4: { | ||
| if (txbuf) { | ||
| txrx = ((uint32_t *)txbuf)[data->tx_count]; | ||
| } | ||
| spi_pico_pio_sm_put32(data->pio, data->pio_sm, txrx); | ||
| data->tx_count += 4; | ||
| } break; | ||
| #endif /* CONFIG_SPI_RPI_PICO_PIO_32_BIT */ | ||
|
|
||
| #if defined(CONFIG_SPI_RPI_PICO_PIO_16_BIT) | ||
| case 2: { | ||
| if (txbuf) { | ||
| txrx = ((uint16_t *)txbuf)[data->tx_count]; | ||
| } | ||
| spi_pico_pio_sm_put16(data->pio, data->pio_sm, txrx); | ||
| data->tx_count += 2; | ||
| } break; | ||
| #endif /* CONFIG_SPI_RPI_PICO_PIO_16_BIT */ | ||
|
|
||
| #if defined(CONFIG_SPI_RPI_PICO_PIO_8_BIT) | ||
| case 1: { | ||
| if (txbuf) { | ||
| txrx = ((uint8_t *)txbuf)[data->tx_count]; | ||
| } | ||
| spi_pico_pio_sm_put8(data->pio, data->pio_sm, txrx); | ||
| data->tx_count++; | ||
| } break; | ||
| #endif /*CONFIG_SPI_RPI_PICO_PIO_8_BIT*/ |
There was a problem hiding this comment.
This is just a matter of style and preference, but there are ifdefs all over the place, so it would be nice to see them put together. For example, how about changing the function to something like this?
static inline int spi_pico_pio_sm_put(PIO pio, uint sm, void *dataptr, size_t bits)
{
switch(bits) {
case 4:
*((io_rw_32 *)&pio->txf[sm]) = *(uint32_t*)dataptr;
return 4;
case 2:
*((io_rw_8 *)&pio->txf[sm]) = *(uint16_t*)dataptr;
return 2;
case 1:
*((io_rw_8 *)&pio->txf[sm]) = *(uint8_t*)dataptr;
return 1;
default:
return 0;
}
}
There was a problem hiding this comment.
Unless one wants to save some ROM bytes, in this case the defines are useful. (unless, again, these defines disappear and become DTS properties)
There was a problem hiding this comment.
Condensing the ..._put and ..._get code, as @soburi suggested, is a good idea. That makes the source code a lot shorter and cleaner. I'll try to get that to work.
There was a problem hiding this comment.
Unfortunately, condensing the get and set functions won't work. The logic is different in the 4-wire and 3-wire cases, and it's necessary to split reading the data from the tx buffer and writing to the send FIFO: if there's data to send, it needs to use the content from the txbuf, but if there's no txbuf data, 0 has to be sent in order to clock the receive data. Similarly, the data in the receive FIFO has to be read out, even if there's no rxbuf, in order to sync the send and receive cycle with the spi_context.
The only changes I can see that might make sense would be to move the FIFO operations out of the get and put functions, eliminating those, which can reduce the number of places where compile-time checks are needed for the 2 and 4 byte transactions.
Would it make sense to remove those Kconfig options? Is that modest increase in overhead for the most common 8-bit case acceptable?
ThreeEights
force-pushed
the
rpi-pico-half-duplex-spi
branch
from
c2d61d7
to
33d1ca0
Compare
ThreeEights
force-pushed
the
rpi-pico-half-duplex-spi
branch
from
33d1ca0
to
241fd07
Compare
|
@soburi, @tbursztyka, @yonsch - Can you please take a look at the latest changes and see if the issues are resolved? Thanks! |
|
@ThreeEights: Is this the last gap that is needed to use bluetooth/wifi on the Pico W? Or is there further glue code between the CYW43xxx driver and this required? Do you happen to have device-trees for enabling the cyw43439 around? Then I would be happy to give it a try :). |
|
@Ablu - Alas, no: The current Infineon WiFi driver for the CYW43439 does not have SPI support. If you're interested, you can view my latest attempt (as of yesterday!) on my fork: |
Add support for half-duplex (3-wire) SPI operation using the Raspberry Pi Pico PIO. To allow control of the size of the driver, including half-duplex support is optional, under the control of Kconfig options. The original PIO source code is also included as a reference. Corrected 3-wire tx/rx counts. Enable half-duplex code based on DTS configuration Replace runtime checks with static BUILD_ASSERT() Remove too-fussy Kconfig options Removed PIO source per review request Signed-off-by: Steve Boylan <stephen.boylan@beechwoods.com>
ThreeEights
force-pushed
the
rpi-pico-half-duplex-spi
branch
from
957b14d
to
a56fd72
Compare
|
@tbursztyka |
|
@yonsch - Could you please take a look at this pull request? Thanks. |
Add support for half-duplex (3-wire) SPI operation using the Raspberry Pi Pico PIO. To allow control of the size of the driver, including half-duplex support is optional, under the control of Kconfig options.
The original PIO source code is also included as a reference.