Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH RFC 1/4] drivers: irqchip: pdc: add support for PDC
 interrupt controller
To:     Lina Iyer <ilina@codeaurora.org>, tglx@linutronix.de,
        jason@lakedaemon.net
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        sboyd@codeaurora.org, rnayak@codeaurora.org,
        asathyak@codeaurora.org
References: <20180123175656.11942-1-ilina@codeaurora.org>
 <20180123175656.11942-2-ilina@codeaurora.org>
From:   Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <9dd8307c-4906-4707-d4d7-2ef0fc8307b6@arm.com>
Date:   Wed, 24 Jan 2018 14:20:25 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20180123175656.11942-2-ilina@codeaurora.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Lina, Archana,

On 23/01/18 17:56, Lina Iyer wrote:
> From : Archana Sathyakumar <asathyak@codeaurora.org>
> 
> The Power Domain Controller (PDC) hardware block on Qualcomm SoCs 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, but only a subset of the processor's interrupts
> are routed through the PDC to the GIC. The PDC powers on the processor's
> domain, bringing the domain out of low power mode and replays the
> pending interrupts so the GIC may wake up the processor.
> 
> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [Lina: Split out DT bindings target data and initialization changes]
> ---
>  drivers/irqchip/Kconfig    |   9 ++
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/qcom-pdc.c | 282 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/qcom-pdc.h |  30 +++++
>  4 files changed, 322 insertions(+)
>  create mode 100644 drivers/irqchip/qcom-pdc.c
>  create mode 100644 drivers/irqchip/qcom-pdc.h
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index c70476b34a53..9d54151157d5 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.'s (QTI) mobile SoCs.
> +
>  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..57f1348bd81c
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -0,0 +1,282 @@
> +/* 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>
> +
> +#include "qcom-pdc.h"
> +
> +#define PDC_MAX_IRQS			126
> +
> +#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
> +
> +static DEFINE_SPINLOCK(pdc_lock);
> +static void __iomem *pdc_base;
> +
> +static int get_pdc_pin(irq_hw_number_t hwirq, void *data)
> +{
> +	int i;
> +	struct pdc_pin *pdc_data = (struct pdc_pin *) data;
> +
> +	for (i = 0; pdc_data[i].pin >= 0; i++) {
> +		if (pdc_data[i].hwirq == hwirq)
> +			return pdc_data[i].pin;
> +	}
> +
> +	return -EINVAL;
> +}

That's a total NAK. See my reply to patch #4.

> +
> +static inline void pdc_enable_intr(struct irq_data *d, bool on)
> +{
> +	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
> +	unsigned int index, mask;
> +	u32 enable, r_enable;
> +	unsigned long flags;
> +
> +	if (pin_out < 0)
> +		return;
> +
> +	index = pin_out / 32;
> +	mask = pin_out % 32;
> +	spin_lock_irqsave(&pdc_lock, flags);
> +	enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK + (index *
> +					sizeof(uint32_t)));
> +	enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
> +	writel_relaxed(enable, pdc_base + IRQ_ENABLE_BANK + (index *
> +						sizeof(uint32_t)));
> +	do {
> +		r_enable = readl_relaxed(pdc_base + IRQ_ENABLE_BANK +
> +					(index * sizeof(uint32_t)));

Consider either using an intermediate variable or an accessor for this
"pdc_base + IRQ... + ...".

> +		if (r_enable == enable)
> +			break;
> +		udelay(5);
> +	} while (1);
> +	spin_unlock_irqrestore(&pdc_lock, flags);

Ouch. this feels very heavy handed. How about placing the polling side
outside of the spinlock section, only testing the affected bit instead?
And maybe a timeout just in case the HW is getting wedged.

> +	WARN_ON(r_enable != enable);
> +}
> +
> +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);
> +}
> +
> +static void qcom_pdc_gic_enable(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, true);
> +	irq_chip_enable_parent(d);
> +}
> +
> +static void qcom_pdc_gic_disable(struct irq_data *d)
> +{
> +	pdc_enable_intr(d, false);
> +	irq_chip_disable_parent(d);
> +}

Why do you have to provide both {en,dis}able and {un,}mask? The GIC only
has {un,}mask, and given that this is pretty tied to it, you should
probably stick with what it implements.

> +
> +/*
> + * 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 )  ORIG          POL CONV     POLARITY
> + * 3'b0 00  Level sensitive active low    (~~~|_____)   (___|~~~~~)   LOW
> + * 3'b0 10  Falling edge sensitive        (~~~|__|~~)   (___|~~|__)   LOW
> + * 3'b1 00  Level senstive active High    (___|~~~~~)   (___|~~~~~)   HIGH
> + * 3'b1 10  Rising edge sensitive         (___|~~|__)   (___|~~|__)   HIGH

I'm not sure about the ASCII art. I'd rather see a table that outlines
the use of each bit individually (and potentially document bit 0 which
seems to be unused).

> + */
> +enum pdc_irq_config_bits {
> +	POLARITY_LOW	= 0, // b000
> +	FALLING_EDGE	= 2, // b010
> +	POLARITY_HIGH	= 4, // b100
> +	RISING_EDGE	= 6, // b110

Consider prefixing those with PDC_

> +};
> +
> +static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> +{
> +	int pin_out = get_pdc_pin(d->hwirq, d->chip_data);
> +	u32 pdc_type = 0, config;
> +
> +	if (pin_out < 0)
> +		goto fwd_to_parent;

How can that even happen?

> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pdc_type = RISING_EDGE;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pdc_type = FALLING_EDGE;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pdc_type = POLARITY_HIGH;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pdc_type = POLARITY_LOW;
> +		break;
> +	default:
> +		pdc_type = POLARITY_HIGH;
> +		break;

If I say "IRQ_TYPE_EDGE_BOTH", I get level high? Probably not what you want.

> +	}
> +	writel_relaxed(pdc_type, pdc_base + IRQ_i_CFG +
> +			(pin_out * sizeof(uint32_t)));
> +
> +	do {
> +		config = readl_relaxed(pdc_base + IRQ_i_CFG +
> +				(pin_out * sizeof(uint32_t)));
> +		if (config == pdc_type)
> +			break;
> +		udelay(5);
> +	} while (1);

Same remark about the timeout.

> +
> +	/*
> +	 * If type is edge triggered, forward that as Rising edge as PDC
> +	 * takes care of converting falling edge to rising edge signal
> +	 */
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		type = IRQ_TYPE_EDGE_RISING;
> +
> +	/*
> +	 * If type is level, then forward that as level high as PDC
> +	 * takes care of converting falling edge to rising edge signal
> +	 */
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		type = IRQ_TYPE_LEVEL_HIGH;

You can fold that into the switch statement above.

> +
> +fwd_to_parent:
> +	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_enable		= qcom_pdc_gic_enable,
> +	.irq_unmask		= qcom_pdc_gic_unmask,
> +	.irq_disable		= qcom_pdc_gic_disable,
> +	.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 int qcom_pdc_translate(struct irq_domain *d,
> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
> +{
> +	return d->parent->ops->translate(d->parent, fwspec, hwirq, type);

No way. The translate operation is local to your domain. You don't go
and fish in another domain's private stuff. Please implement your own.
The reason you're getting away with it is because you're in the DT by
providing the GIC SPI instead of the pin into the PDC.

Don't do that. Expose the pin in the DT, use the alloc method to map the
PDC pin into the GIC pin.

> +}
> +
> +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;
> +	int i;
> +	unsigned int type;
> +	int ret;
> +
> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +				&qcom_pdc_gic_chip, domain->host_data);

You don't need this loop. It is only when you deal with Multi-MSI that
you can end-up allocating more than a single interrupt per allocation.
In the case of wired interrupts, you are guaranteed that nr_irqs == 1.

> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +
> +	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 const struct of_device_id pdc_table[] = {
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pdc_table);
> +
> +int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *parent_domain,  *pdc_domain;
> +	const struct pdc_pin *data;
> +	const struct of_device_id *id;
> +	int ret;
> +
> +	id = of_match_node(pdc_table, node);
> +	if (id)
> +		data = id->data;
> +	else
> +		return -ENODEV;
> +
> +	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;
> +	}
> +
> +	pdc_domain = irq_domain_add_hierarchy(parent_domain, 0, PDC_MAX_IRQS,> +					node, &qcom_pdc_ops, (void *)data);

Consider using irq_domain_create_hierarchy instead, as it uses fwnode,
and thus consistent with the use of irq_fwspec elsewhere in the code.

> +	if (!pdc_domain) {
> +		pr_err("GIC domain add failed\n");
> +		ret = -ENOMEM;
> +		goto failure;
> +	}
> +
> +	pdc_domain->name = "qcom,pdc";

Why do you need this? The irqdomain layer should already assign a name
already.

> +	return 0;
> +
> +failure:
> +	iounmap(pdc_base);
> +	return ret;
> +}
> diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
> new file mode 100644
> index 000000000000..b5b64390175e
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc.h
> @@ -0,0 +1,30 @@
> +/* 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.
> + *
> + */
> +
> +#ifndef __QCOM_PDC_H__
> +#define __QCOM_PDC_H__
> +
> +#include <linux/irq.h>
> +
> +/**
> + * pdc_pin: Mapping of PDC port to hwirq
> + *
> + * @pin: PDC port for the IRQ
> + * @hwirq: hw IRQ number
> + */
> +struct pdc_pin {
> +	int pin;
> +	irq_hw_number_t hwirq;
> +};
> +
> +#endif /* __QCOM_PDC_H__ */
> 

What's the point of this include file?

There is one thing that worries me in this driver. You say that the PDC
"replays the pending interrupts so the GIC may wake up the processor".
How is that done without any PM hook allowing for a switch from GIC to
PDC? How do you ensure that you transition from one to the other without
loosing interrupts (edge interrupts, in particular)? Or can you get
spurious interrupts instead?

Thanks,

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