Alistair suggested moving to Fifo8, which I think is
a good improvement.
Broken out for individual review, but IMO we should
squash before merge since it changes VMState format.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
hw/i2c/designware_i2c.c | 37 ++++++++++++++++++++-------------
include/hw/i2c/designware_i2c.h | 7 +++----
2 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/hw/i2c/designware_i2c.c b/hw/i2c/designware_i2c.c
index 1e5505f571..83d0968580 100644
--- a/hw/i2c/designware_i2c.c
+++ b/hw/i2c/designware_i2c.c
@@ -208,10 +208,8 @@ static void dw_i2c_update_irq(DesignWareI2CState *s)
qemu_set_irq(s->irq, level);
}
-static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
+static uint8_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
{
- uint32_t value = s->rx_fifo[s->rx_cur];
-
if (s->status != DW_I2C_STATUS_RECEIVING) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Attempted to read from RX fifo when not in receive "
@@ -223,22 +221,21 @@ static uint32_t dw_i2c_read_ic_data_cmd(DesignWareI2CState *s)
return 0;
}
- s->rx_cur = (s->rx_cur + 1) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+ g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
- if (s->ic_rxflr > 0) {
- s->ic_rxflr--;
- } else {
+ if (fifo8_is_empty(&s->rx_fifo)) {
s->ic_raw_intr_stat |= DW_IC_INTR_RX_UNDER;
dw_i2c_update_irq(s);
return 0;
}
+ s->ic_rxflr--;
if (s->ic_rxflr <= s->ic_rx_tl) {
s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_FULL;
dw_i2c_update_irq(s);
}
- return value;
+ return fifo8_pop(&s->rx_fifo);
}
static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size)
@@ -445,6 +442,7 @@ static void dw_i2c_reset_to_idle(DesignWareI2CState *s)
s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER;
s->ic_raw_intr_stat &= ~DW_IC_INTR_RX_OVER;
s->ic_rxflr = 0;
+ fifo8_reset(&s->rx_fifo);
s->ic_status &= ~DW_IC_STATUS_ACTIVITY;
s->status = DW_I2C_STATUS_IDLE;
dw_i2c_update_irq(s);
@@ -509,10 +507,10 @@ static void dw_i2c_write_ic_data_cmd(DesignWareI2CState *s, uint32_t value)
/* Receive data */
if (recv) {
- uint8_t pos = (s->rx_cur + s->ic_rxflr) % DESIGNWARE_I2C_RX_FIFO_SIZE;
+ g_assert(s->ic_rxflr == fifo8_num_used(&s->rx_fifo));
- if (s->ic_rxflr < DESIGNWARE_I2C_RX_FIFO_SIZE) {
- s->rx_fifo[pos] = i2c_recv(s->bus);
+ if (!fifo8_is_full(&s->rx_fifo)) {
+ fifo8_push(&s->rx_fifo, i2c_recv(s->bus));
s->ic_rxflr++;
} else {
s->ic_raw_intr_stat |= DW_IC_INTR_RX_OVER;
@@ -738,7 +736,8 @@ static void designware_i2c_enter_reset(Object *obj, ResetType type)
s->ic_comp_version = DW_IC_COMP_VERSION_INIT_VAL;
s->ic_comp_type = DW_IC_COMP_TYPE_INIT_VAL;
- s->rx_cur = 0;
+ fifo8_reset(&s->rx_fifo);
+
s->status = DW_I2C_STATUS_IDLE;
}
@@ -777,10 +776,8 @@ static const VMStateDescription vmstate_designware_i2c = {
VMSTATE_UINT32(ic_comp_param_1, DesignWareI2CState),
VMSTATE_UINT32(ic_comp_version, DesignWareI2CState),
VMSTATE_UINT32(ic_comp_type, DesignWareI2CState),
+ VMSTATE_FIFO8(rx_fifo, DesignWareI2CState),
VMSTATE_UINT32(status, DesignWareI2CState),
- VMSTATE_UINT8_ARRAY(rx_fifo, DesignWareI2CState,
- DESIGNWARE_I2C_RX_FIFO_SIZE),
- VMSTATE_UINT8(rx_cur, DesignWareI2CState),
VMSTATE_END_OF_LIST(),
},
};
@@ -790,6 +787,8 @@ static void designware_i2c_smbus_init(Object *obj)
DesignWareI2CState *s = DESIGNWARE_I2C(obj);
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+ fifo8_create(&s->rx_fifo, DESIGNWARE_I2C_RX_FIFO_SIZE);
+
sysbus_init_irq(sbd, &s->irq);
memory_region_init_io(&s->iomem, obj, &designware_i2c_ops, s,
@@ -799,6 +798,13 @@ static void designware_i2c_smbus_init(Object *obj)
s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
}
+static void designware_i2c_finalize(Object *obj)
+{
+ DesignWareI2CState *s = DESIGNWARE_I2C(obj);
+
+ fifo8_destroy(&s->rx_fifo);
+}
+
static void designware_i2c_class_init(ObjectClass *klass, const void *data)
{
ResettableClass *rc = RESETTABLE_CLASS(klass);
@@ -819,6 +825,7 @@ static const TypeInfo designware_i2c_types[] = {
.instance_size = sizeof(DesignWareI2CState),
.class_init = designware_i2c_class_init,
.instance_init = designware_i2c_smbus_init,
+ .instance_finalize = designware_i2c_finalize,
},
};
DEFINE_TYPES(designware_i2c_types);
diff --git a/include/hw/i2c/designware_i2c.h b/include/hw/i2c/designware_i2c.h
index 71d74c9141..affaf983a2 100644
--- a/include/hw/i2c/designware_i2c.h
+++ b/include/hw/i2c/designware_i2c.h
@@ -8,6 +8,7 @@
#ifndef DESIGNWARE_I2C_H
#define DESIGNWARE_I2C_H
+#include "qemu/fifo8.h"
#include "hw/i2c/i2c.h"
#include "hw/core/irq.h"
#include "hw/core/sysbus.h"
@@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
* @ic_comp_version: I2C component version register
* @ic_comp_type: I2C component type register
* @rx_fifo: The FIFO buffer for receiving in FIFO mode.
- * @rx_cur: The current position of rx_fifo.
- * @status: The current status of the SMBus.
*/
typedef struct DesignWareI2CState {
SysBusDevice parent_obj;
@@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
uint32_t ic_comp_version;
uint32_t ic_comp_type;
- uint8_t rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
- uint8_t rx_cur;
+ /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
+ Fifo8 rx_fifo;
DesignWareI2CStatus status;
} DesignWareI2CState;
--
2.53.0On 7/5/26 14:05, Nicholas Piggin wrote:
> Alistair suggested moving to Fifo8, which I think is
> a good improvement.
>
> Broken out for individual review, but IMO we should
> squash before merge since it changes VMState format.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> hw/i2c/designware_i2c.c | 37 ++++++++++++++++++++-------------
> include/hw/i2c/designware_i2c.h | 7 +++----
> 2 files changed, 25 insertions(+), 19 deletions(-)
> @@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
> * @ic_comp_version: I2C component version register
> * @ic_comp_type: I2C component type register
> * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
> - * @rx_cur: The current position of rx_fifo.
> - * @status: The current status of the SMBus.
> */
> typedef struct DesignWareI2CState {
> SysBusDevice parent_obj;
> @@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
> uint32_t ic_comp_version;
> uint32_t ic_comp_type;
>
> - uint8_t rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
> - uint8_t rx_cur;
> + /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
Why not remove ic_rxflr then?
> + Fifo8 rx_fifo;
>
> DesignWareI2CStatus status;
> } DesignWareI2CState;
On Mon, May 11, 2026 at 12:18:41PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/5/26 14:05, Nicholas Piggin wrote:
> > Alistair suggested moving to Fifo8, which I think is
> > a good improvement.
> >
> > Broken out for individual review, but IMO we should
> > squash before merge since it changes VMState format.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > hw/i2c/designware_i2c.c | 37 ++++++++++++++++++++-------------
> > include/hw/i2c/designware_i2c.h | 7 +++----
> > 2 files changed, 25 insertions(+), 19 deletions(-)
>
>
> > @@ -53,8 +54,6 @@ typedef enum DesignWareI2CStatus {
> > * @ic_comp_version: I2C component version register
> > * @ic_comp_type: I2C component type register
> > * @rx_fifo: The FIFO buffer for receiving in FIFO mode.
> > - * @rx_cur: The current position of rx_fifo.
> > - * @status: The current status of the SMBus.
> > */
> > typedef struct DesignWareI2CState {
> > SysBusDevice parent_obj;
> > @@ -88,8 +87,8 @@ typedef struct DesignWareI2CState {
> > uint32_t ic_comp_version;
> > uint32_t ic_comp_type;
> > - uint8_t rx_fifo[DESIGNWARE_I2C_RX_FIFO_SIZE];
> > - uint8_t rx_cur;
> > + /* fifo8_num_used(rx_fifo) should always equal ic_rxflr */
>
> Why not remove ic_rxflr then?
I did consider that... but the next patch to rework the registers
adds storage for them all as a block. We could still just ignore
the storage and use fifo8_num_used() instead but I was vaguely
worried about complexity of vmstate compat if we change things, but
not sure if that is a rational concern. In the end the code is
quite simple and only a couple of places where this is adjusted so
I kept it this way.
Thanks,
Nick
© 2016 - 2026 Red Hat, Inc.