<div dir="ltr"><div dir="ltr">On Mon, Dec 30, 2019 at 6:56 PM Christian Mauderer <<a href="mailto:list@c-mauderer.de">list@c-mauderer.de</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 30/12/2019 09:21, Niteesh wrote:<br>
> I just looked at the dts files for rpi4, the uart 0, uart 1 are the<br>
> same, the additional 4 uarts are in different pins and can be used<br>
> simultaneously. They are all pl011 and are disabled by default, and are<br>
> enabled using overlays in linux. Do you want to handle then now or<br>
> sometime later?<br>
<br>
I think currently you only have a RPi3 to test so optimize for that<br>
case. I'm not even sure whether the RPi4 would be binary compatible?<br>
<br>
> <br>
> <br>
> On Mon, 30 Dec, 2019, 1:20 PM Niteesh, <<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a><br>
> <mailto:<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a>>> wrote:<br>
> <br>
> On Mon, 30 Dec, 2019, 2:07 AM Christian Mauderer,<br>
> <<a href="mailto:list@c-mauderer.de" target="_blank">list@c-mauderer.de</a> <mailto:<a href="mailto:list@c-mauderer.de" target="_blank">list@c-mauderer.de</a>>> wrote:<br>
> <br>
> I assume you want suggestions regarding this code rather than the<br>
> original patch?<br>
> <br>
> One suggestion: If you post code please try to use a mail<br>
> program that<br>
> keeps the indentation. It's hard to read in this form. But<br>
> that's most<br>
> likely not the suggestion you searched for ;-)<br>
> <br>
> On 29/12/2019 19:48, Niteesh wrote:<br>
> > I am not happy with my code at all, I can please provide some<br>
> suggestions.<br>
> ><br>
> > On Mon, Dec 30, 2019 at 12:17 AM Niteesh <<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a><br>
> <mailto:<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a>><br>
> > <mailto:<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a> <mailto:<a href="mailto:gsnb.gn@gmail.com" target="_blank">gsnb.gn@gmail.com</a>>>> wrote:<br>
> ><br>
> > arm_pl011_context pl011_context;<br>
> > rpi_fb_context fb_context;<br>
> <br>
> Is it sure that we only ever have one UART of each type?<br>
> Otherwise some<br>
> dynamic handling (allocating the memory for the context) might<br>
> would be<br>
> necessary. I think the chip has only these two modules? In that case<br>
> this is OK for now.<br>
> <br>
> I just checked the uarts for rpi4, it has 6uarts not sure which<br>
> model they are though.<br>
> My idea is to call all the uart register functions, register_pl011,<br>
> register_aux or register_xxx inside uart_probe which will see if<br>
> they can handle that particular node. If not they just return. The<br>
> FB is registered seperately in the console_initialise. Are you okay<br>
> with or would like to change something?<br>
<br>
That is OK. Handling for the multiple UARTS in RPi4 can be added later.<br>
It wouldn't be a huge change and it's a lot simpler if you already have<br>
a running support for the RPi3.<br>
<br>
> <br>
> <br>
> > char uart_instance[20];<br>
> ><br>
> <br>
> A global just to pass a name around? Would it be possible to<br>
> search for<br>
> a more elegant solution?<br>
> <br>
> I first was setting the name for the uarts from the fdt property<br>
> itself. In fdt, the pl011 is uart0 and aux is uart1. So the correct<br>
> path name is set from the fdt property itself. For eg: if the<br>
> current node is uart0 it will be /dev/ttyS0 and ttyS1 if uart1. Is<br>
> this approach fine?<br>
<br>
Is there a reason that you translate from uart0 to ttyS0? You could just<br>
name them tty<fdt-alias> so you would get ttyuart0 and ttyuart1 or<br>
ttyserial0 and ttyserial1.<br></blockquote><div>I thought it is how we name the serial drivers. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
How do you go from the compatible string to the aliases in the fdt? Or<br>
do you search for the aliases and only check the compatible?<br></blockquote><div> </div><div>the register_uart function's search the aliases for a compatible device.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Is there any chance we can create names that can be used to parse the<br>
cmdline.txt directly? I found the namings ttyAMA0 and serial0 in there:<br>
<a href="https://elinux.org/RPi_cmdline.txt" rel="noreferrer" target="_blank">https://elinux.org/RPi_cmdline.txt</a> and<br>
<a href="https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md" rel="noreferrer" target="_blank">https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md</a></blockquote><div>I really didn't get you, can you elaborate a bit more.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
> <br>
> ><br>
> > static void output_char_serial(char c)<br>
> > {<br>
> > arm_pl011_write_polled(&pl011_context.base, c);<br>
> > }<br>
> ><br>
> > void output_char_fb(char c)<br>
> > {<br>
> > fbcons_write_polled(&fb_context.base, c);<br>
> > }<br>
> ><br>
> ><br>
> > static void *get_reg_of_node(const void *fdt, int node)<br>
> > {<br>
> > int len;<br>
> > const uint32_t *val;<br>
> ><br>
> > val = fdt_getprop(fdt, node, "reg", &len);<br>
> > if (val == NULL || len < 4) {<br>
> > return NULL;<br>
> > }<br>
> ><br>
> > return (void *) fdt32_to_cpu(val[0]);<br>
> > }<br>
> ><br>
> > static void arm_pl011_init_ctx(<br>
> > const void *fdt,<br>
> > const char *serial<br>
> > )<br>
> > {<br>
> > arm_pl011_context *ctx = &pl011_context;<br>
> > int node;<br>
> ><br>
> > if (strcmp(serial, "uart0") == 0) {<br>
> ><br>
> > rtems_termios_device_context_initialize(&ctx->base, "UART");<br>
> > node = fdt_path_offset(fdt, serial);<br>
> > ctx->regs = get_reg_of_node(fdt, node);<br>
> > }<br>
> > }<br>
> ><br>
> > static void console_select( const char *console )<br>
> > {<br>
> > const char *opt;<br>
> ><br>
> > opt = rpi_cmdline_get_arg("--console=");<br>
> ><br>
> > if ( opt ) {<br>
> > if ( strncmp( opt, "fbcons", sizeof( "fbcons" ) - 1 ) == 0 ) {<br>
> > if ( rpi_video_is_initialized() > 0 ) {<br>
> > strcpy(uart_instance, "/dev/fbcons");<br>
> > BSP_output_char = output_char_fb;<br>
> > }<br>
> > }else{<br>
> ><br>
> > if ( console == NULL ){<br>
> > bsp_fatal( BSP_FATAL_CONSOLE_NO_DEV );<br>
> > }<br>
> > BSP_output_char = output_char_serial;<br>
> > strcpy(uart_instance, "/dev/ttyS0");<br>
> > }<br>
> > }<br>
> > }<br>
> ><br>
> > static void uart_probe(void)<br>
> > {<br>
> > const void *fdt;<br>
> > const char *console;<br>
> > int node;<br>
> ><br>
> > fdt = bsp_fdt_get();<br>
> > node = fdt_path_offset(fdt, "/chosen");<br>
> ><br>
> > console = fdt_getprop(fdt, node, "stdout-path", NULL);<br>
> ><br>
> > node = fdt_path_offset(fdt, "/aliases");<br>
> ><br>
> > int offset = fdt_first_property_offset(fdt, node);<br>
> ><br>
> > while (offset >= 0) {<br>
> > const struct fdt_property *prop;<br>
> > prop = fdt_get_property_by_offset(fdt, offset, NULL);<br>
> > if (prop != NULL) {<br>
> > const char *name;<br>
> ><br>
> > name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));<br>
> > if(strstr(name, "serial") != NULL) {<br>
> > const char *serial;<br>
> > serial = prop->data;<br>
> ><br>
> > arm_pl011_init_ctx(fdt, serial);<br>
> > }<br>
> > }<br>
> ><br>
> > offset = fdt_next_property_offset(fdt, offset);<br>
> > }<br>
> > console_select(console);<br>
> > }<br>
> <br>
> I assume that you copied most of that from the imx uart? For the<br>
> raspberry I would suggest to search for a node that is<br>
> compatible with<br>
> the device. Use fdt_node_offset_by_compatible(...) for that. I'm not<br>
> sure whether there is the same path structure in the raspberry<br>
> fdt like<br>
> in the imx fdt (with a "chosen" path and a "stdout-path").<br>
> <br>
> I did checkout the dts file from FreeBSD, it does use the same path<br>
> structure as imx.<br>
<br>
OK.<br>
<br>
> <br>
> I'm not sure whether fdt_node_offset_by_compatible(...) delivers<br>
> only<br>
> eneabled ones so you may have to loop over all compatible nodes and<br>
> search for an enabled one.<br>
> <br>
> Can we please explain what does fdt_node_offset_by_compatible do??<br>
<br>
Best to take a look at the official documentation:<br>
<br>
<a href="https://git.rtems.org/rtems/tree/cpukit/include/libfdt.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n991" rel="noreferrer" target="_blank">https://git.rtems.org/rtems/tree/cpukit/include/libfdt.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n991</a><br>
<br>
Basically it searches for a node that has a 'compatible' property<br>
matching the one you pass. Similar functions are used in FreeBSD to<br>
check whether the driver wants to drive a hardware or not.<br>
<br>
If used in a loop this allows to search for all fdt nodes that can be<br>
handled by the driver and create one instance for each of them. That<br>
would be useful for the rpi4 case you mentioned. Basically you would do<br>
the following in the probe function:<br>
<br>
- get fdt<br>
- loop over all compatible nodes<br>
- allocate a context<br>
- register driver for that node<br></blockquote><div> </div><div>I totally forgot about the compatible property, I get it now, but thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It might gets interesting when trying to set up the polled console. That<br>
might need a bit of a special handling. So I would suggest to ignore<br>
that for now and only initialize the one instance used in RPi1 to 3. You<br>
don't have to cover all cases in one patch ;-)<br>
<br>
> <br>
> ><br>
> ><br>
> > static void output_char(char c)<br>
> > {<br>
> > uart_probe();<br>
> > (*BSP_output_char)(c);<br>
> > }<br>
> ><br>
> <br>
> Seems like a hack in the imx BSP. I'm not sure whether it is a good<br>
> idea. Maybe it is possible to construct a case where uart_probe is<br>
> called twice.<br>
> <br>
> > rtems_status_code console_initialize(<br>
> > rtems_device_major_number major,<br>
> > rtems_device_minor_number minor,<br>
> > void *arg<br>
> > )<br>
> > {<br>
> ><br>
> > rtems_termios_initialize();<br>
> > rtems_termios_device_install(<br>
> > "/dev/ttyS0",<br>
> > &arm_pl011_fns,<br>
> > NULL,<br>
> > &pl011_context.base<br>
> > );<br>
> > rtems_termios_device_install(<br>
> > "/dev/fbcons",<br>
> > &fbcons_fns,<br>
> > NULL,<br>
> > &fb_context.base);<br>
> ><br>
> > link(uart_instance, CONSOLE_DEVICE_NAME);<br>
> ><br>
> > return RTEMS_SUCCESSFUL;<br>
> > }<br>
> ><br>
> > BSP_output_char_function_type BSP_output_char = output_char;<br>
> ><br>
> > BSP_polling_getchar_function_type BSP_poll_char = NULL;<br>
> ><br>
> > RTEMS_SYSINIT_ITEM(<br>
> > uart_probe,<br>
> > RTEMS_SYSINIT_BSP_START,<br>
> > RTEMS_SYSINIT_ORDER_LAST<br>
> > );<br>
> <br>
> Till now the console_initialize was able to do the frame buffer<br>
> initialization on the raspberry, correct? So maybe it would be<br>
> possible<br>
> to just do the stuff from uart_probe in the console_initialize?<br>
> <br>
> When is console_initialize called. I first had the uart probe in<br>
> console_initialize. But the console_initialize was never called. So<br>
> I moved it to output char.<br>
> <br>
<br>
Ah, interesting question:<br>
<br>
tl;dr: Basically during system initialization after some other drivers.<br>
If some other sysinit items want to use a printk it might doesn't work.<br>
<br>
Long version:<br>
<br>
console_initialize is put in a "CONSOLE_DRIVER_TABLE_ENTRY" in<br>
cpukit/include/rtems/console.h. That one is used in<br>
cpukit/include/rtems/confdefs.h in the _IO_Driver_address_table. The<br>
initialize functions of the table are called in rtems_io_initialize<br>
(cpukit/sapi/src/ioinitialize.c) which is called from<br>
_IO_Initialize_all_drivers (cpukit/sapi/src/io.c). That one is called<br>
via a RTEMS_SYSINIT_ITEM:<br>
<br>
<br>
RTEMS_SYSINIT_ITEM(<br>
_IO_Initialize_all_drivers,<br>
RTEMS_SYSINIT_DEVICE_DRIVERS,<br>
RTEMS_SYSINIT_ORDER_MIDDLE<br>
);<br>
<br>
<br>
This linker set is parsed during rtems_initialize_executive<br>
(cpukit/sapi/src/exinit.c) which is called from boot_card<br>
(bsps/shared/start/bootcard.c) which is called from the assembler<br>
initialization code. So not the shortest one to find ;-)<br>
<br>
The order is a bit late. Some drivers might try to initialize earlier.<br>
Therefore the initialization might is too late and a printk doesn't work<br>
for another driver. Maybe that's the reason why imx put it in<br>
SYSINIT_BSP_START which is quite a bit before SYSINIT_DEVICE_DRIVERS:<br>
<br>
<a href="https://git.rtems.org/rtems/tree/cpukit/include/rtems/sysinit.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n29" rel="noreferrer" target="_blank">https://git.rtems.org/rtems/tree/cpukit/include/rtems/sysinit.h?id=7c21077db6d42a8176f29e83fbe51afac6f6a717#n29</a><br>
</blockquote></div></div>