[PATCH v3 00/12] drm, coreboot: Add DRM coreboot driver

[PATCH v3 00/12] drm, coreboot: Add DRM coreboot driver

[Thomas Zimmermann]

Coreboot implements framebuffer support via simple-framebuffer. Provide a
dedicated DRM driver. Keep the simple-framebuffer code for now.

For each firmware's provided framebuffer, we prefer a dedicated DRM driver
tailored towards the platform's feature set. The coreboot framebuffer
device currently creates a simple-framebuffer device for the provided
framebuffer aperture. But simple-framebuffer is for DeviceTree nodes; not
for coreboot. The simple-framebuffer infrastructure should be phased out
for non-DT use cases. Coreboot is one of the final users of the code
(besides n64).

Patches 1 to 5 start by fixing problems in the coreboot framebuffer
implementation. There is a possible dangling pointer, the memory is
marked as busy, the device hierarchy is incorrect, and a few minor things.

Patches 6 to 9 prepare the coreboot support for use by external drivers.
Specifically, structures for the entries os the coreboot payload table
have to be exported.

Patches 10 to 12 add corebootdrm, a DRM driver for the new
coreboot-framebuffer platform device. Corebootdrm follows the pattern
established by similar drivers. It also uses the same sysfb helpers. It
is therefore fairly small. With patch 11, it has feature parity with
simpledrm on the old simple-framebuffer. Patch 12 adds support for panel-
orientation flags that coreboot makes available.

Tested on an HP Chromebook with MrChromebox 4.16. Runs with Weston and
fbcon. Xorg requires an additional patch available at [1].

v3:
- avoid packed types for coreboot headers
- comment in various places
- fix typos
v2:
- keep design of coreboot framebuffer code intact (Julius)
- fix bugs in the coreboot framebuffer code
- rewrite corebootdrm as platform device
- support panel orientation

[1] https://gitlab.freedesktop.org/tzimmermann/xserver/-/commit/0b326aad28549762ed2b0e2bedf8f8a42f1f6b3b

Thomas Zimmermann (12):
  firmware: google: framebuffer: Do not unregister platform device
  firmware: google: framebuffer: Do not mark framebuffer as busy
  firmware: google: framebuffer: Init memory resource with helper macro
  firmware: google: framebuffer: Tie platform device to PCI hardware
  firmware: google: framebuffer: Fix dependencies
  firmware: google: Init coreboot bus with subsys_initcall()
  firmware: google: Clean up include statements in coreboot_table.h
  firmware: google: Export coreboot table entries
  firmware: google: Pack structures for coreboot table entries
  drm/sysfb: Generalize pixel-format matching
  drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers
  drm/sysfb: corebootdrm: Support panel orientation

 MAINTAINERS                                   |   1 +
 drivers/firmware/google/Kconfig               |   5 +-
 drivers/firmware/google/cbmem.c               |   1 +
 drivers/firmware/google/coreboot_table.c      |  13 +-
 drivers/firmware/google/coreboot_table.h      |  59 +--
 .../firmware/google/framebuffer-coreboot.c    | 126 +++--
 drivers/firmware/google/memconsole-coreboot.c |   1 +
 drivers/firmware/google/vpd.c                 |   1 +
 drivers/gpu/drm/sysfb/Kconfig                 |  16 +
 drivers/gpu/drm/sysfb/Makefile                |   1 +
 drivers/gpu/drm/sysfb/corebootdrm.c           | 434 ++++++++++++++++++
 drivers/gpu/drm/sysfb/drm_sysfb.c             |  24 +
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      |   8 +-
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c |  30 --
 drivers/gpu/drm/sysfb/efidrm.c                |   8 +-
 drivers/gpu/drm/sysfb/vesadrm.c               |   8 +-
 include/linux/coreboot.h                      |  90 ++++
 17 files changed, 702 insertions(+), 124 deletions(-)
 create mode 100644 drivers/gpu/drm/sysfb/corebootdrm.c
 create mode 100644 include/linux/coreboot.h


base-commit: 6e53f6296065672f8a0c7f98b4b6c409dac382b4
-- 
2.52.0

[PATCH v3 01/12] firmware: google: framebuffer: Do not unregister platform device

[Thomas Zimmermann]

The native driver takes over the framebuffer aperture by removing the
system- framebuffer platform device. Afterwards the pointer in drvdata
is dangling. Remove the entire logic around drvdata and let the kernel's
aperture helpers handle this. The platform device depends on the native
hardware device instead of the coreboot device anyway.

When commit 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer
driver") added the coreboot framebuffer code, the kernel did not support
device-based aperture management. Instead native driviers only removed
the conflicting fbdev device. At that point, unregistering the framebuffer
device most likely worked correctly. It was definitely broken after
commit d9702b2a2171 ("fbdev/simplefb: Do not use struct
fb_info.apertures"). So take this commit for the Fixes tag. Earlier
releases might work depending on the native hardware driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d9702b2a2171 ("fbdev/simplefb: Do not use struct fb_info.apertures")
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Hans de Goede <hansg@kernel.org>
Cc: linux-fbdev@vger.kernel.org
Cc: <stable@vger.kernel.org> # v6.3+
---
 drivers/firmware/google/framebuffer-coreboot.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index c68c9f56370f..4e9177105992 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -81,19 +81,10 @@ static int framebuffer_probe(struct coreboot_device *dev)
 						 sizeof(pdata));
 	if (IS_ERR(pdev))
 		pr_warn("coreboot: could not register framebuffer\n");
-	else
-		dev_set_drvdata(&dev->dev, pdev);
 
 	return PTR_ERR_OR_ZERO(pdev);
 }
 
-static void framebuffer_remove(struct coreboot_device *dev)
-{
-	struct platform_device *pdev = dev_get_drvdata(&dev->dev);
-
-	platform_device_unregister(pdev);
-}
-
 static const struct coreboot_device_id framebuffer_ids[] = {
 	{ .tag = CB_TAG_FRAMEBUFFER },
 	{ /* sentinel */ }
@@ -102,7 +93,6 @@ MODULE_DEVICE_TABLE(coreboot, framebuffer_ids);
 
 static struct coreboot_driver framebuffer_driver = {
 	.probe = framebuffer_probe,
-	.remove = framebuffer_remove,
 	.drv = {
 		.name = "framebuffer",
 	},
-- 
2.52.0

[PATCH v3 02/12] firmware: google: framebuffer: Do not mark framebuffer as busy

[Thomas Zimmermann]

Remove the flag IORESOURCE_BUSY flag from coreboot's framebuffer
resource. It prevents simpledrm from successfully requesting the
range for its own use; resulting in errors such as

[    2.775430] simple-framebuffer simple-framebuffer.0: [drm] could not acquire memory region [mem 0x80000000-0x80407fff flags 0x80000200]

As with other uses of simple-framebuffer, the simple-framebuffer
device should only declare it's I/O resources, but not actively use
them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 851b4c14532d ("firmware: coreboot: Add coreboot framebuffer driver")
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: chrome-platform@lists.linux.dev
Cc: <stable@vger.kernel.org> # v4.18+
---
 drivers/firmware/google/framebuffer-coreboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 4e9177105992..f44183476ed7 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -67,7 +67,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		return -ENODEV;
 
 	memset(&res, 0, sizeof(res));
-	res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+	res.flags = IORESOURCE_MEM;
 	res.name = "Coreboot Framebuffer";
 	res.start = fb->physical_address;
 	length = PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line);
-- 
2.52.0

[PATCH v3 03/12] firmware: google: framebuffer: Init memory resource with helper macro

[Thomas Zimmermann]

Initialize framebuffer memory resource with DEFINE_RES_MEM() instead
of open-coding the setup.

While at it, drop the resource name to make the kernel use the device
name of the simple-framebuffer device for the resource. The latter
includes a device number. While the meaning of the resource name is
somewhat fuzzy and varies across entries in /proc/iomem, showing the
device name seems more helpful than showing a fixed name.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/framebuffer-coreboot.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index f44183476ed7..767515a30a52 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -26,7 +26,6 @@ static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
 static int framebuffer_probe(struct coreboot_device *dev)
 {
 	int i;
-	u32 length;
 	struct lb_framebuffer *fb = &dev->framebuffer;
 	struct platform_device *pdev;
 	struct resource res;
@@ -53,6 +52,11 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (!fb->physical_address)
 		return -ENODEV;
 
+	res = DEFINE_RES_MEM(fb->physical_address,
+			     PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line));
+	if (res.end <= res.start)
+		return -EINVAL;
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -66,15 +70,6 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (!pdata.format)
 		return -ENODEV;
 
-	memset(&res, 0, sizeof(res));
-	res.flags = IORESOURCE_MEM;
-	res.name = "Coreboot Framebuffer";
-	res.start = fb->physical_address;
-	length = PAGE_ALIGN(fb->y_resolution * fb->bytes_per_line);
-	res.end = res.start + length - 1;
-	if (res.end <= res.start)
-		return -EINVAL;
-
 	pdev = platform_device_register_resndata(&dev->dev,
 						 "simple-framebuffer", 0,
 						 &res, 1, &pdata,
-- 
2.52.0

[PATCH v3 04/12] firmware: google: framebuffer: Tie platform device to PCI hardware

[Thomas Zimmermann]

Use the PCI device as parent of the system-framebuffer device instead
of the coreboot device. Prevents SIGBUS or SIGSEG after hot-unplug of
the PCI device while the framebuffer is active.

The simple-framebuffer device depends on the PCI hardware, so this
device needs to be its parent. The current coreboot parent is no
longer needed after the system-framebuffer device has been created.

On systems without PCI or if no PCI parent device could be found,
the platform device hangs on the platform bus directly.

The fix here is similar to code in sysfb, which contained that same
bug.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 .../firmware/google/framebuffer-coreboot.c    | 82 +++++++++++++++++--
 1 file changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 767515a30a52..07e9c7be94fa 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/sysfb.h>
@@ -21,14 +22,71 @@
 
 #define CB_TAG_FRAMEBUFFER 0x12
 
+#if defined(CONFIG_PCI)
+static bool framebuffer_pci_dev_is_enabled(struct pci_dev *pdev)
+{
+	/*
+	 * TODO: Try to integrate this code into the PCI subsystem
+	 */
+	int ret;
+	u16 command;
+
+	ret = pci_read_config_word(pdev, PCI_COMMAND, &command);
+	if (ret != PCIBIOS_SUCCESSFUL)
+		return false;
+	if (!(command & PCI_COMMAND_MEMORY))
+		return false;
+	return true;
+}
+
+static struct pci_dev *framebuffer_parent_pci_dev(struct resource *res)
+{
+	struct pci_dev *pdev = NULL;
+	const struct resource *r = NULL;
+
+	while (!r && (pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev)))
+		r = pci_find_resource(pdev, res);
+
+	if (!r || !pdev)
+		return NULL; /* not found; not an error */
+
+	if (!framebuffer_pci_dev_is_enabled(pdev)) {
+		pci_dev_put(pdev);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return pdev;
+}
+#else
+static struct pci_dev *framebuffer_parent_pci_dev(struct resource *res)
+{
+	return NULL;
+}
+#endif
+
+static struct device *framebuffer_parent_dev(struct resource *res)
+{
+	struct pci_dev *pdev;
+
+	pdev = framebuffer_parent_pci_dev(res);
+	if (IS_ERR(pdev))
+		return ERR_CAST(pdev);
+	else if (pdev)
+		return &pdev->dev;
+
+	return NULL;
+}
+
 static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
 
 static int framebuffer_probe(struct coreboot_device *dev)
 {
 	int i;
 	struct lb_framebuffer *fb = &dev->framebuffer;
+	struct device *parent;
 	struct platform_device *pdev;
 	struct resource res;
+	int ret;
 	struct simplefb_platform_data pdata = {
 		.width = fb->x_resolution,
 		.height = fb->y_resolution,
@@ -57,6 +115,10 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (res.end <= res.start)
 		return -EINVAL;
 
+	parent = framebuffer_parent_dev(&res);
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -67,17 +129,27 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		    fb->blue_mask_size     == formats[i].blue.length)
 			pdata.format = formats[i].name;
 	}
-	if (!pdata.format)
-		return -ENODEV;
+	if (!pdata.format) {
+		ret = -ENODEV;
+		goto out_put_device_parent;
+	}
 
-	pdev = platform_device_register_resndata(&dev->dev,
+	pdev = platform_device_register_resndata(parent,
 						 "simple-framebuffer", 0,
 						 &res, 1, &pdata,
 						 sizeof(pdata));
-	if (IS_ERR(pdev))
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
 		pr_warn("coreboot: could not register framebuffer\n");
+		goto out_put_device_parent;
+	}
+
+	ret = 0;
 
-	return PTR_ERR_OR_ZERO(pdev);
+out_put_device_parent:
+	if (parent)
+		put_device(parent);
+	return ret;
 }
 
 static const struct coreboot_device_id framebuffer_ids[] = {
-- 
2.52.0

[PATCH v3 05/12] firmware: google: framebuffer: Fix dependencies

[Thomas Zimmermann]

The framebuffer on the coreboot bus represents an entry in the
coreboot payload table; not the actual device. [1] Hence it must
not depend on any other driver setting.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://lore.kernel.org/dri-devel/CAODwPW9_ym3E4za3yoUAs0+1sQfaKTDOau4Oh9Zm8+2uvYVgFQ@mail.gmail.com/ # [1]
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 41b78f5cb735..3ab3e089328b 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -59,11 +59,11 @@ config GOOGLE_MEMCONSOLE_X86_LEGACY
 
 config GOOGLE_FRAMEBUFFER_COREBOOT
 	tristate "Coreboot Framebuffer"
-	depends on FB_SIMPLE || DRM_SIMPLEDRM
 	depends on GOOGLE_COREBOOT_TABLE
 	help
 	  This option enables the kernel to search for a framebuffer in
-	  the coreboot table.  If found, it is registered with simplefb.
+	  the coreboot table.  If found, it is registered with a platform
+	  device of type simple-framebuffer.
 
 config GOOGLE_MEMCONSOLE_COREBOOT
 	tristate "Firmware Memory Console"
-- 
2.52.0

[PATCH v3 06/12] firmware: google: Init coreboot bus with subsys_initcall()

[Thomas Zimmermann]

Using module_init()/device_initcall() is too late to initialize
the coreboot bus, as there might already be drivers that depend
on it.

So far this hasn't been a problem, as coreboot controls all device
creation. Initializing the coreboot bus earlier in subsys_initcall()
will allow for external coreboot drivers to register themselves
with device_initcall(). Prepares coreboot to support additional
coreboot drivers from other subsystems.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 882db32e51be..26d93781e64a 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -251,7 +251,7 @@ static void __exit coreboot_table_driver_exit(void)
 	bus_unregister(&coreboot_bus_type);
 }
 
-module_init(coreboot_table_driver_init);
+subsys_initcall(coreboot_table_driver_init);
 module_exit(coreboot_table_driver_exit);
 
 MODULE_AUTHOR("Google, Inc.");
-- 
2.52.0

[PATCH v3 07/12] firmware: google: Clean up include statements in coreboot_table.h

[Thomas Zimmermann]

Include <linux/mod_devicetable.h> from source files and only forward-
declare struct coreboot_device_id in coreboot_table.h.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/cbmem.c                | 1 +
 drivers/firmware/google/coreboot_table.c       | 1 +
 drivers/firmware/google/coreboot_table.h       | 3 ++-
 drivers/firmware/google/framebuffer-coreboot.c | 1 +
 drivers/firmware/google/memconsole-coreboot.c  | 1 +
 drivers/firmware/google/vpd.c                  | 1 +
 6 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
index 54c3b8b05e5d..3397bacdfdbe 100644
--- a/drivers/firmware/google/cbmem.c
+++ b/drivers/firmware/google/cbmem.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/kobject.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 26d93781e64a..a031d6fe6bc5 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index bb6f0f7299b4..17e9e5c3f6e1 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -13,7 +13,8 @@
 #define __COREBOOT_TABLE_H
 
 #include <linux/device.h>
-#include <linux/mod_devicetable.h>
+
+struct coreboot_device_id;
 
 /* Coreboot table header structure */
 struct coreboot_table_header {
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 07e9c7be94fa..81aa522edb1e 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_data/simplefb.h>
diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index c5f08617aa8d..4aa9b1cad3c3 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 
 #include "memconsole.h"
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 339a3f74b247..8d7f123f96f4 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/kobject.h>
 #include <linux/list.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
-- 
2.52.0

[PATCH v3 08/12] firmware: google: Export coreboot table entries

[Thomas Zimmermann]

Move types for coreboot table entries to <linux/coreboot.h>. Allows
drivers in other subsystems to use these structures.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 MAINTAINERS                                   |  1 +
 drivers/firmware/google/coreboot_table.c      | 10 +++
 drivers/firmware/google/coreboot_table.h      | 60 +----------------
 .../firmware/google/framebuffer-coreboot.c    |  2 -
 include/linux/coreboot.h                      | 66 +++++++++++++++++++
 5 files changed, 78 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/coreboot.h

diff --git a/MAINTAINERS b/MAINTAINERS
index cd371029629e..c0a8de9fb58d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10742,6 +10742,7 @@ L:	chrome-platform@lists.linux.dev
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git
 F:	drivers/firmware/google/
+F:	include/linux/coreboot.h
 
 GOOGLE TENSOR SoC SUPPORT
 M:	Peter Griffin <peter.griffin@linaro.org>
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index a031d6fe6bc5..c769631ea15d 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -22,6 +22,16 @@
 
 #include "coreboot_table.h"
 
+/* Coreboot table header structure */
+struct coreboot_table_header {
+	char signature[4];
+	u32 header_bytes;
+	u32 header_checksum;
+	u32 table_bytes;
+	u32 table_checksum;
+	u32 table_entries;
+};
+
 #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
 #define CB_DRV(d) container_of_const(d, struct coreboot_driver, drv)
 
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 17e9e5c3f6e1..616ca3903e5c 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -12,67 +12,9 @@
 #ifndef __COREBOOT_TABLE_H
 #define __COREBOOT_TABLE_H
 
+#include <linux/coreboot.h>
 #include <linux/device.h>
 
-struct coreboot_device_id;
-
-/* Coreboot table header structure */
-struct coreboot_table_header {
-	char signature[4];
-	u32 header_bytes;
-	u32 header_checksum;
-	u32 table_bytes;
-	u32 table_checksum;
-	u32 table_entries;
-};
-
-/* List of coreboot entry structures that is used */
-/* Generic */
-struct coreboot_table_entry {
-	u32 tag;
-	u32 size;
-};
-
-/* Points to a CBMEM entry */
-struct lb_cbmem_ref {
-	u32 tag;
-	u32 size;
-
-	u64 cbmem_addr;
-};
-
-#define LB_TAG_CBMEM_ENTRY 0x31
-
-/* Corresponds to LB_TAG_CBMEM_ENTRY */
-struct lb_cbmem_entry {
-	u32 tag;
-	u32 size;
-
-	u64 address;
-	u32 entry_size;
-	u32 id;
-};
-
-/* Describes framebuffer setup by coreboot */
-struct lb_framebuffer {
-	u32 tag;
-	u32 size;
-
-	u64 physical_address;
-	u32 x_resolution;
-	u32 y_resolution;
-	u32 bytes_per_line;
-	u8  bits_per_pixel;
-	u8  red_mask_pos;
-	u8  red_mask_size;
-	u8  green_mask_pos;
-	u8  green_mask_size;
-	u8  blue_mask_pos;
-	u8  blue_mask_size;
-	u8  reserved_mask_pos;
-	u8  reserved_mask_size;
-};
-
 /* A device, additionally with information from coreboot. */
 struct coreboot_device {
 	struct device dev;
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index 81aa522edb1e..fab3f28655d3 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -21,8 +21,6 @@
 
 #include "coreboot_table.h"
 
-#define CB_TAG_FRAMEBUFFER 0x12
-
 #if defined(CONFIG_PCI)
 static bool framebuffer_pci_dev_is_enabled(struct pci_dev *pdev)
 {
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
new file mode 100644
index 000000000000..48705b439c6e
--- /dev/null
+++ b/include/linux/coreboot.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * coreboot.h
+ *
+ * Coreboot device and driver interfaces.
+ *
+ * Copyright 2014 Gerd Hoffmann <kraxel@redhat.com>
+ * Copyright 2017 Google Inc.
+ * Copyright 2017 Samuel Holland <samuel@sholland.org>
+ */
+
+#ifndef _LINUX_COREBOOT_H
+#define _LINUX_COREBOOT_H
+
+#include <linux/types.h>
+
+/* List of coreboot entry structures that is used */
+
+#define CB_TAG_FRAMEBUFFER 0x12
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+/* Generic */
+struct coreboot_table_entry {
+	u32 tag;
+	u32 size;
+};
+
+/* Points to a CBMEM entry */
+struct lb_cbmem_ref {
+	u32 tag;
+	u32 size;
+
+	u64 cbmem_addr;
+};
+
+/* Corresponds to LB_TAG_CBMEM_ENTRY */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
+/* Describes framebuffer setup by coreboot */
+struct lb_framebuffer {
+	u32 tag;
+	u32 size;
+
+	u64 physical_address;
+	u32 x_resolution;
+	u32 y_resolution;
+	u32 bytes_per_line;
+	u8  bits_per_pixel;
+	u8  red_mask_pos;
+	u8  red_mask_size;
+	u8  green_mask_pos;
+	u8  green_mask_size;
+	u8  blue_mask_pos;
+	u8  blue_mask_size;
+	u8  reserved_mask_pos;
+	u8  reserved_mask_size;
+};
+
+#endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

[PATCH v3 09/12] firmware: google: Pack structures for coreboot table entries

[Thomas Zimmermann]

Pack the fields in the coreboot table entries. These entries are part of
the coreboot ABI, so they don't follow regular calling conventions. Fields
of type u64 are aligned to boundaries of 4 bytes instead of 8. [1]

So far this has not been a problem. In the future, padding bytes should
be added where explicit alignment is required.

v2:
- use aligned 64-bit type instead of __packed__ attribute (Julius)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://github.com/coreboot/coreboot/blob/main/payloads/libpayload/include/coreboot_tables.h#L96 # [1]
Suggested-by: Julius Werner <jwerner@chromium.org>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 include/linux/coreboot.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index 48705b439c6e..5746b99a070d 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -12,8 +12,11 @@
 #ifndef _LINUX_COREBOOT_H
 #define _LINUX_COREBOOT_H
 
+#include <linux/compiler_attributes.h>
 #include <linux/types.h>
 
+typedef __aligned(4) u64 cb_u64;
+
 /* List of coreboot entry structures that is used */
 
 #define CB_TAG_FRAMEBUFFER 0x12
@@ -30,7 +33,7 @@ struct lb_cbmem_ref {
 	u32 tag;
 	u32 size;
 
-	u64 cbmem_addr;
+	cb_u64 cbmem_addr;
 };
 
 /* Corresponds to LB_TAG_CBMEM_ENTRY */
@@ -38,7 +41,7 @@ struct lb_cbmem_entry {
 	u32 tag;
 	u32 size;
 
-	u64 address;
+	cb_u64 address;
 	u32 entry_size;
 	u32 id;
 };
@@ -48,7 +51,7 @@ struct lb_framebuffer {
 	u32 tag;
 	u32 size;
 
-	u64 physical_address;
+	cb_u64 physical_address;
 	u32 x_resolution;
 	u32 y_resolution;
 	u32 bytes_per_line;
-- 
2.52.0

[PATCH v3 10/12] drm/sysfb: Generalize pixel-format matching

[Thomas Zimmermann]

Provide drm_sysfb_get_format(), a helper that finds a specific DRM
format from a list of pixel formats. The new function builds upon
drm_sysfb_get_format_si(), which finds the DRM format from a given
instance of struct screen_info. Now get the screen_info's pixel format
in the caller. Allows for matching pixel formats in drivers without
screen_info.

Convert the callers in efidrm and vesadrm to the new interface.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/gpu/drm/sysfb/drm_sysfb.c             | 24 +++++++++++++++
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      |  8 ++---
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 30 -------------------
 drivers/gpu/drm/sysfb/efidrm.c                |  8 ++++-
 drivers/gpu/drm/sysfb/vesadrm.c               |  8 ++++-
 5 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb.c b/drivers/gpu/drm/sysfb/drm_sysfb.c
index 308f82153b15..fbfb37d0fae1 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb.c
@@ -31,5 +31,29 @@ int drm_sysfb_get_validated_int0(struct drm_device *dev, const char *name,
 }
 EXPORT_SYMBOL(drm_sysfb_get_validated_int0);
 
+const struct drm_format_info *drm_sysfb_get_format(struct drm_device *dev,
+						   const struct drm_sysfb_format *formats,
+						   size_t nformats,
+						   const struct pixel_format *pixel)
+{
+	const struct drm_format_info *format = NULL;
+	size_t i;
+
+	for (i = 0; i < nformats; ++i) {
+		const struct drm_sysfb_format *f = &formats[i];
+
+		if (pixel_format_equal(pixel, &f->pixel)) {
+			format = drm_format_info(f->fourcc);
+			break;
+		}
+	}
+
+	if (!format)
+		drm_warn(dev, "No compatible color format found\n");
+
+	return format;
+}
+EXPORT_SYMBOL(drm_sysfb_get_format);
+
 MODULE_DESCRIPTION("Helpers for DRM sysfb drivers");
 MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
index de96bfe7562c..b14df5b54bc9 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
@@ -36,6 +36,10 @@ int drm_sysfb_get_validated_int(struct drm_device *dev, const char *name,
 				u64 value, u32 max);
 int drm_sysfb_get_validated_int0(struct drm_device *dev, const char *name,
 				 u64 value, u32 max);
+const struct drm_format_info *drm_sysfb_get_format(struct drm_device *dev,
+						   const struct drm_sysfb_format *formats,
+						   size_t nformats,
+						   const struct pixel_format *pixel);
 
 #if defined(CONFIG_SCREEN_INFO)
 int drm_sysfb_get_width_si(struct drm_device *dev, const struct screen_info *si);
@@ -48,10 +52,6 @@ int drm_sysfb_get_stride_si(struct drm_device *dev, const struct screen_info *si
 			    unsigned int width, unsigned int height, u64 size);
 u64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_info *si,
 				  unsigned int height, unsigned int stride, u64 size);
-const struct drm_format_info *drm_sysfb_get_format_si(struct drm_device *dev,
-						      const struct drm_sysfb_format *formats,
-						      size_t nformats,
-						      const struct screen_info *si);
 #endif
 
 /*
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
index 885864168c54..749290196c6a 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
@@ -72,33 +72,3 @@ u64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_in
 	return drm_sysfb_get_validated_size0(dev, "visible size", vsize, size);
 }
 EXPORT_SYMBOL(drm_sysfb_get_visible_size_si);
-
-const struct drm_format_info *drm_sysfb_get_format_si(struct drm_device *dev,
-						      const struct drm_sysfb_format *formats,
-						      size_t nformats,
-						      const struct screen_info *si)
-{
-	const struct drm_format_info *format = NULL;
-	struct pixel_format pixel;
-	size_t i;
-	int ret;
-
-	ret = screen_info_pixel_format(si, &pixel);
-	if (ret)
-		return NULL;
-
-	for (i = 0; i < nformats; ++i) {
-		const struct drm_sysfb_format *f = &formats[i];
-
-		if (pixel_format_equal(&pixel, &f->pixel)) {
-			format = drm_format_info(f->fourcc);
-			break;
-		}
-	}
-
-	if (!format)
-		drm_warn(dev, "No compatible color format found\n");
-
-	return format;
-}
-EXPORT_SYMBOL(drm_sysfb_get_format_si);
diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 1b683d55d6ea..ee525f330441 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
@@ -45,8 +45,14 @@ static const struct drm_format_info *efidrm_get_format_si(struct drm_device *dev
 		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
 		{ PIXEL_FORMAT_XRGB2101010, DRM_FORMAT_XRGB2101010, },
 	};
+	struct pixel_format pixel;
+	int ret;
+
+	ret = screen_info_pixel_format(si, &pixel);
+	if (ret)
+		return NULL;
 
-	return drm_sysfb_get_format_si(dev, formats, ARRAY_SIZE(formats), si);
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
 }
 
 static u64 efidrm_get_mem_flags(struct drm_device *dev, resource_size_t start,
diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
index 7b7b5ba26317..b8c16ae3faad 100644
--- a/drivers/gpu/drm/sysfb/vesadrm.c
+++ b/drivers/gpu/drm/sysfb/vesadrm.c
@@ -49,8 +49,14 @@ static const struct drm_format_info *vesadrm_get_format_si(struct drm_device *de
 		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
 		{ PIXEL_FORMAT_C8, DRM_FORMAT_C8, },
 	};
+	struct pixel_format pixel;
+	int ret;
+
+	ret = screen_info_pixel_format(si, &pixel);
+	if (ret)
+		return NULL;
 
-	return drm_sysfb_get_format_si(dev, formats, ARRAY_SIZE(formats), si);
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
 }
 
 /*
-- 
2.52.0

[PATCH v3 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Thomas Zimmermann]

Add corebootdrm, a DRM driver for coreboot framebuffers. The driver
supports a pre-initialized framebuffer with various packed RGB formats.
The driver code is fairly small and uses the same logic as the other
sysfb drivers. Most of the implementation comes from existing sysfb
helpers.

Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
output. Initialize the platform device for corebootdrm in the same place
in framebuffer_probe(). With a later commit, the simple-framebuffer should
be removed.

v3:
- comment on _HAS_LFB semantics (Tzung-Bi)
- fix typo in commit description (Tzung-Bi)
- comment on simple-framebuffer being obsolete for coreboot
v2:
- reimplement as platform driver
- limit resources and mappings to known framebuffer memory; no
  page alignment
- create corebootdrm device from coreboot framebuffer code

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/firmware/google/Kconfig               |   3 +-
 .../firmware/google/framebuffer-coreboot.c    |  22 +-
 drivers/gpu/drm/sysfb/Kconfig                 |  16 +
 drivers/gpu/drm/sysfb/Makefile                |   1 +
 drivers/gpu/drm/sysfb/corebootdrm.c           | 412 ++++++++++++++++++
 include/linux/coreboot.h                      |   8 +
 6 files changed, 458 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/sysfb/corebootdrm.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 3ab3e089328b..b78c644fa253 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -63,7 +63,8 @@ config GOOGLE_FRAMEBUFFER_COREBOOT
 	help
 	  This option enables the kernel to search for a framebuffer in
 	  the coreboot table.  If found, it is registered with a platform
-	  device of type simple-framebuffer.
+	  device of type coreboot-framebuffer. Using the old device of
+	  type simple-framebuffer is deprecated.
 
 config GOOGLE_MEMCONSOLE_COREBOOT
 	tristate "Firmware Memory Console"
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index fab3f28655d3..2c63a9bd0dcb 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -76,22 +76,23 @@ static struct device *framebuffer_parent_dev(struct resource *res)
 	return NULL;
 }
 
-static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
-
 static int framebuffer_probe(struct coreboot_device *dev)
 {
-	int i;
 	struct lb_framebuffer *fb = &dev->framebuffer;
 	struct device *parent;
 	struct platform_device *pdev;
 	struct resource res;
 	int ret;
+#if !IS_ENABLED(CONFIG_DRM_COREBOOTDRM)
 	struct simplefb_platform_data pdata = {
 		.width = fb->x_resolution,
 		.height = fb->y_resolution,
 		.stride = fb->bytes_per_line,
 		.format = NULL,
 	};
+	int i;
+	static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
+#endif
 
 	/*
 	 * On coreboot systems, the advertised LB_TAG_FRAMEBUFFER entry
@@ -118,6 +119,20 @@ static int framebuffer_probe(struct coreboot_device *dev)
 	if (IS_ERR(parent))
 		return PTR_ERR(parent);
 
+#if IS_ENABLED(CONFIG_DRM_COREBOOTDRM)
+	pdev = platform_device_register_resndata(parent, "coreboot-framebuffer", 0,
+						 &res, 1, fb, fb->size);
+	if (IS_ERR(pdev)) {
+		pr_warn("coreboot: could not register framebuffer\n");
+		ret = PTR_ERR(pdev);
+		goto out_put_device_parent;
+	}
+#else
+	/*
+	 * FIXME: Coreboot systems should use a driver that binds to
+	 *        coreboot-framebuffer devices. Remove support for
+	 *        simple-framebuffer at some point.
+	 */
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&
@@ -142,6 +157,7 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		pr_warn("coreboot: could not register framebuffer\n");
 		goto out_put_device_parent;
 	}
+#endif
 
 	ret = 0;
 
diff --git a/drivers/gpu/drm/sysfb/Kconfig b/drivers/gpu/drm/sysfb/Kconfig
index 9c9884c7efc6..2559ead6cf1f 100644
--- a/drivers/gpu/drm/sysfb/Kconfig
+++ b/drivers/gpu/drm/sysfb/Kconfig
@@ -7,6 +7,22 @@ config DRM_SYSFB_HELPER
 	tristate
 	depends on DRM
 
+config DRM_COREBOOTDRM
+	tristate "Coreboot framebuffer driver"
+	depends on DRM && MMU
+	depends on GOOGLE_FRAMEBUFFER_COREBOOT
+	select APERTURE_HELPERS
+	select DRM_CLIENT_SELECTION
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	select DRM_SYSFB_HELPER
+	help
+	  DRM driver for coreboot-provided framebuffers.
+
+	  This driver assumes that the display hardware has been initialized
+	  by coreboot firmware before the kernel boots. Scanout buffer, size,
+	  and display format must be provided via coreboot framebuffer device.
+
 config DRM_EFIDRM
 	tristate "EFI framebuffer driver"
 	depends on DRM && MMU && EFI && (!SYSFB_SIMPLEFB || COMPILE_TEST)
diff --git a/drivers/gpu/drm/sysfb/Makefile b/drivers/gpu/drm/sysfb/Makefile
index a156c496413d..85c9087ab03d 100644
--- a/drivers/gpu/drm/sysfb/Makefile
+++ b/drivers/gpu/drm/sysfb/Makefile
@@ -6,6 +6,7 @@ drm_sysfb_helper-y := \
 drm_sysfb_helper-$(CONFIG_SCREEN_INFO) += drm_sysfb_screen_info.o
 obj-$(CONFIG_DRM_SYSFB_HELPER)	+= drm_sysfb_helper.o
 
+obj-$(CONFIG_DRM_COREBOOTDRM)	+= corebootdrm.o
 obj-$(CONFIG_DRM_EFIDRM)	+= efidrm.o
 obj-$(CONFIG_DRM_OFDRM)		+= ofdrm.o
 obj-$(CONFIG_DRM_SIMPLEDRM)	+= simpledrm.o
diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
new file mode 100644
index 000000000000..745318580a5d
--- /dev/null
+++ b/drivers/gpu/drm/sysfb/corebootdrm.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/aperture.h>
+#include <linux/coreboot.h>
+#include <linux/minmax.h>
+#include <linux/platform_device.h>
+
+#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_state_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_device.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_shmem.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+
+#include "drm_sysfb_helper.h"
+
+#define DRIVER_NAME	"corebootdrm"
+#define DRIVER_DESC	"DRM driver for Coreboot framebuffers"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+static const struct drm_format_info *
+corebootdrm_get_format_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	static const struct drm_sysfb_format formats[] = {
+		{ PIXEL_FORMAT_XRGB1555, DRM_FORMAT_XRGB1555, },
+		{ PIXEL_FORMAT_RGB565, DRM_FORMAT_RGB565, },
+		{ PIXEL_FORMAT_RGB888, DRM_FORMAT_RGB888, },
+		{ PIXEL_FORMAT_XRGB8888, DRM_FORMAT_XRGB8888, },
+		{ PIXEL_FORMAT_XBGR8888, DRM_FORMAT_XBGR8888, },
+		{ PIXEL_FORMAT_XRGB2101010, DRM_FORMAT_XRGB2101010, },
+	};
+	const struct pixel_format pixel = {
+		.bits_per_pixel = fb->bits_per_pixel,
+		.indexed  = false,
+		.alpha = {
+			.offset = 0,
+			.length = 0,
+		},
+		.red = {
+			.offset = fb->red_mask_pos,
+			.length = fb->red_mask_size,
+		},
+		.green = {
+			.offset = fb->green_mask_pos,
+			.length = fb->green_mask_size,
+		},
+		.blue = {
+			.offset = fb->blue_mask_pos,
+			.length = fb->blue_mask_size,
+		},
+	};
+
+	return drm_sysfb_get_format(dev, formats, ARRAY_SIZE(formats), &pixel);
+}
+
+static int corebootdrm_get_width_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	return drm_sysfb_get_validated_int0(dev, "width", fb->x_resolution, INT_MAX);
+}
+
+static int corebootdrm_get_height_fb(struct drm_device *dev, const struct lb_framebuffer *fb)
+{
+	return drm_sysfb_get_validated_int0(dev, "height", fb->y_resolution, INT_MAX);
+}
+
+static int corebootdrm_get_pitch_fb(struct drm_device *dev, const struct drm_format_info *format,
+				    unsigned int width, const struct lb_framebuffer *fb)
+{
+	u64 bytes_per_line = fb->bytes_per_line;
+
+	if (!bytes_per_line)
+		bytes_per_line = drm_format_info_min_pitch(format, 0, width);
+
+	return drm_sysfb_get_validated_int0(dev, "pitch", bytes_per_line, INT_MAX);
+}
+
+static resource_size_t corebootdrm_get_size_fb(struct drm_device *dev, unsigned int height,
+					       unsigned int pitch,
+					       const struct lb_framebuffer *fb)
+{
+	resource_size_t size;
+
+	if (check_mul_overflow(height, pitch, &size))
+		return 0;
+
+	return size;
+}
+
+static phys_addr_t corebootdrm_get_address_fb(struct drm_device *dev, resource_size_t size,
+					      const struct lb_framebuffer *fb)
+{
+	if (size > PHYS_ADDR_MAX)
+		return 0;
+	if (!fb->physical_address)
+		return 0;
+	if (fb->physical_address > (PHYS_ADDR_MAX - size))
+		return 0;
+
+	return fb->physical_address;
+}
+
+/*
+ * Simple Framebuffer device
+ */
+
+struct corebootdrm_device {
+	struct drm_sysfb_device sysfb;
+
+	/* modesetting */
+	u32 formats[DRM_SYSFB_PLANE_NFORMATS(1)];
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+};
+
+/*
+ * Modesetting
+ */
+
+static const u64 corebootdrm_primary_plane_format_modifiers[] = {
+	DRM_SYSFB_PLANE_FORMAT_MODIFIERS,
+};
+
+static const struct drm_plane_helper_funcs corebootdrm_primary_plane_helper_funcs = {
+	DRM_SYSFB_PLANE_HELPER_FUNCS,
+};
+
+static const struct drm_plane_funcs corebootdrm_primary_plane_funcs = {
+	DRM_SYSFB_PLANE_FUNCS,
+	.destroy = drm_plane_cleanup,
+};
+
+static const struct drm_crtc_helper_funcs corebootdrm_crtc_helper_funcs = {
+	DRM_SYSFB_CRTC_HELPER_FUNCS,
+};
+
+static const struct drm_crtc_funcs corebootdrm_crtc_funcs = {
+	DRM_SYSFB_CRTC_FUNCS,
+	.destroy = drm_crtc_cleanup,
+};
+
+static const struct drm_encoder_funcs corebootdrm_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static const struct drm_connector_helper_funcs corebootdrm_connector_helper_funcs = {
+	DRM_SYSFB_CONNECTOR_HELPER_FUNCS,
+};
+
+static const struct drm_connector_funcs corebootdrm_connector_funcs = {
+	DRM_SYSFB_CONNECTOR_FUNCS,
+	.destroy = drm_connector_cleanup,
+};
+
+static const struct drm_mode_config_funcs corebootdrm_mode_config_funcs = {
+	DRM_SYSFB_MODE_CONFIG_FUNCS,
+};
+
+static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
+{
+	struct drm_sysfb_device *sysfb = &cdev->sysfb;
+	struct drm_device *dev = &sysfb->dev;
+	const struct drm_format_info *format = sysfb->fb_format;
+	unsigned int width = sysfb->fb_mode.hdisplay;
+	unsigned int height = sysfb->fb_mode.vdisplay;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	size_t nformats;
+	int ret;
+
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+
+	dev->mode_config.min_width = width;
+	dev->mode_config.max_width = max_t(unsigned int, width, DRM_SHADOW_PLANE_MAX_WIDTH);
+	dev->mode_config.min_height = height;
+	dev->mode_config.max_height = max_t(unsigned int, height, DRM_SHADOW_PLANE_MAX_HEIGHT);
+	dev->mode_config.funcs = &corebootdrm_mode_config_funcs;
+	dev->mode_config.preferred_depth = format->depth;
+
+	/* Primary plane */
+
+	nformats = drm_sysfb_build_fourcc_list(dev, &format->format, 1,
+					       cdev->formats, ARRAY_SIZE(cdev->formats));
+
+	primary_plane = &cdev->primary_plane;
+	ret = drm_universal_plane_init(dev, primary_plane, 0, &corebootdrm_primary_plane_funcs,
+				       cdev->formats, nformats,
+				       corebootdrm_primary_plane_format_modifiers,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+	drm_plane_helper_add(primary_plane, &corebootdrm_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	/* CRTC */
+
+	crtc = &cdev->crtc;
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&corebootdrm_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+	drm_crtc_helper_add(crtc, &corebootdrm_crtc_helper_funcs);
+
+	/* Encoder */
+
+	encoder = &cdev->encoder;
+	ret = drm_encoder_init(dev, encoder, &corebootdrm_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	/* Connector */
+
+	connector = &cdev->connector;
+	ret = drm_connector_init(dev, connector, &corebootdrm_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret)
+		return ret;
+	drm_connector_helper_add(connector, &corebootdrm_connector_helper_funcs);
+	drm_connector_set_panel_orientation_with_quirk(connector,
+						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+						       width, height);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_FOPS(corebootdrm_fops);
+
+static struct drm_driver corebootdrm_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &corebootdrm_fops,
+};
+
+/*
+ * Coreboot driver
+ */
+
+static int corebootdrm_probe(struct platform_device *pdev)
+{
+	const struct lb_framebuffer *fb = dev_get_platdata(&pdev->dev);
+	struct corebootdrm_device *cdev;
+	struct drm_sysfb_device *sysfb;
+	struct drm_device *dev;
+	const struct drm_format_info *format;
+	int width, height, pitch;
+	resource_size_t size;
+	phys_addr_t address;
+	struct resource *res, *mem = NULL;
+	struct resource aperture;
+	void __iomem *screen_base;
+	int ret;
+
+	cdev = devm_drm_dev_alloc(&pdev->dev, &corebootdrm_drm_driver,
+				  struct corebootdrm_device, sysfb.dev);
+	if (IS_ERR(cdev))
+		return PTR_ERR(cdev);
+	platform_set_drvdata(pdev, cdev);
+
+	sysfb = &cdev->sysfb;
+	dev = &sysfb->dev;
+
+	if (!fb) {
+		drm_err(dev, "coreboot framebuffer not found\n");
+		return -EINVAL;
+	} else if (!LB_FRAMEBUFFER_HAS_LFB(fb)) {
+		drm_err(dev, "coreboot framebuffer entry too small\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Hardware settings
+	 */
+
+	format = corebootdrm_get_format_fb(dev, fb);
+	if (!format)
+		return -EINVAL;
+	width = corebootdrm_get_width_fb(dev, fb);
+	if (width < 0)
+		return width;
+	height = corebootdrm_get_height_fb(dev, fb);
+	if (height < 0)
+		return height;
+	pitch = corebootdrm_get_pitch_fb(dev, format, width, fb);
+	if (pitch < 0)
+		return pitch;
+	size = corebootdrm_get_size_fb(dev, height, pitch, fb);
+	if (!size)
+		return -EINVAL;
+	address = corebootdrm_get_address_fb(dev, size, fb);
+	if (!address)
+		return -EINVAL;
+
+	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
+	sysfb->fb_format = format;
+	sysfb->fb_pitch = pitch;
+
+	drm_dbg(dev, "display mode={" DRM_MODE_FMT "}\n", DRM_MODE_ARG(&sysfb->fb_mode));
+	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, pitch=%d byte\n",
+		&format->format, width, height, pitch);
+
+	/*
+	 * Memory management
+	 */
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		drm_err(dev, "memory resource not found\n");
+		return -EINVAL;
+	}
+
+	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
+				      dev->driver->name);
+	if (!mem) {
+		drm_warn(dev, "could not acquire memory resource at %pr\n", res);
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about. Use
+		 * the memory resource as-is and try to map that instead.
+		 */
+		mem = res;
+	}
+
+	drm_dbg(dev, "using memory resource at %pr\n", mem);
+
+	aperture = DEFINE_RES_MEM(address, size);
+	if (!resource_contains(mem, &aperture)) {
+		drm_err(dev, "framebuffer aperture at invalid memory range %pr\n", &aperture);
+		return -EINVAL;
+	}
+
+	ret = devm_aperture_acquire_for_platform_device(pdev, address, size);
+	if (ret) {
+		drm_err(dev, "could not acquire framebuffer aperture: %d\n", ret);
+		return ret;
+	}
+
+	screen_base = devm_ioremap_wc(&pdev->dev, address, size);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr_iomem(&sysfb->fb_addr, screen_base);
+
+	/*
+	 * DRM mode setting and registration
+	 */
+
+	ret = corebootdrm_mode_config_init(cdev);
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(dev);
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		return ret;
+
+	drm_client_setup(dev, sysfb->fb_format);
+
+	return 0;
+}
+
+static void corebootdrm_remove(struct platform_device *pdev)
+{
+	struct corebootdrm_device *cdev = platform_get_drvdata(pdev);
+	struct drm_device *dev = &cdev->sysfb.dev;
+
+	drm_dev_unplug(dev);
+}
+
+static struct platform_driver corebootdrm_platform_driver = {
+	.driver = {
+		.name = "coreboot-framebuffer",
+	},
+	.probe = corebootdrm_probe,
+	.remove = corebootdrm_remove,
+};
+
+module_platform_driver(corebootdrm_platform_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index 5746b99a070d..b51665165f9f 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -14,6 +14,7 @@
 
 #include <linux/compiler_attributes.h>
 #include <linux/types.h>
+#include <linux/stddef.h>
 
 typedef __aligned(4) u64 cb_u64;
 
@@ -66,4 +67,11 @@ struct lb_framebuffer {
 	u8  reserved_mask_size;
 };
 
+/*
+ * True if the coreboot-provided data is large enough to hold information
+ * on the linear framebuffer. False otherwise.
+ */
+#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
+	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
+
 #endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

[PATCH v3 12/12] drm/sysfb: corebootdrm: Support panel orientation

[Thomas Zimmermann]

Add fields and constants for coreboot framebuffer orientation. Set
corebootdrm's DRM connector state from the values. Not all firmware
provides orientation, so make it optional. Systems without, continue
to use unknown orientation.

v3:
- comment on _HAS_ORIENTATION semantics (Tzung-Bi)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Julius Werner <jwerner@chromium.org>
---
 drivers/gpu/drm/sysfb/corebootdrm.c | 30 +++++++++++++++++++++++++----
 include/linux/coreboot.h            | 13 +++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
index 745318580a5d..5dc6f3c76f7b 100644
--- a/drivers/gpu/drm/sysfb/corebootdrm.c
+++ b/drivers/gpu/drm/sysfb/corebootdrm.c
@@ -110,6 +110,26 @@ static phys_addr_t corebootdrm_get_address_fb(struct drm_device *dev, resource_s
 	return fb->physical_address;
 }
 
+static enum drm_panel_orientation corebootdrm_get_orientation_fb(struct drm_device *dev,
+								 const struct lb_framebuffer *fb)
+{
+	if (!LB_FRAMEBUFFER_HAS_ORIENTATION(fb))
+		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+
+	switch (fb->orientation) {
+	case LB_FRAMEBUFFER_ORIENTATION_NORMAL:
+		return DRM_MODE_PANEL_ORIENTATION_NORMAL;
+	case LB_FRAMEBUFFER_ORIENTATION_BOTTOM_UP:
+		return DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+	case LB_FRAMEBUFFER_ORIENTATION_LEFT_UP:
+		return DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+	case LB_FRAMEBUFFER_ORIENTATION_RIGHT_UP:
+		return DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+	}
+
+	return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+}
+
 /*
  * Simple Framebuffer device
  */
@@ -168,7 +188,8 @@ static const struct drm_mode_config_funcs corebootdrm_mode_config_funcs = {
 	DRM_SYSFB_MODE_CONFIG_FUNCS,
 };
 
-static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
+static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev,
+					enum drm_panel_orientation orientation)
 {
 	struct drm_sysfb_device *sysfb = &cdev->sysfb;
 	struct drm_device *dev = &sysfb->dev;
@@ -234,8 +255,7 @@ static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev)
 	if (ret)
 		return ret;
 	drm_connector_helper_add(connector, &corebootdrm_connector_helper_funcs);
-	drm_connector_set_panel_orientation_with_quirk(connector,
-						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+	drm_connector_set_panel_orientation_with_quirk(connector, orientation,
 						       width, height);
 
 	ret = drm_connector_attach_encoder(connector, encoder);
@@ -276,6 +296,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	int width, height, pitch;
 	resource_size_t size;
 	phys_addr_t address;
+	enum drm_panel_orientation orientation;
 	struct resource *res, *mem = NULL;
 	struct resource aperture;
 	void __iomem *screen_base;
@@ -320,6 +341,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	address = corebootdrm_get_address_fb(dev, size, fb);
 	if (!address)
 		return -EINVAL;
+	orientation = corebootdrm_get_orientation_fb(dev, fb);
 
 	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
 	sysfb->fb_format = format;
@@ -375,7 +397,7 @@ static int corebootdrm_probe(struct platform_device *pdev)
 	 * DRM mode setting and registration
 	 */
 
-	ret = corebootdrm_mode_config_init(cdev);
+	ret = corebootdrm_mode_config_init(cdev, orientation);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
index b51665165f9f..de9652e1a53f 100644
--- a/include/linux/coreboot.h
+++ b/include/linux/coreboot.h
@@ -47,6 +47,11 @@ struct lb_cbmem_entry {
 	u32 id;
 };
 
+#define LB_FRAMEBUFFER_ORIENTATION_NORMAL	0
+#define LB_FRAMEBUFFER_ORIENTATION_BOTTOM_UP	1
+#define LB_FRAMEBUFFER_ORIENTATION_LEFT_UP	2
+#define LB_FRAMEBUFFER_ORIENTATION_RIGHT_UP	3
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -65,6 +70,7 @@ struct lb_framebuffer {
 	u8  blue_mask_size;
 	u8  reserved_mask_pos;
 	u8  reserved_mask_size;
+	u8  orientation;
 };
 
 /*
@@ -74,4 +80,11 @@ struct lb_framebuffer {
 #define LB_FRAMEBUFFER_HAS_LFB(__fb) \
 	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
 
+/*
+ * True if the coreboot-provided data is large enough to hold information
+ * on the display orientation. False otherwise.
+ */
+#define LB_FRAMEBUFFER_HAS_ORIENTATION(__fb) \
+	((__fb)->size >= offsetofend(struct lb_framebuffer, orientation))
+
 #endif /* _LINUX_COREBOOT_H */
-- 
2.52.0

Re: [PATCH v3 09/12] firmware: google: Pack structures for coreboot table entries

[Tzung-Bi Shih]

On Tue, Feb 03, 2026 at 02:52:28PM +0100, Thomas Zimmermann wrote:
> Pack the fields in the coreboot table entries. These entries are part of
> the coreboot ABI, so they don't follow regular calling conventions. Fields
> of type u64 are aligned to boundaries of 4 bytes instead of 8. [1]
> 
> So far this has not been a problem. In the future, padding bytes should
> be added where explicit alignment is required.
> 
> v2:
> - use aligned 64-bit type instead of __packed__ attribute (Julius)

Changelog should be after "---" otherwise it becomes part of commit message.

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://github.com/coreboot/coreboot/blob/main/payloads/libpayload/include/coreboot_tables.h#L96 # [1]
> Suggested-by: Julius Werner <jwerner@chromium.org>
> Acked-by: Julius Werner <jwerner@chromium.org>

Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v3 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Tzung-Bi Shih]

On Tue, Feb 03, 2026 at 02:52:30PM +0100, Thomas Zimmermann wrote:
> Add corebootdrm, a DRM driver for coreboot framebuffers. The driver
> supports a pre-initialized framebuffer with various packed RGB formats.
> The driver code is fairly small and uses the same logic as the other
> sysfb drivers. Most of the implementation comes from existing sysfb
> helpers.
> 
> Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
> output. Initialize the platform device for corebootdrm in the same place
> in framebuffer_probe(). With a later commit, the simple-framebuffer should
> be removed.
> 
> v3:
> - comment on _HAS_LFB semantics (Tzung-Bi)
> - fix typo in commit description (Tzung-Bi)
> - comment on simple-framebuffer being obsolete for coreboot
> v2:
> - reimplement as platform driver
> - limit resources and mappings to known framebuffer memory; no
>   page alignment
> - create corebootdrm device from coreboot framebuffer code

Changelog should be after "---" otherwise it becomes part of commit message.

> +static int corebootdrm_probe(struct platform_device *pdev)
> +{
[...]
> +	if (!fb) {
> +		drm_err(dev, "coreboot framebuffer not found\n");
> +		return -EINVAL;
> +	} else if (!LB_FRAMEBUFFER_HAS_LFB(fb)) {
> +		drm_err(dev, "coreboot framebuffer entry too small\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Hardware settings
> +	 */
> +
> +	format = corebootdrm_get_format_fb(dev, fb);
> +	if (!format)
> +		return -EINVAL;
> +	width = corebootdrm_get_width_fb(dev, fb);
> +	if (width < 0)
> +		return width;
> +	height = corebootdrm_get_height_fb(dev, fb);
> +	if (height < 0)
> +		return height;
[...]
> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
> index 5746b99a070d..b51665165f9f 100644
> --- a/include/linux/coreboot.h
> +++ b/include/linux/coreboot.h
> @@ -14,6 +14,7 @@
>  
>  #include <linux/compiler_attributes.h>
>  #include <linux/types.h>
> +#include <linux/stddef.h>

Move it before types.h?  's' vs. 't'.

> +/*
> + * True if the coreboot-provided data is large enough to hold information
> + * on the linear framebuffer. False otherwise.
> + */
> +#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
> +	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
> +

To make sure I understand, do you mean:

- The __fb->size is possibly less than sizeof(struct lb_framebuffer).
  LB_FRAMEBUFFER_HAS_LFB() is for checking the following fields
  (e.g. fb->x_resolution) are available?

    struct lb_framebuffer {
    	u32 tag;
    	u32 size;

    	u64 physical_address;
    	u32 x_resolution;
    	u32 y_resolution;
    	u32 bytes_per_line;
    	u8  bits_per_pixel;
    	u8  red_mask_pos;
    	u8  red_mask_size;
    	u8  green_mask_pos;
    	u8  green_mask_size;
    	u8  blue_mask_pos;
    	u8  blue_mask_size;
    	u8  reserved_mask_pos;
    	u8  reserved_mask_size;
    };

- If answer of the previous question is yes, the next question: does
  LB_FRAMEBUFFER_HAS_LFB() needs to check up to `reserved_mask_size`?
  As in the patch, it only accesses up to `blue_mask_size`.

Re: [PATCH v3 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Thomas Zimmermann]

Hi

Am 06.02.26 um 06:14 schrieb Tzung-Bi Shih:
> On Tue, Feb 03, 2026 at 02:52:30PM +0100, Thomas Zimmermann wrote:
>> Add corebootdrm, a DRM driver for coreboot framebuffers. The driver
>> supports a pre-initialized framebuffer with various packed RGB formats.
>> The driver code is fairly small and uses the same logic as the other
>> sysfb drivers. Most of the implementation comes from existing sysfb
>> helpers.
>>
>> Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
>> output. Initialize the platform device for corebootdrm in the same place
>> in framebuffer_probe(). With a later commit, the simple-framebuffer should
>> be removed.
>>
>> v3:
>> - comment on _HAS_LFB semantics (Tzung-Bi)
>> - fix typo in commit description (Tzung-Bi)
>> - comment on simple-framebuffer being obsolete for coreboot
>> v2:
>> - reimplement as platform driver
>> - limit resources and mappings to known framebuffer memory; no
>>    page alignment
>> - create corebootdrm device from coreboot framebuffer code
> Changelog should be after "---" otherwise it becomes part of commit message.

I see. In DRM land, we usually keep the change log in the commit 
message. I'll change it for the coreboot patches, but I'd rather keep it 
for the DRM patches. I can split off the coreboot changes for this patch 
into its own.

>
>> +static int corebootdrm_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	if (!fb) {
>> +		drm_err(dev, "coreboot framebuffer not found\n");
>> +		return -EINVAL;
>> +	} else if (!LB_FRAMEBUFFER_HAS_LFB(fb)) {
>> +		drm_err(dev, "coreboot framebuffer entry too small\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Hardware settings
>> +	 */
>> +
>> +	format = corebootdrm_get_format_fb(dev, fb);
>> +	if (!format)
>> +		return -EINVAL;
>> +	width = corebootdrm_get_width_fb(dev, fb);
>> +	if (width < 0)
>> +		return width;
>> +	height = corebootdrm_get_height_fb(dev, fb);
>> +	if (height < 0)
>> +		return height;
> [...]
>> diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
>> index 5746b99a070d..b51665165f9f 100644
>> --- a/include/linux/coreboot.h
>> +++ b/include/linux/coreboot.h
>> @@ -14,6 +14,7 @@
>>   
>>   #include <linux/compiler_attributes.h>
>>   #include <linux/types.h>
>> +#include <linux/stddef.h>
> Move it before types.h?  's' vs. 't'.
>
>> +/*
>> + * True if the coreboot-provided data is large enough to hold information
>> + * on the linear framebuffer. False otherwise.
>> + */
>> +#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
>> +	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
>> +
> To make sure I understand, do you mean:
>
> - The __fb->size is possibly less than sizeof(struct lb_framebuffer).
>    LB_FRAMEBUFFER_HAS_LFB() is for checking the following fields
>    (e.g. fb->x_resolution) are available?

Yes.

>
>      struct lb_framebuffer {
>      	u32 tag;
>      	u32 size;
>
>      	u64 physical_address;
>      	u32 x_resolution;
>      	u32 y_resolution;
>      	u32 bytes_per_line;
>      	u8  bits_per_pixel;
>      	u8  red_mask_pos;
>      	u8  red_mask_size;
>      	u8  green_mask_pos;
>      	u8  green_mask_size;
>      	u8  blue_mask_pos;
>      	u8  blue_mask_size;
>      	u8  reserved_mask_pos;
>      	u8  reserved_mask_size;
>      };
>
> - If answer of the previous question is yes, the next question: does
>    LB_FRAMEBUFFER_HAS_LFB() needs to check up to `reserved_mask_size`?
>    As in the patch, it only accesses up to `blue_mask_size`.

Well, it's a correctness thing. The reserved_mask fields have been part 
of the structure since the first version in commit b700254aa5 ("Add 
coreboot framebuffer support to libpayload"). I'd that expect that the 
framebuffer entry is bogus if it does not contain these fields. If you 
really want to leave them out, we can do that of course.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH v3 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Tzung-Bi Shih]

On Fri, Feb 06, 2026 at 08:44:02AM +0100, Thomas Zimmermann wrote:
> Am 06.02.26 um 06:14 schrieb Tzung-Bi Shih:
> > On Tue, Feb 03, 2026 at 02:52:30PM +0100, Thomas Zimmermann wrote:
> > > Add corebootdrm, a DRM driver for coreboot framebuffers. The driver
> > > supports a pre-initialized framebuffer with various packed RGB formats.
> > > The driver code is fairly small and uses the same logic as the other
> > > sysfb drivers. Most of the implementation comes from existing sysfb
> > > helpers.
> > > 
> > > Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
> > > output. Initialize the platform device for corebootdrm in the same place
> > > in framebuffer_probe(). With a later commit, the simple-framebuffer should
> > > be removed.
> > > 
> > > v3:
> > > - comment on _HAS_LFB semantics (Tzung-Bi)
> > > - fix typo in commit description (Tzung-Bi)
> > > - comment on simple-framebuffer being obsolete for coreboot
> > > v2:
> > > - reimplement as platform driver
> > > - limit resources and mappings to known framebuffer memory; no
> > >    page alignment
> > > - create corebootdrm device from coreboot framebuffer code
> > Changelog should be after "---" otherwise it becomes part of commit message.
> 
> I see. In DRM land, we usually keep the change log in the commit message.
> I'll change it for the coreboot patches, but I'd rather keep it for the DRM
> patches. I can split off the coreboot changes for this patch into its own.

I see.  Please keep it if this is a convention in DRM land.

> > > +static int corebootdrm_probe(struct platform_device *pdev)
> > > +{
> > [...]
> > > +	if (!fb) {
> > > +		drm_err(dev, "coreboot framebuffer not found\n");
> > > +		return -EINVAL;
> > > +	} else if (!LB_FRAMEBUFFER_HAS_LFB(fb)) {
> > > +		drm_err(dev, "coreboot framebuffer entry too small\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Hardware settings
> > > +	 */
> > > +
> > > +	format = corebootdrm_get_format_fb(dev, fb);
> > > +	if (!format)
> > > +		return -EINVAL;
> > > +	width = corebootdrm_get_width_fb(dev, fb);
> > > +	if (width < 0)
> > > +		return width;
> > > +	height = corebootdrm_get_height_fb(dev, fb);
> > > +	if (height < 0)
> > > +		return height;
> > [...]
> > > diff --git a/include/linux/coreboot.h b/include/linux/coreboot.h
> > > index 5746b99a070d..b51665165f9f 100644
> > > --- a/include/linux/coreboot.h
> > > +++ b/include/linux/coreboot.h
> > > @@ -14,6 +14,7 @@
> > >   #include <linux/compiler_attributes.h>
> > >   #include <linux/types.h>
> > > +#include <linux/stddef.h>
> > Move it before types.h?  's' vs. 't'.
> > 
> > > +/*
> > > + * True if the coreboot-provided data is large enough to hold information
> > > + * on the linear framebuffer. False otherwise.
> > > + */
> > > +#define LB_FRAMEBUFFER_HAS_LFB(__fb) \
> > > +	((__fb)->size >= offsetofend(struct lb_framebuffer, reserved_mask_size))
> > > +
> > To make sure I understand, do you mean:
> > 
> > - The __fb->size is possibly less than sizeof(struct lb_framebuffer).
> >    LB_FRAMEBUFFER_HAS_LFB() is for checking the following fields
> >    (e.g. fb->x_resolution) are available?
> 
> Yes.
> 
> > 
> >      struct lb_framebuffer {
> >      	u32 tag;
> >      	u32 size;
> > 
> >      	u64 physical_address;
> >      	u32 x_resolution;
> >      	u32 y_resolution;
> >      	u32 bytes_per_line;
> >      	u8  bits_per_pixel;
> >      	u8  red_mask_pos;
> >      	u8  red_mask_size;
> >      	u8  green_mask_pos;
> >      	u8  green_mask_size;
> >      	u8  blue_mask_pos;
> >      	u8  blue_mask_size;
> >      	u8  reserved_mask_pos;
> >      	u8  reserved_mask_size;
> >      };
> > 
> > - If answer of the previous question is yes, the next question: does
> >    LB_FRAMEBUFFER_HAS_LFB() needs to check up to `reserved_mask_size`?
> >    As in the patch, it only accesses up to `blue_mask_size`.
> 
> Well, it's a correctness thing. The reserved_mask fields have been part of
> the structure since the first version in commit b700254aa5 ("Add coreboot
> framebuffer support to libpayload"). I'd that expect that the framebuffer
> entry is bogus if it does not contain these fields. If you really want to
> leave them out, we can do that of course.

I see.  All makes sense.  Let's leave it as is.

For drivers/firmware/google/ and include/linux/coreboot.h,
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v3 12/12] drm/sysfb: corebootdrm: Support panel orientation

[Tzung-Bi Shih]

On Tue, Feb 03, 2026 at 02:52:31PM +0100, Thomas Zimmermann wrote:
> Add fields and constants for coreboot framebuffer orientation. Set
> corebootdrm's DRM connector state from the values. Not all firmware
> provides orientation, so make it optional. Systems without, continue
> to use unknown orientation.
> 
> v3:
> - comment on _HAS_ORIENTATION semantics (Tzung-Bi)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Julius Werner <jwerner@chromium.org>

For include/linux/coreboot.h,
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v3 11/12] drm/sysfb: corebootdrm: Add DRM driver for coreboot framebuffers

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Add corebootdrm, a DRM driver for coreboot framebuffers. The driver
> supports a pre-initialized framebuffer with various packed RGB formats.
> The driver code is fairly small and uses the same logic as the other
> sysfb drivers. Most of the implementation comes from existing sysfb
> helpers.
>
> Until now, coreboot relied on simpledrm or simplefb for boot-up graphics
> output. Initialize the platform device for corebootdrm in the same place
> in framebuffer_probe(). With a later commit, the simple-framebuffer should
> be removed.
>
> v3:
> - comment on _HAS_LFB semantics (Tzung-Bi)
> - fix typo in commit description (Tzung-Bi)
> - comment on simple-framebuffer being obsolete for coreboot
> v2:
> - reimplement as platform driver
> - limit resources and mappings to known framebuffer memory; no
>   page alignment
> - create corebootdrm device from coreboot framebuffer code
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Julius Werner <jwerner@chromium.org>
> ---

The driver looks good to me. I couldn't spot anything wrong on it.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v3 12/12] drm/sysfb: corebootdrm: Support panel orientation

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Add fields and constants for coreboot framebuffer orientation. Set
> corebootdrm's DRM connector state from the values. Not all firmware
> provides orientation, so make it optional. Systems without, continue
> to use unknown orientation.
>
> v3:
> - comment on _HAS_ORIENTATION semantics (Tzung-Bi)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Julius Werner <jwerner@chromium.org>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat