qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property


From: Andrew Jeffery
Subject: Re: [Qemu-devel] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level
Date: Fri, 29 Jul 2016 10:46:13 +0930

On Thu, 2016-07-28 at 09:51 +0200, Cédric Le Goater wrote:
> On 07/28/2016 04:14 AM, Andrew Jeffery wrote:
> > 
> > On Wed, 2016-07-27 at 18:46 +0200, Cédric Le Goater wrote:
> > > 
> > > The SCU controler holds the board revision number in its 0x7C
> > > register. Let's use an alias to link a "silicon-rev" property of the
> > > soc to the "silicon-rev" property of the SCU controler.
> > > 
> > > The SDMC controler "silicon-rev" property is derived from the SCU one
> > > at realize time. I did not find a better way to handle this part.
> > > Links and aliases being a one-to-one relation, I could not use one of
> > > them. I might wrong though.
> > Are we trying to over-use the silicon-rev value (it would seem so at
> > least in the face of the link/alias constraints)? We know which SDMC
> > revision we need for each SoC and we'll be modelling an explicit SoC
> > revision, so should we instead set a separate property on the SDMC in
> > the SoCs' respective initialise functions (and leave silicon-rev to the
> > SCU)? 
> This is the case. no ? 

No, because you are selecting the SDMC configuration from the silicon-
rev rather than letting e.g. the SDMC configuration define which
silicon-rev the SoC takes.

My thinking is the silicon rev is a property of the SoC. The platform
should select a SoC to use and not be setting silicon revision values,
that is I think it's a layering violation for the platform to be
setting the silicon-rev (and also the CPU).

We also wind up in a situation where the ast2500 soc identifies as an
ast2400 in TypeInfo.name due to the approach to reuse by this series.

> 
> SCU holds the silicon-rev value. The patch adds a property alias to the 
> SCU 'silicon-rev' property at the soc level. This is convenient

Right, but "convenient" here is a bit of a stretch given we are
subsequently fetching the value out of the SCU to select the SDMC
configuration. You might argue that it's due to limitations of the
property alias/link API, but maybe we could rearrange things so this
goes away.

>  for the
> platform to initialize the soc. This is similar to what the rpi2 does,
> which goes one level in the aliasing.

Okay, maybe I'm barking up trees unnecessarily.

> 
> Then, at initialize time, the SCU 'silicon-rev' property value is read
> to initialize the SDMC controller. If we have more controllers in the 
> future needing 'silicon-rev,  we could follow the same pattern. Not 
> saying this is perfect. 
> 
> What I would have liked to do, is to link all the 'silicon-rev' do
> the SCU one. I did not find a way.

Yes, that would be nice. I did briefly poke around to see if there was
a solution to the link/alias issue but it seems not. 

> 
> > 
> > My thought was the silicon-rev value is reflective of the SoC
> > design rather than the other way around - but maybe that's splitting
> > hairs. 
> ah. is your concern about which object is holding the value ? If so,
> I thought that keeping it where it belongs on real HW was the better 
> option, that is in SCU, and then build from there.

No, that's not my concern, but I agree that it would not reflect the
hardware if there was a property on the SoC (i.e. there is nowhere
besides the SCU that the value is held).

> 
> > 
> > It would also be trading off a bit of ugliness in this patch for
> > potential bugs if the properties get out of sync.
> This is the exact the purpose of the patch ! I failed to make it feel
> that way :)

Right. I think we need another layer of abstraction, essentially a soc
configuration struct that is accessed by what are currently the
ast2400_{init,realise}() functions. This will capture differences like
changes in IO addresses, changes to IP behaviour, the CPU types and
ultimately the silicon-rev value. What is now ast2400_init() and
ast2400_realise() can become generic aspeed_soc_{init,realise}(), and
then we wrap calls to this up in SoC-specific
ast2{4,5}00_{init,realise}() where we set the configuration struct. It
is a bit more work but I think the result would better reflect the
hardware and avoid introducing what feel to me like layering
violations.

Thoughts?

Andrew

Attachment: signature.asc
Description: This is a digitally signed message part


reply via email to

[Prev in Thread] Current Thread [Next in Thread]