Re: [PATCH 2/6] hw/i2c: Implement NPCM7XX SMBus Module Single Mode

[Peter Maydell]

On Tue, 26 Jan 2021 at 19:32, Hao Wu <wuhaotsh@google.com> wrote:
>
> This commit implements the single-byte mode of the SMBus.
>
> Each Nuvoton SoC has 16 System Management Bus (SMBus). These buses
> compliant with SMBus and I2C protocol.
>
> This patch implements the single-byte mode of the SMBus. In this mode,
> the user sends or receives a byte each time. The SMBus device transmits
> it to the underlying i2c device and sends an interrupt back to the QEMU
> guest.
>
> Reviewed-by: Doug Evans<dje@google.com>
> Reviewed-by: Tyrong Ting<kfting@nuvoton.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  docs/system/arm/nuvoton.rst    |   2 +-
>  hw/arm/npcm7xx.c               |  68 ++-
>  hw/i2c/meson.build             |   1 +
>  hw/i2c/npcm7xx_smbus.c         | 766 +++++++++++++++++++++++++++++++++
>  hw/i2c/trace-events            |  11 +
>  include/hw/arm/npcm7xx.h       |   2 +
>  include/hw/i2c/npcm7xx_smbus.h |  88 ++++
>  7 files changed, 921 insertions(+), 17 deletions(-)
>  create mode 100644 hw/i2c/npcm7xx_smbus.c
>  create mode 100644 include/hw/i2c/npcm7xx_smbus.h
>
> diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
> index a1786342e2..34fc799b2d 100644
> --- a/docs/system/arm/nuvoton.rst
> +++ b/docs/system/arm/nuvoton.rst
> @@ -43,6 +43,7 @@ Supported devices
>   * GPIO controller
>   * Analog to Digital Converter (ADC)
>   * Pulse Width Modulation (PWM)
> + * SMBus controller (SMBF)
>
>  Missing devices
>  ---------------
> @@ -58,7 +59,6 @@ Missing devices
>
>   * Ethernet controllers (GMAC and EMC)
>   * USB device (USBD)
> - * SMBus controller (SMBF)
>   * Peripheral SPI controller (PSPI)
>   * SD/MMC host
>   * PECI interface
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index d1fe9bd1df..8f596ffd69 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -104,6 +104,22 @@ enum NPCM7xxInterrupt {
>      NPCM7XX_OHCI_IRQ            = 62,
>      NPCM7XX_PWM0_IRQ            = 93,   /* PWM module 0 */
>      NPCM7XX_PWM1_IRQ,                   /* PWM module 1 */
> +    NPCM7XX_SMBUS0_IRQ          = 64,
> +    NPCM7XX_SMBUS1_IRQ,
> +    NPCM7XX_SMBUS2_IRQ,
> +    NPCM7XX_SMBUS3_IRQ,
> +    NPCM7XX_SMBUS4_IRQ,
> +    NPCM7XX_SMBUS5_IRQ,
> +    NPCM7XX_SMBUS6_IRQ,
> +    NPCM7XX_SMBUS7_IRQ,
> +    NPCM7XX_SMBUS8_IRQ,
> +    NPCM7XX_SMBUS9_IRQ,
> +    NPCM7XX_SMBUS10_IRQ,
> +    NPCM7XX_SMBUS11_IRQ,
> +    NPCM7XX_SMBUS12_IRQ,
> +    NPCM7XX_SMBUS13_IRQ,
> +    NPCM7XX_SMBUS14_IRQ,
> +    NPCM7XX_SMBUS15_IRQ,

Would be nicer to put these in their correct place in numerical
order, ie above the PWM IRQs rather than below them. (The list
is otherwise already in numerical order.)

>      NPCM7XX_GPIO0_IRQ           = 116,
>      NPCM7XX_GPIO1_IRQ,
>      NPCM7XX_GPIO2_IRQ,
> @@ -152,6 +168,26 @@ static const hwaddr npcm7xx_pwm_addr[] = {
>      0xf0104000,

> +static const VMStateDescription vmstate_npcm7xx_smbus = {
> +    .name = "npcm7xx-smbus",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST(),
> +    },

Looks like you forgot to fill in the fields in the vmstate :-)

> +};
> +

thanks
-- PMM