Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

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

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

On Tue, Jun 19, 2018 at 11:29:09AM +0200, BALATON Zoltan wrote:
> On Mon, 18 Jun 2018, David Gibson wrote:
> > On Wed, Jun 13, 2018 at 04:03:18PM +0200, BALATON Zoltan wrote:
> > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> > > > > On Wed, 13 Jun 2018, David Gibson wrote:
> > > > > > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > > > index a68b5f7..5806209 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,7 +47,13 @@
> > > > > > > 
> > > > > > >  #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)
> > > > > > > +
> > > > > > >  typedef struct {
> > > > > > > +    bitbang_i2c_interface *bitbang;
> > > > > > >      uint8_t mdata;
> > > > > > >      uint8_t lmadr;
> > > > > > >      uint8_t hmadr;
> > > > > > > @@ -308,7 +315,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);
> > > > > > 
> > > > > > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > > > > > 
> > > > > > > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > > > > > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > > > > > 
> > > > > > Last expression might be clearer as:
> > > > > > 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> > > > > 
> > > > > I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> > > > > position in the register so I use that when accessing that bit but when I
> > > > > check for the values of a bit being 0 or 1 I don't use the define which is
> > > > > for something else, just happens to have value 1 as well.
> > > > 
> > > > Hmm.. but the bit is being store in i2c->directcntl, which means it
> > > > can be read back from the register in that position, no?
> > > 
> > > Which of the above two do you mean?
> > > 
> > > In the first one I test for the 1/0 value set by the previous line before
> > > the bitbang_i2c_set call. This could be accessed as MSCL later but using
> > > that here would just make it longer and less obvious. If I want to be
> > > absolutely precise maybe it should be (value & IIC_DIRECTCNTL_SCL ? 1 : 0)
> > > in this line too but that was just stored in the register one line before so
> > > I can reuse that here as well. Otherwise I could add another variable just
> > > for this bit value and use that in both lines but why make it more
> > > complicated for a simple 1 or 0 value?
> > 
> > Longer maybe, but I don't know about less obvious.  Actually I think
> > you should use IIC_DIRECTCNTL_MSCL instead of a bare '1' in both the
> > line setting i2c->directcntl, then the next line checking that bit to
> > pass it into bitbang_i2c_set.  The point is you're modifying the
> > effective register contents, so it makes sense to make it clearer
> > which bit of the register you're setting.
> 
> When setting the bit it's the value 1 so that's not the bit
> position,

Huh??  The constants aren't bit positions either, they're masks.  How
is IIC_DIRECTCNTL_MSCL wrong here?

> I
> think 1 : 0 is correct there.

Correct, sure, but less clear than it could be.

> I've changed the next line in v4 I've just
> sent to the constant when checking the value of the MSCL bit.
> 
> > > In the second case using MSDA is really not correct because the level to set
> > > is defined by SDAC bit. The SDAC, SCLC bits are what the program sets to
> > > tell which states the two i2c lines should be and the MSDA, MSCL are read
> > > only bits that show what states the lines really are.
> > 
> > Ok...
> > 
> > > IIC_DIRECTCNTL_MSDA has value of 1 but it means the second bit in the
> > > directcntl reg (which could have 0 or 1 value) not 1 value of a bit or i2c
> > > line.
> > 
> > Uh.. what?  AFAICT, based on the result of bitbang_i2c_set() you're
> > updating the value of the MSDA (== 0x2) bit in i2c->directcntl
> > register state.  Why doesn't the symbolic name make sense here?
> 
> Sorry, I may not have been able to clearly say what I mean. I meant that
> IIC_DIRECTCNTL_MSDA means the bit in position 1 (numbering from LSB being
> bit number 0) which may have value 1 or 0. In cases I mean the value I use 1
> or 0. In case I refer to the bit position I use constants. In the line
> 
> bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> 
> it should be the constant, just used 1 there for brevity because it's
> obvious from the previous line what's meant.

Maybe, but using the constant is still clearer, and friendly to people
grepping the source.

> I've changed this now. At other
> places the values of the bits are written as 1 or 0 so I think for those
> constants should not be needed.

I have no idea what you mean by this.

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