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.

Sign up for GitHub

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

Jump to bottom

cpu/stm32f1: optimize SPI driver #5921

Merged
merged 1 commit into from
Oct 19, 2016
Merged

cpu/stm32f1: optimize SPI driver #5921

merged 1 commit into from
Oct 19, 2016

Conversation

PeterKietzmann
Copy link
Member

  • first commit will disappear soon
  • second commit is ugly but I won't find time to adapt all other stm32 SPI drivers right now even tough I need this improvement on stm32f1 MCUs
    -this PR needs further testing (that's why I set the WIP label). Support is happily welcome!

@PeterKietzmann PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Oct 10, 2016
@PeterKietzmann PeterKietzmann changed the title cpu/stm32f: optimize SPI driver cpu/stm32f1: optimize SPI driver Oct 11, 2016
@PeterKietzmann
Copy link
Member Author

I've tested this PR with gnrc_networking on iotlab-m3 boards and between one samr21-xpro and one iotlab-m3 board. Pinging 1000 times with 1000 Bytes payload and without delay never lead to packet losses of more than 1%, independent who has pinged.

@PeterKietzmann
Copy link
Member Author

With this PR the inter-byte delay can be reduced from 800ns to ~440ns

lebrush
lebrush suggested changes Oct 11, 2016
View reviewed changes
cpu/stm32f1/periph/spi.c
transferred++;
while (!(spi->SR & SPI_SR_RXNE)) {}
in[i] = (char)spi->DR;
transferred++;
Copy link
Member

Choose a reason for hiding this comment

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

Is it desired to increase transferred twice when you are sending and receiving? If not, you could use transferred (or a renamed version of it) as a counter for the loops.

cpu/stm32f1/periph/spi.c
}
else {
DEBUG("\nout: %x in: was nullPointer transferred: %x\n", out, transferred);
DEBUG("\nSPI: 'in' NullPointer. Transferred %i Bytes\n", transferred);
}
#endif /*ENABLE_DEBUG */
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can merge those two now that you only have a parameter to print and remove the if

@kaspar030
Copy link
Contributor

kaspar030 commented Oct 11, 2016 via email

@PeterKietzmann
Copy link
Member Author

Addressed. Actually I was quite confused about that as well but decided to leave it as is at that time.

@PeterKietzmann
Copy link
Member Author

Added a commit to improve "read only" speed

haukepetersen
haukepetersen reviewed Oct 12, 2016
View reviewed changes
cpu/stm32f1/periph/spi.c
@@ -275,20 +288,23 @@ void spi_poweroff(spi_t dev)
switch (dev) {
#if SPI_0_EN
case SPI_0:
while ((SPI_0_DEV->SR & SPI_SR_BSY)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Really needed? IMHO the device can not be busy when this function is called, as it is only busy during transfer and it is check for not being busy when we leave that function. Also the device is locked when using _transfer() -> so it can not be busy here. Makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see it critical as well. I've added it for safety reasons. In the referance manual page 721 sec. 25.3.8 Disabling the SPI it is said "Then wait until BSY=0"

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@PeterKietzmann PeterKietzmann mentioned this pull request Oct 12, 2016
Merged
@miri64 miri64 added this to the Release 2016.10 milestone Oct 17, 2016
haukepetersen
haukepetersen approved these changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

ACK

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 17, 2016
@haukepetersen
Copy link
Contributor

I removed the WIP, was that ok?

@PeterKietzmann
Copy link
Member Author

Wait I need to squash first and revert the first commit. Need to wait for #5886.

@PeterKietzmann
Copy link
Member Author

Rebased and squashed

@PeterKietzmann
Copy link
Member Author

@haukepetersen I see many green lights

lebrush
lebrush reviewed Oct 18, 2016
View reviewed changes
cpu/stm32f1/periph/spi.c
}
else {
DEBUG("\nout: %x in: was nullPointer transferred: %x\n", out, transferred);
DEBUG("\nSPI: transferred %i Bytes\n", transferred);
Copy link
Member

Choose a reason for hiding this comment

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

One last comment (sorry @PeterKietzmann), we could actually get rid of transferred since at this point transferred will always be length. Its super minor but would save 4 bytes :-)

@PeterKietzmann
Copy link
Member Author

@lebrush fine, did that

@haukepetersen
Copy link
Contributor

Everyone is happy -> go

@haukepetersen haukepetersen merged commit f6fbee5 into RIOT-OS:master Oct 19, 2016
@lebrush
Copy link
Member

lebrush commented Oct 19, 2016

Wait! you left transferred variable defined and returning always 0 ...

kaspar030
kaspar030 reviewed Oct 19, 2016
View reviewed changes
cpu/stm32f1/periph/spi.c
{

SPI_TypeDef *spi;
int transferred = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

yup, this needs to be int transferred = 0 = length;

PeterKietzmann added a commit to PeterKietzmann/RIOT that referenced this pull request Oct 19, 2016
@PeterKietzmann
cpu/stm32f1: remove leftover from RIOT-OS#5921
f57e185
@PeterKietzmann PeterKietzmann mentioned this pull request Oct 19, 2016
Merged
@PeterKietzmann
Copy link
Member Author

Sorry. Never do such things in a hurry (like exactly now). See #5966

miri64 added a commit that referenced this pull request Oct 19, 2016
@miri64
Merge pull request #5966 from PeterKietzmann/opt_stm32f1_spi_leftover
2c3a6e4 
cpu/stm32f1: remove leftover from #5921
neiljay pushed a commit to neiljay/RIOT that referenced this pull request Nov 4, 2016
@PeterKietzmann
cpu/stm32f1: remove leftover from RIOT-OS#5921
e6cfc70
@PeterKietzmann PeterKietzmann deleted the opt_stm32f1_spi branch January 17, 2017 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lebrush lebrush lebrush requested changes

@kaspar030 kaspar030 kaspar030 left review comments

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees

@haukepetersen haukepetersen

Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2016.10
Development

Successfully merging this pull request may close these issues.
5 participants
@PeterKietzmann @kaspar030 @haukepetersen @lebrush @miri64