diff mbox

[V2,2/7] i2c: implement broadcast write.

Message ID 1434381343-7583-3-git-send-email-fred.konrad@greensocs.com
State New
Headers show

Commit Message

fred.konrad@greensocs.com June 15, 2015, 3:15 p.m. UTC 
From: KONRAD Frederic <fred.konrad@greensocs.com>

This does a write to every slaves when the I2C bus get a write to address 0.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/i2c/core.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

Comments

Peter Crosthwaite June 24, 2015, 6:35 a.m. UTC | #1 
On Mon, Jun 15, 2015 at 8:15 AM,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This does a write to every slaves when the I2C bus get a write to address 0.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/i2c/core.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 5a64026..db1cbdd 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -15,6 +15,7 @@ struct I2CBus
>      I2CSlave *current_dev;
>      I2CSlave *dev;
>      uint8_t saved_address;
> +    bool broadcast;
>  };
>
>  static Property i2c_props[] = {
> @@ -67,6 +68,8 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
>
>      bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
>      vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
> +
> +    bus->broadcast = false;

0 initialiser should not be needed for new QOM object.

>      return bus;
>  }
>
> @@ -89,6 +92,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
>      I2CSlave *slave = NULL;
>      I2CSlaveClass *sc;
>
> +    if (address == 0x00) {
> +        /*
> +         * This is a broadcast.
> +         */

One line comment.

> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            bus->broadcast = true;

Move outside loop.

> +            if (sc->event) {
> +                sc->event(dev, recv ? I2C_START_RECV : I2C_START_SEND);
> +            }
> +        }
> +        return 0;
> +    }
> +
>      QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>          DeviceState *qdev = kid->child;
>          I2CSlave *candidate = I2C_SLAVE(qdev);
> @@ -114,9 +132,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
>
>  void i2c_end_transfer(I2CBus *bus)
>  {
> +    BusChild *kid;
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
>
> +    if (bus->broadcast) {
> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            if (sc->event) {
> +                sc->event(dev, I2C_FINISH);
> +            }
> +        }
> +        bus->broadcast = false;
> +    }
> +
>      if (!dev) {
>          return;
>      }
> @@ -131,8 +161,22 @@ void i2c_end_transfer(I2CBus *bus)
>
>  int i2c_send(I2CBus *bus, uint8_t data)
>  {
> +    BusChild *kid;
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
> +    int ret = 0;
> +
> +    if (bus->broadcast) {
> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +            I2CSlave *dev = I2C_SLAVE(kid->child);
> +            sc = I2C_SLAVE_GET_CLASS(dev);
> +            bus->broadcast = true;
> +            if (sc->send) {
> +                ret |= sc->send(dev, data);
> +            }
> +        }

Still not sure about the duped core functionality of each of these
APIs. That is, the same code is needed in both a looped form and a 1
form. Can this be solved by listifying current_dev? That is, ->current
dev is turned into a list which in the normal case will be populated
with 1 element by start_transfer() for the current dev. In the
broadcast case, all qbus.children are added to the list. The broadcast
bool is then removed. start() send() and end_transfer() then just loop
through the list unconditionally.

Regards,
Peter

> +        return ret;
> +    }
>
>      if (!dev) {
>          return -1;
> @@ -151,7 +195,7 @@ int i2c_recv(I2CBus *bus)
>      I2CSlave *dev = bus->current_dev;
>      I2CSlaveClass *sc;
>
> -    if (!dev) {
> +    if ((!dev) || (bus->broadcast)) {
>          return -1;
>      }
>
> --
> 1.9.0
>
>
fred.konrad@greensocs.com July 6, 2015, 4:28 p.m. UTC | #2 
On 24/06/2015 08:35, Peter Crosthwaite wrote:
> On Mon, Jun 15, 2015 at 8:15 AM,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This does a write to every slaves when the I2C bus get a write to address 0.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   hw/i2c/core.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 5a64026..db1cbdd 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -15,6 +15,7 @@ struct I2CBus
>>       I2CSlave *current_dev;
>>       I2CSlave *dev;
>>       uint8_t saved_address;
>> +    bool broadcast;
>>   };
>>
>>   static Property i2c_props[] = {
>> @@ -67,6 +68,8 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
>>
>>       bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
>>       vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
>> +
>> +    bus->broadcast = false;
> 0 initialiser should not be needed for new QOM object.
>
>>       return bus;
>>   }
>>
>> @@ -89,6 +92,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
>>       I2CSlave *slave = NULL;
>>       I2CSlaveClass *sc;
>>
>> +    if (address == 0x00) {
>> +        /*
>> +         * This is a broadcast.
>> +         */
> One line comment.
>
>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>> +            bus->broadcast = true;
> Move outside loop.
>
>> +            if (sc->event) {
>> +                sc->event(dev, recv ? I2C_START_RECV : I2C_START_SEND);
>> +            }
>> +        }
>> +        return 0;
>> +    }
>> +
>>       QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>           DeviceState *qdev = kid->child;
>>           I2CSlave *candidate = I2C_SLAVE(qdev);
>> @@ -114,9 +132,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
>>
>>   void i2c_end_transfer(I2CBus *bus)
>>   {
>> +    BusChild *kid;
>>       I2CSlave *dev = bus->current_dev;
>>       I2CSlaveClass *sc;
>>
>> +    if (bus->broadcast) {
>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>> +            if (sc->event) {
>> +                sc->event(dev, I2C_FINISH);
>> +            }
>> +        }
>> +        bus->broadcast = false;
>> +    }
>> +
>>       if (!dev) {
>>           return;
>>       }
>> @@ -131,8 +161,22 @@ void i2c_end_transfer(I2CBus *bus)
>>
>>   int i2c_send(I2CBus *bus, uint8_t data)
>>   {
>> +    BusChild *kid;
>>       I2CSlave *dev = bus->current_dev;
>>       I2CSlaveClass *sc;
>> +    int ret = 0;
>> +
>> +    if (bus->broadcast) {
>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>> +            bus->broadcast = true;
>> +            if (sc->send) {
>> +                ret |= sc->send(dev, data);
>> +            }
>> +        }
> Still not sure about the duped core functionality of each of these
> APIs. That is, the same code is needed in both a looped form and a 1
> form. Can this be solved by listifying current_dev? That is, ->current
> dev is turned into a list which in the normal case will be populated
> with 1 element by start_transfer() for the current dev. In the
> broadcast case, all qbus.children are added to the list. The broadcast
> bool is then removed. start() send() and end_transfer() then just loop
> through the list unconditionally.

I think better keeping this broadcast as we need it for VMSD anyway?

> Regards,
> Peter
>
>> +        return ret;
>> +    }
>>
>>       if (!dev) {
>>           return -1;
>> @@ -151,7 +195,7 @@ int i2c_recv(I2CBus *bus)
>>       I2CSlave *dev = bus->current_dev;
>>       I2CSlaveClass *sc;
>>
>> -    if (!dev) {
>> +    if ((!dev) || (bus->broadcast)) {
>>           return -1;
>>       }
>>
>> --
>> 1.9.0
>>
>>
Peter Crosthwaite July 6, 2015, 4:54 p.m. UTC | #3 
On Mon, Jul 6, 2015 at 9:28 AM, Frederic Konrad
<fred.konrad@greensocs.com> wrote:
> On 24/06/2015 08:35, Peter Crosthwaite wrote:
>>
>> On Mon, Jun 15, 2015 at 8:15 AM,  <fred.konrad@greensocs.com> wrote:
>>>
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> This does a write to every slaves when the I2C bus get a write to address
>>> 0.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>   hw/i2c/core.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>>> index 5a64026..db1cbdd 100644
>>> --- a/hw/i2c/core.c
>>> +++ b/hw/i2c/core.c
>>> @@ -15,6 +15,7 @@ struct I2CBus
>>>       I2CSlave *current_dev;
>>>       I2CSlave *dev;
>>>       uint8_t saved_address;
>>> +    bool broadcast;
>>>   };
>>>
>>>   static Property i2c_props[] = {
>>> @@ -67,6 +68,8 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char
>>> *name)
>>>
>>>       bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
>>>       vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
>>> +
>>> +    bus->broadcast = false;
>>
>> 0 initialiser should not be needed for new QOM object.
>>
>>>       return bus;
>>>   }
>>>
>>> @@ -89,6 +92,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address,
>>> int recv)
>>>       I2CSlave *slave = NULL;
>>>       I2CSlaveClass *sc;
>>>
>>> +    if (address == 0x00) {
>>> +        /*
>>> +         * This is a broadcast.
>>> +         */
>>
>> One line comment.
>>
>>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>>> +            bus->broadcast = true;
>>
>> Move outside loop.
>>
>>> +            if (sc->event) {
>>> +                sc->event(dev, recv ? I2C_START_RECV : I2C_START_SEND);
>>> +            }
>>> +        }
>>> +        return 0;
>>> +    }
>>> +
>>>       QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>>           DeviceState *qdev = kid->child;
>>>           I2CSlave *candidate = I2C_SLAVE(qdev);
>>> @@ -114,9 +132,21 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address,
>>> int recv)
>>>
>>>   void i2c_end_transfer(I2CBus *bus)
>>>   {
>>> +    BusChild *kid;
>>>       I2CSlave *dev = bus->current_dev;
>>>       I2CSlaveClass *sc;
>>>
>>> +    if (bus->broadcast) {
>>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>>> +            if (sc->event) {
>>> +                sc->event(dev, I2C_FINISH);
>>> +            }
>>> +        }
>>> +        bus->broadcast = false;
>>> +    }
>>> +
>>>       if (!dev) {
>>>           return;
>>>       }
>>> @@ -131,8 +161,22 @@ void i2c_end_transfer(I2CBus *bus)
>>>
>>>   int i2c_send(I2CBus *bus, uint8_t data)
>>>   {
>>> +    BusChild *kid;
>>>       I2CSlave *dev = bus->current_dev;
>>>       I2CSlaveClass *sc;
>>> +    int ret = 0;
>>> +
>>> +    if (bus->broadcast) {
>>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>> +            I2CSlave *dev = I2C_SLAVE(kid->child);
>>> +            sc = I2C_SLAVE_GET_CLASS(dev);
>>> +            bus->broadcast = true;
>>> +            if (sc->send) {
>>> +                ret |= sc->send(dev, data);
>>> +            }
>>> +        }
>>
>> Still not sure about the duped core functionality of each of these
>> APIs. That is, the same code is needed in both a looped form and a 1
>> form. Can this be solved by listifying current_dev? That is, ->current
>> dev is turned into a list which in the normal case will be populated
>> with 1 element by start_transfer() for the current dev. In the
>> broadcast case, all qbus.children are added to the list. The broadcast
>> bool is then removed. start() send() and end_transfer() then just loop
>> through the list unconditionally.
>
>
> I think better keeping this broadcast as we need it for VMSD anyway?
>

Ok I see the VMSD issue and can keep the boolean, but I'm more
concerned about the duped code. It's hard to patch this reliably, if
the same core of code is duped.

Regards,
Peter

>
>> Regards,
>> Peter
>>
>>> +        return ret;
>>> +    }
>>>
>>>       if (!dev) {
>>>           return -1;
>>> @@ -151,7 +195,7 @@ int i2c_recv(I2CBus *bus)
>>>       I2CSlave *dev = bus->current_dev;
>>>       I2CSlaveClass *sc;
>>>
>>> -    if (!dev) {
>>> +    if ((!dev) || (bus->broadcast)) {
>>>           return -1;
>>>       }
>>>
>>> --
>>> 1.9.0
>>>
>>>
>
>
diff mbox

Patch

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 5a64026..db1cbdd 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -15,6 +15,7 @@  struct I2CBus
     I2CSlave *current_dev;
     I2CSlave *dev;
     uint8_t saved_address;
+    bool broadcast;
 };
 
 static Property i2c_props[] = {
@@ -67,6 +68,8 @@  I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
 
     bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
     vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
+
+    bus->broadcast = false;
     return bus;
 }
 
@@ -89,6 +92,21 @@  int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
     I2CSlave *slave = NULL;
     I2CSlaveClass *sc;
 
+    if (address == 0x00) {
+        /*
+         * This is a broadcast.
+         */
+        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+            I2CSlave *dev = I2C_SLAVE(kid->child);
+            sc = I2C_SLAVE_GET_CLASS(dev);
+            bus->broadcast = true;
+            if (sc->event) {
+                sc->event(dev, recv ? I2C_START_RECV : I2C_START_SEND);
+            }
+        }
+        return 0;
+    }
+
     QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         I2CSlave *candidate = I2C_SLAVE(qdev);
@@ -114,9 +132,21 @@  int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 
 void i2c_end_transfer(I2CBus *bus)
 {
+    BusChild *kid;
     I2CSlave *dev = bus->current_dev;
     I2CSlaveClass *sc;
 
+    if (bus->broadcast) {
+        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+            I2CSlave *dev = I2C_SLAVE(kid->child);
+            sc = I2C_SLAVE_GET_CLASS(dev);
+            if (sc->event) {
+                sc->event(dev, I2C_FINISH);
+            }
+        }
+        bus->broadcast = false;
+    }
+
     if (!dev) {
         return;
     }
@@ -131,8 +161,22 @@  void i2c_end_transfer(I2CBus *bus)
 
 int i2c_send(I2CBus *bus, uint8_t data)
 {
+    BusChild *kid;
     I2CSlave *dev = bus->current_dev;
     I2CSlaveClass *sc;
+    int ret = 0;
+
+    if (bus->broadcast) {
+        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+            I2CSlave *dev = I2C_SLAVE(kid->child);
+            sc = I2C_SLAVE_GET_CLASS(dev);
+            bus->broadcast = true;
+            if (sc->send) {
+                ret |= sc->send(dev, data);
+            }
+        }
+        return ret;
+    }
 
     if (!dev) {
         return -1;
@@ -151,7 +195,7 @@  int i2c_recv(I2CBus *bus)
     I2CSlave *dev = bus->current_dev;
     I2CSlaveClass *sc;
 
-    if (!dev) {
+    if ((!dev) || (bus->broadcast)) {
         return -1;
     }