[Qemu-devel] [PATCH v2 0/3] Add ASPEED SCU device

[Qemu-devel] [PATCH v2 0/3] Add ASPEED SCU device

[Andrew Jeffery]

Hi all,

These are three patches implementing minimal functionality for the ASPEED System
Control Unit device and integrating it into the AST2400 SoC model/palmetto-bmc
machine. The device is critical for initialisation of u-boot and the kernel as
it provides chip level control registers, influencing the configuration of the
software and the software's configuration of the SoC.

Since v1:

* Select reset values based on silicon ID
* Expose hardware strapping values via properties

Cheers,

Andrew

Andrew Jeffery (3):
  hw/misc: Add a model for the ASPEED System Control Unit
  ast2400: Integrate the SCU model and set silicon revision
  palmetto-bmc: Configure the SCU's hardware strapping register

 hw/arm/ast2400.c             |  17 +++
 hw/arm/palmetto-bmc.c        |   2 +
 hw/misc/Makefile.objs        |   1 +
 hw/misc/aspeed_scu.c         | 258 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/ast2400.h     |   2 +
 include/hw/misc/aspeed_scu.h |  34 ++++++
 trace-events                 |   3 +
 7 files changed, 317 insertions(+)
 create mode 100644 hw/misc/aspeed_scu.c
 create mode 100644 include/hw/misc/aspeed_scu.h

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Andrew Jeffery]

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 <andrew@aj.id.au>
---

Since v1:

* Move reset values into SCU implementation (also make register defines private)
* Expose silicon-rev property which is used to select appropriate reset values
* Expose hw-strap1/hw-strap2 properties for board-specific SoC configuration

 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 <andrew@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 const uint32_t ast2400_resets[ASPEED_SCU_NR_REGS] = {
+     [SYS_RST_CTRL]    = 0xFFCFFEDCU,
+     [CLK_SEL]         = 0xF3F40000U,
+     [CLK_STOP_CTRL]   = 0x19FC3E8BU,
+     [D2PLL_PARAM]     = 0x00026108U,
+     [MPLL_PARAM]      = 0x00030291U,
+     [HPLL_PARAM]      = 0x00000291U,
+     [MISC_CTRL1]      = 0x00000010U,
+     [PCI_CTRL1]       = 0x20001A03U,
+     [PCI_CTRL2]       = 0x20001A03U,
+     [PCI_CTRL3]       = 0x04000030U,
+     [SYS_RST_STATUS]  = 0x00000001U,
+     [SOC_SCRATCH1]    = 0x000000C0U, /* SoC completed DRAM init */
+     [MISC_CTRL2]      = 0x00000023U,
+     [RNG_CTRL]        = 0x0000000EU,
+     [SILICON_REV]     = 0x02000303U,
+     [PINMUX_CTRL2]    = 0x0000F000U,
+     [PINMUX_CTRL3]    = 0x01000000U,
+     [PINMUX_CTRL4]    = 0x000000FFU,
+     [PINMUX_CTRL5]    = 0x0000A000U,
+     [WDT_RST_CTRL]    = 0x003FFFF3U,
+     [PINMUX_CTRL8]    = 0xFFFF0000U,
+     [PINMUX_CTRL9]    = 0x000FFFFFU,
+     [FREE_CNTR4]      = 0x000000FFU,
+     [FREE_CNTR4_EXT]  = 0x000000FFU,
+     [CPU2_BASE_SEG1]  = 0x80000000U,
+     [CPU2_BASE_SEG4]  = 0x1E600000U,
+     [CPU2_BASE_SEG5]  = 0xC0000000U,
+     [UART_HPLL_CLK]   = 0x00001903U,
+     [PCIE_CTRL]       = 0x0000007BU,
+     [BMC_DEV_ID]      = 0x00002402U
+};
+
+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:
+        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) {
+        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;
+}
+
+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;
+    }
+
+    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);
+
+    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);
+}
+
+static const VMStateDescription vmstate_aspeed_scu = {
+    .name = "aspeed.scu",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedSCUState, ASPEED_SCU_NR_REGS),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property aspeed_scu_properties[] = {
+    DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
+    DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
+    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap1, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_scu_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    dc->realize = aspeed_scu_realize;
+    dc->reset = aspeed_scu_reset;
+    dc->desc = "ASPEED System Control Unit";
+    dc->vmsd = &vmstate_aspeed_scu;
+    dc->props = aspeed_scu_properties;
+}
+
+static const TypeInfo aspeed_scu_info = {
+    .name = TYPE_ASPEED_SCU,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedSCUState),
+    .class_init = aspeed_scu_class_init,
+};
+
+static void aspeed_scu_register_types(void)
+{
+    type_register_static(&aspeed_scu_info);
+}
+
+type_init(aspeed_scu_register_types);
diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
new file mode 100644
index 000000000000..6b8e46f85fad
--- /dev/null
+++ b/include/hw/misc/aspeed_scu.h
@@ -0,0 +1,34 @@
+/*
+ * ASPEED System Control Unit
+ *
+ * Andrew Jeffery <andrew@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.
+ */
+#ifndef ASPEED_SCU_H
+#define ASPEED_SCU_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_SCU "aspeed.scu"
+#define ASPEED_SCU(obj) OBJECT_CHECK(AspeedSCUState, (obj), TYPE_ASPEED_SCU)
+
+#define ASPEED_SCU_NR_REGS (0x1A8 >> 2)
+
+typedef struct AspeedSCUState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion iomem;
+
+    uint32_t regs[ASPEED_SCU_NR_REGS];
+    uint32_t silicon_rev;
+    uint32_t hw_strap1;
+    uint32_t hw_strap2;
+} AspeedSCUState;
+
+#endif /* ASPEED_SCU_H */
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
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/3] ast2400: Integrate the SCU model and set silicon revision

[Andrew Jeffery]

By specifying the silicon revision we select the appropriate reset
values for the SoC.

Additionally, expose hardware strapping properties aliasing those
provided by the SCU for board-specific configuration.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---

Since v1:

* Remove reset value configuration
* Configure SoC silicon revision in the SCU via property
* Alias the SCU's hardware strapping properties to expose them to boards

 hw/arm/ast2400.c         | 17 +++++++++++++++++
 include/hw/arm/ast2400.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
index 4a9de0e10cbc..1a26d74e695c 100644
--- a/hw/arm/ast2400.c
+++ b/hw/arm/ast2400.c
@@ -24,6 +24,7 @@
 #define AST2400_IOMEM_SIZE       0x00200000
 #define AST2400_IOMEM_BASE       0x1E600000
 #define AST2400_VIC_BASE         0x1E6C0000
+#define AST2400_SCU_BASE         0x1E6E2000
 #define AST2400_TIMER_BASE       0x1E782000
 #define AST2400_I2C_BASE         0x1E78A000
 
@@ -72,6 +73,14 @@ static void ast2400_init(Object *obj)
     object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
     object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
     qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
+
+    object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
+    object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
+    qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+    object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
+                              "hw-strap1", &error_abort);
+    object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
+                              "hw-strap2", &error_abort);
 }
 
 static void ast2400_realize(DeviceState *dev, Error **errp)
@@ -110,6 +119,14 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
+    /* SCU */
+    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->scu), 0, AST2400_SCU_BASE);
+
     /* UART - attach an 8250 to the IO space as our UART5 */
     if (serial_hds[0]) {
         qemu_irq uart5 = qdev_get_gpio_in(DEVICE(&s->vic), uart_irqs[4]);
diff --git a/include/hw/arm/ast2400.h b/include/hw/arm/ast2400.h
index c05ed5376736..f1a64fd3893d 100644
--- a/include/hw/arm/ast2400.h
+++ b/include/hw/arm/ast2400.h
@@ -14,6 +14,7 @@
 
 #include "hw/arm/arm.h"
 #include "hw/intc/aspeed_vic.h"
+#include "hw/misc/aspeed_scu.h"
 #include "hw/timer/aspeed_timer.h"
 #include "hw/i2c/aspeed_i2c.h"
 
@@ -27,6 +28,7 @@ typedef struct AST2400State {
     AspeedVICState vic;
     AspeedTimerCtrlState timerctrl;
     AspeedI2CState i2c;
+    AspeedSCUState scu;
 } AST2400State;
 
 #define TYPE_AST2400 "ast2400"
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Andrew Jeffery]

The magic constant configures the following options:

* 28:27: Configure DRAM size as 256MB
* 26:24: DDR3 SDRAM with CL = 6, CWL = 5
* 23: Configure 24/48MHz CLKIN
* 22: Disable GPIOE pass-through mode
* 21: Disable GPIOD pass-through mode
* 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
* 19: Disable ACPI
* 18: Configure 48MHz CLKIN
* 17: Disable BMC 2nd boot watchdog timer
* 16: Decode SuperIO address 0x2E
* 15: VGA Class Code
* 14: Enable LPC dedicated reset pin
* 13:12: Enable SPI Master and SPI Slave to AHB Bridge
* 11:10: Select CPU:AHB ratio = 2:1
* 9:8: Select 384MHz H-PLL
* 7: Configure MAC#2 for RMII/NCSI
* 6: Configure MAC#1 for RMII/NCSI
* 5: No VGA BIOS ROM
* 4: Boot using 32bit SPI address mode
* 3:2: Select 16MB VGA memory
* 1:0: Boot from SPI flash memory

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/palmetto-bmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
index a51d960510ee..b8eed21348d8 100644
--- a/hw/arm/palmetto-bmc.c
+++ b/hw/arm/palmetto-bmc.c
@@ -44,6 +44,8 @@ static void palmetto_bmc_init(MachineState *machine)
                                 &bmc->ram);
     object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
                                    &error_abort);
+    object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
+                            &error_abort);
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Cédric Le Goater]

On 06/23/2016 04:15 AM, Andrew Jeffery 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 <andrew@aj.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Cédric Le Goater]

On 06/23/2016 04:15 AM, Andrew Jeffery wrote:
> The magic constant configures the following options:
> 
> * 28:27: Configure DRAM size as 256MB
> * 26:24: DDR3 SDRAM with CL = 6, CWL = 5
> * 23: Configure 24/48MHz CLKIN
> * 22: Disable GPIOE pass-through mode
> * 21: Disable GPIOD pass-through mode
> * 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
> * 19: Disable ACPI
> * 18: Configure 48MHz CLKIN
> * 17: Disable BMC 2nd boot watchdog timer
> * 16: Decode SuperIO address 0x2E
> * 15: VGA Class Code
> * 14: Enable LPC dedicated reset pin
> * 13:12: Enable SPI Master and SPI Slave to AHB Bridge
> * 11:10: Select CPU:AHB ratio = 2:1
> * 9:8: Select 384MHz H-PLL
> * 7: Configure MAC#2 for RMII/NCSI
> * 6: Configure MAC#1 for RMII/NCSI
> * 5: No VGA BIOS ROM
> * 4: Boot using 32bit SPI address mode
> * 3:2: Select 16MB VGA memory
> * 1:0: Boot from SPI flash memory
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 2/3] ast2400: Integrate the SCU model and set silicon revision

[Cédric Le Goater]

On 06/23/2016 04:15 AM, Andrew Jeffery wrote:
> By specifying the silicon revision we select the appropriate reset
> values for the SoC.
> 
> Additionally, expose hardware strapping properties aliasing those
> provided by the SCU for board-specific configuration.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Peter Maydell]

On 23 June 2016 at 03:15, Andrew Jeffery <andrew@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 <andrew@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 <andrew@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

Re: [Qemu-devel] [PATCH v2 2/3] ast2400: Integrate the SCU model and set silicon revision

[Peter Maydell]

On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> By specifying the silicon revision we select the appropriate reset
> values for the SoC.
>
> Additionally, expose hardware strapping properties aliasing those
> provided by the SCU for board-specific configuration.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Peter Maydell]

On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> The magic constant configures the following options:
>
> * 28:27: Configure DRAM size as 256MB
> * 26:24: DDR3 SDRAM with CL = 6, CWL = 5
> * 23: Configure 24/48MHz CLKIN
> * 22: Disable GPIOE pass-through mode
> * 21: Disable GPIOD pass-through mode
> * 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
> * 19: Disable ACPI
> * 18: Configure 48MHz CLKIN
> * 17: Disable BMC 2nd boot watchdog timer
> * 16: Decode SuperIO address 0x2E
> * 15: VGA Class Code
> * 14: Enable LPC dedicated reset pin
> * 13:12: Enable SPI Master and SPI Slave to AHB Bridge
> * 11:10: Select CPU:AHB ratio = 2:1
> * 9:8: Select 384MHz H-PLL
> * 7: Configure MAC#2 for RMII/NCSI
> * 6: Configure MAC#1 for RMII/NCSI
> * 5: No VGA BIOS ROM
> * 4: Boot using 32bit SPI address mode
> * 3:2: Select 16MB VGA memory
> * 1:0: Boot from SPI flash memory

Maybe we should say this in a comment in the code?

> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/arm/palmetto-bmc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> index a51d960510ee..b8eed21348d8 100644
> --- a/hw/arm/palmetto-bmc.c
> +++ b/hw/arm/palmetto-bmc.c
> @@ -44,6 +44,8 @@ static void palmetto_bmc_init(MachineState *machine)
>                                  &bmc->ram);
>      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
>                                     &error_abort);
> +    object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
> +                            &error_abort);
>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>                               &error_abort);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Peter Maydell]

On 23 June 2016 at 03:15, Andrew Jeffery <andrew@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.

> +static Property aspeed_scu_properties[] = {
> +    DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
> +    DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
> +    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap1, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

You don't seem to specify in the board layer or the SoC layer
any of these except hw-strap1, so should the default values
for these really all be zero?

I suspect silicon-rev at least should either have a default
value specified here, or have the SoC layer specify it.
(It probably should not be specified at the board level.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 11745 bytes --]

On Thu, 2016-06-23 at 18:37 +0100, Peter Maydell wrote:
> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@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 <andrew@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 <andrew@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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] hw/misc: Add a model for the ASPEED System Control Unit

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On Thu, 2016-06-23 at 18:42 +0100, Peter Maydell wrote:
> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@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.
> > 
> > +static Property aspeed_scu_properties[] = {
> > +    DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
> > +    DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
> > +    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap1, 0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> You don't seem to specify in the board layer or the SoC layer
> any of these except hw-strap1, so should the default values
> for these really all be zero?

Both strap register default values are 0 according to the datasheet.

> 
> I suspect silicon-rev at least should either have a default
> value specified here, or have the SoC layer specify it.
> (It probably should not be specified at the board level.)

I intended to set silicon-rev in the SoC layer, so I'll fix patch 2/3.
With the addition of sanity checking in the SCU's realise() we'll catch
the case where it's an invalid value (eg 0). I don't think it's right
to plow ahead with an unexpected configuration if a chosen default
value doesn't match the SoC at hand.

Maybe I shouldn't send patches with a heavy head cold :/

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

On Thu, 2016-06-23 at 18:39 +0100, Peter Maydell wrote:
> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > The magic constant configures the following options:
> > 
> > * 28:27: Configure DRAM size as 256MB
> > * 26:24: DDR3 SDRAM with CL = 6, CWL = 5
> > * 23: Configure 24/48MHz CLKIN
> > * 22: Disable GPIOE pass-through mode
> > * 21: Disable GPIOD pass-through mode
> > * 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
> > * 19: Disable ACPI
> > * 18: Configure 48MHz CLKIN
> > * 17: Disable BMC 2nd boot watchdog timer
> > * 16: Decode SuperIO address 0x2E
> > * 15: VGA Class Code
> > * 14: Enable LPC dedicated reset pin
> > * 13:12: Enable SPI Master and SPI Slave to AHB Bridge
> > * 11:10: Select CPU:AHB ratio = 2:1
> > * 9:8: Select 384MHz H-PLL
> > * 7: Configure MAC#2 for RMII/NCSI
> > * 6: Configure MAC#1 for RMII/NCSI
> > * 5: No VGA BIOS ROM
> > * 4: Boot using 32bit SPI address mode
> > * 3:2: Select 16MB VGA memory
> > * 1:0: Boot from SPI flash memory
> Maybe we should say this in a comment in the code?

The list describes our specific value choices in the register's
bitfields rather than fully documenting the bitfields and values. If
you prefer I could switch to the latter and make it a comment, but
failing that my only thought was if we tweaked the value the comment
maybe come out of sync. By putting our choices in the commit message
the description is at least accurate for what was configured at the
time.

However I don't expect we will tweak the value...

> 
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  hw/arm/palmetto-bmc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
> > index a51d960510ee..b8eed21348d8 100644
> > --- a/hw/arm/palmetto-bmc.c
> > +++ b/hw/arm/palmetto-bmc.c
> > @@ -44,6 +44,8 @@ static void palmetto_bmc_init(MachineState *machine)
> >                                  &bmc->ram);
> >      object_property_add_const_link(OBJECT(&bmc->soc), "ram", OBJECT(&bmc->ram),
> >                                     &error_abort);
> > +    object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
> > +                            &error_abort);
> >      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
> >                               &error_abort);
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Peter Maydell]

On 24 June 2016 at 03:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2016-06-23 at 18:39 +0100, Peter Maydell wrote:
>> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
>> >
>> > The magic constant configures the following options:
>> >
>> > * 28:27: Configure DRAM size as 256MB
>> > * 26:24: DDR3 SDRAM with CL = 6, CWL = 5
>> > * 23: Configure 24/48MHz CLKIN
>> > * 22: Disable GPIOE pass-through mode
>> > * 21: Disable GPIOD pass-through mode
>> > * 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
>> > * 19: Disable ACPI
>> > * 18: Configure 48MHz CLKIN
>> > * 17: Disable BMC 2nd boot watchdog timer
>> > * 16: Decode SuperIO address 0x2E
>> > * 15: VGA Class Code
>> > * 14: Enable LPC dedicated reset pin
>> > * 13:12: Enable SPI Master and SPI Slave to AHB Bridge
>> > * 11:10: Select CPU:AHB ratio = 2:1
>> > * 9:8: Select 384MHz H-PLL
>> > * 7: Configure MAC#2 for RMII/NCSI
>> > * 6: Configure MAC#1 for RMII/NCSI
>> > * 5: No VGA BIOS ROM
>> > * 4: Boot using 32bit SPI address mode
>> > * 3:2: Select 16MB VGA memory
>> > * 1:0: Boot from SPI flash memory
>> Maybe we should say this in a comment in the code?
>
> The list describes our specific value choices in the register's
> bitfields rather than fully documenting the bitfields and values. If
> you prefer I could switch to the latter and make it a comment, but
> failing that my only thought was if we tweaked the value the comment
> maybe come out of sync. By putting our choices in the commit message
> the description is at least accurate for what was configured at the
> time.

I'd just like some idea of where the magic number comes
from. At the moment the source code doesn't even have a
reference to a data sheet that would indicate where to look.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] palmetto-bmc: Configure the SCU's hardware strapping register

[Cédric Le Goater]

On 06/24/2016 12:55 PM, Peter Maydell wrote:
> On 24 June 2016 at 03:21, Andrew Jeffery <andrew@aj.id.au> wrote:
>> On Thu, 2016-06-23 at 18:39 +0100, Peter Maydell wrote:
>>> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
>>>>
>>>> The magic constant configures the following options:
>>>>
>>>> * 28:27: Configure DRAM size as 256MB
>>>> * 26:24: DDR3 SDRAM with CL = 6, CWL = 5
>>>> * 23: Configure 24/48MHz CLKIN
>>>> * 22: Disable GPIOE pass-through mode
>>>> * 21: Disable GPIOD pass-through mode
>>>> * 20: Enable LPC decode of SuperIO 0x2E/0x4E addresses
>>>> * 19: Disable ACPI
>>>> * 18: Configure 48MHz CLKIN
>>>> * 17: Disable BMC 2nd boot watchdog timer
>>>> * 16: Decode SuperIO address 0x2E
>>>> * 15: VGA Class Code
>>>> * 14: Enable LPC dedicated reset pin
>>>> * 13:12: Enable SPI Master and SPI Slave to AHB Bridge
>>>> * 11:10: Select CPU:AHB ratio = 2:1
>>>> * 9:8: Select 384MHz H-PLL
>>>> * 7: Configure MAC#2 for RMII/NCSI
>>>> * 6: Configure MAC#1 for RMII/NCSI
>>>> * 5: No VGA BIOS ROM
>>>> * 4: Boot using 32bit SPI address mode
>>>> * 3:2: Select 16MB VGA memory
>>>> * 1:0: Boot from SPI flash memory
>>> Maybe we should say this in a comment in the code?
>>
>> The list describes our specific value choices in the register's
>> bitfields rather than fully documenting the bitfields and values. If
>> you prefer I could switch to the latter and make it a comment, but
>> failing that my only thought was if we tweaked the value the comment
>> maybe come out of sync. By putting our choices in the commit message
>> the description is at least accurate for what was configured at the
>> time.
> 
> I'd just like some idea of where the magic number comes
> from. At the moment the source code doesn't even have a
> reference to a data sheet that would indicate where to look.

Yes. I think the HW_STRAP1 register deserves its list of defines. 
There are some differences between the SOCs. With defines, it will 
be easier to build and read the values in the platform file. 

I can do that with a patch I will probably need to unset SPI boot.

Thanks,

C.