Re: [Qemu-devel] [PATCH V7 4/9] introduce aux-bus

[Alistair Francis]

On Mon, Feb 1, 2016 at 7:43 AM,  <address@hidden> wrote:
> From: KONRAD Frederic <address@hidden>
>
> 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 <address@hidden>
> Tested-By: Hyun Kwon <address@hidden>
> ---
>  default-configs/aarch64-softmmu.mak |   1 +
>  hw/misc/Makefile.objs               |   1 +
>  hw/misc/aux.c                       | 291 
> ++++++++++++++++++++++++++++++++++++
>  include/hw/misc/aux.h               | 124 +++++++++++++++
>  4 files changed, 417 insertions(+)
>  create mode 100644 hw/misc/aux.c
>  create mode 100644 include/hw/misc/aux.h
>
> diff --git a/default-configs/aarch64-softmmu.mak 
> b/default-configs/aarch64-softmmu.mak
> index 96dd994..d3a2665 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -3,4 +3,5 @@
>  # We support all the 32 bit boards so need all their config
>  include arm-softmmu.mak
>
> +CONFIG_AUX=y
>  CONFIG_XLNX_ZYNQMP=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index d4765c2..4af5fbe 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -44,3 +44,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>  obj-$(CONFIG_EDU) += edu.o
>  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
> +obj-$(CONFIG_AUX) += aux.o
> diff --git a/hw/misc/aux.c b/hw/misc/aux.c
> new file mode 100644
> index 0000000..1412ce0
> --- /dev/null
> +++ b/hw/misc/aux.c
> @@ -0,0 +1,291 @@
> +/*
> + * aux.c
> + *
> + *  Copyright 2015 : GreenSocs Ltd
> + *      http://www.greensocs.com/ , email: address@hidden
> + *
> + *  Developed by :
> + *  Frederic Konrad   <address@hidden>
> + *
> + * 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/misc/aux.h"
> +#include "hw/i2c/i2c.h"
> +#include "monitor/monitor.h"
> +
> +#ifndef DEBUG_AUX
> +#define DEBUG_AUX 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) do {                                               
>   \
> +    if (DEBUG_AUX) {                                                         
>   \
> +        qemu_log("aux: " fmt , ## __VA_ARGS__);                              
>   \
> +    }                                                                        
>   \
> +} while (0);
> +
> +#define TYPE_AUXTOI2C "aux-to-i2c-bridge"
> +#define AUXTOI2C(obj) OBJECT_CHECK(AUXTOI2CState, (obj), TYPE_AUXTOI2C)
> +
> +#define TYPE_AUX_BUS "aux-bus"
> +#define AUX_BUS(obj) OBJECT_CHECK(AUXBus, (obj), TYPE_AUX_BUS)

I this it's worth adding a comment here that these are 'internal'
devices and are not publicly exposed.

> +
> +static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent);
> +
> +static void aux_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +
> +    /* AUXSlave has an MMIO so we need to change the way we print information
> +     * in monitor.
> +     */
> +    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));
> +    bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C));
> +
> +    /* Memory related. */
> +    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, hwaddr addr)
> +{
> +    memory_region_add_subregion(bus->aux_io, addr, dev->mmio);
> +}
> +
> +static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev)
> +{
> +    return (dev == DEVICE(bus->bridge));
> +}
> +
> +AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> +                      uint8_t len, uint8_t *data)
> +{
> +    AUXReply ret = AUX_NACK;
> +    I2CBus *i2c_bus = aux_get_i2c_bus(bus);
> +    size_t i;
> +    bool is_write = false;
> +
> +    DPRINTF("request at address 0x%" PRIX32 ", command %u, len %u\n", 
> address,
> +            cmd, len);
> +
> +    switch (cmd) {
> +    /*
> +     * Forward the request on the AUX bus..
> +     */
> +    case WRITE_AUX:
> +        is_write = true;
> +        /* fallthrough */
> +    case READ_AUX:
> +        for (i = 0; i < len; i++) {
> +            if (!address_space_rw(&bus->aux_addr_space, address++,
> +                                  MEMTXATTRS_UNSPECIFIED, data++, 1,
> +                                  is_write)) {
> +                ret = AUX_I2C_ACK;
> +            } else {
> +                ret = AUX_NACK;
> +                break;
> +            }
> +        }
> +        break;
> +    /*
> +     * Classic I2C transactions..
> +     */
> +    case READ_I2C:
> +    case WRITE_I2C:
> +        is_write = cmd == READ_I2C ? false : true;
> +        if (i2c_bus_busy(i2c_bus)) {
> +            i2c_end_transfer(i2c_bus);
> +        }
> +
> +        if (i2c_start_transfer(i2c_bus, address, is_write)) {
> +            ret = AUX_I2C_NACK;
> +            break;
> +        }
> +
> +        ret = AUX_I2C_ACK;
> +        while (len > 0) {
> +            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
> +                ret = AUX_I2C_NACK;
> +                break;
> +            }
> +            len--;
> +        }
> +        i2c_end_transfer(i2c_bus);
> +        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 changed the address.
> +     */
> +    case WRITE_I2C_MOT:
> +    case READ_I2C_MOT:
> +        is_write = cmd == READ_I2C_MOT ? false : true;
> +        if (!i2c_bus_busy(i2c_bus)) {
> +            /*
> +             * No transactions started..
> +             */
> +            if (i2c_start_transfer(i2c_bus, address, is_write)) {
> +                ret = AUX_I2C_NACK;
> +                break;
> +            }
> +        } else if ((address != bus->last_i2c_address) ||
> +                   (bus->last_transaction != cmd)) {
> +            /*
> +             * Transaction started but we need to restart..
> +             */
> +            i2c_end_transfer(i2c_bus);
> +            if (i2c_start_transfer(i2c_bus, address, is_write)) {
> +                ret = AUX_I2C_NACK;
> +                break;
> +            }
> +        }
> +
> +        while (len > 0) {
> +            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
> +                ret = AUX_I2C_NACK;
> +                i2c_end_transfer(i2c_bus);
> +                break;
> +            }
> +            len--;
> +        }
> +        bus->last_transaction = cmd;
> +        bus->last_i2c_address = address;
> +        ret = AUX_I2C_ACK;
> +        break;
> +    default:
> +        DPRINTF("Not implemented!\n");
> +        ret = AUX_NACK;
> +        break;
> +    }

This is going to fall through to say Not implemented and reply.
Instead of a break here it should be a return.

> +
> +    DPRINTF("reply: %u\n", ret);
> +    return ret;
> +}
> +
> +/*
> + * AUX to I2C bridge.
> + */
> +struct AUXTOI2CState {
> +    /*< private >*/
> +    DeviceState parent_obj;

Newline

> +    /*< public >*/
> +    I2CBus *i2c_bus;
> +};
> +
> +I2CBus *aux_get_i2c_bus(AUXBus *bus)
> +{
> +    return bus->bridge->i2c_bus;
> +}
> +
> +static void aux_bridge_init(Object *obj)
> +{
> +    AUXTOI2CState *s = AUXTOI2C(obj);
> +
> +    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));
> +    AUXSlave *s;
> +
> +    /* Don't print anything if the device is I2C "bridge". */
> +    if (aux_bus_is_bridge(bus, dev)) {
> +        return;
> +    }
> +
> +    s = AUX_SLAVE(dev);
> +
> +    monitor_printf(mon, "%*smemory " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
> +                   indent, "",
> +                   object_property_get_int(OBJECT(s->mmio), "addr", NULL),
> +                   memory_region_size(s->mmio));
> +}

This function can be moved further up in the file right?

Can you sort the file so that the AUX bus, AUX slave and AUX to I2C
devices are as separate as possible?

> +
> +DeviceState *aux_create_slave(AUXBus *bus, const char *type, uint32_t addr)
> +{
> +    DeviceState *dev;
> +
> +    dev = DEVICE(object_new(type));
> +    assert(dev);
> +    qdev_set_parent_bus(dev, &bus->qbus);
> +    qdev_init_nofail(dev);
> +    aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev), 
> addr);
> +    return dev;
> +}
> +
> +void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio)
> +{
> +    assert(!aux_slave->mmio);
> +    aux_slave->mmio = mmio;
> +}
> +
> +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;
> +}
> +
> +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/misc/aux.h b/include/hw/misc/aux.h
> new file mode 100644
> index 0000000..5d3e7a5
> --- /dev/null
> +++ b/include/hw/misc/aux.h
> @@ -0,0 +1,124 @@
> +/*
> + * aux.h
> + *
> + *  Copyright (C)2014 : GreenSocs Ltd
> + *      http://www.greensocs.com/ , email: address@hidden
> + *
> + *  Developed by :
> + *  Frederic Konrad   <address@hidden>
> + *
> + * 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"
> +
> +typedef struct AUXBus AUXBus;
> +typedef struct AUXSlave AUXSlave;
> +typedef enum AUXCommand AUXCommand;
> +typedef enum AUXReply AUXReply;
> +typedef struct AUXTOI2CState AUXTOI2CState;
> +
> +enum 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 AUXReply {
> +    AUX_I2C_ACK = 0,
> +    AUX_NACK = 1,
> +    AUX_DEFER = 2,
> +    AUX_I2C_NACK = 4,
> +    AUX_I2C_DEFER = 8
> +};
> +
> +struct AUXBus {
> +    /* < private > */
> +    BusState qbus;

Newline

Thanks,

Alistair

> +    /* < public > */
> +    AUXSlave *current_dev;
> +    AUXSlave *dev;
> +    uint32_t last_i2c_address;
> +    AUXCommand last_transaction;
> +
> +    AUXTOI2CState *bridge;
> +
> +    MemoryRegion *aux_io;
> +    AddressSpace aux_addr_space;
> +};
> +
> +#define TYPE_AUX_SLAVE "aux-slave"
> +#define AUX_SLAVE(obj) \
> +     OBJECT_CHECK(AUXSlave, (obj), TYPE_AUX_SLAVE)
> +
> +struct AUXSlave {
> +    /* < private > */
> +    DeviceState parent_obj;
> +
> +    /* < public > */
> +    MemoryRegion *mmio;
> +};
> +
> +/**
> + * aux_init_bus: Initialize an AUX bus.
> + *
> + * Returns the new AUX bus created.
> + *
> + * @parent The device where this bus is located.
> + * @name The name of the bus.
> + */
> +AUXBus *aux_init_bus(DeviceState *parent, const char *name);
> +
> +/*
> + * aux_request: Make a request on the bus.
> + *
> + * Returns the reply of the request.
> + *
> + * @bus Ths bus where the request happen.
> + * @cmd The command requested.
> + * @address The 20bits address of the slave.
> + * @len The length of the read or write.
> + * @data The data array which will be filled or read during transfer.
> + */
> +AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
> +                              uint8_t len, uint8_t *data);
> +
> +/*
> + * aux_get_i2c_bus: Get the i2c bus for I2C over AUX command.
> + *
> + * Returns the i2c bus associated to this AUX bus.
> + *
> + * @bus The AUX bus.
> + */
> +I2CBus *aux_get_i2c_bus(AUXBus *bus);
> +
> +/*
> + * aux_init_mmio: Init an mmio for an AUX slave.
> + *
> + * @aux_slave The AUX slave.
> + * @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
>
>