PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30
diff mbox series

Message ID 20190618073810.30270-1-vidyas@nvidia.com
State New
Headers show
Series
  • PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30
Related show

Commit Message

Vidya Sagar June 18, 2019, 7:38 a.m. UTC
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(-)

Comments

Thierry Reding June 20, 2019, 11:18 a.m. UTC | #1
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>
Vidya Sagar June 26, 2019, 9:48 a.m. UTC | #2
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
Lorenzo Pieralisi June 27, 2019, 10:06 a.m. UTC | #3
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
>
Vidya Sagar June 27, 2019, 3:11 p.m. UTC | #4
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
>>
Lorenzo Pieralisi June 27, 2019, 3:23 p.m. UTC | #5
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
> > > 
>
Vidya Sagar July 1, 2019, 10:50 a.m. UTC | #6
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
>>>>
>>

Patch
diff mbox series

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