On 17/01/2020 14:57, Niteesh wrote: > On Fri, Jan 17, 2020 at 1:40 AM Christian Mauderer <l...@c-mauderer.de > <mailto:l...@c-mauderer.de>> wrote: > > On 15/01/2020 13:05, G S Niteesh wrote: > > This patch adds the driver for aux driver present in Raspberry > > Pi 3 and above, this uart is currently used as the primary uart. > > Most of the time UART is written all upper case. > > > The AUX uart is similar to ns16550, it uses the libchip/ns16550 > > driver. > > --- > > bsps/arm/raspberrypi/console/console-config.c | 113 > ++++++++++++++++-- > > bsps/arm/raspberrypi/include/bsp/usart.h | 1 + > > 2 files changed, 107 insertions(+), 7 deletions(-) > > > > diff --git a/bsps/arm/raspberrypi/console/console-config.c > b/bsps/arm/raspberrypi/console/console-config.c > > index 48c4c6a3ec7..6ff018fb8ef 100644 > > --- a/bsps/arm/raspberrypi/console/console-config.c > > +++ b/bsps/arm/raspberrypi/console/console-config.c > > @@ -24,6 +24,7 @@ > > > > #include <libchip/serial.h> > > #include <libfdt.h> > > +#include <libchip/ns16550.h> > > > > #include <bspopts.h> > > #include <bsp/usart.h> > > @@ -34,35 +35,103 @@ > > #include <bsp/console-termios.h> > > #include <bsp/fdt.h> > > #include <bsp/fatal.h> > > +#include <bsp/gpio.h> > > +#include <bsp/rpi-gpio.h> > > > > - > > +/** > > + * UART0 - PL011 > > + * UART1 - AUX UART > > Isn't it called "Mini UART" in the peripheral manual? That would affect > all following "aux" too. > > > > > + */ > > #define UART0 "/dev/ttyS0" > > +#define UART1 "/dev/ttyS1" > > > Should I leave them as UART0 and UART1 or change them > to their respective peripheral names?
I would rename them. Random numbers are allways a bit confusing. > > > #define FBCONS "/dev/fbcons" > > > > > arm_pl011_context pl011_context; > > +ns16550_context aux_context; > > > > rpi_fb_context fb_context; > > > > -static void output_char_serial(char c) > > +static void output_char_pl011(char c) > > { > > arm_pl011_write_polled(&pl011_context.base, c); > > } > > > > +static void output_char_aux(char c) > > +{ > > + ns16550_polled_putchar(&aux_context.base, c); > > +} > > + > > void output_char_fb(char c) > > { > > fbcons_write_polled(&fb_context.base, c); > > } > > > > +static uint8_t raspberrypi_get_reg(uintptr_t port, uint8_t index) > > Is it a "raspberrypi_get_reg" or a "mini_uart_get_reg"? > > > +{ > > + volatile uint32_t *val = (volatile uint32_t *)port + index; > > + return (uint8_t) *val; > > +} > > + > > +static void raspberrypi_set_reg(uintptr_t port, uint8_t index, > uint8_t val) > > +{ > > + volatile uint32_t *reg = (volatile uint32_t *)port + index; > > + *reg = val; > > +} > > + > > static void init_ctx_arm_pl011( > > const void *fdt, > > int node > > ) > > { > > arm_pl011_context *ctx = &pl011_context; > > - rtems_termios_device_context_initialize(&ctx->base, "UART"); > > + rtems_termios_device_context_initialize(&ctx->base, "UART 0"); > > Maybe "PL011UART" would be a better name? > > > ctx->regs = raspberrypi_get_reg_of_node(fdt, node); > > } > > > > +static uint32_t calculate_baud_divisor( > > + ns16550_context *ctx, > > + uint32_t baud > > +) > > +{ > > + uint32_t baudDivisor = (ctx->clock / (8 * baud)) - 1; > > + return baudDivisor; > > +} > > + > > +static void init_ctx_aux( > > + const void *fdt, > > + int node > > +) > > +{ > > + const char *status; > > + int len; > > + ns16550_context *ctx = &aux_context; > > Although your ctx is a global variable and therefore (most likely) zero > already: If you don't write all fields of a context (you missed for > example has_fractional_divider_register) please set it to zero > explicitly (with memset). It would be possible that someone allocates > that context for example for rpi4 and forgets it. That leads to odd > effects that only happen sometimes. > > > + rtems_termios_device_context_initialize(&ctx->base, "UART 1"); > > I would suggest "MiniUART" for the name. It's just easier if you debug > something and you get told "it's this or that hardware module" without > having to look at some mapping. > > > + > > + status = fdt_getprop(fdt, node, "status", &len); > > + if (status == NULL){ > > + return ; > > + } > > + > > + if ( strcmp(status, "disabled") == 0 ) { > > + return ; > > + } > > You could put that into one if. Would be a bit shorter to read: > > if (status == NULL || strcmp(status, "disabled") == 0) { > return; > } > > > + > > + ctx->port = (uintptr_t) raspberrypi_get_reg_of_node(fdt, node); > > + ctx->initial_baud = AUX_DEFAULT_BAUD; > > + ctx->clock = BCM2835_CLOCK_FREQ; > > + ctx->calculate_baud_divisor = calculate_baud_divisor; > > + ctx->get_reg = raspberrypi_get_reg; > > + ctx->set_reg = raspberrypi_set_reg; > > + > > + rtems_gpio_bsp_select_specific_io(0, 14, RPI_ALT_FUNC_5, NULL); > > + rtems_gpio_bsp_select_specific_io(0, 15, RPI_ALT_FUNC_5, NULL); > > + rtems_gpio_bsp_set_resistor_mode(0, 14, NO_PULL_RESISTOR); > > + rtems_gpio_bsp_set_resistor_mode(0, 15, NO_PULL_RESISTOR); > > + > > + BCM2835_REG(AUX_ENABLES) |= 0x1; > > + ns16550_probe(&ctx->base); > > +} > > + > > static void register_fb( void ) > > { > > if (fbcons_probe(&fb_context.base) == true) { > > @@ -87,16 +156,27 @@ static void console_select( void ) > > link(FBCONS, CONSOLE_DEVICE_NAME); > > return ; > > } > > + } else if ( strncmp( opt, UART1, sizeof(UART1) - 1 ) == 0) { > > Hm. UART1 is "/dev/ttyS1". Linux calls the miniUART "/dev/ttyS0" and the > PL011 "/dev/ttyAMA0", right? So the command line will be different for > Linux and RTEMS? > > I think I would preferre to use the Linux names. > > > + BSP_output_char = output_char_aux; > > + link(UART1, CONSOLE_DEVICE_NAME); > > + } else if ( strncmp( opt, UART0, sizeof(UART0) - 1 ) == 0) { > > + BSP_output_char = output_char_pl011; > > + link(UART0, CONSOLE_DEVICE_NAME); > > } > > } > > - BSP_output_char = output_char_serial; > > - link(UART0, CONSOLE_DEVICE_NAME); > > + /** > > + * If no command line option was given, default to AUX uart. > > + */ > > + BSP_output_char = output_char_aux; > > + link(UART1, CONSOLE_DEVICE_NAME); > > Is it really a good idea to default to the AUX uart? The BSP is still > named raspberrypi2. So I think the default should match the RPi2. > > > } > > > > static void uart_probe(void) > > { > > static bool initialized = false; > > const void *fdt; > > + const char *console; > > + int len; > > int node; > > > > if ( initialized ) { > > @@ -104,17 +184,29 @@ static void uart_probe(void) > > } > > > > fdt = bsp_fdt_get(); > > - node = fdt_node_offset_by_compatible(fdt, -1, > "brcm,bcm2835-pl011"); > > > > + node = fdt_node_offset_by_compatible(fdt, -1, > "brcm,bcm2835-pl011"); > > init_ctx_arm_pl011(fdt, node); > > > > + node = fdt_node_offset_by_compatible(fdt, -1, > "brcm,bcm2835-aux-uart"); > > + init_ctx_aux(fdt, node); > > + > > + node = fdt_path_offset(fdt, "/aliases"); > > + console = fdt_getprop(fdt, node, "serial0", &len); > > + > > + if (strcmp(console, "/soc/serial@7e215040") == 0) { > > + BSP_output_char = output_char_aux; > > + }else { > > + BSP_output_char = output_char_pl011; > > + } > > + > > initialized = true; > > } > > > > static void output_char(char c) > > { > > uart_probe(); > > - output_char_serial(c); > > + (*BSP_output_char)(c); > > } > > > > rtems_status_code console_initialize( > > @@ -133,6 +225,13 @@ rtems_status_code console_initialize( > > &pl011_context.base > > ); > > > > + rtems_termios_device_install( > > + UART1, > > + &ns16550_handler_polled, > > + NULL, > > + &aux_context.base > > + ); > > + > > register_fb(); > > > > console_select(); > > diff --git a/bsps/arm/raspberrypi/include/bsp/usart.h > b/bsps/arm/raspberrypi/include/bsp/usart.h > > index abbf53626c9..c73cebc86bd 100644 > > --- a/bsps/arm/raspberrypi/include/bsp/usart.h > > +++ b/bsps/arm/raspberrypi/include/bsp/usart.h > > @@ -33,6 +33,7 @@ extern "C" { > > #endif /* __cplusplus */ > > > > #define PL011_DEFAULT_BAUD 115000 > > +#define AUX_DEFAULT_BAUD 115200 > > > I have also changed AUX_DEFAULT_BAUD to MINI_UART_DEFAULT_BAUD > is that okay? Yes. > > > #define BCM2835_PL011_BASE (RPI_PERIPHERAL_BASE + 0x201000) > > > > #ifdef __cplusplus > > > > I fixed everthing else, that you mentioned. > > _______________________________________________ > devel mailing list > devel@rtems.org > http://lists.rtems.org/mailman/listinfo/devel > -- -------------------------------------------- embedded brains GmbH Herr Christian Mauderer Dornierstr. 4 D-82178 Puchheim Germany email: christian.maude...@embedded-brains.de Phone: +49-89-18 94 741 - 18 Fax: +49-89-18 94 741 - 08 PGP: Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel