| Message ID | 20220511183210.5248-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Renesas RZ/G2L IRQC support | expand |
Hi Prabhakar, Thanks for the patch. > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to > handle GPIO interrupt > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be used as > IRQ lines at given time. Selection of pins as IRQ lines is handled by IA55 > (which is the IRQC block) which sits in between the GPIO and GIC. Do we need to update bindings with interrupt-cells on [1] like [2] as it act as parent for GPIO interrupts? [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/pinctrl/renesas,rzg2l-pinctrl.yaml?h=next-20220511 [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/gpio/renesas,rcar-gpio.yaml?h=next-20220511#n81 Cheers, Biju > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++ > 1 file changed, 202 insertions(+) > > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > index a48cac55152c..af2c739cdbaa 100644 > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -9,8 +9,10 @@ > #include <linux/clk.h> > #include <linux/gpio/driver.h> > #include <linux/io.h> > +#include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <linux/pinctrl/pinconf-generic.h> #include > <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -89,6 > +91,7 @@ > #define PIN(n) (0x0800 + 0x10 + (n)) > #define IOLH(n) (0x1000 + (n) * 8) > #define IEN(n) (0x1800 + (n) * 8) > +#define ISEL(n) (0x2c80 + (n) * 8) > #define PWPR (0x3014) > #define SD_CH(n) (0x3000 + (n) * 4) > #define QSPI (0x3008) > @@ -112,6 +115,10 @@ > #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) + > 0x10) > #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT) > > +#define RZG2L_TINT_MAX_INTERRUPT 32 > +#define RZG2L_TINT_IRQ_START_INDEX 9 > +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i)) > + > struct rzg2l_dedicated_configs { > const char *name; > u32 config; > @@ -137,6 +144,9 @@ struct rzg2l_pinctrl { > > struct gpio_chip gpio_chip; > struct pinctrl_gpio_range gpio_range; > + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT); > + spinlock_t bitmap_lock; > + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; > > spinlock_t lock; > }; > @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, > unsigned int offset) > > static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset) > { > + unsigned int virq; > + > pinctrl_gpio_free(chip->base + offset); > > /* > @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, > unsigned int offset) > * drive the GPIO pin as an output. > */ > rzg2l_gpio_direction_input(chip, offset); > + > + virq = irq_find_mapping(chip->irq.domain, offset); > + if (virq) > + irq_dispose_mapping(virq); > } > > static const char * const rzg2l_gpio_names[] = { @@ -1104,14 +1120,190 @@ > static struct { > } > }; > > +static int rzg2l_gpio_get_gpioint(unsigned int virq) { > + unsigned int gpioint; > + unsigned int i; > + u32 port, bit; > + > + port = virq / 8; > + bit = virq % 8; > + > + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) || > + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port])) > + return -EINVAL; > + > + gpioint = bit; > + for (i = 0; i < port; i++) > + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]); > + > + return gpioint; > +} > + > +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned > int virq, > + unsigned int nr_irqs) > +{ > + struct irq_data *d; > + > + d = irq_domain_get_irq_data(domain, virq); > + if (d) { > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct > rzg2l_pinctrl, gpio_chip); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + unsigned int i; > + > + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) { > + if (pctrl->hwirq[i] == hwirq) { > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > + bitmap_release_region(pctrl->tint_slot, i, > get_order(1)); > + spin_unlock_irqrestore(&pctrl->bitmap_lock, > flags); > + pctrl->hwirq[i] = 0; > + break; > + } > + } > + } > + irq_domain_free_irqs_common(domain, virq, nr_irqs); } > + > +static void rzg2l_gpio_irq_disable(struct irq_data *d) { > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, > gpio_chip); > + unsigned int hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + void __iomem *addr; > + u32 port; > + u8 bit; > + > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > + > + addr = pctrl->base + ISEL(port); > + if (bit >= 4) { > + bit -= 4; > + addr += 4; > + } > + > + spin_lock_irqsave(&pctrl->lock, flags); > + writel(readl(addr) & ~BIT(bit * 8), addr); > + spin_unlock_irqrestore(&pctrl->lock, flags); > + > + irq_chip_disable_parent(d); > +} > + > +static void rzg2l_gpio_irq_enable(struct irq_data *d) { > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, > gpio_chip); > + unsigned int hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + void __iomem *addr; > + u32 port; > + u8 bit; > + > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > + > + addr = pctrl->base + ISEL(port); > + if (bit >= 4) { > + bit -= 4; > + addr += 4; > + } > + > + spin_lock_irqsave(&pctrl->lock, flags); > + writel(readl(addr) | BIT(bit * 8), addr); > + spin_unlock_irqrestore(&pctrl->lock, flags); > + > + irq_chip_enable_parent(d); > +} > + > +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int > +type) { > + return irq_chip_set_type_parent(d, type); } > + > +static void rzg2l_gpio_irqc_eoi(struct irq_data *d) { > + irq_chip_eoi_parent(d); > +} > + > +static struct irq_chip rzg2l_gpio_irqchip = { > + .name = "rzg2l-gpio", > + .irq_disable = rzg2l_gpio_irq_disable, > + .irq_enable = rzg2l_gpio_irq_enable, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_set_type = rzg2l_gpio_irq_set_type, > + .irq_eoi = rzg2l_gpio_irqc_eoi, > +}; > + > +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > + unsigned int child, > + unsigned int child_type, > + unsigned int *parent, > + unsigned int *parent_type) > +{ > + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc); > + unsigned long flags; > + int gpioint, irq; > + > + gpioint = rzg2l_gpio_get_gpioint(child); > + if (gpioint < 0) > + return gpioint; > + > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > + irq = bitmap_find_free_region(pctrl->tint_slot, > RZG2L_TINT_MAX_INTERRUPT, get_order(1)); > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags); > + if (irq < 0) > + return -ENOSPC; > + pctrl->hwirq[irq] = child; > + irq += RZG2L_TINT_IRQ_START_INDEX; > + > + /* All these interrupts are level high in the CPU */ > + *parent_type = IRQ_TYPE_LEVEL_HIGH; > + *parent = RZG2L_PACK_HWIRQ(gpioint, irq); > + return 0; > +} > + > +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip, > + unsigned int parent_hwirq, > + unsigned int parent_type) > +{ > + struct irq_fwspec *fwspec; > + > + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); > + if (!fwspec) > + return NULL; > + > + fwspec->fwnode = chip->irq.parent_domain->fwnode; > + fwspec->param_count = 2; > + fwspec->param[0] = parent_hwirq; > + fwspec->param[1] = parent_type; > + > + return fwspec; > +} > + > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) { > struct device_node *np = pctrl->dev->of_node; > struct gpio_chip *chip = &pctrl->gpio_chip; > const char *name = dev_name(pctrl->dev); > + struct irq_domain *parent_domain; > struct of_phandle_args of_args; > + struct device_node *parent_np; > + struct gpio_irq_chip *girq; > int ret; > > + parent_np = of_irq_find_parent(np); > + if (!parent_np) > + return -ENXIO; > + > + parent_domain = irq_find_host(parent_np); > + of_node_put(parent_np); > + if (!parent_domain) > + return -EPROBE_DEFER; > + > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, > &of_args); > if (ret) { > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); @@ - > 1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl > *pctrl) > chip->base = -1; > chip->ngpio = of_args.args[2]; > > + girq = &chip->irq; > + girq->chip = &rzg2l_gpio_irqchip; > + girq->fwnode = of_node_to_fwnode(np); > + girq->parent_domain = parent_domain; > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > + > pctrl->gpio_range.id = 0; > pctrl->gpio_range.pin_base = 0; > pctrl->gpio_range.base = 0; > @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device > *pdev) > } > > spin_lock_init(&pctrl->lock); > + spin_lock_init(&pctrl->bitmap_lock); > > platform_set_drvdata(pdev, pctrl); > > -- > 2.25.1
Hi Prabhakar, > Subject: [PATCH v3 1/5] dt-bindings: interrupt-controller: Add Renesas > RZ/G2L Interrupt Controller > > Add DT bindings for the Renesas RZ/G2L Interrupt Controller. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Rob Herring <robh@kernel.org> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > .../renesas,rzg2l-irqc.yaml | 134 ++++++++++++++++++ > 1 file changed, 134 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt- > controller/renesas,rzg2l-irqc.yaml > > diff --git a/Documentation/devicetree/bindings/interrupt- > controller/renesas,rzg2l-irqc.yaml > b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l- > irqc.yaml > new file mode 100644 > index 000000000000..02bfd631e532 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg > +++ 2l-irqc.yaml > @@ -0,0 +1,134 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > +--- > +$id: > + > +title: Renesas RZ/G2L (and alike SoC's) Interrupt Controller (IA55) > + > +maintainers: > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > + - Geert Uytterhoeven <geert+renesas@glider.be> > + > +description: | > + IA55 performs various interrupt controls including synchronization > +for the external > + interrupts of NMI, IRQ, and GPIOINT and the interrupts of the > +built-in peripheral > + interrupts output by each IP. And it notifies the interrupt to the GIC > + - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI > interrupts > + - GPIO pins used as external interrupt input pins, mapped to 32 GIC > SPI interrupts > + - NMI edge select (NMI is not treated as NMI exception and supports > fall edge and > + stand-up edge detection interrupts) > + > +allOf: > + - $ref: /schemas/interrupt-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - renesas,r9a07g044-irqc # RZ/G2L > + - const: renesas,rzg2l-irqc > + > + '#interrupt-cells': > + description: The first cell should contain interrupt number and the > second cell > + is used to specify the flag. > + const: 2 As DT user will fill interrupt cells only for IRQ case, for GPIOINT case it Will be handled in pin controller and pin controller maps to TINT which is internal to this driver, So do we need to explicitly mention it as Interrupt number of IRQ here?? Cheers, Biju > + > + > + '#address-cells': > + const: 0 > + > + interrupt-controller: true > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 41 > + > + clocks: > + maxItems: 2 > + > + clock-names: > + items: > + - const: clk > + - const: pclk > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > +required: > + - compatible > + - '#interrupt-cells' > + - '#address-cells' > + - interrupt-controller > + - reg > + - interrupts > + - clocks > + - clock-names > + - power-domains > + - resets > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/r9a07g044-cpg.h> > + > + irqc: interrupt-controller@110a0000 { > + compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc"; > + reg = <0x110a0000 0x10000>; > + #interrupt-cells = <2>; > + #address-cells = <0>; > + interrupt-controller; > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>, > + <&cpg CPG_MOD R9A07G044_IA55_PCLK>; > + clock-names = "clk", "pclk"; > + power-domains = <&cpg>; > + resets = <&cpg R9A07G044_IA55_RESETN>; > + }; > -- > 2.25.1
Hi Prabhakar, On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add DT bindings for the Renesas RZ/G2L Interrupt Controller. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for the update! > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml > @@ -0,0 +1,134 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interrupt-controller/renesas,rzg2l-irqc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Renesas RZ/G2L (and alike SoC's) Interrupt Controller (IA55) > + > +maintainers: > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > + - Geert Uytterhoeven <geert+renesas@glider.be> > + > +description: | > + IA55 performs various interrupt controls including synchronization for the external > + interrupts of NMI, IRQ, and GPIOINT and the interrupts of the built-in peripheral > + interrupts output by each IP. And it notifies the interrupt to the GIC > + - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI interrupts > + - GPIO pins used as external interrupt input pins, mapped to 32 GIC SPI interrupts > + - NMI edge select (NMI is not treated as NMI exception and supports fall edge and > + stand-up edge detection interrupts) > + > +allOf: > + - $ref: /schemas/interrupt-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - renesas,r9a07g044-irqc # RZ/G2L > + - const: renesas,rzg2l-irqc > + > + '#interrupt-cells': > + description: The first cell should contain interrupt number and the second cell > + is used to specify the flag. The important part is still missing: which interrupt number (the general description mentions 3 types)? I believe the answer is "external interrupt number". > + const: 2 > + > + Double blank line. > + '#address-cells': > + const: 0 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Allow free() callback to be overridden from irq_domain_ops for > hierarchical chips. > > This allows drivers to free any resources which are allocated during > populate_parent_alloc_arg(). > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Prabhakar, On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. domain > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > used as IRQ lines at given time. Selection of pins as IRQ lines at a given time > is handled by IA55 (which is the IRQC block) which sits in between the > GPIO and GIC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > { > struct device_node *np = pctrl->dev->of_node; > struct gpio_chip *chip = &pctrl->gpio_chip; > const char *name = dev_name(pctrl->dev); > + struct irq_domain *parent_domain; > struct of_phandle_args of_args; > + struct device_node *parent_np; > + struct gpio_irq_chip *girq; > int ret; > > + parent_np = of_irq_find_parent(np); > + if (!parent_np) > + return -ENXIO; > + > + parent_domain = irq_find_host(parent_np); > + of_node_put(parent_np); > + if (!parent_domain) > + return -EPROBE_DEFER; > + > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > if (ret) { > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > chip->base = -1; > chip->ngpio = of_args.args[2]; > > + girq = &chip->irq; > + girq->chip = &rzg2l_gpio_irqchip; > + girq->fwnode = of_node_to_fwnode(np); > + girq->parent_domain = parent_domain; > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > + I think you need to provide a .init_valid_mask() callback, as gpiochip_irqchip_remove() relies on that for destroying interrupts. However, the mask will need to be dynamic, as GPIO interrupts can be mapped and unmapped to one of the 32 available interrupts dynamically, right? I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() is ever called too early, before the mapping is done, it would fail. > pctrl->gpio_range.id = 0; > pctrl->gpio_range.pin_base = 0; > pctrl->gpio_range.base = 0; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, 11 May 2022 19:32:10 +0100, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > used as IRQ lines at given time. Selection of pins as IRQ lines > is handled by IA55 (which is the IRQC block) which sits in between the > GPIO and GIC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++ > 1 file changed, 202 insertions(+) > > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > index a48cac55152c..af2c739cdbaa 100644 > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -9,8 +9,10 @@ > #include <linux/clk.h> > #include <linux/gpio/driver.h> > #include <linux/io.h> > +#include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <linux/pinctrl/pinconf-generic.h> > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinctrl.h> > @@ -89,6 +91,7 @@ > #define PIN(n) (0x0800 + 0x10 + (n)) > #define IOLH(n) (0x1000 + (n) * 8) > #define IEN(n) (0x1800 + (n) * 8) > +#define ISEL(n) (0x2c80 + (n) * 8) > #define PWPR (0x3014) > #define SD_CH(n) (0x3000 + (n) * 4) > #define QSPI (0x3008) > @@ -112,6 +115,10 @@ > #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) + 0x10) > #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT) > > +#define RZG2L_TINT_MAX_INTERRUPT 32 > +#define RZG2L_TINT_IRQ_START_INDEX 9 > +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i)) > + > struct rzg2l_dedicated_configs { > const char *name; > u32 config; > @@ -137,6 +144,9 @@ struct rzg2l_pinctrl { > > struct gpio_chip gpio_chip; > struct pinctrl_gpio_range gpio_range; > + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT); > + spinlock_t bitmap_lock; > + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; > > spinlock_t lock; > }; > @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset) > > static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset) > { > + unsigned int virq; > + > pinctrl_gpio_free(chip->base + offset); > > /* > @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset) > * drive the GPIO pin as an output. > */ > rzg2l_gpio_direction_input(chip, offset); > + > + virq = irq_find_mapping(chip->irq.domain, offset); > + if (virq) > + irq_dispose_mapping(virq); > } > > static const char * const rzg2l_gpio_names[] = { > @@ -1104,14 +1120,190 @@ static struct { > } > }; > > +static int rzg2l_gpio_get_gpioint(unsigned int virq) > +{ > + unsigned int gpioint; > + unsigned int i; > + u32 port, bit; > + > + port = virq / 8; > + bit = virq % 8; > + > + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) || > + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port])) > + return -EINVAL; > + > + gpioint = bit; > + for (i = 0; i < port; i++) > + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]); > + > + return gpioint; > +} > + > +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs) > +{ > + struct irq_data *d; > + > + d = irq_domain_get_irq_data(domain, virq); > + if (d) { > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + unsigned int i; > + > + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) { > + if (pctrl->hwirq[i] == hwirq) { > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > + bitmap_release_region(pctrl->tint_slot, i, get_order(1)); > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags); > + pctrl->hwirq[i] = 0; > + break; > + } > + } > + } > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > +} > + > +static void rzg2l_gpio_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > + unsigned int hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + void __iomem *addr; > + u32 port; > + u8 bit; > + > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > + > + addr = pctrl->base + ISEL(port); > + if (bit >= 4) { > + bit -= 4; > + addr += 4; > + } > + > + spin_lock_irqsave(&pctrl->lock, flags); > + writel(readl(addr) & ~BIT(bit * 8), addr); > + spin_unlock_irqrestore(&pctrl->lock, flags); > + > + irq_chip_disable_parent(d); > +} > + > +static void rzg2l_gpio_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > + unsigned int hwirq = irqd_to_hwirq(d); > + unsigned long flags; > + void __iomem *addr; > + u32 port; > + u8 bit; > + > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > + > + addr = pctrl->base + ISEL(port); > + if (bit >= 4) { > + bit -= 4; > + addr += 4; > + } > + > + spin_lock_irqsave(&pctrl->lock, flags); > + writel(readl(addr) | BIT(bit * 8), addr); > + spin_unlock_irqrestore(&pctrl->lock, flags); > + > + irq_chip_enable_parent(d); > +} > + > +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + return irq_chip_set_type_parent(d, type); > +} > + > +static void rzg2l_gpio_irqc_eoi(struct irq_data *d) > +{ > + irq_chip_eoi_parent(d); > +} > + > +static struct irq_chip rzg2l_gpio_irqchip = { > + .name = "rzg2l-gpio", > + .irq_disable = rzg2l_gpio_irq_disable, > + .irq_enable = rzg2l_gpio_irq_enable, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_set_type = rzg2l_gpio_irq_set_type, > + .irq_eoi = rzg2l_gpio_irqc_eoi, Please see the changes[1] that are queued in -next around immutable GPIO irqchips. This needs to be made const, the enable/disable methods have the right callbacks added, the resource management methods plumbed, and the correct flag exposed. > +}; > + > +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > + unsigned int child, > + unsigned int child_type, > + unsigned int *parent, > + unsigned int *parent_type) > +{ > + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc); > + unsigned long flags; > + int gpioint, irq; > + > + gpioint = rzg2l_gpio_get_gpioint(child); > + if (gpioint < 0) > + return gpioint; > + > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > + irq = bitmap_find_free_region(pctrl->tint_slot, RZG2L_TINT_MAX_INTERRUPT, get_order(1)); > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags); > + if (irq < 0) > + return -ENOSPC; > + pctrl->hwirq[irq] = child; > + irq += RZG2L_TINT_IRQ_START_INDEX; > + > + /* All these interrupts are level high in the CPU */ > + *parent_type = IRQ_TYPE_LEVEL_HIGH; > + *parent = RZG2L_PACK_HWIRQ(gpioint, irq); > + return 0; > +} > + > +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip, > + unsigned int parent_hwirq, > + unsigned int parent_type) > +{ > + struct irq_fwspec *fwspec; > + > + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); > + if (!fwspec) > + return NULL; > + > + fwspec->fwnode = chip->irq.parent_domain->fwnode; > + fwspec->param_count = 2; > + fwspec->param[0] = parent_hwirq; > + fwspec->param[1] = parent_type; > + > + return fwspec; > +} > + > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > { > struct device_node *np = pctrl->dev->of_node; > struct gpio_chip *chip = &pctrl->gpio_chip; > const char *name = dev_name(pctrl->dev); > + struct irq_domain *parent_domain; > struct of_phandle_args of_args; > + struct device_node *parent_np; > + struct gpio_irq_chip *girq; > int ret; > > + parent_np = of_irq_find_parent(np); > + if (!parent_np) > + return -ENXIO; > + > + parent_domain = irq_find_host(parent_np); > + of_node_put(parent_np); > + if (!parent_domain) > + return -EPROBE_DEFER; > + > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > if (ret) { > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > chip->base = -1; > chip->ngpio = of_args.args[2]; > > + girq = &chip->irq; Same thing, this needs to use the appropriate setter. > + girq->chip = &rzg2l_gpio_irqchip; > + girq->fwnode = of_node_to_fwnode(np); > + girq->parent_domain = parent_domain; > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > + > pctrl->gpio_range.id = 0; > pctrl->gpio_range.pin_base = 0; > pctrl->gpio_range.base = 0; > @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev) > } > > spin_lock_init(&pctrl->lock); > + spin_lock_init(&pctrl->bitmap_lock); > > platform_set_drvdata(pdev, pctrl); > > -- > 2.25.1 > > Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable
On Wed, 11 May 2022 19:32:08 +0100, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Allow free() callback to be overridden from irq_domain_ops for > hierarchical chips. > > This allows drivers to free any resources which are allocated during > populate_parent_alloc_arg(). Do you mean more than the fwspec? I don't see this being used. There is also the question of why we need to have dynamic allocation for the fwspec itself. Why isn't that a simple stack allocation in the context of gpiochip_hierarchy_irq_domain_alloc()? M. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/gpio/gpiolib.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index b7694171655c..d36c4a965efc 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1187,15 +1187,18 @@ static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops) > ops->activate = gpiochip_irq_domain_activate; > ops->deactivate = gpiochip_irq_domain_deactivate; > ops->alloc = gpiochip_hierarchy_irq_domain_alloc; > - ops->free = irq_domain_free_irqs_common; > > /* > - * We only allow overriding the translate() function for > + * We only allow overriding the translate() and free() functions for > * hierarchical chips, and this should only be done if the user > - * really need something other than 1:1 translation. > + * really need something other than 1:1 translation for translate() > + * callback and free if user wants to free up any resources which > + * were allocated during callbacks, for example populate_parent_alloc_arg. > */ > if (!ops->translate) > ops->translate = gpiochip_hierarchy_irq_domain_translate; > + if (!ops->free) > + ops->free = irq_domain_free_irqs_common; > } > > static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) > -- > 2.25.1 > >
Hi Marc, Thank you for the review. On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 11 May 2022 19:32:08 +0100, > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Allow free() callback to be overridden from irq_domain_ops for > > hierarchical chips. > > > > This allows drivers to free any resources which are allocated during > > populate_parent_alloc_arg(). > > Do you mean more than the fwspec? I don't see this being used. > The free callback is used in patch 5/5 where free is overridden by rzg2l_gpio_irq_domain_free. I just gave an example there as an populate_parent_alloc_arg() In actual in the child_to_parent_hwirq callback I am using a bitmap [0] to get a free tint slot, this bitmap needs freeing up when the GPIO interrupt is released from the driver that as when overridden free callback frees the allocated tint slot so that its available for re-use. > There is also the question of why we need to have dynamic allocation > for the fwspec itself. Why isn't that a simple stack allocation in the > context of gpiochip_hierarchy_irq_domain_alloc()? > you mean gpio core itself should handle the fwspec allocation/freeing? [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220511183210.5248-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar > M. > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/gpio/gpiolib.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index b7694171655c..d36c4a965efc 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -1187,15 +1187,18 @@ static void gpiochip_hierarchy_setup_domain_ops(struct irq_domain_ops *ops) > > ops->activate = gpiochip_irq_domain_activate; > > ops->deactivate = gpiochip_irq_domain_deactivate; > > ops->alloc = gpiochip_hierarchy_irq_domain_alloc; > > - ops->free = irq_domain_free_irqs_common; > > > > /* > > - * We only allow overriding the translate() function for > > + * We only allow overriding the translate() and free() functions for > > * hierarchical chips, and this should only be done if the user > > - * really need something other than 1:1 translation. > > + * really need something other than 1:1 translation for translate() > > + * callback and free if user wants to free up any resources which > > + * were allocated during callbacks, for example populate_parent_alloc_arg. > > */ > > if (!ops->translate) > > ops->translate = gpiochip_hierarchy_irq_domain_translate; > > + if (!ops->free) > > + ops->free = irq_domain_free_irqs_common; > > } > > > > static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) > > -- > > 2.25.1 > > > > > > -- > Without deviation from the norm, progress is not possible.
On Thu, 12 May 2022 13:48:53 +0100, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > Hi Marc, > > Thank you for the review. > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 11 May 2022 19:32:08 +0100, > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > hierarchical chips. > > > > > > This allows drivers to free any resources which are allocated during > > > populate_parent_alloc_arg(). > > > > Do you mean more than the fwspec? I don't see this being used. > > > The free callback is used in patch 5/5 where free is overridden by > rzg2l_gpio_irq_domain_free. I just gave an example there as an > populate_parent_alloc_arg() In actual in the child_to_parent_hwirq > callback I am using a bitmap [0] to get a free tint slot, this bitmap > needs freeing up when the GPIO interrupt is released from the driver > that as when overridden free callback frees the allocated tint slot so > that its available for re-use. Right, so that's actually a different life-cycle, and the whole populate_parent_alloc_arg() is a red herring. What you want is to free resources that have been allocated via some other paths. It'd be good if your commit message actually reflected this instead of using an example that doesn't actually exist. > > > There is also the question of why we need to have dynamic allocation > > for the fwspec itself. Why isn't that a simple stack allocation in the > > context of gpiochip_hierarchy_irq_domain_alloc()? > > > you mean gpio core itself should handle the fwspec > allocation/freeing? Yes. The only reason we resort to dynamic allocation is because ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no firmware is involved here). If we had a union of the two types, we could just have a stack variable, and pass that along, completely sidestepping the whole dynamic allocation/freeing business. M.
Hi Marc, On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 12 May 2022 13:48:53 +0100, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > Hi Marc, > > > > Thank you for the review. > > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 11 May 2022 19:32:08 +0100, > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > > hierarchical chips. > > > > > > > > This allows drivers to free any resources which are allocated during > > > > populate_parent_alloc_arg(). > > > > > > Do you mean more than the fwspec? I don't see this being used. > > > > > The free callback is used in patch 5/5 where free is overridden by > > rzg2l_gpio_irq_domain_free. I just gave an example there as an > > populate_parent_alloc_arg() In actual in the child_to_parent_hwirq > > callback I am using a bitmap [0] to get a free tint slot, this bitmap > > needs freeing up when the GPIO interrupt is released from the driver > > that as when overridden free callback frees the allocated tint slot so > > that its available for re-use. > > Right, so that's actually a different life-cycle, and the whole > populate_parent_alloc_arg() is a red herring. What you want is to free > resources that have been allocated via some other paths. It'd be good Is there any other path which I have missed where I can free up resources? > if your commit message actually reflected this instead of using an > example that doesn't actually exist. > My bad, I will update the commit message. > > > > > There is also the question of why we need to have dynamic allocation > > > for the fwspec itself. Why isn't that a simple stack allocation in the > > > context of gpiochip_hierarchy_irq_domain_alloc()? > > > > > you mean gpio core itself should handle the fwspec > > allocation/freeing? > > Yes. The only reason we resort to dynamic allocation is because > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no > firmware is involved here). > I see.. > If we had a union of the two types, we could just have a stack > variable, and pass that along, completely sidestepping the whole > dynamic allocation/freeing business. > Right agreed. Cheers, Prabhakar
On Thu, 12 May 2022 14:50:05 +0100, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > Hi Marc, > > On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 12 May 2022 13:48:53 +0100, > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > Hi Marc, > > > > > > Thank you for the review. > > > > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Wed, 11 May 2022 19:32:08 +0100, > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > > > hierarchical chips. > > > > > > > > > > This allows drivers to free any resources which are allocated during > > > > > populate_parent_alloc_arg(). > > > > > > > > Do you mean more than the fwspec? I don't see this being used. > > > > > > > The free callback is used in patch 5/5 where free is overridden by > > > rzg2l_gpio_irq_domain_free. I just gave an example there as an > > > populate_parent_alloc_arg() In actual in the child_to_parent_hwirq > > > callback I am using a bitmap [0] to get a free tint slot, this bitmap > > > needs freeing up when the GPIO interrupt is released from the driver > > > that as when overridden free callback frees the allocated tint slot so > > > that its available for re-use. > > > > Right, so that's actually a different life-cycle, and the whole > > populate_parent_alloc_arg() is a red herring. What you want is to free > > resources that have been allocated via some other paths. It'd be good > Is there any other path which I have missed where I can free up resources? No, that's the only one. It is just that usually, the alloc() callback is where you are supposed to perform... allocations. It'd be good if you could move your allocation there, as I would expect calls to child_to_parent_hwirq() to be idempotent. > > > if your commit message actually reflected this instead of using an > > example that doesn't actually exist. > > > My bad, I will update the commit message. > > > > > > > > There is also the question of why we need to have dynamic allocation > > > > for the fwspec itself. Why isn't that a simple stack allocation in the > > > > context of gpiochip_hierarchy_irq_domain_alloc()? > > > > > > > you mean gpio core itself should handle the fwspec > > > allocation/freeing? > > > > Yes. The only reason we resort to dynamic allocation is because > > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no > > firmware is involved here). > > > I see.. > > > If we had a union of the two types, we could just have a stack > > variable, and pass that along, completely sidestepping the whole > > dynamic allocation/freeing business. > > > Right agreed. FWIW, I've just posted a PoC patch[1]. Thanks, M. [1] https://lore.kernel.org/r/20220512162320.2213488-1-maz@kernel.org
Hi Geert, Thank you for the review. On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > domain > ouch. > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > used as IRQ lines at given time. Selection of pins as IRQ lines > > at a given time > will fix that. > > is handled by IA55 (which is the IRQC block) which sits in between the > > GPIO and GIC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > { > > struct device_node *np = pctrl->dev->of_node; > > struct gpio_chip *chip = &pctrl->gpio_chip; > > const char *name = dev_name(pctrl->dev); > > + struct irq_domain *parent_domain; > > struct of_phandle_args of_args; > > + struct device_node *parent_np; > > + struct gpio_irq_chip *girq; > > int ret; > > > > + parent_np = of_irq_find_parent(np); > > + if (!parent_np) > > + return -ENXIO; > > + > > + parent_domain = irq_find_host(parent_np); > > + of_node_put(parent_np); > > + if (!parent_domain) > > + return -EPROBE_DEFER; > > + > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > if (ret) { > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > chip->base = -1; > > chip->ngpio = of_args.args[2]; > > > > + girq = &chip->irq; > > + girq->chip = &rzg2l_gpio_irqchip; > > + girq->fwnode = of_node_to_fwnode(np); > > + girq->parent_domain = parent_domain; > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > + > > I think you need to provide a .init_valid_mask() callback, as > gpiochip_irqchip_remove() relies on that for destroying interrupts. Are you suggesting the callback to avoid looping through all the GPIO pins? > However, the mask will need to be dynamic, as GPIO interrupts can be > mapped and unmapped to one of the 32 available interrupts dynamically, > right? Yep that's correct. > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() > is ever called too early, before the mapping is done, it would fail. > The mask initialization is a one time process and that is during adding the GPIO chip. At this stage we won't be knowing what will be the valid GPIO pins used as interrupts. Maybe the core needs to implement a callback which lands in the GPIO controller driver to tell if the gpio irq line is valid. This way we can handle dynamic interrupts. Cheers, Prabhakar > > pctrl->gpio_range.id = 0; > > pctrl->gpio_range.pin_base = 0; > > pctrl->gpio_range.base = 0; > > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Biju, Thank you for the review. On Thu, May 12, 2022 at 6:35 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > Thanks for the patch. > > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to > > handle GPIO interrupt > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be used as > > IRQ lines at given time. Selection of pins as IRQ lines is handled by IA55 > > (which is the IRQC block) which sits in between the GPIO and GIC. > > Do we need to update bindings with interrupt-cells on [1] like [2] as it act as parent for GPIO interrupts? > Yes interrupt-controller and interrupt-parent needs to be added. I'm wondering if "interrupt-cells" is not required. If the pin is an interrupt it will be passed as an GPIO. @Geert - your thoughts ? Cheers, Prabhakar
Hi Marc, Thank you for the review. On Thu, May 12, 2022 at 12:15 PM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 11 May 2022 19:32:10 +0100, > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > used as IRQ lines at given time. Selection of pins as IRQ lines > > is handled by IA55 (which is the IRQC block) which sits in between the > > GPIO and GIC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++++++++++++++++++ > > 1 file changed, 202 insertions(+) > > > > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > index a48cac55152c..af2c739cdbaa 100644 > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > @@ -9,8 +9,10 @@ > > #include <linux/clk.h> > > #include <linux/gpio/driver.h> > > #include <linux/io.h> > > +#include <linux/interrupt.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > +#include <linux/of_irq.h> > > #include <linux/pinctrl/pinconf-generic.h> > > #include <linux/pinctrl/pinconf.h> > > #include <linux/pinctrl/pinctrl.h> > > @@ -89,6 +91,7 @@ > > #define PIN(n) (0x0800 + 0x10 + (n)) > > #define IOLH(n) (0x1000 + (n) * 8) > > #define IEN(n) (0x1800 + (n) * 8) > > +#define ISEL(n) (0x2c80 + (n) * 8) > > #define PWPR (0x3014) > > #define SD_CH(n) (0x3000 + (n) * 4) > > #define QSPI (0x3008) > > @@ -112,6 +115,10 @@ > > #define RZG2L_PIN_ID_TO_PORT_OFFSET(id) (RZG2L_PIN_ID_TO_PORT(id) + 0x10) > > #define RZG2L_PIN_ID_TO_PIN(id) ((id) % RZG2L_PINS_PER_PORT) > > > > +#define RZG2L_TINT_MAX_INTERRUPT 32 > > +#define RZG2L_TINT_IRQ_START_INDEX 9 > > +#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i)) > > + > > struct rzg2l_dedicated_configs { > > const char *name; > > u32 config; > > @@ -137,6 +144,9 @@ struct rzg2l_pinctrl { > > > > struct gpio_chip gpio_chip; > > struct pinctrl_gpio_range gpio_range; > > + DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT); > > + spinlock_t bitmap_lock; > > + unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; > > > > spinlock_t lock; > > }; > > @@ -883,6 +893,8 @@ static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset) > > > > static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset) > > { > > + unsigned int virq; > > + > > pinctrl_gpio_free(chip->base + offset); > > > > /* > > @@ -890,6 +902,10 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset) > > * drive the GPIO pin as an output. > > */ > > rzg2l_gpio_direction_input(chip, offset); > > + > > + virq = irq_find_mapping(chip->irq.domain, offset); > > + if (virq) > > + irq_dispose_mapping(virq); > > } > > > > static const char * const rzg2l_gpio_names[] = { > > @@ -1104,14 +1120,190 @@ static struct { > > } > > }; > > > > +static int rzg2l_gpio_get_gpioint(unsigned int virq) > > +{ > > + unsigned int gpioint; > > + unsigned int i; > > + u32 port, bit; > > + > > + port = virq / 8; > > + bit = virq % 8; > > + > > + if (port >= ARRAY_SIZE(rzg2l_gpio_configs) || > > + bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[port])) > > + return -EINVAL; > > + > > + gpioint = bit; > > + for (i = 0; i < port; i++) > > + gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]); > > + > > + return gpioint; > > +} > > + > > +static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq, > > + unsigned int nr_irqs) > > +{ > > + struct irq_data *d; > > + > > + d = irq_domain_get_irq_data(domain, virq); > > + if (d) { > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > > + unsigned long flags; > > + unsigned int i; > > + > > + for (i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) { > > + if (pctrl->hwirq[i] == hwirq) { > > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > > + bitmap_release_region(pctrl->tint_slot, i, get_order(1)); > > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags); > > + pctrl->hwirq[i] = 0; > > + break; > > + } > > + } > > + } > > + irq_domain_free_irqs_common(domain, virq, nr_irqs); > > +} > > + > > +static void rzg2l_gpio_irq_disable(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > > + unsigned int hwirq = irqd_to_hwirq(d); > > + unsigned long flags; > > + void __iomem *addr; > > + u32 port; > > + u8 bit; > > + > > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > > + > > + addr = pctrl->base + ISEL(port); > > + if (bit >= 4) { > > + bit -= 4; > > + addr += 4; > > + } > > + > > + spin_lock_irqsave(&pctrl->lock, flags); > > + writel(readl(addr) & ~BIT(bit * 8), addr); > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > + > > + irq_chip_disable_parent(d); > > +} > > + > > +static void rzg2l_gpio_irq_enable(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip); > > + unsigned int hwirq = irqd_to_hwirq(d); > > + unsigned long flags; > > + void __iomem *addr; > > + u32 port; > > + u8 bit; > > + > > + port = RZG2L_PIN_ID_TO_PORT(hwirq); > > + bit = RZG2L_PIN_ID_TO_PIN(hwirq); > > + > > + addr = pctrl->base + ISEL(port); > > + if (bit >= 4) { > > + bit -= 4; > > + addr += 4; > > + } > > + > > + spin_lock_irqsave(&pctrl->lock, flags); > > + writel(readl(addr) | BIT(bit * 8), addr); > > + spin_unlock_irqrestore(&pctrl->lock, flags); > > + > > + irq_chip_enable_parent(d); > > +} > > + > > +static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + return irq_chip_set_type_parent(d, type); > > +} > > + > > +static void rzg2l_gpio_irqc_eoi(struct irq_data *d) > > +{ > > + irq_chip_eoi_parent(d); > > +} > > + > > +static struct irq_chip rzg2l_gpio_irqchip = { > > + .name = "rzg2l-gpio", > > + .irq_disable = rzg2l_gpio_irq_disable, > > + .irq_enable = rzg2l_gpio_irq_enable, > > + .irq_mask = irq_chip_mask_parent, > > + .irq_unmask = irq_chip_unmask_parent, > > + .irq_set_type = rzg2l_gpio_irq_set_type, > > + .irq_eoi = rzg2l_gpio_irqc_eoi, > > Please see the changes[1] that are queued in -next around immutable > GPIO irqchips. This needs to be made const, the enable/disable methods > have the right callbacks added, the resource management methods > plumbed, and the correct flag exposed. > Thank you for the pointer, I'll rebase my patches on top of it and implement an immutable GPIO irqchip. Cheers, Prabhakar > > +}; > > + > > +static int rzg2l_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > > + unsigned int child, > > + unsigned int child_type, > > + unsigned int *parent, > > + unsigned int *parent_type) > > +{ > > + struct rzg2l_pinctrl *pctrl = gpiochip_get_data(gc); > > + unsigned long flags; > > + int gpioint, irq; > > + > > + gpioint = rzg2l_gpio_get_gpioint(child); > > + if (gpioint < 0) > > + return gpioint; > > + > > + spin_lock_irqsave(&pctrl->bitmap_lock, flags); > > + irq = bitmap_find_free_region(pctrl->tint_slot, RZG2L_TINT_MAX_INTERRUPT, get_order(1)); > > + spin_unlock_irqrestore(&pctrl->bitmap_lock, flags); > > + if (irq < 0) > > + return -ENOSPC; > > + pctrl->hwirq[irq] = child; > > + irq += RZG2L_TINT_IRQ_START_INDEX; > > + > > + /* All these interrupts are level high in the CPU */ > > + *parent_type = IRQ_TYPE_LEVEL_HIGH; > > + *parent = RZG2L_PACK_HWIRQ(gpioint, irq); > > + return 0; > > +} > > + > > +static void *rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip, > > + unsigned int parent_hwirq, > > + unsigned int parent_type) > > +{ > > + struct irq_fwspec *fwspec; > > + > > + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); > > + if (!fwspec) > > + return NULL; > > + > > + fwspec->fwnode = chip->irq.parent_domain->fwnode; > > + fwspec->param_count = 2; > > + fwspec->param[0] = parent_hwirq; > > + fwspec->param[1] = parent_type; > > + > > + return fwspec; > > +} > > + > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > { > > struct device_node *np = pctrl->dev->of_node; > > struct gpio_chip *chip = &pctrl->gpio_chip; > > const char *name = dev_name(pctrl->dev); > > + struct irq_domain *parent_domain; > > struct of_phandle_args of_args; > > + struct device_node *parent_np; > > + struct gpio_irq_chip *girq; > > int ret; > > > > + parent_np = of_irq_find_parent(np); > > + if (!parent_np) > > + return -ENXIO; > > + > > + parent_domain = irq_find_host(parent_np); > > + of_node_put(parent_np); > > + if (!parent_domain) > > + return -EPROBE_DEFER; > > + > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > if (ret) { > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > chip->base = -1; > > chip->ngpio = of_args.args[2]; > > > > + girq = &chip->irq; > > Same thing, this needs to use the appropriate setter. > > > + girq->chip = &rzg2l_gpio_irqchip; > > + girq->fwnode = of_node_to_fwnode(np); > > + girq->parent_domain = parent_domain; > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > + > > pctrl->gpio_range.id = 0; > > pctrl->gpio_range.pin_base = 0; > > pctrl->gpio_range.base = 0; > > @@ -1253,6 +1454,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev) > > } > > > > spin_lock_init(&pctrl->lock); > > + spin_lock_init(&pctrl->bitmap_lock); > > > > platform_set_drvdata(pdev, pctrl); > > > > -- > > 2.25.1 > > > > > > Thanks, > > M. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gpio-immutable > > -- > Without deviation from the norm, progress is not possible.
Hi Marc, On Thu, May 12, 2022 at 5:26 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 12 May 2022 14:50:05 +0100, > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > Hi Marc, > > > > On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Thu, 12 May 2022 13:48:53 +0100, > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > Hi Marc, > > > > > > > > Thank you for the review. > > > > > > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Wed, 11 May 2022 19:32:08 +0100, > > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > > > > hierarchical chips. > > > > > > > > > > > > This allows drivers to free any resources which are allocated during > > > > > > populate_parent_alloc_arg(). > > > > > > > > > > Do you mean more than the fwspec? I don't see this being used. > > > > > > > > > The free callback is used in patch 5/5 where free is overridden by > > > > rzg2l_gpio_irq_domain_free. I just gave an example there as an > > > > populate_parent_alloc_arg() In actual in the child_to_parent_hwirq > > > > callback I am using a bitmap [0] to get a free tint slot, this bitmap > > > > needs freeing up when the GPIO interrupt is released from the driver > > > > that as when overridden free callback frees the allocated tint slot so > > > > that its available for re-use. > > > > > > Right, so that's actually a different life-cycle, and the whole > > > populate_parent_alloc_arg() is a red herring. What you want is to free > > > resources that have been allocated via some other paths. It'd be good > > Is there any other path which I have missed where I can free up resources? > > No, that's the only one. It is just that usually, the alloc() > callback is where you are supposed to perform... allocations. > OK. > It'd be good if you could move your allocation there, as I would > expect calls to child_to_parent_hwirq() to be idempotent. > For now I'll go with the current implementation, as currently a an array is maintained which is tied with the tint slot and child (which is obtained from child_to_parent_hwirq) > > > > > if your commit message actually reflected this instead of using an > > > example that doesn't actually exist. > > > > > My bad, I will update the commit message. > > > > > > > > > > > There is also the question of why we need to have dynamic allocation > > > > > for the fwspec itself. Why isn't that a simple stack allocation in the > > > > > context of gpiochip_hierarchy_irq_domain_alloc()? > > > > > > > > > you mean gpio core itself should handle the fwspec > > > > allocation/freeing? > > > > > > Yes. The only reason we resort to dynamic allocation is because > > > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no > > > firmware is involved here). > > > > > I see.. > > > > > If we had a union of the two types, we could just have a stack > > > variable, and pass that along, completely sidestepping the whole > > > dynamic allocation/freeing business. > > > > > Right agreed. > > FWIW, I've just posted a PoC patch[1]. > I guess I'll have to rebase my changes on top of it now ;) Cheers, Prabhakar
Hi Geert, Thank you for the review. On Thu, May 12, 2022 at 7:43 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add DT bindings for the Renesas RZ/G2L Interrupt Controller. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for the update! > > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml > > @@ -0,0 +1,134 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/interrupt-controller/renesas,rzg2l-irqc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Renesas RZ/G2L (and alike SoC's) Interrupt Controller (IA55) > > + > > +maintainers: > > + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > + - Geert Uytterhoeven <geert+renesas@glider.be> > > + > > +description: | > > + IA55 performs various interrupt controls including synchronization for the external > > + interrupts of NMI, IRQ, and GPIOINT and the interrupts of the built-in peripheral > > + interrupts output by each IP. And it notifies the interrupt to the GIC > > + - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI interrupts > > + - GPIO pins used as external interrupt input pins, mapped to 32 GIC SPI interrupts > > + - NMI edge select (NMI is not treated as NMI exception and supports fall edge and > > + stand-up edge detection interrupts) > > + > > +allOf: > > + - $ref: /schemas/interrupt-controller.yaml# > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,r9a07g044-irqc # RZ/G2L > > + - const: renesas,rzg2l-irqc > > + > > + '#interrupt-cells': > > + description: The first cell should contain interrupt number and the second cell > > + is used to specify the flag. > > The important part is still missing: which interrupt number (the general > description mentions 3 types)? > I believe the answer is "external interrupt number". > My bad will update to "The first cell should contain an external interrupt number (IRQ0-7) and the second cell is used to specify the flag." Cheers, Prabhakar > > + const: 2 > > + > > + > > Double blank line. > > > + '#address-cells': > > + const: 0 > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Prabhakar, > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain > to handle GPIO interrupt > > Hi Biju, > > Thank you for the review. > > On Thu, May 12, 2022 at 6:35 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > Hi Prabhakar, > > > > Thanks for the patch. > > > > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > domain to handle GPIO interrupt > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > used as IRQ lines at given time. Selection of pins as IRQ lines is > > > handled by IA55 (which is the IRQC block) which sits in between the > GPIO and GIC. > > > > Do we need to update bindings with interrupt-cells on [1] like [2] as it > act as parent for GPIO interrupts? > > > Yes interrupt-controller and interrupt-parent needs to be added. I'm > wondering if "interrupt-cells" is not required. If the pin is an interrupt > it will be passed as an GPIO. It is same as external interrupt case right? For eg:- Ethernet PHY case, interrupt-parent = <&irqc>; interrupts = <3 IRQ_TYPE_LEVEL_LOW>; if you use GPIO, it will be like this right? interrupt-parent = <&pinctrl>; interrupts = <RZG2L_GPIO(43, 0) IRQ_TYPE_LEVEL_LOW>; Cheers, Biju > > @Geert - your thoughts ? > > Cheers, > Prabhakar
On Thu, 12 May 2022 18:55:38 +0100, "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > Hi Marc, > > On Thu, May 12, 2022 at 5:26 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 12 May 2022 14:50:05 +0100, > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > Hi Marc, > > > > > > On Thu, May 12, 2022 at 2:24 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Thu, 12 May 2022 13:48:53 +0100, > > > > "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > Hi Marc, > > > > > > > > > > Thank you for the review. > > > > > > > > > > On Thu, May 12, 2022 at 12:19 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > On Wed, 11 May 2022 19:32:08 +0100, > > > > > > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > > > > > > > Allow free() callback to be overridden from irq_domain_ops for > > > > > > > hierarchical chips. > > > > > > > > > > > > > > This allows drivers to free any resources which are allocated during > > > > > > > populate_parent_alloc_arg(). > > > > > > > > > > > > Do you mean more than the fwspec? I don't see this being used. > > > > > > > > > > > The free callback is used in patch 5/5 where free is overridden by > > > > > rzg2l_gpio_irq_domain_free. I just gave an example there as an > > > > > populate_parent_alloc_arg() In actual in the child_to_parent_hwirq > > > > > callback I am using a bitmap [0] to get a free tint slot, this bitmap > > > > > needs freeing up when the GPIO interrupt is released from the driver > > > > > that as when overridden free callback frees the allocated tint slot so > > > > > that its available for re-use. > > > > > > > > Right, so that's actually a different life-cycle, and the whole > > > > populate_parent_alloc_arg() is a red herring. What you want is to free > > > > resources that have been allocated via some other paths. It'd be good > > > Is there any other path which I have missed where I can free up resources? > > > > No, that's the only one. It is just that usually, the alloc() > > callback is where you are supposed to perform... allocations. > > > OK. > > > It'd be good if you could move your allocation there, as I would > > expect calls to child_to_parent_hwirq() to be idempotent. > > > For now I'll go with the current implementation, as currently a an > array is maintained which is tied with the tint slot and child (which > is obtained from child_to_parent_hwirq) > > > > > > > > if your commit message actually reflected this instead of using an > > > > example that doesn't actually exist. > > > > > > > My bad, I will update the commit message. > > > > > > > > > > > > > > There is also the question of why we need to have dynamic allocation > > > > > > for the fwspec itself. Why isn't that a simple stack allocation in the > > > > > > context of gpiochip_hierarchy_irq_domain_alloc()? > > > > > > > > > > > you mean gpio core itself should handle the fwspec > > > > > allocation/freeing? > > > > > > > > Yes. The only reason we resort to dynamic allocation is because > > > > ThunderX is using MSI-based GPIOs, and thus doesn't use a fwspec (no > > > > firmware is involved here). > > > > > > > I see.. > > > > > > > If we had a union of the two types, we could just have a stack > > > > variable, and pass that along, completely sidestepping the whole > > > > dynamic allocation/freeing business. > > > > > > > Right agreed. > > > > FWIW, I've just posted a PoC patch[1]. > > > I guess I'll have to rebase my changes on top of it now ;) Not yet. Let's see what people say about it. M.
> -----Original Message----- > From: Biju Das > Sent: 12 May 2022 18:59 > To: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; Geert > Uytterhoeven <geert+renesas@glider.be>; Linus Walleij > <linus.walleij@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Marc > Zyngier <maz@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Bartosz Golaszewski > <brgl@bgdev.pl>; Philipp Zabel <p.zabel@pengutronix.de>; linux- > gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; devicetree@vger.kernel.org; Phil Edworthy > <phil.edworthy@renesas.com> > Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain > to handle GPIO interrupt > > Hi Prabhakar, > > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > domain to handle GPIO interrupt > > > > Hi Biju, > > > > Thank you for the review. > > > > On Thu, May 12, 2022 at 6:35 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > > > Hi Prabhakar, > > > > > > Thanks for the patch. > > > > > > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > > domain to handle GPIO interrupt > > > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can > > > > be used as IRQ lines at given time. Selection of pins as IRQ lines > > > > is handled by IA55 (which is the IRQC block) which sits in between > > > > the > > GPIO and GIC. > > > > > > Do we need to update bindings with interrupt-cells on [1] like [2] > > > as it > > act as parent for GPIO interrupts? > > > > > Yes interrupt-controller and interrupt-parent needs to be added. I'm > > wondering if "interrupt-cells" is not required. If the pin is an > > interrupt it will be passed as an GPIO. > > It is same as external interrupt case right? > > For eg:- Ethernet PHY case, > > interrupt-parent = <&irqc>; > interrupts = <3 IRQ_TYPE_LEVEL_LOW>; > > if you use GPIO, it will be like this right? > > interrupt-parent = <&pinctrl>; > interrupts = <RZG2L_GPIO(1, 0) IRQ_TYPE_LEVEL_LOW>; FYI, Previously, I have tested ADV HPD interrupt with below changes while investigating [1] interrupt-parent = <&pinctrl>; interrupts = <RZG2L_GPIO(2, 1) IRQ_TYPE_EDGE_FALLING>; [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220512&id=04b19d32213654e54ec819b6ac033360f1551902 > > Cheers, > Biju > > > > > > > > > > @Geert - your thoughts ? > > > > Cheers, > > Prabhakar
Hi Prabhakar, On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > GPIO and GIC. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > { > > > struct device_node *np = pctrl->dev->of_node; > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > const char *name = dev_name(pctrl->dev); > > > + struct irq_domain *parent_domain; > > > struct of_phandle_args of_args; > > > + struct device_node *parent_np; > > > + struct gpio_irq_chip *girq; > > > int ret; > > > > > > + parent_np = of_irq_find_parent(np); > > > + if (!parent_np) > > > + return -ENXIO; > > > + > > > + parent_domain = irq_find_host(parent_np); > > > + of_node_put(parent_np); > > > + if (!parent_domain) > > > + return -EPROBE_DEFER; > > > + > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > if (ret) { > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > chip->base = -1; > > > chip->ngpio = of_args.args[2]; > > > > > > + girq = &chip->irq; > > > + girq->chip = &rzg2l_gpio_irqchip; > > > + girq->fwnode = of_node_to_fwnode(np); > > > + girq->parent_domain = parent_domain; > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > + > > > > I think you need to provide a .init_valid_mask() callback, as > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > Are you suggesting the callback to avoid looping through all the GPIO pins? gpiochip_irqchip_remove() does: /* Remove all IRQ mappings and delete the domain */ if (gc->irq.domain) { unsigned int irq; for (offset = 0; offset < gc->ngpio; offset++) { if (!gpiochip_irqchip_irq_valid(gc, offset)) continue; irq = irq_find_mapping(gc->irq.domain, offset); irq_dispose_mapping(irq); } irq_domain_remove(gc->irq.domain); } The main thing is not about avoiding to loop through all GPIO pins, but to avoid irq_{find,dispose}_mapping() doing the wrong thing. The loop is over all GPIO offsets, while not all of them are mapped to valid interrupts. Does the above work correctly? > > However, the mask will need to be dynamic, as GPIO interrupts can be > > mapped and unmapped to one of the 32 available interrupts dynamically, > > right? > Yep that's correct. > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() > > is ever called too early, before the mapping is done, it would fail. > > > The mask initialization is a one time process and that is during > adding the GPIO chip. At this stage we won't be knowing what will be > the valid GPIO pins used as interrupts. Maybe the core needs to > implement a callback which lands in the GPIO controller driver to tell > if the gpio irq line is valid. This way we can handle dynamic > interrupts. Upon closer look, I think the mask is a red herring, and we don't need it. But we do need to handle the (possible) mismatch between GPIO offset (index) and IRQ offset in the above code. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, May 13, 2022 at 7:12 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > -----Original Message----- > > From: Biju Das > > Sent: 12 May 2022 18:59 > > To: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; Geert > > Uytterhoeven <geert+renesas@glider.be>; Linus Walleij > > <linus.walleij@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Marc > > Zyngier <maz@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof > > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Bartosz Golaszewski > > <brgl@bgdev.pl>; Philipp Zabel <p.zabel@pengutronix.de>; linux- > > gpio@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas- > > soc@vger.kernel.org; devicetree@vger.kernel.org; Phil Edworthy > > <phil.edworthy@renesas.com> > > Subject: RE: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain > > to handle GPIO interrupt > > > > Hi Prabhakar, > > > > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > domain to handle GPIO interrupt > > > > > > Hi Biju, > > > > > > Thank you for the review. > > > > > > On Thu, May 12, 2022 at 6:35 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > > > > Hi Prabhakar, > > > > > > > > Thanks for the patch. > > > > > > > > > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > Subject: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > > > domain to handle GPIO interrupt > > > > > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can > > > > > be used as IRQ lines at given time. Selection of pins as IRQ lines > > > > > is handled by IA55 (which is the IRQC block) which sits in between > > > > > the > > > GPIO and GIC. > > > > > > > > Do we need to update bindings with interrupt-cells on [1] like [2] > > > > as it > > > act as parent for GPIO interrupts? > > > > > > > Yes interrupt-controller and interrupt-parent needs to be added. I'm > > > wondering if "interrupt-cells" is not required. If the pin is an > > > interrupt it will be passed as an GPIO. > > > > It is same as external interrupt case right? > > > > For eg:- Ethernet PHY case, > > > > interrupt-parent = <&irqc>; > > interrupts = <3 IRQ_TYPE_LEVEL_LOW>; > > > > if you use GPIO, it will be like this right? > > > > interrupt-parent = <&pinctrl>; > > interrupts = <RZG2L_GPIO(1, 0) IRQ_TYPE_LEVEL_LOW>; > > FYI, > > Previously, I have tested ADV HPD interrupt with below changes while investigating [1] > > interrupt-parent = <&pinctrl>; > interrupts = <RZG2L_GPIO(2, 1) IRQ_TYPE_EDGE_FALLING>; > Right, #interrupt-cells=<2> , where the first cell is the GPIO pin and the second cell is the flag. Cheers, Prabhakar
Hi Geert, On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > > GPIO and GIC. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > { > > > > struct device_node *np = pctrl->dev->of_node; > > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > > const char *name = dev_name(pctrl->dev); > > > > + struct irq_domain *parent_domain; > > > > struct of_phandle_args of_args; > > > > + struct device_node *parent_np; > > > > + struct gpio_irq_chip *girq; > > > > int ret; > > > > > > > > + parent_np = of_irq_find_parent(np); > > > > + if (!parent_np) > > > > + return -ENXIO; > > > > + > > > > + parent_domain = irq_find_host(parent_np); > > > > + of_node_put(parent_np); > > > > + if (!parent_domain) > > > > + return -EPROBE_DEFER; > > > > + > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > > if (ret) { > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > chip->base = -1; > > > > chip->ngpio = of_args.args[2]; > > > > > > > > + girq = &chip->irq; > > > > + girq->chip = &rzg2l_gpio_irqchip; > > > > + girq->fwnode = of_node_to_fwnode(np); > > > > + girq->parent_domain = parent_domain; > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > > + > > > > > > I think you need to provide a .init_valid_mask() callback, as > > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > > Are you suggesting the callback to avoid looping through all the GPIO pins? > > gpiochip_irqchip_remove() does: > > /* Remove all IRQ mappings and delete the domain */ > if (gc->irq.domain) { > unsigned int irq; > > for (offset = 0; offset < gc->ngpio; offset++) { > if (!gpiochip_irqchip_irq_valid(gc, offset)) > continue; > > irq = irq_find_mapping(gc->irq.domain, offset); > irq_dispose_mapping(irq); > } > > irq_domain_remove(gc->irq.domain); > > } > > The main thing is not about avoiding to loop through all GPIO pins, > but to avoid irq_{find,dispose}_mapping() doing the wrong thing. So in our case if we don't implement valid masks, that would mean all the pins are valid. irq_find_mapping() would return 0 if no mapping is found to the corresponding offset and irq_dispose_mapping() would simply return back without doing anything if virq == 0.(In this patch rzg2l_gpio_free() does call irq_{find,dispose}_mapping()) > The loop is over all GPIO offsets, while not all of them are mapped > to valid interrupts. Does the above work correctly? > I haven't tested unloading the pinctrl driver which should call gpiochip_irqchip_remove() (we don't have remove call back for pinctrl driver) > > > However, the mask will need to be dynamic, as GPIO interrupts can be > > > mapped and unmapped to one of the 32 available interrupts dynamically, > > > right? > > Yep that's correct. > > > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() > > > is ever called too early, before the mapping is done, it would fail. > > > > > The mask initialization is a one time process and that is during > > adding the GPIO chip. At this stage we won't be knowing what will be > > the valid GPIO pins used as interrupts. Maybe the core needs to > > implement a callback which lands in the GPIO controller driver to tell > > if the gpio irq line is valid. This way we can handle dynamic > > interrupts. > > Upon closer look, I think the mask is a red herring, and we don't > need it. Agreed. > But we do need to handle the (possible) mismatch between GPIO > offset (index) and IRQ offset in the above code. > Agreed, do you see any possibility of the mismatch I have missed? Cheers, Prabhakar
Hi Prabhakar, On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > > > GPIO and GIC. > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Thanks for your patch! > > > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > { > > > > > struct device_node *np = pctrl->dev->of_node; > > > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > > > const char *name = dev_name(pctrl->dev); > > > > > + struct irq_domain *parent_domain; > > > > > struct of_phandle_args of_args; > > > > > + struct device_node *parent_np; > > > > > + struct gpio_irq_chip *girq; > > > > > int ret; > > > > > > > > > > + parent_np = of_irq_find_parent(np); > > > > > + if (!parent_np) > > > > > + return -ENXIO; > > > > > + > > > > > + parent_domain = irq_find_host(parent_np); > > > > > + of_node_put(parent_np); > > > > > + if (!parent_domain) > > > > > + return -EPROBE_DEFER; > > > > > + > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > > > if (ret) { > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > chip->base = -1; > > > > > chip->ngpio = of_args.args[2]; > > > > > > > > > > + girq = &chip->irq; > > > > > + girq->chip = &rzg2l_gpio_irqchip; > > > > > + girq->fwnode = of_node_to_fwnode(np); > > > > > + girq->parent_domain = parent_domain; > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > > > + > > > > > > > > I think you need to provide a .init_valid_mask() callback, as > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > > > Are you suggesting the callback to avoid looping through all the GPIO pins? > > > > gpiochip_irqchip_remove() does: > > > > /* Remove all IRQ mappings and delete the domain */ > > if (gc->irq.domain) { > > unsigned int irq; > > > > for (offset = 0; offset < gc->ngpio; offset++) { > > if (!gpiochip_irqchip_irq_valid(gc, offset)) > > continue; > > > > irq = irq_find_mapping(gc->irq.domain, offset); > > irq_dispose_mapping(irq); > > } > > > > irq_domain_remove(gc->irq.domain); > > > > } > > > > The main thing is not about avoiding to loop through all GPIO pins, > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing. > So in our case if we don't implement valid masks, that would mean all > the pins are valid. irq_find_mapping() would return 0 if no mapping is > found to the corresponding offset and irq_dispose_mapping() would > simply return back without doing anything if virq == 0.(In this patch > rzg2l_gpio_free() does call irq_{find,dispose}_mapping()) But "offset" is a number from the GPIO offset space (0-122), while irq_find_mapping() expects a number from the domain's IRQ space, which is only 0-31? > > The loop is over all GPIO offsets, while not all of them are mapped > > to valid interrupts. Does the above work correctly? > > > I haven't tested unloading the pinctrl driver which should call > gpiochip_irqchip_remove() (we don't have remove call back for pinctrl > driver) > > > > > However, the mask will need to be dynamic, as GPIO interrupts can be > > > > mapped and unmapped to one of the 32 available interrupts dynamically, > > > > right? > > > Yep that's correct. > > > > > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() > > > > is ever called too early, before the mapping is done, it would fail. > > > > > > > The mask initialization is a one time process and that is during > > > adding the GPIO chip. At this stage we won't be knowing what will be > > > the valid GPIO pins used as interrupts. Maybe the core needs to > > > implement a callback which lands in the GPIO controller driver to tell > > > if the gpio irq line is valid. This way we can handle dynamic > > > interrupts. > > > > Upon closer look, I think the mask is a red herring, and we don't > > need it. > Agreed. > > > But we do need to handle the (possible) mismatch between GPIO > > offset (index) and IRQ offset in the above code. > > > Agreed, do you see any possibility of the mismatch I have missed? gpiochip_to_irq(): if (irq_domain_is_hierarchy(domain)) { struct irq_fwspec spec; spec.fwnode = domain->fwnode; spec.param_count = 2; spec.param[0] = gc->irq.child_offset_to_irq(gc, offset); spec.param[1] = IRQ_TYPE_NONE; return irq_create_fwspec_mapping(&spec); } Same here: in the absence of a child_offset_to_irq() callback, the default gpiochip_child_offset_to_irq_noop() will be used, assuming an identity mapping between GPIO numbers and IRQ numbers. So perhaps 1. you need to provide a child_offset_to_irq() callback, 2. gpiochip_irqchip_remove() needs to apply the child_offset_to_irq() mapping too? 3. you do need the mask, or let child_offset_to_irq() an error code, to avoid irq_{find,dispose}_mapping() handling non-existent irqs? Or am I missing something? I guess this is easy to verify by adding some debug prints to the code. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert On Fri, May 13, 2022 at 3:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > > > > GPIO and GIC. > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > { > > > > > > struct device_node *np = pctrl->dev->of_node; > > > > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > > > > const char *name = dev_name(pctrl->dev); > > > > > > + struct irq_domain *parent_domain; > > > > > > struct of_phandle_args of_args; > > > > > > + struct device_node *parent_np; > > > > > > + struct gpio_irq_chip *girq; > > > > > > int ret; > > > > > > > > > > > > + parent_np = of_irq_find_parent(np); > > > > > > + if (!parent_np) > > > > > > + return -ENXIO; > > > > > > + > > > > > > + parent_domain = irq_find_host(parent_np); > > > > > > + of_node_put(parent_np); > > > > > > + if (!parent_domain) > > > > > > + return -EPROBE_DEFER; > > > > > > + > > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > > > > if (ret) { > > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > chip->base = -1; > > > > > > chip->ngpio = of_args.args[2]; > > > > > > > > > > > > + girq = &chip->irq; > > > > > > + girq->chip = &rzg2l_gpio_irqchip; > > > > > > + girq->fwnode = of_node_to_fwnode(np); > > > > > > + girq->parent_domain = parent_domain; > > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > > > > + > > > > > > > > > > I think you need to provide a .init_valid_mask() callback, as > > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > > > > Are you suggesting the callback to avoid looping through all the GPIO pins? > > > > > > gpiochip_irqchip_remove() does: > > > > > > /* Remove all IRQ mappings and delete the domain */ > > > if (gc->irq.domain) { > > > unsigned int irq; > > > > > > for (offset = 0; offset < gc->ngpio; offset++) { > > > if (!gpiochip_irqchip_irq_valid(gc, offset)) > > > continue; > > > > > > irq = irq_find_mapping(gc->irq.domain, offset); > > > irq_dispose_mapping(irq); > > > } > > > > > > irq_domain_remove(gc->irq.domain); > > > > > > } > > > > > > The main thing is not about avoiding to loop through all GPIO pins, > > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing. > > So in our case if we don't implement valid masks, that would mean all > > the pins are valid. irq_find_mapping() would return 0 if no mapping is > > found to the corresponding offset and irq_dispose_mapping() would > > simply return back without doing anything if virq == 0.(In this patch > > rzg2l_gpio_free() does call irq_{find,dispose}_mapping()) > > But "offset" is a number from the GPIO offset space (0-122), while The "offset" reported by kernel is 120-511: root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl, 11030000.pinctrl: gpio-120 (P0_0 ) gpio-121 (P0_1 ) gpio-122 (P0_2 ) gpio-123 (P0_3 ) gpio-124 (P0_4 ) ..... gpio-507 (P48_3 ) gpio-508 (P48_4 ) gpio-509 (P48_5 ) gpio-510 (P48_6 ) gpio-511 (P48_7 ) > irq_find_mapping() expects a number from the domain's IRQ space, > which is only 0-31? > Nope, let me demonstrate with an example, I have configured the gpio pins as GPIO keys in DTS: + keyboard { + compatible = "gpio-keys"; + status = "okay"; + + key-1 { + gpios = <&pinctrl RZG2L_GPIO(43, 0) GPIO_ACTIVE_HIGH>; + linux,code = <KEY_1>; + linux,input-type = <EV_KEY>; + wakeup-source; + label = "SW1"; + }; + + key-2 { + gpios = <&pinctrl RZG2L_GPIO(41, 0) GPIO_ACTIVE_HIGH>; + linux,code = <KEY_2>; + linux,input-type = <EV_KEY>; + wakeup-source; + label = "SW2"; + }; + + key-3 { + gpios = <&pinctrl RZG2L_GPIO(43, 1) GPIO_ACTIVE_HIGH>; + linux,code = <KEY_3>; + linux,input-type = <EV_KEY>; + wakeup-source; + label = "SW3"; + }; + }; root@smarc-rzg2l:~# cat /proc/interrupts | grep SW root@smarc-rzg2l:~# root@smarc-rzg2l:~# insmod gpio_keys.ko [ 925.002720] input: keyboard as /devices/platform/keyboard/input/input3 root@smarc-rzg2l:~# cat /proc/interrupts | grep SW 82: 0 0 11030000.pinctrl 344 Edge SW1 83: 0 0 11030000.pinctrl 328 Edge SW2 84: 0 0 11030000.pinctrl 345 Edge SW3 root@smarc-rzg2l:~# In here 82/83/84 are virq and 344/328/345 are hwirq, which can be confirmed from sysfs file: root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/82 handler: handle_fasteoi_irq device: (null) status: 0x00000001 istate: 0x00000000 ddepth: 0 wdepth: 0 dstate: 0x13400201 IRQ_TYPE_EDGE_RISING IRQD_ACTIVATED IRQD_IRQ_STARTED IRQD_SINGLE_TARGET IRQD_DEFAULT_TRIGGER_SET IRQD_HANDLE_ENFORCE_IRQCTX node: 0 affinity: 0-1 effectiv: domain: :soc:pinctrl@11030000 hwirq: 0x158 chip: 11030000.pinctrl flags: 0x800 IRQCHIP_IMMUTABLE parent: domain: :soc:interrupt-controller@110a0000 hwirq: 0x9 chip: rzg2l-irqc flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE parent: domain: :soc:interrupt-controller@11900000-1 hwirq: 0x1dc chip: GICv3 flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE root@smarc-rzg2l:~# root@smarc-rzg2l:~# root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/83 handler: handle_fasteoi_irq device: (null) status: 0x00000001 istate: 0x00000000 ddepth: 0 wdepth: 0 dstate: 0x13400201 IRQ_TYPE_EDGE_RISING IRQD_ACTIVATED IRQD_IRQ_STARTED IRQD_SINGLE_TARGET IRQD_DEFAULT_TRIGGER_SET IRQD_HANDLE_ENFORCE_IRQCTX node: 0 affinity: 0-1 effectiv: domain: :soc:pinctrl@11030000 hwirq: 0x148 chip: 11030000.pinctrl flags: 0x800 IRQCHIP_IMMUTABLE parent: domain: :soc:interrupt-controller@110a0000 hwirq: 0xa chip: rzg2l-irqc flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE parent: domain: :soc:interrupt-controller@11900000-1 hwirq: 0x1dd chip: GICv3 flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE root@smarc-rzg2l:~# root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/84 handler: handle_fasteoi_irq device: (null) status: 0x00000001 istate: 0x00000000 ddepth: 0 wdepth: 0 dstate: 0x13400201 IRQ_TYPE_EDGE_RISING IRQD_ACTIVATED IRQD_IRQ_STARTED IRQD_SINGLE_TARGET IRQD_DEFAULT_TRIGGER_SET IRQD_HANDLE_ENFORCE_IRQCTX node: 0 affinity: 0-1 effectiv: domain: :soc:pinctrl@11030000 hwirq: 0x159 chip: 11030000.pinctrl flags: 0x800 IRQCHIP_IMMUTABLE parent: domain: :soc:interrupt-controller@110a0000 hwirq: 0xb chip: rzg2l-irqc flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE parent: domain: :soc:interrupt-controller@11900000-1 hwirq: 0x1de chip: GICv3 flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE root@smarc-rzg2l:~# root@smarc-rzg2l:~# root@smarc-rzg2l:~# root@smarc-rzg2l:~# rmmod gpio_keys.ko [ 1143.037314] rzg2l_gpio_free offset:345 virq:84 [ 1143.042488] rzg2l_gpio_free offset:328 virq:83 [ 1143.048700] rzg2l_gpio_free offset:344 virq:82 root@smarc-rzg2l:~# root@smarc-rzg2l:~# I have added print in gpio_free callback where irq_{find,dispose}_mapping()) prints the correct value above. > > > The loop is over all GPIO offsets, while not all of them are mapped > > > to valid interrupts. Does the above work correctly? > > > > > I haven't tested unloading the pinctrl driver which should call > > gpiochip_irqchip_remove() (we don't have remove call back for pinctrl > > driver) > > > > > > > However, the mask will need to be dynamic, as GPIO interrupts can be > > > > > mapped and unmapped to one of the 32 available interrupts dynamically, > > > > > right? > > > > Yep that's correct. > > > > > > > > > I'm not sure if that can be done easily: if gpiochip_irqchip_irq_valid() > > > > > is ever called too early, before the mapping is done, it would fail. > > > > > > > > > The mask initialization is a one time process and that is during > > > > adding the GPIO chip. At this stage we won't be knowing what will be > > > > the valid GPIO pins used as interrupts. Maybe the core needs to > > > > implement a callback which lands in the GPIO controller driver to tell > > > > if the gpio irq line is valid. This way we can handle dynamic > > > > interrupts. > > > > > > Upon closer look, I think the mask is a red herring, and we don't > > > need it. > > Agreed. > > > > > But we do need to handle the (possible) mismatch between GPIO > > > offset (index) and IRQ offset in the above code. > > > > > Agreed, do you see any possibility of the mismatch I have missed? > > gpiochip_to_irq(): > > if (irq_domain_is_hierarchy(domain)) { > struct irq_fwspec spec; > > spec.fwnode = domain->fwnode; > spec.param_count = 2; > spec.param[0] = gc->irq.child_offset_to_irq(gc, offset); > spec.param[1] = IRQ_TYPE_NONE; > > return irq_create_fwspec_mapping(&spec); > } > > Same here: in the absence of a child_offset_to_irq() callback, > the default gpiochip_child_offset_to_irq_noop() will be used, > assuming an identity mapping between GPIO numbers and IRQ > numbers. > Agreed, gpiochip_child_offset_to_irq_noop will return the "offset", but irq_create_fwspec_mapping() in gpiochip_to_irq() will return the virq number which will not be equal to the offset. I added the below change in gpio_keys.c where it calls gpiod_to_irq() -> to_irq() and the below is the log: --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -589,6 +589,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev, button->gpio, error); return error; } + dev_err(dev,"%s gpiod_to_irq() = (irq) %d\n", __func__, irq); + bdata->irq = irq; } root@smarc-rzg2l:~# insmod gpio_keys.ko [ 54.288678] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 82 [ 54.297230] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 83 [ 54.311256] gpio-keys keyboard: gpio_keys_setup_key gpiod_to_irq() = (irq) 84 [ 54.332560] input: keyboard as /devices/platform/keyboard/input/input0 root@smarc-rzg2l:~# > So perhaps > 1. you need to provide a child_offset_to_irq() callback, > 2. gpiochip_irqchip_remove() needs to apply the child_offset_to_irq() > mapping too? > 3. you do need the mask, or let child_offset_to_irq() an error code, > to avoid irq_{find,dispose}_mapping() handling non-existent irqs? >
Hi Prabhakar, Thanks for the example. > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain > to handle GPIO interrupt > > > But "offset" is a number from the GPIO offset space (0-122), while > > The "offset" reported by kernel is 120-511: > > root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio > gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl, > 11030000.pinctrl: > gpio-120 (P0_0 ) > gpio-121 (P0_1 ) > gpio-122 (P0_2 ) > gpio-123 (P0_3 ) > gpio-124 (P0_4 ) > ..... > gpio-507 (P48_3 ) > gpio-508 (P48_4 ) > gpio-509 (P48_5 ) > gpio-510 (P48_6 ) > gpio-511 (P48_7 ) > > > irq_find_mapping() expects a number from the domain's IRQ space, which > > is only 0-31? > > > Nope, let me demonstrate with an example, I have configured the gpio pins > as GPIO keys in DTS: > > + keyboard { > + compatible = "gpio-keys"; > + status = "okay"; > + > + key-1 { > + gpios = <&pinctrl RZG2L_GPIO(43, 0) > GPIO_ACTIVE_HIGH>; > + linux,code = <KEY_1>; > + linux,input-type = <EV_KEY>; > + wakeup-source; > + label = "SW1"; > + }; > + > + key-2 { > + gpios = <&pinctrl RZG2L_GPIO(41, 0) > GPIO_ACTIVE_HIGH>; > + linux,code = <KEY_2>; > + linux,input-type = <EV_KEY>; > + wakeup-source; > + label = "SW2"; > + }; > + > + key-3 { > + gpios = <&pinctrl RZG2L_GPIO(43, 1) > GPIO_ACTIVE_HIGH>; > + linux,code = <KEY_3>; > + linux,input-type = <EV_KEY>; > + wakeup-source; > + label = "SW3"; > + }; > + }; > > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW root@smarc-rzg2l:~# > root@smarc-rzg2l:~# insmod gpio_keys.ko [ 925.002720] input: keyboard as > /devices/platform/keyboard/input/input3 > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW > 82: 0 0 11030000.pinctrl 344 Edge SW1 > 83: 0 0 11030000.pinctrl 328 Edge SW2 > 84: 0 0 11030000.pinctrl 345 Edge SW3 > root@smarc-rzg2l:~# > > In here 82/83/84 are virq and 344/328/345 are hwirq, which can be confirmed > from sysfs file: From your example, Looks like I believe from interrupt statistics point of view, cat /proc/interrupts should report actual gpioint number (0->122) corresponding to pin index for SW1, SW2 and SW3 ?? May be another mapping required for pinindex to gpioint to get proper statistics?? From usage point, another point is, who will track gpioint statistics, pinctrl driver or framework?? Example Use case:- create gpioint0-30 which will fill tint0-tint30. Then insmod gpioint corresponding to SW1 and trigger 1 interrupt and check cat /proc/interrupts for tint31 and SW1 Then rmmode gpioint corresponding to SW1 and insmod SW2 and trigger 5 interrupts and check cat /proc/interrupts for tint31 and SW2 Then rmmode gpioint corresponding to SW2 and insmod SW3 and trigger 7 interrupts and check cat /proc/interrupts for tint31 and SW3 Then rmmode gpioint corresponding to SW3 and insmod SW1 and check cat /proc/interrupts for tint31 and SW1 Then rmmode gpioint corresponding to SW1 and insmod SW2 and check cat /proc/interrupts for tint31 and SW2 Tint31 should report 13 interrupts gpioint corresponding to SW1 should report 1 interrupt gpioint corresponding to SW2 should report 5 interrupts gpioint corresponding to SW3 should report 7 interrupts Cheers, Biju
Hi Prabhakar, On Fri, May 13, 2022 at 8:13 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Fri, May 13, 2022 at 3:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar > > > > <prabhakar.csengg@gmail.com> wrote: > > > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > > > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > > > > > GPIO and GIC. > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > > { > > > > > > > struct device_node *np = pctrl->dev->of_node; > > > > > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > > > > > const char *name = dev_name(pctrl->dev); > > > > > > > + struct irq_domain *parent_domain; > > > > > > > struct of_phandle_args of_args; > > > > > > > + struct device_node *parent_np; > > > > > > > + struct gpio_irq_chip *girq; > > > > > > > int ret; > > > > > > > > > > > > > > + parent_np = of_irq_find_parent(np); > > > > > > > + if (!parent_np) > > > > > > > + return -ENXIO; > > > > > > > + > > > > > > > + parent_domain = irq_find_host(parent_np); > > > > > > > + of_node_put(parent_np); > > > > > > > + if (!parent_domain) > > > > > > > + return -EPROBE_DEFER; > > > > > > > + > > > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > > > > > if (ret) { > > > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > > chip->base = -1; > > > > > > > chip->ngpio = of_args.args[2]; > > > > > > > > > > > > > > + girq = &chip->irq; > > > > > > > + girq->chip = &rzg2l_gpio_irqchip; > > > > > > > + girq->fwnode = of_node_to_fwnode(np); > > > > > > > + girq->parent_domain = parent_domain; > > > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > > > > > + > > > > > > > > > > > > I think you need to provide a .init_valid_mask() callback, as > > > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > > > > > Are you suggesting the callback to avoid looping through all the GPIO pins? > > > > > > > > gpiochip_irqchip_remove() does: > > > > > > > > /* Remove all IRQ mappings and delete the domain */ > > > > if (gc->irq.domain) { > > > > unsigned int irq; > > > > > > > > for (offset = 0; offset < gc->ngpio; offset++) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > if (!gpiochip_irqchip_irq_valid(gc, offset)) > > > > continue; > > > > > > > > irq = irq_find_mapping(gc->irq.domain, offset); > > > > irq_dispose_mapping(irq); > > > > } > > > > > > > > irq_domain_remove(gc->irq.domain); > > > > > > > > } > > > > > > > > The main thing is not about avoiding to loop through all GPIO pins, > > > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing. > > > So in our case if we don't implement valid masks, that would mean all > > > the pins are valid. irq_find_mapping() would return 0 if no mapping is > > > found to the corresponding offset and irq_dispose_mapping() would > > > simply return back without doing anything if virq == 0.(In this patch > > > rzg2l_gpio_free() does call irq_{find,dispose}_mapping()) > > > > But "offset" is a number from the GPIO offset space (0-122), while > > The "offset" reported by kernel is 120-511: Offsets 120-511 are global GPIO numbers, i.e. starting from gpio_chip.base. The loop in gpiochip_irqchip_remove() uses local GPIO numbers, starting from zero. So these offsets are not the same. Likewise, I believe the "offset" passed to irq_find_mapping() is an irq number (hwirq) local to the domain, i.e. also starting at 0. And it must be smaller than the size (32) passed to irq_domain_create_hierarchy(). When passed a non-zero size, irq_domain_create_hierarchy() calls into __irq_domain_add(), with size == hwirq_max == 32: /** * __irq_domain_add() - Allocate a new irq_domain data structure * @fwnode: firmware node for the interrupt controller * @size: Size of linear map; 0 for radix mapping only * @hwirq_max: Maximum number of interrupts supported by controller * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no * direct mapping * @ops: domain callbacks * @host_data: Controller private data pointer * * Allocates and initializes an irq_domain structure. * Returns pointer to IRQ domain, or NULL on failure. */ struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, unsigned int size, irq_hw_number_t hwirq_max, int direct_max, const struct irq_domain_ops *ops, void *host_data) > > > > But we do need to handle the (possible) mismatch between GPIO > > > > offset (index) and IRQ offset in the above code. > > > > > > > Agreed, do you see any possibility of the mismatch I have missed? > > > > gpiochip_to_irq(): > > > > if (irq_domain_is_hierarchy(domain)) { > > struct irq_fwspec spec; > > > > spec.fwnode = domain->fwnode; > > spec.param_count = 2; > > spec.param[0] = gc->irq.child_offset_to_irq(gc, offset); > > spec.param[1] = IRQ_TYPE_NONE; > > > > return irq_create_fwspec_mapping(&spec); > > } > > > > Same here: in the absence of a child_offset_to_irq() callback, > > the default gpiochip_child_offset_to_irq_noop() will be used, > > assuming an identity mapping between GPIO numbers and IRQ > > numbers. > > > Agreed, gpiochip_child_offset_to_irq_noop will return the "offset", > but irq_create_fwspec_mapping() in gpiochip_to_irq() will return the > virq number which will not be equal to the offset. Shouldn't spec.param[0] be in the range 0-31, as 32 is the size of the IRQ domain allocated? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Marc, Thanks for the feedback. > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain > to handle GPIO interrupt > > On Sun, 15 May 2022 06:13:22 +0100, > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > Thanks for the example. > > > > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > domain to handle GPIO interrupt > > > > > > > But "offset" is a number from the GPIO offset space (0-122), while > > > > > > The "offset" reported by kernel is 120-511: > > > > > > root@smarc-rzg2l:~# cat /sys/kernel/debug/gpio > > > gpiochip0: GPIOs 120-511, parent: platform/11030000.pinctrl, > > > 11030000.pinctrl: > > > gpio-120 (P0_0 ) > > > gpio-121 (P0_1 ) > > > gpio-122 (P0_2 ) > > > gpio-123 (P0_3 ) > > > gpio-124 (P0_4 ) > > > ..... > > > gpio-507 (P48_3 ) > > > gpio-508 (P48_4 ) > > > gpio-509 (P48_5 ) > > > gpio-510 (P48_6 ) > > > gpio-511 (P48_7 ) > > > > > > > irq_find_mapping() expects a number from the domain's IRQ space, > > > > which is only 0-31? > > > > > > > Nope, let me demonstrate with an example, I have configured the gpio > > > pins as GPIO keys in DTS: > > > > > > + keyboard { > > > + compatible = "gpio-keys"; > > > + status = "okay"; > > > + > > > + key-1 { > > > + gpios = <&pinctrl RZG2L_GPIO(43, 0) > > > GPIO_ACTIVE_HIGH>; > > > + linux,code = <KEY_1>; > > > + linux,input-type = <EV_KEY>; > > > + wakeup-source; > > > + label = "SW1"; > > > + }; > > > + > > > + key-2 { > > > + gpios = <&pinctrl RZG2L_GPIO(41, 0) > > > GPIO_ACTIVE_HIGH>; > > > + linux,code = <KEY_2>; > > > + linux,input-type = <EV_KEY>; > > > + wakeup-source; > > > + label = "SW2"; > > > + }; > > > + > > > + key-3 { > > > + gpios = <&pinctrl RZG2L_GPIO(43, 1) > > > GPIO_ACTIVE_HIGH>; > > > + linux,code = <KEY_3>; > > > + linux,input-type = <EV_KEY>; > > > + wakeup-source; > > > + label = "SW3"; > > > + }; > > > + }; > > > > > > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW > > > root@smarc-rzg2l:~# root@smarc-rzg2l:~# insmod gpio_keys.ko [ > > > 925.002720] input: keyboard as > > > /devices/platform/keyboard/input/input3 > > > root@smarc-rzg2l:~# cat /proc/interrupts | grep SW > > > 82: 0 0 11030000.pinctrl 344 Edge SW1 > > > 83: 0 0 11030000.pinctrl 328 Edge SW2 > > > 84: 0 0 11030000.pinctrl 345 Edge SW3 > > > root@smarc-rzg2l:~# > > > > > > In here 82/83/84 are virq and 344/328/345 are hwirq, which can be > > > confirmed from sysfs file: > > > > From your example, Looks like > > > > I believe from interrupt statistics point of view, cat > > /proc/interrupts should report actual gpioint number (0->122) > > corresponding to pin index for SW1, SW2 and SW3 ?? > > No. There is no need for such userspace-visible behaviour. Userspace has no > business tracking those. The required information is in debugfs, and that > more than enough. Ok, So far I used cat /proc/interrupts for debugging, since I don't need to enable DEBUG config for Enabling Debugfs for irq. This Debugfs irq is new info to me. Our hardware manual has below info for usb-phy irq 2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the HW manual 113: 0 0 GICv3 126 Level 11c50200.usb-phy Debugfs is also showing similar info like hwirq and interrupt type. But I don't know which field corresponds to number of interrupts? root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113 handler: handle_fasteoi_irq device: (null) status: 0x00000104 istate: 0x00000000 ddepth: 0 wdepth: 0 dstate: 0x13402204 IRQ_TYPE_LEVEL_HIGH IRQD_LEVEL IRQD_ACTIVATED IRQD_IRQ_STARTED IRQD_SINGLE_TARGET IRQD_DEFAULT_TRIGGER_SET IRQD_HANDLE_ENFORCE_IRQCTX node: 0 affinity: 0-1 effectiv: 0 domain: :soc:interrupt-controller@11900000-1 hwirq: 0x7e chip: GICv3 flags: 0x15 IRQCHIP_SET_TYPE_MASKED IRQCHIP_MASK_ON_SUSPEND IRQCHIP_SKIP_SET_WAKE Now coming to current case, Currently GPIO INT 0-122(123 interrupts) corresponding to 120-511(291 interrupts) with same invalid lines.
Hi Marc, .org>; Phil Edworthy > <phil.edworthy@renesas.com> > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > domain to handle GPIO interrupt > > On Mon, 16 May 2022 08:20:47 +0100, > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > > > I believe from interrupt statistics point of view, cat > > > > /proc/interrupts should report actual gpioint number (0->122) > > > > corresponding to pin index for SW1, SW2 and SW3 ?? > > > > > > No. There is no need for such userspace-visible behaviour. Userspace > > > has no business tracking those. The required information is in > > > debugfs, and that more than enough. > > > > Ok, So far I used cat /proc/interrupts for debugging, since I don't > > need to enable DEBUG config for Enabling Debugfs for irq. This Debugfs > > irq is new info to me. > > > > Our hardware manual has below info for usb-phy irq > > 2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level > > > > cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the HW > manual > > 113: 0 0 GICv3 126 Level 11c50200.usb-phy > > > > Debugfs is also showing similar info like hwirq and interrupt type. > > But I don't know which field corresponds to number of interrupts? > > > > root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113 > > handler: handle_fasteoi_irq > > device: (null) > > status: 0x00000104 > > istate: 0x00000000 > > ddepth: 0 > > wdepth: 0 > > dstate: 0x13402204 > > IRQ_TYPE_LEVEL_HIGH > > IRQD_LEVEL > > IRQD_ACTIVATED > > IRQD_IRQ_STARTED > > IRQD_SINGLE_TARGET > > IRQD_DEFAULT_TRIGGER_SET > > IRQD_HANDLE_ENFORCE_IRQCTX > > node: 0 > > affinity: 0-1 > > effectiv: 0 > > domain: :soc:interrupt-controller@11900000-1 > > hwirq: 0x7e > > 0x7e = 126 = 94 - 32 -> SPI94. > > What else do you need? OK, similar to GIC, I thought for gpio interrupts, The hwirq should match with gpiointN mentioned in hwmanual. That is all. Any way it is minor thing, it may be not at all needed. Please ignore this. Eg:-for gpioint0, it should be root@smarc-rzg2l:~# cat /proc/interrupts | grep SW 82: 0 0 11030000.pinctrl 0 Edge XXX Not like root@smarc-rzg2l:~# cat /proc/interrupts | grep SW 82: 0 0 11030000.pinctrl 120 Edge XXX Cheers, Biju > > > chip: GICv3 > > flags: 0x15 > > IRQCHIP_SET_TYPE_MASKED > > IRQCHIP_MASK_ON_SUSPEND > > IRQCHIP_SKIP_SET_WAKE > > > > Now coming to current case, > > > > Currently GPIO INT 0-122(123 interrupts) corresponding to > > 120-511(291 interrupts) with same invalid lines. > > > > From a debugging point, If user has put same irq name for gpioints(cat > > /proc/interrupts case), then how do we distinguish these interrupts?? > > (using hwirq??) > > Yes. > > > > > For using Debugfs, Do we need to first execute cat /proc/interrupts to > > get virq and from there we need to use virq to get statistics, right? > > It depends what you want to do. /sys/kernel/debug/irq/irqs/ has the exact > same information. The only thing /proc/interrupts has that debugfs doesn't > is the per-CPU accounting of delivered interrupts.
Hi Marc, > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > domain to handle GPIO interrupt > > On Mon, 16 May 2022 09:33:03 +0100, > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Marc, > > > > .org>; Phil Edworthy > > > <phil.edworthy@renesas.com> > > > Subject: Re: [PATCH v3 5/5] pinctrl: renesas: pinctrl-rzg2l: Add IRQ > > > domain to handle GPIO interrupt > > > > > > On Mon, 16 May 2022 08:20:47 +0100, > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > > > > > > > > > I believe from interrupt statistics point of view, cat > > > > > > /proc/interrupts should report actual gpioint number (0->122) > > > > > > corresponding to pin index for SW1, SW2 and SW3 ?? > > > > > > > > > > No. There is no need for such userspace-visible behaviour. > > > > > Userspace has no business tracking those. The required > > > > > information is in debugfs, and that more than enough. > > > > > > > > Ok, So far I used cat /proc/interrupts for debugging, since I > > > > don't need to enable DEBUG config for Enabling Debugfs for irq. > > > > This Debugfs irq is new info to me. > > > > > > > > Our hardware manual has below info for usb-phy irq > > > > 2H0_OBINT 126(InterruptID) SPI 94 IRQ 94 Level > > > > > > > > cat /proc/interrupts matches with GICV3 Interrupt ID/ type in the > > > > HW > > > manual > > > > 113: 0 0 GICv3 126 Level 11c50200.usb-phy > > > > > > > > Debugfs is also showing similar info like hwirq and interrupt type. > > > > But I don't know which field corresponds to number of interrupts? > > > > > > > > root@smarc-rzg2l:~# cat /sys/kernel/debug/irq/irqs/113 > > > > handler: handle_fasteoi_irq > > > > device: (null) > > > > status: 0x00000104 > > > > istate: 0x00000000 > > > > ddepth: 0 > > > > wdepth: 0 > > > > dstate: 0x13402204 > > > > IRQ_TYPE_LEVEL_HIGH > > > > IRQD_LEVEL > > > > IRQD_ACTIVATED > > > > IRQD_IRQ_STARTED > > > > IRQD_SINGLE_TARGET > > > > IRQD_DEFAULT_TRIGGER_SET > > > > IRQD_HANDLE_ENFORCE_IRQCTX > > > > node: 0 > > > > affinity: 0-1 > > > > effectiv: 0 > > > > domain: :soc:interrupt-controller@11900000-1 > > > > hwirq: 0x7e > > > > > > 0x7e = 126 = 94 - 32 -> SPI94. > > > > > > What else do you need? > > > > OK, similar to GIC, I thought for gpio interrupts, > > Err. This *IS* the GIC. Yes, tint0-31(IA55 driver) is connected to the GIC which is backend On the frontend(Pincontrol driver) where we have 123 interrupts (gpioint0-122) is mapped to tint0_31. So parent of a particular gpio interrupt can be changed during insmod/rmmod operation. > > > The hwirq should match with gpiointN mentioned in hwmanual. That is > all. > > There is no such need. > > hwirq is whatever the driver decides it is, and userspace can't rely on it > one way or another. If the driver author decides that it is more > convenient to number hwirqs backwards, that's absolutely fine. If you are > debugging, you have access to the driver code, the DT, and all the debugfs > information. You can trace things back and forth as you please. OK agreed. Regards, Biju
Hi Geert, On Mon, May 16, 2022 at 8:14 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Fri, May 13, 2022 at 8:13 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Fri, May 13, 2022 at 3:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, May 13, 2022 at 3:56 PM Lad, Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > On Fri, May 13, 2022 at 7:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Thu, May 12, 2022 at 7:36 PM Lad, Prabhakar > > > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > On Thu, May 12, 2022 at 8:39 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > > > > > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt. > > > > > > > > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be > > > > > > > > used as IRQ lines at given time. Selection of pins as IRQ lines > > > > > > > > is handled by IA55 (which is the IRQC block) which sits in between the > > > > > > > > GPIO and GIC. > > > > > > > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > > > > > > > > > > > > > > > static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > > > { > > > > > > > > struct device_node *np = pctrl->dev->of_node; > > > > > > > > struct gpio_chip *chip = &pctrl->gpio_chip; > > > > > > > > const char *name = dev_name(pctrl->dev); > > > > > > > > + struct irq_domain *parent_domain; > > > > > > > > struct of_phandle_args of_args; > > > > > > > > + struct device_node *parent_np; > > > > > > > > + struct gpio_irq_chip *girq; > > > > > > > > int ret; > > > > > > > > > > > > > > > > + parent_np = of_irq_find_parent(np); > > > > > > > > + if (!parent_np) > > > > > > > > + return -ENXIO; > > > > > > > > + > > > > > > > > + parent_domain = irq_find_host(parent_np); > > > > > > > > + of_node_put(parent_np); > > > > > > > > + if (!parent_domain) > > > > > > > > + return -EPROBE_DEFER; > > > > > > > > + > > > > > > > > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args); > > > > > > > > if (ret) { > > > > > > > > dev_err(pctrl->dev, "Unable to parse gpio-ranges\n"); > > > > > > > > @@ -1138,6 +1330,15 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl) > > > > > > > > chip->base = -1; > > > > > > > > chip->ngpio = of_args.args[2]; > > > > > > > > > > > > > > > > + girq = &chip->irq; > > > > > > > > + girq->chip = &rzg2l_gpio_irqchip; > > > > > > > > + girq->fwnode = of_node_to_fwnode(np); > > > > > > > > + girq->parent_domain = parent_domain; > > > > > > > > + girq->child_to_parent_hwirq = rzg2l_gpio_child_to_parent_hwirq; > > > > > > > > + girq->populate_parent_alloc_arg = rzg2l_gpio_populate_parent_fwspec; > > > > > > > > + girq->child_irq_domain_ops.free = rzg2l_gpio_irq_domain_free; > > > > > > > > + girq->ngirq = RZG2L_TINT_MAX_INTERRUPT; > > > > > > > > + > > > > > > > > > > > > > > I think you need to provide a .init_valid_mask() callback, as > > > > > > > gpiochip_irqchip_remove() relies on that for destroying interrupts. > > > > > > Are you suggesting the callback to avoid looping through all the GPIO pins? > > > > > > > > > > gpiochip_irqchip_remove() does: > > > > > > > > > > /* Remove all IRQ mappings and delete the domain */ > > > > > if (gc->irq.domain) { > > > > > unsigned int irq; > > > > > > > > > > for (offset = 0; offset < gc->ngpio; offset++) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > if (!gpiochip_irqchip_irq_valid(gc, offset)) > > > > > continue; > > > > > > > > > > irq = irq_find_mapping(gc->irq.domain, offset); > > > > > irq_dispose_mapping(irq); > > > > > } > > > > > > > > > > irq_domain_remove(gc->irq.domain); > > > > > > > > > > } > > > > > > > > > > The main thing is not about avoiding to loop through all GPIO pins, > > > > > but to avoid irq_{find,dispose}_mapping() doing the wrong thing. > > > > So in our case if we don't implement valid masks, that would mean all > > > > the pins are valid. irq_find_mapping() would return 0 if no mapping is > > > > found to the corresponding offset and irq_dispose_mapping() would > > > > simply return back without doing anything if virq == 0.(In this patch > > > > rzg2l_gpio_free() does call irq_{find,dispose}_mapping()) > > > > > > But "offset" is a number from the GPIO offset space (0-122), while > > > > The "offset" reported by kernel is 120-511: > > Offsets 120-511 are global GPIO numbers, i.e. starting from > gpio_chip.base. > The loop in gpiochip_irqchip_remove() uses local GPIO numbers, > starting from zero. > So these offsets are not the same. > My bad, offsets will be raging from 0 - 392 > Likewise, I believe the "offset" passed to irq_find_mapping() is an > irq number (hwirq) local to the domain, i.e. also starting at 0. > And it must be smaller than the size (32) passed to > irq_domain_create_hierarchy(). > Since in the current implementation, offset is used as hwirq, the irq_find_mapping() returned the correct virqs. > When passed a non-zero size, irq_domain_create_hierarchy() > calls into __irq_domain_add(), with size == hwirq_max == 32: > > /** > * __irq_domain_add() - Allocate a new irq_domain data structure > * @fwnode: firmware node for the interrupt controller > * @size: Size of linear map; 0 for radix mapping only > * @hwirq_max: Maximum number of interrupts supported by controller > * @direct_max: Maximum value of direct maps; Use ~0 for no limit; 0 for no > * direct mapping > * @ops: domain callbacks > * @host_data: Controller private data pointer > * > * Allocates and initializes an irq_domain structure. > * Returns pointer to IRQ domain, or NULL on failure. > */ > struct irq_domain *__irq_domain_add(struct fwnode_handle *fwnode, > unsigned int size, > irq_hw_number_t hwirq_max, int > direct_max, > const struct irq_domain_ops *ops, > void *host_data) > I have now updated the code to have hwirq's ranging from 0-31 and implemented the child_offset_to_irq() callback. > > > > > But we do need to handle the (possible) mismatch between GPIO > > > > > offset (index) and IRQ offset in the above code. > > > > > > > > > Agreed, do you see any possibility of the mismatch I have missed? > > > > > > gpiochip_to_irq(): > > > > > > if (irq_domain_is_hierarchy(domain)) { > > > struct irq_fwspec spec; > > > > > > spec.fwnode = domain->fwnode; > > > spec.param_count = 2; > > > spec.param[0] = gc->irq.child_offset_to_irq(gc, offset); > > > spec.param[1] = IRQ_TYPE_NONE; > > > > > > return irq_create_fwspec_mapping(&spec); > > > } > > > > > > Same here: in the absence of a child_offset_to_irq() callback, > > > the default gpiochip_child_offset_to_irq_noop() will be used, > > > assuming an identity mapping between GPIO numbers and IRQ > > > numbers. > > > > > Agreed, gpiochip_child_offset_to_irq_noop will return the "offset", > > but irq_create_fwspec_mapping() in gpiochip_to_irq() will return the > > virq number which will not be equal to the offset. > > Shouldn't spec.param[0] be in the range 0-31, as 32 is the size of > the IRQ domain allocated? > Right agreed, but looks like GPIO core is lenient. I have created a patch to do some checking in the GPIO core. Cheers, Prabhakar
Hi All, The RZ/G2L Interrupt Controller is a front-end for the GIC found on Renesas RZ/G2L SoC's with below pins: - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI interrupts - GPIO pins used as external interrupt input pins out of GPIOINT0-122 a maximum of only 32 can be mapped to 32 GIC SPI interrupts, - NMI edge select. _____________ | GIC | | ________ | ____________ | | | | NMI --------------------------------->| | SPI0-479 | | GIC-600| | _______ | |------------>| | | | | | | PPI16-31 | | | | | | IRQ0-IRQ7 | IRQC |------------>| | | P0_P48_4 --->| GPIO |---------------->| | | |________| | | |GPIOINT0-122 | | | | | |---------------->| TINT0-31 | | | |______| |__________| |____________| The proposed patches add hierarchical IRQ domain, one in IRQC driver and another in pinctrl driver. Upon interrupt requests map the interrupt to GIC. Out of GPIOINT0-122 only 32 can be mapped to GIC SPI, this mapping is handled by the pinctrl and IRQC driver. Cheers, Prabhakar Changes for v2->v3: * Updated description for interrupts-cells property in patch #1 * Included RB tag from Geert for binding patch * Fixed review comments pointed by Geert, Biju and Sergei. Changes for v1->v2: * Included RB tag from Rob * Fixed review comments pointed by Geert * included GPIO driver changes Changes for RFCV4 -> V1: * Used unevaluatedProperties. * Altered the sequence of reg property * Set the parent type * Used raw_spin_lock() instead of raw_spin_lock_irqsave() * Simplified parsing IRQ map. * Will send the GPIO and pinctrl changes as part of separate series Changes for RFC v4: * Used locking while RMW * Now using interrupts property instead of interrupt-map * Patch series depends on [0] * Updated binding doc * Fixed comments pointed by Andy [0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/ 20220316200633.28974-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ Changes for RFC v3: -> Re-structured the driver as a hierarchical irq domain instead of chained -> made use of IRQCHIP_* macros -> dropped locking -> Added support for IRQ0-7 interrupts -> Introduced 2 new patches for GPIOLIB -> Switched to using GPIOLIB for irqdomains in pinctrl RFC v2: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ 20210921193028.13099-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ RFC v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/ 20210803175109.1729-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/ Lad Prabhakar (5): dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller irqchip: Add RZ/G2L IA55 Interrupt Controller driver gpio: gpiolib: Allow free() callback to be overridden gpio: gpiolib: Add ngirq member to struct gpio_irq_chip pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt .../renesas,rzg2l-irqc.yaml | 134 ++++++ drivers/gpio/gpiolib.c | 13 +- drivers/irqchip/Kconfig | 8 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-renesas-rzg2l.c | 444 ++++++++++++++++++ drivers/pinctrl/renesas/pinctrl-rzg2l.c | 202 ++++++++ include/linux/gpio/driver.h | 8 + 7 files changed, 805 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c