Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
On Wed, 20 Jun 2018, David Gibson wrote:
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.
OK, I'm lost now. Is v4 acceptable or are there any more changes you
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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)
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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, I
think 1 : 0 is correct there. 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. 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.
Regards,
BALATON Zoltan
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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
> > > > > ---
> > > > > 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
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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
---
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
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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
> > > ---
> > > 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?
> If this does not explain your question and you think it's better to use
> defines here I can change that in next version, please let me know.
>
> 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
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
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
---
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.
If this does not explain your question and you think it's better to use
defines here I can change that in next version, please let me know.
Regards,
BALATON Zoltan
Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register
On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan
> ---
> 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
> break;
> default:
> if (addr < PPC4xx_I2C_MEM_SIZE) {
> @@ -343,6 +354,7 @@ static void ppc4xx_i2c_init(Object *o)
> sysbus_init_mmio(SYS_BUS_DEVICE(s), >iomem);
> sysbus_init_irq(SYS_BUS_DEVICE(s), >irq);
> s->bus = i2c_init_bus(DEVICE(s), "i2c");
> +r->bitbang = bitbang_i2c_init(s->bus);
> }
>
> static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
--
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
