Misc sam460ex improvements

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 <[email protected]>

---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++-----------------------
 include/hw/i2c/ppc4xx_i2c.h |   3 +-
 2 files changed, 110 insertions(+), 115 deletions(-)

hw/i2c/ppc4xx_i2c.c +13 -8

index fca80d6..3ebce17 100644
@@ -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;

hw/i2c/ppc4xx_i2c.c +15 -55

     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:

hw/i2c/ppc4xx_i2c.c +0 -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)) {

hw/i2c/ppc4xx_i2c.c +0 -6

         }
         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) {

hw/i2c/ppc4xx_i2c.c +7 -0

+            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) {

hw/i2c/ppc4xx_i2c.c +5 -7

+                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;*/

hw/i2c/ppc4xx_i2c.c +5 -0

+            i2c->mdidx = i - 1;
+            if (recv && i2c->mdidx >= 0) {
+                i2c->sts |= IIC_STS_MDBS;
+            }
+            if (recv && i2c->mdidx == 3) {

hw/i2c/ppc4xx_i2c.c +4 -1

+        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);

include/hw/i2c/ppc4xx_i2c.h +2 -1

index ea6c8e1..0891a9c 100644
@@ -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;

Quoted 5 lines · BALATON Zoltan

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,

Quoted 54 lines

Signed-off-by BALATON Zoltan <[email protected]>

---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++-----------------------
 include/hw/i2c/ppc4xx_i2c.h |   3 +-
 2 files changed, 110 insertions(+), 115 deletions(-)

hw/i2c/ppc4xx_i2c.c +16 -8

index fca80d6..3ebce17 100644
@@ -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;

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_reset(DeviceState *s) · line 78

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -3

     i2c->mdcntl = 0;
     i2c->sts = 0;
-    i2c->extsts = 0x8f;
-    i2c->lsadr = 0;
-    i2c->hsadr = 0;
+    i2c->extsts = (1 << 6);

#define EXTSTS_BCS_FREE 0x40 ?

Quoted 87 lines

hw/i2c/ppc4xx_i2c.c +15 -55

     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;

hw/i2c/ppc4xx_i2c.c static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size) · line 128

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -0

     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 ?

Quoted 15 lines

hw/i2c/ppc4xx_i2c.c +0 -8

         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;

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 172

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -5

-            } else {
-                i2c->sts |= IIC_STS_PT;
-                i2c->sts &= ~IIC_STS_ERR;
-                i2c->extsts = 0;
-            }
+        if (i2c->mdidx >= 3) {

can mdidx go above 3 ?

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 177

Quoted 13 lines

hw/i2c/ppc4xx_i2c.c +4 -8

+            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

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 179

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        } else if (i2c->mdidx == 0) {
+            i2c->sts |= IIC_STS_MDBS;

what about MDBS ?

Quoted 15 lines

hw/i2c/ppc4xx_i2c.c +0 -6

         }
         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) {

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 194

Quoted 10 lines

hw/i2c/ppc4xx_i2c.c +6 -4

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

Quoted 11 lines

hw/i2c/ppc4xx_i2c.c +11 -0

+            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;
+

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 211

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +6 -0

+            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

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 213

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+                if (!i2c_bus_busy(i2c->bus)) {
+                    i2c->extsts = (1 << 6);

please add a definition for this bit.

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 221

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c +8 -0

+                    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.

Quoted 13 lines

hw/i2c/ppc4xx_i2c.c +5 -7

+                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;*/

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 231

Quoted 10 lines

hw/i2c/ppc4xx_i2c.c +3 -6

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

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 232

Quoted 1 line

hw/i2c/ppc4xx_i2c.c +1 -0

+            i2c->xfrcnt = i;

what is that xfrcnt field/reg for ?

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +5 -0

+            i2c->mdidx = i - 1;
+            if (recv && i2c->mdidx >= 0) {
+                i2c->sts |= IIC_STS_MDBS;
+            }
+            if (recv && i2c->mdidx == 3) {

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 247

Quoted 10 lines

hw/i2c/ppc4xx_i2c.c +6 -0

+                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.

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 248

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +1 -1

-        i2c->mdcntl = value & 0xdf;
+        i2c->mdcntl = value & 0x3d;

Could we use a mask built from the bits instead of raw hex value ?

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 253

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +5 -0

+        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 ?

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value, · line 260

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c +5 -1

+            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.

Quoted 35 lines

hw/i2c/ppc4xx_i2c.c +4 -1

+        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);

include/hw/i2c/ppc4xx_i2c.h +2 -1

index ea6c8e1..0891a9c 100644
@@ -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;

Hello,

Quoted 11 lines · Cédric Le Goater

Quoted 5 lines · BALATON Zoltan

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.

Quoted 6 lines

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.

Quoted 57 lines

Some comments below,

Quoted 54 lines

Signed-off-by BALATON Zoltan <[email protected]>

---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++----------------------- include/hw/i2c/ppc4xx_i2c.h |   3 +-

 2 files changed, 110 insertions(+), 115 deletions(-)

hw/i2c/ppc4xx_i2c.c +16 -8

index fca80d6..3ebce17 100644
@@ -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;

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_reset(DeviceState *s) · line 78

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -3

      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.

Quoted 87 lines

Quoted 87 lines

hw/i2c/ppc4xx_i2c.c +1 -5

      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;

hw/i2c/ppc4xx_i2c.c +13 -48

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

Quoted 8 lines

Quoted 6 lines

     case 4:

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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.

Quoted 15 lines

Quoted 15 lines

         break;
     case 10:
         ret = i2c->lsadr;

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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;

Quoted 8 lines

Quoted 6 lines

-            } 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.

Quoted 14 lines

Quoted 5 lines

+            break;
         }
-        if (i2c_bus_busy(i2c->bus)) {
-            if (i2c_send(i2c->bus, i2c->mdata)) {
-                /* if the target return non zero then end the transfer */

Quoted 8 lines

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

Quoted 1 line

MDBF sounds like a 'M ... Data Buffer Full' status

Master Data Buffer Full

Quoted 4 lines

Quoted 2 lines

+        } else if (i2c->mdidx == 0) {
+            i2c->sts |= IIC_STS_MDBS;

what about MDBS ?

Master Data Buffer Status, shows if buffer has data or not.

Quoted 26 lines

Quoted 17 lines

         }
         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 */

Quoted 8 lines

-                    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)) {

Quoted 1 line

That's an abort ? correct ?

HMT: Halt Master Transfer, issue stop to halt master transfer.

Quoted 11 lines

Quoted 11 lines

+            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;
+

Quoted 8 lines

Quoted 6 lines

+            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.

Quoted 4 lines

Quoted 2 lines

+                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.

Quoted 9 lines

Quoted 8 lines

+                    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;
+                    }
+                }

Quoted 3 lines

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.

Quoted 24 lines

Quoted 15 lines

+                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;

Quoted 8 lines

-                    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;
+            }

Quoted 5 lines

That's the end of the loop I suppose ?

Quoted 1 line

+            i2c->xfrcnt = i;

what is that xfrcnt field/reg for ?

Transfer count, number of bytes transmitted.

Quoted 16 lines

Quoted 7 lines

+            i2c->mdidx = i - 1;
+            if (recv && i2c->mdidx >= 0) {
+                i2c->sts |= IIC_STS_MDBS;
+            }
+            if (recv && i2c->mdidx == 3) {
+                i2c->sts |= IIC_STS_MDBF;
+            }

Quoted 8 lines

+            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:

Quoted 1 line

So that seems to be 'control' register of the  I2C controller.

Yes.

Quoted 4 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

+        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

Quoted 53 lines

Quoted 8 lines

+            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.

Quoted 35 lines

+        if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) { +            qemu_irq_lower(i2c->irq);
+        }
         break;
     case 9:
         i2c->extsts &= ~(value & 0x8f);

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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);

include/hw/i2c/ppc4xx_i2c.h +2 -1

index ea6c8e1..0891a9c 100644
@@ -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;

Hello,

Quoted 11 lines · Cédric Le Goater

Quoted 5 lines · BALATON Zoltan

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.

Quoted 6 lines

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.

Quoted 56 lines

Some comments below,

Quoted 54 lines

Signed-off-by BALATON Zoltan <[email protected]>

---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++----------------------- include/hw/i2c/ppc4xx_i2c.h |   3 +-

 2 files changed, 110 insertions(+), 115 deletions(-)

hw/i2c/ppc4xx_i2c.c +16 -8

index fca80d6..3ebce17 100644
@@ -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;

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c static void ppc4xx_i2c_reset(DeviceState *s) · line 78

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -3

      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.

Quoted 87 lines

Quoted 87 lines

hw/i2c/ppc4xx_i2c.c +1 -5

      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;

hw/i2c/ppc4xx_i2c.c +13 -48

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

Quoted 8 lines

Quoted 6 lines

     case 4:

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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.

Quoted 15 lines

Quoted 15 lines

         break;
     case 10:
         ret = i2c->lsadr;

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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;

Quoted 8 lines

Quoted 6 lines

-            } 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.

Quoted 14 lines

Quoted 5 lines

+            break;
         }
-        if (i2c_bus_busy(i2c->bus)) {
-            if (i2c_send(i2c->bus, i2c->mdata)) {
-                /* if the target return non zero then end the transfer */

Quoted 8 lines

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

Quoted 1 line

MDBF sounds like a 'M ... Data Buffer Full' status

Master Data Buffer Full

Quoted 4 lines

Quoted 2 lines

+        } else if (i2c->mdidx == 0) {
+            i2c->sts |= IIC_STS_MDBS;

what about MDBS ?

Master Data Buffer Status, shows if buffer has data or not.

Quoted 26 lines

Quoted 17 lines

         }
         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 */

Quoted 8 lines

-                    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)) {

Quoted 1 line

That's an abort ? correct ?

HMT: Halt Master Transfer, issue stop to halt master transfer.

Quoted 11 lines

Quoted 11 lines

+            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;
+

Quoted 8 lines

Quoted 6 lines

+            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.

Quoted 4 lines

Quoted 2 lines

+                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.

Quoted 9 lines

Quoted 8 lines

+                    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;
+                    }
+                }

Quoted 3 lines

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.

Quoted 24 lines

Quoted 15 lines

+                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;

Quoted 8 lines

-                    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;
+            }

Quoted 5 lines

That's the end of the loop I suppose ?

Quoted 1 line

+            i2c->xfrcnt = i;

what is that xfrcnt field/reg for ?

Transfer count, number of bytes transmitted.

Quoted 16 lines

Quoted 7 lines

+            i2c->mdidx = i - 1;
+            if (recv && i2c->mdidx >= 0) {
+                i2c->sts |= IIC_STS_MDBS;
+            }
+            if (recv && i2c->mdidx == 3) {
+                i2c->sts |= IIC_STS_MDBF;
+            }

Quoted 8 lines

+            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:

Quoted 1 line

So that seems to be 'control' register of the  I2C controller.

Yes.

Quoted 4 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 5 lines

+        if (value & IIC_MDCNTL_ESM) {
+            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", +                          __func__);
+        }
+        if (value & IIC_MDCNTL_FMDB) {

Quoted 1 line

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

Quoted 53 lines

Quoted 8 lines

+            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.

Quoted 35 lines

+        if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) { +            qemu_irq_lower(i2c->irq);
+        }
         break;
     case 9:
         i2c->extsts &= ~(value & 0x8f);

hw/i2c/ppc4xx_i2c.c +0 -0

@@ -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);

include/hw/i2c/ppc4xx_i2c.h +2 -1

index ea6c8e1..0891a9c 100644
@@ -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;

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 <[email protected]>

Thanks,

C.

Quoted 18 lines · Cédric Le Goater

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

Quoted 7 lines

 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.

Quoted 3 lines

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.)

Quoted 2 lines

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

(quoted diff) +0 -2

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

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 <[email protected]>

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

MAINTAINERS +1 -0

index 2874ddc..3bfc4a8 100644
@@ -839,6 +839,7 @@ M: BALATON Zoltan <[email protected]>
 L: [email protected]
 S: Maintained
 F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
 
 SH4 Machines
 ------------

default-configs/ppc-softmmu.mak +1 -0

index 851b4af..b8b0526 100644
@@ -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

hw/timer/Makefile.objs +1 -0

index 8b27a4b..e16b2b9 100644
@@ -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

hw/timer/m41t80.c new file +117 -0

new file mode 100644
index 0000000..734d7d9
@@ -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)

The Sam460ex has an M41T80 serial RTC chip on I2C bus 0 at address 0x68.

Signed-off-by BALATON Zoltan <[email protected]>

---
 hw/ppc/sam460ex.c | 1 +
 1 file changed, 1 insertion(+)

hw/ppc/sam460ex.c +1 -0

index bdc53d2..dc730cc 100644
@@ -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);

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 <[email protected]>

---
 hw/ppc/ppc440.h    |   1 +
 hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/sam460ex.c  |   3 +
 3 files changed, 219 insertions(+)

hw/ppc/ppc440.h +1 -0

index ad27db1..7cef936 100644
@@ -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 */

hw/ppc/ppc440_uc.c +22 -0

index 123f4ac..38aadd9 100644
@@ -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,

hw/ppc/ppc440_uc.c +59 -0

+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) {

hw/ppc/ppc440_uc.c +10 -0

+                        /* 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;

hw/ppc/ppc440_uc.c +44 -0

+            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;
+

hw/ppc/ppc440_uc.c +31 -0

+    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.

hw/ppc/sam460ex.c +3 -0

index dc730cc..4f9248e 100644
@@ -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? */

Quoted 64 lines · BALATON Zoltan

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 <[email protected]>

---
 hw/ppc/ppc440.h    |   1 +
 hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/sam460ex.c  |   3 +
 3 files changed, 219 insertions(+)

hw/ppc/ppc440.h +1 -0

index ad27db1..7cef936 100644
@@ -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 */

hw/ppc/ppc440_uc.c +26 -0

index 123f4ac..38aadd9 100644
@@ -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 {

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 837

Quoted 6 lines

hw/ppc/ppc440_uc.c +6 -0

+    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.

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 843

Quoted 5 lines

hw/ppc/ppc440_uc.c +5 -0

+typedef struct {
+    int base;
+    ppc4xx_dma_ch_t ch[4];
+    uint32_t sr;
+} ppc4xx_dma_t;

Can't you QOM'ify the model ?

Quoted 63 lines

hw/ppc/ppc440_uc.c +63 -0

+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;

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 913

Quoted 6 lines

hw/ppc/ppc440_uc.c +6 -0

+                    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

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 915

Quoted 3 lines

hw/ppc/ppc440_uc.c +2 -0

+                    if (!(val & DMA0_CR_DEC) &&
+                        val & DMA0_CR_SAI && val & DMA0_CR_DAI) {

some extra () would help reading the if.

Quoted 10 lines

hw/ppc/ppc440_uc.c +10 -0

+                        /* 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;

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 935

Quoted 10 lines

hw/ppc/ppc440_uc.c +10 -0

+                            }
+                            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.

Quoted 44 lines

hw/ppc/ppc440_uc.c +44 -0

+            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;
+

hw/ppc/ppc440_uc.c void ppc4xx_ahb_init(CPUPPCState *env) · line 989

Quoted 10 lines

hw/ppc/ppc440_uc.c +10 -0

+    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.

Quoted 49 lines

hw/ppc/ppc440_uc.c +31 -0

+    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.

hw/ppc/sam460ex.c +3 -0

index dc730cc..4f9248e 100644
@@ -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? */