Message ID | 51f6ea83be6b66d61e3129edecddc3399a1df1b8.1528935420.git.balaton@eik.bme.hu |
---|---|
State | New |
Headers | show |
Series | Misc sam460ex improvements | expand |
On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote: > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> Patch looks good, but it needs a commit message. What is the directcntl register? Now does the bitbang interface play into that? (Note that I know the answer to those questions right now, but it needs to be in the commit message for the benefit of people looking back in the future). > --- > default-configs/ppc-softmmu.mak | 1 + > default-configs/ppcemb-softmmu.mak | 1 + > hw/i2c/ppc4xx_i2c.c | 13 ++++++++++++- > include/hw/i2c/ppc4xx_i2c.h | 4 ++++ > 4 files changed, 18 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 4e0aaae..c0a1930 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,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); > + 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 +332,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 Thu, 14 Jun 2018, David Gibson wrote: > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote: >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > Patch looks good, but it needs a commit message. What is the > directcntl register? Now does the bitbang interface play into that? > (Note that I know the answer to those questions right now, but it > needs to be in the commit message for the benefit of people looking > back in the future). If you know the answer could you please suggest an appropriate commit message? I'm a bit lost what should be written here that would explain the patch to someone who knows nothing about what this is. And for those who know, the patch itself should be fairly simple and not need more explanation. Regards, BALATON Zoltan >> --- >> default-configs/ppc-softmmu.mak | 1 + >> default-configs/ppcemb-softmmu.mak | 1 + >> hw/i2c/ppc4xx_i2c.c | 13 ++++++++++++- >> include/hw/i2c/ppc4xx_i2c.h | 4 ++++ >> 4 files changed, 18 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 4e0aaae..c0a1930 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,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); >> + 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 +332,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 Thu, Jun 14, 2018 at 09:51:33AM +0200, BALATON Zoltan wrote: > On Thu, 14 Jun 2018, David Gibson wrote: > > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote: > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > > Patch looks good, but it needs a commit message. What is the > > directcntl register? Now does the bitbang interface play into that? > > (Note that I know the answer to those questions right now, but it > > needs to be in the commit message for the benefit of people looking > > back in the future). > > If you know the answer could you please suggest an appropriate commit > message? I'm a bit lost what should be written here that would explain the > patch to someone who knows nothing about what this is. And for those who > know, the patch itself should be fairly simple and not need more > explanation. It's not really about explaining it to someone ho knows nothing about what it is. Rather it's about explaining to someone who has a general familiarity but hasn't been looking at this code recently. Try imagining explaining itself to yourself in several years time having been working on something entirely different. So, perhaps: | 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. > > Regards, > BALATON Zoltan > > > > --- > > > default-configs/ppc-softmmu.mak | 1 + > > > default-configs/ppcemb-softmmu.mak | 1 + > > > hw/i2c/ppc4xx_i2c.c | 13 ++++++++++++- > > > include/hw/i2c/ppc4xx_i2c.h | 4 ++++ > > > 4 files changed, 18 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 4e0aaae..c0a1930 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,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); > > > + 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 +332,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; > > > > >
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 4e0aaae..c0a1930 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,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); + 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 +332,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;
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 | 13 ++++++++++++- include/hw/i2c/ppc4xx_i2c.h | 4 ++++ 4 files changed, 18 insertions(+), 1 deletion(-)