Skip to content

cpu/fe310: slightly rework the uart driver#12917

Merged
fjmolinas merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/cpu/fe310_uart_rework
Dec 20, 2019
Merged

cpu/fe310: slightly rework the uart driver#12917
fjmolinas merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/cpu/fe310_uart_rework

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Dec 10, 2019 •
edited
Loading

Contribution description

While working on #12891, I noticed that the UART driver of the fe310 CPU would need some improvement:

  • not inline with the usual way of configuring peripheral at board level
  • code duplication
  • gpio used by UART initialized in board_init

This PR fixes all these problems while keeping the UART working. This PR is also saving some ROM memory (40B):
Strange, the ROM size is increased now. Need to investigate...

  • This PR:
   text	   data	    bss	    dec	    hex	filename
  10628	    112	   2408	  13148	   335c	/work/riot/RIOT/examples/hello-world/bin/hifive1b/hello-world.elf
  • master:
   text	   data	    bss	    dec	    hex	filename
  10592	    112	   2408	  13112	   3338	/work/riot/RIOT/examples/hello-world/bin/hifive1b/hello-world.elf

Testing procedure

  • A green CI
  • Run an application with the shell, it should still work

Issues/PRs references

None

@aabadie aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Dec 10, 2019
@aabadie aabadie requested a review from fjmolinas December 10, 2019 13:25
@aabadie aabadie force-pushed the pr/cpu/fe310_uart_rework branch from 9e942cc to 07a1d34 Compare December 10, 2019 14:29
@kaspar030
Copy link
Contributor

kaspar030 commented Dec 10, 2019

Force pushing causes github notification links to go stale. :(
Do you mind going with regular fixup commits?

kaspar030
kaspar030 reviewed Dec 10, 2019
View reviewed changes
@aabadie
Copy link
Contributor Author

aabadie commented Dec 10, 2019

Do you mind going with regular fixup commits?

Sorry, some racy workflow: since I got no review yet and noticed failures on Murdock, I thought it was ok.

@aabadie aabadie force-pushed the pr/cpu/fe310_uart_rework branch from 07a1d34 to 7aa7cec Compare December 10, 2019 17:58
@aabadie aabadie mentioned this pull request Dec 12, 2019
Merged
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@aabadie aabadie mentioned this pull request Dec 13, 2019
Closed
@fjmolinas
Copy link
Contributor

fjmolinas commented Dec 15, 2019

@aabadie you have pushed a lot to this one, is this WIP or ready for review?

@aabadie
Copy link
Contributor Author

aabadie commented Dec 15, 2019

It's not wip anymore and should be easy to review/test if you have the hardware :)

@aabadie aabadie force-pushed the pr/cpu/fe310_uart_rework branch from e8d611c to 4006e89 Compare December 16, 2019 12:39
@aabadie
Copy link
Contributor Author

aabadie commented Dec 16, 2019

rebased (there was a conflict because #12902 was just merged)

fjmolinas
fjmolinas reviewed Dec 20, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, reduces code duplication and is more inline with out coding styles. One minor comment otherwise looks good. Tested with tests/shell and tests/stdin.

  • Master:
text    data     bss     dec     hex filename
  11746     136    2472   14354    3812 /data/riotbuild/riotbase/tests/stdin/bin/hifive1b/tests_stdin.elf
  • PR:
   text    data     bss     dec     hex filename
  11730     136    2472   14338    3802 /data/riotbuild/riotbase/tests/stdin/bin/hifive1b/tests_stdin.elf

cpu/fe310/periph/uart.c
for (size_t i = 0; i < len; i++) {
/* Wait for FIFO to empty */
while ((_REG32(uart_config[dev].addr, UART_REG_TXFIFO) & UART_TXFIFO_FULL)
== (uint32_t)UART_TXFIFO_FULL) {};
Copy link
Contributor

@fjmolinas fjmolinas Dec 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cast needed?

Copy link
Contributor Author

@aabadie aabadie Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it doesn't build without ("error: comparison of integer expressions of different signedness")

Copy link
Contributor

@fjmolinas fjmolinas Dec 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!

@aabadie
Copy link
Contributor Author

aabadie commented Dec 20, 2019

Ok to squash @fjmolinas ?

Regarding the code size increase, it will be greatly compensated when #12934 is merged.

@fjmolinas
Copy link
Contributor

fjmolinas commented Dec 20, 2019

please squash!

@aabadie aabadie force-pushed the pr/cpu/fe310_uart_rework branch from 6d353ba to 8768242 Compare December 20, 2019 14:22
fjmolinas
fjmolinas approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK!

@fjmolinas fjmolinas merged commit c9955f9 into RIOT-OS:master Dec 20, 2019
@aabadie aabadie deleted the pr/cpu/fe310_uart_rework branch December 20, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@fjmolinas fjmolinas fjmolinas approved these changes

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @kaspar030 @fjmolinas