[PATCH 0/2] Tegra PCI multiplatform fixes

[PATCH 0/2] Tegra PCI multiplatform fixes

[Lucas Stach]

Two of the registered fixups in the Tegra PCI driver
would be executed unconditionally and potentially
harm PCI support on other platforms than Tegra when
running a multiplatform kernel.

I've tested that this doesn't alter PCI behavior on both
T30 Beaver and T124 Jetson-TK1.

Lucas Stach (2):
  PCI: tegra: apply relaxed ordering fixup only on Tegra
  PCI: tegra: remove bogus bridge setup fixup

 drivers/pci/host/pci-tegra.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.1.1

[PATCH 0/2] Tegra PCI multiplatform fixes

[Lucas Stach]

Two of the registered fixups in the Tegra PCI driver
would be executed unconditionally and potentially
harm PCI support on other platforms than Tegra when
running a multiplatform kernel.

I've tested that this doesn't alter PCI behavior on both
T30 Beaver and T124 Jetson-TK1.

Lucas Stach (2):
  PCI: tegra: apply relaxed ordering fixup only on Tegra
  PCI: tegra: remove bogus bridge setup fixup

 drivers/pci/host/pci-tegra.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.1.1

[PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

The fixup to enable relaxed ordering on all PCI devices was
executed unconditionally if the Tegra PCI host driver was
built into the kernel. This doesn't play nice with a
multiplatform kernel executed on other platforms which
may not need this fixup.

Make sure to only apply the fixup if the root port is
a Tegra.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 3d43874319be..d5a14f22ebb8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -647,10 +647,34 @@ 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);
 
+static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pci_dev *root_bridge;
+
+	/* walk up the PCIe hierarchy to the first level below the root bus */
+	while (bus->parent && bus->parent->self)
+		bus = bus->parent;
+
+	/*
+	 * If there is no bridge on the bus the passed device is the root
+	 * bridge itself.
+	 */
+	root_bridge = bus->self ? bus->self : dev;
+	if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
+	    (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
+	     root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
+	     root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
+		return 1;
+
+	return 0;
+}
 /* Tegra 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);
+	if (tegra_pcie_root_is_tegra(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);
 
-- 
2.1.1

[PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

The fixup to enable relaxed ordering on all PCI devices was
executed unconditionally if the Tegra PCI host driver was
built into the kernel. This doesn't play nice with a
multiplatform kernel executed on other platforms which
may not need this fixup.

Make sure to only apply the fixup if the root port is
a Tegra.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 3d43874319be..d5a14f22ebb8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -647,10 +647,34 @@ 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);
 
+static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pci_dev *root_bridge;
+
+	/* walk up the PCIe hierarchy to the first level below the root bus */
+	while (bus->parent && bus->parent->self)
+		bus = bus->parent;
+
+	/*
+	 * If there is no bridge on the bus the passed device is the root
+	 * bridge itself.
+	 */
+	root_bridge = bus->self ? bus->self : dev;
+	if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
+	    (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
+	     root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
+	     root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
+		return 1;
+
+	return 0;
+}
 /* Tegra 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);
+	if (tegra_pcie_root_is_tegra(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);
 
-- 
2.1.1

[PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Lucas Stach]

The bridge setup is already done by generic code
while scanning the buses. Do not duplicate (or potentially
alter) this setup as a fixup.

Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index d5a14f22ebb8..0ef22505cead 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -624,19 +624,6 @@ static void tegra_pcie_port_free(struct tegra_pcie_port *port)
 	devm_kfree(pcie->dev, port);
 }
 
-static void tegra_pcie_fixup_bridge(struct pci_dev *dev)
-{
-	u16 reg;
-
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
-		pci_read_config_word(dev, PCI_COMMAND, &reg);
-		reg |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
-			PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
-		pci_write_config_word(dev, PCI_COMMAND, reg);
-	}
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_fixup_bridge);
-
 /* Tegra PCIE root complex wrongly reports device class */
 static void tegra_pcie_fixup_class(struct pci_dev *dev)
 {
-- 
2.1.1

[PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Lucas Stach]

The bridge setup is already done by generic code
while scanning the buses. Do not duplicate (or potentially
alter) this setup as a fixup.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/host/pci-tegra.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index d5a14f22ebb8..0ef22505cead 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -624,19 +624,6 @@ static void tegra_pcie_port_free(struct tegra_pcie_port *port)
 	devm_kfree(pcie->dev, port);
 }
 
-static void tegra_pcie_fixup_bridge(struct pci_dev *dev)
-{
-	u16 reg;
-
-	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
-		pci_read_config_word(dev, PCI_COMMAND, &reg);
-		reg |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
-			PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
-		pci_write_config_word(dev, PCI_COMMAND, reg);
-	}
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_fixup_bridge);
-
 /* Tegra PCIE root complex wrongly reports device class */
 static void tegra_pcie_fixup_class(struct pci_dev *dev)
 {
-- 
2.1.1

Re: [PATCH 0/2] Tegra PCI multiplatform fixes

[Lucas Stach]

Thierry,

could you please have a look at this series? It is needed for Tegra PCI
to be good citizen in a multiplatform kernel configuration and I would
really like to see this going in.

Thanks,
Lucas

Am Donnerstag, den 13.11.2014, 20:37 +0100 schrieb Lucas Stach:
> Two of the registered fixups in the Tegra PCI driver
> would be executed unconditionally and potentially
> harm PCI support on other platforms than Tegra when
> running a multiplatform kernel.
> 
> I've tested that this doesn't alter PCI behavior on both
> T30 Beaver and T124 Jetson-TK1.
> 
> Lucas Stach (2):
>   PCI: tegra: apply relaxed ordering fixup only on Tegra
>   PCI: tegra: remove bogus bridge setup fixup
> 
>  drivers/pci/host/pci-tegra.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 

Re: [PATCH 0/2] Tegra PCI multiplatform fixes

[Lucas Stach]

Thierry,

could you please have a look at this series? It is needed for Tegra PCI
to be good citizen in a multiplatform kernel configuration and I would
really like to see this going in.

Thanks,
Lucas

Am Donnerstag, den 13.11.2014, 20:37 +0100 schrieb Lucas Stach:
> Two of the registered fixups in the Tegra PCI driver
> would be executed unconditionally and potentially
> harm PCI support on other platforms than Tegra when
> running a multiplatform kernel.
> 
> I've tested that this doesn't alter PCI behavior on both
> T30 Beaver and T124 Jetson-TK1.
> 
> Lucas Stach (2):
>   PCI: tegra: apply relaxed ordering fixup only on Tegra
>   PCI: tegra: remove bogus bridge setup fixup
> 
>  drivers/pci/host/pci-tegra.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 

Re: [PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Alexandre Courbot]

On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The bridge setup is already done by generic code
> while scanning the buses. Do not duplicate (or potentially
> alter) this setup as a fixup.

Jetson seems to be happy with this change.

Tested-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Alexandre Courbot]

On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> The bridge setup is already done by generic code
> while scanning the buses. Do not duplicate (or potentially
> alter) this setup as a fixup.

Jetson seems to be happy with this change.

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Alexandre Courbot]

Hi Lucas,

Apologies for taking so long to come back to this. The patch looks ok
to me, just a minor comment about the Tegra PCI detection:

On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> The fixup to enable relaxed ordering on all PCI devices was
> executed unconditionally if the Tegra PCI host driver was
> built into the kernel. This doesn't play nice with a
> multiplatform kernel executed on other platforms which
> may not need this fixup.
>
> Make sure to only apply the fixup if the root port is
> a Tegra.
>
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 3d43874319be..d5a14f22ebb8 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -647,10 +647,34 @@ 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);
>
> +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> +{
> +       struct pci_bus *bus = dev->bus;
> +       struct pci_dev *root_bridge;
> +
> +       /* walk up the PCIe hierarchy to the first level below the root bus */
> +       while (bus->parent && bus->parent->self)
> +               bus = bus->parent;
> +
> +       /*
> +        * If there is no bridge on the bus the passed device is the root
> +        * bridge itself.
> +        */
> +       root_bridge = bus->self ? bus->self : dev;
> +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> +               return 1;

I am not very familiar with PCI so sorry if these are stupid
questions, but where do these device IDs come from? Are they needed at
all, e.g. can't you just test against root_bridge->vendor ==
PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
to increase as new chips get released? If that's the case, how can we
make sure we won't forget to update it?

If you need to test against the device ID, it might be more legible
(and easier to update) if you use a switch case, e.g:

    if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
        return 0;
    switch (root_bridge->device) {
    case 0x0bf0:
    case 0x0bf1:
    case 0x0e1c:
    case 0x0e1d:
    case 0x0e12:
    case 0x0e12:
        return 1;
    default:
        return 0;
    }

Otherwise I have not noticed any problem using this patch.

Tested-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Alexandre Courbot]

Hi Lucas,

Apologies for taking so long to come back to this. The patch looks ok
to me, just a minor comment about the Tegra PCI detection:

On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> The fixup to enable relaxed ordering on all PCI devices was
> executed unconditionally if the Tegra PCI host driver was
> built into the kernel. This doesn't play nice with a
> multiplatform kernel executed on other platforms which
> may not need this fixup.
>
> Make sure to only apply the fixup if the root port is
> a Tegra.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 3d43874319be..d5a14f22ebb8 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -647,10 +647,34 @@ 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);
>
> +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> +{
> +       struct pci_bus *bus = dev->bus;
> +       struct pci_dev *root_bridge;
> +
> +       /* walk up the PCIe hierarchy to the first level below the root bus */
> +       while (bus->parent && bus->parent->self)
> +               bus = bus->parent;
> +
> +       /*
> +        * If there is no bridge on the bus the passed device is the root
> +        * bridge itself.
> +        */
> +       root_bridge = bus->self ? bus->self : dev;
> +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> +               return 1;

I am not very familiar with PCI so sorry if these are stupid
questions, but where do these device IDs come from? Are they needed at
all, e.g. can't you just test against root_bridge->vendor ==
PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
to increase as new chips get released? If that's the case, how can we
make sure we won't forget to update it?

If you need to test against the device ID, it might be more legible
(and easier to update) if you use a switch case, e.g:

    if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
        return 0;
    switch (root_bridge->device) {
    case 0x0bf0:
    case 0x0bf1:
    case 0x0e1c:
    case 0x0e1d:
    case 0x0e12:
    case 0x0e12:
        return 1;
    default:
        return 0;
    }

Otherwise I have not noticed any problem using this patch.

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> Hi Lucas,
> 
> Apologies for taking so long to come back to this. The patch looks ok
> to me, just a minor comment about the Tegra PCI detection:
> 
> On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > The fixup to enable relaxed ordering on all PCI devices was
> > executed unconditionally if the Tegra PCI host driver was
> > built into the kernel. This doesn't play nice with a
> > multiplatform kernel executed on other platforms which
> > may not need this fixup.
> >
> > Make sure to only apply the fixup if the root port is
> > a Tegra.
> >
> > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index 3d43874319be..d5a14f22ebb8 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -647,10 +647,34 @@ 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);
> >
> > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > +{
> > +       struct pci_bus *bus = dev->bus;
> > +       struct pci_dev *root_bridge;
> > +
> > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > +       while (bus->parent && bus->parent->self)
> > +               bus = bus->parent;
> > +
> > +       /*
> > +        * If there is no bridge on the bus the passed device is the root
> > +        * bridge itself.
> > +        */
> > +       root_bridge = bus->self ? bus->self : dev;
> > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > +               return 1;
> 
> I am not very familiar with PCI so sorry if these are stupid
> questions, but where do these device IDs come from? Are they needed at
> all, e.g. can't you just test against root_bridge->vendor ==
> PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> to increase as new chips get released? If that's the case, how can we
> make sure we won't forget to update it?
> 

The device IDs are assigned by NVIDIA HW for the different Tegra PCI
generation/link width combinations. Note that the K1 TRM is wrong as it
still lists the T30 device IDs, instead of the ones used on K1.

While we technically could test only against vendor==nvidia I don't
think it is entirely safe. As this is a PCI fixup it will get executed
on every device running a kernel including this PCI host bridge driver. 

So only testing for the vendor assumes that every ARM device with a PCI
host bridge built by NVIDIA will be a Tegra. Do you think this is a
reasonable assertion? I'm on the fence here.
 
> If you need to test against the device ID, it might be more legible
> (and easier to update) if you use a switch case, e.g:
> 
>     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
>         return 0;
>     switch (root_bridge->device) {
>     case 0x0bf0:
>     case 0x0bf1:
>     case 0x0e1c:
>     case 0x0e1d:
>     case 0x0e12:
>     case 0x0e12:
>         return 1;
>     default:
>         return 0;
>     }
> 
Right, this looks nicer and easier to extend. I'll have to think about
if we need the device IDs at all and respin accordingly.

> Otherwise I have not noticed any problem using this patch.
> 
> Tested-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Thanks,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> Hi Lucas,
> 
> Apologies for taking so long to come back to this. The patch looks ok
> to me, just a minor comment about the Tegra PCI detection:
> 
> On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > The fixup to enable relaxed ordering on all PCI devices was
> > executed unconditionally if the Tegra PCI host driver was
> > built into the kernel. This doesn't play nice with a
> > multiplatform kernel executed on other platforms which
> > may not need this fixup.
> >
> > Make sure to only apply the fixup if the root port is
> > a Tegra.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index 3d43874319be..d5a14f22ebb8 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -647,10 +647,34 @@ 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);
> >
> > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > +{
> > +       struct pci_bus *bus = dev->bus;
> > +       struct pci_dev *root_bridge;
> > +
> > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > +       while (bus->parent && bus->parent->self)
> > +               bus = bus->parent;
> > +
> > +       /*
> > +        * If there is no bridge on the bus the passed device is the root
> > +        * bridge itself.
> > +        */
> > +       root_bridge = bus->self ? bus->self : dev;
> > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > +               return 1;
> 
> I am not very familiar with PCI so sorry if these are stupid
> questions, but where do these device IDs come from? Are they needed at
> all, e.g. can't you just test against root_bridge->vendor ==
> PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> to increase as new chips get released? If that's the case, how can we
> make sure we won't forget to update it?
> 

The device IDs are assigned by NVIDIA HW for the different Tegra PCI
generation/link width combinations. Note that the K1 TRM is wrong as it
still lists the T30 device IDs, instead of the ones used on K1.

While we technically could test only against vendor==nvidia I don't
think it is entirely safe. As this is a PCI fixup it will get executed
on every device running a kernel including this PCI host bridge driver. 

So only testing for the vendor assumes that every ARM device with a PCI
host bridge built by NVIDIA will be a Tegra. Do you think this is a
reasonable assertion? I'm on the fence here.
 
> If you need to test against the device ID, it might be more legible
> (and easier to update) if you use a switch case, e.g:
> 
>     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
>         return 0;
>     switch (root_bridge->device) {
>     case 0x0bf0:
>     case 0x0bf1:
>     case 0x0e1c:
>     case 0x0e1d:
>     case 0x0e12:
>     case 0x0e12:
>         return 1;
>     default:
>         return 0;
>     }
> 
Right, this looks nicer and easier to extend. I'll have to think about
if we need the device IDs at all and respin accordingly.

> Otherwise I have not noticed any problem using this patch.
> 
> Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Bjorn Helgaas]

On Thu, Nov 13, 2014 at 08:37:36PM +0100, Lucas Stach wrote:
> The fixup to enable relaxed ordering on all PCI devices was
> executed unconditionally if the Tegra PCI host driver was
> built into the kernel. This doesn't play nice with a
> multiplatform kernel executed on other platforms which
> may not need this fixup.
> 
> Make sure to only apply the fixup if the root port is
> a Tegra.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 3d43874319be..d5a14f22ebb8 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -647,10 +647,34 @@ 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);
>  
> +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	struct pci_dev *root_bridge;
> +
> +	/* walk up the PCIe hierarchy to the first level below the root bus */
> +	while (bus->parent && bus->parent->self)
> +		bus = bus->parent;
> +
> +	/*
> +	 * If there is no bridge on the bus the passed device is the root
> +	 * bridge itself.
> +	 */
> +	root_bridge = bus->self ? bus->self : dev;
> +	if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> +	    (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> +	     root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> +	     root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))

This looks like a good fix.  Can you rework it slightly to:

  - Use "root_port" instead of "root_bridge" to match the spec terminology.
    When people say "root bridge," they usually mean a host bridge, which
    is not a PCI device.

  - Is it feasible to factor out the hierarchy traversal into something
    like a separate pcie_root_port(pci_dev *) interface?  I think that
    might be useful other places as well.

Bjorn

> +		return 1;
> +
> +	return 0;
> +}
>  /* Tegra 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);
> +	if (tegra_pcie_root_is_tegra(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);
>  
> -- 
> 2.1.1
> 

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Thierry Reding]

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

On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > Hi Lucas,
> > 
> > Apologies for taking so long to come back to this. The patch looks ok
> > to me, just a minor comment about the Tegra PCI detection:
> > 
> > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > The fixup to enable relaxed ordering on all PCI devices was
> > > executed unconditionally if the Tegra PCI host driver was
> > > built into the kernel. This doesn't play nice with a
> > > multiplatform kernel executed on other platforms which
> > > may not need this fixup.
> > >
> > > Make sure to only apply the fixup if the root port is
> > > a Tegra.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > index 3d43874319be..d5a14f22ebb8 100644
> > > --- a/drivers/pci/host/pci-tegra.c
> > > +++ b/drivers/pci/host/pci-tegra.c
> > > @@ -647,10 +647,34 @@ 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);
> > >
> > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > +{
> > > +       struct pci_bus *bus = dev->bus;
> > > +       struct pci_dev *root_bridge;
> > > +
> > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > +       while (bus->parent && bus->parent->self)
> > > +               bus = bus->parent;
> > > +
> > > +       /*
> > > +        * If there is no bridge on the bus the passed device is the root
> > > +        * bridge itself.
> > > +        */
> > > +       root_bridge = bus->self ? bus->self : dev;
> > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > +               return 1;
> > 
> > I am not very familiar with PCI so sorry if these are stupid
> > questions, but where do these device IDs come from? Are they needed at
> > all, e.g. can't you just test against root_bridge->vendor ==
> > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > to increase as new chips get released? If that's the case, how can we
> > make sure we won't forget to update it?
> > 
> 
> The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> generation/link width combinations. Note that the K1 TRM is wrong as it
> still lists the T30 device IDs, instead of the ones used on K1.
> 
> While we technically could test only against vendor==nvidia I don't
> think it is entirely safe. As this is a PCI fixup it will get executed
> on every device running a kernel including this PCI host bridge driver. 
> 
> So only testing for the vendor assumes that every ARM device with a PCI
> host bridge built by NVIDIA will be a Tegra. Do you think this is a
> reasonable assertion? I'm on the fence here.
>  
> > If you need to test against the device ID, it might be more legible
> > (and easier to update) if you use a switch case, e.g:
> > 
> >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> >         return 0;
> >     switch (root_bridge->device) {
> >     case 0x0bf0:
> >     case 0x0bf1:
> >     case 0x0e1c:
> >     case 0x0e1d:
> >     case 0x0e12:
> >     case 0x0e12:
> >         return 1;
> >     default:
> >         return 0;
> >     }
> > 
> Right, this looks nicer and easier to extend. I'll have to think about
> if we need the device IDs at all and respin accordingly.

I think using the device ID is fine. If nothing else it'll at least
document the various device IDs. Perhaps you could extend this patch
with comments as to which device ID maps to which SoC. Or better yet
add them to include/linux/pci_ids.h with names matching the SoC.

Also I'm wondering if perhaps it'd be better yet to add these as a table
of struct pci_device_id:s and use pci_match_id() to avoid the switch
here. Granted, the table will be bigger in size because of the unused
fields, but it'd more clearly separate the data and code.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Thierry Reding]

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

On Thu, Nov 13, 2014 at 08:37:37PM +0100, Lucas Stach wrote:
> The bridge setup is already done by generic code
> while scanning the buses. Do not duplicate (or potentially
> alter) this setup as a fixup.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/host/pci-tegra.c | 13 -------------
>  1 file changed, 13 deletions(-)

I've been carrying the same patch locally for a while now, I thought I
had sent it out long ago, but I can't find any record of that, so I
probably didn't. Anyway:

Reviewed-by: Thierry Reding <treding@nvidia.com>
Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > Hi Lucas,
> > > 
> > > Apologies for taking so long to come back to this. The patch looks ok
> > > to me, just a minor comment about the Tegra PCI detection:
> > > 
> > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > executed unconditionally if the Tegra PCI host driver was
> > > > built into the kernel. This doesn't play nice with a
> > > > multiplatform kernel executed on other platforms which
> > > > may not need this fixup.
> > > >
> > > > Make sure to only apply the fixup if the root port is
> > > > a Tegra.
> > > >
> > > > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > ---
> > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > --- a/drivers/pci/host/pci-tegra.c
> > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > @@ -647,10 +647,34 @@ 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);
> > > >
> > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > +{
> > > > +       struct pci_bus *bus = dev->bus;
> > > > +       struct pci_dev *root_bridge;
> > > > +
> > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > +       while (bus->parent && bus->parent->self)
> > > > +               bus = bus->parent;
> > > > +
> > > > +       /*
> > > > +        * If there is no bridge on the bus the passed device is the root
> > > > +        * bridge itself.
> > > > +        */
> > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > +               return 1;
> > > 
> > > I am not very familiar with PCI so sorry if these are stupid
> > > questions, but where do these device IDs come from? Are they needed at
> > > all, e.g. can't you just test against root_bridge->vendor ==
> > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > to increase as new chips get released? If that's the case, how can we
> > > make sure we won't forget to update it?
> > > 
> > 
> > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > generation/link width combinations. Note that the K1 TRM is wrong as it
> > still lists the T30 device IDs, instead of the ones used on K1.
> > 
> > While we technically could test only against vendor==nvidia I don't
> > think it is entirely safe. As this is a PCI fixup it will get executed
> > on every device running a kernel including this PCI host bridge driver. 
> > 
> > So only testing for the vendor assumes that every ARM device with a PCI
> > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > reasonable assertion? I'm on the fence here.
> >  
> > > If you need to test against the device ID, it might be more legible
> > > (and easier to update) if you use a switch case, e.g:
> > > 
> > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > >         return 0;
> > >     switch (root_bridge->device) {
> > >     case 0x0bf0:
> > >     case 0x0bf1:
> > >     case 0x0e1c:
> > >     case 0x0e1d:
> > >     case 0x0e12:
> > >     case 0x0e12:
> > >         return 1;
> > >     default:
> > >         return 0;
> > >     }
> > > 
> > Right, this looks nicer and easier to extend. I'll have to think about
> > if we need the device IDs at all and respin accordingly.
> 
> I think using the device ID is fine. If nothing else it'll at least
> document the various device IDs. Perhaps you could extend this patch
> with comments as to which device ID maps to which SoC. Or better yet
> add them to include/linux/pci_ids.h with names matching the SoC.
> 
The IDs used by the Tegra root ports are not shared between multiple
drivers, so no way for them to go into that file.

> Also I'm wondering if perhaps it'd be better yet to add these as a table
> of struct pci_device_id:s and use pci_match_id() to avoid the switch
> here. Granted, the table will be bigger in size because of the unused
> fields, but it'd more clearly separate the data and code.
> 
Hm, right. This seems like a good idea. I'll respin the series with the
feedback I received.

Thanks,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > Hi Lucas,
> > > 
> > > Apologies for taking so long to come back to this. The patch looks ok
> > > to me, just a minor comment about the Tegra PCI detection:
> > > 
> > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > executed unconditionally if the Tegra PCI host driver was
> > > > built into the kernel. This doesn't play nice with a
> > > > multiplatform kernel executed on other platforms which
> > > > may not need this fixup.
> > > >
> > > > Make sure to only apply the fixup if the root port is
> > > > a Tegra.
> > > >
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > ---
> > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > --- a/drivers/pci/host/pci-tegra.c
> > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > @@ -647,10 +647,34 @@ 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);
> > > >
> > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > +{
> > > > +       struct pci_bus *bus = dev->bus;
> > > > +       struct pci_dev *root_bridge;
> > > > +
> > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > +       while (bus->parent && bus->parent->self)
> > > > +               bus = bus->parent;
> > > > +
> > > > +       /*
> > > > +        * If there is no bridge on the bus the passed device is the root
> > > > +        * bridge itself.
> > > > +        */
> > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > +               return 1;
> > > 
> > > I am not very familiar with PCI so sorry if these are stupid
> > > questions, but where do these device IDs come from? Are they needed at
> > > all, e.g. can't you just test against root_bridge->vendor ==
> > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > to increase as new chips get released? If that's the case, how can we
> > > make sure we won't forget to update it?
> > > 
> > 
> > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > generation/link width combinations. Note that the K1 TRM is wrong as it
> > still lists the T30 device IDs, instead of the ones used on K1.
> > 
> > While we technically could test only against vendor==nvidia I don't
> > think it is entirely safe. As this is a PCI fixup it will get executed
> > on every device running a kernel including this PCI host bridge driver. 
> > 
> > So only testing for the vendor assumes that every ARM device with a PCI
> > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > reasonable assertion? I'm on the fence here.
> >  
> > > If you need to test against the device ID, it might be more legible
> > > (and easier to update) if you use a switch case, e.g:
> > > 
> > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > >         return 0;
> > >     switch (root_bridge->device) {
> > >     case 0x0bf0:
> > >     case 0x0bf1:
> > >     case 0x0e1c:
> > >     case 0x0e1d:
> > >     case 0x0e12:
> > >     case 0x0e12:
> > >         return 1;
> > >     default:
> > >         return 0;
> > >     }
> > > 
> > Right, this looks nicer and easier to extend. I'll have to think about
> > if we need the device IDs at all and respin accordingly.
> 
> I think using the device ID is fine. If nothing else it'll at least
> document the various device IDs. Perhaps you could extend this patch
> with comments as to which device ID maps to which SoC. Or better yet
> add them to include/linux/pci_ids.h with names matching the SoC.
> 
The IDs used by the Tegra root ports are not shared between multiple
drivers, so no way for them to go into that file.

> Also I'm wondering if perhaps it'd be better yet to add these as a table
> of struct pci_device_id:s and use pci_match_id() to avoid the switch
> here. Granted, the table will be bigger in size because of the unused
> fields, but it'd more clearly separate the data and code.
> 
Hm, right. This seems like a good idea. I'll respin the series with the
feedback I received.

Thanks,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Thierry Reding]

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

On Wed, Dec 10, 2014 at 01:23:40PM +0100, Lucas Stach wrote:
> Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> > On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > > Hi Lucas,
> > > > 
> > > > Apologies for taking so long to come back to this. The patch looks ok
> > > > to me, just a minor comment about the Tegra PCI detection:
> > > > 
> > > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > > executed unconditionally if the Tegra PCI host driver was
> > > > > built into the kernel. This doesn't play nice with a
> > > > > multiplatform kernel executed on other platforms which
> > > > > may not need this fixup.
> > > > >
> > > > > Make sure to only apply the fixup if the root port is
> > > > > a Tegra.
> > > > >
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > ---
> > > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > > --- a/drivers/pci/host/pci-tegra.c
> > > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > > @@ -647,10 +647,34 @@ 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);
> > > > >
> > > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > > +{
> > > > > +       struct pci_bus *bus = dev->bus;
> > > > > +       struct pci_dev *root_bridge;
> > > > > +
> > > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > > +       while (bus->parent && bus->parent->self)
> > > > > +               bus = bus->parent;
> > > > > +
> > > > > +       /*
> > > > > +        * If there is no bridge on the bus the passed device is the root
> > > > > +        * bridge itself.
> > > > > +        */
> > > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > > +               return 1;
> > > > 
> > > > I am not very familiar with PCI so sorry if these are stupid
> > > > questions, but where do these device IDs come from? Are they needed at
> > > > all, e.g. can't you just test against root_bridge->vendor ==
> > > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > > to increase as new chips get released? If that's the case, how can we
> > > > make sure we won't forget to update it?
> > > > 
> > > 
> > > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > > generation/link width combinations. Note that the K1 TRM is wrong as it
> > > still lists the T30 device IDs, instead of the ones used on K1.
> > > 
> > > While we technically could test only against vendor==nvidia I don't
> > > think it is entirely safe. As this is a PCI fixup it will get executed
> > > on every device running a kernel including this PCI host bridge driver. 
> > > 
> > > So only testing for the vendor assumes that every ARM device with a PCI
> > > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > > reasonable assertion? I'm on the fence here.
> > >  
> > > > If you need to test against the device ID, it might be more legible
> > > > (and easier to update) if you use a switch case, e.g:
> > > > 
> > > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > > >         return 0;
> > > >     switch (root_bridge->device) {
> > > >     case 0x0bf0:
> > > >     case 0x0bf1:
> > > >     case 0x0e1c:
> > > >     case 0x0e1d:
> > > >     case 0x0e12:
> > > >     case 0x0e12:
> > > >         return 1;
> > > >     default:
> > > >         return 0;
> > > >     }
> > > > 
> > > Right, this looks nicer and easier to extend. I'll have to think about
> > > if we need the device IDs at all and respin accordingly.
> > 
> > I think using the device ID is fine. If nothing else it'll at least
> > document the various device IDs. Perhaps you could extend this patch
> > with comments as to which device ID maps to which SoC. Or better yet
> > add them to include/linux/pci_ids.h with names matching the SoC.
> > 
> The IDs used by the Tegra root ports are not shared between multiple
> drivers, so no way for them to go into that file.

Since when has that been a requirement? Randomly grepping for a couple
of the IDs defined in that file they are either not used at all or in a
single driver.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Mittwoch, den 10.12.2014, 15:11 +0100 schrieb Thierry Reding:
> On Wed, Dec 10, 2014 at 01:23:40PM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> > > On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > > > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > > > Hi Lucas,
> > > > > 
> > > > > Apologies for taking so long to come back to this. The patch looks ok
> > > > > to me, just a minor comment about the Tegra PCI detection:
> > > > > 
> > > > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > > > executed unconditionally if the Tegra PCI host driver was
> > > > > > built into the kernel. This doesn't play nice with a
> > > > > > multiplatform kernel executed on other platforms which
> > > > > > may not need this fixup.
> > > > > >
> > > > > > Make sure to only apply the fixup if the root port is
> > > > > > a Tegra.
> > > > > >
> > > > > > Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > > ---
> > > > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > > > --- a/drivers/pci/host/pci-tegra.c
> > > > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > > > @@ -647,10 +647,34 @@ 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);
> > > > > >
> > > > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > > > +{
> > > > > > +       struct pci_bus *bus = dev->bus;
> > > > > > +       struct pci_dev *root_bridge;
> > > > > > +
> > > > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > > > +       while (bus->parent && bus->parent->self)
> > > > > > +               bus = bus->parent;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * If there is no bridge on the bus the passed device is the root
> > > > > > +        * bridge itself.
> > > > > > +        */
> > > > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > > > +               return 1;
> > > > > 
> > > > > I am not very familiar with PCI so sorry if these are stupid
> > > > > questions, but where do these device IDs come from? Are they needed at
> > > > > all, e.g. can't you just test against root_bridge->vendor ==
> > > > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > > > to increase as new chips get released? If that's the case, how can we
> > > > > make sure we won't forget to update it?
> > > > > 
> > > > 
> > > > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > > > generation/link width combinations. Note that the K1 TRM is wrong as it
> > > > still lists the T30 device IDs, instead of the ones used on K1.
> > > > 
> > > > While we technically could test only against vendor==nvidia I don't
> > > > think it is entirely safe. As this is a PCI fixup it will get executed
> > > > on every device running a kernel including this PCI host bridge driver. 
> > > > 
> > > > So only testing for the vendor assumes that every ARM device with a PCI
> > > > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > > > reasonable assertion? I'm on the fence here.
> > > >  
> > > > > If you need to test against the device ID, it might be more legible
> > > > > (and easier to update) if you use a switch case, e.g:
> > > > > 
> > > > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > > > >         return 0;
> > > > >     switch (root_bridge->device) {
> > > > >     case 0x0bf0:
> > > > >     case 0x0bf1:
> > > > >     case 0x0e1c:
> > > > >     case 0x0e1d:
> > > > >     case 0x0e12:
> > > > >     case 0x0e12:
> > > > >         return 1;
> > > > >     default:
> > > > >         return 0;
> > > > >     }
> > > > > 
> > > > Right, this looks nicer and easier to extend. I'll have to think about
> > > > if we need the device IDs at all and respin accordingly.
> > > 
> > > I think using the device ID is fine. If nothing else it'll at least
> > > document the various device IDs. Perhaps you could extend this patch
> > > with comments as to which device ID maps to which SoC. Or better yet
> > > add them to include/linux/pci_ids.h with names matching the SoC.
> > > 
> > The IDs used by the Tegra root ports are not shared between multiple
> > drivers, so no way for them to go into that file.
> 
> Since when has that been a requirement? Randomly grepping for a couple
> of the IDs defined in that file they are either not used at all or in a
> single driver.
> 
It's stated right in the header of that file and I've seen quite a few
occasions where this rule has been enforced. If there are entries in
there that are only used by a single driver that's either legacy entries
or bad review.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Lucas Stach]

Am Mittwoch, den 10.12.2014, 15:11 +0100 schrieb Thierry Reding:
> On Wed, Dec 10, 2014 at 01:23:40PM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> > > On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > > > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > > > Hi Lucas,
> > > > > 
> > > > > Apologies for taking so long to come back to this. The patch looks ok
> > > > > to me, just a minor comment about the Tegra PCI detection:
> > > > > 
> > > > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > > > executed unconditionally if the Tegra PCI host driver was
> > > > > > built into the kernel. This doesn't play nice with a
> > > > > > multiplatform kernel executed on other platforms which
> > > > > > may not need this fixup.
> > > > > >
> > > > > > Make sure to only apply the fixup if the root port is
> > > > > > a Tegra.
> > > > > >
> > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > > > --- a/drivers/pci/host/pci-tegra.c
> > > > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > > > @@ -647,10 +647,34 @@ 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);
> > > > > >
> > > > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > > > +{
> > > > > > +       struct pci_bus *bus = dev->bus;
> > > > > > +       struct pci_dev *root_bridge;
> > > > > > +
> > > > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > > > +       while (bus->parent && bus->parent->self)
> > > > > > +               bus = bus->parent;
> > > > > > +
> > > > > > +       /*
> > > > > > +        * If there is no bridge on the bus the passed device is the root
> > > > > > +        * bridge itself.
> > > > > > +        */
> > > > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > > > +               return 1;
> > > > > 
> > > > > I am not very familiar with PCI so sorry if these are stupid
> > > > > questions, but where do these device IDs come from? Are they needed at
> > > > > all, e.g. can't you just test against root_bridge->vendor ==
> > > > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > > > to increase as new chips get released? If that's the case, how can we
> > > > > make sure we won't forget to update it?
> > > > > 
> > > > 
> > > > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > > > generation/link width combinations. Note that the K1 TRM is wrong as it
> > > > still lists the T30 device IDs, instead of the ones used on K1.
> > > > 
> > > > While we technically could test only against vendor==nvidia I don't
> > > > think it is entirely safe. As this is a PCI fixup it will get executed
> > > > on every device running a kernel including this PCI host bridge driver. 
> > > > 
> > > > So only testing for the vendor assumes that every ARM device with a PCI
> > > > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > > > reasonable assertion? I'm on the fence here.
> > > >  
> > > > > If you need to test against the device ID, it might be more legible
> > > > > (and easier to update) if you use a switch case, e.g:
> > > > > 
> > > > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > > > >         return 0;
> > > > >     switch (root_bridge->device) {
> > > > >     case 0x0bf0:
> > > > >     case 0x0bf1:
> > > > >     case 0x0e1c:
> > > > >     case 0x0e1d:
> > > > >     case 0x0e12:
> > > > >     case 0x0e12:
> > > > >         return 1;
> > > > >     default:
> > > > >         return 0;
> > > > >     }
> > > > > 
> > > > Right, this looks nicer and easier to extend. I'll have to think about
> > > > if we need the device IDs at all and respin accordingly.
> > > 
> > > I think using the device ID is fine. If nothing else it'll at least
> > > document the various device IDs. Perhaps you could extend this patch
> > > with comments as to which device ID maps to which SoC. Or better yet
> > > add them to include/linux/pci_ids.h with names matching the SoC.
> > > 
> > The IDs used by the Tegra root ports are not shared between multiple
> > drivers, so no way for them to go into that file.
> 
> Since when has that been a requirement? Randomly grepping for a couple
> of the IDs defined in that file they are either not used at all or in a
> single driver.
> 
It's stated right in the header of that file and I've seen quite a few
occasions where this rule has been enforced. If there are entries in
there that are only used by a single driver that's either legacy entries
or bad review.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] PCI: tegra: apply relaxed ordering fixup only on Tegra

[Thierry Reding]

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

On Wed, Dec 10, 2014 at 03:15:55PM +0100, Lucas Stach wrote:
> Am Mittwoch, den 10.12.2014, 15:11 +0100 schrieb Thierry Reding:
> > On Wed, Dec 10, 2014 at 01:23:40PM +0100, Lucas Stach wrote:
> > > Am Mittwoch, den 10.12.2014, 13:13 +0100 schrieb Thierry Reding:
> > > > On Tue, Dec 09, 2014 at 11:28:17AM +0100, Lucas Stach wrote:
> > > > > Am Dienstag, den 09.12.2014, 12:23 +0900 schrieb Alexandre Courbot:
> > > > > > Hi Lucas,
> > > > > > 
> > > > > > Apologies for taking so long to come back to this. The patch looks ok
> > > > > > to me, just a minor comment about the Tegra PCI detection:
> > > > > > 
> > > > > > On Fri, Nov 14, 2014 at 4:37 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > > The fixup to enable relaxed ordering on all PCI devices was
> > > > > > > executed unconditionally if the Tegra PCI host driver was
> > > > > > > built into the kernel. This doesn't play nice with a
> > > > > > > multiplatform kernel executed on other platforms which
> > > > > > > may not need this fixup.
> > > > > > >
> > > > > > > Make sure to only apply the fixup if the root port is
> > > > > > > a Tegra.
> > > > > > >
> > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > > > > ---
> > > > > > >  drivers/pci/host/pci-tegra.c | 26 +++++++++++++++++++++++++-
> > > > > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > > > > > index 3d43874319be..d5a14f22ebb8 100644
> > > > > > > --- a/drivers/pci/host/pci-tegra.c
> > > > > > > +++ b/drivers/pci/host/pci-tegra.c
> > > > > > > @@ -647,10 +647,34 @@ 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);
> > > > > > >
> > > > > > > +static int tegra_pcie_root_is_tegra(struct pci_dev *dev)
> > > > > > > +{
> > > > > > > +       struct pci_bus *bus = dev->bus;
> > > > > > > +       struct pci_dev *root_bridge;
> > > > > > > +
> > > > > > > +       /* walk up the PCIe hierarchy to the first level below the root bus */
> > > > > > > +       while (bus->parent && bus->parent->self)
> > > > > > > +               bus = bus->parent;
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * If there is no bridge on the bus the passed device is the root
> > > > > > > +        * bridge itself.
> > > > > > > +        */
> > > > > > > +       root_bridge = bus->self ? bus->self : dev;
> > > > > > > +       if (root_bridge->vendor == PCI_VENDOR_ID_NVIDIA &&
> > > > > > > +           (root_bridge->device == 0x0bf0 || root_bridge->device == 0x0bf1 ||
> > > > > > > +            root_bridge->device == 0x0e1c || root_bridge->device == 0x0e1d ||
> > > > > > > +            root_bridge->device == 0x0e12 || root_bridge->device == 0x0e13))
> > > > > > > +               return 1;
> > > > > > 
> > > > > > I am not very familiar with PCI so sorry if these are stupid
> > > > > > questions, but where do these device IDs come from? Are they needed at
> > > > > > all, e.g. can't you just test against root_bridge->vendor ==
> > > > > > PCI_VENDOR_ID_NVIDIA to detect a NVIDIA root? Is the list susceptible
> > > > > > to increase as new chips get released? If that's the case, how can we
> > > > > > make sure we won't forget to update it?
> > > > > > 
> > > > > 
> > > > > The device IDs are assigned by NVIDIA HW for the different Tegra PCI
> > > > > generation/link width combinations. Note that the K1 TRM is wrong as it
> > > > > still lists the T30 device IDs, instead of the ones used on K1.
> > > > > 
> > > > > While we technically could test only against vendor==nvidia I don't
> > > > > think it is entirely safe. As this is a PCI fixup it will get executed
> > > > > on every device running a kernel including this PCI host bridge driver. 
> > > > > 
> > > > > So only testing for the vendor assumes that every ARM device with a PCI
> > > > > host bridge built by NVIDIA will be a Tegra. Do you think this is a
> > > > > reasonable assertion? I'm on the fence here.
> > > > >  
> > > > > > If you need to test against the device ID, it might be more legible
> > > > > > (and easier to update) if you use a switch case, e.g:
> > > > > > 
> > > > > >     if (root_bridge->vendor != PCI_VENDOR_ID_NVIDIA)
> > > > > >         return 0;
> > > > > >     switch (root_bridge->device) {
> > > > > >     case 0x0bf0:
> > > > > >     case 0x0bf1:
> > > > > >     case 0x0e1c:
> > > > > >     case 0x0e1d:
> > > > > >     case 0x0e12:
> > > > > >     case 0x0e12:
> > > > > >         return 1;
> > > > > >     default:
> > > > > >         return 0;
> > > > > >     }
> > > > > > 
> > > > > Right, this looks nicer and easier to extend. I'll have to think about
> > > > > if we need the device IDs at all and respin accordingly.
> > > > 
> > > > I think using the device ID is fine. If nothing else it'll at least
> > > > document the various device IDs. Perhaps you could extend this patch
> > > > with comments as to which device ID maps to which SoC. Or better yet
> > > > add them to include/linux/pci_ids.h with names matching the SoC.
> > > > 
> > > The IDs used by the Tegra root ports are not shared between multiple
> > > drivers, so no way for them to go into that file.
> > 
> > Since when has that been a requirement? Randomly grepping for a couple
> > of the IDs defined in that file they are either not used at all or in a
> > single driver.
> > 
> It's stated right in the header of that file and I've seen quite a few
> occasions where this rule has been enforced. If there are entries in
> there that are only used by a single driver that's either legacy entries
> or bad review.

Oh well. We could still have a list with symbolic names in the Tegra
driver to clarify which device ID belongs to which SoC. But both a
comment or a symbolic name are fine with me.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Bjorn Helgaas]

On Thu, Nov 13, 2014 at 08:37:37PM +0100, Lucas Stach wrote:
> The bridge setup is already done by generic code
> while scanning the buses. Do not duplicate (or potentially
> alter) this setup as a fixup.
> 
> Signed-off-by: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Applied to next-pci/host-tegra for v3.19, with Tested-by, Reviewed-by, and
Acked-by from Alexandre and Thierry.  This branch will be rebased to
v3.19-rc1 when it comes out.

Bjorn

> ---
>  drivers/pci/host/pci-tegra.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index d5a14f22ebb8..0ef22505cead 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -624,19 +624,6 @@ static void tegra_pcie_port_free(struct tegra_pcie_port *port)
>  	devm_kfree(pcie->dev, port);
>  }
>  
> -static void tegra_pcie_fixup_bridge(struct pci_dev *dev)
> -{
> -	u16 reg;
> -
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
> -		pci_read_config_word(dev, PCI_COMMAND, &reg);
> -		reg |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> -			PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
> -		pci_write_config_word(dev, PCI_COMMAND, reg);
> -	}
> -}
> -DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_fixup_bridge);
> -
>  /* Tegra PCIE root complex wrongly reports device class */
>  static void tegra_pcie_fixup_class(struct pci_dev *dev)
>  {
> -- 
> 2.1.1
> 

Re: [PATCH 2/2] PCI: tegra: remove bogus bridge setup fixup

[Bjorn Helgaas]

On Thu, Nov 13, 2014 at 08:37:37PM +0100, Lucas Stach wrote:
> The bridge setup is already done by generic code
> while scanning the buses. Do not duplicate (or potentially
> alter) this setup as a fixup.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Applied to next-pci/host-tegra for v3.19, with Tested-by, Reviewed-by, and
Acked-by from Alexandre and Thierry.  This branch will be rebased to
v3.19-rc1 when it comes out.

Bjorn

> ---
>  drivers/pci/host/pci-tegra.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index d5a14f22ebb8..0ef22505cead 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -624,19 +624,6 @@ static void tegra_pcie_port_free(struct tegra_pcie_port *port)
>  	devm_kfree(pcie->dev, port);
>  }
>  
> -static void tegra_pcie_fixup_bridge(struct pci_dev *dev)
> -{
> -	u16 reg;
> -
> -	if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
> -		pci_read_config_word(dev, PCI_COMMAND, &reg);
> -		reg |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> -			PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
> -		pci_write_config_word(dev, PCI_COMMAND, reg);
> -	}
> -}
> -DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_fixup_bridge);
> -
>  /* Tegra PCIE root complex wrongly reports device class */
>  static void tegra_pcie_fixup_class(struct pci_dev *dev)
>  {
> -- 
> 2.1.1
>