[PATCH v3] bsp/beagle: Update console to new Termios API.
Christian Mauderer
oss at c-mauderer.de
Fri Apr 10 15:48:12 UTC 2020
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.
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
>
More information about the devel
mailing list