| Message ID | 20170616141923.31226-5-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/06/17 15:19, Thomas Petazzoni wrote: > The Marvell ICU unit is found in the CP110 block of the Marvell Armada > 7K and 8K SoCs. It collects the wired interrupts of the devices located > in the CP110 and turns them into SPI interrupts in the GIC located in > the AP806 side of the SoC, by using a memory transaction. > > Until now, the ICU was configured in a static fashion by the firmware, > and Linux was relying on this static configuration. By having Linux > configure the ICU, we are more flexible, and we can allocate dynamically > the GIC SPI interrupts only for devices that are actually in use. > > The driver was initially written by Hanna Hawa <hannah@marvell.com>. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/irqchip/Kconfig | 3 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-mvebu-icu.c | 299 +++++++++++++++++++++ > .../dt-bindings/interrupt-controller/mvebu-icu.h | 15 ++ > 4 files changed, 318 insertions(+) > create mode 100644 drivers/irqchip/irq-mvebu-icu.c > create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index e527ee5..676232a 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -271,6 +271,9 @@ config IRQ_MXS > config MVEBU_GICP > bool > > +config MVEBU_ICU > + bool > + > config MVEBU_ODMI > bool > select GENERIC_MSI_IRQ_DOMAIN > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 11eb858..18fa5fa 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ) += irq-ingenic.o > obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o > obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o > obj-$(CONFIG_MVEBU_GICP) += irq-mvebu-gicp.o > +obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o > obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o > obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o > obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > new file mode 100644 > index 0000000..6b33998 > --- /dev/null > +++ b/drivers/irqchip/irq-mvebu-icu.c > @@ -0,0 +1,299 @@ > +/* > + * Copyright (C) 2017 Marvell > + * > + * Hanna Hawa <hannah@marvell.com> > + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqchip.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/msi.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +#include <dt-bindings/interrupt-controller/mvebu-icu.h> > + > +#include "irq-mvebu-gicp.h" > + > +/* ICU registers */ > +#define ICU_SETSPI_NSR_AL 0x10 > +#define ICU_SETSPI_NSR_AH 0x14 > +#define ICU_CLRSPI_NSR_AL 0x18 > +#define ICU_CLRSPI_NSR_AH 0x1c > +#define ICU_INT_CFG(x) (0x100 + 4 * (x)) > +#define ICU_INT_ENABLE BIT(24) > +#define ICU_IS_EDGE BIT(28) > +#define ICU_GROUP_SHIFT 29 > + > +/* ICU definitions */ > +#define ICU_MAX_IRQS 207 > +#define ICU_SATA0_ICU_ID 109 > +#define ICU_SATA1_ICU_ID 107 > + > +struct mvebu_icu { > + struct irq_chip irq_chip; > + void __iomem *base; > + struct irq_domain *domain; > + struct device *dev; > + struct mvebu_gicp *gicp; > +}; > + > +struct mvebu_icu_irq_data { > + struct mvebu_icu *icu; > + unsigned int icu_group; > + unsigned int type; > +}; > + > +static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > +{ > + struct irq_data *d = irq_get_irq_data(desc->irq); > + struct mvebu_icu_irq_data *icu_irqd = d->chip_data; > + struct mvebu_icu *icu = icu_irqd->icu; > + unsigned int icu_int; > + > + if (msg->address_lo) { This should probably test both _lo and _hi. > + /* Configure the ICU with irq number & type */ > + icu_int = msg->data | ICU_INT_ENABLE; > + if (icu_irqd->type & IRQ_TYPE_EDGE_RISING) > + icu_int |= ICU_IS_EDGE; > + icu_int |= icu_irqd->icu_group << ICU_GROUP_SHIFT; > + } else { > + /* De-configure the ICU */ > + icu_int = 0; > + } > + > + writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); > + > + /* > + * The SATA unit has 2 ports, and a dedicated ICU entry per > + * port. The ahci sata driver supports only one irq interrupt > + * per SATA unit. To solve this conflict, we configure the 2 > + * SATA wired interrupts in the south bridge into 1 GIC > + * interrupt in the north bridge. Even if only a single port > + * is enabled, if sata node is enabled, both interrupts are > + * configured (regardless of which port is actually in use). > + */ > + if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) { > + writel_relaxed(icu_int, > + icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID)); > + writel_relaxed(icu_int, > + icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID)); > + } > +} > + > +static int > +mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > + unsigned long *hwirq, unsigned int *type) > +{ > + struct mvebu_icu *icu = d->host_data; > + unsigned int icu_group; > + > + /* Check the count of the parameters in dt */ > + if (WARN_ON(fwspec->param_count < 3)) { > + dev_err(icu->dev, "wrong ICU parameter count %d\n", > + fwspec->param_count); > + return -EINVAL; > + } > + > + /* Only ICU group type is handled */ > + icu_group = fwspec->param[0]; > + if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR && > + icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) { > + dev_err(icu->dev, "wrong ICU group type %x\n", icu_group); > + return -EINVAL; > + } > + > + *hwirq = fwspec->param[1]; > + if (*hwirq < 0 || *hwirq >= ICU_MAX_IRQS) { *hwirq is unlikely to become negative... > + dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq); > + return -EINVAL; > + } > + > + /* Mask the type to prevent wrong DT configuration */ > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > + > + return 0; > +} > + > +static int > +mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs, void *args) > +{ > + int err; > + unsigned long hwirq; > + struct irq_fwspec *fwspec = args; > + struct mvebu_icu *icu = platform_msi_get_host_data(domain); > + struct mvebu_icu_irq_data *icu_irqd; > + > + icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); > + if (!icu_irqd) > + return -ENOMEM; > + > + err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq, > + &icu_irqd->type); > + if (err) { > + dev_err(icu->dev, "failed to translate ICU parameters\n"); > + goto free_irqd; > + } > + > + icu_irqd->icu_group = fwspec->param[0]; > + icu_irqd->icu = icu; > + > + err = platform_msi_domain_alloc(domain, virq, nr_irqs); > + if (err) { > + dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n"); > + goto free_irqd; > + } > + > + /* Make sure there is no interrupt left pending by the firmware */ > + err = irq_set_irqchip_state(virq, IRQCHIP_STATE_PENDING, false); > + if (err) > + goto free_msi; > + > + err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, > + &icu->irq_chip, icu_irqd); > + if (err) { > + dev_err(icu->dev, "failed to set the data to IRQ domain\n"); > + goto free_msi; > + } I think you may want to issue a irq_set_type here, because it is not completely clear to me if the core code will be doing it by default for you... > + > + return 0; > + > +free_msi: > + platform_msi_domain_free(domain, virq, nr_irqs); > +free_irqd: > + kfree(icu_irqd); > + return err; > +} > + > +static void > +mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs) > +{ > + struct irq_data *d = irq_get_irq_data(virq); > + struct mvebu_icu_irq_data *icu_irqd = d->chip_data; > + > + kfree(icu_irqd); > + > + platform_msi_domain_free(domain, virq, nr_irqs); > +} > + > +static const struct irq_domain_ops mvebu_icu_domain_ops = { > + .translate = mvebu_icu_irq_domain_translate, > + .alloc = mvebu_icu_irq_domain_alloc, > + .free = mvebu_icu_irq_domain_free, > +}; > + > +static int mvebu_icu_probe(struct platform_device *pdev) > +{ > + struct mvebu_icu *icu; > + struct device_node *node = pdev->dev.of_node; > + struct platform_device *gicp_pdev; > + struct device_node *gicp_dn; > + struct resource *res; > + phys_addr_t setspi, clrspi; > + u32 i, icu_int; > + > + icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu), > + GFP_KERNEL); > + if (!icu) > + return -ENOMEM; > + > + icu->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + icu->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(icu->base)) { > + dev_err(&pdev->dev, "Failed to map icu base address.\n"); > + return PTR_ERR(icu->base); > + } > + > + icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "ICU.%x", > + (unsigned int)res->start); > + if (!icu->irq_chip.name) > + return -ENOMEM; > + > + icu->irq_chip.irq_mask = irq_chip_mask_parent; > + icu->irq_chip.irq_unmask = irq_chip_unmask_parent; > + icu->irq_chip.irq_eoi = irq_chip_eoi_parent; > + icu->irq_chip.irq_set_type = irq_chip_set_type_parent; > +#ifdef CONFIG_SMP > + icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent; > +#endif > + > + /* > + * We're probed after MSI domains have been resolved, so force > + * resolution here. > + */ > + pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, node, > + DOMAIN_BUS_PLATFORM_MSI); > + if (!pdev->dev.msi_domain) > + return -EPROBE_DEFER; > + > + gicp_dn = of_parse_phandle(node, "msi-parent", 0); > + if (!gicp_dn) { > + dev_err(&pdev->dev, "Missing marvell,gicp property.\n"); > + return -ENODEV; > + } > + > + gicp_pdev = of_find_device_by_node(gicp_dn); > + if (!gicp_pdev) { > + dev_err(&pdev->dev, "Cannot find gicp device.\n"); > + return -ENODEV; > + } > + > + icu->gicp = platform_get_drvdata(gicp_pdev); > + > + /* Set Clear/Set ICU SPI message address in AP */ > + setspi = mvebu_gicp_setspi_phys_addr(icu->gicp); I must say that I find this quite horrible. The idea of digging into the internals of another driver and forcing it to blindly dereference a pointer feels just wrong. Instead, why don't you directly pass the device node, and kindly ask the GICP driver to give you the two addresses? Something along the lines of: err = mvebu_gicp_get_doorbells(gicp_dn, &setspi, &clrspi); if (err) [...] which at least gives a the GICP driver chance to check that this is something it knows about. And you can then drop the icu->gicp field. > + writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH); > + writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL); > + clrspi = mvebu_gicp_clrspi_phys_addr(icu->gicp); > + writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH); > + writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL); > + > + /* > + * Clean all ICU interrupts with type SPI_NSR, required to > + * avoid unpredictable SPI assignments done by firmware. > + */ > + for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > + icu_int = readl(icu->base + ICU_INT_CFG(i)); > + if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR) > + writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); > + } I had questions about the safety of this in a previous review. Do you have any update? Also, shouldn't you check that same thing in the translate callback (so that you detect clashes between firmware and DT)? > + > + icu->domain = > + platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS, > + mvebu_icu_write_msg, > + &mvebu_icu_domain_ops, icu); > + if (!icu->domain) { > + dev_err(&pdev->dev, "Failed to create ICU domain\n"); > + put_device(&gicp_pdev->dev); > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static const struct of_device_id mvebu_icu_of_match[] = { > + { .compatible = "marvell,cp110-icu", }, > + {}, > +}; > + > +static struct platform_driver mvebu_icu_driver = { > + .probe = mvebu_icu_probe, > + .driver = { > + .name = "mvebu-icu", > + .of_match_table = mvebu_icu_of_match, > + }, > +}; > +builtin_platform_driver(mvebu_icu_driver); > diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h > new file mode 100644 > index 0000000..8249558 > --- /dev/null > +++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h > @@ -0,0 +1,15 @@ > +/* > + * This header provides constants for the MVEBU ICU driver. > + */ > + > +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H > +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H > + > +/* interrupt specifier cell 0 */ > + > +#define ICU_GRP_NSR 0x0 > +#define ICU_GRP_SR 0x1 > +#define ICU_GRP_SEI 0x4 > +#define ICU_GRP_REI 0x5 > + > +#endif > Otherwise looking pretty neat. Thanks, M.
Hello, On Mon, 19 Jun 2017 18:40:53 +0100, Marc Zyngier wrote: > > + if (msg->address_lo) { > > This should probably test both _lo and _hi. Not sure what test you want to do on _hi. Since the physical address I'm using is below the 4 GB boundary, the high bits are all zeroes, even for a valid address. So to distinguish whether we're configuring or de-configuring the MSI, I don't see how the address_hi value is useful. Am I missing something obvious here? > > + *hwirq = fwspec->param[1]; > > + if (*hwirq < 0 || *hwirq >= ICU_MAX_IRQS) { > > *hwirq is unlikely to become negative... Fixed. Weird that gcc didn't complain here. hwirq is a unsigned long*, so I would have expected gcc to complain when looking at *hwirq < 0. Anyway, fixed, will be in v4. > > + err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, > > + &icu->irq_chip, icu_irqd); > > + if (err) { > > + dev_err(icu->dev, "failed to set the data to IRQ domain\n"); > > + goto free_msi; > > + } > > I think you may want to issue a irq_set_type here, because it is not > completely clear to me if the core code will be doing it by default for > you... It's not needed I believe. I've added some trace in gic_set_type(), and it's really called for every ICU interrupt as expected, as soon as the interrupt is configured. And indeed, if you look at __setup_irq(), it calls __irq_set_trigger(), see http://elixir.free-electrons.com/linux/latest/source/kernel/irq/manage.c#L1309. I've added a dump_stack() in git_set_type() to make sure when I was getting called for the SPI interrupts corresponding to the GICP/ICU stuff. Here is one example, from the XHCI driver: [ 1.815712] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc1 #613 [ 1.822180] Hardware name: Marvell Armada 8040 DB board (DT) [ 1.827863] Call trace: [ 1.830329] [<ffff000008088528>] dump_backtrace+0x0/0x228 [ 1.835752] [<ffff00000808881c>] show_stack+0x14/0x20 [ 1.840828] [<ffff00000838fd80>] dump_stack+0x90/0xb0 [ 1.845903] [<ffff0000083bf13c>] gic_set_type+0x94/0x98 [ 1.851154] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 [ 1.857449] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 [ 1.863743] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 [ 1.870037] [<ffff00000810d0a0>] __irq_set_trigger+0x60/0x178 [ 1.875808] [<ffff00000810d764>] __setup_irq+0x5ac/0x690 [ 1.881143] [<ffff00000810da1c>] request_threaded_irq+0xec/0x1c0 [ 1.887177] [<ffff0000086a84dc>] usb_add_hcd+0x50c/0x800 [ 1.892513] [<ffff0000087052ec>] xhci_plat_probe+0x584/0x768 [ 1.898199] [<ffff00000854cc28>] platform_drv_probe+0x58/0xc0 [ 1.903969] [<ffff00000854ad74>] driver_probe_device+0x214/0x2d0 [ 1.910002] [<ffff00000854aedc>] __driver_attach+0xac/0xb0 [ 1.915511] [<ffff000008548ef8>] bus_for_each_dev+0x60/0xa0 [ 1.921107] [<ffff00000854a690>] driver_attach+0x20/0x28 [ 1.926442] [<ffff00000854a1e0>] bus_add_driver+0x110/0x230 [ 1.932038] [<ffff00000854b858>] driver_register+0x60/0xf8 [ 1.937547] [<ffff00000854cb5c>] __platform_driver_register+0x44/0x50 [ 1.944019] [<ffff000008d41a60>] xhci_plat_init+0x2c/0x34 [ 1.949441] [<ffff0000080830f8>] do_one_initcall+0x38/0x120 [ 1.955038] [<ffff000008d00ce8>] kernel_init_freeable+0x198/0x238 [ 1.961159] [<ffff0000088fe470>] kernel_init+0x10/0x100 [ 1.966406] [<ffff000008082ec0>] ret_from_fork+0x10/0x50 So, whenever you do the request_irq(), __setup_irq() calls __irq_set_trigger(), which ends in the ICU ->irq_set_type(), calling the GICP MSI domain ->irq_set_type(), calling the GICP inner domain ->irq_set_type(), itself calling the GIC ->irq_set_type(). > > + icu->gicp = platform_get_drvdata(gicp_pdev); > > + > > + /* Set Clear/Set ICU SPI message address in AP */ > > + setspi = mvebu_gicp_setspi_phys_addr(icu->gicp); > > > I must say that I find this quite horrible. The idea of digging into the > internals of another driver and forcing it to blindly dereference a > pointer feels just wrong. > > Instead, why don't you directly pass the device node, and kindly ask the > GICP driver to give you the two addresses? Something along the lines of: > > err = mvebu_gicp_get_doorbells(gicp_dn, &setspi, &clrspi); > if (err) > [...] > > which at least gives a the GICP driver chance to check that this is > something it knows about. And you can then drop the icu->gicp field. ACK, fixed for the next version. > > + /* > > + * Clean all ICU interrupts with type SPI_NSR, required to > > + * avoid unpredictable SPI assignments done by firmware. > > + */ > > + for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > > + icu_int = readl(icu->base + ICU_INT_CFG(i)); > > + if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR) > > + writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); > > + } > > I had questions about the safety of this in a previous review. Do you > have any update? Also, shouldn't you check that same thing in the > translate callback (so that you detect clashes between firmware and DT)? I'm still waiting for feedback from Hannah and Yehuda in Cc on this question. They should answer soon, hopefully. > Otherwise looking pretty neat. Thanks again for the review. You can expect v4 today. Best regards, Thomas
On 20/06/17 14:46, Thomas Petazzoni wrote: > Hello, > > On Mon, 19 Jun 2017 18:40:53 +0100, Marc Zyngier wrote: > >>> + if (msg->address_lo) { >> >> This should probably test both _lo and _hi. > > Not sure what test you want to do on _hi. Since the physical address > I'm using is below the 4 GB boundary, the high bits are all zeroes, > even for a valid address. So to distinguish whether we're configuring > or de-configuring the MSI, I don't see how the address_hi value is > useful. > > Am I missing something obvious here? There is a few things: this driver could (mostly) work with a GICv3 distributor (located way above 4GB) instead of the GICP, and I'd rather make no assumption of where GICP is located in the memory map. So I'd rather see: if (msg->address_lo || msg->address_hi) { [...] } else { /* deconfiguration case */ } [...] >> I think you may want to issue a irq_set_type here, because it is not >> completely clear to me if the core code will be doing it by default for >> you... > > It's not needed I believe. I've added some trace in gic_set_type(), and > it's really called for every ICU interrupt as expected, as soon as the > interrupt is configured. And indeed, if you look at __setup_irq(), it > calls __irq_set_trigger(), see > http://elixir.free-electrons.com/linux/latest/source/kernel/irq/manage.c#L1309. > I've added a dump_stack() in git_set_type() to make sure when I was > getting called for the SPI interrupts corresponding to the GICP/ICU > stuff. Here is one example, from the XHCI driver: > > [ 1.815712] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc1 #613 > [ 1.822180] Hardware name: Marvell Armada 8040 DB board (DT) > [ 1.827863] Call trace: > [ 1.830329] [<ffff000008088528>] dump_backtrace+0x0/0x228 > [ 1.835752] [<ffff00000808881c>] show_stack+0x14/0x20 > [ 1.840828] [<ffff00000838fd80>] dump_stack+0x90/0xb0 > [ 1.845903] [<ffff0000083bf13c>] gic_set_type+0x94/0x98 > [ 1.851154] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 > [ 1.857449] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 > [ 1.863743] [<ffff00000810e734>] irq_chip_set_type_parent+0x1c/0x30 > [ 1.870037] [<ffff00000810d0a0>] __irq_set_trigger+0x60/0x178 > [ 1.875808] [<ffff00000810d764>] __setup_irq+0x5ac/0x690 > [ 1.881143] [<ffff00000810da1c>] request_threaded_irq+0xec/0x1c0 > [ 1.887177] [<ffff0000086a84dc>] usb_add_hcd+0x50c/0x800 > [ 1.892513] [<ffff0000087052ec>] xhci_plat_probe+0x584/0x768 > [ 1.898199] [<ffff00000854cc28>] platform_drv_probe+0x58/0xc0 > [ 1.903969] [<ffff00000854ad74>] driver_probe_device+0x214/0x2d0 > [ 1.910002] [<ffff00000854aedc>] __driver_attach+0xac/0xb0 > [ 1.915511] [<ffff000008548ef8>] bus_for_each_dev+0x60/0xa0 > [ 1.921107] [<ffff00000854a690>] driver_attach+0x20/0x28 > [ 1.926442] [<ffff00000854a1e0>] bus_add_driver+0x110/0x230 > [ 1.932038] [<ffff00000854b858>] driver_register+0x60/0xf8 > [ 1.937547] [<ffff00000854cb5c>] __platform_driver_register+0x44/0x50 > [ 1.944019] [<ffff000008d41a60>] xhci_plat_init+0x2c/0x34 > [ 1.949441] [<ffff0000080830f8>] do_one_initcall+0x38/0x120 > [ 1.955038] [<ffff000008d00ce8>] kernel_init_freeable+0x198/0x238 > [ 1.961159] [<ffff0000088fe470>] kernel_init+0x10/0x100 > [ 1.966406] [<ffff000008082ec0>] ret_from_fork+0x10/0x50 > > So, whenever you do the request_irq(), __setup_irq() calls > __irq_set_trigger(), which ends in the ICU ->irq_set_type(), calling > the GICP MSI domain ->irq_set_type(), calling the GICP inner domain > ->irq_set_type(), itself calling the GIC ->irq_set_type(). Fair enough. > >>> + icu->gicp = platform_get_drvdata(gicp_pdev); >>> + >>> + /* Set Clear/Set ICU SPI message address in AP */ >>> + setspi = mvebu_gicp_setspi_phys_addr(icu->gicp); >> >> >> I must say that I find this quite horrible. The idea of digging into the >> internals of another driver and forcing it to blindly dereference a >> pointer feels just wrong. >> >> Instead, why don't you directly pass the device node, and kindly ask the >> GICP driver to give you the two addresses? Something along the lines of: >> >> err = mvebu_gicp_get_doorbells(gicp_dn, &setspi, &clrspi); >> if (err) >> [...] >> >> which at least gives a the GICP driver chance to check that this is >> something it knows about. And you can then drop the icu->gicp field. > > ACK, fixed for the next version. > >>> + /* >>> + * Clean all ICU interrupts with type SPI_NSR, required to >>> + * avoid unpredictable SPI assignments done by firmware. >>> + */ >>> + for (i = 0 ; i < ICU_MAX_IRQS ; i++) { >>> + icu_int = readl(icu->base + ICU_INT_CFG(i)); >>> + if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR) >>> + writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); >>> + } >> >> I had questions about the safety of this in a previous review. Do you >> have any update? Also, shouldn't you check that same thing in the >> translate callback (so that you detect clashes between firmware and DT)? > > I'm still waiting for feedback from Hannah and Yehuda in Cc on this > question. They should answer soon, hopefully. > >> Otherwise looking pretty neat. > > Thanks again for the review. You can expect v4 today. OK, thanks. M.
Hello, On Tue, 20 Jun 2017 15:00:19 +0100, Marc Zyngier wrote: > There is a few things: this driver could (mostly) work with a GICv3 > distributor (located way above 4GB) instead of the GICP, and I'd rather > make no assumption of where GICP is located in the memory map. > > So I'd rather see: > > if (msg->address_lo || msg->address_hi) { > [...] > } else { > /* deconfiguration case */ > } Ha, OK, indeed. /me feels stupid now. Unfortunately, I just sent v4 a few minutes ago. I'll wait for you to look at this v4. If there's nothing else to be changed, then I'll send a v5 with just this change. Thanks! Thomas
On 20/06/17 15:09, Thomas Petazzoni wrote: > Hello, > > On Tue, 20 Jun 2017 15:00:19 +0100, Marc Zyngier wrote: > >> There is a few things: this driver could (mostly) work with a GICv3 >> distributor (located way above 4GB) instead of the GICP, and I'd rather >> make no assumption of where GICP is located in the memory map. >> >> So I'd rather see: >> >> if (msg->address_lo || msg->address_hi) { >> [...] >> } else { >> /* deconfiguration case */ >> } > > Ha, OK, indeed. /me feels stupid now. > > Unfortunately, I just sent v4 a few minutes ago. I'll wait for you to > look at this v4. If there's nothing else to be changed, then I'll send > a v5 with just this change. If that's the only thing, I can apply the fixlet locally. Thanks, M.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index e527ee5..676232a 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -271,6 +271,9 @@ config IRQ_MXS config MVEBU_GICP bool +config MVEBU_ICU + bool + config MVEBU_ODMI bool select GENERIC_MSI_IRQ_DOMAIN diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 11eb858..18fa5fa 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ) += irq-ingenic.o obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o obj-$(CONFIG_MVEBU_GICP) += irq-mvebu-gicp.o +obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c new file mode 100644 index 0000000..6b33998 --- /dev/null +++ b/drivers/irqchip/irq-mvebu-icu.c @@ -0,0 +1,299 @@ +/* + * Copyright (C) 2017 Marvell + * + * Hanna Hawa <hannah@marvell.com> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/msi.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#include <dt-bindings/interrupt-controller/mvebu-icu.h> + +#include "irq-mvebu-gicp.h" + +/* ICU registers */ +#define ICU_SETSPI_NSR_AL 0x10 +#define ICU_SETSPI_NSR_AH 0x14 +#define ICU_CLRSPI_NSR_AL 0x18 +#define ICU_CLRSPI_NSR_AH 0x1c +#define ICU_INT_CFG(x) (0x100 + 4 * (x)) +#define ICU_INT_ENABLE BIT(24) +#define ICU_IS_EDGE BIT(28) +#define ICU_GROUP_SHIFT 29 + +/* ICU definitions */ +#define ICU_MAX_IRQS 207 +#define ICU_SATA0_ICU_ID 109 +#define ICU_SATA1_ICU_ID 107 + +struct mvebu_icu { + struct irq_chip irq_chip; + void __iomem *base; + struct irq_domain *domain; + struct device *dev; + struct mvebu_gicp *gicp; +}; + +struct mvebu_icu_irq_data { + struct mvebu_icu *icu; + unsigned int icu_group; + unsigned int type; +}; + +static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) +{ + struct irq_data *d = irq_get_irq_data(desc->irq); + struct mvebu_icu_irq_data *icu_irqd = d->chip_data; + struct mvebu_icu *icu = icu_irqd->icu; + unsigned int icu_int; + + if (msg->address_lo) { + /* Configure the ICU with irq number & type */ + icu_int = msg->data | ICU_INT_ENABLE; + if (icu_irqd->type & IRQ_TYPE_EDGE_RISING) + icu_int |= ICU_IS_EDGE; + icu_int |= icu_irqd->icu_group << ICU_GROUP_SHIFT; + } else { + /* De-configure the ICU */ + icu_int = 0; + } + + writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq)); + + /* + * The SATA unit has 2 ports, and a dedicated ICU entry per + * port. The ahci sata driver supports only one irq interrupt + * per SATA unit. To solve this conflict, we configure the 2 + * SATA wired interrupts in the south bridge into 1 GIC + * interrupt in the north bridge. Even if only a single port + * is enabled, if sata node is enabled, both interrupts are + * configured (regardless of which port is actually in use). + */ + if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) { + writel_relaxed(icu_int, + icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID)); + writel_relaxed(icu_int, + icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID)); + } +} + +static int +mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, + unsigned long *hwirq, unsigned int *type) +{ + struct mvebu_icu *icu = d->host_data; + unsigned int icu_group; + + /* Check the count of the parameters in dt */ + if (WARN_ON(fwspec->param_count < 3)) { + dev_err(icu->dev, "wrong ICU parameter count %d\n", + fwspec->param_count); + return -EINVAL; + } + + /* Only ICU group type is handled */ + icu_group = fwspec->param[0]; + if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR && + icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) { + dev_err(icu->dev, "wrong ICU group type %x\n", icu_group); + return -EINVAL; + } + + *hwirq = fwspec->param[1]; + if (*hwirq < 0 || *hwirq >= ICU_MAX_IRQS) { + dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq); + return -EINVAL; + } + + /* Mask the type to prevent wrong DT configuration */ + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; + + return 0; +} + +static int +mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *args) +{ + int err; + unsigned long hwirq; + struct irq_fwspec *fwspec = args; + struct mvebu_icu *icu = platform_msi_get_host_data(domain); + struct mvebu_icu_irq_data *icu_irqd; + + icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); + if (!icu_irqd) + return -ENOMEM; + + err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq, + &icu_irqd->type); + if (err) { + dev_err(icu->dev, "failed to translate ICU parameters\n"); + goto free_irqd; + } + + icu_irqd->icu_group = fwspec->param[0]; + icu_irqd->icu = icu; + + err = platform_msi_domain_alloc(domain, virq, nr_irqs); + if (err) { + dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n"); + goto free_irqd; + } + + /* Make sure there is no interrupt left pending by the firmware */ + err = irq_set_irqchip_state(virq, IRQCHIP_STATE_PENDING, false); + if (err) + goto free_msi; + + err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, + &icu->irq_chip, icu_irqd); + if (err) { + dev_err(icu->dev, "failed to set the data to IRQ domain\n"); + goto free_msi; + } + + return 0; + +free_msi: + platform_msi_domain_free(domain, virq, nr_irqs); +free_irqd: + kfree(icu_irqd); + return err; +} + +static void +mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs) +{ + struct irq_data *d = irq_get_irq_data(virq); + struct mvebu_icu_irq_data *icu_irqd = d->chip_data; + + kfree(icu_irqd); + + platform_msi_domain_free(domain, virq, nr_irqs); +} + +static const struct irq_domain_ops mvebu_icu_domain_ops = { + .translate = mvebu_icu_irq_domain_translate, + .alloc = mvebu_icu_irq_domain_alloc, + .free = mvebu_icu_irq_domain_free, +}; + +static int mvebu_icu_probe(struct platform_device *pdev) +{ + struct mvebu_icu *icu; + struct device_node *node = pdev->dev.of_node; + struct platform_device *gicp_pdev; + struct device_node *gicp_dn; + struct resource *res; + phys_addr_t setspi, clrspi; + u32 i, icu_int; + + icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu), + GFP_KERNEL); + if (!icu) + return -ENOMEM; + + icu->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + icu->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(icu->base)) { + dev_err(&pdev->dev, "Failed to map icu base address.\n"); + return PTR_ERR(icu->base); + } + + icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, + "ICU.%x", + (unsigned int)res->start); + if (!icu->irq_chip.name) + return -ENOMEM; + + icu->irq_chip.irq_mask = irq_chip_mask_parent; + icu->irq_chip.irq_unmask = irq_chip_unmask_parent; + icu->irq_chip.irq_eoi = irq_chip_eoi_parent; + icu->irq_chip.irq_set_type = irq_chip_set_type_parent; +#ifdef CONFIG_SMP + icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent; +#endif + + /* + * We're probed after MSI domains have been resolved, so force + * resolution here. + */ + pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, node, + DOMAIN_BUS_PLATFORM_MSI); + if (!pdev->dev.msi_domain) + return -EPROBE_DEFER; + + gicp_dn = of_parse_phandle(node, "msi-parent", 0); + if (!gicp_dn) { + dev_err(&pdev->dev, "Missing marvell,gicp property.\n"); + return -ENODEV; + } + + gicp_pdev = of_find_device_by_node(gicp_dn); + if (!gicp_pdev) { + dev_err(&pdev->dev, "Cannot find gicp device.\n"); + return -ENODEV; + } + + icu->gicp = platform_get_drvdata(gicp_pdev); + + /* Set Clear/Set ICU SPI message address in AP */ + setspi = mvebu_gicp_setspi_phys_addr(icu->gicp); + writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH); + writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL); + clrspi = mvebu_gicp_clrspi_phys_addr(icu->gicp); + writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH); + writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL); + + /* + * Clean all ICU interrupts with type SPI_NSR, required to + * avoid unpredictable SPI assignments done by firmware. + */ + for (i = 0 ; i < ICU_MAX_IRQS ; i++) { + icu_int = readl(icu->base + ICU_INT_CFG(i)); + if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR) + writel_relaxed(0x0, icu->base + ICU_INT_CFG(i)); + } + + icu->domain = + platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS, + mvebu_icu_write_msg, + &mvebu_icu_domain_ops, icu); + if (!icu->domain) { + dev_err(&pdev->dev, "Failed to create ICU domain\n"); + put_device(&gicp_pdev->dev); + return -ENOMEM; + } + + return 0; +} + +static const struct of_device_id mvebu_icu_of_match[] = { + { .compatible = "marvell,cp110-icu", }, + {}, +}; + +static struct platform_driver mvebu_icu_driver = { + .probe = mvebu_icu_probe, + .driver = { + .name = "mvebu-icu", + .of_match_table = mvebu_icu_of_match, + }, +}; +builtin_platform_driver(mvebu_icu_driver); diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h new file mode 100644 index 0000000..8249558 --- /dev/null +++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h @@ -0,0 +1,15 @@ +/* + * This header provides constants for the MVEBU ICU driver. + */ + +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H + +/* interrupt specifier cell 0 */ + +#define ICU_GRP_NSR 0x0 +#define ICU_GRP_SR 0x1 +#define ICU_GRP_SEI 0x4 +#define ICU_GRP_REI 0x5 + +#endif
The Marvell ICU unit is found in the CP110 block of the Marvell Armada 7K and 8K SoCs. It collects the wired interrupts of the devices located in the CP110 and turns them into SPI interrupts in the GIC located in the AP806 side of the SoC, by using a memory transaction. Until now, the ICU was configured in a static fashion by the firmware, and Linux was relying on this static configuration. By having Linux configure the ICU, we are more flexible, and we can allocate dynamically the GIC SPI interrupts only for devices that are actually in use. The driver was initially written by Hanna Hawa <hannah@marvell.com>. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/irqchip/Kconfig | 3 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-mvebu-icu.c | 299 +++++++++++++++++++++ .../dt-bindings/interrupt-controller/mvebu-icu.h | 15 ++ 4 files changed, 318 insertions(+) create mode 100644 drivers/irqchip/irq-mvebu-icu.c create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h