Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller
On 2015-01-08 10:13, Thierry Reding wrote: On Wed, Jan 07, 2015 at 05:42:37PM +, Marc Zyngier wrote: Tegra's LIC (Legacy Interrupt Controller) has been so far only supported as a weird extension of the GIC, which is not exactly pretty. The stacked irq domain framework fits this pretty well, and allows Nit: s/irq/IRQ/ the LIC code to be turned into a standalone irqchip. In the process, make the driver DT aware, something that was sorely missing from the mach-tegra implementation. Signed-off-by: Marc Zyngier --- drivers/irqchip/Makefile| 1 + drivers/irqchip/irq-tegra.c | 335 2 files changed, 336 insertions(+) create mode 100644 drivers/irqchip/irq-tegra.c This matches largely what I have in a local patch (modulo the stacked domains vs. gic_arch_extn). A few comments below. [snip] Thanks for the extensive review. I've implemented all of this, except for the hunk below: diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 9516a32..59f34be 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_HIP04)+= irq-hip04.o obj-$(CONFIG_ARCH_MMP) += irq-mmp.o obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o obj-$(CONFIG_ARCH_MXS) += irq-mxs.o +obj-$(CONFIG_ARCH_TEGRA) += irq-tegra.o obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o Should these be sorted alphabetically? Well, the left side is (up to S3C24xx, and then it all goes down the drain). Do you have a suggestion? Thanks, M. -- Fast, cheap, reliable. Pick two. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller
On 2015-01-08 15:06, Nishanth Menon wrote: On 17:42-20150107, Marc Zyngier wrote: Tegra's LIC (Legacy Interrupt Controller) has been so far only supported as a weird extension of the GIC, which is not exactly pretty. The stacked irq domain framework fits this pretty well, and allows the LIC code to be turned into a standalone irqchip. In the process, make the driver DT aware, something that was sorely missing from the mach-tegra implementation. Signed-off-by: Marc Zyngier --- Saw a few checkpatch warnings as below: all of them seem minors. @@ -0,0 +1,35 @@ +WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? I'll leave to the Tegra maintainers to update this file it they want to. +#36: +new file mode 100644 +WARNING: line over 80 characters +#169: FILE: drivers/irqchip/irq-tegra.c:129: ++ tegra_ictlr_info->cpu_ier[i] = readl_relaxed(ictlr + ICTLR_CPU_IER); As a matter of principle, I ignore what checkpatch says about the length of lines of code. I trust my eyes more than the tool. [...] +WARNING: Missing a blank line after declarations +#196: FILE: drivers/irqchip/irq-tegra.c:156: ++ void __iomem *ictlr = tegra_ictlr_info->ictlr_reg_base[i]; ++ writel_relaxed(tegra_ictlr_info->cpu_iep[i], Done. +WARNING: line over 80 characters +#284: FILE: drivers/irqchip/irq-tegra.c:244: ++ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_args); +CHECK: Please don't use multiple blank lines +#287: FILE: drivers/irqchip/irq-tegra.c:247: ++ ++ +WARNING: Missing a blank line after declarations +#296: FILE: drivers/irqchip/irq-tegra.c:256: ++ struct irq_data *d = irq_domain_get_irq_data(domain, virq + i); ++ irq_domain_reset_irq_data(d); ` Thanks, M. -- Fast, cheap, reliable. Pick two. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller
On 17:42-20150107, Marc Zyngier wrote: > Tegra's LIC (Legacy Interrupt Controller) has been so far only > supported as a weird extension of the GIC, which is not exactly > pretty. > > The stacked irq domain framework fits this pretty well, and allows > the LIC code to be turned into a standalone irqchip. In the process, > make the driver DT aware, something that was sorely missing from > the mach-tegra implementation. > > Signed-off-by: Marc Zyngier > --- Saw a few checkpatch warnings as below: all of them seem minors. @@ -0,0 +1,35 @@ +WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? +#36: +new file mode 100644 +WARNING: line over 80 characters +#169: FILE: drivers/irqchip/irq-tegra.c:129: ++ tegra_ictlr_info->cpu_ier[i] = readl_relaxed(ictlr + ICTLR_CPU_IER); +WARNING: line over 80 characters +#170: FILE: drivers/irqchip/irq-tegra.c:130: ++ tegra_ictlr_info->cpu_iep[i] = readl_relaxed(ictlr + ICTLR_CPU_IEP_CLASS); +WARNING: line over 80 characters +#171: FILE: drivers/irqchip/irq-tegra.c:131: ++ tegra_ictlr_info->cop_ier[i] = readl_relaxed(ictlr + ICTLR_COP_IER); +WARNING: line over 80 characters +#172: FILE: drivers/irqchip/irq-tegra.c:132: ++ tegra_ictlr_info->cop_iep[i] = readl_relaxed(ictlr + ICTLR_COP_IEP_CLASS); +WARNING: line over 80 characters +#181: FILE: drivers/irqchip/irq-tegra.c:141: ++ writel_relaxed(tegra_ictlr_info->ictlr_wake_mask[i], ictlr + ICTLR_CPU_IER_SET); +WARNING: Missing a blank line after declarations +#196: FILE: drivers/irqchip/irq-tegra.c:156: ++ void __iomem *ictlr = tegra_ictlr_info->ictlr_reg_base[i]; ++ writel_relaxed(tegra_ictlr_info->cpu_iep[i], +WARNING: line over 80 characters +#284: FILE: drivers/irqchip/irq-tegra.c:244: ++ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_args); +CHECK: Please don't use multiple blank lines +#287: FILE: drivers/irqchip/irq-tegra.c:247: ++ ++ +WARNING: Missing a blank line after declarations +#296: FILE: drivers/irqchip/irq-tegra.c:256: ++ struct irq_data *d = irq_domain_get_irq_data(domain, virq + i); ++ irq_domain_reset_irq_data(d); ` -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller
On Wed, Jan 07, 2015 at 05:42:37PM +, Marc Zyngier wrote:
> Tegra's LIC (Legacy Interrupt Controller) has been so far only
> supported as a weird extension of the GIC, which is not exactly
> pretty.
>
> The stacked irq domain framework fits this pretty well, and allows
Nit: s/irq/IRQ/
> the LIC code to be turned into a standalone irqchip. In the process,
> make the driver DT aware, something that was sorely missing from
> the mach-tegra implementation.
>
> Signed-off-by: Marc Zyngier
> ---
> drivers/irqchip/Makefile| 1 +
> drivers/irqchip/irq-tegra.c | 335
>
> 2 files changed, 336 insertions(+)
> create mode 100644 drivers/irqchip/irq-tegra.c
This matches largely what I have in a local patch (modulo the stacked
domains vs. gic_arch_extn). A few comments below.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 9516a32..59f34be 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_MMP) += irq-mmp.o
> obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
> obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
> +obj-$(CONFIG_ARCH_TEGRA) += irq-tegra.o
> obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
Should these be sorted alphabetically?
> diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
[...]
> +#define TEGRA_MAX_NUM_ICTLRS 5
> +
> +static int num_ictlrs;
This could be unsigned int.
> +struct tegra_ictlr_info {
> + void __iomem *ictlr_reg_base[TEGRA_MAX_NUM_ICTLRS];
Maybe only "reg_base" or "base". The ictlr_ prefix is redundant.
> +#ifdef CONFIG_PM_SLEEP
> + u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
> + u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
> + u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
> + u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
> +
> + u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
Same here.
> +#endif
> +};
> +
> +static struct tegra_ictlr_info *tegra_ictlr_info;
This variable name could be shorter, say "lic", to make some of the code
below more terse.
> +static int tegra_ictlr_suspend(void)
> +{
> + unsigned long flags;
> + int i;
This could be unsigned int, too.
[...]
> +static void tegra_ictlr_resume(void)
> +{
> + unsigned long flags;
> + int i;
And here.
> +static struct syscore_ops tegra_ictlr_syscore_ops = {
> + .suspend= tegra_ictlr_suspend,
> + .resume = tegra_ictlr_resume,
> +};
> +
> +static int tegra_ictlr_syscore_init(void)
> +{
> + register_syscore_ops(&tegra_ictlr_syscore_ops);
> +
> + return 0;
> +}
> +#else
> +#define tegra_set_wake NULL
> +static inline void tegra_ictlr_syscore_init(void) {}
> +#endif
Both prototypes for tegra_ictlr_syscore_init() should match here. Since
it never fails, returning void for both should be fine.
> +static struct irq_chip tegra_ictlr_chip = {
> + .name = "ICTLR",
Maybe name it "LIC" since that's the more common name for it?
> +static int tegra_ictlr_domain_xlate(struct irq_domain *domain,
> + struct device_node *controller,
> + const u32 *intspec,
> + unsigned int intsize,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (domain->of_node != controller)
> + return -EINVAL; /* Shouldn't happen, really... */
> + if (intsize != 3)
> + return -EINVAL; /* Not GIC compliant */
> + if (intspec[0] != 0)
Maybe use GIC_SPI from dt-bindings/interrupt-controller/arm-gic.h here?
To match the DTS content?
> +static int tegra_ictlr_domain_alloc(struct irq_domain *domain,
> + unsigned int virq,
> + unsigned int nr_irqs, void *data)
> +{
> + struct of_phandle_args *args = data;
> + struct of_phandle_args parent_args;
> + struct tegra_ictlr_info *info = domain->host_data;
> + irq_hw_number_t hwirq;
> + int i;
unsigned int?
> +
> + if (args->args_count != 3)
> + return -EINVAL; /* Not GIC compliant */
> + if (args->args[0] != 0)
GIC_SPI?
> + hwirq = args->args[1];
> + if (hwirq >= (num_ictlrs * 32))
> + return -EINVAL; /* Can't deal with this */
Not sure if that comment is necessary here. It's fairly obvious why this
is an error. But it doesn't hurt, so if you prefer to leave it, that's
fine with me.
> +
> + for (i = 0; i < nr_irqs; i++) {
> + int ictlr = (hwirq + i) / 32;
irq_hw_number_t? Or unsigned int?
> +
> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &tegra_ictlr_chip,
> + &info->ictlr_reg_base[ictlr]);
> + }
> +
> + parent_args = *a
[PATCH v2 02/21] irqchip: tegra: add DT-based support for legacy interrupt controller
Tegra's LIC (Legacy Interrupt Controller) has been so far only
supported as a weird extension of the GIC, which is not exactly
pretty.
The stacked irq domain framework fits this pretty well, and allows
the LIC code to be turned into a standalone irqchip. In the process,
make the driver DT aware, something that was sorely missing from
the mach-tegra implementation.
Signed-off-by: Marc Zyngier
---
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-tegra.c | 335
2 files changed, 336 insertions(+)
create mode 100644 drivers/irqchip/irq-tegra.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..59f34be 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_HIP04)+= irq-hip04.o
obj-$(CONFIG_ARCH_MMP) += irq-mmp.o
obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
+obj-$(CONFIG_ARCH_TEGRA) += irq-tegra.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
obj-$(CONFIG_METAG)+= irq-metag-ext.o
diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
new file mode 100644
index 000..b4fc2e3
--- /dev/null
+++ b/drivers/irqchip/irq-tegra.c
@@ -0,0 +1,335 @@
+/*
+ * Driver code for Tegra's Legacy Interrupt Controller
+ *
+ * Author: Marc Zyngier
+ *
+ * Heavily based on the original arch/arm/mach-tegra/irq.c code:
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * Author:
+ * Colin Cross
+ *
+ * Copyright (C) 2010,2013, NVIDIA Corporation
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+
+#include "irqchip.h"
+
+#define ICTLR_CPU_IEP_VFIQ 0x08
+#define ICTLR_CPU_IEP_FIR 0x14
+#define ICTLR_CPU_IEP_FIR_SET 0x18
+#define ICTLR_CPU_IEP_FIR_CLR 0x1c
+
+#define ICTLR_CPU_IER 0x20
+#define ICTLR_CPU_IER_SET 0x24
+#define ICTLR_CPU_IER_CLR 0x28
+#define ICTLR_CPU_IEP_CLASS0x2C
+
+#define ICTLR_COP_IER 0x30
+#define ICTLR_COP_IER_SET 0x34
+#define ICTLR_COP_IER_CLR 0x38
+#define ICTLR_COP_IEP_CLASS0x3c
+
+#define TEGRA_MAX_NUM_ICTLRS 5
+
+static int num_ictlrs;
+
+struct tegra_ictlr_info {
+ void __iomem *ictlr_reg_base[TEGRA_MAX_NUM_ICTLRS];
+#ifdef CONFIG_PM_SLEEP
+ u32 cop_ier[TEGRA_MAX_NUM_ICTLRS];
+ u32 cop_iep[TEGRA_MAX_NUM_ICTLRS];
+ u32 cpu_ier[TEGRA_MAX_NUM_ICTLRS];
+ u32 cpu_iep[TEGRA_MAX_NUM_ICTLRS];
+
+ u32 ictlr_wake_mask[TEGRA_MAX_NUM_ICTLRS];
+#endif
+};
+
+static struct tegra_ictlr_info *tegra_ictlr_info;
+
+static inline void tegra_ictlr_write_mask(struct irq_data *d, unsigned long
reg)
+{
+ void __iomem *base = d->chip_data;
+ u32 mask;
+
+ mask = BIT(d->hwirq % 32);
+ writel_relaxed(mask, base + reg);
+}
+
+static void tegra_mask(struct irq_data *d)
+{
+ tegra_ictlr_write_mask(d, ICTLR_CPU_IER_CLR);
+ irq_chip_mask_parent(d);
+}
+
+static void tegra_unmask(struct irq_data *d)
+{
+ tegra_ictlr_write_mask(d, ICTLR_CPU_IER_SET);
+ irq_chip_unmask_parent(d);
+}
+
+static void tegra_eoi(struct irq_data *d)
+{
+ tegra_ictlr_write_mask(d, ICTLR_CPU_IEP_FIR_CLR);
+ irq_chip_eoi_parent(d);
+}
+
+static int tegra_retrigger(struct irq_data *d)
+{
+ tegra_ictlr_write_mask(d, ICTLR_CPU_IEP_FIR_SET);
+ return irq_chip_retrigger_hierarchy(d);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int tegra_set_wake(struct irq_data *d, unsigned int enable)
+{
+ u32 irq = d->hwirq;
+ u32 index, mask;
+
+ index = (irq / 32);
+ mask = BIT(irq % 32);
+ if (enable)
+ tegra_ictlr_info->ictlr_wake_mask[index] |= mask;
+ else
+ tegra_ictlr_info->ictlr_wake_mask[index] &= ~mask;
+
+ /*
+* Do *not* call into the parent, as the GIC doesn't have any
+* wake-up facility...
+*/
+ return 0;
+}
+
+static int tegra_ictlr_suspend(void)
+{
+ unsigned long flags;
+ int i;
+
+ local_irq_save(flags);
+ for (i = 0; i < num_ictlrs; i++) {
+ void __iomem *ictlr = tegra_ictlr_info->ictlr_reg_base[i];
+ /* Save interrupt state */
+ tegra_ictlr_info->cpu_ier[i] = readl_relaxed(ictlr +
ICTLR_CPU_IER);
+ tegra_ictlr_info->cpu_iep[i] = readl_relaxed(ictlr +
ICTLR_CPU_IEP_CLASS);
+ teg
