On Wed, Jul 29, 2020 at 3:58 PM Serge Semin
[off-list ref] wrote:

On Mon, Jul 27, 2020 at 12:22:28AM +0200, Linus Walleij wrote:

...

Sorry for a delay with a response to this issue. I had to give it a more thorough
thought since the problem is a bit more complex than it seemed originally. As I
see it now It might be wrong to implement the cases 2) and 3), but 1) is more
appropriate.

First of all we need to note that GPIOlib framework provides the next parameters
to describe the IRQ-chip:
gc->irq.num_parents - number of parental IRQ numbers.
gc->irq.parents[] - array of parental IRQ numbers.
*gc->irq.valid_mask - a mask of IRQ/GPIO lines describing a valid IRQ.
*gc->irq.map - mapping of hw IRQ/GPIO ID -> parental IRQ numbers.

Using that set we can handle any case of linear and sparse parental IRQs. Here
is how it can be implemented in the framework of DW APB GPIO controller.

DW APB GPIO can be synthesized with two configs:
1) Combined IRQ line (GPIO_INTR_IO == True),
2) Multiple interrupt signals for each GPIO (GPIO_INTR_IO == False).

Obviously the former case is trivial:

     IRQ_combined
    ______^________
   /_ _ _ _ _ ___ _\
   |_|_|_|_|_|...|_| - GPIOs

In that case
gc->irq.num_parents = 1;
gc->irq.parents[0] = IRQ_combined;
*gc->irq.valid_mask = GENMASK(ngpio - 1, 0); // This is done by the GPIOlib core itself.

The later one (when multiple interrupt signals are involved) can be a bit more
complicated. It can be also split up into two cases:
2a) One-on-one GPIO-IRQ mapping.
2b) Sparse GPIO-IRQ mapping.

It's straightforward to implement 2a):

   i1i2i3i4i5 ... iN
    _ _ _ _ _ ___ _
   |_|_|_|_|_|...|_| - GPIOs

In that case
gc->irq.num_parents = ngpio;
gc->irq.parents[] = {i1, i2, i3, i4, i5, ... iN};
gc->irq.map = {i1, i2, i3, i4, i5, ... iN};
*gc->irq.valid_mask = GENMASK(ngpio - 1, 0);

This case puzzles me. Why is it not NULL and 0 and actually you handle
everything as a nested case?

The complication starts when we get to implementing 2b):

   i1 xi3i4 x ... iN
    _ _ _ _ _ ___ _
   |_|_|_|_|_|...|_| - GPIOs

So does this.

Valid mask will define exactly GPIOs that are IRQs. So, we will handle
only nested IRQs which are valid.

In order to cover this case we need to answer on two question.
Firstly how to get such platform config? I am not sure about ACPI, but
aside from straightforward platform_data-based setup such configuration
can be reached by setting up the "interrupts-extended" DT-property with
zeroed phandle.

Ok, since it's possible to meet such platform config, we need to think
how to handle it and here is the second question. How to describe such
case in the framework of GPIOlib-IRQchip?

So from my side it was wrong to set the sparse IRQs array to
gc->irq.parents. Instead I should have scanned the sparse IRQs array,
calculated the number of non-empty parental IRQs, created an array of linear
(non-sparse) IRQs, initialized *gc->irq.valid_mask in accordance with the
sparse parental IRQs array. In other words it was wrong to assume, that
each gc->irq.parents entry corresponds to the IRQ/GPIO line. The gc->irq.parents
array just describes the parental IRQs and nothing else.

Shortly speaking here is how the GPIOlib IRQchip parameters should be
initialized in this case:
gc->irq.num_parents - number of valid parental IRQs.
gc->irq.parents - non-sparse, linear array of valid IRQs.
*gc->irq.valid_mask - mask initialized by means of the gc->irq.init_valid_mask()
callback, which indicates valid IRQ/GPIO IDs.
*gc->irq.map - sparse array of parental IRQ numbers (which I mistakenly tried to
pass through the gc->irq.parents pointer).

After that GPIOlib IRQchip should work just fine without need to be patched
in order to check whether the passed parental IRQs are valid or not.

Please correct me if I am wrong in some aspects of the solution described above.
I'll send a fix of the problem shortly.

Maybe I'm missing something, but looks like you are solving the issue
which is not so complex / doesn't exist.

-- 
With Best Regards,
Andy Shevchenko