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 <bala...@eik.bme.hu> > > > --- > > > 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. > In summary, guest sets SDAC and SCLC as it wants the i2c lines and MSDA and > MSCL are set by the device to what state the lines are actually in. (The S > in first two regs may stand for Set while M stands for Monitor.) > > Regards, > BALATON Zoltan > -- 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
signature.asc
Description: PGP signature