| Message ID | 20180522094042.24770-1-miquel.raynal@bootlin.com |
|---|---|
| Headers | show |
| Series | Add System Error Interrupt support to Armada SoCs | expand |
Hi Miquel, On mar., mai 22 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ICU size in CP110 is not 0x10 but at least 0x440 bytes long (from the > specification). > > Fixes: 6ef84a827c37 ("arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K") > Cc: stable@vger.kernel.org > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Applied on mvebu/fixes Thanks, Gregory > --- > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > index 48cad7919efa..9fa41c54f69c 100644 > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi > @@ -146,7 +146,7 @@ > > CP110_LABEL(icu): interrupt-controller@1e0000 { > compatible = "marvell,cp110-icu"; > - reg = <0x1e0000 0x10>; > + reg = <0x1e0000 0x440>; > #interrupt-cells = <3>; > interrupt-controller; > msi-parent = <&gicp>; > -- > 2.14.1 >
On 22/05/18 10:40, Miquel Raynal wrote: > This is a cascaded interrupt controller in the AP806 GIC that collapses > SEIs (System Error Interrupt) coming from the AP and the CPs (through > the ICU). > > The SEI handles up to 64 interrupts. The first 21 interrupts are wired > and come from the AP. The next 43 interrupts are from the CPs and are wired to the AP (no interrupts come from it) > triggered through MSI messages. To handle this complexity, the driver > has to declare to the upper layer: one IRQ domain for the wired > interrupts, one IRQ domain for the MSIs; and acts as a MSI server s/server/controller/ > ('parent') by declaring an MSI domain. > > Suggested-by: Haim Boot <hayim@marvell.com> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/irqchip/Kconfig | 3 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-mvebu-sei.c | 422 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 426 insertions(+) > create mode 100644 drivers/irqchip/irq-mvebu-sei.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index e9233db16e03..922e2a919cf3 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -310,6 +310,9 @@ config MVEBU_ODMI > config MVEBU_PIC > bool > > +config MVEBU_SEI > + bool > + > config LS_SCFG_MSI > def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE > depends on PCI && PCI_MSI > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 15f268f646bf..69d2ccb454ef 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -76,6 +76,7 @@ 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_MVEBU_SEI) += irq-mvebu-sei.o > obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o > obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o > obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o > diff --git a/drivers/irqchip/irq-mvebu-sei.c b/drivers/irqchip/irq-mvebu-sei.c > new file mode 100644 > index 000000000000..d9abd5e10741 > --- /dev/null > +++ b/drivers/irqchip/irq-mvebu-sei.c > @@ -0,0 +1,422 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#define pr_fmt(fmt) "mvebu-sei: " fmt > + > +#include <linux/irq.h> > +#include <linux/interrupt.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/msi.h> > +#include <linux/platform_device.h> > +#include <linux/irqchip.h> > + > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +/* Cause register */ > +#define GICP_SECR(idx) (0x0 + (idx * 0x4)) > +/* Mask register */ > +#define GICP_SEMR(idx) (0x20 + (idx * 0x4)) > +#define GICP_SET_SEI_OFFSET 0x30 > + > +#define SEI_IRQ_COUNT_PER_REG 32 > +#define SEI_IRQ_REG_COUNT 2 > +#define SEI_IRQ_COUNT (SEI_IRQ_COUNT_PER_REG * SEI_IRQ_REG_COUNT) > +#define SEI_IRQ_REG_IDX(irq_id) (irq_id / SEI_IRQ_COUNT_PER_REG) > +#define SEI_IRQ_REG_BIT(irq_id) (irq_id % SEI_IRQ_COUNT_PER_REG) > + > +struct mvebu_sei_interrupt_range { > + u32 first; > + u32 number; > +}; > + > +struct mvebu_sei { > + struct device *dev; > + void __iomem *base; > + struct resource *res; > + struct irq_domain *ap_domain; > + struct irq_domain *cp_domain; > + struct mvebu_sei_interrupt_range ap_interrupts; > + struct mvebu_sei_interrupt_range cp_interrupts; > + /* Lock on MSI allocations/releases */ > + spinlock_t cp_msi_lock; > + DECLARE_BITMAP(cp_msi_bitmap, SEI_IRQ_COUNT); > +}; > + > +static int mvebu_sei_domain_to_sei_irq(struct mvebu_sei *sei, > + struct irq_domain *domain, > + irq_hw_number_t hwirq) > +{ > + if (domain == sei->ap_domain) > + return sei->ap_interrupts.first + hwirq; > + else > + return sei->cp_interrupts.first + hwirq; > +} > + > +static void mvebu_sei_reset(struct mvebu_sei *sei) > +{ > + u32 reg_idx; > + > + /* Clear IRQ cause registers */ > + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++) > + writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx)); > +} > + > +static void mvebu_sei_mask_irq(struct irq_data *d) > +{ > + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > + u32 reg_idx = SEI_IRQ_REG_IDX(sei_irq); > + u32 reg; > + > + /* 1 disables the interrupt */ > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > + reg |= BIT(SEI_IRQ_REG_BIT(sei_irq)); > + writel(reg, sei->base + GICP_SEMR(reg_idx)); > +} > + > +static void mvebu_sei_unmask_irq(struct irq_data *d) > +{ > + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > + u32 reg_idx = SEI_IRQ_REG_IDX(sei_irq); > + u32 reg; > + > + /* 0 enables the interrupt */ > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > + reg &= ~BIT(SEI_IRQ_REG_BIT(sei_irq)); > + writel(reg, sei->base + GICP_SEMR(reg_idx)); > +} > + > +static void mvebu_sei_compose_msi_msg(struct irq_data *data, > + struct msi_msg *msg) > +{ > + struct mvebu_sei *sei = data->chip_data; > + phys_addr_t set = sei->res->start + GICP_SET_SEI_OFFSET; > + > + msg->data = mvebu_sei_domain_to_sei_irq(sei, data->domain, data->hwirq); > + msg->address_lo = lower_32_bits(set); > + msg->address_hi = upper_32_bits(set); > +} > + > +static struct irq_chip mvebu_sei_ap_wired_irq_chip = { > + .name = "AP wired SEI", > + .irq_mask = mvebu_sei_mask_irq, > + .irq_unmask = mvebu_sei_unmask_irq, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = irq_chip_set_affinity_parent, > + .irq_set_type = irq_chip_set_type_parent, You seem to assume that this driver is purely dealing with edge interrupts. And yet you pass the request directly to the parrent. What does it mean? Shouldn't you at least check that this is an edge request and fail otherwise? > +}; > + > +static struct irq_chip mvebu_sei_cp_msi_irq_chip = { > + .name = "CP MSI SEI", > + .irq_mask = mvebu_sei_mask_irq, > + .irq_unmask = mvebu_sei_unmask_irq, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = irq_chip_set_affinity_parent, > + .irq_set_type = irq_chip_set_type_parent, Same here. > + .irq_compose_msi_msg = mvebu_sei_compose_msi_msg, > +}; > + > +static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, unsigned int nr_irqs, > + void *args) > +{ > + struct mvebu_sei *sei = domain->host_data; > + struct irq_fwspec *fwspec = args; > + struct irq_chip *irq_chip; > + int sei_hwirq, hwirq; > + int ret; > + > + /* Software only supports single allocations for now */ > + if (nr_irqs != 1) > + return -ENOTSUPP; > + > + if (domain == sei->ap_domain) { That's not great, really. Pushing the management of the two domains into the same callbacks is pretty ugly, and makes the code rather confusing. Is there anything really preventing the two domains from being managed independently? > + irq_chip = &mvebu_sei_ap_wired_irq_chip; > + hwirq = fwspec->param[0]; > + } else { > + irq_chip = &mvebu_sei_cp_msi_irq_chip; > + spin_lock(&sei->cp_msi_lock); This could as well be a mutex. > + hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, > + SEI_IRQ_COUNT, 0); It is a bit weird that you're allocating from a 64bit bitmap while you only have 43 interrupts available... At the 44th interrupt, something bad is going to happen. > + spin_unlock(&sei->cp_msi_lock); > + if (hwirq < 0) > + return -ENOSPC; > + } > + > + sei_hwirq = mvebu_sei_domain_to_sei_irq(sei, domain, hwirq); Splitting the callbacks would definitely avoid that kind of horror. > + > + fwspec->fwnode = domain->parent->fwnode; > + fwspec->param_count = 3; > + fwspec->param[0] = GIC_SPI; > + fwspec->param[1] = sei_hwirq; > + fwspec->param[2] = IRQ_TYPE_EDGE_RISING; > + > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, fwspec); > + if (ret) > + goto release_region; > + > + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, irq_chip, sei); > + if (ret) > + goto free_irq_parents; > + > + return 0; > + > +free_irq_parents: > + irq_domain_free_irqs_parent(domain, virq, nr_irqs); > +release_region: > + if (domain == sei->cp_domain) { > + spin_lock(&sei->cp_msi_lock); > + bitmap_release_region(sei->cp_msi_bitmap, hwirq, 0); > + spin_unlock(&sei->cp_msi_lock); > + } > + > + return ret; > +} > + > +static void mvebu_sei_irq_domain_free(struct irq_domain *domain, > + unsigned int virq, unsigned int nr_irqs) > +{ > + struct mvebu_sei *sei = domain->host_data; > + struct irq_data *d = irq_domain_get_irq_data(domain, virq); > + u32 irq_nb = sei->ap_interrupts.number + sei->cp_interrupts.number; > + > + if (nr_irqs != 1 || d->hwirq >= irq_nb) { > + dev_err(sei->dev, "Invalid hwirq %lu\n", d->hwirq); > + return; > + } > + > + irq_domain_free_irqs_parent(domain, virq, nr_irqs); > + > + spin_lock(&sei->cp_msi_lock); > + bitmap_release_region(sei->cp_msi_bitmap, d->hwirq, 0); > + spin_unlock(&sei->cp_msi_lock); > +} > + > +static const struct irq_domain_ops mvebu_sei_ap_domain_ops = { > + .xlate = irq_domain_xlate_onecell, > + .alloc = mvebu_sei_irq_domain_alloc, > + .free = mvebu_sei_irq_domain_free, > +}; > + > +static const struct irq_domain_ops mvebu_sei_cp_domain_ops = { > + .alloc = mvebu_sei_irq_domain_alloc, > + .free = mvebu_sei_irq_domain_free, > +}; > + > +static struct irq_chip mvebu_sei_msi_irq_chip = { > + .name = "SEI", > + .irq_set_type = irq_chip_set_type_parent, Same question here. > +}; > + > +static struct msi_domain_ops mvebu_sei_msi_ops = { > +}; > + > +static struct msi_domain_info mvebu_sei_msi_domain_info = { > + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS, > + .ops = &mvebu_sei_msi_ops, > + .chip = &mvebu_sei_msi_irq_chip, > +}; > + > +static void mvebu_sei_handle_cascade_irq(struct irq_desc *desc) > +{ > + struct mvebu_sei *sei = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned long irqmap, irq_bit; > + u32 reg_idx, virq, irqn; > + > + chained_irq_enter(chip, desc); > + > + /* Read both SEI cause registers (64 bits) */ > + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++) { > + irqmap = readl_relaxed(sei->base + GICP_SECR(reg_idx)); > + > + /* Call handler for each set bit */ > + for_each_set_bit(irq_bit, &irqmap, SEI_IRQ_COUNT_PER_REG) { > + /* Cause Register gives the SEI number */ > + irqn = irq_bit + reg_idx * SEI_IRQ_COUNT_PER_REG; > + /* > + * Finding Linux mapping (virq) needs the right domain > + * and the relative hwirq (which start at 0 in both > + * cases, while irqn is relative to all SEI interrupts). > + */ It is a bit odd that you're virtualizing the hwirq number. The whole point of splitting hwirq from virq is that you don't have to do that and can use the the raw HW number. You're saving a tiny bit of memory in the irq_domain, at the expense of more complexity. I don't know if that's worth it... > + if (irqn < sei->ap_interrupts.number) { > + virq = irq_find_mapping(sei->ap_domain, irqn); > + } else { > + irqn -= sei->ap_interrupts.number; > + virq = irq_find_mapping(sei->cp_domain, irqn); > + } > + > + /* Call IRQ handler */ > + generic_handle_irq(virq); > + } > + > + /* Clear interrupt indication by writing 1 to it */ > + writel(irqmap, sei->base + GICP_SECR(reg_idx)); > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static int mvebu_sei_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node, *parent, *child; > + struct irq_domain *parent_domain, *plat_domain; > + struct mvebu_sei *sei; > + const __be32 *property; > + u32 parent_irq, size; > + int ret; > + > + sei = devm_kzalloc(&pdev->dev, sizeof(*sei), GFP_KERNEL); > + if (!sei) > + return -ENOMEM; > + > + sei->dev = &pdev->dev; > + > + spin_lock_init(&sei->cp_msi_lock); > + > + sei->res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sei->base = devm_ioremap_resource(sei->dev, sei->res); > + if (!sei->base) { > + dev_err(sei->dev, "Failed to remap SEI resource\n"); > + return -ENODEV; > + } > + > + mvebu_sei_reset(sei); > + > + /* > + * Reserve the single (top-level) parent SPI IRQ from which all the > + * interrupts handled by this driver will be signaled. > + */ > + parent_irq = irq_of_parse_and_map(node, 0); > + if (parent_irq <= 0) { > + dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n"); > + return -ENODEV; > + } > + > + irq_set_chained_handler(parent_irq, mvebu_sei_handle_cascade_irq); > + irq_set_handler_data(parent_irq, sei); > + > + /* > + * SEIs in the range [ 0; 20] are wired and come from the AP. > + * SEIs in the range [21; 63] are CP SEI and are triggered through MSIs. > + * > + * Each SEI 'domain' is represented as a subnode. > + */ > + > + /* Get a reference to the parent domain to create a hierarchy */ > + parent = of_irq_find_parent(node); > + if (!parent) { > + dev_err(sei->dev, "Failed to find parent IRQ node\n"); > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + parent_domain = irq_find_host(parent); > + if (!parent_domain) { > + dev_err(sei->dev, "Failed to find parent IRQ domain\n"); > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + /* Create the 'wired' hierarchy */ > + child = of_find_node_by_name(node, "sei-wired-controller"); > + if (!child) { > + dev_err(sei->dev, "Missing 'sei-wired-controller' subnode\n"); > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + property = of_get_property(child, "marvell,sei-ranges", &size); > + if (!property || size != (2 * sizeof(u32))) { > + dev_err(sei->dev, "Missing 'marvell,sei-ranges' property\n"); > + of_node_put(child); > + ret = -ENODEV; > + goto dispose_irq; > + } > + > + sei->ap_interrupts.first = be32_to_cpu(property[0]); > + sei->ap_interrupts.number = be32_to_cpu(property[1]); > + sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0, > + sei->ap_interrupts.number, > + of_node_to_fwnode(child), > + &mvebu_sei_ap_domain_ops, > + sei); > + of_node_put(child); > + if (!sei->ap_domain) { > + dev_err(sei->dev, "Failed to create AP IRQ domain\n"); > + ret = -ENOMEM; > + goto dispose_irq; > + } > + > + /* Create the 'MSI' hierarchy */ > + child = of_find_node_by_name(node, "sei-msi-controller"); > + if (!child) { > + dev_err(sei->dev, "Missing 'sei-msi-controller' subnode\n"); > + ret = -ENODEV; > + goto remove_ap_domain; > + } > + > + property = of_get_property(child, "marvell,sei-ranges", &size); > + if (!property || size != (2 * sizeof(u32))) { > + dev_err(sei->dev, "Missing 'marvell,sei-ranges' property\n"); > + of_node_put(child); > + ret = -ENODEV; > + goto remove_ap_domain; > + } > + > + sei->cp_interrupts.first = be32_to_cpu(property[0]); > + sei->cp_interrupts.number = be32_to_cpu(property[1]); > + sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0, > + sei->cp_interrupts.number, > + of_node_to_fwnode(child), > + &mvebu_sei_cp_domain_ops, > + sei); > + if (!sei->cp_domain) { > + pr_err("Failed to create CPs IRQ domain\n"); > + of_node_put(child); > + ret = -ENOMEM; > + goto remove_ap_domain; > + } > + > + plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(child), > + &mvebu_sei_msi_domain_info, > + sei->cp_domain); > + of_node_put(child); > + if (!plat_domain) { > + pr_err("Failed to create CPs MSI domain\n"); > + ret = -ENOMEM; > + goto remove_cp_domain; > + } > + > + platform_set_drvdata(pdev, sei); > + > + return 0; > + > +remove_cp_domain: > + irq_domain_remove(sei->cp_domain); > +remove_ap_domain: > + irq_domain_remove(sei->ap_domain); > +dispose_irq: > + irq_dispose_mapping(parent_irq); > + > + return ret; > +} > + > +static const struct of_device_id mvebu_sei_of_match[] = { > + { .compatible = "marvell,armada-8k-sei", }, > + {}, > +}; > + > +static struct platform_driver mvebu_sei_driver = { > + .probe = mvebu_sei_probe, > + .driver = { > + .name = "mvebu-sei", > + .of_match_table = mvebu_sei_of_match, > + }, > +}; > +builtin_platform_driver(mvebu_sei_driver); > It feels like this patch could do with a total split: - Introduce the wired side of the driver - then the MSI part Drop the common domain callbacks, and treat the two domains separately. I seriously doubt there will be much of an overlap anyway. Thanks, M.
On 22/05/18 10:40, Miquel Raynal wrote: > An SEI driver provides an MSI domain through which it is possible to > raise SEIs. > > Handle the NSR probe function in a more generic way to support other > type of interrupts (ie. the SEIs). > > For clarity we do not use tree IRQ domains for now but linear ones > instead, allocating the 207 ICU lines for each interrupt group. What's the rational for not using trees? Because that's effectively a 100% overhead... > Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/irqchip/irq-mvebu-icu.c | 126 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 108 insertions(+), 18 deletions(-) > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > index 977e47b2716f..6ad6236d6ff1 100644 > --- a/drivers/irqchip/irq-mvebu-icu.c > +++ b/drivers/irqchip/irq-mvebu-icu.c > @@ -28,6 +28,10 @@ > #define ICU_SETSPI_NSR_AH 0x14 > #define ICU_CLRSPI_NSR_AL 0x18 > #define ICU_CLRSPI_NSR_AH 0x1c > +#define ICU_SET_SEI_AL 0x50 > +#define ICU_SET_SEI_AH 0x54 > +#define ICU_CLR_SEI_AL 0x58 > +#define ICU_CLR_SEI_AH 0x5C > #define ICU_INT_CFG(x) (0x100 + 4 * (x)) > #define ICU_INT_ENABLE BIT(24) > #define ICU_IS_EDGE BIT(28) > @@ -38,12 +42,28 @@ > #define ICU_SATA0_ICU_ID 109 > #define ICU_SATA1_ICU_ID 107 > > +struct mvebu_icu_subset_data { > + unsigned int icu_group; > + unsigned int offset_set_ah; > + unsigned int offset_set_al; > + unsigned int offset_clr_ah; > + unsigned int offset_clr_al; > +}; > + > struct mvebu_icu { > struct irq_chip irq_chip; > struct regmap *regmap; > struct device *dev; > - atomic_t initialized; > bool legacy_bindings; > + /* Lock on interrupt allocations/releases */ > + spinlock_t msi_lock; > + DECLARE_BITMAP(msi_bitmap, ICU_MAX_IRQS); > +}; > + > +struct mvebu_icu_msi_data { > + struct mvebu_icu *icu; > + atomic_t initialized; > + const struct mvebu_icu_subset_data *subset_data; > }; > > struct mvebu_icu_irq_data { > @@ -76,16 +96,25 @@ static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev) > return icu; > } > > -static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > +static void mvebu_icu_init(struct mvebu_icu *icu, struct irq_domain *d, > + struct msi_msg *msg) > { > - if (atomic_cmpxchg(&icu->initialized, false, true)) > + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d); > + const struct mvebu_icu_subset_data *subset = msi_data->subset_data; > + > + if (atomic_cmpxchg(&msi_data->initialized, false, true)) > + return; > + > + /* Set 'SET' ICU SPI message address in AP */ > + regmap_write(icu->regmap, subset->offset_set_ah, msg[0].address_hi); > + regmap_write(icu->regmap, subset->offset_set_al, msg[0].address_lo); > + > + if (subset->icu_group != ICU_GRP_NSR) > return; > > - /* Set Clear/Set ICU SPI message address in AP */ > - regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi); > - regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo); > - regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi); > - regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo); > + /* Set 'CLEAR' ICU SPI message address in AP (level-MSI only) */ > + regmap_write(icu->regmap, subset->offset_clr_ah, msg[1].address_hi); > + regmap_write(icu->regmap, subset->offset_clr_al, msg[1].address_lo); > } > > static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > @@ -96,8 +125,8 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > unsigned int icu_int; > > if (msg->address_lo || msg->address_hi) { > - /* One off initialization */ > - mvebu_icu_init(icu, msg); > + /* One off initialization per domain */ > + mvebu_icu_init(icu, d->domain, msg); > /* Configure the ICU with irq number & type */ > icu_int = msg->data | ICU_INT_ENABLE; > if (icu_irqd->type & IRQ_TYPE_EDGE_RISING) > @@ -131,7 +160,8 @@ 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 = platform_msi_get_host_data(d); > + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d); > + struct mvebu_icu *icu = msi_data->icu; > unsigned int param_count = icu->legacy_bindings ? 3 : 2; > > /* Check the count of the parameters in dt */ > @@ -172,7 +202,9 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > int err; > unsigned long hwirq; > struct irq_fwspec *fwspec = args; > - struct mvebu_icu *icu = platform_msi_get_host_data(domain); > + struct mvebu_icu_msi_data *msi_data = > + platform_msi_get_host_data(domain); > + struct mvebu_icu *icu = msi_data->icu; > struct mvebu_icu_irq_data *icu_irqd; > > icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); > @@ -186,16 +218,22 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > goto free_irqd; > } > > + spin_lock(&icu->msi_lock); > + err = bitmap_allocate_region(icu->msi_bitmap, hwirq, 0); > + spin_unlock(&icu->msi_lock); This (and the freeing counterpart) could deserve a couple of helpers. > + if (err < 0) > + goto free_irqd; > + > if (icu->legacy_bindings) > icu_irqd->icu_group = fwspec->param[0]; > else > - icu_irqd->icu_group = ICU_GRP_NSR; > + icu_irqd->icu_group = msi_data->subset_data->icu_group; > 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; > + goto free_bitmap; > } > > /* Make sure there is no interrupt left pending by the firmware */ > @@ -214,6 +252,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > free_msi: > platform_msi_domain_free(domain, virq, nr_irqs); > +free_bitmap: > + spin_lock(&icu->msi_lock); > + bitmap_release_region(icu->msi_bitmap, hwirq, 0); > + spin_unlock(&icu->msi_lock); > free_irqd: > kfree(icu_irqd); > return err; > @@ -223,12 +265,19 @@ static void > mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs) > { > + struct mvebu_icu_msi_data *msi_data = > + platform_msi_get_host_data(domain); > + struct mvebu_icu *icu = msi_data->icu; > 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); > + > + spin_lock(&icu->msi_lock); > + bitmap_release_region(icu->msi_bitmap, d->hwirq, 0); > + spin_unlock(&icu->msi_lock); > } > > static const struct irq_domain_ops mvebu_icu_domain_ops = { > @@ -239,14 +288,29 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = { > > static int mvebu_icu_subset_probe(struct platform_device *pdev) > { > + const struct mvebu_icu_subset_data *subset; > + struct mvebu_icu_msi_data *msi_data; > struct device_node *msi_parent_dn; > struct irq_domain *irq_domain; > struct mvebu_icu *icu; > > + msi_data = devm_kzalloc(&pdev->dev, sizeof(*msi_data), GFP_KERNEL); > + if (!msi_data) > + return -ENOMEM; > + > icu = mvebu_icu_dev_get_drvdata(pdev); > if (IS_ERR(icu)) > return PTR_ERR(icu); > > + subset = of_device_get_match_data(&pdev->dev); > + if (!subset) { > + dev_err(&pdev->dev, "Could not retrieve subset data\n"); > + return -EINVAL; > + } > + > + msi_data->icu = icu; > + msi_data->subset_data = subset; > + > pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, pdev->dev.of_node, > DOMAIN_BUS_PLATFORM_MSI); > if (!pdev->dev.msi_domain) > @@ -259,7 +323,7 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev) > irq_domain = platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS, > mvebu_icu_write_msg, > &mvebu_icu_domain_ops, > - icu); > + msi_data); > if (!irq_domain) { > dev_err(&pdev->dev, "Failed to create ICU MSI domain\n"); > return -ENOMEM; > @@ -268,9 +332,30 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev) > return 0; > } > > +static const struct mvebu_icu_subset_data mvebu_icu_nsr_subset_data = { > + .icu_group = ICU_GRP_NSR, > + .offset_set_ah = ICU_SETSPI_NSR_AH, > + .offset_set_al = ICU_SETSPI_NSR_AL, > + .offset_clr_ah = ICU_CLRSPI_NSR_AH, > + .offset_clr_al = ICU_CLRSPI_NSR_AL, > +}; > + > +static const struct mvebu_icu_subset_data mvebu_icu_sei_subset_data = { > + .icu_group = ICU_GRP_SEI, > + .offset_set_ah = ICU_SET_SEI_AH, > + .offset_set_al = ICU_SET_SEI_AL, > + .offset_clr_ah = ICU_CLR_SEI_AH, > + .offset_clr_al = ICU_CLR_SEI_AL, I thought SEI was edge only, given what you do in mvebu_icu_init. Confused... > +}; > + > static const struct of_device_id mvebu_icu_subset_of_match[] = { > { > .compatible = "marvell,cp110-icu-nsr", > + .data = &mvebu_icu_nsr_subset_data, > + }, > + { > + .compatible = "marvell,cp110-icu-sei", > + .data = &mvebu_icu_sei_subset_data, > }, > {}, > }; > @@ -317,6 +402,8 @@ static int mvebu_icu_probe(struct platform_device *pdev) > if (IS_ERR(icu->regmap)) > return PTR_ERR(icu->regmap); > > + spin_lock_init(&icu->msi_lock); > + > icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > "ICU.%x", > (unsigned int)res->start); > @@ -341,7 +428,7 @@ static int mvebu_icu_probe(struct platform_device *pdev) > #endif > > /* > - * Clean all ICU interrupts with type SPI_NSR, required to > + * Clean all ICU interrupts of type NSR and SEI, required to > * avoid unpredictable SPI assignments done by firmware. > */ > for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > @@ -350,7 +437,8 @@ static int mvebu_icu_probe(struct platform_device *pdev) > regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int); > icu_grp = icu_int >> ICU_GROUP_SHIFT; > > - if (icu_grp == ICU_GRP_NSR) > + if (icu_grp == ICU_GRP_NSR || > + (icu_grp == ICU_GRP_SEI && !icu->legacy_bindings)) > regmap_write(icu->regmap, ICU_INT_CFG(i), 0); > } > > @@ -363,7 +451,9 @@ static int mvebu_icu_probe(struct platform_device *pdev) > } > > static const struct of_device_id mvebu_icu_of_match[] = { > - { .compatible = "marvell,cp110-icu", }, > + { > + .compatible = "marvell,cp110-icu", > + }, > {}, > }; > > Thanks, M.
Hi Marc, > > +static struct irq_chip mvebu_sei_ap_wired_irq_chip = { > > + .name = "AP wired SEI", > > + .irq_mask = mvebu_sei_mask_irq, > > + .irq_unmask = mvebu_sei_unmask_irq, > > + .irq_eoi = irq_chip_eoi_parent, > > + .irq_set_affinity = irq_chip_set_affinity_parent, > > + .irq_set_type = irq_chip_set_type_parent, > > You seem to assume that this driver is purely dealing with edge > interrupts. And yet you pass the request directly to the parrent. What > does it mean? Shouldn't you at least check that this is an edge request > and fail otherwise? MSI are rising-edge interrupts while wired ones are level (high) interrupts. I will correct this. > > + irq_chip = &mvebu_sei_ap_wired_irq_chip; > > + hwirq = fwspec->param[0]; > > + } else { > > + irq_chip = &mvebu_sei_cp_msi_irq_chip; > > + spin_lock(&sei->cp_msi_lock); > > This could as well be a mutex. Ok. > > > + hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, > > + SEI_IRQ_COUNT, 0); > > It is a bit weird that you're allocating from a 64bit bitmap while you > only have 43 interrupts available... At the 44th interrupt, something > bad is going to happen. Absolutely, to solve this issue, I just had to: s/SEI_IRQ_COUNT/sei->cp_interrupts.number/ > > > + spin_unlock(&sei->cp_msi_lock); > > + if (hwirq < 0) > > + return -ENOSPC; > > + } > > + [...] > > +static void mvebu_sei_handle_cascade_irq(struct irq_desc *desc) > > +{ > > + struct mvebu_sei *sei = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned long irqmap, irq_bit; > > + u32 reg_idx, virq, irqn; > > + > > + chained_irq_enter(chip, desc); > > + > > + /* Read both SEI cause registers (64 bits) */ > > + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++) { > > + irqmap = readl_relaxed(sei->base + GICP_SECR(reg_idx)); > > + > > + /* Call handler for each set bit */ > > + for_each_set_bit(irq_bit, &irqmap, SEI_IRQ_COUNT_PER_REG) { > > + /* Cause Register gives the SEI number */ > > + irqn = irq_bit + reg_idx * SEI_IRQ_COUNT_PER_REG; > > + /* > > + * Finding Linux mapping (virq) needs the right domain > > + * and the relative hwirq (which start at 0 in both > > + * cases, while irqn is relative to all SEI interrupts). > > + */ > > It is a bit odd that you're virtualizing the hwirq number. The whole > point of splitting hwirq from virq is that you don't have to do that and > can use the the raw HW number. You're saving a tiny bit of memory in the > irq_domain, at the expense of more complexity. I don't know if that's > worth it... > > > + if (irqn < sei->ap_interrupts.number) { > > + virq = irq_find_mapping(sei->ap_domain, irqn); > > + } else { > > + irqn -= sei->ap_interrupts.number; > > + virq = irq_find_mapping(sei->cp_domain, irqn); > > + } > > + > > + /* Call IRQ handler */ > > + generic_handle_irq(virq); > > + } > > + > > + /* Clear interrupt indication by writing 1 to it */ > > + writel(irqmap, sei->base + GICP_SECR(reg_idx)); > > + } > > + > > + chained_irq_exit(chip, desc); > > +} [...] > It feels like this patch could do with a total split: > > - Introduce the wired side of the driver > - then the MSI part > > Drop the common domain callbacks, and treat the two domains separately. > I seriously doubt there will be much of an overlap anyway. Maybe I don't get what "saving a tiny bit of memory" really means in this situation. What I am doing right now is duplicating hundreds of lines and changing things like: sei_hwirq = mvebu_sei_domain_to_sei_irq(..., hwirq) into sei_hwirq = sei->ap_interrupts.first + d->hwirq; and sei_hwirq = sei->cp_interrupts.first + d->hwirq; because I still need to translate this hwirq number into an offset within 64 bits. In fact, for each configuration/management operation like clearing, checking or masking an interrupt, a bit must be twisted within a pair of registers. This offset cannot be just the hwirq number, it must be shifted depending on the IRQ domain/type of interrupt. I'm sorry but I will need more guidance on this because I don't see the point in duplicating so much code that was factorized. Thanks, Miquèl -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The ICU is an IRQ chip found in Armada CP110. It currently has 207 wired inputs. Its purpose is to aggregate all CP interrupts and report them to the AP through MSIs. The ICU writes into GIC registers (AP side) by way of the interconnect. These interrupts can be of several groups: - SecuRe (SR); - Non-SecuRe (NSR); - System Error Interrupts (SEI); - RAM Error Interrupts (REI); - ... Each ICU wired interrupt can be of any of these groups. The group is encoded in the MSI payload. Until now, only the non-secure interrupts (NSR) were handled by the ICU driver. Interrupts of another group could work by chance because the ICU driver does not erase all ATF configuration; it only erases the configuration for NSR interrupts. This series aims at adding support for the System Error Interrupts (SEI). For this purpose, the ICU driver is a bit reworked to separate the ICU 'generic' configuration from the NSR-related handling. Then, the SEI driver (part of the GIC) is introduced and finally, support for SEI interrupts are also added to the ICU driver. The SEI driver is a bit different than its cousin the GICP because it must handle MSIs from the CPs, as well as wired interrupts from the AP itself. MSIs and wired interrupts will automatically update two registers (GICP_SECR0/GICP_SECR1) that will trigger a single top-level interrupt (SPI #32). As this is my first contribution in the IRQ subsystem I might have missed some specificities or misunderstood the API, please do not hesitate to correct me if I'm wrong. Also, for the sake of understandability (and because I love ASCII art), this is a try to explain the ICU/SEI architecture: +----------------------------------------------------------------------+ | | | | | SPIa SPIb SPIz SPI 32 | | ^ ^ ^ ^ | | | | . . . | | | | | | | | | | | | . . . | | | | +------------------------+ +---------------------------------+ | | | | | | | | | | | | | | | | | | SEI | | | | | | | . . . | | | ________|_______ | | | | | | | | | /___SEI_SECR_____\ | | | | |____|___________| | | / | \\ | | | | \_GICP_SETSPI _/ | | / | \\ | | | | || | | / ... | \\ | | | | GICP || | | | | \\ | | | +----------------||------+ +--|----------|------------||-----+ | | || | | || | | || | ... | || | | || | | || | | || | | || | | \\_______ int 0 ... int 20 // | | \_NSR__ \ // | | \\ ____________________// | | \\ /________SEI_________/ | | AP 806 \\// | | || | +---------------------------------||-----------------------------------+ || || Interconnect ||\ ||\\______ || \______ <---> Others CP 110 || +---------------------------------||-----------------------------------+ | || | | CP 110 || | | || | | +-------------------------||------------------------+ | | | || MSI | | | | ICU || | | | | /--------------/ \------\ | | | | / /-------/ \ | | | | / / / \ | | | | / / / . . . \ | | | | / / / \ | | | | NSR NSR SEI NSR | | | | | | | | | | | +----^-------^-------^-----------------^------------+ | | | | | | | | | | | . . . | | | | | | | | | int 0 int 1 int 2 int 206 | | | | | +----------------------------------------------------------------------+ Thank you, Miquèl Changes since v1: ================= General ------- * Spelling/function names/comments. * Added Reviewed-by tags. * Rebased on top of Marc Zyngier level-MSI series (tip:irq/core). SEI --- * Change the license for GPL-2.0 only in irq-mvebu-sei.c C file. * Used alphabetic ordering when adding SEI driver in Makefile. * Re-ordered register definitions by increasing offset. * s/NB/COUNT/ in register definitions. * avoid enabling all interrupt by default. * fixed mask/unmask functions using the wrong hwirq number. * removed hackish doorbell mechanism. * Removed the ->xlate hook assigned for CP MSIs. * Used devm_*() helpers. * s/top_level_spi/parent_irq/ in probe. * Added forgotten of_node_put(child). * Reset the SEI registers before registering the IRQ domains. * Introduced new DT property "marvell,sei-ranges" instead of using "reg" to declare the range of MSI interrupts vs. wired interrupts in the SEI subnodes. * Finally did not change the ->alloc() about the fwspec->param[1] line (to be checked by Marc). ICU --- * Updated the ICU documentation so the legacy bindings are still documented somewhere. * Added stable tags on the commit fixing the CP110 ICU node size. * Removed the "syscon" compatible from the ICU node, instead the syscon is created at probe time. * s/user data/private data/ in the title of commit "irqchip/irq-mvebu-icu: fix wrong user data retrieval" Miquel Raynal (16): dt-bindings/interrupt-controller: fix Marvell ICU length in the example arm64: dts: marvell: fix CP110 ICU node size irqchip/irq-mvebu-icu: fix wrong private data retrieval irqchip/irq-mvebu-icu: clarify the reset operation of configured interrupts irqchip/irq-mvebu-icu: switch to regmap irqchip/irq-mvebu-icu: make irq_domain local irqchip/irq-mvebu-icu: disociate ICU and NSR irqchip/irq-mvebu-icu: support ICU subnodes irqchip/irq-mvebu-sei: add new driver for Marvell SEI arm64: marvell: enable SEI driver irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI) dt-bindings/interrupt-controller: update Marvell ICU bindings dt-bindings/interrupt-controller: add documentation for Marvell SEI controller arm64: dts: marvell: add AP806 SEI subnode arm64: dts: marvell: use new bindings for CP110 interrupts arm64: dts: marvell: add CP110 ICU SEI subnode .../bindings/interrupt-controller/marvell,icu.txt | 83 +++- .../bindings/interrupt-controller/marvell,sei.txt | 50 +++ arch/arm64/Kconfig.platforms | 1 + arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 19 + arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 123 +++--- drivers/irqchip/Kconfig | 3 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-mvebu-icu.c | 301 ++++++++++++--- drivers/irqchip/irq-mvebu-sei.c | 422 +++++++++++++++++++++ 9 files changed, 872 insertions(+), 131 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt create mode 100644 drivers/irqchip/irq-mvebu-sei.c