drivers: SPI: Adds CS flags from devicetree #26269

JordanYates · 2020-06-18T08:22:01Z

The SPI drivers were previously ignoring the devicetree flags specified for CS pins.
This pull request allows flags such as GPIO_OPEN_SOURCE to be specified and used.
This resolves the issue raised in #26267.

Out of tree drivers that do not setup gpio_dt_flags will retain the same behaviour, as flags of 0x00 corresponds to the old push-pull default.

In the interest of making the diff smaller, ACTIVE_HIGH and ACTIVE_LOW from devicetree are not used to set the CS active levels as this is currently handled in an alternate struct. If it is desirable to use the CS active levels from devicetree, that can be a follow on PR.

tbursztyka

@mnkp Is this the idea you had a few months ago? I did not get it that time, sorry for this. And I overlooked the fact gpio_pin_t is 8bits, so it is possible to do all of this without bloating the struct. Quite the contrary in fact as we end up with 16 unused bits in the end.

drivers/spi/spi_context.h

include/drivers/spi.h

pabigot

Some comments. Only blocker is resolution of whether devicetree active level flags should be respected or ignored; no change requested until we reach consensus.

drivers/spi/spi_context.h

include/drivers/spi.h

pabigot · 2020-06-18T09:43:33Z

include/drivers/spi.h

@@ -129,11 +130,17 @@ extern "C" {
 *    to act as a CS line
 * @param delay is a delay in microseconds to wait before starting the
 *    transmission and before releasing the CS line
+ * @param gpio_dt_flags is the devicetree flags corresponding to how the CS
+ *    line should be driven. GPIO_ACTIVE_LOW and GPIO_ACTIVE_HIGH are ignored
+ *    in favor of SPI_CS_ACTIVE_HIGH and SPI_CS_ACTIVE_LOW options in struct


May need to update if devicetree flags are respected as suggested above.

The original documentation was to that effect. From @tbursztyka's review I gathered transitioning to the devicetree flags for active level wouldn't be possible due to non GPIO CS controllers

See my comment below about an assert that would check SPI_CS_ACTIV_* vs GPIO_ACTIVE_* compliance

pabigot · 2020-06-18T09:47:56Z

drivers/spi/spi_context.h

-		gpio_flags_t flags = spi_context_cs_active_level(ctx);
+		/* ACTIVE_HIGH & ACTIVE_LOW from DT must be ignored */
+		gpio_dt_flags_t flags = ctx->config->cs->gpio_dt_flags &
+					~GPIO_ACTIVE_LOW;


I'm going to disagree with @tbursztyka here, and would ask @mnkp to weigh in.

I propose that the active level flags from devicetree should be respected, as a particular signal may pass through an inverter in hardware. SPI_CS_ACTIVE, via spi_context_cs_{,in}active_value(), should still be used to determine the value passed to gpio_pin_set(), and affect the selection of GPIO_OUTPUT_level in gpio_pin_configure().

AFAICT SPI_CS_ACTIVE_ is only supported with GPIO (in-tree at least) so perhaps it should be deprecated.

We used to have a controller that could manage that internally (an intel controller afaik). I am actually surprised to see that many controllers which have CS lines and are thus managing them, do not provide a way for the user to tweak the CS active mode, forcing the user to switch to a gpio line. Let's keep it at it is.

cs-gpios property attached to spi controller node is there strictly to describe a gpio pin (pin controlled by a GPIO driver) that the SPI driver should use. It cannot be used if the CS pin is to be driven by the SPI module itself. To configure a pin that is driven directly by the SPI module one would have to use pinctrl properties (which we don't support yet).

So in this case we should honor all the supported flags, including GPIO_ACTIVE_LOW. At some point SPI_CS_ACTIVE_ should be deprecated, definitely when we have pinctrl support. And if the following is the case

AFAICT SPI_CS_ACTIVE_ is only supported with GPIO (in-tree at least) so perhaps it should be deprecated.

we could deprecate SPI_CS_ACTIVE_ already.

To configure a pin that is driven directly by the SPI module one would have to use pinctrl properties (which we don't support yet).

Thus let's deprecate once we support pinctrl. It does not make sens to deprecate if there is no replacement already available.

From what I can tell @mnkp and I agree that the devicetree flags should define the base active level for GPIO control of CS. @tbursztyka can you accept this?

Whether SPI_CS_ACTIVE_HIGH is deprecated can be a separate discussion. The only in-tree uses of that flag are in drivers that return an error if it's set, so it doesn't seem worthwhile to make it to affect GPIO CS (overriding the devicetree flags), especially as there's agreement to deprecate once pinmux is fully implemented.

So it looks like there are changes to be made to both implementation and documention. Do we need to bring this to dev-review to try to get agreement on what those changes are?

From what I can tell @mnkp and I agree that the devicetree flags should define the base active level for GPIO control of CS. @tbursztyka can you accept this?

Yes. However SPI_CS_ACTIVE_* should be checked for compliance, so maybe an assert to verify that the gpio dt flags follows the SPI flag would be nice. (@JordanYates could you add this in your PR?)

Do we need to bring this to dev-review to try to get agreement on what those changes are?

This PR is pretty concise by itself and already gathered relevant stakeholders (reviewers list).

Let's bring the deprecation of SPI_CS_ACTIVE_* flags when there will be a pinctrl API. That's unrelated to that PR.

CS active level is now taken from devicetree, with checks that it is equivalent to the SPI_CS_ACTIVE_* value.

pabigot · 2020-06-18T09:52:29Z

drivers/gpio/gpio_mcp23s17.h

@@ -55,6 +55,7 @@ struct mcp23s17_config {
 	const uint32_t freq;
 	const char *const cs_dev;
 	const uint32_t cs_pin;
+	const uint8_t cs_flags;


Since the structure is being modified it would be nice to adjust these to use the (new-ish) canonical types gpio_pin_t and gpio_dt_flags_t (after confirming that usage doesn't require gpio_flags_t). But I'd accept an "out of scope" judgment from the contributor.

When going through the various drivers, I think I found 4 different ways that the SPI buses are handled through configuration. I would prefer to leave that out of this pull request, but wholeheartedly support a cleanup effort.

Actually, your comments make me think there is an actual issue in these drivers: it would be worth modifying these so instead of setting custom attributes in a custom *_config structure, that they would directly initialize their dedicated spi_cs_control attribute in their private data. That would lower the size of their *_config structure, and avoid unnecessary copy of these values.

That would be for another PR, nothing to be fixed here.

mnkp

@mnkp Is this the idea you had a few months ago?

Yes, the fact that SPI drivers don't fully honor cs-gpios property defined in DTS breaks the pattern (and the promise we make to the users) so this should be considered a bug.

mnkp · 2020-06-19T12:04:59Z

drivers/spi/spi_context.h

-		gpio_flags_t flags = spi_context_cs_active_level(ctx);
+		/* ACTIVE_HIGH & ACTIVE_LOW from DT must be ignored */
+		gpio_dt_flags_t flags = ctx->config->cs->gpio_dt_flags &
+					~GPIO_ACTIVE_LOW;


cs-gpios property attached to spi controller node is there strictly to describe a gpio pin (pin controlled by a GPIO driver) that the SPI driver should use. It cannot be used if the CS pin is to be driven by the SPI module itself. To configure a pin that is driven directly by the SPI module one would have to use pinctrl properties (which we don't support yet).

So in this case we should honor all the supported flags, including GPIO_ACTIVE_LOW. At some point SPI_CS_ACTIVE_ should be deprecated, definitely when we have pinctrl support. And if the following is the case

AFAICT SPI_CS_ACTIVE_ is only supported with GPIO (in-tree at least) so perhaps it should be deprecated.

we could deprecate SPI_CS_ACTIVE_ already.

mbolivar-nordic · 2020-06-19T18:27:00Z

Was this closed on purpose?

JordanYates · 2020-06-20T00:17:47Z

It was not, not sure how I managed that...

JordanYates · 2020-06-21T23:12:56Z

As far as I can tell, the CI failures are unrelated to this PR.

pabigot · 2020-06-22T12:57:07Z

@JordanYates Could you squash the first two commits? They really seem to belong together, especially since the second modifies what the first did.

JordanYates · 2020-06-22T13:09:47Z

@JordanYates Could you squash the first two commits? They really seem to belong together, especially since the second modifies what the first did.

Done

mbolivar-nordic · 2020-06-22T16:14:55Z

At least some of the CI warnings-turned-errors seem relevant:

/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/spi/spi_context.h: In function 'spi_context_cs_configure':
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/spi/spi_context.h:165:19: error: unused variable 'dt_level' [-Werror=unused-variable]
  165 |   gpio_dt_flags_t dt_level = ctx->config->cs->gpio_dt_flags &
      |                   ^~~~~~~~
/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/drivers/spi/spi_context.h:164:19: error: unused variable 'ctx_level' [-Werror=unused-variable]
  164 |   gpio_dt_flags_t ctx_level = spi_context_cs_active_level(ctx);
      |                   ^~~~~~~~~
cc1: all warnings being treated as errors

Add an additional option to the spi_cs_control struct that records how the pin has been configured in devicetree. For drivers that are not updated, the CS behaviour is the same as before (Push-Pull). Use the devicetree knowledge with the GPIO subsystem so that the correct physical pin levels for the CS pin are automatically selected. Fixes #26267 Signed-off-by: Jordan Yates <[email protected]>

Adds the chip select devicetree flags to the spi_cs_control instance. Signed-off-by: Jordan Yates <[email protected]>

JordanYates · 2020-06-23T07:44:35Z

At least some of the CI warnings-turned-errors seem relevant:

Issues have been resolved.

mbolivar-nordic · 2020-07-01T20:00:30Z

Can someone please merge? The buildkite failures look spurious to me; they're the usual timing related false negatives

galak · 2020-07-01T21:40:18Z

Merged by hand as the CI failures are not related to this PR at all.

jfischer-no · 2020-07-06T12:04:14Z

Who will fix all the broken boards and shields?

JordanYates · 2020-07-06T14:17:27Z

I didn't realize it at the time, but the late decision to use the active level from devicetree rather than the struct spi_control does indeed change the default behavior of the SPI drivers when the flags are not provided. As it wasn't part of the original pull request, I didn't make any modifications to boards etc.

vanwinkeljan · 2020-07-06T19:19:51Z

Could we revert this PR as it breaks some of the shields?

pabigot · 2020-07-06T19:31:27Z

Could, but it's also fairly easy to fix:

git grep -wl cs-gpios -- '*/*.dts*' \
  | xargs sed -i -r -e '/cs-gpios/s@ 0>@ GPIO_ACTIVE_LOW>@g'
git grep -wl cs-gpios -- '*/*.overlay' \
  | xargs sed -i -r -e '/cs-gpios/s@ 0>@ GPIO_ACTIVE_LOW>@g'

does 90% of the work.

I'll do a PR for that then we can see which way to proceed.

pabigot · 2020-07-06T19:43:26Z

does 90% of the work.

No, it only handles devicetree. All the drivers that receive those flags also need to be updated, in part because we haven't addressed #23691.

I'd still rather see this fixed than revert.

MaureenHelm · 2020-07-06T20:14:42Z

There's also a problem with tests/drivers/spi/spi_loopback

jfischer-no · 2020-07-07T08:34:36Z

Could we revert this PR as it breaks some of the shields?

#26670 fixes display shields, I had no time to go through the leftover I am not familiar with.

It is not always a flag in device-tree, probably there are still few drivers to adapt.

The pull request zephyrproject-rtos#26269 drivers: SPI: Adds CS flags from devicetree missed out on the APA102 led_strip driver, most likely due to the driver not utilising the chip select signal until zephyrproject-rtos#25310 drivers: led_strip: Add support for external SPI CS on APA102 LED strips Signed-off-by: Roman Vaughan <[email protected]>

The pull request #26269 drivers: SPI: Adds CS flags from devicetree missed out on the APA102 led_strip driver, most likely due to the driver not utilising the chip select signal until #25310 drivers: led_strip: Add support for external SPI CS on APA102 LED strips Signed-off-by: Roman Vaughan <[email protected]>

JordanYates requested review from avisconti, jukkar, karstenkoenig, MaureenHelm, mnkp, nashif, nvlsianpu, pabigot, pfalcon, tbursztyka and vanwinkeljan as code owners June 18, 2020 08:22

zephyrbot added area: Sensors Sensors area: SPI SPI bus area: API Changes to public APIs labels Jun 18, 2020

tbursztyka requested changes Jun 18, 2020

View reviewed changes

drivers/spi/spi_context.h Outdated Show resolved Hide resolved

drivers/spi/spi_context.h Outdated Show resolved Hide resolved

include/drivers/spi.h Outdated Show resolved Hide resolved

include/drivers/spi.h Outdated Show resolved Hide resolved

pabigot requested changes Jun 18, 2020

View reviewed changes

galak requested a review from mbolivar-nordic June 19, 2020 01:43

JordanYates closed this Jun 19, 2020

mnkp reviewed Jun 19, 2020

View reviewed changes

JordanYates reopened this Jun 20, 2020

pabigot approved these changes Jun 22, 2020

View reviewed changes

JordanYates requested a review from tbursztyka June 23, 2020 00:22

Jordan Yates added 2 commits June 23, 2020 11:51

display: ili9340: Add chip select flags

c9523ee

Adds the chip select devicetree flags to the spi_cs_control instance. Signed-off-by: Jordan Yates <[email protected]>

Jordan Yates added 3 commits June 23, 2020 11:51

sensor: lsm6dsl: Add chip select flags

8dc7e8b

Adds the chip select devicetree flags to the spi_cs_control instance. Signed-off-by: Jordan Yates <[email protected]>

sensor: ms5607: Add chip select flags

812f36b

Adds the chip select devicetree flags to the spi_cs_control instance. Signed-off-by: Jordan Yates <[email protected]>

wifi: winc1500: Add chip select flags

ed797df

Adds the chip select devicetree flags to the spi_cs_control instance. Signed-off-by: Jordan Yates <[email protected]>

mbolivar-nordic approved these changes Jun 23, 2020

View reviewed changes

tbursztyka approved these changes Jun 24, 2020

View reviewed changes

galak merged commit ab3e778 into zephyrproject-rtos:master Jul 1, 2020

JordanYates deleted the spi_cs_c99 branch July 1, 2020 23:09

andysan mentioned this pull request Jul 5, 2020

Fix SPI CS handling in LoRa radios #26653

Merged

pabigot mentioned this pull request Jul 6, 2020

spi-device: provide CS gpio flags and update drivers to use them #26684

Merged

jfischer-no mentioned this pull request Jul 7, 2020

LPC55xxx SPI fixes #26371

Merged

nandojve mentioned this pull request Jul 7, 2020

drivers: ieee802154: rf2xx: Add CS gpio flags from DT #26718

Merged

lochej mentioned this pull request Jul 31, 2020

drivers: bluetooth: hci: spi: CS DT config not working because CS gpio_dt_flags are not set in the spi_cs_config struct #27280

Closed

pabigot mentioned this pull request Aug 6, 2020

doc: releases: 2.4: note SPI gpio CS active level config change #27413

Merged

NZSmartie mentioned this pull request Sep 10, 2020

drivers: led_strip: APA102 use CS flags from devicetree #28257

Merged

Marco-Peter mentioned this pull request Oct 14, 2020

driver: ssd1306: Add chip select flags for SPI #29186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: SPI: Adds CS flags from devicetree #26269

drivers: SPI: Adds CS flags from devicetree #26269

JordanYates commented Jun 18, 2020 •

edited

Loading

tbursztyka left a comment

pabigot left a comment

pabigot Jun 18, 2020

JordanYates Jun 18, 2020

tbursztyka Jun 22, 2020

pabigot Jun 18, 2020 •

edited

Loading

tbursztyka Jun 18, 2020

mnkp Jun 19, 2020

tbursztyka Jun 19, 2020

pabigot Jun 22, 2020

tbursztyka Jun 22, 2020 •

edited

Loading

JordanYates Jun 22, 2020

pabigot Jun 18, 2020

JordanYates Jun 18, 2020

tbursztyka Jun 18, 2020

mnkp left a comment

mnkp Jun 19, 2020

mbolivar-nordic commented Jun 19, 2020

JordanYates commented Jun 20, 2020 •

edited

Loading

JordanYates commented Jun 21, 2020

pabigot commented Jun 22, 2020

JordanYates commented Jun 22, 2020

mbolivar-nordic commented Jun 22, 2020

JordanYates commented Jun 23, 2020 •

edited

Loading

mbolivar-nordic commented Jul 1, 2020

galak commented Jul 1, 2020

jfischer-no commented Jul 6, 2020

JordanYates commented Jul 6, 2020

vanwinkeljan commented Jul 6, 2020

pabigot commented Jul 6, 2020

pabigot commented Jul 6, 2020

MaureenHelm commented Jul 6, 2020

jfischer-no commented Jul 7, 2020 •

edited

Loading

drivers: SPI: Adds CS flags from devicetree #26269

drivers: SPI: Adds CS flags from devicetree #26269

Conversation

JordanYates commented Jun 18, 2020 • edited Loading

tbursztyka left a comment

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar-nordic commented Jun 19, 2020

JordanYates commented Jun 20, 2020 • edited Loading

JordanYates commented Jun 21, 2020

pabigot commented Jun 22, 2020

JordanYates commented Jun 22, 2020

mbolivar-nordic commented Jun 22, 2020

JordanYates commented Jun 23, 2020 • edited Loading

mbolivar-nordic commented Jul 1, 2020

galak commented Jul 1, 2020

jfischer-no commented Jul 6, 2020

JordanYates commented Jul 6, 2020

vanwinkeljan commented Jul 6, 2020

pabigot commented Jul 6, 2020

pabigot commented Jul 6, 2020

MaureenHelm commented Jul 6, 2020

jfischer-no commented Jul 7, 2020 • edited Loading

JordanYates commented Jun 18, 2020 •

edited

Loading

pabigot Jun 18, 2020 •

edited

Loading

tbursztyka Jun 22, 2020 •

edited

Loading

JordanYates commented Jun 20, 2020 •

edited

Loading

JordanYates commented Jun 23, 2020 •

edited

Loading

jfischer-no commented Jul 7, 2020 •

edited

Loading