Re: [Qemu-devel] [PATCH for-2.5 v2 2/4] mips: add Global Config Register block (part)

[James Hogan]

On Fri, Oct 30, 2015 at 12:36:07AM +0000, James Hogan wrote:
> Hi Yongbok,
> 
> On Tue, Oct 27, 2015 at 05:12:35PM +0000, Yongbok Kim wrote:
> > Add part of GCR Block which Linux Kernel utilises and it is enough
> > to bring the GIC up.
> > It defines full 32 Kbytes address space allocated for GCR but only few
> > registers are implemented such as config, revision, status, L2 config and
> > local config registers.
> > To support MIPS Coherent Manager, this module would be necessary to be 
> > extended
> 
> s/Coherent/Coherence/
> 
> > further.
> > 
> > Signed-off-by: Yongbok Kim <address@hidden>
> > ---
> >  default-configs/mips-softmmu.mak     |    1 +
> >  default-configs/mips64-softmmu.mak   |    1 +
> >  default-configs/mips64el-softmmu.mak |    1 +
> >  default-configs/mipsel-softmmu.mak   |    1 +
> >  hw/misc/Makefile.objs                |    1 +
> >  hw/misc/mips_gcr.c                   |  113 
> > ++++++++++++++++++++++++++++++++++
> >  include/hw/misc/mips_gcr.h           |   58 +++++++++++++++++
> 
> would these be better named mips_cmgcr.{h,c}?
> 
> >  7 files changed, 176 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/misc/mips_gcr.c
> >  create mode 100644 include/hw/misc/mips_gcr.h
> > 
> > diff --git a/default-configs/mips-softmmu.mak 
> > b/default-configs/mips-softmmu.mak
> > index 44467c3..a784644 100644
> > --- a/default-configs/mips-softmmu.mak
> > +++ b/default-configs/mips-softmmu.mak
> > @@ -30,3 +30,4 @@ CONFIG_I8259=y
> >  CONFIG_MC146818RTC=y
> >  CONFIG_ISA_TESTDEV=y
> >  CONFIG_EMPTY_SLOT=y
> > +CONFIG_MIPS_GIC=y
> > diff --git a/default-configs/mips64-softmmu.mak 
> > b/default-configs/mips64-softmmu.mak
> > index 66ed5f9..957508d 100644
> > --- a/default-configs/mips64-softmmu.mak
> > +++ b/default-configs/mips64-softmmu.mak
> > @@ -36,3 +36,4 @@ CONFIG_JAZZ_LED=y
> >  CONFIG_MC146818RTC=y
> >  CONFIG_ISA_TESTDEV=y
> >  CONFIG_EMPTY_SLOT=y
> > +CONFIG_MIPS_GIC=y
> > diff --git a/default-configs/mips64el-softmmu.mak 
> > b/default-configs/mips64el-softmmu.mak
> > index bfca2b2..6c1065f 100644
> > --- a/default-configs/mips64el-softmmu.mak
> > +++ b/default-configs/mips64el-softmmu.mak
> > @@ -39,3 +39,4 @@ CONFIG_MC146818RTC=y
> >  CONFIG_VT82C686=y
> >  CONFIG_ISA_TESTDEV=y
> >  CONFIG_EMPTY_SLOT=y
> > +CONFIG_MIPS_GIC=y
> > diff --git a/default-configs/mipsel-softmmu.mak 
> > b/default-configs/mipsel-softmmu.mak
> > index 0162ef0..4633b0b 100644
> > --- a/default-configs/mipsel-softmmu.mak
> > +++ b/default-configs/mipsel-softmmu.mak
> > @@ -30,3 +30,4 @@ CONFIG_I8259=y
> >  CONFIG_MC146818RTC=y
> >  CONFIG_ISA_TESTDEV=y
> >  CONFIG_EMPTY_SLOT=y
> > +CONFIG_MIPS_GIC=y
> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > index 4aa76ff..02ac5bb 100644
> > --- a/hw/misc/Makefile.objs
> > +++ b/hw/misc/Makefile.objs
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_OMAP) += omap_tap.o
> >  obj-$(CONFIG_SLAVIO) += slavio_misc.o
> >  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> >  obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
> > +obj-$(CONFIG_MIPS_GIC) += mips_gcr.o
> 
> would CONFIG_MIPS_CMGCR be a better name?
> 
> >  
> >  obj-$(CONFIG_PVPANIC) += pvpanic.o
> >  obj-$(CONFIG_EDU) += edu.o
> > diff --git a/hw/misc/mips_gcr.c b/hw/misc/mips_gcr.c
> > new file mode 100644
> > index 0000000..6db4a9d
> > --- /dev/null
> > +++ b/hw/misc/mips_gcr.c
> > @@ -0,0 +1,113 @@
> > +/*
> > + * This file is subject to the terms and conditions of the GNU General 
> > Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + *
> > + * Copyright (C) 2012  MIPS Technologies, Inc.  All rights reserved.
> > + * Authors: Sanjay Lal <address@hidden>
> > + *
> > + * Copyright (C) 2015 Imagination Technologies
> > + */
> > +
> > +#include "hw/hw.h"
> > +#include "hw/sysbus.h"
> > +#include "sysemu/sysemu.h"
> > +#include "hw/misc/mips_gcr.h"
> > +#include "hw/intc/mips_gic.h"
> 
> I don't think this header exists yet at this point in the patchset.
> 
> > +
> > +/* Read GCR registers */
> > +static uint64_t gcr_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    MIPSGCRState *gcr = (MIPSGCRState *) opaque;
> > +
> > +    switch (addr) {
> > +    /* Global Control Block Register */
> > +    case GCR_CONFIG_OFS:
> > +        /* Set PCORES to 0 */
> > +        return 0;
> > +    case GCR_BASE_OFS:
> > +        return gcr->gcr_base;
> > +    case GCR_REV_OFS:
> > +        return gcr->gcr_rev;
> > +    case GCR_GIC_BASE_OFS:
> > +        return gcr->gic_base;

Note also, that this is a read-write register. It starts undefined and
the kernel will write the address it wants it to appear at. The GIC_EN
also resets to 0, and needs setting to 1 by software to enable the GIC
region.

Cheers
James

> > +    case GCR_GIC_STATUS_OFS:
> > +        return GCR_GIC_STATUS_GICEX_MSK;
> 
> well, it doesn't exist yet, does it?
> 
> > +    case GCR_CPC_STATUS_OFS:
> > +        return 0;
> > +    case GCR_L2_CONFIG_OFS:
> > +        /* L2 BYPASS */
> > +        return GCR_L2_CONFIG_BYPASS_MSK;
> > +        /* Core-Local and Core-Other Control Blocks */
> > +    case MIPS_CLCB_OFS + GCR_CL_CONFIG_OFS:
> > +    case MIPS_COCB_OFS + GCR_CL_CONFIG_OFS:
> > +        /* Set PVP to # cores - 1 */
> > +        return gcr->num_vps - 1;
> > +    case MIPS_CLCB_OFS + GCR_CL_OTHER_OFS:
> > +        return 0;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP, "Read %d bytes at GCR offset 0x%" PRIx64 
> > "\n",
> > +                      size, addr);
> > +        return 0;
> > +    }
> > +    return 0;
> > +}
> > +
> > +/* Write GCR registers */
> > +static void gcr_write(void *opaque, hwaddr addr, uint64_t data, unsigned 
> > size)
> > +{
> > +    switch (addr) {
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP, "Write %d bytes at GCR offset 0x%" PRIx64
> > +                      " 0x%08lx\n", size, addr, data);
> > +        break;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps gcr_ops = {
> > +    .read = gcr_read,
> > +    .write = gcr_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .impl = {
> > +        .max_access_size = 8,
> > +    },
> > +};
> > +
> > +static void mips_gcr_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    MIPSGCRState *s = MIPS_GCR(obj);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &gcr_ops, s,
> > +                          "mips-gcr", GCR_ADDRSPACE_SZ);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +}
> > +
> > +static Property mips_gcr_properties[] = {
> > +    DEFINE_PROP_INT32("num-vp", MIPSGCRState, num_vps, 1),
> > +    DEFINE_PROP_INT32("gcr-rev", MIPSGCRState, gcr_rev, 0x800),
> 
> Is this configurable per core?
> 
> > +    DEFINE_PROP_UINT64("gcr-base", MIPSGCRState, gcr_base, GCR_BASE_ADDR),
> > +    DEFINE_PROP_UINT64("gic-base", MIPSGCRState, gic_base, GIC_BASE_ADDR),
> 
> GIC_BASE_ADDR is not defined yet. Maybe this should be added in a later
> patch.
> 
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void mips_gcr_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    dc->props = mips_gcr_properties;
> > +}
> > +
> > +static const TypeInfo mips_gcr_info = {
> > +    .name          = TYPE_MIPS_GCR,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(MIPSGCRState),
> > +    .instance_init = mips_gcr_init,
> > +    .class_init    = mips_gcr_class_init,
> > +};
> > +
> > +static void mips_gcr_register_types(void)
> > +{
> > +    type_register_static(&mips_gcr_info);
> > +}
> > +
> > +type_init(mips_gcr_register_types)
> > diff --git a/include/hw/misc/mips_gcr.h b/include/hw/misc/mips_gcr.h
> > new file mode 100644
> > index 0000000..28f3ca7
> > --- /dev/null
> > +++ b/include/hw/misc/mips_gcr.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * This file is subject to the terms and conditions of the GNU General 
> > Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + *
> > + * Copyright (C) 2015 Imagination Technologies
> > + *
> > + */
> > +
> > +#ifndef _MIPS_GCR_H
> > +#define _MIPS_GCR_H
> > +
> > +#define TYPE_MIPS_GCR "mips-gcr"
> > +#define MIPS_GCR(obj) OBJECT_CHECK(MIPSGCRState, (obj), TYPE_MIPS_GCR)
> > +
> > +#define GCR_BASE_ADDR           0x1fbf8000ULL
> > +#define GCR_ADDRSPACE_SZ        0x8000
> > +
> > +/* Offsets to register blocks */
> > +#define MIPS_GCB_OFS        0x0000 /* Global Control Block */
> > +#define MIPS_CLCB_OFS       0x2000 /* Core Local Control Block */
> > +#define MIPS_COCB_OFS       0x4000 /* Core Other Control Block */
> > +#define MIPS_GDB_OFS        0x6000 /* Global Debug Block */
> > +
> > +/* Global Control Block Register Map */
> > +#define GCR_CONFIG_OFS      0x0000
> > +#define GCR_BASE_OFS        0x0008
> 
> Since QEMU supports XPA, perhaps we should have GCR_BASE_UPPER
> implemented too (like P5600).
> 
> > +#define GCR_REV_OFS         0x0030
> > +#define GCR_GIC_BASE_OFS    0x0080
> 
> Same, GIC_BASE_UPPER?
> 
> > +#define GCR_GIC_STATUS_OFS  0x00D0
> > +#define GCR_CPC_STATUS_OFS  0x00F0
> > +#define GCR_L2_CONFIG_OFS   0x0130
> > +
> > +/* Core Local and Core Other Block Register Map */
> > +#define GCR_CL_CONFIG_OFS   0x0010
> > +#define GCR_CL_OTHER_OFS    0x0018
> > +
> > +/* GCR_GIC_STATUS register fields */
> > +#define GCR_GIC_STATUS_GICEX_SHF    0
> > +#define GCR_GIC_STATUS_GICEX_MSK    ((0x1ULL) << GCR_GIC_STATUS_GICEX_SHF)
> > +
> > +/* GCR_L2_CONFIG register fields */
> > +#define GCR_L2_CONFIG_BYPASS_SHF    20
> > +#define GCR_L2_CONFIG_BYPASS_MSK    ((0x1ULL) << GCR_L2_CONFIG_BYPASS_SHF)
> > +
> > +
> > +typedef struct MIPSGCRState MIPSGCRState;
> > +struct MIPSGCRState {
> > +    SysBusDevice parent_obj;
> > +
> > +    int32_t gcr_rev;
> > +    int32_t num_vps;
> > +    hwaddr gcr_base;
> > +    hwaddr gic_base;
> > +    MemoryRegion iomem;
> > +} ;
> 
> unneeded whitespace before ";"
> 
> Cheers
> James
> 
> > +
> > +#endif /* _MIPS_GCR_H */
> > -- 
> > 1.7.1
> >

signature.asc
Description: Digital signature