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

[Edgar E. Iglesias]

On Mon, Jan 31, 2022 at 06:27:09PM +0100, Edgar E. Iglesias wrote:
> 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?

I guess we could xlnx-zynqmp-crf.h:
#define CRF_R_MAX ((0x108 / 4) + 1)

and in xlnx-zynqmp-crf.c crf_init():
assert((R_RST_DDR_SS + 1) == CRF_R_MAX);

Or something along those lines...


> 
> 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;