diff mbox

[V3,2/4] ARM: tegra: pcie: Add tegra3 support

Message ID 1370372252-4332-2-git-send-email-jagarwal@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jay Agarwal June 4, 2013, 6:57 p.m. UTC
- Enable PCIe root port 2 for Cardhu
- Make private data structure for each SoC
- Add required Tegra30 clocks and regulators
- Add Tegra30 specific code in enable controller
- Corrected logic in read/write config space to
  display right device number on bus 0
- Fixed unnecessary freeing of tegra_pcie_bus structure in tegra_pcie_remove
- Added Tegra30 specific properties in pci binding document

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.

Changes in V3:
- Added entry in required properties in pci document as per review comment
- Corrected logic in read/write config space to display right device number on bus 0
- Fixed unnecessary freeing of tegra_pcie_bus structure in tegra_pcie_remove
- Took care of other review comments to change data types and names of some members of tegra_pcie_soc_data structure

 .../bindings/pci/nvidia,tegra20-pcie.txt           |    2 +
 drivers/pci/host/pci-tegra.c                       |  157 ++++++++++++++++----
 2 files changed, 133 insertions(+), 26 deletions(-)

Comments

Stephen Warren June 4, 2013, 7:17 p.m. UTC | #1
On 06/04/2013 12:57 PM, Jay Agarwal wrote:
> - Enable PCIe root port 2 for Cardhu
> - Make private data structure for each SoC
> - Add required Tegra30 clocks and regulators
> - Add Tegra30 specific code in enable controller
> - Corrected logic in read/write config space to
>   display right device number on bus 0
> - Fixed unnecessary freeing of tegra_pcie_bus structure in tegra_pcie_remove
> - Added Tegra30 specific properties in pci binding document

> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> +- avdd-supply: Power supply for controller (1.05V)

> +  "cml": The Tegra clock of that name

Both those changes need to mention that those additions are only
required for Tegra30, not Tegra20. In other words,

+- avdd-supply: Power supply for controller (1.05V) (not required for
Tegra20)

+  "cml": The Tegra clock of that name (not required for Tegra20)

You probably also want to explicitly mention nvidia,tegra30-pcie as a
legal compatible value.

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

> +/* used to differentiate tegra chips code */
> +struct tegra_pcie_soc_data {
> +	unsigned int num_max_ports;

nit: "num max" seems redundant. max_ports or num_ports would be better.

>  struct tegra_pcie_port {
> @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
>  		struct tegra_pcie_port *port;
>  
>  		list_for_each_entry(port, &pcie->ports, list) {
> -			if (port->index + 1 == slot) {
> +			if (port->index == slot) {

This and the equivalent change in tegra_pcie_write_conf() seem like a
bug-fix unrelated to the addition of Tegra30 support. Hence, they should
be a separate patch.


> @@ -398,7 +422,6 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
>  			*value = 0xffffffff;
>  			return PCIBIOS_DEVICE_NOT_FOUND;
>  		}
> -
>  		addr += tegra_pcie_conf_offset(devfn, where);

Unnecessary white-space change.

> @@ -1549,10 +1660,9 @@ static int tegra_pcie_remove(struct platform_device *pdev)
>  	struct tegra_pcie_bus *bus;
>  	int err;
>  
> -	list_for_each_entry(bus, &pcie->busses, list) {
> +	list_for_each_entry(bus, &pcie->busses, list)
>  		vunmap(bus->area->addr);
> -		kfree(bus);
> -	}
> +	kfree(bus);

This doesn't look right. Can you please explain it further? This is
looping over every bus in a dynamic list, so surely each entry needs to
be freed? Did you do this to avoid a crash? If so, the issue is likely
that the loop should use list_for_each_entry_safe() rather than
list_for_each_entry().
Jay Agarwal June 5, 2013, 2:57 p.m. UTC | #2
> > diff --git
> > a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> > b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> 
> > +- avdd-supply: Power supply for controller (1.05V)
> 
> > +  "cml": The Tegra clock of that name
> 
> Both those changes need to mention that those additions are only required
> for Tegra30, not Tegra20. In other words,
> 
> +- avdd-supply: Power supply for controller (1.05V) (not required for
> Tegra20)
> 
> +  "cml": The Tegra clock of that name (not required for Tegra20)
> 
> You probably also want to explicitly mention nvidia,tegra30-pcie as a legal
> compatible value.
> 
> > diff --git a/drivers/pci/host/pci-tegra.c
> > b/drivers/pci/host/pci-tegra.c
> 
> > +/* used to differentiate tegra chips code */ struct
> > +tegra_pcie_soc_data {
> > +	unsigned int num_max_ports;
> 
> nit: "num max" seems redundant. max_ports or num_ports would be better.
> 
> >  struct tegra_pcie_port {
> > @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus,
> unsigned int devfn,
> >  		struct tegra_pcie_port *port;
> >
> >  		list_for_each_entry(port, &pcie->ports, list) {
> > -			if (port->index + 1 == slot) {
> > +			if (port->index == slot) {
> 
> This and the equivalent change in tegra_pcie_write_conf() seem like a bug-
> fix unrelated to the addition of Tegra30 support. Hence, they should be a
> separate patch.
> 
> 
> > @@ -398,7 +422,6 @@ static int tegra_pcie_read_conf(struct pci_bus *bus,
> unsigned int devfn,
> >  			*value = 0xffffffff;
> >  			return PCIBIOS_DEVICE_NOT_FOUND;
> >  		}
> > -
> >  		addr += tegra_pcie_conf_offset(devfn, where);
> 
> Unnecessary white-space change.
> 
> > @@ -1549,10 +1660,9 @@ static int tegra_pcie_remove(struct
> platform_device *pdev)
> >  	struct tegra_pcie_bus *bus;
> >  	int err;
> >
> > -	list_for_each_entry(bus, &pcie->busses, list) {
> > +	list_for_each_entry(bus, &pcie->busses, list)
> >  		vunmap(bus->area->addr);
> > -		kfree(bus);
> > -	}
> > +	kfree(bus);
> 
> This doesn't look right. Can you please explain it further? This is looping
> over every bus in a dynamic list, so surely each entry needs to be freed? Did
> you do this to avoid a crash? If so, the issue is likely that the loop should use
> list_for_each_entry_safe() rather than list_for_each_entry().
Yes you are right, it is not required. I will revert it along with taking care of other comments.
Thierry Reding June 10, 2013, 7:50 p.m. UTC | #3
On Tue, Jun 04, 2013 at 01:17:15PM -0600, Stephen Warren wrote:
> On 06/04/2013 12:57 PM, Jay Agarwal wrote:
[...]
> >  struct tegra_pcie_port {
> > @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
> >  		struct tegra_pcie_port *port;
> >  
> >  		list_for_each_entry(port, &pcie->ports, list) {
> > -			if (port->index + 1 == slot) {
> > +			if (port->index == slot) {
> 
> This and the equivalent change in tegra_pcie_write_conf() seem like a
> bug-fix unrelated to the addition of Tegra30 support. Hence, they should
> be a separate patch.

What exactly is this change supposed to fix? The description doesn't
provide any details about why this is required. Furthermore this was
done on purpose to model the Tegra PCIe controller according to what
typical Linux systems provide.

Device 0:00.0 is usually the root complex, and device 0:01.0, 0:02.0
etc are the root ports. The change proposed above makes 0:00.0 the
first root port, therefore breaking what systems usually expect.

Thierry
Jay Agarwal June 11, 2013, 4:43 a.m. UTC | #4
> * PGP Signed by an unknown key
> 
> On Tue, Jun 04, 2013 at 01:17:15PM -0600, Stephen Warren wrote:
> > On 06/04/2013 12:57 PM, Jay Agarwal wrote:
> [...]
> > >  struct tegra_pcie_port {
> > > @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus
> *bus, unsigned int devfn,
> > >  		struct tegra_pcie_port *port;
> > >
> > >  		list_for_each_entry(port, &pcie->ports, list) {
> > > -			if (port->index + 1 == slot) {
> > > +			if (port->index == slot) {
> >
> > This and the equivalent change in tegra_pcie_write_conf() seem like a
> > bug-fix unrelated to the addition of Tegra30 support. Hence, they
> > should be a separate patch.
> 
> What exactly is this change supposed to fix? The description doesn't provide
> any details about why this is required. Furthermore this was done on
> purpose to model the Tegra PCIe controller according to what typical Linux
> systems provide.

I have mentioned it in description as -> "Corrected logic in read/write config space to display right device number on bus 0"

> Device 0:00.0 is usually the root complex, and device 0:01.0, 0:02.0 etc are
> the root ports. The change proposed above makes 0:00.0 the first root port,
> therefore breaking what systems usually expect.
> 
I was seeing root port 2 in cardhu being enumerated as pci_bus 0000:03, which I thought should be pci_bus 0000:02, so made this change.

> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
Thierry Reding June 11, 2013, 10:16 a.m. UTC | #5
On Tue, Jun 11, 2013 at 10:13:38AM +0530, Jay Agarwal wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jun 04, 2013 at 01:17:15PM -0600, Stephen Warren wrote:
> > > On 06/04/2013 12:57 PM, Jay Agarwal wrote:
> > [...]
> > > >  struct tegra_pcie_port {
> > > > @@ -384,7 +408,7 @@ static int tegra_pcie_read_conf(struct pci_bus
> > *bus, unsigned int devfn,
> > > >  		struct tegra_pcie_port *port;
> > > >
> > > >  		list_for_each_entry(port, &pcie->ports, list) {
> > > > -			if (port->index + 1 == slot) {
> > > > +			if (port->index == slot) {
> > >
> > > This and the equivalent change in tegra_pcie_write_conf() seem like a
> > > bug-fix unrelated to the addition of Tegra30 support. Hence, they
> > > should be a separate patch.
> > 
> > What exactly is this change supposed to fix? The description doesn't provide
> > any details about why this is required. Furthermore this was done on
> > purpose to model the Tegra PCIe controller according to what typical Linux
> > systems provide.
> 
> I have mentioned it in description as -> "Corrected logic in read/write config space to display right device number on bus 0"
> 
> > Device 0:00.0 is usually the root complex, and device 0:01.0, 0:02.0 etc are
> > the root ports. The change proposed above makes 0:00.0 the first root port,
> > therefore breaking what systems usually expect.
> > 
> I was seeing root port 2 in cardhu being enumerated as pci_bus 0000:03, which I thought should be pci_bus 0000:02, so made this change.

Yes, that's done on purpose to mirror what a typical PCI tree looks
like, as I already explained. So unless this fixes a real bug I'll just
drop it while applying.

Thierry
Jay Agarwal June 11, 2013, 10:40 a.m. UTC | #6
> > I have mentioned it in description as -> "Corrected logic in read/write
> config space to display right device number on bus 0"
> >
> > > Device 0:00.0 is usually the root complex, and device 0:01.0, 0:02.0
> > > etc are the root ports. The change proposed above makes 0:00.0 the
> > > first root port, therefore breaking what systems usually expect.
> > >
> > I was seeing root port 2 in cardhu being enumerated as pci_bus 0000:03,
> which I thought should be pci_bus 0000:02, so made this change.
> 
> Yes, that's done on purpose to mirror what a typical PCI tree looks like, as I
> already explained. So unless this fixes a real bug I'll just drop it while
> applying.

Please apply V4 patch instead which does not contain this change along with taking care of other review comments.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
index 90c112f..4ac22ab 100644
--- a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
@@ -16,6 +16,7 @@  Required properties:
   "msi": The Tegra interrupt that is asserted when an MSI is received
 - pex-clk-supply: Supply voltage for internal reference clock
 - vdd-supply: Power supply for controller (1.05V)
+- avdd-supply: Power supply for controller (1.05V)
 - bus-range: Range of bus numbers associated with this controller
 - #address-cells: Address representation for root ports (must be 3)
   - cell 0 specifies the bus and device numbers of the root port:
@@ -48,6 +49,7 @@  Required properties:
   "afi": The Tegra clock of that name
   "pcie_xclk": The Tegra clock of that name
   "pll_e": The Tegra clock of that name
+  "cml": The Tegra clock of that name
 
 Root ports are defined as subnodes of the PCIe controller node.
 
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c2d0b22..27c39d8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1,5 +1,5 @@ 
 /*
- * PCIe host controller driver for TEGRA(2) SOCs
+ * PCIe host controller driver for TEGRA SOCs
  *
  * Copyright (c) 2010, CompuLab, Ltd.
  * Author: Mike Rapoport <mike@compulab.co.il>
@@ -50,7 +50,6 @@ 
 #include <asm/mach/pci.h>
 
 #define INT_PCI_MSI_NR (8 * 32)
-#define TEGRA_MAX_PORTS 2
 
 /* register definitions */
 
@@ -142,14 +141,15 @@ 
 #define  AFI_INTR_EN_DFPCI_DECERR	(1 << 5)
 #define  AFI_INTR_EN_AXI_DECERR		(1 << 6)
 #define  AFI_INTR_EN_FPCI_TIMEOUT	(1 << 7)
+#define AFI_INTR_EN_PRSNT_SENSE		(1 << 8)
 
 #define AFI_PCIE_CONFIG					0x0f8
 #define  AFI_PCIE_CONFIG_PCIE_DISABLE(x)		(1 << ((x) + 1))
 #define  AFI_PCIE_CONFIG_PCIE_DISABLE_ALL		0xe
 #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_MASK	(0xf << 20)
 #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_SINGLE	(0x0 << 20)
-#define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_420	(0x0 << 20)
 #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL	(0x1 << 20)
+#define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_420	(0x0 << 20)
 #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_222	(0x1 << 20)
 #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_411	(0x2 << 20)
 
@@ -160,8 +160,11 @@ 
 #define AFI_PEX1_CTRL			0x118
 #define AFI_PEX2_CTRL			0x128
 #define  AFI_PEX_CTRL_RST		(1 << 0)
+#define  AFI_PEX_CTRL_CLKREQ_EN		(1 << 1)
 #define  AFI_PEX_CTRL_REFCLK_EN		(1 << 3)
 
+#define  AFI_PEXBIAS_CTRL_0		0x168
+
 #define RP_VEND_XP	0x00000F00
 #define  RP_VEND_XP_DL_UP	(1 << 30)
 
@@ -175,7 +178,8 @@ 
 #define  PADS_CTL_TX_DATA_EN_1L	(1 << 6)
 #define  PADS_CTL_RX_DATA_EN_1L	(1 << 10)
 
-#define PADS_PLL_CTL				0x000000B8
+#define  PADS_PLL_CTL_T20			0x000000B8
+#define  PADS_PLL_CTL_T30			0x000000B4
 #define  PADS_PLL_CTL_RST_B4SM			(1 << 1)
 #define  PADS_PLL_CTL_LOCKDET			(1 << 8)
 #define  PADS_PLL_CTL_REFCLK_MASK		(0x3 << 16)
@@ -183,8 +187,11 @@ 
 #define  PADS_PLL_CTL_REFCLK_INTERNAL_CMOS	(1 << 16)
 #define  PADS_PLL_CTL_REFCLK_EXTERNAL		(2 << 16)
 #define  PADS_PLL_CTL_TXCLKREF_MASK		(0x1 << 20)
+#define  PADS_PLL_CTL_TXCLKREF_BUF_EN		(1 << 22)
 #define  PADS_PLL_CTL_TXCLKREF_DIV10		(0 << 20)
 #define  PADS_PLL_CTL_TXCLKREF_DIV5		(1 << 20)
+#define  PADS_REFCLK_CFG0			0x000000C8
+#define  PADS_REFCLK_CFG1			0x000000CC
 
 struct tegra_msi {
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
@@ -195,6 +202,19 @@  struct tegra_msi {
 	int irq;
 };
 
+/* used to differentiate tegra chips code */
+struct tegra_pcie_soc_data {
+	unsigned int num_max_ports;
+	unsigned int msi_base_shift;
+	u32 pads_pll_ctl;
+	u32 tx_ref_sel;
+	bool has_pex_clkreq_en;
+	bool has_pex_bias_ctrl;
+	bool has_intr_prsnt_sense;
+	bool has_avdd_supply;
+	bool has_cml_clk;
+};
+
 static inline struct tegra_msi *to_tegra_msi(struct msi_chip *chip)
 {
 	return container_of(chip, struct tegra_msi, chip);
@@ -219,6 +239,7 @@  struct tegra_pcie {
 	struct clk *afi_clk;
 	struct clk *pcie_xclk;
 	struct clk *pll_e;
+	struct clk *cml_clk;
 
 	struct tegra_msi msi;
 
@@ -228,6 +249,9 @@  struct tegra_pcie {
 
 	struct regulator *pex_clk_supply;
 	struct regulator *vdd_supply;
+	struct regulator *avdd_supply;
+
+	struct tegra_pcie_soc_data *soc_data;
 };
 
 struct tegra_pcie_port {
@@ -384,7 +408,7 @@  static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 		struct tegra_pcie_port *port;
 
 		list_for_each_entry(port, &pcie->ports, list) {
-			if (port->index + 1 == slot) {
+			if (port->index == slot) {
 				addr = port->base + (where & ~3);
 				break;
 			}
@@ -398,7 +422,6 @@  static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 			*value = 0xffffffff;
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
-
 		addr += tegra_pcie_conf_offset(devfn, where);
 	}
 
@@ -429,7 +452,7 @@  static int tegra_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
 		struct tegra_pcie_port *port;
 
 		list_for_each_entry(port, &pcie->ports, list) {
-			if (port->index + 1 == slot) {
+			if (port->index == slot) {
 				addr = port->base + (where & ~3);
 				break;
 			}
@@ -510,12 +533,15 @@  static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
 
 static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
 {
+	struct tegra_pcie_soc_data *soc = port->pcie->soc_data;
 	unsigned long ctrl = tegra_pcie_port_get_pex_ctrl(port);
 	unsigned long value;
 
 	/* enable reference clock */
 	value = afi_readl(port->pcie, ctrl);
 	value |= AFI_PEX_CTRL_REFCLK_EN;
+	if (soc->has_pex_clkreq_en)
+		value |= AFI_PEX_CTRL_CLKREQ_EN;
 	afi_writel(port->pcie, value, ctrl);
 
 	tegra_pcie_port_reset(port);
@@ -568,6 +594,8 @@  static void tegra_pcie_fixup_class(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_NVIDIA, 0x0bf0, tegra_pcie_fixup_class);
 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 */
 static void tegra_pcie_relax_enable(struct pci_dev *dev)
@@ -748,6 +776,11 @@  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 	struct tegra_pcie_port *port;
 	unsigned int timeout;
 	unsigned long value;
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
+
+	/* power down to PCIe slot clock bias pad */
+	if (soc->has_pex_bias_ctrl)
+		afi_writel(pcie, 0, AFI_PEXBIAS_CTRL_0);
 
 	/* configure mode and disable all ports */
 	value = afi_readl(pcie, AFI_PCIE_CONFIG);
@@ -775,26 +808,26 @@  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 	 * Set up PHY PLL inputs select PLLE output as refclock,
 	 * set TX ref sel to div10 (not div5).
 	 */
-	value = pads_readl(pcie, PADS_PLL_CTL);
+	value = pads_readl(pcie, soc->pads_pll_ctl);
 	value &= ~(PADS_PLL_CTL_REFCLK_MASK | PADS_PLL_CTL_TXCLKREF_MASK);
-	value |= (PADS_PLL_CTL_REFCLK_INTERNAL_CML | PADS_PLL_CTL_TXCLKREF_DIV10);
-	pads_writel(pcie, value, PADS_PLL_CTL);
+	value |= PADS_PLL_CTL_REFCLK_INTERNAL_CML | soc->tx_ref_sel;
+	pads_writel(pcie, value, soc->pads_pll_ctl);
 
 	/* take PLL out of reset  */
-	value = pads_readl(pcie, PADS_PLL_CTL);
+	value = pads_readl(pcie, soc->pads_pll_ctl);
 	value |= PADS_PLL_CTL_RST_B4SM;
-	pads_writel(pcie, value, PADS_PLL_CTL);
+	pads_writel(pcie, value, soc->pads_pll_ctl);
 
 	/*
 	 * Hack, set the clock voltage to the DEFAULT provided by hw folks.
 	 * This doesn't exist in the documentation.
 	 */
-	pads_writel(pcie, 0xfa5cfa5c, 0xc8);
+	pads_writel(pcie, 0xfa5cfa5c, PADS_REFCLK_CFG0);
 
 	/* wait for the PLL to lock */
 	timeout = 300;
 	do {
-		value = pads_readl(pcie, PADS_PLL_CTL);
+		value = pads_readl(pcie, soc->pads_pll_ctl);
 		usleep_range(1000, 1000);
 		if (--timeout == 0) {
 			pr_err("Tegra PCIe error: timeout waiting for PLL\n");
@@ -823,6 +856,8 @@  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 	value = AFI_INTR_EN_INI_SLVERR | AFI_INTR_EN_INI_DECERR |
 		AFI_INTR_EN_TGT_SLVERR | AFI_INTR_EN_TGT_DECERR |
 		AFI_INTR_EN_TGT_WRERR | AFI_INTR_EN_DFPCI_DECERR;
+	if (soc->has_intr_prsnt_sense)
+		value |= AFI_INTR_EN_PRSNT_SENSE;
 	afi_writel(pcie, value, AFI_AFI_INTR_ENABLE);
 	afi_writel(pcie, 0xffffffff, AFI_SM_INTR_ENABLE);
 
@@ -837,6 +872,7 @@  static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 
 static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 {
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	int err;
 
 	/* TODO: disable and unprepare clocks? */
@@ -848,19 +884,26 @@  static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 	tegra_pmc_pcie_xclk_clamp(true);
 
+	if (soc->has_avdd_supply) {
+		err = regulator_disable(pcie->avdd_supply);
+		if (err < 0)
+			dev_warn(pcie->dev, "failed to disable AVDD regulator: %d\n",
+			err);
+	}
 	err = regulator_disable(pcie->pex_clk_supply);
 	if (err < 0)
-		dev_err(pcie->dev, "failed to disable pex-clk regulator: %d\n",
+		dev_warn(pcie->dev, "failed to disable pex-clk regulator: %d\n",
 			err);
 
 	err = regulator_disable(pcie->vdd_supply);
 	if (err < 0)
-		dev_err(pcie->dev, "failed to disable VDD regulator: %d\n",
+		dev_warn(pcie->dev, "failed to disable VDD regulator: %d\n",
 			err);
 }
 
 static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 {
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	int err;
 
 	tegra_periph_reset_assert(pcie->pcie_xclk);
@@ -884,6 +927,15 @@  static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 		return err;
 	}
 
+	if (soc->has_avdd_supply) {
+		err = regulator_enable(pcie->avdd_supply);
+		if (err < 0) {
+			dev_err(pcie->dev, "failed to enable AVDD regulator: %d\n",
+				err);
+			return err;
+		}
+	}
+
 	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
 						pcie->pex_clk);
 	if (err) {
@@ -901,6 +953,15 @@  static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 		return err;
 	}
 
+	if (soc->has_cml_clk) {
+		err = clk_prepare_enable(pcie->cml_clk);
+		if (err < 0) {
+			dev_err(pcie->dev, "failed to enable cml clock: %d\n",
+				err);
+			return err;
+		}
+	}
+
 	err = clk_prepare_enable(pcie->pll_e);
 	if (err < 0) {
 		dev_err(pcie->dev, "failed to enable PLLE clock: %d\n", err);
@@ -912,6 +973,8 @@  static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 
 static int tegra_pcie_clocks_get(struct tegra_pcie *pcie)
 {
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
+
 	pcie->pex_clk = devm_clk_get(pcie->dev, "pex");
 	if (IS_ERR(pcie->pex_clk))
 		return PTR_ERR(pcie->pex_clk);
@@ -928,6 +991,11 @@  static int tegra_pcie_clocks_get(struct tegra_pcie *pcie)
 	if (IS_ERR(pcie->pll_e))
 		return PTR_ERR(pcie->pll_e);
 
+	if (soc->has_cml_clk) {
+		pcie->cml_clk = devm_clk_get(pcie->dev, "cml");
+		if (IS_ERR(pcie->cml_clk))
+			return PTR_ERR(pcie->cml_clk);
+	}
 	return 0;
 }
 
@@ -1150,6 +1218,7 @@  static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 {
 	struct platform_device *pdev = to_platform_device(pcie->dev);
 	struct tegra_msi *msi = &pcie->msi;
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	unsigned long base;
 	int err;
 	u32 reg;
@@ -1186,7 +1255,7 @@  static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	msi->pages = __get_free_pages(GFP_KERNEL, 3);
 	base = virt_to_phys((void *)msi->pages);
 
-	afi_writel(pcie, base, AFI_MSI_FPCI_BAR_ST);
+	afi_writel(pcie, base >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, base, AFI_MSI_AXI_BAR_ST);
 	/* this register is in 4K increments */
 	afi_writel(pcie, 1, AFI_MSI_BAR_SZ);
@@ -1283,6 +1352,7 @@  static u32 tegra_pcie_get_xbar_config(struct tegra_pcie *pcie, u32 lanes)
 static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 {
 	struct device_node *np = pcie->dev->of_node, *port;
+	struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	struct of_pci_range_parser parser;
 	struct of_pci_range range;
 	struct resource res;
@@ -1302,6 +1372,12 @@  static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 	if (IS_ERR(pcie->pex_clk_supply))
 		return PTR_ERR(pcie->pex_clk_supply);
 
+	if (soc->has_avdd_supply) {
+		pcie->avdd_supply = devm_regulator_get(pcie->dev, "avdd");
+		if (IS_ERR(pcie->avdd_supply))
+			return PTR_ERR(pcie->avdd_supply);
+	}
+
 	for_each_of_pci_range(&parser, &range) {
 		of_pci_range_to_resource(&range, np, &res);
 
@@ -1348,7 +1424,7 @@  static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 
 		index = PCI_SLOT(err);
 
-		if (index < 1 || index > TEGRA_MAX_PORTS) {
+		if (index < 1 || index > pcie->soc_data->num_max_ports) {
 			dev_err(pcie->dev, "invalid port number: %d\n", index);
 			return -EINVAL;
 		}
@@ -1484,8 +1560,39 @@  static int tegra_pcie_enable(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static const struct tegra_pcie_soc_data tegra20_pcie_data = {
+	.num_max_ports = 2,
+	.msi_base_shift = 0,
+	.pads_pll_ctl = PADS_PLL_CTL_T20,
+	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
+	.has_pex_clkreq_en = false,
+	.has_pex_bias_ctrl = false,
+	.has_intr_prsnt_sense = false,
+	.has_avdd_supply = false,
+	.has_cml_clk = false,
+};
+
+static const struct tegra_pcie_soc_data tegra30_pcie_data = {
+	.num_max_ports = 3,
+	.msi_base_shift = 8,
+	.pads_pll_ctl = PADS_PLL_CTL_T30,
+	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
+	.has_pex_clkreq_en = true,
+	.has_pex_bias_ctrl = true,
+	.has_intr_prsnt_sense = true,
+	.has_avdd_supply = true,
+	.has_cml_clk = true,
+};
+
+static const struct of_device_id tegra_pcie_of_match[] = {
+	{ .compatible = "nvidia,tegra30-pcie", .data = &tegra30_pcie_data},
+	{ .compatible = "nvidia,tegra20-pcie", .data = &tegra20_pcie_data},
+	{ },
+};
+
 static int tegra_pcie_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct tegra_pcie *pcie;
 	int err;
 
@@ -1496,6 +1603,10 @@  static int tegra_pcie_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&pcie->busses);
 	INIT_LIST_HEAD(&pcie->ports);
 	pcie->dev = &pdev->dev;
+	match = of_match_device(tegra_pcie_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+	pcie->soc_data = (struct tegra_pcie_soc_data *)match->data;
 
 	err = tegra_pcie_parse_dt(pcie);
 	if (err < 0)
@@ -1549,10 +1660,9 @@  static int tegra_pcie_remove(struct platform_device *pdev)
 	struct tegra_pcie_bus *bus;
 	int err;
 
-	list_for_each_entry(bus, &pcie->busses, list) {
+	list_for_each_entry(bus, &pcie->busses, list)
 		vunmap(bus->area->addr);
-		kfree(bus);
-	}
+	kfree(bus);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		err = tegra_pcie_disable_msi(pcie);
@@ -1567,11 +1677,6 @@  static int tegra_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id tegra_pcie_of_match[] = {
-	{ .compatible = "nvidia,tegra20-pcie", },
-	{ },
-};
-
 static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",