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