[PATCH v3] Raspberrypi: updated the console interface.

[Christian Mauderer]

I assume you want suggestions regarding this code rather than the
original patch?

One suggestion: If you post code please try to use a mail program that
keeps the indentation. It's hard to read in this form. But that's most
likely not the suggestion you searched for ;-)

On 29/12/2019 19:48, Niteesh wrote:
> I am not happy with my code at all, I can please provide some suggestions.
> 
> On Mon, Dec 30, 2019 at 12:17 AM Niteesh <gsnb.gn at gmail.com
> <mailto:gsnb.gn at gmail.com>> wrote:
> 
>     arm_pl011_context pl011_context;
>     rpi_fb_context fb_context;

Is it sure that we only ever have one UART of each type? Otherwise some
dynamic handling (allocating the memory for the context) might would be
necessary. I think the chip has only these two modules? In that case
this is OK for now.

>     char uart_instance[20];
> 

A global just to pass a name around? Would it be possible to search for
a more elegant solution?

> 
>     static void output_char_serial(char c)
>     {
>     arm_pl011_write_polled(&pl011_context.base, c);
>     }
> 
>     void output_char_fb(char c)
>     {
>     fbcons_write_polled(&fb_context.base, c);
>     }
> 
> 
>     static void *get_reg_of_node(const void *fdt, int node)
>     {
>     int len;
>     const uint32_t *val;
> 
>     val = fdt_getprop(fdt, node, "reg", &len);
>     if (val == NULL || len < 4) {
>     return NULL;
>     }
> 
>     return (void *) fdt32_to_cpu(val[0]);
>     }
> 
>     static void arm_pl011_init_ctx(
>     const void *fdt,
>     const char *serial
>     )
>     {
>     arm_pl011_context *ctx = &pl011_context;
>     int node;
> 
>     if (strcmp(serial, "uart0") == 0) {
> 
>     rtems_termios_device_context_initialize(&ctx->base, "UART");
>     node = fdt_path_offset(fdt, serial);
>     ctx->regs = get_reg_of_node(fdt, node);
>     }
>     }
> 
>     static void console_select( const char *console )
>     {
>     const char *opt;
> 
>     opt = rpi_cmdline_get_arg("--console=");
> 
>     if ( opt ) {
>     if ( strncmp( opt, "fbcons", sizeof( "fbcons" ) - 1 ) == 0 ) {
>     if ( rpi_video_is_initialized() > 0 ) {
>     strcpy(uart_instance, "/dev/fbcons");
>     BSP_output_char = output_char_fb;
>     }
>     }else{
> 
>     if ( console == NULL ){
>     bsp_fatal( BSP_FATAL_CONSOLE_NO_DEV );
>     }
>     BSP_output_char = output_char_serial;
>     strcpy(uart_instance, "/dev/ttyS0");
>     }
>     }
>     }
> 
>     static void uart_probe(void)
>     {
>     const void *fdt;
>     const char *console;
>     int node;
> 
>     fdt = bsp_fdt_get();
>     node = fdt_path_offset(fdt, "/chosen");
> 
>     console = fdt_getprop(fdt, node, "stdout-path", NULL);
> 
>     node = fdt_path_offset(fdt, "/aliases");
> 
>     int offset = fdt_first_property_offset(fdt, node);
> 
>     while (offset >= 0) {
>     const struct fdt_property *prop;
>     prop = fdt_get_property_by_offset(fdt, offset, NULL);
>     if (prop != NULL) {
>     const char *name;
> 
>     name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
>     if(strstr(name, "serial") != NULL) {
>     const char *serial;
>     serial = prop->data;
> 
>     arm_pl011_init_ctx(fdt, serial);
>     }
>     }
> 
>     offset = fdt_next_property_offset(fdt, offset);
>     }
>     console_select(console);
>     }

I assume that you copied most of that from the imx uart? For the
raspberry I would suggest to search for a node that is compatible with
the device. Use fdt_node_offset_by_compatible(...) for that. I'm not
sure whether there is the same path structure in the raspberry fdt like
in the imx fdt (with a "chosen" path and a "stdout-path").

I'm not sure whether fdt_node_offset_by_compatible(...) delivers only
eneabled ones so you may have to loop over all compatible nodes and
search for an enabled one.

> 
> 
>     static void output_char(char c)
>     {
>     uart_probe();
>     (*BSP_output_char)(c);
>     }
> 

Seems like a hack in the imx BSP. I'm not sure whether it is a good
idea. Maybe it is possible to construct a case where uart_probe is
called twice.

>     rtems_status_code console_initialize(
>     rtems_device_major_number major,
>     rtems_device_minor_number minor,
>     void *arg
>     )
>     {
> 
>     rtems_termios_initialize();
>     rtems_termios_device_install(
>     "/dev/ttyS0",
>     &arm_pl011_fns,
>     NULL,
>     &pl011_context.base
>     );
>     rtems_termios_device_install(
>     "/dev/fbcons",
>     &fbcons_fns,
>     NULL,
>     &fb_context.base);
> 
>     link(uart_instance, CONSOLE_DEVICE_NAME);
> 
>     return RTEMS_SUCCESSFUL;
>     }
> 
>     BSP_output_char_function_type BSP_output_char = output_char;
> 
>     BSP_polling_getchar_function_type BSP_poll_char = NULL;
> 
>     RTEMS_SYSINIT_ITEM(
>     uart_probe,
>     RTEMS_SYSINIT_BSP_START,
>     RTEMS_SYSINIT_ORDER_LAST
>     );

Till now the console_initialize was able to do the frame buffer
initialization on the raspberry, correct? So maybe it would be possible
to just do the stuff from uart_probe in the console_initialize?