| Message ID | 20180421135537.24716-11-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Sat, 21 Apr 2018 15:55:30 +0200, Miquel Raynal wrote: > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 5ed465ab1c76..6b5b75cb4694 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -73,6 +73,7 @@ obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o > obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o > obj-$(CONFIG_MSCC_OCELOT_IRQ) += irq-mscc-ocelot.o > obj-$(CONFIG_MVEBU_GICP) += irq-mvebu-gicp.o > +obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.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 Alphabetic ordering would put SEI after PIC I guess :) > diff --git a/drivers/irqchip/irq-mvebu-sei.c b/drivers/irqchip/irq-mvebu-sei.c > new file mode 100644 > index 000000000000..5c12c74e3f09 > --- /dev/null > +++ b/drivers/irqchip/irq-mvebu-sei.c > @@ -0,0 +1,449 @@ > +// SPDX-License-Identifier: GPL-2.0 OR X11 License for code is GPL-2.0 only. We use GPL-2.0 OR X11 for Device Trees. > +#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> > + > +#define GICP_SET_SEI_OFFSET 0x30 > +#define GICP_CLR_SEI_OFFSET GICP_SET_SEI_OFFSET Why do you have this concept of set/clr if there is only one register ? I assume that if there is only a "set" register, it means that SEI interrupts can only be edge triggered, contrary to NSR interrupts, which have separate set/clr to support level-triggered interrupts. Having only a "set" offset means that we can rely on the existing "struct msi_msg" to convey both the MSI doorbell address and payload, and we don't need the mvebu_gicp_get_doorbells() hack. > +/* Cause register */ > +#define GICP_SECR(idx) (0x0 + (idx * 0x4)) > +/* Mask register */ > +#define GICP_SEMR(idx) (0x20 + (idx * 0x4)) Minor nit: order register definitions by order of increasing offset, i.e the GICP_SET_SEI_OFFSET should be defined here. > +#define SEI_IRQ_NB_PER_REG 32 > +#define SEI_IRQ_REG_NB 2 s/NB/COUNT/ > +#define SEI_IRQ_NB (SEI_IRQ_NB_PER_REG * SEI_IRQ_REG_NB) Ditto. > +#define SEI_IRQ_REG_IDX(irq_id) (irq_id / SEI_IRQ_NB_PER_REG) > +#define SEI_IRQ_REG_BIT(irq_id) (irq_id % SEI_IRQ_NB_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_NB); > +}; > + > +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; I am not entirely clear whether we need subnodes or not in this binding, but I guess we do because we have one subset of the interrupts that are wired interrupts, and another part that are MSI triggered. Perhaps this is one aspect on which Marc Zyngier can comment ? > +static void mvebu_sei_reset(struct mvebu_sei *sei) > +{ > + u32 reg_idx; > + > + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_NB; reg_idx++) { > + /* Clear all cause bits */ > + writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx)); > + /* Enable all interrupts */ > + writel(0, sei->base + GICP_SEMR(reg_idx)); Enabling interrupts by default ? This looks weird. They should only be enabled... when enabled. > +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, > + phys_addr_t *clr) > +{ > + struct platform_device *pdev; > + struct mvebu_sei *sei; > + > + pdev = of_find_device_by_node(dn->parent); > + if (!pdev) > + return -ENODEV; > + > + sei = platform_get_drvdata(pdev); > + if (!sei) > + return -ENODEV; > + > + *set = (phys_addr_t)(sei->res->start + GICP_SET_SEI_OFFSET); > + *clr = (phys_addr_t)(sei->res->start + GICP_CLR_SEI_OFFSET); > + > + return 0; > +} As I said above, I believe this hack is not needed, because SEIs are edge-triggered, and we have a single SET_SEI_OFFSET MSI doorbell address to convey, which makes "struct msi_msg" as it is today sufficient. > +static void mvebu_sei_mask_irq(struct irq_data *d) > +{ > + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); > + u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); This doesn't look right. The d->hwirq is relative to the beginning of the domain, while you should use sei_irq here. For example, the SEI n°32 (first interrupt in the second register) will have d->hwirq = 11, because it is the 11th SEI interrupt for the CP. So here, you will conclude that reg_idx = 0, while it should be reg_idx = 1. > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); > + u32 reg; > + > + /* 1 disables the interrupt */ > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > + writel(reg | irq_mask, sei->base + GICP_SEMR(reg_idx)); Personal taste here, but I prefer: 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 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); Same mistake as above I believe. > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); > + u32 reg; > + > + /* 0 enables the interrupt */ > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > + writel(reg & ~irq_mask, sei->base + GICP_SEMR(reg_idx)); And same nitpick comment :-) > +static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, unsigned int nr_irqs, > + void *args) I think the coding style says that arguments should be aligned, no ? > +{ > + 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) { > + 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); > + hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, SEI_IRQ_NB, > + 0); > + spin_unlock(&sei->cp_msi_lock); > + if (hwirq < 0) > + return -ENOSPC; > + } > + > + sei_hwirq = mvebu_sei_domain_to_sei_irq(sei, domain, hwirq); > + > + 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; Maybe it's me being confused, but I thought all SEI interrupts were muxed together to a single SPI for the parent GIC. But here, you allocate different SPIs at the GIC level. Intuitively, this doesn't look good. Haven't you copy/pasted too much from the gicp driver, where we have a 1:1 mapping between interrupts coming into the GICP and interrupts signaled by the GICP to the GIC, while here we have a N:1 mapping, with N interrupts coming into the GICP_SEI, and only one interrupt leaving the GICP_SEI to the GIC ? > +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 = { > + .xlate = irq_domain_xlate_twocell, > + .alloc = mvebu_sei_irq_domain_alloc, > + .free = mvebu_sei_irq_domain_free, > +}; Why do you need two cells for the interrupts coming from the CP and only one cell for the interrupts coming from the AP ? For thermal in the AP, you do: + interrupt-parent = <&sei_wired_controller>; + interrupts = <18>; i.e, you don't specify an interrupt type. For thermal in the CP, you do: + interrupts-extended = + <&CP110_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>; here you specify an interrupt type. I'm not sure why you have this difference. Even more so because I think a SEI level interrupt is not possible, since you only have a "SET" register and no "CLR" register. Some guidance from Marc here might be useful perhaps. > +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 top_level_spi, 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); > + if (!sei->res) { > + dev_err(sei->dev, "Failed to retrieve SEI resource\n"); > + return -ENODEV; > + } > + > + sei->base = devm_ioremap(sei->dev, sei->res->start, > + resource_size(sei->res)); > + if (!sei->base) { > + dev_err(sei->dev, "Failed to remap SEI resource\n"); > + return -ENODEV; > + } Use devm_ioremap_resource() here, and remove the error handling of platform_get_resource(), because it's already taken care of by devm_ioremap_resource(). > + /* > + * Reserve the single (top-level) parent SPI IRQ from which all the > + * interrupts handled by this driver will be signaled. > + */ > + top_level_spi = irq_of_parse_and_map(node, 0); > + if (top_level_spi <= 0) { > + dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n"); > + return -ENODEV; > + } Rather than top_level_spi, something like parent_irq would make more sense to me. > + irq_set_chained_handler(top_level_spi, mvebu_sei_handle_cascade_irq); > + irq_set_handler_data(top_level_spi, 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; > + } Don't forget to of_node_put(child) once you're done using this DT node reference. > + > + property = of_get_property(child, "reg", &size); > + if (!property || size != (2 * sizeof(u32))) { > + dev_err(sei->dev, "Missing subnode 'reg' property\n"); > + ret = -ENODEV; > + goto dispose_irq; > + } As Rob said, I don't think the "reg" property is appropriate for this usage. > + 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); > + 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; > + } Ditto: missing of_node_put(child) somewhere below to balance of_find_node_by_name(). > + property = of_get_property(child, "reg", &size); > + if (!property || size != (2 * sizeof(u32))) { > + dev_err(sei->dev, "Missing subnode 'reg' property\n"); > + 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"); > + 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); > + if (!plat_domain) { > + pr_err("Failed to create CPs MSI domain\n"); > + ret = -ENOMEM; > + goto remove_cp_domain; > + } > + > + platform_set_drvdata(pdev, sei); > + > + mvebu_sei_reset(sei); Please do the reset *before* registering the IRQ domains, it's more logical to have the HW ready and then expose it to Linux rather than the opposite. It would be nice to have the review from Marc on this driver, especially on whether the SEI is properly modeled in terms of IRQ domains; > diff --git a/drivers/irqchip/irq-mvebu-sei.h b/drivers/irqchip/irq-mvebu-sei.h > new file mode 100644 > index 000000000000..f0c12a441923 > --- /dev/null > +++ b/drivers/irqchip/irq-mvebu-sei.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __MVEBU_SEI_H__ > +#define __MVEBU_SEI_H__ > + > +#include <linux/types.h> > + > +struct device_node; > + > +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, > + phys_addr_t *clr); > + > +#endif /* __MVEBU_SEI_H__ */ This header file can be removed if you drop mvebu_sei_get_doorbells(), as suggested above. Best regards, Thomas
Hello, On Wed, 2 May 2018 11:17:44 +0200, Thomas Petazzoni wrote: > > +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 = { > > + .xlate = irq_domain_xlate_twocell, > > + .alloc = mvebu_sei_irq_domain_alloc, > > + .free = mvebu_sei_irq_domain_free, > > +}; > > Why do you need two cells for the interrupts coming from the CP and > only one cell for the interrupts coming from the AP ? > > For thermal in the AP, you do: > > + interrupt-parent = <&sei_wired_controller>; > + interrupts = <18>; > > i.e, you don't specify an interrupt type. For thermal in the CP, you do: > > + interrupts-extended = > + <&CP110_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>; > > here you specify an interrupt type. I'm not sure why you have this > difference. Even more so because I think a SEI level interrupt is not > possible, since you only have a "SET" register and no "CLR" register. OK, my comment is not very correct here, I'm comparing apple to oranges. The former its an interrupt directly pointing to the GICP_SEI, while the latter is an interrupt of the ICU, which itself will notify the GICP_SEI through an MSI. However, I'm still confused as to why you have .xlate = irq_domain_xlate_twocell for the mvebu_sei_cp_domain_ops. I think there is no need for ->xlate() call back here because it's going to be a MSI domain. Best regards, Thomas
Hi Thomas, On Wed, 2 May 2018 11:17:44 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Sat, 21 Apr 2018 15:55:30 +0200, Miquel Raynal wrote: > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 5ed465ab1c76..6b5b75cb4694 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -73,6 +73,7 @@ obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o > > obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o > > obj-$(CONFIG_MSCC_OCELOT_IRQ) += irq-mscc-ocelot.o > > obj-$(CONFIG_MVEBU_GICP) += irq-mvebu-gicp.o > > +obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.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 > > Alphabetic ordering would put SEI after PIC I guess :) Sure. > > > diff --git a/drivers/irqchip/irq-mvebu-sei.c b/drivers/irqchip/irq-mvebu-sei.c > > new file mode 100644 > > index 000000000000..5c12c74e3f09 > > --- /dev/null > > +++ b/drivers/irqchip/irq-mvebu-sei.c > > @@ -0,0 +1,449 @@ > > +// SPDX-License-Identifier: GPL-2.0 OR X11 > > License for code is GPL-2.0 only. We use GPL-2.0 OR X11 for Device > Trees. I did not know, thanks for the explanation. > > > +#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> > > + > > +#define GICP_SET_SEI_OFFSET 0x30 > > +#define GICP_CLR_SEI_OFFSET GICP_SET_SEI_OFFSET > > Why do you have this concept of set/clr if there is only one register ? > I assume that if there is only a "set" register, it means that SEI > interrupts can only be edge triggered, contrary to NSR interrupts, > which have separate set/clr to support level-triggered interrupts. > > Having only a "set" offset means that we can rely on the existing > "struct msi_msg" to convey both the MSI doorbell address and payload, > and we don't need the mvebu_gicp_get_doorbells() hack. I misunderstood the specification at first (when copying code from the gicp driver), and then continued in my mistake. I removed the whole doorbell thing. > > > +/* Cause register */ > > +#define GICP_SECR(idx) (0x0 + (idx * 0x4)) > > +/* Mask register */ > > +#define GICP_SEMR(idx) (0x20 + (idx * 0x4)) > > Minor nit: order register definitions by order of increasing offset, > i.e the GICP_SET_SEI_OFFSET should be defined here. Ok. > > > +#define SEI_IRQ_NB_PER_REG 32 > > +#define SEI_IRQ_REG_NB 2 > > s/NB/COUNT/ Changed. > > > +#define SEI_IRQ_NB (SEI_IRQ_NB_PER_REG * SEI_IRQ_REG_NB) > > Ditto. > > > +#define SEI_IRQ_REG_IDX(irq_id) (irq_id / SEI_IRQ_NB_PER_REG) > > +#define SEI_IRQ_REG_BIT(irq_id) (irq_id % SEI_IRQ_NB_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_NB); > > +}; > > + > > +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; > > I am not entirely clear whether we need subnodes or not in this > binding, but I guess we do because we have one subset of the interrupts > that are wired interrupts, and another part that are MSI triggered. > > Perhaps this is one aspect on which Marc Zyngier can comment ? > > > +static void mvebu_sei_reset(struct mvebu_sei *sei) > > +{ > > + u32 reg_idx; > > + > > + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_NB; reg_idx++) { > > + /* Clear all cause bits */ > > + writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx)); > > + /* Enable all interrupts */ > > + writel(0, sei->base + GICP_SEMR(reg_idx)); > > Enabling interrupts by default ? This looks weird. They should only be > enabled... when enabled. That's right, no need to enable them here. > > > +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, > > + phys_addr_t *clr) > > +{ > > + struct platform_device *pdev; > > + struct mvebu_sei *sei; > > + > > + pdev = of_find_device_by_node(dn->parent); > > + if (!pdev) > > + return -ENODEV; > > + > > + sei = platform_get_drvdata(pdev); > > + if (!sei) > > + return -ENODEV; > > + > > + *set = (phys_addr_t)(sei->res->start + GICP_SET_SEI_OFFSET); > > + *clr = (phys_addr_t)(sei->res->start + GICP_CLR_SEI_OFFSET); > > + > > + return 0; > > +} > > As I said above, I believe this hack is not needed, because SEIs are > edge-triggered, and we have a single SET_SEI_OFFSET MSI doorbell > address to convey, which makes "struct msi_msg" as it is today > sufficient. Removed. > > > +static void mvebu_sei_mask_irq(struct irq_data *d) > > +{ > > + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); > > + u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); > > This doesn't look right. The d->hwirq is relative to the beginning of > the domain, while you should use sei_irq here. For example, the SEI > n°32 (first interrupt in the second register) will have d->hwirq = 11, > because it is the 11th SEI interrupt for the CP. So here, you will > conclude that reg_idx = 0, while it should be reg_idx = 1. This is true. I did not saw it because... well... all interrupts were enabled by default. > > > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > > + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); > > + u32 reg; > > + > > + /* 1 disables the interrupt */ > > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > > + writel(reg | irq_mask, sei->base + GICP_SEMR(reg_idx)); > > Personal taste here, but I prefer: > > reg = readl(sei->base + GICP_SEMR(reg_idx)); > reg |= BIT(SEI_IRQ_REG_BIT(sei_irq)); > writel(reg, sei->base + GICP_SEMR(reg_idx)); No problem. > > > +static void mvebu_sei_unmask_irq(struct irq_data *d) > > +{ > > + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); > > + u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); > > Same mistake as above I believe. > > > + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); > > + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); > > + u32 reg; > > + > > + /* 0 enables the interrupt */ > > + reg = readl(sei->base + GICP_SEMR(reg_idx)); > > + writel(reg & ~irq_mask, sei->base + GICP_SEMR(reg_idx)); > > And same nitpick comment :-) > > > +static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain, > > + unsigned int virq, unsigned int nr_irqs, > > + void *args) > > I think the coding style says that arguments should be aligned, no ? > > > +{ > > + 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) { > > + 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); > > + hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, SEI_IRQ_NB, > > + 0); > > + spin_unlock(&sei->cp_msi_lock); > > + if (hwirq < 0) > > + return -ENOSPC; > > + } > > + > > + sei_hwirq = mvebu_sei_domain_to_sei_irq(sei, domain, hwirq); > > + > > + 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; > > Maybe it's me being confused, but I thought all SEI interrupts were > muxed together to a single SPI for the parent GIC. But here, you > allocate different SPIs at the GIC level. Intuitively, this doesn't > look good. Haven't you copy/pasted too much from the gicp driver, where > we have a 1:1 mapping between interrupts coming into the GICP and > interrupts signaled by the GICP to the GIC, while here we have a N:1 > mapping, with N interrupts coming into the GICP_SEI, and only one > interrupt leaving the GICP_SEI to the GIC ? I am a bit in troubles understanding what fwspec->param[1] exactly means here. I suppose I should s/sei_hwirq/0/ as there is only one SPI interrupt to refer to? Maybe Marc can comment on this too? > > > +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 = { > > + .xlate = irq_domain_xlate_twocell, > > + .alloc = mvebu_sei_irq_domain_alloc, > > + .free = mvebu_sei_irq_domain_free, > > +}; > > Why do you need two cells for the interrupts coming from the CP and > only one cell for the interrupts coming from the AP ? > > For thermal in the AP, you do: > > + interrupt-parent = <&sei_wired_controller>; > + interrupts = <18>; > > i.e, you don't specify an interrupt type. For thermal in the CP, you do: > > + interrupts-extended = > + <&CP110_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>; > > here you specify an interrupt type. I'm not sure why you have this > difference. Even more so because I think a SEI level interrupt is not > possible, since you only have a "SET" register and no "CLR" register. and then you wrote: <quote> > OK, my comment is not very correct here, I'm comparing apple to > oranges. The former its an interrupt directly pointing to the GICP_SEI, > while the latter is an interrupt of the ICU, which itself will notify > the GICP_SEI through an MSI. > However, I'm still confused as to why you have .xlate = > irq_domain_xlate_twocell for the mvebu_sei_cp_domain_ops. I think there > is no need for ->xlate() call back here because it's going to be a MSI > domain. </quote> For thermal in the AP I should probably add an IRQ_TYPE_LEVEL_HIGH in order to describe properly the interrupt (wired). I also removed the .xlate entry for the CP domain, I was not sure it was useless for MSI but it looks it is. > > Some guidance from Marc here might be useful perhaps. > > > > +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 top_level_spi, 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); > > + if (!sei->res) { > > + dev_err(sei->dev, "Failed to retrieve SEI resource\n"); > > + return -ENODEV; > > + } > > + > > + sei->base = devm_ioremap(sei->dev, sei->res->start, > > + resource_size(sei->res)); > > + if (!sei->base) { > > + dev_err(sei->dev, "Failed to remap SEI resource\n"); > > + return -ENODEV; > > + } > > Use devm_ioremap_resource() here, and remove the error handling of > platform_get_resource(), because it's already taken care of by > devm_ioremap_resource(). Good tip, I'll remember. > > > + /* > > + * Reserve the single (top-level) parent SPI IRQ from which all the > > + * interrupts handled by this driver will be signaled. > > + */ > > + top_level_spi = irq_of_parse_and_map(node, 0); > > + if (top_level_spi <= 0) { > > + dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n"); > > + return -ENODEV; > > + } > > Rather than top_level_spi, something like parent_irq would make more > sense to me. Renamed. > > > + irq_set_chained_handler(top_level_spi, mvebu_sei_handle_cascade_irq); > > + irq_set_handler_data(top_level_spi, 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; > > + } > > Don't forget to of_node_put(child) once you're done using this DT node > reference. Ok. > > > + > > + property = of_get_property(child, "reg", &size); > > + if (!property || size != (2 * sizeof(u32))) { > > + dev_err(sei->dev, "Missing subnode 'reg' property\n"); > > + ret = -ENODEV; > > + goto dispose_irq; > > + } > > As Rob said, I don't think the "reg" property is appropriate for this > usage. I will change. > > > + 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); > > + 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; > > + } > > Ditto: missing of_node_put(child) somewhere below to balance > of_find_node_by_name(). Ok. > > > + property = of_get_property(child, "reg", &size); > > + if (!property || size != (2 * sizeof(u32))) { > > + dev_err(sei->dev, "Missing subnode 'reg' property\n"); > > + 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"); > > + 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); > > + if (!plat_domain) { > > + pr_err("Failed to create CPs MSI domain\n"); > > + ret = -ENOMEM; > > + goto remove_cp_domain; > > + } > > > > > + > > + platform_set_drvdata(pdev, sei); > > + > > + mvebu_sei_reset(sei); > > Please do the reset *before* registering the IRQ domains, it's more > logical to have the HW ready and then expose it to Linux rather than > the opposite. Sure. > > It would be nice to have the review from Marc on this driver, > especially on whether the SEI is properly modeled in terms of IRQ > domains; > > > diff --git a/drivers/irqchip/irq-mvebu-sei.h b/drivers/irqchip/irq-mvebu-sei.h > > new file mode 100644 > > index 000000000000..f0c12a441923 > > --- /dev/null > > +++ b/drivers/irqchip/irq-mvebu-sei.h > > @@ -0,0 +1,12 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef __MVEBU_SEI_H__ > > +#define __MVEBU_SEI_H__ > > + > > +#include <linux/types.h> > > + > > +struct device_node; > > + > > +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, > > + phys_addr_t *clr); > > + > > +#endif /* __MVEBU_SEI_H__ */ > > This header file can be removed if you drop mvebu_sei_get_doorbells(), > as suggested above. Indeed. > > Best regards, > > Thomas Thanks! Miquèl
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 5ed465ab1c76..6b5b75cb4694 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -73,6 +73,7 @@ obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o obj-$(CONFIG_PIC32_EVIC) += irq-pic32-evic.o obj-$(CONFIG_MSCC_OCELOT_IRQ) += irq-mscc-ocelot.o obj-$(CONFIG_MVEBU_GICP) += irq-mvebu-gicp.o +obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.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 diff --git a/drivers/irqchip/irq-mvebu-sei.c b/drivers/irqchip/irq-mvebu-sei.c new file mode 100644 index 000000000000..5c12c74e3f09 --- /dev/null +++ b/drivers/irqchip/irq-mvebu-sei.c @@ -0,0 +1,449 @@ +// SPDX-License-Identifier: GPL-2.0 OR X11 + +#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> + +#define GICP_SET_SEI_OFFSET 0x30 +#define GICP_CLR_SEI_OFFSET GICP_SET_SEI_OFFSET + +/* Cause register */ +#define GICP_SECR(idx) (0x0 + (idx * 0x4)) +/* Mask register */ +#define GICP_SEMR(idx) (0x20 + (idx * 0x4)) + +#define SEI_IRQ_NB_PER_REG 32 +#define SEI_IRQ_REG_NB 2 +#define SEI_IRQ_NB (SEI_IRQ_NB_PER_REG * SEI_IRQ_REG_NB) +#define SEI_IRQ_REG_IDX(irq_id) (irq_id / SEI_IRQ_NB_PER_REG) +#define SEI_IRQ_REG_BIT(irq_id) (irq_id % SEI_IRQ_NB_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_NB); +}; + +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; + + for (reg_idx = 0; reg_idx < SEI_IRQ_REG_NB; reg_idx++) { + /* Clear all cause bits */ + writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx)); + /* Enable all interrupts */ + writel(0, sei->base + GICP_SEMR(reg_idx)); + } +} + +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, + phys_addr_t *clr) +{ + struct platform_device *pdev; + struct mvebu_sei *sei; + + pdev = of_find_device_by_node(dn->parent); + if (!pdev) + return -ENODEV; + + sei = platform_get_drvdata(pdev); + if (!sei) + return -ENODEV; + + *set = (phys_addr_t)(sei->res->start + GICP_SET_SEI_OFFSET); + *clr = (phys_addr_t)(sei->res->start + GICP_CLR_SEI_OFFSET); + + return 0; +} + +static void mvebu_sei_mask_irq(struct irq_data *d) +{ + struct mvebu_sei *sei = irq_data_get_irq_chip_data(d); + u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); + u32 reg; + + /* 1 disables the interrupt */ + reg = readl(sei->base + GICP_SEMR(reg_idx)); + writel(reg | irq_mask, 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 reg_idx = SEI_IRQ_REG_IDX(d->hwirq); + u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq); + u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq)); + u32 reg; + + /* 0 enables the interrupt */ + reg = readl(sei->base + GICP_SEMR(reg_idx)); + writel(reg & ~irq_mask, 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, +}; + +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, + .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) { + 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); + hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, SEI_IRQ_NB, + 0); + spin_unlock(&sei->cp_msi_lock); + if (hwirq < 0) + return -ENOSPC; + } + + sei_hwirq = mvebu_sei_domain_to_sei_irq(sei, domain, hwirq); + + 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 = { + .xlate = irq_domain_xlate_twocell, + .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, +}; + +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_NB; 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_NB_PER_REG) { + /* Cause Register gives the SEI number */ + irqn = irq_bit + reg_idx * SEI_IRQ_NB_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). + */ + 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 top_level_spi, 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); + if (!sei->res) { + dev_err(sei->dev, "Failed to retrieve SEI resource\n"); + return -ENODEV; + } + + sei->base = devm_ioremap(sei->dev, sei->res->start, + resource_size(sei->res)); + if (!sei->base) { + dev_err(sei->dev, "Failed to remap SEI resource\n"); + return -ENODEV; + } + + /* + * Reserve the single (top-level) parent SPI IRQ from which all the + * interrupts handled by this driver will be signaled. + */ + top_level_spi = irq_of_parse_and_map(node, 0); + if (top_level_spi <= 0) { + dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n"); + return -ENODEV; + } + + irq_set_chained_handler(top_level_spi, mvebu_sei_handle_cascade_irq); + irq_set_handler_data(top_level_spi, 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, "reg", &size); + if (!property || size != (2 * sizeof(u32))) { + dev_err(sei->dev, "Missing subnode 'reg' property\n"); + 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); + 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, "reg", &size); + if (!property || size != (2 * sizeof(u32))) { + dev_err(sei->dev, "Missing subnode 'reg' property\n"); + 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"); + 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); + if (!plat_domain) { + pr_err("Failed to create CPs MSI domain\n"); + ret = -ENOMEM; + goto remove_cp_domain; + } + + platform_set_drvdata(pdev, sei); + + mvebu_sei_reset(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(top_level_spi); + + 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); diff --git a/drivers/irqchip/irq-mvebu-sei.h b/drivers/irqchip/irq-mvebu-sei.h new file mode 100644 index 000000000000..f0c12a441923 --- /dev/null +++ b/drivers/irqchip/irq-mvebu-sei.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __MVEBU_SEI_H__ +#define __MVEBU_SEI_H__ + +#include <linux/types.h> + +struct device_node; + +int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set, + phys_addr_t *clr); + +#endif /* __MVEBU_SEI_H__ */
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 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 ('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 | 449 ++++++++++++++++++++++++++++++++++++++++ drivers/irqchip/irq-mvebu-sei.h | 12 ++ 4 files changed, 465 insertions(+) create mode 100644 drivers/irqchip/irq-mvebu-sei.c create mode 100644 drivers/irqchip/irq-mvebu-sei.h