From mboxrd@z Thu Jan 1 00:00:00 1970 From: haojian.zhuang@gmail.com (Haojian Zhuang) Date: Thu, 27 Feb 2014 10:19:41 +0800 Subject: [patch 09/26] arm: mmp: Remove pointless fiddling with irq internals In-Reply-To: References: <20140223212703.511977310@linutronix.de> <20140223212737.214342433@linutronix.de> <20140223231755.GA27579@pengutronix.de> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie wrote: > On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner wrote: >> On Mon, 24 Feb 2014, Haojian Zhuang wrote: >> >>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie wrote: >>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig >>> > wrote: >>> >> Hi Thomas, >>> >> >>> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote: >>> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake >>> >>> callbacks fiddle pointlessly with the irq actions for no reason except >>> >>> for lack of understanding how the wakeup mechanism works. >>> >>> >>> >>> On supsend the core disables all interrupts lazily, i.e. it does not >>> >>> mask them at the irq controller level. So any interrupt which is >>> >>> firing during supsend will mark the corresponding interrupt line as >>> >> s/supsend/suspend/ twice >>> >>> pending. Just before the core powers down it checks whether there are >>> >>> interrupts pending from interrupt lines which are marked as wakeup >>> >>> sources and if so it aborts the resume and resends the interrupts. >>> >> It's the suspend that is aborted, not the resume. >>> >> >>> >> Other than that your change looks fine. >>> >> >>> > For pxa910 and MMP2, wake up source only wake up the AP subsystem. >>> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores. >>> > Now the core is still powered down. APMU will check the interrupt >>> > lines, and find >>> > that there are interrupt pending, it will power on the cores. >>> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the >>> > core is still powered down. It will not be powered up by APMU. >>> > >>> >>> Yes, suspend/resume can't work if the above code is removed. >>> >>> Interrupt source (logic AND with interrupt mask register) is connected >>> to MPMU as >>> wakeup source. If the interrupt is disabled, there's no wakeup source event. >>> >>> And APMU is waken up by MPMU. >>> >>> So please don't remove the above code. We must keep these interrupt lines active >>> to wake up the whole system. >> >> They are kept active at the interrupt controller level. You just >> refuse to understand how the internals of the interrupt subsystem >> work. >> > If no irq_disable callback is hooked, when do irq_disable, it will not > actually disable > the interrupt, it will depend on next time when the irq happens, the > handler will first mask > the interrupt as this interrupt never happens. > So after system suspended, the interrupt happens, but the device > driver will not recieve this interrupt > because it is masked. > It results in that the device driver miss a important interrupt which > related to something need to be > handled. If user application for example android has power managment > daemon. It will find that nothing > to handle, it will make the system enter suspend again. > Let me list the logic to make it easier to understand. suspend_enter() --> dpm_suspend_end() --> dpm_suspend_noirq() --> suspend_device_irqs() --> __disable_irq() --> set IRQS_SUSPENDED && call irq_disable() if necessary --> syscore_suspend() --> check_wakeup_irqs() If there's no pending irq in suspend process && IRQS_SUSPENDED is set, then mask the irq. Yes, we didn't implement disable_irq(). But we must implement mask_irq(). So system suspends. Then system will never be waken up by this irq any more since it's masked. >> And even if you would need this flag, then fiddling with the irq desc >> internals is a big NONO. There is a proper way to hand that in. >> > > So can you suggest the proper way to handle it? Thanks. > >> Thanks, >> >> tglx >>