[PATCH v3] Raspberrypi: updated the console interface.
Christian Mauderer
list at c-mauderer.de
Mon Dec 30 15:50:16 UTC 2019
On 30/12/2019 16:02, Niteesh wrote:
> On Mon, Dec 30, 2019 at 6:56 PM Christian Mauderer <list at c-mauderer.de
> <mailto:list at 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.gn at gmail.com
> <mailto:gsnb.gn at gmail.com>
> > <mailto:gsnb.gn at gmail.com <mailto:gsnb.gn at gmail.com>>> wrote:
> >
> > On Mon, 30 Dec, 2019, 2:07 AM Christian Mauderer,
> > <list at c-mauderer.de <mailto:list at c-mauderer.de>
> <mailto:list at c-mauderer.de <mailto: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>
> > <mailto:gsnb.gn at gmail.com <mailto:gsnb.gn at gmail.com>>
> > > <mailto:gsnb.gn at gmail.com <mailto:gsnb.gn at gmail.com>
> <mailto: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?
>
> 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.
It's a possibility but I don't think that there is a rule for it. Most
likely that naming is found because it's the most common one in Linux:
Name classic / integrated serial ports ttySx. For USB use ttyUSBx. But
it seems that for example for the PL011 based UARTS Linux uses ttyAMA0.
For the mini UARTs it uses ttyS0. So the only real convention is to name
them "tty...".
On the other hand backward compatibility would be a more important
point. So maybe try to keep the naming that was used before your patch.
>
> 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.
OK.
>
> 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.
For the frame buffer console you have something like
console=fbcons
For serial port it seems to be one of the following
console=ttyAMA0,115200
or
console=serial0,115200
A optimal solution would have been if the name just could have been used
directly so that you can create a link to /dev/<nameFromConsole> for the
/dev/Console. But it seems that the name changed during the different
Raspberry revisions. So most likely that won't work anyway.
>
>
>
> >
> > >
> > > 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
>
More information about the devel
mailing list