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 <inttypes.h>
> +#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.
> + 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...
> + 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.
> +}
> +
> +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.
> + }
> +
> + 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.
> + 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.
thanks
-- PMM
