New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Master<->Slave SPI test #5200

Closed
wants to merge 2 commits into
base: master
from

Conversation

@superna9999
Copy link
Collaborator

superna9999 commented Nov 29, 2017

This patchset:

  • Fixes the spi_ll_stm32 Slave frame shifting to avoid leaving the transaction with bytes in the TX FIFO
  • Adds a new Master<->Slave SPI test using a physical on-board/header loopback with the SPI Slave controller in async mode while the Master controller acts in Sync mode.

I know @tbursztyka is working on a proper Slave API, but here its not incompatible since the test uses the transceive() call directly, and could be updated later on.

This patchset need #5199 to be tested

@SebastianBoe SebastianBoe removed their request for review Nov 29, 2017

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented Nov 29, 2017

@tbursztyka could you have a look at the test ?

@@ -121,8 +119,12 @@ static void spi_stm32_shift_s(SPI_TypeDef *spi, struct spi_stm32_data *data)
spi_context_update_tx(&data->ctx, 2, 1);
}
}
else

This comment has been minimized.

@tbursztyka

tbursztyka Nov 29, 2017

Contributor

Always put {} on if/for/while/else etc... even on 1 liners statement then.

and btw, move the else right next to the last }

This comment has been minimized.

@superna9999

superna9999 Nov 29, 2017

Collaborator

Damn... ok

@mbolivar
Copy link
Member

mbolivar left a comment

Awesome, thanks so much for doing this! It was really needed.

I've got a couple comments on the STM32 changes and some ideas on how to structure the test so it will be easier to port to other boards. I need to find more time to look at the rest.


tx_frame = spi_stm32_next_tx(data);
if (LL_SPI_IsActiveFlag_TXE(spi)) {
if (SPI_WORD_SIZE_GET(data->ctx.config->operation) == 8) {
LL_SPI_TransmitData8(spi, tx_frame);
/* The update is ignored if TX is off. */

This comment has been minimized.

@mbolivar

mbolivar Nov 29, 2017

Member

This comment and its duplicate below should be removed now, since this change checks that TX is on earlier.

This comment has been minimized.

@superna9999

superna9999 Nov 30, 2017

Collaborator

ok


if (LL_SPI_IsActiveFlag_RXNE(spi)) {
if (SPI_WORD_SIZE_GET(data->ctx.config->operation) == 8) {
rx_frame = LL_SPI_ReceiveData8(spi);
if (spi_context_rx_on(&data->ctx)) {

This comment has been minimized.

@mbolivar

mbolivar Nov 29, 2017

Member

Similarly, the change means that RX on has already been checked here, so this existing check (and the one below) should be removed.

This comment has been minimized.

@superna9999

superna9999 Nov 30, 2017

Collaborator

OK

@@ -0,0 +1,13 @@
CONFIG_BOOT_BANNER=y

This comment has been minimized.

@mbolivar

mbolivar Nov 29, 2017

Member

What do you think about following the pattern in the led_ws2812 and led_lpd8806 samples (in samples/drivers) to have a top-level prj.conf, and merge in a board-specific fragment?

That will make it easier to add additional boards and have a clean top-level prj.conf.

This comment has been minimized.

@superna9999

superna9999 Nov 30, 2017

Collaborator

Good idea, I will do that

#define SPI_MASTER_CS &spi_master_cs
#define SPI_SLAVE_CS &spi_slave_cs

#if defined(CONFIG_BOARD_NUCLEO_F091RC)

This comment has been minimized.

@mbolivar

mbolivar Nov 29, 2017

Member

Following the led sample app pattern I referred to in another comment, to make this easier to port to other boards, how about doing following something like this here instead:

#define SPI_MASTER_DRV_NAME "spi-loopback-master"
#define SPI_SLAVE_DRV_NAME "spi-loopback-slave"

Then in the nucleo f091rc .conf fragment:

CONFIG_SPI_1_NAME="spi-loopback-master"
CONFIG_SPI_2_NAME="spi-loopback-slave"

Then device_get_binding(SPI_xxx_DRV_NAME) can still be used to find the device, and additional boards be ported to this test without adding anything to the source code.

Similarly, an application-specific Kconfig file (specified with KCONFIG_ROOT; see the application development primer for details) could be used to configure the rest -- SPI_SLAVE, MIN_FREQ, etc. -- in a board-specific conf file. (Or perhaps DT would be better -- @nashif / @galak any opinion?)

This comment has been minimized.

@superna9999

superna9999 Nov 30, 2017

Collaborator

Good ideas aswell, thanks

This comment has been minimized.

@superna9999

superna9999 Nov 30, 2017

Collaborator

I did the DRV_NAME trick, I think the other stuff will need to wait until DT has been implemented for SPI

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch from 80d03f3 to f19502e Nov 30, 2017

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented Nov 30, 2017

Fine by me, I see no problem with that test. slave does work that way, it's the API not addressing the issue when the slave protocol is more complicated (i.e. reacting at first byte reception, getting awake at slave select, etc... ) which is limiting here.

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch from f19502e to e6bc8fa Nov 30, 2017

@mbolivar
Copy link
Member

mbolivar left a comment

Thanks for the update! Just a couple of additional questions about porting to other boards.

#define SPI_SLAVE 0
#define MIN_FREQ 500000

#undef SPI_MASTER_CS

This comment has been minimized.

@mbolivar

mbolivar Nov 30, 2017

Member

It looks like SPI_MASTER_CS ends up defined as null and MASTER_CS_CTRL_GPIO_DRV_NAME is "" no matter which branch is taken -- is that what you meant?

This comment has been minimized.

@mbolivar

mbolivar Dec 2, 2017

Member

I'm still not sure I know what your idea is here, since the definitions seem to end up the same.

This comment has been minimized.

@superna9999

superna9999 Dec 2, 2017

Collaborator

I left it open for new implementations...

This comment has been minimized.

@superna9999

superna9999 Dec 5, 2017

Collaborator

I simplified it with the Kconfig


#if defined(CONFIG_BOARD_NUCLEO_F091RC)
#define SPI_SLAVE 0
#define MIN_FREQ 500000

This comment has been minimized.

@mbolivar

mbolivar Nov 30, 2017

Member

I guess you'd rather not make these application-specific Kconfig options added in with KCONFIG_ROOT, as discussed in the previous version -- any reason why not?

The advantage to doing it that way is to configure additional boards just with fragments, instead of having to touch this file.

This comment has been minimized.

@mbolivar

mbolivar Dec 2, 2017

Member

Ah, sorry, I think I see your point now:

I did the DRV_NAME trick, I think the other stuff will need to wait until DT has been implemented for SPI

I'm personally OK with deferring this portability consideration until DT for SPI has been defined, especially if @tbursztyka has an idea for this. And even otherwise, I think it's better to have this code in tree than not.

So even though I like having a Kconfig solution that can be replaced with DT better than having a source-only solution, I'm OK with this change.

This comment has been minimized.

@superna9999

superna9999 Dec 5, 2017

Collaborator

OK, I added the Kconfig in the last push, please tell me if it's ok

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch 3 times, most recently from 525368a to eaceae7 Dec 5, 2017

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented Dec 7, 2017

@mbolivar Is the Kconfig addition to the test ok for you ?

@mbolivar
Copy link
Member

mbolivar left a comment

@superna9999 awesome, thanks for the update! I really like the Kconfig changes for increased portability; thanks for adding them.

I see a couple of things that are either bugs or just me misunderstanding. Do you mind explaining what's up here?

.gpio_pin = 0,
.delay = 0
};
#define SPI_SLAVE_CS (&spi_slave_cs)

This comment has been minimized.

@mbolivar

mbolivar Dec 11, 2017

Member

Sorry, I'm confused about this behavior, probably I am missing something.

If you want to ignore CS on the slave (i.e. CONFIG_TEST_SPI_SLAVE_CS_IGNORE=y), shouldn't SPI_SLAVE_CS be NULL here, instead of &spi_slave_cs? I.e. shouldn't this be the code for the #else branch?

This comment has been minimized.

@superna9999

superna9999 Dec 12, 2017

Collaborator

OK, It needs a comment here to explain.

On STM32, there is 3 behaviours:

  • cs=NULL leads to use the Hardware NSS pin, if not muxed it will never start
  • cs=valid, dev=NULL leads to ignore Hardware NSS and won't use a GPIO
  • cs=valid, dev=valid leads to use a GPIO as CS, but it makes no sense (yet) in slave mode

With @tbursztyka we aggred about that and the spi_context.h handles the second case.

I added a comment for that


#ifdef CONFIG_TEST_SPI_MASTER_CS_GPIO_ENABLE
struct spi_cs_control spi_master_cs = {
.gpio_pin = 0,

This comment has been minimized.

@mbolivar

mbolivar Dec 11, 2017

Member

Thanks a lot for the update!

I see that the gpio_dev field is set based on CONFIG_TEST_MASTER_CS_GPIO_DRV_NAME below in master_cs_ctrl_gpio_config(), but I don't see a config option or code for setting this gpio_pin field. Am I missing it?

This comment has been minimized.

@superna9999

superna9999 Dec 12, 2017

Collaborator

I added the missing pin config

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch from eaceae7 to 2a2891f Dec 12, 2017

@superna9999 superna9999 requested review from dbkinder and erwango as code owners Dec 12, 2017

@superna9999 superna9999 changed the base branch from master to arm Dec 12, 2017

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch 2 times, most recently from 0b8f691 to 0f18a85 Dec 12, 2017

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented Dec 12, 2017

recheck

@mbolivar
Copy link
Member

mbolivar left a comment

Thanks for the update and the explanations! I'm good with these changes. Will try to find some time to port to STM32F4.

@mbolivar

This comment has been minimized.

Copy link
Member

mbolivar commented Dec 12, 2017

Shippable error says:

Warning: Could not extract kernel version

Not sure what is going on. Does it need a rebase or something?

@dbkinder

This comment has been minimized.

Copy link
Collaborator

dbkinder commented Dec 12, 2017

The shippable failure "Warning: Could not extract kernel version" is coming from a step in the documentation build process that's trying to extract the kernel version information from the VERSION file (within the doc/conf.py configuration file) and is failing. @nashif merged a patch (#5329) for this in the master branch, but it has not been applied to the arm branch.

@galak galak force-pushed the zephyrproject-rtos:arm branch from dc71422 to 43c111e Dec 12, 2017

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch from b5b1428 to 10d963a Feb 23, 2018

@superna9999 superna9999 changed the base branch from arm to master Feb 23, 2018

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented Feb 23, 2018

I'll rebase against #5921 when merged.

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented Mar 19, 2018

@superna9999 While investigating SPI slave API (as seen in #5921) I had issues with designware driver, with a bit more complex samples (one that tries to use the slave input event): the slave gets out of sync. I thought I was missing something on designware side to I tested your patch made on stm32. And the exact behaviour happens.

The difference with your sample here, besides using the SPI input event, is that I do my tests on 2 different boards: one acting as a master and one as a slave. Because on same board you get the possibility to trick the irq priorities so that slave gets a higher priority than the master, as you did, so that the slave is never "late" in reading the input data.

Actually I start to believe that it might not be exactly feasible to do what I want: slave being extremely dependent on host cpu and input event cb code, there is no way to guaranty that the slave will be answering on time to the master since the master drives the spi bus clock: so it might be on time, but it might also be too late in reading bytes from master and replying by writing to it. On designware, at best I get 1 byte out of sync, on stm32 I get 2 bytes. (With full debug, it of course gets really worse).

Check https://github.com/tbursztyka/zephyr/commits/spi_slave_wip

Maybe it's not worth trying to support "complex" slave protocols, and that we should just go with symmetrical transceive call from the start (so without any break in the transaction process: master and slave would have exact same length of buffers from the start). I might be missing something here.

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented Mar 20, 2018

Hi @tbursztyka
I had a hard time imaginating a way to handle the callbacks you implemented without a well defined Host behavior. Unlike I2C, SPI needs the host to leave setup/hold time after the CS and leave dummy bytes in the transaction, the problem is : how do enforce this ? We can't.... I think your code is ok, but you may add a big fat warning about the Host behaviour. The NOR Flash protocol needs a long setup and hold time, and some dummy bytes for some commands, for the same reasons. This is why we should support the symmetrical transceive calls as the default Slave interface with these optional calls if necessary.

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented Mar 20, 2018

Yep looks like I was banging my head on a non-solvable issue. That I think I'll drop the input stuff, and just have the normal spi_transceive() that could be used for slave as usual. Well, it's much easier then!
Thanks

@carlescufi

This comment has been minimized.

Copy link
Member

carlescufi commented May 16, 2018

@tbursztyka @anangl @MaureenHelm is this still relevant?

superna9999 added some commits Nov 29, 2017

spi: spi_ll_stm32: fix slave frame shifting
In the current form of the code, the slave frame shifting leaves the TX
full and will shift out some unwanted bytes out on the next transaction.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
tests: spi: Add a dual controller Master<->Slave Loopback test
This patch introduce a SPI Master<->Slave test for SoCs with two SPI
controllers with a physical loopback on the board/headers.

One controller is used as Master doing synchronous transactions while
the other controller run the complement Slave transaction using
an asynchronous transceive call.

This test had been tested on a nucleo_f091rc board.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

@superna9999 superna9999 force-pushed the superna9999:for-pr/nucleo_f091rc_spi_slave_test branch from 10d963a to bc50511 May 16, 2018

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented May 16, 2018

@carlescufi yes, at least the fix in the stm32 spi driver to fix slave mode

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 16, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5200      +/-   ##
==========================================
+ Coverage   55.25%   55.25%   +<.01%     
==========================================
  Files         468      468              
  Lines       51671    51671              
  Branches     9895     9895              
==========================================
+ Hits        28550    28551       +1     
+ Misses      19208    19207       -1     
  Partials     3913     3913
Impacted Files Coverage Δ
lib/posix/pthread.c 69.84% <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 e2393a0...bc50511. Read the comment docs.

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented May 16, 2018

@carlescufi yes definitely.
However, it needs to be rebased on top of master. Unless github is showing me outdated patches, I don't see the SPI changes applied on the test app (the changes are trivial)

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented May 16, 2018

@tbursztyka I just rebased it on master

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented May 16, 2018

@superna9999 hum, check spi.h, there was a big update ~2 months ago. dev is back as a parameter before config, and spi_buf are not directly passed, but instead go through spi_buf_set in order to remove the length parameters.

@avisconti

This comment has been minimized.

Copy link
Collaborator

avisconti commented May 21, 2018

@superna9999 @tbursztyka
I'm using this PR and seems to work fine. Nevertheless I put another fix which I think you need to take a look at: PR #7687

Moreover I had to decrease the SPI master speed at 500000, otherwise it was going in OVR error.
I'm investigating about the possibility to add DMA to STM32 SPI. Any plan yet?

@avisconti

This comment has been minimized.

Copy link
Collaborator

avisconti commented May 21, 2018

Ah, in this PR I'm using this commit only: 7f07c8a

@superna9999

This comment has been minimized.

Copy link
Collaborator

superna9999 commented May 23, 2018

@avisconti On which SoC ? you should be able to receive at more than 500KHz on L4 and even F0
There is no plan to add DMA, but feel free to add it, it will be welcome !
I think I'll drop the second patch and repost the first patch in a separate PR, what do you think @tbursztyka ?

@tbursztyka

This comment has been minimized.

Copy link
Contributor

tbursztyka commented May 23, 2018

@superna9999 sure, the stm32 fix could be applied asap.

@avisconti

This comment has been minimized.

Copy link
Collaborator

avisconti commented May 23, 2018

@superna9999
I'm using ArgonKey board which has a STM32F412 SoC.
I'm working fine with speed==500000. With speed>=1000000 it goes in OVR almost immediately.
Yes, it's better to split the commits.
I'm investigating the dma part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment