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
signature.asc
Description: This is a digitally signed message part