[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V8 4/9] introduce aux-bus
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH V8 4/9] introduce aux-bus |
Date: |
Tue, 7 Jun 2016 13:21:47 -0700 |
On Tue, Jun 7, 2016 at 12:02 AM, KONRAD Frederic
<address@hidden> wrote:
>
>
> Le 06/06/2016 à 20:41, Alistair Francis a écrit :
>>
>> On Mon, Jun 6, 2016 at 7:21 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 | 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: 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 "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 <address@hidden>
>>
>> Thanks,
>>
>> Alistair
- [Qemu-devel] [PATCH V8 0/9] Xilinx DisplayPort., fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 1/9] i2cbus: remove unused dev field, fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 6/9] hw/i2c-ddc.c: Implement DDC I2C slave, fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 5/9] introduce dpcd module, fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 9/9] arm: xlnx-zynqmp: Add xlnx-dp and xlnx-dpdma, fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 7/9] introduce xlnx-dpdma, fred . konrad, 2016/06/06
- [Qemu-devel] [PATCH V8 8/9] introduce xlnx-dp, fred . konrad, 2016/06/06