All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Move qfw to DM, add Arm support
@ 2021-02-23 11:43 Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include " Asherah Connor
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 11:43 UTC (permalink / raw)
  To: u-boot

Version 3 of this series moves the QFW driver into a uclass, UCLASS_QFW,
and splits the driver into qfw_pio and qfw_mmio.  The ugly arch-specific
ifdefs are now gone, since regular Makefile conditional compilation of
each driver takes care of it for us.

As before, on x86 a U_BOOT_DRVINFO is used to configure the qfw_pio
driver, otherwise we configure qfw_mmio from device tree if present.

I continue to test this on arm64 and x86_64 qemu.  A sandbox driver is
also included, and a DM unit test for it.

A test that runs in the qemu platform is still yet to come -- I wanted
to submit this ahead of that for more feedback.  One question: how much
should I roll these patches together?  The first introduces a few
changes that are immediately superceded by the second, but maybe it's
helpful for review?

To view the changes online, see:
https://git.src.kameliya.ee/~kameliya/u-boot/log/qfw-priv

Changes in v3:
- ARCH_QEMU now implies CMD_QFW, not QFW
- rename qemu_fwcfg_read_entry_pio to ..._io

Asherah Connor (4):
  arm: x86: qemu: move qfw to DM, include Arm support
  arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio
  arm: x86: qemu: unify qfw driver functions as qfw_
  qemu: add sandbox driver and tests

 arch/arm/Kconfig              |   1 +
 arch/sandbox/dts/sandbox.dtsi |   4 +
 arch/sandbox/dts/test.dts     |   4 +
 arch/x86/cpu/qemu/cpu.c       |   7 +-
 arch/x86/cpu/qemu/qemu.c      |  52 ++------
 arch/x86/cpu/qfw_cpu.c        |  11 +-
 cmd/qfw.c                     |  56 ++++----
 common/Makefile               |   2 +
 common/qfw.c                  | 111 ++++++++++++++++
 drivers/misc/Makefile         |   7 +-
 drivers/misc/qfw.c            | 239 ++++++++++++----------------------
 drivers/misc/qfw_mmio.c       | 101 ++++++++++++++
 drivers/misc/qfw_pio.c        |  66 ++++++++++
 drivers/misc/qfw_sandbox.c    | 129 ++++++++++++++++++
 include/dm/uclass-id.h        |   1 +
 include/qfw.h                 |  90 +++++++++----
 test/dm/Makefile              |   1 +
 test/dm/qfw.c                 |  42 ++++++
 18 files changed, 665 insertions(+), 259 deletions(-)
 create mode 100644 common/qfw.c
 create mode 100644 drivers/misc/qfw_mmio.c
 create mode 100644 drivers/misc/qfw_pio.c
 create mode 100644 drivers/misc/qfw_sandbox.c
 create mode 100644 test/dm/qfw.c

-- 
2.20.1

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 11:43 [PATCH v3 0/4] Move qfw to DM, add Arm support Asherah Connor
@ 2021-02-23 11:43 ` Asherah Connor
  2021-02-23 12:59   ` Heinrich Schuchardt
  2021-02-23 11:43 ` [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio Asherah Connor
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 11:43 UTC (permalink / raw)
  To: u-boot

Updates the QFW driver to use the driver model, and adds support for QFW
on Arm platforms by configuring from the device tree and using MMIO
accordingly.  A sandbox driver for QFW is also included, and a simple DM
unit test for it.

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---

Changes in v3:
- ARCH_QEMU now implies CMD_QFW, not QFW
- rename qemu_fwcfg_read_entry_pio to ..._io

 arch/arm/Kconfig         |   1 +
 arch/x86/cpu/qemu/cpu.c  |   7 +-
 arch/x86/cpu/qemu/qemu.c |  54 ++------
 arch/x86/cpu/qfw_cpu.c   |  11 +-
 cmd/qfw.c                |  44 +++---
 drivers/misc/Kconfig     |   1 +
 drivers/misc/qfw.c       | 289 +++++++++++++++++++++++++++------------
 include/qfw.h            |  63 +++++----
 8 files changed, 292 insertions(+), 178 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d51abbeaf0..cd01dc458a 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -937,6 +937,7 @@ config ARCH_QEMU
 	imply DM_RNG
 	imply DM_RTC
 	imply RTC_PL031
+	imply CMD_QFW
 
 config ARCH_RMOBILE
 	bool "Renesas ARM SoCs"
diff --git a/arch/x86/cpu/qemu/cpu.c b/arch/x86/cpu/qemu/cpu.c
index 9ce86b379c..ab1b797f9a 100644
--- a/arch/x86/cpu/qemu/cpu.c
+++ b/arch/x86/cpu/qemu/cpu.c
@@ -22,7 +22,12 @@ int cpu_qemu_get_desc(const struct udevice *dev, char *buf, int size)
 
 static int cpu_qemu_get_count(const struct udevice *dev)
 {
-	return qemu_fwcfg_online_cpus();
+	struct udevice *qfw_dev = qemu_fwcfg_dev();
+
+	if (!qfw_dev)
+		return -ENODEV;
+
+	return qemu_fwcfg_online_cpus(qfw_dev);
 }
 
 static const struct cpu_ops cpu_qemu_ops = {
diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
index 044a429c13..e255af9a4a 100644
--- a/arch/x86/cpu/qemu/qemu.c
+++ b/arch/x86/cpu/qemu/qemu.c
@@ -8,6 +8,7 @@
 #include <init.h>
 #include <pci.h>
 #include <qfw.h>
+#include <dm/platdata.h>
 #include <asm/irq.h>
 #include <asm/post.h>
 #include <asm/processor.h>
@@ -19,45 +20,20 @@ static bool i440fx;
 #ifdef CONFIG_QFW
 
 /* on x86, the qfw registers are all IO ports */
-#define FW_CONTROL_PORT	0x510
-#define FW_DATA_PORT		0x511
-#define FW_DMA_PORT_LOW	0x514
-#define FW_DMA_PORT_HIGH	0x518
-
-static void qemu_x86_fwcfg_read_entry_pio(uint16_t entry,
-		uint32_t size, void *address)
-{
-	uint32_t i = 0;
-	uint8_t *data = address;
-
-	/*
-	 * writting FW_CFG_INVALID will cause read operation to resume at
-	 * last offset, otherwise read will start at offset 0
-	 *
-	 * Note: on platform where the control register is IO port, the
-	 * endianness is little endian.
-	 */
-	if (entry != FW_CFG_INVALID)
-		outw(cpu_to_le16(entry), FW_CONTROL_PORT);
-
-	/* the endianness of data register is string-preserving */
-	while (size--)
-		data[i++] = inb(FW_DATA_PORT);
-}
-
-static void qemu_x86_fwcfg_read_entry_dma(struct fw_cfg_dma_access *dma)
-{
-	/* the DMA address register is big endian */
-	outl(cpu_to_be32((uintptr_t)dma), FW_DMA_PORT_HIGH);
-
-	while (be32_to_cpu(dma->control) & ~FW_CFG_DMA_ERROR)
-		__asm__ __volatile__ ("pause");
-}
+static const struct qfw_plat x86_qfw_plat = {
+	.io = {
+		.control_port	= 0x510,
+		.data_port	= 0x511,
+		.dma_port_low	= 0x514,
+		.dma_port_high	= 0x518,
+	},
+};
 
-static struct fw_cfg_arch_ops fwcfg_x86_ops = {
-	.arch_read_pio = qemu_x86_fwcfg_read_entry_pio,
-	.arch_read_dma = qemu_x86_fwcfg_read_entry_dma
+U_BOOT_DRVINFO(x86_qfw) = {
+	.name = "qfw",
+	.plat = &x86_qfw_plat,
 };
+
 #endif
 
 static void enable_pm_piix(void)
@@ -132,10 +108,6 @@ static void qemu_chipset_init(void)
 
 		enable_pm_ich9();
 	}
-
-#ifdef CONFIG_QFW
-	qemu_fwcfg_init(&fwcfg_x86_ops);
-#endif
 }
 
 #if !CONFIG_IS_ENABLED(SPL_X86_32BIT_INIT)
diff --git a/arch/x86/cpu/qfw_cpu.c b/arch/x86/cpu/qfw_cpu.c
index b959eaddde..c8fb918494 100644
--- a/arch/x86/cpu/qfw_cpu.c
+++ b/arch/x86/cpu/qfw_cpu.c
@@ -18,7 +18,7 @@ int qemu_cpu_fixup(void)
 	int cpu_num;
 	int cpu_online;
 	struct uclass *uc;
-	struct udevice *dev, *pdev;
+	struct udevice *dev, *pdev, *qfwdev;
 	struct cpu_plat *plat;
 	char *cpu;
 
@@ -39,6 +39,13 @@ int qemu_cpu_fixup(void)
 		return -ENODEV;
 	}
 
+	/* get qfw dev */
+	qfwdev = qemu_fwcfg_dev();
+	if (!qfwdev) {
+		printf("unable to find qfw device\n");
+		return -ENODEV;
+	}
+
 	/* calculate cpus that are already bound */
 	cpu_num = 0;
 	for (uclass_find_first_device(UCLASS_CPU, &dev);
@@ -48,7 +55,7 @@ int qemu_cpu_fixup(void)
 	}
 
 	/* get actual cpu number */
-	cpu_online = qemu_fwcfg_online_cpus();
+	cpu_online = qemu_fwcfg_online_cpus(qfwdev);
 	if (cpu_online < 0) {
 		printf("unable to get online cpu number: %d\n", cpu_online);
 		return cpu_online;
diff --git a/cmd/qfw.c b/cmd/qfw.c
index bb571487f0..ec80a9a3b5 100644
--- a/cmd/qfw.c
+++ b/cmd/qfw.c
@@ -8,19 +8,22 @@
 #include <env.h>
 #include <errno.h>
 #include <qfw.h>
+#include <dm.h>
+
+static struct udevice *qfw_dev;
 
 /*
  * This function prepares kernel for zboot. It loads kernel data
  * to 'load_addr', initrd to 'initrd_addr' and kernel command
  * line using qemu fw_cfg interface.
  */
-static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
+static int qemu_fwcfg_cmd_setup_kernel(void *load_addr, void *initrd_addr)
 {
 	char *data_addr;
 	uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
 
-	qemu_fwcfg_read_entry(FW_CFG_SETUP_SIZE, 4, &setup_size);
-	qemu_fwcfg_read_entry(FW_CFG_KERNEL_SIZE, 4, &kernel_size);
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_SIZE, 4, &setup_size);
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_SIZE, 4, &kernel_size);
 
 	if (setup_size == 0 || kernel_size == 0) {
 		printf("warning: no kernel available\n");
@@ -28,27 +31,27 @@ static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
 	}
 
 	data_addr = load_addr;
-	qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA,
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_DATA,
 			      le32_to_cpu(setup_size), data_addr);
 	data_addr += le32_to_cpu(setup_size);
 
-	qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA,
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_DATA,
 			      le32_to_cpu(kernel_size), data_addr);
 	data_addr += le32_to_cpu(kernel_size);
 
 	data_addr = initrd_addr;
-	qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_SIZE, 4, &initrd_size);
 	if (initrd_size == 0) {
 		printf("warning: no initrd available\n");
 	} else {
-		qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA,
+		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_DATA,
 				      le32_to_cpu(initrd_size), data_addr);
 		data_addr += le32_to_cpu(initrd_size);
 	}
 
-	qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
+	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
 	if (cmdline_size) {
-		qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA,
+		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_DATA,
 				      le32_to_cpu(cmdline_size), data_addr);
 		/*
 		 * if kernel cmdline only contains '\0', (e.g. no -append
@@ -72,19 +75,18 @@ static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
 	return 0;
 }
 
-static int qemu_fwcfg_list_firmware(void)
+static int qemu_fwcfg_cmd_list_firmware(void)
 {
 	int ret;
 	struct fw_cfg_file_iter iter;
 	struct fw_file *file;
 
 	/* make sure fw_list is loaded */
-	ret = qemu_fwcfg_read_firmware_list();
+	ret = qemu_fwcfg_read_firmware_list(qfw_dev);
 	if (ret)
 		return ret;
 
-
-	for (file = qemu_fwcfg_file_iter_init(&iter);
+	for (file = qemu_fwcfg_file_iter_init(qfw_dev, &iter);
 	     !qemu_fwcfg_file_iter_end(&iter);
 	     file = qemu_fwcfg_file_iter_next(&iter)) {
 		printf("%-56s\n", file->cfg.name);
@@ -96,7 +98,7 @@ static int qemu_fwcfg_list_firmware(void)
 static int qemu_fwcfg_do_list(struct cmd_tbl *cmdtp, int flag,
 			      int argc, char *const argv[])
 {
-	if (qemu_fwcfg_list_firmware() < 0)
+	if (qemu_fwcfg_cmd_list_firmware() < 0)
 		return CMD_RET_FAILURE;
 
 	return 0;
@@ -105,14 +107,7 @@ static int qemu_fwcfg_do_list(struct cmd_tbl *cmdtp, int flag,
 static int qemu_fwcfg_do_cpus(struct cmd_tbl *cmdtp, int flag,
 			      int argc, char *const argv[])
 {
-	int ret = qemu_fwcfg_online_cpus();
-	if (ret < 0) {
-		printf("QEMU fw_cfg interface not found\n");
-		return CMD_RET_FAILURE;
-	}
-
-	printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus());
-
+	printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus(qfw_dev));
 	return 0;
 }
 
@@ -153,7 +148,7 @@ static int qemu_fwcfg_do_load(struct cmd_tbl *cmdtp, int flag,
 		return CMD_RET_FAILURE;
 	}
 
-	return qemu_fwcfg_setup_kernel(load_addr, initrd_addr);
+	return qemu_fwcfg_cmd_setup_kernel(load_addr, initrd_addr);
 }
 
 static struct cmd_tbl fwcfg_commands[] = {
@@ -168,7 +163,8 @@ static int do_qemu_fw(struct cmd_tbl *cmdtp, int flag, int argc,
 	int ret;
 	struct cmd_tbl *fwcfg_cmd;
 
-	if (!qemu_fwcfg_present()) {
+	qfw_dev = qemu_fwcfg_dev();
+	if (!qfw_dev) {
 		printf("QEMU fw_cfg interface not found\n");
 		return CMD_RET_USAGE;
 	}
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 7d2a299779..0a65f29acd 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -367,6 +367,7 @@ config WINBOND_W83627
 
 config QFW
 	bool
+	depends on MISC
 	help
 	  Hidden option to enable QEMU fw_cfg interface. This will be selected by
 	  either CONFIG_CMD_QFW or CONFIG_GENERATE_ACPI_TABLE.
diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
index f6eb6583ed..eae3afd23b 100644
--- a/drivers/misc/qfw.c
+++ b/drivers/misc/qfw.c
@@ -9,17 +9,18 @@
 #include <log.h>
 #include <malloc.h>
 #include <qfw.h>
+#include <dm.h>
 #include <asm/io.h>
+#include <misc.h>
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 #include <asm/tables.h>
 #endif
-#include <linux/list.h>
 
-static bool fwcfg_present;
-static bool fwcfg_dma_present;
-static struct fw_cfg_arch_ops *fwcfg_arch_ops;
-
-static LIST_HEAD(fw_list);
+/* Determined at runtime. */
+struct qfw_priv {
+	bool dma_present;
+	struct list_head fw_list;
+};
 
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 /*
@@ -32,7 +33,8 @@ static LIST_HEAD(fw_list);
  *          be ignored.
  * @return: 0 on success, or negative value on failure
  */
-static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
+static int bios_linker_allocate(struct udevice *dev,
+				struct bios_linker_entry *entry, ulong *addr)
 {
 	uint32_t size, align;
 	struct fw_file *file;
@@ -45,7 +47,7 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
 		return -EINVAL;
 	}
 
-	file = qemu_fwcfg_find_file(entry->alloc.file);
+	file = qemu_fwcfg_find_file(dev, entry->alloc.file);
 	if (!file) {
 		printf("error: can't find file %s\n", entry->alloc.file);
 		return -ENOENT;
@@ -75,7 +77,7 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
 	debug("bios_linker_allocate: allocate file %s, size %u, zone %d, align %u, addr 0x%lx\n",
 	      file->cfg.name, size, entry->alloc.zone, align, aligned_addr);
 
-	qemu_fwcfg_read_entry(be16_to_cpu(file->cfg.select),
+	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
 			      size, (void *)aligned_addr);
 	file->addr = aligned_addr;
 
@@ -94,16 +96,17 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
  *          ACPI tables
  * @return: 0 on success, or negative value on failure
  */
-static int bios_linker_add_pointer(struct bios_linker_entry *entry)
+static int bios_linker_add_pointer(struct udevice *dev,
+				   struct bios_linker_entry *entry)
 {
 	struct fw_file *dest, *src;
 	uint32_t offset = le32_to_cpu(entry->pointer.offset);
 	uint64_t pointer = 0;
 
-	dest = qemu_fwcfg_find_file(entry->pointer.dest_file);
+	dest = qemu_fwcfg_find_file(dev, entry->pointer.dest_file);
 	if (!dest || !dest->addr)
 		return -ENOENT;
-	src = qemu_fwcfg_find_file(entry->pointer.src_file);
+	src = qemu_fwcfg_find_file(dev, entry->pointer.src_file);
 	if (!src || !src->addr)
 		return -ENOENT;
 
@@ -127,13 +130,14 @@ static int bios_linker_add_pointer(struct bios_linker_entry *entry)
  *          checksums
  * @return: 0 on success, or negative value on failure
  */
-static int bios_linker_add_checksum(struct bios_linker_entry *entry)
+static int bios_linker_add_checksum(struct udevice *dev,
+				    struct bios_linker_entry *entry)
 {
 	struct fw_file *file;
 	uint8_t *data, cksum = 0;
 	uint8_t *cksum_start;
 
-	file = qemu_fwcfg_find_file(entry->cksum.file);
+	file = qemu_fwcfg_find_file(dev, entry->cksum.file);
 	if (!file || !file->addr)
 		return -ENOENT;
 
@@ -154,15 +158,22 @@ ulong write_acpi_tables(ulong addr)
 	struct bios_linker_entry *table_loader;
 	struct bios_linker_entry *entry;
 	uint32_t size;
+	struct udevice *dev;
+
+	dev = qemu_fwcfg_dev();
+	if (!dev) {
+		printf("error: no qfw\n");
+		return addr;
+	}
 
 	/* make sure fw_list is loaded */
-	ret = qemu_fwcfg_read_firmware_list();
+	ret = qemu_fwcfg_read_firmware_list(dev);
 	if (ret) {
 		printf("error: can't read firmware file list\n");
 		return addr;
 	}
 
-	file = qemu_fwcfg_find_file("etc/table-loader");
+	file = qemu_fwcfg_find_file(dev, "etc/table-loader");
 	if (!file) {
 		printf("error: can't find etc/table-loader\n");
 		return addr;
@@ -180,24 +191,24 @@ ulong write_acpi_tables(ulong addr)
 		return addr;
 	}
 
-	qemu_fwcfg_read_entry(be16_to_cpu(file->cfg.select),
+	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
 			      size, table_loader);
 
 	for (i = 0; i < (size / sizeof(*entry)); i++) {
 		entry = table_loader + i;
 		switch (le32_to_cpu(entry->command)) {
 		case BIOS_LINKER_LOADER_COMMAND_ALLOCATE:
-			ret = bios_linker_allocate(entry, &addr);
+			ret = bios_linker_allocate(dev, entry, &addr);
 			if (ret)
 				goto out;
 			break;
 		case BIOS_LINKER_LOADER_COMMAND_ADD_POINTER:
-			ret = bios_linker_add_pointer(entry);
+			ret = bios_linker_add_pointer(dev, entry);
 			if (ret)
 				goto out;
 			break;
 		case BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM:
-			ret = bios_linker_add_checksum(entry);
+			ret = bios_linker_add_checksum(dev, entry);
 			if (ret)
 				goto out;
 			break;
@@ -209,7 +220,7 @@ ulong write_acpi_tables(ulong addr)
 out:
 	if (ret) {
 		struct fw_cfg_file_iter iter;
-		for (file = qemu_fwcfg_file_iter_init(&iter);
+		for (file = qemu_fwcfg_file_iter_init(dev, &iter);
 		     !qemu_fwcfg_file_iter_end(&iter);
 		     file = qemu_fwcfg_file_iter_next(&iter)) {
 			if (file->addr) {
@@ -226,31 +237,91 @@ out:
 ulong acpi_get_rsdp_addr(void)
 {
 	struct fw_file *file;
+	struct udevice *dev;
 
-	file = qemu_fwcfg_find_file("etc/acpi/rsdp");
+	dev = qemu_fwcfg_dev();
+	if (!dev) {
+		printf("error: no qfw\n");
+		return 0;
+	}
+
+	file = qemu_fwcfg_find_file(dev, "etc/acpi/rsdp");
 	return file->addr;
 }
 #endif
 
-/* Read configuration item using fw_cfg PIO interface */
-static void qemu_fwcfg_read_entry_pio(uint16_t entry,
-		uint32_t size, void *address)
+/* Read configuration item using fw_cfg PIO/MMIO interface */
+static void qemu_fwcfg_read_entry_io(struct qfw_plat *plat, u16 entry,
+				     u32 size, void *address)
 {
-	debug("qemu_fwcfg_read_entry_pio: entry 0x%x, size %u address %p\n",
+	debug("qemu_fwcfg_read_entry_io: entry 0x%x, size %u address %p\n",
 	      entry, size, address);
 
-	return fwcfg_arch_ops->arch_read_pio(entry, size, address);
+	/*
+	 * writing FW_CFG_INVALID will cause read operation to resume at last
+	 * offset, otherwise read will start at offset 0
+	 *
+	 * Note: on platform where the control register is IO port, the
+	 * endianness is little endian.  Where it is on MMIO, the register is
+	 * big endian.
+	 */
+	if (entry != FW_CFG_INVALID) {
+		if (plat->mmio)
+			plat->mmio->selector = cpu_to_be16(entry);
+#ifdef CONFIG_X86
+		else
+			outw(cpu_to_le16(entry), plat->io.control_port);
+#endif
+	}
+
+	/* the endianness of data register is string-preserving */
+
+	if (plat->mmio) {
+		while (size >= 8) {
+			*(u64 *)address = plat->mmio->data64;
+			address += 8;
+			size -= 8;
+		}
+		while (size >= 4) {
+			*(u32 *)address = plat->mmio->data32;
+			address += 4;
+			size -= 4;
+		}
+		while (size >= 2) {
+			*(u16 *)address = plat->mmio->data16;
+			address += 2;
+			size -= 2;
+		}
+		while (size >= 1) {
+			*(u8 *)address = plat->mmio->data8;
+			address += 1;
+			size -= 1;
+		}
+	}
+#ifdef CONFIG_X86
+	else {
+		u32 i = 0;
+		u8 *data = address;
+
+		while (size--)
+			data[i++] = inb(plat->io.data_port);
+	}
+#endif
 }
 
 /* Read configuration item using fw_cfg DMA interface */
-static void qemu_fwcfg_read_entry_dma(uint16_t entry,
-		uint32_t size, void *address)
+static void qemu_fwcfg_read_entry_dma(struct qfw_plat *plat, u16 entry,
+				      u32 size, void *address)
 {
-	struct fw_cfg_dma_access dma;
-
-	dma.length = cpu_to_be32(size);
-	dma.address = cpu_to_be64((uintptr_t)address);
-	dma.control = cpu_to_be32(FW_CFG_DMA_READ);
+	struct {
+		__be32 control;
+		__be32 length;
+		__be64 address;
+	} dma = {
+		.length = cpu_to_be32(size),
+		.address = cpu_to_be64((uintptr_t)address),
+		.control = cpu_to_be32(FW_CFG_DMA_READ),
+	};
 
 	/*
 	 * writting FW_CFG_INVALID will cause read operation to resume at
@@ -264,51 +335,58 @@ static void qemu_fwcfg_read_entry_dma(uint16_t entry,
 	debug("qemu_fwcfg_read_entry_dma: entry 0x%x, size %u address %p, control 0x%x\n",
 	      entry, size, address, be32_to_cpu(dma.control));
 
-	fwcfg_arch_ops->arch_read_dma(&dma);
-}
+	/* the DMA address register is big-endian */
+	if (plat->mmio)
+		plat->mmio->dma = cpu_to_be64((uintptr_t)&dma);
+#ifdef CONFIG_X86
+	else
+		outl(cpu_to_be32((uintptr_t)&dma), plat->io.dma_port_high);
+#endif
 
-bool qemu_fwcfg_present(void)
-{
-	return fwcfg_present;
-}
 
-bool qemu_fwcfg_dma_present(void)
-{
-	return fwcfg_dma_present;
+	while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
+#ifdef CONFIG_X86
+		__asm__ __volatile__ ("pause");
+#else
+		__asm__ __volatile__ ("yield");
+#endif
 }
 
-void qemu_fwcfg_read_entry(uint16_t entry, uint32_t length, void *address)
+void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
+			   void *address)
 {
-	if (fwcfg_dma_present)
-		qemu_fwcfg_read_entry_dma(entry, length, address);
+	struct qfw_plat *plat = dev_get_plat(dev);
+	struct qfw_priv *priv = dev_get_priv(dev);
+
+	if (priv->dma_present)
+		qemu_fwcfg_read_entry_dma(plat, entry, length, address);
 	else
-		qemu_fwcfg_read_entry_pio(entry, length, address);
+		qemu_fwcfg_read_entry_io(plat, entry, length, address);
 }
 
-int qemu_fwcfg_online_cpus(void)
+int qemu_fwcfg_online_cpus(struct udevice *dev)
 {
-	uint16_t nb_cpus;
-
-	if (!fwcfg_present)
-		return -ENODEV;
+	u16 nb_cpus;
 
-	qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
+	qemu_fwcfg_read_entry(dev, FW_CFG_NB_CPUS, 2, &nb_cpus);
 
 	return le16_to_cpu(nb_cpus);
 }
 
-int qemu_fwcfg_read_firmware_list(void)
+int qemu_fwcfg_read_firmware_list(struct udevice *dev)
 {
 	int i;
-	uint32_t count;
+	u32 count;
 	struct fw_file *file;
 	struct list_head *entry;
 
+	struct qfw_priv *priv = dev_get_priv(dev);
+
 	/* don't read it twice */
-	if (!list_empty(&fw_list))
+	if (!list_empty(&priv->fw_list))
 		return 0;
 
-	qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
+	qemu_fwcfg_read_entry(dev, FW_CFG_FILE_DIR, 4, &count);
 	if (!count)
 		return 0;
 
@@ -319,16 +397,16 @@ int qemu_fwcfg_read_firmware_list(void)
 			printf("error: allocating resource\n");
 			goto err;
 		}
-		qemu_fwcfg_read_entry(FW_CFG_INVALID,
+		qemu_fwcfg_read_entry(dev, FW_CFG_INVALID,
 				      sizeof(struct fw_cfg_file), &file->cfg);
 		file->addr = 0;
-		list_add_tail(&file->list, &fw_list);
+		list_add_tail(&file->list, &priv->fw_list);
 	}
 
 	return 0;
 
 err:
-	list_for_each(entry, &fw_list) {
+	list_for_each(entry, &priv->fw_list) {
 		file = list_entry(entry, struct fw_file, list);
 		free(file);
 	}
@@ -336,12 +414,14 @@ err:
 	return -ENOMEM;
 }
 
-struct fw_file *qemu_fwcfg_find_file(const char *name)
+struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
 {
 	struct list_head *entry;
 	struct fw_file *file;
 
-	list_for_each(entry, &fw_list) {
+	struct qfw_priv *priv = dev_get_priv(dev);
+
+	list_for_each(entry, &priv->fw_list) {
 		file = list_entry(entry, struct fw_file, list);
 		if (!strcmp(file->cfg.name, name))
 			return file;
@@ -350,9 +430,13 @@ struct fw_file *qemu_fwcfg_find_file(const char *name)
 	return NULL;
 }
 
-struct fw_file *qemu_fwcfg_file_iter_init(struct fw_cfg_file_iter *iter)
+struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
+					  struct fw_cfg_file_iter *iter)
 {
-	iter->entry = fw_list.next;
+	struct qfw_priv *priv = dev_get_priv(dev);
+
+	iter->entry = priv->fw_list.next;
+	iter->end = &priv->fw_list;
 	return list_entry((struct list_head *)iter->entry,
 			  struct fw_file, list);
 }
@@ -366,29 +450,62 @@ struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter)
 
 bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter)
 {
-	return iter->entry == &fw_list;
+	return iter->entry == iter->end;
 }
 
-void qemu_fwcfg_init(struct fw_cfg_arch_ops *ops)
+static int qfw_of_to_plat(struct udevice *dev)
 {
-	uint32_t qemu;
-	uint32_t dma_enabled;
-
-	fwcfg_present = false;
-	fwcfg_dma_present = false;
-	fwcfg_arch_ops = NULL;
-
-	if (!ops || !ops->arch_read_pio || !ops->arch_read_dma)
-		return;
-	fwcfg_arch_ops = ops;
-
-	qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
-	if (be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE)
-		fwcfg_present = true;
-
-	if (fwcfg_present) {
-		qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
-		if (dma_enabled & FW_CFG_DMA_ENABLED)
-			fwcfg_dma_present = true;
-	}
+	struct qfw_plat *plat = dev_get_plat(dev);
+
+	plat->mmio = map_physmem(dev_read_addr(dev),
+				 sizeof(struct qfw_mmio),
+				 MAP_NOCACHE);
+
+	return 0;
+}
+
+static int qfw_probe(struct udevice *dev)
+{
+	u32 qemu, dma_enabled;
+	struct qfw_plat *plat = dev_get_plat(dev);
+	struct qfw_priv *priv = dev_get_priv(dev);
+
+	INIT_LIST_HEAD(&priv->fw_list);
+
+	qemu_fwcfg_read_entry_io(plat, FW_CFG_SIGNATURE, 4, &qemu);
+	if (be32_to_cpu(qemu) != QEMU_FW_CFG_SIGNATURE)
+		return -ENODEV;
+
+	qemu_fwcfg_read_entry_io(plat, FW_CFG_ID, 1, &dma_enabled);
+	if (dma_enabled & FW_CFG_DMA_ENABLED)
+		priv->dma_present = true;
+
+	return 0;
+}
+
+static const struct udevice_id qfw_ids[] = {
+	{ .compatible = "qemu,fw-cfg-mmio" },
+	{}
+};
+
+U_BOOT_DRIVER(qfw) = {
+	.name		= "qfw",
+	.id		= UCLASS_MISC,
+	.of_match	= qfw_ids,
+	.of_to_plat	= qfw_of_to_plat,
+	.plat_auto	= sizeof(struct qfw_plat),
+	.priv_auto	= sizeof(struct qfw_priv),
+	.probe		= qfw_probe,
+};
+
+struct udevice *qemu_fwcfg_dev(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	/* XXX: decide what to do here */
+	ret = uclass_first_device(UCLASS_MISC, &dev);
+	if (ret)
+		return NULL;
+	return dev;
 }
diff --git a/include/qfw.h b/include/qfw.h
index cea8e11d44..f9c6828841 100644
--- a/include/qfw.h
+++ b/include/qfw.h
@@ -82,19 +82,7 @@ struct fw_file {
 };
 
 struct fw_cfg_file_iter {
-	struct list_head *entry; /* structure to iterate file list */
-};
-
-struct fw_cfg_dma_access {
-	__be32 control;
-	__be32 length;
-	__be64 address;
-};
-
-struct fw_cfg_arch_ops {
-	void (*arch_read_pio)(uint16_t selector, uint32_t size,
-			void *address);
-	void (*arch_read_dma)(struct fw_cfg_dma_access *dma);
+	struct list_head *entry, *end; /* structures to iterate file list */
 };
 
 struct bios_linker_entry {
@@ -146,32 +134,59 @@ struct bios_linker_entry {
 	};
 } __packed;
 
+struct qfw_mmio {
+	/*
+	 * Each access to the 64-bit data register can be 8/16/32/64 bits wide.
+	 */
+	union {
+		u8 data8;
+		u16 data16;
+		u32 data32;
+		u64 data64;
+	};
+	u16 selector;
+	u8 padding[6];
+	u64 dma;
+};
+
+struct qfw_plat {
+	/* MMIO used on Arm. */
+	volatile struct qfw_mmio *mmio;
+	/* IO used on x86. */
+	struct {
+		u16 control_port;
+		u16 data_port;
+		u16 dma_port_low;
+		u16 dma_port_high;
+	} io;
+};
+
+struct udevice;
 /**
- * Initialize QEMU fw_cfg interface
+ * Get QEMU firmware config device
  *
- * @ops: arch specific read operations
+ * @return struct udevice * if present, NULL otherwise
  */
-void qemu_fwcfg_init(struct fw_cfg_arch_ops *ops);
+struct udevice *qemu_fwcfg_dev(void);
 
-void qemu_fwcfg_read_entry(uint16_t entry, uint32_t length, void *address);
-int qemu_fwcfg_read_firmware_list(void);
-struct fw_file *qemu_fwcfg_find_file(const char *name);
+void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
+			   void *address);
+int qemu_fwcfg_read_firmware_list(struct udevice *dev);
+struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name);
 
 /**
  * Get system cpu number
  *
  * @return:   cpu number in system
  */
-int qemu_fwcfg_online_cpus(void);
+int qemu_fwcfg_online_cpus(struct udevice *dev);
 
 /* helper functions to iterate firmware file list */
-struct fw_file *qemu_fwcfg_file_iter_init(struct fw_cfg_file_iter *iter);
+struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
+					  struct fw_cfg_file_iter *iter);
 struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter);
 bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter);
 
-bool qemu_fwcfg_present(void);
-bool qemu_fwcfg_dma_present(void);
-
 /**
  * qemu_cpu_fixup() - Fix up the CPUs for QEMU
  *
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio
  2021-02-23 11:43 [PATCH v3 0/4] Move qfw to DM, add Arm support Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include " Asherah Connor
@ 2021-02-23 11:43 ` Asherah Connor
  2021-02-23 15:58   ` Simon Glass
  2021-02-23 11:43 ` [PATCH v3 3/4] arm: x86: qemu: unify qfw driver functions as qfw_ Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 4/4] qemu: add sandbox driver and tests Asherah Connor
  3 siblings, 1 reply; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 11:43 UTC (permalink / raw)
  To: u-boot

Split the qfw driver into qfw_pio and qfw_mmio, under their own uclass.
Each driver does arch/platform-specific I/O.

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---

Changes in v3:
- Add new UCLASS_QFW, split qfw driver into PIO and MMIO variants.
- QFW no longer depends on or selects MISC.

 arch/x86/cpu/qemu/cpu.c  |   2 +-
 arch/x86/cpu/qemu/qemu.c |  18 ++-
 arch/x86/cpu/qfw_cpu.c   |   2 +-
 cmd/qfw.c                |  26 ++--
 common/Makefile          |   2 +
 common/qfw.c             | 111 ++++++++++++++++
 drivers/misc/Kconfig     |   1 -
 drivers/misc/Makefile    |   6 +
 drivers/misc/qfw.c       | 270 ++++++---------------------------------
 drivers/misc/qfw_mmio.c  | 101 +++++++++++++++
 drivers/misc/qfw_pio.c   |  66 ++++++++++
 include/dm/uclass-id.h   |   1 +
 include/qfw.h            |  45 +++++--
 13 files changed, 382 insertions(+), 269 deletions(-)
 create mode 100644 common/qfw.c
 create mode 100644 drivers/misc/qfw_mmio.c
 create mode 100644 drivers/misc/qfw_pio.c

diff --git a/arch/x86/cpu/qemu/cpu.c b/arch/x86/cpu/qemu/cpu.c
index ab1b797f9a..09499aad78 100644
--- a/arch/x86/cpu/qemu/cpu.c
+++ b/arch/x86/cpu/qemu/cpu.c
@@ -22,7 +22,7 @@ int cpu_qemu_get_desc(const struct udevice *dev, char *buf, int size)
 
 static int cpu_qemu_get_count(const struct udevice *dev)
 {
-	struct udevice *qfw_dev = qemu_fwcfg_dev();
+	struct udevice *qfw_dev = qfw_get_dev();
 
 	if (!qfw_dev)
 		return -ENODEV;
diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
index e255af9a4a..5a61cc3bb4 100644
--- a/arch/x86/cpu/qemu/qemu.c
+++ b/arch/x86/cpu/qemu/qemu.c
@@ -20,18 +20,16 @@ static bool i440fx;
 #ifdef CONFIG_QFW
 
 /* on x86, the qfw registers are all IO ports */
-static const struct qfw_plat x86_qfw_plat = {
-	.io = {
-		.control_port	= 0x510,
-		.data_port	= 0x511,
-		.dma_port_low	= 0x514,
-		.dma_port_high	= 0x518,
-	},
+static const struct qfw_pio_plat x86_qfw_pio_plat = {
+	.control_port	= 0x510,
+	.data_port	= 0x511,
+	.dma_port_low	= 0x514,
+	.dma_port_high	= 0x518,
 };
 
-U_BOOT_DRVINFO(x86_qfw) = {
-	.name = "qfw",
-	.plat = &x86_qfw_plat,
+U_BOOT_DRVINFO(x86_qfw_pio) = {
+	.name = "qfw_pio",
+	.plat = &x86_qfw_pio_plat,
 };
 
 #endif
diff --git a/arch/x86/cpu/qfw_cpu.c b/arch/x86/cpu/qfw_cpu.c
index c8fb918494..e80cec0ca4 100644
--- a/arch/x86/cpu/qfw_cpu.c
+++ b/arch/x86/cpu/qfw_cpu.c
@@ -40,7 +40,7 @@ int qemu_cpu_fixup(void)
 	}
 
 	/* get qfw dev */
-	qfwdev = qemu_fwcfg_dev();
+	qfwdev = qfw_get_dev();
 	if (!qfwdev) {
 		printf("unable to find qfw device\n");
 		return -ENODEV;
diff --git a/cmd/qfw.c b/cmd/qfw.c
index ec80a9a3b5..a983a45380 100644
--- a/cmd/qfw.c
+++ b/cmd/qfw.c
@@ -22,8 +22,8 @@ static int qemu_fwcfg_cmd_setup_kernel(void *load_addr, void *initrd_addr)
 	char *data_addr;
 	uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
 
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_SIZE, 4, &setup_size);
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_SIZE, 4, &kernel_size);
+	qfw_read_entry(qfw_dev, FW_CFG_SETUP_SIZE, 4, &setup_size);
+	qfw_read_entry(qfw_dev, FW_CFG_KERNEL_SIZE, 4, &kernel_size);
 
 	if (setup_size == 0 || kernel_size == 0) {
 		printf("warning: no kernel available\n");
@@ -31,28 +31,28 @@ static int qemu_fwcfg_cmd_setup_kernel(void *load_addr, void *initrd_addr)
 	}
 
 	data_addr = load_addr;
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_DATA,
-			      le32_to_cpu(setup_size), data_addr);
+	qfw_read_entry(qfw_dev, FW_CFG_SETUP_DATA,
+		       le32_to_cpu(setup_size), data_addr);
 	data_addr += le32_to_cpu(setup_size);
 
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_DATA,
-			      le32_to_cpu(kernel_size), data_addr);
+	qfw_read_entry(qfw_dev, FW_CFG_KERNEL_DATA,
+		       le32_to_cpu(kernel_size), data_addr);
 	data_addr += le32_to_cpu(kernel_size);
 
 	data_addr = initrd_addr;
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_SIZE, 4, &initrd_size);
+	qfw_read_entry(qfw_dev, FW_CFG_INITRD_SIZE, 4, &initrd_size);
 	if (initrd_size == 0) {
 		printf("warning: no initrd available\n");
 	} else {
-		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_DATA,
-				      le32_to_cpu(initrd_size), data_addr);
+		qfw_read_entry(qfw_dev, FW_CFG_INITRD_DATA,
+			       le32_to_cpu(initrd_size), data_addr);
 		data_addr += le32_to_cpu(initrd_size);
 	}
 
-	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
+	qfw_read_entry(qfw_dev, FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
 	if (cmdline_size) {
-		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_DATA,
-				      le32_to_cpu(cmdline_size), data_addr);
+		qfw_read_entry(qfw_dev, FW_CFG_CMDLINE_DATA,
+			       le32_to_cpu(cmdline_size), data_addr);
 		/*
 		 * if kernel cmdline only contains '\0', (e.g. no -append
 		 * when invoking qemu), do not update bootargs
@@ -163,7 +163,7 @@ static int do_qemu_fw(struct cmd_tbl *cmdtp, int flag, int argc,
 	int ret;
 	struct cmd_tbl *fwcfg_cmd;
 
-	qfw_dev = qemu_fwcfg_dev();
+	qfw_dev = qfw_get_dev();
 	if (!qfw_dev) {
 		printf("QEMU fw_cfg interface not found\n");
 		return CMD_RET_USAGE;
diff --git a/common/Makefile b/common/Makefile
index daeea67cf2..f174a06c33 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -137,3 +137,5 @@ obj-$(CONFIG_CMD_LOADB) += xyzModem.o
 obj-$(CONFIG_$(SPL_TPL_)YMODEM_SUPPORT) += xyzModem.o
 
 obj-$(CONFIG_AVB_VERIFY) += avb_verify.o
+
+obj-$(CONFIG_QFW) += qfw.o
diff --git a/common/qfw.c b/common/qfw.c
new file mode 100644
index 0000000000..b4c9e4483c
--- /dev/null
+++ b/common/qfw.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
+ * (C) Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
+ */
+
+#include <dm.h>
+#include <dm/uclass.h>
+#include <qfw.h>
+#include <stdlib.h>
+
+struct udevice *qfw_get_dev(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	ret = uclass_first_device(UCLASS_QFW, &dev);
+	if (ret)
+		return NULL;
+	return dev;
+}
+
+int qemu_fwcfg_online_cpus(struct udevice *dev)
+{
+	u16 nb_cpus;
+
+	qfw_read_entry(dev, FW_CFG_NB_CPUS, 2, &nb_cpus);
+
+	return le16_to_cpu(nb_cpus);
+}
+
+int qemu_fwcfg_read_firmware_list(struct udevice *dev)
+{
+	int i;
+	u32 count;
+	struct fw_file *file;
+	struct list_head *entry;
+
+	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
+
+	/* don't read it twice */
+	if (!list_empty(&qdev->fw_list))
+		return 0;
+
+	qfw_read_entry(dev, FW_CFG_FILE_DIR, 4, &count);
+	if (!count)
+		return 0;
+
+	count = be32_to_cpu(count);
+	for (i = 0; i < count; i++) {
+		file = malloc(sizeof(*file));
+		if (!file) {
+			printf("error: allocating resource\n");
+			goto err;
+		}
+		qfw_read_entry(dev, FW_CFG_INVALID,
+			       sizeof(struct fw_cfg_file), &file->cfg);
+		file->addr = 0;
+		list_add_tail(&file->list, &qdev->fw_list);
+	}
+
+	return 0;
+
+err:
+	list_for_each(entry, &qdev->fw_list) {
+		file = list_entry(entry, struct fw_file, list);
+		free(file);
+	}
+
+	return -ENOMEM;
+}
+
+struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
+{
+	struct list_head *entry;
+	struct fw_file *file;
+
+	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
+
+	list_for_each(entry, &qdev->fw_list) {
+		file = list_entry(entry, struct fw_file, list);
+		if (!strcmp(file->cfg.name, name))
+			return file;
+	}
+
+	return NULL;
+}
+
+struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
+					  struct fw_cfg_file_iter *iter)
+{
+	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
+
+	iter->entry = qdev->fw_list.next;
+	iter->end = &qdev->fw_list;
+	return list_entry((struct list_head *)iter->entry,
+			  struct fw_file, list);
+}
+
+struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter)
+{
+	iter->entry = ((struct list_head *)iter->entry)->next;
+	return list_entry((struct list_head *)iter->entry,
+			  struct fw_file, list);
+}
+
+bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter)
+{
+	return iter->entry == iter->end;
+}
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0a65f29acd..7d2a299779 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -367,7 +367,6 @@ config WINBOND_W83627
 
 config QFW
 	bool
-	depends on MISC
 	help
 	  Hidden option to enable QEMU fw_cfg interface. This will be selected by
 	  either CONFIG_CMD_QFW or CONFIG_GENERATE_ACPI_TABLE.
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d737203704..e6e1dfea95 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,6 +56,12 @@ obj-$(CONFIG_P2SB) += p2sb-uclass.o
 obj-$(CONFIG_PCA9551_LED) += pca9551_led.o
 obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o
 obj-$(CONFIG_QFW) += qfw.o
+ifdef CONFIG_X86
+obj-$(CONFIG_QFW) += qfw_pio.o
+endif
+ifdef CONFIG_ARM
+obj-$(CONFIG_QFW) += qfw_mmio.o
+endif
 obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
 obj-$(CONFIG_ROCKCHIP_OTP) += rockchip-otp.o
 obj-$(CONFIG_SANDBOX) += syscon_sandbox.o misc_sandbox.o
diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
index eae3afd23b..25b203375b 100644
--- a/drivers/misc/qfw.c
+++ b/drivers/misc/qfw.c
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
+ * (C) Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
  */
 
+#define LOG_CATEGORY UCLASS_QFW
+
 #include <common.h>
 #include <command.h>
 #include <errno.h>
@@ -10,18 +13,11 @@
 #include <malloc.h>
 #include <qfw.h>
 #include <dm.h>
-#include <asm/io.h>
 #include <misc.h>
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 #include <asm/tables.h>
 #endif
 
-/* Determined at runtime. */
-struct qfw_priv {
-	bool dma_present;
-	struct list_head fw_list;
-};
-
 #ifdef CONFIG_GENERATE_ACPI_TABLE
 /*
  * This function allocates memory for ACPI tables
@@ -77,8 +73,8 @@ static int bios_linker_allocate(struct udevice *dev,
 	debug("bios_linker_allocate: allocate file %s, size %u, zone %d, align %u, addr 0x%lx\n",
 	      file->cfg.name, size, entry->alloc.zone, align, aligned_addr);
 
-	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
-			      size, (void *)aligned_addr);
+	qfw_read_entry(dev, be16_to_cpu(file->cfg.select), size,
+		       (void *)aligned_addr);
 	file->addr = aligned_addr;
 
 	/* adjust address for low memory allocation */
@@ -160,7 +156,7 @@ ulong write_acpi_tables(ulong addr)
 	uint32_t size;
 	struct udevice *dev;
 
-	dev = qemu_fwcfg_dev();
+	dev = qfw_get_dev();
 	if (!dev) {
 		printf("error: no qfw\n");
 		return addr;
@@ -191,8 +187,7 @@ ulong write_acpi_tables(ulong addr)
 		return addr;
 	}
 
-	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
-			      size, table_loader);
+	qfw_read_entry(dev, be16_to_cpu(file->cfg.select), size, table_loader);
 
 	for (i = 0; i < (size / sizeof(*entry)); i++) {
 		entry = table_loader + i;
@@ -239,7 +234,7 @@ ulong acpi_get_rsdp_addr(void)
 	struct fw_file *file;
 	struct udevice *dev;
 
-	dev = qemu_fwcfg_dev();
+	dev = qfw_get_dev();
 	if (!dev) {
 		printf("error: no qfw\n");
 		return 0;
@@ -250,262 +245,75 @@ ulong acpi_get_rsdp_addr(void)
 }
 #endif
 
-/* Read configuration item using fw_cfg PIO/MMIO interface */
-static void qemu_fwcfg_read_entry_io(struct qfw_plat *plat, u16 entry,
-				     u32 size, void *address)
+static void qfw_read_entry_io(struct qfw_dev *qdev, u16 entry, u32 size,
+			      void *address)
 {
-	debug("qemu_fwcfg_read_entry_io: entry 0x%x, size %u address %p\n",
-	      entry, size, address);
+	struct dm_qfw_ops *ops = dm_qfw_get_ops(qdev->dev);
 
-	/*
-	 * writing FW_CFG_INVALID will cause read operation to resume at last
-	 * offset, otherwise read will start at offset 0
-	 *
-	 * Note: on platform where the control register is IO port, the
-	 * endianness is little endian.  Where it is on MMIO, the register is
-	 * big endian.
-	 */
-	if (entry != FW_CFG_INVALID) {
-		if (plat->mmio)
-			plat->mmio->selector = cpu_to_be16(entry);
-#ifdef CONFIG_X86
-		else
-			outw(cpu_to_le16(entry), plat->io.control_port);
-#endif
-	}
+	debug("%s: entry 0x%x, size %u address %p\n", __func__, entry, size,
+	      address);
 
-	/* the endianness of data register is string-preserving */
-
-	if (plat->mmio) {
-		while (size >= 8) {
-			*(u64 *)address = plat->mmio->data64;
-			address += 8;
-			size -= 8;
-		}
-		while (size >= 4) {
-			*(u32 *)address = plat->mmio->data32;
-			address += 4;
-			size -= 4;
-		}
-		while (size >= 2) {
-			*(u16 *)address = plat->mmio->data16;
-			address += 2;
-			size -= 2;
-		}
-		while (size >= 1) {
-			*(u8 *)address = plat->mmio->data8;
-			address += 1;
-			size -= 1;
-		}
-	}
-#ifdef CONFIG_X86
-	else {
-		u32 i = 0;
-		u8 *data = address;
-
-		while (size--)
-			data[i++] = inb(plat->io.data_port);
-	}
-#endif
+	ops->read_entry_io(qdev->dev, entry, size, address);
 }
 
-/* Read configuration item using fw_cfg DMA interface */
-static void qemu_fwcfg_read_entry_dma(struct qfw_plat *plat, u16 entry,
-				      u32 size, void *address)
+static void qfw_read_entry_dma(struct qfw_dev *qdev, u16 entry, u32 size,
+			       void *address)
 {
-	struct {
-		__be32 control;
-		__be32 length;
-		__be64 address;
-	} dma = {
+	struct dm_qfw_ops *ops = dm_qfw_get_ops(qdev->dev);
+
+	struct qfw_dma dma = {
 		.length = cpu_to_be32(size),
 		.address = cpu_to_be64((uintptr_t)address),
 		.control = cpu_to_be32(FW_CFG_DMA_READ),
 	};
 
 	/*
-	 * writting FW_CFG_INVALID will cause read operation to resume at
-	 * last offset, otherwise read will start at offset 0
+	 * writing FW_CFG_INVALID will cause read operation to resume at last
+	 * offset, otherwise read will start at offset 0
 	 */
 	if (entry != FW_CFG_INVALID)
 		dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
 
-	barrier();
-
-	debug("qemu_fwcfg_read_entry_dma: entry 0x%x, size %u address %p, control 0x%x\n",
+	debug("%s: entry 0x%x, size %u address %p, control 0x%x\n", __func__,
 	      entry, size, address, be32_to_cpu(dma.control));
 
-	/* the DMA address register is big-endian */
-	if (plat->mmio)
-		plat->mmio->dma = cpu_to_be64((uintptr_t)&dma);
-#ifdef CONFIG_X86
-	else
-		outl(cpu_to_be32((uintptr_t)&dma), plat->io.dma_port_high);
-#endif
-
+	barrier();
 
-	while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
-#ifdef CONFIG_X86
-		__asm__ __volatile__ ("pause");
-#else
-		__asm__ __volatile__ ("yield");
-#endif
+	ops->read_entry_dma(qdev->dev, &dma);
 }
 
-void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
-			   void *address)
+void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address)
 {
-	struct qfw_plat *plat = dev_get_plat(dev);
-	struct qfw_priv *priv = dev_get_priv(dev);
+	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
 
-	if (priv->dma_present)
-		qemu_fwcfg_read_entry_dma(plat, entry, length, address);
+	if (qdev->dma_present)
+		qfw_read_entry_dma(qdev, entry, length, address);
 	else
-		qemu_fwcfg_read_entry_io(plat, entry, length, address);
-}
-
-int qemu_fwcfg_online_cpus(struct udevice *dev)
-{
-	u16 nb_cpus;
-
-	qemu_fwcfg_read_entry(dev, FW_CFG_NB_CPUS, 2, &nb_cpus);
-
-	return le16_to_cpu(nb_cpus);
-}
-
-int qemu_fwcfg_read_firmware_list(struct udevice *dev)
-{
-	int i;
-	u32 count;
-	struct fw_file *file;
-	struct list_head *entry;
-
-	struct qfw_priv *priv = dev_get_priv(dev);
-
-	/* don't read it twice */
-	if (!list_empty(&priv->fw_list))
-		return 0;
-
-	qemu_fwcfg_read_entry(dev, FW_CFG_FILE_DIR, 4, &count);
-	if (!count)
-		return 0;
-
-	count = be32_to_cpu(count);
-	for (i = 0; i < count; i++) {
-		file = malloc(sizeof(*file));
-		if (!file) {
-			printf("error: allocating resource\n");
-			goto err;
-		}
-		qemu_fwcfg_read_entry(dev, FW_CFG_INVALID,
-				      sizeof(struct fw_cfg_file), &file->cfg);
-		file->addr = 0;
-		list_add_tail(&file->list, &priv->fw_list);
-	}
-
-	return 0;
-
-err:
-	list_for_each(entry, &priv->fw_list) {
-		file = list_entry(entry, struct fw_file, list);
-		free(file);
-	}
-
-	return -ENOMEM;
-}
-
-struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
-{
-	struct list_head *entry;
-	struct fw_file *file;
-
-	struct qfw_priv *priv = dev_get_priv(dev);
-
-	list_for_each(entry, &priv->fw_list) {
-		file = list_entry(entry, struct fw_file, list);
-		if (!strcmp(file->cfg.name, name))
-			return file;
-	}
-
-	return NULL;
-}
-
-struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
-					  struct fw_cfg_file_iter *iter)
-{
-	struct qfw_priv *priv = dev_get_priv(dev);
-
-	iter->entry = priv->fw_list.next;
-	iter->end = &priv->fw_list;
-	return list_entry((struct list_head *)iter->entry,
-			  struct fw_file, list);
-}
-
-struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter)
-{
-	iter->entry = ((struct list_head *)iter->entry)->next;
-	return list_entry((struct list_head *)iter->entry,
-			  struct fw_file, list);
-}
-
-bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter)
-{
-	return iter->entry == iter->end;
-}
-
-static int qfw_of_to_plat(struct udevice *dev)
-{
-	struct qfw_plat *plat = dev_get_plat(dev);
-
-	plat->mmio = map_physmem(dev_read_addr(dev),
-				 sizeof(struct qfw_mmio),
-				 MAP_NOCACHE);
-
-	return 0;
+		qfw_read_entry_io(qdev, entry, length, address);
 }
 
-static int qfw_probe(struct udevice *dev)
+int qfw_register(struct udevice *dev)
 {
+	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
 	u32 qemu, dma_enabled;
-	struct qfw_plat *plat = dev_get_plat(dev);
-	struct qfw_priv *priv = dev_get_priv(dev);
 
-	INIT_LIST_HEAD(&priv->fw_list);
+	qdev->dev = dev;
+	INIT_LIST_HEAD(&qdev->fw_list);
 
-	qemu_fwcfg_read_entry_io(plat, FW_CFG_SIGNATURE, 4, &qemu);
+	qfw_read_entry_io(qdev, FW_CFG_SIGNATURE, 4, &qemu);
 	if (be32_to_cpu(qemu) != QEMU_FW_CFG_SIGNATURE)
 		return -ENODEV;
 
-	qemu_fwcfg_read_entry_io(plat, FW_CFG_ID, 1, &dma_enabled);
+	qfw_read_entry_io(qdev, FW_CFG_ID, 1, &dma_enabled);
 	if (dma_enabled & FW_CFG_DMA_ENABLED)
-		priv->dma_present = true;
+		qdev->dma_present = true;
 
 	return 0;
 }
 
-static const struct udevice_id qfw_ids[] = {
-	{ .compatible = "qemu,fw-cfg-mmio" },
-	{}
-};
-
-U_BOOT_DRIVER(qfw) = {
+UCLASS_DRIVER(qfw) = {
+	.id		= UCLASS_QFW,
 	.name		= "qfw",
-	.id		= UCLASS_MISC,
-	.of_match	= qfw_ids,
-	.of_to_plat	= qfw_of_to_plat,
-	.plat_auto	= sizeof(struct qfw_plat),
-	.priv_auto	= sizeof(struct qfw_priv),
-	.probe		= qfw_probe,
+	.per_device_auto	= sizeof(struct qfw_dev),
 };
 
-struct udevice *qemu_fwcfg_dev(void)
-{
-	struct udevice *dev;
-	int ret;
-
-	/* XXX: decide what to do here */
-	ret = uclass_first_device(UCLASS_MISC, &dev);
-	if (ret)
-		return NULL;
-	return dev;
-}
diff --git a/drivers/misc/qfw_mmio.c b/drivers/misc/qfw_mmio.c
new file mode 100644
index 0000000000..aa903cb51c
--- /dev/null
+++ b/drivers/misc/qfw_mmio.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MMIO interface for QFW
+ *
+ * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
+ * (C) Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
+ */
+
+#define LOG_CATEGORY UCLASS_QFW
+
+#include <asm/types.h>
+#include <asm/io.h>
+#include <dm.h>
+#include <dm/device.h>
+#include <qfw.h>
+
+static void qfw_mmio_read_entry_io(struct udevice *dev, u16 entry, u32 size,
+				   void *address)
+{
+	struct qfw_mmio_plat *plat = dev_get_plat(dev);
+
+	/*
+	 * writing FW_CFG_INVALID will cause read operation to resume at last
+	 * offset, otherwise read will start at offset 0
+	 *
+	 * Note: on platform where the control register is MMIO, the register
+	 * is big endian.
+	 */
+	if (entry != FW_CFG_INVALID)
+		plat->mmio->selector = cpu_to_be16(entry);
+
+	/* the endianness of data register is string-preserving */
+	while (size >= 8) {
+		*(u64 *)address = plat->mmio->data64;
+		address += 8;
+		size -= 8;
+	}
+	while (size >= 4) {
+		*(u32 *)address = plat->mmio->data32;
+		address += 4;
+		size -= 4;
+	}
+	while (size >= 2) {
+		*(u16 *)address = plat->mmio->data16;
+		address += 2;
+		size -= 2;
+	}
+	while (size >= 1) {
+		*(u8 *)address = plat->mmio->data8;
+		address += 1;
+		size -= 1;
+	}
+}
+
+/* Read configuration item using fw_cfg DMA interface */
+static void qfw_mmio_read_entry_dma(struct udevice *dev, struct qfw_dma *dma)
+{
+	struct qfw_mmio_plat *plat = dev_get_plat(dev);
+
+	/* the DMA address register is big-endian */
+	plat->mmio->dma = cpu_to_be64((uintptr_t)dma);
+
+	while (be32_to_cpu(dma->control) & ~FW_CFG_DMA_ERROR)
+		__asm__ __volatile__ ("yield");
+}
+
+static int qfw_mmio_of_to_plat(struct udevice *dev)
+{
+	struct qfw_mmio_plat *plat = dev_get_plat(dev);
+
+	plat->mmio = map_physmem(dev_read_addr(dev),
+				 sizeof(struct qfw_mmio),
+				 MAP_NOCACHE);
+
+	return 0;
+}
+
+static int qfw_mmio_probe(struct udevice *dev)
+{
+	return qfw_register(dev);
+}
+
+static struct dm_qfw_ops qfw_mmio_ops = {
+	.read_entry_io = qfw_mmio_read_entry_io,
+	.read_entry_dma = qfw_mmio_read_entry_dma,
+};
+
+static const struct udevice_id qfw_mmio_ids[] = {
+	{ .compatible = "qemu,fw-cfg-mmio" },
+	{}
+};
+
+U_BOOT_DRIVER(qfw_mmio) = {
+	.name	= "qfw_mmio",
+	.id	= UCLASS_QFW,
+	.of_match	= qfw_mmio_ids,
+	.plat_auto	= sizeof(struct qfw_mmio_plat),
+	.of_to_plat	= qfw_mmio_of_to_plat,
+	.probe	= qfw_mmio_probe,
+	.ops	= &qfw_mmio_ops,
+};
diff --git a/drivers/misc/qfw_pio.c b/drivers/misc/qfw_pio.c
new file mode 100644
index 0000000000..beb722ad22
--- /dev/null
+++ b/drivers/misc/qfw_pio.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PIO interface for QFW
+ *
+ * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
+ * (C) Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
+ */
+
+#define LOG_CATEGORY UCLASS_QFW
+
+#include <asm/io.h>
+#include <dm/device.h>
+#include <qfw.h>
+
+static void qfw_pio_read_entry_io(struct udevice *dev, u16 entry, u32 size,
+				  void *address)
+{
+	struct qfw_pio_plat *plat = dev_get_plat(dev);
+
+	/*
+	 * writing FW_CFG_INVALID will cause read operation to resume at last
+	 * offset, otherwise read will start at offset 0
+	 *
+	 * Note: on platform where the control register is IO port, the
+	 * endianness is little endian.
+	 */
+	if (entry != FW_CFG_INVALID)
+		outw(cpu_to_le16(entry), plat->control_port);
+
+	/* the endianness of data register is string-preserving */
+	u32 i = 0;
+	u8 *data = address;
+
+	while (size--)
+		data[i++] = inb(plat->data_port);
+}
+
+/* Read configuration item using fw_cfg DMA interface */
+static void qfw_pio_read_entry_dma(struct udevice *dev, struct qfw_dma *dma)
+{
+	struct qfw_pio_plat *plat = dev_get_plat(dev);
+
+	/* the DMA address register is big-endian */
+	outl(cpu_to_be32((uintptr_t)dma), plat->dma_port_high);
+
+	while (be32_to_cpu(dma->control) & ~FW_CFG_DMA_ERROR)
+		__asm__ __volatile__ ("pause");
+}
+
+static int qfw_pio_probe(struct udevice *dev)
+{
+	return qfw_register(dev);
+}
+
+static struct dm_qfw_ops qfw_pio_ops = {
+	.read_entry_io = qfw_pio_read_entry_io,
+	.read_entry_dma = qfw_pio_read_entry_dma,
+};
+
+U_BOOT_DRIVER(qfw_pio) = {
+	.name	= "qfw_pio",
+	.id	= UCLASS_QFW,
+	.plat_auto	= sizeof(struct qfw_pio_plat),
+	.probe	= qfw_pio_probe,
+	.ops	= &qfw_pio_ops,
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index d75de368c5..d800f679d5 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -90,6 +90,7 @@ enum uclass_id {
 	UCLASS_POWER_DOMAIN,	/* (SoC) Power domains */
 	UCLASS_PWM,		/* Pulse-width modulator */
 	UCLASS_PWRSEQ,		/* Power sequence device */
+	UCLASS_QFW,		/* QEMU firmware config device */
 	UCLASS_RAM,		/* RAM controller */
 	UCLASS_REGULATOR,	/* Regulator device */
 	UCLASS_REMOTEPROC,	/* Remote Processor device */
diff --git a/include/qfw.h b/include/qfw.h
index f9c6828841..d7e16651a2 100644
--- a/include/qfw.h
+++ b/include/qfw.h
@@ -149,28 +149,49 @@ struct qfw_mmio {
 	u64 dma;
 };
 
-struct qfw_plat {
-	/* MMIO used on Arm. */
+struct qfw_pio_plat {
+	u16 control_port;
+	u16 data_port;
+	u16 dma_port_low;
+	u16 dma_port_high;
+};
+
+struct qfw_mmio_plat {
 	volatile struct qfw_mmio *mmio;
-	/* IO used on x86. */
-	struct {
-		u16 control_port;
-		u16 data_port;
-		u16 dma_port_low;
-		u16 dma_port_high;
-	} io;
 };
 
+struct qfw_dma {
+	__be32 control;
+	__be32 length;
+	__be64 address;
+};
+
+struct qfw_dev {
+	struct udevice *dev;
+	bool dma_present;
+	struct list_head fw_list;
+};
+
+struct dm_qfw_ops {
+	void (*read_entry_io)(struct udevice *dev, u16 entry, u32 size,
+			      void *address);
+	void (*read_entry_dma)(struct udevice *dev, struct qfw_dma *dma);
+};
+
+#define dm_qfw_get_ops(dev) \
+		((struct dm_qfw_ops *)(dev)->driver->ops)
+
+int qfw_register(struct udevice *dev);
+
 struct udevice;
 /**
  * Get QEMU firmware config device
  *
  * @return struct udevice * if present, NULL otherwise
  */
-struct udevice *qemu_fwcfg_dev(void);
+struct udevice *qfw_get_dev(void);
 
-void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
-			   void *address);
+void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address);
 int qemu_fwcfg_read_firmware_list(struct udevice *dev);
 struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name);
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 3/4] arm: x86: qemu: unify qfw driver functions as qfw_
  2021-02-23 11:43 [PATCH v3 0/4] Move qfw to DM, add Arm support Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include " Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio Asherah Connor
@ 2021-02-23 11:43 ` Asherah Connor
  2021-02-23 11:43 ` [PATCH v3 4/4] qemu: add sandbox driver and tests Asherah Connor
  3 siblings, 0 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 11:43 UTC (permalink / raw)
  To: u-boot

There's a mixture of "qemu_fwcfg_"-prefixed functions and
"qfw_"-prefixed ones.  Now that the qfw name is applied in multiple
places (i.e. the uclass itself, and each of its drivers), let's
consistently use the prefix "qfw_" for anything relating to the
drivers.

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---

(no changes since v1)

 arch/x86/cpu/qemu/cpu.c |  2 +-
 arch/x86/cpu/qfw_cpu.c  |  2 +-
 cmd/qfw.c               | 10 +++++-----
 common/qfw.c            | 14 +++++++-------
 drivers/misc/qfw.c      | 20 ++++++++++----------
 include/qfw.h           | 16 ++++++++--------
 6 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/arch/x86/cpu/qemu/cpu.c b/arch/x86/cpu/qemu/cpu.c
index 09499aad78..0a3962df41 100644
--- a/arch/x86/cpu/qemu/cpu.c
+++ b/arch/x86/cpu/qemu/cpu.c
@@ -27,7 +27,7 @@ static int cpu_qemu_get_count(const struct udevice *dev)
 	if (!qfw_dev)
 		return -ENODEV;
 
-	return qemu_fwcfg_online_cpus(qfw_dev);
+	return qfw_online_cpus(qfw_dev);
 }
 
 static const struct cpu_ops cpu_qemu_ops = {
diff --git a/arch/x86/cpu/qfw_cpu.c b/arch/x86/cpu/qfw_cpu.c
index e80cec0ca4..3b94686193 100644
--- a/arch/x86/cpu/qfw_cpu.c
+++ b/arch/x86/cpu/qfw_cpu.c
@@ -55,7 +55,7 @@ int qemu_cpu_fixup(void)
 	}
 
 	/* get actual cpu number */
-	cpu_online = qemu_fwcfg_online_cpus(qfwdev);
+	cpu_online = qfw_online_cpus(qfwdev);
 	if (cpu_online < 0) {
 		printf("unable to get online cpu number: %d\n", cpu_online);
 		return cpu_online;
diff --git a/cmd/qfw.c b/cmd/qfw.c
index a983a45380..e972b26d1e 100644
--- a/cmd/qfw.c
+++ b/cmd/qfw.c
@@ -82,13 +82,13 @@ static int qemu_fwcfg_cmd_list_firmware(void)
 	struct fw_file *file;
 
 	/* make sure fw_list is loaded */
-	ret = qemu_fwcfg_read_firmware_list(qfw_dev);
+	ret = qfw_read_firmware_list(qfw_dev);
 	if (ret)
 		return ret;
 
-	for (file = qemu_fwcfg_file_iter_init(qfw_dev, &iter);
-	     !qemu_fwcfg_file_iter_end(&iter);
-	     file = qemu_fwcfg_file_iter_next(&iter)) {
+	for (file = qfw_file_iter_init(qfw_dev, &iter);
+	     !qfw_file_iter_end(&iter);
+	     file = qfw_file_iter_next(&iter)) {
 		printf("%-56s\n", file->cfg.name);
 	}
 
@@ -107,7 +107,7 @@ static int qemu_fwcfg_do_list(struct cmd_tbl *cmdtp, int flag,
 static int qemu_fwcfg_do_cpus(struct cmd_tbl *cmdtp, int flag,
 			      int argc, char *const argv[])
 {
-	printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus(qfw_dev));
+	printf("%d cpu(s) online\n", qfw_online_cpus(qfw_dev));
 	return 0;
 }
 
diff --git a/common/qfw.c b/common/qfw.c
index b4c9e4483c..36b19bdc9b 100644
--- a/common/qfw.c
+++ b/common/qfw.c
@@ -20,7 +20,7 @@ struct udevice *qfw_get_dev(void)
 	return dev;
 }
 
-int qemu_fwcfg_online_cpus(struct udevice *dev)
+int qfw_online_cpus(struct udevice *dev)
 {
 	u16 nb_cpus;
 
@@ -29,7 +29,7 @@ int qemu_fwcfg_online_cpus(struct udevice *dev)
 	return le16_to_cpu(nb_cpus);
 }
 
-int qemu_fwcfg_read_firmware_list(struct udevice *dev)
+int qfw_read_firmware_list(struct udevice *dev)
 {
 	int i;
 	u32 count;
@@ -70,7 +70,7 @@ err:
 	return -ENOMEM;
 }
 
-struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
+struct fw_file *qfw_find_file(struct udevice *dev, const char *name)
 {
 	struct list_head *entry;
 	struct fw_file *file;
@@ -86,8 +86,8 @@ struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
 	return NULL;
 }
 
-struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
-					  struct fw_cfg_file_iter *iter)
+struct fw_file *qfw_file_iter_init(struct udevice *dev,
+				   struct fw_cfg_file_iter *iter)
 {
 	struct qfw_dev *qdev = dev_get_uclass_priv(dev);
 
@@ -97,14 +97,14 @@ struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
 			  struct fw_file, list);
 }
 
-struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter)
+struct fw_file *qfw_file_iter_next(struct fw_cfg_file_iter *iter)
 {
 	iter->entry = ((struct list_head *)iter->entry)->next;
 	return list_entry((struct list_head *)iter->entry,
 			  struct fw_file, list);
 }
 
-bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter)
+bool qfw_file_iter_end(struct fw_cfg_file_iter *iter)
 {
 	return iter->entry == iter->end;
 }
diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
index 25b203375b..4569f1886b 100644
--- a/drivers/misc/qfw.c
+++ b/drivers/misc/qfw.c
@@ -43,7 +43,7 @@ static int bios_linker_allocate(struct udevice *dev,
 		return -EINVAL;
 	}
 
-	file = qemu_fwcfg_find_file(dev, entry->alloc.file);
+	file = qfw_find_file(dev, entry->alloc.file);
 	if (!file) {
 		printf("error: can't find file %s\n", entry->alloc.file);
 		return -ENOENT;
@@ -99,10 +99,10 @@ static int bios_linker_add_pointer(struct udevice *dev,
 	uint32_t offset = le32_to_cpu(entry->pointer.offset);
 	uint64_t pointer = 0;
 
-	dest = qemu_fwcfg_find_file(dev, entry->pointer.dest_file);
+	dest = qfw_find_file(dev, entry->pointer.dest_file);
 	if (!dest || !dest->addr)
 		return -ENOENT;
-	src = qemu_fwcfg_find_file(dev, entry->pointer.src_file);
+	src = qfw_find_file(dev, entry->pointer.src_file);
 	if (!src || !src->addr)
 		return -ENOENT;
 
@@ -133,7 +133,7 @@ static int bios_linker_add_checksum(struct udevice *dev,
 	uint8_t *data, cksum = 0;
 	uint8_t *cksum_start;
 
-	file = qemu_fwcfg_find_file(dev, entry->cksum.file);
+	file = qfw_find_file(dev, entry->cksum.file);
 	if (!file || !file->addr)
 		return -ENOENT;
 
@@ -163,13 +163,13 @@ ulong write_acpi_tables(ulong addr)
 	}
 
 	/* make sure fw_list is loaded */
-	ret = qemu_fwcfg_read_firmware_list(dev);
+	ret = qfw_read_firmware_list(dev);
 	if (ret) {
 		printf("error: can't read firmware file list\n");
 		return addr;
 	}
 
-	file = qemu_fwcfg_find_file(dev, "etc/table-loader");
+	file = qfw_find_file(dev, "etc/table-loader");
 	if (!file) {
 		printf("error: can't find etc/table-loader\n");
 		return addr;
@@ -215,9 +215,9 @@ ulong write_acpi_tables(ulong addr)
 out:
 	if (ret) {
 		struct fw_cfg_file_iter iter;
-		for (file = qemu_fwcfg_file_iter_init(dev, &iter);
-		     !qemu_fwcfg_file_iter_end(&iter);
-		     file = qemu_fwcfg_file_iter_next(&iter)) {
+		for (file = qfw_file_iter_init(dev, &iter);
+		     !qfw_file_iter_end(&iter);
+		     file = qfw_file_iter_next(&iter)) {
 			if (file->addr) {
 				free((void *)file->addr);
 				file->addr = 0;
@@ -240,7 +240,7 @@ ulong acpi_get_rsdp_addr(void)
 		return 0;
 	}
 
-	file = qemu_fwcfg_find_file(dev, "etc/acpi/rsdp");
+	file = qfw_find_file(dev, "etc/acpi/rsdp");
 	return file->addr;
 }
 #endif
diff --git a/include/qfw.h b/include/qfw.h
index d7e16651a2..52ca1533f5 100644
--- a/include/qfw.h
+++ b/include/qfw.h
@@ -8,7 +8,7 @@
 
 #include <linux/list.h>
 
-enum qemu_fwcfg_items {
+enum {
 	FW_CFG_SIGNATURE	= 0x00,
 	FW_CFG_ID		= 0x01,
 	FW_CFG_UUID		= 0x02,
@@ -192,21 +192,21 @@ struct udevice;
 struct udevice *qfw_get_dev(void);
 
 void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address);
-int qemu_fwcfg_read_firmware_list(struct udevice *dev);
-struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name);
+int qfw_read_firmware_list(struct udevice *dev);
+struct fw_file *qfw_find_file(struct udevice *dev, const char *name);
 
 /**
  * Get system cpu number
  *
  * @return:   cpu number in system
  */
-int qemu_fwcfg_online_cpus(struct udevice *dev);
+int qfw_online_cpus(struct udevice *dev);
 
 /* helper functions to iterate firmware file list */
-struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
-					  struct fw_cfg_file_iter *iter);
-struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter);
-bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter);
+struct fw_file *qfw_file_iter_init(struct udevice *dev,
+				   struct fw_cfg_file_iter *iter);
+struct fw_file *qfw_file_iter_next(struct fw_cfg_file_iter *iter);
+bool qfw_file_iter_end(struct fw_cfg_file_iter *iter);
 
 /**
  * qemu_cpu_fixup() - Fix up the CPUs for QEMU
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 4/4] qemu: add sandbox driver and tests
  2021-02-23 11:43 [PATCH v3 0/4] Move qfw to DM, add Arm support Asherah Connor
                   ` (2 preceding siblings ...)
  2021-02-23 11:43 ` [PATCH v3 3/4] arm: x86: qemu: unify qfw driver functions as qfw_ Asherah Connor
@ 2021-02-23 11:43 ` Asherah Connor
  2021-02-23 12:26   ` Asherah Connor
  2021-02-23 15:58   ` Simon Glass
  3 siblings, 2 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 11:43 UTC (permalink / raw)
  To: u-boot

We minimally exercise the sandbox driver.

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---

(no changes since v1)

 arch/sandbox/dts/sandbox.dtsi |   4 ++
 arch/sandbox/dts/test.dts     |   4 ++
 drivers/misc/Makefile         |  11 ++-
 drivers/misc/qfw_sandbox.c    | 129 ++++++++++++++++++++++++++++++++++
 test/dm/Makefile              |   1 +
 test/dm/qfw.c                 |  42 +++++++++++
 6 files changed, 185 insertions(+), 6 deletions(-)
 create mode 100644 drivers/misc/qfw_sandbox.c
 create mode 100644 test/dm/qfw.c

diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi
index dc933f3bfc..7ce05b9662 100644
--- a/arch/sandbox/dts/sandbox.dtsi
+++ b/arch/sandbox/dts/sandbox.dtsi
@@ -390,6 +390,10 @@
 	sandbox_tee {
 		compatible = "sandbox,tee";
 	};
+
+	qfw {
+		compatible = "sandbox,qemu-fw-cfg-mmio";
+	};
 };
 
 &cros_ec {
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index d4195b45bb..4b3f8831d5 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -112,6 +112,10 @@
 		compatible = "sandbox,dsi-host";
 	};
 
+	qfw {
+		compatible = "sandbox,qemu-fw-cfg-mmio";
+	};
+
 	a-test {
 		reg = <0 1>;
 		compatible = "denx,u-boot-fdt-test";
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e6e1dfea95..ea04abd6c5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -55,12 +55,11 @@ obj-$(CONFIG_NUVOTON_NCT6102D) += nuvoton_nct6102d.o
 obj-$(CONFIG_P2SB) += p2sb-uclass.o
 obj-$(CONFIG_PCA9551_LED) += pca9551_led.o
 obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o
-obj-$(CONFIG_QFW) += qfw.o
-ifdef CONFIG_X86
-obj-$(CONFIG_QFW) += qfw_pio.o
-endif
-ifdef CONFIG_ARM
-obj-$(CONFIG_QFW) += qfw_mmio.o
+ifdef CONFIG_QFW
+obj-y	+= qfw.o
+obj-$(CONFIG_X86) += qfw_pio.o
+obj-$(CONFIG_ARM) += qfw_mmio.o
+obj-$(CONFIG_SANDBOX) += qfw_sandbox.o
 endif
 obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
 obj-$(CONFIG_ROCKCHIP_OTP) += rockchip-otp.o
diff --git a/drivers/misc/qfw_sandbox.c b/drivers/misc/qfw_sandbox.c
new file mode 100644
index 0000000000..fc7006ae19
--- /dev/null
+++ b/drivers/misc/qfw_sandbox.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Sandbox interface for QFW
+ *
+ * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
+ * (C) Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
+ */
+
+#define LOG_CATEGORY UCLASS_QFW
+
+#include <asm/types.h>
+#include <asm/io.h>
+#include <dm.h>
+#include <dm/device.h>
+#include <qfw.h>
+
+struct qfw_sandbox_plat {
+	u8 file_dir_offset;
+};
+
+static void qfw_sandbox_read_entry_io(struct udevice *dev, u16 entry, u32 size,
+				      void *address)
+{
+	debug("%s: entry 0x%x size %u address %p\n", __func__, entry, size,
+	      address);
+
+	switch (entry) {
+	case FW_CFG_SIGNATURE:
+		if (size == 4)
+			*((u32 *)address) = cpu_to_be32(QEMU_FW_CFG_SIGNATURE);
+		break;
+	case FW_CFG_ID:
+		/* Advertise DMA support */
+		if (size == 1)
+			*((u8 *)address) = FW_CFG_DMA_ENABLED;
+		break;
+	default:
+		debug("%s got unsupported entry 0x%x\n", __func__, entry);
+	/*
+	 * Sandbox driver doesn't support other entries here, assume we use DMA
+	 * to read them -- the uclass driver will exclusively use it when
+	 * advertised.
+	 */
+	}
+}
+
+static void qfw_sandbox_read_entry_dma(struct udevice *dev, struct qfw_dma *dma)
+{
+	u16 entry;
+	u32 control = be32_to_cpu(dma->control);
+	void *address = (void *)be64_to_cpu(dma->address);
+	u32 length = be32_to_cpu(dma->length);
+	struct qfw_sandbox_plat *plat = dev_get_plat(dev);
+	struct fw_cfg_file *file;
+
+	debug("%s\n", __func__);
+
+	if (!(control & FW_CFG_DMA_READ))
+		return;
+
+	if (control & FW_CFG_DMA_SELECT) {
+		/* Start new read. */
+		entry = control >> 16;
+
+		/* Arbitrary values to be used by tests. */
+		switch (entry) {
+		case FW_CFG_NB_CPUS:
+			if (length == 2)
+				*((u16 *)address) = cpu_to_le16(5);
+			break;
+		case FW_CFG_FILE_DIR:
+			if (length == 4) {
+				*((u32 *)address) = cpu_to_be32(2);
+				plat->file_dir_offset = 1;
+			}
+			break;
+		default:
+			debug("%s got unsupported entry 0x%x\n", __func__,
+			      entry);
+		}
+	} else if (plat->file_dir_offset && length == 64) {
+		file = address;
+		switch (plat->file_dir_offset) {
+		case 1:
+			file->size = cpu_to_be32(8);
+			file->select = cpu_to_be16(FW_CFG_FILE_FIRST);
+			strcpy(file->name, "test-one");
+			plat->file_dir_offset++;
+			break;
+		case 2:
+			file->size = cpu_to_be32(8);
+			file->select = cpu_to_be16(FW_CFG_FILE_FIRST + 1);
+			strcpy(file->name, "test-two");
+			plat->file_dir_offset++;
+			break;
+		}
+	}
+
+	/*
+	 * Signal that we are finished. No-one checks this in sandbox --
+	 * normally the platform-specific driver looks for it -- but let's
+	 * replicate the behaviour in case someone relies on it later.
+	 */
+	dma->control = 0;
+}
+
+static int qfw_sandbox_probe(struct udevice *dev)
+{
+	return qfw_register(dev);
+}
+
+static struct dm_qfw_ops qfw_sandbox_ops = {
+	.read_entry_io = qfw_sandbox_read_entry_io,
+	.read_entry_dma = qfw_sandbox_read_entry_dma,
+};
+
+static const struct udevice_id qfw_sandbox_ids[] = {
+	{ .compatible = "sandbox,qemu-fw-cfg-mmio" },
+	{}
+};
+
+U_BOOT_DRIVER(qfw_sandbox) = {
+	.name	= "qfw_sandbox",
+	.id	= UCLASS_QFW,
+	.of_match	= qfw_sandbox_ids,
+	.plat_auto	= sizeof(struct qfw_sandbox_plat),
+	.probe	= qfw_sandbox_probe,
+	.ops	= &qfw_sandbox_ops,
+};
diff --git a/test/dm/Makefile b/test/dm/Makefile
index 6275ec56ea..bdca7ae613 100644
--- a/test/dm/Makefile
+++ b/test/dm/Makefile
@@ -95,5 +95,6 @@ obj-$(CONFIG_SCMI_FIRMWARE) += scmi.o
 ifneq ($(CONFIG_PINMUX),)
 obj-$(CONFIG_PINCONF) += pinmux.o
 endif
+obj-$(CONFIG_QFW) += qfw.o
 endif
 endif # !SPL
diff --git a/test/dm/qfw.c b/test/dm/qfw.c
new file mode 100644
index 0000000000..51b1c42dda
--- /dev/null
+++ b/test/dm/qfw.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 Asherah Connor <ashe@kivikakk.ee>
+ */
+
+#include <common.h>
+#include <qfw.h>
+#include <dm.h>
+#include <asm/test.h>
+#include <dm/test.h>
+#include <test/ut.h>
+
+/*
+ * Exercise the device enough to be satisfied the initialisation and DMA
+ * interfaces work.
+ */
+
+static int dm_test_qfw_cpus(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+
+	ut_assertok(uclass_first_device_err(UCLASS_QFW, &dev));
+	ut_asserteq(5, qfw_online_cpus(dev));
+
+	return 0;
+}
+
+DM_TEST(dm_test_qfw_cpus, UT_TESTF_SCAN_FDT);
+
+static int dm_test_qfw_firmware_list(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	struct fw_file *file;
+
+	ut_assertok(uclass_first_device_err(UCLASS_QFW, &dev));
+	ut_assertok(qfw_read_firmware_list(dev));
+	ut_assertok_ptr((file = qfw_find_file(dev, "test-one")));
+
+	return 0;
+}
+
+DM_TEST(dm_test_qfw_firmware_list, UT_TESTF_SCAN_FDT);
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 4/4] qemu: add sandbox driver and tests
  2021-02-23 11:43 ` [PATCH v3 4/4] qemu: add sandbox driver and tests Asherah Connor
@ 2021-02-23 12:26   ` Asherah Connor
  2021-02-23 15:58   ` Simon Glass
  1 sibling, 0 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 12:26 UTC (permalink / raw)
  To: u-boot

On 21/02/23 10:02:p, Asherah Connor wrote:
> We minimally exercise the sandbox driver.

It looks like the test in QEMU is pretty trivial.  I'll include it in
the next series version, but here's what it looks like.  New tests have
been tested as passing against qemu_arm, qemu_arm64, qemu-x86, and
qemu-x86_64.  :)

Best,

Asherah

--8<--

Subject: [PATCH] test: qemu: add simple test for cmd_qfw

Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
---
 test/py/tests/test_qfw.py | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 test/py/tests/test_qfw.py

diff --git a/test/py/tests/test_qfw.py b/test/py/tests/test_qfw.py
new file mode 100644
index 0000000000..a2631a0fa6
--- /dev/null
+++ b/test/py/tests/test_qfw.py
@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2021, Asherah Connor <ashe@kivikakk.ee>
+
+# Test qfw command implementation
+
+import pytest
+
+ at pytest.mark.buildconfigspec('cmd_qfw')
+def test_qfw_cpus(u_boot_console):
+    "Test QEMU firmware config reports the CPU count correctly."
+
+    output = u_boot_console.run_command('qfw cpus')
+    assert '1 cpu(s) online' in output
+
+ at pytest.mark.buildconfigspec('cmd_qfw')
+def test_qfw_list(u_boot_console):
+    "Test QEMU firmware config lists devices."
+
+    output = u_boot_console.run_command('qfw list')
+    assert 'bootorder' in output
+    assert 'etc/table-loader' in output
--
2.20.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 11:43 ` [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include " Asherah Connor
@ 2021-02-23 12:59   ` Heinrich Schuchardt
  2021-02-23 14:53     ` Tom Rini
  2021-02-24  0:22     ` Asherah Connor
  0 siblings, 2 replies; 18+ messages in thread
From: Heinrich Schuchardt @ 2021-02-23 12:59 UTC (permalink / raw)
  To: u-boot

On 23.02.21 12:43, Asherah Connor wrote:
> Updates the QFW driver to use the driver model, and adds support for QFW
> on Arm platforms by configuring from the device tree and using MMIO
> accordingly.  A sandbox driver for QFW is also included, and a simple DM
> unit test for it.

For which architectures does the fw_cfg device exist?

It it is only ARM and X86, than I am missing such a dependency on
CONFIG_CMD_QFW.

>
> Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
> ---
>
> Changes in v3:
> - ARCH_QEMU now implies CMD_QFW, not QFW
> - rename qemu_fwcfg_read_entry_pio to ..._io
>
>  arch/arm/Kconfig         |   1 +
>  arch/x86/cpu/qemu/cpu.c  |   7 +-
>  arch/x86/cpu/qemu/qemu.c |  54 ++------
>  arch/x86/cpu/qfw_cpu.c   |  11 +-
>  cmd/qfw.c                |  44 +++---
>  drivers/misc/Kconfig     |   1 +
>  drivers/misc/qfw.c       | 289 +++++++++++++++++++++++++++------------
>  include/qfw.h            |  63 +++++----
>  8 files changed, 292 insertions(+), 178 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d51abbeaf0..cd01dc458a 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -937,6 +937,7 @@ config ARCH_QEMU
>  	imply DM_RNG
>  	imply DM_RTC
>  	imply RTC_PL031
> +	imply CMD_QFW
>
>  config ARCH_RMOBILE
>  	bool "Renesas ARM SoCs"
> diff --git a/arch/x86/cpu/qemu/cpu.c b/arch/x86/cpu/qemu/cpu.c
> index 9ce86b379c..ab1b797f9a 100644
> --- a/arch/x86/cpu/qemu/cpu.c
> +++ b/arch/x86/cpu/qemu/cpu.c
> @@ -22,7 +22,12 @@ int cpu_qemu_get_desc(const struct udevice *dev, char *buf, int size)
>
>  static int cpu_qemu_get_count(const struct udevice *dev)
>  {
> -	return qemu_fwcfg_online_cpus();
> +	struct udevice *qfw_dev = qemu_fwcfg_dev();
> +
> +	if (!qfw_dev)
> +		return -ENODEV;
> +
> +	return qemu_fwcfg_online_cpus(qfw_dev);
>  }
>
>  static const struct cpu_ops cpu_qemu_ops = {
> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
> index 044a429c13..e255af9a4a 100644
> --- a/arch/x86/cpu/qemu/qemu.c
> +++ b/arch/x86/cpu/qemu/qemu.c
> @@ -8,6 +8,7 @@
>  #include <init.h>
>  #include <pci.h>
>  #include <qfw.h>
> +#include <dm/platdata.h>
>  #include <asm/irq.h>
>  #include <asm/post.h>
>  #include <asm/processor.h>
> @@ -19,45 +20,20 @@ static bool i440fx;
>  #ifdef CONFIG_QFW
>
>  /* on x86, the qfw registers are all IO ports */
> -#define FW_CONTROL_PORT	0x510
> -#define FW_DATA_PORT		0x511
> -#define FW_DMA_PORT_LOW	0x514
> -#define FW_DMA_PORT_HIGH	0x518
> -
> -static void qemu_x86_fwcfg_read_entry_pio(uint16_t entry,
> -		uint32_t size, void *address)
> -{
> -	uint32_t i = 0;
> -	uint8_t *data = address;
> -
> -	/*
> -	 * writting FW_CFG_INVALID will cause read operation to resume at
> -	 * last offset, otherwise read will start at offset 0
> -	 *
> -	 * Note: on platform where the control register is IO port, the
> -	 * endianness is little endian.
> -	 */
> -	if (entry != FW_CFG_INVALID)
> -		outw(cpu_to_le16(entry), FW_CONTROL_PORT);
> -
> -	/* the endianness of data register is string-preserving */
> -	while (size--)
> -		data[i++] = inb(FW_DATA_PORT);
> -}
> -
> -static void qemu_x86_fwcfg_read_entry_dma(struct fw_cfg_dma_access *dma)
> -{
> -	/* the DMA address register is big endian */
> -	outl(cpu_to_be32((uintptr_t)dma), FW_DMA_PORT_HIGH);
> -
> -	while (be32_to_cpu(dma->control) & ~FW_CFG_DMA_ERROR)
> -		__asm__ __volatile__ ("pause");
> -}
> +static const struct qfw_plat x86_qfw_plat = {
> +	.io = {
> +		.control_port	= 0x510,
> +		.data_port	= 0x511,
> +		.dma_port_low	= 0x514,
> +		.dma_port_high	= 0x518,
> +	},
> +};

If these numbers are constants, why should they be copied to platform
data? This only increases code size.

I think there is nothing wrong with using constants here.


>
> -static struct fw_cfg_arch_ops fwcfg_x86_ops = {
> -	.arch_read_pio = qemu_x86_fwcfg_read_entry_pio,
> -	.arch_read_dma = qemu_x86_fwcfg_read_entry_dma
> +U_BOOT_DRVINFO(x86_qfw) = {
> +	.name = "qfw",
> +	.plat = &x86_qfw_plat,
>  };
> +
>  #endif
>
>  static void enable_pm_piix(void)
> @@ -132,10 +108,6 @@ static void qemu_chipset_init(void)
>
>  		enable_pm_ich9();
>  	}
> -
> -#ifdef CONFIG_QFW
> -	qemu_fwcfg_init(&fwcfg_x86_ops);
> -#endif
>  }
>
>  #if !CONFIG_IS_ENABLED(SPL_X86_32BIT_INIT)
> diff --git a/arch/x86/cpu/qfw_cpu.c b/arch/x86/cpu/qfw_cpu.c
> index b959eaddde..c8fb918494 100644
> --- a/arch/x86/cpu/qfw_cpu.c
> +++ b/arch/x86/cpu/qfw_cpu.c
> @@ -18,7 +18,7 @@ int qemu_cpu_fixup(void)
>  	int cpu_num;
>  	int cpu_online;
>  	struct uclass *uc;
> -	struct udevice *dev, *pdev;
> +	struct udevice *dev, *pdev, *qfwdev;
>  	struct cpu_plat *plat;
>  	char *cpu;
>
> @@ -39,6 +39,13 @@ int qemu_cpu_fixup(void)
>  		return -ENODEV;
>  	}
>
> +	/* get qfw dev */
> +	qfwdev = qemu_fwcfg_dev();
> +	if (!qfwdev) {
> +		printf("unable to find qfw device\n");
> +		return -ENODEV;
> +	}
> +
>  	/* calculate cpus that are already bound */
>  	cpu_num = 0;
>  	for (uclass_find_first_device(UCLASS_CPU, &dev);
> @@ -48,7 +55,7 @@ int qemu_cpu_fixup(void)
>  	}
>
>  	/* get actual cpu number */
> -	cpu_online = qemu_fwcfg_online_cpus();
> +	cpu_online = qemu_fwcfg_online_cpus(qfwdev);
>  	if (cpu_online < 0) {
>  		printf("unable to get online cpu number: %d\n", cpu_online);
>  		return cpu_online;
> diff --git a/cmd/qfw.c b/cmd/qfw.c
> index bb571487f0..ec80a9a3b5 100644
> --- a/cmd/qfw.c
> +++ b/cmd/qfw.c
> @@ -8,19 +8,22 @@
>  #include <env.h>
>  #include <errno.h>
>  #include <qfw.h>
> +#include <dm.h>
> +
> +static struct udevice *qfw_dev;
>
>  /*
>   * This function prepares kernel for zboot. It loads kernel data
>   * to 'load_addr', initrd to 'initrd_addr' and kernel command
>   * line using qemu fw_cfg interface.
>   */
> -static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
> +static int qemu_fwcfg_cmd_setup_kernel(void *load_addr, void *initrd_addr)
>  {
>  	char *data_addr;
>  	uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
>
> -	qemu_fwcfg_read_entry(FW_CFG_SETUP_SIZE, 4, &setup_size);
> -	qemu_fwcfg_read_entry(FW_CFG_KERNEL_SIZE, 4, &kernel_size);
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_SIZE, 4, &setup_size);
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_SIZE, 4, &kernel_size);
>
>  	if (setup_size == 0 || kernel_size == 0) {
>  		printf("warning: no kernel available\n");
> @@ -28,27 +31,27 @@ static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
>  	}
>
>  	data_addr = load_addr;
> -	qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA,
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_SETUP_DATA,
>  			      le32_to_cpu(setup_size), data_addr);
>  	data_addr += le32_to_cpu(setup_size);
>
> -	qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA,
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_KERNEL_DATA,
>  			      le32_to_cpu(kernel_size), data_addr);
>  	data_addr += le32_to_cpu(kernel_size);
>
>  	data_addr = initrd_addr;
> -	qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_SIZE, 4, &initrd_size);
>  	if (initrd_size == 0) {
>  		printf("warning: no initrd available\n");
>  	} else {
> -		qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA,
> +		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_INITRD_DATA,
>  				      le32_to_cpu(initrd_size), data_addr);
>  		data_addr += le32_to_cpu(initrd_size);
>  	}
>
> -	qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
> +	qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
>  	if (cmdline_size) {
> -		qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA,
> +		qemu_fwcfg_read_entry(qfw_dev, FW_CFG_CMDLINE_DATA,
>  				      le32_to_cpu(cmdline_size), data_addr);
>  		/*
>  		 * if kernel cmdline only contains '\0', (e.g. no -append
> @@ -72,19 +75,18 @@ static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
>  	return 0;
>  }
>
> -static int qemu_fwcfg_list_firmware(void)
> +static int qemu_fwcfg_cmd_list_firmware(void)
>  {
>  	int ret;
>  	struct fw_cfg_file_iter iter;
>  	struct fw_file *file;
>
>  	/* make sure fw_list is loaded */
> -	ret = qemu_fwcfg_read_firmware_list();
> +	ret = qemu_fwcfg_read_firmware_list(qfw_dev);
>  	if (ret)
>  		return ret;
>
> -
> -	for (file = qemu_fwcfg_file_iter_init(&iter);
> +	for (file = qemu_fwcfg_file_iter_init(qfw_dev, &iter);
>  	     !qemu_fwcfg_file_iter_end(&iter);
>  	     file = qemu_fwcfg_file_iter_next(&iter)) {
>  		printf("%-56s\n", file->cfg.name);
> @@ -96,7 +98,7 @@ static int qemu_fwcfg_list_firmware(void)
>  static int qemu_fwcfg_do_list(struct cmd_tbl *cmdtp, int flag,
>  			      int argc, char *const argv[])
>  {
> -	if (qemu_fwcfg_list_firmware() < 0)
> +	if (qemu_fwcfg_cmd_list_firmware() < 0)
>  		return CMD_RET_FAILURE;
>
>  	return 0;
> @@ -105,14 +107,7 @@ static int qemu_fwcfg_do_list(struct cmd_tbl *cmdtp, int flag,
>  static int qemu_fwcfg_do_cpus(struct cmd_tbl *cmdtp, int flag,
>  			      int argc, char *const argv[])
>  {
> -	int ret = qemu_fwcfg_online_cpus();
> -	if (ret < 0) {
> -		printf("QEMU fw_cfg interface not found\n");
> -		return CMD_RET_FAILURE;
> -	}
> -
> -	printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus());
> -
> +	printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus(qfw_dev));
>  	return 0;
>  }
>
> @@ -153,7 +148,7 @@ static int qemu_fwcfg_do_load(struct cmd_tbl *cmdtp, int flag,
>  		return CMD_RET_FAILURE;
>  	}
>
> -	return qemu_fwcfg_setup_kernel(load_addr, initrd_addr);
> +	return qemu_fwcfg_cmd_setup_kernel(load_addr, initrd_addr);
>  }
>
>  static struct cmd_tbl fwcfg_commands[] = {
> @@ -168,7 +163,8 @@ static int do_qemu_fw(struct cmd_tbl *cmdtp, int flag, int argc,
>  	int ret;
>  	struct cmd_tbl *fwcfg_cmd;
>
> -	if (!qemu_fwcfg_present()) {
> +	qfw_dev = qemu_fwcfg_dev();
> +	if (!qfw_dev) {
>  		printf("QEMU fw_cfg interface not found\n");
>  		return CMD_RET_USAGE;
>  	}
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 7d2a299779..0a65f29acd 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -367,6 +367,7 @@ config WINBOND_W83627
>
>  config QFW
>  	bool
> +	depends on MISC
>  	help
>  	  Hidden option to enable QEMU fw_cfg interface. This will be selected by
>  	  either CONFIG_CMD_QFW or CONFIG_GENERATE_ACPI_TABLE.
> diff --git a/drivers/misc/qfw.c b/drivers/misc/qfw.c
> index f6eb6583ed..eae3afd23b 100644
> --- a/drivers/misc/qfw.c
> +++ b/drivers/misc/qfw.c
> @@ -9,17 +9,18 @@
>  #include <log.h>
>  #include <malloc.h>
>  #include <qfw.h>
> +#include <dm.h>
>  #include <asm/io.h>
> +#include <misc.h>
>  #ifdef CONFIG_GENERATE_ACPI_TABLE
>  #include <asm/tables.h>
>  #endif
> -#include <linux/list.h>
>
> -static bool fwcfg_present;
> -static bool fwcfg_dma_present;
> -static struct fw_cfg_arch_ops *fwcfg_arch_ops;
> -
> -static LIST_HEAD(fw_list);
> +/* Determined at runtime. */
> +struct qfw_priv {
> +	bool dma_present;
> +	struct list_head fw_list;
> +};
>
>  #ifdef CONFIG_GENERATE_ACPI_TABLE
>  /*
> @@ -32,7 +33,8 @@ static LIST_HEAD(fw_list);
>   *          be ignored.
>   * @return: 0 on success, or negative value on failure
>   */
> -static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
> +static int bios_linker_allocate(struct udevice *dev,
> +				struct bios_linker_entry *entry, ulong *addr)
>  {
>  	uint32_t size, align;
>  	struct fw_file *file;
> @@ -45,7 +47,7 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
>  		return -EINVAL;
>  	}
>
> -	file = qemu_fwcfg_find_file(entry->alloc.file);
> +	file = qemu_fwcfg_find_file(dev, entry->alloc.file);
>  	if (!file) {
>  		printf("error: can't find file %s\n", entry->alloc.file);
>  		return -ENOENT;
> @@ -75,7 +77,7 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
>  	debug("bios_linker_allocate: allocate file %s, size %u, zone %d, align %u, addr 0x%lx\n",
>  	      file->cfg.name, size, entry->alloc.zone, align, aligned_addr);
>
> -	qemu_fwcfg_read_entry(be16_to_cpu(file->cfg.select),
> +	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
>  			      size, (void *)aligned_addr);
>  	file->addr = aligned_addr;
>
> @@ -94,16 +96,17 @@ static int bios_linker_allocate(struct bios_linker_entry *entry, ulong *addr)
>   *          ACPI tables
>   * @return: 0 on success, or negative value on failure
>   */
> -static int bios_linker_add_pointer(struct bios_linker_entry *entry)
> +static int bios_linker_add_pointer(struct udevice *dev,
> +				   struct bios_linker_entry *entry)
>  {
>  	struct fw_file *dest, *src;
>  	uint32_t offset = le32_to_cpu(entry->pointer.offset);
>  	uint64_t pointer = 0;
>
> -	dest = qemu_fwcfg_find_file(entry->pointer.dest_file);
> +	dest = qemu_fwcfg_find_file(dev, entry->pointer.dest_file);
>  	if (!dest || !dest->addr)
>  		return -ENOENT;
> -	src = qemu_fwcfg_find_file(entry->pointer.src_file);
> +	src = qemu_fwcfg_find_file(dev, entry->pointer.src_file);
>  	if (!src || !src->addr)
>  		return -ENOENT;
>
> @@ -127,13 +130,14 @@ static int bios_linker_add_pointer(struct bios_linker_entry *entry)
>   *          checksums
>   * @return: 0 on success, or negative value on failure
>   */
> -static int bios_linker_add_checksum(struct bios_linker_entry *entry)
> +static int bios_linker_add_checksum(struct udevice *dev,
> +				    struct bios_linker_entry *entry)
>  {
>  	struct fw_file *file;
>  	uint8_t *data, cksum = 0;
>  	uint8_t *cksum_start;
>
> -	file = qemu_fwcfg_find_file(entry->cksum.file);
> +	file = qemu_fwcfg_find_file(dev, entry->cksum.file);
>  	if (!file || !file->addr)
>  		return -ENOENT;
>
> @@ -154,15 +158,22 @@ ulong write_acpi_tables(ulong addr)
>  	struct bios_linker_entry *table_loader;
>  	struct bios_linker_entry *entry;
>  	uint32_t size;
> +	struct udevice *dev;
> +
> +	dev = qemu_fwcfg_dev();
> +	if (!dev) {
> +		printf("error: no qfw\n");
> +		return addr;
> +	}
>
>  	/* make sure fw_list is loaded */
> -	ret = qemu_fwcfg_read_firmware_list();
> +	ret = qemu_fwcfg_read_firmware_list(dev);
>  	if (ret) {
>  		printf("error: can't read firmware file list\n");
>  		return addr;
>  	}
>
> -	file = qemu_fwcfg_find_file("etc/table-loader");
> +	file = qemu_fwcfg_find_file(dev, "etc/table-loader");
>  	if (!file) {
>  		printf("error: can't find etc/table-loader\n");
>  		return addr;
> @@ -180,24 +191,24 @@ ulong write_acpi_tables(ulong addr)
>  		return addr;
>  	}
>
> -	qemu_fwcfg_read_entry(be16_to_cpu(file->cfg.select),
> +	qemu_fwcfg_read_entry(dev, be16_to_cpu(file->cfg.select),
>  			      size, table_loader);
>
>  	for (i = 0; i < (size / sizeof(*entry)); i++) {
>  		entry = table_loader + i;
>  		switch (le32_to_cpu(entry->command)) {
>  		case BIOS_LINKER_LOADER_COMMAND_ALLOCATE:
> -			ret = bios_linker_allocate(entry, &addr);
> +			ret = bios_linker_allocate(dev, entry, &addr);
>  			if (ret)
>  				goto out;
>  			break;
>  		case BIOS_LINKER_LOADER_COMMAND_ADD_POINTER:
> -			ret = bios_linker_add_pointer(entry);
> +			ret = bios_linker_add_pointer(dev, entry);
>  			if (ret)
>  				goto out;
>  			break;
>  		case BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM:
> -			ret = bios_linker_add_checksum(entry);
> +			ret = bios_linker_add_checksum(dev, entry);
>  			if (ret)
>  				goto out;
>  			break;
> @@ -209,7 +220,7 @@ ulong write_acpi_tables(ulong addr)
>  out:
>  	if (ret) {
>  		struct fw_cfg_file_iter iter;
> -		for (file = qemu_fwcfg_file_iter_init(&iter);
> +		for (file = qemu_fwcfg_file_iter_init(dev, &iter);
>  		     !qemu_fwcfg_file_iter_end(&iter);
>  		     file = qemu_fwcfg_file_iter_next(&iter)) {
>  			if (file->addr) {
> @@ -226,31 +237,91 @@ out:
>  ulong acpi_get_rsdp_addr(void)
>  {
>  	struct fw_file *file;
> +	struct udevice *dev;
>
> -	file = qemu_fwcfg_find_file("etc/acpi/rsdp");
> +	dev = qemu_fwcfg_dev();
> +	if (!dev) {
> +		printf("error: no qfw\n");
> +		return 0;
> +	}
> +
> +	file = qemu_fwcfg_find_file(dev, "etc/acpi/rsdp");
>  	return file->addr;
>  }
>  #endif
>
> -/* Read configuration item using fw_cfg PIO interface */
> -static void qemu_fwcfg_read_entry_pio(uint16_t entry,
> -		uint32_t size, void *address)
> +/* Read configuration item using fw_cfg PIO/MMIO interface */
> +static void qemu_fwcfg_read_entry_io(struct qfw_plat *plat, u16 entry,
> +				     u32 size, void *address)
>  {
> -	debug("qemu_fwcfg_read_entry_pio: entry 0x%x, size %u address %p\n",
> +	debug("qemu_fwcfg_read_entry_io: entry 0x%x, size %u address %p\n",
>  	      entry, size, address);
>
> -	return fwcfg_arch_ops->arch_read_pio(entry, size, address);
> +	/*
> +	 * writing FW_CFG_INVALID will cause read operation to resume at last
> +	 * offset, otherwise read will start at offset 0
> +	 *
> +	 * Note: on platform where the control register is IO port, the
> +	 * endianness is little endian.  Where it is on MMIO, the register is
> +	 * big endian.
> +	 */
> +	if (entry != FW_CFG_INVALID) {
> +		if (plat->mmio)
> +			plat->mmio->selector = cpu_to_be16(entry);
> +#ifdef CONFIG_X86
> +		else
> +			outw(cpu_to_le16(entry), plat->io.control_port);
> +#endif
> +	}
> +
> +	/* the endianness of data register is string-preserving */
> +
> +	if (plat->mmio) {
> +		while (size >= 8) {
> +			*(u64 *)address = plat->mmio->data64;
> +			address += 8;
> +			size -= 8;
> +		}
> +		while (size >= 4) {
> +			*(u32 *)address = plat->mmio->data32;
> +			address += 4;
> +			size -= 4;
> +		}
> +		while (size >= 2) {
> +			*(u16 *)address = plat->mmio->data16;
> +			address += 2;
> +			size -= 2;
> +		}
> +		while (size >= 1) {
> +			*(u8 *)address = plat->mmio->data8;
> +			address += 1;
> +			size -= 1;
> +		}
> +	}
> +#ifdef CONFIG_X86
> +	else {
> +		u32 i = 0;
> +		u8 *data = address;
> +
> +		while (size--)
> +			data[i++] = inb(plat->io.data_port);
> +	}
> +#endif
>  }
>
>  /* Read configuration item using fw_cfg DMA interface */
> -static void qemu_fwcfg_read_entry_dma(uint16_t entry,
> -		uint32_t size, void *address)
> +static void qemu_fwcfg_read_entry_dma(struct qfw_plat *plat, u16 entry,
> +				      u32 size, void *address)
>  {
> -	struct fw_cfg_dma_access dma;
> -
> -	dma.length = cpu_to_be32(size);
> -	dma.address = cpu_to_be64((uintptr_t)address);
> -	dma.control = cpu_to_be32(FW_CFG_DMA_READ);
> +	struct {
> +		__be32 control;
> +		__be32 length;
> +		__be64 address;
> +	} dma = {
> +		.length = cpu_to_be32(size),
> +		.address = cpu_to_be64((uintptr_t)address),
> +		.control = cpu_to_be32(FW_CFG_DMA_READ),
> +	};
>
>  	/*
>  	 * writting FW_CFG_INVALID will cause read operation to resume at
> @@ -264,51 +335,58 @@ static void qemu_fwcfg_read_entry_dma(uint16_t entry,
>  	debug("qemu_fwcfg_read_entry_dma: entry 0x%x, size %u address %p, control 0x%x\n",
>  	      entry, size, address, be32_to_cpu(dma.control));
>
> -	fwcfg_arch_ops->arch_read_dma(&dma);
> -}
> +	/* the DMA address register is big-endian */
> +	if (plat->mmio)
> +		plat->mmio->dma = cpu_to_be64((uintptr_t)&dma);
> +#ifdef CONFIG_X86
> +	else
> +		outl(cpu_to_be32((uintptr_t)&dma), plat->io.dma_port_high);
> +#endif
>
> -bool qemu_fwcfg_present(void)
> -{
> -	return fwcfg_present;
> -}
>
> -bool qemu_fwcfg_dma_present(void)
> -{
> -	return fwcfg_dma_present;
> +	while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
> +#ifdef CONFIG_X86
> +		__asm__ __volatile__ ("pause");
> +#else
> +		__asm__ __volatile__ ("yield");

ARM yield is meant to be used on multi-threaded systems to indicate that
the thread can be swapped. Why would we need it in U-Boot which is
single-threaded?

Can't we simply use

while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR);

with no command in the loop for all architectures?

Best regards

Heinrich

> +#endif
>  }
>
> -void qemu_fwcfg_read_entry(uint16_t entry, uint32_t length, void *address)
> +void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
> +			   void *address)
>  {
> -	if (fwcfg_dma_present)
> -		qemu_fwcfg_read_entry_dma(entry, length, address);
> +	struct qfw_plat *plat = dev_get_plat(dev);
> +	struct qfw_priv *priv = dev_get_priv(dev);
> +
> +	if (priv->dma_present)
> +		qemu_fwcfg_read_entry_dma(plat, entry, length, address);
>  	else
> -		qemu_fwcfg_read_entry_pio(entry, length, address);
> +		qemu_fwcfg_read_entry_io(plat, entry, length, address);
>  }
>
> -int qemu_fwcfg_online_cpus(void)
> +int qemu_fwcfg_online_cpus(struct udevice *dev)
>  {
> -	uint16_t nb_cpus;
> -
> -	if (!fwcfg_present)
> -		return -ENODEV;
> +	u16 nb_cpus;
>
> -	qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
> +	qemu_fwcfg_read_entry(dev, FW_CFG_NB_CPUS, 2, &nb_cpus);
>
>  	return le16_to_cpu(nb_cpus);
>  }
>
> -int qemu_fwcfg_read_firmware_list(void)
> +int qemu_fwcfg_read_firmware_list(struct udevice *dev)
>  {
>  	int i;
> -	uint32_t count;
> +	u32 count;
>  	struct fw_file *file;
>  	struct list_head *entry;
>
> +	struct qfw_priv *priv = dev_get_priv(dev);
> +
>  	/* don't read it twice */
> -	if (!list_empty(&fw_list))
> +	if (!list_empty(&priv->fw_list))
>  		return 0;
>
> -	qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
> +	qemu_fwcfg_read_entry(dev, FW_CFG_FILE_DIR, 4, &count);
>  	if (!count)
>  		return 0;
>
> @@ -319,16 +397,16 @@ int qemu_fwcfg_read_firmware_list(void)
>  			printf("error: allocating resource\n");
>  			goto err;
>  		}
> -		qemu_fwcfg_read_entry(FW_CFG_INVALID,
> +		qemu_fwcfg_read_entry(dev, FW_CFG_INVALID,
>  				      sizeof(struct fw_cfg_file), &file->cfg);
>  		file->addr = 0;
> -		list_add_tail(&file->list, &fw_list);
> +		list_add_tail(&file->list, &priv->fw_list);
>  	}
>
>  	return 0;
>
>  err:
> -	list_for_each(entry, &fw_list) {
> +	list_for_each(entry, &priv->fw_list) {
>  		file = list_entry(entry, struct fw_file, list);
>  		free(file);
>  	}
> @@ -336,12 +414,14 @@ err:
>  	return -ENOMEM;
>  }
>
> -struct fw_file *qemu_fwcfg_find_file(const char *name)
> +struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name)
>  {
>  	struct list_head *entry;
>  	struct fw_file *file;
>
> -	list_for_each(entry, &fw_list) {
> +	struct qfw_priv *priv = dev_get_priv(dev);
> +
> +	list_for_each(entry, &priv->fw_list) {
>  		file = list_entry(entry, struct fw_file, list);
>  		if (!strcmp(file->cfg.name, name))
>  			return file;
> @@ -350,9 +430,13 @@ struct fw_file *qemu_fwcfg_find_file(const char *name)
>  	return NULL;
>  }
>
> -struct fw_file *qemu_fwcfg_file_iter_init(struct fw_cfg_file_iter *iter)
> +struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
> +					  struct fw_cfg_file_iter *iter)
>  {
> -	iter->entry = fw_list.next;
> +	struct qfw_priv *priv = dev_get_priv(dev);
> +
> +	iter->entry = priv->fw_list.next;
> +	iter->end = &priv->fw_list;
>  	return list_entry((struct list_head *)iter->entry,
>  			  struct fw_file, list);
>  }
> @@ -366,29 +450,62 @@ struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter)
>
>  bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter)
>  {
> -	return iter->entry == &fw_list;
> +	return iter->entry == iter->end;
>  }
>
> -void qemu_fwcfg_init(struct fw_cfg_arch_ops *ops)
> +static int qfw_of_to_plat(struct udevice *dev)
>  {
> -	uint32_t qemu;
> -	uint32_t dma_enabled;
> -
> -	fwcfg_present = false;
> -	fwcfg_dma_present = false;
> -	fwcfg_arch_ops = NULL;
> -
> -	if (!ops || !ops->arch_read_pio || !ops->arch_read_dma)
> -		return;
> -	fwcfg_arch_ops = ops;
> -
> -	qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
> -	if (be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE)
> -		fwcfg_present = true;
> -
> -	if (fwcfg_present) {
> -		qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
> -		if (dma_enabled & FW_CFG_DMA_ENABLED)
> -			fwcfg_dma_present = true;
> -	}
> +	struct qfw_plat *plat = dev_get_plat(dev);
> +
> +	plat->mmio = map_physmem(dev_read_addr(dev),
> +				 sizeof(struct qfw_mmio),
> +				 MAP_NOCACHE);
> +
> +	return 0;
> +}
> +
> +static int qfw_probe(struct udevice *dev)
> +{
> +	u32 qemu, dma_enabled;
> +	struct qfw_plat *plat = dev_get_plat(dev);
> +	struct qfw_priv *priv = dev_get_priv(dev);
> +
> +	INIT_LIST_HEAD(&priv->fw_list);
> +
> +	qemu_fwcfg_read_entry_io(plat, FW_CFG_SIGNATURE, 4, &qemu);
> +	if (be32_to_cpu(qemu) != QEMU_FW_CFG_SIGNATURE)
> +		return -ENODEV;
> +
> +	qemu_fwcfg_read_entry_io(plat, FW_CFG_ID, 1, &dma_enabled);
> +	if (dma_enabled & FW_CFG_DMA_ENABLED)
> +		priv->dma_present = true;
> +
> +	return 0;
> +}
> +
> +static const struct udevice_id qfw_ids[] = {
> +	{ .compatible = "qemu,fw-cfg-mmio" },
> +	{}
> +};
> +
> +U_BOOT_DRIVER(qfw) = {
> +	.name		= "qfw",
> +	.id		= UCLASS_MISC,
> +	.of_match	= qfw_ids,
> +	.of_to_plat	= qfw_of_to_plat,
> +	.plat_auto	= sizeof(struct qfw_plat),
> +	.priv_auto	= sizeof(struct qfw_priv),
> +	.probe		= qfw_probe,
> +};
> +
> +struct udevice *qemu_fwcfg_dev(void)
> +{
> +	struct udevice *dev;
> +	int ret;
> +
> +	/* XXX: decide what to do here */
> +	ret = uclass_first_device(UCLASS_MISC, &dev);
> +	if (ret)
> +		return NULL;
> +	return dev;
>  }
> diff --git a/include/qfw.h b/include/qfw.h
> index cea8e11d44..f9c6828841 100644
> --- a/include/qfw.h
> +++ b/include/qfw.h
> @@ -82,19 +82,7 @@ struct fw_file {
>  };
>
>  struct fw_cfg_file_iter {
> -	struct list_head *entry; /* structure to iterate file list */
> -};
> -
> -struct fw_cfg_dma_access {
> -	__be32 control;
> -	__be32 length;
> -	__be64 address;
> -};
> -
> -struct fw_cfg_arch_ops {
> -	void (*arch_read_pio)(uint16_t selector, uint32_t size,
> -			void *address);
> -	void (*arch_read_dma)(struct fw_cfg_dma_access *dma);
> +	struct list_head *entry, *end; /* structures to iterate file list */
>  };
>
>  struct bios_linker_entry {
> @@ -146,32 +134,59 @@ struct bios_linker_entry {
>  	};
>  } __packed;
>
> +struct qfw_mmio {
> +	/*
> +	 * Each access to the 64-bit data register can be 8/16/32/64 bits wide.
> +	 */
> +	union {
> +		u8 data8;
> +		u16 data16;
> +		u32 data32;
> +		u64 data64;
> +	};
> +	u16 selector;
> +	u8 padding[6];
> +	u64 dma;
> +};
> +
> +struct qfw_plat {
> +	/* MMIO used on Arm. */
> +	volatile struct qfw_mmio *mmio;
> +	/* IO used on x86. */
> +	struct {
> +		u16 control_port;
> +		u16 data_port;
> +		u16 dma_port_low;
> +		u16 dma_port_high;
> +	} io;
> +};
> +
> +struct udevice;
>  /**
> - * Initialize QEMU fw_cfg interface
> + * Get QEMU firmware config device
>   *
> - * @ops: arch specific read operations
> + * @return struct udevice * if present, NULL otherwise
>   */
> -void qemu_fwcfg_init(struct fw_cfg_arch_ops *ops);
> +struct udevice *qemu_fwcfg_dev(void);
>
> -void qemu_fwcfg_read_entry(uint16_t entry, uint32_t length, void *address);
> -int qemu_fwcfg_read_firmware_list(void);
> -struct fw_file *qemu_fwcfg_find_file(const char *name);
> +void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
> +			   void *address);
> +int qemu_fwcfg_read_firmware_list(struct udevice *dev);
> +struct fw_file *qemu_fwcfg_find_file(struct udevice *dev, const char *name);
>
>  /**
>   * Get system cpu number
>   *
>   * @return:   cpu number in system
>   */
> -int qemu_fwcfg_online_cpus(void);
> +int qemu_fwcfg_online_cpus(struct udevice *dev);
>
>  /* helper functions to iterate firmware file list */
> -struct fw_file *qemu_fwcfg_file_iter_init(struct fw_cfg_file_iter *iter);
> +struct fw_file *qemu_fwcfg_file_iter_init(struct udevice *dev,
> +					  struct fw_cfg_file_iter *iter);
>  struct fw_file *qemu_fwcfg_file_iter_next(struct fw_cfg_file_iter *iter);
>  bool qemu_fwcfg_file_iter_end(struct fw_cfg_file_iter *iter);
>
> -bool qemu_fwcfg_present(void);
> -bool qemu_fwcfg_dma_present(void);
> -
>  /**
>   * qemu_cpu_fixup() - Fix up the CPUs for QEMU
>   *
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 12:59   ` Heinrich Schuchardt
@ 2021-02-23 14:53     ` Tom Rini
  2021-02-23 15:54       ` Heinrich Schuchardt
  2021-02-24  0:22     ` Asherah Connor
  1 sibling, 1 reply; 18+ messages in thread
From: Tom Rini @ 2021-02-23 14:53 UTC (permalink / raw)
  To: u-boot

On Tue, Feb 23, 2021 at 01:59:52PM +0100, Heinrich Schuchardt wrote:
> On 23.02.21 12:43, Asherah Connor wrote:
> > Updates the QFW driver to use the driver model, and adds support for QFW
> > on Arm platforms by configuring from the device tree and using MMIO
> > accordingly.  A sandbox driver for QFW is also included, and a simple DM
> > unit test for it.
> 
> For which architectures does the fw_cfg device exist?
> 
> It it is only ARM and X86, than I am missing such a dependency on
> CONFIG_CMD_QFW.

The qemu 'qfw' interface is I believe a generic QEMU thing and a generic
U-Boot cleanup would be to have a common QEMU symbol for everyone to
select.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/5a9023ae/attachment.sig>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 14:53     ` Tom Rini
@ 2021-02-23 15:54       ` Heinrich Schuchardt
  2021-02-23 16:03         ` Tom Rini
  0 siblings, 1 reply; 18+ messages in thread
From: Heinrich Schuchardt @ 2021-02-23 15:54 UTC (permalink / raw)
  To: u-boot

Am 23. Februar 2021 15:53:38 MEZ schrieb Tom Rini <trini@konsulko.com>:
>On Tue, Feb 23, 2021 at 01:59:52PM +0100, Heinrich Schuchardt wrote:
>> On 23.02.21 12:43, Asherah Connor wrote:
>> > Updates the QFW driver to use the driver model, and adds support
>for QFW
>> > on Arm platforms by configuring from the device tree and using MMIO
>> > accordingly.  A sandbox driver for QFW is also included, and a
>simple DM
>> > unit test for it.
>> 
>> For which architectures does the fw_cfg device exist?
>> 
>> It it is only ARM and X86, than I am missing such a dependency on
>> CONFIG_CMD_QFW.
>
>The qemu 'qfw' interface is I believe a generic QEMU thing and a
>generic
>U-Boot cleanup would be to have a common QEMU symbol for everyone to
>select.


qemu-system-riscv64 does not allow me to specify a file for the qfw interface.

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio
  2021-02-23 11:43 ` [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio Asherah Connor
@ 2021-02-23 15:58   ` Simon Glass
  0 siblings, 0 replies; 18+ messages in thread
From: Simon Glass @ 2021-02-23 15:58 UTC (permalink / raw)
  To: u-boot

Hi Asherah,

On Tue, 23 Feb 2021 at 06:44, Asherah Connor <ashe@kivikakk.ee> wrote:
>
> Split the qfw driver into qfw_pio and qfw_mmio, under their own uclass.
> Each driver does arch/platform-specific I/O.
>
> Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
> ---
>
> Changes in v3:
> - Add new UCLASS_QFW, split qfw driver into PIO and MMIO variants.
> - QFW no longer depends on or selects MISC.
>
>  arch/x86/cpu/qemu/cpu.c  |   2 +-
>  arch/x86/cpu/qemu/qemu.c |  18 ++-
>  arch/x86/cpu/qfw_cpu.c   |   2 +-
>  cmd/qfw.c                |  26 ++--
>  common/Makefile          |   2 +
>  common/qfw.c             | 111 ++++++++++++++++
>  drivers/misc/Kconfig     |   1 -
>  drivers/misc/Makefile    |   6 +
>  drivers/misc/qfw.c       | 270 ++++++---------------------------------
>  drivers/misc/qfw_mmio.c  | 101 +++++++++++++++
>  drivers/misc/qfw_pio.c   |  66 ++++++++++
>  include/dm/uclass-id.h   |   1 +
>  include/qfw.h            |  45 +++++--
>  13 files changed, 382 insertions(+), 269 deletions(-)
>  create mode 100644 common/qfw.c
>  create mode 100644 drivers/misc/qfw_mmio.c
>  create mode 100644 drivers/misc/qfw_pio.c
>


[..]

> diff --git a/include/qfw.h b/include/qfw.h
> index f9c6828841..d7e16651a2 100644
> --- a/include/qfw.h
> +++ b/include/qfw.h
> @@ -149,28 +149,49 @@ struct qfw_mmio {
>         u64 dma;
>  };
>
> -struct qfw_plat {
> -       /* MMIO used on Arm. */
> +struct qfw_pio_plat {
> +       u16 control_port;
> +       u16 data_port;
> +       u16 dma_port_low;
> +       u16 dma_port_high;
> +};
> +
> +struct qfw_mmio_plat {
>         volatile struct qfw_mmio *mmio;
> -       /* IO used on x86. */
> -       struct {
> -               u16 control_port;
> -               u16 data_port;
> -               u16 dma_port_low;
> -               u16 dma_port_high;
> -       } io;
>  };
>
> +struct qfw_dma {
> +       __be32 control;
> +       __be32 length;
> +       __be64 address;
> +};
> +
> +struct qfw_dev {
> +       struct udevice *dev;
> +       bool dma_present;
> +       struct list_head fw_list;
> +};
> +
> +struct dm_qfw_ops {

comment struct and methods. See other uclasses for how this is done.

> +       void (*read_entry_io)(struct udevice *dev, u16 entry, u32 size,
> +                             void *address);
> +       void (*read_entry_dma)(struct udevice *dev, struct qfw_dma *dma);
> +};
> +
> +#define dm_qfw_get_ops(dev) \
> +               ((struct dm_qfw_ops *)(dev)->driver->ops)
> +
> +int qfw_register(struct udevice *dev);
> +
>  struct udevice;
>  /**
>   * Get QEMU firmware config device
>   *
>   * @return struct udevice * if present, NULL otherwise
>   */
> -struct udevice *qemu_fwcfg_dev(void);
> +struct udevice *qfw_get_dev(void);

The problem with this function is that you drop the error number. Can
you instead do something like:

int qfw_get_dev(struct udevice **devp)

>
> -void qemu_fwcfg_read_entry(struct udevice *dev, u16 entry, u32 length,
> -                          void *address);
> +void qfw_read_entry(struct udevice *dev, u16 entry, u32 length, void *address);

Could you please add full function/struct comments to the things you
modify in the header file? That way people can read the API in one
place.

I'm not sure if it is easy to split up your patches a bit more. It is
often tricky with a DM conversion.

Regards,
Simon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 4/4] qemu: add sandbox driver and tests
  2021-02-23 11:43 ` [PATCH v3 4/4] qemu: add sandbox driver and tests Asherah Connor
  2021-02-23 12:26   ` Asherah Connor
@ 2021-02-23 15:58   ` Simon Glass
  1 sibling, 0 replies; 18+ messages in thread
From: Simon Glass @ 2021-02-23 15:58 UTC (permalink / raw)
  To: u-boot

On Tue, 23 Feb 2021 at 06:44, Asherah Connor <ashe@kivikakk.ee> wrote:
>
> We minimally exercise the sandbox driver.
>
> Signed-off-by: Asherah Connor <ashe@kivikakk.ee>
> ---
>
> (no changes since v1)
>
>  arch/sandbox/dts/sandbox.dtsi |   4 ++
>  arch/sandbox/dts/test.dts     |   4 ++
>  drivers/misc/Makefile         |  11 ++-
>  drivers/misc/qfw_sandbox.c    | 129 ++++++++++++++++++++++++++++++++++
>  test/dm/Makefile              |   1 +
>  test/dm/qfw.c                 |  42 +++++++++++
>  6 files changed, 185 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/misc/qfw_sandbox.c
>  create mode 100644 test/dm/qfw.c

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 15:54       ` Heinrich Schuchardt
@ 2021-02-23 16:03         ` Tom Rini
  2021-02-23 16:15           ` Heinrich Schuchardt
  0 siblings, 1 reply; 18+ messages in thread
From: Tom Rini @ 2021-02-23 16:03 UTC (permalink / raw)
  To: u-boot

On Tue, Feb 23, 2021 at 04:54:45PM +0100, Heinrich Schuchardt wrote:
> Am 23. Februar 2021 15:53:38 MEZ schrieb Tom Rini <trini@konsulko.com>:
> >On Tue, Feb 23, 2021 at 01:59:52PM +0100, Heinrich Schuchardt wrote:
> >> On 23.02.21 12:43, Asherah Connor wrote:
> >> > Updates the QFW driver to use the driver model, and adds support
> >for QFW
> >> > on Arm platforms by configuring from the device tree and using MMIO
> >> > accordingly.  A sandbox driver for QFW is also included, and a
> >simple DM
> >> > unit test for it.
> >> 
> >> For which architectures does the fw_cfg device exist?
> >> 
> >> It it is only ARM and X86, than I am missing such a dependency on
> >> CONFIG_CMD_QFW.
> >
> >The qemu 'qfw' interface is I believe a generic QEMU thing and a
> >generic
> >U-Boot cleanup would be to have a common QEMU symbol for everyone to
> >select.
> 
> qemu-system-riscv64 does not allow me to specify a file for the qfw interface.

Really?  It's listed under the help (taken from out docker images):
$ /opt/qemu/bin/qemu-system-riscv64 --help
...
Debug/Expert options:
-fw_cfg [name=]<name>,file=<file>
                add named fw_cfg entry with contents from file
-fw_cfg [name=]<name>,string=<str>
                add named fw_cfg entry with contents from string

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/b31ee3f7/attachment.sig>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 16:03         ` Tom Rini
@ 2021-02-23 16:15           ` Heinrich Schuchardt
  2021-02-23 16:33             ` Tom Rini
  0 siblings, 1 reply; 18+ messages in thread
From: Heinrich Schuchardt @ 2021-02-23 16:15 UTC (permalink / raw)
  To: u-boot

On 2/23/21 5:03 PM, Tom Rini wrote:
> On Tue, Feb 23, 2021 at 04:54:45PM +0100, Heinrich Schuchardt wrote:
>> Am 23. Februar 2021 15:53:38 MEZ schrieb Tom Rini <trini@konsulko.com>:
>>> On Tue, Feb 23, 2021 at 01:59:52PM +0100, Heinrich Schuchardt wrote:
>>>> On 23.02.21 12:43, Asherah Connor wrote:
>>>>> Updates the QFW driver to use the driver model, and adds support
>>> for QFW
>>>>> on Arm platforms by configuring from the device tree and using MMIO
>>>>> accordingly.  A sandbox driver for QFW is also included, and a
>>> simple DM
>>>>> unit test for it.
>>>>
>>>> For which architectures does the fw_cfg device exist?
>>>>
>>>> It it is only ARM and X86, than I am missing such a dependency on
>>>> CONFIG_CMD_QFW.
>>>
>>> The qemu 'qfw' interface is I believe a generic QEMU thing and a
>>> generic
>>> U-Boot cleanup would be to have a common QEMU symbol for everyone to
>>> select.
>>
>> qemu-system-riscv64 does not allow me to specify a file for the qfw interface.
>
> Really?  It's listed under the help (taken from out docker images):
> $ /opt/qemu/bin/qemu-system-riscv64 --help
> ...
> Debug/Expert options:
> -fw_cfg [name=]<name>,file=<file>
>                  add named fw_cfg entry with contents from file
> -fw_cfg [name=]<name>,string=<str>
>                  add named fw_cfg entry with contents from string
>

The man-page is shared by all qemu-system-*. It is not architecture
specific. That is why it shows items like: "PS/2 mouse and keyboard".

qemu-system-riscv64 (v5.0.0) has no fw_cfg device:

$ qemu-system-riscv64 -machine virt -m 1G -nographic -bios u-boot
-fw_cfg opt/foo,file=foo
qemu-system-riscv64: -fw_cfg opt/foo,file=foo: fw_cfg device not available

qemu-system-aarch64 does not complain:

$ qemu-system-aarch64 -machine virt -m 1G -nographic -bios u-boot
-fw_cfg opt/foo,file=foo

Best regards

Heinrich

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 16:15           ` Heinrich Schuchardt
@ 2021-02-23 16:33             ` Tom Rini
  2021-02-23 23:54               ` Asherah Connor
  0 siblings, 1 reply; 18+ messages in thread
From: Tom Rini @ 2021-02-23 16:33 UTC (permalink / raw)
  To: u-boot

On Tue, Feb 23, 2021 at 05:15:49PM +0100, Heinrich Schuchardt wrote:
> On 2/23/21 5:03 PM, Tom Rini wrote:
> > On Tue, Feb 23, 2021 at 04:54:45PM +0100, Heinrich Schuchardt wrote:
> > > Am 23. Februar 2021 15:53:38 MEZ schrieb Tom Rini <trini@konsulko.com>:
> > > > On Tue, Feb 23, 2021 at 01:59:52PM +0100, Heinrich Schuchardt wrote:
> > > > > On 23.02.21 12:43, Asherah Connor wrote:
> > > > > > Updates the QFW driver to use the driver model, and adds support
> > > > for QFW
> > > > > > on Arm platforms by configuring from the device tree and using MMIO
> > > > > > accordingly.  A sandbox driver for QFW is also included, and a
> > > > simple DM
> > > > > > unit test for it.
> > > > > 
> > > > > For which architectures does the fw_cfg device exist?
> > > > > 
> > > > > It it is only ARM and X86, than I am missing such a dependency on
> > > > > CONFIG_CMD_QFW.
> > > > 
> > > > The qemu 'qfw' interface is I believe a generic QEMU thing and a
> > > > generic
> > > > U-Boot cleanup would be to have a common QEMU symbol for everyone to
> > > > select.
> > > 
> > > qemu-system-riscv64 does not allow me to specify a file for the qfw interface.
> > 
> > Really?  It's listed under the help (taken from out docker images):
> > $ /opt/qemu/bin/qemu-system-riscv64 --help
> > ...
> > Debug/Expert options:
> > -fw_cfg [name=]<name>,file=<file>
> >                  add named fw_cfg entry with contents from file
> > -fw_cfg [name=]<name>,string=<str>
> >                  add named fw_cfg entry with contents from string
> > 
> 
> The man-page is shared by all qemu-system-*. It is not architecture
> specific. That is why it shows items like: "PS/2 mouse and keyboard".
> 
> qemu-system-riscv64 (v5.0.0) has no fw_cfg device:
> 
> $ qemu-system-riscv64 -machine virt -m 1G -nographic -bios u-boot
> -fw_cfg opt/foo,file=foo
> qemu-system-riscv64: -fw_cfg opt/foo,file=foo: fw_cfg device not available
> 
> qemu-system-aarch64 does not complain:
> 
> $ qemu-system-aarch64 -machine virt -m 1G -nographic -bios u-boot
> -fw_cfg opt/foo,file=foo

So all that's missing is someone hooking that up inside qemu itself.
I'm pretty sure it works on PowerPC, from when I was trying to figure
out how to pass something in to qemu-ppce500 a while ago.
qemu-system-mips/sh4 don't complain.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/8a569b88/attachment.sig>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 16:33             ` Tom Rini
@ 2021-02-23 23:54               ` Asherah Connor
  2021-02-23 23:58                 ` Tom Rini
  0 siblings, 1 reply; 18+ messages in thread
From: Asherah Connor @ 2021-02-23 23:54 UTC (permalink / raw)
  To: u-boot

On 21/02/23 11:02:p, Tom Rini wrote:
> On Tue, Feb 23, 2021 at 05:15:49PM +0100, Heinrich Schuchardt wrote:
> > On 2/23/21 5:03 PM, Tom Rini wrote:
> > > On Tue, Feb 23, 2021 at 04:54:45PM +0100, Heinrich Schuchardt wrote:
> > > > qemu-system-riscv64 does not allow me to specify a file for the qfw interface.
> > > 
> > > Really?  It's listed under the help (taken from out docker images):
> > > $ /opt/qemu/bin/qemu-system-riscv64 --help
> > > ...
> > > Debug/Expert options:
> > > -fw_cfg [name=]<name>,file=<file>
> > >                  add named fw_cfg entry with contents from file
> > > -fw_cfg [name=]<name>,string=<str>
> > >                  add named fw_cfg entry with contents from string
> > > 
> > 
> > The man-page is shared by all qemu-system-*. It is not architecture
> > specific. That is why it shows items like: "PS/2 mouse and keyboard".
> > 
> > qemu-system-riscv64 (v5.0.0) has no fw_cfg device:
> > 
> > $ qemu-system-riscv64 -machine virt -m 1G -nographic -bios u-boot
> > -fw_cfg opt/foo,file=foo
> > qemu-system-riscv64: -fw_cfg opt/foo,file=foo: fw_cfg device not available
> > 
> > qemu-system-aarch64 does not complain:
> > 
> > $ qemu-system-aarch64 -machine virt -m 1G -nographic -bios u-boot
> > -fw_cfg opt/foo,file=foo
> 
> So all that's missing is someone hooking that up inside qemu itself.
> I'm pretty sure it works on PowerPC, from when I was trying to figure
> out how to pass something in to qemu-ppce500 a while ago.
> qemu-system-mips/sh4 don't complain.

A review of the various FW_CFG_ Kconfig options in QEMU itself suggests
there is support on at least i386, arm, ppc, and mips (their arch
terms).  Looking further, it appears sparc(64) and hppa (HP PA-RISC) are
also supported in the source base.

But actually trying it, it looks like a freshly built QEMU 5.2.0 does
not support qfw on ppce500:

$ qemu-system-ppc -M ppce500 -nographic -bios \
  build-qemu-ppce500/u-boot -fw_cfg xyz,string=hello
qemu-system-ppc: -fw_cfg xyz,string=hello: fw_cfg device not available

Same for mips(64)(el).  Maybe I missed a configuration option somewhere.
The sparc systems work fine.

Best,

Asherah

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 23:54               ` Asherah Connor
@ 2021-02-23 23:58                 ` Tom Rini
  2021-02-24  0:12                   ` Asherah Connor
  0 siblings, 1 reply; 18+ messages in thread
From: Tom Rini @ 2021-02-23 23:58 UTC (permalink / raw)
  To: u-boot

On Tue, Feb 23, 2021 at 11:54:01PM +0000, Asherah Connor wrote:
> On 21/02/23 11:02:p, Tom Rini wrote:
> > On Tue, Feb 23, 2021 at 05:15:49PM +0100, Heinrich Schuchardt wrote:
> > > On 2/23/21 5:03 PM, Tom Rini wrote:
> > > > On Tue, Feb 23, 2021 at 04:54:45PM +0100, Heinrich Schuchardt wrote:
> > > > > qemu-system-riscv64 does not allow me to specify a file for the qfw interface.
> > > > 
> > > > Really?  It's listed under the help (taken from out docker images):
> > > > $ /opt/qemu/bin/qemu-system-riscv64 --help
> > > > ...
> > > > Debug/Expert options:
> > > > -fw_cfg [name=]<name>,file=<file>
> > > >                  add named fw_cfg entry with contents from file
> > > > -fw_cfg [name=]<name>,string=<str>
> > > >                  add named fw_cfg entry with contents from string
> > > > 
> > > 
> > > The man-page is shared by all qemu-system-*. It is not architecture
> > > specific. That is why it shows items like: "PS/2 mouse and keyboard".
> > > 
> > > qemu-system-riscv64 (v5.0.0) has no fw_cfg device:
> > > 
> > > $ qemu-system-riscv64 -machine virt -m 1G -nographic -bios u-boot
> > > -fw_cfg opt/foo,file=foo
> > > qemu-system-riscv64: -fw_cfg opt/foo,file=foo: fw_cfg device not available
> > > 
> > > qemu-system-aarch64 does not complain:
> > > 
> > > $ qemu-system-aarch64 -machine virt -m 1G -nographic -bios u-boot
> > > -fw_cfg opt/foo,file=foo
> > 
> > So all that's missing is someone hooking that up inside qemu itself.
> > I'm pretty sure it works on PowerPC, from when I was trying to figure
> > out how to pass something in to qemu-ppce500 a while ago.
> > qemu-system-mips/sh4 don't complain.
> 
> A review of the various FW_CFG_ Kconfig options in QEMU itself suggests
> there is support on at least i386, arm, ppc, and mips (their arch
> terms).  Looking further, it appears sparc(64) and hppa (HP PA-RISC) are
> also supported in the source base.
> 
> But actually trying it, it looks like a freshly built QEMU 5.2.0 does
> not support qfw on ppce500:
> 
> $ qemu-system-ppc -M ppce500 -nographic -bios \
>   build-qemu-ppce500/u-boot -fw_cfg xyz,string=hello
> qemu-system-ppc: -fw_cfg xyz,string=hello: fw_cfg device not available
> 
> Same for mips(64)(el).  Maybe I missed a configuration option somewhere.
> The sparc systems work fine.

Ah well, so my experiment would likely have not worked back then anyhow
(but I don't recall seeing an error at the time).  Anyhow, for now in
U-Boot as there's not a generic QEMU symbol, this side of things should
depend on ARM||X86 for now and let future enhancements add it elsewhere.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210223/4938f7b0/attachment.sig>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 23:58                 ` Tom Rini
@ 2021-02-24  0:12                   ` Asherah Connor
  0 siblings, 0 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-24  0:12 UTC (permalink / raw)
  To: u-boot

On 21/02/23 06:02:p, Tom Rini wrote:
> Ah well, so my experiment would likely have not worked back then anyhow
> (but I don't recall seeing an error at the time).  Anyhow, for now in
> U-Boot as there's not a generic QEMU symbol, this side of things should
> depend on ARM||X86 for now and let future enhancements add it elsewhere.
> Thanks!

I think this is a fine way to do it.  If users come forward wanting to
use it on other archs we can rethink it, but as long as there are only
two it seems not yet worth making fully generic.

Best,

Asherah

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include Arm support
  2021-02-23 12:59   ` Heinrich Schuchardt
  2021-02-23 14:53     ` Tom Rini
@ 2021-02-24  0:22     ` Asherah Connor
  1 sibling, 0 replies; 18+ messages in thread
From: Asherah Connor @ 2021-02-24  0:22 UTC (permalink / raw)
  To: u-boot

On 21/02/23 01:02:p, Heinrich Schuchardt wrote:
> On 23.02.21 12:43, Asherah Connor wrote:
> For which architectures does the fw_cfg device exist?
> 
> It it is only ARM and X86, than I am missing such a dependency on
> CONFIG_CMD_QFW.

Right now we have:

arch/arm/Kconfig:
	...
	config ARCH_QEMU
		...
		imply CMD_QFW

I think we also want this:

arch/x86/cpu/qemu/Kconfig
	config QEMU
		...
		imply CMD_QFW

This is my first time using Kconfig and I'll admit I'm not too certain
where things go.


> If these numbers are constants, why should they be copied to platform
> data? This only increases code size.
> 
> I think there is nothing wrong with using constants here.

Okay, excellent.  I'll fold those into drivers/misc/qfw_pio.c and get
rid of the qfw_pio_plat struct entirely.  


> ARM yield is meant to be used on multi-threaded systems to indicate that
> the thread can be swapped. Why would we need it in U-Boot which is
> single-threaded?
> 
> Can't we simply use
> 
> while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR);
> 
> with no command in the loop for all architectures?

Good question.  This code originated here, where the original (x86-only)
driver used pause:

https://gitlab.denx.de/u-boot/u-boot/-/commit/f60df20aa966c3de850afafe3cce70a51d0b261c#41c93c056084377352da52f1d88fc49288a4846f_0_59

When porting to Arm I used the equivalent.

While U-Boot is single-threaded, the architecture that executes this
instruction is always QEMU, and -- at a guess -- it might be that
pause/yield here lets QEMU finish its part of the DMA faster.

I've run the QEMU tests on arm(64)/x86(_64) without the yield or pause
and they still pass.  It might be simply unnecessary, so I'll remove for
now in favour of simplicity and less arch-specific code.

Best,

Asherah

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-02-24  0:22 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 11:43 [PATCH v3 0/4] Move qfw to DM, add Arm support Asherah Connor
2021-02-23 11:43 ` [PATCH v3 1/4] arm: x86: qemu: move qfw to DM, include " Asherah Connor
2021-02-23 12:59   ` Heinrich Schuchardt
2021-02-23 14:53     ` Tom Rini
2021-02-23 15:54       ` Heinrich Schuchardt
2021-02-23 16:03         ` Tom Rini
2021-02-23 16:15           ` Heinrich Schuchardt
2021-02-23 16:33             ` Tom Rini
2021-02-23 23:54               ` Asherah Connor
2021-02-23 23:58                 ` Tom Rini
2021-02-24  0:12                   ` Asherah Connor
2021-02-24  0:22     ` Asherah Connor
2021-02-23 11:43 ` [PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio Asherah Connor
2021-02-23 15:58   ` Simon Glass
2021-02-23 11:43 ` [PATCH v3 3/4] arm: x86: qemu: unify qfw driver functions as qfw_ Asherah Connor
2021-02-23 11:43 ` [PATCH v3 4/4] qemu: add sandbox driver and tests Asherah Connor
2021-02-23 12:26   ` Asherah Connor
2021-02-23 15:58   ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.