[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus.
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus. |
Date: |
Tue, 23 Jun 2015 23:21:10 -0700 |
On Mon, Jun 15, 2015 at 8:15 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>
> ---
> 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: 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/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: 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"
> +
> +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
>
>
- [Qemu-devel] [PATCH V2 0/7] Xilinx DisplayPort., fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 3/7] introduce dpcd module., fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus., fred . konrad, 2015/06/15
- Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus.,
Peter Crosthwaite <=
- [Qemu-devel] [PATCH V2 7/7] arm: xlnx-zynqmp: Add DisplayPort and DPDMA., fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 2/7] i2c: implement broadcast write., fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 4/7] hw/i2c-ddc.c: Implement DDC I2C slave, fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 5/7] Introduce xilinx dpdma., fred . konrad, 2015/06/15
- [Qemu-devel] [PATCH V2 6/7] Introduce xilinx dp., fred . konrad, 2015/06/15