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

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

Merged
merged 4 commits into from Mar 1, 2019

Conversation

Projects
None yet
@kjbracey-arm
Copy link
Contributor

commented Jan 23, 2019

Description

This commit takes some of the work done on the SPI class from #8445, and refines it, to provide the per-peripheral mutex functionality.

This also implements GPIO-based SSEL, which exposes a new select()/deselect() API for users to group transfers, and should work on every platform (unlike the HAL-based SSEL). This requires users to use a new constructor to avoid backwards compatibility issues.

To activate the per-peripheral mutex, the HAL must define SPI_COUNT and provide spi_get_peripheral_name(). (In #8445 this is a reworked spi_get_module, but the name is changed here to avoid a collision with existing HALs - this commit is designed to work without wider HAL changes).

Fixes: #9149

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@jarlamsa, @c1728p9, @ithinuel

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

First draft - just for discussion and @jarlamsa to try out at the minute, really.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:spi_muxing branch 2 times, most recently Jan 23, 2019

@kjbracey-arm kjbracey-arm changed the title SPI upgrade - per-bus mutex and GPIO-based SSEL SPI upgrade - per-peripheral mutex and GPIO-based SSEL Jan 23, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:spi_muxing branch Jan 23, 2019

@ciarmcom ciarmcom requested review from c1728p9, ithinuel, jarlamsa and ARMmbed/mbed-os-maintainers Jan 23, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@jarlamsa

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

[Error] SPI.cpp@239,37: 'struct SingletonPtr<mbed::CircularBuffer<mbed::Transaction<mbed::SPI>, 2ul> >' has no member named 'reset'
[Error] SPI.cpp@271,41: 'struct SingletonPtr<mbed::CircularBuffer<mbed::Transaction<mbed::SPI>, 2ul> >' has no member named 'full'
[Error] SPI.cpp@275,41: 'struct SingletonPtr<mbed::CircularBuffer<mbed::Transaction<mbed::SPI>, 2ul> >' has no member named 'push'
[Error] SPI.cpp@323,41: 'struct SingletonPtr<mbed::CircularBuffer<mbed::Transaction<mbed::SPI>, 2ul> >' has no member named 'pop'

You probably wanted to use the member access through pointer operator with the access to the transaction_buffer.

drivers/SPI.cpp Outdated
@@ -167,7 +236,7 @@ void SPI::abort_transfer()
void SPI::clear_transfer_buffer()
{
#if TRANSACTION_QUEUE_SIZE_SPI
_transaction_buffer.reset();
_peripheral->transaction_buffer.reset();

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Jan 30, 2019

Contributor

transaction_buffer->reset

drivers/SPI.cpp Outdated
@@ -199,12 +268,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()) {

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Jan 30, 2019

Contributor

transaction_buffer->full

drivers/SPI.cpp Outdated
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);

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Jan 30, 2019

Contributor

transaction_buffer->push

drivers/SPI.cpp Outdated
@@ -250,7 +320,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)) {

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Jan 30, 2019

Contributor

transaction_buffer->pop

@jarlamsa

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Otherwise seems to be working without issues on NUCLEO_F429ZI when made changes to stm_spi_api.c and TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/PeripheralNames.h

Do we bring the HAL-changes in with this PR or in a separate PR?

@ithinuel

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Won't this change collide with the work done on branch feature-hal-spec-spi ?

Wouldn't it be better to keep the status quo until the feature gets actually merged into master ?
(I am asking this with in mind the maintenance cost induced by changes on master when time comes to rebase the feature branch.)

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

Yes, it collides, and the feature branch will need to be rebased and aligned with this :/

The problem is that this feature is required for 5.12, but your feature branch isn't due to land for 5.12.

The system-wide mutex is a blocker for @jarlamsa's project - SD card accesses break the radio driver.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

First draft - just for discussion and @jarlamsa to try out at the minute, really.

Will like to see at least some test code to verify the changes in this PR

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

I'm not sure what the current SPI test infrastructure consists of. I see #7976 in progress for specific testing, but that's part of the pending feature branch, and it's testing the HAL, not class SPI.

For the new select/deselect functionality, bringing it into use by SD drivers and 802.15.4 radio drivers would get that exercised by their test suites. (I've asked @jarlamsa to look at that - would simplify their code and offset code size increase here).

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:spi_muxing branch 2 times, most recently Jan 31, 2019

@cmonr cmonr added needs: work and removed needs: review labels Jan 31, 2019

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:spi_muxing branch Feb 1, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@ithinuel - new commit reworks the peripheral blocks to make them allocated on-demand from the heap. This optimises RAM usage for the typical "SPI peripherals in use much fewer than those available in hardware" case.

I think that's the least worst option, having considered the "static array of spi peripheral count" and "put 1 peripheral object inside each SPI object" options. The last would have been cunning, and optimal for the typical "1 device per peripheral" case, but I couldn't figure out how to hand over correctly and safely when an SPI object was deleted.

Does eliminate the need for SPI_COUNT from the HAL, but increases ROM size. Ends up pulling in mutex deletion code in a simple test image, which the static version avoids. Can't see a cunning way around that. (It comes via SPI's virtual destructor).

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Requires fix in #9580.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Extra SPI_COUNT conditional added which should fix those Mbed 2 builds.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 1, 2019

Test run: FAILED

Summary: 1 of 9 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC5
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Some targets report [DEBUG] Errors: Error: L6218E: Undefined symbol spi_get_peripheral_name (referred from BUILD/tests/NRF52_DK/ARM/drivers/SPI.o). . Not certain why only ARMC5 failed.

kjbracey-arm added some commits Jan 23, 2019

SPI upgrade - per-peripheral mutex and GPIO-based SSEL
This commit takes some of the work done on the SPI class from #8445, and
refines it, to provide the per-peripheral mutex functionality.

This also implements GPIO-based SSEL, which exposes a new
select()/deselect() API for users to group transfers, and should work on
every platform (unlike the HAL-based SSEL). This requires users to use a
new constructor to avoid backwards compatibility issues.

To activate the per-peripheral mutex, the HAL must define SPI_COUNT and
provide spi_get_peripheral_name().  (In #8445 this is a reworked
spi_get_module, but the name is changed here to avoid a collision with
existing HALs - this commit is designed to work without wider HAL
changes).

Fixes: #9149
SPIFBlockDevice.h: include SingletonPtr.h
SPIFBlockDevice was using SingletonPtr without an include,
and only getting it via SPI.h.

Spotted while changing SPI to not use SingletonPtr - now
abandoned, but still this shouldn't have been relying on it.
Rename SPI_COUNT to DEVICE_SPI_COUNT
Avoid collision with some HALs that already define SPI_COUNT.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:spi_muxing branch to b12be6b Mar 1, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Some HALs had their own existing SPI_COUNT definition. Moved our new one to DEVICE_SPI_COUNT to avoid collision.

Rebased.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 1, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

CI restarted

@ithinuel
Copy link
Member

left a comment

Aside from the two "style" comment, LGTM

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

This comment has been minimized.

Copy link
@ithinuel

ithinuel Mar 1, 2019

Member

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 1, 2019

Author Contributor

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).


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

This comment has been minimized.

Copy link
@ithinuel

ithinuel Mar 1, 2019

Member

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

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 1, 2019

Test run: FAILED

Summary: 1 of 14 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_tools-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@ithinuel Happy with this PR?

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 1, 2019

@0xc0170 0xc0170 merged commit 7c9a718 into ARMmbed:master Mar 1, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-24 bytes) RAM(+48 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9138 cycles (-1078 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Mar 1, 2019

@stevew817

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@0xc0170 This broke SPI for Silicon Labs targets, as we currently don't allow instantiating NC pins and the new constructor does exactly that.

How was this PR tested?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Expected behaviour is that targets should permit gpio_init with NC, but are free to assert or crash if any real read/write operations are made on the NC pin.

This isn't explicitly documented, as far as I can see, but is implied by the existence of gpio_is_connected:

/* Checks if gpio object is connected (pin was not initialized with NC)
 * @param pin The pin to be set as GPIO
 * @return 0 if port is initialized with NC
 **/
int gpio_is_connected(const gpio_t *obj);

The PR would have been tested on whichever devices we have present and operational in CI - but as I believe we don't have any sort of universal SPI test rigs it would have been exercised in devices using 802.15.4 radio modules and the like, and I guess Silicon Labs targets would not have been represented.

The Silicon Labs targets should be updated to permit NC GPIO initialisation - I would suggest removing the assert from gpio_init, and instead having MBED_ASSERT(gpio_is_connected(obj)) in other calls.

@stevew817

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@kjbracey-arm The other issue is targets using asynchronous SPI. the irq_handler is called from IRQ context, and this PR changed the transaction buffer to be a singletonPtr, see https://github.com/ARMmbed/mbed-os/pull/9469/files#diff-ade6379acb4d9069bc0cc21788bb6883R359

Inside the IRQ handler, dequeue_transaction can be called, which in turn tries to dequeue from the transaction buffer. This will cause failure, since the transaction buffer now needs to lock a mutex to get to the singletonPtr. That evidently won't work from IRQ context...

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

SingletonPtr's mutex lock only occurs on first use, so AFAICT there would only be an issue if the very first asynchronous transaction was started directly from an interrupt handler. Are you doing that?

I guess we could prime the transaction buffer in the constructor to help that case - a call to its get() in _do_construct.

@stevew817

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Priming would be helpful.
Starting an asynch transaction on a timer is not really an exotic use case?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

The fact that async calls can be initiated from interrupt does illustrate a (pre-existing) design problem - the mutex locks for the peripheral are being bypassed, which means it won't work correctly in any sort of environment where there are multiple users sharing one SPI peripheral.

We do need to pin down exactly what the rules about async use are - I believe the guys working on the SPI HAL upgrade feature branch have been looking at this.

Still, for the current environment where asynch is historically lockless, it would make sense to prime it so it continues to work in that scenario. I'll make a patch for that. Will cost a bit of ROM space pulling in the init code for unused asynch, but not a lot.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Thanks @stevew817 for getting back to us with these issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.