"Shilimkar, Santosh" <santosh.shilimkar@xxxxxx> writes: > On Sat, Sep 8, 2012 at 3:07 AM, Kevin Hilman > <khilman@xxxxxxxxxxxxxxxxxxx> wrote: >> Hi Neil, >> >> NeilBrown <neilb@xxxxxxx> writes: >> >>> On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" >>> <santosh.shilimkar@xxxxxx> wrote: >>> >>>> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@xxxxxxx> wrote: >>>> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >>>> > <santosh.shilimkar@xxxxxx> wrote: >>> >>>> >> After thinking bit more on this, the problem seems to be coming >>>> >> mainly because the gpio device is runtime suspended bit early than >>>> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >>>> >> was discussed with Rafael at LPC last week. The idea is to move >>>> >> the pm_runtime_enable/disable() calls entirely up to the >>>> >> _late/_early stage of device suspend/resume. >>>> >> Will update this thread once I have further update. >>>> > >>>> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >>>> > the _late callbacks have been called. >>>> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >>>> > that the interrupt needs to be masked in the ->suspend callback. any later >>>> > is too late. >>>> > >>>> Thanks for information about your discussion. Will wait for the patch then. >>>> >>>> Regards >>>> santosh >>> >>> I already sent a patch - that is what started this thread :-) >>> >>> I include it below. >>> You said "The patch doesn't seems to be correct" but didn't expand on why. >>> Do you still think it is not correct? I wouldn't be surprised if there is >>> some case that it doesn't handle quite right, but it seems right to me. >>> >>> Thanks, >>> NeilBrown >>> >>> >>> From: NeilBrown <neilb@xxxxxxx> >>> Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. >>> >>> Current kernel will wake from suspend on an event on any active >>> GPIO even if enable_irq_wake() wasn't called. >>> >>> There are two reasons that the hardware wake-enable bit should be set: >>> >>> 1/ while non-suspended the CPU might go into a deep sleep (off_mode) >>> in which the wake-enable bit is needed for an interrupt to be >>> recognised. >>> 2/ while suspended the GPIO interrupt should wake from suspend if and >>> only if irq_wake as been enabled. >>> >>> The code currently doesn't keep these two reasons separate so they get >>> confused and sometimes the wakeup flags is set incorrectly. >>> >>> This patch reverts: >>> commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc >>> gpio/omap: remove suspend/resume callbacks >>> and >>> commit 0aa2727399c0b78225021413022c164cb99fbc5e >>> gpio/omap: remove suspend_wakeup field from struct gpio_bank >>> >>> and makes some minor changes so that we have separate flags for "GPIO >>> should wake from deep idle" and "GPIO should wake from suspend". >>> >>> With this patch, the GPIO from my touch screen doesn't wake my device >>> any more, which is what I want. >> >> I think the direction is right here. We never should've separated the >> handling of idle vs suspend wakeups. However, I have a few >> questions/doubts below... >> > I thought irq_set_wake() is suspend only wakeup functionality. In idle, the > driver IRQs are not disabled/masked so there no need of any special > wakeup calls for idle. More ever patch is adding the suspend hooks > for wakeups. > > I have no objection on the subject patch, but the suspend wakeup > facility is easy enough to implement for IRQCHIPS and that is > what I was proposing it. Infact the mask on suspend patch almost > works and it fails only because the GPIO driver is suspended earlier > than the IRQ framework expect it to be. That is a pretty big problem to overcome. :) That being said, I don't see how simply using MASK_ON_SUSPEND can work for GPIO. AFAICT, that flag is for the whole irq_chip, not for individual IRQs. We really need to keep track at the bank/IRQ level, as in the proposed patch from Neil (actually, we used to have this featur, but I screwed up by not catching this removal when reviewing the GPIO cleanup/reorg series.) Because of retention/off in idle, we set *all* GPIOs with IRQ triggering to be wakeup enabled so they will cause wakeups during idle. During suspend, we only want the irq_set_wake() ones to cause wakeups. > Anyways I step back here since the proposed patch already fixes > the issue seen. Assuming the IRQCHIP mask on suspend doesn't > seems to work well with drivers as Neil mentioned, the $subject patch > seems to be the right option. OK thanks, I'll queue this up for v3.6-rc as this should qualify as a regression. Also, the IRQCHIP mask feature seems to have been designed for IRQ chips without the control registers to handle this. We have the control registers to handle it, so I believe it's better to keep this handled in the driver itself. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
