And also should I change everything else to mini_uart or just leave
it as mini, for example, init_ctx_mini or init_ctx_mini_uart.
mini_uart seems good to me. So should I change everything to
mini_uart_context, output_char_mini_uart or leave them as
mini_context, mini_uart?
On Fri, Jan 17, 2020 at 7:27 PM Niteesh <gsnb...@gmail.com> wrote:
> On Fri, Jan 17, 2020 at 1:40 AM Christian Mauderer <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?
>
> > #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?
>
> > #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
