Skip to contentSkip to conversation

Misc sam460ex improvements

PATCH Applied BALATON Zoltan 2 participants 30 messages Jun 14, 2018 – Jun 18, 2018

v3 of 6 Applied Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v3 cover Misc sam460ex improvements

This is v3 of sam460ex improvements addressing review comments and adding two additional patches that allow AmigaOS to progress further (does not boot fully yet).

BALATON Zoltan (8):
ppc4xx_i2c: Remove unimplemented sdata and intr registers
ppc4xx_i2c: Implement directcntl register
ppc4xx_i2c: Rewrite to model hardware more closely hw/timer: Add basic M41T80 emulation
sam460ex: Add RTC device
sm501: Do not clear read only bits when writing registers
sm501: Implement i2c part for reading monitor EDID target/ppc: Add missing opcode for icbt on PPC440

Sebastian Bauer (1):
sm501: Perform a full update after palette change

 MAINTAINERS                        |   1 +
 default-configs/ppc-softmmu.mak    |   3 +
 default-configs/ppcemb-softmmu.mak |   2 +
 default-configs/sh4-softmmu.mak    |   1 +
 default-configs/sh4eb-softmmu.mak  |   1 +
 hw/display/sm501.c                 | 159 +++++++++++++++++++++--
 hw/i2c/ppc4xx_i2c.c                | 251 ++++++++++++++++++-------------------
 hw/ppc/sam460ex.c                  |   1 +
 hw/timer/Makefile.objs             |   1 +
 hw/timer/m41t80.c                  | 117 +++++++++++++++++
 include/hw/i2c/ppc4xx_i2c.h        |  11 +-
 target/ppc/translate.c             |   2 +
 12 files changed, 406 insertions(+), 144 deletions(-)
 create mode 100644 hw/timer/m41t80.c
Signature · 1 line
2.7.6
#1 lore raw

PATCH v3 1/9 ppc4xx_i2c: Remove unimplemented sdata and intr registers

---
 hw/i2c/ppc4xx_i2c.c         | 16 +---------------
 include/hw/i2c/ppc4xx_i2c.h |  4 +---
 2 files changed, 2 insertions(+), 18 deletions(-)
hw/i2c/ppc4xx_i2c.c +1 -15
index d1936db..4e0aaae 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
@@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;
@@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;
@@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
include/hw/i2c/ppc4xx_i2c.h +1 -3
index 3c60307..e4b6ded 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
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
     uint8_t mdcntl;
     uint8_t sts;
     uint8_t extsts;
-    uint8_t sdata;
     uint8_t lsadr;
     uint8_t hsadr;
     uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
     uint8_t xfrcnt;
     uint8_t xtcntlss;
     uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */
Signature · 1 line
2.7.6
#2 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers

Quoted 1 line · BALATON Zoltan

But.. they are implemented. Albeit as an entirely software controlled register.

I'm guessing that's not what they're supposed to do, which is why you're removing them, but that needs to be explained in the commit message.

As a general rule cases where a one line commit message is acceptable are *very* rare.

Quoted 103 lines
---
 hw/i2c/ppc4xx_i2c.c         | 16 +---------------
 include/hw/i2c/ppc4xx_i2c.h |  4 +---
 2 files changed, 2 insertions(+), 18 deletions(-)
hw/i2c/ppc4xx_i2c.c +1 -15
index d1936db..4e0aaae 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
@@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;
@@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;
@@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
include/hw/i2c/ppc4xx_i2c.h +1 -3
index 3c60307..e4b6ded 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
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
     uint8_t mdcntl;
     uint8_t sts;
     uint8_t extsts;
-    uint8_t sdata;
     uint8_t lsadr;
     uint8_t hsadr;
     uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
     uint8_t xfrcnt;
     uint8_t xtcntlss;
     uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#3 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers

Quoted 5 lines · David Gibson
Quoted 1 line · BALATON Zoltan

But.. they are implemented. Albeit as an entirely software controlled register.

They were implemented only in that they could be written and read but did not implement any of the functionality they have in real hardware so in that sense they were not really implemented.

Quoted 3 lines

I'm guessing that's not what they're supposed to do, which is why you're removing them, but that needs to be explained in the commit message.

Maybe adding this explanation:

We dont emulate slave mode so related registers are not needed. [lh]sadr are only retained to avoid too many warnings and simplify debugging but sdata is not even correct because device has a 4 byte FIFO instead so just remove this unimplemented register for now.

The intr register is also not implemented correctly, it is for diagnostics and normally not even visible on device without explicitly enabling it. As no guests are known to need this remove it as well.

Regards,
BALATON Zoltan

Quoted 108 lines

As a general rule cases where a one line commit message is acceptable are *very* rare.

Quoted 103 lines
---
 hw/i2c/ppc4xx_i2c.c         | 16 +---------------
 include/hw/i2c/ppc4xx_i2c.h |  4 +---
 2 files changed, 2 insertions(+), 18 deletions(-)
hw/i2c/ppc4xx_i2c.c +1 -15
index d1936db..4e0aaae 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
@@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;
@@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;
@@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
include/hw/i2c/ppc4xx_i2c.h +1 -3
index 3c60307..e4b6ded 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
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
     uint8_t mdcntl;
     uint8_t sts;
     uint8_t extsts;
-    uint8_t sdata;
     uint8_t lsadr;
     uint8_t hsadr;
     uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
     uint8_t xfrcnt;
     uint8_t xtcntlss;
     uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */
#4 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers

Quoted 17 lines · BALATON Zoltan
Quoted 5 lines · David Gibson
Quoted 1 line · BALATON Zoltan

But.. they are implemented. Albeit as an entirely software controlled register.

They were implemented only in that they could be written and read but did not implement any of the functionality they have in real hardware so in that sense they were not really implemented.

Quoted 3 lines

I'm guessing that's not what they're supposed to do, which is why you're removing them, but that needs to be explained in the commit message.

Maybe adding this explanation:

Quoted 8 lines

We dont emulate slave mode so related registers are not needed. [lh]sadr are only retained to avoid too many warnings and simplify debugging but sdata is not even correct because device has a 4 byte FIFO instead so just remove this unimplemented register for now.

The intr register is also not implemented correctly, it is for diagnostics and normally not even visible on device without explicitly enabling it. As no guests are known to need this remove it as well.

That sounds reasonable.

Quoted 112 lines

Regards,
BALATON Zoltan

Quoted 108 lines

As a general rule cases where a one line commit message is acceptable are *very* rare.

Quoted 103 lines
---
 hw/i2c/ppc4xx_i2c.c         | 16 +---------------
 include/hw/i2c/ppc4xx_i2c.h |  4 +---
 2 files changed, 2 insertions(+), 18 deletions(-)
hw/i2c/ppc4xx_i2c.c +1 -15
index d1936db..4e0aaae 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
@@ -63,7 +63,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->mdcntl = 0;
     i2c->sts = 0;
     i2c->extsts = 0x8f;
-    i2c->sdata = 0;
     i2c->lsadr = 0;
     i2c->hsadr = 0;
     i2c->clkdiv = 0;
@@ -71,7 +70,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
     i2c->xfrcnt = 0;
     i2c->xtcntlss = 0;
     i2c->directcntl = 0xf;
-    i2c->intr = 0;
 }
 
 static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
@@ -139,9 +137,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
                           TYPE_PPC4xx_I2C, __func__);
         }
         break;
-    case 2:
-        ret = i2c->sdata;
-        break;
     case 4:
         ret = i2c->lmadr;
         break;
@@ -181,9 +176,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
     case 16:
         ret = i2c->directcntl;
         break;
-    case 17:
-        ret = i2c->intr;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
@@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
             }
         }
         break;
-    case 2:
-        i2c->sdata = value;
-        break;
     case 4:
         i2c->lmadr = value;
         if (i2c_bus_busy(i2c->bus)) {
@@ -302,9 +291,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
     case 16:
         i2c->directcntl = value & 0x7;
         break;
-    case 17:
-        i2c->intr = value;
-        break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
             qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
include/hw/i2c/ppc4xx_i2c.h +1 -3
index 3c60307..e4b6ded 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
@@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
     uint8_t mdcntl;
     uint8_t sts;
     uint8_t extsts;
-    uint8_t sdata;
     uint8_t lsadr;
     uint8_t hsadr;
     uint8_t clkdiv;
@@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
     uint8_t xfrcnt;
     uint8_t xtcntlss;
     uint8_t directcntl;
-    uint8_t intr;
 } PPC4xxI2CState;
 
 #endif /* PPC4XX_I2C_H */
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#5 lore raw

PATCH v3 2/9 ppc4xx_i2c: Implement directcntl register

In reply to BALATON Zoltan
---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
 include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
 4 files changed, 18 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 +12 -1
index 4e0aaae..c0a1930 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,6 +47,11 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,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);
+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -322,6 +332,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    s->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
include/hw/i2c/ppc4xx_i2c.h +4 -0
index e4b6ded..ea6c8e1 100644
@@ -31,6 +31,9 @@
 #include "hw/sysbus.h"
 #include "hw/i2c/i2c.h"
 
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
 #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
 #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
 
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
Signature · 1 line
2.7.6
#6 lore raw

Re: [Qemu-devel] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

Quoted 1 line · BALATON Zoltan

Patch looks good, but it needs a commit message. What is the directcntl register? Now does the bitbang interface play into that? (Note that I know the answer to those questions right now, but it needs to be in the commit message for the benefit of people looking back in the future).

Quoted 95 lines
---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
 include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
 4 files changed, 18 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 +12 -1
index 4e0aaae..c0a1930 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,6 +47,11 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,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);
+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -322,6 +332,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    s->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
include/hw/i2c/ppc4xx_i2c.h +4 -0
index e4b6ded..ea6c8e1 100644
@@ -31,6 +31,9 @@
 #include "hw/sysbus.h"
 #include "hw/i2c/i2c.h"
 
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
 #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
 #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
 
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#7 lore raw

Re: [Qemu-devel] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

Quoted 8 lines · David Gibson
Quoted 1 line · BALATON Zoltan

Patch looks good, but it needs a commit message. What is the directcntl register? Now does the bitbang interface play into that? (Note that I know the answer to those questions right now, but it needs to be in the commit message for the benefit of people looking back in the future).

If you know the answer could you please suggest an appropriate commit message? I'm a bit lost what should be written here that would explain the patch to someone who knows nothing about what this is. And for those who know, the patch itself should be fairly simple and not need more explanation.

Regards,
BALATON Zoltan

Quoted 97 lines
Quoted 95 lines
---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
 include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
 4 files changed, 18 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 +12 -1
index 4e0aaae..c0a1930 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,6 +47,11 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,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);
+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -322,6 +332,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    s->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
include/hw/i2c/ppc4xx_i2c.h +4 -0
index e4b6ded..ea6c8e1 100644
@@ -31,6 +31,9 @@
 #include "hw/sysbus.h"
 #include "hw/i2c/i2c.h"
 
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
 #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
 #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
 
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
#8 lore raw

Re: [Qemu-devel] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

Quoted 10 lines · BALATON Zoltan
Quoted 8 lines · David Gibson
Quoted 1 line · BALATON Zoltan

Patch looks good, but it needs a commit message. What is the directcntl register? Now does the bitbang interface play into that? (Note that I know the answer to those questions right now, but it needs to be in the commit message for the benefit of people looking back in the future).

Quoted 5 lines

If you know the answer could you please suggest an appropriate commit message? I'm a bit lost what should be written here that would explain the patch to someone who knows nothing about what this is. And for those who know, the patch itself should be fairly simple and not need more explanation.

It's not really about explaining it to someone ho knows nothing about what it is. Rather it's about explaining to someone who has a general familiarity but hasn't been looking at this code recently. Try imagining explaining itself to yourself in several years time having been working on something entirely different.

So, perhaps:

| As well as being able to generate its own i2c transactions, the
| ppc4xx i2c controller has a DIRECTCNTL register which allows
| explicit control of the i2c lines.
|
| Using this register an OS can directly bitbang i2c operations.  In
| order to let emulated i2c devices respond to this, we need to wire
| up the DIRECTCNTL register to qemu's bitbanged i2c handling code.
Quoted 102 lines

Regards,
BALATON Zoltan

Quoted 97 lines
Quoted 95 lines
---
 default-configs/ppc-softmmu.mak    |  1 +
 default-configs/ppcemb-softmmu.mak |  1 +
 hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
 include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
 4 files changed, 18 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 +12 -1
index 4e0aaae..c0a1930 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,6 +47,11 @@
 
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
+#define IIC_DIRECTCNTL_SDAC (1 << 3)
+#define IIC_DIRECTCNTL_SCLC (1 << 2)
+#define IIC_DIRECTCNTL_MSDA (1 << 1)
+#define IIC_DIRECTCNTL_MSCL (1 << 0)
+
 static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
@@ -289,7 +295,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);
+        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
         break;
     default:
         if (addr < PPC4xx_I2C_MEM_SIZE) {
@@ -322,6 +332,7 @@ static void ppc4xx_i2c_init(Object *o)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
     s->bus = i2c_init_bus(DEVICE(s), "i2c");
+    s->bitbang = bitbang_i2c_init(s->bus);
 }
 
 static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
include/hw/i2c/ppc4xx_i2c.h +4 -0
index e4b6ded..ea6c8e1 100644
@@ -31,6 +31,9 @@
 #include "hw/sysbus.h"
 #include "hw/i2c/i2c.h"
 
+/* from hw/i2c/bitbang_i2c.h */
+typedef struct bitbang_i2c_interface bitbang_i2c_interface;
+
 #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
 #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
 
@@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
     I2CBus *bus;
     qemu_irq irq;
     MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
     uint8_t mdata;
     uint8_t lmadr;
     uint8_t hmadr;
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#9 lore raw

PATCH v3 3/9 ppc4xx_i2c: Rewrite to model hardware more closely

In reply to 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.

---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++-----------------------
 include/hw/i2c/ppc4xx_i2c.h |   3 +-
 2 files changed, 110 insertions(+), 115 deletions(-)
hw/i2c/ppc4xx_i2c.c +108 -114
index c0a1930..73d6ba5 100644
@@ -38,13 +38,26 @@
 #define IIC_CNTL_READ       (1 << 1)
 #define IIC_CNTL_CHT        (1 << 2)
 #define IIC_CNTL_RPST       (1 << 3)
+#define IIC_CNTL_AMD        (1 << 6)
+#define IIC_CNTL_HMT        (1 << 7)
+
+#define IIC_MDCNTL_EINT     (1 << 2)
+#define IIC_MDCNTL_ESM      (1 << 3)
+#define IIC_MDCNTL_FMDB     (1 << 6)
 
 #define IIC_STS_PT          (1 << 0)
+#define IIC_STS_IRQA        (1 << 1)
 #define IIC_STS_ERR         (1 << 2)
+#define IIC_STS_MDBF        (1 << 4)
 #define IIC_STS_MDBS        (1 << 5)
 
 #define IIC_EXTSTS_XFRA     (1 << 0)
 
+#define IIC_INTRMSK_EIMTC   (1 << 0)
+#define IIC_INTRMSK_EITA    (1 << 1)
+#define IIC_INTRMSK_EIIC    (1 << 2)
+#define IIC_INTRMSK_EIHE    (1 << 3)
+
 #define IIC_XTCNTLSS_SRST   (1 << 0)
 
 #define IIC_DIRECTCNTL_SDAC (1 << 3)
@@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
 {
     PPC4xxI2CState *i2c = PPC4xx_I2C(s);
 
-    /* FIXME: Should also reset bus?
-     *if (s->address != ADDR_RESET) {
-     *    i2c_end_transfer(s->bus);
-     *}
-     */
-
-    i2c->mdata = 0;
-    i2c->lmadr = 0;
-    i2c->hmadr = 0;
+    i2c->mdidx = -1;
+    memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
+    /* [hl][ms]addr are not affected by reset */
     i2c->cntl = 0;
     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);
include/hw/i2c/ppc4xx_i2c.h +2 -1
index ea6c8e1..0891a9c 100644
@@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
     qemu_irq irq;
     MemoryRegion iomem;
     bitbang_i2c_interface *bitbang;
-    uint8_t mdata;
+    int mdidx;
+    uint8_t mdata[4];
     uint8_t lmadr;
     uint8_t hmadr;
     uint8_t cntl;
Signature · 1 line
2.7.6
#10 lore raw

PATCH v3 4/9 hw/timer: Add basic M41T80 emulation

In reply to 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.

---

Notes:
v3: Fixed \n-s in log messages

 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c
MAINTAINERS +1 -0
index 8a94517..74ae589 100644
@@ -829,6 +829,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 +117 -0

117 lines not on this page · raw message

Signature · 1 line
2.7.6
Acked-by on v3 4/9
#11 lore raw

Re: [Qemu-devel] [PATCH v3 4/9] hw/timer: Add basic M41T80 emulation

Quoted 4 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 171 lines

---

Notes:
v3: Fixed \n-s in log messages

 MAINTAINERS                     |   1 +
 default-configs/ppc-softmmu.mak |   1 +
 hw/timer/Makefile.objs          |   1 +
 hw/timer/m41t80.c               | 117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)
 create mode 100644 hw/timer/m41t80.c
MAINTAINERS +1 -0
index 8a94517..74ae589 100644
@@ -829,6 +829,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 +117 -0
new file mode 100644
index 0000000..734d7d9
@@ -0,0 +1,117 @@
+/*
+ * M41T80 serial rtc emulation
+ *
+ * Copyright (c) 2018 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+#include "qemu/bcd.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_M41T80 "m41t80"
+#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
+
+typedef struct M41t80State {
+    I2CSlave parent_obj;
+    int8_t addr;
+} M41t80State;
+
+static void m41t80_realize(DeviceState *dev, Error **errp)
+{
+    M41t80State *s = M41T80(dev);
+
+    s->addr = -1;
+}
+
+static int m41t80_send(I2CSlave *i2c, uint8_t data)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (s->addr < 0) {
+        s->addr = data;
+    } else {
+        s->addr++;
+    }
+    return 0;
+}
+
+static int m41t80_recv(I2CSlave *i2c)
+{
+    M41t80State *s = M41T80(i2c);
+    struct tm now;
+    qemu_timeval tv;
+
+    if (s->addr < 0) {
+        s->addr = 0;
+    }
+    if (s->addr >= 1 && s->addr <= 7) {
+        qemu_get_timedate(&now, -1);
+    }
+    switch (s->addr++) {
+    case 0:
+        qemu_gettimeofday(&tv);
+        return to_bcd(tv.tv_usec / 10000);
+    case 1:
+        return to_bcd(now.tm_sec);
+    case 2:
+        return to_bcd(now.tm_min);
+    case 3:
+        return to_bcd(now.tm_hour);
+    case 4:
+        return to_bcd(now.tm_wday);
+    case 5:
+        return to_bcd(now.tm_mday);
+    case 6:
+        return to_bcd(now.tm_mon + 1);
+    case 7:
+        return to_bcd(now.tm_year % 100);
+    case 8 ... 19:
+        qemu_log_mask(LOG_UNIMP, "%s: unimplemented register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid register: %d\n",
+                      __func__, s->addr - 1);
+        return 0;
+    }
+}
+
+static int m41t80_event(I2CSlave *i2c, enum i2c_event event)
+{
+    M41t80State *s = M41T80(i2c);
+
+    if (event == I2C_START_SEND) {
+        s->addr = -1;
+    }
+    return 0;
+}
+
+static void m41t80_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = m41t80_realize;
+    sc->send = m41t80_send;
+    sc->recv = m41t80_recv;
+    sc->event = m41t80_event;
+}
+
+static const TypeInfo m41t80_info = {
+    .name          = TYPE_M41T80,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(M41t80State),
+    .class_init    = m41t80_class_init,
+};
+
+static void m41t80_register_types(void)
+{
+    type_register_static(&m41t80_info);
+}
+
+type_init(m41t80_register_types)
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#12 lore raw

PATCH v3 5/9 sam460ex: Add RTC device

In reply to BALATON Zoltan

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

---
 hw/ppc/sam460ex.c | 1 +
 1 file changed, 1 insertion(+)
hw/ppc/sam460ex.c +1 -0
index bdc53d2..dc730cc 100644
@@ -457,6 +457,7 @@ static void sam460ex_init(MachineState *machine)
     object_property_set_bool(OBJECT(dev), true, "realized", NULL);
     smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
     g_free(smbus_eeprom_buf);
+    i2c_create_slave(i2c[0]->bus, "m41t80", 0x68);
 
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
     i2c[1] = PPC4xx_I2C(dev);
Signature · 1 line
2.7.6
Reviewed-by on v3 5/9
#13 lore raw

Re: [Qemu-devel] [PATCH v3 5/9] sam460ex: Add RTC device

Quoted 3 lines · BALATON Zoltan

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

Quoted 16 lines
---
 hw/ppc/sam460ex.c | 1 +
 1 file changed, 1 insertion(+)
hw/ppc/sam460ex.c +1 -0
index bdc53d2..dc730cc 100644
@@ -457,6 +457,7 @@ static void sam460ex_init(MachineState *machine)
     object_property_set_bool(OBJECT(dev), true, "realized", NULL);
     smbus_eeprom_init(i2c[0]->bus, 8, smbus_eeprom_buf, smbus_eeprom_size);
     g_free(smbus_eeprom_buf);
+    i2c_create_slave(i2c[0]->bus, "m41t80", 0x68);
 
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600800, uic[0][3]);
     i2c[1] = PPC4xx_I2C(dev);
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#14 lore raw

PATCH v3 6/9 sm501: Do not clear read only bits when writing registers

In reply to BALATON Zoltan

When writing registers that have read only bits we have to avoid changing these bits as they may have non zero values. Make sure we use the correct masks to mask out read only and reserved bits when changing registers.

Also remove extra spaces from dram_control and arbitration_control assignments.

---
v3: Not only preserve read only bits but also allow clearing r/w bits

 hw/display/sm501.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
hw/display/sm501.c +9 -6
index e47be99..ca0840f 100644
@@ -836,27 +836,30 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case SM501_SYSTEM_CONTROL:
-        s->system_control = value & 0xE300B8F7;
+        s->system_control &= 0x10DB0000;
+        s->system_control |= value & 0xEF00B8F7;
         break;
     case SM501_MISC_CONTROL:
-        s->misc_control = value & 0xFF7FFF20;
+        s->misc_control &= 0xEF;
+        s->misc_control |= value & 0xFF7FFF10;
         break;
     case SM501_GPIO31_0_CONTROL:
         s->gpio_31_0_control = value;
         break;
     case SM501_GPIO63_32_CONTROL:
-        s->gpio_63_32_control = value;
+        s->gpio_63_32_control = value & 0xFF80FFFF;
         break;
     case SM501_DRAM_CONTROL:
         s->local_mem_size_index = (value >> 13) & 0x7;
         /* TODO : check validity of size change */
-        s->dram_control |=  value & 0x7FFFFFC3;
+        s->dram_control &= 0x80000000;
+        s->dram_control |= value & 0x7FFFFFC3;
         break;
     case SM501_ARBTRTN_CONTROL:
-        s->arbitration_control =  value & 0x37777777;
+        s->arbitration_control = value & 0x37777777;
         break;
     case SM501_IRQ_MASK:
-        s->irq_mask = value;
+        s->irq_mask = value & 0xFFDF3F5F;
         break;
     case SM501_MISC_TIMING:
         s->misc_timing = value & 0xF31F1FFF;
Signature · 1 line
2.7.6
Applied
#15 lore raw

Re: [Qemu-devel] [PATCH v3 6/9] sm501: Do not clear read only bits when writing registers

Quoted 5 lines · BALATON Zoltan

When writing registers that have read only bits we have to avoid changing these bits as they may have non zero values. Make sure we use the correct masks to mask out read only and reserved bits when changing registers.

Quoted 7 lines

Also remove extra spaces from dram_control and arbitration_control assignments.

---
v3: Not only preserve read only bits but also allow clearing r/w bits

Applied to ppc-for-3.0, thanks.

Quoted 45 lines
 hw/display/sm501.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
hw/display/sm501.c +9 -6
index e47be99..ca0840f 100644
@@ -836,27 +836,30 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case SM501_SYSTEM_CONTROL:
-        s->system_control = value & 0xE300B8F7;
+        s->system_control &= 0x10DB0000;
+        s->system_control |= value & 0xEF00B8F7;
         break;
     case SM501_MISC_CONTROL:
-        s->misc_control = value & 0xFF7FFF20;
+        s->misc_control &= 0xEF;
+        s->misc_control |= value & 0xFF7FFF10;
         break;
     case SM501_GPIO31_0_CONTROL:
         s->gpio_31_0_control = value;
         break;
     case SM501_GPIO63_32_CONTROL:
-        s->gpio_63_32_control = value;
+        s->gpio_63_32_control = value & 0xFF80FFFF;
         break;
     case SM501_DRAM_CONTROL:
         s->local_mem_size_index = (value >> 13) & 0x7;
         /* TODO : check validity of size change */
-        s->dram_control |=  value & 0x7FFFFFC3;
+        s->dram_control &= 0x80000000;
+        s->dram_control |= value & 0x7FFFFFC3;
         break;
     case SM501_ARBTRTN_CONTROL:
-        s->arbitration_control =  value & 0x37777777;
+        s->arbitration_control = value & 0x37777777;
         break;
     case SM501_IRQ_MASK:
-        s->irq_mask = value;
+        s->irq_mask = value & 0xFFDF3F5F;
         break;
     case SM501_MISC_TIMING:
         s->misc_timing = value & 0xF31F1FFF;
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#16 lore raw

PATCH v3 7/9 sm501: Implement i2c part for reading monitor EDID

In reply to BALATON Zoltan
---
 default-configs/ppc-softmmu.mak    |   1 +
 default-configs/ppcemb-softmmu.mak |   1 +
 default-configs/sh4-softmmu.mak    |   1 +
 default-configs/sh4eb-softmmu.mak  |   1 +
 hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
 5 files changed, 136 insertions(+), 4 deletions(-)
default-configs/ppc-softmmu.mak +1 -0
index 9fbaadc..860de80 100644
@@ -24,6 +24,7 @@ CONFIG_ETSEC=y
 # For Sam460ex
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
default-configs/ppcemb-softmmu.mak +1 -0
index 37af193..ac44f15 100644
@@ -17,6 +17,7 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
default-configs/sh4-softmmu.mak +1 -0
index 546d855..72d8fca 100644
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
default-configs/sh4eb-softmmu.mak +1 -0
index 2d3fd49..c686637 100644
@@ -9,6 +9,7 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
hw/display/sm501.c +132 -4

206 lines not on this page · raw message

Signature · 1 line
2.7.6
#17 lore raw

Re: [Qemu-devel] [PATCH v3 7/9] sm501: Implement i2c part for reading monitor EDID

Quoted 1 line · BALATON Zoltan

Again needs a commit message expanding on what this is and why it's useful.

[snip]

hw/display/sm501.c static void sm501_reset_sysbus(DeviceState *dev) · line 1905

Quoted 9 lines
 static const VMStateDescription vmstate_sm501_sysbus = {
     .name = TYPE_SYSBUS_SM501,
hw/display/sm501.c +2 -2
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(state, SM501SysBusState, 1,
                        vmstate_sm501_state, SM501State),

Additionally, since you're changing the migration stream, you need to point out that this is not used in any machine types which support cross-version migration.

Quoted 11 lines
hw/display/sm501.c +2 -2
@@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
 
 static const VMStateDescription vmstate_sm501_pci = {
     .name = TYPE_PCI_SM501,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
         VMSTATE_STRUCT(state, SM501PCIState, 1,
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#18 lore raw

Re: [Qemu-devel] [PATCH v3 7/9] sm501: Implement i2c part for reading monitor EDID

Quoted 5 lines · David Gibson
Quoted 1 line · BALATON Zoltan

Again needs a commit message expanding on what this is and why it's useful.

What else need to be explained apart from that SM501 has an i2c part which is used to access EDID info of attached monitor. But this is what the commit title already says. This patch implements that so far not emulated part of the chip.

Quoted 11 lines

[snip]

hw/display/sm501.c static void sm501_reset_sysbus(DeviceState *dev) · line 1905

Quoted 9 lines
 static const VMStateDescription vmstate_sm501_sysbus = {
     .name = TYPE_SYSBUS_SM501,
hw/display/sm501.c +2 -2
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(state, SM501SysBusState, 1,
                        vmstate_sm501_state, SM501State),
Quoted 3 lines

Additionally, since you're changing the migration stream, you need to point out that this is not used in any machine types which support cross-version migration.

AFAIK it is only used in SH4 and PPC sam460ex.

#19 lore raw

Re: [Qemu-devel] [PATCH v3 7/9] sm501: Implement i2c part for reading monitor EDID

Quoted 11 lines · BALATON Zoltan
Quoted 5 lines · David Gibson
Quoted 1 line · BALATON Zoltan

Again needs a commit message expanding on what this is and why it's useful.

What else need to be explained apart from that SM501 has an i2c part which is used to access EDID info of attached monitor. But this is what the commit title already says. This patch implements that so far not emulated part of the chip.

In this case it would be sufficient just to say the same thing, but in a less compressed form than the subject line. e.g.

| The sm501 controller includes an i2c slave from which the monitor's
| EDID information can be read.  This wasn't yet implemented in qemu,
| so this patch adds it.
|
| (The sm501 part is used only by the SH4 and PPC sam460ex machine
| types in qemu)
Quoted 18 lines
Quoted 12 lines

[snip]

hw/display/sm501.c static void sm501_reset_sysbus(DeviceState *dev) · line 1905

Quoted 9 lines
 static const VMStateDescription vmstate_sm501_sysbus = {
     .name = TYPE_SYSBUS_SM501,
hw/display/sm501.c +2 -2
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(state, SM501SysBusState, 1,
                        vmstate_sm501_state, SM501State),
Quoted 3 lines

Additionally, since you're changing the migration stream, you need to point out that this is not used in any machine types which support cross-version migration.

AFAIK it is only used in SH4 and PPC sam460ex.

Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#20 lore raw

PATCH v3 8/9 sm501: Perform a full update after palette change

In reply to BALATON Zoltan

From: Sebastian Bauer <[email protected]>

---
 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)
hw/display/sm501.c +8 -0
index 0625cf5..a2ee6e3 100644
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
Signature · 1 line
2.7.6
#21 lore raw

Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

Quoted 4 lines · BALATON Zoltan

From: Sebastian Bauer <[email protected]>

Commit message. Why is this necessary?

Quoted 37 lines
---
 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)
hw/display/sm501.c +8 -0
index 0625cf5..a2ee6e3 100644
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#22 lore raw

Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

Quoted 7 lines · David Gibson
Quoted 4 lines · BALATON Zoltan

From: Sebastian Bauer <[email protected]>

Commit message. Why is this necessary?

Maybe because changing palette does not change display unless an update is done and dirty tracking which is used in update_display does not detect changes in device registers where palette is stored.

I'm not sure this is needed in all modes but I guess palette is not used in modes that are not indexed so unecessary updates should not happen evem in those cases because if palette is not used anyway, guest is unlikely to change it unless doing something really strange but we haven't seen any guests yet that would do that. Therefore this simple way of handling this should be OK.

Regards,
BALATON Zoltan

Quoted 39 lines
Quoted 37 lines
---
 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)
hw/display/sm501.c +8 -0
index 0625cf5..a2ee6e3 100644
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
#23 lore raw

Re: [Qemu-devel] [PATCH v3 8/9] sm501: Perform a full update after palette change

Quoted 13 lines · BALATON Zoltan
Quoted 7 lines · David Gibson
Quoted 4 lines · BALATON Zoltan

From: Sebastian Bauer <[email protected]>

Commit message. Why is this necessary?

Maybe because changing palette does not change display unless an update is done and dirty tracking which is used in update_display does not detect changes in device registers where palette is stored.

Quoted 6 lines

I'm not sure this is needed in all modes but I guess palette is not used in modes that are not indexed so unecessary updates should not happen evem in those cases because if palette is not used anyway, guest is unlikely to change it unless doing something really strange but we haven't seen any guests yet that would do that. Therefore this simple way of handling this should be OK.

Sure, so, put that into a commit message in the next spin.

Quoted 44 lines

Regards,
BALATON Zoltan

Quoted 39 lines
Quoted 37 lines
---
 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)
hw/display/sm501.c +8 -0
index 0625cf5..a2ee6e3 100644
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#24 lore raw

PATCH v3 9/9 target/ppc: Add missing opcode for icbt on PPC440

In reply to BALATON Zoltan

PPC440 has two opcodes for icbt, add the missing one.

---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
Signature · 1 line
2.7.6
#25 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

Quoted 1 line · BALATON Zoltan

PPC440 has two opcodes for icbt, add the missing one.

A document reference to confim this would be nice.

Quoted 18 lines
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#26 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

Quoted 1 line · David Gibson

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:

Maybe amend commit message like this:

According to PPC440 User Manual

Quoted 1 line
Quoted 1 line

PPC440 has two opcodes for icbt, add the missing one.

If you can do this when committing then please feel free to adjust this commit message as necessary, otherwise I'll change it in next iteration.

Regards,
BALATON Zoltan

Quoted 22 lines

A document reference to confim this would be nice.

Quoted 18 lines
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
#27 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

Quoted 6 lines · BALATON Zoltan
Quoted 1 line · David Gibson

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:

Maybe amend commit message like this:

According to PPC440 User Manual

On which page? Where can I get that manual?

Quoted 31 lines
Quoted 1 line
Quoted 1 line

PPC440 has two opcodes for icbt, add the missing one.

If you can do this when committing then please feel free to adjust this commit message as necessary, otherwise I'll change it in next iteration.

Regards,
BALATON Zoltan

Quoted 22 lines

A document reference to confim this would be nice.

Quoted 18 lines
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#28 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

Quoted 9 lines · David Gibson
Quoted 6 lines · BALATON Zoltan
Quoted 1 line · David Gibson

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:

Maybe amend commit message like this:

According to PPC440 User Manual

On which page? Where can I get that manual?

By searching for "PPC440 User Manual"? The one I've found had an opcode table at the end in an appendix but I've seen one guest using this machine code and get invalid instruction on QEMU while apparently it works on real hardware (but I can't test that myself as I don't have real hardware).

Regards,
BALATON Zoltan

Quoted 33 lines
Quoted 31 lines
Quoted 1 line
Quoted 1 line

PPC440 has two opcodes for icbt, add the missing one.

If you can do this when committing then please feel free to adjust this commit message as necessary, otherwise I'll change it in next iteration.

Regards,
BALATON Zoltan

Quoted 22 lines

A document reference to confim this would be nice.

Quoted 18 lines
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
#29 lore raw

Re: [Qemu-devel] [PATCH v3 9/9] target/ppc: Add missing opcode for icbt on PPC440

Quoted 9 lines · BALATON Zoltan
Quoted 8 lines · David Gibson
Quoted 6 lines · BALATON Zoltan
Quoted 1 line · David Gibson

On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:

Maybe amend commit message like this:

According to PPC440 User Manual

Quoted 7 lines
Quoted 1 line

On which page? Where can I get that manual?

By searching for "PPC440 User Manual"? The one I've found had an opcode table at the end in an appendix but I've seen one guest using this machine code and get invalid instruction on QEMU while apparently it works on real hardware (but I can't test that myself as I don't have real hardware).

Ok, I'm looking for something like this.

| According to the PPC440 User Manual[0] page XXXX, PPC440 has two
| opcodes for icbt, but qemu currently only implements one of them.
| Add the missing one.
|
| [0] https://link/to/the/manual (if you can find one)
Quoted 38 lines

Regards,
BALATON Zoltan

Quoted 33 lines
Quoted 31 lines
Quoted 1 line
Quoted 1 line

PPC440 has two opcodes for icbt, add the missing one.

If you can do this when committing then please feel free to adjust this commit message as necessary, otherwise I'll change it in next iteration.

Regards,
BALATON Zoltan

Quoted 22 lines

A document reference to confim this would be nice.

Quoted 18 lines
---
 target/ppc/translate.c | 2 ++
 1 file changed, 2 insertions(+)
target/ppc/translate.c +2 -0
index 5fe1ba6..3a215a1 100644
@@ -6707,6 +6707,8 @@ GEN_HANDLER_E(mbar, 0x1F, 0x16, 0x1a, 0x001FF801,
 GEN_HANDLER(msync_4xx, 0x1F, 0x16, 0x12, 0x03FFF801, PPC_BOOKE),
 GEN_HANDLER2_E(icbt_440, "icbt", 0x1F, 0x16, 0x00, 0x03E00001,
                PPC_BOOKE, PPC2_BOOKE206),
+GEN_HANDLER2(icbt_440, "icbt", 0x1F, 0x06, 0x08, 0x03E00001,
+               PPC_440_SPEC),
 GEN_HANDLER(lvsl, 0x1f, 0x06, 0x00, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(lvsr, 0x1f, 0x06, 0x01, 0x00000001, PPC_ALTIVEC),
 GEN_HANDLER(mfvscr, 0x04, 0x2, 0x18, 0x001ff800, PPC_ALTIVEC),
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson