* [PATCH 1/4] Split virq setup logic out into irq_setup_virq()
@ 2007-06-04 12:59 Michael Ellerman
2007-06-04 13:00 ` [PATCH 2/4] Add irq_create_direct_mapping() Michael Ellerman
` (3 more replies)
0 siblings, 4 replies; 31+ messages in thread
From: Michael Ellerman @ 2007-06-04 12:59 UTC (permalink / raw)
To: linuxppc-dev
A future patch will need the logic at the end of irq_create_mapping()
which setups a virq and installs it in the irq_map. So split it out
into a new function irq_setup_virq().
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/kernel/irq.c | 32 +++++++++++++++++++++-----------
1 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 42c8ed6..9bafc88 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -591,6 +591,25 @@ static void irq_radix_rdunlock(unsigned long flags)
local_irq_restore(flags);
}
+static int irq_setup_virq(struct irq_host *host, unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ /* Clear IRQ_NOREQUEST flag */
+ get_irq_desc(virq)->status &= ~IRQ_NOREQUEST;
+
+ /* map it */
+ smp_wmb();
+ irq_map[virq].hwirq = hwirq;
+ smp_mb();
+
+ if (host->ops->map(host, virq, hwirq)) {
+ pr_debug("irq: -> mapping failed, freeing\n");
+ irq_free_virt(virq, 1);
+ return -1;
+ }
+
+ return 0;
+}
unsigned int irq_create_mapping(struct irq_host *host,
irq_hw_number_t hwirq)
@@ -639,18 +658,9 @@ unsigned int irq_create_mapping(struct irq_host *host,
}
pr_debug("irq: -> obtained virq %d\n", virq);
- /* Clear IRQ_NOREQUEST flag */
- get_irq_desc(virq)->status &= ~IRQ_NOREQUEST;
-
- /* map it */
- smp_wmb();
- irq_map[virq].hwirq = hwirq;
- smp_mb();
- if (host->ops->map(host, virq, hwirq)) {
- pr_debug("irq: -> mapping failed, freeing\n");
- irq_free_virt(virq, 1);
+ if (irq_setup_virq(host, virq, hwirq))
return NO_IRQ;
- }
+
return virq;
}
EXPORT_SYMBOL_GPL(irq_create_mapping);
--
1.5.1.3.g7a33b-dirty
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/4] Add irq_create_direct_mapping()
2007-06-04 12:59 [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Michael Ellerman
@ 2007-06-04 13:00 ` Michael Ellerman
2007-06-04 22:44 ` Benjamin Herrenschmidt
2007-06-04 13:00 ` [PATCH 3/4] Add for_each_compatible_node() Michael Ellerman
` (2 subsequent siblings)
3 siblings, 1 reply; 31+ messages in thread
From: Michael Ellerman @ 2007-06-04 13:00 UTC (permalink / raw)
To: linuxppc-dev
This patch adds irq_create_direct_mapping(). This routine is
an alternative to irq_create_mapping(), for irq controllers that
can use linux virq numbers directly as hardware numbers.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
arch/powerpc/kernel/irq.c | 24 ++++++++++++++++++++++++
include/asm-powerpc/irq.h | 9 +++++++++
2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 9bafc88..fdb3b00 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -611,6 +611,30 @@ static int irq_setup_virq(struct irq_host *host, unsigned int virq,
return 0;
}
+unsigned int irq_create_direct_mapping(struct irq_host *host)
+{
+ unsigned int virq;
+
+ if (host == NULL)
+ host = irq_default_host;
+
+ BUG_ON(host == NULL);
+ WARN_ON(host->revmap_type != IRQ_HOST_MAP_NOMAP);
+
+ virq = irq_alloc_virt(host, 1, 0);
+ if (virq == NO_IRQ) {
+ pr_debug("irq: create_direct virq allocation failed\n");
+ return NO_IRQ;
+ }
+
+ pr_debug("irq: create_direct obtained virq %d\n", virq);
+
+ if (irq_setup_virq(host, virq, virq))
+ return NO_IRQ;
+
+ return virq;
+}
+
unsigned int irq_create_mapping(struct irq_host *host,
irq_hw_number_t hwirq)
{
diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h
index 4734cc1..8384e3f 100644
--- a/include/asm-powerpc/irq.h
+++ b/include/asm-powerpc/irq.h
@@ -226,6 +226,15 @@ extern void irq_dispose_mapping(unsigned int virq);
extern unsigned int irq_find_mapping(struct irq_host *host,
irq_hw_number_t hwirq);
+/**
+ * irq_create_direct_mapping - Allocate a virq for direct mapping
+ * @host: host to allocate the virq for or NULL for default host
+ *
+ * This routine is used for irq controllers which can choose the hardware
+ * interrupt numbers they generate. In such a case it's simplest to use
+ * the linux virq as the hardware interrupt number.
+ */
+extern unsigned int irq_create_direct_mapping(struct irq_host *host);
/**
* irq_radix_revmap - Find a linux virq from a hw irq number.
--
1.5.1.3.g7a33b-dirty
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/4] Add for_each_compatible_node()
2007-06-04 12:59 [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Michael Ellerman
2007-06-04 13:00 ` [PATCH 2/4] Add irq_create_direct_mapping() Michael Ellerman
@ 2007-06-04 13:00 ` Michael Ellerman
2007-06-04 22:44 ` Benjamin Herrenschmidt
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
2007-06-04 22:43 ` [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Benjamin Herrenschmidt
3 siblings, 1 reply; 31+ messages in thread
From: Michael Ellerman @ 2007-06-04 13:00 UTC (permalink / raw)
To: linuxppc-dev
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
include/asm-powerpc/prom.h | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
index 6845af9..1122a92 100644
--- a/include/asm-powerpc/prom.h
+++ b/include/asm-powerpc/prom.h
@@ -124,6 +124,9 @@ extern struct device_node *of_find_node_by_type(struct device_node *from,
dn = of_find_node_by_type(dn, type))
extern struct device_node *of_find_compatible_node(struct device_node *from,
const char *type, const char *compat);
+#define for_each_compatible_node(dn, type, compatible) \
+ for (dn = of_find_compatible_node(NULL, type, compatible); dn; \
+ dn = of_find_compatible_node(dn, type, compatible))
extern struct device_node *of_find_node_by_path(const char *path);
extern struct device_node *of_find_node_by_phandle(phandle handle);
extern struct device_node *of_find_all_nodes(struct device_node *prev);
--
1.5.1.3.g7a33b-dirty
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 12:59 [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Michael Ellerman
2007-06-04 13:00 ` [PATCH 2/4] Add irq_create_direct_mapping() Michael Ellerman
2007-06-04 13:00 ` [PATCH 3/4] Add for_each_compatible_node() Michael Ellerman
@ 2007-06-04 13:00 ` Michael Ellerman
2007-06-04 17:09 ` Arnd Bergmann
` (4 more replies)
2007-06-04 22:43 ` [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Benjamin Herrenschmidt
3 siblings, 5 replies; 31+ messages in thread
From: Michael Ellerman @ 2007-06-04 13:00 UTC (permalink / raw)
To: linuxppc-dev
This patch adds support for the setup and decoding of MSIs
on Axon-based Cell systems.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
This still needs a bit of cleanup, but sending now for an early review.
arch/powerpc/platforms/cell/Makefile | 2 +
arch/powerpc/platforms/cell/axon_msi.c | 412 ++++++++++++++++++++++++++++++++
2 files changed, 414 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/cell/Makefile b/arch/powerpc/platforms/cell/Makefile
index 869af89..6615d40 100644
--- a/arch/powerpc/platforms/cell/Makefile
+++ b/arch/powerpc/platforms/cell/Makefile
@@ -23,3 +23,5 @@ obj-$(CONFIG_SPU_BASE) += spu_callbacks.o spu_base.o \
$(spu-priv1-y) \
$(spu-manage-y) \
spufs/
+
+obj-$(CONFIG_PCI_MSI) += axon_msi.o
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
new file mode 100644
index 0000000..2709853
--- /dev/null
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -0,0 +1,412 @@
+/*
+ * Copyright 2007, Michael Ellerman, IBM 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; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/msi.h>
+#include <linux/reboot.h>
+
+#include <asm/dcr.h>
+#include <asm/machdep.h>
+#include <asm/prom.h>
+
+
+/*
+ * MSIC Control Register
+ */
+#define MSIC_CTRL_REG_ADDR 0x6F
+
+/* Flags */
+#define MSIC_ENABLE 0x0001 /* Bit 31 */
+#define MSIC_FIFO_FULL_ENABLE 0x0002 /* Bit 30 */
+#define MSIC_IRQ_ENABLE 0x0008 /* Bit 28 */
+#define MSIC_FULL_STOP_ENABLE 0x0010 /* Bit 27 */
+
+/* Size configuration constants */
+#define MSIC_SIZE_MASK 0x0180 /* Bits 22:23 */
+#define MSIC_SIZE_SHIFT (7)
+#define MSIC_SIZE_32K 0x0
+#define MSIC_SIZE_64K 0x1
+#define MSIC_SIZE_128K 0x2
+#define MSIC_SIZE_256K 0x3
+
+/* The size we're actually using */
+#define MSIC_FIFO_SIZE MSIC_SIZE_32K
+
+/* Different representations of the fifo size */
+#define MSIC_FIFO_SHIFT (MSIC_FIFO_SIZE + 0xF)
+#define MSIC_FIFO_BYTES (1 << MSIC_FIFO_SHIFT)
+#define MSIC_FIFO_MASK (MSIC_FIFO_BYTES - 1)
+#define MSIC_FIFO_ORDER max(MSIC_FIFO_SHIFT - PAGE_SHIFT, 0)
+
+/*
+ * MSIC Base Address registers (FIFO location)
+ */
+#define MSIC_BASE_ADDR_HI_REG 0x72
+#define MSIC_BASE_ADDR_LO_REG 0x73
+
+#define MSIC_READ_OFFSET_REG 0x74
+#define MSIC_WRITE_OFFSET_REG 0x75
+
+#define MSIC_DCR_BASE MSIC_CTRL_REG_ADDR
+#define MSIC_DCR_SIZE (MSIC_WRITE_OFFSET_REG - MSIC_CTRL_REG_ADDR)
+
+
+struct axon_msic {
+ struct device_node *dn;
+ struct irq_host *irq_host;
+ void *fifo;
+ dcr_host_t dcr_host;
+ struct list_head list;
+ u32 read_offset;
+};
+
+
+static LIST_HEAD(axon_msic_list);
+
+static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
+{
+ struct axon_msic *msic = get_irq_data(irq);
+ u32 write_offset, msi;
+ unsigned int cirq;
+
+ write_offset = dcr_read(msic->dcr_host, MSIC_WRITE_OFFSET_REG);
+ pr_debug("axon_msi: original write_offset 0x%x\n", write_offset);
+
+ /* write_offset doesn't wrap properly, so we have to mask it */
+ write_offset &= MSIC_FIFO_MASK;
+
+ while (msic->read_offset != write_offset) {
+ msi = le32_to_cpup((__le32*)(msic->fifo + msic->read_offset));
+ msi &= 0xFFFF;
+
+ pr_debug("axon_msi: woff %x roff %x @ %p msi %x msi@0 %x\n",
+ write_offset, msic->read_offset,
+ msic->fifo + msic->read_offset, msi, *(u32*)msic->fifo);
+
+ msic->read_offset += 0x10;
+ msic->read_offset &= MSIC_FIFO_MASK;
+
+ cirq = irq_linear_revmap(msic->irq_host, msi);
+ if (cirq != NO_IRQ)
+ generic_handle_irq(cirq);
+ else
+ pr_debug("axon_msi: mapped to NO_IRQ!\n");
+ }
+
+ dcr_write(msic->dcr_host, MSIC_READ_OFFSET_REG, msic->read_offset);
+
+ desc->chip->eoi(irq);
+}
+
+static struct axon_msic *find_msi_translator(struct pci_dev *dev)
+{
+ struct irq_host *irq_host;
+ struct device_node *np, *tmp;
+ const phandle *ph;
+
+ np = pci_device_to_OF_node(dev);
+ if (!np) {
+ dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
+ return NULL;
+ }
+
+ for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp) {
+ ph = of_get_property(np, "msi-translator", NULL);
+ if (ph)
+ break;
+ }
+
+ if (!ph) {
+ dev_dbg(&dev->dev, "axon_msi: no msi-translator found\n");
+ goto out_error;
+ }
+
+ tmp = np;
+ np = of_find_node_by_phandle(*ph);
+ if (!np) {
+ dev_dbg(&dev->dev, "axon_msi: invalid msi-translator found\n");
+ goto out_error;
+ }
+
+ irq_host = irq_find_host(np);
+ if (!irq_host) {
+ dev_dbg(&dev->dev, "axon_msi: no irq_host found\n");
+ goto out_error;
+ }
+
+ return irq_host->host_data;
+
+out_error:
+ of_node_put(np);
+ of_node_put(tmp);
+
+ return NULL;
+}
+
+static int axon_msi_check_device(struct pci_dev *dev, int nvec, int type)
+{
+ if (!find_msi_translator(dev))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg *msg)
+{
+ struct device_node *np, *tmp;
+ int pos, len;
+ u16 flags;
+ const u32 *prop;
+
+ np = pci_device_to_OF_node(dev);
+ if (!np) {
+ dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
+ return -ENODEV;
+ }
+
+ pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
+ if (!pos || pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &flags)) {
+ dev_err(&dev->dev, "axon_msi: error reading config space!\n");
+ return -EIO;
+ }
+
+ for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp) {
+ if (flags & PCI_MSI_FLAGS_64BIT) {
+ prop = of_get_property(np, "msi-address-64", &len);
+ if (prop)
+ break;
+ }
+
+ prop = of_get_property(np, "msi-address-32", &len);
+ if (prop)
+ break;
+ }
+ of_node_put(np);
+
+ if (!prop) {
+ dev_dbg(&dev->dev, "axon_msi: no msi-address-(32|64) found\n");
+ return -ENOENT;
+ }
+
+ switch (len) {
+ case 8:
+ msg->address_hi = prop[0];
+ msg->address_lo = prop[1];
+ break;
+ case 4:
+ msg->address_hi = 0;
+ msg->address_lo = prop[0];
+ break;
+ default:
+ dev_dbg(&dev->dev, "axon_msi: malformed msi-address-(32|64)\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+{
+ unsigned int virq, rc;
+ struct msi_desc *entry;
+ struct msi_msg msg;
+ struct axon_msic *msic;
+
+ msic = find_msi_translator(dev);
+ if (!msic)
+ return -ENODEV;
+
+ rc = setup_msi_msg_address(dev, &msg);
+ if (rc)
+ return rc;
+
+ /* We rely on being able to stash a virq in a u16 */
+ BUILD_BUG_ON(NR_IRQS > 65536);
+
+ list_for_each_entry(entry, &dev->msi_list, list) {
+ virq = irq_create_direct_mapping(msic->irq_host);
+ if (virq == NO_IRQ) {
+ pr_debug("axon_msi: virq allocation failed!\n");
+ return -1;
+ }
+ pr_debug("axon_msi: allocated virq 0x%x for %s\n",
+ virq, pci_name(dev));
+
+ set_irq_msi(virq, entry);
+ msg.data = virq;
+ write_msi_msg(virq, &msg);
+ }
+
+ return 0;
+}
+
+static void axon_msi_teardown_msi_irqs(struct pci_dev *dev)
+{
+ struct msi_desc *entry;
+
+ pr_debug("axon_msi: tearing down irqs for %s\n", pci_name(dev));
+
+ list_for_each_entry(entry, &dev->msi_list, list) {
+ if (entry->irq == NO_IRQ)
+ continue;
+
+ set_irq_msi(entry->irq, NULL);
+ irq_dispose_mapping(entry->irq);
+ }
+}
+
+static struct irq_chip msic_irq_chip = {
+ .mask = mask_msi_irq,
+ .unmask = unmask_msi_irq,
+ .shutdown = unmask_msi_irq,
+ .typename = "AXON-MSI",
+};
+
+static int msic_host_map(struct irq_host *h, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ set_irq_chip_and_handler(virq, &msic_irq_chip, handle_simple_irq);
+ return 0;
+}
+
+static int msic_host_match(struct irq_host *host, struct device_node *node)
+{
+ struct axon_msic *msic = host->host_data;
+ return msic->dn == node;
+}
+
+static struct irq_host_ops msic_host_ops = {
+ .match = msic_host_match,
+ .map = msic_host_map,
+};
+
+static int axon_msi_notify_reboot(struct notifier_block *nb,
+ unsigned long code, void *data)
+{
+ struct axon_msic *msic;
+ u32 tmp;
+
+ list_for_each_entry(msic, &axon_msic_list, list) {
+ tmp = dcr_read(msic->dcr_host, MSIC_CTRL_REG_ADDR);
+ tmp &= ~MSIC_ENABLE;
+ dcr_write(msic->dcr_host, MSIC_CTRL_REG_ADDR, tmp);
+ pr_debug("axon_msi: disabling %s\n", msic->dn->full_name);
+ }
+
+ return 0;
+}
+
+static struct notifier_block axon_msi_reboot_notifier = {
+ .notifier_call = axon_msi_notify_reboot
+};
+
+static int axon_msi_setup_one(struct device_node *node)
+{
+ struct page *page;
+ struct axon_msic *msic;
+ unsigned int virq;
+
+ pr_debug("axon_msi: setting up dn %s\n", node->full_name);
+
+ msic = kzalloc(sizeof(struct axon_msic), GFP_KERNEL);
+ if (!msic) {
+ printk(KERN_ERR "axon_msi: couldn't allocate msic\n");
+ goto out_put;
+ }
+
+ msic->dn = node;
+
+ msic->dcr_host = dcr_map(node, MSIC_DCR_BASE, MSIC_DCR_SIZE);
+ if (!DCR_MAP_OK(msic->dcr_host)) {
+ printk(KERN_ERR "axon_msi: dcr_map failed\n");
+ goto out_free_msic;
+ }
+
+ page = alloc_pages_node(of_node_to_nid(node),
+ GFP_KERNEL, MSIC_FIFO_ORDER);
+ if (!page) {
+ printk(KERN_ERR "axon_msi: couldn't allocate fifo\n");
+ goto out_free_msic;
+ }
+
+ msic->fifo = page_address(page);
+
+ msic->irq_host = irq_alloc_host(IRQ_HOST_MAP_NOMAP, NR_IRQS,
+ &msic_host_ops, 0);
+ if (!msic->irq_host) {
+ printk(KERN_ERR "axon_msi: couldn't allocate host\n");
+ goto out_free_fifo;
+ }
+
+ msic->irq_host->host_data = msic;
+
+ virq = irq_of_parse_and_map(node, 0);
+ if (virq == NO_IRQ) {
+ printk(KERN_ERR "axon_msi: irq parse/map failed!\n");
+ goto out_free_host;
+ }
+
+ set_irq_data(virq, msic);
+ set_irq_chained_handler(virq, axon_msi_cascade);
+ pr_debug("axon_msi: irq 0x%x setup for axon_msi!\n", virq);
+
+ /* Enable the MSIC hardware */
+ dcr_write(msic->dcr_host, MSIC_BASE_ADDR_HI_REG,
+ (u64)msic->fifo >> 32);
+ dcr_write(msic->dcr_host, MSIC_BASE_ADDR_LO_REG,
+ (u64)msic->fifo & 0xFFFFFFFF);
+ dcr_write(msic->dcr_host, MSIC_CTRL_REG_ADDR,
+ MSIC_IRQ_ENABLE | MSIC_ENABLE |
+ (MSIC_FIFO_SIZE << MSIC_SIZE_SHIFT));
+
+ list_add(&msic->list, &axon_msic_list);
+
+ return 0;
+
+out_free_host:
+ kfree(msic->irq_host);
+out_free_fifo:
+ __free_pages(virt_to_page(msic->fifo), MSIC_FIFO_ORDER);
+out_free_msic:
+ kfree(msic);
+out_put:
+ of_node_put(node);
+
+ return -1;
+}
+
+static int axon_msi_init(void)
+{
+ struct device_node *node;
+ int found = 0;
+
+ pr_debug("axon_msi: initialising ...\n");
+
+ for_each_compatible_node(node, NULL, "ibm,axon-msic") {
+ if (axon_msi_setup_one(of_node_get(node)) == 0)
+ found++;
+ }
+ of_node_put(node);
+
+ if (found) {
+ ppc_md.setup_msi_irqs = axon_msi_setup_msi_irqs;
+ ppc_md.teardown_msi_irqs = axon_msi_teardown_msi_irqs;
+ ppc_md.msi_check_device = axon_msi_check_device;
+
+ register_reboot_notifier(&axon_msi_reboot_notifier);
+
+ pr_debug("axon_msi: registered callbacks!\n");
+ }
+
+ return 0;
+}
+arch_initcall(axon_msi_init);
--
1.5.1.3.g7a33b-dirty
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
@ 2007-06-04 17:09 ` Arnd Bergmann
2007-06-04 17:12 ` Arnd Bergmann
` (3 subsequent siblings)
4 siblings, 0 replies; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-04 17:09 UTC (permalink / raw)
To: linuxppc-dev
On Monday 04 June 2007, Michael Ellerman wrote:
> This patch adds support for the setup and decoding of MSIs
> on Axon-based Cell systems.
Looks pretty good, just two details I noticed:
> +/* Flags */
> +#define MSIC_ENABLE 0x0001 /* Bit 31 */
> +#define MSIC_FIFO_FULL_ENABLE 0x0002 /* Bit 30 */
> +#define MSIC_IRQ_ENABLE 0x0008 /* Bit 28 */
> +#define MSIC_FULL_STOP_ENABLE 0x0010 /* Bit 27 */
The comments behind each definition look rather bogus, as much as
we all love to have documentation normally.
If you have the specification for the hardware, the bit numbers
are in there, for everyone else, the IBM numbering scheme can
only confuse the reader...
> +#define MSIC_DCR_BASE MSIC_CTRL_REG_ADDR
> +#define MSIC_DCR_SIZE (MSIC_WRITE_OFFSET_REG - MSIC_CTRL_REG_ADDR)
shouldn't that come from the device tree?
Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
2007-06-04 17:09 ` Arnd Bergmann
@ 2007-06-04 17:12 ` Arnd Bergmann
2007-06-08 2:53 ` Michael Ellerman
2007-06-04 22:45 ` Benjamin Herrenschmidt
` (2 subsequent siblings)
4 siblings, 1 reply; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-04 17:12 UTC (permalink / raw)
To: linuxppc-dev
T24gTW9uZGF5IDA0IEp1bmUgMjAwNywgTWljaGFlbCBFbGxlcm1hbiB3cm90ZToKPiAroKCgoKCg
oGZvcl9lYWNoX2NvbXBhdGlibGVfbm9kZShub2RlLCBOVUxMLCAiaWJtLGF4b24tbXNpYyIpIHsK
PiAroKCgoKCgoKCgoKCgoKCgaWYgKGF4b25fbXNpX3NldHVwX29uZShvZl9ub2RlX2dldChub2Rl
KSkgPT0gMCkKPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBmb3VuZCsrOwo+ICugoKCgoKCgfQo+
ICugoKCgoKCgb2Zfbm9kZV9wdXQobm9kZSk7CgpPbmUgbW9yZSB0aGluZzogQUZBSUNTICdub2Rl
JyBpcyBndWFyYW50ZWVkIHRvIGJlIE5VTEwgd2hlbiB5b3UgZ2V0Cm91dCBvZiB0aGUgZm9yX2Vh
Y2hfY29tcGF0aWJsZV9ub2RlIGxvb3AsIHNvIHlvdSBkb24ndCBuZWVkIHRoZQpvZl9ub2RlX3B1
dCgpLgoKCUFybmQgPD48Cg==
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] Split virq setup logic out into irq_setup_virq()
2007-06-04 12:59 [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Michael Ellerman
` (2 preceding siblings ...)
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
@ 2007-06-04 22:43 ` Benjamin Herrenschmidt
3 siblings, 0 replies; 31+ messages in thread
From: Benjamin Herrenschmidt @ 2007-06-04 22:43 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Mon, 2007-06-04 at 22:59 +1000, Michael Ellerman wrote:
> A future patch will need the logic at the end of irq_create_mapping()
> which setups a virq and installs it in the irq_map. So split it out
> into a new function irq_setup_virq().
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/kernel/irq.c | 32 +++++++++++++++++++++-----------
> 1 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 42c8ed6..9bafc88 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -591,6 +591,25 @@ static void irq_radix_rdunlock(unsigned long flags)
> local_irq_restore(flags);
> }
>
> +static int irq_setup_virq(struct irq_host *host, unsigned int virq,
> + irq_hw_number_t hwirq)
> +{
> + /* Clear IRQ_NOREQUEST flag */
> + get_irq_desc(virq)->status &= ~IRQ_NOREQUEST;
> +
> + /* map it */
> + smp_wmb();
> + irq_map[virq].hwirq = hwirq;
> + smp_mb();
> +
> + if (host->ops->map(host, virq, hwirq)) {
> + pr_debug("irq: -> mapping failed, freeing\n");
> + irq_free_virt(virq, 1);
> + return -1;
> + }
> +
> + return 0;
> +}
>
> unsigned int irq_create_mapping(struct irq_host *host,
> irq_hw_number_t hwirq)
> @@ -639,18 +658,9 @@ unsigned int irq_create_mapping(struct irq_host *host,
> }
> pr_debug("irq: -> obtained virq %d\n", virq);
>
> - /* Clear IRQ_NOREQUEST flag */
> - get_irq_desc(virq)->status &= ~IRQ_NOREQUEST;
> -
> - /* map it */
> - smp_wmb();
> - irq_map[virq].hwirq = hwirq;
> - smp_mb();
> - if (host->ops->map(host, virq, hwirq)) {
> - pr_debug("irq: -> mapping failed, freeing\n");
> - irq_free_virt(virq, 1);
> + if (irq_setup_virq(host, virq, hwirq))
> return NO_IRQ;
> - }
> +
> return virq;
> }
> EXPORT_SYMBOL_GPL(irq_create_mapping);
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] Add irq_create_direct_mapping()
2007-06-04 13:00 ` [PATCH 2/4] Add irq_create_direct_mapping() Michael Ellerman
@ 2007-06-04 22:44 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 31+ messages in thread
From: Benjamin Herrenschmidt @ 2007-06-04 22:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Mon, 2007-06-04 at 23:00 +1000, Michael Ellerman wrote:
> This patch adds irq_create_direct_mapping(). This routine is
> an alternative to irq_create_mapping(), for irq controllers that
> can use linux virq numbers directly as hardware numbers.
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/kernel/irq.c | 24 ++++++++++++++++++++++++
> include/asm-powerpc/irq.h | 9 +++++++++
> 2 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 9bafc88..fdb3b00 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -611,6 +611,30 @@ static int irq_setup_virq(struct irq_host *host, unsigned int virq,
> return 0;
> }
>
> +unsigned int irq_create_direct_mapping(struct irq_host *host)
> +{
> + unsigned int virq;
> +
> + if (host == NULL)
> + host = irq_default_host;
> +
> + BUG_ON(host == NULL);
> + WARN_ON(host->revmap_type != IRQ_HOST_MAP_NOMAP);
> +
> + virq = irq_alloc_virt(host, 1, 0);
> + if (virq == NO_IRQ) {
> + pr_debug("irq: create_direct virq allocation failed\n");
> + return NO_IRQ;
> + }
> +
> + pr_debug("irq: create_direct obtained virq %d\n", virq);
> +
> + if (irq_setup_virq(host, virq, virq))
> + return NO_IRQ;
> +
> + return virq;
> +}
> +
> unsigned int irq_create_mapping(struct irq_host *host,
> irq_hw_number_t hwirq)
> {
> diff --git a/include/asm-powerpc/irq.h b/include/asm-powerpc/irq.h
> index 4734cc1..8384e3f 100644
> --- a/include/asm-powerpc/irq.h
> +++ b/include/asm-powerpc/irq.h
> @@ -226,6 +226,15 @@ extern void irq_dispose_mapping(unsigned int virq);
> extern unsigned int irq_find_mapping(struct irq_host *host,
> irq_hw_number_t hwirq);
>
> +/**
> + * irq_create_direct_mapping - Allocate a virq for direct mapping
> + * @host: host to allocate the virq for or NULL for default host
> + *
> + * This routine is used for irq controllers which can choose the hardware
> + * interrupt numbers they generate. In such a case it's simplest to use
> + * the linux virq as the hardware interrupt number.
> + */
> +extern unsigned int irq_create_direct_mapping(struct irq_host *host);
>
> /**
> * irq_radix_revmap - Find a linux virq from a hw irq number.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] Add for_each_compatible_node()
2007-06-04 13:00 ` [PATCH 3/4] Add for_each_compatible_node() Michael Ellerman
@ 2007-06-04 22:44 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 31+ messages in thread
From: Benjamin Herrenschmidt @ 2007-06-04 22:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Mon, 2007-06-04 at 23:00 +1000, Michael Ellerman wrote:
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> include/asm-powerpc/prom.h | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
> index 6845af9..1122a92 100644
> --- a/include/asm-powerpc/prom.h
> +++ b/include/asm-powerpc/prom.h
> @@ -124,6 +124,9 @@ extern struct device_node *of_find_node_by_type(struct device_node *from,
> dn = of_find_node_by_type(dn, type))
> extern struct device_node *of_find_compatible_node(struct device_node *from,
> const char *type, const char *compat);
> +#define for_each_compatible_node(dn, type, compatible) \
> + for (dn = of_find_compatible_node(NULL, type, compatible); dn; \
> + dn = of_find_compatible_node(dn, type, compatible))
> extern struct device_node *of_find_node_by_path(const char *path);
> extern struct device_node *of_find_node_by_phandle(phandle handle);
> extern struct device_node *of_find_all_nodes(struct device_node *prev);
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
2007-06-04 17:09 ` Arnd Bergmann
2007-06-04 17:12 ` Arnd Bergmann
@ 2007-06-04 22:45 ` Benjamin Herrenschmidt
2007-06-05 7:00 ` Jean-Christophe Dubois
2007-06-05 14:12 ` Milton Miller
4 siblings, 0 replies; 31+ messages in thread
From: Benjamin Herrenschmidt @ 2007-06-04 22:45 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Mon, 2007-06-04 at 23:00 +1000, Michael Ellerman wrote:
> This patch adds support for the setup and decoding of MSIs
> on Axon-based Cell systems.
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> This still needs a bit of cleanup, but sending now for an early review.
>
> arch/powerpc/platforms/cell/Makefile | 2 +
> arch/powerpc/platforms/cell/axon_msi.c | 412 ++++++++++++++++++++++++++++++++
> 2 files changed, 414 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/platforms/cell/Makefile b/arch/powerpc/platforms/cell/Makefile
> index 869af89..6615d40 100644
> --- a/arch/powerpc/platforms/cell/Makefile
> +++ b/arch/powerpc/platforms/cell/Makefile
> @@ -23,3 +23,5 @@ obj-$(CONFIG_SPU_BASE) += spu_callbacks.o spu_base.o \
> $(spu-priv1-y) \
> $(spu-manage-y) \
> spufs/
> +
> +obj-$(CONFIG_PCI_MSI) += axon_msi.o
> diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
> new file mode 100644
> index 0000000..2709853
> --- /dev/null
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
> @@ -0,0 +1,412 @@
> +/*
> + * Copyright 2007, Michael Ellerman, IBM 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; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/msi.h>
> +#include <linux/reboot.h>
> +
> +#include <asm/dcr.h>
> +#include <asm/machdep.h>
> +#include <asm/prom.h>
> +
> +
> +/*
> + * MSIC Control Register
> + */
> +#define MSIC_CTRL_REG_ADDR 0x6F
> +
> +/* Flags */
> +#define MSIC_ENABLE 0x0001 /* Bit 31 */
> +#define MSIC_FIFO_FULL_ENABLE 0x0002 /* Bit 30 */
> +#define MSIC_IRQ_ENABLE 0x0008 /* Bit 28 */
> +#define MSIC_FULL_STOP_ENABLE 0x0010 /* Bit 27 */
> +
> +/* Size configuration constants */
> +#define MSIC_SIZE_MASK 0x0180 /* Bits 22:23 */
> +#define MSIC_SIZE_SHIFT (7)
> +#define MSIC_SIZE_32K 0x0
> +#define MSIC_SIZE_64K 0x1
> +#define MSIC_SIZE_128K 0x2
> +#define MSIC_SIZE_256K 0x3
> +
> +/* The size we're actually using */
> +#define MSIC_FIFO_SIZE MSIC_SIZE_32K
> +
> +/* Different representations of the fifo size */
> +#define MSIC_FIFO_SHIFT (MSIC_FIFO_SIZE + 0xF)
> +#define MSIC_FIFO_BYTES (1 << MSIC_FIFO_SHIFT)
> +#define MSIC_FIFO_MASK (MSIC_FIFO_BYTES - 1)
> +#define MSIC_FIFO_ORDER max(MSIC_FIFO_SHIFT - PAGE_SHIFT, 0)
> +
> +/*
> + * MSIC Base Address registers (FIFO location)
> + */
> +#define MSIC_BASE_ADDR_HI_REG 0x72
> +#define MSIC_BASE_ADDR_LO_REG 0x73
> +
> +#define MSIC_READ_OFFSET_REG 0x74
> +#define MSIC_WRITE_OFFSET_REG 0x75
> +
> +#define MSIC_DCR_BASE MSIC_CTRL_REG_ADDR
> +#define MSIC_DCR_SIZE (MSIC_WRITE_OFFSET_REG - MSIC_CTRL_REG_ADDR)
> +
> +
> +struct axon_msic {
> + struct device_node *dn;
> + struct irq_host *irq_host;
> + void *fifo;
> + dcr_host_t dcr_host;
> + struct list_head list;
> + u32 read_offset;
> +};
> +
> +
> +static LIST_HEAD(axon_msic_list);
> +
> +static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> + struct axon_msic *msic = get_irq_data(irq);
> + u32 write_offset, msi;
> + unsigned int cirq;
> +
> + write_offset = dcr_read(msic->dcr_host, MSIC_WRITE_OFFSET_REG);
> + pr_debug("axon_msi: original write_offset 0x%x\n", write_offset);
> +
> + /* write_offset doesn't wrap properly, so we have to mask it */
> + write_offset &= MSIC_FIFO_MASK;
> +
> + while (msic->read_offset != write_offset) {
> + msi = le32_to_cpup((__le32*)(msic->fifo + msic->read_offset));
> + msi &= 0xFFFF;
> +
> + pr_debug("axon_msi: woff %x roff %x @ %p msi %x msi@0 %x\n",
> + write_offset, msic->read_offset,
> + msic->fifo + msic->read_offset, msi, *(u32*)msic->fifo);
> +
> + msic->read_offset += 0x10;
> + msic->read_offset &= MSIC_FIFO_MASK;
> +
> + cirq = irq_linear_revmap(msic->irq_host, msi);
> + if (cirq != NO_IRQ)
> + generic_handle_irq(cirq);
> + else
> + pr_debug("axon_msi: mapped to NO_IRQ!\n");
> + }
> +
> + dcr_write(msic->dcr_host, MSIC_READ_OFFSET_REG, msic->read_offset);
> +
> + desc->chip->eoi(irq);
> +}
> +
> +static struct axon_msic *find_msi_translator(struct pci_dev *dev)
> +{
> + struct irq_host *irq_host;
> + struct device_node *np, *tmp;
> + const phandle *ph;
> +
> + np = pci_device_to_OF_node(dev);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
> + return NULL;
> + }
> +
> + for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp) {
> + ph = of_get_property(np, "msi-translator", NULL);
> + if (ph)
> + break;
> + }
> +
> + if (!ph) {
> + dev_dbg(&dev->dev, "axon_msi: no msi-translator found\n");
> + goto out_error;
> + }
> +
> + tmp = np;
> + np = of_find_node_by_phandle(*ph);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: invalid msi-translator found\n");
> + goto out_error;
> + }
> +
> + irq_host = irq_find_host(np);
> + if (!irq_host) {
> + dev_dbg(&dev->dev, "axon_msi: no irq_host found\n");
> + goto out_error;
> + }
> +
> + return irq_host->host_data;
> +
> +out_error:
> + of_node_put(np);
> + of_node_put(tmp);
> +
> + return NULL;
> +}
> +
> +static int axon_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +{
> + if (!find_msi_translator(dev))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg *msg)
> +{
> + struct device_node *np, *tmp;
> + int pos, len;
> + u16 flags;
> + const u32 *prop;
> +
> + np = pci_device_to_OF_node(dev);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
> + return -ENODEV;
> + }
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> + if (!pos || pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &flags)) {
> + dev_err(&dev->dev, "axon_msi: error reading config space!\n");
> + return -EIO;
> + }
> +
> + for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp) {
> + if (flags & PCI_MSI_FLAGS_64BIT) {
> + prop = of_get_property(np, "msi-address-64", &len);
> + if (prop)
> + break;
> + }
> +
> + prop = of_get_property(np, "msi-address-32", &len);
> + if (prop)
> + break;
> + }
> + of_node_put(np);
> +
> + if (!prop) {
> + dev_dbg(&dev->dev, "axon_msi: no msi-address-(32|64) found\n");
> + return -ENOENT;
> + }
> +
> + switch (len) {
> + case 8:
> + msg->address_hi = prop[0];
> + msg->address_lo = prop[1];
> + break;
> + case 4:
> + msg->address_hi = 0;
> + msg->address_lo = prop[0];
> + break;
> + default:
> + dev_dbg(&dev->dev, "axon_msi: malformed msi-address-(32|64)\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int axon_msi_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +{
> + unsigned int virq, rc;
> + struct msi_desc *entry;
> + struct msi_msg msg;
> + struct axon_msic *msic;
> +
> + msic = find_msi_translator(dev);
> + if (!msic)
> + return -ENODEV;
> +
> + rc = setup_msi_msg_address(dev, &msg);
> + if (rc)
> + return rc;
> +
> + /* We rely on being able to stash a virq in a u16 */
> + BUILD_BUG_ON(NR_IRQS > 65536);
> +
> + list_for_each_entry(entry, &dev->msi_list, list) {
> + virq = irq_create_direct_mapping(msic->irq_host);
> + if (virq == NO_IRQ) {
> + pr_debug("axon_msi: virq allocation failed!\n");
> + return -1;
> + }
> + pr_debug("axon_msi: allocated virq 0x%x for %s\n",
> + virq, pci_name(dev));
> +
> + set_irq_msi(virq, entry);
> + msg.data = virq;
> + write_msi_msg(virq, &msg);
> + }
> +
> + return 0;
> +}
> +
> +static void axon_msi_teardown_msi_irqs(struct pci_dev *dev)
> +{
> + struct msi_desc *entry;
> +
> + pr_debug("axon_msi: tearing down irqs for %s\n", pci_name(dev));
> +
> + list_for_each_entry(entry, &dev->msi_list, list) {
> + if (entry->irq == NO_IRQ)
> + continue;
> +
> + set_irq_msi(entry->irq, NULL);
> + irq_dispose_mapping(entry->irq);
> + }
> +}
> +
> +static struct irq_chip msic_irq_chip = {
> + .mask = mask_msi_irq,
> + .unmask = unmask_msi_irq,
> + .shutdown = unmask_msi_irq,
> + .typename = "AXON-MSI",
> +};
> +
> +static int msic_host_map(struct irq_host *h, unsigned int virq,
> + irq_hw_number_t hw)
> +{
> + set_irq_chip_and_handler(virq, &msic_irq_chip, handle_simple_irq);
> + return 0;
> +}
> +
> +static int msic_host_match(struct irq_host *host, struct device_node *node)
> +{
> + struct axon_msic *msic = host->host_data;
> + return msic->dn == node;
> +}
> +
> +static struct irq_host_ops msic_host_ops = {
> + .match = msic_host_match,
> + .map = msic_host_map,
> +};
> +
> +static int axon_msi_notify_reboot(struct notifier_block *nb,
> + unsigned long code, void *data)
> +{
> + struct axon_msic *msic;
> + u32 tmp;
> +
> + list_for_each_entry(msic, &axon_msic_list, list) {
> + tmp = dcr_read(msic->dcr_host, MSIC_CTRL_REG_ADDR);
> + tmp &= ~MSIC_ENABLE;
> + dcr_write(msic->dcr_host, MSIC_CTRL_REG_ADDR, tmp);
> + pr_debug("axon_msi: disabling %s\n", msic->dn->full_name);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block axon_msi_reboot_notifier = {
> + .notifier_call = axon_msi_notify_reboot
> +};
> +
> +static int axon_msi_setup_one(struct device_node *node)
> +{
> + struct page *page;
> + struct axon_msic *msic;
> + unsigned int virq;
> +
> + pr_debug("axon_msi: setting up dn %s\n", node->full_name);
> +
> + msic = kzalloc(sizeof(struct axon_msic), GFP_KERNEL);
> + if (!msic) {
> + printk(KERN_ERR "axon_msi: couldn't allocate msic\n");
> + goto out_put;
> + }
> +
> + msic->dn = node;
> +
> + msic->dcr_host = dcr_map(node, MSIC_DCR_BASE, MSIC_DCR_SIZE);
> + if (!DCR_MAP_OK(msic->dcr_host)) {
> + printk(KERN_ERR "axon_msi: dcr_map failed\n");
> + goto out_free_msic;
> + }
> +
> + page = alloc_pages_node(of_node_to_nid(node),
> + GFP_KERNEL, MSIC_FIFO_ORDER);
> + if (!page) {
> + printk(KERN_ERR "axon_msi: couldn't allocate fifo\n");
> + goto out_free_msic;
> + }
> +
> + msic->fifo = page_address(page);
> +
> + msic->irq_host = irq_alloc_host(IRQ_HOST_MAP_NOMAP, NR_IRQS,
> + &msic_host_ops, 0);
> + if (!msic->irq_host) {
> + printk(KERN_ERR "axon_msi: couldn't allocate host\n");
> + goto out_free_fifo;
> + }
> +
> + msic->irq_host->host_data = msic;
> +
> + virq = irq_of_parse_and_map(node, 0);
> + if (virq == NO_IRQ) {
> + printk(KERN_ERR "axon_msi: irq parse/map failed!\n");
> + goto out_free_host;
> + }
> +
> + set_irq_data(virq, msic);
> + set_irq_chained_handler(virq, axon_msi_cascade);
> + pr_debug("axon_msi: irq 0x%x setup for axon_msi!\n", virq);
> +
> + /* Enable the MSIC hardware */
> + dcr_write(msic->dcr_host, MSIC_BASE_ADDR_HI_REG,
> + (u64)msic->fifo >> 32);
> + dcr_write(msic->dcr_host, MSIC_BASE_ADDR_LO_REG,
> + (u64)msic->fifo & 0xFFFFFFFF);
> + dcr_write(msic->dcr_host, MSIC_CTRL_REG_ADDR,
> + MSIC_IRQ_ENABLE | MSIC_ENABLE |
> + (MSIC_FIFO_SIZE << MSIC_SIZE_SHIFT));
> +
> + list_add(&msic->list, &axon_msic_list);
> +
> + return 0;
> +
> +out_free_host:
> + kfree(msic->irq_host);
> +out_free_fifo:
> + __free_pages(virt_to_page(msic->fifo), MSIC_FIFO_ORDER);
> +out_free_msic:
> + kfree(msic);
> +out_put:
> + of_node_put(node);
> +
> + return -1;
> +}
> +
> +static int axon_msi_init(void)
> +{
> + struct device_node *node;
> + int found = 0;
> +
> + pr_debug("axon_msi: initialising ...\n");
> +
> + for_each_compatible_node(node, NULL, "ibm,axon-msic") {
> + if (axon_msi_setup_one(of_node_get(node)) == 0)
> + found++;
> + }
> + of_node_put(node);
> +
> + if (found) {
> + ppc_md.setup_msi_irqs = axon_msi_setup_msi_irqs;
> + ppc_md.teardown_msi_irqs = axon_msi_teardown_msi_irqs;
> + ppc_md.msi_check_device = axon_msi_check_device;
> +
> + register_reboot_notifier(&axon_msi_reboot_notifier);
> +
> + pr_debug("axon_msi: registered callbacks!\n");
> + }
> +
> + return 0;
> +}
> +arch_initcall(axon_msi_init);
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
` (2 preceding siblings ...)
2007-06-04 22:45 ` Benjamin Herrenschmidt
@ 2007-06-05 7:00 ` Jean-Christophe Dubois
2007-06-05 9:16 ` Arnd Bergmann
2007-06-07 7:58 ` Michael Ellerman
2007-06-05 14:12 ` Milton Miller
4 siblings, 2 replies; 31+ messages in thread
From: Jean-Christophe Dubois @ 2007-06-05 7:00 UTC (permalink / raw)
To: linuxppc-dev
On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> +=A0=A0=A0=A0=A0=A0=A0pr_debug("axon_msi: initialising ...\n");
> +
> +=A0=A0=A0=A0=A0=A0=A0for_each_compatible_node(node, NULL, "ibm,axon-msic=
") {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (axon_msi_setup_one(of_n=
ode_get(node)) =3D=3D 0)
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0fou=
nd++;
> +=A0=A0=A0=A0=A0=A0=A0}
> +=A0=A0=A0=A0=A0=A0=A0of_node_put(node);
2 comments:
1) There is no "ibm,axon-msic" compatible property in the SLOF tree provide=
d=20
by IBM on the CAB (or other such platforms). Therefore this code will not=20
work on these platforms.
2) you should somehow check for the Axon version. Axon 1.1 (and prior) are=
=20
know to be loosing MSI/MBX interrupts (but not the data part associated to=
=20
them). Therefore this should be enabled only on Axon 2.1 (or later).
JC
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-05 7:00 ` Jean-Christophe Dubois
@ 2007-06-05 9:16 ` Arnd Bergmann
2007-06-05 9:29 ` Jean-Christophe Dubois
2007-06-05 9:30 ` Arnd Bergmann
2007-06-07 7:58 ` Michael Ellerman
1 sibling, 2 replies; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-05 9:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Christian Rund
On Tuesday 05 June 2007, Jean-Christophe Dubois wrote:
> On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> > +=A0=A0=A0=A0=A0=A0=A0pr_debug("axon_msi: initialising ...\n");
> > +
> > +=A0=A0=A0=A0=A0=A0=A0for_each_compatible_node(node, NULL, "ibm,axon-ms=
ic") {
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (axon_msi_setup_one(of=
_node_get(node)) =3D=3D 0)
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0f=
ound++;
> > +=A0=A0=A0=A0=A0=A0=A0}
> > +=A0=A0=A0=A0=A0=A0=A0of_node_put(node);
>=20
> 2 comments:
>=20
> 1) There is no "ibm,axon-msic" compatible property in the SLOF tree provi=
ded=20
> by IBM on the CAB (or other such platforms). Therefore this code will not=
=20
> work on these platforms.
Right, the firmware needs to support msi so we can use it. I guess taht's t=
rue
for every device: If the firmware decies to hide it from us, we better don't
try to touch it.
> 2) you should somehow check for the Axon version. Axon 1.1 (and prior) ar=
e=20
> know to be loosing MSI/MBX interrupts (but not the data part associated t=
o=20
> them). Therefore this should be enabled only on Axon 2.1 (or later).
Good point, thanks for mentioning it. Christian, can you confirm that the
ibm,axon-msic node is only created on systems with axon 2.1 or later?
Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-05 9:16 ` Arnd Bergmann
@ 2007-06-05 9:29 ` Jean-Christophe Dubois
2007-06-05 9:30 ` Arnd Bergmann
1 sibling, 0 replies; 31+ messages in thread
From: Jean-Christophe Dubois @ 2007-06-05 9:29 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, Christian Rund
On Tuesday 05 June 2007 11:16:13 Arnd Bergmann wrote:
> On Tuesday 05 June 2007, Jean-Christophe Dubois wrote:
> > On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> > > +=A0=A0=A0=A0=A0=A0=A0pr_debug("axon_msi: initialising ...\n");
> > > +
> > > +=A0=A0=A0=A0=A0=A0=A0for_each_compatible_node(node, NULL, "ibm,axon-=
msic") {
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (axon_msi_setup_one(=
of_node_get(node)) =3D=3D 0)
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0found++;
> > > +=A0=A0=A0=A0=A0=A0=A0}
> > > +=A0=A0=A0=A0=A0=A0=A0of_node_put(node);
> >
> > 2 comments:
> >
> > 1) There is no "ibm,axon-msic" compatible property in the SLOF tree
> > provided by IBM on the CAB (or other such platforms). Therefore this co=
de
> > will not work on these platforms.
>
> Right, the firmware needs to support msi so we can use it. I guess taht's
> true for every device: If the firmware decies to hide it from us, we bett=
er
> don't try to touch it.
The SLOF firmware we have has an MSI node, just not one that=20
is "ibm,axon-msic" compatible. So nothing is hidden, quite the opposite.
> > 2) you should somehow check for the Axon version. Axon 1.1 (and prior)
> > are know to be loosing MSI/MBX interrupts (but not the data part
> > associated to them). Therefore this should be enabled only on Axon 2.1
> > (or later).
>
> Good point, thanks for mentioning it. Christian, can you confirm that the
> ibm,axon-msic node is only created on systems with axon 2.1 or later?
>
> Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-05 9:16 ` Arnd Bergmann
2007-06-05 9:29 ` Jean-Christophe Dubois
@ 2007-06-05 9:30 ` Arnd Bergmann
1 sibling, 0 replies; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-05 9:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Christian.Rund
On Tuesday 05 June 2007, Arnd Bergmann wrote:
Sorry, I had the wrong address for Christian, resending to the right one...
> On Tuesday 05 June 2007, Jean-Christophe Dubois wrote:
> > On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> > > +=A0=A0=A0=A0=A0=A0=A0pr_debug("axon_msi: initialising ...\n");
> > > +
> > > +=A0=A0=A0=A0=A0=A0=A0for_each_compatible_node(node, NULL, "ibm,axon-=
msic") {
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if (axon_msi_setup_one(=
of_node_get(node)) =3D=3D 0)
> > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0found++;
> > > +=A0=A0=A0=A0=A0=A0=A0}
> > > +=A0=A0=A0=A0=A0=A0=A0of_node_put(node);
> >=20
> > 2 comments:
> >=20
> > 1) There is no "ibm,axon-msic" compatible property in the SLOF tree pro=
vided=20
> > by IBM on the CAB (or other such platforms). Therefore this code will n=
ot=20
> > work on these platforms.
>=20
> Right, the firmware needs to support msi so we can use it. I guess taht's=
true
> for every device: If the firmware decies to hide it from us, we better do=
n't
> try to touch it.
>=20
> > 2) you should somehow check for the Axon version. Axon 1.1 (and prior) =
are=20
> > know to be loosing MSI/MBX interrupts (but not the data part associated=
to=20
> > them). Therefore this should be enabled only on Axon 2.1 (or later).
>=20
> Good point, thanks for mentioning it. Christian, can you confirm that the
> ibm,axon-msic node is only created on systems with axon 2.1 or later?
>=20
> Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
` (3 preceding siblings ...)
2007-06-05 7:00 ` Jean-Christophe Dubois
@ 2007-06-05 14:12 ` Milton Miller
2007-06-06 5:19 ` Michael Ellerman
4 siblings, 1 reply; 31+ messages in thread
From: Milton Miller @ 2007-06-05 14:12 UTC (permalink / raw)
To: Michael Ellerman; +Cc: ppcdev
On Mon Jun 4 23:00:05 EST 2007, Michael Ellerman wrote:
> This patch adds support for the setup and decoding of MSIs
> on Axon-based Cell systems.
>
> This still needs a bit of cleanup, but sending now for an early review.
Ok here is some from someone without the documentation.
> diff --git a/arch/powerpc/platforms/cell/axon_msi.c
> b/arch/powerpc/platforms/cell/axon_msi.c
> new file mode 100644
> index 0000000..2709853
> --- /dev/null
> +++ b/arch/powerpc/platforms/cell/axon_msi.c
...
> +/*
> + * MSIC Control Register
> + */
> +#define MSIC_CTRL_REG_ADDR 0x6F
> +
> +/* Flags */
Comments might be a bit verbose, the defines explain them.
> +#define MSIC_ENABLE 0x0001 /* Bit 31 */
> +#define MSIC_FIFO_FULL_ENABLE 0x0002 /* Bit 30 */
> +#define MSIC_IRQ_ENABLE 0x0008 /* Bit 28 */
> +#define MSIC_FULL_STOP_ENABLE 0x0010 /* Bit 27 */
> +
> +/* Size configuration constants */
> +#define MSIC_SIZE_MASK 0x0180 /* Bits 22:23 */
> +#define MSIC_SIZE_SHIFT (7)
> +#define MSIC_SIZE_32K 0x0
> +#define MSIC_SIZE_64K 0x1
> +#define MSIC_SIZE_128K 0x2
> +#define MSIC_SIZE_256K 0x3
> +
> +/* The size we're actually using */
> +#define MSIC_FIFO_SIZE MSIC_SIZE_32K
Oh, so those were fifo sizes.
Not a config var?
> +
> +/* Different representations of the fifo size */
> +#define MSIC_FIFO_SHIFT (MSIC_FIFO_SIZE + 0xF)
Where did the 0xF come from? everything was derived up until now.
Why is it 0xF and not 15? (We often do shifts in decimal where there
isn't another reason to choose the base.)
> +#define MSIC_FIFO_BYTES (1 << MSIC_FIFO_SHIFT)
> +#define MSIC_FIFO_MASK (MSIC_FIFO_BYTES - 1)
> +#define MSIC_FIFO_ORDER max(MSIC_FIFO_SHIFT -
> PAGE_SHIFT, 0)
> +
> +/*
> + * MSIC Base Address registers (FIFO location)
> + */
At first I thought you were referring to location relative to fifo, but
now I realize its the location and size of the fifo.
> +#define MSIC_BASE_ADDR_HI_REG 0x72
> +#define MSIC_BASE_ADDR_LO_REG 0x73
> +
> +#define MSIC_READ_OFFSET_REG 0x74
> +#define MSIC_WRITE_OFFSET_REG 0x75
> +
> +#define MSIC_DCR_BASE MSIC_CTRL_REG_ADDR
> +#define MSIC_DCR_SIZE (MSIC_WRITE_OFFSET_REG -
> MSIC_CTRL_REG_ADDR)
> +
> +
> +struct axon_msic {
> + struct device_node *dn;
> + struct irq_host *irq_host;
> + void *fifo;
> + dcr_host_t dcr_host;
> + struct list_head list;
> + u32 read_offset;
> +};
> +
> +
> +static LIST_HEAD(axon_msic_list);
> +
> +static void axon_msi_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> + struct axon_msic *msic = get_irq_data(irq);
> + u32 write_offset, msi;
> + unsigned int cirq;
> +
> + write_offset = dcr_read(msic->dcr_host, MSIC_WRITE_OFFSET_REG);
> + pr_debug("axon_msi: original write_offset 0x%x\n",
> write_offset);
> +
> + /* write_offset doesn't wrap properly, so we have to mask it */
> + write_offset &= MSIC_FIFO_MASK;
> +
> + while (msic->read_offset != write_offset) {
> + msi = le32_to_cpup((__le32*)(msic->fifo +
> msic->read_offset));
> + msi &= 0xFFFF;
> +
> + pr_debug("axon_msi: woff %x roff %x @ %p msi %x msi at
> 0 %x\n",
> + write_offset, msic->read_offset,
> + msic->fifo + msic->read_offset, msi,
> *(u32*)msic->fifo);
> +
> + msic->read_offset += 0x10;
Another magic number. Is this the size of an entry?
> + msic->read_offset &= MSIC_FIFO_MASK;
> +
If we increment by 0x10, how do we know write_offset or the original
read_offset is aligned to that value and we terminate? Should the
MSIC_FIFO_MASK mask off the low order bits?
> + cirq = irq_linear_revmap(msic->irq_host, msi);
> + if (cirq != NO_IRQ)
> + generic_handle_irq(cirq);
> + else
> + pr_debug("axon_msi: mapped to NO_IRQ!\n");
> + }
> +
> + dcr_write(msic->dcr_host, MSIC_READ_OFFSET_REG,
> msic->read_offset);
> +
> + desc->chip->eoi(irq);
> +}
> +
> +static struct axon_msic *find_msi_translator(struct pci_dev *dev)
> +{
> + struct irq_host *irq_host;
> + struct device_node *np, *tmp;
> + const phandle *ph;
> +
> + np = pci_device_to_OF_node(dev);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
> + return NULL;
> + }
> +
> + for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp)
> {
> + ph = of_get_property(np, "msi-translator", NULL);
> + if (ph)
> + break;
> + }
> +
> + if (!ph) {
> + dev_dbg(&dev->dev, "axon_msi: no msi-translator
> found\n");
> + goto out_error;
> + }
> +
> + tmp = np;
> + np = of_find_node_by_phandle(*ph);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: invalid msi-translator
> found\n");
msi-translator device not found? It's not clear this is the property
(nor above).
> + goto out_error;
> + }
> +
> + irq_host = irq_find_host(np);
> + if (!irq_host) {
> + dev_dbg(&dev->dev, "axon_msi: no irq_host found\n");
> + goto out_error;
> + }
> +
> + return irq_host->host_data;
> +
> +out_error:
> + of_node_put(np);
> + of_node_put(tmp);
> +
> + return NULL;
> +}
> +
> +static int axon_msi_check_device(struct pci_dev *dev, int nvec, int
> type)
> +{
> + if (!find_msi_translator(dev))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg
> *msg)
> +{
> + struct device_node *np, *tmp;
> + int pos, len;
> + u16 flags;
> + const u32 *prop;
> +
> + np = pci_device_to_OF_node(dev);
> + if (!np) {
> + dev_dbg(&dev->dev, "axon_msi: no pci_dn found\n");
> + return -ENODEV;
> + }
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> + if (!pos || pci_read_config_word(dev, pos + PCI_MSI_FLAGS,
> &flags)) {
> + dev_err(&dev->dev, "axon_msi: error reading config
> space!\n");
> + return -EIO;
> + }
What about MSIX ?
> +
> + for (; np; tmp = of_get_parent(np), of_node_put(np), np = tmp)
> {
> + if (flags & PCI_MSI_FLAGS_64BIT) {
> + prop = of_get_property(np, "msi-address-64",
> &len);
> + if (prop)
> + break;
> + }
> +
> + prop = of_get_property(np, "msi-address-32", &len);
> + if (prop)
> + break;
> + }
> + of_node_put(np);
> +
> + if (!prop) {
> + dev_dbg(&dev->dev, "axon_msi: no msi-address-(32|64)
> found\n");
> + return -ENOENT;
> + }
> +
> + switch (len) {
> + case 8:
> + msg->address_hi = prop[0];
> + msg->address_lo = prop[1];
> + break;
> + case 4:
> + msg->address_hi = 0;
> + msg->address_lo = prop[0];
> + break;
> + default:
> + dev_dbg(&dev->dev, "axon_msi: malformed
> msi-address-(32|64)\n");
> + return -EINVAL;
> + }
How about of_read_number into a u64 and then write out the pieces?
Should be more compact?
> +
> + return 0;
> +}
...
> +static int axon_msi_notify_reboot(struct notifier_block *nb,
> + unsigned long code, void *data)
> +{
> + struct axon_msic *msic;
> + u32 tmp;
> +
> + list_for_each_entry(msic, &axon_msic_list, list) {
> + tmp = dcr_read(msic->dcr_host, MSIC_CTRL_REG_ADDR);
> + tmp &= ~MSIC_ENABLE;
> + dcr_write(msic->dcr_host, MSIC_CTRL_REG_ADDR, tmp);
> + pr_debug("axon_msi: disabling %s\n",
> msic->dn->full_name);
disabled? (or move print)
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block axon_msi_reboot_notifier = {
> + .notifier_call = axon_msi_notify_reboot
> +};
> +
> +static int axon_msi_setup_one(struct device_node *node)
> +{
> + struct page *page;
> + struct axon_msic *msic;
> + unsigned int virq;
> +
> + pr_debug("axon_msi: setting up dn %s\n", node->full_name);
> +
> + msic = kzalloc(sizeof(struct axon_msic), GFP_KERNEL);
> + if (!msic) {
> + printk(KERN_ERR "axon_msi: couldn't allocate msic\n");
Add node->name here (all of the error outs)
> + goto out_put;
> + }
>
...
> + list_add(&msic->list, &axon_msic_list);
>
what is the locking for this list?
milton
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-05 14:12 ` Milton Miller
@ 2007-06-06 5:19 ` Michael Ellerman
0 siblings, 0 replies; 31+ messages in thread
From: Michael Ellerman @ 2007-06-06 5:19 UTC (permalink / raw)
To: Milton Miller; +Cc: ppcdev
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On Tue, 2007-06-05 at 09:12 -0500, Milton Miller wrote:
> On Mon Jun 4 23:00:05 EST 2007, Michael Ellerman wrote:
> > This patch adds support for the setup and decoding of MSIs
> > on Axon-based Cell systems.
> >
> > This still needs a bit of cleanup, but sending now for an early review.
>
> Ok here is some from someone without the documentation.
Thanks Milton, that's what I was after :)
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-05 7:00 ` Jean-Christophe Dubois
2007-06-05 9:16 ` Arnd Bergmann
@ 2007-06-07 7:58 ` Michael Ellerman
2007-06-07 8:10 ` Jean-Christophe Dubois
1 sibling, 1 reply; 31+ messages in thread
From: Michael Ellerman @ 2007-06-07 7:58 UTC (permalink / raw)
To: Jean-Christophe Dubois; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]
On Tue, 2007-06-05 at 09:00 +0200, Jean-Christophe Dubois wrote:
> On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> > + pr_debug("axon_msi: initialising ...\n");
> > +
> > + for_each_compatible_node(node, NULL, "ibm,axon-msic") {
> > + if (axon_msi_setup_one(of_node_get(node)) == 0)
> > + found++;
> > + }
> > + of_node_put(node);
>
> 2 comments:
>
> 1) There is no "ibm,axon-msic" compatible property in the SLOF tree provided
> by IBM on the CAB (or other such platforms). Therefore this code will not
> work on these platforms.
It's not clear to me if MSIs will work on CAB, with Axon in end-point
mode. But it'd be good to try it at some point.
The compatible property is new, it's only just been added to SLOF
recently by Heiko, not sure when it will be in released firmware.
> 2) you should somehow check for the Axon version. Axon 1.1 (and prior) are
> know to be loosing MSI/MBX interrupts (but not the data part associated to
> them). Therefore this should be enabled only on Axon 2.1 (or later).
Yep. It works some of the time on 1.1, but not enough to enable it.
I guess I'd say firmware that runs on 1.1 should just not export the msi
nodes, or not put the compatible property on them - rather than having
the kernel explicitly checking for the version.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-07 7:58 ` Michael Ellerman
@ 2007-06-07 8:10 ` Jean-Christophe Dubois
2007-06-07 11:55 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 31+ messages in thread
From: Jean-Christophe Dubois @ 2007-06-07 8:10 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev
On Thursday 07 June 2007 09:58:59 Michael Ellerman wrote:
> On Tue, 2007-06-05 at 09:00 +0200, Jean-Christophe Dubois wrote:
> > On Monday 04 June 2007 15:00:05 Michael Ellerman wrote:
> > > + pr_debug("axon_msi: initialising ...\n");
> > > +
> > > + for_each_compatible_node(node, NULL, "ibm,axon-msic") {
> > > + if (axon_msi_setup_one(of_node_get(node)) == 0)
> > > + found++;
> > > + }
> > > + of_node_put(node);
> >
> > 2 comments:
> >
> > 1) There is no "ibm,axon-msic" compatible property in the SLOF tree
> > provided by IBM on the CAB (or other such platforms). Therefore this code
> > will not work on these platforms.
>
> It's not clear to me if MSIs will work on CAB, with Axon in end-point
> mode. But it'd be good to try it at some point.
The CAB (or other derived products) can be set in RC mode (via a jumper). In
this case the MSI could be supported.
> The compatible property is new, it's only just been added to SLOF
> recently by Heiko, not sure when it will be in released firmware.
>
> > 2) you should somehow check for the Axon version. Axon 1.1 (and prior)
> > are know to be loosing MSI/MBX interrupts (but not the data part
> > associated to them). Therefore this should be enabled only on Axon 2.1
> > (or later).
>
> Yep. It works some of the time on 1.1, but not enough to enable it.
>
> I guess I'd say firmware that runs on 1.1 should just not export the msi
> nodes, or not put the compatible property on them - rather than having
> the kernel explicitly checking for the version.
>
> cheers
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-07 8:10 ` Jean-Christophe Dubois
@ 2007-06-07 11:55 ` Benjamin Herrenschmidt
2007-06-07 12:00 ` Jean-Christophe Dubois
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Herrenschmidt @ 2007-06-07 11:55 UTC (permalink / raw)
To: Jean-Christophe Dubois; +Cc: linuxppc-dev
On Thu, 2007-06-07 at 10:10 +0200, Jean-Christophe Dubois wrote:
> The CAB (or other derived products) can be set in RC mode (via a jumper). In
> this case the MSI could be supported.
Is there any plan to have CABs based on AXON DD >= 2.0
I still think the right approach is to have the right properties set by
the firmware if the Axon version supports MSIs reliably. Do you see any
problem with this approach ?
Ben.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-07 11:55 ` Benjamin Herrenschmidt
@ 2007-06-07 12:00 ` Jean-Christophe Dubois
0 siblings, 0 replies; 31+ messages in thread
From: Jean-Christophe Dubois @ 2007-06-07 12:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
On Thursday 07 June 2007 13:55:41 Benjamin Herrenschmidt wrote:
> On Thu, 2007-06-07 at 10:10 +0200, Jean-Christophe Dubois wrote:
> > The CAB (or other derived products) can be set in RC mode (via a jumper).
> > In this case the MSI could be supported.
>
> Is there any plan to have CABs based on AXON DD >= 2.0
Sure, There are several at Mercury (based on Axon 2.1) and I have one. And I
believe it will be based on Axon 3.0 when available.
> I still think the right approach is to have the right properties set by
> the firmware if the Axon version supports MSIs reliably. Do you see any
> problem with this approach ?
Once again SLOF firmware is out of my influence realm. I work with what IBM is
delivering for CAB ... So if the firmware has some deficiencies I must make
sure we can work around them.
JC
> Ben.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-04 17:12 ` Arnd Bergmann
@ 2007-06-08 2:53 ` Michael Ellerman
2007-06-08 8:47 ` Segher Boessenkool
0 siblings, 1 reply; 31+ messages in thread
From: Michael Ellerman @ 2007-06-08 2:53 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 958 bytes --]
On Mon, 2007-06-04 at 19:12 +0200, Arnd Bergmann wrote:
> On Monday 04 June 2007, Michael Ellerman wrote:
> > + for_each_compatible_node(node, NULL, "ibm,axon-msic") {
> > + if (axon_msi_setup_one(of_node_get(node)) == 0)
> > + found++;
> > + }
> > + of_node_put(node);
>
> One more thing: AFAICS 'node' is guaranteed to be NULL when you get
> out of the for_each_compatible_node loop, so you don't need the
> of_node_put().
Yeah it is. I kind of like having the put there, because the idiom is
often to have a for_each_* with a break in it, in which case you need
the put.
But in this case it's a waste so I'll remove it.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 2:53 ` Michael Ellerman
@ 2007-06-08 8:47 ` Segher Boessenkool
2007-06-08 19:26 ` Arnd Bergmann
0 siblings, 1 reply; 31+ messages in thread
From: Segher Boessenkool @ 2007-06-08 8:47 UTC (permalink / raw)
To: michael; +Cc: linuxppc-dev, Arnd Bergmann
>>> + for_each_compatible_node(node, NULL, "ibm,axon-msic") {
>>> + if (axon_msi_setup_one(of_node_get(node)) == 0)
>>> + found++;
>>> + }
>>> + of_node_put(node);
So where is the of_node_put() done for the of_node_get()
inside the loop?
Segher
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 8:47 ` Segher Boessenkool
@ 2007-06-08 19:26 ` Arnd Bergmann
2007-06-08 19:36 ` Segher Boessenkool
0 siblings, 1 reply; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-08 19:26 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
On Friday 08 June 2007, Segher Boessenkool wrote:
> >>> + =A0 =A0 =A0 for_each_compatible_node(node, NULL, "ibm,axon-msic") {
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (axon_msi_setup_one(of_node_get(node=
)) =3D=3D 0)
> >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 found++;
> >>> + =A0 =A0 =A0 }
> >>> + =A0 =A0 =A0 of_node_put(node);
>=20
> So where is the of_node_put() done for the of_node_get()
> inside the loop?
of_find_compatible_node does an of_node_put() on the device_node
that you pass in as the 'from' argument. In the last step, 'node'
is set to NULL and we put the previous element.
Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 19:26 ` Arnd Bergmann
@ 2007-06-08 19:36 ` Segher Boessenkool
2007-06-08 19:59 ` Segher Boessenkool
2007-06-08 20:01 ` Arnd Bergmann
0 siblings, 2 replies; 31+ messages in thread
From: Segher Boessenkool @ 2007-06-08 19:36 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
>>>>> + =A0 =A0 =A0 for_each_compatible_node(node, NULL, =
"ibm,axon-msic") {
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if =
(axon_msi_setup_one(of_node_get(node)) =3D=3D 0)
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 found++;
>>>>> + =A0 =A0 =A0 }
>>>>> + =A0 =A0 =A0 of_node_put(node);
>>
>> So where is the of_node_put() done for the of_node_get()
>> inside the loop?
>
> of_find_compatible_node does an of_node_put() on the device_node
> that you pass in as the 'from' argument. In the last step, 'node'
> is set to NULL and we put the previous element.
That wasn't my question though?
Segher
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 19:36 ` Segher Boessenkool
@ 2007-06-08 19:59 ` Segher Boessenkool
2007-06-08 20:06 ` Arnd Bergmann
2007-06-08 20:01 ` Arnd Bergmann
1 sibling, 1 reply; 31+ messages in thread
From: Segher Boessenkool @ 2007-06-08 19:59 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Arnd Bergmann
>>>>>> + =A0 =A0 =A0 for_each_compatible_node(node, NULL, =
"ibm,axon-msic") {
>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if =
(axon_msi_setup_one(of_node_get(node)) =3D=3D 0)
>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 found++;
>>>>>> + =A0 =A0 =A0 }
>>>>>> + =A0 =A0 =A0 of_node_put(node);
>>>
>>> So where is the of_node_put() done for the of_node_get()
>>> inside the loop?
>>
>> of_find_compatible_node does an of_node_put() on the device_node
>> that you pass in as the 'from' argument. In the last step, 'node'
>> is set to NULL and we put the previous element.
>
> That wasn't my question though?
Oh wait. You're saying for_each_compatible_node() does
a put() on all nodes it traverses, but no get()? Ouch!
Segher
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 19:36 ` Segher Boessenkool
2007-06-08 19:59 ` Segher Boessenkool
@ 2007-06-08 20:01 ` Arnd Bergmann
2007-06-08 20:06 ` Segher Boessenkool
1 sibling, 1 reply; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-08 20:01 UTC (permalink / raw)
To: linuxppc-dev
On Friday 08 June 2007, Segher Boessenkool wrote:
> >> So where is the of_node_put() done for the of_node_get()
> >> inside the loop?
> >
> > of_find_compatible_node does an of_node_put() on the device_node
> > that you pass in as the 'from' argument. In the last step, 'node'
> > is set to NULL and we put the previous element.
>
> That wasn't my question though?
Sorry, I misread this. The of_node_get is done because we keep
a reference to the device node in the axon_msic struct. There is
no module_exit function in the driver that could clean up the
axon_msic, so we must never have an of_node_put as far as I
understand.
Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 20:01 ` Arnd Bergmann
@ 2007-06-08 20:06 ` Segher Boessenkool
2007-06-12 2:30 ` Michael Ellerman
0 siblings, 1 reply; 31+ messages in thread
From: Segher Boessenkool @ 2007-06-08 20:06 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
>> That wasn't my question though?
>
> Sorry, I misread this. The of_node_get is done because we keep
> a reference to the device node in the axon_msic struct. There is
> no module_exit function in the driver that could clean up the
> axon_msic, so we must never have an of_node_put as far as I
> understand.
>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if =
(axon_msi_setup_one(of_node_get(node)) =3D=3D 0)
>>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 found++;
What if axon_msi_setup_one() returns an error? Sounds
to me like the get() should be inside that function no
matter what?
If the reference counting isn't obvious, it is obviously
wrong ;-)
Segher
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 19:59 ` Segher Boessenkool
@ 2007-06-08 20:06 ` Arnd Bergmann
2007-06-08 20:16 ` Segher Boessenkool
0 siblings, 1 reply; 31+ messages in thread
From: Arnd Bergmann @ 2007-06-08 20:06 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
On Friday 08 June 2007, Segher Boessenkool wrote:
> > That wasn't my question though?
>=20
> Oh wait. =A0You're saying for_each_compatible_node() does
> a put() on all nodes it traverses, but no get()? =A0Ouch!
No, just read the code, it does exactly the right thing.
If you do
for_each_compatible_node(node, NULL, "foo") {
do_something(node);
}
then the reference count is held just as long as the do_something()
function is run, and if you do
for_each_compatible_node(node, NULL, "foo")
if (conditional_func(node))
break;
then the node variable will have the reference count.
Arnd <><
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 20:06 ` Arnd Bergmann
@ 2007-06-08 20:16 ` Segher Boessenkool
2007-06-12 2:32 ` Michael Ellerman
0 siblings, 1 reply; 31+ messages in thread
From: Segher Boessenkool @ 2007-06-08 20:16 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev
> If you do
>
> for_each_compatible_node(node, NULL, "foo") {
> do_something(node);
> }
>
> then the reference count is held just as long as the do_something()
> function is run,
Yeah. so (like we discussed on irc) the of_node_get() in
the code under discussion should really be done inside the
axon_msi_setup_one() function.
> and if you do
>
> for_each_compatible_node(node, NULL, "foo")
> if (conditional_func(node))
> break;
>
> then the node variable will have the reference count.
Yes. This is bad form though, "for_each" means for
_each_, open coding the loop would be clearer IMHO.
Segher
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 20:06 ` Segher Boessenkool
@ 2007-06-12 2:30 ` Michael Ellerman
0 siblings, 0 replies; 31+ messages in thread
From: Michael Ellerman @ 2007-06-12 2:30 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Arnd Bergmann
[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]
On Fri, 2007-06-08 at 22:06 +0200, Segher Boessenkool wrote:
> >> That wasn't my question though?
> >
> > Sorry, I misread this. The of_node_get is done because we keep
> > a reference to the device node in the axon_msic struct. There is
> > no module_exit function in the driver that could clean up the
> > axon_msic, so we must never have an of_node_put as far as I
> > understand.
>
> >>>>>> + if (axon_msi_setup_one(of_node_get(node)) == 0)
> >>>>>> + found++;
>
> What if axon_msi_setup_one() returns an error? Sounds
> to me like the get() should be inside that function no
> matter what?
It drops the ref if there's an error.
> If the reference counting isn't obvious, it is obviously
> wrong ;-)
But if it is obvious it may well be unobviously wrong, which is
better? ;)
I'll rework it.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems
2007-06-08 20:16 ` Segher Boessenkool
@ 2007-06-12 2:32 ` Michael Ellerman
0 siblings, 0 replies; 31+ messages in thread
From: Michael Ellerman @ 2007-06-12 2:32 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, Arnd Bergmann
[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]
On Fri, 2007-06-08 at 22:16 +0200, Segher Boessenkool wrote:
> > If you do
> >
> > for_each_compatible_node(node, NULL, "foo") {
> > do_something(node);
> > }
> >
> > then the reference count is held just as long as the do_something()
> > function is run,
>
> Yeah. so (like we discussed on irc) the of_node_get() in
> the code under discussion should really be done inside the
> axon_msi_setup_one() function.
Yep.
> > and if you do
> >
> > for_each_compatible_node(node, NULL, "foo")
> > if (conditional_func(node))
> > break;
> >
> > then the node variable will have the reference count.
>
> Yes. This is bad form though, "for_each" means for
> _each_, open coding the loop would be clearer IMHO.
No. That just means more places people can get the refcounting wrong.
If in doubt, just do:
for_each_compatible_node(node, NULL, "foo") {
..
}
...
of_node_put(node);
And then you're free to do whatever inside the loop, including breaking
out.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2007-06-12 2:32 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-06-04 12:59 [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Michael Ellerman
2007-06-04 13:00 ` [PATCH 2/4] Add irq_create_direct_mapping() Michael Ellerman
2007-06-04 22:44 ` Benjamin Herrenschmidt
2007-06-04 13:00 ` [PATCH 3/4] Add for_each_compatible_node() Michael Ellerman
2007-06-04 22:44 ` Benjamin Herrenschmidt
2007-06-04 13:00 ` [RFC/PATCH 4/4] Add support for MSI on Axon-based Cell systems Michael Ellerman
2007-06-04 17:09 ` Arnd Bergmann
2007-06-04 17:12 ` Arnd Bergmann
2007-06-08 2:53 ` Michael Ellerman
2007-06-08 8:47 ` Segher Boessenkool
2007-06-08 19:26 ` Arnd Bergmann
2007-06-08 19:36 ` Segher Boessenkool
2007-06-08 19:59 ` Segher Boessenkool
2007-06-08 20:06 ` Arnd Bergmann
2007-06-08 20:16 ` Segher Boessenkool
2007-06-12 2:32 ` Michael Ellerman
2007-06-08 20:01 ` Arnd Bergmann
2007-06-08 20:06 ` Segher Boessenkool
2007-06-12 2:30 ` Michael Ellerman
2007-06-04 22:45 ` Benjamin Herrenschmidt
2007-06-05 7:00 ` Jean-Christophe Dubois
2007-06-05 9:16 ` Arnd Bergmann
2007-06-05 9:29 ` Jean-Christophe Dubois
2007-06-05 9:30 ` Arnd Bergmann
2007-06-07 7:58 ` Michael Ellerman
2007-06-07 8:10 ` Jean-Christophe Dubois
2007-06-07 11:55 ` Benjamin Herrenschmidt
2007-06-07 12:00 ` Jean-Christophe Dubois
2007-06-05 14:12 ` Milton Miller
2007-06-06 5:19 ` Michael Ellerman
2007-06-04 22:43 ` [PATCH 1/4] Split virq setup logic out into irq_setup_virq() Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).