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

[RFC PATCH v2 0/7] 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 it would be nice if you
can test it with your chrome devices running v4.12.

Hi Rob,

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


Changes in v2:
- deprecate legacy PHY model
- improve rockchip_pcie_phy_of_xlate
- fix wrong calculation of pwr_cnt and add new init_cnt
- add internal locking
- introduce per-lane data to simply the code
- convert that for all rk3399 platforms
- make listing all 4 lanes as mandatory

Shawn Lin (7):
  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
  dt-bindings: PCI: rockchip: convert to use per-lane PHY model
  Documentation: bindings: convert to use per-lane Rockchip PCIe PHY

 .../devicetree/bindings/pci/rockchip-pcie.txt      |  25 ++-
 .../devicetree/bindings/phy/rockchip-pcie-phy.txt  |   7 +-
 arch/arm64/boot/dts/rockchip/rk3399.dtsi           |   8 +-
 drivers/pci/host/pcie-rockchip.c                   | 168 ++++++++++++++++++---
 drivers/phy/rockchip/phy-rockchip-pcie.c           | 128 ++++++++++++++--
 5 files changed, 297 insertions(+), 39 deletions(-)

-- 
1.9.1

[PATCH 1/7] 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

[PATCH 1/7] 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@rock-chips.com>
---

 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

[PATCH 2/7] 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

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

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 6632a51..f755df5 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 *phy_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,104 @@ 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_get(dev, "pcie-phy");
+	if (IS_ERR(rockchip->phy)) {
+		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
+			return PTR_ERR(rockchip->phy);
+		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
+	} else {
+		rockchip->legacy_phy = true;
+		dev_warn(dev, "legacy phy model is deprecated!\n");
+		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)) {
+			if (PTR_ERR(phy) != -EPROBE_DEFER)
+				dev_err(dev, "missing phy for lane %d: %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:
+			if (phy->init_count > phy_num)
+				continue;
+			err = phy_init(phy);
+			break;
+		case PHY_PWR_ON:
+			if (phy->power_count > phy_num)
+				continue;
+			err = phy_power_on(phy);
+			break;
+		case PHY_PWR_OFF:
+			if (!phy->power_count)
+				continue;
+			err = phy_power_off(phy);
+			break;
+		case PHY_EXIT:
+			if (!phy->init_count)
+				continue;
+			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",
+					phy_ops_name[type], err);
+			else
+				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
+					phy_ops_name[type], i, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @rockchip: PCIe port information
@@ -537,11 +647,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 +710,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 +959,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 +1387,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 +1630,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


--
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

[PATCH 2/7] 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 | 144 ++++++++++++++++++++++++++++++++-------
 1 file changed, 118 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 6632a51..f755df5 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 *phy_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,104 @@ 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_get(dev, "pcie-phy");
+	if (IS_ERR(rockchip->phy)) {
+		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
+			return PTR_ERR(rockchip->phy);
+		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
+	} else {
+		rockchip->legacy_phy = true;
+		dev_warn(dev, "legacy phy model is deprecated!\n");
+		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)) {
+			if (PTR_ERR(phy) != -EPROBE_DEFER)
+				dev_err(dev, "missing phy for lane %d: %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:
+			if (phy->init_count > phy_num)
+				continue;
+			err = phy_init(phy);
+			break;
+		case PHY_PWR_ON:
+			if (phy->power_count > phy_num)
+				continue;
+			err = phy_power_on(phy);
+			break;
+		case PHY_PWR_OFF:
+			if (!phy->power_count)
+				continue;
+			err = phy_power_off(phy);
+			break;
+		case PHY_EXIT:
+			if (!phy->init_count)
+				continue;
+			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",
+					phy_ops_name[type], err);
+			else
+				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
+					phy_ops_name[type], i, err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @rockchip: PCIe port information
@@ -537,11 +647,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 +710,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 +959,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 +1387,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 +1630,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

[PATCH 3/7] 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@rock-chips.com>
---

 drivers/phy/rockchip/phy-rockchip-pcie.c | 126 +++++++++++++++++++++++++++----
 1 file changed, 112 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 6904633..f48b188 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -70,13 +70,46 @@ struct rockchip_pcie_data {
 	unsigned int pcie_laneoff;
 };
 
+struct phy_pcie_instance;
+
+/* internal lock for multiple phys */
+DEFINE_MUTEX(pcie_mutex);
+
 struct rockchip_pcie_phy {
 	struct rockchip_pcie_data *phy_data;
 	struct regmap *reg_base;
+	struct phy_pcie_instance {
+		struct phy *phy;
+		u32 index;
+	} phys[PHY_MAX_LANE_NUM];
 	struct reset_control *phy_rst;
 	struct clk *clk_pciephy_ref;
+	int pwr_cnt;
+	int init_cnt;
 };
 
+static inline
+struct rockchip_pcie_phy *to_pcie_phy(struct phy_pcie_instance *inst)
+{
+	return container_of(inst, struct rockchip_pcie_phy,
+					phys[inst->index]);
+}
+
+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 (args->args_count == 0)
+		return rk_phy->phys[0].phy;
+
+	if (WARN_ON(args->args[0] > PHY_MAX_LANE_NUM))
+		return ERR_PTR(-ENODEV);
+
+	return rk_phy->phys[args->args[0]].phy;
+}
+
+
 static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
 			      u32 addr, u32 data)
 {
@@ -116,29 +149,65 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
 
 static int rockchip_pcie_phy_power_off(struct phy *phy)
 {
-	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
+	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 
+	mutex_lock(&pcie_mutex);
+
+	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 + inst->index));
+
+	if (--rk_phy->pwr_cnt)
+		goto err_out;
+
 	err = reset_control_assert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
-		return err;
+		goto err_restore;
 	}
 
+err_out:
+	mutex_unlock(&pcie_mutex);
 	return 0;
+
+err_restore:
+	++rk_phy->pwr_cnt;
+	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 + inst->index));
+	mutex_unlock(&pcie_mutex);
+	return err;
 }
 
 static int rockchip_pcie_phy_power_on(struct phy *phy)
 {
-	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
+	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 	u32 status;
 	unsigned long timeout;
 
+	mutex_lock(&pcie_mutex);
+
+	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 + inst->index));
+
+	if (rk_phy->pwr_cnt++)
+		goto err_out;
+
 	err = reset_control_deassert(rk_phy->phy_rst);
 	if (err) {
 		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
-		return err;
+		goto err_pwr_cnt;
 	}
 
 	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
@@ -214,18 +283,29 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 		goto err_pll_lock;
 	}
 
+err_out:
+	mutex_unlock(&pcie_mutex);
 	return 0;
 
 err_pll_lock:
 	reset_control_assert(rk_phy->phy_rst);
+err_pwr_cnt:
+	--rk_phy->pwr_cnt;
+	mutex_unlock(&pcie_mutex);
 	return err;
 }
 
 static int rockchip_pcie_phy_init(struct phy *phy)
 {
-	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
+	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
 	int err = 0;
 
+	mutex_lock(&pcie_mutex);
+
+	if (rk_phy->init_cnt++)
+		goto err_out;
+
 	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
 	if (err) {
 		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
@@ -238,20 +318,33 @@ static int rockchip_pcie_phy_init(struct phy *phy)
 		goto err_reset;
 	}
 
-	return err;
+err_out:
+	mutex_unlock(&pcie_mutex);
+	return 0;
 
 err_reset:
+
 	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
 err_refclk:
+	--rk_phy->init_cnt;
+	mutex_unlock(&pcie_mutex);
 	return err;
 }
 
 static int rockchip_pcie_phy_exit(struct phy *phy)
 {
-	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
+	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
+	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
+
+	mutex_lock(&pcie_mutex);
+
+	if (--rk_phy->init_cnt)
+		goto err_init_cnt;
 
 	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
 
+err_init_cnt:
+	mutex_unlock(&pcie_mutex);
 	return 0;
 }
 
@@ -283,10 +376,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 +412,19 @@ 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);
+	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
+		rk_phy->phys[i].phy = devm_phy_create(dev, dev->of_node, &ops);
+		if (IS_ERR(rk_phy->phys[i].phy)) {
+			dev_err(dev, "failed to create PHY%d\n", i);
+			return PTR_ERR(rk_phy->phys[i].phy);
+		}
+		rk_phy->phys[i].index = i;
+		phy_set_drvdata(rk_phy->phys[i].phy, &rk_phy->phys[i]);
 	}
 
-	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

[PATCH 4/7] 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 f755df5..fa30ffb 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)
 {
@@ -624,7 +640,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);
@@ -797,6 +814,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_dbg(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

[PATCH 4/7] 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@rock-chips.com>
---

 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 f755df5..fa30ffb 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)
 {
@@ -624,7 +640,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);
@@ -797,6 +814,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_dbg(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

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

[Shawn Lin]

Convert all RK3399 platforms to use per-lane PHY model in
order to save more power by idling the unused lane(s).

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

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 69c56f7..5b78ce1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -238,8 +238,10 @@
 		linux,pci-domain = <0>;
 		max-link-speed = <1>;
 		msi-map = <0x0 &its 0x0 0x1000>;
-		phys = <&pcie_phy>;
-		phy-names = "pcie-phy";
+		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";
 		ranges = <0x83000000 0x0 0xfa000000 0x0 0xfa000000 0x0 0x1e00000
 			  0x81000000 0x0 0xfbe00000 0x0 0xfbe00000 0x0 0x100000>;
 		resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
@@ -1295,7 +1297,7 @@
 			compatible = "rockchip,rk3399-pcie-phy";
 			clocks = <&cru SCLK_PCIEPHY_REF>;
 			clock-names = "refclk";
-			#phy-cells = <0>;
+			#phy-cells = <1>;
 			resets = <&cru SRST_PCIEPHY>;
 			reset-names = "phy";
 			status = "disabled";
-- 
1.9.1

[PATCH 6/7] 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..41bced4 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 (preferred):
+- phys: Must contain an phandle to a PHY for each entry in phy-names.
+- phy-names: Must include 4 entries for all 4 lanes even if some of
+  them won't be used for your cases. Entries are of the form "pcie-phy-N":
+  where N ranges from 0 to 3.
+  (see example below and you MUST also refer to ../phy/rockchip-pcie-phy.txt
+  for changing the #phy-cells of phy node to support it)
+
 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

[PATCH 6/7] 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@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..41bced4 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 (preferred):
+- phys: Must contain an phandle to a PHY for each entry in phy-names.
+- phy-names: Must include 4 entries for all 4 lanes even if some of
+  them won't be used for your cases. Entries are of the form "pcie-phy-N":
+  where N ranges from 0 to 3.
+  (see example below and you MUST also refer to ../phy/rockchip-pcie-phy.txt
+  for changing the #phy-cells of phy node to support it)
+
 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

[PATCH 7/7] Documentation: bindings: convert to use per-lane Rockchip PCIe PHY

[Shawn Lin]

This patch deprecate the legacy PCIe PHY and encourage user
to use per-lane PHY mode by setting #phy-cells to 1.

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

 Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt
index 0f6222a..b496042 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt
@@ -3,7 +3,6 @@ Rockchip PCIE PHY
 
 Required properties:
  - compatible: rockchip,rk3399-pcie-phy
- - #phy-cells: must be 0
  - clocks: Must contain an entry in clock-names.
 	See ../clocks/clock-bindings.txt for details.
  - clock-names: Must be "refclk"
@@ -11,6 +10,12 @@ Required properties:
 	See ../reset/reset.txt for details.
  - reset-names: Must be "phy"
 
+Required properties for legacy PHY mode (deprecated):
+ - #phy-cells: must be 0
+
+Required properties for per-lane PHY mode (preferred):
+ - #phy-cells: must be 1
+
 Example:
 
 grf: syscon@ff770000 {
-- 
1.9.1

Re: [RFC PATCH v2 0/7] Reconstruct rockchip's PCIe and PCIe-PHY driver for per-lane PHY model

[jeffy]

Hi guys,

On 07/17/2017 03:36 PM, Shawn Lin wrote:
> 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 it would be nice if you
> can test it with your chrome devices running v4.12.
I tested these patches on my chromebook bob(based on next-20170714), 
pcie/pcie wifi(mrvl 8997) work well, and bind/unbind suspend/resume 
shows unused lanes powered off as expected.

Tested-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

>
> Hi Rob,
>
> Does the changes for rockchip-pcie.txt in patch 6 & 7 look good to you
> from the perspective of DT?
>
>
> Changes in v2:
> - deprecate legacy PHY model
> - improve rockchip_pcie_phy_of_xlate
> - fix wrong calculation of pwr_cnt and add new init_cnt
> - add internal locking
> - introduce per-lane data to simply the code
> - convert that for all rk3399 platforms
> - make listing all 4 lanes as mandatory
>
> Shawn Lin (7):
>    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
>    dt-bindings: PCI: rockchip: convert to use per-lane PHY model
>    Documentation: bindings: convert to use per-lane Rockchip PCIe PHY
>
>   .../devicetree/bindings/pci/rockchip-pcie.txt      |  25 ++-
>   .../devicetree/bindings/phy/rockchip-pcie-phy.txt  |   7 +-
>   arch/arm64/boot/dts/rockchip/rk3399.dtsi           |   8 +-
>   drivers/pci/host/pcie-rockchip.c                   | 168 ++++++++++++++++++---
>   drivers/phy/rockchip/phy-rockchip-pcie.c           | 128 ++++++++++++++--
>   5 files changed, 297 insertions(+), 39 deletions(-)
>


--
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 v2 0/7] Reconstruct rockchip's PCIe and PCIe-PHY driver for per-lane PHY model

[jeffy]

Hi guys,

On 07/17/2017 03:36 PM, Shawn Lin wrote:
> 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 it would be nice if you
> can test it with your chrome devices running v4.12.
I tested these patches on my chromebook bob(based on next-20170714), 
pcie/pcie wifi(mrvl 8997) work well, and bind/unbind suspend/resume 
shows unused lanes powered off as expected.

Tested-by: Jeffy Chen <jeffy.chen@rock-chips.com>

>
> Hi Rob,
>
> Does the changes for rockchip-pcie.txt in patch 6 & 7 look good to you
> from the perspective of DT?
>
>
> Changes in v2:
> - deprecate legacy PHY model
> - improve rockchip_pcie_phy_of_xlate
> - fix wrong calculation of pwr_cnt and add new init_cnt
> - add internal locking
> - introduce per-lane data to simply the code
> - convert that for all rk3399 platforms
> - make listing all 4 lanes as mandatory
>
> Shawn Lin (7):
>    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
>    dt-bindings: PCI: rockchip: convert to use per-lane PHY model
>    Documentation: bindings: convert to use per-lane Rockchip PCIe PHY
>
>   .../devicetree/bindings/pci/rockchip-pcie.txt      |  25 ++-
>   .../devicetree/bindings/phy/rockchip-pcie-phy.txt  |   7 +-
>   arch/arm64/boot/dts/rockchip/rk3399.dtsi           |   8 +-
>   drivers/pci/host/pcie-rockchip.c                   | 168 ++++++++++++++++++---
>   drivers/phy/rockchip/phy-rockchip-pcie.c           | 128 ++++++++++++++--
>   5 files changed, 297 insertions(+), 39 deletions(-)
>

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

[Brian Norris]

Hi Shawn,

On Mon, Jul 17, 2017 at 03:36:18PM +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 | 126 +++++++++++++++++++++++++++----
>  1 file changed, 112 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 6904633..f48b188 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -70,13 +70,46 @@ struct rockchip_pcie_data {
>  	unsigned int pcie_laneoff;
>  };
>  
> +struct phy_pcie_instance;

Is this forward declaration actually needed? You have it defined just a
few lines below.

> +
> +/* internal lock for multiple phys */
> +DEFINE_MUTEX(pcie_mutex);

Put this inside struct rockchip_pcie_phy. It shouldn't be a global lock;
it's just for coordinating the 4 lanes within a single
"rockchip_pcie_phy".

> +
>  struct rockchip_pcie_phy {
>  	struct rockchip_pcie_data *phy_data;
>  	struct regmap *reg_base;
> +	struct phy_pcie_instance {
> +		struct phy *phy;
> +		u32 index;
> +	} phys[PHY_MAX_LANE_NUM];
>  	struct reset_control *phy_rst;
>  	struct clk *clk_pciephy_ref;
> +	int pwr_cnt;
> +	int init_cnt;
>  };
>  
> +static inline
> +struct rockchip_pcie_phy *to_pcie_phy(struct phy_pcie_instance *inst)
> +{
> +	return container_of(inst, struct rockchip_pcie_phy,
> +					phys[inst->index]);
> +}
> +
> +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 (args->args_count == 0)
> +		return rk_phy->phys[0].phy;
> +
> +	if (WARN_ON(args->args[0] > PHY_MAX_LANE_NUM))

This should be ">=", not ">" (an index of PHY_MAX_LANE_NUM would be out
of bounds).

> +		return ERR_PTR(-ENODEV);
> +
> +	return rk_phy->phys[args->args[0]].phy;
> +}
> +
> +
>  static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>  			      u32 addr, u32 data)
>  {
> @@ -116,29 +149,65 @@ static inline u32 phy_rd_cfg(struct rockchip_pcie_phy *rk_phy,
>  
>  static int rockchip_pcie_phy_power_off(struct phy *phy)
>  {
> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
>  	int err = 0;
>  
> +	mutex_lock(&pcie_mutex);
> +
> +	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 + inst->index));
> +
> +	if (--rk_phy->pwr_cnt)
> +		goto err_out;
> +
>  	err = reset_control_assert(rk_phy->phy_rst);
>  	if (err) {
>  		dev_err(&phy->dev, "assert phy_rst err %d\n", err);
> -		return err;
> +		goto err_restore;
>  	}
>  
> +err_out:
> +	mutex_unlock(&pcie_mutex);
>  	return 0;
> +
> +err_restore:
> +	++rk_phy->pwr_cnt;
> +	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 + inst->index));
> +	mutex_unlock(&pcie_mutex);
> +	return err;
>  }
>  
>  static int rockchip_pcie_phy_power_on(struct phy *phy)
>  {
> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
>  	int err = 0;
>  	u32 status;
>  	unsigned long timeout;
>  
> +	mutex_lock(&pcie_mutex);
> +
> +	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 + inst->index));

It seems a little weird to do this before deasserting the reset, but
only on the first lane to powered on. Should this actually be the last
step in this function? Or does it really not matter?

Brian

> +
> +	if (rk_phy->pwr_cnt++)
> +		goto err_out;
> +
>  	err = reset_control_deassert(rk_phy->phy_rst);
>  	if (err) {
>  		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
> -		return err;
> +		goto err_pwr_cnt;
>  	}
>  
>  	regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
> @@ -214,18 +283,29 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>  		goto err_pll_lock;
>  	}
>  
> +err_out:
> +	mutex_unlock(&pcie_mutex);
>  	return 0;
>  
>  err_pll_lock:
>  	reset_control_assert(rk_phy->phy_rst);
> +err_pwr_cnt:
> +	--rk_phy->pwr_cnt;
> +	mutex_unlock(&pcie_mutex);
>  	return err;
>  }
>  
>  static int rockchip_pcie_phy_init(struct phy *phy)
>  {
> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
>  	int err = 0;
>  
> +	mutex_lock(&pcie_mutex);
> +
> +	if (rk_phy->init_cnt++)
> +		goto err_out;
> +
>  	err = clk_prepare_enable(rk_phy->clk_pciephy_ref);
>  	if (err) {
>  		dev_err(&phy->dev, "Fail to enable pcie ref clock.\n");
> @@ -238,20 +318,33 @@ static int rockchip_pcie_phy_init(struct phy *phy)
>  		goto err_reset;
>  	}
>  
> -	return err;
> +err_out:
> +	mutex_unlock(&pcie_mutex);
> +	return 0;
>  
>  err_reset:
> +
>  	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
>  err_refclk:
> +	--rk_phy->init_cnt;
> +	mutex_unlock(&pcie_mutex);
>  	return err;
>  }
>  
>  static int rockchip_pcie_phy_exit(struct phy *phy)
>  {
> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
> +
> +	mutex_lock(&pcie_mutex);
> +
> +	if (--rk_phy->init_cnt)
> +		goto err_init_cnt;
>  
>  	clk_disable_unprepare(rk_phy->clk_pciephy_ref);
>  
> +err_init_cnt:
> +	mutex_unlock(&pcie_mutex);
>  	return 0;
>  }
>  
> @@ -283,10 +376,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 +412,19 @@ 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);
> +	for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> +		rk_phy->phys[i].phy = devm_phy_create(dev, dev->of_node, &ops);
> +		if (IS_ERR(rk_phy->phys[i].phy)) {
> +			dev_err(dev, "failed to create PHY%d\n", i);
> +			return PTR_ERR(rk_phy->phys[i].phy);
> +		}
> +		rk_phy->phys[i].index = i;
> +		phy_set_drvdata(rk_phy->phys[i].phy, &rk_phy->phys[i]);
>  	}
>  
> -	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: [PATCH 6/7] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

[Rob Herring]

On Mon, Jul 17, 2017 at 03:38:41PM +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(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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: [PATCH 6/7] dt-bindings: PCI: rockchip: convert to use per-lane PHY model

[Rob Herring]

On Mon, Jul 17, 2017 at 03:38:41PM +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(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 7/7] Documentation: bindings: convert to use per-lane Rockchip PCIe PHY

[Rob Herring]

On Mon, Jul 17, 2017 at 03:38:42PM +0800, Shawn Lin wrote:
> This patch deprecate the legacy PCIe PHY and encourage user
> to use per-lane PHY mode by setting #phy-cells to 1.

Can we have some consistency in the subjects? For this one: 
"dt-bindings: phy: ..."

> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
>  Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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: [PATCH 7/7] Documentation: bindings: convert to use per-lane Rockchip PCIe PHY

[Rob Herring]

On Mon, Jul 17, 2017 at 03:38:42PM +0800, Shawn Lin wrote:
> This patch deprecate the legacy PCIe PHY and encourage user
> to use per-lane PHY mode by setting #phy-cells to 1.

Can we have some consistency in the subjects? For this one: 
"dt-bindings: phy: ..."

> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/phy/rockchip-pcie-phy.txt | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/7] PCI: rockchip: introduce per-lanes PHYs support

[Brian Norris]

On Mon, Jul 17, 2017 at 03:36:17PM +0800, Shawn Lin wrote:
> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> 
>  drivers/pci/host/pcie-rockchip.c | 144 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 118 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 6632a51..f755df5 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;

Would this be simpler as just an array?

	struct phy *phys[MAX_LANE_NUM};

You can probably also combine it with the 'phy' field, and just treat it
differently based on the 'legacy_phy' switch.

>  	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 *phy_ops_name[] = {"init", "power on", "power off", "exit"};

This could be 'static'. But if it's only for printing error
messages...do you really even need this? Somebody could easily refer
back to the driver if they need to convert an int/enum to something
meaningful.

> +
>  static u32 rockchip_pcie_read(struct rockchip_pcie *rockchip, u32 reg)
>  {
>  	return readl(rockchip->apb_base + reg);
> @@ -507,6 +519,104 @@ 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_get(dev, "pcie-phy");
> +	if (IS_ERR(rockchip->phy)) {
> +		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
> +			return PTR_ERR(rockchip->phy);
> +		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
> +	} else {
> +		rockchip->legacy_phy = true;
> +		dev_warn(dev, "legacy phy model is deprecated!\n");
> +		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);

Since the string is constant, this could just be:

		kasprintf(..., "pcie-phy-%u", i);

> +		if (!name)
> +			return -ENOMEM;
> +
> +		phy = devm_of_phy_get(rockchip->dev,
> +				      rockchip->dev->of_node, name);
> +		kfree(name);
> +
> +		if (IS_ERR(phy)) {
> +			if (PTR_ERR(phy) != -EPROBE_DEFER)
> +				dev_err(dev, "missing phy for lane %d: %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:
> +			if (phy->init_count > phy_num)

This looks illegal and badly designed. Illegal, because this looks to be
a count that should only be touched by the PHY core (and protected by
its mutex). Badly designed, because this is *not* the right way to
handle overflow/underflow. You should make sure that this driver does
init/exit and power on/off in a properly-balanced fashion. And that
doesn't mean just "skip" it when the count is too high; it means this
driver should know on its own when is the right time to power on/off the
phy/lane.

And this logic doesn't even make sense. Why should I be allowed to init
lane 0 only once, but I can init lane 3 up to 4 times?

> +				continue;
> +			err = phy_init(phy);
> +			break;
> +		case PHY_PWR_ON:
> +			if (phy->power_count > phy_num)

Same goes here.

> +				continue;
> +			err = phy_power_on(phy);
> +			break;
> +		case PHY_PWR_OFF:
> +			if (!phy->power_count)

And here.

> +				continue;
> +			err = phy_power_off(phy);
> +			break;
> +		case PHY_EXIT:
> +			if (!phy->init_count)

And here.

> +				continue;
> +			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",
> +					phy_ops_name[type], err);
> +			else
> +				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
> +					phy_ops_name[type], i, err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

Really, I think you should kill this whole function, and code the loop
elsewhere.

> +
>  /**
>   * rockchip_pcie_init_port - Initialize hardware
>   * @rockchip: PCIe port information
> @@ -537,11 +647,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 +710,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 +959,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 +1387,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);

What's wrong with this (once you unify the 'phys' array)?

	for (i = 0; i < MAX_LANE_NUM; i++) {
		if (rockchip->phys[i]) { // Not necessarily required;
					 // phy APIs handle NULL
			phy_power_off(rockchip->phys[i]);
			phy_exit(rockchip->phys[i]);
		}
	}

Similar for all other uses, AFAICT.

Once you actually need to prevent multiple power-offs (when you power
down some lanes, but not others), you can avoid the illegal access to
PHY-internal counters by just keeping your own mask of on/off PHYs.

>  
>  	clk_disable_unprepare(rockchip->clk_pcie_pm);
>  	clk_disable_unprepare(rockchip->hclk_pcie);
> @@ -1538,8 +1630,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);

Same here.

Brian

>  
>  	clk_disable_unprepare(rockchip->clk_pcie_pm);
>  	clk_disable_unprepare(rockchip->hclk_pcie);
> -- 
> 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: [PATCH 2/7] PCI: rockchip: introduce per-lanes PHYs support

[Brian Norris]

On Mon, Jul 17, 2017 at 03:36:17PM +0800, Shawn Lin wrote:
> 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 | 144 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 118 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 6632a51..f755df5 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;

Would this be simpler as just an array?

	struct phy *phys[MAX_LANE_NUM};

You can probably also combine it with the 'phy' field, and just treat it
differently based on the 'legacy_phy' switch.

>  	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 *phy_ops_name[] = {"init", "power on", "power off", "exit"};

This could be 'static'. But if it's only for printing error
messages...do you really even need this? Somebody could easily refer
back to the driver if they need to convert an int/enum to something
meaningful.

> +
>  static u32 rockchip_pcie_read(struct rockchip_pcie *rockchip, u32 reg)
>  {
>  	return readl(rockchip->apb_base + reg);
> @@ -507,6 +519,104 @@ 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_get(dev, "pcie-phy");
> +	if (IS_ERR(rockchip->phy)) {
> +		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
> +			return PTR_ERR(rockchip->phy);
> +		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
> +	} else {
> +		rockchip->legacy_phy = true;
> +		dev_warn(dev, "legacy phy model is deprecated!\n");
> +		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);

Since the string is constant, this could just be:

		kasprintf(..., "pcie-phy-%u", i);

> +		if (!name)
> +			return -ENOMEM;
> +
> +		phy = devm_of_phy_get(rockchip->dev,
> +				      rockchip->dev->of_node, name);
> +		kfree(name);
> +
> +		if (IS_ERR(phy)) {
> +			if (PTR_ERR(phy) != -EPROBE_DEFER)
> +				dev_err(dev, "missing phy for lane %d: %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:
> +			if (phy->init_count > phy_num)

This looks illegal and badly designed. Illegal, because this looks to be
a count that should only be touched by the PHY core (and protected by
its mutex). Badly designed, because this is *not* the right way to
handle overflow/underflow. You should make sure that this driver does
init/exit and power on/off in a properly-balanced fashion. And that
doesn't mean just "skip" it when the count is too high; it means this
driver should know on its own when is the right time to power on/off the
phy/lane.

And this logic doesn't even make sense. Why should I be allowed to init
lane 0 only once, but I can init lane 3 up to 4 times?

> +				continue;
> +			err = phy_init(phy);
> +			break;
> +		case PHY_PWR_ON:
> +			if (phy->power_count > phy_num)

Same goes here.

> +				continue;
> +			err = phy_power_on(phy);
> +			break;
> +		case PHY_PWR_OFF:
> +			if (!phy->power_count)

And here.

> +				continue;
> +			err = phy_power_off(phy);
> +			break;
> +		case PHY_EXIT:
> +			if (!phy->init_count)

And here.

> +				continue;
> +			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",
> +					phy_ops_name[type], err);
> +			else
> +				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
> +					phy_ops_name[type], i, err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

Really, I think you should kill this whole function, and code the loop
elsewhere.

> +
>  /**
>   * rockchip_pcie_init_port - Initialize hardware
>   * @rockchip: PCIe port information
> @@ -537,11 +647,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 +710,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 +959,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 +1387,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);

What's wrong with this (once you unify the 'phys' array)?

	for (i = 0; i < MAX_LANE_NUM; i++) {
		if (rockchip->phys[i]) { // Not necessarily required;
					 // phy APIs handle NULL
			phy_power_off(rockchip->phys[i]);
			phy_exit(rockchip->phys[i]);
		}
	}

Similar for all other uses, AFAICT.

Once you actually need to prevent multiple power-offs (when you power
down some lanes, but not others), you can avoid the illegal access to
PHY-internal counters by just keeping your own mask of on/off PHYs.

>  
>  	clk_disable_unprepare(rockchip->clk_pcie_pm);
>  	clk_disable_unprepare(rockchip->hclk_pcie);
> @@ -1538,8 +1630,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);

Same here.

Brian

>  
>  	clk_disable_unprepare(rockchip->clk_pcie_pm);
>  	clk_disable_unprepare(rockchip->hclk_pcie);
> -- 
> 1.9.1
> 
> 

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

[Shawn Lin]

Hi Brian,

On 2017/7/18 2:39, Brian Norris wrote:
> Hi Shawn,
> 
> On Mon, Jul 17, 2017 at 03:36:18PM +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 | 126 +++++++++++++++++++++++++++----
>>   1 file changed, 112 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> index 6904633..f48b188 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> @@ -70,13 +70,46 @@ struct rockchip_pcie_data {
>>   	unsigned int pcie_laneoff;
>>   };
>>   
>> +struct phy_pcie_instance;
> 
> Is this forward declaration actually needed? You have it defined just a
> few lines below.
> 

I will remove this one.

>> +
>> +/* internal lock for multiple phys */
>> +DEFINE_MUTEX(pcie_mutex);
> 
> Put this inside struct rockchip_pcie_phy. It shouldn't be a global lock;
> it's just for coordinating the 4 lanes within a single
> "rockchip_pcie_phy".
> 

okay.

>> +
>>   struct rockchip_pcie_phy {
>>   	struct rockchip_pcie_data *phy_data;
>>   	struct regmap *reg_base;
>> +	struct phy_pcie_instance {
>> +		struct phy *phy;
>> +		u32 index;
>> +	} phys[PHY_MAX_LANE_NUM];
>>   	struct reset_control *phy_rst;
>>   	struct clk *clk_pciephy_ref;
>> +	int pwr_cnt;
>> +	int init_cnt;
>>   };
>>   
>> +static inline
>> +struct rockchip_pcie_phy *to_pcie_phy(struct phy_pcie_instance *inst)
>> +{
>> +	return container_of(inst, struct rockchip_pcie_phy,
>> +					phys[inst->index]);
>> +}
>> +
>> +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 (args->args_count == 0)
>> +		return rk_phy->phys[0].phy;
>> +
>> +	if (WARN_ON(args->args[0] > PHY_MAX_LANE_NUM))
> 
> This should be ">=", not ">" (an index of PHY_MAX_LANE_NUM would be out
> of bounds).

will fix.

> 
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	return rk_phy->phys[args->args[0]].phy;
>> +}
>> +
>> +
>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>   			      u32 addr, u32 data)

[...]

>>   }
>>   
>>   static int rockchip_pcie_phy_power_on(struct phy *phy)
>>   {
>> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
>> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
>>   	int err = 0;
>>   	u32 status;
>>   	unsigned long timeout;
>>   
>> +	mutex_lock(&pcie_mutex);
>> +
>> +	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 + inst->index));
> 
> It seems a little weird to do this before deasserting the reset, but
> only on the first lane to powered on. Should this actually be the last
> step in this function? Or does it really not matter?

It doesn't matter that we do this before deasserting the reset. I could
move this after the deasserting. However it shouldn't be the last setp.
If you only use lane 0 for your devcice, and it will be idle when doing
unbind since we call phy_power_off. Then bind will be failed as you
leave 4 lanes inactive finally which makes the phy fail to lock the PLL
internally. In other words, we should at least keep one active lane
when re-init the phy.


> 
> Brian
> 



--
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: [PATCH 3/7] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[Shawn Lin]

Hi Brian,

On 2017/7/18 2:39, Brian Norris wrote:
> Hi Shawn,
> 
> On Mon, Jul 17, 2017 at 03:36:18PM +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 | 126 +++++++++++++++++++++++++++----
>>   1 file changed, 112 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> index 6904633..f48b188 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
>> @@ -70,13 +70,46 @@ struct rockchip_pcie_data {
>>   	unsigned int pcie_laneoff;
>>   };
>>   
>> +struct phy_pcie_instance;
> 
> Is this forward declaration actually needed? You have it defined just a
> few lines below.
> 

I will remove this one.

>> +
>> +/* internal lock for multiple phys */
>> +DEFINE_MUTEX(pcie_mutex);
> 
> Put this inside struct rockchip_pcie_phy. It shouldn't be a global lock;
> it's just for coordinating the 4 lanes within a single
> "rockchip_pcie_phy".
> 

okay.

>> +
>>   struct rockchip_pcie_phy {
>>   	struct rockchip_pcie_data *phy_data;
>>   	struct regmap *reg_base;
>> +	struct phy_pcie_instance {
>> +		struct phy *phy;
>> +		u32 index;
>> +	} phys[PHY_MAX_LANE_NUM];
>>   	struct reset_control *phy_rst;
>>   	struct clk *clk_pciephy_ref;
>> +	int pwr_cnt;
>> +	int init_cnt;
>>   };
>>   
>> +static inline
>> +struct rockchip_pcie_phy *to_pcie_phy(struct phy_pcie_instance *inst)
>> +{
>> +	return container_of(inst, struct rockchip_pcie_phy,
>> +					phys[inst->index]);
>> +}
>> +
>> +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 (args->args_count == 0)
>> +		return rk_phy->phys[0].phy;
>> +
>> +	if (WARN_ON(args->args[0] > PHY_MAX_LANE_NUM))
> 
> This should be ">=", not ">" (an index of PHY_MAX_LANE_NUM would be out
> of bounds).

will fix.

> 
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	return rk_phy->phys[args->args[0]].phy;
>> +}
>> +
>> +
>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>   			      u32 addr, u32 data)

[...]

>>   }
>>   
>>   static int rockchip_pcie_phy_power_on(struct phy *phy)
>>   {
>> -	struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>> +	struct phy_pcie_instance *inst = phy_get_drvdata(phy);
>> +	struct rockchip_pcie_phy *rk_phy = to_pcie_phy(inst);
>>   	int err = 0;
>>   	u32 status;
>>   	unsigned long timeout;
>>   
>> +	mutex_lock(&pcie_mutex);
>> +
>> +	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 + inst->index));
> 
> It seems a little weird to do this before deasserting the reset, but
> only on the first lane to powered on. Should this actually be the last
> step in this function? Or does it really not matter?

It doesn't matter that we do this before deasserting the reset. I could
move this after the deasserting. However it shouldn't be the last setp.
If you only use lane 0 for your devcice, and it will be idle when doing
unbind since we call phy_power_off. Then bind will be failed as you
leave 4 lanes inactive finally which makes the phy fail to lock the PLL
internally. In other words, we should at least keep one active lane
when re-init the phy.


> 
> Brian
> 

Re: [PATCH 2/7] PCI: rockchip: introduce per-lanes PHYs support

[Shawn Lin]

Hi Brian,

On 2017/7/18 4:14, Brian Norris wrote:
> On Mon, Jul 17, 2017 at 03:36:17PM +0800, Shawn Lin wrote:
>> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>>   drivers/pci/host/pcie-rockchip.c | 144 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 118 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 6632a51..f755df5 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;
> 
> Would this be simpler as just an array?
> 
> 	struct phy *phys[MAX_LANE_NUM};
> 
> You can probably also combine it with the 'phy' field, and just treat it
> differently based on the 'legacy_phy' switch.

Make sense.

> 
>>   	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 *phy_ops_name[] = {"init", "power on", "power off", "exit"};
> 
> This could be 'static'. But if it's only for printing error
> messages...do you really even need this? Somebody could easily refer
> back to the driver if they need to convert an int/enum to something
> meaningful.

ok, I will kill all these above including the following ugly
rockchip_pcie_manipulate_phys.

> 
>> +
>>   static u32 rockchip_pcie_read(struct rockchip_pcie *rockchip, u32 reg)
>>   {
>>   	return readl(rockchip->apb_base + reg);
>> @@ -507,6 +519,104 @@ 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_get(dev, "pcie-phy");
>> +	if (IS_ERR(rockchip->phy)) {
>> +		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
>> +			return PTR_ERR(rockchip->phy);
>> +		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
>> +	} else {
>> +		rockchip->legacy_phy = true;
>> +		dev_warn(dev, "legacy phy model is deprecated!\n");
>> +		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);
> 
> Since the string is constant, this could just be:
> 
> 		kasprintf(..., "pcie-phy-%u", i);
> 

Looks good, and will improve it.

>> +		if (!name)
>> +			return -ENOMEM;
>> +
>> +		phy = devm_of_phy_get(rockchip->dev,
>> +				      rockchip->dev->of_node, name);
>> +		kfree(name);
>> +
>> +		if (IS_ERR(phy)) {
>> +			if (PTR_ERR(phy) != -EPROBE_DEFER)
>> +				dev_err(dev, "missing phy for lane %d: %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:
>> +			if (phy->init_count > phy_num)
> 
> This looks illegal and badly designed. Illegal, because this looks to be
> a count that should only be touched by the PHY core (and protected by
> its mutex). Badly designed, because this is *not* the right way to
> handle overflow/underflow. You should make sure that this driver does
> init/exit and power on/off in a properly-balanced fashion. And that
> doesn't mean just "skip" it when the count is too high; it means this
> driver should know on its own when is the right time to power on/off the
> phy/lane.

right,  init_cout/power_count should be kept inside the phy core but
we didn't want to duplicate it for consumer. I will try to kill all
of these.

> 
> And this logic doesn't even make sense. Why should I be allowed to init
> lane 0 only once, but I can init lane 3 up to 4 time >
>> +				continue;
>> +			err = phy_init(phy);
>> +			break;
>> +		case PHY_PWR_ON:
>> +			if (phy->power_count > phy_num)
> 
> Same goes here.
> 
>> +				continue;
>> +			err = phy_power_on(phy);
>> +			break;
>> +		case PHY_PWR_OFF:
>> +			if (!phy->power_count)
> 
> And here.
> 
>> +				continue;
>> +			err = phy_power_off(phy);
>> +			break;
>> +		case PHY_EXIT:
>> +			if (!phy->init_count)
> 
> And here.
> 
>> +				continue;
>> +			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",
>> +					phy_ops_name[type], err);
>> +			else
>> +				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
>> +					phy_ops_name[type], i, err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 
> Really, I think you should kill this whole function, and code the loop
> elsewhere.
> 
>> +
>>   /**
>>    * rockchip_pcie_init_port - Initialize hardware
>>    * @rockchip: PCIe port information
>> @@ -537,11 +647,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 +710,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 +959,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 +1387,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);
> 
> What's wrong with this (once you unify the 'phys' array)?
> 
> 	for (i = 0; i < MAX_LANE_NUM; i++) {
> 		if (rockchip->phys[i]) { // Not necessarily required;
> 					 // phy APIs handle NULL
> 			phy_power_off(rockchip->phys[i]);
> 			phy_exit(rockchip->phys[i]);
> 		}
> 	}
> 
> Similar for all other uses, AFAICT.
> 
> Once you actually need to prevent multiple power-offs (when you power
> down some lanes, but not others), you can avoid the illegal access to
> PHY-internal counters by just keeping your own mask of on/off PHYs.

I would like to see if I could reconstruct the phy consumer/provider
to avoid all these counters and fix all corner cases . :)


> 
>>   
>>   	clk_disable_unprepare(rockchip->clk_pcie_pm);
>>   	clk_disable_unprepare(rockchip->hclk_pcie);
>> @@ -1538,8 +1630,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);
> 
> Same here.
> 
> Brian
> 
>>   
>>   	clk_disable_unprepare(rockchip->clk_pcie_pm);
>>   	clk_disable_unprepare(rockchip->hclk_pcie);
>> -- 
>> 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: [PATCH 2/7] PCI: rockchip: introduce per-lanes PHYs support

[Shawn Lin]

Hi Brian,

On 2017/7/18 4:14, Brian Norris wrote:
> On Mon, Jul 17, 2017 at 03:36:17PM +0800, Shawn Lin wrote:
>> 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 | 144 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 118 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>> index 6632a51..f755df5 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;
> 
> Would this be simpler as just an array?
> 
> 	struct phy *phys[MAX_LANE_NUM};
> 
> You can probably also combine it with the 'phy' field, and just treat it
> differently based on the 'legacy_phy' switch.

Make sense.

> 
>>   	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 *phy_ops_name[] = {"init", "power on", "power off", "exit"};
> 
> This could be 'static'. But if it's only for printing error
> messages...do you really even need this? Somebody could easily refer
> back to the driver if they need to convert an int/enum to something
> meaningful.

ok, I will kill all these above including the following ugly
rockchip_pcie_manipulate_phys.

> 
>> +
>>   static u32 rockchip_pcie_read(struct rockchip_pcie *rockchip, u32 reg)
>>   {
>>   	return readl(rockchip->apb_base + reg);
>> @@ -507,6 +519,104 @@ 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_get(dev, "pcie-phy");
>> +	if (IS_ERR(rockchip->phy)) {
>> +		if (PTR_ERR(rockchip->phy) == -EPROBE_DEFER)
>> +			return PTR_ERR(rockchip->phy);
>> +		dev_dbg(dev, "missing legacy phy, and search for per-lane PHY\n");
>> +	} else {
>> +		rockchip->legacy_phy = true;
>> +		dev_warn(dev, "legacy phy model is deprecated!\n");
>> +		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);
> 
> Since the string is constant, this could just be:
> 
> 		kasprintf(..., "pcie-phy-%u", i);
> 

Looks good, and will improve it.

>> +		if (!name)
>> +			return -ENOMEM;
>> +
>> +		phy = devm_of_phy_get(rockchip->dev,
>> +				      rockchip->dev->of_node, name);
>> +		kfree(name);
>> +
>> +		if (IS_ERR(phy)) {
>> +			if (PTR_ERR(phy) != -EPROBE_DEFER)
>> +				dev_err(dev, "missing phy for lane %d: %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:
>> +			if (phy->init_count > phy_num)
> 
> This looks illegal and badly designed. Illegal, because this looks to be
> a count that should only be touched by the PHY core (and protected by
> its mutex). Badly designed, because this is *not* the right way to
> handle overflow/underflow. You should make sure that this driver does
> init/exit and power on/off in a properly-balanced fashion. And that
> doesn't mean just "skip" it when the count is too high; it means this
> driver should know on its own when is the right time to power on/off the
> phy/lane.

right,  init_cout/power_count should be kept inside the phy core but
we didn't want to duplicate it for consumer. I will try to kill all
of these.

> 
> And this logic doesn't even make sense. Why should I be allowed to init
> lane 0 only once, but I can init lane 3 up to 4 time >
>> +				continue;
>> +			err = phy_init(phy);
>> +			break;
>> +		case PHY_PWR_ON:
>> +			if (phy->power_count > phy_num)
> 
> Same goes here.
> 
>> +				continue;
>> +			err = phy_power_on(phy);
>> +			break;
>> +		case PHY_PWR_OFF:
>> +			if (!phy->power_count)
> 
> And here.
> 
>> +				continue;
>> +			err = phy_power_off(phy);
>> +			break;
>> +		case PHY_EXIT:
>> +			if (!phy->init_count)
> 
> And here.
> 
>> +				continue;
>> +			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",
>> +					phy_ops_name[type], err);
>> +			else
>> +				dev_err(dev, "fail to %s per-lane PHY#%u, err %d\n",
>> +					phy_ops_name[type], i, err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 
> Really, I think you should kill this whole function, and code the loop
> elsewhere.
> 
>> +
>>   /**
>>    * rockchip_pcie_init_port - Initialize hardware
>>    * @rockchip: PCIe port information
>> @@ -537,11 +647,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 +710,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 +959,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 +1387,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);
> 
> What's wrong with this (once you unify the 'phys' array)?
> 
> 	for (i = 0; i < MAX_LANE_NUM; i++) {
> 		if (rockchip->phys[i]) { // Not necessarily required;
> 					 // phy APIs handle NULL
> 			phy_power_off(rockchip->phys[i]);
> 			phy_exit(rockchip->phys[i]);
> 		}
> 	}
> 
> Similar for all other uses, AFAICT.
> 
> Once you actually need to prevent multiple power-offs (when you power
> down some lanes, but not others), you can avoid the illegal access to
> PHY-internal counters by just keeping your own mask of on/off PHYs.

I would like to see if I could reconstruct the phy consumer/provider
to avoid all these counters and fix all corner cases . :)


> 
>>   
>>   	clk_disable_unprepare(rockchip->clk_pcie_pm);
>>   	clk_disable_unprepare(rockchip->hclk_pcie);
>> @@ -1538,8 +1630,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);
> 
> Same here.
> 
> Brian
> 
>>   
>>   	clk_disable_unprepare(rockchip->clk_pcie_pm);
>>   	clk_disable_unprepare(rockchip->hclk_pcie);
>> -- 
>> 1.9.1
>>
>>
> 
> 
>