Misc sam460ex improvements

Make it more readable by converting register indexes to decimal (avoids lot of superfluous 0x0) and distinguish errors caused by accessing non-existent vs. unimplemented registers.
No functional change.

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

---
 hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index ab64d19..d1936db 100644

hw/i2c/ppc4xx_i2c.c +29 -25

 #define IIC_CNTL_PT         (1 << 0)
 #define IIC_CNTL_READ       (1 << 1)

@@ -70,7 +70,7 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->intrmsk = 0;
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
-    i2c->directcntl = 0x0f;
+    i2c->directcntl = 0xf;
     i2c->intr = 0;
 }
 

@@ -85,7 +85,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     uint64_t ret;
 
     switch (addr) {
-    case 0x00:
+    case 0:
         ret = i2c->mdata;
         if (ppc4xx_i2c_is_master(i2c)) {
             ret = 0xff;

@@ -139,58 +139,62 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 0x02:
+    case 2:
         ret = i2c->sdata;
         break;
-    case 0x04:
+    case 4:
         ret = i2c->lmadr;
         break;
-    case 0x05:
+    case 5:
         ret = i2c->hmadr;
         break;
-    case 0x06:
+    case 6:
         ret = i2c->cntl;
         break;
-    case 0x07:
+    case 7:
         ret = i2c->mdcntl;
         break;
-    case 0x08:
+    case 8:
         ret = i2c->sts;
         break;
-    case 0x09:
+    case 9:
         ret = i2c->extsts;
         break;
-    case 0x0A:
+    case 10:
         ret = i2c->lsadr;
         break;
-    case 0x0B:
+    case 11:
         ret = i2c->hsadr;
         break;
-    case 0x0C:
+    case 12:
         ret = i2c->clkdiv;
         break;
-    case 0x0D:
+    case 13:
         ret = i2c->intrmsk;
         break;
-    case 0x0E:
+    case 14:
         ret = i2c->xfrcnt;
         break;
-    case 0x0F:
+    case 15:
         ret = i2c->xtcntlss;
         break;
-    case 0x10:
+    case 16:
         ret = i2c->directcntl;
         break;
-    case 0x11:
+    case 17:
         ret = i2c->intr;
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
-                      HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
+        if (addr < PPC4xx_I2C_MEM_SIZE) {
+            qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
+                          HWADDR_PRIx "\n", __func__, addr);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
+                          HWADDR_PRIx "\n", __func__, addr);
+        }
         ret = 0;
         break;
     }
-
     return ret;
 }
 

@@ -200,7 +204,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     PPC4xxI2CState *i2c = opaque;
 
     switch (addr) {
-    case 0x00:
+    case 0:
         i2c->mdata = value;
         if (!i2c_bus_busy(i2c->bus)) {
             /* assume we start a write transfer */

@@ -225,19 +229,19 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 0x02:
+    case 2:
         i2c->sdata = value;
         break;
-    case 0x04:
+    case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
             i2c_end_transfer(i2c->bus);
         }
         break;
-    case 0x05:
+    case 5:
         i2c->hmadr = value;
         break;
-    case 0x06:
+    case 6:
         i2c->cntl = value;
         if (i2c->cntl & IIC_CNTL_PT) {
             if (i2c->cntl & IIC_CNTL_READ) {

@@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }

hw/i2c/ppc4xx_i2c.c +17 -13

         break;
-    case 0x09:
-        i2c->extsts &= ~(value & 0x8F);
+    case 9:
+        i2c->extsts &= ~(value & 0x8f);
         break;
-    case 0x0A:
+    case 10:
         i2c->lsadr = value;
-        /*i2c_set_slave_address(i2c->bus, i2c->lsadr);*/
         break;
-    case 0x0B:
+    case 11:
         i2c->hsadr = value;
         break;
-    case 0x0C:
+    case 12:
         i2c->clkdiv = value;
         break;
-    case 0x0D:
+    case 13:
         i2c->intrmsk = value;
         break;
-    case 0x0E:
+    case 14:
         i2c->xfrcnt = value & 0x77;
         break;
-    case 0x0F:
+    case 15:
         if (value & IIC_XTCNTLSS_SRST) {
             /* Is it actually a full reset? U-Boot sets some regs before */
             ppc4xx_i2c_reset(DEVICE(i2c));

@@ -296,15 +299,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         }
         i2c->xtcntlss = value;
         break;
-    case 0x10:
+    case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 0x11:
+    case 17:
         i2c->intr = value;
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
-                      HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
+        if (addr < PPC4xx_I2C_MEM_SIZE) {
+            qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
+                          HWADDR_PRIx "\n", __func__, addr);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
+                          HWADDR_PRIx "\n", __func__, addr);
+        }
         break;
     }
 }

Quoted 15 lines · BALATON Zoltan

Make it more readable by converting register indexes to decimal (avoids lot of superfluous 0x0) and distinguish errors caused by accessing non-existent vs. unimplemented registers.
No functional change.

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

---
 hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index ab64d19..d1936db 100644

hw/i2c/ppc4xx_i2c.c line 34

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +1 -1

@@ -31,7 +31,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
 
-#define PPC4xx_I2C_MEM_SIZE 0x12
+#define PPC4xx_I2C_MEM_SIZE 18

This looks weird, it seems all memory range sizes are in hex in other QEMU devices.

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c +2 -2

@@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 0x07:
-        i2c->mdcntl = value & 0xDF;
+    case 7:
+        i2c->mdcntl = value & 0xdf;

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

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +2 -2

         break;
-    case 0x08:
-        i2c->sts &= ~(value & 0x0A);
+    case 8:
+        i2c->sts &= ~(value & 0xa);

'value & 0x0a' implicitly denotes than 'value' is a 8-bit register. Matter of taste...

Quoted 24 lines · Philippe Mathieu-Daudé

Quoted 15 lines · BALATON Zoltan

Make it more readable by converting register indexes to decimal (avoids lot of superfluous 0x0) and distinguish errors caused by accessing non-existent vs. unimplemented registers.
No functional change.

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

---
 hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index ab64d19..d1936db 100644

hw/i2c/ppc4xx_i2c.c line 34

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +1 -1

@@ -31,7 +31,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
 
-#define PPC4xx_I2C_MEM_SIZE 0x12
+#define PPC4xx_I2C_MEM_SIZE 18

This looks weird, it seems all memory range sizes are in hex in other QEMU devices.

[snip]

Quoted 8 lines

Quoted 8 lines

hw/i2c/ppc4xx_i2c.c +2 -2

@@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 0x07:
-        i2c->mdcntl = value & 0xDF;
+    case 7:
+        i2c->mdcntl = value & 0xdf;

Quoted 8 lines

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

Quoted 5 lines

hw/i2c/ppc4xx_i2c.c +2 -2

         break;
-    case 0x08:
-        i2c->sts &= ~(value & 0x0A);
+    case 8:
+        i2c->sts &= ~(value & 0xa);

'value & 0x0a' implicitly denotes than 'value' is a 8-bit register. Matter of taste...

I tend to prefer the forms you suggest, Philippe, but not by enough to delay this otherwise good cleanup. Especially since Balaton is taking on this long neglected area of the code.

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

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

hw/i2c/ppc4xx_i2c.c +41 -34

index d1936db..a68b5f7 100644
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2007 Jocelyn Mayer
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal

@@ -46,9 +46,26 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+typedef struct {
+    uint8_t mdata;
+    uint8_t lmadr;
+    uint8_t hmadr;
+    uint8_t cntl;
+    uint8_t mdcntl;
+    uint8_t sts;
+    uint8_t extsts;
+    uint8_t lsadr;
+    uint8_t hsadr;
+    uint8_t clkdiv;
+    uint8_t intrmsk;
+    uint8_t xfrcnt;
+    uint8_t xtcntlss;
+    uint8_t directcntl;
+} PPC4xxI2CRegs;
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
-    PPC4xxI2CState *i2c = PPC4xx_I2C(s);
+    PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
 
     /* FIXME: Should also reset bus?
      *if (s->address != ADDR_RESET) {

@@ -63,7 +80,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;

@@ -71,7 +87,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)

@@ -81,13 +96,14 @@ static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
 
 static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
 {
-    PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
+    PPC4xxI2CState *s = PPC4xx_I2C(opaque);
+    PPC4xxI2CRegs *i2c = s->regs;
     uint64_t ret;
 
     switch (addr) {
     case 0:
         ret = i2c->mdata;
-        if (ppc4xx_i2c_is_master(i2c)) {
+        if (ppc4xx_i2c_is_master(s)) {
             ret = 0xff;
 
             if (!(i2c->sts & IIC_STS_MDBS)) {

@@ -98,7 +114,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                 int pending = (i2c->cntl >> 4) & 3;
 
                 /* get the next byte */
-                int byte = i2c_recv(i2c->bus);
+                int byte = i2c_recv(s->bus);
 
                 if (byte < 0) {
                     qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "

@@ -113,13 +129,13 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
 
                 if (!pending) {
                     i2c->sts &= ~IIC_STS_MDBS;
-                    /*i2c_end_transfer(i2c->bus);*/
+                    /*i2c_end_transfer(s->bus);*/
                 /*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
                 } else if (pending) {
                     /* current smbus implementation doesn't like
                        multibyte xfer repeated start */
-                    i2c_end_transfer(i2c->bus);
-                    if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
+                    i2c_end_transfer(s->bus);
+                    if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
                         /* if non zero is returned, the adress is not valid */
                         i2c->sts &= ~IIC_STS_PT;
                         i2c->sts |= IIC_STS_ERR;

@@ -139,9 +155,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;

@@ -181,9 +194,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"

@@ -201,14 +211,15 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
 static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
                               unsigned int size)
 {
-    PPC4xxI2CState *i2c = opaque;
+    PPC4xxI2CState *s = PPC4xx_I2C(opaque);
+    PPC4xxI2CRegs *i2c = s->regs;
 
     switch (addr) {
     case 0:
         i2c->mdata = value;
-        if (!i2c_bus_busy(i2c->bus)) {
+        if (!i2c_bus_busy(s->bus)) {
             /* assume we start a write transfer */
-            if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 0)) {
+            if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 0)) {
                 /* if non zero is returned, the adress is not valid */
                 i2c->sts &= ~IIC_STS_PT;
                 i2c->sts |= IIC_STS_ERR;

@@ -219,23 +230,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
                 i2c->extsts = 0;
             }
         }
-        if (i2c_bus_busy(i2c->bus)) {
-            if (i2c_send(i2c->bus, i2c->mdata)) {
+        if (i2c_bus_busy(s->bus)) {
+            if (i2c_send(s->bus, i2c->mdata)) {
                 /* if the target return non zero then end the transfer */
                 i2c->sts &= ~IIC_STS_PT;
                 i2c->sts |= IIC_STS_ERR;
                 i2c->extsts |= IIC_EXTSTS_XFRA;
-                i2c_end_transfer(i2c->bus);
+                i2c_end_transfer(s->bus);
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
-        if (i2c_bus_busy(i2c->bus)) {
-            i2c_end_transfer(i2c->bus);
+        if (i2c_bus_busy(s->bus)) {
+            i2c_end_transfer(s->bus);
         }
         break;
     case 5:

@@ -245,12 +253,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->cntl = value;
         if (i2c->cntl & IIC_CNTL_PT) {
             if (i2c->cntl & IIC_CNTL_READ) {
-                if (i2c_bus_busy(i2c->bus)) {
+                if (i2c_bus_busy(s->bus)) {
                     /* end previous transfer */
                     i2c->sts &= ~IIC_STS_PT;
-                    i2c_end_transfer(i2c->bus);
+                    i2c_end_transfer(s->bus);
                 }
-                if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
+                if (i2c_start_transfer(s->bus, i2c->lmadr >> 1, 1)) {
                     /* if non zero is returned, the adress is not valid */
                     i2c->sts &= ~IIC_STS_PT;
                     i2c->sts |= IIC_STS_ERR;

@@ -294,7 +302,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 15:
         if (value & IIC_XTCNTLSS_SRST) {
             /* Is it actually a full reset? U-Boot sets some regs before */
-            ppc4xx_i2c_reset(DEVICE(i2c));
+            ppc4xx_i2c_reset(DEVICE(s));
             break;
         }
         i2c->xtcntlss = value;

@@ -302,9 +310,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
 
+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

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

index 3c60307..1d5ba0c 100644
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2007 Jocelyn Mayer
  * Copyright (c) 2012 François Revol
- * Copyright (c) 2016 BALATON Zoltan
+ * Copyright (c) 2016-2018 BALATON Zoltan
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal

@@ -37,27 +37,12 @@
 typedef struct PPC4xxI2CState {
     /*< private >*/
     SysBusDevice parent_obj;
+    void *regs;
 
     /*< public >*/
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
-    uint8_t mdata;
-    uint8_t lmadr;
-    uint8_t hmadr;
-    uint8_t cntl;
-    uint8_t mdcntl;
-    uint8_t sts;
-    uint8_t extsts;
-    uint8_t sdata;
-    uint8_t lsadr;
-    uint8_t hsadr;
-    uint8_t clkdiv;
-    uint8_t intrmsk;
-    uint8_t xfrcnt;
-    uint8_t xtcntlss;
-    uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */

Quoted 1 line · BALATON Zoltan

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

It's not clear to me why this is preferable to having the registers embedded in the state structure. The latter is pretty standard practice for qemu.

Quoted 9 lines

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index d1936db..a68b5f7 100644

hw/i2c/ppc4xx_i2c.c static const MemoryRegionOps ppc4xx_i2c_ops = { · line 343

Quoted 10 lines

hw/i2c/ppc4xx_i2c.c +2 -0

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
 
+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

Quoted 6 lines · David Gibson

Quoted 1 line · BALATON Zoltan

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

It's not clear to me why this is preferable to having the registers embedded in the state structure. The latter is pretty standard practice for qemu.

Maybe it will be clearer after the next patch in the series. I needed a place to store the bitbang_i2c_interface for the directcntl way of accessing the i2c bus but I can't include bitbang_i2c.h from the public header because it's a local header. So I needed a local extension to the state struct. Once I have that then it's a good place to also store private registers which are now defined in the same file so I don't have to look up them in a different place. This seemed clearer to me and easier to work with. Maybe the spliting of the rewrite did not make this clear.

One thing I'm not sure about though:

Quoted 9 lines

Quoted 9 lines

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index d1936db..a68b5f7 100644

[...]

Quoted 6 lines

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -0

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
 

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c static const MemoryRegionOps ppc4xx_i2c_ops = { · line 343

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c +1 -0

+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

I allocate memory here but I'm not sure if it should be g_free'd somewhere and if so where? I was not able to detangle QOM object hierarchies and there seems to be no good docs available or I haven't found them. (PCI devices seem to have unrealize methods but this did not work for I2C objects.)

Quoted 8 lines · BALATON Zoltan

Quoted 6 lines · David Gibson

Quoted 1 line · BALATON Zoltan

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

It's not clear to me why this is preferable to having the registers embedded in the state structure. The latter is pretty standard practice for qemu.

Quoted 8 lines

Maybe it will be clearer after the next patch in the series. I needed a place to store the bitbang_i2c_interface for the directcntl way of accessing the i2c bus but I can't include bitbang_i2c.h from the public header because it's a local header. So I needed a local extension to the state struct. Once I have that then it's a good place to also store private registers which are now defined in the same file so I don't have to look up them in a different place. This seemed clearer to me and easier to work with. Maybe the spliting of the rewrite did not make this clear.

Oh.. right. There's a better way.

You can just forward declare the bitbang_i2c_interface structure like this in your header:

	typdef struct bitbang_i2c_interface bitbang_i2c_interface;

So you're declaring the existence of the structure, but not its contents - that's sufficient to create a pointer to it. Then you don't need to creat the substructure and extra level of indirection.

Quoted 18 lines

One thing I'm not sure about though:

Quoted 9 lines

Quoted 9 lines

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index d1936db..a68b5f7 100644

[...]

Quoted 6 lines

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -0

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
 

Quoted 9 lines

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c static const MemoryRegionOps ppc4xx_i2c_ops = { · line 343

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c +1 -0

+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

I allocate memory here but I'm not sure if it should be g_free'd somewhere and if so where? I was not able to detangle QOM object hierarchies and there seems to be no good docs available or I haven't found them. (PCI devices seem to have unrealize methods but this did not work for I2C objects.)

Yes, if you're allocating you definitely should be free()ing. It should go in the corresponding cleanup routine to where it is allocated. Since the allocation is in instance_init(), the free() should be in instance_finalize() (which you'd need to add).

Except that the above should let you avoid that.

..and I guess this won't actually ever be finalized in practice.

..and there doesn't seem to be a way to free up a bitbang_interface, so even if you added the finalize, it still wouldn't really clean up properly.

Quoted 61 lines · David Gibson

Quoted 8 lines · BALATON Zoltan

Quoted 6 lines · David Gibson

Quoted 1 line · BALATON Zoltan

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

It's not clear to me why this is preferable to having the registers embedded in the state structure. The latter is pretty standard practice for qemu.

Quoted 8 lines

Maybe it will be clearer after the next patch in the series. I needed a place to store the bitbang_i2c_interface for the directcntl way of accessing the i2c bus but I can't include bitbang_i2c.h from the public header because it's a local header. So I needed a local extension to the state struct. Once I have that then it's a good place to also store private registers which are now defined in the same file so I don't have to look up them in a different place. This seemed clearer to me and easier to work with. Maybe the spliting of the rewrite did not make this clear.

Oh.. right. There's a better way.

You can just forward declare the bitbang_i2c_interface structure like this in your header:

	typdef struct bitbang_i2c_interface bitbang_i2c_interface;

So you're declaring the existence of the structure, but not its contents - that's sufficient to create a pointer to it. Then you don't need to creat the substructure and extra level of indirection.

Quoted 18 lines

One thing I'm not sure about though:

Quoted 9 lines

Quoted 9 lines

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

hw/i2c/ppc4xx_i2c.c +0 -0

index d1936db..a68b5f7 100644

[...]

Quoted 6 lines

Quoted 6 lines

hw/i2c/ppc4xx_i2c.c +1 -0

@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
 

Quoted 9 lines

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c static const MemoryRegionOps ppc4xx_i2c_ops = { · line 343

Quoted 4 lines

hw/i2c/ppc4xx_i2c.c +1 -0

+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

I allocate memory here but I'm not sure if it should be g_free'd somewhere and if so where? I was not able to detangle QOM object hierarchies and there seems to be no good docs available or I haven't found them. (PCI devices seem to have unrealize methods but this did not work for I2C objects.)

Yes, if you're allocating you definitely should be free()ing. It should go in the corresponding cleanup routine to where it is allocated. Since the allocation is in instance_init(), the free() should be in instance_finalize() (which you'd need to add).

Quoted 7 lines

Except that the above should let you avoid that.

..and I guess this won't actually ever be finalized in practice.

..and there doesn't seem to be a way to free up a bitbang_interface, so even if you added the finalize, it still wouldn't really clean up properly.

Yes, I suspected it won't matter anyway. I'll try your suggestion to just declare the bitbang_i2c_interface in the public header in next version.

Any more reviews to expect from you for other patches or should I send a v3 with the changes so far?

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

---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

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

index 4d7be45..7d0dc2f 100644
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y

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

index 67d18b2..37af193 100644
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

hw/i2c/ppc4xx_i2c.c +1 -0

         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {

@@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    r->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)

Quoted 55 lines · BALATON Zoltan

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

---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

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

index 4d7be45..7d0dc2f 100644
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y

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

index 67d18b2..37af193 100644
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 69 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

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

---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

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

index 4d7be45..7d0dc2f 100644
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y

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

index 67d18b2..37af193 100644
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Quoted 2 lines

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

If this does not explain your question and you think it's better to use defines here I can change that in next version, please let me know.

Regards,
BALATON Zoltan

Quoted 73 lines · BALATON Zoltan

Quoted 71 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

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

---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

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

index 4d7be45..7d0dc2f 100644
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y

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

index 67d18b2..37af193 100644
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 4 lines

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it can be read back from the register in that position, no?

Quoted 74 lines · David Gibson

Quoted 73 lines · BALATON Zoltan

Quoted 71 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

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

---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

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

index 4d7be45..7d0dc2f 100644
@@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y
 
 # For Macs
 CONFIG_MAC=y

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

index 67d18b2..37af193 100644
@@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
+CONFIG_BITBANG_I2C=y

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 7 lines

Quoted 4 lines

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it can be read back from the register in that position, no?

Which of the above two do you mean?

In the first one I test for the 1/0 value set by the previous line before the bitbang_i2c_set call. This could be accessed as MSCL later but using that here would just make it longer and less obvious. If I want to be absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0) in this line too but that was just stored in the register one line before so I can reuse that here as well. Otherwise I could add another variable just for this bit value and use that in both lines but why make it more complicated for a simple 1 or 0 value?

In the second case using MSDA is really not correct because the level to set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to tell which states the two i2c lines should be and the MSDA, MSCL are read only bits that show what states the lines really are.

IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c line.

Quoted 57 lines · BALATON Zoltan

Quoted 46 lines · David Gibson

Quoted 45 lines · BALATON Zoltan

Quoted 43 lines · David Gibson

Quoted 27 lines · BALATON Zoltan

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 7 lines

Quoted 4 lines

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it can be read back from the register in that position, no?

Which of the above two do you mean?

Quoted 8 lines

In the first one I test for the 1/0 value set by the previous line before the bitbang_i2c_set call. This could be accessed as MSCL later but using that here would just make it longer and less obvious. If I want to be absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0) in this line too but that was just stored in the register one line before so I can reuse that here as well. Otherwise I could add another variable just for this bit value and use that in both lines but why make it more complicated for a simple 1 or 0 value?

Longer maybe, but I don't know about less obvious. Actually I think you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the line setting i2c->directcntl, then the next line checking that bit to pass it into bitbang_i2c_set. The point is you're modifying the effective register contents, so it makes sense to make it clearer which bit of the register you're setting.

Quoted 4 lines

In the second case using MSDA is really not correct because the level to set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to tell which states the two i2c lines should be and the MSDA, MSCL are read only bits that show what states the lines really are.

Ok...

Quoted 3 lines

IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c line.

Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're updating the value of the MSDA (== 0x2) bit in i2c->directcntl register state. Why doesn't the symbolic name make sense here?

Quoted 67 lines · David Gibson

Quoted 57 lines · BALATON Zoltan

Quoted 46 lines · David Gibson

Quoted 45 lines · BALATON Zoltan

Quoted 43 lines · David Gibson

Quoted 27 lines · BALATON Zoltan

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 7 lines

Quoted 4 lines

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it can be read back from the register in that position, no?

Which of the above two do you mean?

Quoted 8 lines

In the first one I test for the 1/0 value set by the previous line before the bitbang_i2c_set call. This could be accessed as MSCL later but using that here would just make it longer and less obvious. If I want to be absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0) in this line too but that was just stored in the register one line before so I can reuse that here as well. Otherwise I could add another variable just for this bit value and use that in both lines but why make it more complicated for a simple 1 or 0 value?

Quoted 6 lines

Longer maybe, but I don't know about less obvious. Actually I think you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the line setting i2c->directcntl, then the next line checking that bit to pass it into bitbang_i2c_set. The point is you're modifying the effective register contents, so it makes sense to make it clearer which bit of the register you're setting.

When setting the bit it's the value 1 so that's not the bit position,

I
think 1 : 0 is correct there.

I've changed the next line in v4 I've just
sent to the constant when checking the value of the MSCL bit.

Quoted 7 lines

Quoted 4 lines

In the second case using MSDA is really not correct because the level to set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to tell which states the two i2c lines should be and the MSDA, MSCL are read only bits that show what states the lines really are.

Ok...

Quoted 7 lines

Quoted 3 lines

IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c line.

Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're updating the value of the MSDA (== 0x2) bit in i2c->directcntl register state. Why doesn't the symbolic name make sense here?

Sorry, I may not have been able to clearly say what I mean. I meant that IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being bit number 0) which may have value 1 or 0. In cases I mean the value I use 1 or 0. In case I refer to the bit position I use constants. In the line

bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

it should be the constant, just used 1 there for brevity because it's obvious from the previous line what's meant.

I've changed this now. At other
places the values of the bits are written as 1 or 0 so I think for those constants should not be needed.

Quoted 75 lines · BALATON Zoltan

Quoted 67 lines · David Gibson

Quoted 57 lines · BALATON Zoltan

Quoted 46 lines · David Gibson

Quoted 45 lines · BALATON Zoltan

Quoted 43 lines · David Gibson

Quoted 27 lines · BALATON Zoltan

hw/i2c/ppc4xx_i2c.c +7 -0

index a68b5f7..5806209 100644
@@ -30,6 +30,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/i2c/ppc4xx_i2c.h"
+#include "bitbang_i2c.h"
 
 #define PPC4xx_I2C_MEM_SIZE 18
 

@@ -46,7 +47,13 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 typedef struct {
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;

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

Quoted 7 lines

hw/i2c/ppc4xx_i2c.c +3 -1

@@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
         i2c->xtcntlss = value;
         break;
     case 16:
-        i2c->directcntl = value & 0x7;
+        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
+        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
+        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?

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

Quoted 2 lines

hw/i2c/ppc4xx_i2c.c +2 -0

+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;

Last expression might be clearer as:
value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0

Quoted 7 lines

Quoted 4 lines

I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit position in the register so I use that when accessing that bit but when I check for the values of a bit being 0 or 1 I don't use the define which is for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it can be read back from the register in that position, no?

Which of the above two do you mean?

Quoted 8 lines

In the first one I test for the 1/0 value set by the previous line before the bitbang_i2c_set call. This could be accessed as MSCL later but using that here would just make it longer and less obvious. If I want to be absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0) in this line too but that was just stored in the register one line before so I can reuse that here as well. Otherwise I could add another variable just for this bit value and use that in both lines but why make it more complicated for a simple 1 or 0 value?

Quoted 6 lines

Longer maybe, but I don't know about less obvious. Actually I think you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the line setting i2c->directcntl, then the next line checking that bit to pass it into bitbang_i2c_set. The point is you're modifying the effective register contents, so it makes sense to make it clearer which bit of the register you're setting.

Quoted 2 lines

When setting the bit it's the value 1 so that's not the bit position,

Huh?? The constants aren't bit positions either, they're masks. How is IIC_DIRECTCNTL_MSCL wrong here?

Quoted 2 lines

I
think 1 : 0 is correct there.

Correct, sure, but less clear than it could be.

Quoted 23 lines

I've changed the next line in v4 I've just
sent to the constant when checking the value of the MSCL bit.

Quoted 7 lines

Quoted 4 lines

In the second case using MSDA is really not correct because the level to set is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to tell which states the two i2c lines should be and the MSDA, MSCL are read only bits that show what states the lines really are.

Ok...

Quoted 7 lines

Quoted 3 lines

IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c line.

Uh.. what? AFAICT, based on the result of bitbang_i2c_set() you're updating the value of the MSDA (== 0x2) bit in i2c->directcntl register state. Why doesn't the symbolic name make sense here?

Sorry, I may not have been able to clearly say what I mean. I meant that IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being bit number 0) which may have value 1 or 0. In cases I mean the value I use 1 or 0. In case I refer to the bit position I use constants. In the line

Quoted 4 lines

bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);

it should be the constant, just used 1 there for brevity because it's obvious from the previous line what's meant.

Maybe, but using the constant is still clearer, and friendly to people grepping the source.

Quoted 3 lines

I've changed this now. At other
places the values of the bits are written as 1 or 0 so I think for those constants should not be needed.

I have no idea what you mean by this.

Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time of day is implemented. Setting time and RTC alarm are not supported.

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

---
 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c

MAINTAINERS +1 -0

index 41cd373..9e13bc1 100644
@@ -826,6 +826,7 @@ M: BALATON Zoltan <[email protected]>
 L: [email protected]
 S: Maintained
 F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
 
 SH4 Machines
 ------------

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

index 7d0dc2f..9fbaadc 100644
@@ -27,6 +27,7 @@ CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
+CONFIG_M41T80=y
 
 # For Macs
 CONFIG_MAC=y

hw/timer/Makefile.objs +1 -0

index 8b27a4b..e16b2b9 100644
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
+common-obj-$(CONFIG_M41T80) += m41t80.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
 ifeq ($(CONFIG_ISA_BUS),y)
 common-obj-$(CONFIG_M48T59) += m48t59-isa.o

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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;
+}

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

+    if (s->addr >= 1 && s->addr <= 7) {
+        qemu_get_timedate(&now, -1);
+    }
+    switch (s->addr++) {
+    case 0:
+        qemu_gettimeofday(&tv);
+        return to_bcd(tv.tv_usec / 10000);
+    case 1:
+        return to_bcd(now.tm_sec);
+    case 2:
+        return to_bcd(now.tm_min);
+    case 3:
+        return to_bcd(now.tm_hour);
+    case 4:
+        return to_bcd(now.tm_wday);
+    case 5:
+        return to_bcd(now.tm_mday);
+    case 6:
+        return to_bcd(now.tm_mon + 1);
+    case 7:
+        return to_bcd(now.tm_year % 100);
+    case 8 ... 19:
+        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
+                      __func__, s->addr - 1);
+        return 0;
+    }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->addr = -1;
+    }
+    return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = m41t80_realize;
+    sc->send = m41t80_send;
+    sc->recv = m41t80_recv;
+    sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+    .name          = TYPE_M41T80,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(M41t80State),
+    .class_init    = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+    type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)

Quoted 2 lines · 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.

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

hw/timer/m41t80.c line 47

Quoted 8 lines

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

+    return 0;
+}
+
+static int m41t80_recv(I2CSlave *i2c)
+{
+    M41t80State *s = M41T80(i2c);
+    struct tm now;
+    qemu_timeval tv;

      int8_t addr;

hw/timer/m41t80.c line 51

Quoted 4 lines

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

+
+    if (s->addr < 0) {
+        s->addr = 0;
+    }

      addr = s->addr++;

So you can simply use 'addr' bellow.

Quoted 67 lines

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

+    if (s->addr >= 1 && s->addr <= 7) {
+        qemu_get_timedate(&now, -1);
+    }
+    switch (s->addr++) {
+    case 0:
+        qemu_gettimeofday(&tv);
+        return to_bcd(tv.tv_usec / 10000);
+    case 1:
+        return to_bcd(now.tm_sec);
+    case 2:
+        return to_bcd(now.tm_min);
+    case 3:
+        return to_bcd(now.tm_hour);
+    case 4:
+        return to_bcd(now.tm_wday);
+    case 5:
+        return to_bcd(now.tm_mday);
+    case 6:
+        return to_bcd(now.tm_mon + 1);
+    case 7:
+        return to_bcd(now.tm_year % 100);
+    case 8 ... 19:
+        qemu_log_mask(LOG_UNIMP, "\n%s: unimplemented register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
+                      __func__, s->addr - 1);
+        return 0;
+    }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->addr = -1;
+    }
+    return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = m41t80_realize;
+    sc->send = m41t80_send;
+    sc->recv = m41t80_recv;
+    sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+    .name          = TYPE_M41T80,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(M41t80State),
+    .class_init    = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+    type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)

Quoted 3 lines · Philippe Mathieu-Daudé

Quoted 2 lines · 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.

[...]

Quoted 41 lines

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

Quoted 8 lines

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

Thanks for the review. I guess we could add enum for device bytes and the special case -1 meaning no register address selected yet but this is a very simple device with only 20 bytes and the datasheet also lists them by number without naming them so I think we can also refer to them by number. Since the device has only this 20 bytes the case with 127 should also not be a problem as that's invalid address anyway. Or did you mean something else?

Quoted 55 lines · BALATON Zoltan

Quoted 3 lines · Philippe Mathieu-Daudé

Quoted 2 lines · 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.

[...]

Quoted 41 lines

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

Quoted 8 lines

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

Quoted 6 lines

Thanks for the review. I guess we could add enum for device bytes and the special case -1 meaning no register address selected yet but this is a very simple device with only 20 bytes and the datasheet also lists them by number without naming them so I think we can also refer to them by number. Since the device has only this 20 bytes the case with 127 should also not be a problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here? You're assigning the uint8_t data to addr - could that result in a negative value?

Quoted 65 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

Quoted 3 lines · Philippe Mathieu-Daudé

Quoted 2 lines · 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.

[...]

Quoted 41 lines

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

Quoted 8 lines

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

Quoted 6 lines

Thanks for the review. I guess we could add enum for device bytes and the special case -1 meaning no register address selected yet but this is a very simple device with only 20 bytes and the datasheet also lists them by number without naming them so I think we can also refer to them by number. Since the device has only this 20 bytes the case with 127 should also not be a problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

Quoted 2 lines

But is using addr < 0 safe here? You're assigning the uint8_t data to addr - could that result in a negative value?

Why wouldn't it be safe with the expected values for register address between 0-19? If the guest sends garbage values over 127 it will either result in invalid register or unselected register and lead to an error when trying to read/write that register so I don't see what other problem this may cause.

The addr < 0 is to check if no address was selected before (on creating the device and when sending first value from host addr is set to -1. In this case first write will set register address, then subsequent reads/writes increment register address as the datasheet says).

Regards,
BALATON Zoltan

Quoted 69 lines · BALATON Zoltan

Quoted 67 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

Quoted 3 lines · Philippe Mathieu-Daudé

Quoted 2 lines · 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.

[...]

Quoted 41 lines

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

Quoted 8 lines

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

Quoted 6 lines

Thanks for the review. I guess we could add enum for device bytes and the special case -1 meaning no register address selected yet but this is a very simple device with only 20 bytes and the datasheet also lists them by number without naming them so I think we can also refer to them by number. Since the device has only this 20 bytes the case with 127 should also not be a problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here? You're assigning the uint8_t data to addr - could that result in a negative value?

Quoted 5 lines

Why wouldn't it be safe with the expected values for register address between 0-19? If the guest sends garbage values over 127 it will either result in invalid register or unselected register and lead to an error when trying to read/write that register so I don't see what other problem this may cause.

Ok, but where is that enforced?

Quoted 70 lines · David Gibson

Quoted 69 lines · BALATON Zoltan

Quoted 67 lines · David Gibson

Quoted 55 lines · BALATON Zoltan

Quoted 3 lines · Philippe Mathieu-Daudé

Quoted 2 lines · 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.

[...]

Quoted 41 lines

Quoted 41 lines

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {

Quoted 8 lines

hw/timer/m41t80.c line 39

Quoted 4 lines

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

+        s->addr = data;
+    } else {
+        s->addr++;
+    }

What about adding enum i2c_event in M41t80State and use the enum here rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this expected?

Quoted 6 lines

Thanks for the review. I guess we could add enum for device bytes and the special case -1 meaning no register address selected yet but this is a very simple device with only 20 bytes and the datasheet also lists them by number without naming them so I think we can also refer to them by number. Since the device has only this 20 bytes the case with 127 should also not be a problem as that's invalid address anyway. Or did you mean something else?

So, I'm not particularly in favour of adding extra state variables.

But is using addr < 0 safe here? You're assigning the uint8_t data to addr - could that result in a negative value?

Quoted 7 lines

Quoted 5 lines

Why wouldn't it be safe with the expected values for register address between 0-19? If the guest sends garbage values over 127 it will either result in invalid register or unselected register and lead to an error when trying to read/write that register so I don't see what other problem this may cause.

Ok, but where is that enforced?

I don't see the problem. The addr register selects the register to read or write. It is set by the first write when the device is accessed the first time (this is denoted by addr == -1 (or really any negative value). The device has 20 registers and trying to read any register outside addr between 0-19 will result in returning 0 and logging a warning about invalid register in m41t80_recv. What could fail here when guest sends garbage? It will set addr to invalid value and try to read non-exitent register and get an error just like for any other nonexistent value of addr (or start to read from register 0 if it manages to set a negative value). All writes of registers are ignored currently (except setting addr by the first write). What should be enforced here?

Quoted 11 lines · BALATON Zoltan

I don't see the problem. The addr register selects the register to read or write. It is set by the first write when the device is accessed the first time (this is denoted by addr == -1 (or really any negative value). The device has 20 registers and trying to read any register outside addr between 0-19 will result in returning 0 and logging a warning about invalid register in m41t80_recv. What could fail here when guest sends garbage? It will set addr to invalid value and try to read non-exitent register and get an error just like for any other nonexistent value of addr (or start to read from register 0 if it manages to set a negative value). All writes of registers are ignored currently (except setting addr by the first write). What should be enforced here?

Ah, I see your point. I mean strictly we should match the hardware behaviour if you write garbage addresses here, but really I don't think it matters much.

Quoted 13 lines · David Gibson

Quoted 11 lines · BALATON Zoltan

I don't see the problem. The addr register selects the register to read or write. It is set by the first write when the device is accessed the first time (this is denoted by addr == -1 (or really any negative value). The device has 20 registers and trying to read any register outside addr between 0-19 will result in returning 0 and logging a warning about invalid register in m41t80_recv. What could fail here when guest sends garbage? It will set addr to invalid value and try to read non-exitent register and get an error just like for any other nonexistent value of addr (or start to read from register 0 if it manages to set a negative value). All writes of registers are ignored currently (except setting addr by the first write). What should be enforced here?

Quoted 3 lines

Ah, I see your point. I mean strictly we should match the hardware behaviour if you write garbage addresses here, but really I don't think it matters much.

Problem is like usual I have no idea what the real hardware does. I've only seen the datasheet, never seen real device and have no way to test. All the clients I've tested seem to be OK with the current emulation so unless someone has more info on how this should work I think we can live with this version and then fix it later if found to be needed.

Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time of day is implemented. Setting time and RTC alarm are not supported.

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

---
 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c

MAINTAINERS +1 -0

index 41cd373..9e13bc1 100644
@@ -826,6 +826,7 @@ M: BALATON Zoltan <[email protected]>
 L: [email protected]
 S: Maintained
 F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
 
 SH4 Machines
 ------------

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

index 7d0dc2f..9fbaadc 100644
@@ -27,6 +27,7 @@ CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
+CONFIG_M41T80=y
 
 # For Macs
 CONFIG_MAC=y

hw/timer/Makefile.objs +1 -0

index 8b27a4b..e16b2b9 100644
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
+common-obj-$(CONFIG_M41T80) += m41t80.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
 ifeq ($(CONFIG_ISA_BUS),y)
 common-obj-$(CONFIG_M48T59) += m48t59-isa.o

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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);

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

+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "\n%s: invalid register: %d",
+                      __func__, s->addr - 1);
+        return 0;
+    }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->addr = -1;
+    }
+    return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = m41t80_realize;
+    sc->send = m41t80_send;
+    sc->recv = m41t80_recv;
+    sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+    .name          = TYPE_M41T80,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(M41t80State),
+    .class_init    = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+    type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)

Quoted 103 lines · 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 <[email protected]>

---
 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c

MAINTAINERS +1 -0

index 41cd373..9e13bc1 100644
@@ -826,6 +826,7 @@ M: BALATON Zoltan <[email protected]>
 L: [email protected]
 S: Maintained
 F: hw/ide/sii3112.c
+F: hw/timer/m41t80.c
 
 SH4 Machines
 ------------

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

index 7d0dc2f..9fbaadc 100644
@@ -27,6 +27,7 @@ CONFIG_SM501=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
+CONFIG_M41T80=y
 
 # For Macs
 CONFIG_MAC=y

hw/timer/Makefile.objs +1 -0

index 8b27a4b..e16b2b9 100644
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
 common-obj-$(CONFIG_HPET) += hpet.o
 common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
+common-obj-$(CONFIG_M41T80) += m41t80.o
 common-obj-$(CONFIG_M48T59) += m48t59.o
 ifeq ($(CONFIG_ISA_BUS),y)
 common-obj-$(CONFIG_M48T59) += m48t59-isa.o

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

new file mode 100644
index 0000000..9dbdb1b
@@ -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) {