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

[Andrew Jeffery]

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)? 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. It would also be trading off a bit of ugliness in this patch for
potential bugs if the properties get out of sync.

Cheers,

Andrew

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

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