diff mbox

[v2,1/3] hw/misc: Add a model for the ASPEED System Control Unit

Message ID 1466648115-17015-2-git-send-email-andrew@aj.id.au
State New
Headers show

Commit Message

Andrew Jeffery June 23, 2016, 2:15 a.m. UTC 
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

Comments

Cédric Le Goater June 23, 2016, 6:27 a.m. UTC | #1 
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.
Peter Maydell June 23, 2016, 5:37 p.m. UTC | #2 
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
Peter Maydell June 23, 2016, 5:42 p.m. UTC | #3 
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
Andrew Jeffery June 24, 2016, 1:51 a.m. UTC | #4 
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
Andrew Jeffery June 24, 2016, 2:04 a.m. UTC | #5 
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
diff mbox

Patch

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