[PATCH] bsp/raspberrypi: AUX uart driver
Christian Mauderer
christian.mauderer at embedded-brains.de
Fri Jan 17 14:34:20 UTC 2020
On 17/01/2020 14:57, Niteesh wrote:
> On Fri, Jan 17, 2020 at 1:40 AM Christian Mauderer <list at c-mauderer.de
> <mailto:list at 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 at 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 at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
--
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer at 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.
More information about the devel
mailing list