drivers: spi_ll_stm32: Enable SPI driver for F1 family #5413

ydamigos · 2017-12-15T21:25:20Z

Enables SPI driver for STM32F1 SoCs.

Tested on olimexino_stm32 using spi_loopback test.

Fixes #1281 and #3850.

mbolivar

Some questions

mbolivar · 2017-12-16T02:33:09Z

boards/arm/olimexino_stm32/olimexino_stm32_defconfig

How is that SPI related? If the interrupt driver requires CONFIG_POLL=y, maybe it should select it.

I just copy/paste some CONFIG_* from the spi_loopback prj.conf and forget to remove it. I will fix it.

mbolivar · 2017-12-16T02:36:31Z

include/dt-bindings/pinctrl/stm32-pinctrlf1.h

Shouldn't master NSS be output push pull?

According to RM0008 NSS can be configured as

Hardware master /slave -> Input floating/ Input pull-up / Input pull-down Hardware master/ NSS output enabled -> Alternate function push-pull Software -> Not used. Can be used as a GPIO

I just added the first option to support multi master bus.

Since this part is a bit tricky, it might be worse adding some comments.
You could also provide definitions for NSS output enabled, even if not used.
It might help user to understand what he need.
Exact reference to User manual (§25.3.1: Slave select (NSS) pin management) would also help.

mbolivar · 2017-12-16T02:38:29Z

drivers/spi/spi_ll_stm32.c

Why does this remain outside the new if statement, instead of moving inside? Similar questions below.

I thought it should but it broke spi_loopback test. If the spi_context_update_rx() is not called, it will never move to the second rx_buf. I still try to understand why we use a null rx_buf in spi_loopback.

I propose to move spi_context_update_rx() inside if and push another commit which will substitute null rx_bufs with dummy_rx_bufs in spi_loopback test. How does that sound to you?

Interesting, so the .buf = NULL inside of the rx_bufs definitions (like in spi_rx_half_end()) in the spi_loopback test is breaking things, you mean?

If so, perhaps this is a bug in the SPI core? Haven't had time to look into it in detail, just to be clear.

Interesting, so the .buf = NULL inside of the rx_bufs definitions (like in spi_rx_half_end()) in the spi_loopback test is breaking things, you mean?

Yes, it causes bus error on some SoCs (e.g. stm32f103, stm32f334).

Thank you for your response!

@tbursztyka, could the test possibly be structuring its buffers incorrectly from what you meant?

If not, perhaps this is a bug in the STM32 area.

dbkinder

+1 for docs

erwango

Some minor comments otherwise looks good

erwango · 2017-12-20T12:26:39Z

drivers/spi/spi_ll_stm32.c

Why not using && data->ctx.rx_buf as you did for tx?
It makes both change and code lighter.
I guess this is a leftover from initial commit. Latest code allows you to do it now.

I'll fix it.

erwango · 2017-12-20T12:26:54Z

drivers/spi/spi_ll_stm32.c

I'll fix it.

erwango · 2017-12-20T12:27:00Z

drivers/spi/spi_ll_stm32.c

I'll fix it.

erwango · 2017-12-20T12:27:05Z

drivers/spi/spi_ll_stm32.c

I'll fix it.

erwango · 2017-12-20T13:00:04Z

include/dt-bindings/pinctrl/stm32-pinctrlf1.h

Since this part is a bit tricky, it might be worse adding some comments.
You could also provide definitions for NSS output enabled, even if not used.
It might help user to understand what he need.
Exact reference to User manual (§25.3.1: Slave select (NSS) pin management) would also help.

ydamigos · 2017-12-20T17:37:40Z

include/dt-bindings/pinctrl/stm32-pinctrlf1.h

@erwango Is this clear enough? STM32_PIN_SPI_MASTER_NSS_OUTPUT_ENABLED is too long.

Yes this is fine, thanks

ydamigos · 2018-01-03T11:00:51Z

recheck

mbolivar · 2018-01-04T20:22:02Z

(FTR, my questions on this PR are all addressed. Not adding a +1 as I haven't looked at it in detail)

superna9999 · 2018-01-09T13:21:34Z

All this NULL buffers should be clarified, apart from that it's OK for me.

superna9999 · 2018-01-11T10:54:24Z

I'd like to have @tbursztyka review the changes around these NULL buffers check, the spi loopback test is running correctly on F0, L4 and F4.

codecov-io · 2018-01-12T09:39:10Z

Codecov Report

Merging #5413 into master will decrease coverage by 3.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5413      +/-   ##
==========================================
- Coverage   54.46%   51.29%   -3.17%     
==========================================
  Files         458      441      -17     
  Lines       43413    42268    -1145     
  Branches     8307     8063     -244     
==========================================
- Hits        23644    21683    -1961     
- Misses      19628    20066     +438     
- Partials      141      519     +378

Impacted Files	Coverage Δ
tests/benchmarks/sys_kernel/src/syskernel.h	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/memmap_b.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/receiver.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/mailbox_b.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/sema_b.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/sys_kernel/src/sema.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/mutex_b.c	`0% <0%> (-100%)`	⬇️
include/crypto/cipher.h	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/fifo_r.c	`0% <0%> (-100%)`	⬇️
tests/benchmarks/app_kernel/src/fifo_b.c	`0% <0%> (-100%)`	⬇️
... and 181 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2151b86...ef67f98. Read the comment docs.

dwagenk · 2018-01-23T08:57:40Z

it's not working for me.

I added SPI conf to stm32f3_disco and experienced hard fault problems as described in issue SPI fails on Nucleo_f334r8 #3850 when running spi loopback test.
Worked when I applied the patches from this pull request until the last update about 1 hour ago.
updated to current code in this PR
it hangs forever (no hard fault anymore).

***** BOOTING ZEPHYR OS v1.10.99 - BUILD: Jan 23 2018 08:29:17 *****
[general] [INF] main: SPI test on buffers TX/RX 0x20001344/0x20000000
[general] [DBG] spi_async_call_cb: Polling...
[general] [INF] spi_complete_loop: Start
[general] [INF] spi_complete_loop: Passed
[general] [INF] spi_rx_half_start: Start
[general] [INF] spi_rx_half_start: Passed
[general] [INF] spi_rx_every_4: Start

Problem seems to be, that spi_context_update_rx is not called when buffer is empty. See in code comment in my review

ydamigos · 2018-01-23T09:19:01Z

@dwagenk Could you try it again? It should be fixed now.

dwagenk · 2018-01-23T09:46:59Z

@ydamigos it's working now. Thanks

b0661 · 2018-01-23T17:42:47Z

ext/hal/st/stm32cube/CMakeLists.txt

This is not necessary anymore after PR #4646 is merged. Works for me without.

ydamigos · 2018-01-23T18:28:07Z

I rebased it to resolve some conflicts.

Enables SPI driver for STM32F1 SoCs Signed-off-by: Yannis Damigos <[email protected]>

Enable SPI1 port on olimexino_stm32. Signed-off-by: Yannis Damigos <[email protected]>

TX/RX buffer may be NULL, so check them before use. Signed-off-by: Yannis Damigos <[email protected]>

ydamigos · 2018-01-30T11:35:26Z

Now that PR #5785 is merged, I rebased it against master and removed the DNM flag.

Please review it again.

erwango

Tested ok on nucleo_f103rb

galak · 2018-01-31T14:36:47Z

@dwagenk still have issues with this?

dwagenk · 2018-02-01T06:49:11Z

no, ydamigos solved those problems right away!

DWagenk commented 9 days ago
ydamigos it's working now. Thanks

Issues addressed

ydamigos requested review from dbkinder, erwango, mbolivar and tbursztyka as code owners December 15, 2017 21:25

ydamigos force-pushed the stm32f1-spi branch from 5b9fdb1 to 9c62b21 Compare December 15, 2017 22:01

ydamigos mentioned this pull request Dec 15, 2017

spi_ll_stm32 driver does not support stm32f1soc #1281

Closed

mbolivar reviewed Dec 16, 2017

View reviewed changes

ydamigos force-pushed the stm32f1-spi branch from 9c62b21 to 596bd63 Compare December 16, 2017 16:09

dbkinder previously approved these changes Dec 18, 2017

View reviewed changes

ydamigos dismissed dbkinder’s stale review via 8577922 December 18, 2017 17:55

ydamigos force-pushed the stm32f1-spi branch from c142511 to 8577922 Compare December 18, 2017 17:55

erwango requested changes Dec 20, 2017

View reviewed changes

ydamigos force-pushed the stm32f1-spi branch from 8577922 to 4ffb815 Compare December 20, 2017 17:34

ydamigos commented Dec 20, 2017

View reviewed changes

ydamigos mentioned this pull request Dec 21, 2017

spi: stm32: Correctly apply CPOL and CPHA settings #5462

Merged

ydamigos force-pushed the stm32f1-spi branch from 4ffb815 to 1326a41 Compare December 21, 2017 19:05

ydamigos mentioned this pull request Dec 30, 2017

Stm32f1 spi #5506

Closed

galak added the platform: STM32 ST Micro STM32 label Jan 9, 2018

ydamigos force-pushed the stm32f1-spi branch from 1326a41 to 1361304 Compare January 11, 2018 10:25

ydamigos requested a review from superna9999 as a code owner January 11, 2018 10:25

galak force-pushed the arm branch 2 times, most recently from ee5b12f to f95e5fd Compare January 11, 2018 22:07

ydamigos force-pushed the stm32f1-spi branch from 1361304 to 2634bf5 Compare January 12, 2018 09:16

galak added area: ARM ARM (32-bit) Architecture area: ADC Analog-to-Digital Converter (ADC) labels Jan 13, 2018

ydamigos dismissed erwango’s stale review via 0c0058b January 23, 2018 09:15

ydamigos force-pushed the stm32f1-spi branch from e796681 to 0c0058b Compare January 23, 2018 09:15

galak force-pushed the arm branch from 6b38c09 to ff730a4 Compare January 23, 2018 14:54

b0661 reviewed Jan 23, 2018

View reviewed changes

ydamigos force-pushed the stm32f1-spi branch from 0c0058b to 759b741 Compare January 23, 2018 18:27

b0661 mentioned this pull request Jan 23, 2018

drivers: pinctrl device interface (Linux-like) and STM32F0 driver #5799

Closed

ydamigos mentioned this pull request Jan 24, 2018

drivers/spi: Properly check for rx/tx and buffering on #5785

Merged

galak force-pushed the arm branch from ff730a4 to d5216a5 Compare January 29, 2018 19:42

ydamigos force-pushed the stm32f1-spi branch from 759b741 to 5f4a34e Compare January 30, 2018 07:22

erwango mentioned this pull request Jan 30, 2018

dts: spi: add more dts nodes and fixup defines, convert boards to using dts #5883

Merged

ydamigos added 3 commits January 30, 2018 13:22

drivers: spi_ll_stm32: Enable SPI driver for F1 family

00ef394

Enables SPI driver for STM32F1 SoCs Signed-off-by: Yannis Damigos <[email protected]>

boards: arm: olimexino_stm32: Enable SPI1 port

c7a170b

Enable SPI1 port on olimexino_stm32. Signed-off-by: Yannis Damigos <[email protected]>

drivers: spi_ll_stm32: Protect against null TX/RX buffer

ef67f98

TX/RX buffer may be NULL, so check them before use. Signed-off-by: Yannis Damigos <[email protected]>

ydamigos changed the base branch from arm to master January 30, 2018 11:29

ydamigos force-pushed the stm32f1-spi branch from 5f4a34e to ef67f98 Compare January 30, 2018 11:30

ydamigos removed the DNM This PR should not be merged (Do Not Merge) label Jan 30, 2018

erwango approved these changes Jan 31, 2018

View reviewed changes

tbursztyka approved these changes Jan 31, 2018

View reviewed changes

superna9999 approved these changes Jan 31, 2018

View reviewed changes

galak approved these changes Jan 31, 2018

View reviewed changes

galak merged commit a3474a2 into zephyrproject-rtos:master Feb 1, 2018

ydamigos deleted the stm32f1-spi branch February 3, 2018 18:06

Uh oh!

drivers: spi_ll_stm32: Enable SPI driver for F1 family #5413

drivers: spi_ll_stm32: Enable SPI driver for F1 family #5413

Uh oh!

Conversation

ydamigos commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbolivar left a comment

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!

ydamigos Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

erwango left a comment

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!

ydamigos commented Jan 3, 2018

Uh oh!

mbolivar commented Jan 4, 2018

Uh oh!

superna9999 commented Jan 9, 2018

Uh oh!

superna9999 commented Jan 11, 2018

Uh oh!

codecov-io commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dwagenk commented Jan 23, 2018

Uh oh!

ydamigos commented Jan 23, 2018

Uh oh!

dwagenk commented Jan 23, 2018

ydamigos commented Dec 15, 2017 •

edited

Loading

ydamigos Dec 16, 2017 •

edited

Loading

codecov-io commented Jan 12, 2018 •

edited

Loading