[PATCH v2 2/2] drivers/firmware: consolidate EFI framebuffer setup for all arches
Thomas Zimmermann
tzimmermann at suse.de
Wed Jun 23 03:06:41 PDT 2021
Hi
Am 01.06.21 um 17:00 schrieb Javier Martinez Canillas:
> The register_gop_device() function registers an "efi-framebuffer" platform
> device to match against the efifb driver, to have an early framebuffer for
> EFI platforms.
>
> But there is already support to do exactly the same by the Generic System
> Framebuffers (sysfb) driver. This used to be only for X86 but it has been
> moved to drivers/firmware and could be reused by other architectures.
>
> Also, besides supporting registering an "efi-framebuffer", this driver can
> register a "simple-framebuffer" allowing to use the siple{fb,drm} drivers
> on non-X86 EFI platforms. For example, on aarch64 these drivers can only
> be used with DT and doesn't have code to register a "simple-frambuffer"
> platform device when booting with EFI.
>
> For these reasons, let's remove the register_gop_device() duplicated code
> and instead move the platform specific logic that's there to sysfb driver.
>
> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>
> ---
>
> Changes in v2:
> - Use "depends on" for the supported architectures instead of selecting it.
> - Improve commit message to explain the benefits of reusing sysfb for !X86.
>
> arch/arm/include/asm/efi.h | 5 +-
> arch/arm64/include/asm/efi.h | 5 +-
> arch/riscv/include/asm/efi.h | 5 +-
> drivers/firmware/Kconfig | 8 +--
> drivers/firmware/Makefile | 2 +-
> drivers/firmware/efi/efi-init.c | 90 -------------------------------
> drivers/firmware/efi/sysfb_efi.c | 77 +++++++++++++++++++++++++-
> drivers/firmware/sysfb.c | 40 +++++++++-----
> drivers/firmware/sysfb_simplefb.c | 29 ++++++----
> include/linux/sysfb.h | 28 +++++-----
> 10 files changed, 143 insertions(+), 146 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 9de7ab2ce05..a6f3b179e8a 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -17,6 +17,7 @@
>
> #ifdef CONFIG_EFI
> void efi_init(void);
> +extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
>
> int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
> @@ -52,10 +53,6 @@ void efi_virtmap_unload(void);
> struct screen_info *alloc_screen_info(void);
> void free_screen_info(struct screen_info *si);
>
> -static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
> -{
> -}
> -
> /*
> * A reasonable upper bound for the uncompressed kernel size is 32 MBytes,
> * so we will reserve that amount of memory. We have no easy way to tell what
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 1bed37eb013..d3e1825337b 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -14,6 +14,7 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
> #else
> #define efi_init()
> #endif
> @@ -85,10 +86,6 @@ static inline void free_screen_info(struct screen_info *si)
> {
> }
>
> -static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
> -{
> -}
> -
> #define EFI_ALLOC_ALIGN SZ_64K
>
> /*
> diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> index 6d98cd99968..7a8f0d45b13 100644
> --- a/arch/riscv/include/asm/efi.h
> +++ b/arch/riscv/include/asm/efi.h
> @@ -13,6 +13,7 @@
>
> #ifdef CONFIG_EFI
> extern void efi_init(void);
> +extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
> #else
> #define efi_init()
> #endif
> @@ -39,10 +40,6 @@ static inline void free_screen_info(struct screen_info *si)
> {
> }
>
> -static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
> -{
> -}
> -
> void efi_virtmap_load(void);
> void efi_virtmap_unload(void);
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 4392fc57cf3..c704ac441fb 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -254,9 +254,9 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> config SYSFB
> bool
> default y
> - depends on X86 || COMPILE_TEST
> + depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
>
> -config X86_SYSFB
> +config SYSFB_SIMPLEFB
You should also update the help text for simpledrm to reflect this
change. See drivers/gpu/drm/tiny/Kconfig.
> bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> depends on SYSFB
> help
> @@ -264,10 +264,10 @@ config X86_SYSFB
> bootloader or kernel can show basic video-output during boot for
> user-guidance and debugging. Historically, x86 used the VESA BIOS
> Extensions and EFI-framebuffers for this, which are mostly limited
> - to x86.
> + to x86 BIOS or EFI systems.
> This option, if enabled, marks VGA/VBE/EFI framebuffers as generic
> framebuffers so the new generic system-framebuffer drivers can be
> - used on x86. If the framebuffer is not compatible with the generic
> + used instead. If the framebuffer is not compatible with the generic
> modes, it is advertised as fallback platform framebuffer so legacy
> drivers like efifb, vesafb and uvesafb can pick it up.
> If this option is not selected, all system framebuffers are always
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 946dda07443..705fabe8815 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -19,7 +19,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
> obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
> obj-$(CONFIG_QCOM_SCM) += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> obj-$(CONFIG_SYSFB) += sysfb.o
> -obj-$(CONFIG_X86_SYSFB) += sysfb_simplefb.o
> +obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
> obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
> obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
> obj-$(CONFIG_TURRIS_MOX_RWTM) += turris-mox-rwtm.o
> diff --git a/drivers/firmware/efi/efi-init.c b/drivers/firmware/efi/efi-init.c
> index a552a08a174..b19ce1a83f9 100644
> --- a/drivers/firmware/efi/efi-init.c
> +++ b/drivers/firmware/efi/efi-init.c
> @@ -275,93 +275,3 @@ void __init efi_init(void)
> }
> #endif
> }
> -
> -static bool efifb_overlaps_pci_range(const struct of_pci_range *range)
> -{
> - u64 fb_base = screen_info.lfb_base;
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32;
> -
> - return fb_base >= range->cpu_addr &&
> - fb_base < (range->cpu_addr + range->size);
> -}
> -
> -static struct device_node *find_pci_overlap_node(void)
> -{
> - struct device_node *np;
> -
> - for_each_node_by_type(np, "pci") {
> - struct of_pci_range_parser parser;
> - struct of_pci_range range;
> - int err;
> -
> - err = of_pci_range_parser_init(&parser, np);
> - if (err) {
> - pr_warn("of_pci_range_parser_init() failed: %d\n", err);
> - continue;
> - }
> -
> - for_each_of_pci_range(&parser, &range)
> - if (efifb_overlaps_pci_range(&range))
> - return np;
> - }
> - return NULL;
> -}
> -
> -/*
> - * If the efifb framebuffer is backed by a PCI graphics controller, we have
> - * to ensure that this relation is expressed using a device link when
> - * running in DT mode, or the probe order may be reversed, resulting in a
> - * resource reservation conflict on the memory window that the efifb
> - * framebuffer steals from the PCIe host bridge.
> - */
> -static int efifb_add_links(struct fwnode_handle *fwnode)
> -{
> - struct device_node *sup_np;
> -
> - sup_np = find_pci_overlap_node();
> -
> - /*
> - * If there's no PCI graphics controller backing the efifb, we are
> - * done here.
> - */
> - if (!sup_np)
> - return 0;
> -
> - fwnode_link_add(fwnode, of_fwnode_handle(sup_np));
> - of_node_put(sup_np);
> -
> - return 0;
> -}
> -
> -static const struct fwnode_operations efifb_fwnode_ops = {
> - .add_links = efifb_add_links,
> -};
> -
> -static struct fwnode_handle efifb_fwnode;
> -
> -static int __init register_gop_device(void)
> -{
> - struct platform_device *pd;
> - int err;
> -
> - if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
> - return 0;
> -
> - pd = platform_device_alloc("efi-framebuffer", 0);
> - if (!pd)
> - return -ENOMEM;
> -
> - if (IS_ENABLED(CONFIG_PCI)) {
> - fwnode_init(&efifb_fwnode, &efifb_fwnode_ops);
> - pd->dev.fwnode = &efifb_fwnode;
> - }
> -
> - err = platform_device_add_data(pd, &screen_info, sizeof(screen_info));
> - if (err)
> - return err;
> -
> - return platform_device_add(pd);
> -}
> -subsys_initcall(register_gop_device);
> diff --git a/drivers/firmware/efi/sysfb_efi.c b/drivers/firmware/efi/sysfb_efi.c
> index 9f035b15501..2814af6baf1 100644
> --- a/drivers/firmware/efi/sysfb_efi.c
> +++ b/drivers/firmware/efi/sysfb_efi.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
> - * Generic System Framebuffers on x86
> + * Generic System Framebuffers
> * Copyright (c) 2012-2013 David Herrmann <dh.herrmann at gmail.com>
> *
> * EFI Quirks Copyright (c) 2006 Edgar Hucek <gimli at dark-green.com>
> @@ -19,7 +19,9 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> +#include <linux/of_address.h>
> #include <linux/pci.h>
> +#include <linux/platform_device.h>
> #include <linux/screen_info.h>
> #include <linux/sysfb.h>
> #include <video/vga.h>
> @@ -267,7 +269,72 @@ static const struct dmi_system_id efifb_dmi_swap_width_height[] __initconst = {
> {},
> };
>
> -__init void sysfb_apply_efi_quirks(void)
> +static bool efifb_overlaps_pci_range(const struct of_pci_range *range)
> +{
> + u64 fb_base = screen_info.lfb_base;
> +
> + if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> + fb_base |= (u64)(unsigned long)screen_info.ext_lfb_base << 32;
> +
> + return fb_base >= range->cpu_addr &&
> + fb_base < (range->cpu_addr + range->size);
> +}
> +
> +static struct device_node *find_pci_overlap_node(void)
> +{
> + struct device_node *np;
> +
> + for_each_node_by_type(np, "pci") {
> + struct of_pci_range_parser parser;
> + struct of_pci_range range;
> + int err;
> +
> + err = of_pci_range_parser_init(&parser, np);
> + if (err) {
> + pr_warn("of_pci_range_parser_init() failed: %d\n", err);
> + continue;
> + }
> +
> + for_each_of_pci_range(&parser, &range)
> + if (efifb_overlaps_pci_range(&range))
> + return np;
> + }
> + return NULL;
> +}
> +
> +/*
> + * If the efifb framebuffer is backed by a PCI graphics controller, we have
> + * to ensure that this relation is expressed using a device link when
> + * running in DT mode, or the probe order may be reversed, resulting in a
> + * resource reservation conflict on the memory window that the efifb
> + * framebuffer steals from the PCIe host bridge.
> + */
> +static int efifb_add_links(struct fwnode_handle *fwnode)
> +{
> + struct device_node *sup_np;
> +
> + sup_np = find_pci_overlap_node();
> +
> + /*
> + * If there's no PCI graphics controller backing the efifb, we are
> + * done here.
> + */
> + if (!sup_np)
> + return 0;
> +
> + fwnode_link_add(fwnode, of_fwnode_handle(sup_np));
> + of_node_put(sup_np);
> +
> + return 0;
> +}
> +
> +static const struct fwnode_operations efifb_fwnode_ops = {
> + .add_links = efifb_add_links,
> +};
> +
> +static struct fwnode_handle efifb_fwnode;
> +
> +__init void sysfb_apply_efi_quirks(struct platform_device *pd)
> {
> if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI ||
> !(screen_info.capabilities & VIDEO_CAPABILITY_SKIP_QUIRKS))
> @@ -281,4 +348,10 @@ __init void sysfb_apply_efi_quirks(void)
> screen_info.lfb_height = temp;
> screen_info.lfb_linelength = 4 * screen_info.lfb_width;
> }
> +
> + if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI &&
> + IS_ENABLED(CONFIG_PCI)) {
We have a 100-character limit now. This should (?) fit onto a single line.
> + fwnode_init(&efifb_fwnode, &efifb_fwnode_ops);
> + pd->dev.fwnode = &efifb_fwnode;
> + }
> }
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 1337515963d..3ecd60a0215 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -1,11 +1,11 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
> - * Generic System Framebuffers on x86
> + * Generic System Framebuffers
> * Copyright (c) 2012-2013 David Herrmann <dh.herrmann at gmail.com>
> */
>
> /*
> - * Simple-Framebuffer support for x86 systems
> + * Simple-Framebuffer support
> * Create a platform-device for any available boot framebuffer. The
> * simple-framebuffer platform device is already available on DT systems, so
> * this module parses the global "screen_info" object and creates a suitable
> @@ -16,12 +16,12 @@
> * to pick these devices up without messing with simple-framebuffer drivers.
> * The global "screen_info" is still valid at all times.
> *
> - * If CONFIG_X86_SYSFB is not selected, we never register "simple-framebuffer"
> + * If CONFIG_SYSFB_SIMPLEFB is not selected, never register "simple-framebuffer"
> * platform devices, but only use legacy framebuffer devices for
> * backwards compatibility.
> *
> * TODO: We set the dev_id field of all platform-devices to 0. This allows
> - * other x86 OF/DT parsers to create such devices, too. However, they must
> + * other OF/DT parsers to create such devices, too. However, they must
> * start at offset 1 for this to work.
> */
>
> @@ -39,31 +39,43 @@ static __init int sysfb_init(void)
> struct screen_info *si = &screen_info;
> struct simplefb_platform_data mode;
> struct platform_device *pd;
> - const char *name;
> bool compatible;
> int ret;
>
> - sysfb_apply_efi_quirks();
> + pd = platform_device_alloc("", 0);
> + if (!pd)
> + return -ENOMEM;
> +
> + sysfb_apply_efi_quirks(pd);
>
> /* try to create a simple-framebuffer device */
> - compatible = parse_mode(si, &mode);
> + compatible = sysfb_parse_mode(si, &mode);
> if (compatible) {
> - ret = create_simplefb(si, &mode);
> + ret = sysfb_create_simplefb(si, &mode, pd);
> if (!ret)
> return 0;
> }
>
> /* if the FB is incompatible, create a legacy framebuffer device */
> if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
> - name = "efi-framebuffer";
> + pd->name = "efi-framebuffer";
> else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
> - name = "vesa-framebuffer";
> + pd->name = "vesa-framebuffer";
> else
> - name = "platform-framebuffer";
> + pd->name = "platform-framebuffer";
You're allocating the platform device with an empty name string. And
here you're overwriting the pointer. Can you rearrange the code to first
detect a proper name and then allocate the device with that name? It
takes a few bytes more memory, but seems in the spirit of the interface.
Best regards
Thomas
> +
> + ret = platform_device_add_data(pd, si, sizeof(*si));
> + if (ret)
> + goto err;
> +
> + ret = platform_device_add(pd);
> + if (ret)
> + goto err;
>
> - pd = platform_device_register_resndata(NULL, name, 0,
> - NULL, 0, si, sizeof(*si));
> - return PTR_ERR_OR_ZERO(pd);
> + return 0;
> +err:
> + platform_device_put(pd);
> + return ret;
> }
>
> /* must execute after PCI subsystem for EFI quirks */
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index df892444ea1..cffff4283f3 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-or-later
> /*
> - * Generic System Framebuffers on x86
> + * Generic System Framebuffers
> * Copyright (c) 2012-2013 David Herrmann <dh.herrmann at gmail.com>
> */
>
> @@ -23,9 +23,9 @@
> static const char simplefb_resname[] = "BOOTFB";
> static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
>
> -/* try parsing x86 screen_info into a simple-framebuffer mode struct */
> -__init bool parse_mode(const struct screen_info *si,
> - struct simplefb_platform_data *mode)
> +/* try parsing screen_info into a simple-framebuffer mode struct */
> +__init bool sysfb_parse_mode(const struct screen_info *si,
> + struct simplefb_platform_data *mode)
> {
> const struct simplefb_format *f;
> __u8 type;
> @@ -57,13 +57,14 @@ __init bool parse_mode(const struct screen_info *si,
> return false;
> }
>
> -__init int create_simplefb(const struct screen_info *si,
> - const struct simplefb_platform_data *mode)
> +__init int sysfb_create_simplefb(const struct screen_info *si,
> + const struct simplefb_platform_data *mode,
> + struct platform_device *pd)
> {
> - struct platform_device *pd;
> struct resource res;
> u64 base, size;
> u32 length;
> + int ret;
>
> /*
> * If the 64BIT_BASE capability is set, ext_lfb_base will contain the
> @@ -105,7 +106,15 @@ __init int create_simplefb(const struct screen_info *si,
> if (res.end <= res.start)
> return -EINVAL;
>
> - pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
> - &res, 1, mode, sizeof(*mode));
> - return PTR_ERR_OR_ZERO(pd);
> + pd->name = "simple-framebuffer";
> +
> + ret = platform_device_add_resources(pd, &res, 1);
> + if (ret)
> + return ret;
> +
> + ret = platform_device_add_data(pd, mode, sizeof(*mode));
> + if (ret)
> + return ret;
> +
> + return platform_device_add(pd);
> }
> diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h
> index 3e5355769dc..d97162f4b97 100644
> --- a/include/linux/sysfb.h
> +++ b/include/linux/sysfb.h
> @@ -58,37 +58,39 @@ struct efifb_dmi_info {
> #ifdef CONFIG_EFI
>
> extern struct efifb_dmi_info efifb_dmi_list[];
> -void sysfb_apply_efi_quirks(void);
> +void sysfb_apply_efi_quirks(struct platform_device *pd);
>
> #else /* CONFIG_EFI */
>
> -static inline void sysfb_apply_efi_quirks(void)
> +static inline void sysfb_apply_efi_quirks(struct platform_device *pd)
> {
> }
>
> #endif /* CONFIG_EFI */
>
> -#ifdef CONFIG_X86_SYSFB
> +#ifdef CONFIG_SYSFB_SIMPLEFB
>
> -bool parse_mode(const struct screen_info *si,
> - struct simplefb_platform_data *mode);
> -int create_simplefb(const struct screen_info *si,
> - const struct simplefb_platform_data *mode);
> +bool sysfb_parse_mode(const struct screen_info *si,
> + struct simplefb_platform_data *mode);
> +int sysfb_create_simplefb(const struct screen_info *si,
> + const struct simplefb_platform_data *mode,
> + struct platform_device *pd);
>
> -#else /* CONFIG_X86_SYSFB */
> +#else /* CONFIG_SYSFB_SIMPLE */
>
> -static inline bool parse_mode(const struct screen_info *si,
> - struct simplefb_platform_data *mode)
> +static inline bool sysfb_parse_mode(const struct screen_info *si,
> + struct simplefb_platform_data *mode)
> {
> return false;
> }
>
> -static inline int create_simplefb(const struct screen_info *si,
> - const struct simplefb_platform_data *mode)
> +static inline int sysfb_create_simplefb(const struct screen_info *si,
> + const struct simplefb_platform_data *mode,
> + struct platform_device *pd)
> {
> return -EINVAL;
> }
>
> -#endif /* CONFIG_X86_SYSFB */
> +#endif /* CONFIG_SYSFB_SIMPLE */
>
> #endif /* _LINUX_SYSFB_H */
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20210623/b4eda7c7/attachment.sig>
More information about the linux-riscv
mailing list