Re: [PATCH v2 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode

[Hao Wu <wuhaotsh@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]

On Mon, Feb 8, 2021 at 8:59 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Fri, 29 Jan 2021 at 01:04, Hao Wu <wuhaotsh@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<dje@google.com>
> > Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> > Reviewed-by: Corey Minyard <cminyard@mvista.com>
> > Ack-by: Corey Minyard <cminyard@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
>

[-- Attachment #2: Type: text/html, Size: 4596 bytes --]