Re: [PATCH v2 4/5] PCI: iproc: Add iProc PCIe MSI support
From: Marc Zyngier
Date: Wed Nov 25 2015 - 12:36:55 EST
On Tue, 24 Nov 2015 15:04:53 -0800
Ray Jui <rjui@xxxxxxxxxxxx> wrote:
> This patch adds PCIe MSI support for both PAXB and PAXC interfaces on
> all iProc based platforms. The patch follows the latest trend in the
> kernel to use MSI domain based implementation
>
That's a pretty useless comment. The general trend in the kernel is to
use the most appropriate infrastructure.
> This iProc event queue based MSI support should not be used with newer
> platforms with integrated MSI support in the GIC (e.g., giv2m or
> gicv3-its)
>
I'd be more interested in some documentation explaining how the HW
works, how the various data structures are updated, and when.
> Signed-off-by: Ray Jui <rjui@xxxxxxxxxxxx>
> Reviewed-by: Anup Patel <anup.patel@xxxxxxxxxxxx>
> Reviewed-by: Vikram Prakash <vikramp@xxxxxxxxxxxx>
> Reviewed-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> ---
> drivers/pci/host/Kconfig | 9 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-iproc-msi.c | 662 ++++++++++++++++++++++++++++++++++++++
> drivers/pci/host/pcie-iproc.c | 26 ++
> drivers/pci/host/pcie-iproc.h | 21 +-
> 5 files changed, 717 insertions(+), 2 deletions(-)
> create mode 100644 drivers/pci/host/pcie-iproc-msi.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index f131ba9..972e906 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -126,6 +126,15 @@ config PCIE_IPROC
> iProc family of SoCs. An appropriate bus interface driver also needs
> to be enabled
>
> +config PCIE_IPROC_MSI
> + bool "Broadcom iProc PCIe MSI support"
> + depends on ARCH_BCM_IPROC && PCI_MSI
> + select PCI_MSI_IRQ_DOMAIN
> + default ARCH_BCM_IPROC
> + help
> + Say Y here if you want to enable MSI support for Broadcom's iProc
> + PCIe controller
> +
> config PCIE_IPROC_PLATFORM
> tristate "Broadcom iProc PCIe platform bus driver"
> depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 9d4d3c6..0e4e95e 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
> obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
> +obj-$(CONFIG_PCIE_IPROC_MSI) += pcie-iproc-msi.o
> obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
> obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
> diff --git a/drivers/pci/host/pcie-iproc-msi.c b/drivers/pci/host/pcie-iproc-msi.c
> new file mode 100644
> index 0000000..afc54c2
> --- /dev/null
> +++ b/drivers/pci/host/pcie-iproc-msi.c
> @@ -0,0 +1,662 @@
> +/*
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +
> +#include "pcie-iproc.h"
> +
> +#define IPROC_MSI_INTR_EN_SHIFT 11
> +#define IPROC_MSI_INTR_EN BIT(IPROC_MSI_INTR_EN_SHIFT)
> +#define IPROC_MSI_INT_N_EVENT_SHIFT 1
> +#define IPROC_MSI_INT_N_EVENT BIT(IPROC_MSI_INT_N_EVENT_SHIFT)
> +#define IPROC_MSI_EQ_EN_SHIFT 0
> +#define IPROC_MSI_EQ_EN BIT(IPROC_MSI_EQ_EN_SHIFT)
> +
> +#define IPROC_MSI_EQ_MASK 0x3f
> +
> +/* max number of GIC interrupts */
> +#define NR_HW_IRQS 6
> +
> +/* number of entries in each event queue */
> +#define EQ_LEN 64
> +
> +/* size of each event queue memory region */
> +#define EQ_MEM_REGION_SIZE SZ_4K
> +
> +/* size of each MSI message memory region */
> +#define MSI_MEM_REGION_SIZE SZ_4K
> +
> +enum iproc_msi_reg {
> + IPROC_MSI_EQ_PAGE = 0,
> + IPROC_MSI_EQ_PAGE_UPPER,
> + IPROC_MSI_PAGE,
> + IPROC_MSI_PAGE_UPPER,
> + IPROC_MSI_CTRL,
> + IPROC_MSI_EQ_HEAD,
> + IPROC_MSI_EQ_TAIL,
> + IPROC_MSI_INTS_EN,
> + IPROC_MSI_REG_SIZE,
> +};
> +
> +struct iproc_msi;
> +
> +/**
> + * iProc MSI group
> + *
> + * One MSI group is allocated per GIC interrupt, serviced by one iProc MSI
> + * event queue
> + *
> + * @msi: pointer to iProc MSI data
> + * @gic_irq: GIC interrupt
> + * @eq: Event queue number
> + */
> +struct iproc_msi_grp {
> + struct iproc_msi *msi;
> + int gic_irq;
> + unsigned int eq;
> +};
> +
> +/**
> + * iProc event queue based MSI
> + *
> + * Only meant to be used on platforms without MSI support integrated into the
> + * GIC
> + *
> + * @pcie: pointer to iProc PCIe data
> + * @reg_offsets: MSI register offsets
> + * @grps: MSI groups
> + * @nr_irqs: number of total interrupts connected to GIC
> + * @nr_cpus: number of toal CPUs
> + * @has_inten_reg: indicates the MSI interrupt enable register needs to be
> + * set explicitly (required for some legacy platforms)
> + * @bitmap: MSI vector bitmap
> + * @bitmap_lock: lock to protect access to the MSI bitmap
> + * @nr_msi_vecs: total number of MSI vectors
> + * @inner_domain: inner IRQ domain
> + * @msi_domain: MSI IRQ domain
> + * @nr_eq_region: required number of 4K aligned memory region for MSI event
> + * queues
> + * @nr_msi_region: required number of 4K aligned memory region for MSI posted
> + * writes
> + * @eq_cpu: pointer to allocated memory region for MSI event queues
> + * @eq_dma: DMA address of MSI event queues
> + * @msi_cpu: pointer to allocated memory region for MSI posted writes
> + * @msi_dma: DMA address of MSI posted writes
> + */
> +struct iproc_msi {
> + struct iproc_pcie *pcie;
> + const u16 (*reg_offsets)[IPROC_MSI_REG_SIZE];
> + struct iproc_msi_grp *grps;
> + int nr_irqs;
> + int nr_cpus;
> + bool has_inten_reg;
> + unsigned long *bitmap;
> + struct mutex bitmap_lock;
> + unsigned int nr_msi_vecs;
> + struct irq_domain *inner_domain;
> + struct irq_domain *msi_domain;
> + unsigned int nr_eq_region;
> + unsigned int nr_msi_region;
> + void *eq_cpu;
> + dma_addr_t eq_dma;
> + void *msi_cpu;
> + dma_addr_t msi_dma;
> +};
> +
> +static const u16 iproc_msi_reg_paxb[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x210, 0x250, 0x254, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x214, 0x258, 0x25c, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x218, 0x260, 0x264, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x21c, 0x268, 0x26c, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x220, 0x270, 0x274, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x224, 0x278, 0x27c, 0x208 },
> +};
> +
> +static const u16 iproc_msi_reg_paxc[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
> + { 0xc00, 0xc04, 0xc08, 0xc0c, 0xc40, 0xc50, 0xc60 },
> + { 0xc10, 0xc14, 0xc18, 0xc1c, 0xc44, 0xc54, 0xc64 },
> + { 0xc20, 0xc24, 0xc28, 0xc2c, 0xc48, 0xc58, 0xc68 },
> + { 0xc30, 0xc34, 0xc38, 0xc3c, 0xc4c, 0xc5c, 0xc6c },
> +};
> +
> +static inline u32 iproc_msi_read_reg(struct iproc_msi *msi,
> + enum iproc_msi_reg reg,
> + unsigned int eq)
> +{
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + return readl_relaxed(pcie->base + msi->reg_offsets[eq][reg]);
> +}
> +
> +static inline void iproc_msi_write_reg(struct iproc_msi *msi,
> + enum iproc_msi_reg reg,
> + int eq, u32 val)
> +{
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + writel_relaxed(val, pcie->base + msi->reg_offsets[eq][reg]);
> +}
> +
> +static inline bool iproc_msi_has_mult_regions(struct iproc_msi *msi)
> +{
> + return ((msi->nr_msi_region > 1) ? true : false);
return msi->nr_msi_region > 1;
> +}
> +
> +static inline bool iproc_eq_has_mult_regions(struct iproc_msi *msi)
> +{
> + return ((msi->nr_eq_region > 1) ? true : false);
return msi->nr_eq_region > 1;
> +}
> +
> +static struct irq_chip iproc_msi_irq_chip = {
> + .name = "iProc-MSI",
> +};
> +
> +static struct msi_domain_info iproc_msi_domain_info = {
> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSIX,
> + .chip = &iproc_msi_irq_chip,
> +};
> +
> +/*
> + * In iProc PCIe core, each MSI group is serviced by a GIC interrupt and a
> + * dedicated event queue. Each MSI group can support up to 64 MSI vectors
> + *
> + * The number of MSI groups varies between different iProc SoCs. The total
> + * number of CPU cores also varies. To support MSI IRQ affinity, we
> + * distribute GIC interrupts across all available CPUs. MSI vector is moved
> + * from one GIC interrupt to another to steer to the target CPU
> + *
> + * Assuming:
> + * - the number of MSI groups is M
> + * - the number of CPU cores is N
> + * - M is always a multiple of N
How do you enforce that last condition?
> + *
> + * Total number of raw MSI vectors = M * 64
> + * Total number of supported MSI vectors = (M * 64) / N
> + */
> +static inline int hwirq_to_cpu(struct iproc_msi *msi, unsigned long hwirq)
> +{
> + return (hwirq % msi->nr_cpus);
> +}
> +
> +static inline unsigned long hwirq_to_canonical_hwirq(struct iproc_msi *msi,
> + unsigned long hwirq)
> +{
> + return (hwirq - hwirq_to_cpu(msi, hwirq));
> +}
> +
> +static int iproc_msi_irq_set_affinity(struct irq_data *data,
> + const struct cpumask *mask, bool force)
> +{
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + int target_cpu = cpumask_first(mask);
> + int curr_cpu;
> +
> + curr_cpu = hwirq_to_cpu(msi, data->hwirq);
> + if (curr_cpu == target_cpu)
> + return IRQ_SET_MASK_OK_DONE;
> +
> + /* steer MSI to the target CPU */
> + data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +
> + return IRQ_SET_MASK_OK;
> +}
> +
> +static inline u32 hwirq_to_group(struct iproc_msi *msi, unsigned long hwirq)
> +{
> + return (hwirq % msi->nr_irqs);
> +}
> +
> +static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
> + struct msi_msg *msg)
> +{
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + dma_addr_t addr;
> + unsigned int mul;
> +
> + if (iproc_msi_has_mult_regions(msi))
> + mul = MSI_MEM_REGION_SIZE;
> + else
> + mul = sizeof(u32);
Since this is the only use of that function, why don't you have it to
directly return the right multiplier?
> +
> + addr = msi->msi_dma + hwirq_to_group(msi, data->hwirq) * mul;
> + msg->address_lo = lower_32_bits(addr);
> + msg->address_hi = upper_32_bits(addr);
> + msg->data = data->hwirq;
> +}
> +
> +static struct irq_chip iproc_msi_bottom_irq_chip = {
> + .name = "MSI",
> + .irq_set_affinity = iproc_msi_irq_set_affinity,
> + .irq_compose_msi_msg = iproc_msi_irq_compose_msi_msg,
> +};
> +
> +static int iproc_msi_irq_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *args)
> +{
> + struct iproc_msi *msi = domain->host_data;
> + int hwirq;
> +
> + mutex_lock(&msi->bitmap_lock);
> +
> + /* allocate 'nr_cpus' number of MSI vectors each time */
> + hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
> + msi->nr_cpus, 0);
> + if (hwirq < msi->nr_msi_vecs)
> + bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
> + else
> + return -ENOSPC;
Deadlock, here we come...
> +
> + mutex_unlock(&msi->bitmap_lock);
> +
> + irq_domain_set_info(domain, virq, hwirq, &iproc_msi_bottom_irq_chip,
> + domain->host_data, handle_simple_irq, NULL, NULL);
> +
> + return 0;
> +}
> +
> +static void iproc_msi_irq_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + unsigned int hwirq;
> +
> + mutex_lock(&msi->bitmap_lock);
> +
> + hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
> + bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
> +
> + mutex_unlock(&msi->bitmap_lock);
> +
> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +}
> +
> +static const struct irq_domain_ops msi_domain_ops = {
> + .alloc = iproc_msi_irq_domain_alloc,
> + .free = iproc_msi_irq_domain_free,
> +};
> +
> +static inline u32 decode_msi_hwirq(struct iproc_msi *msi, u32 eq, u32 head)
> +{
> + u32 *msg, hwirq;
> + unsigned int offs;
> +
> + if (iproc_eq_has_mult_regions(msi))
> + offs = eq * EQ_MEM_REGION_SIZE;
> + else
> + offs = eq * EQ_LEN * sizeof(u32);
Same here.
> +
> + offs += head * sizeof(u32);
> + msg = (u32 *)(msi->eq_cpu + offs);
If that's the only place where you dereference msi->eq_cpu, why doesn't
it have the right type?
> + hwirq = *msg & IPROC_MSI_EQ_MASK;
> +
> + /*
> + * Since we have multiple hwirq mapped to a single MSI vector,
> + * now we need to derive the hwirq at CPU0. It can then be used to
> + * mapped back to virq
> + */
> + return hwirq_to_canonical_hwirq(msi, hwirq);
> +}
> +
> +static void iproc_msi_handler(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct iproc_msi_grp *grp;
> + struct iproc_msi *msi;
> + struct iproc_pcie *pcie;
> + u32 eq, head, tail, nr_events;
> + unsigned long hwirq;
> + int virq;
> +
> + chained_irq_enter(chip, desc);
> +
> + grp = irq_desc_get_handler_data(desc);
> + msi = grp->msi;
> + pcie = msi->pcie;
> + eq = grp->eq;
> +
> + /*
> + * iProc MSI event queue is tracked by head and tail pointers. Head
> + * pointer indicates the next entry to be processed by SW in the
> + * queue. Entries between head and tail pointers contain valid MSI
> + * data to be processed
> + */
> + head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD,
> + eq) & IPROC_MSI_EQ_MASK;
> + do {
> + tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL,
> + eq) & IPROC_MSI_EQ_MASK;
> +
> + /*
> + * Figure out total number of events (MSI data) to be
> + * processed
> + */
> + nr_events = (tail < head) ?
> + (EQ_LEN - (head - tail)) : (tail - head);
> + if (!nr_events)
> + break;
> +
> + /* process all outstanding events */
> + while (nr_events--) {
> + hwirq = decode_msi_hwirq(msi, eq, head);
> + virq = irq_find_mapping(msi->inner_domain, hwirq);
> + generic_handle_irq(virq);
> +
> + head++;
> + head %= EQ_LEN;
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
> + }
Wouldn't it be better to process all nr_events and only update head
once?
> +
> + /*
> + * Now go read the tail pointer again to see if there are new
> + * oustanding events that came in during the above window
> + */
> + } while (true);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static void iproc_msi_enable(struct iproc_msi *msi)
> +{
> + int i, eq;
> + u32 val;
> +
> + /* program memory region for each event queue */
> + for (i = 0; i < msi->nr_eq_region; i++) {
> + dma_addr_t addr = msi->eq_dma + (i * EQ_MEM_REGION_SIZE);
> +
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE, i,
> + lower_32_bits(addr));
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE_UPPER, i,
> + upper_32_bits(addr));
> + }
> +
> + /* program memory region for MSI posted writes */
> + for (i = 0; i < msi->nr_msi_region; i++) {
> + dma_addr_t addr = msi->msi_dma + (i * MSI_MEM_REGION_SIZE);
> +
> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE, i,
> + lower_32_bits(addr));
> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE_UPPER, i,
> + upper_32_bits(addr));
> + }
> +
> + for (eq = 0; eq < msi->nr_irqs; eq++) {
> + /* enable MSI event queue */
> + val = IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
> + IPROC_MSI_EQ_EN;
> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
> +
> + /*
> + * Some legacy platforms require the MSI interrupt enable
> + * register to be set explicitly
> + */
> + if (msi->has_inten_reg) {
> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
> + val |= BIT(eq);
> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
> + }
> + }
> +}
> +
> +static void iproc_msi_disable(struct iproc_msi *msi)
> +{
> + u32 eq, val;
> +
> + for (eq = 0; eq < msi->nr_irqs; eq++) {
> + if (msi->has_inten_reg) {
> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
> + val &= ~BIT(eq);
> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
> + }
> +
> + val = iproc_msi_read_reg(msi, IPROC_MSI_CTRL, eq);
> + val &= ~(IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
> + IPROC_MSI_EQ_EN);
> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
> + }
> +}
> +
> +static int iproc_msi_alloc_domains(struct device_node *node,
> + struct iproc_msi *msi)
> +{
> + msi->inner_domain = irq_domain_add_linear(NULL, msi->nr_msi_vecs,
> + &msi_domain_ops, msi);
> + if (!msi->inner_domain)
> + return -ENOMEM;
> +
> + msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
> + &iproc_msi_domain_info,
> + msi->inner_domain);
> + if (!msi->msi_domain) {
> + irq_domain_remove(msi->inner_domain);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void iproc_msi_free_domains(struct iproc_msi *msi)
> +{
> + if (msi->msi_domain)
> + irq_domain_remove(msi->msi_domain);
> +
> + if (msi->inner_domain)
> + irq_domain_remove(msi->inner_domain);
> +}
> +
> +static int iproc_msi_irq_setup(struct iproc_msi *msi, unsigned int cpu)
> +{
> + int i, ret;
> + cpumask_var_t mask;
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + iproc_msi_handler,
> + &msi->grps[i]);
> + /* dedicate GIC interrupt to each CPU core */
> + if (alloc_cpumask_var(&mask, GFP_KERNEL)) {
> + cpumask_clear(mask);
> + cpumask_set_cpu(cpu, mask);
> + ret = irq_set_affinity(msi->grps[i].gic_irq, mask);
> + if (ret)
> + dev_err(pcie->dev,
> + "failed to set affinity for IRQ%d\n",
> + msi->grps[i].gic_irq);
> + free_cpumask_var(mask);
> + } else {
> + dev_err(pcie->dev, "failed to alloc CPU mask\n");
> + ret = -EINVAL;
> + }
> +
> + if (ret) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + NULL, NULL);
> + return ret;
What happens to interrupts you've already configured? I'd expect a full
rollback.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void iproc_msi_irq_free(struct iproc_msi *msi, unsigned int cpu)
> +{
> + int i;
> +
> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + NULL, NULL);
> + }
> +}
> +
> +int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node)
> +{
> + struct iproc_msi *msi;
> + int i, ret;
> + unsigned int cpu;
> +
> + if (!of_device_is_compatible(node, "brcm,iproc-msi"))
> + return -ENODEV;
> +
> + if (!of_find_property(node, "msi-controller", NULL))
> + return -ENODEV;
> +
> + if (pcie->msi)
> + return -EBUSY;
> +
> + msi = devm_kzalloc(pcie->dev, sizeof(*msi), GFP_KERNEL);
> + if (!msi)
> + return -ENOMEM;
> +
> + msi->pcie = pcie;
> + pcie->msi = msi;
> + mutex_init(&msi->bitmap_lock);
> + msi->nr_cpus = num_possible_cpus();
> +
> + switch (pcie->type) {
> + case IPROC_PCIE_PAXB:
> + msi->reg_offsets = iproc_msi_reg_paxb;
> + break;
> + case IPROC_PCIE_PAXC:
> + msi->reg_offsets = iproc_msi_reg_paxc;
> + break;
> + default:
> + dev_err(pcie->dev, "incompatible iProc PCIe interface\n");
> + return -EINVAL;
> + }
> +
> + if (of_find_property(node, "brcm,pcie-msi-inten", NULL))
> + msi->has_inten_reg = true;
> +
> + ret = of_property_read_u32(node, "brcm,num-eq-region",
> + &msi->nr_eq_region);
> + if (ret || !msi->nr_eq_region)
> + msi->nr_eq_region = 1;
> +
> + ret = of_property_read_u32(node, "brcm,num-msi-msg-region",
> + &msi->nr_msi_region);
> + if (ret || !msi->nr_msi_region)
> + msi->nr_msi_region = 1;
> +
> + msi->nr_irqs = of_irq_count(node);
> + if (!msi->nr_irqs) {
> + dev_err(pcie->dev, "found no MSI GIC interrupt\n");
> + return -ENODEV;
> + }
> + if (msi->nr_irqs > NR_HW_IRQS) {
> + dev_warn(pcie->dev, "too many MSI GIC interrupts defined %d\n",
> + msi->nr_irqs);
> + msi->nr_irqs = NR_HW_IRQS;
> + }
> +
> + msi->nr_msi_vecs = msi->nr_irqs * EQ_LEN;
> + msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs),
> + sizeof(*msi->bitmap), GFP_KERNEL);
> + if (!msi->bitmap)
> + return -ENOMEM;
> +
> + msi->grps = devm_kcalloc(pcie->dev, msi->nr_irqs, sizeof(*msi->grps),
> + GFP_KERNEL);
> + if (!msi->grps)
> + return -ENOMEM;
> +
> + for (i = 0; i < msi->nr_irqs; i++) {
> + unsigned int irq = irq_of_parse_and_map(node, i);
> +
> + if (!irq) {
> + dev_err(pcie->dev, "unable to parse/map interrupt\n");
> + return -ENODEV;
> + }
> + msi->grps[i].gic_irq = irq;
> + msi->grps[i].msi = msi;
> + msi->grps[i].eq = i;
> + }
> +
> + /* reserve memory for MSI event queue */
> + msi->eq_cpu = dma_alloc_coherent(pcie->dev,
> + msi->nr_eq_region * EQ_MEM_REGION_SIZE,
> + &msi->eq_dma, GFP_KERNEL);
Do you need to zero that memory? Or is the HW happy with whatever will
be there?
> + if (!msi->eq_cpu)
> + return -ENOMEM;
> +
> + /* reserve memory for MSI posted writes */
> + msi->msi_cpu = dma_alloc_coherent(pcie->dev,
> + msi->nr_msi_region * MSI_MEM_REGION_SIZE,
> + &msi->msi_dma, GFP_KERNEL);
Same here. Also, what is the exact purpose of that memory? You have a
coherent mapping with the CPU, but you never read from it. So what's
the point?
> + if (!msi->msi_cpu) {
> + ret = -ENOMEM;
> + goto free_eq_dma;
> + }
> +
> + ret = iproc_msi_alloc_domains(node, msi);
> + if (ret) {
> + dev_err(pcie->dev, "failed to create MSI domains\n");
> + goto free_msi_dma;
> + }
> +
> + for_each_online_cpu(cpu) {
> + ret = iproc_msi_irq_setup(msi, cpu);
> + if (ret)
> + goto free_msi_irq;
> + }
> +
> + iproc_msi_enable(msi);
> +
> + return 0;
> +
> +free_msi_irq:
> + for_each_online_cpu(cpu)
> + iproc_msi_irq_free(msi, cpu);
> + iproc_msi_free_domains(msi);
> +
> +free_msi_dma:
> + dma_free_coherent(pcie->dev, msi->nr_msi_region * MSI_MEM_REGION_SIZE,
> + msi->msi_cpu, msi->msi_dma);
> +free_eq_dma:
> + dma_free_coherent(pcie->dev, msi->nr_eq_region * EQ_MEM_REGION_SIZE,
> + msi->eq_cpu, msi->eq_dma);
> + pcie->msi = NULL;
> + return ret;
> +}
> +EXPORT_SYMBOL(iproc_msi_init);
[...]
Thanks,
M.
--
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/