Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Marc Zyngier]

Hi Lina,

On 02/02/18 14:21, Lina Iyer wrote:
> From : Archana Sathyakumar <asathyak@xxxxxxxxxxxxxx>
> 
> The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an
> interrupt controller along with other domain control functions to handle
> interrupt related functions like handle falling edge or active low which
> are not detected at the GIC and handle wakeup interrupts.
> 
> The interrupt controller is on an always-on domain for the purpose of
> waking up the processor. Only a subset of the processor's interrupts are
> routed through the PDC to the GIC. The PDC powers on the processors'
> domain, when in low power mode and replays pending interrupts so the GIC
> may wake up the processor.
> 
> Signed-off-by: Archana Sathyakumar <asathyak@xxxxxxxxxxxxxx>
> Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> ---
>  drivers/irqchip/Kconfig    |   9 ++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/qcom-pdc.c | 326 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/irqchip/qcom-pdc.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c70476b34a53..506c6aa7f0b4 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -343,4 +343,13 @@ config MESON_IRQ_GPIO
>         help
>           Support Meson SoC Family GPIO Interrupt Multiplexer
>  
> +config QCOM_PDC
> +	bool "QCOM PDC"
> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Power Domain Controller driver to manage and configure wakeup
> +	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index d2df34a54d38..280723d83916 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
>  obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
>  obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
> +obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> new file mode 100644
> index 000000000000..a392380eada6
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -0,0 +1,326 @@
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define PDC_MAX_IRQS		126

Is that an absolute, architectural maximum? Or should it come from the
DT (being the sum of all ranges that are provided by this PDC)?

> +
> +#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
> +
> +struct pdc_pin_data {
> +	int pin;
> +	int hwirq;
> +};

I seriously doubt you need this structure, see below.

> +
> +static DEFINE_SPINLOCK(pdc_lock);
> +static void __iomem *pdc_base;

You only have one of those per SoC?

> +
> +static inline void pdc_reg_write(int reg, u32 i, u32 val)
> +{
> +	writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
> +}
> +
> +static inline u32 pdc_reg_read(int reg, u32 i)
> +{
> +	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
> +}
> +
> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
> +{
> +	int pin_out = d->hwirq;
> +	u32 index, mask;
> +	u32 enable;
> +	unsigned long flags;
> +
> +	index = pin_out / 32;
> +	mask = pin_out % 32;
> +
> +	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);
> +	spin_unlock_irqrestore(&pdc_lock, flags);
> +}
> +
> +static void qcom_pdc_gic_mask(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, false);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void qcom_pdc_gic_unmask(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, true);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +/*
> + * GIC does not handle falling edge or active low. To allow falling edge and
> + * active low interrupts to be handled at GIC, PDC has an inverter that inverts
> + * falling edge into a rising edge and active low into an active high.
> + * For the inverter to work, the polarity bit in the IRQ_CONFIG register has to
> + * set as per the table below.
> + * (polarity, falling edge, rising edge ) POLARITY
> + * 3'b0 00  Level sensitive active low    LOW
> + * 3'b0 01  Rising edge sensitive         NOT USED
> + * 3'b0 10  Falling edge sensitive        LOW
> + * 3'b0 11  Dual Edge sensitive           NOT USED
> + * 3'b1 00  Level senstive active High    HIGH
> + * 3'b1 01  Falling Edge sensitive        NOT USED
> + * 3'b1 10  Rising edge sensitive         HIGH
> + * 3'b1 11  Dual Edge sensitive           HIGH
> + */
> +enum pdc_irq_config_bits {
> +	PDC_POLARITY_LOW	= 0, // 0 00
> +	PDC_FALLING_EDGE	= 2, // 0 10
> +	PDC_POLARITY_HIGH	= 4, // 1 00
> +	PDC_RISING_EDGE		= 6, // 1 10
> +	PDC_DUAL_EDGE		= 7, // 1 11
> +};
> +
> +/**
> + * qcom_pdc_gic_set_type: Configure PDC for the interrupt
> + *
> + * @d: the interrupt data
> + * @type: the interrupt type
> + *
> + * If @type is edge triggered, forward that as Rising edge as PDC
> + * takes care of converting falling edge to rising edge signal
> + * If @type is level, then forward that as level high as PDC
> + * takes care of converting falling edge to rising edge signal
> + */
> +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;
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pdc_type = PDC_RISING_EDGE;
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pdc_type = PDC_FALLING_EDGE;
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		pdc_type = PDC_DUAL_EDGE;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pdc_type = PDC_POLARITY_HIGH;
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pdc_type = PDC_POLARITY_LOW;
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +		break;
> +	default:
> +		pdc_type = PDC_POLARITY_HIGH;
> +		break;

If this default clause triggers, something is pretty wrong. You may want
to warn and bail out instead.

> +	}
> +
> +	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
> +
> +	return irq_chip_set_type_parent(d, type);
> +}
> +
> +static struct irq_chip qcom_pdc_gic_chip = {
> +	.name			= "pdc-gic",
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_mask		= qcom_pdc_gic_mask,
> +	.irq_unmask		= qcom_pdc_gic_unmask,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_type		= qcom_pdc_gic_set_type,
> +	.flags			= IRQCHIP_MASK_ON_SUSPEND |
> +				  IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE,
> +	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +#endif
> +};
> +
> +static irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data)
> +{
> +	int i;
> +
> +	for (i = 0; pdc_data[i].pin >= 0; i++)
> +		if (pdc_data[i].pin == pin)
> +			return pdc_data[i].hwirq;
> +
> +	return pin;
> +}

This is the function that irks me. The DT already gives you a nice set
of ranges with all the information you need. And yet, you expand the
whole thing into at most 127 structures, wasting memory and making the
search time a function of the number of interrupts instead of being that
of the number of regions. Not very nice. How about something like this
(untested):

struct pin_region {
	u32 pin_base;
	u32 parent_base;
	u32 cnt;
};

struct pin_region *pin_regions;
int pin_region_count;

static int pdc_pin_to_parent_hwirq(int pin)
{
	int i;

	for (i = 0; i < pin_region_count; i++) {
		if (pin >= pin_regions[i].pin_base &&
		    pin < (pin_regions[i].pin_base + pin_regions[i].cnt)
			return (pin_regions[i].parent_base + pin -
				pin_regions[i].pin_base);
	}

	WARN();
	return -1;
}

Less memory, less comparisons.

> +
> +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 < 3)
> +			return -EINVAL;

Why 3? Reading the DT binding, this is indeed set to 3 without any
reason. I'd suggest this becomes 2, encoding the pin number and the
trigger information, as the leading 0 is quite useless. Yes, I know
other examples in the kernel are using this 0, and that was a
consequence of retrofitting the omitted interrupt controllers (back in
the days of the stupid gic_arch_extn...). Don't do that.

> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;

... and fix this accordingly.

> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int qcom_pdc_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 -EINVAL;
> +
> +	parent_hwirq = get_irq_for_pin(hwirq, domain->host_data);
> +	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +			&qcom_pdc_gic_chip, (void *)parent_hwirq);
> +	if (ret)
> +		return ret;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.param[1] = parent_hwirq;
> +	parent_fwspec.param[2] &= ~IRQ_TYPE_SENSE_MASK;
> +	parent_fwspec.param[2] |= type;
> +	parent_fwspec.fwnode = domain->parent->fwnode;

This becomes:

	parent_fwspec.fwnode = domain->parent->fwnode;
	parent_fwspec.param_count = 3; // That's what the GIC wants
	parent_fwspec.param[0] = 0; //GIC_SPI
	parent_fwspec.param[1] = parent_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 pdc_setup_pin_mapping(struct device_node *np,
> +				struct pdc_pin_data **data)
> +{
> +	int ret;
> +	int n, i, j, k, pins = 0;
> +	int *val;
> +	struct pdc_pin_data *map;
> +
> +	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
> +	if (!n || n % 3)
> +		return -EINVAL;
> +
> +	val = kcalloc(n, sizeof(u32), GFP_KERNEL);
> +	if (!val)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n);
> +	if (ret)
> +		return ret;

Memory leak.

> +
> +	for (i = 0; i < n; i += 3)
> +		pins += val[i + 2];
> +
> +	if (pins > PDC_MAX_IRQS)
> +		return -EFAULT;

EFAULT is for an actual page fault. If you get one here, you have
bigger problems. EINVAL is better. is You're also leaking memory.

> +
> +	map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL);
> +	if (!map) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	for (i = 0, k = 0; i < n; i += 3) {
> +		for (j = 0; j < val[i + 2]; j++, k++) {
> +			map[k].pin = val[i] + j;
> +			map[k].hwirq = val[i + 1] + j;
> +		}
> +	}

That's the thing that needs to go. Just store the ranges as exposed by
the DT, maybe with some checking that there is no overlap in the
ranges (not that it matters much...).

> +
> +	map[k].pin = -1;
> +	*data = map;
> +fail:
> +	kfree(val);
> +	return ret;
> +}
> +
> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *parent_domain, *pdc_domain;
> +	struct pdc_pin_data *pdc_data = NULL;
> +	int ret;
> +
> +	pdc_base = of_iomap(node, 0);
> +	if (!pdc_base) {
> +		pr_err("%s(): unable to map PDC registers\n", node->full_name);
> +		return -ENXIO;
> +	}
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain) {
> +		pr_err("unable to obtain PDC parent domain\n");
> +		ret = -ENXIO;
> +		goto failure;
> +	}
> +
> +	ret = pdc_setup_pin_mapping(node, &pdc_data);
> +	if (ret) {
> +		pr_err("failed to setup PDC pin mapping\n");
> +		goto failure;
> +	}
> +
> +	pdc_domain = irq_domain_create_hierarchy(parent_domain, 0, PDC_MAX_IRQS,
> +					of_fwnode_handle(node), &qcom_pdc_ops,
> +					pdc_data);
> +	if (!pdc_domain) {
> +		pr_err("GIC domain add failed\n");
> +		ret = -ENOMEM;
> +		goto failure;
> +	}
> +
> +	return 0;
> +
> +failure:
> +	kfree(pdc_data);
> +	iounmap(pdc_base);
> +	return ret;
> +}
> +
> +IRQCHIP_DECLARE(pdc_sdm845, "qcom,sdm845-pdc", qcom_pdc_init);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

-- 
Jazz is not dead, it just smell funny.