| Message ID | 1590253873-11556-5-git-send-email-mkshah@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip: qcom: pdc: Introduce irq_set_wake call | expand |
Quoting Maulik Shah (2020-05-23 10:11:13) > Remove irq_disable callback to allow lazy disable for pdc interrupts. > > Add irq_set_wake callback that unmask interrupt in HW when drivers > mark interrupt for wakeup. Interrupt will be cleared in HW during > lazy disable if its not marked for wakeup. > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index 6ae9e1f..f7c0662 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -36,6 +36,7 @@ struct pdc_pin_region { > u32 cnt; > }; > > +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS); static? > static DEFINE_RAW_SPINLOCK(pdc_lock); > static void __iomem *pdc_base; > static struct pdc_pin_region *pdc_region; > @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on) > raw_spin_unlock(&pdc_lock); > } > > -static void qcom_pdc_gic_disable(struct irq_data *d) > +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) > { > if (d->hwirq == GPIO_NO_WAKE_IRQ) > - return; > - > - pdc_enable_intr(d, false); > - irq_chip_disable_parent(d); > -} > + return 0; Shouldn't this fail if we can't set for wake? > > -static void qcom_pdc_gic_enable(struct irq_data *d) > -{ > - if (d->hwirq == GPIO_NO_WAKE_IRQ) > - return; > + if (on) { > + pdc_enable_intr(d, true); > + irq_chip_enable_parent(d); > + set_bit(d->hwirq, pdc_wake_irqs); > + } else { > + clear_bit(d->hwirq, pdc_wake_irqs); > + } > > - pdc_enable_intr(d, true); > - irq_chip_enable_parent(d); > + return irq_chip_set_wake_parent(d, on); > } > > static void qcom_pdc_gic_mask(struct irq_data *d) The diff is really hard to read too. Maybe set_wake can be added first and then the enable/disable functions removed? > @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d) > if (d->hwirq == GPIO_NO_WAKE_IRQ) > return; > > + if (!test_bit(d->hwirq, pdc_wake_irqs)) > + pdc_enable_intr(d, false); > + > irq_chip_mask_parent(d); > } > > @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) > if (d->hwirq == GPIO_NO_WAKE_IRQ) > return; > > + pdc_enable_intr(d, true); > irq_chip_unmask_parent(d); > } > I find these two hunks deeply confusing. I'm not sure what the maintainers think though. I hope it would be simpler to always enable the hwirqs in the pdc when an irq is requested and only disable it in the pdc when the system goes to suspend and the pdc pin isn't for an irq that's marked for wakeup. Does that break somehow? My understanding of the hardware is that the GPIO controller has lines directly connected to various SPI lines on the GIC and PDC has a way to monitor those direct connections and wakeup the CPUs when they trigger the detection logic in the PDC. The enable/disable bit in PDC gates that logic for each wire between the GPIO controller and the GIC. So isn't it simpler to leave the PDC monitoring pins that we care about all the time and only stop monitoring when we enter and leave suspend? And shouldn't the driver set something sane in qcom_pdc_init() to disable all the pdc pins so that we don't rely on boot state to configure pins for wakeup? > @@ -197,15 +200,13 @@ static struct irq_chip qcom_pdc_gic_chip = { > .irq_eoi = irq_chip_eoi_parent, > .irq_mask = qcom_pdc_gic_mask, > .irq_unmask = qcom_pdc_gic_unmask, > - .irq_disable = qcom_pdc_gic_disable, > - .irq_enable = qcom_pdc_gic_enable, > .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state, > .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state, > .irq_retrigger = irq_chip_retrigger_hierarchy, > .irq_set_type = qcom_pdc_gic_set_type, > + .irq_set_wake = qcom_pdc_gic_set_wake, > .flags = IRQCHIP_MASK_ON_SUSPEND | > - IRQCHIP_SET_TYPE_MASKED | > - IRQCHIP_SKIP_SET_WAKE, > + IRQCHIP_SET_TYPE_MASKED, > .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, > .irq_set_affinity = irq_chip_set_affinity_parent,
Hi, On 5/27/2020 3:45 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-05-23 10:11:13) >> Remove irq_disable callback to allow lazy disable for pdc interrupts. >> >> Add irq_set_wake callback that unmask interrupt in HW when drivers >> mark interrupt for wakeup. Interrupt will be cleared in HW during >> lazy disable if its not marked for wakeup. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++---------------- >> 1 file changed, 17 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c >> index 6ae9e1f..f7c0662 100644 >> --- a/drivers/irqchip/qcom-pdc.c >> +++ b/drivers/irqchip/qcom-pdc.c >> @@ -36,6 +36,7 @@ struct pdc_pin_region { >> u32 cnt; >> }; >> >> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS); > static? Thanks i will declare as static in v3. > >> static DEFINE_RAW_SPINLOCK(pdc_lock); >> static void __iomem *pdc_base; >> static struct pdc_pin_region *pdc_region; >> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on) >> raw_spin_unlock(&pdc_lock); >> } >> >> -static void qcom_pdc_gic_disable(struct irq_data *d) >> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) >> { >> if (d->hwirq == GPIO_NO_WAKE_IRQ) >> - return; >> - >> - pdc_enable_intr(d, false); >> - irq_chip_disable_parent(d); >> -} >> + return 0; > Shouldn't this fail if we can't set for wake? we return success/failure from parent chip with below call at end of set_wake. return irq_chip_set_wake_parent(d, on); > >> >> -static void qcom_pdc_gic_enable(struct irq_data *d) >> -{ >> - if (d->hwirq == GPIO_NO_WAKE_IRQ) >> - return; >> + if (on) { >> + pdc_enable_intr(d, true); >> + irq_chip_enable_parent(d); >> + set_bit(d->hwirq, pdc_wake_irqs); >> + } else { >> + clear_bit(d->hwirq, pdc_wake_irqs); >> + } >> >> - pdc_enable_intr(d, true); >> - irq_chip_enable_parent(d); >> + return irq_chip_set_wake_parent(d, on); >> } >> >> static void qcom_pdc_gic_mask(struct irq_data *d) > The diff is really hard to read too. Maybe set_wake can be added first > and then the enable/disable functions removed? i think should be ok in same patch, if you insist i can split this change in to two. > >> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d) >> if (d->hwirq == GPIO_NO_WAKE_IRQ) >> return; >> >> + if (!test_bit(d->hwirq, pdc_wake_irqs)) >> + pdc_enable_intr(d, false); >> + >> irq_chip_mask_parent(d); >> } >> >> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) >> if (d->hwirq == GPIO_NO_WAKE_IRQ) >> return; >> >> + pdc_enable_intr(d, true); >> irq_chip_unmask_parent(d); >> } >> > I find these two hunks deeply confusing. I'm not sure what the > maintainers think though. I hope it would be simpler to always enable > the hwirqs in the pdc when an irq is requested and only disable it in > the pdc when the system goes to suspend and the pdc pin isn't for an irq > that's marked for wakeup. Does that break somehow? PDC monitors interrupts during CPUidle as well, in cases where deepest low power mode happened from cpuidle where GIC is not active. If we keep PDC IRQ always enabled/unmasked during idle and then disable/mask when entering to suspend, it will break cpuidle. > > My understanding of the hardware is that the GPIO controller has lines > directly connected to various SPI lines on the GIC and PDC has a way to > monitor those direct connections and wakeup the CPUs when they trigger > the detection logic in the PDC. The enable/disable bit in PDC gates that > logic for each wire between the GPIO controller and the GIC. > > So isn't it simpler to leave the PDC monitoring pins that we care about > all the time and only stop monitoring when we enter and leave suspend? it can affect idle path as explained above. > And shouldn't the driver set something sane in qcom_pdc_init() to > disable all the pdc pins so that we don't rely on boot state to > configure pins for wakeup? We don't rely on boot state, by default all interrupt will be disabled. This is same to GIC driver having GICD_ISENABLER register, where all bits (one bit per interrupt) set to 0 (masked irqs) during boot up. Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK. Thanks, Maulik > >> @@ -197,15 +200,13 @@ static struct irq_chip qcom_pdc_gic_chip = { >> .irq_eoi = irq_chip_eoi_parent, >> .irq_mask = qcom_pdc_gic_mask, >> .irq_unmask = qcom_pdc_gic_unmask, >> - .irq_disable = qcom_pdc_gic_disable, >> - .irq_enable = qcom_pdc_gic_enable, >> .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state, >> .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state, >> .irq_retrigger = irq_chip_retrigger_hierarchy, >> .irq_set_type = qcom_pdc_gic_set_type, >> + .irq_set_wake = qcom_pdc_gic_set_wake, >> .flags = IRQCHIP_MASK_ON_SUSPEND | >> - IRQCHIP_SET_TYPE_MASKED | >> - IRQCHIP_SKIP_SET_WAKE, >> + IRQCHIP_SET_TYPE_MASKED, >> .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> .irq_set_affinity = irq_chip_set_affinity_parent,
Quoting Maulik Shah (2020-05-29 02:20:32) > Hi, > > On 5/27/2020 3:45 PM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-05-23 10:11:13) > >> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on) > >> raw_spin_unlock(&pdc_lock); > >> } > >> > >> -static void qcom_pdc_gic_disable(struct irq_data *d) > >> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) > >> { > >> if (d->hwirq == GPIO_NO_WAKE_IRQ) > >> - return; > >> - > >> - pdc_enable_intr(d, false); > >> - irq_chip_disable_parent(d); > >> -} > >> + return 0; > > Shouldn't this fail if we can't set for wake? > > we return success/failure from parent chip with below call at end of > set_wake. > > return irq_chip_set_wake_parent(d, on); It's not a question about the parent irqchip. I'm wondering why we would return success for a gpio irq that can't be marked for wakeup when a client driver tries to enable wake on it. My understanding is that all gpios irqs call here and PDC can't monitor all of them so some are GPIO_NO_WAKE_IRQ and thus trying to mark those for wakeup should fail. Of course msm_gpio_irq_set_wake() should also fail if it can't mark the gpio for wakeup, but that's another problem. > >> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) > >> if (d->hwirq == GPIO_NO_WAKE_IRQ) > >> return; > >> > >> + pdc_enable_intr(d, true); > >> irq_chip_unmask_parent(d); > >> } > >> > > I find these two hunks deeply confusing. I'm not sure what the > > maintainers think though. I hope it would be simpler to always enable > > the hwirqs in the pdc when an irq is requested and only disable it in > > the pdc when the system goes to suspend and the pdc pin isn't for an irq > > that's marked for wakeup. Does that break somehow? > PDC monitors interrupts during CPUidle as well, in cases where deepest > low power mode happened from cpuidle where GIC is not active. > If we keep PDC IRQ always enabled/unmasked during idle and then > disable/mask when entering to suspend, it will break cpuidle. How does it break cpuidle? The irqs that would be enabled/unmasked in pdc would only be the irqs that the kernel has setup irq handlers for (from request_irq() and friends). We want those irqs to keep working during cpuidle and wake the CPU from the deepest idle states. > > > > > My understanding of the hardware is that the GPIO controller has lines > > directly connected to various SPI lines on the GIC and PDC has a way to > > monitor those direct connections and wakeup the CPUs when they trigger > > the detection logic in the PDC. The enable/disable bit in PDC gates that > > logic for each wire between the GPIO controller and the GIC. > > > > So isn't it simpler to leave the PDC monitoring pins that we care about > > all the time and only stop monitoring when we enter and leave suspend? > > it can affect idle path as explained above. > > > And shouldn't the driver set something sane in qcom_pdc_init() to > > disable all the pdc pins so that we don't rely on boot state to > > configure pins for wakeup? > > We don't rely on boot state, by default all interrupt will be disabled. Does 'default' mean the hardware register reset state? I'm worried that we will kexec and then various pdc pins will be enabled because the previous kernel had them enabled but then the new kernel doesn't care about those pins and we'll never be able to suspend or go idle. I don't know what happens in the GIC case but I think gic_dist_config() and things set a sane state at kernel boot. > > This is same to GIC driver having GICD_ISENABLER register, where all > bits (one bit per interrupt) set to 0 (masked irqs) during boot up. > > Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK. > What code sets the IRQ_ENABLE_BANK to all zero when this driver probes?
Hi, On 5/31/2020 12:56 AM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-05-29 02:20:32) >> Hi, >> >> On 5/27/2020 3:45 PM, Stephen Boyd wrote: >>> Quoting Maulik Shah (2020-05-23 10:11:13) >>>> @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on) >>>> raw_spin_unlock(&pdc_lock); >>>> } >>>> >>>> -static void qcom_pdc_gic_disable(struct irq_data *d) >>>> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) >>>> { >>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) >>>> - return; >>>> - >>>> - pdc_enable_intr(d, false); >>>> - irq_chip_disable_parent(d); >>>> -} >>>> + return 0; >>> Shouldn't this fail if we can't set for wake? >> we return success/failure from parent chip with below call at end of >> set_wake. >> >> return irq_chip_set_wake_parent(d, on); > It's not a question about the parent irqchip. I'm wondering why we would > return success for a gpio irq that can't be marked for wakeup when a > client driver tries to enable wake on it. My understanding is that all > gpios irqs call here and PDC can't monitor all of them so some are > GPIO_NO_WAKE_IRQ and thus trying to mark those for wakeup should fail. > Of course msm_gpio_irq_set_wake() should also fail if it can't mark the > gpio for wakeup, but that's another problem. i can change this to return error code. PDC's caller msmgpio chip, currently don't check for return value for irq_set_wake callback. i will udpate that as well in next revision. > >>>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) >>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) >>>> return; >>>> >>>> + pdc_enable_intr(d, true); >>>> irq_chip_unmask_parent(d); >>>> } >>>> >>> I find these two hunks deeply confusing. I'm not sure what the >>> maintainers think though. I hope it would be simpler to always enable >>> the hwirqs in the pdc when an irq is requested and only disable it in >>> the pdc when the system goes to suspend and the pdc pin isn't for an irq >>> that's marked for wakeup. Does that break somehow? >> PDC monitors interrupts during CPUidle as well, in cases where deepest >> low power mode happened from cpuidle where GIC is not active. >> If we keep PDC IRQ always enabled/unmasked during idle and then >> disable/mask when entering to suspend, it will break cpuidle. > How does it break cpuidle? The irqs that would be enabled/unmasked in > pdc would only be the irqs that the kernel has setup irq handlers for > (from request_irq() and friends). We want those irqs to keep working > during cpuidle and wake the CPU from the deepest idle states. >>I hope it would be simpler to always enable >>the hwirqs in the pdc when an irq is requested and only disable it in >>the pdc when the system goes to suspend and the pdc pin isn't for an irq >>that's marked for wakeup >>How does it break cpuidle? Consider a scenario.. 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW. 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ. 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC. 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW. 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled) 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states. > >>> My understanding of the hardware is that the GPIO controller has lines >>> directly connected to various SPI lines on the GIC and PDC has a way to >>> monitor those direct connections and wakeup the CPUs when they trigger >>> the detection logic in the PDC. The enable/disable bit in PDC gates that >>> logic for each wire between the GPIO controller and the GIC. >>> >>> So isn't it simpler to leave the PDC monitoring pins that we care about >>> all the time and only stop monitoring when we enter and leave suspend? >> it can affect idle path as explained above. >> >>> And shouldn't the driver set something sane in qcom_pdc_init() to >>> disable all the pdc pins so that we don't rely on boot state to >>> configure pins for wakeup? >> We don't rely on boot state, by default all interrupt will be disabled. > Does 'default' mean the hardware register reset state? correct. > I'm worried that > we will kexec and then various pdc pins will be enabled because the > previous kernel had them enabled but then the new kernel doesn't care > about those pins and we'll never be able to suspend or go idle. I don't > know what happens in the GIC case but I think gic_dist_config() and > things set a sane state at kernel boot. Right however when switching kernel, i suppose client drivers will do a free_irq(), then this will clear the PDC interrupt in HW by invoking mask_irq() from within free_irq(). > >> This is same to GIC driver having GICD_ISENABLER register, where all >> bits (one bit per interrupt) set to 0 (masked irqs) during boot up. >> >> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK. >> > What code sets the IRQ_ENABLE_BANK to all zero when this driver probes? Enable bank will be zero as part of HW reset status when booting up first time. Thanks, Maulik
Quoting Maulik Shah (2020-06-01 04:38:25) > On 5/31/2020 12:56 AM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-05-29 02:20:32) > >> On 5/27/2020 3:45 PM, Stephen Boyd wrote: > >>> Quoting Maulik Shah (2020-05-23 10:11:13) > >>>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) > >>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) > >>>> return; > >>>> > >>>> + pdc_enable_intr(d, true); > >>>> irq_chip_unmask_parent(d); > >>>> } > >>>> > >>> I find these two hunks deeply confusing. I'm not sure what the > >>> maintainers think though. I hope it would be simpler to always enable > >>> the hwirqs in the pdc when an irq is requested and only disable it in > >>> the pdc when the system goes to suspend and the pdc pin isn't for an irq > >>> that's marked for wakeup. Does that break somehow? > >> PDC monitors interrupts during CPUidle as well, in cases where deepest > >> low power mode happened from cpuidle where GIC is not active. > >> If we keep PDC IRQ always enabled/unmasked during idle and then > >> disable/mask when entering to suspend, it will break cpuidle. > > How does it break cpuidle? The irqs that would be enabled/unmasked in > > pdc would only be the irqs that the kernel has setup irq handlers for > > (from request_irq() and friends). We want those irqs to keep working > > during cpuidle and wake the CPU from the deepest idle states. > > >>I hope it would be simpler to always enable > >>the hwirqs in the pdc when an irq is requested and only disable it in > >>the pdc when the system goes to suspend and the pdc pin isn't for an irq > >>that's marked for wakeup > > >>How does it break cpuidle? > > Consider a scenario.. > 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens > 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW. > 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ. > 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC. > 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW. > 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled) > 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states. Ok so in summary, irq is left unmasked in pdc during deep cpu idle and it keeps waking up the CPU because it isn't masked at the PDC after the first time it interrupts? Is this a power problem? Because from a correctness standpoint we don't really care. It woke up the CPU because it happened, and the GIC can decide to ignore it or not by masking it at the GIC. I thought that the PDC wouldn't wake up the CPU if we masked the irq at the GIC level. Is that not true? > > > > >>> My understanding of the hardware is that the GPIO controller has lines > >>> directly connected to various SPI lines on the GIC and PDC has a way to > >>> monitor those direct connections and wakeup the CPUs when they trigger > >>> the detection logic in the PDC. The enable/disable bit in PDC gates that > >>> logic for each wire between the GPIO controller and the GIC. > >>> > >>> So isn't it simpler to leave the PDC monitoring pins that we care about > >>> all the time and only stop monitoring when we enter and leave suspend? > >> it can affect idle path as explained above. > >> > >>> And shouldn't the driver set something sane in qcom_pdc_init() to > >>> disable all the pdc pins so that we don't rely on boot state to > >>> configure pins for wakeup? > >> We don't rely on boot state, by default all interrupt will be disabled. > > Does 'default' mean the hardware register reset state? > correct. > > I'm worried that > > we will kexec and then various pdc pins will be enabled because the > > previous kernel had them enabled but then the new kernel doesn't care > > about those pins and we'll never be able to suspend or go idle. I don't > > know what happens in the GIC case but I think gic_dist_config() and > > things set a sane state at kernel boot. > > Right however when switching kernel, i suppose client drivers will do a > free_irq(), then this will > > clear the PDC interrupt in HW by invoking mask_irq() from within free_irq(). We can't rely on drivers to do that. > > > > >> This is same to GIC driver having GICD_ISENABLER register, where all > >> bits (one bit per interrupt) set to 0 (masked irqs) during boot up. > >> > >> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK. > >> > > What code sets the IRQ_ENABLE_BANK to all zero when this driver probes? > > Enable bank will be zero as part of HW reset status when booting up > first time. > It's not a concern about the hardware reset state of these registers at boot. I'm worried that the bootloaders or previous OS will configure pdc pins to wake us up. It's better to just force it to something sane, i.e. everything disabled in the PDC, at driver probe time so that nothing can be wrong.
Hi, On 6/16/2020 5:27 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-06-01 04:38:25) >> On 5/31/2020 12:56 AM, Stephen Boyd wrote: >>> Quoting Maulik Shah (2020-05-29 02:20:32) >>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote: >>>>> Quoting Maulik Shah (2020-05-23 10:11:13) >>>>>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) >>>>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) >>>>>> return; >>>>>> >>>>>> + pdc_enable_intr(d, true); >>>>>> irq_chip_unmask_parent(d); >>>>>> } >>>>>> >>>>> I find these two hunks deeply confusing. I'm not sure what the >>>>> maintainers think though. I hope it would be simpler to always enable >>>>> the hwirqs in the pdc when an irq is requested and only disable it in >>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq >>>>> that's marked for wakeup. Does that break somehow? >>>> PDC monitors interrupts during CPUidle as well, in cases where deepest >>>> low power mode happened from cpuidle where GIC is not active. >>>> If we keep PDC IRQ always enabled/unmasked during idle and then >>>> disable/mask when entering to suspend, it will break cpuidle. >>> How does it break cpuidle? The irqs that would be enabled/unmasked in >>> pdc would only be the irqs that the kernel has setup irq handlers for >>> (from request_irq() and friends). We want those irqs to keep working >>> during cpuidle and wake the CPU from the deepest idle states. >>>> I hope it would be simpler to always enable >>>> the hwirqs in the pdc when an irq is requested and only disable it in >>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq >>>> that's marked for wakeup >>>> How does it break cpuidle? >> Consider a scenario.. >> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens >> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW. >> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ. >> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC. >> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW. >> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled) >> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states. > Ok so in summary, irq is left unmasked in pdc during deep cpu idle and > it keeps waking up the CPU because it isn't masked at the PDC after the > first time it interrupts? Is this a power problem? yes it can be a power problem. > Because from a > correctness standpoint we don't really care. It woke up the CPU because > it happened, and the GIC can decide to ignore it or not by masking it at > the GIC. I thought that the PDC wouldn't wake up the CPU if we masked > the irq at the GIC level. Is that not true? once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to GIC. since at GIC its masked, GIC doesn't forward to cpu to immediatly wake it up. however after PDC detecting IRQ, it exits low power mode and watchdog/timer can wakeup upon expiry. > >>>>> My understanding of the hardware is that the GPIO controller has lines >>>>> directly connected to various SPI lines on the GIC and PDC has a way to >>>>> monitor those direct connections and wakeup the CPUs when they trigger >>>>> the detection logic in the PDC. The enable/disable bit in PDC gates that >>>>> logic for each wire between the GPIO controller and the GIC. >>>>> >>>>> So isn't it simpler to leave the PDC monitoring pins that we care about >>>>> all the time and only stop monitoring when we enter and leave suspend? >>>> it can affect idle path as explained above. >>>> >>>>> And shouldn't the driver set something sane in qcom_pdc_init() to >>>>> disable all the pdc pins so that we don't rely on boot state to >>>>> configure pins for wakeup? >>>> We don't rely on boot state, by default all interrupt will be disabled. >>> Does 'default' mean the hardware register reset state? >> correct. >>> I'm worried that >>> we will kexec and then various pdc pins will be enabled because the >>> previous kernel had them enabled but then the new kernel doesn't care >>> about those pins and we'll never be able to suspend or go idle. I don't >>> know what happens in the GIC case but I think gic_dist_config() and >>> things set a sane state at kernel boot. >> Right however when switching kernel, i suppose client drivers will do a >> free_irq(), then this will >> >> clear the PDC interrupt in HW by invoking mask_irq() from within free_irq(). > We can't rely on drivers to do that. > >>>> This is same to GIC driver having GICD_ISENABLER register, where all >>>> bits (one bit per interrupt) set to 0 (masked irqs) during boot up. >>>> >>>> Similarly PDC also have all bits set to 0 in PDC's IRQ_ENABLE_BANK. >>>> >>> What code sets the IRQ_ENABLE_BANK to all zero when this driver probes? >> Enable bank will be zero as part of HW reset status when booting up >> first time. >> > It's not a concern about the hardware reset state of these registers at > boot. I'm worried that the bootloaders or previous OS will configure pdc > pins to wake us up. It's better to just force it to something sane, i.e. > everything disabled in the PDC, at driver probe time so that nothing can > be wrong. okay, i will include a patch to disable all IRQs in PDC hw during init time in next revision. Thanks, Maulik
Quoting Maulik Shah (2020-06-18 03:03:03) > On 6/16/2020 5:27 PM, Stephen Boyd wrote: > > Quoting Maulik Shah (2020-06-01 04:38:25) > >> On 5/31/2020 12:56 AM, Stephen Boyd wrote: > >>> Quoting Maulik Shah (2020-05-29 02:20:32) > >>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote: > >>>>> Quoting Maulik Shah (2020-05-23 10:11:13) > >>>>>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) > >>>>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) > >>>>>> return; > >>>>>> > >>>>>> + pdc_enable_intr(d, true); > >>>>>> irq_chip_unmask_parent(d); > >>>>>> } > >>>>>> > >>>>> I find these two hunks deeply confusing. I'm not sure what the > >>>>> maintainers think though. I hope it would be simpler to always enable > >>>>> the hwirqs in the pdc when an irq is requested and only disable it in > >>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq > >>>>> that's marked for wakeup. Does that break somehow? > >>>> PDC monitors interrupts during CPUidle as well, in cases where deepest > >>>> low power mode happened from cpuidle where GIC is not active. > >>>> If we keep PDC IRQ always enabled/unmasked during idle and then > >>>> disable/mask when entering to suspend, it will break cpuidle. > >>> How does it break cpuidle? The irqs that would be enabled/unmasked in > >>> pdc would only be the irqs that the kernel has setup irq handlers for > >>> (from request_irq() and friends). We want those irqs to keep working > >>> during cpuidle and wake the CPU from the deepest idle states. > >>>> I hope it would be simpler to always enable > >>>> the hwirqs in the pdc when an irq is requested and only disable it in > >>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq > >>>> that's marked for wakeup > >>>> How does it break cpuidle? > >> Consider a scenario.. > >> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens > >> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW. > >> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ. > >> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC. > >> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW. > >> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled) > >> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states. > > Ok so in summary, irq is left unmasked in pdc during deep cpu idle and > > it keeps waking up the CPU because it isn't masked at the PDC after the > > first time it interrupts? Is this a power problem? > yes it can be a power problem. > > Because from a > > correctness standpoint we don't really care. It woke up the CPU because > > it happened, and the GIC can decide to ignore it or not by masking it at > > the GIC. I thought that the PDC wouldn't wake up the CPU if we masked > > the irq at the GIC level. Is that not true? > > once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to > GIC. > > since at GIC its masked, GIC doesn't forward to cpu to immediatly wake > it up. > > however after PDC detecting IRQ, it exits low power mode and > watchdog/timer can wakeup upon expiry. Ok. So the only problem is some screaming irq that really wants to be handled but the driver that requested it has disabled it at runtime. The IRQ keeps kicking the CPUs out of deep idle and then eventually the timer tick happens and we've run the CPUs in a shallower idle state for this time? Presumably we'd like to have these irqs be lazily masked at the PDC so that they can become pending when they first arrive but not block deep idle states if they're interrupting often while being handled. On the other hand, we want irq wake state to be the only factor in irqs being unmasked at the PDC on the entry to suspend. Purely masking/unmasking at the PDC when the irq is masked in software doesn't work because suspend/resume will break for disabled but wake enabled irqs. But doing that makes idle work easily because we can assume during idle that leaving it unmasked until it fires and then masking it in the PDC until it is handled gives us good deep idle states in the face of screaming irqs. What are the actual requirements? Here is my attempt to boil this discussion down into a few bullet points: 1. During system suspend, wake enabled irqs should be enabled in PDC and all other irqs should be disabled in PDC. 2. During idle, enabled irqs must be enabled in PDC, unless they're pending in which case they should be masked in the PDC so as to not wake up the CPU from deep idle states 3. During non-idle, non-suspend, enabled irqs must be enabled in PDC. Or is #3 actually false and PDC has no bearing on this?
Hi, On 6/19/2020 2:56 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-06-18 03:03:03) >> On 6/16/2020 5:27 PM, Stephen Boyd wrote: >>> Quoting Maulik Shah (2020-06-01 04:38:25) >>>> On 5/31/2020 12:56 AM, Stephen Boyd wrote: >>>>> Quoting Maulik Shah (2020-05-29 02:20:32) >>>>>> On 5/27/2020 3:45 PM, Stephen Boyd wrote: >>>>>>> Quoting Maulik Shah (2020-05-23 10:11:13) >>>>>>>> @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) >>>>>>>> if (d->hwirq == GPIO_NO_WAKE_IRQ) >>>>>>>> return; >>>>>>>> >>>>>>>> + pdc_enable_intr(d, true); >>>>>>>> irq_chip_unmask_parent(d); >>>>>>>> } >>>>>>>> >>>>>>> I find these two hunks deeply confusing. I'm not sure what the >>>>>>> maintainers think though. I hope it would be simpler to always enable >>>>>>> the hwirqs in the pdc when an irq is requested and only disable it in >>>>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq >>>>>>> that's marked for wakeup. Does that break somehow? >>>>>> PDC monitors interrupts during CPUidle as well, in cases where deepest >>>>>> low power mode happened from cpuidle where GIC is not active. >>>>>> If we keep PDC IRQ always enabled/unmasked during idle and then >>>>>> disable/mask when entering to suspend, it will break cpuidle. >>>>> How does it break cpuidle? The irqs that would be enabled/unmasked in >>>>> pdc would only be the irqs that the kernel has setup irq handlers for >>>>> (from request_irq() and friends). We want those irqs to keep working >>>>> during cpuidle and wake the CPU from the deepest idle states. >>>>>> I hope it would be simpler to always enable >>>>>> the hwirqs in the pdc when an irq is requested and only disable it in >>>>>> the pdc when the system goes to suspend and the pdc pin isn't for an irq >>>>>> that's marked for wakeup >>>>>> How does it break cpuidle? >>>> Consider a scenario.. >>>> 1. All PDC irqs enabled/unmasked in HW when request_irq() happened/alloc happens >>>> 2. Client driver disable's irq. (lazy disable is there, so in HW its still unmasked) but disabled in SW. >>>> 3. Device enters deep CPUidle low power modes where only PDC monitors IRQ. >>>> 4. This IRQ can still wakeup from CPUidle since it was monitored by PDC. >>>> 5. From handler, it comes to know that IRQ is disabled in SW, so it really invokes irq_mask callback now to disable in HW. >>>> 6. This mask callback doesn't operate on PDC (since in PDC, IRQs gets masked only during suspend, all other times its enabled) >>>> 7. step 3 to 6 repeats, if this IRQ keeps on coming and waking up from deep cpuidle states. >>> Ok so in summary, irq is left unmasked in pdc during deep cpu idle and >>> it keeps waking up the CPU because it isn't masked at the PDC after the >>> first time it interrupts? Is this a power problem? >> yes it can be a power problem. >>> Because from a >>> correctness standpoint we don't really care. It woke up the CPU because >>> it happened, and the GIC can decide to ignore it or not by masking it at >>> the GIC. I thought that the PDC wouldn't wake up the CPU if we masked >>> the irq at the GIC level. Is that not true? >> once PDC detects IRQ, it directly doesn't wake up CPU. it replays IRQ to >> GIC. >> >> since at GIC its masked, GIC doesn't forward to cpu to immediatly wake >> it up. >> >> however after PDC detecting IRQ, it exits low power mode and >> watchdog/timer can wakeup upon expiry. > Ok. So the only problem is some screaming irq that really wants to be > handled but the driver that requested it has disabled it at runtime. The > IRQ keeps kicking the CPUs out of deep idle and then eventually the > timer tick happens and we've run the CPUs in a shallower idle state for > this time? No it may still enter deeper state next time. > Presumably we'd like to have these irqs be lazily masked at > the PDC so that they can become pending when they first arrive but not > block deep idle states if they're interrupting often while being > handled. We do lazily disable IRQ. but didnot understand why lazily disable when they are being handled? The edge type irqs gets masked immediatly if one irq is being handled and another comes in. but that's not a problem. > > On the other hand, we want irq wake state to be the only factor in irqs > being unmasked at the PDC on the entry to suspend. Purely > masking/unmasking at the PDC when the irq is masked in software doesn't > work because suspend/resume will break for disabled but wake enabled > irqs. But doing that makes idle work easily because we can assume during > idle that leaving it unmasked until it fires and then masking it in the > PDC until it is handled gives us good deep idle states in the face of > screaming irqs. > > What are the actual requirements? Here is my attempt to boil this > discussion down into a few bullet points: > > 1. During system suspend, wake enabled irqs should be enabled in PDC > and all other irqs should be disabled in PDC. yes, IRQs should be enabled in both PDC and GIC before platform (PSCI suspend) happens if they are marked for wakeup (enable_irq_wake()) > > 2. During idle, enabled irqs must be enabled in PDC, unless they're > pending in which case they should be masked in the PDC so as to not > wake up the CPU from deep idle states i didn't get this point. During idle, if the driver choosen to keep IRQ enabled, it should be enabled in both PDC and GIC if the driver choosen to keep IRQ disabled, with this series... a. do a lay disable when driver's call disable_irq(), meaning set the SW state as disabled but leave in PDC and GIC HW as unmasked/enabled. b. if the IRQ comes inbetween and its of edge type, the generic handle_edge_irq will really mask in HW. > > 3. During non-idle, non-suspend, enabled irqs must be enabled in PDC. > > Or is #3 actually false and PDC has no bearing on this? Correct, During this time (non-idle, non-suspend) PDC will be in something called "by pass mode" where it plays role of type conversion. (a level low to level high / edge falling to edge rising) since GIC doesn't detect level low/falling edge IRQs. Thanks, Maulik
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 6ae9e1f..f7c0662 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -36,6 +36,7 @@ struct pdc_pin_region { u32 cnt; }; +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS); static DEFINE_RAW_SPINLOCK(pdc_lock); static void __iomem *pdc_base; static struct pdc_pin_region *pdc_region; @@ -87,22 +88,20 @@ static void pdc_enable_intr(struct irq_data *d, bool on) raw_spin_unlock(&pdc_lock); } -static void qcom_pdc_gic_disable(struct irq_data *d) +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on) { if (d->hwirq == GPIO_NO_WAKE_IRQ) - return; - - pdc_enable_intr(d, false); - irq_chip_disable_parent(d); -} + return 0; -static void qcom_pdc_gic_enable(struct irq_data *d) -{ - if (d->hwirq == GPIO_NO_WAKE_IRQ) - return; + if (on) { + pdc_enable_intr(d, true); + irq_chip_enable_parent(d); + set_bit(d->hwirq, pdc_wake_irqs); + } else { + clear_bit(d->hwirq, pdc_wake_irqs); + } - pdc_enable_intr(d, true); - irq_chip_enable_parent(d); + return irq_chip_set_wake_parent(d, on); } static void qcom_pdc_gic_mask(struct irq_data *d) @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d) if (d->hwirq == GPIO_NO_WAKE_IRQ) return; + if (!test_bit(d->hwirq, pdc_wake_irqs)) + pdc_enable_intr(d, false); + irq_chip_mask_parent(d); } @@ -118,6 +120,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d) if (d->hwirq == GPIO_NO_WAKE_IRQ) return; + pdc_enable_intr(d, true); irq_chip_unmask_parent(d); } @@ -197,15 +200,13 @@ static struct irq_chip qcom_pdc_gic_chip = { .irq_eoi = irq_chip_eoi_parent, .irq_mask = qcom_pdc_gic_mask, .irq_unmask = qcom_pdc_gic_unmask, - .irq_disable = qcom_pdc_gic_disable, - .irq_enable = qcom_pdc_gic_enable, .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state, .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = qcom_pdc_gic_set_type, + .irq_set_wake = qcom_pdc_gic_set_wake, .flags = IRQCHIP_MASK_ON_SUSPEND | - IRQCHIP_SET_TYPE_MASKED | - IRQCHIP_SKIP_SET_WAKE, + IRQCHIP_SET_TYPE_MASKED, .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, .irq_set_affinity = irq_chip_set_affinity_parent, };
Remove irq_disable callback to allow lazy disable for pdc interrupts. Add irq_set_wake callback that unmask interrupt in HW when drivers mark interrupt for wakeup. Interrupt will be cleared in HW during lazy disable if its not marked for wakeup. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/irqchip/qcom-pdc.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)