linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mfd: tps6586x: Handle interrupts on suspend
@ 2018-10-19 13:22 Jon Hunter
  2018-10-20 21:18 ` Dmitry Osipenko
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Jon Hunter @ 2018-10-19 13:22 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-kernel, linux-tegra, Dmitry Osipenko, Jonathan Hunter, stable

From: Jonathan Hunter <jonathanh@nvidia.com>

The tps6586x driver creates an irqchip that is used by its various child
devices for managing interrupts. The tps6586x-rtc device is one of its
children that uses the tps6586x irqchip. When using the tps6586x-rtc as
a wake-up device from suspend, the following is seen:

 PM: Syncing filesystems ... done.
 Freezing user space processes ... (elapsed 0.001 seconds) done.
 OOM killer disabled.
 Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
 Disabling non-boot CPUs ...
 Entering suspend state LP1
 Enabling non-boot CPUs ...
 CPU1 is up
 tps6586x 3-0034: failed to read interrupt status
 tps6586x 3-0034: failed to read interrupt status

The reason why the tps6586x interrupt status cannot be read is because
the tps6586x interrupt is not masked during suspend and when the
tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
seen before the i2c controller has been resumed in order to read the
tps6586x interrupt status.

The tps6586x-rtc driver sets it's interrupt as a wake-up source during
suspend, which gets propagated to the parent tps6586x interrupt.
However, the tps6586x-rtc driver cannot disable it's interrupt during
suspend otherwise we would never be woken up and so the tps6586x must
disable it's interrupt instead.

Prevent the tps6586x interrupt handler from executing on exiting suspend
before the i2c controller has been resumed by disabling the tps6586x
interrupt on entering suspend and re-enabling it on resuming from
suspend.

Cc: stable@vger.kernel.org

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index b89379782741..9c7925ca13cf 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -592,6 +592,29 @@ static int tps6586x_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int __maybe_unused tps6586x_i2c_suspend(struct device *dev)
+{
+	struct tps6586x *tps6586x = dev_get_drvdata(dev);
+
+	if (tps6586x->client->irq)
+		disable_irq(tps6586x->client->irq);
+
+	return 0;
+}
+
+static int __maybe_unused tps6586x_i2c_resume(struct device *dev)
+{
+	struct tps6586x *tps6586x = dev_get_drvdata(dev);
+
+	if (tps6586x->client->irq)
+		enable_irq(tps6586x->client->irq);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(tps6586x_pm_ops, tps6586x_i2c_suspend,
+			 tps6586x_i2c_resume);
+
 static const struct i2c_device_id tps6586x_id_table[] = {
 	{ "tps6586x", 0 },
 	{ },
@@ -602,6 +625,7 @@ static struct i2c_driver tps6586x_driver = {
 	.driver	= {
 		.name	= "tps6586x",
 		.of_match_table = of_match_ptr(tps6586x_of_match),
+		.pm	= &tps6586x_pm_ops,
 	},
 	.probe		= tps6586x_i2c_probe,
 	.remove		= tps6586x_i2c_remove,
-- 
2.7.4


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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-19 13:22 [PATCH] mfd: tps6586x: Handle interrupts on suspend Jon Hunter
@ 2018-10-20 21:18 ` Dmitry Osipenko
  2018-10-22  9:52 ` Thierry Reding
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2018-10-20 21:18 UTC (permalink / raw)
  To: Jon Hunter, Lee Jones; +Cc: linux-kernel, linux-tegra, stable

On 10/19/18 4:22 PM, Jon Hunter wrote:
> From: Jonathan Hunter <jonathanh@nvidia.com>
> 
> The tps6586x driver creates an irqchip that is used by its various child
> devices for managing interrupts. The tps6586x-rtc device is one of its
> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
> a wake-up device from suspend, the following is seen:
> 
>  PM: Syncing filesystems ... done.
>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>  OOM killer disabled.
>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>  Disabling non-boot CPUs ...
>  Entering suspend state LP1
>  Enabling non-boot CPUs ...
>  CPU1 is up
>  tps6586x 3-0034: failed to read interrupt status
>  tps6586x 3-0034: failed to read interrupt status
> 
> The reason why the tps6586x interrupt status cannot be read is because
> the tps6586x interrupt is not masked during suspend and when the
> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
> seen before the i2c controller has been resumed in order to read the
> tps6586x interrupt status.
> 
> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
> suspend, which gets propagated to the parent tps6586x interrupt.
> However, the tps6586x-rtc driver cannot disable it's interrupt during
> suspend otherwise we would never be woken up and so the tps6586x must
> disable it's interrupt instead.
> 
> Prevent the tps6586x interrupt handler from executing on exiting suspend
> before the i2c controller has been resumed by disabling the tps6586x
> interrupt on entering suspend and re-enabling it on resuming from
> suspend.
> 
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---

Tested patch on linux-next and v4.14, works fine. Thank you!

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-19 13:22 [PATCH] mfd: tps6586x: Handle interrupts on suspend Jon Hunter
  2018-10-20 21:18 ` Dmitry Osipenko
@ 2018-10-22  9:52 ` Thierry Reding
  2018-10-22 11:19   ` Dmitry Osipenko
  2018-10-24 10:41   ` Jon Hunter
  2018-10-29  9:53 ` Thierry Reding
  2018-11-05 11:24 ` Jon Hunter
  3 siblings, 2 replies; 12+ messages in thread
From: Thierry Reding @ 2018-10-22  9:52 UTC (permalink / raw)
  To: Jon Hunter; +Cc: Lee Jones, linux-kernel, linux-tegra, Dmitry Osipenko, stable

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

On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
> From: Jonathan Hunter <jonathanh@nvidia.com>
> 
> The tps6586x driver creates an irqchip that is used by its various child
> devices for managing interrupts. The tps6586x-rtc device is one of its
> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
> a wake-up device from suspend, the following is seen:
> 
>  PM: Syncing filesystems ... done.
>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>  OOM killer disabled.
>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>  Disabling non-boot CPUs ...
>  Entering suspend state LP1
>  Enabling non-boot CPUs ...
>  CPU1 is up
>  tps6586x 3-0034: failed to read interrupt status
>  tps6586x 3-0034: failed to read interrupt status
> 
> The reason why the tps6586x interrupt status cannot be read is because
> the tps6586x interrupt is not masked during suspend and when the
> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
> seen before the i2c controller has been resumed in order to read the
> tps6586x interrupt status.
> 
> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
> suspend, which gets propagated to the parent tps6586x interrupt.
> However, the tps6586x-rtc driver cannot disable it's interrupt during
> suspend otherwise we would never be woken up and so the tps6586x must
> disable it's interrupt instead.
> 
> Prevent the tps6586x interrupt handler from executing on exiting suspend
> before the i2c controller has been resumed by disabling the tps6586x
> interrupt on entering suspend and re-enabling it on resuming from
> suspend.
> 
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

So does this mean that the SPI interrupt for the PMIC can still be a
wakeup source even if it is masked? This is slightly odd because now
you're saying that this does work while it doesn't work for the RTC
interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
which doesn't extend to the TPS6586x? Or am I missing something?

Thierry

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

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-22  9:52 ` Thierry Reding
@ 2018-10-22 11:19   ` Dmitry Osipenko
  2018-10-24 10:49     ` Jon Hunter
  2018-10-24 10:41   ` Jon Hunter
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Osipenko @ 2018-10-22 11:19 UTC (permalink / raw)
  To: Thierry Reding, Jon Hunter; +Cc: Lee Jones, linux-kernel, linux-tegra, stable

On 10/22/18 12:52 PM, Thierry Reding wrote:
> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>
>> The tps6586x driver creates an irqchip that is used by its various child
>> devices for managing interrupts. The tps6586x-rtc device is one of its
>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>> a wake-up device from suspend, the following is seen:
>>
>>  PM: Syncing filesystems ... done.
>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>  OOM killer disabled.
>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>  Disabling non-boot CPUs ...
>>  Entering suspend state LP1
>>  Enabling non-boot CPUs ...
>>  CPU1 is up
>>  tps6586x 3-0034: failed to read interrupt status
>>  tps6586x 3-0034: failed to read interrupt status
>>
>> The reason why the tps6586x interrupt status cannot be read is because
>> the tps6586x interrupt is not masked during suspend and when the
>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>> seen before the i2c controller has been resumed in order to read the
>> tps6586x interrupt status.
>>
>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>> suspend, which gets propagated to the parent tps6586x interrupt.
>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>> suspend otherwise we would never be woken up and so the tps6586x must
>> disable it's interrupt instead.
>>
>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>> before the i2c controller has been resumed by disabling the tps6586x
>> interrupt on entering suspend and re-enabling it on resuming from
>> suspend.
>>
>> Cc: stable@vger.kernel.org
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
> 
> So does this mean that the SPI interrupt for the PMIC can still be a
> wakeup source even if it is masked? This is slightly odd because now
> you're saying that this does work while it doesn't work for the RTC
> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
> which doesn't extend to the TPS6586x? Or am I missing something?

What is the expected behaviour of IRQ disabling? Should it disable wakeup ability or only mask IRQ handling?

Couple months ago disabling of IRQ was disabling the wakeup, now something has been changed in kernel and wakeup isn't getting disabled. So either there was a bug before that was fixed or there is a bug now.

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-22  9:52 ` Thierry Reding
  2018-10-22 11:19   ` Dmitry Osipenko
@ 2018-10-24 10:41   ` Jon Hunter
  1 sibling, 0 replies; 12+ messages in thread
From: Jon Hunter @ 2018-10-24 10:41 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Lee Jones, linux-kernel, linux-tegra, Dmitry Osipenko, stable


On 22/10/2018 10:52, Thierry Reding wrote:
> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>
>> The tps6586x driver creates an irqchip that is used by its various child
>> devices for managing interrupts. The tps6586x-rtc device is one of its
>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>> a wake-up device from suspend, the following is seen:
>>
>>  PM: Syncing filesystems ... done.
>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>  OOM killer disabled.
>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>  Disabling non-boot CPUs ...
>>  Entering suspend state LP1
>>  Enabling non-boot CPUs ...
>>  CPU1 is up
>>  tps6586x 3-0034: failed to read interrupt status
>>  tps6586x 3-0034: failed to read interrupt status
>>
>> The reason why the tps6586x interrupt status cannot be read is because
>> the tps6586x interrupt is not masked during suspend and when the
>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>> seen before the i2c controller has been resumed in order to read the
>> tps6586x interrupt status.
>>
>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>> suspend, which gets propagated to the parent tps6586x interrupt.
>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>> suspend otherwise we would never be woken up and so the tps6586x must
>> disable it's interrupt instead.
>>
>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>> before the i2c controller has been resumed by disabling the tps6586x
>> interrupt on entering suspend and re-enabling it on resuming from
>> suspend.
>>
>> Cc: stable@vger.kernel.org
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
> 
> So does this mean that the SPI interrupt for the PMIC can still be a
> wakeup source even if it is masked? This is slightly odd because now
> you're saying that this does work while it doesn't work for the RTC
> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
> which doesn't extend to the TPS6586x? Or am I missing something?

No it is not a quirk for the Tegra LIC/GIC, it is purely an issue with
the PMIC driver and this issue could occur on other platforms not just
Tegra.

During suspend, we want the PMIC's host interrupt to be masked in the
LIC/GIC level, but the wake-up for this interrupt to be enabled in the
LIC. Currently, the wake-up is enabled in the LIC, but the interrupt is
not being masked in the LIC/GIC which is the problem.

If the RTC interrupt within the PMIC is disabled, then the PMIC
generates no interrupt to the host (ie. Tegra) on an alarm. The PMIC
interrupt controller only has an interrupt mask for the various
interrupt sources within the PMIC. Therefore, if the RTC is
enabled/in-use we should never mask its interrupt within the PMIC.

Once the PMIC has been suspended, then it makes sense to disable its
interrupt, because it cannot be service until it has been resumed,
implying that I2C controller (ie. its parent) has also been resumed.

Cheers
Jon

-- 
nvpublic

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-22 11:19   ` Dmitry Osipenko
@ 2018-10-24 10:49     ` Jon Hunter
  2018-10-24 12:44       ` Dmitry Osipenko
  0 siblings, 1 reply; 12+ messages in thread
From: Jon Hunter @ 2018-10-24 10:49 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: Lee Jones, linux-kernel, linux-tegra, stable


On 22/10/2018 12:19, Dmitry Osipenko wrote:
> On 10/22/18 12:52 PM, Thierry Reding wrote:
>> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>>
>>> The tps6586x driver creates an irqchip that is used by its various child
>>> devices for managing interrupts. The tps6586x-rtc device is one of its
>>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>>> a wake-up device from suspend, the following is seen:
>>>
>>>  PM: Syncing filesystems ... done.
>>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>>  OOM killer disabled.
>>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>>  Disabling non-boot CPUs ...
>>>  Entering suspend state LP1
>>>  Enabling non-boot CPUs ...
>>>  CPU1 is up
>>>  tps6586x 3-0034: failed to read interrupt status
>>>  tps6586x 3-0034: failed to read interrupt status
>>>
>>> The reason why the tps6586x interrupt status cannot be read is because
>>> the tps6586x interrupt is not masked during suspend and when the
>>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>>> seen before the i2c controller has been resumed in order to read the
>>> tps6586x interrupt status.
>>>
>>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>>> suspend, which gets propagated to the parent tps6586x interrupt.
>>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>>> suspend otherwise we would never be woken up and so the tps6586x must
>>> disable it's interrupt instead.
>>>
>>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>>> before the i2c controller has been resumed by disabling the tps6586x
>>> interrupt on entering suspend and re-enabling it on resuming from
>>> suspend.
>>>
>>> Cc: stable@vger.kernel.org
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>
>> So does this mean that the SPI interrupt for the PMIC can still be a
>> wakeup source even if it is masked? This is slightly odd because now
>> you're saying that this does work while it doesn't work for the RTC
>> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
>> which doesn't extend to the TPS6586x? Or am I missing something?
> 
> What is the expected behaviour of IRQ disabling? Should it disable wakeup ability or only mask IRQ handling?

I believe only mask the interrupt. However, the caveat here could be if
the parent interrupt controller actually supports wake-up. For Tegra it
is the LIC that handles the wake-up.

> Couple months ago disabling of IRQ was disabling the wakeup, now something has been changed in kernel and wakeup isn't getting disabled. So either there was a bug before that was fixed or there is a bug now.

Are you sure you were disabling the PMIC host interrupt? If you disable
the RTC interrupt in the PMIC's RTC driver, then this will prevent the
wake-up from occurring because you are masking the interrupt within the
PMIC and so it will never generate an interrupt to the host.

Cheers
Jon

-- 
nvpublic

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-24 10:49     ` Jon Hunter
@ 2018-10-24 12:44       ` Dmitry Osipenko
  2018-10-24 14:19         ` Jon Hunter
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Osipenko @ 2018-10-24 12:44 UTC (permalink / raw)
  To: Jon Hunter, Thierry Reding; +Cc: Lee Jones, linux-kernel, linux-tegra, stable

On 10/24/18 1:49 PM, Jon Hunter wrote:
> 
> On 22/10/2018 12:19, Dmitry Osipenko wrote:
>> On 10/22/18 12:52 PM, Thierry Reding wrote:
>>> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>>>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>>>
>>>> The tps6586x driver creates an irqchip that is used by its various child
>>>> devices for managing interrupts. The tps6586x-rtc device is one of its
>>>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>>>> a wake-up device from suspend, the following is seen:
>>>>
>>>>  PM: Syncing filesystems ... done.
>>>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>>>  OOM killer disabled.
>>>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>>>  Disabling non-boot CPUs ...
>>>>  Entering suspend state LP1
>>>>  Enabling non-boot CPUs ...
>>>>  CPU1 is up
>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>
>>>> The reason why the tps6586x interrupt status cannot be read is because
>>>> the tps6586x interrupt is not masked during suspend and when the
>>>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>>>> seen before the i2c controller has been resumed in order to read the
>>>> tps6586x interrupt status.
>>>>
>>>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>>>> suspend, which gets propagated to the parent tps6586x interrupt.
>>>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>>>> suspend otherwise we would never be woken up and so the tps6586x must
>>>> disable it's interrupt instead.
>>>>
>>>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>>>> before the i2c controller has been resumed by disabling the tps6586x
>>>> interrupt on entering suspend and re-enabling it on resuming from
>>>> suspend.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>
>>> So does this mean that the SPI interrupt for the PMIC can still be a
>>> wakeup source even if it is masked? This is slightly odd because now
>>> you're saying that this does work while it doesn't work for the RTC
>>> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
>>> which doesn't extend to the TPS6586x? Or am I missing something?
>>
>> What is the expected behaviour of IRQ disabling? Should it disable wakeup ability or only mask IRQ handling?
> 
> I believe only mask the interrupt. However, the caveat here could be if
> the parent interrupt controller actually supports wake-up. For Tegra it
> is the LIC that handles the wake-up.
> 
>> Couple months ago disabling of IRQ was disabling the wakeup, now something has been changed in kernel and wakeup isn't getting disabled. So either there was a bug before that was fixed or there is a bug now.
> 
> Are you sure you were disabling the PMIC host interrupt? If you disable
> the RTC interrupt in the PMIC's RTC driver, then this will prevent the
> wake-up from occurring because you are masking the interrupt within the
> PMIC and so it will never generate an interrupt to the host.

I'm pretty sure (but not 100%) that was trying the same change as in your patch and it didn't work sometime before. If disable_irq() shouldn't disable wakeup, then everything is perfect now.

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-24 12:44       ` Dmitry Osipenko
@ 2018-10-24 14:19         ` Jon Hunter
  2018-10-25 13:01           ` Dmitry Osipenko
  0 siblings, 1 reply; 12+ messages in thread
From: Jon Hunter @ 2018-10-24 14:19 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: Lee Jones, linux-kernel, linux-tegra, stable


On 24/10/2018 13:44, Dmitry Osipenko wrote:
> On 10/24/18 1:49 PM, Jon Hunter wrote:
>>
>> On 22/10/2018 12:19, Dmitry Osipenko wrote:
>>> On 10/22/18 12:52 PM, Thierry Reding wrote:
>>>> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>>>>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>>>>
>>>>> The tps6586x driver creates an irqchip that is used by its various child
>>>>> devices for managing interrupts. The tps6586x-rtc device is one of its
>>>>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>>>>> a wake-up device from suspend, the following is seen:
>>>>>
>>>>>  PM: Syncing filesystems ... done.
>>>>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>>>>  OOM killer disabled.
>>>>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>>>>  Disabling non-boot CPUs ...
>>>>>  Entering suspend state LP1
>>>>>  Enabling non-boot CPUs ...
>>>>>  CPU1 is up
>>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>>
>>>>> The reason why the tps6586x interrupt status cannot be read is because
>>>>> the tps6586x interrupt is not masked during suspend and when the
>>>>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>>>>> seen before the i2c controller has been resumed in order to read the
>>>>> tps6586x interrupt status.
>>>>>
>>>>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>>>>> suspend, which gets propagated to the parent tps6586x interrupt.
>>>>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>>>>> suspend otherwise we would never be woken up and so the tps6586x must
>>>>> disable it's interrupt instead.
>>>>>
>>>>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>>>>> before the i2c controller has been resumed by disabling the tps6586x
>>>>> interrupt on entering suspend and re-enabling it on resuming from
>>>>> suspend.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>> ---
>>>>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> So does this mean that the SPI interrupt for the PMIC can still be a
>>>> wakeup source even if it is masked? This is slightly odd because now
>>>> you're saying that this does work while it doesn't work for the RTC
>>>> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
>>>> which doesn't extend to the TPS6586x? Or am I missing something?
>>>
>>> What is the expected behaviour of IRQ disabling? Should it disable wakeup ability or only mask IRQ handling?
>>
>> I believe only mask the interrupt. However, the caveat here could be if
>> the parent interrupt controller actually supports wake-up. For Tegra it
>> is the LIC that handles the wake-up.
>>
>>> Couple months ago disabling of IRQ was disabling the wakeup, now something has been changed in kernel and wakeup isn't getting disabled. So either there was a bug before that was fixed or there is a bug now.
>>
>> Are you sure you were disabling the PMIC host interrupt? If you disable
>> the RTC interrupt in the PMIC's RTC driver, then this will prevent the
>> wake-up from occurring because you are masking the interrupt within the
>> PMIC and so it will never generate an interrupt to the host.
> 
> I'm pretty sure (but not 100%) that was trying the same change as in your patch and it didn't work sometime before. If disable_irq() shouldn't disable wakeup, then everything is perfect now.

Please note that this is very similar to the following fix where I
experienced the same problem with another PMIC a couple years back ...

35deff7eb212 ("mfd: as3722: Handle interrupts on suspend")

I did not bother setting the enable/disable_irq_wake() for the tps6586x
host irq during resume/suspend because the irqchip for the tps6586x has
an irq_set_wake function that propagates the wake enable/disable.

Cheers
Jon

-- 
nvpublic

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-24 14:19         ` Jon Hunter
@ 2018-10-25 13:01           ` Dmitry Osipenko
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2018-10-25 13:01 UTC (permalink / raw)
  To: Jon Hunter, Thierry Reding; +Cc: Lee Jones, linux-kernel, linux-tegra, stable

On 10/24/18 5:19 PM, Jon Hunter wrote:
> 
> On 24/10/2018 13:44, Dmitry Osipenko wrote:
>> On 10/24/18 1:49 PM, Jon Hunter wrote:
>>>
>>> On 22/10/2018 12:19, Dmitry Osipenko wrote:
>>>> On 10/22/18 12:52 PM, Thierry Reding wrote:
>>>>> On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
>>>>>> From: Jonathan Hunter <jonathanh@nvidia.com>
>>>>>>
>>>>>> The tps6586x driver creates an irqchip that is used by its various child
>>>>>> devices for managing interrupts. The tps6586x-rtc device is one of its
>>>>>> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
>>>>>> a wake-up device from suspend, the following is seen:
>>>>>>
>>>>>>  PM: Syncing filesystems ... done.
>>>>>>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>>>>>>  OOM killer disabled.
>>>>>>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>>>>>>  Disabling non-boot CPUs ...
>>>>>>  Entering suspend state LP1
>>>>>>  Enabling non-boot CPUs ...
>>>>>>  CPU1 is up
>>>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>>>  tps6586x 3-0034: failed to read interrupt status
>>>>>>
>>>>>> The reason why the tps6586x interrupt status cannot be read is because
>>>>>> the tps6586x interrupt is not masked during suspend and when the
>>>>>> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
>>>>>> seen before the i2c controller has been resumed in order to read the
>>>>>> tps6586x interrupt status.
>>>>>>
>>>>>> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
>>>>>> suspend, which gets propagated to the parent tps6586x interrupt.
>>>>>> However, the tps6586x-rtc driver cannot disable it's interrupt during
>>>>>> suspend otherwise we would never be woken up and so the tps6586x must
>>>>>> disable it's interrupt instead.
>>>>>>
>>>>>> Prevent the tps6586x interrupt handler from executing on exiting suspend
>>>>>> before the i2c controller has been resumed by disabling the tps6586x
>>>>>> interrupt on entering suspend and re-enabling it on resuming from
>>>>>> suspend.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org
>>>>>>
>>>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>>>> ---
>>>>>>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>>>>>>  1 file changed, 24 insertions(+)
>>>>>
>>>>> So does this mean that the SPI interrupt for the PMIC can still be a
>>>>> wakeup source even if it is masked? This is slightly odd because now
>>>>> you're saying that this does work while it doesn't work for the RTC
>>>>> interrupt. So is this an implementation quirk of the LIC/GIC on Tegra
>>>>> which doesn't extend to the TPS6586x? Or am I missing something?
>>>>
>>>> What is the expected behaviour of IRQ disabling? Should it disable wakeup ability or only mask IRQ handling?
>>>
>>> I believe only mask the interrupt. However, the caveat here could be if
>>> the parent interrupt controller actually supports wake-up. For Tegra it
>>> is the LIC that handles the wake-up.
>>>
>>>> Couple months ago disabling of IRQ was disabling the wakeup, now something has been changed in kernel and wakeup isn't getting disabled. So either there was a bug before that was fixed or there is a bug now.
>>>
>>> Are you sure you were disabling the PMIC host interrupt? If you disable
>>> the RTC interrupt in the PMIC's RTC driver, then this will prevent the
>>> wake-up from occurring because you are masking the interrupt within the
>>> PMIC and so it will never generate an interrupt to the host.
>>
>> I'm pretty sure (but not 100%) that was trying the same change as in your patch and it didn't work sometime before. If disable_irq() shouldn't disable wakeup, then everything is perfect now.
> 
> Please note that this is very similar to the following fix where I
> experienced the same problem with another PMIC a couple years back ...
> 
> 35deff7eb212 ("mfd: as3722: Handle interrupts on suspend")
> 
> I did not bother setting the enable/disable_irq_wake() for the tps6586x
> host irq during resume/suspend because the irqchip for the tps6586x has
> an irq_set_wake function that propagates the wake enable/disable.

Okay, thanks.

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-19 13:22 [PATCH] mfd: tps6586x: Handle interrupts on suspend Jon Hunter
  2018-10-20 21:18 ` Dmitry Osipenko
  2018-10-22  9:52 ` Thierry Reding
@ 2018-10-29  9:53 ` Thierry Reding
  2018-11-05 11:24 ` Jon Hunter
  3 siblings, 0 replies; 12+ messages in thread
From: Thierry Reding @ 2018-10-29  9:53 UTC (permalink / raw)
  To: Jon Hunter; +Cc: Lee Jones, linux-kernel, linux-tegra, Dmitry Osipenko, stable

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

On Fri, Oct 19, 2018 at 02:22:53PM +0100, Jon Hunter wrote:
> From: Jonathan Hunter <jonathanh@nvidia.com>
> 
> The tps6586x driver creates an irqchip that is used by its various child
> devices for managing interrupts. The tps6586x-rtc device is one of its
> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
> a wake-up device from suspend, the following is seen:
> 
>  PM: Syncing filesystems ... done.
>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>  OOM killer disabled.
>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>  Disabling non-boot CPUs ...
>  Entering suspend state LP1
>  Enabling non-boot CPUs ...
>  CPU1 is up
>  tps6586x 3-0034: failed to read interrupt status
>  tps6586x 3-0034: failed to read interrupt status
> 
> The reason why the tps6586x interrupt status cannot be read is because
> the tps6586x interrupt is not masked during suspend and when the
> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
> seen before the i2c controller has been resumed in order to read the
> tps6586x interrupt status.
> 
> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
> suspend, which gets propagated to the parent tps6586x interrupt.
> However, the tps6586x-rtc driver cannot disable it's interrupt during
> suspend otherwise we would never be woken up and so the tps6586x must
> disable it's interrupt instead.
> 
> Prevent the tps6586x interrupt handler from executing on exiting suspend
> before the i2c controller has been resumed by disabling the tps6586x
> interrupt on entering suspend and re-enabling it on resuming from
> suspend.
> 
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/mfd/tps6586x.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

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

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-10-19 13:22 [PATCH] mfd: tps6586x: Handle interrupts on suspend Jon Hunter
                   ` (2 preceding siblings ...)
  2018-10-29  9:53 ` Thierry Reding
@ 2018-11-05 11:24 ` Jon Hunter
  2018-11-13  7:52   ` Lee Jones
  3 siblings, 1 reply; 12+ messages in thread
From: Jon Hunter @ 2018-11-05 11:24 UTC (permalink / raw)
  To: Lee Jones; +Cc: linux-kernel, linux-tegra, Dmitry Osipenko, stable

Hi Lee,

On 19/10/2018 14:22, Jon Hunter wrote:
> From: Jonathan Hunter <jonathanh@nvidia.com>
> 
> The tps6586x driver creates an irqchip that is used by its various child
> devices for managing interrupts. The tps6586x-rtc device is one of its
> children that uses the tps6586x irqchip. When using the tps6586x-rtc as
> a wake-up device from suspend, the following is seen:
> 
>  PM: Syncing filesystems ... done.
>  Freezing user space processes ... (elapsed 0.001 seconds) done.
>  OOM killer disabled.
>  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
>  Disabling non-boot CPUs ...Hi 
>  Entering suspend state LP1
>  Enabling non-boot CPUs ...
>  CPU1 is up
>  tps6586x 3-0034: failed to read interrupt status
>  tps6586x 3-0034: failed to read interrupt status
> 
> The reason why the tps6586x interrupt status cannot be read is because
> the tps6586x interrupt is not masked during suspend and when the
> tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
> seen before the i2c controller has been resumed in order to read the
> tps6586x interrupt status.
> 
> The tps6586x-rtc driver sets it's interrupt as a wake-up source during
> suspend, which gets propagated to the parent tps6586x interrupt.
> However, the tps6586x-rtc driver cannot disable it's interrupt during
> suspend otherwise we would never be woken up and so the tps6586x must
> disable it's interrupt instead.
> 
> Prevent the tps6586x interrupt handler from executing on exiting suspend
> before the i2c controller has been resumed by disabling the tps6586x
> interrupt on entering suspend and re-enabling it on resuming from
> suspend.
> 
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---

Thierry and Dmitry have acked/reviewed this, let me know if you have any
comments.

Cheers
Jon

-- 
nvpublic

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

* Re: [PATCH] mfd: tps6586x: Handle interrupts on suspend
  2018-11-05 11:24 ` Jon Hunter
@ 2018-11-13  7:52   ` Lee Jones
  0 siblings, 0 replies; 12+ messages in thread
From: Lee Jones @ 2018-11-13  7:52 UTC (permalink / raw)
  To: Jon Hunter; +Cc: linux-kernel, linux-tegra, Dmitry Osipenko, stable

On Mon, 05 Nov 2018, Jon Hunter wrote:

> Hi Lee,
> 
> On 19/10/2018 14:22, Jon Hunter wrote:
> > From: Jonathan Hunter <jonathanh@nvidia.com>
> > 
> > The tps6586x driver creates an irqchip that is used by its various child
> > devices for managing interrupts. The tps6586x-rtc device is one of its
> > children that uses the tps6586x irqchip. When using the tps6586x-rtc as
> > a wake-up device from suspend, the following is seen:
> > 
> >  PM: Syncing filesystems ... done.
> >  Freezing user space processes ... (elapsed 0.001 seconds) done.
> >  OOM killer disabled.
> >  Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> >  Disabling non-boot CPUs ...Hi 
> >  Entering suspend state LP1
> >  Enabling non-boot CPUs ...
> >  CPU1 is up
> >  tps6586x 3-0034: failed to read interrupt status
> >  tps6586x 3-0034: failed to read interrupt status
> > 
> > The reason why the tps6586x interrupt status cannot be read is because
> > the tps6586x interrupt is not masked during suspend and when the
> > tps6586x-rtc interrupt occurs, to wake-up the device, the interrupt is
> > seen before the i2c controller has been resumed in order to read the
> > tps6586x interrupt status.
> > 
> > The tps6586x-rtc driver sets it's interrupt as a wake-up source during
> > suspend, which gets propagated to the parent tps6586x interrupt.
> > However, the tps6586x-rtc driver cannot disable it's interrupt during
> > suspend otherwise we would never be woken up and so the tps6586x must
> > disable it's interrupt instead.
> > 
> > Prevent the tps6586x interrupt handler from executing on exiting suspend
> > before the i2c controller has been resumed by disabling the tps6586x
> > interrupt on entering suspend and re-enabling it on resuming from
> > suspend.
> > 
> > Cc: stable@vger.kernel.org
> > 
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> > ---
> 
> Thierry and Dmitry have acked/reviewed this, let me know if you have any
> comments.

Best thing you can do to regain attention on a potentially forgotten
submission is to collect any Acks you have accrued (if any) and submit
a [RESEND].

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

end of thread, other threads:[~2018-11-13  7:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-19 13:22 [PATCH] mfd: tps6586x: Handle interrupts on suspend Jon Hunter
2018-10-20 21:18 ` Dmitry Osipenko
2018-10-22  9:52 ` Thierry Reding
2018-10-22 11:19   ` Dmitry Osipenko
2018-10-24 10:49     ` Jon Hunter
2018-10-24 12:44       ` Dmitry Osipenko
2018-10-24 14:19         ` Jon Hunter
2018-10-25 13:01           ` Dmitry Osipenko
2018-10-24 10:41   ` Jon Hunter
2018-10-29  9:53 ` Thierry Reding
2018-11-05 11:24 ` Jon Hunter
2018-11-13  7:52   ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).