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