[PATCH v3] bsp/beagle: Update console to new Termios API.

Niteesh G. S. niteesh.gs at gmail.com
Fri Apr 10 18:53:36 UTC 2020


On Fri, Apr 10, 2020 at 9:18 PM Christian Mauderer <oss at c-mauderer.de>
wrote:

> Hello Niteesh,
>
> sorry for the late review. It somehow slipped my attention.
>
> Although I'm generally OK with it, my suggestion would be to move it
> after the release. It's a not strictly necessary change and therefore I
> don't want to potentially break stuff with it now.
>
Okay then, I will make the changes and will send the patches after the
release.

> I added some comments below:
>
> On 23/03/2020 11:28, G S Niteesh Babu wrote:
> > This patch updates the console driver to new Termios API.
> >
> > Update #3034
> > ---
> >  bsps/arm/beagle/console/console-config.c | 117 +++++++++++++----------
> >  c/src/lib/libbsp/arm/beagle/Makefile.am  |   4 +-
> >  2 files changed, 69 insertions(+), 52 deletions(-)
> >
> > diff --git a/bsps/arm/beagle/console/console-config.c
> b/bsps/arm/beagle/console/console-config.c
> > index 78af5f6a93..b2246571f8 100644
> > --- a/bsps/arm/beagle/console/console-config.c
> > +++ b/bsps/arm/beagle/console/console-config.c
> > @@ -21,12 +21,15 @@
> >   *
> >   * Modified by Ben Gras <beng at shrike-systems.com> to make
> >   * interrupt-driven uart i/o work for beagleboards; beaglebone support
> added.
> > + *
> > + * Modified by Niteesh Babu G S <niteesh.gs at gmail.com>
> > + * Update console to new termios API.
> >   */
> >
> > -#include <libchip/serial.h>
> >  #include <libchip/ns16550.h>
> >
> >  #include <rtems/bspIo.h>
> > +#include <rtems/console.h>
> >
> >  #include <bsp.h>
> >  #include <bsp/irq.h>
> > @@ -41,6 +44,8 @@
> >  #define TX_FIFO_E (1<<5)
> >  #define RX_FIFO_E (1<<0)
> >
> > +#define BEAGLE_UART0 "/dev/ttyS0"
> > +
> >  static uint8_t beagle_uart_get_register(uintptr_t addr, uint8_t i)
> >  {
> >    uint8_t v;
> > @@ -65,77 +70,91 @@ static void beagle_uart_set_register(uintptr_t addr,
> uint8_t i, uint8_t val)
> >    reg [i] = val;
> >  }
> >
> > -console_tbl Console_Configuration_Ports [] = {
> > -    {
> > -      .sDeviceName = "/dev/ttyS0",
> > -      .deviceType = SERIAL_NS16550,
> > -#if CONSOLE_POLLED   /* option to facilitate running the tests */
> > -      .pDeviceFns = &ns16550_fns_polled,
> > -#else
> > -      .pDeviceFns = &ns16550_fns,
> > -#endif
> > -      .ulMargin = 16,
> > -      .ulHysteresis = 8,
> > -      .pDeviceParams = (void *) CONSOLE_BAUD,
> > -      .ulCtrlPort1 = BSP_CONSOLE_UART_BASE,
> > -      .ulDataPort = BSP_CONSOLE_UART_BASE,
> > -      .ulIntVector = BSP_CONSOLE_UART_IRQ,
> > -      .getRegister = beagle_uart_get_register,
> > -      .setRegister = beagle_uart_set_register,
> > -      .ulClock = UART_CLOCK,  /* 48MHz base clock */
> > -    },
> > -};
> > -
> > -unsigned long Console_Configuration_Count = 1;
> > -
> > -static int init_needed = 1; // don't rely on bss being 0
> > +static ns16550_context uart_context;
> > +static void uart_write_polled( char c );
> >
> >  static void beagle_console_init(void)
> >  {
> > -  if(init_needed) {
> > -    const uint32_t div = UART_CLOCK / 16 / CONSOLE_BAUD;
> > -    CONSOLE_SYSC = 2;
> > -    while ((CONSOLE_SYSS & 1) == 0)
> > -      ;
> > -    if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) ==
> CONSOLE_LSR_THRE) {
> > -      CONSOLE_LCR = 0x83;
> > -      CONSOLE_DLL = div;
> > -      CONSOLE_DLM = (div >> 8) & 0xff;
> > -      CONSOLE_LCR = 0x03;
> > -      CONSOLE_ACR = 0x00;
> > -    }
> > +  ns16550_context *ctx;
> > +  static bool initialized = false;
> >
> > -    while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0)
> > -      ;
> > +  if ( initialized ) {
>
> You never set initialized to true. So this will never happen.
>
> > +    return ;
> > +  }
> >
> > -    CONSOLE_LCR = 0x80 | 0x03;
> > -    CONSOLE_DLL = 0x00;
> > -    CONSOLE_DLM = 0x00;
> > -    CONSOLE_LCR = 0x03;
> > -    CONSOLE_MCR = 0x03;
> > -    CONSOLE_FCR = 0x07;
> > +  /*
> > +   * Don't rely on BSS being zeroed
> > +   */
> > +  memset(&uart_context, 0, sizeof(uart_context));
> > +  ctx = &uart_context;
> > +
> > +  ctx->port = BSP_CONSOLE_UART_BASE;
> > +  ctx->irq = BSP_CONSOLE_UART_IRQ;
> > +  ctx->set_reg = beagle_uart_set_register;
> > +  ctx->get_reg = beagle_uart_get_register;
> > +  ctx->initial_baud = CONSOLE_BAUD;
> > +  ctx->clock = UART_CLOCK;
> > +
> > +  const uint32_t div = UART_CLOCK / 16 / CONSOLE_BAUD;
>
> I would prefer declarations at the top of the function.
>
> > +  CONSOLE_SYSC = 2;
> > +  while ((CONSOLE_SYSS & 1) == 0)
> > +    ;
> > +  if ((CONSOLE_LSR & (CONSOLE_LSR_THRE | CONSOLE_LSR_TEMT)) ==
> CONSOLE_LSR_THRE) {
> >      CONSOLE_LCR = 0x83;
> >      CONSOLE_DLL = div;
> >      CONSOLE_DLM = (div >> 8) & 0xff;
> >      CONSOLE_LCR = 0x03;
> >      CONSOLE_ACR = 0x00;
> > -    init_needed = 0;
> >    }
> > +  while ((CONSOLE_LSR & CONSOLE_LSR_TEMT) == 0)
> > +    ;
> > +  CONSOLE_LCR = 0x80 | 0x03;
> > +  CONSOLE_DLL = 0x00;
> > +  CONSOLE_DLM = 0x00;
> > +  CONSOLE_LCR = 0x03;
> > +  CONSOLE_MCR = 0x03;
> > +  CONSOLE_FCR = 0x07;
> > +  CONSOLE_LCR = 0x83;
> > +  CONSOLE_DLL = div;
> > +  CONSOLE_DLM = (div >> 8) & 0xff;
> > +  CONSOLE_LCR = 0x03;
> > +  CONSOLE_ACR = 0x00;
> > +
> > +  BSP_output_char = uart_write_polled;
> >  }
> >
> >  #define CONSOLE_THR8 (*(volatile uint8_t *) (BSP_CONSOLE_UART_BASE +
> 0x00))
> >
> >  static void uart_write_polled( char c )
> >  {
> > -  if(init_needed) beagle_console_init();
> > -
> >    while( ( CONSOLE_LSR & TX_FIFO_E ) == 0 )
> >      ;
> >    CONSOLE_THR8 = c;
> >  }
> >
> > +rtems_status_code console_initialize(
> > +  rtems_device_major_number major,
> > +  rtems_device_minor_number minor,
> > +  void *arg
> > +)
> > +{
> > +  rtems_termios_initialize();
> > +
> > +  beagle_console_init();
> > +
> > +  rtems_termios_device_install(
> > +    BEAGLE_UART0,
> > +    &ns16550_handler_polled,
> > +    NULL,
> > +    &uart_context.base
> > +  );
> > +
> > +  return RTEMS_SUCCESSFUL;
> > +}
> > +
> >  static void _BSP_put_char( char c ) {
>
> Maybe in this case it would be more correct to call it something like
> "early_uart_write_polled" or "init_uart_and_write_polled" or
> "uart_write_polled_first". This makes it a bit more clear what the
> function does and that it is only called once.
>
> > -   uart_write_polled( c );
> > +  beagle_console_init();
> > +  uart_write_polled( c );
> >  }
> >
> >  static int _BSP_get_char(void)
> > diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am
> b/c/src/lib/libbsp/arm/beagle/Makefile.am
> > index e37472373c..1cd9920b2f 100644
> > --- a/c/src/lib/libbsp/arm/beagle/Makefile.am
> > +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
> > @@ -62,9 +62,7 @@ librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/shared/irq/irq-default-handler.c
> >  librtemsbsp_a_SOURCES += ../../../../../../bsps/arm/beagle/irq/irq.c
> >
> >  # Console
> > -librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/shared/dev/serial/legacy-console.c
> > -librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/shared/dev/serial/legacy-console-control.c
> > -librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/shared/dev/serial/legacy-console-select.c
> > +librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/shared/dev/serial/console-termios.c
> >  librtemsbsp_a_SOURCES +=
> ../../../../../../bsps/arm/beagle/console/console-config.c
> >
> >  # I2C
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200411/66fc36ff/attachment-0001.html>


More information about the devel mailing list