Re: [PATCH v1 3/6] hw/misc: Add a model of the Xilinx ZynqMP CRF

[Edgar E. Iglesias]

On Mon, Jan 31, 2022 at 03:35:57PM +0000, Francisco Iglesias wrote:
> Hi Edgar,
> 
> A couple of minor comments below, looks good to me otherwise!
> 
> On Mon, Jan 31, 2022 at 12:12:03AM +0100, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > Add a model of the Xilinx ZynqMP CRF. At the moment this
> > is mostly a stub model.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  include/hw/misc/xlnx-zynqmp-crf.h | 209 +++++++++++++++++++++++
> >  hw/misc/xlnx-zynqmp-crf.c         | 270 ++++++++++++++++++++++++++++++
> >  hw/misc/meson.build               |   1 +
> >  3 files changed, 480 insertions(+)
> >  create mode 100644 include/hw/misc/xlnx-zynqmp-crf.h
> >  create mode 100644 hw/misc/xlnx-zynqmp-crf.c
> > 
> > diff --git a/include/hw/misc/xlnx-zynqmp-crf.h 
> > b/include/hw/misc/xlnx-zynqmp-crf.h
> > new file mode 100644
> > index 0000000000..b173ea4a08
> > --- /dev/null
> > +++ b/include/hw/misc/xlnx-zynqmp-crf.h
> > @@ -0,0 +1,209 @@
> > +/*
> > + * QEMU model of the CRF - Clock Reset FPD.
> > + *
> > + * Copyright (c) 2022 Xilinx Inc.
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > + */
> > +
> 
> Include guard seem to be missing:
> 
> #ifndef XLNX_ZYNQMP_CRF_H
> #define XLNX_ZYNQMP_CRF_H
> 
> 
> > +#include "hw/sysbus.h"
> > +#include "hw/register.h"
> > +
> > +#define TYPE_XLNX_ZYNQMP_CRF "xlnx.zynqmp_crf"
> > +
> > +#define XILINX_CRF(obj) \
> > +     OBJECT_CHECK(XlnxZynqMPCRF, (obj), TYPE_XLNX_ZYNQMP_CRF)
> > +
> > +REG32(ERR_CTRL, 0x0)
> 
> (Optional but in case the these ones wont be used outside
> xlnx-zynqmp-crf.c we could consider placing them there).


Hi,

Yeah, I thought about that but the issue is with defining:

#define CRF_R_MAX (R_RST_DDR_SS + 1)

We could move these regdefs out and hardcode CRF_R_MAX, e.g:
#define CRF_R_MAX (0x108 / 4)

But that seems error-phrone.
The trade-off is keeping them here and pollute the name-space.
I already to rename one of these macros to avoid conflict...

Does someone have other ideas or preferences?

Cheers,
Edgar

....

> > +REG32(RST_DDR_SS, 0x108)
> > +    FIELD(RST_DDR_SS, DDR_RESET, 3, 1)
> > +    FIELD(RST_DDR_SS, APM_RESET, 2, 1)
> > +
> > +#define CRF_R_MAX (R_RST_DDR_SS + 1)
> > +
> > +typedef struct XlnxZynqMPCRF {
> > +    SysBusDevice parent_obj;
> > +    MemoryRegion iomem;
> > +    qemu_irq irq_ir;
> > +
> > +    RegisterInfoArray *reg_array;
> > +    uint32_t regs[CRF_R_MAX];
> > +    RegisterInfo regs_info[CRF_R_MAX];
> > +} XlnxZynqMPCRF;