gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
v2 cover gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
V2 Changes:
- support X-Gene standby GPIO as an interrupt controller.
Y Vo (3):
gpio: xgene: add support to configure GPIO line as input, output or
external IRQ pin
Documentation: gpio: Update description for X-Gene standby GPIO
controller DTS binding
arm64: dts: Update APM X-Gene standby GPIO controller DTS entries
.../devicetree/bindings/gpio/gpio-xgene-sb.txt | 23 ++- arch/arm64/boot/dts/apm/apm-storm.dtsi | 15 +- drivers/gpio/gpio-xgene-sb.c | 234 +++++++++++++++++--- 3 files changed, 223 insertions(+), 49 deletions(-)
PATCH v2 1/3 gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
Add support to configure GPIO line as input, output or external IRQ pin.
--- drivers/gpio/gpio-xgene-sb.c | 234 +++++++++++++++++++++++++++++++++++------ 1 files changed, 199 insertions(+), 35 deletions(-)
drivers/gpio/gpio-xgene-sb.c +199 -35
@@ -4,6 +4,7 @@* Copyright (c) 2014, Applied Micro Circuits Corporation * Author: Tin Huynh <[email protected]>. * Y Vo <[email protected]>. +* Quan Nguyen <[email protected]>. * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the
@@ -32,14 +33,24 @@#define XGENE_MAX_GPIO_DS 22#define XGENE_MAX_GPIO_DS_IRQ 6 +#define XGENE_GPIO8_HWIRQ 0x48 +#define XGENE_GPIO8_IDX 8#define GPIO_MASK (x) (1U << ((x) % 32))#define MPA_GPIO_INT_LVL 0x0290#define MPA_GPIO_OE_ADDR 0x029c#define MPA_GPIO_OUT_ADDR 0x02a0 -#define MPA_GPIO_IN_ADDR 0x02a4 -#define MPA_GPIO_SEL_LO 0x0294 +#define MPA_GPIO_IN_ADDR 0x02a4 +#define MPA_GPIO_SEL_LO 0x0294 + +#define GPIO_INT_LVL_LEVEL_HIGH 0x000001 +#define GPIO_INT_LVL_LEVEL_LOW 0x000000 + +#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)/** * struct xgene_gpio_sb - GPIO-Standby private data structure.
@@ -49,18 +60,14 @@*/ struct xgene_gpio_sb {struct bgpio_chip bgc; -u32 *irq; +void __iomem *regs; +struct irq_domain *gic_domain; +struct irq_domain *irq_domain;u32 nirq; }; -static inline struct xgene_gpio_sb *to_xgene_gpio_sb (struct gpio_chip *gc) -{ -struct bgpio_chip *bgc =to_bgpio_chip (gc); - - return container_of(bgc, struct xgene_gpio_sb, bgc); -} - -static void xgene_gpio_set_bit (struct bgpio_chip *bgc,void __iomem *reg,u32 gpio,int val) +static void xgene_gpio_set_bit (struct bgpio_chip *bgc, +void __iomem *reg,u32 gpio,int val) {u32 data;
@@ -72,21 +79,152 @@ static void xgene_gpio_set_bit(struct bgpio_chip *bgc, void __iomem *reg, u32 gp bgc->write_reg (reg, data); } -static int apm_gpio_sb_to_irq (struct gpio_chip *gc,u32 gpio) +/* +* NOP functions +*/ +static void xgene_gpio_sb_nop (struct irq_data *data) { } + +static void xgene_gpio_sb_irq_mask (struct irq_data *d) +{ +unsigned int gic_irq; +struct irq_data *irqdata; +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); + + gic_irq =irq_find_mapping (priv->gic_domain, +GPIO_IRQ_TO_GIC_IRQ (d->hwirq)); + irqdata =irq_get_irq_data (gic_irq); +if (!irqdata || !irqdata->chip) +return ; + +if (irqdata->chip->irq_mask) + irqdata->chip->irq_mask (irqdata); +} + +static void xgene_gpio_sb_irq_unmask (struct irq_data *d) +{ +unsigned int gic_irq; +struct irq_data *irqdata; +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); + + gic_irq =irq_find_mapping (priv->gic_domain, +GPIO_IRQ_TO_GIC_IRQ (d->hwirq)); + irqdata =irq_get_irq_data (gic_irq); +if (!irqdata || !irqdata->chip) +return ; + +if (irqdata->chip->irq_unmask) + irqdata->chip->irq_unmask (irqdata); +} + +static int xgene_gpio_sb_irq_set_type (struct irq_data *d,unsigned int type) +{ +int hwirq = d->hwirq; +int gpio =XGENE_HWIRQ_TO_GPIO (hwirq); +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); +int lvl_type; +int ret; + +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_EDGE_RISING : +case IRQ_TYPE_LEVEL_HIGH : + lvl_type =GPIO_INT_LVL_LEVEL_HIGH ; +break ; +case IRQ_TYPE_EDGE_FALLING : +case IRQ_TYPE_LEVEL_LOW : + lvl_type =GPIO_INT_LVL_LEVEL_LOW ; +break ; +default : +return -EINVAL ; + } + + ret =gpiochip_lock_as_irq (&priv->bgc.gc, gpio); +if (ret) { +dev_err (priv->bgc.gc.dev, +"Unable to configure XGene GPIO standby pin %d as IRQ\n" , + gpio); +return ret; + } + +if ((gpio >=XGENE_GPIO8_IDX ) && + (hwirq <XGENE_MAX_GPIO_DS_IRQ )) { +xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_SEL_LO , + gpio *2 ,1 ); +xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_INT_LVL , + hwirq, lvl_type); + } +if (type & (IRQ_TYPE_EDGE_RISING |IRQ_TYPE_EDGE_FALLING )) +irq_set_handler_locked (d, handle_edge_irq); +else +irq_set_handler_locked (d, handle_level_irq); + +return 0 ; +} + +static void xgene_gpio_sb_irq_shutdown (struct irq_data *d) +{ +int gpio =XGENE_HWIRQ_TO_GPIO (d->hwirq); +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); + +gpiochip_unlock_as_irq (&priv->bgc.gc, gpio); +} + +static void xgene_gpio_sb_irq_handler (struct irq_desc *desc) +{ +unsigned int cascade_irq; +struct irq_chip *chip =irq_desc_get_chip (desc); +struct xgene_gpio_sb *priv =irq_desc_get_handler_data (desc); + +chained_irq_enter (chip, desc); + + cascade_irq =irq_find_mapping (priv->irq_domain, +GIC_IRQ_TO_GPIO_IRQ (irq_desc_get_irq_data (desc)->hwirq)); + +if (cascade_irq) +generic_handle_irq (cascade_irq); + +chained_irq_exit (chip, desc); +} + +static struct irq_chip xgene_gpio_sb_irq_chip = { + .name ="sbgpio" , + .irq_ack = xgene_gpio_sb_nop, + .irq_mask = xgene_gpio_sb_irq_mask, + .irq_unmask = xgene_gpio_sb_irq_unmask, + .irq_set_type = xgene_gpio_sb_irq_set_type, + .irq_shutdown = xgene_gpio_sb_irq_shutdown, +}; + +static inline struct xgene_gpio_sb *to_xgene_gpio_sb (struct gpio_chip *gc) +{ +struct bgpio_chip *bgc =to_bgpio_chip (gc); + + return container_of(bgc, struct xgene_gpio_sb, bgc); +} + +static int xgene_gpio_sb_to_irq (struct gpio_chip *gc,u32 gpio) {struct xgene_gpio_sb *priv =to_xgene_gpio_sb (gc); -if (priv->irq[gpio]) -return priv->irq[gpio]; +if ((gpio <XGENE_GPIO8_IDX ) || + (gpio >XGENE_MAX_GPIO_DS_IRQ +XGENE_GPIO8_IDX )) +return -ENXIO ; -return -ENXIO ; +return irq_find_mapping (priv->irq_domain,XGENE_GPIO_TO_HWIRQ (gpio)); +} + +static int xgene_irq_to_line (u32 irq) +{ +u32 offset =GIC_IRQ_TO_GPIO_IRQ (irq_get_irq_data (irq)->hwirq); + +return (offset <XGENE_MAX_GPIO_DS_IRQ ) ? + (offset +XGENE_GPIO8_IDX ) : -EINVAL ; }static int xgene_gpio_sb_probe (struct platform_device *pdev) {struct xgene_gpio_sb *priv;u32 ret, i; -u32 default_lines[] = {0x08 ,0x09 ,0x0A ,0x0B ,0x0C ,0x0D }; +int virq, line;struct resource *res;void __iomem *regs;
@@ -99,38 +237,63 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)if (IS_ERR (regs))return PTR_ERR (regs); + priv->regs = regs; + ret =bgpio_init (&priv->bgc, &pdev->dev,4 , regs +MPA_GPIO_IN_ADDR , regs +MPA_GPIO_OUT_ADDR ,NULL , regs +MPA_GPIO_OE_ADDR ,NULL ,0 ); -if (ret) -return ret; +if (ret) +return ret; - priv->bgc.gc.to_irq = apm_gpio_sb_to_irq; + priv->bgc.gc.to_irq = xgene_gpio_sb_to_irq; priv->bgc.gc.ngpio =XGENE_MAX_GPIO_DS ; - priv->nirq =XGENE_MAX_GPIO_DS_IRQ ; - - priv->irq =devm_kzalloc (&pdev->dev,sizeof (u32) *XGENE_MAX_GPIO_DS , -GFP_KERNEL ); -if (!priv->irq) -return -ENOMEM ; +/* Mapping and handling GIC irqs*/ +for (i =0 ; i <XGENE_MAX_GPIO_DS_IRQ ; i++) { + virq =platform_get_irq (pdev, i); +if (virq <0 ) +break ; + line =xgene_irq_to_line (virq); +if (line <XGENE_GPIO8_IDX ) +break ; + +irq_set_chained_handler_and_data (virq, + &xgene_gpio_sb_irq_handler, priv); + } -for (i =0 ; i < priv->nirq; i++) { - priv->irq[default_lines[i]] =platform_get_irq (pdev, i); -xgene_gpio_set_bit (&priv->bgc, regs +MPA_GPIO_SEL_LO , - default_lines[i] *2 ,1 ); -xgene_gpio_set_bit (&priv->bgc, regs +MPA_GPIO_INT_LVL , i,1 ); + priv->nirq = i; +if (priv->nirq >0 ) { + virq =platform_get_irq (pdev,0 ); + priv->gic_domain =irq_get_irq_data (virq)->domain; }platform_set_drvdata (pdev, priv); + priv->irq_domain =irq_domain_add_linear (pdev->dev.of_node, + priv->nirq, + &irq_domain_simple_ops, priv); +if (!priv->irq_domain) +return -ENODEV ; + ret =gpiochip_add (&priv->bgc.gc); -if (ret) -dev_err (&pdev->dev,"failed to register X-Gene GPIO Standby driver\n" ); -else +if (ret) { +dev_err (&pdev->dev, +"failed to register X-Gene GPIO Standby driver\n" ); +irq_domain_remove (priv->irq_domain); + }else dev_info (&pdev->dev,"X-Gene GPIO Standby driver registered\n" ); + priv->bgc.gc.irqdomain = priv->irq_domain; + +/* Init for new mapped irqs*/ +for (i =0 ; i < priv->nirq; i++) { +int irq =irq_create_mapping (priv->irq_domain, i); + +irq_set_chip_data (irq, priv); +irq_set_chip (irq, &xgene_gpio_sb_irq_chip); + } +if (priv->nirq >0 ) {/* Register interrupt handlers for gpio signaled acpi events */ acpi_gpiochip_request_interrupts (&priv->bgc.gc);
@@ -143,9 +306,10 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev) {struct xgene_gpio_sb *priv =platform_get_drvdata (pdev); -if (priv->nirq >0 ) { +if (priv->nirq >0 )acpi_gpiochip_free_interrupts (&priv->bgc.gc); - } + +irq_domain_remove (priv->irq_domain);return bgpio_remove (&priv->bgc); }
Signature · 1 line
1.7.1
PATCH v2 1/3 gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
Quoted 3 lines · Y Vo
Add support to configure GPIO line as input, output or external IRQ pin.
Signed-off-by Y Vo <[email protected]>
As I will say again, this description is too terse, add lots of information on how IRQs work on this controller please. I get confused.
(...)
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c +2
-0+#define XGENE_GPIO8_HWIRQ 0x48 +#define XGENE_GPIO8_IDX 8
(...)
drivers/gpio/gpio-xgene-sb.c line 53
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +4
-0+#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.
drivers/gpio/gpio-xgene-sb.c line 65
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +3
-1-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.
Quoted 33 lines
drivers/gpio/gpio-xgene-sb.c +33
-0+static int xgene_gpio_sb_irq_set_type (struct irq_data *d,unsigned int type) +{ +int hwirq = d->hwirq; +int gpio =XGENE_HWIRQ_TO_GPIO (hwirq); +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); +int lvl_type; +int ret; + +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_EDGE_RISING : +case IRQ_TYPE_LEVEL_HIGH : + lvl_type =GPIO_INT_LVL_LEVEL_HIGH ; +break ; +case IRQ_TYPE_EDGE_FALLING : +case IRQ_TYPE_LEVEL_LOW : + lvl_type =GPIO_INT_LVL_LEVEL_LOW ; +break ; +default : +return -EINVAL ; + } + + ret =gpiochip_lock_as_irq (&priv->bgc.gc, gpio); +if (ret) { +dev_err (priv->bgc.gc.dev, +"Unable to configure XGene GPIO standby pin %d as IRQ\n" , + gpio); +return ret; + } + +if ((gpio >=XGENE_GPIO8_IDX ) && + (hwirq <XGENE_MAX_GPIO_DS_IRQ )) { +xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_SEL_LO , + gpio *2 ,1 );
drivers/gpio/gpio-xgene-sb.c static void xgene_gpio_set_bit(struct bgpio_chip *bgc, void __iomem *reg, u32 gp · line 161
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c +10
-0+xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_INT_LVL , + hwirq, lvl_type); + } +if (type & (IRQ_TYPE_EDGE_RISING |IRQ_TYPE_EDGE_FALLING )) +irq_set_handler_locked (d, handle_edge_irq); +else +irq_set_handler_locked (d, handle_level_irq); + +return 0 ; +}
If you are assigning hadle_edge_irq() your irqchip *must* have an .irq_ack() callback that acknowledges the IRQs as they come in.
This makes me suspect that you haven't really tested edge interrupts, because if you did, the kernel would crash as it is unconditionally calling the .irq_ack() from handle_level_irq().
Yours,
Linus Walleij
PATCH v2 1/3 gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
Forgive me for not turn on plain text mode my last email.
Hi Linus,
My name is Quan Nguyen, I'm working with Y Vo on this patch.
Allow me to explain as below:
In current implementation, gic irq resources were used in both sbgpio and gpios-key nodes, and this causes confusion.
To avoid this, we introduce sbgpio driver as interrupt controller, it now provides 6 external irq pins mapped from gpio line 8-13. The gpio-keys node now uses sbgpio irq resources instead.
-- Quan
Quoted 8 lines · Linus Walleij
Quoted 3 lines · Y Vo
Add support to configure GPIO line as input, output or external IRQ pin.
Signed-off-by Y Vo <[email protected]>As I will say again, this description is too terse, add lots of information on how IRQs work on this controller please. I get confused.
How about:
+++++++++++++++++++++++++++++++++
Enable X-Gene standby GPIO driver as interrupt controller, it provides 6 external irq pins via gpio line 8-13.
+++++++++++++++++++++++++++++++++
Quoted 21 lines
(...)
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c +2
-0+#define XGENE_GPIO8_HWIRQ 0x48 +#define XGENE_GPIO8_IDX 8(...)
drivers/gpio/gpio-xgene-sb.c line 53
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +4
-0+#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.
Quoted 6 lines
Is this
0x48really 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.
Quoted 9 lines
drivers/gpio/gpio-xgene-sb.c line 65
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c +3
-1-u32 *irq; +void __iomem *regs; +struct irq_domain *gic_domain; +struct irq_domain *irq_domain;And there is some secondary
gic_domainhere, which is confusing since the GIC does have an IRQ domain too.
Quoted 6 lines
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.
Quoted 48 lines
Quoted 34 lines
drivers/gpio/gpio-xgene-sb.c +33
-0+static int xgene_gpio_sb_irq_set_type (struct irq_data *d,unsigned int type) +{ +int hwirq = d->hwirq; +int gpio =XGENE_HWIRQ_TO_GPIO (hwirq); +struct xgene_gpio_sb *priv =irq_data_get_irq_chip_data (d); +int lvl_type; +int ret; + +switch (type &IRQ_TYPE_SENSE_MASK ) { +case IRQ_TYPE_EDGE_RISING : +case IRQ_TYPE_LEVEL_HIGH : + lvl_type =GPIO_INT_LVL_LEVEL_HIGH ; +break ; +case IRQ_TYPE_EDGE_FALLING : +case IRQ_TYPE_LEVEL_LOW : + lvl_type =GPIO_INT_LVL_LEVEL_LOW ; +break ; +default : +return -EINVAL ; + } + + ret =gpiochip_lock_as_irq (&priv->bgc.gc, gpio); +if (ret) { +dev_err (priv->bgc.gc.dev, +"Unable to configure XGene GPIO standby pin %d as IRQ\n" , + gpio); +return ret; + } + +if ((gpio >=XGENE_GPIO8_IDX ) && + (hwirq <XGENE_MAX_GPIO_DS_IRQ )) { +xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_SEL_LO , + gpio *2 ,1 );drivers/gpio/gpio-xgene-sb.c static void xgene_gpio_set_bit(struct bgpio_chip *bgc, void __iomem *reg, u32 gp · line 161
Quoted 10 lines
drivers/gpio/gpio-xgene-sb.c +10
-0+xgene_gpio_set_bit (&priv->bgc, priv->regs +MPA_GPIO_INT_LVL , + hwirq, lvl_type); + } +if (type & (IRQ_TYPE_EDGE_RISING |IRQ_TYPE_EDGE_FALLING )) +irq_set_handler_locked (d, handle_edge_irq); +else +irq_set_handler_locked (d, handle_level_irq); + +return 0 ; +}If you are assigning
hadle_edge_irq()your irqchip *must* have an .irq_ack() callback that acknowledges the IRQs as they come in.
Quoted 3 lines
This makes me suspect that you haven't really tested edge interrupts, because if you did, the kernel would crash as it is unconditionally calling the .irq_ack() from
handle_level_irq().
irq_ack() callback is no-op function in this patch. +static struct irq_chip xgene_gpio_sb_irq_chip = {
+ .name = "sbgpio", + .irq_ack = xgene_gpio_sb_nop,
Quoted 3 lines
Yours,
Linus Walleij
PATCH v2 1/3 gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
Quoted 31 lines · On Friday 30 October 2015 12:41:03 Quan Nguyen wrote:
Quoted 17 lines · Linus Walleij
On Mon, Oct 26, 2015 at 7:49 AM, Y Vo <[email protected]> wrote:
(...)drivers/gpio/gpio-xgene-sb.c line 53
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +4
-0+#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.Quoted 6 lines
Is this
0x48really 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:
Quoted 7 lines
+ 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 13 lines
Quoted 13 lines
drivers/gpio/gpio-xgene-sb.c line 65
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +3
-1-u32 *irq; +void __iomem *regs; +struct irq_domain *gic_domain; +struct irq_domain *irq_domain;And there is some secondary
gic_domainhere, 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).
Quoted 6 lines
Quoted 1 line
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
PATCH v2 1/3 gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
In reply to Quan NguyenQuoted 88 lines · On Fri, 30 Oct 2015 12:41:03 +0700 Quan Nguyen <[email protected]> wrote:
Forgive me for not turn on plain text mode my last email.
Hi Linus,
My name is Quan Nguyen, I'm working with Y Vo on this patch.
Allow me to explain as below:
In current implementation, gic irq resources were used in both sbgpio and gpios-key nodes, and this causes confusion.
To avoid this, we introduce sbgpio driver as interrupt controller, it now provides 6 external irq pins mapped from gpio line 8-13. The gpio-keys node now uses sbgpio irq resources instead.-- Quan
Quoted 8 lines · Linus Walleij
Quoted 3 lines · Y Vo
Add support to configure GPIO line as input, output or external IRQ pin.
Signed-off-by Y Vo <[email protected]>As I will say again, this description is too terse, add lots of information on how IRQs work on this controller please. I get confused.
How about:
+++++++++++++++++++++++++++++++++
Enable X-Gene standby GPIO driver as interrupt controller, it provides 6 external irq pins via gpio line 8-13.
+++++++++++++++++++++++++++++++++Quoted 21 lines
(...)
Quoted 2 lines
drivers/gpio/gpio-xgene-sb.c +2
-0+#define XGENE_GPIO8_HWIRQ 0x48 +#define XGENE_GPIO8_IDX 8(...)
drivers/gpio/gpio-xgene-sb.c line 53
Quoted 4 lines
drivers/gpio/gpio-xgene-sb.c +4
-0+#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.Quoted 6 lines
Is this
0x48really 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.
Quoted 14 lines
drivers/gpio/gpio-xgene-sb.c line 65
Quoted 5 lines
drivers/gpio/gpio-xgene-sb.c +3
-1-u32 *irq; +void __iomem *regs; +struct irq_domain *gic_domain; +struct irq_domain *irq_domain;And there is some secondary
gic_domainhere, 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).
Quoted 6 lines
Quoted 1 line
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.
All this only means one thing: this should be implemented as a hierarchical domain, just like any other "random widget stacked on top of the GIC". Digging into the internals of the GIC driver is not acceptable anymore, and this patch is just horrible.
There is plenty of examples in the tree for you to look at and rewrite that code using the abstractions that are already in place.
In the meantime, I am NAKing this patch. Please include the irqchip maintainers in the next iteration of this series.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny.
PATCH v2 2/3 Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding
In reply to Y VoUpdate description for X-Gene standby GPIO controller DTS binding to support GPIO line configuration as input, output or external IRQ pin.
--- .../devicetree/bindings/gpio/gpio-xgene-sb.txt | 23 +++++++++++++------- 1 files changed, 15 insertions(+), 8 deletions(-)
Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt +15 -8
@@ -3,8 +3,7 @@ APM X-Gene Standby GPIO controller bindings This is a gpio controller in the standby domain. There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15, -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping -is currently 1-to-1 on interrupts 0x28 thru 0x2d. +only GPIO_DS8..GPIO_DS13 support interrupts. Required properties: - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
@@ -16,6 +15,11 @@ Required properties: 1 = active low - gpio-controller: Marks the device node as a GPIO controller. - interrupts: Shall contain exactly 6 interrupts. +- interrupt-parent: Phandle of the parent interrupt controller. +- interrupt-cells: Shoule be two. + - first cell is 0-5 coresponding for GPIO pin 8..13. + - second cell is used to specify flags. +- interrupt-controller: Marks the device node as an interrupt controller. Example: sbgpio: sbgpio at 17001000 {
@@ -23,10 +27,13 @@ Example: reg = <0x0 0x17001000 0x0 0x400>; #gpio-cells = <2>; gpio-controller; - interrupts = <0x0 0x28 0x1>, - <0x0 0x29 0x1>, - <0x0 0x2a 0x1>, - <0x0 0x2b 0x1>, - <0x0 0x2c 0x1>, - <0x0 0x2d 0x1>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x28 0x4>, + <0x0 0x29 0x4>, + <0x0 0x2a 0x4>, + <0x0 0x2b 0x4>, + <0x0 0x2c 0x4>, + <0x0 0x2d 0x4>; + #interrupt-cells = <2>; + interrupt-controller; };
Signature · 1 line
1.7.1
PATCH v2 3/3 arm64: dts: Update APM X-Gene standby GPIO controller DTS entries
In reply to Y VoUpdate APM X-Gene standby GPIO controller DTS entries to support GPIO line configuration as input, output or external IRQ pin.
--- arch/arm64/boot/dts/apm/apm-storm.dtsi | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/apm/apm-storm.dtsi +9 -6
@@ -765,12 +765,15 @@ reg = <0x0 0x17001000 0x0 0x400>; #gpio-cells = <2>; gpio-controller; - interrupts = <0x0 0x28 0x1>, - <0x0 0x29 0x1>, - <0x0 0x2a 0x1>, - <0x0 0x2b 0x1>, - <0x0 0x2c 0x1>, - <0x0 0x2d 0x1>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x28 0x4>, + <0x0 0x29 0x4>, + <0x0 0x2a 0x4>, + <0x0 0x2b 0x4>, + <0x0 0x2c 0x4>, + <0x0 0x2d 0x4>; + #interrupt-cells = <2>; + interrupt-controller; }; rtc: rtc at 10510000 {
Signature · 1 line
1.7.1