On Tue, Jun 7, 2016 at 12:02 AM, KONRAD Frederic <fred.kon...@greensocs.com> wrote: > > > Le 06/06/2016 à 20:41, Alistair Francis a écrit : >> >> On Mon, Jun 6, 2016 at 7:21 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> >>> Tested-By: Hyun Kwon <hyun.k...@xilinx.com> >>> --- >>> default-configs/aarch64-softmmu.mak | 1 + >>> hw/misc/Makefile.objs | 1 + >>> hw/misc/aux.c | 297 >>> ++++++++++++++++++++++++++++++++++++ >>> include/hw/misc/aux.h | 125 +++++++++++++++ >>> 4 files changed, 424 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 bc0dd2c..ffb49c1 100644 >>> --- a/hw/misc/Makefile.objs >>> +++ b/hw/misc/Makefile.objs >>> @@ -51,3 +51,4 @@ obj-$(CONFIG_MIPS_ITU) += mips_itu.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..6605224 >>> --- /dev/null >>> +++ b/hw/misc/aux.c >>> @@ -0,0 +1,297 @@ >>> +/* >>> + * 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 "qemu/osdep.h" >>> +#include "qemu/log.h" >>> +#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) >> >> >> This should be in the header file where the struct is. > > > Ok > >> >>> + >>> +static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int >>> indent); >>> +static inline I2CBus *aux_bridge_get_i2c_bus(AUXTOI2CState *bridge); >>> + >>> +/* aux-bus implementation (internal not public) */ >>> +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; >>> +} >>> + >>> +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)); >>> +} >>> + >>> +I2CBus *aux_get_i2c_bus(AUXBus *bus) >>> +{ >>> + return aux_bridge_get_i2c_bus(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 */ >> >> >> Can you use the conditional setting like you do with the others? > > yes sorry I missed this one. > > >> >>> + 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 should be a return, not a break. Otherwise you are printing the >> reply line as well. > > Yes, I don't think it is an issue as I return AUX_NACK?
I don't there is any problem functionality wise. I was just thinking that the print statements might be a bit confusing. On second thought though it isn't a big deal. You can leave it as is, it's your call. Thanks, Alistair > > Fred > > >> >> Looks good otherwise, if you fix the comments I made: >> >> Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com> >> >> Thanks, >> >> Alistair
