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).
Signed-off-by: Jay Agarwal <[email protected]>
---
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.
Changes in V3:
- Avoid removing pciex duplicate clock as per review comment
- Corrected tegra pcie driver name for duplicate clocks
drivers/clk/tegra/clk-tegra30.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index c6921f5..edb2b9b 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1598,6 +1598,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", "tegra-pcie");
+ clks[pciex] = clk;
+
/* kfuse */
clk = tegra_clk_register_periph_gate("kfuse", "clk_m",
TEGRA_PERIPH_ON_APB,
@@ -1716,11 +1722,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)
@@ -1942,8 +1943,8 @@ static struct tegra_clk_duplicate tegra_clk_duplicates[] = {
TEGRA_CLK_DUPLICATE(bsea, "tegra-aes", "bsea"),
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(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
- Add interrupt-names property
- Correct downstream I/O size
Signed-off-by: Jay Agarwal <[email protected]>
---
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.
Changes in V3:
- Avoided changes in cml clock as per review comment
arch/arm/boot/dts/tegra30.dtsi | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index a242b2e..a301389 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -21,7 +21,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>;
@@ -29,7 +29,7 @@
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 */
--
1.7.0.4
- Enable PCIe controller on Cardhu
- Only port 2 is connected on this board
- Add regulators required for Tegra30
Signed-off-by: Jay Agarwal <[email protected]>
---
Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.
Changes in V3:
- Added num-lanes property for cardhu as per review comments
arch/arm/boot/dts/tegra30-cardhu.dtsi | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index 8ad4841..b6270e3 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -27,6 +27,24 @@
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@1,0 {
+ nvidia,num-lanes = <4>;
+ };
+ pci@2,0 {
+ nvidia,num-lanes = <1>;
+ };
+ pci@3,0 {
+ status = "okay";
+ nvidia,num-lanes = <1>;
+ };
+ };
+
host1x {
dc@54200000 {
rgb {
--
1.7.0.4
- 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 <[email protected]>
---
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(-)
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 <[email protected]>
@@ -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",
--
1.7.0.4
On 06/04/2013 12:57 PM, 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).
>
> Signed-off-by: Jay Agarwal <[email protected]>
> ---
> Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.
For this change, Mike may as well apply it directly to the clock tree.
Thierry can then pick it up when he rebases his tegra/next tree.
That said, I don't think you should need any of the
TEGRA_CLK_DUPLICATE() entries; the PCIe driver should get its clocks
from device tree now, and hence the driver name in the clock
registration shouldn't be necessary. All of these TEGRA_CLK_DUPLICATE()
entries should be removed en mass sometime soon with luck. So, can you
simply leave the two TEGRA_CLK_DUPLICATE() entries untouched, rather
than changing them?
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().
> > 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.
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
On Wed, Jun 05, 2013 at 12:27:31AM +0530, Jay Agarwal wrote:
[...]
> @@ -29,7 +29,7 @@
> 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 */
That increases the I/O region size from 64 KiB to 1 MiB. Why is that
necessary? I/O operations can only address 64 KiB, so I don't think
adding more makes any sense.
Thierry
> * 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
> On Wed, Jun 05, 2013 at 12:27:31AM +0530, Jay Agarwal wrote:
> [...]
> > @@ -29,7 +29,7 @@
> > 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 */
>
> That increases the I/O region size from 64 KiB to 1 MiB. Why is that
> necessary? I/O operations can only address 64 KiB, so I don't think adding
> more makes any sense.
>
Okay, you can keep it 64KiB then, I did it to match with downstream.
Please let me know if you want me to upload new patch for this or you will take care while integrating.
On Mon, Jun 10, 2013 at 09:55:12PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jun 05, 2013 at 12:27:31AM +0530, Jay Agarwal wrote:
> [...]
> > @@ -29,7 +29,7 @@
> > 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 */
>
> That increases the I/O region size from 64 KiB to 1 MiB. Why is that
> necessary? I/O operations can only address 64 KiB, so I don't think
> adding more makes any sense.
At least PCI allows 32bit I/O addresses. No idea if anyone uses them though.
Cheers,
Peter.
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
> > 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.
Quoting Stephen Warren (2013-06-04 12:08:08)
> On 06/04/2013 12:57 PM, 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).
> >
> > Signed-off-by: Jay Agarwal <[email protected]>
> > ---
> > Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and should be applied on top of this.
>
> For this change, Mike may as well apply it directly to the clock tree.
> Thierry can then pick it up when he rebases his tegra/next tree.
>
> That said, I don't think you should need any of the
> TEGRA_CLK_DUPLICATE() entries; the PCIe driver should get its clocks
> from device tree now, and hence the driver name in the clock
> registration shouldn't be necessary. All of these TEGRA_CLK_DUPLICATE()
> entries should be removed en mass sometime soon with luck. So, can you
> simply leave the two TEGRA_CLK_DUPLICATE() entries untouched, rather
> than changing them?
Ping on this patch. I can take it through my tree, but is there going
to be rework based on Stephen's comments?
Regards,
Mike
> Quoting Stephen Warren (2013-06-04 12:08:08)
> > On 06/04/2013 12:57 PM, 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).
> > >
> > > Signed-off-by: Jay Agarwal <[email protected]>
> > > ---
> > > Patch is based on remotes/gitorious_thierryreding_linux/tegra/next and
> should be applied on top of this.
> >
> > For this change, Mike may as well apply it directly to the clock tree.
> > Thierry can then pick it up when he rebases his tegra/next tree.
> >
> > That said, I don't think you should need any of the
> > TEGRA_CLK_DUPLICATE() entries; the PCIe driver should get its clocks
> > from device tree now, and hence the driver name in the clock
> > registration shouldn't be necessary. All of these
> > TEGRA_CLK_DUPLICATE() entries should be removed en mass sometime
> soon
> > with luck. So, can you simply leave the two TEGRA_CLK_DUPLICATE()
> > entries untouched, rather than changing them?
>
> Ping on this patch. I can take it through my tree, but is there going to be
> rework based on Stephen's comments?
Hi Mike,
I have uploaded next version V4 of this patch after taking care of this comment.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Jun 11, 2013 at 10:30:48AM +0300, Peter De Schrijver wrote:
> On Mon, Jun 10, 2013 at 09:55:12PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Wed, Jun 05, 2013 at 12:27:31AM +0530, Jay Agarwal wrote:
> > [...]
> > > @@ -29,7 +29,7 @@
> > > 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 */
> >
> > That increases the I/O region size from 64 KiB to 1 MiB. Why is that
> > necessary? I/O operations can only address 64 KiB, so I don't think
> > adding more makes any sense.
>
> At least PCI allows 32bit I/O addresses. No idea if anyone uses them though.
I just realized that we are constrained to 64 KiB by the implementation
of pci_ioremap_io(), which assumes each mapping is 64 KiB. Not that it
couldn't be changed, but unless there actually is a use-case where more
than 64 KiB are required I don't think we should worry about it.
Thierry