<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"></div><div class="gmail_default" style="font-size:small">On Fri, Apr 10, 2020 at 9:18 PM Christian Mauderer <<a href="mailto:oss@c-mauderer.de">oss@c-mauderer.de</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Niteesh,<br>
<br>
sorry for the late review. It somehow slipped my attention.<br>
<br>
Although I'm generally OK with it, my suggestion would be to move it<br>
after the release. It's a not strictly necessary change and therefore I<br>
don't want to potentially break stuff with it now.<br></blockquote><div><span class="gmail_default" style="font-size:small">Okay then, I will make the changes and will send the patches after the release.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I added some comments below:<br>
<br>
On 23/03/2020 11:28, G S Niteesh Babu wrote:<br>
> This patch updates the console driver to new Termios API.<br>
> <br>
> Update #3034<br>
> ---<br>
>  bsps/arm/beagle/console/console-config.c | 117 +++++++++++++----------<br>
>  c/src/lib/libbsp/arm/beagle/Makefile.am  |   4 +-<br>
>  2 files changed, 69 insertions(+), 52 deletions(-)<br>
> <br>
> diff --git a/bsps/arm/beagle/console/console-config.c b/bsps/arm/beagle/console/console-config.c<br>
> index 78af5f6a93..b2246571f8 100644<br>
> --- a/bsps/arm/beagle/console/console-config.c<br>
> +++ b/bsps/arm/beagle/console/console-config.c<br>
> @@ -21,12 +21,15 @@<br>
>   *<br>
>   * Modified by Ben Gras <<a href="mailto:beng@shrike-systems.com" target="_blank">beng@shrike-systems.com</a>> to make<br>
>   * interrupt-driven uart i/o work for beagleboards; beaglebone support added.<br>
> + *<br>
> + * Modified by Niteesh Babu G S <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>><br>
> + * Update console to new termios API.<br>
>   */<br>
>  <br>
> -#include <libchip/serial.h><br>
>  #include <libchip/ns16550.h><br>
>  <br>
>  #include <rtems/bspIo.h><br>
> +#include <rtems/console.h><br>
>  <br>
>  #include <bsp.h><br>
>  #include <bsp/irq.h><br>
> @@ -41,6 +44,8 @@<br>
>  #define TX_FIFO_E (1<<5)<br>
>  #define RX_FIFO_E (1<<0)<br>
>  <br>
> +#define BEAGLE_UART0 "/dev/ttyS0"<br>
> +<br>
>  static uint8_t beagle_uart_get_register(uintptr_t addr, uint8_t i)<br>
>  {<br>
>    uint8_t v;<br>
> @@ -65,77 +70,91 @@ static void beagle_uart_set_register(uintptr_t addr, uint8_t i, uint8_t val)<br>
>    reg [i] = val;<br>
>  }<br>
>  <br>
> -console_tbl Console_Configuration_Ports [] = {<br>
> -    {<br>
> -      .sDeviceName = "/dev/ttyS0",<br>
> -      .deviceType = SERIAL_NS16550,<br>
> -#if CONSOLE_POLLED   /* option to facilitate running the tests */<br>
> -      .pDeviceFns = &ns16550_fns_polled,<br>
> -#else<br>
> -      .pDeviceFns = &ns16550_fns,<br>
> -#endif<br>
> -      .ulMargin = 16,<br>
> -      .ulHysteresis = 8,<br>
> -      .pDeviceParams = (void *) CONSOLE_BAUD,<br>
> -      .ulCtrlPort1 = BSP_CONSOLE_UART_BASE,<br>
> -      .ulDataPort = BSP_CONSOLE_UART_BASE,<br>
> -      .ulIntVector = BSP_CONSOLE_UART_IRQ,<br>
> -      .getRegister = beagle_uart_get_register,<br>
> -      .setRegister = beagle_uart_set_register,<br>
> -      .ulClock = UART_CLOCK,  /* 48MHz base clock */<br>
> -    },<br>
> -};<br>
> -<br>
> -unsigned long Console_Configuration_Count = 1;<br>
> -<br>
> -static int init_needed = 1; // don't rely on bss being 0<br>
> +static ns16550_context uart_context;<br>
> +static void uart_write_polled( char c );<br>
>  <br>
>  static void beagle_console_init(void)<br>
>  {<br>
> -  if(init_needed) {<br>
> -    const uint32_t div = UART_CLOCK / 16 / CONSOLE_BAUD;<br>
> -    CONSOLE_SYSC = 2;<br>
> -    while ((CONSOLE_SYSS & 1) == 0)<br>
> -      ;<br>
> -    if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) == CONSOLE_LSR_THRE) {<br>
> -      CONSOLE_LCR = 0x83;<br>
> -      CONSOLE_DLL = div;<br>
> -      CONSOLE_DLM = (div >> 8) & 0xff;<br>
> -      CONSOLE_LCR = 0x03;<br>
> -      CONSOLE_ACR = 0x00;<br>
> -    }<br>
> +  ns16550_context *ctx;<br>
> +  static bool initialized = false;<br>
>  <br>
> -    while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0)<br>
> -      ;<br>
> +  if ( initialized ) {<br>
<br>
You never set initialized to true. So this will never happen.<br>
<br>
> +    return ;<br>
> +  }<br>
>  <br>
> -    CONSOLE_LCR = 0x80 | 0x03;<br>
> -    CONSOLE_DLL = 0x00;<br>
> -    CONSOLE_DLM = 0x00;<br>
> -    CONSOLE_LCR = 0x03;<br>
> -    CONSOLE_MCR = 0x03;<br>
> -    CONSOLE_FCR = 0x07;<br>
> +  /*<br>
> +   * Don't rely on BSS being zeroed<br>
> +   */<br>
> +  memset(&uart_context, 0, sizeof(uart_context));<br>
> +  ctx = &uart_context;<br>
> +<br>
> +  ctx->port = BSP_CONSOLE_UART_BASE;<br>
> +  ctx->irq = BSP_CONSOLE_UART_IRQ;<br>
> +  ctx->set_reg = beagle_uart_set_register;<br>
> +  ctx->get_reg = beagle_uart_get_register;<br>
> +  ctx->initial_baud = CONSOLE_BAUD;<br>
> +  ctx->clock = UART_CLOCK;<br>
> +<br>
> +  const uint32_t div = UART_CLOCK / 16 / CONSOLE_BAUD;<br>
<br>
I would prefer declarations at the top of the function.<br>
<br>
> +  CONSOLE_SYSC = 2;<br>
> +  while ((CONSOLE_SYSS & 1) == 0)<br>
> +    ;<br>
> +  if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) == CONSOLE_LSR_THRE) {<br>
>      CONSOLE_LCR = 0x83;<br>
>      CONSOLE_DLL = div;<br>
>      CONSOLE_DLM = (div >> 8) & 0xff;<br>
>      CONSOLE_LCR = 0x03;<br>
>      CONSOLE_ACR = 0x00;<br>
> -    init_needed = 0;<br>
>    }<br>
> +  while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0)<br>
> +    ;<br>
> +  CONSOLE_LCR = 0x80 | 0x03;<br>
> +  CONSOLE_DLL = 0x00;<br>
> +  CONSOLE_DLM = 0x00;<br>
> +  CONSOLE_LCR = 0x03;<br>
> +  CONSOLE_MCR = 0x03;<br>
> +  CONSOLE_FCR = 0x07;<br>
> +  CONSOLE_LCR = 0x83;<br>
> +  CONSOLE_DLL = div;<br>
> +  CONSOLE_DLM = (div >> 8) & 0xff;<br>
> +  CONSOLE_LCR = 0x03;<br>
> +  CONSOLE_ACR = 0x00;<br>
> +<br>
> +  BSP_output_char = uart_write_polled;<br>
>  }<br>
>  <br>
>  #define CONSOLE_THR8 (*(volatile uint8_t *) (BSP_CONSOLE_UART_BASE + 0x00))<br>
>  <br>
>  static void uart_write_polled( char c )<br>
>  {<br>
> -  if(init_needed) beagle_console_init();<br>
> -<br>
>    while( ( CONSOLE_LSR & TX_FIFO_E ) == 0 )<br>
>      ;<br>
>    CONSOLE_THR8 = c;<br>
>  }<br>
>  <br>
> +rtems_status_code console_initialize(<br>
> +  rtems_device_major_number major,<br>
> +  rtems_device_minor_number minor,<br>
> +  void *arg<br>
> +)<br>
> +{<br>
> +  rtems_termios_initialize();<br>
> +<br>
> +  beagle_console_init();<br>
> +<br>
> +  rtems_termios_device_install(<br>
> +    BEAGLE_UART0,<br>
> +    &ns16550_handler_polled,<br>
> +    NULL,<br>
> +    &uart_context.base<br>
> +  );<br>
> +<br>
> +  return RTEMS_SUCCESSFUL;<br>
> +}<br>
> +<br>
>  static void _BSP_put_char( char c ) {<br>
<br>
Maybe in this case it would be more correct to call it something like<br>
"early_uart_write_polled" or "init_uart_and_write_polled" or<br>
"uart_write_polled_first". This makes it a bit more clear what the<br>
function does and that it is only called once.<br>
<br>
> -   uart_write_polled( c );<br>
> +  beagle_console_init();<br>
> +  uart_write_polled( c );<br>
>  }<br>
>  <br>
>  static int _BSP_get_char(void)<br>
> diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am b/c/src/lib/libbsp/arm/beagle/Makefile.am<br>
> index e37472373c..1cd9920b2f 100644<br>
> --- a/c/src/lib/libbsp/arm/beagle/Makefile.am<br>
> +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am<br>
> @@ -62,9 +62,7 @@ librtemsbsp_a_SOURCES += ../../../../../../bsps/shared/irq/irq-default-handler.c<br>
>  librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/beagle/irq/irq.c<br>
>  <br>
>  # Console<br>
> -librtemsbsp_a_SOURCES += ../../../../../../bsps/shared/dev/serial/legacy-console.c<br>
> -librtemsbsp_a_SOURCES += ../../../../../../bsps/shared/dev/serial/legacy-console-control.c<br>
> -librtemsbsp_a_SOURCES += ../../../../../../bsps/shared/dev/serial/legacy-console-select.c<br>
> +librtemsbsp_a_SOURCES += ../../../../../../bsps/shared/dev/serial/console-termios.c<br>
>  librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/beagle/console/console-config.c<br>
>  <br>
>  # I2C<br>
> <br>
</blockquote></div></div>