[PATCH v2 0/4] i2c: testunit: add rep_start test and rework versioning

[PATCH v2 0/4] i2c: testunit: add rep_start test and rework versioning

[Wolfram Sang]

On my way to let the testunit trigger SMBusAlert interrupts, I needed to
rework the way the testunit responds to read messages. This series is
the result of that with some very nice side effects. First, we now have
a new test to check for proper repeated starts between messages in a
transfer which will also improve versioning (patch 3). Also, the state
of the testunit can now be checked (patch 4). Patch 1 makes the state
machine a little easiert to follow. And patch 2 improves documentation.

The patches are based on Linus' tree close to rc3 and has been tested on
a Renesas Lager board (R-Car H2) and a Renesas Falcon board (R-Car V3U).

Looking forward to comments. Once we got this in, I will send out the
SMBusAlert additions. They do work already, they just need some
documentation.

Changes since v1:
* patch 2 added to improve documentation
* added newline to the version string to make its printout
  "self-contained"
* for reading the new version string, the shell one liner to print
  ASCII directly has been removed. Now we use a new feature of
  i2ctransfer. Note, the docs mention version 4.4 already which is not
  out yet, but it will be by the time 6.11 is released (tm).
* rebased, dependencies are now in Linus' tree

Happy hacking!


Wolfram Sang (4):
  i2c: testunit: sort case blocks
  i2c: testunit: use decimal values in docs when appropriate
  i2c: testunit: add command to support versioning and test rep_start
  i2c: testunit: return current command on read messages

 Documentation/i2c/slave-testunit-backend.rst | 60 ++++++++++++++++----
 drivers/i2c/i2c-slave-testunit.c             | 43 ++++++++++----
 2 files changed, 82 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] i2c: testunit: sort case blocks

[Wolfram Sang]

Because a 'fallthrough' was refactored away, the order of 'case'
statements can be sorted better now to ease understanding the flow of
events.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-slave-testunit.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-slave-testunit.c b/drivers/i2c/i2c-slave-testunit.c
index 4c550306f3ec..be1d2e900aef 100644
--- a/drivers/i2c/i2c-slave-testunit.c
+++ b/drivers/i2c/i2c-slave-testunit.c
@@ -94,6 +94,14 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 	int ret = 0;
 
 	switch (event) {
+	case I2C_SLAVE_WRITE_REQUESTED:
+		if (test_bit(TU_FLAG_IN_PROCESS, &tu->flags))
+			return -EBUSY;
+
+		memset(tu->regs, 0, TU_NUM_REGS);
+		tu->reg_idx = 0;
+		break;
+
 	case I2C_SLAVE_WRITE_RECEIVED:
 		if (test_bit(TU_FLAG_IN_PROCESS, &tu->flags))
 			return -EBUSY;
@@ -127,14 +135,6 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 		tu->reg_idx = 0;
 		break;
 
-	case I2C_SLAVE_WRITE_REQUESTED:
-		if (test_bit(TU_FLAG_IN_PROCESS, &tu->flags))
-			return -EBUSY;
-
-		memset(tu->regs, 0, TU_NUM_REGS);
-		tu->reg_idx = 0;
-		break;
-
 	case I2C_SLAVE_READ_PROCESSED:
 		if (is_proc_call && tu->regs[TU_REG_DATAH])
 			tu->regs[TU_REG_DATAH]--;
-- 
2.43.0

Re: [PATCH v2 1/4] i2c: testunit: sort case blocks

[Wolfram Sang]

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

On Sun, Aug 11, 2024 at 11:23:13PM +0200, Wolfram Sang wrote:
> Because a 'fallthrough' was refactored away, the order of 'case'
> statements can be sorted better now to ease understanding the flow of
> events.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH v2 2/4] i2c: testunit: use decimal values in docs when appropriate

[Wolfram Sang]

Sometimes decimal values are just shorter (like for cmds), sometimes
they are even easier to understand (like for the delay value). Make use
of them.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-testunit-backend.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/i2c/slave-testunit-backend.rst b/Documentation/i2c/slave-testunit-backend.rst
index 37142a48ab35..ee019db53938 100644
--- a/Documentation/i2c/slave-testunit-backend.rst
+++ b/Documentation/i2c/slave-testunit-backend.rst
@@ -75,7 +75,7 @@ from another device on the bus. If the bus master under test also wants to
 access the bus at the same time, the bus will be busy. Example to read 128
 bytes from device 0x50 after 50ms of delay::
 
-  # i2cset -y 0 0x30 0x01 0x50 0x80 0x05 i
+  # i2cset -y 0 0x30 1 0x50 0x80 5 i
 
 0x02 SMBUS_HOST_NOTIFY
 ~~~~~~~~~~~~~~~~~~~~~~
@@ -95,9 +95,9 @@ bytes from device 0x50 after 50ms of delay::
 
 Also needs master mode. 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 10ms::
+Example to send a notification with status word 0x6442 after 10ms::
 
-  # i2cset -y 0 0x30 0x02 0x42 0x64 0x01 i
+  # i2cset -y 0 0x30 2 0x42 0x64 1 i
 
 If the host controller supports HostNotify, this message with debug level
 should appear (Linux 6.11 and later)::
@@ -116,7 +116,7 @@ should appear (Linux 6.11 and later)::
     - DELAY
 
   * - 0x03
-    - must be '1', i.e. one further byte will be written
+    - 0x01 (i.e. one further byte will be written)
     - number of bytes to be sent back
     - leave out, partial command!
 
@@ -131,5 +131,5 @@ from length-1 to 0. Here is an example which emulates
 i2c_smbus_block_process_call() using i2ctransfer (you need i2c-tools v4.2 or
 later)::
 
-  # i2ctransfer -y 0 w3@0x30 0x03 0x01 0x10 r?
+  # i2ctransfer -y 0 w3@0x30 3 1 0x10 r?
   0x10 0x0f 0x0e 0x0d 0x0c 0x0b 0x0a 0x09 0x08 0x07 0x06 0x05 0x04 0x03 0x02 0x01 0x00
-- 
2.43.0

Re: [PATCH v2 2/4] i2c: testunit: use decimal values in docs when appropriate

[Wolfram Sang]

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

On Sun, Aug 11, 2024 at 11:23:14PM +0200, Wolfram Sang wrote:
> Sometimes decimal values are just shorter (like for cmds), sometimes
> they are even easier to understand (like for the delay value). Make use
> of them.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH v2 3/4] i2c: testunit: add command to support versioning and test rep_start

[Wolfram Sang]

For some devices, it is essential that controllers handle repeated start
correctly and do not replace it with a stop/start combination. This
addition helps to test that because it will only return a version string
if repeated start is done properly.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-testunit-backend.rst | 38 ++++++++++++++++++++
 drivers/i2c/i2c-slave-testunit.c             | 25 +++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/Documentation/i2c/slave-testunit-backend.rst b/Documentation/i2c/slave-testunit-backend.rst
index ee019db53938..110c0055064f 100644
--- a/Documentation/i2c/slave-testunit-backend.rst
+++ b/Documentation/i2c/slave-testunit-backend.rst
@@ -133,3 +133,41 @@ later)::
 
   # i2ctransfer -y 0 w3@0x30 3 1 0x10 r?
   0x10 0x0f 0x0e 0x0d 0x0c 0x0b 0x0a 0x09 0x08 0x07 0x06 0x05 0x04 0x03 0x02 0x01 0x00
+
+0x04 GET_VERSION_WITH_REP_START
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. list-table::
+  :header-rows: 1
+
+  * - CMD
+    - DATAL
+    - DATAH
+    - DELAY
+
+  * - 0x04
+    - currently unused
+    - currently unused
+    - leave out, partial command!
+
+Partial command. After sending this command, the testunit will reply to a read
+message with a NUL terminated version string based on UTS_RELEASE. The first
+character is always a 'v' and the length of the version string is at maximum
+128 bytes. However, it will only respond if the read message is connected to
+the write message via repeated start. If your controller driver handles
+repeated start correctly, this will work::
+
+  # i2ctransfer -y 0 w3@0x30 4 0 0 r128
+  0x76 0x36 0x2e 0x31 0x31 0x2e 0x30 0x2d 0x72 0x63 0x31 0x2d 0x30 0x30 0x30 0x30 ...
+
+If you have i2c-tools 4.4 or later, you can print out the data right away::
+
+  # i2ctransfer -y -b 0 w3@0x30 4 0 0 r128
+  v6.11.0-rc1-00009-gd37a1b4d3fd0
+
+STOP/START combinations between the two messages will *not* work because they
+are not equivalent to a REPEATED START. As an example, this returns just the
+default response::
+
+  # i2cset -y 0 0x30 4 0 0 i; i2cget -y 0 0x30
+  0x01
diff --git a/drivers/i2c/i2c-slave-testunit.c b/drivers/i2c/i2c-slave-testunit.c
index be1d2e900aef..51b399aa09a0 100644
--- a/drivers/i2c/i2c-slave-testunit.c
+++ b/drivers/i2c/i2c-slave-testunit.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2020 by Renesas Electronics Corporation
  */
 
+#include <generated/utsrelease.h>
 #include <linux/bitops.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -15,11 +16,13 @@
 #include <linux/workqueue.h> /* FIXME: is system_long_wq the best choice? */
 
 #define TU_CUR_VERSION 0x01
+#define TU_VERSION_MAX_LENGTH 128
 
 enum testunit_cmds {
 	TU_CMD_READ_BYTES = 1,	/* save 0 for ABORT, RESET or similar */
 	TU_CMD_SMBUS_HOST_NOTIFY,
 	TU_CMD_SMBUS_BLOCK_PROC_CALL,
+	TU_CMD_GET_VERSION_WITH_REP_START,
 	TU_NUM_CMDS
 };
 
@@ -39,10 +42,13 @@ struct testunit_data {
 	unsigned long flags;
 	u8 regs[TU_NUM_REGS];
 	u8 reg_idx;
+	u8 read_idx;
 	struct i2c_client *client;
 	struct delayed_work worker;
 };
 
+static char tu_version_info[] = "v" UTS_RELEASE "\n\0";
+
 static void i2c_slave_testunit_work(struct work_struct *work)
 {
 	struct testunit_data *tu = container_of(work, struct testunit_data, worker.work);
@@ -91,6 +97,8 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 	struct testunit_data *tu = i2c_get_clientdata(client);
 	bool is_proc_call = tu->reg_idx == 3 && tu->regs[TU_REG_DATAL] == 1 &&
 			    tu->regs[TU_REG_CMD] == TU_CMD_SMBUS_BLOCK_PROC_CALL;
+	bool is_get_version = tu->reg_idx == 3 &&
+			      tu->regs[TU_REG_CMD] == TU_CMD_GET_VERSION_WITH_REP_START;
 	int ret = 0;
 
 	switch (event) {
@@ -100,6 +108,7 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 
 		memset(tu->regs, 0, TU_NUM_REGS);
 		tu->reg_idx = 0;
+		tu->read_idx = 0;
 		break;
 
 	case I2C_SLAVE_WRITE_RECEIVED:
@@ -136,12 +145,21 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 		break;
 
 	case I2C_SLAVE_READ_PROCESSED:
-		if (is_proc_call && tu->regs[TU_REG_DATAH])
+		/* Advance until we reach the NUL character */
+		if (is_get_version && tu_version_info[tu->read_idx] != 0)
+			tu->read_idx++;
+		else if (is_proc_call && tu->regs[TU_REG_DATAH])
 			tu->regs[TU_REG_DATAH]--;
+
 		fallthrough;
 
 	case I2C_SLAVE_READ_REQUESTED:
-		*val = is_proc_call ? tu->regs[TU_REG_DATAH] : TU_CUR_VERSION;
+		if (is_get_version)
+			*val = tu_version_info[tu->read_idx];
+		else if (is_proc_call)
+			*val = tu->regs[TU_REG_DATAH];
+		else
+			*val = TU_CUR_VERSION;
 		break;
 	}
 
@@ -160,6 +178,9 @@ static int i2c_slave_testunit_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, tu);
 	INIT_DELAYED_WORK(&tu->worker, i2c_slave_testunit_work);
 
+	if (sizeof(tu_version_info) > TU_VERSION_MAX_LENGTH)
+		tu_version_info[TU_VERSION_MAX_LENGTH - 1] = 0;
+
 	return i2c_slave_register(client, i2c_slave_testunit_slave_cb);
 };
 
-- 
2.43.0

Re: [PATCH v2 3/4] i2c: testunit: add command to support versioning and test rep_start

[Wolfram Sang]

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

On Sun, Aug 11, 2024 at 11:23:15PM +0200, Wolfram Sang wrote:
> For some devices, it is essential that controllers handle repeated start
> correctly and do not replace it with a stop/start combination. This
> addition helps to test that because it will only return a version string
> if repeated start is done properly.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH v2 4/4] i2c: testunit: return current command on read messages

[Wolfram Sang]

Because the testunit can start tests in the future via the DELAY
register, it may happen that a command is still pending. Support
detecting that by returning the number of a command in progress (if
there is one).

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/i2c/slave-testunit-backend.rst | 14 ++++++++------
 drivers/i2c/i2c-slave-testunit.c             |  4 ++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/i2c/slave-testunit-backend.rst b/Documentation/i2c/slave-testunit-backend.rst
index 110c0055064f..d3ab5944877d 100644
--- a/Documentation/i2c/slave-testunit-backend.rst
+++ b/Documentation/i2c/slave-testunit-backend.rst
@@ -20,11 +20,13 @@ 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 just return
-an 8-bit version number of the testunit. When writing, the device consists of 4
-8-bit registers and, except for some "partial" commands, all registers must be
-written to start a testcase, i.e. you usually write 4 bytes to the device. The
-registers are:
+After that, you will have the device listening. Reading will return a single
+byte. Its value is 0 if the testunit is idle, otherwise the command number of
+the currently running command.
+
+When writing, the device consists of 4 8-bit registers and, except for some
+"partial" commands, all registers must be written to start a testcase, i.e. you
+usually write 4 bytes to the device. The registers are:
 
 .. csv-table::
   :header: "Offset", "Name", "Description"
@@ -170,4 +172,4 @@ are not equivalent to a REPEATED START. As an example, this returns just the
 default response::
 
   # i2cset -y 0 0x30 4 0 0 i; i2cget -y 0 0x30
-  0x01
+  0x00
diff --git a/drivers/i2c/i2c-slave-testunit.c b/drivers/i2c/i2c-slave-testunit.c
index 51b399aa09a0..04060bc8a9d0 100644
--- a/drivers/i2c/i2c-slave-testunit.c
+++ b/drivers/i2c/i2c-slave-testunit.c
@@ -15,7 +15,6 @@
 #include <linux/slab.h>
 #include <linux/workqueue.h> /* FIXME: is system_long_wq the best choice? */
 
-#define TU_CUR_VERSION 0x01
 #define TU_VERSION_MAX_LENGTH 128
 
 enum testunit_cmds {
@@ -159,7 +158,8 @@ static int i2c_slave_testunit_slave_cb(struct i2c_client *client,
 		else if (is_proc_call)
 			*val = tu->regs[TU_REG_DATAH];
 		else
-			*val = TU_CUR_VERSION;
+			*val = test_bit(TU_FLAG_IN_PROCESS, &tu->flags) ?
+					tu->regs[TU_REG_CMD] : 0;
 		break;
 	}
 
-- 
2.43.0

Re: [PATCH v2 4/4] i2c: testunit: return current command on read messages

[Wolfram Sang]

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

On Sun, Aug 11, 2024 at 11:23:16PM +0200, Wolfram Sang wrote:
> Because the testunit can start tests in the future via the DELAY
> register, it may happen that a command is still pending. Support
> detecting that by returning the number of a command in progress (if
> there is one).
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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