| Message ID | 20220714182836.89602-3-wuhaotsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc NPCM7XX patches | expand |
On Thu, 14 Jul 2022 at 19:28, Hao Wu <wuhaotsh@google.com> wrote: > > Originally we read in from SMBus when RXF_STS is cleared. However, > the driver clears RXF_STS before setting RXF_CTL, causing the SM bus > module to read incorrect amount of bytes in FIFO mode when the number > of bytes read changed. This patch fixes this issue. > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > Reviewed-by: Titus Rwantare <titusr@google.com> > Acked-by: Corey Minyard <cminyard@mvista.com> > --- > hw/i2c/npcm7xx_smbus.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c > index f18e311556..1435daea94 100644 > --- a/hw/i2c/npcm7xx_smbus.c > +++ b/hw/i2c/npcm7xx_smbus.c > @@ -637,9 +637,6 @@ static void npcm7xx_smbus_write_rxf_sts(NPCM7xxSMBusState *s, uint8_t value) > { > if (value & NPCM7XX_SMBRXF_STS_RX_THST) { > s->rxf_sts &= ~NPCM7XX_SMBRXF_STS_RX_THST; > - if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) { > - npcm7xx_smbus_recv_fifo(s); > - } > } > } > > @@ -651,6 +648,9 @@ static void npcm7xx_smbus_write_rxf_ctl(NPCM7xxSMBusState *s, uint8_t value) > new_ctl = KEEP_OLD_BIT(s->rxf_ctl, new_ctl, NPCM7XX_SMBRXF_CTL_LAST); > } > s->rxf_ctl = new_ctl; > + if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) { > + npcm7xx_smbus_recv_fifo(s); > + } > } I don't know anything about this hardware, but this looks a bit odd. Why should we care what order the driver does the register operations in? Do we really want to read new fifo data regardless of what value the driver writes to RXF_CTL ? Should the logic actually be "if the new device register state is <whatever> then read fifo data", and checked in both places ? thanks -- PMM
diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c index f18e311556..1435daea94 100644 --- a/hw/i2c/npcm7xx_smbus.c +++ b/hw/i2c/npcm7xx_smbus.c @@ -637,9 +637,6 @@ static void npcm7xx_smbus_write_rxf_sts(NPCM7xxSMBusState *s, uint8_t value) { if (value & NPCM7XX_SMBRXF_STS_RX_THST) { s->rxf_sts &= ~NPCM7XX_SMBRXF_STS_RX_THST; - if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) { - npcm7xx_smbus_recv_fifo(s); - } } } @@ -651,6 +648,9 @@ static void npcm7xx_smbus_write_rxf_ctl(NPCM7xxSMBusState *s, uint8_t value) new_ctl = KEEP_OLD_BIT(s->rxf_ctl, new_ctl, NPCM7XX_SMBRXF_CTL_LAST); } s->rxf_ctl = new_ctl; + if (s->status == NPCM7XX_SMBUS_STATUS_RECEIVING) { + npcm7xx_smbus_recv_fifo(s); + } } static uint64_t npcm7xx_smbus_read(void *opaque, hwaddr offset, unsigned size)