From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47935) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnraB-0001Vk-4x for qemu-devel@nongnu.org; Sun, 18 Oct 2015 13:17:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Znra8-0005Hv-GS for qemu-devel@nongnu.org; Sun, 18 Oct 2015 13:17:19 -0400 Received: from mail-wi0-x22f.google.com ([2a00:1450:400c:c05::22f]:34434) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Znra8-0005HQ-7m for qemu-devel@nongnu.org; Sun, 18 Oct 2015 13:17:16 -0400 Received: by wikq8 with SMTP id q8so22359039wik.1 for ; Sun, 18 Oct 2015 10:17:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1445002914-7351-3-git-send-email-fred.konrad@greensocs.com> References: <1445002914-7351-1-git-send-email-fred.konrad@greensocs.com> <1445002914-7351-3-git-send-email-fred.konrad@greensocs.com> Date: Sun, 18 Oct 2015 10:17:15 -0700 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH V5 2/8] introduce aux-bus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frederic Konrad Cc: Edgar Iglesias , Peter Maydell , hyunk@xilinx.com, Mark Burton , "qemu-devel@nongnu.org Developers" , Alistair Francis , Guillaume Delbergue On Fri, Oct 16, 2015 at 6:41 AM, wrote: > From: KONRAD Frederic > > 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 > Tested-By: Hyun Kwon > --- > default-configs/aarch64-softmmu.mak | 1 + > hw/misc/Makefile.objs | 1 + > hw/misc/aux.c | 374 ++++++++++++++++++++++++++++++++++++ > include/hw/misc/aux.h | 125 ++++++++++++ > 4 files changed, 501 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 4aa76ff..e859a4b 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_AUX) += aux.o > diff --git a/hw/misc/aux.c b/hw/misc/aux.c > new file mode 100644 > index 0000000..bf300f7 > --- /dev/null > +++ b/hw/misc/aux.c > @@ -0,0 +1,374 @@ > +/* > + * aux.c > + * > + * Copyright 2015 : GreenSocs Ltd > + * http://www.greensocs.com/ , email: info@greensocs.com > + * > + * Developed by : > + * Frederic Konrad > + * > + * 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 . > + * > + */ > + > +/* > + * 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) > + > +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)); > +} > + > +/* > + * Make a native request on the AUX bus. > + */ > +static AUXReply aux_native_request(AUXBus *bus, AUXCommand cmd, > + uint32_t address, uint8_t len, > + uint8_t *data) > +{ > + /* > + * Transactions on aux address map are 1bytes len time. This doesn't parse. > + */ > + AUXReply ret = AUX_NACK; > + size_t i; > + bool is_write = false; > + > + switch (cmd) { > + 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; > + default: > + g_assert_not_reached(); > + break; > + } > + > + return ret; > +} > + > +AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address, > + uint8_t len, uint8_t *data) > +{ > + int temp; > + AUXReply ret = AUX_NACK; > + I2CBus *i2c_bus = aux_get_i2c_bus(bus); > + > + 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: > + case READ_AUX: > + ret = aux_native_request(bus, cmd, address, len, data); aux_native_request is only used once and then needs to decode cmd again. I think it can just be inlined. > + break; > + /* > + * 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; > + } > + > + ret = AUX_I2C_ACK; > + while (len > 0) { > + temp = i2c_recv(i2c_bus); > + > + if (temp < 0) { > + ret = AUX_I2C_NACK; > + break; > + } > + > + *data++ = temp; > + len--; > + } > + i2c_end_transfer(i2c_bus); > + break; > + 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; > + } > + > + ret = AUX_I2C_ACK; > + while (len > 0) { > + if (!i2c_send(i2c_bus, *data++)) { > + ret = AUX_I2C_NACK; > + i2c_end_transfer(i2c_bus); This looks un-needed, as the i2c_end_transfer will happen anyway after you break the while. > + break; > + } > + len--; > + } > + i2c_end_transfer(i2c_bus); > + break; This is repetitious between read and write. With a small patch to the I2C core, the send and recv APIs can be unified, giving something like this: case READ_I2C: is_read = 1; case WRITE_I2C: if (i2c_bus_busy(i2c_bus)) { i2c_end_transfer(i2c_bus); } if (i2c_start_transfer(i2c_bus, address, is_read)) { ret = AUX_I2C_NACK; break; } ret = AUX_I2C_ACK; while (len > 0) { if (!i2c_send_recv(i2c_bus, data++, is_read)) { ret = AUX_I2C_NACK; break; } len--; } i2c_end_transfer(i2c_bus); break; I have a draft patch for the I2C core change. > + /* > + * 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: > + 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: > + if (!i2c_bus_busy(i2c_bus)) { > + /* > + * No transactions started.. > + */ > + if (i2c_start_transfer(i2c_bus, address, 1)) { > + ret = AUX_I2C_NACK; > + break; > + } > + } else if (address != bus->last_i2c_address) { > + /* > + * Transaction started but we need to restart.. > + */ > + 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; > + i2c_end_transfer(i2c_bus); > + break; > + } > + > + *data++ = temp; > + len--; > + } > + bus->last_transaction = READ_I2C_MOT; > + bus->last_i2c_address = address; > + ret = AUX_I2C_ACK; > + break; Similar repetition here > + 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; > +}; > + > +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)); > +} > + > +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..6c88cc1 > --- /dev/null > +++ b/include/hw/misc/aux.h > @@ -0,0 +1,125 @@ > +/* > + * aux.h > + * > + * Copyright (C)2014 : GreenSocs Ltd > + * http://www.greensocs.com/ , email: info@greensocs.com > + * > + * Developed by : > + * Frederic Konrad > + * > + * 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 . > + * > + */ > + > +#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; > + /* < 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. You use all caps for AUX in camelcase, so you should probably follow the same in comments (similar to always capsing "QEMU") > + * > + * @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, must be called after > + * memory_region_init_io. Why? Also does it strictly have to be _io (e.g. could I memory_region_init_ram and map to aux? I think you can just cut the second half of the comment. Regards, Peter > + * > + * @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 >