On Jun 24, 2015 11:22 AM, <
address@hidden> wrote:
Send Qemu-devel mailing list submissions to
address@hidden
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.nongnu.org/mailman/listinfo/qemu-devel
or, via email, send a message with subject or body 'help' to
address@hidden
You can reach the person managing the list at
address@hidden
When replying, please edit your Subject line so it is more specific
than "Re: Contents of Qemu-devel digest..."
Today's Topics:
1. Re: [PATCH v2 0/5] Add feature to start QEMU without
vhost-user backend (Michael S. Tsirkin)
2. Re: [PATCH v7 3/4] i.MX: Add i.MX25 3DS evaluation board
support (Jean-Christophe DUBOIS)
3. Re: [PATCH V2 1/7] Introduce AUX bus. (Peter Crosthwaite)
----------------------------------------------------------------------
Message: 1
Date: Wed, 24 Jun 2015 07:57:45 +0200
From: "Michael S. Tsirkin" <address@hidden>
To: Tetsuya Mukawa <address@hidden>
Cc: address@hidden, address@hidden,
address@hidden, address@hidden
Subject: Re: [Qemu-devel] [PATCH v2 0/5] Add feature to start QEMU
without vhost-user backend
Message-ID: <address@hidden>
Content-Type: text/plain; charset=us-ascii
On Wed, Jun 24, 2015 at 02:46:30PM +0900, Tetsuya Mukawa wrote:
> On 2015/06/23 18:41, Michael S. Tsirkin wrote:
> > On Tue, Jun 23, 2015 at 05:31:06PM +0900, Tetsuya Mukawa wrote:
> >> On 2015/06/22 17:14, Michael S. Tsirkin wrote:
> >>> On Mon, Jun 22, 2015 at 12:50:43PM +0900, Tetsuya Mukawa wrote:
> >>>> Hi guys,
> >>>>
> >>>> Here are patches to add feature to start QEMU without vhost-user backend.
> >>>> Currently, if we want to use vhost-user backend, the backend must start before
> >>>> QEMU.
> >>> Aren't you looking for something like xinetd?
> >> It will help for restricting starting order, but not help for
> >> reconnection of vhost-user backend.
> > At this point, I suggest that we always connect at vm start.
> > With that, you can add an option to reset the VM
> > on backend disconnect.
> > So
> > - detect backend disconnect
> > - reset and stop (notifying management)
> > - reconnect or detect backend reconnect
> > - proceed with boot
> >
> > As I tried to explain below, getting the full functionality
> > will require guest driver changes. They aren't hard to get
> > done, patches welcome.
> >
>
> Could you please let me know your thinking about using
> DEVICE_NEEDS_RESET for vhost-user reconnection?
> If it's works, I will try to submit it.
DEVICE_NEEDS_RESET is hard to handle correctly in guest:
you need to reconfigure a bunch of state,
so far no one wrote the necessary support.
> >
> >>>> Also, if QEMU or the backend is closed unexpectedly, there is no way to
> >>>> recover without restarting both applications.
> >>> This was previously discussed:
> >>> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg00585.html
> >>> It doesn't look like any of the issues have been addressed.
> >>>
> >>> In my opinion, the right way to handle this requires
> >>> guest cooperation. For example, we can extend virtio 1 to allow a
> >>> single queue to reset.
> >>>
> >>> We then can do something like the following:
> >>> - hypervisor detects queue backend disconnect.
> >>> - hypervisor sets flag and notifies guest
> >>> - guest resets queue, optionally discarding queued packets
> >>> - hypervisor detects (or requests if it's a client), backend reconnect
> >>> - hypervisor detects guest queue reset
> >>> - hypervisor notifies guests about backend reconnect
> >>> - hypervisor sends hypervisor device state (e.g. queue addresses and
> >>> indices) to backend
> >>>
> >>> Reconnect isn't robust without such an extension.
> >> I appreciate your all comments. It's truly helpful.
> >> Do you have any plans to support such queue reset features in virtio spec?
> > Maybe, but I don't have the time to work on this :(
> > You are welcome to contribute this.
>
> To do that, I need to talk around my boss to join OASIS :'(
That's not true at all. Non members can submit feedback.
You do need permission from your employer, but
no need to join and pay fees if you don't want to.
See
https://www.oasis-open.org/committees/comments/index.php?wg_abbrev=virtio
for the details.
> Probably it's not so easy, so I will just wait, if we need to change
> spec itself.
The QEMU patch that allows handling backend disconnect similar to
disk errors can be implemented without guest/spec changes
let me know if you need more explanation.
> >> Also, I haven't known following behavior.
> >>
> >>> some guests crash if we never complete handling buffers,
> >>> or networking breaks, etc ...
> >> Does it mean that device needs to return buffers through used ring as
> >> soon as possible?
> >> If so, when backend switch is under heavy load, some guests could be
> >> crashed?
> > Yes. Some guests crash when disk times out, too.
>
> It's bad news. I haven't assumed the case while writing patches.
>
> >>>> Practically, it's not useful.
> >>> One thing we do seem to lack is specifying an action
> >>> taken on disconnect. It could be an action similar to r/werror.
> >>> One extra useful option could be to exit qemu.
> >> My implementation treats disconnection as link down.
> >> And when backend is connected, link status will be changed.
> >> So here is behavior of my patch.
> >>
> >> - hypervisor detects queue backend disconnect.
> >> - hypervisor sets link status down and notifies guest
> >> - optionally, guest stops sending and receiving packets
> >> => As you mentioned above, this step may crash some guests.
> > It's not just that.
> > Under the virtio protocol, you can not, just by looking at the ring,
> > detect which buffers have been used.
> > Imagine:
> >
> > - guest adds buffer A
> > - guest adds buffer B * 10000 - backend uses buffer B * 10000
> >
> > buffer A is nowhere in the available ring since it has
> > since wrapped around many times.
> > So backend that crashes and re-connects simply can not recover
> > and complete buffer A.
> >
>
> Could I make sure that I understand the issue correctly?
> I guess that an one of cause of the above issue is that the guest
> doesn't check used_ring before filling avail ring to make sure that
> buffers overwritten by new filling buffers have already returned through
> used_ring.
> If the guest checks used_ring, probably performance will be poor, but
> there are no buffer loss.
>
> Also I guess the above issue might be happen if backend is too slow,
> because the guest will overwrite avail_ring without checking.
> Is this correct?
No - it happens with a correct guest too, because buffers
can be consumed out of order.
> >
> >
> >
> >> - hypervisor detects (or requests if it's a client), backend reconnect
> >> - hypervisor make sure backend supports features that the former backend
> >> supports.
> >> - hypervisor sends hypervisor device state (e.g. queue addresses and
> >> indices) to backend
> >> - backend reads virtqueues, and recover correct indices.
> >> => Some backends needs to have this feature to reconnect correctly.
> >> - hypervisor notifies guests about backend reconnect as link status up
> >>
> >>>> This patch series adds following features.
> >>>> - QEMU can start before the backend.
> >>> I don't see the point of this one.
> >> Sorry for my bad English. Here is correct.
> >> - Guest can start before the backend.
> >>
> >> Currently, guest will not start without vhost-user backend application
> >> when vhost-user is specified.
> > E.g. with control VQ (which vhost user doesn't handle, but really
> > should, and I think it will in the future), guest will loop waiting for
> > the command to complete.
>
> I agree.
> I guess control queue should work fine even when link status is down,
> but my patches doesn't care about it.
>
> >>>> - QEMU or the backend can restart anytime.
> >>>> connectivity will be recovered automatically, when app starts again.
> >>>> (if QEMU is server, QEMU just wait reconnection)
> >>>> while lost connection, link status of virtio-net device is down,
> >>>> so virtio-net driver on the guest can know it
> >>>>
> >>>> To work like above, the patch introduces flags to specify features vhost-user
> >>>> backend will support.
> >>>>
> >>>> Here are examples.
> >>>> ('backend_features' is the new flags. Each bit of the flag represents
> >>>> VIRTIO_NET_F_* in linux/virtio_net.h)
> >>>>
> >>>> * QEMU is configured as vhost-user client.
> >>>> -chardev socket,id=chr0,path=/tmp/sock,reconnect=3 \
> >>>> -netdev vhost-user,id=net0,chardev=chr0,vhostforce,backend-features=0x68000 \
> >>>> -device virtio-net-pci,netdev=net0 \
> >>>>
> >>>> * QEMU is configured as vhost-user server.
> >>>> -chardev socket,id=chr0,path=/tmp/sock,server,nowait \
> >>>> -netdev vhost-user,id=net0,chardev=chr0,vhostforce,backend-features=0x68000 \
> >>>> -device virtio-net-pci,netdev=net0 \
> >>> This relies on management knowing the feature bitmap encoding of the
> >>> backend, not sure how practical that is.
> >> If we can assume which vhost-user backend application is used, we can
> >> specify backend-features value like above.
> >> Probably such an assumption is not so strange in practice.
> >> And above option allow us to start VM before starting backend application.
> >>
> >> I guess it's useful, but if you don't think, I could remove this feature.
> > To make it useful, management needs a way to find out what are the
> > values. We don't want it to deal with the virtio spec, do we?
>
> I agree.
>
> >
> >> In the case, still VM needs to start before backend.
> >> And when backend is connected, virtio-net device will keep backend
> >> features for future reconnection.
> >> (When backend reconnects again, device will make sure that backend
> >> supports features that the former backend supports.)
> >>
> >> Regards,
> >> Tetsuya
> > I think that
> > - reconnect requires guest support. let's add this to virtio
> > - when not connected, the cleanest thing is to block VM
> > options to exit and reset might also make sense
> > - if you want to start guest before backend, I think
> > we are looking at a spec extension, to tell guest
> > not to access device since backend is not ready.
> > we also need to solve two problems
> > - how does management find out features supported by qemu?
> > - how does management find out features supported by backend?
>
> Thanks for clarifying. I understand the correct steps.
>
> Regards,
> Tetsuya
--
MST
------------------------------
Message: 2
Date: Wed, 24 Jun 2015 08:09:41 +0200
From: Jean-Christophe DUBOIS <address@hidden>
To: Peter Crosthwaite <address@hidden>, Peter Maydell
<address@hidden>, Andreas F?rber <address@hidden>
Cc: "address@hidden Developers" <address@hidden>
Subject: Re: [Qemu-devel] [PATCH v7 3/4] i.MX: Add i.MX25 3DS
evaluation board support
Message-ID: <address@hidden>
Content-Type: text/plain; charset=utf-8; format=flowed
Le 24/06/2015 07:07, Peter Crosthwaite a ?crit :
> On Mon, Jun 22, 2015 at 12:06 PM, Jean-Christophe Dubois
> <address@hidden> wrote:
>> For now we support:
>> * timers (GPT and EPIT)
>> * serial ports
>> * ethernet (through the newly added FEC emulator)
>> * I2C (through the newly added I2C emulator)
>>
>> Signed-off-by: Jean-Christophe Dubois <address@hidden>
>> ---
>>
>> Changes since v1:
>> * Added a ds1338 I2C device for qtest purpose.
>>
>> Changes since v2:
>> * none
>>
>> Changes since v3:
>> * Rework GPL header
>> * use I2C constructor helper.
>>
>> Changes since v4:
>> * use sysbus_create_simple() instead of I2C constructor helper
>>
>> Changes since v5:
>> * Add ds1338 only for qtest mode.
>> * small comment fixes.
>>
>> Changes since v6:
>> * Allow for more than 4 serial if suppoted by Qemu.
>>
>> hw/arm/Makefile.objs | 1 +
>> hw/arm/imx25_3ds.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++
> So the (new since v6) guideline with ARM SoCs is to split the SoC and
> machine models. This would give you two files. A SoC file for imx25
> and board for 3ds.
>
> I'm not 100% on the "3ds" board following some googling but is it
> really the PDK board? I found this:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=635baf6b40ebaef683abf87d677467cba71a0d50
>
> and can find product info for PDK from there:
>
> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=IMX25PDK
Yes this is the board.
>
> To see modern examples of split SoC/board, see stm32f205_soc wiith
> netduino2 or xlnx-zynqmp with xlnx-ep108 (all in hw/arm).
I'll look at it.
>
> It seems the only board level feature you are implementing is this
> ds1338 which is not or the real board. So I guess qtest-only code can
> be left in the SoC model.
OK
>
> This would make your board level very similar to netduino2 (perhaps
> even slightly simpler).
Yes, for now there is basically only the SOC supported in my case.
>
> Regards,
> Peter
Thanks for the feedback.
JC
------------------------------
Message: 3
Date: Tue, 23 Jun 2015 23:21:10 -0700
From: Peter Crosthwaite <address@hidden>
To: Fr?deric Konrad <address@hidden>, Markus Armbruster
<address@hidden>
Cc: Peter Maydell <address@hidden>, Mark Burton
<address@hidden>, "address@hidden Developers"
<address@hidden>, address@hidden,
address@hidden
Subject: Re: [Qemu-devel] [PATCH V2 1/7] Introduce AUX bus.
Message-ID:
<CAEgOgz66j=-address@hidden>
Content-Type: text/plain; charset=UTF-8
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 mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/qemu-devel
End of Qemu-devel Digest, Vol 147, Issue 1155
*********************************************