* [Qemu-devel] [PATCH 0/2] Add ASPEED SCU device @ 2016-06-16 7:48 Andrew Jeffery 2016-06-16 7:48 ` [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit Andrew Jeffery 2016-06-16 7:48 ` [Qemu-devel] [PATCH 2/2] ast2400: Integrate the SCU model and configure reset values Andrew Jeffery 0 siblings, 2 replies; 9+ messages in thread From: Andrew Jeffery @ 2016-06-16 7:48 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, qemu-devel, qemu-arm, Andrew Jeffery Hi all, These are two patches implementing minimal functionality for the ASPEED System Control Unit device and integrating it into the AST2400 SoC model. 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. Cheers, Andrew Andrew Jeffery (2): hw/misc: Add a model for the ASPEED System Control Unit ast2400: Integrate the SCU model and configure reset values hw/arm/ast2400.c | 57 +++++++++++++++++ hw/misc/Makefile.objs | 1 + hw/misc/aspeed_scu.c | 147 +++++++++++++++++++++++++++++++++++++++++++ include/hw/arm/ast2400.h | 2 + include/hw/misc/aspeed_scu.h | 108 +++++++++++++++++++++++++++++++ trace-events | 3 + 6 files changed, 318 insertions(+) create mode 100644 hw/misc/aspeed_scu.c create mode 100644 include/hw/misc/aspeed_scu.h -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-16 7:48 [Qemu-devel] [PATCH 0/2] Add ASPEED SCU device Andrew Jeffery @ 2016-06-16 7:48 ` Andrew Jeffery 2016-06-17 14:22 ` Peter Maydell 2016-06-16 7:48 ` [Qemu-devel] [PATCH 2/2] ast2400: Integrate the SCU model and configure reset values Andrew Jeffery 1 sibling, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2016-06-16 7:48 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, qemu-devel, qemu-arm, Andrew Jeffery The SCU is a collection of chip-level control registers that manage the various functions supported by the AST2400. 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. qdev properties are exposed so that the integrating SoC model can configure the appropriate reset values. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Joel Stanley <joel@jms.id.au> --- hw/misc/Makefile.objs | 1 + hw/misc/aspeed_scu.c | 147 +++++++++++++++++++++++++++++++++++++++++++ include/hw/misc/aspeed_scu.h | 108 +++++++++++++++++++++++++++++++ trace-events | 3 + 4 files changed, 259 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..492cf82af425 --- /dev/null +++ b/hw/misc/aspeed_scu.c @@ -0,0 +1,147 @@ +/* + * 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 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 (ASPEED_SCU_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 ASPEED_SCU_WAKEUP_EN: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + break; + } + + return s->regs[ASPEED_SCU_TO_REG(offset)]; +} + +static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data, + unsigned size) +{ + AspeedSCUState *s = ASPEED_SCU(opaque); + + if (ASPEED_SCU_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 > ASPEED_SCU_PROT_KEY && offset < ASPEED_SCU_CPU2_BASE_SEG1 && + s->regs[ASPEED_SCU_TO_REG(ASPEED_SCU_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 ASPEED_SCU_FREQ_CNTR_EVAL: + case ASPEED_SCU_VGA_SCRATCH1 ... ASPEED_SCU_VGA_SCRATCH8: + case ASPEED_SCU_RNG_DATA: + case ASPEED_SCU_REV_ID: + case ASPEED_SCU_FREE_CNTR4: + case ASPEED_SCU_FREE_CNTR4_EXT: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Write to read-only offset 0x%" HWADDR_PRIx "\n", + __func__, offset); + return; + } + + s->regs[ASPEED_SCU_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); + + memcpy(s->regs, s->reset, sizeof(s->regs)); +} + +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.new-vic", + .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_ARRAY("reset", AspeedSCUState, num_resets, reset, + qdev_prop_uint32, uint32_t), + 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..f95c5c8a7d74 --- /dev/null +++ b/include/hw/misc/aspeed_scu.h @@ -0,0 +1,108 @@ +/* + * 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) + +#define ASPEED_SCU_PROT_KEY 0x00 +#define ASPEED_SCU_SYS_RST_CTRL 0x04 +#define ASPEED_SCU_CLK_SEL 0x08 +#define ASPEED_SCU_CLK_STOP_CTRL 0x0C +#define ASPEED_SCU_FREQ_CNTR_CTRL 0x10 +#define ASPEED_SCU_FREQ_CNTR_EVAL 0x14 +#define ASPEED_SCU_IRQ_CTRL 0x18 +#define ASPEED_SCU_D2PLL_PARAM 0x1C +#define ASPEED_SCU_MPLL_PARAM 0x20 +#define ASPEED_SCU_HPLL_PARAM 0x24 +#define ASPEED_SCU_FREQ_CNTR_RANGE 0x28 +#define ASPEED_SCU_MISC_CTRL1 0x2C +#define ASPEED_SCU_PCI_CTRL1 0x30 +#define ASPEED_SCU_PCI_CTRL2 0x34 +#define ASPEED_SCU_PCI_CTRL3 0x38 +#define ASPEED_SCU_SYS_RST_STATUS 0x3C +#define ASPEED_SCU_SOC_SCRATCH1 0x40 +#define ASPEED_SCU_SOC_SCRATCH2 0x44 +#define ASPEED_SCU_MAC_CLK_DELAY 0x48 +#define ASPEED_SCU_MISC_CTRL2 0x4C +#define ASPEED_SCU_VGA_SCRATCH1 0x50 +#define ASPEED_SCU_VGA_SCRATCH2 0x54 +#define ASPEED_SCU_VGA_SCRATCH3 0x58 +#define ASPEED_SCU_VGA_SCRATCH4 0x5C +#define ASPEED_SCU_VGA_SCRATCH5 0x60 +#define ASPEED_SCU_VGA_SCRATCH6 0x64 +#define ASPEED_SCU_VGA_SCRATCH7 0x68 +#define ASPEED_SCU_VGA_SCRATCH8 0x6C +#define ASPEED_SCU_HW_STRAP1 0x70 +#define ASPEED_SCU_RNG_CTRL 0x74 +#define ASPEED_SCU_RNG_DATA 0x78 +#define ASPEED_SCU_REV_ID 0x7C +#define ASPEED_SCU_PINMUX_CTRL1 0x80 +#define ASPEED_SCU_PINMUX_CTRL2 0x84 +#define ASPEED_SCU_PINMUX_CTRL3 0x88 +#define ASPEED_SCU_PINMUX_CTRL4 0x8C +#define ASPEED_SCU_PINMUX_CTRL5 0x90 +#define ASPEED_SCU_PINMUX_CTRL6 0x94 +#define ASPEED_SCU_WDT_RST_CTRL 0x9C +#define ASPEED_SCU_PINMUX_CTRL7 0xA0 +#define ASPEED_SCU_PINMUX_CTRL8 0xA4 +#define ASPEED_SCU_PINMUX_CTRL9 0xA8 +#define ASPEED_SCU_WAKEUP_EN 0xC0 +#define ASPEED_SCU_WAKEUP_CTRL 0xC4 +#define ASPEED_SCU_HW_STRAP2 0xD0 +#define ASPEED_SCU_FREE_CNTR4 0xE0 +#define ASPEED_SCU_FREE_CNTR4_EXT 0xE4 +#define ASPEED_SCU_CPU2_CTRL 0x100 +#define ASPEED_SCU_CPU2_BASE_SEG1 0x104 +#define ASPEED_SCU_CPU2_BASE_SEG2 0x108 +#define ASPEED_SCU_CPU2_BASE_SEG3 0x10C +#define ASPEED_SCU_CPU2_BASE_SEG4 0x110 +#define ASPEED_SCU_CPU2_BASE_SEG5 0x114 +#define ASPEED_SCU_CPU2_CACHE_CTRL 0x118 +#define ASPEED_SCU_UART_HPLL_CLK 0x160 +#define ASPEED_SCU_PCIE_CTRL 0x180 +#define ASPEED_SCU_BMC_MMIO_CTRL 0x184 +#define ASPEED_SCU_RELOC_DECODE_BASE1 0x188 +#define ASPEED_SCU_RELOC_DECODE_BASE2 0x18C +#define ASPEED_SCU_MAILBOX_DECODE_BASE 0x190 +#define ASPEED_SCU_SRAM_DECODE_BASE1 0x194 +#define ASPEED_SCU_SRAM_DECODE_BASE2 0x198 +#define ASPEED_SCU_BMC_REV_ID 0x19C +#define ASPEED_SCU_BMC_DEV_ID 0x1A4 + +#define ASPEED_SCU_TO_REG(offset) ((offset) >> 2) + +typedef struct AspeedSCUState { + /*< private >*/ + SysBusDevice parent_obj; + + /*< public >*/ + MemoryRegion iomem; + + uint32_t regs[ASPEED_SCU_NR_REGS]; + uint32_t *reset; + uint32_t num_resets; +} AspeedSCUState; + +typedef struct AspeedSCUResetCfg { + uint32_t offset; + uint32_t val; +} AspeedSCUResetCfg; + +void aspeed_scu_configure_reset(AspeedSCUState *scu, + const struct AspeedSCUResetCfg vals[], int n, Error **errp); + +#endif /* ASPEED_SCU_H */ diff --git a/trace-events b/trace-events index 2f14205de047..24749c414ee7 100644 --- a/trace-events +++ b/trace-events @@ -2164,3 +2164,6 @@ e1000e_cfg_support_virtio(bool support) "Virtio header supported: %d" e1000e_vm_state_running(void) "VM state is running" e1000e_vm_state_stopped(void) "VM state is stopped" + +# 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-16 7:48 ` [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit Andrew Jeffery @ 2016-06-17 14:22 ` Peter Maydell 2016-06-20 3:44 ` Andrew Jeffery 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2016-06-17 14:22 UTC (permalink / raw) To: Andrew Jeffery Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm On 16 June 2016 at 08:48, Andrew Jeffery <andrew@aj.id.au> wrote: > The SCU is a collection of chip-level control registers that manage the > various functions supported by the AST2400. 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. qdev properties are exposed so that > the integrating SoC model can configure the appropriate reset values. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Reviewed-by: Cédric Le Goater <clg@kaod.org> > Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > +static Property aspeed_scu_properties[] = { > + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, > + qdev_prop_uint32, uint32_t), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) This seems like a very unwieldy way of specifying the reset values for this device. Are they really all fully configurable in the hardware? It seems unlikely. I'd much rather see something that looks more like what you might plausibly be configuring when wiring up the SoC, which might be some version/revision numbers and/or some particular tweakable parameters. thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-17 14:22 ` Peter Maydell @ 2016-06-20 3:44 ` Andrew Jeffery 2016-06-20 13:57 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2016-06-20 3:44 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm [-- Attachment #1: Type: text/plain, Size: 2379 bytes --] On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: On 16 June 2016 at 08:48, Andrew Jeffery <andrew@aj.id.au> wrote: The SCU is a collection of chip-level control registers that manage the various functions supported by the AST2400. 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. qdev properties are exposed so that the integrating SoC model can configure the appropriate reset values. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Joel Stanley <joel@jms.id.au> --- +static Property aspeed_scu_properties[] = { + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, + qdev_prop_uint32, uint32_t), + DEFINE_PROP_END_OF_LIST(), +}; + +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) This seems like a very unwieldy way of specifying the reset values for this device. Are they really all fully configurable in the hardware? It seems unlikely. I'd much rather see something that looks more like what you might plausibly be configuring when wiring up the SoC, which might be some version/revision numbers and/or some particular tweakable parameters. Right. I left out some context which may clear things up: We are working with two SoCs at the moment, the AST2400 and AST2500 (hopefully the AST2500 patches will be sent to the list soon). I wanted to abstract the configuration to cater for the differences in register values between the SoCs, less so for wiring the one SoC up in a different fashion. For what it's worth, out of 86 registers defined in the IO space between the two SoCs, 37 take the same value and 49 differ. I can rework the commit message to say as much. Separately, the qdev array approach was lifted from your commit 9c7d489379c2 hw/vexpress: Set reset values for daughterboard oscillators. Another approach would be to set the members of the SCU's reset array directly as we know the type. That seems less convoluted but my gut instinct was that wasn't how we should be configuring the devices. Is qdev the right way to go in the SCU's case? Cheers, Andrew [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-20 3:44 ` Andrew Jeffery @ 2016-06-20 13:57 ` Peter Maydell 2016-06-21 3:49 ` Andrew Jeffery 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2016-06-20 13:57 UTC (permalink / raw) To: Andrew Jeffery Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm On 20 June 2016 at 04:44, Andrew Jeffery <andrew@aj.id.au> wrote: > On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: >> +static Property aspeed_scu_properties[] = { >> + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, >> + qdev_prop_uint32, uint32_t), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) >> This seems like a very unwieldy way of specifying the reset values >> for this device. Are they really all fully configurable in the >> hardware? It seems unlikely. I'd much rather see something that >> looks more like what you might plausibly be configuring when wiring >> up the SoC, which might be some version/revision numbers and/or >> some particular tweakable parameters. > > Right. I left out some context which may clear things up: We are > working with two SoCs at the moment, the AST2400 and AST2500 (hopefully > the AST2500 patches will be sent to the list soon). I wanted to > abstract the configuration to cater for the differences in register > values between the SoCs, less so for wiring the one SoC up in a > different fashion. For what it's worth, out of 86 registers defined in > the IO space between the two SoCs, 37 take the same value and 49 > differ. I think there are a couple of plausible ways you might model this: (a) just have a single property for "revision" which corresponds to the revision of this bit of silicon IP within the SoC; the model of the device itself then knows what the reset state is for this revision of the device. (b) ditto, but also have some configurable flags where relevant (ie approximately where it's the same IP rev within the SoC but it's been configured by tying down different config lines; for instance hw/dma/pl330.c has a collection of properties which match the configurable knobs for the hardware.) You might or might not have enough visibility into the thing to know which of these is closest to what the real hardware is doing; if not then it's a matter of taste, looking at what is varying between the two and what isn't, etc. But "board level specifies all the register reset values" is definitely far too broad and generalised an API, I think. > Separately, the qdev array approach was lifted from your commit > 9c7d489379c2 hw/vexpress: Set reset values for daughterboard > oscillators. You'll notice that we only configure the specific things that need configuring with interfaces specific to those things (eg "daughterboard clocks" and "daughterboard voltages" are separate), not a single "have a complete set of register values" API. > Another approach would be to set the members of the SCU's > reset array directly as we know the type. That seems less convoluted > but my gut instinct was that wasn't how we should be configuring the > devices. Is qdev the right way to go in the SCU's case? We should definitely use qdev rather than poking directly at the device's internal state struct. thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-20 13:57 ` Peter Maydell @ 2016-06-21 3:49 ` Andrew Jeffery 2016-06-21 6:56 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2016-06-21 3:49 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm [-- Attachment #1: Type: text/plain, Size: 3705 bytes --] On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: > On 20 June 2016 at 04:44, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Fri, 2016-06-17 at 15:22 +0100, Peter Maydell wrote: > > > > > > +static Property aspeed_scu_properties[] = { > > > + DEFINE_PROP_ARRAY("reset", AspeedSCUState, num_resets, reset, > > > + qdev_prop_uint32, uint32_t), > > > + DEFINE_PROP_END_OF_LIST(), > > > +}; > > > + > > > +#define ASPEED_SCU_NR_REGS (0x1A8 >> 2) > > > This seems like a very unwieldy way of specifying the reset values > > > for this device. Are they really all fully configurable in the > > > hardware? It seems unlikely. I'd much rather see something that > > > looks more like what you might plausibly be configuring when wiring > > > up the SoC, which might be some version/revision numbers and/or > > > some particular tweakable parameters. > > Right. I left out some context which may clear things up: We are > > working with two SoCs at the moment, the AST2400 and AST2500 (hopefully > > the AST2500 patches will be sent to the list soon). I wanted to > > abstract the configuration to cater for the differences in register > > values between the SoCs, less so for wiring the one SoC up in a > > different fashion. For what it's worth, out of 86 registers defined in > > the IO space between the two SoCs, 37 take the same value and 49 > > differ. > I think there are a couple of plausible ways you might model this: > > (a) just have a single property for "revision" which corresponds > to the revision of this bit of silicon IP within the SoC; the > model of the device itself then knows what the reset state is > for this revision of the device. > (b) ditto, but also have some configurable flags where relevant > (ie approximately where it's the same IP rev within the SoC > but it's been configured by tying down different config lines; > for instance hw/dma/pl330.c has a collection of properties > which match the configurable knobs for the hardware.) Okay. I think (b) is the most appropriate. The board-controllable bits are primarily in the hardware strapping register. The register is composed of fields of mostly unrelated bits, so we could go two ways here: (1) expose the register through a single 32bit property (2) break out a property for each bitfield Do you have a preference? grepping the tree suggests there isn't a precedent for "large" numbers of properties* so maybe (2) is overkill, but (1) feels like it might fit into the overly-general-interface problem that we're trying to eliminate. * Seems the microblaze CPU defines the most with 9 properties? Approach (2) will leave us with 21 properties for the SCU. $ git grep -c DEFINE_PROP | sort -t: -k2 -r | head -n1 target-microblaze/cpu.c:9 > > You might or might not have enough visibility into the thing to > know which of these is closest to what the real hardware is doing; > if not then it's a matter of taste, looking at what is varying > between the two and what isn't, etc. But "board level specifies > all the register reset values" is definitely far too broad > and generalised an API, I think. > > > > > Separately, the qdev array approach was lifted from your commit > > 9c7d489379c2 hw/vexpress: Set reset values for daughterboard > > oscillators. > You'll notice that we only configure the specific things > that need configuring with interfaces specific to those things > (eg "daughterboard clocks" and "daughterboard voltages" are > separate), not a single "have a complete set of register values" API. Yes, I appreciate that now. Thanks. Andrew [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-21 3:49 ` Andrew Jeffery @ 2016-06-21 6:56 ` Peter Maydell 2016-06-21 7:11 ` Andrew Jeffery 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2016-06-21 6:56 UTC (permalink / raw) To: Andrew Jeffery Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm On 21 June 2016 at 04:49, Andrew Jeffery <andrew@aj.id.au> wrote: > On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: >> I think there are a couple of plausible ways you might model this: >> >> (a) just have a single property for "revision" which corresponds >> to the revision of this bit of silicon IP within the SoC; the >> model of the device itself then knows what the reset state is >> for this revision of the device. >> (b) ditto, but also have some configurable flags where relevant >> (ie approximately where it's the same IP rev within the SoC >> but it's been configured by tying down different config lines; >> for instance hw/dma/pl330.c has a collection of properties >> which match the configurable knobs for the hardware.) > > Okay. I think (b) is the most appropriate. The board-controllable bits > are primarily in the hardware strapping register. The register is > composed of fields of mostly unrelated bits, so we could go two ways > here: > > (1) expose the register through a single 32bit property > (2) break out a property for each bitfield > > Do you have a preference? grepping the tree suggests there isn't a > precedent for "large" numbers of properties* so maybe (2) is overkill, > but (1) feels like it might fit into the overly-general-interface > problem that we're trying to eliminate. > > * Seems the microblaze CPU defines the most with 9 properties? (You forgot '-n' on your sort rune.) hw/dma/pl330.c has 15. > Approach (2) will leave us with 21 properties for the SCU. What would the 21 properties be in this case? But yes, a single 32 bit property would probably be better. thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit 2016-06-21 6:56 ` Peter Maydell @ 2016-06-21 7:11 ` Andrew Jeffery 0 siblings, 0 replies; 9+ messages in thread From: Andrew Jeffery @ 2016-06-21 7:11 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, QEMU Developers, qemu-arm [-- Attachment #1: Type: text/plain, Size: 1898 bytes --] On Tue, 2016-06-21 at 07:56 +0100, Peter Maydell wrote: > On 21 June 2016 at 04:49, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: > > > > > > I think there are a couple of plausible ways you might model this: > > > > > > (a) just have a single property for "revision" which corresponds > > > to the revision of this bit of silicon IP within the SoC; the > > > model of the device itself then knows what the reset state is > > > for this revision of the device. > > > (b) ditto, but also have some configurable flags where relevant > > > (ie approximately where it's the same IP rev within the SoC > > > but it's been configured by tying down different config lines; > > > for instance hw/dma/pl330.c has a collection of properties > > > which match the configurable knobs for the hardware.) > > Okay. I think (b) is the most appropriate. The board-controllable bits > > are primarily in the hardware strapping register. The register is > > composed of fields of mostly unrelated bits, so we could go two ways > > here: > > > > (1) expose the register through a single 32bit property > > (2) break out a property for each bitfield > > > > Do you have a preference? grepping the tree suggests there isn't a > > precedent for "large" numbers of properties* so maybe (2) is overkill, > > but (1) feels like it might fit into the overly-general-interface > > problem that we're trying to eliminate. > > > > * Seems the microblaze CPU defines the most with 9 properties? > (You forgot '-n' on your sort rune.) > hw/dma/pl330.c has 15. Ugh. > > > > > Approach (2) will leave us with 21 properties for the SCU. > What would the 21 properties be in this case? > > But yes, a single 32 bit property would probably be better. Alright, I'll go ahead with that. Thanks for the feedback. Andrew [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] ast2400: Integrate the SCU model and configure reset values 2016-06-16 7:48 [Qemu-devel] [PATCH 0/2] Add ASPEED SCU device Andrew Jeffery 2016-06-16 7:48 ` [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit Andrew Jeffery @ 2016-06-16 7:48 ` Andrew Jeffery 1 sibling, 0 replies; 9+ messages in thread From: Andrew Jeffery @ 2016-06-16 7:48 UTC (permalink / raw) To: Peter Maydell Cc: Cédric Le Goater, Joel Stanley, qemu-devel, qemu-arm, Andrew Jeffery Almost all configured reset values are specified by the datasheet. The notable exception is ASPEED_SCU_SOC_SCRATCH1, where we mark the DRAM as successfully initialised by the SoC 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> Reviewed-by: Joel Stanley <joel@jms.id.au> --- hw/arm/ast2400.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/arm/ast2400.h | 2 ++ 2 files changed, 59 insertions(+) diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c index 4a9de0e10cbc..fc0da5c38557 100644 --- a/hw/arm/ast2400.c +++ b/hw/arm/ast2400.c @@ -24,12 +24,48 @@ #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 static const int uart_irqs[] = { 9, 32, 33, 34, 10 }; static const int timer_irqs[] = { 16, 17, 18, 35, 36, 37, 38, 39, }; +static const AspeedSCUResetCfg scu_reset[] = { + /* Values are defaults from the datasheet except where noted */ + { ASPEED_SCU_SYS_RST_CTRL, 0xFFCFFEDCU }, + { ASPEED_SCU_CLK_SEL, 0xF3F40000U }, + { ASPEED_SCU_CLK_STOP_CTRL, 0x19FC3E8BU }, + { ASPEED_SCU_D2PLL_PARAM, 0x00026108U }, + { ASPEED_SCU_MPLL_PARAM, 0x00030291U }, + { ASPEED_SCU_HPLL_PARAM, 0x00000291U }, + { ASPEED_SCU_MISC_CTRL1, 0x00000010U }, + { ASPEED_SCU_PCI_CTRL1, 0x20001A03U }, + { ASPEED_SCU_PCI_CTRL2, 0x20001A03U }, + { ASPEED_SCU_PCI_CTRL3, 0x04000030U }, + { ASPEED_SCU_SYS_RST_STATUS, 0x00000001U }, + { ASPEED_SCU_SOC_SCRATCH1, 0x000000C0U }, /* SoC completed DRAM init */ + { ASPEED_SCU_MISC_CTRL2, 0x00000023U }, + { ASPEED_SCU_HW_STRAP1, 0x120CE416U }, /* Extracted after boot */ + { ASPEED_SCU_RNG_CTRL, 0x0000000EU }, + { ASPEED_SCU_REV_ID, 0x02000303U }, + { ASPEED_SCU_PINMUX_CTRL2, 0x0000F000U }, + { ASPEED_SCU_PINMUX_CTRL3, 0x01000000U }, + { ASPEED_SCU_PINMUX_CTRL4, 0x000000FFU }, + { ASPEED_SCU_PINMUX_CTRL5, 0x0000A000U }, + { ASPEED_SCU_WDT_RST_CTRL, 0x003FFFF3U }, + { ASPEED_SCU_PINMUX_CTRL8, 0xFFFF0000U }, + { ASPEED_SCU_PINMUX_CTRL9, 0x000FFFFFU }, + { ASPEED_SCU_FREE_CNTR4, 0x000000FFU }, + { ASPEED_SCU_FREE_CNTR4_EXT, 0x000000FFU }, + { ASPEED_SCU_CPU2_BASE_SEG1, 0x80000000U }, + { ASPEED_SCU_CPU2_BASE_SEG4, 0x1E600000U }, + { ASPEED_SCU_CPU2_BASE_SEG5, 0xC0000000U }, + { ASPEED_SCU_UART_HPLL_CLK, 0x00001903U }, + { ASPEED_SCU_PCIE_CTRL, 0x0000007BU }, + { ASPEED_SCU_BMC_DEV_ID, 0x00002402U } +}; + /* * IO handlers: simply catch any reads/writes to IO addresses that aren't * handled by a device mapping. @@ -57,6 +93,7 @@ static const MemoryRegionOps ast2400_io_ops = { static void ast2400_init(Object *obj) { + int i; AST2400State *s = AST2400(obj); s->cpu = cpu_arm_init("arm926"); @@ -72,6 +109,18 @@ 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()); + + qdev_prop_set_uint32(DEVICE(&s->scu), "len-reset", ASPEED_SCU_NR_REGS); + for (i = 0; i < ARRAY_SIZE(scu_reset); i++) { + int reg = ASPEED_SCU_TO_REG(scu_reset[i].offset); + char *propname = g_strdup_printf("reset[%d]", reg); + qdev_prop_set_uint32(DEVICE(&s->scu), propname, scu_reset[i].val); + g_free(propname); + } } static void ast2400_realize(DeviceState *dev, Error **errp) @@ -110,6 +159,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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-21 7:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-16 7:48 [Qemu-devel] [PATCH 0/2] Add ASPEED SCU device Andrew Jeffery 2016-06-16 7:48 ` [Qemu-devel] [PATCH 1/2] hw/misc: Add a model for the ASPEED System Control Unit Andrew Jeffery 2016-06-17 14:22 ` Peter Maydell 2016-06-20 3:44 ` Andrew Jeffery 2016-06-20 13:57 ` Peter Maydell 2016-06-21 3:49 ` Andrew Jeffery 2016-06-21 6:56 ` Peter Maydell 2016-06-21 7:11 ` Andrew Jeffery 2016-06-16 7:48 ` [Qemu-devel] [PATCH 2/2] ast2400: Integrate the SCU model and configure reset values Andrew Jeffery
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.