| Message ID | 20200506193358.2807244-3-thierry.reding@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | i2c: tegra: Various fixes and improvements | expand |
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
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
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
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?
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
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?
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?
> 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.
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
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 --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);