Re: [Qemu-devel] [PATCH RFC] i2c-tiny-usb

[Tim Sander <tim@krieglstein.org>]

Hi Alex

Thanks for your feedback, answers as usual inline.

Am Donnerstag, 26. November 2015, 18:07:35 schrieb Alex Bennée:
> Tim Sander <tim@krieglstein.org> writes:
> > Hi
> > 
> > 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".
> > 
> > With this device it should be possible to define i2c busses
> > via command line e.g:
> > -device usb-i2c-tiny,id=i2c-0 -device tmp105,bus=i2c,address=0x50
> > have been used for the first test.
> 
> You are missing a s-o-b line and scripts/checkpatch.pl complains about 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 
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 
come up with the slightly older script. These new occurences will be fixed.
> > Best regards
> > Tim
> > ---
> > 
> >  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
> > 
> > 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=y
> > 
> >  CONFIG_USB_SERIAL=y
> >  CONFIG_USB_NETWORK=y
> >  CONFIG_USB_BLUETOOTH=y
> > 
> > +CONFIG_USB_I2C_TINY=y
> > 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)        += dev-audio.o
> > 
> >  common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
> >  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
> >  common-obj-$(CONFIG_USB_BLUETOOTH)    += dev-bluetooth.o
> > 
> > +common-obj-$(CONFIG_USB_I2C_TINY)     += dev-i2c-tiny.o
> > 
> >  ifeq ($(CONFIG_USB_SMARTCARD),y)
> >  common-obj-y                          += dev-smartcard-reader.o
> > 
> > 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 <tim@krieglstein.org>
> > + *
> > + * Loosly based on usb dev-serial.c:
> > + * Copyright (c) 2006 CodeSourcery.
> > + * Copyright (c) 2008 Samuel Thibault <samuel.thibault@ens-lyon.org>
> > + * 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 /* SMBus 2.0
> > */
> > +#define I2C_FUNC_SMBUS_READ_WORD_DATA_PEC       0x00000800 /* SMBus 2.0
> > */
> > +#define I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC      0x00001000 /* SMBus 2.0
> > */
> > +#define I2C_FUNC_SMBUS_PROC_CALL_PEC            0x00002000 /* SMBus 2.0
> > */
> > +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL_PEC      0x00004000 /* SMBus 2.0
> > */
> > +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL          0x00008000 /* SMBus 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-like
> > blk-xfr */ +#define I2C_FUNC_SMBUS_WRITE_I2C_BLOCK          0x08000000
> > /*1-byte reg. addr.*/ +#define I2C_FUNC_SMBUS_READ_I2C_BLOCK_2        
> > 0x10000000 /*I2C-like blk-xfer*/ +#define
> > I2C_FUNC_SMBUS_WRITE_I2C_BLOCK_2        0x20000000 /* w/ 2-byte regadr*/
> > +#define I2C_FUNC_SMBUS_READ_BLOCK_DATA_PEC      0x40000000 /* SMBus 2.0
> > */ +#define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC     0x80000000 /* SMBus
> > 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 = 1,
> > +    STR_PRODUCT_SERIAL,
> > +    STR_SERIALNUMBER,
> > +};
> > +
> > +static const USBDescStrings desc_strings = {
> > +    [STR_MANUFACTURER]    = "QEMU",
> > +    [STR_PRODUCT_SERIAL]  = "QEMU USB I2C Tiny",
> > +    [STR_SERIALNUMBER]    = "1",
> > +};
> > +
> > +static const USBDescIface desc_iface0 = {
> > +    .bInterfaceNumber              = 1,
> > +    .bNumEndpoints                 = 0,
> > +    .bInterfaceClass               = 0xff,
> > +    .bInterfaceSubClass            = 0xff,
> > +    .bInterfaceProtocol            = 0xff,
> > +    .eps = (USBDescEndpoint[]) {
> > +    }
> > +};
> > +
> > +static const USBDescDevice desc_device = {
> > +    .bcdUSB                        = 0x0110,
> > +    .bMaxPacketSize0               = 8,
> > +    .bNumConfigurations            = 1,
> > +    .confs = (USBDescConfig[]) {
> > +        {
> > +            .bNumInterfaces        = 1,
> > +            .bConfigurationValue   = 1,
> > +            .bmAttributes          = USB_CFG_ATT_ONE,
> > +            .bMaxPower             = 50,
> > +            .nif = 1,
> > +            .ifs = &desc_iface0,
> > +        },
> > +    },
> > +};
> > +
> > +static const USBDesc desc_usb_i2c = {
> > +    .id = {
> > +        .idVendor          = 0x0403,
> > +        .idProduct         = 0xc631,
> > +        .bcdDevice         = 0x0205,
> 
> Where did these magic numbers come from?
I guess i have forgotton to add the link of the hardware which i am 
aiming to emulate:
 http://www.harbaum.org/till/i2c_tiny_usb/index.shtml
Looking at the Linux driver it seems there are two supported manufacturer 
numbers, i just took one of them.

> > +        .iManufacturer     = STR_MANUFACTURER,
> > +        .iProduct          = STR_PRODUCT_SERIAL,
> > +        .iSerialNumber     = STR_SERIALNUMBER,
> > +    },
> > +    .full = &desc_device,
> > +    .str  = desc_strings,
> > +};
> > +
> > +static int usb_i2c_read_byte(I2CBus *bus, uint8_t addr)
> > +{
> > +    int retval;
> > +    i2c_start_transfer(bus, addr, 1);
> > +    retval = i2c_recv(bus);
> > +    i2c_end_transfer(bus);
> > +    return retval;
> > +}
> > +
> > +static int usb_i2c_write_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> > +{
> > +    int retval;
> > +    i2c_start_transfer(bus, addr, 0);
> > +    retval = 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, uint8_t
> > *data) +{
> > +    UsbI2cTinyState *s = (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 = usb_desc_handle_control(dev, p, request, value, index, length,
> > data); +    DPRINTF("usb_desc_handle_control return value: %i status:
> > %x\n", ret, +            p->status);
> 
> 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 
http://wiki.qemu.org/Features/Tracing/Roadmap page but this seems pretty 
outdated?

> > +    if (ret >= 0) {
> > +        return;
> > +    }
> > +
> > +    switch (request) {
> > +    case EndpointOutRequest | USB_REQ_CLEAR_FEATURE:
> > +        break;
> > +
> 
> 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: starting 
up the linux i2c-tiny-usb module and look for the results. So i don't know 
more than what i have written into the comments.

<snip>

Best regards
Tim