[PATCH v3] Raspberrypi: updated the console interface.
Christian Mauderer
list at c-mauderer.de
Mon Dec 30 13:26:22 UTC 2019
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.
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?
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
>
> >
> > 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
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