Re: [Qemu-devel] [PATCH V2 2/7] i2c: implement broadcast write.

[Frederic Konrad <fred.konrad@greensocs.com>]

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
>>
>>