diff mbox series

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

Message ID 20200506193358.2807244-3-thierry.reding@gmail.com
State New
Headers show
Series i2c: tegra: Various fixes and improvements | expand

Commit Message

Thierry Reding May 6, 2020, 7:33 p.m. UTC
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(-)

Comments

mirq-test@rere.qmqm.pl May 6, 2020, 10:43 p.m. UTC | #1
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
Thierry Reding May 7, 2020, 10:03 a.m. UTC | #2
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
Michał Mirosław May 7, 2020, 12:20 p.m. UTC | #3
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
Dmitry Osipenko May 7, 2020, 9:50 p.m. UTC | #4
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?
Thierry Reding May 8, 2020, 10:31 a.m. UTC | #5
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
Dmitry Osipenko May 8, 2020, 3 p.m. UTC | #6
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?
Dmitry Osipenko May 8, 2020, 3:05 p.m. UTC | #7
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?
Dmitry Osipenko May 9, 2020, 3:35 p.m. UTC | #8
> 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.
Thierry Reding May 19, 2020, 3:57 p.m. UTC | #9
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
Thierry Reding May 19, 2020, 4:07 p.m. UTC | #10
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
diff mbox series

Patch

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);