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

[David Gibson]

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.

> 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