Hi Zoltan,
On 06/21/2018 05:08 AM, 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 <bala...@eik.bme.hu>
> ---
> default-configs/ppc-softmmu.mak | 1 +
> default-configs/ppcemb-softmmu.mak | 1 +
> default-configs/sh4-softmmu.mak | 2 +
> default-configs/sh4eb-softmmu.mak | 2 +
> hw/display/sm501.c | 136
> +++++++++++++++++++++++++++++++++++--
> 5 files changed, 138 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..caeccd5 100644
> --- a/default-configs/sh4-softmmu.mak
> +++ b/default-configs/sh4-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
> CONFIG_SH4=y
> CONFIG_IDE_MMIO=y
> CONFIG_SM501=y
> +CONFIG_I2C=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..53b9cd7 100644
> --- a/default-configs/sh4eb-softmmu.mak
> +++ b/default-configs/sh4eb-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
> CONFIG_SH4=y
> CONFIG_IDE_MMIO=y
> CONFIG_SM501=y
> +CONFIG_I2C=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 ca0840f..0625cf5 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];
Using:
uint8_t i2c_regs[0x20];
Would ease vmstate management and further refactors.
> +
> 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,
> };
>
To help code review you can use the "hw/registerfields.h" API:
FIELD(CTRL, ENABLE, 0, 1)
FIELD(CTRL, BUS_SPEED, 1, 1)
FIELD(CTRL, START, 2, 1)
FIELD(CTRL, INT_ENABLE, 4, 1)
FIELD(CTRL, INT_ACK, 5, 1)
FIELD(CTRL, REPEAT_START, 6, 1)
FIELD(STATUS, BUSY, 0, 1)
FIELD(STATUS, ACK, 1, 1)
FIELD(STATUS, ERROR, 2, 1)
FIELD(STATUS, COMPLETE, 3, 1)
FIELD(RESET, ERROR, 2, 1)
> +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);
At some point we should replace the SM501_DPRINTF() macro by trace events.
> + 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) {
value & R_CTRL_ENABLE_MASK
> + if (value & 4) {
value & R_CTRL_START_MASK
> + int res = i2c_start_transfer(s->i2c_bus,
> + s->i2c_addr >> 1,
> + s->i2c_addr & 1);
> + s->i2c_status |= (res ? 1 << 2 : 0);
Hmm if you previously had the ERROR bit set, you don't clear it.
s->i2c_status = FIELD_DP32(s->i2c_status, STATUS,
ERROR, res);
> + 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);
Ditto.
This could be a goto transfer_error at end of func.
> + return;
> + }
> + }
> + if (i) {
> + SM501_DPRINTF("sm501 i2c : transferred %d bytes\n",
> i);
> + s->i2c_status = 8;
s->i2c_status = R_STATUS_COMPLETE_MASK;
> + }
> + }
> + } else {
> + SM501_DPRINTF("sm501 i2c : end transfer\n");
> + i2c_end_transfer(s->i2c_bus);
> + s->i2c_status &= ~4;
s->i2c_status &= ~R_STATUS_ERROR_MASK;
> + }
> + }
> + break;
> + case SM501_I2C_RESET:
> + s->i2c_status &= ~4;
I understand "0 clear" as:
if (FIELD_EX32(value, RESET, ERROR) == 0) {
s->i2c_status &= ~R_STATUS_ERROR_MASK;
}
also:
value &= ~R_RESET_ERROR_MASK;
if (value) {
qemu_log_mask(LOG_GUEST_ERROR,
"sm501_i2c_write(RESET) bad value...",
value);
}
> + break;
> + case SM501_I2C_SLAVE_ADDRESS:
> + s->i2c_addr = value & 0xff;
Implicit mask, fine.
> + break;
> + case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> + s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
Ditto.
> + 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,
Per "Table 9-1: GPIO and I2C Register Summary" valid.max_access_size is
1 (8-bit registers) for registers 0, 1, 2, 3;
and 4 for registers 4 to 15.
To keep things simple you could add in sm501_i2c_read/sm501_i2c_write
prologue:
if (addr < 4 && size > 1) {
qemu_log_mask(LOG_GUEST_ERROR, ...
return ...
}
> + },
> + .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");
/* ddc */
> + I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
> + i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
I think this is board/screen related but that's way easier this way.
> +
> /* 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);
Per "Figure 9-2: GPIO / I2C Master Register Space" the region size is 0x20.
> + 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,
>
Fixing "0 clear" and region size to 0x20:
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Regards,
Phil.
