On Mon, Jun 15, 2015 at 8:15 AM, <fred.kon...@greensocs.com> wrote: > From: KONRAD Frederic <fred.kon...@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.kon...@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: i...@greensocs.com > + * > + * Developed by : > + * Frederic Konrad <fred.kon...@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. > + > +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: i...@greensocs.com > + * > + * Developed by : > + * Frederic Konrad <fred.kon...@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 > >
