[PATCH] i2c: sh_mobile: implement atomic transfers

[PATCH] i2c: sh_mobile: implement atomic transfers

[Ulrich Hecht]

Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
similar boards.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 100 +++++++++++++++++++++++++++----------
 1 file changed, 74 insertions(+), 26 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index d83ca40..e8436f4 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -129,6 +129,7 @@ struct sh_mobile_i2c_data {
 	int sr;
 	bool send_stop;
 	bool stop_after_dma;
+	bool atomic_xfer;
 
 	struct resource *res;
 	struct dma_chan *dma_tx;
@@ -330,13 +331,15 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op
 		ret = iic_rd(pd, ICDR);
 		break;
 	case OP_RX_STOP: /* enable DTE interrupt, issue stop */
-		iic_wr(pd, ICIC,
-		       ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
+		if (!pd->atomic_xfer)
+			iic_wr(pd, ICIC,
+			       ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
 		iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK);
 		break;
 	case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */
-		iic_wr(pd, ICIC,
-		       ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
+		if (!pd->atomic_xfer)
+			iic_wr(pd, ICIC,
+			       ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
 		ret = iic_rd(pd, ICDR);
 		iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK);
 		break;
@@ -366,7 +369,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 
 static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
 {
-	unsigned char data;
+	unsigned char data = 0;
 	int real_pos;
 
 	/* switch from TX (address) to RX (data) adds two interrupts */
@@ -432,7 +435,8 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 
 	if (wakeup) {
 		pd->sr |= SW_DONE;
-		wake_up(&pd->wait);
+		if (!pd->atomic_xfer)
+			wake_up(&pd->wait);
 	}
 
 	/* defeat write posting to avoid spurious WAIT interrupts */
@@ -584,12 +588,14 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 	pd->pos = -1;
 	pd->sr = 0;
 
-	pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
-	if (pd->dma_buf)
-		sh_mobile_i2c_xfer_dma(pd);
-
-	/* Enable all interrupts to begin with */
-	iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
+	if (!pd->atomic_xfer) {
+		pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
+		if (pd->dma_buf)
+			sh_mobile_i2c_xfer_dma(pd);
+		/* Enable all interrupts to begin with */
+		iic_wr(pd, ICIC,
+		       ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
+	}
 }
 
 static int poll_dte(struct sh_mobile_i2c_data *pd)
@@ -640,18 +646,16 @@ static int poll_busy(struct sh_mobile_i2c_data *pd)
 	return i ? 0 : -ETIMEDOUT;
 }
 
-static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
-			      struct i2c_msg *msgs,
-			      int num)
+static int xfer(struct sh_mobile_i2c_data *pd, struct i2c_msg *msgs, int num)
 {
-	struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter);
 	struct i2c_msg	*msg;
 	int err = 0;
 	int i;
 	long timeout;
 
 	/* Wake up device and enable clock */
-	pm_runtime_get_sync(pd->dev);
+	if (!pd->atomic_xfer)
+		pm_runtime_get_sync(pd->dev);
 
 	/* Process all messages */
 	for (i = 0; i < num; i++) {
@@ -665,13 +669,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 		if (do_start)
 			i2c_op(pd, OP_START);
 
-		/* The interrupt handler takes care of the rest... */
-		timeout = wait_event_timeout(pd->wait,
-				       pd->sr & (ICSR_TACK | SW_DONE),
-				       adapter->timeout);
+		if (pd->atomic_xfer) {
+			unsigned long j = jiffies + pd->adap.timeout;
+
+			timeout = 1;
+			while (!time_after(jiffies, j) &&
+			       !(pd->sr & (ICSR_TACK | SW_DONE))) {
+				unsigned char sr = iic_rd(pd, ICSR);
+
+				if (sr & (ICSR_AL   | ICSR_TACK |
+					  ICSR_WAIT | ICSR_DTE)) {
+					sh_mobile_i2c_isr(0, pd);
+					udelay(150);
+				} else
+					cpu_relax();
+			}
+
+			if (time_after(jiffies, j))
+				timeout = 0;
+		} else {
+			/* The interrupt handler takes care of the rest... */
+			timeout = wait_event_timeout(pd->wait,
+					pd->sr & (ICSR_TACK | SW_DONE),
+					pd->adap.timeout);
 
-		/* 'stop_after_dma' tells if DMA transfer was complete */
-		i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma);
+			/* 'stop_after_dma' tells if DMA xfer was complete */
+			i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg,
+						 pd->stop_after_dma);
+
+		}
 
 		if (!timeout) {
 			dev_err(pd->dev, "Transfer request timed out\n");
@@ -694,19 +720,41 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 	iic_wr(pd, ICCR, ICCR_SCP);
 
 	/* Disable clock and mark device as idle */
-	pm_runtime_put_sync(pd->dev);
+	if (!pd->atomic_xfer)
+		pm_runtime_put_sync(pd->dev);
 
 	return err ?: num;
 }
 
+static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
+			      struct i2c_msg *msgs,
+			      int num)
+{
+	struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter);
+
+	pd->atomic_xfer = false;
+	return xfer(pd, msgs, num);
+}
+
+static int sh_mobile_i2c_xfer_atomic(struct i2c_adapter *adapter,
+				     struct i2c_msg *msgs,
+				     int num)
+{
+	struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter);
+
+	pd->atomic_xfer = true;
+	return xfer(pd, msgs, num);
+}
+
 static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING;
 }
 
 static const struct i2c_algorithm sh_mobile_i2c_algorithm = {
-	.functionality	= sh_mobile_i2c_func,
-	.master_xfer	= sh_mobile_i2c_xfer,
+	.functionality		= sh_mobile_i2c_func,
+	.master_xfer		= sh_mobile_i2c_xfer,
+	.master_xfer_atomic	= sh_mobile_i2c_xfer_atomic,
 };
 
 static const struct i2c_adapter_quirks sh_mobile_i2c_quirks = {
-- 
2.7.4

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang]

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

On Wed, Jun 10, 2020 at 09:33:11PM +0200, Ulrich Hecht wrote:
> Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
> similar boards.
> 
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks, Uli! Works fine here. Really nice to finally being able to
reboot now without WARNings.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Some review comments:


> @@ -366,7 +369,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
>  
>  static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
>  {
> -	unsigned char data;
> +	unsigned char data = 0;

Please rebase against i2c/for-next. 'data' is gone since recently.

> -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
> -			      struct i2c_msg *msgs,
> -			      int num)
> +static int xfer(struct sh_mobile_i2c_data *pd, struct i2c_msg *msgs, int num)

'xfer' is too generic IMO. '__sh_mobile_i2c_xfer' maybe?

> -	pm_runtime_get_sync(pd->dev);
> +	if (!pd->atomic_xfer)
> +		pm_runtime_get_sync(pd->dev);

This was a small surprise to me. I assume RPM is disabled that late?
But can we be sure the clock is on, then?

> +		if (pd->atomic_xfer) {
> +			unsigned long j = jiffies + pd->adap.timeout;
> +
> +			timeout = 1;
> +			while (!time_after(jiffies, j) &&

To avoid the negation, maybe 'time_before_eq(...)'?

> +			       !(pd->sr & (ICSR_TACK | SW_DONE))) {
> +				unsigned char sr = iic_rd(pd, ICSR);
> +
> +				if (sr & (ICSR_AL   | ICSR_TACK |
> +					  ICSR_WAIT | ICSR_DTE)) {
> +					sh_mobile_i2c_isr(0, pd);
> +					udelay(150);
> +				} else
> +					cpu_relax();

Braces for else block.

> +			}
> +
> +			if (time_after(jiffies, j))
> +				timeout = 0;

Uhh, 'timeout' should have been named 'time_left' back then. Then, this
all would be more readable and we could do here:

	time_left = time_before_eq(...);

and avoid both 'timeout' assignments above.

> +static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
> +			      struct i2c_msg *msgs,
> +			      int num)
> +{
> +	struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter);
> +
> +	pd->atomic_xfer = false;


Maybe move this above to the xfer function ...

> +	return xfer(pd, msgs, num);


... and have here only:

	return __sh_mobile_i2c_xfer(adapter, msgs, num, false);

But yeah, this is bike-shedding. I don't mind much.

>  static const struct i2c_algorithm sh_mobile_i2c_algorithm = {
> -	.functionality	= sh_mobile_i2c_func,
> -	.master_xfer	= sh_mobile_i2c_xfer,
> +	.functionality		= sh_mobile_i2c_func,
> +	.master_xfer		= sh_mobile_i2c_xfer,
> +	.master_xfer_atomic	= sh_mobile_i2c_xfer_atomic,

Just convert to a single space before the '='.

All the best,

   Wolfram


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

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Geert Uytterhoeven]

Hi Uli,

On Wed, Jun 10, 2020 at 10:19 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote:
> Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
> similar boards.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

After removing the first hunk, this works fine on r8a7791/koelsch.
The following warnings are no longer seen:

    WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:41 i2c_transfer+0x80/0xf8
    No atomic I2C transfer handler for 'i2c-6'
    ...
    WARNING: CPU: 0 PID: 1 at kernel/sched/core.c:1731 set_task_cpu+0xf4/0x1fc

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Geert Uytterhoeven]

Hi Wolfram, Uli,

On Sun, Jun 14, 2020 at 11:31 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Wed, Jun 10, 2020 at 09:33:11PM +0200, Ulrich Hecht wrote:
> > Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
> > similar boards.
> >
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
>
> Thanks, Uli! Works fine here. Really nice to finally being able to
> reboot now without WARNings.

> > -     pm_runtime_get_sync(pd->dev);
> > +     if (!pd->atomic_xfer)
> > +             pm_runtime_get_sync(pd->dev);
>
> This was a small surprise to me. I assume RPM is disabled that late?
> But can we be sure the clock is on, then?

And the power domain, as this is for i2c-sh_mobile.c.
On SH/R-Mobile SoCs, the i2c block is part of a power area.
Most are part of A3SP (which is never really disabled due to being
shared with other devices), or C5 (always-on), but i2c0 on R-Mobile A1
is part of A4R, which is usually suspended.

Uli: can you check if atomic transfers work with the touchscreen or
codec on Armadillo-800-EVA?

Thanks!


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang]

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


> Uli: can you check if atomic transfers work with the touchscreen or
> codec on Armadillo-800-EVA?

Atmoic transfers are used for very late communication (e.g. PMIC).
Touchscreens and codecs should be already down, then.


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

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang]

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


> After removing the first hunk, this works fine on r8a7791/koelsch.

Removing the first hunk? This one?

+       bool atomic_xfer;


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

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jun 15, 2020 at 2:50 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > After removing the first hunk, this works fine on r8a7791/koelsch.
>
> Removing the first hunk? This one?
>
> +       bool atomic_xfer;

Oops, no.  I hadn't looked that close at the numbers, and assumed the failure
was about the first hunk:

Hunk #3 FAILED at 369.
Hunk #4 succeeded at 432 (offset -3 lines).
Hunk #5 succeeded at 585 (offset -3 lines).
Hunk #6 succeeded at 643 (offset -3 lines).
Hunk #7 succeeded at 666 (offset -3 lines).
Hunk #8 succeeded at 717 (offset -3 lines).
1 out of 8 hunks FAILED -- saving rejects to file
drivers/i2c/busses/i2c-sh_mobile.c.rej

I meant the one with the "data" var.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Jun 15, 2020 at 2:48 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > Uli: can you check if atomic transfers work with the touchscreen or
> > codec on Armadillo-800-EVA?
>
> Atmoic transfers are used for very late communication (e.g. PMIC).
> Touchscreens and codecs should be already down, then.

So how to test atomic transfers are working if the I2C controller is part of
a real power domain? Add a fake PMIC?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang]

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


> I meant the one with the "data" var.

Ah, okay. That makes sense.


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

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Wolfram Sang]

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

On Mon, Jun 15, 2020 at 02:58:59PM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Mon, Jun 15, 2020 at 2:48 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > Uli: can you check if atomic transfers work with the touchscreen or
> > > codec on Armadillo-800-EVA?
> >
> > Atmoic transfers are used for very late communication (e.g. PMIC).
> > Touchscreens and codecs should be already down, then.
> 
> So how to test atomic transfers are working if the I2C controller is part of
> a real power domain? Add a fake PMIC?

Hack something like this into a driver which is executed late:

	struct i2c_msg simple_msg = { ... }

	adap = i2c_get_adapter(0); // or whatever number
	i2c_transfer(adap, simple_msg, 1);

You don't need client struct, you can hardcode some address into
simple_msg directly.


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

Re: [PATCH] i2c: sh_mobile: implement atomic transfers

[Ulrich Hecht]

Thank you for the review.

> On June 14, 2020 11:31 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > -	pm_runtime_get_sync(pd->dev);
> > +	if (!pd->atomic_xfer)
> > +		pm_runtime_get_sync(pd->dev);
> 
> This was a small surprise to me. I assume RPM is disabled that late?
> But can we be sure the clock is on, then?

Turns out it's unnecessary to mess with PM handling here. I assumed that it must not be touched during atomic transfers, but that seems not to be the case. I'll drop that bit in the next revision.

CU
Uli