| Message ID | 20220224101226.88373-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | irqchip/qcom-pdc: Assorted cleanups and fixes | expand |
Hi, On 2/24/2022 3:42 PM, Marc Zyngier wrote: > 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. The GPIO is kept under separate domain to handle extra configuration for wake GPIO handling. On targets like SM8150/SM8250 each wake up capable GPIO (if totan n) line has dedicated parent PDC irq (if total m, n = m) associated with it. However on targets like sdx55 PDC has muxes where each wake GPIO (if total n) line goes through each PDC muxes (if total m, n > m) and any of these muxes can be used to route any one GPIO to PDC (and parent GIC) but unlike other targets it doesn't have one to one mapping for GPIO to GIC interrupt. So this will need to be kept as is to support sdx55 target. Thanks, Maulik > > Kill the non-wakeup domain and unify all the interrupt handling. > > Signed-off-by: Marc Zyngier <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 <linux/slab.h> > #include <linux/types.h> > > -#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);
On Mon, 28 Feb 2022 19:29:41 +0000, "Maulik Shah (mkshah)" <quic_mkshah@quicinc.com> wrote: > > Hi, > > On 2/24/2022 3:42 PM, Marc Zyngier wrote: > > 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. > The GPIO is kept under separate domain to handle extra configuration > for wake GPIO handling. Which extra configuration? The irq_chip structure is the same, the translation is the same, the GIC mapping is the same, and the select method only serves to select between two irq domains that do the same thing. So please point me to what the difference is. > > On targets like SM8150/SM8250 each wake up capable GPIO (if totan n) > line has dedicated parent PDC irq (if total m, n = m) associated with > it. > However on targets like sdx55 PDC has muxes where each wake GPIO (if > total n) line goes through each PDC muxes (if total m, n > m) and > any of these muxes can be used to route any one GPIO to PDC (and > parent GIC) but unlike other targets it doesn't have one to one > mapping for GPIO to GIC interrupt. > So this will need to be kept as is to support sdx55 target. As far as I can tell, the current code doesn't have any support for this. And if there is a mux involved in the interrupt routing, this should be something entierly separate, as the current code is strictly hierarchical. What am I missing? M.
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 <linux/slab.h> #include <linux/types.h> -#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);
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 <maz@kernel.org> --- drivers/irqchip/qcom-pdc.c | 84 +++++--------------------------------- 1 file changed, 10 insertions(+), 74 deletions(-)