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...@gmail.com > <mailto:gsnb...@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? _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
