Re: [PATCH v3 10/17] irqchip/irq-mvebu-sei: add new driver for Marvell SEI

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 15:54:30
+0100:

> On 22/06/18 16:14, 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
> > 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 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 | 444 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 448 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..4a62cd2385d7
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-mvebu-sei.c
> > @@ -0,0 +1,444 @@
> > +// 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;  
> 
> nit: number is a bit vague. consider using size (or anything similar).

Ack.

> 
> > +};
> > +
> > +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 */
> > +	struct mutex 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));  
> 
> Consider using _relaxed accessors, unless this relies on ordering with
> other memory accesses (I really don't think it does).

I don't think it does neither, I just forgot about the _relaxed
variants. I will update for all register reads/writes.

> 
> > +}
> > +
> > +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 int mvebu_sei_ap_set_type(struct irq_data *data, unsigned int type)
> > +{
> > +	if (!(type & IRQ_TYPE_LEVEL_HIGH))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mvebu_sei_cp_set_type(struct irq_data *data, unsigned int type)
> > +{
> > +	if (!(type & IRQ_TYPE_EDGE_RISING))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +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		= mvebu_sei_ap_set_type,
> > +};
> > +
> > +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		= mvebu_sei_cp_set_type,
> > +	.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;
> > +
> > +	/* The 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;
> > +		mutex_lock(&sei->cp_msi_lock);
> > +		hwirq = bitmap_find_free_region(sei->cp_msi_bitmap,
> > +						sei->cp_interrupts.number, 0);
> > +		mutex_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) {
> > +		mutex_lock(&sei->cp_msi_lock);
> > +		bitmap_release_region(sei->cp_msi_bitmap, hwirq, 0);
> > +		mutex_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);
> > +
> > +	mutex_lock(&sei->cp_msi_lock);
> > +	bitmap_release_region(sei->cp_msi_bitmap, d->hwirq, 0);
> > +	mutex_unlock(&sei->cp_msi_lock);
> > +}
> > +
> > +static int mvebu_sei_ap_match(struct irq_domain *d, struct device_node *node,
> > +			      enum irq_domain_bus_token bus_token)
> > +{
> > +	struct mvebu_sei *sei = d->host_data;  
> 
> If you're dereferencing the domain here...
> 
> > +
> > +	if (!sei)
> > +		return 0;
> > +
> > +	if (sei->dev->of_node != node)
> > +		return 0;
> > +
> > +	if (!d || !sei->ap_domain)  
> 
> ... how can it be NULL here? Actually, how can it *ever* be NULL?

Over-sanitization indeed. d cannot be NULL.

> 
> Also, it is not clear to me how can sei or sei->ap_domain be NULL.

I misread some internal functions of the core and after a second look
you are right that there is no possibility for both of them to be NULL.

I'll simplify.

> 
> > +		return 0;
> > +
> > +	if (d == sei->ap_domain)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops mvebu_sei_ap_domain_ops = {
> > +	.match = mvebu_sei_ap_match,
> > +	.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 MSI controller",
> > +	.irq_set_type	= mvebu_sei_cp_set_type,
> > +};
> > +
> > +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));  
> 
> I think it'd be a lot clearer if you just read both 32bit registers and
> treat the resulting 64bit quantity as a single bitmap, rather than
> having this outer loop.

True, there is no need for this outer loop once the bitmap is 64-bit
wide.

> 
> > +
> > +		/* 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).
> > +			 */
> > +			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));  
> 
> write_relaxed. It would probably make sense not to write to that
> register if no bits were set to 1 the first place.

Will do it.

> 
> > +	}
> > +
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static int mvebu_sei_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *node = pdev->dev.of_node, *parent;
> > +	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;
> > +
> > +	mutex_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 can be triggered from the AP through wired interrupts and from
> > +	 * the CPs through MSIs.
> > +	 */
> > +
> > +	/* 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;
> > +	}  
> 
> Wouldn't it make sense to check this before having configured things
> with a chained handler? i.e. check everything that can be checked, and
> once you have everything in order, do the plumbing?

Ok

> 
> > +
> > +	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;
> > +	}
> > +
> > +	/*
> > +	 * Retrieve the IRQ organization (AP/CP): the index of the first one and
> > +	 * the number of them for each domain.
> > +	 */
> > +	property = of_get_property(node, "marvell,sei-ap-ranges", &size);
> > +	if (!property || (size != (2 * sizeof(u32)))) {
> > +		dev_err(sei->dev, "Missing 'marvell,sei-ap-ranges' 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]);
> > +
> > +	property = of_get_property(node, "marvell,sei-cp-ranges", &size);
> > +	if (!property || (size != (2 * sizeof(u32)))) {
> > +		dev_err(sei->dev, "Missing 'marvell,sei-cp-ranges' property\n");
> > +		ret = -ENODEV;
> > +		goto dispose_irq;
> > +	}
> > +
> > +	sei->cp_interrupts.first = be32_to_cpu(property[0]);
> > +	sei->cp_interrupts.number = be32_to_cpu(property[1]);
> > +
> > +	/* Create the 'wired' hierarchy */
> > +	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
> > +						     sei->ap_interrupts.number,
> > +						     of_node_to_fwnode(node),
> > +						     &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 */
> > +	sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0,
> > +						     sei->cp_interrupts.number,
> > +						     of_node_to_fwnode(node),
> > +						     &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(node),
> > +						     &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);
> > +
> > +	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);
> >   
> 
> Thanks,
> 
> 	M.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel