Re: [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR

[Peter Maydell]

On Wed, 1 Dec 2021 at 15:40, Francisco Iglesias
<francisco.iglesias@xilinx.com> wrote:
>
> Add a model of Versal's PMC SLCR (system-level control registers).
>
> Signed-off-by: Francisco Iglesias <francisco.iglesias@xilinx.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> +static void xlnx_versal_pmc_iou_slcr_realize(DeviceState *dev, Error **errp)
> +{
> +    XlnxVersalPmcIouSlcr *s = XILINX_VERSAL_PMC_IOU_SLCR(dev);
> +
> +    qdev_init_gpio_out(dev, s->sd_emmc_sel, 2);
> +    qdev_init_gpio_out(dev, &s->qspi_ospi_mux_sel, 1);
> +    qdev_init_gpio_out(dev, &s->ospi_mux_sel, 1);

Could we use named GPIOs for these? That would be clearer
in the code that wires them up than having to remember what
unnamed gpio lines 0,1,2,3 do.

(Also, I don't see anywhere in the board code in patch 2
that wires these GPIO lines up. Did I miss it?)

> +#ifndef XILINX_VERSAL_PMC_IOU_SLCR_H
> +#define XILINX_VERSAL_PMC_IOU_SLCR_H

For a complicated device like this which has multiple IRQs
and GPIOs, I would suggest having a comment here which
defines the "QEMU interface", which is just a list
of all the sysbus MMIO regions, QOM properties, named and
unnamed GPIO inputs and outputs, sysbus IRQs, etc, which the
device has (basically, anything that code creating one of these
devices might want to configure or wire up). There's an
example in include/hw/misc/tz-ppc.h (or grep in include/ for
"QEMU interface" for others).

> +
> +#include "hw/register.h"
> +
> +#define TYPE_XILINX_VERSAL_PMC_IOU_SLCR "xlnx.versal-pmc-iou-slcr"
> +
> +#define XILINX_VERSAL_PMC_IOU_SLCR(obj) \
> +     OBJECT_CHECK(XlnxVersalPmcIouSlcr, (obj), 
> TYPE_XILINX_VERSAL_PMC_IOU_SLCR)

Don't define cast macros by hand, please. Prefer
OBJECT_DECLARE_SIMPLE_TYPE() (which will also do the
typedef for you, so you don't need it on the struct).

> +
> +#define XILINX_VERSAL_PMC_IOU_SLCR_R_MAX (0x828 / 4 + 1)
> +
> +typedef struct XlnxVersalPmcIouSlcr {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +    qemu_irq irq_parity_imr;
> +    qemu_irq irq_imr;
> +    qemu_irq sd_emmc_sel[2];
> +    qemu_irq qspi_ospi_mux_sel;
> +    qemu_irq ospi_mux_sel;
> +
> +    uint32_t regs[XILINX_VERSAL_PMC_IOU_SLCR_R_MAX];
> +    RegisterInfo regs_info[XILINX_VERSAL_PMC_IOU_SLCR_R_MAX];
> +} XlnxVersalPmcIouSlcr;
> +
> +#endif /* XILINX_VERSAL_PMC_IOU_SLCR_H */

Otherwise this patch looks OK.

-- PMM