Skip to content

SPI upgrade - per-peripheral mutex and GPIO-based SSEL #9469

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 1, 2019
Merged

SPI upgrade - per-peripheral mutex and GPIO-based SSEL #9469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e059b7
SPI upgrade - per-peripheral mutex and GPIO-based SSEL
kjbracey Jan 23, 2019
c368021
SPIFBlockDevice.h: include SingletonPtr.h
kjbracey Feb 5, 2019
df7e336
Cope with HALs not defining SPIName
kjbracey Feb 28, 2019
b12be6b
Rename SPI_COUNT to DEVICE_SPI_COUNT
kjbracey Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#ifndef MBED_SPIF_BLOCK_DEVICE_H
#define MBED_SPIF_BLOCK_DEVICE_H

#include "platform/SingletonPtr.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the static array of peripheral management blocks contains Mutexes, and if those weren't wrapped in a SingletonPtr, the array would be placed into RAM even if an image didn't use SPI.

#include "SPI.h"
#include "DigitalOut.h"
#include "BlockDevice.h"
Expand Down
188 changes: 132 additions & 56 deletions drivers/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,31 +25,96 @@

namespace mbed {

#if DEVICE_SPI_ASYNCH && TRANSACTION_QUEUE_SIZE_SPI
CircularBuffer<Transaction<SPI>, TRANSACTION_QUEUE_SIZE_SPI> SPI::_transaction_buffer;
#endif
SPI::spi_peripheral_s SPI::_peripherals[SPI_PERIPHERALS_USED];
int SPI::_peripherals_used;

SPI::SPI(PinName mosi, PinName miso, PinName sclk, PinName ssel) :
_spi(),
#if DEVICE_SPI_ASYNCH
_irq(this),
_usage(DMA_USAGE_NEVER),
_deep_sleep_locked(false),
#endif
_bits(8),
_mode(0),
_hz(1000000),
_write_fill(SPI_FILL_CHAR)
_mosi(mosi),
_miso(miso),
_sclk(sclk),
_hw_ssel(ssel),
_sw_ssel(NC)
{
_do_construct();
}

SPI::SPI(PinName mosi, PinName miso, PinName sclk, PinName ssel, use_gpio_ssel_t) :
#if DEVICE_SPI_ASYNCH
_irq(this),
#endif
_mosi(mosi),
_miso(miso),
_sclk(sclk),
_hw_ssel(NC),
_sw_ssel(ssel, 1)
{
_do_construct();
}

void SPI::_do_construct()
{
// No lock needed in the constructor
spi_init(&_spi, mosi, miso, sclk, ssel);
#if DEVICE_SPI_ASYNCH
_usage = DMA_USAGE_NEVER;
_deep_sleep_locked = false;
#endif
_select_count = 0;
_bits = 8;
_mode = 0;
_hz = 1000000;
_write_fill = SPI_FILL_CHAR;

// Need backwards compatibility with HALs not providing API
#ifdef DEVICE_SPI_COUNT
SPIName name = spi_get_peripheral_name(_mosi, _miso, _sclk);
#else
SPIName name = GlobalSPI;
#endif

core_util_critical_section_enter();
// lookup in a critical section if we already have it else initialize it

_peripheral = SPI::_lookup(name);
if (!_peripheral) {
_peripheral = SPI::_alloc();
_peripheral->name = name;
}
core_util_critical_section_exit();
// we don't need to _acquire at this stage.
// this will be done anyway before any operation.
}

SPI::~SPI()
{
if (_owner == this) {
_owner = NULL;
lock();
/* Make sure a stale pointer isn't left in peripheral's owner field */
if (_peripheral->owner == this) {
_peripheral->owner = NULL;
}
unlock();
}

SPI::spi_peripheral_s *SPI::_lookup(SPI::SPIName name)
{
SPI::spi_peripheral_s *result = NULL;
core_util_critical_section_enter();
for (int idx = 0; idx < _peripherals_used; idx++) {
if (_peripherals[idx].name == name) {
result = &_peripherals[idx];
break;
}
}
core_util_critical_section_exit();
return result;
}

SPI::spi_peripheral_s *SPI::_alloc()
{
MBED_ASSERT(_peripherals_used < SPI_PERIPHERALS_USED);
return &_peripherals[_peripherals_used++];
}

void SPI::format(int bits, int mode)
Expand All @@ -60,8 +125,8 @@ void SPI::format(int bits, int mode)
// If changing format while you are the owner then just
// update format, but if owner is changed then even frequency should be
// updated which is done by acquire.
if (_owner == this) {
spi_format(&_spi, _bits, _mode, 0);
if (_peripheral->owner == this) {
spi_format(&_peripheral->spi, _bits, _mode, 0);
} else {
_acquire();
}
Expand All @@ -75,69 +140,78 @@ void SPI::frequency(int hz)
// If changing format while you are the owner then just
// update frequency, but if owner is changed then even frequency should be
// updated which is done by acquire.
if (_owner == this) {
spi_frequency(&_spi, _hz);
if (_peripheral->owner == this) {
spi_frequency(&_peripheral->spi, _hz);
} else {
_acquire();
}
unlock();
}

SPI *SPI::_owner = NULL;
SingletonPtr<PlatformMutex> SPI::_mutex;

// ignore the fact there are multiple physical spis, and always update if it wasn't us last
void SPI::aquire()
{
lock();
if (_owner != this) {
spi_format(&_spi, _bits, _mode, 0);
spi_frequency(&_spi, _hz);
_owner = this;
}
unlock();
}

// Note: Private function with no locking
void SPI::_acquire()
{
if (_owner != this) {
spi_format(&_spi, _bits, _mode, 0);
spi_frequency(&_spi, _hz);
_owner = this;
if (_peripheral->owner != this) {
spi_init(&_peripheral->spi, _mosi, _miso, _sclk, _hw_ssel);
spi_format(&_peripheral->spi, _bits, _mode, 0);
spi_frequency(&_peripheral->spi, _hz);
_peripheral->owner = this;
}
}

int SPI::write(int value)
{
lock();
_acquire();
int ret = spi_master_write(&_spi, value);
unlock();
select();
int ret = spi_master_write(&_peripheral->spi, value);
deselect();
return ret;
}

int SPI::write(const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length)
{
lock();
_acquire();
int ret = spi_master_block_write(&_spi, tx_buffer, tx_length, rx_buffer, rx_length, _write_fill);
unlock();
select();
int ret = spi_master_block_write(&_peripheral->spi, tx_buffer, tx_length, rx_buffer, rx_length, _write_fill);
deselect();
return ret;
}

void SPI::_set_ssel(int val)
{
if (_sw_ssel.is_connected()) {
_sw_ssel = val;
}
}

void SPI::lock()
{
_mutex->lock();
_peripheral->mutex->lock();
}

void SPI::select()
{
lock();
if (_select_count++ == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a matter of coding style, but for clarity, could this increment be done after the if ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did that quite deliberately because a couple of days before writing it I saw some existing code containing this:

core_util_atomic_incr_u32(foo, 1);
if (foo == 1) {

which is not thread-safe, and needs to be

if (core_util_atomic_incr_u32(foo, 1) == 1) {

I think if people can't get comfortable with the increment/decrement pattern, it just encourages that sort of mistake. (Although breaking it up doesn't hurt in this case, as we are in a lock).

_acquire();
_set_ssel(0);
}
}

void SPI::unlock()
{
_mutex->unlock();
_peripheral->mutex->unlock();
}

void SPI::deselect()
{
if (--_select_count == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in the previous commend, could this decrement be done before the if to avoid any confusion ?

_set_ssel(1);
}
unlock();
}

void SPI::set_default_write_value(char data)
{
// this does not actually need to lock the peripheral.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So could we remove lock/unlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily - the lock is also functioning as the "API lock". If the SPI object is itself supposed to be thread-safe (as opposed to just thread safety for multiple SPI objects accessing one bus), then there needs to be synchronisation on its members. It currently seems that such objects are trying to be locally thread-safe, but maybe that needs more thought.

lock();
_write_fill = data;
unlock();
Expand All @@ -147,7 +221,7 @@ void SPI::set_default_write_value(char data)

int SPI::transfer(const void *tx_buffer, int tx_length, void *rx_buffer, int rx_length, unsigned char bit_width, const event_callback_t &callback, int event)
{
if (spi_active(&_spi)) {
if (spi_active(&_peripheral->spi)) {
return queue_transfer(tx_buffer, tx_length, rx_buffer, rx_length, bit_width, callback, event);
}
start_transfer(tx_buffer, tx_length, rx_buffer, rx_length, bit_width, callback, event);
Expand All @@ -156,7 +230,7 @@ int SPI::transfer(const void *tx_buffer, int tx_length, void *rx_buffer, int rx_

void SPI::abort_transfer()
{
spi_abort_asynch(&_spi);
spi_abort_asynch(&_peripheral->spi);
unlock_deep_sleep();
#if TRANSACTION_QUEUE_SIZE_SPI
dequeue_transaction();
Expand All @@ -167,7 +241,7 @@ void SPI::abort_transfer()
void SPI::clear_transfer_buffer()
{
#if TRANSACTION_QUEUE_SIZE_SPI
_transaction_buffer.reset();
_peripheral->transaction_buffer->reset();
#endif
}

Expand All @@ -179,7 +253,7 @@ void SPI::abort_all_transfers()

int SPI::set_dma_usage(DMAUsage usage)
{
if (spi_active(&_spi)) {
if (spi_active(&_peripheral->spi)) {
return -1;
}
_usage = usage;
Expand All @@ -199,12 +273,12 @@ int SPI::queue_transfer(const void *tx_buffer, int tx_length, void *rx_buffer, i
t.callback = callback;
t.width = bit_width;
Transaction<SPI> transaction(this, t);
if (_transaction_buffer.full()) {
if (_peripheral->transaction_buffer->full()) {
return -1; // the buffer is full
} else {
core_util_critical_section_enter();
_transaction_buffer.push(transaction);
if (!spi_active(&_spi)) {
_peripheral->transaction_buffer->push(transaction);
if (!spi_active(&_peripheral->spi)) {
dequeue_transaction();
}
core_util_critical_section_exit();
Expand All @@ -219,9 +293,10 @@ void SPI::start_transfer(const void *tx_buffer, int tx_length, void *rx_buffer,
{
lock_deep_sleep();
_acquire();
_set_ssel(0);
_callback = callback;
_irq.callback(&SPI::irq_handler_asynch);
spi_master_transfer(&_spi, tx_buffer, tx_length, rx_buffer, rx_length, bit_width, _irq.entry(), event, _usage);
spi_master_transfer(&_peripheral->spi, tx_buffer, tx_length, rx_buffer, rx_length, bit_width, _irq.entry(), event, _usage);
}

void SPI::lock_deep_sleep()
Expand Down Expand Up @@ -250,7 +325,7 @@ void SPI::start_transaction(transaction_t *data)
void SPI::dequeue_transaction()
{
Transaction<SPI> t;
if (_transaction_buffer.pop(t)) {
if (_peripheral->transaction_buffer->pop(t)) {
SPI *obj = t.get_object();
transaction_t *data = t.get_transaction();
obj->start_transaction(data);
Expand All @@ -261,8 +336,9 @@ void SPI::dequeue_transaction()

void SPI::irq_handler_asynch(void)
{
int event = spi_irq_handler_asynch(&_spi);
int event = spi_irq_handler_asynch(&_peripheral->spi);
if (_callback && (event & SPI_EVENT_ALL)) {
_set_ssel(1);
unlock_deep_sleep();
_callback.call(event & SPI_EVENT_ALL);
}
Expand Down
Loading