[PATCH v3] Raspberrypi: updated the console interface.
Niteesh
gsnb.gn at gmail.com
Mon Dec 30 15:02:28 UTC 2019
On Mon, Dec 30, 2019 at 6:56 PM Christian Mauderer <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>> wrote:
> >
> > On Mon, 30 Dec, 2019, 2:07 AM Christian Mauderer,
> > <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>>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20191230/3c8d534c/attachment-0001.html>
More information about the devel
mailing list