* [Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device
2018-06-24 11:20 [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
@ 2018-06-24 11:20 ` BALATON Zoltan
2018-06-24 17:55 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation BALATON Zoltan
3 siblings, 1 reply; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 11:20 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] 15+ messages in thread
* [Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation
2018-06-24 11:20 [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements BALATON Zoltan
` (2 preceding siblings ...)
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device BALATON Zoltan
@ 2018-06-24 11:20 ` BALATON Zoltan
2018-06-24 17:55 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
3 siblings, 1 reply; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 11:20 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
of day is implemented. Setting time and RTC alarm are not supported.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
MAINTAINERS | 1 +
default-configs/ppc-softmmu.mak | 1 +
hw/timer/Makefile.objs | 1 +
hw/timer/m41t80.c | 117 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 120 insertions(+)
create mode 100644 hw/timer/m41t80.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 2874ddc..3bfc4a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -839,6 +839,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] 15+ messages in thread
* [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements
@ 2018-06-24 11:20 BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller BALATON Zoltan
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 11:20 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
These are the remaining patches for sam460ex needed to implement RTC
and get AmigaOS to boot. The sm501 patches are also needed but that's
now a separate series. I'd appreciate if this could be reviewed and
merged before the imminent 3.0 freeze.
I think only patch 1/4 (i2c rewrite) is a bit more complex but given
that originally it was only a stub and previous patches just hacked it
for u-boot IMO it does not worth trying to make incremental changes to
the previous nonsense behaviour instead of replacing it with a more
proper implementation which is what this patch does. Basically it
implements the 4 byte FIFO for mdata register and implements
interrupts the device should generate. Considering that this device is
only used on some not too well maintained boards it should be low risk
to make this change.
Patches 1-3 are for RTC emulation, patch 4 is new and is needed by
AmigaOS.
Regards,
BALATON Zoltan
BALATON Zoltan (4):
ppc4xx_i2c: Rewrite to model hardware more closely
hw/timer: Add basic M41T80 emulation
sam460ex: Add RTC device
ppc440_uc: Basic emulation of PPC440 DMA controller
MAINTAINERS | 1 +
default-configs/ppc-softmmu.mak | 1 +
hw/i2c/ppc4xx_i2c.c | 222 +++++++++++++++++++---------------------
hw/ppc/ppc440.h | 1 +
hw/ppc/ppc440_uc.c | 215 ++++++++++++++++++++++++++++++++++++++
hw/ppc/sam460ex.c | 4 +
hw/timer/Makefile.objs | 1 +
hw/timer/m41t80.c | 117 +++++++++++++++++++++
include/hw/i2c/ppc4xx_i2c.h | 3 +-
9 files changed, 450 insertions(+), 115 deletions(-)
create mode 100644 hw/timer/m41t80.c
--
2.7.6
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller
2018-06-24 11:20 [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements BALATON Zoltan
@ 2018-06-24 11:20 ` BALATON Zoltan
2018-06-28 10:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 11:20 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: Alexander Graf, David Gibson
PPC440 SoCs such as the AMCC 460EX have a DMA controller which is used
by AmigaOS on the sam460ex. Implement the parts used by AmigaOS so it
can get further booting on the sam460ex machine.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/ppc/ppc440.h | 1 +
hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/sam460ex.c | 3 +
3 files changed, 219 insertions(+)
diff --git a/hw/ppc/ppc440.h b/hw/ppc/ppc440.h
index ad27db1..7cef936 100644
--- a/hw/ppc/ppc440.h
+++ b/hw/ppc/ppc440.h
@@ -21,6 +21,7 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks,
hwaddr *ram_bases, hwaddr *ram_sizes,
int do_init);
void ppc4xx_ahb_init(CPUPPCState *env);
+void ppc4xx_dma_init(CPUPPCState *env, int dcr_base);
void ppc460ex_pcie_init(CPUPPCState *env);
#endif /* PPC440_H */
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 123f4ac..38aadd9 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -13,6 +13,7 @@
#include "qemu/cutils.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "qemu/log.h"
#include "cpu.h"
#include "hw/hw.h"
#include "exec/address-spaces.h"
@@ -803,6 +804,220 @@ void ppc4xx_ahb_init(CPUPPCState *env)
}
/*****************************************************************************/
+/* DMA controller */
+
+#define DMA0_CR_CE (1 << 31)
+#define DMA0_CR_PW (1 << 26 | 1 << 25)
+#define DMA0_CR_DAI (1 << 24)
+#define DMA0_CR_SAI (1 << 23)
+#define DMA0_CR_DEC (1 << 2)
+
+enum {
+ DMA0_CR = 0x00,
+ DMA0_CT,
+ DMA0_SAH,
+ DMA0_SAL,
+ DMA0_DAH,
+ DMA0_DAL,
+ DMA0_SGH,
+ DMA0_SGL,
+
+ DMA0_SR = 0x20,
+ DMA0_SGC = 0x23,
+ DMA0_SLP = 0x25,
+ DMA0_POL = 0x26,
+};
+
+typedef struct {
+ uint32_t cr;
+ uint32_t ct;
+ uint64_t sa;
+ uint64_t da;
+ uint64_t sg;
+} ppc4xx_dma_ch_t;
+
+typedef struct {
+ int base;
+ ppc4xx_dma_ch_t ch[4];
+ uint32_t sr;
+} ppc4xx_dma_t;
+
+static uint32_t dcr_read_dma(void *opaque, int dcrn)
+{
+ ppc4xx_dma_t *dma = opaque;
+ uint32_t val = 0;
+ int addr = dcrn - dma->base;
+ int chnl = addr / 8;
+
+ switch (addr) {
+ case 0x00 ... 0x1f:
+ switch (addr % 8) {
+ case DMA0_CR:
+ val = dma->ch[chnl].cr;
+ break;
+ case DMA0_CT:
+ val = dma->ch[chnl].ct;
+ break;
+ case DMA0_SAH:
+ val = dma->ch[chnl].sa >> 32;
+ break;
+ case DMA0_SAL:
+ val = dma->ch[chnl].sa;
+ break;
+ case DMA0_DAH:
+ val = dma->ch[chnl].da >> 32;
+ break;
+ case DMA0_DAL:
+ val = dma->ch[chnl].da;
+ break;
+ case DMA0_SGH:
+ val = dma->ch[chnl].sg >> 32;
+ break;
+ case DMA0_SGL:
+ val = dma->ch[chnl].sg;
+ break;
+ }
+ break;
+ case DMA0_SR:
+ val = dma->sr;
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
+ __func__, dcrn, chnl, addr);
+ }
+
+ return val;
+}
+
+static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
+{
+ ppc4xx_dma_t *dma = opaque;
+ int addr = dcrn - dma->base;
+ int chnl = addr / 8;
+
+ switch (addr) {
+ case 0x00 ... 0x1f:
+ switch (addr % 8) {
+ case DMA0_CR:
+ dma->ch[chnl].cr = val;
+ if (val & DMA0_CR_CE) {
+ int count = dma->ch[chnl].ct & 0xffff;
+
+ if (count) {
+ int width, i, sidx, didx;
+ uint8_t *rptr, *wptr;
+ hwaddr rlen, wlen;
+
+ width = 1 << ((val & DMA0_CR_PW) >> 25);
+ rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen, 0);
+ wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen, 1);
+ if (!(val & DMA0_CR_DEC) &&
+ val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
+ /* optimise common case */
+ memmove(wptr, rptr, count * width);
+ sidx = didx = count * width;
+ } else {
+ /* do it the slow way */
+ for (sidx = didx = i = 0; i < count; i++) {
+ uint64_t v = ldn_le_p(rptr + sidx, width);
+ stn_le_p(wptr + didx, width, v);
+ if (val & DMA0_CR_SAI) {
+ sidx += width;
+ }
+ if (val & DMA0_CR_DAI) {
+ didx += width;
+ }
+ }
+ }
+ cpu_physical_memory_unmap(wptr, wlen, 1, didx);
+ cpu_physical_memory_unmap(rptr, rlen, 0, sidx);
+ }
+ }
+ break;
+ case DMA0_CT:
+ dma->ch[chnl].ct = val;
+ break;
+ case DMA0_SAH:
+ dma->ch[chnl].sa &= 0xffffffffULL;
+ dma->ch[chnl].sa |= (uint64_t)val << 32;
+ break;
+ case DMA0_SAL:
+ dma->ch[chnl].sa &= 0xffffffff00000000ULL;
+ dma->ch[chnl].sa |= val;
+ break;
+ case DMA0_DAH:
+ dma->ch[chnl].da &= 0xffffffffULL;
+ dma->ch[chnl].da |= (uint64_t)val << 32;
+ break;
+ case DMA0_DAL:
+ dma->ch[chnl].da &= 0xffffffff00000000ULL;
+ dma->ch[chnl].da |= val;
+ break;
+ case DMA0_SGH:
+ dma->ch[chnl].sg &= 0xffffffffULL;
+ dma->ch[chnl].sg |= (uint64_t)val << 32;
+ break;
+ case DMA0_SGL:
+ dma->ch[chnl].sg &= 0xffffffff00000000ULL;
+ dma->ch[chnl].sg |= val;
+ break;
+ }
+ break;
+ case DMA0_SR:
+ dma->sr &= ~val;
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
+ __func__, dcrn, chnl, addr);
+ }
+}
+
+static void ppc4xx_dma_reset(void *opaque)
+{
+ ppc4xx_dma_t *dma = opaque;
+ int dma_base = dma->base;
+
+ memset(dma, 0, sizeof(*dma));
+ dma->base = dma_base;
+}
+
+void ppc4xx_dma_init(CPUPPCState *env, int dcr_base)
+{
+ ppc4xx_dma_t *dma;
+ int i;
+
+ dma = g_malloc0(sizeof(*dma));
+ dma->base = dcr_base;
+ qemu_register_reset(&ppc4xx_dma_reset, dma);
+ for (i = 0; i < 4; i++) {
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CR,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CT,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAH,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAL,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAH,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAL,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGH,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGL,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ }
+ ppc_dcr_register(env, dcr_base + DMA0_SR,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + DMA0_SGC,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + DMA0_SLP,
+ dma, &dcr_read_dma, &dcr_write_dma);
+ ppc_dcr_register(env, dcr_base + DMA0_POL,
+ dma, &dcr_read_dma, &dcr_write_dma);
+}
+
+/*****************************************************************************/
/* PCI Express controller */
/* FIXME: This is not complete and does not work, only implemented partially
* to allow firmware and guests to find an empty bus. Cards should use PCI.
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index dc730cc..4f9248e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -477,6 +477,9 @@ static void sam460ex_init(MachineState *machine)
/* MAL */
ppc4xx_mal_init(env, 4, 16, &uic[2][3]);
+ /* DMA */
+ ppc4xx_dma_init(env, 0x200);
+
/* 256K of L2 cache as memory */
ppc4xx_l2sram_init(env);
/* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
--
2.7.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-24 11:20 [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller BALATON Zoltan
@ 2018-06-24 11:20 ` BALATON Zoltan
2018-06-24 17:53 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation BALATON Zoltan
3 siblings, 1 reply; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 11:20 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] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
@ 2018-06-24 17:53 ` Cédric Le Goater
2018-06-24 20:37 ` BALATON Zoltan
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-24 17:53 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: David Gibson
On 06/24/2018 01:20 PM, 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.
ppc4xx support was dropped from u-boot but there is some work being done
to re-add at least ppc-460x. These models should be of interest to emulate
BMC like boards and, in some near future, they could even run OpenBMC.
I understand that you are adding extended status support, multi transfer
support, better interrupt support. Having some comments on the bit
definitions and register names would help a lot.
Is there a public datasheet for the I2C controller of the Sam460ex board ?
and a simple boot image we could use to test ?
Some comments below,
>
> 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);
#define EXTSTS_BCS_FREE 0x40 ?
> 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;
Don't we have a bit definition for this ?
> 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) {
can mdidx go above 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;
MDBF sounds like a 'M ... Data Buffer Full' status
> + } else if (i2c->mdidx == 0) {
> + i2c->sts |= IIC_STS_MDBS;
what about 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)) {
That's an abort ? correct ?
> + 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++) {
ok. i is used for mdata, but the tct definition should not exceed 3
> + if (!i2c_bus_busy(i2c->bus)) {
> + i2c->extsts = (1 << 6);
please add a definition for this bit.
> + 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;
> + }
> + }
do we need to start the transfer in the loop ? The device address
does not change if I am correct. We would not need to test sts against
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;
> + }
That's the end of the loop I suppose ?
> + i2c->xfrcnt = i;
what is that xfrcnt field/reg for ?
> + 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:
So that seems to be 'control' register of the I2C controller.
> - i2c->mdcntl = value & 0xdf;
> + i2c->mdcntl = value & 0x3d;
Could we use a mask built from the bits instead of raw hex value ?
> + if (value & IIC_MDCNTL_ESM) {
> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> + __func__);
> + }
> + if (value & IIC_MDCNTL_FMDB) {
that's a flush ?
> + 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);
ditto for the mask.
Thanks,
C.
> + 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] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation BALATON Zoltan
@ 2018-06-24 17:55 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-24 17:55 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: David Gibson
On 06/24/2018 01:20 PM, BALATON Zoltan wrote:
> Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> of day is implemented. Setting time and RTC alarm are not supported.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> 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 2874ddc..3bfc4a8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -839,6 +839,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)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 3/4] sam460ex: Add RTC device
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device BALATON Zoltan
@ 2018-06-24 17:55 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-24 17:55 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: David Gibson
On 06/24/2018 01:20 PM, BALATON Zoltan wrote:
> The Sam460ex has an M41T80 serial RTC chip on I2C bus 0 at address 0x68.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> 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);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-24 17:53 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
@ 2018-06-24 20:37 ` BALATON Zoltan
2018-06-25 9:27 ` Cédric Le Goater
2018-06-28 9:57 ` Cédric Le Goater
0 siblings, 2 replies; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-24 20:37 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel, qemu-ppc, David Gibson
Hello,
On Sun, 24 Jun 2018, Cédric Le Goater wrote:
> On 06/24/2018 01:20 PM, 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.
>
> ppc4xx support was dropped from u-boot but there is some work being done
> to re-add at least ppc-460x. These models should be of interest to emulate
> BMC like boards and, in some near future, they could even run OpenBMC.
>
> I understand that you are adding extended status support, multi transfer
> support, better interrupt support. Having some comments on the bit
> definitions and register names would help a lot.
>
> Is there a public datasheet for the I2C controller of the Sam460ex board ?
> and a simple boot image we could use to test ?
I don't have the manual of this SoC but some of the devices including this
i2c controller is similar to the 440EP for which there's a manual online.
That's the best reference I know of even if not always applicable for
460ex.
> Some comments below,
>
>>
>> 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);
>
> #define EXTSTS_BCS_FREE 0x40 ?
I guess this could be defined but I did not bother as this is not fully
emulated. If you think it's worth to add it without the other states I can
do in next version.
>> 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;
>
> Don't we have a bit definition for this ?
No, like I said above not all states in extsts are emulated, just the bits
needed for the guests I've tested, because I don't know how real hardware
works. So extsts is mostly a placeholder if it ever needs to be emulated
more closely at which points appropriate defines could also be added.
>> 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) {
>
> can mdidx go above 3 ?
No, it shouldn't, the > is just to make sure no buffer overrun is
possible.
>> + 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;
>
> MDBF sounds like a 'M ... Data Buffer Full' status
Master Data Buffer Full
>> + } else if (i2c->mdidx == 0) {
>> + i2c->sts |= IIC_STS_MDBS;
>
> what about MDBS ?
Master Data Buffer Status, shows if buffer has data or not.
>> }
>> 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)) {
>
> That's an abort ? correct ?
HMT: Halt Master Transfer, issue stop to halt master transfer.
>> + 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++) {
>
> ok. i is used for mdata, but the tct definition should not exceed 3
TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2
bits so it should also not be higher than 3.
>> + if (!i2c_bus_busy(i2c->bus)) {
>> + i2c->extsts = (1 << 6);
>
> please add a definition for this bit.
See above, this basically resets extsts to initial value which may not
match what real hardware does but enough for tested guests.
>> + 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;
>> + }
>> + }
>
> do we need to start the transfer in the loop ? The device address
> does not change if I am correct. We would not need to test sts against
> IIC_STS_ERR.
I have no idea. This works with all guests I tested, that's U-Boot, AROS,
AmigaOS, Linux. Don't know if it matches what real hardware does though.
But there are some modes where repeated start is used so I think this
needs to be within the loop to allow that.
>> + 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;
>> + }
>
> That's the end of the loop I suppose ?
>
>> + i2c->xfrcnt = i;
>
> what is that xfrcnt field/reg for ?
Transfer count, number of bytes transmitted.
>> + 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:
>
> So that seems to be 'control' register of the I2C controller.
Yes.
>> - i2c->mdcntl = value & 0xdf;
>> + i2c->mdcntl = value & 0x3d;
>
> Could we use a mask built from the bits instead of raw hex value ?
Probably, I don't remember the details any more.
>> + if (value & IIC_MDCNTL_ESM) {
>> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>> + __func__);
>> + }
>> + if (value & IIC_MDCNTL_FMDB) {
>
> that's a flush ?
FMDB: Flush Master Data Buffer
All these names are in the documentation, you should consult that instead
of using this emulation as documentation which it's not just approximate
emulation of hardware to satisfy guests.
Do we need a new version with more constants added or is this acceptable
now?
Thanks you,
BALATON Zoltan
>> + 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);
>
> ditto for the mask.
>
> Thanks,
>
> C.
>
>> + 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] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-24 20:37 ` BALATON Zoltan
@ 2018-06-25 9:27 ` Cédric Le Goater
2018-06-28 9:57 ` Cédric Le Goater
1 sibling, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-25 9:27 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, David Gibson
On 06/24/2018 10:37 PM, BALATON Zoltan wrote:
> Hello,
>
> On Sun, 24 Jun 2018, Cédric Le Goater wrote:
>> On 06/24/2018 01:20 PM, 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.
>>
>> ppc4xx support was dropped from u-boot but there is some work being done
>> to re-add at least ppc-460x. These models should be of interest to emulate
>> BMC like boards and, in some near future, they could even run OpenBMC.
>>
>> I understand that you are adding extended status support, multi transfer
>> support, better interrupt support. Having some comments on the bit
>> definitions and register names would help a lot.
>>
>> Is there a public datasheet for the I2C controller of the Sam460ex board ?
>> and a simple boot image we could use to test ?
>
> I don't have the manual of this SoC but some of the devices including this i2c controller is similar to the 440EP for which there's a manual online. That's the best reference I know of even if not always applicable for 460ex.
I could not find one. Can you provide an URL ? I would to take a look
at the register and bit definitions of the I2C controller of the SoC.
Thanks,
C.
>
>> Some comments below,
>>
>>>
>>> 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);
>>
>> #define EXTSTS_BCS_FREE 0x40 ?
>
> I guess this could be defined but I did not bother as this is not fully emulated. If you think it's worth to add it without the other states I can do in next version.
>
>>> 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;
>>
>> Don't we have a bit definition for this ?
>
> No, like I said above not all states in extsts are emulated, just the bits needed for the guests I've tested, because I don't know how real hardware works. So extsts is mostly a placeholder if it ever needs to be emulated more closely at which points appropriate defines could also be added.
>
>>> 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) {
>>
>> can mdidx go above 3 ?
>
> No, it shouldn't, the > is just to make sure no buffer overrun is possible.
>
>>> + 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;
>>
>> MDBF sounds like a 'M ... Data Buffer Full' status
>
> Master Data Buffer Full
>
>>> + } else if (i2c->mdidx == 0) {
>>> + i2c->sts |= IIC_STS_MDBS;
>>
>> what about MDBS ?
>
> Master Data Buffer Status, shows if buffer has data or not.
>
>>> }
>>> 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)) {
>>
>> That's an abort ? correct ?
>
> HMT: Halt Master Transfer, issue stop to halt master transfer.
>
>>> + 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++) {
>>
>> ok. i is used for mdata, but the tct definition should not exceed 3
>
> TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2 bits so it should also not be higher than 3.
>
>>> + if (!i2c_bus_busy(i2c->bus)) {
>>> + i2c->extsts = (1 << 6);
>>
>> please add a definition for this bit.
>
> See above, this basically resets extsts to initial value which may not match what real hardware does but enough for tested guests.
>
>>> + 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;
>>> + }
>>> + }
>>
>> do we need to start the transfer in the loop ? The device address
>> does not change if I am correct. We would not need to test sts against
>> IIC_STS_ERR.
>
> I have no idea. This works with all guests I tested, that's U-Boot, AROS, AmigaOS, Linux. Don't know if it matches what real hardware does though. But there are some modes where repeated start is used so I think this needs to be within the loop to allow that.
>
>>> + 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;
>>> + }
>>
>> That's the end of the loop I suppose ?
>>
>>> + i2c->xfrcnt = i;
>>
>> what is that xfrcnt field/reg for ?
>
> Transfer count, number of bytes transmitted.
>
>>> + 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:
>>
>> So that seems to be 'control' register of the I2C controller.
>
> Yes.
>
>>> - i2c->mdcntl = value & 0xdf;
>>> + i2c->mdcntl = value & 0x3d;
>>
>> Could we use a mask built from the bits instead of raw hex value ?
>
> Probably, I don't remember the details any more.
>
>>> + if (value & IIC_MDCNTL_ESM) {
>>> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>> + __func__);
>>> + }
>>> + if (value & IIC_MDCNTL_FMDB) {
>>
>> that's a flush ?
>
> FMDB: Flush Master Data Buffer
>
> All these names are in the documentation, you should consult that instead of using this emulation as documentation which it's not just approximate emulation of hardware to satisfy guests.
>
> Do we need a new version with more constants added or is this acceptable now?
>
> Thanks you,
> BALATON Zoltan
>
>>> + 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);
>>
>> ditto for the mask.
>>
>> Thanks,
>>
>> C.
>>
>>> + 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] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-24 20:37 ` BALATON Zoltan
2018-06-25 9:27 ` Cédric Le Goater
@ 2018-06-28 9:57 ` Cédric Le Goater
2018-06-28 14:34 ` BALATON Zoltan
1 sibling, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-28 9:57 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, qemu-ppc, David Gibson
On 06/24/2018 10:37 PM, BALATON Zoltan wrote:
> Hello,
>
> On Sun, 24 Jun 2018, Cédric Le Goater wrote:
>> On 06/24/2018 01:20 PM, 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.
>>
>> ppc4xx support was dropped from u-boot but there is some work being done
>> to re-add at least ppc-460x. These models should be of interest to emulate
>> BMC like boards and, in some near future, they could even run OpenBMC.
>>
>> I understand that you are adding extended status support, multi transfer
>> support, better interrupt support. Having some comments on the bit
>> definitions and register names would help a lot.
>>
>> Is there a public datasheet for the I2C controller of the Sam460ex board ?
>> and a simple boot image we could use to test ?
>
> I don't have the manual of this SoC but some of the devices including this i2c controller is similar to the 440EP for which there's a manual online. That's the best reference I know of even if not always applicable for 460ex.
>
>> Some comments below,
>>
>>>
>>> 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);
>>
>> #define EXTSTS_BCS_FREE 0x40 ?
>
> I guess this could be defined but I did not bother as this is not fully emulated. If you think it's worth to add it without the other states I can do in next version.
>
>>> 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;
>>
>> Don't we have a bit definition for this ?
>
> No, like I said above not all states in extsts are emulated, just the bits needed for the guests I've tested, because I don't know how real hardware works. So extsts is mostly a placeholder if it ever needs to be emulated more closely at which points appropriate defines could also be added.
>
>>> 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) {
>>
>> can mdidx go above 3 ?
>
> No, it shouldn't, the > is just to make sure no buffer overrun is possible.
>
>>> + 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;
>>
>> MDBF sounds like a 'M ... Data Buffer Full' status
>
> Master Data Buffer Full
>
>>> + } else if (i2c->mdidx == 0) {
>>> + i2c->sts |= IIC_STS_MDBS;
>>
>> what about MDBS ?
>
> Master Data Buffer Status, shows if buffer has data or not.
>
>>> }
>>> 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)) {
>>
>> That's an abort ? correct ?
>
> HMT: Halt Master Transfer, issue stop to halt master transfer.
>
>>> + 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++) {
>>
>> ok. i is used for mdata, but the tct definition should not exceed 3
>
> TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2 bits so it should also not be higher than 3.
>
>>> + if (!i2c_bus_busy(i2c->bus)) {
>>> + i2c->extsts = (1 << 6);
>>
>> please add a definition for this bit.
>
> See above, this basically resets extsts to initial value which may not match what real hardware does but enough for tested guests.
>
>>> + 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;
>>> + }
>>> + }
>>
>> do we need to start the transfer in the loop ? The device address
>> does not change if I am correct. We would not need to test sts against
>> IIC_STS_ERR.
>
> I have no idea. This works with all guests I tested, that's U-Boot, AROS, AmigaOS, Linux. Don't know if it matches what real hardware does though. But there are some modes where repeated start is used so I think this needs to be within the loop to allow that.
>
>>> + 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;
>>> + }
>>
>> That's the end of the loop I suppose ?
>>
>>> + i2c->xfrcnt = i;
>>
>> what is that xfrcnt field/reg for ?
>
> Transfer count, number of bytes transmitted.
>
>>> + 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:
>>
>> So that seems to be 'control' register of the I2C controller.
>
> Yes.
>
>>> - i2c->mdcntl = value & 0xdf;
>>> + i2c->mdcntl = value & 0x3d;
>>
>> Could we use a mask built from the bits instead of raw hex value ?
>
> Probably, I don't remember the details any more.
>
>>> + if (value & IIC_MDCNTL_ESM) {
>>> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>> + __func__);
>>> + }
>>> + if (value & IIC_MDCNTL_FMDB) {
>>
>> that's a flush ?
>
> FMDB: Flush Master Data Buffer
>
> All these names are in the documentation, you should consult that instead of using this emulation as documentation which it's not just approximate emulation of hardware to satisfy guests.
>
> Do we need a new version with more constants added or is this acceptable now?
I have tried an ISO from :
http://aros.sourceforge.net/nightly1.php
which boots and writes :
U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02)
CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115)
No Security/Kasumi support
Bootstrap Option A - Boot ROM Location EBC (8 bits)
Internal PCI arbiter disabled
32 kB I-Cache 32 kB D-Cache
Board: Sam460ex, PCIe 4x + SATA-2
I2C: ready
DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0)
*** Warning - bad CRC, using default environment
PCI: Bus Dev VenId DevId Class Int
00 06 126f 0501 0380 00
PCIE1: successfully set as root-complex
Net: ppc_4xx_eth0
FPGA: Revision 00 (20 0-00-00)
SM502: found
VGA: NO CARDS
and on the graphic window :
FLB: no SLB found in any of the designated boot sources, returning to u-boot.
the graphical bool loader menu works but I couldn't exercise much more the
system.
I then used the 4.5 kernel from :
http://www.supertuxkart-amiga.de/amiga/sam.html#downloads
with this command line :
qemu-system-ppc -M sam460ex -serial mon:stdio -nodefaults -kernel ./Sam460ex-4.5.0/boot/uImage-sam460ex-4.5.0 -dtb Sam460ex-4.5.0/boot/canyonlands.dtb -append "console=ttyS0,119200"
output was :
[ 0.000000] Using Canyonlands machine description
[ 0.000000] Linux version 4.5.0-sam460ex (root@julian-VirtualBox) (gcc version 5.3.1 20160205 (Ubuntu 5.3.1-8ubuntu2) ) #1 PREEMPT Mon Mar 14 06:27:13 AST 2016
...
[ 2.194282] i2c /dev entries driver
[ 2.201326] rtc-m41t80 0-0068: rtc core: registered m41t80 as rtc0
[ 2.202357] ibm-iic 4ef600700.i2c: using standard (100 kHz) mode
[ 2.203796] ibm-iic 4ef600800.i2c: using standard (100 kHz) mode
...
[ 2.269934] rtc-m41t80 0-0068: setting system clock to 2018-06-28 09:49:28 UTC (1530179368)
So I would say from the review of the code and the tests that we are fine.
Could please document a little more the register and bit definitions and
remove the raw hex values when possible ?
With these addressed :
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> Thanks you,
> BALATON Zoltan
>
>>> + 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);
>>
>> ditto for the mask.
>>
>> Thanks,
>>
>> C.
>>
>>> + 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] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller BALATON Zoltan
@ 2018-06-28 10:20 ` Cédric Le Goater
2018-06-28 14:25 ` BALATON Zoltan
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-28 10:20 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: David Gibson
On 06/24/2018 01:20 PM, BALATON Zoltan wrote:
> PPC440 SoCs such as the AMCC 460EX have a DMA controller which is used
> by AmigaOS on the sam460ex. Implement the parts used by AmigaOS so it
> can get further booting on the sam460ex machine.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/ppc440.h | 1 +
> hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/sam460ex.c | 3 +
> 3 files changed, 219 insertions(+)
>
> diff --git a/hw/ppc/ppc440.h b/hw/ppc/ppc440.h
> index ad27db1..7cef936 100644
> --- a/hw/ppc/ppc440.h
> +++ b/hw/ppc/ppc440.h
> @@ -21,6 +21,7 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks,
> hwaddr *ram_bases, hwaddr *ram_sizes,
> int do_init);
> void ppc4xx_ahb_init(CPUPPCState *env);
> +void ppc4xx_dma_init(CPUPPCState *env, int dcr_base);
> void ppc460ex_pcie_init(CPUPPCState *env);
>
> #endif /* PPC440_H */
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index 123f4ac..38aadd9 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -13,6 +13,7 @@
> #include "qemu/cutils.h"
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "qemu/log.h"
> #include "cpu.h"
> #include "hw/hw.h"
> #include "exec/address-spaces.h"
> @@ -803,6 +804,220 @@ void ppc4xx_ahb_init(CPUPPCState *env)
> }
>
> /*****************************************************************************/
> +/* DMA controller */
> +
> +#define DMA0_CR_CE (1 << 31)
> +#define DMA0_CR_PW (1 << 26 | 1 << 25)
> +#define DMA0_CR_DAI (1 << 24)
> +#define DMA0_CR_SAI (1 << 23)
> +#define DMA0_CR_DEC (1 << 2)
> +
> +enum {
> + DMA0_CR = 0x00,
> + DMA0_CT,
> + DMA0_SAH,
> + DMA0_SAL,
> + DMA0_DAH,
> + DMA0_DAL,
> + DMA0_SGH,
> + DMA0_SGL,
> +
> + DMA0_SR = 0x20,
> + DMA0_SGC = 0x23,
> + DMA0_SLP = 0x25,
> + DMA0_POL = 0x26,
> +};
> +
> +typedef struct {
> + uint32_t cr;
> + uint32_t ct;
> + uint64_t sa;
> + uint64_t da;
> + uint64_t sg;
> +} ppc4xx_dma_ch_t;
You need to use CamelCase for the type names.
> +typedef struct {
> + int base;
> + ppc4xx_dma_ch_t ch[4];
> + uint32_t sr;
> +} ppc4xx_dma_t;
Can't you QOM'ify the model ?
> +static uint32_t dcr_read_dma(void *opaque, int dcrn)
> +{
> + ppc4xx_dma_t *dma = opaque;
> + uint32_t val = 0;
> + int addr = dcrn - dma->base;
> + int chnl = addr / 8;
> +
> + switch (addr) {
> + case 0x00 ... 0x1f:
> + switch (addr % 8) {
> + case DMA0_CR:
> + val = dma->ch[chnl].cr;
> + break;
> + case DMA0_CT:
> + val = dma->ch[chnl].ct;
> + break;
> + case DMA0_SAH:
> + val = dma->ch[chnl].sa >> 32;
> + break;
> + case DMA0_SAL:
> + val = dma->ch[chnl].sa;
> + break;
> + case DMA0_DAH:
> + val = dma->ch[chnl].da >> 32;
> + break;
> + case DMA0_DAL:
> + val = dma->ch[chnl].da;
> + break;
> + case DMA0_SGH:
> + val = dma->ch[chnl].sg >> 32;
> + break;
> + case DMA0_SGL:
> + val = dma->ch[chnl].sg;
> + break;
> + }
> + break;
> + case DMA0_SR:
> + val = dma->sr;
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
> + __func__, dcrn, chnl, addr);
> + }
> +
> + return val;
> +}
> +
> +static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
> +{
> + ppc4xx_dma_t *dma = opaque;
> + int addr = dcrn - dma->base;
> + int chnl = addr / 8;
> +
> + switch (addr) {
> + case 0x00 ... 0x1f:
> + switch (addr % 8) {
> + case DMA0_CR:
> + dma->ch[chnl].cr = val;
> + if (val & DMA0_CR_CE) {
> + int count = dma->ch[chnl].ct & 0xffff;
> +
> + if (count) {
> + int width, i, sidx, didx;
> + uint8_t *rptr, *wptr;
> + hwaddr rlen, wlen;
> +
> + width = 1 << ((val & DMA0_CR_PW) >> 25);
> + rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen, 0);
> + wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen, 1);
you don't need necessarily to map. You could just use cpu_physical_memory_rw
> + if (!(val & DMA0_CR_DEC) &&
> + val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
some extra () would help reading the if.
> + /* optimise common case */
> + memmove(wptr, rptr, count * width);
> + sidx = didx = count * width;
> + } else {
> + /* do it the slow way */
> + for (sidx = didx = i = 0; i < count; i++) {
> + uint64_t v = ldn_le_p(rptr + sidx, width);
> + stn_le_p(wptr + didx, width, v);
> + if (val & DMA0_CR_SAI) {
> + sidx += width;
> + }
> + if (val & DMA0_CR_DAI) {
> + didx += width;
> + }
> + }
> + }
> + cpu_physical_memory_unmap(wptr, wlen, 1, didx);
> + cpu_physical_memory_unmap(rptr, rlen, 0, sidx);
> + }
> + }
I would put all this code doing the dma in its own routine.
> + break;
> + case DMA0_CT:
> + dma->ch[chnl].ct = val;
> + break;
> + case DMA0_SAH:
> + dma->ch[chnl].sa &= 0xffffffffULL;
> + dma->ch[chnl].sa |= (uint64_t)val << 32;
> + break;
> + case DMA0_SAL:
> + dma->ch[chnl].sa &= 0xffffffff00000000ULL;
> + dma->ch[chnl].sa |= val;
> + break;
> + case DMA0_DAH:
> + dma->ch[chnl].da &= 0xffffffffULL;
> + dma->ch[chnl].da |= (uint64_t)val << 32;
> + break;
> + case DMA0_DAL:
> + dma->ch[chnl].da &= 0xffffffff00000000ULL;
> + dma->ch[chnl].da |= val;
> + break;
> + case DMA0_SGH:
> + dma->ch[chnl].sg &= 0xffffffffULL;
> + dma->ch[chnl].sg |= (uint64_t)val << 32;
> + break;
> + case DMA0_SGL:
> + dma->ch[chnl].sg &= 0xffffffff00000000ULL;
> + dma->ch[chnl].sg |= val;
> + break;
> + }
> + break;
> + case DMA0_SR:
> + dma->sr &= ~val;
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
> + __func__, dcrn, chnl, addr);
> + }
> +}
> +
> +static void ppc4xx_dma_reset(void *opaque)
> +{
> + ppc4xx_dma_t *dma = opaque;
> + int dma_base = dma->base;
> +
> + memset(dma, 0, sizeof(*dma));
> + dma->base = dma_base;
> +}
> +
> +void ppc4xx_dma_init(CPUPPCState *env, int dcr_base)
> +{
> + ppc4xx_dma_t *dma;
> + int i;
> +
> + dma = g_malloc0(sizeof(*dma));
Can't you QOM'ify the model ? A part from that it looks correct.
Thanks,
C.
> + dma->base = dcr_base;
> + qemu_register_reset(&ppc4xx_dma_reset, dma);
> + for (i = 0; i < 4; i++) {
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CR,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CT,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAH,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAL,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAH,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAL,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGH,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGL,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + }
> + ppc_dcr_register(env, dcr_base + DMA0_SR,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + DMA0_SGC,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + DMA0_SLP,
> + dma, &dcr_read_dma, &dcr_write_dma);
> + ppc_dcr_register(env, dcr_base + DMA0_POL,
> + dma, &dcr_read_dma, &dcr_write_dma);
> +}
> +
> +/*****************************************************************************/
> /* PCI Express controller */
> /* FIXME: This is not complete and does not work, only implemented partially
> * to allow firmware and guests to find an empty bus. Cards should use PCI.
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index dc730cc..4f9248e 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -477,6 +477,9 @@ static void sam460ex_init(MachineState *machine)
> /* MAL */
> ppc4xx_mal_init(env, 4, 16, &uic[2][3]);
>
> + /* DMA */
> + ppc4xx_dma_init(env, 0x200);
> +
> /* 256K of L2 cache as memory */
> ppc4xx_l2sram_init(env);
> /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller
2018-06-28 10:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
@ 2018-06-28 14:25 ` BALATON Zoltan
0 siblings, 0 replies; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-28 14:25 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-devel, qemu-ppc, David Gibson
On Thu, 28 Jun 2018, Cédric Le Goater wrote:
> On 06/24/2018 01:20 PM, BALATON Zoltan wrote:
>> PPC440 SoCs such as the AMCC 460EX have a DMA controller which is used
>> by AmigaOS on the sam460ex. Implement the parts used by AmigaOS so it
>> can get further booting on the sam460ex machine.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/ppc/ppc440.h | 1 +
>> hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/ppc/sam460ex.c | 3 +
>> 3 files changed, 219 insertions(+)
>>
>> diff --git a/hw/ppc/ppc440.h b/hw/ppc/ppc440.h
>> index ad27db1..7cef936 100644
>> --- a/hw/ppc/ppc440.h
>> +++ b/hw/ppc/ppc440.h
>> @@ -21,6 +21,7 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks,
>> hwaddr *ram_bases, hwaddr *ram_sizes,
>> int do_init);
>> void ppc4xx_ahb_init(CPUPPCState *env);
>> +void ppc4xx_dma_init(CPUPPCState *env, int dcr_base);
>> void ppc460ex_pcie_init(CPUPPCState *env);
>>
>> #endif /* PPC440_H */
>> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
>> index 123f4ac..38aadd9 100644
>> --- a/hw/ppc/ppc440_uc.c
>> +++ b/hw/ppc/ppc440_uc.c
>> @@ -13,6 +13,7 @@
>> #include "qemu/cutils.h"
>> #include "qemu/error-report.h"
>> #include "qapi/error.h"
>> +#include "qemu/log.h"
>> #include "cpu.h"
>> #include "hw/hw.h"
>> #include "exec/address-spaces.h"
>> @@ -803,6 +804,220 @@ void ppc4xx_ahb_init(CPUPPCState *env)
>> }
>>
>> /*****************************************************************************/
>> +/* DMA controller */
>> +
>> +#define DMA0_CR_CE (1 << 31)
>> +#define DMA0_CR_PW (1 << 26 | 1 << 25)
>> +#define DMA0_CR_DAI (1 << 24)
>> +#define DMA0_CR_SAI (1 << 23)
>> +#define DMA0_CR_DEC (1 << 2)
>> +
>> +enum {
>> + DMA0_CR = 0x00,
>> + DMA0_CT,
>> + DMA0_SAH,
>> + DMA0_SAL,
>> + DMA0_DAH,
>> + DMA0_DAL,
>> + DMA0_SGH,
>> + DMA0_SGL,
>> +
>> + DMA0_SR = 0x20,
>> + DMA0_SGC = 0x23,
>> + DMA0_SLP = 0x25,
>> + DMA0_POL = 0x26,
>> +};
>> +
>> +typedef struct {
>> + uint32_t cr;
>> + uint32_t ct;
>> + uint64_t sa;
>> + uint64_t da;
>> + uint64_t sg;
>> +} ppc4xx_dma_ch_t;
>
> You need to use CamelCase for the type names.
OK.
>> +typedef struct {
>> + int base;
>> + ppc4xx_dma_ch_t ch[4];
>> + uint32_t sr;
>> +} ppc4xx_dma_t;
>
>
> Can't you QOM'ify the model ?
Maybe I could but none of these SoC models are QOMified yet so maybe it
would be better to do that as separate patch after thinking about how to
model this correctly. So now this just follows the existing way which I
prefer versus mixing QOM and old style devices models in a single SoC
without clear plan to QOMify everything. Hope this won't block this patch
and then QOMification could be done as separate cleanup series. (These SoC
models will need to be cleaned up eventually but for now getting it work
was my priority.)
>> +static uint32_t dcr_read_dma(void *opaque, int dcrn)
>> +{
>> + ppc4xx_dma_t *dma = opaque;
>> + uint32_t val = 0;
>> + int addr = dcrn - dma->base;
>> + int chnl = addr / 8;
>> +
>> + switch (addr) {
>> + case 0x00 ... 0x1f:
>> + switch (addr % 8) {
>> + case DMA0_CR:
>> + val = dma->ch[chnl].cr;
>> + break;
>> + case DMA0_CT:
>> + val = dma->ch[chnl].ct;
>> + break;
>> + case DMA0_SAH:
>> + val = dma->ch[chnl].sa >> 32;
>> + break;
>> + case DMA0_SAL:
>> + val = dma->ch[chnl].sa;
>> + break;
>> + case DMA0_DAH:
>> + val = dma->ch[chnl].da >> 32;
>> + break;
>> + case DMA0_DAL:
>> + val = dma->ch[chnl].da;
>> + break;
>> + case DMA0_SGH:
>> + val = dma->ch[chnl].sg >> 32;
>> + break;
>> + case DMA0_SGL:
>> + val = dma->ch[chnl].sg;
>> + break;
>> + }
>> + break;
>> + case DMA0_SR:
>> + val = dma->sr;
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
>> + __func__, dcrn, chnl, addr);
>> + }
>> +
>> + return val;
>> +}
>> +
>> +static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
>> +{
>> + ppc4xx_dma_t *dma = opaque;
>> + int addr = dcrn - dma->base;
>> + int chnl = addr / 8;
>> +
>> + switch (addr) {
>> + case 0x00 ... 0x1f:
>> + switch (addr % 8) {
>> + case DMA0_CR:
>> + dma->ch[chnl].cr = val;
>> + if (val & DMA0_CR_CE) {
>> + int count = dma->ch[chnl].ct & 0xffff;
>> +
>> + if (count) {
>> + int width, i, sidx, didx;
>> + uint8_t *rptr, *wptr;
>> + hwaddr rlen, wlen;
>> +
>> + width = 1 << ((val & DMA0_CR_PW) >> 25);
>> + rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen, 0);
>> + wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen, 1);
>
> you don't need necessarily to map. You could just use cpu_physical_memory_rw
Would that also update dirty bitmap as unmap does? Also this way I can use
memmove to optimise common case so this looked better but I'm not sure
about which is the best way to implement this.
>
>> + if (!(val & DMA0_CR_DEC) &&
>> + val & DMA0_CR_SAI && val & DMA0_CR_DAI) {
>
> some extra () would help reading the if.
This is personal taste I guess, I've seen other reviewers ask for less ()
in the past. I don't mind either way, David what do you prefer as
maintainer?
>> + /* optimise common case */
>> + memmove(wptr, rptr, count * width);
>> + sidx = didx = count * width;
>> + } else {
>> + /* do it the slow way */
>> + for (sidx = didx = i = 0; i < count; i++) {
>> + uint64_t v = ldn_le_p(rptr + sidx, width);
>> + stn_le_p(wptr + didx, width, v);
>> + if (val & DMA0_CR_SAI) {
>> + sidx += width;
>> + }
>> + if (val & DMA0_CR_DAI) {
>> + didx += width;
>> + }
>> + }
>> + }
>> + cpu_physical_memory_unmap(wptr, wlen, 1, didx);
>> + cpu_physical_memory_unmap(rptr, rlen, 0, sidx);
>> + }
>> + }
>
> I would put all this code doing the dma in its own routine.
I could do that but it's pretty simple so maybe does not worth splitting
out yet. Maybe when model is ever improved to handle more exotic cases and
this gets mode complex, if a guest actually using those found.
Thank you for the review,
BALATON Zoltan
>> + break;
>> + case DMA0_CT:
>> + dma->ch[chnl].ct = val;
>> + break;
>> + case DMA0_SAH:
>> + dma->ch[chnl].sa &= 0xffffffffULL;
>> + dma->ch[chnl].sa |= (uint64_t)val << 32;
>> + break;
>> + case DMA0_SAL:
>> + dma->ch[chnl].sa &= 0xffffffff00000000ULL;
>> + dma->ch[chnl].sa |= val;
>> + break;
>> + case DMA0_DAH:
>> + dma->ch[chnl].da &= 0xffffffffULL;
>> + dma->ch[chnl].da |= (uint64_t)val << 32;
>> + break;
>> + case DMA0_DAL:
>> + dma->ch[chnl].da &= 0xffffffff00000000ULL;
>> + dma->ch[chnl].da |= val;
>> + break;
>> + case DMA0_SGH:
>> + dma->ch[chnl].sg &= 0xffffffffULL;
>> + dma->ch[chnl].sg |= (uint64_t)val << 32;
>> + break;
>> + case DMA0_SGL:
>> + dma->ch[chnl].sg &= 0xffffffff00000000ULL;
>> + dma->ch[chnl].sg |= val;
>> + break;
>> + }
>> + break;
>> + case DMA0_SR:
>> + dma->sr &= ~val;
>> + break;
>> + default:
>> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
>> + __func__, dcrn, chnl, addr);
>> + }
>> +}
>> +
>> +static void ppc4xx_dma_reset(void *opaque)
>> +{
>> + ppc4xx_dma_t *dma = opaque;
>> + int dma_base = dma->base;
>> +
>> + memset(dma, 0, sizeof(*dma));
>> + dma->base = dma_base;
>> +}
>> +
>> +void ppc4xx_dma_init(CPUPPCState *env, int dcr_base)
>> +{
>> + ppc4xx_dma_t *dma;
>> + int i;
>> +
>> + dma = g_malloc0(sizeof(*dma));
>
> Can't you QOM'ify the model ? A part from that it looks correct.
>
> Thanks,
>
> C.
>
>> + dma->base = dcr_base;
>> + qemu_register_reset(&ppc4xx_dma_reset, dma);
>> + for (i = 0; i < 4; i++) {
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CR,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CT,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAH,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAL,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAH,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAL,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGH,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGL,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + }
>> + ppc_dcr_register(env, dcr_base + DMA0_SR,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + DMA0_SGC,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + DMA0_SLP,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> + ppc_dcr_register(env, dcr_base + DMA0_POL,
>> + dma, &dcr_read_dma, &dcr_write_dma);
>> +}
>> +
>> +/*****************************************************************************/
>> /* PCI Express controller */
>> /* FIXME: This is not complete and does not work, only implemented partially
>> * to allow firmware and guests to find an empty bus. Cards should use PCI.
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index dc730cc..4f9248e 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -477,6 +477,9 @@ static void sam460ex_init(MachineState *machine)
>> /* MAL */
>> ppc4xx_mal_init(env, 4, 16, &uic[2][3]);
>>
>> + /* DMA */
>> + ppc4xx_dma_init(env, 0x200);
>> +
>> /* 256K of L2 cache as memory */
>> ppc4xx_l2sram_init(env);
>> /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
>>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-28 9:57 ` Cédric Le Goater
@ 2018-06-28 14:34 ` BALATON Zoltan
2018-06-28 15:50 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: BALATON Zoltan @ 2018-06-28 14:34 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson
On Thu, 28 Jun 2018, Cédric Le Goater wrote:
> I have tried an ISO from :
>
> http://aros.sourceforge.net/nightly1.php
>
> which boots and writes :
>
> U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02)
>
> CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115)
> No Security/Kasumi support
> Bootstrap Option A - Boot ROM Location EBC (8 bits)
> Internal PCI arbiter disabled
> 32 kB I-Cache 32 kB D-Cache
> Board: Sam460ex, PCIe 4x + SATA-2
> I2C: ready
> DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0)
> *** Warning - bad CRC, using default environment
>
> PCI: Bus Dev VenId DevId Class Int
> 00 06 126f 0501 0380 00
> PCIE1: successfully set as root-complex
> Net: ppc_4xx_eth0
> FPGA: Revision 00 (20 0-00-00)
> SM502: found
> VGA: NO CARDS
This is the U-Boot firmware that prints this.
> and on the graphic window :
>
> FLB: no SLB found in any of the designated boot sources, returning to u-boot.
And also this is from U-Boot saying it could not find bootable image so it
did not boot. (One could test i2c from u_boot with date and i2c commands
as well.)
> the graphical bool loader menu works but I couldn't exercise much more the
> system.
Have you by chance used the -cdrom shortcut? That tries to connect CDROM
to secondary master but the sii3112 this board uses only has 2 SATA ports
so this won't work. You need to use the long options like
-drive if=none,id=cd,file=aros-sam440-ppc.iso,format=raw \
-device ide-cd,drive=cd,bus=ide.0
I'm not sure how to fix -cdrom or have it print a meaningful warning or
error.
> I then used the 4.5 kernel from :
>
> http://www.supertuxkart-amiga.de/amiga/sam.html#downloads
>
> with this command line :
>
> qemu-system-ppc -M sam460ex -serial mon:stdio -nodefaults -kernel ./Sam460ex-4.5.0/boot/uImage-sam460ex-4.5.0 -dtb Sam460ex-4.5.0/boot/canyonlands.dtb -append "console=ttyS0,119200"
>
> output was :
>
>
> [ 0.000000] Using Canyonlands machine description
> [ 0.000000] Linux version 4.5.0-sam460ex (root@julian-VirtualBox) (gcc version 5.3.1 20160205 (Ubuntu 5.3.1-8ubuntu2) ) #1 PREEMPT Mon Mar 14 06:27:13 AST 2016
> ...
> [ 2.194282] i2c /dev entries driver
> [ 2.201326] rtc-m41t80 0-0068: rtc core: registered m41t80 as rtc0
> [ 2.202357] ibm-iic 4ef600700.i2c: using standard (100 kHz) mode
> [ 2.203796] ibm-iic 4ef600800.i2c: using standard (100 kHz) mode
> ...
> [ 2.269934] rtc-m41t80 0-0068: setting system clock to 2018-06-28 09:49:28 UTC (1530179368)
>
>
> So I would say from the review of the code and the tests that we are fine.
>
> Could please document a little more the register and bit definitions and
> remove the raw hex values when possible ?
I'll check what can be dont but I think by now only the extsts state are
not #define-d which are also not really implemented and they are not bits
but 3 bit values so not sure what's the best way to define them. Other hex
values are some reserved bit masks I think that probably should not need
to be #defined.
> With these addressed :
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> Thanks,
>
> C.
>
>
Thanks for testing,
BALATON Zoltan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely
2018-06-28 14:34 ` BALATON Zoltan
@ 2018-06-28 15:50 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2018-06-28 15:50 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-ppc, qemu-devel, David Gibson
On 06/28/2018 04:34 PM, BALATON Zoltan wrote:
> On Thu, 28 Jun 2018, Cédric Le Goater wrote:
>> I have tried an ISO from :
>>
>> http://aros.sourceforge.net/nightly1.php
>>
>> which boots and writes :
>>
>> U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02)
>>
>> CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115)
>> No Security/Kasumi support
>> Bootstrap Option A - Boot ROM Location EBC (8 bits)
>> Internal PCI arbiter disabled
>> 32 kB I-Cache 32 kB D-Cache
>> Board: Sam460ex, PCIe 4x + SATA-2
>> I2C: ready
>> DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0)
>> *** Warning - bad CRC, using default environment
>>
>> PCI: Bus Dev VenId DevId Class Int
>> 00 06 126f 0501 0380 00
>> PCIE1: successfully set as root-complex
>> Net: ppc_4xx_eth0
>> FPGA: Revision 00 (20 0-00-00)
>> SM502: found
>> VGA: NO CARDS
>
> This is the U-Boot firmware that prints this.
>
>> and on the graphic window :
>>
>> FLB: no SLB found in any of the designated boot sources, returning to u-boot.
>
> And also this is from U-Boot saying it could not find bootable image so it did not boot. (One could test i2c from u_boot with date and i2c commands as well.)
>
>> the graphical bool loader menu works but I couldn't exercise much more the
>> system.
>
> Have you by chance used the -cdrom shortcut? That tries to connect CDROM to secondary master but the sii3112 this board uses only has 2 SATA ports so this won't work. You need to use the long options like
>
> -drive if=none,id=cd,file=aros-sam440-ppc.iso,format=raw \
> -device ide-cd,drive=cd,bus=ide.0
>
> I'm not sure how to fix -cdrom or have it print a meaningful warning or error.
Looks good :
[BATT] i2c=011bc904
[BATT] Probing i2c RTC...
[BATT] RTC found. Object=011ba234
[BATT] Dump: 35 46 15 04 28 06 18
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-28 15:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-24 11:20 [Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller BALATON Zoltan
2018-06-28 10:20 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-28 14:25 ` BALATON Zoltan
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
2018-06-24 17:53 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-24 20:37 ` BALATON Zoltan
2018-06-25 9:27 ` Cédric Le Goater
2018-06-28 9:57 ` Cédric Le Goater
2018-06-28 14:34 ` BALATON Zoltan
2018-06-28 15:50 ` Cédric Le Goater
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device BALATON Zoltan
2018-06-24 17:55 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
2018-06-24 11:20 ` [Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation BALATON Zoltan
2018-06-24 17:55 ` [Qemu-devel] [Qemu-ppc] " Cédric Le Goater
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).