[v3] i2c: sh_mobile: implement atomic transfers

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!

This is a rebased version of v2 with a minor issue fixed. It does not
resolve the runtime PM issue that may arise (see "watchdog: da9063: wake up
parent ahead of reboot", https://patchwork.kernel.org/patch/11749121/ ), but
in practice it works, and our understanding so far is that this will have to
be resolved outside this driver and should IMO not block this patch.

CU
Uli


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 | 96 ++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 25 deletions(-)

> This is a rebased version of v2 with a minor issue fixed. It does not

> resolve the runtime PM issue that may arise (see "watchdog: da9063: wake up

> parent ahead of reboot", https://patchwork.kernel.org/patch/11749121/ ), but

> in practice it works, and our understanding so far is that this will have to

> be resolved outside this driver and should IMO not block this patch.


I totally agree. Other opinions?

Hi Uli,

On Tue, Sep 22, 2020 at 5:49 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>
>
> This is a rebased version of v2 with a minor issue fixed. It does not
> resolve the runtime PM issue that may arise (see "watchdog: da9063: wake up
> parent ahead of reboot", https://patchwork.kernel.org/patch/11749121/ ), but
> in practice it works, and our understanding so far is that this will have to
> be resolved outside this driver and should IMO not block this patch.

See my comment below.

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

Thanks for the update!

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

> @@ -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,12 +585,14 @@ 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;

and be done with it?

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

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

To make sure external conditions are satisfied, and we never deadlock:

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

Perhaps this can even be done in the i2c core instead?

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

> To make sure external conditions are satisfied, and we never deadlock:
> 
>     if (pd->dev->power.is_suspended)
>             return -EPERM;  /* any other suitable error code? */
> 
> Perhaps this can even be done in the i2c core instead?

Good question! My gut feeling says "i2c core", but that would need more
research. I'd say we tackle this in a seperate patch but also for the
next merge window. Sounds good?

Hi Wolfram,

On Wed, Sep 23, 2020 at 6:06 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > To make sure external conditions are satisfied, and we never deadlock:
> >
> >     if (pd->dev->power.is_suspended)
> >             return -EPERM;  /* any other suitable error code? */
> >
> > Perhaps this can even be done in the i2c core instead?
>
> Good question! My gut feeling says "i2c core", but that would need more
> research. I'd say we tackle this in a seperate patch but also for the
> next merge window. Sounds good?

Fine for me, it's already late in the v5.9-rc cycle.

Gr{oetje,eeting}s,

                        Geert