[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V5 2/8] introduce aux-bus
From: |
Frederic Konrad |
Subject: |
Re: [Qemu-devel] [PATCH V5 2/8] introduce aux-bus |
Date: |
Thu, 29 Oct 2015 16:29:16 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 18/10/2015 19:17, Peter Crosthwaite wrote:
> On Fri, Oct 16, 2015 at 6:41 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 | 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: 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)
>> +
>> +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.
True fixed.
>> + 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.
True.
>
>> + 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.
Yes that makes sense I'll try and see if I can change that.
>
>> + /*
>> + * 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: 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;
>> + /* < 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")
Ok fixed.
>> + *
>> + * @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.
Ok I dropped the second part.
Thanks,
Fred
> 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
>>
- [Qemu-devel] [PATCH V5 0/8] Xilinx DisplayPort., fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 1/8] i2cbus: remove unused dev field, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 3/8] i2c: implement broadcast write, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 4/8] introduce dpcd module, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 2/8] introduce aux-bus, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 5/8] hw/i2c-ddc.c: Implement DDC I2C slave, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 8/8] arm: xlnx-zynqmp: Add xlnx-dp and xlnx-dpdma, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 7/8] introduce xlnx-dp, fred . konrad, 2015/10/16
- [Qemu-devel] [PATCH V5 6/8] introduce xlnx-dpdma, fred . konrad, 2015/10/16
- Re: [Qemu-devel] [PATCH V5 0/8] Xilinx DisplayPort., Alistair Francis, 2015/10/16