[PATCH] PCI: imx6: Add DT bindings to configure Tx Driver settings

[PATCH] PCI: imx6: Add DT bindings to configure Tx Driver settings

[Justin Waters]

The settings in GPR8 are dependent upon the particular layout of the
hardware platform. As such, they should be configurable via the device
tree.

As many boards have been using the default values, this implementation
uses the old values if they are not specified in the device tree.

Signed-off-by: Justin Waters <justin.waters@timesys.com>
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     | 13 +++++++
 drivers/pci/host/pci-imx6.c                        | 43 +++++++++++++++++++---
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 6fbba53..3aa462d 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -13,6 +13,17 @@ Required properties:
 - clock-names: Must include the following additional entries:
 	- "pcie_phy"
 
+Optional properties:
+- fsl,tx-deemph-gen1: Sets GPR8 value for Tx driver de-emphasis when PHY is
+  running at Gen1 rates.
+- fsl,tx-deemph-gen2-3p5db: Sets GPR8 value for Tx driver de-emphasis when
+  PHY is running at Gen2 (3.5db) rate.
+- fsl,tx-demmph-gen2-6db: Sets GPR8 value for Tx driver de-emphasis when
+  PHY is running at Gen2 (3.5db) rate.
+- fsl,tx-swing-full: Sets GPR8 value for Tx driver SWING_FULL.
+- fsl,tx-swing-low: Sets GPR8 value for launch amplitude of transmitter when
+  pipe0_tx_swing is set to 0.
+
 Example:
 
 	pcie@0x01000000 {
@@ -37,4 +48,6 @@ Example:
 		                <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
+		fsl,tx-swing-full = <103>;
+		fsl,tx-swing-low = <103>;
 	};
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 230ebf9..10036e4 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -39,6 +39,11 @@ struct imx6_pcie {
 	struct pcie_port	pp;
 	struct regmap		*iomuxc_gpr;
 	void __iomem		*mem_base;
+	u32			tx_deemph_gen1;
+	u32			tx_deemph_gen2_3p5db;
+	u32			tx_deemph_gen2_6db;
+	u32			tx_swing_full;
+	u32			tx_swing_low;
 };
 
 /* PCIe Root Complex registers (memory-mapped) */
@@ -317,15 +322,20 @@ static void imx6_pcie_init_phy(struct pcie_port *pp)
 			IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
 
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
-			IMX6Q_GPR8_TX_DEEMPH_GEN1, 0 << 0);
+			   IMX6Q_GPR8_TX_DEEMPH_GEN1,
+			   imx6_pcie->tx_deemph_gen1 << 0);
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
-			IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB, 0 << 6);
+			   IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
+			   imx6_pcie->tx_deemph_gen2_3p5db << 6);
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
-			IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB, 20 << 12);
+			   IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
+			   imx6_pcie->tx_deemph_gen2_6db << 12);
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
-			IMX6Q_GPR8_TX_SWING_FULL, 127 << 18);
+			   IMX6Q_GPR8_TX_SWING_FULL,
+			   imx6_pcie->tx_swing_full << 18);
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
-			IMX6Q_GPR8_TX_SWING_LOW, 127 << 25);
+			   IMX6Q_GPR8_TX_SWING_LOW,
+			   imx6_pcie->tx_swing_low << 25);
 }
 
 static int imx6_pcie_wait_for_link(struct pcie_port *pp)
@@ -565,6 +575,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 	struct imx6_pcie *imx6_pcie;
 	struct pcie_port *pp;
 	struct resource *dbi_base;
+	struct device_node *node = pdev->dev.of_node;
 	int ret;
 
 	imx6_pcie = devm_kzalloc(&pdev->dev, sizeof(*imx6_pcie), GFP_KERNEL);
@@ -617,6 +628,28 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(imx6_pcie->iomuxc_gpr);
 	}
 
+	/* Grab PCIe PHY Tx Settings */
+	if (of_property_read_u32
+	    (node, "fsl,tx-deemph-gen1", &imx6_pcie->tx_deemph_gen1))
+		imx6_pcie->tx_deemph_gen1 = 0;
+
+	if (of_property_read_u32
+	    (node, "fsl,tx-deemph-gen2-3p5db",
+	     &imx6_pcie->tx_deemph_gen2_3p5db))
+		imx6_pcie->tx_deemph_gen2_3p5db = 0;
+
+	if (of_property_read_u32
+	    (node, "fsl,tx-deemph-gen2-6db", &imx6_pcie->tx_deemph_gen2_6db))
+		imx6_pcie->tx_deemph_gen2_6db = 20;
+
+	if (of_property_read_u32
+	    (node, "fsl,tx-swing-full", &imx6_pcie->tx_swing_full))
+		imx6_pcie->tx_swing_full = 127;
+
+	if (of_property_read_u32
+	    (node, "fsl,tx-swing-low", &imx6_pcie->tx_swing_low))
+		imx6_pcie->tx_swing_low = 127;
+
 	ret = imx6_add_pcie_port(pp, pdev);
 	if (ret < 0)
 		return ret;
-- 
2.5.0

Re: [PATCH] PCI: imx6: Add DT bindings to configure Tx Driver settings

[Lucas Stach]

Am Donnerstag, den 14.01.2016, 15:27 -0500 schrieb Justin Waters:
> The settings in GPR8 are dependent upon the particular layout of the
> hardware platform. As such, they should be configurable via the device
> tree.
> 
> As many boards have been using the default values, this implementation
> uses the old values if they are not specified in the device tree.
> 
> Signed-off-by: Justin Waters <justin.waters@timesys.com>
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     | 13 +++++++
>  drivers/pci/host/pci-imx6.c                        | 43 +++++++++++++++++++---
>  2 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 6fbba53..3aa462d 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -13,6 +13,17 @@ Required properties:
>  - clock-names: Must include the following additional entries:
>  	- "pcie_phy"
>  
> +Optional properties:
> +- fsl,tx-deemph-gen1: Sets GPR8 value for Tx driver de-emphasis when PHY is
> +  running at Gen1 rates.

GPR is a hardware dependent thing and should not be mentioned in the DT
binding. So this should be a plain: "TX driver de-emphasis value when
PHY is running at Gen1 rate." Same comment applied to all other
properties.

Also all those optional properties should mention their default value if
absent: "If this property is missing a default value of 0 will be used."

> +- fsl,tx-deemph-gen2-3p5db: Sets GPR8 value for Tx driver de-emphasis when
> +  PHY is running at Gen2 (3.5db) rate.
> +- fsl,tx-demmph-gen2-6db: Sets GPR8 value for Tx driver de-emphasis when
> +  PHY is running at Gen2 (3.5db) rate.

Wrong spelling of the property + copy&paste issue in the description
(3.5 instead of 6db).

> +- fsl,tx-swing-full: Sets GPR8 value for Tx driver SWING_FULL.
> +- fsl,tx-swing-low: Sets GPR8 value for launch amplitude of transmitter when
> +  pipe0_tx_swing is set to 0.

pipe0_tx_swing again is hardware dependent. Just say "TX swing low"
here.

> +
>  Example:
>  
>  	pcie@0x01000000 {
> @@ -37,4 +48,6 @@ Example:
>  		                <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
>  		clock-names = "pcie", "pcie_bus", "pcie_phy";
> +		fsl,tx-swing-full = <103>;
> +		fsl,tx-swing-low = <103>;

We generally don't add optional properties to the example.

>  	};
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 230ebf9..10036e4 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -39,6 +39,11 @@ struct imx6_pcie {
>  	struct pcie_port	pp;
>  	struct regmap		*iomuxc_gpr;
>  	void __iomem		*mem_base;
> +	u32			tx_deemph_gen1;
> +	u32			tx_deemph_gen2_3p5db;
> +	u32			tx_deemph_gen2_6db;
> +	u32			tx_swing_full;
> +	u32			tx_swing_low;
>  };
>  
>  /* PCIe Root Complex registers (memory-mapped) */
> @@ -317,15 +322,20 @@ static void imx6_pcie_init_phy(struct pcie_port *pp)
>  			IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
>  
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> -			IMX6Q_GPR8_TX_DEEMPH_GEN1, 0 << 0);
> +			   IMX6Q_GPR8_TX_DEEMPH_GEN1,
> +			   imx6_pcie->tx_deemph_gen1 << 0);
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> -			IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB, 0 << 6);
> +			   IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
> +			   imx6_pcie->tx_deemph_gen2_3p5db << 6);
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> -			IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB, 20 << 12);
> +			   IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
> +			   imx6_pcie->tx_deemph_gen2_6db << 12);
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> -			IMX6Q_GPR8_TX_SWING_FULL, 127 << 18);
> +			   IMX6Q_GPR8_TX_SWING_FULL,
> +			   imx6_pcie->tx_swing_full << 18);
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> -			IMX6Q_GPR8_TX_SWING_LOW, 127 << 25);
> +			   IMX6Q_GPR8_TX_SWING_LOW,
> +			   imx6_pcie->tx_swing_low << 25);
>  }
>  
>  static int imx6_pcie_wait_for_link(struct pcie_port *pp)
> @@ -565,6 +575,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
>  	struct resource *dbi_base;
> +	struct device_node *node = pdev->dev.of_node;
>  	int ret;
>  
>  	imx6_pcie = devm_kzalloc(&pdev->dev, sizeof(*imx6_pcie), GFP_KERNEL);
> @@ -617,6 +628,28 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(imx6_pcie->iomuxc_gpr);
>  	}
>  
> +	/* Grab PCIe PHY Tx Settings */
> +	if (of_property_read_u32
> +	    (node, "fsl,tx-deemph-gen1", &imx6_pcie->tx_deemph_gen1))
> +		imx6_pcie->tx_deemph_gen1 = 0;
> +
> +	if (of_property_read_u32
> +	    (node, "fsl,tx-deemph-gen2-3p5db",
> +	     &imx6_pcie->tx_deemph_gen2_3p5db))
> +		imx6_pcie->tx_deemph_gen2_3p5db = 0;

This is a nit, but I would rather have a slight violation of the 80
chars limit than this inconsistent indentation.

> +
> +	if (of_property_read_u32
> +	    (node, "fsl,tx-deemph-gen2-6db", &imx6_pcie->tx_deemph_gen2_6db))
> +		imx6_pcie->tx_deemph_gen2_6db = 20;
> +
> +	if (of_property_read_u32
> +	    (node, "fsl,tx-swing-full", &imx6_pcie->tx_swing_full))
> +		imx6_pcie->tx_swing_full = 127;
> +
> +	if (of_property_read_u32
> +	    (node, "fsl,tx-swing-low", &imx6_pcie->tx_swing_low))
> +		imx6_pcie->tx_swing_low = 127;
> +
>  	ret = imx6_add_pcie_port(pp, pdev);
>  	if (ret < 0)
>  		return ret;

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |