On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote:
On Wed, 20 Jun 2018, David Gibson wrote:
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
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.
Signed-off-by: BALATON Zoltan <address@hidden>
---
v4: Updated commit message and use defined constant where
appropriate
I'm still don't quite understand your approach to the symbolic
constants here, but I don't care enough to hold this up any further.
So, applied to ppc-for-3.0.
Thanks, just to try to clear this up, I consider symbolic constants to be
the name of bits 0-3 in the directntl register so while MSCL equals 1 it's
only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of
directcntl reg.
Right..
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index 4e0aaae..fca80d6 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -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,12 @@ 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);
This clears all bits but SDAC and SCLC so constants are OK here as they
refer to bits in the register. (Guest can set the S* bits to say what state
it wants the i2c lines to become.)
+ i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as
SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans
are not appropriate here.
This is what I don't get. Regardless of the method of it, you *are*
setting bit 1 of the directcntl register, so why would the MSCL name
not be appropriate?
+ bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
+ i2c->directcntl & IIC_DIRECTCNTL_MSCL);
This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so
constant here is OK, previously it was just 1 for brevity which may have
confused you.
+ i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
+ (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
This sets MSDA bit of directcntl to the value returned by bitbang_i2c
emulation when sending it the bit in SDAC. So the
(value & IIC_DIRECTCNTL_SDAC) != 0)
tests what value the SDAC bit has so 0 means the value of the bit and
constant refers to the bit in the register. (Because SDAC is not the LSB and
we need 1 or 0 here hence the equality test to normalise the value, maybe
the !! construct could also be used, I'm not sure.) The << 1 at the end
makes sure we set the MSDA bit but that constant cannot be used here and
using MSCL instead is not correct because we mean the MSDA bit.
Right, I'm not suggesting you use MSCL here, I'm suggesting you use
MSDA.