[PATCH RFC 0/1] i2c: add slave testunit driver

[PATCH RFC 0/1] i2c: add slave testunit driver

[Wolfram Sang]

Motivated by a series by Alain Volmat which implements SMBus Host Notify
support as a slave backend[1], I wondered how I could actually test it.
Then, I picked up my old idea of a "custom remote device" and
implemented it as another slave backend. This is the first draft and it
works quite well on my Renesas Lager board where I connected two I2C
busses where both I2C controllers are master and slave. One slave is the
testunit, one slave is the HostNotify listener.

While I really like Alain's approach, there is still some more testing
needed. So, I already release my testing environment, maybe other people
are interested, too. This patch depends on a documentation update. Also,
for Renesas R-Car SoCs, some fixes are needed. I suggest you simply pull
this branch here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/slave-testunit

As mentioned elsewhere, support for SMBus Alert and I2C_M_RECV_LEN are
already planned. But I guess you can do much more.

Ideas and comments welcome!

Happy hacking,

   Wolfram

[1] http://patchwork.ozlabs.org/project/linux-i2c/list/?series=185718&state=*

Wolfram Sang (1):
  i2c: add slave testunit driver

 Documentation/i2c/slave-testunit-backend.rst |  48 ++++++
 drivers/i2c/Kconfig                          |   8 +
 drivers/i2c/Makefile                         |   1 +
 drivers/i2c/i2c-slave-testunit.c             | 146 +++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 Documentation/i2c/slave-testunit-backend.rst
 create mode 100644 drivers/i2c/i2c-slave-testunit.c

-- 
2.20.1

[PATCH RFC 1/1] i2c: add slave testunit driver

[Wolfram Sang]

Here is a I2C slave backend driver which allows to test some uncommon
functionalities of the I2C and SMBus world. Usually, you need specific
devices to test e.g. SMBus Host Notify and such. With this driver you
just need the slave interface of your host controller.

This initial version has testcases for multi-master and SMBus Host
Notify. Already planned but not yet implemented are SMBus Alert and
messages with I2C_M_RECV_LEN.

Please read the documentation for further details.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-testunit-backend.rst |  48 ++++++
 drivers/i2c/Kconfig                          |   8 +
 drivers/i2c/Makefile                         |   1 +
 drivers/i2c/i2c-slave-testunit.c             | 146 +++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 Documentation/i2c/slave-testunit-backend.rst
 create mode 100644 drivers/i2c/i2c-slave-testunit.c

diff --git a/Documentation/i2c/slave-testunit-backend.rst b/Documentation/i2c/slave-testunit-backend.rst
new file mode 100644
index 000000000000..f4f79db0917a
--- /dev/null
+++ b/Documentation/i2c/slave-testunit-backend.rst
@@ -0,0 +1,48 @@
+================================
+Linux I2C slave testunit backend
+================================
+
+by Wolfram Sang <wsa@sang-engineering.com> in 2020
+
+This backend can be used to trigger test cases for I2C bus masters which
+require a remote device with certain capabilities (and which are usually not so
+easy to obtain). Examples include multi-master testing, and SMBus Host Notify
+testing. For tests marked with '*', your I2C controller must be able to switch
+between master and slave mode because it needs to send data.
+
+Instantiating the device is regular. Example for bus 0, address 0x30:
+
+# echo "slave-testunit 0x1030" > /sys/bus/i2c/devices/i2c-0/new_device
+
+After that, you will have a write-only device listening. Reads will only return
+0xff. The device consists of 4 8-bit registers and all must be written to start
+a testcase, i.e. you must always write 4 bytes to the device. The registers are:
+
+0x00 CMD   - which test to trigger
+0x01 DATAL - configuration byte 1 for the test
+0x02 DATAH - configuration byte 2 for the test
+0x03 DELAY - delay in n * 100ms until test is started
+
+Commands
+--------
+
+0x00 READ_BYTES*
+   DATAL - address to read data from
+   DATAH - number of bytes to read
+
+This is useful to test if your bus master driver is handling multi-master
+correctly. You can trigger the testunit to read bytes from another device on
+the bus. If the bus master under test also wants to access the bus, it will be
+busy. Example to read 128 bytes from device 0x50 after 500ms of delay:
+
+# i2cset -y 1 0x30 0x00 0x50 0x80 0x05 i
+
+0x01 SMBUS_HOST_NOTIFY*
+   DATAL - low byte of the status word to send
+   DATAH - high byte of the status word to send
+
+This test will send an SMBUS_HOST_NOTIFY message to the host. Note that the
+status word is currently ignored in the Linux Kernel. Example to send a
+notification after 100ms:
+
+# i2cset -y 1 0x30 0x01 0x42 0x42 0x01 i
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index bae1dc08ec9a..2b550b5168e5 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -126,6 +126,14 @@ config I2C_SLAVE_EEPROM
 	  This backend makes Linux behave like an I2C EEPROM. Please read
 	  Documentation/i2c/slave-eeprom-backend.rst for further details.
 
+config I2C_SLAVE_TESTUNIT
+	tristate "I2C eeprom testunit driver"
+	help
+	  This backend can be used to trigger test cases for I2C bus masters
+	  which require a remote device with certain capabilities, e.g.
+	  multi-master, SMBus Host Notify, etc. Please read
+	  Documentation/i2c/slave-testunit-backend.rst for further details.
+
 endif
 
 config I2C_DEBUG_CORE
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index bed6ba63c983..c1d493dc9bac 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -16,5 +16,6 @@ obj-$(CONFIG_I2C_MUX)		+= i2c-mux.o
 obj-y				+= algos/ busses/ muxes/
 obj-$(CONFIG_I2C_STUB)		+= i2c-stub.o
 obj-$(CONFIG_I2C_SLAVE_EEPROM)	+= i2c-slave-eeprom.o
+obj-$(CONFIG_I2C_SLAVE_TESTUNIT)	+= i2c-slave-testunit.o
 
 ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
diff --git a/drivers/i2c/i2c-slave-testunit.c b/drivers/i2c/i2c-slave-testunit.c
new file mode 100644
index 000000000000..ba82712eac5f
--- /dev/null
+++ b/drivers/i2c/i2c-slave-testunit.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * I2C slave mode testunit
+ *
+ * Copyright (C) 2020 by Wolfram Sang, Sang Engineering <wsa@sang-engineering.com>
+ * Copyright (C) 2020 by Renesas Electronics Corporation
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h> // FIXME: own workqueue?
+
+enum testunit_cmds {
+	TU_CMD_READ_BYTES,
+	TU_CMD_HOST_NOTIFY,
+};
+
+enum testunit_regs {
+	TU_REG_CMD,
+	TU_REG_DATAL,
+	TU_REG_DATAH,
+	TU_REG_DELAY,
+	TU_NUM_REGS
+};
+
+struct testunit_data {
+	u8 regs[TU_NUM_REGS];	//FIXME: add locking
+	u8 reg_idx;
+	struct i2c_client *client;
+	struct delayed_work worker;
+	struct i2c_msg msg;
+	u8 msgbuf[256];
+};
+
+static void i2c_slave_testunit_work(struct work_struct *work)
+{
+	struct testunit_data *tu = container_of(work, struct testunit_data, worker.work);
+	int ret;
+
+	switch (tu->regs[TU_REG_CMD]) {
+
+	case TU_CMD_READ_BYTES:
+		tu->msg.addr = tu->regs[TU_REG_DATAL];
+		tu->msg.flags = I2C_M_RD;
+		tu->msg.len = tu->regs[TU_REG_DATAH];
+		break;
+
+	case TU_CMD_HOST_NOTIFY:
+		tu->msg.addr = 0x08;
+		tu->msg.flags = 0;
+		tu->msg.len = 3;
+		tu->msgbuf[0] = tu->client->addr;
+		tu->msgbuf[1] = tu->regs[TU_REG_DATAL];
+		tu->msgbuf[2] = tu->regs[TU_REG_DATAH];
+		break;
+	}
+
+	ret = i2c_transfer(tu->client->adapter, &tu->msg, 1);
+	if (ret != 1)
+		dev_err(&tu->client->dev, "CMD%02X failed (%d)\n", tu->regs[TU_REG_CMD], ret);
+}
+
+static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
+				     enum i2c_slave_event event, u8 *val)
+{
+	struct testunit_data *tu = i2c_get_clientdata(client);
+	int ret = 0;
+
+	switch (event) {
+	case I2C_SLAVE_WRITE_REQUESTED:
+		break;
+
+	case I2C_SLAVE_WRITE_RECEIVED:
+		if (tu->reg_idx < TU_NUM_REGS)
+			tu->regs[tu->reg_idx] = *val;
+		else
+			ret = -EMSGSIZE;
+
+		if (tu->reg_idx <= TU_NUM_REGS)
+			tu->reg_idx++;
+
+		break;
+
+	case I2C_SLAVE_STOP:
+		if (tu->reg_idx == TU_NUM_REGS)
+			queue_delayed_work(system_long_wq, &tu->worker,
+					   msecs_to_jiffies(100 * tu->regs[TU_REG_DELAY]));
+		tu->reg_idx = 0;
+		break;
+
+	case I2C_SLAVE_READ_REQUESTED:
+	case I2C_SLAVE_READ_PROCESSED:
+		*val = 0xff;
+		break;
+	}
+
+	return ret;
+}
+
+static int i2c_slave_testunit_probe(struct i2c_client *client)
+{
+	struct testunit_data *tu;
+
+	tu = devm_kzalloc(&client->dev, sizeof(struct testunit_data), GFP_KERNEL);
+	if (!tu)
+		return -ENOMEM;
+
+	tu->msg.buf = tu->msgbuf;
+	tu->client = client;
+	i2c_set_clientdata(client, tu);
+	INIT_DELAYED_WORK(&tu->worker, i2c_slave_testunit_work);
+
+	return i2c_slave_register(client, i2c_slave_testunit_slave_cb);
+};
+
+static int i2c_slave_testunit_remove(struct i2c_client *client)
+{
+	struct testunit_data *tu = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&tu->worker);
+	i2c_slave_unregister(client);
+	return 0;
+}
+
+static const struct i2c_device_id i2c_slave_testunit_id[] = {
+	{ "slave-testunit", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, i2c_slave_testunit_id);
+
+static struct i2c_driver i2c_slave_testunit_driver = {
+	.driver = {
+		.name = "i2c-slave-testunit",
+	},
+	.probe_new = i2c_slave_testunit_probe,
+	.remove = i2c_slave_testunit_remove,
+	.id_table = i2c_slave_testunit_id,
+};
+module_i2c_driver(i2c_slave_testunit_driver);
+
+MODULE_AUTHOR("Wolfram Sang <wsa@sang-engineering.com>");
+MODULE_DESCRIPTION("I2C slave mode test unit");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

Re: [PATCH RFC 0/1] i2c: add slave testunit driver

[Alain Volmat]

Hi Wolfram,

On Mon, Jun 29, 2020 at 06:53:17PM +0000, Wolfram Sang wrote:
> Motivated by a series by Alain Volmat which implements SMBus Host Notify
> support as a slave backend[1], I wondered how I could actually test it.
> Then, I picked up my old idea of a "custom remote device" and
> implemented it as another slave backend. This is the first draft and it
> works quite well on my Renesas Lager board where I connected two I2C
> busses where both I2C controllers are master and slave. One slave is the
> testunit, one slave is the HostNotify listener.
> 
> While I really like Alain's approach, there is still some more testing
> needed. So, I already release my testing environment, maybe other people
> are interested, too. This patch depends on a documentation update. Also,
> for Renesas R-Car SoCs, some fixes are needed. I suggest you simply pull
> this branch here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/slave-testunit
> 
> As mentioned elsewhere, support for SMBus Alert and I2C_M_RECV_LEN are
> already planned. But I guess you can do much more.
> 
> Ideas and comments welcome!

Very good idea to use a slave to send the Host-Notify command to the host
for testing. Later on, for SMBus-Alert, a GPIO can be used to loop it back
to the tested master to verify that SMBus-Alert is working fine.

What you implemented is the "remote" side which I understood is meant to
replace a "real" device for those features which are not that common.
Shouldn't we also have the "master" side loopback test driver as well to
work with this test slave driver ?
For example for the Host-Notify that master side loopback test driver would
perform the request_irq allowing it to be called back when the slave test
driver sends the host-notify command.
In case of SMBus-Alert, that would be implementing the .alert function that
would be called when the SMBus-Alert is received ..

With that the whole loop can be automatically tested. This kind of stuff
can of course be enhanced to a LOT of cases .... basically something similar
to spi-loopback driver for example except that in case of i2c it needs 2
I2C controllers instead of one for the SPI.

> 
> Happy hacking,
> 
>    Wolfram
> 
> [1] http://patchwork.ozlabs.org/project/linux-i2c/list/?series=185718&state=*
> 
> Wolfram Sang (1):
>   i2c: add slave testunit driver
> 
>  Documentation/i2c/slave-testunit-backend.rst |  48 ++++++
>  drivers/i2c/Kconfig                          |   8 +
>  drivers/i2c/Makefile                         |   1 +
>  drivers/i2c/i2c-slave-testunit.c             | 146 +++++++++++++++++++
>  4 files changed, 203 insertions(+)
>  create mode 100644 Documentation/i2c/slave-testunit-backend.rst
>  create mode 100644 drivers/i2c/i2c-slave-testunit.c
> 
> -- 
> 2.20.1
> 

Re: [PATCH RFC 0/1] i2c: add slave testunit driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]


> Very good idea to use a slave to send the Host-Notify command to the host
> for testing. Later on, for SMBus-Alert, a GPIO can be used to loop it back
> to the tested master to verify that SMBus-Alert is working fine.

Glad you like it! Yes, SMBusAlert is the next addition planned.

> What you implemented is the "remote" side which I understood is meant to
> replace a "real" device for those features which are not that common.

Correct.

> Shouldn't we also have the "master" side loopback test driver as well to
> work with this test slave driver ?

Yes, ultimately we want that. But for this first draft, I simply
triggered with 'i2cset' and checked debug prints plus debugfs for
desired outcome.

> For example for the Host-Notify that master side loopback test driver would
> perform the request_irq allowing it to be called back when the slave test
> driver sends the host-notify command.
> In case of SMBus-Alert, that would be implementing the .alert function that
> would be called when the SMBus-Alert is received ..

Exactly. I am simply focussed on the remote side for now because I am
curious if it works at all. And what other parts need fixing, e.g. the
I2C core patch I sent a few minutes ago.

> With that the whole loop can be automatically tested. This kind of stuff
> can of course be enhanced to a LOT of cases .... basically something similar
> to spi-loopback driver for example except that in case of i2c it needs 2
> I2C controllers instead of one for the SPI.

This is my ultimate goal, too: scriptable tests for I2C mastes. Basic
functionality can be tested with a simple device, say an EEPROM. But the
rare stuff needs something like this testunit.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]