* [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 22:22 ` Philippe Mathieu-Daudé
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 02/12] i2c: have I2C receive operation return uint8_t minyard
` (11 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
smbus.c and smbus.h had device side code, master side code, and
smbus.h has some smbus_eeprom.c definitions. Split them into
separate files.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/arm/aspeed.c | 2 +-
hw/i2c/Makefile.objs | 2 +-
hw/i2c/pm_smbus.c | 2 +-
hw/i2c/smbus_eeprom.c | 3 +-
hw/i2c/smbus_ich9.c | 2 -
hw/i2c/smbus_master.c | 165 ++++++++++++++++++++++
hw/i2c/{smbus.c => smbus_slave.c} | 153 +-------------------
hw/i386/pc_piix.c | 2 +-
hw/i386/pc_q35.c | 2 +-
hw/isa/vt82c686.c | 1 -
hw/mips/mips_fulong2e.c | 2 +-
hw/mips/mips_malta.c | 2 +-
hw/ppc/sam460ex.c | 2 +-
include/hw/i2c/pm_smbus.h | 2 +
include/hw/i2c/smbus_eeprom.h | 11 ++
include/hw/i2c/smbus_master.h | 55 ++++++++
include/hw/i2c/{smbus.h => smbus_slave.h} | 35 +----
17 files changed, 251 insertions(+), 192 deletions(-)
create mode 100644 hw/i2c/smbus_master.c
rename hw/i2c/{smbus.c => smbus_slave.c} (64%)
create mode 100644 include/hw/i2c/smbus_eeprom.h
create mode 100644 include/hw/i2c/smbus_master.h
rename include/hw/i2c/{smbus.h => smbus_slave.h} (65%)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6b33ecd5aa..69a19df00d 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -18,7 +18,7 @@
#include "hw/arm/aspeed.h"
#include "hw/arm/aspeed_soc.h"
#include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "qemu/log.h"
#include "sysemu/block-backend.h"
#include "hw/loader.h"
diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 37cacde978..8973edfa22 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o
+common-obj-$(CONFIG_I2C) += core.o smbus_slave.o smbus_master.o smbus_eeprom.o
common-obj-$(CONFIG_DDC) += i2c-ddc.o
common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
common-obj-$(CONFIG_ACPI_X86) += smbus_ich9.o
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 685a2378ed..f3c6cc46f9 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -20,7 +20,7 @@
#include "qemu/osdep.h"
#include "hw/hw.h"
#include "hw/i2c/pm_smbus.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_master.h"
#define SMBHSTSTS 0x00
#define SMBHSTCNT 0x02
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3de35..d82423aa7e 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -25,7 +25,8 @@
#include "qemu/osdep.h"
#include "hw/hw.h"
#include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/i2c/smbus_eeprom.h"
//#define DEBUG
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 2a8b49e02f..e6d8d28194 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -29,8 +29,6 @@
#include "hw/i2c/pm_smbus.h"
#include "hw/pci/pci.h"
#include "sysemu/sysemu.h"
-#include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
#include "hw/i386/ich9.h"
diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
new file mode 100644
index 0000000000..0a6223744c
--- /dev/null
+++ b/hw/i2c/smbus_master.c
@@ -0,0 +1,165 @@
+/*
+ * QEMU SMBus host (master) emulation.
+ *
+ * This code emulates SMBus transactions from the master point of view,
+ * it runs the individual I2C transaction to do the SMBus protocol
+ * over I2C.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ * Written by Paul Brook
+ *
+ * This code is licensed under the LGPL.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/hw.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/smbus_master.h"
+
+/* Master device commands. */
+int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
+{
+ if (i2c_start_transfer(bus, addr, read)) {
+ return -1;
+ }
+ i2c_end_transfer(bus);
+ return 0;
+}
+
+int smbus_receive_byte(I2CBus *bus, uint8_t addr)
+{
+ uint8_t data;
+
+ if (i2c_start_transfer(bus, addr, 1)) {
+ return -1;
+ }
+ data = i2c_recv(bus);
+ i2c_nack(bus);
+ i2c_end_transfer(bus);
+ return data;
+}
+
+int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
+{
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, data);
+ i2c_end_transfer(bus);
+ return 0;
+}
+
+int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
+{
+ uint8_t data;
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ if (i2c_start_transfer(bus, addr, 1)) {
+ i2c_end_transfer(bus);
+ return -1;
+ }
+ data = i2c_recv(bus);
+ i2c_nack(bus);
+ i2c_end_transfer(bus);
+ return data;
+}
+
+int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
+{
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ i2c_send(bus, data);
+ i2c_end_transfer(bus);
+ return 0;
+}
+
+int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
+{
+ uint16_t data;
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ if (i2c_start_transfer(bus, addr, 1)) {
+ i2c_end_transfer(bus);
+ return -1;
+ }
+ data = i2c_recv(bus);
+ data |= i2c_recv(bus) << 8;
+ i2c_nack(bus);
+ i2c_end_transfer(bus);
+ return data;
+}
+
+int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
+{
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ i2c_send(bus, data & 0xff);
+ i2c_send(bus, data >> 8);
+ i2c_end_transfer(bus);
+ return 0;
+}
+
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool recv_len, bool send_cmd)
+{
+ int rlen;
+ int i;
+
+ if (send_cmd) {
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ }
+ if (i2c_start_transfer(bus, addr, 1)) {
+ if (send_cmd) {
+ i2c_end_transfer(bus);
+ }
+ return -1;
+ }
+ if (recv_len) {
+ rlen = i2c_recv(bus);
+ } else {
+ rlen = len;
+ }
+ if (rlen > len) {
+ rlen = 0;
+ }
+ for (i = 0; i < rlen; i++) {
+ data[i] = i2c_recv(bus);
+ }
+ i2c_nack(bus);
+ i2c_end_transfer(bus);
+ return rlen;
+}
+
+int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool send_len)
+{
+ int i;
+
+ if (len > 32) {
+ len = 32;
+ }
+
+ if (i2c_start_transfer(bus, addr, 0)) {
+ return -1;
+ }
+ i2c_send(bus, command);
+ if (send_len) {
+ i2c_send(bus, len);
+ }
+ for (i = 0; i < len; i++) {
+ i2c_send(bus, data[i]);
+ }
+ i2c_end_transfer(bus);
+ return 0;
+}
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus_slave.c
similarity index 64%
rename from hw/i2c/smbus.c
rename to hw/i2c/smbus_slave.c
index 6ff77c582f..81d2a38111 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus_slave.c
@@ -1,6 +1,10 @@
/*
* QEMU SMBus device emulation.
*
+ * This code is a helper for SMBus device emulation. It implement an
+ * I2C device inteface and runs the SMBus protocol from the device
+ * point of view and maps those to simple calls to emulate.
+ *
* Copyright (c) 2007 CodeSourcery.
* Written by Paul Brook
*
@@ -12,7 +16,7 @@
#include "qemu/osdep.h"
#include "hw/hw.h"
#include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_slave.h"
//#define DEBUG_SMBUS 1
@@ -202,153 +206,6 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
return 0;
}
-/* Master device commands. */
-int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
-{
- if (i2c_start_transfer(bus, addr, read)) {
- return -1;
- }
- i2c_end_transfer(bus);
- return 0;
-}
-
-int smbus_receive_byte(I2CBus *bus, uint8_t addr)
-{
- uint8_t data;
-
- if (i2c_start_transfer(bus, addr, 1)) {
- return -1;
- }
- data = i2c_recv(bus);
- i2c_nack(bus);
- i2c_end_transfer(bus);
- return data;
-}
-
-int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
-{
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, data);
- i2c_end_transfer(bus);
- return 0;
-}
-
-int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
-{
- uint8_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- if (i2c_start_transfer(bus, addr, 1)) {
- i2c_end_transfer(bus);
- return -1;
- }
- data = i2c_recv(bus);
- i2c_nack(bus);
- i2c_end_transfer(bus);
- return data;
-}
-
-int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
-{
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- i2c_send(bus, data);
- i2c_end_transfer(bus);
- return 0;
-}
-
-int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
-{
- uint16_t data;
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- if (i2c_start_transfer(bus, addr, 1)) {
- i2c_end_transfer(bus);
- return -1;
- }
- data = i2c_recv(bus);
- data |= i2c_recv(bus) << 8;
- i2c_nack(bus);
- i2c_end_transfer(bus);
- return data;
-}
-
-int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
-{
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- i2c_send(bus, data & 0xff);
- i2c_send(bus, data >> 8);
- i2c_end_transfer(bus);
- return 0;
-}
-
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
- int len, bool recv_len, bool send_cmd)
-{
- int rlen;
- int i;
-
- if (send_cmd) {
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- }
- if (i2c_start_transfer(bus, addr, 1)) {
- if (send_cmd) {
- i2c_end_transfer(bus);
- }
- return -1;
- }
- if (recv_len) {
- rlen = i2c_recv(bus);
- } else {
- rlen = len;
- }
- if (rlen > len) {
- rlen = 0;
- }
- for (i = 0; i < rlen; i++) {
- data[i] = i2c_recv(bus);
- }
- i2c_nack(bus);
- i2c_end_transfer(bus);
- return rlen;
-}
-
-int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
- int len, bool send_len)
-{
- int i;
-
- if (len > 32)
- len = 32;
-
- if (i2c_start_transfer(bus, addr, 0)) {
- return -1;
- }
- i2c_send(bus, command);
- if (send_len) {
- i2c_send(bus, len);
- }
- for (i = 0; i < len; i++) {
- i2c_send(bus, data[i]);
- }
- i2c_end_transfer(bus);
- return 0;
-}
-
static void smbus_device_class_init(ObjectClass *klass, void *data)
{
I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..cb28227cc3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -42,7 +42,7 @@
#include "sysemu/sysemu.h"
#include "hw/sysbus.h"
#include "sysemu/arch_init.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "hw/xen/xen.h"
#include "exec/memory.h"
#include "exec/address-spaces.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..90e88c9b28 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -33,7 +33,7 @@
#include "hw/hw.h"
#include "hw/loader.h"
#include "sysemu/arch_init.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "hw/boards.h"
#include "hw/timer/mc146818rtc.h"
#include "hw/xen/xen.h"
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 7302f6d74b..85d0532dd5 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -14,7 +14,6 @@
#include "hw/hw.h"
#include "hw/isa/vt82c686.h"
#include "hw/i2c/i2c.h"
-#include "hw/i2c/smbus.h"
#include "hw/pci/pci.h"
#include "hw/isa/isa.h"
#include "hw/isa/superio.h"
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 2fbba32c48..dae8acc108 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -27,7 +27,7 @@
#include "hw/isa/superio.h"
#include "net/net.h"
#include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "hw/block/flash.h"
#include "hw/mips/mips.h"
#include "hw/mips/cpudevs.h"
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index c1cf0fe12e..1fb7170f5e 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -33,7 +33,7 @@
#include "hw/char/serial.h"
#include "net/net.h"
#include "hw/boards.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "hw/block/flash.h"
#include "hw/mips/mips.h"
#include "hw/mips/cpudevs.h"
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 5aac58f36e..7136b23f91 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -34,7 +34,7 @@
#include "hw/sysbus.h"
#include "hw/char/serial.h"
#include "hw/i2c/ppc4xx_i2c.h"
-#include "hw/i2c/smbus.h"
+#include "hw/i2c/smbus_eeprom.h"
#include "hw/usb/hcd-ehci.h"
#include "hw/ppc/fdt.h"
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 060d3c6ac0..6dd5b7040b 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -1,6 +1,8 @@
#ifndef PM_SMBUS_H
#define PM_SMBUS_H
+#include "hw/i2c/smbus_master.h"
+
#define PM_SMBUS_MAX_MSG_SIZE 32
typedef struct PMSMBus {
diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
new file mode 100644
index 0000000000..2f56e5dc4e
--- /dev/null
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -0,0 +1,11 @@
+
+#ifndef QEMU_SMBUS_EEPROM_H
+#define QEMU_SMBUS_EEPROM_H
+
+#include "hw/i2c/i2c.h"
+
+void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
+void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
+ const uint8_t *eeprom_spd, int size);
+
+#endif
diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
new file mode 100644
index 0000000000..7d8bba5e37
--- /dev/null
+++ b/include/hw/i2c/smbus_master.h
@@ -0,0 +1,55 @@
+#ifndef QEMU_SMBUS_MASTER_H
+#define QEMU_SMBUS_MASTER_H
+
+/*
+ * QEMU SMBus host (master) API
+ *
+ * Copyright (c) 2007 Arastra, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/i2c/i2c.h"
+
+/* Master device commands. */
+int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
+int smbus_receive_byte(I2CBus *bus, uint8_t addr);
+int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
+int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
+int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
+int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
+int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
+
+/*
+ * Do a block transfer from an I2C device. If recv_len is set, then the
+ * first received byte is a length field and is used to know how much data
+ * to receive. Otherwise receive "len" bytes. If send_cmd is set, send
+ * the command byte first before receiving the data.
+ */
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool recv_len, bool send_cmd);
+
+/*
+ * Do a block transfer to an I2C device. If send_len is set, send the
+ * "len" value before the data.
+ */
+int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+ int len, bool send_len);
+
+#endif
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus_slave.h
similarity index 65%
rename from include/hw/i2c/smbus.h
rename to include/hw/i2c/smbus_slave.h
index d8b1b9ee81..8a40fdd34e 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -1,8 +1,8 @@
-#ifndef QEMU_SMBUS_H
-#define QEMU_SMBUS_H
+#ifndef QEMU_SMBUS_DEV_H
+#define QEMU_SMBUS_DEV_H
/*
- * QEMU SMBus API
+ * QEMU SMBus device (slave) API
*
* Copyright (c) 2007 Arastra, Inc.
*
@@ -64,33 +64,4 @@ struct SMBusDevice {
uint8_t command;
};
-/* Master device commands. */
-int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
-int smbus_receive_byte(I2CBus *bus, uint8_t addr);
-int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
-int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
-int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
-int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
-int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
-
-/*
- * Do a block transfer from an I2C device. If recv_len is set, then the
- * first received byte is a length field and is used to know how much data
- * to receive. Otherwise receive "len" bytes. If send_cmd is set, send
- * the command byte first before receiving the data.
- */
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
- int len, bool recv_len, bool send_cmd);
-
-/*
- * Do a block transfer to an I2C device. If send_len is set, send the
- * "len" value before the data.
- */
-int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
- int len, bool send_len);
-
-void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
-void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
- const uint8_t *eeprom_spd, int size);
-
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts minyard
@ 2018-11-15 22:22 ` Philippe Mathieu-Daudé
2018-11-16 13:20 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-15 22:22 UTC (permalink / raw)
To: minyard, qemu-devel
Cc: Paolo Bonzini, Corey Minyard, Dr . David Alan Gilbert,
Michael S . Tsirkin
On 15/11/18 20:24, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> smbus.c and smbus.h had device side code, master side code, and
> smbus.h has some smbus_eeprom.c definitions. Split them into
> separate files.
Lovely cleanup!
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
> hw/arm/aspeed.c | 2 +-
> hw/i2c/Makefile.objs | 2 +-
> hw/i2c/pm_smbus.c | 2 +-
> hw/i2c/smbus_eeprom.c | 3 +-
> hw/i2c/smbus_ich9.c | 2 -
> hw/i2c/smbus_master.c | 165 ++++++++++++++++++++++
> hw/i2c/{smbus.c => smbus_slave.c} | 153 +-------------------
> hw/i386/pc_piix.c | 2 +-
> hw/i386/pc_q35.c | 2 +-
> hw/isa/vt82c686.c | 1 -
> hw/mips/mips_fulong2e.c | 2 +-
> hw/mips/mips_malta.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> include/hw/i2c/pm_smbus.h | 2 +
> include/hw/i2c/smbus_eeprom.h | 11 ++
> include/hw/i2c/smbus_master.h | 55 ++++++++
> include/hw/i2c/{smbus.h => smbus_slave.h} | 35 +----
> 17 files changed, 251 insertions(+), 192 deletions(-)
> create mode 100644 hw/i2c/smbus_master.c
> rename hw/i2c/{smbus.c => smbus_slave.c} (64%)
> create mode 100644 include/hw/i2c/smbus_eeprom.h
> create mode 100644 include/hw/i2c/smbus_master.h
> rename include/hw/i2c/{smbus.h => smbus_slave.h} (65%)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 6b33ecd5aa..69a19df00d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -18,7 +18,7 @@
> #include "hw/arm/aspeed.h"
> #include "hw/arm/aspeed_soc.h"
> #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "qemu/log.h"
> #include "sysemu/block-backend.h"
> #include "hw/loader.h"
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 37cacde978..8973edfa22 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o
> +common-obj-$(CONFIG_I2C) += core.o smbus_slave.o smbus_master.o smbus_eeprom.o
> common-obj-$(CONFIG_DDC) += i2c-ddc.o
> common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
> common-obj-$(CONFIG_ACPI_X86) += smbus_ich9.o
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 685a2378ed..f3c6cc46f9 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -20,7 +20,7 @@
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> #include "hw/i2c/pm_smbus.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_master.h"
>
> #define SMBHSTSTS 0x00
> #define SMBHSTCNT 0x02
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..d82423aa7e 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -25,7 +25,8 @@
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_slave.h"
> +#include "hw/i2c/smbus_eeprom.h"
>
> //#define DEBUG
>
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index 2a8b49e02f..e6d8d28194 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -29,8 +29,6 @@
> #include "hw/i2c/pm_smbus.h"
> #include "hw/pci/pci.h"
> #include "sysemu/sysemu.h"
> -#include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
>
> #include "hw/i386/ich9.h"
>
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> new file mode 100644
> index 0000000000..0a6223744c
> --- /dev/null
> +++ b/hw/i2c/smbus_master.c
> @@ -0,0 +1,165 @@
> +/*
> + * QEMU SMBus host (master) emulation.
> + *
> + * This code emulates SMBus transactions from the master point of view,
> + * it runs the individual I2C transaction to do the SMBus protocol
> + * over I2C.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the LGPL.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/hw.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/smbus_master.h"
> +
> +/* Master device commands. */
> +int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> +{
> + if (i2c_start_transfer(bus, addr, read)) {
> + return -1;
> + }
> + i2c_end_transfer(bus);
> + return 0;
> +}
> +
> +int smbus_receive_byte(I2CBus *bus, uint8_t addr)
> +{
> + uint8_t data;
> +
> + if (i2c_start_transfer(bus, addr, 1)) {
> + return -1;
> + }
> + data = i2c_recv(bus);
> + i2c_nack(bus);
> + i2c_end_transfer(bus);
> + return data;
> +}
> +
> +int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> +{
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, data);
> + i2c_end_transfer(bus);
> + return 0;
> +}
> +
> +int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
> +{
> + uint8_t data;
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + if (i2c_start_transfer(bus, addr, 1)) {
> + i2c_end_transfer(bus);
> + return -1;
> + }
> + data = i2c_recv(bus);
> + i2c_nack(bus);
> + i2c_end_transfer(bus);
> + return data;
> +}
> +
> +int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> +{
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + i2c_send(bus, data);
> + i2c_end_transfer(bus);
> + return 0;
> +}
> +
> +int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
> +{
> + uint16_t data;
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + if (i2c_start_transfer(bus, addr, 1)) {
> + i2c_end_transfer(bus);
> + return -1;
> + }
> + data = i2c_recv(bus);
> + data |= i2c_recv(bus) << 8;
> + i2c_nack(bus);
> + i2c_end_transfer(bus);
> + return data;
> +}
> +
> +int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
> +{
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + i2c_send(bus, data & 0xff);
> + i2c_send(bus, data >> 8);
> + i2c_end_transfer(bus);
> + return 0;
> +}
> +
> +int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> + int len, bool recv_len, bool send_cmd)
> +{
> + int rlen;
> + int i;
> +
> + if (send_cmd) {
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + }
> + if (i2c_start_transfer(bus, addr, 1)) {
> + if (send_cmd) {
> + i2c_end_transfer(bus);
> + }
> + return -1;
> + }
> + if (recv_len) {
> + rlen = i2c_recv(bus);
> + } else {
> + rlen = len;
> + }
> + if (rlen > len) {
> + rlen = 0;
> + }
> + for (i = 0; i < rlen; i++) {
> + data[i] = i2c_recv(bus);
> + }
> + i2c_nack(bus);
> + i2c_end_transfer(bus);
> + return rlen;
> +}
> +
> +int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> + int len, bool send_len)
> +{
> + int i;
> +
> + if (len > 32) {
> + len = 32;
> + }
> +
> + if (i2c_start_transfer(bus, addr, 0)) {
> + return -1;
> + }
> + i2c_send(bus, command);
> + if (send_len) {
> + i2c_send(bus, len);
> + }
> + for (i = 0; i < len; i++) {
> + i2c_send(bus, data[i]);
> + }
> + i2c_end_transfer(bus);
> + return 0;
> +}
> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus_slave.c
> similarity index 64%
> rename from hw/i2c/smbus.c
> rename to hw/i2c/smbus_slave.c
> index 6ff77c582f..81d2a38111 100644
> --- a/hw/i2c/smbus.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -1,6 +1,10 @@
> /*
> * QEMU SMBus device emulation.
> *
> + * This code is a helper for SMBus device emulation. It implement an
> + * I2C device inteface and runs the SMBus protocol from the device
> + * point of view and maps those to simple calls to emulate.
> + *
> * Copyright (c) 2007 CodeSourcery.
> * Written by Paul Brook
> *
> @@ -12,7 +16,7 @@
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_slave.h"
>
> //#define DEBUG_SMBUS 1
>
> @@ -202,153 +206,6 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
> return 0;
> }
>
> -/* Master device commands. */
> -int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> -{
> - if (i2c_start_transfer(bus, addr, read)) {
> - return -1;
> - }
> - i2c_end_transfer(bus);
> - return 0;
> -}
> -
> -int smbus_receive_byte(I2CBus *bus, uint8_t addr)
> -{
> - uint8_t data;
> -
> - if (i2c_start_transfer(bus, addr, 1)) {
> - return -1;
> - }
> - data = i2c_recv(bus);
> - i2c_nack(bus);
> - i2c_end_transfer(bus);
> - return data;
> -}
> -
> -int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
> -{
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, data);
> - i2c_end_transfer(bus);
> - return 0;
> -}
> -
> -int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
> -{
> - uint8_t data;
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - if (i2c_start_transfer(bus, addr, 1)) {
> - i2c_end_transfer(bus);
> - return -1;
> - }
> - data = i2c_recv(bus);
> - i2c_nack(bus);
> - i2c_end_transfer(bus);
> - return data;
> -}
> -
> -int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
> -{
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - i2c_send(bus, data);
> - i2c_end_transfer(bus);
> - return 0;
> -}
> -
> -int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
> -{
> - uint16_t data;
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - if (i2c_start_transfer(bus, addr, 1)) {
> - i2c_end_transfer(bus);
> - return -1;
> - }
> - data = i2c_recv(bus);
> - data |= i2c_recv(bus) << 8;
> - i2c_nack(bus);
> - i2c_end_transfer(bus);
> - return data;
> -}
> -
> -int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
> -{
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - i2c_send(bus, data & 0xff);
> - i2c_send(bus, data >> 8);
> - i2c_end_transfer(bus);
> - return 0;
> -}
> -
> -int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> - int len, bool recv_len, bool send_cmd)
> -{
> - int rlen;
> - int i;
> -
> - if (send_cmd) {
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - }
> - if (i2c_start_transfer(bus, addr, 1)) {
> - if (send_cmd) {
> - i2c_end_transfer(bus);
> - }
> - return -1;
> - }
> - if (recv_len) {
> - rlen = i2c_recv(bus);
> - } else {
> - rlen = len;
> - }
> - if (rlen > len) {
> - rlen = 0;
> - }
> - for (i = 0; i < rlen; i++) {
> - data[i] = i2c_recv(bus);
> - }
> - i2c_nack(bus);
> - i2c_end_transfer(bus);
> - return rlen;
> -}
> -
> -int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> - int len, bool send_len)
> -{
> - int i;
> -
> - if (len > 32)
> - len = 32;
> -
> - if (i2c_start_transfer(bus, addr, 0)) {
> - return -1;
> - }
> - i2c_send(bus, command);
> - if (send_len) {
> - i2c_send(bus, len);
> - }
> - for (i = 0; i < len; i++) {
> - i2c_send(bus, data[i]);
> - }
> - i2c_end_transfer(bus);
> - return 0;
> -}
> -
> static void smbus_device_class_init(ObjectClass *klass, void *data)
> {
> I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dc09466b3e..cb28227cc3 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -42,7 +42,7 @@
> #include "sysemu/sysemu.h"
> #include "hw/sysbus.h"
> #include "sysemu/arch_init.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "hw/xen/xen.h"
> #include "exec/memory.h"
> #include "exec/address-spaces.h"
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 532241e3f8..90e88c9b28 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -33,7 +33,7 @@
> #include "hw/hw.h"
> #include "hw/loader.h"
> #include "sysemu/arch_init.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "hw/boards.h"
> #include "hw/timer/mc146818rtc.h"
> #include "hw/xen/xen.h"
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 7302f6d74b..85d0532dd5 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -14,7 +14,6 @@
> #include "hw/hw.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/i2c/i2c.h"
> -#include "hw/i2c/smbus.h"
> #include "hw/pci/pci.h"
> #include "hw/isa/isa.h"
> #include "hw/isa/superio.h"
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 2fbba32c48..dae8acc108 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -27,7 +27,7 @@
> #include "hw/isa/superio.h"
> #include "net/net.h"
> #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "hw/block/flash.h"
> #include "hw/mips/mips.h"
> #include "hw/mips/cpudevs.h"
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index c1cf0fe12e..1fb7170f5e 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -33,7 +33,7 @@
> #include "hw/char/serial.h"
> #include "net/net.h"
> #include "hw/boards.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "hw/block/flash.h"
> #include "hw/mips/mips.h"
> #include "hw/mips/cpudevs.h"
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 5aac58f36e..7136b23f91 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -34,7 +34,7 @@
> #include "hw/sysbus.h"
> #include "hw/char/serial.h"
> #include "hw/i2c/ppc4xx_i2c.h"
> -#include "hw/i2c/smbus.h"
> +#include "hw/i2c/smbus_eeprom.h"
> #include "hw/usb/hcd-ehci.h"
> #include "hw/ppc/fdt.h"
>
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 060d3c6ac0..6dd5b7040b 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -1,6 +1,8 @@
> #ifndef PM_SMBUS_H
> #define PM_SMBUS_H
>
> +#include "hw/i2c/smbus_master.h"
> +
> #define PM_SMBUS_MAX_MSG_SIZE 32
>
> typedef struct PMSMBus {
> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
> new file mode 100644
> index 0000000000..2f56e5dc4e
> --- /dev/null
> +++ b/include/hw/i2c/smbus_eeprom.h
> @@ -0,0 +1,11 @@
You missed the copyright notice here.
> +
> +#ifndef QEMU_SMBUS_EEPROM_H
> +#define QEMU_SMBUS_EEPROM_H
> +
> +#include "hw/i2c/i2c.h"
> +
> +void smbus_eeprom_init_one(I2CBus *bus, uint8_t address, uint8_t *eeprom_buf);
> +void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
> + const uint8_t *eeprom_spd, int size);
> +
> +#endif
> diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> new file mode 100644
> index 0000000000..7d8bba5e37
> --- /dev/null
> +++ b/include/hw/i2c/smbus_master.h
> @@ -0,0 +1,55 @@
> +#ifndef QEMU_SMBUS_MASTER_H
> +#define QEMU_SMBUS_MASTER_H
Maybe HW_SMBUS_MASTER_H?
(same apply for other headers: QEMU_ -> HW_).
> +
> +/*
> + * QEMU SMBus host (master) API
> + *
> + * Copyright (c) 2007 Arastra, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
Personally I prefer the #ifndef/define HW_SMBUS_MASTER_H here, matter of
taste.
> +
> +#include "hw/i2c/i2c.h"
> +
> +/* Master device commands. */
> +int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> +int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> +int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
> +int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
> +int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
> +int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
> +int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
> +
> +/*
> + * Do a block transfer from an I2C device. If recv_len is set, then the
> + * first received byte is a length field and is used to know how much data
> + * to receive. Otherwise receive "len" bytes. If send_cmd is set, send
> + * the command byte first before receiving the data.
> + */
> +int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> + int len, bool recv_len, bool send_cmd);
> +
> +/*
> + * Do a block transfer to an I2C device. If send_len is set, send the
> + * "len" value before the data.
> + */
> +int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> + int len, bool send_len);
> +
> +#endif
> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus_slave.h
> similarity index 65%
> rename from include/hw/i2c/smbus.h
> rename to include/hw/i2c/smbus_slave.h
> index d8b1b9ee81..8a40fdd34e 100644
> --- a/include/hw/i2c/smbus.h
> +++ b/include/hw/i2c/smbus_slave.h
> @@ -1,8 +1,8 @@
> -#ifndef QEMU_SMBUS_H
> -#define QEMU_SMBUS_H
> +#ifndef QEMU_SMBUS_DEV_H
> +#define QEMU_SMBUS_DEV_H
>
> /*
> - * QEMU SMBus API
> + * QEMU SMBus device (slave) API
> *
> * Copyright (c) 2007 Arastra, Inc.
> *
> @@ -64,33 +64,4 @@ struct SMBusDevice {
> uint8_t command;
> };
>
> -/* Master device commands. */
> -int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> -int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> -int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data);
> -int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
> -int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
> -int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
> -int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
> -
> -/*
> - * Do a block transfer from an I2C device. If recv_len is set, then the
> - * first received byte is a length field and is used to know how much data
> - * to receive. Otherwise receive "len" bytes. If send_cmd is set, send
> - * the command byte first before receiving the data.
> - */
> -int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> - int len, bool recv_len, bool send_cmd);
> -
> -/*
> - * Do a block transfer to an I2C device. If send_len is set, send the
> - * "len" value before the data.
> - */
> -int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
> - int len, bool send_len);
> -
> -void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
> -void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> - const uint8_t *eeprom_spd, int size);
> -
> #endif
>
LGTM, thanks,
Phil.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-15 22:22 ` Philippe Mathieu-Daudé
@ 2018-11-16 13:20 ` Corey Minyard
2018-11-20 15:47 ` Peter Maydell
0 siblings, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-16 13:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Corey Minyard, Dr . David Alan Gilbert,
Michael S . Tsirkin
On 11/15/18 4:22 PM, Philippe Mathieu-Daudé wrote:
> On 15/11/18 20:24, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> smbus.c and smbus.h had device side code, master side code, and
>> smbus.h has some smbus_eeprom.c definitions. Split them into
>> separate files.
>
> Lovely cleanup!
>
Yes, this really needed to be done. It took me a while to understand
this code originally because it was all mixed up.
I've fixed the things you pointed out. One thing, though:
>>
>> diff --git a/include/hw/i2c/smbus_eeprom.h
>> b/include/hw/i2c/smbus_eeprom.h
>> new file mode 100644
>> index 0000000000..2f56e5dc4e
>> --- /dev/null
>> +++ b/include/hw/i2c/smbus_eeprom.h
>> @@ -0,0 +1,11 @@
>
> You missed the copyright notice here.
Other files don't have copyright notices (i2c.h, for instance), and for
the smbus.[ch] case the copyrights are kind of mixed up, the include
files had the big header with a copyright by one company and the C
file had a different copyright notice by a different company.
Not a huge deal, but I didn't include it in that file because I didn't
think it was necessary. I'm wondering if it would be best to
establish a style like Linux has, with the // SPDX... thing on the
first line.
Thanks,
-corey
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-16 13:20 ` Corey Minyard
@ 2018-11-20 15:47 ` Peter Maydell
2018-11-20 19:30 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 38+ messages in thread
From: Peter Maydell @ 2018-11-20 15:47 UTC (permalink / raw)
To: Corey Minyard
Cc: Philippe Mathieu-Daudé,
QEMU Developers, Paolo Bonzini, Michael S . Tsirkin,
Dr . David Alan Gilbert, Corey Minyard
On 16 November 2018 at 13:20, Corey Minyard <minyard@acm.org> wrote:
> On 11/15/18 4:22 PM, Philippe Mathieu-Daudé wrote:
>>> --- /dev/null
>>> +++ b/include/hw/i2c/smbus_eeprom.h
>>> @@ -0,0 +1,11 @@
>>
>>
>> You missed the copyright notice here.
>
>
> Other files don't have copyright notices (i2c.h, for instance), and for
> the smbus.[ch] case the copyrights are kind of mixed up, the include
> files had the big header with a copyright by one company and the C
> file had a different copyright notice by a different company.
>
> Not a huge deal, but I didn't include it in that file because I didn't
> think it was necessary. I'm wondering if it would be best to
> establish a style like Linux has, with the // SPDX... thing on the
> first line.
Yeah, we have some legacy files with no copyright notice, but we
usually try to avoid that for new files. New files should have
a copyright notice and a license statement. (If you copied from
a file without a license statement, LICENSE says that means
2-or-later.)
We don't yet use SPDX headers. (They're just a different and
shorter way to write the license statement.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-20 15:47 ` Peter Maydell
@ 2018-11-20 19:30 ` Philippe Mathieu-Daudé
2018-11-21 11:59 ` Peter Maydell
0 siblings, 1 reply; 38+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-20 19:30 UTC (permalink / raw)
To: Peter Maydell, Corey Minyard
Cc: QEMU Developers, Paolo Bonzini, Michael S . Tsirkin,
Dr . David Alan Gilbert, Corey Minyard
On 20/11/18 16:47, Peter Maydell wrote:
> On 16 November 2018 at 13:20, Corey Minyard <minyard@acm.org> wrote:
>> On 11/15/18 4:22 PM, Philippe Mathieu-Daudé wrote:
>>>> --- /dev/null
>>>> +++ b/include/hw/i2c/smbus_eeprom.h
>>>> @@ -0,0 +1,11 @@
>>>
>>>
>>> You missed the copyright notice here.
>>
>>
>> Other files don't have copyright notices (i2c.h, for instance), and for
>> the smbus.[ch] case the copyrights are kind of mixed up, the include
>> files had the big header with a copyright by one company and the C
>> file had a different copyright notice by a different company.
>>
>> Not a huge deal, but I didn't include it in that file because I didn't
>> think it was necessary. I'm wondering if it would be best to
>> establish a style like Linux has, with the // SPDX... thing on the
>> first line.
>
> Yeah, we have some legacy files with no copyright notice, but we
> usually try to avoid that for new files. New files should have
> a copyright notice and a license statement. (If you copied from
> a file without a license statement, LICENSE says that means
> 2-or-later.)
>
> We don't yet use SPDX headers. (They're just a different and
> shorter way to write the license statement.)
Does that mean we can use them, or you rather prefer we don't?
While they are machine parseable, I find them easier to understand than
the big chunk of legal text that sometime are not correctly written.
Thanks,
Phil.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts
2018-11-20 19:30 ` Philippe Mathieu-Daudé
@ 2018-11-21 11:59 ` Peter Maydell
0 siblings, 0 replies; 38+ messages in thread
From: Peter Maydell @ 2018-11-21 11:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Corey Minyard, QEMU Developers, Paolo Bonzini,
Michael S . Tsirkin, Dr . David Alan Gilbert, Corey Minyard
On 20 November 2018 at 19:30, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 20/11/18 16:47, Peter Maydell wrote:
>> We don't yet use SPDX headers. (They're just a different and
>> shorter way to write the license statement.)
>
>
> Does that mean we can use them, or you rather prefer we don't?
>
> While they are machine parseable, I find them easier to understand than the
> big chunk of legal text that sometime are not correctly written.
It means that I'm not going to absolutely insist on dropping
the line if somebody submits a patch with an SPDX tag, but
I probably will mention that we don't use SPDX tags, and
definitely I'm not going to ask for them.
Overall I don't think there's much point in having them
added to one or two files randomly. If we want them then
we should consistently require them as policy. But that is
work, and so I think we should not do that until/unless
somebody (probably a corporate somebody) steps forward
to make the argument for "this is why we should have them,
we as a contributor to the project think they are worthwhile
and a useful feature for us, and we will make the effort to
add them, review that they are correct, update checkpatch to
insist on tags for new files, etc". In other words, "if it
ain't broke, don't fix it"; nobody is yet complaining that
our current setup is broken.
thanks
-- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 02/12] i2c: have I2C receive operation return uint8_t
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts minyard
@ 2018-11-15 19:24 ` minyard
2018-11-20 15:31 ` Peter Maydell
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 03/12] i2c: Simplify and correct the SMBus state machine minyard
` (10 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
It is never supposed to fail and cannot return an error, so just
have it return the proper type. Have it return 0xff on nothing
available, since that's what would happen on a real bus.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/arm/pxa2xx.c | 2 +-
hw/arm/tosa.c | 4 ++--
hw/arm/z2.c | 2 +-
hw/audio/wm8750.c | 2 +-
hw/display/sii9022.c | 2 +-
hw/display/ssd0303.c | 4 ++--
hw/gpio/max7310.c | 2 +-
hw/i2c/core.c | 32 +++++++++++++-------------------
hw/i2c/i2c-ddc.c | 2 +-
hw/i2c/smbus_slave.c | 4 ++--
hw/input/lm832x.c | 2 +-
hw/misc/pca9552.c | 2 +-
hw/misc/tmp105.c | 2 +-
hw/misc/tmp421.c | 2 +-
hw/nvram/eeprom_at24c.c | 4 ++--
hw/timer/ds1338.c | 2 +-
hw/timer/m41t80.c | 2 +-
hw/timer/twl92230.c | 2 +-
include/hw/i2c/i2c.h | 7 +++----
19 files changed, 37 insertions(+), 44 deletions(-)
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f598a1c053..3d7c88910e 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1286,7 +1286,7 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int pxa2xx_i2c_rx(I2CSlave *i2c)
+static uint8_t pxa2xx_i2c_rx(I2CSlave *i2c)
{
PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
PXA2xxI2CState *s = slave->host;
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 7a925fa5e6..eef9d427e7 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -197,10 +197,10 @@ static int tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int tosa_dac_recv(I2CSlave *s)
+static uint8_t tosa_dac_recv(I2CSlave *s)
{
printf("%s: recv not supported!!!\n", __func__);
- return -1;
+ return 0xff;
}
static void tosa_tg_init(PXA2xxState *cpu)
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 697a822f1e..6f18d924df 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -243,7 +243,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int aer915_recv(I2CSlave *slave)
+static uint8_t aer915_recv(I2CSlave *slave)
{
AER915State *s = AER915(slave);
int retval = 0x00;
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index f4aa838f62..169b006ade 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -561,7 +561,7 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
return 0;
}
-static int wm8750_rx(I2CSlave *i2c)
+static uint8_t wm8750_rx(I2CSlave *i2c)
{
return 0x00;
}
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index eaf11a6e7b..9994385c35 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -79,7 +79,7 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int sii9022_rx(I2CSlave *i2c)
+static uint8_t sii9022_rx(I2CSlave *i2c)
{
sii9022_state *s = SII9022(i2c);
uint8_t res = 0x00;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index eb90ba26be..8edf34986c 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -62,10 +62,10 @@ typedef struct {
uint8_t framebuffer[132*8];
} ssd0303_state;
-static int ssd0303_recv(I2CSlave *i2c)
+static uint8_t ssd0303_recv(I2CSlave *i2c)
{
BADF("Reads not implemented\n");
- return -1;
+ return 0xff;
}
static int ssd0303_send(I2CSlave *i2c, uint8_t data)
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index a560e3afd2..f35a930276 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -39,7 +39,7 @@ static void max7310_reset(DeviceState *dev)
s->command = 0x00;
}
-static int max7310_rx(I2CSlave *i2c)
+static uint8_t max7310_rx(I2CSlave *i2c)
{
MAX7310State *s = MAX7310(i2c);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index b54725985a..15237ad073 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -191,23 +191,17 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
}
return ret ? -1 : 0;
} else {
- if ((QLIST_EMPTY(&bus->current_devs)) || (bus->broadcast)) {
- return -1;
- }
-
- sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
- if (sc->recv) {
- s = QLIST_FIRST(&bus->current_devs)->elt;
- ret = sc->recv(s);
- trace_i2c_recv(s->address, ret);
- if (ret < 0) {
- return ret;
- } else {
- *data = ret;
- return 0;
+ ret = 0xff;
+ if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
+ sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
+ if (sc->recv) {
+ s = QLIST_FIRST(&bus->current_devs)->elt;
+ ret = sc->recv(s);
+ trace_i2c_recv(s->address, ret);
}
}
- return -1;
+ *data = ret;
+ return 0;
}
}
@@ -216,12 +210,12 @@ int i2c_send(I2CBus *bus, uint8_t data)
return i2c_send_recv(bus, &data, true);
}
-int i2c_recv(I2CBus *bus)
+uint8_t i2c_recv(I2CBus *bus)
{
- uint8_t data;
- int ret = i2c_send_recv(bus, &data, false);
+ uint8_t data = 0xff;
- return ret < 0 ? ret : data;
+ i2c_send_recv(bus, &data, false);
+ return data;
}
void i2c_nack(I2CBus *bus)
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index be34fe072c..95325358db 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -51,7 +51,7 @@ static int i2c_ddc_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int i2c_ddc_rx(I2CSlave *i2c)
+static uint8_t i2c_ddc_rx(I2CSlave *i2c)
{
I2CDDCState *s = I2CDDC(i2c);
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 81d2a38111..9e34023e6d 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -156,11 +156,11 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
return 0;
}
-static int smbus_i2c_recv(I2CSlave *s)
+static uint8_t smbus_i2c_recv(I2CSlave *s)
{
SMBusDevice *dev = SMBUS_DEVICE(s);
SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
- int ret;
+ uint8_t ret;
switch (dev->mode) {
case SMBUS_RECV_BYTE:
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index 74da30d9ca..9ae037953d 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -401,7 +401,7 @@ static int lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int lm_i2c_rx(I2CSlave *i2c)
+static uint8_t lm_i2c_rx(I2CSlave *i2c)
{
LM823KbdState *s = LM8323(i2c);
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 9775d5274a..7325d3f287 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -115,7 +115,7 @@ static void pca9552_autoinc(PCA9552State *s)
}
}
-static int pca9552_recv(I2CSlave *i2c)
+static uint8_t pca9552_recv(I2CSlave *i2c)
{
PCA9552State *s = PCA9552(i2c);
uint8_t ret;
diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
index 0918f3a6ea..a4cae665b7 100644
--- a/hw/misc/tmp105.c
+++ b/hw/misc/tmp105.c
@@ -147,7 +147,7 @@ static void tmp105_write(TMP105State *s)
}
}
-static int tmp105_rx(I2CSlave *i2c)
+static uint8_t tmp105_rx(I2CSlave *i2c)
{
TMP105State *s = TMP105(i2c);
diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
index c234044305..a75eb994a8 100644
--- a/hw/misc/tmp421.c
+++ b/hw/misc/tmp421.c
@@ -249,7 +249,7 @@ static void tmp421_write(TMP421State *s)
}
}
-static int tmp421_rx(I2CSlave *i2c)
+static uint8_t tmp421_rx(I2CSlave *i2c)
{
TMP421State *s = TMP421(i2c);
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 27cd01e615..d1456dafbd 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -74,10 +74,10 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
}
static
-int at24c_eeprom_recv(I2CSlave *s)
+uint8_t at24c_eeprom_recv(I2CSlave *s)
{
EEPROMState *ee = AT24C_EE(s);
- int ret;
+ uint8_t ret;
ret = ee->mem[ee->cur];
diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
index 3849b74a68..03da75486b 100644
--- a/hw/timer/ds1338.c
+++ b/hw/timer/ds1338.c
@@ -117,7 +117,7 @@ static int ds1338_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int ds1338_recv(I2CSlave *i2c)
+static uint8_t ds1338_recv(I2CSlave *i2c)
{
DS1338State *s = DS1338(i2c);
uint8_t res;
diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
index 734d7d95fc..c45b9297d8 100644
--- a/hw/timer/m41t80.c
+++ b/hw/timer/m41t80.c
@@ -40,7 +40,7 @@ static int m41t80_send(I2CSlave *i2c, uint8_t data)
return 0;
}
-static int m41t80_recv(I2CSlave *i2c)
+static uint8_t m41t80_recv(I2CSlave *i2c)
{
M41t80State *s = M41T80(i2c);
struct tm now;
diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
index 3b43b46199..659b216dca 100644
--- a/hw/timer/twl92230.c
+++ b/hw/timer/twl92230.c
@@ -737,7 +737,7 @@ static int menelaus_tx(I2CSlave *i2c, uint8_t data)
return 0;
}
-static int menelaus_rx(I2CSlave *i2c)
+static uint8_t menelaus_rx(I2CSlave *i2c)
{
MenelausState *s = TWL92230(i2c);
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 5dc166158b..75c5bd638b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -33,10 +33,9 @@ typedef struct I2CSlaveClass {
/*
* Slave to master. This cannot fail, the device should always
- * return something here. Negative values probably result in 0xff
- * and a possible log from the driver, and shouldn't be used.
+ * return something here.
*/
- int (*recv)(I2CSlave *s);
+ uint8_t (*recv)(I2CSlave *s);
/*
* Notify the slave of a bus state change. For start event,
@@ -78,7 +77,7 @@ void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
int i2c_send(I2CBus *bus, uint8_t data);
-int i2c_recv(I2CBus *bus);
+uint8_t i2c_recv(I2CBus *bus);
DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/12] i2c: have I2C receive operation return uint8_t
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 02/12] i2c: have I2C receive operation return uint8_t minyard
@ 2018-11-20 15:31 ` Peter Maydell
0 siblings, 0 replies; 38+ messages in thread
From: Peter Maydell @ 2018-11-20 15:31 UTC (permalink / raw)
To: Corey Minyard
Cc: QEMU Developers, Paolo Bonzini, Corey Minyard,
Dr . David Alan Gilbert, Michael S . Tsirkin
On 15 November 2018 at 19:24, <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> It is never supposed to fail and cannot return an error, so just
> have it return the proper type. Have it return 0xff on nothing
> available, since that's what would happen on a real bus.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
This is a nice cleanup. There are some followups at
callsites of i2c_recv():
hw/arm/stellaris.c: s->mdr = i2c_recv(s->bus) & 0xff;
-- mask not needed
hw/i2c/aspeed_i2c.c: ret = i2c_recv(bus->bus);
hw/i2c/aspeed_i2c.c- if (ret < 0) {
hw/i2c/aspeed_i2c.c- qemu_log_mask(LOG_GUEST_ERROR, "%s: read
failed\n", __func__);
hw/i2c/aspeed_i2c.c- ret = 0xff;
-- error handling can be deleted
hw/i2c/exynos4210_i2c.c: ret = i2c_recv(s->bus);
hw/i2c/exynos4210_i2c.c- if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
hw/i2c/exynos4210_i2c.c- s->i2cstat |= I2CSTAT_LAST_BIT; /*
Data is not acknowledged */
-- ditto
hw/i2c/imx_i2c.c: ret = i2c_recv(s->bus);
hw/i2c/imx_i2c.c-
hw/i2c/imx_i2c.c- if (ret >= 0) {
hw/i2c/imx_i2c.c- imx_i2c_raise_interrupt(s);
-- condition now always-true
which should probably go in their own patch(es).
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 03/12] i2c: Simplify and correct the SMBus state machine
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 01/12] i2c: Split smbus into parts minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 02/12] i2c: have I2C receive operation return uint8_t minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling minyard
` (9 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
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 <cminyard@mvista.com>
---
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;
-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
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (2 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 03/12] i2c: Simplify and correct the SMBus state machine minyard
@ 2018-11-15 19:24 ` minyard
2018-11-20 15:33 ` Peter Maydell
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 05/12] i2c: Fix pm_smbus handling of I2C block read minyard
` (8 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Avoid an overflow.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/i2c/smbus_slave.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 83ca041b5d..fa988919d8 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
switch (dev->mode) {
case SMBUS_WRITE_DATA:
DPRINTF("Write data %02x\n", data);
- dev->data_buf[dev->data_len++] = data;
+ if (dev->data_len >= sizeof(dev->data_buf)) {
+ BADF("Too many bytes sent\n");
+ } else {
+ dev->data_buf[dev->data_len++] = data;
+ }
break;
default:
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling minyard
@ 2018-11-20 15:33 ` Peter Maydell
2018-11-20 16:58 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Peter Maydell @ 2018-11-20 15:33 UTC (permalink / raw)
To: Corey Minyard
Cc: QEMU Developers, Paolo Bonzini, Corey Minyard,
Dr . David Alan Gilbert, Michael S . Tsirkin
On 15 November 2018 at 19:24, <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> Avoid an overflow.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
> hw/i2c/smbus_slave.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 83ca041b5d..fa988919d8 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
> switch (dev->mode) {
> case SMBUS_WRITE_DATA:
> DPRINTF("Write data %02x\n", data);
> - dev->data_buf[dev->data_len++] = data;
> + if (dev->data_len >= sizeof(dev->data_buf)) {
> + BADF("Too many bytes sent\n");
> + } else {
> + dev->data_buf[dev->data_len++] = data;
> + }
> break;
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
What happens on a real device in this situation ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling
2018-11-20 15:33 ` Peter Maydell
@ 2018-11-20 16:58 ` Corey Minyard
0 siblings, 0 replies; 38+ messages in thread
From: Corey Minyard @ 2018-11-20 16:58 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Paolo Bonzini, Corey Minyard,
Dr . David Alan Gilbert, Michael S . Tsirkin
On 11/20/18 9:33 AM, Peter Maydell wrote:
> On 15 November 2018 at 19:24, <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Avoid an overflow.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
>> hw/i2c/smbus_slave.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
>> index 83ca041b5d..fa988919d8 100644
>> --- a/hw/i2c/smbus_slave.c
>> +++ b/hw/i2c/smbus_slave.c
>> @@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
>> switch (dev->mode) {
>> case SMBUS_WRITE_DATA:
>> DPRINTF("Write data %02x\n", data);
>> - dev->data_buf[dev->data_len++] = data;
>> + if (dev->data_len >= sizeof(dev->data_buf)) {
>> + BADF("Too many bytes sent\n");
>> + } else {
>> + dev->data_buf[dev->data_len++] = data;
>> + }
>> break;
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> What happens on a real device in this situation ?
It's device specific. Some devices (most eeproms, I suspect) will just keep
taking data, wrapping around when you hit the end of the memory. Some
devices (IPMI BMCs, generally) will ignore the extra data. Some devices
may return a NAK on a byte if it gets too much data. The specification
says:
The slave device detects an invalid command or invalid data. In this
case the slave
device must NACK the received byte. The master upon detection of
this condition
must generate a STOP condition and retry the transaction.
So a NAK may be appropriate here, but it's kind of fuzzy. Since generating
a NAK is more complicated, I would guess that most devices don't.
-corey
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 05/12] i2c: Fix pm_smbus handling of I2C block read
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (3 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 04/12] i2c: Add a length check to the SMBus write handling minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines minyard
` (7 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
The I2C block read function of pm_smbus was completely broken. It
required doing some direct I2C handling because it didn't have a
defined size, the OS code just reads bytes until it marks the
transaction finished.
This also required adjusting how the AMIBIOS workaround code worked,
the I2C block mode was setting STS_HOST_BUSY during a transaction,
so that bit could no longer be used to inform the host status read
code to start the transaction. Create a explicit bool for that
operation.
Also, don't read the next byte from the device in byte-by-byte
mode unless the OS is actually clearing the byte done bit. Just
assuming that's what the OS is doing is a bad idea.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/i2c/pm_smbus.c | 86 ++++++++++++++++++++++++++++++---------
include/hw/i2c/pm_smbus.h | 6 +++
2 files changed, 73 insertions(+), 19 deletions(-)
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index f3c6cc46f9..8793113c25 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -118,19 +118,30 @@ static void smb_transaction(PMSMBus *s)
}
break;
case PROT_I2C_BLOCK_READ:
- if (read) {
- int xfersize = s->smb_data0;
- if (xfersize > sizeof(s->smb_data)) {
- xfersize = sizeof(s->smb_data);
- }
- ret = smbus_read_block(bus, addr, s->smb_data1, s->smb_data,
- xfersize, false, true);
- goto data8;
- } else {
- /* The manual says the behavior is undefined, just set DEV_ERR. */
+ /* According to the Linux i2c-i801 driver:
+ * NB: page 240 of ICH5 datasheet shows that the R/#W
+ * bit should be cleared here, even when reading.
+ * However if SPD Write Disable is set (Lynx Point and later),
+ * the read will fail if we don't set the R/#W bit.
+ * So at least Linux may or may not set the read bit here.
+ * So just ignore the read bit for this command.
+ */
+ if (i2c_start_transfer(bus, addr, 0)) {
goto error;
}
- break;
+ ret = i2c_send(bus, s->smb_data1);
+ if (ret) {
+ goto error;
+ }
+ if (i2c_start_transfer(bus, addr, 1)) {
+ goto error;
+ }
+ s->in_i2c_block_read = true;
+ s->smb_blkdata = i2c_recv(s->smbus);
+ s->op_done = false;
+ s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+ goto out;
+
case PROT_BLOCK_DATA:
if (read) {
ret = smbus_read_block(bus, addr, cmd, s->smb_data,
@@ -208,6 +219,7 @@ static void smb_transaction_start(PMSMBus *s)
{
if (s->smb_ctl & CTL_INTREN) {
smb_transaction(s);
+ s->start_transaction_on_status_read = false;
} else {
/* Do not execute immediately the command; it will be
* executed when guest will read SMB_STAT register. This
@@ -217,6 +229,7 @@ static void smb_transaction_start(PMSMBus *s)
* checking for status. If STS_HOST_BUSY doesn't get
* set, it gets stuck. */
s->smb_stat |= STS_HOST_BUSY;
+ s->start_transaction_on_status_read = true;
}
}
@@ -226,19 +239,38 @@ smb_irq_value(PMSMBus *s)
return ((s->smb_stat & ~STS_HOST_BUSY) != 0) && (s->smb_ctl & CTL_INTREN);
}
+static bool
+smb_byte_by_byte(PMSMBus *s)
+{
+ if (s->op_done) {
+ return false;
+ }
+ if (s->in_i2c_block_read) {
+ return true;
+ }
+ return !(s->smb_auxctl & AUX_BLK);
+}
+
static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
unsigned width)
{
PMSMBus *s = opaque;
+ uint8_t clear_byte_done;
SMBUS_DPRINTF("SMB writeb port=0x%04" HWADDR_PRIx
" val=0x%02" PRIx64 "\n", addr, val);
switch(addr) {
case SMBHSTSTS:
+ clear_byte_done = s->smb_stat & val & STS_BYTE_DONE;
s->smb_stat &= ~(val & ~STS_HOST_BUSY);
- if (!s->op_done && !(s->smb_auxctl & AUX_BLK)) {
+ if (clear_byte_done && smb_byte_by_byte(s)) {
uint8_t read = s->smb_addr & 0x01;
+ if (s->in_i2c_block_read) {
+ /* See comment below PROT_I2C_BLOCK_READ above. */
+ read = 1;
+ }
+
s->smb_index++;
if (!read && s->smb_index == s->smb_data0) {
uint8_t prot = (s->smb_ctl >> 2) & 0x07;
@@ -265,12 +297,23 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
s->smb_stat |= STS_BYTE_DONE;
} else if (s->smb_ctl & CTL_LAST_BYTE) {
s->op_done = true;
- s->smb_blkdata = s->smb_data[s->smb_index];
+ if (s->in_i2c_block_read) {
+ s->in_i2c_block_read = false;
+ s->smb_blkdata = i2c_recv(s->smbus);
+ i2c_nack(s->smbus);
+ i2c_end_transfer(s->smbus);
+ } else {
+ s->smb_blkdata = s->smb_data[s->smb_index];
+ }
s->smb_index = 0;
s->smb_stat |= STS_INTR;
s->smb_stat &= ~STS_HOST_BUSY;
} else {
- s->smb_blkdata = s->smb_data[s->smb_index];
+ if (s->in_i2c_block_read) {
+ s->smb_blkdata = i2c_recv(s->smbus);
+ } else {
+ s->smb_blkdata = s->smb_data[s->smb_index];
+ }
s->smb_stat |= STS_BYTE_DONE;
}
}
@@ -281,6 +324,10 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
if (!s->op_done) {
s->smb_index = 0;
s->op_done = true;
+ if (s->in_i2c_block_read) {
+ s->in_i2c_block_read = false;
+ i2c_end_transfer(s->smbus);
+ }
}
smb_transaction_start(s);
}
@@ -334,8 +381,9 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
switch(addr) {
case SMBHSTSTS:
val = s->smb_stat;
- if (s->smb_stat & STS_HOST_BUSY) {
+ if (s->start_transaction_on_status_read) {
/* execute command now */
+ s->start_transaction_on_status_read = false;
s->smb_stat &= ~STS_HOST_BUSY;
smb_transaction(s);
}
@@ -356,10 +404,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
val = s->smb_data1;
break;
case SMBBLKDAT:
- if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
- s->smb_index = 0;
- }
- if (s->smb_auxctl & AUX_BLK) {
+ if (s->smb_auxctl & AUX_BLK && !s->in_i2c_block_read) {
+ if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
+ s->smb_index = 0;
+ }
val = s->smb_data[s->smb_index++];
if (!s->op_done && s->smb_index == s->smb_data0) {
s->op_done = true;
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 6dd5b7040b..7bcca97672 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,6 +33,12 @@ typedef struct PMSMBus {
/* Set on block transfers after the last byte has been read, so the
INTR bit can be set at the right time. */
bool op_done;
+
+ /* Set during an I2C block read, so we know how to handle data. */
+ bool in_i2c_block_read;
+
+ /* Used to work around a bug in AMIBIOS, see smb_transaction_start() */
+ bool start_transaction_on_status_read;
} PMSMBus;
void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (4 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 05/12] i2c: Fix pm_smbus handling of I2C block read minyard
@ 2018-11-15 19:24 ` minyard
2018-11-26 17:23 ` Dr. David Alan Gilbert
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer minyard
` (6 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard, Eduardo Habkost, Marcel Apfelbaum
From: Corey Minyard <cminyard@mvista.com>
Migration capability is being added for pm_smbus and SMBus devices.
This change will allow backwards compatibility to be kept when
migrating back to an old qemu version. Add a bool to the machine
class tho keep smbus migration from happening. Future changes
will use this.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
hw/i386/pc_piix.c | 1 +
hw/i386/pc_q35.c | 1 +
include/hw/boards.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index cb28227cc3..3d1ccb1af1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
pc_i440fx_3_0_machine_options(m);
m->is_default = 0;
m->alias = NULL;
+ m->smbus_no_migration_support = true;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 90e88c9b28..0c6fca6a40 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
{
pc_q35_3_0_machine_options(m);
m->alias = NULL;
+ m->smbus_no_migration_support = true;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index f82f28468b..65314fbe2a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -207,6 +207,7 @@ struct MachineClass {
void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
int nb_nodes, ram_addr_t size);
bool ignore_boot_device_suffixes;
+ bool smbus_no_migration_support;
HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
DeviceState *dev);
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines minyard
@ 2018-11-26 17:23 ` Dr. David Alan Gilbert
2018-11-26 18:22 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-26 17:23 UTC (permalink / raw)
To: minyard
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> Migration capability is being added for pm_smbus and SMBus devices.
> This change will allow backwards compatibility to be kept when
> migrating back to an old qemu version. Add a bool to the machine
> class tho keep smbus migration from happening. Future changes
> will use this.
So this is also going to have to be in the 3.0 and 3.1 machine options
and maybe some other architectures?
Dave
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> ---
> hw/i386/pc_piix.c | 1 +
> hw/i386/pc_q35.c | 1 +
> include/hw/boards.h | 1 +
> 3 files changed, 3 insertions(+)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index cb28227cc3..3d1ccb1af1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
> pc_i440fx_3_0_machine_options(m);
> m->is_default = 0;
> m->alias = NULL;
> + m->smbus_no_migration_support = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 90e88c9b28..0c6fca6a40 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
> {
> pc_q35_3_0_machine_options(m);
> m->alias = NULL;
> + m->smbus_no_migration_support = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> }
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index f82f28468b..65314fbe2a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -207,6 +207,7 @@ struct MachineClass {
> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
> int nb_nodes, ram_addr_t size);
> bool ignore_boot_device_suffixes;
> + bool smbus_no_migration_support;
>
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-26 17:23 ` Dr. David Alan Gilbert
@ 2018-11-26 18:22 ` Corey Minyard
2018-11-27 16:01 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-26 18:22 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Migration capability is being added for pm_smbus and SMBus devices.
>> This change will allow backwards compatibility to be kept when
>> migrating back to an old qemu version. Add a bool to the machine
>> class tho keep smbus migration from happening. Future changes
>> will use this.
> So this is also going to have to be in the 3.0 and 3.1 machine options
> and maybe some other architectures?
I'm not sure why it would need to be in both the 3.0 and 3.1 machine
options.
If this goes in, I'm assuming it's for 3.1 and it should only need to be
there.
That's if it's too late for 3.0.
The pm_smbus device is also used by mips fulong. From what I can tell, that
machine doesn't have any concept of backwards compatibility, so it seemed
save to no worry about it there. If it weren't for that, this bool
could go into the
PC specific structure.
Thanks,
-corey
> Dave
>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> ---
>> hw/i386/pc_piix.c | 1 +
>> hw/i386/pc_q35.c | 1 +
>> include/hw/boards.h | 1 +
>> 3 files changed, 3 insertions(+)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index cb28227cc3..3d1ccb1af1 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
>> pc_i440fx_3_0_machine_options(m);
>> m->is_default = 0;
>> m->alias = NULL;
>> + m->smbus_no_migration_support = true;
>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>> }
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 90e88c9b28..0c6fca6a40 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
>> {
>> pc_q35_3_0_machine_options(m);
>> m->alias = NULL;
>> + m->smbus_no_migration_support = true;
>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>> }
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index f82f28468b..65314fbe2a 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -207,6 +207,7 @@ struct MachineClass {
>> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>> int nb_nodes, ram_addr_t size);
>> bool ignore_boot_device_suffixes;
>> + bool smbus_no_migration_support;
>>
>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>> DeviceState *dev);
>> --
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-26 18:22 ` Corey Minyard
@ 2018-11-27 16:01 ` Dr. David Alan Gilbert
2018-11-27 16:59 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-27 16:01 UTC (permalink / raw)
To: Corey Minyard
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
* Corey Minyard (minyard@acm.org) wrote:
> On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
> > * minyard@acm.org (minyard@acm.org) wrote:
> > > From: Corey Minyard <cminyard@mvista.com>
> > >
> > > Migration capability is being added for pm_smbus and SMBus devices.
> > > This change will allow backwards compatibility to be kept when
> > > migrating back to an old qemu version. Add a bool to the machine
> > > class tho keep smbus migration from happening. Future changes
> > > will use this.
> > So this is also going to have to be in the 3.0 and 3.1 machine options
> > and maybe some other architectures?
>
> I'm not sure why it would need to be in both the 3.0 and 3.1 machine
> options.
> If this goes in, I'm assuming it's for 3.1 and it should only need to be
> there.
> That's if it's too late for 3.0.
You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at
the moment (hence you've missed it unless some part of this is
critical); so this will go in during the 4.0 release.
> The pm_smbus device is also used by mips fulong. From what I can tell, that
> machine doesn't have any concept of backwards compatibility, so it seemed
> save to no worry about it there. If it weren't for that, this bool could go
> into the
Yeh that's fine.
What about the eeprom's are they used more widely?
Dave
> PC specific structure.
>
> Thanks,
>
> -corey
>
> > Dave
> >
> > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > ---
> > > hw/i386/pc_piix.c | 1 +
> > > hw/i386/pc_q35.c | 1 +
> > > include/hw/boards.h | 1 +
> > > 3 files changed, 3 insertions(+)
> > >
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index cb28227cc3..3d1ccb1af1 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
> > > pc_i440fx_3_0_machine_options(m);
> > > m->is_default = 0;
> > > m->alias = NULL;
> > > + m->smbus_no_migration_support = true;
> > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > > }
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index 90e88c9b28..0c6fca6a40 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
> > > {
> > > pc_q35_3_0_machine_options(m);
> > > m->alias = NULL;
> > > + m->smbus_no_migration_support = true;
> > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > > }
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index f82f28468b..65314fbe2a 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -207,6 +207,7 @@ struct MachineClass {
> > > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
> > > int nb_nodes, ram_addr_t size);
> > > bool ignore_boot_device_suffixes;
> > > + bool smbus_no_migration_support;
> > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > DeviceState *dev);
> > > --
> > > 2.17.1
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-27 16:01 ` Dr. David Alan Gilbert
@ 2018-11-27 16:59 ` Corey Minyard
2018-11-27 18:14 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-27 16:59 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote:
> * Corey Minyard (minyard@acm.org) wrote:
>> On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
>>> * minyard@acm.org (minyard@acm.org) wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> Migration capability is being added for pm_smbus and SMBus devices.
>>>> This change will allow backwards compatibility to be kept when
>>>> migrating back to an old qemu version. Add a bool to the machine
>>>> class tho keep smbus migration from happening. Future changes
>>>> will use this.
>>> So this is also going to have to be in the 3.0 and 3.1 machine options
>>> and maybe some other architectures?
>> I'm not sure why it would need to be in both the 3.0 and 3.1 machine
>> options.
>> If this goes in, I'm assuming it's for 3.1 and it should only need to be
>> there.
>> That's if it's too late for 3.0.
> You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at
> the moment (hence you've missed it unless some part of this is
> critical); so this will go in during the 4.0 release.
Yes, you are right, this is for 4.0.
>
>> The pm_smbus device is also used by mips fulong. From what I can tell, that
>> machine doesn't have any concept of backwards compatibility, so it seemed
>> save to no worry about it there. If it weren't for that, this bool could go
>> into the
> Yeh that's fine.
>
> What about the eeprom's are they used more widely?
The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device
is only
configured for PPC. It has a block device backend, too. This is a
little annoying
because smbus_eeprom.c emulates an at24c02. It would be nice to only have
one of these.
I can add it to that file, too, it wouldn't be hard to do and I could
put it into the
x86 config to test. It would be nicer to combine the two; smbus_eeprom
seems
like a hack to me. at24c.c is done as an SMBus slave, its done as a raw I2C
device, which is IMHO is more correct since the at24cxx devices are not
SMBus
devices, they are I2C devices.
In fact, smbus_eeprom.c is the only thing currently using the
smbus_slave code.
The IPMI interface on SMBus that I have pending uses the smbus_slave
code, too,
so perhaps there's value, though it's no big deal either way. It would
be nice to get
this right the first time and not hack it up in the future.
Thanks,
-corey
>
> Dave
>
>> PC specific structure.
>>
>> Thanks,
>>
>> -corey
>>
>>> Dave
>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>>> ---
>>>> hw/i386/pc_piix.c | 1 +
>>>> hw/i386/pc_q35.c | 1 +
>>>> include/hw/boards.h | 1 +
>>>> 3 files changed, 3 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index cb28227cc3..3d1ccb1af1 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
>>>> pc_i440fx_3_0_machine_options(m);
>>>> m->is_default = 0;
>>>> m->alias = NULL;
>>>> + m->smbus_no_migration_support = true;
>>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>>>> }
>>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>>> index 90e88c9b28..0c6fca6a40 100644
>>>> --- a/hw/i386/pc_q35.c
>>>> +++ b/hw/i386/pc_q35.c
>>>> @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
>>>> {
>>>> pc_q35_3_0_machine_options(m);
>>>> m->alias = NULL;
>>>> + m->smbus_no_migration_support = true;
>>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
>>>> }
>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>> index f82f28468b..65314fbe2a 100644
>>>> --- a/include/hw/boards.h
>>>> +++ b/include/hw/boards.h
>>>> @@ -207,6 +207,7 @@ struct MachineClass {
>>>> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>>>> int nb_nodes, ram_addr_t size);
>>>> bool ignore_boot_device_suffixes;
>>>> + bool smbus_no_migration_support;
>>>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>>> DeviceState *dev);
>>>> --
>>>> 2.17.1
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-27 16:59 ` Corey Minyard
@ 2018-11-27 18:14 ` Dr. David Alan Gilbert
2018-11-27 18:41 ` Laurent Vivier
0 siblings, 1 reply; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-27 18:14 UTC (permalink / raw)
To: Corey Minyard, lvivier, dgibson
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
* Corey Minyard (minyard@acm.org) wrote:
> On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote:
> > * Corey Minyard (minyard@acm.org) wrote:
> > > On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
> > > > * minyard@acm.org (minyard@acm.org) wrote:
> > > > > From: Corey Minyard <cminyard@mvista.com>
> > > > >
> > > > > Migration capability is being added for pm_smbus and SMBus devices.
> > > > > This change will allow backwards compatibility to be kept when
> > > > > migrating back to an old qemu version. Add a bool to the machine
> > > > > class tho keep smbus migration from happening. Future changes
> > > > > will use this.
> > > > So this is also going to have to be in the 3.0 and 3.1 machine options
> > > > and maybe some other architectures?
> > > I'm not sure why it would need to be in both the 3.0 and 3.1 machine
> > > options.
> > > If this goes in, I'm assuming it's for 3.1 and it should only need to be
> > > there.
> > > That's if it's too late for 3.0.
> > You're out by 1-2 releases; 3.0 shipped in August, 3.1 is in freeze at
> > the moment (hence you've missed it unless some part of this is
> > critical); so this will go in during the 4.0 release.
>
> Yes, you are right, this is for 4.0.
>
>
> >
> > > The pm_smbus device is also used by mips fulong. From what I can tell, that
> > > machine doesn't have any concept of backwards compatibility, so it seemed
> > > save to no worry about it there. If it weren't for that, this bool could go
> > > into the
> > Yeh that's fine.
> >
> > What about the eeprom's are they used more widely?
>
> The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device is
> only
> configured for PPC. It has a block device backend, too. This is a little
> annoying
> because smbus_eeprom.c emulates an at24c02. It would be nice to only have
> one of these.
Added in David and Laurent for odd-ppc stuff; if I'm reading it right
it's not used by one of the versioned machines.
> I can add it to that file, too, it wouldn't be hard to do and I could put it
> into the
> x86 config to test. It would be nicer to combine the two; smbus_eeprom
> seems
> like a hack to me. at24c.c is done as an SMBus slave, its done as a raw I2C
> device, which is IMHO is more correct since the at24cxx devices are not
> SMBus
> devices, they are I2C devices.
>
> In fact, smbus_eeprom.c is the only thing currently using the smbus_slave
> code.
> The IPMI interface on SMBus that I have pending uses the smbus_slave code,
> too,
> so perhaps there's value, though it's no big deal either way. It would be
> nice to get
> this right the first time and not hack it up in the future.
Dave
>
> Thanks,
>
> -corey
>
>
> >
> > Dave
> >
> > > PC specific structure.
> > >
> > > Thanks,
> > >
> > > -corey
> > >
> > > > Dave
> > > >
> > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > > ---
> > > > > hw/i386/pc_piix.c | 1 +
> > > > > hw/i386/pc_q35.c | 1 +
> > > > > include/hw/boards.h | 1 +
> > > > > 3 files changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index cb28227cc3..3d1ccb1af1 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -443,6 +443,7 @@ static void pc_i440fx_2_12_machine_options(MachineClass *m)
> > > > > pc_i440fx_3_0_machine_options(m);
> > > > > m->is_default = 0;
> > > > > m->alias = NULL;
> > > > > + m->smbus_no_migration_support = true;
> > > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > > > > }
> > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > > index 90e88c9b28..0c6fca6a40 100644
> > > > > --- a/hw/i386/pc_q35.c
> > > > > +++ b/hw/i386/pc_q35.c
> > > > > @@ -324,6 +324,7 @@ static void pc_q35_2_12_machine_options(MachineClass *m)
> > > > > {
> > > > > pc_q35_3_0_machine_options(m);
> > > > > m->alias = NULL;
> > > > > + m->smbus_no_migration_support = true;
> > > > > SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> > > > > }
> > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > > index f82f28468b..65314fbe2a 100644
> > > > > --- a/include/hw/boards.h
> > > > > +++ b/include/hw/boards.h
> > > > > @@ -207,6 +207,7 @@ struct MachineClass {
> > > > > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
> > > > > int nb_nodes, ram_addr_t size);
> > > > > bool ignore_boot_device_suffixes;
> > > > > + bool smbus_no_migration_support;
> > > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > > > DeviceState *dev);
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines
2018-11-27 18:14 ` Dr. David Alan Gilbert
@ 2018-11-27 18:41 ` Laurent Vivier
0 siblings, 0 replies; 38+ messages in thread
From: Laurent Vivier @ 2018-11-27 18:41 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Corey Minyard, dgibson
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Eduardo Habkost, Marcel Apfelbaum
On 27/11/2018 19:14, Dr. David Alan Gilbert wrote:
> * Corey Minyard (minyard@acm.org) wrote:
>> On 11/27/18 10:01 AM, Dr. David Alan Gilbert wrote:
>>> * Corey Minyard (minyard@acm.org) wrote:
>>>> On 11/26/18 11:23 AM, Dr. David Alan Gilbert wrote:
>>>>> * minyard@acm.org (minyard@acm.org) wrote:
>>>>>> From: Corey Minyard <cminyard@mvista.com>
...
>>>> The pm_smbus device is also used by mips fulong. From what I can tell, that
>>>> machine doesn't have any concept of backwards compatibility, so it seemed
>>>> save to no worry about it there. If it weren't for that, this bool could go
>>>> into the
>>> Yeh that's fine.
>>>
>>> What about the eeprom's are they used more widely?
>>
>> The only other I2C EEPROM I see is hw/nvram/eeprom_at24c.c. That device is
>> only
>> configured for PPC. It has a block device backend, too. This is a little
>> annoying
>> because smbus_eeprom.c emulates an at24c02. It would be nice to only have
>> one of these.
>
> Added in David and Laurent for odd-ppc stuff; if I'm reading it right
> it's not used by one of the versioned machines.
Yes, it seems unused.
It has been introduced for the mvme31100 machine emulation, by:
commit 5d8424dbd3e8335ea3d57f64eaa603c8fc80706f
Author: Michael Davidsaver <mdavidsaver@gmail.com>
Date: Sun Nov 19 21:24:17 2017 -0600
nvram: add AT24Cx i2c eeprom
Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
from the series:
[00/12] Add MVME3100 PPC SBC
https://patchwork.ozlabs.org/cover/839400/
[12/12] tests: add mvme3100-test
[11/12] ppc: add mvme3100 machine
[10/12] timer: add ds1375 RTC
[09/12] nvram: add AT24Cx i2c eeprom
[08/12] e500: add mpc8540 i2c controller to ccsr
[07/12] qtest: add e500_i2c_create()
[06/12] i2c: add mpc8540 i2c controller
[05/12] e500: name openpic and pci host bridge
[04/12] e500: additional CCSR registers
[03/12] e500: note possible bug with host bridge
[02/12] e500: consolidate mpc8540 guts with e500-ccsr
[01/12] e500: add board config options
https://patchwork.ozlabs.org/project/qemu-devel/list/?series=14303
But the patch adding the machine type has never been commited:
[11/12] ppc: add mvme3100 machine
https://patchwork.ozlabs.org/patch/839407/
Thanks,
Laurent
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (5 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 06/12] boards.h: Ignore migration for SMBus devices on older machines minyard
@ 2018-11-15 19:24 ` minyard
2018-11-26 17:20 ` Dr. David Alan Gilbert
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 08/12] i2c: Add an SMBus vmstate structure minyard
` (5 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Transfer the state information for the SMBus registers and
internal data so it will work on a VM transfer.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/acpi/piix4.c | 3 ++-
hw/i2c/pm_smbus.c | 31 +++++++++++++++++++++++++++++++
hw/i2c/smbus_ich9.c | 6 ++++--
include/hw/i2c/pm_smbus.h | 2 ++
4 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index e330f24c71..313305f5a0 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
*/
static const VMStateDescription vmstate_acpi = {
.name = "piix4_pm",
- .version_id = 3,
+ .version_id = 4,
.minimum_version_id = 3,
.minimum_version_id_old = 1,
.load_state_old = acpi_load_old,
@@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
+ VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 8793113c25..75907e1c22 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
#include "hw/hw.h"
+#include "hw/boards.h"
#include "hw/i2c/pm_smbus.h"
#include "hw/i2c/smbus_master.h"
@@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static bool pm_smbus_vmstate_needed(void *opaque)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+ return !mc->smbus_no_migration_support;
+}
+
+const VMStateDescription pmsmb_vmstate = {
+ .name = "pmsmb",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = pm_smbus_vmstate_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(smb_stat, PMSMBus),
+ VMSTATE_UINT8(smb_ctl, PMSMBus),
+ VMSTATE_UINT8(smb_cmd, PMSMBus),
+ VMSTATE_UINT8(smb_addr, PMSMBus),
+ VMSTATE_UINT8(smb_data0, PMSMBus),
+ VMSTATE_UINT8(smb_data1, PMSMBus),
+ VMSTATE_UINT32(smb_index, PMSMBus),
+ VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
+ VMSTATE_UINT8(smb_auxctl, PMSMBus),
+ VMSTATE_BOOL(i2c_enable, PMSMBus),
+ VMSTATE_BOOL(op_done, PMSMBus),
+ VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
+ VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
{
smb->op_done = true;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index e6d8d28194..c9b7482a54 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -45,10 +45,12 @@ typedef struct ICH9SMBState {
static const VMStateDescription vmstate_ich9_smbus = {
.name = "ich9_smb",
- .version_id = 1,
+ .version_id = 2,
.minimum_version_id = 1,
.fields = (VMStateField[]) {
- VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+ VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+ VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 2),
+ VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
VMSTATE_END_OF_LIST()
}
};
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 7bcca97672..5075fc64fa 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -43,4 +43,6 @@ typedef struct PMSMBus {
void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
+extern const VMStateDescription pmsmb_vmstate;
+
#endif /* PM_SMBUS_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer minyard
@ 2018-11-26 17:20 ` Dr. David Alan Gilbert
2018-11-26 18:24 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-26 17:20 UTC (permalink / raw)
To: minyard; +Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard
* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> Transfer the state information for the SMBus registers and
> internal data so it will work on a VM transfer.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/acpi/piix4.c | 3 ++-
> hw/i2c/pm_smbus.c | 31 +++++++++++++++++++++++++++++++
> hw/i2c/smbus_ich9.c | 6 ++++--
> include/hw/i2c/pm_smbus.h | 2 ++
> 4 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e330f24c71..313305f5a0 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
> */
> static const VMStateDescription vmstate_acpi = {
> .name = "piix4_pm",
> - .version_id = 3,
> + .version_id = 4,
OK, so do we need to bump this version ? Ideally you'd keep the version
as is and let the needed do the work.
> .minimum_version_id = 3,
> .minimum_version_id_old = 1,
> .load_state_old = acpi_load_old,
> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
> VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
> VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
> VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> + VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
> VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
> VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
> VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 8793113c25..75907e1c22 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -19,6 +19,7 @@
> */
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> +#include "hw/boards.h"
> #include "hw/i2c/pm_smbus.h"
> #include "hw/i2c/smbus_master.h"
>
> @@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +static bool pm_smbus_vmstate_needed(void *opaque)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +
> + return !mc->smbus_no_migration_support;
> +}
OK
> +const VMStateDescription pmsmb_vmstate = {
> + .name = "pmsmb",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = pm_smbus_vmstate_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(smb_stat, PMSMBus),
> + VMSTATE_UINT8(smb_ctl, PMSMBus),
> + VMSTATE_UINT8(smb_cmd, PMSMBus),
> + VMSTATE_UINT8(smb_addr, PMSMBus),
> + VMSTATE_UINT8(smb_data0, PMSMBus),
> + VMSTATE_UINT8(smb_data1, PMSMBus),
> + VMSTATE_UINT32(smb_index, PMSMBus),
> + VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> + VMSTATE_UINT8(smb_auxctl, PMSMBus),
> + VMSTATE_BOOL(i2c_enable, PMSMBus),
> + VMSTATE_BOOL(op_done, PMSMBus),
> + VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
> + VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
> {
> smb->op_done = true;
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index e6d8d28194..c9b7482a54 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -45,10 +45,12 @@ typedef struct ICH9SMBState {
>
> static const VMStateDescription vmstate_ich9_smbus = {
> .name = "ich9_smb",
> - .version_id = 1,
> + .version_id = 2,
Again, do we need to bump this?
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> - VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> + VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> + VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 2),
Can we make this a VMSTATE_BOOL_TEST (see VMSTATE_UINT64_TEST for the
pattern) tied to the same .neede, and again we don't need to bump the
version.
> + VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
So that's taken care of by the .needed.
Dave
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 7bcca97672..5075fc64fa 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -43,4 +43,6 @@ typedef struct PMSMBus {
>
> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>
> +extern const VMStateDescription pmsmb_vmstate;
> +
> #endif /* PM_SMBUS_H */
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer
2018-11-26 17:20 ` Dr. David Alan Gilbert
@ 2018-11-26 18:24 ` Corey Minyard
2018-11-26 19:41 ` Corey Minyard
0 siblings, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-26 18:24 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard
On 11/26/18 11:20 AM, Dr. David Alan Gilbert wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Transfer the state information for the SMBus registers and
>> internal data so it will work on a VM transfer.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>> hw/acpi/piix4.c | 3 ++-
>> hw/i2c/pm_smbus.c | 31 +++++++++++++++++++++++++++++++
>> hw/i2c/smbus_ich9.c | 6 ++++--
>> include/hw/i2c/pm_smbus.h | 2 ++
>> 4 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index e330f24c71..313305f5a0 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
>> */
>> static const VMStateDescription vmstate_acpi = {
>> .name = "piix4_pm",
>> - .version_id = 3,
>> + .version_id = 4,
> OK, so do we need to bump this version ? Ideally you'd keep the version
> as is and let the needed do the work.
Got it, that makes sense. Same for the comments below, I'll get all those.
Thanks,
-corey
>> .minimum_version_id = 3,
>> .minimum_version_id_old = 1,
>> .load_state_old = acpi_load_old,
>> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
>> VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>> VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>> VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
>> + VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
>> VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>> VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>> VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>> index 8793113c25..75907e1c22 100644
>> --- a/hw/i2c/pm_smbus.c
>> +++ b/hw/i2c/pm_smbus.c
>> @@ -19,6 +19,7 @@
>> */
>> #include "qemu/osdep.h"
>> #include "hw/hw.h"
>> +#include "hw/boards.h"
>> #include "hw/i2c/pm_smbus.h"
>> #include "hw/i2c/smbus_master.h"
>>
>> @@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> +static bool pm_smbus_vmstate_needed(void *opaque)
>> +{
>> + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> +
>> + return !mc->smbus_no_migration_support;
>> +}
> OK
>
>> +const VMStateDescription pmsmb_vmstate = {
>> + .name = "pmsmb",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = pm_smbus_vmstate_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT8(smb_stat, PMSMBus),
>> + VMSTATE_UINT8(smb_ctl, PMSMBus),
>> + VMSTATE_UINT8(smb_cmd, PMSMBus),
>> + VMSTATE_UINT8(smb_addr, PMSMBus),
>> + VMSTATE_UINT8(smb_data0, PMSMBus),
>> + VMSTATE_UINT8(smb_data1, PMSMBus),
>> + VMSTATE_UINT32(smb_index, PMSMBus),
>> + VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
>> + VMSTATE_UINT8(smb_auxctl, PMSMBus),
>> + VMSTATE_BOOL(i2c_enable, PMSMBus),
>> + VMSTATE_BOOL(op_done, PMSMBus),
>> + VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
>> + VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
>> {
>> smb->op_done = true;
>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>> index e6d8d28194..c9b7482a54 100644
>> --- a/hw/i2c/smbus_ich9.c
>> +++ b/hw/i2c/smbus_ich9.c
>> @@ -45,10 +45,12 @@ typedef struct ICH9SMBState {
>>
>> static const VMStateDescription vmstate_ich9_smbus = {
>> .name = "ich9_smb",
>> - .version_id = 1,
>> + .version_id = 2,
> Again, do we need to bump this?
>
>> .minimum_version_id = 1,
>> .fields = (VMStateField[]) {
>> - VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
>> + VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
>> + VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 2),
> Can we make this a VMSTATE_BOOL_TEST (see VMSTATE_UINT64_TEST for the
> pattern) tied to the same .neede, and again we don't need to bump the
> version.
>
>> + VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
> So that's taken care of by the .needed.
>
> Dave
>
>> VMSTATE_END_OF_LIST()
>> }
>> };
>> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
>> index 7bcca97672..5075fc64fa 100644
>> --- a/include/hw/i2c/pm_smbus.h
>> +++ b/include/hw/i2c/pm_smbus.h
>> @@ -43,4 +43,6 @@ typedef struct PMSMBus {
>>
>> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>>
>> +extern const VMStateDescription pmsmb_vmstate;
>> +
>> #endif /* PM_SMBUS_H */
>> --
>> 2.17.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer
2018-11-26 18:24 ` Corey Minyard
@ 2018-11-26 19:41 ` Corey Minyard
2018-11-27 18:20 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-26 19:41 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard
On 11/26/18 12:24 PM, Corey Minyard wrote:
> On 11/26/18 11:20 AM, Dr. David Alan Gilbert wrote:
>> * minyard@acm.org (minyard@acm.org) wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Transfer the state information for the SMBus registers and
>>> internal data so it will work on a VM transfer.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>> hw/acpi/piix4.c | 3 ++-
>>> hw/i2c/pm_smbus.c | 31 +++++++++++++++++++++++++++++++
>>> hw/i2c/smbus_ich9.c | 6 ++++--
>>> include/hw/i2c/pm_smbus.h | 2 ++
>>> 4 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>> index e330f24c71..313305f5a0 100644
>>> --- a/hw/acpi/piix4.c
>>> +++ b/hw/acpi/piix4.c
>>> @@ -309,7 +309,7 @@ static const VMStateDescription
>>> vmstate_cpuhp_state = {
>>> */
>>> static const VMStateDescription vmstate_acpi = {
>>> .name = "piix4_pm",
>>> - .version_id = 3,
>>> + .version_id = 4,
>> OK, so do we need to bump this version ? Ideally you'd keep the version
>> as is and let the needed do the work.
>
>
> Got it, that makes sense. Same for the comments below, I'll get all
> those.
>
Well, maybe not. the .needed function is only called on the save side,
it is
not called on the load side So a 2.12 to 3.0 transfer fails. So I've
exported
the migration needed function and I'm using the VMSTATE_STRUCT_TEST()
function to transfer it in each case. With that I can migrate from 2.12 to
3.0 and back without issue.
-corey
> Thanks,
>
> -corey
>
>
>>> .minimum_version_id = 3,
>>> .minimum_version_id_old = 1,
>>> .load_state_old = acpi_load_old,
>>> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
>>> VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>>> VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>>> VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
>>> + VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
>>> VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>>> VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>>> VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe,
>>> ACPIGPE),
>>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>>> index 8793113c25..75907e1c22 100644
>>> --- a/hw/i2c/pm_smbus.c
>>> +++ b/hw/i2c/pm_smbus.c
>>> @@ -19,6 +19,7 @@
>>> */
>>> #include "qemu/osdep.h"
>>> #include "hw/hw.h"
>>> +#include "hw/boards.h"
>>> #include "hw/i2c/pm_smbus.h"
>>> #include "hw/i2c/smbus_master.h"
>>> @@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>> +static bool pm_smbus_vmstate_needed(void *opaque)
>>> +{
>>> + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>>> +
>>> + return !mc->smbus_no_migration_support;
>>> +}
>> OK
>>
>>> +const VMStateDescription pmsmb_vmstate = {
>>> + .name = "pmsmb",
>>> + .version_id = 1,
>>> + .minimum_version_id = 1,
>>> + .needed = pm_smbus_vmstate_needed,
>>> + .fields = (VMStateField[]) {
>>> + VMSTATE_UINT8(smb_stat, PMSMBus),
>>> + VMSTATE_UINT8(smb_ctl, PMSMBus),
>>> + VMSTATE_UINT8(smb_cmd, PMSMBus),
>>> + VMSTATE_UINT8(smb_addr, PMSMBus),
>>> + VMSTATE_UINT8(smb_data0, PMSMBus),
>>> + VMSTATE_UINT8(smb_data1, PMSMBus),
>>> + VMSTATE_UINT32(smb_index, PMSMBus),
>>> + VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
>>> + VMSTATE_UINT8(smb_auxctl, PMSMBus),
>>> + VMSTATE_BOOL(i2c_enable, PMSMBus),
>>> + VMSTATE_BOOL(op_done, PMSMBus),
>>> + VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
>>> + VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> +};
>>> +
>>> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool
>>> force_aux_blk)
>>> {
>>> smb->op_done = true;
>>> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
>>> index e6d8d28194..c9b7482a54 100644
>>> --- a/hw/i2c/smbus_ich9.c
>>> +++ b/hw/i2c/smbus_ich9.c
>>> @@ -45,10 +45,12 @@ typedef struct ICH9SMBState {
>>> static const VMStateDescription vmstate_ich9_smbus = {
>>> .name = "ich9_smb",
>>> - .version_id = 1,
>>> + .version_id = 2,
>> Again, do we need to bump this?
>>
>>> .minimum_version_id = 1,
>>> .fields = (VMStateField[]) {
>>> - VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
>>> + VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
>>> + VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 2),
>> Can we make this a VMSTATE_BOOL_TEST (see VMSTATE_UINT64_TEST for the
>> pattern) tied to the same .neede, and again we don't need to bump the
>> version.
>>
>>> + VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
>> So that's taken care of by the .needed.
>>
>> Dave
>>
>>> VMSTATE_END_OF_LIST()
>>> }
>>> };
>>> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
>>> index 7bcca97672..5075fc64fa 100644
>>> --- a/include/hw/i2c/pm_smbus.h
>>> +++ b/include/hw/i2c/pm_smbus.h
>>> @@ -43,4 +43,6 @@ typedef struct PMSMBus {
>>> void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool
>>> force_aux_blk);
>>> +extern const VMStateDescription pmsmb_vmstate;
>>> +
>>> #endif /* PM_SMBUS_H */
>>> --
>>> 2.17.1
>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer
2018-11-26 19:41 ` Corey Minyard
@ 2018-11-27 18:20 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-27 18:20 UTC (permalink / raw)
To: Corey Minyard
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard
* Corey Minyard (minyard@acm.org) wrote:
> On 11/26/18 12:24 PM, Corey Minyard wrote:
> > On 11/26/18 11:20 AM, Dr. David Alan Gilbert wrote:
> > > * minyard@acm.org (minyard@acm.org) wrote:
> > > > From: Corey Minyard <cminyard@mvista.com>
> > > >
> > > > Transfer the state information for the SMBus registers and
> > > > internal data so it will work on a VM transfer.
> > > >
> > > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > ---
> > > > hw/acpi/piix4.c | 3 ++-
> > > > hw/i2c/pm_smbus.c | 31 +++++++++++++++++++++++++++++++
> > > > hw/i2c/smbus_ich9.c | 6 ++++--
> > > > include/hw/i2c/pm_smbus.h | 2 ++
> > > > 4 files changed, 39 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index e330f24c71..313305f5a0 100644
> > > > --- a/hw/acpi/piix4.c
> > > > +++ b/hw/acpi/piix4.c
> > > > @@ -309,7 +309,7 @@ static const VMStateDescription
> > > > vmstate_cpuhp_state = {
> > > > */
> > > > static const VMStateDescription vmstate_acpi = {
> > > > .name = "piix4_pm",
> > > > - .version_id = 3,
> > > > + .version_id = 4,
> > > OK, so do we need to bump this version ? Ideally you'd keep the version
> > > as is and let the needed do the work.
> >
> >
> > Got it, that makes sense. Same for the comments below, I'll get all
> > those.
> >
> Well, maybe not. the .needed function is only called on the save side, it
> is
> not called on the load side So a 2.12 to 3.0 transfer fails. So I've
> exported
> the migration needed function and I'm using the VMSTATE_STRUCT_TEST()
> function to transfer it in each case. With that I can migrate from 2.12 to
> 3.0 and back without issue.
Ah OK, that's an interesting observation; I hadn't realised it wasn't
symmetric like that, but I can kind of see why thinking about how the
code got that way.
Dave
> -corey
>
>
> > Thanks,
> >
> > -corey
> >
> >
> > > > .minimum_version_id = 3,
> > > > .minimum_version_id_old = 1,
> > > > .load_state_old = acpi_load_old,
> > > > @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
> > > > VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
> > > > VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
> > > > VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> > > > + VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
> > > > VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
> > > > VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
> > > > VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe,
> > > > ACPIGPE),
> > > > diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> > > > index 8793113c25..75907e1c22 100644
> > > > --- a/hw/i2c/pm_smbus.c
> > > > +++ b/hw/i2c/pm_smbus.c
> > > > @@ -19,6 +19,7 @@
> > > > */
> > > > #include "qemu/osdep.h"
> > > > #include "hw/hw.h"
> > > > +#include "hw/boards.h"
> > > > #include "hw/i2c/pm_smbus.h"
> > > > #include "hw/i2c/smbus_master.h"
> > > > @@ -450,6 +451,36 @@ static const MemoryRegionOps pm_smbus_ops = {
> > > > .endianness = DEVICE_LITTLE_ENDIAN,
> > > > };
> > > > +static bool pm_smbus_vmstate_needed(void *opaque)
> > > > +{
> > > > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > > +
> > > > + return !mc->smbus_no_migration_support;
> > > > +}
> > > OK
> > >
> > > > +const VMStateDescription pmsmb_vmstate = {
> > > > + .name = "pmsmb",
> > > > + .version_id = 1,
> > > > + .minimum_version_id = 1,
> > > > + .needed = pm_smbus_vmstate_needed,
> > > > + .fields = (VMStateField[]) {
> > > > + VMSTATE_UINT8(smb_stat, PMSMBus),
> > > > + VMSTATE_UINT8(smb_ctl, PMSMBus),
> > > > + VMSTATE_UINT8(smb_cmd, PMSMBus),
> > > > + VMSTATE_UINT8(smb_addr, PMSMBus),
> > > > + VMSTATE_UINT8(smb_data0, PMSMBus),
> > > > + VMSTATE_UINT8(smb_data1, PMSMBus),
> > > > + VMSTATE_UINT32(smb_index, PMSMBus),
> > > > + VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> > > > + VMSTATE_UINT8(smb_auxctl, PMSMBus),
> > > > + VMSTATE_BOOL(i2c_enable, PMSMBus),
> > > > + VMSTATE_BOOL(op_done, PMSMBus),
> > > > + VMSTATE_BOOL(in_i2c_block_read, PMSMBus),
> > > > + VMSTATE_BOOL(start_transaction_on_status_read, PMSMBus),
> > > > + VMSTATE_END_OF_LIST()
> > > > + }
> > > > +};
> > > > +
> > > > void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool
> > > > force_aux_blk)
> > > > {
> > > > smb->op_done = true;
> > > > diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> > > > index e6d8d28194..c9b7482a54 100644
> > > > --- a/hw/i2c/smbus_ich9.c
> > > > +++ b/hw/i2c/smbus_ich9.c
> > > > @@ -45,10 +45,12 @@ typedef struct ICH9SMBState {
> > > > static const VMStateDescription vmstate_ich9_smbus = {
> > > > .name = "ich9_smb",
> > > > - .version_id = 1,
> > > > + .version_id = 2,
> > > Again, do we need to bump this?
> > >
> > > > .minimum_version_id = 1,
> > > > .fields = (VMStateField[]) {
> > > > - VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> > > > + VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> > > > + VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 2),
> > > Can we make this a VMSTATE_BOOL_TEST (see VMSTATE_UINT64_TEST for the
> > > pattern) tied to the same .neede, and again we don't need to bump the
> > > version.
> > >
> > > > + VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
> > > So that's taken care of by the .needed.
> > >
> > > Dave
> > >
> > > > VMSTATE_END_OF_LIST()
> > > > }
> > > > };
> > > > diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> > > > index 7bcca97672..5075fc64fa 100644
> > > > --- a/include/hw/i2c/pm_smbus.h
> > > > +++ b/include/hw/i2c/pm_smbus.h
> > > > @@ -43,4 +43,6 @@ typedef struct PMSMBus {
> > > > void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool
> > > > force_aux_blk);
> > > > +extern const VMStateDescription pmsmb_vmstate;
> > > > +
> > > > #endif /* PM_SMBUS_H */
> > > > --
> > > > 2.17.1
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 08/12] i2c: Add an SMBus vmstate structure
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (6 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 07/12] i2c:pm_smbus: Fix state transfer minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 09/12] i2c: Add normal type name and cast to smbus_eeprom.c minyard
` (4 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
There is no vmstate handling for SMBus, so no device sitting on SMBus
can have a state transfer that works reliably. So add it.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/i2c/smbus_slave.c | 18 ++++++++++++++++++
include/hw/i2c/smbus_slave.h | 24 +++++++++++++++++++++---
2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index fa988919d8..b8a2d521f4 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -206,6 +206,24 @@ static void smbus_device_class_init(ObjectClass *klass, void *data)
sc->send = smbus_i2c_send;
}
+bool smbus_vmstate_needed(SMBusDevice *dev)
+{
+ return dev->mode != SMBUS_IDLE;
+}
+
+const VMStateDescription vmstate_smbus_device = {
+ .name = TYPE_SMBUS_DEVICE,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
+ VMSTATE_INT32(mode, SMBusDevice),
+ VMSTATE_INT32(data_len, SMBusDevice),
+ VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const TypeInfo smbus_device_type_info = {
.name = TYPE_SMBUS_DEVICE,
.parent = TYPE_I2C_SLAVE,
diff --git a/include/hw/i2c/smbus_slave.h b/include/hw/i2c/smbus_slave.h
index eabac1dd73..d53d691bf6 100644
--- a/include/hw/i2c/smbus_slave.h
+++ b/include/hw/i2c/smbus_slave.h
@@ -75,14 +75,32 @@ typedef struct SMBusDeviceClass
void (*transaction_complete)(SMBusDevice *dev);
} SMBusDeviceClass;
+#define SMBUS_DATA_MAX_LEN 34 /* command + len + 32 bytes of data. */
+
struct SMBusDevice {
/* The SMBus protocol is implemented on top of I2C. */
I2CSlave i2c;
/* Remaining fields for internal use only. */
- int mode;
- int data_len;
- uint8_t data_buf[34]; /* command + len + 32 bytes of data. */
+ int32_t mode;
+ int32_t data_len;
+ uint8_t data_buf[SMBUS_DATA_MAX_LEN];
};
+extern const VMStateDescription vmstate_smbus_device;
+
+#define VMSTATE_SMBUS_DEVICE(_field, _state) { \
+ .name = (stringify(_field)), \
+ .size = sizeof(SMBusDevice), \
+ .vmsd = &vmstate_smbus_device, \
+ .flags = VMS_STRUCT, \
+ .offset = vmstate_offset_value(_state, _field, SMBusDevice), \
+}
+
+/*
+ * Users should call this in their .needed functions to know if the
+ * SMBus slave data needs to be transferred.
+ */
+bool smbus_vmstate_needed(SMBusDevice *dev);
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 09/12] i2c: Add normal type name and cast to smbus_eeprom.c
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (7 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 08/12] i2c: Add an SMBus vmstate structure minyard
@ 2018-11-15 19:24 ` minyard
2018-11-20 15:34 ` Peter Maydell
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 10/12] i2c: Add a size constant for the smbus_eeprom size minyard
` (3 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Create a type name and a cast macro and use those through the
code.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/i2c/smbus_eeprom.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 4d25222e23..8d4eed129f 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -30,6 +30,11 @@
//#define DEBUG
+#define TYPE_SMBUS_EEPROM "smbus-eeprom"
+
+#define SMBUS_EEPROM(obj) \
+ OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
+
typedef struct SMBusEEPROMDevice {
SMBusDevice smbusdev;
void *data;
@@ -38,7 +43,7 @@ typedef struct SMBusEEPROMDevice {
static uint8_t eeprom_receive_byte(SMBusDevice *dev)
{
- SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+ SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
uint8_t *data = eeprom->data;
uint8_t val = data[eeprom->offset++];
@@ -51,7 +56,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
{
- SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+ SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
uint8_t *data = eeprom->data;
#ifdef DEBUG
@@ -73,7 +78,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
{
- SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
+ SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
eeprom->offset = 0;
}
@@ -97,7 +102,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
}
static const TypeInfo smbus_eeprom_info = {
- .name = "smbus-eeprom",
+ .name = TYPE_SMBUS_EEPROM,
.parent = TYPE_SMBUS_DEVICE,
.instance_size = sizeof(SMBusEEPROMDevice),
.class_init = smbus_eeprom_class_initfn,
@@ -114,7 +119,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
{
DeviceState *dev;
- dev = qdev_create((BusState *) smbus, "smbus-eeprom");
+ dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
qdev_prop_set_uint8(dev, "address", address);
qdev_prop_set_ptr(dev, "data", eeprom_buf);
qdev_init_nofail(dev);
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v2 10/12] i2c: Add a size constant for the smbus_eeprom size
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (8 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 09/12] i2c: Add normal type name and cast to smbus_eeprom.c minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 22:34 ` Philippe Mathieu-Daudé
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 11/12] i2c: Add vmstate handling to the smbus eeprom minyard
` (2 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
It was hard-coded to 256 in a number of places, create a constant
for that.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/i2c/smbus_eeprom.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 8d4eed129f..8e9b734c09 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -35,6 +35,8 @@
#define SMBUS_EEPROM(obj) \
OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
+#define SMBUS_EEPROM_SIZE 256
+
typedef struct SMBusEEPROMDevice {
SMBusDevice smbusdev;
void *data;
@@ -70,7 +72,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
for (; len > 0; len--) {
data[eeprom->offset] = *buf++;
- eeprom->offset = (eeprom->offset + 1) % 256;
+ eeprom->offset = (eeprom->offset + 1) % SMBUS_EEPROM_SIZE;
}
return 0;
@@ -129,12 +131,14 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
const uint8_t *eeprom_spd, int eeprom_spd_size)
{
int i;
- uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */
+ /* XXX: make this persistent */
+ uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
if (eeprom_spd_size > 0) {
memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
}
for (i = 0; i < nb_eeprom; i++) {
- smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
+ smbus_eeprom_init_one(smbus, 0x50 + i,
+ eeprom_buf + (i * SMBUS_EEPROM_SIZE));
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 10/12] i2c: Add a size constant for the smbus_eeprom size
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 10/12] i2c: Add a size constant for the smbus_eeprom size minyard
@ 2018-11-15 22:34 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-15 22:34 UTC (permalink / raw)
To: minyard, qemu-devel
Cc: Paolo Bonzini, Corey Minyard, Dr . David Alan Gilbert,
Michael S . Tsirkin
On 15/11/18 20:24, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> It was hard-coded to 256 in a number of places, create a constant
> for that.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
> hw/i2c/smbus_eeprom.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 8d4eed129f..8e9b734c09 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -35,6 +35,8 @@
> #define SMBUS_EEPROM(obj) \
> OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
>
> +#define SMBUS_EEPROM_SIZE 256
> +
> typedef struct SMBusEEPROMDevice {
> SMBusDevice smbusdev;
> void *data;
> @@ -70,7 +72,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
>
> for (; len > 0; len--) {
> data[eeprom->offset] = *buf++;
> - eeprom->offset = (eeprom->offset + 1) % 256;
> + eeprom->offset = (eeprom->offset + 1) % SMBUS_EEPROM_SIZE;
> }
>
> return 0;
> @@ -129,12 +131,14 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> const uint8_t *eeprom_spd, int eeprom_spd_size)
> {
> int i;
> - uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */
> + /* XXX: make this persistent */
> + uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
Ideally this requires a previous patch replacing 8 -> nb_eeprom, fixing
a long standing bug.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> if (eeprom_spd_size > 0) {
> memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
> }
>
> for (i = 0; i < nb_eeprom; i++) {
> - smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
> + smbus_eeprom_init_one(smbus, 0x50 + i,
> + eeprom_buf + (i * SMBUS_EEPROM_SIZE));
> }
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 11/12] i2c: Add vmstate handling to the smbus eeprom
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (9 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 10/12] i2c: Add a size constant for the smbus_eeprom size minyard
@ 2018-11-15 19:24 ` minyard
2018-11-26 17:30 ` Dr. David Alan Gilbert
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 12/12] i2c: Add a reset function to smbus_eeprom minyard
2018-11-15 23:01 ` [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code Philippe Mathieu-Daudé
12 siblings, 1 reply; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard, Peter Maydell
From: Corey Minyard <cminyard@mvista.com>
Transfer the state of the EEPROM on a migration. This way the
data remains consistent on migration.
This required moving the actual data to a separate array and
using the data provided in the init function as a separate
initialization array, since a pointer property has to be a
void * and the array needs to be uint8_t[].
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 8e9b734c09..942057dc10 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -24,6 +24,7 @@
#include "qemu/osdep.h"
#include "hw/hw.h"
+#include "hw/boards.h"
#include "hw/i2c/i2c.h"
#include "hw/i2c/smbus_slave.h"
#include "hw/i2c/smbus_eeprom.h"
@@ -39,8 +40,10 @@
typedef struct SMBusEEPROMDevice {
SMBusDevice smbusdev;
- void *data;
+ uint8_t data[SMBUS_EEPROM_SIZE];
+ void *init_data;
uint8_t offset;
+ bool accessed;
} SMBusEEPROMDevice;
static uint8_t eeprom_receive_byte(SMBusDevice *dev)
@@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
uint8_t *data = eeprom->data;
uint8_t val = data[eeprom->offset++];
+ eeprom->accessed = true;
#ifdef DEBUG
printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
dev->i2c.address, val);
@@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
uint8_t *data = eeprom->data;
+ eeprom->accessed = true;
#ifdef DEBUG
printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
dev->i2c.address, cmd, buf[0]);
@@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
return 0;
}
+static bool smbus_eeprom_vmstate_needed(void *opaque)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+ SMBusEEPROMDevice *eeprom = opaque;
+
+ return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
+ !mc->smbus_no_migration_support;
+}
+
+static const VMStateDescription vmstate_smbus_eeprom = {
+ .name = "smbus-eeprom",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = smbus_eeprom_vmstate_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+ VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
+ VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+ VMSTATE_BOOL(accessed, SMBusEEPROMDevice),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
+ memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
eeprom->offset = 0;
}
static Property smbus_eeprom_properties[] = {
- DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
+ DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
DEFINE_PROP_END_OF_LIST(),
};
@@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
dc->props = smbus_eeprom_properties;
+ dc->vmsd = &vmstate_smbus_eeprom;
/* Reason: pointer property "data" */
dc->user_creatable = false;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 11/12] i2c: Add vmstate handling to the smbus eeprom
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 11/12] i2c: Add vmstate handling to the smbus eeprom minyard
@ 2018-11-26 17:30 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 38+ messages in thread
From: Dr. David Alan Gilbert @ 2018-11-26 17:30 UTC (permalink / raw)
To: minyard
Cc: qemu-devel, Paolo Bonzini, Michael S . Tsirkin, Corey Minyard,
Peter Maydell
* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> Transfer the state of the EEPROM on a migration. This way the
> data remains consistent on migration.
>
> This required moving the actual data to a separate array and
> using the data provided in the init function as a separate
> initialization array, since a pointer property has to be a
> void * and the array needs to be uint8_t[].
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 8e9b734c09..942057dc10 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -24,6 +24,7 @@
>
> #include "qemu/osdep.h"
> #include "hw/hw.h"
> +#include "hw/boards.h"
> #include "hw/i2c/i2c.h"
> #include "hw/i2c/smbus_slave.h"
> #include "hw/i2c/smbus_eeprom.h"
> @@ -39,8 +40,10 @@
>
> typedef struct SMBusEEPROMDevice {
> SMBusDevice smbusdev;
> - void *data;
> + uint8_t data[SMBUS_EEPROM_SIZE];
> + void *init_data;
> uint8_t offset;
> + bool accessed;
> } SMBusEEPROMDevice;
>
> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> uint8_t *data = eeprom->data;
> uint8_t val = data[eeprom->offset++];
>
> + eeprom->accessed = true;
> #ifdef DEBUG
> printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
> dev->i2c.address, val);
> @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> uint8_t *data = eeprom->data;
>
> + eeprom->accessed = true;
> #ifdef DEBUG
> printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
> dev->i2c.address, cmd, buf[0]);
> @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> return 0;
> }
>
> +static bool smbus_eeprom_vmstate_needed(void *opaque)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> + SMBusEEPROMDevice *eeprom = opaque;
> +
> + return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) &&
> + !mc->smbus_no_migration_support;
> +}
> +
> +static const VMStateDescription vmstate_smbus_eeprom = {
> + .name = "smbus-eeprom",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = smbus_eeprom_vmstate_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> + VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE),
> + VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> + VMSTATE_BOOL(accessed, SMBusEEPROMDevice),
> + VMSTATE_END_OF_LIST()
I think this is OK from the vmstate side of things.
Dave
> + }
> +};
> +
> static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> {
> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>
> + memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
> eeprom->offset = 0;
> }
>
> static Property smbus_eeprom_properties[] = {
> - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data),
> + DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> sc->receive_byte = eeprom_receive_byte;
> sc->write_data = eeprom_write_data;
> dc->props = smbus_eeprom_properties;
> + dc->vmsd = &vmstate_smbus_eeprom;
> /* Reason: pointer property "data" */
> dc->user_creatable = false;
> }
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v2 12/12] i2c: Add a reset function to smbus_eeprom
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (10 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 11/12] i2c: Add vmstate handling to the smbus eeprom minyard
@ 2018-11-15 19:24 ` minyard
2018-11-15 23:01 ` [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code Philippe Mathieu-Daudé
12 siblings, 0 replies; 38+ messages in thread
From: minyard @ 2018-11-15 19:24 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S . Tsirkin, Dr . David Alan Gilbert,
minyard, Corey Minyard
From: Corey Minyard <cminyard@mvista.com>
Reset the contents to init data and reset the offset on a machine
reset.
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
hw/i2c/smbus_eeprom.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 942057dc10..d0a8d63869 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -106,7 +106,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
}
};
-static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+static void smbus_eeprom_reset(DeviceState *dev)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
@@ -114,6 +114,11 @@ static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
eeprom->offset = 0;
}
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+{
+ smbus_eeprom_reset(dev);
+}
+
static Property smbus_eeprom_properties[] = {
DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
DEFINE_PROP_END_OF_LIST(),
@@ -125,6 +130,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
dc->realize = smbus_eeprom_realize;
+ dc->reset = smbus_eeprom_reset;
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
dc->props = smbus_eeprom_properties;
--
2.17.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code
2018-11-15 19:24 [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code minyard
` (11 preceding siblings ...)
2018-11-15 19:24 ` [Qemu-devel] [PATCH v2 12/12] i2c: Add a reset function to smbus_eeprom minyard
@ 2018-11-15 23:01 ` Philippe Mathieu-Daudé
2018-11-16 13:30 ` Corey Minyard
2018-11-26 14:11 ` Corey Minyard
12 siblings, 2 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-15 23:01 UTC (permalink / raw)
To: minyard, qemu-devel
Cc: Paolo Bonzini, Dr . David Alan Gilbert, Michael S . Tsirkin
Hi Corey,
On 15/11/18 20:24, minyard@acm.org wrote:
> These changes allow SMBus access while doing a state transfer.
> Seems like a good idea to me in general.
>
> I have these queued for the SMBus IPMI driver work, of course.
>
> I had submitted this before and then lost track of the work since I
> started finding all kinds of broken things in the I2C code. I
> have fixed the broken things I found first, and then added the
> previous patches.
>
> I have tested this in q35 and it works without issue. On piix4 the
> pm_smbus code is broken on a migration, however. The device disappears
> from the PCI bus on a migration, from what I can tell. It's not the
> fault of this code, something more fundamental is going on. The
> following comment in piix4.c may have something to do with it:
>
> /* qemu-kvm 1.2 uses version 3 but advertised as 2
> * To support incoming qemu-kvm 1.2 migration, change version_id
> * and minimum_version_id to 2 below (which breaks migration from
> * qemu 1.2).
>
> Anyway, I need to chase that down.
>
> I'm primarily submitting this to make sure I'm doing the backwards
> compatability with .needed correctly. I'm adding a new field in
> the machine class and setting it in the initialization code for
> older versions. David, is this what you wanted? It will have to
> be adjusted for the proper version if/when it really goes in, of
> course. You can see those in the following commits:
> boards.h: Ignore migration for SMBus devices on
> i2c:pm_smbus: Fix state transfer
> i2c: Add vmstate handling to the smbus eeprom
> I thought about adding a field to the pm_smbus code to only transfer
> if it was accessed, but I'm assuming that most modern OSes will
> at least initialized the device based on its presence on the PCI
> bus. So that didn't seem like it would add any value.
>
> I'm also submitting to see if all the fixes and cleanups look ok.
> That's the first 5 commits.
$ git diff origin/master --summary
delete mode 100644 hw/i2c/smbus.c
create mode 100644 hw/i2c/smbus_master.c
create mode 100644 hw/i2c/smbus_slave.c
create mode 100644 include/hw/i2c/smbus_eeprom.h
rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
create mode 100644 include/hw/i2c/smbus_slave.h
Can you add the following files in the MAINTAINERS file:
- hw/i2c/smbus_master.c
- hw/i2c/smbus_slave.c
- include/hw/i2c/smbus_eeprom.h
- include/hw/i2c/smbus_master.h
- include/hw/i2c/smbus_slave.h
Thanks,
Phil.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code
2018-11-15 23:01 ` [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code Philippe Mathieu-Daudé
@ 2018-11-16 13:30 ` Corey Minyard
2018-11-26 14:11 ` Corey Minyard
1 sibling, 0 replies; 38+ messages in thread
From: Corey Minyard @ 2018-11-16 13:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Paolo Bonzini, Dr . David Alan Gilbert, Michael S . Tsirkin
On 11/15/18 5:01 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 15/11/18 20:24, minyard@acm.org wrote:
>> These changes allow SMBus access while doing a state transfer.
>> Seems like a good idea to me in general.
>>
>> I have these queued for the SMBus IPMI driver work, of course.
>>
>> I had submitted this before and then lost track of the work since I
>> started finding all kinds of broken things in the I2C code. I
>> have fixed the broken things I found first, and then added the
>> previous patches.
>>
>> I have tested this in q35 and it works without issue. On piix4 the
>> pm_smbus code is broken on a migration, however. The device disappears
>> from the PCI bus on a migration, from what I can tell. It's not the
>> fault of this code, something more fundamental is going on. The
>> following comment in piix4.c may have something to do with it:
>>
>> /* qemu-kvm 1.2 uses version 3 but advertised as 2
>> * To support incoming qemu-kvm 1.2 migration, change version_id
>> * and minimum_version_id to 2 below (which breaks migration from
>> * qemu 1.2).
>>
>> Anyway, I need to chase that down.
>>
>> I'm primarily submitting this to make sure I'm doing the backwards
>> compatability with .needed correctly. I'm adding a new field in
>> the machine class and setting it in the initialization code for
>> older versions. David, is this what you wanted? It will have to
>> be adjusted for the proper version if/when it really goes in, of
>> course. You can see those in the following commits:
>> boards.h: Ignore migration for SMBus devices on
>> i2c:pm_smbus: Fix state transfer
>> i2c: Add vmstate handling to the smbus eeprom
>> I thought about adding a field to the pm_smbus code to only transfer
>> if it was accessed, but I'm assuming that most modern OSes will
>> at least initialized the device based on its presence on the PCI
>> bus. So that didn't seem like it would add any value.
>>
>> I'm also submitting to see if all the fixes and cleanups look ok.
>> That's the first 5 commits.
>
> $ git diff origin/master --summary
> delete mode 100644 hw/i2c/smbus.c
> create mode 100644 hw/i2c/smbus_master.c
> create mode 100644 hw/i2c/smbus_slave.c
> create mode 100644 include/hw/i2c/smbus_eeprom.h
> rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
> create mode 100644 include/hw/i2c/smbus_slave.h
>
> Can you add the following files in the MAINTAINERS file:
> - hw/i2c/smbus_master.c
> - hw/i2c/smbus_slave.c
> - include/hw/i2c/smbus_eeprom.h
> - include/hw/i2c/smbus_master.h
> - include/hw/i2c/smbus_slave.h
>
Ok, but who should the maintainer be? I guess I can take
it, if that is what you are implying. But most of the files in
i2c are not maintained.
Thanks,
-corey
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code
2018-11-15 23:01 ` [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code Philippe Mathieu-Daudé
2018-11-16 13:30 ` Corey Minyard
@ 2018-11-26 14:11 ` Corey Minyard
2018-11-26 14:35 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 38+ messages in thread
From: Corey Minyard @ 2018-11-26 14:11 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, Dr . David Alan Gilbert
Cc: Paolo Bonzini, Michael S . Tsirkin
On 11/15/18 5:01 PM, Philippe Mathieu-Daudé wrote:
> Hi Corey,
>
> On 15/11/18 20:24, minyard@acm.org wrote:
>> These changes allow SMBus access while doing a state transfer.
>> Seems like a good idea to me in general.
>>
>>
>>
>> I'm primarily submitting this to make sure I'm doing the backwards
>> compatability with .needed correctly. I'm adding a new field in
>> the machine class and setting it in the initialization code for
>> older versions. David, is this what you wanted? It will have to
>> be adjusted for the proper version if/when it really goes in, of
>> course. You can see those in the following commits:
>> boards.h: Ignore migration for SMBus devices on
>> i2c:pm_smbus: Fix state transfer
>> i2c: Add vmstate handling to the smbus eeprom
>> I thought about adding a field to the pm_smbus code to only transfer
>> if it was accessed, but I'm assuming that most modern OSes will
>> at least initialized the device based on its presence on the PCI
>> bus. So that didn't seem like it would add any value.
>>
>> I'm also submitting to see if all the fixes and cleanups look ok.
>> That's the first 5 commits.
>
> $ git diff origin/master --summary
> delete mode 100644 hw/i2c/smbus.c
> create mode 100644 hw/i2c/smbus_master.c
> create mode 100644 hw/i2c/smbus_slave.c
> create mode 100644 include/hw/i2c/smbus_eeprom.h
> rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
> create mode 100644 include/hw/i2c/smbus_slave.h
>
> Can you add the following files in the MAINTAINERS file:
> - hw/i2c/smbus_master.c
> - hw/i2c/smbus_slave.c
> - include/hw/i2c/smbus_eeprom.h
> - include/hw/i2c/smbus_master.h
> - include/hw/i2c/smbus_slave.h
I'm almost ready to re-submit this series, but I'd like to do 3
things:
* Add the proper person as the maintainer. I can be the
maintainer, but I don't want to presume that's what you
meant. No general I2C code has a maintainer at the moment.
* I'd like to get David's comments on the .needed addition, as
I mention above.
* I need to figure out why piix4 smbus does not work after a
migration. I'll work on that today.
Thanks,
-corey
>
> Thanks,
>
> Phil.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code
2018-11-26 14:11 ` Corey Minyard
@ 2018-11-26 14:35 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 38+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-26 14:35 UTC (permalink / raw)
To: minyard, qemu-devel, Dr . David Alan Gilbert
Cc: Paolo Bonzini, Michael S . Tsirkin, David Gibson
On 26/11/18 15:11, Corey Minyard wrote:
> On 11/15/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>> Hi Corey,
>>
>> On 15/11/18 20:24, minyard@acm.org wrote:
>>> These changes allow SMBus access while doing a state transfer.
>>> Seems like a good idea to me in general.
>>>
>>>
>>>
>>> I'm primarily submitting this to make sure I'm doing the backwards
>>> compatability with .needed correctly. I'm adding a new field in
>>> the machine class and setting it in the initialization code for
>>> older versions. David, is this what you wanted? It will have to
>>> be adjusted for the proper version if/when it really goes in, of
>>> course. You can see those in the following commits:
>>> boards.h: Ignore migration for SMBus devices on
>>> i2c:pm_smbus: Fix state transfer
>>> i2c: Add vmstate handling to the smbus eeprom
>>> I thought about adding a field to the pm_smbus code to only transfer
>>> if it was accessed, but I'm assuming that most modern OSes will
>>> at least initialized the device based on its presence on the PCI
>>> bus. So that didn't seem like it would add any value.
>>>
>>> I'm also submitting to see if all the fixes and cleanups look ok.
>>> That's the first 5 commits.
>>
>> $ git diff origin/master --summary
>> delete mode 100644 hw/i2c/smbus.c
>> create mode 100644 hw/i2c/smbus_master.c
>> create mode 100644 hw/i2c/smbus_slave.c
>> create mode 100644 include/hw/i2c/smbus_eeprom.h
>> rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
>> create mode 100644 include/hw/i2c/smbus_slave.h
>>
>> Can you add the following files in the MAINTAINERS file:
>> - hw/i2c/smbus_master.c
>> - hw/i2c/smbus_slave.c
>> - include/hw/i2c/smbus_eeprom.h
>> - include/hw/i2c/smbus_master.h
>> - include/hw/i2c/smbus_slave.h
>
> I'm almost ready to re-submit this series, but I'd like to do 3
> things:
>
> * Add the proper person as the maintainer. I can be the
> maintainer, but I don't want to presume that's what you
> meant. No general I2C code has a maintainer at the moment.
Looking at the git history, the i2c logical code seems stable, most of
the recent changes are API improvements.
The devices are mostly split into x86 (old, stable) and arm/ppc, merged
by respective maintainers, except various cleanups/improvements merged
by Paolo's Misc tree, but Paolo recently asked for help to reduce his
workload.
You seem to worry enough about this subsystem to refactor/improve it,
and this series show you have a deep understanding.
I'm not a QEMU maintainer, but if you agree to step in with the I2C
subsystem, it looks natural to me for you to be there.
This doesn't mean you will be alone, I am pretty sure the previous
maintainers merging the previous patches there will help you reviewing.
For the I2C devices, the get_maintainer script already show the
maintainers, so if this isn't a core I2C change, you can review the
patch and let them merge. For example:
$ ./scripts/get_maintainer.pl -f hw/i2c/versatile_i2c.c
Peter Maydell <peter.maydell@linaro.org> (maintainer:Versatile PB)
qemu-arm@nongnu.org (open list:Versatile PB)
$ ./scripts/get_maintainer.pl -f hw/i2c/smbus_ich9.c
"Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:PC)
$ ./scripts/get_maintainer.pl -f hw/i2c/ppc4xx_i2c.c
David Gibson <david@gibson.dropbear.id.au> (odd fixer:ppc4xx)
qemu-ppc@nongnu.org (open list:ppc4xx)
Regards,
Phil.
> * I'd like to get David's comments on the .needed addition, as
> I mention above.
> * I need to figure out why piix4 smbus does not work after a
> migration. I'll work on that today.
>
> Thanks,
>
> -corey
>
>>
>> Thanks,
>>
>> Phil.
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread