| Message ID | 20190618073810.30270-1-vidyas@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30 | expand |
On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: > Currently Relaxed Ordering bit in the configuration space is enabled for > all devices whereas it should be enabled only for root ports for Tegra20 > and Tegra30 chips to avoid deadlock in hardware. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > drivers/pci/controller/pci-tegra.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On 6/20/2019 4:48 PM, Thierry Reding wrote: > On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: >> Currently Relaxed Ordering bit in the configuration space is enabled for >> all devices whereas it should be enabled only for root ports for Tegra20 >> and Tegra30 chips to avoid deadlock in hardware. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> drivers/pci/controller/pci-tegra.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) > > Acked-by: Thierry Reding <treding@nvidia.com> > Bjorn / Lorenzo, Can you please consider this patch? Thanks, Vidya Sagar
On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: > Currently Relaxed Ordering bit in the configuration space is enabled for > all devices whereas it should be enabled only for root ports for Tegra20 > and Tegra30 chips to avoid deadlock in hardware. This is a fix so I think you had better add a Fixes: tag and if the erratum is a public document it would be good to add it to the log to explain what the problem is. Thanks, Lorenzo > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > 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 464ba2538d52..bc7be369c1b3 100644 > --- a/drivers/pci/controller/pci-tegra.c > +++ b/drivers/pci/controller/pci-tegra.c > @@ -545,12 +545,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 >
On 6/27/2019 3:36 PM, Lorenzo Pieralisi wrote: > On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: >> Currently Relaxed Ordering bit in the configuration space is enabled for >> all devices whereas it should be enabled only for root ports for Tegra20 >> and Tegra30 chips to avoid deadlock in hardware. > > This is a fix so I think you had better add a Fixes: tag and if the > erratum is a public document it would be good to add it to the log > to explain what the problem is. > > Thanks, > Lorenzo This has been there from the beginning. I mean, if I have to give a tag as part of Fixes: , then I should give the tag that adds this driver itself. Is that fine? I can quote the following document section 31.1 for Tegra20, but the same information is not mentioned in Tegra30 specific public document. https://www.chiark.greenend.org.uk/~theom/riscos/docs/Tegra2_TRM_DP04508001v01p.pdf Is it fine to just quote only this document? Thanks, Vidya Sagar > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> 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 464ba2538d52..bc7be369c1b3 100644 >> --- a/drivers/pci/controller/pci-tegra.c >> +++ b/drivers/pci/controller/pci-tegra.c >> @@ -545,12 +545,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 >>
On Thu, Jun 27, 2019 at 08:41:37PM +0530, Vidya Sagar wrote: > On 6/27/2019 3:36 PM, Lorenzo Pieralisi wrote: > > On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: > > > Currently Relaxed Ordering bit in the configuration space is enabled for > > > all devices whereas it should be enabled only for root ports for Tegra20 > > > and Tegra30 chips to avoid deadlock in hardware. > > > > This is a fix so I think you had better add a Fixes: tag and if the > > erratum is a public document it would be good to add it to the log > > to explain what the problem is. > > > > Thanks, > > Lorenzo > This has been there from the beginning. I mean, if I have to give a tag as part > of Fixes: , then I should give the tag that adds this driver itself. Is that fine? > I can quote the following document section 31.1 for Tegra20, but the same information > is not mentioned in Tegra30 specific public document. > https://www.chiark.greenend.org.uk/~theom/riscos/docs/Tegra2_TRM_DP04508001v01p.pdf > Is it fine to just quote only this document? I am pretty sure an errata document should exist about this and Nvidia owns it (if it is not public, well, there is nothing to quote then); I would not quote this website, it does not look like Nvidia official documentation. For the Fixes: tag, it is up to you, to me this seems like a critical bug (deadlock in hardware) and I do not think you want to ship stable kernels with it. It depends on the entity of the bug we are fixing, I leave it to you to decide the best course of action, I am just trying to help. Lorenzo > Thanks, > Vidya Sagar > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > 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 464ba2538d52..bc7be369c1b3 100644 > > > --- a/drivers/pci/controller/pci-tegra.c > > > +++ b/drivers/pci/controller/pci-tegra.c > > > @@ -545,12 +545,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 > > > >
On 6/27/2019 8:53 PM, Lorenzo Pieralisi wrote: > On Thu, Jun 27, 2019 at 08:41:37PM +0530, Vidya Sagar wrote: >> On 6/27/2019 3:36 PM, Lorenzo Pieralisi wrote: >>> On Tue, Jun 18, 2019 at 01:08:10PM +0530, Vidya Sagar wrote: >>>> Currently Relaxed Ordering bit in the configuration space is enabled for >>>> all devices whereas it should be enabled only for root ports for Tegra20 >>>> and Tegra30 chips to avoid deadlock in hardware. >>> >>> This is a fix so I think you had better add a Fixes: tag and if the >>> erratum is a public document it would be good to add it to the log >>> to explain what the problem is. >>> >>> Thanks, >>> Lorenzo >> This has been there from the beginning. I mean, if I have to give a tag as part >> of Fixes: , then I should give the tag that adds this driver itself. Is that fine? >> I can quote the following document section 31.1 for Tegra20, but the same information >> is not mentioned in Tegra30 specific public document. >> https://www.chiark.greenend.org.uk/~theom/riscos/docs/Tegra2_TRM_DP04508001v01p.pdf >> Is it fine to just quote only this document? > > I am pretty sure an errata document should exist about this and Nvidia > owns it (if it is not public, well, there is nothing to quote then); I > would not quote this website, it does not look like Nvidia official > documentation. We have the TRM (Technical Reference Manual) document available for Tegra20 at https://developer.nvidia.com/embedded/downloads#?search=tegra%202 but basic registration is required to download the document. I'll mention this link in the commit message. > > For the Fixes: tag, it is up to you, to me this seems like a > critical bug (deadlock in hardware) and I do not think you > want to ship stable kernels with it. It depends on the entity > of the bug we are fixing, I leave it to you to decide the > best course of action, I am just trying to help. Since there is no specific commit that introduced this issue and the issue has been there from the beginning, I'm of the opinion that giving initial commit tag (the one that added the driver itself) for 'Fixes: tag doesn't really help here. > > Lorenzo > >> Thanks, >> Vidya Sagar >>> >>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>>> --- >>>> 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 464ba2538d52..bc7be369c1b3 100644 >>>> --- a/drivers/pci/controller/pci-tegra.c >>>> +++ b/drivers/pci/controller/pci-tegra.c >>>> @@ -545,12 +545,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 >>>> >>
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index 464ba2538d52..bc7be369c1b3 100644 --- a/drivers/pci/controller/pci-tegra.c +++ b/drivers/pci/controller/pci-tegra.c @@ -545,12 +545,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) {
Currently Relaxed Ordering bit in the configuration space is enabled for all devices whereas it should be enabled only for root ports for Tegra20 and Tegra30 chips to avoid deadlock in hardware. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- drivers/pci/controller/pci-tegra.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)