From patchwork Thu Nov 15 19:24:37 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 151242 Delivered-To: patch@linaro.org Received: by 2002:a2e:299d:0:0:0:0:0 with SMTP id p29-v6csp7401355ljp; Thu, 15 Nov 2018 11:28:29 -0800 (PST) X-Google-Smtp-Source: AJdET5fuJ8NHC5btmh/6K9Y7KQxJhwKOK1ksRoMD3Prr0h/JrmYqfPUK2iZiIWt9TBgCRGG/+rWK X-Received: by 2002:aed:2a1a:: with SMTP id c26mr7201775qtd.147.1542310109327; Thu, 15 Nov 2018 11:28:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1542310109; cv=none; d=google.com; s=arc-20160816; b=G5xj2dQ7oKbYBZRXcfe5GrZ1k8dQFZ0QAlT+ZPI4a4WlpZA/HTi4Rcbz8KAoKRUjyH 1RYS28HgQKKa34EbnCf+U0AE2Q9zP0JNy7BgRtjxD3YPU3s9B37lelFJ2lPf7SU4GJt4 zIJ1BOgagoZQJ8oLsHH8C/xWeC6XrGhsUkx19Yyq+/lq1EOh9C+QlZHdXVpRkc8hboL3 /MFSK0AJ+oMneG+84y82shR46kVlU7m+SSGHLOQ3HXtDo8OyrwcvX9iKZEiSMmAfCiaQ SPSHsihIoHTc6XSeU4MGagANzTuJ3aRozryN+qMowbW4w+F3iccGZW4TXbiE8POdL3rR WYcw== 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=H/8Awj42/YgcnJsXHCmjWezKe+7Qp2kff9WmYGe52xE=; b=K3LS5uK6LPuntuLFE8bsTLi1RPRnNgqL76VcK8+gW6ffBiUJjIDoTlviLWkE2jgU3h LfYLgh9iXo7Nw6QrPiwhocRTksgjKYBySFxPE1Jjtkkhi4vnSi6WZ4YLqJb4NC3rJtlD ROA3YymRP86kM7MF9fk2gexg/Mmf14BXEDgiCtjpXlCiGZlPzxxHCMPUG7UuMLcqOllE knGUFXHZQ2nykinmczPMJCi8oq/AWDrvNv0t6rfRlLI/Up0TmN+KXmUwYKCF2OI3qrJe fdaNhkEr4F/KRqqLiTQUKEEBF+3FUzXheacSDmFhbiMob9GIRtyAecOeDB7NfNNIsp18 08yg== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=vO0pHBRp; 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 b22-v6si4190604qtr.173.2018.11.15.11.28.29 for (version=TLS1 cipher=AES128-SHA bits=128/128); Thu, 15 Nov 2018 11:28:29 -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=vO0pHBRp; 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]:40441 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gNNJU-0004nQ-N0 for patch@linaro.org; Thu, 15 Nov 2018 14:28:28 -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) 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-Received: by 2002:aca:5e84:: with SMTP id s126-v6mr4122069oib.115.1542309893199; Thu, 15 Nov 2018 11:24:53 -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) 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+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 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 = -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 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 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