RE: [rtc-linux] [PATCH v3 1/5] rtc: OMAP: Add system pm_power_off to rtc driver
On Wed, Nov 28, 2012 at 16:42:26, Russell King - ARM Linux wrote:
On Tue, Nov 27, 2012 at 03:42:39PM -0800, Andrew Morton wrote:
+ /* Do not allow to execute any other task */
+ spin_lock_irqsave(lock, flags);
+ while (1);
I suspect this doesn't do what you want it to do.
Firstly, please provide adequate code comments here so that code
readers do not also need to be mind readers.
If you want to stop this CPU dead in its tracks (why?) then
local_irq_disable();
while (1)
; /* Note correct code layout */
will do it. But it means that the NMI watchdog (if present) will come
along and whack the machine in the head a few seconds later. And this
does nothing to stop other CPUs.
But not being a mind reader, I'm really at a loss to suggest what
should be done here.
Here intention is to disable all interrupts between rtc power_off request
till system actually went to power off mode, max 2 seconds based on when
the AM335x RTC alarm2 expires. The idea was that since the system is
going to shutdown, it is better to not process any more interrupts.
It's hooking into the pm_power_off hook, which is called from kernel/sys.c
via arch code. We will have already stopped all other CPUs at this point.
Why there's that while (1) there I don't know; when pm_power_off is not
hooked, we don't do anything like that - and what will happen in that
case is we'll return all the way back to sys_reboot(), which will call
do_exit(0) on us.
When testing with v3.7-rc7, I saw the following BUG() triggered when I did
not use a while(1). Logs below.
Before calling do_exit(0), there is a reboot_mutex lock that is taken in
reboot syscall. do_exit() function is , checking for any locks held by the
processor and if yes then it is printing a BUG_ON from print_held_locks_bug()
function along with the locks held information.
Even though the BUG triggers, the system is going to power off because
The hardware has been triggered.
(log)
[root@arago /]# poweroff
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system poweroff
[ 32.125900] Disabling non-boot CPUs ...
[ 32.130155] Power down.
[ 32.132741] System will go to power_off state in approx. 2 secs
[ 32.139994]
[ 32.141575] =
[ 32.146564] [ BUG: lock held at task exit time! ]
[ 32.151522] 3.7.0-rc7-00015-g3f8bbe0 #32 Not tainted
[ 32.156721] -
[ 32.161676] init/622 is exiting with locks still held!
[ 32.167085] 1 lock held by init/622:
[ 32.170831] #0: (reboot_mutex){+.+...}, at: [c0056fb8]
sys_reboot+0xf4/0x1e8
[ 32.178679]
[ 32.178679] stack backtrace:
[ 32.183305] [c001af24] (unwind_backtrace+0x0/0xf0) from [c0046978]
(do_exit+0x488/0x7e8)
[ 32.192183] [c0046978] (do_exit+0x488/0x7e8) from [c0056fc4]
(sys_reboot+0x100/0x1e8)
[ 32.200797] [c0056fc4] (sys_reboot+0x100/0x1e8) from [c0013320]
(ret_fast_syscall+0x0/0x3c)
=
It is not clear to me where reboot_mutex should have been unlocked before
debug_check_no_locks_held() is called in do_exit()?
Note: In latest linux-next tip I am *not* seeing this error. I have not
bisected yet to see what fixed this. Since with latest linux-next I am
not seeing an issue, I will remove spinlock and the while(1). Hope that
will make the patch more acceptable.
I don't see a problem with that, and I don't see why we need to spin
(without any power saving too) waiting for some event. If we've called
sys_reboot with LINUX_REBOOT_CMD_POWER_OFF, we'd better have already
killed most of userspace off by that time anyway.
I did a search of various power-off implementations in *arch/arm*.
Those that use a while(1):
=
arch/arm/mach-at91/board-gsia18s.c
arch/arm/mach-at91/setup.c doesn't
arch/arm/mach-cns3xxx/cns3420vb.c
arch/arm/mach-iop32x/glantank.c
arch/arm/mach-iop32x/iq31244.c
arch/arm/mach-iop32x/n2100.c local_irq_disable() and then a while(1)
arch/arm/mach-kirkwood/board-lsxl.c
arch/arm/mach-highbank/highbank.c does while(1) cpu_do_idle()
Those that don't have a while(1):
arch/arm/mach-imx/mach-mx31moboard.c
arch/arm/mach-iop32x/em7210.c
Doesn't have a while(1) but setting a gpio so presumably kills the system
immediately:
===
arch/arm/mach-ixp4xx/dsmg600-setup.c
arch/arm/mach-ixp4xx/nas100d-setup.c
arch/arm/mach-ixp4xx/nslu2-setup.c
arch/arm/mach-kirkwood/board-dnskw.c
arch/arm/mach-kirkwood/board-ib62x0.c
arch/arm/mach-kirkwood/board-dnskw.c
arch/arm/mach-kirkwood/board-ib62x0.c
arch/arm/mach-kirkwood/d2net_v2-setup.c
arch/arm/mach-kirkwood/netspace_v2-setup.c
arch/arm/mach-kirkwood/netxbig_v2-setup.c
arch/arm/mach-kirkwood/t5325-setup.c
arch/arm/mach-orion5x/dns323-setup.c
arch/arm/mach-vexpress/v2m.c take a spinlock, not sure when it will shut down
arch/arm/mach-kirkwood/board-ts219.c sends a character to PIC
Re: [rtc-linux] [PATCH v3 1/5] rtc: OMAP: Add system pm_power_off to rtc driver
On Tue, Nov 27, 2012 at 03:42:39PM -0800, Andrew Morton wrote: + /* Do not allow to execute any other task */ + spin_lock_irqsave(lock, flags); + while (1); I suspect this doesn't do what you want it to do. Firstly, please provide adequate code comments here so that code readers do not also need to be mind readers. If you want to stop this CPU dead in its tracks (why?) then local_irq_disable(); while (1) ; /* Note correct code layout */ will do it. But it means that the NMI watchdog (if present) will come along and whack the machine in the head a few seconds later. And this does nothing to stop other CPUs. But not being a mind reader, I'm really at a loss to suggest what should be done here. It's hooking into the pm_power_off hook, which is called from kernel/sys.c via arch code. We will have already stopped all other CPUs at this point. Why there's that while (1) there I don't know; when pm_power_off is not hooked, we don't do anything like that - and what will happen in that case is we'll return all the way back to sys_reboot(), which will call do_exit(0) on us. I don't see a problem with that, and I don't see why we need to spin (without any power saving too) waiting for some event. If we've called sys_reboot with LINUX_REBOOT_CMD_POWER_OFF, we'd better have already killed most of userspace off by that time anyway. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [rtc-linux] [PATCH v3 1/5] rtc: OMAP: Add system pm_power_off to rtc driver
On Tue, 20 Nov 2012 15:18:43 +0530
AnilKumar Ch anilku...@ti.com wrote:
From: Colin Foe-Parker colin.foepar...@logicpd.com
Add system power off control to rtc driver which is the in-charge
of controlling the BeagleBone system power. The power_off routine
can be hooked up to pm_power_off system call.
System power off sequence:-
* Set PMIC STATUS_OFF when PMIC_POWER_EN is pulled low
* Enable PMIC_POWER_EN in rtc module
* Set rtc ALARM2 time
* Enable ALARM2 interrupt
Added while (1); after the above steps to make sure that no other
process acquire cpu. Otherwise we might see an unexpected behaviour
because we are shutting down all the power rails of SoC except RTC.
Signed-off-by: Colin Foe-Parker colin.foepar...@logicpd.com
[anilku...@ti.com: move poweroff additions to rtc driver]
Signed-off-by: AnilKumar Ch anilku...@ti.com
...
+/*
+ * rtc_power_off: Set the pmic power off sequence. The RTC generates
+ * pmic_pwr_enable control, which can be used to control an external
+ * PMIC.
+ */
+static void rtc_power_off(void)
+{
+ u32 val;
+ struct rtc_time tm;
+ spinlock_t lock;
What on earth?
+ unsigned long flags, time;
+
+ spin_lock_init(lock);
+
+ /* Set PMIC power enable */
+ val = readl(rtc_base + OMAP_RTC_PMIC_REG);
+ writel(val | OMAP_RTC_PMIC_POWER_EN_EN, rtc_base + OMAP_RTC_PMIC_REG);
+
+ /* Read rtc time */
+ omap_rtc_read_time(NULL, tm);
+
+ /* Convert Gregorian date to seconds since 01-01-1970 00:00:00 */
+ rtc_tm_to_time(tm, time);
+
+ /* Add shutdown time to the current value */
+ time += SHUTDOWN_TIME_SEC;
+
+ /* Convert seconds since 01-01-1970 00:00:00 to Gregorian date */
+ rtc_time_to_tm(time, tm);
+
+ if (tm2bcd(tm) 0)
+ return;
+
+ pr_info(System will go to power_off state in approx. %d secs\n,
+ SHUTDOWN_TIME_SEC);
+
+ /*
+ * pmic_pwr_enable is controlled by means of ALARM2 event. So here
+ * programming alarm2 expiry time and enabling alarm2 interrupt
+ */
+ rtc_write(tm.tm_sec, OMAP_RTC_ALARM2_SECONDS_REG);
+ rtc_write(tm.tm_min, OMAP_RTC_ALARM2_MINUTES_REG);
+ rtc_write(tm.tm_hour, OMAP_RTC_ALARM2_HOURS_REG);
+ rtc_write(tm.tm_mday, OMAP_RTC_ALARM2_DAYS_REG);
+ rtc_write(tm.tm_mon, OMAP_RTC_ALARM2_MONTHS_REG);
+ rtc_write(tm.tm_year, OMAP_RTC_ALARM2_YEARS_REG);
+
+ /* Enable alarm2 interrupt */
+ val = readl(rtc_base + OMAP_RTC_INTERRUPTS_REG);
+ writel(val | OMAP_RTC_INTERRUPTS_IT_ALARM2,
+ rtc_base + OMAP_RTC_INTERRUPTS_REG);
+
+ /* Do not allow to execute any other task */
+ spin_lock_irqsave(lock, flags);
+ while (1);
I suspect this doesn't do what you want it to do.
Firstly, please provide adequate code comments here so that code
readers do not also need to be mind readers.
If you want to stop this CPU dead in its tracks (why?) then
local_irq_disable();
while (1)
; /* Note correct code layout */
will do it. But it means that the NMI watchdog (if present) will come
along and whack the machine in the head a few seconds later. And this
does nothing to stop other CPUs.
But not being a mind reader, I'm really at a loss to suggest what
should be done here.
+}
...
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
