diff mbox

PCI: MVEBU: Use Device ID and revision from underlying endpoint

Message ID 1391597749-29807-1-git-send-email-andrew@lunn.ch (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Andrew Lunn Feb. 5, 2014, 10:55 a.m. UTC
Marvell SoCs place the SoC number into the PCIe endpoint device ID.
The SoC stepping is placed into the PCIe revision. The old plat-orion
PCIe driver allowed this information to be seen in user space with a
simple lspci command.

The new driver places a virtual PCI-PCI bridge on top of these
endpoints. It has its own hard coded PCI device ID. Thus it is no
longer possible to see what the SoC is using lspci.

When initializing the PCI-PCI bridge, set its device ID and revision
from the underlying endpoint, thus restoring this functionality.
Debian would like to use this in order to aid installing the correct
DTB file.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/pci/host/pci-mvebu.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Comments

Thomas Petazzoni Feb. 5, 2014, 4:21 p.m. UTC | #1
Dear Andrew Lunn,

On Wed,  5 Feb 2014 11:55:49 +0100, Andrew Lunn wrote:
> Marvell SoCs place the SoC number into the PCIe endpoint device ID.
> The SoC stepping is placed into the PCIe revision. The old plat-orion
> PCIe driver allowed this information to be seen in user space with a
> simple lspci command.
> 
> The new driver places a virtual PCI-PCI bridge on top of these
> endpoints. It has its own hard coded PCI device ID. Thus it is no
> longer possible to see what the SoC is using lspci.
> 
> When initializing the PCI-PCI bridge, set its device ID and revision
> from the underlying endpoint, thus restoring this functionality.
> Debian would like to use this in order to aid installing the correct
> DTB file.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/pci/host/pci-mvebu.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

I have two comments below, though.

> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13478ecd4113..0e79665afd44 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -60,14 +60,6 @@
>  #define PCIE_DEBUG_CTRL         0x1a60
>  #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
>  
> -/*
> - * This product ID is registered by Marvell, and used when the Marvell
> - * SoC is not the root complex, but an endpoint on the PCIe bus. It is
> - * therefore safe to re-use this PCI ID for our emulated PCI-to-PCI
> - * bridge.
> - */
> -#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 0x7846
> -
>  /* PCI configuration space of a PCI-to-PCI bridge */
>  struct mvebu_sw_pci_bridge {
>  	u16 vendor;
> @@ -388,7 +380,8 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
>  
>  	bridge->class = PCI_CLASS_BRIDGE_PCI;
>  	bridge->vendor = PCI_VENDOR_ID_MARVELL;

This could also have been replaced by:

	bridge->vendor = mvebu_readl(port, PCIE_DEV_ID_OFF) & 0xff;

> -	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
> +	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
> +	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;

On Armada 370 and XP, this field is apparently always 0x0, so not very
useful. But if it's useful on other mvebu SoCs, that's fine, it's just
an informative field anyway.

Thanks!

Thomas
Andrew Lunn Feb. 5, 2014, 4:54 p.m. UTC | #2
> > @@ -388,7 +380,8 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
> >  
> >  	bridge->class = PCI_CLASS_BRIDGE_PCI;
> >  	bridge->vendor = PCI_VENDOR_ID_MARVELL;
> 
> This could also have been replaced by:
> 
> 	bridge->vendor = mvebu_readl(port, PCIE_DEV_ID_OFF) & 0xff;

O.K, but do we ever expect it not to be PCI_VENDOR_ID_MARVELL on the
underlying hardware?

> 
> > -	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
> > +	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
> > +	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
> 
> On Armada 370 and XP, this field is apparently always 0x0, so not very
> useful. But if it's useful on other mvebu SoCs, that's fine, it's just
> an informative field anyway.

Humm, that should be the stepping, unless i have made a mistake. The
code Gregory wrote for mvebu-soc-id.c does:

       /* SoC ID */
        soc_dev_id = readl(pci_base + PCIE_DEV_ID_OFF) >> 16;

        /* SoC revision */
        soc_rev = readl(pci_base + PCIE_DEV_REV_OFF) & SOC_REV_MASK;

However, the box i'm testing on has stepping 0, so it is hard to test.
Could you test this in an OpenBlocks AX3 B0, or some other B0 device?

Thanks
	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Feb. 5, 2014, 5:03 p.m. UTC | #3
Dear Andrew Lunn,

On Wed, 5 Feb 2014 17:54:32 +0100, Andrew Lunn wrote:
> > > @@ -388,7 +380,8 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
> > >  
> > >  	bridge->class = PCI_CLASS_BRIDGE_PCI;
> > >  	bridge->vendor = PCI_VENDOR_ID_MARVELL;
> > 
> > This could also have been replaced by:
> > 
> > 	bridge->vendor = mvebu_readl(port, PCIE_DEV_ID_OFF) & 0xff;
> 
> O.K, but do we ever expect it not to be PCI_VENDOR_ID_MARVELL on the
> underlying hardware?

No, I don't expect it to be different. Which is why I said it "could"
have been replaced, because I am not sure it is worth doing it.

> > > -	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
> > > +	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
> > > +	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
> > 
> > On Armada 370 and XP, this field is apparently always 0x0, so not very
> > useful. But if it's useful on other mvebu SoCs, that's fine, it's just
> > an informative field anyway.
> 
> Humm, that should be the stepping, unless i have made a mistake. The
> code Gregory wrote for mvebu-soc-id.c does:
> 
>        /* SoC ID */
>         soc_dev_id = readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
> 
>         /* SoC revision */
>         soc_rev = readl(pci_base + PCIE_DEV_REV_OFF) & SOC_REV_MASK;
> 
> However, the box i'm testing on has stepping 0, so it is hard to test.
> Could you test this in an OpenBlocks AX3 B0, or some other B0 device?

Tested on a B0 now, and the revision returned is indeed 0x2, so you're
correct. Seems like the datasheet that indicates the value is 0x0 is
wrong :)

Thanks!

Thomas
Jason Cooper Feb. 5, 2014, 6:42 p.m. UTC | #4
On Wed, Feb 05, 2014 at 11:55:49AM +0100, Andrew Lunn wrote:
> Marvell SoCs place the SoC number into the PCIe endpoint device ID.
> The SoC stepping is placed into the PCIe revision. The old plat-orion
> PCIe driver allowed this information to be seen in user space with a
> simple lspci command.
> 
> The new driver places a virtual PCI-PCI bridge on top of these
> endpoints. It has its own hard coded PCI device ID. Thus it is no
> longer possible to see what the SoC is using lspci.
> 
> When initializing the PCI-PCI bridge, set its device ID and revision
> from the underlying endpoint, thus restoring this functionality.
> Debian would like to use this in order to aid installing the correct
> DTB file.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/pci/host/pci-mvebu.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Acked-by: Jason Cooper <jason@lakedaemon.net>

Also, since this fixes a userspace regression:

Fixes: 45361a4fe4464 ("pci: PCIe driver for Marvell Armada 370/XP systems")
Cc: <stable@vger.kernel.org> # v3.11+

thx,

Jason.

> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13478ecd4113..0e79665afd44 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -60,14 +60,6 @@
>  #define PCIE_DEBUG_CTRL         0x1a60
>  #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
>  
> -/*
> - * This product ID is registered by Marvell, and used when the Marvell
> - * SoC is not the root complex, but an endpoint on the PCIe bus. It is
> - * therefore safe to re-use this PCI ID for our emulated PCI-to-PCI
> - * bridge.
> - */
> -#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 0x7846
> -
>  /* PCI configuration space of a PCI-to-PCI bridge */
>  struct mvebu_sw_pci_bridge {
>  	u16 vendor;
> @@ -388,7 +380,8 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
>  
>  	bridge->class = PCI_CLASS_BRIDGE_PCI;
>  	bridge->vendor = PCI_VENDOR_ID_MARVELL;
> -	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
> +	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
> +	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
>  	bridge->header_type = PCI_HEADER_TYPE_BRIDGE;
>  	bridge->cache_line_size = 0x10;
>  
> -- 
> 1.8.5.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 12, 2014, 9:06 p.m. UTC | #5
On Wed, Feb 05, 2014 at 11:55:49AM +0100, Andrew Lunn wrote:
> Marvell SoCs place the SoC number into the PCIe endpoint device ID.
> The SoC stepping is placed into the PCIe revision. The old plat-orion
> PCIe driver allowed this information to be seen in user space with a
> simple lspci command.
> 
> The new driver places a virtual PCI-PCI bridge on top of these
> endpoints. It has its own hard coded PCI device ID. Thus it is no
> longer possible to see what the SoC is using lspci.
> 
> When initializing the PCI-PCI bridge, set its device ID and revision
> from the underlying endpoint, thus restoring this functionality.
> Debian would like to use this in order to aid installing the correct
> DTB file.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Applied to pci/host-mvebu with acks from Thomas and Jason.  Since it fixes
a regression, I cherry-picked it into for-linus for v3.14.  Thanks!

Bjorn

> ---
>  drivers/pci/host/pci-mvebu.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13478ecd4113..0e79665afd44 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -60,14 +60,6 @@
>  #define PCIE_DEBUG_CTRL         0x1a60
>  #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
>  
> -/*
> - * This product ID is registered by Marvell, and used when the Marvell
> - * SoC is not the root complex, but an endpoint on the PCIe bus. It is
> - * therefore safe to re-use this PCI ID for our emulated PCI-to-PCI
> - * bridge.
> - */
> -#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 0x7846
> -
>  /* PCI configuration space of a PCI-to-PCI bridge */
>  struct mvebu_sw_pci_bridge {
>  	u16 vendor;
> @@ -388,7 +380,8 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
>  
>  	bridge->class = PCI_CLASS_BRIDGE_PCI;
>  	bridge->vendor = PCI_VENDOR_ID_MARVELL;
> -	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
> +	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
> +	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
>  	bridge->header_type = PCI_HEADER_TYPE_BRIDGE;
>  	bridge->cache_line_size = 0x10;
>  
> -- 
> 1.8.5.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13478ecd4113..0e79665afd44 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -60,14 +60,6 @@ 
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
 
-/*
- * This product ID is registered by Marvell, and used when the Marvell
- * SoC is not the root complex, but an endpoint on the PCIe bus. It is
- * therefore safe to re-use this PCI ID for our emulated PCI-to-PCI
- * bridge.
- */
-#define MARVELL_EMULATED_PCI_PCI_BRIDGE_ID 0x7846
-
 /* PCI configuration space of a PCI-to-PCI bridge */
 struct mvebu_sw_pci_bridge {
 	u16 vendor;
@@ -388,7 +380,8 @@  static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
 
 	bridge->class = PCI_CLASS_BRIDGE_PCI;
 	bridge->vendor = PCI_VENDOR_ID_MARVELL;
-	bridge->device = MARVELL_EMULATED_PCI_PCI_BRIDGE_ID;
+	bridge->device = mvebu_readl(port, PCIE_DEV_ID_OFF) >> 16;
+	bridge->revision = mvebu_readl(port, PCIE_DEV_REV_OFF) & 0xff;
 	bridge->header_type = PCI_HEADER_TYPE_BRIDGE;
 	bridge->cache_line_size = 0x10;