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

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On Thu, 2016-06-23 at 18:42 +0100, Peter Maydell wrote:
> On 23 June 2016 at 03:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > The SCU is a collection of chip-level control registers that manage the
> > various functions supported by ASPEED SoCs. 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 (eg AST2400 vs AST2500). A qdev
> > property is exposed so that the integrating SoC model can configure the
> > silicon revision, which in-turn selects the appropriate reset values.
> > Further qdev properties are exposed so the board model can configure the
> > board-dependent hardware strapping.
> > 
> > Almost all provided AST2400 reset values are specified by the datasheet.
> > The notable exception is SOC_SCRATCH1, where we mark the DRAM as
> > successfully initialised to avoid unnecessary dark corners in the SoC's
> > u-boot support.
> > 
> > +static Property aspeed_scu_properties[] = {
> > +    DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
> > +    DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
> > +    DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap1, 0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> You don't seem to specify in the board layer or the SoC layer
> any of these except hw-strap1, so should the default values
> for these really all be zero?

Both strap register default values are 0 according to the datasheet.

> 
> I suspect silicon-rev at least should either have a default
> value specified here, or have the SoC layer specify it.
> (It probably should not be specified at the board level.)

I intended to set silicon-rev in the SoC layer, so I'll fix patch 2/3.
With the addition of sanity checking in the SCU's realise() we'll catch
the case where it's an invalid value (eg 0). I don't think it's right
to plow ahead with an unexpected configuration if a chosen default
value doesn't match the SoC at hand.

Maybe I shouldn't send patches with a heavy head cold :/

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]