| Message ID | 1242861576-13008-2-git-send-email-khilman@deeprootsystems.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
Kevin Hilman wrote: > This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. > > Adding a disable hook to the irq_chip is not the way to fix the > problem being addressed by this patch. Instead, we need to fix > support for [enable|disable]_irq_wake(). Agree with you if we can use disable_irq_wake for MPU Interrupt with not masking the IRQ. Can you explain how we can fix support for disable_irq_wake() for omap_irq_chip? > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> > --- > arch/arm/mach-omap2/irq.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c > index 998c5c4..08a3b99 100644 > --- a/arch/arm/mach-omap2/irq.c > +++ b/arch/arm/mach-omap2/irq.c > @@ -134,7 +134,6 @@ static struct irq_chip omap_irq_chip = { > .ack = omap_mask_ack_irq, > .mask = omap_mask_irq, > .unmask = omap_unmask_irq, > - .disable = omap_mask_irq, > }; > > static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kim Kyuwon <q1.kim@samsung.com> writes: > Kevin Hilman wrote: >> This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. >> >> Adding a disable hook to the irq_chip is not the way to fix the >> problem being addressed by this patch. Instead, we need to fix >> support for [enable|disable]_irq_wake(). > > Agree with you if we can use disable_irq_wake for MPU Interrupt with > not masking the IRQ. Can you explain how we can fix support for > disable_irq_wake() for omap_irq_chip? Yes. The PRCM has a wake-enable per device bit that can be set (see PM_WKEN_<pwrdm>) to control device wakeup enables. But the implemenation of that should not hold up this revert because this patch breaks *all* wakeups since the PRCM interrupt itself is disabled in the suspend path. As a workaround for your USB problem that this patch was initially intended to fix you could manually disable USB OTG wakeups like this: wken = prm_read_mod_reg(CORE_MOD, PM_WKEN); wken &= ~OMAP3430_EN_HSOTGUSB; prm_write_mod_reg(wken, CORE_MOD, PM_WKEN); At least until the irq_wake stuff is fixed. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 22, 2009 at 11:54 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Kim Kyuwon <q1.kim@samsung.com> writes: > >> Kevin Hilman wrote: >>> This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. >>> >>> Adding a disable hook to the irq_chip is not the way to fix the >>> problem being addressed by this patch. Â Instead, we need to fix >>> support for [enable|disable]_irq_wake(). >> >> Agree with you if we can use disable_irq_wake for MPU Interrupt with >> not masking the IRQ. Can you explain how we can fix support for >> disable_irq_wake() for omap_irq_chip? > > Yes. Â The PRCM has a wake-enable per device bit that can be set (see > PM_WKEN_<pwrdm>) to control device wakeup enables. PM_WKEN_<pwrdm> is not entirely matched to each MPU interrupt. If you want to use disable_irq_wake() with enabling PRCM_Interrupt, you should disable all PM_WKEN_<pwrdm> bits. And how can you make support of disable_irq_wake() for other MPU interrupts? > But the implemenation of that should not hold up this revert because > this patch breaks *all* wakeups since the PRCM interrupt itself is > disabled in the suspend path. Yes, I saw the same problem. This is caused by resume_device_irqs() recently added by Rafael not by my patch. I'm asking him that he made a right decision. > As a workaround for your USB problem that this patch was initially > intended to fix you could manually disable USB OTG wakeups like this: > > Â Â Â Â wken = prm_read_mod_reg(CORE_MOD, PM_WKEN); > Â Â Â Â wken &= ~OMAP3430_EN_HSOTGUSB; > Â Â Â Â prm_write_mod_reg(wken, CORE_MOD, PM_WKEN); Did you checked this masking prevent waking up from the interrupt of USB HOST? Regards, Kyuwon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kim Kyuwon <chammoru@gmail.com> writes: > On Fri, May 22, 2009 at 11:54 PM, Kevin Hilman > <khilman@deeprootsystems.com> wrote: >> Kim Kyuwon <q1.kim@samsung.com> writes: >> >>> Kevin Hilman wrote: >>>> This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. >>>> >>>> Adding a disable hook to the irq_chip is not the way to fix the >>>> problem being addressed by this patch. Â Instead, we need to fix >>>> support for [enable|disable]_irq_wake(). >>> >>> Agree with you if we can use disable_irq_wake for MPU Interrupt with >>> not masking the IRQ. Can you explain how we can fix support for >>> disable_irq_wake() for omap_irq_chip? >> >> Yes. Â The PRCM has a wake-enable per device bit that can be set (see >> PM_WKEN_<pwrdm>) to control device wakeup enables. > > PM_WKEN_<pwrdm> is not entirely matched to each MPU interrupt. Correct. This bit disables the module from generating any wakeup event to the PRCM. > If you want to use disable_irq_wake() with enabling PRCM_Interrupt, > you should disable all PM_WKEN_<pwrdm> bits. > And how can you make support of disable_irq_wake() for other MPU interrupts? To control the ability of a module to wake the MPU directly, we would need to use the PM_MPUGRPSEL_<pwrdm> regs. >> But the implemenation of that should not hold up this revert because >> this patch breaks *all* wakeups since the PRCM interrupt itself is >> disabled in the suspend path. > > Yes, I saw the same problem. This is caused by resume_device_irqs() > recently added by Rafael not by my patch. The point is, with your patch applied, *all* OMAP wakeups are broken upstream. You're right, your patch didn't cause the broken wakeup problem by itself, but your patch on top of Rafael's in combination with the new lazy-disable support which are both already in mainline breaks *all* OMAP wakeup capabilities. Therefore it should be reverted and the OMAP specific IRQ wake support fixed. I am working on fixing the OMAP IRQ wake support, but I do not want that to hold up this series getting upstream. > I'm asking him that he made a right decision. Yes, we had a long discussion on that on linux-pm and I came to the conclusion that while his change was probably premature, it's the right decision and platforms with wakeup capabilities should use/fix their set_irq_wakeup() functionality. >> As a workaround for your USB problem that this patch was initially >> intended to fix you could manually disable USB OTG wakeups like this: >> >> Â Â Â Â wken = prm_read_mod_reg(CORE_MOD, PM_WKEN); >> Â Â Â Â wken &= ~OMAP3430_EN_HSOTGUSB; >> Â Â Â Â prm_write_mod_reg(wken, CORE_MOD, PM_WKEN); > > Did you checked this masking prevent waking up from the interrupt of USB HOST? No I did not test, nor was I able to reproduce your original problem since the description wasn't that clear to me. This will disable the USB OTG controller module from generating wakeups for any reason. If disabling the device wakeup in PM_WKEN is still resutling in interrupts, then the powerdomain with that device is most likely not in retention/off. I do know that disabling PM_WKEN for UARTs in the same powerdomain as USB OTG (CORE) will stop the UART from waking, and thus from generating interrupts, as long as the powerdomain has actually transitioned to retention/off. Re: USB HOST. The problem you reported in your original patch was that you were waking from IRQ 92, which is the USB OTG interrupt. If your problem is with USBHOST, that is in a different powerdomain. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 23, 2009 at 3:16 AM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Kim Kyuwon <chammoru@gmail.com> writes: > >> On Fri, May 22, 2009 at 11:54 PM, Kevin Hilman >> <khilman@deeprootsystems.com> wrote: >>> Kim Kyuwon <q1.kim@samsung.com> writes: >>> >>>> Kevin Hilman wrote: >>>>> This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. >>>>> >>>>> Adding a disable hook to the irq_chip is not the way to fix the >>>>> problem being addressed by this patch. Â Instead, we need to fix >>>>> support for [enable|disable]_irq_wake(). >>>> >>>> Agree with you if we can use disable_irq_wake for MPU Interrupt with >>>> not masking the IRQ. Can you explain how we can fix support for >>>> disable_irq_wake() for omap_irq_chip? >>> >>> Yes. Â The PRCM has a wake-enable per device bit that can be set (see >>> PM_WKEN_<pwrdm>) to control device wakeup enables. >> >> PM_WKEN_<pwrdm> is not entirely matched to each MPU interrupt. > > Correct. Â This bit disables the module from generating any wakeup > event to the PRCM. > >> If you want to use disable_irq_wake() with enabling PRCM_Interrupt, >> you should disable all PM_WKEN_<pwrdm> bits. >> And how can you make support of disable_irq_wake() for other MPU interrupts? > > To control the ability of a module to wake the MPU directly, we would > need to use the PM_MPUGRPSEL_<pwrdm> regs. Sorry but I can't believe until it is implemented in OMAP and I can confirm it. >>> But the implemenation of that should not hold up this revert because >>> this patch breaks *all* wakeups since the PRCM interrupt itself is >>> disabled in the suspend path. >> >> Yes, I saw the same problem. This is caused by resume_device_irqs() >> recently added by Rafael not by my patch. > > The point is, with your patch applied, *all* OMAP wakeups are broken > upstream. > > You're right, your patch didn't cause the broken wakeup problem by > itself, but your patch on top of Rafael's in combination with the new > lazy-disable support which are both already in mainline breaks *all* > OMAP wakeup capabilities. Â Therefore it should be reverted and the > OMAP specific IRQ wake support fixed. > > I am working on fixing the OMAP IRQ wake support, but I do not want > that to hold up this series getting upstream. However, we don't know yet OMAP IRQ wake support is well implemented or not. >> I'm asking him that he made a right decision. > > Yes, we had a long discussion on that on linux-pm and I came to the > conclusion that while his change was probably premature, it's the > right decision and platforms with wakeup capabilities should > use/fix their set_irq_wakeup() functionality. I know... I jumped into that discussion. But you insisted as following... -- > Interrupt wakeups can be disabled at the PRCM level. Or more simply > we can keep a mask of wakeup-enable interrupts and use that. > I will experiment with getting disable_irq_wake() working. -- So I quited. But I think we should have this discussion again. >>> As a workaround for your USB problem that this patch was initially >>> intended to fix you could manually disable USB OTG wakeups like this: >>> >>> Â Â Â Â wken = prm_read_mod_reg(CORE_MOD, PM_WKEN); >>> Â Â Â Â wken &= ~OMAP3430_EN_HSOTGUSB; >>> Â Â Â Â prm_write_mod_reg(wken, CORE_MOD, PM_WKEN); >> >> Did you checked this masking prevent waking up from the interrupt of USB HOST? > > No I did not test, nor was I able to reproduce your original problem > since the description wasn't that clear to me. > > This will disable the USB OTG controller module from generating > wakeups for any reason. Â If disabling the device wakeup in PM_WKEN is > still resutling in interrupts, then the powerdomain with that device > is most likely not in retention/off. > > I do know that disabling PM_WKEN for UARTs in the same powerdomain as > USB OTG (CORE) will stop the UART from waking, and thus from > generating interrupts, as long as the powerdomain has actually > transitioned to retention/off. > > Re: USB HOST. Â The problem you reported in your original patch was > that you were waking from IRQ 92, which is the USB OTG interrupt. Â If > your problem is with USBHOST, that is in a different powerdomain. OK I will check it when I'm in my office. Now it's great weekend. Have a good weekend!. Regards, Kyuwon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 22, 2009 at 07:54:32AM -0700, Kevin Hilman wrote: > But the implemenation of that should not hold up this revert because > this patch breaks *all* wakeups since the PRCM interrupt itself is > disabled in the suspend path. That is not mentioned in the patch description (and it should be). What this paragraph is saying is that this revert is most definitely fixing a regression. Any "fix" which causes other breakage is not a fix, and therefore this revert needs to go in no matter what. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 23, 2009 at 8:22 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, May 22, 2009 at 07:54:32AM -0700, Kevin Hilman wrote: >> But the implemenation of that should not hold up this revert because >> this patch breaks *all* wakeups since the PRCM interrupt itself is >> disabled in the suspend path. > > That is not mentioned in the patch description (and it should be). > What this paragraph is saying is that this revert is most definitely > fixing a regression. > > Any "fix" which causes other breakage is not a fix, and therefore > this revert needs to go in no matter what. The problem is that this patch(which is written by me) was merged much earlier than Rafael's patch. So I think Rafael's patch causes other breakage. > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Sat, May 23, 2009 at 3:16 AM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Kim Kyuwon <chammoru@gmail.com> writes: > >> On Fri, May 22, 2009 at 11:54 PM, Kevin Hilman >> <khilman@deeprootsystems.com> wrote: >>> Kim Kyuwon <q1.kim@samsung.com> writes: >>> >>>> Kevin Hilman wrote: >>>>> This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. >>>>> >>>>> Adding a disable hook to the irq_chip is not the way to fix the >>>>> problem being addressed by this patch. Â Instead, we need to fix >>>>> support for [enable|disable]_irq_wake(). >>>> >>>> Agree with you if we can use disable_irq_wake for MPU Interrupt with >>>> not masking the IRQ. Can you explain how we can fix support for >>>> disable_irq_wake() for omap_irq_chip? >>> >>> Yes. Â The PRCM has a wake-enable per device bit that can be set (see >>> PM_WKEN_<pwrdm>) to control device wakeup enables. >> >> PM_WKEN_<pwrdm> is not entirely matched to each MPU interrupt. > > Correct. Â This bit disables the module from generating any wakeup > event to the PRCM. > >> If you want to use disable_irq_wake() with enabling PRCM_Interrupt, >> you should disable all PM_WKEN_<pwrdm> bits. >> And how can you make support of disable_irq_wake() for other MPU interrupts? > > To control the ability of a module to wake the MPU directly, we would > need to use the PM_MPUGRPSEL_<pwrdm> regs. PM_MPUGRPSEL_<pwrdm> regs can't disable irq wakeup, it is just related wake-up dependencies. I tested with this code: prm_write_mod_reg(0, WKUP_MOD, OMAP3430_PM_MPUGRPSEL); prm_write_mod_reg(0, MPU_MOD, OMAP3430_PM_MPUGRPSEL); prm_write_mod_reg(0, CORE_MOD, OMAP3430_PM_MPUGRPSEL); prm_write_mod_reg(0, OMAP3430_PER_MOD, OMAP3430_PM_MPUGRPSEL); But USB interrupt have continuously woken up the system. I checked with my omap wakeup source driver and wake-up source was shown below # cat /sys/power/omap_resume_irq 92 >>> But the implemenation of that should not hold up this revert because >>> this patch breaks *all* wakeups since the PRCM interrupt itself is >>> disabled in the suspend path. >> >> Yes, I saw the same problem. This is caused by resume_device_irqs() >> recently added by Rafael not by my patch. > > The point is, with your patch applied, *all* OMAP wakeups are broken > upstream. > > You're right, your patch didn't cause the broken wakeup problem by > itself, but your patch on top of Rafael's in combination with the new > lazy-disable support which are both already in mainline breaks *all* > OMAP wakeup capabilities. Â Therefore it should be reverted and the > OMAP specific IRQ wake support fixed. > > I am working on fixing the OMAP IRQ wake support, but I do not want > that to hold up this series getting upstream. > >> I'm asking him that he made a right decision. > > Yes, we had a long discussion on that on linux-pm and I came to the > conclusion that while his change was probably premature, it's the > right decision and platforms with wakeup capabilities should > use/fix their set_irq_wakeup() functionality. > >>> As a workaround for your USB problem that this patch was initially >>> intended to fix you could manually disable USB OTG wakeups like this: >>> >>> Â Â Â Â wken = prm_read_mod_reg(CORE_MOD, PM_WKEN); >>> Â Â Â Â wken &= ~OMAP3430_EN_HSOTGUSB; >>> Â Â Â Â prm_write_mod_reg(wken, CORE_MOD, PM_WKEN); I also confirmed this code can't help disabling interrupt wakeup. >> Did you checked this masking prevent waking up from the interrupt of USB HOST? > > No I did not test, nor was I able to reproduce your original problem > since the description wasn't that clear to me. > > This will disable the USB OTG controller module from generating > wakeups for any reason. Â If disabling the device wakeup in PM_WKEN is > still resutling in interrupts, then the powerdomain with that device > is most likely not in retention/off. > > I do know that disabling PM_WKEN for UARTs in the same powerdomain as > USB OTG (CORE) will stop the UART from waking, and thus from > generating interrupts, as long as the powerdomain has actually > transitioned to retention/off. > > Re: USB HOST. Â The problem you reported in your original patch was > that you were waking from IRQ 92, which is the USB OTG interrupt. Â If > your problem is with USBHOST, that is in a different powerdomain. > > Kevin > > Kevin, I'm terribly sorry to hold up your work. I will not disturb you anymore in relation to this revert patch. However, please keep in mind that adding a disable hook to the irq_chip was the best way to fix the problem addressed by my original patch. Regards, Kyuwon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c index 998c5c4..08a3b99 100644 --- a/arch/arm/mach-omap2/irq.c +++ b/arch/arm/mach-omap2/irq.c @@ -134,7 +134,6 @@ static struct irq_chip omap_irq_chip = { .ack = omap_mask_ack_irq, .mask = omap_mask_irq, .unmask = omap_unmask_irq, - .disable = omap_mask_irq, }; static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
This reverts commit 5461af5af5c6a7fee78978aafe720541bf3a2f55. Adding a disable hook to the irq_chip is not the way to fix the problem being addressed by this patch. Instead, we need to fix support for [enable|disable]_irq_wake(). Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> --- arch/arm/mach-omap2/irq.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)