On 2014-12-15 10:59, Marc Zyngier wrote: > Hi Stefan, > > On 14/12/14 22:09, Stefan Agner wrote: >> This adds support for Vybrid's interrupt router. On VF6xx models, >> almost all peripherals can be accessed from either of the two >> CPU's, from the Cortex-A5 or from the Cortex-M4. The interrupt >> router routes the peripheral interrupts to the configured CPU. >> >> The driver makes use of the irqdomain hierarchy support. The >> parent is either the ARM GIC or the ARM NVIC interrupt controller >> depending on which CPU the kernel is executed on. Currently only >> ARM GIC is supported because the NVIC driver lacks hierarchical >> irqdomain support as of now. >> >> Currently, there is no resource control mechnism implemented to >> avoid concurrent access of the same peripheral. The user needs >> to make sure to use device trees which assign the peripherals >> orthogonally. However, this driver warns the user in case the >> interrupt is already configured for the other CPU. This provides >> a poor man's resource controller. >> >> Signed-off-by: Stefan Agner <stefan@xxxxxxxx> >> --- >> Thanks for the feedback on the initial driver, I'm quite happy >> with the outcome using the hierarchic irqdomain support. > > Great stuff, pleased to see the stacked domain proving to be useful. > >> The driver is tested on Vybrid running on the Cortex-A5 CPU. >> However, to properly support Cortex-M4, some more work will be >> needed. Beside the hierarchic irqdomain support for NVIC, the >> different IRQ cell layout need to be solved: NVIC uses only >> one cell, whereas GIC uses three. I see two possible solutions: >> - Support two layouts in this driver. Maybe by using IS_ENABLED, >> since it is not possible to compile a kernel for the A5 and >> M4. >> - Define a 3 cell layout as GIC uses it for the MSCM, and pass >> a syntetic one cell layout to the parent when calling >> irq_domain_alloc_irqs_parent. This driver would then still >> need to know what type of interrupt controller the parent is... >> >> Ideas/advice welcome... > > You shouldn't use the GIC format for the MSCM, as it doesn't mean > anything for it. Yes, I know that everybody did that, but that's just > wrong (MSCM itself shouldn't care about SPIs, except when it is actually > talking to a GIC). The only reason I didn't clean that up in my ongoing > series is to avoid having to rewrite all the DTs entirely. > > My hunch is that you should have a MSCM-specific interrupt description > (I guess two cells should be enough, one for the interrupt number and > one for the trigger if necessary), and translate this to the format that > the backing interrupt controller is using (only the map function should > be affected). Ok, so foremost you suggest to use always the same interrupt specification, no matter if I use the dt for the A5 or the M4. Hm, just some weeks ago I extracted the interrupt properties of all peripherals and made a base device tree without interrupt properties, just so that I could create a device tree with the interrupt properties for NVIC and one for GIC (see vf500.dtsi vs the preliminary vf610m4.dtsi from the Cortex-M4 support patchset). Back then, I did not put much thought into MSCM etc., and just adjusted the interrupt properties to the needs of those two interrupt controllers. When having a common definition, I can merge those interrupt nodes back into the common device tree, which is much nicer anyway! Regarding format, since I have to touch all the interrupt properties anyway, it's not much hassle to use a new format in that process. So my MSCM format would be, as you suggested, two cells with interrupt number and the trigger specification (IRQ_TYPE... from ./include/dt-bindings/interrupt-controller/irq.h). One open thing: How should I determine the backing interrupt controller? Maybe by just reading the interrupt-cells property of the parent interrupt controller, and depending on the cell count create that format? > > That would also make your DT binding a lot less ambiguous. > > Other than that, it looks pretty good to me. Just a few cosmetic remarks > below: > >> >> arch/arm/mach-imx/Kconfig | 1 + >> drivers/irqchip/Kconfig | 11 +++ >> drivers/irqchip/Makefile | 1 + >> drivers/irqchip/irq-vf610-mscm.c | 198 +++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 211 insertions(+) >> create mode 100644 drivers/irqchip/irq-vf610-mscm.c >> >> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig >> index e8627e0..3c5859e 100644 >> --- a/arch/arm/mach-imx/Kconfig >> +++ b/arch/arm/mach-imx/Kconfig >> @@ -631,6 +631,7 @@ config SOC_IMX6SX >> >> config SOC_VF610 >> bool "Vybrid Family VF610 support" >> + select VF610_MSCM >> select ARM_GIC >> select PINCTRL_VF610 >> select PL310_ERRATA_769419 if CACHE_L2X0 >> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >> index cc79d2a..af5e72a 100644 >> --- a/drivers/irqchip/Kconfig >> +++ b/drivers/irqchip/Kconfig >> @@ -136,6 +136,17 @@ config IRQ_CROSSBAR >> a free irq and configures the IP. Thus the peripheral interrupts are >> routed to one of the free irqchip interrupt lines. >> >> +config VF610_MSCM >> + bool >> + help >> + Support for MSCM interrupt router available on Vybrid SoC's. The >> + interrupt router is between the CPU's interrupt controller and the >> + peripheral. The router allows to route the peripheral interrupts to >> + one of the two available CPU's on Vybrid VF6xx SoC's (Cortex-A5 or >> + Cortex-M4). The router will be configured transparently on a IRQ >> + request. >> + select IRQ_DOMAIN_HIERARCHY >> + >> config KEYSTONE_IRQ >> tristate "Keystone 2 IRQ controller IP" >> depends on ARCH_KEYSTONE >> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >> index 9516a32..85651be 100644 >> --- a/drivers/irqchip/Makefile >> +++ b/drivers/irqchip/Makefile >> @@ -37,6 +37,7 @@ obj-$(CONFIG_TB10X_IRQC) += irq-tb10x.o >> obj-$(CONFIG_XTENSA) += irq-xtensa-pic.o >> obj-$(CONFIG_XTENSA_MX) += irq-xtensa-mx.o >> obj-$(CONFIG_IRQ_CROSSBAR) += irq-crossbar.o >> +obj-$(CONFIG_VF610_MSCM) += irq-vf610-mscm.o >> obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o >> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o >> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o >> diff --git a/drivers/irqchip/irq-vf610-mscm.c b/drivers/irqchip/irq-vf610-mscm.c >> new file mode 100644 >> index 0000000..1597185 >> --- /dev/null >> +++ b/drivers/irqchip/irq-vf610-mscm.c >> @@ -0,0 +1,198 @@ >> +/* >> + * Copyright 2014 Stefan Agner >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +#include <linux/cpu_pm.h> >> +#include <linux/io.h> >> +#include <linux/irq.h> >> +#include <linux/irqdomain.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/slab.h> >> + >> +#include "irqchip.h" >> + >> +#define MSCM_CPxNUM 0x4 >> +#define MSCM_IRSPRC(n) (0x880 + 2 * (n)) >> +#define MSCM_IRSPRC_CPEN_MASK 0x3 >> + >> +#define MSCM_IRSPRC_NUM 112 >> + >> +struct vf610_mscm_chip_data { >> + void __iomem *mscm_base; >> + u16 cpu_mask; >> + u16 mscm_saved_irsprc[MSCM_IRSPRC_NUM]; >> +}; >> + >> +static struct vf610_mscm_chip_data *chip_data; >> + >> +static int vf610_mscm_notifier(struct notifier_block *self, unsigned long cmd, >> + void *v) >> +{ >> + int i; >> + >> + switch (cmd) { >> + case CPU_CLUSTER_PM_ENTER: >> + for (i = 0; i < MSCM_IRSPRC_NUM; i++) >> + chip_data->mscm_saved_irsprc[i] = readw_relaxed( >> + chip_data->mscm_base + MSCM_IRSPRC(i)); > > Please keep the argument to readw_relaxed on the same line as the > function call. Makes it easier to read. > >> + break; >> + case CPU_CLUSTER_PM_ENTER_FAILED: >> + case CPU_CLUSTER_PM_EXIT: >> + for (i = 0; i < MSCM_IRSPRC_NUM; i++) >> + writew_relaxed(chip_data->mscm_saved_irsprc[i], >> + chip_data->mscm_base + MSCM_IRSPRC(i)); >> + break; >> + } >> + >> + return NOTIFY_OK; >> +} >> + >> +static struct notifier_block mscm_notifier_block = { >> + .notifier_call = vf610_mscm_notifier, >> +}; >> + >> +static void vf610_mscm_enable(struct irq_data *data) >> +{ >> + irq_hw_number_t hwirq = data->hwirq; >> + struct vf610_mscm_chip_data *chip_data = data->chip_data; >> + u16 irsprc; >> + >> + irsprc = readw_relaxed(chip_data->mscm_base + MSCM_IRSPRC(hwirq)); >> + irsprc &= MSCM_IRSPRC_CPEN_MASK; >> + >> + WARN_ON(irsprc); > > Does this read have an influence on the interrupt routing? No it doesn't. The warn here implements the poor man's resource controller (see commit message above). > >> + >> + writew_relaxed(chip_data->cpu_mask, >> + chip_data->mscm_base + MSCM_IRSPRC(hwirq)); >> + >> + irq_chip_unmask_parent(data); >> +} >> + >> +static void vf610_mscm_disable(struct irq_data *data) >> +{ >> + irq_hw_number_t hwirq = data->hwirq; >> + struct vf610_mscm_chip_data *chip_data = data->chip_data; >> + u16 irsprc; >> + >> + irsprc = readw_relaxed(chip_data->mscm_base + MSCM_IRSPRC(hwirq)); >> + irsprc &= MSCM_IRSPRC_CPEN_MASK; > > And this one? Ok, I had a warning in disable too before, but now this read is really superfluous. > >> + writew_relaxed(0x0, chip_data->mscm_base + MSCM_IRSPRC(hwirq)); >> + >> + irq_chip_mask_parent(data); >> +} >> + >> +static struct irq_chip vf610_mscm_irq_chip = { >> + .name = "MSCM_IR", >> + .irq_mask = irq_chip_mask_parent, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_enable = vf610_mscm_enable, >> + .irq_disable = vf610_mscm_disable, >> + .irq_retrigger = irq_chip_retrigger_hierarchy, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> +}; >> + >> +static int vf610_mscm_domain_xlate(struct irq_domain *d, >> + struct device_node *controller, >> + const u32 *intspec, unsigned int intsize, >> + unsigned long *out_hwirq, >> + unsigned int *out_type) >> +{ >> + if (intsize != 3) >> + return -EINVAL; >> + >> + /* MSCM doesn't handle PPI */ >> + if (intspec[0]) >> + return -EINVAL; >> + >> + *out_hwirq = intspec[1]; >> + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK; >> + return 0; >> +} >> + >> +static int vf610_mscm_domain_alloc(struct irq_domain *domain, unsigned int virq, >> + unsigned int nr_irqs, void *arg) >> +{ >> + int i; >> + irq_hw_number_t hwirq; >> + struct of_phandle_args *irq_data = arg; >> + struct of_phandle_args gic_data = *irq_data; >> + >> + if (irq_data->args_count != 3) >> + return -EINVAL; >> + >> + /* MSCM doesn't handle PPI */ >> + if (irq_data->args[0]) >> + return -EINVAL; >> + >> + hwirq = irq_data->args[1]; >> + for (i = 0; i < nr_irqs; i++) >> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, >> + &vf610_mscm_irq_chip, >> + domain->host_data); >> + >> + gic_data.np = domain->parent->of_node; >> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_data); >> +} >> + >> +static const struct irq_domain_ops mscm_irq_domain_ops = { >> + .xlate = vf610_mscm_domain_xlate, >> + .alloc = vf610_mscm_domain_alloc, >> + .free = irq_domain_free_irqs_common, >> +}; >> + >> +static int __init vf610_mscm_of_init(struct device_node *node, >> + struct device_node *parent) >> +{ >> + struct irq_domain *domain, *domain_parent; >> + int ret; >> + >> + domain_parent = irq_find_host(parent); >> + if (!domain_parent) { >> + pr_err("vf610_mscm: interrupt-parent not found\n"); >> + return -EINVAL; >> + } >> + >> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); >> + if (!chip_data) >> + return -ENOMEM; >> + >> + chip_data->mscm_base = of_io_request_and_map(node, 0, "mscm"); >> + >> + if (!chip_data->mscm_base) { >> + pr_err("vf610_mscm: unable to map mscm register\n"); >> + ret = -ENOMEM; >> + goto out_free; >> + } >> + >> + domain = irq_domain_add_hierarchy(domain_parent, 0, >> + MSCM_IRSPRC_NUM, node, >> + &mscm_irq_domain_ops, chip_data); >> + if (!domain) { >> + ret = -ENOMEM; >> + goto out_unmap; >> + } >> + >> + chip_data->cpu_mask = 0x1 << >> + readl_relaxed(chip_data->mscm_base + MSCM_CPxNUM); > > That's a bit hard to read. Put it on the same line. > >> + >> + cpu_pm_register_notifier(&mscm_notifier_block); >> + >> + return 0; >> + >> +out_unmap: >> + iounmap(chip_data->mscm_base); >> +out_free: >> + kfree(chip_data); >> + return ret; >> +} >> +IRQCHIP_DECLARE(vf610_mscm, "fsl,vf610-mscm", vf610_mscm_of_init); >> > > Otherwise, looks pretty good to me. > The same line adjustment will break the 80 character border... But I agree, it's ugly the way it is now. Will put them in the same line. -- Stefan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
