[RFC PATCH 0/6] Reconstruct rockchip's PCIe and PCIe-PHY driver for per-lane PHY model

[RFC PATCH 0/6] Reconstruct rockchip's PCIe and PCIe-PHY driver for per-lane PHY model

[Shawn Lin]

This patchset is trying to reconstruct PCIe and PCIe-PHY driver
for rockchip platform in order to support per-lane PHY mode. And
we could idle the inactive lane(s) finally.

We deprecate the legacy PHY mode but the code could still support
it in order not to break backware compatibility of DTB. And I organize
the patches carefully so that we don't introduce git-bisect issue.

Hi Brian & Jeffy,

I tested it by backporting all things into my kernel 4.4 tree, and it
works fine for both legacy PHY mode and per-lane PHY model. However I
couldn't run 4.12 for my rk3399-evb now, so could you please help test
it on your Chromebook devices which can run 4.12?

Hi Rob,

Does the changes for rockchip-pcie.txt in patch 6 look good to you
from the perspective of DT?



Shawn Lin (6):
  PCI: rockchip: split out rockchip_pcie_get_phys
  PCI: rockchip: introduce per-lanes PHYs support
  phy: rockcip-pcie: reconstruct driver to support per-lane PHYs
  PCI: rockchip: idle the inactive PHY(s)
  arm64: dts: rockchip: convert PCIe to use per-lane PHYs for rk3339-evb
  dt-bindings: PCI: rockchip: convert to use per-lane PHY model

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  25 +++-
 arch/arm64/boot/dts/rockchip/rk3399-evb.dts        |   2 +
 drivers/pci/host/pcie-rockchip.c                   | 162 ++++++++++++++++++---
 drivers/phy/rockchip/phy-rockchip-pcie.c           | 116 +++++++++++++--
 4 files changed, 269 insertions(+), 36 deletions(-)

-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/6] PCI: rockchip: split out rockchip_pcie_get_phys

[Shawn Lin]

We plan to introduce per-lanes PHY, so split out new
function to make it easy in the future. No functional
change intended.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/pci/host/pcie-rockchip.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 5acf869..6632a51 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -853,6 +853,19 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int rockchip_pcie_get_phys(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->dev;
+
+	rockchip->phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(rockchip->phy)) {
+		if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER)
+			dev_err(dev, "missing phy\n");
+		return PTR_ERR(rockchip->phy);
+	}
+
+	return 0;
+}
 
 /**
  * rockchip_pcie_parse_dt - Parse Device Tree
@@ -883,12 +896,8 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	if (IS_ERR(rockchip->apb_base))
 		return PTR_ERR(rockchip->apb_base);
 
-	rockchip->phy = devm_phy_get(dev, "pcie-phy");
-	if (IS_ERR(rockchip->phy)) {
-		if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER)
-			dev_err(dev, "missing phy\n");
+	if (rockchip_pcie_get_phys(rockchip))
 		return PTR_ERR(rockchip->phy);
-	}
 
 	rockchip->lanes = 1;
 	err = of_property_read_u32(node, "num-lanes", &rockchip->lanes);
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/6] PCI: rockchip: introduce per-lanes PHYs support

[Shawn Lin]

We distinguish the legacy PHY with the newer per-lane
PHYs by adding legacy_phy flag and consolidate all
the phy operations into a single function to simply the
code. Note that the legacy phy is still the first option
to be searched in order not to break the backward compatibility
of DT blob, althoug we use devm_phy_optional_get instead which
seams to violate the original statement of pcie-rockchip's DT
document.

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

 drivers/pci/host/pcie-rockchip.c | 138 +++++++++++++++++++++++++++++++--------
 1 file changed, 112 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 6632a51..a3dc7bd 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -47,6 +47,7 @@
 #define HIWORD_UPDATE_BIT(val)		HIWORD_UPDATE(val, val)
 
 #define ENCODE_LANES(x)			((((x) >> 1) & 3) << 4)
+#define MAX_LANE_NUM			4
 
 #define PCIE_CLIENT_BASE		0x0
 #define PCIE_CLIENT_CONFIG		(PCIE_CLIENT_BASE + 0x00)
@@ -210,7 +211,9 @@
 struct rockchip_pcie {
 	void	__iomem *reg_base;		/* DT axi-base */
 	void	__iomem *apb_base;		/* DT apb-base */
+	bool	legacy_phy;
 	struct	phy *phy;
+	struct  phy **phys;
 	struct	reset_control *core_rst;
 	struct	reset_control *mgmt_rst;
 	struct	reset_control *mgmt_sticky_rst;
@@ -242,6 +245,15 @@ struct rockchip_pcie {
 	phys_addr_t mem_bus_addr;
 };
 
+enum phy_ops_type {
+	PHY_INIT,
+	PHY_PWR_ON,
+	PHY_PWR_OFF,
+	PHY_EXIT,
+};
+
+const char *ops_name[] = {"init", "power on", "power off", "exit"};
+
 static u32 rockchip_pcie_read(struct rockchip_pcie *rockchip, u32 reg)
 {
 	return readl(rockchip->apb_base + reg);
@@ -507,6 +519,98 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
 }
 
+static int rockchip_pcie_get_phys(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->dev;
+	struct phy *phy;
+	char *name;
+	u32 i;
+
+	rockchip->phy = devm_phy_optional_get(dev, "pcie-phy");
+	if (IS_ERR(rockchip->phy)) {
+		if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER)
+			dev_err(dev, "missing phy\n");
+		return PTR_ERR(rockchip->phy);
+	} else if (!rockchip->phy) {
+		rockchip->legacy_phy = false;
+	} else {
+		rockchip->legacy_phy = true;
+		return 0;
+	}
+
+	/* per-lane PHYs */
+	rockchip->phys = devm_kcalloc(dev, sizeof(phy), MAX_LANE_NUM,
+				      GFP_KERNEL);
+	if (!rockchip->phys)
+		return -ENOMEM;
+
+	for (i = 0; i < MAX_LANE_NUM; i++) {
+		name = kasprintf(GFP_KERNEL, "%s-%u", "pcie-phy", i);
+		if (!name)
+			return -ENOMEM;
+
+		phy = devm_of_phy_get(rockchip->dev,
+				      rockchip->dev->of_node, name);
+		kfree(name);
+
+		if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) {
+			phy = NULL;
+		} else if (IS_ERR(phy)) {
+			dev_err(dev, "failed to get per-lane PHY#%u: %ld\n", i,
+				PTR_ERR(phy));
+			return PTR_ERR(phy);
+		}
+
+		rockchip->phys[i] = phy;
+	}
+
+	return 0;
+}
+
+static int rockchip_pcie_manipulate_phys(struct rockchip_pcie *rockchip,
+					 enum phy_ops_type type)
+{
+	int i, phy_num, err;
+	struct device *dev = rockchip->dev;
+	struct phy *phy;
+
+	phy_num = rockchip->legacy_phy ? 1 : MAX_LANE_NUM;
+
+	for (i = 0; i < phy_num; i++) {
+		phy = rockchip->legacy_phy ? rockchip->phy :
+					     rockchip->phys[i];
+		switch (type) {
+		case PHY_INIT:
+			err = phy_init(phy);
+			break;
+		case PHY_PWR_ON:
+			err = phy_power_on(phy);
+			break;
+		case PHY_PWR_OFF:
+			err = phy_power_off(phy);
+			break;
+		case PHY_EXIT:
+			err = phy_exit(phy);
+			break;
+		default:
+			dev_err(dev, "unsupported phy_ops_type\n");
+			return -EOPNOTSUPP;
+		}
+
+		if (err < 0) {
+			if (rockchip->legacy_phy)
+				dev_err(dev, "fail to %s legacy PHY, err %d\n",
+					ops_name[type], err);
+			else
+				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
+					ops_name[type], i, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @rockchip: PCIe port information
@@ -537,11 +641,9 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		return err;
 	}
 
-	err = phy_init(rockchip->phy);
-	if (err < 0) {
-		dev_err(dev, "fail to init phy, err %d\n", err);
+	err = rockchip_pcie_manipulate_phys(rockchip, PHY_INIT);
+	if (err < 0)
 		return err;
-	}
 
 	err = reset_control_assert(rockchip->core_rst);
 	if (err) {
@@ -602,11 +704,9 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 			    PCIE_CLIENT_MODE_RC,
 			    PCIE_CLIENT_CONFIG);
 
-	err = phy_power_on(rockchip->phy);
-	if (err) {
-		dev_err(dev, "fail to power on phy, err %d\n", err);
+	err = rockchip_pcie_manipulate_phys(rockchip, PHY_PWR_ON);
+	if (err)
 		return err;
-	}
 
 	/*
 	 * Please don't reorder the deassert sequence of the following
@@ -853,20 +953,6 @@ static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static int rockchip_pcie_get_phys(struct rockchip_pcie *rockchip)
-{
-	struct device *dev = rockchip->dev;
-
-	rockchip->phy = devm_phy_get(dev, "pcie-phy");
-	if (IS_ERR(rockchip->phy)) {
-		if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER)
-			dev_err(dev, "missing phy\n");
-		return PTR_ERR(rockchip->phy);
-	}
-
-	return 0;
-}
-
 /**
  * rockchip_pcie_parse_dt - Parse Device Tree
  * @rockchip: PCIe port information
@@ -1295,8 +1381,8 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
 		return ret;
 	}
 
-	phy_power_off(rockchip->phy);
-	phy_exit(rockchip->phy);
+	rockchip_pcie_manipulate_phys(rockchip, PHY_PWR_OFF);
+	rockchip_pcie_manipulate_phys(rockchip, PHY_EXIT);
 
 	clk_disable_unprepare(rockchip->clk_pcie_pm);
 	clk_disable_unprepare(rockchip->hclk_pcie);
@@ -1538,8 +1624,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
 	pci_unmap_iospace(rockchip->io);
 	irq_domain_remove(rockchip->irq_domain);
 
-	phy_power_off(rockchip->phy);
-	phy_exit(rockchip->phy);
+	rockchip_pcie_manipulate_phys(rockchip, PHY_PWR_OFF);
+	rockchip_pcie_manipulate_phys(rockchip, PHY_EXIT);
 
 	clk_disable_unprepare(rockchip->clk_pcie_pm);
 	clk_disable_unprepare(rockchip->hclk_pcie);
-- 
1.9.1

[RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[Shawn Lin]

This patch reconstructs the whole driver to support per-lane
PHYs. Note that we could also support the legacy PHY if you
don't provide argument to our of_xlate.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 15 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 6904633..da74b47 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -73,10 +73,35 @@ struct rockchip_pcie_data {
 struct rockchip_pcie_phy {
 	struct rockchip_pcie_data *phy_data;
 	struct regmap *reg_base;
+	struct phy **phys;
 	struct reset_control *phy_rst;
 	struct clk *clk_pciephy_ref;
+	u32 pwr_cnt;
+	bool initialized;
 };
 
+static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev,
+					      struct of_phandle_args *args)
+{
+	struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev);
+
+	if (!rk_phy)
+		return ERR_PTR(-ENODEV);
+
+	switch (args->args[0]) {
+	case 1:
+		return rk_phy->phys[1];
+	case 2:
+		return rk_phy->phys[2];
+	case 3:
+		return rk_phy->phys[3];
+	case 0:
+		/* keep backward compatibility to legacy phy */
+	default:
+		return rk_phy->phys[0];
+	}
+}
+
 static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
 			      u32 addr, u32 data)
 {
@@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
 	return val;
 }
 
-static int rockchip_pcie_phy_power_off(struct phy *phy)
+static int rockchip_pcie_phy_common_power_off(struct phy *phy)
 {
 	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
 	int err = 0;
 
+	if (WARN_ON(!rk_phy->pwr_cnt))
+		return -EINVAL;
+
+	if (rk_phy->pwr_cnt > 0)
+		return 0;
+
 	err = reset_control_assert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
 		return err;
 	}
 
+	rk_phy->pwr_cnt--;
+
 	return 0;
 }
 
+#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
+static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \
+{ \
+	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \
+\
+	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 + id)); \
+	return rockchip_pcie_phy_common_power_off(phy); \
+}
+
+DECLARE_PHY_POWER_OFF_PER_LANE(0);
+DECLARE_PHY_POWER_OFF_PER_LANE(1);
+DECLARE_PHY_POWER_OFF_PER_LANE(2);
+DECLARE_PHY_POWER_OFF_PER_LANE(3);
+
+#define PROVIDE_PHY_OPS(id) \
+	{ \
+		.init = rockchip_pcie_phy_init, \
+		.exit = rockchip_pcie_phy_exit, \
+		.power_on = rockchip_pcie_phy_power_on, \
+		.power_off = rockchip_pcie_lane##id##_phy_power_off, \
+		.owner  = THIS_MODULE, \
+}
+
 static int rockchip_pcie_phy_power_on(struct phy *phy)
 {
 	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
@@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 	u32 status;
 	unsigned long timeout;
 
+	if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM))
+		return -EINVAL;
+
+	if (rk_phy->pwr_cnt)
+		return 0;
+
 	err = reset_control_deassert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
@@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		goto err_pll_lock;
 	}
 
+	rk_phy->pwr_cnt++;
 	return 0;
 
 err_pll_lock:
@@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
 	int err = 0;
 
+	if (rk_phy->initialized)
+		return 0;
+
 	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
 	if (err) {
 		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
@@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 		goto err_reset;
 	}
 
-	return err;
+	rk_phy->initialized = true;
+
+	return 0;
 
 err_reset:
 	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
@@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
 {
 	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
 
+	if (!rk_phy->initialized)
+		return 0;
+
 	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
 
+	rk_phy->initialized = false;
+
 	return 0;
 }
 
-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 phy_ops ops[PHY_MAX_LANE_NUM] = {
+	PROVIDE_PHY_OPS(0),
+	PROVIDE_PHY_OPS(1),
+	PROVIDE_PHY_OPS(2),
+	PROVIDE_PHY_OPS(3),
 };
 
 static const struct rockchip_pcie_data rk3399_pcie_data = {
@@ -283,10 +359,10 @@ 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;
+	int i;
 
 	grf = syscon_node_to_regmap(dev->parent->of_node);
 	if (IS_ERR(grf)) {
@@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
 		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);
+	rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy),
+				    PHY_MAX_LANE_NUM, GFP_KERNEL);
+	if (!rk_phy->phys)
+		return -ENOMEM;
+
+	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
+		rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]);
+		if (IS_ERR(rk_phy->phys[i])) {
+			dev_err(dev, "failed to create PHY%d\n", i);
+			return PTR_ERR(rk_phy->phys[i]);
+		}
+
+		phy_set_drvdata(rk_phy->phys[i], rk_phy);
 	}
 
-	phy_set_drvdata(generic_phy, rk_phy);
-	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	platform_set_drvdata(pdev, rk_phy);
+	phy_provider = devm_of_phy_provider_register(dev,
+					rockchip_pcie_phy_of_xlate);
 
 	return PTR_ERR_OR_ZERO(phy_provider);
 }
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[Brian Norris]

On Fri, Jul 14, 2017 at 11:52:43AM +0800, Shawn Lin wrote:
> This patch reconstructs the whole driver to support per-lane
> PHYs. Note that we could also support the legacy PHY if you
> don't provide argument to our of_xlate.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
>  drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 6904633..da74b47 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -73,10 +73,35 @@ struct rockchip_pcie_data {
>  struct rockchip_pcie_phy {
>  	struct rockchip_pcie_data *phy_data;
>  	struct regmap *reg_base;
> +	struct phy **phys;
>  	struct reset_control *phy_rst;
>  	struct clk *clk_pciephy_ref;
> +	u32 pwr_cnt;
> +	bool initialized;
>  };
>  
> +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev,
> +					      struct of_phandle_args *args)
> +{
> +	struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev);
> +
> +	if (!rk_phy)
> +		return ERR_PTR(-ENODEV);

Shouldn't you just check args->args_count to determine legacy vs. new
style?

> +
> +	switch (args->args[0]) {
> +	case 1:
> +		return rk_phy->phys[1];
> +	case 2:
> +		return rk_phy->phys[2];
> +	case 3:
> +		return rk_phy->phys[3];
> +	case 0:
> +		/* keep backward compatibility to legacy phy */
> +	default:

This also ends up accepting invalid indeces. You should probably
bounds-check args->args[0].

Then this can just be:

	if (legacy)
		return rk_phy->phys[0];
	else
		return rk_phy->phys[index];

> +		return rk_phy->phys[0];
> +	}
> +}
> +
>  static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>  			      u32 addr, u32 data)
>  {
> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
>  	return val;
>  }
>  
> -static int rockchip_pcie_phy_power_off(struct phy *phy)
> +static int rockchip_pcie_phy_common_power_off(struct phy *phy)
>  {
>  	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>  	int err = 0;
>  
> +	if (WARN_ON(!rk_phy->pwr_cnt))
> +		return -EINVAL;
> +
> +	if (rk_phy->pwr_cnt > 0)

This should be:

	if (--rk_phy->pwr_cnt)

Also, you technically might need locking, now that multiple phys (which
each only have their own independent mutex) are accessing the same
refcount. Or maybe just make this an atomic variable.

> +		return 0;
> +
>  	err = reset_control_assert(rk_phy->phy_rst);
>  	if (err) {
>  		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
>  		return err;
>  	}
>  
> +	rk_phy->pwr_cnt--;

You've got things backwards... how do you expect to ever decrement this,
if you return earlier in the function? The effect is that you never
power off after you've powered on. (You should try instrumenting and
testing this better.)

> +
>  	return 0;
>  }
>  
> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \

What? All this macro magic (and duplicate generated functions) should
not be necessary. You just need some per-phy data that keeps the index.

> +{ \
> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \
> +\
> +	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 + id)); \
> +	return rockchip_pcie_phy_common_power_off(phy); \
> +}
> +
> +DECLARE_PHY_POWER_OFF_PER_LANE(0);
> +DECLARE_PHY_POWER_OFF_PER_LANE(1);
> +DECLARE_PHY_POWER_OFF_PER_LANE(2);
> +DECLARE_PHY_POWER_OFF_PER_LANE(3);
> +
> +#define PROVIDE_PHY_OPS(id) \
> +	{ \
> +		.init = rockchip_pcie_phy_init, \
> +		.exit = rockchip_pcie_phy_exit, \
> +		.power_on = rockchip_pcie_phy_power_on, \
> +		.power_off = rockchip_pcie_lane##id##_phy_power_off, \
> +		.owner  = THIS_MODULE, \
> +}
> +
>  static int rockchip_pcie_phy_power_on(struct phy *phy)
>  {
>  	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> @@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>  	u32 status;
>  	unsigned long timeout;
>  
> +	if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM))
> +		return -EINVAL;
> +
> +	if (rk_phy->pwr_cnt)

This could just be:

	if (rk_phy->pwr_cnt++)

> +		return 0;
> +
>  	err = reset_control_deassert(rk_phy->phy_rst);
>  	if (err) {
>  		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
> @@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>  		goto err_pll_lock;
>  	}
>  
> +	rk_phy->pwr_cnt++;

Similar problem to what you're doing in power_off(); you're not doing
the refcount right.

Brian

>  	return 0;
>  
>  err_pll_lock:
> @@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
>  	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>  	int err = 0;
>  
> +	if (rk_phy->initialized)
> +		return 0;
> +
>  	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
>  	if (err) {
>  		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
> @@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
>  		goto err_reset;
>  	}
>  
> -	return err;
> +	rk_phy->initialized = true;
> +
> +	return 0;
>  
>  err_reset:
>  	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
> @@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>  {
>  	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>  
> +	if (!rk_phy->initialized)
> +		return 0;
> +
>  	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
>  
> +	rk_phy->initialized = false;
> +
>  	return 0;
>  }
>  
> -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 phy_ops ops[PHY_MAX_LANE_NUM] = {
> +	PROVIDE_PHY_OPS(0),
> +	PROVIDE_PHY_OPS(1),
> +	PROVIDE_PHY_OPS(2),
> +	PROVIDE_PHY_OPS(3),
>  };
>  
>  static const struct rockchip_pcie_data rk3399_pcie_data = {
> @@ -283,10 +359,10 @@ 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;
> +	int i;
>  
>  	grf = syscon_node_to_regmap(dev->parent->of_node);
>  	if (IS_ERR(grf)) {
> @@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
>  		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);
> +	rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy),
> +				    PHY_MAX_LANE_NUM, GFP_KERNEL);
> +	if (!rk_phy->phys)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]);
> +		if (IS_ERR(rk_phy->phys[i])) {
> +			dev_err(dev, "failed to create PHY%d\n", i);
> +			return PTR_ERR(rk_phy->phys[i]);
> +		}
> +
> +		phy_set_drvdata(rk_phy->phys[i], rk_phy);
>  	}
>  
> -	phy_set_drvdata(generic_phy, rk_phy);
> -	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	platform_set_drvdata(pdev, rk_phy);
> +	phy_provider = devm_of_phy_provider_register(dev,
> +					rockchip_pcie_phy_of_xlate);
>  
>  	return PTR_ERR_OR_ZERO(phy_provider);
>  }
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[Shawn Lin]

Hi Brian,

On 2017/7/14 13:10, Brian Norris wrote:
> On Fri, Jul 14, 2017 at 11:52:43AM +0800, Shawn Lin wrote:
>> This patch reconstructs the whole driver to support per-lane
>> PHYs. Note that we could also support the legacy PHY if you
>> don't provide argument to our of_xlate.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   drivers/phy/rockchip/phy-rockchip-pcie.c | 116 +++++++++++++++++++++++++++----
>>   1 file changed, 101 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> index 6904633..da74b47 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> @@ -73,10 +73,35 @@ struct rockchip_pcie_data {
>>   struct rockchip_pcie_phy {
>>   	struct rockchip_pcie_data *phy_data;
>>   	struct regmap *reg_base;
>> +	struct phy **phys;
>>   	struct reset_control *phy_rst;
>>   	struct clk *clk_pciephy_ref;
>> +	u32 pwr_cnt;
>> +	bool initialized;
>>   };
>>   
>> +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev,
>> +					      struct of_phandle_args *args)
>> +{
>> +	struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev);
>> +
>> +	if (!rk_phy)
>> +		return ERR_PTR(-ENODEV);
> 
> Shouldn't you just check args->args_count to determine legacy vs. new
> style?
> 

args_count is 1 for legacy mode but could also means you just add one
phy with the new per-lane mode?

>> +
>> +	switch (args->args[0]) {
>> +	case 1:
>> +		return rk_phy->phys[1];
>> +	case 2:
>> +		return rk_phy->phys[2];
>> +	case 3:
>> +		return rk_phy->phys[3];
>> +	case 0:
>> +		/* keep backward compatibility to legacy phy */
>> +	default:
> 
> This also ends up accepting invalid indeces. You should probably
> bounds-check args->args[0].
> 
> Then this can just be:
> 
> 	if (legacy)
> 		return rk_phy->phys[0];
> 	else
> 		return rk_phy->phys[index];


However, checking args_count to see if it's legacy seems to simply
the code a lot. So I would fix that above.

> 
>> +		return rk_phy->phys[0];
>> +	}
>> +}
>> +
>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>   			      u32 addr, u32 data)
>>   {
>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
>>   	return val;
>>   }
>>   
>> -static int rockchip_pcie_phy_power_off(struct phy *phy)
>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy)
>>   {
>>   	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>   	int err = 0;
>>   
>> +	if (WARN_ON(!rk_phy->pwr_cnt))
>> +		return -EINVAL;
>> +
>> +	if (rk_phy->pwr_cnt > 0)
> 
> This should be:
> 
> 	if (--rk_phy->pwr_cnt)
> 
> Also, you technically might need locking, now that multiple phys (which
> each only have their own independent mutex) are accessing the same
> refcount. Or maybe just make this an atomic variable.

Good catch!

> 
>> +		return 0;
>> +
>>   	err = reset_control_assert(rk_phy->phy_rst);
>>   	if (err) {
>>   		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
>>   		return err;
>>   	}
>>   
>> +	rk_phy->pwr_cnt--;
> 
> You've got things backwards... how do you expect to ever decrement this,
> if you return earlier in the function? The effect is that you never
> power off after you've powered on. (You should try instrumenting and
> testing this better.)

Right, I should notice this if I checked the power for S3 but
unfortunately I didn't..

> 
>> +
>>   	return 0;
>>   }
>>   
>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \
> 
> What? All this macro magic (and duplicate generated functions) should
> not be necessary. You just need some per-phy data that keeps the index.

I can't quite follow yours here. The only argument passing on to
the PHY APIs is 'struct phy *phy', and how could you trace the index
from it? The caller should save phy instead of 'rockchip_pcie_phy', in 
which the per-phy data should be.

Or could you kindly show me some example here:)


> 
>> +{ \
>> +	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy); \
>> +\
>> +	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 + id)); \
>> +	return rockchip_pcie_phy_common_power_off(phy); \
>> +}
>> +
>> +DECLARE_PHY_POWER_OFF_PER_LANE(0);
>> +DECLARE_PHY_POWER_OFF_PER_LANE(1);
>> +DECLARE_PHY_POWER_OFF_PER_LANE(2);
>> +DECLARE_PHY_POWER_OFF_PER_LANE(3);
>> +
>> +#define PROVIDE_PHY_OPS(id) \
>> +	{ \
>> +		.init = rockchip_pcie_phy_init, \
>> +		.exit = rockchip_pcie_phy_exit, \
>> +		.power_on = rockchip_pcie_phy_power_on, \
>> +		.power_off = rockchip_pcie_lane##id##_phy_power_off, \
>> +		.owner  = THIS_MODULE, \
>> +}
>> +
>>   static int rockchip_pcie_phy_power_on(struct phy *phy)
>>   {
>>   	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>> @@ -135,6 +195,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>>   	u32 status;
>>   	unsigned long timeout;
>>   
>> +	if (WARN_ON(rk_phy->pwr_cnt > PHY_MAX_LANE_NUM))
>> +		return -EINVAL;
>> +
>> +	if (rk_phy->pwr_cnt)
> 
> This could just be:
> 
> 	if (rk_phy->pwr_cnt++)
> 
>> +		return 0;
>> +
>>   	err = reset_control_deassert(rk_phy->phy_rst);
>>   	if (err) {
>>   		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
>> @@ -214,6 +280,7 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>>   		goto err_pll_lock;
>>   	}
>>   
>> +	rk_phy->pwr_cnt++;
> 
> Similar problem to what you're doing in power_off(); you're not doing
> the refcount right.

Will fix as well.

> 
> Brian
> 
>>   	return 0;
>>   
>>   err_pll_lock:
>> @@ -226,6 +293,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
>>   	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>   	int err = 0;
>>   
>> +	if (rk_phy->initialized)
>> +		return 0;
>> +
>>   	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
>>   	if (err) {
>>   		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
>> @@ -238,7 +308,9 @@ static int rockchip_pcie_phy_init(struct phy *phy)
>>   		goto err_reset;
>>   	}
>>   
>> -	return err;
>> +	rk_phy->initialized = true;
>> +
>> +	return 0;
>>   
>>   err_reset:
>>   	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
>> @@ -250,17 +322,21 @@ static int rockchip_pcie_phy_exit(struct phy *phy)
>>   {
>>   	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>   
>> +	if (!rk_phy->initialized)
>> +		return 0;
>> +
>>   	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
>>   
>> +	rk_phy->initialized = false;
>> +
>>   	return 0;
>>   }
>>   
>> -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 phy_ops ops[PHY_MAX_LANE_NUM] = {
>> +	PROVIDE_PHY_OPS(0),
>> +	PROVIDE_PHY_OPS(1),
>> +	PROVIDE_PHY_OPS(2),
>> +	PROVIDE_PHY_OPS(3),
>>   };
>>   
>>   static const struct rockchip_pcie_data rk3399_pcie_data = {
>> @@ -283,10 +359,10 @@ 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;
>> +	int i;
>>   
>>   	grf = syscon_node_to_regmap(dev->parent->of_node);
>>   	if (IS_ERR(grf)) {
>> @@ -319,14 +395,24 @@ static int rockchip_pcie_phy_probe(struct platform_device *pdev)
>>   		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);
>> +	rk_phy->phys = devm_kcalloc(dev, sizeof(struct phy),
>> +				    PHY_MAX_LANE_NUM, GFP_KERNEL);
>> +	if (!rk_phy->phys)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>> +		rk_phy->phys[i] = devm_phy_create(dev, dev->of_node, &ops[i]);
>> +		if (IS_ERR(rk_phy->phys[i])) {
>> +			dev_err(dev, "failed to create PHY%d\n", i);
>> +			return PTR_ERR(rk_phy->phys[i]);
>> +		}
>> +
>> +		phy_set_drvdata(rk_phy->phys[i], rk_phy);
>>   	}
>>   
>> -	phy_set_drvdata(generic_phy, rk_phy);
>> -	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +	platform_set_drvdata(pdev, rk_phy);
>> +	phy_provider = devm_of_phy_provider_register(dev,
>> +					rockchip_pcie_phy_of_xlate);
>>   
>>   	return PTR_ERR_OR_ZERO(phy_provider);
>>   }
>> -- 
>> 1.9.1
>>
>>
> 
> 
> 

Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[jeffy]

Hi Shawn,

On 07/14/2017 02:33 PM, Shawn Lin wrote:
>>
>>> +        return rk_phy->phys[0];
>>> +    }
>>> +}
>>> +
>>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>>                     u32 addr, u32 data)
>>>   {
>>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct
>>> rockchip_pcie_phy *rk_phy,
>>>       return val;
>>>   }
>>> -static int rockchip_pcie_phy_power_off(struct phy *phy)
>>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy)
>>>   {
>>>       struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>       int err = 0;
>>> +    if (WARN_ON(!rk_phy->pwr_cnt))
>>> +        return -EINVAL;
>>> +
>>> +    if (rk_phy->pwr_cnt > 0)
>>
>> This should be:
>>
>>     if (--rk_phy->pwr_cnt)
>>
>> Also, you technically might need locking, now that multiple phys (which
>> each only have their own independent mutex) are accessing the same
>> refcount. Or maybe just make this an atomic variable.
>
> Good catch!
Sounds like we need something similar to phy-core.c's power_count and 
init_count.

>>> +
>>>       return 0;
>>>   }
>>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
>>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \
>>
>> What? All this macro magic (and duplicate generated functions) should
>> not be necessary. You just need some per-phy data that keeps the index.
>
> I can't quite follow yours here. The only argument passing on to
> the PHY APIs is 'struct phy *phy', and how could you trace the index
> from it? The caller should save phy instead of 'rockchip_pcie_phy', in
> which the per-phy data should be.
>
> Or could you kindly show me some example here:)
>
Maybe add a struct rockchip_pcie_phy_data for each phy, contains their 
index and a pointer to the common struct rockchip_pcie_phy?



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[Shawn Lin]

Hi Jeffy,

On 2017/7/14 15:03, jeffy wrote:
> Hi Shawn,
> 
> On 07/14/2017 02:33 PM, Shawn Lin wrote:
>>>
>>>> +        return rk_phy->phys[0];
>>>> +    }
>>>> +}
>>>> +
>>>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>>>                     u32 addr, u32 data)
>>>>   {
>>>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct
>>>> rockchip_pcie_phy *rk_phy,
>>>>       return val;
>>>>   }
>>>> -static int rockchip_pcie_phy_power_off(struct phy *phy)
>>>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy)
>>>>   {
>>>>       struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>>       int err = 0;
>>>> +    if (WARN_ON(!rk_phy->pwr_cnt))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (rk_phy->pwr_cnt > 0)
>>>
>>> This should be:
>>>
>>>     if (--rk_phy->pwr_cnt)
>>>
>>> Also, you technically might need locking, now that multiple phys (which
>>> each only have their own independent mutex) are accessing the same
>>> refcount. Or maybe just make this an atomic variable.
>>
>> Good catch!
> Sounds like we need something similar to phy-core.c's power_count and 
> init_count.

Probably, and I will look into it later.

> 
>>>> +
>>>>       return 0;
>>>>   }
>>>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
>>>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \
>>>
>>> What? All this macro magic (and duplicate generated functions) should
>>> not be necessary. You just need some per-phy data that keeps the index.
>>
>> I can't quite follow yours here. The only argument passing on to
>> the PHY APIs is 'struct phy *phy', and how could you trace the index
>> from it? The caller should save phy instead of 'rockchip_pcie_phy', in
>> which the per-phy data should be.
>>
>> Or could you kindly show me some example here:)
>>
> Maybe add a struct rockchip_pcie_phy_data for each phy, contains their 
> index and a pointer to the common struct rockchip_pcie_phy?

yes, I got Brian's point after reading it more times, and I almost
finish converting to per-lane data now..

> 
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[jeffy]

Hi Shawn,

On 07/14/2017 02:33 PM, Shawn Lin wrote:
>>>
>>> +static struct phy *rockchip_pcie_phy_of_xlate(struct device *dev,
>>> +                          struct of_phandle_args *args)
>>> +{
>>> +    struct rockchip_pcie_phy *rk_phy = dev_get_drvdata(dev);
>>> +
>>> +    if (!rk_phy)
>>> +        return ERR_PTR(-ENODEV);
>>
>> Shouldn't you just check args->args_count to determine legacy vs. new
>> style?
>>
>
> args_count is 1 for legacy mode but could also means you just add one
> phy with the new per-lane mode?

The new per-lane mode should follow by a lane number, so their 
args_count should be 1, while in the legacy mode it should be 0.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 4/6] PCI: rockchip: idle the inactive PHY(s)

[Shawn Lin]

Check the status of all lanes and idle the inactive one(s).

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/pci/host/pcie-rockchip.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index a3dc7bd..d678a93 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -15,6 +15,7 @@
  * (at your option) any later version.
  */
 
+#include <linux/bitrev.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -112,6 +113,9 @@
 #define   PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT	16
 #define   PCIE_CORE_TXCREDIT_CFG1_MUI_ENCODE(x) \
 		(((x) >> 3) << PCIE_CORE_TXCREDIT_CFG1_MUI_SHIFT)
+#define PCIE_CORE_LANE_MAP             (PCIE_CORE_CTRL_MGMT_BASE + 0x200)
+#define   PCIE_CORE_LANE_MAP_MASK              0x0000000f
+#define   PCIE_CORE_LANE_MAP_REVERSE           BIT(16)
 #define PCIE_CORE_INT_STATUS		(PCIE_CORE_CTRL_MGMT_BASE + 0x20c)
 #define   PCIE_CORE_INT_PRFPE			BIT(0)
 #define   PCIE_CORE_INT_CRFPE			BIT(1)
@@ -311,6 +315,18 @@ static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
 	return 1;
 }
 
+static u8 rockchip_pcie_lane_map(struct rockchip_pcie *rockchip)
+{
+	u32 val = rockchip_pcie_read(rockchip, PCIE_CORE_LANE_MAP);
+	u8 map = val & PCIE_CORE_LANE_MAP_MASK;
+
+	/* The link may be using a reverse-indexed mapping. */
+	if (val & PCIE_CORE_LANE_MAP_REVERSE)
+		map = bitrev8(map) >> 4;
+
+	return map;
+}
+
 static int rockchip_pcie_rd_own_conf(struct rockchip_pcie *rockchip,
 				     int where, int size, u32 *val)
 {
@@ -618,7 +634,8 @@ static int rockchip_pcie_manipulate_phys(struct rockchip_pcie *rockchip,
 static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 {
 	struct device *dev = rockchip->dev;
-	int err;
+	int err, i;
+	u8 lane_map;
 	u32 status;
 
 	gpiod_set_value(rockchip->ep_gpio, 0);
@@ -791,6 +808,18 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 			  PCIE_CORE_PL_CONF_LANE_SHIFT);
 	dev_dbg(dev, "current link width is x%d\n", status);
 
+	if (!rockchip->legacy_phy) {
+		/*  power off unused lane(s) */
+		lane_map = rockchip_pcie_lane_map(rockchip);
+		for (i = 0; i < MAX_LANE_NUM; i++) {
+			if (lane_map & BIT(i))
+				continue;
+
+			dev_err(dev, "idling lane %d\n", i);
+			phy_power_off(rockchip->phys[i]);
+		}
+	}
+
 	rockchip_pcie_write(rockchip, ROCKCHIP_VENDOR_ID,
 			    PCIE_CORE_CONFIG_VENDOR);
 	rockchip_pcie_write(rockchip,
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 5/6] arm64: dts: rockchip: convert PCIe to use per-lane PHYs for rk3339-evb

[Shawn Lin]

DON'T MERGE currently as it's just for test and we could
introduce it for rk3399.dtsi when the PHY & PCIe drivers
finally got merged.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 arch/arm64/boot/dts/rockchip/rk3399-evb.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
index 42033bc..6398ca6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
@@ -198,6 +198,8 @@
 &pcie0 {
 	ep-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
 	num-lanes = <4>;
+	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
+	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_clkreqn>;
 	status = "disabled";
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 6/6] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

[Shawn Lin]

Deprecate legacy PHY model and encourage to use per-lane PHY
model.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

---

 .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 1453a73..78d4469 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -19,8 +19,6 @@ Required properties:
 	- "pm"
 - msi-map: Maps a Requester ID to an MSI controller and associated
 	msi-specifier data. See ./pci-msi.txt
-- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
-- phy-names:  MUST be "pcie-phy".
 - interrupts: Three interrupt entries must be specified.
 - interrupt-names: Must include the following names
 	- "sys"
@@ -42,6 +40,18 @@ Required properties:
 	interrupt source. The value must be 1.
 - interrupt-map-mask and interrupt-map: standard PCI properties
 
+Required properties for legacy PHY model (deprecated):
+- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
+- phy-names:  MUST be "pcie-phy".
+
+Required properties for per-lane PHY model:
+- phys: Must contain an phandle to a PHY for each entry in phy-names.
+- phy-names: Must include an entry for each active lane. Note that the number
+  of entries does not have to (though usually will) be equal to the specified
+  number of lanes in the num-lanes property. Entries are of the form
+  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
+  (see example below)
+
 Optional Property:
 - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
 	using 24MHz OSC for RC's PHY.
@@ -95,6 +105,7 @@ pcie0: pcie@f8000000 {
 		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
 	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
 		      "pm", "pclk", "aclk";
+	/* deprecated legacy PHY model */
 	phys = <&pcie_phy>;
 	phy-names = "pcie-phy";
 	pinctrl-names = "default";
@@ -111,3 +122,13 @@ pcie0: pcie@f8000000 {
 		#interrupt-cells = <1>;
 	};
 };
+
+pcie0: pcie@f8000000 {
+	...
+
+	/* preferred per-lane PHY model */
+	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
+	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
+
+	...
+};
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 6/6] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

[Brian Norris]

Hi Shawn,

On Fri, Jul 14, 2017 at 11:52:46AM +0800, Shawn Lin wrote:
> Deprecate legacy PHY model and encourage to use per-lane PHY
> model.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index 1453a73..78d4469 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -19,8 +19,6 @@ Required properties:
>  	- "pm"
>  - msi-map: Maps a Requester ID to an MSI controller and associated
>  	msi-specifier data. See ./pci-msi.txt
> -- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
> -- phy-names:  MUST be "pcie-phy".
>  - interrupts: Three interrupt entries must be specified.
>  - interrupt-names: Must include the following names
>  	- "sys"
> @@ -42,6 +40,18 @@ Required properties:
>  	interrupt source. The value must be 1.
>  - interrupt-map-mask and interrupt-map: standard PCI properties
>  
> +Required properties for legacy PHY model (deprecated):
> +- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
> +- phy-names:  MUST be "pcie-phy".
> +
> +Required properties for per-lane PHY model:
> +- phys: Must contain an phandle to a PHY for each entry in phy-names.
> +- phy-names: Must include an entry for each active lane. Note that the number
> +  of entries does not have to (though usually will) be equal to the specified
> +  number of lanes in the num-lanes property. Entries are of the form
> +  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
> +  (see example below)

So (for the non-legacy case) are you requiring listing all 4 PHY lanes,
or not? If you aren't, then you need to make the PHY driver handle the
case were lanes {X..3} were never "powered on", but we want them idled.
IIUC, your current (broken) implementation is trying (incorrectly) to
only idle a lane once it has been powered on and then back off. But that
won't ever happen if we only request (for example) PHY lane 0.

It's also misleading that it's possible to specify only some subset of
the PHY lanes, but the driver might still try to use them all.

Brian

> +
>  Optional Property:
>  - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
>  	using 24MHz OSC for RC's PHY.
> @@ -95,6 +105,7 @@ pcie0: pcie@f8000000 {
>  		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
>  	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
>  		      "pm", "pclk", "aclk";
> +	/* deprecated legacy PHY model */
>  	phys = <&pcie_phy>;
>  	phy-names = "pcie-phy";
>  	pinctrl-names = "default";
> @@ -111,3 +122,13 @@ pcie0: pcie@f8000000 {
>  		#interrupt-cells = <1>;
>  	};
>  };
> +
> +pcie0: pcie@f8000000 {
> +	...
> +
> +	/* preferred per-lane PHY model */
> +	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> +	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
> +
> +	...
> +};
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 6/6] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

[Shawn Lin]

Hi Brian,

On 2017/7/15 4:47, Brian Norris wrote:
> Hi Shawn,
> 
> On Fri, Jul 14, 2017 at 11:52:46AM +0800, Shawn Lin wrote:
>> Deprecate legacy PHY model and encourage to use per-lane PHY
>> model.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>>   .../devicetree/bindings/pci/rockchip-pcie.txt      | 25 ++++++++++++++++++++--
>>   1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> index 1453a73..78d4469 100644
>> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> @@ -19,8 +19,6 @@ Required properties:
>>   	- "pm"
>>   - msi-map: Maps a Requester ID to an MSI controller and associated
>>   	msi-specifier data. See ./pci-msi.txt
>> -- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
>> -- phy-names:  MUST be "pcie-phy".
>>   - interrupts: Three interrupt entries must be specified.
>>   - interrupt-names: Must include the following names
>>   	- "sys"
>> @@ -42,6 +40,18 @@ Required properties:
>>   	interrupt source. The value must be 1.
>>   - interrupt-map-mask and interrupt-map: standard PCI properties
>>   
>> +Required properties for legacy PHY model (deprecated):
>> +- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
>> +- phy-names:  MUST be "pcie-phy".
>> +
>> +Required properties for per-lane PHY model:
>> +- phys: Must contain an phandle to a PHY for each entry in phy-names.
>> +- phy-names: Must include an entry for each active lane. Note that the number
>> +  of entries does not have to (though usually will) be equal to the specified
>> +  number of lanes in the num-lanes property. Entries are of the form
>> +  "pcie-phy-N": where N ranges from 0 to the value specified of (num-lanes - 1).
>> +  (see example below)
> 
> So (for the non-legacy case) are you requiring listing all 4 PHY lanes,
> or not? 

I intended to do that however that makes the code more complex and I
didn't see any gains here. So I would say "list 4 of them here is
mandatory".

Will update this Doc to reflect the fact.

Thanks.

If you aren't, then you need to make the PHY driver handle the
> case were lanes {X..3} were never "powered on", but we want them idled.
> IIUC, your current (broken) implementation is trying (incorrectly) to
> only idle a lane once it has been powered on and then back off. But that
> won't ever happen if we only request (for example) PHY lane 0.
> 
> It's also misleading that it's possible to specify only some subset of
> the PHY lanes, but the driver might still try to use them al
> 
> Brian
> 
>> +
>>   Optional Property:
>>   - aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
>>   	using 24MHz OSC for RC's PHY.
>> @@ -95,6 +105,7 @@ pcie0: pcie@f8000000 {
>>   		 <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
>>   	reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
>>   		      "pm", "pclk", "aclk";
>> +	/* deprecated legacy PHY model */
>>   	phys = <&pcie_phy>;
>>   	phy-names = "pcie-phy";
>>   	pinctrl-names = "default";
>> @@ -111,3 +122,13 @@ pcie0: pcie@f8000000 {
>>   		#interrupt-cells = <1>;
>>   	};
>>   };
>> +
>> +pcie0: pcie@f8000000 {
>> +	...
>> +
>> +	/* preferred per-lane PHY model */
>> +	phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
>> +	phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
>> +
>> +	...
>> +};
>> -- 
>> 1.9.1
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
>