- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Looking into wiced_spi_* and platform_spi_* APIs and they don't look like thread safe to me, is that the case ? Also, it looks like current SPI implementation would not support multiples devices with difference settings such as clock frequency or bus mode, is that the case as well ?
Solved! Go to Solution.
- Tags:
- spi
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hi,
Yes, the wiced_spi_* APIs are not thread safe. Locking/Unlocking is required if multiple threads are accessing to the same SPI peripheral.
Yes, currently the same speed, mode and data size devices could share the same SPI peripheral.
Seyhan
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hi,
Yes, the wiced_spi_* APIs are not thread safe. Locking/Unlocking is required if multiple threads are accessing to the same SPI peripheral.
Yes, currently the same speed, mode and data size devices could share the same SPI peripheral.
Seyhan
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Thanks for confirming, I have added locking/unlocking as well as per bus context to allow different device configuration. It seems to work well for me. Let me know if you would like to share that back.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Why is the same speed, mode and data size shared? It looks like the wiced_spi_device_t structure is passed into the init and the deinit (which looks more like a reinit to me). Since you have to specify a different GPIO pin to select by, why can't the speed/mode/size be specified on each init/reinit when you select the desired slave?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
SPI bus is initialized once, during spi_init. Previous settings are not retained. Currently, if you init port with settings A, then again for another device with setting B, settings A will be lost. Worst, a transfer for device A, after device B has been initialized will be performed with most recent settings.
Current SPI bus driver is pretty much broken for multi-client use. A concept of bus context needs to be introduced and bus re-configure for each client when necessary between transactions as it is not the case currently.
Also, a lock / port needs to be added so multi-threaded system can access ports safely.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
So is this a problem with the SDK or the driver talking to the SDK?
On the side of the SDK, you could locally have a state that indicates which SPI slave you are talking to, you can easily switch states on the calling side by calling init/deinit. As long as you switch at request boundaries, which would be ensured by your locking code, the slave should not see the switch and the reconfigure should work fine.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I believe the is should be fixed on the MCU platform code, it is quite easy to achieve, what is the point of having SPI device config passed down to the SPI platform APIs if they are not used to reconfigure the bus prior to transfer ?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I am not disagreeing with you. The simplest interface that could have been given to you would be one that just requested a transfer and passed a wiced_spi_device_t , which it does, and have that interface perform the init/deinit as is required. That would be clean and would solve this part of your request.
I don't know what the API designer(s) had in mind. It is possible they separated the init/deinit from the transfer to allow the hardware to be turned off to save power and pass the structure pointer just as a handle. It is hard to tell without asking them.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
The deinit does actually do anything, I think SPI wasn't quite thought through as a bus, with multiple client in mind. It was clearly design to handle the serial flash that comes with the ref design.
Since I/O are pretty limited on the module itself, I use a SPI GPIO expander, a SPI display, etc... and the driver in its current state does not quite cut it.
I open that thread before taking on the task of modifying the SDK code to my need, just to check that I was not missing anything.
Maybe my use case is singular, but maybe not 😉
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
The deinit in its current state, looks like you could pass a different wiced_spi_device_t structure and it would reinit, which was part of my first comment.
The API designer thought it out enough to pass a GPIO and didn't make the assumption that there was a hard wired slave select line. So they did move in the right direction. Making the change to drop init/deinit and just perform the transfer (configuring as required based on the passed wiced_spi_device_t) seems like it is a simple request and one that should be quite common, and not just a singular case. Does anyone know who designed this interface?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
You are indeed right about deinit calling init, I was looking at the MCU code itself.
Maybe they were expecting caller to take care of handling the lock, calling init before every single transfer... That would probably work, but it is not intuitive, not efficient either if you ask me.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
The locking is best handled by the caller. This could be handled by a semaphore or disabling interrupts, as is required. The latter could be a performance hit and should not be inflicted on all for the cases that the request is being performed at an ISR level, I would assume this to be a minority.
If the MCU code kept the state of the current connection and compared against that before performing the inefficient configuration setup, then you would only take the hit when you change the device you are talking to. In most cases, you are going to have bursts of activity on one device at a time and this would solve that issue.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I am not sure I agree by the lock handled by caller, but we can agree to disagree on that 🙂
Regarding current port state, this is what I did, comparison between caller device settings and current set settings comes down to a few memcpm() on a few bytes, this is quite painless. If setting is different, port is reconfigured.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I will agree on the disagreeing 🙂
You chose the right answer and we just need to get this pushed into the MCU code and it will be handled for all. I will ask around to see who designed that and if it can be changed.
Thanks, Denis
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Thanks for the feedback !
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I believe that the future direction for this interface is to keep the three interfaces.
- Init will register the SPI device.
- Deinit will unregister it and allow the interface to be shut off, assuming no other registered SPI devices are still outstanding.
- Transfer will check the passed wiced_spi_device_t and verify it is registered and will perform any required configuration to make sure that the SPI bus is ready to talk to the specific device.
The reasoning behind keeping the init/deinit is as I mentioned above that it might be there for power management reasons and that was a correct assumption.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
I think this is fine, as long as the the transfer part get fixed and allow different configurations for different devices.
I still do not understand why the lock should be handled by the upper layer, but I feel like we could be discussing this all day 🙂
As for power management, in a SPI master configuration, interface could be shutdown at all times, except during transfer, powerUp -> transfer -> powerDown. This is assuming power up does not imply long timing delay, like waiting for PLL to lock for instance, but I don't think this is the case.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
The transfer part will get fixed.
The lock that works for you may not work for me. I may have only one device and I don't want to take any extra cycles performing useless code. I may interact with SPI in a task. I may talk at the ISR level... The answers are all different and to make this generic would be difficult at best if efficiency is top priority.
You can take the entire interface down, but that is not very efficient. You will gain the same feel to init and deinit for each transfer. If I am talking to a SPI flash, then I may need to perform a number of operations to make data flow in one direction or the other. In this case, I don't want to take the power to come up and restart my clocks for each SPI transaction.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Alright, fair enough, maybe a sample app for SPI showing API flow would be great, serial flash driver isn't really appropriate for that.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hi
If I'm using SPI6 and SPI1(each has only one device connecetd to it) do I need to call the spi_init() function each time while using SPI6 or SPI1 ?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hi,
As long as you have only one slave per master, I don't think you need to call spi_init() between each transfer.
Did you actually try ?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Thanks,
I haven't tried it yet, but will post the result once I test it....
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Discussion is being locked. If you have any follow-up, please start a new discussion.