[Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements

[Qemu-devel] [PATCH v5 0/4] Misc sam460ex improvements

[BALATON Zoltan]

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

[Qemu-devel] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

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

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[Cédric Le Goater]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[BALATON Zoltan]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[Cédric Le Goater]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[Cédric Le Goater]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[BALATON Zoltan]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 1/4] ppc4xx_i2c: Rewrite to model hardware more closely

[Cédric Le Goater]

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. 

[Qemu-devel] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller

[Cédric Le Goater]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller

[BALATON Zoltan]

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

[Qemu-devel] [PATCH v5 3/4] sam460ex: Add RTC device

[BALATON Zoltan]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 3/4] sam460ex: Add RTC device

[Cédric Le Goater]

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

[Qemu-devel] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation

[BALATON Zoltan]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 2/4] hw/timer: Add basic M41T80 emulation

[Cédric Le Goater]

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