[PATCH v4 0/3] Add atomic transfers to s3c24xx i2c driver

[PATCH v4 0/3] Add atomic transfers to s3c24xx i2c driver

[Marek Szyprowski]

Dear All,

This patchset adds support for atomic transfers, which has been added to
the i2c core recently by the commit 63b96983a5dd ("i2c: core: introduce
callbacks for atomic transfers") to hide warnings observed during system
reboot and power-off. Almost everything needed for that was already in
the driver as so called polling mode. Unfortunately, that polling mode
has been tested only with single message, write transfers so far and it
turned out that it doesn't work well with read and multi-message
transfers, so first it had to be fixed.

Best regards,
Marek Szyprowski


Changelog:
v4:
- added a comment about the delay value

v3:
- fixed style issue pointed by Andi, extended commit message

v2:
- updated and extended commit messages


Patch summary:

Marek Szyprowski (3):
  i2c: s3c24xx: fix read transfers in polling mode
  i2c: s3c24xx: fix transferring more than one message in polling mode
  i2c: s3c24xx: add support for atomic transfers

 drivers/i2c/busses/i2c-s3c2410.c | 61 +++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 21 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] i2c: s3c24xx: fix read transfers in polling mode

[Marek Szyprowski]

To properly handle read transfers in polling mode, no waiting for the ACK
state is needed as it will never come. Just wait a bit to ensure start
state is on the bus and continue processing next bytes.

Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Chanho Park <chanho61.park@samsung.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-s3c2410.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index c56886af724e..bf9a5670ef33 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -216,8 +216,17 @@ static bool is_ack(struct s3c24xx_i2c *i2c)
 	int tries;
 
 	for (tries = 50; tries; --tries) {
-		if (readl(i2c->regs + S3C2410_IICCON)
-			& S3C2410_IICCON_IRQPEND) {
+		unsigned long tmp = readl(i2c->regs + S3C2410_IICCON);
+
+		if (!(tmp & S3C2410_IICCON_ACKEN)) {
+			/*
+			 * Wait a bit for the bus to stabilize,
+			 * delay estimated experimentally.
+			 */
+			usleep_range(100, 200);
+			return true;
+		}
+		if (tmp & S3C2410_IICCON_IRQPEND) {
 			if (!(readl(i2c->regs + S3C2410_IICSTAT)
 				& S3C2410_IICSTAT_LASTBIT))
 				return true;
-- 
2.34.1

[PATCH v4 2/3] i2c: s3c24xx: fix transferring more than one message in polling mode

[Marek Szyprowski]

To properly handle ACK on the bus when transferring more than one
message in polling mode, move the polling handling loop from
s3c24xx_i2c_message_start() to s3c24xx_i2c_doxfer(). This way
i2c_s3c_irq_nextbyte() is always executed till the end, properly
acknowledging the IRQ bits and no recursive calls to
i2c_s3c_irq_nextbyte() are made.

While touching this, also fix finishing transfers in polling mode by
using common code path and always waiting for the bus to become idle
and disabled.

Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-s3c2410.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index bf9a5670ef33..c0fe96a4f2c4 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -279,16 +279,6 @@ static void s3c24xx_i2c_message_start(struct s3c24xx_i2c *i2c,
 
 	stat |= S3C2410_IICSTAT_START;
 	writel(stat, i2c->regs + S3C2410_IICSTAT);
-
-	if (i2c->quirks & QUIRK_POLL) {
-		while ((i2c->msg_num != 0) && is_ack(i2c)) {
-			i2c_s3c_irq_nextbyte(i2c, stat);
-			stat = readl(i2c->regs + S3C2410_IICSTAT);
-
-			if (stat & S3C2410_IICSTAT_ARBITR)
-				dev_err(i2c->dev, "deal with arbitration loss\n");
-		}
-	}
 }
 
 static inline void s3c24xx_i2c_stop(struct s3c24xx_i2c *i2c, int ret)
@@ -694,7 +684,7 @@ static void s3c24xx_i2c_wait_idle(struct s3c24xx_i2c *i2c)
 static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 			      struct i2c_msg *msgs, int num)
 {
-	unsigned long timeout;
+	unsigned long timeout = 0;
 	int ret;
 
 	ret = s3c24xx_i2c_set_master(i2c);
@@ -714,16 +704,19 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	s3c24xx_i2c_message_start(i2c, msgs);
 
 	if (i2c->quirks & QUIRK_POLL) {
-		ret = i2c->msg_idx;
+		while ((i2c->msg_num != 0) && is_ack(i2c)) {
+			unsigned long stat = readl(i2c->regs + S3C2410_IICSTAT);
 
-		if (ret != num)
-			dev_dbg(i2c->dev, "incomplete xfer (%d)\n", ret);
+			i2c_s3c_irq_nextbyte(i2c, stat);
 
-		goto out;
+			stat = readl(i2c->regs + S3C2410_IICSTAT);
+			if (stat & S3C2410_IICSTAT_ARBITR)
+				dev_err(i2c->dev, "deal with arbitration loss\n");
+		}
+	} else {
+		timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
 	}
 
-	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);
-
 	ret = i2c->msg_idx;
 
 	/*
-- 
2.34.1

[PATCH v4 3/3] i2c: s3c24xx: add support for atomic transfers

[Marek Szyprowski]

Add support for atomic transfers using polling mode with interrupts
intentionally disabled to get rid of the following warning introduced by
commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
transfers") during system reboot and power-off:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1518 at drivers/i2c/i2c-core.h:40 i2c_transfer+0xe8/0xf4
No atomic I2C transfer handler for 'i2c-0'
Modules linked in:
CPU: 0 PID: 1518 Comm: reboot Not tainted 6.6.0-next-20231031 #7453
Hardware name: Samsung Exynos (Flattened Device Tree)
 unwind_backtrace from show_stack+0x10/0x14
 show_stack from dump_stack_lvl+0x40/0x4c
 dump_stack_lvl from __warn+0x7c/0x124
 __warn from warn_slowpath_fmt+0x110/0x178
 warn_slowpath_fmt from i2c_transfer+0xe8/0xf4
 i2c_transfer from regmap_i2c_read+0x58/0x88
 regmap_i2c_read from _regmap_raw_read+0xfc/0x260
 _regmap_raw_read from _regmap_bus_read+0x44/0x70
 _regmap_bus_read from _regmap_read+0x60/0x14c
 _regmap_read from regmap_read+0x3c/0x60
 regmap_read from regulator_get_voltage_sel_regmap+0x2c/0x74
 regulator_get_voltage_sel_regmap from regulator_get_voltage_rdev+0x64/0x15c
 regulator_get_voltage_rdev from _regulator_do_set_voltage+0x2c/0x5a8
 _regulator_do_set_voltage from regulator_set_voltage_rdev+0x90/0x248
 regulator_set_voltage_rdev from regulator_do_balance_voltage+0x350/0x4d0
 regulator_do_balance_voltage from regulator_set_voltage_unlocked+0xd4/0x118
 regulator_set_voltage_unlocked from regulator_set_voltage+0x40/0x74
 regulator_set_voltage from _opp_config_regulator_single+0x44/0x110
 _opp_config_regulator_single from _set_opp+0x118/0x500
 _set_opp from dev_pm_opp_set_rate+0x108/0x20c
 dev_pm_opp_set_rate from __cpufreq_driver_target+0x568/0x6cc
 __cpufreq_driver_target from cpufreq_generic_suspend+0x28/0x50
 cpufreq_generic_suspend from cpufreq_suspend+0xbc/0x124
 cpufreq_suspend from device_shutdown+0x18/0x230
 device_shutdown from kernel_restart+0x38/0x90
 kernel_restart from __do_sys_reboot+0x12c/0x1f8
 __do_sys_reboot from ret_fast_syscall+0x0/0x54
Exception stack(0xf0fedfa8 to 0xf0fedff0)
...
---[ end trace 0000000000000000 ]---

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Chanho Park <chanho61.park@samsung.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-s3c2410.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index c0fe96a4f2c4..275f7c42165c 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -76,6 +76,7 @@
 #define QUIRK_HDMIPHY		(1 << 1)
 #define QUIRK_NO_GPIO		(1 << 2)
 #define QUIRK_POLL		(1 << 3)
+#define QUIRK_ATOMIC		(1 << 4)
 
 /* Max time to wait for bus to become idle after a xfer (in us) */
 #define S3C2410_IDLE_TIMEOUT	5000
@@ -174,7 +175,7 @@ static inline void s3c24xx_i2c_master_complete(struct s3c24xx_i2c *i2c, int ret)
 	if (ret)
 		i2c->msg_idx = ret;
 
-	if (!(i2c->quirks & QUIRK_POLL))
+	if (!(i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)))
 		wake_up(&i2c->wait);
 }
 
@@ -703,7 +704,7 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	s3c24xx_i2c_enable_irq(i2c);
 	s3c24xx_i2c_message_start(i2c, msgs);
 
-	if (i2c->quirks & QUIRK_POLL) {
+	if (i2c->quirks & (QUIRK_POLL | QUIRK_ATOMIC)) {
 		while ((i2c->msg_num != 0) && is_ack(i2c)) {
 			unsigned long stat = readl(i2c->regs + S3C2410_IICSTAT);
 
@@ -775,6 +776,21 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	return -EREMOTEIO;
 }
 
+static int s3c24xx_i2c_xfer_atomic(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num)
+{
+	struct s3c24xx_i2c *i2c = (struct s3c24xx_i2c *)adap->algo_data;
+	int ret;
+
+	disable_irq(i2c->irq);
+	i2c->quirks |= QUIRK_ATOMIC;
+	ret = s3c24xx_i2c_xfer(adap, msgs, num);
+	i2c->quirks &= ~QUIRK_ATOMIC;
+	enable_irq(i2c->irq);
+
+	return ret;
+}
+
 /* declare our i2c functionality */
 static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
 {
@@ -785,6 +801,7 @@ static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
 /* i2c bus registration info */
 static const struct i2c_algorithm s3c24xx_i2c_algorithm = {
 	.master_xfer		= s3c24xx_i2c_xfer,
+	.master_xfer_atomic     = s3c24xx_i2c_xfer_atomic,
 	.functionality		= s3c24xx_i2c_func,
 };
 
-- 
2.34.1

Re: [PATCH v4 0/3] Add atomic transfers to s3c24xx i2c driver

[Wolfram Sang]

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

On Wed, Nov 08, 2023 at 05:43:51PM +0100, Marek Szyprowski wrote:
> Dear All,
> 
> This patchset adds support for atomic transfers, which has been added to
> the i2c core recently by the commit 63b96983a5dd ("i2c: core: introduce
> callbacks for atomic transfers") to hide warnings observed during system
> reboot and power-off. Almost everything needed for that was already in
> the driver as so called polling mode. Unfortunately, that polling mode
> has been tested only with single message, write transfers so far and it
> turned out that it doesn't work well with read and multi-message
> transfers, so first it had to be fixed.
> 
> Best regards,
> Marek Szyprowski
> 
> 
> Changelog:
> v4:
> - added a comment about the delay value
> 
> v3:
> - fixed style issue pointed by Andi, extended commit message
> 
> v2:
> - updated and extended commit messages
> 
> 
> Patch summary:
> 
> Marek Szyprowski (3):
>   i2c: s3c24xx: fix read transfers in polling mode
>   i2c: s3c24xx: fix transferring more than one message in polling mode
>   i2c: s3c24xx: add support for atomic transfers
> 

Applied to for-next, thanks!


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