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

[RFC, WIP] drivers/periph_spi: improve API of spi_acquire #15904

Closed
wants to merge 4 commits into from

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 1, 2021

Contribution description

Note: only the last commit belongs to this PR, the rest are dependencies.

For specifying the SPI clock rather use a numeric value than a set of predetermined enum values. This has the following advantages:

  • Code portability
    • If currently an unsupported SPI clock is requested, the API mandates spi_acquire() to fail. For sharing code between two MCU types with a different set of supported SPI cocks, this code would have to specify a clock supported by both MCUs
  • Better matching hardware requirements
    • Real SPI attached devices usually support arbitrary slow SPI clocks and only have an upper limit. The new API matches this behavior better and allows specifying the maximum SPI clock without worrying what the used MCU actually supports
  • Future proofing the API
    • Many MCUs supported by RIOT support SPI clocks way faster than 10 MHz, but the API doesn't expose this. Adding more enum values would fix this, but would require touching all implementations. The new API allows the implementations provide as many and as high clock frequencies as they like, without worrying about other implementations
  • Increasing performance
    • Let's say your network devices can work with SPI clocks up to 7 MHz and your MCU supports SPI clocks of 1.5 MHz, 3 MHz, 6MHz and 12 MHz. With the old API, the network device would have to ask for 5 MHz clock and the MCU could only run at 3 MHz when 5 MHz is requested (to remain compatible with devices requesting 5 MHz which run stable at 5 MHz but not with 6 MHz). With the new API, the selected SPI clock would be twice as high in this case.

Returning the actually used frequency can aid in a number of use cases:

  • Bit-Banging
    • The SPI COPI pin can be abused for bit-banging protocols like the ws281x LEDs are using. This allows using the DMA for bit-banging, which can greatly improve responsiveness of the system
  • Better estimations of I/O time
    • A real time system with two external devices connected via the same SPI bus might needs to limit the time a low priority device has access to the bus to fulfill real time requirements. By having good estimations of how long it needs to transfer a byte, the maximum amount of data that can be exchanged with the low priority devices within a single transacting marked by spi_acquire() and spi_release() can be easily computed.

Testing procedure

Read the API. Once this API is agreed upon and all dependencies are merged, I can update the implementations. Once this is done, there something to test :-)

Issues/PRs references

Depends on and includes #15902

@maribu maribu added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Feb 1, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
cpu/sam3/periph/spi.c Outdated Show resolved Hide resolved
@@ -26,6 +26,8 @@
* @}
*/

#include <assert.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This one also have #include "assert.h" on line 34

Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot this one

@github-actions github-actions bot removed Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Sep 2, 2021
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 8, 2021
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 8, 2021
Copy link
Contributor

@hugueslarrive hugueslarrive left a comment

Choose a reason for hiding this comment

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

Many thanks ! Now I m going back to #16727.

Copy link
Contributor

@hugueslarrive hugueslarrive left a comment

Choose a reason for hiding this comment

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

Some MCU have SPI peripherals on different clock bus so we must add a bus parameter to the two new functions.

Unfortunately this also break SPI_CLK_*HZ macros...

hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 8, 2021
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 8, 2021
- adapted to the new API (RIOT-OS#15904)
- arbitrary bus speed support (improves RIOT-OS#16811)
@maribu
Copy link
Member Author

maribu commented Sep 9, 2021

Some MCU have SPI peripherals on different clock bus so we must add a bus parameter to the two new functions.

But since spi_clk_t is opaque and can be defined differently for reach MCU, this could even be a struct if needed.

@maribu maribu closed this Sep 9, 2021
@maribu
Copy link
Member Author

maribu commented Sep 9, 2021

Misclicked

@maribu maribu reopened this Sep 9, 2021
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 11, 2021
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing except kinetis.

nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 13, 2021
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)
@hugueslarrive
Copy link
Contributor

Some MCU have SPI peripherals on different clock bus so we must add a bus parameter to the two new functions.

But since spi_clk_t is opaque and can be defined differently for reach MCU, this could even be a struct if needed.

But spi_clk_t is the output of spi_get_clk() which basically doing something like return a prescaler >= source_clock / target frequency. So it need to know which source_clock to use i.e. on same54-xpro (SAM0_GCLK_PERIPH / 2) for SPI_DEV(0), ( SAM0_GCLK_48MHZ / 2) for SPI_DEV(1), or SAM0_GCLK_MAIN for SPI_DEV(2) (SPI_ON_QSPI). Or on nucleo-f103rb APB2 (72MHz) for SPI_DEV(0) or APB1 (36MHz) for SPI_DEV(1).

@maribu
Copy link
Member Author

maribu commented Sep 21, 2021

#16727 implements already the interface proposed here. Closing it in favor of #16727

@maribu maribu closed this Sep 21, 2021
MrKevinWeiss pushed a commit to MrKevinWeiss/RIOT that referenced this pull request Jul 26, 2022
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)
MrKevinWeiss pushed a commit to MrKevinWeiss/RIOT that referenced this pull request Aug 30, 2022
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jun 29, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jun 30, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

2023-06:
- rebased on current master
- some backports from 2022 RIOT-OS#18374
- 3 new implementations adapted (gd32v, rpx0xx, and esp32)
- minial frequency asserts was replaced by return codes
- useless upper frequency bounding removed from many implementations
- SPI_DIV_UP was replaced by the new DIV_ROUND_UP from macros/math.h
- driver clock configuration caching was removed from implementations
  where it exists because it should be done at application level with
  this new API
- br computation was simplified for stm32 / gd32v as performace
  optimisation is no longer needed at this level and the inaccuracy
  of the fixed point arithmetic was unreliable for frequencies
  requested lower but close to resulting frequencies
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jun 30, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

2023-06:
- rebased on current master
- some backports from 2022 RIOT-OS#18374
- 3 new implementations adapted (gd32v, rpx0xx, and esp32)
- minial frequency asserts was replaced by return codes
- useless upper frequency bounding removed from many implementations
- SPI_DIV_UP was replaced by the new DIV_ROUND_UP from macros/math.h
- driver clock configuration caching was removed from implementations
  where it exists because it should be done at application level with
  this new API
- br computation was simplified for stm32 / gd32v as performace
  optimisation is no longer needed at this level and the inaccuracy
  of the fixed point arithmetic was unreliable for frequencies
  requested lower but close to resulting frequencies
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jul 2, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

2023-06:
- rebased on current master
- some backports from 2022 RIOT-OS#18374
- 3 new implementations adapted (gd32v, rpx0xx, and esp32)
- minial frequency asserts was replaced by return codes
- useless upper frequency bounding removed from many implementations
- SPI_DIV_UP was replaced by the new DIV_ROUND_UP from macros/math.h
- driver clock configuration caching was removed from implementations
  where it exists because it should be done at application level with
  this new API
- br computation was simplified for stm32 / gd32v as performace
  optimisation is no longer needed at this level and the inaccuracy
  of the fixed point arithmetic was unreliable for frequencies
  requested lower but close to resulting frequencies
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jul 12, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

2023-06:
- rebased on current master
- some backports from 2022 RIOT-OS#18374
- 3 new implementations adapted (gd32v, rpx0xx, and esp32)
- minial frequency asserts was replaced by return codes
- useless upper frequency bounding removed from many implementations
- SPI_DIV_UP was replaced by the new DIV_ROUND_UP from macros/math.h
- driver clock configuration caching was removed from implementations
  where it exists because it should be done at application level with
  this new API
- br computation was simplified for stm32 / gd32v as performace
  optimisation is no longer needed at this level and the inaccuracy
  of the fixed point arithmetic was unreliable for frequencies
  requested lower but close to resulting frequencies
hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Jul 12, 2023
tests/periph_spi: printing and testing SPI clock rates

drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904)

drivers/periph_spi: add the `bus` parameter to spi_get_*()
This was necessary for implementations where multiple
devices can have different clock sources. This broke
the macros SPI_CLK_* that were reverted to an enum.

periph/spi: adapted to the new API
Arbitrary speed support was added to all implementations
where it was missing.

2023-06:
- rebased on current master
- some backports from 2022 RIOT-OS#18374
- 3 new implementations adapted (gd32v, rpx0xx, and esp32)
- minial frequency asserts was replaced by return codes
- useless upper frequency bounding removed from many implementations
- SPI_DIV_UP was replaced by the new DIV_ROUND_UP from macros/math.h
- driver clock configuration caching was removed from implementations
  where it exists because it should be done at application level with
  this new API
- br computation was simplified for stm32 / gd32v as performace
  optimisation is no longer needed at this level and the inaccuracy
  of the fixed point arithmetic was unreliable for frequencies
  requested lower but close to resulting frequencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants