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

[Andrew Jeffery <andrew@aj.id.au>]

[-- 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 --]