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.igles...@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.igles...@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.igles...@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;
