Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus.

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

On 24/06/2015 08:21, 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 introduces a new bus: aux-bus.
>>
>> It contains an address space for aux slaves devices and a bridge to an I2C bus
>> for I2C through AUX transactions.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   hw/misc/Makefile.objs |   1 +
>>   hw/misc/aux.c         | 411 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/aux.h      | 116 ++++++++++++++
>>   3 files changed, 528 insertions(+)
>>   create mode 100644 hw/misc/aux.c
>>   create mode 100644 include/hw/aux.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 4aa76ff..11a721f 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -40,3 +40,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>>
>>   obj-$(CONFIG_PVPANIC) += pvpanic.o
>>   obj-$(CONFIG_EDU) += edu.o
>> +obj-$(CONFIG_XLNX_ZYNQMP) += aux.o
> Aux is not ZYNQ specific, it should have its own config that is just
> set by the aarch64 defconfig.
>
>> diff --git a/hw/misc/aux.c b/hw/misc/aux.c
>> new file mode 100644
>> index 0000000..b72608e
>> --- /dev/null
>> +++ b/hw/misc/aux.c
>> @@ -0,0 +1,411 @@
>> +/*
>> + * aux.c
>> + *
>> + *  Copyright 2015 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad   <fred.konrad@greensocs.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option)any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +/*
>> + * This is an implementation of the AUX bus for VESA Display Port v1.1a.
>> + */
>> +
>> +#include "hw/aux.h"
>> +#include "hw/i2c/i2c.h"
>> +#include "monitor/monitor.h"
>> +
>> +/* #define DEBUG_AUX */
> Just drop the commented out define.
>
>> +
>> +#ifdef DEBUG_AUX
>> +#define DPRINTF(fmt, ...)\
>> +do { printf("aux: " fmt , ## __VA_ARGS__); } while (0)
> Use a regular if for conditional debug prinfery.
>
> Also do not use printf, use qemu_log.
>
>> +#else
>> +#define DPRINTF(fmt, ...)do {} while (0)
>> +#endif
>> +
>> +#define TYPE_AUXTOI2C "aux-to-i2c-bridge"
>> +#define AUXTOI2C(obj) OBJECT_CHECK(AUXTOI2CState, (obj), TYPE_AUXTOI2C)
>> +
>> +typedef struct AUXTOI2CState AUXTOI2CState;
>> +
>> +struct AUXBus {
> /*< private >*/
>
>> +    BusState qbus;
> /*< public > */
>
>> +    AUXSlave *current_dev;
>> +    AUXSlave *dev;
>> +    uint32_t last_i2c_address;
>> +    aux_command last_transaction;
>> +
>> +    AUXTOI2CState *bridge;
>> +
>> +    MemoryRegion *aux_io;
>> +    AddressSpace aux_addr_space;
>> +};
> Modern QOM conventions require the state struct to be in a header.
> This allows for embedding the device its containers.
>
>> +
>> +static Property aux_props[] = {
>> +    DEFINE_PROP_UINT64("address", struct AUXSlave, address, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +#define TYPE_AUX_BUS "aux-bus"
>> +#define AUX_BUS(obj) OBJECT_CHECK(AUXBus, (obj), TYPE_AUX_BUS)
>> +
>> +static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent);
>> +
>> +static void aux_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> +    /*
>> +     * AUXSlave has an mmio so we need to change the way we print information
> MMIO
>
>> +     * in monitor.
>> +     */
> Can you move the comment below the declaration?
>
>> +    BusClass *k = BUS_CLASS(klass);
> blank line.
>
>> +    k->print_dev = aux_slave_dev_print;
>> +}
>> +
>> +static const TypeInfo aux_bus_info = {
>> +    .name = TYPE_AUX_BUS,
>> +    .parent = TYPE_BUS,
>> +    .instance_size = sizeof(AUXBus),
>> +    .class_init = aux_bus_class_init
>> +};
>> +
>> +AUXBus *aux_init_bus(DeviceState *parent, const char *name)
>> +{
>> +    AUXBus *bus;
>> +
>> +    bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name));
>> +
>> +    /*
>> +     * Create the bridge.
>> +     */
> Code is self documenting, comment unneeded.
>
>> +    bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C));
>> +
>> +    /*
>> +     * Memory related.
>> +     */
> Make a one-line comment.
>
>> +    bus->aux_io = g_malloc(sizeof(*bus->aux_io));
>> +    memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", (1 << 20));
>> +    address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
>> +    return bus;
>> +}
>> +
>> +static void aux_bus_map_device(AUXBus *bus, AUXSlave *dev)
>> +{
>> +    memory_region_add_subregion(bus->aux_io, dev->address, dev->mmio);
>> +}
>> +
>> +void aux_set_slave_address(AUXSlave *dev, uint32_t address)
>> +{
>> +    qdev_prop_set_uint64(DEVICE(dev), "address", address);
>> +}
>> +
> Do these two need to be separate? Can you just pass the address to
> aux_bus_map_device and remove the .address field from the bus?
>
>> +static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev)
>> +{
>> +    return (dev == DEVICE(bus->bridge));
>> +}
>> +
>> +/*
>> + * Make a native request on the AUX bus.
>> + */
>> +static aux_reply aux_native_request(AUXBus *bus, aux_command cmd,
>> +                                    uint32_t address, uint8_t len,
>> +                                    uint8_t *data)
>> +{
>> +    /*
>> +     * Transactions on aux address map are 1bytes len time.
>> +     */
>> +    aux_reply ret = AUX_NACK;
>> +    size_t i;
>> +
>> +    switch (cmd) {
>> +    case READ_AUX:
>> +        for (i = 0; i < len; i++) {
>> +            if (!address_space_rw(&bus->aux_addr_space, address++,
>> +                                  MEMTXATTRS_UNSPECIFIED, data++, 1, false)) {
> address_space_read. Although ...
>
>> +                ret = AUX_I2C_ACK;
>> +            } else {
>> +                ret = AUX_NACK;
>> +                break;
>> +            }
>> +        }
>> +    break;
>> +    case WRITE_AUX:
> You can remove the code duplication with:
>
> switch(cmd) {
> case (WRITE_AUX):
>      is_write = true;
>      /* fallthrough */
> case (READ_AUX):
>      for(...) {
>          address_space_rw(..., is_write);
>
>> +        for (i = 0; i < len; i++) {
>> +            if (!address_space_rw(&bus->aux_addr_space, address++,
>> +                                  MEMTXATTRS_UNSPECIFIED, data++, 1, true)) {
>> +                ret = AUX_I2C_ACK;
>> +            } else {
>> +                ret = AUX_NACK;
>> +                break;
>> +            }
>> +        }
>> +    break;
>> +    default:
>> +        abort();
> g_assert_not_reached
>
>> +    break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +aux_reply aux_request(AUXBus *bus, aux_command cmd, uint32_t address,
>> +                      uint8_t len, uint8_t *data)
>> +{
>> +    DPRINTF("request at address 0x%5.5X, command %u, len %u\n", address, cmd,
>> +            len);
> PRIx32
>
>> +
>> +    int temp;
>> +    aux_reply ret = AUX_NACK;
>> +    I2CBus *i2c_bus = aux_get_i2c_bus(bus);
>> +
> The DRPINTF before the declarations is a C99 mixed code and decls
> which is discouraged. Do the DPRINTF after the decls.
>
>> +    switch (cmd) {
>> +    /*
>> +     * Forward the request on the AUX bus..
>> +     */
>> +    case WRITE_AUX:
>> +    case READ_AUX:
>> +        ret = aux_native_request(bus, cmd, address, len, data);
>> +    break;
> indentation.
>
>> +    /*
>> +     * Classic I2C transactions..
>> +     */
>> +    case READ_I2C:
>> +        if (i2c_bus_busy(i2c_bus)) {
>> +            i2c_end_transfer(i2c_bus);
>> +        }
>> +
>> +        if (i2c_start_transfer(i2c_bus, address, 1)) {
>> +            ret = AUX_I2C_NACK;
>> +            break;
>> +        }
>> +
>> +        while (len > 0) {
>> +            temp = i2c_recv(i2c_bus);
>> +
>> +            if (temp < 0) {
>> +                ret = AUX_I2C_NACK;
> This nack ...
>
>> +                i2c_end_transfer(i2c_bus);
>> +                break;
>> +            }
>> +
>> +            *data++ = temp;
>> +            len--;
>> +        }
>> +        i2c_end_transfer(i2c_bus);
>> +        ret = AUX_I2C_ACK;
> ... will get overridden by this ack.
>
>> +    break;
> Indentation.
>
>> +    case WRITE_I2C:
>> +        if (i2c_bus_busy(i2c_bus)) {
>> +            i2c_end_transfer(i2c_bus);
>> +        }
>> +
>> +        if (i2c_start_transfer(i2c_bus, address, 0)) {
>> +            ret = AUX_I2C_NACK;
>> +            break;
>> +        }
>> +
>> +        while (len > 0) {
>> +            if (!i2c_send(i2c_bus, *data++)) {
>> +                ret = AUX_I2C_NACK;
>> +                i2c_end_transfer(i2c_bus);
>> +                break;
>> +            }
>> +            len--;
>> +        }
>> +        i2c_end_transfer(i2c_bus);
>> +        ret = AUX_I2C_ACK;
> same. You might be needing a goto from those in-the-loop nacks, but
> the shortest way I can think of is:
>
> ret = AUX_I2C_ACK;
> while (...) {
>      if (!i2c_send) {
>          ret = NACK;
>          break;
>      }
>      len--;
> }
> i2c_end_transfer(...).
>
>> +    break;
>> +    /*
>> +     * I2C MOT transactions.
>> +     *
>> +     * Here we send a start when:
>> +     *  - We didn't start transaction yet.
>> +     *  - We had a READ and we do a WRITE.
>> +     *  - We change the address.
> "changed"
>
>> +     */
>> +    case WRITE_I2C_MOT:
>> +        if (!i2c_bus_busy(i2c_bus)) {
>> +            /*
>> +             * No transactions started..
>> +             */
>> +            if (i2c_start_transfer(i2c_bus, address, 0)) {
>> +                ret = AUX_I2C_NACK;
>> +                break;
>> +            }
>> +        } else if ((address != bus->last_i2c_address) ||
>> +                   (bus->last_transaction == READ_I2C_MOT)) {
>> +            /*
>> +             * Transaction started but we need to restart..
>> +             */
>> +            i2c_end_transfer(i2c_bus);
>> +            if (i2c_start_transfer(i2c_bus, address, 0)) {
>> +                ret = AUX_I2C_NACK;
>> +                break;
>> +            }
>> +        }
>> +
>> +        while (len > 0) {
>> +            if (!i2c_send(i2c_bus, *data++)) {
>> +                ret = AUX_I2C_NACK;
>> +                i2c_end_transfer(i2c_bus);
>> +                break;
>> +            }
>> +            len--;
>> +        }
>> +        bus->last_transaction = WRITE_I2C_MOT;
>> +        bus->last_i2c_address = address;
>> +        ret = AUX_I2C_ACK;
>> +    break;
>> +    case READ_I2C_MOT:
> This read vs write code is very similar from one to the other. It can
> be factored out as such:
>
> case WRITE_I2C_MOT:
>      is_write = true;
>      /*fallthrough */
> case READ_I2C_MOT:
>
>
>> +        if (!i2c_bus_busy(i2c_bus)) {
>> +            /*
>> +             * No transactions started..
>> +             */
>> +            if (i2c_start_transfer(i2c_bus, address, 0)) {
>> +                ret = AUX_I2C_NACK;
>> +                break;
>> +            }
>> +        } else if (address != bus->last_i2c_address) {
> The restart condition here is different to write. Mainly, you do not
> restart on a change from write to read. Perhaps worth a comment, or
> list-comment the read restart conditions like you did for write.
>
>> +            /*
>> +             * Transaction started but we need to restart..
>> +             */
>> +            i2c_end_transfer(i2c_bus);
>> +            if (i2c_start_transfer(i2c_bus, address, 0)) {
>> +                ret = AUX_I2C_NACK;
>> +                break;
>> +            }
>> +        }
>> +
>> +        while (len > 0) {
> if (is_write) {
>      i2c_err = i2c_send(...) ? - 1 : 0;
> } else {
>> +            temp = i2c_recv(i2c_bus);
>      i2c_err = temp < 0;
> }
>
>> +
>> +            if (temp < 0) {
>> +                ret = AUX_I2C_NACK;
>> +                i2c_end_transfer(i2c_bus);
>> +                break;
>> +            }
>> +
> if (is_write) {
>> +            *data++ = temp;
> }
>
>> +            len--;
>> +        }
>> +        bus->last_transaction = READ_I2C_MOT;
>> +        bus->last_i2c_address = address;
>> +        ret = AUX_I2C_ACK;
>> +    break;
>> +    default:
>> +        DPRINTF("Not implemented!\n");
>> +        ret = AUX_NACK;
>> +    break;
>> +    }
>> +
>> +    DPRINTF("reply: %u\n", ret);
>> +    return ret;
>> +}
>> +
>> +/*
>> + * AUX to I2C bridge.
>> + */
>> +struct AUXTOI2CState {
> /*< private >*/
>
>> +    DeviceState parent_obj;
> /*< public >*/
>
>> +    I2CBus *i2c_bus;
>> +};
> Will need to move to the header.

This AUXTOI2C is only used here in aux.c.. I don't think it should be 
public?
>
>> +
>> +I2CBus *aux_get_i2c_bus(AUXBus *bus)
>> +{
>> +    return bus->bridge->i2c_bus;
>> +}
>> +
>> +static void aux_bridge_init(Object *obj)
>> +{
>> +    AUXTOI2CState *s = AUXTOI2C(obj);
>> +    /*
>> +     * Create the I2C Bus.
>> +     */
> self documenting.
>
>> +    s->i2c_bus = i2c_init_bus(DEVICE(obj), "aux-i2c");
>> +}
>> +
>> +static const TypeInfo aux_to_i2c_type_info = {
>> +    .name = TYPE_AUXTOI2C,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(AUXTOI2CState),
>> +    .instance_init = aux_bridge_init
>> +};
>> +
>> +/*
>> + * AUX Slave.
>> + */
>> +static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent)
>> +{
>> +    AUXBus *bus = AUX_BUS(qdev_get_parent_bus(dev));
>> +    hwaddr size;
>> +    AUXSlave *s;
>> +
>> +    /*
>> +     * Don't print anything if the device is I2C "bridge".
>> +     */
>> +    if (aux_bus_is_bridge(bus, dev)) {
>> +        return;
>> +    }
>> +
>> +    s = AUX_SLAVE(dev);
>> +
>> +    size = memory_region_size(s->mmio);
>> +    monitor_printf(mon, "%*smemory " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
>> +                   indent, "", s->address, size);
>> +}
>> +
>> +DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create(&bus->qbus, name);
>> +    qdev_prop_set_uint64(dev, "address", addr);
>> +    qdev_init_nofail(dev);
>> +    aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev));
>> +    return dev;
>> +}
> qdev_create helpers are depracated. The code should just be inlined
> into the creating machine models or container devs.
>
>> +
>> +void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio)
>> +{
>> +    aux_slave->mmio = mmio;
> Should this assert on repeated calls?
>
>> +}
>> +
>> +static void aux_slave_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *k = DEVICE_CLASS(klass);
>> +    set_bit(DEVICE_CATEGORY_MISC, k->categories);
>> +    k->bus_type = TYPE_AUX_BUS;
>> +    k->props = aux_props;
>> +}
>> +
>> +static const TypeInfo aux_slave_type_info = {
>> +    .name = TYPE_AUX_SLAVE,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(AUXSlave),
>> +    .abstract = true,
>> +    .class_init = aux_slave_class_init,
>> +};
>> +
>> +static void aux_slave_register_types(void)
>> +{
>> +    type_register_static(&aux_bus_info);
>> +    type_register_static(&aux_slave_type_info);
>> +    type_register_static(&aux_to_i2c_type_info);
>> +}
>> +
>> +type_init(aux_slave_register_types)
>> diff --git a/include/hw/aux.h b/include/hw/aux.h
>> new file mode 100644
>> index 0000000..7b29ee1
>> --- /dev/null
>> +++ b/include/hw/aux.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * aux.h
>> + *
>> + *  Copyright (C)2014 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad   <fred.konrad@greensocs.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option)any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#ifndef QEMU_AUX_H
>> +#define QEMU_AUX_H
>> +
>> +#include "hw/qdev.h"
>> +
>> +enum aux_command {
> AUXCommand.
>
>> +    WRITE_I2C = 0,
>> +    READ_I2C = 1,
>> +    WRITE_I2C_STATUS = 2,
>> +    WRITE_I2C_MOT = 4,
>> +    READ_I2C_MOT = 5,
>> +    WRITE_AUX = 8,
>> +    READ_AUX = 9
>> +};
>> +
>> +enum aux_reply {
> AUXReply.
>
>> +    AUX_I2C_ACK = 0,
>> +    AUX_NACK = 1,
>> +    AUX_DEFER = 2,
>> +    AUX_I2C_NACK = 4,
>> +    AUX_I2C_DEFER = 8
>> +};
>> +
>> +typedef struct AUXBus AUXBus;
>> +typedef struct AUXSlave AUXSlave;
>> +typedef enum aux_command aux_command;
>> +typedef enum aux_reply aux_reply;
>> +
>> +#define TYPE_AUX_SLAVE "aux-slave"
>> +#define AUX_SLAVE(obj) \
>> +     OBJECT_CHECK(AUXSlave, (obj), TYPE_AUX_SLAVE)
>> +
>> +struct AUXSlave {
>> +    /* < private > */
>> +    DeviceState parent_obj;
>> +
> /*< public >*/
>
>> +    /* address of the device on the aux bus. */
>> +    hwaddr address;
> Can this be encapsulated by mmio. There is memory_region_get_addr().
>
>> +    /* memory region associated. */
>> +    MemoryRegion *mmio;
>> +};
>> +
>> +/*
>> + * \func aux_init_bus
>> + * \brief Init an aux bus.
>> + * \param parent The device where this bus is located.
>> + * \param name The name of the bus.
>> + * \return The new aux bus.
> Please use the /** @ style documentation.
>
> Regards,
> Peter
>
>> + */
>> +AUXBus *aux_init_bus(DeviceState *parent, const char *name);
>> +
>> +/*
>> + * \func aux_slave_set_address
>> + * \brief Set the address of the slave on the aux bus.
>> + * \param dev The aux slave device.
>> + * \param address The address to give to the slave.
>> + */
>> +void aux_set_slave_address(AUXSlave *dev, uint32_t address);
>> +
>> +/*
>> + * \func aux_request
>> + * \brief Make a request on the bus.
>> + * \param bus Ths bus where the request happen.
>> + * \param cmd The command requested.
>> + * \param address The 20bits address of the slave.
>> + * \param len The length of the read or write.
>> + * \param data The data array which will be filled or read during transfer.
>> + * \return Return the reply of the request.
>> + */
>> +aux_reply aux_request(AUXBus *bus, aux_command cmd, uint32_t address,
>> +                              uint8_t len, uint8_t *data);
>> +
>> +/*
>> + * \func aux_get_i2c_bus
>> + * \brief Get the i2c bus for I2C over AUX command.
>> + * \param bus The aux bus.
>> + * \return Return the i2c bus associated.
>> + */
>> +I2CBus *aux_get_i2c_bus(AUXBus *bus);
>> +
>> +/*
>> + * \func aux_init_mmio
>> + * \brief Init an mmio for an aux slave, must be called after
>> + *        memory_region_init_io.
>> + * \param aux_slave The aux slave.
>> + * \param mmio The mmio to be registered.
>> + */
>> +void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio);
>> +
>> +DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr);
>> +
>> +#endif /* !QEMU_AUX_H */
>> --
>> 1.9.0
>>
>>