On Mon, Feb 8, 2021 at 8:59 AM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Fri, 29 Jan 2021 at 01:04, Hao Wu <wuhao...@google.com> wrote:
> >
> > This patch implements the FIFO mode of the SMBus module. In FIFO, the
> > user transmits or receives at most 16 bytes at a time. The FIFO mode
> > allows the module to transmit large amount of data faster than single
> > byte mode.
> >
> > Reviewed-by: Doug Evans<d...@google.com>
> > Reviewed-by: Tyrong Ting<kft...@nuvoton.com>
> > Signed-off-by: Hao Wu <wuhao...@google.com>
> > Reviewed-by: Corey Minyard <cminy...@mvista.com>
> > Ack-by: Corey Minyard <cminy...@mvista.com>
> > ---
> > hw/i2c/npcm7xx_smbus.c | 342 +++++++++++++++++++++++++++++--
> > hw/i2c/trace-events | 1 +
> > include/hw/i2c/npcm7xx_smbus.h | 25 +++
> > tests/qtest/npcm7xx_smbus-test.c | 149 +++++++++++++-
> > 4 files changed, 501 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
> > index c72b6e446f..be3253d251 100644
> > --- a/hw/i2c/npcm7xx_smbus.c
> > +++ b/hw/i2c/npcm7xx_smbus.c
> > @@ -27,7 +27,7 @@
> > #include "trace.h"
> >
> > #define NPCM7XX_SMBUS_VERSION 1
> > -#define NPCM7XX_SMBUS_FIFO_EN 0
> > +#define NPCM7XX_SMBUS_FIFO_EN 1
>
> Why has this define changed ?
>
> > #define NPCM7XX_SMBUS_ENABLED(s) ((s)->ctl2 & NPCM7XX_SMBCTL2_ENABLE)
> > +#define NPCM7XX_SMBUS_FIFO_ENABLED(s) (NPCM7XX_SMBUS_FIFO_EN && \
> > + (s)->fif_ctl & NPCM7XX_SMBFIF_CTL_FIFO_EN)
>
> ...and why are we testing something that's always 1 ?
> Is NPCM7XX_SMBUS_FIFO_EN supposed to be a debug "turn this feature off"
> switch for QEMU developers? If, so it would be helpful to give it a name
> that doesn't look like it's defining a bit value for the hardware
> and adding a comment saying what it does.
>
No, it's a specific bit in the NPCM7XX SMBus module's version reg that
indicates whether this module
supports FIFO mode. I'll rename it so it's more clear to the reader.
>
> > @@ -754,6 +1059,17 @@ static const VMStateDescription
> vmstate_npcm7xx_smbus = {
> > VMSTATE_UINT8_ARRAY(addr, NPCM7xxSMBusState,
> NPCM7XX_SMBUS_NR_ADDRS),
> > VMSTATE_UINT8(scllt, NPCM7xxSMBusState),
> > VMSTATE_UINT8(sclht, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(fif_ctl, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(fif_cts, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(fair_per, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(txf_ctl, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(t_out, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(txf_sts, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(rxf_sts, NPCM7xxSMBusState),
> > + VMSTATE_UINT8(rxf_ctl, NPCM7xxSMBusState),
> > + VMSTATE_UINT8_ARRAY(rx_fifo, NPCM7xxSMBusState,
> > + NPCM7XX_SMBUS_FIFO_SIZE),
> > + VMSTATE_UINT8(rx_cur, NPCM7xxSMBusState),
> > VMSTATE_END_OF_LIST(),
> > },
> > };
>
> It's OK to add fields to the vmstate without bumping the version
> number in this special case, because we only just added the device
> a few commits earlier in the series, but it's worth specifically
> saying that in the commit message.
>
> thanks
> -- PMM
>
