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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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