rtc: omap: fixes and power-off feature
Subsystems (4)
cover rtc: omap: fixes and power-off feature
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my pmic_power_en patch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]
So my suggestion is to merge the first seven patches, rebase Lokesh series on top of that, and then I rebase my final five patches on top of that in turn. Hopefully, the new power-off handler call chain will have stabilised by then.
Johan
[1] http://www.spinics.net/lists/devicetree/msg50869.html
[2] https://lkml.org/lkml/2014/10/7/30
Johan Hovold (12):
rtc: omap: fix clock-source configuration
rtc: omap: fix missing wakealarm attribute
rtc: omap: fix class-device registration
rtc: omap: remove unused register-base define
rtc: omap: remove redundant debug message
rtc: omap: use dev_info and dev_dbg
rtc: omap: silence bogus power-up reset message at probe
rtc: omap: restore irq state after reading TC registers
rtc: omap: add support for pmic_power_en
rtc: omap: enable wake-up from power off
ARM: dts: am33xx: update rtc node compatible property
ARM: dts: am335x-bone-common: enable power off and rtc wake up
Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- arch/arm/boot/dts/am335x-bone-common.dtsi | 4 + arch/arm/boot/dts/am33xx.dtsi | 2 +- drivers/rtc/rtc-omap.c | 202 ++++++++++++++++----- 4 files changed, 173 insertions(+), 44 deletions(-)
Signature · 1 line
2.0.4
PATCH 1/12 rtc: omap: fix clock-source configuration
Make sure not to reset the clock-source configuration when enabling the 32kHz clock mux.
Until the clock source can be configured through device tree we must not overwrite settings made by the bootloader (e.g. clock-source selection).
32khz clock")
Cc: stable <[email protected]>
--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/rtc/rtc-omap.c +4 -2
@@ -431,8 +431,10 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG );/* enable RTC functional clock */ -if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) -rtc_writel (OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); +if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) { + reg =rtc_read (OMAP_RTC_OSC_REG ); +rtc_writel (reg |OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); + }/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );
Signature · 1 line
2.0.4
Re: [PATCH 01/12] rtc: omap: fix clock-source configuration
Quoted 11 lines · Johan Hovold
Make sure not to reset the clock-source configuration when enabling the 32kHz clock mux.
Until the clock source can be configured through device tree we must not overwrite settings made by the bootloader (e.g. clock-source selection).
Fixes cd914bba03d8 ("drivers/rtc/rtc-omap.c: add support for enabling32khz clock")
Cc: stable <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 24 lines
--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +4
-2@@ -431,8 +431,10 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG );/* enable RTC functional clock */ -if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) -rtc_writel (OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); +if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) { + reg =rtc_read (OMAP_RTC_OSC_REG ); +rtc_writel (reg |OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); + }/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/7bd0b537/attachment.sig>
PATCH 2/12 rtc: omap: fix missing wakealarm attribute
In reply to Johan HovoldThe platform device must be registered as wakeup capable before registering the class device, or the wakealarm attribute will not be created.
Also make sure to unregister the wakeup source on probe errors.
--- drivers/rtc/rtc-omap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/rtc/rtc-omap.c +3 -2
@@ -416,6 +416,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } +device_init_wakeup (&pdev->dev, true); + rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE );if (IS_ERR (rtc)) {
@@ -484,8 +486,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)* is write-only, and always reads as zero...) */ -device_init_wakeup (&pdev->dev, true); -if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT )pr_info ("%s: split power mode\n" , pdev->name);
@@ -495,6 +495,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return 0 ; fail0: +device_init_wakeup (&pdev->dev, false);if (id_entry->driver_data &OMAP_RTC_HAS_KICKER )rtc_writel (0 ,OMAP_RTC_KICK0_REG );pm_runtime_put_sync (&pdev->dev);
Signature · 1 line
2.0.4
Re: [PATCH 02/12] rtc: omap: fix missing wakealarm attribute
Quoted 10 lines · Johan Hovold
The platform device must be registered as wakeup capable before registering the class device, or the wakealarm attribute will not be created.
Also make sure to unregister the wakeup source on probe errors.
Fixes 1d2e2b65d098 ("rtc: omap: restore back (hard-code) wakeup support")Cc stable <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Quoted 36 lines
drivers/rtc/rtc-omap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +3
-2@@ -416,6 +416,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } +device_init_wakeup (&pdev->dev, true); + rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE );if (IS_ERR (rtc)) {@@ -484,8 +486,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)* is write-only, and always reads as zero...) */ -device_init_wakeup (&pdev->dev, true); -if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT )pr_info ("%s: split power mode\n" , pdev->name);@@ -495,6 +495,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return 0 ; fail0: +device_init_wakeup (&pdev->dev, false);if (id_entry->driver_data &OMAP_RTC_HAS_KICKER )rtc_writel (0 ,OMAP_RTC_KICK0_REG );pm_runtime_put_sync (&pdev->dev);Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/0c56a428/attachment.sig>
PATCH 3/12 rtc: omap: fix class-device registration
In reply to Johan HovoldMake sure not to register the class device until after it has been configured and interrupt handlers registered at probe.
Currently, the device is not fully configured (e.g. 24-hour mode) when the class device is registered, something which involves driver callbacks for example to read the current time.
This reordering also prevents user space from enabling an alarm before an interrupt handler has been registered.
Note that the interrupts are now registered using the platform-device rather than class-device name.
--- drivers/rtc/rtc-omap.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
drivers/rtc/rtc-omap.c +17 -15
@@ -147,8 +147,9 @@ static void rtc_wait_not_busy(void)/* now we have ~15 usec to read/write various registers */ } -static irqreturn_t rtc_irq (int irq,void *rtc) +static irqreturn_t rtc_irq (int irq,void *dev_id) { +struct rtc_device *rtc =platform_get_drvdata (dev_id);unsigned long events =0 ;u8 irq_data;
@@ -164,7 +165,8 @@ static irqreturn_t rtc_irq(int irq, void *rtc)if (irq_data &OMAP_RTC_STATUS_1S_EVENT ) events |=RTC_IRQF |RTC_UF ; -rtc_update_irq (rtc,1 , events); +if (rtc) +rtc_update_irq (rtc,1 , events);return IRQ_HANDLED ; }
@@ -416,17 +418,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } -device_init_wakeup (&pdev->dev, true); - - rtc =devm_rtc_device_register (&pdev->dev, pdev->name, - &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); - goto fail0; - } -platform_set_drvdata (pdev, rtc); -/* clear pending irqs, and set 1/second periodic, * which we'll use instead of update irqs */
@@ -450,14 +441,14 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc)) { +dev_name (&pdev->dev), pdev)) {pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , pdev->name, omap_rtc_timer); goto fail0; }if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , -dev_name (&rtc->dev), rtc))) { +dev_name (&pdev->dev), pdev))) {pr_debug ("%s: RTC alarm interrupt IRQ%d already claimed\n" , pdev->name, omap_rtc_alarm); goto fail0;
@@ -492,6 +483,17 @@ static int __init omap_rtc_probe(struct platform_device *pdev)if (reg != new_ctrl)rtc_write (new_ctrl,OMAP_RTC_CTRL_REG ); +device_init_wakeup (&pdev->dev, true); + + rtc =devm_rtc_device_register (&pdev->dev, pdev->name, + &omap_rtc_ops,THIS_MODULE ); +if (IS_ERR (rtc)) { +pr_debug ("%s: can't register RTC device, err %ld\n" , + pdev->name,PTR_ERR (rtc)); + goto fail0; + } +platform_set_drvdata (pdev, rtc); +return 0 ; fail0:
Signature · 1 line
2.0.4
Re: [PATCH 03/12] rtc: omap: fix class-device registration
Quoted 65 lines · Johan Hovold
Make sure not to register the class device until after it has been configured and interrupt handlers registered at probe.
Currently, the device is not fully configured (e.g. 24-hour mode) when the class device is registered, something which involves driver callbacks for example to read the current time.
This reordering also prevents user space from enabling an alarm before an interrupt handler has been registered.
Note that the interrupts are now registered using the platform-device rather than class-device name.
Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)drivers/rtc/rtc-omap.c +4
-13@@ -147,8 +147,9 @@ static void rtc_wait_not_busy(void)/* now we have ~15 usec to read/write various registers */ } -static irqreturn_t rtc_irq (int irq,void *rtc) +static irqreturn_t rtc_irq (int irq,void *dev_id) { +struct rtc_device *rtc =platform_get_drvdata (dev_id);unsigned long events =0 ;u8 irq_data;@@ -164,7 +165,8 @@ static irqreturn_t rtc_irq(int irq, void *rtc)if (irq_data &OMAP_RTC_STATUS_1S_EVENT ) events |=RTC_IRQF |RTC_UF ; -rtc_update_irq (rtc,1 , events); +if (rtc) +rtc_update_irq (rtc,1 , events);return IRQ_HANDLED ; }@@ -416,17 +418,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } -device_init_wakeup (&pdev->dev, true); - - rtc =devm_rtc_device_register (&pdev->dev, pdev->name, - &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); - goto fail0; - } -platform_set_drvdata (pdev, rtc); -/* clear pending irqs, and set 1/second periodic, * which we'll use instead of update irqs */ @@ -450,14 +441,14 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq(&pdev->dev, omap_rtc_timer, rtc_irq, 0,
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 451
Quoted 10 lines
drivers/rtc/rtc-omap.c +2
-2- dev_name(&rtc->dev), rtc)) { + dev_name(&pdev->dev), pdev)) {pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , pdev->name, omap_rtc_timer); goto fail0; }if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , -dev_name (&rtc->dev), rtc))) { +dev_name (&pdev->dev), pdev))) {
i don't get it. Why pass pdev as cookie when all you need is rtc ? Doesn't sound very good to me. Also, IRQ handler *must* be registered only after RTC is registered. That's because if you request an IRQ here, you could already have a pending interrupt and since you won't have a valid rtc pointer you won't be able to clear the interrupt line.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/272463ed/attachment-0001.sig>
Re: [PATCH 03/12] rtc: omap: fix class-device registration
Quoted 74 lines · Felipe Balbi
Quoted 73 lines · Johan Hovold
Make sure not to register the class device until after it has been configured and interrupt handlers registered at probe.
Currently, the device is not fully configured (e.g. 24-hour mode) when the class device is registered, something which involves driver callbacks for example to read the current time.
This reordering also prevents user space from enabling an alarm before an interrupt handler has been registered.
Note that the interrupts are now registered using the platform-device rather than class-device name.
Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)drivers/rtc/rtc-omap.c +5
-14@@ -147,8 +147,9 @@ static void rtc_wait_not_busy(void)/* now we have ~15 usec to read/write various registers */ } -static irqreturn_t rtc_irq (int irq,void *rtc) +static irqreturn_t rtc_irq (int irq,void *dev_id) { +struct rtc_device *rtc =platform_get_drvdata (dev_id);unsigned long events =0 ;u8 irq_data;@@ -164,7 +165,8 @@ static irqreturn_t rtc_irq(int irq, void *rtc)if (irq_data &OMAP_RTC_STATUS_1S_EVENT ) events |=RTC_IRQF |RTC_UF ; -rtc_update_irq (rtc,1 , events); +if (rtc) +rtc_update_irq (rtc,1 , events);return IRQ_HANDLED ; }@@ -416,17 +418,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } -device_init_wakeup (&pdev->dev, true); - - rtc =devm_rtc_device_register (&pdev->dev, pdev->name, - &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); - goto fail0; - } -platform_set_drvdata (pdev, rtc); -/* clear pending irqs, and set 1/second periodic, * which we'll use instead of update irqs */ @@ -450,14 +441,14 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc)) { +dev_name (&pdev->dev), pdev)) {pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , pdev->name, omap_rtc_timer); goto fail0; } if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq(&pdev->dev, omap_rtc_alarm, rtc_irq, 0,
Quoted 8 lines
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 451
Quoted 2 lines
drivers/rtc/rtc-omap.c +1
-1- dev_name(&rtc->dev), rtc))) { + dev_name(&pdev->dev), pdev))) {i don't get it. Why pass pdev as cookie when all you need is rtc ? Doesn't sound very good to me. Also, IRQ handler *must* be registered only after RTC is registered. That's because if you request an IRQ here, you could already have a pending interrupt and since you won't have a valid rtc pointer you won't be able to clear the interrupt line.
We don't need the class device (rtc pointer) to clear the interrupt line. The iomem base is enough, and that is static until Lokesh adds the struct omap_rtc_dev abstraction. Then we pass that as the cookie.
And all RTC interrupts have been disabled, and pending interrupts cleared before registering the handlers so that should not be a problem.
To the contrary, once the class device has been registered user-space could enable interrupts at any time and that is why everything should have been set up before.
Johan
Re: [PATCH 03/12] rtc: omap: fix class-device registration
Hi,
Quoted 21 lines · Johan Hovold
Quoted 12 lines
Quoted 12 lines
drivers/rtc/rtc-omap.c +1
-1@@ -450,14 +441,14 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc)) { +dev_name (&pdev->dev), pdev)) {pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , pdev->name, omap_rtc_timer); goto fail0; } if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq(&pdev->dev, omap_rtc_alarm, rtc_irq, 0,Quoted 8 lines
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 451
Quoted 2 lines
drivers/rtc/rtc-omap.c +1
-1- dev_name(&rtc->dev), rtc))) { + dev_name(&pdev->dev), pdev))) {i don't get it. Why pass pdev as cookie when all you need is rtc ? Doesn't sound very good to me. Also, IRQ handler *must* be registered only after RTC is registered. That's because if you request an IRQ here, you could already have a pending interrupt and since you won't have a valid rtc pointer you won't be able to clear the interrupt line.
Quoted 3 lines
We don't need the class device (rtc pointer) to clear the interrupt line. The iomem base is enough, and that is static until Lokesh adds the struct
omap_rtc_devabstraction. Then we pass that as the cookie.
I wonder how that patch will look like ;-)
Quoted 6 lines
And all RTC interrupts have been disabled, and pending interrupts cleared before registering the handlers so that should not be a problem.
To the contrary, once the class device has been registered user-space could enable interrupts at any time and that is why everything should have been set up before.
alright, got it. Thanks
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141013/5e5940c7/attachment.sig>
PATCH 4/12 rtc: omap: remove unused register-base define
In reply to Johan HovoldRemove register-base define, which is no longer used.
--- drivers/rtc/rtc-omap.c | 2 -- 1 file changed, 2 deletions(-)
drivers/rtc/rtc-omap.c +0 -2
@@ -41,8 +41,6 @@#define DRIVER_NAME "omap_rtc" -#define OMAP_RTC_BASE 0xfffb4800 -/* RTC registers */ #define OMAP_RTC_SECONDS_REG 0x00#define OMAP_RTC_MINUTES_REG 0x04
Signature · 1 line
2.0.4
Re: [PATCH 04/12] rtc: omap: remove unused register-base define
Quoted 3 lines · Johan Hovold
Remove register-base define, which is no longer used.
Signed-off-by Johan Hovold <[email protected]>
Quoted 20 lines
--- drivers/rtc/rtc-omap.c | 2 -- 1 file changed, 2 deletions(-)drivers/rtc/rtc-omap.c +0
-2@@ -41,8 +41,6 @@#define DRIVER_NAME "omap_rtc" -#define OMAP_RTC_BASE 0xfffb4800 -/* RTC registers */ #define OMAP_RTC_SECONDS_REG 0x00#define OMAP_RTC_MINUTES_REG 0x04Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/250cc496/attachment.sig>
PATCH 5/12 rtc: omap: remove redundant debug message
In reply to Johan HovoldRemove redundant debug message.
The corresponding error has already been logged by rtc core.
--- drivers/rtc/rtc-omap.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/rtc/rtc-omap.c +2 -4
@@ -485,11 +485,9 @@ static int __init omap_rtc_probe(struct platform_device *pdev) rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); +if (IS_ERR (rtc)) goto fail0; - } +platform_set_drvdata (pdev, rtc);return 0 ;
Signature · 1 line
2.0.4
Re: [PATCH 05/12] rtc: omap: remove redundant debug message
Quoted 5 lines · Johan Hovold
Remove redundant debug message.
The corresponding error has already been logged by rtc core.
Signed-off-by Johan Hovold <[email protected]>
Quoted 25 lines
--- drivers/rtc/rtc-omap.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/rtc/rtc-omap.c +2
-4@@ -485,11 +485,9 @@ static int __init omap_rtc_probe(struct platform_device *pdev) rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); +if (IS_ERR (rtc)) goto fail0; - } +platform_set_drvdata (pdev, rtc);return 0 ;Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/063501fc/attachment.sig>
PATCH 6/12 rtc: omap: use dev_info and dev_dbg
In reply to Johan HovoldUse dev_info and dev_dbg rather than pr_info and pr_debug.
--- drivers/rtc/rtc-omap.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
drivers/rtc/rtc-omap.c +9 -10
@@ -392,13 +392,13 @@ static int __init omap_rtc_probe(struct platform_device *pdev) omap_rtc_timer =platform_get_irq (pdev,0 );if (omap_rtc_timer <=0 ) { -pr_debug ("%s: no update irq?\n" , pdev->name); +dev_dbg (&pdev->dev,"no update irq\n" );return -ENOENT ; } omap_rtc_alarm =platform_get_irq (pdev,1 );if (omap_rtc_alarm <=0 ) { -pr_debug ("%s: no alarm irq?\n" , pdev->name); +dev_dbg (&pdev->dev,"no alarm irq\n" );return -ENOENT ; }
@@ -430,8 +430,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -pr_info ("%s: RTC power up reset detected\n" , - pdev->name); +dev_info (&pdev->dev,"RTC power up reset detected\n" );rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); }if (reg & (u8 )OMAP_RTC_STATUS_ALARM )
@@ -440,22 +439,22 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 ,dev_name (&pdev->dev), pdev)) { -pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_timer); +dev_dbg (&pdev->dev,"RTC timer interrupt IRQ%d already claimed\n" , + omap_rtc_timer); goto fail0; }if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 ,dev_name (&pdev->dev), pdev))) { -pr_debug ("%s: RTC alarm interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_alarm); +dev_dbg (&pdev->dev,"RTC alarm interrupt IRQ%d already claimed\n" , + omap_rtc_alarm); goto fail0; }/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (OMAP_RTC_CTRL_REG );if (reg & (u8 )OMAP_RTC_CTRL_STOP ) -pr_info ("%s: already running\n" , pdev->name); +dev_info (&pdev->dev,"already running\n" );/* force to 24 hour mode */ new_ctrl = reg & (OMAP_RTC_CTRL_SPLIT |OMAP_RTC_CTRL_AUTO_COMP );
@@ -476,7 +475,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)*/ if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT ) -pr_info ("%s: split power mode\n" , pdev->name); +dev_info (&pdev->dev,"split power mode\n" );if (reg != new_ctrl)rtc_write (new_ctrl,OMAP_RTC_CTRL_REG );
Signature · 1 line
2.0.4
Re: [PATCH 06/12] rtc: omap: use dev_info and dev_dbg
Quoted 3 lines · Johan Hovold
Use
dev_infoanddev_dbgrather thanpr_infoandpr_debug.Signed-off-by Johan Hovold <[email protected]>
thanks!
Quoted 74 lines
--- drivers/rtc/rtc-omap.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)drivers/rtc/rtc-omap.c +9
-10@@ -392,13 +392,13 @@ static int __init omap_rtc_probe(struct platform_device *pdev) omap_rtc_timer =platform_get_irq (pdev,0 );if (omap_rtc_timer <=0 ) { -pr_debug ("%s: no update irq?\n" , pdev->name); +dev_dbg (&pdev->dev,"no update irq\n" );return -ENOENT ; } omap_rtc_alarm =platform_get_irq (pdev,1 );if (omap_rtc_alarm <=0 ) { -pr_debug ("%s: no alarm irq?\n" , pdev->name); +dev_dbg (&pdev->dev,"no alarm irq\n" );return -ENOENT ; }@@ -430,8 +430,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -pr_info ("%s: RTC power up reset detected\n" , - pdev->name); +dev_info (&pdev->dev,"RTC power up reset detected\n" );rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); }if (reg & (u8 )OMAP_RTC_STATUS_ALARM )@@ -440,22 +439,22 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* handle periodic and alarm irqs */ if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 ,dev_name (&pdev->dev), pdev)) { -pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_timer); +dev_dbg (&pdev->dev,"RTC timer interrupt IRQ%d already claimed\n" , + omap_rtc_timer); goto fail0; }if ((omap_rtc_timer != omap_rtc_alarm) && (devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 ,dev_name (&pdev->dev), pdev))) { -pr_debug ("%s: RTC alarm interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_alarm); +dev_dbg (&pdev->dev,"RTC alarm interrupt IRQ%d already claimed\n" , + omap_rtc_alarm); goto fail0; }/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (OMAP_RTC_CTRL_REG );if (reg & (u8 )OMAP_RTC_CTRL_STOP ) -pr_info ("%s: already running\n" , pdev->name); +dev_info (&pdev->dev,"already running\n" );/* force to 24 hour mode */ new_ctrl = reg & (OMAP_RTC_CTRL_SPLIT |OMAP_RTC_CTRL_AUTO_COMP );@@ -476,7 +475,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)*/ if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT ) -pr_info ("%s: split power mode\n" , pdev->name); +dev_info (&pdev->dev,"split power mode\n" );if (reg != new_ctrl)rtc_write (new_ctrl,OMAP_RTC_CTRL_REG );Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/74a7d161/attachment.sig>
PATCH 7/12 rtc: omap: silence bogus power-up reset message at probe
In reply to Johan HovoldSome legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
--- drivers/rtc/rtc-omap.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
drivers/rtc/rtc-omap.c +17 -6
@@ -118,6 +118,12 @@*/ #define OMAP_RTC_HAS_32KCLK_EN BIT(2) +/* +* Some legacy RTC IP revisions has a power-up reset flag which later +* revisions lack. +*/ +#define OMAP_RTC_HAS_POWER_UP_RESET BIT(3) +static void __iomem *rtc_base;#define rtc_read (addr) readb(rtc_base + (addr))
@@ -347,6 +353,7 @@ static int omap_rtc_timer;static struct platform_device_id omap_rtc_devtype[] = { { .name =DRIVER_NAME , + .driver_data =OMAP_RTC_HAS_POWER_UP_RESET , }, [OMAP_RTC_DATA_AM3352_IDX ] = { .name ="am3352-rtc" ,
@@ -376,7 +383,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev) {struct resource *res;struct rtc_device *rtc; -u8 reg, new_ctrl; +u8 reg, mask, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id;
@@ -429,12 +436,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG ); -if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -dev_info (&pdev->dev,"RTC power up reset detected\n" ); -rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); + + mask =OMAP_RTC_STATUS_ALARM ; +if (id_entry->driver_data &OMAP_RTC_HAS_POWER_UP_RESET ) { + mask |=OMAP_RTC_STATUS_POWER_UP ; +if (reg &OMAP_RTC_STATUS_POWER_UP ) +dev_info (&pdev->dev,"RTC power up reset detected\n" ); } -if (reg & (u8 )OMAP_RTC_STATUS_ALARM ) -rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG ); + +if (reg & mask) +rtc_write (reg & mask,OMAP_RTC_STATUS_REG );/* handle periodic and alarm irqs */ if (devm_request_irq(&pdev->dev, omap_rtc_timer, rtc_irq, 0,
Signature · 1 line
2.0.4
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Quoted 8 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Signed-off-by Johan Hovold <[email protected]>
Quoted 63 lines
--- drivers/rtc/rtc-omap.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/rtc/rtc-omap.c +17
-6@@ -118,6 +118,12 @@*/ #define OMAP_RTC_HAS_32KCLK_EN BIT(2) +/* +* Some legacy RTC IP revisions has a power-up reset flag which later +* revisions lack. +*/ +#define OMAP_RTC_HAS_POWER_UP_RESET BIT(3) +static void __iomem *rtc_base;#define rtc_read (addr) readb(rtc_base + (addr))@@ -347,6 +353,7 @@ static int omap_rtc_timer;static struct platform_device_id omap_rtc_devtype[] = { { .name =DRIVER_NAME , + .driver_data =OMAP_RTC_HAS_POWER_UP_RESET , }, [OMAP_RTC_DATA_AM3352_IDX ] = { .name ="am3352-rtc" ,@@ -376,7 +383,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev) {struct resource *res;struct rtc_device *rtc; -u8 reg, new_ctrl; +u8 reg, mask, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id;@@ -429,12 +436,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG ); -if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -dev_info (&pdev->dev,"RTC power up reset detected\n" ); -rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); + + mask =OMAP_RTC_STATUS_ALARM ; +if (id_entry->driver_data &OMAP_RTC_HAS_POWER_UP_RESET ) { + mask |=OMAP_RTC_STATUS_POWER_UP ; +if (reg &OMAP_RTC_STATUS_POWER_UP ) +dev_info (&pdev->dev,"RTC power up reset detected\n" ); } -if (reg & (u8 )OMAP_RTC_STATUS_ALARM ) -rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG ); + +if (reg & mask) +rtc_write (reg & mask,OMAP_RTC_STATUS_REG );/* handle periodic and alarm irqs */ if (devm_request_irq(&pdev->dev, omap_rtc_timer, rtc_irq, 0,Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/60129a7b/attachment.sig>
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Signed-off-by Johan Hovold <[email protected]>Reviewed-by Felipe Balbi <[email protected]>
oh, btw. This should be done for DT probes too.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/2bd6b1c2/attachment.sig>
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Quoted 9 lines · Felipe Balbi
Quoted 8 lines · Felipe Balbi
Quoted 7 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Johan Hovold <[email protected]>Reviewed-by Felipe Balbi <[email protected]>oh, btw. This should be done for DT probes too.
What do you mean? This is for DT probes (of "ti,da830-rtc" and "ti,am3352-rtc"). DT probing of legacy RTC IPs is currently not supported.
Are you suggesting I should add a new compatible string for that?
Johan
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Hi,
Quoted 14 lines · Johan Hovold
Quoted 13 lines · Felipe Balbi
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Signed-off-by Johan Hovold <[email protected]>Reviewed-by Felipe Balbi <[email protected]>
Quoted 7 lines
Quoted 1 line
oh, btw. This should be done for DT probes too.
What do you mean? This is for DT probes (of "ti,da830-rtc" and "ti,am3352-rtc"). DT probing of legacy RTC IPs is currently not supported.
Are you suggesting I should add a new compatible string for that?
it seemed to me that you're only passing the new flag through platform_device_id table, not DT table. Did I miss something ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141011/4f0bcc7f/attachment.sig>
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Quoted 19 lines · Felipe Balbi
Hi,
Quoted 16 lines · Johan Hovold
Quoted 14 lines · Felipe Balbi
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Signed-off-by Johan Hovold <[email protected]>Reviewed-by Felipe Balbi <[email protected]>oh, btw. This should be done for DT probes too.
Quoted 8 lines
Quoted 5 lines
What do you mean? This is for DT probes (of "ti,da830-rtc" and "ti,am3352-rtc"). DT probing of legacy RTC IPs is currently not supported.
Are you suggesting I should add a new compatible string for that?
it seemed to me that you're only passing the new flag through
platform_device_idtable, not DT table. Did I miss something ?
Ah, that's just the way the driver currently encodes the IP revision differences. The platform_device_id entries are references from of_device_id data. Should probably clean that up as well.
Thanks again for the review. I'll post a new version when I'm back from Düsseldorf and ELC-E/Plumbers.
Johan
Re: [PATCH 07/12] rtc: omap: silence bogus power-up reset message at probe
Hi,
Quoted 24 lines · Johan Hovold
Quoted 23 lines · Felipe Balbi
Hi,
Quoted 20 lines · Johan Hovold
Quoted 9 lines · Felipe Balbi
Quoted 8 lines · Felipe Balbi
Quoted 7 lines · Johan Hovold
Some legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Johan Hovold <[email protected]>Reviewed-by Felipe Balbi <[email protected]>oh, btw. This should be done for DT probes too.
What do you mean? This is for DT probes (of "ti,da830-rtc" and "ti,am3352-rtc"). DT probing of legacy RTC IPs is currently not supported.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Are you suggesting I should add a new compatible string for that?
it seemed to me that you're only passing the new flag through
platform_device_idtable, not DT table. Did I miss something ?Ah, that's just the way the driver currently encodes the IP revision differences. The
platform_device_identries are references fromof_device_iddata. Should probably clean that up as well.
oh, alright :-)
Quoted 1 line
Thanks again for the review. I'll post a new version when I'm back from
no problem.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141013/b6ee0637/attachment.sig>
PATCH 8/12 rtc: omap: restore irq state after reading TC registers
In reply to Johan HovoldMake sure to restore local irq state when reading the timer/calendar (TC) registers, so that omap_rtc_read_time() can be called with interrupts disabled.
--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/rtc/rtc-omap.c +4 -2
@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);rtc_wait_not_busy (); tm->tm_sec =rtc_read (OMAP_RTC_SECONDS_REG );
@@ -250,7 +252,7 @@ static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm) tm->tm_mon =rtc_read (OMAP_RTC_MONTHS_REG ); tm->tm_year =rtc_read (OMAP_RTC_YEARS_REG ); -local_irq_enable (); +local_irq_restore (flags);bcd2tm (tm);return 0 ;
Signature · 1 line
2.0.4
Re: [PATCH 08/12] rtc: omap: restore irq state after reading TC registers
Hi,
Quoted 14 lines · Johan Hovold
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that
omap_rtc_read_time()can be called with interrupts disabled.Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +0
-0
drivers/rtc/rtc-omap.c static void bcd2tm(struct rtc_time *tm) · line 245
Quoted 8 lines
drivers/rtc/rtc-omap.c +3
-1@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);
you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/bd63d886/attachment-0001.sig>
Re: [PATCH 08/12] rtc: omap: restore irq state after reading TC registers
Quoted 18 lines · Felipe Balbi
Hi,
Quoted 15 lines · Johan Hovold
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that
omap_rtc_read_time()can be called with interrupts disabled.Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +0
-0@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)
Quoted 11 lines
drivers/rtc/rtc-omap.c static void bcd2tm(struct rtc_time *tm) · line 245
Quoted 7 lines
drivers/rtc/rtc-omap.c +3
-1static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
The driver uses local_irq* throughout to guarantee registers are not read or written during an update event.
In fact, at least on AM33xx, this is not even necessary when reading (as opposed to writing) the TC registers, but I did not dare change that without knowing how the legacy platforms work in this respect.
Do you suggest doing this conversion as part of, or on top of, this series?
Johan
Re: [PATCH 08/12] rtc: omap: restore irq state after reading TC registers
Quoted 34 lines · Johan Hovold
Quoted 18 lines · Felipe Balbi
Hi,
Quoted 15 lines · Johan Hovold
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that
omap_rtc_read_time()can be called with interrupts disabled.Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +0
-0@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)Quoted 11 lines
drivers/rtc/rtc-omap.c static void bcd2tm(struct rtc_time *tm) · line 245
Quoted 7 lines
drivers/rtc/rtc-omap.c +3
-1static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
The driver uses local_irq* throughout to guarantee registers are not read or written during an update event.
Quoted 6 lines
In fact, at least on AM33xx, this is not even necessary when reading (as opposed to writing) the TC registers, but I did not dare change that without knowing how the legacy platforms work in this respect.
Do you suggest doing this conversion as part of, or on top of, this series?
probably on top of is. Safer that way.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141011/dbf042e4/attachment.sig>
Re: [PATCH 08/12] rtc: omap: restore irq state after reading TC registers
Quoted 35 lines · Felipe Balbi
Quoted 34 lines · Johan Hovold
Quoted 18 lines · Felipe Balbi
Hi,
Quoted 15 lines · Johan Hovold
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that
omap_rtc_read_time()can be called with interrupts disabled.Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +0
-0@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)Quoted 11 lines
drivers/rtc/rtc-omap.c static void bcd2tm(struct rtc_time *tm) · line 245
Quoted 7 lines
drivers/rtc/rtc-omap.c +3
-1static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
The driver uses local_irq* throughout to guarantee registers are not read or written during an update event.
Quoted 8 lines
Quoted 6 lines
In fact, at least on AM33xx, this is not even necessary when reading (as opposed to writing) the TC registers, but I did not dare change that without knowing how the legacy platforms work in this respect.
Do you suggest doing this conversion as part of, or on top of, this series?
probably on top of is. Safer that way.
I didn't include this conversion in v2 as you may have noticed. In fact, I'm not even sure it should be done.
Using local_irq* "breaks" RT, but if we change it to spin_lock_irq* and RT preempts us, we may miss the 15us access window which would break the driver:
"If the ARM accesses the TC registers outside of the access
period, then the access is not guaranteed."
On am33xx this only affects updating the TC (timer/calendar) or alarm registers, but the driver currently disables interrupts also on reading the time (possibly because the early IP revisions did not have the shadow read registers).
Johan
Re: [PATCH 08/12] rtc: omap: restore irq state after reading TC registers
Quoted 52 lines · Johan Hovold
Quoted 35 lines · Felipe Balbi
Quoted 34 lines · Johan Hovold
Quoted 18 lines · Felipe Balbi
Hi,
Quoted 15 lines · Johan Hovold
Make sure to restore local irq state when reading the timer/calendar (TC) registers, so that
omap_rtc_read_time()can be called with interrupts disabled.Signed-off-by Johan Hovold <[email protected]>--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/rtc/rtc-omap.c +0
-0@@ -239,8 +239,10 @@ static void bcd2tm(struct rtc_time *tm)Quoted 11 lines
drivers/rtc/rtc-omap.c static void bcd2tm(struct rtc_time *tm) · line 245
Quoted 7 lines
drivers/rtc/rtc-omap.c +3
-1static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +unsigned long flags; +/* we don't report wday/yday/isdst ... */ -local_irq_disable (); +local_irq_save (flags);you should really convert these to a real spin_lock_irq*(), that's because local_irq* do not get re-written with RT patchset, so this pretty much "breaks" RT.
The driver uses local_irq* throughout to guarantee registers are not read or written during an update event.
Quoted 8 lines
Quoted 6 lines
In fact, at least on AM33xx, this is not even necessary when reading (as opposed to writing) the TC registers, but I did not dare change that without knowing how the legacy platforms work in this respect.
Do you suggest doing this conversion as part of, or on top of, this series?
probably on top of is. Safer that way.
I didn't include this conversion in v2 as you may have noticed. In fact, I'm not even sure it should be done.
Using local_irq* "breaks" RT, but if we change it to spin_lock_irq* and RT preempts us, we may miss the 15us access window which would break the driver:
Quoted 7 lines
"If the ARM accesses the TC registers outside of the access
period, then the access is not guaranteed."On am33xx this only affects updating the TC (timer/calendar) or alarm registers, but the driver currently disables interrupts also on reading the time (possibly because the early IP revisions did not have the shadow read registers).
that's alright, better not to change the behavior in this case. But as a future patch, you might want to convert this to a raw_spinlock. That won't get preempted.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141023/d6b894e5/attachment.sig>
PATCH 9/12 rtc: omap: add support for pmic_power_en
In reply to Johan HovoldAdd new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the pmic_power_en pin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.
This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
--- Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 107 ++++++++++++++++++++- 2 files changed, 110 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/rtc/rtc-omap.txt +8 -1
@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller +Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system power + through pmic_power_en + Example: rtc at 1c23000 {
@@ -18,4 +24,5 @@ rtc at 1c23000 { interrupts = <19 19>; interrupt-parent = <&intc>; + ti,system-power-controller; };
drivers/rtc/rtc-omap.c +102 -5
190 lines not on this page · raw message
Signature · 1 line
2.0.4
Re: [PATCH 09/12] rtc: omap: add support for pmic_power_en
Hi,
Quoted 6 lines · Johan Hovold
drivers/rtc/rtc-omap.c +2
-0@@ -124,11 +138,18 @@*/ #define OMAP_RTC_HAS_POWER_UP_RESET BIT(3) +/* +* Some RTC IP revisions can control an external PMIC via the pmic_power_en
drivers/rtc/rtc-omap.c line 152
Quoted 10 lines
drivers/rtc/rtc-omap.c +5
-0+* pin. +*/ +#define OMAP_RTC_HAS_PMIC_MODE BIT(4) +static void __iomem *rtc_base;#define rtc_read (addr) readb(rtc_base + (addr))#define rtc_write (val, addr) writeb(val, rtc_base + (addr)) +#define rtc_readl (addr) readl(rtc_base + (addr))
looks like this should be part of another patch. Or, at a minimum (since it's so minor), it deserves a mention on commit log.
Quoted 9 lines
drivers/rtc/rtc-omap.c +2
-0#define rtc_writel (val, addr) writel(val, rtc_base + (addr))@@ -338,6 +359,61 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)return 0 ; } +static struct platform_device *omap_rtc_power_off_dev; +
drivers/rtc/rtc-omap.c static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) · line 373
Quoted 10 lines
drivers/rtc/rtc-omap.c +10
-0+/* +* omap_rtc_poweroff: RTC-controlled power off +* +* The RTC can be used to control an external PMIC via the pmic_power_en pin, +* which can be configured to transition to OFF on ALARM2 events. +* +* Notes: +* The two-second alarm offset is the shortest offset possible as the alarm +* registers must be set before the next timer update and the offset +* calculation is to heavy for everything to be done within a single access
s/to/too
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 513
Quoted 13 lines
drivers/rtc/rtc-omap.c +5
-3@@ -425,10 +505,12 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } -/* clear pending irqs, and set 1/second periodic, -* which we'll use instead of update irqs +/* +* disable interrupts +* +* NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG );
bug fix, should be part of a separate patch and Cc stable :-)
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/bc17e9ca/attachment.sig>
Re: [PATCH 09/12] rtc: omap: add support for pmic_power_en
Quoted 15 lines · Felipe Balbi
Hi,
Quoted 12 lines · Johan Hovold
drivers/rtc/rtc-omap.c +6
-0@@ -124,11 +138,18 @@*/ #define OMAP_RTC_HAS_POWER_UP_RESET BIT(3) +/* +* Some RTC IP revisions can control an external PMIC via the pmic_power_en +* pin. +*/ +#define OMAP_RTC_HAS_PMIC_MODE BIT(4) +static void __iomem *rtc_base;
Quoted 7 lines
drivers/rtc/rtc-omap.c line 152
Quoted 4 lines
drivers/rtc/rtc-omap.c +1
-0#define rtc_read (addr) readb(rtc_base + (addr))#define rtc_write (val, addr) writeb(val, rtc_base + (addr)) +#define rtc_readl (addr) readl(rtc_base + (addr))looks like this should be part of another patch. Or, at a minimum (since it's so minor), it deserves a mention on commit log.
Ok, I'll add it separately.
Quoted 7 lines
Quoted 7 lines
drivers/rtc/rtc-omap.c +0
-0#define rtc_writel (val, addr) writel(val, rtc_base + (addr))@@ -338,6 +359,61 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)return 0 ; }
Quoted 14 lines
drivers/rtc/rtc-omap.c static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) · line 373
Quoted 12 lines
drivers/rtc/rtc-omap.c +12
-0+static struct platform_device *omap_rtc_power_off_dev; + +/* +* omap_rtc_poweroff: RTC-controlled power off +* +* The RTC can be used to control an external PMIC via the pmic_power_en pin, +* which can be configured to transition to OFF on ALARM2 events. +* +* Notes: +* The two-second alarm offset is the shortest offset possible as the alarm +* registers must be set before the next timer update and the offset +* calculation is to heavy for everything to be done within a single access s/to/too
Thanks.
Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-omap.c +0
-0@@ -425,10 +505,12 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); }
Quoted 11 lines
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 513
Quoted 9 lines
drivers/rtc/rtc-omap.c +5
-3-/* clear pending irqs, and set 1/second periodic, -* which we'll use instead of update irqs +/* +* disable interrupts +* +* NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG );bug fix, should be part of a separate patch and Cc stable :-)
Yeah, I guess you're right. Even though ALARM2 would never have been set by Linux before this patch we might as well fix this separately.
Thanks,
Johan
Re: [PATCH 09/12] rtc: omap: add support for pmic_power_en
In reply to Felipe BalbiQuoted 13 lines · Felipe Balbi
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 513
Quoted 9 lines · Johan Hovold
drivers/rtc/rtc-omap.c +5
-3-/* clear pending irqs, and set 1/second periodic, -* which we'll use instead of update irqs +/* +* disable interrupts +* +* NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG );bug fix, should be part of a separate patch and Cc stable :-)
I made this a separate fix in v2 but did not mark it for stable for now as no one has been bitten by it (ALARM2 is never enabled by the current driver).
Johan
Re: [PATCH 09/12] rtc: omap: add support for pmic_power_en
Quoted 15 lines · Johan Hovold
Quoted 13 lines · Felipe Balbi
drivers/rtc/rtc-omap.c static int __init omap_rtc_probe(struct platform_device *pdev) · line 513
Quoted 9 lines · Johan Hovold
drivers/rtc/rtc-omap.c +5
-3-/* clear pending irqs, and set 1/second periodic, -* which we'll use instead of update irqs +/* +* disable interrupts +* +* NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG );bug fix, should be part of a separate patch and Cc stable :-)
Quoted 3 lines
I made this a separate fix in v2 but did not mark it for stable for now as no one has been bitten by it (ALARM2 is never enabled by the current driver).
makes sense.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141023/b4bbb60b/attachment.sig>
PATCH 10/12 rtc: omap: enable wake-up from power off
In reply to Johan HovoldThe ALARM interrupt must not be disabled during shutdown in order to be able to power up the system using an RTC alarm.
--- drivers/rtc/rtc-omap.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/rtc/rtc-omap.c +9 -1
@@ -670,7 +670,15 @@ static SIMPLE_DEV_PM_OPS(omap_rtc_pm_ops, omap_rtc_suspend, omap_rtc_resume);static void omap_rtc_shutdown (struct platform_device *pdev) { -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +u8 mask; + +/* +* Keep the ALARM interrupt enabled to allow the system to power up on +* alarm events. +*/ + mask =rtc_read (OMAP_RTC_INTERRUPTS_REG ); + mask &=OMAP_RTC_INTERRUPTS_IT_ALARM ; +rtc_write (mask,OMAP_RTC_INTERRUPTS_REG ); }MODULE_ALIAS ("platform:omap_rtc" );
Signature · 1 line
2.0.4
Re: [PATCH 10/12] rtc: omap: enable wake-up from power off
Quoted 4 lines · Johan Hovold
The ALARM interrupt must not be disabled during shutdown in order to be able to power up the system using an RTC alarm.
Signed-off-by Johan Hovold <[email protected]>
nicely done!
Quoted 28 lines
--- drivers/rtc/rtc-omap.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)drivers/rtc/rtc-omap.c +9
-1@@ -670,7 +670,15 @@ static SIMPLE_DEV_PM_OPS(omap_rtc_pm_ops, omap_rtc_suspend, omap_rtc_resume);static void omap_rtc_shutdown (struct platform_device *pdev) { -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +u8 mask; + +/* +* Keep the ALARM interrupt enabled to allow the system to power up on +* alarm events. +*/ + mask =rtc_read (OMAP_RTC_INTERRUPTS_REG ); + mask &=OMAP_RTC_INTERRUPTS_IT_ALARM ; +rtc_write (mask,OMAP_RTC_INTERRUPTS_REG ); }MODULE_ALIAS ("platform:omap_rtc" );Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/cefe0c51/attachment-0001.sig>
PATCH 11/12 ARM: dts: am33xx: update rtc node compatible property
In reply to Johan HovoldEnable am33xx specific RTC features (e.g. PMIC control) by adding "ti,am3352-rtc" to the compatible property of the rtc node.
--- arch/arm/boot/dts/am33xx.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/boot/dts/am33xx.dtsi +1 -1
@@ -411,7 +411,7 @@ }; rtc: rtc at 44e3e000 { - compatible = "ti,da830-rtc"; + compatible = "ti,am3352-rtc", "ti,da830-rtc"; reg = <0x44e3e000 0x1000>; interrupts = <75 76>;
Signature · 1 line
2.0.4
Re: [PATCH 11/12] ARM: dts: am33xx: update rtc node compatible property
Quoted 4 lines · Johan Hovold
Enable am33xx specific RTC features (e.g. PMIC control) by adding "ti,am3352-rtc" to the compatible property of the rtc node.
Signed-off-by Johan Hovold <[email protected]>
Quoted 20 lines
--- arch/arm/boot/dts/am33xx.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/boot/dts/am33xx.dtsi +1
-1@@ -411,7 +411,7 @@ }; rtc: rtc at 44e3e000 { - compatible = "ti,da830-rtc"; + compatible = "ti,am3352-rtc", "ti,da830-rtc"; reg = <0x44e3e000 0x1000>; interrupts = <75 76>;Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/c1ae66ae/attachment.sig>
PATCH 12/12 ARM: dts: am335x-bone-common: enable power off and rtc wake up
In reply to Johan HovoldConfigure the RTC as system-power controller, which allows the system to be powered off as well as woken up again on subsequent RTC alarms.
Note that the PMIC needs to be put in SLEEP (rather than OFF) mode to maintain RTC power. Specifically, this means that the PMIC ti,pmic-shutdown-controller property must be left unset in order to be able to wake up on RTC alarms.
Tested on BeagleBone Black (rev A5).
--- arch/arm/boot/dts/am335x-bone-common.dtsi | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm/boot/dts/am335x-bone-common.dtsi +4 -0
@@ -298,3 +298,7 @@ cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>; cd-inverted; }; + +&rtc { + ti,system-power-controller; +};
Signature · 1 line
2.0.4
Re: [PATCH 12/12] ARM: dts: am335x-bone-common: enable power off and rtc wake up
Quoted 9 lines · Johan Hovold
Configure the RTC as system-power controller, which allows the system to be powered off as well as woken up again on subsequent RTC alarms.
Note that the PMIC needs to be put in SLEEP (rather than OFF) mode to maintain RTC power. Specifically, this means that the PMIC ti,pmic-shutdown-controller property must be left unset in order to be able to wake up on RTC alarms.
Tested on BeagleBone Black (rev A5).
one thing though. IIRC, rtc-only mode does not work with ES1.0 and beagle-bone white uses that. Perhaps move this from bone-common to boneblack.dts ?
other than that
Quoted 20 lines
Signed-off-by Johan Hovold <[email protected]>--- arch/arm/boot/dts/am335x-bone-common.dtsi | 4 ++++ 1 file changed, 4 insertions(+)arch/arm/boot/dts/am335x-bone-common.dtsi +4
-0@@ -298,3 +298,7 @@ cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>; cd-inverted; }; + +&rtc { + ti,system-power-controller; +};Signature · 1 line
2.0.4
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/aa2fa97d/attachment.sig>
Re: [PATCH 12/12] ARM: dts: am335x-bone-common: enable power off and rtc wake up
Quoted 9 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Configure the RTC as system-power controller, which allows the system to be powered off as well as woken up again on subsequent RTC alarms.
Note that the PMIC needs to be put in SLEEP (rather than OFF) mode to maintain RTC power. Specifically, this means that the PMIC ti,pmic-shutdown-controller property must be left unset in order to be able to wake up on RTC alarms.
Quoted 5 lines
Quoted 1 line
Tested on BeagleBone Black (rev A5).
one thing though. IIRC, rtc-only mode does not work with ES1.0 and beagle-bone white uses that. Perhaps move this from bone-common to
boneblack.dts?
Yeah, I had a quick look at the errata and you seem to be correct. I'll move it to boneblack.dts.
Quoted 3 lines
other than that
Reviewed-by Felipe Balbi <[email protected]>
Thanks,
Johan
HI,
Quoted 7 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
Quoted 7 lines
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my
pmic_power_enpatch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]
is this power-off feature "RTC-only mode" ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141010/0d041bc2/attachment.sig>
Quoted 18 lines · Felipe Balbi
HI,
Quoted 7 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
Quoted 7 lines
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my
pmic_power_enpatch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]
Quoted 1 line
is this power-off feature "RTC-only mode" ?
Yes, I believe so, at least as long as RTC power is maintained. The pmic_power_en pin can otherwise still be used to power off.
On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Thanks for the fast review, Felipe.
Johan
Quoted 4 lines · Johan Hovold
Quoted 2 lines · Felipe Balbi
is this power-off feature "RTC-only mode" ?
Quoted 8 lines
Yes, I believe so, at least as long as RTC power is maintained. The
pmic_power_enpin can otherwise still be used to power off.On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Thanks for the fast review, Felipe.
Great :) I assume adding the regulator support to this driver should also help in the 35xx case?
That is assuming that RTC power can be configured as a Linux regulator on 35xx.
Regards,
Tony
Quoted 8 lines · Tony Lindgren
Quoted 7 lines · Johan Hovold
Quoted 2 lines · Felipe Balbi
is this power-off feature "RTC-only mode" ?
Yes, I believe so, at least as long as RTC power is maintained. The
pmic_power_enpin can otherwise still be used to power off.
Quoted 8 lines
Quoted 5 lines
On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Thanks for the fast review, Felipe.
Great :) I assume adding the regulator support to this driver should also help in the 35xx case?
If I remember correctly both OMAP and Sitara 35xx have no on-chip RTC so they won't be using this driver.
Quoted 2 lines
That is assuming that RTC power can be configured as a Linux regulator on 35xx.
Johan
Quoted 13 lines · Johan Hovold
Quoted 12 lines · Tony Lindgren
Quoted 11 lines · Johan Hovold
Quoted 2 lines · Felipe Balbi
is this power-off feature "RTC-only mode" ?
Yes, I believe so, at least as long as RTC power is maintained. The
pmic_power_enpin can otherwise still be used to power off.On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Thanks for the fast review, Felipe.
Great :) I assume adding the regulator support to this driver should also help in the 35xx case?
If I remember correctly both OMAP and Sitara 35xx have no on-chip RTC so they won't be using this driver.
Sorry I made a typo.. I meanto to ask if adding the regulator helps in your 335x case (not 35xx case)?
Regards,
Tony
Quoted 16 lines · Tony Lindgren
Quoted 15 lines · Johan Hovold
Quoted 14 lines · Tony Lindgren
Quoted 12 lines · Johan Hovold
Quoted 2 lines · Felipe Balbi
is this power-off feature "RTC-only mode" ?
Yes, I believe so, at least as long as RTC power is maintained. The
pmic_power_enpin can otherwise still be used to power off.On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Thanks for the fast review, Felipe.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Great :) I assume adding the regulator support to this driver should also help in the 35xx case?
If I remember correctly both OMAP and Sitara 35xx have no on-chip RTC so they won't be using this driver.
Sorry I made a typo.. I meanto to ask if adding the regulator helps in your 335x case (not 35xx case)?
Ah, sorry, that's what I first read before I noticed you wrote "35xx" twice. :)
Yes, RTC power can be configured as a regulator on am33xx (e.g. provided by LDO1 of the PMIC on BBB). I guess you would still need to configure it as always-on to be able to use the RTC-only mode, though.
Johan
Quoted 20 lines · Johan Hovold
Quoted 19 lines · Tony Lindgren
Quoted 18 lines · Johan Hovold
Quoted 16 lines · Tony Lindgren
Quoted 4 lines · Johan Hovold
Quoted 2 lines · Felipe Balbi
is this power-off feature "RTC-only mode" ?
Quoted 8 lines
Yes, I believe so, at least as long as RTC power is maintained. The
pmic_power_enpin can otherwise still be used to power off.On the BeagleBone the PMIC can be configured to enter OFF or SLEEP mode on power off, where the latter (and default) mode keeps the RTC power supply enabled.
Thanks for the fast review, Felipe.
Great :) I assume adding the regulator support to this driver should also help in the 35xx case?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
If I remember correctly both OMAP and Sitara 35xx have no on-chip RTC so they won't be using this driver.
Sorry I made a typo.. I meanto to ask if adding the regulator helps in your 335x case (not 35xx case)?
Ah, sorry, that's what I first read before I noticed you wrote "35xx" twice. :)
Yes sorry my nerves are doing things on their own for some stuff it seems like :)
Quoted 3 lines
Yes, RTC power can be configured as a regulator on am33xx (e.g. provided by LDO1 of the PMIC on BBB). I guess you would still need to configure it as always-on to be able to use the RTC-only mode, though.
OK great.
Tony
Hi,
Quoted 15 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my
pmic_power_enpatch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]
Quoted 4 lines
So my suggestion is to merge the first seven patches, rebase Lokesh series on top of that, and then I rebase my final five patches on top of that in turn. Hopefully, the new power-off handler call chain will have stabilised by then.
BTW, how do you test this series ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141015/80c03d36/attachment.sig>
Quoted 18 lines · Felipe Balbi
Hi,
Quoted 15 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my
pmic_power_enpatch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]
Quoted 6 lines
Quoted 4 lines
So my suggestion is to merge the first seven patches, rebase Lokesh series on top of that, and then I rebase my final five patches on top of that in turn. Hopefully, the new power-off handler call chain will have stabilised by then.
BTW, how do you test this series ?
Set a 30 second wakealarm using the sysfs attribute of the rtc class device and power off the BBB:
echo +30 >wakealarm; poweroff
Johan
Felipe Balbi
Tested-byQuoted 24 lines · Johan Hovold
Quoted 23 lines · Felipe Balbi
Hi,
Quoted 19 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
I noticed yesterday that Lokesh Vutla recently posted some patches to add regulator support to this driver. [1] While that series has some issues that needs to be addressed, I still think it could be a good idea to rebase my
pmic_power_enpatch on top of it as it adds some long overdue device abstraction. That would be needed anyway in order to support the new power-off infrastructure that's in the pipe in a clean way. [2]So my suggestion is to merge the first seven patches, rebase Lokesh series on top of that, and then I rebase my final five patches on top of that in turn. Hopefully, the new power-off handler call chain will have stabilised by then.
Quoted 6 lines
Quoted 1 line
BTW, how do you test this series ?
Set a 30 second wakealarm using the sysfs attribute of the rtc class device and power off the BBB:
echo +30 >wakealarm; poweroff
sweet!!!! It worked just fine :-)
Too bad there's yet another DRM bug:
[ 271.150158] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 271.150158] [ 271.159725] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.17.0-08392-g7fe2269-dirty #326 [ 271.168567] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 271.176668] [<c00132f0>] (show_stack) from [<c06573a4>] (dump_stack+0x8c/0xa4) [ 271.184215] [<c06573a4>] (dump_stack) from [<c0654f10>] (panic+0xa0/0x220) [ 271.191405] [<c0654f10>] (panic) from [<c0049e54>] (do_exit+0x974/0x9d0) [ 271.198410] [<c0049e54>] (do_exit) from [<c0067694>] (SyS_reboot+0x14c/0x1e8) [ 271.205870] [<c0067694>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 271.214023] drm_kms_helper: panic occurred, switching back to text console [ 271.221312] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
In any case, board still powered up after 30 seconds(-ish). So this gets a:
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141015/828ceb03/attachment-0001.sig>
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Quoted 1 line · Felipe Balbi
BTW, how do you test this series ?
Set a 30 second wakealarm using the sysfs attribute of the rtc class device and power off the BBB:
echo +30 >wakealarm; poweroff
sweet!!!! It worked just fine :-)
:)
Quoted 14 lines
Too bad there's yet another DRM bug:
[ 271.150158] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 271.150158] [ 271.159725] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.17.0-08392-g7fe2269-dirty #326 [ 271.168567] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 271.176668] [<c00132f0>] (show_stack) from [<c06573a4>] (dump_stack+0x8c/0xa4) [ 271.184215] [<c06573a4>] (dump_stack) from [<c0654f10>] (panic+0xa0/0x220) [ 271.191405] [<c0654f10>] (panic) from [<c0049e54>] (do_exit+0x974/0x9d0) [ 271.198410] [<c0049e54>] (do_exit) from [<c0067694>] (SyS_reboot+0x14c/0x1e8) [ 271.205870] [<c0067694>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 271.214023] drm_kms_helper: panic occurred, switching back to text console [ 271.221312] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
Quoted 4 lines
In any case, board still powered up after 30 seconds(-ish). So this gets a:
Tested-by Felipe Balbi <[email protected]>
Thanks for testing. I didn't include your Tested-by in v2 due to the invasiveness of the new device-abstraction patch. Still works fine on BBB here though.
Johan
Quoted 29 lines · Johan Hovold
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Quoted 1 line · Felipe Balbi
BTW, how do you test this series ?
Set a 30 second wakealarm using the sysfs attribute of the rtc class device and power off the BBB:
echo +30 >wakealarm; poweroff
sweet!!!! It worked just fine :-)
:)
Quoted 14 lines
Too bad there's yet another DRM bug:
[ 271.150158] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 271.150158] [ 271.159725] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.17.0-08392-g7fe2269-dirty #326 [ 271.168567] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 271.176668] [<c00132f0>] (show_stack) from [<c06573a4>] (dump_stack+0x8c/0xa4) [ 271.184215] [<c06573a4>] (dump_stack) from [<c0654f10>] (panic+0xa0/0x220) [ 271.191405] [<c0654f10>] (panic) from [<c0049e54>] (do_exit+0x974/0x9d0) [ 271.198410] [<c0049e54>] (do_exit) from [<c0067694>] (SyS_reboot+0x14c/0x1e8) [ 271.205870] [<c0067694>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 271.214023] drm_kms_helper: panic occurred, switching back to text console [ 271.221312] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
Quoted 8 lines
Quoted 4 lines
In any case, board still powered up after 30 seconds(-ish). So this gets a:
Tested-by Felipe Balbi <[email protected]>Thanks for testing. I didn't include your Tested-by in v2 due to the invasiveness of the new device-abstraction patch. Still works fine on BBB here though.
alright, if I have time I'll test again, but not this week.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141023/0faf7154/attachment.sig>
Quoted 33 lines · Felipe Balbi
Quoted 32 lines · Johan Hovold
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Johan Hovold
Quoted 1 line · Felipe Balbi
BTW, how do you test this series ?
Set a 30 second wakealarm using the sysfs attribute of the rtc class device and power off the BBB:
echo +30 >wakealarm; poweroff
sweet!!!! It worked just fine :-)
:)
Quoted 17 lines
Too bad there's yet another DRM bug:
[ 271.150158] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 271.150158] [ 271.159725] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.17.0-08392-g7fe2269-dirty #326 [ 271.168567] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 271.176668] [<c00132f0>] (show_stack) from [<c06573a4>] (dump_stack+0x8c/0xa4) [ 271.184215] [<c06573a4>] (dump_stack) from [<c0654f10>] (panic+0xa0/0x220) [ 271.191405] [<c0654f10>] (panic) from [<c0049e54>] (do_exit+0x974/0x9d0) [ 271.198410] [<c0049e54>] (do_exit) from [<c0067694>] (SyS_reboot+0x14c/0x1e8) [ 271.205870] [<c0067694>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 271.214023] drm_kms_helper: panic occurred, switching back to text console [ 271.221312] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000In any case, board still powered up after 30 seconds(-ish). So this gets a:
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Tested-by Felipe Balbi <[email protected]>Thanks for testing. I didn't include your Tested-by in v2 due to the invasiveness of the new device-abstraction patch. Still works fine on BBB here though.
alright, if I have time I'll test again, but not this week.
Great, thanks! I believe Andrew usually adds any late Reviewed-by or Tested-by tags while the patches live in his tree.
Johan
v2 cover rtc: omap: fixes and power-off feature
In reply to Johan HovoldThis series fixes a few issues with the omap rtc-driver, cleans up a bit, adds device abstraction, and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
Changes since v1 include:
- add device abstraction
- add structured device-type info
- fix interrupt disable at probe separately
- register interrupts after class device as before
- add helper to read raw bcd time
- more clean ups
- add copyright entry
- enable pmic control only for Beaglebone Black
I realised that it was better to keep this patch series self-contained and include the device abstraction in it, rather than rely on some patches yet to be posted (contrary to what I proposed in v1). It should now be straight forward to add deferred-probing and regulator support on top of this series instead. [1]
I've kept Felipe's Reviewed-by tags only on the first and last couple of patches which have not changed (in any significant way).
For the record, the new functionality is easily tested on BBB with:
echo +30 >/sys/class/rtc/rtc0/wakealarm; poweroff
There are some patches floating around to add power-off handler call chains [2] as well as an RFC for a generic "poweroff-source" property [3]. Since it is unclear if, when (and through which tree) this will eventually go in, I suggest merging this series as-is now and do the trivial updates to use these new interfaces once the infrastructure is in place.
Johan
[1] https://lkml.org/lkml/2014/10/9/427
[2] https://lkml.org/lkml/2014/10/7/30
[3] https://lkml.org/lkml/2014/10/7/654
Johan Hovold (20):
rtc: omap: fix clock-source configuration
rtc: omap: fix missing wakealarm attribute
rtc: omap: fix interrupt disable at probe
rtc: omap: clean up probe error handling
rtc: omap: fix class-device registration
rtc: omap: remove unused register-base define
rtc: omap: use dev_info
rtc: omap: make platform-device id table const
rtc: omap: add device abstraction
rtc: omap: remove DRIVER_NAME macro
rtc: omap: add structured device-type info
rtc: omap: silence bogus power-up reset message at probe
rtc: omap: add helper to read raw bcd time
rtc: omap: add helper to read 32-bit registers
rtc: omap: add support for pmic_power_en
rtc: omap: enable wake-up from power off
rtc: omap: fix minor coding style issues
rtc: omap: add copyright entry
ARM: dts: am33xx: update rtc-node compatible property
ARM: dts: am335x-boneblack: enable power off and rtc wake up
Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- arch/arm/boot/dts/am335x-boneblack.dts | 4 + arch/arm/boot/dts/am33xx.dtsi | 2 +- drivers/rtc/rtc-omap.c | 540 +++++++++++++-------- 4 files changed, 355 insertions(+), 200 deletions(-)
Signature · 1 line
2.0.4
PATCH v2 1/20 rtc: omap: fix clock-source configuration
Make sure not to reset the clock-source configuration when enabling the 32kHz clock mux.
Until the clock source can be configured through device tree we must not overwrite settings made by the bootloader (e.g. clock-source selection).
32khz clock")
--- drivers/rtc/rtc-omap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/rtc/rtc-omap.c +4 -2
@@ -431,8 +431,10 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG );/* enable RTC functional clock */ -if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) -rtc_writel (OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); +if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) { + reg =rtc_read (OMAP_RTC_OSC_REG ); +rtc_writel (reg |OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); + }/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );
Signature · 1 line
2.0.4
PATCH v2 2/20 rtc: omap: fix missing wakealarm attribute
In reply to Johan HovoldThe platform device must be registered as wakeup capable before registering the class device, or the wakealarm attribute will not be created.
Also make sure to unregister the wakeup source on probe errors.
--- drivers/rtc/rtc-omap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/rtc/rtc-omap.c +3 -2
@@ -416,6 +416,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } +device_init_wakeup (&pdev->dev, true); + rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE );if (IS_ERR (rtc)) {
@@ -484,8 +486,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)* is write-only, and always reads as zero...) */ -device_init_wakeup (&pdev->dev, true); -if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT )pr_info ("%s: split power mode\n" , pdev->name);
@@ -495,6 +495,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return 0 ; fail0: +device_init_wakeup (&pdev->dev, false);if (id_entry->driver_data &OMAP_RTC_HAS_KICKER )rtc_writel (0 ,OMAP_RTC_KICK0_REG );pm_runtime_put_sync (&pdev->dev);
Signature · 1 line
2.0.4
PATCH v2 3/20 rtc: omap: fix interrupt disable at probe
In reply to Johan HovoldUse writel instead of writeb when disabling interrupts at probe as ALARM2 is not cleared otherwise on some IP-block revisions (e.g. AM3352).
Note that the driver currently never enables the ALARM2 interrupt.
--- drivers/rtc/rtc-omap.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/rtc/rtc-omap.c +5 -3
@@ -427,10 +427,12 @@ static int __init omap_rtc_probe(struct platform_device *pdev) }platform_set_drvdata (pdev, rtc); -/* clear pending irqs, and set 1/second periodic, -* which we'll use instead of update irqs +/* +* disable interrupts +* +* NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG );/* enable RTC functional clock */ if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) {
Signature · 1 line
2.0.4
PATCH v2 4/20 rtc: omap: clean up probe error handling
In reply to Johan HovoldRemove some debug messages and return errors from subsystems rather than always fail with -EIO.
Note that the class-registration error has already been logged by rtc core.
--- drivers/rtc/rtc-omap.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-)
drivers/rtc/rtc-omap.c +18 -23
@@ -379,6 +379,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)u8 reg, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id; +int ret; of_id =of_match_device (omap_rtc_of_match, &pdev->dev);if (of_id)
@@ -391,16 +392,12 @@ static int __init omap_rtc_probe(struct platform_device *pdev) } omap_rtc_timer =platform_get_irq (pdev,0 ); -if (omap_rtc_timer <=0 ) { -pr_debug ("%s: no update irq?\n" , pdev->name); +if (omap_rtc_timer <=0 )return -ENOENT ; - } omap_rtc_alarm =platform_get_irq (pdev,1 ); -if (omap_rtc_alarm <=0 ) { -pr_debug ("%s: no alarm irq?\n" , pdev->name); +if (omap_rtc_alarm <=0 )return -ENOENT ; - } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); rtc_base =devm_ioremap_resource (&pdev->dev, res);
@@ -421,9 +418,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev) rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE );if (IS_ERR (rtc)) { -pr_debug ("%s: can't register RTC device, err %ld\n" , - pdev->name,PTR_ERR (rtc)); - goto fail0; + ret =PTR_ERR (rtc); + goto err; }platform_set_drvdata (pdev, rtc);
@@ -451,18 +447,16 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG );/* handle periodic and alarm irqs */ -if (devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc)) { -pr_debug ("%s: RTC timer interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_timer); - goto fail0; - } -if ((omap_rtc_timer != omap_rtc_alarm) && - (devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , -dev_name (&rtc->dev), rtc))) { -pr_debug ("%s: RTC alarm interrupt IRQ%d already claimed\n" , - pdev->name, omap_rtc_alarm); - goto fail0; + ret =devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , +dev_name (&rtc->dev), rtc); +if (ret) + goto err; + +if (omap_rtc_timer != omap_rtc_alarm) { + ret =devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , +dev_name (&rtc->dev), rtc); +if (ret) + goto err; }/* On boards with split power, RTC_ON_NOFF won't reset the RTC */
@@ -496,13 +490,14 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return 0 ; -fail0: +err:device_init_wakeup (&pdev->dev, false);if (id_entry->driver_data &OMAP_RTC_HAS_KICKER )rtc_writel (0 ,OMAP_RTC_KICK0_REG );pm_runtime_put_sync (&pdev->dev);pm_runtime_disable (&pdev->dev); -return -EIO ; + +return ret; } static int __exit omap_rtc_remove(struct platform_device *pdev)
Signature · 1 line
2.0.4
PATCH v2 5/20 rtc: omap: fix class-device registration
In reply to Johan HovoldMake sure not to register the class device until after the device has been configured.
Currently, the device is not fully configured (e.g. 24-hour mode) when the class device is registered, something which involves driver callbacks for example to read the current time.
--- drivers/rtc/rtc-omap.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)
drivers/rtc/rtc-omap.c +23 -23
@@ -413,16 +413,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); } -device_init_wakeup (&pdev->dev, true); - - rtc =devm_rtc_device_register (&pdev->dev, pdev->name, - &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { - ret =PTR_ERR (rtc); - goto err; - } -platform_set_drvdata (pdev, rtc); -/* * disable interrupts *
@@ -446,19 +436,6 @@ static int __init omap_rtc_probe(struct platform_device *pdev)if (reg & (u8 )OMAP_RTC_STATUS_ALARM )rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG ); -/* handle periodic and alarm irqs */ - ret =devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc); -if (ret) - goto err; - -if (omap_rtc_timer != omap_rtc_alarm) { - ret =devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , -dev_name (&rtc->dev), rtc); -if (ret) - goto err; - } -/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (OMAP_RTC_CTRL_REG );if (reg & (u8 )OMAP_RTC_CTRL_STOP )
@@ -488,6 +465,29 @@ static int __init omap_rtc_probe(struct platform_device *pdev)if (reg != new_ctrl)rtc_write (new_ctrl,OMAP_RTC_CTRL_REG ); +device_init_wakeup (&pdev->dev, true); + + rtc =devm_rtc_device_register (&pdev->dev, pdev->name, + &omap_rtc_ops,THIS_MODULE ); +if (IS_ERR (rtc)) { + ret =PTR_ERR (rtc); + goto err; + } +platform_set_drvdata (pdev, rtc); + +/* handle periodic and alarm irqs */ + ret =devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , +dev_name (&rtc->dev), rtc); +if (ret) + goto err; + +if (omap_rtc_timer != omap_rtc_alarm) { + ret =devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , +dev_name (&rtc->dev), rtc); +if (ret) + goto err; + } +return 0 ; err:
Signature · 1 line
2.0.4
PATCH v2 6/20 rtc: omap: remove unused register-base define
In reply to Johan HovoldRemove register-base define, which is no longer used.
--- drivers/rtc/rtc-omap.c | 2 -- 1 file changed, 2 deletions(-)
drivers/rtc/rtc-omap.c +0 -2
@@ -41,8 +41,6 @@#define DRIVER_NAME "omap_rtc" -#define OMAP_RTC_BASE 0xfffb4800 -/* RTC registers */ #define OMAP_RTC_SECONDS_REG 0x00#define OMAP_RTC_MINUTES_REG 0x04
Signature · 1 line
2.0.4
PATCH v2 7/20 rtc: omap: use dev_info
In reply to Johan HovoldUse dev_info rather than pr_info.
--- drivers/rtc/rtc-omap.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/rtc/rtc-omap.c +3 -4
@@ -427,8 +427,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (OMAP_RTC_STATUS_REG );if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -pr_info ("%s: RTC power up reset detected\n" , - pdev->name); +dev_info (&pdev->dev,"RTC power up reset detected\n" );rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); }if (reg & (u8 )OMAP_RTC_STATUS_ALARM )
@@ -437,7 +436,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (OMAP_RTC_CTRL_REG );if (reg & (u8 )OMAP_RTC_CTRL_STOP ) -pr_info ("%s: already running\n" , pdev->name); +dev_info (&pdev->dev,"already running\n" );/* force to 24 hour mode */ new_ctrl = reg & (OMAP_RTC_CTRL_SPLIT |OMAP_RTC_CTRL_AUTO_COMP );
@@ -458,7 +457,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)*/ if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT ) -pr_info ("%s: split power mode\n" , pdev->name); +dev_info (&pdev->dev,"split power mode\n" );if (reg != new_ctrl)rtc_write (new_ctrl,OMAP_RTC_CTRL_REG );
Signature · 1 line
2.0.4
PATCH v2 8/20 rtc: omap: make platform-device id table const
In reply to Johan HovoldMake platform-device id table const.
--- drivers/rtc/rtc-omap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rtc/rtc-omap.c +1 -1
@@ -342,7 +342,7 @@ static int omap_rtc_timer;#define OMAP_RTC_DATA_AM3352_IDX 1#define OMAP_RTC_DATA_DA830_IDX 2 -static struct platform_device_id omap_rtc_devtype[] = { +static const struct platform_device_id omap_rtc_devtype[] = { { .name =DRIVER_NAME , },
Signature · 1 line
2.0.4
PATCH v2 9/20 rtc: omap: add device abstraction
In reply to Johan HovoldAdd struct omap_rtc to hold previously global data as well as the IP-block feature flags.
Also convert the register-access macros to proper inline helper functions.
--- drivers/rtc/rtc-omap.c | 239 +++++++++++++++++++++++++++---------------------- 1 file changed, 133 insertions(+), 106 deletions(-)
drivers/rtc/rtc-omap.c +133 -106
@@ -118,26 +118,42 @@*/ #define OMAP_RTC_HAS_32KCLK_EN BIT(2) -static void __iomem *rtc_base; +struct omap_rtc { +struct rtc_device *rtc; +void __iomem *base; +int irq_alarm; +int irq_timer; +u8 interrupts_reg; +unsigned long flags; +}; -#define rtc_read (addr) readb(rtc_base + (addr)) -#define rtc_write (val, addr) writeb(val, rtc_base + (addr)) +static inline u8 rtc_read (struct omap_rtc *rtc,unsigned int reg) +{ +return readb (rtc->base + reg); +} -#define rtc_writel (val, addr) writel(val, rtc_base + (addr)) +static inline void rtc_write (struct omap_rtc *rtc,unsigned int reg,u8 val) +{ +writeb (val, rtc->base + reg); +} +static inline void rtc_writel (struct omap_rtc *rtc,unsigned int reg,u32 val) +{ +writel (val, rtc->base + reg); +}/* we rely on the rtc framework to handle locking (rtc->ops_lock), * so the only other requirement is that register accesses which * require BUSY to be clear are made with IRQs locally disabled */ -static void rtc_wait_not_busy (void ) +static void rtc_wait_not_busy (struct omap_rtc *rtc) {int count =0 ;u8 status;/* BUSY may stay active for 1/32768 second (~30 usec) */ for (count =0 ; count <50 ; count++) { - status =rtc_read (OMAP_RTC_STATUS_REG ); + status =rtc_read (rtc,OMAP_RTC_STATUS_REG );if ((status & (u8 )OMAP_RTC_STATUS_BUSY ) ==0 )break ;udelay (1 );
@@ -145,16 +161,17 @@ static void rtc_wait_not_busy(void)/* now we have ~15 usec to read/write various registers */ } -static irqreturn_t rtc_irq (int irq,void *rtc) +static irqreturn_t rtc_irq (int irq,void *dev_id) { +struct omap_rtc *rtc = dev_id;unsigned long events =0 ;u8 irq_data; - irq_data =rtc_read (OMAP_RTC_STATUS_REG ); + irq_data =rtc_read (rtc,OMAP_RTC_STATUS_REG );/* alarm irq? */ if (irq_data &OMAP_RTC_STATUS_ALARM ) { -rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG ); +rtc_write (rtc,OMAP_RTC_STATUS_REG ,OMAP_RTC_STATUS_ALARM ); events |=RTC_IRQF |RTC_AF ; }
@@ -162,23 +179,21 @@ static irqreturn_t rtc_irq(int irq, void *rtc)if (irq_data &OMAP_RTC_STATUS_1S_EVENT ) events |=RTC_IRQF |RTC_UF ; -rtc_update_irq (rtc,1 , events); +rtc_update_irq (rtc->rtc,1 , events);return IRQ_HANDLED ; }static int omap_rtc_alarm_irq_enable (struct device *dev,unsigned int enabled) { +struct omap_rtc *rtc =dev_get_drvdata (dev);u8 reg, irqwake_reg =0 ; -struct platform_device *pdev =to_platform_device (dev); -const struct platform_device_id *id_entry = -platform_get_device_id (pdev);local_irq_disable (); -rtc_wait_not_busy (); - reg =rtc_read (OMAP_RTC_INTERRUPTS_REG ); -if (id_entry->driver_data &OMAP_RTC_HAS_IRQWAKEEN ) - irqwake_reg =rtc_read (OMAP_RTC_IRQWAKEEN ); +rtc_wait_not_busy (rtc); + reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); +if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) + irqwake_reg =rtc_read (rtc,OMAP_RTC_IRQWAKEEN );if (enabled) { reg |=OMAP_RTC_INTERRUPTS_IT_ALARM ;
@@ -187,10 +202,10 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) reg &= ~OMAP_RTC_INTERRUPTS_IT_ALARM ; irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN ; } -rtc_wait_not_busy (); -rtc_write (reg,OMAP_RTC_INTERRUPTS_REG ); -if (id_entry->driver_data &OMAP_RTC_HAS_IRQWAKEEN ) -rtc_write (irqwake_reg,OMAP_RTC_IRQWAKEEN ); +rtc_wait_not_busy (rtc); +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , reg); +if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +rtc_write (rtc,OMAP_RTC_IRQWAKEEN , irqwake_reg);local_irq_enable ();return 0 ;
@@ -231,16 +246,18 @@ static void bcd2tm(struct rtc_time *tm)static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) { +struct omap_rtc *rtc =dev_get_drvdata (dev); +/* we don't report wday/yday/isdst ... */ local_irq_disable (); -rtc_wait_not_busy (); +rtc_wait_not_busy (rtc); - tm->tm_sec =rtc_read (OMAP_RTC_SECONDS_REG ); - tm->tm_min =rtc_read (OMAP_RTC_MINUTES_REG ); - tm->tm_hour =rtc_read (OMAP_RTC_HOURS_REG ); - tm->tm_mday =rtc_read (OMAP_RTC_DAYS_REG ); - tm->tm_mon =rtc_read (OMAP_RTC_MONTHS_REG ); - tm->tm_year =rtc_read (OMAP_RTC_YEARS_REG ); + tm->tm_sec =rtc_read (rtc,OMAP_RTC_SECONDS_REG ); + tm->tm_min =rtc_read (rtc,OMAP_RTC_MINUTES_REG ); + tm->tm_hour =rtc_read (rtc,OMAP_RTC_HOURS_REG ); + tm->tm_mday =rtc_read (rtc,OMAP_RTC_DAYS_REG ); + tm->tm_mon =rtc_read (rtc,OMAP_RTC_MONTHS_REG ); + tm->tm_year =rtc_read (rtc,OMAP_RTC_YEARS_REG );local_irq_enable ();
@@ -250,17 +267,19 @@ static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm)static int omap_rtc_set_time (struct device *dev,struct rtc_time *tm) { +struct omap_rtc *rtc =dev_get_drvdata (dev); +if (tm2bcd (tm) <0 )return -EINVAL ;local_irq_disable (); -rtc_wait_not_busy (); +rtc_wait_not_busy (rtc); -rtc_write (tm->tm_year,OMAP_RTC_YEARS_REG ); -rtc_write (tm->tm_mon,OMAP_RTC_MONTHS_REG ); -rtc_write (tm->tm_mday,OMAP_RTC_DAYS_REG ); -rtc_write (tm->tm_hour,OMAP_RTC_HOURS_REG ); -rtc_write (tm->tm_min,OMAP_RTC_MINUTES_REG ); -rtc_write (tm->tm_sec,OMAP_RTC_SECONDS_REG ); +rtc_write (rtc,OMAP_RTC_YEARS_REG , tm->tm_year); +rtc_write (rtc,OMAP_RTC_MONTHS_REG , tm->tm_mon); +rtc_write (rtc,OMAP_RTC_DAYS_REG , tm->tm_mday); +rtc_write (rtc,OMAP_RTC_HOURS_REG , tm->tm_hour); +rtc_write (rtc,OMAP_RTC_MINUTES_REG , tm->tm_min); +rtc_write (rtc,OMAP_RTC_SECONDS_REG , tm->tm_sec);local_irq_enable ();
@@ -269,20 +288,22 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)static int omap_rtc_read_alarm (struct device *dev,struct rtc_wkalrm *alm) { +struct omap_rtc *rtc =dev_get_drvdata (dev); +local_irq_disable (); -rtc_wait_not_busy (); +rtc_wait_not_busy (rtc); - alm->time.tm_sec =rtc_read (OMAP_RTC_ALARM_SECONDS_REG ); - alm->time.tm_min =rtc_read (OMAP_RTC_ALARM_MINUTES_REG ); - alm->time.tm_hour =rtc_read (OMAP_RTC_ALARM_HOURS_REG ); - alm->time.tm_mday =rtc_read (OMAP_RTC_ALARM_DAYS_REG ); - alm->time.tm_mon =rtc_read (OMAP_RTC_ALARM_MONTHS_REG ); - alm->time.tm_year =rtc_read (OMAP_RTC_ALARM_YEARS_REG ); + alm->time.tm_sec =rtc_read (rtc,OMAP_RTC_ALARM_SECONDS_REG ); + alm->time.tm_min =rtc_read (rtc,OMAP_RTC_ALARM_MINUTES_REG ); + alm->time.tm_hour =rtc_read (rtc,OMAP_RTC_ALARM_HOURS_REG ); + alm->time.tm_mday =rtc_read (rtc,OMAP_RTC_ALARM_DAYS_REG ); + alm->time.tm_mon =rtc_read (rtc,OMAP_RTC_ALARM_MONTHS_REG ); + alm->time.tm_year =rtc_read (rtc,OMAP_RTC_ALARM_YEARS_REG );local_irq_enable ();bcd2tm (&alm->time); - alm->enabled = !!(rtc_read (OMAP_RTC_INTERRUPTS_REG ) + alm->enabled = !!(rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ) &OMAP_RTC_INTERRUPTS_IT_ALARM );return 0 ;
@@ -290,27 +311,25 @@ static int omap_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)static int omap_rtc_set_alarm (struct device *dev,struct rtc_wkalrm *alm) { +struct omap_rtc *rtc =dev_get_drvdata (dev);u8 reg, irqwake_reg =0 ; -struct platform_device *pdev =to_platform_device (dev); -const struct platform_device_id *id_entry = -platform_get_device_id (pdev);if (tm2bcd (&alm->time) <0 )return -EINVAL ;local_irq_disable (); -rtc_wait_not_busy (); +rtc_wait_not_busy (rtc); -rtc_write (alm->time.tm_year,OMAP_RTC_ALARM_YEARS_REG ); -rtc_write (alm->time.tm_mon,OMAP_RTC_ALARM_MONTHS_REG ); -rtc_write (alm->time.tm_mday,OMAP_RTC_ALARM_DAYS_REG ); -rtc_write (alm->time.tm_hour,OMAP_RTC_ALARM_HOURS_REG ); -rtc_write (alm->time.tm_min,OMAP_RTC_ALARM_MINUTES_REG ); -rtc_write (alm->time.tm_sec,OMAP_RTC_ALARM_SECONDS_REG ); +rtc_write (rtc,OMAP_RTC_ALARM_YEARS_REG , alm->time.tm_year); +rtc_write (rtc,OMAP_RTC_ALARM_MONTHS_REG , alm->time.tm_mon); +rtc_write (rtc,OMAP_RTC_ALARM_DAYS_REG , alm->time.tm_mday); +rtc_write (rtc,OMAP_RTC_ALARM_HOURS_REG , alm->time.tm_hour); +rtc_write (rtc,OMAP_RTC_ALARM_MINUTES_REG , alm->time.tm_min); +rtc_write (rtc,OMAP_RTC_ALARM_SECONDS_REG , alm->time.tm_sec); - reg =rtc_read (OMAP_RTC_INTERRUPTS_REG ); -if (id_entry->driver_data &OMAP_RTC_HAS_IRQWAKEEN ) - irqwake_reg =rtc_read (OMAP_RTC_IRQWAKEEN ); + reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); +if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) + irqwake_reg =rtc_read (rtc,OMAP_RTC_IRQWAKEEN );if (alm->enabled) { reg |=OMAP_RTC_INTERRUPTS_IT_ALARM ;
@@ -319,9 +338,9 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) reg &= ~OMAP_RTC_INTERRUPTS_IT_ALARM ; irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN ; } -rtc_write (reg,OMAP_RTC_INTERRUPTS_REG ); -if (id_entry->driver_data &OMAP_RTC_HAS_IRQWAKEEN ) -rtc_write (irqwake_reg,OMAP_RTC_IRQWAKEEN ); +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , reg); +if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +rtc_write (rtc,OMAP_RTC_IRQWAKEEN , irqwake_reg);local_irq_enable ();
@@ -336,9 +355,6 @@ static struct rtc_class_ops omap_rtc_ops = { .alarm_irq_enable = omap_rtc_alarm_irq_enable, }; -static int omap_rtc_alarm; -static int omap_rtc_timer; -#define OMAP_RTC_DATA_AM3352_IDX 1#define OMAP_RTC_DATA_DA830_IDX 2
@@ -372,13 +388,17 @@ MODULE_DEVICE_TABLE(of, omap_rtc_of_match);static int __init omap_rtc_probe (struct platform_device *pdev) { +struct omap_rtc *rtc;struct resource *res; -struct rtc_device *rtc;u8 reg, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id;int ret; + rtc =devm_kzalloc (&pdev->dev,sizeof (*rtc),GFP_KERNEL ); +if (!rtc) +return -ENOMEM ; + of_id =of_match_device (omap_rtc_of_match, &pdev->dev);if (of_id) pdev->id_entry = of_id->data;
@@ -389,26 +409,30 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return -ENODEV ; } - omap_rtc_timer =platform_get_irq (pdev,0 ); -if (omap_rtc_timer <=0 ) + rtc->flags = id_entry->driver_data; + + rtc->irq_timer =platform_get_irq (pdev,0 ); +if (rtc->irq_timer <=0 )return -ENOENT ; - omap_rtc_alarm =platform_get_irq (pdev,1 ); -if (omap_rtc_alarm <=0 ) + rtc->irq_alarm =platform_get_irq (pdev,1 ); +if (rtc->irq_alarm <=0 )return -ENOENT ; res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); - rtc_base =devm_ioremap_resource (&pdev->dev, res); -if (IS_ERR (rtc_base)) -return PTR_ERR (rtc_base); + rtc->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (rtc->base)) +return PTR_ERR (rtc->base); + +platform_set_drvdata (pdev, rtc);/* Enable the clock/module so that we can access the registers */ pm_runtime_enable (&pdev->dev);pm_runtime_get_sync (&pdev->dev); -if (id_entry->driver_data &OMAP_RTC_HAS_KICKER ) { -rtc_writel (KICK0_VALUE ,OMAP_RTC_KICK0_REG ); -rtc_writel (KICK1_VALUE ,OMAP_RTC_KICK1_REG ); +if (rtc->flags &OMAP_RTC_HAS_KICKER ) { +rtc_writel (rtc,OMAP_RTC_KICK0_REG ,KICK0_VALUE ); +rtc_writel (rtc,OMAP_RTC_KICK1_REG ,KICK1_VALUE ); }/*
@@ -416,25 +440,26 @@ static int __init omap_rtc_probe(struct platform_device *pdev)* * NOTE: ALARM2 is not cleared on AM3352 if rtc_write (writeb) is used */ -rtc_writel (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG ,0 );/* enable RTC functional clock */ -if (id_entry->driver_data &OMAP_RTC_HAS_32KCLK_EN ) { - reg =rtc_read (OMAP_RTC_OSC_REG ); -rtc_writel (reg |OMAP_RTC_OSC_32KCLK_EN ,OMAP_RTC_OSC_REG ); +if (rtc->flags &OMAP_RTC_HAS_32KCLK_EN ) { + reg =rtc_read (rtc,OMAP_RTC_OSC_REG ); +rtc_writel (rtc,OMAP_RTC_OSC_REG , + reg |OMAP_RTC_OSC_32KCLK_EN ); }/* clear old status */ - reg =rtc_read (OMAP_RTC_STATUS_REG ); + reg =rtc_read (rtc,OMAP_RTC_STATUS_REG );if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) {dev_info (&pdev->dev,"RTC power up reset detected\n" ); -rtc_write (OMAP_RTC_STATUS_POWER_UP ,OMAP_RTC_STATUS_REG ); +rtc_write (rtc,OMAP_RTC_STATUS_REG ,OMAP_RTC_STATUS_POWER_UP ); }if (reg & (u8 )OMAP_RTC_STATUS_ALARM ) -rtc_write (OMAP_RTC_STATUS_ALARM ,OMAP_RTC_STATUS_REG ); +rtc_write (rtc,OMAP_RTC_STATUS_REG ,OMAP_RTC_STATUS_ALARM );/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ - reg =rtc_read (OMAP_RTC_CTRL_REG ); + reg =rtc_read (rtc,OMAP_RTC_CTRL_REG );if (reg & (u8 )OMAP_RTC_CTRL_STOP )dev_info (&pdev->dev,"already running\n" );
@@ -460,27 +485,26 @@ static int __init omap_rtc_probe(struct platform_device *pdev)dev_info (&pdev->dev,"split power mode\n" );if (reg != new_ctrl) -rtc_write (new_ctrl,OMAP_RTC_CTRL_REG ); +rtc_write (rtc,OMAP_RTC_CTRL_REG , new_ctrl);device_init_wakeup (&pdev->dev, true); - rtc =devm_rtc_device_register (&pdev->dev, pdev->name, + rtc->rtc =devm_rtc_device_register (&pdev->dev, pdev->name, &omap_rtc_ops,THIS_MODULE ); -if (IS_ERR (rtc)) { - ret =PTR_ERR (rtc); +if (IS_ERR (rtc->rtc)) { + ret =PTR_ERR (rtc->rtc); goto err; } -platform_set_drvdata (pdev, rtc);/* handle periodic and alarm irqs */ - ret =devm_request_irq (&pdev->dev, omap_rtc_timer, rtc_irq,0 , -dev_name (&rtc->dev), rtc); + ret =devm_request_irq (&pdev->dev, rtc->irq_timer, rtc_irq,0 , +dev_name (&rtc->rtc->dev), rtc);if (ret) goto err; -if (omap_rtc_timer != omap_rtc_alarm) { - ret =devm_request_irq (&pdev->dev, omap_rtc_alarm, rtc_irq,0 , -dev_name (&rtc->dev), rtc); +if (rtc->irq_timer != rtc->irq_alarm) { + ret =devm_request_irq (&pdev->dev, rtc->irq_alarm, rtc_irq,0 , +dev_name (&rtc->rtc->dev), rtc);if (ret) goto err; }
@@ -489,8 +513,8 @@ static int __init omap_rtc_probe(struct platform_device *pdev) err:device_init_wakeup (&pdev->dev, false); -if (id_entry->driver_data &OMAP_RTC_HAS_KICKER ) -rtc_writel (0 ,OMAP_RTC_KICK0_REG ); +if (rtc->flags &OMAP_RTC_HAS_KICKER ) +rtc_writel (rtc,OMAP_RTC_KICK0_REG ,0 );pm_runtime_put_sync (&pdev->dev);pm_runtime_disable (&pdev->dev);
@@ -499,16 +523,15 @@ err: static int __exit omap_rtc_remove(struct platform_device *pdev) { -const struct platform_device_id *id_entry = -platform_get_device_id (pdev); +struct omap_rtc *rtc =platform_get_drvdata (pdev);device_init_wakeup (&pdev->dev,0 );/* leave rtc running, but disable irqs */ -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 ); -if (id_entry->driver_data &OMAP_RTC_HAS_KICKER ) -rtc_writel (0 ,OMAP_RTC_KICK0_REG ); +if (rtc->flags &OMAP_RTC_HAS_KICKER ) +rtc_writel (rtc,OMAP_RTC_KICK0_REG ,0 );/* Disable the clock/module */ pm_runtime_put_sync (&pdev->dev);
@@ -518,20 +541,20 @@ static int __exit omap_rtc_remove(struct platform_device *pdev) }#ifdef CONFIG_PM_SLEEP -static u8 irqstat; -static int omap_rtc_suspend (struct device *dev) { - irqstat =rtc_read (OMAP_RTC_INTERRUPTS_REG ); +struct omap_rtc *rtc =dev_get_drvdata (dev); + + rtc->interrupts_reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );/* FIXME the RTC alarm is not currently acting as a wakeup event * source on some platforms, and in fact this enable() call is just * saving a flag that's never used... */ if (device_may_wakeup (dev)) -enable_irq_wake (omap_rtc_alarm); +enable_irq_wake (rtc->irq_alarm);else -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 );/* Disable the clock/module */ pm_runtime_put_sync (dev);
@@ -541,13 +564,15 @@ static int omap_rtc_suspend(struct device *dev)static int omap_rtc_resume (struct device *dev) { +struct omap_rtc *rtc =dev_get_drvdata (dev); +/* Enable the clock/module so that we can access the registers */ pm_runtime_get_sync (dev);if (device_may_wakeup (dev)) -disable_irq_wake (omap_rtc_alarm); +disable_irq_wake (rtc->irq_alarm);else -rtc_write (irqstat,OMAP_RTC_INTERRUPTS_REG ); +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , rtc->interrupts_reg);return 0 ; }
@@ -557,7 +582,9 @@ static SIMPLE_DEV_PM_OPS(omap_rtc_pm_ops, omap_rtc_suspend, omap_rtc_resume);static void omap_rtc_shutdown (struct platform_device *pdev) { -rtc_write (0 ,OMAP_RTC_INTERRUPTS_REG ); +struct omap_rtc *rtc =platform_get_drvdata (pdev); + +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 ); }MODULE_ALIAS ("platform:omap_rtc" );
Signature · 1 line
2.0.4
PATCH v2 10/20 rtc: omap: remove DRIVER_NAME macro
In reply to Johan HovoldRemove DRIVER_NAME macro which was used for unrelated strings (e.g. id-table entry and module name), but not for related ones (e.g. module name and alias).
Also move the module alias to the other module-info entries.
--- drivers/rtc/rtc-omap.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/rtc/rtc-omap.c +3 -5
@@ -39,8 +39,6 @@* the SoC). See the BOARD-SPECIFIC CUSTOMIZATION comment. */ -#define DRIVER_NAME "omap_rtc" -/* RTC registers */ #define OMAP_RTC_SECONDS_REG 0x00#define OMAP_RTC_MINUTES_REG 0x04
@@ -360,7 +358,7 @@ static struct rtc_class_ops omap_rtc_ops = {static const struct platform_device_id omap_rtc_devtype[] = { { - .name =DRIVER_NAME , + .name ="omap_rtc" , }, [OMAP_RTC_DATA_AM3352_IDX ] = { .name ="am3352-rtc" ,
@@ -587,12 +585,11 @@ static void omap_rtc_shutdown(struct platform_device *pdev)rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 ); } -MODULE_ALIAS ("platform:omap_rtc" );static struct platform_driver omap_rtc_driver = { .remove =__exit_p (omap_rtc_remove), .shutdown = omap_rtc_shutdown, .driver = { - .name =DRIVER_NAME , + .name ="omap_rtc" , .owner =THIS_MODULE , .pm = &omap_rtc_pm_ops, .of_match_table = omap_rtc_of_match,
@@ -602,5 +599,6 @@ static struct platform_driver omap_rtc_driver = {module_platform_driver_probe (omap_rtc_driver, omap_rtc_probe); +MODULE_ALIAS ("platform:omap_rtc" );MODULE_AUTHOR ("George G. Davis (and others)" );MODULE_LICENSE ("GPL" );
Signature · 1 line
2.0.4
PATCH v2 11/20 rtc: omap: add structured device-type info
In reply to Johan HovoldAdd structured device-type info to encode IP-block revision differences.
--- drivers/rtc/rtc-omap.c | 103 ++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 52 deletions(-)
drivers/rtc/rtc-omap.c +51 -52
@@ -102,19 +102,11 @@#define KICK0_VALUE 0x83e70b13#define KICK1_VALUE 0x95a4f1e0 -#define OMAP_RTC_HAS_KICKER BIT(0) - -/* -* Few RTC IP revisions has special WAKE-EN Register to enable Wakeup -* generation for event Alarm. -*/ -#define OMAP_RTC_HAS_IRQWAKEEN BIT(1) - -/* -* Some RTC IP revisions (like those in AM335x and DRA7x) need -* the 32KHz clock to be explicitly enabled. -*/ -#define OMAP_RTC_HAS_32KCLK_EN BIT(2) +struct omap_rtc_device_type { +bool has_32kclk_en; +bool has_kicker; +bool has_irqwakeen; +};struct omap_rtc {struct rtc_device *rtc;
@@ -122,7 +114,7 @@ struct omap_rtc {int irq_alarm;int irq_timer;u8 interrupts_reg; -unsigned long flags; +const struct omap_rtc_device_type *type; };static inline u8 rtc_read (struct omap_rtc *rtc,unsigned int reg)
@@ -190,7 +182,7 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)local_irq_disable ();rtc_wait_not_busy (rtc); reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); -if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +if (rtc->type->has_irqwakeen) irqwake_reg =rtc_read (rtc,OMAP_RTC_IRQWAKEEN );if (enabled) {
@@ -202,7 +194,7 @@ static int omap_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) }rtc_wait_not_busy (rtc);rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , reg); -if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +if (rtc->type->has_irqwakeen)rtc_write (rtc,OMAP_RTC_IRQWAKEEN , irqwake_reg);local_irq_enable ();
@@ -326,7 +318,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)rtc_write (rtc,OMAP_RTC_ALARM_SECONDS_REG , alm->time.tm_sec); reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); -if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +if (rtc->type->has_irqwakeen) irqwake_reg =rtc_read (rtc,OMAP_RTC_IRQWAKEEN );if (alm->enabled) {
@@ -337,7 +329,7 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) irqwake_reg &= ~OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN ; }rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , reg); -if (rtc->flags &OMAP_RTC_HAS_IRQWAKEEN ) +if (rtc->type->has_irqwakeen)rtc_write (rtc,OMAP_RTC_IRQWAKEEN , irqwake_reg);local_irq_enable ();
@@ -353,34 +345,45 @@ static struct rtc_class_ops omap_rtc_ops = { .alarm_irq_enable = omap_rtc_alarm_irq_enable, }; -#define OMAP_RTC_DATA_AM3352_IDX 1 -#define OMAP_RTC_DATA_DA830_IDX 2 +static const struct omap_rtc_device_type omap_rtc_default_type = { +}; + +static const struct omap_rtc_device_type omap_rtc_am3352_type = { + .has_32kclk_en = true, + .has_kicker = true, + .has_irqwakeen = true, +}; + +static const struct omap_rtc_device_type omap_rtc_da830_type = { + .has_kicker = true, +}; -static const struct platform_device_id omap_rtc_devtype[] = { +static const struct platform_device_id omap_rtc_id_table[] = { { .name ="omap_rtc" , - }, - [OMAP_RTC_DATA_AM3352_IDX ] = { + .driver_data = (kernel_ulong_t)&omap_rtc_default_type, + }, { .name ="am3352-rtc" , - .driver_data =OMAP_RTC_HAS_KICKER |OMAP_RTC_HAS_IRQWAKEEN | -OMAP_RTC_HAS_32KCLK_EN , - }, - [OMAP_RTC_DATA_DA830_IDX ] = { + .driver_data = (kernel_ulong_t)&omap_rtc_am3352_type, + }, { .name ="da830-rtc" , - .driver_data =OMAP_RTC_HAS_KICKER , - }, - {}, + .driver_data = (kernel_ulong_t)&omap_rtc_da830_type, + }, { +/* sentinel */ + } }; -MODULE_DEVICE_TABLE (platform, omap_rtc_devtype); +MODULE_DEVICE_TABLE (platform, omap_rtc_id_table);static const struct of_device_id omap_rtc_of_match[] = { - { .compatible ="ti,da830-rtc" , - .data = &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX ], - }, - { .compatible ="ti,am3352-rtc" , - .data = &omap_rtc_devtype[OMAP_RTC_DATA_AM3352_IDX ], - }, - {}, + { + .compatible ="ti,am3352-rtc" , + .data = &omap_rtc_am3352_type, + }, { + .compatible ="ti,da830-rtc" , + .data = &omap_rtc_da830_type, + }, { +/* sentinel */ + } };MODULE_DEVICE_TABLE (of, omap_rtc_of_match);
@@ -398,17 +401,13 @@ static int __init omap_rtc_probe(struct platform_device *pdev)return -ENOMEM ; of_id =of_match_device (omap_rtc_of_match, &pdev->dev); -if (of_id) - pdev->id_entry = of_id->data; - - id_entry =platform_get_device_id (pdev); -if (!id_entry) { -dev_err (&pdev->dev,"no matching device entry\n" ); -return -ENODEV ; +if (of_id) { + rtc->type = of_id->data; + }else { + id_entry =platform_get_device_id (pdev); + rtc->type = (void *)id_entry->driver_data; } - rtc->flags = id_entry->driver_data; - rtc->irq_timer =platform_get_irq (pdev,0 );if (rtc->irq_timer <=0 )return -ENOENT ;
@@ -428,7 +427,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)pm_runtime_enable (&pdev->dev);pm_runtime_get_sync (&pdev->dev); -if (rtc->flags &OMAP_RTC_HAS_KICKER ) { +if (rtc->type->has_kicker) {rtc_writel (rtc,OMAP_RTC_KICK0_REG ,KICK0_VALUE );rtc_writel (rtc,OMAP_RTC_KICK1_REG ,KICK1_VALUE ); }
@@ -441,7 +440,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG ,0 );/* enable RTC functional clock */ -if (rtc->flags &OMAP_RTC_HAS_32KCLK_EN ) { +if (rtc->type->has_32kclk_en) { reg =rtc_read (rtc,OMAP_RTC_OSC_REG );rtc_writel (rtc,OMAP_RTC_OSC_REG , reg |OMAP_RTC_OSC_32KCLK_EN );
@@ -511,7 +510,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev) err:device_init_wakeup (&pdev->dev, false); -if (rtc->flags &OMAP_RTC_HAS_KICKER ) +if (rtc->type->has_kicker)rtc_writel (rtc,OMAP_RTC_KICK0_REG ,0 );pm_runtime_put_sync (&pdev->dev);pm_runtime_disable (&pdev->dev);
@@ -528,7 +527,7 @@ static int __exit omap_rtc_remove(struct platform_device *pdev)/* leave rtc running, but disable irqs */ rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 ); -if (rtc->flags &OMAP_RTC_HAS_KICKER ) +if (rtc->type->has_kicker)rtc_writel (rtc,OMAP_RTC_KICK0_REG ,0 );/* Disable the clock/module */
@@ -594,7 +593,7 @@ static struct platform_driver omap_rtc_driver = { .pm = &omap_rtc_pm_ops, .of_match_table = omap_rtc_of_match, }, - .id_table = omap_rtc_devtype, + .id_table = omap_rtc_id_table, };module_platform_driver_probe (omap_rtc_driver, omap_rtc_probe);
Signature · 1 line
2.0.4
PATCH v2 12/20 rtc: omap: silence bogus power-up reset message at probe
In reply to Johan HovoldSome legacy RTC IP revisions has a power-up reset flag in the status register that later revisions lack.
As this flag is always read back as set on later revisions (or is overloaded with a different flag), make sure to only clear the flag and print the info message on legacy platforms.
--- drivers/rtc/rtc-omap.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/rtc/rtc-omap.c +13 -6
@@ -106,6 +106,7 @@ struct omap_rtc_device_type {bool has_32kclk_en;bool has_kicker;bool has_irqwakeen; +bool has_power_up_reset; };struct omap_rtc {
@@ -346,6 +347,7 @@ static struct rtc_class_ops omap_rtc_ops = { };static const struct omap_rtc_device_type omap_rtc_default_type = { + .has_power_up_reset = true, };static const struct omap_rtc_device_type omap_rtc_am3352_type = {
@@ -391,7 +393,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev) {struct omap_rtc *rtc;struct resource *res; -u8 reg, new_ctrl; +u8 reg, mask, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id;int ret;
@@ -448,12 +450,17 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* clear old status */ reg =rtc_read (rtc,OMAP_RTC_STATUS_REG ); -if (reg & (u8 )OMAP_RTC_STATUS_POWER_UP ) { -dev_info (&pdev->dev,"RTC power up reset detected\n" ); -rtc_write (rtc,OMAP_RTC_STATUS_REG ,OMAP_RTC_STATUS_POWER_UP ); + + mask =OMAP_RTC_STATUS_ALARM ; + +if (rtc->type->has_power_up_reset) { + mask |=OMAP_RTC_STATUS_POWER_UP ; +if (reg &OMAP_RTC_STATUS_POWER_UP ) +dev_info (&pdev->dev,"RTC power up reset detected\n" ); } -if (reg & (u8 )OMAP_RTC_STATUS_ALARM ) -rtc_write (rtc,OMAP_RTC_STATUS_REG ,OMAP_RTC_STATUS_ALARM ); + +if (reg & mask) +rtc_write (rtc,OMAP_RTC_STATUS_REG , reg & mask);/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (rtc,OMAP_RTC_CTRL_REG );
Signature · 1 line
2.0.4
PATCH v2 13/20 rtc: omap: add helper to read raw bcd time
In reply to Johan HovoldAdd helper to read raw BCD time that can be used in interrupt context.
--- drivers/rtc/rtc-omap.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/rtc/rtc-omap.c +10 -8
@@ -234,22 +234,24 @@ static void bcd2tm(struct rtc_time *tm) tm->tm_year =bcd2bin (tm->tm_year) +100 ; } - -static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) +static void omap_rtc_read_time_raw (struct omap_rtc *rtc,struct rtc_time *tm) { -struct omap_rtc *rtc =dev_get_drvdata (dev); - -/* we don't report wday/yday/isdst ... */ -local_irq_disable (); -rtc_wait_not_busy (rtc); - tm->tm_sec =rtc_read (rtc,OMAP_RTC_SECONDS_REG ); tm->tm_min =rtc_read (rtc,OMAP_RTC_MINUTES_REG ); tm->tm_hour =rtc_read (rtc,OMAP_RTC_HOURS_REG ); tm->tm_mday =rtc_read (rtc,OMAP_RTC_DAYS_REG ); tm->tm_mon =rtc_read (rtc,OMAP_RTC_MONTHS_REG ); tm->tm_year =rtc_read (rtc,OMAP_RTC_YEARS_REG ); +} + +static int omap_rtc_read_time (struct device *dev,struct rtc_time *tm) +{ +struct omap_rtc *rtc =dev_get_drvdata (dev); +/* we don't report wday/yday/isdst ... */ +local_irq_disable (); +rtc_wait_not_busy (rtc); +omap_rtc_read_time_raw (rtc, tm);local_irq_enable ();bcd2tm (tm);
Signature · 1 line
2.0.4
PATCH v2 14/20 rtc: omap: add helper to read 32-bit registers
In reply to Johan HovoldAdd helper to read full register width.
--- drivers/rtc/rtc-omap.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/rtc/rtc-omap.c +5 -0
@@ -123,6 +123,11 @@ static inline u8 rtc_read(struct omap_rtc *rtc, unsigned int reg)return readb (rtc->base + reg); } +static inline u32 rtc_readl (struct omap_rtc *rtc,unsigned int reg) +{ +return readl (rtc->base + reg); +} +static inline void rtc_write (struct omap_rtc *rtc,unsigned int reg,u8 val) {writeb (val, rtc->base + reg);
Signature · 1 line
2.0.4
PATCH v2 15/20 rtc: omap: add support for pmic_power_en
In reply to Johan HovoldAdd new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the pmic_power_en pin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.
This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
--- Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 ++- drivers/rtc/rtc-omap.c | 93 ++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/rtc/rtc-omap.txt +8 -1
@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller +Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system power + through pmic_power_en + Example: rtc at 1c23000 {
@@ -18,4 +24,5 @@ rtc at 1c23000 { interrupts = <19 19>; interrupt-parent = <&intc>; + ti,system-power-controller; };
drivers/rtc/rtc-omap.c +93 -0
165 lines not on this page · raw message
Signature · 1 line
2.0.4
PATCH v2 16/20 rtc: omap: enable wake-up from power off
In reply to Johan HovoldThe ALARM interrupt must not be disabled during shutdown in order to be able to power up the system using an RTC alarm.
--- drivers/rtc/rtc-omap.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/rtc/rtc-omap.c +8 -1
@@ -687,8 +687,15 @@ static SIMPLE_DEV_PM_OPS(omap_rtc_pm_ops, omap_rtc_suspend, omap_rtc_resume);static void omap_rtc_shutdown (struct platform_device *pdev) {struct omap_rtc *rtc =platform_get_drvdata (pdev); +u8 mask; -rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG ,0 ); +/* +* Keep the ALARM interrupt enabled to allow the system to power up on +* alarm events. +*/ + mask =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); + mask &=OMAP_RTC_INTERRUPTS_IT_ALARM ; +rtc_write (rtc,OMAP_RTC_INTERRUPTS_REG , mask); }static struct platform_driver omap_rtc_driver = {
Signature · 1 line
2.0.4
PATCH v2 17/20 rtc: omap: fix minor coding style issues
In reply to Johan HovoldFix minor coding style issues like comment style, indentation and remove a few unnecessary casts.
Also drop the 1 from OMAP1 in the driver description.
--- drivers/rtc/rtc-omap.c | 46 +++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 19 deletions(-)
drivers/rtc/rtc-omap.c +27 -19
@@ -1,5 +1,5 @@/* -* TI OMAP1 Real Time Clock interface for Linux +* TI OMAP Real Time Clock interface for Linux * * Copyright (C) 2003 MontaVista Software, Inc. * Author: George G. Davis <[email protected]> or <[email protected]>
@@ -25,7 +25,8 @@#include <linux/pm_runtime.h> #include <linux/io.h> -/* The OMAP1 RTC is a year/month/day/hours/minutes/seconds BCD clock +/* +* The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock * with century-range alarm matching, driven by the 32kHz clock. * * The main user-visible ways it differs from PC RTCs are by omitting
@@ -154,19 +155,20 @@ static inline void rtc_writel(struct omap_rtc *rtc, unsigned int reg, u32 val)writel (val, rtc->base + reg); } -/* we rely on the rtc framework to handle locking (rtc->ops_lock), +/* +* We rely on the rtc framework to handle locking (rtc->ops_lock), * so the only other requirement is that register accesses which * require BUSY to be clear are made with IRQs locally disabled */ static void rtc_wait_not_busy (struct omap_rtc *rtc) { -int count =0 ; -u8 status; +int count; +u8 status;/* BUSY may stay active for 1/32768 second (~30 usec) */ for (count =0 ; count <50 ; count++) { status =rtc_read (rtc,OMAP_RTC_STATUS_REG ); -if ((status & (u8 )OMAP_RTC_STATUS_BUSY ) ==0 ) +if (!(status &OMAP_RTC_STATUS_BUSY ))break ;udelay (1 ); }
@@ -175,9 +177,9 @@ static void rtc_wait_not_busy(struct omap_rtc *rtc)static irqreturn_t rtc_irq (int irq,void *dev_id) { -struct omap_rtc *rtc = dev_id; -unsigned long events =0 ; -u8 irq_data; +struct omap_rtc *rtc = dev_id; +unsigned long events =0 ; +u8 irq_data; irq_data =rtc_read (rtc,OMAP_RTC_STATUS_REG );
@@ -276,6 +278,7 @@ static int omap_rtc_read_time(struct device *dev, struct rtc_time *tm)local_irq_enable ();bcd2tm (tm); +return 0 ; }
@@ -285,6 +288,7 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)if (tm2bcd (tm) <0 )return -EINVAL ; +local_irq_disable ();rtc_wait_not_busy (rtc);
@@ -303,6 +307,7 @@ static int omap_rtc_set_time(struct device *dev, struct rtc_time *tm)static int omap_rtc_read_alarm (struct device *dev,struct rtc_wkalrm *alm) {struct omap_rtc *rtc =dev_get_drvdata (dev); +u8 interrupts;local_irq_disable ();rtc_wait_not_busy (rtc);
@@ -317,8 +322,9 @@ static int omap_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)local_irq_enable ();bcd2tm (&alm->time); - alm->enabled = !!(rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ) - &OMAP_RTC_INTERRUPTS_IT_ALARM ); + + interrupts =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); + alm->enabled = !!(interrupts &OMAP_RTC_INTERRUPTS_IT_ALARM );return 0 ; }
@@ -472,9 +478,9 @@ MODULE_DEVICE_TABLE(of, omap_rtc_of_match);static int __init omap_rtc_probe (struct platform_device *pdev) { -struct omap_rtc *rtc; -struct resource *res; -u8 reg, mask, new_ctrl; +struct omap_rtc *rtc; +struct resource *res; +u8 reg, mask, new_ctrl;const struct platform_device_id *id_entry;const struct of_device_id *of_id;int ret;
@@ -551,14 +557,15 @@ static int __init omap_rtc_probe(struct platform_device *pdev)/* On boards with split power, RTC_ON_NOFF won't reset the RTC */ reg =rtc_read (rtc,OMAP_RTC_CTRL_REG ); -if (reg & (u8 )OMAP_RTC_CTRL_STOP ) +if (reg &OMAP_RTC_CTRL_STOP )dev_info (&pdev->dev,"already running\n" );/* force to 24 hour mode */ - new_ctrl = reg & (OMAP_RTC_CTRL_SPLIT |OMAP_RTC_CTRL_AUTO_COMP ); + new_ctrl = reg & (OMAP_RTC_CTRL_SPLIT |OMAP_RTC_CTRL_AUTO_COMP ); new_ctrl |=OMAP_RTC_CTRL_STOP ; -/* BOARD-SPECIFIC CUSTOMIZATION CAN GO HERE: +/* +* BOARD-SPECIFIC CUSTOMIZATION CAN GO HERE: * * - Device wake-up capability setting should come through chip * init logic. OMAP1 boards should initialize the "wakeup capable"
@@ -572,7 +579,7 @@ static int __init omap_rtc_probe(struct platform_device *pdev)* is write-only, and always reads as zero...) */ -if (new_ctrl & (u8 )OMAP_RTC_CTRL_SPLIT ) +if (new_ctrl &OMAP_RTC_CTRL_SPLIT )dev_info (&pdev->dev,"split power mode\n" );if (reg != new_ctrl)
@@ -651,7 +658,8 @@ static int omap_rtc_suspend(struct device *dev) rtc->interrupts_reg =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG ); -/* FIXME the RTC alarm is not currently acting as a wakeup event +/* +* FIXME: the RTC alarm is not currently acting as a wakeup event * source on some platforms, and in fact this enable() call is just * saving a flag that's never used... */
Signature · 1 line
2.0.4
PATCH v2 18/20 rtc: omap: add copyright entry
In reply to Johan HovoldAdd myself to the list of copyright holders.
--- drivers/rtc/rtc-omap.c | 1 + 1 file changed, 1 insertion(+)
drivers/rtc/rtc-omap.c +1 -0
@@ -5,6 +5,7 @@* Author: George G. Davis <[email protected]> or <[email protected]> * * Copyright (C) 2006 David Brownell (new RTC framework) +* Copyright (C) 2014 Johan Hovold <[email protected]> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License
Signature · 1 line
2.0.4
PATCH v2 19/20 ARM: dts: am33xx: update rtc-node compatible property
In reply to Johan HovoldEnable am33xx specific RTC features (e.g. PMIC control) by adding "ti,am3352-rtc" to the compatible property of the rtc node.
--- arch/arm/boot/dts/am33xx.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/boot/dts/am33xx.dtsi +1 -1
@@ -411,7 +411,7 @@ }; rtc: rtc at 44e3e000 { - compatible = "ti,da830-rtc"; + compatible = "ti,am3352-rtc", "ti,da830-rtc"; reg = <0x44e3e000 0x1000>; interrupts = <75 76>;
Signature · 1 line
2.0.4
PATCH v2 20/20 ARM: dts: am335x-boneblack: enable power off and rtc wake up
In reply to Johan HovoldConfigure the RTC as system-power controller, which allows the system to be powered off as well as woken up again on subsequent RTC alarms.
Note that the PMIC needs to be put in SLEEP (rather than OFF) mode to maintain RTC power. Specifically, this means that the PMIC ti,pmic-shutdown-controller property must be left unset in order to be able to wake up on RTC alarms.
Tested on BeagleBone Black (rev A5).
--- arch/arm/boot/dts/am335x-boneblack.dts | 4 ++++ 1 file changed, 4 insertions(+)
arch/arm/boot/dts/am335x-boneblack.dts +4 -0
@@ -75,3 +75,7 @@ status = "okay"; }; }; + +&rtc { + ti,system-power-controller; +};
Signature · 1 line
2.0.4
Hi,
Quoted 29 lines · Johan Hovold
This series fixes a few issues with the omap rtc-driver, cleans up a bit, adds device abstraction, and finally adds support for the PMIC control feature found in some revisions of this RTC IP block.
Ultimately, this allows for powering off the Beaglebone and waking it up again on RTC alarms.
Changes since v1 include:
- add device abstraction
- add structured device-type info
- fix interrupt disable at probe separately
- register interrupts after class device as before
- add helper to read raw bcd time
- more clean ups
- add copyright entry
- enable pmic control only for Beaglebone BlackI realised that it was better to keep this patch series self-contained and include the device abstraction in it, rather than rely on some patches yet to be posted (contrary to what I proposed in v1). It should now be straight forward to add deferred-probing and regulator support on top of this series instead. [1]
I've kept Felipe's Reviewed-by tags only on the first and last couple of patches which have not changed (in any significant way).
For the record, the new functionality is easily tested on BBB with:
Quoted 8 lines
echo +30 >/sys/class/rtc/rtc0/wakealarm; poweroff
There are some patches floating around to add power-off handler call chains [2] as well as an RFC for a generic "poweroff-source" property [3]. Since it is unclear if, when (and through which tree) this will eventually go in, I suggest merging this series as-is now and do the trivial updates to use these new interfaces once the infrastructure is in place.
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
Then again, this also happens by simply calling poweroff without enabling wakealarm.
In any case, for the whole series:
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/0e8c1969/attachment.sig>
[ +CC: Russell ]
Quoted 14 lines · Felipe Balbi
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
Quoted 2 lines
Then again, this also happens by simply calling poweroff without enabling wakealarm.
Bah, I forgot to look into that. I haven't seen this myself as I don't use systemd (which does the syscall from process 0).
Some driver power-off implementations and some arch machine_power_off spin indefinitely (or use an mdelay and WARN) after trying to power off.
I think this is really a bug in arch/arm that should be fixed analogously to how failed reboot is handled in machine_restart().
Care to try the patch below?
I should still add a two-second delay to rtc-omap to avoid the arch error message.
Andrew, can you update one patch in the series or should I just resend them all (with proper Tested-by tags)?
Quoted 3 lines
In any case, for the whole series:
Tested-by Felipe Balbi <[email protected]>
Thanks for testing!
Johan
Quoted 1 line
From
aaa1d1d6171c895b6966ba5b738ac7946ada97c7Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Fri, 24 Oct 2014 18:53:09 +0200
Subject: [PATCH] ARM: fix failed power-off handling
Make sure to handle failed power off by printing an error message and halting (analogously to how failed reboot is handled).
Power off can fail for example if the hardware has not been wired up correctly.
This avoids a kernel panic when called from process 0:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
--- arch/arm/kernel/process.c | 6 ++++++ 1 file changed, 6 insertions(+)
arch/arm/kernel/process.c +6 -0
@@ -212,6 +212,12 @@ void machine_power_off(void)if (pm_power_off)pm_power_off (); + +/* Give a grace period for failure to power off */ +mdelay (1000 ); + +pr_err ("Power off failed -- system halted\n" ); +while (1 ); }/*
Signature · 1 line
2.0.4
Quoted 81 lines · Johan Hovold
[ +CC: Russell ]
Quoted 14 lines · Felipe Balbi
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Quoted 2 lines
Then again, this also happens by simply calling poweroff without enabling wakealarm.
Bah, I forgot to look into that. I haven't seen this myself as I don't use systemd (which does the syscall from process 0).
Some driver power-off implementations and some arch
machine_power_offspin indefinitely (or use an mdelay and WARN) after trying to power off.I think this is really a bug in
arch/armthat should be fixed analogously to how failed reboot is handled inmachine_restart().Care to try the patch below?
I should still add a two-second delay to rtc-omap to avoid the arch error message.
Andrew, can you update one patch in the series or should I just resend them all (with proper Tested-by tags)?
Quoted 3 lines
In any case, for the whole series:
Tested-by Felipe Balbi <[email protected]>Thanks for testing!
Johan
Quoted 1 line
From
aaa1d1d6171c895b6966ba5b738ac7946ada97c7Mon Sep 17 00:00:00 2001From: Johan Hovold <[email protected]>
Date: Fri, 24 Oct 2014 18:53:09 +0200
Subject: [PATCH] ARM: fix failed power-off handlingMake sure to handle failed power off by printing an error message and halting (analogously to how failed reboot is handled).
Power off can fail for example if the hardware has not been wired up correctly.
This avoids a kernel panic when called from process 0:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Signed-off-by Johan Hovold <[email protected]>--- arch/arm/kernel/process.c | 6 ++++++ 1 file changed, 6 insertions(+)arch/arm/kernel/process.c +0
-0
arch/arm/kernel/process.c void machine_power_off(void) · line 221
Quoted 10 lines
arch/arm/kernel/process.c +6
-0@@ -212,6 +212,12 @@ void machine_power_off(void)if (pm_power_off)pm_power_off (); + +/* Give a grace period for failure to power off */ +mdelay (1000 ); + +pr_err ("Power off failed -- system halted\n" ); +while (1 ); }
with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/aef25905/attachment-0001.sig>
Hi,
Quoted 83 lines · Felipe Balbi
Quoted 82 lines · Johan Hovold
[ +CC: Russell ]
Quoted 14 lines · Felipe Balbi
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Quoted 2 lines
Then again, this also happens by simply calling poweroff without enabling wakealarm.
Bah, I forgot to look into that. I haven't seen this myself as I don't use systemd (which does the syscall from process 0).
Some driver power-off implementations and some arch
machine_power_offspin indefinitely (or use an mdelay and WARN) after trying to power off.I think this is really a bug in
arch/armthat should be fixed analogously to how failed reboot is handled inmachine_restart().Care to try the patch below?
I should still add a two-second delay to rtc-omap to avoid the arch error message.
Andrew, can you update one patch in the series or should I just resend them all (with proper Tested-by tags)?
Quoted 3 lines
In any case, for the whole series:
Tested-by Felipe Balbi <[email protected]>Thanks for testing!
Johan
Quoted 1 line
From
aaa1d1d6171c895b6966ba5b738ac7946ada97c7Mon Sep 17 00:00:00 2001From: Johan Hovold <[email protected]>
Date: Fri, 24 Oct 2014 18:53:09 +0200
Subject: [PATCH] ARM: fix failed power-off handlingMake sure to handle failed power off by printing an error message and halting (analogously to how failed reboot is handled).
Power off can fail for example if the hardware has not been wired up correctly.
This avoids a kernel panic when called from process 0:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Signed-off-by Johan Hovold <[email protected]>--- arch/arm/kernel/process.c | 6 ++++++ 1 file changed, 6 insertions(+)arch/arm/kernel/process.c +0
-0@@ -212,6 +212,12 @@ void machine_power_off(void)
Quoted 13 lines
arch/arm/kernel/process.c void machine_power_off(void) · line 221
Quoted 9 lines
arch/arm/kernel/process.c +6
-0if (pm_power_off)pm_power_off (); + +/* Give a grace period for failure to power off */ +mdelay (1000 ); + +pr_err ("Power off failed -- system halted\n" ); +while (1 ); }with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that mdelay() to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay to machine_power_off(). Should it be generic, or should we add it to our rtc pm_power_off implementation ?
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/72bc6b29/attachment.sig>
Quoted 99 lines · Felipe Balbi
Hi,
Quoted 96 lines · Felipe Balbi
Quoted 81 lines · Johan Hovold
[ +CC: Russell ]
Quoted 14 lines · Felipe Balbi
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Quoted 2 lines
Then again, this also happens by simply calling poweroff without enabling wakealarm.
Bah, I forgot to look into that. I haven't seen this myself as I don't use systemd (which does the syscall from process 0).
Some driver power-off implementations and some arch
machine_power_offspin indefinitely (or use an mdelay and WARN) after trying to power off.I think this is really a bug in
arch/armthat should be fixed analogously to how failed reboot is handled inmachine_restart().Care to try the patch below?
I should still add a two-second delay to rtc-omap to avoid the arch error message.
Andrew, can you update one patch in the series or should I just resend them all (with proper Tested-by tags)?
Quoted 3 lines
In any case, for the whole series:
Tested-by Felipe Balbi <[email protected]>Thanks for testing!
Johan
Quoted 1 line
From
aaa1d1d6171c895b6966ba5b738ac7946ada97c7Mon Sep 17 00:00:00 2001From: Johan Hovold <[email protected]>
Date: Fri, 24 Oct 2014 18:53:09 +0200
Subject: [PATCH] ARM: fix failed power-off handlingMake sure to handle failed power off by printing an error message and halting (analogously to how failed reboot is handled).
Power off can fail for example if the hardware has not been wired up correctly.
This avoids a kernel panic when called from process 0:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Signed-off-by Johan Hovold <[email protected]>--- arch/arm/kernel/process.c | 6 ++++++ 1 file changed, 6 insertions(+)arch/arm/kernel/process.c +0
-0arch/arm/kernel/process.c void machine_power_off(void) · line 221
Quoted 10 lines
arch/arm/kernel/process.c +6
-0@@ -212,6 +212,12 @@ void machine_power_off(void)if (pm_power_off)pm_power_off (); + +/* Give a grace period for failure to power off */ +mdelay (1000 ); + +pr_err ("Power off failed -- system halted\n" ); +while (1 ); }with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
That's expected (see below). It works with v3.18-rc1 vanilla because machine_halt is called instead of machine_power_off as there is no registered power-off handler.
Quoted 5 lines
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that
mdelay()to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay tomachine_power_off(). Should it be generic, or should we add it to our rtcpm_power_offimplementation ?
As I wrote above, we still need a 2-second mdelay in rtc-omap, which I intend to add to the pmic_power_en patch.
The one-second delay is there in machine_power_off to catch most cases which wouldn't take nearly as long as rtc-omap and hence wouldn't be adding an explicit delay in the driver power-off handler.
Johan
Quoted 101 lines · Johan Hovold
Quoted 99 lines · Felipe Balbi
Hi,
Quoted 96 lines · Felipe Balbi
Quoted 81 lines · Johan Hovold
[ +CC: Russell ]
Quoted 14 lines · Felipe Balbi
I tested this entire series with my BBB and it still works fine. However I still get below panic. This time without any DRM errors:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Quoted 2 lines
Then again, this also happens by simply calling poweroff without enabling wakealarm.
Bah, I forgot to look into that. I haven't seen this myself as I don't use systemd (which does the syscall from process 0).
Some driver power-off implementations and some arch
machine_power_offspin indefinitely (or use an mdelay and WARN) after trying to power off.I think this is really a bug in
arch/armthat should be fixed analogously to how failed reboot is handled inmachine_restart().Care to try the patch below?
I should still add a two-second delay to rtc-omap to avoid the arch error message.
Andrew, can you update one patch in the series or should I just resend them all (with proper Tested-by tags)?
Quoted 3 lines
In any case, for the whole series:
Tested-by Felipe Balbi <[email protected]>Thanks for testing!
Johan
Quoted 1 line
From
aaa1d1d6171c895b6966ba5b738ac7946ada97c7Mon Sep 17 00:00:00 2001From: Johan Hovold <[email protected]>
Date: Fri, 24 Oct 2014 18:53:09 +0200
Subject: [PATCH] ARM: fix failed power-off handlingMake sure to handle failed power off by printing an error message and halting (analogously to how failed reboot is handled).
Power off can fail for example if the hardware has not been wired up correctly.
This avoids a kernel panic when called from process 0:
[ 63.087832] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000 [ 63.087832] [ 63.097399] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 3.18.0-rc1-00095-g8524e69 #556 [ 63.106060] [<c00175a4>] (unwind_backtrace) from [<c00132f0>] (show_stack+0x20/0x24) [ 63.114160] [<c00132f0>] (show_stack) from [<c0657404>] (dump_stack+0x8c/0xa4) [ 63.121706] [<c0657404>] (dump_stack) from [<c0654f70>] (panic+0xa0/0x220) [ 63.128895] [<c0654f70>] (panic) from [<c0049e64>] (do_exit+0x974/0x9d0) [ 63.135900] [<c0049e64>] (do_exit) from [<c006775c>] (SyS_reboot+0x14c/0x1e8) [ 63.143361] [<c006775c>] (SyS_reboot) from [<c000f080>] (ret_fast_syscall+0x0/0x48) [ 63.151596] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000Signed-off-by Johan Hovold <[email protected]>--- arch/arm/kernel/process.c | 6 ++++++ 1 file changed, 6 insertions(+)arch/arm/kernel/process.c +0
-0arch/arm/kernel/process.c void machine_power_off(void) · line 221
Quoted 10 lines
arch/arm/kernel/process.c +6
-0@@ -212,6 +212,12 @@ void machine_power_off(void)if (pm_power_off)pm_power_off (); + +/* Give a grace period for failure to power off */ +mdelay (1000 ); + +pr_err ("Power off failed -- system halted\n" ); +while (1 ); }with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
Quoted 3 lines
That's expected (see below). It works with v3.18-rc1 vanilla because
machine_haltis called instead ofmachine_power_offas there is no registered power-off handler.
yeah, that much I figured :-)
Quoted 8 lines
Quoted 5 lines
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that
mdelay()to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay tomachine_power_off(). Should it be generic, or should we add it to our rtcpm_power_offimplementation ?As I wrote above, we still need a 2-second mdelay in rtc-omap, which I intend to add to the
pmic_power_enpatch.
oh, alright then. If you can Cc me, I'll make sure to test that too ;-)
Quoted 3 lines
The one-second delay is there in
machine_power_offto catch most cases which wouldn't take nearly as long as rtc-omap and hence wouldn't be adding an explicit delay in the driver power-off handler.
alright.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/47356d74/attachment.sig>
Quoted 22 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Quoted 3 lines · Felipe Balbi
Hi,
On Fri, Oct 24, 2014 at 02:25:40PM -0500, Felipe Balbi wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
That's expected (see below). It works with v3.18-rc1 vanilla because
machine_haltis called instead ofmachine_power_offas there is no registered power-off handler.yeah, that much I figured :-)
Quoted 6 lines
Quoted 5 lines
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that
mdelay()to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay tomachine_power_off(). Should it be generic, or should we add it to our rtcpm_power_offimplementation ?
Quoted 4 lines
Quoted 2 lines
As I wrote above, we still need a 2-second mdelay in rtc-omap, which I intend to add to the
pmic_power_enpatch.oh, alright then. If you can Cc me, I'll make sure to test that too ;-)
I will. :) Just wanted to see whether Andrew preferred I resend the whole series or just that one patch first.
The diff is minimal:
drivers/rtc/rtc-omap.c +2 -0
@@ -423,6 +423,8 @@ static void omap_rtc_power_off(void) val =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); + +mdelay (2000 ); }
Quoted 5 lines
Quoted 3 lines
The one-second delay is there in
machine_power_offto catch most cases which wouldn't take nearly as long as rtc-omap and hence wouldn't be adding an explicit delay in the driver power-off handler.alright.
I'll call it a day now. Will keep you posted.
Thanks,
Johan
Felipe Balbi
Tested-byQuoted 38 lines · Johan Hovold
Quoted 22 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Quoted 3 lines · Felipe Balbi
Hi,
On Fri, Oct 24, 2014 at 02:25:40PM -0500, Felipe Balbi wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
That's expected (see below). It works with v3.18-rc1 vanilla because
machine_haltis called instead ofmachine_power_offas there is no registered power-off handler.yeah, that much I figured :-)
Quoted 6 lines
Quoted 5 lines
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that
mdelay()to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay tomachine_power_off(). Should it be generic, or should we add it to our rtcpm_power_offimplementation ?Quoted 4 lines
Quoted 2 lines
As I wrote above, we still need a 2-second mdelay in rtc-omap, which I intend to add to the
pmic_power_enpatch.oh, alright then. If you can Cc me, I'll make sure to test that too ;-)
I will. :) Just wanted to see whether Andrew preferred I resend the whole series or just that one patch first.
The diff is minimal:
drivers/rtc/rtc-omap.c +0
-0
drivers/rtc/rtc-omap.c static void omap_rtc_power_off(void) · line 428
Quoted 6 lines
drivers/rtc/rtc-omap.c +2
-0@@ -423,6 +423,8 @@ static void omap_rtc_power_off(void) val =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); + +mdelay (2000 ); }
you can add my:
for this too. Now it's all good.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/50526f0d/attachment.sig>
Quoted 38 lines · On Fri, 24 Oct 2014 21:55:32 +0200 Johan Hovold <[email protected]> wrote:
Quoted 22 lines · Felipe Balbi
Quoted 5 lines · Johan Hovold
Quoted 3 lines · Felipe Balbi
Hi,
On Fri, Oct 24, 2014 at 02:25:40PM -0500, Felipe Balbi wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
with this I always get to "Power off failed -- system halted". If I switch to v3.18-rc1 vanilla, then it works. So it's definitely caused by your rtc-only patches.
That's expected (see below). It works with v3.18-rc1 vanilla because
machine_haltis called instead ofmachine_power_offas there is no registered power-off handler.yeah, that much I figured :-)
Quoted 6 lines
Quoted 5 lines
ok, so it seems like it takes more than 1 second for things to propagate. If I increase that
mdelay()to 3000, then everything works fine on my end. I think we should get RMK's input on this 3000ms delay tomachine_power_off(). Should it be generic, or should we add it to our rtcpm_power_offimplementation ?Quoted 4 lines
Quoted 2 lines
As I wrote above, we still need a 2-second mdelay in rtc-omap, which I intend to add to the
pmic_power_enpatch.oh, alright then. If you can Cc me, I'll make sure to test that too ;-)
I will. :) Just wanted to see whether Andrew preferred I resend the whole series or just that one patch first.
The diff is minimal:
drivers/rtc/rtc-omap.c +0
-0
drivers/rtc/rtc-omap.c static void omap_rtc_power_off(void) · line 428
Quoted 6 lines
drivers/rtc/rtc-omap.c +2
-0@@ -423,6 +423,8 @@ static void omap_rtc_power_off(void) val =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); + +mdelay (2000 ); }
Yes, having read this threadlet: we need a very good comment in there explaining what's going on, please.
Do we even need this delay on anything other than arm? Or even on all arm?
Quoted 21 lines · Andrew Morton
Quoted 10 lines · On Fri, 24 Oct 2014 21:55:32 +0200 Johan Hovold <[email protected]> wrote:
I will. :) Just wanted to see whether Andrew preferred I resend the whole series or just that one patch first.
The diff is minimal:
drivers/rtc/rtc-omap.c +0
-0drivers/rtc/rtc-omap.c static void omap_rtc_power_off(void) · line 428
Quoted 6 lines
drivers/rtc/rtc-omap.c +2
-0@@ -423,6 +423,8 @@ static void omap_rtc_power_off(void) val =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); + +mdelay (2000 ); }Yes, having read this threadlet: we need a very good comment in there explaining what's going on, please.
Quoted 1 line
Do we even need this delay on anything other than arm? Or even on all arm?
I think I've already commented on the behaviour of the reboot syscalls such as power off which can return to userspace, pointing out that x86 can return to userspace.
As long as x86 can return to userspace, I see no harm in ARM returning to userspace. If a driver which is hooking into the power off stuff is unable to immediately shut off the power (wtf it can't for 2 sec I've no idea) then having that driver work around that hardware's specific brokenness with a delay seems entirely reasonable.
That allows those SoCs which can do the right thing to do the right thing without being hindered by such silliness. And it also stops the next person coming along and bumping the delay from 2 to 3, to 5, and then what... 10 seconds?
Keeping it in the driver means that the workaround for the broken hardware is kept with the driver for the broken hardware - exactly where it should be.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.
Quoted 28 lines · Russell King - ARM Linux
Quoted 21 lines · Andrew Morton
Quoted 10 lines · On Fri, 24 Oct 2014 21:55:32 +0200 Johan Hovold <[email protected]> wrote:
I will. :) Just wanted to see whether Andrew preferred I resend the whole series or just that one patch first.
The diff is minimal:
drivers/rtc/rtc-omap.c +0
-0drivers/rtc/rtc-omap.c static void omap_rtc_power_off(void) · line 428
Quoted 6 lines
drivers/rtc/rtc-omap.c +2
-0@@ -423,6 +423,8 @@ static void omap_rtc_power_off(void) val =rtc_read (rtc,OMAP_RTC_INTERRUPTS_REG );rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); + +mdelay (2000 ); }Yes, having read this threadlet: we need a very good comment in there explaining what's going on, please.
Quoted 1 line
Do we even need this delay on anything other than arm? Or even on all arm?
I think I've already commented on the behaviour of the reboot syscalls such as power off which can return to userspace, pointing out that x86 can return to userspace.
Quoted 5 lines
As long as x86 can return to userspace, I see no harm in ARM returning to userspace. If a driver which is hooking into the power off stuff is unable to immediately shut off the power (wtf it can't for 2 sec I've no idea) then having that driver work around that hardware's specific brokenness with a delay seems entirely reasonable.
Yeah, there are two issues here. If a power-off handler is crazy slow there really should be a delay in the handler. That was just an oversight on my part. [ In this case it takes between one and two seconds due to the resolution of the rtc and they way it's alarm events are triggered. ]
The other issue is whether arch code should inform the user about failed power-off, in really exactly the same way as it does for failed reboot, see:
ac15e00b1efe ("ARM: restart: move reboot failure handing into machine_restart()"
by Russell.
It looks like we're soon to be having power-off call chains, with configurable priorities, to shut of various parts of the hardware and this is all at least partly configurable through DT. [1] I think it's reasonable to expect to see more frequent failures to power off either due to (DT) misconfiguration or broken or flakey hardware.
Having a short delay (I proposed 1s as for reboot) would also prevent any oopses when returning to user-space for just quite slow devices (e.g. millisecond range) without requiring explicit delays in these handlers.
But as Andrew points out above, this really isn't an arm-specific issue, and currently various arches deal with this differently, where some return to user-space, some spin indefinitely (without an error message), and some spin on failed reboot but not power-off (e.g. arm and arm64).
Quoted 4 lines
That allows those SoCs which can do the right thing to do the right thing without being hindered by such silliness. And it also stops the next person coming along and bumping the delay from 2 to 3, to 5, and then what... 10 seconds?
That wouldn't be an issue then. Arch code would only handle the non-crazy case and complete power-off failures.
Quoted 3 lines
Keeping it in the driver means that the workaround for the broken hardware is kept with the driver for the broken hardware - exactly where it should be.
Agreed.
Johan
Quoted 2 lines · Johan Hovold
It looks like we're soon to be having power-off call chains, with configurable priorities, to shut of various parts of the hardware
I really hope that they *don't* get used like that. I guess this is what happens when people don't read the code before they decide to implement something.
All the reboot/power off/halt methods already call into the driver model, and the driver model then iterates over all bound drivers calling their "shutdown" method. So, today, as it has been for years, all device drivers are notified of system power off.
That's where most device drivers should be cleanly stopping their hardware.
The only thing which is left is the actual hardware triggering of the action, and that should be what's done via the notifier.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.
Quoted 13 lines · Russell King - ARM Linux
Quoted 2 lines · Johan Hovold
It looks like we're soon to be having power-off call chains, with configurable priorities, to shut of various parts of the hardware
I really hope that they *don't* get used like that. I guess this is what happens when people don't read the code before they decide to implement something.
All the reboot/power off/halt methods already call into the driver model, and the driver model then iterates over all bound drivers calling their "shutdown" method. So, today, as it has been for years, all device drivers are notified of system power off.
Quoted 5 lines
That's where most device drivers should be cleanly stopping their hardware.
The only thing which is left is the actual hardware triggering of the action, and that should be what's done via the notifier.
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
Johan
Quoted 9 lines · Johan Hovold
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Or different from having no power-off handlers.
Here's the x86 code:
void machine_power_off(void)
{
machine_ops.power_off();
}struct machine_ops machine_ops = {
.power_off = native_machine_power_off,
...
static void native_machine_power_off(void)
{
if (pm_power_off) {
if (!reboot_force)
machine_shutdown();
pm_power_off();
}
/* A fallback in case there is no PM info available */
tboot_shutdown(TB_SHUTDOWN_HALT);
}void tboot_shutdown(u32 shutdown_type)
{
void (*shutdown)(void);
if (!tboot_enabled())
return;See - x86 can very well just fall straight back out of machine_power_off() if there's no pm_power_off() hook and tboot is not enabled.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Johan Hovold
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
Quoted 8 lines
Quoted 3 lines
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call machine_halt instead (via kernel_halt).
Quoted 23 lines
Here's the x86 code:
void
machine_power_off(void){ machine_ops.power_off(); }struct
machine_opsmachine_ops= {
.power_off =native_machine_power_off,
...static void
native_machine_power_off(void){ if (pm_power_off) { if (!reboot_force) machine_shutdown(); pm_power_off(); } /* A fallback in case there is no PM info available */ tboot_shutdown(TB_SHUTDOWN_HALT); }void
tboot_shutdown(u32 shutdown_type)
Quoted 8 lines
{ void (*shutdown)(void); if (!tboot_enabled()) return;See - x86 can very well just fall straight back out of
machine_power_off()if there's nopm_power_off()hook and tboot is not enabled.
I never doubted that, but is the right thing to do? Not all arches do it that way.
And what about the killing of init? Shall we simply consider that a systemd bug?
case LINUX_REBOOT_CMD_POWER_OFF: kernel_power_off(); do_exit(0); break;
If power-off fails (for whatever reason), do_exit(0) will trigger a panic when called from PID 1.
Johan
Johan:. do you really plan to use this "poweroff-source" property ? As you proposed a renaming few days ago...
I don't really want to waste time to propose patches to fix things incrementally and rename it if the old one is used...
Romain
2014-10-29 13:34 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 87 lines · Russell King - ARM Linux
Quoted 7 lines · Johan Hovold
Quoted 6 lines
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
Quoted 8 lines
Quoted 3 lines
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).Quoted 23 lines
Here's the x86 code:
void
machine_power_off(void){ machine_ops.power_off(); }struct
machine_opsmachine_ops= {
.power_off =native_machine_power_off,
...static void
native_machine_power_off(void){ if (pm_power_off) { if (!reboot_force) machine_shutdown(); pm_power_off(); } /* A fallback in case there is no PM info available */ tboot_shutdown(TB_SHUTDOWN_HALT); }void
tboot_shutdown(u32 shutdown_type)Quoted 8 lines
{ void (*shutdown)(void); if (!tboot_enabled()) return;See - x86 can very well just fall straight back out of
machine_power_off()if there's nopm_power_off()hook and tboot is not enabled.I never doubted that, but is the right thing to do? Not all arches do it that way.
And what about the killing of init? Shall we simply consider that a systemd bug?
case LINUX_REBOOT_CMD_POWER_OFF: kernel_power_off(); do_exit(0); break;If power-off fails (for whatever reason),
do_exit(0)will trigger a panic when called from PID 1.Johan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[ Please do not top-post. ]
Quoted 4 lines · Romain Perier
Johan:. do you really plan to use this "poweroff-source" property ? As you proposed a renaming few days ago...
I don't really want to waste time to propose patches to fix things incrementally and rename it if the old one is used...
Why would I want to use your retracted renaming proposal for this property (i.e. "poweroff-source")?
These patches use "ti,system-power-controller" and the vendor prefix will be dropped when your patches have been merged.
Johan
This just to be sure, nothing more. I did read that you mentionned "poweroff-source" earlier. However If I am wrong, my bad, everything is fine.
2014-10-29 14:00 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 15 lines
[ Please do not top-post. ]
Quoted 4 lines · Romain Perier
Johan:. do you really plan to use this "poweroff-source" property ? As you proposed a renaming few days ago...
I don't really want to waste time to propose patches to fix things incrementally and rename it if the old one is used...Why would I want to use your retracted renaming proposal for this property (i.e. "poweroff-source")?
These patches use "ti,system-power-controller" and the vendor prefix will be dropped when your patches have been merged.
Johan
[ Again, please stop with the top-posting.
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing? ]
Quoted 3 lines · Romain Perier
This just to be sure, nothing more. I did read that you mentionned "poweroff-source" earlier. However If I am wrong, my bad, everything is fine.
I only mentioned that you had proposed that name in my cover letter.
Johan
Quoted 10 lines · Johan Hovold
Quoted 4 lines · Russell King - ARM Linux
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Quoted 8 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
If the handler has determined that it has failed, then why delay before trying the next handler? At the point it has decided it has failed, surely that's after it has waited sufficient time to determine that failure?
Quoted 4 lines
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).
Today, ARM does exactly what x86 does. If there's no power off handler registered, machine_power_off() shuts down other CPUs and returns.
Quoted 26 lines
Quoted 26 lines
Here's the x86 code:
void
machine_power_off(void){ machine_ops.power_off(); }struct
machine_opsmachine_ops= {
.power_off =native_machine_power_off,
...static void
native_machine_power_off(void){ if (pm_power_off) { if (!reboot_force) machine_shutdown(); pm_power_off(); } /* A fallback in case there is no PM info available */ tboot_shutdown(TB_SHUTDOWN_HALT); }void
tboot_shutdown(u32 shutdown_type){ void (*shutdown)(void);
Quoted 8 lines
Quoted 5 lines
if (!tboot_enabled()) return;See - x86 can very well just fall straight back out of
machine_power_off()if there's nopm_power_off()hook and tboot is not enabled.I never doubted that, but is the right thing to do? Not all arches do it that way.
Well, the biggest question there is: if the power off or restart syscall fails, what is the _generic_ non-architecture action which is supposed to happen?
Whatever the answer is to that question, that action should be performed by the _generic_ non-architecture code, rather than having the same implementation spread across all 30 architectures which the kernel supports today.
Quoted 10 lines
And what about the killing of init? Shall we simply consider that a systemd bug?
case LINUX_REBOOT_CMD_POWER_OFF: kernel_power_off(); do_exit(0); break;If power-off fails (for whatever reason),
do_exit(0)will trigger a panic when called from PID 1.
Oh, systemd calls this from PID1? I guess that's another reason to hate systemd with vitriol. :) SysVinit and upstart implementations call it from the "halt" command, which is itself normally run from a script, which totally avoids that problem.
I'm quite sure the insane systemd lobby will scream that this is a kernel bug and will want to change it somehow, just like they want to change the kernel in soo many other silly ways.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.
Quoted 18 lines · Russell King - ARM Linux
Quoted 17 lines · Johan Hovold
Quoted 4 lines · Russell King - ARM Linux
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Quoted 6 lines
Quoted 1 line
Should there always be a short delay before calling the next handler?
If the handler has determined that it has failed, then why delay before trying the next handler? At the point it has decided it has failed, surely that's after it has waited sufficient time to determine that failure?
The current handlers we have are not expecting any other handler to be run after they return. My question was whether all these handlers should get a short mdelay added to them (e.g. to compensate for bus latencies) or if this could be done in the power-off handler (e.g. before printing the error message).
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).Today, ARM does exactly what x86 does. If there's no power off handler registered,
machine_power_off()shuts down other CPUs and returns.
No, if there are no power-off handlers registered, kernel/reboot.c will never call machine_power_off:
/* Instead of trying to make the power_off code look like * halt when pm_power_off is not set do it the easy way. */ if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) cmd = LINUX_REBOOT_CMD_HALT;
So in that case on arm, a system-halted message is printed, and we never return to user-space.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Here's the x86 code:
void
machine_power_off(void){ machine_ops.power_off(); }struct
machine_opsmachine_ops= {
.power_off =native_machine_power_off,
...static void
native_machine_power_off(void){ if (pm_power_off) { if (!reboot_force) machine_shutdown(); pm_power_off(); } /* A fallback in case there is no PM info available */ tboot_shutdown(TB_SHUTDOWN_HALT); }void
tboot_shutdown(u32 shutdown_type){ void (*shutdown)(void);Quoted 8 lines
Quoted 5 lines
if (!tboot_enabled()) return;See - x86 can very well just fall straight back out of
machine_power_off()if there's nopm_power_off()hook and tboot is not enabled.I never doubted that, but is the right thing to do? Not all arches do it that way.
Quoted 8 lines
Well, the biggest question there is: if the power off or restart syscall fails, what is the _generic_ non-architecture action which is supposed to happen?
Whatever the answer is to that question, that action should be performed by the _generic_ non-architecture code, rather than having the same implementation spread across all 30 architectures which the kernel supports today.
I fully agree.
Quoted 8 lines
Quoted 8 lines
And what about the killing of init? Shall we simply consider that a systemd bug?
case LINUX_REBOOT_CMD_POWER_OFF: kernel_power_off(); do_exit(0); break;
Quoted 7 lines
Quoted 2 lines
If power-off fails (for whatever reason),
do_exit(0)will trigger a panic when called from PID 1.Oh, systemd calls this from PID1? I guess that's another reason to hate systemd with vitriol. :) SysVinit and upstart implementations call it from the "halt" command, which is itself normally run from a script, which totally avoids that problem.
Yeah, that's why I never noticed the missing mdelay.
Quoted 3 lines
I'm quite sure the insane systemd lobby will scream that this is a kernel bug and will want to change it somehow, just like they want to change the kernel in soo many other silly ways.
Will be interesting to follow. :)
Johan
Quoted 21 lines · Johan Hovold
Quoted 20 lines · Russell King - ARM Linux
Quoted 18 lines · Johan Hovold
Quoted 4 lines · Russell King - ARM Linux
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
Quoted 8 lines
Quoted 4 lines
If the handler has determined that it has failed, then why delay before trying the next handler? At the point it has decided it has failed, surely that's after it has waited sufficient time to determine that failure?
The current handlers we have are not expecting any other handler to be run after they return. My question was whether all these handlers should get a short mdelay added to them (e.g. to compensate for bus latencies)
Some of them do add a delay.
Quoted 3 lines
or if this could be done in the power-off handler (e.g. before printing the error message).
That might make sense, but it would have to be configurable, since the delay is platform specific and power-off handler does not know how long to wait (the longest delay I have seen is 10 seconds).
Quoted 11 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).Today, ARM does exactly what x86 does. If there's no power off handler registered,
machine_power_off()shuts down other CPUs and returns.No, if there are no power-off handlers registered,
kernel/reboot.cwill never callmachine_power_off:
Quoted 9 lines
/* Instead of trying to make the power_off code look like * halt when pm_power_off is not set do it the easy way. */ if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) cmd = LINUX_REBOOT_CMD_HALT;So in that case on arm, a system-halted message is printed, and we never return to user-space.
Some architectures do that, or go into an endless loop. Others do return from machine_power_off. Having a well defined behavior would be nice (such as dumping an error mesasge and calling machine_halt if machine_power_off returns). Only question would be where to put it. kernel_power_off() might be a good place; only problem is that there are direct callers of machine_power_off().
Guenter
Quoted 27 lines · Guenter Roeck
Quoted 26 lines · Johan Hovold
Quoted 24 lines · Russell King - ARM Linux
Quoted 10 lines · Johan Hovold
Quoted 4 lines · Russell King - ARM Linux
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Quoted 8 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
If the handler has determined that it has failed, then why delay before trying the next handler? At the point it has decided it has failed, surely that's after it has waited sufficient time to determine that failure?
Quoted 5 lines
Quoted 3 lines
The current handlers we have are not expecting any other handler to be run after they return. My question was whether all these handlers should get a short mdelay added to them (e.g. to compensate for bus latencies)
Some of them do add a delay.
Yes, but not all.
Quoted 6 lines
Quoted 3 lines
or if this could be done in the power-off handler (e.g. before printing the error message).
That might make sense, but it would have to be configurable, since the delay is platform specific and power-off handler does not know how long to wait (the longest delay I have seen is 10 seconds).
We've already concluded in this thread that such delays must be encoded in the actual handler (even if it's an argument to the power-off call chain code). The only exception should be generic handlers such as gpio-poweroff, which may need to define different delays depending on board. This could of course just be an argument of the corresponding DT node.
My question above was if it was reasonable to add some generic short delay after calling each power-off handler to handle short power-off latencies (e.g. bus latencies) so that not every handler needs an explicit delay.
Quoted 17 lines
Quoted 17 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).Today, ARM does exactly what x86 does. If there's no power off handler registered,
machine_power_off()shuts down other CPUs and returns.No, if there are no power-off handlers registered,
kernel/reboot.cwill never callmachine_power_off:/* Instead of trying to make the power_off code look like * halt when pm_power_off is not set do it the easy way. */ if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) cmd = LINUX_REBOOT_CMD_HALT;
Quoted 5 lines
Quoted 3 lines
So in that case on arm, a system-halted message is printed, and we never return to user-space.
Some architectures do that, or go into an endless loop. Others do return from
machine_power_off.
Please re-read my comment and the code above. machine_power_off is never called if there's no handler registered.
Some archs machine_power_off to spin on failed power-off (i.e. when there is a handler), something which I've mentioned a few times already in this thread.
Quoted 5 lines
Having a well defined behavior would be nice
(such as dumping an error mesasge and callingmachine_haltifmachine_power_offreturns). Only question would be where to put it.kernel_power_off()might be a good place; only problem is that there are direct callers ofmachine_power_off().
Indeed. Adding an error message to the power-off handler call chain code would solve the first problem as I mentioned before.
Then it's mostly a matter of whether we care about consistency, and either remove the indefinite spins from those non-x86/non-arm arches, or prevent the latter (and some others) from returning to user-space.
I'm inclined to having all arches return to user space on failed power-off, even if it means systemd cannot call reboot() from PID 1.
Johan
Quoted 43 lines · Johan Hovold
Quoted 27 lines · Guenter Roeck
Quoted 26 lines · Johan Hovold
Quoted 24 lines · Russell King - ARM Linux
Quoted 10 lines · Johan Hovold
Quoted 4 lines · Russell King - ARM Linux
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Quoted 8 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
If the handler has determined that it has failed, then why delay before trying the next handler? At the point it has decided it has failed, surely that's after it has waited sufficient time to determine that failure?
Quoted 5 lines
Quoted 3 lines
The current handlers we have are not expecting any other handler to be run after they return. My question was whether all these handlers should get a short mdelay added to them (e.g. to compensate for bus latencies)
Some of them do add a delay.
Yes, but not all.
Quoted 6 lines
Quoted 3 lines
or if this could be done in the power-off handler (e.g. before printing the error message).
That might make sense, but it would have to be configurable, since the delay is platform specific and power-off handler does not know how long to wait (the longest delay I have seen is 10 seconds).
Quoted 6 lines
We've already concluded in this thread that such delays must be encoded in the actual handler (even if it's an argument to the power-off call chain code). The only exception should be generic handlers such as gpio-poweroff, which may need to define different delays depending on board. This could of course just be an argument of the corresponding DT node.
There probably should be a generic device-tree property for this, even if some delays would be device specific (e.g. the two second delay for omap rtc).
But on top of that, external latencies would be quite board specific. The PMIC on the Beaglebone black adds a 50ms deglitch time, for instance.
Quoted 4 lines
My question above was if it was reasonable to add some generic short delay after calling each power-off handler to handle short power-off latencies (e.g. bus latencies) so that not every handler needs an explicit delay.
I just added a 500ms margin to the OMAP rtc power-off handler. That should be more than enough, and perhaps something like that could be a default in the power-off call chain. Consider that arch-arm has a 1s grace period before reporting failed reboot already today.
Johan
Quoted 12 lines · Johan Hovold
Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Johan Hovold
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
Quoted 9 lines
Quoted 4 lines
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Good that I just replaced notifier chain with an open coded implementation. Sure, that is possible, but I would prefer to do that as a follow-up commit, and it should be discussed in the context of the power-off handler patch set.
Quoted 9 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
That delay would depend on the individual power-off handler, so I think the current implementation works just fine (where power-off handlers implement the delay).
We could move the delay into the infrastructure, but it would have to be configurable. I would prefer to consider that as a follow-up patch to not overload the power-off handler patch set with too many changes at the same time.
Quoted 43 lines
Quoted 1 line
Or different from having no power-off handlers.
That is actually quite different, as in that case we call
machine_haltinstead (viakernel_halt).Quoted 23 lines
Here's the x86 code:
void
machine_power_off(void){ machine_ops.power_off(); }struct
machine_opsmachine_ops= {
.power_off =native_machine_power_off,
...static void
native_machine_power_off(void){ if (pm_power_off) { if (!reboot_force) machine_shutdown(); pm_power_off(); } /* A fallback in case there is no PM info available */ tboot_shutdown(TB_SHUTDOWN_HALT); }void
tboot_shutdown(u32 shutdown_type)Quoted 8 lines
{ void (*shutdown)(void); if (!tboot_enabled()) return;See - x86 can very well just fall straight back out of
machine_power_off()if there's nopm_power_off()hook and tboot is not enabled.I never doubted that, but is the right thing to do? Not all arches do it that way.
And what about the killing of init? Shall we simply consider that a systemd bug?
Quoted 8 lines
case LINUX_REBOOT_CMD_POWER_OFF: kernel_power_off(); do_exit(0); break;If power-off fails (for whatever reason),
do_exit(0)will trigger a panic when called from PID 1.
Common handling of that condition - eg to call machine_halt() - might be an option. Separate patch, though.
Guenter
Quoted 18 lines · Guenter Roeck
Quoted 17 lines · Johan Hovold
Quoted 15 lines · Russell King - ARM Linux
Quoted 9 lines · Johan Hovold
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Quoted 5 lines
Quoted 3 lines
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Good that I just replaced notifier chain with an open coded implementation.
Good to hear.
Quoted 2 lines
Sure, that is possible, but I would prefer to do that as a follow-up commit, and it should be discussed in the context of the power-off handler patch set.
Fine with me.
Quoted 7 lines
Quoted 7 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Quoted 5 lines
Quoted 1 line
Should there always be a short delay before calling the next handler?
That delay would depend on the individual power-off handler, so I think the current implementation works just fine (where power-off handlers implement the delay).
Some don't, and could possibly unknowingly have been relying on the fact that they could return to user space and be powered off at some later time. With systemd that would have caused a panic.
Also consider generic power-off handlers such as gpio-poweroff. It currently hard-codes a three-second delay but the actual delay would really be board specific.
Quoted 4 lines
We could move the delay into the infrastructure, but it would have to be configurable. I would prefer to consider that as a follow-up patch to not overload the power-off handler patch set with too many changes at the same time.
Sure.
Johan
Quoted 41 lines · Johan Hovold
Quoted 18 lines · Guenter Roeck
Quoted 17 lines · Johan Hovold
Quoted 15 lines · Russell King - ARM Linux
Quoted 9 lines · Johan Hovold
That's not what I was trying to refer to. But the patch set explicitly allows for multiple, prioritised power-off handlers, which can power off a board in different ways and with various degrees of success. Specifically, it allows for fallback handlers in case one or more power-off handlers fail.
So if we allow for that, what is to prevent the final power-off handler from failing? And should this not be logged by arch code in the same way as failure to restart is?
And how is that different from having a set of power-off handlers, and reporting when each individual one fails? Don't you want to know if your primary high priority reboot handler fails, just as much as you want to know if your final last-resort power-off handler fails?
Quoted 5 lines
Quoted 3 lines
Good point. Failed power-off should probably be logged by the power-off call chain implementation (which seems to makes notifier chains a bad fit).
Good that I just replaced notifier chain with an open coded implementation.
Good to hear.
Quoted 2 lines
Sure, that is possible, but I would prefer to do that as a follow-up commit, and it should be discussed in the context of the power-off handler patch set.
Fine with me.
Quoted 9 lines
Quoted 8 lines
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
Quoted 8 lines
Quoted 3 lines
That delay would depend on the individual power-off handler, so I think the current implementation works just fine (where power-off handlers implement the delay).
Some don't, and could possibly unknowingly have been relying on the fact that they could return to user space and be powered off at some later time. With systemd that would have caused a panic.
Agreed, but there are two cases to consider: What should be the delay before the next power-off handler is called, and what should the system do if all power-off handlers fail (or if there are none). The current behavior isn't exactly well defined. Ok, with systemd that results in a crash, but I am not really sure if one can or should blame systemd for that. The discussion about systemd and its philosophy should not cloud the fact that power-off behavior isn't exactly well defined.
Quoted 4 lines
Also consider generic power-off handlers such as gpio-poweroff. It currently hard-codes a three-second delay but the actual delay would really be board specific.
A configurable delay would address that. The actually required delay could be provided in platform data or as devicetree property.
Thanks,
Guenter
Quoted 21 lines · Guenter Roeck
Quoted 12 lines · Johan Hovold
Quoted 11 lines · Guenter Roeck
Quoted 8 lines · Johan Hovold
And what about any power-off latencies? Should this always be dealt with in the power-off handler?
Again, if it's predictable and high, as in the OMAP RTC case, it should go in the handler. But what if it's just normal bus latencies (peripheral busses, i2c, or whatever people may come up with)?
Should there always be a short delay before calling the next handler?
Quoted 7 lines
Quoted 3 lines
That delay would depend on the individual power-off handler, so I think the current implementation works just fine (where power-off handlers implement the delay).
Some don't, and could possibly unknowingly have been relying on the fact that they could return to user space and be powered off at some later time. With systemd that would have caused a panic.
Quoted 7 lines
Agreed, but there are two cases to consider: What should be the delay before the next power-off handler is called, and what should the system do if all power-off handlers fail (or if there are none). The current behavior isn't exactly well defined. Ok, with systemd that results in a crash, but I am not really sure if one can or should blame systemd for that. The discussion about systemd and its philosophy should not cloud the fact that power-off behavior isn't exactly well defined.
Sounds like we pretty much agree. See my response to your last mail.
Quoted 6 lines
Quoted 4 lines
Also consider generic power-off handlers such as gpio-poweroff. It currently hard-codes a three-second delay but the actual delay would really be board specific.
A configurable delay would address that. The actually required delay could be provided in platform data or as devicetree property.
Yep, see mail mentioned above.
Johan
PATCH v3 1/1 rtc: omap: add support for pmic_power_en
In reply to Johan HovoldAdd new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the pmic_power_en pin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.
This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
---
Changes since v2:
- add two-second delay to allow alarm to trigger before returning
Andrew, can you replace just this patch in the series that you already have in your tree, or do you prefer I resend the whole series (with Felipe's Tested-by tags)?
Thanks,
Johan
Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 95 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/rtc/rtc-omap.txt +8 -1
@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller +Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system power + through pmic_power_en + Example: rtc at 1c23000 {
@@ -18,4 +24,5 @@ rtc at 1c23000 { interrupts = <19 19>; interrupt-parent = <&intc>; + ti,system-power-controller; };
drivers/rtc/rtc-omap.c +95 -0
167 lines not on this page · raw message
Signature · 1 line
2.0.4
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 38 lines · Johan Hovold
Add new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the
pmic_power_enpin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
Tested-by Felipe Balbi <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Changes since v2:
- add two-second delay to allow alarm to trigger before returningAndrew, can you replace just this patch in the series that you already have in your tree, or do you prefer I resend the whole series (with Felipe's Tested-by tags)?
Thanks,
JohanDocumentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 95 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/rtc/rtc-omap.txt +0
-0@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable
Documentation/devicetree/bindings/rtc/rtc-omap.txt Required properties: · line 16
Quoted 10 lines
Documentation/devicetree/bindings/rtc/rtc-omap.txt +5
-1- Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller +Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system power
isn't there a discussion going on to drop the vendor prefix ? I wonder if we should just use the final binding to avoid supporting this for rtc-omap too. OTOH, all of that can be hidden under of_is_system_power_controller() itself.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141027/213f7a3f/attachment.sig>
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 47 lines · Felipe Balbi
Quoted 46 lines · Johan Hovold
Add new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the
pmic_power_enpin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
Tested-by Felipe Balbi <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Changes since v2:
- add two-second delay to allow alarm to trigger before returningAndrew, can you replace just this patch in the series that you already have in your tree, or do you prefer I resend the whole series (with Felipe's Tested-by tags)?
Thanks,
JohanDocumentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 95 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/rtc/rtc-omap.txt +3
-1@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller
Quoted 7 lines
Documentation/devicetree/bindings/rtc/rtc-omap.txt Required properties: · line 16
Quoted 2 lines
Documentation/devicetree/bindings/rtc/rtc-omap.txt +2
-0+Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system powerisn't there a discussion going on to drop the vendor prefix ? I wonder if we should just use the final binding to avoid supporting this for rtc-omap too. OTOH, all of that can be hidden under
of_is_system_power_controller()itself.
Exactly, Romain intends to support both versions (i.e. with and without the prefix) with his helper function as there are already other uses of this variant in the wild.
As I believe I mentioned in my cover letter, I suggest simply updating to the generic property name (i.e. use the helper and drop the prefix) once that code has been merged. Either way, we should always be able to change the property name before it has been used in a mainline release (as long as we update the driver and any dts use in one commit in order not to break bisectability).
Johan
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 60 lines · Johan Hovold
Quoted 47 lines · Felipe Balbi
Quoted 46 lines · Johan Hovold
Add new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the
pmic_power_enpin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
Tested-by Felipe Balbi <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Changes since v2:
- add two-second delay to allow alarm to trigger before returningAndrew, can you replace just this patch in the series that you already have in your tree, or do you prefer I resend the whole series (with Felipe's Tested-by tags)?
Thanks,
JohanDocumentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 95 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/rtc/rtc-omap.txt +3
-1@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controllerQuoted 7 lines
Documentation/devicetree/bindings/rtc/rtc-omap.txt Required properties: · line 16
Quoted 2 lines
Documentation/devicetree/bindings/rtc/rtc-omap.txt +2
-0+Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system powerisn't there a discussion going on to drop the vendor prefix ? I wonder if we should just use the final binding to avoid supporting this for rtc-omap too. OTOH, all of that can be hidden under
of_is_system_power_controller()itself.Exactly, Romain intends to support both versions (i.e. with and without the prefix) with his helper function as there are already other uses of this variant in the wild.
Quoted 6 lines
As I believe I mentioned in my cover letter, I suggest simply updating to the generic property name (i.e. use the helper and drop the prefix) once that code has been merged. Either way, we should always be able to change the property name before it has been used in a mainline release (as long as we update the driver and any dts use in one commit in order not to break bisectability).
fine by me :-) cheers
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141027/7e5fe957/attachment.sig>
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
In reply to Johan HovoldQuoted 9 lines · On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <[email protected]> wrote:
Add new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the
pmic_power_enpin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
Quoted 8 lines
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
Tested-by Felipe Balbi <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Changes since v2:
- add two-second delay to allow alarm to trigger before returning
hmpf. As this sentence is below the ^--- it doesn't get into the changelog.
Quoted 33 lines
...
(quoted diff) +31
-0+static void omap_rtc_power_off(void) +{ + struct omap_rtc *rtc = omap_rtc_power_off_rtc; + struct rtc_time tm; + unsigned long now; + u32 val; + + /* enable pmic_power_en control */ + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); + rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN); + + /* set alarm two seconds from now */ + omap_rtc_read_time_raw(rtc, &tm); + bcd2tm(&tm); + rtc_tm_to_time(&tm, &now); + rtc_time_to_tm(now + 2, &tm); + + if (tm2bcd(&tm) < 0) { + dev_err(&rtc->rtc->dev, "power off failed\n"); + return; + } + + rtc_wait_not_busy(rtc); + + rtc_write(rtc, OMAP_RTC_ALARM2_SECONDS_REG, tm.tm_sec); + rtc_write(rtc, OMAP_RTC_ALARM2_MINUTES_REG, tm.tm_min); + rtc_write(rtc, OMAP_RTC_ALARM2_HOURS_REG, tm.tm_hour); + rtc_write(rtc, OMAP_RTC_ALARM2_DAYS_REG, tm.tm_mday); + rtc_write(rtc, OMAP_RTC_ALARM2_MONTHS_REG, tm.tm_mon); + rtc_write(rtc, OMAP_RTC_ALARM2_YEARS_REG, tm.tm_year); +
Review
Quoted 10 lines
(quoted diff) +10
-0+ /* + * enable ALARM2 interrupt + * + * NOTE: this fails on AM3352 if rtc_write (writeb) is used + */ + val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG); + rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG, + val | OMAP_RTC_INTERRUPTS_IT_ALARM2); + + mdelay(2000);
And it is uncommented.
How on earth is a reader to know why this is here?
I can do this
drivers/rtc/rtc-omap.c renamed from drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix +1 -0
@@ -417,6 +417,7 @@ static void omap_rtc_power_off(void)rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); +/* Allow alarm to trigger before returning */ mdelay (2000 ); }
But it doesn't explain *why* we want the alarm to trigger before returning.
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 17 lines · Andrew Morton
Quoted 15 lines · On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <[email protected]> wrote:
Add new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the
pmic_power_enpin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
Tested-by Felipe Balbi <[email protected]>Signed-off-by Johan Hovold <[email protected]>---
Quoted 5 lines
Quoted 2 lines
Changes since v2:
- add two-second delay to allow alarm to trigger before returninghmpf. As this sentence is below the ^--- it doesn't get into the changelog.
We usually don't keep the patch-revision change log in the commit message (e.g. put in the cover letter).
But in general, how do you want to handle updates to a single patch in a series you already have in your tree? Do you prefer a proper incremental-fix patch (with commit message), just an updated single patch, or a resend of the whole series?
Quoted 46 lines
Quoted 33 lines
...
(quoted diff) +31
-0+static void omap_rtc_power_off(void) +{ + struct omap_rtc *rtc = omap_rtc_power_off_rtc; + struct rtc_time tm; + unsigned long now; + u32 val; + + /* enable pmic_power_en control */ + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); + rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN); + + /* set alarm two seconds from now */ + omap_rtc_read_time_raw(rtc, &tm); + bcd2tm(&tm); + rtc_tm_to_time(&tm, &now); + rtc_time_to_tm(now + 2, &tm); + + if (tm2bcd(&tm) < 0) { + dev_err(&rtc->rtc->dev, "power off failed\n"); + return; + } + + rtc_wait_not_busy(rtc); + + rtc_write(rtc, OMAP_RTC_ALARM2_SECONDS_REG, tm.tm_sec); + rtc_write(rtc, OMAP_RTC_ALARM2_MINUTES_REG, tm.tm_min); + rtc_write(rtc, OMAP_RTC_ALARM2_HOURS_REG, tm.tm_hour); + rtc_write(rtc, OMAP_RTC_ALARM2_DAYS_REG, tm.tm_mday); + rtc_write(rtc, OMAP_RTC_ALARM2_MONTHS_REG, tm.tm_mon); + rtc_write(rtc, OMAP_RTC_ALARM2_YEARS_REG, tm.tm_year); +Review
Quoted 10 lines
(quoted diff) +10
-0+ /* + * enable ALARM2 interrupt + * + * NOTE: this fails on AM3352 if rtc_write (writeb) is used + */ + val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG); + rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG, + val | OMAP_RTC_INTERRUPTS_IT_ALARM2); + + mdelay(2000);And it is uncommented.
Quoted 1 line
How on earth is a reader to know why this is here?
The comment above the function reads:
* The RTC can be used to control an external PMIC via the pmic_power_en pin,
* which can be configured to transition to OFF on ALARM2 events. *
* Notes:
* The two-second alarm offset is the shortest offset possible as the alarm
* registers must be set before the next timer update and the offset
* calculation is too heavy for everything to be done within a single access
* period (~15us).
So it's effect is at least fairly obvious and documented.
drivers/rtc/rtc-omap.c static void omap_rtc_power_off(void) · line 422
Quoted 11 lines
I can do this
drivers/rtc/rtc-omap.c renamed from drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix +1
-0@@ -417,6 +417,7 @@ static void omap_rtc_power_off(void)rtc_writel (rtc,OMAP_RTC_INTERRUPTS_REG , val |OMAP_RTC_INTERRUPTS_IT_ALARM2 ); +/* Allow alarm to trigger before returning */ mdelay (2000 ); }
Looks good, and I should have put something like that there nonetheless.
Quoted 2 lines
But it doesn't explain *why* we want the alarm to trigger before returning.
Should we really require every power-off handler to document arch behaviour (even if its inconsistent and currently undocumented); in this case that some arches return to user-space where we may oops if called from process 0 (e.g. systemd, but not if using sysvinit)?
Johan
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 7 lines · On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold <[email protected]> wrote:
Quoted 2 lines
But it doesn't explain *why* we want the alarm to trigger before returning.
Should we really require every power-off handler to document arch behaviour (even if its inconsistent and currently undocumented); in this case that some arches return to user-space where we may oops if called from process 0 (e.g. systemd, but not if using sysvinit)?
The kernel really doesn't have a problem related to excessive amounts of useful code comments :(
The bottom line is: did I provide a reader with the ability to understand why the code is this way? If "no" then improvements beckon.
This does look like one code site where an elaborate explanation is warranted. There's no way in which a reader can get to your above paragraph from the current rtc-omap.c.
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 13 lines · Andrew Morton
Quoted 7 lines · On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold <[email protected]> wrote:
Quoted 2 lines
But it doesn't explain *why* we want the alarm to trigger before returning.
Should we really require every power-off handler to document arch behaviour (even if its inconsistent and currently undocumented); in this case that some arches return to user-space where we may oops if called from process 0 (e.g. systemd, but not if using sysvinit)?
The kernel really doesn't have a problem related to excessive amounts of useful code comments :(
Quoted 6 lines
The bottom line is: did I provide a reader with the ability to understand why the code is this way? If "no" then improvements beckon.
This does look like one code site where an elaborate explanation is warranted. There's no way in which a reader can get to your above paragraph from the current
rtc-omap.c.
I agree with you that I should add a comment on why that mdelay is there to make it perfectly clear and obvious that it's purpose is to let the alarm trigger, which in turn causes the pmic to power off the system.
It is a power-off handler, and any power-off handler should not return until it has at least attempted to power off the system. In this case, that mandates a two-second delay.
So far, so good. I don't agree with you that every power-off handler should document what happens if it fails to power-off the system. That is, to describe what arches will happily return to user space, and under what conditions (e.g. what init system is used) that this will cause a panic.
If anything that belongs in arch code or kernel/reboot.c, and is also something that is likely to change over time (consider the power-off-handler call chains).
Johan
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
In reply to Johan HovoldQuoted 7 lines · Johan Hovold
Quoted 1 line · Andrew Morton
On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <[email protected]> wrote:
But in general, how do you want to handle updates to a single patch in a series you already have in your tree? Do you prefer a proper incremental-fix patch (with commit message), just an updated single patch, or a resend of the whole series?
How should I best send the updated patch? Can you just replace the current three incremental patches:
rtc-omap-add-support-for-pmic_power_en.patch rtc-omap-add-support-for-pmic_power_en-v3.patch rtc-omap-add-support-for-pmic_power_en-v3-fix.patch
that you have in your tree, with a single new v4 which adds a more elaborate comment?
Thanks,
Johan
Re: [PATCH v3] rtc: omap: add support for pmic_power_en
Quoted 12 lines · On Wed, 29 Oct 2014 13:50:05 +0100 Johan Hovold <[email protected]> wrote:
Quoted 7 lines · Johan Hovold
Quoted 1 line · Andrew Morton
On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <[email protected]> wrote:
But in general, how do you want to handle updates to a single patch in a series you already have in your tree? Do you prefer a proper incremental-fix patch (with commit message), just an updated single patch, or a resend of the whole series?
How should I best send the updated patch? Can you just replace the current three incremental patches:
Quoted 6 lines
rtc-omap-add-support-for-pmic_power_en.patch rtc-omap-add-support-for-pmic_power_en-v3.patch rtc-omap-add-support-for-pmic_power_en-v3-fix.patch
that you have in your tree, with a single new v4 which adds a more elaborate comment?
Yep, that works. I'll almost always turn the new patch into an incremental, mainly so that I (and others) can see what was changed.
PATCH v4 1/1 rtc: omap: add support for pmic_power_en
In reply to Johan HovoldAdd new property "ti,system-power-controller" to register the RTC as a power-off handler.
Some RTC IP revisions can control an external PMIC via the pmic_power_en pin, which can be configured to transition to OFF on ALARM2 events and back to ON on subsequent ALARM (wakealarm) events.
This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1]
[1] https://www.mail-archive.com/linux-omap at vger.kernel.org/msg82127.html
---
Here's an updated patch, which adds a comment on why there is a delay after activating the alarm.
Note that I also increased the delay to include a margin for external latencies. For example, on Beaglebone Black, the PMIC has a 50ms deglitch time on the PWR_EN pin (which is connected to pmic_power_en).
Such margins should probably eventually be specified through device tree, for instance, as an attribute to accompany the system-power-controller attribute and be handled by the power-off call chain infrastructure.
Johan
Changes since v3:
- increase the delay when waiting for power-off to include a 500ms margin for external latencies (e.g. debounce circuits)
- add comment on why that delay is there
- add a space to one comment
Changes since v2:
- add two-second delay to allow alarm to trigger before returning
Documentation/devicetree/bindings/rtc/rtc-omap.txt | 9 +- drivers/rtc/rtc-omap.c | 100 +++++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/rtc/rtc-omap.txt +8 -1
@@ -5,11 +5,17 @@ Required properties: - "ti,da830-rtc" - for RTC IP used similar to that on DA8xx SoC family. - "ti,am3352-rtc" - for RTC IP used similar to that on AM335x SoC family. This RTC IP has special WAKE-EN Register to enable - Wakeup generation for event Alarm. + Wakeup generation for event Alarm. It can also be + used to control an external PMIC via the + pmic_power_en pin. - reg: Address range of rtc register set - interrupts: rtc timer, alarm interrupts in order - interrupt-parent: phandle for the interrupt controller +Optional properties: +- ti,system-power-controller: whether the rtc is controlling the system power + through pmic_power_en + Example: rtc at 1c23000 {
@@ -18,4 +24,5 @@ rtc at 1c23000 { interrupts = <19 19>; interrupt-parent = <&intc>; + ti,system-power-controller; };
drivers/rtc/rtc-omap.c +100 -0
172 lines not on this page · raw message
Signature · 1 line
2.0.4