From 8d4c998919320206f8832dc413e23fdd27ef2274 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 24 Feb 2022 10:12:22 +0000 Subject: irqchip/qcom-pdc: Kill PDC_NO_PARENT_IRQ PDC_NO_PARENT_IRQ is pretty pointless, as all it indicates is that the PDC terminates the interrupt hierarchy. Which is exactly the same as not having a mapping in the GIC space. This is also bad practice to treat the absence of a hwirq as a hwirq itself. Just explicitly use the region mapping pointer, and drop the definition. Signed-off-by: Marc Zyngier Reviewed-by: Maulik Shah Link: https://lore.kernel.org/r/20220224101226.88373-2-maz@kernel.org --- drivers/irqchip/qcom-pdc.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 173e6520e06e..3b214c4e6755 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -30,14 +30,14 @@ #define IRQ_ENABLE_BANK 0x10 #define IRQ_i_CFG 0x110 -#define PDC_NO_PARENT_IRQ ~0UL - struct pdc_pin_region { u32 pin_base; u32 parent_base; u32 cnt; }; +#define pin_to_hwirq(r, p) ((r)->parent_base + (p) - (r)->pin_base) + static DEFINE_RAW_SPINLOCK(pdc_lock); static void __iomem *pdc_base; static struct pdc_pin_region *pdc_region; @@ -186,19 +186,17 @@ static struct irq_chip qcom_pdc_gic_chip = { .irq_set_affinity = irq_chip_set_affinity_parent, }; -static irq_hw_number_t get_parent_hwirq(int pin) +static struct pdc_pin_region *get_pin_region(int pin) { int i; - struct pdc_pin_region *region; for (i = 0; i < pdc_region_cnt; i++) { - region = &pdc_region[i]; - if (pin >= region->pin_base && - pin < region->pin_base + region->cnt) - return (region->parent_base + pin - region->pin_base); + if (pin >= pdc_region[i].pin_base && + pin < pdc_region[i].pin_base + pdc_region[i].cnt) + return &pdc_region[i]; } - return PDC_NO_PARENT_IRQ; + return NULL; } static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec, @@ -221,7 +219,8 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, { struct irq_fwspec *fwspec = data; struct irq_fwspec parent_fwspec; - irq_hw_number_t hwirq, parent_hwirq; + struct pdc_pin_region *region; + irq_hw_number_t hwirq; unsigned int type; int ret; @@ -234,8 +233,8 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, if (ret) return ret; - parent_hwirq = get_parent_hwirq(hwirq); - if (parent_hwirq == PDC_NO_PARENT_IRQ) + region = get_pin_region(hwirq); + if (!region) return irq_domain_disconnect_hierarchy(domain->parent, virq); if (type & IRQ_TYPE_EDGE_BOTH) @@ -247,7 +246,7 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, parent_fwspec.fwnode = domain->parent->fwnode; parent_fwspec.param_count = 3; parent_fwspec.param[0] = 0; - parent_fwspec.param[1] = parent_hwirq; + parent_fwspec.param[1] = pin_to_hwirq(region, hwirq); parent_fwspec.param[2] = type; return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, @@ -265,7 +264,8 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq, { struct irq_fwspec *fwspec = data; struct irq_fwspec parent_fwspec; - irq_hw_number_t hwirq, parent_hwirq; + struct pdc_pin_region *region; + irq_hw_number_t hwirq; unsigned int type; int ret; @@ -281,8 +281,8 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq, if (ret) return ret; - parent_hwirq = get_parent_hwirq(hwirq); - if (parent_hwirq == PDC_NO_PARENT_IRQ) + region = get_pin_region(hwirq); + if (!region) return irq_domain_disconnect_hierarchy(domain->parent, virq); if (type & IRQ_TYPE_EDGE_BOTH) @@ -294,7 +294,7 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq, parent_fwspec.fwnode = domain->parent->fwnode; parent_fwspec.param_count = 3; parent_fwspec.param[0] = 0; - parent_fwspec.param[1] = parent_hwirq; + parent_fwspec.param[1] = pin_to_hwirq(region, hwirq); parent_fwspec.param[2] = type; return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, -- cgit v1.2.3 From 4dc70713dc24dceeea7f106828674744a6294860 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 24 Feb 2022 10:12:23 +0000 Subject: irqchip/qcom-pdc: Kill non-wakeup irqdomain A careful look at the way the PDC driver works shows that: - all interrupts are in the same space - all interrupts are treated the same And yet the driver creates two domains based on whether the interrupt gets mapped directly or from the pinctrl code, which is obviously a waste of resources. Kill the non-wakeup domain and unify all the interrupt handling. Signed-off-by: Marc Zyngier Link: https://lore.kernel.org/r/20220224101226.88373-3-maz@kernel.org --- drivers/irqchip/qcom-pdc.c | 84 ++++++---------------------------------------- 1 file changed, 10 insertions(+), 74 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 3b214c4e6755..5be531403f50 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -21,7 +21,6 @@ #include #include -#define PDC_MAX_IRQS 168 #define PDC_MAX_GPIO_IRQS 256 #define CLEAR_INTR(reg, intr) (reg & ~(1 << intr)) @@ -224,51 +223,6 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, unsigned int type; int ret; - ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type); - if (ret) - return ret; - - ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, - &qcom_pdc_gic_chip, NULL); - if (ret) - return ret; - - region = get_pin_region(hwirq); - if (!region) - return irq_domain_disconnect_hierarchy(domain->parent, virq); - - if (type & IRQ_TYPE_EDGE_BOTH) - type = IRQ_TYPE_EDGE_RISING; - - if (type & IRQ_TYPE_LEVEL_MASK) - type = IRQ_TYPE_LEVEL_HIGH; - - parent_fwspec.fwnode = domain->parent->fwnode; - parent_fwspec.param_count = 3; - parent_fwspec.param[0] = 0; - parent_fwspec.param[1] = pin_to_hwirq(region, hwirq); - parent_fwspec.param[2] = type; - - return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, - &parent_fwspec); -} - -static const struct irq_domain_ops qcom_pdc_ops = { - .translate = qcom_pdc_translate, - .alloc = qcom_pdc_alloc, - .free = irq_domain_free_irqs_common, -}; - -static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq, - unsigned int nr_irqs, void *data) -{ - struct irq_fwspec *fwspec = data; - struct irq_fwspec parent_fwspec; - struct pdc_pin_region *region; - irq_hw_number_t hwirq; - unsigned int type; - int ret; - ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type); if (ret) return ret; @@ -301,16 +255,9 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq, &parent_fwspec); } -static int qcom_pdc_gpio_domain_select(struct irq_domain *d, - struct irq_fwspec *fwspec, - enum irq_domain_bus_token bus_token) -{ - return bus_token == DOMAIN_BUS_WAKEUP; -} - -static const struct irq_domain_ops qcom_pdc_gpio_ops = { - .select = qcom_pdc_gpio_domain_select, - .alloc = qcom_pdc_gpio_alloc, +static const struct irq_domain_ops qcom_pdc_ops = { + .translate = qcom_pdc_translate, + .alloc = qcom_pdc_alloc, .free = irq_domain_free_irqs_common, }; @@ -361,7 +308,7 @@ static int pdc_setup_pin_mapping(struct device_node *np) static int qcom_pdc_init(struct device_node *node, struct device_node *parent) { - struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain; + struct irq_domain *parent_domain, *pdc_domain; int ret; pdc_base = of_iomap(node, 0); @@ -383,32 +330,21 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) goto fail; } - pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS, - of_fwnode_handle(node), - &qcom_pdc_ops, NULL); - if (!pdc_domain) { - pr_err("%pOF: GIC domain add failed\n", node); - ret = -ENOMEM; - goto fail; - } - - pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain, + pdc_domain = irq_domain_create_hierarchy(parent_domain, IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP, PDC_MAX_GPIO_IRQS, of_fwnode_handle(node), - &qcom_pdc_gpio_ops, NULL); - if (!pdc_gpio_domain) { - pr_err("%pOF: PDC domain add failed for GPIO domain\n", node); + &qcom_pdc_ops, NULL); + if (!pdc_domain) { + pr_err("%pOF: PDC domain add failed\n", node); ret = -ENOMEM; - goto remove; + goto fail; } - irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP); + irq_domain_update_bus_token(pdc_domain, DOMAIN_BUS_WAKEUP); return 0; -remove: - irq_domain_remove(pdc_domain); fail: kfree(pdc_region); iounmap(pdc_base); -- cgit v1.2.3 From d494d088ac44b9cf561362a7856fa20b656be64f Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 24 Feb 2022 10:12:24 +0000 Subject: irqchip/qcom-pdc: Kill qcom_pdc_translate helper qcom_pdc_translate() really is nothing but an open coded version of irq_domain_translate_twocell(). Get rid of it and use the common version instead. Signed-off-by: Marc Zyngier Reviewed-by: Maulik Shah Link: https://lore.kernel.org/r/20220224101226.88373-4-maz@kernel.org --- drivers/irqchip/qcom-pdc.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 5be531403f50..837ca6998f6a 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -198,21 +198,6 @@ static struct pdc_pin_region *get_pin_region(int pin) return NULL; } -static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec, - unsigned long *hwirq, unsigned int *type) -{ - if (is_of_node(fwspec->fwnode)) { - if (fwspec->param_count != 2) - return -EINVAL; - - *hwirq = fwspec->param[0]; - *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; - return 0; - } - - return -EINVAL; -} - static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *data) { @@ -223,7 +208,7 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, unsigned int type; int ret; - ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type); + ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type); if (ret) return ret; @@ -256,7 +241,7 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, } static const struct irq_domain_ops qcom_pdc_ops = { - .translate = qcom_pdc_translate, + .translate = irq_domain_translate_twocell, .alloc = qcom_pdc_alloc, .free = irq_domain_free_irqs_common, }; -- cgit v1.2.3 From a6aca2f460e203781dc41391913cc5b54f4bc0ce Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 24 Feb 2022 10:12:25 +0000 Subject: irqchip/qcom-pdc: Fix broken locking pdc_enable_intr() serves as a primitive to qcom_pdc_gic_{en,dis}able, and has a raw spinlock for mutual exclusion, which is uses with interruptible primitives. This means that this critical section can itself be interrupted. Should the interrupt also be a PDC interrupt, and the endpoint driver perform an irq_disable() on that interrupt, we end-up in a deadlock. Fix this by using the irqsave/irqrestore variants of the locking primitives. Signed-off-by: Marc Zyngier Reviewed-by: Maulik Shah Link: https://lore.kernel.org/r/20220224101226.88373-5-maz@kernel.org --- drivers/irqchip/qcom-pdc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 837ca6998f6a..0cd20ddfae2a 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -55,17 +55,18 @@ static u32 pdc_reg_read(int reg, u32 i) static void pdc_enable_intr(struct irq_data *d, bool on) { int pin_out = d->hwirq; + unsigned long flags; u32 index, mask; u32 enable; index = pin_out / 32; mask = pin_out % 32; - raw_spin_lock(&pdc_lock); + raw_spin_lock_irqsave(&pdc_lock, flags); enable = pdc_reg_read(IRQ_ENABLE_BANK, index); enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask); pdc_reg_write(IRQ_ENABLE_BANK, index, enable); - raw_spin_unlock(&pdc_lock); + raw_spin_unlock_irqrestore(&pdc_lock, flags); } static void qcom_pdc_gic_disable(struct irq_data *d) -- cgit v1.2.3 From d2febf6bbec5466824432e3d8850fc49e4343572 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Thu, 24 Feb 2022 10:12:26 +0000 Subject: irqchip/qcom-pdc: Drop open coded version of __assign_bit() The driver uses what looks like an open-coded version of __assign_bit(). Replace it with the real thing. Signed-off-by: Marc Zyngier Reviewed-by: Maulik Shah Link: https://lore.kernel.org/r/20220224101226.88373-6-maz@kernel.org --- drivers/irqchip/qcom-pdc.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 0cd20ddfae2a..d96916cf6a41 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -23,9 +23,6 @@ #define PDC_MAX_GPIO_IRQS 256 -#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr)) -#define ENABLE_INTR(reg, intr) (reg | (1 << intr)) - #define IRQ_ENABLE_BANK 0x10 #define IRQ_i_CFG 0x110 @@ -55,16 +52,16 @@ static u32 pdc_reg_read(int reg, u32 i) static void pdc_enable_intr(struct irq_data *d, bool on) { int pin_out = d->hwirq; + unsigned long enable; unsigned long flags; u32 index, mask; - u32 enable; index = pin_out / 32; mask = pin_out % 32; raw_spin_lock_irqsave(&pdc_lock, flags); enable = pdc_reg_read(IRQ_ENABLE_BANK, index); - enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask); + __assign_bit(mask, &enable, on); pdc_reg_write(IRQ_ENABLE_BANK, index, enable); raw_spin_unlock_irqrestore(&pdc_lock, flags); } -- cgit v1.2.3