On Thu, Jul 04, 2019 at 08:34:01PM +0530, Vidya Sagar wrote: > 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. This is an unadulterated mess. AFAICS, this quirk forces Relaxed Ordering on any PCIe device on *any* ARM64 platform - DT and ACPI alike. I will add a Fixes: tag and you must ensure this is backported, I am not happy at all about this. Lorenzo > > > > > 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 > > > > > > > > >
