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...
