cpu/msp430fxyz: improved SPI driver speed #5886

haukepetersen · 2016-09-28T18:10:24Z

From the discussion in #4780 we heard, that the SPI driver speed for the MSP430 platform performs poorly when writing a burst of data (e.g. when transferring network packets to the radio). This PR should improve the situation quite a bit.

Changes:

improved the transfer function when only writing data to a device
added the transfer_byte() (convenience) function to the periph_common module while making the transfer_bytes() function local to the driver. This will significantly reduce the number of function calls while transferring data...

@PeterKietzmann would you mind to test this and benchmark this against your existing numbers? Thx

kaspar030 · 2016-09-28T19:35:28Z

cpu/msp430fxyz/periph/spi.c

+
+    return length;
+}
+
 int spi_transfer_byte(spi_t dev, char out, char *in)


you'll have to remove this.

haukepetersen · 2016-09-29T06:24:25Z

fixed and squashed the occurrence of the old transfer_byte() function

PeterKietzmann · 2016-09-29T11:35:35Z

Theoretically I agree with you about this adoption. Practically it appears that the cc2420 on the z1 node does not work with this PR. As a test I took to "old" SPI implementation and added a spi_fast_write method which I only use for writing the complete frame to the FIFO. That function is implemented as proposed in #4780 and everything works. Unfortunately this PR influences device drivers. I'm still about to figure out weather this is a problem of SPI or the device driver.

haukepetersen · 2016-09-29T12:04:11Z

But could you hook up your scope and check out the output signal/timings with this PR, to make sure the driver is not broken in general but that the male functioning has something to do with timings?!

haukepetersen · 2016-09-29T12:12:57Z

I don't have a Z1 here available right now, so I can't really test anything here. But by looking at the code again, the only thing that has changed should be, that we leave out a function call for each byte transferred, though we should have tighter inter-byte timings. Looking at the CC2420 datasheet, an SPI clock up to 20MHz should be fine (and we are currently at 5MHz, right?), so seems that the radio should actually work fine under the new conditions. Wonder what we are doing wrong here.

PeterKietzmann · 2016-09-30T08:56:38Z

The SPI driver itself works like this, I checked it. However, the cc2420 driver (@4Mhz) has some problems here and there. There are some SPI accesses which are not successful when using NULL as output parameter. I think I identified most (still one issue open), but need to dig deeper into the question why. Luckily, writing the frame to the FIFO works with high speed feature (which usually consumes most of the time).
However, I fear that this change might introduce further device driver problems. Especially when thinking about adapting other SPI drivers like this. So again the counter question: How about we introduce an optional spi_write_fast function the the API?
I will report back once I proceeded with debugging the cc2420 driver using this PR. Hopefully everything will clear up.

haukepetersen · 2016-09-30T09:42:14Z

I still have some trouble to understand, why the cc2420 driver has problems with the new code. When calling the transfer_bytes function with out := NULL, the adapted SPI driver is actually transferring a logical 0, while the current master is doing exactly the same...

However, I fear that this change might introduce further device driver problems.

Not sure... We need to figure out, why exactly the cc2420 is not behaving correctly. And if we open up new problems, it IMHO points more in the direction of bad driver implementation than to a broken SPI driver. And if so, we should face the problems with other drivers and fix them by the root instead of building upon a non-optimal SPI driver below to obfuscate those problems. But first step first, we need to understand whats going wrong here.

How about we introduce an optional spi_write_fast function the the API?

State now, I am absolutely against it - feels to me like building a quick-fix workaround into a main RIOT API: not good...

jnohlgard · 2016-10-01T08:41:16Z

I am against the idea of creating two separate API calls for the same functionality. It should be possible to rewrite the loops for efficiency to take different code paths depending on whether the input is NULL or not and depending on the desired bus clock vs. the core clock to try to speed up the transfer.
Size-wise it should be the same or smaller compared to having two separate functions in the API.

kaspar030 · 2016-10-01T14:08:13Z

+1

-----Original Message-----
From: "Joakim Nohlgård" notifications@github.com
To: RIOT-OS/RIOT RIOT@noreply.github.com
Cc: Kaspar Schleiser kaspar@schleiser.de, Comment comment@noreply.github.com
Sent: Sa., 01 Okt. 2016 10:40
Subject: Re: [RIOT-OS/RIOT] cpu/msp430fxyz: improved SPI driver (write) speed (#5886)

I am against the idea of creating two separate API calls for the same functionality. It should be possible to rewrite the loops for efficiency to take different code paths depending on whether the input is NULL or not and depending on the desired bus clock vs. the core clock to try to speed up the transfer.
Size-wise it should be the same or smaller compared to having two separate functions in the API.

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#5886 (comment)

PeterKietzmann · 2016-10-04T07:11:23Z

@gebart that's what this PR does, isn't it? I still need to get this PR running with the cc2420 radio.

PeterKietzmann · 2016-10-05T08:16:10Z

cpu/msp430fxyz/periph/spi.c

+        for (unsigned i = 0; i < length; i++) {
+            while (!(SPI_IF & SPI_IE_TX_BIT)) {}
+            SPI_DEV->TXBUF = (uint8_t)out[i];
+        }


Please add the following lines here:

while(SPI_DEV->STAT & 1){}; SPI_DEV->RXBUF;

The first line waits for any ongoing transmission by reading the UCBUSY bit in the status register (compare datasheet sec. 16.4.5). The second line simply reads the receive buffer which will clear the UCxRXIFG bit. In master mode that bit indicates that data has been received. I tell you, without this read operation, things can get messy :/ ...

@haukepetersen you wrote the msp_430-regs file. Maybe you want to add a macro for the UCBUSY bit in the status register?

haukepetersen · 2016-10-05T09:35:58Z

done.

@PeterKietzmann: could you do me one more favor? Could you connect your logic analyzer and post the output for
a) transfer_bytes(dev, buf, NULL, 10);
b) transfer_bytes(dev, buf, buf2, 10);
with buf and buf2 being some random 10 byte data buffer?

I would be interested in how much performance difference we encounter between the two code paths in the current form. But as said, I don't have a z1 here anymore, so I can't do it myself... Thx

PeterKietzmann · 2016-10-05T09:45:28Z

@haukepetersen my logic analyser currently strikes, that's why I used an oscilloscope on the clock line. Case a) has an inter-byte-delay of ~360 ns (yes nano seconds) whereas b) has about 4,8 us. Is that enough for you?
My proposed change does not work for telosb :(. I need to figure out why before merging...

haukepetersen · 2016-10-05T13:38:22Z

thanks!

An inter-byte difference of 4us, wow! So I guess this short-path makes sense here. Will play around with this on some other platforms as well... Thx again and post your progress on the telosb :-)

PeterKietzmann · 2016-10-10T13:08:54Z

In the periph_conf.h of different msp430 based boards, SPI_IE_TX_BIT and SPI_IE_RX_BIT are defined. I think we should rename them to SPI_IF_TX_BIT and SPI_IF_RX_BIT as these were part of the "Interrupt Flag Register" (compare msp430xf2xx family user guid page 510 section 19.3.13. Same applies for older msp430xf1xx MCUs)

PeterKietzmann · 2016-10-10T13:00:32Z

cpu/msp430fxyz/periph/spi.c

+            while (!(SPI_IF & SPI_IE_TX_BIT)) {}
+            SPI_DEV->TXBUF = (uint8_t)out[i];
+            while (SPI_DEV->STAT & USCI_SPI_STAT_UCBUSY) {}
+            SPI_DEV->RXBUF;


Both lines should go below the closing brace in line 227 (just be called once, after the transmission of multiple bytes). However, we will need to adapt this passage for the older msp430 revisions so we can do it then.

What we need here (below the closing brace in line 227) is:

#ifndef SPI_USE_USCI while (!(SPI_IF & SPI_IE_TX_BIT) || !(SPI_IF & SPI_IE_RX_BIT)) {} SPI_DEV->RXBUF; #else while (SPI_DEV->STAT & USCI_SPI_STAT_UCBUSY) {} SPI_DEV->RXBUF; #endif

why not just use while (!(SPI_IF & SPI_IE_TX_BIT) || !(SPI_IF & SPI_IE_RX_BIT)) {} for both variants?

No this (and simply waiting for !(SPI_IF & SPI_IE_RX_BIT)) does not work on z1 nodes with msp430xf2xx. I can't fully explain it but I've tested it several times.

PeterKietzmann · 2016-10-11T16:38:24Z

cpu/msp430fxyz/periph/spi.c

    }
-    return 1;
+    else {


@haukepetersen I'd like to improve "simple" receiving as well

else if (!out) { for (unsigned i = 0; i < length; i++) { SPI_DEV->TXBUF = 0; while (!(SPI_IF & SPI_IE_RX_BIT)) {} in[i] = (char)SPI_DEV->RXBUF; } }

haukepetersen · 2016-10-12T08:01:22Z

took the read RX buf part out of the loop for the 'fast send' -> @PeterKietzmann simplified your proposal slightly, does this work?
added a 'fast receive' path

PeterKietzmann · 2016-10-12T08:37:37Z

fast receive is ok

kaspar030 · 2016-10-12T08:38:42Z

cpu/msp430fxyz/periph/spi.c

+    else {
+        char tmp;
+        for (unsigned i = 0; i < length; i++) {
+            tmp = (out) ? out[i] : 0;


Isn't the !out case handled above?

yep. will adapt

Sorry I confused this PRs. Can't adapt this one but #5921 later. However, your comment makes sense and test succeeded.

kaspar030 · 2016-10-12T08:46:52Z

cpu/msp430fxyz/periph/spi.c

+            in[i] = (char)SPI_DEV->RXBUF;
+        }
+    }
+


It leads to the same assembly, but I'd prefer a little more C-like code:

unsigned n = length; while(n--) { SPI_DEV->TXBUF = *out++; }

makes the for loops less verbose

saves the compiler from doing all the dereferences (in[i] = x ...)

hm, I actually tend to like the newby-array-notation better. I think it is easier to read for non-professional-pointer-magicians (or am I the only one?), and the effects on the compiler are neglectable...

haukepetersen · 2016-10-17T07:31:07Z

fixed spi_transfer_ and rebased.

@kaspar030 I won't fix the array-style notation in the loop.

haukepetersen · 2016-10-17T07:33:17Z

also was so free to squash...

PeterKietzmann · 2016-10-17T08:58:16Z

I prefer the "old" notations as well, in terms of readability. However, I won't have NACKed Kaspars proposal. Will check this PR in the afternoon

PeterKietzmann · 2016-10-17T13:28:05Z

cpu/msp430fxyz/periph/spi.c

+            SPI_DEV->TXBUF = (uint8_t)out[i];
+        }
+        while (!(SPI_IF & SPI_IE_RX_BIT)) {}
+        SPI_DEV->RXBUF;


We need the following here, even though it's not the prettiest solution. I've read the manual several times and tested different configurations on (and between) z1 and telosb motes so I'm pretty convinced this is the right thing to do:

#ifndef SPI_USE_USCI while (!(SPI_IF & SPI_IE_TX_BIT) || !(SPI_IF & SPI_IE_RX_BIT)) {} SPI_DEV->RXBUF; #else while (SPI_DEV->STAT & 1) {} SPI_DEV->RXBUF; #endif

IIRC you already introduced a macro for checking bit no 1 (BUSY bit) in the status register for devices with SPI_USE_USCI but this was lost I guess.

PeterKietzmann · 2016-10-17T13:39:57Z

ACK once addressed. If we consider this PR for the release can we probably include #5921 as well?!

haukepetersen · 2016-10-17T14:59:21Z

fixed and squashed right in.

PeterKietzmann · 2016-10-17T15:07:39Z

cpu/msp430fxyz/periph/spi.c

+            while (!(SPI_IF & SPI_IE_TX_BIT)) {}
+            SPI_DEV->TXBUF = (uint8_t)out[i];
+        }
+        while (!(SPI_IF & SPI_IE_RX_BIT)) {}


Sorry @haukepetersen I was to imprecise in my previous comment. This line can go out.

PeterKietzmann

ACK and go

haukepetersen added this to the Release 2016.10 milestone Sep 28, 2016

haukepetersen assigned PeterKietzmann Sep 28, 2016

haukepetersen mentioned this pull request Sep 28, 2016

drivers/spi: reworked SPI driver interface #4780

Merged

kaspar030 reviewed Sep 28, 2016

View reviewed changes

OlegHahm assigned rousselk Sep 28, 2016

haukepetersen force-pushed the opt_msp_spi branch from c1a982e to b381fd5 Compare September 29, 2016 06:23

PeterKietzmann reviewed Oct 5, 2016

View reviewed changes

PeterKietzmann reviewed Oct 10, 2016

View reviewed changes

PeterKietzmann reviewed Oct 11, 2016

View reviewed changes

haukepetersen force-pushed the opt_msp_spi branch from 88e0ff0 to df4b80c Compare October 12, 2016 08:00

PeterKietzmann changed the title ~~cpu/msp430fxyz: improved SPI driver (write) speed~~ cpu/msp430fxyz: improved SPI driver speed Oct 12, 2016

kaspar030 reviewed Oct 12, 2016

View reviewed changes

haukepetersen force-pushed the opt_msp_spi branch from df4b80c to 5d939ca Compare October 17, 2016 07:30

haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 17, 2016

haukepetersen force-pushed the opt_msp_spi branch from 5d939ca to 864ebed Compare October 17, 2016 07:33

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 17, 2016

PeterKietzmann reviewed Oct 17, 2016

View reviewed changes

drivers/periph_common: added spi_transfer_byte()

08610ee

haukepetersen force-pushed the opt_msp_spi branch from 864ebed to 0e65637 Compare October 17, 2016 14:59

PeterKietzmann mentioned this pull request Oct 17, 2016

cpu/stm32f1: optimize SPI driver #5921

Merged

PeterKietzmann reviewed Oct 17, 2016

View reviewed changes

haukepetersen added 2 commits October 17, 2016 20:47

cpu/msp430fxyz: improved SPI driver speed

6cd8d98

cpu/msp430fxyz: added bifields for SPI STAT reg

470fe20

haukepetersen force-pushed the opt_msp_spi branch from 0e65637 to 470fe20 Compare October 17, 2016 18:47

PeterKietzmann approved these changes Oct 17, 2016

View reviewed changes

PeterKietzmann merged commit ed67143 into RIOT-OS:master Oct 17, 2016

haukepetersen deleted the opt_msp_spi branch October 19, 2016 08:13

cpu/msp430fxyz: improved SPI driver speed #5886

cpu/msp430fxyz: improved SPI driver speed #5886

Conversation

haukepetersen commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Sep 29, 2016

PeterKietzmann commented Sep 29, 2016

haukepetersen commented Sep 29, 2016

haukepetersen commented Sep 29, 2016

PeterKietzmann commented Sep 30, 2016

haukepetersen commented Sep 30, 2016

jnohlgard commented Oct 1, 2016

kaspar030 commented Oct 1, 2016

PeterKietzmann commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Oct 5, 2016

PeterKietzmann commented Oct 5, 2016

haukepetersen commented Oct 5, 2016

PeterKietzmann commented Oct 10, 2016

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

haukepetersen commented Oct 12, 2016

PeterKietzmann commented Oct 12, 2016

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

haukepetersen commented Oct 17, 2016

haukepetersen commented Oct 17, 2016

PeterKietzmann commented Oct 17, 2016

Choose a reason for hiding this comment

PeterKietzmann commented Oct 17, 2016

haukepetersen commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann left a comment

Choose a reason for hiding this comment