diff mbox

[v3,02/14] mfd: max77686: Allow the max77686 rtc to wakeup the system

Message ID 1403202040-12641-3-git-send-email-javier.martinez@collabora.co.uk
State Superseded
Headers show

Commit Message

Javier Martinez Canillas June 19, 2014, 6:20 p.m. UTC
From: Doug Anderson <dianders@chromium.org>

The max77686 includes an RTC that keeps power during suspend.  It's
convenient to be able to use it as a wakeup source.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/rtc/rtc-max77686.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Krzysztof Kozlowski June 25, 2014, 10:05 a.m. UTC | #1
On czw, 2014-06-19 at 20:20 +0200, Javier Martinez Canillas wrote:
> From: Doug Anderson <dianders@chromium.org>
> 
> The max77686 includes an RTC that keeps power during suspend.  It's
> convenient to be able to use it as a wakeup source.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/rtc/rtc-max77686.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index d20a7f0..c1c6055 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -583,6 +583,33 @@ static void max77686_rtc_shutdown(struct platform_device *pdev)
>  #endif /* MAX77686_RTC_WTSR_SMPL */
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int max77686_rtc_suspend(struct device *dev)
> +{
> +	if (device_may_wakeup(dev)) {
> +		struct max77686_rtc_info *info = dev_get_drvdata(dev);
> +
> +		return enable_irq_wake(info->virq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int max77686_rtc_resume(struct device *dev)
> +{
> +	if (device_may_wakeup(dev)) {
> +		struct max77686_rtc_info *info = dev_get_drvdata(dev);
> +
> +		return disable_irq_wake(info->virq);
> +	}
> +
> +	return 0;
> +}
> +#endif

Haven't you noticed un-acked interrupts after first resume? Does the
alarm IRQ works after first suspend-resume?

This happens quite often (at least on boards with max14577, max77836 and
s2m/s5m). The drivers implementing own irq_chip often just call irq
worker thread (see max77693_irq_resume [1]). With regmap_irq_chip you
can disable/enable the IRQ [2][3].


P.S. Sorry for late reply. I was on holidays.


[1] http://lxr.free-electrons.com/source/drivers/mfd/max77693-irq.c#L233
[2] http://lxr.free-electrons.com/source/drivers/mfd/max14577.c#L181
[2] http://lxr.free-electrons.com/source/drivers/mfd/sec-core.c#L444

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas June 25, 2014, 10:29 a.m. UTC | #2
Hello Krzysztof,

On 06/25/2014 12:05 PM, Krzysztof Kozlowski wrote:
> On czw, 2014-06-19 at 20:20 +0200, Javier Martinez Canillas wrote:
>> From: Doug Anderson <dianders@chromium.org>
>> 
>> The max77686 includes an RTC that keeps power during suspend.  It's
>> convenient to be able to use it as a wakeup source.
>> 
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/rtc/rtc-max77686.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index d20a7f0..c1c6055 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -583,6 +583,33 @@ static void max77686_rtc_shutdown(struct platform_device *pdev)
>>  #endif /* MAX77686_RTC_WTSR_SMPL */
>>  }
>>  
>> +#ifdef CONFIG_PM_SLEEP
>> +static int max77686_rtc_suspend(struct device *dev)
>> +{
>> +	if (device_may_wakeup(dev)) {
>> +		struct max77686_rtc_info *info = dev_get_drvdata(dev);
>> +
>> +		return enable_irq_wake(info->virq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int max77686_rtc_resume(struct device *dev)
>> +{
>> +	if (device_may_wakeup(dev)) {
>> +		struct max77686_rtc_info *info = dev_get_drvdata(dev);
>> +
>> +		return disable_irq_wake(info->virq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
> 
> Haven't you noticed un-acked interrupts after first resume? Does the
> alarm IRQ works after first suspend-resume?
> 
> This happens quite often (at least on boards with max14577, max77836 and
> s2m/s5m). The drivers implementing own irq_chip often just call irq
> worker thread (see max77693_irq_resume [1]). With regmap_irq_chip you
> can disable/enable the IRQ [2][3].
> 

Yes, the original Chrome OS 3.8 max77xxx also called the irq worker thread to
ack the interrupt.

So the real problem is that an interrupt occurs before the I2C bus controller is
resumed and so the interrupt handler is not able to access the registers over I2C.

Doug posted the following patches [0,1] that AFAIU solves the issue by making
I2C controllers to be resumed in the noirq time to ensure that drivers will be
able to use the I2C bus to handler their wakeup.

In fact, Doug's original patch had this as a part of the commit message:

NOTE: due to wakeup ordering problems this patch alone doesn't work so
well on exynos5250-snow.  You also need something that brings the i2c
bus up before the max77686 wakeup runs.

I removed that note since I (probably wrong) thought that he didn't mean it to
be part of the commit message but just was side information.

> 
> P.S. Sorry for late reply. I was on holidays.
> 

No worries, thanks a lot for your feedback.

> 
> [1] http://lxr.free-electrons.com/source/drivers/mfd/max77693-irq.c#L233
> [2] http://lxr.free-electrons.com/source/drivers/mfd/max14577.c#L181
> [2] http://lxr.free-electrons.com/source/drivers/mfd/sec-core.c#L444
> 
> Best regards,
> Krzysztof
> 
> 
> 

Best regards,
Javier

[0]: https://patchwork.kernel.org/patch/4414851/
[1]: https://patchwork.kernel.org/patch/4414871/
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson June 25, 2014, 5:24 p.m. UTC | #3
Hi,

On Wed, Jun 25, 2014 at 3:29 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Yes, the original Chrome OS 3.8 max77xxx also called the irq worker thread to
> ack the interrupt.
>
> So the real problem is that an interrupt occurs before the I2C bus controller is
> resumed and so the interrupt handler is not able to access the registers over I2C.
>
> Doug posted the following patches [0,1] that AFAIU solves the issue by making
> I2C controllers to be resumed in the noirq time to ensure that drivers will be
> able to use the I2C bus to handler their wakeup.

Yup, that's the solution as far as I know.  I know that in ChromeOS we
still have the extra call to the worker thread (despite the controller
waking up early), but that might be related to some other problem?

I was able to successfully suspend/resume multiple times once I woke
the i2c controller up earlier.

If you feel like adding Reviewed-by / Tested-by to my i2c patch then
feel free!  ;)


> In fact, Doug's original patch had this as a part of the commit message:
>
> NOTE: due to wakeup ordering problems this patch alone doesn't work so
> well on exynos5250-snow.  You also need something that brings the i2c
> bus up before the max77686 wakeup runs.
>
> I removed that note since I (probably wrong) thought that he didn't mean it to
> be part of the commit message but just was side information.

I probably would have left that in the patch, but I'm not objecting to
you taking it out.  :)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas June 25, 2014, 6:11 p.m. UTC | #4
On 06/25/2014 07:24 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 25, 2014 at 3:29 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> Yes, the original Chrome OS 3.8 max77xxx also called the irq worker thread to
>> ack the interrupt.
>>
>> So the real problem is that an interrupt occurs before the I2C bus controller is
>> resumed and so the interrupt handler is not able to access the registers over I2C.
>>
>> Doug posted the following patches [0,1] that AFAIU solves the issue by making
>> I2C controllers to be resumed in the noirq time to ensure that drivers will be
>> able to use the I2C bus to handler their wakeup.
> 
> Yup, that's the solution as far as I know.  I know that in ChromeOS we
> still have the extra call to the worker thread (despite the controller
> waking up early), but that might be related to some other problem?
> 
> I was able to successfully suspend/resume multiple times once I woke
> the i2c controller up earlier.
> 
> If you feel like adding Reviewed-by / Tested-by to my i2c patch then
> feel free!  ;)
> 
> 

Done :)

>> In fact, Doug's original patch had this as a part of the commit message:
>>
>> NOTE: due to wakeup ordering problems this patch alone doesn't work so
>> well on exynos5250-snow.  You also need something that brings the i2c
>> bus up before the max77686 wakeup runs.
>>
>> I removed that note since I (probably wrong) thought that he didn't mean it to
>> be part of the commit message but just was side information.
> 
> I probably would have left that in the patch, but I'm not objecting to
> you taking it out.  :)
> 

Yes, I should had left that as well. I'll send a v4 shortly and will include the
note again.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index d20a7f0..c1c6055 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -583,6 +583,33 @@  static void max77686_rtc_shutdown(struct platform_device *pdev)
 #endif /* MAX77686_RTC_WTSR_SMPL */
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int max77686_rtc_suspend(struct device *dev)
+{
+	if (device_may_wakeup(dev)) {
+		struct max77686_rtc_info *info = dev_get_drvdata(dev);
+
+		return enable_irq_wake(info->virq);
+	}
+
+	return 0;
+}
+
+static int max77686_rtc_resume(struct device *dev)
+{
+	if (device_may_wakeup(dev)) {
+		struct max77686_rtc_info *info = dev_get_drvdata(dev);
+
+		return disable_irq_wake(info->virq);
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
+			 max77686_rtc_suspend, max77686_rtc_resume);
+
 static const struct platform_device_id rtc_id[] = {
 	{ "max77686-rtc", 0 },
 	{},
@@ -592,6 +619,7 @@  static struct platform_driver max77686_rtc_driver = {
 	.driver		= {
 		.name	= "max77686-rtc",
 		.owner	= THIS_MODULE,
+		.pm	= &max77686_rtc_pm_ops,
 	},
 	.probe		= max77686_rtc_probe,
 	.shutdown	= max77686_rtc_shutdown,