| Message ID | 20220606150732.2282041-3-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: Extend ast2600 I2C model with new mode | expand |
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 >
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 >>
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; 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(), };