[PATCH 0/9] PHY: Enhance Sierra SERDES

[PATCH 0/9] PHY: Enhance Sierra SERDES

[Kishon Vijay Abraham I]

Hi Vinod,

This series enhances Sierra SERDES for two things
1) Skip configuring SERDES if it's already configured by bootloader
2) Model PLLs within SERDES as DT in order to use any of the external
   clocks connected to the two external reference clock pins.

The DT binding in this series depends on
http://lore.kernel.org/r/1603898561-5142-1-git-send-email-sjakhade@cadence.com

Faiz Abbas (2):
  phy: ti: j721e-wiz: Don't configure wiz if its already configured
  phy: cadence: sierra: Don't configure if any plls are already locked

Kishon Vijay Abraham I (7):
  dt-bindings: phy: cadence-sierra: Add bindings for the PLLs within
    SERDES
  phy: ti: j721e-wiz: Get PHY properties only for "phy" subnode
  phy: cadence: cadence-sierra: Create PHY only for "phy" sub-nodes
  phy: cadence: Sierra: Fix PHY power_on sequence
  phy: cadence: sierra: Model reference receiver as clocks (gate clocks)
  phy: cadence: sierra: Model PLL_CMNLC and PLL_CMNLC1 as clocks (mux
    clocks)
  phy: cadence: sierra: Enable pll_cmnlc and pll_cmnlc1 clocks

 .../bindings/phy/phy-cadence-sierra.yaml      |  89 +++-
 drivers/phy/cadence/phy-cadence-sierra.c      | 499 ++++++++++++++++--
 drivers/phy/ti/phy-j721e-wiz.c                |  39 +-
 3 files changed, 565 insertions(+), 62 deletions(-)

-- 
2.17.1

[PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for the PLLs within SERDES

[Kishon Vijay Abraham I]

Add binding for the PLLs within SERDES.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/phy/phy-cadence-sierra.yaml      | 89 ++++++++++++++++++-
 1 file changed, 86 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
index d210843863df..f574b8ed358c 100644
--- a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
+++ b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
@@ -49,12 +49,14 @@ properties:
     const: serdes
 
   clocks:
-    maxItems: 2
+    maxItems: 4
 
   clock-names:
     items:
       - const: cmn_refclk_dig_div
       - const: cmn_refclk1_dig_div
+      - const: pll_cmnlc
+      - const: pll_cmnlc1
 
   cdns,autoconf:
     type: boolean
@@ -107,6 +109,58 @@ patternProperties:
 
     additionalProperties: false
 
+  "^refrcv1?$":
+    type: object
+    description: |
+      Reference receivers that enables routing external clocks to the alternate
+      PLLCMNLC.
+    properties:
+      clocks:
+        maxItems: 1
+        description: Phandle to clock nodes representing the input to the
+          reference receiver.
+
+      clock-names:
+        items:
+          - const: pll_refclk
+
+      "#clock-cells":
+        const: 0
+
+    required:
+      - clocks
+      - "#clock-cells"
+
+  "^pll_cmnlc1?$":
+    type: object
+    description: |
+      SERDES node should have subnodes for each of the PLLs present in
+      the SERDES.
+    properties:
+      clocks:
+        maxItems: 2
+        description: Phandle to clock nodes representing the two inputs to PLL.
+
+      clock-names:
+        items:
+          - const: pll_refclk
+          - const: refrcv
+
+      "#clock-cells":
+        const: 0
+
+      assigned-clocks:
+        maxItems: 1
+
+      assigned-clock-parents:
+        maxItems: 1
+
+    required:
+      - clocks
+      - "#clock-cells"
+      - assigned-clocks
+      - assigned-clock-parents
+
 required:
   - compatible
   - "#address-cells"
@@ -130,10 +184,39 @@ examples:
             reg = <0x0 0xfd240000 0x0 0x40000>;
             resets = <&phyrst 0>, <&phyrst 1>;
             reset-names = "sierra_reset", "sierra_apb";
-            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>;
-            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div";
+            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>, <&serdes_pll_cmnlc>, <&serdes_pll_cmnlc1>;
+            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div", "pll_cmnlc", "pll_cmnlc1";
             #address-cells = <1>;
             #size-cells = <0>;
+
+            serdes_refrcv: refrcv {
+                    clocks = <&pll0_refclk>;
+                    clock-names = "pll_refclk";
+                    #clock-cells = <0>;
+            };
+
+            serdes_refrcv1: refrcv1 {
+                    clocks = <&pll1_refclk>;
+                    clock-names = "pll_refclk";
+                    #clock-cells = <0>;
+            };
+
+            serdes_pll_cmnlc: pll_cmnlc {
+                    clocks = <&pll0_refclk>, <&serdes_refrcv1>;
+                    clock-names = "pll_refclk", "refrcv";
+                    #clock-cells = <0>;
+                    assigned-clocks = <&serdes_pll_cmnlc>;
+                    assigned-clock-parents = <&pll0_refclk>;
+            };
+
+            serdes_pll_cmnlc1: pll_cmnlc1 {
+                    clocks = <&pll1_refclk>, <&serdes_refrcv>;
+                    clock-names = "pll_refclk", "refrcv";
+                    #clock-cells = <0>;
+                    assigned-clocks = <&serdes_pll_cmnlc1>;
+                    assigned-clock-parents = <&pll1_refclk>;
+            };
+
             pcie0_phy0: phy@0 {
                 reg = <0>;
                 resets = <&phyrst 2>;
-- 
2.17.1

[PATCH 2/9] phy: ti: j721e-wiz: Get PHY properties only for "phy" subnode

[Kishon Vijay Abraham I]

"serdes" node (child node of WIZ) can have sub-nodes for representing links
or it can have sub-nodes for representing the various clocks within the
serdes. Instead of trying to read "reg" from every child node used for
assigning "lane_phy_type", read only if the child node's name is "phy".

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index c9cfafe89cbf..d57d29382ce4 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -787,6 +787,9 @@ static int wiz_get_lane_phy_types(struct device *dev, struct wiz *wiz)
 		u32 reg, num_lanes = 1, phy_type = PHY_NONE;
 		int ret, i;
 
+		if (!(of_node_name_eq(subnode, "phy")))
+			continue;
+
 		ret = of_property_read_u32(subnode, "reg", &reg);
 		if (ret) {
 			dev_err(dev,
-- 
2.17.1

[PATCH 3/9] phy: ti: j721e-wiz: Don't configure wiz if its already configured

[Kishon Vijay Abraham I]

From: Faiz Abbas <faiz_abbas@ti.com>

Serdes lanes might be shared between multiple cores in some usecases
and its not possible to lock PLLs for both the lanes independently
by the two cores. This requires a bootloader to configure both the
lanes at early boot time.

To handle this case, skip all configuration if any of the lanes has
already been enabled.

While we are here, also fix the wiz_init() to be called before the
of_platform_device_create() call.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 36 +++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index d57d29382ce4..9786e8aec252 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -816,13 +816,14 @@ static int wiz_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct platform_device *serdes_pdev;
+	bool already_configured = false;
 	struct device_node *child_node;
 	struct regmap *regmap;
 	struct resource res;
 	void __iomem *base;
 	struct wiz *wiz;
 	u32 num_lanes;
-	int ret;
+	int ret, val, i;
 
 	wiz = devm_kzalloc(dev, sizeof(*wiz), GFP_KERNEL);
 	if (!wiz)
@@ -944,10 +945,26 @@ static int wiz_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
-	ret = wiz_clock_init(wiz, node);
-	if (ret < 0) {
-		dev_warn(dev, "Failed to initialize clocks\n");
-		goto err_get_sync;
+	for (i = 0; i < wiz->num_lanes; i++) {
+		regmap_field_read(wiz->p_enable[i], &val);
+		if (val & (P_ENABLE | P_ENABLE_FORCE)) {
+			already_configured = true;
+			break;
+		}
+	}
+
+	if (!already_configured) {
+		ret = wiz_clock_init(wiz, node);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to initialize clocks\n");
+			goto err_get_sync;
+		}
+
+		ret = wiz_init(wiz);
+		if (ret) {
+			dev_err(dev, "WIZ initialization failed\n");
+			goto err_pdev_create;
+		}
 	}
 
 	serdes_pdev = of_platform_device_create(child_node, NULL, dev);
@@ -958,18 +975,9 @@ static int wiz_probe(struct platform_device *pdev)
 	}
 	wiz->serdes_pdev = serdes_pdev;
 
-	ret = wiz_init(wiz);
-	if (ret) {
-		dev_err(dev, "WIZ initialization failed\n");
-		goto err_wiz_init;
-	}
-
 	of_node_put(child_node);
 	return 0;
 
-err_wiz_init:
-	of_platform_device_destroy(&serdes_pdev->dev, NULL);
-
 err_pdev_create:
 	wiz_clock_cleanup(wiz, node);
 
-- 
2.17.1

[PATCH 4/9] phy: cadence: cadence-sierra: Create PHY only for "phy" sub-nodes

[Kishon Vijay Abraham I]

Cadence Sierra PHY driver registers PHY using devm_phy_create()
for all sub-nodes of Sierra device tree node. However Sierra device
tree node can have sub-nodes for the various clocks in addtion to the
PHY. Use devm_phy_create() only for nodes with name "phy" which
represent the actual PHY.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index 453ef26fa1c7..4429f41a8f58 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -573,6 +573,9 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	for_each_available_child_of_node(dn, child) {
 		struct phy *gphy;
 
+		if (!(of_node_name_eq(child, "phy")))
+			continue;
+
 		sp->phys[node].lnk_rst =
 			of_reset_control_array_get_exclusive(child);
 
-- 
2.17.1

[PATCH 5/9] phy: cadence: Sierra: Fix PHY power_on sequence

[Kishon Vijay Abraham I]

Commit 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
de-asserts PHY_RESET even before the configurations are loaded in
phy_init(). However PHY_RESET should be de-asserted only after
all the configurations has been initialized, instead of de-asserting
in probe. Fix it here.

Fixes: 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index 4429f41a8f58..e08548417bce 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -319,6 +319,12 @@ static int cdns_sierra_phy_on(struct phy *gphy)
 	u32 val;
 	int ret;
 
+	ret = reset_control_deassert(sp->phy_rst);
+	if (ret) {
+		dev_err(dev, "Failed to take the PHY out of reset\n");
+		return ret;
+	}
+
 	/* Take the PHY lane group out of reset */
 	ret = reset_control_deassert(ins->lnk_rst);
 	if (ret) {
@@ -621,7 +627,6 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
-	reset_control_deassert(sp->phy_rst);
 	return PTR_ERR_OR_ZERO(phy_provider);
 
 put_child:
-- 
2.17.1

[PATCH 6/9] phy: cadence: sierra: Don't configure if any plls are already locked

[Kishon Vijay Abraham I]

From: Faiz Abbas <faiz_abbas@ti.com>

Serdes lanes might be shared between multiple cores in some usecases
and its not possible to lock PLLs for both the lanes independently
by the two cores. This requires a bootloader to configure both the
lanes at early boot time.

To handle this case, skip all configuration if any of the plls are
already locked. This is done by adding an already_configured flag
and using it to gate every register access as well as any phy_ops.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 127 ++++++++++++++---------
 1 file changed, 78 insertions(+), 49 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index e08548417bce..145e42837b7b 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -364,6 +364,10 @@ static const struct phy_ops ops = {
 	.owner		= THIS_MODULE,
 };
 
+static const struct phy_ops noop_ops = {
+	.owner		= THIS_MODULE,
+};
+
 static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
 				    struct device_node *child)
 {
@@ -477,6 +481,49 @@ static int cdns_regmap_init_blocks(struct cdns_sierra_phy *sp,
 	return 0;
 }
 
+static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
+				      struct device *dev)
+{
+	struct clk *clk;
+	int ret;
+
+	sp->clk = devm_clk_get_optional(dev, "phy_clk");
+	if (IS_ERR(sp->clk)) {
+		dev_err(dev, "failed to get clock phy_clk\n");
+		return PTR_ERR(sp->clk);
+	}
+
+	sp->phy_rst = devm_reset_control_get(dev, "sierra_reset");
+	if (IS_ERR(sp->phy_rst)) {
+		dev_err(dev, "failed to get reset\n");
+		return PTR_ERR(sp->phy_rst);
+	}
+
+	sp->apb_rst = devm_reset_control_get_optional(dev, "sierra_apb");
+	if (IS_ERR(sp->apb_rst)) {
+		dev_err(dev, "failed to get apb reset\n");
+		return PTR_ERR(sp->apb_rst);
+	}
+
+	clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "cmn_refclk_dig_div clock not found\n");
+		ret = PTR_ERR(clk);
+		return ret;
+	}
+	sp->cmn_refclk_dig_div = clk;
+
+	clk = devm_clk_get_optional(dev, "cmn_refclk1_dig_div");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "cmn_refclk1_dig_div clock not found\n");
+		ret = PTR_ERR(clk);
+		return ret;
+	}
+	sp->cmn_refclk1_dig_div = clk;
+
+	return 0;
+}
+
 static int cdns_sierra_phy_probe(struct platform_device *pdev)
 {
 	struct cdns_sierra_phy *sp;
@@ -486,10 +533,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	struct cdns_sierra_data *data;
 	unsigned int id_value;
 	struct resource *res;
-	int i, ret, node = 0;
+	int i, val, ret, node = 0;
 	void __iomem *base;
-	struct clk *clk;
 	struct device_node *dn = dev->of_node, *child;
+	bool already_configured = false;
 
 	if (of_get_child_count(dn) == 0)
 		return -ENODEV;
@@ -524,54 +571,33 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	platform_set_drvdata(pdev, sp);
-
-	sp->clk = devm_clk_get_optional(dev, "phy_clk");
-	if (IS_ERR(sp->clk)) {
-		dev_err(dev, "failed to get clock phy_clk\n");
-		return PTR_ERR(sp->clk);
-	}
-
-	sp->phy_rst = devm_reset_control_get(dev, "sierra_reset");
-	if (IS_ERR(sp->phy_rst)) {
-		dev_err(dev, "failed to get reset\n");
-		return PTR_ERR(sp->phy_rst);
-	}
-
-	sp->apb_rst = devm_reset_control_get_optional(dev, "sierra_apb");
-	if (IS_ERR(sp->apb_rst)) {
-		dev_err(dev, "failed to get apb reset\n");
-		return PTR_ERR(sp->apb_rst);
-	}
-
-	clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "cmn_refclk_dig_div clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
-	}
-	sp->cmn_refclk_dig_div = clk;
-
-	clk = devm_clk_get_optional(dev, "cmn_refclk1_dig_div");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "cmn_refclk1_dig_div clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
+	for (i = 0; i < SIERRA_MAX_LANES; i++) {
+		regmap_field_read(sp->pllctrl_lock[i], &val);
+		if (val) {
+			already_configured = true;
+			break;
+		}
 	}
-	sp->cmn_refclk1_dig_div = clk;
 
-	ret = clk_prepare_enable(sp->clk);
-	if (ret)
-		return ret;
-
-	/* Enable APB */
-	reset_control_deassert(sp->apb_rst);
+	platform_set_drvdata(pdev, sp);
 
-	/* Check that PHY is present */
-	regmap_field_read(sp->macro_id_type, &id_value);
-	if  (sp->init_data->id_value != id_value) {
-		ret = -EINVAL;
-		goto clk_disable;
+	if (!already_configured) {
+		ret = cdns_sierra_phy_get_clocks(sp, dev);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(sp->clk);
+		if (ret)
+			return ret;
+		/* Enable APB */
+		reset_control_deassert(sp->apb_rst);
+
+		/* Check that PHY is present */
+		regmap_field_read(sp->macro_id_type, &id_value);
+		if  (sp->init_data->id_value != id_value) {
+			ret = -EINVAL;
+			goto clk_disable;
+		}
 	}
 
 	sp->autoconf = of_property_read_bool(dn, "cdns,autoconf");
@@ -603,7 +629,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 
 		sp->num_lanes += sp->phys[node].num_lanes;
 
-		gphy = devm_phy_create(dev, child, &ops);
+		if (already_configured)
+			gphy = devm_phy_create(dev, child, &noop_ops);
+		else
+			gphy = devm_phy_create(dev, child, &ops);
 
 		if (IS_ERR(gphy)) {
 			ret = PTR_ERR(gphy);
@@ -622,7 +651,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	}
 
 	/* If more than one subnode, configure the PHY as multilink */
-	if (!sp->autoconf && sp->nsubnodes > 1)
+	if (!sp->autoconf && sp->nsubnodes > 1 && !already_configured)
 		regmap_field_write(sp->phy_pll_cfg_1, 0x1);
 
 	pm_runtime_enable(dev);
-- 
2.17.1

[PATCH 7/9] phy: cadence: sierra: Model reference receiver as clocks (gate clocks)

[Kishon Vijay Abraham I]

Sierra has two reference recievers REFRCV and REFRCV1. REFRCV is used to
drive reference clock cmn_refclk_m/p to PLL_CMNLC1 and REFRCV1 is used to
drive reference clock cmn_refclk1_m/p to PLL_CMNLC. Model these
reference receivers as clocks in order for PLL_CMNLC and PLL_CMNLC1 to
be able to seamlessly use any of the external reference clocks.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 177 +++++++++++++++++++++++
 1 file changed, 177 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index 145e42837b7b..ab7a3e2795cd 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -7,6 +7,7 @@
  *
  */
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -31,6 +32,8 @@
 #define SIERRA_CMN_PLLLC_BWCAL_MODE1_PREG		0x4F
 #define SIERRA_CMN_PLLLC_BWCAL_MODE0_PREG		0x50
 #define SIERRA_CMN_PLLLC_SS_TIME_STEPSIZE_MODE_PREG	0x62
+#define SIERRA_CMN_REFRCV_PREG				0x98
+#define SIERRA_CMN_REFRCV1_PREG				0xB8
 
 #define SIERRA_LANE_CDB_OFFSET(ln, block_offset, reg_offset)	\
 				((0x4000 << (block_offset)) + \
@@ -151,6 +154,35 @@ static const struct reg_field phy_pll_cfg_1 =
 static const struct reg_field pllctrl_lock =
 				REG_FIELD(SIERRA_PLLCTRL_STATUS_PREG, 0, 0);
 
+enum cdns_sierra_cmn_refrcv {
+	CMN_REFRCV,
+	CMN_REFRCV1,
+};
+
+#define SIERRA_NUM_REFRCV	0x2
+
+static const struct reg_field cmn_refrcv_refclk_plllc1en_preg[] = {
+	[CMN_REFRCV]	= REG_FIELD(SIERRA_CMN_REFRCV_PREG, 8, 8),
+	[CMN_REFRCV1]	= REG_FIELD(SIERRA_CMN_REFRCV1_PREG, 8, 8),
+};
+
+static const struct reg_field cmn_refrcv_refclk_termen_preg[] = {
+	[CMN_REFRCV]	= REG_FIELD(SIERRA_CMN_REFRCV_PREG, 0, 0),
+	[CMN_REFRCV1]	= REG_FIELD(SIERRA_CMN_REFRCV1_PREG, 0, 0),
+};
+
+static char *refrcv_node_name[] = { "refrcv", "refrcv1" };
+
+struct cdns_sierra_refrcv {
+	struct clk_hw		hw;
+	struct regmap_field	*plllc1en_field;
+	struct regmap_field	*termen_field;
+	struct clk_init_data	clk_data;
+};
+
+#define to_cdns_sierra_refrcv(_hw)	\
+			container_of(_hw, struct cdns_sierra_refrcv, hw)
+
 struct cdns_sierra_inst {
 	struct phy *phy;
 	u32 phy_type;
@@ -197,6 +229,8 @@ struct cdns_sierra_phy {
 	struct regmap_field *macro_id_type;
 	struct regmap_field *phy_pll_cfg_1;
 	struct regmap_field *pllctrl_lock[SIERRA_MAX_LANES];
+	struct regmap_field *cmn_refrcv_refclk_plllc1en_preg[SIERRA_NUM_REFRCV];
+	struct regmap_field *cmn_refrcv_refclk_termen_preg[SIERRA_NUM_REFRCV];
 	struct clk *clk;
 	struct clk *cmn_refclk_dig_div;
 	struct clk *cmn_refclk1_dig_div;
@@ -368,6 +402,93 @@ static const struct phy_ops noop_ops = {
 	.owner		= THIS_MODULE,
 };
 
+static int cdns_sierra_refrcv_enable(struct clk_hw *hw)
+{
+	struct cdns_sierra_refrcv *refrcv = to_cdns_sierra_refrcv(hw);
+	struct regmap_field *plllc1en_field = refrcv->plllc1en_field;
+	struct regmap_field *termen_field = refrcv->termen_field;
+
+	regmap_field_write(plllc1en_field, 1);
+	regmap_field_write(termen_field, 1);
+
+	return 0;
+}
+
+static void cdns_sierra_refrcv_disable(struct clk_hw *hw)
+{
+	struct cdns_sierra_refrcv *refrcv = to_cdns_sierra_refrcv(hw);
+	struct regmap_field *plllc1en_field = refrcv->plllc1en_field;
+	struct regmap_field *termen_field = refrcv->termen_field;
+
+	regmap_field_write(plllc1en_field, 0);
+	regmap_field_write(termen_field, 0);
+}
+
+static int cdns_sierra_refrcv_is_enabled(struct clk_hw *hw)
+{
+	struct cdns_sierra_refrcv *refrcv = to_cdns_sierra_refrcv(hw);
+	struct regmap_field *plllc1en_field = refrcv->plllc1en_field;
+	int val;
+
+	regmap_field_read(plllc1en_field, &val);
+
+	return !!val;
+}
+
+static const struct clk_ops cdns_sierra_refrcv_ops = {
+	.enable = cdns_sierra_refrcv_enable,
+	.disable = cdns_sierra_refrcv_disable,
+	.is_enabled = cdns_sierra_refrcv_is_enabled,
+};
+
+static int cdns_sierra_refrcv_register(struct cdns_sierra_phy *sp,
+				       struct device_node *node,
+				       struct regmap_field *plllc1en_field,
+				       struct regmap_field *termen_field)
+{
+	struct cdns_sierra_refrcv *refrcv;
+	struct device *dev = sp->dev;
+	struct clk_init_data *init;
+	unsigned int num_parents;
+	const char *parent_name;
+	char clk_name[100];
+	struct clk *clk;
+	int ret;
+
+	refrcv = devm_kzalloc(dev, sizeof(*refrcv), GFP_KERNEL);
+	if (!refrcv)
+		return -ENOMEM;
+
+	num_parents = of_clk_get_parent_count(node);
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	snprintf(clk_name, sizeof(clk_name), "%s_%s", dev_name(dev),
+		 node->name);
+
+	init = &refrcv->clk_data;
+
+	init->ops = &cdns_sierra_refrcv_ops;
+	init->flags = 0;
+	init->parent_names = parent_name ? &parent_name : NULL;
+	init->num_parents = num_parents ? 1 : 0;
+	init->name = clk_name;
+
+	refrcv->plllc1en_field = plllc1en_field;
+	refrcv->termen_field = termen_field;
+	refrcv->hw.init = init;
+
+	clk = devm_clk_register(dev, &refrcv->hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (ret)
+		dev_err(dev, "Failed to add refrcv clock provider: %s\n",
+			clk_name);
+
+	return ret;
+}
+
 static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
 				    struct device_node *child)
 {
@@ -406,6 +527,7 @@ static int cdns_regfield_init(struct cdns_sierra_phy *sp)
 {
 	struct device *dev = sp->dev;
 	struct regmap_field *field;
+	struct reg_field reg_field;
 	struct regmap *regmap;
 	int i;
 
@@ -417,6 +539,24 @@ static int cdns_regfield_init(struct cdns_sierra_phy *sp)
 	}
 	sp->macro_id_type = field;
 
+	for (i = 0; i < SIERRA_NUM_REFRCV; i++) {
+		reg_field = cmn_refrcv_refclk_plllc1en_preg[i];
+		field = devm_regmap_field_alloc(dev, regmap, reg_field);
+		if (IS_ERR(field)) {
+			dev_err(dev, "REFRCV%d_REFCLK_PLLLC1EN failed\n", i);
+			return PTR_ERR(field);
+		}
+		sp->cmn_refrcv_refclk_plllc1en_preg[i] = field;
+
+		reg_field = cmn_refrcv_refclk_termen_preg[i];
+		field = devm_regmap_field_alloc(dev, regmap, reg_field);
+		if (IS_ERR(field)) {
+			dev_err(dev, "REFRCV%d_REFCLK_TERMEN failed\n", i);
+			return PTR_ERR(field);
+		}
+		sp->cmn_refrcv_refclk_termen_preg[i] = field;
+	}
+
 	regmap = sp->regmap_phy_config_ctrl;
 	field = devm_regmap_field_alloc(dev, regmap, phy_pll_cfg_1);
 	if (IS_ERR(field)) {
@@ -481,6 +621,38 @@ static int cdns_regmap_init_blocks(struct cdns_sierra_phy *sp,
 	return 0;
 }
 
+static int cdns_sierra_phy_register_refrcv(struct cdns_sierra_phy *sp,
+					   struct device_node *dn)
+{
+	struct regmap_field *plllc1en_field;
+	struct device_node *of_node = NULL;
+	struct regmap_field *termen_field;
+	struct device *dev = sp->dev;
+	int ret = 0, i;
+
+	for (i = 0; i < SIERRA_NUM_REFRCV; i++) {
+		of_node = of_get_child_by_name(dn, refrcv_node_name[i]);
+		if (!of_node)
+			return 0;
+
+		plllc1en_field = sp->cmn_refrcv_refclk_plllc1en_preg[i];
+		termen_field = sp->cmn_refrcv_refclk_termen_preg[i];
+
+		ret = cdns_sierra_refrcv_register(sp, of_node, plllc1en_field,
+						  termen_field);
+		if (ret) {
+			dev_err(dev, "Fail to register reference receiver %s\n",
+				refrcv_node_name[i]);
+			goto err;
+		}
+	}
+
+err:
+	of_node_put(of_node);
+
+	return ret;
+}
+
 static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
 				      struct device *dev)
 {
@@ -582,6 +754,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sp);
 
 	if (!already_configured) {
+		ret = cdns_sierra_phy_register_refrcv(sp, dn);
+		if (ret)
+			return ret;
+
 		ret = cdns_sierra_phy_get_clocks(sp, dev);
 		if (ret)
 			return ret;
@@ -589,6 +765,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 		ret = clk_prepare_enable(sp->clk);
 		if (ret)
 			return ret;
+
 		/* Enable APB */
 		reset_control_deassert(sp->apb_rst);
 
-- 
2.17.1

[PATCH 8/9] phy: cadence: sierra: Model PLL_CMNLC and PLL_CMNLC1 as clocks (mux clocks)

[Kishon Vijay Abraham I]

Sierra has two PLLs, PLL_CMNLC and PLL_CMNLC1 and each of these PLLs has
two inputs, plllc_refclk and refrcv. Model PLL_CMNLC and PLL_CMNLC1 as
clocks so that it's possible to select one of these two inputs from
device tree.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 158 +++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index ab7a3e2795cd..c4751fe9edfd 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -25,6 +25,7 @@
 /* PHY register offsets */
 #define SIERRA_COMMON_CDB_OFFSET			0x0
 #define SIERRA_MACRO_ID_REG				0x0
+#define SIERRA_CMN_PLLLC_GEN_PREG			0x42
 #define SIERRA_CMN_PLLLC_MODE_PREG			0x48
 #define SIERRA_CMN_PLLLC_LF_COEFF_MODE1_PREG		0x49
 #define SIERRA_CMN_PLLLC_LF_COEFF_MODE0_PREG		0x4A
@@ -34,6 +35,7 @@
 #define SIERRA_CMN_PLLLC_SS_TIME_STEPSIZE_MODE_PREG	0x62
 #define SIERRA_CMN_REFRCV_PREG				0x98
 #define SIERRA_CMN_REFRCV1_PREG				0xB8
+#define SIERRA_CMN_PLLLC1_GEN_PREG			0xC2
 
 #define SIERRA_LANE_CDB_OFFSET(ln, block_offset, reg_offset)	\
 				((0x4000 << (block_offset)) + \
@@ -183,6 +185,36 @@ struct cdns_sierra_refrcv {
 #define to_cdns_sierra_refrcv(_hw)	\
 			container_of(_hw, struct cdns_sierra_refrcv, hw)
 
+enum cdns_sierra_cmn_plllc {
+	CMN_PLLLC,
+	CMN_PLLLC1,
+};
+
+#define SIERRA_NUM_CMN_PLLC	0x2
+
+static const struct reg_field cmn_plllc_pfdclk1_sel_preg[] = {
+	[CMN_PLLLC]	= REG_FIELD(SIERRA_CMN_PLLLC_GEN_PREG, 1, 1),
+	[CMN_PLLLC1]	= REG_FIELD(SIERRA_CMN_PLLLC1_GEN_PREG, 1, 1),
+};
+
+static char *cmn_plllc_node_name[] = { "pll_cmnlc", "pll_cmnlc1" };
+
+struct cdns_sierra_pll_mux {
+	struct clk_hw		hw;
+	struct regmap_field	*pfdclk_sel_preg;
+	u32			*table;
+	struct clk_init_data	clk_data;
+};
+
+#define to_cdns_sierra_pll_mux(_hw)	\
+			container_of(_hw, struct cdns_sierra_pll_mux, hw)
+
+/*
+ * Mux value to be configured for each of the input clocks
+ * in the order populated in device tree
+ */
+static u32 cdns_sierra_pll_mux_table[] = { 0, 1 };
+
 struct cdns_sierra_inst {
 	struct phy *phy;
 	u32 phy_type;
@@ -231,6 +263,7 @@ struct cdns_sierra_phy {
 	struct regmap_field *pllctrl_lock[SIERRA_MAX_LANES];
 	struct regmap_field *cmn_refrcv_refclk_plllc1en_preg[SIERRA_NUM_REFRCV];
 	struct regmap_field *cmn_refrcv_refclk_termen_preg[SIERRA_NUM_REFRCV];
+	struct regmap_field *cmn_plllc_pfdclk1_sel_preg[SIERRA_NUM_CMN_PLLC];
 	struct clk *clk;
 	struct clk *cmn_refclk_dig_div;
 	struct clk *cmn_refclk1_dig_div;
@@ -402,6 +435,117 @@ static const struct phy_ops noop_ops = {
 	.owner		= THIS_MODULE,
 };
 
+static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw)
+{
+	struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw);
+	struct regmap_field *field = mux->pfdclk_sel_preg;
+	unsigned int val;
+
+	regmap_field_read(field, &val);
+	return clk_mux_val_to_index(hw, mux->table, 0, val);
+}
+
+static int cdns_sierra_pll_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw);
+	struct regmap_field *field = mux->pfdclk_sel_preg;
+	int val;
+
+	val = mux->table[index];
+	return regmap_field_write(field, val);
+}
+
+static const struct clk_ops cdns_sierra_pll_mux_ops = {
+	.set_parent = cdns_sierra_pll_mux_set_parent,
+	.get_parent = cdns_sierra_pll_mux_get_parent,
+};
+
+static int cdns_sierra_pll_mux_register(struct cdns_sierra_phy *sp,
+					struct device_node *node,
+					struct regmap_field *field)
+{
+	struct cdns_sierra_pll_mux *mux;
+	struct device *dev = sp->dev;
+	struct clk_init_data *init;
+	const char **parent_names;
+	unsigned int num_parents;
+	char clk_name[100];
+	struct clk *clk;
+	int ret;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents < 2) {
+		dev_err(dev, "SERDES clock must have parents\n");
+		return -EINVAL;
+	}
+
+	parent_names = devm_kzalloc(dev, (sizeof(char *) * num_parents),
+				    GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
+
+	of_clk_parent_fill(node, parent_names, num_parents);
+
+	snprintf(clk_name, sizeof(clk_name), "%s_%s", dev_name(dev),
+		 node->name);
+
+	init = &mux->clk_data;
+
+	init->ops = &cdns_sierra_pll_mux_ops;
+	init->flags = CLK_SET_RATE_NO_REPARENT;
+	init->parent_names = parent_names;
+	init->num_parents = num_parents;
+	init->name = clk_name;
+
+	mux->pfdclk_sel_preg = field;
+	mux->table = cdns_sierra_pll_mux_table;
+	mux->hw.init = init;
+
+	clk = devm_clk_register(dev, &mux->hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (ret)
+		dev_err(dev, "Fail to add pll mux clock provider: %s\n",
+			clk_name);
+
+	return ret;
+}
+
+static int cdns_sierra_phy_register_pll_mux(struct cdns_sierra_phy *sp,
+					    struct device_node *dn)
+{
+	struct regmap_field *pfdclk1_sel_field;
+	struct device_node *of_node = NULL;
+	struct device *dev = sp->dev;
+	int ret = 0, i;
+
+	for (i = 0; i < SIERRA_NUM_CMN_PLLC; i++) {
+		of_node = of_get_child_by_name(dn, cmn_plllc_node_name[i]);
+		if (!of_node)
+			return 0;
+
+		pfdclk1_sel_field = sp->cmn_plllc_pfdclk1_sel_preg[i];
+		ret = cdns_sierra_pll_mux_register(sp, of_node,
+						   pfdclk1_sel_field);
+		if (ret) {
+			dev_err(dev, "Fail to register cmn plllc mux %s\n",
+				cmn_plllc_node_name[i]);
+			goto err;
+		}
+	}
+
+err:
+	of_node_put(of_node);
+
+	return 0;
+}
+
 static int cdns_sierra_refrcv_enable(struct clk_hw *hw)
 {
 	struct cdns_sierra_refrcv *refrcv = to_cdns_sierra_refrcv(hw);
@@ -557,6 +701,16 @@ static int cdns_regfield_init(struct cdns_sierra_phy *sp)
 		sp->cmn_refrcv_refclk_termen_preg[i] = field;
 	}
 
+	for (i = 0; i < SIERRA_NUM_CMN_PLLC; i++) {
+		reg_field = cmn_plllc_pfdclk1_sel_preg[i];
+		field = devm_regmap_field_alloc(dev, regmap, reg_field);
+		if (IS_ERR(field)) {
+			dev_err(dev, "PLLLC%d_PFDCLK1_SEL failed\n", i);
+			return PTR_ERR(field);
+		}
+		sp->cmn_plllc_pfdclk1_sel_preg[i] = field;
+	}
+
 	regmap = sp->regmap_phy_config_ctrl;
 	field = devm_regmap_field_alloc(dev, regmap, phy_pll_cfg_1);
 	if (IS_ERR(field)) {
@@ -758,6 +912,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 		if (ret)
 			return ret;
 
+		ret = cdns_sierra_phy_register_pll_mux(sp, dn);
+		if (ret)
+			return ret;
+
 		ret = cdns_sierra_phy_get_clocks(sp, dev);
 		if (ret)
 			return ret;
-- 
2.17.1

[PATCH 9/9] phy: cadence: sierra: Enable pll_cmnlc and pll_cmnlc1 clocks

[Kishon Vijay Abraham I]

Get pll_cmnlc and pll_cmnlc1 optional clocks and enable them.
This will enable REFRCV/1 in case the pll_cmnlc/1 takes input
from REFRCV/1 respectively.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 39 +++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index c4751fe9edfd..94fd9ce4223e 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -267,6 +267,8 @@ struct cdns_sierra_phy {
 	struct clk *clk;
 	struct clk *cmn_refclk_dig_div;
 	struct clk *cmn_refclk1_dig_div;
+	struct clk *pll_cmnlc;
+	struct clk *pll_cmnlc1;
 	int nsubnodes;
 	u32 num_lanes;
 	bool autoconf;
@@ -847,6 +849,41 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
 	}
 	sp->cmn_refclk1_dig_div = clk;
 
+	clk = devm_clk_get_optional(dev, "pll_cmnlc");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "pll_cmnlc clock not found\n");
+		ret = PTR_ERR(clk);
+		return ret;
+	}
+	sp->pll_cmnlc = clk;
+
+	clk = devm_clk_get_optional(dev, "pll_cmnlc1");
+	if (IS_ERR(clk)) {
+		dev_err(dev, "pll_cmnlc1 clock not found\n");
+		ret = PTR_ERR(clk);
+		return ret;
+	}
+	sp->pll_cmnlc1 = clk;
+
+	return 0;
+}
+
+static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(sp->clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(sp->pll_cmnlc);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(sp->pll_cmnlc1);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -920,7 +957,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 		if (ret)
 			return ret;
 
-		ret = clk_prepare_enable(sp->clk);
+		ret = cdns_sierra_phy_enable_clocks(sp);
 		if (ret)
 			return ret;
 
-- 
2.17.1

Re: [PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for the PLLs within SERDES

[Rob Herring]

On Tue, Nov 03, 2020 at 09:25:48AM +0530, Kishon Vijay Abraham I wrote:
> Add binding for the PLLs within SERDES.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/phy/phy-cadence-sierra.yaml      | 89 ++++++++++++++++++-
>  1 file changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
> index d210843863df..f574b8ed358c 100644
> --- a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
> @@ -49,12 +49,14 @@ properties:
>      const: serdes
>  
>    clocks:
> -    maxItems: 2
> +    maxItems: 4
>  
>    clock-names:
>      items:
>        - const: cmn_refclk_dig_div
>        - const: cmn_refclk1_dig_div
> +      - const: pll_cmnlc
> +      - const: pll_cmnlc1
>  
>    cdns,autoconf:
>      type: boolean
> @@ -107,6 +109,58 @@ patternProperties:
>  
>      additionalProperties: false
>  
> +  "^refrcv1?$":
> +    type: object
> +    description: |
> +      Reference receivers that enables routing external clocks to the alternate
> +      PLLCMNLC.
> +    properties:
> +      clocks:
> +        maxItems: 1
> +        description: Phandle to clock nodes representing the input to the
> +          reference receiver.
> +
> +      clock-names:
> +        items:
> +          - const: pll_refclk
> +
> +      "#clock-cells":
> +        const: 0
> +
> +    required:
> +      - clocks
> +      - "#clock-cells"
> +
> +  "^pll_cmnlc1?$":
> +    type: object
> +    description: |
> +      SERDES node should have subnodes for each of the PLLs present in
> +      the SERDES.
> +    properties:
> +      clocks:
> +        maxItems: 2
> +        description: Phandle to clock nodes representing the two inputs to PLL.
> +
> +      clock-names:
> +        items:
> +          - const: pll_refclk
> +          - const: refrcv
> +
> +      "#clock-cells":
> +        const: 0
> +
> +      assigned-clocks:
> +        maxItems: 1
> +
> +      assigned-clock-parents:
> +        maxItems: 1
> +
> +    required:
> +      - clocks
> +      - "#clock-cells"
> +      - assigned-clocks
> +      - assigned-clock-parents
> +
>  required:
>    - compatible
>    - "#address-cells"
> @@ -130,10 +184,39 @@ examples:
>              reg = <0x0 0xfd240000 0x0 0x40000>;
>              resets = <&phyrst 0>, <&phyrst 1>;
>              reset-names = "sierra_reset", "sierra_apb";
> -            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>;
> -            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div";
> +            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>, <&serdes_pll_cmnlc>, <&serdes_pll_cmnlc1>;
> +            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div", "pll_cmnlc", "pll_cmnlc1";
>              #address-cells = <1>;
>              #size-cells = <0>;
> +
> +            serdes_refrcv: refrcv {
> +                    clocks = <&pll0_refclk>;
> +                    clock-names = "pll_refclk";
> +                    #clock-cells = <0>;
> +            };
> +
> +            serdes_refrcv1: refrcv1 {
> +                    clocks = <&pll1_refclk>;
> +                    clock-names = "pll_refclk";
> +                    #clock-cells = <0>;
> +            };
> +
> +            serdes_pll_cmnlc: pll_cmnlc {
> +                    clocks = <&pll0_refclk>, <&serdes_refrcv1>;
> +                    clock-names = "pll_refclk", "refrcv";
> +                    #clock-cells = <0>;
> +                    assigned-clocks = <&serdes_pll_cmnlc>;

Isn't assigned-clocks supposed to be one of the clocks in 'clocks'?

> +                    assigned-clock-parents = <&pll0_refclk>;

And this should not be a clock in 'clocks'...


More generally, why do we need to expose all these details in DT?

Rob

Re: [PATCH 6/9] phy: cadence: sierra: Don't configure if any plls are already locked

[Philipp Zabel]

On Tue, 2020-11-03 at 09:25 +0530, Kishon Vijay Abraham I wrote:
> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Serdes lanes might be shared between multiple cores in some usecases
> and its not possible to lock PLLs for both the lanes independently
> by the two cores. This requires a bootloader to configure both the
> lanes at early boot time.
> 
> To handle this case, skip all configuration if any of the plls are
> already locked. This is done by adding an already_configured flag
> and using it to gate every register access as well as any phy_ops.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/cadence/phy-cadence-sierra.c | 127 ++++++++++++++---------
>  1 file changed, 78 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
> index e08548417bce..145e42837b7b 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -364,6 +364,10 @@ static const struct phy_ops ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static const struct phy_ops noop_ops = {
> +	.owner		= THIS_MODULE,
> +};
> +
>  static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
>  				    struct device_node *child)
>  {
> @@ -477,6 +481,49 @@ static int cdns_regmap_init_blocks(struct cdns_sierra_phy *sp,
>  	return 0;
>  }
>  
> +static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
> +				      struct device *dev)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	sp->clk = devm_clk_get_optional(dev, "phy_clk");
> +	if (IS_ERR(sp->clk)) {
> +		dev_err(dev, "failed to get clock phy_clk\n");
> +		return PTR_ERR(sp->clk);
> +	}
> +
> +	sp->phy_rst = devm_reset_control_get(dev, "sierra_reset");

While you're at it, please use devm_reset_control_get_exclusive() here
and ...

> +	if (IS_ERR(sp->phy_rst)) {
> +		dev_err(dev, "failed to get reset\n");
> +		return PTR_ERR(sp->phy_rst);
> +	}
> +
> +	sp->apb_rst = devm_reset_control_get_optional(dev, "sierra_apb");

... devm_reset_control_get_optional_exclusive() here.

regards
Philipp

Re: [PATCH 3/9] phy: ti: j721e-wiz: Don't configure wiz if its already configured

[Vinod Koul]

On 03-11-20, 09:25, Kishon Vijay Abraham I wrote:
> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Serdes lanes might be shared between multiple cores in some usecases
> and its not possible to lock PLLs for both the lanes independently
> by the two cores. This requires a bootloader to configure both the
> lanes at early boot time.
> 
> To handle this case, skip all configuration if any of the lanes has
> already been enabled.
> 
> While we are here, also fix the wiz_init() to be called before the
> of_platform_device_create() call.

Let's do two patches for these two issues :-)

Other than that, change lgtm, with exception of minor nit

> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/ti/phy-j721e-wiz.c | 36 +++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index d57d29382ce4..9786e8aec252 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -816,13 +816,14 @@ static int wiz_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = dev->of_node;
>  	struct platform_device *serdes_pdev;
> +	bool already_configured = false;
>  	struct device_node *child_node;
>  	struct regmap *regmap;
>  	struct resource res;
>  	void __iomem *base;
>  	struct wiz *wiz;
>  	u32 num_lanes;
> -	int ret;
> +	int ret, val, i;
>  
>  	wiz = devm_kzalloc(dev, sizeof(*wiz), GFP_KERNEL);
>  	if (!wiz)
> @@ -944,10 +945,26 @@ static int wiz_probe(struct platform_device *pdev)
>  		goto err_get_sync;
>  	}
>  
> -	ret = wiz_clock_init(wiz, node);
> -	if (ret < 0) {
> -		dev_warn(dev, "Failed to initialize clocks\n");
> -		goto err_get_sync;
> +	for (i = 0; i < wiz->num_lanes; i++) {
> +		regmap_field_read(wiz->p_enable[i], &val);
> +		if (val & (P_ENABLE | P_ENABLE_FORCE)) {
> +			already_configured = true;
> +			break;
> +		}
> +	}
> +
> +	if (!already_configured) {

do you really need this variable and check, why not move the below into
precceding block and do wiz_clock_init() and wiz_init() inside the
if condition and drop the variable

> +		ret = wiz_clock_init(wiz, node);
> +		if (ret < 0) {
> +			dev_warn(dev, "Failed to initialize clocks\n");
> +			goto err_get_sync;
> +		}
> +
> +		ret = wiz_init(wiz);
> +		if (ret) {
> +			dev_err(dev, "WIZ initialization failed\n");
> +			goto err_pdev_create;
> +		}
>  	}
>  
>  	serdes_pdev = of_platform_device_create(child_node, NULL, dev);
> @@ -958,18 +975,9 @@ static int wiz_probe(struct platform_device *pdev)
>  	}
>  	wiz->serdes_pdev = serdes_pdev;
>  
> -	ret = wiz_init(wiz);
> -	if (ret) {
> -		dev_err(dev, "WIZ initialization failed\n");
> -		goto err_wiz_init;
> -	}
> -
>  	of_node_put(child_node);
>  	return 0;
>  
> -err_wiz_init:
> -	of_platform_device_destroy(&serdes_pdev->dev, NULL);
> -
>  err_pdev_create:
>  	wiz_clock_cleanup(wiz, node);
>  
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 5/9] phy: cadence: Sierra: Fix PHY power_on sequence

[Vinod Koul]

On 03-11-20, 09:25, Kishon Vijay Abraham I wrote:
> Commit 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
> de-asserts PHY_RESET even before the configurations are loaded in
> phy_init(). However PHY_RESET should be de-asserted only after
> all the configurations has been initialized, instead of de-asserting
> in probe. Fix it here.

Move this up in series..? Also I think we should apply this to fixes and
perhaps cc stable..?

> 
> Fixes: 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/cadence/phy-cadence-sierra.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
> index 4429f41a8f58..e08548417bce 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -319,6 +319,12 @@ static int cdns_sierra_phy_on(struct phy *gphy)
>  	u32 val;
>  	int ret;
>  
> +	ret = reset_control_deassert(sp->phy_rst);
> +	if (ret) {
> +		dev_err(dev, "Failed to take the PHY out of reset\n");
> +		return ret;
> +	}
> +
>  	/* Take the PHY lane group out of reset */
>  	ret = reset_control_deassert(ins->lnk_rst);
>  	if (ret) {
> @@ -621,7 +627,6 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(dev);
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> -	reset_control_deassert(sp->phy_rst);
>  	return PTR_ERR_OR_ZERO(phy_provider);
>  
>  put_child:
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/9] dt-bindings: phy: cadence-sierra: Add bindings for the PLLs within SERDES

[Kishon Vijay Abraham I]

Hi Rob,

On 05/11/20 11:33 pm, Rob Herring wrote:
> On Tue, Nov 03, 2020 at 09:25:48AM +0530, Kishon Vijay Abraham I wrote:
>> Add binding for the PLLs within SERDES.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/phy/phy-cadence-sierra.yaml      | 89 ++++++++++++++++++-
>>  1 file changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> index d210843863df..f574b8ed358c 100644
>> --- a/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-sierra.yaml
>> @@ -49,12 +49,14 @@ properties:
>>      const: serdes
>>  
>>    clocks:
>> -    maxItems: 2
>> +    maxItems: 4
>>  
>>    clock-names:
>>      items:
>>        - const: cmn_refclk_dig_div
>>        - const: cmn_refclk1_dig_div
>> +      - const: pll_cmnlc
>> +      - const: pll_cmnlc1
>>  
>>    cdns,autoconf:
>>      type: boolean
>> @@ -107,6 +109,58 @@ patternProperties:
>>  
>>      additionalProperties: false
>>  
>> +  "^refrcv1?$":
>> +    type: object
>> +    description: |
>> +      Reference receivers that enables routing external clocks to the alternate
>> +      PLLCMNLC.
>> +    properties:
>> +      clocks:
>> +        maxItems: 1
>> +        description: Phandle to clock nodes representing the input to the
>> +          reference receiver.
>> +
>> +      clock-names:
>> +        items:
>> +          - const: pll_refclk
>> +
>> +      "#clock-cells":
>> +        const: 0
>> +
>> +    required:
>> +      - clocks
>> +      - "#clock-cells"
>> +
>> +  "^pll_cmnlc1?$":
>> +    type: object
>> +    description: |
>> +      SERDES node should have subnodes for each of the PLLs present in
>> +      the SERDES.
>> +    properties:
>> +      clocks:
>> +        maxItems: 2
>> +        description: Phandle to clock nodes representing the two inputs to PLL.
>> +
>> +      clock-names:
>> +        items:
>> +          - const: pll_refclk
>> +          - const: refrcv
>> +
>> +      "#clock-cells":
>> +        const: 0
>> +
>> +      assigned-clocks:
>> +        maxItems: 1
>> +
>> +      assigned-clock-parents:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - clocks
>> +      - "#clock-cells"
>> +      - assigned-clocks
>> +      - assigned-clock-parents
>> +
>>  required:
>>    - compatible
>>    - "#address-cells"
>> @@ -130,10 +184,39 @@ examples:
>>              reg = <0x0 0xfd240000 0x0 0x40000>;
>>              resets = <&phyrst 0>, <&phyrst 1>;
>>              reset-names = "sierra_reset", "sierra_apb";
>> -            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>;
>> -            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div";
>> +            clocks = <&cmn_refclk_dig_div>, <&cmn_refclk1_dig_div>, <&serdes_pll_cmnlc>, <&serdes_pll_cmnlc1>;
>> +            clock-names = "cmn_refclk_dig_div", "cmn_refclk1_dig_div", "pll_cmnlc", "pll_cmnlc1";
>>              #address-cells = <1>;
>>              #size-cells = <0>;
>> +
>> +            serdes_refrcv: refrcv {
>> +                    clocks = <&pll0_refclk>;
>> +                    clock-names = "pll_refclk";
>> +                    #clock-cells = <0>;
>> +            };
>> +
>> +            serdes_refrcv1: refrcv1 {
>> +                    clocks = <&pll1_refclk>;
>> +                    clock-names = "pll_refclk";
>> +                    #clock-cells = <0>;
>> +            };
>> +
>> +            serdes_pll_cmnlc: pll_cmnlc {
>> +                    clocks = <&pll0_refclk>, <&serdes_refrcv1>;
>> +                    clock-names = "pll_refclk", "refrcv";
>> +                    #clock-cells = <0>;
>> +                    assigned-clocks = <&serdes_pll_cmnlc>;
> 
> Isn't assigned-clocks supposed to be one of the clocks in 'clocks'?
> 
>> +                    assigned-clock-parents = <&pll0_refclk>;
> 
> And this should not be a clock in 'clocks'...
> 
> 
> More generally, why do we need to expose all these details in DT?

Sierra serdes is highly configurable w.r.t which clock can be used for
its internal PLL. The Same SoC, depending on how it is configured in the
EVM can either use internal clock or external clock. In order to
flexible support all the options, have to expose these in DT.

Thank You,
Kishon

Re: [PATCH 3/9] phy: ti: j721e-wiz: Don't configure wiz if its already configured

[Kishon Vijay Abraham I]

Hi Vinod,

On 16/11/20 1:00 pm, Vinod Koul wrote:
> On 03-11-20, 09:25, Kishon Vijay Abraham I wrote:
>> From: Faiz Abbas <faiz_abbas@ti.com>
>>
>> Serdes lanes might be shared between multiple cores in some usecases
>> and its not possible to lock PLLs for both the lanes independently
>> by the two cores. This requires a bootloader to configure both the
>> lanes at early boot time.
>>
>> To handle this case, skip all configuration if any of the lanes has
>> already been enabled.
>>
>> While we are here, also fix the wiz_init() to be called before the
>> of_platform_device_create() call.
> 
> Let's do two patches for these two issues :-)
> 
> Other than that, change lgtm, with exception of minor nit
> 
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/phy/ti/phy-j721e-wiz.c | 36 +++++++++++++++++++++-------------
>>  1 file changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
>> index d57d29382ce4..9786e8aec252 100644
>> --- a/drivers/phy/ti/phy-j721e-wiz.c
>> +++ b/drivers/phy/ti/phy-j721e-wiz.c
>> @@ -816,13 +816,14 @@ static int wiz_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *node = dev->of_node;
>>  	struct platform_device *serdes_pdev;
>> +	bool already_configured = false;
>>  	struct device_node *child_node;
>>  	struct regmap *regmap;
>>  	struct resource res;
>>  	void __iomem *base;
>>  	struct wiz *wiz;
>>  	u32 num_lanes;
>> -	int ret;
>> +	int ret, val, i;
>>  
>>  	wiz = devm_kzalloc(dev, sizeof(*wiz), GFP_KERNEL);
>>  	if (!wiz)
>> @@ -944,10 +945,26 @@ static int wiz_probe(struct platform_device *pdev)
>>  		goto err_get_sync;
>>  	}
>>  
>> -	ret = wiz_clock_init(wiz, node);
>> -	if (ret < 0) {
>> -		dev_warn(dev, "Failed to initialize clocks\n");
>> -		goto err_get_sync;
>> +	for (i = 0; i < wiz->num_lanes; i++) {
>> +		regmap_field_read(wiz->p_enable[i], &val);
>> +		if (val & (P_ENABLE | P_ENABLE_FORCE)) {
>> +			already_configured = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!already_configured) {
> 
> do you really need this variable and check, why not move the below into
> precceding block and do wiz_clock_init() and wiz_init() inside the
> if condition and drop the variable

Don't see a clean way to do it in the preceding block. So we have "N"
lanes and even if any one of the lanes is already configured, the
following block has to be executed once.

Thanks
Kishon