[PATCH v3] bsp/beagle: Update console to new Termios API.
Christian Mauderer
oss at c-mauderer.de
Sat Apr 11 07:47:12 UTC 2020
On 10/04/2020 20:53, Niteesh G. S. wrote:
> On Fri, Apr 10, 2020 at 9:18 PM Christian Mauderer <oss at c-mauderer.de
> <mailto: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.
You can send it anytime before the release too so that we can merge it
right after it.
>
> 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
> <mailto: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
> <mailto: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
> >
>
More information about the devel
mailing list