Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
@ 2021-04-08  2:00 Marek Behún
  2021-04-10 16:47 ` Marek Behún
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Marek Behún @ 2021-04-08  2:00 UTC (permalink / raw)
  To: linux-i2c, Wolfram Sang, Gregory CLEMENT
  Cc: Samuel Holland, Ondrej Jirman, Marek Behún

I noticed a weird bug with this driver on Marvell CN9130 Customer
Reference Board.

Sometime after boot, the system locks with the following message:
 [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0

The system does not respond afterwards, only warns about RCU stalls.

This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
PM support").

With further experimentation I discovered that adding a delay into
mv64xxx_i2c_hw_init() fixes this issue. This function is called before
every xfer, due to how runtime PM works in this driver. It seems that in
order to work correctly, a delay is needed after the bus is reset in
this function.

Since there already is a known erratum with this controller needing a
delay, I assume that this is just another place this needs to be
applied. Therefore I apply the delay only if errata_delay is true.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index c590d36b5fd1..5c8e94b6cdb5 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -221,6 +221,10 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
 	writel(0, drv_data->reg_base + drv_data->reg_offsets.ext_addr);
 	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
 		drv_data->reg_base + drv_data->reg_offsets.control);
+
+	if (drv_data->errata_delay)
+		udelay(5);
+
 	drv_data->state = MV64XXX_I2C_STATE_IDLE;
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-08  2:00 [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM Marek Behún
@ 2021-04-10 16:47 ` Marek Behún
  2021-04-13 19:58 ` Wolfram Sang
  2021-04-15 20:13 ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: Marek Behún @ 2021-04-10 16:47 UTC (permalink / raw)
  To: linux-i2c, Wolfram Sang, Gregory CLEMENT; +Cc: Samuel Holland, Ondrej Jirman

On Thu,  8 Apr 2021 04:00:00 +0200
Marek Behún <kabel@kernel.org> wrote:

> e5c02cf54154 ("i2c: mv64xxx: Add runtime
> PM support").

This commit should also contain a Fixes tag:

Fixes: e5c02cf54154 ("i2c: mv64xxx: Add runtime PM support").

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-08  2:00 [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM Marek Behún
  2021-04-10 16:47 ` Marek Behún
@ 2021-04-13 19:58 ` Wolfram Sang
  2021-04-14 13:28   ` Gregory CLEMENT
  2021-04-14 13:42   ` Samuel Holland
  2021-04-15 20:13 ` Wolfram Sang
  2 siblings, 2 replies; 7+ messages in thread
From: Wolfram Sang @ 2021-04-13 19:58 UTC (permalink / raw)
  To: Marek Behún, Gregory CLEMENT
  Cc: linux-i2c, Samuel Holland, Ondrej Jirman


[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On Thu, Apr 08, 2021 at 04:00:00AM +0200, Marek Behún wrote:
> I noticed a weird bug with this driver on Marvell CN9130 Customer
> Reference Board.
> 
> Sometime after boot, the system locks with the following message:
>  [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0
> 
> The system does not respond afterwards, only warns about RCU stalls.
> 
> This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
> PM support").
> 
> With further experimentation I discovered that adding a delay into
> mv64xxx_i2c_hw_init() fixes this issue. This function is called before
> every xfer, due to how runtime PM works in this driver. It seems that in
> order to work correctly, a delay is needed after the bus is reset in
> this function.
> 
> Since there already is a known erratum with this controller needing a
> delay, I assume that this is just another place this needs to be
> applied. Therefore I apply the delay only if errata_delay is true.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Gregory? Looks reasonable to me and if so, we should have this in 5.12
already. Comments from others are welcome, too, of course.

> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index c590d36b5fd1..5c8e94b6cdb5 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -221,6 +221,10 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>  	writel(0, drv_data->reg_base + drv_data->reg_offsets.ext_addr);
>  	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
>  		drv_data->reg_base + drv_data->reg_offsets.control);
> +
> +	if (drv_data->errata_delay)
> +		udelay(5);
> +
>  	drv_data->state = MV64XXX_I2C_STATE_IDLE;
>  }
>  
> -- 
> 2.26.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-13 19:58 ` Wolfram Sang
@ 2021-04-14 13:28   ` Gregory CLEMENT
  2021-04-14 14:29     ` Marek Behún
  2021-04-14 13:42   ` Samuel Holland
  1 sibling, 1 reply; 7+ messages in thread
From: Gregory CLEMENT @ 2021-04-14 13:28 UTC (permalink / raw)
  To: Wolfram Sang, Marek Behún; +Cc: linux-i2c, Samuel Holland, Ondrej Jirman

> On Thu, Apr 08, 2021 at 04:00:00AM +0200, Marek Behún wrote:
>> I noticed a weird bug with this driver on Marvell CN9130 Customer
>> Reference Board.
>> 
>> Sometime after boot, the system locks with the following message:
>>  [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0
>> 
>> The system does not respond afterwards, only warns about RCU stalls.
>> 
>> This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
>> PM support").
>> 
>> With further experimentation I discovered that adding a delay into
>> mv64xxx_i2c_hw_init() fixes this issue. This function is called before
>> every xfer, due to how runtime PM works in this driver. It seems that in
>> order to work correctly, a delay is needed after the bus is reset in
>> this function.

Marek,

As you mentioned it was related to reset and the issue occurred with the
support of runtime PM. Did you try to add the delay only in the function
mv64xxx_i2c_runtime_resume(), just after the mv64xxx_i2c_hw_init() call ?

>> 
>> Since there already is a known erratum with this controller needing a
>> delay, I assume that this is just another place this needs to be
>> applied. Therefore I apply the delay only if errata_delay is true.
>> 
>> Signed-off-by: Marek Behún <kabel@kernel.org>
>
> Gregory? Looks reasonable to me and if so, we should have this in 5.12
> already. Comments from others are welcome, too, of course.

Hello Wolfram,

I don't have this specific platform. However, as you said it looks
reasonable and as it fixes an issue. And even if I had a pending
question, it is just an optimisation so you can add my

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Gregory


>
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index c590d36b5fd1..5c8e94b6cdb5 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -221,6 +221,10 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>>  	writel(0, drv_data->reg_base + drv_data->reg_offsets.ext_addr);
>>  	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
>>  		drv_data->reg_base + drv_data->reg_offsets.control);
>> +
>> +	if (drv_data->errata_delay)
>> +		udelay(5);
>> +
>>  	drv_data->state = MV64XXX_I2C_STATE_IDLE;
>>  }
>>  
>> -- 
>> 2.26.2
>> 

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-13 19:58 ` Wolfram Sang
  2021-04-14 13:28   ` Gregory CLEMENT
@ 2021-04-14 13:42   ` Samuel Holland
  1 sibling, 0 replies; 7+ messages in thread
From: Samuel Holland @ 2021-04-14 13:42 UTC (permalink / raw)
  To: Wolfram Sang, Marek Behún, Gregory CLEMENT, linux-i2c,
	Ondrej Jirman

On 4/13/21 2:58 PM, Wolfram Sang wrote:
> On Thu, Apr 08, 2021 at 04:00:00AM +0200, Marek Behún wrote:
>> I noticed a weird bug with this driver on Marvell CN9130 Customer
>> Reference Board.
>>
>> Sometime after boot, the system locks with the following message:
>>  [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0
>>
>> The system does not respond afterwards, only warns about RCU stalls.
>>
>> This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
>> PM support").
>>
>> With further experimentation I discovered that adding a delay into
>> mv64xxx_i2c_hw_init() fixes this issue. This function is called before
>> every xfer, due to how runtime PM works in this driver. It seems that in
>> order to work correctly, a delay is needed after the bus is reset in
>> this function.
>>
>> Since there already is a known erratum with this controller needing a
>> delay, I assume that this is just another place this needs to be
>> applied. Therefore I apply the delay only if errata_delay is true.
>>
>> Signed-off-by: Marek Behún <kabel@kernel.org>
> 
> Gregory? Looks reasonable to me and if so, we should have this in 5.12
> already. Comments from others are welcome, too, of course.

I also don't have an affected platform, so I did not notice this when adding
runtime PM. The change makes sense to me as well.

Reviewed-by: Samuel Holland <samuel@sholland.org>

Cheers,
Samuel

>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index c590d36b5fd1..5c8e94b6cdb5 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -221,6 +221,10 @@ mv64xxx_i2c_hw_init(struct mv64xxx_i2c_data *drv_data)
>>  	writel(0, drv_data->reg_base + drv_data->reg_offsets.ext_addr);
>>  	writel(MV64XXX_I2C_REG_CONTROL_TWSIEN | MV64XXX_I2C_REG_CONTROL_STOP,
>>  		drv_data->reg_base + drv_data->reg_offsets.control);
>> +
>> +	if (drv_data->errata_delay)
>> +		udelay(5);
>> +
>>  	drv_data->state = MV64XXX_I2C_STATE_IDLE;
>>  }
>>  
>> -- 
>> 2.26.2
>>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-14 13:28   ` Gregory CLEMENT
@ 2021-04-14 14:29     ` Marek Behún
  0 siblings, 0 replies; 7+ messages in thread
From: Marek Behún @ 2021-04-14 14:29 UTC (permalink / raw)
  To: Gregory CLEMENT; +Cc: Wolfram Sang, linux-i2c, Samuel Holland, Ondrej Jirman

On Wed, 14 Apr 2021 15:28:18 +0200
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> > On Thu, Apr 08, 2021 at 04:00:00AM +0200, Marek Behún wrote:  
> >> I noticed a weird bug with this driver on Marvell CN9130 Customer
> >> Reference Board.
> >> 
> >> Sometime after boot, the system locks with the following message:
> >>  [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0
> >> 
> >> The system does not respond afterwards, only warns about RCU stalls.
> >> 
> >> This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
> >> PM support").
> >> 
> >> With further experimentation I discovered that adding a delay into
> >> mv64xxx_i2c_hw_init() fixes this issue. This function is called before
> >> every xfer, due to how runtime PM works in this driver. It seems that in
> >> order to work correctly, a delay is needed after the bus is reset in
> >> this function.  
> 
> Marek,
> 
> As you mentioned it was related to reset and the issue occurred with the
> support of runtime PM. Did you try to add the delay only in the function
> mv64xxx_i2c_runtime_resume(), just after the mv64xxx_i2c_hw_init() call ?
> 

I did indeed discover this when I added this delay into the resume
function. In fact I discovered this when I added printf()s into suspend
and resume when debugging. The problem disappeared with these printf()s
(UART is slow so printf() counted as the necessary delay it seems).

I then moved the delay into the hw_init() function because that is what
made sense to me, that the delay is necessary after the reset, not only
when resuming, but always. We just did not notice because a xfer was
never done immediately after reset before the PM patch. (But maybe I am
wrong, maybe it is not needed in the reset. It just makes the most
sense to me...)

Marek

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM
  2021-04-08  2:00 [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM Marek Behún
  2021-04-10 16:47 ` Marek Behún
  2021-04-13 19:58 ` Wolfram Sang
@ 2021-04-15 20:13 ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2021-04-15 20:13 UTC (permalink / raw)
  To: Marek Behún
  Cc: linux-i2c, Gregory CLEMENT, Samuel Holland, Ondrej Jirman


[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Thu, Apr 08, 2021 at 04:00:00AM +0200, Marek Behún wrote:
> I noticed a weird bug with this driver on Marvell CN9130 Customer
> Reference Board.
> 
> Sometime after boot, the system locks with the following message:
>  [104.071363] i2c i2c-0: mv64xxx: I2C bus locked, block: 1, time_left: 0
> 
> The system does not respond afterwards, only warns about RCU stalls.
> 
> This first appeared with commit e5c02cf54154 ("i2c: mv64xxx: Add runtime
> PM support").
> 
> With further experimentation I discovered that adding a delay into
> mv64xxx_i2c_hw_init() fixes this issue. This function is called before
> every xfer, due to how runtime PM works in this driver. It seems that in
> order to work correctly, a delay is needed after the bus is reset in
> this function.
> 
> Since there already is a known erratum with this controller needing a
> delay, I assume that this is just another place this needs to be
> applied. Therefore I apply the delay only if errata_delay is true.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08  2:00 [PATCH] i2c: mv64xxx: Fix random system lock caused by runtime PM Marek Behún
2021-04-10 16:47 ` Marek Behún
2021-04-13 19:58 ` Wolfram Sang
2021-04-14 13:28   ` Gregory CLEMENT
2021-04-14 14:29     ` Marek Behún
2021-04-14 13:42   ` Samuel Holland
2021-04-15 20:13 ` Wolfram Sang

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git