diff mbox

irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver

Message ID 1501758910-3488-1-git-send-email-yamada.masahiro@socionext.com (mailing list archive)
State New, archived
Headers show

Commit Message

Masahiro Yamada Aug. 3, 2017, 11:15 a.m. UTC 
UniPhier SoCs contain AIDET (ARM Interrupt Detector).  This is intended
to provide additional features that are not covered by GIC.  The main
purpose is to provide logic inverter to support low level and falling
edge trigger type for interrupt lines from on-board devices.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 .../socionext,uniphier-aidet.txt                   |  32 +++
 MAINTAINERS                                        |   1 +
 drivers/irqchip/Kconfig                            |   5 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-uniphier-aidet.c               | 252 +++++++++++++++++++++
 5 files changed, 291 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
 create mode 100644 drivers/irqchip/irq-uniphier-aidet.c

Comments

Marc Zyngier Aug. 7, 2017, 10:43 a.m. UTC | #1 
On 03/08/17 12:15, Masahiro Yamada wrote:
> UniPhier SoCs contain AIDET (ARM Interrupt Detector).  This is intended
> to provide additional features that are not covered by GIC.  The main
> purpose is to provide logic inverter to support low level and falling
> edge trigger type for interrupt lines from on-board devices.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  .../socionext,uniphier-aidet.txt                   |  32 +++
>  MAINTAINERS                                        |   1 +
>  drivers/irqchip/Kconfig                            |   5 +
>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-uniphier-aidet.c               | 252 +++++++++++++++++++++
>  5 files changed, 291 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>  create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> new file mode 100644
> index 000000000000..af57fbccd234
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
> @@ -0,0 +1,32 @@
> +UniPhier AIDET
> +
> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
> +Interrupt Controller).  GIC itself can handle only high level and rising edge
> +interrupts.  The AIDET provides logic inverter to support low level and falling
> +edge interrupts.
> +
> +Required properties:
> +- compatible: Should be one of the following:
> +    "socionext,uniphier-ld4-aidet"  - for LD4 SoC
> +    "socionext,uniphier-pro4-aidet" - for Pro4 SoC
> +    "socionext,uniphier-sld8-aidet" - for sLD8 SoC
> +    "socionext,uniphier-pro5-aidet" - for Pro5 SoC
> +    "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
> +    "socionext,uniphier-ld11-aidet" - for LD11 SoC
> +    "socionext,uniphier-ld20-aidet" - for LD20 SoC
> +    "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
> +- reg: Specifies offset and length of the register set for the device.
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
> +  source.  The value should be 2.  The first cell defines the interrupt number.
> +  The second cell specifies the trigger type as defined in interrupts.txt in
> +  this directory.

"interrupt number" is a pretty vague concept. You probably want to
explain how it relates to the GIC (is that the INTID? or the SPI number?
What about PPIs?).

> +
> +Example:
> +
> +	aidet: aidet@5fc20000 {
> +		compatible = "socionext,uniphier-pro4-aidet";
> +		reg = <0x5fc20000 0x200>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 567343b8ffaa..26b3d10b7344 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1993,6 +1993,7 @@ F:	arch/arm64/boot/dts/socionext/
>  F:	drivers/bus/uniphier-system-bus.c
>  F:	drivers/clk/uniphier/
>  F:	drivers/i2c/busses/i2c-uniphier*
> +F:	drivers/irqchip/irq-uniphier-aidet.c
>  F:	drivers/pinctrl/uniphier/
>  F:	drivers/reset/reset-uniphier.c
>  F:	drivers/tty/serial/8250/8250_uniphier.c
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f1fd5f44d1d4..a0d7218a1677 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -306,3 +306,8 @@ config QCOM_IRQ_COMBINER
>  	help
>  	  Say yes here to add support for the IRQ combiner devices embedded
>  	  in Qualcomm Technologies chips.
> +
> +config IRQ_UNIPHIER_AIDET
> +	def_bool y
> +	depends on ARCH_UNIPHIER || COMPILE_TEST
> +	select IRQ_DOMAIN_HIERARCHY
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e88d856cc09c..2c630574986f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> +obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
> diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
> new file mode 100644
> index 000000000000..8ceed5070e77
> --- /dev/null
> +++ b/drivers/irqchip/irq-uniphier-aidet.c
> @@ -0,0 +1,252 @@
> +/*
> + * Driver for UniPhier AIDET (ARM Interrupt Detector)
> + *
> + * Copyright (C) 2017 Socionext Inc.
> + *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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/bitops.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define UNIPHIER_AIDET_NR_IRQS		256
> +
> +#define UNIPHIER_AIDET_DETCONF		0x00
> +
> +struct uniphier_aidet_priv {
> +	struct irq_chip chip;
> +	struct irq_domain *domain;
> +	void __iomem *reg_base;
> +	spinlock_t lock;
> +	u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
> +};
> +
> +static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
> +				      unsigned int reg, u32 mask, u32 val)
> +{
> +	unsigned long flags;
> +	u32 tmp;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	tmp = readl(priv->reg_base + reg);
> +	tmp &= ~mask;
> +	tmp |= mask & val;
> +	writel(tmp, priv->reg_base + reg);

Given that these accesses do not seem to rely on anything making it into
memory before the access, consider using the _relaxed accessors (no need
for two DSBs here).

> +	spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
> +					  unsigned long index, unsigned int val)
> +{
> +	unsigned int reg;
> +	u32 mask;
> +
> +	reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;

What is the purpose of UNIPHIER_AIDET_DETCONF here, which is always 0?

> +	mask = BIT(index % 32);
> +
> +	uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
> +}
> +
> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct uniphier_aidet_priv *priv = data->chip_data;
> +	unsigned int val = 0;
> +
> +	/* enable inverter for active low triggers */
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:

Nit: consider moving the "val" assignment here so that the symmetry
between the two cases becomes obvious.

> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		val = 1;
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		val = 1;
> +		type = IRQ_TYPE_LEVEL_HIGH;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	uniphier_aidet_detconf_update(priv, data->hwirq, val);
> +
> +	return irq_chip_set_type_parent(data, type);
> +}
> +
> +static int uniphier_aidet_domain_translate(struct irq_domain *domain,
> +					   struct irq_fwspec *fwspec,
> +					   unsigned long *out_hwirq,
> +					   unsigned int *out_type)
> +{
> +	if (WARN_ON(fwspec->param_count < 2))
> +		return -EINVAL;
> +
> +	*out_hwirq = fwspec->param[0];
> +	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs,
> +				       void *arg)
> +{
> +	struct uniphier_aidet_priv *priv = domain->host_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int i, ret;
> +
> +	ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
> +		return -ENXIO;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		struct irq_fwspec parent_fwspec;
> +
> +		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +						    &priv->chip, priv);
> +		if (ret)
> +			return ret;
> +
> +		/* parent is GIC */
> +		parent_fwspec.fwnode = domain->parent->fwnode;
> +		parent_fwspec.param_count = 3;
> +		parent_fwspec.param[0] = 0;		/* SPI */
> +		parent_fwspec.param[1] = hwirq - 32;
> +		parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;

So this is SPI only? And your first line starts at 32? So what is in the
32bit register?

> +
> +		ret = irq_domain_alloc_irqs_parent(domain, virq, 1,
> +						   &parent_fwspec);
> +		if (ret)
> +			return ret;
> +
> +		virq++;
> +		hwirq++;
> +	}

Two things here: is there any case where you actually have nr_irqs not
being 1? As far as I know, this is only used for Multi-MSI, and nothing
else. And if you do need it, then you should probably have a slightly
better error handling (you're leaking interrupts on error here).

> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops uniphier_aidet_domain_ops = {
> +	.alloc = uniphier_aidet_domain_alloc,
> +	.free = irq_domain_free_irqs_common,
> +	.translate = uniphier_aidet_domain_translate,
> +};
> +
> +static int uniphier_aidet_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *parent_np;
> +	struct irq_domain *parent_domain;
> +	struct uniphier_aidet_priv *priv;
> +	struct resource *res;
> +
> +	parent_np = of_irq_find_parent(dev->of_node);
> +	if (!parent_np)
> +		return -ENXIO;
> +
> +	parent_domain = irq_find_host(parent_np);
> +	of_node_put(parent_np);
> +	if (!parent_domain)
> +		return -EPROBE_DEFER;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->reg_base))
> +		return PTR_ERR(priv->reg_base);
> +
> +	spin_lock_init(&priv->lock);
> +
> +	priv->chip.name = dev_name(dev);
> +	priv->chip.irq_mask = irq_chip_mask_parent;
> +	priv->chip.irq_unmask = irq_chip_unmask_parent;
> +	priv->chip.irq_eoi = irq_chip_eoi_parent;
> +	priv->chip.irq_set_type = uniphier_aidet_irq_set_type;
> +
> +	priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
> +						UNIPHIER_AIDET_NR_IRQS,
> +						dev->of_node,
> +						&uniphier_aidet_domain_ops,
> +						priv);

You seem to be able to handle multiple AIDET devices, and yet your DT
description doesn't specify a base/span for the interrupt lines that are
covered by this device. Is that something you intend to support?

> +	if (!priv->domain)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
> +{
> +	struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> +		priv->saved_vals[i] = readl(priv->reg_base +

readl_relaxed

> +					    UNIPHIER_AIDET_DETCONF + i * 4);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused uniphier_aidet_resume(struct device *dev)
> +{
> +	struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
> +		writel(priv->saved_vals[i],

writel_relaxed

> +		       priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops uniphier_aidet_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
> +				      uniphier_aidet_resume)
> +};
> +
> +static const struct of_device_id uniphier_aidet_match[] = {
> +	{ .compatible = "socionext,uniphier-ld4-aidet" },
> +	{ .compatible = "socionext,uniphier-pro4-aidet" },
> +	{ .compatible = "socionext,uniphier-sld8-aidet" },
> +	{ .compatible = "socionext,uniphier-pro5-aidet" },
> +	{ .compatible = "socionext,uniphier-pxs2-aidet" },
> +	{ .compatible = "socionext,uniphier-ld11-aidet" },
> +	{ .compatible = "socionext,uniphier-ld20-aidet" },
> +	{ .compatible = "socionext,uniphier-pxs3-aidet" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver uniphier_aidet_driver = {
> +	.probe = uniphier_aidet_probe,
> +	.driver = {
> +		.name = "uniphier-aidet",
> +		.of_match_table = uniphier_aidet_match,
> +		.pm = &uniphier_aidet_pm_ops,
> +	},
> +};
> +builtin_platform_driver(uniphier_aidet_driver);
> 

Thanks,

	M.
Masahiro Yamada Aug. 7, 2017, 11:59 a.m. UTC | #2 
Hi Marc,

Thanks for your comments.


2017-08-07 19:43 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 03/08/17 12:15, Masahiro Yamada wrote:
>> UniPhier SoCs contain AIDET (ARM Interrupt Detector).  This is intended
>> to provide additional features that are not covered by GIC.  The main
>> purpose is to provide logic inverter to support low level and falling
>> edge trigger type for interrupt lines from on-board devices.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  .../socionext,uniphier-aidet.txt                   |  32 +++
>>  MAINTAINERS                                        |   1 +
>>  drivers/irqchip/Kconfig                            |   5 +
>>  drivers/irqchip/Makefile                           |   1 +
>>  drivers/irqchip/irq-uniphier-aidet.c               | 252 +++++++++++++++++++++
>>  5 files changed, 291 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>>  create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>> new file mode 100644
>> index 000000000000..af57fbccd234
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>> @@ -0,0 +1,32 @@
>> +UniPhier AIDET
>> +
>> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
>> +Interrupt Controller).  GIC itself can handle only high level and rising edge
>> +interrupts.  The AIDET provides logic inverter to support low level and falling
>> +edge interrupts.
>> +
>> +Required properties:
>> +- compatible: Should be one of the following:
>> +    "socionext,uniphier-ld4-aidet"  - for LD4 SoC
>> +    "socionext,uniphier-pro4-aidet" - for Pro4 SoC
>> +    "socionext,uniphier-sld8-aidet" - for sLD8 SoC
>> +    "socionext,uniphier-pro5-aidet" - for Pro5 SoC
>> +    "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
>> +    "socionext,uniphier-ld11-aidet" - for LD11 SoC
>> +    "socionext,uniphier-ld20-aidet" - for LD20 SoC
>> +    "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
>> +- reg: Specifies offset and length of the register set for the device.
>> +- interrupt-controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
>> +  source.  The value should be 2.  The first cell defines the interrupt number.
>> +  The second cell specifies the trigger type as defined in interrupts.txt in
>> +  this directory.
>
> "interrupt number" is a pretty vague concept. You probably want to
> explain how it relates to the GIC (is that the INTID? or the SPI number?
> What about PPIs?).

I do not know the term "INITD".

As far as I understood from the register bit arrangements,
this hardware block seems to want to count IRQ numbers starting from SGI/PPI.

reg_offset 0x00:  for IRQ 0 - 31       (SGI/PPI)
reg_offset 0x04:  for IRQ 32 - 63      (SPI 0-31)
reg_offset 0x08:  for IRQ 64 - 95      (SPI 32-63)
reg_offset 0x0c:  for IRQ 96 - 127     (SPI 64-95)
   ...


The reg_offset 0x00 corresponds to PPI of GIC,
but never supported by this hardware.  So, reg_offset 0x00 is never used.

This hardware only supports SPI, and
the reg_offset 0x04 corresponds to the start of SPI.

Perhaps, DT binding can describe
"The first cell defines the interrupt number (SPI + 32)".



>> +     spin_lock_irqsave(&priv->lock, flags);
>> +     tmp = readl(priv->reg_base + reg);
>> +     tmp &= ~mask;
>> +     tmp |= mask & val;
>> +     writel(tmp, priv->reg_base + reg);
>
> Given that these accesses do not seem to rely on anything making it into
> memory before the access, consider using the _relaxed accessors (no need
> for two DSBs here).

Will fix.



>> +     spin_unlock_irqrestore(&priv->lock, flags);
>> +}
>> +
>> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
>> +                                       unsigned long index, unsigned int val)
>> +{
>> +     unsigned int reg;
>> +     u32 mask;
>> +
>> +     reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
>
> What is the purpose of UNIPHIER_AIDET_DETCONF here, which is always 0?

I saw a little more registers in the hardware spec, but
DETCONF was the only register base I thought useful for the Linux driver.

I can remove this macro if desired.



>> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +     struct uniphier_aidet_priv *priv = data->chip_data;
>> +     unsigned int val = 0;
>> +
>> +     /* enable inverter for active low triggers */
>> +     switch (type) {
>> +     case IRQ_TYPE_EDGE_RISING:
>> +     case IRQ_TYPE_LEVEL_HIGH:
>
> Nit: consider moving the "val" assignment here so that the symmetry
> between the two cases becomes obvious.

Will update.



>> +
>> +             /* parent is GIC */
>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>> +             parent_fwspec.param_count = 3;
>> +             parent_fwspec.param[0] = 0;             /* SPI */
>> +             parent_fwspec.param[1] = hwirq - 32;
>> +             parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>
> So this is SPI only? And your first line starts at 32? So what is in the
> 32bit register?


As mentioned above, reg_offset 0 is never used, but I guess
the developer of this hardware block wanted to match the register bit
and hwirq number.

So, I respect that and hwirq 32 of this hardware corresponds to the
start of SPI.




>> +
>> +             ret = irq_domain_alloc_irqs_parent(domain, virq, 1,
>> +                                                &parent_fwspec);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             virq++;
>> +             hwirq++;
>> +     }
>
> Two things here: is there any case where you actually have nr_irqs not
> being 1? As far as I know, this is only used for Multi-MSI, and nothing
> else. And if you do need it, then you should probably have a slightly
> better error handling (you're leaking interrupts on error here).


I always expect nr_irqs == 1.

I did not now how to handle cases where nr_irqs is greater than 1.
How should I change it?

If nr_irqs is not 1, error out immediately?

       if (nr_irqs != 1)
               return -EINVAL;



>> +     priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
>> +                                             UNIPHIER_AIDET_NR_IRQS,
>> +                                             dev->of_node,
>> +                                             &uniphier_aidet_domain_ops,
>> +                                             priv);
>
> You seem to be able to handle multiple AIDET devices, and yet your DT
> description doesn't specify a base/span for the interrupt lines that are
> covered by this device. Is that something you intend to support?


I expect only one instance of this hardware in the system.

If desired, I can allocate struct irq_chip statically:


static struct irq_chip uniphier_aidet_irq_chip = {
      .name = "UniPhier AIDET",
      .irq_mask = ...,
      .irq_unmask = ...,

     ..
};



Thanks.
Marc Zyngier Aug. 7, 2017, 1:36 p.m. UTC | #3 
On 07/08/17 12:59, Masahiro Yamada wrote:
> Hi Marc,
> 
> Thanks for your comments.
> 
> 
> 2017-08-07 19:43 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>> On 03/08/17 12:15, Masahiro Yamada wrote:
>>> UniPhier SoCs contain AIDET (ARM Interrupt Detector).  This is intended
>>> to provide additional features that are not covered by GIC.  The main
>>> purpose is to provide logic inverter to support low level and falling
>>> edge trigger type for interrupt lines from on-board devices.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  .../socionext,uniphier-aidet.txt                   |  32 +++
>>>  MAINTAINERS                                        |   1 +
>>>  drivers/irqchip/Kconfig                            |   5 +
>>>  drivers/irqchip/Makefile                           |   1 +
>>>  drivers/irqchip/irq-uniphier-aidet.c               | 252 +++++++++++++++++++++
>>>  5 files changed, 291 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>>>  create mode 100644 drivers/irqchip/irq-uniphier-aidet.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>>> new file mode 100644
>>> index 000000000000..af57fbccd234
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
>>> @@ -0,0 +1,32 @@
>>> +UniPhier AIDET
>>> +
>>> +UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
>>> +Interrupt Controller).  GIC itself can handle only high level and rising edge
>>> +interrupts.  The AIDET provides logic inverter to support low level and falling
>>> +edge interrupts.
>>> +
>>> +Required properties:
>>> +- compatible: Should be one of the following:
>>> +    "socionext,uniphier-ld4-aidet"  - for LD4 SoC
>>> +    "socionext,uniphier-pro4-aidet" - for Pro4 SoC
>>> +    "socionext,uniphier-sld8-aidet" - for sLD8 SoC
>>> +    "socionext,uniphier-pro5-aidet" - for Pro5 SoC
>>> +    "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
>>> +    "socionext,uniphier-ld11-aidet" - for LD11 SoC
>>> +    "socionext,uniphier-ld20-aidet" - for LD20 SoC
>>> +    "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
>>> +- reg: Specifies offset and length of the register set for the device.
>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>> +- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
>>> +  source.  The value should be 2.  The first cell defines the interrupt number.
>>> +  The second cell specifies the trigger type as defined in interrupts.txt in
>>> +  this directory.
>>
>> "interrupt number" is a pretty vague concept. You probably want to
>> explain how it relates to the GIC (is that the INTID? or the SPI number?
>> What about PPIs?).
> 
> I do not know the term "INITD".

That's the interrupt number as exposed by the GIC HW (see the GICv3
specification).

> 
> As far as I understood from the register bit arrangements,
> this hardware block seems to want to count IRQ numbers starting from SGI/PPI.
> 
> reg_offset 0x00:  for IRQ 0 - 31       (SGI/PPI)
> reg_offset 0x04:  for IRQ 32 - 63      (SPI 0-31)
> reg_offset 0x08:  for IRQ 64 - 95      (SPI 32-63)
> reg_offset 0x0c:  for IRQ 96 - 127     (SPI 64-95)
>    ...
> 
> 
> The reg_offset 0x00 corresponds to PPI of GIC,
> but never supported by this hardware.  So, reg_offset 0x00 is never used.
> 
> This hardware only supports SPI, and
> the reg_offset 0x04 corresponds to the start of SPI.
> 
> Perhaps, DT binding can describe
> "The first cell defines the interrupt number (SPI + 32)".

Something like that. Or you could make it easy for whoever is writing
the DT and adopt the same convention as the GIC itself, exposing the SPI
number on its own.

> 
> 
> 
>>> +     spin_lock_irqsave(&priv->lock, flags);
>>> +     tmp = readl(priv->reg_base + reg);
>>> +     tmp &= ~mask;
>>> +     tmp |= mask & val;
>>> +     writel(tmp, priv->reg_base + reg);
>>
>> Given that these accesses do not seem to rely on anything making it into
>> memory before the access, consider using the _relaxed accessors (no need
>> for two DSBs here).
> 
> Will fix.
> 
> 
> 
>>> +     spin_unlock_irqrestore(&priv->lock, flags);
>>> +}
>>> +
>>> +static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
>>> +                                       unsigned long index, unsigned int val)
>>> +{
>>> +     unsigned int reg;
>>> +     u32 mask;
>>> +
>>> +     reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
>>
>> What is the purpose of UNIPHIER_AIDET_DETCONF here, which is always 0?
> 
> I saw a little more registers in the hardware spec, but
> DETCONF was the only register base I thought useful for the Linux driver.
> 
> I can remove this macro if desired.

The convention for these register offsets is to write something like:

	writel_relaxed(val, base + REG_OFFSET + reg);

which makes it clear what this offset is. I'm happy for you to keep it
around if you write it in a similar way.

> 
> 
> 
>>> +static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
>>> +{
>>> +     struct uniphier_aidet_priv *priv = data->chip_data;
>>> +     unsigned int val = 0;
>>> +
>>> +     /* enable inverter for active low triggers */
>>> +     switch (type) {
>>> +     case IRQ_TYPE_EDGE_RISING:
>>> +     case IRQ_TYPE_LEVEL_HIGH:
>>
>> Nit: consider moving the "val" assignment here so that the symmetry
>> between the two cases becomes obvious.
> 
> Will update.
> 
> 
> 
>>> +
>>> +             /* parent is GIC */
>>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>>> +             parent_fwspec.param_count = 3;
>>> +             parent_fwspec.param[0] = 0;             /* SPI */
>>> +             parent_fwspec.param[1] = hwirq - 32;
>>> +             parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>>
>> So this is SPI only? And your first line starts at 32? So what is in the
>> 32bit register?
> 
> 
> As mentioned above, reg_offset 0 is never used, but I guess
> the developer of this hardware block wanted to match the register bit
> and hwirq number.
> 
> So, I respect that and hwirq 32 of this hardware corresponds to the
> start of SPI.

Fair enough.

> 
> 
> 
> 
>>> +
>>> +             ret = irq_domain_alloc_irqs_parent(domain, virq, 1,
>>> +                                                &parent_fwspec);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             virq++;
>>> +             hwirq++;
>>> +     }
>>
>> Two things here: is there any case where you actually have nr_irqs not
>> being 1? As far as I know, this is only used for Multi-MSI, and nothing
>> else. And if you do need it, then you should probably have a slightly
>> better error handling (you're leaking interrupts on error here).
> 
> 
> I always expect nr_irqs == 1.
> 
> I did not now how to handle cases where nr_irqs is greater than 1.
> How should I change it?
> 
> If nr_irqs is not 1, error out immediately?
> 
>        if (nr_irqs != 1)
>                return -EINVAL;

That'd be fine indeed.

> 
> 
> 
>>> +     priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
>>> +                                             UNIPHIER_AIDET_NR_IRQS,
>>> +                                             dev->of_node,
>>> +                                             &uniphier_aidet_domain_ops,
>>> +                                             priv);
>>
>> You seem to be able to handle multiple AIDET devices, and yet your DT
>> description doesn't specify a base/span for the interrupt lines that are
>> covered by this device. Is that something you intend to support?
> 
> 
> I expect only one instance of this hardware in the system.
> 
> If desired, I can allocate struct irq_chip statically:
> 
> 
> static struct irq_chip uniphier_aidet_irq_chip = {
>       .name = "UniPhier AIDET",
>       .irq_mask = ...,
>       .irq_unmask = ...,
> 
>      ..
> };

I think that would indeed make it obvious that you do not intend to
support multiple of these. And maybe simplify the name to "AIDET", as
"UniPhier AIDET" is quite large in /proc/interrupts...

Thanks,

	M.
Masahiro Yamada Aug. 8, 2017, 1:20 a.m. UTC | #4 
Hi Marc,


2017-08-07 22:36 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 07/08/17 12:59, Masahiro Yamada wrote:
>> Hi Marc,
>>
>> Thanks for your comments.
>>
>>
>> 2017-08-07 19:43 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>:
>>> On 03/08/17 12:15, Masahiro Yamada wrote:
>>>> UniPhier SoCs contain AIDET (ARM Interrupt Detector).  This is intended
>>>> to provide additional features that are not covered by GIC.  The main
>>>> purpose is to provide logic inverter to support low level and falling
>>>> edge trigger type for interrupt lines from on-board devices.
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Thanks.  I will send v2 based on your comments.



If possible, could you help me with my GPIO driver?

I implemented a similar IRQ domain hierarchy handling in the driver.

This patch:
http://patchwork.ozlabs.org/patch/797145/


If I am understanding correctly, the IRQ mapping to the parent
must be somehow hard-coded in the .alloc() hook of irq_domain_ops.

I asked a question in the following.
https://lkml.org/lkml/2017/7/6/758


I do not have a solution to get the IRQ info from "interrupts" property,
so I am hard-coding it in the driver.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
new file mode 100644
index 000000000000..af57fbccd234
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/socionext,uniphier-aidet.txt
@@ -0,0 +1,32 @@ 
+UniPhier AIDET
+
+UniPhier AIDET (ARM Interrupt Detector) is an add-on block for ARM GIC (Generic
+Interrupt Controller).  GIC itself can handle only high level and rising edge
+interrupts.  The AIDET provides logic inverter to support low level and falling
+edge interrupts.
+
+Required properties:
+- compatible: Should be one of the following:
+    "socionext,uniphier-ld4-aidet"  - for LD4 SoC
+    "socionext,uniphier-pro4-aidet" - for Pro4 SoC
+    "socionext,uniphier-sld8-aidet" - for sLD8 SoC
+    "socionext,uniphier-pro5-aidet" - for Pro5 SoC
+    "socionext,uniphier-pxs2-aidet" - for PXs2/LD6b SoC
+    "socionext,uniphier-ld11-aidet" - for LD11 SoC
+    "socionext,uniphier-ld20-aidet" - for LD20 SoC
+    "socionext,uniphier-pxs3-aidet" - for PXs3 SoC
+- reg: Specifies offset and length of the register set for the device.
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an interrupt
+  source.  The value should be 2.  The first cell defines the interrupt number.
+  The second cell specifies the trigger type as defined in interrupts.txt in
+  this directory.
+
+Example:
+
+	aidet: aidet@5fc20000 {
+		compatible = "socionext,uniphier-pro4-aidet";
+		reg = <0x5fc20000 0x200>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 567343b8ffaa..26b3d10b7344 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1993,6 +1993,7 @@  F:	arch/arm64/boot/dts/socionext/
 F:	drivers/bus/uniphier-system-bus.c
 F:	drivers/clk/uniphier/
 F:	drivers/i2c/busses/i2c-uniphier*
+F:	drivers/irqchip/irq-uniphier-aidet.c
 F:	drivers/pinctrl/uniphier/
 F:	drivers/reset/reset-uniphier.c
 F:	drivers/tty/serial/8250/8250_uniphier.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..a0d7218a1677 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -306,3 +306,8 @@  config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config IRQ_UNIPHIER_AIDET
+	def_bool y
+	depends on ARCH_UNIPHIER || COMPILE_TEST
+	select IRQ_DOMAIN_HIERARCHY
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e88d856cc09c..2c630574986f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
diff --git a/drivers/irqchip/irq-uniphier-aidet.c b/drivers/irqchip/irq-uniphier-aidet.c
new file mode 100644
index 000000000000..8ceed5070e77
--- /dev/null
+++ b/drivers/irqchip/irq-uniphier-aidet.c
@@ -0,0 +1,252 @@ 
+/*
+ * Driver for UniPhier AIDET (ARM Interrupt Detector)
+ *
+ * Copyright (C) 2017 Socionext Inc.
+ *   Author: Masahiro Yamada <yamada.masahiro@socionext.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 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/bitops.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define UNIPHIER_AIDET_NR_IRQS		256
+
+#define UNIPHIER_AIDET_DETCONF		0x00
+
+struct uniphier_aidet_priv {
+	struct irq_chip chip;
+	struct irq_domain *domain;
+	void __iomem *reg_base;
+	spinlock_t lock;
+	u32 saved_vals[UNIPHIER_AIDET_NR_IRQS / 32];
+};
+
+static void uniphier_aidet_reg_update(struct uniphier_aidet_priv *priv,
+				      unsigned int reg, u32 mask, u32 val)
+{
+	unsigned long flags;
+	u32 tmp;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	tmp = readl(priv->reg_base + reg);
+	tmp &= ~mask;
+	tmp |= mask & val;
+	writel(tmp, priv->reg_base + reg);
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static void uniphier_aidet_detconf_update(struct uniphier_aidet_priv *priv,
+					  unsigned long index, unsigned int val)
+{
+	unsigned int reg;
+	u32 mask;
+
+	reg = UNIPHIER_AIDET_DETCONF + index / 32 * 4;
+	mask = BIT(index % 32);
+
+	uniphier_aidet_reg_update(priv, reg, mask, val ? mask : 0);
+}
+
+static int uniphier_aidet_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct uniphier_aidet_priv *priv = data->chip_data;
+	unsigned int val = 0;
+
+	/* enable inverter for active low triggers */
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		val = 1;
+		type = IRQ_TYPE_EDGE_RISING;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		val = 1;
+		type = IRQ_TYPE_LEVEL_HIGH;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	uniphier_aidet_detconf_update(priv, data->hwirq, val);
+
+	return irq_chip_set_type_parent(data, type);
+}
+
+static int uniphier_aidet_domain_translate(struct irq_domain *domain,
+					   struct irq_fwspec *fwspec,
+					   unsigned long *out_hwirq,
+					   unsigned int *out_type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+
+	*out_hwirq = fwspec->param[0];
+	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static int uniphier_aidet_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs,
+				       void *arg)
+{
+	struct uniphier_aidet_priv *priv = domain->host_data;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int i, ret;
+
+	ret = uniphier_aidet_domain_translate(domain, arg, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	if (hwirq >= UNIPHIER_AIDET_NR_IRQS)
+		return -ENXIO;
+
+	for (i = 0; i < nr_irqs; i++) {
+		struct irq_fwspec parent_fwspec;
+
+		ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+						    &priv->chip, priv);
+		if (ret)
+			return ret;
+
+		/* parent is GIC */
+		parent_fwspec.fwnode = domain->parent->fwnode;
+		parent_fwspec.param_count = 3;
+		parent_fwspec.param[0] = 0;		/* SPI */
+		parent_fwspec.param[1] = hwirq - 32;
+		parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
+
+		ret = irq_domain_alloc_irqs_parent(domain, virq, 1,
+						   &parent_fwspec);
+		if (ret)
+			return ret;
+
+		virq++;
+		hwirq++;
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops uniphier_aidet_domain_ops = {
+	.alloc = uniphier_aidet_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+	.translate = uniphier_aidet_domain_translate,
+};
+
+static int uniphier_aidet_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent_np;
+	struct irq_domain *parent_domain;
+	struct uniphier_aidet_priv *priv;
+	struct resource *res;
+
+	parent_np = of_irq_find_parent(dev->of_node);
+	if (!parent_np)
+		return -ENXIO;
+
+	parent_domain = irq_find_host(parent_np);
+	of_node_put(parent_np);
+	if (!parent_domain)
+		return -EPROBE_DEFER;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->reg_base))
+		return PTR_ERR(priv->reg_base);
+
+	spin_lock_init(&priv->lock);
+
+	priv->chip.name = dev_name(dev);
+	priv->chip.irq_mask = irq_chip_mask_parent;
+	priv->chip.irq_unmask = irq_chip_unmask_parent;
+	priv->chip.irq_eoi = irq_chip_eoi_parent;
+	priv->chip.irq_set_type = uniphier_aidet_irq_set_type;
+
+	priv->domain = irq_domain_add_hierarchy(parent_domain, 0,
+						UNIPHIER_AIDET_NR_IRQS,
+						dev->of_node,
+						&uniphier_aidet_domain_ops,
+						priv);
+	if (!priv->domain)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int __maybe_unused uniphier_aidet_suspend(struct device *dev)
+{
+	struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+		priv->saved_vals[i] = readl(priv->reg_base +
+					    UNIPHIER_AIDET_DETCONF + i * 4);
+
+	return 0;
+}
+
+static int __maybe_unused uniphier_aidet_resume(struct device *dev)
+{
+	struct uniphier_aidet_priv *priv = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->saved_vals); i++)
+		writel(priv->saved_vals[i],
+		       priv->reg_base + UNIPHIER_AIDET_DETCONF + i * 4);
+
+	return 0;
+}
+
+static const struct dev_pm_ops uniphier_aidet_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(uniphier_aidet_suspend,
+				      uniphier_aidet_resume)
+};
+
+static const struct of_device_id uniphier_aidet_match[] = {
+	{ .compatible = "socionext,uniphier-ld4-aidet" },
+	{ .compatible = "socionext,uniphier-pro4-aidet" },
+	{ .compatible = "socionext,uniphier-sld8-aidet" },
+	{ .compatible = "socionext,uniphier-pro5-aidet" },
+	{ .compatible = "socionext,uniphier-pxs2-aidet" },
+	{ .compatible = "socionext,uniphier-ld11-aidet" },
+	{ .compatible = "socionext,uniphier-ld20-aidet" },
+	{ .compatible = "socionext,uniphier-pxs3-aidet" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver uniphier_aidet_driver = {
+	.probe = uniphier_aidet_probe,
+	.driver = {
+		.name = "uniphier-aidet",
+		.of_match_table = uniphier_aidet_match,
+		.pm = &uniphier_aidet_pm_ops,
+	},
+};
+builtin_platform_driver(uniphier_aidet_driver);