[dev][uart][pl011] Move to common driver #274
Conversation
|
Nice, looks like a good start. I'll do a review in a little bit but on pass, can you update the comment itself to include what it's doing? I'm not a fan at all of commit changes that just say it fixes a commit # without any context. Something like [dev][uart][pl011] Move to common driver And then a line or two describing what it did, including the bug fix and a mention that it fixes a bug #. |
|
i do have some further improvements to the uart driver on the vc4 fork, but trying to keep this commit clear of those, so its easier to see just the merging, and the improvements can come later |
merges the qemu and bcm28xx versions of the uart driver together fixed a bug that was present in the qemu version, that only reveals itself on real hw increase the fifo level interrupt to 7/8 to get more bytes/irq under high load fix bug littlekernel#272
cleverca22
changed the title
| @@ -0,0 +1,4 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Please add the standard license header here.
| } | ||
|
|
||
| static enum handler_return uart_irq(void *arg) { | ||
| bool resched = false; | ||
| uint port = (uintptr_t)arg; | ||
| uint port = (uint)arg; |
There was a problem hiding this comment.
This is an issue with 64bit targets, since its down casting to a smaller size. Perhaps cast it through uintptr_t first, then down to uint.
| void uart_init(void) { | ||
| for (size_t i = 0; i < NUM_UART; i++) { | ||
| uintptr_t base = uart_to_ptr(i); | ||
| void pl011_uart_init(int irq, int nr, uintptr_t base) { |
There was a problem hiding this comment.
To generally be consistent with existing apis, put the number first.
| // create circular buffer to hold received data | ||
| cbuf_initialize(&uart_rx_buf[i], RXBUF_SIZE); | ||
| // assumes interrupts are contiguous | ||
| register_int_handler(irq, &uart_irq, (void *)nr); |
There was a problem hiding this comment.
the (void *) is a warning on arm64. Perhaps as before, cast it through uintptr_t first.
| @@ -33,6 +32,7 @@ MODULE_DEPS += \ | |||
| dev/virtio/block \ | |||
| dev/virtio/gpu \ | |||
| dev/virtio/net \ | |||
| dev/pl011 \ | |||
There was a problem hiding this comment.
To be consistent, sort the list of the devs.
| @@ -42,8 +42,8 @@ SMP_CPU_ID_BITS := 8 | |||
| GLOBAL_DEFINES += \ | |||
| BCM2836=1 | |||
|
|
|||
| MODULE_SRCS += \ | |||
| $(LOCAL_DIR)/uart.c | |||
| MODULES += \ | |||
There was a problem hiding this comment.
Use MODULE_DEPS instead of MODULES. MODULES sometimes works but the _DEPS is the correct solution.
| @@ -0,0 +1,7 @@ | |||
| LOCAL_DIR := $(GET_LOCAL_DIR) | |||
There was a problem hiding this comment.
Please name this dev/uart/pl011 instead of at the top level of dev.
while merging the 2 drivers, i found a bug in the qemu variant of
uart_getcthat didnt reveal itself under qemu, and fixed iti also added some code for the other 4 PL011's in the pi4, but no target for that yet
confirmed to function on both
PROJECT=rpi2-testandscripts/do-qemuarm