From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC9GN-0007Fn-LW for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:29:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZC9GI-0003tq-Cf for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:28:59 -0400 Received: from greensocs.com ([193.104.36.180]:34867) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC9GI-0003tj-2n for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:28:54 -0400 Message-ID: <559AACC3.1090009@greensocs.com> Date: Mon, 06 Jul 2015 18:28:51 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1434381343-7583-1-git-send-email-fred.konrad@greensocs.com> <1434381343-7583-3-git-send-email-fred.konrad@greensocs.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 2/7] i2c: implement broadcast write. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , Mark Burton , "qemu-devel@nongnu.org Developers" , hyunk@xilinx.com, guillaume.delbergue@greensocs.com On 24/06/2015 08:35, Peter Crosthwaite wrote: > On Mon, Jun 15, 2015 at 8:15 AM, wrote: >> From: KONRAD Frederic >> >> This does a write to every slaves when the I2C bus get a write to address 0. >> >> Signed-off-by: KONRAD Frederic >> --- >> 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 >> >>