From nobody Thu Nov 6 18:07:57 2025 Delivered-To: importer@patchew.org Received-SPF: temperror (zoho.com: Error in retrieving data from DNS) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=temperror (zoho.com: Error in retrieving data from DNS) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1542310058627974.2670268790208; Thu, 15 Nov 2018 11:27:38 -0800 (PST) Received: from localhost ([::1]:40441 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gNNIf-0004nQ-6Z for importer@patchew.org; Thu, 15 Nov 2018 14:27:37 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56163) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gNNGF-0003Rq-GC for qemu-devel@nongnu.org; Thu, 15 Nov 2018 14:25:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gNNG2-0007yp-O5 for qemu-devel@nongnu.org; Thu, 15 Nov 2018 14:25:07 -0500 Received: from mail-oi1-x244.google.com ([2607:f8b0:4864:20::244]:43458) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gNNG2-0007yK-HM for qemu-devel@nongnu.org; Thu, 15 Nov 2018 14:24:54 -0500 Received: by mail-oi1-x244.google.com with SMTP id j202-v6so17681185oih.10 for ; Thu, 15 Nov 2018 11:24:54 -0800 (PST) Received: from serve.minyard.net ([47.184.128.64]) by smtp.gmail.com with ESMTPSA id p37sm20465792otc.27.2018.11.15.11.24.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Nov 2018 11:24:51 -0800 (PST) Received: from t430.minyard.net (t430m.minyard.net [192.168.27.3]) by serve.minyard.net (Postfix) with ESMTPA id 87338C5F; Thu, 15 Nov 2018 13:24:49 -0600 (CST) Received: by t430.minyard.net (Postfix, from userid 1000) id 24E01301463; Thu, 15 Nov 2018 13:24:48 -0600 (CST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=H/8Awj42/YgcnJsXHCmjWezKe+7Qp2kff9WmYGe52xE=; b=vO0pHBRptCH8NCbxDyCrqPU2atvFltIQigY8imrF+BrHfDjr6nhH4vVNF3ME/yuuAc PXv6O0J3/7Nz9ZMqr1BPQrzYKUN8GZ4P//d9HY8m8t7ry1EnPrK1DPUr1upm4A89K+Ls 1IqgjoFoEx3zw+Ra8fGAzB8y/zrYzHAcmRseATCOAvBDaX0JAyGTKj8SGq2VD4Q0PH85 aqVrx59yFnxZyLD4ZnYK4FEKazFgilc23Wb9vs0590yT6EIGg/yJ85GgJXiVLsTBqV2F CXcTgGWuFmc/LqvWwH2Z2PyGro64PliDAMDRtGx/yiWJ0F3ZCVJGaOYahmTEDCuRY6DN be7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=H/8Awj42/YgcnJsXHCmjWezKe+7Qp2kff9WmYGe52xE=; b=b2ANUL1pwoSpGkql3dMz+6GlcDVKEQWs6ycOD8/w2gywGFHe9/hn7rVDZB+NPYrM2I SacIBmKL62JuYn7t87r7TvaUfSc6JJ6dYP/QhzVuduRcRL33iEN5abYEUavbOwRgbNT0 hLOEvbtGrVXcLeE74k1s1l3+KvArsD1GSQLvLob/ok3t/CGLn8PDd642jzyqR2qprbv6 jLX4pBTY7luF36/Cr2M+4d7QNvo2n2HSIXIGvTGRN8x9kBsCsWZGBOfHuPqRBSNNB78J IYecgCeBsm/qGzV67RAZAt0UtWptBkhlEwG6QjhS+SEPsB0ApOf+ABNKp7JXE9IooIGS qn+g== X-Gm-Message-State: AGRZ1gLUnqlwlPp04D2vzlilTulxfu7SVZAgKyePzVioHpNkgaWvd/rk 88jrEB3AIi9YGL8QT3qZW7l+c34= X-Google-Smtp-Source: AJdET5fxSQTbU1orKg3UQqMHk2u6iVzsjZBzpxS4GM7KR1o4h4lphKOjT15CJRPuLgM+GQPNb3JBHQ== X-Received: by 2002:aca:5e84:: with SMTP id s126-v6mr4122069oib.115.1542309893199; Thu, 15 Nov 2018 11:24:53 -0800 (PST) From: minyard@acm.org To: qemu-devel@nongnu.org Date: Thu, 15 Nov 2018 13:24:37 -0600 Message-Id: <20181115192446.17187-4-minyard@acm.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20181115192446.17187-1-minyard@acm.org> References: <20181115192446.17187-1-minyard@acm.org> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::244 Subject: [Qemu-devel] [PATCH v2 03/12] i2c: Simplify and correct the SMBus state machine X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paolo Bonzini , Corey Minyard , "Dr . David Alan Gilbert" , minyard@acm.org, "Michael S . Tsirkin" Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail-DKIM: fail (Header signature does not verify) Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Corey Minyard The SMBus slave code had an unneeded state, unnecessary function pointers and incorrectly handled quick commands. Rewrite it to simplify the code and make it work correctly. smbus_eeprom is the only user, so no other effects and the eeprom code also gets a significant simplification. Signed-off-by: Corey Minyard --- hw/i2c/smbus_eeprom.c | 58 ++++++----------------- hw/i2c/smbus_slave.c | 91 ++++++++++++++++-------------------- include/hw/i2c/smbus_slave.h | 45 +++++++++++++----- 3 files changed, 86 insertions(+), 108 deletions(-) diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index d82423aa7e..4d25222e23 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -36,28 +36,12 @@ typedef struct SMBusEEPROMDevice { uint8_t offset; } SMBusEEPROMDevice; =20 -static void eeprom_quick_cmd(SMBusDevice *dev, uint8_t read) -{ -#ifdef DEBUG - printf("eeprom_quick_cmd: addr=3D0x%02x read=3D%d\n", dev->i2c.address= , read); -#endif -} - -static void eeprom_send_byte(SMBusDevice *dev, uint8_t val) -{ - SMBusEEPROMDevice *eeprom =3D (SMBusEEPROMDevice *) dev; -#ifdef DEBUG - printf("eeprom_send_byte: addr=3D0x%02x val=3D0x%02x\n", - dev->i2c.address, val); -#endif - eeprom->offset =3D val; -} - static uint8_t eeprom_receive_byte(SMBusDevice *dev) { SMBusEEPROMDevice *eeprom =3D (SMBusEEPROMDevice *) dev; uint8_t *data =3D eeprom->data; uint8_t val =3D data[eeprom->offset++]; + #ifdef DEBUG printf("eeprom_receive_byte: addr=3D0x%02x val=3D0x%02x\n", dev->i2c.address, val); @@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) return val; } =20 -static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf,= int len) +static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) { SMBusEEPROMDevice *eeprom =3D (SMBusEEPROMDevice *) dev; - int n; + uint8_t *data =3D eeprom->data; + #ifdef DEBUG printf("eeprom_write_byte: addr=3D0x%02x cmd=3D0x%02x val=3D0x%02x\n", dev->i2c.address, cmd, buf[0]); #endif - /* A page write operation is not a valid SMBus command. - It is a block write without a length byte. Fortunately we - get the full block anyway. */ - /* TODO: Should this set the current location? */ - if (cmd + len > 256) - n =3D 256 - cmd; - else - n =3D len; - memcpy(eeprom->data + cmd, buf, n); - len -=3D n; - if (len) - memcpy(eeprom->data, buf + n, len); -} + /* len is guaranteed to be > 0 */ + eeprom->offset =3D buf[0]; + buf++; + len--; + + for (; len > 0; len--) { + data[eeprom->offset] =3D *buf++; + eeprom->offset =3D (eeprom->offset + 1) % 256; + } =20 -static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n) -{ - SMBusEEPROMDevice *eeprom =3D (SMBusEEPROMDevice *) dev; - /* If this is the first byte then set the current position. */ - if (n =3D=3D 0) - eeprom->offset =3D cmd; - /* As with writes, we implement block reads without the - SMBus length byte. */ - return eeprom_receive_byte(dev); + return 0; } =20 static void smbus_eeprom_realize(DeviceState *dev, Error **errp) @@ -116,11 +89,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klas= s, void *data) SMBusDeviceClass *sc =3D SMBUS_DEVICE_CLASS(klass); =20 dc->realize =3D smbus_eeprom_realize; - sc->quick_cmd =3D eeprom_quick_cmd; - sc->send_byte =3D eeprom_send_byte; sc->receive_byte =3D eeprom_receive_byte; sc->write_data =3D eeprom_write_data; - sc->read_data =3D eeprom_read_data; dc->props =3D smbus_eeprom_properties; /* Reason: pointer property "data" */ dc->user_creatable =3D false; diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c index 9e34023e6d..83ca041b5d 100644 --- a/hw/i2c/smbus_slave.c +++ b/hw/i2c/smbus_slave.c @@ -34,7 +34,6 @@ do { fprintf(stderr, "smbus: error: " fmt , ## __VA_ARGS_= _);} while (0) enum { SMBUS_IDLE, SMBUS_WRITE_DATA, - SMBUS_RECV_BYTE, SMBUS_READ_DATA, SMBUS_DONE, SMBUS_CONFUSED =3D -1 @@ -54,20 +53,9 @@ static void smbus_do_write(SMBusDevice *dev) { SMBusDeviceClass *sc =3D SMBUS_DEVICE_GET_CLASS(dev); =20 - if (dev->data_len =3D=3D 0) { - smbus_do_quick_cmd(dev, 0); - } else if (dev->data_len =3D=3D 1) { - DPRINTF("Send Byte\n"); - if (sc->send_byte) { - sc->send_byte(dev, dev->data_buf[0]); - } - } else { - dev->command =3D dev->data_buf[0]; - DPRINTF("Command %d len %d\n", dev->command, dev->data_len - 1); - if (sc->write_data) { - sc->write_data(dev, dev->command, dev->data_buf + 1, - dev->data_len - 1); - } + DPRINTF("Command %d len %d\n", dev->data_buf[0], dev->data_len); + if (sc->write_data) { + sc->write_data(dev, dev->data_buf, dev->data_len); } } =20 @@ -82,6 +70,7 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event ev= ent) DPRINTF("Incoming data\n"); dev->mode =3D SMBUS_WRITE_DATA; break; + default: BADF("Unexpected send start condition in state %d\n", dev->mod= e); dev->mode =3D SMBUS_CONFUSED; @@ -93,25 +82,20 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event = event) switch (dev->mode) { case SMBUS_IDLE: DPRINTF("Read mode\n"); - dev->mode =3D SMBUS_RECV_BYTE; + dev->mode =3D SMBUS_READ_DATA; break; + case SMBUS_WRITE_DATA: if (dev->data_len =3D=3D 0) { BADF("Read after write with no data\n"); dev->mode =3D SMBUS_CONFUSED; } else { - if (dev->data_len > 1) { - smbus_do_write(dev); - } else { - dev->command =3D dev->data_buf[0]; - DPRINTF("%02x: Command %d\n", dev->i2c.address, - dev->command); - } + smbus_do_write(dev); DPRINTF("Read mode\n"); - dev->data_len =3D 0; dev->mode =3D SMBUS_READ_DATA; } break; + default: BADF("Unexpected recv start condition in state %d\n", dev->mod= e); dev->mode =3D SMBUS_CONFUSED; @@ -120,19 +104,29 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_even= t event) break; =20 case I2C_FINISH: - switch (dev->mode) { - case SMBUS_WRITE_DATA: - smbus_do_write(dev); - break; - case SMBUS_RECV_BYTE: - smbus_do_quick_cmd(dev, 1); - break; - case SMBUS_READ_DATA: - BADF("Unexpected stop during receive\n"); - break; - default: - /* Nothing to do. */ - break; + if (dev->data_len =3D=3D 0) { + if (dev->mode =3D=3D SMBUS_WRITE_DATA || dev->mode =3D=3D SMBU= S_READ_DATA) { + smbus_do_quick_cmd(dev, dev->mode =3D=3D SMBUS_READ_DATA); + } + } else { + SMBusDeviceClass *sc =3D SMBUS_DEVICE_GET_CLASS(dev); + + switch (dev->mode) { + case SMBUS_WRITE_DATA: + smbus_do_write(dev); + break; + + case SMBUS_READ_DATA: + BADF("Unexpected stop during receive\n"); + break; + + default: + /* Nothing to do. */ + break; + } + if (sc->transaction_complete) { + sc->transaction_complete(dev); + } } dev->mode =3D SMBUS_IDLE; dev->data_len =3D 0; @@ -143,9 +137,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event= event) case SMBUS_DONE: /* Nothing to do. */ break; + case SMBUS_READ_DATA: dev->mode =3D SMBUS_DONE; break; + default: BADF("Unexpected NACK in state %d\n", dev->mode); dev->mode =3D SMBUS_CONFUSED; @@ -160,33 +156,22 @@ static uint8_t smbus_i2c_recv(I2CSlave *s) { SMBusDevice *dev =3D SMBUS_DEVICE(s); SMBusDeviceClass *sc =3D SMBUS_DEVICE_GET_CLASS(dev); - uint8_t ret; + uint8_t ret =3D 0xff; =20 switch (dev->mode) { - case SMBUS_RECV_BYTE: + case SMBUS_READ_DATA: if (sc->receive_byte) { ret =3D sc->receive_byte(dev); - } else { - ret =3D 0; - } - DPRINTF("Receive Byte %02x\n", ret); - dev->mode =3D SMBUS_DONE; - break; - case SMBUS_READ_DATA: - if (sc->read_data) { - ret =3D sc->read_data(dev, dev->command, dev->data_len); - dev->data_len++; - } else { - ret =3D 0; } DPRINTF("Read data %02x\n", ret); break; + default: BADF("Unexpected read in state %d\n", dev->mode); dev->mode =3D SMBUS_CONFUSED; - ret =3D 0; break; } + return ret; } =20 @@ -199,10 +184,12 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data) DPRINTF("Write data %02x\n", data); dev->data_buf[dev->data_len++] =3D data; break; + default: BADF("Unexpected write in state %d\n", dev->mode); break; } + return 0; } =20 diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h index 8a40fdd34e..eabac1dd73 100644 --- a/include/hw/i2c/smbus_slave.h +++ b/include/hw/i2c/smbus_slave.h @@ -38,19 +38,41 @@ typedef struct SMBusDeviceClass { I2CSlaveClass parent_class; + + /* + * An operation with no data, special in SMBus. + * This may be NULL, quick commands are ignore in that case. + */ void (*quick_cmd)(SMBusDevice *dev, uint8_t read); - void (*send_byte)(SMBusDevice *dev, uint8_t val); + + /* + * We can't distinguish between a word write and a block write with + * length 1, so pass the whole data block including the length byte + * (if present). The device is responsible figuring out what type of + * command this is. + * This may be NULL if no data is written to the device. Writes + * will be ignore in that case. + */ + int (*write_data)(SMBusDevice *dev, uint8_t *buf, uint8_t len); + + /* + * Likewise we can't distinguish between different reads, or even know + * the length of the read until the read is complete, so read data a + * byte at a time. The device is responsible for adding the length + * byte on block reads. This call cannot fail, it should return + * something, preferably 0xff if nothing is available. + * This may be NULL if no data is read from the device. Reads will + * return 0xff in that case. + */ uint8_t (*receive_byte)(SMBusDevice *dev); - /* We can't distinguish between a word write and a block write with - length 1, so pass the whole data block including the length byte - (if present). The device is responsible figuring out what type of - command this is. */ - void (*write_data)(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int le= n); - /* Likewise we can't distinguish between different reads, or even know - the length of the read until the read is complete, so read data a - byte at a time. The device is responsible for adding the length - byte on block reads. */ - uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n); + + /* + * Called whan an SMBus transaction has completed. This can be used + * so the device knows when an operation completes. This is not + * called after quick commands, those are complete by nature. + * This may be NULL if the device doesn't need this. + */ + void (*transaction_complete)(SMBusDevice *dev); } SMBusDeviceClass; =20 struct SMBusDevice { @@ -61,7 +83,6 @@ struct SMBusDevice { int mode; int data_len; uint8_t data_buf[34]; /* command + len + 32 bytes of data. */ - uint8_t command; }; =20 #endif --=20 2.17.1