Re: [PATCH][2/2] RTAS MSI

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3216 bytes --]

On Mon, 2006-07-31 at 16:01 -0500, Jake Moilanen wrote:
> Here's version 3 which addresses all of Michael's comments.

Sorry, more questions :)

> Index: 2.6-msi/drivers/pci/msi-rtas.c
> ===================================================================
> --- /dev/null
> +++ 2.6-msi/drivers/pci/msi-rtas.c
> @@ -0,0 +1,99 @@
> +/*
> + * Jake Moilanen <moilanen@austin.ibm.com>
> + * Copyright (C) 2006 IBM
> + *
> + * 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 of the
> + * License.
> + *
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <asm/rtas.h>
> +#include <asm/hw_irq.h>
> +#include <asm/ppc-pci.h>
> +
> +int rtas_enable_msi(struct pci_dev* pdev)
> +{
> +	int seq_num = 1;
> +	int i;
> +	int rc;
> +	int query_token = rtas_token("ibm,query-interrupt-source-number");
> +	int ret[2];
> +	int n_intr;
> +	int last_virq = NO_IRQ;
> +	int virq;
> +	unsigned int addr;
> +	unsigned long buid;
> +	struct device_node * dn;
> +
> +	dn = pci_device_to_OF_node(pdev);
> +
> +	if (!of_find_property(dn, "ibm,req#msi", NULL))
> +		return -ENOENT;
> +
> +	buid = get_phb_buid(dn->parent);
> +	addr = (pdev->bus->number << 16) | (pdev->devfn << 8);
> +
> +	do {
> +		rc = rtas_call(rtas_token("ibm,change-msi"), 6, 3, ret, addr,
> +			       buid >> 32, buid & 0xffffffff,
> +			       0, 0, seq_num);
> +
> +		seq_num = ret[1];
> +	} while (rtas_busy_delay(rc));
> +
> +	if (rc)	{
> +		printk(KERN_WARNING "error[%d]: getting the number of "
> +		       "MSI interrupts for %s\n", rc, dn->name);
> +		return -EIO;
> +	}
> +
> +	/* Return if there's no MSI interrupts */
> +	if (!ret[0])
> +		return -ENOENT;
> +
> +	n_intr = ret[0];
> +
> +	for (i = 0; i < n_intr; i++) {
> +		do {
> +			rc = rtas_call(query_token, 4, 3, ret, addr,
> +				       buid >> 32, buid & 0xffffffff, i);
> +		} while (rtas_busy_delay(rc));
> +
> +		if (!rc) {
> +			virq = irq_create_mapping(irq_find_host(dn), ret[0],
> +						ret[1] ? IRQ_TYPE_EDGE_RISING :
> +						IRQ_TYPE_LEVEL_LOW);

I'm only just starting to get benh's new irq code, but I think
irq_find_host(dn) isn't doing what we want here. It's probably harmless,
but AFAICT irq_find_host() is only meant to be called when you have the
node of the irq controller, not for an arbitrary dn. The doco's a bit
ambiguous:

 * irq_find_host - Locates a host for a given device node
 * @node: device-tree node of the interrupt controller

But looking at the implementation, it doesn't do a search up the tree or
anything, it just checks node against each host.

Also, since's benh's latest patch went in we'll have to split this into
two calls, I think we want:

virq = irq_create_mapping(NULL ???, ret[0]);
set_irq_type(virq, ret[1] ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_LOW);

cheers

-- 
Michael Ellerman
IBM OzLabs

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: 191 bytes --]