On 25/10/16 15:22, Jerome Brunet wrote:

On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:

quoted

On 25/10/16 14:08, Jerome Brunet wrote:

quoted

On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:

quoted

quoted

On 25/10/16 10:14, Linus Walleij wrote:

quoted

On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
re.c
om> wrote:

quoted

quoted

Isn't this usecase (also as described in the cover letter)
a
textbook
example of when you should be using hierarchical irqdomain?

Please check with Marc et al on hierarchical irqdomains.

Linus,
Do you mean I should create a new hierarchical irqdomains in
each
of
the two pinctrl instances we have in these SoC, these domains
being
stacked on the one I just added for controller in irqchip ?

I did not understand this is what you meant when I asked you
the
question at ELCE.

Honestly, I do not understand when and where to properly use
hierarchical irqdomain, even after Marc's talk at ELC-E.

I probably didn't do that good a job explaining it then. Let's
try
again. You want to use hierarchical domains when you want to
describe
an
interrupt whose path traverses multiple controllers without ever
being
multiplexed with other signals. As long as you have this 1:1
relationship between controllers, you can use them.

Linus, Marc,

The calculation is question here is meant to get the appropriate
hwirq
number from a particular gpio (and deal with the gpios that can't
provide an irq at all). 

If I look at other gpio drivers, many are doing exactly this kind
of
calculation before calling 'irq_create_mapping' in the to_irq
callback.
For example:
- pinctrl/nomadik/pinctrl-abx500.c
- pinctrl/samsung/pinctrl-exynos5440.c

Some can afford to create all the mappings in the probe and just
call
'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
here. We
have only 8 upstream irqs for 130+ pins, so only 8 mappings
possible at
a time. 

My understanding is that irqdomain provide a way to map hwirq to
linux
virq (and back), not map gpio number to hwirq, right?

But why are those number different? Why don't you use the same
namespace? If gpio == hwirq, all your problems are already solved. If
you don't find the mapping in the irqdomain, then there is no irq,
end
of story. What am I missing?

There is a few problems to guarantee that gpio == hwirq.
1. We have 2 instances of pinctrl, to guarantee that the linux gpio
number == hwirq, we would have to guarantee the order in which they are
probed. At least this my understanding 

Maybe I wasn't clear enough, and my use of gpio is probably wrong. So
Linux has a gpio number, which is obviously an abstract number (just
like the Linux irq number). But the pad number, in the context of given
SoC, is constant. So we have:

	pad->gpio
	hwirq->irq

Why can't you have pad == hwirq, always? This is already what you have
in the irqchip driver. This would simplify a lot of things.

2. Inside each instance, we may have banks that can't have irq. We even
have a bank on meson8b which has a mixed state (the last pins don't
have irqs, while the first ones do). those banks/pins are still valid
gpios with gpio numbers. This introduce a shift between the gpio
numbering and the hwirq.

The point of this calculation is take the offset given to the 'to_irq'
callback, remove the gpio bank base number and add irq base number.
There is a few trick added to handled the case in 2.

You seem to assume linear mappings, which is pretty dangerous.

In addition, to call 'irq_find_mapping', we would first have to create
the mapping, in the probe I suppose. This would call the allocate
callback of the domain, in which we can allocate only 8 interrupts.

That's why I create the mapping in the .to_irq callback.

Is gpio_to_irq() supposed to allocate an interrupt? Or merely to report
the existence of a mapping?

quoted

quoted

Even if I implement an another irqdomain at the gpio level, I would
still have to perform this kind of calculation, one way or the
other.

quoted

quoted

Which is problematic since quite a few GPIO drivers now
need to use them.

I will review his slides, in the meantime I would say: whatever
Marc ACKs is fine with me. I trust this guy 100%. So I guess I
could ask that he ACK the entire chain of patches
from GIC->specialchip->GPIO.

Actually this discussion go me thinking about another issue we have
with this hardware.
We are looking for a way to implement support for
IRQ_TYPE_EDGE_BOTH
(needed for things like gpio-keys or mmc card detect). 
The controller can do each edge but not both at the same time.
I'm thinking that implementing another irqdomain at the gpio level
would allow to properly check the pad level in the EOI callback
then
set the next expected edge type accordingly (using
'irq_chip_set_type_parent')

Would it be acceptable ?

I really don't see what another irqdomain brings to the table. This
is
not a separate piece of HW, so the hwirq:irq mapping is still the
same.
I fail to see what the benefit is.

The separate piece of hw is the gpio itself. 
The irq-controller (in irqchip) can't read the gpio state because it is
simply another device.

The domain I'm thinking about wouldn't do much, I reckon. 
It would just register an irqchip which would only implement eoi.
For everything else, it would rely the parent controller.
From your explanation, I understood this is the purpose of hierarchy
domains, For each hw in the chain to handle only what it can, and rely
on its parent for the rest.

Right. But that doesn't make it more reliable, see below.

quoted

quoted

It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
similar
way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)

Being already done doesn't make it reliable. If the line goes low
between latching the rising edge and reprogramming the trigger,
you've
lost at least *two* interrupts (the falling edge and the following
rising edge).

If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes
low between latching rising edge and handling the interrupt, wouldn't
you miss the falling edge anyway ? The signal is just going too fast
the HW to handle everything.

That's the role of the HW to ensure that you don't loose any interrupt,
up to the sampling frequency of the controller. Doing it in SW is always
going to be a wonderful case of "it mostly work".

For the second rising edge, I disagree, it would not be lost, since we
would read the pad state, get a low level, and reprogram the controller
for another rising edge.

You always have a race between checking your level and switching the
configuration, which is likely to be slow anyway. In the meantime, the
level has changed and you're dead.

Anyway, I suggest you focus on getting something simple up and running,
and postpone the funky (read broken) stuff for later, once you have
something that looks vaguely sane (because we're not quite there yet).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...