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