| Message ID | 557AB00C.5040606@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12.06.2015 19:10, Sudeep Holla wrote: > > > On 12/06/15 06:43, Javier Martinez Canillas wrote: >> The Exynos interrupt combiner IP loses its state when the SoC enters >> into a low power state during a Suspend-to-RAM. This means that if a >> IRQ is used as a source, the interrupts for the devices are disabled >> when the system is resumed from a sleep state so are not triggered. >> >> Save the interrupt enable set register for each combiner group and >> restore it after resume to make sure that the interrupts are enabled. >> > > Not sure if you need this. IMO it's not clean and redundant though I > admit many drivers do exactly same thing. I am trying to remove or point > out those redundant code as irqchip core has options/flags to do what > you need. I assume there are no wakeup sources connected to this > combiner. It may have wake up sources connected. Correct me if I am wrong but (at least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be external interrupts (e.g. power key on Exynos5250 Snow). I didn't check other boards. Best regards, Krzysztof > Setting irqchip flags should solve this problem. A simple > patch below should do the job ? > > -->8 > > diff --git a/drivers/irqchip/exynos-combiner.c > b/drivers/irqchip/exynos-combiner.c > index 5945223b73fa..c0bcec59f829 100644 > --- a/drivers/irqchip/exynos-combiner.c > +++ b/drivers/irqchip/exynos-combiner.c > @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = { > #ifdef CONFIG_SMP > .irq_set_affinity = combiner_set_affinity, > #endif > + .flags = IRQCHIP_MASK_ON_SUSPEND, > }; > > >
On 12/06/15 11:42, Krzysztof Kozlowski wrote: > On 12.06.2015 19:10, Sudeep Holla wrote: >> >> >> On 12/06/15 06:43, Javier Martinez Canillas wrote: >>> The Exynos interrupt combiner IP loses its state when the SoC enters >>> into a low power state during a Suspend-to-RAM. This means that if a >>> IRQ is used as a source, the interrupts for the devices are disabled >>> when the system is resumed from a sleep state so are not triggered. >>> >>> Save the interrupt enable set register for each combiner group and >>> restore it after resume to make sure that the interrupts are enabled. >>> >> >> Not sure if you need this. IMO it's not clean and redundant though I >> admit many drivers do exactly same thing. I am trying to remove or point >> out those redundant code as irqchip core has options/flags to do what >> you need. I assume there are no wakeup sources connected to this >> combiner. > > It may have wake up sources connected. Correct me if I am wrong but (at > least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be > external interrupts (e.g. power key on Exynos5250 Snow). I didn't check > other boards. > In that case, this irqchip should implement irq_set_wake and the driver implementing power key should use enable_irq_wake in the suspend path. Just saving all the mask/enable registers is not scalable solution and also useless if it's just one or to interrupts that are very few IRQs registered/actively being used. Regards, Sudeep
Hello Sudeep, Thanks a lot for the feedback. On 06/12/2015 12:10 PM, Sudeep Holla wrote: > > > On 12/06/15 06:43, Javier Martinez Canillas wrote: >> The Exynos interrupt combiner IP loses its state when the SoC enters >> into a low power state during a Suspend-to-RAM. This means that if a >> IRQ is used as a source, the interrupts for the devices are disabled >> when the system is resumed from a sleep state so are not triggered. >> >> Save the interrupt enable set register for each combiner group and >> restore it after resume to make sure that the interrupts are enabled. >> > > Not sure if you need this. IMO it's not clean and redundant though I > admit many drivers do exactly same thing. I am trying to remove or point > out those redundant code as irqchip core has options/flags to do what > you need. I assume there are no wakeup sources connected to this Yes, there are wakeup sources connected to this combiner. As Krzysztof said some wakeup sources use a GPIO line as IRQ whose interrupt parent is the combiner. As an example the Power gpio-key and cros_ec keyboard for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi. Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the right thing though and call {enable,disable}_irq_wake() on the S2R path. And in fact, the peripherals resume the system after a suspend but after resume, interrupts are not triggered anymore. > combiner. Setting irqchip flags should solve this problem. A simple > patch below should do the job ? > I don't see how the below patch is going to work for the case I'm trying to solve. If I understand correctly, the IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup interrupt in the suspend path (instead of just disabling the interrupts on suspend and not masking at the hardware level) But my problem is not about interrupts needed to be masked on suspend but the opposite, that interrupts have to be unmasked on resume since the IP loses its state so after a resume, interrupts are not triggered anymore. So I also don't see how implementing irq_set_wake() as you suggested is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for this case? > -->8 > > diff --git a/drivers/irqchip/exynos-combiner.c > b/drivers/irqchip/exynos-combiner.c > index 5945223b73fa..c0bcec59f829 100644 > --- a/drivers/irqchip/exynos-combiner.c > +++ b/drivers/irqchip/exynos-combiner.c > @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = { > #ifdef CONFIG_SMP > .irq_set_affinity = combiner_set_affinity, > #endif > + .flags = IRQCHIP_MASK_ON_SUSPEND, > }; > > Best regards, Javier
On 12/06/15 12:27, Javier Martinez Canillas wrote: > Hello Sudeep, > > Thanks a lot for the feedback. > > On 06/12/2015 12:10 PM, Sudeep Holla wrote: >> >> >> On 12/06/15 06:43, Javier Martinez Canillas wrote: >>> The Exynos interrupt combiner IP loses its state when the SoC enters >>> into a low power state during a Suspend-to-RAM. This means that if a >>> IRQ is used as a source, the interrupts for the devices are disabled >>> when the system is resumed from a sleep state so are not triggered. >>> >>> Save the interrupt enable set register for each combiner group and >>> restore it after resume to make sure that the interrupts are enabled. >>> >> >> Not sure if you need this. IMO it's not clean and redundant though I >> admit many drivers do exactly same thing. I am trying to remove or point >> out those redundant code as irqchip core has options/flags to do what >> you need. I assume there are no wakeup sources connected to this > > Yes, there are wakeup sources connected to this combiner. As Krzysztof > said some wakeup sources use a GPIO line as IRQ whose interrupt parent > is the combiner. As an example the Power gpio-key and cros_ec keyboard > for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi. > > Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the > right thing though and call {enable,disable}_irq_wake() on the S2R path. > > And in fact, the peripherals resume the system after a suspend but after > resume, interrupts are not triggered anymore. > I agree for the wakeup source, but I need to go through the irqchip core again to understand this better. >> combiner. Setting irqchip flags should solve this problem. A >> simple patch below should do the job ? >> > > I don't see how the below patch is going to work for the case I'm > trying to solve. If I understand correctly, the > IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup > interrupt in the suspend path (instead of just disabling the > interrupts on suspend and not masking at the hardware level) > It also takes re-enables all the IRQs in the resume path that were disabled on the suspend path. > But my problem is not about interrupts needed to be masked on suspend > but the opposite, that interrupts have to be unmasked on resume since > the IP loses its state so after a resume, interrupts are not > triggered anymore. > As I mentioned above this happens for all non-wake up interrupts. However this needs to done for wake up interrupts IIUC. BTW if these registers are lost assuming the combiner was powered down, even the status register will be lost and you will not know exactly the wakeup reason right ? > So I also don't see how implementing irq_set_wake() as you suggested > is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for > this case? > IIRC these combiner feeds to main interrupt controller and you need to make sure that interrupt is set as wakeup if required. Right ? so you may still need irq_set_wake IMO. Regards, Sudeep
Hello Sudeep, On 06/12/2015 01:54 PM, Sudeep Holla wrote: > > > On 12/06/15 12:27, Javier Martinez Canillas wrote: >> Hello Sudeep, >> >> Thanks a lot for the feedback. >> >> On 06/12/2015 12:10 PM, Sudeep Holla wrote: >>> >>> >>> On 12/06/15 06:43, Javier Martinez Canillas wrote: >>>> The Exynos interrupt combiner IP loses its state when the SoC enters >>>> into a low power state during a Suspend-to-RAM. This means that if a >>>> IRQ is used as a source, the interrupts for the devices are disabled >>>> when the system is resumed from a sleep state so are not triggered. >>>> >>>> Save the interrupt enable set register for each combiner group and >>>> restore it after resume to make sure that the interrupts are enabled. >>>> >>> >>> Not sure if you need this. IMO it's not clean and redundant though I >>> admit many drivers do exactly same thing. I am trying to remove or point >>> out those redundant code as irqchip core has options/flags to do what >>> you need. I assume there are no wakeup sources connected to this >> >> Yes, there are wakeup sources connected to this combiner. As Krzysztof >> said some wakeup sources use a GPIO line as IRQ whose interrupt parent >> is the combiner. As an example the Power gpio-key and cros_ec keyboard >> for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi. >> >> Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the >> right thing though and call {enable,disable}_irq_wake() on the S2R path. >> >> And in fact, the peripherals resume the system after a suspend but after >> resume, interrupts are not triggered anymore. >> > > I agree for the wakeup source, but I need to go through the irqchip core > again to understand this better. > >>> combiner. Setting irqchip flags should solve this problem. A >>> simple patch below should do the job ? >>> >> >> I don't see how the below patch is going to work for the case I'm >> trying to solve. If I understand correctly, the >> IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup >> interrupt in the suspend path (instead of just disabling the >> interrupts on suspend and not masking at the hardware level) >> > > It also takes re-enables all the IRQs in the resume path that were > disabled on the suspend path. > Ok, I only saw that a call to mask_irq was made in suspend_device_irq [0] if IRQCHIP_MASK_ON_SUSPEND was set but I didn't see the inverse operation in the resume path. But now looking at irq_enable() I see that the IRQ is unmasked if it was disabled so that's why is not needed. >> But my problem is not about interrupts needed to be masked on suspend >> but the opposite, that interrupts have to be unmasked on resume since >> the IP loses its state so after a resume, interrupts are not >> triggered anymore. >> > > As I mentioned above this happens for all non-wake up interrupts. > However this needs to done for wake up interrupts IIUC. BTW if these Well both interrupts that are a wakeup source and those that are not, don't work after a resume. IOW, all the interrupts whose source is the combiner are not working. > registers are lost assuming the combiner was powered down, even the > status register will be lost and you will not know exactly the wakeup > reason right ? > Good question, I didn't find in the documentation I've access to that this happen but just found through experimentation that the IRQ enable set register values are lost after a resume and that saving/restoring the values makes the interrupts to be triggered again. >> So I also don't see how implementing irq_set_wake() as you suggested >> is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for >> this case? >> > > IIRC these combiner feeds to main interrupt controller and you need to > make sure that interrupt is set as wakeup if required. Right ? so you > may still need irq_set_wake IMO. > Yes, I agree that is good to have a irq_set_wake callback, what is still not clear to me is if is needed to solve this particular issue or not. > Regards, > Sudeep > Best regards, Javier [0]: http://lxr.free-electrons.com/source/kernel/irq/pm.c#L90
Hello Sudeep, On 06/12/2015 02:57 PM, Javier Martinez Canillas wrote: > On 06/12/2015 01:54 PM, Sudeep Holla wrote: [snip] > >> registers are lost assuming the combiner was powered down, even the >> status register will be lost and you will not know exactly the wakeup >> reason right ? >> > > Good question, I didn't find in the documentation I've access to that > this happen but just found through experimentation that the IRQ enable > set register values are lost after a resume and that saving/restoring > the values makes the interrupts to be triggered again. > I'll share here too the findings I mentioned over IRC. As you suggested I add some printouts and noticed that the ISTRn (Interrupt Status) registers values are indeed preserved on resume so I can know for example if the wakeup source was the power gpio-key or cros_ec keyboard. I've checked the values of the registers against the Exynos manual and they corresponds to the interrupt sources in each case so the values are correct. So as you said, it seems that is not that the IP block loses its state on S2R but that something is blindly writing the IESRn (Interrupt Enable Set) registers. To reduce the possible s/w components that could be doing this, I booted a signed FIT image directly using the RO U-Boot instead of chain loading a mainline nv-uboot. In this configuration I've the same issue so it seems that if something is zeroing those registers on S2R, this can't be changed without void the warranty of these machines. I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that they have a very similar solution than my patch, the IESRn are also saved but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead or registering a syscore ops but the idea is basically the same. I have to take a look to the U-boot that is shipped on the device, I think the correct branch is [1] but I'm not sure if that is the correct one. Best regards, Javier [0]: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/arch/arm/mach-exynos/common.c#657 [1]: https://chromium.googlesource.com/chromiumos/third_party/u-boot firmware-pit-4482.B
Hi, On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >>> registers are lost assuming the combiner was powered down, even the >>> status register will be lost and you will not know exactly the wakeup >>> reason right ? >>> >> >> Good question, I didn't find in the documentation I've access to that >> this happen but just found through experimentation that the IRQ enable >> set register values are lost after a resume and that saving/restoring >> the values makes the interrupts to be triggered again. >> > > I'll share here too the findings I mentioned over IRC. As you suggested I > add some printouts and noticed that the ISTRn (Interrupt Status) registers > values are indeed preserved on resume so I can know for example if the > wakeup source was the power gpio-key or cros_ec keyboard. I've checked the > values of the registers against the Exynos manual and they corresponds to > the interrupt sources in each case so the values are correct. > > So as you said, it seems that is not that the IP block loses its state on > S2R but that something is blindly writing the IESRn (Interrupt Enable Set) > registers. I'll postulate an alternate theory here. You can tell me if you buy it or if you think I've been out in the sun too long. Let's say that the interrupt combiner's status registers show the raw status as asserted by whatever is hooked up to the combiner. This means that even if the combiner got reset we could still read the status register and get the status of the source. Imagine that the combiner is like a GPIO bank. If you reset the GPIO bank, you'll lose all kinds of config (input vs. output, edge interrupt status, maybe pulls, etc). ...but you can still read the state asserted by an external source, right? In this case the combiner's interrupt source is 'EINT 11'. ...and I'm pretty sure that the controller managine 'EINT 11' _doesn't_ lose power across suspend/resume... I'll further bolster my theory by saying that _almost nothing_ in the exynos keeps power across suspend/resume. The UART? Nope. The GPIO controllers? Nuh-uh. The GIC? Sorry, but no. The clock tree? It might be nice, but you're out of luck. ...so it would make me terribly surprised to see the combiner keep power. > To reduce the possible s/w components that could be doing this, I booted a > signed FIT image directly using the RO U-Boot instead of chain loading a > mainline nv-uboot. In this configuration I've the same issue so it seems > that if something is zeroing those registers on S2R, this can't be changed > without void the warranty of these machines. > > I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that > they have a very similar solution than my patch, the IESRn are also saved > but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead > or registering a syscore ops but the idea is basically the same. Yup, you can see where kliegs added it in <https://chromium-review.googlesource.com/#/c/27964/>. As per the comments in that CL, this was probably broken in: 063bd6f ARM: EXYNOS: Remove GIC save & restore function > I have to take a look to the U-boot that is shipped on the device, I think > the correct branch is [1] but I'm not sure if that is the correct one. It is the right one. If U-Boot were touching this (which would greatly surprise me) it should be here: arch/arm/include/asm/arch-exynos/cpu.h ...and it's not. Doing a grep for '10440000' (the combiner base address) doesn't find anything in U-Boot either, which makes it less likely. ...and it's even less likely since the amount of code that is in U-Boot that runs at resume time is a very small subset and I'm fairly familiar with it and I would have remembered it touching the combiner. It's POSSIBLE that the internal ROM in exynos is clobbering this, but as per above it seems crazy unlikely and I think it's just losing power. -Doug
Hello Doug, Thanks a lot for your comments. On 06/12/2015 10:17 PM, Doug Anderson wrote: > Hi, > > On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: >>>> registers are lost assuming the combiner was powered down, even the >>>> status register will be lost and you will not know exactly the wakeup >>>> reason right ? >>>> >>> >>> Good question, I didn't find in the documentation I've access to that >>> this happen but just found through experimentation that the IRQ enable >>> set register values are lost after a resume and that saving/restoring >>> the values makes the interrupts to be triggered again. >>> >> >> I'll share here too the findings I mentioned over IRC. As you suggested I >> add some printouts and noticed that the ISTRn (Interrupt Status) registers >> values are indeed preserved on resume so I can know for example if the >> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the >> values of the registers against the Exynos manual and they corresponds to >> the interrupt sources in each case so the values are correct. >> >> So as you said, it seems that is not that the IP block loses its state on >> S2R but that something is blindly writing the IESRn (Interrupt Enable Set) >> registers. > > I'll postulate an alternate theory here. You can tell me if you buy > it or if you think I've been out in the sun too long. > > Let's say that the interrupt combiner's status registers show the raw > status as asserted by whatever is hooked up to the combiner. This > means that even if the combiner got reset we could still read the > status register and get the status of the source. Imagine that the > combiner is like a GPIO bank. If you reset the GPIO bank, you'll lose > all kinds of config (input vs. output, edge interrupt status, maybe > pulls, etc). ...but you can still read the state asserted by an > external source, right? > That definitely makes a lot of sense to me. > In this case the combiner's interrupt source is 'EINT 11'. ...and I'm > pretty sure that the controller managine 'EINT 11' _doesn't_ lose > power across suspend/resume... > Yes, the External Interrupt sources come from the GPIO controller and the driver has a similar logic than $subject to save the registers state. > > I'll further bolster my theory by saying that _almost nothing_ in the > exynos keeps power across suspend/resume. The UART? Nope. The GPIO > controllers? Nuh-uh. The GIC? Sorry, but no. The clock tree? It > might be nice, but you're out of luck. ...so it would make me > terribly surprised to see the combiner keep power. > It surprised me as well but I didn't think about what you said that the combiner interrupt source was restored so the status register could just be showing the combiner's input raw status even after the chip was reset. > >> To reduce the possible s/w components that could be doing this, I booted a >> signed FIT image directly using the RO U-Boot instead of chain loading a >> mainline nv-uboot. In this configuration I've the same issue so it seems >> that if something is zeroing those registers on S2R, this can't be changed >> without void the warranty of these machines. >> >> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that >> they have a very similar solution than my patch, the IESRn are also saved >> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead >> or registering a syscore ops but the idea is basically the same. > > Yup, you can see where kliegs added it in > <https://chromium-review.googlesource.com/#/c/27964/>. As per the > comments in that CL, this was probably broken in: > > 063bd6f ARM: EXYNOS: Remove GIC save & restore function > > Nod, thanks for the pointer. >> I have to take a look to the U-boot that is shipped on the device, I think >> the correct branch is [1] but I'm not sure if that is the correct one. > > It is the right one. If U-Boot were touching this (which would > greatly surprise me) it should be here: > > arch/arm/include/asm/arch-exynos/cpu.h > > ...and it's not. Doing a grep for '10440000' (the combiner base > address) doesn't find anything in U-Boot either, which makes it less > likely. ...and it's even less likely since the amount of code that is > in U-Boot that runs at resume time is a very small subset and I'm > fairly familiar with it and I would have remembered it touching the > combiner. > That explains why I was also not able to find were U-Boot was overwriting those registers. > It's POSSIBLE that the internal ROM in exynos is clobbering this, but > as per above it seems crazy unlikely and I think it's just losing > power. > Agreed. > > -Doug > Sudeep, so we may need something like $subject after all from Doug's explanations since the combiner chip state is lost during a S2R. I know that it adds more duplicated code (others irqchip drivers do the same) and it may not scale well if a chip has many registers but is the best solution I could came with. If you have a suggestion for a better alternative, I can give a try and write the patch. But I think $subject could also land to fix this issue since is a very non intrusive change and later can be changed once the irqchip core supports this use case. Best regards, Javier
On 12/06/15 21:17, Doug Anderson wrote: > Hi, > > On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: >>>> registers are lost assuming the combiner was powered down, even the >>>> status register will be lost and you will not know exactly the wakeup >>>> reason right ? >>>> >>> >>> Good question, I didn't find in the documentation I've access to that >>> this happen but just found through experimentation that the IRQ enable >>> set register values are lost after a resume and that saving/restoring >>> the values makes the interrupts to be triggered again. >>> >> >> I'll share here too the findings I mentioned over IRC. As you suggested I >> add some printouts and noticed that the ISTRn (Interrupt Status) registers >> values are indeed preserved on resume so I can know for example if the >> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the >> values of the registers against the Exynos manual and they corresponds to >> the interrupt sources in each case so the values are correct. >> >> So as you said, it seems that is not that the IP block loses its state on >> S2R but that something is blindly writing the IESRn (Interrupt Enable Set) >> registers. > > I'll postulate an alternate theory here. You can tell me if you buy > it or if you think I've been out in the sun too long. > > Let's say that the interrupt combiner's status registers show the raw > status as asserted by whatever is hooked up to the combiner. This > means that even if the combiner got reset we could still read the > status register and get the status of the source. Imagine that the > combiner is like a GPIO bank. If you reset the GPIO bank, you'll lose > all kinds of config (input vs. output, edge interrupt status, maybe > pulls, etc). ...but you can still read the state asserted by an > external source, right? > Right, this makes sense. I assumed status was latched output and might be lost. But that's wrong assumption I believe. > In this case the combiner's interrupt source is 'EINT 11'. ...and I'm > pretty sure that the controller managine 'EINT 11' _doesn't_ lose > power across suspend/resume... > > > I'll further bolster my theory by saying that _almost nothing_ in the > exynos keeps power across suspend/resume. The UART? Nope. The GPIO > controllers? Nuh-uh. The GIC? Sorry, but no. The clock tree? It > might be nice, but you're out of luck. ...so it would make me > terribly surprised to see the combiner keep power. > Interesting even GIC loses power ? I would be interested in knowing more details as who will wake up the system then. Is the wakeup source offloaded to some external power controller ? > >> To reduce the possible s/w components that could be doing this, I booted a >> signed FIT image directly using the RO U-Boot instead of chain loading a >> mainline nv-uboot. In this configuration I've the same issue so it seems >> that if something is zeroing those registers on S2R, this can't be changed >> without void the warranty of these machines. >> >> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that >> they have a very similar solution than my patch, the IESRn are also saved >> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead >> or registering a syscore ops but the idea is basically the same. > > Yup, you can see where kliegs added it in > <https://chromium-review.googlesource.com/#/c/27964/>. As per the > comments in that CL, this was probably broken in: > > 063bd6f ARM: EXYNOS: Remove GIC save & restore function > > >> I have to take a look to the U-boot that is shipped on the device, I think >> the correct branch is [1] but I'm not sure if that is the correct one. > > It is the right one. If U-Boot were touching this (which would > greatly surprise me) it should be here: > > arch/arm/include/asm/arch-exynos/cpu.h > > ...and it's not. Doing a grep for '10440000' (the combiner base > address) doesn't find anything in U-Boot either, which makes it less > likely. ...and it's even less likely since the amount of code that is > in U-Boot that runs at resume time is a very small subset and I'm > fairly familiar with it and I would have remembered it touching the > combiner. > Yes, I did search and find nothing, so definitely not U-Boot if I am looking at the right source. > It's POSSIBLE that the internal ROM in exynos is clobbering this, but > as per above it seems crazy unlikely and I think it's just losing > power. > Agreed, not because I believe ROM code are not crazy but because of the theory you have provided above :) Regards, Sudeep
On 15/06/15 08:46, Javier Martinez Canillas wrote: [...] > > Sudeep, so we may need something like $subject after all from Doug's > explanations since the combiner chip state is lost during a S2R. I know > that it adds more duplicated code (others irqchip drivers do the same) > and it may not scale well if a chip has many registers but is the best > solution I could came with. > OK > If you have a suggestion for a better alternative, I can give a try and > write the patch. But I think $subject could also land to fix this issue > since is a very non intrusive change and later can be changed once the > irqchip core supports this use case. > Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake also and then you can restore iff it's non-zero as irq core will take care of most of the non-wakeup sources. Because I am planning to push MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the combiner interrupts are always on in GIC. Implement set_irq_wake as enable_irq_wake (comb_irq_to_GIC). Regards, Sudeep
Hello Sudeep, On 06/15/2015 11:01 AM, Sudeep Holla wrote: > > > On 15/06/15 08:46, Javier Martinez Canillas wrote: > [...] > >> >> Sudeep, so we may need something like $subject after all from Doug's >> explanations since the combiner chip state is lost during a S2R. I know >> that it adds more duplicated code (others irqchip drivers do the same) >> and it may not scale well if a chip has many registers but is the best >> solution I could came with. >> > > OK > >> If you have a suggestion for a better alternative, I can give a try and >> write the patch. But I think $subject could also land to fix this issue >> since is a very non intrusive change and later can be changed once the >> irqchip core supports this use case. >> > > Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake > also and then you can restore iff it's non-zero as irq core will take > care of most of the non-wakeup sources. Because I am planning to push I've looking at this and a problem I found is that IIUC the set_irq_wake is not propagated from the the Exynos pinctrl / GPIO driver which is the combiner's external interrupt source so the callback is never called. Which means that right now only the state of the wakeup source IRQs can't be saved since that information is not present. The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables the combiner interrupts but its .irq_set_wake handler only updates the wakeup source mask for the external interrupts but does not call the combiner .set_irq_wake so that should be changed as well. But even for non-wakeup interrupts, I found that they are not enabled when adding the MASK_ON_SUSPEND on my tests. I don't know if that is related to the pinctrl driver missing something else though. > MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the > combiner interrupts are always on in GIC. Implement set_irq_wake as > enable_irq_wake (comb_irq_to_GIC). > Right, but can we do this as a follow-up? S2R is currently broken on these machines and $subject is I think a reasonable small fix that won't introduce any regressions. To do a more intrusive change, I should better understand the interactions between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the meantime S2R will continue to be broken on these platforms unless someone more familiar with all this could point me in the right direction. > Regards, > Sudeep > Best regards, Javier
On 15/06/15 16:00, Javier Martinez Canillas wrote: > Hello Sudeep, > > On 06/15/2015 11:01 AM, Sudeep Holla wrote: >> >> >> On 15/06/15 08:46, Javier Martinez Canillas wrote: >> [...] >> >>> >>> Sudeep, so we may need something like $subject after all from Doug's >>> explanations since the combiner chip state is lost during a S2R. I know >>> that it adds more duplicated code (others irqchip drivers do the same) >>> and it may not scale well if a chip has many registers but is the best >>> solution I could came with. >>> >> >> OK >> >>> If you have a suggestion for a better alternative, I can give a try and >>> write the patch. But I think $subject could also land to fix this issue >>> since is a very non intrusive change and later can be changed once the >>> irqchip core supports this use case. >>> >> >> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake >> also and then you can restore iff it's non-zero as irq core will take >> care of most of the non-wakeup sources. Because I am planning to push > > I've looking at this and a problem I found is that IIUC the set_irq_wake > is not propagated from the the Exynos pinctrl / GPIO driver which is the > combiner's external interrupt source so the callback is never called. > Which means that right now only the state of the wakeup source IRQs can't > be saved since that information is not present. > > The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables > the combiner interrupts but its .irq_set_wake handler only updates the > wakeup source mask for the external interrupts but does not call the > combiner .set_irq_wake so that should be changed as well. > Thanks for the looking at this. > But even for non-wakeup interrupts, I found that they are not enabled when > adding the MASK_ON_SUSPEND on my tests. I don't know if that is related > to the pinctrl driver missing something else though. > Even GIC is not masking any interrupt on suspend and if other irqchip or drivers are assuming that, it will work fine for now. But once I introduce that change in GIC it will break. >> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the >> combiner interrupts are always on in GIC. Implement set_irq_wake as >> enable_irq_wake (comb_irq_to_GIC). >> > > Right, but can we do this as a follow-up? S2R is currently broken on these > machines and $subject is I think a reasonable small fix that won't introduce > any regressions. > No issues, I just wanted to understand the issue better and also make sure we understand that things might break in future once that GIC change is introduced. So we already know the reason or atleast have some basic understanding as why would it break if it breaks :) > To do a more intrusive change, I should better understand the interactions > between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the > meantime S2R will continue to be broken on these platforms unless someone > more familiar with all this could point me in the right direction. > As I said I am fine with this patch for now and I don't want to block it. Regards, Sudeep
Hello Sudeep, On 06/15/2015 05:08 PM, Sudeep Holla wrote: > On 15/06/15 16:00, Javier Martinez Canillas wrote: >> On 06/15/2015 11:01 AM, Sudeep Holla wrote: >>> On 15/06/15 08:46, Javier Martinez Canillas wrote: [...] >>> >>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake >>> also and then you can restore iff it's non-zero as irq core will take >>> care of most of the non-wakeup sources. Because I am planning to push >> >> I've looking at this and a problem I found is that IIUC the set_irq_wake >> is not propagated from the the Exynos pinctrl / GPIO driver which is the >> combiner's external interrupt source so the callback is never called. >> Which means that right now only the state of the wakeup source IRQs can't >> be saved since that information is not present. >> >> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables >> the combiner interrupts but its .irq_set_wake handler only updates the >> wakeup source mask for the external interrupts but does not call the >> combiner .set_irq_wake so that should be changed as well. >> > > Thanks for the looking at this. > You are welcome and thanks to you for pointing this out. >> But even for non-wakeup interrupts, I found that they are not enabled when >> adding the MASK_ON_SUSPEND on my tests. I don't know if that is related >> to the pinctrl driver missing something else though. >> > > Even GIC is not masking any interrupt on suspend and if other irqchip or > drivers are assuming that, it will work fine for now. But once I > introduce that change in GIC it will break. > >>> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the >>> combiner interrupts are always on in GIC. Implement set_irq_wake as >>> enable_irq_wake (comb_irq_to_GIC). >>> >> >> Right, but can we do this as a follow-up? S2R is currently broken on these >> machines and $subject is I think a reasonable small fix that won't introduce >> any regressions. >> > > No issues, I just wanted to understand the issue better and also make > sure we understand that things might break in future once that GIC > change is introduced. So we already know the reason or atleast have some > basic understanding as why would it break if it breaks :) > Indeed, in the meantime I will continue looking at this to better understand all the interactions so if something breaks with your changes I may be able to test / debug and hopefully fix it :) >> To do a more intrusive change, I should better understand the interactions >> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the >> meantime S2R will continue to be broken on these platforms unless someone >> more familiar with all this could point me in the right direction. >> > > As I said I am fine with this patch for now and I don't want to block it. > Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed with the patch so hopefully this can land sooner rather than later. > Regards, > Sudeep > Best regards, Javier
On 16.06.2015 00:23, Javier Martinez Canillas wrote: (...) >>> To do a more intrusive change, I should better understand the interactions >>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the >>> meantime S2R will continue to be broken on these platforms unless someone >>> more familiar with all this could point me in the right direction. >>> >> >> As I said I am fine with this patch for now and I don't want to block it. >> > > Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed > with the patch so hopefully this can land sooner rather than later. I assume this will go through irqchip tree, right? Best regards, Krzysztof
Hello Krzysztof, On 06/16/2015 01:57 AM, Krzysztof Kozlowski wrote: > On 16.06.2015 00:23, Javier Martinez Canillas wrote: (...) >>>> To do a more intrusive change, I should better understand the >>>> interactions between the Exynos pinctrl / GPIO, interrupt >>>> combiner and the GIC and in the meantime S2R will continue to >>>> be broken on these platforms unless someone more familiar with >>>> all this could point me in the right direction. >>>> >>> >>> As I said I am fine with this patch for now and I don't want to >>> block it. >>> >> >> Thanks a lot, Krzysztof who is one of the Exynos maintainers has >> also agreed with the patch so hopefully this can land sooner rather >> than later. > > I assume this will go through irqchip tree, right? > That is my assumption as well. I meant the fact that you as an Exynos maintainer thinks $subject is a sensible solution to the issue, would give confidence to the irqchip maintainers to pick this patch soon. > Best regards, Krzysztof > Best regards, Javier
On Tue, 16 Jun 2015, Krzysztof Kozlowski wrote: > On 16.06.2015 00:23, Javier Martinez Canillas wrote: > (...) > >>> To do a more intrusive change, I should better understand the interactions > >>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the > >>> meantime S2R will continue to be broken on these platforms unless someone > >>> more familiar with all this could point me in the right direction. > >>> > >> > >> As I said I am fine with this patch for now and I don't want to block it. > >> > > > > Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed > > with the patch so hopefully this can land sooner rather than later. > > I assume this will go through irqchip tree, right? yes, picking it up now Thanks, tglx
2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas <javier.martinez@collabora.co.uk>: > Hello Sudeep, > > On 06/15/2015 11:01 AM, Sudeep Holla wrote: >> >> >> On 15/06/15 08:46, Javier Martinez Canillas wrote: >> [...] >> >>> >>> Sudeep, so we may need something like $subject after all from Doug's >>> explanations since the combiner chip state is lost during a S2R. I know >>> that it adds more duplicated code (others irqchip drivers do the same) >>> and it may not scale well if a chip has many registers but is the best >>> solution I could came with. >>> >> >> OK >> >>> If you have a suggestion for a better alternative, I can give a try and >>> write the patch. But I think $subject could also land to fix this issue >>> since is a very non intrusive change and later can be changed once the >>> irqchip core supports this use case. >>> >> >> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake >> also and then you can restore iff it's non-zero as irq core will take >> care of most of the non-wakeup sources. Because I am planning to push > > I've looking at this and a problem I found is that IIUC the set_irq_wake > is not propagated from the the Exynos pinctrl / GPIO driver which is the > combiner's external interrupt source so the callback is never called. > Which means that right now only the state of the wakeup source IRQs can't > be saved since that information is not present. > > The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables > the combiner interrupts but its .irq_set_wake handler only updates the > wakeup source mask for the external interrupts but does not call the > combiner .set_irq_wake so that should be changed as well. > As far as I'm aware of, wake-up events from pin controllers don't go through GIC, but rather directly to PMU, which is a dedicated unit responsible for power management and not a standalone interrupt controller (well actually I saw a series making it a cascaded controller some time ago, but I'm not sure if that went in). Based on this, I don't think we have to call set_irq_wake on GIC. Correct me if I'm wrong, though. Best regards, Tomasz
On 16/06/15 13:32, Tomasz Figa wrote: > 2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas <javier.martinez@collabora.co.uk>: >> On 06/15/2015 11:01 AM, Sudeep Holla wrote: [...] >>> >>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and >>> set_irq_wake also and then you can restore iff it's non-zero as >>> irq core will take care of most of the non-wakeup sources. >>> Because I am planning to push >> >> I've looking at this and a problem I found is that IIUC the >> set_irq_wake is not propagated from the the Exynos pinctrl / GPIO >> driver which is the combiner's external interrupt source so the >> callback is never called. Which means that right now only the >> state of the wakeup source IRQs can't be saved since that >> information is not present. >> >> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and >> disables the combiner interrupts but its .irq_set_wake handler >> only updates the wakeup source mask for the external interrupts but >> does not call the combiner .set_irq_wake so that should be changed >> as well. >> > > As far as I'm aware of, wake-up events from pin controllers don't go > through GIC, but rather directly to PMU, which is a dedicated unit > responsible for power management and not a standalone interrupt > controller (well actually I saw a series making it a cascaded > controller some time ago, but I'm not sure if that went in). Based on > this, I don't think we have to call set_irq_wake on GIC. Correct me > if I'm wrong, though. Thanks for the details, this was my assumption when Doug confirmed that the combiner is also powered down, either there must be some bypass or a dedicated logic to wakeup. But I was not sure though so insisted set_irq_wake but based on what you say it's not required. Regards, Sudeep
diff --git a/drivers/irqchip/exynos-combiner.c b/drivers/irqchip/exynos-combiner.c index 5945223b73fa..c0bcec59f829 100644 --- a/drivers/irqchip/exynos-combiner.c +++ b/drivers/irqchip/exynos-combiner.c @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = { #ifdef CONFIG_SMP .irq_set_affinity = combiner_set_affinity, #endif + .flags = IRQCHIP_MASK_ON_SUSPEND, };