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

[Peter Maydell]

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.

> @@ -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