On Mon, Dec 30, 2019 at 6:56 PM Christian Mauderer <l...@c-mauderer.de> wrote:
> On 30/12/2019 09:21, Niteesh wrote: > > I just looked at the dts files for rpi4, the uart 0, uart 1 are the > > same, the additional 4 uarts are in different pins and can be used > > simultaneously. They are all pl011 and are disabled by default, and are > > enabled using overlays in linux. Do you want to handle then now or > > sometime later? > > I think currently you only have a RPi3 to test so optimize for that > case. I'm not even sure whether the RPi4 would be binary compatible? > > > > > > > On Mon, 30 Dec, 2019, 1:20 PM Niteesh, <gsnb...@gmail.com > > <mailto:gsnb...@gmail.com>> wrote: > > > > On Mon, 30 Dec, 2019, 2:07 AM Christian Mauderer, > > <l...@c-mauderer.de <mailto:l...@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...@gmail.com > > <mailto:gsnb...@gmail.com> > > > <mailto: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. > > > > 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? > > That is OK. Handling for the multiple UARTS in RPi4 can be added later. > It wouldn't be a huge change and it's a lot simpler if you already have > a running support for the RPi3. > > > > > > > > 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? > > Is there a reason that you translate from uart0 to ttyS0? You could just > name them tty<fdt-alias> so you would get ttyuart0 and ttyuart1 or > ttyserial0 and ttyserial1. > I thought it is how we name the serial drivers. > How do you go from the compatible string to the aliases in the fdt? Or > do you search for the aliases and only check the compatible? > the register_uart function's search the aliases for a compatible device. Is there any chance we can create names that can be used to parse the > cmdline.txt directly? I found the namings ttyAMA0 and serial0 in there: > https://elinux.org/RPi_cmdline.txt and > https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md I really didn't get you, can you elaborate a bit more. > > > > > > > > > > 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. > > OK. > > > > > 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?? > > Best to take a look at the official documentation: > > > https://git.rtems.org/rtems/tree/cpukit/include/libfdt.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n991 > > Basically it searches for a node that has a 'compatible' property > matching the one you pass. Similar functions are used in FreeBSD to > check whether the driver wants to drive a hardware or not. > > If used in a loop this allows to search for all fdt nodes that can be > handled by the driver and create one instance for each of them. That > would be useful for the rpi4 case you mentioned. Basically you would do > the following in the probe function: > > - get fdt > - loop over all compatible nodes > - allocate a context > - register driver for that node > I totally forgot about the compatible property, I get it now, but thanks. > It might gets interesting when trying to set up the polled console. That > might need a bit of a special handling. So I would suggest to ignore > that for now and only initialize the one instance used in RPi1 to 3. You > don't have to cover all cases in one patch ;-) > > > > > > > > > > > > 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. > > > > Ah, interesting question: > > tl;dr: Basically during system initialization after some other drivers. > If some other sysinit items want to use a printk it might doesn't work. > > Long version: > > console_initialize is put in a "CONSOLE_DRIVER_TABLE_ENTRY" in > cpukit/include/rtems/console.h. That one is used in > cpukit/include/rtems/confdefs.h in the _IO_Driver_address_table. The > initialize functions of the table are called in rtems_io_initialize > (cpukit/sapi/src/ioinitialize.c) which is called from > _IO_Initialize_all_drivers (cpukit/sapi/src/io.c). That one is called > via a RTEMS_SYSINIT_ITEM: > > > RTEMS_SYSINIT_ITEM( > _IO_Initialize_all_drivers, > RTEMS_SYSINIT_DEVICE_DRIVERS, > RTEMS_SYSINIT_ORDER_MIDDLE > ); > > > This linker set is parsed during rtems_initialize_executive > (cpukit/sapi/src/exinit.c) which is called from boot_card > (bsps/shared/start/bootcard.c) which is called from the assembler > initialization code. So not the shortest one to find ;-) > > The order is a bit late. Some drivers might try to initialize earlier. > Therefore the initialization might is too late and a printk doesn't work > for another driver. Maybe that's the reason why imx put it in > SYSINIT_BSP_START which is quite a bit before SYSINIT_DEVICE_DRIVERS: > > > https://git.rtems.org/rtems/tree/cpukit/include/rtems/sysinit.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n29 >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
