[PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Shawn Lin]

This patch to add a generic PHY driver for rockchip PCIe PHY.
Access the PHY via registers provided by GRF (general register
files) module.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v3: None
Changes in v2: None

 drivers/phy/Kconfig             |   7 +
 drivers/phy/Makefile            |   1 +
 drivers/phy/phy-rockchip-pcie.c | 378 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 386 insertions(+)
 create mode 100644 drivers/phy/phy-rockchip-pcie.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index b869b98..d4fc293 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -361,6 +361,13 @@ config PHY_ROCKCHIP_DP
 	help
 	  Enable this to support the Rockchip Display Port PHY.
 
+config PHY_ROCKCHIP_PCIE
+	tristate "Rockchip PCIe PHY Driver"
+	depends on ARCH_ROCKCHIP && OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the Rockchip PCIe PHY.
+
 config PHY_ST_SPEAR1310_MIPHY
 	tristate "ST SPEAR1310-MIPHY driver"
 	select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9c3e73c..725e55d 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
 obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)	+= phy-qcom-apq8064-sata.o
 obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
 obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
+obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
 obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
 obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
new file mode 100644
index 0000000..bc6cd17
--- /dev/null
+++ b/drivers/phy/phy-rockchip-pcie.c
@@ -0,0 +1,378 @@
+/*
+ * Rockchip PCIe PHY driver
+ *
+ * Copyright (C) 2016 Shawn Lin <shawn.lin@rock-chips.com>
+ * Copyright (C) 2016 ROCKCHIP, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+/*
+ * The higher 16-bit of this register is used for write protection
+ * only if BIT(x + 16) set to 1 the BIT(x) can be written.
+ */
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
+
+#define PHY_MAX_LANE_NUM      4
+#define PHY_CFG_DATA_SHIFT    7
+#define PHY_CFG_ADDR_SHIFT    1
+#define PHY_CFG_DATA_MASK     0xf
+#define PHY_CFG_ADDR_MASK     0x3f
+#define PHY_CFG_RD_MASK       0x3ff
+#define PHY_CFG_WR_ENABLE     1
+#define PHY_CFG_WR_DISABLE    1
+#define PHY_CFG_WR_SHIFT      0
+#define PHY_CFG_WR_MASK       1
+#define PHY_CFG_PLL_LOCK      0x10
+#define PHY_CFG_CLK_TEST      0x10
+#define PHY_CFG_CLK_SCC       0x12
+#define PHY_CFG_SEPE_RATE     BIT(3)
+#define PHY_CFG_PLL_100M      BIT(3)
+#define PHY_PLL_LOCKED        BIT(9)
+#define PHY_PLL_OUTPUT        BIT(10)
+#define PHY_LANE_A_STATUS     0x30
+#define PHY_LANE_B_STATUS     0x31
+#define PHY_LANE_C_STATUS     0x32
+#define PHY_LANE_D_STATUS     0x33
+#define PHY_LANE_RX_DET_SHIFT 11
+#define PHY_LANE_RX_DET_TH    0x1
+#define PHY_LANE_IDLE_OFF     0x1
+#define PHY_LANE_IDLE_MASK    0x1
+#define PHY_LANE_IDLE_A_SHIFT 3
+#define PHY_LANE_IDLE_B_SHIFT 4
+#define PHY_LANE_IDLE_C_SHIFT 5
+#define PHY_LANE_IDLE_D_SHIFT 6
+
+struct rockchip_pcie_data {
+	unsigned int pcie_conf;
+	unsigned int pcie_status;
+	unsigned int pcie_laneoff;
+};
+
+struct rockchip_pcie_phy {
+	struct rockchip_pcie_data *phy_data;
+	struct regmap *reg_base;
+	struct reset_control *phy_rst;
+	struct clk *clk_pciephy_ref;
+};
+
+static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
+			      u32 addr, u32 data)
+{
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(data,
+				   PHY_CFG_DATA_MASK,
+				   PHY_CFG_DATA_SHIFT) |
+		     HIWORD_UPDATE(addr,
+				   PHY_CFG_ADDR_MASK,
+				   PHY_CFG_ADDR_SHIFT));
+	udelay(1);
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(PHY_CFG_WR_ENABLE,
+				   PHY_CFG_WR_MASK,
+				   PHY_CFG_WR_SHIFT));
+	udelay(1);
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
+				   PHY_CFG_WR_MASK,
+				   PHY_CFG_WR_SHIFT));
+}
+
+static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
+			     u32 addr)
+{
+	u32 val;
+
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(addr,
+				   PHY_CFG_RD_MASK,
+				   PHY_CFG_ADDR_SHIFT));
+	regmap_read(rk_phy->reg_base,
+		    rk_phy->phy_data->pcie_status,
+		    &val);
+	return val;
+}
+
+void rockchip_pcie_phy_laneoff(struct phy *phy)
+{
+	u32 status;
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	int i;
+
+	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
+		status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
+		if (!((status >> PHY_LANE_RX_DET_SHIFT) &
+		       PHY_LANE_RX_DET_TH))
+			pr_debug("lane %d is used\n", i);
+		else
+			regmap_write(rk_phy->reg_base,
+				     rk_phy->phy_data->pcie_laneoff,
+				     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
+						   PHY_LANE_IDLE_MASK,
+						   PHY_LANE_IDLE_A_SHIFT + i));
+	}
+}
+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
+
+static int rockchip_pcie_phy_power_off(struct phy *phy)
+{
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	int err = 0;
+
+	err = reset_control_assert(rk_phy->phy_rst);
+	if (err) {
+		pr_err("assert phy_rst err %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int rockchip_pcie_phy_power_on(struct phy *phy)
+{
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	int err = 0;
+	u32 status;
+	unsigned long timeout;
+
+	err = reset_control_deassert(rk_phy->phy_rst);
+	if (err) {
+		pr_err("deassert phy_rst err %d\n", err);
+		return err;
+	}
+
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(PHY_CFG_PLL_LOCK,
+				   PHY_CFG_ADDR_MASK,
+				   PHY_CFG_ADDR_SHIFT));
+
+	/*
+	 * No documented timeout value for phy operation below,
+	 * so we make it large enough here. And we use loop-break
+	 * method which should not be harmful.
+	 */
+	timeout = jiffies + msecs_to_jiffies(1000);
+
+	err = -EINVAL;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(rk_phy->reg_base,
+			    rk_phy->phy_data->pcie_status,
+			    &status);
+		if (status & PHY_PLL_LOCKED) {
+			pr_debug("pll locked!\n");
+			err = 0;
+			break;
+		}
+		msleep(20);
+	}
+
+	if (err) {
+		pr_err("pll lock timeout!\n");
+		goto err_pll_lock;
+	}
+
+	phy_wr_cfg(rk_phy, PHY_CFG_CLK_TEST, PHY_CFG_SEPE_RATE);
+	phy_wr_cfg(rk_phy, PHY_CFG_CLK_SCC, PHY_CFG_PLL_100M);
+
+	err = -ETIMEDOUT;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(rk_phy->reg_base,
+			    rk_phy->phy_data->pcie_status,
+			    &status);
+		if (!(status & PHY_PLL_OUTPUT)) {
+			pr_debug("pll output enable done!\n");
+			err = 0;
+			break;
+		}
+		msleep(20);
+	}
+
+	if (err) {
+		pr_err("pll output enable timeout!\n");
+		goto err_pll_lock;
+	}
+
+	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
+		     HIWORD_UPDATE(PHY_CFG_PLL_LOCK,
+				   PHY_CFG_ADDR_MASK,
+				   PHY_CFG_ADDR_SHIFT));
+	err = -EINVAL;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(rk_phy->reg_base,
+			    rk_phy->phy_data->pcie_status,
+			    &status);
+		if (status & PHY_PLL_LOCKED) {
+			pr_debug("pll relocked!\n");
+			err = 0;
+			break;
+		}
+		msleep(20);
+	}
+
+	if (err) {
+		pr_err("pll relock timeout!\n");
+		goto err_pll_lock;
+	}
+
+	return 0;
+
+err_pll_lock:
+	reset_control_assert(rk_phy->phy_rst);
+	return err;
+}
+
+static int rockchip_pcie_phy_init(struct phy *phy)
+{
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	int err = 0;
+
+	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
+	if (err) {
+		pr_err("Fail to enable pcie ref clock.\n");
+		goto err_refclk;
+	}
+
+	err = reset_control_assert(rk_phy->phy_rst);
+	if (err) {
+		pr_err("assert phy_rst err %d\n", err);
+		goto err_reset;
+	}
+
+	return err;
+
+err_reset:
+	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
+err_refclk:
+	return err;
+}
+
+static int rockchip_pcie_phy_exit(struct phy *phy)
+{
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	int err = 0;
+
+	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
+
+	err = reset_control_deassert(rk_phy->phy_rst);
+	if (err) {
+		pr_err("deassert phy_rst err %d\n", err);
+		goto err_reset;
+	}
+
+	return err;
+
+err_reset:
+	clk_prepare_enable(rk_phy->clk_pciephy_ref);
+	return err;
+}
+
+static const struct phy_ops ops = {
+	.init		= rockchip_pcie_phy_init,
+	.exit		= rockchip_pcie_phy_exit,
+	.power_on	= rockchip_pcie_phy_power_on,
+	.power_off	= rockchip_pcie_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const struct rockchip_pcie_data rk3399_pcie_data = {
+	.pcie_conf = 0xe220,
+	.pcie_status = 0xe2a4,
+	.pcie_laneoff = 0xe214,
+};
+
+static const struct of_device_id rockchip_pcie_phy_dt_ids[] = {
+	{
+		.compatible = "rockchip,rk3399-pcie-phy",
+		.data = &rk3399_pcie_data,
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, rockchip_pcie_phy_dt_ids);
+
+static int rockchip_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_pcie_phy *rk_phy;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
+	struct regmap *grf;
+	const struct of_device_id *of_id;
+
+	grf = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(grf)) {
+		dev_err(dev, "Missing rockchip,grf property\n");
+		return PTR_ERR(grf);
+	}
+
+	rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
+	if (!rk_phy)
+		return -ENOMEM;
+
+	of_id = of_match_device(rockchip_pcie_phy_dt_ids, &pdev->dev);
+	if (!of_id)
+		return -EINVAL;
+
+	rk_phy->phy_data = (struct rockchip_pcie_data *)of_id->data;
+	rk_phy->reg_base = grf;
+
+	rk_phy->phy_rst = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(rk_phy->phy_rst)) {
+		if (PTR_ERR(rk_phy->phy_rst) != -EPROBE_DEFER)
+			dev_err(dev,
+				"missing phy property for reset controller\n");
+		return PTR_ERR(rk_phy->phy_rst);
+	}
+
+	rk_phy->clk_pciephy_ref = devm_clk_get(dev, "refclk");
+	if (IS_ERR(rk_phy->clk_pciephy_ref)) {
+		dev_err(dev, "refclk not found.\n");
+		return PTR_ERR(rk_phy->clk_pciephy_ref);
+	}
+
+	generic_phy = devm_phy_create(dev, dev->of_node, &ops);
+	if (IS_ERR(generic_phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		return PTR_ERR(generic_phy);
+	}
+
+	phy_set_drvdata(generic_phy, rk_phy);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static struct platform_driver rockchip_pcie_driver = {
+	.probe		= rockchip_pcie_phy_probe,
+	.driver		= {
+		.name	= "rockchip-pcie-phy",
+		.of_match_table = rockchip_pcie_phy_dt_ids,
+	},
+};
+
+module_platform_driver(rockchip_pcie_driver);
+
+MODULE_AUTHOR("Shawn Lin <shawn.lin@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip PCIe PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.3.7

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
> This patch to add a generic PHY driver for rockchip PCIe PHY.
> Access the PHY via registers provided by GRF (general register
> files) module.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/phy/Kconfig             |   7 +
>  drivers/phy/Makefile            |   1 +
>  drivers/phy/phy-rockchip-pcie.c | 378 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 386 insertions(+)
>  create mode 100644 drivers/phy/phy-rockchip-pcie.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index b869b98..d4fc293 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -361,6 +361,13 @@ config PHY_ROCKCHIP_DP
>  	help
>  	  Enable this to support the Rockchip Display Port PHY.
>  
> +config PHY_ROCKCHIP_PCIE
> +	tristate "Rockchip PCIe PHY Driver"
> +	depends on ARCH_ROCKCHIP && OF
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the Rockchip PCIe PHY.
> +
>  config PHY_ST_SPEAR1310_MIPHY
>  	tristate "ST SPEAR1310-MIPHY driver"
>  	select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9c3e73c..725e55d 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
>  obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)	+= phy-qcom-apq8064-sata.o
>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
> +obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
>  obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
> new file mode 100644
> index 0000000..bc6cd17
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-pcie.c
> @@ -0,0 +1,378 @@
> +/*
> + * Rockchip PCIe PHY driver
> + *
> + * Copyright (C) 2016 Shawn Lin <shawn.lin@rock-chips.com>
> + * Copyright (C) 2016 ROCKCHIP, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +/*
> + * The higher 16-bit of this register is used for write protection
> + * only if BIT(x + 16) set to 1 the BIT(x) can be written.
> + */
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> +
> +#define PHY_MAX_LANE_NUM      4
> +#define PHY_CFG_DATA_SHIFT    7
> +#define PHY_CFG_ADDR_SHIFT    1
> +#define PHY_CFG_DATA_MASK     0xf
> +#define PHY_CFG_ADDR_MASK     0x3f
> +#define PHY_CFG_RD_MASK       0x3ff
> +#define PHY_CFG_WR_ENABLE     1
> +#define PHY_CFG_WR_DISABLE    1
> +#define PHY_CFG_WR_SHIFT      0
> +#define PHY_CFG_WR_MASK       1
> +#define PHY_CFG_PLL_LOCK      0x10
> +#define PHY_CFG_CLK_TEST      0x10
> +#define PHY_CFG_CLK_SCC       0x12
> +#define PHY_CFG_SEPE_RATE     BIT(3)
> +#define PHY_CFG_PLL_100M      BIT(3)
> +#define PHY_PLL_LOCKED        BIT(9)
> +#define PHY_PLL_OUTPUT        BIT(10)
> +#define PHY_LANE_A_STATUS     0x30
> +#define PHY_LANE_B_STATUS     0x31
> +#define PHY_LANE_C_STATUS     0x32
> +#define PHY_LANE_D_STATUS     0x33
> +#define PHY_LANE_RX_DET_SHIFT 11
> +#define PHY_LANE_RX_DET_TH    0x1
> +#define PHY_LANE_IDLE_OFF     0x1
> +#define PHY_LANE_IDLE_MASK    0x1
> +#define PHY_LANE_IDLE_A_SHIFT 3
> +#define PHY_LANE_IDLE_B_SHIFT 4
> +#define PHY_LANE_IDLE_C_SHIFT 5
> +#define PHY_LANE_IDLE_D_SHIFT 6
> +
> +struct rockchip_pcie_data {
> +	unsigned int pcie_conf;
> +	unsigned int pcie_status;
> +	unsigned int pcie_laneoff;
> +};
> +
> +struct rockchip_pcie_phy {
> +	struct rockchip_pcie_data *phy_data;
> +	struct regmap *reg_base;
> +	struct reset_control *phy_rst;
> +	struct clk *clk_pciephy_ref;
> +};
> +
> +static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
> +			      u32 addr, u32 data)
> +{
> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
> +		     HIWORD_UPDATE(data,
> +				   PHY_CFG_DATA_MASK,
> +				   PHY_CFG_DATA_SHIFT) |
> +		     HIWORD_UPDATE(addr,
> +				   PHY_CFG_ADDR_MASK,
> +				   PHY_CFG_ADDR_SHIFT));
> +	udelay(1);
> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
> +		     HIWORD_UPDATE(PHY_CFG_WR_ENABLE,
> +				   PHY_CFG_WR_MASK,
> +				   PHY_CFG_WR_SHIFT));
> +	udelay(1);
> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
> +		     HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
> +				   PHY_CFG_WR_MASK,
> +				   PHY_CFG_WR_SHIFT));
> +}
> +
> +static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
> +			     u32 addr)
> +{
> +	u32 val;
> +
> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
> +		     HIWORD_UPDATE(addr,
> +				   PHY_CFG_RD_MASK,
> +				   PHY_CFG_ADDR_SHIFT));
> +	regmap_read(rk_phy->reg_base,
> +		    rk_phy->phy_data->pcie_status,
> +		    &val);
> +	return val;
> +}
> +
> +void rockchip_pcie_phy_laneoff(struct phy *phy)
> +{
> +	u32 status;
> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	int i;
> +
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
> +		if (!((status >> PHY_LANE_RX_DET_SHIFT) &
> +		       PHY_LANE_RX_DET_TH))
> +			pr_debug("lane %d is used\n", i);
> +		else
> +			regmap_write(rk_phy->reg_base,
> +				     rk_phy->phy_data->pcie_laneoff,
> +				     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
> +						   PHY_LANE_IDLE_MASK,
> +						   PHY_LANE_IDLE_A_SHIFT + i));
> +	}
> +}
> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);

Er.. don't use export symbols from phy driver. I think it would be nice if you
can model the driver in such a way that the PCIe driver can control individual
phy's.

Thanks
Kishon

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Shawn Lin]

Hi Kishon,

On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>> Access the PHY via registers provided by GRF (general register
>> files) module.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/phy/Kconfig             |   7 +
>>  drivers/phy/Makefile            |   1 +
>>  drivers/phy/phy-rockchip-pcie.c | 378 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 386 insertions(+)
>>  create mode 100644 drivers/phy/phy-rockchip-pcie.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index b869b98..d4fc293 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -361,6 +361,13 @@ config PHY_ROCKCHIP_DP
>>  	help
>>  	  Enable this to support the Rockchip Display Port PHY.
>>
>> +config PHY_ROCKCHIP_PCIE
>> +	tristate "Rockchip PCIe PHY Driver"
>> +	depends on ARCH_ROCKCHIP && OF
>> +	select GENERIC_PHY
>> +	help
>> +	  Enable this to support the Rockchip PCIe PHY.
>> +
>>  config PHY_ST_SPEAR1310_MIPHY
>>  	tristate "ST SPEAR1310-MIPHY driver"
>>  	select GENERIC_PHY
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 9c3e73c..725e55d 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
>>  obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)	+= phy-qcom-apq8064-sata.o
>>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
>> +obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
>>  obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
>>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
>>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>> new file mode 100644
>> index 0000000..bc6cd17
>> --- /dev/null
>> +++ b/drivers/phy/phy-rockchip-pcie.c
>> @@ -0,0 +1,378 @@
>> +/*
>> + * Rockchip PCIe PHY driver
>> + *
>> + * Copyright (C) 2016 Shawn Lin <shawn.lin@rock-chips.com>
>> + * Copyright (C) 2016 ROCKCHIP, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +/*
>> + * The higher 16-bit of this register is used for write protection
>> + * only if BIT(x + 16) set to 1 the BIT(x) can be written.
>> + */
>> +#define HIWORD_UPDATE(val, mask, shift) \
>> +		((val) << (shift) | (mask) << ((shift) + 16))
>> +
>> +#define PHY_MAX_LANE_NUM      4
>> +#define PHY_CFG_DATA_SHIFT    7
>> +#define PHY_CFG_ADDR_SHIFT    1
>> +#define PHY_CFG_DATA_MASK     0xf
>> +#define PHY_CFG_ADDR_MASK     0x3f
>> +#define PHY_CFG_RD_MASK       0x3ff
>> +#define PHY_CFG_WR_ENABLE     1
>> +#define PHY_CFG_WR_DISABLE    1
>> +#define PHY_CFG_WR_SHIFT      0
>> +#define PHY_CFG_WR_MASK       1
>> +#define PHY_CFG_PLL_LOCK      0x10
>> +#define PHY_CFG_CLK_TEST      0x10
>> +#define PHY_CFG_CLK_SCC       0x12
>> +#define PHY_CFG_SEPE_RATE     BIT(3)
>> +#define PHY_CFG_PLL_100M      BIT(3)
>> +#define PHY_PLL_LOCKED        BIT(9)
>> +#define PHY_PLL_OUTPUT        BIT(10)
>> +#define PHY_LANE_A_STATUS     0x30
>> +#define PHY_LANE_B_STATUS     0x31
>> +#define PHY_LANE_C_STATUS     0x32
>> +#define PHY_LANE_D_STATUS     0x33
>> +#define PHY_LANE_RX_DET_SHIFT 11
>> +#define PHY_LANE_RX_DET_TH    0x1
>> +#define PHY_LANE_IDLE_OFF     0x1
>> +#define PHY_LANE_IDLE_MASK    0x1
>> +#define PHY_LANE_IDLE_A_SHIFT 3
>> +#define PHY_LANE_IDLE_B_SHIFT 4
>> +#define PHY_LANE_IDLE_C_SHIFT 5
>> +#define PHY_LANE_IDLE_D_SHIFT 6
>> +
>> +struct rockchip_pcie_data {
>> +	unsigned int pcie_conf;
>> +	unsigned int pcie_status;
>> +	unsigned int pcie_laneoff;
>> +};
>> +
>> +struct rockchip_pcie_phy {
>> +	struct rockchip_pcie_data *phy_data;
>> +	struct regmap *reg_base;
>> +	struct reset_control *phy_rst;
>> +	struct clk *clk_pciephy_ref;
>> +};
>> +
>> +static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>> +			      u32 addr, u32 data)
>> +{
>> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>> +		     HIWORD_UPDATE(data,
>> +				   PHY_CFG_DATA_MASK,
>> +				   PHY_CFG_DATA_SHIFT) |
>> +		     HIWORD_UPDATE(addr,
>> +				   PHY_CFG_ADDR_MASK,
>> +				   PHY_CFG_ADDR_SHIFT));
>> +	udelay(1);
>> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>> +		     HIWORD_UPDATE(PHY_CFG_WR_ENABLE,
>> +				   PHY_CFG_WR_MASK,
>> +				   PHY_CFG_WR_SHIFT));
>> +	udelay(1);
>> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>> +		     HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
>> +				   PHY_CFG_WR_MASK,
>> +				   PHY_CFG_WR_SHIFT));
>> +}
>> +
>> +static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
>> +			     u32 addr)
>> +{
>> +	u32 val;
>> +
>> +	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>> +		     HIWORD_UPDATE(addr,
>> +				   PHY_CFG_RD_MASK,
>> +				   PHY_CFG_ADDR_SHIFT));
>> +	regmap_read(rk_phy->reg_base,
>> +		    rk_phy->phy_data->pcie_status,
>> +		    &val);
>> +	return val;
>> +}
>> +
>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>> +{
>> +	u32 status;
>> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>> +	int i;
>> +
>> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>> +		status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>> +		if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>> +		       PHY_LANE_RX_DET_TH))
>> +			pr_debug("lane %d is used\n", i);
>> +		else
>> +			regmap_write(rk_phy->reg_base,
>> +				     rk_phy->phy_data->pcie_laneoff,
>> +				     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>> +						   PHY_LANE_IDLE_MASK,
>> +						   PHY_LANE_IDLE_A_SHIFT + i));
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
>
> Er.. don't use export symbols from phy driver. I think it would be nice if you
> can model the driver in such a way that the PCIe driver can control individual
> phy's.
>

Yes, I was trying to look for a way not to export symbols from
phy... But I failed to find it as there at least need three
interaction between controller and phy which made me believe we
at least need to export one symbol without adding new API for phy.

And I found lots of phy drivers export symbols the same way as mine.
Does it sound okay for me to do the same thing here? :)


> Thanks
> Kishon
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> Hi Kishon,
> 
> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>>> Access the PHY via registers provided by GRF (general register
>>> files) module.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>>
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>>  drivers/phy/Kconfig             |   7 +
>>>  drivers/phy/Makefile            |   1 +
>>>  drivers/phy/phy-rockchip-pcie.c | 378 ++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 386 insertions(+)
>>>  create mode 100644 drivers/phy/phy-rockchip-pcie.c
>>>
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index b869b98..d4fc293 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -361,6 +361,13 @@ config PHY_ROCKCHIP_DP
>>>      help
>>>        Enable this to support the Rockchip Display Port PHY.
>>>
>>> +config PHY_ROCKCHIP_PCIE
>>> +    tristate "Rockchip PCIe PHY Driver"
>>> +    depends on ARCH_ROCKCHIP && OF
>>> +    select GENERIC_PHY
>>> +    help
>>> +      Enable this to support the Rockchip PCIe PHY.
>>> +
>>>  config PHY_ST_SPEAR1310_MIPHY
>>>      tristate "ST SPEAR1310-MIPHY driver"
>>>      select GENERIC_PHY
>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>> index 9c3e73c..725e55d 100644
>>> --- a/drivers/phy/Makefile
>>> +++ b/drivers/phy/Makefile
>>> @@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_EXYNOS5_USBDRD)    += phy-exynos5-usbdrd.o
>>>  obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)    += phy-qcom-apq8064-sata.o
>>>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>>>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
>>> +obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
>>>  obj-$(CONFIG_PHY_ROCKCHIP_DP)        += phy-rockchip-dp.o
>>>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)    += phy-qcom-ipq806x-sata.o
>>>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)    += phy-spear1310-miphy.o
>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>>> new file mode 100644
>>> index 0000000..bc6cd17
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-rockchip-pcie.c
>>> @@ -0,0 +1,378 @@
>>> +/*
>>> + * Rockchip PCIe PHY driver
>>> + *
>>> + * Copyright (C) 2016 Shawn Lin <shawn.lin@rock-chips.com>
>>> + * Copyright (C) 2016 ROCKCHIP, Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/io.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/reset.h>
>>> +
>>> +/*
>>> + * The higher 16-bit of this register is used for write protection
>>> + * only if BIT(x + 16) set to 1 the BIT(x) can be written.
>>> + */
>>> +#define HIWORD_UPDATE(val, mask, shift) \
>>> +        ((val) << (shift) | (mask) << ((shift) + 16))
>>> +
>>> +#define PHY_MAX_LANE_NUM      4
>>> +#define PHY_CFG_DATA_SHIFT    7
>>> +#define PHY_CFG_ADDR_SHIFT    1
>>> +#define PHY_CFG_DATA_MASK     0xf
>>> +#define PHY_CFG_ADDR_MASK     0x3f
>>> +#define PHY_CFG_RD_MASK       0x3ff
>>> +#define PHY_CFG_WR_ENABLE     1
>>> +#define PHY_CFG_WR_DISABLE    1
>>> +#define PHY_CFG_WR_SHIFT      0
>>> +#define PHY_CFG_WR_MASK       1
>>> +#define PHY_CFG_PLL_LOCK      0x10
>>> +#define PHY_CFG_CLK_TEST      0x10
>>> +#define PHY_CFG_CLK_SCC       0x12
>>> +#define PHY_CFG_SEPE_RATE     BIT(3)
>>> +#define PHY_CFG_PLL_100M      BIT(3)
>>> +#define PHY_PLL_LOCKED        BIT(9)
>>> +#define PHY_PLL_OUTPUT        BIT(10)
>>> +#define PHY_LANE_A_STATUS     0x30
>>> +#define PHY_LANE_B_STATUS     0x31
>>> +#define PHY_LANE_C_STATUS     0x32
>>> +#define PHY_LANE_D_STATUS     0x33
>>> +#define PHY_LANE_RX_DET_SHIFT 11
>>> +#define PHY_LANE_RX_DET_TH    0x1
>>> +#define PHY_LANE_IDLE_OFF     0x1
>>> +#define PHY_LANE_IDLE_MASK    0x1
>>> +#define PHY_LANE_IDLE_A_SHIFT 3
>>> +#define PHY_LANE_IDLE_B_SHIFT 4
>>> +#define PHY_LANE_IDLE_C_SHIFT 5
>>> +#define PHY_LANE_IDLE_D_SHIFT 6
>>> +
>>> +struct rockchip_pcie_data {
>>> +    unsigned int pcie_conf;
>>> +    unsigned int pcie_status;
>>> +    unsigned int pcie_laneoff;
>>> +};
>>> +
>>> +struct rockchip_pcie_phy {
>>> +    struct rockchip_pcie_data *phy_data;
>>> +    struct regmap *reg_base;
>>> +    struct reset_control *phy_rst;
>>> +    struct clk *clk_pciephy_ref;
>>> +};
>>> +
>>> +static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>> +                  u32 addr, u32 data)
>>> +{
>>> +    regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>>> +             HIWORD_UPDATE(data,
>>> +                   PHY_CFG_DATA_MASK,
>>> +                   PHY_CFG_DATA_SHIFT) |
>>> +             HIWORD_UPDATE(addr,
>>> +                   PHY_CFG_ADDR_MASK,
>>> +                   PHY_CFG_ADDR_SHIFT));
>>> +    udelay(1);
>>> +    regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>>> +             HIWORD_UPDATE(PHY_CFG_WR_ENABLE,
>>> +                   PHY_CFG_WR_MASK,
>>> +                   PHY_CFG_WR_SHIFT));
>>> +    udelay(1);
>>> +    regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>>> +             HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
>>> +                   PHY_CFG_WR_MASK,
>>> +                   PHY_CFG_WR_SHIFT));
>>> +}
>>> +
>>> +static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
>>> +                 u32 addr)
>>> +{
>>> +    u32 val;
>>> +
>>> +    regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>>> +             HIWORD_UPDATE(addr,
>>> +                   PHY_CFG_RD_MASK,
>>> +                   PHY_CFG_ADDR_SHIFT));
>>> +    regmap_read(rk_phy->reg_base,
>>> +            rk_phy->phy_data->pcie_status,
>>> +            &val);
>>> +    return val;
>>> +}
>>> +
>>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>>> +{
>>> +    u32 status;
>>> +    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>> +    int i;
>>> +
>>> +    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>>> +        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>>> +        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>>> +               PHY_LANE_RX_DET_TH))
>>> +            pr_debug("lane %d is used\n", i);
>>> +        else
>>> +            regmap_write(rk_phy->reg_base,
>>> +                     rk_phy->phy_data->pcie_laneoff,
>>> +                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>>> +                           PHY_LANE_IDLE_MASK,
>>> +                           PHY_LANE_IDLE_A_SHIFT + i));
>>> +    }
>>> +}
>>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
>>
>> Er.. don't use export symbols from phy driver. I think it would be nice if you
>> can model the driver in such a way that the PCIe driver can control individual
>> phy's.
>>
> 
> Yes, I was trying to look for a way not to export symbols from
> phy... But I failed to find it as there at least need three
> interaction between controller and phy which made me believe we
> at least need to export one symbol without adding new API for phy.

That can be managed by implementing a small state machine within the PHY driver.
> 
> And I found lots of phy drivers export symbols the same way as mine.
> Does it sound okay for me to do the same thing here? :)

That's not a good reason. I'll try to avoid having export symbols inside phy
driver.

Thanks
Kishon

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Brian Norris]

Hi,

On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
> >>>>This patch to add a generic PHY driver for rockchip PCIe PHY.
> >>>>Access the PHY via registers provided by GRF (general register
> >>>>files) module.
> >>>>
> >>>>Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>>---
> >>>>
> >>>>Changes in v3: None
> >>>>Changes in v2: None
> >>>>
[...]
> >>>>diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
> >>>>new file mode 100644
> >>>>index 0000000..bc6cd17
> >>>>--- /dev/null
> >>>>+++ b/drivers/phy/phy-rockchip-pcie.c
> >>>>@@ -0,0 +1,378 @@

[...]

> >>>>+void rockchip_pcie_phy_laneoff(struct phy *phy)
> >>>>+{
> >>>>+    u32 status;
> >>>>+    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> >>>>+    int i;
> >>>>+
> >>>>+    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> >>>>+        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
> >>>>+        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
> >>>>+               PHY_LANE_RX_DET_TH))
> >>>>+            pr_debug("lane %d is used\n", i);
> >>>>+        else
> >>>>+            regmap_write(rk_phy->reg_base,
> >>>>+                     rk_phy->phy_data->pcie_laneoff,
> >>>>+                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
> >>>>+                           PHY_LANE_IDLE_MASK,
> >>>>+                           PHY_LANE_IDLE_A_SHIFT + i));
> >>>>+    }
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);

Shawn, I can't find an example of how you planned to use this (though I
can make educated guesses). As such, it's possible there's some
misunderstanding. Maybe you can include a sample patch for the PCIe
controller driver?

Related: it might make sense to have the PCIe controller and PHY
drivers/bindings all in the same patch series (with proper threading,
which we already talked about off-list).

> >>>Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>can model the driver in such a way that the PCIe driver can control individual
> >>>phy's.
> >>>
> >>
> >>Yes, I was trying to look for a way not to export symbols from
> >>phy... But I failed to find it as there at least need three
> >>interaction between controller and phy which made me believe we
> >>at least need to export one symbol without adding new API for phy.

My interpretation of the above is that Shawn means we might turn off up
to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).

> >That can be managed by implementing a small state machine within the PHY driver.
> 
> I don't understand your point of implementing a small state machine
> within the PHY driver.

I'm not 100% sure I understand, but I think I have a reasonable
interpretation below.

> Do you mean I need to call vaarious of power_on/off and count the
> on/off times to decide the state machine?
> 
> I would appreciate it If you could elaborate this a bit more or
> show me a example. :)

My interpretation: rather than associating a single PCIe controller
device with a single struct phy that controls up to 4 lanes, Kishon is
suggesting you should have this driver implement 4 phy objects, one for
each lane. You'd need to add #phy-cells = <1> to the DT binding, and
implement an ->of_xlate() hook so we can associate/address them
properly. Then the PCIe controller would call phy_power_off() on each
lane that's not used.

The state machine would come into play because you have additional power
savings to utilize, but only when all PHYs are off. So the state machine
would just track how many of the lane PHYs are still on, and when the
count reaches 0, you call reset_control_assert(rk_phy->phy_rst).

The DT for this would be:

	pcie0: pcie@f8000000 {
		compatible = "rockchip,rk3399-pcie";
		...
		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
		...
	};

	pcie_phy: pcie-phy {
		compatible = "rockchip,rk3399-pcie-phy";
		...
		#phy-cells = <1>;
		...
	};

(See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
#phy-cells explanation.)

Is that close to what you're suggesting, Kishon? Seems reasonable enough
to me, even if it's slightly more complicated.

Brian

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Shawn Lin]

在 2016/6/24 7:37, Brian Norris 写道:
> Hi,
>
> On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
>> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
>>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
>>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
>>>>> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>>>>>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>>>>>> Access the PHY via registers provided by GRF (general register
>>>>>> files) module.
>>>>>>
>>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
> [...]
>>>>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>>>>>> new file mode 100644
>>>>>> index 0000000..bc6cd17
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/phy/phy-rockchip-pcie.c
>>>>>> @@ -0,0 +1,378 @@
>
> [...]
>
>>>>>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>>>>>> +{
>>>>>> +    u32 status;
>>>>>> +    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>>>>>> +        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>>>>>> +        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>>>>>> +               PHY_LANE_RX_DET_TH))
>>>>>> +            pr_debug("lane %d is used\n", i);
>>>>>> +        else
>>>>>> +            regmap_write(rk_phy->reg_base,
>>>>>> +                     rk_phy->phy_data->pcie_laneoff,
>>>>>> +                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>>>>>> +                           PHY_LANE_IDLE_MASK,
>>>>>> +                           PHY_LANE_IDLE_A_SHIFT + i));
>>>>>> +    }
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
>
> Shawn, I can't find an example of how you planned to use this (though I
> can make educated guesses). As such, it's possible there's some
> misunderstanding. Maybe you can include a sample patch for the PCIe
> controller driver?

It will be called after rockchip_pcie_init_port for phy to disable
the unused lanes.

>
> Related: it might make sense to have the PCIe controller and PHY
> drivers/bindings all in the same patch series (with proper threading,
> which we already talked about off-list).
>
>>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
>>>>> can model the driver in such a way that the PCIe driver can control individual
>>>>> phy's.
>>>>>
>>>>
>>>> Yes, I was trying to look for a way not to export symbols from
>>>> phy... But I failed to find it as there at least need three
>>>> interaction between controller and phy which made me believe we
>>>> at least need to export one symbol without adding new API for phy.
>
> My interpretation of the above is that Shawn means we might turn off up
> to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).

yes, pcie drivers support up to 4 lanes. But the device may only
support x2. This is beyound the awareness of PCIe controller, so pcie
controller can't tell which one(s) should be turned off.

>
>>> That can be managed by implementing a small state machine within the PHY driver.
>>
>> I don't understand your point of implementing a small state machine
>> within the PHY driver.
>
> I'm not 100% sure I understand, but I think I have a reasonable
> interpretation below.
>
>> Do you mean I need to call vaarious of power_on/off and count the
>> on/off times to decide the state machine?
>>
>> I would appreciate it If you could elaborate this a bit more or
>> show me a example. :)
>
> My interpretation: rather than associating a single PCIe controller
> device with a single struct phy that controls up to 4 lanes, Kishon is
> suggesting you should have this driver implement 4 phy objects, one for
> each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> implement an ->of_xlate() hook so we can associate/address them
> properly. Then the PCIe controller would call phy_power_off() on each
> lane that's not used.

As I say above, even we have 4 phy objects, PCIe controller still
doesn't know which one(s) to be turned off, so you have to call 4 times
phy_power_off if I understand it correctly. This doesn't look
okay to me.

>
> The state machine would come into play because you have additional power
> savings to utilize, but only when all PHYs are off. So the state machine
> would just track how many of the lane PHYs are still on, and when the
> count reaches 0, you call reset_control_assert(rk_phy->phy_rst).
>
> The DT for this would be:
>
> 	pcie0: pcie@f8000000 {
> 		compatible = "rockchip,rk3399-pcie";
> 		...
> 		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> 		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
> 		...
> 	};
>
> 	pcie_phy: pcie-phy {
> 		compatible = "rockchip,rk3399-pcie-phy";
> 		...
> 		#phy-cells = <1>;
> 		...
> 	};
>
> (See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
> #phy-cells explanation.)
>
> Is that close to what you're suggesting, Kishon? Seems reasonable enough
> to me, even if it's slightly more complicated.
>
> Brian
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Kishon Vijay Abraham I]

Hi,

On Friday 24 June 2016 05:07 AM, Brian Norris wrote:
> Hi,
> 
> On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
>> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
>>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
>>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
>>>>> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>>>>>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>>>>>> Access the PHY via registers provided by GRF (general register
>>>>>> files) module.
>>>>>>
>>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
> [...]
>>>>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>>>>>> new file mode 100644
>>>>>> index 0000000..bc6cd17
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/phy/phy-rockchip-pcie.c
>>>>>> @@ -0,0 +1,378 @@
> 
> [...]
> 
>>>>>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>>>>>> +{
>>>>>> +    u32 status;
>>>>>> +    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>>>>>> +        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>>>>>> +        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>>>>>> +               PHY_LANE_RX_DET_TH))
>>>>>> +            pr_debug("lane %d is used\n", i);
>>>>>> +        else
>>>>>> +            regmap_write(rk_phy->reg_base,
>>>>>> +                     rk_phy->phy_data->pcie_laneoff,
>>>>>> +                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>>>>>> +                           PHY_LANE_IDLE_MASK,
>>>>>> +                           PHY_LANE_IDLE_A_SHIFT + i));
>>>>>> +    }
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
> 
> Shawn, I can't find an example of how you planned to use this (though I
> can make educated guesses). As such, it's possible there's some
> misunderstanding. Maybe you can include a sample patch for the PCIe
> controller driver?
> 
> Related: it might make sense to have the PCIe controller and PHY
> drivers/bindings all in the same patch series (with proper threading,
> which we already talked about off-list).
> 
>>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
>>>>> can model the driver in such a way that the PCIe driver can control individual
>>>>> phy's.
>>>>>
>>>>
>>>> Yes, I was trying to look for a way not to export symbols from
>>>> phy... But I failed to find it as there at least need three
>>>> interaction between controller and phy which made me believe we
>>>> at least need to export one symbol without adding new API for phy.
> 
> My interpretation of the above is that Shawn means we might turn off up
> to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> 
>>> That can be managed by implementing a small state machine within the PHY driver.
>>
>> I don't understand your point of implementing a small state machine
>> within the PHY driver.
> 
> I'm not 100% sure I understand, but I think I have a reasonable
> interpretation below.
> 
>> Do you mean I need to call vaarious of power_on/off and count the
>> on/off times to decide the state machine?
>>
>> I would appreciate it If you could elaborate this a bit more or
>> show me a example. :)
> 
> My interpretation: rather than associating a single PCIe controller
> device with a single struct phy that controls up to 4 lanes, Kishon is
> suggesting you should have this driver implement 4 phy objects, one for
> each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> implement an ->of_xlate() hook so we can associate/address them
> properly. Then the PCIe controller would call phy_power_off() on each
> lane that's not used.
> 
> The state machine would come into play because you have additional power
> savings to utilize, but only when all PHYs are off. So the state machine
> would just track how many of the lane PHYs are still on, and when the
> count reaches 0, you call reset_control_assert(rk_phy->phy_rst).
> 
> The DT for this would be:
> 
> 	pcie0: pcie@f8000000 {
> 		compatible = "rockchip,rk3399-pcie";
> 		...
> 		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> 		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
> 		...
> 	};
> 
> 	pcie_phy: pcie-phy {
> 		compatible = "rockchip,rk3399-pcie-phy";
> 		...
> 		#phy-cells = <1>;
> 		...
> 	};
> 
> (See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
> #phy-cells explanation.)
> 
> Is that close to what you're suggesting, Kishon? Seems reasonable enough
> to me, even if it's slightly more complicated.

That's pretty much what I had in mind. Thanks for putting this down in an
elaborate manner.

Thanks
Kishon

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Brian Norris]

Hi Shawn,

On Fri, Jun 24, 2016 at 09:37:41AM +0800, Shawn Lin wrote:
> 在 2016/6/24 7:37, Brian Norris 写道:
> >On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> >>在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >>>On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>>>On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>>>On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:

> >>>>>>+void rockchip_pcie_phy_laneoff(struct phy *phy)
> >>>>>>+{
> >>>>>>+    u32 status;
> >>>>>>+    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> >>>>>>+    int i;
> >>>>>>+
> >>>>>>+    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> >>>>>>+        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
> >>>>>>+        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
> >>>>>>+               PHY_LANE_RX_DET_TH))
> >>>>>>+            pr_debug("lane %d is used\n", i);
> >>>>>>+        else
> >>>>>>+            regmap_write(rk_phy->reg_base,
> >>>>>>+                     rk_phy->phy_data->pcie_laneoff,
> >>>>>>+                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
> >>>>>>+                           PHY_LANE_IDLE_MASK,
> >>>>>>+                           PHY_LANE_IDLE_A_SHIFT + i));
> >>>>>>+    }
> >>>>>>+}
> >>>>>>+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
> >
> >Shawn, I can't find an example of how you planned to use this (though I
> >can make educated guesses). As such, it's possible there's some
> >misunderstanding. Maybe you can include a sample patch for the PCIe
> >controller driver?
> 
> It will be called after rockchip_pcie_init_port for phy to disable
> the unused lanes.

A real, working patch would be nice. FWIW, when I try the following
diff, my PCIe WiFi dies gloriously:


diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 661d6e04af71..fbb3dd8da0c7 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -373,6 +373,8 @@ static struct pci_ops rockchip_pcie_ops = {
 	.write = rockchip_pcie_wr_conf,
 };
 
+extern void rockchip_pcie_phy_laneoff(struct phy *phy);
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @port: PCIe port information
@@ -533,6 +535,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
 	pcie_write(port, 0x0,
 		   PCIE_CORE_AXI_CONF_BASE + PCIE_CORE_OB_REGION_DESC1);
 
+	rockchip_pcie_phy_laneoff(port->phy);
+
 	return 0;
 }
 

> >Related: it might make sense to have the PCIe controller and PHY
> >drivers/bindings all in the same patch series (with proper threading,
> >which we already talked about off-list).
> >
> >>>>>Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>>>can model the driver in such a way that the PCIe driver can control individual
> >>>>>phy's.
> >>>>>
> >>>>
> >>>>Yes, I was trying to look for a way not to export symbols from
> >>>>phy... But I failed to find it as there at least need three
> >>>>interaction between controller and phy which made me believe we
> >>>>at least need to export one symbol without adding new API for phy.
> >
> >My interpretation of the above is that Shawn means we might turn off up
> >to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> 
> yes, pcie drivers support up to 4 lanes. But the device may only
> support x2. This is beyound the awareness of PCIe controller, so pcie
> controller can't tell which one(s) should be turned off.

Are you sure the PCIe controller can't know? I'm pretty sure it has know
how many lanes are in use. And your controller even has this coded in
it:

	status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
	status =  0x1 << ((status >> PCIE_CORE_PL_CONF_LANE_SHIFT) &
			   PCIE_CORE_PL_CONF_LANE_MASK);
	dev_dbg(port->dev, "current link width is x%d\n", status);

So, doesn't that mean that out of lanes {0, 1, 2, 3}, that every lane
other than 0 <= lane < width is unused? So you can just disable those,
and have the same effect as the rockchip_pcie_phy_laneoff() function?

> >>>That can be managed by implementing a small state machine within the PHY driver.
> >>
> >>I don't understand your point of implementing a small state machine
> >>within the PHY driver.
> >
> >I'm not 100% sure I understand, but I think I have a reasonable
> >interpretation below.
> >
> >>Do you mean I need to call vaarious of power_on/off and count the
> >>on/off times to decide the state machine?
> >>
> >>I would appreciate it If you could elaborate this a bit more or
> >>show me a example. :)
> >
> >My interpretation: rather than associating a single PCIe controller
> >device with a single struct phy that controls up to 4 lanes, Kishon is
> >suggesting you should have this driver implement 4 phy objects, one for
> >each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> >implement an ->of_xlate() hook so we can associate/address them
> >properly. Then the PCIe controller would call phy_power_off() on each
> >lane that's not used.
> 
> As I say above, even we have 4 phy objects, PCIe controller still
> doesn't know which one(s) to be turned off, so you have to call 4 times
> phy_power_off if I understand it correctly. This doesn't look
> okay to me.

Brian

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

[Brian Norris]

Hi Kishon,

On Mon, Jun 27, 2016 at 10:54:19AM +0530, Kishon Vijay Abraham I wrote:
> On Friday 24 June 2016 05:07 AM, Brian Norris wrote:
> > On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> >> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>>> can model the driver in such a way that the PCIe driver can control individual
> >>>>> phy's.
> >>>>>
> >>>>
> >>>> Yes, I was trying to look for a way not to export symbols from
> >>>> phy... But I failed to find it as there at least need three
> >>>> interaction between controller and phy which made me believe we
> >>>> at least need to export one symbol without adding new API for phy.
> > 
> > My interpretation of the above is that Shawn means we might turn off up
> > to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> > 
> >>> That can be managed by implementing a small state machine within the PHY driver.
> >>
> >> I don't understand your point of implementing a small state machine
> >> within the PHY driver.
> > 
> > I'm not 100% sure I understand, but I think I have a reasonable
> > interpretation below.
> > 
> >> Do you mean I need to call vaarious of power_on/off and count the
> >> on/off times to decide the state machine?
> >>
> >> I would appreciate it If you could elaborate this a bit more or
> >> show me a example. :)
> > 
> > My interpretation: rather than associating a single PCIe controller
> > device with a single struct phy that controls up to 4 lanes, Kishon is
> > suggesting you should have this driver implement 4 phy objects, one for
> > each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> > implement an ->of_xlate() hook so we can associate/address them
> > properly. Then the PCIe controller would call phy_power_off() on each
> > lane that's not used.
> > 
> > The state machine would come into play because you have additional power
> > savings to utilize, but only when all PHYs are off. So the state machine
> > would just track how many of the lane PHYs are still on, and when the
> > count reaches 0, you call reset_control_assert(rk_phy->phy_rst).

[...]

> That's pretty much what I had in mind. Thanks for putting this down in an
> elaborate manner.

No problem. Glad we understand your suggestion now.

It remains to be seen whether we can reasonably utilize this suggestion
though. According to Shawn, the PCIe controller doesn't have anough
information to be able to utilize this model effectively. But I have my
doubts, which I've posted in reply to Shawn.

Regards,
Brian