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 <b...@shrike-systems.com> to make
> * interrupt-driven uart i/o work for beagleboards; beaglebone support added.
> + *
> + * Modified by Niteesh Babu G S <niteesh...@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
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
