Subject: Re: [PATCH v3 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU
To: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>,
        Kumar Gala <galak@codeaurora.org>, Andrew Lunn <andrew@lunn.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        Gregory Clement <gregory.clement@free-electrons.com>
References: <20170616141923.31226-1-thomas.petazzoni@free-electrons.com>
 <20170616141923.31226-5-thomas.petazzoni@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org,
        Nadav Haklai <nadavh@marvell.com>, Hanna Hawa <hannah@marvell.com>,
        Yehuda Yitschak <yehuday@marvell.com>,
        Antoine Tenart <antoine.tenart@free-electrons.com>
From: Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <2934dbf3-e1f0-3997-ba70-3b6a7a2973d9@arm.com>
Date: Mon, 19 Jun 2017 18:40:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170616141923.31226-5-thomas.petazzoni@free-electrons.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 13198
Lines: 416

On 16/06/17 15:19, Thomas Petazzoni wrote:
> The Marvell ICU unit is found in the CP110 block of the Marvell Armada
> 7K and 8K SoCs. It collects the wired interrupts of the devices located
> in the CP110 and turns them into SPI interrupts in the GIC located in
> the AP806 side of the SoC, by using a memory transaction.
> 
> Until now, the ICU was configured in a static fashion by the firmware,
> and Linux was relying on this static configuration. By having Linux
> configure the ICU, we are more flexible, and we can allocate dynamically
> the GIC SPI interrupts only for devices that are actually in use.
> 
> The driver was initially written by Hanna Hawa <hannah@marvell.com>.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/Kconfig                            |   3 +
>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-mvebu-icu.c                    | 299 +++++++++++++++++++++
>  .../dt-bindings/interrupt-controller/mvebu-icu.h   |  15 ++
>  4 files changed, 318 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mvebu-icu.c
>  create mode 100644 include/dt-bindings/interrupt-controller/mvebu-icu.h
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e527ee5..676232a 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -271,6 +271,9 @@ config IRQ_MXS
>  config MVEBU_GICP
>  	bool
>  
> +config MVEBU_ICU
> +	bool
> +
>  config MVEBU_ODMI
>  	bool
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 11eb858..18fa5fa 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>  obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
> +obj-$(CONFIG_MVEBU_ICU)			+= irq-mvebu-icu.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> new file mode 100644
> index 0000000..6b33998
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-icu.c
> @@ -0,0 +1,299 @@
> +/*
> + * Copyright (C) 2017 Marvell
> + *
> + * Hanna Hawa <hannah@marvell.com>
> + * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/interrupt-controller/mvebu-icu.h>
> +
> +#include "irq-mvebu-gicp.h"
> +
> +/* ICU registers */
> +#define ICU_SETSPI_NSR_AL	0x10
> +#define ICU_SETSPI_NSR_AH	0x14
> +#define ICU_CLRSPI_NSR_AL	0x18
> +#define ICU_CLRSPI_NSR_AH	0x1c
> +#define ICU_INT_CFG(x)          (0x100 + 4 * (x))
> +#define   ICU_INT_ENABLE	BIT(24)
> +#define   ICU_IS_EDGE		BIT(28)
> +#define   ICU_GROUP_SHIFT	29
> +
> +/* ICU definitions */
> +#define ICU_MAX_IRQS		207
> +#define ICU_SATA0_ICU_ID	109
> +#define ICU_SATA1_ICU_ID	107
> +
> +struct mvebu_icu {
> +	struct irq_chip irq_chip;
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	struct device *dev;
> +	struct mvebu_gicp *gicp;
> +};
> +
> +struct mvebu_icu_irq_data {
> +	struct mvebu_icu *icu;
> +	unsigned int icu_group;
> +	unsigned int type;
> +};
> +
> +static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct irq_data *d = irq_get_irq_data(desc->irq);
> +	struct mvebu_icu_irq_data *icu_irqd = d->chip_data;
> +	struct mvebu_icu *icu = icu_irqd->icu;
> +	unsigned int icu_int;
> +
> +	if (msg->address_lo) {

This should probably test both _lo and _hi.

> +		/* Configure the ICU with irq number & type */
> +		icu_int = msg->data | ICU_INT_ENABLE;
> +		if (icu_irqd->type & IRQ_TYPE_EDGE_RISING)
> +			icu_int |= ICU_IS_EDGE;
> +		icu_int |= icu_irqd->icu_group << ICU_GROUP_SHIFT;
> +	} else {
> +		/* De-configure the ICU */
> +		icu_int = 0;
> +	}
> +
> +	writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq));
> +
> +	/*
> +	 * The SATA unit has 2 ports, and a dedicated ICU entry per
> +	 * port. The ahci sata driver supports only one irq interrupt
> +	 * per SATA unit. To solve this conflict, we configure the 2
> +	 * SATA wired interrupts in the south bridge into 1 GIC
> +	 * interrupt in the north bridge. Even if only a single port
> +	 * is enabled, if sata node is enabled, both interrupts are
> +	 * configured (regardless of which port is actually in use).
> +	 */
> +	if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) {
> +		writel_relaxed(icu_int,
> +			       icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID));
> +		writel_relaxed(icu_int,
> +			       icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID));
> +	}
> +}
> +
> +static int
> +mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> +			       unsigned long *hwirq, unsigned int *type)
> +{
> +	struct mvebu_icu *icu = d->host_data;
> +	unsigned int icu_group;
> +
> +	/* Check the count of the parameters in dt */
> +	if (WARN_ON(fwspec->param_count < 3)) {
> +		dev_err(icu->dev, "wrong ICU parameter count %d\n",
> +			fwspec->param_count);
> +		return -EINVAL;
> +	}
> +
> +	/* Only ICU group type is handled */
> +	icu_group = fwspec->param[0];
> +	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
> +	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
> +		dev_err(icu->dev, "wrong ICU group type %x\n", icu_group);
> +		return -EINVAL;
> +	}
> +
> +	*hwirq = fwspec->param[1];
> +	if (*hwirq < 0 || *hwirq >= ICU_MAX_IRQS) {

*hwirq is unlikely to become negative...

> +		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
> +		return -EINVAL;
> +	}
> +
> +	/* Mask the type to prevent wrong DT configuration */
> +	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}
> +
> +static int
> +mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +			   unsigned int nr_irqs, void *args)
> +{
> +	int err;
> +	unsigned long hwirq;
> +	struct irq_fwspec *fwspec = args;
> +	struct mvebu_icu *icu = platform_msi_get_host_data(domain);
> +	struct mvebu_icu_irq_data *icu_irqd;
> +
> +	icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL);
> +	if (!icu_irqd)
> +		return -ENOMEM;
> +
> +	err = mvebu_icu_irq_domain_translate(domain, fwspec, &hwirq,
> +					     &icu_irqd->type);
> +	if (err) {
> +		dev_err(icu->dev, "failed to translate ICU parameters\n");
> +		goto free_irqd;
> +	}
> +
> +	icu_irqd->icu_group = fwspec->param[0];
> +	icu_irqd->icu = icu;
> +
> +	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
> +	if (err) {
> +		dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n");
> +		goto free_irqd;
> +	}
> +
> +	/* Make sure there is no interrupt left pending by the firmware */
> +	err = irq_set_irqchip_state(virq, IRQCHIP_STATE_PENDING, false);
> +	if (err)
> +		goto free_msi;
> +
> +	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &icu->irq_chip, icu_irqd);
> +	if (err) {
> +		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
> +		goto free_msi;
> +	}

I think you may want to issue a irq_set_type here, because it is not
completely clear to me if the core code will be doing it by default for
you...

> +
> +	return 0;
> +
> +free_msi:
> +	platform_msi_domain_free(domain, virq, nr_irqs);
> +free_irqd:
> +	kfree(icu_irqd);
> +	return err;
> +}
> +
> +static void
> +mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +			  unsigned int nr_irqs)
> +{
> +	struct irq_data *d = irq_get_irq_data(virq);
> +	struct mvebu_icu_irq_data *icu_irqd = d->chip_data;
> +
> +	kfree(icu_irqd);
> +
> +	platform_msi_domain_free(domain, virq, nr_irqs);
> +}
> +
> +static const struct irq_domain_ops mvebu_icu_domain_ops = {
> +	.translate = mvebu_icu_irq_domain_translate,
> +	.alloc     = mvebu_icu_irq_domain_alloc,
> +	.free      = mvebu_icu_irq_domain_free,
> +};
> +
> +static int mvebu_icu_probe(struct platform_device *pdev)
> +{
> +	struct mvebu_icu *icu;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct platform_device *gicp_pdev;
> +	struct device_node *gicp_dn;
> +	struct resource *res;
> +	phys_addr_t setspi, clrspi;
> +	u32 i, icu_int;
> +
> +	icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu),
> +			   GFP_KERNEL);
> +	if (!icu)
> +		return -ENOMEM;
> +
> +	icu->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	icu->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(icu->base)) {
> +		dev_err(&pdev->dev, "Failed to map icu base address.\n");
> +		return PTR_ERR(icu->base);
> +	}
> +
> +	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +					    "ICU.%x",
> +					    (unsigned int)res->start);
> +	if (!icu->irq_chip.name)
> +		return -ENOMEM;
> +
> +	icu->irq_chip.irq_mask = irq_chip_mask_parent;
> +	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
> +	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
> +	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
> +#ifdef CONFIG_SMP
> +	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
> +#endif
> +
> +	/*
> +	 * We're probed after MSI domains have been resolved, so force
> +	 * resolution here.
> +	 */
> +	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, node,
> +						 DOMAIN_BUS_PLATFORM_MSI);
> +	if (!pdev->dev.msi_domain)
> +		return -EPROBE_DEFER;
> +
> +	gicp_dn = of_parse_phandle(node, "msi-parent", 0);
> +	if (!gicp_dn) {
> +		dev_err(&pdev->dev, "Missing marvell,gicp property.\n");
> +		return -ENODEV;
> +	}
> +
> +	gicp_pdev = of_find_device_by_node(gicp_dn);
> +	if (!gicp_pdev) {
> +		dev_err(&pdev->dev, "Cannot find gicp device.\n");
> +		return -ENODEV;
> +	}
> +
> +	icu->gicp = platform_get_drvdata(gicp_pdev);
> +
> +	/* Set Clear/Set ICU SPI message address in AP */
> +	setspi = mvebu_gicp_setspi_phys_addr(icu->gicp);


I must say that I find this quite horrible. The idea of digging into the
internals of another driver and forcing it to blindly dereference a
pointer feels just wrong.

Instead, why don't you directly pass the device node, and kindly ask the
GICP driver to give you the two addresses? Something along the lines of:

	err = mvebu_gicp_get_doorbells(gicp_dn, &setspi, &clrspi);
	if (err)
		[...]

which at least gives a the GICP driver chance to check that this is
something it knows about. And you can then drop the icu->gicp field.

> +	writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH);
> +	writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL);
> +	clrspi = mvebu_gicp_clrspi_phys_addr(icu->gicp);
> +	writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH);
> +	writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL);
> +
> +	/*
> +	 * Clean all ICU interrupts with type SPI_NSR, required to
> +	 * avoid unpredictable SPI assignments done by firmware.
> +	 */
> +	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
> +		icu_int = readl(icu->base + ICU_INT_CFG(i));
> +		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
> +			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
> +	}

I had questions about the safety of this in a previous review. Do you
have any update? Also, shouldn't you check that same thing in the
translate callback (so that you detect clashes between firmware and DT)?

> +
> +	icu->domain =
> +		platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS,
> +						  mvebu_icu_write_msg,
> +						  &mvebu_icu_domain_ops, icu);
> +	if (!icu->domain) {
> +		dev_err(&pdev->dev, "Failed to create ICU domain\n");
> +		put_device(&gicp_pdev->dev);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mvebu_icu_of_match[] = {
> +	{ .compatible = "marvell,cp110-icu", },
> +	{},
> +};
> +
> +static struct platform_driver mvebu_icu_driver = {
> +	.probe  = mvebu_icu_probe,
> +	.driver = {
> +		.name = "mvebu-icu",
> +		.of_match_table = mvebu_icu_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_icu_driver);
> diff --git a/include/dt-bindings/interrupt-controller/mvebu-icu.h b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> new file mode 100644
> index 0000000..8249558
> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/mvebu-icu.h
> @@ -0,0 +1,15 @@
> +/*
> + * This header provides constants for the MVEBU ICU driver.
> + */
> +
> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MVEBU_ICU_H
> +
> +/* interrupt specifier cell 0 */
> +
> +#define ICU_GRP_NSR		0x0
> +#define ICU_GRP_SR		0x1
> +#define ICU_GRP_SEI		0x4
> +#define ICU_GRP_REI		0x5
> +
> +#endif
> 

Otherwise looking pretty neat.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...