[PATCH 1/4] ARM: tegra30: clocks: Fix pciex clock registration

[PATCH 1/4] ARM: tegra30: clocks: Fix pciex clock registration

[Jay Agarwal]

Registering pciex as peripheral clock instead of fixed clock
as tegra_perih_reset_assert(deassert) api of this clock api 
gives warning and ultimately does not succeed to assert(deassert).
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
and should be applied on top of this.

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
 drivers/clk/tegra/clk-tegra30.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index ba6f51b..6a80b40 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1592,6 +1592,12 @@ static void __init tegra30_periph_clk_init(void)
 	clk_register_clkdev(clk, "afi", "tegra-pcie");
 	clks[afi] = clk;
 
+	/* pciex */
+	clk = tegra_clk_register_periph_gate("pciex", "pll_e", 0, clk_base, 0,
+				    74, &periph_u_regs, periph_clk_enb_refcnt);
+	clk_register_clkdev(clk, "pciex", NULL);
+	clks[pciex] = clk;
+
 	/* kfuse */
 	clk = tegra_clk_register_periph_gate("kfuse", "clk_m",
 				    TEGRA_PERIPH_ON_APB,
@@ -1710,11 +1716,6 @@ static void __init tegra30_fixed_clk_init(void)
 				1, 0, &cml_lock);
 	clk_register_clkdev(clk, "cml1", NULL);
 	clks[cml1] = clk;
-
-	/* pciex */
-	clk = clk_register_fixed_rate(NULL, "pciex", "pll_e", 0, 100000000);
-	clk_register_clkdev(clk, "pciex", NULL);
-	clks[pciex] = clk;
 }
 
 static void __init tegra30_osc_clk_init(void)
@@ -1930,7 +1931,6 @@ static struct tegra_clk_duplicate tegra_clk_duplicates[] = {
 	TEGRA_CLK_DUPLICATE(bsea, "nvavp", "bsea"),
 	TEGRA_CLK_DUPLICATE(cml1, "tegra_sata_cml", NULL),
 	TEGRA_CLK_DUPLICATE(cml0, "tegra_pcie", "cml"),
-	TEGRA_CLK_DUPLICATE(pciex, "tegra_pcie", "pciex"),
 	TEGRA_CLK_DUPLICATE(vcp, "nvavp", "vcp"),
 	TEGRA_CLK_DUPLICATE(clk_max, NULL, NULL), /* MUST be the last entry */
 };
-- 
1.7.0.4

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

[Jay Agarwal]

- 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
- Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
- and should be applied on top of this.

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
 .../bindings/pci/nvidia,tegra20-pcie.txt           |    1 +
 drivers/pci/host/pci-tegra.c                       |  145 +++++++++++++++++---
 2 files changed, 127 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
index 1ebc526..b4c4e42 100644
--- a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
@@ -91,6 +91,7 @@ Board DTS:
 	pcie-controller {
 		vdd-supply = <&pci_vdd_reg>;
 		pex-clk-supply = <&pci_clk_reg>;
+		avdd-supply = <&ldo2_reg>; /* required for tegra30 */
 		status = "okay";
 
 		/* root port 00:01.0 */
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 24085ed..f7fc650 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -51,7 +51,6 @@
 #include <mach/powergate.h>
 
 #define INT_PCI_MSI_NR (8 * 32)
-#define TEGRA_MAX_PORTS 2
 
 /* register definitions */
 
@@ -143,14 +142,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)
 
@@ -161,8 +161,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)
 
@@ -176,7 +179,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)
@@ -184,8 +188,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);
@@ -196,6 +203,19 @@ struct tegra_msi {
 	int irq;
 };
 
+/* used to differentiate tegra chips code */
+struct tegra_pcie_soc_data {
+	unsigned int num_max_ports;
+	unsigned int pads_pll_ctl;
+	unsigned int tx_ref_sel;
+	unsigned int msi_base_shift;
+	bool pex_clkreq_en;
+	bool pex_bias_ctrl;
+	bool intr_prsnt_sense;
+	bool has_avdd_supply;
+	bool has_cml0_clk;
+};
+
 static inline struct tegra_msi *to_tegra_msi(struct msi_chip *chip)
 {
 	return container_of(chip, struct tegra_msi, chip);
@@ -220,6 +240,7 @@ struct tegra_pcie {
 	struct clk *afi_clk;
 	struct clk *pcie_xclk;
 	struct clk *pll_e;
+	struct clk *cml0_clk;
 
 	struct tegra_msi msi;
 
@@ -229,6 +250,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 {
@@ -511,12 +535,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->pex_clkreq_en)
+		value |= AFI_PEX_CTRL_CLKREQ_EN;
 	afi_writel(port->pcie, value, ctrl);
 
 	tegra_pcie_port_reset(port);
@@ -569,6 +596,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)
@@ -749,6 +778,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->pex_bias_ctrl)
+		afi_writel(pcie, 0, AFI_PEXBIAS_CTRL_0);
 
 	/* configure mode and disable all ports */
 	value = afi_readl(pcie, AFI_PCIE_CONFIG);
@@ -776,26 +810,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");
@@ -824,6 +858,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->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);
 
@@ -838,6 +874,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? */
@@ -849,19 +886,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);
@@ -885,6 +929,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) {
@@ -902,6 +955,15 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 		return err;
 	}
 
+	if (soc->has_cml0_clk) {
+		err = clk_prepare_enable(pcie->cml0_clk);
+		if (err < 0) {
+			dev_err(pcie->dev, "failed to enable cml0 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);
@@ -913,6 +975,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);
@@ -929,6 +993,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_cml0_clk) {
+		pcie->cml0_clk = devm_clk_get(pcie->dev, "cml0");
+		if (IS_ERR(pcie->cml0_clk))
+			return PTR_ERR(pcie->cml0_clk);
+	}
 	return 0;
 }
 
@@ -1151,6 +1220,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;
@@ -1187,7 +1257,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);
@@ -1284,6 +1354,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;
 	const __be32 *range = NULL;
 	struct resource res;
 	u32 lanes = 0;
@@ -1297,6 +1368,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);
+	}
+
 	while ((range = of_pci_process_ranges(np, &res, range)) != NULL) {
 		switch (res.flags & IORESOURCE_TYPE_BITS) {
 		case IORESOURCE_IO:
@@ -1341,7 +1418,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;
 		}
@@ -1477,8 +1554,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,
+	.pads_pll_ctl = PADS_PLL_CTL_T20,
+	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10,
+	.msi_base_shift = 0,
+	.pex_clkreq_en = false,
+	.pex_bias_ctrl = false,
+	.intr_prsnt_sense = false,
+	.has_avdd_supply = false,
+	.has_cml0_clk = false,
+};
+
+static const struct tegra_pcie_soc_data tegra30_pcie_data = {
+	.num_max_ports = 3,
+	.pads_pll_ctl = PADS_PLL_CTL_T30,
+	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
+	.msi_base_shift = 8,
+	.pex_clkreq_en = true,
+	.pex_bias_ctrl = true,
+	.intr_prsnt_sense = true,
+	.has_avdd_supply = true,
+	.has_cml0_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;
 
@@ -1489,6 +1597,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)
@@ -1560,11 +1672,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",
-- 
1.7.0.4

Re: [PATCH 2/4] ARM: tegra: pcie: Add tegra3 support

[Stephen Warren]

On 05/08/2013 04:57 AM, 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

> - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> - and should be applied on top of this.

Again, those two lines would go below the ---. And since that's all one
bullet point, why is the second line indented with a "-"?

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

>  	pcie-controller {
>  		vdd-supply = <&pci_vdd_reg>;
>  		pex-clk-supply = <&pci_clk_reg>;
> +		avdd-supply = <&ldo2_reg>; /* required for tegra30 */

I would simply drop that comment, and perhaps even the whole line; this
is just an example after all, and doesn't need to cover the latest SoC.
Equally, the example SoC DTSI section is an example for Tegra20, so
making the example board DTS section contain a Tegra30 example would be
inconsistent. Tegra30 should be capitalized; it's a name.

Why is there no change to the "Required Properties" section of this
document? It should list the set of clocks and regulators that are
required. That section is the definitive reference, whereas the example
is just than; an example.

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

> +	unsigned int pads_pll_ctl;
> +	unsigned int tx_ref_sel;

Perhaps those 2 values should be u32 to match the readl/writel
parameters? They're HW register values after all.

> @@ -220,6 +240,7 @@ struct tegra_pcie {
>  	struct clk *afi_clk;
>  	struct clk *pcie_xclk;
>  	struct clk *pll_e;
> +	struct clk *cml0_clk;

I think this clock should be called "cml" not "cml0". The clocks within
the driver and DT binding should be named from the perspective of the
PCI HW unit, and not from the perspective of the system that surrounds
it and provides those clocks.

The only exception would be if some future Tegra version might end up
with multiple cml clocks to a single PCIe unit, and we need to number
them to identify them. However, that seems quite unlikely since the PCIe
unit already supports multiple ports, and multiple port support would be
about the only reason that I could think of to have multiple clock
instances.

I think I mentioned this when I reviewed the previous version of the
patch, but you didn't respond to the suggestion.

> @@ -749,6 +778,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->pex_bias_ctrl)
> +		afi_writel(pcie, 0, AFI_PEXBIAS_CTRL_0);

Renaming pex_bias_ctrl to has_pex_bias_ctrl might make it more obvious
what that variable means.

A similar comment might apply to soc->pex_clkreq_en and
soc->intr_prsnt_sense.

[PATCH 3/4] ARM: dts: tegra: Correct PCIe entry

[Jay Agarwal]

- Add interrupt-names property
- Correct downstream I/O size
- Correct cml clock name for Tegra30
- Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
- and should be applied on top of this.

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
 arch/arm/boot/dts/tegra30.dtsi |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 5a270ff..289ef93 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -124,7 +124,7 @@
 		reg-names = "pads", "afi", "cs";
 		interrupts = <0 98 0x04   /* controller interrupt */
 		              0 99 0x04>; /* MSI interrupt */
-
+		interrupt-names = "intr", "msi";
 		bus-range = <0x00 0xff>;
 		#address-cells = <3>;
 		#size-cells = <2>;
@@ -132,13 +132,13 @@
 		ranges = <0x82000000 0 0x00000000 0x00000000 0 0x00001000   /* port 0 configuration space */
 			  0x82000000 0 0x00001000 0x00001000 0 0x00001000   /* port 1 configuration space */
 			  0x82000000 0 0x00004000 0x00004000 0 0x00001000   /* port 2 configuration space */
-			  0x81000000 0 0          0x02000000 0 0x00010000   /* downstream I/O */
+			  0x81000000 0 0          0x02000000 0 0x00100000   /* downstream I/O */
 			  0x82000000 0 0x20000000 0x20000000 0 0x10000000   /* non-prefetchable memory */
 			  0xc2000000 0 0x30000000 0x30000000 0 0x10000000>; /* prefetchable memory */
 
 		clocks = <&tegra_car 70>, <&tegra_car 72>, <&tegra_car 74>,
 			 <&tegra_car 118>, <&tegra_car 215>;
-		clock-names = "pex", "afi", "pcie_xclk", "pll_e", "cml";
+		clock-names = "pex", "afi", "pcie_xclk", "pll_e", "cml0";
 		status = "disabled";
 
 		pci@1,0 {
-- 
1.7.0.4

Re: [PATCH 3/4] ARM: dts: tegra: Correct PCIe entry

[Stephen Warren]

On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> - Add interrupt-names property
> - Correct downstream I/O size
> - Correct cml clock name for Tegra30
> - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> - and should be applied on top of this.

Another change that needs to be made to this file (probably as a
separate change that Thierry can squash into one of his earlier changes)
is to move the pcie-controller node; it is currently not in the correct
place in the .dtsi file; it's not sorted by reg addresss.

> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi

>  		clocks = <&tegra_car 70>, <&tegra_car 72>, <&tegra_car 74>,
>  			 <&tegra_car 118>, <&tegra_car 215>;
> -		clock-names = "pex", "afi", "pcie_xclk", "pll_e", "cml";
> +		clock-names = "pex", "afi", "pcie_xclk", "pll_e", "cml0";

You could drop that change if the driver named the clock "cml" rather
than "cml0", which as I explained in my previous email seems like a good
idea anyway.

Applying the same reasoning, I wonder if for Tegra 20 too, the PCIe
driver shouldn't expect clock names of just "xclk" and "pll" rather than
"pcie_xclk" and "pll_e".

[PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

- Enable PCIe controller on Cardhu
- Only port 2 is connected on this board
- Add regulators required for Tegra30
- Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
- and should be applied on top of this.

Signed-off-by: Jay Agarwal <jagarwal@nvidia.com>
---
 arch/arm/boot/dts/tegra30-cardhu.dtsi |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index d2fdf95..8f8e07b 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -27,6 +27,17 @@
 	model = "NVIDIA Tegra30 Cardhu evaluation board";
 	compatible = "nvidia,cardhu", "nvidia,tegra30";
 
+	pcie-controller {
+		status = "okay";
+		pex-clk-supply = <&pex_hvdd_3v3_reg>;
+		vdd-supply = <&ldo1_reg>;
+		avdd-supply = <&ldo2_reg>;
+
+		pci@3,0 {
+			status = "okay";
+		};
+	};
+
 	host1x {
 		dc@54200000 {
 			rgb {
-- 
1.7.0.4

Re: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Stephen Warren]

On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> - Enable PCIe controller on Cardhu
> - Only port 2 is connected on this board
> - Add regulators required for Tegra30
> - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> - and should be applied on top of this.

> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi

> +	pcie-controller {
> +		status = "okay";
> +		pex-clk-supply = <&pex_hvdd_3v3_reg>;
> +		vdd-supply = <&ldo1_reg>;
> +		avdd-supply = <&ldo2_reg>;
> +
> +		pci@3,0 {
> +			status = "okay";
> +		};
> +	};

So, if I apply this series, I do see the PCIe bridge and Ethernet device
get enumerated, but I don't see the USB3 controller get enumerated. I
believe that is a PCIe device behind the same bridge on the same Tegra
PCIe port. Shouldn't this device show up?

The Ethernet interface gets an IP address by DHCP, so some amount of
data must be flowing. However, I cannot ping anything. If I run the same
kernel on a Tegra20 TrimSlice board, which has the same Ethernet chip,
then everything works as expected. Have you fully tested network
connectivity with these patches applied? Perhaps this is related to the
next problem:

According to the Cardhu schematics, the PCIe link to the dock is a
single lane. Hence, I believe that the Cardhu DT should describe a 411
port configuration. However, the Cardhu DT doesn't describe any
particular link configuration, but just inherits the default from
tegra30.dtsi, which describes a 222 link configuration. I would have
expected the following in the Cardhu DT:

		pci@1,0 {
			nvidia,num-lanes = <4>;
		};

		pci@2,0 {
			nvidia,num-lanes = <1>;
		};

		pci@3,0 {
			status = "okay";
			nvidia,num-lanes = <1>;
		};

However, if I put that there, no PCIe links are detected at all. Why
does the driver work with the wrong link configuration, but fail with
the correct one?

Related, I notice that in Thierry's patches which you're building on,
tegra_pcie_get_xbar_config() still has a bug where a zero return value
is treated as an error, even though it's a valid HW register value.
Perhaps this is related?

Re: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Stephen Warren]

On 05/08/2013 11:04 AM, Stephen Warren wrote:
> On 05/08/2013 04:57 AM, Jay Agarwal wrote:
>> - Enable PCIe controller on Cardhu
>> - Only port 2 is connected on this board
>> - Add regulators required for Tegra30
>> - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
>> - and should be applied on top of this.
> 
>> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> 
>> +	pcie-controller {
>> +		status = "okay";
>> +		pex-clk-supply = <&pex_hvdd_3v3_reg>;
>> +		vdd-supply = <&ldo1_reg>;
>> +		avdd-supply = <&ldo2_reg>;
>> +
>> +		pci@3,0 {
>> +			status = "okay";
>> +		};
>> +	};
...
> According to the Cardhu schematics, the PCIe link to the dock is a
> single lane. Hence, I believe that the Cardhu DT should describe a 411
> port configuration. However, the Cardhu DT doesn't describe any
> particular link configuration, but just inherits the default from
> tegra30.dtsi, which describes a 222 link configuration. I would have
> expected the following in the Cardhu DT:
> 
> 		pci@1,0 {
> 			nvidia,num-lanes = <4>;
> 		};
> 
> 		pci@2,0 {
> 			nvidia,num-lanes = <1>;
> 		};
> 
> 		pci@3,0 {
> 			status = "okay";
> 			nvidia,num-lanes = <1>;
> 		};
> 
> However, if I put that there, no PCIe links are detected at all. Why
> does the driver work with the wrong link configuration, but fail with
> the correct one?

I take this back. Fixing the DT as shown above to represent the correct
4/1/1 configuration does still yield a working system. Please
incorporate this into a future patch revision.

The issue is more that PCIe enumeration is only reliable after a cold
power cycle of the dock (the dock appears to be powered solely by its
power cable and never the battery in Cardhu, and hence isn't affected by
the main tablet PMIC's power on/off state like most of the board is). Is
there some reset signal to the dock that the bootloader or kernel should
be driving to solve this?

RE: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

> On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> > - Enable PCIe controller on Cardhu
> > - Only port 2 is connected on this board
> > - Add regulators required for Tegra30
> > - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> > - and should be applied on top of this.
> 
> > diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> > b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> 
> > +	pcie-controller {
> > +		status = "okay";
> > +		pex-clk-supply = <&pex_hvdd_3v3_reg>;
> > +		vdd-supply = <&ldo1_reg>;
> > +		avdd-supply = <&ldo2_reg>;
> > +
> > +		pci@3,0 {
> > +			status = "okay";
> > +		};
> > +	};
> 
> So, if I apply this series, I do see the PCIe bridge and Ethernet device get
> enumerated, but I don't see the USB3 controller get enumerated. I believe
> that is a PCIe device behind the same bridge on the same Tegra PCIe port.
> Shouldn't this device show up?
[>]  I have also reproduced this problem. I see somehow no non-prefetchable memory is assigned to any of pcie devices.
Probably that is the reason for USB3 (pci 0000:04:00.0) not getting enumerated since it uses only non-prefetchable memory.

RE: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

> > On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> > > - Enable PCIe controller on Cardhu
> > > - Only port 2 is connected on this board
> > > - Add regulators required for Tegra30
> > > - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> > > - and should be applied on top of this.
> >
> > > diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> > > b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> >
> > > +	pcie-controller {
> > > +		status = "okay";
> > > +		pex-clk-supply = <&pex_hvdd_3v3_reg>;
> > > +		vdd-supply = <&ldo1_reg>;
> > > +		avdd-supply = <&ldo2_reg>;
> > > +
> > > +		pci@3,0 {
> > > +			status = "okay";
> > > +		};
> > > +	};
> >
> > So, if I apply this series, I do see the PCIe bridge and Ethernet
> > device get enumerated, but I don't see the USB3 controller get
> > enumerated. I believe that is a PCIe device behind the same bridge on the
> same Tegra PCIe port.
> > Shouldn't this device show up?
> I have also reproduced this problem. I see somehow no non-
> prefetchable memory is assigned to any of pcie devices.
> Probably that is the reason for USB3 (pci 0000:04:00.0) not getting
> enumerated since it uses only non-prefetchable memory.

1. Bus4(on which usb3 device resides) always return 0xffffffff from it's config space. which means device is not present?
2. That's why it is not assigned any resources and hence no usb3 probe happens.
3. But same bus does return valid info like vendor/device id etc from it's config space in downstream kernel and hence usb3 probe does happen.

Thierry, Stephen,
4. Any idea why bus4 should return 0xffffffff values in upstream kernel? Anything missing?
5. Also, how config space of all pcie devices are mapped? I mean if I change the config space offset in dts file, then also I find correct vendor/device id etc for bus0/device0/fun0.
    So how this mapping happens that even after changing the config space offset in PCIe address space, it always finds correct vendor/device id.

Re: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Stephen Warren]

On 05/17/2013 10:51 AM, Jay Agarwal wrote:
>>> On 05/08/2013 04:57 AM, Jay Agarwal wrote:
>>>> - Enable PCIe controller on Cardhu
>>>> - Only port 2 is connected on this board
>>>> - Add regulators required for Tegra30
>>>> - Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
>>>> - and should be applied on top of this.
...
>>> So, if I apply this series, I do see the PCIe bridge and Ethernet
>>> device get enumerated, but I don't see the USB3 controller get
>>> enumerated. I believe that is a PCIe device behind the same bridge on the
>>> same Tegra PCIe port.
>>> Shouldn't this device show up?
>>
>> I have also reproduced this problem. I see somehow no non-
>> prefetchable memory is assigned to any of pcie devices.
>> Probably that is the reason for USB3 (pci 0000:04:00.0) not getting
>> enumerated since it uses only non-prefetchable memory.
> 
> 1. Bus4(on which usb3 device resides) always return 0xffffffff from it's config space. which means device is not present?
> 2. That's why it is not assigned any resources and hence no usb3 probe happens.
> 3. But same bus does return valid info like vendor/device id etc from it's config space in downstream kernel and hence usb3 probe does happen.
> 
> Thierry, Stephen,
> 4. Any idea why bus4 should return 0xffffffff values in upstream kernel? Anything missing?

Is there some reset/enable GPIO or regulator that needs to be programmed
to enable the PCIe USB3 controller? Take a look at the schematic. If you
can make it work by tweaking those GPIOs/... manually, then we can
ignore this issue and fix it up later, since it's not directly related
to the PCIe controller driver patches. It's more important to get this
Ethernet working than USB, I think.

> 5. Also, how config space of all pcie devices are mapped? I mean if I change the config space offset in dts file, then also I find correct vendor/device id etc for bus0/device0/fun0.
>     So how this mapping happens that even after changing the config space offset in PCIe address space, it always finds correct vendor/device id.

Can you please word-wrap your email less than 80 columns? It's quite
hard to read.

I don't quite understand your question. The PCIe driver implements the
functions that access config space. I don't recall that being influenced
by anything much in the device tree, except for the 3rd entry in the
top-level PCIe controller's reg property:

        pcie-controller {
                compatible = "nvidia,tegra30-pcie";
                device_type = "pci";
                reg = <0x00003000 0x00000800   /* PADS registers */
                       0x00003800 0x00000200   /* AFI registers */
                       0x10000000 0x10000000>; /* configuration space */

RE: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

> > > So, if I apply this series, I do see the PCIe bridge and Ethernet
> > > device get enumerated, but I don't see the USB3 controller get
> > > enumerated. I believe that is a PCIe device behind the same bridge
> > > on the
> > same Tegra PCIe port.
> > > Shouldn't this device show up?
> > I have also reproduced this problem. I see somehow no non-
> > prefetchable memory is assigned to any of pcie devices.
> > Probably that is the reason for USB3 (pci 0000:04:00.0) not getting
> > enumerated since it uses only non-prefetchable memory.
> 
> 1. Bus4(on which usb3 device resides) always return 0xffffffff from it's
> config space. which means device is not present?
> 2. That's why it is not assigned any resources and hence no usb3 probe
> happens.
> 3. But same bus does return valid info like vendor/device id etc from it's
> config space in downstream kernel and hence usb3 probe does happen.
> 
> Thierry, Stephen,
> 4. Any idea why bus4 should return 0xffffffff values in upstream kernel?
> Anything missing?
> 5. Also, how config space of all pcie devices are mapped? I mean if I change
> the config space offset in dts file, then also I find correct vendor/device id
> etc for bus0/device0/fun0.
>     So how this mapping happens that even after changing the config space
> offset in PCIe address space, it always finds correct vendor/device id.

 Any idea on this?

Re: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Stephen Warren]

On 05/29/2013 04:10 AM, Jay Agarwal wrote:
>>>> So, if I apply this series, I do see the PCIe bridge and Ethernet
>>>> device get enumerated, but I don't see the USB3 controller get
>>>> enumerated. I believe that is a PCIe device behind the same bridge
>>>> on the
>>> same Tegra PCIe port.
>>>> Shouldn't this device show up?
>>> I have also reproduced this problem. I see somehow no non-
>>> prefetchable memory is assigned to any of pcie devices.
>>> Probably that is the reason for USB3 (pci 0000:04:00.0) not getting
>>> enumerated since it uses only non-prefetchable memory.
>>
>> 1. Bus4(on which usb3 device resides) always return 0xffffffff from it's
>> config space. which means device is not present?
>> 2. That's why it is not assigned any resources and hence no usb3 probe
>> happens.
>> 3. But same bus does return valid info like vendor/device id etc from it's
>> config space in downstream kernel and hence usb3 probe does happen.
>>
>> Thierry, Stephen,
>> 4. Any idea why bus4 should return 0xffffffff values in upstream kernel?
>> Anything missing?
>> 5. Also, how config space of all pcie devices are mapped? I mean if I change
>> the config space offset in dts file, then also I find correct vendor/device id
>> etc for bus0/device0/fun0.
>>     So how this mapping happens that even after changing the config space
>> offset in PCIe address space, it always finds correct vendor/device id.
> 
>  Any idea on this?

I did already reply the same day you sent the original email. My
response was:

Is there some reset/enable GPIO or regulator that needs to be programmed
to enable the PCIe USB3 controller? Take a look at the schematic. If you
can make it work by tweaking those GPIOs/... manually, then we can
ignore this issue and fix it up later, since it's not directly related
to the PCIe controller driver patches. It's more important to get the
Ethernet working than USB, I think.

To be honest though, I would expect you to be asking around inside
NVIDIA to determine the answer here. As the PCIe SW expert, I'd expect
you to drive this process. Try asking the Cardhu board and PCIe HW
experts within NVIDIA.

Did you make any progress on the issues with the Ethernet device?

RE: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

> On 05/29/2013 04:10 AM, Jay Agarwal wrote:
> >>>> So, if I apply this series, I do see the PCIe bridge and Ethernet
> >>>> device get enumerated, but I don't see the USB3 controller get
> >>>> enumerated. I believe that is a PCIe device behind the same bridge
> >>>> on the
> >>> same Tegra PCIe port.
> >>>> Shouldn't this device show up?
> >>> I have also reproduced this problem. I see somehow no non-
> >>> prefetchable memory is assigned to any of pcie devices.
> >>> Probably that is the reason for USB3 (pci 0000:04:00.0) not getting
> >>> enumerated since it uses only non-prefetchable memory.
> >>
> >> 1. Bus4(on which usb3 device resides) always return 0xffffffff from
> >> it's config space. which means device is not present?
> >> 2. That's why it is not assigned any resources and hence no usb3
> >> probe happens.
> >> 3. But same bus does return valid info like vendor/device id etc from
> >> it's config space in downstream kernel and hence usb3 probe does
> happen.
> >>
> >> Thierry, Stephen,
> >> 4. Any idea why bus4 should return 0xffffffff values in upstream kernel?
> >> Anything missing?
> >> 5. Also, how config space of all pcie devices are mapped? I mean if I
> >> change the config space offset in dts file, then also I find correct
> >> vendor/device id etc for bus0/device0/fun0.
> >>     So how this mapping happens that even after changing the config
> >> space offset in PCIe address space, it always finds correct vendor/device
> id.
> >
> >  Any idea on this?
> 
> I did already reply the same day you sent the original email. My response
> was:
> 
> Is there some reset/enable GPIO or regulator that needs to be programmed
> to enable the PCIe USB3 controller? Take a look at the schematic. If you can
> make it work by tweaking those GPIOs/... manually, then we can ignore this
> issue and fix it up later, since it's not directly related to the PCIe controller
> driver patches. It's more important to get the Ethernet working than USB, I
> think.
> 
> To be honest though, I would expect you to be asking around inside NVIDIA
> to determine the answer here. As the PCIe SW expert, I'd expect you to
> drive this process. Try asking the Cardhu board and PCIe HW experts within
> NVIDIA.
> 
> Did you make any progress on the issues with the Ethernet device?

Stephen, 
I have taken care of all your comments, but Ethernet device is not working for me neither on cardhu nor harmony. 
Could be related to my process or board, Currently debugging this.
Parallely, should I push my patches for review so that it is clear from other aspects?
 

Re: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Stephen Warren]

On 05/30/2013 11:37 AM, Jay Agarwal wrote:
...
> I have taken care of all your comments, but Ethernet device is not working for me neither on cardhu nor harmony. 
> Could be related to my process or board, Currently debugging this.

Are you talking about a PCIe-based Ethernet device on Harmony, or the
one that's built into the board?

The on-board Ethernet device is USB, and should work fine already, and
irrespective of any PCIe patches.

I have only tried an XHCI USB controller in the PCIe slot on Harmony, so
I can't say if Ethernet not working is a regression or not. Does the
Ethernet device work with just Thierry's patches and not yours?

> Parallely, should I push my patches for review so that it is clear from other aspects?

Well, if the patches don't work, I suppose there's not much use posting
them since they'll just need changes before they can be usefully applied
anyway. IIRC, most of my comments last time were fairly minor, so there
isn't much need for a separate review of them, right?

FW: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

[Jay Agarwal]

<Forwarding to bigger aliases>

Anybody aware of hang in r8169 driver over pcie interface mentioned at bottom of this mail?

-----Original Message-----
From: Jay Agarwal 
Sent: Tuesday, June 04, 2013 7:17 PM
To: 'Stephen Warren'
Cc: 'thierry.reding@avionic-design.de'
Subject: RE: [PATCH 4/4] ARM: tegra: pcie: Enable PCIe controller on Cardhu

> > > > > On 05/31/2013 06:17 AM, Jay Agarwal wrote:
> > > > > >>> I have taken care of all your comments, but Ethernet 
> > > > > >>> device is not working
> > > > > >> for me neither on cardhu nor harmony.
> > > > > >>> Could be related to my process or board, Currently 
> > > > > >>> debugging
> this.
> > > > > >>
> > > > > >> Are you talking about a PCIe-based Ethernet device on 
> > > > > >> Harmony, or the one that's built into the board?
> > > > > >>
> > > > > >> The on-board Ethernet device is USB, and should work fine 
> > > > > >> already, and irrespective of any PCIe patches.
> > > > > >
> > > > > > No I am talking about ethernet over PCIe interface.  There 
> > > > > > is no problem
> > > > > with built-in ethernet over USB.
> > > > > >
> > > > > >> I have only tried an XHCI USB controller in the PCIe slot 
> > > > > >> on Harmony, so I can't say if Ethernet not working is a 
> > > > > >> regression or not. Does the Ethernet device work with just 
> > > > > >> Thierry's patches and not
> > > > yours?
> > > > > >
> > > > > > It does not work w/o any of my patches also.
> > > > > >
> > > > > > Thierry,
> > > > > > Have you verified any ethernet card working on harmony?
> > > > > > I am using Broadcom Ethernet card with TIGON3 driver, but it 
> > > > > > does not
> > > > > work although PCIe enumeration and driver probe looks fine as
> > > attached.
> > > > > > Any idea?
> > > > >
> > > > > Oh, one thing to consider here: PCIe interrupts don't work 
> > > > > correctly when
> > > > > LP2 is enabled, at least in the upstream kernel. It's 
> > > > > something I've been trying to investigate, but haven't tracked down yet.
> > > > >
> > > > > Anyway, you need to disable the LP2 CPU power management state 
> > > > > for sure.
> > > > > Hopefully this will solve the issue for you. One way to do 
> > > > > this is the following patch, although we need a better 
> > > > > solution before we can actually merge anything:
> > > > >
> > > > > https://patchwork.kernel.org/patch/2525151/
> > > > Stephen,
> > > > This patch also did not resolve the issue although I do get 
> > > > "Disabling
> > > > LP2 cpuidle state..."  message in logs.
> > >
> > > It stucks as below:
> > >
> > > root@localhost:~# dhclient eth0
> > > [ 1444.579544] smsc95xx 3-1.1:1.0 eth0: hardware isn't capable of 
> > > remote wakeup [ 1444.587300] IPv6: ADDRCONF(NETDEV_UP): eth0: link 
> > > is not ready
> >
> > My system does not have ping command, so dhclient fails like below:
> > /sbin/dhclient-script: line 325: ping: command not found
> >
> > So I provided static IP as : ifconfig eth0 <IP> but it also did not help.
> 
>  Actually sorry, I was using wrong Ethernet interface(eth0) on 
> harmony, but correct one is eth1(over pcie interface) which works fine 
> on Thierry's repo after applying patches to disable LP2.
> Now I will try after applying my patches on both harmony and cardhu.

Ethernet is working on harmony is working w/ & a/o my patches but cardhu is hanging as below:

root@localhost:~# ifconfig eth0 10.19.65.115 [  102.525245] r8169 0000:03:00.0 eth0: unable to load firmware patch rtl_nic/rtl8168e-3.fw (-2) [  102.951516] r8169 0000:03:00.0 eth0: rtl_phyar_cond == 1 (loop: 20, delay: 25).
<System hangs here..>

1. Harmony is using Broadcom pcie Ethernet card and cardhu is using realtek one.
2. Enumeration and lspci seems to be fine though.
3. I tried applying similar patches for disabling LP2 for tegra 3 as well but it did not help.
4. Any idea/opinion?

Re: [PATCH 1/4] ARM: tegra30: clocks: Fix pciex clock registration

[Stephen Warren]

On 05/08/2013 04:57 AM, Jay Agarwal wrote:
> Registering pciex as peripheral clock instead of fixed clock
> as tegra_perih_reset_assert(deassert) api of this clock api 
> gives warning and ultimately does not succeed to assert(deassert).

A few procedural comments: I believe this is at least the second version
of these patches that have been posted. As such, the email subject
should say e.e. "PATCH V2" not just "PATCH". You can pass
--subject-prefix='PATCH V2' to git format-patch to achieve this.

Since this is an updated version of the patches, each patch should
describe what changed in the patch, so that people know what issues
you've fixed in this version. Most people believe that the description
of changes should be below the --- line, so that it doesn't get included
in the commit description when the patch is applied.

> Patch is based on remotes/gitorious_thierryreding_linux/tegra/next
> and should be applied on top of this.

Those two lines should be below the --- line so that they don't get
included in the commit description when the patch is applied. git
metadata will provide clues re: who applied the patch and to which
branch, so there's no need to duplicate this information in the commit
description itself, but rather include it below --- so that it's still
present and people can see it.

Some comments on the patch and series below...

> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c

> +	/* pciex */
> +	clk = tegra_clk_register_periph_gate("pciex", "pll_e", 0, clk_base, 0,
> +				    74, &periph_u_regs, periph_clk_enb_refcnt);
> +	clk_register_clkdev(clk, "pciex", NULL);
> +	clks[pciex] = clk;

Hmmm. This change perhaps isn't technically correct, but is the best we
can do for now, so it's fine. It's also consistent with how the Tegra20
clock driver is written.

This clock has a "reset" bit in the CLK_RST_* registers, but not an
"enable" bit in the CLK_ENB_* registers. Hence, representing this as a
gated clock isn't correct, since there is not gate control in HW. The
correct way to handle this would be to implement the new reset
controller API for Tegra, and expose the reset control only, and not
touch the clock registration at all. However, we do this exact same
thing for a number of Tegra clocks right now, hence I think this change
is fine for now; we'll clean this up, along with some other instances of
the same issue, once the reset API is implemented for Tegra. Of course,
if that happens before the PCI code is checked in, then my opinion will
change.

> @@ -1930,7 +1931,6 @@ static struct tegra_clk_duplicate tegra_clk_duplicates[] = {
>  	TEGRA_CLK_DUPLICATE(bsea, "nvavp", "bsea"),
>  	TEGRA_CLK_DUPLICATE(cml1, "tegra_sata_cml", NULL),
>  	TEGRA_CLK_DUPLICATE(cml0, "tegra_pcie", "cml"),
> -	TEGRA_CLK_DUPLICATE(pciex, "tegra_pcie", "pciex"),

That seems like an unrelated change, and isn't mentioned in the commit
description. Is the change strictly necessary? Is it cleanup that can
happen as a separate patch later in the series? Aren't you able to
remove the CLK_DUPLICATE() entries for other PCIe-related clocks too?