Patchwork versatilepb: add i2c support

login
register
mail settings
Submitter Oskar Andero
Date April 2, 2012, 8:17 p.m.
Message ID <1333397837-5389-1-git-send-email-oskar.andero@gmail.com>
Download mbox | patch
Permalink /patch/150352/
State New
Headers show

Comments

Oskar Andero - April 2, 2012, 8:17 p.m.
Signed-off-by: Oskar Andero <oskar.andero@gmail.com>
---
 hw/versatilepb.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
Andreas Färber - April 3, 2012, 9:13 a.m.
Am 02.04.2012 22:17, schrieb Oskar Andero:
> Signed-off-by: Oskar Andero <oskar.andero@gmail.com>
> ---
>  hw/versatilepb.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 25afb1e..014621a 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -16,9 +16,18 @@
>  #include "boards.h"
>  #include "blockdev.h"
>  #include "exec-memory.h"
> +#include "bitbang_i2c.h"
>  
>  /* Primary interrupt controller.  */
>  
> +typedef struct {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    bitbang_i2c_interface *bitbang;
> +    int out;
> +    int in;
> +} vpb_i2c_state;

Please use Camel Case for new types, e.g., VPBI2CState.

> +
>  typedef struct vpb_sic_state
>  {
>    SysBusDevice busdev;
[...]
> @@ -380,9 +452,17 @@ static TypeInfo vpb_sic_info = {
>      .class_init    = vpb_sic_class_init,
>  };
>  
> +static TypeInfo vpb_i2c_info = {

static const, please, since you don't modify it.

> +    .name          = "vpb_i2c",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(vpb_i2c_state),
> +    .class_init    = vpb_i2c_class_init,
> +};
> +
>  static void versatilepb_register_types(void)
>  {
>      type_register_static(&vpb_sic_info);
> +    type_register_static(&vpb_i2c_info);
>  }
>  
>  type_init(versatilepb_register_types)

Otherwise looks okay technically; don't know about I2C personally.

Andreas
Peter Maydell - April 5, 2012, 5:45 p.m.
On 2 April 2012 21:17, Oskar Andero <oskar.andero@gmail.com> wrote:
> Signed-off-by: Oskar Andero <oskar.andero@gmail.com>
> ---
>  hw/versatilepb.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 80 insertions(+), 0 deletions(-)

This is just a cut-n-paste of the i2c device in hw/realview.c.
Instead of duplicating the code we should pull it out into
its own source file, give it a name which doesn't have 'realview'
in it (since it's the same thing in all these boards) and
then use that new device in all the boards which have it.

-- PMM
Oskar Andero - April 9, 2012, 7:56 p.m.
On 18:45 Thu 05 Apr     , Peter Maydell wrote:
> On 2 April 2012 21:17, Oskar Andero <oskar.andero@gmail.com> wrote:
> > Signed-off-by: Oskar Andero <oskar.andero@gmail.com>
> > ---
> >  hw/versatilepb.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 80 insertions(+), 0 deletions(-)
> 
> This is just a cut-n-paste of the i2c device in hw/realview.c.
> Instead of duplicating the code we should pull it out into
> its own source file, give it a name which doesn't have 'realview'
> in it (since it's the same thing in all these boards) and
> then use that new device in all the boards which have it.

Yes, you are absolutely right. I will create a new file, e.g.
i2c_generic.c or something.

Thanks,
 Oskar
Peter A. G. Crosthwaite - April 9, 2012, 11:56 p.m.
On Tue, Apr 10, 2012 at 5:56 AM, Oskar Andero <oskar.andero@gmail.com> wrote:
> On 18:45 Thu 05 Apr     , Peter Maydell wrote:
>> On 2 April 2012 21:17, Oskar Andero <oskar.andero@gmail.com> wrote:
>> > Signed-off-by: Oskar Andero <oskar.andero@gmail.com>
>> > ---
>> >  hw/versatilepb.c |   80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 files changed, 80 insertions(+), 0 deletions(-)
>>
>> This is just a cut-n-paste of the i2c device in hw/realview.c.
>> Instead of duplicating the code we should pull it out into
>> its own source file, give it a name which doesn't have 'realview'
>> in it (since it's the same thing in all these boards) and
>> then use that new device in all the boards which have it.
>
> Yes, you are absolutely right. I will create a new file, e.g.
> i2c_generic.c or something.
>

Does this I2C controller IP common to all these arm platforms have a
name? i2c_generic.c sounds too generic a name for a specific
implementation of I2C. Just browsing the linux source, the driver is
called i2c-versatile, perhaps this should be the name of the device
and this new file?

Peter

Patch

diff --git a/hw/versatilepb.c b/hw/versatilepb.c
index 25afb1e..014621a 100644
--- a/hw/versatilepb.c
+++ b/hw/versatilepb.c
@@ -16,9 +16,18 @@ 
 #include "boards.h"
 #include "blockdev.h"
 #include "exec-memory.h"
+#include "bitbang_i2c.h"
 
 /* Primary interrupt controller.  */
 
+typedef struct {
+    SysBusDevice busdev;
+    MemoryRegion iomem;
+    bitbang_i2c_interface *bitbang;
+    int out;
+    int in;
+} vpb_i2c_state;
+
 typedef struct vpb_sic_state
 {
   SysBusDevice busdev;
@@ -153,6 +162,64 @@  static int vpb_sic_init(SysBusDevice *dev)
     return 0;
 }
 
+static uint64_t vpb_i2c_read(void *opaque, target_phys_addr_t offset,
+                             unsigned size)
+{
+    vpb_i2c_state *s = (vpb_i2c_state *)opaque;
+
+    if (offset == 0) {
+        return (s->out & 1) | (s->in << 1);
+    } else {
+        hw_error("%s: Bad offset 0x%x\n", __func__, (int)offset);
+        return -1;
+    }
+}
+
+static void vpb_i2c_write(void *opaque, target_phys_addr_t offset,
+                          uint64_t value, unsigned size)
+{
+    vpb_i2c_state *s = (vpb_i2c_state *)opaque;
+
+    switch (offset) {
+    case 0:
+        s->out |= value & 3;
+        break;
+    case 4:
+        s->out &= ~value;
+        break;
+    default:
+        hw_error("%s: Bad offset 0x%x\n", __func__, (int)offset);
+    }
+    bitbang_i2c_set(s->bitbang, BITBANG_I2C_SCL, (s->out & 1) != 0);
+    s->in = bitbang_i2c_set(s->bitbang, BITBANG_I2C_SDA, (s->out & 2) != 0);
+}
+
+static const MemoryRegionOps vpb_i2c_ops = {
+    .read = vpb_i2c_read,
+    .write = vpb_i2c_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static int vpb_i2c_init(SysBusDevice *dev)
+{
+    vpb_i2c_state *s = FROM_SYSBUS(vpb_i2c_state, dev);
+    i2c_bus *bus;
+
+    bus = i2c_init_bus(&dev->qdev, "i2c");
+    s->bitbang = bitbang_i2c_init(bus);
+    memory_region_init_io(&s->iomem, &vpb_i2c_ops, s,
+                          "vpb-i2c", 0x1000);
+    sysbus_init_mmio(dev, &s->iomem);
+    return 0;
+}
+
+static void vpb_i2c_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = vpb_i2c_init;
+}
+
 /* Board init.  */
 
 /* The AB and PB boards both use the same core, just with different
@@ -177,6 +244,7 @@  static void versatile_init(ram_addr_t ram_size,
     SysBusDevice *busdev;
     DeviceState *pl041;
     PCIBus *pci_bus;
+    i2c_bus *i2c;
     NICInfo *nd;
     int n;
     int done_smc = 0;
@@ -268,6 +336,10 @@  static void versatile_init(ram_addr_t ram_size,
     /* Add PL031 Real Time Clock. */
     sysbus_create_simple("pl031", 0x101e8000, pic[10]);
 
+    dev = sysbus_create_simple("vpb_i2c", 0x10002000, NULL);
+    i2c = (i2c_bus *)qdev_get_child_bus(dev, "i2c");
+    i2c_create_slave(i2c, "ds1338", 0x68);
+
     /* Add PL041 AACI Interface to the LM4549 codec */
     pl041 = qdev_create(NULL, "pl041");
     qdev_prop_set_uint32(pl041, "nc_fifo_depth", 512);
@@ -380,9 +452,17 @@  static TypeInfo vpb_sic_info = {
     .class_init    = vpb_sic_class_init,
 };
 
+static TypeInfo vpb_i2c_info = {
+    .name          = "vpb_i2c",
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(vpb_i2c_state),
+    .class_init    = vpb_i2c_class_init,
+};
+
 static void versatilepb_register_types(void)
 {
     type_register_static(&vpb_sic_info);
+    type_register_static(&vpb_i2c_info);
 }
 
 type_init(versatilepb_register_types)