[PATCH v2 0/7] Add support for QEMU's ramfb display

[PATCH v2 0/7] Add support for QEMU's ramfb display

[Alper Nebi Yasak]

This is a rebase of Alexander Graf's QEMU ramfb support series with
some changes of mine, in part to address reviews and to accommodate
other changes enabling Bochs video driver. The original cover letter is
as follows:

> QEMU implements multiple ways to expose graphics output to the virt
> machine, but most of them are incompatible with hardware virtualization.
>
> The one that does work reliably is ramfb. It's a very simple mechanism
> in which the guest reserves a memory region for the frame buffer and then
> notifies the host about its location and properties. The host then just
> displays the contents of the frame buffer on screen.
>
> This patch set adds support to drive the ramfb device in our QEMU arm
> targets. Theoretically, it should work as is with any of the other
> architectures as well though.
>
> With this driver, we have a very simple, KVM compatible way to expose
> GOP via UEFI to payloads and thus enable development and testing of
> graphical OS functionality with QEMU's -M virt.

Inspired by the "video: bochs: Remove the x86 limitation" series [1]
that was recently merged, I have also sent a series adding Bochs
video device support for ARM [2], and assuming that would get merged
before this I've rebased this on that to avoid conflicts.

[1] https://lore.kernel.org/u-boot/20230723044041.1089804-1-bmeng@tinylab.org/
[2] https://lore.kernel.org/u-boot/20230814173944.288356-1-alpernebiyasak@gmail.com/

Changes in v2:
- Remove extra "depends on DM_VIDEO" already in "if VIDEO"
- Drop drivers/video/MAINTAINERS file
- Decouple framebuffer allocation from EFI_LOADER
- Add .bind() method for ramfb driver
- Make resolution configurable with kconfig
- Move struct to qfw.h and add comments for members
- Use RAMFB_* definitions instead of DEFAULT_*
- Use if (IS_ENABLED(CONFIG_VIDEO_RAMFB)) instead of #ifdef
- Add patch "qfw: Add flag to allow probing before relocation"
- Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
- Drop env changes from ARM (necessary but in prerequisite series)
- Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
- Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
- Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
- Drop imply VIDEO, env changes from RISC-V patch (in u-boot/next)
- Drop imply SYS_CONSOLE_IS_IN_ENV from RISC-V patch (def. y if VIDEO)
- Probe QFW in RISC-V QEMU board_early_init_f to bind ramfb pre-reloc
- Add IS_ENABLED(CONFIG_QFW) check and error handling to RISC-V QEMU
- Add patch "x86: qemu: Enable ramfb by default"

v1: https://lore.kernel.org/u-boot/20220227144043.37359-1-agraf@csgraf.de/

Alexander Graf (5):
  qfw: Add WRITE definition
  ramfb: Add driver for ramfb display
  qfw: Spawn ramfb device if its file is present
  arm: qemu: Enable ramfb by default
  riscv: qemu: Enable ramfb by default

Alper Nebi Yasak (2):
  qfw: Add flag to allow probing before relocation
  x86: qemu: Enable ramfb by default

 arch/arm/Kconfig                        |  3 +
 arch/x86/cpu/qemu/Kconfig               |  4 +
 board/emulation/qemu-arm/qemu-arm.c     | 41 +++++++++++
 board/emulation/qemu-riscv/Kconfig      |  5 ++
 board/emulation/qemu-riscv/qemu-riscv.c | 41 +++++++++++
 board/emulation/qemu-x86/qemu-x86.c     | 47 ++++++++++++
 configs/qemu-x86_64_defconfig           |  4 +-
 configs/qemu-x86_defconfig              |  1 -
 drivers/misc/qfw.c                      | 25 +++++++
 drivers/misc/qfw_mmio.c                 |  1 +
 drivers/misc/qfw_pio.c                  |  1 +
 drivers/misc/qfw_sandbox.c              |  1 +
 drivers/video/Kconfig                   | 30 ++++++++
 drivers/video/Makefile                  |  1 +
 drivers/video/ramfb.c                   | 97 +++++++++++++++++++++++++
 include/qfw.h                           | 11 +++
 16 files changed, 309 insertions(+), 4 deletions(-)
 create mode 100644 drivers/video/ramfb.c


base-commit: 832148f675e427060be074c276956962fa9b5cb6
prerequisite-patch-id: 2c8662d18f6f1e2d4e014bcd5d468e0396064f4f
prerequisite-patch-id: 5f3e5e1d814187841ce299df0da16df16d0121c3
prerequisite-patch-id: adbd0fb953e726ea5ea0b248e00092dfc318fd46
prerequisite-patch-id: 7470d3af5e9ded16eb5a4de7c7264062dcb3ef7f
-- 
2.40.1

[PATCH v2 1/7] qfw: Add WRITE definition

[Alper Nebi Yasak]

From: Alexander Graf <agraf@csgraf.de>

The QEMU fw_cfg device supports writing entries as well. Add the constant
define for it so that we can leverage write functionality later.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

(no changes since v1)

 include/qfw.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qfw.h b/include/qfw.h
index 42798fea7db4..d3aaa4d54efc 100644
--- a/include/qfw.h
+++ b/include/qfw.h
@@ -70,6 +70,7 @@ enum {
 #define FW_CFG_DMA_READ	(1 << 1)
 #define FW_CFG_DMA_SKIP	(1 << 2)
 #define FW_CFG_DMA_SELECT	(1 << 3)
+#define FW_CFG_DMA_WRITE	(1 << 4)
 
 /* Bit set in FW_CFG_ID response to indicate DMA interface availability. */
 #define FW_CFG_DMA_ENABLED	(1 << 1)
-- 
2.40.1

[PATCH v2 2/7] ramfb: Add driver for ramfb display

[Alper Nebi Yasak]

From: Alexander Graf <agraf@csgraf.de>

QEMU implements multiple ways to expose graphics output to the virt
machine, but most of them are incompatible with hardware virtualization.

The one that does work reliably is ramfb. It's a very simple mechanism
in which the guest reserves a memory region for the frame buffer and then
notifies the host about its location and properties. The host then just
displays the contents of the frame buffer on screen.

This patch implements a trivial version of a ramfb driver - hard coded
to a single resolution set in Kconfig.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
[Alper: Deduplicate depends on DM_VIDEO, drop MAINTAINERS, decouple from
        EFI_LOADER, add .bind(), kconfigurable resolution, struct in .h]
Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

Changes in v2:
- Remove extra "depends on DM_VIDEO" already in "if VIDEO"
- Drop drivers/video/MAINTAINERS file
- Decouple framebuffer allocation from EFI_LOADER
- Add .bind() method for ramfb driver
- Make resolution configurable with kconfig
- Move struct to qfw.h and add comments for members
- Use RAMFB_* definitions instead of DEFAULT_*

 drivers/video/Kconfig  | 30 +++++++++++++
 drivers/video/Makefile |  1 +
 drivers/video/ramfb.c  | 97 ++++++++++++++++++++++++++++++++++++++++++
 include/qfw.h          | 10 +++++
 4 files changed, 138 insertions(+)
 create mode 100644 drivers/video/ramfb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 69f4809cf4a6..6ab46107483e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -849,6 +849,36 @@ config VIDEO_MCDE_SIMPLE
 	  before u-boot starts, and u-boot will simply render to the pre-
 	  allocated frame buffer surface.
 
+config VIDEO_RAMFB
+	bool "QEMU ramfb display driver for in-RAM display"
+	depends on QFW
+	help
+	  Enables a RAM based simple frame buffer driver which uses qfw to
+	  notify the hypervisor about the location of a Frame Buffer allocated
+	  in guest RAM as well as its properties.
+
+if VIDEO_RAMFB
+
+config VIDEO_RAMFB_SIZE_X
+	int "Width of ramfb display (X resolution)"
+	default 1280
+	help
+	  Sets the width of the ramfb display.
+
+	  These two options control the size of the display set up by QEMU.
+	  Typical sizes are 1024 x 768 or 1280 x 1024.
+
+config VIDEO_RAMFB_SIZE_Y
+	int "Height of ramfb display (Y resolution)"
+	default 1024
+	help
+	  Sets the height of the ramfb display.
+
+	  These two options control the size of the display set up by QEMU.
+	  Typical sizes are 1024 x 768 or 1280 x 1024.
+
+endif
+
 config OSD
 	bool "Enable OSD support"
 	depends on DM
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index d13af9f3b19b..775b4e46e600 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_SANDBOX_SDL) += sandbox_sdl.o
 obj-$(CONFIG_VIDEO_SIMPLE) += simplefb.o
 obj-$(CONFIG_VIDEO_VESA) += vesa.o
 obj-$(CONFIG_VIDEO_SEPS525) += seps525.o
+obj-$(CONFIG_VIDEO_RAMFB) += ramfb.o
 obj-$(CONFIG_VIDEO_ZYNQMP_DPSUB) += zynqmp/
 
 obj-y += bridge/
diff --git a/drivers/video/ramfb.c b/drivers/video/ramfb.c
new file mode 100644
index 000000000000..ba17cac312b6
--- /dev/null
+++ b/drivers/video/ramfb.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2022 Alexander Graf
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <log.h>
+#include <video.h>
+#include <asm/global_data.h>
+#include <qfw.h>
+
+#define fourcc_code(a, b, c, d) ((u32)(a) | ((u32)(b) << 8) | \
+				 ((u32)(c) << 16) | ((u32)(d) << 24))
+#define DRM_FORMAT_XRGB8888 fourcc_code('X', 'R', '2', '4')
+
+#define RAMFB_WIDTH	CONFIG_VIDEO_RAMFB_SIZE_X
+#define RAMFB_HEIGHT	CONFIG_VIDEO_RAMFB_SIZE_Y
+#define RAMFB_BPIX	VIDEO_BPP32
+#define RAMFB_FORMAT	VIDEO_X8R8G8B8
+
+static int ramfb_probe(struct udevice *dev)
+{
+	struct video_uc_plat *plat = dev_get_uclass_plat(dev);
+	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
+	u32 selector;
+	int ret;
+	struct fw_file *file;
+	struct udevice *qfw;
+	struct dm_qfw_ops *ops;
+	struct qfw_dma dma = {};
+	struct ramfb_cfg cfg = {
+		.addr = cpu_to_be64(plat->base),
+		.fourcc = cpu_to_be32(DRM_FORMAT_XRGB8888),
+		.flags = 0,
+		.width = cpu_to_be32(RAMFB_WIDTH),
+		.height = cpu_to_be32(RAMFB_HEIGHT),
+		.stride = 0,
+	};
+
+	if (plat->base == 0)
+		return -EPROBE_DEFER;
+
+	debug("%s: Frame buffer base %lx\n", __func__, plat->base);
+
+	ret = qfw_get_dev(&qfw);
+	if (ret)
+		return -EPROBE_DEFER;
+
+	ops = dm_qfw_get_ops(qfw);
+	if (!ops)
+		return -EPROBE_DEFER;
+
+	file = qfw_find_file(qfw, "etc/ramfb");
+	if (!file) {
+		/* No ramfb available. At least we tried. */
+		return -ENOENT;
+	}
+
+	uc_priv->xsize = RAMFB_WIDTH;
+	uc_priv->ysize = RAMFB_HEIGHT;
+	uc_priv->bpix = RAMFB_BPIX;
+	uc_priv->format = RAMFB_FORMAT;
+	uc_priv->fb = (void *)plat->base;
+	uc_priv->fb_size = plat->size;
+
+	selector = be16_to_cpu(file->cfg.select);
+	dma.length = cpu_to_be32(sizeof(cfg));
+	dma.address = cpu_to_be64((uintptr_t)&cfg);
+	dma.control = cpu_to_be32(FW_CFG_DMA_WRITE | FW_CFG_DMA_SELECT |
+				  (selector << 16));
+
+	barrier();
+
+	/* Send a DMA write request which enables the screen */
+	ops->read_entry_dma(qfw, &dma);
+
+	return 0;
+}
+
+static int ramfb_bind(struct udevice *dev)
+{
+	struct video_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+
+	/* Set the maximum supported resolution */
+	uc_plat->size = RAMFB_WIDTH * RAMFB_HEIGHT * VNBYTES(RAMFB_BPIX);
+	debug("%s: Frame buffer size %x\n", __func__, uc_plat->size);
+
+	return 0;
+}
+
+U_BOOT_DRIVER(ramfb) = {
+	.name	= "ramfb",
+	.id	= UCLASS_VIDEO,
+	.probe	= ramfb_probe,
+	.bind   = ramfb_bind,
+};
diff --git a/include/qfw.h b/include/qfw.h
index d3aaa4d54efc..811abac6fd27 100644
--- a/include/qfw.h
+++ b/include/qfw.h
@@ -142,6 +142,16 @@ struct bios_linker_entry {
 	};
 } __packed;
 
+/* ramfb configuration details to read into etc/ramfb fw_cfg file */
+struct ramfb_cfg {
+	__be64 addr;	/* Address of an allocated framebuffer */
+	__be32 fourcc;	/* Pixel format in Linux DRM fourcc code */
+	__be32 flags;	/* Unknown, appears unused */
+	__be32 width;	/* Width of the requested ramfb display */
+	__be32 height;	/* Height of the requested ramfb display */
+	__be32 stride;	/* Number of bytes per line? */
+} __packed;
+
 /* DMA transfer control data between UCLASS_QFW and QEMU. */
 struct qfw_dma {
 	__be32 control;
-- 
2.40.1

[PATCH v2 3/7] qfw: Spawn ramfb device if its file is present

[Alper Nebi Yasak]

From: Alexander Graf <agraf@csgraf.de>

Now that we have a ramfb device driver, let's add the necessary glueing
magic to also spawn it when we find its qfw file node.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
[Alper: Use if IS_ENABLED() instead of #ifdef]
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

Changes in v2:
- Use if (IS_ENABLED(CONFIG_VIDEO_RAMFB)) instead of #ifdef

 drivers/misc/qfw.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
index 7c01bf23d53b..4e4260982cce 100644
--- a/drivers/misc/qfw.c
+++ b/drivers/misc/qfw.c
@@ -16,6 +16,7 @@
 #include <malloc.h>
 #include <qfw.h>
 #include <dm.h>
+#include <dm/lists.h>
 #include <misc.h>
 #include <tables_csum.h>
 #include <asm/acpi_table.h>
@@ -307,6 +308,27 @@ void qfw_read_entry(struct udevice *dev, u16 entry, u32 size, void *address)
 		qfw_read_entry_io(qdev, entry, size, address);
 }
 
+static void qfw_bind_ramfb(struct udevice *dev)
+{
+	struct fw_file *file;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_VIDEO_RAMFB))
+		return;
+
+	ret = qfw_read_firmware_list(dev);
+	if (ret)
+		return;
+
+	file = qfw_find_file(dev, "etc/ramfb");
+	if (!file) {
+		/* No ramfb available. */
+		return;
+	}
+
+	device_bind_driver(dev, "ramfb", "qfw-ramfb", NULL);
+}
+
 int qfw_register(struct udevice *dev)
 {
 	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
@@ -323,6 +345,8 @@ int qfw_register(struct udevice *dev)
 	if (dma_enabled & FW_CFG_DMA_ENABLED)
 		qdev->dma_present = true;
 
+	qfw_bind_ramfb(dev);
+
 	return 0;
 }
 
-- 
2.40.1

[PATCH v2 4/7] qfw: Add flag to allow probing before relocation

[Alper Nebi Yasak]

QEMU firmware config drivers need to be probed to bind the ramfb device.
The ramfb driver needs to be bound before relocation to properly reserve
video memory for it, otherwise it cannot be probed after relocation. Add
the flag to probe QEMU firmware config drivers before relocation so that
ramfb can work as an initial vidconsole.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
Alternatively, I guess I could default VIDEO_PCI_DEFAULT_FB_SIZE to a
higher size with "if VIDEO_RAMFB". But it exists because "PCI drivers
cannot be bound before relocation unless they are mentioned in the
devicetree" and qfw is in the QEMU-generated devicetree unlike those, so
I assumed this would be the preferred way.

Changes in v2:
- Add patch "qfw: Add flag to allow probing before relocation"

 drivers/misc/qfw.c         | 1 +
 drivers/misc/qfw_mmio.c    | 1 +
 drivers/misc/qfw_pio.c     | 1 +
 drivers/misc/qfw_sandbox.c | 1 +
 4 files changed, 4 insertions(+)

diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
index 4e4260982cce..265f45290011 100644
--- a/drivers/misc/qfw.c
+++ b/drivers/misc/qfw.c
@@ -414,6 +414,7 @@ UCLASS_DRIVER(qfw) = {
 	.name		= "qfw",
 	.post_bind	= qfw_post_bind,
 	.per_device_auto	= sizeof(struct qfw_dev),
+	.flags = DM_FLAG_PRE_RELOC,
 };
 
 struct bootdev_ops qfw_bootdev_ops = {
diff --git a/drivers/misc/qfw_mmio.c b/drivers/misc/qfw_mmio.c
index f397384054a6..a5274982711c 100644
--- a/drivers/misc/qfw_mmio.c
+++ b/drivers/misc/qfw_mmio.c
@@ -116,4 +116,5 @@ U_BOOT_DRIVER(qfw_mmio) = {
 	.of_to_plat	= qfw_mmio_of_to_plat,
 	.probe	= qfw_mmio_probe,
 	.ops	= &qfw_mmio_ops,
+	.flags = DM_FLAG_PRE_RELOC,
 };
diff --git a/drivers/misc/qfw_pio.c b/drivers/misc/qfw_pio.c
index e2f628d33830..dae809bd3fea 100644
--- a/drivers/misc/qfw_pio.c
+++ b/drivers/misc/qfw_pio.c
@@ -66,4 +66,5 @@ U_BOOT_DRIVER(qfw_pio) = {
 	.id	= UCLASS_QFW,
 	.probe	= qfw_pio_probe,
 	.ops	= &qfw_pio_ops,
+	.flags = DM_FLAG_PRE_RELOC,
 };
diff --git a/drivers/misc/qfw_sandbox.c b/drivers/misc/qfw_sandbox.c
index 1002df75339e..0bd36acd1a09 100644
--- a/drivers/misc/qfw_sandbox.c
+++ b/drivers/misc/qfw_sandbox.c
@@ -120,6 +120,7 @@ U_BOOT_DRIVER(qfw_sandbox) = {
 	.plat_auto	= sizeof(struct qfw_sandbox_plat),
 	.probe	= qfw_sandbox_probe,
 	.ops	= &qfw_sandbox_ops,
+	.flags = DM_FLAG_PRE_RELOC,
 };
 
 U_BOOT_DRVINFO(qfw_sandbox) = {
-- 
2.40.1

[PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Alper Nebi Yasak]

From: Alexander Graf <agraf@csgraf.de>

Now that we have everything in place to support ramfb, let's wire it up
by default in the ARM QEMU targets. That way, you can easily use a
graphical console by just passing -device ramfb to the QEMU command line.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
[Alper: Rebase on bochs changes, add pre-reloc init, error handling]
Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

Changes in v2:
- Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
- Drop env changes from ARM (necessary but in prerequisite series)
- Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
- Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
- Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU

 arch/arm/Kconfig                    |  3 +++
 board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 1fd3ccd1607f..7afe26ac804f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1046,6 +1046,9 @@ config ARCH_QEMU
 	imply USB_XHCI_PCI
 	imply USB_KEYBOARD
 	imply CMD_USB
+	imply VIDEO_RAMFB
+	imply BOARD_EARLY_INIT_F
+	imply BOARD_EARLY_INIT_R
 
 config ARCH_RMOBILE
 	bool "Renesas ARM SoCs"
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
index 942f1fff5717..23ef31cb7feb 100644
--- a/board/emulation/qemu-arm/qemu-arm.c
+++ b/board/emulation/qemu-arm/qemu-arm.c
@@ -11,6 +11,7 @@
 #include <fdtdec.h>
 #include <init.h>
 #include <log.h>
+#include <qfw.h>
 #include <usb.h>
 #include <virtio_types.h>
 #include <virtio.h>
@@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
 struct mm_region *mem_map = qemu_arm64_mem_map;
 #endif
 
+int board_early_init_f(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to bind ramfb
+	 * so video_reserve() can reserve memory for it.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+int board_early_init_r(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to find ramfb
+	 * before console_init.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 int board_init(void)
 {
 	return 0;
-- 
2.40.1

[PATCH v2 6/7] riscv: qemu: Enable ramfb by default

[Alper Nebi Yasak]

From: Alexander Graf <agraf@csgraf.de>

Now that we have everything in place to support ramfb, let's wire it up
by default in the RISC-V QEMU targets. That way, you can easily use a
graphical console by just passing -device ramfb to the QEMU command line.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
[Alper: Rebase on bochs changes, add pre-reloc init and error handling]
Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

Changes in v2:
- Drop imply VIDEO, env changes from RISC-V patch (in u-boot/next)
- Drop imply SYS_CONSOLE_IS_IN_ENV from RISC-V patch (def. y if VIDEO)
- Probe QFW in RISC-V QEMU board_early_init_f to bind ramfb pre-reloc
- Add IS_ENABLED(CONFIG_QFW) check and error handling to RISC-V QEMU

 board/emulation/qemu-riscv/Kconfig      |  5 +++
 board/emulation/qemu-riscv/qemu-riscv.c | 41 +++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/board/emulation/qemu-riscv/Kconfig b/board/emulation/qemu-riscv/Kconfig
index d56b4b5bc1ed..b5c72982e565 100644
--- a/board/emulation/qemu-riscv/Kconfig
+++ b/board/emulation/qemu-riscv/Kconfig
@@ -82,5 +82,10 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	imply USB_XHCI_PCI
 	imply USB_KEYBOARD
 	imply CMD_USB
+	imply BOARD_EARLY_INIT_F
+	imply BOARD_EARLY_INIT_R
+	imply VIDEO_RAMFB
+	imply CMD_QFW
+	imply QFW_MMIO
 
 endif
diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
index 181abbbf97d8..1041e936e9d1 100644
--- a/board/emulation/qemu-riscv/qemu-riscv.c
+++ b/board/emulation/qemu-riscv/qemu-riscv.c
@@ -10,6 +10,7 @@
 #include <fdtdec.h>
 #include <image.h>
 #include <log.h>
+#include <qfw.h>
 #include <spl.h>
 #include <init.h>
 #include <usb.h>
@@ -29,6 +30,46 @@ int is_flash_available(void)
 }
 #endif
 
+int board_early_init_f(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to bind ramfb
+	 * so video_reserve() can reserve memory for it.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+int board_early_init_r(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to find ramfb
+	 * before console_init.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 int board_init(void)
 {
 	/*
-- 
2.40.1

[PATCH v2 7/7] x86: qemu: Enable ramfb by default

[Alper Nebi Yasak]

Now that we have everything in place to support ramfb, let's wire it up
by default in the x86 QEMU targets. That way, we can use ramfb graphical
console instead of the default by passing -vga none -device ramfb to the
QEMU command line.

Also increase SYS_MALLOC_F_LEN for QEMU x86_64 to be the same as its SPL
counterpart, because we're running out of alloc space in pre-reloc stage
with ramfb enabled.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
This also suffers from the same issue with distros as the Bochs display
driver [1], where it results in a hang after GRUB menu selection before
the kernel can display anything. Couldn't reproduce on arm*/riscv*.

But just having it enabled doesn't seem to cause problems unless you run
QEMU with -device ramfb, so this (unlike the Bochs video driver) can
actually be co-enabled with VIDEO_VESA.

[1] https://lore.kernel.org/u-boot/20230724145210.304917-4-sjg@chromium.org/

Changes in v2:
- Add patch "x86: qemu: Enable ramfb by default"

 arch/x86/cpu/qemu/Kconfig           |  4 +++
 board/emulation/qemu-x86/qemu-x86.c | 47 +++++++++++++++++++++++++++++
 configs/qemu-x86_64_defconfig       |  4 +--
 configs/qemu-x86_defconfig          |  1 -
 4 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/arch/x86/cpu/qemu/Kconfig b/arch/x86/cpu/qemu/Kconfig
index f8f2f6473088..e0a57ac2d687 100644
--- a/arch/x86/cpu/qemu/Kconfig
+++ b/arch/x86/cpu/qemu/Kconfig
@@ -13,6 +13,10 @@ config QEMU
 	imply USB
 	imply USB_EHCI_HCD
 	imply VIDEO_VESA
+	imply VIDEO_RAMFB
+	imply BOARD_EARLY_INIT_F
+	imply BOARD_EARLY_INIT_R
+	imply CMD_QFW
 
 if QEMU
 
diff --git a/board/emulation/qemu-x86/qemu-x86.c b/board/emulation/qemu-x86/qemu-x86.c
index e69de29bb2d1..3a8a580cc591 100644
--- a/board/emulation/qemu-x86/qemu-x86.c
+++ b/board/emulation/qemu-x86/qemu-x86.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#include <dm.h>
+#include <init.h>
+#include <log.h>
+#include <qfw.h>
+
+int board_early_init_f(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to bind ramfb
+	 * so video_reserve() can reserve memory for it.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+int board_early_init_r(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/*
+	 * Make sure we enumerate the QEMU Firmware device to find ramfb
+	 * before console_init.
+	 */
+	if (IS_ENABLED(CONFIG_QFW)) {
+		ret = qfw_get_dev(&dev);
+		if (ret) {
+			log_err("Failed to get QEMU FW device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
index c6f30674a8fc..c9ad6eebd35c 100644
--- a/configs/qemu-x86_64_defconfig
+++ b/configs/qemu-x86_64_defconfig
@@ -1,13 +1,12 @@
 CONFIG_X86=y
 CONFIG_TEXT_BASE=0x1110000
-CONFIG_SYS_MALLOC_F_LEN=0x1000
+CONFIG_SYS_MALLOC_F_LEN=0x2000
 CONFIG_NR_DRAM_BANKS=8
 CONFIG_ENV_SIZE=0x40000
 CONFIG_MAX_CPUS=2
 CONFIG_SPL_DM_SPI=y
 CONFIG_DEFAULT_DEVICE_TREE="qemu-x86_i440fx"
 CONFIG_SPL_TEXT_BASE=0xfffd8000
-CONFIG_SPL_SYS_MALLOC_F_LEN=0x2000
 CONFIG_DEBUG_UART_BASE=0x3f8
 CONFIG_DEBUG_UART_CLOCK=1843200
 CONFIG_X86_RUN_64BIT=y
@@ -59,7 +58,6 @@ CONFIG_CMD_USB=y
 CONFIG_BOOTP_BOOTFILESIZE=y
 CONFIG_CMD_EFIDEBUG=y
 CONFIG_CMD_TIME=y
-CONFIG_CMD_QFW=y
 CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_ENV_OVERWRITE=y
diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
index 24682a5387df..8a2311720f02 100644
--- a/configs/qemu-x86_defconfig
+++ b/configs/qemu-x86_defconfig
@@ -37,7 +37,6 @@ CONFIG_CMD_USB=y
 CONFIG_BOOTP_BOOTFILESIZE=y
 CONFIG_CMD_EFIDEBUG=y
 CONFIG_CMD_TIME=y
-CONFIG_CMD_QFW=y
 CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_EXT4_WRITE=y
 CONFIG_MAC_PARTITION=y
-- 
2.40.1

Re: [PATCH v2 2/7] ramfb: Add driver for ramfb display

[Simon Glass]

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> From: Alexander Graf <agraf@csgraf.de>
>
> QEMU implements multiple ways to expose graphics output to the virt
> machine, but most of them are incompatible with hardware virtualization.
>
> The one that does work reliably is ramfb. It's a very simple mechanism
> in which the guest reserves a memory region for the frame buffer and then
> notifies the host about its location and properties. The host then just
> displays the contents of the frame buffer on screen.
>
> This patch implements a trivial version of a ramfb driver - hard coded
> to a single resolution set in Kconfig.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> [Alper: Deduplicate depends on DM_VIDEO, drop MAINTAINERS, decouple from
>         EFI_LOADER, add .bind(), kconfigurable resolution, struct in .h]
> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Remove extra "depends on DM_VIDEO" already in "if VIDEO"
> - Drop drivers/video/MAINTAINERS file
> - Decouple framebuffer allocation from EFI_LOADER
> - Add .bind() method for ramfb driver
> - Make resolution configurable with kconfig
> - Move struct to qfw.h and add comments for members
> - Use RAMFB_* definitions instead of DEFAULT_*
>
>  drivers/video/Kconfig  | 30 +++++++++++++
>  drivers/video/Makefile |  1 +
>  drivers/video/ramfb.c  | 97 ++++++++++++++++++++++++++++++++++++++++++
>  include/qfw.h          | 10 +++++
>  4 files changed, 138 insertions(+)
>  create mode 100644 drivers/video/ramfb.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 3/7] qfw: Spawn ramfb device if its file is present

[Simon Glass]

Hi Alper,

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> From: Alexander Graf <agraf@csgraf.de>
>
> Now that we have a ramfb device driver, let's add the necessary glueing
> magic to also spawn it when we find its qfw file node.

So then how do we select which video driver is used? I think we should
have this in the DT so there is some control.

>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> [Alper: Use if IS_ENABLED() instead of #ifdef]
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Use if (IS_ENABLED(CONFIG_VIDEO_RAMFB)) instead of #ifdef
>
>  drivers/misc/qfw.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
> index 7c01bf23d53b..4e4260982cce 100644
> --- a/drivers/misc/qfw.c
> +++ b/drivers/misc/qfw.c
> @@ -16,6 +16,7 @@
>  #include <malloc.h>
>  #include <qfw.h>
>  #include <dm.h>
> +#include <dm/lists.h>
>  #include <misc.h>
>  #include <tables_csum.h>
>  #include <asm/acpi_table.h>
> @@ -307,6 +308,27 @@ void qfw_read_entry(struct udevice *dev, u16 entry, u32 size, void *address)
>                 qfw_read_entry_io(qdev, entry, size, address);
>  }
>
> +static void qfw_bind_ramfb(struct udevice *dev)
> +{
> +       struct fw_file *file;
> +       int ret;
> +
> +       if (!IS_ENABLED(CONFIG_VIDEO_RAMFB))
> +               return;

Needs an error-code return

> +
> +       ret = qfw_read_firmware_list(dev);
> +       if (ret)
> +               return;
> +
> +       file = qfw_find_file(dev, "etc/ramfb");
> +       if (!file) {
> +               /* No ramfb available. */
> +               return;
> +       }
> +
> +       device_bind_driver(dev, "ramfb", "qfw-ramfb", NULL);

check error

> +}
> +
>  int qfw_register(struct udevice *dev)
>  {
>         struct qfw_dev *qdev = dev_get_uclass_priv(dev);
> @@ -323,6 +345,8 @@ int qfw_register(struct udevice *dev)
>         if (dma_enabled & FW_CFG_DMA_ENABLED)
>                 qdev->dma_present = true;
>
> +       qfw_bind_ramfb(dev);
> +
>         return 0;
>  }
>
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH v2 4/7] qfw: Add flag to allow probing before relocation

[Simon Glass]

Hi Alper,

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> QEMU firmware config drivers need to be probed to bind the ramfb device.
> The ramfb driver needs to be bound before relocation to properly reserve
> video memory for it, otherwise it cannot be probed after relocation. Add
> the flag to probe QEMU firmware config drivers before relocation so that
> ramfb can work as an initial vidconsole.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
> Alternatively, I guess I could default VIDEO_PCI_DEFAULT_FB_SIZE to a
> higher size with "if VIDEO_RAMFB". But it exists because "PCI drivers
> cannot be bound before relocation unless they are mentioned in the
> devicetree" and qfw is in the QEMU-generated devicetree unlike those, so
> I assumed this would be the preferred way.
>
> Changes in v2:
> - Add patch "qfw: Add flag to allow probing before relocation"
>
>  drivers/misc/qfw.c         | 1 +
>  drivers/misc/qfw_mmio.c    | 1 +
>  drivers/misc/qfw_pio.c     | 1 +
>  drivers/misc/qfw_sandbox.c | 1 +
>  4 files changed, 4 insertions(+)
>
> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
> index 4e4260982cce..265f45290011 100644
> --- a/drivers/misc/qfw.c
> +++ b/drivers/misc/qfw.c
> @@ -414,6 +414,7 @@ UCLASS_DRIVER(qfw) = {
>         .name           = "qfw",
>         .post_bind      = qfw_post_bind,
>         .per_device_auto        = sizeof(struct qfw_dev),
> +       .flags = DM_FLAG_PRE_RELOC,
>  };

Should we add this to the DT instead?

In the case where it isn't present it can return -EPERM.

Regards,
Simon

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> From: Alexander Graf <agraf@csgraf.de>
>
> Now that we have everything in place to support ramfb, let's wire it up
> by default in the ARM QEMU targets. That way, you can easily use a
> graphical console by just passing -device ramfb to the QEMU command line.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
> - Drop env changes from ARM (necessary but in prerequisite series)
> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
>
>  arch/arm/Kconfig                    |  3 +++
>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 1fd3ccd1607f..7afe26ac804f 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
>         imply USB_XHCI_PCI
>         imply USB_KEYBOARD
>         imply CMD_USB
> +       imply VIDEO_RAMFB
> +       imply BOARD_EARLY_INIT_F
> +       imply BOARD_EARLY_INIT_R
>
>  config ARCH_RMOBILE
>         bool "Renesas ARM SoCs"
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index 942f1fff5717..23ef31cb7feb 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -11,6 +11,7 @@
>  #include <fdtdec.h>
>  #include <init.h>
>  #include <log.h>
> +#include <qfw.h>
>  #include <usb.h>
>  #include <virtio_types.h>
>  #include <virtio.h>
> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
>  struct mm_region *mem_map = qemu_arm64_mem_map;
>  #endif
>
> +int board_early_init_f(void)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       /*
> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
> +        * so video_reserve() can reserve memory for it.
> +        */
> +       if (IS_ENABLED(CONFIG_QFW)) {
> +               ret = qfw_get_dev(&dev);
> +               if (ret) {
> +                       log_err("Failed to get QEMU FW device: %d\n", ret);

We should only present an error if the device is present but
failed...so if the user doesn't provide the flag, all should be well.

> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +int board_early_init_r(void)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       /*
> +        * Make sure we enumerate the QEMU Firmware device to find ramfb
> +        * before console_init.
> +        */
> +       if (IS_ENABLED(CONFIG_QFW)) {
> +               ret = qfw_get_dev(&dev);
> +               if (ret) {
> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  int board_init(void)
>  {
>         return 0;
> --
> 2.40.1
>

The glue here feels like a bit of a hack...we should rely on normal DT
mechanisms here.

Regards,
Simon

Re: [PATCH v2 6/7] riscv: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> From: Alexander Graf <agraf@csgraf.de>
>
> Now that we have everything in place to support ramfb, let's wire it up
> by default in the RISC-V QEMU targets. That way, you can easily use a
> graphical console by just passing -device ramfb to the QEMU command line.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> [Alper: Rebase on bochs changes, add pre-reloc init and error handling]
> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Drop imply VIDEO, env changes from RISC-V patch (in u-boot/next)
> - Drop imply SYS_CONSOLE_IS_IN_ENV from RISC-V patch (def. y if VIDEO)
> - Probe QFW in RISC-V QEMU board_early_init_f to bind ramfb pre-reloc
> - Add IS_ENABLED(CONFIG_QFW) check and error handling to RISC-V QEMU
>
>  board/emulation/qemu-riscv/Kconfig      |  5 +++
>  board/emulation/qemu-riscv/qemu-riscv.c | 41 +++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)

Again, please investigate DT mechanism which will work for all archs.

Regards,
Simon

Re: [PATCH v2 7/7] x86: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,

On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Now that we have everything in place to support ramfb, let's wire it up
> by default in the x86 QEMU targets. That way, we can use ramfb graphical
> console instead of the default by passing -vga none -device ramfb to the
> QEMU command line.
>
> Also increase SYS_MALLOC_F_LEN for QEMU x86_64 to be the same as its SPL
> counterpart, because we're running out of alloc space in pre-reloc stage
> with ramfb enabled.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
> This also suffers from the same issue with distros as the Bochs display
> driver [1], where it results in a hang after GRUB menu selection before
> the kernel can display anything. Couldn't reproduce on arm*/riscv*.

Yes I see that problem too. I wonder how we can debug it?

>
> But just having it enabled doesn't seem to cause problems unless you run
> QEMU with -device ramfb, so this (unlike the Bochs video driver) can
> actually be co-enabled with VIDEO_VESA.

Indeed...which makes me wonder if we can do something similar with
Bochs, so that (from the cmdline) it is possible to chose ramfb, bochs
or vesa?

>
> [1] https://lore.kernel.org/u-boot/20230724145210.304917-4-sjg@chromium.org/
>
> Changes in v2:
> - Add patch "x86: qemu: Enable ramfb by default"
>
>  arch/x86/cpu/qemu/Kconfig           |  4 +++
>  board/emulation/qemu-x86/qemu-x86.c | 47 +++++++++++++++++++++++++++++
>  configs/qemu-x86_64_defconfig       |  4 +--
>  configs/qemu-x86_defconfig          |  1 -
>  4 files changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/cpu/qemu/Kconfig b/arch/x86/cpu/qemu/Kconfig
> index f8f2f6473088..e0a57ac2d687 100644
> --- a/arch/x86/cpu/qemu/Kconfig
> +++ b/arch/x86/cpu/qemu/Kconfig
> @@ -13,6 +13,10 @@ config QEMU
>         imply USB
>         imply USB_EHCI_HCD
>         imply VIDEO_VESA
> +       imply VIDEO_RAMFB
> +       imply BOARD_EARLY_INIT_F
> +       imply BOARD_EARLY_INIT_R
> +       imply CMD_QFW
>
>  if QEMU
>
> diff --git a/board/emulation/qemu-x86/qemu-x86.c b/board/emulation/qemu-x86/qemu-x86.c
> index e69de29bb2d1..3a8a580cc591 100644
> --- a/board/emulation/qemu-x86/qemu-x86.c
> +++ b/board/emulation/qemu-x86/qemu-x86.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <init.h>
> +#include <log.h>
> +#include <qfw.h>
> +
> +int board_early_init_f(void)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       /*
> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
> +        * so video_reserve() can reserve memory for it.
> +        */
> +       if (IS_ENABLED(CONFIG_QFW)) {
> +               ret = qfw_get_dev(&dev);
> +               if (ret) {
> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;

Same point about DT here.

> +}
> +
> +int board_early_init_r(void)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       /*
> +        * Make sure we enumerate the QEMU Firmware device to find ramfb
> +        * before console_init.
> +        */
> +       if (IS_ENABLED(CONFIG_QFW)) {
> +               ret = qfw_get_dev(&dev);
> +               if (ret) {
> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> diff --git a/configs/qemu-x86_64_defconfig b/configs/qemu-x86_64_defconfig
> index c6f30674a8fc..c9ad6eebd35c 100644
> --- a/configs/qemu-x86_64_defconfig
> +++ b/configs/qemu-x86_64_defconfig
> @@ -1,13 +1,12 @@
>  CONFIG_X86=y
>  CONFIG_TEXT_BASE=0x1110000
> -CONFIG_SYS_MALLOC_F_LEN=0x1000
> +CONFIG_SYS_MALLOC_F_LEN=0x2000
>  CONFIG_NR_DRAM_BANKS=8
>  CONFIG_ENV_SIZE=0x40000
>  CONFIG_MAX_CPUS=2
>  CONFIG_SPL_DM_SPI=y
>  CONFIG_DEFAULT_DEVICE_TREE="qemu-x86_i440fx"
>  CONFIG_SPL_TEXT_BASE=0xfffd8000
> -CONFIG_SPL_SYS_MALLOC_F_LEN=0x2000
>  CONFIG_DEBUG_UART_BASE=0x3f8
>  CONFIG_DEBUG_UART_CLOCK=1843200
>  CONFIG_X86_RUN_64BIT=y
> @@ -59,7 +58,6 @@ CONFIG_CMD_USB=y
>  CONFIG_BOOTP_BOOTFILESIZE=y
>  CONFIG_CMD_EFIDEBUG=y
>  CONFIG_CMD_TIME=y
> -CONFIG_CMD_QFW=y

What is happening here? Why disable it?

>  CONFIG_CMD_BOOTSTAGE=y
>  CONFIG_CMD_EXT4_WRITE=y
>  CONFIG_ENV_OVERWRITE=y
> diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig
> index 24682a5387df..8a2311720f02 100644
> --- a/configs/qemu-x86_defconfig
> +++ b/configs/qemu-x86_defconfig
> @@ -37,7 +37,6 @@ CONFIG_CMD_USB=y
>  CONFIG_BOOTP_BOOTFILESIZE=y
>  CONFIG_CMD_EFIDEBUG=y
>  CONFIG_CMD_TIME=y
> -CONFIG_CMD_QFW=y
>  CONFIG_CMD_BOOTSTAGE=y
>  CONFIG_CMD_EXT4_WRITE=y
>  CONFIG_MAC_PARTITION=y
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH v2 3/7] qfw: Spawn ramfb device if its file is present

[Alper Nebi Yasak]

On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> From: Alexander Graf <agraf@csgraf.de>
>>
>> Now that we have a ramfb device driver, let's add the necessary glueing
>> magic to also spawn it when we find its qfw file node.
> 
> So then how do we select which video driver is used? I think we should
> have this in the DT so there is some control.

I think you mean choosing between multiple potential video devices for a
video console, but just in case: AFAIK only the ramfb driver can be used
with -device ramfb, unlike how either vesa or bochs can be used for
-device VGA.

This series is less about being able to use multiple video devices, but
more about supporting by default different configurations in which QEMU
can be launched: with "-device VGA" or "-vga none -device ramfb".

That being said, I had some success with multiple video console
attempts. For "-device VGA -device ramfb", we need to make `*addrp -=
CONFIG_VAL(VIDEO_PCI_DEFAULT_FB_SIZE);` unconditional in video_reserve()
 to correctly allocate for both vesa and ramfb. Then we can run `env set
serial,vidconsole,vidconsole1` which sets up both video/vidconsole
devices with same text content.

Regardless of this series, with VIDEO_PCI_DEFAULT_FB_SIZE=0x3000000 and
"-device VGA -device VGA -device VGA" we get two broken and one empty
display, and `env set serial,vidconsole,vidconsole1,vidconsole2` puts a
console to the third display. I think it's a bug the first two are
broken, and it's initially using the first broken display as vidconsole.

Anyway, my opinion is: if stdout/stderr has "vidconsole", we should
enable a vidconsole on all consoles, but if it has "vidconsole#" we
should only enable the one on that video# device. Then device-tree could
have /aliases { video0 = "..."; video1 = &...; } or something to have a
stable order?

Or, you might mean adding a ramfb node to the device-tree, in which case
that's going to reopen a can of OF_BOARD worms... (Same as what applies
to putting bootph,* into qfw device-tree node, so I'll reply there.)

>> [...]
>> 
>> +static void qfw_bind_ramfb(struct udevice *dev)
>> +{
>> +       struct fw_file *file;
>> +       int ret;
>> +
>> +       if (!IS_ENABLED(CONFIG_VIDEO_RAMFB))
>> +               return;
> 
> Needs an error-code return
> 
>> +
>> +       ret = qfw_read_firmware_list(dev);
>> +       if (ret)
>> +               return;
>> +
>> +       file = qfw_find_file(dev, "etc/ramfb");
>> +       if (!file) {
>> +               /* No ramfb available. */
>> +               return;
>> +       }
>> +
>> +       device_bind_driver(dev, "ramfb", "qfw-ramfb", NULL);
> 
> check error
> 

Will try to fix both, thanks.

Re: [PATCH v2 4/7] qfw: Add flag to allow probing before relocation

[Alper Nebi Yasak]

On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> QEMU firmware config drivers need to be probed to bind the ramfb device.
>> The ramfb driver needs to be bound before relocation to properly reserve
>> video memory for it, otherwise it cannot be probed after relocation. Add
>> the flag to probe QEMU firmware config drivers before relocation so that
>> ramfb can work as an initial vidconsole.
>>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> ---
>> Alternatively, I guess I could default VIDEO_PCI_DEFAULT_FB_SIZE to a
>> higher size with "if VIDEO_RAMFB". But it exists because "PCI drivers
>> cannot be bound before relocation unless they are mentioned in the
>> devicetree" and qfw is in the QEMU-generated devicetree unlike those, so
>> I assumed this would be the preferred way.
>>
>> Changes in v2:
>> - Add patch "qfw: Add flag to allow probing before relocation"
>>
>>  drivers/misc/qfw.c         | 1 +
>>  drivers/misc/qfw_mmio.c    | 1 +
>>  drivers/misc/qfw_pio.c     | 1 +
>>  drivers/misc/qfw_sandbox.c | 1 +
>>  4 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
>> index 4e4260982cce..265f45290011 100644
>> --- a/drivers/misc/qfw.c
>> +++ b/drivers/misc/qfw.c
>> @@ -414,6 +414,7 @@ UCLASS_DRIVER(qfw) = {
>>         .name           = "qfw",
>>         .post_bind      = qfw_post_bind,
>>         .per_device_auto        = sizeof(struct qfw_dev),
>> +       .flags = DM_FLAG_PRE_RELOC,
>>  };
> 
> Should we add this to the DT instead?

I've experimented a bit, and got something that works nice on x86
because we have the device-trees in-tree. So I can add something to
arch/x86/dts/qemu-*.dts like:

    / {
        fw-cfg {
            compatible = "qemu,fw-cfg-pio";
            bootph-some-ram;

            qfw-ramfb {
                compatible = "qemu,qfw-ramfb";
                bootph-some-ram;
            };
        };
    };

Then add the compatibles as .of_match to qfw_pio and ramfb drivers, call
dm_scan_fdt_dev(dev) in qfw_post_bind(), merge qfw_bind_ramfb() into the
ramfb driver probe, and it will work without the board_early_init_[fr]
hooks or these flags. That all seems to be fine.

The problem is other arches where QEMU generates a device-tree at
runtime and passes it to U-Boot with OF_BOARD. It has, on arm64 (and
with @10100000 instead on riscv64):

    fw-cfg@9020000 {
        dma-coherent;
        reg = <0x00 0x9020000 0x00 0x18>;
        compatible = "qemu,fw-cfg-mmio";
    };

As a proof of concept, I used `qemu-system-* -M dumpdtb=file.dtb` and
`dtc -I dtb -O dts` to replace the empty qemu dts files, then set
OF_BOARD=n and OF_OMIT_DTB=n, and added similar modifications to
qemu-*-u-boot.dtsi. And everything seems fine like that as well.

I think it can be automated in Makefile, but I don't think disabling
OF_BOARD is right here. I see there's OF_BOARD_FIXUP, so I guess I'll
try finding the fw-cfg node there and adding the bootph props and ramfb
nodes... Is this going the right way?

(And the pio/ramfb compatibles don't exist, do we have to use u-boot,*?)

> In the case where it isn't present it can return -EPERM.
> 
> Regards,
> Simon

Re: [PATCH v2 4/7] qfw: Add flag to allow probing before relocation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3613 bytes --]

On Mon, Aug 28, 2023 at 06:22:31PM +0300, Alper Nebi Yasak wrote:
> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> > Hi Alper,
> > 
> > On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> QEMU firmware config drivers need to be probed to bind the ramfb device.
> >> The ramfb driver needs to be bound before relocation to properly reserve
> >> video memory for it, otherwise it cannot be probed after relocation. Add
> >> the flag to probe QEMU firmware config drivers before relocation so that
> >> ramfb can work as an initial vidconsole.
> >>
> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >> ---
> >> Alternatively, I guess I could default VIDEO_PCI_DEFAULT_FB_SIZE to a
> >> higher size with "if VIDEO_RAMFB". But it exists because "PCI drivers
> >> cannot be bound before relocation unless they are mentioned in the
> >> devicetree" and qfw is in the QEMU-generated devicetree unlike those, so
> >> I assumed this would be the preferred way.
> >>
> >> Changes in v2:
> >> - Add patch "qfw: Add flag to allow probing before relocation"
> >>
> >>  drivers/misc/qfw.c         | 1 +
> >>  drivers/misc/qfw_mmio.c    | 1 +
> >>  drivers/misc/qfw_pio.c     | 1 +
> >>  drivers/misc/qfw_sandbox.c | 1 +
> >>  4 files changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
> >> index 4e4260982cce..265f45290011 100644
> >> --- a/drivers/misc/qfw.c
> >> +++ b/drivers/misc/qfw.c
> >> @@ -414,6 +414,7 @@ UCLASS_DRIVER(qfw) = {
> >>         .name           = "qfw",
> >>         .post_bind      = qfw_post_bind,
> >>         .per_device_auto        = sizeof(struct qfw_dev),
> >> +       .flags = DM_FLAG_PRE_RELOC,
> >>  };
> > 
> > Should we add this to the DT instead?
> 
> I've experimented a bit, and got something that works nice on x86
> because we have the device-trees in-tree. So I can add something to
> arch/x86/dts/qemu-*.dts like:
> 
>     / {
>         fw-cfg {
>             compatible = "qemu,fw-cfg-pio";
>             bootph-some-ram;
> 
>             qfw-ramfb {
>                 compatible = "qemu,qfw-ramfb";
>                 bootph-some-ram;
>             };
>         };
>     };
> 
> Then add the compatibles as .of_match to qfw_pio and ramfb drivers, call
> dm_scan_fdt_dev(dev) in qfw_post_bind(), merge qfw_bind_ramfb() into the
> ramfb driver probe, and it will work without the board_early_init_[fr]
> hooks or these flags. That all seems to be fine.
> 
> The problem is other arches where QEMU generates a device-tree at
> runtime and passes it to U-Boot with OF_BOARD. It has, on arm64 (and
> with @10100000 instead on riscv64):
> 
>     fw-cfg@9020000 {
>         dma-coherent;
>         reg = <0x00 0x9020000 0x00 0x18>;
>         compatible = "qemu,fw-cfg-mmio";
>     };
> 
> As a proof of concept, I used `qemu-system-* -M dumpdtb=file.dtb` and
> `dtc -I dtb -O dts` to replace the empty qemu dts files, then set
> OF_BOARD=n and OF_OMIT_DTB=n, and added similar modifications to
> qemu-*-u-boot.dtsi. And everything seems fine like that as well.
> 
> I think it can be automated in Makefile, but I don't think disabling
> OF_BOARD is right here. I see there's OF_BOARD_FIXUP, so I guess I'll
> try finding the fw-cfg node there and adding the bootph props and ramfb
> nodes... Is this going the right way?
> 
> (And the pio/ramfb compatibles don't exist, do we have to use u-boot,*?)

This seems like a whole lot of work to avoid using the flags mechanism
that we have, intentionally.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 7/7] x86: qemu: Enable ramfb by default

[Alper Nebi Yasak]

On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> Now that we have everything in place to support ramfb, let's wire it up
>> by default in the x86 QEMU targets. That way, we can use ramfb graphical
>> console instead of the default by passing -vga none -device ramfb to the
>> QEMU command line.
>>
>> Also increase SYS_MALLOC_F_LEN for QEMU x86_64 to be the same as its SPL
>> counterpart, because we're running out of alloc space in pre-reloc stage
>> with ramfb enabled.
>>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> ---
>> This also suffers from the same issue with distros as the Bochs display
>> driver [1], where it results in a hang after GRUB menu selection before
>> the kernel can display anything. Couldn't reproduce on arm*/riscv*.
> 
> Yes I see that problem too. I wonder how we can debug it?

No idea, and I couldn't find a good commit to bisect from, tried as far
back as v2021.10.

>> But just having it enabled doesn't seem to cause problems unless you run
>> QEMU with -device ramfb, so this (unlike the Bochs video driver) can
>> actually be co-enabled with VIDEO_VESA.
> 
> Indeed...which makes me wonder if we can do something similar with
> Bochs, so that (from the cmdline) it is possible to chose ramfb, bochs
> or vesa?

(Tried to answer video choice and DT concerns in another mail)

>> [1] https://lore.kernel.org/u-boot/20230724145210.304917-4-sjg@chromium.org/
>>
>> Changes in v2:
>> - Add patch "x86: qemu: Enable ramfb by default"
>>
>>  arch/x86/cpu/qemu/Kconfig           |  4 +++
>>  board/emulation/qemu-x86/qemu-x86.c | 47 +++++++++++++++++++++++++++++
>>  configs/qemu-x86_64_defconfig       |  4 +--
>>  configs/qemu-x86_defconfig          |  1 -
>>  4 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/cpu/qemu/Kconfig b/arch/x86/cpu/qemu/Kconfig
>> index f8f2f6473088..e0a57ac2d687 100644
>> --- a/arch/x86/cpu/qemu/Kconfig
>> +++ b/arch/x86/cpu/qemu/Kconfig
>> @@ -13,6 +13,10 @@ config QEMU
>>         imply USB
>>         imply USB_EHCI_HCD
>>         imply VIDEO_VESA
>> +       imply VIDEO_RAMFB
>> +       imply BOARD_EARLY_INIT_F
>> +       imply BOARD_EARLY_INIT_R
>> +       imply CMD_QFW
>>
>>  if QEMU
>>
>> [...]
>> 
>> @@ -59,7 +58,6 @@ CONFIG_CMD_USB=y
>>  CONFIG_BOOTP_BOOTFILESIZE=y
>>  CONFIG_CMD_EFIDEBUG=y
>>  CONFIG_CMD_TIME=y
>> -CONFIG_CMD_QFW=y
> 
> What is happening here? Why disable it?

I used `imply CMD_QFW` above to be consistent with previous patches, so
it's no longer necessary in defconfigs. Should I drop the imply and keep
these in defconfig?

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Alper Nebi Yasak]

On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> From: Alexander Graf <agraf@csgraf.de>
>>
>> Now that we have everything in place to support ramfb, let's wire it up
>> by default in the ARM QEMU targets. That way, you can easily use a
>> graphical console by just passing -device ramfb to the QEMU command line.
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> ---
>>
>> Changes in v2:
>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
>> - Drop env changes from ARM (necessary but in prerequisite series)
>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
>>
>>  arch/arm/Kconfig                    |  3 +++
>>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 1fd3ccd1607f..7afe26ac804f 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
>>         imply USB_XHCI_PCI
>>         imply USB_KEYBOARD
>>         imply CMD_USB
>> +       imply VIDEO_RAMFB
>> +       imply BOARD_EARLY_INIT_F
>> +       imply BOARD_EARLY_INIT_R
>>
>>  config ARCH_RMOBILE
>>         bool "Renesas ARM SoCs"
>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
>> index 942f1fff5717..23ef31cb7feb 100644
>> --- a/board/emulation/qemu-arm/qemu-arm.c
>> +++ b/board/emulation/qemu-arm/qemu-arm.c
>> @@ -11,6 +11,7 @@
>>  #include <fdtdec.h>
>>  #include <init.h>
>>  #include <log.h>
>> +#include <qfw.h>
>>  #include <usb.h>
>>  #include <virtio_types.h>
>>  #include <virtio.h>
>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
>>  struct mm_region *mem_map = qemu_arm64_mem_map;
>>  #endif
>>
>> +int board_early_init_f(void)
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       /*
>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
>> +        * so video_reserve() can reserve memory for it.
>> +        */
>> +       if (IS_ENABLED(CONFIG_QFW)) {
>> +               ret = qfw_get_dev(&dev);
>> +               if (ret) {
>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> 
> We should only present an error if the device is present but
> failed...so if the user doesn't provide the flag, all should be well.

I don't understand what you mean by "user doesn't provide the flag". But
I assume this should ignore the error (unless what?) so that we can
continue to boot. Would that apply for board_early_init_r below as well?

>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +int board_early_init_r(void)
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       /*
>> +        * Make sure we enumerate the QEMU Firmware device to find ramfb
>> +        * before console_init.
>> +        */
>> +       if (IS_ENABLED(CONFIG_QFW)) {
>> +               ret = qfw_get_dev(&dev);
>> +               if (ret) {
>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  int board_init(void)
>>  {
>>         return 0;
>> --
>> 2.40.1
>>
> 
> The glue here feels like a bit of a hack...we should rely on normal DT
> mechanisms here.
> 
> Regards,
> Simon

Re: [PATCH v2 7/7] x86: qemu: Enable ramfb by default

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]

On Mon, Aug 28, 2023 at 06:40:10PM +0300, Alper Nebi Yasak wrote:
> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> > Hi Alper,
> > 
> > On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> Now that we have everything in place to support ramfb, let's wire it up
> >> by default in the x86 QEMU targets. That way, we can use ramfb graphical
> >> console instead of the default by passing -vga none -device ramfb to the
> >> QEMU command line.
> >>
> >> Also increase SYS_MALLOC_F_LEN for QEMU x86_64 to be the same as its SPL
> >> counterpart, because we're running out of alloc space in pre-reloc stage
> >> with ramfb enabled.
> >>
> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >> ---
> >> This also suffers from the same issue with distros as the Bochs display
> >> driver [1], where it results in a hang after GRUB menu selection before
> >> the kernel can display anything. Couldn't reproduce on arm*/riscv*.
> > 
> > Yes I see that problem too. I wonder how we can debug it?
> 
> No idea, and I couldn't find a good commit to bisect from, tried as far
> back as v2021.10.
> 
> >> But just having it enabled doesn't seem to cause problems unless you run
> >> QEMU with -device ramfb, so this (unlike the Bochs video driver) can
> >> actually be co-enabled with VIDEO_VESA.
> > 
> > Indeed...which makes me wonder if we can do something similar with
> > Bochs, so that (from the cmdline) it is possible to chose ramfb, bochs
> > or vesa?
> 
> (Tried to answer video choice and DT concerns in another mail)
> 
> >> [1] https://lore.kernel.org/u-boot/20230724145210.304917-4-sjg@chromium.org/
> >>
> >> Changes in v2:
> >> - Add patch "x86: qemu: Enable ramfb by default"
> >>
> >>  arch/x86/cpu/qemu/Kconfig           |  4 +++
> >>  board/emulation/qemu-x86/qemu-x86.c | 47 +++++++++++++++++++++++++++++
> >>  configs/qemu-x86_64_defconfig       |  4 +--
> >>  configs/qemu-x86_defconfig          |  1 -
> >>  4 files changed, 52 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/cpu/qemu/Kconfig b/arch/x86/cpu/qemu/Kconfig
> >> index f8f2f6473088..e0a57ac2d687 100644
> >> --- a/arch/x86/cpu/qemu/Kconfig
> >> +++ b/arch/x86/cpu/qemu/Kconfig
> >> @@ -13,6 +13,10 @@ config QEMU
> >>         imply USB
> >>         imply USB_EHCI_HCD
> >>         imply VIDEO_VESA
> >> +       imply VIDEO_RAMFB
> >> +       imply BOARD_EARLY_INIT_F
> >> +       imply BOARD_EARLY_INIT_R
> >> +       imply CMD_QFW
> >>
> >>  if QEMU
> >>
> >> [...]
> >> 
> >> @@ -59,7 +58,6 @@ CONFIG_CMD_USB=y
> >>  CONFIG_BOOTP_BOOTFILESIZE=y
> >>  CONFIG_CMD_EFIDEBUG=y
> >>  CONFIG_CMD_TIME=y
> >> -CONFIG_CMD_QFW=y
> > 
> > What is happening here? Why disable it?
> 
> I used `imply CMD_QFW` above to be consistent with previous patches, so
> it's no longer necessary in defconfigs. Should I drop the imply and keep
> these in defconfig?

I think our Kconfig logic around the QFW options is a bit backwards,
given that it's universal to the "qemu" boards and not used on
qemu-emulates-known-hardware platforms.  The qemu boards should be
select'ing QFW, and CMD_QFW should be depends on QFW and default y,
rather than selects.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,
On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> > Hi Alper,
> >
> > On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> From: Alexander Graf <agraf@csgraf.de>
> >>
> >> Now that we have everything in place to support ramfb, let's wire it up
> >> by default in the ARM QEMU targets. That way, you can easily use a
> >> graphical console by just passing -device ramfb to the QEMU command line.
> >>
> >> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> >> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
> >> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >> ---
> >>
> >> Changes in v2:
> >> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
> >> - Drop env changes from ARM (necessary but in prerequisite series)
> >> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
> >> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
> >> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
> >>
> >>  arch/arm/Kconfig                    |  3 +++
> >>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
> >>  2 files changed, 44 insertions(+)
> >>
> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> index 1fd3ccd1607f..7afe26ac804f 100644
> >> --- a/arch/arm/Kconfig
> >> +++ b/arch/arm/Kconfig
> >> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
> >>         imply USB_XHCI_PCI
> >>         imply USB_KEYBOARD
> >>         imply CMD_USB
> >> +       imply VIDEO_RAMFB
> >> +       imply BOARD_EARLY_INIT_F
> >> +       imply BOARD_EARLY_INIT_R
> >>
> >>  config ARCH_RMOBILE
> >>         bool "Renesas ARM SoCs"
> >> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> >> index 942f1fff5717..23ef31cb7feb 100644
> >> --- a/board/emulation/qemu-arm/qemu-arm.c
> >> +++ b/board/emulation/qemu-arm/qemu-arm.c
> >> @@ -11,6 +11,7 @@
> >>  #include <fdtdec.h>
> >>  #include <init.h>
> >>  #include <log.h>
> >> +#include <qfw.h>
> >>  #include <usb.h>
> >>  #include <virtio_types.h>
> >>  #include <virtio.h>
> >> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
> >>  struct mm_region *mem_map = qemu_arm64_mem_map;
> >>  #endif
> >>
> >> +int board_early_init_f(void)
> >> +{
> >> +       struct udevice *dev;
> >> +       int ret;
> >> +
> >> +       /*
> >> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
> >> +        * so video_reserve() can reserve memory for it.
> >> +        */
> >> +       if (IS_ENABLED(CONFIG_QFW)) {
> >> +               ret = qfw_get_dev(&dev);
> >> +               if (ret) {
> >> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> >
> > We should only present an error if the device is present but
> > failed...so if the user doesn't provide the flag, all should be well.
>
> I don't understand what you mean by "user doesn't provide the flag". But
> I assume this should ignore the error (unless what?) so that we can
> continue to boot. Would that apply for board_early_init_r below as well?

I thought you said there was a qemu flag to enable ramfb? So add it to
the DT and then the device can (in its bind routine) decide not to be
bound by returning -ENODEV

>
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +int board_early_init_r(void)
> >> +{
> >> +       struct udevice *dev;
> >> +       int ret;
> >> +
> >> +       /*
> >> +        * Make sure we enumerate the QEMU Firmware device to find ramfb
> >> +        * before console_init.
> >> +        */
> >> +       if (IS_ENABLED(CONFIG_QFW)) {
> >> +               ret = qfw_get_dev(&dev);
> >> +               if (ret) {
> >> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>  int board_init(void)
> >>  {
> >>         return 0;
> >> --
> >> 2.40.1
> >>
> >
> > The glue here feels like a bit of a hack...we should rely on normal DT
> > mechanisms here.
> >
> > Regards,
> > Simon

Regards.

Simon

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Alper Nebi Yasak]

On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> Hi Alper,
> On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
>>> Hi Alper,
>>>
>>> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>
>>>> From: Alexander Graf <agraf@csgraf.de>
>>>>
>>>> Now that we have everything in place to support ramfb, let's wire it up
>>>> by default in the ARM QEMU targets. That way, you can easily use a
>>>> graphical console by just passing -device ramfb to the QEMU command line.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
>>>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
>>>> - Drop env changes from ARM (necessary but in prerequisite series)
>>>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
>>>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
>>>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
>>>>
>>>>  arch/arm/Kconfig                    |  3 +++
>>>>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
>>>>  2 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>>> index 1fd3ccd1607f..7afe26ac804f 100644
>>>> --- a/arch/arm/Kconfig
>>>> +++ b/arch/arm/Kconfig
>>>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
>>>>         imply USB_XHCI_PCI
>>>>         imply USB_KEYBOARD
>>>>         imply CMD_USB
>>>> +       imply VIDEO_RAMFB
>>>> +       imply BOARD_EARLY_INIT_F
>>>> +       imply BOARD_EARLY_INIT_R
>>>>
>>>>  config ARCH_RMOBILE
>>>>         bool "Renesas ARM SoCs"
>>>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
>>>> index 942f1fff5717..23ef31cb7feb 100644
>>>> --- a/board/emulation/qemu-arm/qemu-arm.c
>>>> +++ b/board/emulation/qemu-arm/qemu-arm.c
>>>> @@ -11,6 +11,7 @@
>>>>  #include <fdtdec.h>
>>>>  #include <init.h>
>>>>  #include <log.h>
>>>> +#include <qfw.h>
>>>>  #include <usb.h>
>>>>  #include <virtio_types.h>
>>>>  #include <virtio.h>
>>>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
>>>>  struct mm_region *mem_map = qemu_arm64_mem_map;
>>>>  #endif
>>>>
>>>> +int board_early_init_f(void)
>>>> +{
>>>> +       struct udevice *dev;
>>>> +       int ret;
>>>> +
>>>> +       /*
>>>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
>>>> +        * so video_reserve() can reserve memory for it.
>>>> +        */
>>>> +       if (IS_ENABLED(CONFIG_QFW)) {
>>>> +               ret = qfw_get_dev(&dev);
>>>> +               if (ret) {
>>>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
>>>
>>> We should only present an error if the device is present but
>>> failed...so if the user doesn't provide the flag, all should be well.
>>
>> I don't understand what you mean by "user doesn't provide the flag". But
>> I assume this should ignore the error (unless what?) so that we can
>> continue to boot. Would that apply for board_early_init_r below as well?
> 
> I thought you said there was a qemu flag to enable ramfb? So add it to
> the DT and then the device can (in its bind routine) decide not to be
> bound by returning -ENODEV

Ah. I'm not used to calling them flags, especially those that have
values as in "-device ramfb". My mind jumped to DM_FLAG_PRE_RELOC /
bootph-some-ram.

But note that what this code tries to probe here is not the ramfb
device, it's the bus/parent of that. If this fails, we can't know if the
user specified "-device ramfb" or not.

Is the parent node's device probed by the time .bind() is called? Or,
can I have it probed by calling qfw_get_dev() (which is just
uclass_first_device_err(UCLASS_QFW, ...))?

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Alexander Graf]

On 28.08.23 20:28, Alper Nebi Yasak wrote:
> On 2023-08-28 20:54 +03:00, Simon Glass wrote:
>> Hi Alper,
>> On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
>>>> Hi Alper,
>>>>
>>>> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>> From: Alexander Graf <agraf@csgraf.de>
>>>>>
>>>>> Now that we have everything in place to support ramfb, let's wire it up
>>>>> by default in the ARM QEMU targets. That way, you can easily use a
>>>>> graphical console by just passing -device ramfb to the QEMU command line.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>>>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
>>>>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
>>>>> - Drop env changes from ARM (necessary but in prerequisite series)
>>>>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
>>>>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
>>>>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
>>>>>
>>>>>   arch/arm/Kconfig                    |  3 +++
>>>>>   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
>>>>>   2 files changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>>>> index 1fd3ccd1607f..7afe26ac804f 100644
>>>>> --- a/arch/arm/Kconfig
>>>>> +++ b/arch/arm/Kconfig
>>>>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
>>>>>          imply USB_XHCI_PCI
>>>>>          imply USB_KEYBOARD
>>>>>          imply CMD_USB
>>>>> +       imply VIDEO_RAMFB
>>>>> +       imply BOARD_EARLY_INIT_F
>>>>> +       imply BOARD_EARLY_INIT_R
>>>>>
>>>>>   config ARCH_RMOBILE
>>>>>          bool "Renesas ARM SoCs"
>>>>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
>>>>> index 942f1fff5717..23ef31cb7feb 100644
>>>>> --- a/board/emulation/qemu-arm/qemu-arm.c
>>>>> +++ b/board/emulation/qemu-arm/qemu-arm.c
>>>>> @@ -11,6 +11,7 @@
>>>>>   #include <fdtdec.h>
>>>>>   #include <init.h>
>>>>>   #include <log.h>
>>>>> +#include <qfw.h>
>>>>>   #include <usb.h>
>>>>>   #include <virtio_types.h>
>>>>>   #include <virtio.h>
>>>>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
>>>>>   struct mm_region *mem_map = qemu_arm64_mem_map;
>>>>>   #endif
>>>>>
>>>>> +int board_early_init_f(void)
>>>>> +{
>>>>> +       struct udevice *dev;
>>>>> +       int ret;
>>>>> +
>>>>> +       /*
>>>>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
>>>>> +        * so video_reserve() can reserve memory for it.
>>>>> +        */
>>>>> +       if (IS_ENABLED(CONFIG_QFW)) {
>>>>> +               ret = qfw_get_dev(&dev);
>>>>> +               if (ret) {
>>>>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
>>>> We should only present an error if the device is present but
>>>> failed...so if the user doesn't provide the flag, all should be well.
>>> I don't understand what you mean by "user doesn't provide the flag". But
>>> I assume this should ignore the error (unless what?) so that we can
>>> continue to boot. Would that apply for board_early_init_r below as well?
>> I thought you said there was a qemu flag to enable ramfb? So add it to
>> the DT and then the device can (in its bind routine) decide not to be
>> bound by returning -ENODEV
> Ah. I'm not used to calling them flags, especially those that have
> values as in "-device ramfb". My mind jumped to DM_FLAG_PRE_RELOC /
> bootph-some-ram.
>
> But note that what this code tries to probe here is not the ramfb
> device, it's the bus/parent of that. If this fails, we can't know if the
> user specified "-device ramfb" or not.


You bring up a great point here. I don't think we should model the 
actual ramfb device as device tree (platform) device. It's 100% 
enumerable from QFW which means we should treat it as a dynamic entity 
that gets created by QFW instead of from device tree, just like USB or 
PCI devices.

And when you do that, everything should fall into place nicely: You 
don't need to dynamically modify device trees on boot, the DT you get 
from QEMU is sufficient to enumerate everything, etc etc.


Alex

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,

On Mon, 28 Aug 2023 at 12:29, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > Hi Alper,
> > On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> >>> Hi Alper,
> >>>
> >>> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>>>
> >>>> From: Alexander Graf <agraf@csgraf.de>
> >>>>
> >>>> Now that we have everything in place to support ramfb, let's wire it up
> >>>> by default in the ARM QEMU targets. That way, you can easily use a
> >>>> graphical console by just passing -device ramfb to the QEMU command line.
> >>>>
> >>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> >>>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
> >>>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
> >>>> - Drop env changes from ARM (necessary but in prerequisite series)
> >>>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
> >>>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
> >>>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
> >>>>
> >>>>  arch/arm/Kconfig                    |  3 +++
> >>>>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
> >>>>  2 files changed, 44 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >>>> index 1fd3ccd1607f..7afe26ac804f 100644
> >>>> --- a/arch/arm/Kconfig
> >>>> +++ b/arch/arm/Kconfig
> >>>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
> >>>>         imply USB_XHCI_PCI
> >>>>         imply USB_KEYBOARD
> >>>>         imply CMD_USB
> >>>> +       imply VIDEO_RAMFB
> >>>> +       imply BOARD_EARLY_INIT_F
> >>>> +       imply BOARD_EARLY_INIT_R
> >>>>
> >>>>  config ARCH_RMOBILE
> >>>>         bool "Renesas ARM SoCs"
> >>>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> >>>> index 942f1fff5717..23ef31cb7feb 100644
> >>>> --- a/board/emulation/qemu-arm/qemu-arm.c
> >>>> +++ b/board/emulation/qemu-arm/qemu-arm.c
> >>>> @@ -11,6 +11,7 @@
> >>>>  #include <fdtdec.h>
> >>>>  #include <init.h>
> >>>>  #include <log.h>
> >>>> +#include <qfw.h>
> >>>>  #include <usb.h>
> >>>>  #include <virtio_types.h>
> >>>>  #include <virtio.h>
> >>>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
> >>>>  struct mm_region *mem_map = qemu_arm64_mem_map;
> >>>>  #endif
> >>>>
> >>>> +int board_early_init_f(void)
> >>>> +{
> >>>> +       struct udevice *dev;
> >>>> +       int ret;
> >>>> +
> >>>> +       /*
> >>>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
> >>>> +        * so video_reserve() can reserve memory for it.
> >>>> +        */
> >>>> +       if (IS_ENABLED(CONFIG_QFW)) {
> >>>> +               ret = qfw_get_dev(&dev);
> >>>> +               if (ret) {
> >>>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> >>>
> >>> We should only present an error if the device is present but
> >>> failed...so if the user doesn't provide the flag, all should be well.
> >>
> >> I don't understand what you mean by "user doesn't provide the flag". But
> >> I assume this should ignore the error (unless what?) so that we can
> >> continue to boot. Would that apply for board_early_init_r below as well?
> >
> > I thought you said there was a qemu flag to enable ramfb? So add it to
> > the DT and then the device can (in its bind routine) decide not to be
> > bound by returning -ENODEV
>
> Ah. I'm not used to calling them flags, especially those that have
> values as in "-device ramfb". My mind jumped to DM_FLAG_PRE_RELOC /
> bootph-some-ram.
>
> But note that what this code tries to probe here is not the ramfb
> device, it's the bus/parent of that. If this fails, we can't know if the
> user specified "-device ramfb" or not.
>
> Is the parent node's device probed by the time .bind() is called? Or,
> can I have it probed by calling qfw_get_dev() (which is just
> uclass_first_device_err(UCLASS_QFW, ...))?

I suppose your problem might be that there are up to three possible
video devices (vesa, bochs, ramfb) and one of them needs to return 0
from bind() and the others need to return -ENODEV?

Perhaps another way would be to update the logic in
stdio_init_tables() to find them all (which I believe it already
does?) and then select one?

Regards,
Simon

Re: [PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Alexander Graf]

On 28.08.23 22:38, Simon Glass wrote:
> Hi Alper,
>
> On Mon, 28 Aug 2023 at 12:29, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> On 2023-08-28 20:54 +03:00, Simon Glass wrote:
>>> Hi Alper,
>>> On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
>>>>> Hi Alper,
>>>>>
>>>>> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>>> From: Alexander Graf <agraf@csgraf.de>
>>>>>>
>>>>>> Now that we have everything in place to support ramfb, let's wire it up
>>>>>> by default in the ARM QEMU targets. That way, you can easily use a
>>>>>> graphical console by just passing -device ramfb to the QEMU command line.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>>>>>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
>>>>>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
>>>>>> - Drop env changes from ARM (necessary but in prerequisite series)
>>>>>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
>>>>>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
>>>>>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
>>>>>>
>>>>>>   arch/arm/Kconfig                    |  3 +++
>>>>>>   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
>>>>>>   2 files changed, 44 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>>>>> index 1fd3ccd1607f..7afe26ac804f 100644
>>>>>> --- a/arch/arm/Kconfig
>>>>>> +++ b/arch/arm/Kconfig
>>>>>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
>>>>>>          imply USB_XHCI_PCI
>>>>>>          imply USB_KEYBOARD
>>>>>>          imply CMD_USB
>>>>>> +       imply VIDEO_RAMFB
>>>>>> +       imply BOARD_EARLY_INIT_F
>>>>>> +       imply BOARD_EARLY_INIT_R
>>>>>>
>>>>>>   config ARCH_RMOBILE
>>>>>>          bool "Renesas ARM SoCs"
>>>>>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
>>>>>> index 942f1fff5717..23ef31cb7feb 100644
>>>>>> --- a/board/emulation/qemu-arm/qemu-arm.c
>>>>>> +++ b/board/emulation/qemu-arm/qemu-arm.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>   #include <fdtdec.h>
>>>>>>   #include <init.h>
>>>>>>   #include <log.h>
>>>>>> +#include <qfw.h>
>>>>>>   #include <usb.h>
>>>>>>   #include <virtio_types.h>
>>>>>>   #include <virtio.h>
>>>>>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
>>>>>>   struct mm_region *mem_map = qemu_arm64_mem_map;
>>>>>>   #endif
>>>>>>
>>>>>> +int board_early_init_f(void)
>>>>>> +{
>>>>>> +       struct udevice *dev;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
>>>>>> +        * so video_reserve() can reserve memory for it.
>>>>>> +        */
>>>>>> +       if (IS_ENABLED(CONFIG_QFW)) {
>>>>>> +               ret = qfw_get_dev(&dev);
>>>>>> +               if (ret) {
>>>>>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
>>>>> We should only present an error if the device is present but
>>>>> failed...so if the user doesn't provide the flag, all should be well.
>>>> I don't understand what you mean by "user doesn't provide the flag". But
>>>> I assume this should ignore the error (unless what?) so that we can
>>>> continue to boot. Would that apply for board_early_init_r below as well?
>>> I thought you said there was a qemu flag to enable ramfb? So add it to
>>> the DT and then the device can (in its bind routine) decide not to be
>>> bound by returning -ENODEV
>> Ah. I'm not used to calling them flags, especially those that have
>> values as in "-device ramfb". My mind jumped to DM_FLAG_PRE_RELOC /
>> bootph-some-ram.
>>
>> But note that what this code tries to probe here is not the ramfb
>> device, it's the bus/parent of that. If this fails, we can't know if the
>> user specified "-device ramfb" or not.
>>
>> Is the parent node's device probed by the time .bind() is called? Or,
>> can I have it probed by calling qfw_get_dev() (which is just
>> uclass_first_device_err(UCLASS_QFW, ...))?
> I suppose your problem might be that there are up to three possible
> video devices (vesa, bochs, ramfb) and one of them needs to return 0
> from bind() and the others need to return -ENODEV?


How do multiple Legacy VGA devices work in parallel in the first place? 
Do we access the legacy VGA PIO ports or memory ranges? If so, which of 
the VGA devices owns it at which point in time?

IIRC the only way you can actually multiplex real VGA is by putting a 
VGA arbiter[1] in between that literally maps the BARs in and out or 
enables/disables device mapping dynamically, no?

It probably makes most sense to make multiple video outputs work 
smoothly with at most 1 VGA device at a time for now to resolve any 
U-Boot internal infrastructure problems, then maybe later try multiple VGAs.


Alex

[1] https://docs.kernel.org/gpu/vgaarbiter.html