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

[Cédric Le Goater]

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 ? 

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 for the
platform to initialize the soc. This is similar to what the rpi2 does,
which goes one level in the aliasing.

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.

> 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.

> 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 :) 

Thanks,

C. 

>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/arm/ast2400.c      | 18 +++++++++++++-----
>>  hw/arm/palmetto-bmc.c |  2 ++
>>  2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index 136bf6464e1d..fa535065f765 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -84,8 +84,8 @@ static void ast2400_init(Object *obj)
>>      object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
>>      object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
>>      qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
>> -    qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
>> -                         AST2400_A0_SILICON_REV);
>> +    object_property_add_alias(obj, "silicon-rev", OBJECT(&s->scu),
>> +                              "silicon-rev", &error_abort);
>>      object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu),
>>                                "hw-strap1", &error_abort);
>>      object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
>> @@ -102,8 +102,6 @@ static void ast2400_init(Object *obj)
>>      object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
>>      object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
>>      qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
>> -    qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
>> -                         AST2400_A0_SILICON_REV);
>>  }
>>  
>>  static void ast2400_realize(DeviceState *dev, Error **errp)
>> @@ -111,6 +109,7 @@ static void ast2400_realize(DeviceState *dev, Error 
>> **errp)
>>      int i;
>>      AST2400State *s = AST2400(dev);
>>      Error *err = NULL, *local_err = NULL;
>> +    uint32_t silicon_rev;
>>  
>>      /* IO space */
>>      memory_region_init_io(&s->iomem, NULL, &ast2400_io_ops, NULL,
>> @@ -192,7 +191,16 @@ static void ast2400_realize(DeviceState *dev, Error 
>> **errp)
>>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
>>  
>>      /* SDMC - SDRAM Memory Controller */
>> -    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", &err);
>> +    silicon_rev = (uint32_t)
>> +        object_property_get_int(OBJECT(&s->scu), "silicon-rev", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    object_property_set_int(OBJECT(&s->sdmc), silicon_rev, "silicon-rev", 
>> &err);
>> +    object_property_set_bool(OBJECT(&s->sdmc), true, "realized", 
>> &local_err);
>> +    error_propagate(&err, local_err);
>>      if (err) {
>>          error_propagate(errp, err);
>>          return;
>> diff --git a/hw/arm/palmetto-bmc.c b/hw/arm/palmetto-bmc.c
>> index 54e29a865d88..1ee13d578899 100644
>> --- a/hw/arm/palmetto-bmc.c
>> +++ b/hw/arm/palmetto-bmc.c
>> @@ -74,6 +74,8 @@ static void palmetto_bmc_init(MachineState *machine)
>>                                     &error_abort);
>>      object_property_set_int(OBJECT(&bmc->soc), 0x120CE416, "hw-strap1",
>>                              &error_abort);
>> +    object_property_set_int(OBJECT(&bmc->soc), AST2400_A0_SILICON_REV,
>> +                            "silicon-rev", &error_abort);
>>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
>>                               &error_abort);
>>