Re: [PATCH v2 1/2] ppc/pnv: Add an I2C controller model

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Glenn, Cédric,

On 12/10/23 22:08, Glenn Miles wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> The more recent IBM power processors have an embedded I2C
> controller that is accessible by software via the XSCOM
> address space.
> 
> Each instance of the I2C controller is capable of controlling
> multiple I2C buses (one at a time).  Prior to beginning a
> transaction on an I2C bus, the bus must be selected by writing
> the port number associated with the bus into the PORT_NUM
> field of the MODE register.  Once an I2C bus is selected,
> the status of the bus can be determined by reading the
> Status and Extended Status registers.
> 
> I2C bus transactions can be started by writing a command to
> the Command register and reading/writing data from/to the
> FIFO register.
> 
> Not supported :
> 
>   . 10 bit I2C addresses
>   . Multimaster
>   . Slave
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> [milesg: Split wiring to powernv9 into its own commit]
> [milesg: Added more detail to commit message]
> [milesg: Added SPDX Licensed Identifier to new files]
> [milesg: updated copyright dates]
> [milesg: Added use of g_autofree]
> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> ---
> 
> Changes in v2:
>      - Updated copyright dates
>      - Removed copyright paragraph (replaced by SPDX-License-Identifier)
>      - Added use of g_autofree
> 
>   hw/ppc/meson.build         |   1 +
>   hw/ppc/pnv_i2c.c           | 673 +++++++++++++++++++++++++++++++++++++
>   include/hw/ppc/pnv_i2c.h   |  38 +++
>   include/hw/ppc/pnv_xscom.h |   3 +
>   4 files changed, 715 insertions(+)
>   create mode 100644 hw/ppc/pnv_i2c.c
>   create mode 100644 include/hw/ppc/pnv_i2c.h


> +/* I2C mode register */
> +#define I2C_MODE_REG                    0x6
> +#define I2C_MODE_BIT_RATE_DIV           PPC_BITMASK(0, 15)
> +#define I2C_MODE_PORT_NUM               PPC_BITMASK(16, 21)
> +#define I2C_MODE_ENHANCED               PPC_BIT(28)
> +#define I2C_MODE_DIAGNOSTIC             PPC_BIT(29)
> +#define I2C_MODE_PACING_ALLOW           PPC_BIT(30)
> +#define I2C_MODE_WRAP                   PPC_BIT(31)


> +static I2CBus *pnv_i2c_get_bus(PnvI2C *i2c)
> +{
> +    uint8_t port = GETFIELD(I2C_MODE_PORT_NUM, i2c->regs[I2C_MODE_REG]);
> +
> +    if (port >= i2c->num_busses) {

Can we sanitize in pnv_i2c_xscom_write() instead ...?

> +        qemu_log_mask(LOG_GUEST_ERROR, "I2C: invalid bus number %d/%d\n", port,
> +                      i2c->num_busses);
> +        return NULL;
> +    }
> +    return i2c->busses[port];
> +}


> +static void pnv_i2c_xscom_write(void *opaque, hwaddr addr,
> +                                uint64_t val, unsigned size)
> +{
> +    PnvI2C *i2c = PNV_I2C(opaque);
> +    uint32_t offset = addr >> 3;
> +
> +    switch (offset) {
> +    case I2C_MODE_REG:

... here?

> +        i2c->regs[offset] = val;
> +        if (i2c_bus_busy(pnv_i2c_get_bus(i2c))) {
> +            i2c->regs[I2C_STAT_REG] |= I2C_STAT_INVALID_CMD;
> +            qemu_log_mask(LOG_GUEST_ERROR, "I2C: command in progress\n");
> +        }
> +        break;

[...]

Regards,

Phil.