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

[BALATON Zoltan]

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 <address@hidden>
> > > > ---
> > > >  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?

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.

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.

Regards,
BALATON Zoltan