[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR
From: |
Peter Maydell |
Subject: |
Re: [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR |
Date: |
Fri, 10 Dec 2021 15:11:41 +0000 |
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
- [PATCH v4 00/11] Xilinx Versal's PMC SLCR and OSPI support, Francisco Iglesias, 2021/12/01
- [PATCH v4 02/11] hw/arm/xlnx-versal: Connect Versal's PMC SLCR, Francisco Iglesias, 2021/12/01
- [PATCH v4 03/11] include/hw/dma/xlnx_csu_dma: Add in missing includes in the header, Francisco Iglesias, 2021/12/01
- [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR, Francisco Iglesias, 2021/12/01
- Re: [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR,
Peter Maydell <=
- [PATCH v4 05/11] hw/dma/xlnx_csu_dma: Implement the DMA control interface, Francisco Iglesias, 2021/12/01
- [PATCH v4 04/11] hw/dma: Add the DMA control interface, Francisco Iglesias, 2021/12/01
- [PATCH v4 06/11] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller, Francisco Iglesias, 2021/12/01
- [PATCH v4 08/11] hw/block/m25p80: Add support for Micron Xccela flash mt35xu01g, Francisco Iglesias, 2021/12/01
- [PATCH v4 09/11] hw/arm/xlnx-versal-virt: Connect mt35xu01g flashes to the OSPI, Francisco Iglesias, 2021/12/01