From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2Ebc-0002Pg-As for qemu-devel@nongnu.org; Fri, 27 Nov 2015 03:42:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2EbY-0002DI-7F for qemu-devel@nongnu.org; Fri, 27 Nov 2015 03:42:12 -0500 Received: from lvps176-28-13-145.dedicated.hosteurope.de ([176.28.13.145]:58830) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2EbX-0002Cx-Tw for qemu-devel@nongnu.org; Fri, 27 Nov 2015 03:42:08 -0500 From: Tim Sander Date: Fri, 27 Nov 2015 09:41:35 +0100 Message-ID: <1727507.5NJ00yyck5@dabox> In-Reply-To: <87si3sbpnc.fsf@linaro.org> References: <1664220.kcr3K9QWbf@dabox> <87si3sbpnc.fsf@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [Qemu-devel] [PATCH RFC] i2c-tiny-usb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex =?ISO-8859-1?Q?Benn=E9e?= Cc: Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann Hi Alex Thanks for your feedback, answers as usual inline. Am Donnerstag, 26. November 2015, 18:07:35 schrieb Alex Benn=E9e: > Tim Sander writes: > > Hi > >=20 > > Below is a patch implementing the i2c-tiny-usb device. > > I am currently not sure about the i2c semantics. I think > > incrementing the address on longer reads is wrong? > > But at least i can read the high byte(?) of the temperature > > via "i2cget -y 0 0x50". > >=20 > > With this device it should be possible to define i2c busses > > via command line e.g: > > -device usb-i2c-tiny,id=3Di2c-0 -device tmp105,bus=3Di2c,address=3D= 0x50 > > have been used for the first test. >=20 > You are missing a s-o-b line and scripts/checkpatch.pl complains abou= t a > few things you should fix before your next submission. I was unsure about the access length so i was just asking for feedback = this=20 time, thats why omitted the signed of by line. It seems as if i grabbed an older version of checkpatch as these errors= didn't=20 come up with the slightly older script. These new occurences will be fi= xed. > > Best regards > > Tim > > --- > >=20 > > default-configs/usb.mak | 1 + > > hw/usb/Makefile.objs | 1 + > > hw/usb/dev-i2c-tiny.c | 383 > > ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, = 385 > > insertions(+) > > create mode 100644 hw/usb/dev-i2c-tiny.c > >=20 > > diff --git a/default-configs/usb.mak b/default-configs/usb.mak > > index f4b8568..01d2c9f 100644 > > --- a/default-configs/usb.mak > > +++ b/default-configs/usb.mak > > @@ -8,3 +8,4 @@ CONFIG_USB_AUDIO=3Dy > >=20 > > CONFIG_USB_SERIAL=3Dy > > CONFIG_USB_NETWORK=3Dy > > CONFIG_USB_BLUETOOTH=3Dy > >=20 > > +CONFIG_USB_I2C_TINY=3Dy > > diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs > > index 8f00fbd..3a4c337 100644 > > --- a/hw/usb/Makefile.objs > > +++ b/hw/usb/Makefile.objs > > @@ -20,6 +20,7 @@ common-obj-$(CONFIG_USB_AUDIO) +=3D dev-au= dio.o > >=20 > > common-obj-$(CONFIG_USB_SERIAL) +=3D dev-serial.o > > common-obj-$(CONFIG_USB_NETWORK) +=3D dev-network.o > > common-obj-$(CONFIG_USB_BLUETOOTH) +=3D dev-bluetooth.o > >=20 > > +common-obj-$(CONFIG_USB_I2C_TINY) +=3D dev-i2c-tiny.o > >=20 > > ifeq ($(CONFIG_USB_SMARTCARD),y) > > common-obj-y +=3D dev-smartcard-reader.o > >=20 > > diff --git a/hw/usb/dev-i2c-tiny.c b/hw/usb/dev-i2c-tiny.c > > new file mode 100644 > > index 0000000..1dabb36 > > --- /dev/null > > +++ b/hw/usb/dev-i2c-tiny.c > > @@ -0,0 +1,383 @@ > > +/* > > + * I2C tiny usb device emulation > > + * > > + * Copyright (c) 2015 Tim Sander > > + * > > + * Loosly based on usb dev-serial.c: > > + * Copyright (c) 2006 CodeSourcery. > > + * Copyright (c) 2008 Samuel Thibault > > + * Written by Paul Brook, reused for FTDI by Samuel Thibault > > + * > > + * This code is licensed under the LGPL. > > + * > > + */ > > + > > +#include "qemu-common.h" > > +#include "qemu/error-report.h" > > +#include "hw/usb.h" > > +#include "hw/usb/desc.h" > > +#include "hw/i2c/i2c.h" > > +#include "sysemu/char.h" > > +#include "endian.h" > > + > > +/* #define DEBUG_USBI2C */ > > + > > +#ifdef DEBUG_USBI2C > > +#define DPRINTF(fmt, ...) \ > > +do { printf("usb-i2c-tiny: " fmt , ## __VA_ARGS__); } while (0) > > +#else > > +#define DPRINTF(fmt, ...) do {} while (0) > > +#endif > > + > > +/* commands from USB, must e.g. match command ids in kernel driver= */ > > +#define CMD_ECHO 0 > > +#define CMD_GET_FUNC 1 > > +#define CMD_SET_DELAY 2 > > +#define CMD_GET_STATUS 3 > > + > > +/* To determine what functionality is present */ > > +#define I2C_FUNC_I2C 0x00000001 > > +#define I2C_FUNC_10BIT_ADDR 0x00000002 > > +#define I2C_FUNC_PROTOCOL_MANGLING 0x00000004 > > +#define I2C_FUNC_SMBUS_HWPEC_CALC 0x00000008 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_READ_WORD_DATA_PEC 0x00000800 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC 0x00001000 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_PROC_CALL_PEC 0x00002000 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL_PEC 0x00004000 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL 0x00008000 /* SMBu= s 2.0 > > */ > > +#define I2C_FUNC_SMBUS_QUICK 0x00010000 > > +#define I2C_FUNC_SMBUS_READ_BYTE 0x00020000 > > +#define I2C_FUNC_SMBUS_WRITE_BYTE 0x00040000 > > +#define I2C_FUNC_SMBUS_READ_BYTE_DATA 0x00080000 > > +#define I2C_FUNC_SMBUS_WRITE_BYTE_DATA 0x00100000 > > +#define I2C_FUNC_SMBUS_READ_WORD_DATA 0x00200000 > > +#define I2C_FUNC_SMBUS_WRITE_WORD_DATA 0x00400000 > > +#define I2C_FUNC_SMBUS_PROC_CALL 0x00800000 > > +#define I2C_FUNC_SMBUS_READ_BLOCK_DATA 0x01000000 > > +#define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA 0x02000000 > > +#define I2C_FUNC_SMBUS_READ_I2C_BLOCK 0x04000000 /*I2C-l= ike > > blk-xfr */ +#define I2C_FUNC_SMBUS_WRITE_I2C_BLOCK 0x08000= 000 > > /*1-byte reg. addr.*/ +#define I2C_FUNC_SMBUS_READ_I2C_BLOCK_2 = =20 > > 0x10000000 /*I2C-like blk-xfer*/ +#define > > I2C_FUNC_SMBUS_WRITE_I2C_BLOCK_2 0x20000000 /* w/ 2-byte reg= adr*/ > > +#define I2C_FUNC_SMBUS_READ_BLOCK_DATA_PEC 0x40000000 /* SMBu= s 2.0 > > */ +#define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC 0x80000000 /* S= MBus > > 2.0 */ + > > +#define I2C_FUNC_SMBUS_BYTE (I2C_FUNC_SMBUS_READ_BYTE | \ > > + I2C_FUNC_SMBUS_WRITE_BYTE) > > +#define I2C_FUNC_SMBUS_BYTE_DATA (I2C_FUNC_SMBUS_READ_BYTE_DATA | = \ > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA) > > +#define I2C_FUNC_SMBUS_WORD_DATA (I2C_FUNC_SMBUS_READ_WORD_DATA | = \ > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA) > > +#define I2C_FUNC_SMBUS_BLOCK_DATA (I2C_FUNC_SMBUS_READ_BLOCK_DATA = | \ > > + I2C_FUNC_SMBUS_WRITE_BLOCK_DATA) > > +#define I2C_FUNC_SMBUS_I2C_BLOCK (I2C_FUNC_SMBUS_READ_I2C_BLOCK | = \ > > + I2C_FUNC_SMBUS_WRITE_I2C_BLOCK) > > + > > +#define I2C_FUNC_SMBUS_EMUL (I2C_FUNC_SMBUS_QUICK | \ > > + I2C_FUNC_SMBUS_BYTE | \ > > + I2C_FUNC_SMBUS_BYTE_DATA | \ > > + I2C_FUNC_SMBUS_WORD_DATA | \ > > + I2C_FUNC_SMBUS_PROC_CALL | \ > > + I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | \ > > + I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC | \ > > + I2C_FUNC_SMBUS_I2C_BLOCK) > > + > > +#define DeviceOutVendor > > ((USB_DIR_OUT|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8) +#define > > DeviceInVendor ((USB_DIR_IN|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8) = + > > +typedef struct { > > + USBDevice dev; > > + I2CBus *i2cbus; > > +} UsbI2cTinyState; > > + > > +#define TYPE_USB_I2C_TINY "usb-i2c-dev" > > +#define USB_I2C_TINY_DEV(obj) OBJECT_CHECK(UsbI2cTinyState, \ > > + (obj), TYPE_USB_I2C_TINY) > > + > > +enum { > > + STR_MANUFACTURER =3D 1, > > + STR_PRODUCT_SERIAL, > > + STR_SERIALNUMBER, > > +}; > > + > > +static const USBDescStrings desc_strings =3D { > > + [STR_MANUFACTURER] =3D "QEMU", > > + [STR_PRODUCT_SERIAL] =3D "QEMU USB I2C Tiny", > > + [STR_SERIALNUMBER] =3D "1", > > +}; > > + > > +static const USBDescIface desc_iface0 =3D { > > + .bInterfaceNumber =3D 1, > > + .bNumEndpoints =3D 0, > > + .bInterfaceClass =3D 0xff, > > + .bInterfaceSubClass =3D 0xff, > > + .bInterfaceProtocol =3D 0xff, > > + .eps =3D (USBDescEndpoint[]) { > > + } > > +}; > > + > > +static const USBDescDevice desc_device =3D { > > + .bcdUSB =3D 0x0110, > > + .bMaxPacketSize0 =3D 8, > > + .bNumConfigurations =3D 1, > > + .confs =3D (USBDescConfig[]) { > > + { > > + .bNumInterfaces =3D 1, > > + .bConfigurationValue =3D 1, > > + .bmAttributes =3D USB_CFG_ATT_ONE, > > + .bMaxPower =3D 50, > > + .nif =3D 1, > > + .ifs =3D &desc_iface0, > > + }, > > + }, > > +}; > > + > > +static const USBDesc desc_usb_i2c =3D { > > + .id =3D { > > + .idVendor =3D 0x0403, > > + .idProduct =3D 0xc631, > > + .bcdDevice =3D 0x0205, >=20 > Where did these magic numbers come from? I guess i have forgotton to add the link of the hardware which i am=20 aiming to emulate: http://www.harbaum.org/till/i2c_tiny_usb/index.shtml Looking at the Linux driver it seems there are two supported manufactur= er=20 numbers, i just took one of them. > > + .iManufacturer =3D STR_MANUFACTURER, > > + .iProduct =3D STR_PRODUCT_SERIAL, > > + .iSerialNumber =3D STR_SERIALNUMBER, > > + }, > > + .full =3D &desc_device, > > + .str =3D desc_strings, > > +}; > > + > > +static int usb_i2c_read_byte(I2CBus *bus, uint8_t addr) > > +{ > > + int retval; > > + i2c_start_transfer(bus, addr, 1); > > + retval =3D i2c_recv(bus); > > + i2c_end_transfer(bus); > > + return retval; > > +} > > + > > +static int usb_i2c_write_byte(I2CBus *bus, uint8_t addr, uint8_t d= ata) > > +{ > > + int retval; > > + i2c_start_transfer(bus, addr, 0); > > + retval =3D i2c_send(bus, data); > > + i2c_end_transfer(bus); > > + return retval; > > +} > > + > > +static void usb_i2c_handle_reset(USBDevice *dev) > > +{ > > + DPRINTF("reset\n"); > > +} > > + > > +static void usb_i2c_handle_control(USBDevice *dev, USBPacket *p, > > + int request, int value, int index, int length, uint= 8_t > > *data) +{ > > + UsbI2cTinyState *s =3D (UsbI2cTinyState *)dev; > > + int ret; > > + > > + DPRINTF("got control %x, value %x\n", request, value); > > + DPRINTF("iov_base:%lx pid:%x stream:%x param:%lx status:%x len= :%x\n", > > + (uint64_t)(p->iov).iov->iov_base, p->pid, p->stream, > > p->parameter, + p->status, p->actual_length); > > + ret =3D usb_desc_handle_control(dev, p, request, value, index,= length, > > data); + DPRINTF("usb_desc_handle_control return value: %i statu= s: > > %x\n", ret, + p->status); >=20 > I get that debug output is useful for debugging but if you want to > maintain ability to look at the i2c transactions you might want to > consider the tracing infrastructure. Any pointers a quick search turned up the=20 http://wiki.qemu.org/Features/Tracing/Roadmap page but this seems prett= y=20 outdated? > > + if (ret >=3D 0) { > > + return; > > + } > > + > > + switch (request) { > > + case EndpointOutRequest | USB_REQ_CLEAR_FEATURE: > > + break; > > + >=20 > Again where are these magic numbers coming from? I choose this usb device as it has a mainline driver and is reasonable = simple. These numbers are just what i have seen during reverse engineering: sta= rting=20 up the linux i2c-tiny-usb module and look for the results. So i don't k= now=20 more than what i have written into the comments. Best regards Tim