Re: [PATCH v1 1/2] hw/misc: versal: Add a model of the XRAM controller

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 2 Mar 2021 at 11:09, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add a model of the Xilinx Versal Accelerator RAM (XRAM).
> This is mainly a stub to make firmware happy. The size of
> the RAMs can be probed. The interrupt mask logic is
> modelled but none of the interrups will ever be raised
> unless injected.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  include/hw/misc/xlnx-versal-xramc.h | 102 +++++++++++
>  hw/misc/xlnx-versal-xramc.c         | 253 ++++++++++++++++++++++++++++
>  hw/misc/meson.build                 |   1 +
>  3 files changed, 356 insertions(+)
>  create mode 100644 include/hw/misc/xlnx-versal-xramc.h
>  create mode 100644 hw/misc/xlnx-versal-xramc.c
>
> diff --git a/include/hw/misc/xlnx-versal-xramc.h b/include/hw/misc/xlnx-versal-xramc.h
> new file mode 100644
> index 0000000000..68163cf330
> --- /dev/null
> +++ b/include/hw/misc/xlnx-versal-xramc.h
> @@ -0,0 +1,102 @@
> +/*
> + * QEMU model of the Xilinx XRAM Controller.
> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> + */
> +
> +#ifndef XLNX_VERSAL_XRAMC_H
> +#define XLNX_VERSAL_XRAMC_H
> +
> +#include "qemu/osdep.h"

Headers must never include osdep.h.

> +#include "hw/sysbus.h"
> +#include "hw/register.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +#include "migration/vmstate.h"
> +#include "hw/irq.h"

I bet you don't really need all of these includes in the header file;
some of them belong in the .c file.

> +static void xram_ctrl_init(Object *obj)
> +{
> +    XlnxXramCtrl *s = XLNX_XRAM_CTRL(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    RegisterInfoArray *reg_array;
> +
> +    memory_region_init(&s->iomem, obj, TYPE_XLNX_XRAM_CTRL,
> +                       XRAM_CTRL_R_MAX * 4);
> +    reg_array =
> +        register_init_block32(DEVICE(obj), xram_ctrl_regs_info,
> +                              ARRAY_SIZE(xram_ctrl_regs_info),
> +                              s->regs_info, s->regs,
> +                              &xram_ctrl_ops,
> +                              XLNX_XRAM_CTRL_ERR_DEBUG,
> +                              XRAM_CTRL_R_MAX * 4);
> +    memory_region_add_subregion(&s->iomem,
> +                                0x0,
> +                                &reg_array->mem);

Isn't this just creating a container region that contains
exactly one subregion that is the same size as it? Do we
need to do this so that the reg_array is disposed of via
refcounting or something ?

> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +}

thanks
-- PMM