pl022 spi v2 #905

vchong · 2016-07-07T10:49:18Z

Signed-off-by: Victor Chong [email protected]

vchong · 2016-07-07T10:50:01Z

etienne-lms · 2016-07-07T15:08:49Z

core/drivers/pl022_spi.c

+
+static void pl022_sanity_check(struct pl022_data *pd)
+{
+	DMSG("SSPB2BTRANS: Expected: 0x2. Read: 0x%x\n",


could check here that fields expected by pl022_configure() are initialized: non null, assuming pl022_init() resets the pl022_data structure. i.e base, and cs_gpio_pin

Something like this?

edited

assert(pd); assert(pd->chip.ops); assert(pd->base); assert(pd->cs_gpio_base); assert(pd->clk_hz); assert(pd->speed_hz && pd->speed_hz <= pd->clk_hz/2); assert(pd->mode <= SPI_MODE3); assert(pd->data_size_bits == 8 || pd->data_size_bits == 16);

mode and data_size_bitsdo not need to be asserted. their value are checked in pl022_configure().

Isn't it easier to check/assert them here as you suggested than to wait till we get to pl022_configure()? If so, I can remove the value checkings in pl022_configure(), but should be no harm leaving them there?

etienne-lms · 2016-07-07T15:12:28Z

thanks for the instantiation of the spi devices.

as a general minor comment: most of the uint32_t are simply unsigned values. I think you should better declare them as unsigned int, not caring about being 32b or 64b:

i and j counter in loops
num_txpkts and num_rxpkts arguments of send/recv handlers.
clk_hz and speed_hz fields of pl022_data structure, and related freqX local variables.

also the pl022_configuration() should fail somehow in case of bad configuration. panic or return with some non-null value ?

jenswi-linaro · 2016-07-07T16:51:13Z

core/drivers/pl022_spi.c

+
+			/* rx 1 packet */
+			if (read32(pd->base + SSPSR) & SSPSR_RNE)
+				rdat[j++] = read8(pd->base + SSPDR);


How can we know that the buffer is large enough?

The transfer by itself is always one packet/word in and one out, but since SPI doesn't specify rules for number of packets transmitted and/or received (does it?), the code was written with the assumption that there's a possibility to receive more packets than sent, and decisions regarding buffer size left to the caller assumed to have that knowledge, but yes, this does lead to a possibility of overflow.

What about

Read only as many packets as transmitted, or

Add a parameter to specify the length of the buffer, and only read up to its size, or

Other better suggestions?

Normally one would expect *num_rxpkts hold the number of max packets to receive.

Ok, will use num_rxpkts as inout.

vchong · 2016-07-08T03:01:09Z

thanks for the instantiation of the spi devices.

np, thanks for the review! :)

i and j counter in loops
num_txpkts and num_rxpkts arguments of send/recv handlers.

Changed these to size_t per Jens.

clk_hz and speed_hz fields of pl022_data structure, and related freqX local variables.

Changed these to unsigned int as suggested.

also the pl022_configuration() should fail somehow in case of bad configuration. panic or return with some non-null value ?

Is the assert suggested in the sanity check function above sufficient?

jenswi-linaro · 2016-07-08T07:11:29Z

core/drivers/pl022_spi.c

+				tee_time_wait(1);
+
+			DMSG("TX FIFO available.. resuming..\n");
+			i--; /* retry current packet in next loop */


This is a bit complicated. Why not just test this stuff at the beginning of the loop instead of decreasing i to make another round.

Ok. Will try to simplify this.

vchong · 2016-07-09T10:15:44Z

Comments addressed. Please continue review.

Thanks!

etienne-lms · 2016-07-16T12:37:20Z

core/drivers/pl022_spi.c

+	size_t i = 0, j = 0;
+	struct pl022_data *pd = container_of(chip, struct pl022_data, chip);
+
+	gpio_set_value(pd->cs_gpio_pin, GPIO_LEVEL_LOW);


in case no data to read/write, it could be better to not trig the chip-select low/high.
maybe it's not an issue with SPI.

It's not an issue to toggle cs on no data as long as clk stays still. I can add a check to return if num_txpkts == 0, but that does seem to unnecessarily complicate the code for a use case that's probably close to 0%? I.e. why call read/write when there's nothing to read/write?

etienne-lms · 2016-07-19T13:13:35Z

Reviewed-by: etienne carriere <[email protected]>

vchong · 2016-07-19T14:47:48Z

@jenswi-linaro Are you ok with the new changes as well?

jenswi-linaro · 2016-07-19T15:51:23Z

Reviewed-by: Jens Wiklander <[email protected]>

vchong · 2016-07-19T16:58:27Z

Rebased, squashed, tags added and ready for merge. Thanks!

jforissier · 2016-07-20T07:23:35Z

I'd like to see the outcome of the Travis build. For some reason it has not triggered; I will investigate.
I found a compile error when CFG_TEE_CORE_LOG_LEVEL=2:

core/drivers/pl022_spi.c: In function ‘pl022_flush_fifo’:
core/drivers/pl022_spi.c:402:11: error: variable ‘rdat’ set but not used [-Werror=unused-but-set-variable]
  uint32_t rdat;
           ^
cc1: all warnings being treated as errors
mk/compile.mk:146: recipe for target 'out/arm-plat-hikey/core/drivers/pl022_spi.o' failed

(you need to add a __maybe_unused qualifier).

jforissier · 2016-07-20T07:24:45Z

core/drivers/pl022_spi.c

+#include <trace.h>
+#include <util.h>
+
+/* spi register offsets */


jforissier · 2016-07-20T07:37:03Z

Please add $make CFG_PL022=y to .travis.yml so that this code gets compiled by the CI loop.

vchong · 2016-07-20T11:09:42Z

__maybe_unused added to rdat

vchong · 2016-07-21T11:08:44Z

Added $make PLATFORM=hikey CFG_ARM64_core=y CFG_TEE_TA_LOG_LEVEL=4 DEBUG=1 CFG_PL061=y CFG_PL022=y to .travis.yml.

vchong · 2016-07-21T13:02:04Z

Comments addressed. Please continue review.

vchong · 2016-07-21T16:00:17Z

Update: Added CFG_PL061=y CFG_PL022=y to some other platforms in .travis.yml.

jforissier · 2016-07-22T07:22:12Z

.travis.yml

@@ -94,9 +94,11 @@ script:

  # FVP
  - $make PLATFORM=vexpress-fvp CFG_ARM32_core=y
+  - $make PLATFORM=vexpress-fvp CFG_ARM32_core=y CFG_PL061=y CFG_PL022=y


I'm wondering if we should enable PL061 and PL022 by default on HiKey instead. Then, no need to add anything to .travis.yml to have the code build-tested by Travis. Enabling them on other platforms would not check more things it seems. What do you think?

Sounds good to me. Will change.

OK thanks, sorry for my bad suggestion in the first place ;)

No problem. Updated and waiting for travis results.

jforissier · 2016-07-22T09:02:05Z

Reviewed-by: Jerome Forissier <[email protected]>

vchong · 2016-07-22T09:07:45Z

Rebased, squashed and additional tag added. Ok for merge? Thanks!

Signed-off-by: Victor Chong <[email protected]> Reviewed-by: etienne carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]>

vchong mentioned this pull request Jul 7, 2016

pl022 spi #897

Closed

etienne-lms reviewed Jul 7, 2016
View reviewed changes

jenswi-linaro reviewed Jul 7, 2016
View reviewed changes

jenswi-linaro reviewed Jul 8, 2016
View reviewed changes

vchong force-pushed the spi2 branch 7 times, most recently from 4c1bfb2 to 45352cc Compare July 9, 2016 10:04

vchong force-pushed the spi2 branch 2 times, most recently from 5be9077 to 8d58f7c Compare July 9, 2016 10:25

etienne-lms reviewed Jul 16, 2016
View reviewed changes

vchong force-pushed the spi2 branch from efae02d to 49d8c28 Compare July 19, 2016 16:56

jforissier reviewed Jul 20, 2016
View reviewed changes

vchong force-pushed the spi2 branch 3 times, most recently from 715fc3b to 60ad160 Compare July 21, 2016 12:59

vchong force-pushed the spi2 branch from 60ad160 to 3508ea0 Compare July 21, 2016 15:57

jforissier reviewed Jul 22, 2016
View reviewed changes

vchong force-pushed the spi2 branch from 7007b52 to aca1545 Compare July 22, 2016 09:05

drivers: add spi framework and pl022 driver

aca1545

Signed-off-by: Victor Chong <[email protected]> Reviewed-by: etienne carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]>

jforissier merged commit aca1545 into OP-TEE:master Jul 22, 2016

vchong deleted the spi2 branch November 6, 2016 10:46

vchong mentioned this pull request Dec 2, 2016

SPI Test for HiKey: Size of Tx seems to be limited to first transaction? #1208

Closed

vchong mentioned this pull request Jan 5, 2017

some SPI refactorization #1251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pl022 spi v2 #905

pl022 spi v2 #905

vchong commented Jul 7, 2016

vchong commented Jul 7, 2016

etienne-lms Jul 7, 2016

vchong Jul 8, 2016 •

edited

Loading

etienne-lms Jul 8, 2016

vchong Jul 8, 2016

etienne-lms commented Jul 7, 2016

jenswi-linaro Jul 7, 2016

vchong Jul 8, 2016

jenswi-linaro Jul 8, 2016

vchong Jul 8, 2016

vchong commented Jul 8, 2016 •

edited

Loading

jenswi-linaro Jul 8, 2016

vchong Jul 8, 2016

vchong commented Jul 9, 2016

etienne-lms Jul 16, 2016

vchong Jul 19, 2016

etienne-lms commented Jul 19, 2016

vchong commented Jul 19, 2016

jenswi-linaro commented Jul 19, 2016

vchong commented Jul 19, 2016

jforissier commented Jul 20, 2016

jforissier Jul 20, 2016

vchong Jul 20, 2016

jforissier commented Jul 20, 2016

vchong commented Jul 20, 2016

vchong commented Jul 21, 2016

vchong commented Jul 21, 2016

vchong commented Jul 21, 2016

jforissier Jul 22, 2016

vchong Jul 22, 2016

jforissier Jul 22, 2016

vchong Jul 22, 2016

jforissier commented Jul 22, 2016

vchong commented Jul 22, 2016

pl022 spi v2 #905

pl022 spi v2 #905

Conversation

vchong commented Jul 7, 2016

vchong commented Jul 7, 2016

Choose a reason for hiding this comment

vchong Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Jul 7, 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

vchong commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchong commented Jul 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Jul 19, 2016

vchong commented Jul 19, 2016

jenswi-linaro commented Jul 19, 2016

vchong commented Jul 19, 2016

jforissier commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Jul 20, 2016

vchong commented Jul 20, 2016

vchong commented Jul 21, 2016

vchong commented Jul 21, 2016

vchong commented Jul 21, 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

jforissier commented Jul 22, 2016

vchong commented Jul 22, 2016

vchong Jul 8, 2016 •

edited

Loading

vchong commented Jul 8, 2016 •

edited

Loading