Add spi_get_peripheral_name() to stm_spi #9845

jarlamsa · 2019-02-26T07:53:19Z

This is to have support for per-peripheral mutex introduced in #9469
Together fixes an issue seen in #9149

Depends on: #9469

Background:
The issue has been seen while testing Wi-SUN with mbed-cloud-client while using SPI storage. The tested devices have been NUCLEO_F429ZI. If we add new targets for the Wi-SUN testing, the HAL-changes are needed to be added there also.

Description

Pull request type

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

Reviewers

@kjbracey-arm @bulislaw @ithinuel

Release Notes

ciarmcom · 2019-02-26T08:00:25Z

@jarlamsa, thank you for your changes.
@bulislaw @kjbracey-arm @ithinuel @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2019-02-26T08:08:55Z

Could you clarify a little on the background here - why this platform, specifically? (This is pre-empting wider changes including adding this facility to all platforms in #8445.)

jarlamsa · 2019-02-26T08:28:25Z

Could you clarify a little on the background here

Added some background to description.

jeromecoutant · 2019-02-26T09:45:28Z

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/PeripheralNames.h

@@ -57,6 +57,7 @@ typedef enum {
    UART_8 = (int)UART8_BASE
 } UARTName;

+#define SPI_COUNT 6


I think this addition is not needed for this PR ?

It wasn't until yesterday.
#9469 introduced the static array (it was put back yesterday), so it is needed.

#9469 is not merged.

No it is not, but as said in the description, this PR depends on #9469

Then all the STM targets need to be updated, why updating F429ZI only ?

Background is stated on the description. Other STM targets would work like they would before like stated in the #9469

No, it means that targets that don't provide this extra facility retain the existing single whole-system SPI mutex.

But why would this change be valid for WISUN and F429 only ? Does the change have drawbacks that we don't want to deploy everywhere ? If there are only advantages and no drawbacks, I'd prefer to have the change deployed widely. We try to keep all STM targets behavior as aligned as possible otherwise maintenance becomes more complex.

AFAIK there are mostly advantages, bit larger memory footprint because of the static array for peripherals is a drawback though. The changes done now are preceding a major change to SPI that are coming in a future release. (That will effectively overwrite the changes done now)

Wi-SUN testing has been mainly done with F429, so that is why it is the first one to get changes. Changes to different HAL's can be added later on if there is a need.

ok then, but when the "major changes" are made, all targets should be aligned.

LMESTM · 2019-02-26T13:19:38Z

targets/TARGET_STM/stm_spi_api.c

@@ -92,6 +92,16 @@ void init_spi(spi_t *obj)
    }
 }

+SPIName spi_get_peripheral_name(PinName mosi, PinName miso, PinName sclk) {
+    SPIName spi_mosi = (SPIName)pinmap_peripheral(mosi, PinMap_SPI_MOSI);
+    SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);


In case of "3 wires SPI" , miso is set as "NC" - so this needs to be taken into account.
Please add check, if miso is NC, only merge mosi and sclk

pinmap_merge should handle a situation when other pin is set as NC, but I'll provide also a check here.

I think this needs more change. The check should happen before calling to
SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);

Otherwise, the system would crash here:
https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L78

If miso is NC, should it even get there?
https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L73

You're right - so that's OK - thanks !

This is to have support for per-peripheral mutex introduced in ARMmbed#9469 Together fixes an issue seen in ARMmbed#9149

kjbracey · 2019-02-28T08:27:15Z

Actually this doesn't depend on the previous PR, at least for merging.

jarlamsa · 2019-02-28T08:31:19Z

Actually this doesn't depend on the previous PR, at least for merging

True, the previous PR only justifies the changes made in this.
Can the CI be started for this?

LMESTM · 2019-02-28T08:41:26Z

targets/TARGET_STM/stm_spi_api.c

@@ -92,6 +92,16 @@ void init_spi(spi_t *obj)
    }
 }

+SPIName spi_get_peripheral_name(PinName mosi, PinName miso, PinName sclk) {
+    SPIName spi_mosi = (SPIName)pinmap_peripheral(mosi, PinMap_SPI_MOSI);
+    SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);


I think this needs more change. The check should happen before calling to
SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);

Otherwise, the system would crash here:
https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L78

LMESTM · 2019-02-28T08:42:08Z

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/PeripheralNames.h

@@ -57,6 +57,7 @@ typedef enum {
    UART_8 = (int)UART8_BASE
 } UARTName;

+#define SPI_COUNT 6


Then all the STM targets need to be updated, why updating F429ZI only ?

cmonr · 2019-02-28T22:23:35Z

Can the CI be started for this?

@jarlamsa Since this depends on #9469, we need it to come in first before this PR can be started.

jarlamsa · 2019-03-01T07:18:25Z

Since this depends on #9469, we need it to come in first before this PR can be started.

Basically yes, but as @kjbracey-arm stated:

Actually this doesn't depend on the previous PR, at least for merging.

0xc0170 · 2019-03-01T07:40:34Z

CI started

mbed-ci · 2019-03-01T09:44:43Z

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

0xc0170 · 2019-03-01T09:54:38Z

@kjbracey-arm Shall this go in as it is?

kjbracey · 2019-03-01T10:53:42Z

Yes, I think this is fine as-is. If the other PR needed a change, it could be done in that PR.

Fix issue ARMmbed#9149. Port changes from ARMmbed#9845 also to targets: K64F, K66F, KW24D and KW41Z

Fix issue #9149. Port changes from #9845 also to targets: K64F, K66F, KW24D and KW41Z

ciarmcom requested review from bulislaw, ithinuel, kjbracey and a team February 26, 2019 08:00

ciarmcom added the needs: review label Feb 26, 2019

kjbracey approved these changes Feb 26, 2019

View reviewed changes

jeromecoutant reviewed Feb 26, 2019

View reviewed changes

LMESTM suggested changes Feb 26, 2019

View reviewed changes

kjbracey added the needs: preceding PR label Feb 26, 2019

Add spi_get_peripheral_name() to stm_spi

a9f0924

This is to have support for per-peripheral mutex introduced in ARMmbed#9469 Together fixes an issue seen in ARMmbed#9149

jarlamsa force-pushed the stm_spi_peripheral_name branch from 9377d16 to a9f0924 Compare February 27, 2019 11:33

cmonr added release-version: 5.12.0-rc1 risk: R labels Feb 28, 2019

LMESTM suggested changes Feb 28, 2019

View reviewed changes

LMESTM approved these changes Feb 28, 2019

View reviewed changes

cmonr added needs: CI and removed needs: review labels Feb 28, 2019

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

0xc0170 removed the risk: R label Mar 1, 2019

0xc0170 merged commit 857cd9f into ARMmbed:master Mar 1, 2019

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

artokin pushed a commit to artokin/mbed-os that referenced this pull request Mar 11, 2019

Add spi_get_peripheral_name to MCUEpresso spi_api

e1deb88

Fix issue ARMmbed#9149. Port changes from ARMmbed#9845 also to targets: K64F, K66F, KW24D and KW41Z

artokin mentioned this pull request Mar 11, 2019

Port spi_get_peripheral_name fix to some Freescale boards #10033

Merged

cmonr pushed a commit that referenced this pull request Mar 12, 2019

Add spi_get_peripheral_name to MCUEpresso spi_api

c7c0af8

Fix issue #9149. Port changes from #9845 also to targets: K64F, K66F, KW24D and KW41Z

teetak01 mentioned this pull request Aug 14, 2019

Wi-SUN causing file corruption and assert erros on LittleFS #11218

Closed

Add spi_get_peripheral_name() to stm_spi #9845

Add spi_get_peripheral_name() to stm_spi #9845

Uh oh!

Conversation

jarlamsa commented Feb 26, 2019 • edited by cmonr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Feb 26, 2019

Uh oh!

kjbracey commented Feb 26, 2019

Uh oh!

jarlamsa commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Feb 28, 2019

Uh oh!

jarlamsa commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Feb 28, 2019

Uh oh!

jarlamsa commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

mbed-ci commented Mar 1, 2019

Uh oh!

0xc0170 commented Mar 1, 2019

Uh oh!

kjbracey commented Mar 1, 2019

Uh oh!

Uh oh!

jarlamsa commented Feb 26, 2019 •

edited by cmonr

Loading

jarlamsa commented Feb 26, 2019 •

edited

Loading