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@xxxxxxxx>
> Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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:
signature.asc
Description: PGP signature
