[Qemu-devel] [PATCH v2 6/7] sm501: Fix support for non-zero frame buffer start address

[Qemu-devel] [PATCH v2 6/7] sm501: Fix support for non-zero frame buffer start address

[BALATON Zoltan]

Display updates and drawing hardware cursor did not work when frame
buffer address was non-zero. Fix this by taking the frame buffer
address into account in these cases. This fixes screen dragging on
AmigaOS. Based on patch by Sebastian Bauer.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: fixed crash with Linux setting extra bits and log unimplemented case

 hw/display/sm501.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 7404035..6e78f73 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -586,6 +586,11 @@ static uint32_t get_local_mem_size_index(uint32_t size)
     return index;
 }
 
+static ram_addr_t get_fb_addr(SM501State *s, int crt)
+{
+    return (crt ? s->dc_crt_fb_addr : s->dc_panel_fb_addr) & 0x3FFFFF0;
+}
+
 static inline int get_width(SM501State *s, int crt)
 {
     int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
@@ -688,7 +693,8 @@ static inline void hwc_invalidate(SM501State *s, int crt)
     start *= w * bpp;
     end *= w * bpp;
 
-    memory_region_set_dirty(&s->local_mem_region, start, end - start);
+    memory_region_set_dirty(&s->local_mem_region,
+                            get_fb_addr(s, crt) + start, end - start);
 }
 
 static void sm501_2d_operation(SM501State *s)
@@ -1213,6 +1219,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
         break;
     case SM501_DC_PANEL_FB_ADDR:
         s->dc_panel_fb_addr = value & 0x8FFFFFF0;
+        if (value & 0x8000000) {
+            qemu_log_mask(LOG_UNIMP, "Panel external memory not supported\n");
+        }
         break;
     case SM501_DC_PANEL_FB_OFFSET:
         s->dc_panel_fb_offset = value & 0x3FF03FF0;
@@ -1273,6 +1282,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
         break;
     case SM501_DC_CRT_FB_ADDR:
         s->dc_crt_fb_addr = value & 0x8FFFFFF0;
+        if (value & 0x8000000) {
+            qemu_log_mask(LOG_UNIMP, "CRT external memory not supported\n");
+        }
         break;
     case SM501_DC_CRT_FB_OFFSET:
         s->dc_crt_fb_offset = value & 0x3FF03FF0;
@@ -1615,7 +1627,7 @@ static void sm501_update_display(void *opaque)
     draw_hwc_line_func *draw_hwc_line = NULL;
     int full_update = 0;
     int y_start = -1;
-    ram_addr_t offset = 0;
+    ram_addr_t offset;
     uint32_t *palette;
     uint8_t hwc_palette[3 * 3];
     uint8_t *hwc_src = NULL;
@@ -1672,9 +1684,10 @@ static void sm501_update_display(void *opaque)
     }
 
     /* draw each line according to conditions */
+    offset = get_fb_addr(s, crt);
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
-    for (y = 0, offset = 0; y < height; y++, offset += width * src_bpp) {
+    for (y = 0; y < height; y++, offset += width * src_bpp) {
         int update, update_hwc;
 
         /* check if hardware cursor is enabled and we're within its range */
-- 
2.7.6

[Qemu-devel] [PATCH v2 7/7] sm501: Set updated region dirty after 2D operation

[BALATON Zoltan]

Set the changed memory region dirty after performed a 2D operation to
ensure that the screen is updated properly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: fixed to work with non-zero fb_addr

 hw/display/sm501.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 6e78f73..090bc6a 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -715,12 +715,16 @@ static void sm501_2d_operation(SM501State *s)
     /* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
     int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
     int rop = s->twoD_control & 0xFF;
+    uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
+    uint32_t dst_base = s->twoD_destination_base & 0x03FFFFFF;
 
     /* get frame buffer info */
-    uint8_t *src = s->local_mem + (s->twoD_source_base & 0x03FFFFFF);
-    uint8_t *dst = s->local_mem + (s->twoD_destination_base & 0x03FFFFFF);
+    uint8_t *src = s->local_mem + src_base;
+    uint8_t *dst = s->local_mem + dst_base;
     int src_width = s->twoD_pitch & 0x1FFF;
     int dst_width = (s->twoD_pitch >> 16) & 0x1FFF;
+    int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
+    int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
 
     if (addressing != 0x0) {
         printf("%s: only XY addressing is supported.\n", __func__);
@@ -821,6 +825,15 @@ static void sm501_2d_operation(SM501State *s)
         abort();
         break;
     }
+
+    if (dst_base >= get_fb_addr(s, crt) &&
+        dst_base <= get_fb_addr(s, crt) + fb_len) {
+        int dst_len = MIN(fb_len, ((dst_y + operation_height - 1) * dst_width +
+                           dst_x + operation_width) * (1 << format_flags));
+        if (dst_len) {
+            memory_region_set_dirty(&s->local_mem_region, dst_base, dst_len);
+        }
+    }
 }
 
 static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
-- 
2.7.6

[Qemu-devel] [PATCH v2 4/7] sm501: Implement negated destination raster operation mode

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Add support for the negated destination operation mode. This is used e.g.
by AmigaOS for the INVERSEVID drawing mode. With this change, the cursor
in the shell and non-immediate window adjustment are working now.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 8522042..08631d5 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -705,6 +705,8 @@ static void sm501_2d_operation(SM501State *s)
     uint32_t color = s->twoD_foreground;
     int format_flags = (s->twoD_stretch >> 20) & 0x3;
     int addressing = (s->twoD_stretch >> 16) & 0xF;
+    int rop_mode = (s->twoD_control >> 15) & 0x1; /* 1 for rop2, else rop3 */
+    int rop = s->twoD_control & 0xFF;
 
     /* get frame buffer info */
     uint8_t *src = s->local_mem + (s->twoD_source_base & 0x03FFFFFF);
@@ -729,6 +731,8 @@ static void sm501_2d_operation(SM501State *s)
         int y, x, index_d, index_s;                                           \
         for (y = 0; y < operation_height; y++) {                              \
             for (x = 0; x < operation_width; x++) {                           \
+                _pixel_type val;                                              \
+                                                                              \
                 if (rtl) {                                                    \
                     index_s = ((src_y - y) * src_width + src_x - x) * _bpp;   \
                     index_d = ((dst_y - y) * dst_width + dst_x - x) * _bpp;   \
@@ -736,7 +740,13 @@ static void sm501_2d_operation(SM501State *s)
                     index_s = ((src_y + y) * src_width + src_x + x) * _bpp;   \
                     index_d = ((dst_y + y) * dst_width + dst_x + x) * _bpp;   \
                 }                                                             \
-                *(_pixel_type *)&dst[index_d] = *(_pixel_type *)&src[index_s];\
+                if (rop_mode == 1 && rop == 5) {                              \
+                    /* Invert dest */                                         \
+                    val = ~*(_pixel_type *)&dst[index_d];                     \
+                } else {                                                      \
+                    val = *(_pixel_type *)&src[index_s];                      \
+                }                                                             \
+                *(_pixel_type *)&dst[index_d] = val;                          \
             }                                                                 \
         }                                                                     \
     }
-- 
2.7.6

[Qemu-devel] [PATCH v2 5/7] sm501: Log unimplemented raster operation modes

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

The sm501 currently implements only a very limited set of raster operation
modes. After this change, unknown raster operation modes are logged so
these can be easily spotted.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 08631d5..7404035 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -706,6 +706,8 @@ static void sm501_2d_operation(SM501State *s)
     int format_flags = (s->twoD_stretch >> 20) & 0x3;
     int addressing = (s->twoD_stretch >> 16) & 0xF;
     int rop_mode = (s->twoD_control >> 15) & 0x1; /* 1 for rop2, else rop3 */
+    /* 1 if rop2 source is the pattern, otherwise the source is the bitmap */
+    int rop2_source_is_pattern = (s->twoD_control >> 14) & 0x1;
     int rop = s->twoD_control & 0xFF;
 
     /* get frame buffer info */
@@ -719,6 +721,27 @@ static void sm501_2d_operation(SM501State *s)
         abort();
     }
 
+    if (rop_mode == 0) {
+        if (rop != 0xcc) {
+            /* Anything other than plain copies are not supported */
+            qemu_log_mask(LOG_UNIMP, "sm501: rop3 mode with rop %x is not "
+                          "supported.\n", rop);
+        }
+    } else {
+        if (rop2_source_is_pattern && rop != 0x5) {
+            /* For pattern source, we support only inverse dest */
+            qemu_log_mask(LOG_UNIMP, "sm501: rop2 source being the pattern and "
+                          "rop %x is not supported.\n", rop);
+        } else {
+            if (rop != 0x5 && rop != 0xc) {
+                /* Anything other than plain copies or inverse dest is not
+                 * supported */
+                qemu_log_mask(LOG_UNIMP, "sm501: rop mode %x is not "
+                              "supported.\n", rop);
+            }
+        }
+    }
+
     if ((s->twoD_source_base & 0x08000000) ||
         (s->twoD_destination_base & 0x08000000)) {
         printf("%s: only local memory is supported.\n", __func__);
-- 
2.7.6

[Qemu-devel] [PATCH v2 3/7] sm501: Use values from the pitch register for 2D operations

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Before, crt_h_total was used for src_width and dst_width. This is a
property of the current display setting and not relevant for the 2D
operation that also can be done off-screen. The pitch register's purpose
is to describe line pitch relevant of the 2D operation.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 2fbb10e..8522042 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -709,8 +709,8 @@ static void sm501_2d_operation(SM501State *s)
     /* get frame buffer info */
     uint8_t *src = s->local_mem + (s->twoD_source_base & 0x03FFFFFF);
     uint8_t *dst = s->local_mem + (s->twoD_destination_base & 0x03FFFFFF);
-    int src_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
-    int dst_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
+    int src_width = s->twoD_pitch & 0x1FFF;
+    int dst_width = (s->twoD_pitch >> 16) & 0x1FFF;
 
     if (addressing != 0x0) {
         printf("%s: only XY addressing is supported.\n", __func__);
-- 
2.7.6

[Qemu-devel] [PATCH v2 2/7] sm501: Perform a full update after palette change

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Changing the palette of a color index has as an immediate effect on
all pixels with the corresponding index on real hardware. Performing a
full update after a palette change is a simple way to emulate this
effect.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: change type to bool

 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 273495e..2fbb10e 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -487,6 +487,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    bool do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1042,6 +1043,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = true;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1630,6 +1632,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = false;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
-- 
2.7.6

[Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[BALATON Zoltan]

Version 2 of the sm501 changes with fixes that are needed to get
AmigaOS 4.1FE to boot and able to produce graphics.

The strange blue-white colors that first appear are actually correct
and because of AmigaOS selecting a low resolution PAL mode by default
instead of a board specific mode. To work around this one can select
the last option to boot the live CD and then select a better board
specific mode from ScreenMode Prefs. It takes a while for the
ScreenMode window to appear and graphics operations are slow which
could use some improvement but at least it seems to work correctly now
apart from some unimplemented drawing modes for compositing.

If this could be merged before the freeze with the sam460ex patches
and Sebastian's ehci patch then QEMU 3.0 could be the first version
that can boot AmigaOS.

BALATON Zoltan (3):
  sm501: Implement i2c part for reading monitor EDID
  sm501: Fix support for non-zero frame buffer start address
  sm501: Set updated region dirty after 2D operation

Sebastian Bauer (4):
  sm501: Perform a full update after palette change
  sm501: Use values from the pitch register for 2D operations
  sm501: Implement negated destination raster operation mode
  sm501: Log unimplemented raster operation modes

 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                 | 229 +++++++++++++++++++++++++++++++++++--
 5 files changed, 223 insertions(+), 12 deletions(-)

-- 
2.7.6

[Qemu-devel] [PATCH v2 1/7] sm501: Implement i2c part for reading monitor EDID

[BALATON Zoltan]

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>
---
v2:
- added constants for register bits
- fix clearing error bit in reset reg
- set max access size to 1
v1: fixed build with SH

 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                 | 146 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 148 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 8206ae8..273495e 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"
 
@@ -216,6 +219,14 @@
 #define SM501_I2C_SLAVE_ADDRESS         (0x03)
 #define SM501_I2C_DATA                  (0x04)
 
+#define SM501_I2C_CONTROL_START         (1 << 2)
+#define SM501_I2C_CONTROL_ENABLE        (1 << 0)
+
+#define SM501_I2C_STATUS_COMPLETE       (1 << 3)
+#define SM501_I2C_STATUS_ERROR          (1 << 2)
+
+#define SM501_I2C_RESET_ERROR           (1 << 2)
+
 /* SSP base */
 #define SM501_SSP                       (0x020000)
 
@@ -471,10 +482,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 +500,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 +915,109 @@ 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 & SM501_I2C_CONTROL_ENABLE) {
+            if (value & SM501_I2C_CONTROL_START) {
+                int res = i2c_start_transfer(s->i2c_bus,
+                                             s->i2c_addr >> 1,
+                                             s->i2c_addr & 1);
+                s->i2c_status |= (res ? SM501_I2C_STATUS_ERROR : 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 ? SM501_I2C_STATUS_ERROR : 0);
+                            return;
+                        }
+                    }
+                    if (i) {
+                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
+                        s->i2c_status = SM501_I2C_STATUS_COMPLETE;
+                    }
+                }
+            } else {
+                SM501_DPRINTF("sm501 i2c : end transfer\n");
+                i2c_end_transfer(s->i2c_bus);
+                s->i2c_status &= ~SM501_I2C_STATUS_ERROR;
+            }
+        }
+        break;
+    case SM501_I2C_RESET:
+        if ((value & SM501_I2C_RESET_ERROR) == 0) {
+            s->i2c_status &= SM501_I2C_STATUS_ERROR;
+        }
+        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 = 1,
+    },
+    .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 +1698,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 +1740,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 +1752,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 +1838,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 +1908,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 +1981,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,
-- 
2.7.6

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[BALATON Zoltan]

On Tue, 26 Jun 2018, BALATON Zoltan wrote:
> Version 2 of the sm501 changes with fixes that are needed to get
> AmigaOS 4.1FE to boot and able to produce graphics.
>
> The strange blue-white colors that first appear are actually correct
> and because of AmigaOS selecting a low resolution PAL mode by default
> instead of a board specific mode. To work around this one can select
> the last option to boot the live CD and then select a better board
> specific mode from ScreenMode Prefs. It takes a while for the
> ScreenMode window to appear and graphics operations are slow which
> could use some improvement but at least it seems to work correctly now
> apart from some unimplemented drawing modes for compositing.
>
> If this could be merged before the freeze with the sam460ex patches
> and Sebastian's ehci patch then QEMU 3.0 could be the first version
> that can boot AmigaOS.

Ping? Sorry to push it but it would be great if this could get in for 3.0. 
Peter, this may need your attention like last time as it's not clear whose 
tree it should go through and David did not seem to be confident taking it 
via the ppc tree.

Thank you,
BALATON Zoltan

> BALATON Zoltan (3):
>  sm501: Implement i2c part for reading monitor EDID
>  sm501: Fix support for non-zero frame buffer start address
>  sm501: Set updated region dirty after 2D operation
>
> Sebastian Bauer (4):
>  sm501: Perform a full update after palette change
>  sm501: Use values from the pitch register for 2D operations
>  sm501: Implement negated destination raster operation mode
>  sm501: Log unimplemented raster operation modes
>
> 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                 | 229 +++++++++++++++++++++++++++++++++++--
> 5 files changed, 223 insertions(+), 12 deletions(-)
>
>

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[Peter Maydell]

On 30 June 2018 at 21:34, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Tue, 26 Jun 2018, BALATON Zoltan wrote:
>>
>> Version 2 of the sm501 changes with fixes that are needed to get
>> AmigaOS 4.1FE to boot and able to produce graphics.
>>
>> The strange blue-white colors that first appear are actually correct
>> and because of AmigaOS selecting a low resolution PAL mode by default
>> instead of a board specific mode. To work around this one can select
>> the last option to boot the live CD and then select a better board
>> specific mode from ScreenMode Prefs. It takes a while for the
>> ScreenMode window to appear and graphics operations are slow which
>> could use some improvement but at least it seems to work correctly now
>> apart from some unimplemented drawing modes for compositing.
>>
>> If this could be merged before the freeze with the sam460ex patches
>> and Sebastian's ehci patch then QEMU 3.0 could be the first version
>> that can boot AmigaOS.
>
>
> Ping? Sorry to push it but it would be great if this could get in for 3.0.
> Peter, this may need your attention like last time as it's not clear whose
> tree it should go through and David did not seem to be confident taking it
> via the ppc tree.

I can have a look, but really I think these should go via the
ppc tree -- the device is used in a ppc machine and an sh4 one,
and the ppc tree is much more active than sh4.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[BALATON Zoltan]

On Mon, 2 Jul 2018, Peter Maydell wrote:
> On 30 June 2018 at 21:34, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Tue, 26 Jun 2018, BALATON Zoltan wrote:
>>>
>>> Version 2 of the sm501 changes with fixes that are needed to get
>>> AmigaOS 4.1FE to boot and able to produce graphics.
>>>
>>> The strange blue-white colors that first appear are actually correct
>>> and because of AmigaOS selecting a low resolution PAL mode by default
>>> instead of a board specific mode. To work around this one can select
>>> the last option to boot the live CD and then select a better board
>>> specific mode from ScreenMode Prefs. It takes a while for the
>>> ScreenMode window to appear and graphics operations are slow which
>>> could use some improvement but at least it seems to work correctly now
>>> apart from some unimplemented drawing modes for compositing.
>>>
>>> If this could be merged before the freeze with the sam460ex patches
>>> and Sebastian's ehci patch then QEMU 3.0 could be the first version
>>> that can boot AmigaOS.
>>
>>
>> Ping? Sorry to push it but it would be great if this could get in for 3.0.
>> Peter, this may need your attention like last time as it's not clear whose
>> tree it should go through and David did not seem to be confident taking it
>> via the ppc tree.
>
> I can have a look, but really I think these should go via the
> ppc tree -- the device is used in a ppc machine and an sh4 one,
> and the ppc tree is much more active than sh4.

Thank you. Please agree on what should be the needed action here. It would 
be a pity if this missed 3.0 because of this. Could it be merged before 
the freeze if no obvious problems are found with that it can be removed 
later if it causes any trouble (I think that's unlikely as it's only 
touches rarely used parts).

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[Peter Maydell]

On 2 July 2018 at 11:33, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Thank you. Please agree on what should be the needed action here. It would
> be a pity if this missed 3.0 because of this. Could it be merged before the
> freeze if no obvious problems are found with that it can be removed later if
> it causes any trouble (I think that's unlikely as it's only touches rarely
> used parts).

These are bug fixes, so there is no strict requirement for them
to get in for the softfreeze deadline. I agree that we should
get these in for 3.0.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[BALATON Zoltan]

On Mon, 2 Jul 2018, Peter Maydell wrote:
> On 2 July 2018 at 11:33, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> Thank you. Please agree on what should be the needed action here. It would
>> be a pity if this missed 3.0 because of this. Could it be merged before the
>> freeze if no obvious problems are found with that it can be removed later if
>> it causes any trouble (I think that's unlikely as it's only touches rarely
>> used parts).
>
> These are bug fixes, so there is no strict requirement for them
> to get in for the softfreeze deadline. I agree that we should
> get these in for 3.0.

Some of them do implement new features of sm501 previously not emulated so 
it depends on your judgement but if we get a green card from you that 
these can get in as bugfixes before hard freeze then I'm not worried that 
much. :-)

Thank you,
BALATON Zoltan

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[David Gibson]

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

On Mon, Jul 02, 2018 at 10:42:07AM +0100, Peter Maydell wrote:
> On 30 June 2018 at 21:34, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > On Tue, 26 Jun 2018, BALATON Zoltan wrote:
> >>
> >> Version 2 of the sm501 changes with fixes that are needed to get
> >> AmigaOS 4.1FE to boot and able to produce graphics.
> >>
> >> The strange blue-white colors that first appear are actually correct
> >> and because of AmigaOS selecting a low resolution PAL mode by default
> >> instead of a board specific mode. To work around this one can select
> >> the last option to boot the live CD and then select a better board
> >> specific mode from ScreenMode Prefs. It takes a while for the
> >> ScreenMode window to appear and graphics operations are slow which
> >> could use some improvement but at least it seems to work correctly now
> >> apart from some unimplemented drawing modes for compositing.
> >>
> >> If this could be merged before the freeze with the sam460ex patches
> >> and Sebastian's ehci patch then QEMU 3.0 could be the first version
> >> that can boot AmigaOS.
> >
> >
> > Ping? Sorry to push it but it would be great if this could get in for 3.0.
> > Peter, this may need your attention like last time as it's not clear whose
> > tree it should go through and David did not seem to be confident taking it
> > via the ppc tree.
> 
> I can have a look, but really I think these should go via the
> ppc tree -- the device is used in a ppc machine and an sh4 one,
> and the ppc tree is much more active than sh4.

Hm, well, if you like.  Although the gradual creep of my
maintainership scope into things I know less and less about makes me
rather nervous.  I tried to convince Balaton Zoltan to become sm501
maintainer, but he didn't bite.

Zoltan, I'm sorry I haven't been following this series closely since I
asked you to direct it to Peter.  If you resend to me, with whatever
accumulated reviews you have, I'll merge it via the ppc tree.  It
won't make the soft freeze, but seems we have a quorum to consider it
close enough to a bug fix, so we should be able to squeeze it in
before the hard freeze.

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

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[Peter Maydell]

On 3 July 2018 at 01:28, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Jul 02, 2018 at 10:42:07AM +0100, Peter Maydell wrote:
>> I can have a look, but really I think these should go via the
>> ppc tree -- the device is used in a ppc machine and an sh4 one,
>> and the ppc tree is much more active than sh4.
>
> Hm, well, if you like.  Although the gradual creep of my
> maintainership scope into things I know less and less about makes me
> rather nervous.  I tried to convince Balaton Zoltan to become sm501
> maintainer, but he didn't bite.

It's not like I know anything more about the sm501 than you do :-)

The arm parts of the tree have a lot of board and device code
I'm not familiar with either. Generally the approach I use is:
 * eyeball the code to see if it's doing anything that looks
   "obviously wrong", failing to use correct QEMU APIs, etc
 * anything that touches 'core' code or code common to multiple
   machines gets closer scrutiny
 * I might check the data sheet if I'm feeling enthusastic or
   if the code looks like it's doing weird things, but often
   not, especially if the code has had review from somebody else
 * if I have a test image to hand I'll do a smoke test, but often
   I don't have a test image

Basically I think the important thing is to make sure the
codebase stays maintainable and generally the quality bar
in terms of using the right APIs and design approaches
tends to ratchet up rather than down. If our implementation
of an obscure device isn't actually right, that doesn't
really affect very many users, so I worry less about that
side of things.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/7] sm501: Implement i2c part for reading monitor EDID

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 06/26/2018 06:18 PM, 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>
> ---
> v2:
> - added constants for register bits
> - fix clearing error bit in reset reg
> - set max access size to 1
> v1: fixed build with SH
> 
>  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                 | 146 ++++++++++++++++++++++++++++++++++++-
>  5 files changed, 148 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 8206ae8..273495e 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"
>  
> @@ -216,6 +219,14 @@
>  #define SM501_I2C_SLAVE_ADDRESS         (0x03)
>  #define SM501_I2C_DATA                  (0x04)
>  
> +#define SM501_I2C_CONTROL_START         (1 << 2)
> +#define SM501_I2C_CONTROL_ENABLE        (1 << 0)
> +
> +#define SM501_I2C_STATUS_COMPLETE       (1 << 3)
> +#define SM501_I2C_STATUS_ERROR          (1 << 2)
> +
> +#define SM501_I2C_RESET_ERROR           (1 << 2)
> +
>  /* SSP base */
>  #define SM501_SSP                       (0x020000)
>  
> @@ -471,10 +482,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 +500,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 +915,109 @@ 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 & SM501_I2C_CONTROL_ENABLE) {
> +            if (value & SM501_I2C_CONTROL_START) {
> +                int res = i2c_start_transfer(s->i2c_bus,
> +                                             s->i2c_addr >> 1,
> +                                             s->i2c_addr & 1);
> +                s->i2c_status |= (res ? SM501_I2C_STATUS_ERROR : 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 ? SM501_I2C_STATUS_ERROR : 0);
> +                            return;
> +                        }
> +                    }
> +                    if (i) {
> +                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
> +                        s->i2c_status = SM501_I2C_STATUS_COMPLETE;
> +                    }
> +                }
> +            } else {
> +                SM501_DPRINTF("sm501 i2c : end transfer\n");
> +                i2c_end_transfer(s->i2c_bus);
> +                s->i2c_status &= ~SM501_I2C_STATUS_ERROR;
> +            }
> +        }
> +        break;
> +    case SM501_I2C_RESET:
> +        if ((value & SM501_I2C_RESET_ERROR) == 0) {
> +            s->i2c_status &= SM501_I2C_STATUS_ERROR;

Shouldn't be the negate mask?:

               s->i2c_status &= ~SM501_I2C_STATUS_ERROR;

> +        }
> +        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 = 1,
> +    },
> +    .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 +1698,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);

       memset(s->i2c_data, 0, sizeof(s->i2c_data));

>      s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>      s->dc_video_control = 0;
>      s->dc_crt_control = 0x00010000;
> @@ -1615,6 +1740,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");

I'd appreciate a separator here:

       /* ddc */

> +    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 +1752,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 +1838,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 +1908,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 +1981,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,
> 

With the negate mask in SM501_I2C_RESET:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[Philippe Mathieu-Daudé]

Hi David, I'll reply to you using Peter mail :)

On 07/03/2018 06:13 AM, Peter Maydell wrote:
> On 3 July 2018 at 01:28, David Gibson <david@gibson.dropbear.id.au> wrote:
>> On Mon, Jul 02, 2018 at 10:42:07AM +0100, Peter Maydell wrote:
>>> I can have a look, but really I think these should go via the
>>> ppc tree -- the device is used in a ppc machine and an sh4 one,
>>> and the ppc tree is much more active than sh4.
>>
>> Hm, well, if you like.  Although the gradual creep of my
>> maintainership scope into things I know less and less about makes me
>> rather nervous.  I tried to convince Balaton Zoltan to become sm501
>> maintainer, but he didn't bite.
> 
> It's not like I know anything more about the sm501 than you do :-)
> 
> The arm parts of the tree have a lot of board and device code
> I'm not familiar with either. Generally the approach I use is:
>  * eyeball the code to see if it's doing anything that looks
>    "obviously wrong", failing to use correct QEMU APIs, etc

Gerd Hoffmann did a review for the graphic code,
then commented "all look sane":
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06493.html

I don't have a serious understanding of QEMU graphic internals, but
looked at those patches and didn't find anything "obviously wrong".

>  * anything that touches 'core' code or code common to multiple
>    machines gets closer scrutiny

The only SH4 tests I run are using Aurelien Debian:
https://people.debian.org/~aurel32/qemu/sh4/

>  * I might check the data sheet if I'm feeling enthusastic or
>    if the code looks like it's doing weird things, but often
>    not, especially if the code has had review from somebody else

I did a review of the I2C/DDC patch with the specs at hand.

>  * if I have a test image to hand I'll do a smoke test, but often
>    I don't have a test image

Neither do I.

> Basically I think the important thing is to make sure the
> codebase stays maintainable and generally the quality bar
> in terms of using the right APIs and design approaches
> tends to ratchet up rather than down. If our implementation
> of an obscure device isn't actually right, that doesn't
> really affect very many users, so I worry less about that
> side of things.

Zoltan used few deprecated API but said updating "could be done in
separate clean up", which I agree :)
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06911.html

FWIW and using Peter's criteria I think this series is good to go.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2 0/7] Misc sm501 improvements

[BALATON Zoltan]

On Wed, 4 Jul 2018, Philippe Mathieu-Daudé wrote:
> Hi David, I'll reply to you using Peter mail :)

Thanks a lot for this summary and your review spotting an error. I forgot 
about Gerd's OK as he did not send formal Reviewed or Acked but you're 
right that's also counts to show that the series should be sane.

I did not want to become maintainer of sm501 for at least two reasons:

1. Because it was part of SH and thus it should already have a maintainer 
or if it doesn't any more then I can't take that either as I don't know SH

2. I'm not familiar with the tasks a maintainer has to do and don't have 
enough time now to do that

But if it's only a question of responsibility I don't ask you to take 
that, only ask David or Peter to help in merging this. I take 
responsibility for problems it causes and will try to fix it or I'm OK to 
have it removed if I fail to do that. Hope that's acceptable at least for 
now, then we can discuss about what to do with the sam460ex and related 
parts for the future.

Thank you,
BALATON Zoltan

> On 07/03/2018 06:13 AM, Peter Maydell wrote:
>> On 3 July 2018 at 01:28, David Gibson <david@gibson.dropbear.id.au> wrote:
>>> On Mon, Jul 02, 2018 at 10:42:07AM +0100, Peter Maydell wrote:
>>>> I can have a look, but really I think these should go via the
>>>> ppc tree -- the device is used in a ppc machine and an sh4 one,
>>>> and the ppc tree is much more active than sh4.
>>>
>>> Hm, well, if you like.  Although the gradual creep of my
>>> maintainership scope into things I know less and less about makes me
>>> rather nervous.  I tried to convince Balaton Zoltan to become sm501
>>> maintainer, but he didn't bite.
>>
>> It's not like I know anything more about the sm501 than you do :-)
>>
>> The arm parts of the tree have a lot of board and device code
>> I'm not familiar with either. Generally the approach I use is:
>>  * eyeball the code to see if it's doing anything that looks
>>    "obviously wrong", failing to use correct QEMU APIs, etc
>
> Gerd Hoffmann did a review for the graphic code,
> then commented "all look sane":
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06493.html
>
> I don't have a serious understanding of QEMU graphic internals, but
> looked at those patches and didn't find anything "obviously wrong".
>
>>  * anything that touches 'core' code or code common to multiple
>>    machines gets closer scrutiny
>
> The only SH4 tests I run are using Aurelien Debian:
> https://people.debian.org/~aurel32/qemu/sh4/
>
>>  * I might check the data sheet if I'm feeling enthusastic or
>>    if the code looks like it's doing weird things, but often
>>    not, especially if the code has had review from somebody else
>
> I did a review of the I2C/DDC patch with the specs at hand.
>
>>  * if I have a test image to hand I'll do a smoke test, but often
>>    I don't have a test image
>
> Neither do I.
>
>> Basically I think the important thing is to make sure the
>> codebase stays maintainable and generally the quality bar
>> in terms of using the right APIs and design approaches
>> tends to ratchet up rather than down. If our implementation
>> of an obscure device isn't actually right, that doesn't
>> really affect very many users, so I worry less about that
>> side of things.
>
> Zoltan used few deprecated API but said updating "could be done in
> separate clean up", which I agree :)
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06911.html
>
> FWIW and using Peter's criteria I think this series is good to go.
>
> Regards,
>
> Phil.
>
>