Re: [PATCH 02/21] aspeed: i2c: Add ctrl_global_rsvd property

[Joel Stanley]

On Mon, 6 Jun 2022 at 15:08, Cédric Le Goater <clg@kaod.org> wrote:
>
> From: Joe Komlodi <komlodi@google.com>
>
> The Aspeed I2C controller is used across other SKUs that have different
> reserved bits for the ctrl_global_rsvd register.

I think rsvd stands for reserved? Lets spell out the full name in the
variable to keep it clear.

You could also call global_control_mask (or ctrl_global_mask if you
prefer), as it's a mask of valid bits.

>
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Change-Id: I606c5933c527274a9d2b0afe559b2e895767636c
> Message-Id: <20220331043248.2237838-3-komlodi@google.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/i2c/aspeed_i2c.h | 2 ++
>  hw/arm/aspeed_ast2600.c     | 2 ++
>  hw/i2c/aspeed_i2c.c         | 4 ++++
>  3 files changed, 8 insertions(+)
>
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 4b9be09274c7..3912fcc3ff53 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -71,6 +71,8 @@ struct AspeedI2CState {
>      MemoryRegion pool_iomem;
>      uint8_t pool[ASPEED_I2C_MAX_POOL_SIZE];
>
> +    uint32_t ctrl_global_rsvd;
> +
>      AspeedI2CBus busses[ASPEED_I2C_NR_BUSSES];
>      MemoryRegion *dram_mr;
>      AddressSpace dram_as;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index b0a4199b6960..cc57c8b437d8 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -375,6 +375,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
> Error **errp)
>      aspeed_soc_uart_init(s);
>
>      /* I2C */
> +    object_property_set_int(OBJECT(&s->i2c), "ctrl-global-rsvd", 0xfffc3e00,
> +                            &error_abort);
>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                               &error_abort);
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->i2c), errp)) {
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 03a4f5a91010..97eb9d57929c 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -648,6 +648,7 @@ static void aspeed_i2c_ctrl_write(void *opaque, hwaddr 
> offset,
>
>      switch (offset) {
>      case I2C_CTRL_GLOBAL:
> +        value &= ~s->ctrl_global_rsvd;

Is there value in printing a guest error when the reserved bits are set?

If not, is it worth having this property at all? It doesn't affect the
ability to model it.

>          s->ctrl_global = value;
>          break;
>      case I2C_CTRL_STATUS:
> @@ -730,6 +731,7 @@ static const VMStateDescription aspeed_i2c_vmstate = {
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(intr_status, AspeedI2CState),
> +        VMSTATE_UINT32(ctrl_global_rsvd, AspeedI2CState),
>          VMSTATE_STRUCT_ARRAY(busses, AspeedI2CState,
>                               ASPEED_I2C_NR_BUSSES, 1, aspeed_i2c_bus_vmstate,
>                               AspeedI2CBus),
> @@ -828,6 +830,8 @@ static void aspeed_i2c_realize(DeviceState *dev, Error 
> **errp)
>  static Property aspeed_i2c_properties[] = {
>      DEFINE_PROP_LINK("dram", AspeedI2CState, dram_mr,
>                       TYPE_MEMORY_REGION, MemoryRegion *),
> +    DEFINE_PROP_UINT32("ctrl-global-rsvd", AspeedI2CState, ctrl_global_rsvd,
> +                       0xfffffffe),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.35.3
>