i2c: tegra: Restore pinmux on system resume
diff mbox series

Message ID 20191213134417.222720-1-thierry.reding@gmail.com
State New
Headers show
Series
  • i2c: tegra: Restore pinmux on system resume
Related show

Commit Message

Thierry Reding Dec. 13, 2019, 1:44 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

Mikko Perttunen Dec. 13, 2019, 2:03 p.m. UTC | #1
Could this just be pm_runtime_force_suspend and pm_runtime_force_resume?

Mikko

On 13.12.2019 15.44, Thierry Reding wrote:
> 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 a98bf31d0e5c..dae2a3d7b512 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1710,10 +1710,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)
> @@ -1729,9 +1733,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);
>   
>
Mikko Perttunen Dec. 13, 2019, 2:04 p.m. UTC | #2
Oh, I guess that option was already discussed =)

Sorry,
Mikko

On 13.12.2019 16.03, Mikko Perttunen wrote:
> Could this just be pm_runtime_force_suspend and pm_runtime_force_resume?
> 
> Mikko
> 
> On 13.12.2019 15.44, Thierry Reding wrote:
>> 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 a98bf31d0e5c..dae2a3d7b512 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -1710,10 +1710,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)
>> @@ -1729,9 +1733,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);
>>
Wolfram Sang Jan. 23, 2020, 11:19 a.m. UTC | #3
On Fri, Dec 13, 2019 at 02:44:17PM +0100, Thierry Reding wrote:
> 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>

Do we still need this after I applied "[PATCH v5 0/8] NVIDIA Tegra I2C
driver fixes and improvements" ?
Dmitry Osipenko Jan. 23, 2020, 3:10 p.m. UTC | #4
23.01.2020 14:19, Wolfram Sang пишет:
> On Fri, Dec 13, 2019 at 02:44:17PM +0100, Thierry Reding wrote:
>> 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>
> 
> Do we still need this after I applied "[PATCH v5 0/8] NVIDIA Tegra I2C
> driver fixes and improvements" ?
> 

No, it shouldn't be needed.

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index a98bf31d0e5c..dae2a3d7b512 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1710,10 +1710,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)
@@ -1729,9 +1733,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);