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

[Cédric Le Goater]

Hello Joe,

On 6/7/22 02:05, Joel Stanley wrote:
> 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.


Could you tell us more about the 0xfffc3e00 value. It doesn't match
any documents I have access to. If it is for a specific board, then
it should be added to QEMU. We can keep the property to begin with,
if that helps

Thanks,

C.


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