| Message ID | 20200709003608.3834629-2-hskinnemoen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines | expand |
On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > Implement a device model for the System Global Control Registers in the > NPCM730 and NPCM750 BMC SoCs. > > This is primarily used to enable SMP boot (the boot ROM spins reading > the SCRPAD register) and DDR memory initialization; other registers are > best effort for now. > > The reset values of the MDLR and PWRON registers are determined by the > SoC variant (730 vs 750) and board straps respectively. > > Reviewed-by: Joel Stanley <joel@jms.id.au> > Reviewed-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > --- > include/hw/misc/npcm7xx_gcr.h | 76 ++++++++++++ > hw/misc/npcm7xx_gcr.c | 226 ++++++++++++++++++++++++++++++++++ > MAINTAINERS | 8 ++ > hw/arm/Kconfig | 3 + > hw/misc/Makefile.objs | 1 + > hw/misc/trace-events | 4 + > 6 files changed, 318 insertions(+) > create mode 100644 include/hw/misc/npcm7xx_gcr.h > create mode 100644 hw/misc/npcm7xx_gcr.c > > diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h > new file mode 100644 > index 0000000000..4884676be2 > --- /dev/null > +++ b/include/hw/misc/npcm7xx_gcr.h > @@ -0,0 +1,76 @@ > +/* > + * Nuvoton NPCM7xx System Global Control Registers. > + * > + * Copyright 2020 Google LLC > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + */ > +#ifndef NPCM7XX_GCR_H > +#define NPCM7XX_GCR_H > + > +#include "exec/memory.h" > +#include "hw/sysbus.h" > + > +enum NPCM7xxGCRRegisters { > + NPCM7XX_GCR_PDID, > + NPCM7XX_GCR_PWRON, > + NPCM7XX_GCR_MFSEL1 = 0x0c / sizeof(uint32_t), > + NPCM7XX_GCR_MFSEL2, > + NPCM7XX_GCR_MISCPE, > + NPCM7XX_GCR_SPSWC = 0x038 / sizeof(uint32_t), > + NPCM7XX_GCR_INTCR, > + NPCM7XX_GCR_INTSR, > + NPCM7XX_GCR_HIFCR = 0x050 / sizeof(uint32_t), > + NPCM7XX_GCR_INTCR2 = 0x060 / sizeof(uint32_t), > + NPCM7XX_GCR_MFSEL3, > + NPCM7XX_GCR_SRCNT, > + NPCM7XX_GCR_RESSR, > + NPCM7XX_GCR_RLOCKR1, > + NPCM7XX_GCR_FLOCKR1, > + NPCM7XX_GCR_DSCNT, > + NPCM7XX_GCR_MDLR, > + NPCM7XX_GCR_SCRPAD3, > + NPCM7XX_GCR_SCRPAD2, > + NPCM7XX_GCR_DAVCLVLR = 0x098 / sizeof(uint32_t), > + NPCM7XX_GCR_INTCR3, > + NPCM7XX_GCR_VSINTR = 0x0ac / sizeof(uint32_t), > + NPCM7XX_GCR_MFSEL4, > + NPCM7XX_GCR_CPBPNTR = 0x0c4 / sizeof(uint32_t), > + NPCM7XX_GCR_CPCTL = 0x0d0 / sizeof(uint32_t), > + NPCM7XX_GCR_CP2BST, > + NPCM7XX_GCR_B2CPNT, > + NPCM7XX_GCR_CPPCTL, > + NPCM7XX_GCR_I2CSEGSEL, > + NPCM7XX_GCR_I2CSEGCTL, > + NPCM7XX_GCR_VSRCR, > + NPCM7XX_GCR_MLOCKR, > + NPCM7XX_GCR_SCRPAD = 0x013c / sizeof(uint32_t), > + NPCM7XX_GCR_USB1PHYCTL, > + NPCM7XX_GCR_USB2PHYCTL, > + NPCM7XX_GCR_NR_REGS, > +}; > + > +typedef struct NPCM7xxGCRState { > + SysBusDevice parent; > + > + MemoryRegion iomem; > + > + uint32_t regs[NPCM7XX_GCR_NR_REGS]; > + > + uint32_t reset_pwron; > + uint32_t reset_mdlr; > + uint32_t reset_intcr3; > +} NPCM7xxGCRState; > + > +#define TYPE_NPCM7XX_GCR "npcm7xx-gcr" > +#define NPCM7XX_GCR(obj) OBJECT_CHECK(NPCM7xxGCRState, (obj), TYPE_NPCM7XX_GCR) > + > +#endif /* NPCM7XX_GCR_H */ > diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c > new file mode 100644 > index 0000000000..9934cd238d > --- /dev/null > +++ b/hw/misc/npcm7xx_gcr.c > @@ -0,0 +1,226 @@ > +/* > + * Nuvoton NPCM7xx System Global Control Registers. > + * > + * Copyright 2020 Google LLC > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + */ > + > +#include "qemu/osdep.h" > + > +#include "hw/misc/npcm7xx_gcr.h" > +#include "hw/qdev-properties.h" > +#include "migration/vmstate.h" > +#include "qapi/error.h" > +#include "qemu/log.h" > +#include "qemu/module.h" > +#include "qemu/units.h" > + > +#include "trace.h" > + > +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { > + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ > + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, > + [NPCM7XX_GCR_SPSWC] = 0x00000003, > + [NPCM7XX_GCR_INTCR] = 0x0000035e, > + [NPCM7XX_GCR_HIFCR] = 0x0000004e, > + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ > + [NPCM7XX_GCR_RESSR] = 0x80000000, > + [NPCM7XX_GCR_DSCNT] = 0x000000c0, > + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, > + [NPCM7XX_GCR_SCRPAD] = 0x00000008, > + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, > + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, > +}; > + > +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) > +{ > + uint32_t reg = offset / sizeof(uint32_t); > + NPCM7xxGCRState *s = opaque; > + > + if (reg >= NPCM7XX_GCR_NR_REGS) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > + __func__, (unsigned int)offset); Maybe use HWADDR_PRIx instead of casting to int? > + return 0; > + } > + > + trace_npcm7xx_gcr_read(offset, s->regs[reg]); > + > + return s->regs[reg]; > +} > + > +static void npcm7xx_gcr_write(void *opaque, hwaddr offset, > + uint64_t v, unsigned size) > +{ > + uint32_t reg = offset / sizeof(uint32_t); > + NPCM7xxGCRState *s = opaque; > + uint32_t value = v; > + > + trace_npcm7xx_gcr_write(offset, value); > + > + if (reg >= NPCM7XX_GCR_NR_REGS) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > + __func__, (unsigned int)offset); > + return; > + } > + > + switch (reg) { > + case NPCM7XX_GCR_PDID: > + case NPCM7XX_GCR_PWRON: > + case NPCM7XX_GCR_INTSR: > + qemu_log_mask(LOG_GUEST_ERROR, "%s: register @ 0x%04x is read-only\n", > + __func__, (unsigned int)offset); > + return; > + > + case NPCM7XX_GCR_RESSR: > + case NPCM7XX_GCR_CP2BST: > + /* Write 1 to clear */ > + value = s->regs[reg] & ~value; > + break; > + > + case NPCM7XX_GCR_RLOCKR1: > + case NPCM7XX_GCR_MDLR: > + /* Write 1 to set */ > + value |= s->regs[reg]; > + break; > + }; > + > + s->regs[reg] = value; > +} > + > +static const struct MemoryRegionOps npcm7xx_gcr_ops = { > + .read = npcm7xx_gcr_read, > + .write = npcm7xx_gcr_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 4, > + .max_access_size = 4, > + .unaligned = false, > + }, > +}; > + > +static void npcm7xx_gcr_enter_reset(Object *obj, ResetType type) 2nd user of this new API, nice :) > +{ > + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); > + > + QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values)); > + > + switch (type) { > + case RESET_TYPE_COLD: > + memcpy(s->regs, cold_reset_values, sizeof(s->regs)); > + s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron; > + s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr; > + s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3; > + break; > + } > +} > + > +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) > +{ > + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); > + uint64_t dram_size; > + Error *err = NULL; > + Object *obj; > + > + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); > + if (!obj) { > + error_setg(errp, "%s: required dram-mr link not found: %s", > + __func__, error_get_pretty(err)); > + return; > + } > + dram_size = memory_region_size(MEMORY_REGION(obj)); > + > + /* Power-on reset value */ > + s->reset_intcr3 = 0x00001002; > + > + /* > + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the > + * DRAM size, and is normally initialized by the boot block as part of DRAM > + * training. However, since we don't have a complete emulation of the > + * memory controller and try to make it look like it has already been > + * initialized, the boot block will skip this initialization, and we need > + * to make sure this field is set correctly up front. > + * > + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or > + * more of DRAM will be interpreted as 128 MiB. I'd say u-boot is right in wrapping at 2GB, as more DRAM is un-addressable. > + * > + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 > + */ > + if (dram_size >= 2 * GiB) { See my comment in this series on the machine patch. > + s->reset_intcr3 |= 4 << 8; > + } else if (dram_size >= 1 * GiB) { > + s->reset_intcr3 |= 3 << 8; > + } else if (dram_size >= 512 * MiB) { > + s->reset_intcr3 |= 2 << 8; > + } else if (dram_size >= 256 * MiB) { > + s->reset_intcr3 |= 1 << 8; > + } else if (dram_size >= 128 * MiB) { > + s->reset_intcr3 |= 0 << 8; > + } else { > + error_setg(errp, > + "npcm7xx_gcr: DRAM size %" PRIu64 > + " is too small (need 128 MiB minimum)", > + dram_size); Ah, so you could add the same error for >2GB. Easier. Preferably using HWADDR_PRIx, and similar error for >2GB: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > + return; > + } > +} > + > +static void npcm7xx_gcr_init(Object *obj) > +{ > + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); > + > + memory_region_init_io(&s->iomem, obj, &npcm7xx_gcr_ops, s, > + TYPE_NPCM7XX_GCR, 4 * KiB); > + sysbus_init_mmio(&s->parent, &s->iomem); > +} > + > +static const VMStateDescription vmstate_npcm7xx_gcr = { > + .name = "npcm7xx-gcr", > + .version_id = 0, > + .minimum_version_id = 0, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32_ARRAY(regs, NPCM7xxGCRState, NPCM7XX_GCR_NR_REGS), > + VMSTATE_END_OF_LIST(), > + }, > +}; > + > +static Property npcm7xx_gcr_properties[] = { > + DEFINE_PROP_UINT32("disabled-modules", NPCM7xxGCRState, reset_mdlr, 0), > + DEFINE_PROP_UINT32("power-on-straps", NPCM7xxGCRState, reset_pwron, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void npcm7xx_gcr_class_init(ObjectClass *klass, void *data) > +{ > + ResettableClass *rc = RESETTABLE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->desc = "NPCM7xx System Global Control Registers"; > + dc->realize = npcm7xx_gcr_realize; > + dc->vmsd = &vmstate_npcm7xx_gcr; > + rc->phases.enter = npcm7xx_gcr_enter_reset; > + > + device_class_set_props(dc, npcm7xx_gcr_properties); > +} > + > +static const TypeInfo npcm7xx_gcr_info = { > + .name = TYPE_NPCM7XX_GCR, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(NPCM7xxGCRState), > + .instance_init = npcm7xx_gcr_init, > + .class_init = npcm7xx_gcr_class_init, > +}; > + > +static void npcm7xx_gcr_register_type(void) > +{ > + type_register_static(&npcm7xx_gcr_info); > +} > +type_init(npcm7xx_gcr_register_type); > diff --git a/MAINTAINERS b/MAINTAINERS > index 42388f1de2..43173a338a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -723,6 +723,14 @@ S: Odd Fixes > F: hw/arm/musicpal.c > F: docs/system/arm/musicpal.rst > > +Nuvoton NPCM7xx > +M: Havard Skinnemoen <hskinnemoen@google.com> > +M: Tyrone Ting <kfting@nuvoton.com> > +L: qemu-arm@nongnu.org > +S: Supported > +F: hw/*/npcm7xx* > +F: include/hw/*/npcm7xx* > + > nSeries > M: Andrzej Zaborowski <balrogg@gmail.com> > M: Peter Maydell <peter.maydell@linaro.org> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index 4a224a6351..192a8dec3b 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -354,6 +354,9 @@ config XLNX_VERSAL > select VIRTIO_MMIO > select UNIMP > > +config NPCM7XX > + bool > + > config FSL_IMX25 > bool > select IMX > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > index 5aaca8a039..40a9d1c01e 100644 > --- a/hw/misc/Makefile.objs > +++ b/hw/misc/Makefile.objs > @@ -51,6 +51,7 @@ common-obj-$(CONFIG_IMX) += imx_rngc.o > common-obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o > common-obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o > common-obj-$(CONFIG_MAINSTONE) += mst_fpga.o > +common-obj-$(CONFIG_NPCM7XX) += npcm7xx_gcr.o > common-obj-$(CONFIG_OMAP) += omap_clk.o > common-obj-$(CONFIG_OMAP) += omap_gpmc.o > common-obj-$(CONFIG_OMAP) += omap_l4.o > diff --git a/hw/misc/trace-events b/hw/misc/trace-events > index ebea53735c..48e2d54c49 100644 > --- a/hw/misc/trace-events > +++ b/hw/misc/trace-events > @@ -107,6 +107,10 @@ mos6522_set_sr_int(void) "set sr_int" > mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64 > mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x" > > +# npcm7xx_gcr.c > +npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 > +npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 > + > # stm32f4xx_syscfg > stm32f4xx_syscfg_set_irq(int gpio, int line, int level) "Interupt: GPIO: %d, Line: %d; Level: %d" > stm32f4xx_pulse_exti(int irq) "Pulse EXTI: %d" >
On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > > Implement a device model for the System Global Control Registers in the > > NPCM730 and NPCM750 BMC SoCs. > > > > This is primarily used to enable SMP boot (the boot ROM spins reading > > the SCRPAD register) and DDR memory initialization; other registers are > > best effort for now. > > > > The reset values of the MDLR and PWRON registers are determined by the > > SoC variant (730 vs 750) and board straps respectively. > > > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > > --- > > include/hw/misc/npcm7xx_gcr.h | 76 ++++++++++++ > > hw/misc/npcm7xx_gcr.c | 226 ++++++++++++++++++++++++++++++++++ > > MAINTAINERS | 8 ++ > > hw/arm/Kconfig | 3 + > > hw/misc/Makefile.objs | 1 + > > hw/misc/trace-events | 4 + > > 6 files changed, 318 insertions(+) > > create mode 100644 include/hw/misc/npcm7xx_gcr.h > > create mode 100644 hw/misc/npcm7xx_gcr.c > > > > diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h > > new file mode 100644 > > index 0000000000..4884676be2 > > --- /dev/null > > +++ b/include/hw/misc/npcm7xx_gcr.h > > @@ -0,0 +1,76 @@ > > +/* > > + * Nuvoton NPCM7xx System Global Control Registers. > > + * > > + * Copyright 2020 Google LLC > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > + * for more details. > > + */ > > +#ifndef NPCM7XX_GCR_H > > +#define NPCM7XX_GCR_H > > + > > +#include "exec/memory.h" > > +#include "hw/sysbus.h" > > + > > +enum NPCM7xxGCRRegisters { > > + NPCM7XX_GCR_PDID, > > + NPCM7XX_GCR_PWRON, > > + NPCM7XX_GCR_MFSEL1 = 0x0c / sizeof(uint32_t), > > + NPCM7XX_GCR_MFSEL2, > > + NPCM7XX_GCR_MISCPE, > > + NPCM7XX_GCR_SPSWC = 0x038 / sizeof(uint32_t), > > + NPCM7XX_GCR_INTCR, > > + NPCM7XX_GCR_INTSR, > > + NPCM7XX_GCR_HIFCR = 0x050 / sizeof(uint32_t), > > + NPCM7XX_GCR_INTCR2 = 0x060 / sizeof(uint32_t), > > + NPCM7XX_GCR_MFSEL3, > > + NPCM7XX_GCR_SRCNT, > > + NPCM7XX_GCR_RESSR, > > + NPCM7XX_GCR_RLOCKR1, > > + NPCM7XX_GCR_FLOCKR1, > > + NPCM7XX_GCR_DSCNT, > > + NPCM7XX_GCR_MDLR, > > + NPCM7XX_GCR_SCRPAD3, > > + NPCM7XX_GCR_SCRPAD2, > > + NPCM7XX_GCR_DAVCLVLR = 0x098 / sizeof(uint32_t), > > + NPCM7XX_GCR_INTCR3, > > + NPCM7XX_GCR_VSINTR = 0x0ac / sizeof(uint32_t), > > + NPCM7XX_GCR_MFSEL4, > > + NPCM7XX_GCR_CPBPNTR = 0x0c4 / sizeof(uint32_t), > > + NPCM7XX_GCR_CPCTL = 0x0d0 / sizeof(uint32_t), > > + NPCM7XX_GCR_CP2BST, > > + NPCM7XX_GCR_B2CPNT, > > + NPCM7XX_GCR_CPPCTL, > > + NPCM7XX_GCR_I2CSEGSEL, > > + NPCM7XX_GCR_I2CSEGCTL, > > + NPCM7XX_GCR_VSRCR, > > + NPCM7XX_GCR_MLOCKR, > > + NPCM7XX_GCR_SCRPAD = 0x013c / sizeof(uint32_t), > > + NPCM7XX_GCR_USB1PHYCTL, > > + NPCM7XX_GCR_USB2PHYCTL, > > + NPCM7XX_GCR_NR_REGS, > > +}; > > + > > +typedef struct NPCM7xxGCRState { > > + SysBusDevice parent; > > + > > + MemoryRegion iomem; > > + > > + uint32_t regs[NPCM7XX_GCR_NR_REGS]; > > + > > + uint32_t reset_pwron; > > + uint32_t reset_mdlr; > > + uint32_t reset_intcr3; > > +} NPCM7xxGCRState; > > + > > +#define TYPE_NPCM7XX_GCR "npcm7xx-gcr" > > +#define NPCM7XX_GCR(obj) OBJECT_CHECK(NPCM7xxGCRState, (obj), TYPE_NPCM7XX_GCR) > > + > > +#endif /* NPCM7XX_GCR_H */ > > diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c > > new file mode 100644 > > index 0000000000..9934cd238d > > --- /dev/null > > +++ b/hw/misc/npcm7xx_gcr.c > > @@ -0,0 +1,226 @@ > > +/* > > + * Nuvoton NPCM7xx System Global Control Registers. > > + * > > + * Copyright 2020 Google LLC > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, but WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > + * for more details. > > + */ > > + > > +#include "qemu/osdep.h" > > + > > +#include "hw/misc/npcm7xx_gcr.h" > > +#include "hw/qdev-properties.h" > > +#include "migration/vmstate.h" > > +#include "qapi/error.h" > > +#include "qemu/log.h" > > +#include "qemu/module.h" > > +#include "qemu/units.h" > > + > > +#include "trace.h" > > + > > +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { > > + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ > > + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, > > + [NPCM7XX_GCR_SPSWC] = 0x00000003, > > + [NPCM7XX_GCR_INTCR] = 0x0000035e, > > + [NPCM7XX_GCR_HIFCR] = 0x0000004e, > > + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ > > + [NPCM7XX_GCR_RESSR] = 0x80000000, > > + [NPCM7XX_GCR_DSCNT] = 0x000000c0, > > + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, > > + [NPCM7XX_GCR_SCRPAD] = 0x00000008, > > + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, > > + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, > > +}; > > + > > +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) > > +{ > > + uint32_t reg = offset / sizeof(uint32_t); > > + NPCM7xxGCRState *s = opaque; > > + > > + if (reg >= NPCM7XX_GCR_NR_REGS) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > > + __func__, (unsigned int)offset); > > Maybe use HWADDR_PRIx instead of casting to int? Will do, thanks! > > > + return 0; > > + } > > + > > + trace_npcm7xx_gcr_read(offset, s->regs[reg]); > > + > > + return s->regs[reg]; > > +} > > + > > +static void npcm7xx_gcr_write(void *opaque, hwaddr offset, > > + uint64_t v, unsigned size) > > +{ > > + uint32_t reg = offset / sizeof(uint32_t); > > + NPCM7xxGCRState *s = opaque; > > + uint32_t value = v; > > + > > + trace_npcm7xx_gcr_write(offset, value); > > + > > + if (reg >= NPCM7XX_GCR_NR_REGS) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > > + __func__, (unsigned int)offset); > > + return; > > + } > > + > > + switch (reg) { > > + case NPCM7XX_GCR_PDID: > > + case NPCM7XX_GCR_PWRON: > > + case NPCM7XX_GCR_INTSR: > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: register @ 0x%04x is read-only\n", > > + __func__, (unsigned int)offset); > > + return; > > + > > + case NPCM7XX_GCR_RESSR: > > + case NPCM7XX_GCR_CP2BST: > > + /* Write 1 to clear */ > > + value = s->regs[reg] & ~value; > > + break; > > + > > + case NPCM7XX_GCR_RLOCKR1: > > + case NPCM7XX_GCR_MDLR: > > + /* Write 1 to set */ > > + value |= s->regs[reg]; > > + break; > > + }; > > + > > + s->regs[reg] = value; > > +} > > + > > +static const struct MemoryRegionOps npcm7xx_gcr_ops = { > > + .read = npcm7xx_gcr_read, > > + .write = npcm7xx_gcr_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid = { > > + .min_access_size = 4, > > + .max_access_size = 4, > > + .unaligned = false, > > + }, > > +}; > > + > > +static void npcm7xx_gcr_enter_reset(Object *obj, ResetType type) > > 2nd user of this new API, nice :) > > > +{ > > + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); > > + > > + QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values)); > > + > > + switch (type) { > > + case RESET_TYPE_COLD: > > + memcpy(s->regs, cold_reset_values, sizeof(s->regs)); > > + s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron; > > + s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr; > > + s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3; > > + break; > > + } > > +} > > + > > +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) > > +{ > > + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); > > + uint64_t dram_size; > > + Error *err = NULL; > > + Object *obj; > > + > > + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); > > + if (!obj) { > > + error_setg(errp, "%s: required dram-mr link not found: %s", > > + __func__, error_get_pretty(err)); > > + return; > > + } > > + dram_size = memory_region_size(MEMORY_REGION(obj)); > > + > > + /* Power-on reset value */ > > + s->reset_intcr3 = 0x00001002; > > + > > + /* > > + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the > > + * DRAM size, and is normally initialized by the boot block as part of DRAM > > + * training. However, since we don't have a complete emulation of the > > + * memory controller and try to make it look like it has already been > > + * initialized, the boot block will skip this initialization, and we need > > + * to make sure this field is set correctly up front. > > + * > > + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or > > + * more of DRAM will be interpreted as 128 MiB. > > I'd say u-boot is right in wrapping at 2GB, as more DRAM is > un-addressable. Ah, maybe I shouldn't have said "or more". The bug is that if you specify _exactly_ 2GiB, u-boot will see 128 MiB. But I agree more than 2GiB doesn't make sense, so I'll add a check for that. Not sure if I agree that the check should be here. > 2 GiB is an addressing limitation, and the GCR module shouldn't really know what the SoC's address space looks like. The lower limit is because the GCR module can't encode anything less than 128 MiB. > > + * > > + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 > > + */ > > + if (dram_size >= 2 * GiB) { > > See my comment in this series on the machine patch. > > > + s->reset_intcr3 |= 4 << 8; > > + } else if (dram_size >= 1 * GiB) { > > + s->reset_intcr3 |= 3 << 8; > > + } else if (dram_size >= 512 * MiB) { > > + s->reset_intcr3 |= 2 << 8; > > + } else if (dram_size >= 256 * MiB) { > > + s->reset_intcr3 |= 1 << 8; > > + } else if (dram_size >= 128 * MiB) { > > + s->reset_intcr3 |= 0 << 8; > > + } else { > > + error_setg(errp, > > + "npcm7xx_gcr: DRAM size %" PRIu64 > > + " is too small (need 128 MiB minimum)", > > + dram_size); > > Ah, so you could add the same error for >2GB. Easier. > > Preferably using HWADDR_PRIx, and similar error for >2GB: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > + return; > > + } > > +} > > + > > +static void npcm7xx_gcr_init(Object *obj) > > +{ > > + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); > > + > > + memory_region_init_io(&s->iomem, obj, &npcm7xx_gcr_ops, s, > > + TYPE_NPCM7XX_GCR, 4 * KiB); > > + sysbus_init_mmio(&s->parent, &s->iomem); > > +} > > + > > +static const VMStateDescription vmstate_npcm7xx_gcr = { > > + .name = "npcm7xx-gcr", > > + .version_id = 0, > > + .minimum_version_id = 0, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT32_ARRAY(regs, NPCM7xxGCRState, NPCM7XX_GCR_NR_REGS), > > + VMSTATE_END_OF_LIST(), > > + }, > > +}; > > + > > +static Property npcm7xx_gcr_properties[] = { > > + DEFINE_PROP_UINT32("disabled-modules", NPCM7xxGCRState, reset_mdlr, 0), > > + DEFINE_PROP_UINT32("power-on-straps", NPCM7xxGCRState, reset_pwron, 0), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void npcm7xx_gcr_class_init(ObjectClass *klass, void *data) > > +{ > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->desc = "NPCM7xx System Global Control Registers"; > > + dc->realize = npcm7xx_gcr_realize; > > + dc->vmsd = &vmstate_npcm7xx_gcr; > > + rc->phases.enter = npcm7xx_gcr_enter_reset; > > + > > + device_class_set_props(dc, npcm7xx_gcr_properties); > > +} > > + > > +static const TypeInfo npcm7xx_gcr_info = { > > + .name = TYPE_NPCM7XX_GCR, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(NPCM7xxGCRState), > > + .instance_init = npcm7xx_gcr_init, > > + .class_init = npcm7xx_gcr_class_init, > > +}; > > + > > +static void npcm7xx_gcr_register_type(void) > > +{ > > + type_register_static(&npcm7xx_gcr_info); > > +} > > +type_init(npcm7xx_gcr_register_type); > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 42388f1de2..43173a338a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -723,6 +723,14 @@ S: Odd Fixes > > F: hw/arm/musicpal.c > > F: docs/system/arm/musicpal.rst > > > > +Nuvoton NPCM7xx > > +M: Havard Skinnemoen <hskinnemoen@google.com> > > +M: Tyrone Ting <kfting@nuvoton.com> > > +L: qemu-arm@nongnu.org > > +S: Supported > > +F: hw/*/npcm7xx* > > +F: include/hw/*/npcm7xx* > > + > > nSeries > > M: Andrzej Zaborowski <balrogg@gmail.com> > > M: Peter Maydell <peter.maydell@linaro.org> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > > index 4a224a6351..192a8dec3b 100644 > > --- a/hw/arm/Kconfig > > +++ b/hw/arm/Kconfig > > @@ -354,6 +354,9 @@ config XLNX_VERSAL > > select VIRTIO_MMIO > > select UNIMP > > > > +config NPCM7XX > > + bool > > + > > config FSL_IMX25 > > bool > > select IMX > > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > > index 5aaca8a039..40a9d1c01e 100644 > > --- a/hw/misc/Makefile.objs > > +++ b/hw/misc/Makefile.objs > > @@ -51,6 +51,7 @@ common-obj-$(CONFIG_IMX) += imx_rngc.o > > common-obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o > > common-obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o > > common-obj-$(CONFIG_MAINSTONE) += mst_fpga.o > > +common-obj-$(CONFIG_NPCM7XX) += npcm7xx_gcr.o > > common-obj-$(CONFIG_OMAP) += omap_clk.o > > common-obj-$(CONFIG_OMAP) += omap_gpmc.o > > common-obj-$(CONFIG_OMAP) += omap_l4.o > > diff --git a/hw/misc/trace-events b/hw/misc/trace-events > > index ebea53735c..48e2d54c49 100644 > > --- a/hw/misc/trace-events > > +++ b/hw/misc/trace-events > > @@ -107,6 +107,10 @@ mos6522_set_sr_int(void) "set sr_int" > > mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64 > > mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x" > > > > +# npcm7xx_gcr.c > > +npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 > > +npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 > > + > > # stm32f4xx_syscfg > > stm32f4xx_syscfg_set_irq(int gpio, int line, int level) "Interupt: GPIO: %d, Line: %d; Level: %d" > > stm32f4xx_pulse_exti(int irq) "Pulse EXTI: %d" > >
On 7/9/20 8:43 AM, Havard Skinnemoen wrote: > On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: >>> Implement a device model for the System Global Control Registers in the >>> NPCM730 and NPCM750 BMC SoCs. >>> >>> This is primarily used to enable SMP boot (the boot ROM spins reading >>> the SCRPAD register) and DDR memory initialization; other registers are >>> best effort for now. >>> >>> The reset values of the MDLR and PWRON registers are determined by the >>> SoC variant (730 vs 750) and board straps respectively. >>> >>> Reviewed-by: Joel Stanley <joel@jms.id.au> >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> >>> --- [...] >>> diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c >>> new file mode 100644 >>> index 0000000000..9934cd238d >>> --- /dev/null >>> +++ b/hw/misc/npcm7xx_gcr.c >>> @@ -0,0 +1,226 @@ >>> +/* >>> + * Nuvoton NPCM7xx System Global Control Registers. >>> + * >>> + * Copyright 2020 Google LLC >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms of the GNU General Public License as published by the >>> + * Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that it will be useful, but WITHOUT >>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >>> + * for more details. >>> + */ >>> + >>> +#include "qemu/osdep.h" >>> + >>> +#include "hw/misc/npcm7xx_gcr.h" >>> +#include "hw/qdev-properties.h" >>> +#include "migration/vmstate.h" >>> +#include "qapi/error.h" >>> +#include "qemu/log.h" >>> +#include "qemu/module.h" >>> +#include "qemu/units.h" >>> + >>> +#include "trace.h" >>> + >>> +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { >>> + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ >>> + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, >>> + [NPCM7XX_GCR_SPSWC] = 0x00000003, >>> + [NPCM7XX_GCR_INTCR] = 0x0000035e, >>> + [NPCM7XX_GCR_HIFCR] = 0x0000004e, >>> + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ >>> + [NPCM7XX_GCR_RESSR] = 0x80000000, >>> + [NPCM7XX_GCR_DSCNT] = 0x000000c0, >>> + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, >>> + [NPCM7XX_GCR_SCRPAD] = 0x00000008, >>> + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, >>> + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, >>> +}; >>> + >>> +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) >>> +{ >>> + uint32_t reg = offset / sizeof(uint32_t); >>> + NPCM7xxGCRState *s = opaque; >>> + >>> + if (reg >= NPCM7XX_GCR_NR_REGS) { >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", >>> + __func__, (unsigned int)offset); >> >> Maybe use HWADDR_PRIx instead of casting to int? > > Will do, thanks! Glad you are not annoyed by my review comments. FYI your series quality is in my top-4 "adding new machine to QEMU". I'd like to use it as example to follow. I am slowly reviewing because this is not part of my work duties, so I do that in my free time before/after work, which is why I can barely do more that 2 per day, as I have to learn the SoC and cross check documentation (or in this case, existing firmware code since there is no public doc). [...] >>> +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) >>> +{ >>> + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); >>> + uint64_t dram_size; >>> + Error *err = NULL; >>> + Object *obj; >>> + >>> + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); >>> + if (!obj) { >>> + error_setg(errp, "%s: required dram-mr link not found: %s", >>> + __func__, error_get_pretty(err)); >>> + return; >>> + } >>> + dram_size = memory_region_size(MEMORY_REGION(obj)); >>> + >>> + /* Power-on reset value */ >>> + s->reset_intcr3 = 0x00001002; >>> + >>> + /* >>> + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the >>> + * DRAM size, and is normally initialized by the boot block as part of DRAM >>> + * training. However, since we don't have a complete emulation of the >>> + * memory controller and try to make it look like it has already been >>> + * initialized, the boot block will skip this initialization, and we need >>> + * to make sure this field is set correctly up front. >>> + * >>> + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or >>> + * more of DRAM will be interpreted as 128 MiB. >> >> I'd say u-boot is right in wrapping at 2GB, as more DRAM is >> un-addressable. > > Ah, maybe I shouldn't have said "or more". The bug is that if you > specify _exactly_ 2GiB, u-boot will see 128 MiB. > > But I agree more than 2GiB doesn't make sense, so I'll add a check for that. > > Not sure if I agree that the check should be here. > 2 GiB is an > addressing limitation, and the GCR module shouldn't really know what > the SoC's address space looks like. The lower limit is because the GCR > module can't encode anything less than 128 MiB. > >>> + * >>> + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 >>> + */ >>> + if (dram_size >= 2 * GiB) { >> >> See my comment in this series on the machine patch. >> >>> + s->reset_intcr3 |= 4 << 8; >>> + } else if (dram_size >= 1 * GiB) { >>> + s->reset_intcr3 |= 3 << 8; >>> + } else if (dram_size >= 512 * MiB) { >>> + s->reset_intcr3 |= 2 << 8; >>> + } else if (dram_size >= 256 * MiB) { >>> + s->reset_intcr3 |= 1 << 8; >>> + } else if (dram_size >= 128 * MiB) { >>> + s->reset_intcr3 |= 0 << 8; I think this can be simplified as: s->reset_intcr3 = (4 - clz32(dram_size)) << 8; Which implicitly truncate to power of 2 (which is what this block does, as you can have only 1 bank managed per this SGC). But checking is_power_of_2(dram_size) and reporting an error else, is probably even better. >>> + } else { >>> + error_setg(errp, >>> + "npcm7xx_gcr: DRAM size %" PRIu64 >>> + " is too small (need 128 MiB minimum)", >>> + dram_size); >> >> Ah, so you could add the same error for >2GB. Easier. >> >> Preferably using HWADDR_PRIx, and similar error for >2GB: >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/9/20 8:43 AM, Havard Skinnemoen wrote: > > On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > >>> Implement a device model for the System Global Control Registers in the > >>> NPCM730 and NPCM750 BMC SoCs. > >>> > >>> This is primarily used to enable SMP boot (the boot ROM spins reading > >>> the SCRPAD register) and DDR memory initialization; other registers are > >>> best effort for now. > >>> > >>> The reset values of the MDLR and PWRON registers are determined by the > >>> SoC variant (730 vs 750) and board straps respectively. > >>> > >>> Reviewed-by: Joel Stanley <joel@jms.id.au> > >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> > >>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > >>> --- > [...] > >>> diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c > >>> new file mode 100644 > >>> index 0000000000..9934cd238d > >>> --- /dev/null > >>> +++ b/hw/misc/npcm7xx_gcr.c > >>> @@ -0,0 +1,226 @@ > >>> +/* > >>> + * Nuvoton NPCM7xx System Global Control Registers. > >>> + * > >>> + * Copyright 2020 Google LLC > >>> + * > >>> + * This program is free software; you can redistribute it and/or modify it > >>> + * under the terms of the GNU General Public License as published by the > >>> + * Free Software Foundation; either version 2 of the License, or > >>> + * (at your option) any later version. > >>> + * > >>> + * This program is distributed in the hope that it will be useful, but WITHOUT > >>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > >>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > >>> + * for more details. > >>> + */ > >>> + > >>> +#include "qemu/osdep.h" > >>> + > >>> +#include "hw/misc/npcm7xx_gcr.h" > >>> +#include "hw/qdev-properties.h" > >>> +#include "migration/vmstate.h" > >>> +#include "qapi/error.h" > >>> +#include "qemu/log.h" > >>> +#include "qemu/module.h" > >>> +#include "qemu/units.h" > >>> + > >>> +#include "trace.h" > >>> + > >>> +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { > >>> + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ > >>> + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, > >>> + [NPCM7XX_GCR_SPSWC] = 0x00000003, > >>> + [NPCM7XX_GCR_INTCR] = 0x0000035e, > >>> + [NPCM7XX_GCR_HIFCR] = 0x0000004e, > >>> + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ > >>> + [NPCM7XX_GCR_RESSR] = 0x80000000, > >>> + [NPCM7XX_GCR_DSCNT] = 0x000000c0, > >>> + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, > >>> + [NPCM7XX_GCR_SCRPAD] = 0x00000008, > >>> + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, > >>> + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, > >>> +}; > >>> + > >>> +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) > >>> +{ > >>> + uint32_t reg = offset / sizeof(uint32_t); > >>> + NPCM7xxGCRState *s = opaque; > >>> + > >>> + if (reg >= NPCM7XX_GCR_NR_REGS) { > >>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > >>> + __func__, (unsigned int)offset); > >> > >> Maybe use HWADDR_PRIx instead of casting to int? > > > > Will do, thanks! > > Glad you are not annoyed by my review comments. > > FYI your series quality is in my top-4 "adding new machine to QEMU". > I'd like to use it as example to follow. > > I am slowly reviewing because this is not part of my work duties, > so I do that in my free time before/after work, which is why I can > barely do more that 2 per day, as I have to learn the SoC and > cross check documentation (or in this case, existing firmware code > since there is no public doc). Your feedback is super valuable, thanks a lot. I really appreciate it. > > [...] > >>> +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) > >>> +{ > >>> + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); > >>> + uint64_t dram_size; > >>> + Error *err = NULL; > >>> + Object *obj; > >>> + > >>> + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); > >>> + if (!obj) { > >>> + error_setg(errp, "%s: required dram-mr link not found: %s", > >>> + __func__, error_get_pretty(err)); > >>> + return; > >>> + } > >>> + dram_size = memory_region_size(MEMORY_REGION(obj)); > >>> + > >>> + /* Power-on reset value */ > >>> + s->reset_intcr3 = 0x00001002; > >>> + > >>> + /* > >>> + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the > >>> + * DRAM size, and is normally initialized by the boot block as part of DRAM > >>> + * training. However, since we don't have a complete emulation of the > >>> + * memory controller and try to make it look like it has already been > >>> + * initialized, the boot block will skip this initialization, and we need > >>> + * to make sure this field is set correctly up front. > >>> + * > >>> + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or > >>> + * more of DRAM will be interpreted as 128 MiB. > >> > >> I'd say u-boot is right in wrapping at 2GB, as more DRAM is > >> un-addressable. > > > > Ah, maybe I shouldn't have said "or more". The bug is that if you > > specify _exactly_ 2GiB, u-boot will see 128 MiB. > > > > But I agree more than 2GiB doesn't make sense, so I'll add a check for that. > > > > Not sure if I agree that the check should be here. > 2 GiB is an > > addressing limitation, and the GCR module shouldn't really know what > > the SoC's address space looks like. The lower limit is because the GCR > > module can't encode anything less than 128 MiB. > > > >>> + * > >>> + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 > >>> + */ > >>> + if (dram_size >= 2 * GiB) { > >> > >> See my comment in this series on the machine patch. > >> > >>> + s->reset_intcr3 |= 4 << 8; > >>> + } else if (dram_size >= 1 * GiB) { > >>> + s->reset_intcr3 |= 3 << 8; > >>> + } else if (dram_size >= 512 * MiB) { > >>> + s->reset_intcr3 |= 2 << 8; > >>> + } else if (dram_size >= 256 * MiB) { > >>> + s->reset_intcr3 |= 1 << 8; > >>> + } else if (dram_size >= 128 * MiB) { > >>> + s->reset_intcr3 |= 0 << 8; > > I think this can be simplified as: > > s->reset_intcr3 = (4 - clz32(dram_size)) << 8; > > Which implicitly truncate to power of 2 (which is what this > block does, as you can have only 1 bank managed per this SGC). Good idea. I find this a little easier to understand though: #define NPCM7XX_GCR_MIN_DRAM_SIZE (128 * MiB) s->reset_intcr3 |= ctz64(dram_size / NPCM7XX_GCR_MIN_DRAM_SIZE) << 8; > But checking is_power_of_2(dram_size) and reporting an error > else, is probably even better. OK, I'll add a check for this, and also explicit checks for too large and too small. The former is currently redundant with the DRAM size check I'm adding to npcm7xx_realize(), but I kind of like the idea of checking for encoding limitations and addressing limitations separately, as they may not necessarily stay the same forever. > >>> + } else { > >>> + error_setg(errp, > >>> + "npcm7xx_gcr: DRAM size %" PRIu64 > >>> + " is too small (need 128 MiB minimum)", > >>> + dram_size); > >> > >> Ah, so you could add the same error for >2GB. Easier. > >> > >> Preferably using HWADDR_PRIx, and similar error for >2GB: > >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 7/9/20 7:09 PM, Havard Skinnemoen wrote: > On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> On 7/9/20 8:43 AM, Havard Skinnemoen wrote: >>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: >>>>> Implement a device model for the System Global Control Registers in the >>>>> NPCM730 and NPCM750 BMC SoCs. >>>>> >>>>> This is primarily used to enable SMP boot (the boot ROM spins reading >>>>> the SCRPAD register) and DDR memory initialization; other registers are >>>>> best effort for now. >>>>> >>>>> The reset values of the MDLR and PWRON registers are determined by the >>>>> SoC variant (730 vs 750) and board straps respectively. >>>>> >>>>> Reviewed-by: Joel Stanley <joel@jms.id.au> >>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> >>>>> --- >> [...] >>>>> diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c >>>>> new file mode 100644 >>>>> index 0000000000..9934cd238d >>>>> --- /dev/null >>>>> +++ b/hw/misc/npcm7xx_gcr.c >>>>> @@ -0,0 +1,226 @@ >>>>> +/* >>>>> + * Nuvoton NPCM7xx System Global Control Registers. >>>>> + * >>>>> + * Copyright 2020 Google LLC >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify it >>>>> + * under the terms of the GNU General Public License as published by the >>>>> + * Free Software Foundation; either version 2 of the License, or >>>>> + * (at your option) any later version. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, but WITHOUT >>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >>>>> + * for more details. >>>>> + */ >>>>> + >>>>> +#include "qemu/osdep.h" >>>>> + >>>>> +#include "hw/misc/npcm7xx_gcr.h" >>>>> +#include "hw/qdev-properties.h" >>>>> +#include "migration/vmstate.h" >>>>> +#include "qapi/error.h" >>>>> +#include "qemu/log.h" >>>>> +#include "qemu/module.h" >>>>> +#include "qemu/units.h" >>>>> + >>>>> +#include "trace.h" >>>>> + >>>>> +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { >>>>> + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ >>>>> + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, >>>>> + [NPCM7XX_GCR_SPSWC] = 0x00000003, >>>>> + [NPCM7XX_GCR_INTCR] = 0x0000035e, >>>>> + [NPCM7XX_GCR_HIFCR] = 0x0000004e, >>>>> + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ >>>>> + [NPCM7XX_GCR_RESSR] = 0x80000000, >>>>> + [NPCM7XX_GCR_DSCNT] = 0x000000c0, >>>>> + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, >>>>> + [NPCM7XX_GCR_SCRPAD] = 0x00000008, >>>>> + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, >>>>> + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, >>>>> +}; >>>>> + >>>>> +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) >>>>> +{ >>>>> + uint32_t reg = offset / sizeof(uint32_t); >>>>> + NPCM7xxGCRState *s = opaque; >>>>> + >>>>> + if (reg >= NPCM7XX_GCR_NR_REGS) { >>>>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", >>>>> + __func__, (unsigned int)offset); >>>> >>>> Maybe use HWADDR_PRIx instead of casting to int? >>> >>> Will do, thanks! >> >> Glad you are not annoyed by my review comments. >> >> FYI your series quality is in my top-4 "adding new machine to QEMU". >> I'd like to use it as example to follow. >> >> I am slowly reviewing because this is not part of my work duties, >> so I do that in my free time before/after work, which is why I can >> barely do more that 2 per day, as I have to learn the SoC and >> cross check documentation (or in this case, existing firmware code >> since there is no public doc). > > Your feedback is super valuable, thanks a lot. I really appreciate it. OK I'll continue, but might not have time until next week. After I plan to test too. What would be useful is an acceptance test (see examples in tests/acceptance/), using the artefacts from the link you shared elsewhere: https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/ But these are apparently not stable links (expire after 30 days?). >> [...] >>>>> +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) >>>>> +{ >>>>> + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); >>>>> + uint64_t dram_size; >>>>> + Error *err = NULL; >>>>> + Object *obj; >>>>> + >>>>> + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); >>>>> + if (!obj) { >>>>> + error_setg(errp, "%s: required dram-mr link not found: %s", >>>>> + __func__, error_get_pretty(err)); >>>>> + return; >>>>> + } >>>>> + dram_size = memory_region_size(MEMORY_REGION(obj)); >>>>> + >>>>> + /* Power-on reset value */ >>>>> + s->reset_intcr3 = 0x00001002; >>>>> + >>>>> + /* >>>>> + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the >>>>> + * DRAM size, and is normally initialized by the boot block as part of DRAM >>>>> + * training. However, since we don't have a complete emulation of the >>>>> + * memory controller and try to make it look like it has already been >>>>> + * initialized, the boot block will skip this initialization, and we need >>>>> + * to make sure this field is set correctly up front. >>>>> + * >>>>> + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or >>>>> + * more of DRAM will be interpreted as 128 MiB. >>>> >>>> I'd say u-boot is right in wrapping at 2GB, as more DRAM is >>>> un-addressable. >>> >>> Ah, maybe I shouldn't have said "or more". The bug is that if you >>> specify _exactly_ 2GiB, u-boot will see 128 MiB. >>> >>> But I agree more than 2GiB doesn't make sense, so I'll add a check for that. >>> >>> Not sure if I agree that the check should be here. > 2 GiB is an >>> addressing limitation, and the GCR module shouldn't really know what >>> the SoC's address space looks like. The lower limit is because the GCR >>> module can't encode anything less than 128 MiB. >>> >>>>> + * >>>>> + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 >>>>> + */ >>>>> + if (dram_size >= 2 * GiB) { >>>> >>>> See my comment in this series on the machine patch. >>>> >>>>> + s->reset_intcr3 |= 4 << 8; >>>>> + } else if (dram_size >= 1 * GiB) { >>>>> + s->reset_intcr3 |= 3 << 8; >>>>> + } else if (dram_size >= 512 * MiB) { >>>>> + s->reset_intcr3 |= 2 << 8; >>>>> + } else if (dram_size >= 256 * MiB) { >>>>> + s->reset_intcr3 |= 1 << 8; >>>>> + } else if (dram_size >= 128 * MiB) { >>>>> + s->reset_intcr3 |= 0 << 8; >> >> I think this can be simplified as: >> >> s->reset_intcr3 = (4 - clz32(dram_size)) << 8; >> >> Which implicitly truncate to power of 2 (which is what this >> block does, as you can have only 1 bank managed per this SGC). > > Good idea. I find this a little easier to understand though: > > #define NPCM7XX_GCR_MIN_DRAM_SIZE (128 * MiB) > > s->reset_intcr3 |= ctz64(dram_size / NPCM7XX_GCR_MIN_DRAM_SIZE) << 8; I like it, furthermore it will work with >4GB if this model is ever reused on an ARMv8-A SoC. >> But checking is_power_of_2(dram_size) and reporting an error >> else, is probably even better. > > OK, I'll add a check for this, and also explicit checks for too large > and too small. The former is currently redundant with the DRAM size > check I'm adding to npcm7xx_realize(), but I kind of like the idea of > checking for encoding limitations and addressing limitations > separately, as they may not necessarily stay the same forever. > >>>>> + } else { >>>>> + error_setg(errp, >>>>> + "npcm7xx_gcr: DRAM size %" PRIu64 >>>>> + " is too small (need 128 MiB minimum)", >>>>> + dram_size); >>>> >>>> Ah, so you could add the same error for >2GB. Easier. >>>> >>>> Preferably using HWADDR_PRIx, and similar error for >2GB: >>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >
On Thu, Jul 9, 2020 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/9/20 7:09 PM, Havard Skinnemoen wrote: > > On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> On 7/9/20 8:43 AM, Havard Skinnemoen wrote: > >>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > >>>>> Implement a device model for the System Global Control Registers in the > >>>>> NPCM730 and NPCM750 BMC SoCs. > >>>>> > >>>>> This is primarily used to enable SMP boot (the boot ROM spins reading > >>>>> the SCRPAD register) and DDR memory initialization; other registers are > >>>>> best effort for now. > >>>>> > >>>>> The reset values of the MDLR and PWRON registers are determined by the > >>>>> SoC variant (730 vs 750) and board straps respectively. > >>>>> > >>>>> Reviewed-by: Joel Stanley <joel@jms.id.au> > >>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> > >>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > >>>>> --- > >> [...] > >>>>> diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c > >>>>> new file mode 100644 > >>>>> index 0000000000..9934cd238d > >>>>> --- /dev/null > >>>>> +++ b/hw/misc/npcm7xx_gcr.c > >>>>> @@ -0,0 +1,226 @@ > >>>>> +/* > >>>>> + * Nuvoton NPCM7xx System Global Control Registers. > >>>>> + * > >>>>> + * Copyright 2020 Google LLC > >>>>> + * > >>>>> + * This program is free software; you can redistribute it and/or modify it > >>>>> + * under the terms of the GNU General Public License as published by the > >>>>> + * Free Software Foundation; either version 2 of the License, or > >>>>> + * (at your option) any later version. > >>>>> + * > >>>>> + * This program is distributed in the hope that it will be useful, but WITHOUT > >>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > >>>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > >>>>> + * for more details. > >>>>> + */ > >>>>> + > >>>>> +#include "qemu/osdep.h" > >>>>> + > >>>>> +#include "hw/misc/npcm7xx_gcr.h" > >>>>> +#include "hw/qdev-properties.h" > >>>>> +#include "migration/vmstate.h" > >>>>> +#include "qapi/error.h" > >>>>> +#include "qemu/log.h" > >>>>> +#include "qemu/module.h" > >>>>> +#include "qemu/units.h" > >>>>> + > >>>>> +#include "trace.h" > >>>>> + > >>>>> +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { > >>>>> + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ > >>>>> + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, > >>>>> + [NPCM7XX_GCR_SPSWC] = 0x00000003, > >>>>> + [NPCM7XX_GCR_INTCR] = 0x0000035e, > >>>>> + [NPCM7XX_GCR_HIFCR] = 0x0000004e, > >>>>> + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ > >>>>> + [NPCM7XX_GCR_RESSR] = 0x80000000, > >>>>> + [NPCM7XX_GCR_DSCNT] = 0x000000c0, > >>>>> + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, > >>>>> + [NPCM7XX_GCR_SCRPAD] = 0x00000008, > >>>>> + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, > >>>>> + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, > >>>>> +}; > >>>>> + > >>>>> +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) > >>>>> +{ > >>>>> + uint32_t reg = offset / sizeof(uint32_t); > >>>>> + NPCM7xxGCRState *s = opaque; > >>>>> + > >>>>> + if (reg >= NPCM7XX_GCR_NR_REGS) { > >>>>> + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", > >>>>> + __func__, (unsigned int)offset); > >>>> > >>>> Maybe use HWADDR_PRIx instead of casting to int? > >>> > >>> Will do, thanks! > >> > >> Glad you are not annoyed by my review comments. > >> > >> FYI your series quality is in my top-4 "adding new machine to QEMU". > >> I'd like to use it as example to follow. > >> > >> I am slowly reviewing because this is not part of my work duties, > >> so I do that in my free time before/after work, which is why I can > >> barely do more that 2 per day, as I have to learn the SoC and > >> cross check documentation (or in this case, existing firmware code > >> since there is no public doc). > > > > Your feedback is super valuable, thanks a lot. I really appreciate it. > > OK I'll continue, but might not have time until next week. > After I plan to test too. OK, I'll try to post a v6 before the weekend, unless you prefer that I hold off until you've reviewed the whole series. > What would be useful is an acceptance test (see examples in > tests/acceptance/), using the artefacts from the link you shared > elsewhere: > https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/ Yes, tests are definitely on my radar. I'm working on SPI flash qtests. I haven't had much success with avocado yet, but I'll keep trying (perhaps using docker to control the environment more tightly). Since the 5.1 release is frozen now, I might post some followup patches before this series is merged, e.g. tests, bootrom submodule+blob, more peripherals, etc. Is it preferable to keep this series frozen (modulo API updates) since you spent a lot of time reviewing it, and post the new stuff separately, or is it better to add new patches to the end of the series and resend the whole thing? > But these are apparently not stable links (expire after 30 days?). Sorry, I'm too ignorant about Jenkins to know. I'll see if I can figure something out. > >> [...] > >>>>> +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) > >>>>> +{ > >>>>> + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); > >>>>> + uint64_t dram_size; > >>>>> + Error *err = NULL; > >>>>> + Object *obj; > >>>>> + > >>>>> + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); > >>>>> + if (!obj) { > >>>>> + error_setg(errp, "%s: required dram-mr link not found: %s", > >>>>> + __func__, error_get_pretty(err)); > >>>>> + return; > >>>>> + } > >>>>> + dram_size = memory_region_size(MEMORY_REGION(obj)); > >>>>> + > >>>>> + /* Power-on reset value */ > >>>>> + s->reset_intcr3 = 0x00001002; > >>>>> + > >>>>> + /* > >>>>> + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the > >>>>> + * DRAM size, and is normally initialized by the boot block as part of DRAM > >>>>> + * training. However, since we don't have a complete emulation of the > >>>>> + * memory controller and try to make it look like it has already been > >>>>> + * initialized, the boot block will skip this initialization, and we need > >>>>> + * to make sure this field is set correctly up front. > >>>>> + * > >>>>> + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or > >>>>> + * more of DRAM will be interpreted as 128 MiB. > >>>> > >>>> I'd say u-boot is right in wrapping at 2GB, as more DRAM is > >>>> un-addressable. > >>> > >>> Ah, maybe I shouldn't have said "or more". The bug is that if you > >>> specify _exactly_ 2GiB, u-boot will see 128 MiB. > >>> > >>> But I agree more than 2GiB doesn't make sense, so I'll add a check for that. > >>> > >>> Not sure if I agree that the check should be here. > 2 GiB is an > >>> addressing limitation, and the GCR module shouldn't really know what > >>> the SoC's address space looks like. The lower limit is because the GCR > >>> module can't encode anything less than 128 MiB. > >>> > >>>>> + * > >>>>> + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 > >>>>> + */ > >>>>> + if (dram_size >= 2 * GiB) { > >>>> > >>>> See my comment in this series on the machine patch. > >>>> > >>>>> + s->reset_intcr3 |= 4 << 8; > >>>>> + } else if (dram_size >= 1 * GiB) { > >>>>> + s->reset_intcr3 |= 3 << 8; > >>>>> + } else if (dram_size >= 512 * MiB) { > >>>>> + s->reset_intcr3 |= 2 << 8; > >>>>> + } else if (dram_size >= 256 * MiB) { > >>>>> + s->reset_intcr3 |= 1 << 8; > >>>>> + } else if (dram_size >= 128 * MiB) { > >>>>> + s->reset_intcr3 |= 0 << 8; > >> > >> I think this can be simplified as: > >> > >> s->reset_intcr3 = (4 - clz32(dram_size)) << 8; > >> > >> Which implicitly truncate to power of 2 (which is what this > >> block does, as you can have only 1 bank managed per this SGC). > > > > Good idea. I find this a little easier to understand though: > > > > #define NPCM7XX_GCR_MIN_DRAM_SIZE (128 * MiB) > > > > s->reset_intcr3 |= ctz64(dram_size / NPCM7XX_GCR_MIN_DRAM_SIZE) << 8; > > I like it, furthermore it will work with >4GB if this model is > ever reused on an ARMv8-A SoC. > > >> But checking is_power_of_2(dram_size) and reporting an error > >> else, is probably even better. > > > > OK, I'll add a check for this, and also explicit checks for too large > > and too small. The former is currently redundant with the DRAM size > > check I'm adding to npcm7xx_realize(), but I kind of like the idea of > > checking for encoding limitations and addressing limitations > > separately, as they may not necessarily stay the same forever. > > > >>>>> + } else { > >>>>> + error_setg(errp, > >>>>> + "npcm7xx_gcr: DRAM size %" PRIu64 > >>>>> + " is too small (need 128 MiB minimum)", > >>>>> + dram_size); > >>>> > >>>> Ah, so you could add the same error for >2GB. Easier. > >>>> > >>>> Preferably using HWADDR_PRIx, and similar error for >2GB: > >>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >
On 7/9/20 7:42 PM, Havard Skinnemoen wrote: > On Thu, Jul 9, 2020 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> On 7/9/20 7:09 PM, Havard Skinnemoen wrote: >>> On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> On 7/9/20 8:43 AM, Havard Skinnemoen wrote: >>>>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: >>>>>>> Implement a device model for the System Global Control Registers in the >>>>>>> NPCM730 and NPCM750 BMC SoCs. >>>>>>> >>>>>>> This is primarily used to enable SMP boot (the boot ROM spins reading >>>>>>> the SCRPAD register) and DDR memory initialization; other registers are >>>>>>> best effort for now. >>>>>>> >>>>>>> The reset values of the MDLR and PWRON registers are determined by the >>>>>>> SoC variant (730 vs 750) and board straps respectively. >>>>>>> >>>>>>> Reviewed-by: Joel Stanley <joel@jms.id.au> >>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>>>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> >>>>>>> --- [...] >>>>>> Maybe use HWADDR_PRIx instead of casting to int? >>>>> >>>>> Will do, thanks! >>>> >>>> Glad you are not annoyed by my review comments. >>>> >>>> FYI your series quality is in my top-4 "adding new machine to QEMU". >>>> I'd like to use it as example to follow. >>>> >>>> I am slowly reviewing because this is not part of my work duties, >>>> so I do that in my free time before/after work, which is why I can >>>> barely do more that 2 per day, as I have to learn the SoC and >>>> cross check documentation (or in this case, existing firmware code >>>> since there is no public doc). >>> >>> Your feedback is super valuable, thanks a lot. I really appreciate it. >> >> OK I'll continue, but might not have time until next week. >> After I plan to test too. > > OK, I'll try to post a v6 before the weekend, unless you prefer that I > hold off until you've reviewed the whole series. I don't mind, if it is not too much overhead on your side. What I noticed on the QEMU community is: - If a series reviewed the same day and is almost done except a single bugfix, it is OK to repost the same day, so the maintainer is likely to queue it directly. - If there are various reviews, and you do gradual improvements, it is OK to repost every 3 days. Else reviewers tend to skip/ postpone your series for later. - Pinging for review before 1 week passed is stressing reviewers, except in case of critical security bug (CVE) or during freeze. - Series with integrated test provided are usually reviewed first. >> What would be useful is an acceptance test (see examples in >> tests/acceptance/), using the artefacts from the link you shared >> elsewhere: >> https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/ > > Yes, tests are definitely on my radar. I'm working on SPI flash > qtests. I haven't had much success with avocado yet, but I'll keep > trying (perhaps using docker to control the environment more tightly). This might help: https://www.mail-archive.com/qemu-devel@nongnu.org/msg704907.html > Since the 5.1 release is frozen now, I might post some followup > patches before this series is merged, e.g. tests, bootrom > submodule+blob, more peripherals, etc. Is it preferable to keep this > series frozen (modulo API updates) since you spent a lot of time > reviewing it, and post the new stuff separately, or is it better to > add new patches to the end of the series and resend the whole thing? If you rework a peripheral, you need to reset the Reviewed-by/Tested-by tags. If you add new peripherals, you only need to reset these tags on the SoC patch. I'm fine either way, I use git-backport-diff to see the SoC changes easily: https://github.com/codyprime/git-scripts/blob/master/git-backport-diff > >> But these are apparently not stable links (expire after 30 days?). > > Sorry, I'm too ignorant about Jenkins to know. I'll see if I can > figure something out. What I do in that case is take the binary used for the test, write a comment and push it in a stable branch to my own repo: https://github.com/philmd/qemu-testing-blob/ and use the stable url in the test. We know QEMU emulation worked with this particular binary at some point. We want to avoid regressions in QEMU, so let's keep testing what we know worked. We don't want to track 2 bugs at a time (one in the updated guest and one in QEMU). Regards, Phil.
On Fri, Jul 10, 2020 at 2:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/9/20 7:42 PM, Havard Skinnemoen wrote: > > On Thu, Jul 9, 2020 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> On 7/9/20 7:09 PM, Havard Skinnemoen wrote: > >>> On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>>> On 7/9/20 8:43 AM, Havard Skinnemoen wrote: > >>>>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>>>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > >>>>>>> Implement a device model for the System Global Control Registers in the > >>>>>>> NPCM730 and NPCM750 BMC SoCs. > >>>>>>> > >>>>>>> This is primarily used to enable SMP boot (the boot ROM spins reading > >>>>>>> the SCRPAD register) and DDR memory initialization; other registers are > >>>>>>> best effort for now. > >>>>>>> > >>>>>>> The reset values of the MDLR and PWRON registers are determined by the > >>>>>>> SoC variant (730 vs 750) and board straps respectively. > >>>>>>> > >>>>>>> Reviewed-by: Joel Stanley <joel@jms.id.au> > >>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> > >>>>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > >>>>>>> --- > [...] > >>>>>> Maybe use HWADDR_PRIx instead of casting to int? > >>>>> > >>>>> Will do, thanks! > >>>> > >>>> Glad you are not annoyed by my review comments. > >>>> > >>>> FYI your series quality is in my top-4 "adding new machine to QEMU". > >>>> I'd like to use it as example to follow. > >>>> > >>>> I am slowly reviewing because this is not part of my work duties, > >>>> so I do that in my free time before/after work, which is why I can > >>>> barely do more that 2 per day, as I have to learn the SoC and > >>>> cross check documentation (or in this case, existing firmware code > >>>> since there is no public doc). > >>> > >>> Your feedback is super valuable, thanks a lot. I really appreciate it. > >> > >> OK I'll continue, but might not have time until next week. > >> After I plan to test too. > > > > OK, I'll try to post a v6 before the weekend, unless you prefer that I > > hold off until you've reviewed the whole series. > > I don't mind, if it is not too much overhead on your side. > > What I noticed on the QEMU community is: > > - If a series reviewed the same day and is almost done except > a single bugfix, it is OK to repost the same day, so the > maintainer is likely to queue it directly. > > - If there are various reviews, and you do gradual improvements, > it is OK to repost every 3 days. Else reviewers tend to skip/ > postpone your series for later. > > - Pinging for review before 1 week passed is stressing reviewers, > except in case of critical security bug (CVE) or during freeze. > > - Series with integrated test provided are usually reviewed first. This is very helpful, thanks. > >> What would be useful is an acceptance test (see examples in > >> tests/acceptance/), using the artefacts from the link you shared > >> elsewhere: > >> https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/ > > > > Yes, tests are definitely on my radar. I'm working on SPI flash > > qtests. I haven't had much success with avocado yet, but I'll keep > > trying (perhaps using docker to control the environment more tightly). > > This might help: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg704907.html Thanks, I got it working now. > > Since the 5.1 release is frozen now, I might post some followup > > patches before this series is merged, e.g. tests, bootrom > > submodule+blob, more peripherals, etc. Is it preferable to keep this > > series frozen (modulo API updates) since you spent a lot of time > > reviewing it, and post the new stuff separately, or is it better to > > add new patches to the end of the series and resend the whole thing? > > If you rework a peripheral, you need to reset the Reviewed-by/Tested-by > tags. If you add new peripherals, you only need to reset these tags on > the SoC patch. I'm fine either way, I use git-backport-diff to see the > SoC changes easily: > > https://github.com/codyprime/git-scripts/blob/master/git-backport-diff I've been adding new peripherals incrementally after the basic SoC support patch. Is that OK to do without resetting the tags? But it's more likely that I'll add other things than peripherals next, i.e. bootrom and tests. > > > >> But these are apparently not stable links (expire after 30 days?). > > > > Sorry, I'm too ignorant about Jenkins to know. I'll see if I can > > figure something out. > > What I do in that case is take the binary used for the test, > write a comment and push it in a stable branch to my own repo: > https://github.com/philmd/qemu-testing-blob/ and use the stable > url in the test. > > We know QEMU emulation worked with this particular binary at some > point. We want to avoid regressions in QEMU, so let's keep testing > what we know worked. We don't want to track 2 bugs at a time (one > in the updated guest and one in QEMU). Good point. I'll see if I can upload images to github. I might fork the openbmc repository and attach binaries to a github release, to make it clear where the binaries came from. I accidentally broke my test image and had some trouble recreating it, so I ended up reworking my various hacks a bit. The good news is that I got most of them turned into proper bug fixes that I can send upstream. It might take a little longer than I said previously, but I'll try to include acceptance tests in the next series. Havard
On Fri, Jul 10, 2020 at 11:46 PM Havard Skinnemoen <hskinnemoen@google.com> wrote: > > On Fri, Jul 10, 2020 at 2:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > On 7/9/20 7:42 PM, Havard Skinnemoen wrote: > > > On Thu, Jul 9, 2020 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > >> On 7/9/20 7:09 PM, Havard Skinnemoen wrote: > > >>> On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > >>>> On 7/9/20 8:43 AM, Havard Skinnemoen wrote: > > >>>>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > >>>>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote: > > >>>>>>> Implement a device model for the System Global Control Registers in the > > >>>>>>> NPCM730 and NPCM750 BMC SoCs. > > >>>>>>> > > >>>>>>> This is primarily used to enable SMP boot (the boot ROM spins reading > > >>>>>>> the SCRPAD register) and DDR memory initialization; other registers are > > >>>>>>> best effort for now. > > >>>>>>> > > >>>>>>> The reset values of the MDLR and PWRON registers are determined by the > > >>>>>>> SoC variant (730 vs 750) and board straps respectively. > > >>>>>>> > > >>>>>>> Reviewed-by: Joel Stanley <joel@jms.id.au> > > >>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org> > > >>>>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com> > > >>>>>>> --- > > [...] > > >>>>>> Maybe use HWADDR_PRIx instead of casting to int? > > >>>>> > > >>>>> Will do, thanks! > > >>>> > > >>>> Glad you are not annoyed by my review comments. > > >>>> > > >>>> FYI your series quality is in my top-4 "adding new machine to QEMU". > > >>>> I'd like to use it as example to follow. > > >>>> > > >>>> I am slowly reviewing because this is not part of my work duties, > > >>>> so I do that in my free time before/after work, which is why I can > > >>>> barely do more that 2 per day, as I have to learn the SoC and > > >>>> cross check documentation (or in this case, existing firmware code > > >>>> since there is no public doc). > > >>> > > >>> Your feedback is super valuable, thanks a lot. I really appreciate it. > > >> > > >> OK I'll continue, but might not have time until next week. > > >> After I plan to test too. > > > > > > OK, I'll try to post a v6 before the weekend, unless you prefer that I > > > hold off until you've reviewed the whole series. > > > > I don't mind, if it is not too much overhead on your side. > > > > What I noticed on the QEMU community is: > > > > - If a series reviewed the same day and is almost done except > > a single bugfix, it is OK to repost the same day, so the > > maintainer is likely to queue it directly. > > > > - If there are various reviews, and you do gradual improvements, > > it is OK to repost every 3 days. Else reviewers tend to skip/ > > postpone your series for later. > > > > - Pinging for review before 1 week passed is stressing reviewers, > > except in case of critical security bug (CVE) or during freeze. > > > > - Series with integrated test provided are usually reviewed first. > > This is very helpful, thanks. > > > >> What would be useful is an acceptance test (see examples in > > >> tests/acceptance/), using the artefacts from the link you shared > > >> elsewhere: > > >> https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/ > > > > > > Yes, tests are definitely on my radar. I'm working on SPI flash > > > qtests. I haven't had much success with avocado yet, but I'll keep > > > trying (perhaps using docker to control the environment more tightly). > > > > This might help: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg704907.html > > Thanks, I got it working now. > > > > Since the 5.1 release is frozen now, I might post some followup > > > patches before this series is merged, e.g. tests, bootrom > > > submodule+blob, more peripherals, etc. Is it preferable to keep this > > > series frozen (modulo API updates) since you spent a lot of time > > > reviewing it, and post the new stuff separately, or is it better to > > > add new patches to the end of the series and resend the whole thing? > > > > If you rework a peripheral, you need to reset the Reviewed-by/Tested-by > > tags. If you add new peripherals, you only need to reset these tags on > > the SoC patch. I'm fine either way, I use git-backport-diff to see the > > SoC changes easily: > > > > https://github.com/codyprime/git-scripts/blob/master/git-backport-diff > > I've been adding new peripherals incrementally after the basic SoC > support patch. Is that OK to do without resetting the tags? > > But it's more likely that I'll add other things than peripherals next, > i.e. bootrom and tests. > > > > > > >> But these are apparently not stable links (expire after 30 days?). > > > > > > Sorry, I'm too ignorant about Jenkins to know. I'll see if I can > > > figure something out. > > > > What I do in that case is take the binary used for the test, > > write a comment and push it in a stable branch to my own repo: > > https://github.com/philmd/qemu-testing-blob/ and use the stable > > url in the test. > > > > We know QEMU emulation worked with this particular binary at some > > point. We want to avoid regressions in QEMU, so let's keep testing > > what we know worked. We don't want to track 2 bugs at a time (one > > in the updated guest and one in QEMU). > > Good point. I'll see if I can upload images to github. I might fork > the openbmc repository and attach binaries to a github release, to > make it clear where the binaries came from. > > I accidentally broke my test image and had some trouble recreating it, > so I ended up reworking my various hacks a bit. The good news is that > I got most of them turned into proper bug fixes that I can send > upstream. > > It might take a little longer than I said previously, but I'll try to > include acceptance tests in the next series. I uploaded the images here: https://github.com/hskinnemoen/openbmc/releases/tag/20200711-gsj-qemu-0 I used them to implement an acceptance test that I'll include in v6.
diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h new file mode 100644 index 0000000000..4884676be2 --- /dev/null +++ b/include/hw/misc/npcm7xx_gcr.h @@ -0,0 +1,76 @@ +/* + * Nuvoton NPCM7xx System Global Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_GCR_H +#define NPCM7XX_GCR_H + +#include "exec/memory.h" +#include "hw/sysbus.h" + +enum NPCM7xxGCRRegisters { + NPCM7XX_GCR_PDID, + NPCM7XX_GCR_PWRON, + NPCM7XX_GCR_MFSEL1 = 0x0c / sizeof(uint32_t), + NPCM7XX_GCR_MFSEL2, + NPCM7XX_GCR_MISCPE, + NPCM7XX_GCR_SPSWC = 0x038 / sizeof(uint32_t), + NPCM7XX_GCR_INTCR, + NPCM7XX_GCR_INTSR, + NPCM7XX_GCR_HIFCR = 0x050 / sizeof(uint32_t), + NPCM7XX_GCR_INTCR2 = 0x060 / sizeof(uint32_t), + NPCM7XX_GCR_MFSEL3, + NPCM7XX_GCR_SRCNT, + NPCM7XX_GCR_RESSR, + NPCM7XX_GCR_RLOCKR1, + NPCM7XX_GCR_FLOCKR1, + NPCM7XX_GCR_DSCNT, + NPCM7XX_GCR_MDLR, + NPCM7XX_GCR_SCRPAD3, + NPCM7XX_GCR_SCRPAD2, + NPCM7XX_GCR_DAVCLVLR = 0x098 / sizeof(uint32_t), + NPCM7XX_GCR_INTCR3, + NPCM7XX_GCR_VSINTR = 0x0ac / sizeof(uint32_t), + NPCM7XX_GCR_MFSEL4, + NPCM7XX_GCR_CPBPNTR = 0x0c4 / sizeof(uint32_t), + NPCM7XX_GCR_CPCTL = 0x0d0 / sizeof(uint32_t), + NPCM7XX_GCR_CP2BST, + NPCM7XX_GCR_B2CPNT, + NPCM7XX_GCR_CPPCTL, + NPCM7XX_GCR_I2CSEGSEL, + NPCM7XX_GCR_I2CSEGCTL, + NPCM7XX_GCR_VSRCR, + NPCM7XX_GCR_MLOCKR, + NPCM7XX_GCR_SCRPAD = 0x013c / sizeof(uint32_t), + NPCM7XX_GCR_USB1PHYCTL, + NPCM7XX_GCR_USB2PHYCTL, + NPCM7XX_GCR_NR_REGS, +}; + +typedef struct NPCM7xxGCRState { + SysBusDevice parent; + + MemoryRegion iomem; + + uint32_t regs[NPCM7XX_GCR_NR_REGS]; + + uint32_t reset_pwron; + uint32_t reset_mdlr; + uint32_t reset_intcr3; +} NPCM7xxGCRState; + +#define TYPE_NPCM7XX_GCR "npcm7xx-gcr" +#define NPCM7XX_GCR(obj) OBJECT_CHECK(NPCM7xxGCRState, (obj), TYPE_NPCM7XX_GCR) + +#endif /* NPCM7XX_GCR_H */ diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c new file mode 100644 index 0000000000..9934cd238d --- /dev/null +++ b/hw/misc/npcm7xx_gcr.c @@ -0,0 +1,226 @@ +/* + * Nuvoton NPCM7xx System Global Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/misc/npcm7xx_gcr.h" +#include "hw/qdev-properties.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qemu/units.h" + +#include "trace.h" + +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { + [NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ + [NPCM7XX_GCR_MISCPE] = 0x0000ffff, + [NPCM7XX_GCR_SPSWC] = 0x00000003, + [NPCM7XX_GCR_INTCR] = 0x0000035e, + [NPCM7XX_GCR_HIFCR] = 0x0000004e, + [NPCM7XX_GCR_INTCR2] = (1U << 19), /* DDR initialized */ + [NPCM7XX_GCR_RESSR] = 0x80000000, + [NPCM7XX_GCR_DSCNT] = 0x000000c0, + [NPCM7XX_GCR_DAVCLVLR] = 0x5a00f3cf, + [NPCM7XX_GCR_SCRPAD] = 0x00000008, + [NPCM7XX_GCR_USB1PHYCTL] = 0x034730e4, + [NPCM7XX_GCR_USB2PHYCTL] = 0x034730e4, +}; + +static uint64_t npcm7xx_gcr_read(void *opaque, hwaddr offset, unsigned size) +{ + uint32_t reg = offset / sizeof(uint32_t); + NPCM7xxGCRState *s = opaque; + + if (reg >= NPCM7XX_GCR_NR_REGS) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", + __func__, (unsigned int)offset); + return 0; + } + + trace_npcm7xx_gcr_read(offset, s->regs[reg]); + + return s->regs[reg]; +} + +static void npcm7xx_gcr_write(void *opaque, hwaddr offset, + uint64_t v, unsigned size) +{ + uint32_t reg = offset / sizeof(uint32_t); + NPCM7xxGCRState *s = opaque; + uint32_t value = v; + + trace_npcm7xx_gcr_write(offset, value); + + if (reg >= NPCM7XX_GCR_NR_REGS) { + qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%04x out of range\n", + __func__, (unsigned int)offset); + return; + } + + switch (reg) { + case NPCM7XX_GCR_PDID: + case NPCM7XX_GCR_PWRON: + case NPCM7XX_GCR_INTSR: + qemu_log_mask(LOG_GUEST_ERROR, "%s: register @ 0x%04x is read-only\n", + __func__, (unsigned int)offset); + return; + + case NPCM7XX_GCR_RESSR: + case NPCM7XX_GCR_CP2BST: + /* Write 1 to clear */ + value = s->regs[reg] & ~value; + break; + + case NPCM7XX_GCR_RLOCKR1: + case NPCM7XX_GCR_MDLR: + /* Write 1 to set */ + value |= s->regs[reg]; + break; + }; + + s->regs[reg] = value; +} + +static const struct MemoryRegionOps npcm7xx_gcr_ops = { + .read = npcm7xx_gcr_read, + .write = npcm7xx_gcr_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + .unaligned = false, + }, +}; + +static void npcm7xx_gcr_enter_reset(Object *obj, ResetType type) +{ + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); + + QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values)); + + switch (type) { + case RESET_TYPE_COLD: + memcpy(s->regs, cold_reset_values, sizeof(s->regs)); + s->regs[NPCM7XX_GCR_PWRON] = s->reset_pwron; + s->regs[NPCM7XX_GCR_MDLR] = s->reset_mdlr; + s->regs[NPCM7XX_GCR_INTCR3] = s->reset_intcr3; + break; + } +} + +static void npcm7xx_gcr_realize(DeviceState *dev, Error **errp) +{ + NPCM7xxGCRState *s = NPCM7XX_GCR(dev); + uint64_t dram_size; + Error *err = NULL; + Object *obj; + + obj = object_property_get_link(OBJECT(dev), "dram-mr", &err); + if (!obj) { + error_setg(errp, "%s: required dram-mr link not found: %s", + __func__, error_get_pretty(err)); + return; + } + dram_size = memory_region_size(MEMORY_REGION(obj)); + + /* Power-on reset value */ + s->reset_intcr3 = 0x00001002; + + /* + * The GMMAP (Graphics Memory Map) field is used by u-boot to detect the + * DRAM size, and is normally initialized by the boot block as part of DRAM + * training. However, since we don't have a complete emulation of the + * memory controller and try to make it look like it has already been + * initialized, the boot block will skip this initialization, and we need + * to make sure this field is set correctly up front. + * + * WARNING: some versions of u-boot only looks at bits 8 and 9, so 2 GiB or + * more of DRAM will be interpreted as 128 MiB. + * + * https://github.com/Nuvoton-Israel/u-boot/blob/2aef993bd2aafeb5408dbaad0f3ce099ee40c4aa/board/nuvoton/poleg/poleg.c#L244 + */ + if (dram_size >= 2 * GiB) { + s->reset_intcr3 |= 4 << 8; + } else if (dram_size >= 1 * GiB) { + s->reset_intcr3 |= 3 << 8; + } else if (dram_size >= 512 * MiB) { + s->reset_intcr3 |= 2 << 8; + } else if (dram_size >= 256 * MiB) { + s->reset_intcr3 |= 1 << 8; + } else if (dram_size >= 128 * MiB) { + s->reset_intcr3 |= 0 << 8; + } else { + error_setg(errp, + "npcm7xx_gcr: DRAM size %" PRIu64 + " is too small (need 128 MiB minimum)", + dram_size); + return; + } +} + +static void npcm7xx_gcr_init(Object *obj) +{ + NPCM7xxGCRState *s = NPCM7XX_GCR(obj); + + memory_region_init_io(&s->iomem, obj, &npcm7xx_gcr_ops, s, + TYPE_NPCM7XX_GCR, 4 * KiB); + sysbus_init_mmio(&s->parent, &s->iomem); +} + +static const VMStateDescription vmstate_npcm7xx_gcr = { + .name = "npcm7xx-gcr", + .version_id = 0, + .minimum_version_id = 0, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(regs, NPCM7xxGCRState, NPCM7XX_GCR_NR_REGS), + VMSTATE_END_OF_LIST(), + }, +}; + +static Property npcm7xx_gcr_properties[] = { + DEFINE_PROP_UINT32("disabled-modules", NPCM7xxGCRState, reset_mdlr, 0), + DEFINE_PROP_UINT32("power-on-straps", NPCM7xxGCRState, reset_pwron, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +static void npcm7xx_gcr_class_init(ObjectClass *klass, void *data) +{ + ResettableClass *rc = RESETTABLE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->desc = "NPCM7xx System Global Control Registers"; + dc->realize = npcm7xx_gcr_realize; + dc->vmsd = &vmstate_npcm7xx_gcr; + rc->phases.enter = npcm7xx_gcr_enter_reset; + + device_class_set_props(dc, npcm7xx_gcr_properties); +} + +static const TypeInfo npcm7xx_gcr_info = { + .name = TYPE_NPCM7XX_GCR, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(NPCM7xxGCRState), + .instance_init = npcm7xx_gcr_init, + .class_init = npcm7xx_gcr_class_init, +}; + +static void npcm7xx_gcr_register_type(void) +{ + type_register_static(&npcm7xx_gcr_info); +} +type_init(npcm7xx_gcr_register_type); diff --git a/MAINTAINERS b/MAINTAINERS index 42388f1de2..43173a338a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -723,6 +723,14 @@ S: Odd Fixes F: hw/arm/musicpal.c F: docs/system/arm/musicpal.rst +Nuvoton NPCM7xx +M: Havard Skinnemoen <hskinnemoen@google.com> +M: Tyrone Ting <kfting@nuvoton.com> +L: qemu-arm@nongnu.org +S: Supported +F: hw/*/npcm7xx* +F: include/hw/*/npcm7xx* + nSeries M: Andrzej Zaborowski <balrogg@gmail.com> M: Peter Maydell <peter.maydell@linaro.org> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index 4a224a6351..192a8dec3b 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -354,6 +354,9 @@ config XLNX_VERSAL select VIRTIO_MMIO select UNIMP +config NPCM7XX + bool + config FSL_IMX25 bool select IMX diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index 5aaca8a039..40a9d1c01e 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -51,6 +51,7 @@ common-obj-$(CONFIG_IMX) += imx_rngc.o common-obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o common-obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o common-obj-$(CONFIG_MAINSTONE) += mst_fpga.o +common-obj-$(CONFIG_NPCM7XX) += npcm7xx_gcr.o common-obj-$(CONFIG_OMAP) += omap_clk.o common-obj-$(CONFIG_OMAP) += omap_gpmc.o common-obj-$(CONFIG_OMAP) += omap_l4.o diff --git a/hw/misc/trace-events b/hw/misc/trace-events index ebea53735c..48e2d54c49 100644 --- a/hw/misc/trace-events +++ b/hw/misc/trace-events @@ -107,6 +107,10 @@ mos6522_set_sr_int(void) "set sr_int" mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64 mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x" +# npcm7xx_gcr.c +npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 +npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32 + # stm32f4xx_syscfg stm32f4xx_syscfg_set_irq(int gpio, int line, int level) "Interupt: GPIO: %d, Line: %d; Level: %d" stm32f4xx_pulse_exti(int irq) "Pulse EXTI: %d"