On Thu, 2016-06-23 at 18:37 +0100, Peter Maydell wrote: > On 23 June 2016 at 03:15, Andrew Jeffery <and...@aj.id.au> wrote: > > > > The SCU is a collection of chip-level control registers that manage the > > various functions supported by ASPEED SoCs. Typically the bits control > > interactions with clocks, external hardware or reset behaviour, and we > > can largly take a hands-off approach to reads and writes. > > > > Firmware makes heavy use of the state to determine how to boot, but the > > reset values vary from SoC to SoC (eg AST2400 vs AST2500). A qdev > > property is exposed so that the integrating SoC model can configure the > > silicon revision, which in-turn selects the appropriate reset values. > > Further qdev properties are exposed so the board model can configure the > > board-dependent hardware strapping. > > > > Almost all provided AST2400 reset values are specified by the datasheet. > > The notable exception is SOC_SCRATCH1, where we mark the DRAM as > > successfully initialised to avoid unnecessary dark corners in the SoC's > > u-boot support. > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au> > Thanks -- I think the interface to the board is much nicer now. > I have a couple of comments below. > > > > > hw/misc/Makefile.objs | 1 + > > hw/misc/aspeed_scu.c | 258 > > +++++++++++++++++++++++++++++++++++++++++++ > > include/hw/misc/aspeed_scu.h | 34 ++++++ > > trace-events | 3 + > > 4 files changed, 296 insertions(+) > > create mode 100644 hw/misc/aspeed_scu.c > > create mode 100644 include/hw/misc/aspeed_scu.h > > > > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > > index ffb49c11aca6..54020aa06c00 100644 > > --- a/hw/misc/Makefile.objs > > +++ b/hw/misc/Makefile.objs > > @@ -52,3 +52,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o > > obj-$(CONFIG_EDU) += edu.o > > obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o > > obj-$(CONFIG_AUX) += aux.o > > +obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o > > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c > > new file mode 100644 > > index 000000000000..a714431c45c5 > > --- /dev/null > > +++ b/hw/misc/aspeed_scu.c > > @@ -0,0 +1,258 @@ > > +/* > > + * ASPEED System Control Unit > > + * > > + * Andrew Jeffery <and...@aj.id.au> > > + * > > + * Copyright 2016 IBM Corp. > > + * > > + * This code is licensed under the GPL version 2 or later. See > > + * the COPYING file in the top-level directory. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include > > +#include "hw/misc/aspeed_scu.h" > > +#include "hw/qdev-properties.h" > > +#include "qapi/error.h" > > +#include "qapi/visitor.h" > > +#include "qemu/bitops.h" > > +#include "trace.h" > > + > > +#define TO_REG(offset) ((offset) >> 2) > > + > > +#define PROT_KEY TO_REG(0x00) > > +#define SYS_RST_CTRL TO_REG(0x04) > > +#define CLK_SEL TO_REG(0x08) > > +#define CLK_STOP_CTRL TO_REG(0x0C) > > +#define FREQ_CNTR_CTRL TO_REG(0x10) > > +#define FREQ_CNTR_EVAL TO_REG(0x14) > > +#define IRQ_CTRL TO_REG(0x18) > > +#define D2PLL_PARAM TO_REG(0x1C) > > +#define MPLL_PARAM TO_REG(0x20) > > +#define HPLL_PARAM TO_REG(0x24) > > +#define FREQ_CNTR_RANGE TO_REG(0x28) > > +#define MISC_CTRL1 TO_REG(0x2C) > > +#define PCI_CTRL1 TO_REG(0x30) > > +#define PCI_CTRL2 TO_REG(0x34) > > +#define PCI_CTRL3 TO_REG(0x38) > > +#define SYS_RST_STATUS TO_REG(0x3C) > > +#define SOC_SCRATCH1 TO_REG(0x40) > > +#define SOC_SCRATCH2 TO_REG(0x44) > > +#define MAC_CLK_DELAY TO_REG(0x48) > > +#define MISC_CTRL2 TO_REG(0x4C) > > +#define VGA_SCRATCH1 TO_REG(0x50) > > +#define VGA_SCRATCH2 TO_REG(0x54) > > +#define VGA_SCRATCH3 TO_REG(0x58) > > +#define VGA_SCRATCH4 TO_REG(0x5C) > > +#define VGA_SCRATCH5 TO_REG(0x60) > > +#define VGA_SCRATCH6 TO_REG(0x64) > > +#define VGA_SCRATCH7 TO_REG(0x68) > > +#define VGA_SCRATCH8 TO_REG(0x6C) > > +#define HW_STRAP1 TO_REG(0x70) > > +#define RNG_CTRL TO_REG(0x74) > > +#define RNG_DATA TO_REG(0x78) > > +#define SILICON_REV TO_REG(0x7C) > > +#define PINMUX_CTRL1 TO_REG(0x80) > > +#define PINMUX_CTRL2 TO_REG(0x84) > > +#define PINMUX_CTRL3 TO_REG(0x88) > > +#define PINMUX_CTRL4 TO_REG(0x8C) > > +#define PINMUX_CTRL5 TO_REG(0x90) > > +#define PINMUX_CTRL6 TO_REG(0x94) > > +#define WDT_RST_CTRL TO_REG(0x9C) > > +#define PINMUX_CTRL7 TO_REG(0xA0) > > +#define PINMUX_CTRL8 TO_REG(0xA4) > > +#define PINMUX_CTRL9 TO_REG(0xA8) > > +#define WAKEUP_EN TO_REG(0xC0) > > +#define WAKEUP_CTRL TO_REG(0xC4) > > +#define HW_STRAP2 TO_REG(0xD0) > > +#define FREE_CNTR4 TO_REG(0xE0) > > +#define FREE_CNTR4_EXT TO_REG(0xE4) > > +#define CPU2_CTRL TO_REG(0x100) > > +#define CPU2_BASE_SEG1 TO_REG(0x104) > > +#define CPU2_BASE_SEG2 TO_REG(0x108) > > +#define CPU2_BASE_SEG3 TO_REG(0x10C) > > +#define CPU2_BASE_SEG4 TO_REG(0x110) > > +#define CPU2_BASE_SEG5 TO_REG(0x114) > > +#define CPU2_CACHE_CTRL TO_REG(0x118) > > +#define UART_HPLL_CLK TO_REG(0x160) > > +#define PCIE_CTRL TO_REG(0x180) > > +#define BMC_MMIO_CTRL TO_REG(0x184) > > +#define RELOC_DECODE_BASE1 TO_REG(0x188) > > +#define RELOC_DECODE_BASE2 TO_REG(0x18C) > > +#define MAILBOX_DECODE_BASE TO_REG(0x190) > > +#define SRAM_DECODE_BASE1 TO_REG(0x194) > > +#define SRAM_DECODE_BASE2 TO_REG(0x198) > > +#define BMC_REV TO_REG(0x19C) > > +#define BMC_DEV_ID TO_REG(0x1A4) > > + > > +#define SCU_KEY 0x1688A8A8 > > +#define SCU_IO_REGION_SIZE 0x20000 > > > > + > > +static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size) > > +{ > > + AspeedSCUState *s = ASPEED_SCU(opaque); > > + > > + if (TO_REG(offset) >= ARRAY_SIZE(s->regs)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx > > "\n", > > + __func__, offset); > > + return 0; > > + } > > + > > + switch (offset) { > > + case WAKEUP_EN: > WAKEUP_EN is defined as TO_REG(something) so you can't use it in > a case statement switching on an offset.
Right. That was quite an oversight and lead to quite misleading guest- error messages. Thanks for picking that up. > > > > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n", > > + __func__, offset); > > + break; > > + } > > + > > + return s->regs[TO_REG(offset)]; > > +} > > + > > +static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data, > > + unsigned size) > > +{ > > + AspeedSCUState *s = ASPEED_SCU(opaque); > > + > > + if (TO_REG(offset) >= ARRAY_SIZE(s->regs)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx > > "\n", > > + __func__, offset); > > + return; > > + } > > + > > + if (offset > PROT_KEY && offset < CPU2_BASE_SEG1 && > > + s->regs[TO_REG(PROT_KEY)] != SCU_KEY) { > PROT_KEY is defined above as TO_REG(0x00), so it's not > an offset and using it in comparisons against offset and > applying TO_REG() to it again doesn't seem right. > Similarly with CPU2_BASE_SEG1, which is more important since > it's not zero... Yes, this was the same issue as above. > > > > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__); > > + return; > > + } > > + > > + trace_aspeed_scu_write(offset, size, data); > > + > > + switch (offset) { > > + case FREQ_CNTR_EVAL: > > + case VGA_SCRATCH1 ... VGA_SCRATCH8: > > + case RNG_DATA: > > + case SILICON_REV: > > + case FREE_CNTR4: > > + case FREE_CNTR4_EXT: > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n", > > + __func__, offset); > > + return; > > + } > > + > > + s->regs[TO_REG(offset)] = (uint32_t) data; > This cast is unnecessary. True! > > > > > +} > > + > > +static const MemoryRegionOps aspeed_scu_ops = { > > + .read = aspeed_scu_read, > > + .write = aspeed_scu_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid.min_access_size = 4, > > + .valid.max_access_size = 4, > > + .valid.unaligned = false, > > +}; > > + > > +static void aspeed_scu_reset(DeviceState *dev) > > +{ > > + AspeedSCUState *s = ASPEED_SCU(dev); > > + const uint32_t *reset; > > + > > + switch (s->silicon_rev) { > > + case 0x02000303U: > > + reset = ast2400_resets; > > + break; > default: > g_assert_not_reached(); > > or the compiler will probably complain that you might > be using reset uninitialized. Will do. > > > > > + } > > + > > + memcpy(s->regs, reset, sizeof(s->regs)); > > + s->regs[SILICON_REV] = s->silicon_rev; > > + s->regs[HW_STRAP1] = s->hw_strap1; > > + s->regs[HW_STRAP2] = s->hw_strap2; > > +} > > + > > +static void aspeed_scu_realize(DeviceState *dev, Error **errp) > > +{ > > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > + AspeedSCUState *s = ASPEED_SCU(dev); > You should sanity check your properties here, and > fail the realize if they aren't valid values. Will do. > > > > > + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_scu_ops, s, > > + TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE); > > + > > + sysbus_init_mmio(sbd, &s->iomem); > > +} > > > > diff --git a/trace-events b/trace-events > > index 9d76de85749d..1b5fd602903c 100644 > > --- a/trace-events > > +++ b/trace-events > > @@ -156,3 +156,6 @@ memory_region_tb_write(int cpu_index, uint64_t addr, > > uint64_t value, unsigned si > > # > > # Targets: TCG(all) > > disable vcpu tcg guest_mem_before(TCGv vaddr, uint8_t info) "info=%d", > > "vaddr=0x%016"PRIx64" info=%d" > > + > > +# hw/misc/aspeed_scu.c > > +aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" > > PRIx64 " of size %u: 0x%" PRIx32 > > -- > This should be in hw/misc/trace-events now -- we've split the > big trace-events file into pieces. Will do. Thanks again for the review and apologies for the noise with the offset/reg mixup, that was an annoying oversight. Cheers, Andrew
signature.asc
Description: This is a digitally signed message part