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.
_______________________________________________ bugs mailing list bugs@rtems.org http://lists.rtems.org/mailman/listinfo/bugs
