[PATCH 0/5] i2c: tegra: Various fixes and improvements

[PATCH 0/5] i2c: tegra: Various fixes and improvements

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi Wolfram, Dmitry,

here's the full set of patches that I've been running local tests on and
which looks like it has no other issues. I'm going to run this through
our internal test system to make sure there aren't any regressions. I
can carry these in the Tegra tree for a little bit until we're
reasonable sure that these patches are all safe to apply and then send a
pull request sometime next week.

I've also included a patch as suggested by Dmitry to keep I2C interrupts
enabled during suspend/resume and I'm currently checking if that may be
a fix for a suspend/resume issue that I've seen on Jetson Nano where we
don't have working suspend/resume yet.

Thierry

Dmitry Osipenko (2):
  i2c: tegra: Better handle case where CPU0 is busy for a long time
  i2c: tegra: Synchronize DMA before termination

Thierry Reding (3):
  Revert "i2c: tegra: Fix suspending in active runtime PM state"
  i2c: tegra: Restore pinmux on system resume
  i2c: tegra: Keep IRQs enabled during suspend/resume

 drivers/i2c/busses/i2c-tegra.c | 61 +++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 26 deletions(-)

-- 
2.24.1

[PATCH 1/5] Revert "i2c: tegra: Fix suspending in active runtime PM state"

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This reverts commit 9f42de8d4ec2304f10bbc51dc0484f3503d61196.

It's not safe to use pm_runtime_force_{suspend,resume}(), especially
during the noirq phase of suspend. See also the guidance provided in
commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM
and system sleep (v2)").

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 4c4d17ddc96b..7c88611c732c 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1769,14 +1769,9 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 static int __maybe_unused tegra_i2c_suspend(struct device *dev)
 {
 	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-	int err;
 
 	i2c_mark_adapter_suspended(&i2c_dev->adapter);
 
-	err = pm_runtime_force_suspend(dev);
-	if (err < 0)
-		return err;
-
 	return 0;
 }
 
@@ -1797,10 +1792,6 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
 	if (err)
 		return err;
 
-	err = pm_runtime_force_resume(dev);
-	if (err < 0)
-		return err;
-
 	i2c_mark_adapter_resumed(&i2c_dev->adapter);
 
 	return 0;
-- 
2.24.1

[PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Depending on the board design, the I2C controllers found on Tegra SoCs
may require pinmuxing in order to function. This is done as part of the
driver's runtime suspend/resume operations. However, the PM core does
not allow devices to go into runtime suspend during system sleep to
avoid potential races with the suspend/resume of their parents.

As a result of this, when Tegra SoCs resume from system suspend, their
I2C controllers may have lost the pinmux state in hardware, whereas the
pinctrl subsystem is not aware of this. To fix this, make sure that if
the I2C controller is not runtime suspended, the runtime suspend code is
still executed in order to disable the module clock (which we don't need
to be enabled during sleep) and set the pinmux to the idle state.

Conversely, make sure that the I2C controller is properly resumed when
waking up from sleep so that pinmux settings are properly restored.

This fixes a bug seen with DDC transactions to an HDMI monitor timing
out when resuming from system suspend.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 7c88611c732c..db142d897604 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 static int __maybe_unused tegra_i2c_suspend(struct device *dev)
 {
 	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
+	int err = 0;
 
 	i2c_mark_adapter_suspended(&i2c_dev->adapter);
 
-	return 0;
+	if (!pm_runtime_status_suspended(dev))
+		err = tegra_i2c_runtime_suspend(dev);
+
+	return err;
 }
 
 static int __maybe_unused tegra_i2c_resume(struct device *dev)
@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
 	if (err)
 		return err;
 
-	err = tegra_i2c_runtime_suspend(dev);
-	if (err)
-		return err;
+	if (pm_runtime_status_suspended(dev)) {
+		err = tegra_i2c_runtime_suspend(dev);
+		if (err)
+			return err;
+	}
 
 	i2c_mark_adapter_resumed(&i2c_dev->adapter);
 
-- 
2.24.1

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[mirq-test]

On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
[...]
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>  {
>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> +	int err = 0;
>  
>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>  
> -	return 0;
> +	if (!pm_runtime_status_suspended(dev))
> +		err = tegra_i2c_runtime_suspend(dev);
> +
> +	return err;
>  }
>  
>  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> -	err = tegra_i2c_runtime_suspend(dev);
> -	if (err)
> -		return err;
> +	if (pm_runtime_status_suspended(dev)) {
[...]
Shouldn't this be negated as in suspend? I would assume that inbetween
suspend and resume nothing changes the stored state.

Best Regards,
Michał Mirosław

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Thierry Reding]

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

On Thu, May 07, 2020 at 12:43:36AM +0200, mirq-test@rere.qmqm.pl wrote:
> On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
> [...]
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
> >  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
> >  {
> >  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> > +	int err = 0;
> >  
> >  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
> >  
> > -	return 0;
> > +	if (!pm_runtime_status_suspended(dev))
> > +		err = tegra_i2c_runtime_suspend(dev);
> > +
> > +	return err;
> >  }
> >  
> >  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> > @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
> >  	if (err)
> >  		return err;
> >  
> > -	err = tegra_i2c_runtime_suspend(dev);
> > -	if (err)
> > -		return err;
> > +	if (pm_runtime_status_suspended(dev)) {
> [...]
> Shouldn't this be negated as in suspend? I would assume that inbetween
> suspend and resume nothing changes the stored state.

I know this is confusing because I have now twice had the same doubts
after looking at the patch after I sent it out and thought I had sent
out a wrong version.

However, I think it starts to make more sense when you look at the
resulting code, which I'll reproduce below:

	static int __maybe_unused tegra_i2c_resume(struct device *dev)
	{
		struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
		int err;

		err = tegra_i2c_runtime_resume(dev);
		if (err)
			return err;

		err = tegra_i2c_init(i2c_dev, false);
		if (err)
			return err;

		if (pm_runtime_status_suspended(dev)) {
			err = tegra_i2c_runtime_suspend(dev);
			if (err)
				return err;
		}

		i2c_mark_adapter_resumed(&i2c_dev->adapter);

		return 0;
	}

So the purpose here is to runtime resume the I2C controller temporarily
so that the register context can be reprogrammed because it was lost
during suspend. Now, if the controller was runtime suspended prior to
system suspend, we want to put it back into suspend after the context
was loaded again. Conversely, if it was not runtime suspended, then we
want to keep it on.

If it helps I can sprinkle some comments throughout this function to try
and explain why exactly this is being done.

Thierry

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

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Michał Mirosław]

On Thu, May 07, 2020 at 12:03:15PM +0200, Thierry Reding wrote:
> On Thu, May 07, 2020 at 12:43:36AM +0200, mirq-test@rere.qmqm.pl wrote:
> > On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
> > [...]
> > > --- a/drivers/i2c/busses/i2c-tegra.c
> > > +++ b/drivers/i2c/busses/i2c-tegra.c
> > > @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
> > >  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
> > >  {
> > >  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> > > +	int err = 0;
> > >  
> > >  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
> > >  
> > > -	return 0;
> > > +	if (!pm_runtime_status_suspended(dev))
> > > +		err = tegra_i2c_runtime_suspend(dev);
> > > +
> > > +	return err;
> > >  }
> > >  
> > >  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> > > @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
> > >  	if (err)
> > >  		return err;
> > >  
> > > -	err = tegra_i2c_runtime_suspend(dev);
> > > -	if (err)
> > > -		return err;
> > > +	if (pm_runtime_status_suspended(dev)) {
> > [...]
> > Shouldn't this be negated as in suspend? I would assume that inbetween
> > suspend and resume nothing changes the stored state.
> 
> I know this is confusing because I have now twice had the same doubts
> after looking at the patch after I sent it out and thought I had sent
> out a wrong version.
> 
> However, I think it starts to make more sense when you look at the
> resulting code, which I'll reproduce below:
> 
> 	static int __maybe_unused tegra_i2c_resume(struct device *dev)
> 	{
> 		struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> 		int err;
> 
> 		err = tegra_i2c_runtime_resume(dev);
> 		if (err)
> 			return err;
> 
> 		err = tegra_i2c_init(i2c_dev, false);
> 		if (err)
> 			return err;
> 
> 		if (pm_runtime_status_suspended(dev)) {
> 			err = tegra_i2c_runtime_suspend(dev);
> 			if (err)
> 				return err;
> 		}
> 
> 		i2c_mark_adapter_resumed(&i2c_dev->adapter);
> 
> 		return 0;
> 	}
> 
> So the purpose here is to runtime resume the I2C controller temporarily
> so that the register context can be reprogrammed because it was lost
> during suspend. Now, if the controller was runtime suspended prior to
> system suspend, we want to put it back into suspend after the context
> was loaded again. Conversely, if it was not runtime suspended, then we
> want to keep it on.
> 
> If it helps I can sprinkle some comments throughout this function to try
> and explain why exactly this is being done.

Now it makes sense. Thanks!

The full function is the missing context. What you wrote here 
put in commit message should also do the job.

Best Regards,
Michał Mirosław

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Dmitry Osipenko]

06.05.2020 22:33, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> Depending on the board design, the I2C controllers found on Tegra SoCs
> may require pinmuxing in order to function. This is done as part of the
> driver's runtime suspend/resume operations. However, the PM core does
> not allow devices to go into runtime suspend during system sleep to
> avoid potential races with the suspend/resume of their parents.
> 
> As a result of this, when Tegra SoCs resume from system suspend, their
> I2C controllers may have lost the pinmux state in hardware, whereas the
> pinctrl subsystem is not aware of this. To fix this, make sure that if
> the I2C controller is not runtime suspended, the runtime suspend code is
> still executed in order to disable the module clock (which we don't need
> to be enabled during sleep) and set the pinmux to the idle state.
> 
> Conversely, make sure that the I2C controller is properly resumed when
> waking up from sleep so that pinmux settings are properly restored.
> 
> This fixes a bug seen with DDC transactions to an HDMI monitor timing
> out when resuming from system suspend.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 7c88611c732c..db142d897604 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>  {
>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> +	int err = 0;
>  
>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>  
> -	return 0;
> +	if (!pm_runtime_status_suspended(dev))
> +		err = tegra_i2c_runtime_suspend(dev);
> +
> +	return err;
>  }
>  
>  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> -	err = tegra_i2c_runtime_suspend(dev);
> -	if (err)
> -		return err;
> +	if (pm_runtime_status_suspended(dev)) {
> +		err = tegra_i2c_runtime_suspend(dev);
> +		if (err)
> +			return err;
> +	}
>  
>  	i2c_mark_adapter_resumed(&i2c_dev->adapter);
>  
> 

Is it legal to touch DPAUX registers while DPAUX is in a suspended state?

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Thierry Reding]

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

On Fri, May 08, 2020 at 12:50:09AM +0300, Dmitry Osipenko wrote:
> 06.05.2020 22:33, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Depending on the board design, the I2C controllers found on Tegra SoCs
> > may require pinmuxing in order to function. This is done as part of the
> > driver's runtime suspend/resume operations. However, the PM core does
> > not allow devices to go into runtime suspend during system sleep to
> > avoid potential races with the suspend/resume of their parents.
> > 
> > As a result of this, when Tegra SoCs resume from system suspend, their
> > I2C controllers may have lost the pinmux state in hardware, whereas the
> > pinctrl subsystem is not aware of this. To fix this, make sure that if
> > the I2C controller is not runtime suspended, the runtime suspend code is
> > still executed in order to disable the module clock (which we don't need
> > to be enabled during sleep) and set the pinmux to the idle state.
> > 
> > Conversely, make sure that the I2C controller is properly resumed when
> > waking up from sleep so that pinmux settings are properly restored.
> > 
> > This fixes a bug seen with DDC transactions to an HDMI monitor timing
> > out when resuming from system suspend.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> > index 7c88611c732c..db142d897604 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
> >  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
> >  {
> >  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> > +	int err = 0;
> >  
> >  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
> >  
> > -	return 0;
> > +	if (!pm_runtime_status_suspended(dev))
> > +		err = tegra_i2c_runtime_suspend(dev);
> > +
> > +	return err;
> >  }
> >  
> >  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> > @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
> >  	if (err)
> >  		return err;
> >  
> > -	err = tegra_i2c_runtime_suspend(dev);
> > -	if (err)
> > -		return err;
> > +	if (pm_runtime_status_suspended(dev)) {
> > +		err = tegra_i2c_runtime_suspend(dev);
> > +		if (err)
> > +			return err;
> > +	}
> >  
> >  	i2c_mark_adapter_resumed(&i2c_dev->adapter);
> >  
> > 
> 
> Is it legal to touch DPAUX registers while DPAUX is in a suspended state?

DPAUX is never runtime suspended and the dependency from the I2C
controller on DPAUX should ensure that they are suspended and resumed in
the right order during system sleep.

Thierry

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

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Dmitry Osipenko]

08.05.2020 13:31, Thierry Reding пишет:
...
>> Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
> 
> DPAUX is never runtime suspended and the dependency from the I2C
> controller on DPAUX should ensure that they are suspended and resumed in
> the right order during system sleep.

1. Could you please explain why DPAUX is never suspended? Isn't it a
problem?

It looks a bit odd that driver is doing this [1][2]. RPM is supposed to
be used for the *dynamic* power management. Should we remove RPM usage
from the DPAUX driver?

[1]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L524
[2]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L591

2. Could you please explain why I2C driver has to care about restoring
the pinmux state? Why pinctrl driver isn't doing that for I2C and
everything else?

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Dmitry Osipenko]

> 2. Could you please explain why I2C driver has to care about restoring
> the pinmux state? Why pinctrl driver isn't doing that for I2C and
> everything else?

Although, now I see what you meant in the commit's message.

Perhaps the "their I2C controllers may have lost the pinmux state in
hardware" paragraph should be removed from the commit's message because
it's irrelevant to this patch. The pinctrl state is changed once
tegra_i2c_runtime_resume() is invoked and it is not about the change
made by this patch.

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Thierry Reding]

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

On Sat, May 09, 2020 at 06:35:41PM +0300, Dmitry Osipenko wrote:
> > 2. Could you please explain why I2C driver has to care about restoring
> > the pinmux state? Why pinctrl driver isn't doing that for I2C and
> > everything else?
> 
> Although, now I see what you meant in the commit's message.
> 
> Perhaps the "their I2C controllers may have lost the pinmux state in
> hardware" paragraph should be removed from the commit's message because
> it's irrelevant to this patch. The pinctrl state is changed once
> tegra_i2c_runtime_resume() is invoked and it is not about the change
> made by this patch.

The pinctrl state is changed in tegra_i2c_runtime_resume() *only if*
tegra_i2c_runtime_resume() has previously been called. So this patch
does indeed cause the pinmux to be restored, even though it does so
indirectly.

I think that paragraph is necessary to explain that. It's the pinctrl
that doesn't "notice" that the actual pinctrl state has changed from
"I2C" to "idle", so on resume it still thinks we're at "I2C" and won't
try to reapply the same state. Calling tegra_i2c_runtime_suspend() is
making sure that pinctrl explicitly switches to "idle", so that during
resume it will apply the "I2C" state since it is different from "idle".

Thierry

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

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Thierry Reding]

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

On Fri, May 08, 2020 at 06:00:57PM +0300, Dmitry Osipenko wrote:
> 08.05.2020 13:31, Thierry Reding пишет:
> ...
> >> Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
> > 
> > DPAUX is never runtime suspended and the dependency from the I2C
> > controller on DPAUX should ensure that they are suspended and resumed in
> > the right order during system sleep.
> 
> 1. Could you please explain why DPAUX is never suspended? Isn't it a
> problem?
> 
> It looks a bit odd that driver is doing this [1][2]. RPM is supposed to
> be used for the *dynamic* power management. Should we remove RPM usage
> from the DPAUX driver?
> 
> [1]
> https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L524
> [2]
> https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L591

Looks more like the intention had been to eventually enable dynamic
power management but I never got around to it. The runtime PM
implementations are what's necessary to runtime suspend the device,
so all that should be needed is hook up the pm_runtime_get() and
pm_runtime_put() calls correctly.

> 2. Could you please explain why I2C driver has to care about restoring
> the pinmux state? Why pinctrl driver isn't doing that for I2C and
> everything else?

We could probably do it either way. I did it this way because the
runtime suspend/resume will get called either way, so might as well
reuse the same code paths rather than add context save/restore.

Thierry

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

Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume

[Dmitry Osipenko]

06.05.2020 22:33, Thierry Reding пишет:
...
>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>  {
>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> +	int err = 0;
>  
>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>  
> -	return 0;
> +	if (!pm_runtime_status_suspended(dev))
> +		err = tegra_i2c_runtime_suspend(dev);

Could you please explain what determines whether I2C is RPM-suspended or
resumed during of the system's suspension?

[PATCH 3/5] i2c: tegra: Keep IRQs enabled during suspend/resume

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

One of the I2C controllers on Tegra SoCs is typically connected to a
system PMIC, which provides controls for critical power supplies for
most platforms.

Some drivers, such as PCI, need to disable these regulators during a
very late stage during suspend and resume them at a very early stage
during resume.

To support these use-cases, keep interrupts disabled during suspend/
resume.

Suggested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index db142d897604..8d2a212db733 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1694,8 +1694,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 
 	irq_set_status_flags(i2c_dev->irq, IRQ_NOAUTOEN);
 
-	ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
-			       tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
+	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr,
+			       IRQF_NO_SUSPEND, dev_name(&pdev->dev), i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
 		goto release_dma;
-- 
2.24.1

[PATCH 4/5] i2c: tegra: Better handle case where CPU0 is busy for a long time

[Thierry Reding]

From: Dmitry Osipenko <digetx@gmail.com>

Boot CPU0 always handle I2C interrupt and under some rare circumstances
(like running KASAN + NFS root) it may stuck in uninterruptible state for
a significant time. In this case we will get timeout if I2C transfer is
running on a sibling CPU, despite of IRQ being raised. In order to handle
this rare condition, the IRQ status needs to be checked after completion
timeout.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 8d2a212db733..d504e272d9fa 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -996,14 +996,13 @@ tegra_i2c_poll_completion_timeout(struct tegra_i2c_dev *i2c_dev,
 	do {
 		u32 status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
-		if (status) {
+		if (status)
 			tegra_i2c_isr(i2c_dev->irq, i2c_dev);
 
-			if (completion_done(complete)) {
-				s64 delta = ktime_ms_delta(ktimeout, ktime);
+		if (completion_done(complete)) {
+			s64 delta = ktime_ms_delta(ktimeout, ktime);
 
-				return msecs_to_jiffies(delta) ?: 1;
-			}
+			return msecs_to_jiffies(delta) ?: 1;
 		}
 
 		ktime = ktime_get();
@@ -1030,14 +1029,18 @@ tegra_i2c_wait_completion_timeout(struct tegra_i2c_dev *i2c_dev,
 		disable_irq(i2c_dev->irq);
 
 		/*
-		 * There is a chance that completion may happen after IRQ
-		 * synchronization, which is done by disable_irq().
+		 * Under some rare circumstances (like running KASAN +
+		 * NFS root) CPU, which handles interrupt, may stuck in
+		 * uninterruptible state for a significant time.  In this
+		 * case we will get timeout if I2C transfer is running on
+		 * a sibling CPU, despite of IRQ being raised.
+		 *
+		 * In order to handle this rare condition, the IRQ status
+		 * needs to be checked after timeout.
 		 */
-		if (ret == 0 && completion_done(complete)) {
-			dev_warn(i2c_dev->dev,
-				 "completion done after timeout\n");
-			ret = 1;
-		}
+		if (ret == 0)
+			ret = tegra_i2c_poll_completion_timeout(i2c_dev,
+								complete, 0);
 	}
 
 	return ret;
-- 
2.24.1

[PATCH 5/5] i2c: tegra: Synchronize DMA before termination

[Thierry Reding]

From: Dmitry Osipenko <digetx@gmail.com>

DMA transfer could be completed, but CPU (which handles DMA interrupt)
may get too busy and can't handle the interrupt in a timely manner,
despite of DMA IRQ being raised. In this case the DMA state needs to
synchronized before terminating DMA transfer in order not to miss the
DMA transfer completion.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d504e272d9fa..4cee596a604d 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1219,6 +1219,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		time_left = tegra_i2c_wait_completion_timeout(
 				i2c_dev, &i2c_dev->dma_complete, xfer_time);
 
+		/*
+		 * Synchronize DMA first, since dmaengine_terminate_sync()
+		 * performs synchronization after the transfer's termination
+		 * and we want to get a completion if transfer succeeded.
+		 */
+		dmaengine_synchronize(i2c_dev->msg_read ?
+				      i2c_dev->rx_dma_chan :
+				      i2c_dev->tx_dma_chan);
+
 		dmaengine_terminate_sync(i2c_dev->msg_read ?
 					 i2c_dev->rx_dma_chan :
 					 i2c_dev->tx_dma_chan);
-- 
2.24.1