Re: [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5716 bytes --]

On Fri, Jun 22, 2018 at 10:00:34AM +0200, BALATON Zoltan wrote:
> On Fri, 22 Jun 2018, David Gibson wrote:
> > 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 <balaton@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?
> 
> I'm setting bit 0 (MSCL) to either 1 or 0. I could probably use the MSCL
> constant in place of the 1 here but would that make it clearer?

Yeah, it kinda would.  Instead of the line saying "it sets bit 0 based
on this" it would say "this sets the MSCL bit based on this" which is
probably more useful information.  In addition, if someone greps
looking find everywhere the MSCL bit is manipulated, they'll find it.

> It would
> just be longer and less clear without looking up the constants so to me this
> looks more comprehensible this way.
> 
> > > > > +        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.
> 
> But how? Third arg of bitbang_i2c_set is level, either 0 or 1. How could a
> constant with value 2 used here? (Also to set bit 1 I have to shift 1 not 2
> so I don't see how the constant could be used there either.)

Don't use the shift.

i2c->directcntl |= bitbang_i2c_set(i2c->bitmang, BITBANG_I2C_SDA,
				   !!(value & IIC_DIRECTCNTL_SDAC))
			? IIC_DIRECTCNTL_MSDA : 0;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]