Date:	Tue, 24 Oct 2006 12:49:27 +1000
From:	Michael Ellerman <michael@...erman.id.au>
To:	Jake Moilanen <moilanen@...tin.ibm.com>
Cc:	linux-kernel@...r.kernel.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linuxppc-dev@...abs.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [RFC/PATCH 6/7] RTAS MSI implementation

On Tue, 2006-10-03 at 16:53 -0500, Jake Moilanen wrote:
> On Fri, 2006-09-29 at 07:53 +1000, Michael Ellerman wrote:
> > Powerpc MSI support via RTAS. Based on Jake's code.
> > 
> > Signed-off-by: Michael Ellerman <michael@...erman.id.au>
> > ---
> > 
> >  arch/powerpc/kernel/Makefile   |    1 
> >  arch/powerpc/kernel/msi-rtas.c |  246 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 247 insertions(+)
> > 
> > Index: to-merge/arch/powerpc/kernel/Makefile
> > ===================================================================
> > --- to-merge.orig/arch/powerpc/kernel/Makefile
> > +++ to-merge/arch/powerpc/kernel/Makefile
> > @@ -69,6 +69,7 @@ pci32-$(CONFIG_PPC32)		:= pci_32.o
> >  obj-$(CONFIG_PCI)		+= $(pci64-y) $(pci32-y)
> >  
> >  msiobj-y			:= msi.o
> > +msiobj-$(CONFIG_PPC_PSERIES)	+= msi-rtas.o
> >  obj-$(CONFIG_PCI_MSI)		+= $(msiobj-y)
> >  
> >  kexec-$(CONFIG_PPC64)		:= machine_kexec_64.o
> > Index: to-merge/arch/powerpc/kernel/msi-rtas.c
> > ===================================================================
> > --- /dev/null
> > +++ to-merge/arch/powerpc/kernel/msi-rtas.c
> > @@ -0,0 +1,246 @@
> > +/*
> > + * Copyright (C) 2006 Jake Moilanen <moilanen@...tin.ibm.com>, IBM Corp.
> > + * Copyright (C) 2006 Michael Ellerman, IBM Corp.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#define DEBUG 1
> > +
> > +#include <linux/irq.h>
> > +#include <asm/msi.h>
> > +#include <asm/rtas.h>
> > +#include <asm/hw_irq.h>
> > +#include <asm/ppc-pci.h>
> > +
> > +static int query_token, change_token;
> > +
> > +#define RTAS_QUERY_MSI_FN	0
> > +#define RTAS_CHANGE_MSI_FN	1
> > +#define RTAS_RESET_MSI_FN	2
> > +
> > +
> > +/* RTAS Helpers */
> > +
> > +static int rtas_change_msi(struct pci_dn *pdn, u32 function, u32 num_irqs)
> > +{
> > +	u32 addr, seq_num, rtas_ret[2];
> > +	unsigned long buid;
> > +	int rc;
> > +
> > +	addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
> > +	buid = pdn->phb->buid;
> > +
> > +	seq_num = 1;
> > +	do {
> > +		rc = rtas_call(change_token, 6, 3, rtas_ret, addr,
> > +				BUID_HI(buid), BUID_LO(buid),
> > +				function, num_irqs, seq_num);
> 
> This call is still currently broken in firmware.  Hopefully we'll have a
> resolution soon.

Any word on that?

> > +
> > +		seq_num = rtas_ret[1];
> > +	} while (rtas_busy_delay(rc));
> > +
> > +	if (rc) {
> > +		printk(KERN_WARNING "Error[%d]: getting the number of"
> > +			" MSI interrupts for %s\n", rc, pci_name(pdn->pcidev));
> > +		return rc;
> > +	}
> > +
> > +	return rtas_ret[0];
> > +}
> > +
> > +static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
> > +{
> > +	u32 addr, rtas_ret[2];
> > +	unsigned long buid;
> > +	int rc;
> > +
> > +	addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
> > +	buid = pdn->phb->buid;
> > +
> > +	do {
> > +		rc = rtas_call(query_token, 4, 3, rtas_ret, addr,
> > +			       BUID_HI(buid), BUID_LO(buid), offset);
> > +	} while (rtas_busy_delay(rc));
> > +
> > +	if (rc) {
> > +		printk(KERN_WARNING "Error[%d]: Querying irq source number "
> > +				"for %s\n", rc, pci_name(pdn->pcidev));
> > +		return rc;
> > +	}
> > +
> > +	return rtas_ret[0];
> > +}
> > +
> > +/*
> > + * The spec gives firmware the option to enable either MSI or MSI-X,
> > + * this doesn't wash with the Linux API. For the time beinging, we
> > + * kludge around that by checking ourselves the right type is enabled.
> > + */
> > +static int check_msi_type(struct pci_dev *pdev, int type)
> > +{
> > +	int pos, msi_enabled, msix_enabled;
> > +	u16 reg;
> > +
> > +	pos = pci_find_capability(pdev, PCI_CAP_ID_MSI);
> > +	if (!pos)
> > +		return -1;
> > +
> > +	pci_read_config_word(pdev, pos + PCI_MSI_FLAGS, &reg);
> > +
> > +	msi_enabled = msix_enabled = 0;
> > +
> > +	if (reg & PCI_MSI_FLAGS_ENABLE)
> > +		msi_enabled = 1;
> > +
> 
> This is not being set correctly by firmware either. I have them looking
> into the problem.

Are you sure? I don't see how MSI could possibly work if they don't
enable it on the device itself. Or is it completely broken at the
moment?

> 
> > +	if (reg & PCI_MSIX_FLAGS_ENABLE)
> > +		msix_enabled = 1;
> > +
> > +	if (type == PCI_CAP_ID_MSI && (msix_enabled || !msi_enabled)) {
> > +		pr_debug("check_msi_type: Expected MSI but got %s.\n",
> > +			msix_enabled ? "MSI-X" : "none");
> > +		return -1;
> > +	}
> > +
> > +	if (type == PCI_CAP_ID_MSIX && (msi_enabled || !msix_enabled)) {
> > +		pr_debug("check_msi_type: Expected MSI-X but got %s.\n",
> > +			msi_enabled ? "MSI" : "none");
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void msi_rtas_free(struct pci_dev *pdev, int num,
> > +			struct msix_entry *entries, int type)
> > +{
> > +	struct device_node *dn;
> > +	struct pci_dn *pdn;
> > +	int i;
> > +
> > +	dn = pci_device_to_OF_node(pdev);
> > +	if (!dn) {
> > +		pr_debug("msi_rtas_free: No OF device node for %s\n",
> > +				pci_name(pdev));
> > +		return;
> > +	}
> > +
> > +	pdn = PCI_DN(dn);
> > +	if (!pdn) {
> > +		pr_debug("msi_rtas_free: No PCI DN for %s\n",
> > +				pci_name(pdev));
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i < num; i++) {
> > +		irq_dispose_mapping(entries[i].vector);
> > +	}
> > +
> > +	/* XXX can we do anything sane if this fails? */
> > +	rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0);
> > +}
> > +
> > +static int msi_rtas_check(struct pci_dev *pdev, int num,
> > +			struct msix_entry *entries, int type)
> > +{
> > +	struct device_node *dn;
> > +	int i;
> > +
> > +	dn = pci_device_to_OF_node(pdev);
> > +
> > +	if (!of_find_property(dn, "ibm,req#msi", NULL)) {
> > +		pr_debug("msi_rtas_check: No ibm,req#msi for %s\n",
> > +				pci_name(pdev));
> > +		return -1;
> > +	}
> > +
> > +	/*
> > +	 * Firmware gives us no control over which entries are allocated
> > +	 * for MSI-X, it seems to assume we want 0 - n. For now just insist
> > +	 * that the entries array entry members are 0 - n.
> > +	 */
> > +	for (i = 0; i < num; i++) {
> > +		if (entries[i].entry != i) {
> > +			pr_debug("msi_rtas_check: entries[i].entry != i\n");
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int msi_rtas_alloc(struct pci_dev *pdev, int num,
> > +			struct msix_entry *entries, int type)
> > +{
> > +	struct pci_dn *pdn;
> > +	int hwirq, virq, i;
> > +
> > +	pdn = PCI_DN(pci_device_to_OF_node(pdev));
> > +
> > +	/*
> > +	 * In the case of an error it's not clear whether the device is left
> > +	 * with MSI enabled or not, I think we should explicitly disable.
> > +	 */
> > +	if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, num) != num)
> > +		goto out_free;
> > +
> > +	if (check_msi_type(pdev, type))
> > +		goto out_free;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		hwirq = rtas_query_irq_number(pdn, i);
> > +		if (hwirq < 0)
> > +			goto out_free;
> > +
> > +		virq = irq_create_mapping(NULL, hwirq);
> > +
> > +		if (virq == NO_IRQ) {
> > +			pr_debug("msi_rtas_alloc: Failed mapping hwirq %d\n",
> > +				hwirq);
> > +			goto out_free;
> > +		}
> > +
> > +		entries[i].vector = virq;
> > +	}
> > +
> > +	return 0;
> > +
> > + out_free:
> > +	msi_rtas_free(pdev, num, entries, type);
> 
> Shouldn't this be:
> 
> 	msi_rtas_free(pdev, i,.......
> 
> Otherwise you'll try freeing unallocated entries.

Yeah maybe. I think I was intending that free be written in such a way
that it could cope with tearing down a partial setup, but I never got
round to writing the patch for irq_dispose_mapping() to make that work -
I should do that.

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

Download attachment "signature.asc" of type "application/pgp-signature" (192 bytes)

Powered by blists - more mailing lists