| Message ID | ce619ddd11a875caae4c221c18ebf7c6a6f4bd4f.1529398335.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc sam460ex improvements | expand |
On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote: > As well as being able to generate its own i2c transactions, the ppc4xx > i2c controller has a DIRECTCNTL register which allows explicit control > of the i2c lines. > > Using this register an OS can directly bitbang i2c operations. In > order to let emulated i2c devices respond to this, we need to wire up > the DIRECTCNTL register to qemu's bitbanged i2c handling code. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > v4: Updated commit message and use defined constant where > appropriate I'm still don't quite understand your approach to the symbolic constants here, but I don't care enough to hold this up any further. So, applied to ppc-for-3.0. > > default-configs/ppc-softmmu.mak | 1 + > default-configs/ppcemb-softmmu.mak | 1 + > hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++- > include/hw/i2c/ppc4xx_i2c.h | 4 ++++ > 4 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak > index abeeb04..851b4af 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 4e0aaae..fca80d6 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,6 +47,11 @@ > > #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) > + > static void ppc4xx_i2c_reset(DeviceState *s) > { > PPC4xxI2CState *i2c = PPC4xx_I2C(s); > @@ -289,7 +295,12 @@ 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 & IIC_DIRECTCNTL_MSCL); > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; > break; > default: > if (addr < PPC4xx_I2C_MEM_SIZE) { > @@ -322,6 +333,7 @@ static void ppc4xx_i2c_init(Object *o) > sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); > sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq); > s->bus = i2c_init_bus(DEVICE(s), "i2c"); > + s->bitbang = bitbang_i2c_init(s->bus); > } > > static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data) > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h > index e4b6ded..ea6c8e1 100644 > --- a/include/hw/i2c/ppc4xx_i2c.h > +++ b/include/hw/i2c/ppc4xx_i2c.h > @@ -31,6 +31,9 @@ > #include "hw/sysbus.h" > #include "hw/i2c/i2c.h" > > +/* from hw/i2c/bitbang_i2c.h */ > +typedef struct bitbang_i2c_interface bitbang_i2c_interface; > + > #define TYPE_PPC4xx_I2C "ppc4xx-i2c" > #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C) > > @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState { > I2CBus *bus; > qemu_irq irq; > MemoryRegion iomem; > + bitbang_i2c_interface *bitbang; > uint8_t mdata; > uint8_t lmadr; > uint8_t hmadr;
On Wed, 20 Jun 2018, David Gibson wrote: > On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote: >> As well as being able to generate its own i2c transactions, the ppc4xx >> i2c controller has a DIRECTCNTL register which allows explicit control >> of the i2c lines. >> >> Using this register an OS can directly bitbang i2c operations. In >> order to let emulated i2c devices respond to this, we need to wire up >> the DIRECTCNTL register to qemu's bitbanged i2c handling code. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> v4: Updated commit message and use defined constant where >> appropriate > > I'm still don't quite understand your approach to the symbolic > constants here, but I don't care enough to hold this up any further. > So, applied to ppc-for-3.0. Thanks, just to try to clear this up, I consider symbolic constants to be the name of bits 0-3 in the directntl register so while MSCL equals 1 it's only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of directcntl reg. >> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c >> index 4e0aaae..fca80d6 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,6 +47,11 @@ >> >> #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) >> + >> static void ppc4xx_i2c_reset(DeviceState *s) >> { >> PPC4xxI2CState *i2c = PPC4xx_I2C(s); >> @@ -289,7 +295,12 @@ 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); This clears all bits but SDAC and SCLC so constants are OK here as they refer to bits in the register. (Guest can set the S* bits to say what state it wants the i2c lines to become.) >> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0); This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans are not appropriate here. >> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, >> + i2c->directcntl & IIC_DIRECTCNTL_MSCL); This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so constant here is OK, previously it was just 1 for brevity which may have confused you. >> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, >> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; This sets MSDA bit of directcntl to the value returned by bitbang_i2c emulation when sending it the bit in SDAC. So the (value & IIC_DIRECTCNTL_SDAC) != 0) tests what value the SDAC bit has so 0 means the value of the bit and constant refers to the bit in the register. (Because SDAC is not the LSB and we need 1 or 0 here hence the equality test to normalise the value, maybe the !! construct could also be used, I'm not sure.) The << 1 at the end makes sure we set the MSDA bit but that constant cannot be used here and using MSCL instead is not correct because we mean the MSDA bit. In summary, guest sets SDAC and SCLC as it wants the i2c lines and MSDA and MSCL are set by the device to what state the lines are actually in. (The S in first two regs may stand for Set while M stands for Monitor.) Regards, BALATON Zoltan
On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote: > On Wed, 20 Jun 2018, David Gibson wrote: > > On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote: > > > As well as being able to generate its own i2c transactions, the ppc4xx > > > i2c controller has a DIRECTCNTL register which allows explicit control > > > of the i2c lines. > > > > > > Using this register an OS can directly bitbang i2c operations. In > > > order to let emulated i2c devices respond to this, we need to wire up > > > the DIRECTCNTL register to qemu's bitbanged i2c handling code. > > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > --- > > > v4: Updated commit message and use defined constant where > > > appropriate > > > > I'm still don't quite understand your approach to the symbolic > > constants here, but I don't care enough to hold this up any further. > > So, applied to ppc-for-3.0. > > Thanks, just to try to clear this up, I consider symbolic constants to be > the name of bits 0-3 in the directntl register so while MSCL equals 1 it's > only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of > directcntl reg. Right.. > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c > > > index 4e0aaae..fca80d6 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,6 +47,11 @@ > > > > > > #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) > > > + > > > static void ppc4xx_i2c_reset(DeviceState *s) > > > { > > > PPC4xxI2CState *i2c = PPC4xx_I2C(s); > > > @@ -289,7 +295,12 @@ 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); > > This clears all bits but SDAC and SCLC so constants are OK here as they > refer to bits in the register. (Guest can set the S* bits to say what state > it wants the i2c lines to become.) > > > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0); > > This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as > SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans > are not appropriate here. This is what I don't get. Regardless of the method of it, you *are* setting bit 1 of the directcntl register, so why would the MSCL name not be appropriate? > > > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, > > > + i2c->directcntl & IIC_DIRECTCNTL_MSCL); > > This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so > constant here is OK, previously it was just 1 for brevity which may have > confused you. > > > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, > > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; > > This sets MSDA bit of directcntl to the value returned by bitbang_i2c > emulation when sending it the bit in SDAC. So the > (value & IIC_DIRECTCNTL_SDAC) != 0) > tests what value the SDAC bit has so 0 means the value of the bit and > constant refers to the bit in the register. (Because SDAC is not the LSB and > we need 1 or 0 here hence the equality test to normalise the value, maybe > the !! construct could also be used, I'm not sure.) The << 1 at the end > makes sure we set the MSDA bit but that constant cannot be used here and > using MSCL instead is not correct because we mean the MSDA bit. Right, I'm not suggesting you use MSCL here, I'm suggesting you use MSDA. > In summary, guest sets SDAC and SCLC as it wants the i2c lines and MSDA and > MSCL are set by the device to what state the lines are actually in. (The S > in first two regs may stand for Set while M stands for Monitor.) > > Regards, > BALATON Zoltan >
On Fri, 22 Jun 2018, David Gibson wrote: > On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote: >> On Wed, 20 Jun 2018, David Gibson wrote: >>> On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote: >>>> As well as being able to generate its own i2c transactions, the ppc4xx >>>> i2c controller has a DIRECTCNTL register which allows explicit control >>>> of the i2c lines. >>>> >>>> Using this register an OS can directly bitbang i2c operations. In >>>> order to let emulated i2c devices respond to this, we need to wire up >>>> the DIRECTCNTL register to qemu's bitbanged i2c handling code. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> v4: Updated commit message and use defined constant where >>>> appropriate >>> >>> I'm still don't quite understand your approach to the symbolic >>> constants here, but I don't care enough to hold this up any further. >>> So, applied to ppc-for-3.0. >> >> Thanks, just to try to clear this up, I consider symbolic constants to be >> the name of bits 0-3 in the directntl register so while MSCL equals 1 it's >> only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of >> directcntl reg. > > Right.. > >> >>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c >>>> index 4e0aaae..fca80d6 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,6 +47,11 @@ >>>> >>>> #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) >>>> + >>>> static void ppc4xx_i2c_reset(DeviceState *s) >>>> { >>>> PPC4xxI2CState *i2c = PPC4xx_I2C(s); >>>> @@ -289,7 +295,12 @@ 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); >> >> This clears all bits but SDAC and SCLC so constants are OK here as they >> refer to bits in the register. (Guest can set the S* bits to say what state >> it wants the i2c lines to become.) >> >>>> + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0); >> >> This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as >> SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans >> are not appropriate here. > > This is what I don't get. Regardless of the method of it, you *are* > setting bit 1 of the directcntl register, so why would the MSCL name > not be appropriate? I'm setting bit 0 (MSCL) to either 1 or 0. I could probably use the MSCL constant in place of the 1 here but would that make it clearer? It would just be longer and less clear without looking up the constants so to me this looks more comprehensible this way. >>>> + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, >>>> + i2c->directcntl & IIC_DIRECTCNTL_MSCL); >> >> This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so >> constant here is OK, previously it was just 1 for brevity which may have >> confused you. >> >>>> + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, >>>> + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; >> >> This sets MSDA bit of directcntl to the value returned by bitbang_i2c >> emulation when sending it the bit in SDAC. So the >> (value & IIC_DIRECTCNTL_SDAC) != 0) >> tests what value the SDAC bit has so 0 means the value of the bit and >> constant refers to the bit in the register. (Because SDAC is not the LSB and >> we need 1 or 0 here hence the equality test to normalise the value, maybe >> the !! construct could also be used, I'm not sure.) The << 1 at the end >> makes sure we set the MSDA bit but that constant cannot be used here and >> using MSCL instead is not correct because we mean the MSDA bit. > > Right, I'm not suggesting you use MSCL here, I'm suggesting you use > MSDA. But how? Third arg of bitbang_i2c_set is level, either 0 or 1. How could a constant with value 2 used here? (Also to set bit 1 I have to shift 1 not 2 so I don't see how the constant could be used there either.) Regards, BALATON Zoltan
On Fri, Jun 22, 2018 at 10:00:34AM +0200, BALATON Zoltan wrote: > On Fri, 22 Jun 2018, David Gibson wrote: > > On Thu, Jun 21, 2018 at 09:17:11AM +0200, BALATON Zoltan wrote: > > > On Wed, 20 Jun 2018, David Gibson wrote: > > > > On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote: > > > > > As well as being able to generate its own i2c transactions, the ppc4xx > > > > > i2c controller has a DIRECTCNTL register which allows explicit control > > > > > of the i2c lines. > > > > > > > > > > Using this register an OS can directly bitbang i2c operations. In > > > > > order to let emulated i2c devices respond to this, we need to wire up > > > > > the DIRECTCNTL register to qemu's bitbanged i2c handling code. > > > > > > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > > > --- > > > > > v4: Updated commit message and use defined constant where > > > > > appropriate > > > > > > > > I'm still don't quite understand your approach to the symbolic > > > > constants here, but I don't care enough to hold this up any further. > > > > So, applied to ppc-for-3.0. > > > > > > Thanks, just to try to clear this up, I consider symbolic constants to be > > > the name of bits 0-3 in the directntl register so while MSCL equals 1 it's > > > only appropriate to use the constant if I really mean (1 << 0) i.e. bit 0 of > > > directcntl reg. > > > > Right.. > > > > > > > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c > > > > > index 4e0aaae..fca80d6 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,6 +47,11 @@ > > > > > > > > > > #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) > > > > > + > > > > > static void ppc4xx_i2c_reset(DeviceState *s) > > > > > { > > > > > PPC4xxI2CState *i2c = PPC4xx_I2C(s); > > > > > @@ -289,7 +295,12 @@ 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); > > > > > > This clears all bits but SDAC and SCLC so constants are OK here as they > > > refer to bits in the register. (Guest can set the S* bits to say what state > > > it wants the i2c lines to become.) > > > > > > > > + i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0); > > > > > > This is directcntl[MSCL] = direcntl[SCLC] that is, set MSCL bit the same as > > > SCLC, the 1 : 0 here are the value of the bit not the MSCL bit so constans > > > are not appropriate here. > > > > This is what I don't get. Regardless of the method of it, you *are* > > setting bit 1 of the directcntl register, so why would the MSCL name > > not be appropriate? > > I'm setting bit 0 (MSCL) to either 1 or 0. I could probably use the MSCL > constant in place of the 1 here but would that make it clearer? Yeah, it kinda would. Instead of the line saying "it sets bit 0 based on this" it would say "this sets the MSCL bit based on this" which is probably more useful information. In addition, if someone greps looking find everywhere the MSCL bit is manipulated, they'll find it. > It would > just be longer and less clear without looking up the constants so to me this > looks more comprehensible this way. > > > > > > + bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, > > > > > + i2c->directcntl & IIC_DIRECTCNTL_MSCL); > > > > > > This lets the bitbang_i2c emulation also know that MSCL is set to 1 or 0 so > > > constant here is OK, previously it was just 1 for brevity which may have > > > confused you. > > > > > > > > + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, > > > > > + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; > > > > > > This sets MSDA bit of directcntl to the value returned by bitbang_i2c > > > emulation when sending it the bit in SDAC. So the > > > (value & IIC_DIRECTCNTL_SDAC) != 0) > > > tests what value the SDAC bit has so 0 means the value of the bit and > > > constant refers to the bit in the register. (Because SDAC is not the LSB and > > > we need 1 or 0 here hence the equality test to normalise the value, maybe > > > the !! construct could also be used, I'm not sure.) The << 1 at the end > > > makes sure we set the MSDA bit but that constant cannot be used here and > > > using MSCL instead is not correct because we mean the MSDA bit. > > > > Right, I'm not suggesting you use MSCL here, I'm suggesting you use > > MSDA. > > But how? Third arg of bitbang_i2c_set is level, either 0 or 1. How could a > constant with value 2 used here? (Also to set bit 1 I have to shift 1 not 2 > so I don't see how the constant could be used there either.) Don't use the shift. i2c->directcntl |= bitbang_i2c_set(i2c->bitmang, BITBANG_I2C_SDA, !!(value & IIC_DIRECTCNTL_SDAC)) ? IIC_DIRECTCNTL_MSDA : 0;
On 2018-06-19 10:52, BALATON Zoltan wrote: > As well as being able to generate its own i2c transactions, the ppc4xx > i2c controller has a DIRECTCNTL register which allows explicit control > of the i2c lines. > > Using this register an OS can directly bitbang i2c operations. In > order to let emulated i2c devices respond to this, we need to wire up > the DIRECTCNTL register to qemu's bitbanged i2c handling code. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- [...] > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h > index e4b6ded..ea6c8e1 100644 > --- a/include/hw/i2c/ppc4xx_i2c.h > +++ b/include/hw/i2c/ppc4xx_i2c.h > @@ -31,6 +31,9 @@ > #include "hw/sysbus.h" > #include "hw/i2c/i2c.h" > > +/* from hw/i2c/bitbang_i2c.h */ > +typedef struct bitbang_i2c_interface bitbang_i2c_interface; This breaks compilation with clang 3.4: In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33: hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef 'bitbang_i2c_interface' is a C11 feature [-Werror,-Wtypedef-redefinition] typedef struct bitbang_i2c_interface bitbang_i2c_interface; ^ include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here typedef struct bitbang_i2c_interface bitbang_i2c_interface; ^ 1 error generated. make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1 Not sure about the best way to fix this ... move the typedef to include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h" header instead of "i2c.h" here? Thomas PS: This pops up quite frequently and is annoying ... Did we ever consider to enforce "-std=gnu11" for compiling QEMU?
On Wed, 28 Nov 2018, Thomas Huth wrote: > On 2018-06-19 10:52, BALATON Zoltan wrote: >> As well as being able to generate its own i2c transactions, the ppc4xx >> i2c controller has a DIRECTCNTL register which allows explicit control >> of the i2c lines. >> >> Using this register an OS can directly bitbang i2c operations. In >> order to let emulated i2c devices respond to this, we need to wire up >> the DIRECTCNTL register to qemu's bitbanged i2c handling code. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- > [...] >> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h >> index e4b6ded..ea6c8e1 100644 >> --- a/include/hw/i2c/ppc4xx_i2c.h >> +++ b/include/hw/i2c/ppc4xx_i2c.h >> @@ -31,6 +31,9 @@ >> #include "hw/sysbus.h" >> #include "hw/i2c/i2c.h" >> >> +/* from hw/i2c/bitbang_i2c.h */ >> +typedef struct bitbang_i2c_interface bitbang_i2c_interface; > > This breaks compilation with clang 3.4: > > In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33: > hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef > 'bitbang_i2c_interface' > is a C11 feature [-Werror,-Wtypedef-redefinition] > typedef struct bitbang_i2c_interface bitbang_i2c_interface; > ^ > include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here > typedef struct bitbang_i2c_interface bitbang_i2c_interface; > ^ > 1 error generated. > make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1 > > Not sure about the best way to fix this ... move the typedef to > include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h" > header instead of "i2c.h" here? Not sure either but I could not include bitbang_i2c.h here because that's a private header in hw/i2c, whereas ppc4xx_i2c.h is a public header in include/hw/i2c. That's why it had to be duplicated here. I had it differently in first version but this was thought to be simpler way, see http://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00374.html Maybe we could move this typedef to include/hw/i2c/i2c.h which both ppc4xx_i2c.h and bitbang_i2c.h include? Regards, BALATON Zoltan
On 2018-11-28 12:26, BALATON Zoltan wrote: > On Wed, 28 Nov 2018, Thomas Huth wrote: >> On 2018-06-19 10:52, BALATON Zoltan wrote: >>> As well as being able to generate its own i2c transactions, the ppc4xx >>> i2c controller has a DIRECTCNTL register which allows explicit control >>> of the i2c lines. >>> >>> Using this register an OS can directly bitbang i2c operations. In >>> order to let emulated i2c devices respond to this, we need to wire up >>> the DIRECTCNTL register to qemu's bitbanged i2c handling code. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >> [...] >>> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h >>> index e4b6ded..ea6c8e1 100644 >>> --- a/include/hw/i2c/ppc4xx_i2c.h >>> +++ b/include/hw/i2c/ppc4xx_i2c.h >>> @@ -31,6 +31,9 @@ >>> #include "hw/sysbus.h" >>> #include "hw/i2c/i2c.h" >>> >>> +/* from hw/i2c/bitbang_i2c.h */ >>> +typedef struct bitbang_i2c_interface bitbang_i2c_interface; >> >> This breaks compilation with clang 3.4: >> >> In file included from /home/thuth/devel/qemu/hw/i2c/ppc4xx_i2c.c:33: >> hw/i2c/bitbang_i2c.h:6:38: error: redefinition of typedef >> 'bitbang_i2c_interface' >> is a C11 feature [-Werror,-Wtypedef-redefinition] >> typedef struct bitbang_i2c_interface bitbang_i2c_interface; >> ^ >> include/hw/i2c/ppc4xx_i2c.h:35:38: note: previous definition is here >> typedef struct bitbang_i2c_interface bitbang_i2c_interface; >> ^ >> 1 error generated. >> make[1]: *** [hw/i2c/ppc4xx_i2c.o] Error 1 >> >> Not sure about the best way to fix this ... move the typedef to >> include/qemu/typedefs.h maybe? Or include the "hw/i2c/bitbang_i2c.h" >> header instead of "i2c.h" here? > > Not sure either but I could not include bitbang_i2c.h here because > that's a private header in hw/i2c, whereas ppc4xx_i2c.h is a public > header in include/hw/i2c. That's why it had to be duplicated here. I had > it differently in first version but this was thought to be simpler way, > see http://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00374.html > > Maybe we could move this typedef to include/hw/i2c/i2c.h which both > ppc4xx_i2c.h and bitbang_i2c.h include? Fine for me, too. Do you have some spare time to create a patch? Thanks, Thomas
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak index abeeb04..851b4af 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 4e0aaae..fca80d6 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,6 +47,11 @@ #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) + static void ppc4xx_i2c_reset(DeviceState *s) { PPC4xxI2CState *i2c = PPC4xx_I2C(s); @@ -289,7 +295,12 @@ 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 & IIC_DIRECTCNTL_MSCL); + i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA, + (value & IIC_DIRECTCNTL_SDAC) != 0) << 1; break; default: if (addr < PPC4xx_I2C_MEM_SIZE) { @@ -322,6 +333,7 @@ static void ppc4xx_i2c_init(Object *o) sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq); s->bus = i2c_init_bus(DEVICE(s), "i2c"); + s->bitbang = bitbang_i2c_init(s->bus); } static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data) diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h index e4b6ded..ea6c8e1 100644 --- a/include/hw/i2c/ppc4xx_i2c.h +++ b/include/hw/i2c/ppc4xx_i2c.h @@ -31,6 +31,9 @@ #include "hw/sysbus.h" #include "hw/i2c/i2c.h" +/* from hw/i2c/bitbang_i2c.h */ +typedef struct bitbang_i2c_interface bitbang_i2c_interface; + #define TYPE_PPC4xx_I2C "ppc4xx-i2c" #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C) @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState { I2CBus *bus; qemu_irq irq; MemoryRegion iomem; + bitbang_i2c_interface *bitbang; uint8_t mdata; uint8_t lmadr; uint8_t hmadr;
As well as being able to generate its own i2c transactions, the ppc4xx i2c controller has a DIRECTCNTL register which allows explicit control of the i2c lines. Using this register an OS can directly bitbang i2c operations. In order to let emulated i2c devices respond to this, we need to wire up the DIRECTCNTL register to qemu's bitbanged i2c handling code. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v4: Updated commit message and use defined constant where appropriate default-configs/ppc-softmmu.mak | 1 + default-configs/ppcemb-softmmu.mak | 1 + hw/i2c/ppc4xx_i2c.c | 14 +++++++++++++- include/hw/i2c/ppc4xx_i2c.h | 4 ++++ 4 files changed, 19 insertions(+), 1 deletion(-)