2006-10-03 21:53:39

by Jake Moilanen

[permalink] [raw]
Subject: Re: [RFC/PATCH 6/7] RTAS MSI implementation

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 <[email protected]>
> ---
>
> 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 <[email protected]>, 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.

> +
> + 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.

> + 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.


> + return -1;
> +}
> +
> +static struct ppc_msi_ops rtas_msi_ops = {
> + .check = msi_rtas_check,
> + .alloc = msi_rtas_alloc,
> + .free = msi_rtas_free
> +};
> +
> +static struct ppc_msi_ops *rtas_get_msi_ops(struct pci_dev *pdev)
> +{
> + return &rtas_msi_ops;
> +}
> +
> +static int msi_rtas_init(void)
> +{
> + query_token = rtas_token("ibm,query-interrupt-source-number");
> + change_token = rtas_token("ibm,change-msi");
> +
> + if ((query_token == RTAS_UNKNOWN_SERVICE) ||
> + (change_token == RTAS_UNKNOWN_SERVICE)) {
> + pr_debug("rtas_msi_init: Couldn't find RTAS tokens, no "
> + "MSI support available.\n");
> + return 0;
> + }
> +
> + pr_debug("rtas_msi_init: Registering RTAS MSI ops.\n");
> +
> + ppc_md.get_msi_ops = rtas_get_msi_ops;
> +
> + return 0;
> +}
> +__initcall(msi_rtas_init);


2006-10-24 02:49:31

by Michael Ellerman

[permalink] [raw]
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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>, 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


Attachments:
signature.asc (191.00 B)
This is a digitally signed message part