* [PATCH v2] i2c: sh_mobile: implement atomic transfers
@ 2020-06-18 15:05 Ulrich Hecht
2020-06-18 16:39 ` Geert Uytterhoeven
0 siblings, 1 reply; 10+ messages in thread
From: Ulrich Hecht @ 2020-06-18 15:05 UTC (permalink / raw)
To: linux-renesas-soc
Cc: wsa, geert, linux-i2c, Ulrich Hecht, Wolfram Sang, Geert Uytterhoeven
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 revision drops disabling runtime PM as it is unnecessary and implements
a couple of style and logic tweaks. Thanks to Geert and Wolfram for the review.
CU
Uli
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 | 95 ++++++++++++++++++++++--------
1 file changed, 70 insertions(+), 25 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 2cca1b21e26e..c863f9640fcc 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,12 +585,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)
@@ -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,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;
+
+ 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();
+ }
+ }
+ } 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 +720,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 = {
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-18 15:05 [PATCH v2] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
@ 2020-06-18 16:39 ` Geert Uytterhoeven
2020-06-21 11:39 ` Geert Uytterhoeven
2020-06-25 7:06 ` Wolfram Sang
0 siblings, 2 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-06-18 16:39 UTC (permalink / raw)
To: Ulrich Hecht; +Cc: Linux-Renesas, Wolfram Sang, Linux I2C, Wolfram Sang
Hi Uli,
On Thu, Jun 18, 2020 at 5:05 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>
Thanks for your patch!
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -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);
> + }
> }
>
> static int poll_dte(struct sh_mobile_i2c_data *pd)
> @@ -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);
pm_runtime_get_sync() is a wrapper around __pm_runtime_resume(), which
does:
might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe &&
dev->power.runtime_status != RPM_ACTIVE);
So if the device is not active (it is not), the might_sleep() is
triggered, and I expect a BUG splat.
However, with CONFIG_DEBUG_ATOMIC_SLEEP disabled (I disabled it on
koelsch, as it increases kernel size beyond the bootloader limit),
might_sleep() is a no-op, so nothing happens.
After enabling it (and disabling drm and media), still nothing...
It turns out ___might_sleep() does:
if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
!is_idle_task(current) && !current->non_block_count) ||
system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
oops_in_progress)
return;
and as per:
static inline bool i2c_in_atomic_xfer_mode(void)
{
return system_state > SYSTEM_RUNNING && irqs_disabled();
}
system_state > SYSTEM_RUNNING, and ___might_sleep() just ignores any
issues. Oops...
After removing that check, it starts complaining:
BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:281
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name:
systemd-shutdow
In general, pm_runtime_get_sync() is not safe to call from atomic
context.
For Renesas SoCs, I think both the power and clock domains are safe, as
the respective drivers don't sleep. The PM core might, though.
> @@ -662,15 +666,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;
> +
> + time_left = time_before_eq(jiffies, j);
> + while (time_left &&
Who's updating 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();
> + }
> + }
> + } 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);
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-18 16:39 ` Geert Uytterhoeven
@ 2020-06-21 11:39 ` Geert Uytterhoeven
2020-06-25 6:59 ` Wolfram Sang
2020-06-25 7:06 ` Wolfram Sang
1 sibling, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-06-21 11:39 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Linux-Renesas, Linux I2C, Wolfram Sang, Ulrich Hecht
Hi Wolfram,
On Thu, Jun 18, 2020 at 6:39 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Jun 18, 2020 at 5:05 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>
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -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);
>
> pm_runtime_get_sync() is a wrapper around __pm_runtime_resume(), which
> does:
>
> might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe &&
> dev->power.runtime_status != RPM_ACTIVE);
>
> So if the device is not active (it is not), the might_sleep() is
> triggered, and I expect a BUG splat.
> However, with CONFIG_DEBUG_ATOMIC_SLEEP disabled (I disabled it on
> koelsch, as it increases kernel size beyond the bootloader limit),
> might_sleep() is a no-op, so nothing happens.
> After enabling it (and disabling drm and media), still nothing...
>
> It turns out ___might_sleep() does:
>
> if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
> !is_idle_task(current) && !current->non_block_count) ||
> system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
> oops_in_progress)
> return;
>
> and as per:
>
> static inline bool i2c_in_atomic_xfer_mode(void)
> {
> return system_state > SYSTEM_RUNNING && irqs_disabled();
So i2c atomic transfers are really meant to be used during late system suspend
only, and not in atomic context before, when irqs_disabled() is true?
> }
>
> system_state > SYSTEM_RUNNING, and ___might_sleep() just ignores any
> issues. Oops...
> After removing that check, it starts complaining:
>
> BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:281
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name:
> systemd-shutdow
Perhaps we need a checker config option, to make sure the atomic transfer
operation is exercised at least once during boot?
I guess scanning the i2c bus is an unsafe operation, but there may be
something we can do in a safe way, without side effects (e.g. redoing
the first i2c read message using atomic transfers)?
Cfr. CONFIG_ARM_PSCI_CHECKER, which cycles through hotplug and suspend
operations during boot.
> In general, pm_runtime_get_sync() is not safe to call from atomic
> context.
> For Renesas SoCs, I think both the power and clock domains are safe, as
> the respective drivers don't sleep. The PM core might, though.
Do we need a way to let i2c slaves indicate they plan to use atomic
transfers later, so the i2c core can keep the i2c controller resumed?
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-21 11:39 ` Geert Uytterhoeven
@ 2020-06-25 6:59 ` Wolfram Sang
0 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2020-06-25 6:59 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Linux-Renesas, Linux I2C, Ulrich Hecht
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
Hi Geert,
> > static inline bool i2c_in_atomic_xfer_mode(void)
> > {
> > return system_state > SYSTEM_RUNNING && irqs_disabled();
>
> So i2c atomic transfers are really meant to be used during late system suspend
> only, and not in atomic context before, when irqs_disabled() is true?
Yes. It is all some time ago, I recall we agreed that there shouldn't be
any other I2C communication at irqs_disabled() stage.
> Perhaps we need a checker config option, to make sure the atomic transfer
> operation is exercised at least once during boot?
Testing I2C controllers (in various ways) is a well-desired feature :/
> Do we need a way to let i2c slaves indicate they plan to use atomic
> transfers later, so the i2c core can keep the i2c controller resumed?
I wanted to have this originally, but in the end I gave up on it. IIRC,
you don't want to whitelist a client in general, but only the very late
messages it sends. However, if a message needs to be flagged may be
board specific. It all looked messy and hard to configure, so we ended
up with what we have now.
Take all this with a grain of salt, it's been a while since the
discussions happened.
All the best,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-18 16:39 ` Geert Uytterhoeven
2020-06-21 11:39 ` Geert Uytterhoeven
@ 2020-06-25 7:06 ` Wolfram Sang
2020-06-25 8:18 ` Geert Uytterhoeven
1 sibling, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2020-06-25 7:06 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]
Hi Geert,
thanks for the review!
> > @@ -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?
I like Uli's version a tad better in case we ever add something for both
cases after the following if-block. But I don't care much, we could
change it later.
> > - 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);
> > + }
...
> After removing that check, it starts complaining:
>
> BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:281
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name:
> systemd-shutdow
>
> In general, pm_runtime_get_sync() is not safe to call from atomic
> context.
> For Renesas SoCs, I think both the power and clock domains are safe, as
> the respective drivers don't sleep. The PM core might, though.
Still, that sounds to me like we should protect these calls as in V1?
> > + time_left = time_before_eq(jiffies, j);
> > + while (time_left &&
>
> Who's updating time_left?
Good question :)
Kind regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-25 7:06 ` Wolfram Sang
@ 2020-06-25 8:18 ` Geert Uytterhoeven
2020-06-25 8:33 ` Wolfram Sang
2020-06-25 15:16 ` Wolfram Sang
0 siblings, 2 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-06-25 8:18 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
Hi Wolfram,
On Thu, Jun 25, 2020 at 9:06 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> ...
>
> > After removing that check, it starts complaining:
> >
> > BUG: sleeping function called from invalid context at
> > kernel/locking/mutex.c:281
> > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name:
> > systemd-shutdow
> >
> > In general, pm_runtime_get_sync() is not safe to call from atomic
> > context.
> > For Renesas SoCs, I think both the power and clock domains are safe, as
> > the respective drivers don't sleep. The PM core might, though.
>
> Still, that sounds to me like we should protect these calls as in V1?
And talk to the i2c controller while it is disabled?
That does seem to work on R-Car Gen2 (similar to SMP bringup accessing
registers of a disabled WDT?), though.
Needs testing on R-Mobile A1....
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-25 8:18 ` Geert Uytterhoeven
@ 2020-06-25 8:33 ` Wolfram Sang
2020-06-25 15:16 ` Wolfram Sang
1 sibling, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2020-06-25 8:33 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
[-- Attachment #1: Type: text/plain, Size: 160 bytes --]
> And talk to the i2c controller while it is disabled?
Hah, stupid me! Coming back from another topic, I overlooked this. You
are right, of course. Hmmm....
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-25 8:18 ` Geert Uytterhoeven
2020-06-25 8:33 ` Wolfram Sang
@ 2020-06-25 15:16 ` Wolfram Sang
2020-06-30 19:45 ` Wolfram Sang
1 sibling, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2020-06-25 15:16 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]
Hi Geert,
I spend some more thoughts on this.
> > > In general, pm_runtime_get_sync() is not safe to call from atomic
> > > context.
> > > For Renesas SoCs, I think both the power and clock domains are safe, as
> > > the respective drivers don't sleep. The PM core might, though.
> >
> > Still, that sounds to me like we should protect these calls as in V1?
I still think we should guard these calls just because it is not safe to
call them from atomic contexts.
> And talk to the i2c controller while it is disabled?
Is there maybe some "always-on" property which we could add to the
respective IIC clock?
> That does seem to work on R-Car Gen2 (similar to SMP bringup accessing
> registers of a disabled WDT?), though.
Yes. Uli's patch will not cause a regression because we are already
calling i2c_transfer very late. And we do call the runtime_pm functions
currently. So, it will improve the situation there.
> Needs testing on R-Mobile A1....
That's armadillo, right? I don't have that, sadly.
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-25 15:16 ` Wolfram Sang
@ 2020-06-30 19:45 ` Wolfram Sang
2020-06-30 19:58 ` Geert Uytterhoeven
0 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2020-06-30 19:45 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]
On Thu, Jun 25, 2020 at 05:16:58PM +0200, Wolfram Sang wrote:
> Hi Geert,
>
> I spend some more thoughts on this.
>
> > > > In general, pm_runtime_get_sync() is not safe to call from atomic
> > > > context.
> > > > For Renesas SoCs, I think both the power and clock domains are safe, as
> > > > the respective drivers don't sleep. The PM core might, though.
> > >
> > > Still, that sounds to me like we should protect these calls as in V1?
>
> I still think we should guard these calls just because it is not safe to
> call them from atomic contexts.
>
> > And talk to the i2c controller while it is disabled?
>
> Is there maybe some "always-on" property which we could add to the
> respective IIC clock?
Ping to this question...
>
> > That does seem to work on R-Car Gen2 (similar to SMP bringup accessing
> > registers of a disabled WDT?), though.
>
> Yes. Uli's patch will not cause a regression because we are already
> calling i2c_transfer very late. And we do call the runtime_pm functions
> currently. So, it will improve the situation there.
>
> > Needs testing on R-Mobile A1....
>
> That's armadillo, right? I don't have that, sadly.
>
> Thanks,
>
> Wolfram
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: sh_mobile: implement atomic transfers
2020-06-30 19:45 ` Wolfram Sang
@ 2020-06-30 19:58 ` Geert Uytterhoeven
0 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-06-30 19:58 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Ulrich Hecht, Linux-Renesas, Linux I2C
Hi Wolfram,
On Tue, Jun 30, 2020 at 9:45 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Jun 25, 2020 at 05:16:58PM +0200, Wolfram Sang wrote:
> > I spend some more thoughts on this.
> >
> > > > > In general, pm_runtime_get_sync() is not safe to call from atomic
> > > > > context.
> > > > > For Renesas SoCs, I think both the power and clock domains are safe, as
> > > > > the respective drivers don't sleep. The PM core might, though.
> > > >
> > > > Still, that sounds to me like we should protect these calls as in V1?
> >
> > I still think we should guard these calls just because it is not safe to
> > call them from atomic contexts.
> >
> > > And talk to the i2c controller while it is disabled?
> >
> > Is there maybe some "always-on" property which we could add to the
> > respective IIC clock?
>
> Ping to this question...
You mean in DT? DT describes hardware, not software policy.
Anyway, won't help on R-Mobile A1, as there's a real power domain.
> > > That does seem to work on R-Car Gen2 (similar to SMP bringup accessing
> > > registers of a disabled WDT?), though.
> >
> > Yes. Uli's patch will not cause a regression because we are already
> > calling i2c_transfer very late. And we do call the runtime_pm functions
> > currently. So, it will improve the situation there.
> >
> > > Needs testing on R-Mobile A1....
> >
> > That's armadillo, right? I don't have that, sadly.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-06-30 19:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18 15:05 [PATCH v2] i2c: sh_mobile: implement atomic transfers Ulrich Hecht
2020-06-18 16:39 ` Geert Uytterhoeven
2020-06-21 11:39 ` Geert Uytterhoeven
2020-06-25 6:59 ` Wolfram Sang
2020-06-25 7:06 ` Wolfram Sang
2020-06-25 8:18 ` Geert Uytterhoeven
2020-06-25 8:33 ` Wolfram Sang
2020-06-25 15:16 ` Wolfram Sang
2020-06-30 19:45 ` Wolfram Sang
2020-06-30 19:58 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).