On Friday 30 October 2015 12:41:03 Quan Nguyen wrote:

On Thu, Oct 29, 2015 at 8:28 PM, Linus Walleij [off-list ref] wrote:

quoted

On Mon, Oct 26, 2015 at 7:49 AM, Y Vo [off-list ref] wrote:
(...)

quoted

+#define XGENE_HWIRQ_TO_GPIO(hwirq)     ((hwirq) + XGENE_GPIO8_IDX)
+#define XGENE_GPIO_TO_HWIRQ(gpio)      ((gpio) - XGENE_GPIO8_IDX)
+#define GIC_IRQ_TO_GPIO_IRQ(hwirq)     ((hwirq) - XGENE_GPIO8_HWIRQ)
+#define GPIO_IRQ_TO_GIC_IRQ(hwirq)     ((hwirq) + XGENE_GPIO8_HWIRQ)

I'm a bit uneasy about this, maybe I just don't get it.

What is this indexing into "GIC IRQ" that is starting to happen
here?

The GIC (if that is drivers/irqchip/irq-gic.c) has a totally dynamic IRQ
translation and the Linux IRQs it is using may change. I am worried
that there is some reliance here on Linux IRQ having certain numbers
because there is certainly no ABI like that.

Is this 0x48 really an index into the *hardware* offset in the GIC?

And if it is: why are we not getting this hardware information from the
device tree like all other interrupt numbers and offsets?

I'm worried about this.

The SPI40(0x48) through SPI45(0x4D) from GIC are mapped as external
IRQ0 - IRQ5 in this GPIO block, so it is hardware offset not mapped
irq number.

Below is detail:

+ GIC: SPI40 (hwirq=0x48)  <=> SB-GPIO: (hwirq=0) (gpio line 8)
+ GIC: SPI41 (hwirq=0x49)  <=> SB-GPIO: (hwirq=1) (gpio line 9)
...
+ GIC: SPI45 (hwirq=0x4D)  <=> SB-GPIO: (hwirq=5) (gpio line 13)

These defines are to help translating between Gic hardware irq and
SBGPIO hardware irq number.

But the numbers are already in DT, so don't duplicate them in the
source code but just use irq_of_parse_and_map() to get the parent
irq number.

quoted

quoted

-       u32 *irq;
+       void __iomem *regs;
+       struct irq_domain *gic_domain;
+       struct irq_domain *irq_domain;

And there is some secondary gic_domain here, which is confusing
since the GIC does have an IRQ domain too.

I think I want a clear explanation to how this GPIO controller interacts
with the GIC, and I want it to be part of the patch, as comments in the
code and in the commit message (which is way too small for a complex
patch like this).

Otherwise I have no chance to understand what is really going on here.

SBGPIO currently is not capable to mask/unmask/... irqs, so these will
rely on the parent (GIC) instead. To do so, we need keep a parent
domain reference here "struct irq_domain *gic_domain" so we can find
correspond parent irq in runtime.

Maybe rename the domain to 'parent_domain' or something like that
to avoid hardcoding any knowledge of the parent IRQ controller being
a GIC. If the same GPIO block gets reused on a PowerPC machine, we
want the driver to just work and not have any ARM specifics in it.

	Arnd