* [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-13 1:22 ` David Gibson
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 6/8] sam460ex: Add RTC device BALATON Zoltan
` (6 subsequent siblings)
7 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
default-configs/ppc-softmmu.mak | 1 +
default-configs/ppcemb-softmmu.mak | 1 +
hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 4d7be45..7d0dc2f 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
CONFIG_SM501=y
CONFIG_IDE_SII3112=y
CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
# For Macs
CONFIG_MAC=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 67d18b2..37af193 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
CONFIG_SM501=y
CONFIG_IDE_SII3112=y
CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index a68b5f7..5806209 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -30,6 +30,7 @@
#include "cpu.h"
#include "hw/hw.h"
#include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
#define PPC4xx_I2C_MEM_SIZE 18
@@ -46,7 +47,13 @@
#define IIC_XTCNTLSS_SRST (1 << 0)
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
typedef struct {
+ bitbang_i2c_interface *bitbang;
uint8_t mdata;
uint8_t lmadr;
uint8_t hmadr;
@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
i2c->xtcntlss = value;
break;
case 16:
- i2c->directcntl = value & 0x7;
+ i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+ i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+ bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
+ i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+ (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
break;
default:
if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
s->bus = i2c_init_bus(DEVICE(s), "i2c");
+ r->bitbang = bitbang_i2c_init(s->bus);
}
static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
` (2 preceding siblings ...)
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 8/8] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 15:56 ` Philippe Mathieu-Daudé
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 4/8] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
` (3 subsequent siblings)
7 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Make it more readable by converting register indexes to decimal
(avoids lot of superfluous 0x0) and distinguish errors caused by
accessing non-existent vs. unimplemented registers.
No functional change.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
1 file changed, 51 insertions(+), 43 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index ab64d19..d1936db 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -31,7 +31,7 @@
#include "hw/hw.h"
#include "hw/i2c/ppc4xx_i2c.h"
-#define PPC4xx_I2C_MEM_SIZE 0x12
+#define PPC4xx_I2C_MEM_SIZE 18
#define IIC_CNTL_PT (1 << 0)
#define IIC_CNTL_READ (1 << 1)
@@ -70,7 +70,7 @@ static void ppc4xx_i2c_reset(DeviceState *s)
i2c->intrmsk = 0;
i2c->xfrcnt = 0;
i2c->xtcntlss = 0;
- i2c->directcntl = 0x0f;
+ i2c->directcntl = 0xf;
i2c->intr = 0;
}
@@ -85,7 +85,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
uint64_t ret;
switch (addr) {
- case 0x00:
+ case 0:
ret = i2c->mdata;
if (ppc4xx_i2c_is_master(i2c)) {
ret = 0xff;
@@ -139,58 +139,62 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
TYPE_PPC4xx_I2C, __func__);
}
break;
- case 0x02:
+ case 2:
ret = i2c->sdata;
break;
- case 0x04:
+ case 4:
ret = i2c->lmadr;
break;
- case 0x05:
+ case 5:
ret = i2c->hmadr;
break;
- case 0x06:
+ case 6:
ret = i2c->cntl;
break;
- case 0x07:
+ case 7:
ret = i2c->mdcntl;
break;
- case 0x08:
+ case 8:
ret = i2c->sts;
break;
- case 0x09:
+ case 9:
ret = i2c->extsts;
break;
- case 0x0A:
+ case 10:
ret = i2c->lsadr;
break;
- case 0x0B:
+ case 11:
ret = i2c->hsadr;
break;
- case 0x0C:
+ case 12:
ret = i2c->clkdiv;
break;
- case 0x0D:
+ case 13:
ret = i2c->intrmsk;
break;
- case 0x0E:
+ case 14:
ret = i2c->xfrcnt;
break;
- case 0x0F:
+ case 15:
ret = i2c->xtcntlss;
break;
- case 0x10:
+ case 16:
ret = i2c->directcntl;
break;
- case 0x11:
+ case 17:
ret = i2c->intr;
break;
default:
- qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
- HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
+ if (addr < PPC4xx_I2C_MEM_SIZE) {
+ qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+ }
ret = 0;
break;
}
-
return ret;
}
@@ -200,7 +204,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
PPC4xxI2CState *i2c = opaque;
switch (addr) {
- case 0x00:
+ case 0:
i2c->mdata = value;
if (!i2c_bus_busy(i2c->bus)) {
/* assume we start a write transfer */
@@ -225,19 +229,19 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
}
}
break;
- case 0x02:
+ case 2:
i2c->sdata = value;
break;
- case 0x04:
+ case 4:
i2c->lmadr = value;
if (i2c_bus_busy(i2c->bus)) {
i2c_end_transfer(i2c->bus);
}
break;
- case 0x05:
+ case 5:
i2c->hmadr = value;
break;
- case 0x06:
+ case 6:
i2c->cntl = value;
if (i2c->cntl & IIC_CNTL_PT) {
if (i2c->cntl & IIC_CNTL_READ) {
@@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
}
}
break;
- case 0x07:
- i2c->mdcntl = value & 0xDF;
+ case 7:
+ i2c->mdcntl = value & 0xdf;
break;
- case 0x08:
- i2c->sts &= ~(value & 0x0A);
+ case 8:
+ i2c->sts &= ~(value & 0xa);
break;
- case 0x09:
- i2c->extsts &= ~(value & 0x8F);
+ case 9:
+ i2c->extsts &= ~(value & 0x8f);
break;
- case 0x0A:
+ case 10:
i2c->lsadr = value;
- /*i2c_set_slave_address(i2c->bus, i2c->lsadr);*/
break;
- case 0x0B:
+ case 11:
i2c->hsadr = value;
break;
- case 0x0C:
+ case 12:
i2c->clkdiv = value;
break;
- case 0x0D:
+ case 13:
i2c->intrmsk = value;
break;
- case 0x0E:
+ case 14:
i2c->xfrcnt = value & 0x77;
break;
- case 0x0F:
+ case 15:
if (value & IIC_XTCNTLSS_SRST) {
/* Is it actually a full reset? U-Boot sets some regs before */
ppc4xx_i2c_reset(DEVICE(i2c));
@@ -296,15 +299,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
}
i2c->xtcntlss = value;
break;
- case 0x10:
+ case 16:
i2c->directcntl = value & 0x7;
break;
- case 0x11:
+ case 17:
i2c->intr = value;
break;
default:
- qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
- HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
+ if (addr < PPC4xx_I2C_MEM_SIZE) {
+ qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
+ HWADDR_PRIx "\n", __func__, addr);
+ }
break;
}
}
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 8/8] sm501: Implement i2c part for reading monitor EDID
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 6/8] sam460ex: Add RTC device BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging BALATON Zoltan
` (4 subsequent siblings)
7 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson, Peter Maydell
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
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 9fbaadc..860de80 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 7ec1434..d357583 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;
@@ -894,6 +904,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;
@@ -1574,6 +1685,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;
@@ -1612,6 +1727,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),
@@ -1619,6 +1739,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);
@@ -1702,6 +1825,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()
}
};
@@ -1767,8 +1895,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),
@@ -1840,8 +1968,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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
` (5 preceding siblings ...)
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-08 8:56 ` David Gibson
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation BALATON Zoltan
7 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 9149 bytes --]
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
2 files changed, 43 insertions(+), 51 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index d1936db..a68b5f7 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -3,7 +3,7 @@
*
* Copyright (c) 2007 Jocelyn Mayer
* Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
@@ -46,9 +46,26 @@
#define IIC_XTCNTLSS_SRST (1 << 0)
+typedef struct {
+ uint8_t mdata;
+ uint8_t lmadr;
+ uint8_t hmadr;
+ uint8_t cntl;
+ uint8_t mdcntl;
+ uint8_t sts;
+ uint8_t extsts;
+ uint8_t lsadr;
+ uint8_t hsadr;
+ uint8_t clkdiv;
+ uint8_t intrmsk;
+ uint8_t xfrcnt;
+ uint8_t xtcntlss;
+ uint8_t directcntl;
+} PPC4xxI2CRegs;
+
static void ppc4xx_i2c_reset(DeviceState *s)
{
- PPC4xxI2CState *i2c = PPC4xx_I2C(s);
+ PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
/* FIXME: Should also reset bus?
*if (s->address != ADDR_RESET) {
@@ -63,7 +80,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
i2c->mdcntl = 0;
i2c->sts = 0;
i2c->extsts = 0x8f;
- i2c->sdata = 0;
i2c->lsadr = 0;
i2c->hsadr = 0;
i2c->clkdiv = 0;
@@ -71,7 +87,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
i2c->xfrcnt = 0;
i2c->xtcntlss = 0;
i2c->directcntl = 0xf;
- i2c->intr = 0;
}
static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -81,13 +96,14 @@ static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
{
- PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
+ PPC4xxI2CState *s = PPC4xx_I2C(opaque);
+ PPC4xxI2CRegs *i2c = s->regs;
uint64_t ret;
switch (addr) {
case 0:
ret = i2c->mdata;
- if (ppc4xx_i2c_is_master(i2c)) {
+ if (ppc4xx_i2c_is_master(s)) {
ret = 0xff;
if (!(i2c->sts & IIC_STS_MDBS)) {
@@ -98,7 +114,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
int pending = (i2c->cntl >> 4) & 3;
/* get the next byte */
- int byte = i2c_recv(i2c->bus);
+ int byte = i2c_recv(s->bus);
if (byte < 0) {
qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
@@ -113,13 +129,13 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
if (!pending) {
i2c->sts &= ~IIC_STS_MDBS;
- /*i2c_end_transfer(i2c->bus);*/
+ /*i2c_end_transfer(s->bus);*/
/*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
} else if (pending) {
/* current smbus implementation doesn't like
multibyte xfer repeated start */
- i2c_end_transfer(i2c->bus);
- if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
+ i2c_end_transfer(s->bus);
+ if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
/* if non zero is returned, the adress is not valid */
i2c->sts &= ~IIC_STS_PT;
i2c->sts |= IIC_STS_ERR;
@@ -139,9 +155,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
TYPE_PPC4xx_I2C, __func__);
}
break;
- case 2:
- ret = i2c->sdata;
- break;
case 4:
ret = i2c->lmadr;
break;
@@ -181,9 +194,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
case 16:
ret = i2c->directcntl;
break;
- case 17:
- ret = i2c->intr;
- break;
default:
if (addr < PPC4xx_I2C_MEM_SIZE) {
qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -201,14 +211,15 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
unsigned int size)
{
- PPC4xxI2CState *i2c = opaque;
+ PPC4xxI2CState *s = PPC4xx_I2C(opaque);
+ PPC4xxI2CRegs *i2c = s->regs;
switch (addr) {
case 0:
i2c->mdata = value;
- if (!i2c_bus_busy(i2c->bus)) {
+ if (!i2c_bus_busy(s->bus)) {
/* assume we start a write transfer */
- if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 0)) {
+ if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 0)) {
/* if non zero is returned, the adress is not valid */
i2c->sts &= ~IIC_STS_PT;
i2c->sts |= IIC_STS_ERR;
@@ -219,23 +230,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
i2c->extsts = 0;
}
}
- if (i2c_bus_busy(i2c->bus)) {
- if (i2c_send(i2c->bus, i2c->mdata)) {
+ if (i2c_bus_busy(s->bus)) {
+ if (i2c_send(s->bus, i2c->mdata)) {
/* if the target return non zero then end the transfer */
i2c->sts &= ~IIC_STS_PT;
i2c->sts |= IIC_STS_ERR;
i2c->extsts |= IIC_EXTSTS_XFRA;
- i2c_end_transfer(i2c->bus);
+ i2c_end_transfer(s->bus);
}
}
break;
- case 2:
- i2c->sdata = value;
- break;
case 4:
i2c->lmadr = value;
- if (i2c_bus_busy(i2c->bus)) {
- i2c_end_transfer(i2c->bus);
+ if (i2c_bus_busy(s->bus)) {
+ i2c_end_transfer(s->bus);
}
break;
case 5:
@@ -245,12 +253,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
i2c->cntl = value;
if (i2c->cntl & IIC_CNTL_PT) {
if (i2c->cntl & IIC_CNTL_READ) {
- if (i2c_bus_busy(i2c->bus)) {
+ if (i2c_bus_busy(s->bus)) {
/* end previous transfer */
i2c->sts &= ~IIC_STS_PT;
- i2c_end_transfer(i2c->bus);
+ i2c_end_transfer(s->bus);
}
- if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
+ if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
/* if non zero is returned, the adress is not valid */
i2c->sts &= ~IIC_STS_PT;
i2c->sts |= IIC_STS_ERR;
@@ -294,7 +302,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
case 15:
if (value & IIC_XTCNTLSS_SRST) {
/* Is it actually a full reset? U-Boot sets some regs before */
- ppc4xx_i2c_reset(DEVICE(i2c));
+ ppc4xx_i2c_reset(DEVICE(s));
break;
}
i2c->xtcntlss = value;
@@ -302,9 +310,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
case 16:
i2c->directcntl = value & 0x7;
break;
- case 17:
- i2c->intr = value;
- break;
default:
if (addr < PPC4xx_I2C_MEM_SIZE) {
qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
static void ppc4xx_i2c_init(Object *o)
{
PPC4xxI2CState *s = PPC4xx_I2C(o);
+ PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
+ s->regs = r;
memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index 3c60307..1d5ba0c 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -3,7 +3,7 @@
*
* Copyright (c) 2007 Jocelyn Mayer
* Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
@@ -37,27 +37,12 @@
typedef struct PPC4xxI2CState {
/*< private >*/
SysBusDevice parent_obj;
+ void *regs;
/*< public >*/
I2CBus *bus;
qemu_irq irq;
MemoryRegion iomem;
- uint8_t mdata;
- uint8_t lmadr;
- uint8_t hmadr;
- uint8_t cntl;
- uint8_t mdcntl;
- uint8_t sts;
- uint8_t extsts;
- uint8_t sdata;
- uint8_t lsadr;
- uint8_t hsadr;
- uint8_t clkdiv;
- uint8_t intrmsk;
- uint8_t xfrcnt;
- uint8_t xtcntlss;
- uint8_t directcntl;
- uint8_t intr;
} PPC4xxI2CState;
#endif /* PPC4XX_I2C_H */
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
` (6 preceding siblings ...)
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 16:03 ` Philippe Mathieu-Daudé
2018-06-08 12:42 ` Cédric Le Goater
7 siblings, 2 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
of day is implemented. Setting time and RTC alarm are not supported.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
MAINTAINERS | 1 +
default-configs/ppc-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 120 insertions(+)
create mode 100644 hw/timer/m41t80.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 41cd373..9e13bc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
L: qemu-ppc@nongnu.org
S: Maintained
F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
SH4 Machines
------------
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index 7d0dc2f..9fbaadc 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -27,6 +27,7 @@ CONFIG_SM501=y
CONFIG_IDE_SII3112=y
CONFIG_I2C=y
CONFIG_BITBANG_I2C=y
+CONFIG_M41T80=y
# For Macs
CONFIG_MAC=y
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 8b27a4b..e16b2b9 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
common-obj-$(CONFIG_DS1338) += ds1338.o
common-obj-$(CONFIG_HPET) += hpet.o
common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
+common-obj-$(CONFIG_M41T80) += m41t80.o
common-obj-$(CONFIG_M48T59) += m48t59.o
ifeq ($(CONFIG_ISA_BUS),y)
common-obj-$(CONFIG_M48T59) += m48t59-isa.o
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
new file mode 100644
index 0000000..9dbdb1b
--- /dev/null
+++ b/hw/timer/m41t80.c
@@ -0,0 +1,117 @@
+/*
+ * M41T80 serial rtc emulation
+ *
+ * Copyright (c) 2018 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_M41T80 "m41t80"
+#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
+
+typedef struct M41t80State {
+ I2CSlave parent_obj;
+ int8_t addr;
+} M41t80State;
+
+static void m41t80_realize(DeviceState *dev, Error **errp)
+{
+ M41t80State *s = M41T80(dev);
+
+ s->addr = -1;
+}
+
+static int m41t80_send(I2CSlave *i2c, uint8_t data)
+{
+ M41t80State *s = M41T80(i2c);
+
+ if (s->addr < 0) {
+ s->addr = data;
+ } else {
+ s->addr++;
+ }
+ return 0;
+}
+
+static int m41t80_recv(I2CSlave *i2c)
+{
+ M41t80State *s = M41T80(i2c);
+ struct tm now;
+ qemu_timeval tv;
+
+ if (s->addr < 0) {
+ s->addr = 0;
+ }
+ if (s->addr >= 1 && s->addr <= 7) {
+ qemu_get_timedate(&now, -1);
+ }
+ switch (s->addr++) {
+ case 0:
+ qemu_gettimeofday(&tv);
+ return to_bcd(tv.tv_usec / 10000);
+ case 1:
+ return to_bcd(now.tm_sec);
+ case 2:
+ return to_bcd(now.tm_min);
+ case 3:
+ return to_bcd(now.tm_hour);
+ case 4:
+ return to_bcd(now.tm_wday);
+ case 5:
+ return to_bcd(now.tm_mday);
+ case 6:
+ return to_bcd(now.tm_mon + 1);
+ case 7:
+ return to_bcd(now.tm_year % 100);
+ case 8 ... 19:
+ qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
+ __func__, s->addr - 1);
+ return 0;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
+ __func__, s->addr - 1);
+ return 0;
+ }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+ M41t80State *s = M41T80(i2c);
+
+ if (event == I2C_START_SEND) {
+ s->addr = -1;
+ }
+ return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+ dc->realize = m41t80_realize;
+ sc->send = m41t80_send;
+ sc->recv = m41t80_recv;
+ sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+ .name = TYPE_M41T80,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_size = sizeof(M41t80State),
+ .class_init = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+ type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements
@ 2018-06-06 13:31 BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
` (7 more replies)
0 siblings, 8 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Some more improvements to sam460ex emulation:
- Implemented RTC (which also needed fixing I2C to work)
- Fix a problem in sm501 which led to AmigaOS slow down
v2:
- split patch 1 (i2c rewrite) into smaller pieces
- Added another sm501 patch
BALATON Zoltan (8):
ppc4xx_i2c: Clean up and improve error logging
ppc4xx_i2c: Move register state to private struct and remove
unimplemented sdata and intr registers
ppc4xx_i2c: Implement directcntl register
ppc4xx_i2c: Rewrite to model hardware more closely
hw/timer: Add basic M41T80 emulation
sam460ex: Add RTC device
sm501: Do not clear read only bits when writing register
sm501: Implement i2c part for reading monitor EDID
MAINTAINERS | 1 +
default-configs/ppc-softmmu.mak | 3 +
default-configs/ppcemb-softmmu.mak | 2 +
default-configs/sh4-softmmu.mak | 1 +
default-configs/sh4eb-softmmu.mak | 1 +
hw/display/sm501.c | 142 ++++++++++++++-
hw/i2c/ppc4xx_i2c.c | 364 ++++++++++++++++++++-----------------
hw/ppc/sam460ex.c | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/m41t80.c | 117 ++++++++++++
include/hw/i2c/ppc4xx_i2c.h | 19 +-
11 files changed, 457 insertions(+), 195 deletions(-)
create mode 100644 hw/timer/m41t80.c
--
2.7.6
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 4/8] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
` (3 preceding siblings ...)
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register BALATON Zoltan
` (2 subsequent siblings)
7 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Rewrite to make it closer to how real device works so that guest OS
drivers can access I2C devices. Previously this was only a hack to
allow U-Boot to get past accessing SPD EEPROMs but to support other
I2C devices and allow guests to access them we need to model real
device more properly.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
Can't split this up more but since previous version really only worked
with U-Boot and this version still works with that what else could break?
This version however also works with AROS, Linux and AmigaOS. So it's
an improvement and if something else breaks due to this then that's a
bug that should be fixed separately instead of this patch being dropped.
hw/i2c/ppc4xx_i2c.c | 223 +++++++++++++++++++++++++---------------------------
1 file changed, 109 insertions(+), 114 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index 5806209..8c381ec 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -38,13 +38,26 @@
#define IIC_CNTL_READ (1 << 1)
#define IIC_CNTL_CHT (1 << 2)
#define IIC_CNTL_RPST (1 << 3)
+#define IIC_CNTL_AMD (1 << 6)
+#define IIC_CNTL_HMT (1 << 7)
+
+#define IIC_MDCNTL_EINT (1 << 2)
+#define IIC_MDCNTL_ESM (1 << 3)
+#define IIC_MDCNTL_FMDB (1 << 6)
#define IIC_STS_PT (1 << 0)
+#define IIC_STS_IRQA (1 << 1)
#define IIC_STS_ERR (1 << 2)
+#define IIC_STS_MDBF (1 << 4)
#define IIC_STS_MDBS (1 << 5)
#define IIC_EXTSTS_XFRA (1 << 0)
+#define IIC_INTRMSK_EIMTC (1 << 0)
+#define IIC_INTRMSK_EITA (1 << 1)
+#define IIC_INTRMSK_EIIC (1 << 2)
+#define IIC_INTRMSK_EIHE (1 << 3)
+
#define IIC_XTCNTLSS_SRST (1 << 0)
#define IIC_DIRECTCNTL_SDAC (1 << 3)
@@ -54,7 +67,8 @@
typedef struct {
bitbang_i2c_interface *bitbang;
- uint8_t mdata;
+ int mdidx;
+ uint8_t mdata[4];
uint8_t lmadr;
uint8_t hmadr;
uint8_t cntl;
@@ -74,21 +88,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
{
PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
- /* FIXME: Should also reset bus?
- *if (s->address != ADDR_RESET) {
- * i2c_end_transfer(s->bus);
- *}
- */
-
- i2c->mdata = 0;
- i2c->lmadr = 0;
- i2c->hmadr = 0;
+ i2c->mdidx = -1;
+ memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
+ /* [hl][ms]addr are not affected by reset */
i2c->cntl = 0;
i2c->mdcntl = 0;
i2c->sts = 0;
- i2c->extsts = 0x8f;
- i2c->lsadr = 0;
- i2c->hsadr = 0;
+ i2c->extsts = (1 << 6);
i2c->clkdiv = 0;
i2c->intrmsk = 0;
i2c->xfrcnt = 0;
@@ -96,70 +102,30 @@ static void ppc4xx_i2c_reset(DeviceState *s)
i2c->directcntl = 0xf;
}
-static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
-{
- return true;
-}
-
static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
{
PPC4xxI2CState *s = PPC4xx_I2C(opaque);
PPC4xxI2CRegs *i2c = s->regs;
uint64_t ret;
+ int i;
switch (addr) {
case 0:
- ret = i2c->mdata;
- if (ppc4xx_i2c_is_master(s)) {
+ if (i2c->mdidx < 0) {
ret = 0xff;
-
- if (!(i2c->sts & IIC_STS_MDBS)) {
- qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read "
- "without starting transfer\n",
- TYPE_PPC4xx_I2C, __func__);
- } else {
- int pending = (i2c->cntl >> 4) & 3;
-
- /* get the next byte */
- int byte = i2c_recv(s->bus);
-
- if (byte < 0) {
- qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
- "for device 0x%02x\n", TYPE_PPC4xx_I2C,
- __func__, i2c->lmadr);
- ret = 0xff;
- } else {
- ret = byte;
- /* Raise interrupt if enabled */
- /*ppc4xx_i2c_raise_interrupt(i2c)*/;
- }
-
- if (!pending) {
- i2c->sts &= ~IIC_STS_MDBS;
- /*i2c_end_transfer(s->bus);*/
- /*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
- } else if (pending) {
- /* current smbus implementation doesn't like
- multibyte xfer repeated start */
- i2c_end_transfer(s->bus);
- if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
- /* if non zero is returned, the adress is not valid */
- i2c->sts &= ~IIC_STS_PT;
- i2c->sts |= IIC_STS_ERR;
- i2c->extsts |= IIC_EXTSTS_XFRA;
- } else {
- /*i2c->sts |= IIC_STS_PT;*/
- i2c->sts |= IIC_STS_MDBS;
- i2c->sts &= ~IIC_STS_ERR;
- i2c->extsts = 0;
- }
- }
- pending--;
- i2c->cntl = (i2c->cntl & 0xcf) | (pending << 4);
- }
- } else {
- qemu_log_mask(LOG_UNIMP, "[%s]%s: slave mode not implemented\n",
- TYPE_PPC4xx_I2C, __func__);
+ break;
+ }
+ ret = i2c->mdata[0];
+ if (i2c->mdidx == 3) {
+ i2c->sts &= ~IIC_STS_MDBF;
+ } else if (i2c->mdidx == 0) {
+ i2c->sts &= ~IIC_STS_MDBS;
+ }
+ for (i = 0; i < i2c->mdidx; i++) {
+ i2c->mdata[i] = i2c->mdata[i + 1];
+ }
+ if (i2c->mdidx >= 0) {
+ i2c->mdidx--;
}
break;
case 4:
@@ -179,6 +145,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
break;
case 9:
ret = i2c->extsts;
+ ret |= !!i2c_bus_busy(s->bus) << 4;
break;
case 10:
ret = i2c->lsadr;
@@ -223,70 +190,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
switch (addr) {
case 0:
- i2c->mdata = value;
- if (!i2c_bus_busy(s->bus)) {
- /* assume we start a write transfer */
- if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 0)) {
- /* if non zero is returned, the adress is not valid */
- i2c->sts &= ~IIC_STS_PT;
- i2c->sts |= IIC_STS_ERR;
- i2c->extsts |= IIC_EXTSTS_XFRA;
- } else {
- i2c->sts |= IIC_STS_PT;
- i2c->sts &= ~IIC_STS_ERR;
- i2c->extsts = 0;
- }
+ if (i2c->mdidx >= 3) {
+ break;
}
- if (i2c_bus_busy(s->bus)) {
- if (i2c_send(s->bus, i2c->mdata)) {
- /* if the target return non zero then end the transfer */
- i2c->sts &= ~IIC_STS_PT;
- i2c->sts |= IIC_STS_ERR;
- i2c->extsts |= IIC_EXTSTS_XFRA;
- i2c_end_transfer(s->bus);
- }
+ i2c->mdata[++i2c->mdidx] = value;
+ if (i2c->mdidx == 3) {
+ i2c->sts |= IIC_STS_MDBF;
+ } else if (i2c->mdidx == 0) {
+ i2c->sts |= IIC_STS_MDBS;
}
break;
case 4:
i2c->lmadr = value;
- if (i2c_bus_busy(s->bus)) {
- i2c_end_transfer(s->bus);
- }
break;
case 5:
i2c->hmadr = value;
break;
case 6:
- i2c->cntl = value;
- if (i2c->cntl & IIC_CNTL_PT) {
- if (i2c->cntl & IIC_CNTL_READ) {
- if (i2c_bus_busy(s->bus)) {
- /* end previous transfer */
- i2c->sts &= ~IIC_STS_PT;
- i2c_end_transfer(s->bus);
+ i2c->cntl = value & 0xfe;
+ if (value & IIC_CNTL_AMD) {
+ qemu_log_mask(LOG_UNIMP, "%s: only 7 bit addresses supported\n",
+ __func__);
+ }
+ if (value & IIC_CNTL_HMT && i2c_bus_busy(s->bus)) {
+ i2c_end_transfer(s->bus);
+ if (i2c->mdcntl & IIC_MDCNTL_EINT &&
+ i2c->intrmsk & IIC_INTRMSK_EIHE) {
+ i2c->sts |= IIC_STS_IRQA;
+ qemu_irq_raise(s->irq);
+ }
+ } else if (value & IIC_CNTL_PT) {
+ int recv = (value & IIC_CNTL_READ) >> 1;
+ int tct = value >> 4 & 3;
+ int i;
+
+ if (recv && (i2c->lmadr >> 1) >= 0x50 && (i2c->lmadr >> 1) < 0x58) {
+ /* smbus emulation does not like multi byte reads w/o restart */
+ value |= IIC_CNTL_RPST;
+ }
+
+ for (i = 0; i <= tct; i++) {
+ if (!i2c_bus_busy(s->bus)) {
+ i2c->extsts = (1 << 6);
+ if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, recv)) {
+ i2c->sts |= IIC_STS_ERR;
+ i2c->extsts |= IIC_EXTSTS_XFRA;
+ break;
+ } else {
+ i2c->sts &= ~IIC_STS_ERR;
+ }
+ }
+ if (!(i2c->sts & IIC_STS_ERR) &&
+ i2c_send_recv(s->bus, &i2c->mdata[i], !recv)) {
+ i2c->sts |= IIC_STS_ERR;
+ i2c->extsts |= IIC_EXTSTS_XFRA;
+ break;
}
- if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
- /* if non zero is returned, the adress is not valid */
- i2c->sts &= ~IIC_STS_PT;
- i2c->sts |= IIC_STS_ERR;
- i2c->extsts |= IIC_EXTSTS_XFRA;
- } else {
- /*i2c->sts |= IIC_STS_PT;*/
- i2c->sts |= IIC_STS_MDBS;
- i2c->sts &= ~IIC_STS_ERR;
- i2c->extsts = 0;
+ if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
+ i2c_end_transfer(s->bus);
}
- } else {
- /* we actually already did the write transfer... */
- i2c->sts &= ~IIC_STS_PT;
+ }
+ i2c->xfrcnt = i;
+ i2c->mdidx = i - 1;
+ if (recv && i2c->mdidx >= 0) {
+ i2c->sts |= IIC_STS_MDBS;
+ }
+ if (recv && i2c->mdidx == 3) {
+ i2c->sts |= IIC_STS_MDBF;
+ }
+ if (i && i2c->mdcntl & IIC_MDCNTL_EINT &&
+ i2c->intrmsk & IIC_INTRMSK_EIMTC) {
+ i2c->sts |= IIC_STS_IRQA;
+ qemu_irq_raise(s->irq);
}
}
break;
case 7:
- i2c->mdcntl = value & 0xdf;
+ i2c->mdcntl = value & 0x3d;
+ if (value & IIC_MDCNTL_ESM) {
+ qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+ __func__);
+ }
+ if (value & IIC_MDCNTL_FMDB) {
+ i2c->mdidx = -1;
+ memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
+ i2c->sts &= ~(IIC_STS_MDBF | IIC_STS_MDBS);
+ }
break;
case 8:
i2c->sts &= ~(value & 0xa);
+ if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
+ qemu_irq_lower(s->irq);
+ }
break;
case 9:
i2c->extsts &= ~(value & 0x8f);
@@ -307,12 +302,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
i2c->xfrcnt = value & 0x77;
break;
case 15:
+ i2c->xtcntlss &= ~(value & 0xf0);
if (value & IIC_XTCNTLSS_SRST) {
/* Is it actually a full reset? U-Boot sets some regs before */
ppc4xx_i2c_reset(DEVICE(s));
break;
}
- i2c->xtcntlss = value;
break;
case 16:
i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 6/8] sam460ex: Add RTC device
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 8/8] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
` (5 subsequent siblings)
7 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
The Sam460ex has an M41T80 serial RTC chip on I2C bus 0 at address 0x68.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/sam460ex.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index bdc53d2..dc730cc 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -457,6 +457,7 @@ static void sam460ex_init(MachineState *machine)
object_property_set_bool(OBJECT(dev), true, "realized", NULL);
smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
g_free(smbus_eeprom_buf);
+ i2c_create_slave(i2c[0]->bus, "m41t80", 0x68);
dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
i2c[1] = PPC4xx_I2C(dev);
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
` (4 preceding siblings ...)
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 4/8] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
@ 2018-06-06 13:31 ` BALATON Zoltan
2018-06-06 14:09 ` Peter Maydell
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation BALATON Zoltan
7 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 13:31 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson, Peter Maydell
When writing a register that has read only bits besides reserved bits
we have to avoid changing read only bits that may have non zero
default values.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e47be99..7ec1434 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
switch (addr) {
case SM501_SYSTEM_CONTROL:
- s->system_control = value & 0xE300B8F7;
+ s->system_control |= value & 0xEF00B8F7;
break;
case SM501_MISC_CONTROL:
- s->misc_control = value & 0xFF7FFF20;
+ s->misc_control |= value & 0xFF7FFF10;
break;
case SM501_GPIO31_0_CONTROL:
s->gpio_31_0_control = value;
@@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
s->dram_control |= value & 0x7FFFFFC3;
break;
case SM501_ARBTRTN_CONTROL:
- s->arbitration_control = value & 0x37777777;
+ s->arbitration_control = value & 0x37777777;
break;
case SM501_IRQ_MASK:
s->irq_mask = value;
--
2.7.6
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register BALATON Zoltan
@ 2018-06-06 14:09 ` Peter Maydell
2018-06-06 14:28 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2018-06-06 14:09 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: QEMU Developers, qemu-ppc, Alexander Graf, David Gibson
On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> When writing a register that has read only bits besides reserved bits
> we have to avoid changing read only bits that may have non zero
> default values.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index e47be99..7ec1434 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>
> switch (addr) {
> case SM501_SYSTEM_CONTROL:
> - s->system_control = value & 0xE300B8F7;
> + s->system_control |= value & 0xEF00B8F7;
This will mean that you can't clear a r/w bit by writing a
zero to it -- is that the hardware behaviour? The
description in the commit message suggests that you want
s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;
?
> break;
> case SM501_MISC_CONTROL:
> - s->misc_control = value & 0xFF7FFF20;
> + s->misc_control |= value & 0xFF7FFF10;
> break;
> case SM501_GPIO31_0_CONTROL:
> s->gpio_31_0_control = value;
> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
> s->dram_control |= value & 0x7FFFFFC3;
> break;
> case SM501_ARBTRTN_CONTROL:
> - s->arbitration_control = value & 0x37777777;
> + s->arbitration_control = value & 0x37777777;
Was this intended to be changed too?
> break;
> case SM501_IRQ_MASK:
> s->irq_mask = value;
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register
2018-06-06 14:09 ` Peter Maydell
@ 2018-06-06 14:28 ` BALATON Zoltan
2018-06-06 15:32 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 14:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, qemu-ppc, Alexander Graf, David Gibson
On Wed, 6 Jun 2018, Peter Maydell wrote:
> On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> When writing a register that has read only bits besides reserved bits
>> we have to avoid changing read only bits that may have non zero
>> default values.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/display/sm501.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index e47be99..7ec1434 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>>
>> switch (addr) {
>> case SM501_SYSTEM_CONTROL:
>> - s->system_control = value & 0xE300B8F7;
>> + s->system_control |= value & 0xEF00B8F7;
>
> This will mean that you can't clear a r/w bit by writing a
> zero to it -- is that the hardware behaviour? The
Not really sure about real hardware behaviour. I only have that not very
detailed docs we've looked at before but I assume r/w bits could be
changed by writing them.
> description in the commit message suggests that you want
> s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;
Wouldn't that always set ro bits? I need to leave ro bits untouched by
written value and only set rw bits. The previous version masked out ro
bits which also cleared them due to assigning with =. Maybe
s->whatever &= ro_bits_mask;
s->whatever |= value & rw_bits_mask;
would work? Could this be simplified?
>> break;
>> case SM501_MISC_CONTROL:
>> - s->misc_control = value & 0xFF7FFF20;
>> + s->misc_control |= value & 0xFF7FFF10;
>> break;
>> case SM501_GPIO31_0_CONTROL:
>> s->gpio_31_0_control = value;
>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>> s->dram_control |= value & 0x7FFFFFC3;
>> break;
>> case SM501_ARBTRTN_CONTROL:
>> - s->arbitration_control = value & 0x37777777;
>> + s->arbitration_control = value & 0x37777777;
>
> Was this intended to be changed too?
Yes, just a simple white space cleanup. Does that worth a separate patch
or enough to mention in commit message to make it clear it's intended
change?
Thanks for the quick review,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register
2018-06-06 14:28 ` BALATON Zoltan
@ 2018-06-06 15:32 ` Peter Maydell
2018-06-07 14:48 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2018-06-06 15:32 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: QEMU Developers, qemu-ppc, Alexander Graf, David Gibson
On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Wed, 6 Jun 2018, Peter Maydell wrote:
>>
>> On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> When writing a register that has read only bits besides reserved bits
>>> we have to avoid changing read only bits that may have non zero
>>> default values.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/display/sm501.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index e47be99..7ec1434 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque,
>>> hwaddr addr,
>>>
>>> switch (addr) {
>>> case SM501_SYSTEM_CONTROL:
>>> - s->system_control = value & 0xE300B8F7;
>>> + s->system_control |= value & 0xEF00B8F7;
>>
>>
>> This will mean that you can't clear a r/w bit by writing a
>> zero to it -- is that the hardware behaviour? The
>
>
> Not really sure about real hardware behaviour. I only have that not very
> detailed docs we've looked at before but I assume r/w bits could be changed
> by writing them.
>
>> description in the commit message suggests that you want
>> s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;
>
>
> Wouldn't that always set ro bits?
It will enforce that the RO 1 bits are always 1 (by
ORing those bits in). It's not the only way to do it.
(NB we OR the "ro_one_bits", not "ro_bits".)
>I need to leave ro bits untouched by
> written value and only set rw bits. The previous version masked out ro bits
> which also cleared them due to assigning with =. Maybe
>
> s->whatever &= ro_bits_mask;
> s->whatever |= value & rw_bits_mask;
Yes, this works (and I have a feeling it's what we tend to use.)
>>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque,
>>> hwaddr addr,
>>> s->dram_control |= value & 0x7FFFFFC3;
Hmm, does dram_control also need to behave the same way?
>>> break;
>>> case SM501_ARBTRTN_CONTROL:
>>> - s->arbitration_control = value & 0x37777777;
>>> + s->arbitration_control = value & 0x37777777;
>>
>>
>> Was this intended to be changed too?
>
>
> Yes, just a simple white space cleanup. Does that worth a separate patch or
> enough to mention in commit message to make it clear it's intended change?
I would personally put it in its own patch, but if you put it
in this one do mention it in the commit message. Otherwise
given the patch is changing four 'foo_control' registers it
looks like this one was typoed. (It will look less so if
the change for the others isn't just "add a '|'", though.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging BALATON Zoltan
@ 2018-06-06 15:56 ` Philippe Mathieu-Daudé
2018-06-08 8:50 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-06 15:56 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> Make it more readable by converting register indexes to decimal
> (avoids lot of superfluous 0x0) and distinguish errors caused by
> accessing non-existent vs. unimplemented registers.
> No functional change.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
> 1 file changed, 51 insertions(+), 43 deletions(-)
>
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index ab64d19..d1936db 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -31,7 +31,7 @@
> #include "hw/hw.h"
> #include "hw/i2c/ppc4xx_i2c.h"
>
> -#define PPC4xx_I2C_MEM_SIZE 0x12
> +#define PPC4xx_I2C_MEM_SIZE 18
This looks weird, it seems all memory range sizes are in hex in other
QEMU devices.
>
> #define IIC_CNTL_PT (1 << 0)
> #define IIC_CNTL_READ (1 << 1)
> @@ -70,7 +70,7 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> i2c->intrmsk = 0;
> i2c->xfrcnt = 0;
> i2c->xtcntlss = 0;
> - i2c->directcntl = 0x0f;
> + i2c->directcntl = 0xf;
> i2c->intr = 0;
> }
>
> @@ -85,7 +85,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> uint64_t ret;
>
> switch (addr) {
> - case 0x00:
> + case 0:
> ret = i2c->mdata;
> if (ppc4xx_i2c_is_master(i2c)) {
> ret = 0xff;
> @@ -139,58 +139,62 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> TYPE_PPC4xx_I2C, __func__);
> }
> break;
> - case 0x02:
> + case 2:
> ret = i2c->sdata;
> break;
> - case 0x04:
> + case 4:
> ret = i2c->lmadr;
> break;
> - case 0x05:
> + case 5:
> ret = i2c->hmadr;
> break;
> - case 0x06:
> + case 6:
> ret = i2c->cntl;
> break;
> - case 0x07:
> + case 7:
> ret = i2c->mdcntl;
> break;
> - case 0x08:
> + case 8:
> ret = i2c->sts;
> break;
> - case 0x09:
> + case 9:
> ret = i2c->extsts;
> break;
> - case 0x0A:
> + case 10:
> ret = i2c->lsadr;
> break;
> - case 0x0B:
> + case 11:
> ret = i2c->hsadr;
> break;
> - case 0x0C:
> + case 12:
> ret = i2c->clkdiv;
> break;
> - case 0x0D:
> + case 13:
> ret = i2c->intrmsk;
> break;
> - case 0x0E:
> + case 14:
> ret = i2c->xfrcnt;
> break;
> - case 0x0F:
> + case 15:
> ret = i2c->xtcntlss;
> break;
> - case 0x10:
> + case 16:
> ret = i2c->directcntl;
> break;
> - case 0x11:
> + case 17:
> ret = i2c->intr;
> break;
> default:
> - qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
> - HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
> + if (addr < PPC4xx_I2C_MEM_SIZE) {
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> + HWADDR_PRIx "\n", __func__, addr);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
> + HWADDR_PRIx "\n", __func__, addr);
> + }
> ret = 0;
> break;
> }
> -
> return ret;
> }
>
> @@ -200,7 +204,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> PPC4xxI2CState *i2c = opaque;
>
> switch (addr) {
> - case 0x00:
> + case 0:
> i2c->mdata = value;
> if (!i2c_bus_busy(i2c->bus)) {
> /* assume we start a write transfer */
> @@ -225,19 +229,19 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> }
> }
> break;
> - case 0x02:
> + case 2:
> i2c->sdata = value;
> break;
> - case 0x04:
> + case 4:
> i2c->lmadr = value;
> if (i2c_bus_busy(i2c->bus)) {
> i2c_end_transfer(i2c->bus);
> }
> break;
> - case 0x05:
> + case 5:
> i2c->hmadr = value;
> break;
> - case 0x06:
> + case 6:
> i2c->cntl = value;
> if (i2c->cntl & IIC_CNTL_PT) {
> if (i2c->cntl & IIC_CNTL_READ) {
> @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> }
> }
> break;
> - case 0x07:
> - i2c->mdcntl = value & 0xDF;
> + case 7:
> + i2c->mdcntl = value & 0xdf;
> break;
> - case 0x08:
> - i2c->sts &= ~(value & 0x0A);
> + case 8:
> + i2c->sts &= ~(value & 0xa);
'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
Matter of taste...
> break;
> - case 0x09:
> - i2c->extsts &= ~(value & 0x8F);
> + case 9:
> + i2c->extsts &= ~(value & 0x8f);
> break;
> - case 0x0A:
> + case 10:
> i2c->lsadr = value;
> - /*i2c_set_slave_address(i2c->bus, i2c->lsadr);*/
> break;
> - case 0x0B:
> + case 11:
> i2c->hsadr = value;
> break;
> - case 0x0C:
> + case 12:
> i2c->clkdiv = value;
> break;
> - case 0x0D:
> + case 13:
> i2c->intrmsk = value;
> break;
> - case 0x0E:
> + case 14:
> i2c->xfrcnt = value & 0x77;
> break;
> - case 0x0F:
> + case 15:
> if (value & IIC_XTCNTLSS_SRST) {
> /* Is it actually a full reset? U-Boot sets some regs before */
> ppc4xx_i2c_reset(DEVICE(i2c));
> @@ -296,15 +299,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> }
> i2c->xtcntlss = value;
> break;
> - case 0x10:
> + case 16:
> i2c->directcntl = value & 0x7;
> break;
> - case 0x11:
> + case 17:
> i2c->intr = value;
> break;
> default:
> - qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
> - HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
> + if (addr < PPC4xx_I2C_MEM_SIZE) {
> + qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> + HWADDR_PRIx "\n", __func__, addr);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
> + HWADDR_PRIx "\n", __func__, addr);
> + }
> break;
> }
> }
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation BALATON Zoltan
@ 2018-06-06 16:03 ` Philippe Mathieu-Daudé
2018-06-06 17:35 ` BALATON Zoltan
2018-06-08 12:42 ` Cédric Le Goater
1 sibling, 1 reply; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-06 16:03 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> of day is implemented. Setting time and RTC alarm are not supported.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> MAINTAINERS | 1 +
> default-configs/ppc-softmmu.mak | 1 +
> hw/timer/Makefile.objs | 1 +
> hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 120 insertions(+)
> create mode 100644 hw/timer/m41t80.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd373..9e13bc1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
> L: qemu-ppc@nongnu.org
> S: Maintained
> F: hw/ide/sii3112.c
> +F: hw/timer/m41t80.c
>
> SH4 Machines
> ------------
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 7d0dc2f..9fbaadc 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SM501=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> CONFIG_BITBANG_I2C=y
> +CONFIG_M41T80=y
>
> # For Macs
> CONFIG_MAC=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 8b27a4b..e16b2b9 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
> common-obj-$(CONFIG_DS1338) += ds1338.o
> common-obj-$(CONFIG_HPET) += hpet.o
> common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> +common-obj-$(CONFIG_M41T80) += m41t80.o
> common-obj-$(CONFIG_M48T59) += m48t59.o
> ifeq ($(CONFIG_ISA_BUS),y)
> common-obj-$(CONFIG_M48T59) += m48t59-isa.o
> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> new file mode 100644
> index 0000000..9dbdb1b
> --- /dev/null
> +++ b/hw/timer/m41t80.c
> @@ -0,0 +1,117 @@
> +/*
> + * M41T80 serial rtc emulation
> + *
> + * Copyright (c) 2018 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qemu/bcd.h"
> +#include "hw/i2c/i2c.h"
> +
> +#define TYPE_M41T80 "m41t80"
> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> +
> +typedef struct M41t80State {
> + I2CSlave parent_obj;
> + int8_t addr;
> +} M41t80State;
> +
> +static void m41t80_realize(DeviceState *dev, Error **errp)
> +{
> + M41t80State *s = M41T80(dev);
> +
> + s->addr = -1;
> +}
> +
> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> +{
> + M41t80State *s = M41T80(i2c);
> +
> + if (s->addr < 0) {
> + s->addr = data;
> + } else {
> + s->addr++;
> + }
What about adding enum i2c_event in M41t80State and use the enum here
rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
expected?
> + return 0;
> +}
> +
> +static int m41t80_recv(I2CSlave *i2c)
> +{
> + M41t80State *s = M41T80(i2c);
> + struct tm now;
> + qemu_timeval tv;
int8_t addr;
> +
> + if (s->addr < 0) {
> + s->addr = 0;
> + }
addr = s->addr++;
So you can simply use 'addr' bellow.
> + if (s->addr >= 1 && s->addr <= 7) {
> + qemu_get_timedate(&now, -1);
> + }
> + switch (s->addr++) {
> + case 0:
> + qemu_gettimeofday(&tv);
> + return to_bcd(tv.tv_usec / 10000);
> + case 1:
> + return to_bcd(now.tm_sec);
> + case 2:
> + return to_bcd(now.tm_min);
> + case 3:
> + return to_bcd(now.tm_hour);
> + case 4:
> + return to_bcd(now.tm_wday);
> + case 5:
> + return to_bcd(now.tm_mday);
> + case 6:
> + return to_bcd(now.tm_mon + 1);
> + case 7:
> + return to_bcd(now.tm_year % 100);
> + case 8 ... 19:
> + qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
> + __func__, s->addr - 1);
> + return 0;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
> + __func__, s->addr - 1);
> + return 0;
> + }
> +}
> +
> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
> +{
> + M41t80State *s = M41T80(i2c);
> +
> + if (event == I2C_START_SEND) {
> + s->addr = -1;
> + }
> + return 0;
> +}
> +
> +static void m41t80_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +
> + dc->realize = m41t80_realize;
> + sc->send = m41t80_send;
> + sc->recv = m41t80_recv;
> + sc->event = m41t80_event;
> +}
> +
> +static const TypeInfo m41t80_info = {
> + .name = TYPE_M41T80,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_size = sizeof(M41t80State),
> + .class_init = m41t80_class_init,
> +};
> +
> +static void m41t80_register_types(void)
> +{
> + type_register_static(&m41t80_info);
> +}
> +
> +type_init(m41t80_register_types)
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-06 16:03 ` Philippe Mathieu-Daudé
@ 2018-06-06 17:35 ` BALATON Zoltan
2018-06-13 4:11 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-06 17:35 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, qemu-ppc, Alexander Graf, David Gibson
On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>> of day is implemented. Setting time and RTC alarm are not supported.
[...]
>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>> new file mode 100644
>> index 0000000..9dbdb1b
>> --- /dev/null
>> +++ b/hw/timer/m41t80.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * M41T80 serial rtc emulation
>> + *
>> + * Copyright (c) 2018 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/bcd.h"
>> +#include "hw/i2c/i2c.h"
>> +
>> +#define TYPE_M41T80 "m41t80"
>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>> +
>> +typedef struct M41t80State {
>> + I2CSlave parent_obj;
>> + int8_t addr;
>> +} M41t80State;
>> +
>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>> +{
>> + M41t80State *s = M41T80(dev);
>> +
>> + s->addr = -1;
>> +}
>> +
>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>> +{
>> + M41t80State *s = M41T80(i2c);
>> +
>> + if (s->addr < 0) {
>> + s->addr = data;
>> + } else {
>> + s->addr++;
>> + }
>
> What about adding enum i2c_event in M41t80State and use the enum here
> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> expected?
Thanks for the review. I guess we could add enum for device bytes and the
special case -1 meaning no register address selected yet but this is a
very simple device with only 20 bytes and the datasheet also lists them by
number without naming them so I think we can also refer to them by number.
Since the device has only this 20 bytes the case with 127 should also not
be a problem as that's invalid address anyway. Or did you mean something
else?
>> + return 0;
>> +}
>> +
>> +static int m41t80_recv(I2CSlave *i2c)
>> +{
>> + M41t80State *s = M41T80(i2c);
>> + struct tm now;
>> + qemu_timeval tv;
>
> int8_t addr;
>
>> +
>> + if (s->addr < 0) {
>> + s->addr = 0;
>> + }
>
> addr = s->addr++;
>
> So you can simply use 'addr' bellow.
But that would mean numbers in switch cases below would be +1 compared to
how they are listed in the register map in the datasheet but now they
match so I'd leave it like this. (In case someone wants to check this with
the datasheet, the one I could find on-line says the range of register
number 4 (holding day of week) is 1-7 but U-Boot and clients expect 0-6 so
I think it's a bug in the datasheet and clients were tested on hardware.)
Regards,
BALATON Zoltan
>> + if (s->addr >= 1 && s->addr <= 7) {
>> + qemu_get_timedate(&now, -1);
>> + }
>> + switch (s->addr++) {
>> + case 0:
>> + qemu_gettimeofday(&tv);
>> + return to_bcd(tv.tv_usec / 10000);
>> + case 1:
>> + return to_bcd(now.tm_sec);
>> + case 2:
>> + return to_bcd(now.tm_min);
>> + case 3:
>> + return to_bcd(now.tm_hour);
>> + case 4:
>> + return to_bcd(now.tm_wday);
>> + case 5:
>> + return to_bcd(now.tm_mday);
>> + case 6:
>> + return to_bcd(now.tm_mon + 1);
>> + case 7:
>> + return to_bcd(now.tm_year % 100);
>> + case 8 ... 19:
>> + qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>> + __func__, s->addr - 1);
>> + return 0;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
>> + __func__, s->addr - 1);
>> + return 0;
>> + }
>> +}
>> +
>> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> + M41t80State *s = M41T80(i2c);
>> +
>> + if (event == I2C_START_SEND) {
>> + s->addr = -1;
>> + }
>> + return 0;
>> +}
>> +
>> +static void m41t80_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
>> +
>> + dc->realize = m41t80_realize;
>> + sc->send = m41t80_send;
>> + sc->recv = m41t80_recv;
>> + sc->event = m41t80_event;
>> +}
>> +
>> +static const TypeInfo m41t80_info = {
>> + .name = TYPE_M41T80,
>> + .parent = TYPE_I2C_SLAVE,
>> + .instance_size = sizeof(M41t80State),
>> + .class_init = m41t80_class_init,
>> +};
>> +
>> +static void m41t80_register_types(void)
>> +{
>> + type_register_static(&m41t80_info);
>> +}
>> +
>> +type_init(m41t80_register_types)
>>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register
2018-06-06 15:32 ` Peter Maydell
@ 2018-06-07 14:48 ` BALATON Zoltan
0 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-07 14:48 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, qemu-ppc, Alexander Graf, David Gibson
On Wed, 6 Jun 2018, Peter Maydell wrote:
> On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> s->whatever &= ro_bits_mask;
>> s->whatever |= value & rw_bits_mask;
>
> Yes, this works (and I have a feeling it's what we tend to use.)
OK, I'll do this then in next respin.
>>>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque,
>>>> hwaddr addr,
>>>> s->dram_control |= value & 0x7FFFFFC3;
>
> Hmm, does dram_control also need to behave the same way?
It has one read only bit which we are not emulating so maybe not
significant but changed it as well for correctness and added some more
similar registers as well.
>>>> break;
>>>> case SM501_ARBTRTN_CONTROL:
>>>> - s->arbitration_control = value & 0x37777777;
>>>> + s->arbitration_control = value & 0x37777777;
>>>
>>>
>>> Was this intended to be changed too?
>>
>>
>> Yes, just a simple white space cleanup. Does that worth a separate patch or
>> enough to mention in commit message to make it clear it's intended change?
>
> I would personally put it in its own patch, but if you put it
> in this one do mention it in the commit message. Otherwise
> given the patch is changing four 'foo_control' registers it
> looks like this one was typoed. (It will look less so if
> the change for the others isn't just "add a '|'", though.)
I've left it in one patch to avoid having two many patches in the series
but added it to commit message together with dram_control which also had
a whitespace removed in same patch.
Before sending v3 I'm waiting if there are any more comments for other
patches in this series
(http://patchew.org/QEMU/cover.1528291908.git.balaton@eik.bme.hu/) but
looks like David is busy and could not look at them yet. Not sure if
you've seen these patches but if you have a moment your comments are
welcome. If no other changes are suggested I'll submit v3 in the coming
days.
Thank you,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging
2018-06-06 15:56 ` Philippe Mathieu-Daudé
@ 2018-06-08 8:50 ` David Gibson
2018-06-08 9:11 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-08 8:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: BALATON Zoltan, qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]
On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > Make it more readable by converting register indexes to decimal
> > (avoids lot of superfluous 0x0) and distinguish errors caused by
> > accessing non-existent vs. unimplemented registers.
> > No functional change.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> > hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
> > 1 file changed, 51 insertions(+), 43 deletions(-)
> >
> > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > index ab64d19..d1936db 100644
> > --- a/hw/i2c/ppc4xx_i2c.c
> > +++ b/hw/i2c/ppc4xx_i2c.c
> > @@ -31,7 +31,7 @@
> > #include "hw/hw.h"
> > #include "hw/i2c/ppc4xx_i2c.h"
> >
> > -#define PPC4xx_I2C_MEM_SIZE 0x12
> > +#define PPC4xx_I2C_MEM_SIZE 18
>
> This looks weird, it seems all memory range sizes are in hex in other
> QEMU devices.
[snip]
> > @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > }
> > }
> > break;
> > - case 0x07:
> > - i2c->mdcntl = value & 0xDF;
> > + case 7:
> > + i2c->mdcntl = value & 0xdf;
> > break;
> > - case 0x08:
> > - i2c->sts &= ~(value & 0x0A);
> > + case 8:
> > + i2c->sts &= ~(value & 0xa);
>
> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
> Matter of taste...
I tend to prefer the forms you suggest, Philippe, but not by enough to
delay this otherwise good cleanup. Especially since Balaton is taking
on this long neglected area of the code.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers BALATON Zoltan
@ 2018-06-08 8:56 ` David Gibson
2018-06-08 9:20 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-08 8:56 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 10339 bytes --]
On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
It's not clear to me why this is preferable to having the registers
embedded in the state structure. The latter is pretty standard
practice for qemu.
> ---
> hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
> include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
> 2 files changed, 43 insertions(+), 51 deletions(-)
>
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d1936db..a68b5f7 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -3,7 +3,7 @@
> *
> * Copyright (c) 2007 Jocelyn Mayer
> * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
> *
> * Permission is hereby granted, free of charge, to any person obtaining a copy
> * of this software and associated documentation files (the "Software"), to deal
> @@ -46,9 +46,26 @@
>
> #define IIC_XTCNTLSS_SRST (1 << 0)
>
> +typedef struct {
> + uint8_t mdata;
> + uint8_t lmadr;
> + uint8_t hmadr;
> + uint8_t cntl;
> + uint8_t mdcntl;
> + uint8_t sts;
> + uint8_t extsts;
> + uint8_t lsadr;
> + uint8_t hsadr;
> + uint8_t clkdiv;
> + uint8_t intrmsk;
> + uint8_t xfrcnt;
> + uint8_t xtcntlss;
> + uint8_t directcntl;
> +} PPC4xxI2CRegs;
> +
> static void ppc4xx_i2c_reset(DeviceState *s)
> {
> - PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> + PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
>
> /* FIXME: Should also reset bus?
> *if (s->address != ADDR_RESET) {
> @@ -63,7 +80,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> i2c->mdcntl = 0;
> i2c->sts = 0;
> i2c->extsts = 0x8f;
> - i2c->sdata = 0;
> i2c->lsadr = 0;
> i2c->hsadr = 0;
> i2c->clkdiv = 0;
> @@ -71,7 +87,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> i2c->xfrcnt = 0;
> i2c->xtcntlss = 0;
> i2c->directcntl = 0xf;
> - i2c->intr = 0;
> }
>
> static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> @@ -81,13 +96,14 @@ static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
>
> static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> {
> - PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
> + PPC4xxI2CState *s = PPC4xx_I2C(opaque);
> + PPC4xxI2CRegs *i2c = s->regs;
> uint64_t ret;
>
> switch (addr) {
> case 0:
> ret = i2c->mdata;
> - if (ppc4xx_i2c_is_master(i2c)) {
> + if (ppc4xx_i2c_is_master(s)) {
> ret = 0xff;
>
> if (!(i2c->sts & IIC_STS_MDBS)) {
> @@ -98,7 +114,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> int pending = (i2c->cntl >> 4) & 3;
>
> /* get the next byte */
> - int byte = i2c_recv(i2c->bus);
> + int byte = i2c_recv(s->bus);
>
> if (byte < 0) {
> qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
> @@ -113,13 +129,13 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>
> if (!pending) {
> i2c->sts &= ~IIC_STS_MDBS;
> - /*i2c_end_transfer(i2c->bus);*/
> + /*i2c_end_transfer(s->bus);*/
> /*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
> } else if (pending) {
> /* current smbus implementation doesn't like
> multibyte xfer repeated start */
> - i2c_end_transfer(i2c->bus);
> - if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> + i2c_end_transfer(s->bus);
> + if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
> /* if non zero is returned, the adress is not valid */
> i2c->sts &= ~IIC_STS_PT;
> i2c->sts |= IIC_STS_ERR;
> @@ -139,9 +155,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> TYPE_PPC4xx_I2C, __func__);
> }
> break;
> - case 2:
> - ret = i2c->sdata;
> - break;
> case 4:
> ret = i2c->lmadr;
> break;
> @@ -181,9 +194,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> case 16:
> ret = i2c->directcntl;
> break;
> - case 17:
> - ret = i2c->intr;
> - break;
> default:
> if (addr < PPC4xx_I2C_MEM_SIZE) {
> qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -201,14 +211,15 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> unsigned int size)
> {
> - PPC4xxI2CState *i2c = opaque;
> + PPC4xxI2CState *s = PPC4xx_I2C(opaque);
> + PPC4xxI2CRegs *i2c = s->regs;
>
> switch (addr) {
> case 0:
> i2c->mdata = value;
> - if (!i2c_bus_busy(i2c->bus)) {
> + if (!i2c_bus_busy(s->bus)) {
> /* assume we start a write transfer */
> - if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 0)) {
> + if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 0)) {
> /* if non zero is returned, the adress is not valid */
> i2c->sts &= ~IIC_STS_PT;
> i2c->sts |= IIC_STS_ERR;
> @@ -219,23 +230,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> i2c->extsts = 0;
> }
> }
> - if (i2c_bus_busy(i2c->bus)) {
> - if (i2c_send(i2c->bus, i2c->mdata)) {
> + if (i2c_bus_busy(s->bus)) {
> + if (i2c_send(s->bus, i2c->mdata)) {
> /* if the target return non zero then end the transfer */
> i2c->sts &= ~IIC_STS_PT;
> i2c->sts |= IIC_STS_ERR;
> i2c->extsts |= IIC_EXTSTS_XFRA;
> - i2c_end_transfer(i2c->bus);
> + i2c_end_transfer(s->bus);
> }
> }
> break;
> - case 2:
> - i2c->sdata = value;
> - break;
> case 4:
> i2c->lmadr = value;
> - if (i2c_bus_busy(i2c->bus)) {
> - i2c_end_transfer(i2c->bus);
> + if (i2c_bus_busy(s->bus)) {
> + i2c_end_transfer(s->bus);
> }
> break;
> case 5:
> @@ -245,12 +253,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> i2c->cntl = value;
> if (i2c->cntl & IIC_CNTL_PT) {
> if (i2c->cntl & IIC_CNTL_READ) {
> - if (i2c_bus_busy(i2c->bus)) {
> + if (i2c_bus_busy(s->bus)) {
> /* end previous transfer */
> i2c->sts &= ~IIC_STS_PT;
> - i2c_end_transfer(i2c->bus);
> + i2c_end_transfer(s->bus);
> }
> - if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> + if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
> /* if non zero is returned, the adress is not valid */
> i2c->sts &= ~IIC_STS_PT;
> i2c->sts |= IIC_STS_ERR;
> @@ -294,7 +302,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> case 15:
> if (value & IIC_XTCNTLSS_SRST) {
> /* Is it actually a full reset? U-Boot sets some regs before */
> - ppc4xx_i2c_reset(DEVICE(i2c));
> + ppc4xx_i2c_reset(DEVICE(s));
> break;
> }
> i2c->xtcntlss = value;
> @@ -302,9 +310,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> case 16:
> i2c->directcntl = value & 0x7;
> break;
> - case 17:
> - i2c->intr = value;
> - break;
> default:
> if (addr < PPC4xx_I2C_MEM_SIZE) {
> qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
> static void ppc4xx_i2c_init(Object *o)
> {
> PPC4xxI2CState *s = PPC4xx_I2C(o);
> + PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
>
> + s->regs = r;
> memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
> TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index 3c60307..1d5ba0c 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -3,7 +3,7 @@
> *
> * Copyright (c) 2007 Jocelyn Mayer
> * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
> *
> * Permission is hereby granted, free of charge, to any person obtaining a copy
> * of this software and associated documentation files (the "Software"), to deal
> @@ -37,27 +37,12 @@
> typedef struct PPC4xxI2CState {
> /*< private >*/
> SysBusDevice parent_obj;
> + void *regs;
>
> /*< public >*/
> I2CBus *bus;
> qemu_irq irq;
> MemoryRegion iomem;
> - uint8_t mdata;
> - uint8_t lmadr;
> - uint8_t hmadr;
> - uint8_t cntl;
> - uint8_t mdcntl;
> - uint8_t sts;
> - uint8_t extsts;
> - uint8_t sdata;
> - uint8_t lsadr;
> - uint8_t hsadr;
> - uint8_t clkdiv;
> - uint8_t intrmsk;
> - uint8_t xfrcnt;
> - uint8_t xtcntlss;
> - uint8_t directcntl;
> - uint8_t intr;
> } PPC4xxI2CState;
>
> #endif /* PPC4XX_I2C_H */
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging
2018-06-08 8:50 ` David Gibson
@ 2018-06-08 9:11 ` BALATON Zoltan
0 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-08 9:11 UTC (permalink / raw)
To: David Gibson
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Alexander Graf
On Fri, 8 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>> Make it more readable by converting register indexes to decimal
>>> (avoids lot of superfluous 0x0) and distinguish errors caused by
>>> accessing non-existent vs. unimplemented registers.
>>> No functional change.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
>>> 1 file changed, 51 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>> index ab64d19..d1936db 100644
>>> --- a/hw/i2c/ppc4xx_i2c.c
>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>> @@ -31,7 +31,7 @@
>>> #include "hw/hw.h"
>>> #include "hw/i2c/ppc4xx_i2c.h"
>>>
>>> -#define PPC4xx_I2C_MEM_SIZE 0x12
>>> +#define PPC4xx_I2C_MEM_SIZE 18
>>
>> This looks weird, it seems all memory range sizes are in hex in other
>> QEMU devices.
>
> [snip]
>>> @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>> }
>>> }
>>> break;
>>> - case 0x07:
>>> - i2c->mdcntl = value & 0xDF;
>>> + case 7:
>>> + i2c->mdcntl = value & 0xdf;
>>> break;
>>> - case 0x08:
>>> - i2c->sts &= ~(value & 0x0A);
>>> + case 8:
>>> + i2c->sts &= ~(value & 0xa);
>>
>> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
>> Matter of taste...
>
> I tend to prefer the forms you suggest, Philippe, but not by enough to
> delay this otherwise good cleanup. Especially since Balaton is taking
> on this long neglected area of the code.
I prefer code that does not have unneeded chars so it's easier to read,
that's why I've removed all 0x0 from this file which made it more
comprehensible. But I'll add the 0 back to this place in next respin as
you both seem to prefer that and since other bit masks are two digit too
it's also consistent that way.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-08 8:56 ` David Gibson
@ 2018-06-08 9:20 ` BALATON Zoltan
2018-06-13 1:20 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-08 9:20 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Fri, 8 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> It's not clear to me why this is preferable to having the registers
> embedded in the state structure. The latter is pretty standard
> practice for qemu.
Maybe it will be clearer after the next patch in the series. I needed a
place to store the bitbang_i2c_interface for the directcntl way of
accessing the i2c bus but I can't include bitbang_i2c.h from the public
header because it's a local header. So I needed a local extension to the
state struct. Once I have that then it's a good place to also store
private registers which are now defined in the same file so I don't have
to look up them in a different place. This seemed clearer to me and easier
to work with. Maybe the spliting of the rewrite did not make this clear.
One thing I'm not sure about though:
>> ---
>> hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
>> include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
>> 2 files changed, 43 insertions(+), 51 deletions(-)
>>
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index d1936db..a68b5f7 100644
>> --- a/hw/i2c/ppc4xx_i2c.c
>> +++ b/hw/i2c/ppc4xx_i2c.c
[...]
>> @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
>> static void ppc4xx_i2c_init(Object *o)
>> {
>> PPC4xxI2CState *s = PPC4xx_I2C(o);
>> + PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
>>
>> + s->regs = r;
>> memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
>> TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
I allocate memory here but I'm not sure if it should be g_free'd somewhere
and if so where? I was not able to detangle QOM object hierarchies and
there seems to be no good docs available or I haven't found them. (PCI
devices seem to have unrealize methods but this did not work for I2C
objects.)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation BALATON Zoltan
2018-06-06 16:03 ` Philippe Mathieu-Daudé
@ 2018-06-08 12:42 ` Cédric Le Goater
2018-06-08 16:16 ` BALATON Zoltan
1 sibling, 1 reply; 41+ messages in thread
From: Cédric Le Goater @ 2018-06-08 12:42 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: David Gibson
On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> of day is implemented. Setting time and RTC alarm are not supported.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> MAINTAINERS | 1 +
> default-configs/ppc-softmmu.mak | 1 +
> hw/timer/Makefile.objs | 1 +
> hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 120 insertions(+)
> create mode 100644 hw/timer/m41t80.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd373..9e13bc1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
> L: qemu-ppc@nongnu.org
> S: Maintained
> F: hw/ide/sii3112.c
> +F: hw/timer/m41t80.c
>
> SH4 Machines
> ------------
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 7d0dc2f..9fbaadc 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SM501=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> CONFIG_BITBANG_I2C=y
> +CONFIG_M41T80=y
>
> # For Macs
> CONFIG_MAC=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 8b27a4b..e16b2b9 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
> common-obj-$(CONFIG_DS1338) += ds1338.o
> common-obj-$(CONFIG_HPET) += hpet.o
> common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
> +common-obj-$(CONFIG_M41T80) += m41t80.o
> common-obj-$(CONFIG_M48T59) += m48t59.o
> ifeq ($(CONFIG_ISA_BUS),y)
> common-obj-$(CONFIG_M48T59) += m48t59-isa.o
> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> new file mode 100644
> index 0000000..9dbdb1b
> --- /dev/null
> +++ b/hw/timer/m41t80.c
> @@ -0,0 +1,117 @@
> +/*
> + * M41T80 serial rtc emulation
> + *
> + * Copyright (c) 2018 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qemu/bcd.h"
> +#include "hw/i2c/i2c.h"
> +
> +#define TYPE_M41T80 "m41t80"
> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> +
> +typedef struct M41t80State {
> + I2CSlave parent_obj;
> + int8_t addr;
> +} M41t80State;
> +
> +static void m41t80_realize(DeviceState *dev, Error **errp)
> +{
> + M41t80State *s = M41T80(dev);
> +
> + s->addr = -1;
> +}
> +
> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> +{
> + M41t80State *s = M41T80(i2c);
> +
> + if (s->addr < 0) {
> + s->addr = data;
> + } else {
> + s->addr++;
> + }
> + return 0;
> +}
> +
> +static int m41t80_recv(I2CSlave *i2c)
> +{
> + M41t80State *s = M41T80(i2c);
> + struct tm now;
> + qemu_timeval tv;
> +
> + if (s->addr < 0) {
> + s->addr = 0;
> + }
> + if (s->addr >= 1 && s->addr <= 7) {
> + qemu_get_timedate(&now, -1);
> + }
> + switch (s->addr++) {
you could use some define to name the registers :
> + case 0:
> + qemu_gettimeofday(&tv);
> + return to_bcd(tv.tv_usec / 10000);> + case 1:
> + return to_bcd(now.tm_sec);
> + case 2:
> + return to_bcd(now.tm_min);
> + case 3:
> + return to_bcd(now.tm_hour);
There is an interesting century bit in specs.
> + case 4:
> + return to_bcd(now.tm_wday);
> + case 5:
> + return to_bcd(now.tm_mday);
> + case 6:
> + return to_bcd(now.tm_mon + 1);
> + case 7:
> + return to_bcd(now.tm_year % 100);
> + case 8 ... 19:
> + qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
is the beginning \n needed ?
Thanks,
C.
> + __func__, s->addr - 1);
> + return 0;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
> + __func__, s->addr - 1);
> + return 0;
> + }
> +}
> +
> +static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
> +{
> + M41t80State *s = M41T80(i2c);
> +
> + if (event == I2C_START_SEND) {
> + s->addr = -1;
> + }
> + return 0;
> +}
> +
> +static void m41t80_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> +
> + dc->realize = m41t80_realize;
> + sc->send = m41t80_send;
> + sc->recv = m41t80_recv;
> + sc->event = m41t80_event;
> +}
> +
> +static const TypeInfo m41t80_info = {
> + .name = TYPE_M41T80,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_size = sizeof(M41t80State),
> + .class_init = m41t80_class_init,
> +};
> +
> +static void m41t80_register_types(void)
> +{
> + type_register_static(&m41t80_info);
> +}
> +
> +type_init(m41t80_register_types)
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-08 12:42 ` Cédric Le Goater
@ 2018-06-08 16:16 ` BALATON Zoltan
2018-06-08 17:49 ` Cédric Le Goater
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-08 16:16 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel, qemu-ppc, David Gibson
On Fri, 8 Jun 2018, Cédric Le Goater wrote:
> On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>> of day is implemented. Setting time and RTC alarm are not supported.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> MAINTAINERS | 1 +
>> default-configs/ppc-softmmu.mak | 1 +
>> hw/timer/Makefile.objs | 1 +
>> hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 120 insertions(+)
>> create mode 100644 hw/timer/m41t80.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 41cd373..9e13bc1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>> L: qemu-ppc@nongnu.org
>> S: Maintained
>> F: hw/ide/sii3112.c
>> +F: hw/timer/m41t80.c
>>
>> SH4 Machines
>> ------------
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 7d0dc2f..9fbaadc 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>> CONFIG_IDE_SII3112=y
>> CONFIG_I2C=y
>> CONFIG_BITBANG_I2C=y
>> +CONFIG_M41T80=y
>>
>> # For Macs
>> CONFIG_MAC=y
>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>> index 8b27a4b..e16b2b9 100644
>> --- a/hw/timer/Makefile.objs
>> +++ b/hw/timer/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>> common-obj-$(CONFIG_DS1338) += ds1338.o
>> common-obj-$(CONFIG_HPET) += hpet.o
>> common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>> +common-obj-$(CONFIG_M41T80) += m41t80.o
>> common-obj-$(CONFIG_M48T59) += m48t59.o
>> ifeq ($(CONFIG_ISA_BUS),y)
>> common-obj-$(CONFIG_M48T59) += m48t59-isa.o
>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>> new file mode 100644
>> index 0000000..9dbdb1b
>> --- /dev/null
>> +++ b/hw/timer/m41t80.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * M41T80 serial rtc emulation
>> + *
>> + * Copyright (c) 2018 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/bcd.h"
>> +#include "hw/i2c/i2c.h"
>> +
>> +#define TYPE_M41T80 "m41t80"
>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>> +
>> +typedef struct M41t80State {
>> + I2CSlave parent_obj;
>> + int8_t addr;
>> +} M41t80State;
>> +
>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>> +{
>> + M41t80State *s = M41T80(dev);
>> +
>> + s->addr = -1;
>> +}
>> +
>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>> +{
>> + M41t80State *s = M41T80(i2c);
>> +
>> + if (s->addr < 0) {
>> + s->addr = data;
>> + } else {
>> + s->addr++;
>> + }
>> + return 0;
>> +}
>> +
>> +static int m41t80_recv(I2CSlave *i2c)
>> +{
>> + M41t80State *s = M41T80(i2c);
>> + struct tm now;
>> + qemu_timeval tv;
>> +
>> + if (s->addr < 0) {
>> + s->addr = 0;
>> + }
>> + if (s->addr >= 1 && s->addr <= 7) {
>> + qemu_get_timedate(&now, -1);
>> + }
>> + switch (s->addr++) {
>
> you could use some define to name the registers :
This was also suggested by Philippe Mathieu-Daudé and my answer to that
was that I don't feel like I want to come up with names the datasheet does
not have either. I think this device is simple enough with just 20
consecutively numbered registers that appear only in these switch cases by
number as in the datasheet table so that I don't want to make it more
difficult to read by encrypting these numbers behind some arbitrary
defines without a good reason. They are also so simple that it's clear
from the usually one line implementation what they do so that's also not a
good reason to name them.
>> + case 0:
>> + qemu_gettimeofday(&tv);
>> + return to_bcd(tv.tv_usec / 10000);> + case 1:
>> + return to_bcd(now.tm_sec);
>> + case 2:
>> + return to_bcd(now.tm_min);
>> + case 3:
>> + return to_bcd(now.tm_hour);
>
> There is an interesting century bit in specs.
Which I could not figure out how should work and guests seem to be happy
without it so I did not try to implement it.
>> + case 4:
>> + return to_bcd(now.tm_wday);
>> + case 5:
>> + return to_bcd(now.tm_mday);
>> + case 6:
>> + return to_bcd(now.tm_mon + 1);
>> + case 7:
>> + return to_bcd(now.tm_year % 100);
>> + case 8 ... 19:
>> + qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>
> is the beginning \n needed ?
Probably not, maybe left there due to previous debug logs I've removed.
I'll drop the beginning \n-s in next version.
> Thanks,
>
> C.
Thanks for the review,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-08 16:16 ` BALATON Zoltan
@ 2018-06-08 17:49 ` Cédric Le Goater
0 siblings, 0 replies; 41+ messages in thread
From: Cédric Le Goater @ 2018-06-08 17:49 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, David Gibson
On 06/08/2018 06:16 PM, BALATON Zoltan wrote:
> On Fri, 8 Jun 2018, Cédric Le Goater wrote:
>> On 06/06/2018 03:31 PM, BALATON Zoltan wrote:
>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>> of day is implemented. Setting time and RTC alarm are not supported.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> MAINTAINERS | 1 +
>>> default-configs/ppc-softmmu.mak | 1 +
>>> hw/timer/Makefile.objs | 1 +
>>> hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 120 insertions(+)
>>> create mode 100644 hw/timer/m41t80.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 41cd373..9e13bc1 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -826,6 +826,7 @@ M: BALATON Zoltan <balaton@eik.bme.hu>
>>> L: qemu-ppc@nongnu.org
>>> S: Maintained
>>> F: hw/ide/sii3112.c
>>> +F: hw/timer/m41t80.c
>>>
>>> SH4 Machines
>>> ------------
>>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>>> index 7d0dc2f..9fbaadc 100644
>>> --- a/default-configs/ppc-softmmu.mak
>>> +++ b/default-configs/ppc-softmmu.mak
>>> @@ -27,6 +27,7 @@ CONFIG_SM501=y
>>> CONFIG_IDE_SII3112=y
>>> CONFIG_I2C=y
>>> CONFIG_BITBANG_I2C=y
>>> +CONFIG_M41T80=y
>>>
>>> # For Macs
>>> CONFIG_MAC=y
>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>>> index 8b27a4b..e16b2b9 100644
>>> --- a/hw/timer/Makefile.objs
>>> +++ b/hw/timer/Makefile.objs
>>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>>> common-obj-$(CONFIG_DS1338) += ds1338.o
>>> common-obj-$(CONFIG_HPET) += hpet.o
>>> common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>>> +common-obj-$(CONFIG_M41T80) += m41t80.o
>>> common-obj-$(CONFIG_M48T59) += m48t59.o
>>> ifeq ($(CONFIG_ISA_BUS),y)
>>> common-obj-$(CONFIG_M48T59) += m48t59-isa.o
>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>> new file mode 100644
>>> index 0000000..9dbdb1b
>>> --- /dev/null
>>> +++ b/hw/timer/m41t80.c
>>> @@ -0,0 +1,117 @@
>>> +/*
>>> + * M41T80 serial rtc emulation
>>> + *
>>> + * Copyright (c) 2018 BALATON Zoltan
>>> + *
>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qemu/timer.h"
>>> +#include "qemu/bcd.h"
>>> +#include "hw/i2c/i2c.h"
>>> +
>>> +#define TYPE_M41T80 "m41t80"
>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>> +
>>> +typedef struct M41t80State {
>>> + I2CSlave parent_obj;
>>> + int8_t addr;
>>> +} M41t80State;
>>> +
>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + M41t80State *s = M41T80(dev);
>>> +
>>> + s->addr = -1;
>>> +}
>>> +
>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>> +{
>>> + M41t80State *s = M41T80(i2c);
>>> +
>>> + if (s->addr < 0) {
>>> + s->addr = data;
>>> + } else {
>>> + s->addr++;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int m41t80_recv(I2CSlave *i2c)
>>> +{
>>> + M41t80State *s = M41T80(i2c);
>>> + struct tm now;
>>> + qemu_timeval tv;
>>> +
>>> + if (s->addr < 0) {
>>> + s->addr = 0;
>>> + }
>>> + if (s->addr >= 1 && s->addr <= 7) {
>>> + qemu_get_timedate(&now, -1);
>>> + }
>>> + switch (s->addr++) {
>>
>> you could use some define to name the registers :
>
> This was also suggested by Philippe Mathieu-Daudé and my answer to that was that I don't feel like I want to come up with names the datasheet does not have either. I think this device is simple enough with just 20 consecutively numbered registers that appear only in these switch cases by number as in the datasheet table so that I don't want to make it more difficult to read by encrypting these numbers behind some arbitrary defines without a good reason. They are also so simple that it's clear from the usually one line implementation what they do so that's also not a good reason to name them.
OK. It's fine with me but you might get some inspiration from Linux
for the names :)
>>> + case 0:
>>> + qemu_gettimeofday(&tv);
>>> + return to_bcd(tv.tv_usec / 10000);> + case 1:
>>> + return to_bcd(now.tm_sec);
>>> + case 2:
>>> + return to_bcd(now.tm_min);
>>> + case 3:
>>> + return to_bcd(now.tm_hour);
>>
>> There is an interesting century bit in specs.
>
> Which I could not figure out how should work and guests seem to be happy without it so I did not try to implement it.
yes. It seems that Linux simply ignores it. Let's forget it.
Thanks,
C.
>>> + case 4:
>>> + return to_bcd(now.tm_wday);
>>> + case 5:
>>> + return to_bcd(now.tm_mday);
>>> + case 6:
>>> + return to_bcd(now.tm_mon + 1);
>>> + case 7:
>>> + return to_bcd(now.tm_year % 100);
>>> + case 8 ... 19:
>>> + qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
>>
>> is the beginning \n needed ?
>
> Probably not, maybe left there due to previous debug logs I've removed. I'll drop the beginning \n-s in next version.
>
>> Thanks,
>>
>> C.
>
> Thanks for the review,
> BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-08 9:20 ` BALATON Zoltan
@ 2018-06-13 1:20 ` David Gibson
2018-06-13 8:56 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-13 1:20 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 3373 bytes --]
On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
> On Fri, 8 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > It's not clear to me why this is preferable to having the registers
> > embedded in the state structure. The latter is pretty standard
> > practice for qemu.
>
> Maybe it will be clearer after the next patch in the series. I needed a
> place to store the bitbang_i2c_interface for the directcntl way of accessing
> the i2c bus but I can't include bitbang_i2c.h from the public header because
> it's a local header. So I needed a local extension to the state struct. Once
> I have that then it's a good place to also store private registers which are
> now defined in the same file so I don't have to look up them in a different
> place. This seemed clearer to me and easier to work with. Maybe the spliting
> of the rewrite did not make this clear.
Oh.. right. There's a better way.
You can just forward declare the bitbang_i2c_interface structure like
this in your header:
typdef struct bitbang_i2c_interface bitbang_i2c_interface;
So you're declaring the existence of the structure, but not its
contents - that's sufficient to create a pointer to it. Then you
don't need to creat the substructure and extra level of indirection.
> One thing I'm not sure about though:
>
> > > ---
> > > hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
> > > include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
> > > 2 files changed, 43 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index d1936db..a68b5f7 100644
> > > --- a/hw/i2c/ppc4xx_i2c.c
> > > +++ b/hw/i2c/ppc4xx_i2c.c
> [...]
> > > @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
> > > static void ppc4xx_i2c_init(Object *o)
> > > {
> > > PPC4xxI2CState *s = PPC4xx_I2C(o);
> > > + PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
> > >
> > > + s->regs = r;
> > > memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
> > > TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
> > > sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>
> I allocate memory here but I'm not sure if it should be g_free'd somewhere
> and if so where? I was not able to detangle QOM object hierarchies and there
> seems to be no good docs available or I haven't found them. (PCI devices
> seem to have unrealize methods but this did not work for I2C objects.)
Yes, if you're allocating you definitely should be free()ing. It
should go in the corresponding cleanup routine to where it is
allocated. Since the allocation is in instance_init(), the free()
should be in instance_finalize() (which you'd need to add).
Except that the above should let you avoid that.
..and I guess this won't actually ever be finalized in practice.
..and there doesn't seem to be a way to free up a bitbang_interface,
so even if you added the finalize, it still wouldn't really clean up
properly.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
@ 2018-06-13 1:22 ` David Gibson
2018-06-13 8:54 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-13 1:22 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]
On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> default-configs/ppc-softmmu.mak | 1 +
> default-configs/ppcemb-softmmu.mak | 1 +
> hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 4d7be45..7d0dc2f 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> CONFIG_SM501=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> +CONFIG_BITBANG_I2C=y
>
> # For Macs
> CONFIG_MAC=y
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index 67d18b2..37af193 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> CONFIG_SM501=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> +CONFIG_BITBANG_I2C=y
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index a68b5f7..5806209 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -30,6 +30,7 @@
> #include "cpu.h"
> #include "hw/hw.h"
> #include "hw/i2c/ppc4xx_i2c.h"
> +#include "bitbang_i2c.h"
>
> #define PPC4xx_I2C_MEM_SIZE 18
>
> @@ -46,7 +47,13 @@
>
> #define IIC_XTCNTLSS_SRST (1 << 0)
>
> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> +
> typedef struct {
> + bitbang_i2c_interface *bitbang;
> uint8_t mdata;
> uint8_t lmadr;
> uint8_t hmadr;
> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> i2c->xtcntlss = value;
> break;
> case 16:
> - i2c->directcntl = value & 0x7;
> + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> break;
> default:
> if (addr < PPC4xx_I2C_MEM_SIZE) {
> @@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
> s->bus = i2c_init_bus(DEVICE(s), "i2c");
> + r->bitbang = bitbang_i2c_init(s->bus);
> }
>
> static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-06 17:35 ` BALATON Zoltan
@ 2018-06-13 4:11 ` David Gibson
2018-06-13 8:50 ` [Qemu-devel] [Qemu-ppc] " BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-13 4:11 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]
On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > of day is implemented. Setting time and RTC alarm are not supported.
> [...]
> > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > new file mode 100644
> > > index 0000000..9dbdb1b
> > > --- /dev/null
> > > +++ b/hw/timer/m41t80.c
> > > @@ -0,0 +1,117 @@
> > > +/*
> > > + * M41T80 serial rtc emulation
> > > + *
> > > + * Copyright (c) 2018 BALATON Zoltan
> > > + *
> > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/timer.h"
> > > +#include "qemu/bcd.h"
> > > +#include "hw/i2c/i2c.h"
> > > +
> > > +#define TYPE_M41T80 "m41t80"
> > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > +
> > > +typedef struct M41t80State {
> > > + I2CSlave parent_obj;
> > > + int8_t addr;
> > > +} M41t80State;
> > > +
> > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > +{
> > > + M41t80State *s = M41T80(dev);
> > > +
> > > + s->addr = -1;
> > > +}
> > > +
> > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > +{
> > > + M41t80State *s = M41T80(i2c);
> > > +
> > > + if (s->addr < 0) {
> > > + s->addr = data;
> > > + } else {
> > > + s->addr++;
> > > + }
> >
> > What about adding enum i2c_event in M41t80State and use the enum here
> > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > expected?
>
> Thanks for the review. I guess we could add enum for device bytes and the
> special case -1 meaning no register address selected yet but this is a very
> simple device with only 20 bytes and the datasheet also lists them by number
> without naming them so I think we can also refer to them by number. Since
> the device has only this 20 bytes the case with 127 should also not be a
> problem as that's invalid address anyway. Or did you mean something else?
So, I'm not particularly in favour of adding extra state variables.
But is using addr < 0 safe here? You're assigning the uint8_t data to
addr - could that result in a negative value?
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-13 4:11 ` David Gibson
@ 2018-06-13 8:50 ` BALATON Zoltan
2018-06-13 10:03 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-13 8:50 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
>>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>>> of day is implemented. Setting time and RTC alarm are not supported.
>> [...]
>>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>>> new file mode 100644
>>>> index 0000000..9dbdb1b
>>>> --- /dev/null
>>>> +++ b/hw/timer/m41t80.c
>>>> @@ -0,0 +1,117 @@
>>>> +/*
>>>> + * M41T80 serial rtc emulation
>>>> + *
>>>> + * Copyright (c) 2018 BALATON Zoltan
>>>> + *
>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qemu/timer.h"
>>>> +#include "qemu/bcd.h"
>>>> +#include "hw/i2c/i2c.h"
>>>> +
>>>> +#define TYPE_M41T80 "m41t80"
>>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>>> +
>>>> +typedef struct M41t80State {
>>>> + I2CSlave parent_obj;
>>>> + int8_t addr;
>>>> +} M41t80State;
>>>> +
>>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> + M41t80State *s = M41T80(dev);
>>>> +
>>>> + s->addr = -1;
>>>> +}
>>>> +
>>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>>> +{
>>>> + M41t80State *s = M41T80(i2c);
>>>> +
>>>> + if (s->addr < 0) {
>>>> + s->addr = data;
>>>> + } else {
>>>> + s->addr++;
>>>> + }
>>>
>>> What about adding enum i2c_event in M41t80State and use the enum here
>>> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
>>> expected?
>>
>> Thanks for the review. I guess we could add enum for device bytes and the
>> special case -1 meaning no register address selected yet but this is a very
>> simple device with only 20 bytes and the datasheet also lists them by number
>> without naming them so I think we can also refer to them by number. Since
>> the device has only this 20 bytes the case with 127 should also not be a
>> problem as that's invalid address anyway. Or did you mean something else?
>
> So, I'm not particularly in favour of adding extra state variables.
>
> But is using addr < 0 safe here? You're assigning the uint8_t data to
> addr - could that result in a negative value?
Why wouldn't it be safe with the expected values for register address
between 0-19? If the guest sends garbage values over 127 it will either
result in invalid register or unselected register and lead to an error
when trying to read/write that register so I don't see what other problem
this may cause.
The addr < 0 is to check if no address was selected before (on creating
the device and when sending first value from host addr is set to -1. In
this case first write will set register address, then subsequent
reads/writes increment register address as the datasheet says).
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-13 1:22 ` David Gibson
@ 2018-06-13 8:54 ` BALATON Zoltan
2018-06-13 10:03 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-13 8:54 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> default-configs/ppc-softmmu.mak | 1 +
>> default-configs/ppcemb-softmmu.mak | 1 +
>> hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index 4d7be45..7d0dc2f 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
>> CONFIG_SM501=y
>> CONFIG_IDE_SII3112=y
>> CONFIG_I2C=y
>> +CONFIG_BITBANG_I2C=y
>>
>> # For Macs
>> CONFIG_MAC=y
>> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
>> index 67d18b2..37af193 100644
>> --- a/default-configs/ppcemb-softmmu.mak
>> +++ b/default-configs/ppcemb-softmmu.mak
>> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
>> CONFIG_SM501=y
>> CONFIG_IDE_SII3112=y
>> CONFIG_I2C=y
>> +CONFIG_BITBANG_I2C=y
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index a68b5f7..5806209 100644
>> --- a/hw/i2c/ppc4xx_i2c.c
>> +++ b/hw/i2c/ppc4xx_i2c.c
>> @@ -30,6 +30,7 @@
>> #include "cpu.h"
>> #include "hw/hw.h"
>> #include "hw/i2c/ppc4xx_i2c.h"
>> +#include "bitbang_i2c.h"
>>
>> #define PPC4xx_I2C_MEM_SIZE 18
>>
>> @@ -46,7 +47,13 @@
>>
>> #define IIC_XTCNTLSS_SRST (1 << 0)
>>
>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>> +
>> typedef struct {
>> + bitbang_i2c_interface *bitbang;
>> uint8_t mdata;
>> uint8_t lmadr;
>> uint8_t hmadr;
>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>> i2c->xtcntlss = value;
>> break;
>> case 16:
>> - i2c->directcntl = value & 0x7;
>> + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>
> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>
>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>
> Last expression might be clearer as:
> value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
position in the register so I use that when accessing that bit but when I
check for the values of a bit being 0 or 1 I don't use the define which is
for something else, just happens to have value 1 as well.
If this does not explain your question and you think it's better to use
defines here I can change that in next version, please let me know.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-13 1:20 ` David Gibson
@ 2018-06-13 8:56 ` BALATON Zoltan
2018-06-13 10:01 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-13 8:56 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Wed, 13 Jun 2018, David Gibson wrote:
> On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
>> On Fri, 8 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>
>>> It's not clear to me why this is preferable to having the registers
>>> embedded in the state structure. The latter is pretty standard
>>> practice for qemu.
>>
>> Maybe it will be clearer after the next patch in the series. I needed a
>> place to store the bitbang_i2c_interface for the directcntl way of accessing
>> the i2c bus but I can't include bitbang_i2c.h from the public header because
>> it's a local header. So I needed a local extension to the state struct. Once
>> I have that then it's a good place to also store private registers which are
>> now defined in the same file so I don't have to look up them in a different
>> place. This seemed clearer to me and easier to work with. Maybe the spliting
>> of the rewrite did not make this clear.
>
> Oh.. right. There's a better way.
>
> You can just forward declare the bitbang_i2c_interface structure like
> this in your header:
> typdef struct bitbang_i2c_interface bitbang_i2c_interface;
>
> So you're declaring the existence of the structure, but not its
> contents - that's sufficient to create a pointer to it. Then you
> don't need to creat the substructure and extra level of indirection.
>
>> One thing I'm not sure about though:
>>
>>>> ---
>>>> hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
>>>> include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
>>>> 2 files changed, 43 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>> index d1936db..a68b5f7 100644
>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>> [...]
>>>> @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
>>>> static void ppc4xx_i2c_init(Object *o)
>>>> {
>>>> PPC4xxI2CState *s = PPC4xx_I2C(o);
>>>> + PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
>>>>
>>>> + s->regs = r;
>>>> memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
>>>> TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
>>>> sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>>
>> I allocate memory here but I'm not sure if it should be g_free'd somewhere
>> and if so where? I was not able to detangle QOM object hierarchies and there
>> seems to be no good docs available or I haven't found them. (PCI devices
>> seem to have unrealize methods but this did not work for I2C objects.)
>
> Yes, if you're allocating you definitely should be free()ing. It
> should go in the corresponding cleanup routine to where it is
> allocated. Since the allocation is in instance_init(), the free()
> should be in instance_finalize() (which you'd need to add).
>
> Except that the above should let you avoid that.
>
> ..and I guess this won't actually ever be finalized in practice.
>
> ..and there doesn't seem to be a way to free up a bitbang_interface,
> so even if you added the finalize, it still wouldn't really clean up
> properly.
Yes, I suspected it won't matter anyway. I'll try your suggestion to just
declare the bitbang_i2c_interface in the public header in next version.
Any more reviews to expect from you for other patches or should I send a
v3 with the changes so far?
Thank you,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
2018-06-13 8:56 ` BALATON Zoltan
@ 2018-06-13 10:01 ` David Gibson
0 siblings, 0 replies; 41+ messages in thread
From: David Gibson @ 2018-06-13 10:01 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 4042 bytes --]
On Wed, Jun 13, 2018 at 10:56:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
> > > On Fri, 8 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > >
> > > > It's not clear to me why this is preferable to having the registers
> > > > embedded in the state structure. The latter is pretty standard
> > > > practice for qemu.
> > >
> > > Maybe it will be clearer after the next patch in the series. I needed a
> > > place to store the bitbang_i2c_interface for the directcntl way of accessing
> > > the i2c bus but I can't include bitbang_i2c.h from the public header because
> > > it's a local header. So I needed a local extension to the state struct. Once
> > > I have that then it's a good place to also store private registers which are
> > > now defined in the same file so I don't have to look up them in a different
> > > place. This seemed clearer to me and easier to work with. Maybe the spliting
> > > of the rewrite did not make this clear.
> >
> > Oh.. right. There's a better way.
> >
> > You can just forward declare the bitbang_i2c_interface structure like
> > this in your header:
> > typdef struct bitbang_i2c_interface bitbang_i2c_interface;
> >
> > So you're declaring the existence of the structure, but not its
> > contents - that's sufficient to create a pointer to it. Then you
> > don't need to creat the substructure and extra level of indirection.
> >
> > > One thing I'm not sure about though:
> > >
> > > > > ---
> > > > > hw/i2c/ppc4xx_i2c.c | 75 +++++++++++++++++++++++++--------------------
> > > > > include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
> > > > > 2 files changed, 43 insertions(+), 51 deletions(-)
> > > > >
> > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > index d1936db..a68b5f7 100644
> > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > [...]
> > > > > @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
> > > > > static void ppc4xx_i2c_init(Object *o)
> > > > > {
> > > > > PPC4xxI2CState *s = PPC4xx_I2C(o);
> > > > > + PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
> > > > >
> > > > > + s->regs = r;
> > > > > memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
> > > > > TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
> > > > > sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> > >
> > > I allocate memory here but I'm not sure if it should be g_free'd somewhere
> > > and if so where? I was not able to detangle QOM object hierarchies and there
> > > seems to be no good docs available or I haven't found them. (PCI devices
> > > seem to have unrealize methods but this did not work for I2C objects.)
> >
> > Yes, if you're allocating you definitely should be free()ing. It
> > should go in the corresponding cleanup routine to where it is
> > allocated. Since the allocation is in instance_init(), the free()
> > should be in instance_finalize() (which you'd need to add).
> >
> > Except that the above should let you avoid that.
> >
> > ..and I guess this won't actually ever be finalized in practice.
> >
> > ..and there doesn't seem to be a way to free up a bitbang_interface,
> > so even if you added the finalize, it still wouldn't really clean up
> > properly.
>
> Yes, I suspected it won't matter anyway. I'll try your suggestion to just
> declare the bitbang_i2c_interface in the public header in next version.
>
> Any more reviews to expect from you for other patches or should I send a v3
> with the changes so far?
Go ahead with v3.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-13 8:54 ` BALATON Zoltan
@ 2018-06-13 10:03 ` David Gibson
2018-06-13 14:03 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-13 10:03 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]
On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > > default-configs/ppc-softmmu.mak | 1 +
> > > default-configs/ppcemb-softmmu.mak | 1 +
> > > hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
> > > 3 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > index 4d7be45..7d0dc2f 100644
> > > --- a/default-configs/ppc-softmmu.mak
> > > +++ b/default-configs/ppc-softmmu.mak
> > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > CONFIG_SM501=y
> > > CONFIG_IDE_SII3112=y
> > > CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > >
> > > # For Macs
> > > CONFIG_MAC=y
> > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > index 67d18b2..37af193 100644
> > > --- a/default-configs/ppcemb-softmmu.mak
> > > +++ b/default-configs/ppcemb-softmmu.mak
> > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > CONFIG_SM501=y
> > > CONFIG_IDE_SII3112=y
> > > CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index a68b5f7..5806209 100644
> > > --- a/hw/i2c/ppc4xx_i2c.c
> > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > @@ -30,6 +30,7 @@
> > > #include "cpu.h"
> > > #include "hw/hw.h"
> > > #include "hw/i2c/ppc4xx_i2c.h"
> > > +#include "bitbang_i2c.h"
> > >
> > > #define PPC4xx_I2C_MEM_SIZE 18
> > >
> > > @@ -46,7 +47,13 @@
> > >
> > > #define IIC_XTCNTLSS_SRST (1 << 0)
> > >
> > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > +
> > > typedef struct {
> > > + bitbang_i2c_interface *bitbang;
> > > uint8_t mdata;
> > > uint8_t lmadr;
> > > uint8_t hmadr;
> > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > > i2c->xtcntlss = value;
> > > break;
> > > case 16:
> > > - i2c->directcntl = value & 0x7;
> > > + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> >
> > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> >
> > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> >
> > Last expression might be clearer as:
> > value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>
> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> position in the register so I use that when accessing that bit but when I
> check for the values of a bit being 0 or 1 I don't use the define which is
> for something else, just happens to have value 1 as well.
Hmm.. but the bit is being store in i2c->directcntl, which means it
can be read back from the register in that position, no?
> If this does not explain your question and you think it's better to use
> defines here I can change that in next version, please let me know.
>
> Regards,
> BALATON Zoltan
>
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-13 8:50 ` [Qemu-devel] [Qemu-ppc] " BALATON Zoltan
@ 2018-06-13 10:03 ` David Gibson
2018-06-13 14:13 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-13 10:03 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3668 bytes --]
On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > > > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > > > of day is implemented. Setting time and RTC alarm are not supported.
> > > [...]
> > > > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > > > new file mode 100644
> > > > > index 0000000..9dbdb1b
> > > > > --- /dev/null
> > > > > +++ b/hw/timer/m41t80.c
> > > > > @@ -0,0 +1,117 @@
> > > > > +/*
> > > > > + * M41T80 serial rtc emulation
> > > > > + *
> > > > > + * Copyright (c) 2018 BALATON Zoltan
> > > > > + *
> > > > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "qemu/log.h"
> > > > > +#include "qemu/timer.h"
> > > > > +#include "qemu/bcd.h"
> > > > > +#include "hw/i2c/i2c.h"
> > > > > +
> > > > > +#define TYPE_M41T80 "m41t80"
> > > > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > > > +
> > > > > +typedef struct M41t80State {
> > > > > + I2CSlave parent_obj;
> > > > > + int8_t addr;
> > > > > +} M41t80State;
> > > > > +
> > > > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > + M41t80State *s = M41T80(dev);
> > > > > +
> > > > > + s->addr = -1;
> > > > > +}
> > > > > +
> > > > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > > > +{
> > > > > + M41t80State *s = M41T80(i2c);
> > > > > +
> > > > > + if (s->addr < 0) {
> > > > > + s->addr = data;
> > > > > + } else {
> > > > > + s->addr++;
> > > > > + }
> > > >
> > > > What about adding enum i2c_event in M41t80State and use the enum here
> > > > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > > > expected?
> > >
> > > Thanks for the review. I guess we could add enum for device bytes and the
> > > special case -1 meaning no register address selected yet but this is a very
> > > simple device with only 20 bytes and the datasheet also lists them by number
> > > without naming them so I think we can also refer to them by number. Since
> > > the device has only this 20 bytes the case with 127 should also not be a
> > > problem as that's invalid address anyway. Or did you mean something else?
> >
> > So, I'm not particularly in favour of adding extra state variables.
> >
> > But is using addr < 0 safe here? You're assigning the uint8_t data to
> > addr - could that result in a negative value?
>
> Why wouldn't it be safe with the expected values for register address
> between 0-19? If the guest sends garbage values over 127 it will either
> result in invalid register or unselected register and lead to an error when
> trying to read/write that register so I don't see what other problem this
> may cause.
Ok, but where is that enforced?
> The addr < 0 is to check if no address was selected before (on creating the
> device and when sending first value from host addr is set to -1. In this
> case first write will set register address, then subsequent reads/writes
> increment register address as the datasheet says).
>
> Regards,
> BALATON Zoltan
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-13 10:03 ` David Gibson
@ 2018-06-13 14:03 ` BALATON Zoltan
2018-06-18 2:46 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-13 14:03 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> default-configs/ppc-softmmu.mak | 1 +
>>>> default-configs/ppcemb-softmmu.mak | 1 +
>>>> hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
>>>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>>>> index 4d7be45..7d0dc2f 100644
>>>> --- a/default-configs/ppc-softmmu.mak
>>>> +++ b/default-configs/ppc-softmmu.mak
>>>> @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
>>>> CONFIG_SM501=y
>>>> CONFIG_IDE_SII3112=y
>>>> CONFIG_I2C=y
>>>> +CONFIG_BITBANG_I2C=y
>>>>
>>>> # For Macs
>>>> CONFIG_MAC=y
>>>> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
>>>> index 67d18b2..37af193 100644
>>>> --- a/default-configs/ppcemb-softmmu.mak
>>>> +++ b/default-configs/ppcemb-softmmu.mak
>>>> @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
>>>> CONFIG_SM501=y
>>>> CONFIG_IDE_SII3112=y
>>>> CONFIG_I2C=y
>>>> +CONFIG_BITBANG_I2C=y
>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>> index a68b5f7..5806209 100644
>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>> @@ -30,6 +30,7 @@
>>>> #include "cpu.h"
>>>> #include "hw/hw.h"
>>>> #include "hw/i2c/ppc4xx_i2c.h"
>>>> +#include "bitbang_i2c.h"
>>>>
>>>> #define PPC4xx_I2C_MEM_SIZE 18
>>>>
>>>> @@ -46,7 +47,13 @@
>>>>
>>>> #define IIC_XTCNTLSS_SRST (1 << 0)
>>>>
>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>> +
>>>> typedef struct {
>>>> + bitbang_i2c_interface *bitbang;
>>>> uint8_t mdata;
>>>> uint8_t lmadr;
>>>> uint8_t hmadr;
>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>> i2c->xtcntlss = value;
>>>> break;
>>>> case 16:
>>>> - i2c->directcntl = value & 0x7;
>>>> + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>
>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>
>>>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>
>>> Last expression might be clearer as:
>>> value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>
>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>> position in the register so I use that when accessing that bit but when I
>> check for the values of a bit being 0 or 1 I don't use the define which is
>> for something else, just happens to have value 1 as well.
>
> Hmm.. but the bit is being store in i2c->directcntl, which means it
> can be read back from the register in that position, no?
Which of the above two do you mean?
In the first one I test for the 1/0 value set by the previous line before
the bitbang_i2c_set call. This could be accessed as MSCL later but using
that here would just make it longer and less obvious. If I want to be
absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
in this line too but that was just stored in the register one line before
so I can reuse that here as well. Otherwise I could add another variable
just for this bit value and use that in both lines but why make it more
complicated for a simple 1 or 0 value?
In the second case using MSDA is really not correct because the level to
set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets
to tell which states the two i2c lines should be and the MSDA, MSCL are
read only bits that show what states the lines really are.
IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
line.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-13 10:03 ` David Gibson
@ 2018-06-13 14:13 ` BALATON Zoltan
2018-06-14 1:27 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-13 14:13 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
On Wed, 13 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
>>>> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
>>>>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>>>>> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
>>>>>> of day is implemented. Setting time and RTC alarm are not supported.
>>>> [...]
>>>>>> diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
>>>>>> new file mode 100644
>>>>>> index 0000000..9dbdb1b
>>>>>> --- /dev/null
>>>>>> +++ b/hw/timer/m41t80.c
>>>>>> @@ -0,0 +1,117 @@
>>>>>> +/*
>>>>>> + * M41T80 serial rtc emulation
>>>>>> + *
>>>>>> + * Copyright (c) 2018 BALATON Zoltan
>>>>>> + *
>>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/log.h"
>>>>>> +#include "qemu/timer.h"
>>>>>> +#include "qemu/bcd.h"
>>>>>> +#include "hw/i2c/i2c.h"
>>>>>> +
>>>>>> +#define TYPE_M41T80 "m41t80"
>>>>>> +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
>>>>>> +
>>>>>> +typedef struct M41t80State {
>>>>>> + I2CSlave parent_obj;
>>>>>> + int8_t addr;
>>>>>> +} M41t80State;
>>>>>> +
>>>>>> +static void m41t80_realize(DeviceState *dev, Error **errp)
>>>>>> +{
>>>>>> + M41t80State *s = M41T80(dev);
>>>>>> +
>>>>>> + s->addr = -1;
>>>>>> +}
>>>>>> +
>>>>>> +static int m41t80_send(I2CSlave *i2c, uint8_t data)
>>>>>> +{
>>>>>> + M41t80State *s = M41T80(i2c);
>>>>>> +
>>>>>> + if (s->addr < 0) {
>>>>>> + s->addr = data;
>>>>>> + } else {
>>>>>> + s->addr++;
>>>>>> + }
>>>>>
>>>>> What about adding enum i2c_event in M41t80State and use the enum here
>>>>> rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
>>>>> expected?
>>>>
>>>> Thanks for the review. I guess we could add enum for device bytes and the
>>>> special case -1 meaning no register address selected yet but this is a very
>>>> simple device with only 20 bytes and the datasheet also lists them by number
>>>> without naming them so I think we can also refer to them by number. Since
>>>> the device has only this 20 bytes the case with 127 should also not be a
>>>> problem as that's invalid address anyway. Or did you mean something else?
>>>
>>> So, I'm not particularly in favour of adding extra state variables.
>>>
>>> But is using addr < 0 safe here? You're assigning the uint8_t data to
>>> addr - could that result in a negative value?
>>
>> Why wouldn't it be safe with the expected values for register address
>> between 0-19? If the guest sends garbage values over 127 it will either
>> result in invalid register or unselected register and lead to an error when
>> trying to read/write that register so I don't see what other problem this
>> may cause.
>
> Ok, but where is that enforced?
I don't see the problem. The addr register selects the register to read or
write. It is set by the first write when the device is accessed the first
time (this is denoted by addr == -1 (or really any negative value). The
device has 20 registers and trying to read any register outside addr
between 0-19 will result in returning 0 and logging a warning about
invalid register in m41t80_recv. What could fail here when guest sends
garbage? It will set addr to invalid value and try to read non-exitent
register and get an error just like for any other nonexistent value of
addr (or start to read from register 0 if it manages to set a negative
value). All writes of registers are ignored currently (except setting addr
by the first write). What should be enforced here?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-13 14:13 ` BALATON Zoltan
@ 2018-06-14 1:27 ` David Gibson
2018-06-14 7:54 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-14 1:27 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4755 bytes --]
On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 10:50:59AM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> > > > > On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > > > > > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > > > > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > > > > > of day is implemented. Setting time and RTC alarm are not supported.
> > > > > [...]
> > > > > > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..9dbdb1b
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/timer/m41t80.c
> > > > > > > @@ -0,0 +1,117 @@
> > > > > > > +/*
> > > > > > > + * M41T80 serial rtc emulation
> > > > > > > + *
> > > > > > > + * Copyright (c) 2018 BALATON Zoltan
> > > > > > > + *
> > > > > > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > > > > > + *
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "qemu/log.h"
> > > > > > > +#include "qemu/timer.h"
> > > > > > > +#include "qemu/bcd.h"
> > > > > > > +#include "hw/i2c/i2c.h"
> > > > > > > +
> > > > > > > +#define TYPE_M41T80 "m41t80"
> > > > > > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > > > > > +
> > > > > > > +typedef struct M41t80State {
> > > > > > > + I2CSlave parent_obj;
> > > > > > > + int8_t addr;
> > > > > > > +} M41t80State;
> > > > > > > +
> > > > > > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > > > > > +{
> > > > > > > + M41t80State *s = M41T80(dev);
> > > > > > > +
> > > > > > > + s->addr = -1;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > > > > > +{
> > > > > > > + M41t80State *s = M41T80(i2c);
> > > > > > > +
> > > > > > > + if (s->addr < 0) {
> > > > > > > + s->addr = data;
> > > > > > > + } else {
> > > > > > > + s->addr++;
> > > > > > > + }
> > > > > >
> > > > > > What about adding enum i2c_event in M41t80State and use the enum here
> > > > > > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > > > > > expected?
> > > > >
> > > > > Thanks for the review. I guess we could add enum for device bytes and the
> > > > > special case -1 meaning no register address selected yet but this is a very
> > > > > simple device with only 20 bytes and the datasheet also lists them by number
> > > > > without naming them so I think we can also refer to them by number. Since
> > > > > the device has only this 20 bytes the case with 127 should also not be a
> > > > > problem as that's invalid address anyway. Or did you mean something else?
> > > >
> > > > So, I'm not particularly in favour of adding extra state variables.
> > > >
> > > > But is using addr < 0 safe here? You're assigning the uint8_t data to
> > > > addr - could that result in a negative value?
> > >
> > > Why wouldn't it be safe with the expected values for register address
> > > between 0-19? If the guest sends garbage values over 127 it will either
> > > result in invalid register or unselected register and lead to an error when
> > > trying to read/write that register so I don't see what other problem this
> > > may cause.
> >
> > Ok, but where is that enforced?
>
> I don't see the problem. The addr register selects the register to read or
> write. It is set by the first write when the device is accessed the first
> time (this is denoted by addr == -1 (or really any negative value). The
> device has 20 registers and trying to read any register outside addr between
> 0-19 will result in returning 0 and logging a warning about invalid register
> in m41t80_recv. What could fail here when guest sends garbage? It will set
> addr to invalid value and try to read non-exitent register and get an error
> just like for any other nonexistent value of addr (or start to read from
> register 0 if it manages to set a negative value). All writes of registers
> are ignored currently (except setting addr by the first write). What should
> be enforced here?
Ah, I see your point. I mean strictly we should match the hardware
behaviour if you write garbage addresses here, but really I don't
think it matters much.
Ok, it should be fine.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-14 1:27 ` David Gibson
@ 2018-06-14 7:54 ` BALATON Zoltan
2018-06-15 4:08 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-14 7:54 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
On Thu, 14 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
>> I don't see the problem. The addr register selects the register to read or
>> write. It is set by the first write when the device is accessed the first
>> time (this is denoted by addr == -1 (or really any negative value). The
>> device has 20 registers and trying to read any register outside addr between
>> 0-19 will result in returning 0 and logging a warning about invalid register
>> in m41t80_recv. What could fail here when guest sends garbage? It will set
>> addr to invalid value and try to read non-exitent register and get an error
>> just like for any other nonexistent value of addr (or start to read from
>> register 0 if it manages to set a negative value). All writes of registers
>> are ignored currently (except setting addr by the first write). What should
>> be enforced here?
>
> Ah, I see your point. I mean strictly we should match the hardware
> behaviour if you write garbage addresses here, but really I don't
> think it matters much.
Problem is like usual I have no idea what the real hardware does. I've
only seen the datasheet, never seen real device and have no way to test.
All the clients I've tested seem to be OK with the current emulation so
unless someone has more info on how this should work I think we can live
with this version and then fix it later if found to be needed.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation
2018-06-14 7:54 ` BALATON Zoltan
@ 2018-06-15 4:08 ` David Gibson
0 siblings, 0 replies; 41+ messages in thread
From: David Gibson @ 2018-06-15 4:08 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]
On Thu, Jun 14, 2018 at 09:54:41AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:13:57PM +0200, BALATON Zoltan wrote:
> > > I don't see the problem. The addr register selects the register to read or
> > > write. It is set by the first write when the device is accessed the first
> > > time (this is denoted by addr == -1 (or really any negative value). The
> > > device has 20 registers and trying to read any register outside addr between
> > > 0-19 will result in returning 0 and logging a warning about invalid register
> > > in m41t80_recv. What could fail here when guest sends garbage? It will set
> > > addr to invalid value and try to read non-exitent register and get an error
> > > just like for any other nonexistent value of addr (or start to read from
> > > register 0 if it manages to set a negative value). All writes of registers
> > > are ignored currently (except setting addr by the first write). What should
> > > be enforced here?
> >
> > Ah, I see your point. I mean strictly we should match the hardware
> > behaviour if you write garbage addresses here, but really I don't
> > think it matters much.
>
> Problem is like usual I have no idea what the real hardware does. I've only
> seen the datasheet, never seen real device and have no way to test. All the
> clients I've tested seem to be OK with the current emulation so unless
> someone has more info on how this should work I think we can live with this
> version and then fix it later if found to be needed.
Ok, fair enough.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-13 14:03 ` BALATON Zoltan
@ 2018-06-18 2:46 ` David Gibson
2018-06-19 9:29 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-18 2:46 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 5611 bytes --]
On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > ---
> > > > > default-configs/ppc-softmmu.mak | 1 +
> > > > > default-configs/ppcemb-softmmu.mak | 1 +
> > > > > hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
> > > > > 3 files changed, 15 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > > > index 4d7be45..7d0dc2f 100644
> > > > > --- a/default-configs/ppc-softmmu.mak
> > > > > +++ b/default-configs/ppc-softmmu.mak
> > > > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > > CONFIG_SM501=y
> > > > > CONFIG_IDE_SII3112=y
> > > > > CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > >
> > > > > # For Macs
> > > > > CONFIG_MAC=y
> > > > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > > > index 67d18b2..37af193 100644
> > > > > --- a/default-configs/ppcemb-softmmu.mak
> > > > > +++ b/default-configs/ppcemb-softmmu.mak
> > > > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > > CONFIG_SM501=y
> > > > > CONFIG_IDE_SII3112=y
> > > > > CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > index a68b5f7..5806209 100644
> > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > > > @@ -30,6 +30,7 @@
> > > > > #include "cpu.h"
> > > > > #include "hw/hw.h"
> > > > > #include "hw/i2c/ppc4xx_i2c.h"
> > > > > +#include "bitbang_i2c.h"
> > > > >
> > > > > #define PPC4xx_I2C_MEM_SIZE 18
> > > > >
> > > > > @@ -46,7 +47,13 @@
> > > > >
> > > > > #define IIC_XTCNTLSS_SRST (1 << 0)
> > > > >
> > > > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > > > +
> > > > > typedef struct {
> > > > > + bitbang_i2c_interface *bitbang;
> > > > > uint8_t mdata;
> > > > > uint8_t lmadr;
> > > > > uint8_t hmadr;
> > > > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > > > > i2c->xtcntlss = value;
> > > > > break;
> > > > > case 16:
> > > > > - i2c->directcntl = value & 0x7;
> > > > > + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > > >
> > > > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > > >
> > > > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > > >
> > > > Last expression might be clearer as:
> > > > value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> > >
> > > I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> > > position in the register so I use that when accessing that bit but when I
> > > check for the values of a bit being 0 or 1 I don't use the define which is
> > > for something else, just happens to have value 1 as well.
> >
> > Hmm.. but the bit is being store in i2c->directcntl, which means it
> > can be read back from the register in that position, no?
>
> Which of the above two do you mean?
>
> In the first one I test for the 1/0 value set by the previous line before
> the bitbang_i2c_set call. This could be accessed as MSCL later but using
> that here would just make it longer and less obvious. If I want to be
> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
> in this line too but that was just stored in the register one line before so
> I can reuse that here as well. Otherwise I could add another variable just
> for this bit value and use that in both lines but why make it more
> complicated for a simple 1 or 0 value?
Longer maybe, but I don't know about less obvious. Actually I think
you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
line setting i2c->directcntl, then the next line checking that bit to
pass it into bitbang_i2c_set. The point is you're modifying the
effective register contents, so it makes sense to make it clearer
which bit of the register you're setting.
> In the second case using MSDA is really not correct because the level to set
> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
> tell which states the two i2c lines should be and the MSDA, MSCL are read
> only bits that show what states the lines really are.
Ok...
> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
> line.
Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're
updating the value of the MSDA (== 0x2) bit in i2c->directcntl
register state. Why doesn't the symbolic name make sense here?
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-18 2:46 ` David Gibson
@ 2018-06-19 9:29 ` BALATON Zoltan
2018-06-20 1:27 ` David Gibson
0 siblings, 1 reply; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-19 9:29 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Mon, 18 Jun 2018, David Gibson wrote:
> On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
>> On Wed, 13 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>>> index a68b5f7..5806209 100644
>>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>>> @@ -30,6 +30,7 @@
>>>>>> #include "cpu.h"
>>>>>> #include "hw/hw.h"
>>>>>> #include "hw/i2c/ppc4xx_i2c.h"
>>>>>> +#include "bitbang_i2c.h"
>>>>>>
>>>>>> #define PPC4xx_I2C_MEM_SIZE 18
>>>>>>
>>>>>> @@ -46,7 +47,13 @@
>>>>>>
>>>>>> #define IIC_XTCNTLSS_SRST (1 << 0)
>>>>>>
>>>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>>>> +
>>>>>> typedef struct {
>>>>>> + bitbang_i2c_interface *bitbang;
>>>>>> uint8_t mdata;
>>>>>> uint8_t lmadr;
>>>>>> uint8_t hmadr;
>>>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>>>> i2c->xtcntlss = value;
>>>>>> break;
>>>>>> case 16:
>>>>>> - i2c->directcntl = value & 0x7;
>>>>>> + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>>>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>>>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>>>
>>>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>>>
>>>>>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>>>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>>>
>>>>> Last expression might be clearer as:
>>>>> value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>>>
>>>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>>>> position in the register so I use that when accessing that bit but when I
>>>> check for the values of a bit being 0 or 1 I don't use the define which is
>>>> for something else, just happens to have value 1 as well.
>>>
>>> Hmm.. but the bit is being store in i2c->directcntl, which means it
>>> can be read back from the register in that position, no?
>>
>> Which of the above two do you mean?
>>
>> In the first one I test for the 1/0 value set by the previous line before
>> the bitbang_i2c_set call. This could be accessed as MSCL later but using
>> that here would just make it longer and less obvious. If I want to be
>> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
>> in this line too but that was just stored in the register one line before so
>> I can reuse that here as well. Otherwise I could add another variable just
>> for this bit value and use that in both lines but why make it more
>> complicated for a simple 1 or 0 value?
>
> Longer maybe, but I don't know about less obvious. Actually I think
> you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
> line setting i2c->directcntl, then the next line checking that bit to
> pass it into bitbang_i2c_set. The point is you're modifying the
> effective register contents, so it makes sense to make it clearer
> which bit of the register you're setting.
When setting the bit it's the value 1 so that's not the bit position, I
think 1 : 0 is correct there. I've changed the next line in v4 I've just
sent to the constant when checking the value of the MSCL bit.
>> In the second case using MSDA is really not correct because the level to set
>> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
>> tell which states the two i2c lines should be and the MSDA, MSCL are read
>> only bits that show what states the lines really are.
>
> Ok...
>
>> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
>> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
>> line.
>
> Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're
> updating the value of the MSDA (== 0x2) bit in i2c->directcntl
> register state. Why doesn't the symbolic name make sense here?
Sorry, I may not have been able to clearly say what I mean. I meant that
IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
bit number 0) which may have value 1 or 0. In cases I mean the value I use
1 or 0. In case I refer to the bit position I use constants. In the line
bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
it should be the constant, just used 1 there for brevity because it's
obvious from the previous line what's meant. I've changed this now. At
other places the values of the bits are written as 1 or 0 so I think for
those constants should not be needed.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-19 9:29 ` BALATON Zoltan
@ 2018-06-20 1:27 ` David Gibson
2018-06-20 3:25 ` BALATON Zoltan
0 siblings, 1 reply; 41+ messages in thread
From: David Gibson @ 2018-06-20 1:27 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 6061 bytes --]
On Tue, Jun 19, 2018 at 11:29:09AM +0200, BALATON Zoltan wrote:
> On Mon, 18 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> > > > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > > > index a68b5f7..5806209 100644
> > > > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > > > > > @@ -30,6 +30,7 @@
> > > > > > > #include "cpu.h"
> > > > > > > #include "hw/hw.h"
> > > > > > > #include "hw/i2c/ppc4xx_i2c.h"
> > > > > > > +#include "bitbang_i2c.h"
> > > > > > >
> > > > > > > #define PPC4xx_I2C_MEM_SIZE 18
> > > > > > >
> > > > > > > @@ -46,7 +47,13 @@
> > > > > > >
> > > > > > > #define IIC_XTCNTLSS_SRST (1 << 0)
> > > > > > >
> > > > > > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > > > > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > > > > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > > > > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > > > > > +
> > > > > > > typedef struct {
> > > > > > > + bitbang_i2c_interface *bitbang;
> > > > > > > uint8_t mdata;
> > > > > > > uint8_t lmadr;
> > > > > > > uint8_t hmadr;
> > > > > > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > > > > > > i2c->xtcntlss = value;
> > > > > > > break;
> > > > > > > case 16:
> > > > > > > - i2c->directcntl = value & 0x7;
> > > > > > > + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > > > > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > > > > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > > > > >
> > > > > > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > > > > >
> > > > > > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > > > > >
> > > > > > Last expression might be clearer as:
> > > > > > value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> > > > >
> > > > > I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> > > > > position in the register so I use that when accessing that bit but when I
> > > > > check for the values of a bit being 0 or 1 I don't use the define which is
> > > > > for something else, just happens to have value 1 as well.
> > > >
> > > > Hmm.. but the bit is being store in i2c->directcntl, which means it
> > > > can be read back from the register in that position, no?
> > >
> > > Which of the above two do you mean?
> > >
> > > In the first one I test for the 1/0 value set by the previous line before
> > > the bitbang_i2c_set call. This could be accessed as MSCL later but using
> > > that here would just make it longer and less obvious. If I want to be
> > > absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
> > > in this line too but that was just stored in the register one line before so
> > > I can reuse that here as well. Otherwise I could add another variable just
> > > for this bit value and use that in both lines but why make it more
> > > complicated for a simple 1 or 0 value?
> >
> > Longer maybe, but I don't know about less obvious. Actually I think
> > you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
> > line setting i2c->directcntl, then the next line checking that bit to
> > pass it into bitbang_i2c_set. The point is you're modifying the
> > effective register contents, so it makes sense to make it clearer
> > which bit of the register you're setting.
>
> When setting the bit it's the value 1 so that's not the bit
> position,
Huh?? The constants aren't bit positions either, they're masks. How
is IIC_DIRECTCNTL_MSCL wrong here?
> I
> think 1 : 0 is correct there.
Correct, sure, but less clear than it could be.
> I've changed the next line in v4 I've just
> sent to the constant when checking the value of the MSCL bit.
>
> > > In the second case using MSDA is really not correct because the level to set
> > > is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
> > > tell which states the two i2c lines should be and the MSDA, MSCL are read
> > > only bits that show what states the lines really are.
> >
> > Ok...
> >
> > > IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
> > > directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
> > > line.
> >
> > Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're
> > updating the value of the MSDA (== 0x2) bit in i2c->directcntl
> > register state. Why doesn't the symbolic name make sense here?
>
> Sorry, I may not have been able to clearly say what I mean. I meant that
> IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
> bit number 0) which may have value 1 or 0. In cases I mean the value I use 1
> or 0. In case I refer to the bit position I use constants. In the line
>
> bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>
> it should be the constant, just used 1 there for brevity because it's
> obvious from the previous line what's meant.
Maybe, but using the constant is still clearer, and friendly to people
grepping the source.
> I've changed this now. At other
> places the values of the bits are written as 1 or 0 so I think for those
> constants should not be needed.
I have no idea what you mean by this.
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
2018-06-20 1:27 ` David Gibson
@ 2018-06-20 3:25 ` BALATON Zoltan
0 siblings, 0 replies; 41+ messages in thread
From: BALATON Zoltan @ 2018-06-20 3:25 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Wed, 20 Jun 2018, David Gibson wrote:
> On Tue, Jun 19, 2018 at 11:29:09AM +0200, BALATON Zoltan wrote:
>> On Mon, 18 Jun 2018, David Gibson wrote:
>>> On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>> On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
>>>>>> On Wed, 13 Jun 2018, David Gibson wrote:
>>>>>>> On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
>>>>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>>>>> index a68b5f7..5806209 100644
>>>>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>>>>> @@ -30,6 +30,7 @@
>>>>>>>> #include "cpu.h"
>>>>>>>> #include "hw/hw.h"
>>>>>>>> #include "hw/i2c/ppc4xx_i2c.h"
>>>>>>>> +#include "bitbang_i2c.h"
>>>>>>>>
>>>>>>>> #define PPC4xx_I2C_MEM_SIZE 18
>>>>>>>>
>>>>>>>> @@ -46,7 +47,13 @@
>>>>>>>>
>>>>>>>> #define IIC_XTCNTLSS_SRST (1 << 0)
>>>>>>>>
>>>>>>>> +#define IIC_DIRECTCNTL_SDAC (1 << 3)
>>>>>>>> +#define IIC_DIRECTCNTL_SCLC (1 << 2)
>>>>>>>> +#define IIC_DIRECTCNTL_MSDA (1 << 1)
>>>>>>>> +#define IIC_DIRECTCNTL_MSCL (1 << 0)
>>>>>>>> +
>>>>>>>> typedef struct {
>>>>>>>> + bitbang_i2c_interface *bitbang;
>>>>>>>> uint8_t mdata;
>>>>>>>> uint8_t lmadr;
>>>>>>>> uint8_t hmadr;
>>>>>>>> @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>>>>>> i2c->xtcntlss = value;
>>>>>>>> break;
>>>>>>>> case 16:
>>>>>>>> - i2c->directcntl = value & 0x7;
>>>>>>>> + i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>>>>>>>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>>>>>>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>>>>>>
>>>>>>> Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
>>>>>>>
>>>>>>>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>>>>>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>>>>>>
>>>>>>> Last expression might be clearer as:
>>>>>>> value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
>>>>>>
>>>>>> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
>>>>>> position in the register so I use that when accessing that bit but when I
>>>>>> check for the values of a bit being 0 or 1 I don't use the define which is
>>>>>> for something else, just happens to have value 1 as well.
>>>>>
>>>>> Hmm.. but the bit is being store in i2c->directcntl, which means it
>>>>> can be read back from the register in that position, no?
>>>>
>>>> Which of the above two do you mean?
>>>>
>>>> In the first one I test for the 1/0 value set by the previous line before
>>>> the bitbang_i2c_set call. This could be accessed as MSCL later but using
>>>> that here would just make it longer and less obvious. If I want to be
>>>> absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
>>>> in this line too but that was just stored in the register one line before so
>>>> I can reuse that here as well. Otherwise I could add another variable just
>>>> for this bit value and use that in both lines but why make it more
>>>> complicated for a simple 1 or 0 value?
>>>
>>> Longer maybe, but I don't know about less obvious. Actually I think
>>> you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
>>> line setting i2c->directcntl, then the next line checking that bit to
>>> pass it into bitbang_i2c_set. The point is you're modifying the
>>> effective register contents, so it makes sense to make it clearer
>>> which bit of the register you're setting.
>>
>> When setting the bit it's the value 1 so that's not the bit
>> position,
>
> Huh?? The constants aren't bit positions either, they're masks. How
> is IIC_DIRECTCNTL_MSCL wrong here?
>
>> I
>> think 1 : 0 is correct there.
>
> Correct, sure, but less clear than it could be.
>
>> I've changed the next line in v4 I've just
>> sent to the constant when checking the value of the MSCL bit.
>>
>>>> In the second case using MSDA is really not correct because the level to set
>>>> is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
>>>> tell which states the two i2c lines should be and the MSDA, MSCL are read
>>>> only bits that show what states the lines really are.
>>>
>>> Ok...
>>>
>>>> IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
>>>> directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
>>>> line.
>>>
>>> Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're
>>> updating the value of the MSDA (== 0x2) bit in i2c->directcntl
>>> register state. Why doesn't the symbolic name make sense here?
>>
>> Sorry, I may not have been able to clearly say what I mean. I meant that
>> IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
>> bit number 0) which may have value 1 or 0. In cases I mean the value I use 1
>> or 0. In case I refer to the bit position I use constants. In the line
>>
>> bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
>>
>> it should be the constant, just used 1 there for brevity because it's
>> obvious from the previous line what's meant.
>
> Maybe, but using the constant is still clearer, and friendly to people
> grepping the source.
>
>> I've changed this now. At other
>> places the values of the bits are written as 1 or 0 so I think for those
>> constants should not be needed.
>
> I have no idea what you mean by this.
OK, I'm lost now. Is v4 acceptable or are there any more changes you
propose for that? Feel free to adjust it as you think would be more clear
or tell me what to change if you want me to sumbit v5 of it.
Thank you,
BALATON Zoltan
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2018-06-20 3:25 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-06 13:31 [Qemu-devel] [PATCH v2 0/8] Misc sam460ex improvements BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
2018-06-13 1:22 ` David Gibson
2018-06-13 8:54 ` BALATON Zoltan
2018-06-13 10:03 ` David Gibson
2018-06-13 14:03 ` BALATON Zoltan
2018-06-18 2:46 ` David Gibson
2018-06-19 9:29 ` BALATON Zoltan
2018-06-20 1:27 ` David Gibson
2018-06-20 3:25 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 6/8] sam460ex: Add RTC device BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 8/8] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 1/8] ppc4xx_i2c: Clean up and improve error logging BALATON Zoltan
2018-06-06 15:56 ` Philippe Mathieu-Daudé
2018-06-08 8:50 ` David Gibson
2018-06-08 9:11 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 4/8] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register BALATON Zoltan
2018-06-06 14:09 ` Peter Maydell
2018-06-06 14:28 ` BALATON Zoltan
2018-06-06 15:32 ` Peter Maydell
2018-06-07 14:48 ` BALATON Zoltan
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers BALATON Zoltan
2018-06-08 8:56 ` David Gibson
2018-06-08 9:20 ` BALATON Zoltan
2018-06-13 1:20 ` David Gibson
2018-06-13 8:56 ` BALATON Zoltan
2018-06-13 10:01 ` David Gibson
2018-06-06 13:31 ` [Qemu-devel] [PATCH v2 5/8] hw/timer: Add basic M41T80 emulation BALATON Zoltan
2018-06-06 16:03 ` Philippe Mathieu-Daudé
2018-06-06 17:35 ` BALATON Zoltan
2018-06-13 4:11 ` David Gibson
2018-06-13 8:50 ` [Qemu-devel] [Qemu-ppc] " BALATON Zoltan
2018-06-13 10:03 ` David Gibson
2018-06-13 14:13 ` BALATON Zoltan
2018-06-14 1:27 ` David Gibson
2018-06-14 7:54 ` BALATON Zoltan
2018-06-15 4:08 ` David Gibson
2018-06-08 12:42 ` Cédric Le Goater
2018-06-08 16:16 ` BALATON Zoltan
2018-06-08 17:49 ` Cédric Le Goater
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.