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

[Francisco Iglesias <francisco.iglesias@xilinx.com>]

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