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: 5611 bytes --]

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:
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > ---
> > > > >  default-configs/ppc-softmmu.mak    |  1 +
> > > > >  default-configs/ppcemb-softmmu.mak |  1 +
> > > > >  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
> > > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > > > index 4d7be45..7d0dc2f 100644
> > > > > --- a/default-configs/ppc-softmmu.mak
> > > > > +++ b/default-configs/ppc-softmmu.mak
> > > > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > >  CONFIG_SM501=y
> > > > >  CONFIG_IDE_SII3112=y
> > > > >  CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > > 
> > > > >  # For Macs
> > > > >  CONFIG_MAC=y
> > > > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > > > index 67d18b2..37af193 100644
> > > > > --- a/default-configs/ppcemb-softmmu.mak
> > > > > +++ b/default-configs/ppcemb-softmmu.mak
> > > > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > > > >  CONFIG_SM501=y
> > > > >  CONFIG_IDE_SII3112=y
> > > > >  CONFIG_I2C=y
> > > > > +CONFIG_BITBANG_I2C=y
> > > > > 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.

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

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