drivers: spi_ll_stm32: Get SPI configuration from DT #5582

ydamigos · 2018-01-06T19:03:57Z

Get SPI_BASE_ADDRESS, SPINAME, SPIIRQ and SPI_IRQ_PRI from DT.

codecov-io · 2018-01-06T19:32:24Z

Codecov Report

Merging #5582 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #5582   +/-   ##
======================================
  Coverage    51.3%   51.3%           
======================================
  Files         440     440           
  Lines       42232   42232           
  Branches     8055    8055           
======================================
  Hits        21667   21667           
  Misses      20046   20046           
  Partials      519     519

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 2b0821c...53c1014. Read the comment docs.

mbolivar

Thanks for the patches.

I've got some comments/questions.

mbolivar · 2018-01-08T17:30:59Z

dts/bindings/spi/spi.yaml

What does this mean? Why are SPI and I2C in a parent/child relationship here?

I used i2c.yaml as a template and I didn't remove it. Fixed.

mbolivar · 2018-01-08T17:32:21Z

dts/bindings/spi/spi.yaml

New to DT here, but I'm not sure from this description what should go into the clocks array.

Also I don't see any clocks properties in any of the real device nodes defined later, even though this binding is category: required. What's wrong here?

New to DT here, but I'm not sure from this description what should go into the clocks array.

We should add a property in the SPI node to enable the SPI peripheral clock. Like it is done in u(s)art nodes (e.g. clocks = <&rcc STM32_CLOCK_BUS_APB2 0x00000020>). I used the same description which is used in uart and i2c yaml files. I plan to create PRs to add the clock property in SPI and I2C nodes for STM32 SOCs.

Also I don't see any clocks properties in any of the real device nodes defined later, even though this binding is category: required. What's wrong here?

The python scripts which extracts the defines from the dts files does not check and force the required properties. Maybe, @erwango could share more info on this.

Ack. #5460 was raised to track this issue.

Yup, aware of the issue with not enforcing the check; my question is if it is really needed in this patch series since the clocks properties are not present in the SPI nodes. (And sorry if this is well understood from DT context).

From your example <&rcc STM32_CLOCK_BUS_APB2 0x00000020>, the contents are SoC specific (in this case, a phandle to the RCC node, parent clock bus ID, and bit mask).

Do we really need this property defined in the generic spi.yaml then, if the contents are always going to be opaque?

Yup, aware of the issue with not enforcing the check; my question is if it is really needed in this patch series since the clocks properties are not present in the SPI nodes. (And sorry if this is well understood from DT context).

I removed the clocks property because it isn't needed in this patch series.

Do we really need this property defined in the generic spi.yaml then, if the contents are always going to be opaque?

I don't know how the SPI peripheral is implemented in each vendor's SOC. We may need to define clocks property in a vendor,spi.yaml file.

Makes sense to me

mbolivar · 2018-01-08T17:33:41Z

dts/bindings/spi/st,stm32-spi.yaml

There is at least one important difference in the SPI IP blocks found on the different families, namely whether or not a FIFO is available for I/O or if just a single register (the DR) can be used. Does that need to be reflected here?

(I'm not saying it does, just asking)

Thanks for the pointer. It should. I added a st,stm32-spi-fifo.yaml.

mbolivar · 2018-01-08T17:35:14Z

dts/bindings/spi/st,stm32-spi.yaml

Isn't this redundant since this inherits spi.yaml?

mbolivar · 2018-01-08T17:38:07Z

dts/arm/st/stm32f1.dtsi

Where are SPI2 (40003800) and SPI3 (40003c00)? Other SPI peripherals present on some SoCs in other families appear missing as well.

It looks like you might be putting just the "least common denominator" in the stm32fX.dtsi DTSI files, and filling in additional peripherals in stm32fXYZ.dtsi for the SoC. If that's right, I guess there's more follow-up work to do to add the missing SPI nodes for other SoCs, right?

Yes, I added the "least common denominator" in the stm32fX.dtsi and filled the additional SPI nodes in stm32fXYZ.dtsi only to the SOCs where boards use the additional SPI. We could add the rest SPI nodes in stm32fXYZ.dtsi when we add SPI support to other boards.

If this is the case (complete set of SPI instances to be provided in a later commit when required) please indicate the scope of current commit in the description.

mbolivar · 2018-01-08T17:53:32Z

boards/arm/96b_carbon/96b_carbon.dts

I think you can / should delete the interrupts properties from the board files. You've already defined them at SOC level, which is where they belong.

mbolivar · 2018-01-08T17:55:06Z

samples/drivers/led_lpd8806/boards/96b_carbon.conf

The commit log does not explain why these should be removed; can you explain why?

Presumably if an application sets a value specifically, it may be a necessary override of the default. Did you check to make sure you're just removing default settings?

If we use DT to get the *_IRQ_PRI, we can't override it using Kconfig. We need to use a BOARD_NAME.overlay file.

Presumably if an application sets a value specifically, it may be a necessary override of the default. Did you check to make sure you're just removing default settings?

You are right. I didn't check if this value is just a default value or it is necessary for the application. I don't have the hardware to check it, so I will try to ask codeowner for review.

Presumably if an application sets a value specifically, it may be a necessary override of the default. Did you check to make sure you're just removing default settings?

You are right. I didn't check if this value is just a default value or it is necessary for the application. I don't have the hardware to check it, so I will try to ask codeowner for review.

And the samples codeowner is you. I apologize for not checking it with you first. If the value is necessary, I will update the PR with the needed overlay files.

I went back and looked at the wire protocols for these drivers.

I think this lpd8806 sample is OK to use the SoC-provided IRQ PRI value (5), but I'm less sure what to do with the ws2812 sample.

(The ws2812 driver plays tricks with SPI to implement a one-wire real-time protocol, so the sample will probably work, but giving higher priority to the SPI IRQ and driver thread is important for applications with multiple tasks.)

This is a personal time driver for me so I will test tonight after work, but my guess is the default will be OK, but I'll need to add extra documentation for this driver about adjusting the IRQ priority using device tree overlays after your series goes in.

@ydamigos I didn't make time to test, but I am OK with moving these to the default SoC provided values since these drivers don't have many users right now. I think I need to send those documentation patches about configuring the IRQ priority no matter what. If I run into problems, I'll add overlays to the samples myself. Thanks for the pointer on how to fix any issues.

erwango

Some comments on IRQ prio
Otherwise, this is a good work, thanks!

erwango · 2018-01-09T10:45:34Z

dts/bindings/spi/spi.yaml

Ack. #5460 was raised to track this issue.

erwango · 2018-01-09T10:55:18Z

dts/arm/st/stm32f0.dtsi

SPI IRQ prio is generally not 0.
Was 0 put here on purpose and meant to be modified in boards dts?

Yes, I set SPI IRQ prio to zero in SOC's dtsi file and meant to override it on application level. Should I change the default value in SOCs level to something else if the SPI IRQ prio is generally not 0? I would like to avoid setting the default SPI IRQ prio in boards level.

erwango · 2018-01-09T10:56:20Z

boards/arm/96b_carbon/96b_carbon.dts

If you don't modify IRQ prio here, it will be 0 instead of 5 as set in Kconfig.defconfig

I configured default SPI IRQ prio to 5 on SOC level.

erwango · 2018-01-09T10:57:34Z

boards/arm/disco_l475_iot1/disco_l475_iot1.dts

Same here, spi3 IRQ pri is defaultly set to 0 instead of 10

I configured default SPI IRQ prio to 5 on SOC level.

erwango · 2018-01-09T11:00:30Z

dts/arm/st/stm32f1.dtsi

If this is the case (complete set of SPI instances to be provided in a later commit when required) please indicate the scope of current commit in the description.

superna9999 · 2018-01-09T12:59:53Z

dts/arm/st/stm32f0.dtsi

Add a space right before the = in all files changed:
label= "SPI_1";
to
label = "SPI_1";

ydamigos · 2018-01-11T10:17:19Z

I pushed an update to just fix some commits with empty bodies after the decision in yesterday's TSC meeting.

galak · 2018-01-13T20:03:00Z

@erwango you good with this?

galak · 2018-01-13T20:04:47Z

Can we look at adding a spi device to any of these device trees?

galak · 2018-01-13T20:08:06Z

What's the difference between st,stm32-spi-fifo.yaml & st,stm32-spi?

ydamigos · 2018-01-14T11:28:49Z

What's the difference between st,stm32-spi-fifo.yaml & st,stm32-spi?

I used to yaml file to show the difference between SPI IP blocks found on the different families, which could have a FIFO available for I/O or just a single register (the DR).

Can we look at adding a spi device to any of these device trees?

I plan to add spi device support in a follow-up PR.

Add yaml files to DT for initial support of STM32 SPI Origin: original Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

This commits adds the "least common denominator" in the stm32fX.dtsi files and fills the additional SPI nodes in stm32fXYZ.dtsi files, only for the SOCs where boards use the additional SPI peripheral. We could add the rest SPI nodes in the stm32fXYZ.dtsi files when we add SPI support to other boards. Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

Get SPI_*_BASE_ADDRESS, SPI_*_NAME, SPI_*_IRQ and SPI_*_IRQ_PRI from DT. Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

Configure SPI using DT for the following STM32 boards: 96b_carbon disco_l475_iot1 Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

Remove SPI_*_IRQ_PRI from tests and samples. Using DT to get the *_IRQ_PRI, we can't override it using Kconfig. If needed, use a BOARD_NAME.overlay file to override default values. Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

ydamigos · 2018-01-14T11:57:13Z

I just added license header to the new spi yaml files.

dwagenk · 2018-01-19T10:46:05Z

drivers/spi/Kconfig

 	string
 	prompt "SPI port 1 device name"
-	depends on SPI_1
+	depends on SPI_1 && !HAS_DTS_SPI


@ydamigos
I know this PR is merged already, but I have a followup question:
why didn't SPI_0_NAME (same for SPI 4-6) get the same dependency?
I see you changed only the parts necessary for stm32 (SPI_1 to SPI_3), but wouldn't it be better to keep KConfig consistent in these parts?

@dwagenk As you mentioned, the PR focused on adding DT support for the STM32 boards. We could add the dependencies to the rest SPI_x when they start to use DT in different PRs.

ydamigos requested review from erwango, mbolivar and tbursztyka as code owners January 6, 2018 19:03

ydamigos force-pushed the dt-stm32-spi branch 2 times, most recently from 45e1f25 to dd257a4 Compare January 7, 2018 14:20

mbolivar reviewed Jan 8, 2018

View reviewed changes

ydamigos force-pushed the dt-stm32-spi branch from dd257a4 to a0b0f49 Compare January 9, 2018 09:34

erwango requested changes Jan 9, 2018

View reviewed changes

ydamigos force-pushed the dt-stm32-spi branch 2 times, most recently from 5c3da68 to 5d28fee Compare January 9, 2018 12:49

superna9999 reviewed Jan 9, 2018

View reviewed changes

superna9999 mentioned this pull request Jan 9, 2018

disk: add a SDHC card over SPI driver. #5528

Closed

3 tasks

ydamigos force-pushed the dt-stm32-spi branch from 5d28fee to 18a8219 Compare January 9, 2018 13:13

superna9999 added area: SPI SPI bus area: ARM ARM (32-bit) Architecture platform: STM32 ST Micro STM32 labels Jan 9, 2018

galak added the area: Devicetree label Jan 9, 2018

ydamigos force-pushed the dt-stm32-spi branch 2 times, most recently from a674f66 to 725578a Compare January 11, 2018 10:12

ydamigos force-pushed the dt-stm32-spi branch from 725578a to 16c17d1 Compare January 12, 2018 09:12

superna9999 approved these changes Jan 12, 2018

View reviewed changes

mbolivar approved these changes Jan 12, 2018

View reviewed changes

ydamigos closed this Jan 14, 2018

ydamigos reopened this Jan 14, 2018

ydamigos added 5 commits January 14, 2018 13:55

dts: bindings: Add yaml files for STM32 SPI support to DT

ef3bb76

Add yaml files to DT for initial support of STM32 SPI Origin: original Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

drivers: spi_ll_stm32: Get SPI configuration from DT

51fcdfe

Get SPI_*_BASE_ADDRESS, SPI_*_NAME, SPI_*_IRQ and SPI_*_IRQ_PRI from DT. Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

boards: arm: STM32 boards use DT to configure SPI

6d4d089

Configure SPI using DT for the following STM32 boards: 96b_carbon disco_l475_iot1 Signed-off-by: Yannis Damigos <giannis.damigos@gmail.com>

ydamigos force-pushed the dt-stm32-spi branch from 16c17d1 to 53c1014 Compare January 14, 2018 11:55

erwango approved these changes Jan 15, 2018

View reviewed changes

ydamigos mentioned this pull request Jan 17, 2018

dts: add SPI device support #5704

Merged

galak merged commit 3d1115a into zephyrproject-rtos:master Jan 17, 2018

ydamigos deleted the dt-stm32-spi branch January 17, 2018 17:54

miyatsu mentioned this pull request Jan 18, 2018

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

Merged

dwagenk reviewed Jan 19, 2018

View reviewed changes

dwagenk mentioned this pull request Jan 26, 2018

spi: stm32: convert remaining boards that support SPI to using dts #5836

Closed

8 tasks

drivers: spi_ll_stm32: Get SPI configuration from DT #5582

drivers: spi_ll_stm32: Get SPI configuration from DT #5582

Uh oh!

Conversation

ydamigos commented Jan 6, 2018

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

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!

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!

codecov-io commented Jan 6, 2018 •

edited

Loading