From patchwork Mon Nov 26 20:04:24 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 152078 Delivered-To: patch@linaro.org Received: by 2002:a2e:299d:0:0:0:0:0 with SMTP id p29-v6csp343220ljp; Mon, 26 Nov 2018 12:37:56 -0800 (PST) X-Google-Smtp-Source: AFSGD/W0TSPFFveuroAL6QP34kZpx5uF55heu0wRIcT7nTrmSsVfG/5uUfvOlsuxLeDO1Uty5YJ4 X-Received: by 2002:a81:ad1b:: with SMTP id l27mr29813396ywh.404.1543264676046; Mon, 26 Nov 2018 12:37:56 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1543264676; cv=none; d=google.com; s=arc-20160816; b=pBUR9jX3d/vQJzg2Q1kyo+HwxMjIKOnUF1Rbr7zumT9MCk6aJfrQSaHdc1Ktwykm99 8IOloomqTJ6Sj2njACpDRAE1ryy5nNooy9o3oqEEcrLAh2jEADxCmBi93Ly0i7VMAXPS wWHcvT0k+MLRqgqS/veX8WiFAbI2pWC7/PBcg/GxYA9fxiiLdqMuQYZSDYDJ8D6GXckb tKi2UBv5xbbmJz94rmsm6jJjlQeY1A1TNnyrZV3mlZ4GL4g6APwq8y6rQdGcL8PQT4vG Bd0H2Z03QRqGv4Jw9XEMUrvwEUh/ruL6sR5Z5RFTPci7Ffl5hfKz+3s94KOW9WXCa/lm jefw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:subject:references:in-reply-to :message-id:date:to:from:dkim-signature; bh=iVvmdeu6KFtf2LR/IN4C8tqjUZRuYpU2xLFosxhXC6U=; b=Y0hXDsAVkWxm+eq6kXL4/gfPmpi0JNFAeJ13ymTf5d6aYBWqw0RBZzURhOIXSa84SG iDeKTiT1q37sz4SMDcgQQB0P97IIsF6GHnZy9tGU96BsJqr/IPdumXoGI6QaRk33D0ka FFvXZlF/mV1ZMI1xjwYb55KereLRxGOUMmt/T0U51DzFhoRr7e/xbxQJQJiO+5mwpDrm p4iWYpZTJTbL3iISuxwrZjesKJm5ggUi3WJx7WBoMfEoAF5D4wT3dXqV7MWj9h58OFyp 0qTyaqnw3lnkLF9DYqI3yDF2ndL8CnlkEt4Ag2gLGzgesn93dLAhtOiMkivzS2LeHS5J Th1A== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=RuRHxumI; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-devel-bounces+patch=linaro.org@nongnu.org" Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id o144-v6si1027695ybg.74.2018.11.26.12.37.55 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 26 Nov 2018 12:37:56 -0800 (PST) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=RuRHxumI; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-devel-bounces+patch=linaro.org@nongnu.org" Received: from localhost ([::1]:38755 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRNdj-0006q7-BU for patch@linaro.org; Mon, 26 Nov 2018 15:37:55 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53728) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRN81-0001lo-PS for qemu-devel@nongnu.org; Mon, 26 Nov 2018 15:05:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRN7y-0001Ln-UZ for qemu-devel@nongnu.org; Mon, 26 Nov 2018 15:05:09 -0500 Received: from mail-ot1-x341.google.com ([2607:f8b0:4864:20::341]:46689) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gRN7y-0001Hj-HV for qemu-devel@nongnu.org; Mon, 26 Nov 2018 15:05:06 -0500 Received: by mail-ot1-x341.google.com with SMTP id w25so17838823otm.13 for ; Mon, 26 Nov 2018 12:04:58 -0800 (PST) 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=iVvmdeu6KFtf2LR/IN4C8tqjUZRuYpU2xLFosxhXC6U=; b=RuRHxumId07wji/LX1/MGzcIthN5aY4u/8R3J7oN6Rul7BQaUr7kiklCu+aL7LHorT EKib9yiuan7rTC37R+WtXd1qOzjtCQTmiqPe/Krr91Vq4adCufVa5Am+joCnS7ibmt5l FP3unKgY3+ry2CuIiXZmsG8fIdXTAwI9YtkgYYGdgS1p6BBQvfFoFNZTPx/qnuynv+cP DEv0XDSYvdJ98Dgn8NprxYRSTWeNJUQluFJ0vu9R0CeitVcghCTY6rlkYPcHlutEFrDI +JvJLW+SXWSvLTfz6qC0bo1cTh5GLcX7/ypaIpT8XLQWQxalPaSW6w25Chl/tHiTnSME h/+w== 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=iVvmdeu6KFtf2LR/IN4C8tqjUZRuYpU2xLFosxhXC6U=; b=S/tecFvlTIGd+Ps66rBnbjqBf5NiMlkkziM3JgCzz5Cx7m/QGiLCm6sodHA31E6r32 sx2zH7myAI8T3it5bamCAFuCtpKG/FMMbXc+JBqlIzcVq1og5xwdCD8tY8w8acjb6agy F4m0FGyaMivag32oxGBXeMSIs5tmLWv0xVvj22rrJYhSqBqYCT+Ch22Z7A9b1tLOv6/0 PzPtOEZWpT+0ZII4eyW1rbXu63J3e255G/qGhRu0YIH/hAOSRcJQCQUMRKlN9TVjHHMh ZTUS5swfEL7NaxfDICafhjrISSKLVGScnUMGbKR948EgVOgFi/xqsPp7tsAwPtapUPxw U8Ag== X-Gm-Message-State: AA+aEWZgPWWuzdmt4RRlP3V4CksWg+lgxe8EUuh++qj+sv8ujJd2DcaM NemWvg9DxNBpBx/uyC3NkA== X-Received: by 2002:a9d:3e50:: with SMTP id h16mr15110540otg.116.1543262697871; Mon, 26 Nov 2018 12:04:57 -0800 (PST) Received: from serve.minyard.net ([47.184.128.64]) by smtp.gmail.com with ESMTPSA id r1sm363443oie.44.2018.11.26.12.04.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 26 Nov 2018 12:04:52 -0800 (PST) Received: from t430.minyard.net (t430m.minyard.net [192.168.27.3]) by serve.minyard.net (Postfix) with ESMTPA id 995521D6C; Mon, 26 Nov 2018 14:04:48 -0600 (CST) Received: by t430.minyard.net (Postfix, from userid 1000) id 4FF65301465; Mon, 26 Nov 2018 14:04:46 -0600 (CST) From: minyard@acm.org To: qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , =?utf-8?q?Philippe_Mathieu-Daud=C3=A9?= , Peter Maydell Date: Mon, 26 Nov 2018 14:04:24 -0600 Message-Id: <20181126200435.23408-6-minyard@acm.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20181126200435.23408-1-minyard@acm.org> References: <20181126200435.23408-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::341 Subject: [Qemu-devel] [PATCH v3 05/16] 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 , Corey Minyard , "Michael S . Tsirkin" Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: "Qemu-devel" 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(-) -- 2.17.1 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; -static void eeprom_quick_cmd(SMBusDevice *dev, uint8_t read) -{ -#ifdef DEBUG - printf("eeprom_quick_cmd: addr=0x%02x read=%d\n", dev->i2c.address, read); -#endif -} - -static void eeprom_send_byte(SMBusDevice *dev, uint8_t val) -{ - SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev; -#ifdef DEBUG - printf("eeprom_send_byte: addr=0x%02x val=0x%02x\n", - dev->i2c.address, val); -#endif - eeprom->offset = val; -} - static uint8_t eeprom_receive_byte(SMBusDevice *dev) { SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev; uint8_t *data = eeprom->data; uint8_t val = data[eeprom->offset++]; + #ifdef DEBUG printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n", dev->i2c.address, val); @@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) return val; } -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 = (SMBusEEPROMDevice *) dev; - int n; + uint8_t *data = eeprom->data; + #ifdef DEBUG printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%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 = 256 - cmd; - else - n = len; - memcpy(eeprom->data + cmd, buf, n); - len -= n; - if (len) - memcpy(eeprom->data, buf + n, len); -} + /* len is guaranteed to be > 0 */ + eeprom->offset = buf[0]; + buf++; + len--; + + for (; len > 0; len--) { + data[eeprom->offset] = *buf++; + eeprom->offset = (eeprom->offset + 1) % 256; + } -static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n) -{ - SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev; - /* If this is the first byte then set the current position. */ - if (n == 0) - eeprom->offset = cmd; - /* As with writes, we implement block reads without the - SMBus length byte. */ - return eeprom_receive_byte(dev); + return 0; } static void smbus_eeprom_realize(DeviceState *dev, Error **errp) @@ -116,11 +89,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass); dc->realize = smbus_eeprom_realize; - sc->quick_cmd = eeprom_quick_cmd; - sc->send_byte = eeprom_send_byte; sc->receive_byte = eeprom_receive_byte; sc->write_data = eeprom_write_data; - sc->read_data = eeprom_read_data; dc->props = smbus_eeprom_properties; /* Reason: pointer property "data" */ dc->user_creatable = false; diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c index 549e7ae933..70ff29c095 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 = -1 @@ -54,20 +53,9 @@ static void smbus_do_write(SMBusDevice *dev) { SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev); - if (dev->data_len == 0) { - smbus_do_quick_cmd(dev, 0); - } else if (dev->data_len == 1) { - DPRINTF("Send Byte\n"); - if (sc->send_byte) { - sc->send_byte(dev, dev->data_buf[0]); - } - } else { - dev->command = 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); } } @@ -82,6 +70,7 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event) DPRINTF("Incoming data\n"); dev->mode = SMBUS_WRITE_DATA; break; + default: BADF("Unexpected send start condition in state %d\n", dev->mode); dev->mode = 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 = SMBUS_RECV_BYTE; + dev->mode = SMBUS_READ_DATA; break; + case SMBUS_WRITE_DATA: if (dev->data_len == 0) { BADF("Read after write with no data\n"); dev->mode = SMBUS_CONFUSED; } else { - if (dev->data_len > 1) { - smbus_do_write(dev); - } else { - dev->command = 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 = 0; dev->mode = SMBUS_READ_DATA; } break; + default: BADF("Unexpected recv start condition in state %d\n", dev->mode); dev->mode = SMBUS_CONFUSED; @@ -120,19 +104,29 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event) break; 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 == 0) { + if (dev->mode == SMBUS_WRITE_DATA || dev->mode == SMBUS_READ_DATA) { + smbus_do_quick_cmd(dev, dev->mode == SMBUS_READ_DATA); + } + } else { + SMBusDeviceClass *sc = 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 = SMBUS_IDLE; dev->data_len = 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 = SMBUS_DONE; break; + default: BADF("Unexpected NACK in state %d\n", dev->mode); dev->mode = SMBUS_CONFUSED; @@ -160,33 +156,22 @@ static uint8_t smbus_i2c_recv(I2CSlave *s) { SMBusDevice *dev = SMBUS_DEVICE(s); SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev); - uint8_t ret; + uint8_t ret = 0xff; switch (dev->mode) { - case SMBUS_RECV_BYTE: + case SMBUS_READ_DATA: if (sc->receive_byte) { ret = sc->receive_byte(dev); - } else { - ret = 0; - } - DPRINTF("Receive Byte %02x\n", ret); - dev->mode = SMBUS_DONE; - break; - case SMBUS_READ_DATA: - if (sc->read_data) { - ret = sc->read_data(dev, dev->command, dev->data_len); - dev->data_len++; - } else { - ret = 0; } DPRINTF("Read data %02x\n", ret); break; + default: BADF("Unexpected read in state %d\n", dev->mode); dev->mode = SMBUS_CONFUSED; - ret = 0; break; } + return ret; } @@ -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++] = data; break; + default: BADF("Unexpected write in state %d\n", dev->mode); break; } + return 0; } diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h index ff07ee005d..fad2db9c76 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 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. */ - 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; 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; }; #endif