From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC9F9-0006Hy-DE for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:27:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZC9F4-0003Mz-6g for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:27:43 -0400 Received: from greensocs.com ([193.104.36.180]:34615) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC9F3-0003Ml-K7 for qemu-devel@nongnu.org; Mon, 06 Jul 2015 12:27:37 -0400 Message-ID: <559AAC75.9020904@greensocs.com> Date: Mon, 06 Jul 2015 18:27:33 +0200 From: Frederic Konrad MIME-Version: 1.0 References: <1434381343-7583-1-git-send-email-fred.konrad@greensocs.com> <1434381343-7583-2-git-send-email-fred.konrad@greensocs.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Markus Armbruster Cc: Peter Maydell , Mark Burton , "qemu-devel@nongnu.org Developers" , hyunk@xilinx.com, guillaume.delbergue@greensocs.com On 24/06/2015 08:21, Peter Crosthwaite wrote: > On Mon, Jun 15, 2015 at 8:15 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 >> --- >> 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 >> + * >> + * 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/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 >> + * >> + * 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" >> + >> +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 >> >>