On 06/16/17 15:23, Mark Cave-Ayland wrote:
> This allows the device to be instantiated externally for boards that
> wish to wire up the fw_cfg device themselves.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> ---
> hw/nvram/fw_cfg.c | 56 -------------------------------------------
> include/hw/nvram/fw_cfg.h | 58
> +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 56 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b5f10ac..00771c9 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -40,14 +40,6 @@
> #define FW_CFG_NAME "fw_cfg"
> #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>
> -#define TYPE_FW_CFG "fw_cfg"
> -#define TYPE_FW_CFG_IO "fw_cfg_io"
> -#define TYPE_FW_CFG_MEM "fw_cfg_mem"
> -
> -#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
> -#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
> -#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
> -
> /* FW_CFG_VERSION bits */
> #define FW_CFG_VERSION 0x01
> #define FW_CFG_VERSION_DMA 0x02
> @@ -61,54 +53,6 @@
>
> #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>
> -typedef struct FWCfgEntry {
> - uint32_t len;
> - bool allow_write;
> - uint8_t *data;
> - void *callback_opaque;
> - FWCfgReadCallback read_callback;
> -} FWCfgEntry;
> -
> -struct FWCfgState {
> - /*< private >*/
> - SysBusDevice parent_obj;
> - /*< public >*/
> -
> - uint16_t file_slots;
> - FWCfgEntry *entries[2];
> - int *entry_order;
> - FWCfgFiles *files;
> - uint16_t cur_entry;
> - uint32_t cur_offset;
> - Notifier machine_ready;
> -
> - int fw_cfg_order_override;
> -
> - bool dma_enabled;
> - dma_addr_t dma_addr;
> - AddressSpace *dma_as;
> - MemoryRegion dma_iomem;
> -};
> -
> -struct FWCfgIoState {
> - /*< private >*/
> - FWCfgState parent_obj;
> - /*< public >*/
> -
> - MemoryRegion comb_iomem;
> - uint32_t iobase, dma_iobase;
> -};
> -
> -struct FWCfgMemState {
> - /*< private >*/
> - FWCfgState parent_obj;
> - /*< public >*/
> -
> - MemoryRegion ctl_iomem, data_iomem;
> - uint32_t data_width;
> - MemoryRegionOps wide_data_ops;
> -};
> -
> #define JPG_FILE 0
> #define BMP_FILE 1
>
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b980cba..a16907a 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -3,6 +3,16 @@
>
> #include "exec/hwaddr.h"
> #include "hw/nvram/fw_cfg_keys.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +
> +#define TYPE_FW_CFG "fw_cfg"
> +#define TYPE_FW_CFG_IO "fw_cfg_io"
> +#define TYPE_FW_CFG_MEM "fw_cfg_mem"
> +
> +#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
> +#define FW_CFG_IO(obj) OBJECT_CHECK(FWCfgIoState, (obj), TYPE_FW_CFG_IO)
> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>
> typedef struct FWCfgFile {
> uint32_t size; /* file size */
> @@ -35,6 +45,54 @@ typedef struct FWCfgDmaAccess {
>
> typedef void (*FWCfgReadCallback)(void *opaque);
>
> +typedef struct FWCfgEntry {
> + uint32_t len;
> + bool allow_write;
> + uint8_t *data;
> + void *callback_opaque;
> + FWCfgReadCallback read_callback;
> +} FWCfgEntry;
> +
> +struct FWCfgState {
> + /*< private >*/
> + SysBusDevice parent_obj;
> + /*< public >*/
> +
> + uint16_t file_slots;
> + FWCfgEntry *entries[2];
> + int *entry_order;
> + FWCfgFiles *files;
> + uint16_t cur_entry;
> + uint32_t cur_offset;
> + Notifier machine_ready;
> +
> + int fw_cfg_order_override;
> +
> + bool dma_enabled;
> + dma_addr_t dma_addr;
> + AddressSpace *dma_as;
> + MemoryRegion dma_iomem;
> +};
> +
> +struct FWCfgIoState {
> + /*< private >*/
> + FWCfgState parent_obj;
> + /*< public >*/
> +
> + MemoryRegion comb_iomem;
> + uint32_t iobase, dma_iobase;
> +};
> +
> +struct FWCfgMemState {
> + /*< private >*/
> + FWCfgState parent_obj;
> + /*< public >*/
> +
> + MemoryRegion ctl_iomem, data_iomem;
> + uint32_t data_width;
> + MemoryRegionOps wide_data_ops;
> +};
> +
> /**
> * fw_cfg_add_bytes:
> * @s: fw_cfg device being modified
>
This patch is generally good, but I'd like to suggest improvements:
(1) In the commit message, please mention that we are exposing the
internals of FWCfgIoState and FWCfgMemState so that board code can map
the MemoryRegion fields (such as comb_iomem) *by name*.
(2) FWCfgEntry need not be moved from the C file to the header file,
since we never depend on the *size* of that structure. Instead, please
add a single line to "include/qemu/typedefs.h":
typedef struct FWCfgEntry FWCfgEntry;
and modify
typedef struct FWCfgEntry {
...
} FWCfgEntry;
in "fw_cfg.c" to just
struct FWCfgEntry {
...
};
Then "fw_cfg.h" can simply include "typedefs.h" and say
...
FWCfgEntry *entries[2];
...
IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".
(3) When you fix up patch #1 like I requested, removing the
"FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
forget to update this patch as well, so that you not re-introduce those
fields in the header file.
... I'm positively satisfied with this series (I plan to "grant" my R-b
for PATCH v5 5/5, with the above updates), but given that I'm no QOM
expert by any means, I'd like someone with more QOM experience to review
the patchset as well.
Thanks!
Laszlo
