Re: [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> Emulate the i2c part of SM501 which is used to access the EDID info
> from a monitor.
> 
> The vmstate structure is changed and its version is increased but
> SM501 is only used on SH and PPC sam460ex machines that don't support
> cross-version migration.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v4: Updated commit message

This patch breaks compile on SH - sm501 now needs various i2c symbols
that aren't configured into the SH builds.

As a rule, it's always wise to try compiling with a bare ./configure
before you post to make sure you didn't break some platform you
weren't thinking about.  Doing a "make check" like that is even better.

> 
>  default-configs/ppc-softmmu.mak    |   1 +
>  default-configs/ppcemb-softmmu.mak |   1 +
>  default-configs/sh4-softmmu.mak    |   1 +
>  default-configs/sh4eb-softmmu.mak  |   1 +
>  hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 136 insertions(+), 4 deletions(-)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index b8b0526..e131e24 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -24,6 +24,7 @@ CONFIG_ETSEC=y
>  # For Sam460ex
>  CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index 37af193..ac44f15 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -17,6 +17,7 @@ CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
> index 546d855..72d8fca 100644
> --- a/default-configs/sh4-softmmu.mak
> +++ b/default-configs/sh4-softmmu.mak
> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
>  CONFIG_SH4=y
>  CONFIG_IDE_MMIO=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_ISA_TESTDEV=y
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
> index 2d3fd49..c686637 100644
> --- a/default-configs/sh4eb-softmmu.mak
> +++ b/default-configs/sh4eb-softmmu.mak
> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
>  CONFIG_SH4=y
>  CONFIG_IDE_MMIO=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_ISA_TESTDEV=y
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 8206ae8..a076248 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -26,6 +26,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "qemu/log.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "hw/hw.h"
> @@ -34,6 +35,8 @@
>  #include "hw/devices.h"
>  #include "hw/sysbus.h"
>  #include "hw/pci/pci.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/i2c-ddc.h"
>  #include "qemu/range.h"
>  #include "ui/pixel_ops.h"
>  
> @@ -471,10 +474,12 @@ typedef struct SM501State {
>      MemoryRegion local_mem_region;
>      MemoryRegion mmio_region;
>      MemoryRegion system_config_region;
> +    MemoryRegion i2c_region;
>      MemoryRegion disp_ctrl_region;
>      MemoryRegion twoD_engine_region;
>      uint32_t last_width;
>      uint32_t last_height;
> +    I2CBus *i2c_bus;
>  
>      /* mmio registers */
>      uint32_t system_control;
> @@ -487,6 +492,11 @@ typedef struct SM501State {
>      uint32_t misc_timing;
>      uint32_t power_mode_control;
>  
> +    uint8_t i2c_byte_count;
> +    uint8_t i2c_status;
> +    uint8_t i2c_addr;
> +    uint8_t i2c_data[16];
> +
>      uint32_t uart0_ier;
>      uint32_t uart0_lcr;
>      uint32_t uart0_mcr;
> @@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    SM501State *s = (SM501State *)opaque;
> +    uint8_t ret = 0;
> +
> +    switch (addr) {
> +    case SM501_I2C_BYTE_COUNT:
> +        ret = s->i2c_byte_count;
> +        break;
> +    case SM501_I2C_STATUS:
> +        ret = s->i2c_status;
> +        break;
> +    case SM501_I2C_SLAVE_ADDRESS:
> +        ret = s->i2c_addr;
> +        break;
> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> +        ret = s->i2c_data[addr - SM501_I2C_DATA];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
> +                      " addr=0x%" HWADDR_PRIx "\n", addr);
> +    }
> +
> +    SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
> +                  addr, ret);
> +    return ret;
> +}
> +
> +static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
> +                            unsigned size)
> +{
> +    SM501State *s = (SM501State *)opaque;
> +    SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
> +                  " val=%" PRIx64 "\n", addr, value);
> +
> +    switch (addr) {
> +    case SM501_I2C_BYTE_COUNT:
> +        s->i2c_byte_count = value & 0xf;
> +        break;
> +    case SM501_I2C_CONTROL:
> +        if (value & 1) {
> +            if (value & 4) {
> +                int res = i2c_start_transfer(s->i2c_bus,
> +                                             s->i2c_addr >> 1,
> +                                             s->i2c_addr & 1);
> +                s->i2c_status |= (res ? 1 << 2 : 0);
> +                if (!res) {
> +                    int i;
> +                    SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
> +                                  s->i2c_byte_count + 1, s->i2c_addr >> 1);
> +                    for (i = 0; i <= s->i2c_byte_count; i++) {
> +                        res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
> +                                            !(s->i2c_addr & 1));
> +                        if (res) {
> +                            SM501_DPRINTF("sm501 i2c : transfer failed"
> +                                          " i=%d, res=%d\n", i, res);
> +                            s->i2c_status |= (res ? 1 << 2 : 0);
> +                            return;
> +                        }
> +                    }
> +                    if (i) {
> +                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
> +                        s->i2c_status = 8;
> +                    }
> +                }
> +            } else {
> +                SM501_DPRINTF("sm501 i2c : end transfer\n");
> +                i2c_end_transfer(s->i2c_bus);
> +                s->i2c_status &= ~4;
> +            }
> +        }
> +        break;
> +    case SM501_I2C_RESET:
> +            s->i2c_status &= ~4;
> +        break;
> +    case SM501_I2C_SLAVE_ADDRESS:
> +        s->i2c_addr = value & 0xff;
> +        break;
> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> +        s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
> +                      "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
> +    }
> +}
> +
> +static const MemoryRegionOps sm501_i2c_ops = {
> +    .read = sm501_i2c_read,
> +    .write = sm501_i2c_write,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
>  {
>      SM501State *s = (SM501State *)opaque;
> @@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
>      s->irq_mask = 0;
>      s->misc_timing = 0;
>      s->power_mode_control = 0;
> +    s->i2c_byte_count = 0;
> +    s->i2c_status = 0;
> +    s->i2c_addr = 0;
> +    memset(s->i2c_data, 0, 16);
>      s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>      s->dc_video_control = 0;
>      s->dc_crt_control = 0x00010000;
> @@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>      memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>  
> +    /* i2c */
> +    s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
> +    I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
> +    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +
>      /* mmio */
>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>      memory_region_init_io(&s->system_config_region, OBJECT(dev),
> @@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>                            "sm501-system-config", 0x6c);
>      memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
>                                  &s->system_config_region);
> +    memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
> +                          "sm501-i2c", 0x14);
> +    memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
>      memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
>                            &sm501_disp_ctrl_ops, s,
>                            "sm501-disp-ctrl", 0x1000);
> @@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
>          VMSTATE_UINT32(twoD_destination_base, SM501State),
>          VMSTATE_UINT32(twoD_alpha, SM501State),
>          VMSTATE_UINT32(twoD_wrap, SM501State),
> +        /* Added in version 2 */
> +        VMSTATE_UINT8(i2c_byte_count, SM501State),
> +        VMSTATE_UINT8(i2c_status, SM501State),
> +        VMSTATE_UINT8(i2c_addr, SM501State),
> +        VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
>          VMSTATE_END_OF_LIST()
>       }
>  };
> @@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
>  
>  static const VMStateDescription vmstate_sm501_sysbus = {
>      .name = TYPE_SYSBUS_SM501,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_STRUCT(state, SM501SysBusState, 1,
>                         vmstate_sm501_state, SM501State),
> @@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
>  
>  static const VMStateDescription vmstate_sm501_pci = {
>      .name = TYPE_PCI_SM501,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
>          VMSTATE_STRUCT(state, SM501PCIState, 1,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]