From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752609AbcHIPIG (ORCPT ); Tue, 9 Aug 2016 11:08:06 -0400 Received: from foss.arm.com ([217.140.101.70]:42863 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752325AbcHIPIE (ORCPT ); Tue, 9 Aug 2016 11:08:04 -0400 Subject: Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons To: Jon Hunter , John Stultz References: <566aa781-6364-07ba-054a-2fcce0f4331b@nvidia.com> <773ccdb0-a225-f2d0-eb5d-3c27243ee76b@nvidia.com> Cc: Thomas Gleixner , lkml , Bjorn Andersson From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <57A9F1D0.3040900@arm.com> Date: Tue, 9 Aug 2016 16:08:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <773ccdb0-a225-f2d0-eb5d-3c27243ee76b@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/08/16 14:20, Jon Hunter wrote: > > On 09/08/16 05:25, John Stultz wrote: > > ... > >> So actually no. We usually call irqd_set_trigger_type() but something >> still doesn't work. >> >> Interestingly, just adding irq_set_irq_type(virq, type); to the top of >> that block (leaving the rest of the code) also works. > > Interesting. By saving the trigger type during the mapping, we defer > setting the interrupt type to when the interrupt is requested. So this > would imply that the interrupt is not being setup as expected when its > requested. > >>> What is odd, is that the above sequence is only executed if a irq >>> mapping exists and so really, AFAICT this should not happen. Ie. the irq >>> descriptor should have been allocated for the mapping to exist. We >>> should probably warn if this happens. >>> >>> Without reverting the above, can you add a print to show the >>> domain->name, hwirq and virq information if !irq_data? That will confirm >>> the domain for us. >> >> So I put some printk info in (in either case since I'm never seeing >> the !irq_data case happen): >> >> [ 1.514217] JDB: virq: 93 hwirq: 74 domain name: msmgpio >> [ 1.838342] JDB: virq: 25 hwirq: 6 domain name: msmgpio >> >> Which is odd, looking at: >> >> shell@flo:/ $ cat /proc/interrupts >> CPU0 CPU1 CPU2 CPU3 >> 16: 1159 1138 1332 1574 GIC-0 18 Edge >> gp_timer >> 25: 0 0 0 0 msmgpio 6 Edge >> ekth3500 >> 111: 6 0 0 0 GIC-0 51 Edge >> qcom_rpm_ack >> 112: 0 0 0 0 GIC-0 53 Edge >> qcom_rpm_err >> 113: 0 0 0 0 GIC-0 54 Edge >> qcom_rpm_wakeup >> 114: 48 0 0 0 GIC-0 132 Edge >> msm_otg, ci_hdrc_msm >> 115: 796 0 0 0 GIC-0 130 Level bam_dma >> 116: 0 0 0 0 GIC-0 128 Level bam_dma >> 117: 0 0 0 0 GIC-0 127 Level bam_dma >> 118: 2627 0 0 0 GIC-0 136 Level >> mmci-pl18x (cmd) >> 119: 54 0 0 0 GIC-0 226 Level i2c_qup >> 120: 21 0 0 0 GIC-0 183 Level i2c_qup >> 122: 0 0 0 0 GIC-0 189 Level i2c_qup >> 123: 202 0 0 0 GIC-0 190 Level >> msm_serial0 >> 124: 0 0 0 0 GIC-0 70 Edge smsm >> 125: 0 0 0 0 GIC-0 121 Edge smsm >> 126: 0 0 0 0 GIC-0 236 Edge smsm >> 127: 0 0 0 0 GIC-0 169 Edge smsm >> 131: 0 0 0 0 pm8xxx 195 Edge >> Volume Up >> 165: 0 0 0 0 pm8xxx 229 Edge >> Volume Down >> 184: 0 0 0 0 pm8xxx 39 Edge >> pm8xxx_rtc_alarm >> 185: 0 0 0 0 pm8xxx 50 Edge >> pmic8xxx_pwrkey_release >> 186: 0 0 0 0 pm8xxx 51 Edge >> pmic8xxx_pwrkey_press >> IPI0: 0 1 1 1 CPU wakeup interrupts >> IPI1: 0 0 0 0 Timer broadcast interrupts >> IPI2: 944 539 1015 529 Rescheduling interrupts >> IPI3: 1 4 6 4 Function call interrupts >> IPI4: 0 0 0 0 CPU stop interrupts >> IPI5: 0 0 0 0 IRQ work interrupts >> IPI6: 0 0 0 0 completion interrupts >> Err: 0 >> >> Since 25 maps to the ekth3500 (touch panel, which is still working >> fine), but 93/74 doesn't seem to map to anything, and the problematic >> irqs are the volume keys 195/229 and power keys 50/51. > > So looking at the DT source, I believe that hwirq 74 (virq 93) is the > problem. This is the parent interrupt from the pm8xxx to the apq8064 > if it is not requested then the type is not set. It seems that for > parent interrupts these are not typically requested, but enabled when > an irqchip is chained. > > To confirm and for testing purposes I am curious if this works ... > > if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) { > irq_data = irq_get_irq_data(virq); > if (!irq_data) > return 0; > > - irqd_set_trigger_type(irq_data, type); > + if (hwirq == 74) > + irq_set_irq_type(virq, type); > + else > + irqd_set_trigger_type(irq_data, type); > return virq; > } > > If that works, then does the following also work (without the above) ... > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index b4c1bc7c9ca2..e111b72e3162 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -824,6 +824,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, > irq_settings_set_norequest(desc); > irq_settings_set_nothread(desc); > desc->action = &chained_action; > + __irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data)); > irq_startup(desc, true); > } > } > > It looks like there is a path for parent interrupts where the type > is not getting set. If the above works then we can discuss with Thomas > and Marc on the correct fix. This definitely looks like an something that is worth a patch anyway, as I otherwise don't see how we configure cascaded interrupts with the new deferred scheme. Thanks, M. -- Jazz is not dead. It just smells funny...