* [Qemu-devel] [PATCH v4 08/11] sm501: Perform a full update after palette change
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440 BALATON Zoltan
` (10 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson, Sebastian Bauer
From: Sebastian Bauer <mail@sebastianbauer.info>
Changing the palette of a color index has as an immediate
effect on all pixels with the corresponding index on real
hardware. Performing a full update after a palette change
is a simple way to emulate this effect.
Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v4: Updated commit message
hw/display/sm501.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index a076248..e5df6c7 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -479,6 +479,7 @@ typedef struct SM501State {
MemoryRegion twoD_engine_region;
uint32_t last_width;
uint32_t last_height;
+ uint32_t do_full_update; /* perform a full update next time */
I2CBus *i2c_bus;
/* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
assert(range_covers_byte(0, 0x400 * 3, addr));
*(uint32_t *)&s->dc_palette[addr] = value;
+ s->do_full_update = 1;
}
static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
full_update = 1;
}
+ /* someone else requested a full update */
+ if (s->do_full_update) {
+ s->do_full_update = 0;
+ full_update = 1;
+ }
+
/* draw each line according to conditions */
snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 08/11] sm501: Perform a full update after palette change BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-20 5:41 ` David Gibson
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 04/11] hw/timer: Add basic M41T80 emulation BALATON Zoltan
` (9 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
According to PPC440 User Manual PPC440 has multiple opcodes for icbt
instruction: one for compatibility with older cores and two 440
specific opcodes one of which is defined in BookE. QEMU only
implements two of these, add the missing one.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v4: Updated commit message, for reference see bottom of:
http://manualzilla.com/doc/6915682/ppc440-processor-user-s-manual?page=451
target/ppc/translate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5fe1ba6..3a215a1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+ PPC_440_SPEC),
GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440 BALATON Zoltan
@ 2018-06-20 5:41 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2018-06-20 5:41 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> According to PPC440 User Manual PPC440 has multiple opcodes for icbt
> instruction: one for compatibility with older cores and two 440
> specific opcodes one of which is defined in BookE. QEMU only
> implements two of these, add the missing one.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Applied to ppc-for-3.0, thanks.
> ---
> v4: Updated commit message, for reference see bottom of:
> http://manualzilla.com/doc/6915682/ppc440-processor-user-s-manual?page=451
>
> target/ppc/translate.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 5fe1ba6..3a215a1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
> GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
> GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
> PPC_BOOKE, PPC2_BOOKE206),
> +GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
> + PPC_440_SPEC),
> GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
> GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
> GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
--
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] 28+ messages in thread
* [Qemu-devel] [PATCH v4 04/11] hw/timer: Add basic M41T80 emulation
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 08/11] sm501: Perform a full update after palette change BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440 BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
` (8 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 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>
---
v3: Fixed \n-s in log messages
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 0fb5f38..34c42b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -829,6 +829,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 851b4af..b8b0526 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..734d7d9
--- /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, "%s: unimplemented register: %d\n",
+ __func__, s->addr - 1);
+ return 0;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid register: %d\n",
+ __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] 28+ messages in thread* [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (2 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 04/11] hw/timer: Add basic M41T80 emulation BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-20 5:20 ` David Gibson
2018-11-28 10:28 ` Thomas Huth
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 09/11] sm501: Use values from the pitch register for 2d operations BALATON Zoltan
` (7 subsequent siblings)
11 siblings, 2 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
As well as being able to generate its own i2c transactions, the ppc4xx
i2c controller has a DIRECTCNTL register which allows explicit control
of the i2c lines.
Using this register an OS can directly bitbang i2c operations. In
order to let emulated i2c devices respond to this, we need to wire up
the DIRECTCNTL register to qemu's bitbanged i2c handling code.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v4: Updated commit message and use defined constant where appropriate
default-configs/ppc-softmmu.mak | 1 +
default-configs/ppcemb-softmmu.mak | 1 +
hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
include/hw/i2c/ppc4xx_i2c.h | 4 ++++
4 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index abeeb04..851b4af 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 4e0aaae..fca80d6 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,6 +47,11 @@
#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)
+
static void ppc4xx_i2c_reset(DeviceState *s)
{
PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,12 @@ 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 & IIC_DIRECTCNTL_MSCL);
+ i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+ (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
break;
default:
if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -322,6 +333,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");
+ s->bitbang = bitbang_i2c_init(s->bus);
}
static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index e4b6ded..ea6c8e1 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -31,6 +31,9 @@
#include "hw/sysbus.h"
#include "hw/i2c/i2c.h"
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
#define TYPE_PPC4xx_I2C "ppc4xx-i2c"
#define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
I2CBus *bus;
qemu_irq irq;
MemoryRegion iomem;
+ bitbang_i2c_interface *bitbang;
uint8_t mdata;
uint8_t lmadr;
uint8_t hmadr;
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
@ 2018-06-20 5:20 ` David Gibson
2018-06-21 7:17 ` BALATON Zoltan
2018-11-28 10:28 ` Thomas Huth
1 sibling, 1 reply; 28+ messages in thread
From: David Gibson @ 2018-06-20 5:20 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 4388 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> As well as being able to generate its own i2c transactions, the ppc4xx
> i2c controller has a DIRECTCNTL register which allows explicit control
> of the i2c lines.
>
> Using this register an OS can directly bitbang i2c operations. In
> order to let emulated i2c devices respond to this, we need to wire up
> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v4: Updated commit message and use defined constant where
> appropriate
I'm still don't quite understand your approach to the symbolic
constants here, but I don't care enough to hold this up any further.
So, applied to ppc-for-3.0.
>
> default-configs/ppc-softmmu.mak | 1 +
> default-configs/ppcemb-softmmu.mak | 1 +
> hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++-
> include/hw/i2c/ppc4xx_i2c.h | 4 ++++
> 4 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index abeeb04..851b4af 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 4e0aaae..fca80d6 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,6 +47,11 @@
>
> #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)
> +
> static void ppc4xx_i2c_reset(DeviceState *s)
> {
> PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> @@ -289,7 +295,12 @@ 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 & IIC_DIRECTCNTL_MSCL);
> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> break;
> default:
> if (addr < PPC4xx_I2C_MEM_SIZE) {
> @@ -322,6 +333,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");
> + s->bitbang = bitbang_i2c_init(s->bus);
> }
>
> static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index e4b6ded..ea6c8e1 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -31,6 +31,9 @@
> #include "hw/sysbus.h"
> #include "hw/i2c/i2c.h"
>
> +/* from hw/i2c/bitbang_i2c.h */
> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> +
> #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
> #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
>
> @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
> I2CBus *bus;
> qemu_irq irq;
> MemoryRegion iomem;
> + bitbang_i2c_interface *bitbang;
> uint8_t mdata;
> uint8_t lmadr;
> uint8_t hmadr;
--
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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-20 5:20 ` David Gibson
@ 2018-06-21 7:17 ` BALATON Zoltan
2018-06-22 1:59 ` David Gibson
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-21 7:17 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 10:52:15AM +0200, BALATON Zoltan wrote:
>> As well as being able to generate its own i2c transactions, the ppc4xx
>> i2c controller has a DIRECTCNTL register which allows explicit control
>> of the i2c lines.
>>
>> Using this register an OS can directly bitbang i2c operations. In
>> order to let emulated i2c devices respond to this, we need to wire up
>> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> v4: Updated commit message and use defined constant where
>> appropriate
>
> I'm still don't quite understand your approach to the symbolic
> constants here, but I don't care enough to hold this up any further.
> So, applied to ppc-for-3.0.
Thanks, just to try to clear this up, I consider symbolic constants to be
the name of bits 0-3 in the directntl register so while MSCL equals 1 it's
only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0
of directcntl reg.
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index 4e0aaae..fca80d6 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,6 +47,11 @@
>>
>> #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)
>> +
>> static void ppc4xx_i2c_reset(DeviceState *s)
>> {
>> PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>> @@ -289,7 +295,12 @@ 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);
This clears all bits but SDAC and SCLC so constants are OK here as they
refer to bits in the register. (Guest can set the S* bits to say what
state it wants the i2c lines to become.)
>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same
as SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so
constans are not appropriate here.
>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
>> + i2c->directcntl & IIC_DIRECTCNTL_MSCL);
This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0
so constant here is OK, previously it was just 1 for brevity which may
have confused you.
>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
This sets MSDA bit of directcntl to the value returned by bitbang_i2c
emulation when sending it the bit in SDAC. So the
(value & IIC_DIRECTCNTL_SDAC) != 0)
tests what value the SDAC bit has so 0 means the value of the bit and
constant refers to the bit in the register. (Because SDAC is not the LSB
and we need 1 or 0 here hence the equality test to normalise the value,
maybe the !! construct could also be used, I'm not sure.) The << 1 at the
end makes sure we set the MSDA bit but that constant cannot be used here
and using MSCL instead is not correct because we mean the MSDA bit.
In summary, guest sets SDAC and SCLC as it wants the i2c lines and MSDA
and MSCL are set by the device to what state the lines are actually in.
(The S in first two regs may stand for Set while M stands for Monitor.)
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-21 7:17 ` BALATON Zoltan
@ 2018-06-22 1:59 ` David Gibson
2018-06-22 8:00 ` BALATON Zoltan
0 siblings, 1 reply; 28+ messages in thread
From: David Gibson @ 2018-06-22 1:59 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 4546 bytes --]
On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote:
> On Wed, 20 Jun 2018, David Gibson wrote:
> > On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> > > As well as being able to generate its own i2c transactions, the ppc4xx
> > > i2c controller has a DIRECTCNTL register which allows explicit control
> > > of the i2c lines.
> > >
> > > Using this register an OS can directly bitbang i2c operations. In
> > > order to let emulated i2c devices respond to this, we need to wire up
> > > the DIRECTCNTL register to qemu's bitbanged i2c handling code.
> > >
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > > v4: Updated commit message and use defined constant where
> > > appropriate
> >
> > I'm still don't quite understand your approach to the symbolic
> > constants here, but I don't care enough to hold this up any further.
> > So, applied to ppc-for-3.0.
>
> Thanks, just to try to clear this up, I consider symbolic constants to be
> the name of bits 0-3 in the directntl register so while MSCL equals 1 it's
> only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of
> directcntl reg.
Right..
>
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index 4e0aaae..fca80d6 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,6 +47,11 @@
> > >
> > > #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)
> > > +
> > > static void ppc4xx_i2c_reset(DeviceState *s)
> > > {
> > > PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> > > @@ -289,7 +295,12 @@ 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);
>
> This clears all bits but SDAC and SCLC so constants are OK here as they
> refer to bits in the register. (Guest can set the S* bits to say what state
> it wants the i2c lines to become.)
>
> > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>
> This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as
> SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans
> are not appropriate here.
This is what I don't get. Regardless of the method of it, you *are*
setting bit 1 of the directcntl register, so why would the MSCL name
not be appropriate?
>
> > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
> > > + i2c->directcntl & IIC_DIRECTCNTL_MSCL);
>
> This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so
> constant here is OK, previously it was just 1 for brevity which may have
> confused you.
>
> > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>
> This sets MSDA bit of directcntl to the value returned by bitbang_i2c
> emulation when sending it the bit in SDAC. So the
> (value & IIC_DIRECTCNTL_SDAC) != 0)
> tests what value the SDAC bit has so 0 means the value of the bit and
> constant refers to the bit in the register. (Because SDAC is not the LSB and
> we need 1 or 0 here hence the equality test to normalise the value, maybe
> the !! construct could also be used, I'm not sure.) The << 1 at the end
> makes sure we set the MSDA bit but that constant cannot be used here and
> using MSCL instead is not correct because we mean the MSDA bit.
Right, I'm not suggesting you use MSCL here, I'm suggesting you use
MSDA.
> In summary, guest sets SDAC and SCLC as it wants the i2c lines and MSDA and
> MSCL are set by the device to what state the lines are actually in. (The S
> in first two regs may stand for Set while M stands for Monitor.)
>
> 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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-22 1:59 ` David Gibson
@ 2018-06-22 8:00 ` BALATON Zoltan
2018-06-22 10:49 ` David Gibson
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-22 8:00 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, Alexander Graf
On Fri, 22 Jun 2018, David Gibson wrote:
> On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote:
>> On Wed, 20 Jun 2018, David Gibson wrote:
>>> On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
>>>> As well as being able to generate its own i2c transactions, the ppc4xx
>>>> i2c controller has a DIRECTCNTL register which allows explicit control
>>>> of the i2c lines.
>>>>
>>>> Using this register an OS can directly bitbang i2c operations. In
>>>> order to let emulated i2c devices respond to this, we need to wire up
>>>> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> v4: Updated commit message and use defined constant where
>>>> appropriate
>>>
>>> I'm still don't quite understand your approach to the symbolic
>>> constants here, but I don't care enough to hold this up any further.
>>> So, applied to ppc-for-3.0.
>>
>> Thanks, just to try to clear this up, I consider symbolic constants to be
>> the name of bits 0-3 in the directntl register so while MSCL equals 1 it's
>> only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of
>> directcntl reg.
>
> Right..
>
>>
>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>> index 4e0aaae..fca80d6 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,6 +47,11 @@
>>>>
>>>> #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)
>>>> +
>>>> static void ppc4xx_i2c_reset(DeviceState *s)
>>>> {
>>>> PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>>>> @@ -289,7 +295,12 @@ 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);
>>
>> This clears all bits but SDAC and SCLC so constants are OK here as they
>> refer to bits in the register. (Guest can set the S* bits to say what state
>> it wants the i2c lines to become.)
>>
>>>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
>>
>> This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as
>> SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans
>> are not appropriate here.
>
> This is what I don't get. Regardless of the method of it, you *are*
> setting bit 1 of the directcntl register, so why would the MSCL name
> not be appropriate?
I'm setting bit 0 (MSCL) to either 1 or 0. I could probably use the MSCL
constant in place of the 1 here but would that make it clearer? It would
just be longer and less clear without looking up the constants so to me
this looks more comprehensible this way.
>>>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
>>>> + i2c->directcntl & IIC_DIRECTCNTL_MSCL);
>>
>> This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so
>> constant here is OK, previously it was just 1 for brevity which may have
>> confused you.
>>
>>>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
>>>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>>
>> This sets MSDA bit of directcntl to the value returned by bitbang_i2c
>> emulation when sending it the bit in SDAC. So the
>> (value & IIC_DIRECTCNTL_SDAC) != 0)
>> tests what value the SDAC bit has so 0 means the value of the bit and
>> constant refers to the bit in the register. (Because SDAC is not the LSB and
>> we need 1 or 0 here hence the equality test to normalise the value, maybe
>> the !! construct could also be used, I'm not sure.) The << 1 at the end
>> makes sure we set the MSDA bit but that constant cannot be used here and
>> using MSCL instead is not correct because we mean the MSDA bit.
>
> Right, I'm not suggesting you use MSCL here, I'm suggesting you use
> MSDA.
But how? Third arg of bitbang_i2c_set is level, either 0 or 1. How could a
constant with value 2 used here? (Also to set bit 1 I have to shift 1 not
2 so I don't see how the constant could be used there either.)
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-22 8:00 ` BALATON Zoltan
@ 2018-06-22 10:49 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2018-06-22 10:49 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 5716 bytes --]
On Fri, Jun 22, 2018 at 10:00:34AM +0200, BALATON Zoltan wrote:
> On Fri, 22 Jun 2018, David Gibson wrote:
> > On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote:
> > > On Wed, 20 Jun 2018, David Gibson wrote:
> > > > On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> > > > > As well as being able to generate its own i2c transactions, the ppc4xx
> > > > > i2c controller has a DIRECTCNTL register which allows explicit control
> > > > > of the i2c lines.
> > > > >
> > > > > Using this register an OS can directly bitbang i2c operations. In
> > > > > order to let emulated i2c devices respond to this, we need to wire up
> > > > > the DIRECTCNTL register to qemu's bitbanged i2c handling code.
> > > > >
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > ---
> > > > > v4: Updated commit message and use defined constant where
> > > > > appropriate
> > > >
> > > > I'm still don't quite understand your approach to the symbolic
> > > > constants here, but I don't care enough to hold this up any further.
> > > > So, applied to ppc-for-3.0.
> > >
> > > Thanks, just to try to clear this up, I consider symbolic constants to be
> > > the name of bits 0-3 in the directntl register so while MSCL equals 1 it's
> > > only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of
> > > directcntl reg.
> >
> > Right..
> >
> > >
> > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > index 4e0aaae..fca80d6 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,6 +47,11 @@
> > > > >
> > > > > #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)
> > > > > +
> > > > > static void ppc4xx_i2c_reset(DeviceState *s)
> > > > > {
> > > > > PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> > > > > @@ -289,7 +295,12 @@ 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);
> > >
> > > This clears all bits but SDAC and SCLC so constants are OK here as they
> > > refer to bits in the register. (Guest can set the S* bits to say what state
> > > it wants the i2c lines to become.)
> > >
> > > > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > >
> > > This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as
> > > SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans
> > > are not appropriate here.
> >
> > This is what I don't get. Regardless of the method of it, you *are*
> > setting bit 1 of the directcntl register, so why would the MSCL name
> > not be appropriate?
>
> I'm setting bit 0 (MSCL) to either 1 or 0. I could probably use the MSCL
> constant in place of the 1 here but would that make it clearer?
Yeah, it kinda would. Instead of the line saying "it sets bit 0 based
on this" it would say "this sets the MSCL bit based on this" which is
probably more useful information. In addition, if someone greps
looking find everywhere the MSCL bit is manipulated, they'll find it.
> It would
> just be longer and less clear without looking up the constants so to me this
> looks more comprehensible this way.
>
> > > > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
> > > > > + i2c->directcntl & IIC_DIRECTCNTL_MSCL);
> > >
> > > This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so
> > > constant here is OK, previously it was just 1 for brevity which may have
> > > confused you.
> > >
> > > > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > >
> > > This sets MSDA bit of directcntl to the value returned by bitbang_i2c
> > > emulation when sending it the bit in SDAC. So the
> > > (value & IIC_DIRECTCNTL_SDAC) != 0)
> > > tests what value the SDAC bit has so 0 means the value of the bit and
> > > constant refers to the bit in the register. (Because SDAC is not the LSB and
> > > we need 1 or 0 here hence the equality test to normalise the value, maybe
> > > the !! construct could also be used, I'm not sure.) The << 1 at the end
> > > makes sure we set the MSDA bit but that constant cannot be used here and
> > > using MSCL instead is not correct because we mean the MSDA bit.
> >
> > Right, I'm not suggesting you use MSCL here, I'm suggesting you use
> > MSDA.
>
> But how? Third arg of bitbang_i2c_set is level, either 0 or 1. How could a
> constant with value 2 used here? (Also to set bit 1 I have to shift 1 not 2
> so I don't see how the constant could be used there either.)
Don't use the shift.
i2c->directcntl |= bitbang_i2c_set(i2c->bitmang, BITBANG_I2C_SDA,
!!(value & IIC_DIRECTCNTL_SDAC))
? IIC_DIRECTCNTL_MSDA : 0;
--
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] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
2018-06-20 5:20 ` David Gibson
@ 2018-11-28 10:28 ` Thomas Huth
2018-11-28 11:26 ` BALATON Zoltan
1 sibling, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2018-11-28 10:28 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc, Peter Maydell
Cc: Alexander Graf, David Gibson
On 2018-06-19 10:52, BALATON Zoltan wrote:
> As well as being able to generate its own i2c transactions, the ppc4xx
> i2c controller has a DIRECTCNTL register which allows explicit control
> of the i2c lines.
>
> Using this register an OS can directly bitbang i2c operations. In
> order to let emulated i2c devices respond to this, we need to wire up
> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
[...]
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index e4b6ded..ea6c8e1 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -31,6 +31,9 @@
> #include "hw/sysbus.h"
> #include "hw/i2c/i2c.h"
>
> +/* from hw/i2c/bitbang_i2c.h */
> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
This breaks compilation with clang 3.4:
In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33:
hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef
'bitbang_i2c_interface'
is a C11 feature [-Werror,-Wtypedef-redefinition]
typedef struct bitbang_i2c_interface bitbang_i2c_interface;
^
include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here
typedef struct bitbang_i2c_interface bitbang_i2c_interface;
^
1 error generated.
make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1
Not sure about the best way to fix this ... move the typedef to
include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h"
header instead of "i2c.h" here?
Thomas
PS: This pops up quite frequently and is annoying ... Did we ever
consider to enforce "-std=gnu11" for compiling QEMU?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-11-28 10:28 ` Thomas Huth
@ 2018-11-28 11:26 ` BALATON Zoltan
2018-11-28 11:30 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-11-28 11:26 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, qemu-ppc, Peter Maydell, Alexander Graf, David Gibson
On Wed, 28 Nov 2018, Thomas Huth wrote:
> On 2018-06-19 10:52, BALATON Zoltan wrote:
>> As well as being able to generate its own i2c transactions, the ppc4xx
>> i2c controller has a DIRECTCNTL register which allows explicit control
>> of the i2c lines.
>>
>> Using this register an OS can directly bitbang i2c operations. In
>> order to let emulated i2c devices respond to this, we need to wire up
>> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
> [...]
>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
>> index e4b6ded..ea6c8e1 100644
>> --- a/include/hw/i2c/ppc4xx_i2c.h
>> +++ b/include/hw/i2c/ppc4xx_i2c.h
>> @@ -31,6 +31,9 @@
>> #include "hw/sysbus.h"
>> #include "hw/i2c/i2c.h"
>>
>> +/* from hw/i2c/bitbang_i2c.h */
>> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>
> This breaks compilation with clang 3.4:
>
> In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33:
> hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef
> 'bitbang_i2c_interface'
> is a C11 feature [-Werror,-Wtypedef-redefinition]
> typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> ^
> include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here
> typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> ^
> 1 error generated.
> make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1
>
> Not sure about the best way to fix this ... move the typedef to
> include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h"
> header instead of "i2c.h" here?
Not sure either but I could not include bitbang_i2c.h here because that's
a private header in hw/i2c, whereas ppc4xx_i2c.h is a public header in
include/hw/i2c. That's why it had to be duplicated here. I had it
differently in first version but this was thought to be simpler way, see
http://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00374.html
Maybe we could move this typedef to include/hw/i2c/i2c.h which both
ppc4xx_i2c.h and bitbang_i2c.h include?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register
2018-11-28 11:26 ` BALATON Zoltan
@ 2018-11-28 11:30 ` Thomas Huth
0 siblings, 0 replies; 28+ messages in thread
From: Thomas Huth @ 2018-11-28 11:30 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, qemu-ppc, Peter Maydell, Alexander Graf, David Gibson
On 2018-11-28 12:26, BALATON Zoltan wrote:
> On Wed, 28 Nov 2018, Thomas Huth wrote:
>> On 2018-06-19 10:52, BALATON Zoltan wrote:
>>> As well as being able to generate its own i2c transactions, the ppc4xx
>>> i2c controller has a DIRECTCNTL register which allows explicit control
>>> of the i2c lines.
>>>
>>> Using this register an OS can directly bitbang i2c operations. In
>>> order to let emulated i2c devices respond to this, we need to wire up
>>> the DIRECTCNTL register to qemu's bitbanged i2c handling code.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>> [...]
>>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
>>> index e4b6ded..ea6c8e1 100644
>>> --- a/include/hw/i2c/ppc4xx_i2c.h
>>> +++ b/include/hw/i2c/ppc4xx_i2c.h
>>> @@ -31,6 +31,9 @@
>>> #include "hw/sysbus.h"
>>> #include "hw/i2c/i2c.h"
>>>
>>> +/* from hw/i2c/bitbang_i2c.h */
>>> +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>>
>> This breaks compilation with clang 3.4:
>>
>> In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33:
>> hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef
>> 'bitbang_i2c_interface'
>> is a C11 feature [-Werror,-Wtypedef-redefinition]
>> typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>> ^
>> include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here
>> typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>> ^
>> 1 error generated.
>> make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1
>>
>> Not sure about the best way to fix this ... move the typedef to
>> include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h"
>> header instead of "i2c.h" here?
>
> Not sure either but I could not include bitbang_i2c.h here because
> that's a private header in hw/i2c, whereas ppc4xx_i2c.h is a public
> header in include/hw/i2c. That's why it had to be duplicated here. I had
> it differently in first version but this was thought to be simpler way,
> see http://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00374.html
>
> Maybe we could move this typedef to include/hw/i2c/i2c.h which both
> ppc4xx_i2c.h and bitbang_i2c.h include?
Fine for me, too. Do you have some spare time to create a patch?
Thanks,
Thomas
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v4 09/11] sm501: Use values from the pitch register for 2d operations.
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (3 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
` (6 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson, Sebastian Bauer
From: Sebastian Bauer <mail@sebastianbauer.info>
Before, crt_h_total was used for src_width and dst_width. This is a
property of the current display setting and not relevant for the 2d
operation that also can be done off-screen. The pitch register's purpose
is to describe line pitch relevant of the 2d operation.
Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e5df6c7..e126c15 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -701,8 +701,8 @@ static void sm501_2d_operation(SM501State *s)
/* get frame buffer info */
uint8_t *src = s->local_mem + (s->twoD_source_base & 0x03FFFFFF);
uint8_t *dst = s->local_mem + (s->twoD_destination_base & 0x03FFFFFF);
- int src_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
- int dst_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
+ int src_width = s->twoD_pitch & 0x1FFF;
+ int dst_width = (s->twoD_pitch >> 16) & 0x1FFF;
if (addressing != 0x0) {
printf("%s: only XY addressing is supported.\n", __func__);
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (4 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 09/11] sm501: Use values from the pitch register for 2d operations BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-20 7:17 ` David Gibson
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers BALATON Zoltan
` (5 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Emulate the i2c part of SM501 which is used to access the EDID info
from a monitor.
The vmstate structure is changed and its version is increased but
SM501 is only used on SH and PPC sam460ex machines that don't support
cross-version migration.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v4: Updated commit message
default-configs/ppc-softmmu.mak | 1 +
default-configs/ppcemb-softmmu.mak | 1 +
default-configs/sh4-softmmu.mak | 1 +
default-configs/sh4eb-softmmu.mak | 1 +
hw/display/sm501.c | 136 +++++++++++++++++++++++++++++++++++--
5 files changed, 136 insertions(+), 4 deletions(-)
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index b8b0526..e131e24 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -24,6 +24,7 @@ CONFIG_ETSEC=y
# For Sam460ex
CONFIG_USB_EHCI_SYSBUS=y
CONFIG_SM501=y
+CONFIG_DDC=y
CONFIG_IDE_SII3112=y
CONFIG_I2C=y
CONFIG_BITBANG_I2C=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 37af193..ac44f15 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -17,6 +17,7 @@ CONFIG_XILINX=y
CONFIG_XILINX_ETHLITE=y
CONFIG_USB_EHCI_SYSBUS=y
CONFIG_SM501=y
+CONFIG_DDC=y
CONFIG_IDE_SII3112=y
CONFIG_I2C=y
CONFIG_BITBANG_I2C=y
diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
index 546d855..72d8fca 100644
--- a/default-configs/sh4-softmmu.mak
+++ b/default-configs/sh4-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
CONFIG_SH4=y
CONFIG_IDE_MMIO=y
CONFIG_SM501=y
+CONFIG_DDC=y
CONFIG_ISA_TESTDEV=y
CONFIG_I82378=y
CONFIG_I8259=y
diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
index 2d3fd49..c686637 100644
--- a/default-configs/sh4eb-softmmu.mak
+++ b/default-configs/sh4eb-softmmu.mak
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
CONFIG_SH4=y
CONFIG_IDE_MMIO=y
CONFIG_SM501=y
+CONFIG_DDC=y
CONFIG_ISA_TESTDEV=y
CONFIG_I82378=y
CONFIG_I8259=y
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 8206ae8..a076248 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "qemu/cutils.h"
#include "qapi/error.h"
+#include "qemu/log.h"
#include "qemu-common.h"
#include "cpu.h"
#include "hw/hw.h"
@@ -34,6 +35,8 @@
#include "hw/devices.h"
#include "hw/sysbus.h"
#include "hw/pci/pci.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c-ddc.h"
#include "qemu/range.h"
#include "ui/pixel_ops.h"
@@ -471,10 +474,12 @@ typedef struct SM501State {
MemoryRegion local_mem_region;
MemoryRegion mmio_region;
MemoryRegion system_config_region;
+ MemoryRegion i2c_region;
MemoryRegion disp_ctrl_region;
MemoryRegion twoD_engine_region;
uint32_t last_width;
uint32_t last_height;
+ I2CBus *i2c_bus;
/* mmio registers */
uint32_t system_control;
@@ -487,6 +492,11 @@ typedef struct SM501State {
uint32_t misc_timing;
uint32_t power_mode_control;
+ uint8_t i2c_byte_count;
+ uint8_t i2c_status;
+ uint8_t i2c_addr;
+ uint8_t i2c_data[16];
+
uint32_t uart0_ier;
uint32_t uart0_lcr;
uint32_t uart0_mcr;
@@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
+{
+ SM501State *s = (SM501State *)opaque;
+ uint8_t ret = 0;
+
+ switch (addr) {
+ case SM501_I2C_BYTE_COUNT:
+ ret = s->i2c_byte_count;
+ break;
+ case SM501_I2C_STATUS:
+ ret = s->i2c_status;
+ break;
+ case SM501_I2C_SLAVE_ADDRESS:
+ ret = s->i2c_addr;
+ break;
+ case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+ ret = s->i2c_data[addr - SM501_I2C_DATA];
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
+ " addr=0x%" HWADDR_PRIx "\n", addr);
+ }
+
+ SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
+ addr, ret);
+ return ret;
+}
+
+static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
+ unsigned size)
+{
+ SM501State *s = (SM501State *)opaque;
+ SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
+ " val=%" PRIx64 "\n", addr, value);
+
+ switch (addr) {
+ case SM501_I2C_BYTE_COUNT:
+ s->i2c_byte_count = value & 0xf;
+ break;
+ case SM501_I2C_CONTROL:
+ if (value & 1) {
+ if (value & 4) {
+ int res = i2c_start_transfer(s->i2c_bus,
+ s->i2c_addr >> 1,
+ s->i2c_addr & 1);
+ s->i2c_status |= (res ? 1 << 2 : 0);
+ if (!res) {
+ int i;
+ SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
+ s->i2c_byte_count + 1, s->i2c_addr >> 1);
+ for (i = 0; i <= s->i2c_byte_count; i++) {
+ res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
+ !(s->i2c_addr & 1));
+ if (res) {
+ SM501_DPRINTF("sm501 i2c : transfer failed"
+ " i=%d, res=%d\n", i, res);
+ s->i2c_status |= (res ? 1 << 2 : 0);
+ return;
+ }
+ }
+ if (i) {
+ SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
+ s->i2c_status = 8;
+ }
+ }
+ } else {
+ SM501_DPRINTF("sm501 i2c : end transfer\n");
+ i2c_end_transfer(s->i2c_bus);
+ s->i2c_status &= ~4;
+ }
+ }
+ break;
+ case SM501_I2C_RESET:
+ s->i2c_status &= ~4;
+ break;
+ case SM501_I2C_SLAVE_ADDRESS:
+ s->i2c_addr = value & 0xff;
+ break;
+ case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+ s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
+ "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
+ }
+}
+
+static const MemoryRegionOps sm501_i2c_ops = {
+ .read = sm501_i2c_read,
+ .write = sm501_i2c_write,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ },
+ .impl = {
+ .min_access_size = 1,
+ .max_access_size = 1,
+ },
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
{
SM501State *s = (SM501State *)opaque;
@@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
s->irq_mask = 0;
s->misc_timing = 0;
s->power_mode_control = 0;
+ s->i2c_byte_count = 0;
+ s->i2c_status = 0;
+ s->i2c_addr = 0;
+ memset(s->i2c_data, 0, 16);
s->dc_panel_control = 0x00010000; /* FIFO level 3 */
s->dc_video_control = 0;
s->dc_crt_control = 0x00010000;
@@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
+ /* i2c */
+ s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
+ I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
+ i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+
/* mmio */
memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
memory_region_init_io(&s->system_config_region, OBJECT(dev),
@@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
"sm501-system-config", 0x6c);
memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
&s->system_config_region);
+ memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
+ "sm501-i2c", 0x14);
+ memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
&sm501_disp_ctrl_ops, s,
"sm501-disp-ctrl", 0x1000);
@@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
VMSTATE_UINT32(twoD_destination_base, SM501State),
VMSTATE_UINT32(twoD_alpha, SM501State),
VMSTATE_UINT32(twoD_wrap, SM501State),
+ /* Added in version 2 */
+ VMSTATE_UINT8(i2c_byte_count, SM501State),
+ VMSTATE_UINT8(i2c_status, SM501State),
+ VMSTATE_UINT8(i2c_addr, SM501State),
+ VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
VMSTATE_END_OF_LIST()
}
};
@@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
static const VMStateDescription vmstate_sm501_sysbus = {
.name = TYPE_SYSBUS_SM501,
- .version_id = 1,
- .minimum_version_id = 1,
+ .version_id = 2,
+ .minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_STRUCT(state, SM501SysBusState, 1,
vmstate_sm501_state, SM501State),
@@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
static const VMStateDescription vmstate_sm501_pci = {
.name = TYPE_PCI_SM501,
- .version_id = 1,
- .minimum_version_id = 1,
+ .version_id = 2,
+ .minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
VMSTATE_STRUCT(state, SM501PCIState, 1,
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
@ 2018-06-20 7:17 ` David Gibson
2018-06-22 12:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: David Gibson @ 2018-06-20 7:17 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 11332 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> Emulate the i2c part of SM501 which is used to access the EDID info
> from a monitor.
>
> The vmstate structure is changed and its version is increased but
> SM501 is only used on SH and PPC sam460ex machines that don't support
> cross-version migration.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v4: Updated commit message
This patch breaks compile on SH - sm501 now needs various i2c symbols
that aren't configured into the SH builds.
As a rule, it's always wise to try compiling with a bare ./configure
before you post to make sure you didn't break some platform you
weren't thinking about. Doing a "make check" like that is even better.
>
> default-configs/ppc-softmmu.mak | 1 +
> default-configs/ppcemb-softmmu.mak | 1 +
> default-configs/sh4-softmmu.mak | 1 +
> default-configs/sh4eb-softmmu.mak | 1 +
> hw/display/sm501.c | 136 +++++++++++++++++++++++++++++++++++--
> 5 files changed, 136 insertions(+), 4 deletions(-)
>
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index b8b0526..e131e24 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -24,6 +24,7 @@ CONFIG_ETSEC=y
> # For Sam460ex
> CONFIG_USB_EHCI_SYSBUS=y
> CONFIG_SM501=y
> +CONFIG_DDC=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index 37af193..ac44f15 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -17,6 +17,7 @@ CONFIG_XILINX=y
> CONFIG_XILINX_ETHLITE=y
> CONFIG_USB_EHCI_SYSBUS=y
> CONFIG_SM501=y
> +CONFIG_DDC=y
> CONFIG_IDE_SII3112=y
> CONFIG_I2C=y
> CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
> index 546d855..72d8fca 100644
> --- a/default-configs/sh4-softmmu.mak
> +++ b/default-configs/sh4-softmmu.mak
> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
> CONFIG_SH4=y
> CONFIG_IDE_MMIO=y
> CONFIG_SM501=y
> +CONFIG_DDC=y
> CONFIG_ISA_TESTDEV=y
> CONFIG_I82378=y
> CONFIG_I8259=y
> diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
> index 2d3fd49..c686637 100644
> --- a/default-configs/sh4eb-softmmu.mak
> +++ b/default-configs/sh4eb-softmmu.mak
> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
> CONFIG_SH4=y
> CONFIG_IDE_MMIO=y
> CONFIG_SM501=y
> +CONFIG_DDC=y
> CONFIG_ISA_TESTDEV=y
> CONFIG_I82378=y
> CONFIG_I8259=y
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 8206ae8..a076248 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -26,6 +26,7 @@
> #include "qemu/osdep.h"
> #include "qemu/cutils.h"
> #include "qapi/error.h"
> +#include "qemu/log.h"
> #include "qemu-common.h"
> #include "cpu.h"
> #include "hw/hw.h"
> @@ -34,6 +35,8 @@
> #include "hw/devices.h"
> #include "hw/sysbus.h"
> #include "hw/pci/pci.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/i2c-ddc.h"
> #include "qemu/range.h"
> #include "ui/pixel_ops.h"
>
> @@ -471,10 +474,12 @@ typedef struct SM501State {
> MemoryRegion local_mem_region;
> MemoryRegion mmio_region;
> MemoryRegion system_config_region;
> + MemoryRegion i2c_region;
> MemoryRegion disp_ctrl_region;
> MemoryRegion twoD_engine_region;
> uint32_t last_width;
> uint32_t last_height;
> + I2CBus *i2c_bus;
>
> /* mmio registers */
> uint32_t system_control;
> @@ -487,6 +492,11 @@ typedef struct SM501State {
> uint32_t misc_timing;
> uint32_t power_mode_control;
>
> + uint8_t i2c_byte_count;
> + uint8_t i2c_status;
> + uint8_t i2c_addr;
> + uint8_t i2c_data[16];
> +
> uint32_t uart0_ier;
> uint32_t uart0_lcr;
> uint32_t uart0_mcr;
> @@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + SM501State *s = (SM501State *)opaque;
> + uint8_t ret = 0;
> +
> + switch (addr) {
> + case SM501_I2C_BYTE_COUNT:
> + ret = s->i2c_byte_count;
> + break;
> + case SM501_I2C_STATUS:
> + ret = s->i2c_status;
> + break;
> + case SM501_I2C_SLAVE_ADDRESS:
> + ret = s->i2c_addr;
> + break;
> + case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> + ret = s->i2c_data[addr - SM501_I2C_DATA];
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
> + " addr=0x%" HWADDR_PRIx "\n", addr);
> + }
> +
> + SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
> + addr, ret);
> + return ret;
> +}
> +
> +static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
> + unsigned size)
> +{
> + SM501State *s = (SM501State *)opaque;
> + SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
> + " val=%" PRIx64 "\n", addr, value);
> +
> + switch (addr) {
> + case SM501_I2C_BYTE_COUNT:
> + s->i2c_byte_count = value & 0xf;
> + break;
> + case SM501_I2C_CONTROL:
> + if (value & 1) {
> + if (value & 4) {
> + int res = i2c_start_transfer(s->i2c_bus,
> + s->i2c_addr >> 1,
> + s->i2c_addr & 1);
> + s->i2c_status |= (res ? 1 << 2 : 0);
> + if (!res) {
> + int i;
> + SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
> + s->i2c_byte_count + 1, s->i2c_addr >> 1);
> + for (i = 0; i <= s->i2c_byte_count; i++) {
> + res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
> + !(s->i2c_addr & 1));
> + if (res) {
> + SM501_DPRINTF("sm501 i2c : transfer failed"
> + " i=%d, res=%d\n", i, res);
> + s->i2c_status |= (res ? 1 << 2 : 0);
> + return;
> + }
> + }
> + if (i) {
> + SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
> + s->i2c_status = 8;
> + }
> + }
> + } else {
> + SM501_DPRINTF("sm501 i2c : end transfer\n");
> + i2c_end_transfer(s->i2c_bus);
> + s->i2c_status &= ~4;
> + }
> + }
> + break;
> + case SM501_I2C_RESET:
> + s->i2c_status &= ~4;
> + break;
> + case SM501_I2C_SLAVE_ADDRESS:
> + s->i2c_addr = value & 0xff;
> + break;
> + case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> + s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
> + "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
> + }
> +}
> +
> +static const MemoryRegionOps sm501_i2c_ops = {
> + .read = sm501_i2c_read,
> + .write = sm501_i2c_write,
> + .valid = {
> + .min_access_size = 1,
> + .max_access_size = 4,
> + },
> + .impl = {
> + .min_access_size = 1,
> + .max_access_size = 1,
> + },
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
> {
> SM501State *s = (SM501State *)opaque;
> @@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
> s->irq_mask = 0;
> s->misc_timing = 0;
> s->power_mode_control = 0;
> + s->i2c_byte_count = 0;
> + s->i2c_status = 0;
> + s->i2c_addr = 0;
> + memset(s->i2c_data, 0, 16);
> s->dc_panel_control = 0x00010000; /* FIFO level 3 */
> s->dc_video_control = 0;
> s->dc_crt_control = 0x00010000;
> @@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
> memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
> s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>
> + /* i2c */
> + s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
> + I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
> + i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
> +
> /* mmio */
> memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
> memory_region_init_io(&s->system_config_region, OBJECT(dev),
> @@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
> "sm501-system-config", 0x6c);
> memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
> &s->system_config_region);
> + memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
> + "sm501-i2c", 0x14);
> + memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
> memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
> &sm501_disp_ctrl_ops, s,
> "sm501-disp-ctrl", 0x1000);
> @@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
> VMSTATE_UINT32(twoD_destination_base, SM501State),
> VMSTATE_UINT32(twoD_alpha, SM501State),
> VMSTATE_UINT32(twoD_wrap, SM501State),
> + /* Added in version 2 */
> + VMSTATE_UINT8(i2c_byte_count, SM501State),
> + VMSTATE_UINT8(i2c_status, SM501State),
> + VMSTATE_UINT8(i2c_addr, SM501State),
> + VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
> VMSTATE_END_OF_LIST()
> }
> };
> @@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
>
> static const VMStateDescription vmstate_sm501_sysbus = {
> .name = TYPE_SYSBUS_SM501,
> - .version_id = 1,
> - .minimum_version_id = 1,
> + .version_id = 2,
> + .minimum_version_id = 2,
> .fields = (VMStateField[]) {
> VMSTATE_STRUCT(state, SM501SysBusState, 1,
> vmstate_sm501_state, SM501State),
> @@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
>
> static const VMStateDescription vmstate_sm501_pci = {
> .name = TYPE_PCI_SM501,
> - .version_id = 1,
> - .minimum_version_id = 1,
> + .version_id = 2,
> + .minimum_version_id = 2,
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
> VMSTATE_STRUCT(state, SM501PCIState, 1,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID
2018-06-20 7:17 ` David Gibson
@ 2018-06-22 12:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-22 12:00 UTC (permalink / raw)
To: David Gibson, BALATON Zoltan; +Cc: qemu-ppc, qemu-devel, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 11649 bytes --]
On 06/20/2018 04:17 AM, David Gibson wrote:
> On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
>> Emulate the i2c part of SM501 which is used to access the EDID info
>> from a monitor.
>>
>> The vmstate structure is changed and its version is increased but
>> SM501 is only used on SH and PPC sam460ex machines that don't support
>> cross-version migration.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> v4: Updated commit message
>
> This patch breaks compile on SH - sm501 now needs various i2c symbols
> that aren't configured into the SH builds.
>
> As a rule, it's always wise to try compiling with a bare ./configure
> before you post to make sure you didn't break some platform you
> weren't thinking about. Doing a "make check" like that is even better.
Or if you lack horsepower and have time (~2h), push your branch to a
github repo with Travis CI enabled, and Travis will build/check testing
all for you.
>
>>
>> default-configs/ppc-softmmu.mak | 1 +
>> default-configs/ppcemb-softmmu.mak | 1 +
>> default-configs/sh4-softmmu.mak | 1 +
>> default-configs/sh4eb-softmmu.mak | 1 +
>> hw/display/sm501.c | 136 +++++++++++++++++++++++++++++++++++--
>> 5 files changed, 136 insertions(+), 4 deletions(-)
>>
>> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
>> index b8b0526..e131e24 100644
>> --- a/default-configs/ppc-softmmu.mak
>> +++ b/default-configs/ppc-softmmu.mak
>> @@ -24,6 +24,7 @@ CONFIG_ETSEC=y
>> # For Sam460ex
>> CONFIG_USB_EHCI_SYSBUS=y
>> CONFIG_SM501=y
>> +CONFIG_DDC=y
>> CONFIG_IDE_SII3112=y
>> CONFIG_I2C=y
>> CONFIG_BITBANG_I2C=y
>> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
>> index 37af193..ac44f15 100644
>> --- a/default-configs/ppcemb-softmmu.mak
>> +++ b/default-configs/ppcemb-softmmu.mak
>> @@ -17,6 +17,7 @@ CONFIG_XILINX=y
>> CONFIG_XILINX_ETHLITE=y
>> CONFIG_USB_EHCI_SYSBUS=y
>> CONFIG_SM501=y
>> +CONFIG_DDC=y
>> CONFIG_IDE_SII3112=y
>> CONFIG_I2C=y
>> CONFIG_BITBANG_I2C=y
>> diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
>> index 546d855..72d8fca 100644
>> --- a/default-configs/sh4-softmmu.mak
>> +++ b/default-configs/sh4-softmmu.mak
>> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
>> CONFIG_SH4=y
>> CONFIG_IDE_MMIO=y
>> CONFIG_SM501=y
>> +CONFIG_DDC=y
>> CONFIG_ISA_TESTDEV=y
>> CONFIG_I82378=y
>> CONFIG_I8259=y
>> diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
>> index 2d3fd49..c686637 100644
>> --- a/default-configs/sh4eb-softmmu.mak
>> +++ b/default-configs/sh4eb-softmmu.mak
>> @@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
>> CONFIG_SH4=y
>> CONFIG_IDE_MMIO=y
>> CONFIG_SM501=y
>> +CONFIG_DDC=y
>> CONFIG_ISA_TESTDEV=y
>> CONFIG_I82378=y
>> CONFIG_I8259=y
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 8206ae8..a076248 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -26,6 +26,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu/cutils.h"
>> #include "qapi/error.h"
>> +#include "qemu/log.h"
>> #include "qemu-common.h"
>> #include "cpu.h"
>> #include "hw/hw.h"
>> @@ -34,6 +35,8 @@
>> #include "hw/devices.h"
>> #include "hw/sysbus.h"
>> #include "hw/pci/pci.h"
>> +#include "hw/i2c/i2c.h"
>> +#include "hw/i2c/i2c-ddc.h"
>> #include "qemu/range.h"
>> #include "ui/pixel_ops.h"
>>
>> @@ -471,10 +474,12 @@ typedef struct SM501State {
>> MemoryRegion local_mem_region;
>> MemoryRegion mmio_region;
>> MemoryRegion system_config_region;
>> + MemoryRegion i2c_region;
>> MemoryRegion disp_ctrl_region;
>> MemoryRegion twoD_engine_region;
>> uint32_t last_width;
>> uint32_t last_height;
>> + I2CBus *i2c_bus;
>>
>> /* mmio registers */
>> uint32_t system_control;
>> @@ -487,6 +492,11 @@ typedef struct SM501State {
>> uint32_t misc_timing;
>> uint32_t power_mode_control;
>>
>> + uint8_t i2c_byte_count;
>> + uint8_t i2c_status;
>> + uint8_t i2c_addr;
>> + uint8_t i2c_data[16];
>> +
>> uint32_t uart0_ier;
>> uint32_t uart0_lcr;
>> uint32_t uart0_mcr;
>> @@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> +static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> + SM501State *s = (SM501State *)opaque;
>> + uint8_t ret = 0;
>> +
>> + switch (addr) {
>> + case SM501_I2C_BYTE_COUNT:
>> + ret = s->i2c_byte_count;
>> + break;
>> + case SM501_I2C_STATUS:
>> + ret = s->i2c_status;
>> + break;
>> + case SM501_I2C_SLAVE_ADDRESS:
>> + ret = s->i2c_addr;
>> + break;
>> + case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
>> + ret = s->i2c_data[addr - SM501_I2C_DATA];
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
>> + " addr=0x%" HWADDR_PRIx "\n", addr);
>> + }
>> +
>> + SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
>> + addr, ret);
>> + return ret;
>> +}
>> +
>> +static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
>> + unsigned size)
>> +{
>> + SM501State *s = (SM501State *)opaque;
>> + SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
>> + " val=%" PRIx64 "\n", addr, value);
>> +
>> + switch (addr) {
>> + case SM501_I2C_BYTE_COUNT:
>> + s->i2c_byte_count = value & 0xf;
>> + break;
>> + case SM501_I2C_CONTROL:
>> + if (value & 1) {
>> + if (value & 4) {
>> + int res = i2c_start_transfer(s->i2c_bus,
>> + s->i2c_addr >> 1,
>> + s->i2c_addr & 1);
>> + s->i2c_status |= (res ? 1 << 2 : 0);
>> + if (!res) {
>> + int i;
>> + SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
>> + s->i2c_byte_count + 1, s->i2c_addr >> 1);
>> + for (i = 0; i <= s->i2c_byte_count; i++) {
>> + res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
>> + !(s->i2c_addr & 1));
>> + if (res) {
>> + SM501_DPRINTF("sm501 i2c : transfer failed"
>> + " i=%d, res=%d\n", i, res);
>> + s->i2c_status |= (res ? 1 << 2 : 0);
>> + return;
>> + }
>> + }
>> + if (i) {
>> + SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
>> + s->i2c_status = 8;
>> + }
>> + }
>> + } else {
>> + SM501_DPRINTF("sm501 i2c : end transfer\n");
>> + i2c_end_transfer(s->i2c_bus);
>> + s->i2c_status &= ~4;
>> + }
>> + }
>> + break;
>> + case SM501_I2C_RESET:
>> + s->i2c_status &= ~4;
>> + break;
>> + case SM501_I2C_SLAVE_ADDRESS:
>> + s->i2c_addr = value & 0xff;
>> + break;
>> + case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
>> + s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
>> + "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
>> + }
>> +}
>> +
>> +static const MemoryRegionOps sm501_i2c_ops = {
>> + .read = sm501_i2c_read,
>> + .write = sm501_i2c_write,
>> + .valid = {
>> + .min_access_size = 1,
>> + .max_access_size = 4,
>> + },
>> + .impl = {
>> + .min_access_size = 1,
>> + .max_access_size = 1,
>> + },
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
>> {
>> SM501State *s = (SM501State *)opaque;
>> @@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
>> s->irq_mask = 0;
>> s->misc_timing = 0;
>> s->power_mode_control = 0;
>> + s->i2c_byte_count = 0;
>> + s->i2c_status = 0;
>> + s->i2c_addr = 0;
>> + memset(s->i2c_data, 0, 16);
>> s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>> s->dc_video_control = 0;
>> s->dc_crt_control = 0x00010000;
>> @@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>> memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>> s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>>
>> + /* i2c */
>> + s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
>> + I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>> + i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
>> +
>> /* mmio */
>> memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>> memory_region_init_io(&s->system_config_region, OBJECT(dev),
>> @@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>> "sm501-system-config", 0x6c);
>> memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
>> &s->system_config_region);
>> + memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
>> + "sm501-i2c", 0x14);
>> + memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
>> memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
>> &sm501_disp_ctrl_ops, s,
>> "sm501-disp-ctrl", 0x1000);
>> @@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
>> VMSTATE_UINT32(twoD_destination_base, SM501State),
>> VMSTATE_UINT32(twoD_alpha, SM501State),
>> VMSTATE_UINT32(twoD_wrap, SM501State),
>> + /* Added in version 2 */
>> + VMSTATE_UINT8(i2c_byte_count, SM501State),
>> + VMSTATE_UINT8(i2c_status, SM501State),
>> + VMSTATE_UINT8(i2c_addr, SM501State),
>> + VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
>> VMSTATE_END_OF_LIST()
>> }
>> };
>> @@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
>>
>> static const VMStateDescription vmstate_sm501_sysbus = {
>> .name = TYPE_SYSBUS_SM501,
>> - .version_id = 1,
>> - .minimum_version_id = 1,
>> + .version_id = 2,
>> + .minimum_version_id = 2,
>> .fields = (VMStateField[]) {
>> VMSTATE_STRUCT(state, SM501SysBusState, 1,
>> vmstate_sm501_state, SM501State),
>> @@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
>>
>> static const VMStateDescription vmstate_sm501_pci = {
>> .name = TYPE_PCI_SM501,
>> - .version_id = 1,
>> - .minimum_version_id = 1,
>> + .version_id = 2,
>> + .minimum_version_id = 2,
>> .fields = (VMStateField[]) {
>> VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
>> VMSTATE_STRUCT(state, SM501PCIState, 1,
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (5 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-20 0:14 ` David Gibson
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
` (4 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 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: 3774 bytes --]
We don't emulate slave mode so related registers are not needed.
[lh]sadr are only retained to avoid too many warnings and simplify
debugging but sdata is not even correct because device has a 4 byte
FIFO instead so just remove this unimplemented register for now.
The intr register is also not implemented correctly, it is for
diagnostics and normally not even visible on device without explicitly
enabling it. As no guests are known to need this remove it as well.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v4: Updated commit message
hw/i2c/ppc4xx_i2c.c | 16 +---------------
include/hw/i2c/ppc4xx_i2c.h | 4 +---
2 files changed, 2 insertions(+), 18 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index d1936db..4e0aaae 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
@@ -63,7 +63,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 +70,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)
@@ -139,9 +137,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 +176,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%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
}
}
break;
- case 2:
- i2c->sdata = value;
- break;
case 4:
i2c->lmadr = value;
if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,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%"
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index 3c60307..e4b6ded 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
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
uint8_t mdcntl;
uint8_t sts;
uint8_t extsts;
- uint8_t sdata;
uint8_t lsadr;
uint8_t hsadr;
uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers BALATON Zoltan
@ 2018-06-20 0:14 ` David Gibson
0 siblings, 0 replies; 28+ messages in thread
From: David Gibson @ 2018-06-20 0:14 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 4405 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> We don't emulate slave mode so related registers are not needed.
> [lh]sadr are only retained to avoid too many warnings and simplify
> debugging but sdata is not even correct because device has a 4 byte
> FIFO instead so just remove this unimplemented register for now.
>
> The intr register is also not implemented correctly, it is for
> diagnostics and normally not even visible on device without explicitly
> enabling it. As no guests are known to need this remove it as well.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v4: Updated commit message
Applied to ppc-for-3.0, thanks.
>
> hw/i2c/ppc4xx_i2c.c | 16 +---------------
> include/hw/i2c/ppc4xx_i2c.h | 4 +---
> 2 files changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d1936db..4e0aaae 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
> @@ -63,7 +63,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 +70,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)
> @@ -139,9 +137,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 +176,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%"
> @@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> }
> }
> break;
> - case 2:
> - i2c->sdata = value;
> - break;
> case 4:
> i2c->lmadr = value;
> if (i2c_bus_busy(i2c->bus)) {
> @@ -302,9 +291,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%"
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index 3c60307..e4b6ded 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
> @@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
> uint8_t mdcntl;
> uint8_t sts;
> uint8_t extsts;
> - uint8_t sdata;
> uint8_t lsadr;
> uint8_t hsadr;
> uint8_t clkdiv;
> @@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
> 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] 28+ messages in thread
* [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (6 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-20 5:25 ` David Gibson
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 11/11] sm501: Fix support for non-zero frame buffer start address BALATON Zoltan
` (3 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 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>
---
hw/i2c/ppc4xx_i2c.c | 222 +++++++++++++++++++++-----------------------
include/hw/i2c/ppc4xx_i2c.h | 3 +-
2 files changed, 110 insertions(+), 115 deletions(-)
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index fca80d6..3ebce17 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)
@@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
{
PPC4xxI2CState *i2c = PPC4xx_I2C(s);
- /* 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;
@@ -78,69 +83,29 @@ 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 *i2c = PPC4xx_I2C(opaque);
uint64_t ret;
+ int i;
switch (addr) {
case 0:
- ret = i2c->mdata;
- if (ppc4xx_i2c_is_master(i2c)) {
+ 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(i2c->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(i2c->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)) {
- /* 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:
@@ -160,6 +125,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
break;
case 9:
ret = i2c->extsts;
+ ret |= !!i2c_bus_busy(i2c->bus) << 4;
break;
case 10:
ret = i2c->lsadr;
@@ -203,70 +169,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
switch (addr) {
case 0:
- i2c->mdata = value;
- if (!i2c_bus_busy(i2c->bus)) {
- /* assume we start a write transfer */
- if (i2c_start_transfer(i2c->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(i2c->bus)) {
- if (i2c_send(i2c->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->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(i2c->bus)) {
- i2c_end_transfer(i2c->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(i2c->bus)) {
- /* end previous transfer */
- i2c->sts &= ~IIC_STS_PT;
- i2c_end_transfer(i2c->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(i2c->bus)) {
+ i2c_end_transfer(i2c->bus);
+ if (i2c->mdcntl & IIC_MDCNTL_EINT &&
+ i2c->intrmsk & IIC_INTRMSK_EIHE) {
+ i2c->sts |= IIC_STS_IRQA;
+ qemu_irq_raise(i2c->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(i2c->bus)) {
+ i2c->extsts = (1 << 6);
+ if (i2c_start_transfer(i2c->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(i2c->bus, &i2c->mdata[i], !recv)) {
+ i2c->sts |= IIC_STS_ERR;
+ i2c->extsts |= IIC_EXTSTS_XFRA;
+ break;
}
- if (i2c_start_transfer(i2c->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(i2c->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(i2c->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);
+ i2c->sts &= ~(value & 0x0a);
+ if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
+ qemu_irq_lower(i2c->irq);
+ }
break;
case 9:
i2c->extsts &= ~(value & 0x8f);
@@ -287,12 +281,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(i2c));
break;
}
- i2c->xtcntlss = value;
break;
case 16:
i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
index ea6c8e1..0891a9c 100644
--- a/include/hw/i2c/ppc4xx_i2c.h
+++ b/include/hw/i2c/ppc4xx_i2c.h
@@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
qemu_irq irq;
MemoryRegion iomem;
bitbang_i2c_interface *bitbang;
- uint8_t mdata;
+ int mdidx;
+ uint8_t mdata[4];
uint8_t lmadr;
uint8_t hmadr;
uint8_t cntl;
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
@ 2018-06-20 5:25 ` David Gibson
2018-06-21 6:51 ` BALATON Zoltan
0 siblings, 1 reply; 28+ messages in thread
From: David Gibson @ 2018-06-20 5:25 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 13345 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> 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>
Ugh. This is still a large enough change that it's pretty difficult
to review, at least for someone not already familiar with the hardware.
> ---
> hw/i2c/ppc4xx_i2c.c | 222 +++++++++++++++++++++-----------------------
> include/hw/i2c/ppc4xx_i2c.h | 3 +-
> 2 files changed, 110 insertions(+), 115 deletions(-)
>
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index fca80d6..3ebce17 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)
> @@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
> {
> PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>
> - /* 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;
> @@ -78,69 +83,29 @@ 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 *i2c = PPC4xx_I2C(opaque);
> uint64_t ret;
> + int i;
>
> switch (addr) {
> case 0:
> - ret = i2c->mdata;
> - if (ppc4xx_i2c_is_master(i2c)) {
> + 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(i2c->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(i2c->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)) {
> - /* 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:
> @@ -160,6 +125,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
> break;
> case 9:
> ret = i2c->extsts;
> + ret |= !!i2c_bus_busy(i2c->bus) << 4;
> break;
> case 10:
> ret = i2c->lsadr;
> @@ -203,70 +169,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>
> switch (addr) {
> case 0:
> - i2c->mdata = value;
> - if (!i2c_bus_busy(i2c->bus)) {
> - /* assume we start a write transfer */
> - if (i2c_start_transfer(i2c->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(i2c->bus)) {
> - if (i2c_send(i2c->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->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(i2c->bus)) {
> - i2c_end_transfer(i2c->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(i2c->bus)) {
> - /* end previous transfer */
> - i2c->sts &= ~IIC_STS_PT;
> - i2c_end_transfer(i2c->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(i2c->bus)) {
> + i2c_end_transfer(i2c->bus);
> + if (i2c->mdcntl & IIC_MDCNTL_EINT &&
> + i2c->intrmsk & IIC_INTRMSK_EIHE) {
> + i2c->sts |= IIC_STS_IRQA;
> + qemu_irq_raise(i2c->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(i2c->bus)) {
> + i2c->extsts = (1 << 6);
> + if (i2c_start_transfer(i2c->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(i2c->bus, &i2c->mdata[i], !recv)) {
> + i2c->sts |= IIC_STS_ERR;
> + i2c->extsts |= IIC_EXTSTS_XFRA;
> + break;
> }
> - if (i2c_start_transfer(i2c->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(i2c->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(i2c->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);
> + i2c->sts &= ~(value & 0x0a);
> + if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
> + qemu_irq_lower(i2c->irq);
> + }
> break;
> case 9:
> i2c->extsts &= ~(value & 0x8f);
> @@ -287,12 +281,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(i2c));
> break;
> }
> - i2c->xtcntlss = value;
> break;
> case 16:
> i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index ea6c8e1..0891a9c 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
> qemu_irq irq;
> MemoryRegion iomem;
> bitbang_i2c_interface *bitbang;
> - uint8_t mdata;
> + int mdidx;
> + uint8_t mdata[4];
> uint8_t lmadr;
> uint8_t hmadr;
> uint8_t cntl;
--
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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-20 5:25 ` David Gibson
@ 2018-06-21 6:51 ` BALATON Zoltan
0 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-21 6:51 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 10:52:15AM +0200, BALATON Zoltan wrote:
>> 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>
>
> Ugh. This is still a large enough change that it's pretty difficult
> to review, at least for someone not already familiar with the hardware.
Then what to do? Does it worth the effort trying to break this up more
when without knowledge about the hardware it's not possible to review even
if it's smaller patches? Also this touches a device which was not emulated
before at all, then patched to make U-Boot happy (see:
http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00193.html) and
this is now the first version that really tries to emulate the device so
that guest OSes can also use it. Considering that it's only used on some
not really maintained or tested SoC emulations what risk is to take this
and then fix it if problem found later? Holding on to a known wrong
emulation just because no one knows better does not help evolve this
device emulation.
Maybe I can try to enhance commit message to list what missing features of
the device this tries to add if that helps. These are basically:
- mdata is a 4 byte FIFO
- device should generate interrupts
Regards,
BALATON Zoltan
>> ---
>> hw/i2c/ppc4xx_i2c.c | 222 +++++++++++++++++++++-----------------------
>> include/hw/i2c/ppc4xx_i2c.h | 3 +-
>> 2 files changed, 110 insertions(+), 115 deletions(-)
>>
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index fca80d6..3ebce17 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)
>> @@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>> {
>> PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>>
>> - /* 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;
>> @@ -78,69 +83,29 @@ 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 *i2c = PPC4xx_I2C(opaque);
>> uint64_t ret;
>> + int i;
>>
>> switch (addr) {
>> case 0:
>> - ret = i2c->mdata;
>> - if (ppc4xx_i2c_is_master(i2c)) {
>> + 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(i2c->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(i2c->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)) {
>> - /* 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:
>> @@ -160,6 +125,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>> break;
>> case 9:
>> ret = i2c->extsts;
>> + ret |= !!i2c_bus_busy(i2c->bus) << 4;
>> break;
>> case 10:
>> ret = i2c->lsadr;
>> @@ -203,70 +169,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>
>> switch (addr) {
>> case 0:
>> - i2c->mdata = value;
>> - if (!i2c_bus_busy(i2c->bus)) {
>> - /* assume we start a write transfer */
>> - if (i2c_start_transfer(i2c->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(i2c->bus)) {
>> - if (i2c_send(i2c->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->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(i2c->bus)) {
>> - i2c_end_transfer(i2c->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(i2c->bus)) {
>> - /* end previous transfer */
>> - i2c->sts &= ~IIC_STS_PT;
>> - i2c_end_transfer(i2c->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(i2c->bus)) {
>> + i2c_end_transfer(i2c->bus);
>> + if (i2c->mdcntl & IIC_MDCNTL_EINT &&
>> + i2c->intrmsk & IIC_INTRMSK_EIHE) {
>> + i2c->sts |= IIC_STS_IRQA;
>> + qemu_irq_raise(i2c->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(i2c->bus)) {
>> + i2c->extsts = (1 << 6);
>> + if (i2c_start_transfer(i2c->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(i2c->bus, &i2c->mdata[i], !recv)) {
>> + i2c->sts |= IIC_STS_ERR;
>> + i2c->extsts |= IIC_EXTSTS_XFRA;
>> + break;
>> }
>> - if (i2c_start_transfer(i2c->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(i2c->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(i2c->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);
>> + i2c->sts &= ~(value & 0x0a);
>> + if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
>> + qemu_irq_lower(i2c->irq);
>> + }
>> break;
>> case 9:
>> i2c->extsts &= ~(value & 0x8f);
>> @@ -287,12 +281,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(i2c));
>> break;
>> }
>> - i2c->xtcntlss = value;
>> break;
>> case 16:
>> i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
>> index ea6c8e1..0891a9c 100644
>> --- a/include/hw/i2c/ppc4xx_i2c.h
>> +++ b/include/hw/i2c/ppc4xx_i2c.h
>> @@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
>> qemu_irq irq;
>> MemoryRegion iomem;
>> bitbang_i2c_interface *bitbang;
>> - uint8_t mdata;
>> + int mdidx;
>> + uint8_t mdata[4];
>> uint8_t lmadr;
>> uint8_t hmadr;
>> uint8_t cntl;
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH v4 11/11] sm501: Fix support for non-zero frame buffer start address
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (7 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 10/11] sm501: Set updated region dirty after 2D operation BALATON Zoltan
` (2 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson, Sebastian Bauer
Display updates and drawing hardware cursor did not work when frame
buffer address was non-zero. Fix this by taking the frame buffer
address into account in these cases. This fixes screen dragging on
AmigaOS. Based on patch by Sebastian Bauer.
Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index ad518ce..55d8193 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -578,6 +578,11 @@ static uint32_t get_local_mem_size_index(uint32_t size)
return index;
}
+static ram_addr_t get_fb_addr(SM501State *s, int crt)
+{
+ return crt ? s->dc_crt_fb_addr : s->dc_panel_fb_addr;
+}
+
static inline int get_width(SM501State *s, int crt)
{
int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
@@ -680,7 +685,8 @@ static inline void hwc_invalidate(SM501State *s, int crt)
start *= w * bpp;
end *= w * bpp;
- memory_region_set_dirty(&s->local_mem_region, start, end - start);
+ memory_region_set_dirty(&s->local_mem_region,
+ get_fb_addr(s, crt) + start, end - start);
}
static void sm501_2d_operation(SM501State *s)
@@ -1577,7 +1583,7 @@ static void sm501_update_display(void *opaque)
draw_hwc_line_func *draw_hwc_line = NULL;
int full_update = 0;
int y_start = -1;
- ram_addr_t offset = 0;
+ ram_addr_t offset;
uint32_t *palette;
uint8_t hwc_palette[3 * 3];
uint8_t *hwc_src = NULL;
@@ -1634,9 +1640,10 @@ static void sm501_update_display(void *opaque)
}
/* draw each line according to conditions */
+ offset = get_fb_addr(s, crt);
snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
- for (y = 0, offset = 0; y < height; y++, offset += width * src_bpp) {
+ for (y = 0; y < height; y++, offset += width * src_bpp) {
int update, update_hwc;
/* check if hardware cursor is enabled and we're within its range */
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v4 10/11] sm501: Set updated region dirty after 2D operation
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (8 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 11/11] sm501: Fix support for non-zero frame buffer start address BALATON Zoltan
@ 2018-06-19 8:52 ` BALATON Zoltan
2018-06-19 9:12 ` [Qemu-devel] [PATCH v4 05/11] sam460ex: Add RTC device BALATON Zoltan
2018-06-20 7:25 ` [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements David Gibson
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 8:52 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Set the changed memory region dirty after performed a 2D operation to
ensure that the screen is updated properly.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/display/sm501.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e126c15..ad518ce 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -774,6 +774,11 @@ static void sm501_2d_operation(SM501State *s)
abort();
break;
}
+
+ memory_region_set_dirty(&s->local_mem_region,
+ s->twoD_destination_base & 0x03FFFFFF,
+ (dst_y + operation_height) * dst_width +
+ dst_x + operation_width);
}
static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
--
2.7.6
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH v4 05/11] sam460ex: Add RTC device
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (9 preceding siblings ...)
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 10/11] sm501: Set updated region dirty after 2D operation BALATON Zoltan
@ 2018-06-19 9:12 ` BALATON Zoltan
2018-06-20 7:25 ` [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements David Gibson
11 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-19 9:12 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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
` (10 preceding siblings ...)
2018-06-19 9:12 ` [Qemu-devel] [PATCH v4 05/11] sam460ex: Add RTC device BALATON Zoltan
@ 2018-06-20 7:25 ` David Gibson
2018-06-21 7:00 ` BALATON Zoltan
11 siblings, 1 reply; 28+ messages in thread
From: David Gibson @ 2018-06-20 7:25 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, Alexander Graf, Sebastian Bauer
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
>
> Next spin of sam460ex pathces with changes according to review and
> some new patches.
So, the sm501 patches I don't really have the technical knowledge to
review, and I'm a little uncomfortable taking them through my tree,
since they aren't strictly related to ppc.
It looks like you're the only person that's made substantive changes
to sm501 lately, though. So, given I haven't heard screaming, I think
it's fair to assume that either you know what you're doing, or you're
the only person that cares about sm501 anyway.
So, I think we should make you sm501 maintainer.
Specifically I think you should:
* Fix the compile breakage I pointed out elsewhere
* Split out your sm501 patches
* Prepend a patch adding it to MAINTAINERS with yourself as
maintainer (whether you call it "Maintained" or "Odd Fixes" is up
to your level of dedication)
* Send this new series direct to Peter Maydell, CC me, Gerd Hoffman
<kraxel@redhat.com> and the SH maintainer(s) (as well as the lists
and whoever else you think's appropriate)
* I'll reply, making the case above to Peter for your
maintainership.
With any luck, from there on you'll be able to send sm501 pull
requests directly.
--
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] 28+ messages in thread* Re: [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements
2018-06-20 7:25 ` [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements David Gibson
@ 2018-06-21 7:00 ` BALATON Zoltan
0 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2018-06-21 7:00 UTC (permalink / raw)
To: David Gibson
Cc: qemu-devel, qemu-ppc, Peter Maydell, Alexander Graf, Sebastian Bauer
Hello,
On Wed, 20 Jun 2018, David Gibson wrote:
> On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
>> Next spin of sam460ex pathces with changes according to review and
>> some new patches.
>
> So, the sm501 patches I don't really have the technical knowledge to
> review, and I'm a little uncomfortable taking them through my tree,
> since they aren't strictly related to ppc.
OK, since the sm501 is also used on SH it's not PPC only so I think you
can ignore them for now and just take the other patches and I can submit
the sm501 related patches as a separate series (although all these changes
are to improve sam460ex emulation).
> It looks like you're the only person that's made substantive changes
> to sm501 lately, though. So, given I haven't heard screaming, I think
> it's fair to assume that either you know what you're doing, or you're
> the only person that cares about sm501 anyway.
>
> So, I think we should make you sm501 maintainer.
I don't feel confident enough to take on maintainership of this device but
hope maybe Peter could help as he did before to get the patches in not
involving your tree.
> Specifically I think you should:
> * Fix the compile breakage I pointed out elsewhere
Thanks, I'll check and fix this.
> * Split out your sm501 patches
OK, as above I'll send these as separate series, you can ignore all sm501:
prefixed patches from this v4 series.
> * Prepend a patch adding it to MAINTAINERS with yourself as
> maintainer (whether you call it "Maintained" or "Odd Fixes" is up
> to your level of dedication)
> * Send this new series direct to Peter Maydell, CC me, Gerd Hoffman
> <kraxel@redhat.com> and the SH maintainer(s) (as well as the lists
> and whoever else you think's appropriate)
> * I'll reply, making the case above to Peter for your
> maintainership.
>
> With any luck, from there on you'll be able to send sm501 pull
> requests directly.
But if there's an other way I'd rather cc those people for review and to
take care of pull requests without me becoming maintainer as I don't have
any experience with that.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread