[PATCH] rtc: imxdi: add wakeup support

[PATCH] rtc: imxdi: add wakeup support

[Martin Kaiser]

The DryIce-based RTC supports alarms that trigger an interrupt.

Add an option to configure this interrupt as a wakeup source that wakes
the system up from standby mode.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---

simple test

   [root@board ]# echo enabled > /sys/class/rtc/rtc0/device/power/wakeup
   [root@board ]# rtcwake -s 3 -m mem
   wakeup from "mem" at Fri Apr 30 09:23:52 2021
   ...
   [root@board ]#

 drivers/rtc/rtc-imxdi.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index c1806f4d68e7..63957be25759 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -98,6 +98,7 @@
  * @pdev: pointer to platform dev
  * @rtc: pointer to rtc struct
  * @ioaddr: IO registers pointer
+ * @norm_irq: irq number of the "normal" irq
  * @clk: input reference clock
  * @dsr: copy of the DSR register
  * @irq_lock: interrupt enable register (DIER) lock
@@ -109,6 +110,7 @@ struct imxdi_dev {
 	struct platform_device *pdev;
 	struct rtc_device *rtc;
 	void __iomem *ioaddr;
+	int norm_irq;
 	struct clk *clk;
 	u32 dsr;
 	spinlock_t irq_lock;
@@ -741,7 +743,7 @@ static void dryice_work(struct work_struct *work)
 static int __init dryice_rtc_probe(struct platform_device *pdev)
 {
 	struct imxdi_dev *imxdi;
-	int norm_irq, sec_irq;
+	int sec_irq;
 	int rc;
 
 	imxdi = devm_kzalloc(&pdev->dev, sizeof(*imxdi), GFP_KERNEL);
@@ -756,9 +758,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&imxdi->irq_lock);
 
-	norm_irq = platform_get_irq(pdev, 0);
-	if (norm_irq < 0)
-		return norm_irq;
+	imxdi->norm_irq = platform_get_irq(pdev, 0);
+	if (imxdi->norm_irq < 0)
+		return imxdi->norm_irq;
 
 	/* the 2nd irq is the security violation irq
 	 * make this optional, don't break the device tree ABI
@@ -795,7 +797,7 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 	if (rc != 0)
 		goto err;
 
-	rc = devm_request_irq(&pdev->dev, norm_irq, dryice_irq,
+	rc = devm_request_irq(&pdev->dev, imxdi->norm_irq, dryice_irq,
 			      IRQF_SHARED, pdev->name, imxdi);
 	if (rc) {
 		dev_warn(&pdev->dev, "interrupt not available.\n");
@@ -811,6 +813,8 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, imxdi);
 
+	device_set_wakeup_capable(&pdev->dev, true);
+
 	imxdi->rtc->ops = &dryice_rtc_ops;
 	imxdi->rtc->range_max = U32_MAX;
 
@@ -830,6 +834,8 @@ static int __exit dryice_rtc_remove(struct platform_device *pdev)
 {
 	struct imxdi_dev *imxdi = platform_get_drvdata(pdev);
 
+	device_set_wakeup_capable(&pdev->dev, false);
+
 	flush_work(&imxdi->work);
 
 	/* mask all interrupts */
@@ -847,10 +853,33 @@ static const struct of_device_id dryice_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, dryice_dt_ids);
 
+#ifdef CONFIG_PM_SLEEP
+static int dryice_suspend(struct device *dev)
+{
+	struct imxdi_dev *imxdi = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(imxdi->norm_irq);
+	return 0;
+}
+
+static int dryice_resume(struct device *dev)
+{
+	struct imxdi_dev *imxdi = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(imxdi->norm_irq);
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(dryice_pm, dryice_suspend, dryice_resume);
+
 static struct platform_driver dryice_rtc_driver = {
 	.driver = {
 		   .name = "imxdi_rtc",
 		   .of_match_table = dryice_dt_ids,
+		   .pm = &dryice_pm,
 		   },
 	.remove = __exit_p(dryice_rtc_remove),
 };
-- 
2.20.1

Re: [PATCH] rtc: imxdi: add wakeup support

[Alexandre Belloni]

Hello,

On 30/04/2021 11:32:10+0200, Martin Kaiser wrote:
> The DryIce-based RTC supports alarms that trigger an interrupt.
> 
> Add an option to configure this interrupt as a wakeup source that wakes
> the system up from standby mode.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> 
> simple test
> 
>    [root@board ]# echo enabled > /sys/class/rtc/rtc0/device/power/wakeup
>    [root@board ]# rtcwake -s 3 -m mem
>    wakeup from "mem" at Fri Apr 30 09:23:52 2021
>    ...
>    [root@board ]#
> 
>  drivers/rtc/rtc-imxdi.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
> index c1806f4d68e7..63957be25759 100644
> --- a/drivers/rtc/rtc-imxdi.c
> +++ b/drivers/rtc/rtc-imxdi.c
> @@ -98,6 +98,7 @@
>   * @pdev: pointer to platform dev
>   * @rtc: pointer to rtc struct
>   * @ioaddr: IO registers pointer
> + * @norm_irq: irq number of the "normal" irq
>   * @clk: input reference clock
>   * @dsr: copy of the DSR register
>   * @irq_lock: interrupt enable register (DIER) lock
> @@ -109,6 +110,7 @@ struct imxdi_dev {
>  	struct platform_device *pdev;
>  	struct rtc_device *rtc;
>  	void __iomem *ioaddr;
> +	int norm_irq;
>  	struct clk *clk;
>  	u32 dsr;
>  	spinlock_t irq_lock;
> @@ -741,7 +743,7 @@ static void dryice_work(struct work_struct *work)
>  static int __init dryice_rtc_probe(struct platform_device *pdev)
>  {
>  	struct imxdi_dev *imxdi;
> -	int norm_irq, sec_irq;
> +	int sec_irq;
>  	int rc;
>  
>  	imxdi = devm_kzalloc(&pdev->dev, sizeof(*imxdi), GFP_KERNEL);
> @@ -756,9 +758,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&imxdi->irq_lock);
>  
> -	norm_irq = platform_get_irq(pdev, 0);
> -	if (norm_irq < 0)
> -		return norm_irq;
> +	imxdi->norm_irq = platform_get_irq(pdev, 0);
> +	if (imxdi->norm_irq < 0)
> +		return imxdi->norm_irq;
>  
>  	/* the 2nd irq is the security violation irq
>  	 * make this optional, don't break the device tree ABI
> @@ -795,7 +797,7 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  	if (rc != 0)
>  		goto err;
>  
> -	rc = devm_request_irq(&pdev->dev, norm_irq, dryice_irq,
> +	rc = devm_request_irq(&pdev->dev, imxdi->norm_irq, dryice_irq,
>  			      IRQF_SHARED, pdev->name, imxdi);
>  	if (rc) {
>  		dev_warn(&pdev->dev, "interrupt not available.\n");
> @@ -811,6 +813,8 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, imxdi);
>  
> +	device_set_wakeup_capable(&pdev->dev, true);

Maybe it makes sense to simply use device_init_wakeup here.

> +
>  	imxdi->rtc->ops = &dryice_rtc_ops;
>  	imxdi->rtc->range_max = U32_MAX;
>  
> @@ -830,6 +834,8 @@ static int __exit dryice_rtc_remove(struct platform_device *pdev)
>  {
>  	struct imxdi_dev *imxdi = platform_get_drvdata(pdev);
>  
> +	device_set_wakeup_capable(&pdev->dev, false);
> +
>  	flush_work(&imxdi->work);
>  
>  	/* mask all interrupts */
> @@ -847,10 +853,33 @@ static const struct of_device_id dryice_dt_ids[] = {
>  
>  MODULE_DEVICE_TABLE(of, dryice_dt_ids);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int dryice_suspend(struct device *dev)
> +{
> +	struct imxdi_dev *imxdi = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(imxdi->norm_irq);
> +	return 0;
> +}
> +
> +static int dryice_resume(struct device *dev)
> +{
> +	struct imxdi_dev *imxdi = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(imxdi->norm_irq);
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(dryice_pm, dryice_suspend, dryice_resume);
> +

I'm wondering, can't you use dev_pm_set_wake_irq to avoid having to
keep the changes to a minimum?

>  static struct platform_driver dryice_rtc_driver = {
>  	.driver = {
>  		   .name = "imxdi_rtc",
>  		   .of_match_table = dryice_dt_ids,
> +		   .pm = &dryice_pm,
>  		   },
>  	.remove = __exit_p(dryice_rtc_remove),
>  };
> -- 
> 2.20.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: imxdi: add wakeup support

[Martin Kaiser]

(added Stephen for alarmtimer)

Hi Alexandre and all,

Thus wrote Alexandre Belloni (alexandre.belloni@bootlin.com):

> >  	platform_set_drvdata(pdev, imxdi);

> > +	device_set_wakeup_capable(&pdev->dev, true);

> Maybe it makes sense to simply use device_init_wakeup here.

the explanation for device_init_wakeup

"By default, most devices should leave wakeup disabled. The exceptions
are devices that everyone expects to be wakeup sources: keyboards, power
buttons, ..."

made me think that device_set_wakeup_capable is more appropriate here. I
can change this if you want.

However, if I compile rtc-imxdi as a module and use device_init_wakeup,
the module can't be unloaded any more. The reason is that alarmtimer
(kernel/time/alarmtimer.c) starts using rtc-imxdi as its backing rtc
device and holds a reference to it. It seems that alarmtimer has no way
to relinquish its backing rtc device, regardless of any pending alarms.

What is the right approach here? Are there any rtc drivers that act as a
wakeup source and can still be unloaded if compiled as a module?

> > +
> > +static SIMPLE_DEV_PM_OPS(dryice_pm, dryice_suspend, dryice_resume);
> > +

> I'm wondering, can't you use dev_pm_set_wake_irq to avoid having to
> keep the changes to a minimum?

I did a quick test, this seems to work. I'll change it in v2.

Thanks,
Martin

Re: [PATCH] rtc: imxdi: add wakeup support

[Alexandre Belloni]

On 04/05/2021 12:08:58+0200, Martin Kaiser wrote:
> Thus wrote Alexandre Belloni (alexandre.belloni@bootlin.com):
> 
> > >  	platform_set_drvdata(pdev, imxdi);
> 
> > > +	device_set_wakeup_capable(&pdev->dev, true);
> 
> > Maybe it makes sense to simply use device_init_wakeup here.
> 
> the explanation for device_init_wakeup
> 
> "By default, most devices should leave wakeup disabled. The exceptions
> are devices that everyone expects to be wakeup sources: keyboards, power
> buttons, ..."
> 
> made me think that device_set_wakeup_capable is more appropriate here. I
> can change this if you want.
> 

Doesn't everyone expect the RTC to be a wakeup source? :)

> However, if I compile rtc-imxdi as a module and use device_init_wakeup,
> the module can't be unloaded any more. The reason is that alarmtimer
> (kernel/time/alarmtimer.c) starts using rtc-imxdi as its backing rtc
> device and holds a reference to it. It seems that alarmtimer has no way
> to relinquish its backing rtc device, regardless of any pending alarms.
> 
> What is the right approach here? Are there any rtc drivers that act as a
> wakeup source and can still be unloaded if compiled as a module?
> 

Yes, when you don't have alarmtimer ;)
I honestly think the RTC selection needs to be a bit more dynamic but at
the same time, it would not be great to change it at suspend time. I
guess the best way would be to allow module unloading and tracking when
the RTC disappears.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: imxdi: add wakeup support

[Martin Kaiser]

Hi Alexandre,

thanks for the quick reply.

Thus wrote Alexandre Belloni (alexandre.belloni@bootlin.com):

> Doesn't everyone expect the RTC to be a wakeup source? :)

Ok, I'll change this.

> > What is the right approach here? Are there any rtc drivers that act as a
> > wakeup source and can still be unloaded if compiled as a module?

> Yes, when you don't have alarmtimer ;)
> I honestly think the RTC selection needs to be a bit more dynamic but at
> the same time, it would not be great to change it at suspend time. I
> guess the best way would be to allow module unloading and tracking when
> the RTC disappears.

Ok, understood. There's no chance to address this in an rtc driver.

Out of curiousity, I tried adding a .remove_dev function in alarmtimer.
It isn't called when the I try to unload the rtc-imxdi module. It seems
that the module layer checks the refcount and returns an error before we
even attempt to remove the rtc device...

Best regards,
Martin

[PATCH v2] rtc: imxdi: add wakeup support

[Martin Kaiser]

The DryIce-based RTC supports alarms that trigger an interrupt.

Add an option to configure this interrupt as a wakeup source that wakes the
system up from standby mode.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - unconditionally declare rtc-imxdi as wakeup source
 - use dev_pm_set_wake_irq instead of manually coding suspend and resume

simple test (no need to configure the wakeup source via sysfs any more)

   [root@board ]# rtcwake -s 3 -m mem
   wakeup from "mem" at Fri Apr 30 09:23:52 2021
   ...
   [root@board ]#

 drivers/rtc/rtc-imxdi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index c1806f4d68e7..4b712e5ab08a 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -24,6 +24,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/rtc.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
@@ -811,6 +812,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, imxdi);
 
+	device_init_wakeup(&pdev->dev, true);
+	dev_pm_set_wake_irq(&pdev->dev, norm_irq);
+
 	imxdi->rtc->ops = &dryice_rtc_ops;
 	imxdi->rtc->range_max = U32_MAX;
 
-- 
2.20.1

Re: [PATCH v2] rtc: imxdi: add wakeup support

[Alexandre Belloni]

Hi,

On 05/05/2021 18:30:09+0200, Martin Kaiser wrote:
> The DryIce-based RTC supports alarms that trigger an interrupt.
> 
> Add an option to configure this interrupt as a wakeup source that wakes the
> system up from standby mode.
> 
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v2:
>  - unconditionally declare rtc-imxdi as wakeup source
>  - use dev_pm_set_wake_irq instead of manually coding suspend and resume
> 
> simple test (no need to configure the wakeup source via sysfs any more)
> 
>    [root@board ]# rtcwake -s 3 -m mem
>    wakeup from "mem" at Fri Apr 30 09:23:52 2021
>    ...
>    [root@board ]#
> 
>  drivers/rtc/rtc-imxdi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
> index c1806f4d68e7..4b712e5ab08a 100644
> --- a/drivers/rtc/rtc-imxdi.c
> +++ b/drivers/rtc/rtc-imxdi.c
> @@ -24,6 +24,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/rtc.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
> @@ -811,6 +812,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, imxdi);
>  
> +	device_init_wakeup(&pdev->dev, true);
> +	dev_pm_set_wake_irq(&pdev->dev, norm_irq);
> +

That's nice and concise ;) I'll apply that once -rc1 is released


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3] rtc: imxdi: add wakeup support

[Martin Kaiser]

The DryIce-based RTC supports alarms that trigger an interrupt.

Configure this interrupt as a wakeup source that wakes the system up
from standby mode.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v3:
 - fix the commit message, the interrupt is always a wakeup source

v2:
 - unconditionally declare rtc-imxdi as wakeup source
 - use dev_pm_set_wake_irq instead of manually coding suspend and resume

 drivers/rtc/rtc-imxdi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/rtc/rtc-imxdi.c b/drivers/rtc/rtc-imxdi.c
index c1806f4d68e7..4b712e5ab08a 100644
--- a/drivers/rtc/rtc-imxdi.c
+++ b/drivers/rtc/rtc-imxdi.c
@@ -24,6 +24,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/rtc.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
@@ -811,6 +812,9 @@ static int __init dryice_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, imxdi);
 
+	device_init_wakeup(&pdev->dev, true);
+	dev_pm_set_wake_irq(&pdev->dev, norm_irq);
+
 	imxdi->rtc->ops = &dryice_rtc_ops;
 	imxdi->rtc->range_max = U32_MAX;
 
-- 
2.20.1

Re: [PATCH v3] rtc: imxdi: add wakeup support

[Alexandre Belloni]

On Tue, 11 May 2021 18:12:44 +0200, Martin Kaiser wrote:
> The DryIce-based RTC supports alarms that trigger an interrupt.
> 
> Configure this interrupt as a wakeup source that wakes the system up
> from standby mode.

Applied, thanks!

[1/1] rtc: imxdi: add wakeup support
      commit: bcae59d0d45b866d5b9525ea8ece6d671e6767c8

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>