| Message ID | 20220511183210.5248-6-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| 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, 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
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 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
> -----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? > From the above logs, I don't think this is needed. Please correct me if I am wrong. > Or am I missing something? > > I guess this is easy to verify by adding some debug prints to the code. > Let me know if you want me to add debug prints at specific places. Cheers, Prabhakar
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. 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??) 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? Cheers, Biju
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
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);
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(+)