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

[Philippe Mathieu-Daudé]

On 06/20/2018 04:17 AM, David Gibson wrote:
> 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 <address@hidden>
>> ---
>> 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.

Or if you lack horsepower and have time (~2h), push your branch to a
github repo with Travis CI enabled, and Travis will build/check testing
all for you.

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

signature.asc
Description: OpenPGP digital signature