[PATCH v3] Raspberrypi: updated the console interface.

[Niteesh]

On Mon, 30 Dec, 2019, 2:07 AM Christian Mauderer, <list at c-mauderer.de>
wrote:

> 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.

I just checked the uarts for rpi4, it has 6uarts not sure which model they
are though.
My idea is to call all the uart register functions, register_pl011,
register_aux or register_xxx inside uart_probe which will see if they can
handle that particular node. If not they just return. The FB is registered
seperately in the console_initialise. Are you okay with or would like to
change something?

>
> >     char uart_instance[20];
> >
>
> A global just to pass a name around? Would it be possible to search for
> a more elegant solution?
>
I first was setting the name for the uarts from the fdt property itself. In
fdt, the pl011 is uart0 and aux is uart1. So the correct path name is set
from the fdt property itself. For eg: if the current node is uart0 it will
be /dev/ttyS0 and ttyS1 if uart1. Is this approach fine?

> >
> >     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 did checkout the dts file from FreeBSD, it does use the same path
structure as imx.

> 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.
>
Can we please explain what does fdt_node_offset_by_compatible do??

> >
> >
> >     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?
>
When is console_initialize called. I first had the uart probe in
console_initialize. But the console_initialize was never called. So I moved
it to output char.

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20191230/727bee12/attachment.html>