You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/06/01 12:37:10 UTC
[GitHub] [incubator-nuttx] jlaitine opened a new pull request #1165: STM32H7 Spi corrections
jlaitine opened a new pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165
## Summary
Here are fixes for the two remaining issues which I have had with stm32h7 spi driver:
1. TX DMA completes before the data has been actually sent out from the SPI fifo. This is expected,
but the exchange should actually wait for spi tx to finish instead of dma to the fifo to finish. This replaces the dma completion event with the SPI TXC event for detecting end of transmission.
The problem comes visible when using simplex-tx mode, where we don't have any rx dma. In normal exchange, the rx dma completion automatically means that also tx has completed.
2. Enabling SPI RX dma is now done also for simplex TX, which is wrong. Also there are code paths (trigger) where enabling RX and TX buffers is not done at all. So, I re-arranged the TXDMAEN and RXDMAEN bit setting.
3. Enabling / disabling the SPI block causes glitches on the bus, which breaks some of my sensors. Luckily, there is an easy solution for this; SPI_CFG2_AFCNTR keeps the state of the signals.
## Impact
## Testing
Tested on 2 different SPI sensors connected to an stm32h7 board + simplex rx/tx communication between two stm32h7 boards. Tested with both in-driver buffers enabled and disabled and on 2 separate SPI busses.
Also tested having two spi devices with different clock speeds connected to the same bus and periodically using both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r433945279
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
The "To Squash or not to Squash" test for me is if it will fail git bisect and does it touch multiple sections of the system. The Squash will reflect the commits with all the comment blocks. I would rebase and fix up (F) the change I requested and sqaush (S) the rest. The PR will live on as the record. master will pass a git bisect. Change set is atomic and can be back ported.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-637583827
I sure can, but why squash? Isn't it nicer to have separate changes as separate commits?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 merged pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-637964216
> @jlaitine please fix the commit message arch/arm/src/stm32h7/stm32h7_spi.c: -> arch/arm/src/stm32h7/stm32_spi.c:
Thanks for pointing this out.
I have now squashed the fixes into one, please check if this is now how you intended. The commit message became quite large. I will modify the commit(s) further according to your instructions if needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-638183148
Great! Thanks again for all the effort with reviewing and testing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r433809648
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
@jlaitine Do you want to squash all the commits and force push?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-638220944
@jlaitine it is a pleasure reviewing your code. Thank you for your diligence and value added communication. It should be a goal for all of the project to have this level of descriptive comments in changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r434016051
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
I am not quite sure why you see this as an one atomic change; maybe I have accidentally mangled the commits together while cherry picking and resolving conflicts accross my messy branches.
Anyhow, I am ok to squash them as you wish, I'll do it as soon as I open my computer next time, at latest tomorrow morning. Please wait with merging until I get there :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r433795441
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
```suggestion
/* TX transmission must be completed before shutting down the SPI */
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-638140782
Waiting on rerun of CI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine removed a comment on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine removed a comment on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-637583827
I sure can, but why squash? Isn't it nicer to have separate changes as separate commits?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r434016051
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
I am not quite sure why you see this as an ole atomic change; maybe I have accidentally mangled the commits together while cherry picking and resolving conflicts accross my messy branches.
Anyhow, I am ok to squash them as you wish, I'll do it as soon as I open my computer next time, at latest tomorrow morning. Please wait with merging until I get there :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on a change in pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on a change in pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#discussion_r433931473
##########
File path: arch/arm/src/stm32h7/stm32_spi.c
##########
@@ -2114,6 +2109,11 @@ static void spi_exchange(FAR struct spi_dev_s *dev, FAR const void *txbuffer,
* bits in the SPI_CFG1 register, if DMA Tx and/or DMA Rx are used.
*/
+ /* TX transmission must be completed before shuttind down the SPI */
Review comment:
I sure can, but why squash? Isn't it nicer to have separate changes as separate commits?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #1165: STM32H7 Spi corrections
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #1165:
URL: https://github.com/apache/incubator-nuttx/pull/1165#issuecomment-636837106
Please, @davids5 comment / test this before merging
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org