| Message ID | 1591817591-852-1-git-send-email-uli+renesas@fpond.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: sh_mobile: implement atomic transfers | expand |
On Wed, Jun 10, 2020 at 09:33:11PM +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> Thanks, Uli! Works fine here. Really nice to finally being able to reboot now without WARNings. Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Some review comments: > @@ -366,7 +369,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd) > > static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd) > { > - unsigned char data; > + unsigned char data = 0; Please rebase against i2c/for-next. 'data' is gone since recently. > -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, > - struct i2c_msg *msgs, > - int num) > +static int xfer(struct sh_mobile_i2c_data *pd, struct i2c_msg *msgs, int num) 'xfer' is too generic IMO. '__sh_mobile_i2c_xfer' maybe? > - pm_runtime_get_sync(pd->dev); > + if (!pd->atomic_xfer) > + pm_runtime_get_sync(pd->dev); This was a small surprise to me. I assume RPM is disabled that late? But can we be sure the clock is on, then? > + if (pd->atomic_xfer) { > + unsigned long j = jiffies + pd->adap.timeout; > + > + timeout = 1; > + while (!time_after(jiffies, j) && To avoid the negation, maybe 'time_before_eq(...)'? > + !(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(); Braces for else block. > + } > + > + if (time_after(jiffies, j)) > + timeout = 0; Uhh, 'timeout' should have been named 'time_left' back then. Then, this all would be more readable and we could do here: time_left = time_before_eq(...); and avoid both 'timeout' assignments above. > +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; Maybe move this above to the xfer function ... > + return xfer(pd, msgs, num); ... and have here only: return __sh_mobile_i2c_xfer(adapter, msgs, num, false); But yeah, this is bike-shedding. I don't mind much. > 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, Just convert to a single space before the '='. All the best, Wolfram
Hi Uli, On Wed, Jun 10, 2020 at 10:19 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> After removing the first hunk, this works fine on r8a7791/koelsch. The following warnings are no longer seen: WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:41 i2c_transfer+0x80/0xf8 No atomic I2C transfer handler for 'i2c-6' ... WARNING: CPU: 0 PID: 1 at kernel/sched/core.c:1731 set_task_cpu+0xf4/0x1fc Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Wolfram, Uli, On Sun, Jun 14, 2020 at 11:31 AM Wolfram Sang <wsa@the-dreams.de> wrote: > On Wed, Jun 10, 2020 at 09:33:11PM +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> > > Thanks, Uli! Works fine here. Really nice to finally being able to > reboot now without WARNings. > > - pm_runtime_get_sync(pd->dev); > > + if (!pd->atomic_xfer) > > + pm_runtime_get_sync(pd->dev); > > This was a small surprise to me. I assume RPM is disabled that late? > But can we be sure the clock is on, then? And the power domain, as this is for i2c-sh_mobile.c. On SH/R-Mobile SoCs, the i2c block is part of a power area. Most are part of A3SP (which is never really disabled due to being shared with other devices), or C5 (always-on), but i2c0 on R-Mobile A1 is part of A4R, which is usually suspended. Uli: can you check if atomic transfers work with the touchscreen or codec on Armadillo-800-EVA? Thanks! Gr{oetje,eeting}s, Geert
> Uli: can you check if atomic transfers work with the touchscreen or > codec on Armadillo-800-EVA? Atmoic transfers are used for very late communication (e.g. PMIC). Touchscreens and codecs should be already down, then.
> After removing the first hunk, this works fine on r8a7791/koelsch.
Removing the first hunk? This one?
+ bool atomic_xfer;
Hi Wolfram, On Mon, Jun 15, 2020 at 2:50 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > After removing the first hunk, this works fine on r8a7791/koelsch. > > Removing the first hunk? This one? > > + bool atomic_xfer; Oops, no. I hadn't looked that close at the numbers, and assumed the failure was about the first hunk: Hunk #3 FAILED at 369. Hunk #4 succeeded at 432 (offset -3 lines). Hunk #5 succeeded at 585 (offset -3 lines). Hunk #6 succeeded at 643 (offset -3 lines). Hunk #7 succeeded at 666 (offset -3 lines). Hunk #8 succeeded at 717 (offset -3 lines). 1 out of 8 hunks FAILED -- saving rejects to file drivers/i2c/busses/i2c-sh_mobile.c.rej I meant the one with the "data" var. Gr{oetje,eeting}s, Geert
Hi Wolfram, On Mon, Jun 15, 2020 at 2:48 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > Uli: can you check if atomic transfers work with the touchscreen or > > codec on Armadillo-800-EVA? > > Atmoic transfers are used for very late communication (e.g. PMIC). > Touchscreens and codecs should be already down, then. So how to test atomic transfers are working if the I2C controller is part of a real power domain? Add a fake PMIC? Gr{oetje,eeting}s, Geert
> I meant the one with the "data" var.
Ah, okay. That makes sense.
On Mon, Jun 15, 2020 at 02:58:59PM +0200, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Mon, Jun 15, 2020 at 2:48 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > Uli: can you check if atomic transfers work with the touchscreen or > > > codec on Armadillo-800-EVA? > > > > Atmoic transfers are used for very late communication (e.g. PMIC). > > Touchscreens and codecs should be already down, then. > > So how to test atomic transfers are working if the I2C controller is part of > a real power domain? Add a fake PMIC? Hack something like this into a driver which is executed late: struct i2c_msg simple_msg = { ... } adap = i2c_get_adapter(0); // or whatever number i2c_transfer(adap, simple_msg, 1); You don't need client struct, you can hardcode some address into simple_msg directly.
Thank you for the review. > On June 14, 2020 11:31 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > - pm_runtime_get_sync(pd->dev); > > + if (!pd->atomic_xfer) > > + pm_runtime_get_sync(pd->dev); > > This was a small surprise to me. I assume RPM is disabled that late? > But can we be sure the clock is on, then? Turns out it's unnecessary to mess with PM handling here. I assumed that it must not be touched during atomic transfers, but that seems not to be the case. I'll drop that bit in the next revision. CU Uli
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index d83ca40..e8436f4 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; @@ -366,7 +369,7 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd) static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd) { - unsigned char data; + unsigned char data = 0; int real_pos; /* switch from TX (address) to RX (data) adds two interrupts */ @@ -432,7 +435,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 */ @@ -584,12 +588,14 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, pd->pos = -1; pd->sr = 0; - 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); + } } static int poll_dte(struct sh_mobile_i2c_data *pd) @@ -640,18 +646,16 @@ 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 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; /* Wake up device and enable clock */ - pm_runtime_get_sync(pd->dev); + if (!pd->atomic_xfer) + pm_runtime_get_sync(pd->dev); /* Process all messages */ for (i = 0; i < num; i++) { @@ -665,13 +669,35 @@ 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; + + timeout = 1; + while (!time_after(jiffies, j) && + !(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(); + } + + if (time_after(jiffies, j)) + timeout = 0; + } else { + /* The interrupt handler takes care of the rest... */ + timeout = wait_event_timeout(pd->wait, + pd->sr & (ICSR_TACK | SW_DONE), + pd->adap.timeout); - /* 'stop_after_dma' tells if DMA transfer was complete */ - i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma); + /* 'stop_after_dma' tells if DMA xfer was complete */ + i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, + pd->stop_after_dma); + + } if (!timeout) { dev_err(pd->dev, "Transfer request timed out\n"); @@ -694,19 +720,41 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, iic_wr(pd, ICCR, ICCR_SCP); /* Disable clock and mark device as idle */ - pm_runtime_put_sync(pd->dev); + if (!pd->atomic_xfer) + pm_runtime_put_sync(pd->dev); 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 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 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 = {
Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and similar boards. Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> --- drivers/i2c/busses/i2c-sh_mobile.c | 100 +++++++++++++++++++++++++++---------- 1 file changed, 74 insertions(+), 26 deletions(-)