Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CS pin to SPIConfig in all architectures #3801

Open
ivoszz opened this issue Jun 20, 2023 · 6 comments
Open

Add CS pin to SPIConfig in all architectures #3801

ivoszz opened this issue Jun 20, 2023 · 6 comments
Labels
duplicate This issue or pull request already exists enhancement New feature or request

Comments

@ivoszz
Copy link
Contributor

ivoszz commented Jun 20, 2023

When configuring the SPI bus, the CS (SS) pin is not directly part of the bus configuration in most MCUs (currently only in ESP32C3 and MIMXRT1062). This does not allow to take full advantage of HW SPI support and activation of the CS pin is part of the driver (see tinygo-org/drivers#583). Where possible it would be a good idea to add the option to configure the CS pin directly in the machine.SPIConfig type definition.

I can try to add it for ESP32 architecture.

@aykevl
Copy link
Member

aykevl commented Jun 20, 2023

This has been suggested before, but it's not that simple. If you want to add an API for this, you also need to think of the following cases:

  • How do you use a single SPI bus with multiple chips? In other words, how should drivers set or change the pin? At the moment, it's simple: they set the CS pin manually to low or high as needed, and for most chips this is fine.
  • Most SPI devices don't actually need to toggle CS on every byte or 16-byte word (as some do), toggling CS would be a performance regression.
  • What about chips (like the nrf) that don't support CS in hardware? How should drivers deal with chips that do and chips that don't?

@ivoszz
Copy link
Contributor Author

ivoszz commented Jun 20, 2023

I understand this could be a big hit.

  1. In the case of multiple devices on one bus, the problem is bigger than just changing the CS pin. Devices can communicate at different frequencies, in different modes or even with different settings (4-wire, 3-wire, even though only 4-wire is supported in tinygo). The only option is to set the parameters before each transaction (as in C now). Or remember the current SPI settings and change them only if modified.
  2. The performance loss in this case is minimal, switching in the driver may instead violate the timing conditions of some devices.
  3. If the CS pin is not supported in HW, CS switching can always be done in software. Changing it in the bus driver is, in my opinion, much more adequate than doing it in the device driver. The CS signal is part of the SPI bus.

My suggestions are not meant as a criticism. It is clear that with limited resources you cannot cover everything. But practically, only one device is currently supported on the SPI bus. It is true that this configuration covers the vast majority of implementations. But it would probably be fair to mention this limitation in the documentation, as well as not supporting 3-wire/half duplex communication.

I would like to know if it makes sense to even start discussing this.

@aykevl
Copy link
Member

aykevl commented Jun 21, 2023

  1. In the case of multiple devices on one bus, the problem is bigger than just changing the CS pin. Devices can communicate at different frequencies, in different modes or even with different settings (4-wire, 3-wire, even though only 4-wire is supported in tinygo). The only option is to set the parameters before each transaction (as in C now). Or remember the current SPI settings and change them only if modified.

True in a strict sense, but in many cases the settings are in fact the same and reconfiguring the SPI is not necessary. For frequency it's always possible to run at a lower frequency. And most devices work at mode 0, or both mode 0 and mode 3. So the only thing different would be the CS pin.

  1. The performance loss in this case is minimal, switching in the driver may instead violate the timing conditions of some devices.

If you switch at each byte transmitted, it is certainly not minimal: it can increase each byte from 8 ticks to 9 or 9.5 ticks (in the case of the rp2040, it's 9.5). That's a 19% loss in performance.
If the CS pin is switched on each transaction (not each byte), then yes it would be much smaller and probably better than manually changing CS. But I'm not sure which of the two your goal is here.

  1. If the CS pin is not supported in HW, CS switching can always be done in software. Changing it in the bus driver is, in my opinion, much more adequate than doing it in the device driver. The CS signal is part of the SPI bus.

I see, you mean supporting CS inside the machine package for chips that don't support it. Yes, that would satisfy my API concerns. However, this would still be a slight loss in performance for chips that don't support CS in hardware. Take a look at tinygo-org/drivers#549 for example. In this case I only switch CS when needed, combining multiple transactions and only changing CS at the start and end of it.

Can you provide some concrete examples where configuring SPI in hardware would be beneficial, and how a driver would use this feature? It doesn't really help talking about hypothetical situations.

@ivoszz
Copy link
Contributor Author

ivoszz commented Jun 28, 2023

I agree that it is good to deal with real problems and situations. And while I believe that HW support for the CS pin in the SPI driver is a good thing and would not be too complicated to implement properly, there are many issues with higher priority. One of them, in my opinion, is the lack of support for the ESP32 family in tinygo. I'd be happy to help improve it, but I'd address this in a separate thread.

Here I would just like to mention an outline of HW support for CS:

  1. If the CS pin of the SPI driver was not set or was set to NoPin, it would be ignored and the behavior would be identical to the current state. This would ensure backward compatibility.
  2. The setting of the CS pin would then depend on the specific architecture. Some have support for multiple devices on the bus (esp32). For those that do not allow this, the current CS pin setting would be part of the SPI bus status information. Before sending data, a check would be made to ensure that the requested CS pin is currently configured for HW SPI. If not, the CS pin configuration would be changed and its current value set.
  3. If the CS pin was not supported in HW, it would be switched to a low state before the communication starts and to a high state after the transaction is completed.

@soypat
Copy link
Contributor

soypat commented Jul 15, 2023

This seems like a hardware dependent thing... could we not add a method on the machine.SPI type, i.e: (*machine.SPI) SetCS(machine.Pin) error?

@deadprogram deadprogram added enhancement New feature or request duplicate This issue or pull request already exists labels Aug 18, 2023
@deadprogram
Copy link
Member

Also duplicate of #1615

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants