Message ID | 1568411962-1022-5-git-send-email-ilina@codeaurora.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Support wakeup capable GPIOs | expand |
Hi, On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <ilina@codeaurora.org> wrote: > > diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h > new file mode 100644 > index 0000000..85ac4b6 > --- /dev/null > +++ b/include/linux/soc/qcom/irq.h > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef __QCOM_IRQ_H > +#define __QCOM_IRQ_H > + I happened to be looking at a pile of patches and one of them added: +#include <linux/irqdomain.h> ...right here. If/when you spin your patch, maybe you should too? At the moment the patch I was looking at is at: https://android.googlesource.com/kernel/common/+log/refs/heads/android-mainline-tracking Specifically: https://android.googlesource.com/kernel/common/+/448e2302f82a70f52475b6fc32bbe30301052e6b -Doug
On Fri, Nov 08 2019 at 14:22 -0700, Doug Anderson wrote: >Hi, > >On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h >> new file mode 100644 >> index 0000000..85ac4b6 >> --- /dev/null >> +++ b/include/linux/soc/qcom/irq.h >> @@ -0,0 +1,19 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef __QCOM_IRQ_H >> +#define __QCOM_IRQ_H >> + > >I happened to be looking at a pile of patches and one of them added: > >+#include <linux/irqdomain.h> > >...right here. If/when you spin your patch, maybe you should too? At >the moment the patch I was looking at is at: > >https://android.googlesource.com/kernel/common/+log/refs/heads/android-mainline-tracking > >Specifically: > >https://android.googlesource.com/kernel/common/+/448e2302f82a70f52475b6fc32bbe30301052e6b > > Sure, will take care of it in the next spin. --Lina
On Fri, Nov 08 2019 at 14:54 -0700, Lina Iyer wrote: >On Fri, Nov 08 2019 at 14:22 -0700, Doug Anderson wrote: >>Hi, >> >>On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <ilina@codeaurora.org> wrote: >>> >>>diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h >>>new file mode 100644 >>>index 0000000..85ac4b6 >>>--- /dev/null >>>+++ b/include/linux/soc/qcom/irq.h >>>@@ -0,0 +1,19 @@ >>>+/* SPDX-License-Identifier: GPL-2.0-only */ >>>+ >>>+#ifndef __QCOM_IRQ_H >>>+#define __QCOM_IRQ_H >>>+ >> >>I happened to be looking at a pile of patches and one of them added: >> >>+#include <linux/irqdomain.h> >> >>...right here. If/when you spin your patch, maybe you should too? At >>the moment the patch I was looking at is at: >> >>https://android.googlesource.com/kernel/common/+log/refs/heads/android-mainline-tracking >> >>Specifically: >> >>https://android.googlesource.com/kernel/common/+/448e2302f82a70f52475b6fc32bbe30301052e6b >> >> >Sure, will take care of it in the next spin. > Checking for this, it seems like it would not be needed by this header. There is nothing in this file that would need that header. It was probably a older version that pulled into that tree. Is there a reason now that you see this need? --Lina
Hi, On Fri, Nov 8, 2019 at 2:16 PM Lina Iyer <ilina@codeaurora.org> wrote: > > On Fri, Nov 08 2019 at 14:54 -0700, Lina Iyer wrote: > >On Fri, Nov 08 2019 at 14:22 -0700, Doug Anderson wrote: > >>Hi, > >> > >>On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <ilina@codeaurora.org> wrote: > >>> > >>>diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h > >>>new file mode 100644 > >>>index 0000000..85ac4b6 > >>>--- /dev/null > >>>+++ b/include/linux/soc/qcom/irq.h > >>>@@ -0,0 +1,19 @@ > >>>+/* SPDX-License-Identifier: GPL-2.0-only */ > >>>+ > >>>+#ifndef __QCOM_IRQ_H > >>>+#define __QCOM_IRQ_H > >>>+ > >> > >>I happened to be looking at a pile of patches and one of them added: > >> > >>+#include <linux/irqdomain.h> > >> > >>...right here. If/when you spin your patch, maybe you should too? At > >>the moment the patch I was looking at is at: > >> > >>https://android.googlesource.com/kernel/common/+log/refs/heads/android-mainline-tracking > >> > >>Specifically: > >> > >>https://android.googlesource.com/kernel/common/+/448e2302f82a70f52475b6fc32bbe30301052e6b > >> > >> > >Sure, will take care of it in the next spin. > > > Checking for this, it seems like it would not be needed by this header. > There is nothing in this file that would need that header. It was > probably a older version that pulled into that tree. > > Is there a reason now that you see this need? From the note in the commit I found I'd assume that Maulik Shah (who is CCed here) has history? ...but looking at it, I see that your header file refers to "IRQ_DOMAIN_FLAG_NONCORE" which is defined in "linux/irqdomain.h". That means it's good hygiene for you to include the header, right? Otherwise all your users need to know that they should include the header themselves, which is a bit ugly. -Doug
On Fri, Nov 08 2019 at 15:57 -0700, Doug Anderson wrote: >Hi, > >On Fri, Nov 8, 2019 at 2:16 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> On Fri, Nov 08 2019 at 14:54 -0700, Lina Iyer wrote: >> >On Fri, Nov 08 2019 at 14:22 -0700, Doug Anderson wrote: >> >>Hi, >> >> >> >>On Fri, Sep 13, 2019 at 3:00 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >>> >> >>>diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h >> >>>new file mode 100644 >> >>>index 0000000..85ac4b6 >> >>>--- /dev/null >> >>>+++ b/include/linux/soc/qcom/irq.h >> >>>@@ -0,0 +1,19 @@ >> >>>+/* SPDX-License-Identifier: GPL-2.0-only */ >> >>>+ >> >>>+#ifndef __QCOM_IRQ_H >> >>>+#define __QCOM_IRQ_H >> >>>+ >> >> >> >>I happened to be looking at a pile of patches and one of them added: >> >> >> >>+#include <linux/irqdomain.h> >> >> >> >>...right here. If/when you spin your patch, maybe you should too? At >> >>the moment the patch I was looking at is at: >> >> >> >>https://android.googlesource.com/kernel/common/+log/refs/heads/android-mainline-tracking >> >> >> >>Specifically: >> >> >> >>https://android.googlesource.com/kernel/common/+/448e2302f82a70f52475b6fc32bbe30301052e6b >> >> >> >> >> >Sure, will take care of it in the next spin. >> > >> Checking for this, it seems like it would not be needed by this header. >> There is nothing in this file that would need that header. It was >> probably a older version that pulled into that tree. >> >> Is there a reason now that you see this need? > >From the note in the commit I found I'd assume that Maulik Shah (who >is CCed here) has history? > >...but looking at it, I see that your header file refers to >"IRQ_DOMAIN_FLAG_NONCORE" which is defined in "linux/irqdomain.h". Ah, ok. That would need the file. Will add. >That means it's good hygiene for you to include the header, right? >Otherwise all your users need to know that they should include the >header themselves, which is a bit ugly. > >-Doug
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 5eef5ea..4abd775 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -13,12 +13,13 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/soc/qcom/irq.h> #include <linux/spinlock.h> -#include <linux/platform_device.h> #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)) #define ENABLE_INTR(reg, intr) (reg | (1 << intr)) @@ -26,6 +27,8 @@ #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; @@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on) static void qcom_pdc_gic_disable(struct irq_data *d) { + if (d->hwirq == GPIO_NO_WAKE_IRQ) + return; + pdc_enable_intr(d, false); irq_chip_disable_parent(d); } static void qcom_pdc_gic_enable(struct irq_data *d) { + if (d->hwirq == GPIO_NO_WAKE_IRQ) + return; + pdc_enable_intr(d, true); irq_chip_enable_parent(d); } static void qcom_pdc_gic_mask(struct irq_data *d) { + if (d->hwirq == GPIO_NO_WAKE_IRQ) + return; + irq_chip_mask_parent(d); } static void qcom_pdc_gic_unmask(struct irq_data *d) { + if (d->hwirq == GPIO_NO_WAKE_IRQ) + return; + irq_chip_unmask_parent(d); } @@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) int pin_out = d->hwirq; enum pdc_irq_config_bits pdc_type; + if (pin_out == GPIO_NO_WAKE_IRQ) + return 0; + switch (type) { case IRQ_TYPE_EDGE_RISING: pdc_type = PDC_EDGE_RISING; @@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin) return (region->parent_base + pin - region->pin_base); } - WARN_ON(1); - return ~0UL; + return PDC_NO_PARENT_IRQ; } static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec, @@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq, ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type); if (ret) - return -EINVAL; - - parent_hwirq = get_parent_hwirq(hwirq); - if (parent_hwirq == ~0UL) - return -EINVAL; + return ret; ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &qcom_pdc_gic_chip, NULL); if (ret) return ret; + parent_hwirq = get_parent_hwirq(hwirq); + if (parent_hwirq == PDC_NO_PARENT_IRQ) + return 0; + if (type & IRQ_TYPE_EDGE_BOTH) type = IRQ_TYPE_EDGE_RISING; @@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = { .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; + irq_hw_number_t hwirq, parent_hwirq; + 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; + + if (hwirq == GPIO_NO_WAKE_IRQ) + return 0; + + parent_hwirq = get_parent_hwirq(hwirq); + if (parent_hwirq == PDC_NO_PARENT_IRQ) + return 0; + + 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] = parent_hwirq; + parent_fwspec.param[2] = type; + + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, + &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, + .free = irq_domain_free_irqs_common, +}; + static int pdc_setup_pin_mapping(struct device_node *np) { int ret, n; @@ -282,7 +353,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; + struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain; int ret; pdc_base = of_iomap(node, 0); @@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) goto fail; } + pdc_gpio_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); + ret = -ENOMEM; + goto remove; + } + + irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP); + return 0; +remove: + irq_domain_remove(pdc_domain); fail: kfree(pdc_region); iounmap(pdc_base); diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h new file mode 100644 index 0000000..85ac4b6 --- /dev/null +++ b/include/linux/soc/qcom/irq.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __QCOM_IRQ_H +#define __QCOM_IRQ_H + +#define GPIO_NO_WAKE_IRQ ~0U + +/** + * QCOM specific IRQ domain flags that distinguishes the handling of wakeup + * capable interrupts by different interrupt controllers. + * + * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the + * interrupt configuration is done at PDC + * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM + */ +#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 0) +#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP (IRQ_DOMAIN_FLAG_NONCORE << 1) + +#endif