linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] i2c: sh_mobile: implement atomic transfers
@ 2020-09-28 15:59 Ulrich Hecht
  2020-10-02 15:44 ` Wolfram Sang
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Ulrich Hecht @ 2020-09-28 15:59 UTC (permalink / raw)
  To: linux-renesas-soc
  Cc: wsa, geert, linux-i2c, Ulrich Hecht, Wolfram Sang, Geert Uytterhoeven

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

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

Hi!

Another minor change in the atomic code path in start_ch() as suggested by
Geert. All power management-related issues have been decided to be solved
outside this driver, so we should be done here.

CU
Uli


Changes since v3:
- cut atomic_xfer code path short in start_ch()

Changes since v2:
- rebase
- make sure time_left is updated

Changes since v1:
- don't disable runtime PM operations for atomic transfers
- rename xfer() to sh_mobile_xfer()
- rename timeout to time_left in sh_mobile_xfer() and simplify logic
- minor style tweaks
- rebase
- add Tested-by's



 drivers/i2c/busses/i2c-sh_mobile.c | 86 +++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index cab725559999..f253f82cbcc8 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;
@@ -429,7 +432,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 */
@@ -581,10 +585,12 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 	pd->pos = -1;
 	pd->sr = 0;
 
+	if (pd->atomic_xfer)
+		return;
+
 	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);
 }
@@ -637,15 +643,13 @@ 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 sh_mobile_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;
+	long time_left;
 
 	/* Wake up device and enable clock */
 	pm_runtime_get_sync(pd->dev);
@@ -662,15 +666,36 @@ 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;
+
+			time_left = time_before_eq(jiffies, j);
+			while (time_left &&
+			       !(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();
+				}
+				time_left = time_before_eq(jiffies, j);
+			}
+		} else {
+			/* The interrupt handler takes care of the rest... */
+			time_left = wait_event_timeout(pd->wait,
+					pd->sr & (ICSR_TACK | SW_DONE),
+					pd->adap.timeout);
+
+			/* 'stop_after_dma' tells if DMA xfer was complete */
+			i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg,
+						 pd->stop_after_dma);
 
-		/* 'stop_after_dma' tells if DMA transfer was complete */
-		i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma);
+		}
 
-		if (!timeout) {
+		if (!time_left) {
 			dev_err(pd->dev, "Transfer request timed out\n");
 			if (pd->dma_direction != DMA_NONE)
 				sh_mobile_i2c_cleanup_dma(pd);
@@ -696,14 +721,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 	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 sh_mobile_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 sh_mobile_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.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-09-28 15:59 [PATCH v4] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
@ 2020-10-02 15:44 ` Wolfram Sang
  2020-10-05  7:36   ` Geert Uytterhoeven
  2020-10-06  7:59   ` Ulrich Hecht
  2020-10-05  7:39 ` Geert Uytterhoeven
  2020-10-08  9:48 ` Wolfram Sang
  2 siblings, 2 replies; 8+ messages in thread
From: Wolfram Sang @ 2020-10-02 15:44 UTC (permalink / raw)
  To: Ulrich Hecht; +Cc: linux-renesas-soc, geert, linux-i2c, Geert Uytterhoeven

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

Hi Uli,

On Mon, Sep 28, 2020 at 05:59:50PM +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>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

It works, but I have two comments and two questions:

> @@ -581,10 +585,12 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
>  	pd->pos = -1;
>  	pd->sr = 0;
>  
> +	if (pd->atomic_xfer)
> +		return;
> +
>  	pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8);
>  	if (pd->dma_buf)
>  		sh_mobile_i2c_xfer_dma(pd);
> -

This blank line should stay.

...

> +		if (pd->atomic_xfer) {
> +			unsigned long j = jiffies + pd->adap.timeout;
> +
> +			time_left = time_before_eq(jiffies, j);
> +			while (time_left &&
> +			       !(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();
> +				}

Is it 100% safe to call cpu_relax() that late? Aren't interrupts
disabled? What is waking the CPU again? And where does the value 150us
come from?

> +				time_left = time_before_eq(jiffies, j);
> +			}
> +		} else {
> +			/* The interrupt handler takes care of the rest... */
> +			time_left = wait_event_timeout(pd->wait,
> +					pd->sr & (ICSR_TACK | SW_DONE),
> +					pd->adap.timeout);
> +
> +			/* 'stop_after_dma' tells if DMA xfer was complete */
> +			i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg,
> +						 pd->stop_after_dma);
>  

This blank line can go.

Thanks and regards,

   Wolfram


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-10-02 15:44 ` Wolfram Sang
@ 2020-10-05  7:36   ` Geert Uytterhoeven
  2020-10-06  7:59   ` Ulrich Hecht
  1 sibling, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C, Geert Uytterhoeven

Hi Wolfram,

On Fri, Oct 2, 2020 at 5:44 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Mon, Sep 28, 2020 at 05:59:50PM +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>

> > @@ -581,10 +585,12 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,

> > +             if (pd->atomic_xfer) {
> > +                     unsigned long j = jiffies + pd->adap.timeout;
> > +
> > +                     time_left = time_before_eq(jiffies, j);
> > +                     while (time_left &&
> > +                            !(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();
> > +                             }
>
> Is it 100% safe to call cpu_relax() that late? Aren't interrupts
> disabled? What is waking the CPU again? And where does the value 150us
> come from?

cpu_relax() does not sleep, usually it's merely a compiler directive.

On arm32/v7 (and most other platforms):

    #define cpu_relax()                     barrier()

    #define barrier() __asm__ __volatile__("": : :"memory")

On arm64:

    static inline void cpu_relax(void)
    {
            asm volatile("yield" ::: "memory");
    }

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-09-28 15:59 [PATCH v4] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
  2020-10-02 15:44 ` Wolfram Sang
@ 2020-10-05  7:39 ` Geert Uytterhoeven
  2020-10-06  6:40   ` Wolfram Sang
  2020-10-08  9:48 ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2020-10-05  7:39 UTC (permalink / raw)
  To: Ulrich Hecht; +Cc: Linux-Renesas, Wolfram Sang, Linux I2C, Wolfram Sang

Hi Uli,

On Mon, Sep 28, 2020 at 6:09 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>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>
> Hi!
>
> Another minor change in the atomic code path in start_ch() as suggested by
> Geert. All power management-related issues have been decided to be solved
> outside this driver, so we should be done here.
>
> CU
> Uli
>
>
> Changes since v3:
> - cut atomic_xfer code path short in start_ch()

Thanks for the update!

> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c

> +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);

To make sure external conditions are satisfied, and we never deadlock,
as discussed in v3?

    if (pd->dev->power.is_suspended)
            return -EPERM;  /* any other suitable error code? */

> +
> +       pd->atomic_xfer = true;
> +       return sh_mobile_xfer(pd, msgs, num);
> +}
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-10-05  7:39 ` Geert Uytterhoeven
@ 2020-10-06  6:40   ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2020-10-06  6:40 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C

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


> To make sure external conditions are satisfied, and we never deadlock,
> as discussed in v3?
> 
>     if (pd->dev->power.is_suspended)
>             return -EPERM;  /* any other suitable error code? */

Let's handle this seperately. I still think this could/should go into
the core but haven't had the time to investigate this further.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-10-02 15:44 ` Wolfram Sang
  2020-10-05  7:36   ` Geert Uytterhoeven
@ 2020-10-06  7:59   ` Ulrich Hecht
  1 sibling, 0 replies; 8+ messages in thread
From: Ulrich Hecht @ 2020-10-06  7:59 UTC (permalink / raw)
  To: Wolfram Sang, Ulrich Hecht
  Cc: linux-renesas-soc, geert, linux-i2c, Geert Uytterhoeven


> On 10/02/2020 5:44 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > +				if (sr & (ICSR_AL   | ICSR_TACK |
> > +					  ICSR_WAIT | ICSR_DTE)) {
> > +					sh_mobile_i2c_isr(0, pd);
> > +					udelay(150);
> > +				} else {
> 
> And where does the value 150us come from?

Anything more than (IIRC) 50us or so works, but I tried to be conservative. Not waiting at all does not work, though. It is not quite clear to me why, because the bits tested here are the conditions for an interrupt to be triggered.

CU
Uli

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-09-28 15:59 [PATCH v4] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
  2020-10-02 15:44 ` Wolfram Sang
  2020-10-05  7:39 ` Geert Uytterhoeven
@ 2020-10-08  9:48 ` Wolfram Sang
  2020-11-06 14:27   ` Wolfram Sang
  2 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2020-10-08  9:48 UTC (permalink / raw)
  To: Ulrich Hecht; +Cc: linux-renesas-soc, geert, linux-i2c, Geert Uytterhoeven

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

On Mon, Sep 28, 2020 at 05:59:50PM +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>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Fixed the blank line issues and applied to for-next, thanks!


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] i2c: sh_mobile: implement atomic transfers
  2020-10-08  9:48 ` Wolfram Sang
@ 2020-11-06 14:27   ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2020-11-06 14:27 UTC (permalink / raw)
  To: Ulrich Hecht; +Cc: linux-renesas-soc, geert, linux-i2c, Geert Uytterhoeven

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

On Thu, Oct 08, 2020 at 11:48:07AM +0200, Wolfram Sang wrote:
> On Mon, Sep 28, 2020 at 05:59:50PM +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>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Fixed the blank line issues and applied to for-next, thanks!

Sorry, I did something wrong somewhere so this was not in my pull
requests during the merge window. I'll put it back into for-current now.


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-11-06 14:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 15:59 [PATCH v4] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
2020-10-02 15:44 ` Wolfram Sang
2020-10-05  7:36   ` Geert Uytterhoeven
2020-10-06  7:59   ` Ulrich Hecht
2020-10-05  7:39 ` Geert Uytterhoeven
2020-10-06  6:40   ` Wolfram Sang
2020-10-08  9:48 ` Wolfram Sang
2020-11-06 14:27   ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).