| Message ID | 1400169692-9677-6-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote: > +static int > +keystone_pcie_fault(unsigned long addr, unsigned int fsr, > + struct pt_regs *regs) > +{ > + unsigned long instr = *(unsigned long *) instruction_pointer(regs); > + > + if ((instr & 0x0e100090) == 0x00100090) { > + int reg = (instr >> 12) & 15; > + > + regs->uregs[reg] = -1; > + regs->ARM_pc += 4; > + } > + > + return 0; > +} This needs to check in the PCI host registers what happened and do some appropriate action. If the fault was not caused by PCIe, it should not be ignored here but get passed on to the next handler. > +static int __init ks_pcie_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device *dev = &pdev->dev; > + struct keystone_pcie *ks_pcie; > + void __iomem *devstat; > + struct pcie_port *pp; > + struct resource *res; > + struct phy *phy; > + int ret = 0; > + u32 val; > + > + ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie), > + GFP_KERNEL); > + if (!ks_pcie) { > + dev_err(dev, "no memory for keystone pcie\n"); > + return -ENOMEM; > + } > + > + /* check if serdes phy needs to be enabled */ > + if (of_get_property(np, "ti,init-phy", NULL) != NULL) { > + phy = devm_phy_get(dev, "pcie-phy"); > + if (IS_ERR(phy)) > + return PTR_ERR(phy); > + > + ret = phy_init(phy); > + if (ret < 0) > + return ret; > + } I think you can just call devm_phy_get() unconditionally here and get rid of the ti,init-phy property. If there is no phy, don't initialize it. > + > + ret = add_pcie_port(ks_pcie, pdev); > + if (ret < 0) > + goto fail_clk; > + > + platform_set_drvdata(pdev, ks_pcie); Set the platform data first, then add the port. > + dev_info(dev, "pcie rc probe success\n"); Remove this. > +#ifdef CONFIG_PCI_KEYSTONE > +/* > + * The KeyStone PCIe controller has maximum read request size of 256 bytes. > + */ > +static void quirk_limit_readrequest(struct pci_dev *dev) > +{ > + int readrq = pcie_get_readrq(dev); > + > + if (readrq > 256) > + pcie_set_readrq(dev, 256); > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > +#endif /* CONFIG_PCI_KEYSTONE */ This doesn't work: you can't just limit do this for all devices just based on PCI_KEYSTONE being enabled, you have to check if you are actually using this controller. Arnd
On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote: > +Sample bindings shown below:- > + > + - Remove ti,enable-linktrain if boot loader already does Link training and do EP > + configuration. > + - Remove ti,init-phy if boot loader already initialize the phy and sets up pcie > + link You actually have to document all the properties. Have a look at the other bindings for what this means. > + > + pcie@21800000 { > + compatible = "ti,keystone-pcie"; > + device_type = "pci"; > + clocks = <&clkpcie>; > + clock-names = "pcie"; The dw-pcie binding lists two clocks. > + #address-cells = >; > + #size-cells = <2>; > + reg = <0x21800000 0x1000>, <0x0262014c 4>; > + reg-names = "reg_rc_app", "reg_devcfg"; There should be standard names that are documented in the binding. > + interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; /* Error IRQ */ What are all these interrupts? > + ranges = <0x00000800 0 0x21801000 0x21801000 0 0x0002000 /* Configuration space */ > + 0x81000000 0 0 0x24000000 0 0x4000 /* downstream I/O */ > + 0x82000000 0 0x50000000 0x50000000 0 0x10000000>; /* non-prefetchable memory */ Please move the config space into 'reg' now instead of 'ranges'. This was a mistake earlier, and there are already other front-ends doing the same thing. > + num-lanes = <2>; > + ti,enable-linktrain; > + ti,init-phy; > + > + /* PCIE phy */ > + phys = <&pcie0_phy>; > + phy-names = "pcie-phy"; > + > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0>; > + interrupt-map = <0 0 0 1 &pcie_intc 1>, // INT A > + <0 0 0 2 &pcie_intc 2>, // INT B > + <0 0 0 3 &pcie_intc 3>, // INT C > + <0 0 0 4 &pcie_intc 4>; // INT D I think this is the wrong map-mask. Arnd
Arnd, Thanks for the review. I may have more questions as I digest the comments. Here is the immediate one. >> +#ifdef CONFIG_PCI_KEYSTONE >> +/* >> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. >> + */ >> +static void quirk_limit_readrequest(struct pci_dev *dev) >> +{ >> + int readrq = pcie_get_readrq(dev); >> + >> + if (readrq > 256) >> + pcie_set_readrq(dev, 256); >> +} >> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); >> +#endif /* CONFIG_PCI_KEYSTONE */ > This doesn't work: you can't just limit do this for all devices just based > on PCI_KEYSTONE being enabled, you have to check if you are actually using > this controller. > > Arnd I assume, I need to check if PCI controller's vendor ID/ device ID match with the keystone PCI controller's ID and call pcie_set_readrq() for all of the slave PCI devices and do this fixup. Is this correct understanding? If you can point me to an example code for this that will be really helpful so that I can avoid re-inventing the wheel. Murali
On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: > >> +#ifdef CONFIG_PCI_KEYSTONE > >> +/* > >> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. > >> + */ > >> +static void quirk_limit_readrequest(struct pci_dev *dev) > >> +{ > >> + int readrq = pcie_get_readrq(dev); > >> + > >> + if (readrq > 256) > >> + pcie_set_readrq(dev, 256); > >> +} > >> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > >> +#endif /* CONFIG_PCI_KEYSTONE */ > > This doesn't work: you can't just limit do this for all devices just based > > on PCI_KEYSTONE being enabled, you have to check if you are actually using > > this controller. > > > > Arnd > I assume, I need to check if PCI controller's vendor ID/ device ID > match with the keystone > PCI controller's ID and call pcie_set_readrq() for all of the slave > PCI devices and do this fixup. > Is this correct understanding? If you can point me to an example code > for this that will be > really helpful so that I can avoid re-inventing the wheel. I think it would be best to move the quirk into the keystone pci driver and compare compare the dev->driver pointer of the PCI controller device. Arnd
On Thu, May 15, 2014 at 08:20:13PM +0200, Arnd Bergmann wrote: > On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: > > >> +#ifdef CONFIG_PCI_KEYSTONE > > >> +/* > > >> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. > > >> + */ > > >> +static void quirk_limit_readrequest(struct pci_dev *dev) > > >> +{ > > >> + int readrq = pcie_get_readrq(dev); > > >> + > > >> + if (readrq > 256) > > >> + pcie_set_readrq(dev, 256); > > >> +} > > >> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > > >> +#endif /* CONFIG_PCI_KEYSTONE */ > > > This doesn't work: you can't just limit do this for all devices just based > > > on PCI_KEYSTONE being enabled, you have to check if you are actually using > > > this controller. > > > > > > Arnd > > I assume, I need to check if PCI controller's vendor ID/ device ID > > match with the keystone > > PCI controller's ID and call pcie_set_readrq() for all of the slave > > PCI devices and do this fixup. > > Is this correct understanding? If you can point me to an example code > > for this that will be > > really helpful so that I can avoid re-inventing the wheel. > > I think it would be best to move the quirk into the keystone pci driver > and compare compare the dev->driver pointer of the PCI controller device. The PCI core handles setting the maximum read request size already, can you figure out why the core code isn't doing what you need and correct the root problem? I'm guessing the values in the root port bridge config space are not correct or something like that?? Jason
On 5/15/2014 2:39 PM, Jason Gunthorpe wrote: > On Thu, May 15, 2014 at 08:20:13PM +0200, Arnd Bergmann wrote: >> On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: >>>>> +#ifdef CONFIG_PCI_KEYSTONE >>>>> +/* >>>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. >>>>> + */ >>>>> +static void quirk_limit_readrequest(struct pci_dev *dev) >>>>> +{ >>>>> + int readrq = pcie_get_readrq(dev); >>>>> + >>>>> + if (readrq > 256) >>>>> + pcie_set_readrq(dev, 256); >>>>> +} >>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); >>>>> +#endif /* CONFIG_PCI_KEYSTONE */ >>>> This doesn't work: you can't just limit do this for all devices just based >>>> on PCI_KEYSTONE being enabled, you have to check if you are actually using >>>> this controller. >>>> >>>> Arnd >>> I assume, I need to check if PCI controller's vendor ID/ device ID >>> match with the keystone >>> PCI controller's ID and call pcie_set_readrq() for all of the slave >>> PCI devices and do this fixup. >>> Is this correct understanding? If you can point me to an example code >>> for this that will be >>> really helpful so that I can avoid re-inventing the wheel. >> I think it would be best to move the quirk into the keystone pci driver >> and compare compare the dev->driver pointer of the PCI controller device. > The PCI core handles setting the maximum read request size already, > can you figure out why the core code isn't doing what you need and > correct the root problem? > > I'm guessing the values in the root port bridge config space are not > correct or something like that?? > > Jason What you mean by "The PCI core handles setting the maximum read request size already" I see there is function pcie_write_mrrs() in the drivers/pci/probe.c that reads the mps using pcie_get_mps() and then set mrrs to mps. But this function is called only from pcie_bus_configure_set() that is called by pcie_bus_configure_settings() pcie_bus_configure_settings() is called by 0 pci-common.c pcibios_scan_phb 1676 pcie_bus_configure_settings(child); 1 pci_gx.c fixup_read_and_payload_sizes 606 pcie_bus_configure_settings(child); 2 acpi.c pci_acpi_scan_root 556 pcie_bus_configure_settings(child); 3 pcihp_slot.c pci_configure_slot 164 pcie_bus_configure_settings(dev->bus); None of them gets called on ARM platform. What is needed for Keystone PCI is that we need to limit mrrs to 256 bytes for all of the downstream devices that talks to PCIe controller. This needs to be enforced through a quirk. Or Is there something missing that Keystone or DW driver is not making use of that reads the mrrs value of the RC and set the same on all of the downstream devices connected to the bus that the controller is connected to? Murali
On Thu, May 15, 2014 at 04:04:47PM -0400, Murali Karicheri wrote: > Jason What you mean by "The PCI core handles setting the maximum > read request size already" I see there is function pcie_write_mrrs() > in the drivers/pci/probe.c that reads the mps using pcie_get_mps() > and then set mrrs to mps. But this function is called only from > pcie_bus_configure_set() that is called by > pcie_bus_configure_settings() Right, that is the common code that correctly sets the MRRS that you should be using instead of quirks. > None of them gets called on ARM platform. Hmm, a cursory glance tells me the same as well. That seems to be the root problem here, ARM needs to do the PCIE setup just as much as any other arch. So, I would prefer to see you fix ARM common code to call pcie_bus_configure_settings() properly, that seems very simple and is obviously needed for any PCI-E host driver on ARM. Thoughts? Arnd? Bjorn? Jason
On 5/15/2014 2:20 PM, Arnd Bergmann wrote: > On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: >>>> +#ifdef CONFIG_PCI_KEYSTONE >>>> +/* >>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. >>>> + */ >>>> +static void quirk_limit_readrequest(struct pci_dev *dev) >>>> +{ >>>> + int readrq = pcie_get_readrq(dev); >>>> + >>>> + if (readrq > 256) >>>> + pcie_set_readrq(dev, 256); >>>> +} >>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); >>>> +#endif /* CONFIG_PCI_KEYSTONE */ >>> This doesn't work: you can't just limit do this for all devices just based >>> on PCI_KEYSTONE being enabled, you have to check if you are actually using >>> this controller. >>> >>> Arnd >> I assume, I need to check if PCI controller's vendor ID/ device ID >> match with the keystone >> PCI controller's ID and call pcie_set_readrq() for all of the slave >> PCI devices and do this fixup. >> Is this correct understanding? If you can point me to an example code >> for this that will be >> really helpful so that I can avoid re-inventing the wheel. > I think it would be best to move the quirk into the keystone pci driver > and compare compare the dev->driver pointer of the PCI controller device. > > Arnd Arnd, I will move this quirk to keystone pci driver. So in that case, I guess your original comment is not applicable as this quirk gets enabled only with PCI keystone driver enabled. Right? Murali
>-----Original Message----- >From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] >Sent: Thursday, May 15, 2014 4:52 PM >To: Karicheri, Muralidharan >Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Strashko, Grygorii; linux- >pci@vger.kernel.org; Jingoo Han; linux-kernel@vger.kernel.org; Shilimkar, Santosh; Mohit >Kumar; Bjorn Helgaas >Subject: Re: [PATCH v1 5/5] pci: keystone: add pcie driver based on designware core >driver > >On Thu, May 15, 2014 at 04:04:47PM -0400, Murali Karicheri wrote: > >> Jason What you mean by "The PCI core handles setting the maximum read >> request size already" I see there is function pcie_write_mrrs() in the >> drivers/pci/probe.c that reads the mps using pcie_get_mps() and then >> set mrrs to mps. But this function is called only from >> pcie_bus_configure_set() that is called by >> pcie_bus_configure_settings() > >Right, that is the common code that correctly sets the MRRS that you should be using >instead of quirks. > >> None of them gets called on ARM platform. > >Hmm, a cursory glance tells me the same as well. > >That seems to be the root problem here, ARM needs to do the PCIE setup just as much as >any other arch. > >So, I would prefer to see you fix ARM common code to call >pcie_bus_configure_settings() properly, that seems very simple and is obviously needed for >any PCI-E host driver on ARM. > But pcie_bus_configure_settings just make sure the mrrs for a device is not greater than the max payload size. But the quirk that I need is to limit the size of mrr to 256 as required by the keystone PCI controller. So I still need to implement a quirk to enforce this limit. In my reply to Arnd, I have agreed to move the quirk to keystone driver. Murali >Thoughts? Arnd? Bjorn? > >Jason
Adding more people to the list for review. >-----Original Message----- >From: Karicheri, Muralidharan >Sent: Thursday, May 15, 2014 12:02 PM >To: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; linux-arm- >kernel@lists.infradead.org >Cc: Karicheri, Muralidharan; Shilimkar, Santosh; Mohit Kumar; Jingoo Han; Bjorn Helgaas; >Strashko, Grygorii >Subject: [PATCH v1 5/5] pci: keystone: add pcie driver based on designware core driver > >keystone pcie hardware is based on designware version 3.65. >This driver make use of the functions from pci-dw-old.c and pci-dw-old-msi.c to implement >the driver. > >Driver mainly handle the platform specific part of the PCI driver and depends on DW Old >driver to configure application specific registers. Also routes the irq events and ack the >interrupt after the same is acked by the end point device driver. This requires irqchip >implementation for legacy and MSI irq handling. This patch adds a quirks to override the >max read request size as PCI controller has a limit of 256 bytes. > >CC: Santosh Shilimkar <santosh.shilimkar@ti.com> >CC: Mohit Kumar <mohit.kumar@st.com> >CC: Jingoo Han <jg1.han@samsung.com> >CC: Bjorn Helgaas <bhelgaas@google.com> > >Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >--- > .../devicetree/bindings/pci/pcie-keystone.txt | 68 ++++ > drivers/pci/host/Kconfig | 8 + > drivers/pci/host/Makefile | 1 + > drivers/pci/host/pci-keystone.c | 400 ++++++++++++++++++++ > drivers/pci/quirks.c | 13 + > 5 files changed, 490 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/pcie-keystone.txt > create mode 100644 drivers/pci/host/pci-keystone.c > >diff --git a/Documentation/devicetree/bindings/pci/pcie-keystone.txt >b/Documentation/devicetree/bindings/pci/pcie-keystone.txt >new file mode 100644 >index 0000000..17cf261 >--- /dev/null >+++ b/Documentation/devicetree/bindings/pci/pcie-keystone.txt >@@ -0,0 +1,68 @@ >+Keystone PCIE Root complex device tree bindings >+----------------------------------------------- >+ >+Sample bindings shown below:- >+ >+ - Remove ti,enable-linktrain if boot loader already does Link training and do EP >+ configuration. >+ - Remove ti,init-phy if boot loader already initialize the phy and sets up pcie >+ link >+ >+ pcie0_phy: pciephy@2320000 { >+ #address-cells = <1>; >+ #size-cells = <1>; >+ #phy-cells = <0>; >+ compatible = "ti,keystone-phy"; >+ reg = <0x02320000 0x4000>; >+ reg-names = "reg_serdes"; >+ }; >+ >+ pcie@21800000 { >+ compatible = "ti,keystone-pcie"; >+ device_type = "pci"; >+ clocks = <&clkpcie>; >+ clock-names = "pcie"; >+ #address-cells = <3>; >+ #size-cells = <2>; >+ reg = <0x21800000 0x1000>, <0x0262014c 4>; >+ reg-names = "reg_rc_app", "reg_devcfg"; >+ interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; /* Error IRQ */ >+ >+ ranges = <0x00000800 0 0x21801000 0x21801000 0 0x0002000 /* >Configuration space */ >+ 0x81000000 0 0 0x24000000 0 0x4000 /* downstream >I/O */ >+ 0x82000000 0 0x50000000 0x50000000 0 0x10000000>; /* >+non-prefetchable memory */ >+ >+ num-lanes = <2>; >+ ti,enable-linktrain; >+ ti,init-phy; >+ >+ /* PCIE phy */ >+ phys = <&pcie0_phy>; >+ phy-names = "pcie-phy"; >+ >+ #interrupt-cells = <1>; >+ interrupt-map-mask = <0 0 0 0>; >+ interrupt-map = <0 0 0 1 &pcie_intc 1>, // INT A >+ <0 0 0 2 &pcie_intc 2>, // INT B >+ <0 0 0 3 &pcie_intc 3>, // INT C >+ <0 0 0 4 &pcie_intc 4>; // INT D >+ >+ pcie_intc: legacy-interrupt-controller { >+ interrupt-controller; >+ #interrupt-cells = <1>; >+ interrupt-parent = <&gic>; >+ interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 28 IRQ_TYPE_EDGE_RISING>, >+ <GIC_SPI 29 IRQ_TYPE_EDGE_RISING>; >+ }; >+ }; >+ >diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index c4f4732..066d611 >100644 >--- a/drivers/pci/host/Kconfig >+++ b/drivers/pci/host/Kconfig >@@ -37,4 +37,12 @@ config PCI_RCAR_GEN2 > Say Y here if you want internal PCI support on R-Car Gen2 SoC. > There are 3 internal PCI controllers available with a single > built-in EHCI/OHCI host controller present on each one. >+ >+config PCI_KEYSTONE >+ bool "TI Keystone PCIe controller" >+ depends on ARCH_KEYSTONE >+ select PCIE_DW >+ select PCI_DW_OLD >+ select PCIEPORTBUS >+ select PHY_TI_KEYSTONE > endmenu >diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index be5d939..10e02f9 >100644 >--- a/drivers/pci/host/Makefile >+++ b/drivers/pci/host/Makefile >@@ -5,3 +5,4 @@ obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o > obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o > obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o > obj-$(CONFIG_PCI_DW_OLD) += pci-dw-old-msi.o pci-dw-old.o >+obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o >diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c new file mode >100644 index 0000000..2636717 >--- /dev/null >+++ b/drivers/pci/host/pci-keystone.c >@@ -0,0 +1,400 @@ >+/* >+ * PCIe host controller driver for Texas Instruments Keystone SoCs >+ * >+ * Copyright (C) 2013-2014 Texas Instruments., Ltd. >+ * http://www.ti.com >+ * >+ * Author: Murali Karicheri <m-karicheri2@ti.com> >+ * Implementation based on pci-exynos.c and pcie-designware.c >+ * >+ * This program is free software; you can redistribute it and/or modify >+ * it under the terms of the GNU General Public License version 2 as >+ * published by the Free Software Foundation. >+ */ >+ >+#include <linux/irqchip/chained_irq.h> >+#include <linux/clk.h> >+#include <linux/delay.h> >+#include <linux/irqdomain.h> >+#include <linux/module.h> >+#include <linux/msi.h> >+#include <linux/of_irq.h> >+#include <linux/of.h> >+#include <linux/of_pci.h> >+#include <linux/platform_device.h> >+#include <linux/phy/phy.h> >+#include <linux/resource.h> >+#include <linux/signal.h> >+ >+#include "pcie-designware.h" >+#include "pci-dw-old.h" >+ >+#define DRIVER_NAME "keystone-pcie" >+ >+/* driver specific constants */ >+#define MAX_MSI_HOST_IRQS 8 >+#define MAX_LEGACY_HOST_IRQS 4 >+ >+/* RC mode settings */ >+#define PCIE_RC_MODE (BIT(2)) >+#define PCIE_MODE_MASK (BIT(1) | BIT(2)) >+ >+static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) { >+ return sys->private_data; >+} >+ >+struct keystone_pcie { >+ struct clk *clk; >+ int en_link_train; >+ struct pcie_port pp; >+ >+ int num_legacy_host_irqs; >+ int legacy_host_irqs[MAX_LEGACY_HOST_IRQS]; >+ struct device_node *np_intc; >+ >+ int num_msi_host_irqs; >+ int msi_host_irqs[MAX_MSI_HOST_IRQS]; >+}; >+ >+#define to_keystone_pcie(x) container_of(x, struct keystone_pcie, pp) >+ >+static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie) { >+ struct pcie_port *pp = &ks_pcie->pp; >+ int count = 200; >+ >+ dw_pcie_setup_rc(pp); >+ >+ /* check if the link is up or not */ >+ while (!dw_pcie_link_up(pp)) { >+ usleep_range(100, 1000); >+ if (--count) >+ continue; >+ dev_err(pp->dev, "phy link never came up\n"); >+ return -EINVAL; >+ } >+ >+ return 0; >+} >+ >+static void ks_pcie_msi_irq_handler(unsigned int irq, struct irq_desc >+*desc) { >+ struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); >+ u32 offset = irq - ks_pcie->msi_host_irqs[0]; >+ struct pcie_port *pp = &ks_pcie->pp; >+ struct irq_chip *chip = irq_desc_get_chip(desc); >+ >+ dev_dbg(pp->dev, "ks_pcie_msi_irq_handler, irq %d\n", irq); >+ >+ /* >+ * The chained irq handler installation would have replaced normal >+ * interrupt driver handler so we need to take care of mask/unmask and >+ * ack operation. >+ */ >+ chained_irq_enter(chip, desc); >+ dw_old_handle_msi_irq(pp, offset); >+ chained_irq_exit(chip, desc); >+} >+ >+/** >+ * ks_pcie_legacy_irq_handler() - Handle legacy interrupt >+ * @irq: IRQ line for legacy interrupts >+ * @desc: Pointer to irq descriptor >+ * >+ * Traverse through pending legacy interrupts and invoke handler for >+each. Also >+ * takes care of interrupt controller level mask/ack operation. >+ */ >+static void ks_pcie_legacy_irq_handler(unsigned int irq, struct >+irq_desc *desc) { >+ struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); >+ u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0]; >+ struct irq_chip *chip = irq_desc_get_chip(desc); >+ struct pcie_port *pp = &ks_pcie->pp; >+ >+ dev_dbg(ks_pcie->pp.dev, ": Handling legacy irq %d\n", irq); >+ >+ /* >+ * The chained irq handler installation would have replaced normal >+ * interrupt driver handler so we need to take care of mask/unmask and >+ * ack operation. >+ */ >+ chained_irq_enter(chip, desc); >+ dw_old_handle_legacy_irq(pp, irq_offset); >+ chained_irq_exit(chip, desc); >+} >+ >+static int ks_pcie_init_legacy_irqs(struct keystone_pcie *ks_pcie) { >+ struct device *dev = ks_pcie->pp.dev; >+ struct device_node *np_pcie = dev->of_node; >+ int num_legacy_irqs; >+ int ret = 0; >+ int i; >+ >+ /* get node */ >+ ks_pcie->np_intc = of_find_node_by_name(np_pcie, >+ "legacy-interrupt-controller"); >+ if (!ks_pcie->np_intc) { >+ dev_err(dev, >+ "Node for legacy-interrupt-controller is absent\n"); >+ goto out; >+ } >+ >+ /* get number of IRQs */ >+ num_legacy_irqs = of_irq_count(ks_pcie->np_intc); >+ if (!num_legacy_irqs) >+ goto out; >+ >+ if (num_legacy_irqs > MAX_LEGACY_HOST_IRQS) { >+ dev_err(dev, "Too many legacy interrupts defined %u\n", >+ num_legacy_irqs); >+ num_legacy_irqs = MAX_LEGACY_HOST_IRQS; >+ } >+ /* >+ * support upto MAX_LEGACY_HOST_IRQS host and legacy IRQs. >+ * In dt from index 0 to 3 >+ */ >+ for (i = 0; i < num_legacy_irqs; i++) { >+ ks_pcie->legacy_host_irqs[i] = >+ irq_of_parse_and_map(ks_pcie->np_intc, i); >+ if (ks_pcie->legacy_host_irqs[i] < 0) >+ break; >+ ks_pcie->num_legacy_host_irqs++; >+ } >+ >+ if (!ks_pcie->num_legacy_host_irqs) { >+ dev_err(dev, "Failed to get legacy interrupts\n"); >+ goto out; >+ } >+ >+out: >+ return ret; >+} >+ >+static void ks_pcie_enable_interrupts(struct keystone_pcie *ks_pcie) { >+ struct pcie_port *pp = &ks_pcie->pp; >+ int i; >+ >+ /* Legacy IRQ */ >+ for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) { >+ irq_set_handler_data(ks_pcie->legacy_host_irqs[i], ks_pcie); >+ irq_set_chained_handler(ks_pcie->legacy_host_irqs[i], >+ ks_pcie_legacy_irq_handler); >+ } >+ dw_old_enable_legacy_irqs(pp); >+ >+ /* MSI IRQ */ >+ if (IS_ENABLED(CONFIG_PCI_MSI)) { >+ for (i = 0; i < ks_pcie->num_msi_host_irqs; i++) { >+ irq_set_chained_handler(ks_pcie->msi_host_irqs[i], >+ ks_pcie_msi_irq_handler); >+ irq_set_handler_data(ks_pcie->msi_host_irqs[i], >+ ks_pcie); >+ >+ } >+ } >+ >+ return; >+} >+ >+static int >+keystone_pcie_fault(unsigned long addr, unsigned int fsr, >+ struct pt_regs *regs) >+{ >+ unsigned long instr = *(unsigned long *) instruction_pointer(regs); >+ >+ if ((instr & 0x0e100090) == 0x00100090) { >+ int reg = (instr >> 12) & 15; >+ >+ regs->uregs[reg] = -1; >+ regs->ARM_pc += 4; >+ } >+ >+ return 0; >+} >+ >+static void __init ks_pcie_host_init(struct pcie_port *pp) { >+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); >+ >+ ks_pcie_establish_link(ks_pcie); >+ dw_old_disable_bars(pp); >+ dw_old_setup_ob_regs(pp); >+ ks_pcie_enable_interrupts(ks_pcie); >+ writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), >+ pp->dbi_base + PCI_IO_BASE); >+ /* >+ * PCIe access errors that result into OCP errors are caught by ARM as >+ * "External aborts" (Precise). >+ */ >+ hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0, >+ "external abort on linefetch"); >+} >+ >+int ks_pcie_link_up(struct pcie_port *pp) { >+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); >+ >+ return dw_old_pcie_link_up(pp, ks_pcie->en_link_train); } >+ >+static struct pcie_host_ops keystone_pcie_host_ops = { >+ .rd_other_conf = dw_old_rd_other_conf, >+ .wr_other_conf = dw_old_wr_other_conf, >+ .link_up = ks_pcie_link_up, >+ .host_init = ks_pcie_host_init, >+ .get_msi_data = dw_old_get_msi_data, >+}; >+ >+static int add_pcie_port(struct keystone_pcie *ks_pcie, >+ struct platform_device *pdev) >+{ >+ struct device_node *np = pdev->dev.of_node; >+ struct pcie_port *pp = &ks_pcie->pp; >+ struct resource *res; >+ int ret, i; >+ >+ ret = ks_pcie_init_legacy_irqs(ks_pcie); >+ if (ret) >+ return ret; >+ >+ if (IS_ENABLED(CONFIG_PCI_MSI)) { >+ /* >+ * support upto 32 MSI irqs mapped to 8 host IRQs. >+ * In dt from index 4 to 11 >+ */ >+ for (i = 0; i < MAX_MSI_HOST_IRQS; i++) { >+ ks_pcie->msi_host_irqs[i] = irq_of_parse_and_map(np, i); >+ if (ks_pcie->msi_host_irqs[i] < 0) >+ break; >+ ks_pcie->num_msi_host_irqs++; >+ } >+ } >+ >+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg_rc_app"); >+ pp->va_app_base = devm_ioremap_resource(&pdev->dev, res); >+ if (IS_ERR(pp->va_app_base)) >+ return PTR_ERR(pp->va_app_base); >+ pp->app_base = res->start; >+ >+ pp->root_bus_nr = -1; >+ pp->version = DW_VERSION_OLD; >+ pp->ops = &keystone_pcie_host_ops; >+ spin_lock_init(&pp->conf_lock); >+ ret = dw_old_pcie_host_init(pp, ks_pcie->np_intc); >+ if (ret) { >+ dev_err(&pdev->dev, "failed to initialize host\n"); >+ return ret; >+ } >+ >+ return ret; >+} >+ >+static const struct of_device_id ks_pcie_of_match[] = { >+ { >+ .type = "pci", >+ .compatible = "ti,keystone-pcie", >+ }, >+ { }, >+}; >+MODULE_DEVICE_TABLE(of, ks_pcie_of_match); >+ >+static int __exit ks_pcie_remove(struct platform_device *pdev) { >+ struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev); >+ >+ clk_disable_unprepare(ks_pcie->clk); >+ >+ return 0; >+} >+ >+static int __init ks_pcie_probe(struct platform_device *pdev) { >+ struct device_node *np = pdev->dev.of_node; >+ struct device *dev = &pdev->dev; >+ struct keystone_pcie *ks_pcie; >+ void __iomem *devstat; >+ struct pcie_port *pp; >+ struct resource *res; >+ struct phy *phy; >+ int ret = 0; >+ u32 val; >+ >+ ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie), >+ GFP_KERNEL); >+ if (!ks_pcie) { >+ dev_err(dev, "no memory for keystone pcie\n"); >+ return -ENOMEM; >+ } >+ >+ /* check if serdes phy needs to be enabled */ >+ if (of_get_property(np, "ti,init-phy", NULL) != NULL) { >+ phy = devm_phy_get(dev, "pcie-phy"); >+ if (IS_ERR(phy)) >+ return PTR_ERR(phy); >+ >+ ret = phy_init(phy); >+ if (ret < 0) >+ return ret; >+ } >+ >+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >+ "reg_devcfg"); >+ devstat = devm_ioremap_resource(dev, res); >+ if (IS_ERR(devstat)) >+ return PTR_ERR(devstat); >+ >+ /* enable RC mode in devcfg */ >+ val = readl(devstat); >+ val &= ~PCIE_MODE_MASK; >+ val |= PCIE_RC_MODE; >+ writel(val, devstat); >+ >+ /* check if we need to enable link training */ >+ ks_pcie->en_link_train = >+ (of_get_property(np, "ti,enable-linktrain", NULL) != NULL); >+ >+ pp = &ks_pcie->pp; >+ pp->dev = dev; >+ >+ ks_pcie->clk = devm_clk_get(dev, "pcie"); >+ if (IS_ERR(ks_pcie->clk)) { >+ dev_err(dev, "Failed to get pcie rc clock\n"); >+ return PTR_ERR(ks_pcie->clk); >+ } >+ ret = clk_prepare_enable(ks_pcie->clk); >+ if (ret) >+ return ret; >+ >+ ret = add_pcie_port(ks_pcie, pdev); >+ if (ret < 0) >+ goto fail_clk; >+ >+ platform_set_drvdata(pdev, ks_pcie); >+ dev_info(dev, "pcie rc probe success\n"); >+ >+ return 0; >+ >+fail_clk: >+ clk_disable_unprepare(ks_pcie->clk); >+ >+ return ret; >+} >+ >+static struct platform_driver ks_pcie_driver __refdata = { >+ .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), >+ }, >+}; >+ >+module_platform_driver(ks_pcie_driver); >+ >+MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); >+MODULE_DESCRIPTION("Keystone PCIe host controller driver"); >+MODULE_LICENSE("GPL v2"); >diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index e729206..b3e12c2 100644 >--- a/drivers/pci/quirks.c >+++ b/drivers/pci/quirks.c >@@ -3651,3 +3651,16 @@ void pci_dev_specific_enable_acs(struct pci_dev *dev) > } > } > } >+#ifdef CONFIG_PCI_KEYSTONE >+/* >+ * The KeyStone PCIe controller has maximum read request size of 256 bytes. >+ */ >+static void quirk_limit_readrequest(struct pci_dev *dev) { >+ int readrq = pcie_get_readrq(dev); >+ >+ if (readrq > 256) >+ pcie_set_readrq(dev, 256); >+} >+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, >+quirk_limit_readrequest); #endif /* CONFIG_PCI_KEYSTONE */ >-- >1.7.9.5
On 5/15/2014 12:28 PM, Arnd Bergmann wrote: > On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote: >> +Sample bindings shown below:- >> + >> + - Remove ti,enable-linktrain if boot loader already does Link training and do EP >> + configuration. >> + - Remove ti,init-phy if boot loader already initialize the phy and sets up pcie >> + link > You actually have to document all the properties. Have a look at the > other bindings for what this means. Actually this is dw pcie variant. So I will document if there are any deviations from the dw-designware bindings. Based on comments from Jingo, I think I need to add a new DT property for old hw that we use in keystone. May be a compatibility string for older dw h/w like "snps ,dw-pcie-v3.65" is that we could use and do things differently in the common code. So I will update the Documentation of dw bindings once we agree on this. >> + >> + pcie@21800000 { >> + compatible = "ti,keystone-pcie"; >> + device_type = "pci"; >> + clocks = <&clkpcie>; >> + clock-names = "pcie"; > The dw-pcie binding lists two clocks. Ok. As per dw documentation, pcie and pcie_bus are the clocks. But pci-imx6 is currently not using pcie_bus. So the pcie_bus clock is actually optional. In the case of keystone pcie, bus clock is provided by the phy hardware and if I need to provide a clock for bus, it has to be a dummy or change documentation to make pcie_bus an optional clock. I prefer later, but want to know if this is true for pci-imx6. There is also a plan to move the pcie clock API call to designware core. So making pcie_bus clock optional looks correct to me. Seen/Jingoo, any comments? >> + #address-cells = >; >> + #size-cells = <2>; >> + reg = <0x21800000 0x1000>, <0x0262014c 4>; >> + reg-names = "reg_rc_app", "reg_devcfg"; > There should be standard names that are documented in the binding. Currently Documentation says - reg: base addresses and lengths of the pcie controller, the phy controller, additional register for the phy controller. And following dw drivers defines as samsung,exynos5440-pcie reg = <0x290000 0x1000 0x270000 0x1000 0x271000 0x40>; fsl,imx6q-pcie" reg = <0x01ffc000 0x4000>; /* DBI */ There are no names used for registers. So this needs to be documented in the dw-designware bindings. reg_dbi is what is common to pci controller assuming at index 0 of the both above drivers use reg_dbi address for config space. Shall I modify the documentation to include optional reg name reg_dbi for config space base address registers? Additional registers are optional and varies from platform to platform. Phy registers are actually part of Phy driver and should be removed from the dw-documentation bindings. Agree? > >> + interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; /* Error IRQ */ > What are all these interrupts? These are MSI interrupts tied to the GIC over which MSI interrupts are multiplexed. I will document it in the keystone PCI DT bindings. >> + ranges = <0x00000800 0 0x21801000 0x21801000 0 0x0002000 /* Configuration space */ >> + 0x81000000 0 0 0x24000000 0 0x4000 /* downstream I/O */ >> + 0x82000000 0 0x50000000 0x50000000 0 0x10000000>; /* non-prefetchable memory */ > Please move the config space into 'reg' now instead of 'ranges'. > This was a mistake earlier, and there are already other front-ends > doing the same thing. This has to be coordinated with other drivers when DW core makes the change. Jingoo, Sean, What is the plan for this change? I have to defer this currently. > >> + num-lanes = <2>; >> + ti,enable-linktrain; >> + ti,init-phy; >> + >> + /* PCIE phy */ >> + phys = <&pcie0_phy>; >> + phy-names = "pcie-phy"; >> + >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 0>; >> + interrupt-map = <0 0 0 1 &pcie_intc 1>, // INT A >> + <0 0 0 2 &pcie_intc 2>, // INT B >> + <0 0 0 3 &pcie_intc 3>, // INT C >> + <0 0 0 4 &pcie_intc 4>; // INT D > I think this is the wrong map-mask. I think this should be changed to interrupt-map-mask = <0 0 0 0x7> ? > > Arnd
On Friday 16 May 2014 16:26:51 Murali Karicheri wrote: > On 5/15/2014 2:20 PM, Arnd Bergmann wrote: > > On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: > >>>> +#ifdef CONFIG_PCI_KEYSTONE > >>>> +/* > >>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. > >>>> + */ > >>>> +static void quirk_limit_readrequest(struct pci_dev *dev) > >>>> +{ > >>>> + int readrq = pcie_get_readrq(dev); > >>>> + > >>>> + if (readrq > 256) > >>>> + pcie_set_readrq(dev, 256); > >>>> +} > >>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > >>>> +#endif /* CONFIG_PCI_KEYSTONE */ > >>> This doesn't work: you can't just limit do this for all devices just based > >>> on PCI_KEYSTONE being enabled, you have to check if you are actually using > >>> this controller. > >>> > >>> Arnd > >> I assume, I need to check if PCI controller's vendor ID/ device ID > >> match with the keystone > >> PCI controller's ID and call pcie_set_readrq() for all of the slave > >> PCI devices and do this fixup. > >> Is this correct understanding? If you can point me to an example code > >> for this that will be > >> really helpful so that I can avoid re-inventing the wheel. > > I think it would be best to move the quirk into the keystone pci driver > > and compare compare the dev->driver pointer of the PCI controller device. > > > > Arnd > Arnd, > > I will move this quirk to keystone pci driver. So in that case, I guess > your original comment > is not applicable as this quirk gets enabled only with PCI keystone > driver enabled. Right? Of course you still have to fix the bug, not just move the code into the driver. Otherwise it's still broken for every machine after the keystone driver is enabled. I also agree with Jason that changing the PCI core to call pcie_bus_configure_settings() would be a better way of dealing with this if it solves the problem. Arnd
On Friday 16 May 2014 18:44:44 Murali Karicheri wrote: > On 5/15/2014 12:28 PM, Arnd Bergmann wrote: > > On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote: > >> +Sample bindings shown below:- > >> + > >> + - Remove ti,enable-linktrain if boot loader already does Link training and do EP > >> + configuration. > >> + - Remove ti,init-phy if boot loader already initialize the phy and sets up pcie > >> + link > > You actually have to document all the properties. Have a look at the > > other bindings for what this means. > > Actually this is dw pcie variant. So I will document if there are any > deviations from the dw-designware > bindings. Based on comments from Jingo, I think I need to add a new DT > property for old hw that we > use in keystone. May be a compatibility string for older dw h/w like > "snps ,dw-pcie-v3.65" is that we > could use and do things differently in the common code. A compatible string with the exact version sounds like a good idea, yes. > So I will update the Documentation of dw bindings once we agree on this. > >> + > >> + pcie@21800000 { > >> + compatible = "ti,keystone-pcie"; > >> + device_type = "pci"; > >> + clocks = <&clkpcie>; > >> + clock-names = "pcie"; > > The dw-pcie binding lists two clocks. > > Ok. As per dw documentation, pcie and pcie_bus are the clocks. But > pci-imx6 is currently not using pcie_bus. So the pcie_bus > clock is actually optional. In the case of keystone pcie, bus clock is > provided by the phy hardware and if I need to provide > a clock for bus, it has to be a dummy or change documentation to make > pcie_bus an optional clock. I prefer later, but want > to know if this is true for pci-imx6. There is also a plan to move the > pcie clock API call to designware core. So making pcie_bus > clock optional looks correct to me. A documentation change sounds fine to me. How about documenting that there should be either a "pcie_bus" clock or a phy reference? > >> + #address-cells = >; > >> + #size-cells = <2>; > >> + reg = <0x21800000 0x1000>, <0x0262014c 4>; > >> + reg-names = "reg_rc_app", "reg_devcfg"; > > There should be standard names that are documented in the binding. > > Currently Documentation says > > - reg: base addresses and lengths of the pcie controller, > the phy controller, additional register for the phy controller. > > And following dw drivers defines as > > samsung,exynos5440-pcie > > reg = <0x290000 0x1000 > 0x270000 0x1000 > 0x271000 0x40>; > > fsl,imx6q-pcie" > reg = <0x01ffc000 0x4000>; /* DBI */ > > There are no names used for registers. So this needs to be documented in > the dw-designware > bindings. reg_dbi is what is common to pci controller assuming at index > 0 of the both above drivers use > reg_dbi address for config space. Shall I modify the documentation to > include optional reg name > reg_dbi for config space base address registers? Additional registers > are optional and varies from > platform to platform. Phy registers are actually part of Phy driver and > should be removed from > the dw-documentation bindings. Agree? It makes sense to remove the phy registers if there is always a separate phy driver. Regarding the names, it's probably easier to remove them completely than to document them. If the register names are optional, you can't rely on them anyway and have to uses the indexes/ > > > >> + interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; /* Error IRQ */ > > What are all these interrupts? > These are MSI interrupts tied to the GIC over which MSI interrupts are > multiplexed. I will document > it in the keystone PCI DT bindings. Maybe they should be moved into the subnode that has the LSI interrupts, or perhaps a separate node? I don't know how the other dw-pcie variants do it, but it always seems like a good idea to keep MSI separate because you might not want to use it if there is also an MSI capable GIC in the system. > >> + ranges = <0x00000800 0 0x21801000 0x21801000 0 0x0002000 /* Configuration space */ > >> + 0x81000000 0 0 0x24000000 0 0x4000 /* downstream I/O */ > >> + 0x82000000 0 0x50000000 0x50000000 0 0x10000000>; /* non-prefetchable memory */ > > Please move the config space into 'reg' now instead of 'ranges'. > > This was a mistake earlier, and there are already other front-ends > > doing the same thing. > This has to be coordinated with other drivers when DW core makes the > change. > > Jingoo, Sean, > > What is the plan for this change? I have to defer this currently. > >> + #interrupt-cells = <1>; > >> + interrupt-map-mask = <0 0 0 0>; > >> + interrupt-map = <0 0 0 1 &pcie_intc 1>, // INT A > >> + <0 0 0 2 &pcie_intc 2>, // INT B > >> + <0 0 0 3 &pcie_intc 3>, // INT C > >> + <0 0 0 4 &pcie_intc 4>; // INT D > > I think this is the wrong map-mask. > I think this should be changed to > > interrupt-map-mask = <0 0 0 0x7> ? That sounds reasonable. Is that what your hardware implements? Arnd
On 5/19/2014 8:06 AM, Arnd Bergmann wrote: > On Friday 16 May 2014 16:26:51 Murali Karicheri wrote: >> On 5/15/2014 2:20 PM, Arnd Bergmann wrote: >>> On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: >>>>>> +#ifdef CONFIG_PCI_KEYSTONE >>>>>> +/* >>>>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. >>>>>> + */ >>>>>> +static void quirk_limit_readrequest(struct pci_dev *dev) >>>>>> +{ >>>>>> + int readrq = pcie_get_readrq(dev); >>>>>> + >>>>>> + if (readrq > 256) >>>>>> + pcie_set_readrq(dev, 256); >>>>>> +} >>>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); >>>>>> +#endif /* CONFIG_PCI_KEYSTONE */ >>>>> This doesn't work: you can't just limit do this for all devices just based >>>>> on PCI_KEYSTONE being enabled, you have to check if you are actually using >>>>> this controller. >>>>> >>>>> Arnd >>>> I assume, I need to check if PCI controller's vendor ID/ device ID >>>> match with the keystone >>>> PCI controller's ID and call pcie_set_readrq() for all of the slave >>>> PCI devices and do this fixup. >>>> Is this correct understanding? If you can point me to an example code >>>> for this that will be >>>> really helpful so that I can avoid re-inventing the wheel. >>> I think it would be best to move the quirk into the keystone pci driver >>> and compare compare the dev->driver pointer of the PCI controller device. >>> >>> Arnd >> Arnd, >> >> I will move this quirk to keystone pci driver. So in that case, I guess >> your original comment >> is not applicable as this quirk gets enabled only with PCI keystone >> driver enabled. Right? > Of course you still have to fix the bug, not just move the code into > the driver. Otherwise it's still broken for every machine after the keystone > driver is enabled. Agree. I have tried following to get this work so that the quirk gets applied only for keystone pci controller. #define KS_K2HK_PCI_DEVICE_ID 0xb008 #define KS_K2E_PCI_DEVICE_ID 0xb009 #define KS_K2L_PCI_DEVICE_ID 0xb00a static u16 root_pci_ids[] = {KS_K2HK_PCI_DEVICE_ID, KS_K2E_PCI_DEVICE_ID, KS_K2L_PCI_DEVICE_ID, 0 }; /* * The KeyStone PCIe controller has maximum read request size of 256 bytes. */ static void quirk_limit_readrequest(struct pci_dev *dev) { struct pci_dev *root_dev; int i = 0; /* Apply quirk only if this bridge device is keystone */ while (root_pci_ids[i]) { root_dev = pci_get_device(PCI_VENDOR_ID_TI, root_pci_ids[i], NULL); if ((root_dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) { int readrq; readrq = pcie_get_readrq(dev); if (pcie_get_readrq(dev) > 256) { pcie_set_readrq(dev, 256); printk("Applied quirk\n"); } break; }; } } DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > > I also agree with Jason that changing the PCI core to call > pcie_bus_configure_settings() would be a better way of dealing with this > if it solves the problem. I tried following piece of code added to bios32.c void pci_common_init_dev(struct device *parent, struct hw_pci *hw) { struct pci_sys_data *sys; LIST_HEAD(head); pci_add_flags(PCI_REASSIGN_ALL_RSRC); if (hw->preinit) hw->preinit(); ........ .... list_for_each_entry(sys, &head, node) { struct pci_bus *bus = sys->bus; if (!pci_has_flag(PCI_PROBE_ONLY)) { /* * Size the bridge windows. */ pci_bus_size_bridges(bus); /* * Assign resources. */ pci_bus_assign_resources(bus); } /* * Tell drivers about devices found. */ pci_bus_add_devices(bus); } // New code starts here list_for_each_entry(sys, &head, node) { struct pci_bus *bus = sys->bus; /* Configure PCI Express settings */ if (bus && !pci_has_flag(PCI_PROBE_ONLY)) { struct pci_bus *child; list_for_each_entry(child, &bus->children, node) pcie_bus_configure_settings(child); } } // New code ends. This seems to do different things based on the pci bootargs. The key requirement for Keystone PCI controller is that the MRRS can't be more than 256 bytes. The only option that works for keystone PCI controller is with pci=pcie_bus_perf. I see following logs:- [ 3.382747] pcie_bus_configure_settings, config 2 [ 3.382753] pcie_bus_configure_set [ 3.382781] pcieport 0000:00:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 [ 3.382788] pcie_bus_configure_set [ 3.382846] pci 0000:01:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 [ 3.382852] pcie_bus_configure_set [ 3.382909] pci 0000:01:00.1: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 On ARM, by default pci_bus_config seems to be set to 0 (PCIE_BUS_TUNE_OFF). So the code doesn't get executed for this default. But for PCIE_BUS_SAFE, it doesn't change the mrrs at the EP and is not good for our platform w.r.t mrrs settings. For PCIE_BUS_PERFORMANCE, it seems to increase the payload size as well and Keystone Payload size is limited to 128 bytes. So it is not safe to increase the payload size to 256 based on the log. On other platforms, Why the PCI core try to set the payload size equal to mrrs? Is this explained in any PCI spec? Looks like this is done for performance? Let me know if you want me to send a patch for review to add the pcie_bus_configure_settings() code to arch/arm/kernel/bios32.c For the Keystone PCI driver, I believe. it is safe to have the quirk so that controller can handle the read requests properly. Let me know if the quirk code above looks good to go. Murali > Arnd
On Monday 19 May 2014 17:10:50 Murali Karicheri wrote: > On 5/19/2014 8:06 AM, Arnd Bergmann wrote: > > On Friday 16 May 2014 16:26:51 Murali Karicheri wrote: > >> On 5/15/2014 2:20 PM, Arnd Bergmann wrote: > >>> On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote: > >>>>>> +#ifdef CONFIG_PCI_KEYSTONE > >>>>>> +/* > >>>>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes. > >>>>>> + */ > >>>>>> +static void quirk_limit_readrequest(struct pci_dev *dev) > >>>>>> +{ > >>>>>> + int readrq = pcie_get_readrq(dev); > >>>>>> + > >>>>>> + if (readrq > 256) > >>>>>> + pcie_set_readrq(dev, 256); > >>>>>> +} > >>>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); > >>>>>> +#endif /* CONFIG_PCI_KEYSTONE */ > >>>>> This doesn't work: you can't just limit do this for all devices just based > >>>>> on PCI_KEYSTONE being enabled, you have to check if you are actually using > >>>>> this controller. > >>>>> > >>>>> Arnd > >>>> I assume, I need to check if PCI controller's vendor ID/ device ID > >>>> match with the keystone > >>>> PCI controller's ID and call pcie_set_readrq() for all of the slave > >>>> PCI devices and do this fixup. > >>>> Is this correct understanding? If you can point me to an example code > >>>> for this that will be > >>>> really helpful so that I can avoid re-inventing the wheel. > >>> I think it would be best to move the quirk into the keystone pci driver > >>> and compare compare the dev->driver pointer of the PCI controller device. > >>> > >>> Arnd > >> Arnd, > >> > >> I will move this quirk to keystone pci driver. So in that case, I guess > >> your original comment > >> is not applicable as this quirk gets enabled only with PCI keystone > >> driver enabled. Right? > > Of course you still have to fix the bug, not just move the code into > > the driver. Otherwise it's still broken for every machine after the keystone > > driver is enabled. > > Agree. I have tried following to get this work so that the quirk gets > applied only for > keystone pci controller. > > #define KS_K2HK_PCI_DEVICE_ID 0xb008 > #define KS_K2E_PCI_DEVICE_ID 0xb009 > #define KS_K2L_PCI_DEVICE_ID 0xb00a > > static u16 root_pci_ids[] = > {KS_K2HK_PCI_DEVICE_ID, KS_K2E_PCI_DEVICE_ID, KS_K2L_PCI_DEVICE_ID, > 0 }; > /* > * The KeyStone PCIe controller has maximum read request size of 256 bytes. > */ > static void quirk_limit_readrequest(struct pci_dev *dev) > { > struct pci_dev *root_dev; > int i = 0; > > /* Apply quirk only if this bridge device is keystone */ > while (root_pci_ids[i]) { > root_dev = pci_get_device(PCI_VENDOR_ID_TI, root_pci_ids[i], NULL); > if ((root_dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) { > int readrq; > > readrq = pcie_get_readrq(dev); > if (pcie_get_readrq(dev) > 256) { > pcie_set_readrq(dev, 256); > printk("Applied quirk\n"); > } > break; > }; > } > } > DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); Why not compare the driver pointer as I suggested? > > I also agree with Jason that changing the PCI core to call > > pcie_bus_configure_settings() would be a better way of dealing with this > > if it solves the problem. > > I tried following piece of code added to bios32.c > > void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > { > struct pci_sys_data *sys; > LIST_HEAD(head); > > pci_add_flags(PCI_REASSIGN_ALL_RSRC); > if (hw->preinit) > hw->preinit(); > > ........ > .... > > list_for_each_entry(sys, &head, node) { > struct pci_bus *bus = sys->bus; > > if (!pci_has_flag(PCI_PROBE_ONLY)) { > /* > * Size the bridge windows. > */ > pci_bus_size_bridges(bus); > > /* > * Assign resources. > */ > pci_bus_assign_resources(bus); > } > /* > * Tell drivers about devices found. > */ > pci_bus_add_devices(bus); > } > > // New code starts here > list_for_each_entry(sys, &head, node) { > struct pci_bus *bus = sys->bus; > > /* Configure PCI Express settings */ > if (bus && !pci_has_flag(PCI_PROBE_ONLY)) { > struct pci_bus *child; > > list_for_each_entry(child, &bus->children, node) > pcie_bus_configure_settings(child); > } > } > // New code ends. This is the ARM specific part of the PCI code. I think it would be better to do this independent of the architecture. > This seems to do different things based on the pci bootargs. The key > requirement for Keystone PCI controller > is that the MRRS can't be more than 256 bytes. The only option that > works for keystone PCI controller > is with pci=pcie_bus_perf. I see following logs:- > > [ 3.382747] pcie_bus_configure_settings, config 2 > [ 3.382753] pcie_bus_configure_set > [ 3.382781] pcieport 0000:00:00.0: Max Payload Size set to 256/ 256 > (was 128), Max Read Rq 256 > [ 3.382788] pcie_bus_configure_set > [ 3.382846] pci 0000:01:00.0: Max Payload Size set to 256/ 256 (was > 128), Max Read Rq 256 > [ 3.382852] pcie_bus_configure_set > [ 3.382909] pci 0000:01:00.1: Max Payload Size set to 256/ 256 (was > 128), Max Read Rq 256 > > On ARM, by default pci_bus_config seems to be set to 0 > (PCIE_BUS_TUNE_OFF). So the code doesn't get > executed for this default. But for PCIE_BUS_SAFE, it doesn't change the > mrrs at the EP and is not > good for our platform w.r.t mrrs settings. For PCIE_BUS_PERFORMANCE, it > seems to increase the payload > size as well and Keystone Payload size is limited to 128 bytes. So it is > not safe to increase the payload > size to 256 based on the log. > > On other platforms, Why the PCI core try to set the payload size equal > to mrrs? Is this explained in any > PCI spec? Looks like this is done for performance? Let me know if you > want me to send a patch for > review to add the pcie_bus_configure_settings() code to > arch/arm/kernel/bios32.c > > For the Keystone PCI driver, I believe. it is safe to have the quirk so > that controller can handle the > read requests properly. Let me know if the quirk code above looks good > to go. I don't know enough about these to give you a definite answer, but I'd still prefer to handle this in generic code. A quirk seems to be the wrong answer here. If this isn't something we can do in generic fashion, can you do it in the add_bus() callback perhaps? Maybe Jason or Bjorn have a better idea. Arnd
On Fri, May 16, 2014 at 08:29:56PM +0000, Karicheri, Muralidharan wrote: > But pcie_bus_configure_settings just make sure the mrrs for a device > is not greater than the max payload size. Not quite, it first scans the network checking the Maximum Payload Size Supported (MPSS) for each device, and chooses the highest supported by all as the MPS for all. PCI-E requires that an end point support all packets up to the MPS, so if your bridge can't generate a 512 byte read response packet, then it must not advertise a MPSS greater than 256 bytes. Setting your MPSS to 128, 256, then using the pcie_bus_configure_settings to run the standard algorithm should properly limit the readrq to 256 and be able to properly support all the fun edge cases like hot plug. If the config space in your root port bridge is correct and already declares a MPSS of 256 then you have nothing else to do but make sure pcie_bus_configure_settings gets calls. If it is broken and claims a higher MPSS than it can support then you need to use a quirk only for the root port bridge or edit the config reply in the driver only to fix the MPSS. Jason
On Tue, May 20, 2014 at 1:55 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 19 May 2014 17:10:50 Murali Karicheri wrote: >> On ARM, by default pci_bus_config seems to be set to 0 >> (PCIE_BUS_TUNE_OFF). So the code doesn't get >> executed for this default. But for PCIE_BUS_SAFE, it doesn't change the >> mrrs at the EP and is not >> good for our platform w.r.t mrrs settings. For PCIE_BUS_PERFORMANCE, it >> seems to increase the payload >> size as well and Keystone Payload size is limited to 128 bytes. So it is >> not safe to increase the payload >> size to 256 based on the log. >> >> On other platforms, Why the PCI core try to set the payload size equal >> to mrrs? Is this explained in any >> PCI spec? Looks like this is done for performance? Let me know if you >> want me to send a patch for >> review to add the pcie_bus_configure_settings() code to >> arch/arm/kernel/bios32.c >> >> For the Keystone PCI driver, I believe. it is safe to have the quirk so >> that controller can handle the >> read requests properly. Let me know if the quirk code above looks good >> to go. > > I don't know enough about these to give you a definite answer, but > I'd still prefer to handle this in generic code. A quirk seems to > be the wrong answer here. If this isn't something we can do in generic > fashion, can you do it in the add_bus() callback perhaps? I definitely prefer a more generic approach than a quirk. Unfortunately the MPS management was implemented originally just for x86, not because there's anything arch-specific about it, but because the author was only concerned about x86. Making that more generic has been an open issue all along. Bjorn
On Tue, May 20, 2014 at 11:02 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, May 16, 2014 at 08:29:56PM +0000, Karicheri, Muralidharan wrote: > >> But pcie_bus_configure_settings just make sure the mrrs for a device >> is not greater than the max payload size. > > Not quite, it first scans the network checking the Maximum Payload Size > Supported (MPSS) for each device, and chooses the highest supported by > all as the MPS for all. The "performance" setting, e.g., "pci=pcie_bus_perf", adds a few wrinkles by setting MRRS in ways that allow some devices to have larger MPS than others. I don't think this is exactly what was envisioned in the spec, and it is not guaranteed to work if there is peer-to-peer DMA. This isn't documented very well; the best I know of is the changelogs for: a1c473aa11e6 pci: Clamp pcie_set_readrq() when using "performance" settings b03e7495a862 PCI: Set PCI-E Max Payload Size on fabric Bjorn
On Tue, May 20, 2014 at 11:22:22AM -0600, Bjorn Helgaas wrote: > On Tue, May 20, 2014 at 11:02 AM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Fri, May 16, 2014 at 08:29:56PM +0000, Karicheri, Muralidharan wrote: > > > >> But pcie_bus_configure_settings just make sure the mrrs for a device > >> is not greater than the max payload size. > > > > Not quite, it first scans the network checking the Maximum Payload Size > > Supported (MPSS) for each device, and chooses the highest supported by > > all as the MPS for all. > > The "performance" setting, e.g., "pci=pcie_bus_perf", adds a few > wrinkles by setting MRRS in ways that allow some devices to have > larger MPS than others. I don't think this is exactly what was > envisioned in the spec, and it is not guaranteed to work if there is > peer-to-peer DMA. This isn't documented very well; the best I know of > is the changelogs for: > > a1c473aa11e6 pci: Clamp pcie_set_readrq() when using "performance" settings > b03e7495a862 PCI: Set PCI-E Max Payload Size on fabric Neat, and does pretty much confirm that setting the host bridge MPSS properly is necessary to support all drivers, particularly the ones that call pcie_set_readrq with any old value. The 'performance' setting is a bit scary, it isn't just peer-to-peer DMA that would be impacted but also CPU initiated burst writes. Eg InfiniBand drivers burst a WQE to the NIC via the CPU. The MPS on the root port bridge is used to segment the write. Probably not a problem in practice because I think this is rare, and even rarer that a burst would be > 128 bytes - but as you say, not really what the spec intended.. Jason
On 5/20/2014 1:02 PM, Jason Gunthorpe wrote: > On Fri, May 16, 2014 at 08:29:56PM +0000, Karicheri, Muralidharan wrote: > >> But pcie_bus_configure_settings just make sure the mrrs for a device >> is not greater than the max payload size. > Not quite, it first scans the network checking the Maximum Payload Size > Supported (MPSS) for each device, and chooses the highest supported by > all as the MPS for all. Why highest? It should be lowest so that all on the bus can handle it?? > > PCI-E requires that an end point support all packets up to the MPS, so > if your bridge can't generate a 512 byte read response packet, then it > must not advertise a MPSS greater than 256 bytes. What is MPSS? Is it the payload size in a message TLP? I read the PCIe spec and find MRSS is the maxumum read request size. So memory reads completion data size is limited to this size, right? So for DMA from EP to RC can't be greater than what RC publishes. Not sure how they are related? I have checked that root port is advertising 256 bytes for mrrs and 128 bytes for mps in the config space. So keystone pcie bridge is doing as expected. In Keystone case, what I see is after adding pcie_bus_configure_settings() with pci=pcie_bus_safe, I get following log. [ 1.988851] pcie_bus_configure_settings, config 1 [ 1.988860] pcie_bus_configure_set [ 1.988879] pcieport 0000:00:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 512 [ 1.988887] pcie_bus_configure_set [ 1.988921] pci 0000:01:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 512 [ 1.988928] pcie_bus_configure_set [ 1.988961] pci 0000:01:00.1: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 512 So it is not limiting MRRS to 256 bytes. With pci=pcie_bus_perf [ 1.985777] pcie_bus_configure_settings, config 2 [ 1.985783] pcie_bus_configure_set [ 1.985810] pcieport 0000:00:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 [ 1.985818] pcie_bus_configure_set [ 1.985875] pci 0000:01:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 [ 1.985882] pcie_bus_configure_set [ 1.985939] pci 0000:01:00.1: Max Payload Size set to 256/ 256 (was 128), Max Read Rq 256 Is this log what you expect? > > Setting your MPSS to 128, 256, then using the > pcie_bus_configure_settings to run the standard algorithm should > properly limit the readrq to 256 and be able to properly support all > the fun edge cases like hot plug. > If the config space in your root port bridge is correct and already > declares a MPSS of 256 then you have nothing else to do but make sure > pcie_bus_configure_settings gets calls. > > If it is broken and claims a higher MPSS than it can support then you > need to use a quirk only for the root port bridge or edit the config > reply in the driver only to fix the MPSS If MRSS is clamped to lowest, then this would work with out a quirk, and has to be unconditional (all cases, safe, performance etc). I would like to go with the quirk approach until this discussion concludes the next step to fix this issue. May be someone can take owner ship of this change at PCI core level? My quirk can be removed once the fix is accepted into the tree. Is that an acceptable path forward? Murali > > Jason
On Wed, May 21, 2014 at 07:32:58PM -0400, Murali Karicheri wrote: > >Not quite, it first scans the network checking the Maximum Payload Size > >Supported (MPSS) for each device, and chooses the highest supported by > >all as the MPS for all. > Why highest? It should be lowest so that all on the bus can handle > it?? Right, that is what 'chooses the highest supported by all' means. > >PCI-E requires that an end point support all packets up to the MPS, so > >if your bridge can't generate a 512 byte read response packet, then it > >must not advertise a MPSS greater than 256 bytes. > What is MPSS? Is it the payload size in a message TLP? I read the > PCIe spec and find MRSS is the maxumum read request size. So memory > reads completion data size is limited to this size, right? So for > DMA from EP to RC can't be greater than what RC publishes. Not sure > how they are related? MPSS is the maximum packet size supported for send or receive, it is the absolute max capability of the port: DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 unlimited MPSS ^^^^^^^^^^^^^^^^^^^^^^^ MPS is then the largest packet the port will send or receive. MRSS is the largest read the port will request. DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- MaxPayload 128 bytes, MaxReadReq 512 bytes MPS ^^^^^^^^^^^^^^^^^^^^^^^ MRSS ^^^^^^^^^^^^^^^^^^^^ It is a little confusing what MRSS > MPS means exactly, but at a minimum if MRSS > MPS then the responding port must segment the read reply into MPS sized TLPs. > I have checked that root port is advertising 256 bytes for mrrs and > 128 bytes for mps in the config space. So keystone pcie bridge is > doing as expected. What is the MPSS? > In Keystone case, what I see is after adding > pcie_bus_configure_settings() with pci=pcie_bus_safe, > I get following log. > > [ 1.988851] pcie_bus_configure_settings, config 1 > [ 1.988860] pcie_bus_configure_set > [ 1.988879] pcieport 0000:00:00.0: Max Payload Size set to 256/ > 256 (was 128), Max Read Rq 512 > [ 1.988887] pcie_bus_configure_set > [ 1.988921] pci 0000:01:00.0: Max Payload Size set to 256/ 256 > (was 128), Max Read Rq 512 > [ 1.988928] pcie_bus_configure_set > [ 1.988961] pci 0000:01:00.1: Max Payload Size set to 256/ 256 > (was 128), Max Read Rq 512 > > So it is not limiting MRRS to 256 bytes. Right, pcie_bus_safe is the option that doesn't really change too much, it is assuming the firmware set everything up properly. > With pci=pcie_bus_perf > > [ 1.985777] pcie_bus_configure_settings, config 2 > [ 1.985783] pcie_bus_configure_set > [ 1.985810] pcieport 0000:00:00.0: Max Payload Size set to 256/ > 256 (was 128), Max Read Rq 256 > [ 1.985818] pcie_bus_configure_set > [ 1.985875] pci 0000:01:00.0: Max Payload Size set to 256/ 256 > (was 128), Max Read Rq 256 > [ 1.985882] pcie_bus_configure_set > [ 1.985939] pci 0000:01:00.1: Max Payload Size set to 256/ 256 > (was 128), Max Read Rq 256 > > Is this log what you expect? Yes, that matches what the code seems to do. > If MRSS is clamped to lowest, then this would work with out a quirk, > and has to be unconditional (all cases, safe, performance etc). Well, except in this is working around a defect in Keystone. There are going to be potential problems no matter which route you take because some drivers do blindly alter the MRRS, and when the core ARM code starts calling pcie_bus_configure_settings() it will alter the MRRS as well. So, the quirk solution is only going to work in some cases. Adding pci=pcie_bus_perf to the command line in the DTS as a work around for the defect might be the most comprehensive option? > I would like to go with the quirk approach until this discussion > concludes the next step to fix this issue. May be someone can take > owner ship of this change at PCI core level? Well, the core code isn't wrong, it just isn't designed to address this sort of HW defect. Regards, Jason
On Thu, May 22, 2014 at 06:20:19PM -0400, Murali Karicheri wrote: >> What is the MPSS? > > MPS published in DEV CAP is 1 (256 bytes). In our IP for some reason, > mrss is set to 2 though IP > support only 256 bytes. I am trying to resolve this with IP team. If > this is an IP issue, then > software will overwrite this to 1. The MRSS of the root port bridge is very rarely used, it is OK to be larger than the MPS, but I'm not sure it makes much sense as a default. > So the mps will not get written to mrss for performance mode. So the > check seems not right. It should be changed to So, looking at this more closely, the MRRS does not *have* to be smaller than the MPS, but it probably performs better if it is. I think that reflects what the code is doing. The fact your root complex can't segment replies is a serious defect, and there is no infrastructure to support that in the kernel. Adding something to this generic code might be your best option to cover the majority of cases. It would also be good to know for sure that it can correctly segment a 256 byte read into 128 byte packets, and that only 512 is mishandled. Jason
On 5/15/2014 12:23 PM, Arnd Bergmann wrote: > On Thursday 15 May 2014 12:01:32 Murali Karicheri wrote: > >> +static int >> +keystone_pcie_fault(unsigned long addr, unsigned int fsr, >> + struct pt_regs *regs) >> +{ >> + unsigned long instr = *(unsigned long *) instruction_pointer(regs); >> + >> + if ((instr & 0x0e100090) == 0x00100090) { >> + int reg = (instr >> 12) & 15; >> + >> + regs->uregs[reg] = -1; >> + regs->ARM_pc += 4; >> + } >> + >> + return 0; >> +} > This needs to check in the PCI host registers what happened and do > some appropriate action. If the fault was not caused by PCIe, it > should not be ignored here but get passed on to the next handler. > > Arnd, The PCI controller is not able to detect the abort error from the EP and this gets reported to ARM as an Asynchronous external abort. PCI fault handler needs to consume this fault, otherwise it can't work on the Keystone h/w. For example pci-imx6.c has /* Added for PCI abort handling */ static int imx6q_pcie_abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { return 0; } Other examples are 1) arch/arm/mach-cns3xxx/pci.c static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { if (fsr & (1 << 10)) regs->ARM_pc += 4; return 0; } 2) arch/.arm/plat-iop/pci.c /* * When a PCI device does not exist during config cycles, the 80200 gets a * bus error instead of returning 0xffffffff. This handler simply returns. */ static int iop3xx_pci_abort(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { DBG("PCI abort: address = 0x%08lx fsr = 0x%03x PC = 0x%08lx LR = 0x%08lx\n", addr, fsr, regs->ARM_pc, regs->ARM_lr); /* * If it was an imprecise abort, then we need to correct the * return address to be _after_ the instruction. */ if (fsr & (1 << 10)) regs->ARM_pc += 4; return 0; } I suggest we document this as done in plat-iop. Let me know as I need to post my v2 of the patch this week that incorporates all the comments so far on v1. Murali
diff --git a/Documentation/devicetree/bindings/pci/pcie-keystone.txt b/Documentation/devicetree/bindings/pci/pcie-keystone.txt new file mode 100644 index 0000000..17cf261 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/pcie-keystone.txt @@ -0,0 +1,68 @@ +Keystone PCIE Root complex device tree bindings +----------------------------------------------- + +Sample bindings shown below:- + + - Remove ti,enable-linktrain if boot loader already does Link training and do EP + configuration. + - Remove ti,init-phy if boot loader already initialize the phy and sets up pcie + link + + pcie0_phy: pciephy@2320000 { + #address-cells = <1>; + #size-cells = <1>; + #phy-cells = <0>; + compatible = "ti,keystone-phy"; + reg = <0x02320000 0x4000>; + reg-names = "reg_serdes"; + }; + + pcie@21800000 { + compatible = "ti,keystone-pcie"; + device_type = "pci"; + clocks = <&clkpcie>; + clock-names = "pcie"; + #address-cells = <3>; + #size-cells = <2>; + reg = <0x21800000 0x1000>, <0x0262014c 4>; + reg-names = "reg_rc_app", "reg_devcfg"; + interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; /* Error IRQ */ + + ranges = <0x00000800 0 0x21801000 0x21801000 0 0x0002000 /* Configuration space */ + 0x81000000 0 0 0x24000000 0 0x4000 /* downstream I/O */ + 0x82000000 0 0x50000000 0x50000000 0 0x10000000>; /* non-prefetchable memory */ + + num-lanes = <2>; + ti,enable-linktrain; + ti,init-phy; + + /* PCIE phy */ + phys = <&pcie0_phy>; + phy-names = "pcie-phy"; + + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 1 &pcie_intc 1>, // INT A + <0 0 0 2 &pcie_intc 2>, // INT B + <0 0 0 3 &pcie_intc 3>, // INT C + <0 0 0 4 &pcie_intc 4>; // INT D + + pcie_intc: legacy-interrupt-controller { + interrupt-controller; + #interrupt-cells = <1>; + interrupt-parent = <&gic>; + interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 28 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 29 IRQ_TYPE_EDGE_RISING>; + }; + }; + diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index c4f4732..066d611 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -37,4 +37,12 @@ config PCI_RCAR_GEN2 Say Y here if you want internal PCI support on R-Car Gen2 SoC. There are 3 internal PCI controllers available with a single built-in EHCI/OHCI host controller present on each one. + +config PCI_KEYSTONE + bool "TI Keystone PCIe controller" + depends on ARCH_KEYSTONE + select PCIE_DW + select PCI_DW_OLD + select PCIEPORTBUS + select PHY_TI_KEYSTONE endmenu diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index be5d939..10e02f9 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -5,3 +5,4 @@ obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o obj-$(CONFIG_PCI_DW_OLD) += pci-dw-old-msi.o pci-dw-old.o +obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c new file mode 100644 index 0000000..2636717 --- /dev/null +++ b/drivers/pci/host/pci-keystone.c @@ -0,0 +1,400 @@ +/* + * PCIe host controller driver for Texas Instruments Keystone SoCs + * + * Copyright (C) 2013-2014 Texas Instruments., Ltd. + * http://www.ti.com + * + * Author: Murali Karicheri <m-karicheri2@ti.com> + * Implementation based on pci-exynos.c and pcie-designware.c + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/irqchip/chained_irq.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/irqdomain.h> +#include <linux/module.h> +#include <linux/msi.h> +#include <linux/of_irq.h> +#include <linux/of.h> +#include <linux/of_pci.h> +#include <linux/platform_device.h> +#include <linux/phy/phy.h> +#include <linux/resource.h> +#include <linux/signal.h> + +#include "pcie-designware.h" +#include "pci-dw-old.h" + +#define DRIVER_NAME "keystone-pcie" + +/* driver specific constants */ +#define MAX_MSI_HOST_IRQS 8 +#define MAX_LEGACY_HOST_IRQS 4 + +/* RC mode settings */ +#define PCIE_RC_MODE (BIT(2)) +#define PCIE_MODE_MASK (BIT(1) | BIT(2)) + +static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) +{ + return sys->private_data; +} + +struct keystone_pcie { + struct clk *clk; + int en_link_train; + struct pcie_port pp; + + int num_legacy_host_irqs; + int legacy_host_irqs[MAX_LEGACY_HOST_IRQS]; + struct device_node *np_intc; + + int num_msi_host_irqs; + int msi_host_irqs[MAX_MSI_HOST_IRQS]; +}; + +#define to_keystone_pcie(x) container_of(x, struct keystone_pcie, pp) + +static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie) +{ + struct pcie_port *pp = &ks_pcie->pp; + int count = 200; + + dw_pcie_setup_rc(pp); + + /* check if the link is up or not */ + while (!dw_pcie_link_up(pp)) { + usleep_range(100, 1000); + if (--count) + continue; + dev_err(pp->dev, "phy link never came up\n"); + return -EINVAL; + } + + return 0; +} + +static void ks_pcie_msi_irq_handler(unsigned int irq, struct irq_desc *desc) +{ + struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); + u32 offset = irq - ks_pcie->msi_host_irqs[0]; + struct pcie_port *pp = &ks_pcie->pp; + struct irq_chip *chip = irq_desc_get_chip(desc); + + dev_dbg(pp->dev, "ks_pcie_msi_irq_handler, irq %d\n", irq); + + /* + * The chained irq handler installation would have replaced normal + * interrupt driver handler so we need to take care of mask/unmask and + * ack operation. + */ + chained_irq_enter(chip, desc); + dw_old_handle_msi_irq(pp, offset); + chained_irq_exit(chip, desc); +} + +/** + * ks_pcie_legacy_irq_handler() - Handle legacy interrupt + * @irq: IRQ line for legacy interrupts + * @desc: Pointer to irq descriptor + * + * Traverse through pending legacy interrupts and invoke handler for each. Also + * takes care of interrupt controller level mask/ack operation. + */ +static void ks_pcie_legacy_irq_handler(unsigned int irq, struct irq_desc *desc) +{ + struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc); + u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0]; + struct irq_chip *chip = irq_desc_get_chip(desc); + struct pcie_port *pp = &ks_pcie->pp; + + dev_dbg(ks_pcie->pp.dev, ": Handling legacy irq %d\n", irq); + + /* + * The chained irq handler installation would have replaced normal + * interrupt driver handler so we need to take care of mask/unmask and + * ack operation. + */ + chained_irq_enter(chip, desc); + dw_old_handle_legacy_irq(pp, irq_offset); + chained_irq_exit(chip, desc); +} + +static int ks_pcie_init_legacy_irqs(struct keystone_pcie *ks_pcie) +{ + struct device *dev = ks_pcie->pp.dev; + struct device_node *np_pcie = dev->of_node; + int num_legacy_irqs; + int ret = 0; + int i; + + /* get node */ + ks_pcie->np_intc = of_find_node_by_name(np_pcie, + "legacy-interrupt-controller"); + if (!ks_pcie->np_intc) { + dev_err(dev, + "Node for legacy-interrupt-controller is absent\n"); + goto out; + } + + /* get number of IRQs */ + num_legacy_irqs = of_irq_count(ks_pcie->np_intc); + if (!num_legacy_irqs) + goto out; + + if (num_legacy_irqs > MAX_LEGACY_HOST_IRQS) { + dev_err(dev, "Too many legacy interrupts defined %u\n", + num_legacy_irqs); + num_legacy_irqs = MAX_LEGACY_HOST_IRQS; + } + /* + * support upto MAX_LEGACY_HOST_IRQS host and legacy IRQs. + * In dt from index 0 to 3 + */ + for (i = 0; i < num_legacy_irqs; i++) { + ks_pcie->legacy_host_irqs[i] = + irq_of_parse_and_map(ks_pcie->np_intc, i); + if (ks_pcie->legacy_host_irqs[i] < 0) + break; + ks_pcie->num_legacy_host_irqs++; + } + + if (!ks_pcie->num_legacy_host_irqs) { + dev_err(dev, "Failed to get legacy interrupts\n"); + goto out; + } + +out: + return ret; +} + +static void ks_pcie_enable_interrupts(struct keystone_pcie *ks_pcie) +{ + struct pcie_port *pp = &ks_pcie->pp; + int i; + + /* Legacy IRQ */ + for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) { + irq_set_handler_data(ks_pcie->legacy_host_irqs[i], ks_pcie); + irq_set_chained_handler(ks_pcie->legacy_host_irqs[i], + ks_pcie_legacy_irq_handler); + } + dw_old_enable_legacy_irqs(pp); + + /* MSI IRQ */ + if (IS_ENABLED(CONFIG_PCI_MSI)) { + for (i = 0; i < ks_pcie->num_msi_host_irqs; i++) { + irq_set_chained_handler(ks_pcie->msi_host_irqs[i], + ks_pcie_msi_irq_handler); + irq_set_handler_data(ks_pcie->msi_host_irqs[i], + ks_pcie); + + } + } + + return; +} + +static int +keystone_pcie_fault(unsigned long addr, unsigned int fsr, + struct pt_regs *regs) +{ + unsigned long instr = *(unsigned long *) instruction_pointer(regs); + + if ((instr & 0x0e100090) == 0x00100090) { + int reg = (instr >> 12) & 15; + + regs->uregs[reg] = -1; + regs->ARM_pc += 4; + } + + return 0; +} + +static void __init ks_pcie_host_init(struct pcie_port *pp) +{ + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); + + ks_pcie_establish_link(ks_pcie); + dw_old_disable_bars(pp); + dw_old_setup_ob_regs(pp); + ks_pcie_enable_interrupts(ks_pcie); + writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8), + pp->dbi_base + PCI_IO_BASE); + /* + * PCIe access errors that result into OCP errors are caught by ARM as + * "External aborts" (Precise). + */ + hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0, + "external abort on linefetch"); +} + +int ks_pcie_link_up(struct pcie_port *pp) +{ + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); + + return dw_old_pcie_link_up(pp, ks_pcie->en_link_train); +} + +static struct pcie_host_ops keystone_pcie_host_ops = { + .rd_other_conf = dw_old_rd_other_conf, + .wr_other_conf = dw_old_wr_other_conf, + .link_up = ks_pcie_link_up, + .host_init = ks_pcie_host_init, + .get_msi_data = dw_old_get_msi_data, +}; + +static int add_pcie_port(struct keystone_pcie *ks_pcie, + struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct pcie_port *pp = &ks_pcie->pp; + struct resource *res; + int ret, i; + + ret = ks_pcie_init_legacy_irqs(ks_pcie); + if (ret) + return ret; + + if (IS_ENABLED(CONFIG_PCI_MSI)) { + /* + * support upto 32 MSI irqs mapped to 8 host IRQs. + * In dt from index 4 to 11 + */ + for (i = 0; i < MAX_MSI_HOST_IRQS; i++) { + ks_pcie->msi_host_irqs[i] = irq_of_parse_and_map(np, i); + if (ks_pcie->msi_host_irqs[i] < 0) + break; + ks_pcie->num_msi_host_irqs++; + } + } + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg_rc_app"); + pp->va_app_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(pp->va_app_base)) + return PTR_ERR(pp->va_app_base); + pp->app_base = res->start; + + pp->root_bus_nr = -1; + pp->version = DW_VERSION_OLD; + pp->ops = &keystone_pcie_host_ops; + spin_lock_init(&pp->conf_lock); + ret = dw_old_pcie_host_init(pp, ks_pcie->np_intc); + if (ret) { + dev_err(&pdev->dev, "failed to initialize host\n"); + return ret; + } + + return ret; +} + +static const struct of_device_id ks_pcie_of_match[] = { + { + .type = "pci", + .compatible = "ti,keystone-pcie", + }, + { }, +}; +MODULE_DEVICE_TABLE(of, ks_pcie_of_match); + +static int __exit ks_pcie_remove(struct platform_device *pdev) +{ + struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev); + + clk_disable_unprepare(ks_pcie->clk); + + return 0; +} + +static int __init ks_pcie_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + struct keystone_pcie *ks_pcie; + void __iomem *devstat; + struct pcie_port *pp; + struct resource *res; + struct phy *phy; + int ret = 0; + u32 val; + + ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie), + GFP_KERNEL); + if (!ks_pcie) { + dev_err(dev, "no memory for keystone pcie\n"); + return -ENOMEM; + } + + /* check if serdes phy needs to be enabled */ + if (of_get_property(np, "ti,init-phy", NULL) != NULL) { + phy = devm_phy_get(dev, "pcie-phy"); + if (IS_ERR(phy)) + return PTR_ERR(phy); + + ret = phy_init(phy); + if (ret < 0) + return ret; + } + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "reg_devcfg"); + devstat = devm_ioremap_resource(dev, res); + if (IS_ERR(devstat)) + return PTR_ERR(devstat); + + /* enable RC mode in devcfg */ + val = readl(devstat); + val &= ~PCIE_MODE_MASK; + val |= PCIE_RC_MODE; + writel(val, devstat); + + /* check if we need to enable link training */ + ks_pcie->en_link_train = + (of_get_property(np, "ti,enable-linktrain", NULL) != NULL); + + pp = &ks_pcie->pp; + pp->dev = dev; + + ks_pcie->clk = devm_clk_get(dev, "pcie"); + if (IS_ERR(ks_pcie->clk)) { + dev_err(dev, "Failed to get pcie rc clock\n"); + return PTR_ERR(ks_pcie->clk); + } + ret = clk_prepare_enable(ks_pcie->clk); + if (ret) + return ret; + + ret = add_pcie_port(ks_pcie, pdev); + if (ret < 0) + goto fail_clk; + + platform_set_drvdata(pdev, ks_pcie); + dev_info(dev, "pcie rc probe success\n"); + + return 0; + +fail_clk: + clk_disable_unprepare(ks_pcie->clk); + + return ret; +} + +static struct platform_driver ks_pcie_driver __refdata = { + .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), + }, +}; + +module_platform_driver(ks_pcie_driver); + +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); +MODULE_DESCRIPTION("Keystone PCIe host controller driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index e729206..b3e12c2 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3651,3 +3651,16 @@ void pci_dev_specific_enable_acs(struct pci_dev *dev) } } } +#ifdef CONFIG_PCI_KEYSTONE +/* + * The KeyStone PCIe controller has maximum read request size of 256 bytes. + */ +static void quirk_limit_readrequest(struct pci_dev *dev) +{ + int readrq = pcie_get_readrq(dev); + + if (readrq > 256) + pcie_set_readrq(dev, 256); +} +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest); +#endif /* CONFIG_PCI_KEYSTONE */