[v3,1/4] arm: x86: qemu: move qfw to DM, include Arm support

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(-)

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
>   *
>

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.

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

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

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

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.

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

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!

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

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