Messages in this thread | | | From | Thomas Zimmermann <> | Subject | Re: [PATCH v2 2/2] drivers/firmware: consolidate EFI framebuffer setup for all arches | Date | Wed, 23 Jun 2021 12:06:41 +0200 |
| |
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@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@gmail.com> > * > * EFI Quirks Copyright (c) 2006 Edgar Hucek <gimli@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@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@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
[unhandled content-type:application/pgp-signature] | |