diff mbox series

[v3,2/9] ppc4xx_i2c: Implement directcntl register

Message ID 51f6ea83be6b66d61e3129edecddc3399a1df1b8.1528935420.git.balaton@eik.bme.hu
State New
Headers show
Series Misc sam460ex improvements | expand

Commit Message

BALATON Zoltan June 14, 2018, 12:17 a.m. UTC 
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(-)

Comments

David Gibson June 14, 2018, 1:17 a.m. UTC | #1 
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;
BALATON Zoltan June 14, 2018, 7:51 a.m. UTC | #2 
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;
>
>
David Gibson June 18, 2018, 12:58 a.m. UTC | #3 
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 mbox series

Patch

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;