From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from moutng.kundenserver.de (moutng.kundenserver.de [212.227.126.186]) by ozlabs.org (Postfix) with ESMTP id A33C1DE7CA for ; Fri, 22 Aug 2008 18:21:52 +1000 (EST) From: Stefan Roese To: linuxppc-embedded@ozlabs.org Subject: Re: [PATCH] Add AMCC 4XX PCIe MSI support Date: Fri, 22 Aug 2008 10:21:47 +0200 References: <1219380144-17175-1-git-send-email-fkan@amcc.com> In-Reply-To: <1219380144-17175-1-git-send-email-fkan@amcc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Message-Id: <200808221021.47253.sr@denx.de> Cc: Preetesh Parekh , fkan@amcc.com List-Id: Linux on Embedded PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Friday 22 August 2008, fkan@amcc.com wrote: > From: Preetesh Parekh Thanks. First of all, this does not apply on the current Linux kernel version (2.6.27-rc4). I fixed this manually and tried the patch on my Kilauea. It does not work. Something seems to be missing. Don't we need a device-tree integration (MSI node)? How did you test this BTW? Please find more comments below. > Signed-off-by: Preetesh Parekh > --- > arch/powerpc/platforms/40x/kilauea.c | 13 ++++ > arch/powerpc/platforms/44x/canyonlands.c | 14 ++++ > arch/powerpc/sysdev/ppc4xx_pci.c | 115 > ++++++++++++++++++++++++++++++ arch/powerpc/sysdev/ppc4xx_pci.h | > 45 ++++++++++++ > include/asm-powerpc/dcr-native.h | 12 +++ > 5 files changed, 199 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/platforms/40x/kilauea.c > b/arch/powerpc/platforms/40x/kilauea.c index 1dd24ff..3610be2 100644 > --- a/arch/powerpc/platforms/40x/kilauea.c > +++ b/arch/powerpc/platforms/40x/kilauea.c > @@ -22,6 +22,11 @@ > #include > #include > > +#ifdef CONFIG_PCI_MSI > +extern int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); > +extern void ppc4xx_teardown_msi_irqs(struct pci_dev *dev); > +#endif > + > static __initdata struct of_device_id kilauea_of_bus[] = { > { .compatible = "ibm,plb4", }, > { .compatible = "ibm,opb", }, > @@ -46,6 +51,14 @@ static int __init kilauea_probe(void) > > ppc_pci_flags = PPC_PCI_REASSIGN_ALL_RSRC; > > +#ifdef CONFIG_PCI_MSI > + /* > + * Setting callback functions for MSI support > + */ > + ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs; > + ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs; > +#endif This belongs into the (to be written) of_platform driver/interface for the MSI support. > return 1; > } > > diff --git a/arch/powerpc/platforms/44x/canyonlands.c > b/arch/powerpc/platforms/44x/canyonlands.c index 3949289..ee38fb7 100644 > --- a/arch/powerpc/platforms/44x/canyonlands.c > +++ b/arch/powerpc/platforms/44x/canyonlands.c > @@ -25,6 +25,12 @@ > #include > #include > > + > +#ifdef CONFIG_PCI_MSI > +extern int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); > +extern void ppc4xx_teardown_msi_irqs(struct pci_dev *dev); > +#endif > + > static __initdata struct of_device_id canyonlands_of_bus[] = { > { .compatible = "ibm,plb4", }, > { .compatible = "ibm,opb", }, > @@ -49,6 +55,14 @@ static int __init canyonlands_probe(void) > > ppc_pci_flags = PPC_PCI_REASSIGN_ALL_RSRC; > > +#ifdef CONFIG_PCI_MSI > + /* > + * Setting callback functions for MSI support > + */ > + ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs; > + ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs; > +#endif > + > return 1; > } > > diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c > b/arch/powerpc/sysdev/ppc4xx_pci.c index fb368df..bdb3663 100644 > --- a/arch/powerpc/sysdev/ppc4xx_pci.c > +++ b/arch/powerpc/sysdev/ppc4xx_pci.c > @@ -35,6 +35,11 @@ > > static int dma_offset_set; > > +#ifdef CONFIG_PCI_MSI > +#include > +#include "../../../drivers/pci/msi.h" Is this really needed? > +#endif > + > /* Move that to a useable header */ > extern unsigned long total_memory; > > @@ -1587,12 +1592,26 @@ static void __init > ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port) out_le16(mbase > + 0x202, val); > > if (!port->endpoint) { > +#ifdef CONFIG_PCI_MSI > + /* Set MSI enable, multiple msg cap = 4 */ > + /* 4 messages allocated, 64 bit capability */ > + out_le32(mbase + 0x048, in_le32(mbase + 0x048) | 0x00a50000); > + > + /* Enable interrupts in BCR */ > + out_le32(mbase + 0x03C, in_le32(mbase + 0x03C) | 0xFF000000); > +#endif > + > /* Set Class Code to PCI-PCI bridge and Revision Id to 1 */ > out_le32(mbase + 0x208, 0x06040001); > > printk(KERN_INFO "PCIE%d: successfully set as root-complex\n", > port->index); > } else { > +#ifdef CONFIG_PCI_MSI > + /* Enable MSI for End-Point */ > + out_le32(mbase + 0x048, (in_le32(mbase + 0x048) | 0x00a50000)); > +#endif > + > /* Set Class Code to Processor/PPC */ > out_le32(mbase + 0x208, 0x0b200001); > > @@ -1704,6 +1723,97 @@ static void __init ppc4xx_probe_pciex_bridge(struct > device_node *np) ppc4xx_pciex_port_setup_hose(port); > } > > +#ifdef CONFIG_PCI_MSI > +int ppc4xx_setup_peih(void) > +{ > + void __iomem *peih_base; > + > + printk(KERN_INFO " %s \n",__FUNCTION__); This seems to be a debug output. > + /* Set base address for PEIH and enable PEIH */ > + SDR_WRITE(PESDR0_4XX_IHS1, PPC4XX_PEIH_REGBASE_HADDR); > + SDR_WRITE(PESDR0_4XX_IHS2, PPC4XX_PEIH_REGBASE_LADDR); The SDR_WRITE marcos are relict's from arch/ppc. Don't use them in arch/powerpc. We have other functions to deal with indirect DCR's now: mfdcri(SDR0, ...) and mtdcri(SDR0, ....) And those PPC4XX_PEIH_REGBASE_xADDR defines should be removed. Those values need to be configured in the device-tree. > + /* Map in PCI-E Interrupt Handler Registers */ > + peih_base = ioremap(PPC4XX_PEIH_REGBASE, PPC4XX_PEIH_REGSIZE); > + if(!peih_base) { > + printk(KERN_ERR "%s: ioremap64 failed for addr 0x%08x\n", ioremap64()? Also a relict from arch/ppc. > + __FUNCTION__, PPC4XX_PEIH_REGBASE); > + return -ENODEV; > + } > + > + /* Progam the Interrupt handler Termination addr registers */ > + out_be32(peih_base + PEIH_TERMADH, PPC4XX_PCIE_MSI_HADDR); > + out_be32(peih_base + PEIH_TERMADL, PPC4XX_PCIE_MSI_LADDR); > + > + /* Program MSI Expected data and Mask bits */ > + out_be32(peih_base + PEIH_MSIED, PPC4XX_MSI_DATA_PTRN); > + out_be32(peih_base + PEIH_MSIMK, PPC4XX_MSI_DATA_VALD); Again, those defines should probably be converted to device-tree properties. > + iounmap(peih_base); > + > + return 0; /* success */ > +} > + > + > +int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) > +{ > + /* Setup MSI Capabilities structure for dev func */ > + int pos; > + u16 control, ctrl; > + > + printk(KERN_INFO " %s \n",__FUNCTION__); > + pos = desc->msi_attrib.pos; > + pci_read_config_word(dev, msi_control_reg(pos), &control); > + > + /* Multiple msg enable */ > + ctrl = multi_msi_enable(control, 4); > + pci_write_config_dword(dev, msi_control_reg(pos), ctrl); > + > + /* setup MSI address registers */ > + if(is_64bit_address(control)) > + pci_write_config_dword(dev, msi_upper_address_reg(pos), > PPC4XX_PCIE_MSI_HADDR); + > + pci_write_config_dword(dev, msi_lower_address_reg(pos), > + PPC4XX_PCIE_MSI_LADDR); > + > + if(is_64bit_address(control)) { Nitpick: if (is_64bit_address(control)) { > + /* setup MSI data register */ > + pci_write_config_dword(dev, msi_data_reg(pos, 1), > + PPC4XX_MSI_DATA_PTRN_EP); > + /* setup MSI mask bits reg */ > + pci_write_config_dword(dev, msi_mask_bits_reg(pos, 1), 0x00000000); > + } > + else { } else { > + pci_write_config_dword(dev, msi_data_reg(pos, 0), > + PPC4XX_MSI_DATA_PTRN_EP); > + pci_write_config_dword(dev, msi_mask_bits_reg(pos, 0), 0x00000000); > + } > + > + return 0; > +} > + > +int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) > +{ > + struct msi_desc *entry; > + int ret; > + > + list_for_each_entry(entry, &dev->msi_list, list) { > + ret = arch_setup_msi_irq(dev, entry); > + if (ret) > + return ret; > + } This seems to be incomplete. Things like "set_irq_msi()" are missing. > + return 0; > +} > + > +void ppc4xx_teardown_msi_irqs(struct pci_dev *dev) > +{ > + return; This needs to get filled with "life" too. > +} > + > + > +#endif /* CONFIG_PCI_MSI */ > + > #endif /* CONFIG_PPC4xx_PCI_EXPRESS */ > > static int __init ppc4xx_pci_find_bridges(void) > @@ -1713,6 +1823,11 @@ static int __init ppc4xx_pci_find_bridges(void) > #ifdef CONFIG_PPC4xx_PCI_EXPRESS > for_each_compatible_node(np, NULL, "ibm,plb-pciex") > ppc4xx_probe_pciex_bridge(np); > + > +#ifdef CONFIG_PCI_MSI > + ppc4xx_setup_peih(); > +#endif > + > #endif > for_each_compatible_node(np, NULL, "ibm,plb-pcix") > ppc4xx_probe_pcix_bridge(np); > diff --git a/arch/powerpc/sysdev/ppc4xx_pci.h > b/arch/powerpc/sysdev/ppc4xx_pci.h index d04e40b..d5fcaa9 100644 > --- a/arch/powerpc/sysdev/ppc4xx_pci.h > +++ b/arch/powerpc/sysdev/ppc4xx_pci.h > @@ -158,6 +158,51 @@ > #define GPL_DMER_MASK_DISA 0x02000000 > > /* > + * 4xx PCIe IRQ Handler register definitions > + */ > +#ifdef CONFIG_PCI_MSI > + > +#if defined(CONFIG_405EX) > +#define PESDR0_4XX_IHS1 0x04B0 > +#define PESDR0_4XX_IHS2 0x04B1 > +#define PPC4XX_PEIH_REGBASE 0x0EF620000 > +#define PPC4XX_PEIH_REGBASE_HADDR 0x0 > +#define PPC4XX_PEIH_REGBASE_LADDR 0xEF620000 > +#define PPC4XX_PCIE_MSI_ADDR 0x080000000 > +#define PPC4XX_PCIE_MSI_HADDR 0x0 > +#define PPC4XX_PCIE_MSI_LADDR 0x80000000 > + > +#elif defined(CONFIG_460EX) > +#define PESDR0_4XX_IHS1 0x036C > +#define PESDR0_4XX_IHS2 0x036D > +#define PPC4XX_PEIH_REGBASE 0xC10000000 > +#define PPC4XX_PEIH_REGBASE_HADDR 0xC > +#define PPC4XX_PEIH_REGBASE_LADDR 0x10000000 > +#define PPC4XX_PCIE_MSI_ADDR 0xe80000000 > +#define PPC4XX_PCIE_MSI_HADDR 0xe > +#define PPC4XX_PCIE_MSI_LADDR 0x80000000 > +#endif Again, most of those defines need to be converted to device-tree regs/properties. > +#define PPC4XX_PEIH_REGSIZE 0x100 > +#define PPC4XX_MSI_DATA_PTRN 0x44440000 > +#define PPC4XX_MSI_DATA_VALD 0xFFFF0000 > +#define PPC4XX_MSI_DATA_PTRN_EP 0x00004444 > + > +#define PEIH_TERMADH 0x00 > +#define PEIH_TERMADL 0x08 > +#define PEIH_MSIED 0x10 > +#define PEIH_MSIMK 0x18 > +#define PEIH_MSIASS 0x20 > +#define PEIH_FLUSH0 0x30 > +#define PEIH_FLUSH1 0x38 > +#define PEIH_CNTRST 0x48 > + > + > +#endif /* CONFIG_PCI_MSI */ > + > + > + > +/* > * System DCRs (SDRs) > */ > #define PESDR0_PLLLCT1 0x03a0 > diff --git a/include/asm-powerpc/dcr-native.h > b/include/asm-powerpc/dcr-native.h index 72d2b72..357ba36 100644 > --- a/include/asm-powerpc/dcr-native.h > +++ b/include/asm-powerpc/dcr-native.h > @@ -111,6 +111,18 @@ static inline void __dcri_clrset(int base_addr, int > base_data, int reg, DCRN_ ## base ## _CONFIG_DATA, \ > reg, clr, set) > > +/* SDR read/write helper macros */ > + > +#define DCRN_SDR_CONFIG_ADDR 0x00E > +#define DCRN_SDR_CONFIG_DATA 0x00F > + > +#define SDR_READ(offset) ({ \ > + mtdcr(DCRN_SDR_CONFIG_ADDR, offset); \ > + mfdcr(DCRN_SDR_CONFIG_DATA);}) > +#define SDR_WRITE(offset, data) ({ \ > + mtdcr(DCRN_SDR_CONFIG_ADDR, offset); \ > + mtdcr(DCRN_SDR_CONFIG_DATA, data);}) Don't add this ancient stuff here. Use the new functions as described above. Thanks. Best regards, Stefan