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

[Peter Maydell]

On 20 June 2016 at 04:44, Andrew Jeffery <address@hidden> 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