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 spi_get_peripheral_name() to stm_spi #9845

Merged
merged 1 commit into from Mar 1, 2019

Conversation

Projects
None yet
8 participants
@jarlamsa
Copy link
Contributor

commented Feb 26, 2019

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 ciarmcom requested review from bulislaw, ithinuel, kjbracey-arm and ARMmbed/mbed-os-maintainers Feb 26, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

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

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Could you clarify a little on the background here

Added some background to description.

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

#define SPI_COUNT 6

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Feb 26, 2019

Contributor

I think this addition is not needed for this PR ?

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Feb 26, 2019

Contributor

#9469 is not merged.

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 28, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 28, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

@@ -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);

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 27, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 27, 2019

Author Contributor

Done

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

This comment has been minimized.

Copy link
@jarlamsa

jarlamsa Feb 28, 2019

Author Contributor

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

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

@jarlamsa jarlamsa force-pushed the jarlamsa:stm_spi_peripheral_name branch to a9f0924 Feb 27, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

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

@jarlamsa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

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?

@@ -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);

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

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

#define SPI_COUNT 6

This comment has been minimized.

Copy link
@LMESTM

LMESTM Feb 28, 2019

Contributor

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

@LMESTM

LMESTM approved these changes Feb 28, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

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.

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

@jarlamsa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 1, 2019

Test run: SUCCESS

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

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

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

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

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

@0xc0170 0xc0170 merged commit 857cd9f 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(+0 bytes) RAM(+0 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 10478 cycles (+29 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

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

Add spi_get_peripheral_name to MCUEpresso spi_api
Fix issue ARMmbed#9149.

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

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

Add spi_get_peripheral_name to MCUEpresso spi_api
Fix issue #9149.

Port changes from #9845 also
to targets: K64F, K66F, KW24D and KW41Z
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.