[PATCH v3 2/4] arm: x86: qemu: add UCLASS_QFW, split driver into _pio and _mmio

[Simon Glass]

Hi Asherah,

On Tue, 23 Feb 2021 at 06:44, Asherah Connor <ashe at 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 at 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