[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: |
Francisco Iglesias |
Subject: |
Re: [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR |
Date: |
Tue, 14 Dec 2021 11:01:20 +0000 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Fri, Dec 10, 2021 at 03:11:41PM +0000, Peter Maydell wrote:
> 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.
>
Hi Peter!
> (Also, I don't see anywhere in the board code in patch 2
> that wires these GPIO lines up. Did I miss it?)
In v5, I added dummy connections outputting unimplemented messages for the
GPIOs whose behaviour is not yet implemented (in this series). Hopefully
that will make it more clear (also for users trying to use them)!
Best regards,
Francisco
>
> > +#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
- [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
- [PATCH v4 07/11] hw/arm/xlnx-versal: Connect the OSPI flash memory controller model, Francisco Iglesias, 2021/12/01