Skip to content

Conversation

@tbursztyka
Copy link
Contributor

@tbursztyka tbursztyka commented Feb 13, 2018

Hello,

While testing something on SPI that required to use stm32's, and since I had these nucleos, I enabled the SPI ports relevantly there.
I could successfully test nucleo_f103rb and nucleo_f334r8

@erwango My nucleo_f030r8 st-link acting weirdly as you know I could not test yet (I'll do you trick of using an external st-link). But I believe it should be fine. The only thing which I have a doubt about is the NSS pinmuxing: should it has a pull up?

@erwango
Copy link
Member

erwango commented Feb 13, 2018

@tbursztyka : You might want to rebase on #5883.
For nucleo_f030r8, I'll do the test

@tbursztyka tbursztyka changed the title St spi on nucleos Enabling SPI ports on some nucleos Feb 13, 2018
@codecov-io
Copy link

codecov-io commented Feb 13, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6172     +/-   ##
=========================================
- Coverage   53.15%   52.86%   -0.3%     
=========================================
  Files         412      412             
  Lines       40139    40284    +145     
  Branches     7732     7802     +70     
=========================================
- Hits        21337    21296     -41     
- Misses      15667    15773    +106     
- Partials     3135     3215     +80
Impacted Files Coverage Δ
kernel/device.c 55% <0%> (-35.48%) ⬇️
tests/kernel/mem_slab/mslab/src/slab.c 77.96% <0%> (-15.59%) ⬇️
tests/kernel/workq/work_queue/src/main.c 93.6% <0%> (-5.78%) ⬇️
kernel/thread.c 75.88% <0%> (-2.79%) ⬇️
subsys/net/lib/app/init.c 66.66% <0%> (-2.78%) ⬇️
subsys/net/lib/app/server.c 25.25% <0%> (-2.03%) ⬇️
kernel/init.c 74.07% <0%> (-1.37%) ⬇️
kernel/work_q.c 80% <0%> (-0.36%) ⬇️
subsys/net/lib/app/net_app.c 20.26% <0%> (-0.26%) ⬇️
subsys/net/lib/app/client.c 48.47% <0%> (-0.24%) ⬇️
... and 50 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 76b262c...04b8505. Read the comment docs.

@tbursztyka
Copy link
Contributor Author

@erwango thx, missed that pr. there is most probably redundant things then

Copy link
Contributor

@ydamigos ydamigos left a comment

Choose a reason for hiding this comment

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

@tbursztyka I will check it again when you rebase it on #5883.

Copy link
Contributor

Choose a reason for hiding this comment

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

SPI3 is not available on stm32f103xb. You should move it inside stm32f103Xe.dtsi and stm32f107.dtsi.

Copy link
Contributor

Choose a reason for hiding this comment

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

spi2 is not present on all f0 SoCs so it shouldn't be in this place.
I'd propse adding an stm32f030Xc.dtsi and a stm32f030X8.dtsi file and adding the node there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought changing the status to "ok" in relevant board would be sufficient. (one that support only spi1, or both etc...), since the base address seems to be the same (well, as you noticed below: if f09x or f07x has spi2 support but with different base address then splitting in relevant files will be necessary)

Copy link
Contributor

Choose a reason for hiding this comment

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

there's been some discussion on this in #5883, eg this: #5883 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did what you proposed yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't looked into it, but are they present on other f0 SoCs as well? f091, f072?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I only looked at f030x specs. We can add support for f09x, f07x later

@tbursztyka
Copy link
Contributor Author

Rebased on top of #5883 and updated : I removed 2 patches that were then unnecessary.

@tbursztyka tbursztyka force-pushed the st_spi_on_nucleos branch 2 times, most recently from 0b764c5 to 5764db0 Compare February 15, 2018 07:34
@tbursztyka
Copy link
Contributor Author

recheck

@dwagenk
Copy link
Contributor

dwagenk commented Feb 16, 2018

@tbursztyka rebase on #5883 again. There were some CI problems solved yesterday by #6223

Copy link
Contributor

@ydamigos ydamigos left a comment

Choose a reason for hiding this comment

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

Right now, enabling SPIx port in dts is not enough. You should also enable SPIx ports in board's Kconfig.defconfig file for boards with STM32 SoC.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also enable SPI_1 port in board's Kconfig.defconfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also enable SPI_1 port in board's Kconfig.defconfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@tbursztyka
Copy link
Contributor Author

@ydamigos I did not do it on purpose: why enabling a port by default if it's not in use? I'd rather let the user do that, depending on his need.

@ydamigos
Copy link
Contributor

@tbursztyka The user might get confused to see them enabled in the dts file. For example, spi_loopback compiles fine but it won't work on the board.

@tbursztyka
Copy link
Contributor Author

@ydamigos ok, well after users should know what they are doing so if they have unused spi port there, up to them to disable these. Will update

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in #6238, SPI irq priority cannot be 5.

Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Tomasz Bursztyka added 4 commits February 20, 2018 16:00
This feature is available on STM32F030x8 and STM32F030xC.

Signed-off-by: Tomasz Bursztyka <[email protected]>
Other variants exist.

Signed-off-by: Tomasz Bursztyka <[email protected]>
Current configuration was choosen to avoid colliding with existing ones
for i2c and uart, but others can be used (see
drivers/pinmux/stm32/stm32_f0.h)

Signed-off-by: Tomasz Bursztyka <[email protected]>
Current configuration was choosen to avoid colliding with existing ones
for i2c and uart, but others can be used.

Signed-off-by: Tomasz Bursztyka <[email protected]>
@tbursztyka
Copy link
Contributor Author

@ydamigos can you review it again? Sounds like it's good to go (hopefully)

@galak galak dismissed dwagenk’s stale review February 20, 2018 18:26

Issues addressed

@galak galak merged commit 82e832f into zephyrproject-rtos:master Feb 20, 2018
@tbursztyka tbursztyka deleted the st_spi_on_nucleos branch March 23, 2018 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants