Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933433AbaGVWxc (ORCPT ); Tue, 22 Jul 2014 18:53:32 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:36324 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932203AbaGVWx3 (ORCPT ); Tue, 22 Jul 2014 18:53:29 -0400 Message-ID: <53CEEB1C.9020202@ti.com> Date: Tue, 22 Jul 2014 18:52:12 -0400 From: Murali Karicheri User-Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Bjorn Helgaas CC: , , , Santosh Shilimkar , Russell King , Grant Likely , Rob Herring , Jingoo Han , Richard Zhu , Kishon Vijay Abraham I , Marek Vasut , Arnd Bergmann , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Randy Dunlap Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w References: <1405961925-27248-1-git-send-email-m-karicheri2@ti.com> <1405961925-27248-5-git-send-email-m-karicheri2@ti.com> <20140722223527.GA27965@google.com> In-Reply-To: <20140722223527.GA27965@google.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Bjorn, On 07/22/2014 06:35 PM, Bjorn Helgaas wrote: > On Mon, Jul 21, 2014 at 12:58:44PM -0400, Murali Karicheri wrote: >> keystone PCIe controller is based on v3.65 version of the >> designware h/w. Main differences are >> 1. No ATU support >> 2. Legacy and MSI irq functions are implemented in >> application register space >> 3. MSI interrupts are multiplexed over 8 IRQ lines to the Host >> side. >> All of the Application register space handing code are organized into >> pci-keystone-dw.c and the functions are called from pci-keystone.c >> to implement PCI controller driver. Also add necessary DT documentation >> for the driver. >> >> Signed-off-by: Murali Karicheri >> Acked-by: Santosh Shilimkar >> ... > >> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt >> ... > >> +Note for PCI driver usage >> +========================= >> +Driver requires pci=pcie_bus_perf in the bootargs for proper functioning. > > Whoa, why is this? Special boot args should not be required. This was discussed initially and I had added following commit to get this working instead of a PCI quirk. To get some background please see the thread for commit below that you also had signed off as well. commit 8b5742ad156d30ee38486652cdbd152e2d6ebbcc Author: Murali Karicheri Date: Wed May 28 13:14:53 2014 -0400 ARM/PCI: Call pcie_bus_configure_settings() to set MPS Call pcie_bus_configure_settings() on ARM, like for other platforms. pcie_bus_configure_settings() makes sure the MPS across the bus is uniform and provides the ability to tune the MRSS and MPS to higher performance values. This is particularly important for embedded where there is no firmware to program these PCIe settings for the OS. Signed-off-by: Murali Karicheri Signed-off-by: Bjorn Helgaas CC: Russell King CC: Arnd Bergmann CC: Jason Gunthorpe CC: Santosh Shilimkar This was added as a preparatory patch to support keystone and avoid a PCI quirk to do the same. Keystone has MRSS limitation of 256 bytes. So adding a bootargs flag was suggested a better option than a PCI quirk. I will look into the rest of the comments and possibly try to address them or discuss. BTW, please apply patch 1-3 that has already got ack from maintainers and is indepdent of this patch. Thanks Murali > >> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig >> index 21df477..f8bc475 100644 >> --- a/drivers/pci/host/Kconfig >> +++ b/drivers/pci/host/Kconfig >> @@ -46,4 +46,9 @@ config PCI_HOST_GENERIC >> Say Y here if you want to support a simple generic PCI host >> controller, such as the one emulated by kvmtool. >> >> +config PCI_KEYSTONE >> + bool "TI Keystone PCIe controller" >> + depends on ARCH_KEYSTONE >> + select PCIE_DW >> + select PCIEPORTBUS > > It'd be nice to have some help text here. I know, not everybody else does. > >> +++ b/drivers/pci/host/pci-keystone-dw.c >> ... >> +void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset) >> +{ >> + struct pcie_port *pp =&ks_pcie->pp; >> + u32 pending, vector; >> + int src, virq; >> + >> + pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset<< 4)); > > Blank line here (before the block comment). > >> + /* >> + * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit >> + * shows 1, 9, 17, 25 and so forth >> + */ >> + for (src = 0; src< 4; src++) { >> + if (BIT(src)& pending) { >> + vector = offset + (src<< 3); >> + virq = irq_linear_revmap(pp->irq_domain, vector); >> + dev_dbg(pp->dev, >> + "irq: bit %d, vector %d, virq %d\n", >> + src, vector, virq); >> + generic_handle_irq(virq); >> + } >> + } >> +} >> + >> ... > >> +static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus, >> + unsigned int devfn) >> +{ >> + u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn); >> + struct pcie_port *pp =&ks_pcie->pp; >> + u32 regval; >> + >> + if (bus == 0) >> + return pp->dbi_base; >> + >> + regval = (bus<< 16) | (device<< 8) | function; >> + /* >> + * Since Bus#1 will be a virtual bus, we need to have TYPE0 >> + * access only. >> + * TYPE 1 >> + */ >> + if (bus != 1) >> + regval |= BIT(24); >> + >> + writel(regval, ks_pcie->va_app_base + CFG_SETUP); >> + return pp->va_cfg0_base; >> +} >> + >> +int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, >> + unsigned int devfn, int where, int size, u32 *val) >> +{ >> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); >> + u8 bus_num = bus->number; >> + void __iomem *addr; >> + int ret; >> + >> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn); >> + ret = dw_pcie_cfg_read(addr + (where& ~0x3), where, size, val); > > This *looks* like it needs a lock to protect against concurrent > ks_pcie_cfg_setup() users, since it writes a register. > >> + >> + return ret; > > Please use the same style as in ks_dw_pcie_wr_other_conf(), i.e., get rid > of "ret". > >> +} >> + >> +int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, >> + struct pci_bus *bus, unsigned int devfn, int where, >> + int size, u32 val) >> +{ >> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); >> + u8 bus_num = bus->number; >> + void __iomem *addr; >> + >> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn); >> + >> + return dw_pcie_cfg_write(addr + (where& ~0x3), where, size, val); >> +} > >> +++ b/drivers/pci/host/pci-keystone.c >> ... > >> +static struct platform_driver ks_pcie_driver __refdata = { > > Why does this need to be __refdata? There are no other occurrences in > drivers/pci. > >> + .probe = ks_pcie_probe, >> + .remove = __exit_p(ks_pcie_remove), >> + .driver = { >> + .name = "keystone-pcie", >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(ks_pcie_of_match), >> + }, >> +}; > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/