cpu/fe310, board/hifive1: SPI support by pyropeter · Pull Request #10833 · RIOT-OS/RIOT

pyropeter · 2019-01-21T14:26:05Z

Please note: This is not tested as much as I would like. Please wait with merging until I had a look with an oscilloscope.

Contribution description

This implements periph/spi on cpu/fe310 and board/hifive1
Hardware Chip Select lines were not implemented, since they don't offer any benefit for transfers larger than one byte in size, due to limitations of the hardware. This simplifies the code.

Testing procedure

make -C tests/periph_spi BOARD=hifive1

Issues/PRs references

~~Depends on and includes PR cpu/fe310: Add call to periph_init() #10825~~ (merged)
~~Depends on and includes PR boards/hifive1: fix CLOCK_CORECLOCK #10805~~ (merged)

Until now fe310's cpu_init() was missing a call to periph_init(). This commit adds this call.

maribu

This looks pretty good to me. I have found something to nitpick about, though ;-)

Sadly I'm unable to test this. Maybe I should consider buying a board...

maribu · 2019-01-23T11:53:40Z

cpu/fe310/Makefile.include

 USEMODULE += periph_pm

+# include common periph drivers
+USEMODULE += periph_common


periph_common is added twice

Anyway, this is not needed, so just remove it.

maribu · 2019-01-23T11:58:12Z

cpu/fe310/periph/spi.c

+/**
+ * @brief   Allocation device locks
+ */
+static mutex_t lock;


Maybe it would make sharing the code with future versions of the CPU that could potentially have multiple SPI interface easier when you use something like that:

#define SPI_INTERFACE_NUM 1 static mutex_t lock[SPI_INTERFACE_NUM];

(I'm only suggesting this, not insisting on this.)

maribu · 2019-01-23T12:00:23Z

cpu/fe310/periph/spi.c

+
+void spi_init_pins(spi_t bus)
+{
+    (void) bus;


Should not needed because of the assert(bus == 0); below. (I believe both GCC and clang should still be aware that bus was used even when NDEBUG is defined, but I'm not 100% sure about that. So please correct me if I'm wrong.)

maribu · 2019-01-23T12:02:13Z

cpu/fe310/periph/spi.c

+
+int spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk)
+{
+    (void) bus;


I think it would make life easier to also add assert()s for bus and cs here, so that developers are supported in finding their bugs in using the SPI interface.

maribu · 2019-01-23T12:02:42Z

cpu/fe310/periph/spi.c

+
+void spi_release(spi_t bus)
+{
+    (void) bus;


Again I would suggest to add an assert()

maribu · 2019-01-23T12:04:50Z

cpu/fe310/periph/spi.c

+        uint32_t rxdata = SPI_RXFIFO_EMPTY;
+        while (rxdata & SPI_RXFIFO_EMPTY) {
+            rxdata = SPI1_REG(SPI_REG_RXFIFO);
+        }


How about

uint32_t rxdata; do { rxdata = SPI1_REG(SPI_REG_RXFIFO); } while (rxdata & SPI_RXFIFO_EMPTY);

stale · 2019-08-10T02:08:02Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

kaspar030 · 2019-09-10T07:47:10Z

I'll try to use phillip to test this!

aabadie

I'm very interested by this PR. I also think that #12934 could be helpful to determine the cpu core clock frequency.

aabadie · 2019-12-13T13:54:57Z

cpu/fe310/Makefile.include

 USEMODULE += sifive_drivers_fe310

 USEMODULE += periph
+USEMODULE += periph_common


This is not needed, just remove it.

aabadie · 2019-12-13T13:55:14Z

cpu/fe310/Makefile.include

 USEMODULE += periph_pm

+# include common periph drivers
+USEMODULE += periph_common


Anyway, this is not needed, so just remove it.

aabadie · 2019-12-13T13:55:51Z

cpu/fe310/cpu.c

    set_csr(mstatus, MSTATUS_DEFAULT);
+
+    /* trigger static peripheral initialization */
+    periph_init();


This is already in master. Maybe just rebase this PR so this change is dropped from GitHub diff ?

aabadie · 2019-12-13T13:57:24Z

boards/hifive1/include/periph_conf.h

- * #define CLOCK_CORECLOCK           (38400000ul)
- */
+/* As defined in boards/hifive1/board.c CPU_DESIRED_FREQ **/
+#define CLOCK_CORECLOCK             (200000000ul)


You might be interested by #12934 where the value of the core clock is automatically computed when using the PLL or HFXOSC.
Maybe rebase this PR on top of it ?

aabadie · 2019-12-13T13:58:14Z

boards/hifive1/Makefile.features

 FEATURES_PROVIDED += periph_rtc
 FEATURES_PROVIDED += periph_rtt
-#FEATURES_PROVIDED += periph_spi
+FEATURES_PROVIDED += periph_spi


This could also be applied to the hifive1b (I can test on this board actually).

aabadie · 2019-12-13T14:00:50Z

boards/hifive1/include/periph_conf.h

+    DIV_UP(CLOCK_CORECLOCK, 2 * 10000000) - 1,
+};
+#undef DIV_UP
+#define SPI_NUMOF                   (1)


I would like to have some struct based configuration. See #12917 where I changed the UART to use this scheme. I find this convenient, at least to know which pins are connected the serial interface and it's more consistent with most of other board configurations in RIOT (like STM, SAM, nRF).

pyropeter · 2019-12-13T19:16:21Z

@aabadie This is quite old, and I forgot most of the details regarding this PR. I also stopped using RIOT and I don't have a lot of time at the moment, so I will not continue working on this PR. Feel free to take the code and do whatever you like with it. (But please update the author and copyright information so that it doesn't look like I alone wrote the resulting code. Where appropriate, feel free to remove my name completely.)

aabadie · 2019-12-13T21:31:20Z

I was planning to write the SPI driver when I saw your PR. I'll use it as a base and will keep your authorship. You have already done most of the work.
Anyway, thanks for your contribution @pyropeter!

aabadie · 2019-12-16T10:03:18Z

The replacement PR is here: #12957. Except the configuration and the reuse of clock improvement from #12934, the initial version was already working well. Thanks for the work @pyropeter.

maribu and others added 4 commits January 21, 2019 15:11

cpu/fe310: Add call to periph_init()

17096a5

Until now fe310's cpu_init() was missing a call to periph_init(). This commit adds this call.

boards/hifive1: fix CLOCK_CORECLOCK

a5cc0ce

cpu/fe310: implement SPI

7c4e810

fixup! cpu/fe310: implement SPI

aa541f2

PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers Area: boards Area: Board ports labels Jan 22, 2019

maribu reviewed Jan 23, 2019

View reviewed changes

maribu added the Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms label Jan 23, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot closed this Sep 10, 2019

miri64 reopened this Sep 10, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 10, 2019

fjmolinas requested a review from aabadie December 13, 2019 09:14

aabadie requested changes Dec 13, 2019

View reviewed changes

pyropeter closed this Dec 13, 2019

aabadie mentioned this pull request Dec 16, 2019

cpu/fe310: add spi peripheral driver #12957

Merged

Conversation

pyropeter commented Jan 21, 2019 • edited by maribu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

kaspar030 commented Sep 10, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyropeter commented Dec 13, 2019

Uh oh!

aabadie commented Dec 13, 2019

Uh oh!

aabadie commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pyropeter commented Jan 21, 2019 •

edited by maribu

Loading