[PATCH 00/17] PCIe support for DRA7xx

[PATCH 00/17] PCIe support for DRA7xx

[Kishon Vijay Abraham I]

This patch series adds support for PCIe in DRA7xx including drivers and dt
data. PCIe in DRA7xx uses desingware IP and hence this re-uses the
pcie desingware driver (pcie-designware.c) by Jingoo.

The last couple of patches are marked as *TEMP* since the TI reset driver [1]
is not yet merged and is in RFC.

Tested broadcom PCIe card and XIO2000 bridge along with DGE530T ethernet
card.

Changes from RFC:
* Added external clock support for PCIE APLL
* Miscellaneous cleanups in Documentation, macro naming etc..

[1] -> http://www.spinics.net/lists/linux-omap/msg106411.html

Keerthy (2):
  ARM: dts: DRA7: Add divider table to optfclk_pciephy_div clock
  ARM: dts: DRA7: Change the parent of apll_pcie_in_clk_mux to
    dpll_pcie_ref_m2ldo_ck

Kishon Vijay Abraham I (15):
  phy: phy-omap-pipe3: Add support for PCIe PHY
  phy: omap-control: add external clock support for PCIe PHY
  phy: ti-pipe3: add external clock support for PCIe PHY
  phy: pipe3: insert delay to enumerate in GEN2 mode
  pci: host: pcie-dra7xx: add support for pcie-dra7xx controller
  pci: host: pcie-designware: Use *base-mask* for configuring the iATU
  arm: dra7xx: Add hwmod data for pcie1 phy and pcie2 phy
  arm: dra7xx: Add hwmod data for pcie1 and pcie2 subsystems
  ARM: dts: dra7xx-clocks: Add missing 32khz clocks used for PHY
  ARM: dts: dra7: Add dt data for PCIe PHY control module
  ARM: dts: dra7: Add dt data for PCIe PHY
  ARM: dts: dra7: Add dt data for PCIe controller
  ARM: OMAP: Enable PCI for DRA7
  pci: host: pcie-dra7xx: use reset framework APIs to reset PCIe
  ARM: dts: dra7: Add *resets* property for PCIe dt node

 .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
 Documentation/devicetree/bindings/pci/ti-pci.txt   |   35 ++
 Documentation/devicetree/bindings/phy/ti-phy.txt   |   21 +-
 arch/arm/boot/dts/dra7.dtsi                        |   55 +++
 arch/arm/boot/dts/dra7xx-clocks.dtsi               |   11 +-
 arch/arm/mach-omap2/Kconfig                        |    2 +
 arch/arm/mach-omap2/cm2_7xx.h                      |    4 +
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c          |  112 ++++++
 arch/arm/mach-omap2/prm7xx.h                       |    4 +
 drivers/pci/host/Kconfig                           |   10 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pci-dra7xx.c                      |  395 ++++++++++++++++++++
 drivers/pci/host/pcie-designware.c                 |   39 +-
 drivers/pci/host/pcie-designware.h                 |    1 +
 drivers/phy/phy-omap-control.c                     |   92 ++++-
 drivers/phy/phy-ti-pipe3.c                         |  142 +++++--
 include/linux/phy/omap_control_phy.h               |   23 ++
 17 files changed, 898 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/ti-pci.txt
 create mode 100644 drivers/pci/host/pci-dra7xx.c

-- 
1.7.9.5

[PATCH 01/17] phy: phy-omap-pipe3: Add support for PCIe PHY

[Kishon Vijay Abraham I]

PCIe PHY uses an external pll instead of the internal pll used by SATA
and USB3. So added support in pipe3 PHY to use external pll.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/phy/ti-phy.txt |    8 +-
 drivers/phy/phy-ti-pipe3.c                       |   99 +++++++++++++++++-----
 2 files changed, 84 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
index 9ce458f..cf3de7e 100644
--- a/Documentation/devicetree/bindings/phy/ti-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -56,8 +56,8 @@ usb2phy@4a0ad080 {
 TI PIPE3 PHY
 
 Required properties:
- - compatible: Should be "ti,phy-usb3" or "ti,phy-pipe3-sata".
-   "ti,omap-usb3" is deprecated.
+ - compatible: Should be "ti,phy-usb3", "ti,phy-pipe3-sata" or
+   "ti,phy-pipe3-pcie. "ti,omap-usb3" is deprecated.
  - reg : Address and length of the register set for the device.
  - reg-names: The names of the register addresses corresponding to the registers
    filled in "reg".
@@ -69,6 +69,10 @@ Required properties:
    * "wkupclk" - wakeup clock.
    * "sysclk" - system clock.
    * "refclk" - reference clock.
+   * "dpll_ref" - external dpll ref clk
+   * "dpll_ref_m2" - external dpll ref clk
+   * "phy-div" - divider for apll
+   * "div-clk" - apll clock
 
 Optional properties:
  - ctrl-module : phandle of the control module used by PHY driver to power on
diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index 5913676..d43019d 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -80,6 +80,7 @@ struct ti_pipe3 {
 	struct clk		*wkupclk;
 	struct clk		*sys_clk;
 	struct clk		*refclk;
+	struct clk		*div_clk;
 	struct pipe3_dpll_map	*dpll_map;
 };
 
@@ -215,6 +216,9 @@ static int ti_pipe3_init(struct phy *x)
 	u32 val;
 	int ret = 0;
 
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie"))
+		return 0;
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {
@@ -238,8 +242,11 @@ static int ti_pipe3_exit(struct phy *x)
 	u32 val;
 	unsigned long timeout;
 
-	/* SATA DPLL can't be powered down due to Errata i783 */
-	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata"))
+	/* SATA DPLL can't be powered down due to Errata i783 and PCIe
+	 * does not have internal DPLL
+	 */
+	if (of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-sata") ||
+	    of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie"))
 		return 0;
 
 	/* Put DPLL in IDLE mode */
@@ -286,32 +293,41 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	struct device_node *control_node;
 	struct platform_device *control_pdev;
 	const struct of_device_id *match;
-
-	match = of_match_device(of_match_ptr(ti_pipe3_id_table), &pdev->dev);
-	if (!match)
-		return -EINVAL;
+	struct clk *clk;
 
 	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy) {
 		dev_err(&pdev->dev, "unable to alloc mem for TI PIPE3 PHY\n");
 		return -ENOMEM;
 	}
+	phy->dev		= &pdev->dev;
 
-	phy->dpll_map = (struct pipe3_dpll_map *)match->data;
-	if (!phy->dpll_map) {
-		dev_err(&pdev->dev, "no DPLL data\n");
-		return -EINVAL;
-	}
+	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
+		match = of_match_device(of_match_ptr(ti_pipe3_id_table),
+					&pdev->dev);
+		if (!match)
+			return -EINVAL;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pll_ctrl");
-	phy->pll_ctrl_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(phy->pll_ctrl_base))
-		return PTR_ERR(phy->pll_ctrl_base);
+		phy->dpll_map = (struct pipe3_dpll_map *)match->data;
+		if (!phy->dpll_map) {
+			dev_err(&pdev->dev, "no DPLL data\n");
+			return -EINVAL;
+		}
 
-	phy->dev		= &pdev->dev;
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   "pll_ctrl");
+		phy->pll_ctrl_base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(phy->pll_ctrl_base))
+			return PTR_ERR(phy->pll_ctrl_base);
 
-	if (!of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
+		phy->sys_clk = devm_clk_get(phy->dev, "sysclk");
+		if (IS_ERR(phy->sys_clk)) {
+			dev_err(&pdev->dev, "unable to get sysclk\n");
+			return -EINVAL;
+		}
+	}
 
+	if (!of_device_is_compatible(node, "ti,phy-pipe3-sata")) {
 		phy->wkupclk = devm_clk_get(phy->dev, "wkupclk");
 		if (IS_ERR(phy->wkupclk)) {
 			dev_err(&pdev->dev, "unable to get wkupclk\n");
@@ -328,10 +344,35 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		phy->refclk = ERR_PTR(-ENODEV);
 	}
 
-	phy->sys_clk = devm_clk_get(phy->dev, "sysclk");
-	if (IS_ERR(phy->sys_clk)) {
-		dev_err(&pdev->dev, "unable to get sysclk\n");
-		return -EINVAL;
+	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
+		clk = devm_clk_get(phy->dev, "dpll_ref");
+		if (IS_ERR(clk)) {
+			dev_err(&pdev->dev, "unable to get dpll ref clk\n");
+			return PTR_ERR(clk);
+		}
+		clk_set_rate(clk, 1500000000);
+
+		clk = devm_clk_get(phy->dev, "dpll_ref_m2");
+		if (IS_ERR(clk)) {
+			dev_err(&pdev->dev, "unable to get dpll ref m2 clk\n");
+			return PTR_ERR(clk);
+		}
+		clk_set_rate(clk, 100000000);
+
+		clk = devm_clk_get(phy->dev, "phy-div");
+		if (IS_ERR(clk)) {
+			dev_err(&pdev->dev, "unable to get phy-div clk\n");
+			return PTR_ERR(clk);
+		}
+		clk_set_rate(clk, 100000000);
+
+		phy->div_clk = devm_clk_get(phy->dev, "div-clk");
+		if (IS_ERR(phy->div_clk)) {
+			dev_err(&pdev->dev, "unable to get div-clk\n");
+			return PTR_ERR(phy->div_clk);
+		}
+	} else {
+		phy->div_clk = ERR_PTR(-ENODEV);
 	}
 
 	control_node = of_parse_phandle(node, "ctrl-module", 0);
@@ -387,6 +428,8 @@ static int ti_pipe3_runtime_suspend(struct device *dev)
 		clk_disable_unprepare(phy->wkupclk);
 	if (!IS_ERR(phy->refclk))
 		clk_disable_unprepare(phy->refclk);
+	if (!IS_ERR(phy->div_clk))
+		clk_disable_unprepare(phy->div_clk);
 
 	return 0;
 }
@@ -412,8 +455,19 @@ static int ti_pipe3_runtime_resume(struct device *dev)
 		}
 	}
 
+	if (!IS_ERR(phy->div_clk)) {
+		ret = clk_prepare_enable(phy->div_clk);
+		if (ret) {
+			dev_err(phy->dev, "Failed to enable div_clk %d\n", ret);
+			goto err3;
+		}
+	}
 	return 0;
 
+err3:
+	if (!IS_ERR(phy->wkupclk))
+		clk_disable_unprepare(phy->wkupclk);
+
 err2:
 	if (!IS_ERR(phy->refclk))
 		clk_disable_unprepare(phy->refclk);
@@ -446,6 +500,9 @@ static const struct of_device_id ti_pipe3_id_table[] = {
 		.compatible = "ti,phy-pipe3-sata",
 		.data = dpll_map_sata,
 	},
+	{
+		.compatible = "ti,phy-pipe3-pcie",
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, ti_pipe3_id_table);
-- 
1.7.9.5

Re: [PATCH 01/17] phy: phy-omap-pipe3: Add support for PCIe PHY

[Roger Quadros]

On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> PCIe PHY uses an external pll instead of the internal pll used by SATA
> and USB3. So added support in pipe3 PHY to use external pll.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

Reviewed-by: Roger Quadros <rogerq@ti.com>

--
cheers,
-roger

[PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

APLL used by PCIE phy can either use external clock as input or the clock
from DPLL. Added support for the APLL to use external clock as input here.

Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
 drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
 2 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
index bc9afb5..d50f8ee 100644
--- a/Documentation/devicetree/bindings/phy/ti-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -76,6 +76,10 @@ Required properties:
    * "dpll_ref_m2" - external dpll ref clk
    * "phy-div" - divider for apll
    * "div-clk" - apll clock
+   * "apll_mux" - mux for pcie apll
+   * "refclk_ext" - external reference clock for pcie apll
+ - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
+   only to PCIE PHY.
 
 Optional properties:
  - ctrl-module : phandle of the control module used by PHY driver to power on
diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
index d43019d..5513aa0 100644
--- a/drivers/phy/phy-ti-pipe3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -293,7 +293,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	struct device_node *control_node;
 	struct platform_device *control_pdev;
 	const struct of_device_id *match;
-	struct clk *clk;
+	struct clk *clk, *pclk;
 
 	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy) {
@@ -302,6 +302,20 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	}
 	phy->dev		= &pdev->dev;
 
+	control_node = of_parse_phandle(node, "ctrl-module", 0);
+	if (!control_node) {
+		dev_err(&pdev->dev, "Failed to get control device phandle\n");
+		return -EINVAL;
+	}
+
+	control_pdev = of_find_device_by_node(control_node);
+	if (!control_pdev) {
+		dev_err(&pdev->dev, "Failed to get control device\n");
+		return -EINVAL;
+	}
+
+	phy->control_dev = &control_pdev->dev;
+
 	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
 		match = of_match_device(of_match_ptr(ti_pipe3_id_table),
 					&pdev->dev);
@@ -345,19 +359,40 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	}
 
 	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
-		clk = devm_clk_get(phy->dev, "dpll_ref");
-		if (IS_ERR(clk)) {
-			dev_err(&pdev->dev, "unable to get dpll ref clk\n");
-			return PTR_ERR(clk);
+		if (!of_property_read_bool(node, "ti,ext-clk")) {
+			clk = devm_clk_get(phy->dev, "dpll_ref");
+			if (IS_ERR(clk)) {
+				dev_err(&pdev->dev,
+					"unable to get dpll ref clk\n");
+				return PTR_ERR(clk);
+			}
+			clk_set_rate(clk, 1500000000);
+
+			clk = devm_clk_get(phy->dev, "dpll_ref_m2");
+			if (IS_ERR(clk)) {
+				dev_err(&pdev->dev,
+					"unable to get dpll ref m2 clk\n");
+				return PTR_ERR(clk);
+			}
+			clk_set_rate(clk, 100000000);
+		} else {
+			omap_control_pcie_tx_rx_control(phy->control_dev,
+						OMAP_CTRL_PCIE_PHY_RX_ACSPCIE);
+
+			clk = clk_get(phy->dev, "apll_mux");
+			if (IS_ERR(clk)) {
+				dev_err(&pdev->dev, "unable to get apll mux clk\n");
+				return PTR_ERR(clk);
+			}
+
+			pclk = clk_get(phy->dev, "refclk_ext");
+			if (IS_ERR(pclk)) {
+				dev_err(&pdev->dev, "unable to get ext ref clk for apll\n");
+				return PTR_ERR(pclk);
+			}
+
+			clk_set_parent(clk, pclk);
 		}
-		clk_set_rate(clk, 1500000000);
-
-		clk = devm_clk_get(phy->dev, "dpll_ref_m2");
-		if (IS_ERR(clk)) {
-			dev_err(&pdev->dev, "unable to get dpll ref m2 clk\n");
-			return PTR_ERR(clk);
-		}
-		clk_set_rate(clk, 100000000);
 
 		clk = devm_clk_get(phy->dev, "phy-div");
 		if (IS_ERR(clk)) {
@@ -375,20 +410,6 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 		phy->div_clk = ERR_PTR(-ENODEV);
 	}
 
-	control_node = of_parse_phandle(node, "ctrl-module", 0);
-	if (!control_node) {
-		dev_err(&pdev->dev, "Failed to get control device phandle\n");
-		return -EINVAL;
-	}
-
-	control_pdev = of_find_device_by_node(control_node);
-	if (!control_pdev) {
-		dev_err(&pdev->dev, "Failed to get control device\n");
-		return -EINVAL;
-	}
-
-	phy->control_dev = &control_pdev->dev;
-
 	omap_control_phy_power(phy->control_dev, 0);
 
 	platform_set_drvdata(pdev, phy);
-- 
1.7.9.5

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Roger Quadros]

Hi Kishon,

On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> APLL used by PCIE phy can either use external clock as input or the clock
> from DPLL. Added support for the APLL to use external clock as input here.
> 
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>  2 files changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
> index bc9afb5..d50f8ee 100644
> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> @@ -76,6 +76,10 @@ Required properties:
>     * "dpll_ref_m2" - external dpll ref clk
>     * "phy-div" - divider for apll
>     * "div-clk" - apll clock
> +   * "apll_mux" - mux for pcie apll
> +   * "refclk_ext" - external reference clock for pcie apll
> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
> +   only to PCIE PHY.

Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?

>  
>  Optional properties:
>   - ctrl-module : phandle of the control module used by PHY driver to power on
> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
> index d43019d..5513aa0 100644
> --- a/drivers/phy/phy-ti-pipe3.c
> +++ b/drivers/phy/phy-ti-pipe3.c
> @@ -293,7 +293,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	struct device_node *control_node;
>  	struct platform_device *control_pdev;
>  	const struct of_device_id *match;
> -	struct clk *clk;
> +	struct clk *clk, *pclk;
>  
>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy) {
> @@ -302,6 +302,20 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	}
>  	phy->dev		= &pdev->dev;
>  
> +	control_node = of_parse_phandle(node, "ctrl-module", 0);
> +	if (!control_node) {
> +		dev_err(&pdev->dev, "Failed to get control device phandle\n");
> +		return -EINVAL;
> +	}
> +
> +	control_pdev = of_find_device_by_node(control_node);
> +	if (!control_pdev) {
> +		dev_err(&pdev->dev, "Failed to get control device\n");
> +		return -EINVAL;
> +	}
> +
> +	phy->control_dev = &control_pdev->dev;
> +

Why this code was moved move is not part of patch description/subject.

>  	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>  		match = of_match_device(of_match_ptr(ti_pipe3_id_table),
>  					&pdev->dev);
> @@ -345,19 +359,40 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	}
>  
>  	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
> -		clk = devm_clk_get(phy->dev, "dpll_ref");
> -		if (IS_ERR(clk)) {
> -			dev_err(&pdev->dev, "unable to get dpll ref clk\n");
> -			return PTR_ERR(clk);
> +		if (!of_property_read_bool(node, "ti,ext-clk")) {
> +			clk = devm_clk_get(phy->dev, "dpll_ref");
> +			if (IS_ERR(clk)) {
> +				dev_err(&pdev->dev,
> +					"unable to get dpll ref clk\n");
> +				return PTR_ERR(clk);
> +			}
> +			clk_set_rate(clk, 1500000000);
> +
> +			clk = devm_clk_get(phy->dev, "dpll_ref_m2");
> +			if (IS_ERR(clk)) {
> +				dev_err(&pdev->dev,
> +					"unable to get dpll ref m2 clk\n");
> +				return PTR_ERR(clk);
> +			}
> +			clk_set_rate(clk, 100000000);
> +		} else {
> +			omap_control_pcie_tx_rx_control(phy->control_dev,
> +						OMAP_CTRL_PCIE_PHY_RX_ACSPCIE);
> +
> +			clk = clk_get(phy->dev, "apll_mux");
> +			if (IS_ERR(clk)) {
> +				dev_err(&pdev->dev, "unable to get apll mux clk\n");
> +				return PTR_ERR(clk);
> +			}
> +
> +			pclk = clk_get(phy->dev, "refclk_ext");
> +			if (IS_ERR(pclk)) {
> +				dev_err(&pdev->dev, "unable to get ext ref clk for apll\n");
> +				return PTR_ERR(pclk);
> +			}
> +
> +			clk_set_parent(clk, pclk);
>  		}
> -		clk_set_rate(clk, 1500000000);
> -
> -		clk = devm_clk_get(phy->dev, "dpll_ref_m2");
> -		if (IS_ERR(clk)) {
> -			dev_err(&pdev->dev, "unable to get dpll ref m2 clk\n");
> -			return PTR_ERR(clk);
> -		}
> -		clk_set_rate(clk, 100000000);
>  
>  		clk = devm_clk_get(phy->dev, "phy-div");
>  		if (IS_ERR(clk)) {
> @@ -375,20 +410,6 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  		phy->div_clk = ERR_PTR(-ENODEV);
>  	}
>  
> -	control_node = of_parse_phandle(node, "ctrl-module", 0);
> -	if (!control_node) {
> -		dev_err(&pdev->dev, "Failed to get control device phandle\n");
> -		return -EINVAL;
> -	}
> -
> -	control_pdev = of_find_device_by_node(control_node);
> -	if (!control_pdev) {
> -		dev_err(&pdev->dev, "Failed to get control device\n");
> -		return -EINVAL;
> -	}
> -
> -	phy->control_dev = &control_pdev->dev;
> -
>  	omap_control_phy_power(phy->control_dev, 0);
>  
>  	platform_set_drvdata(pdev, phy);
> 

--
cheers,
-roger

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi Roger,

On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
> Hi Kishon,
> 
> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>> APLL used by PCIE phy can either use external clock as input or the clock
>> from DPLL. Added support for the APLL to use external clock as input here.
>>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Tero Kristo <t-kristo@ti.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> index bc9afb5..d50f8ee 100644
>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> @@ -76,6 +76,10 @@ Required properties:
>>     * "dpll_ref_m2" - external dpll ref clk
>>     * "phy-div" - divider for apll
>>     * "div-clk" - apll clock
>> +   * "apll_mux" - mux for pcie apll
>> +   * "refclk_ext" - external reference clock for pcie apll
>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>> +   only to PCIE PHY.
> 
> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?

Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
go in the board dt file (suggested by Nishanth).
The point is at some point later if some one wants to change the clock source,
it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
phandle etc..
> 
>>  
>>  Optional properties:
>>   - ctrl-module : phandle of the control module used by PHY driver to power on
>> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
>> index d43019d..5513aa0 100644
>> --- a/drivers/phy/phy-ti-pipe3.c
>> +++ b/drivers/phy/phy-ti-pipe3.c
>> @@ -293,7 +293,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	struct device_node *control_node;
>>  	struct platform_device *control_pdev;
>>  	const struct of_device_id *match;
>> -	struct clk *clk;
>> +	struct clk *clk, *pclk;
>>  
>>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>>  	if (!phy) {
>> @@ -302,6 +302,20 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	}
>>  	phy->dev		= &pdev->dev;
>>  
>> +	control_node = of_parse_phandle(node, "ctrl-module", 0);
>> +	if (!control_node) {
>> +		dev_err(&pdev->dev, "Failed to get control device phandle\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	control_pdev = of_find_device_by_node(control_node);
>> +	if (!control_pdev) {
>> +		dev_err(&pdev->dev, "Failed to get control device\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	phy->control_dev = &control_pdev->dev;
>> +
> 
> Why this code was moved move is not part of patch description/subject.

external clock support needs 'control_dev', so had to move the get control
device part before configuring the clocks.
> 
>>  	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>>  		match = of_match_device(of_match_ptr(ti_pipe3_id_table),
>>  					&pdev->dev);
>> @@ -345,19 +359,40 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>> -		clk = devm_clk_get(phy->dev, "dpll_ref");
>> -		if (IS_ERR(clk)) {
>> -			dev_err(&pdev->dev, "unable to get dpll ref clk\n");
>> -			return PTR_ERR(clk);
>> +		if (!of_property_read_bool(node, "ti,ext-clk")) {
>> +			clk = devm_clk_get(phy->dev, "dpll_ref");
>> +			if (IS_ERR(clk)) {
>> +				dev_err(&pdev->dev,
>> +					"unable to get dpll ref clk\n");
>> +				return PTR_ERR(clk);
>> +			}
>> +			clk_set_rate(clk, 1500000000);
>> +
>> +			clk = devm_clk_get(phy->dev, "dpll_ref_m2");
>> +			if (IS_ERR(clk)) {
>> +				dev_err(&pdev->dev,
>> +					"unable to get dpll ref m2 clk\n");
>> +				return PTR_ERR(clk);
>> +			}
>> +			clk_set_rate(clk, 100000000);
>> +		} else {
>> +			omap_control_pcie_tx_rx_control(phy->control_dev,
>> +						OMAP_CTRL_PCIE_PHY_RX_ACSPCIE);

                        ^^here

Thanks
Kishon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi Roger,
>
> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>> Hi Kishon,
>>
>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>> APLL used by PCIE phy can either use external clock as input or the clock
>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>
>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>> Cc: Tero Kristo <t-kristo@ti.com>
>>> Cc: Paul Walmsley <paul@pwsan.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> index bc9afb5..d50f8ee 100644
>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>> @@ -76,6 +76,10 @@ Required properties:
>>>     * "dpll_ref_m2" - external dpll ref clk
>>>     * "phy-div" - divider for apll
>>>     * "div-clk" - apll clock
>>> +   * "apll_mux" - mux for pcie apll
>>> +   * "refclk_ext" - external reference clock for pcie apll
>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>> +   only to PCIE PHY.
>>
>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>
> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
> go in the board dt file (suggested by Nishanth).
> The point is at some point later if some one wants to change the clock source,
> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
> phandle etc..

Wonder if that is implicit by the presence of  "refclk_ext" in the
clocks provided?

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi Nishanth,

On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi Roger,
>>
>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>> Hi Kishon,
>>>
>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>
>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> index bc9afb5..d50f8ee 100644
>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>     * "phy-div" - divider for apll
>>>>     * "div-clk" - apll clock
>>>> +   * "apll_mux" - mux for pcie apll
>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>> +   only to PCIE PHY.
>>>
>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>
>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>> go in the board dt file (suggested by Nishanth).
>> The point is at some point later if some one wants to change the clock source,
>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>> phandle etc..
> 
> Wonder if that is implicit by the presence of  "refclk_ext" in the
> clocks provided?

IMO the presence of "refclk_ext" is useless unless the board indicates it
provides the clock source.

refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
board provides a clock or not, it can have that handle for configuring in PRCM.
However if the board does not provide the clock source, configuring refclk_ext
in PRCM is useless.

Thanks
Kishon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Roger Quadros]

On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
> Hi Nishanth,
> 
> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> Hi Roger,
>>>
>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>> Hi Kishon,
>>>>
>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>
>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>> index bc9afb5..d50f8ee 100644
>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>     * "phy-div" - divider for apll
>>>>>     * "div-clk" - apll clock
>>>>> +   * "apll_mux" - mux for pcie apll
>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>> +   only to PCIE PHY.
>>>>
>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>
>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>> go in the board dt file (suggested by Nishanth).
>>> The point is at some point later if some one wants to change the clock source,
>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>> phandle etc..
>>
>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>> clocks provided?
> 
> IMO the presence of "refclk_ext" is useless unless the board indicates it
> provides the clock source.
> 
> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
> board provides a clock or not, it can have that handle for configuring in PRCM.
> However if the board does not provide the clock source, configuring refclk_ext
> in PRCM is useless.

I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.

cheers,
-roger

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>> Hi Nishanth,
>>
>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>> Hi Roger,
>>>>
>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>> Hi Kishon,
>>>>>
>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>
>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>> index bc9afb5..d50f8ee 100644
>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>     * "phy-div" - divider for apll
>>>>>>     * "div-clk" - apll clock
>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>> +   only to PCIE PHY.
>>>>>
>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>
>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>> go in the board dt file (suggested by Nishanth).
>>>> The point is at some point later if some one wants to change the clock source,
>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>> phandle etc..
>>>
>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>> clocks provided?
>>
>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>> provides the clock source.
>>
>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>> board provides a clock or not, it can have that handle for configuring in PRCM.
>> However if the board does not provide the clock source, configuring refclk_ext
>> in PRCM is useless.
>
> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.

yes, thank you for clarifying - it does indeed redundant to have
"ti,ext-clk". and apologies on being a little obscure in the comment.

Regards,
Nishanth Menon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi Nishant,

On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>> Hi Nishanth,
>>>
>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>> Hi Kishon,
>>>>>>
>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>
>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>     * "phy-div" - divider for apll
>>>>>>>     * "div-clk" - apll clock
>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>> +   only to PCIE PHY.
>>>>>>
>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>
>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>> go in the board dt file (suggested by Nishanth).
>>>>> The point is at some point later if some one wants to change the clock source,
>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>> phandle etc..
>>>>
>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>> clocks provided?
>>>
>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>> provides the clock source.
>>>
>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>> However if the board does not provide the clock source, configuring refclk_ext
>>> in PRCM is useless.
>>
>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
> 
> yes, thank you for clarifying - it does indeed redundant to have
> "ti,ext-clk". and apologies on being a little obscure in the comment.

Irrespective of whether external reference clock is used or not, all DRA7
(apll) has an input for external reference clock (and also a PRCM register for
programming it) and it has to be specified in dt no?

Thanks
Kishon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi Nishant,
>
> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>>> Hi Nishanth,
>>>>
>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>>> Hi Kishon,
>>>>>>>
>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>>
>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>>     * "phy-div" - divider for apll
>>>>>>>>     * "div-clk" - apll clock
>>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>>> +   only to PCIE PHY.
>>>>>>>
>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>>
>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>>> go in the board dt file (suggested by Nishanth).
>>>>>> The point is at some point later if some one wants to change the clock source,
>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>>> phandle etc..
>>>>>
>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>>> clocks provided?
>>>>
>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>>> provides the clock source.
>>>>
>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>>> However if the board does not provide the clock source, configuring refclk_ext
>>>> in PRCM is useless.
>>>
>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>>
>> yes, thank you for clarifying - it does indeed redundant to have
>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>
> Irrespective of whether external reference clock is used or not, all DRA7
> (apll) has an input for external reference clock (and also a PRCM register for
> programming it) and it has to be specified in dt no?

Why is that a binding for ti-phy? that is a problem for the APLL clock
driver (selecting it's own source). PHY properties should describe
itself -> let the bindings of the APLL describe itself. please dont
mix the two up.

Regards,
Nishanth Menon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi Nishant,
>>
>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>>>> Hi Nishanth,
>>>>>
>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>> Hi Roger,
>>>>>>>
>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>>>> Hi Kishon,
>>>>>>>>
>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>>>
>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>> ---
>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>>>     * "phy-div" - divider for apll
>>>>>>>>>     * "div-clk" - apll clock
>>>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>>>> +   only to PCIE PHY.
>>>>>>>>
>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>>>
>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>>>> go in the board dt file (suggested by Nishanth).
>>>>>>> The point is at some point later if some one wants to change the clock source,
>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>>>> phandle etc..
>>>>>>
>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>>>> clocks provided?
>>>>>
>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>>>> provides the clock source.
>>>>>
>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>>>> However if the board does not provide the clock source, configuring refclk_ext
>>>>> in PRCM is useless.
>>>>
>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>>>
>>> yes, thank you for clarifying - it does indeed redundant to have
>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>>
>> Irrespective of whether external reference clock is used or not, all DRA7
>> (apll) has an input for external reference clock (and also a PRCM register for
>> programming it) and it has to be specified in dt no?
> 
> Why is that a binding for ti-phy? that is a problem for the APLL clock
> driver (selecting it's own source). PHY properties should describe
> itself -> let the bindings of the APLL describe itself. please dont
> mix the two up.

The apll clock node is like this

apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
        compatible = "mux-clock";
        clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
        #clock-cells = <0>;
        reg = <0x4a00821c 0x4>;
        bit-mask = <0x80>;
};

The external reference clock is denoted by *pciesref_acs_clk_ck*.

refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
"clk_set_parent" to set the parent of apll mux.

Thanks
Kishon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
>> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> Hi Nishant,
>>>
>>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>>>>> Hi Nishanth,
>>>>>>
>>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>>> Hi Roger,
>>>>>>>>
>>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>>>>> Hi Kishon,
>>>>>>>>>
>>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>>>>
>>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>>> ---
>>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>>>>     * "phy-div" - divider for apll
>>>>>>>>>>     * "div-clk" - apll clock
>>>>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>>>>> +   only to PCIE PHY.
>>>>>>>>>
>>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>>>>
>>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>>>>> go in the board dt file (suggested by Nishanth).
>>>>>>>> The point is at some point later if some one wants to change the clock source,
>>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>>>>> phandle etc..
>>>>>>>
>>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>>>>> clocks provided?
>>>>>>
>>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>>>>> provides the clock source.
>>>>>>
>>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>>>>> However if the board does not provide the clock source, configuring refclk_ext
>>>>>> in PRCM is useless.
>>>>>
>>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>>>>
>>>> yes, thank you for clarifying - it does indeed redundant to have
>>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>>>
>>> Irrespective of whether external reference clock is used or not, all DRA7
>>> (apll) has an input for external reference clock (and also a PRCM register for
>>> programming it) and it has to be specified in dt no?
>>
>> Why is that a binding for ti-phy? that is a problem for the APLL clock
>> driver (selecting it's own source). PHY properties should describe
>> itself -> let the bindings of the APLL describe itself. please dont
>> mix the two up.
> 
> The apll clock node is like this
> 
> apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
>         compatible = "mux-clock";
>         clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
>         #clock-cells = <0>;
>         reg = <0x4a00821c 0x4>;
>         bit-mask = <0x80>;
> };
> 
> The external reference clock is denoted by *pciesref_acs_clk_ck*.
> 
> refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
> "clk_set_parent" to set the parent of apll mux.

So, How about this: if refclk_ext is not defined, dont do setparent,
if it is defined, do setparent.
in short:
Optional Properties:
* "refclk_ext" - external reference clock for pcie apll - if defined,
used as the parent to "apll_mux"

That said, my problem in general with this approach(which many folks
have taken of describing parent of clock X in hardware block binding
for Y) is the following:

The binding now has dependency on clock tree hierarchy. What if
towmorrow, we have a tree where refclk_ext parent of muxZ parent of
apll_mux? the binding breaks down. Lets not forget that we consider DT
as an ABI - we dont want to change the binding in the future.

I had always preferred describing parent-child relationship of clocks
by the approach Tero posted: https://patchwork.kernel.org/patch/3871551/

Maybe Mike and Tero could help here?

-- 
Regards,
Nishanth Menon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 15 May 2014 06:03 PM, Nishanth Menon wrote:
> On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
>>> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>> Hi Nishant,
>>>>
>>>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>>>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>>>>>> Hi Nishanth,
>>>>>>>
>>>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>>>> Hi Roger,
>>>>>>>>>
>>>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>>>>>> Hi Kishon,
>>>>>>>>>>
>>>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>>>>>     * "phy-div" - divider for apll
>>>>>>>>>>>     * "div-clk" - apll clock
>>>>>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>>>>>> +   only to PCIE PHY.
>>>>>>>>>>
>>>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>>>>>
>>>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>>>>>> go in the board dt file (suggested by Nishanth).
>>>>>>>>> The point is at some point later if some one wants to change the clock source,
>>>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>>>>>> phandle etc..
>>>>>>>>
>>>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>>>>>> clocks provided?
>>>>>>>
>>>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>>>>>> provides the clock source.
>>>>>>>
>>>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>>>>>> However if the board does not provide the clock source, configuring refclk_ext
>>>>>>> in PRCM is useless.
>>>>>>
>>>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>>>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>>>>>
>>>>> yes, thank you for clarifying - it does indeed redundant to have
>>>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>>>>
>>>> Irrespective of whether external reference clock is used or not, all DRA7
>>>> (apll) has an input for external reference clock (and also a PRCM register for
>>>> programming it) and it has to be specified in dt no?
>>>
>>> Why is that a binding for ti-phy? that is a problem for the APLL clock
>>> driver (selecting it's own source). PHY properties should describe
>>> itself -> let the bindings of the APLL describe itself. please dont
>>> mix the two up.
>>
>> The apll clock node is like this
>>
>> apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
>>         compatible = "mux-clock";
>>         clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
>>         #clock-cells = <0>;
>>         reg = <0x4a00821c 0x4>;
>>         bit-mask = <0x80>;
>> };
>>
>> The external reference clock is denoted by *pciesref_acs_clk_ck*.
>>
>> refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
>> "clk_set_parent" to set the parent of apll mux.
> 
> So, How about this: if refclk_ext is not defined, dont do setparent,
> if it is defined, do setparent.
> in short:
> Optional Properties:
> * "refclk_ext" - external reference clock for pcie apll - if defined,
> used as the parent to "apll_mux"

Ok, will remove "ti,ext-clk".

Thanks
Kishon
> 
> That said, my problem in general with this approach(which many folks
> have taken of describing parent of clock X in hardware block binding
> for Y) is the following:
> 
> The binding now has dependency on clock tree hierarchy. What if
> towmorrow, we have a tree where refclk_ext parent of muxZ parent of
> apll_mux? the binding breaks down. Lets not forget that we consider DT
> as an ABI - we dont want to change the binding in the future.
> 
> I had always preferred describing parent-child relationship of clocks
> by the approach Tero posted: https://patchwork.kernel.org/patch/3871551/
> 
> Maybe Mike and Tero could help here?
> 

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 15 May 2014 06:03 PM, Nishanth Menon wrote:
> On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
>>> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>> Hi Nishant,
>>>>
>>>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>>>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>>>>>>> Hi Nishanth,
>>>>>>>
>>>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>>>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>>>> Hi Roger,
>>>>>>>>>
>>>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>>>>>>>>>> Hi Kishon,
>>>>>>>>>>
>>>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>>>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>>>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>>>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>>>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>>>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>>>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> index bc9afb5..d50f8ee 100644
>>>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>>>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>>>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>>>>>>>>>>>     * "phy-div" - divider for apll
>>>>>>>>>>>     * "div-clk" - apll clock
>>>>>>>>>>> +   * "apll_mux" - mux for pcie apll
>>>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>>>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>>>>>>>>>>> +   only to PCIE PHY.
>>>>>>>>>>
>>>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>>>>>>>>>
>>>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>>>>>>>>> go in the board dt file (suggested by Nishanth).
>>>>>>>>> The point is at some point later if some one wants to change the clock source,
>>>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>>>>>>>>> phandle etc..
>>>>>>>>
>>>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>>>>>>>> clocks provided?
>>>>>>>
>>>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>>>>>>> provides the clock source.
>>>>>>>
>>>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>>>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>>>>>>> However if the board does not provide the clock source, configuring refclk_ext
>>>>>>> in PRCM is useless.
>>>>>>
>>>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>>>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>>>>>
>>>>> yes, thank you for clarifying - it does indeed redundant to have
>>>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>>>>
>>>> Irrespective of whether external reference clock is used or not, all DRA7
>>>> (apll) has an input for external reference clock (and also a PRCM register for
>>>> programming it) and it has to be specified in dt no?
>>>
>>> Why is that a binding for ti-phy? that is a problem for the APLL clock
>>> driver (selecting it's own source). PHY properties should describe
>>> itself -> let the bindings of the APLL describe itself. please dont
>>> mix the two up.
>>
>> The apll clock node is like this
>>
>> apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
>>         compatible = "mux-clock";
>>         clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
>>         #clock-cells = <0>;
>>         reg = <0x4a00821c 0x4>;
>>         bit-mask = <0x80>;
>> };
>>
>> The external reference clock is denoted by *pciesref_acs_clk_ck*.
>>
>> refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
>> "clk_set_parent" to set the parent of apll mux.
> 
> So, How about this: if refclk_ext is not defined, dont do setparent,
> if it is defined, do setparent.
> in short:
> Optional Properties:
> * "refclk_ext" - external reference clock for pcie apll - if defined,
> used as the parent to "apll_mux"

just realized, refclk_ext is not a property by itself but just one of the
phandle for clocks/clock-names. Since we want to indicate if we are using
external clock from *board* dts file, I'm not sure if there is a way to
*append* a property.

Thanks
Kishon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Mike Turquette]

Quoting Nishanth Menon (2014-05-15 05:33:13)
> On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
> > Hi,
> > 
> > On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
> >> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>> Hi Nishant,
> >>>
> >>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
> >>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
> >>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
> >>>>>> Hi Nishanth,
> >>>>>>
> >>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
> >>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>>>>>>> Hi Roger,
> >>>>>>>>
> >>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
> >>>>>>>>> Hi Kishon,
> >>>>>>>>>
> >>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> >>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
> >>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
> >>>>>>>>>>
> >>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
> >>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
> >>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
> >>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
> >>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
> >>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
> >>>>>>>>>> index bc9afb5..d50f8ee 100644
> >>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
> >>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
> >>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
> >>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
> >>>>>>>>>>     * "phy-div" - divider for apll
> >>>>>>>>>>     * "div-clk" - apll clock
> >>>>>>>>>> +   * "apll_mux" - mux for pcie apll
> >>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
> >>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
> >>>>>>>>>> +   only to PCIE PHY.
> >>>>>>>>>
> >>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
> >>>>>>>>
> >>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
> >>>>>>>> go in the board dt file (suggested by Nishanth).
> >>>>>>>> The point is at some point later if some one wants to change the clock source,
> >>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
> >>>>>>>> phandle etc..
> >>>>>>>
> >>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
> >>>>>>> clocks provided?
> >>>>>>
> >>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
> >>>>>> provides the clock source.
> >>>>>>
> >>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
> >>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
> >>>>>> However if the board does not provide the clock source, configuring refclk_ext
> >>>>>> in PRCM is useless.
> >>>>>
> >>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
> >>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
> >>>>
> >>>> yes, thank you for clarifying - it does indeed redundant to have
> >>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
> >>>
> >>> Irrespective of whether external reference clock is used or not, all DRA7
> >>> (apll) has an input for external reference clock (and also a PRCM register for
> >>> programming it) and it has to be specified in dt no?
> >>
> >> Why is that a binding for ti-phy? that is a problem for the APLL clock
> >> driver (selecting it's own source). PHY properties should describe
> >> itself -> let the bindings of the APLL describe itself. please dont
> >> mix the two up.
> > 
> > The apll clock node is like this
> > 
> > apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
> >         compatible = "mux-clock";
> >         clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
> >         #clock-cells = <0>;
> >         reg = <0x4a00821c 0x4>;
> >         bit-mask = <0x80>;
> > };
> > 
> > The external reference clock is denoted by *pciesref_acs_clk_ck*.
> > 
> > refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
> > "clk_set_parent" to set the parent of apll mux.
> 
> So, How about this: if refclk_ext is not defined, dont do setparent,
> if it is defined, do setparent.
> in short:
> Optional Properties:
> * "refclk_ext" - external reference clock for pcie apll - if defined,
> used as the parent to "apll_mux"
> 
> That said, my problem in general with this approach(which many folks
> have taken of describing parent of clock X in hardware block binding
> for Y) is the following:
> 
> The binding now has dependency on clock tree hierarchy. What if
> towmorrow, we have a tree where refclk_ext parent of muxZ parent of
> apll_mux? the binding breaks down. Lets not forget that we consider DT
> as an ABI - we dont want to change the binding in the future.
> 
> I had always preferred describing parent-child relationship of clocks
> by the approach Tero posted: https://patchwork.kernel.org/patch/3871551/
> 
> Maybe Mike and Tero could help here?

Hi all,

I didn't read all of this context, but it seems like
https://lkml.org/lkml/2014/5/19/539

might be a good fit here. Assuming that the PIPE3 PHY consumes the APLL
clock, then it reasonable for the dts phy node to specify
assigned-clock-parent.

Regards,
Mike

> 
> -- 
> Regards,
> Nishanth Menon

Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On Tue, May 27, 2014 at 8:54 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Nishanth Menon (2014-05-15 05:33:13)
>> On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
>> > Hi,
>> >
>> > On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
>> >> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> >>> Hi Nishant,
>> >>>
>> >>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>> >>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rogerq@ti.com> wrote:
>> >>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>> >>>>>> Hi Nishanth,
>> >>>>>>
>> >>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>> >>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> >>>>>>>> Hi Roger,
>> >>>>>>>>
>> >>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>> >>>>>>>>> Hi Kishon,
>> >>>>>>>>>
>> >>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>> >>>>>>>>>> APLL used by PCIE phy can either use external clock as input or the clock
>> >>>>>>>>>> from DPLL. Added support for the APLL to use external clock as input here.
>> >>>>>>>>>>
>> >>>>>>>>>> Cc: Rajendra Nayak <rnayak@ti.com>
>> >>>>>>>>>> Cc: Tero Kristo <t-kristo@ti.com>
>> >>>>>>>>>> Cc: Paul Walmsley <paul@pwsan.com>
>> >>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> >>>>>>>>>> ---
>> >>>>>>>>>>  Documentation/devicetree/bindings/phy/ti-phy.txt |    4 ++
>> >>>>>>>>>>  drivers/phy/phy-ti-pipe3.c                       |   75 ++++++++++++++--------
>> >>>>>>>>>>  2 files changed, 52 insertions(+), 27 deletions(-)
>> >>>>>>>>>>
>> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> index bc9afb5..d50f8ee 100644
>> >>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>> >>>>>>>>>>     * "dpll_ref_m2" - external dpll ref clk
>> >>>>>>>>>>     * "phy-div" - divider for apll
>> >>>>>>>>>>     * "div-clk" - apll clock
>> >>>>>>>>>> +   * "apll_mux" - mux for pcie apll
>> >>>>>>>>>> +   * "refclk_ext" - external reference clock for pcie apll
>> >>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external clock. Applicable
>> >>>>>>>>>> +   only to PCIE PHY.
>> >>>>>>>>>
>> >>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock", "refclk_ext" and then specifying a 3rd control option "ti,ext-clk" to select one of the 2 sources, why can't the DT just supply one clock source, i.e. the one that is being used in the board instance? The driver should then just configure the clock rate that is needed at that node. Shouldn't the clock framework automatically take care of muxing and parent rates?
>> >>>>>>>>
>> >>>>>>>> Want the dt to have all the clocks used by the controller. "ti,ext-clk" should
>> >>>>>>>> go in the board dt file (suggested by Nishanth).
>> >>>>>>>> The point is at some point later if some one wants to change the clock source,
>> >>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding the clock
>> >>>>>>>> phandle etc..
>> >>>>>>>
>> >>>>>>> Wonder if that is implicit by the presence of  "refclk_ext" in the
>> >>>>>>> clocks provided?
>> >>>>>>
>> >>>>>> IMO the presence of "refclk_ext" is useless unless the board indicates it
>> >>>>>> provides the clock source.
>> >>>>>>
>> >>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of whether the
>> >>>>>> board provides a clock or not, it can have that handle for configuring in PRCM.
>> >>>>>> However if the board does not provide the clock source, configuring refclk_ext
>> >>>>>> in PRCM is useless.
>> >>>>>
>> >>>>> I think what Nishant meant is that if "refclk_ext" is provided it means that the driver
>> >>>>> should use that over "dpll_ref_clock" so no need of a separate "ti,ext-clk" flag.
>> >>>>
>> >>>> yes, thank you for clarifying - it does indeed redundant to have
>> >>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>> >>>
>> >>> Irrespective of whether external reference clock is used or not, all DRA7
>> >>> (apll) has an input for external reference clock (and also a PRCM register for
>> >>> programming it) and it has to be specified in dt no?
>> >>
>> >> Why is that a binding for ti-phy? that is a problem for the APLL clock
>> >> driver (selecting it's own source). PHY properties should describe
>> >> itself -> let the bindings of the APLL describe itself. please dont
>> >> mix the two up.
>> >
>> > The apll clock node is like this
>> >
>> > apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
>> >         compatible = "mux-clock";
>> >         clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
>> >         #clock-cells = <0>;
>> >         reg = <0x4a00821c 0x4>;
>> >         bit-mask = <0x80>;
>> > };
>> >
>> > The external reference clock is denoted by *pciesref_acs_clk_ck*.
>> >
>> > refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
>> > "clk_set_parent" to set the parent of apll mux.
>>
>> So, How about this: if refclk_ext is not defined, dont do setparent,
>> if it is defined, do setparent.
>> in short:
>> Optional Properties:
>> * "refclk_ext" - external reference clock for pcie apll - if defined,
>> used as the parent to "apll_mux"
>>
>> That said, my problem in general with this approach(which many folks
>> have taken of describing parent of clock X in hardware block binding
>> for Y) is the following:
>>
>> The binding now has dependency on clock tree hierarchy. What if
>> towmorrow, we have a tree where refclk_ext parent of muxZ parent of
>> apll_mux? the binding breaks down. Lets not forget that we consider DT
>> as an ABI - we dont want to change the binding in the future.
>>
>> I had always preferred describing parent-child relationship of clocks
>> by the approach Tero posted: https://patchwork.kernel.org/patch/3871551/
>>
>> Maybe Mike and Tero could help here?
>
> Hi all,
>
> I didn't read all of this context, but it seems like
> https://lkml.org/lkml/2014/5/19/539
>
> might be a good fit here. Assuming that the PIPE3 PHY consumes the APLL
> clock, then it reasonable for the dts phy node to specify
> assigned-clock-parent.


Yes indeed. Seems like the need to describe some sort of parent
description in dt makes sense for other users as well. I wonder if we
will see some alignment soonish enough for us to start using them.

Regards,
Nishanth Menon

[PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Kishon Vijay Abraham I]

Added support for pcie controller in dra7xx. This driver re-uses
the designware core code that is already present in kernel.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/pci/ti-pci.txt |   33 ++
 drivers/pci/host/Kconfig                         |   10 +
 drivers/pci/host/Makefile                        |    1 +
 drivers/pci/host/pci-dra7xx.c                    |  385 ++++++++++++++++++++++
 4 files changed, 429 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/ti-pci.txt
 create mode 100644 drivers/pci/host/pci-dra7xx.c

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
new file mode 100644
index 0000000..6cb6f09
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -0,0 +1,33 @@
+TI PCI Controllers
+
+PCIe Designware Controller
+This node should have the properties described in "designware-pcie.txt".
+ - compatible: Should be "ti,dra7xx-pcie""
+ - reg : Address and length of the register set for the device.
+ - reg-names : "ti_conf" for the TI specific registers and rc_dbics for the
+   "designware" registers.
+ - phys : the phandle for the PHY device (used by generic PHY framework)
+ - phy-names : the names of the PHY corresponding to the PHYs present in the
+   *phy* phandle.
+
+Example:
+pcie@51000000 {
+	compatible = "ti,dra7xx-pcie";
+	reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
+	reg-names = "ti_conf", "rc_dbics";
+	interrupts = <0 232 0x4>, <0 233 0x4>;
+	#address-cells = <3>;
+	#size-cells = <2>;
+	device_type = "pci";
+	ti,device_type = <3>;
+	ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */
+		  0x81000000 0 0          0x20003000 0 0x00010000  /* IO Space */
+		  0x82000000 0 0x20013000 0x20013000 0 0xffed000>; /* MEM Space */
+	#interrupt-cells = <1>;
+	num-lanes = <1>;
+	interrupt-map-mask = <0 0 0 0>;
+	interrupt-map = <0x0 0 &gic 134>;
+	ti,hwmods = "pcie1";
+	phys = <&pcie1_phy>;
+	phy-names = "pcie-phy";
+};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index a6f67ec..7be6393 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -1,6 +1,16 @@
 menu "PCI host controller drivers"
 	depends on PCI
 
+config PCI_DRA7XX
+	bool "TI DRA7xx PCIe controller"
+	select PCIE_DW
+	depends on OF || HAS_IOMEM || TI_PIPE3
+	help
+	 Enables support for the PCIE controller present in DRA7xx SoC. There
+	 are two instances of PCIE controller in DRA7xx. This controller can
+	 act both as EP and RC. This reuses the same Designware core as used
+	 by other SoCs.
+
 config PCI_MVEBU
 	bool "Marvell EBU PCIe controller"
 	depends on ARCH_MVEBU || ARCH_DOVE || ARCH_KIRKWOOD
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 13fb333..5216f55 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_PCIE_DW) += pcie-designware.o
+obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
new file mode 100644
index 0000000..a37c25c
--- /dev/null
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -0,0 +1,385 @@
+/*
+ * pcie-dra7xx - PCIe controller driver for TI DRA7xx SoCs
+ *
+ * Copyright (C) 2013-2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+/* PCIe controller wrapper DRA7XX configuration registers */
+
+#define	PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN		0x0024
+#define	PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN		0x0028
+#define	ERR_SYS						BIT(0)
+#define	ERR_FATAL					BIT(1)
+#define	ERR_NONFATAL					BIT(2)
+#define	ERR_COR						BIT(3)
+#define	ERR_AXI						BIT(4)
+#define	ERR_ECRC					BIT(5)
+#define	PME_TURN_OFF					BIT(8)
+#define	PME_TO_ACK					BIT(9)
+#define	PM_PME						BIT(10)
+#define	LINK_REQ_RST					BIT(11)
+#define	LINK_UP_EVT					BIT(12)
+#define	CFG_BME_EVT					BIT(13)
+#define	CFG_MSE_EVT					BIT(14)
+#define	INTERRUPTS (ERR_SYS | ERR_FATAL | ERR_NONFATAL | ERR_COR | ERR_AXI | \
+			ERR_ECRC | PME_TURN_OFF | PME_TO_ACK | PM_PME | \
+			LINK_REQ_RST | LINK_UP_EVT | CFG_BME_EVT | CFG_MSE_EVT)
+
+#define	PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI		0x0034
+#define	PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI		0x0038
+#define	INTA						BIT(0)
+#define	INTB						BIT(1)
+#define	INTC						BIT(2)
+#define	INTD						BIT(3)
+#define	MSI						BIT(4)
+#define	LEG_EP_INTERRUPTS (INTA | INTB | INTC | INTD)
+
+#define	PCIECTRL_DRA7XX_CONF_DEVICE_CMD			0x0104
+#define	LTSSM_EN					0x1
+
+#define	PCIECTRL_DRA7XX_CONF_PHY_CS			0x010C
+#define	LINK_UP						BIT(16)
+
+struct dra7xx_pcie {
+	void __iomem		*base;
+	struct phy		*phy;
+	struct device		*dev;
+	struct pcie_port	pp;
+};
+
+#define to_dra7xx_pcie(x)	container_of((x), struct dra7xx_pcie, pp)
+
+static inline u32 dra7xx_pcie_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void dra7xx_pcie_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static int dra7xx_pcie_link_up(struct pcie_port *pp)
+{
+	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
+	u32 reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_PHY_CS);
+
+	if (reg & LINK_UP)
+		return true;
+	return false;
+}
+
+static int dra7xx_pcie_establish_link(struct pcie_port *pp)
+{
+	u32 reg;
+	int retries = 1000;
+	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
+
+	if (dw_pcie_link_up(pp)) {
+		dev_err(pp->dev, "link is already up\n");
+		return 0;
+	}
+
+	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
+	reg |= LTSSM_EN;
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
+
+	while (--retries) {
+		reg = dra7xx_pcie_readl(dra7xx->base,
+					PCIECTRL_DRA7XX_CONF_PHY_CS);
+		if (reg & LINK_UP)
+			break;
+		usleep_range(10, 20);
+	}
+
+	if (retries <= 0) {
+		dev_err(pp->dev, "link is not up\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
+{
+	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
+
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
+			   ~INTERRUPTS);
+	dra7xx_pcie_writel(dra7xx->base,
+			   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, INTERRUPTS);
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
+			   ~LEG_EP_INTERRUPTS & ~MSI);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dra7xx_pcie_writel(dra7xx->base,
+				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI, MSI);
+	else
+		dra7xx_pcie_writel(dra7xx->base,
+				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
+				   LEG_EP_INTERRUPTS);
+}
+
+static void dra7xx_pcie_host_init(struct pcie_port *pp)
+{
+	dw_pcie_setup_rc(pp);
+	dra7xx_pcie_establish_link(pp);
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+	dra7xx_pcie_enable_interrupts(pp);
+}
+
+static struct pcie_host_ops dra7xx_pcie_host_ops = {
+	.link_up = dra7xx_pcie_link_up,
+	.host_init = dra7xx_pcie_host_init,
+};
+
+static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
+	u32 reg;
+
+	reg = dra7xx_pcie_readl(dra7xx->base,
+				PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI);
+	dw_handle_msi_irq(pp);
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
+			   reg);
+
+	return IRQ_HANDLED;
+}
+
+
+static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg)
+{
+	struct dra7xx_pcie *dra7xx = arg;
+	u32 reg;
+
+	reg = dra7xx_pcie_readl(dra7xx->base,
+				PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN);
+
+	if (reg & ERR_SYS)
+		dev_dbg(dra7xx->dev, "System Error\n");
+
+	if (reg & ERR_FATAL)
+		dev_dbg(dra7xx->dev, "Fatal Error\n");
+
+	if (reg & ERR_NONFATAL)
+		dev_dbg(dra7xx->dev, "Non Fatal Error\n");
+
+	if (reg & ERR_COR)
+		dev_dbg(dra7xx->dev, "Correctable Error\n");
+
+	if (reg & ERR_AXI)
+		dev_dbg(dra7xx->dev, "AXI tag lookup fatal Error\n");
+
+	if (reg & ERR_ECRC)
+		dev_dbg(dra7xx->dev, "ECRC Error\n");
+
+	if (reg & PME_TURN_OFF)
+		dev_dbg(dra7xx->dev,
+			"Power Management Event Turn-Off message received\n");
+
+	if (reg & PME_TO_ACK)
+		dev_dbg(dra7xx->dev,
+			"Power Management Turn-Off Ack message received\n");
+
+	if (reg & PM_PME)
+		dev_dbg(dra7xx->dev,
+			"PM Power Management Event message received\n");
+
+	if (reg & LINK_REQ_RST)
+		dev_dbg(dra7xx->dev, "Link Request Reset\n");
+
+	if (reg & LINK_UP_EVT)
+		dev_dbg(dra7xx->dev, "Link-up state change\n");
+
+	if (reg & CFG_BME_EVT)
+		dev_dbg(dra7xx->dev, "CFG 'Bus Master Enable' change\n");
+
+	if (reg & CFG_MSE_EVT)
+		dev_dbg(dra7xx->dev, "CFG 'Memory Space Enable' change\n");
+
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
+			   reg);
+
+	return IRQ_HANDLED;
+}
+
+static int add_pcie_port(struct dra7xx_pcie *dra7xx,
+			  struct platform_device *pdev)
+{
+	int ret;
+	struct pcie_port *pp;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+
+	pp = &dra7xx->pp;
+	pp->dev = dev;
+	pp->ops = &dra7xx_pcie_host_ops;
+
+	spin_lock_init(&pp->conf_lock);
+
+	pp->irq = platform_get_irq(pdev, 1);
+	if (pp->irq < 0) {
+		dev_err(dev, "missing IRQ resource\n");
+		return -EINVAL;
+	}
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		ret = devm_request_irq(&pdev->dev, pp->irq,
+				       dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
+				       "dra7-pcie-msi",	pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request irq\n");
+			return ret;
+		}
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbics");
+	pp->dbi_base = devm_ioremap_nocache(dev, res->start,
+		resource_size(res));
+	if (!pp->dbi_base)
+		return -ENOMEM;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dra7xx->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __init dra7xx_pcie_probe(struct platform_device *pdev)
+{
+	u32 reg;
+	int ret;
+	int irq;
+	struct phy *phy;
+	void __iomem *base;
+	struct resource *res;
+	struct dra7xx_pcie *dra7xx;
+	struct device *dev = &pdev->dev;
+
+	dra7xx = devm_kzalloc(&pdev->dev, sizeof(*dra7xx), GFP_KERNEL);
+	if (!dra7xx)
+		return -ENOMEM;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "missing IRQ resource\n");
+		return -EINVAL;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, dra7xx_pcie_irq_handler,
+			       IRQF_SHARED, "dra7xx-pcie-main", dra7xx);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ti_conf");
+	base = devm_ioremap_nocache(dev, res->start, resource_size(res));
+	if (!base)
+		return -ENOMEM;
+
+	phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	ret = phy_init(phy);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_power_on(phy);
+	if (ret < 0)
+		goto err_power_on;
+
+	dra7xx->base = base;
+	dra7xx->phy = phy;
+	dra7xx->dev = dev;
+
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(dev, "pm_runtime_get_sync failed\n");
+		goto err_runtime_get;
+	}
+
+	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
+	reg &= ~LTSSM_EN;
+	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
+
+	ret = add_pcie_port(dra7xx, pdev);
+	if (ret < 0)
+		goto err_add_port;
+
+	platform_set_drvdata(pdev, dra7xx);
+	return 0;
+
+err_power_on:
+	phy_exit(phy);
+
+err_runtime_get:
+	phy_power_off(phy);
+
+err_add_port:
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
+
+static int __exit dra7xx_pcie_remove(struct platform_device *pdev)
+{
+	struct dra7xx_pcie *dra7xx = platform_get_drvdata(pdev);
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	phy_power_off(dra7xx->phy);
+	phy_exit(dra7xx->phy);
+
+	return 0;
+}
+
+static const struct of_device_id of_dra7xx_pcie_match[] = {
+	{ .compatible = "ti,dra7xx-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_dra7xx_pcie_match);
+
+static struct platform_driver dra7xx_pcie_driver = {
+	.remove		= __exit_p(dra7xx_pcie_remove),
+	.driver = {
+		.name	= "dra7xx-pcie",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_dra7xx_pcie_match,
+	},
+};
+
+module_platform_driver_probe(dra7xx_pcie_driver, dra7xx_pcie_probe);
+
+MODULE_AUTHOR("Kishon Vijay Abraham I <kishon@ti.com>");
+MODULE_DESCRIPTION("TI PCIe controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Marek Vasut]

On Tuesday, May 06, 2014 at 03:33:51 PM, Kishon Vijay Abraham I wrote:
> Added support for pcie controller in dra7xx. This driver re-uses
> the designware core code that is already present in kernel.

[...]

> +#define to_dra7xx_pcie(x)	container_of((x), struct dra7xx_pcie, pp)
> +
> +static inline u32 dra7xx_pcie_readl(void __iomem *base, u32 offset)

Just pass struct dra7xx_pcie * instead of *base here , it will make the code 
below shorter.

> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void dra7xx_pcie_writel(void __iomem *base, u32 offset, u32
> value) +{

DTTO.

> +	writel(value, base + offset);
> +}
> +
> +static int dra7xx_pcie_link_up(struct pcie_port *pp)
> +{
> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> +	u32 reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_PHY_CS);
> +
> +	if (reg & LINK_UP)
> +		return true;
> +	return false;

return reg & LINK_UP;

> +}
> +
> +static int dra7xx_pcie_establish_link(struct pcie_port *pp)
> +{
> +	u32 reg;
> +	int retries = 1000;
> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> +
> +	if (dw_pcie_link_up(pp)) {
> +		dev_err(pp->dev, "link is already up\n");

This will spew, since the .link_up (and thus this function) can be called 
repeatedly. The subsystem will query if the link is up via this function.

> +		return 0;
> +	}
> +
> +	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
> +	reg |= LTSSM_EN;
> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
> +
> +	while (--retries) {

Use retries--

> +		reg = dra7xx_pcie_readl(dra7xx->base,
> +					PCIECTRL_DRA7XX_CONF_PHY_CS);
> +		if (reg & LINK_UP)
> +			break;
> +		usleep_range(10, 20);
> +	}
> +
> +	if (retries <= 0) {

Then check if retries == 0 and retries can be unsigned int.

> +		dev_err(pp->dev, "link is not up\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
[...]
> +static int __init dra7xx_pcie_probe(struct platform_device *pdev)
> +{
> +	u32 reg;
> +	int ret;
> +	int irq;
> +	struct phy *phy;
> +	void __iomem *base;
> +	struct resource *res;
> +	struct dra7xx_pcie *dra7xx;
> +	struct device *dev = &pdev->dev;
> +
> +	dra7xx = devm_kzalloc(&pdev->dev, sizeof(*dra7xx), GFP_KERNEL);
> +	if (!dra7xx)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "missing IRQ resource\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, dra7xx_pcie_irq_handler,
> +			       IRQF_SHARED, "dra7xx-pcie-main", dra7xx);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ti_conf");
> +	base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> +	if (!base)
> +		return -ENOMEM;
> +
> +	phy = devm_phy_get(dev, "pcie-phy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	ret = phy_init(phy);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_power_on(phy);
> +	if (ret < 0)
> +		goto err_power_on;
> +
> +	dra7xx->base = base;
> +	dra7xx->phy = phy;
> +	dra7xx->dev = dev;
> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (IS_ERR_VALUE(ret)) {
> +		dev_err(dev, "pm_runtime_get_sync failed\n");
> +		goto err_runtime_get;
> +	}
> +
> +	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
> +	reg &= ~LTSSM_EN;
> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);

platform_set_drvdata() should be here, before you add the port.

> +	ret = add_pcie_port(dra7xx, pdev);
> +	if (ret < 0)
> +		goto err_add_port;
> +
> +	platform_set_drvdata(pdev, dra7xx);
> +	return 0;
[...]

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Kishon Vijay Abraham I]

Hi,

On Tuesday 06 May 2014 07:14 PM, Marek Vasut wrote:
> On Tuesday, May 06, 2014 at 03:33:51 PM, Kishon Vijay Abraham I wrote:
>> Added support for pcie controller in dra7xx. This driver re-uses
>> the designware core code that is already present in kernel.
> 
> [...]
> 
>> +#define to_dra7xx_pcie(x)	container_of((x), struct dra7xx_pcie, pp)
>> +
>> +static inline u32 dra7xx_pcie_readl(void __iomem *base, u32 offset)
> 
> Just pass struct dra7xx_pcie * instead of *base here , it will make the code 
> below shorter.
> 
>> +{
>> +	return readl(base + offset);
>> +}
>> +
>> +static inline void dra7xx_pcie_writel(void __iomem *base, u32 offset, u32
>> value) +{
> 
> DTTO.
> 
>> +	writel(value, base + offset);
>> +}
>> +
>> +static int dra7xx_pcie_link_up(struct pcie_port *pp)
>> +{
>> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
>> +	u32 reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_PHY_CS);
>> +
>> +	if (reg & LINK_UP)
>> +		return true;
>> +	return false;
> 
> return reg & LINK_UP;
> 
>> +}
>> +
>> +static int dra7xx_pcie_establish_link(struct pcie_port *pp)
>> +{
>> +	u32 reg;
>> +	int retries = 1000;
>> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
>> +
>> +	if (dw_pcie_link_up(pp)) {
>> +		dev_err(pp->dev, "link is already up\n");
> 
> This will spew, since the .link_up (and thus this function) can be called 
> repeatedly. The subsystem will query if the link is up via this function.

*dra7xx_pcie_establish_link* is not the callback for link_up function, so it's
actually called only once.
> 
>> +		return 0;
>> +	}
>> +
>> +	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
>> +	reg |= LTSSM_EN;
>> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
>> +
>> +	while (--retries) {
> 
> Use retries--
> 
>> +		reg = dra7xx_pcie_readl(dra7xx->base,
>> +					PCIECTRL_DRA7XX_CONF_PHY_CS);
>> +		if (reg & LINK_UP)
>> +			break;
>> +		usleep_range(10, 20);
>> +	}
>> +
>> +	if (retries <= 0) {
> 
> Then check if retries == 0 and retries can be unsigned int.
> 
>> +		dev_err(pp->dev, "link is not up\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
> [...]
>> +static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>> +{
>> +	u32 reg;
>> +	int ret;
>> +	int irq;
>> +	struct phy *phy;
>> +	void __iomem *base;
>> +	struct resource *res;
>> +	struct dra7xx_pcie *dra7xx;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	dra7xx = devm_kzalloc(&pdev->dev, sizeof(*dra7xx), GFP_KERNEL);
>> +	if (!dra7xx)
>> +		return -ENOMEM;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, irq, dra7xx_pcie_irq_handler,
>> +			       IRQF_SHARED, "dra7xx-pcie-main", dra7xx);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq\n");
>> +		return ret;
>> +	}
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ti_conf");
>> +	base = devm_ioremap_nocache(dev, res->start, resource_size(res));
>> +	if (!base)
>> +		return -ENOMEM;
>> +
>> +	phy = devm_phy_get(dev, "pcie-phy");
>> +	if (IS_ERR(phy))
>> +		return PTR_ERR(phy);
>> +
>> +	ret = phy_init(phy);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = phy_power_on(phy);
>> +	if (ret < 0)
>> +		goto err_power_on;
>> +
>> +	dra7xx->base = base;
>> +	dra7xx->phy = phy;
>> +	dra7xx->dev = dev;
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +	ret = pm_runtime_get_sync(&pdev->dev);
>> +	if (IS_ERR_VALUE(ret)) {
>> +		dev_err(dev, "pm_runtime_get_sync failed\n");
>> +		goto err_runtime_get;
>> +	}
>> +
>> +	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
>> +	reg &= ~LTSSM_EN;
>> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
> 
> platform_set_drvdata() should be here, before you add the port.
> 
>> +	ret = add_pcie_port(dra7xx, pdev);
>> +	if (ret < 0)
>> +		goto err_add_port;
>> +
>> +	platform_set_drvdata(pdev, dra7xx);

Al-right. Will fix this and all your other comments.

Thanks
Kishon

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Pavel Machek]

> > +	writel(value, base + offset);
> > +}
> > +
> > +static int dra7xx_pcie_link_up(struct pcie_port *pp)
> > +{
> > +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> > +	u32 reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_PHY_CS);
> > +
> > +	if (reg & LINK_UP)
> > +		return true;
> > +	return false;
> 
> return reg & LINK_UP;

Function int returning true. I'd change the prototype and would
return !!(reg & ...);

									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Arnd Bergmann]

On Tuesday 06 May 2014 19:03:51 Kishon Vijay Abraham I wrote:
> Added support for pcie controller in dra7xx. This driver re-uses
> the designware core code that is already present in kernel.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

Looks pretty good overall, just a few details I noticed:

> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> new file mode 100644
> index 0000000..6cb6f09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> @@ -0,0 +1,33 @@
> +TI PCI Controllers
> +
> +PCIe Designware Controller
> +This node should have the properties described in "designware-pcie.txt".
> + - compatible: Should be "ti,dra7xx-pcie""

No "xx" in compatible strings please. Just make name this after the first
chip to use this particular interface.

> + - reg : Address and length of the register set for the device.
> + - reg-names : "ti_conf" for the TI specific registers and rc_dbics for the
> +   "designware" registers.

The description uses inconsistent quotation marks. You should also have
a fixed order in the binding, such as

 - reg : Two register ranges as listed in the reg-names property
 - reg-names : The first entry must be "ti-conf" for the TI specific registers
	       The second entry must be "rc-dbics" for the designware pcie registers.

> + - phys : the phandle for the PHY device (used by generic PHY framework)
> + - phy-names : the names of the PHY corresponding to the PHYs present in the
> +   *phy* phandle.

It's not just a phandle, it can be any phy specifier including additional
argument cells.

The second line should just read

 - phy-names : must be "pcie-phy"

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index a6f67ec..7be6393 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -1,6 +1,16 @@
>  menu "PCI host controller drivers"
>  	depends on PCI
>  
> +config PCI_DRA7XX
> +	bool "TI DRA7xx PCIe controller"
> +	select PCIE_DW
> +	depends on OF || HAS_IOMEM || TI_PIPE3

I think you mean &&, not || here.

> +static inline u32 x(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void dra7xx_pcie_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);
> +}

These don't actually seem to add any value, you need more characters
to call the inline function than to open-code it.

> +static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
> +{
> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> +
> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
> +			   ~INTERRUPTS);
> +	dra7xx_pcie_writel(dra7xx->base,
> +			   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, INTERRUPTS);
> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
> +			   ~LEG_EP_INTERRUPTS & ~MSI);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		dra7xx_pcie_writel(dra7xx->base,
> +				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI, MSI);
> +	else
> +		dra7xx_pcie_writel(dra7xx->base,
> +				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
> +				   LEG_EP_INTERRUPTS);

Doesn't this just enable one or the other? In general I'd assume you need
both INTx and MSI, at least if MSI is available.

It probably doesn't hurt to always turn them all on.

> +static int add_pcie_port(struct dra7xx_pcie *dra7xx,
> +			  struct platform_device *pdev)
> +{
> +	int ret;
> +	struct pcie_port *pp;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +
> +	pp = &dra7xx->pp;
> +	pp->dev = dev;
> +	pp->ops = &dra7xx_pcie_host_ops;
> +
> +	spin_lock_init(&pp->conf_lock);
> +
> +	pp->irq = platform_get_irq(pdev, 1);
> +	if (pp->irq < 0) {
> +		dev_err(dev, "missing IRQ resource\n");
> +		return -EINVAL;
> +	}
>

The binding does not list a mandatory "interrupts" property, so
this should not be treated as an error.

> +static int __init dra7xx_pcie_probe(struct platform_device *pdev)
> +{
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "missing IRQ resource\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, dra7xx_pcie_irq_handler,
> +			       IRQF_SHARED, "dra7xx-pcie-main", dra7xx);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq\n");
> +		return ret;
> +	}

Same here.

> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ti_conf");
> +	base = devm_ioremap_nocache(dev, res->start, resource_size(res));

Just use devm_ioremap() instead of devm_ioremap_nocache(). The second
one is just there for historic reasons, and they always do the same
thing.


	Arnd

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Kishon Vijay Abraham I]

Hi,

On Tuesday 06 May 2014 07:24 PM, Arnd Bergmann wrote:
> On Tuesday 06 May 2014 19:03:51 Kishon Vijay Abraham I wrote:
>> Added support for pcie controller in dra7xx. This driver re-uses
>> the designware core code that is already present in kernel.
>>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Mohit Kumar <mohit.kumar@st.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> Looks pretty good overall, just a few details I noticed:
> 
>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> new file mode 100644
>> index 0000000..6cb6f09
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> @@ -0,0 +1,33 @@
>> +TI PCI Controllers
>> +
>> +PCIe Designware Controller
>> +This node should have the properties described in "designware-pcie.txt".
>> + - compatible: Should be "ti,dra7xx-pcie""
> 
> No "xx" in compatible strings please. Just make name this after the first
> chip to use this particular interface.

ok.
> 
>> + - reg : Address and length of the register set for the device.
>> + - reg-names : "ti_conf" for the TI specific registers and rc_dbics for the
>> +   "designware" registers.
> 
> The description uses inconsistent quotation marks. You should also have
> a fixed order in the binding, such as
> 
>  - reg : Two register ranges as listed in the reg-names property
>  - reg-names : The first entry must be "ti-conf" for the TI specific registers
> 	       The second entry must be "rc-dbics" for the designware pcie registers.

ok, looks better.
> 
>> + - phys : the phandle for the PHY device (used by generic PHY framework)
>> + - phy-names : the names of the PHY corresponding to the PHYs present in the
>> +   *phy* phandle.
> 
> It's not just a phandle, it can be any phy specifier including additional
> argument cells.
> 
> The second line should just read
> 
>  - phy-names : must be "pcie-phy"

will fix this.
> 
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index a6f67ec..7be6393 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -1,6 +1,16 @@
>>  menu "PCI host controller drivers"
>>  	depends on PCI
>>  
>> +config PCI_DRA7XX
>> +	bool "TI DRA7xx PCIe controller"
>> +	select PCIE_DW
>> +	depends on OF || HAS_IOMEM || TI_PIPE3
> 
> I think you mean &&, not || here.

ah.. indeed.
> 
>> +static inline u32 x(void __iomem *base, u32 offset)
>> +{
>> +	return readl(base + offset);
>> +}
>> +
>> +static inline void dra7xx_pcie_writel(void __iomem *base, u32 offset, u32 value)
>> +{
>> +	writel(value, base + offset);
>> +}
> 
> These don't actually seem to add any value, you need more characters
> to call the inline function than to open-code it.
> 
>> +static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
>> +{
>> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
>> +
>> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
>> +			   ~INTERRUPTS);
>> +	dra7xx_pcie_writel(dra7xx->base,
>> +			   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, INTERRUPTS);
>> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
>> +			   ~LEG_EP_INTERRUPTS & ~MSI);
>> +
>> +	if (IS_ENABLED(CONFIG_PCI_MSI))
>> +		dra7xx_pcie_writel(dra7xx->base,
>> +				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI, MSI);
>> +	else
>> +		dra7xx_pcie_writel(dra7xx->base,
>> +				   PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
>> +				   LEG_EP_INTERRUPTS);
> 
> Doesn't this just enable one or the other? In general I'd assume you need
> both INTx and MSI, at least if MSI is available.

Not sure since the programming sequence in the TRM explicitly states either
legacy interrupts or MSI interrupts should be enabled but not both.
> 
> It probably doesn't hurt to always turn them all on.
> 
>> +static int add_pcie_port(struct dra7xx_pcie *dra7xx,
>> +			  struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct pcie_port *pp;
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	pp = &dra7xx->pp;
>> +	pp->dev = dev;
>> +	pp->ops = &dra7xx_pcie_host_ops;
>> +
>> +	spin_lock_init(&pp->conf_lock);
>> +
>> +	pp->irq = platform_get_irq(pdev, 1);
>> +	if (pp->irq < 0) {
>> +		dev_err(dev, "missing IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>>
> 
> The binding does not list a mandatory "interrupts" property, so
> this should not be treated as an error.

actually the 'interrupts' property is documented in pci/designware-pcie.txt.
> 
>> +static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>> +{
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, irq, dra7xx_pcie_irq_handler,
>> +			       IRQF_SHARED, "dra7xx-pcie-main", dra7xx);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq\n");
>> +		return ret;
>> +	}
> 
> Same here.
> 
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ti_conf");
>> +	base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> 
> Just use devm_ioremap() instead of devm_ioremap_nocache(). The second
> one is just there for historic reasons, and they always do the same
> thing.

Ok.

Thanks
Kishon

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Arnd Bergmann]

On Wednesday 07 May 2014 14:14:55 Kishon Vijay Abraham I wrote:
> >> +static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
> >> +{
> >> +    struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> >> +
> >> +    dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
> >> +                       ~INTERRUPTS);
> >> +    dra7xx_pcie_writel(dra7xx->base,
> >> +                       PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, INTERRUPTS);
> >> +    dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
> >> +                       ~LEG_EP_INTERRUPTS & ~MSI);
> >> +
> >> +    if (IS_ENABLED(CONFIG_PCI_MSI))
> >> +            dra7xx_pcie_writel(dra7xx->base,
> >> +                               PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI, MSI);
> >> +    else
> >> +            dra7xx_pcie_writel(dra7xx->base,
> >> +                               PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
> >> +                               LEG_EP_INTERRUPTS);
> > 
> > Doesn't this just enable one or the other? In general I'd assume you need
> > both INTx and MSI, at least if MSI is available.
> 
> Not sure since the programming sequence in the TRM explicitly states either
> legacy interrupts or MSI interrupts should be enabled but not both.

Hmm, I think that means you can't have MSI at all. You have to support
legacy PCI devices that can't do MSI.

Do you know if you have a modern GIC implementation with MSI support
in these SoCs? It would be better anyway to use the GIC for doing
MSI, so you can just ignore the internal MSI controller here.

> >> +static int add_pcie_port(struct dra7xx_pcie *dra7xx,
> >> +                      struct platform_device *pdev)
> >> +{
> >> +    int ret;
> >> +    struct pcie_port *pp;
> >> +    struct resource *res;
> >> +    struct device *dev = &pdev->dev;
> >> +
> >> +    pp = &dra7xx->pp;
> >> +    pp->dev = dev;
> >> +    pp->ops = &dra7xx_pcie_host_ops;
> >> +
> >> +    spin_lock_init(&pp->conf_lock);
> >> +
> >> +    pp->irq = platform_get_irq(pdev, 1);
> >> +    if (pp->irq < 0) {
> >> +            dev_err(dev, "missing IRQ resource\n");
> >> +            return -EINVAL;
> >> +    }
> >>
> > 
> > The binding does not list a mandatory "interrupts" property, so
> > this should not be treated as an error.
> 
> actually the 'interrupts' property is documented in pci/designware-pcie.txt.

Hmm, but you don't seem to use it the same way as documented there.
I'm not sure what 'level interrupt, pulse interrupt, special interrupt'
in the parent binding are, but they don't seem to be the ones you use
here.

	Arnd

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Kishon Vijay Abraham I]

Hi Arnd,

On Wednesday 07 May 2014 03:00 PM, Arnd Bergmann wrote:
> On Wednesday 07 May 2014 14:14:55 Kishon Vijay Abraham I wrote:
>>>> +static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
>>>> +{
>>>> +    struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
>>>> +
>>>> +    dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN,
>>>> +                       ~INTERRUPTS);
>>>> +    dra7xx_pcie_writel(dra7xx->base,
>>>> +                       PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, INTERRUPTS);
>>>> +    dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI,
>>>> +                       ~LEG_EP_INTERRUPTS & ~MSI);
>>>> +
>>>> +    if (IS_ENABLED(CONFIG_PCI_MSI))
>>>> +            dra7xx_pcie_writel(dra7xx->base,
>>>> +                               PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI, MSI);
>>>> +    else
>>>> +            dra7xx_pcie_writel(dra7xx->base,
>>>> +                               PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI,
>>>> +                               LEG_EP_INTERRUPTS);
>>>
>>> Doesn't this just enable one or the other? In general I'd assume you need
>>> both INTx and MSI, at least if MSI is available.
>>
>> Not sure since the programming sequence in the TRM explicitly states either
>> legacy interrupts or MSI interrupts should be enabled but not both.
> 
> Hmm, I think that means you can't have MSI at all. You have to support
> legacy PCI devices that can't do MSI.
> 
> Do you know if you have a modern GIC implementation with MSI support
> in these SoCs? It would be better anyway to use the GIC for doing

In DRA7 it is not there. I'm not sure in other platforms.
> MSI, so you can just ignore the internal MSI controller here.
> 
>>>> +static int add_pcie_port(struct dra7xx_pcie *dra7xx,
>>>> +                      struct platform_device *pdev)
>>>> +{
>>>> +    int ret;
>>>> +    struct pcie_port *pp;
>>>> +    struct resource *res;
>>>> +    struct device *dev = &pdev->dev;
>>>> +
>>>> +    pp = &dra7xx->pp;
>>>> +    pp->dev = dev;
>>>> +    pp->ops = &dra7xx_pcie_host_ops;
>>>> +
>>>> +    spin_lock_init(&pp->conf_lock);
>>>> +
>>>> +    pp->irq = platform_get_irq(pdev, 1);
>>>> +    if (pp->irq < 0) {
>>>> +            dev_err(dev, "missing IRQ resource\n");
>>>> +            return -EINVAL;
>>>> +    }
>>>>
>>>
>>> The binding does not list a mandatory "interrupts" property, so
>>> this should not be treated as an error.
>>
>> actually the 'interrupts' property is documented in pci/designware-pcie.txt.
> 
> Hmm, but you don't seem to use it the same way as documented there.
> I'm not sure what 'level interrupt, pulse interrupt, special interrupt'
> in the parent binding are, but they don't seem to be the ones you use
> here.

Yeah. I'll update my Documentation. Thanks for pointing this out.

Thanks
Kishon

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Jason Gunthorpe]

On Tue, May 06, 2014 at 07:03:51PM +0530, Kishon Vijay Abraham I wrote:
> +Example:
> +pcie@51000000 {
> +	compatible = "ti,dra7xx-pcie";
> +	reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
> +	reg-names = "ti_conf", "rc_dbics";
> +	interrupts = <0 232 0x4>, <0 233 0x4>;
> +	#address-cells = <3>;
> +	#size-cells = <2>;
> +	device_type = "pci";
> +	ti,device_type = <3>;
> +	ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */

Configuration space should not show up in the ranges, please don't
copy that mistake from other drivers, put it in reg.

> +	interrupt-map-mask = <0 0 0 0>;
> +	interrupt-map = <0x0 0 &gic 134>;

The HW cannot decode INTA/B/C/D?

> +#define	PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI		0x0034
> +#define	PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI		0x0038
> +#define	INTA						BIT(0)
> +#define	INTB						BIT(1)
> +#define	INTC						BIT(2)
> +#define	INTD						BIT(3)
> +#define	MSI						BIT(4)
> +#define	LEG_EP_INTERRUPTS (INTA | INTB | INTC | INTD)

Oh, it can, it would be wise to export this from the driver. Look at
the latest patches from Srikanth Thokala for the Xilinx PCI driver to
see how this should look

> +static int dra7xx_pcie_establish_link(struct pcie_port *pp)
> +{
> +	u32 reg;
> +	int retries = 1000;
> +	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> +
> +	if (dw_pcie_link_up(pp)) {
> +		dev_err(pp->dev, "link is already up\n");
> +		return 0;
> +	}
> +
> +	reg = dra7xx_pcie_readl(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD);
> +	reg |= LTSSM_EN;
> +	dra7xx_pcie_writel(dra7xx->base, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg);
> +
> +	while (--retries) {
> +		reg = dra7xx_pcie_readl(dra7xx->base,
> +					PCIECTRL_DRA7XX_CONF_PHY_CS);
> +		if (reg & LINK_UP)
> +			break;
> +		usleep_range(10, 20);
> +	}
> +
> +	if (retries <= 0) {
> +		dev_err(pp->dev, "link is not up\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

It would be really nice to see the link bring up process live in the
PCI core, every driver seems to have its own take on this.

The PCI-E spec requires a 100ms delay after link bring up (aka hot
reset) before sending any configuration TLPs.

Jason

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Kishon Vijay Abraham I]

Hi,

On Tuesday 06 May 2014 10:05 PM, Jason Gunthorpe wrote:
> On Tue, May 06, 2014 at 07:03:51PM +0530, Kishon Vijay Abraham I wrote:
>> +Example:
>> +pcie@51000000 {
>> +	compatible = "ti,dra7xx-pcie";
>> +	reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
>> +	reg-names = "ti_conf", "rc_dbics";
>> +	interrupts = <0 232 0x4>, <0 233 0x4>;
>> +	#address-cells = <3>;
>> +	#size-cells = <2>;
>> +	device_type = "pci";
>> +	ti,device_type = <3>;
>> +	ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */
> 
> Configuration space should not show up in the ranges, please don't
> copy that mistake from other drivers, put it in reg.

But then it needs pcie-designware.c to be modified and it will be breaking
other platforms no?
> 
>> +	interrupt-map-mask = <0 0 0 0>;
>> +	interrupt-map = <0x0 0 &gic 134>;
> 
> The HW cannot decode INTA/B/C/D?
> 
>> +#define	PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI		0x0034
>> +#define	PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MSI		0x0038
>> +#define	INTA						BIT(0)
>> +#define	INTB						BIT(1)
>> +#define	INTC						BIT(2)
>> +#define	INTD						BIT(3)
>> +#define	MSI						BIT(4)
>> +#define	LEG_EP_INTERRUPTS (INTA | INTB | INTC | INTD)
> 
> Oh, it can, it would be wise to export this from the driver. Look at
> the latest patches from Srikanth Thokala for the Xilinx PCI driver to
> see how this should look

ok.. will have a look at it.

Thanks
Kishon

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Arnd Bergmann]

On Wednesday 07 May 2014 14:52:47 Kishon Vijay Abraham I wrote:
> On Tuesday 06 May 2014 10:05 PM, Jason Gunthorpe wrote:
> > On Tue, May 06, 2014 at 07:03:51PM +0530, Kishon Vijay Abraham I wrote:
> >> +Example:
> >> +pcie@51000000 {
> >> +    compatible = "ti,dra7xx-pcie";
> >> +    reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
> >> +    reg-names = "ti_conf", "rc_dbics";
> >> +    interrupts = <0 232 0x4>, <0 233 0x4>;
> >> +    #address-cells = >;
> >> +    #size-cells = <2>;
> >> +    device_type = "pci";
> >> +    ti,device_type = >;
> >> +    ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */
> > 
> > Configuration space should not show up in the ranges, please don't
> > copy that mistake from other drivers, put it in reg.
> 
> But then it needs pcie-designware.c to be modified and it will be breaking
> other platforms no?

I think the pcie-designware driver should be changed to allow either way.
Ideally we would deprecate the existing method in a way that for new front-ends
it doesn't work, but the old front-ends can still deal with it but also work
if you put it into the reg property.

	Arnd

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Jingoo Han]

On Wednesday, May 07, 2014 6:26 PM, Arnd Bergmann wrote:
> On Wednesday 07 May 2014 14:52:47 Kishon Vijay Abraham I wrote:
> > On Tuesday 06 May 2014 10:05 PM, Jason Gunthorpe wrote:
> > > On Tue, May 06, 2014 at 07:03:51PM +0530, Kishon Vijay Abraham I wrote:
> > >> +Example:
> > >> +pcie@51000000 {
> > >> +    compatible = "ti,dra7xx-pcie";
> > >> +    reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
> > >> +    reg-names = "ti_conf", "rc_dbics";
> > >> +    interrupts = <0 232 0x4>, <0 233 0x4>;
> > >> +    #address-cells = >;
> > >> +    #size-cells = <2>;
> > >> +    device_type = "pci";
> > >> +    ti,device_type = >;
> > >> +    ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */
> > >
> > > Configuration space should not show up in the ranges, please don't
> > > copy that mistake from other drivers, put it in reg.
> >
> > But then it needs pcie-designware.c to be modified and it will be breaking
> > other platforms no?
> 
> I think the pcie-designware driver should be changed to allow either way.
> Ideally we would deprecate the existing method in a way that for new front-ends
> it doesn't work, but the old front-ends can still deal with it but also work
> if you put it into the reg property.

(+cc Pratyush Anand, Thierry Reding)

Hi Arnd,

Thank you for your comment.
Do you mean the case of Tegra PCIe as below?

./arch/arm/boot/dts/tegra20.dts
	pcie-controller@80003000 {
		...
		reg = <0x80003000 0x00000800   /* PADS registers */
			 0x80003800 0x00000200   /* AFI registers */
			 0x90000000 0x10000000>; /* configuration space */
		...
		ranges = <0x82000000 0 0x80000000 0x80000000 0 0x00001000   /* port 0 registers */
			0x82000000 0 0x80001000 0x80001000 0 0x00001000   /* port 1 registers */
			0x81000000 0 0          0x82000000 0 0x00010000   /* downstream I/O */
			0x82000000 0 0xa0000000 0xa0000000 0 0x08000000   /* non-prefetchable memory */
			0xc2000000 0 0xa8000000 0xa8000000 0 0x18000000>; /* prefetchable memory */
		...

./drivers/pci/host/pci-tegra.c
	/* request configuration space, but remap later, on demand */
	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs");
	...
	pcie->cs = devm_request_mem_region(pcie->dev, res->start,
						resource_size(res), res->name);


Best regards,
Jingoo Han

Re: [PATCH 05/17] pci: host: pcie-dra7xx: add support for pcie-dra7xx controller

[Arnd Bergmann]

On Thursday 08 May 2014 17:56:38 Jingoo Han wrote:
> On Wednesday, May 07, 2014 6:26 PM, Arnd Bergmann wrote:
> > On Wednesday 07 May 2014 14:52:47 Kishon Vijay Abraham I wrote:
> > > On Tuesday 06 May 2014 10:05 PM, Jason Gunthorpe wrote:
> > > > On Tue, May 06, 2014 at 07:03:51PM +0530, Kishon Vijay Abraham I wrote:
> > > >> +Example:
> > > >> +pcie@51000000 {
> > > >> +    compatible = "ti,dra7xx-pcie";
> > > >> +    reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
> > > >> +    reg-names = "ti_conf", "rc_dbics";
> > > >> +    interrupts = <0 232 0x4>, <0 233 0x4>;
> > > >> +    #address-cells = >;
> > > >> +    #size-cells = <2>;
> > > >> +    device_type = "pci";
> > > >> +    ti,device_type = >;
> > > >> +    ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* Configuration Space */
> > > >
> > > > Configuration space should not show up in the ranges, please don't
> > > > copy that mistake from other drivers, put it in reg.
> > >
> > > But then it needs pcie-designware.c to be modified and it will be breaking
> > > other platforms no?
> > 
> > I think the pcie-designware driver should be changed to allow either way.
> > Ideally we would deprecate the existing method in a way that for new front-ends
> > it doesn't work, but the old front-ends can still deal with it but also work
> > if you put it into the reg property.
> 
> (+cc Pratyush Anand, Thierry Reding)
> 
> Hi Arnd,
> 
> Thank you for your comment.
> Do you mean the case of Tegra PCIe as below?
> 
> ./arch/arm/boot/dts/tegra20.dts
>         pcie-controller@80003000 {
>                 ...
>                 reg = <0x80003000 0x00000800   /* PADS registers */
>                          0x80003800 0x00000200   /* AFI registers */
>                          0x90000000 0x10000000>; /* configuration space */
>                 ...
>                 ranges = <0x82000000 0 0x80000000 0x80000000 0 0x00001000   /* port 0 registers */
>                         0x82000000 0 0x80001000 0x80001000 0 0x00001000   /* port 1 registers */
>                         0x81000000 0 0          0x82000000 0 0x00010000   /* downstream I/O */
>                         0x82000000 0 0xa0000000 0xa0000000 0 0x08000000   /* non-prefetchable memory */
>                         0xc2000000 0 0xa8000000 0xa8000000 0 0x18000000>; /* prefetchable memory */
>                 ...
> 
> ./drivers/pci/host/pci-tegra.c
>         /* request configuration space, but remap later, on demand */
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs");
>         ...
>         pcie->cs = devm_request_mem_region(pcie->dev, res->start,
>                                                 resource_size(res), res->name);

Yes, that is how the config space should be handled normally.

	Arnd

[PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
address. So whenever the cpu issues a read/write request, the 4 most
significant bits are used by L3 to determine the target controller.
For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
the outbound translation window the *base* should be programmed as 0x000_0000.
Whenever we try to write to say 0x2000_0000, it will be translated to whatever
we have programmed in the translation window with base as 0x000_0000.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Marek Vasut <marex@denx.de>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Acked-by: Mohit Kumar <mohit.kumar@st.com>
---
 .../devicetree/bindings/pci/designware-pcie.txt    |    1 +
 drivers/pci/host/pcie-designware.c                 |   39 ++++++++++++++------
 drivers/pci/host/pcie-designware.h                 |    1 +
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index d6fae13..c574dd3 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -27,6 +27,7 @@ Optional properties for fsl,imx6q-pcie
 - power-on-gpio: gpio pin number of power-enable signal
 - wake-up-gpio: gpio pin number of incoming wakeup signal
 - disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal
+- base-mask: address mask for the PCIe controller target port
 
 Example:
 
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index c4e3732..243f148 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -463,6 +463,9 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 		return -EINVAL;
 	}
 
+	if (of_property_read_u64(np, "base-mask", &pp->base_mask))
+		pp->base_mask = ~0x0ULL;
+
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
 					MAX_MSI_IRQS, &msi_domain_ops,
@@ -502,12 +505,15 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 
 static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
 {
+	u64 cfg0_base;
+
+	cfg0_base = pp->cfg0_base & pp->base_mask;
 	/* Program viewport 0 : OUTBOUND : CFG0 */
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
 			  PCIE_ATU_VIEWPORT);
-	dw_pcie_writel_rc(pp, pp->cfg0_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->cfg0_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->cfg0_base + pp->config.cfg0_size - 1,
+	dw_pcie_writel_rc(pp, cfg0_base, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (cfg0_base >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, cfg0_base + pp->config.cfg0_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
@@ -517,13 +523,16 @@ static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
 
 static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
 {
+	u64 cfg1_base;
+
+	cfg1_base = pp->cfg1_base & pp->base_mask;
 	/* Program viewport 1 : OUTBOUND : CFG1 */
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_CFG1, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, pp->cfg1_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->cfg1_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->cfg1_base + pp->config.cfg1_size - 1,
+	dw_pcie_writel_rc(pp, cfg1_base, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (cfg1_base >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, cfg1_base + pp->config.cfg1_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, busdev, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, 0, PCIE_ATU_UPPER_TARGET);
@@ -532,13 +541,16 @@ static void dw_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
 
 static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
 {
+	u64 mem_base;
+
+	mem_base = pp->mem_base & pp->base_mask;
 	/* Program viewport 0 : OUTBOUND : MEM */
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, pp->mem_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->mem_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->mem_base + pp->config.mem_size - 1,
+	dw_pcie_writel_rc(pp, mem_base, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (mem_base >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, mem_base + pp->config.mem_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, pp->config.mem_bus_addr, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.mem_bus_addr),
@@ -548,13 +560,16 @@ static void dw_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
 
 static void dw_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
 {
+	u64 io_base;
+
+	io_base = pp->io_base & pp->base_mask;
 	/* Program viewport 1 : OUTBOUND : IO */
 	dw_pcie_writel_rc(pp, PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1,
 			  PCIE_ATU_VIEWPORT);
 	dw_pcie_writel_rc(pp, PCIE_ATU_TYPE_IO, PCIE_ATU_CR1);
-	dw_pcie_writel_rc(pp, pp->io_base, PCIE_ATU_LOWER_BASE);
-	dw_pcie_writel_rc(pp, (pp->io_base >> 32), PCIE_ATU_UPPER_BASE);
-	dw_pcie_writel_rc(pp, pp->io_base + pp->config.io_size - 1,
+	dw_pcie_writel_rc(pp, io_base, PCIE_ATU_LOWER_BASE);
+	dw_pcie_writel_rc(pp, (io_base >> 32), PCIE_ATU_UPPER_BASE);
+	dw_pcie_writel_rc(pp, io_base + pp->config.io_size - 1,
 			  PCIE_ATU_LIMIT);
 	dw_pcie_writel_rc(pp, pp->config.io_bus_addr, PCIE_ATU_LOWER_TARGET);
 	dw_pcie_writel_rc(pp, upper_32_bits(pp->config.io_bus_addr),
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 3063b35..3fa12a6 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -35,6 +35,7 @@ struct pcie_port {
 	struct device		*dev;
 	u8			root_bus_nr;
 	void __iomem		*dbi_base;
+	u64			base_mask;
 	u64			cfg0_base;
 	void __iomem		*va_cfg0_base;
 	u64			cfg1_base;
-- 
1.7.9.5

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> address. So whenever the cpu issues a read/write request, the 4 most
> significant bits are used by L3 to determine the target controller.
> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> the outbound translation window the *base* should be programmed as 0x000_0000.
> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> we have programmed in the translation window with base as 0x000_0000.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Marek Vasut <marex@denx.de>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Acked-by: Mohit Kumar <mohit.kumar@st.com>

Sorry, but NAK.

We have a standard 'dma-ranges' property to handle this, so use it.

See the x-gene PCIe driver patches for an example. Please also talk
to Santosh about it, as he is implementing generic support for
parsing dma-ranges in platform devices at the moment.

I also suspect you will have to implement swiotlb support to make
generic PCI devices work behind this bridge. Otherwise you end up
with random physical addresses passed into DMA registers.

	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Jingoo Han]

On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
> > In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> > address. So whenever the cpu issues a read/write request, the 4 most
> > significant bits are used by L3 to determine the target controller.
> > For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> > the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> > the outbound translation window the *base* should be programmed as 0x000_0000.
> > Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> > we have programmed in the translation window with base as 0x000_0000.
> >
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > Acked-by: Jingoo Han <jg1.han@samsung.com>
> > Acked-by: Mohit Kumar <mohit.kumar@st.com>
> 
> Sorry, but NAK.
> 
> We have a standard 'dma-ranges' property to handle this, so use it.
> 
> See the x-gene PCIe driver patches for an example. Please also talk
> to Santosh about it, as he is implementing generic support for
> parsing dma-ranges in platform devices at the moment.

Hi Arnd,

Do you mean the following patch?
http://www.spinics.net/lists/kernel/msg1737725.html

Thank you.

Best regards,
Jingoo Han

> 
> I also suspect you will have to implement swiotlb support to make
> generic PCI devices work behind this bridge. Otherwise you end up
> with random physical addresses passed into DMA registers.

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
> > On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
> > > In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> > > address. So whenever the cpu issues a read/write request, the 4 most
> > > significant bits are used by L3 to determine the target controller.
> > > For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> > > the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> > > the outbound translation window the *base* should be programmed as 0x000_0000.
> > > Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> > > we have programmed in the translation window with base as 0x000_0000.
> > >
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > > Acked-by: Jingoo Han <jg1.han@samsung.com>
> > > Acked-by: Mohit Kumar <mohit.kumar@st.com>
> > 
> > Sorry, but NAK.
> > 
> > We have a standard 'dma-ranges' property to handle this, so use it.
> > 
> > See the x-gene PCIe driver patches for an example. Please also talk
> > to Santosh about it, as he is implementing generic support for
> > parsing dma-ranges in platform devices at the moment.
> 
> Hi Arnd,
> 
> Do you mean the following patch?
> http://www.spinics.net/lists/kernel/msg1737725.html
> 

That is the patch Santosh did for platform devices, which is related but not
what I meant here. For the PCI inbound window setup, please have a look
at https://lkml.org/lkml/2014/3/19/607

	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

Hi,

On Thursday 08 May 2014 02:48 PM, Arnd Bergmann wrote:
> On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
>> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
>>> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
>>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>>>> address. So whenever the cpu issues a read/write request, the 4 most
>>>> significant bits are used by L3 to determine the target controller.
>>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>>>> the outbound translation window the *base* should be programmed as 0x000_0000.
>>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>>>> we have programmed in the translation window with base as 0x000_0000.
>>>>
>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>>>> Acked-by: Mohit Kumar <mohit.kumar@st.com>
>>>
>>> Sorry, but NAK.
>>>
>>> We have a standard 'dma-ranges' property to handle this, so use it.
>>>
>>> See the x-gene PCIe driver patches for an example. Please also talk
>>> to Santosh about it, as he is implementing generic support for
>>> parsing dma-ranges in platform devices at the moment.
>>
>> Hi Arnd,
>>
>> Do you mean the following patch?
>> http://www.spinics.net/lists/kernel/msg1737725.html
>>
> 
> That is the patch Santosh did for platform devices, which is related but not
> what I meant here. For the PCI inbound window setup, please have a look
> at https://lkml.org/lkml/2014/3/19/607

For some reason lkml is not showing any contents. Do you have a different link?

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Jingoo Han]

On Friday, May 09, 2014 8:50 PM, Kishon Vijay Abraham I wrote:
> On Thursday 08 May 2014 02:48 PM, Arnd Bergmann wrote:
> > On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
> >> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
> >>> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
> >>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> >>>> address. So whenever the cpu issues a read/write request, the 4 most
> >>>> significant bits are used by L3 to determine the target controller.
> >>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> >>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> >>>> the outbound translation window the *base* should be programmed as 0x000_0000.
> >>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> >>>> we have programmed in the translation window with base as 0x000_0000.
> >>>>
> >>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >>>> Cc: Marek Vasut <marex@denx.de>
> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>> Acked-by: Jingoo Han <jg1.han@samsung.com>
> >>>> Acked-by: Mohit Kumar <mohit.kumar@st.com>
> >>>
> >>> Sorry, but NAK.
> >>>
> >>> We have a standard 'dma-ranges' property to handle this, so use it.
> >>>
> >>> See the x-gene PCIe driver patches for an example. Please also talk
> >>> to Santosh about it, as he is implementing generic support for
> >>> parsing dma-ranges in platform devices at the moment.
> >>
> >> Hi Arnd,
> >>
> >> Do you mean the following patch?
> >> http://www.spinics.net/lists/kernel/msg1737725.html
> >>
> >
> > That is the patch Santosh did for platform devices, which is related but not
> > what I meant here. For the PCI inbound window setup, please have a look
> > at https://lkml.org/lkml/2014/3/19/607
> 
> For some reason lkml is not showing any contents. Do you have a different link?

Hi Kishon,

Please refer to the following link. :-)
http://www.spinics.net/lists/linux-pci/msg29855.html


Best regards,
Jingoo Han

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

Hi Arnd,

On Thursday 08 May 2014 02:48 PM, Arnd Bergmann wrote:
> On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
>> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
>>> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
>>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>>>> address. So whenever the cpu issues a read/write request, the 4 most
>>>> significant bits are used by L3 to determine the target controller.
>>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>>>> the outbound translation window the *base* should be programmed as 0x000_0000.
>>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>>>> we have programmed in the translation window with base as 0x000_0000.
>>>>
>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>>>> Acked-by: Mohit Kumar <mohit.kumar@st.com>
>>>
>>> Sorry, but NAK.
>>>
>>> We have a standard 'dma-ranges' property to handle this, so use it.
>>>
>>> See the x-gene PCIe driver patches for an example. Please also talk
>>> to Santosh about it, as he is implementing generic support for
>>> parsing dma-ranges in platform devices at the moment.
>>
>> Hi Arnd,
>>
>> Do you mean the following patch?
>> http://www.spinics.net/lists/kernel/msg1737725.html
>>
> 
> That is the patch Santosh did for platform devices, which is related but not
> what I meant here. For the PCI inbound window setup, please have a look
> at https://lkml.org/lkml/2014/3/19/607

Do you think it can be used for *outbound* window setup too? The problem is the
*ranges* property defines both the pci address and cpu address which should
have been enough to program the ob translation window, but the hw is designed
so that the controller sees only the 28 bits. (The most significant 4 bits is
for the l3 to address the controller).

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Tuesday 13 May 2014 18:01:59 Kishon Vijay Abraham I wrote:
> On Thursday 08 May 2014 02:48 PM, Arnd Bergmann wrote:
> > On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
> >> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
> >>> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
> >>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
> >>>> address. So whenever the cpu issues a read/write request, the 4 most
> >>>> significant bits are used by L3 to determine the target controller.
> >>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
> >>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
> >>>> the outbound translation window the *base* should be programmed as 0x000_0000.
> >>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
> >>>> we have programmed in the translation window with base as 0x000_0000.
> >>>>
> >>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >>>> Cc: Marek Vasut <marex@denx.de>
> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>> Acked-by: Jingoo Han <jg1.han@samsung.com>
> >>>> Acked-by: Mohit Kumar <mohit.kumar@st.com>
> >>>
> >>> Sorry, but NAK.
> >>>
> >>> We have a standard 'dma-ranges' property to handle this, so use it.
> >>>
> >>> See the x-gene PCIe driver patches for an example. Please also talk
> >>> to Santosh about it, as he is implementing generic support for
> >>> parsing dma-ranges in platform devices at the moment.
> >>
> >> Hi Arnd,
> >>
> >> Do you mean the following patch?
> >> http://www.spinics.net/lists/kernel/msg1737725.html
> >>
> > 
> > That is the patch Santosh did for platform devices, which is related but not
> > what I meant here. For the PCI inbound window setup, please have a look
> > at https://lkml.org/lkml/2014/3/19/607
> 
> Do you think it can be used for *outbound* window setup too? The problem is the
> *ranges* property defines both the pci address and cpu address which should
> have been enough to program the ob translation window, but the hw is designed
> so that the controller sees only the 28 bits. (The most significant 4 bits is
> for the l3 to address the controller).

I'm not following what the problem is. You should always be able to describe
in the inbound window (that is from the CPU perspective) using dma-ranges
and the outbound window using ranges.

If you have a case where the outbound translation is a 256MB (i.e. 28bit)
section of the CPU address space, that could be represented as

	ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;

or 

	ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;

depending on whether you want the BARs to be programmed using a low
address 0x0-0x0fffffff or an address matching the window
0xb0000000-0xbfffffff.

	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

Hi Arnd,

On Tuesday 13 May 2014 06:17 PM, Arnd Bergmann wrote:
> On Tuesday 13 May 2014 18:01:59 Kishon Vijay Abraham I wrote:
>> On Thursday 08 May 2014 02:48 PM, Arnd Bergmann wrote:
>>> On Thursday 08 May 2014 18:05:11 Jingoo Han wrote:
>>>> On Tuesday, May 06, 2014 10:59 PM, Arnd Bergmann wrote:
>>>>> On Tuesday 06 May 2014 19:03:52 Kishon Vijay Abraham I wrote:
>>>>>> In DRA7, the cpu sees 32bit address, but the pcie controller can see only 28bit
>>>>>> address. So whenever the cpu issues a read/write request, the 4 most
>>>>>> significant bits are used by L3 to determine the target controller.
>>>>>> For example, the cpu reserves 0x2000_0000 - 0x2FFF_FFFF for PCIe controller but
>>>>>> the PCIe controller will see only (0x000_0000 - 0xFFF_FFF). So for programming
>>>>>> the outbound translation window the *base* should be programmed as 0x000_0000.
>>>>>> Whenever we try to write to say 0x2000_0000, it will be translated to whatever
>>>>>> we have programmed in the translation window with base as 0x000_0000.
>>>>>>
>>>>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>>>>>> Acked-by: Mohit Kumar <mohit.kumar@st.com>
>>>>>
>>>>> Sorry, but NAK.
>>>>>
>>>>> We have a standard 'dma-ranges' property to handle this, so use it.
>>>>>
>>>>> See the x-gene PCIe driver patches for an example. Please also talk
>>>>> to Santosh about it, as he is implementing generic support for
>>>>> parsing dma-ranges in platform devices at the moment.
>>>>
>>>> Hi Arnd,
>>>>
>>>> Do you mean the following patch?
>>>> http://www.spinics.net/lists/kernel/msg1737725.html
>>>>
>>>
>>> That is the patch Santosh did for platform devices, which is related but not
>>> what I meant here. For the PCI inbound window setup, please have a look
>>> at https://lkml.org/lkml/2014/3/19/607
>>
>> Do you think it can be used for *outbound* window setup too? The problem is the
>> *ranges* property defines both the pci address and cpu address which should
>> have been enough to program the ob translation window, but the hw is designed
>> so that the controller sees only the 28 bits. (The most significant 4 bits is
>> for the l3 to address the controller).
> 
> I'm not following what the problem is. You should always be able to describe
> in the inbound window (that is from the CPU perspective) using dma-ranges
> and the outbound window using ranges.
> 
> If you have a case where the outbound translation is a 256MB (i.e. 28bit)
> section of the CPU address space, that could be represented as
> 
> 	ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
> 
> or 
> 
> 	ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
> 
> depending on whether you want the BARs to be programmed using a low
> address 0x0-0x0fffffff or an address matching the window
> 0xb0000000-0xbfffffff.

The problem is, for configuring the window starting at 0xb0000000, the ATU
should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
> > If you have a case where the outbound translation is a 256MB (i.e. 28bit)
> > section of the CPU address space, that could be represented as
> > 
> >       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
> > 
> > or 
> > 
> >       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
> > 
> > depending on whether you want the BARs to be programmed using a low
> > address 0x0-0x0fffffff or an address matching the window
> > 0xb0000000-0xbfffffff.
> 
> The problem is, for configuring the window starting at 0xb0000000, the ATU
> should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
> 

Then use the first of the two?

	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Tuesday 13 May 2014 15:27:46 Arnd Bergmann wrote:
> On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
> > > If you have a case where the outbound translation is a 256MB (i.e. 28bit)
> > > section of the CPU address space, that could be represented as
> > > 
> > >       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
> > > 
> > > or 
> > > 
> > >       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
> > > 
> > > depending on whether you want the BARs to be programmed using a low
> > > address 0x0-0x0fffffff or an address matching the window
> > > 0xb0000000-0xbfffffff.
> > 
> > The problem is, for configuring the window starting at 0xb0000000, the ATU
> > should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
> > 
> 
> Then use the first of the two?
> 

To clarify: using <0x82000000 0 0  0xb0000000  0 0x10000000> will give you 
a mem_offset of 0xb0000000, which should work just fine for this case.

What I don't understand is why the ATU cares about whether the outbound
address is 0x0000000 or 0xb0000000 if it just decodes the lower 28 bit
anyway. Did you mean that we have to program the BARs using low addresses
regardless of what is programmed in the ATU? That would make more sense,
and it also matches what I suggested.

	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

hi Arnd,

On Tuesday 13 May 2014 07:04 PM, Arnd Bergmann wrote:
> On Tuesday 13 May 2014 15:27:46 Arnd Bergmann wrote:
>> On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
>>>> If you have a case where the outbound translation is a 256MB (i.e. 28bit)
>>>> section of the CPU address space, that could be represented as
>>>>
>>>>       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
>>>>
>>>> or 
>>>>
>>>>       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
>>>>
>>>> depending on whether you want the BARs to be programmed using a low
>>>> address 0x0-0x0fffffff or an address matching the window
>>>> 0xb0000000-0xbfffffff.
>>>
>>> The problem is, for configuring the window starting at 0xb0000000, the ATU
>>> should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
>>>
>>
>> Then use the first of the two?
>>
> 
> To clarify: using <0x82000000 0 0  0xb0000000  0 0x10000000> will give you 
> a mem_offset of 0xb0000000, which should work just fine for this case.
> 
> What I don't understand is why the ATU cares about whether the outbound
> address is 0x0000000 or 0xb0000000 if it just decodes the lower 28 bit
> anyway. Did you mean that we have to program the BARs using low addresses
> regardless of what is programmed in the ATU? That would make more sense,
> and it also matches what I suggested.

No, It's not like it decodes only the lower 28bits. The BARs is programmed with
32 bit value.

My pcie dt node has
 ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* CONFIG */
           0x81000000 0 0          0x20003000 0 0x00010000  /* IO */
           0x82000000 0 0x20013000 0x20013000 0 0xffed000>; /* MEM */

Consider MEM address space..

Here both PCI address and CPU address is 0x20013000. So when there is a write
to cpu addr 0x20013000 [writel(virt_addr(0x20013000)], we want it to be
translated to PCI addr 0x20013000. So in 'ATU', we would expect *base* to be
programmed to *0x20013000* and target to be programmed to *0x20013000*. But
that's not the case for DRA7xx. For DRA7xx *base* should be programmed to
*0x0013000* and target should be programmed to *0x20013000*.

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Wednesday 14 May 2014 11:14:45 Kishon Vijay Abraham I wrote:
> hi Arnd,
> 
> On Tuesday 13 May 2014 07:04 PM, Arnd Bergmann wrote:
> > On Tuesday 13 May 2014 15:27:46 Arnd Bergmann wrote:
> >> On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
> >>>> If you have a case where the outbound translation is a 256MB (i.e. 28bit)
> >>>> section of the CPU address space, that could be represented as
> >>>>
> >>>>       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
> >>>>
> >>>> or 
> >>>>
> >>>>       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
> >>>>
> >>>> depending on whether you want the BARs to be programmed using a low
> >>>> address 0x0-0x0fffffff or an address matching the window
> >>>> 0xb0000000-0xbfffffff.
> >>>
> >>> The problem is, for configuring the window starting at 0xb0000000, the ATU
> >>> should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
> >>>
> >>
> >> Then use the first of the two?
> >>
> > 
> > To clarify: using <0x82000000 0 0  0xb0000000  0 0x10000000> will give you 
> > a mem_offset of 0xb0000000, which should work just fine for this case.
> > 
> > What I don't understand is why the ATU cares about whether the outbound
> > address is 0x0000000 or 0xb0000000 if it just decodes the lower 28 bit
> > anyway. Did you mean that we have to program the BARs using low addresses
> > regardless of what is programmed in the ATU? That would make more sense,
> > and it also matches what I suggested.
> 
> No, It's not like it decodes only the lower 28bits. The BARs is programmed with
> 32 bit value.
> 
> My pcie dt node has
>  ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* CONFIG */
>            0x81000000 0 0          0x20003000 0 0x00010000  /* IO */
>            0x82000000 0 0x20013000 0x20013000 0 0xffed000>; /* MEM */
> 
> Consider MEM address space..
> 
> Here both PCI address and CPU address is 0x20013000. So when there is a write
> to cpu addr 0x20013000 [writel(virt_addr(0x20013000)], we want it to be
> translated to PCI addr 0x20013000. So in 'ATU', we would expect *base* to be
> programmed to *0x20013000* and target to be programmed to *0x20013000*. But
> that's not the case for DRA7xx. For DRA7xx *base* should be programmed to
> *0x0013000* and target should be programmed to *0x20013000*.

Ok, got it, thanks for your patience.

I think this would best be modeled as a separate bus node that contains the
restriction, like this:

/ {
	#address-cells = <1>; // or <2> if you support > 4GB address space
	#size-cells = <1>;

	soc {
		#address-cells <1>;
		#size-cells = <1>;
		ranges;
		dma-ranges;

		... // all normal devices

		axi@20000000 {
			#size-cells = <1>;
			#address-cells = <1>;
			dma-ranges; // can access all 4GB outbound
			ranges = <0 0x20000000 0x10000000>; // 28-bit bus

			pci@0 {
				reg = <0x0    0x1000>, // internal regs
				      <0x1000 0x2000>; // config space
				dma-ranges; // 32-bit outbound
				ranges = <0x81000000 0 0           0x3000 0 0x00010000  /* IO */
					  0x82000000 0 0x20013000 0x13000 0 0xffed000>; /* MEM */
			};
		};
	};
};


	Arnd

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

Hi Arnd,

On Wednesday 14 May 2014 06:15 PM, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 11:14:45 Kishon Vijay Abraham I wrote:
>> hi Arnd,
>>
>> On Tuesday 13 May 2014 07:04 PM, Arnd Bergmann wrote:
>>> On Tuesday 13 May 2014 15:27:46 Arnd Bergmann wrote:
>>>> On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
>>>>>> If you have a case where the outbound translation is a 256MB (i.e. 28bit)
>>>>>> section of the CPU address space, that could be represented as
>>>>>>
>>>>>>       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
>>>>>>
>>>>>> or 
>>>>>>
>>>>>>       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
>>>>>>
>>>>>> depending on whether you want the BARs to be programmed using a low
>>>>>> address 0x0-0x0fffffff or an address matching the window
>>>>>> 0xb0000000-0xbfffffff.
>>>>>
>>>>> The problem is, for configuring the window starting at 0xb0000000, the ATU
>>>>> should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
>>>>>
>>>>
>>>> Then use the first of the two?
>>>>
>>>
>>> To clarify: using <0x82000000 0 0  0xb0000000  0 0x10000000> will give you 
>>> a mem_offset of 0xb0000000, which should work just fine for this case.
>>>
>>> What I don't understand is why the ATU cares about whether the outbound
>>> address is 0x0000000 or 0xb0000000 if it just decodes the lower 28 bit
>>> anyway. Did you mean that we have to program the BARs using low addresses
>>> regardless of what is programmed in the ATU? That would make more sense,
>>> and it also matches what I suggested.
>>
>> No, It's not like it decodes only the lower 28bits. The BARs is programmed with
>> 32 bit value.
>>
>> My pcie dt node has
>>  ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* CONFIG */
>>            0x81000000 0 0          0x20003000 0 0x00010000  /* IO */
>>            0x82000000 0 0x20013000 0x20013000 0 0xffed000>; /* MEM */
>>
>> Consider MEM address space..
>>
>> Here both PCI address and CPU address is 0x20013000. So when there is a write
>> to cpu addr 0x20013000 [writel(virt_addr(0x20013000)], we want it to be
>> translated to PCI addr 0x20013000. So in 'ATU', we would expect *base* to be
>> programmed to *0x20013000* and target to be programmed to *0x20013000*. But
>> that's not the case for DRA7xx. For DRA7xx *base* should be programmed to
>> *0x0013000* and target should be programmed to *0x20013000*.
> 
> Ok, got it, thanks for your patience.
> 
> I think this would best be modeled as a separate bus node that contains the
> restriction, like this:
> 
> / {
> 	#address-cells = <1>; // or <2> if you support > 4GB address space
> 	#size-cells = <1>;
> 
> 	soc {
> 		#address-cells <1>;
> 		#size-cells = <1>;
> 		ranges;
> 		dma-ranges;
> 
> 		... // all normal devices
> 
> 		axi@20000000 {
> 			#size-cells = <1>;
> 			#address-cells = <1>;
> 			dma-ranges; // can access all 4GB outbound
> 			ranges = <0 0x20000000 0x10000000>; // 28-bit bus
> 
> 			pci@0 {
> 				reg = <0x0    0x1000>, // internal regs
> 				      <0x1000 0x2000>; // config space
> 				dma-ranges; // 32-bit outbound
> 				ranges = <0x81000000 0 0           0x3000 0 0x00010000  /* IO */
> 					  0x82000000 0 0x20013000 0x13000 0 0xffed000>; /* MEM */
> 			};
> 		};
> 	};
> };

Nice :-)

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Kishon Vijay Abraham I]

Hi Arnd,

On Wednesday 14 May 2014 06:15 PM, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 11:14:45 Kishon Vijay Abraham I wrote:
>> hi Arnd,
>>
>> On Tuesday 13 May 2014 07:04 PM, Arnd Bergmann wrote:
>>> On Tuesday 13 May 2014 15:27:46 Arnd Bergmann wrote:
>>>> On Tuesday 13 May 2014 18:56:23 Kishon Vijay Abraham I wrote:
>>>>>> If you have a case where the outbound translation is a 256MB (i.e. 28bit)
>>>>>> section of the CPU address space, that could be represented as
>>>>>>
>>>>>>       ranges = <0x82000000 0 0  0xb0000000  0 0x10000000>;
>>>>>>
>>>>>> or 
>>>>>>
>>>>>>       ranges = <0x82000000 0 0xb0000000  0xb0000000  0 0x10000000>;
>>>>>>
>>>>>> depending on whether you want the BARs to be programmed using a low
>>>>>> address 0x0-0x0fffffff or an address matching the window
>>>>>> 0xb0000000-0xbfffffff.
>>>>>
>>>>> The problem is, for configuring the window starting at 0xb0000000, the ATU
>>>>> should be programmed 0x0000000 (the cpu address for it will be 0xb0000000 though).
>>>>>
>>>>
>>>> Then use the first of the two?
>>>>
>>>
>>> To clarify: using <0x82000000 0 0  0xb0000000  0 0x10000000> will give you 
>>> a mem_offset of 0xb0000000, which should work just fine for this case.
>>>
>>> What I don't understand is why the ATU cares about whether the outbound
>>> address is 0x0000000 or 0xb0000000 if it just decodes the lower 28 bit
>>> anyway. Did you mean that we have to program the BARs using low addresses
>>> regardless of what is programmed in the ATU? That would make more sense,
>>> and it also matches what I suggested.
>>
>> No, It's not like it decodes only the lower 28bits. The BARs is programmed with
>> 32 bit value.
>>
>> My pcie dt node has
>>  ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000  /* CONFIG */
>>            0x81000000 0 0          0x20003000 0 0x00010000  /* IO */
>>            0x82000000 0 0x20013000 0x20013000 0 0xffed000>; /* MEM */
>>
>> Consider MEM address space..
>>
>> Here both PCI address and CPU address is 0x20013000. So when there is a write
>> to cpu addr 0x20013000 [writel(virt_addr(0x20013000)], we want it to be
>> translated to PCI addr 0x20013000. So in 'ATU', we would expect *base* to be
>> programmed to *0x20013000* and target to be programmed to *0x20013000*. But
>> that's not the case for DRA7xx. For DRA7xx *base* should be programmed to
>> *0x0013000* and target should be programmed to *0x20013000*.
> 
> Ok, got it, thanks for your patience.
> 
> I think this would best be modeled as a separate bus node that contains the
> restriction, like this:
> 
> / {
> 	#address-cells = <1>; // or <2> if you support > 4GB address space
> 	#size-cells = <1>;
> 
> 	soc {
> 		#address-cells <1>;
> 		#size-cells = <1>;
> 		ranges;
> 		dma-ranges;
> 
> 		... // all normal devices
> 
> 		axi@20000000 {
> 			#size-cells = <1>;
> 			#address-cells = <1>;
> 			dma-ranges; // can access all 4GB outbound
> 			ranges = <0 0x20000000 0x10000000>; // 28-bit bus
> 
> 			pci@0 {
> 				reg = <0x0    0x1000>, // internal regs
> 				      <0x1000 0x2000>; // config space

The internal reg address space starts at 0x51000000. By Using this <0
0x20000000 0x10000000>; as ranges, we are not able to get the memory resource
properly. Can we use multiple ranges? how do we specify which ranges the *reg*
property to use?

Btw I was using *simple-bus* as compatible to *axi*. Or should I create a new
*axi* driver to create the pcie memory resources myself?

Thanks
Kishon

Re: [PATCH 06/17] pci: host: pcie-designware: Use *base-mask* for configuring the iATU

[Arnd Bergmann]

On Friday 16 May 2014 14:30:56 Kishon Vijay Abraham I wrote:
> On Wednesday 14 May 2014 06:15 PM, Arnd Bergmann wrote:
> > On Wednesday 14 May 2014 11:14:45 Kishon Vijay Abraham I wrote:
> > / {
> >       #address-cells = <1>; // or <2> if you support > 4GB address space
> >       #size-cells = <1>;
> > 
> >       soc {
> >               #address-cells <1>;
> >               #size-cells = <1>;
> >               ranges;
> >               dma-ranges;
> > 
> >               ... // all normal devices
> > 
> >               axi@20000000 {
> >                       #size-cells = <1>;
> >                       #address-cells = <1>;
> >                       dma-ranges; // can access all 4GB outbound
> >                       ranges = <0 0x20000000 0x10000000>; // 28-bit bus
> > 
> >                       pci@0 {
> >                               reg = <0x0    0x1000>, // internal regs
> >                                     <0x1000 0x2000>; // config space
> 
> The internal reg address space starts at 0x51000000. By Using this <0
> 0x20000000 0x10000000>; as ranges, we are not able to get the memory resource
> properly. Can we use multiple ranges? how do we specify which ranges the *reg*
> property to use?

Yes, multiple ranges will work fine. You can make up a representation
yourself if you don't know what the hardware really does.

Two possible ways of doing this would be

a)

	/* two separate physical connections represented as one logical bus */
	axi@20000000 {
		#address-cells = <2>;
		#size-cells = <1>;
		ranges = <0 0 0x20000000 0x10000000>, /* configurable registers */
			 <1 0 0x51000000 0x01000000>; /* PCI host registers */

		pci@1.0 {
			reg = <1 0 0x01000000>, /* host registers */
				<0 0x1000 0x2000>; /* config space */
		}

	};

b) 

	/* one physical bus, with some address munging */
	axi@20000000 {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0 0x20000000 0x10000000>, /* configurable registers */
			 <0x51000000 0x51000000 0x01000000>; /* PCI host registers */

		pci@1.0 {
			reg = <0x51000000 0x01000000>, /* host registers */
				<0x1000 0x2000>; /* config space */
		}

	};


> Btw I was using *simple-bus* as compatible to *axi*. Or should I create a new
> *axi* driver to create the pcie memory resources myself?

simple-bus is best here, since you don't have a complex bus that needs to
be set up using register accesses or that generates interrupts.

	Arnd

[PATCH 07/17] ARM: dts: DRA7: Add divider table to optfclk_pciephy_div clock

[Kishon Vijay Abraham I]

From: Keerthy <j-keerthy@ti.com>

Add divider table to optfclk_pciephy_div clock. The Documentation
for divider clock can be found at ../clock/ti/divider.txt

Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7xx-clocks.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/dra7xx-clocks.dtsi b/arch/arm/boot/dts/dra7xx-clocks.dtsi
index c767687..55e95c5 100644
--- a/arch/arm/boot/dts/dra7xx-clocks.dtsi
+++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi
@@ -1170,6 +1170,7 @@
 		clocks = <&apll_pcie_ck>;
 		#clock-cells = <0>;
 		reg = <0x021c>;
+		ti,dividers = <2>, <1>;
 		ti,bit-shift = <8>;
 		ti,max-div = <2>;
 	};
-- 
1.7.9.5

[PATCH 08/17] ARM: dts: DRA7: Change the parent of apll_pcie_in_clk_mux to dpll_pcie_ref_m2ldo_ck

[Kishon Vijay Abraham I]

From: Keerthy <j-keerthy@ti.com>

Change the parent of apll_pcie_in_clk_mux to dpll_pcie_ref_m2ldo_ck
from dpll_pcie_ref_ck.

Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7xx-clocks.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/dra7xx-clocks.dtsi b/arch/arm/boot/dts/dra7xx-clocks.dtsi
index 55e95c5..44993ec 100644
--- a/arch/arm/boot/dts/dra7xx-clocks.dtsi
+++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi
@@ -1152,7 +1152,7 @@
 
 	apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
 		compatible = "ti,mux-clock";
-		clocks = <&dpll_pcie_ref_ck>, <&pciesref_acs_clk_ck>;
+		clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
 		#clock-cells = <0>;
 		reg = <0x021c 0x4>;
 		ti,bit-shift = <7>;
-- 
1.7.9.5

[PATCH 09/17] arm: dra7xx: Add hwmod data for pcie1 phy and pcie2 phy

[Kishon Vijay Abraham I]

Added hwmod data for pcie1 and pcie2 phy present in DRA7xx SOC.
Also added the missing CLKCTRL OFFSET macro and CONTEXT OFFSET macro
for pcie1 phy and pcie2 phy.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/mach-omap2/cm2_7xx.h             |    4 ++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |   57 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/prm7xx.h              |    4 ++
 3 files changed, 65 insertions(+)

diff --git a/arch/arm/mach-omap2/cm2_7xx.h b/arch/arm/mach-omap2/cm2_7xx.h
index 9ad7594..e966e3a 100644
--- a/arch/arm/mach-omap2/cm2_7xx.h
+++ b/arch/arm/mach-omap2/cm2_7xx.h
@@ -357,6 +357,10 @@
 #define DRA7XX_CM_L3INIT_SATA_CLKCTRL				DRA7XX_CM_CORE_REGADDR(DRA7XX_CM_CORE_L3INIT_INST, 0x0088)
 #define DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET				0x00a0
 #define DRA7XX_CM_PCIE_STATICDEP_OFFSET				0x00a4
+#define DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL_OFFSET			0x00b0
+#define DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL			DRA7XX_CM_CORE_REGADDR(DRA7XX_CM_CORE_L3INIT_INST, 0x00b0)
+#define DRA7XX_CM_L3INIT_PCIESS2_CLKCTRL_OFFSET			0x00b8
+#define DRA7XX_CM_L3INIT_PCIESS2_CLKCTRL			DRA7XX_CM_CORE_REGADDR(DRA7XX_CM_CORE_L3INIT_INST, 0x00b8)
 #define DRA7XX_CM_GMAC_CLKSTCTRL_OFFSET				0x00c0
 #define DRA7XX_CM_GMAC_STATICDEP_OFFSET				0x00c4
 #define DRA7XX_CM_GMAC_DYNAMICDEP_OFFSET			0x00c8
diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index b9bb476..1282a42 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -1231,6 +1231,45 @@ static struct omap_hwmod dra7xx_ocp2scp3_hwmod = {
 };
 
 /*
+ * 'PCIE PHY' class
+ *
+ */
+
+static struct omap_hwmod_class dra7xx_pcie_phy_hwmod_class = {
+	.name	= "pcie-phy",
+};
+
+/* pcie1 phy */
+static struct omap_hwmod dra7xx_pcie1_phy_hwmod = {
+	.name		= "pcie1-phy",
+	.class		= &dra7xx_pcie_phy_hwmod_class,
+	.clkdm_name	= "l3init_clkdm",
+	.main_clk	= "l4_root_clk_div",
+	.prcm = {
+		.omap4 = {
+			.clkctrl_offs = DRA7XX_CM_L3INIT_PCIESS1_CLKCTRL_OFFSET,
+			.context_offs = DRA7XX_RM_L3INIT_PCIESS1_CONTEXT_OFFSET,
+			.modulemode   = MODULEMODE_SWCTRL,
+		},
+	},
+};
+
+/* pcie2 phy */
+static struct omap_hwmod dra7xx_pcie2_phy_hwmod = {
+	.name		= "pcie2-phy",
+	.class		= &dra7xx_pcie_phy_hwmod_class,
+	.clkdm_name	= "l3init_clkdm",
+	.main_clk	= "l4_root_clk_div",
+	.prcm = {
+		.omap4 = {
+			.clkctrl_offs = DRA7XX_CM_L3INIT_PCIESS2_CLKCTRL_OFFSET,
+			.context_offs = DRA7XX_RM_L3INIT_PCIESS2_CONTEXT_OFFSET,
+			.modulemode   = MODULEMODE_SWCTRL,
+		},
+	},
+};
+
+/*
  * 'qspi' class
  *
  */
@@ -2349,6 +2388,22 @@ static struct omap_hwmod_ocp_if dra7xx_l4_cfg__ocp2scp1 = {
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
+/* l4_cfg -> pcie1 phy */
+static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie1_phy = {
+	.master		= &dra7xx_l4_cfg_hwmod,
+	.slave		= &dra7xx_pcie1_phy_hwmod,
+	.clk		= "l4_root_clk_div",
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* l4_cfg -> pcie2 phy */
+static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie2_phy = {
+	.master		= &dra7xx_l4_cfg_hwmod,
+	.slave		= &dra7xx_pcie2_phy_hwmod,
+	.clk		= "l4_root_clk_div",
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
 static struct omap_hwmod_addr_space dra7xx_qspi_addrs[] = {
 	{
 		.pa_start	= 0x4b300000,
@@ -2696,6 +2751,8 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = {
 	&dra7xx_l4_cfg__mpu,
 	&dra7xx_l4_cfg__ocp2scp1,
 	&dra7xx_l4_cfg__ocp2scp3,
+	&dra7xx_l4_cfg__pcie1_phy,
+	&dra7xx_l4_cfg__pcie2_phy,
 	&dra7xx_l3_main_1__qspi,
 	&dra7xx_l4_cfg__sata,
 	&dra7xx_l4_cfg__smartreflex_core,
diff --git a/arch/arm/mach-omap2/prm7xx.h b/arch/arm/mach-omap2/prm7xx.h
index d92a840..4bb50fbf 100644
--- a/arch/arm/mach-omap2/prm7xx.h
+++ b/arch/arm/mach-omap2/prm7xx.h
@@ -374,6 +374,10 @@
 #define DRA7XX_RM_L3INIT_IEEE1500_2_OCP_CONTEXT_OFFSET		0x007c
 #define DRA7XX_PM_L3INIT_SATA_WKDEP_OFFSET			0x0088
 #define DRA7XX_RM_L3INIT_SATA_CONTEXT_OFFSET			0x008c
+#define DRA7XX_PM_L3INIT_PCIESS1_WKDEP_OFFSET			0x00b0
+#define DRA7XX_RM_L3INIT_PCIESS1_CONTEXT_OFFSET		0x00b4
+#define DRA7XX_PM_L3INIT_PCIESS2_WKDEP_OFFSET			0x00b8
+#define DRA7XX_RM_L3INIT_PCIESS2_CONTEXT_OFFSET		0x00bc
 #define DRA7XX_RM_GMAC_GMAC_CONTEXT_OFFSET			0x00d4
 #define DRA7XX_RM_L3INIT_OCP2SCP1_CONTEXT_OFFSET		0x00e4
 #define DRA7XX_RM_L3INIT_OCP2SCP3_CONTEXT_OFFSET		0x00ec
-- 
1.7.9.5

[PATCH 10/17] arm: dra7xx: Add hwmod data for pcie1 and pcie2 subsystems

[Kishon Vijay Abraham I]

Added hwmod data for pcie1 and pcie2 subsystem present in DRA7xx SOC.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |   55 +++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index 1282a42..a51fa7f 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -1231,6 +1231,43 @@ static struct omap_hwmod dra7xx_ocp2scp3_hwmod = {
 };
 
 /*
+ * 'PCIE' class
+ *
+ */
+
+static struct omap_hwmod_class dra7xx_pcie_hwmod_class = {
+	.name	= "pcie",
+};
+
+/* pcie1 */
+static struct omap_hwmod dra7xx_pcie1_hwmod = {
+	.name		= "pcie1",
+	.class		= &dra7xx_pcie_hwmod_class,
+	.clkdm_name	= "l3init_clkdm",
+	.main_clk	= "l4_root_clk_div",
+	.prcm = {
+		.omap4 = {
+			.clkctrl_offs	= DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET,
+			.modulemode	= MODULEMODE_SWCTRL,
+		},
+	},
+};
+
+/* pcie2 */
+static struct omap_hwmod dra7xx_pcie2_hwmod = {
+	.name		= "pcie2",
+	.class		= &dra7xx_pcie_hwmod_class,
+	.clkdm_name	= "l3init_clkdm",
+	.main_clk	= "l4_root_clk_div",
+	.prcm = {
+		.omap4 = {
+			.clkctrl_offs = DRA7XX_CM_PCIE_CLKSTCTRL_OFFSET,
+			.modulemode   = MODULEMODE_SWCTRL,
+		},
+	},
+};
+
+/*
  * 'PCIE PHY' class
  *
  */
@@ -2388,6 +2425,22 @@ static struct omap_hwmod_ocp_if dra7xx_l4_cfg__ocp2scp1 = {
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
+/* l4_cfg -> pcie1 */
+static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie1 = {
+	.master		= &dra7xx_l4_cfg_hwmod,
+	.slave		= &dra7xx_pcie1_hwmod,
+	.clk		= "l4_root_clk_div",
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* l4_cfg -> pcie2 */
+static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie2 = {
+	.master		= &dra7xx_l4_cfg_hwmod,
+	.slave		= &dra7xx_pcie2_hwmod,
+	.clk		= "l4_root_clk_div",
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
 /* l4_cfg -> pcie1 phy */
 static struct omap_hwmod_ocp_if dra7xx_l4_cfg__pcie1_phy = {
 	.master		= &dra7xx_l4_cfg_hwmod,
@@ -2751,6 +2804,8 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = {
 	&dra7xx_l4_cfg__mpu,
 	&dra7xx_l4_cfg__ocp2scp1,
 	&dra7xx_l4_cfg__ocp2scp3,
+	&dra7xx_l4_cfg__pcie1,
+	&dra7xx_l4_cfg__pcie2,
 	&dra7xx_l4_cfg__pcie1_phy,
 	&dra7xx_l4_cfg__pcie2_phy,
 	&dra7xx_l3_main_1__qspi,
-- 
1.7.9.5

[PATCH 11/17] ARM: dts: dra7xx-clocks: Add missing 32khz clocks used for PHY

[Kishon Vijay Abraham I]

Added missing 32khz clock used by PCIe PHY.
The documention for this node can be found @ ../bindings/clock/ti/gate.txt.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7xx-clocks.dtsi |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/dra7xx-clocks.dtsi b/arch/arm/boot/dts/dra7xx-clocks.dtsi
index 44993ec..e1bd052 100644
--- a/arch/arm/boot/dts/dra7xx-clocks.dtsi
+++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi
@@ -1165,6 +1165,14 @@
 		reg = <0x021c>, <0x0220>;
 	};
 
+	optfclk_pciephy_32khz: optfclk_pciephy_32khz@4a0093b0 {
+		compatible = "ti,gate-clock";
+		clocks = <&sys_32k_ck>;
+		#clock-cells = <0>;
+		reg = <0x13b0>;
+		ti,bit-shift = <8>;
+	};
+
 	optfclk_pciephy_div: optfclk_pciephy_div@4a00821c {
 		compatible = "ti,divider-clock";
 		clocks = <&apll_pcie_ck>;
-- 
1.7.9.5

Re: [PATCH 11/17] ARM: dts: dra7xx-clocks: Add missing 32khz clocks used for PHY

[Roger Quadros]

Hi Kishon,


On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> Added missing 32khz clock used by PCIe PHY.
> The documention for this node can be found @ ../bindings/clock/ti/gate.txt.

Typo in $subject
s/clocks/clock

--
cheers,
-roger

> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  arch/arm/boot/dts/dra7xx-clocks.dtsi |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/dra7xx-clocks.dtsi b/arch/arm/boot/dts/dra7xx-clocks.dtsi
> index 44993ec..e1bd052 100644
> --- a/arch/arm/boot/dts/dra7xx-clocks.dtsi
> +++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi
> @@ -1165,6 +1165,14 @@
>  		reg = <0x021c>, <0x0220>;
>  	};
>  
> +	optfclk_pciephy_32khz: optfclk_pciephy_32khz@4a0093b0 {
> +		compatible = "ti,gate-clock";
> +		clocks = <&sys_32k_ck>;
> +		#clock-cells = <0>;
> +		reg = <0x13b0>;
> +		ti,bit-shift = <8>;
> +	};
> +
>  	optfclk_pciephy_div: optfclk_pciephy_div@4a00821c {
>  		compatible = "ti,divider-clock";
>  		clocks = <&apll_pcie_ck>;
> 

Re: [PATCH 11/17] ARM: dts: dra7xx-clocks: Add missing 32khz clocks used for PHY

[Kishon Vijay Abraham I]

On Wednesday 14 May 2014 06:53 PM, Roger Quadros wrote:
> Hi Kishon,
> 
> 
> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>> Added missing 32khz clock used by PCIe PHY.
>> The documention for this node can be found @ ../bindings/clock/ti/gate.txt.
> 
> Typo in $subject
> s/clocks/clock

Will fix it.

Thanks
Kishon
> 
> --
> cheers,
> -roger
> 
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Rajendra Nayak <rnayak@ti.com>
>> Cc: Tero Kristo <t-kristo@ti.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  arch/arm/boot/dts/dra7xx-clocks.dtsi |    8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/dra7xx-clocks.dtsi b/arch/arm/boot/dts/dra7xx-clocks.dtsi
>> index 44993ec..e1bd052 100644
>> --- a/arch/arm/boot/dts/dra7xx-clocks.dtsi
>> +++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi
>> @@ -1165,6 +1165,14 @@
>>  		reg = <0x021c>, <0x0220>;
>>  	};
>>  
>> +	optfclk_pciephy_32khz: optfclk_pciephy_32khz@4a0093b0 {
>> +		compatible = "ti,gate-clock";
>> +		clocks = <&sys_32k_ck>;
>> +		#clock-cells = <0>;
>> +		reg = <0x13b0>;
>> +		ti,bit-shift = <8>;
>> +	};
>> +
>>  	optfclk_pciephy_div: optfclk_pciephy_div@4a00821c {
>>  		compatible = "ti,divider-clock";
>>  		clocks = <&apll_pcie_ck>;
>>
> 

[PATCH 12/17] ARM: dts: dra7: Add dt data for PCIe PHY control module

[Kishon Vijay Abraham I]

Added dt data for PCIe PHY control module used by PCIe PHY.
The documention for this node can be found @ ../bindings/phy/ti-phy.txt

Cc: Tony Lindgren <tony@atomide.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index f0ca46d..0d3c8c0 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -876,6 +876,14 @@
 			};
 		};
 
+		omap_control_pcie1phy: omap-control-pciephy@0x4a003c40 {
+			compatible = "ti,control-phy-pcie";
+			reg = <0x4a003c40 0x4>, <0x4a003c14 0x4>, <0x4a003c34 0x4>;
+			reg-names = "power", "control_sma", "pcie_pcs";
+			clocks = <&sys_clkin1>;
+			clock-names = "sysclk";
+		};
+
 		/* OCP2SCP3 */
 		ocp2scp@4a090000 {
 			compatible = "ti,omap-ocp2scp";
-- 
1.7.9.5

[PATCH 13/17] ARM: dts: dra7: Add dt data for PCIe PHY

[Kishon Vijay Abraham I]

Added dt data for PCIe PHY as a child node of ocp2scp3.
The documention for this node can be found @ ../bindings/phy/ti-phy.txt.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 0d3c8c0..653b5f6 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -903,6 +903,29 @@
 				clock-names = "sysclk";
 				#phy-cells = <0>;
 			};
+
+			pcie1_phy: pciephy@4a094000 {
+				compatible = "ti,phy-pipe3-pcie";
+				reg = <0x4A094000 0x80>, /* phy_rx */
+				      <0x4A094400 0x64>; /* phy_tx */
+				reg-names = "phy_rx", "phy_tx";
+				ctrl-module = <&omap_control_pcie1phy>;
+				clocks = <&dpll_pcie_ref_ck>,
+					 <&dpll_pcie_ref_m2ldo_ck>,
+					 <&optfclk_pciephy_32khz>,
+					 <&optfclk_pciephy_clk>,
+					 <&optfclk_pciephy_div_clk>,
+					 <&optfclk_pciephy_div>,
+					 <&apll_pcie_in_clk_mux>,
+					 <&pciesref_acs_clk_ck>;
+				clock-names = "dpll_ref", "dpll_ref_m2",
+					      "wkupclk", "refclk",
+					      "div-clk", "phy-div",
+					      "apll_mux", "refclk_ext";
+				#phy-cells = <0>;
+				ti,hwmods = "pcie1-phy";
+				ti,ext-clk;
+			};
 		};
 
 		omap_dwc3_1@48880000 {
-- 
1.7.9.5

[PATCH 14/17] ARM: dts: dra7: Add dt data for PCIe controller

[Kishon Vijay Abraham I]

Added dt data for PCIe controller. This node contains dt data for
both the DRA7 part of designware controller and for the designware core.
The documention for this node can be found @ ../bindings/pci/ti-pci.txt.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 653b5f6..20b1a09 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1011,6 +1011,28 @@
 			};
 		};
 
+		pcie@51000000 {
+			compatible = "ti,dra7xx-pcie";
+			reg = <0x51002000 0x14c>, <0x51000000 0x2000>;
+			reg-names = "ti_conf", "rc_dbics";
+			interrupts = <0 232 0x4>, <0 233 0x4>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			ti,device_type = <3>;
+			ranges = <0x00000800 0 0x20001000 0x20001000 0 0x00002000
+				  0x81000000 0 0          0x20003000 0 0x00010000
+				  0x82000000 0 0x20013000 0x20013000 0 0xffed000>;
+			#interrupt-cells = <1>;
+			base-mask = <0x00000000 0x0fffffff>;
+			num-lanes = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0x0 0 &gic 233>;
+			ti,hwmods = "pcie1";
+			phys = <&pcie1_phy>;
+			phy-names = "pcie-phy";
+		};
+
 		sata: sata@4a141100 {
 			compatible = "snps,dwc-ahci";
 			reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
-- 
1.7.9.5

[PATCH 15/17] ARM: OMAP: Enable PCI for DRA7

[Kishon Vijay Abraham I]

Now that we have added PCIe driver for DRA7 SOCs, enable PCI on
DRA7 SOCs.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/mach-omap2/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index cb31d43..b179e80 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -75,6 +75,8 @@ config SOC_DRA7XX
 	select ARM_GIC
 	select HAVE_ARM_ARCH_TIMER
 	select IRQ_CROSSBAR
+	select MIGHT_HAVE_PCI
+	select ARCH_SUPPORTS_MSI
 
 config ARCH_OMAP2PLUS
 	bool
-- 
1.7.9.5

[TEMP PATCH 16/17] pci: host: pcie-dra7xx: use reset framework APIs to reset PCIe

[Kishon Vijay Abraham I]

Get reset nodes from dt and use reset framework APIs to reset PCIe.
This is needed since reset is handled by the SoC.

Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/pci/ti-pci.txt |    2 ++
 drivers/pci/host/pci-dra7xx.c                    |   10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 6cb6f09..cfb95c0 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -9,6 +9,8 @@ This node should have the properties described in "designware-pcie.txt".
  - phys : the phandle for the PHY device (used by generic PHY framework)
  - phy-names : the names of the PHY corresponding to the PHYs present in the
    *phy* phandle.
+ - resets: phandle used if reset is handled be soc
+ - reset-names: name given to the phandle
 
 Example:
 pcie@51000000 {
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index a37c25c..17d64ee 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/resource.h>
+#include <linux/reset.h>
 #include <linux/types.h>
 
 #include "pcie-designware.h"
@@ -281,6 +282,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct dra7xx_pcie *dra7xx;
 	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
 
 	dra7xx = devm_kzalloc(&pdev->dev, sizeof(*dra7xx), GFP_KERNEL);
 	if (!dra7xx)
@@ -304,6 +306,14 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	if (!base)
 		return -ENOMEM;
 
+	rstc = devm_reset_control_get(dev, "reset");
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);
+
+	ret = reset_control_deassert(rstc);
+	if (ret)
+		return ret;
+
 	phy = devm_phy_get(dev, "pcie-phy");
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
-- 
1.7.9.5

Re: [TEMP PATCH 16/17] pci: host: pcie-dra7xx: use reset framework APIs to reset PCIe

[Dan Murphy]

On 05/06/2014 08:34 AM, Kishon Vijay Abraham I wrote:
> Get reset nodes from dt and use reset framework APIs to reset PCIe.
> This is needed since reset is handled by the SoC.
>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  Documentation/devicetree/bindings/pci/ti-pci.txt |    2 ++
>  drivers/pci/host/pci-dra7xx.c                    |   10 ++++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> index 6cb6f09..cfb95c0 100644
> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> @@ -9,6 +9,8 @@ This node should have the properties described in "designware-pcie.txt".
>   - phys : the phandle for the PHY device (used by generic PHY framework)
>   - phy-names : the names of the PHY corresponding to the PHYs present in the
>     *phy* phandle.
> + - resets: phandle used if reset is handled be soc
> + - reset-names: name given to the phandle

You should just refer to the ti,reset.txt document for this.

>  
>  Example:
>  pcie@51000000 {
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index a37c25c..17d64ee 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/resource.h>
> +#include <linux/reset.h>
>  #include <linux/types.h>
>  
>  #include "pcie-designware.h"
> @@ -281,6 +282,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct dra7xx_pcie *dra7xx;
>  	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
>  
>  	dra7xx = devm_kzalloc(&pdev->dev, sizeof(*dra7xx), GFP_KERNEL);
>  	if (!dra7xx)
> @@ -304,6 +306,14 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  	if (!base)
>  		return -ENOMEM;
>  
> +	rstc = devm_reset_control_get(dev, "reset");
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		return ret;
> +
>  	phy = devm_phy_get(dev, "pcie-phy");
>  	if (IS_ERR(phy))
>  		return PTR_ERR(phy);


-- 
------------------
Dan Murphy

[TEMP PATCH 17/17] ARM: dts: dra7: Add *resets* property for PCIe dt node

[Kishon Vijay Abraham I]

Added *resets* and *reset-names* properies for PCIe dt node.
The documention for this node can be found @ ../bindings/pci/ti-pci.txt.

Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 20b1a09..7bc0555 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1031,6 +1031,8 @@
 			ti,hwmods = "pcie1";
 			phys = <&pcie1_phy>;
 			phy-names = "pcie-phy";
+			resets = <&prm_resets &device_reset>;
+			reset-names = "reset";
 		};
 
 		sata: sata@4a141100 {
-- 
1.7.9.5

Re: [TEMP PATCH 17/17] ARM: dts: dra7: Add *resets* property for PCIe dt node

[Dan Murphy]

On 05/06/2014 08:34 AM, Kishon Vijay Abraham I wrote:
> Added *resets* and *reset-names* properies for PCIe dt node.
> The documention for this node can be found @ ../bindings/pci/ti-pci.txt.
>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  arch/arm/boot/dts/dra7.dtsi |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index 20b1a09..7bc0555 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -1031,6 +1031,8 @@
>  			ti,hwmods = "pcie1";
>  			phys = <&pcie1_phy>;
>  			phy-names = "pcie-phy";
> +			resets = <&prm_resets &device_reset>;

If you look @ v2 of the reset framework patchset your phandle should be

resets = <&prm_resets &pcie1_reset>;

If you call the device_reset phandle you will reset the SoC.


> +			reset-names = "reset";

This needs to be more descriptive.

>  		};
>  
>  		sata: sata@4a141100 {


-- 
------------------
Dan Murphy

Re: [PATCH 02/17] phy: omap-control: add external clock support for PCIe PHY

[Roger Quadros]

On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> Export an API to be called by PIPE3 PHY to enable external clock for
> PCIE PHY. Added a new compatible for PCIE in omap-control in order to
> enable it.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

Reviewed-by: Roger Quadros <rogerq@ti.com>

--
cheers,
-roger

Re: [PATCH 04/17] phy: pipe3: insert delay to enumerate in GEN2 mode

[Roger Quadros]

On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
> 8-bit delay value (0xF1) is required for GEN2 devices to be enumerated
> consistently. Added an API to be called from PHY drivers to set this delay
> value and called it from PIPE3 driver to set the delay value.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

Reviewed-by: Roger Quadros <rogerq@ti.com>

--
cheers,
-roger