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