device tree support gap for SPI chip select

[pabigot]

It appears Zephyr has incomplete support for SPI devices under device tree: specifically identification of the corresponding chip-select.

In Linux the chip select is provided through the device tree reg property, generally as an integer, presumably a globally ordinal pin/GPIO index selecting one of a small set of bus-specific CS signals.

Issue #7463 suggests Zephyr's original design is similar, and in Zephyr's spi-device.yaml the reg property is declared as "Chip select address of device" (with a type array, though I can find nothing that explains what array means and how it differs from compound).

This design appears to support SOCs like Kinetis where the bus controller owns the chip-selects, which might be mapped to GPIOs through a pinmux feature. For SOCs like Nordic where the chip-select is a device-specific GPIO controlled by spi-cs-control an integer reg property value must instead be interpreted as a global GPIO index used to derived the GPIO device and relative pin number.

E.g. Nordic's nrfx HAL might select between GPIO_0 and GPIO_1 based on whether a pin index is greater than 31, but that's below the Zephyr level. For some boards like nrf52_pca20020 the chip-select might be on an IO extender, increasing the maximum potential global index from 47 to 63.

Alternatively a cs-gpios property may be to be added to the device node, which is probably a far simpler approach mostly compatible with existing dts tooling. A global ordinal value reg unique to the device might still be added, but would not contribute to code generation if cs-gpios is present.

This issue blocks support for the SPI NOR serial memory devices on the Particle development boards as well as nrf52840_pca10056, as both CS models need to be supported in the driver.

[vanwinkeljan]

Duplicate of #7463, which is assigned to @erwango

[pabigot]

Thanks for linking, #7463 is definitely relevant. I don't think this is explicitly duplicative, though, and have revised the description here to more clearly identify the gap and suggest a solution.

[galak]

I'm not follow what the issue with gpio based CS is. I know we've had an issue w/regards to supporting multiple devices on the SPI bus from a DTS code generation point of view. However I'm pretty sure we support at least a single device with its CS controlled by a GPIO.

[pabigot]

@galak The issue is Zephyr's device-tree YAML specifies that the chip select for a spi-device is identified with an ordinal provided in the reg property. For SoCs/boards that allow arbitrary GPIOs to be used for SPI chip-select signals and that multiple GPIO devices it has to be specified with a gpios compound, preferably named cs-gpios.

In Zephyr, the only cs-gpios property is at the bus level, not the device level. The chip-select for a specific device does not belong at the bus level when the SOC SPI supports arbitrary GPIOs for chip select, even if there's only one device on the bus in a given hardware configuration. As I recall DT binding macros don't get generated for things that aren't described in the YAML.

Is that more clear? If not, then it'll hopefully become clear when a few of my other PRs finally get merged or dropped and I can start providing some of the enhancements that are blocked by them, including ones that require this issue be resolved.

[galak]

It seems like a few different issues here. Lets first focus on how this should be described things in DTS first. We are trying to stay in sync with the general DTS bindings used by Linux. So the CS should be described in the SPI controller/bus node, not the device.

[pabigot]

No, I'm not saying remove it from there: it probably has to be there, and probably needs to allow for multiple members, to support SOCs where SPI has a hard-coded set of GPIOs that can be pinmuxed into a chip select role. But it also has to be in the device.

[galak]

No, I'm not saying remove it from there: it probably has to be there, and probably needs to allow for multiple members, to support SOCs where SPI has a hard-coded set of GPIOs that can be pinmuxed into a chip select role. But it also has to be in the device.

Why does it also need to be in the device? Why can't the device access cs-gpios in the parent and index it by reg?

[pabigot]

There's one SPI device in the base, and a second in an overlay. They have different GPIOs. How do you specify the set in the bus node?

[galak]

If you put the overlay aside you'd have:

cs-gpios = <&gpiod 13 0>, <&gpioe 0 0>;

[pabigot]

If you put the overlay aside you'd have:

cs-gpios = <&gpiod 13 0>, <&gpioe 0 0>;

Yes, you would. Assuming that generates something that can be used in the driver (which is a whole other conversation). But in the general case you can't put aside the overlay, especially for prototyping where it'll be in the application source rather than a shield.

Another way to handle these type of buses is to go ahead and use the ordinal, leave the bus cs-gpios unpopulated, and provide API to convert the ordinal specified by the device to a pair of gpio-device and pin (approach outlined in the initial comment). That's significantly harder, even if you reject support for overlays that add gpio devices (yes, that is a real use case).

Please don't get hung up on trying to exactly match Linux. Linux probably doesn't target systems like Nordic nRF5 where the SPI hardware is completely decoupled from managing chip select so any GPIO might have to be supported, including ones that are on an IO extender rather than the SOC.

Regardless, this is a real gap. I'm happy to delay further bikeshedding until there's something concrete to use as the basis of discussion, or until code generation gets started and we need a resolution, whichever comes first.

[galak]

Yes, you would. Assuming that generates something that can be used in the driver (which is a whole other conversation). But in the general case you can't put aside the overlay, especially for prototyping where it'll be in the application source rather than a shield.

I'm not trying to ignore it, I'm trying to see where the real issue is. Is it a limitation in tooling, in code, in dts itself. I want us to focus effort on fixing the issue in the right place, and not working around a limitation of a tool.

Another way to handle these type of buses is to go ahead and use the ordinal, leave the bus cs-gpios unpopulated, and provide API to convert the ordinal specified by the device to a pair of gpio-device and pin (approach outlined in the initial comment). That's significantly harder, even if you reject support for overlays that add gpio devices (yes, that is a real use case).

Please don't get hung up on trying to exactly match Linux. Linux probably doesn't target systems like Nordic nRF5 where the SPI hardware is completely decoupled from managing chip select so any GPIO might have to be supported, including ones that are on an IO extender rather than the SOC.

I'm not hung up on it, its just we shouldn't invent something new. Linux has cases I believe similar to nordic in which the CS management has nothing to do w/the SPI controller.

Regardless, this is a real gap. I'm happy to delay further bikeshedding until there's something concrete to use as the basis of discussion, or until code generation gets started and we need a resolution, whichever comes first.

Agreed, I think one gap is w/regards to DTC and how overlay's can specify modifications. I'm thinking we need additional support to manipulate array's.

[pabigot]

I tend to think it's a limitation in the way Zephyr's using device tree, i.e. through build-time processing producing values that get hard-coded into driver data structures. It might go away with code generation. It probably doesn't exist when the drivers can process the tree directly.

At this point I'm not persuaded by the proposal to add array manipulation operations to the device tree source language, but let's see what the upstream response is.

It wouldn't surprise me if Linux has similar SPI cases, I just wasn't able to find them with git grep and obvious keywords. If there's existing practice that we can re-use, it certainly should be preferred.

[galak]

I'm not sure how any patches or code in Linux will be relevant. The point is that the DTS in linux support systems where the CS are completely driven by general GPIO pins. So we will utilize that DTS standard for how we describe CS in Zephyr.

[pabigot]

I just tried this and found when I change from one cs-gpios compound to two I get these diffs:

-#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER   "GPIO_0"
-#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS        0
-#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN          30
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER_0 "GPIO_0"
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER_1 "GPIO_0"
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS_0      0
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS_1      0
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN_0        30
+#define DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN_1        31

-#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_CONTROLLER   DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER
-#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_FLAGS        DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS
-#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_PIN          DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_CONTROLLER_0 DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER_0
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_CONTROLLER_1 DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_CONTROLLER_1
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_FLAGS_0      DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS_0
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_FLAGS_1      DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_FLAGS_1
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_PIN_0        DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN_0
+#define DT_NORDIC_NRF_SPI_SPI_1_CS_GPIOS_PIN_1        DT_NORDIC_NRF_SPI_40004000_CS_GPIOS_PIN_1

I.e. the name associated with the first instance changed. I don't think that should happen, since the every driver would have to support two ways to reference the first controller. (My preference is to always make the instance explicit, but always leaving the first instance unqualified would be acceptable too.)

[galak]

Should be addressed by #12647

[pabigot]

I've come to agree that the chip-select GPIOs be attached to the bus node rather than the device, painful though that is to do in an overlay, and the device should use an index into the cs-gpios property value.

But I don't think the existing SPI API is ready to support that consistently. Instead of having a struct spi_cs_control *cs field in struct spi_config it should be able to access that pointer from the corresponding SPI device with some function like

const struct spi_cs_control *spi_get_cs_control(device *dev, u16_t slave);

which would return a non-null pointer only if there is a GPIO that needs to be manually controlled.

I'm afraid that might be too much to change before LTS, though I also think it's worth considering.

[pabigot]

Below is from #12647 (comment) which is being fast-tracked. After further thought, I think the point that we're going to intentionally (eventually) change the one complete stable API in Zephyr based on conformance to the Linux SPI device tree description is worth at least making explicit, if not reconsidering.

So I agree w/you that really the APIs around SPI should change and the handling of GPIOs used for CS should be done in the bus controller driver and not specified in each "slave"/client driver. However, I didn't want to take on changing the API at this time.

I've gotta say, I'm getting a bit of whiplash trying to figure out how Zephyr architectural decisions are made.

We now agree in #12226 that chip-selects should be associated the bus rather than device because that's what Linux does---even though Zephyr went the other way when the "new" SPI API was added two years ago. That same SPI API which is one of the exactly two APIs Zephyr claims is "stable" (the other being ADC, which is not functionally complete).

But instead of either reconsidering that decision and ignoring Linux to put cs-gpios in the device where Zephyr expects it, or biting the bullet and changing the API, we're going to have the API and DT architectures remain inconsistent. In an LTS release.

[galak]

@pabigot do you want to keep the discussion about how the API should handle CS here or open an new issue for that?

[pabigot]

@galak I feel this issue is about device tree support for SPI CS, which is affected by but not the same as device tree tooling support for SPI CS (#12647). Based on my last comment I think that comes down to a choice: either we change the SPI API to put CS into the bus, or we change DTS to put CS into the device.

I don't think we've gotten enough input here to make that decision, since it's basically you and I going back and forth. If you want to close this then that finalizes the decision that CS goes into the bus, and we certainly need a new issue to change the API. If we leave this open then we could create a new issue about changing the API, but we'd be discussing the same basic question in two places. If we only leave this open, nobody will realize we've got an open question that in its current implicit resolution means we'll have to change the API.

Let's discuss this in the telecon tomorrow.

[pabigot]

Per dev review telecon 2019-01-24 the API will be changed.