drivers: spi: SPI API extension draft

[nordic-krch]

This PR is created to focus on API review. It is extracted from #21641 which contains reference implementation on nRF SPIM peripheral.

A draft of SPI API extension. Following API introduces:

• transaction concept. Transaction consists of SPI configuration and messages
• transaction can be scheduled and canceled
• multiple control pins support to allow:
  • controlling cmd/data pin for displays
  • transaction for multiple devices (with the same configuration)
• message is simplex (one direction) and with additional flags it configures:
  • cs0 control (set/nop on start, clear/nop on end)
  • direction
  • delay
  • pausing after completion (e.g. to wait for rdy pin, spi_resume() resumes)
  • optional cs1 control (e.g. to control cmd/data pin)
• two driver specific functions:
  • spi_single_transfer() - asynchronous raw spi transfer
  • spi_configure() - spi configuration
• generic spi functions (vendor agnostic)
  • spi_schedule()
  • spi_cancel()
  • spi_resume()
• helper macros for creating set of spi messages

Note that:

• qop_mngr is queued operation manager. A module which will be responsible for managing execution of asynchronous operations which are requested by multiple asynchronous users.
• async_client is a generic asynchronous client see struct onoff_client in include/sys/onoff.h

Adding this extension brings following improvements:

• spi transfers can be scheduled from any context
• reduced cpu usage (test shows ~20% gain) since less context switches occurs
• simpler transfer setup (see demo sample https://github.com/nordic-krch/zephyr/blob/i2c_async/samples/drivers/spi_async/src/main.c)
• setting typical simplex transfers (like TX followed by TX or RX) require less memory (2 vs 4 messages)
• support for driving data/cmd pin often seen in displays.
• support for pausing/resuming transactions (e.g. pend on rdy pin) - removing the need for lock/release feature
• ability to setup multi message transaction and get notification once whole is completed (or error occurs), e.g. sensor setup sequence. Messages can be then in flash.
• vendor specific functions simplified

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

[zephyrbot]

Some checks failed. Please fix and resubmit.

checkpatch (informational only, not a failure)

-:20: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#20: FILE: include/drivers/spi.h:124:
+ * @{ */

-:48: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#48: FILE: include/drivers/spi.h:152:
+#define SPI_MSG_CS0_END_NOP	(0))

-:58: WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#58: FILE: include/drivers/spi.h:162:
+ *  on message end. */

-:63: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#63: FILE: include/drivers/spi.h:167:
+#define SPI_MSG_CS1_END_NOP	(0))

-:136: WARNING:TYPO_SPELLING: 'occured' may be misspelled - perhaps 'occurred'?
#136: FILE: include/drivers/spi.h:287:
+ * occured.)

-:149: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#149: FILE: include/drivers/spi.h:300:
+#define SPI_MSG_TXRX(txbuf, txlen, rxbuf, rxlen, _flags) \
+	{ \
+		.buf = (u8_t *)txbuf, \
+		.len = txlen, \
+		.flags = SPI_MSG_WRITE | SPI_MSG_COMMIT | \
+			SPI_MSG_CS0_START_SET | _flags\
+	}, \
+	{ \
+		.buf = (u8_t *)rxbuf, \
+		.len = rxlen, \
+		.flags = SPI_MSG_READ | SPI_MSG_COMMIT | SPI_MSG_CS0_END_CLR \
+	}

-:163: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#163: FILE: include/drivers/spi.h:314:
+#define SPI_MSG_TXTX(txbuf0, txlen0, txbuf1, txlen1, _flags) \
+	{ \
+		.buf = (u8_t *)txbuf0, \
+		.len = txlen0, \
+		.flags = SPI_MSG_WRITE | SPI_MSG_COMMIT | \
+			SPI_MSG_CS0_START_SET | _flags \
+	}, \
+	{ \
+		.buf = (u8_t *)txbuf1, \
+		.len = txlen1, \
+		.flags = SPI_MSG_WRITE | SPI_MSG_COMMIT | SPI_MSG_CS0_END_CLR \
+	}

-:232: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#232: FILE: include/drivers/spi.h:405:
+ * ^I^I^Ifrom interrupt context or caller context.$

-:289: WARNING:TYPO_SPELLING: 'occured' may be misspelled - perhaps 'occurred'?
#289: FILE: include/drivers/spi.h:462:
+ * User is notified once transaction is completed or error occured.

- total: 0 errors, 9 warnings, 325 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Gitlint issues

4: UC4 Line exceeds max length (77>72): "- transaction concept. Transaction consists of SPI configuration and messages"
6: UC4 Line exceeds max length (77>72): "- message is simplex (one direction) and with additional flags it configures:"
10: UC4 Line exceeds max length (77>72): " - pausing after completion (e.g. to wait for rdy pin, spi_resume() resumes)"

Documentation issues

/var/lib/shippable/build/IN/main_repo/zephyr/include/drivers/spi.h:514: warning: The following parameters of spi_transceive(struct device *dev, const struct spi_config *config, const struct spi_buf_set *tx_bufs, const struct spi_buf_set *rx_bufs) are not documented:
  parameter 'dev'
  parameter 'config'
/var/lib/shippable/build/IN/main_repo/zephyr/include/drivers/spi.h:514: warning: The following parameters of spi_transceive(struct device *dev, const struct spi_config *config, const struct spi_buf_set *tx_bufs, const struct spi_buf_set *rx_bufs) are not documented:
  parameter 'dev'
  parameter 'config'
/var/lib/shippable/build/IN/main_repo/zephyr/include/drivers/spi.h:514: warning: The following parameters of spi_transceive(struct device *dev, const struct spi_config *config, const struct spi_buf_set *tx_bufs, const struct spi_buf_set *rx_bufs) are not documented:
  parameter 'dev'
  parameter 'config'
/var/lib/shippable/build/IN/main_repo/zephyr/include/drivers/spi.h:514: warning: The following parameters of spi_transceive(struct device *dev, const struct spi_config *config, const struct spi_buf_set *tx_bufs, const struct spi_buf_set *rx_bufs) are not documented:
  parameter 'dev'
  parameter 'config'

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[tbursztyka]

Hum there is a wind of bloating API in Zephyr these days.

About #21641 I still don't see why you do not create a transaction manager outside of i2c/SPI APIs.
I like the idea of such transaction manager but it should be fully outside. I don't know why you are going per-API, it is imo a bad solution in the long run.

First because, at least with SPI, there is no need of an API change: an API above it could simply add the transaction scheme and keep track of things. I believe it could be done for i2c as well.

Second because part of what is being addressed by this issue is not a specific i2c/SPI issue: the capability to cancel a call and/or handling a proper timeout etc... Are GENERIC device driver issue. But as usual people (you are not the only one) prefer to address the issue case by case instead of looking at the big picture and fixing generically which would avoid bloating each and every API, add clarity, stability etc... That said, my mistake as well: as many of us, I have know these issues for a long time. Did I fix it? ... (same goes to PM etc...)

Anyway, point is: for bus APIs it's far better to have a short API doing only what it is meant to do (i.e. handling i/o messages) and everything else should be above.

If such transaction manager would be above i2c/SPI/ it would provide you much better flexibility in configuring various aspects of it like scheduling (which bus has higher priority for instance, or should it be transaction wise) or scaling (should it run under 1 thread, or one per bus type, etc...). You have to get a solution ready for hundreds of sensors as well as for just 3-4.
But so far, the solution you are looking after, is not going to scale properly. Mostly because you are solving it on a per-bus approach, and not generically.

Please reconsider your approach. You did find out an actual serious issue in #21538 but I think it would deserve to go one step beyond i2c and SPI.

[pabigot]

I like the idea of such transaction manager but it should be fully outside. I don't know why you are going per-API, it is imo a bad solution in the long run.

I complete agree with this, which is why I still want the queued operation manager infrastructure to come first, then a generic transaction manager developed to requirements identified by the needs of multiple driver APIs. As soon as topic-gpio gets settled I think the first step can be made available fairly quickly.

[nordic-krch]

@tbursztyka thanks for your comment.

I still don't see why you do not create a transaction manager outside of i2c/SPI APIs.

It's going that direction. Note that underneath there is qop_mngr API which is "queued operation manager". This one abstracts scenario where there is a one operation executor and multiple clients requesting asynchronously operation with own data. Another thing that started with onoff service is definition of "asynchronous client". onoff service defines asynchronous client and in #21641 it's been extracted into async_client.h. So we already have group of 3 generic api's: queue manager, onoff service and async client.
On one of the last API calls (21.01.2020) I've mentioned that i2c and spi transaction manager could be common so we agree on that, i guess. There is a transaction, scheduling, canceling, etc. so it can be made generic as well.

Anyway, point is: for bus APIs it's far better to have a short API doing only what it is meant to do (i.e. handling i/o messages) and everything else should be above.

Note that new SPI proposal limits vendor specific API to two functions (spi_configure(), spi_single_transfer()) so it is also going into that direction where driver is doing only vendor specific stuff. Also if you look at proposed struct spi_msg it is also very similar to struct i2c (one buffer + flags) which i also did on purpose, having in mind more generic approach.

I actually, hoped that discussion in that PR will go into that direction (generic bus/transaction manager) so i think we are on the same (or similar) page. Lets wait for other comments.

[tbursztyka]

"where driver is doing only vendor specific stuff" You don't want vendor specific hooks in a generic API. This is always a bad idea because once one uses it, the code is in fact not generic anymore and cannot be easily ported to other platforms.

That said, I understand that business customers might want to access some vendor specific features. then do so via include/drivers/spi/<vendor_model>.h

[carlescufi]

On one of the last API calls (21.01.2020) I've mentioned that i2c and spi transaction manager could be common so we agree on that, i guess. There is a transaction, scheduling, canceling, etc. so it can be made generic as well.

@nordic-krch so this functionality would effectively be added to device.h then?

[nordic-krch]

@tbursztyka i think i was not clear on that. What I meant was that driver needs to implement 2 functions: configuring spi, scheduling asynchronous transfer which seemed like most low level driver functions to implement: all generic stuff (delays, pins, access control, etc.) is outside.

[tbursztyka]

On one of the last API calls (21.01.2020) I've mentioned that i2c and spi transaction manager could be common so we agree on that, i guess. There is a transaction, scheduling, canceling, etc. so it can be made generic as well.

@nordic-krch so this functionality would effectively be added to device.h then?

Note that only cancelling an API call and handling an API call timeout should be generic enough to be part of device.h. For the transaction, I think its place is above all the other specific API it is providing a service for.

[nordic-krch]

@carlescufi

@nordic-krch so this functionality would effectively be added to device.h then?

I don't think so. transactions are not that generic. Not sure which APIs would use it except for i2c and spi. ipm? can?

[carlescufi]

@carlescufi

@nordic-krch so this functionality would effectively be added to device.h then?

I don't think so. transactions are not that generic. Not sure which APIs would use it except for i2c and spi. ipm? can?

Well, I think it's worth considering it. Maybe CAN (CC @alexanderwachter) maybe I2S?

[pabigot]

I don't think so. transactions are not that generic. Not sure which APIs would use it except for i2c and spi. ipm? can?

ADC, flash, fs, display, LED, even GPIO.

A transaction manager has relevance in these circumstances:

• A device can be shared among multiple independent functional components;
• The device provides only elemental operations;
• A functional component requires exclusive access to the device for a series of operations (transaction) that must not be interleaved with other uses of the device;
• A functional component has to perform a transaction but can to wait for existing uses to complete before it gets control.

The data required for and behavior of transactions will be specific to an API, or even a use case, but a manager can be a generic solution with wide application.

[alexanderwachter]

For CAN, I already started to pull out common functions from the drivers and put them into zephyr/drivers/can/can_common.c.
The goal was:

• The diver is always async and only has an ISR callback (WIP drivers: CAN: Abstract blocking send call #20890)
• Support for blocking and async sending (WIP drivers: CAN: Abstract blocking send call #20890). Blocking version can be called from user threads
• None-blocking send function with ring-buffers, that can be called from ISRs (planned)
• Timeout for send functions (WIP [WIP] Drivers: CAN: Implementation of a send timeout for can_send(...) #19707)
• Wrappers that allow receiving messages in user threads (WIP for attach_workq, already possible with attach_msgq)

It would be great if we could share some of the common functions.

[carlescufi]

@tbursztyka @nordic-krch @pabigot @alexanderwachter
I think this deserves a discussion in the API meeting. I will schedule it in.

[nordic-krch]

regarding:

I still don't see why you do not create a transaction manager outside of i2c/SPI APIs.

I'm not sure if you already have idea how to do that but i came with following proposal: #22409

Short summary:

• device structure gets additional field - a pointer to extension api structure which contains mask of capabilities and pointer to data. if more than one extension is supported then data is an array of pointers associated with each api.

  zephyr/include/device.h

  Line 302 in 47ec301

  struct device_ext_api *ext_api;

• device.h gets internal api for checking if device supports given extension (z_device_ext_api_supported(dev, bitmask)) and getting data associated with that extension (z_device_ext_api_get_data(dev, bitmask))

  zephyr/include/device.h

  Line 328 in 47ec301

  static inline bool z_device_ext_api_supported(struct device *dev, u32_t api_mask)

• external api (onoff service in that example) implements simple inline like device_get_onoff_service(dev, &srv)

  zephyr/include/sys/onoff.h

  Line 434 in 47ec301

  static inline int device_get_onoff_service(struct device *dev,

• example of usage:

err = device_subsys_get_onoff_service(clock, subsys, &srv);
if (err < 0) {
    LOG_ERR("Failed to get device onoff service (err: %d)", err);
}

err = onoff_request(srv, &client);

[pabigot]

API 2020-07-07: Moved from triage to in-progress; implementation of #22409 supporting #23371.