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 <bala...@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
signature.asc
Description: PGP signature
