On Mon, 2016-06-20 at 14:57 +0100, Peter Maydell wrote: > On 20 June 2016 at 04:44, Andrew Jeffery <[email protected]> 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
signature.asc
Description: This is a digitally signed message part
