| Message ID | 20210430093210.7034-1-martin@kaiser.cx |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: imxdi: add wakeup support | expand |
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 >
(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
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.
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
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), };
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(-)