mbox series

[0/2] Add generic framebuffer support to EFI earlycon driver

Message ID 20220728142824.3836-1-markuss.broks@gmail.com
Headers show
Series Add generic framebuffer support to EFI earlycon driver | expand

Message

Markuss Broks July 28, 2022, 2:28 p.m. UTC
Make the EFI earlycon driver be suitable for any linear framebuffers.
This should be helpful for early porting of boards with no other means of
output, like smartphones/tablets. There seems to be an issue with early_ioremap
function on ARM32, but I am unable to find the exact cause. It appears the mappings
returned by it are somehow incorrect, thus the driver is disabled on ARM. EFI early
console was disabled on IA64 previously, so I kept it in EFI earlycon Kconfig.

This patch also changes behavior on EFI systems, by selecting the mapping type
based on if the framebuffer region intersects with system RAM. If it does, it's
common sense that it should be in RAM as a whole, and so the system RAM mapping is
used. It was tested to be working on my PC (Intel Z490 platform).

Markuss Broks (2):
  drivers: serial: earlycon: Pass device-tree node
  efi: earlycon: Add support for generic framebuffers and move to fbdev
    subsystem

 .../admin-guide/kernel-parameters.txt         |  12 +-
 MAINTAINERS                                   |   5 +
 drivers/firmware/efi/Kconfig                  |   6 +-
 drivers/firmware/efi/Makefile                 |   1 -
 drivers/firmware/efi/earlycon.c               | 246 --------------
 drivers/tty/serial/earlycon.c                 |   3 +
 drivers/video/fbdev/Kconfig                   |  11 +
 drivers/video/fbdev/Makefile                  |   1 +
 drivers/video/fbdev/earlycon.c                | 301 ++++++++++++++++++
 include/linux/serial_core.h                   |   1 +
 10 files changed, 331 insertions(+), 256 deletions(-)
 delete mode 100644 drivers/firmware/efi/earlycon.c
 create mode 100644 drivers/video/fbdev/earlycon.c

Comments

Greg Kroah-Hartman July 28, 2022, 2:38 p.m. UTC | #1
On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote:
> Pass a pointer to device-tree node in case the driver probed from
> OF. This makes early console drivers able to fetch options from
> device-tree node properties.
> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> ---
>  drivers/tty/serial/earlycon.c | 3 +++
>  include/linux/serial_core.h   | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 57c70851f22a0e78805f34d1a7700708104b6f6a..14e8a7fe54486a1c377a6659c37a73858de5bf0b 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -304,6 +304,9 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
>  		strlcpy(early_console_dev.options, options,
>  			sizeof(early_console_dev.options));
>  	}
> +
> +	early_console_dev.node = node;
> +
>  	earlycon_init(&early_console_dev, match->name);
>  	err = match->setup(&early_console_dev, options);
>  	earlycon_print_info(&early_console_dev);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index cbd5070bc87f42aa450c4ca7af8a9b59fbe88574..3295721f33e482124fae8370b5889d5d6c012303 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -349,6 +349,7 @@ struct earlycon_device {
>  	struct uart_port port;
>  	char options[16];		/* e.g., 115200n8 */
>  	unsigned int baud;
> +	unsigned long node;

That should not be an unsigned long, but rather an 'int'.  Something got
messed up, of_setup_earlycon() should be changed to reflect this before
propagating the error to other places in the kernel.

And it's not really a "node" but an "offset", right?

thanks,

greg k-h
Andy Shevchenko July 28, 2022, 9:04 p.m. UTC | #2
On Thu, Jul 28, 2022 at 4:41 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote:
> > Pass a pointer to device-tree node in case the driver probed from
> > OF. This makes early console drivers able to fetch options from
> > device-tree node properties.

...

> > +     unsigned long node;
>
> That should not be an unsigned long, but rather an 'int'.  Something got
> messed up, of_setup_earlycon() should be changed to reflect this before
> propagating the error to other places in the kernel.

It's a pointer, but what puzzles me, why it can't be declared as a such:

 struct device_node *node;

?

> And it's not really a "node" but an "offset", right?

Seems no.
Greg Kroah-Hartman July 29, 2022, 7:57 a.m. UTC | #3
On Thu, Jul 28, 2022 at 11:04:24PM +0200, Andy Shevchenko wrote:
> On Thu, Jul 28, 2022 at 4:41 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote:
> > > Pass a pointer to device-tree node in case the driver probed from
> > > OF. This makes early console drivers able to fetch options from
> > > device-tree node properties.
> 
> ...
> 
> > > +     unsigned long node;
> >
> > That should not be an unsigned long, but rather an 'int'.  Something got
> > messed up, of_setup_earlycon() should be changed to reflect this before
> > propagating the error to other places in the kernel.
> 
> It's a pointer, but what puzzles me, why it can't be declared as a such:
> 
>  struct device_node *node;
> 
> ?

It should not be a pointer, trace things backwards, it comes from a call
to of_setup_earlycon() from early_init_dt_scan_chosen_stdout() which has
offset declared as an int, and then does:
	if (of_setup_earlycon(match, offset, options) == 0)

So why would it be a node?

> > And it's not really a "node" but an "offset", right?
> 
> Seems no.

Really?  What am I missing here?

confused,

greg k-h
Andy Shevchenko July 29, 2022, 10:47 a.m. UTC | #4
On Fri, Jul 29, 2022 at 9:57 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jul 28, 2022 at 11:04:24PM +0200, Andy Shevchenko wrote:
> > On Thu, Jul 28, 2022 at 4:41 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote:

...

> > > > +     unsigned long node;
> > >
> > > That should not be an unsigned long, but rather an 'int'.  Something got
> > > messed up, of_setup_earlycon() should be changed to reflect this before
> > > propagating the error to other places in the kernel.
> >
> > It's a pointer, but what puzzles me, why it can't be declared as a such:
> >
> >  struct device_node *node;
> >
> > ?
>
> It should not be a pointer, trace things backwards, it comes from a call
> to of_setup_earlycon() from early_init_dt_scan_chosen_stdout() which has
> offset declared as an int, and then does:
>         if (of_setup_earlycon(match, offset, options) == 0)
>
> So why would it be a node?

This is a very good question.

> > > And it's not really a "node" but an "offset", right?
> >
> > Seems no.
>
> Really?  What am I missing here?

It's me who is missing something here, thanks for your elaboration!
After it it becomes clear that your first question should be
addressed.