[PATCHv4,5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

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-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c         |   56 -------------------------------------------
 include/hw/nvram/fw_cfg.h |   58 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 56 deletions(-)

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-ayland@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

On 16/06/17 22:28, Laszlo Ersek wrote:

> 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 for the feedback! I've just revised the patchset based upon your
comments and will post the v5 to the list shortly.


ATB,

Mark.