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 <[email protected]>
> Signed-off-by: Lina Iyer <[email protected]>
> ---
> 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 <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 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 <linux/irqdomain.h>
> +
> +/**
> + * 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...
