[PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Vidya Sagar]

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(-)

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

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Vidya Sagar]

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

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Lorenzo Pieralisi]

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
> 

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Vidya Sagar]

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
>>

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Lorenzo Pieralisi]

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
> > > 
> 

Re: [PATCH] PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30

[Vidya Sagar]

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
>>>>
>>