Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains

[Marc Zyngier <maz@kernel.org>]

On 26/08/2019 21:14, Lina Iyer wrote:
> On Mon, Aug 26 2019 at 13:02 -0600, Marc Zyngier wrote:
>> On Mon, 26 Aug 2019 10:15:26 -0600
>> Lina Iyer <ilina@codeaurora.org> wrote:
>>
>>> On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
>>>> On 23/08/2019 09:24, Linus Walleij wrote:
>>>>> Hi Brian,
>>>>>
>>>>> I tried to look into this ssbi-gpio problem...
>>>>>
>>>>> Paging in Marc Z as well.
>>>>>
>>>>> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
>>>>>
>>>>>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
>>>>>> this little snippet that's different from spmi-gpio:
>>>>>>
>>>>>>         [ fwspec mapping code ]
>>>>>>
>>>>>>         /*
>>>>>>          * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
>>>>>>          * line level.
>>>>>>          */
>>>>>>         pin->irq = ret;
>>>>>>
>>>>>> Here's the relevant code in pm8xxx_gpio_get():
>>>>>>
>>>>>>         if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
>>>>>>                 ret = pin->output_value;
>>>>>>         } else if (pin->irq >= 0) {
>>>>>>                 ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
>>>>>>                 ...
>>>>>>         }
>>>>>
>>>>> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
>>>>> the global irq numbers and the internal function using irqdesc
>>>>> __irq_get_irqchip_state() is not exported.
>>>>>
>>>>> We should be encouraged to operate on IRQ descriptors rather
>>>>> than IRQ numbers when doing this kind of deep core work,
>>>>> right?
>>>>
>>>> Certainly, I'd like to gradually move over from the IRQ number to an
>>>> irq_desc. In interrupt-heavy contexts, this ends up being quite time
>>>> consuming. I have an old patch series somewhere changing irq domains to
>>>> use irq_descs internally instead of IRQ numbers, which I should maybe
>>>> revive.
>>>>
>>>> As for __irq_get_irqchip_state, the main issue is that it doesn't
>>>> perform any locking on its own, so you'd have to either provide your
>>>> own, or wrap it with something else.
>>>>
>>> Marc, on a related note, What are your thoughts on a
>>> irq_set_irqchip_state? We are running into an issue where chip state
>>> clear is required as well as clearing out that of the parent. Do you see
>>> problems in doing that from an irqchip driver?
>>
>> [desperately trying to switch to my korg address...]
>>
>> I'm not sure I fully get the question: if this is whether it is worth
>> implementing it, it usually is a good idea if the HW supports is. But I
>> have the feeling that this isn't your question, so maybe explaining
>> your use-case would help.
>>
>> Do you mean using irq_set_irqchip_state from within an irqchip driver?
>> That'd be quite odd, as this function is usually used when something
>> *outside* of the irqchip driver needs to poke at some internal state
>> because "it knows best" (see for example what KVM does with the active
>> state).
>>
> Yeah, I meant to say, when invoked on a parent irqchip from a child
> irqchip.
> 
> In my case when GPIO is used as an interrupt. Even though the interrupt
> is not enabled, the GPIO could cause the GIC_ISPEND to be set at GIC. We
> were wondering if we could use the irq_set_irqchip_state to clear
> pending state at GPIO's irqchip parent and all the way to GIC,  when the
> GPIO is enabled as an interrupt.

You can of course chain the call from one level to its parent. It's a
bit odd to have to clear a pending bit in this context, but I guess
that's a HW-specific limitation.

	M.
-- 
Jazz is not dead, it just smells funny...