Re: [Qemu-arm] [PATCH 1/6] palmetto-bmc: add a "silicon-rev" property at the soc level

[Andrew Jeffery]

On Sat, 2016-07-30 at 10:35 +0200, Cédric Le Goater wrote:
> On 07/29/2016 03:16 AM, Andrew Jeffery wrote:
> > 
> > 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.
> OK. To be cleaner, we could add a AspeedSoCClass and a set of predefined 
> subclasses for each revision we support, that we would instantiate at the 
> platform level.

Yes - an AspeedSocClass is the way I went when I briefly started to
sketch out a patch to make my thoughts more concrete.

>   
> The AspeedSocClass would be similar to the AspeedBoardConfig struct in 
> the current patchset, plus the cpu model. How would that be ?  

Sounds good to me, though hw-strap1 still needs to be set by the
platform and not the SoC.

>  
> 
> > 
> > > 
> > > 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.
> yes that would be nicer not to have to re-set the silicon rev of the 
> controllers of a SoC.
> 
> > 
> > > 
> > >  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.
> No. your point on the SoC reuse is very valid. I try not to overspecify 
> too early but I agree I took a little shortcut. I will kick a v3 with
> the above. It should not be too much of a change.
> 
> > 
> > > 
> > > 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).
> So I understand that the 'silicon-rev' location is not the biggest concern
> and we can keep it as it is in the patchset.

Yes, sounds good. Though with this approach we shouldn't need to fetch
the value out and poke it into the SDMC...

> Being able to alias the 
> different 'silicon-rev' properties to a common one would be nice though.

... which means we shouldn't need this behaviour. But it sounds nice
all-the-same.

>  
> > 
> > > 
> > > > 
> > > > 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?
> Looks good. However I don't think there is no need for init() and realize() 
> ops right now. A .class_data should be sufficient. 

Right - we should do what is elegant. I didn't poke too deeply, I was
just sketching out something that I thought would demonstrate the
layering.

Cheers,

Andrew

> see this patch [1]. I still 
> need to rework that i2c patch btw.  
> 
> We can rework the SoC realize() if the need arise.
> 
> Cheers,
> 
> C.
> 
> [1] https://patchwork.kernel.org/patch/9129887/

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