RTEMS | dev/serial: Refactor the pl011 driver to be extensible (!47)
Gedare Bloom (@gedare)
gitlab at rtems.org
Thu Aug 15 15:32:30 UTC 2024
Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47 was reviewed by Gedare Bloom
--
Gedare Bloom started a new discussion on bsps/include/dev/serial/arm-pl011.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111071
>
> -extern const rtems_termios_device_handler arm_pl011_fns;
> +int arm_pl011_read_polled(rtems_termios_device_context *base);
I think these functions / this file should have doxygen comments.
--
Gedare Bloom started a new discussion on bsps/include/dev/serial/arm-pl011.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111072
> +#endif
> +
> +extern const rtems_termios_device_handler arm_pl011_fns;
I think this object is only used in the `arm-pl011.c` file. Avoid declaring a global symbol here, and instead declare it in that file (with `static`). It is good practice to reduce the number of global symbols.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111073
>
> -static volatile pl011 *pl011_get_regs(rtems_termios_device_context *base)
> +static inline char arm_pl011_read_char(volatile pl011_base *regs_base)
I don't think `volatile` is needed for any of these uses of `pl011_base`.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111074
> +}
> +
> +volatile pl011_base *arm_pl011_get_regs(rtems_termios_device_context *base)
This function should just be:
`return ((arm_pl011_context *)base)->regs`
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111075
> + const uint32_t irqs = regs->uartmis;
> + /* RXFIFO got data */
> + const uint32_t rx_irq_mask = PL011_UARTI_RTI | PL011_UARTI_RXI;
remove `const` and let the compiler figure it out on its own.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111076
> + if ((irqs & rx_irq_mask) != 0) {
> + arm_pl011_clear_irq(regs, rx_irq_mask);
> + char buffer[ARM_PL011_FIFO_DEPTH];
Do not declare variables inside blocks like this. Prefer to declare at the start of the block, or usually the start of the function.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111077
> +
> + /* enable FIFO */
> + lcrh = lcrh | PL011_UARTLCR_H_FEN;
can merge this:
`uint32_t lcrh = PL011_UARTLCR_H_FEN;`
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111078
> + switch (term->c_cflag & CSIZE) {
> + case CS5:
> + lcrh = PL011_UARTLCR_H_WLEN_SET(lcrh, PL011_UARTLCR_H_WLEN_5);
shouldn't all these be `|=` not `=`? Otherwise you are destroying the above `lcrh`
How do you test this?
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111079
> + rx_enabled = (term->c_cflag & CREAD) != 0;
> + if (rx_enabled)
> + cr |= PL011_UARTCR_RXE;
I would eliminate the local variable and write this like the above lines:
```
if ((term->c_cflag & CREAD) != 0)
cr |= PL011_UARTCR_UARTEN | PL011_UARTCR_TXE;
```
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111080
> +
> + err = arm_pl011_compute_baudrate_params(&ibrd,&fbrd,baud,context->clock,3);
> + if (err != 0) return false;
don't merge lines
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111081
> + * transmiter is active.
> + */
> + while ((regs->uartfr & PL011_UARTFR_TXFE) == 0 ||
here you need `volatile`.
--
Gedare Bloom commented on a discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111082
> - arm_pl011_context *ctx = (arm_pl011_context *) base;
> +#ifndef BSP_CONSOLE_USE_INTERRUPTS
> + const
remove the `const`.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111083
> } else {
> - return PL011_UARTDR_DATA_GET(regs->uartdr);
> + return arm_pl011_read_char(regs);
this is casting a `char` to an `int`, it will get sign extension if it can be extended ASCII or UTF-8. confirm this is not possible, or fix it.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111084
> - while ((regs->uartfr & PL011_UARTFR_TXFF) != 0) {
> + /* Wait until TXFIFO has space */
> + while (arm_pl011_is_txfifo_full(regs)) {
this does need `volatile`.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111085
> + rtems_termios_device_context *base,
> + const char *buffer,
> + size_t n
variable name, `buffer_len` or similar is better.
--
Gedare Bloom started a new discussion on bsps/shared/dev/serial/arm-pl011.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47#note_111086
> + */
> + while (arm_pl011_is_txfifo_full(regs));
> + while (!arm_pl011_is_txfifo_full(regs) && i < n) {
these do need `volatile`
--
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/47
You're receiving this email because of your account on gitlab.rtems.org.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20240815/d935710f/attachment-0001.htm>
More information about the bugs
mailing list