[DNM] drivers: spi: Add shim for nrfx SPI driver #6458

anangl · 2018-03-05T12:29:56Z

This adds a translation layer to make the nrfx driver for the legacy
(i.e. without EasyDMA) nRF SPI peripheral accessible via the updated
Zephyr's API of the SPI driver.
Slave mode (i.e. the SPIS peripheral) is not supported yet.

Board-specific configurations for a few nRF5 boards are also added
to the "spi_loopback" test.

Depends on #5921

codecov-io · 2018-03-05T12:59:20Z

Codecov Report

Merging #6458 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6458      +/-   ##
==========================================
+ Coverage   54.96%   54.96%   +<.01%     
==========================================
  Files         450      450              
  Lines       43408    43408              
  Branches     8160     8160              
==========================================
+ Hits        23858    23859       +1     
+ Misses      16236    16235       -1     
  Partials     3314     3314

Impacted Files	Coverage Δ
kernel/posix/pthread.c	`70% <0%> (+0.5%)`	⬆️

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 5e6c2c3...f013ba9. Read the comment docs.

mbolivar

I have tested this driver successfully with a modified samples/drivers/led_ws2812 on nrf52_pca10040 using SPI_0 on the Arduino headers.

I've got some coding style comments

mbolivar · 2018-03-06T16:06:17Z

drivers/spi/Kconfig.nrf5

+
+if SPI_1_NRF5_SPI || SPI_1_NRF5_SPIM || SPI_1_NRF5_SPIS
+
+config SPI_1_NRF5


Neither SPI_0 nor SPI_2 have an analogous option. Should this be here?

It should not, of course. This option was supposed to be replaced by SPI_1_NRF5_SPI. I somehow failed to remove this entry after inserting the choice section above.

mbolivar · 2018-03-06T16:08:07Z

drivers/spi/spi_nrf5.c

+
+#include "spi_context.h"
+
+#ifdef __cplusplus


Isn't this unnecessary in the .c file? C++ code can access it via an extern "C" declaration in a header, no?

Right, it's completely unnecessary here. I'm surprised that I didn't notice its presence. It must have been present in the source file that I started with as a base and I got used to it so much that it just became transparent to me. Thank for pointing this out!

mbolivar · 2018-03-06T16:10:01Z

drivers/spi/spi_nrf5.c

+		return -EINVAL;
+	}
+
+	struct spi_context *ctx = &get_dev_data(spi_cfg->dev)->ctx;


Zephyr code style puts variable declarations at top of function body, all in one block (this applies below in the file as well)

mbolivar · 2018-03-06T16:16:01Z

drivers/spi/spi_nrf5.c

+	if (result != NRFX_SUCCESS) {
+		SYS_LOG_ERR("Failed to initialize device: %s",
+			    dev->config->name);
+		assert(false);


This can just return an error instead, no?

Yes, it should return an error. But this value is currently ignored.
Added a return statement, and a comment.

Still: return a valid error code.

I have plans to check these and do something about it (I may introduce it in the relevant pr actually), so it's preferable that all drivers conforms to the signature, even if it's not handled right now. The day it will be: it will be seamless (no need to fix any drivers).

mbolivar · 2018-03-06T16:21:32Z

tests/drivers/spi/spi_loopback/src/spi.c

@@ -29,6 +29,7 @@ struct spi_cs_control spi_cs = {
 #else
 #define SPI_CS NULL
 #define CS_CTRL_GPIO_DRV_NAME ""
+


This should probably be removed from this patch

tbursztyka · 2018-03-07T07:48:17Z

drivers/spi/spi_nrf5.c

+				  get_nrf_spi_bit_order(spi_cfg->operation));
+	}
+
+	nrf_spi_frequency_set(spi->p_reg,


this should go into the if {} block above.

I thought that frequency could be changed between transceive calls. Looks like I misunderstood the "rest of the attributes" in the note describing spi_config. I thought it referred to operation field only. I'll correct it.

tbursztyka · 2018-03-07T07:52:13Z

drivers/spi/spi_nrf5.c

+	if (result != NRFX_SUCCESS) {
+		SYS_LOG_ERR("Failed to initialize device: %s",
+			    dev->config->name);
+		assert(false);


Still: return a valid error code.

I have plans to check these and do something about it (I may introduce it in the relevant pr actually), so it's preferable that all drivers conforms to the signature, even if it's not handled right now. The day it will be: it will be seamless (no need to fix any drivers).

anangl · 2018-03-07T07:59:55Z

@mbolivar Thanks for looking into this. I addressed your comments.

anangl · 2018-03-07T08:27:40Z

@tbursztyka Applied corrections.

comments applied

tbursztyka · 2018-03-19T12:38:04Z

@anangl I just put you patches in #5921

anangl · 2018-03-19T12:46:08Z

@tbursztyka great!

Split QMSI relevant part into its own file. Some config where using prompt, some not: normalizing it by removing the prompt keyword where relevant. Reducing the file by using if/endif when relevant. However, it still not fully clean default: cfg and default baudrate should disappear. There is no default configuration to apply as long as the controller is not configured to run from any part using spi API. Signed-off-by: Tomasz Bursztyka <[email protected]>

This will be useful on Quark_SE ARC core which can access x86 core SPI controller. Signed-off-by: Tomasz Bursztyka <[email protected]>

s/u32_s/u32t obviously Signed-off-by: Tomasz Bursztyka <[email protected]>

As QMSI driver does not support new SPI API. Signed-off-by: Tomasz Bursztyka <[email protected]>

Native DW driver is relevantly used instead. Signed-off-by: Tomasz Bursztyka <[email protected]>

These now support the new API through SPI DW native driver. Signed-off-by: Tomasz Bursztyka <[email protected]>

Ditch any legacy API support altogether. Signed-off-by: Tomasz Bursztyka <[email protected]>

Adds support for the new spi api to the mcux dspi shim driver. Does not remove support for the legacy spi api since there are still consumers of that api, particularly the mcr20a 802.15.4 driver. Signed-off-by: Maureen Helm <[email protected]>

Adds a board-specific configuration for the frdm_k64f to the spi_loopback test. Signed-off-by: Maureen Helm <[email protected]>

And adding support for GPIO CS as well. It looks like the driver could benefit from centralizing all SPI access into a unique function, the protocol does not seem too convoluted to do so, like CC2520 or CC1200. Signed-off-by: Tomasz Bursztyka <[email protected]>

Now that MCR20A supports the new API, legacy support from mcux dspi driver can be safely removed. Signed-off-by: Tomasz Bursztyka <[email protected]>

It involves a minor change on which register is configured. Most of the change is with threshold handling. Handling the Kconfig based supported mode per-port. Signed-off-by: Tomasz Bursztyka <[email protected]>

This port is a slave SPI port only. Signed-off-by: Tomasz Bursztyka <[email protected]>

Quark SE provides a slave SPI. Signed-off-by: Tomasz Bursztyka <[email protected]>

No need to test it twice then in completed(). Signed-off-by: Tomasz Bursztyka <[email protected]>

No need to check on busy bit from hardware. Signed-off-by: Tomasz Bursztyka <[email protected]>

SPI API is reentrant. Signed-off-by: Tomasz Bursztyka <[email protected]>

Removing old API usage. Signed-off-by: Tomasz Bursztyka <[email protected]>

This adds a translation layer to make the nrfx driver for the legacy (i.e. without EasyDMA) nRF SPI peripheral accessible via the updated Zephyr's API of the SPI driver. Configuration files are already prepared for adding support for SPIM (Master with EasyDMA) and SPIS (Slave with EasyDMA) peripherals. Signed-off-by: Andrzej Głąbek <[email protected]>

This adds to the spi_loopback test board-specific configurations for the following boards: - nrf51_pca10028 - nrf52_pca10040 - nrf52840_pca10056 Signed-off-by: Andrzej Głąbek <[email protected]>

As legacy SPI API is being removed. Signed-off-by: Tomasz Bursztyka <[email protected]>

No drivers nor samples are using it anymore. Signed-off-by: Tomasz Bursztyka <[email protected]>

anangl · 2018-04-04T17:12:43Z

Merged via #5921.

anangl requested review from jukkar, mbolivar, nashif, superna9999 and tbursztyka as code owners March 5, 2018 12:29

anangl force-pushed the nrfx_shim_spi branch 7 times, most recently from 5da31c9 to 71a212d Compare March 6, 2018 18:03

mbolivar reviewed Mar 6, 2018

View reviewed changes

tbursztyka previously requested changes Mar 7, 2018

View reviewed changes

anangl force-pushed the nrfx_shim_spi branch from 71a212d to f364bb5 Compare March 7, 2018 07:58

anangl force-pushed the nrfx_shim_spi branch from f364bb5 to 4851a6c Compare March 7, 2018 08:26

anangl force-pushed the nrfx_shim_spi branch 5 times, most recently from ea2c164 to f51fb85 Compare March 13, 2018 13:12

mbolivar mentioned this pull request Mar 21, 2018

NRF52 SPI master for new SPI API #5670

Closed

nashif added the DNM This PR should not be merged (Do Not Merge) label Mar 28, 2018

Tomasz Bursztyka and others added 11 commits April 3, 2018 14:28

drivers/spi: Enable port 3 and 4 on DW driver

255b970

This will be useful on Quark_SE ARC core which can access x86 core SPI controller. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/spi: Fix typo on parameters type in DW arc regs definitions

b806924

s/u32_s/u32t obviously Signed-off-by: Tomasz Bursztyka <[email protected]>

arch/quark_se_c1000_ss: Switch to SPI DW driver

260a43d

As QMSI driver does not support new SPI API. Signed-off-by: Tomasz Bursztyka <[email protected]>

arch/quark_d2000: Switch to SPI DW driver

8b48fc0

As QMSI driver does not support new SPI API. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/spi: Removing QMSI driver as it does not support new API

8979839

Native DW driver is relevantly used instead. Signed-off-by: Tomasz Bursztyka <[email protected]>

tests/spi: Remove excluded boards

99c65ae

These now support the new API through SPI DW native driver. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/spi: Switch Intel driver to new SPI API

542dc77

Ditch any legacy API support altogether. Signed-off-by: Tomasz Bursztyka <[email protected]>

tests: spi_loopback: Add frdm_k64f configuration

3a5ee47

Adds a board-specific configuration for the frdm_k64f to the spi_loopback test. Signed-off-by: Maureen Helm <[email protected]>

drivers/spi: Remove legacy API support from mcux dspi driver

248c844

Now that MCR20A supports the new API, legacy support from mcux dspi driver can be safely removed. Signed-off-by: Tomasz Bursztyka <[email protected]>

anangl force-pushed the nrfx_shim_spi branch from f51fb85 to 51b92aa Compare April 3, 2018 15:22

Tomasz Bursztyka added 8 commits April 3, 2018 17:30

drivers/spi: Add slave mode support to the DesignWare driver

7609644

It involves a minor change on which register is configured. Most of the change is with threshold handling. Handling the Kconfig based supported mode per-port. Signed-off-by: Tomasz Bursztyka <[email protected]>

boards/x86: Pinmux SPI port 2 relevantly on quark_se_c1000_devboard

f9e588c

This port is a slave SPI port only. Signed-off-by: Tomasz Bursztyka <[email protected]>

arch/quark_se: Enable SPI port 2 as a slave only

bf1c187

Quark SE provides a slave SPI. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/spi: Simplify how error is forwarded from ISR handler in DW

d894887

No need to test it twice then in completed(). Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/spi: spi_context lock makes transceive function reentrant in DW

3deb1f4

No need to check on busy bit from hardware. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/ethernet: No need of semaphore for spi in enc28j60

fb15270

SPI API is reentrant. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/ieee802154: No need of semaphore for spi in mcr20a

fcdcab9

SPI API is reentrant. Signed-off-by: Tomasz Bursztyka <[email protected]>

samples/bluetooth: Move hci_spi to new SPI API

774397e

Removing old API usage. Signed-off-by: Tomasz Bursztyka <[email protected]>

anangl force-pushed the nrfx_shim_spi branch from 51b92aa to 4017e56 Compare April 3, 2018 15:38

anangl and others added 4 commits April 3, 2018 18:25

tests: spi_loopback: Add configurations for a few nRF5 boards

c0b0667

This adds to the spi_loopback test board-specific configurations for the following boards: - nrf51_pca10028 - nrf52_pca10040 - nrf52840_pca10056 Signed-off-by: Andrzej Głąbek <[email protected]>

drivers/spi: Remove legacy NRF5 master and slave drivers

b542ddc

As legacy SPI API is being removed. Signed-off-by: Tomasz Bursztyka <[email protected]>

spi: Remove SPI legacy API

f013ba9

No drivers nor samples are using it anymore. Signed-off-by: Tomasz Bursztyka <[email protected]>

anangl force-pushed the nrfx_shim_spi branch from 4017e56 to f013ba9 Compare April 3, 2018 16:28

anangl changed the title ~~[DNM] drivers: nrf5: Add shim for nrfx SPI driver~~ [DNM] drivers: spi: Add shim for nrfx SPI driver Apr 4, 2018

anangl closed this Apr 4, 2018

anangl deleted the nrfx_shim_spi branch April 13, 2018 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM] drivers: spi: Add shim for nrfx SPI driver #6458

[DNM] drivers: spi: Add shim for nrfx SPI driver #6458

anangl commented Mar 5, 2018 •

edited

Loading

codecov-io commented Mar 5, 2018 •

edited

Loading

mbolivar left a comment

mbolivar Mar 6, 2018

anangl Mar 7, 2018

mbolivar Mar 6, 2018

anangl Mar 7, 2018

mbolivar Mar 6, 2018

anangl Mar 7, 2018

mbolivar Mar 6, 2018

anangl Mar 7, 2018

tbursztyka Mar 7, 2018

mbolivar Mar 6, 2018

anangl Mar 7, 2018

tbursztyka Mar 7, 2018

anangl Mar 7, 2018

tbursztyka Mar 7, 2018

anangl commented Mar 7, 2018

anangl commented Mar 7, 2018

tbursztyka commented Mar 19, 2018

anangl commented Mar 19, 2018

anangl commented Apr 4, 2018


		if SPI_1_NRF5_SPI \|\| SPI_1_NRF5_SPIM \|\| SPI_1_NRF5_SPIS

		config SPI_1_NRF5


		#include "spi_context.h"

		#ifdef __cplusplus

[DNM] drivers: spi: Add shim for nrfx SPI driver #6458

[DNM] drivers: spi: Add shim for nrfx SPI driver #6458

Conversation

anangl commented Mar 5, 2018 • edited Loading

codecov-io commented Mar 5, 2018 • edited Loading

Codecov Report

mbolivar 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

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

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

Choose a reason for hiding this comment

anangl commented Mar 7, 2018

anangl commented Mar 7, 2018

tbursztyka commented Mar 19, 2018

anangl commented Mar 19, 2018

anangl commented Apr 4, 2018

anangl commented Mar 5, 2018 •

edited

Loading

codecov-io commented Mar 5, 2018 •

edited

Loading