From: Marc Zyngier <marc.zyngier@arm.com>
To: Jon Hunter <jonathanh@nvidia.com>, John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
lkml <linux-kernel@vger.kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons
Date: Tue, 9 Aug 2016 16:08:00 +0100 [thread overview]
Message-ID: <57A9F1D0.3040900@arm.com> (raw)
In-Reply-To: <773ccdb0-a225-f2d0-eb5d-3c27243ee76b@nvidia.com>
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...
next prev parent reply other threads:[~2016-08-09 15:08 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-30 4:39 [Regression] "irqdomain: Don't set type when mapping an IRQ" breaks nexus7 gpio buttons John Stultz
2016-07-30 4:52 ` Bjorn Andersson
2016-07-30 11:10 ` Marc Zyngier
2016-07-30 8:07 ` Thomas Gleixner
2016-08-05 18:12 ` John Stultz
2016-08-08 9:04 ` Jon Hunter
2016-08-08 21:50 ` Linus Walleij
2016-08-08 21:35 ` Linus Walleij
2016-08-01 10:26 ` Jon Hunter
2016-08-05 23:45 ` John Stultz
2016-08-08 9:31 ` Jon Hunter
2016-08-09 4:25 ` John Stultz
2016-08-09 13:20 ` Jon Hunter
2016-08-09 15:08 ` Marc Zyngier [this message]
2016-08-09 23:03 ` Linus Walleij
2016-08-10 9:41 ` Marc Zyngier
2016-08-10 9:56 ` Jon Hunter
2016-08-10 10:21 ` Marc Zyngier
2016-08-10 13:58 ` Linus Walleij
2016-08-10 14:12 ` Jon Hunter
2016-08-10 22:06 ` Linus Walleij
2016-08-10 13:50 ` Linus Walleij
2016-08-10 15:17 ` Marc Zyngier
2016-08-10 22:14 ` Linus Walleij
2016-08-08 21:48 ` Linus Walleij
2016-08-11 8:37 ` Marc Zyngier
2016-08-11 9:47 ` Jon Hunter
2016-08-11 11:45 ` Marc Zyngier
2016-08-11 11:53 ` Marc Zyngier
2016-08-11 12:46 ` Marc Zyngier
2016-08-11 13:29 ` Jon Hunter
2016-08-11 13:34 ` Marc Zyngier
2016-08-11 15:32 ` John Stultz
2016-08-11 15:51 ` Marc Zyngier
2016-08-11 21:08 ` Linus Walleij
2016-08-11 21:23 ` Bjorn Andersson
2016-08-12 10:22 ` Marc Zyngier
2016-08-11 12:01 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57A9F1D0.3040900@arm.com \
--to=marc.zyngier@arm.com \
--cc=bjorn.andersson@linaro.org \
--cc=john.stultz@linaro.org \
--cc=jonathanh@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.