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

[Peter Maydell]

On 16 June 2016 at 08:48, Andrew Jeffery <address@hidden> 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 <address@hidden>
> Reviewed-by: Cédric Le Goater <address@hidden>
> Reviewed-by: Joel Stanley <address@hidden>
> ---
> +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