diff mbox series

[v4] i2c: sh_mobile: implement atomic transfers

Message ID 20200928155950.1185-1-uli+renesas@fpond.eu (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show
Series [v4] i2c: sh_mobile: implement atomic transfers | expand

Commit Message

Ulrich Hecht Sept. 28, 2020, 3:59 p.m. UTC 
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(-)

Comments

Wolfram Sang Oct. 2, 2020, 3:44 p.m. UTC | #1 
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
Geert Uytterhoeven Oct. 5, 2020, 7:36 a.m. UTC | #2 
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 Oct. 5, 2020, 7:39 a.m. UTC | #3 
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
Wolfram Sang Oct. 6, 2020, 6:40 a.m. UTC | #4 
> 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.
Ulrich Hecht Oct. 6, 2020, 7:59 a.m. UTC | #5 
> 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
Wolfram Sang Oct. 8, 2020, 9:48 a.m. UTC | #6 
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!
Wolfram Sang Nov. 6, 2020, 2:27 p.m. UTC | #7 
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.
diff mbox series

Patch

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 = {