On 10/04/2020 20:53, Niteesh G. S. wrote: > On Fri, Apr 10, 2020 at 9:18 PM Christian Mauderer <o...@c-mauderer.de > <mailto:o...@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 <b...@shrike-systems.com > <mailto: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 > <mailto: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
