From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 4/9] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs Date: Mon, 11 Mar 2019 10:41:00 +0000 Message-ID: References: <20190222221850.26939-1-ilina@codeaurora.org> <20190222221850.26939-5-ilina@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190222221850.26939-5-ilina@codeaurora.org> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Lina Iyer , swboyd@chromium.org, evgreen@chromium.org Cc: linux-kernel@vger.kernel.org, rplsssn@codeaurora.org, linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com, bjorn.andersson@linaro.org, dianders@chromium.org, linus.walleij@linaro.org List-Id: linux-arm-msm@vger.kernel.org On 22/02/2019 22:18, Lina Iyer wrote: > Introduce a new domain for wakeup capable GPIOs. The domain can be > requested using the bus token DOMAIN_BUS_WAKEUP. In the following > patches, we will specify PDC as the wakeup-parent for the TLMM GPIO > irqchip. Requesting a wakeup GPIO will setup the GPIO and the > corresponding PDC interrupt as its parent. > > Co-developed-by: Stephen Boyd > Signed-off-by: Lina Iyer > --- > Changes in v3: > - Remove PDC GPIO map data (moved to DT) > - hwirq passed in .alloc() is a PDC pin now > Changes in v2: > - Remove separate file for PDC GPIO map data > - Error checks and return > - Whitespace fixes > --- > drivers/irqchip/qcom-pdc.c | 106 ++++++++++++++++++++++++++++++++--- > include/linux/soc/qcom/irq.h | 23 ++++++++ > 2 files changed, 120 insertions(+), 9 deletions(-) > create mode 100644 include/linux/soc/qcom/irq.h > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index faa7d61b9d6c..5a64cbeec410 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -13,12 +13,13 @@ > #include > #include > #include > +#include > #include > -#include > #include > #include > > #define PDC_MAX_IRQS 126 > +#define PDC_MAX_GPIO_IRQS 256 > > #define CLEAR_INTR(reg, intr) (reg & ~(1 << intr)) > #define ENABLE_INTR(reg, intr) (reg | (1 << intr)) > @@ -32,6 +33,16 @@ struct pdc_pin_region { > u32 cnt; > }; > > +struct pdc_gpio_pin_map { > + unsigned int gpio; > + u32 pdc_pin; > +}; > + > +struct pdc_gpio_pin_data { > + size_t size; > + const struct pdc_gpio_pin_map *map; > +}; I can't see this being used anywhere. > + > static DEFINE_RAW_SPINLOCK(pdc_lock); > static void __iomem *pdc_base; > static struct pdc_pin_region *pdc_region; > @@ -47,9 +58,8 @@ static u32 pdc_reg_read(int reg, u32 i) > return readl_relaxed(pdc_base + reg + i * sizeof(u32)); > } > > -static void pdc_enable_intr(struct irq_data *d, bool on) > +static void pdc_enable_intr(irq_hw_number_t pin_out, bool on) > { > - int pin_out = d->hwirq; Why do you need this change? As far as I can tell, you can always use the irq_data here, and it would reduce the size of the patch. > u32 index, mask; > u32 enable; > > @@ -65,13 +75,23 @@ static void pdc_enable_intr(struct irq_data *d, bool on) > > static void qcom_pdc_gic_mask(struct irq_data *d) > { > - pdc_enable_intr(d, false); > + irq_hw_number_t hwirq = d->hwirq; > + > + if (hwirq == ULONG_MAX) Can you define a specific constant here? PDC_WAKEUP_IRQ? Or something? Also, I can't work out where d->hwirq gets set to such value. > + return; > + > + pdc_enable_intr(hwirq, false); > irq_chip_mask_parent(d); > } > > static void qcom_pdc_gic_unmask(struct irq_data *d) > { > - pdc_enable_intr(d, true); > + irq_hw_number_t hwirq = d->hwirq; > + > + if (hwirq == ULONG_MAX) > + return; > + > + pdc_enable_intr(hwirq, true); > irq_chip_unmask_parent(d); > } > > @@ -111,9 +131,12 @@ enum pdc_irq_config_bits { > */ > static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) > { > - int pin_out = d->hwirq; > + irq_hw_number_t pin_out = d->hwirq; > enum pdc_irq_config_bits pdc_type; > > + if (pin_out == ULONG_MAX) > + return 0; > + It is quite odd that you're accepting any odd trigger, even if they are illegal. The API should be consistent, even if you're not applying any change. > switch (type) { > case IRQ_TYPE_EDGE_RISING: > pdc_type = PDC_EDGE_RISING; > @@ -157,7 +180,7 @@ 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 irq_hw_number_t get_parent_hwirq(irq_hw_number_t pin) > { > int i; > struct pdc_pin_region *region; > @@ -169,7 +192,6 @@ static irq_hw_number_t get_parent_hwirq(int pin) > return (region->parent_base + pin - region->pin_base); > } > > - WARN_ON(1); > return ~0UL; Above, you're using ULONG_MAX, and here ~0UL. You need to make up your mind and use one single identifier. Or maybe these are not the same thing? In any case, you need to lift the ambiguity. > } > > @@ -232,6 +254,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 qcom_irq_fwspec *qcom_fwspec = data; > + struct irq_fwspec *fwspec = &qcom_fwspec->fwspec; > + 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 -EINVAL; > + > + parent_hwirq = get_parent_hwirq(hwirq); > + if (parent_hwirq == ~0UL) > + return -EINVAL; > + > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, > + &qcom_pdc_gic_chip, NULL); > + if (ret) > + return ret; > + > + qcom_fwspec->mask = true; > + > + 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; > @@ -270,7 +346,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); > @@ -301,6 +377,18 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > goto fail; > } > > + pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain, 0, > + PDC_MAX_GPIO_IRQS, > + of_fwnode_handle(node), > + &qcom_pdc_gpio_ops, NULL); > + if (!pdc_gpio_domain) { > + pr_err("%pOF: GIC domain add failed for GPIO domain\n", node); > + ret = -ENOMEM; > + goto fail; > + } > + > + irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP); > + > return 0; > > fail: > diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h > new file mode 100644 > index 000000000000..bacc9edbce0d > --- /dev/null > +++ b/include/linux/soc/qcom/irq.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef __QCOM_IRQ_H > +#define __QCOM_IRQ_H > + > +#include > + > +/** > + * struct qcom_irq_fwspec - qcom specific irq fwspec wrapper > + * @fwspec: irq fwspec > + * @mask: if true, keep the irq masked in the gpio controller > + * > + * Use this structure to communicate between the parent irq chip, MPM or PDC, > + * to the gpio chip, TLMM, about the gpio being allocated in the parent > + * and if the gpio chip should keep the line masked because the parent irq > + * chip is handling everything about the irq line. > + */ > +struct qcom_irq_fwspec { > + struct irq_fwspec fwspec; > + bool mask; > +}; > + > +#endif > Thanks, M. -- Jazz is not dead. It just smells funny...