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

[Cédric Le Goater]

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.
  
The AspeedSocClass would be similar to the AspeedBoardConfig struct in 
the current patchset, plus the cpu model. How would that be ?   

>> 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. Being able to alias the 
different 'silicon-rev' properties to a common one would be nice though.
 
>>> 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. 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/