On 7/4/2019 6:25 PM, Lorenzo Pieralisi wrote:
On Wed, Jul 03, 2019 at 10:20:23PM +0530, Vidya Sagar wrote:
On 7/3/2019 10:09 PM, Lorenzo Pieralisi wrote:
On Mon, Jul 01, 2019 at 04:39:42PM +0530, Vidya Sagar wrote:
Currently Relaxed Ordering bit in the configuration space is enabled for
all devices, but, as per the Technical Reference Manual of Tegra20 which is
What devices ?
All PCIe devices, because, current quirk uses PCI_ANY_ID for both Vendor-ID
and Device-ID and that makes it applicable for all PCIe devices.
And this is true regardless of whether the PCI tegra controller is
*the* actual controller in the system, aka as long as the Tegra driver
is compiled in, relaxed ordering is forced for any PCIe device.
This is gross and I do believe the fix should be propagated back to
the commit that introduced it, it should have never been implemented
like this.
Agree.
This was there in the very first commit of this driver i.e.
77ffc1465cec32489889d6bc9c288b7b0d2ce9fb which added this driver for Tegra20
as arch/arm/mach-tegra/pcie.c Since there was no device-tree concept back then,
could it be the case that this quirk comes into picture *only* if kernel is built
for Tegra20 and not otherwise? Even then, I agree that using PCI_ANY_ID is quite
wild way of applying a quirk.
available at https://developer.nvidia.com/embedded/downloads#?search=tegra%202
in Sec 34.1, it is mentioned that Relexed Ordering bit needs to be enabled in
its root ports to avoid deadlock in hardware. The same is applicable for
Tegra30 as well though it is not explicitly mentioned in Tegra30 TRM document,
but the same should not be extended to root ports of other Tegra SoCs or
other hosts.
Should not or must not ? What does this sentence mean ?
I think it is 'must not' here. Since the hardware deadlock issue is not present
in any other root ports other than Tegra20 and Tegra30, this quirk must not be
extended to any other root ports.
Can we try to be more precise please ?
As I said before the commit log must be clear to anyone reading it
even if he has no background information.
Would the following work?
Currently Relaxed Ordering bit in the configuration space is enabled
for all PCIe devices as the quirk uses PCI_ANY_ID for both Vendor-ID
and Device-ID, but, as per the Technical Reference Manual of Tegra20
which is available at
https://developer.nvidia.com/embedded/downloads#?search=tegra%202 in
Sec 34.1, it is mentioned that Relexed Ordering bit needs to be
enabled in its root ports to avoid deadlock in hardware. The same is
applicable for Tegra30 as well though it is not explicitly mentioned
in Tegra30 TRM document, but the same must not be extended to root
ports of other Tegra SoCs or other hosts as the same issue doesn't
exist there.
More or less, I will tweak it myself. Regardless the way it is
handled at the moment is really really not acceptable, one more
reason why this patch should be backported IMO.
Lorenzo
Lorenzo
Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
---
V2:
* Modified commit message to include reference to Tegra20 TRM document.
drivers/pci/controller/pci-tegra.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 9cc03a2549c0..241760aa15bd 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -787,12 +787,15 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_fixup_class);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_fixup_class);
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_fixup_class);
-/* Tegra PCIE requires relaxed ordering */
+/* Tegra20 and Tegra30 PCIE requires relaxed ordering */
static void tegra_pcie_relax_enable(struct pci_dev *dev)
{
pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
}
-DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_relax_enable);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0bf1, tegra_pcie_relax_enable);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1c, tegra_pcie_relax_enable);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0e1d, tegra_pcie_relax_enable);
static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
{
--
2.17.1
