[PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

[PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Dilip Kota]

Combophy subsystem provides PHY support to various
controllers, viz. PCIe, SATA and EMAC.
Adding YAML schemas for the same.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
Changes on v2:

 Add custom 'select'
 Pass hardware instance entries with phandles and
   remove cell-index and bid entries
 Clock, register address space, are same for the children.
   So move them to parent node.
 Two PHY instances cannot run in different modes,
   so move the phy-mode entry to parent node.
 Add second child entry in the DT example.

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
new file mode 100644
index 000000000000..8e65a2a71e7f
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
@@ -0,0 +1,138 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Combophy Subsystem
+
+maintainers:
+  - Dilip Kota <eswara.kota@linux.intel.com>
+
+description: |
+  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
+  controllers. A single Combophy provides two PHY instances.
+
+# We need a select here so we don't match all nodes with 'simple-bus'
+select:
+  properties:
+    compatible:
+      contains:
+        const: intel,combo-phy
+  required:
+    - compatible
+
+properties:
+  $nodename:
+    pattern: "^combophy@[0-9]+$"
+
+  compatible:
+    items:
+      - const: intel,combo-phy
+      - const: simple-bus
+
+  clocks:
+    description: |
+      List of phandle and clock specifier pairs as listed
+      in clock-names property. Configure the clocks according
+      to the PHY mode.
+
+  reg:
+    items:
+      - description: ComboPhy core registers
+      - description: PCIe app core control registers
+
+  reg-names:
+    items:
+      - const: core
+      - const: app
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: phy
+      - const: core
+
+  intel,syscfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Chip configuration registers handle and ComboPhy instance id
+
+  intel,hsio:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: HSIO registers handle and ComboPhy instance id on NOC
+
+  intel,aggregation:
+    description: |
+      Specify the flag to confiure ComboPHY in dual lane mode.
+    type: boolean
+
+  intel,phy-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 2
+    description: |
+      Configure the mode of the PHY.
+        0 - PCIe
+        1 - xpcs
+        2 - sata
+
+patternProperties:
+  "^cb[0-9]phy@[0-9]+$":
+    type: object
+
+    properties:
+      compatible:
+        const: intel,phydev
+
+      "#phy-cells":
+        const: 0
+
+      resets:
+        description: |
+          reset handle according to the PHY mode.
+          See ../reset/reset.txt for details.
+
+    required:
+      - compatible
+      - "#phy-cells"
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - reg-names
+  - intel,syscfg
+  - intel,hsio
+  - intel,phy-mode
+
+additionalProperties: false
+
+examples:
+  - |
+    combophy@0 {
+        compatible = "intel,combo-phy", "simple-bus";
+        clocks = <&cgu0 1>;
+        reg = <0xd0a00000 0x40000>,
+              <0xd0a40000 0x1000>;
+        reg-names = "core", "app";
+        resets = <&rcu0 0x50 6>,
+        	 <&rcu0 0x50 17>;
+        reset-names = "phy", "core";
+        intel,syscfg = <&sysconf 0>;
+        intel,hsio = <&hsiol 0>;
+        intel,phy-mode = <0>;
+
+        cb0phy0:cb0phy@0 {
+            compatible = "intel,phydev";
+            #phy-cells = <0>;
+            resets = <&rcu0 0x50 23>;
+        };
+
+        cb0phy1:cb0phy@1 {
+            compatible = "intel,phydev";
+            #phy-cells = <0>;
+            resets = <&rcu0 0x50 24>;
+        };
+    };
-- 
2.11.0

[PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Dilip Kota]

Combophy subsystem provides PHYs for various
controllers like PCIe, SATA and EMAC.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
Changes on v2:

 intel_iphy_clk_rate -> intel_iphy_clk_rates
 Remove extra spaces and extra lines
 Update conditional logic in intel_cbphy_iphy_cfg() and intel_cbphy_exit()
 Remove function definitions for one time function calls
 fwn -> fwnode
 Simplify the child nodes traversing using fwnode_for_each_available_child_node()
 Fix yoda style conditions
 Add error checks for regmap_write/read
 Use device centric APIs for DT parsing.
 Add remove() functionality
 Add phy_caliberate() functionality
 Remove power_on() and power_off() functionality as no client using it
 Mark 'select REGMAP' in Kconfig
 
 drivers/phy/intel/Kconfig           |  13 +
 drivers/phy/intel/Makefile          |   1 +
 drivers/phy/intel/phy-intel-combo.c | 668 ++++++++++++++++++++++++++++++++++++
 3 files changed, 682 insertions(+)
 create mode 100644 drivers/phy/intel/phy-intel-combo.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 4ea6a8897cd7..bbd0792f5b43 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -2,6 +2,19 @@
 #
 # Phy drivers for Intel Lightning Mountain(LGM) platform
 #
+config PHY_INTEL_COMBO
+	bool "Intel Combo PHY driver"
+	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)
+	select MFD_SYSCON
+	select GENERIC_PHY
+	select REGMAP
+	help
+	  Enable this to support Intel combo phy.
+
+	  This driver configures combo phy subsystem on Intel gateway
+	  chipsets which provides PHYs for various controllers, EMAC,
+	  SATA and PCIe.
+
 config PHY_INTEL_EMMC
 	tristate "Intel EMMC PHY driver"
 	select GENERIC_PHY
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 6b876a75599d..233d530dadde 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.o
 obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
new file mode 100644
index 000000000000..020246f3e211
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-combo.c
@@ -0,0 +1,668 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Combo-PHY driver
+ *
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define PCIE_PHY_GEN_CTRL	0x00
+#define PCIE_PHY_CLK_PAD	BIT(17)
+#define PCIE_PHY_MPLLA_CTRL	0x10
+#define PCIE_PHY_MPLLB_CTRL	0x14
+
+#define PCS_XF_ATE_OVRD_IN_2	0x3008
+#define ADAPT_REQ_MSK		0x30
+#define PCS_XF_RX_ADAPT_ACK	0x3010
+#define RX_ADAPT_ACK_BIT	BIT(0)
+
+#define CR_ADDR(addr, lane)	(((addr) + (lane) * 0x100) << 2)
+#define REG_COMBO_MODE(x)	((x) * 0x200)
+#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)
+
+#define COMBO_PHY_ID(x)		((x)->parent->id)
+#define PHY_ID(x)		((x)->id)
+
+static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
+#define CLK_100MHZ		100000000
+#define CLK_156_25MHZ		156250000
+
+static const unsigned long intel_iphy_clk_rates[] = {
+	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ
+};
+
+enum {
+	PHY_0 = 0,
+	PHY_1,
+	PHY_MAX_NUM,
+};
+
+enum intel_phy_mode {
+	PHY_PCIE_MODE = 0,
+	PHY_XPCS_MODE,
+	PHY_SATA_MODE,
+	PHY_MAX_MODE,
+};
+
+enum intel_combo_mode {
+	PCIE0_PCIE1_MODE = 0,
+	PCIE_DL_MODE,
+	RXAUI_MODE,
+	XPCS0_XPCS1_MODE,
+	SATA0_SATA1_MODE,
+	COMBO_PHY_MODE_MAX,
+};
+
+enum aggregated_mode {
+	PHY_SL_MODE = 0,
+	PHY_DL_MODE,
+};
+
+struct intel_combo_phy;
+
+struct intel_cbphy_iphy {
+	struct phy		*phy;
+	struct device		*dev;
+	bool			enable;
+	struct intel_combo_phy	*parent;
+	struct reset_control	*app_rst;
+	u32			id;
+};
+
+struct intel_combo_phy {
+	struct device		*dev;
+	struct clk		*core_clk;
+	unsigned long		clk_rate;
+	void __iomem		*app_base;
+	void __iomem		*cr_base;
+	struct regmap		*syscfg;
+	struct regmap		*hsiocfg;
+	u32			id;
+	u32			bid;
+	struct reset_control	*phy_rst;
+	struct reset_control	*core_rst;
+	struct intel_cbphy_iphy	iphy[PHY_MAX_NUM];
+	enum intel_phy_mode	phy_mode;
+	enum aggregated_mode	aggr_mode;
+	atomic_t		init_cnt;
+};
+
+static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 val, bitn;
+
+	bitn = cbphy->phy_mode * 2 + iphy->id;
+
+	/* Register: 0 is enable, 1 is disable */
+	val =  set ? 0 : BIT(bitn);
+
+	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
+				 BIT(bitn), val);
+}
+
+static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	const u32 pad_dis_cfg_off = 0x174;
+	u32 val, bitn;
+
+	bitn = cbphy->id * 2 + iphy->id;
+
+	/* Register: 0 is enable, 1 is disable */
+	val = set ? 0 : BIT(bitn);
+
+	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
+				 val);
+}
+
+static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg,
+					  u32 mask, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl(base + reg);
+	reg_val &= ~mask;
+	reg_val |= FIELD_PREP(mask, val);
+	writel(reg_val, base + reg);
+}
+
+static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,
+				int (*phy_cfg)(struct intel_cbphy_iphy *))
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct intel_cbphy_iphy *sphy;
+	int ret;
+
+	ret = phy_cfg(iphy);
+	if (ret)
+		return ret;
+
+	if (cbphy->aggr_mode == PHY_DL_MODE) {
+		sphy = &cbphy->iphy[PHY_1];
+		ret = phy_cfg(sphy);
+	}
+
+	return ret;
+}
+
+static int intel_cbphy_pcie_en_pad_refclk(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	ret = intel_cbphy_pcie_refclk_cfg(iphy, true);
+	if (ret) {
+		dev_err(iphy->dev, "Failed to enable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (atomic_read(&cbphy->init_cnt))
+		return 0;
+
+	combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL,
+			       PCIE_PHY_CLK_PAD, 0);
+	/*
+	 * No way to identify gen1/2/3/4 for mppla and mpplb, just delay
+	 * for stable plla(gen1/gen2) or pllb(gen3/gen4)
+	 */
+	usleep_range(50, 100);
+
+	return 0;
+}
+
+static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	ret = intel_cbphy_pcie_refclk_cfg(iphy, false);
+	if (ret) {
+		dev_err(iphy->dev, "Failed to disable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (atomic_read(&cbphy->init_cnt))
+		return 0;
+
+	combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL,
+			       PCIE_PHY_CLK_PAD, 1);
+
+	return 0;
+}
+
+static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
+{
+	enum intel_combo_mode cb_mode = COMBO_PHY_MODE_MAX;
+	enum aggregated_mode aggr = cbphy->aggr_mode;
+	struct device *dev = cbphy->dev;
+	enum intel_phy_mode mode;
+	int ret;
+
+	mode = cbphy->phy_mode;
+
+	switch (mode) {
+	case PHY_PCIE_MODE:
+		cb_mode = (aggr == PHY_DL_MODE) ?
+			  PCIE_DL_MODE : PCIE0_PCIE1_MODE;
+		break;
+
+	case PHY_XPCS_MODE:
+		cb_mode = (aggr == PHY_DL_MODE) ? RXAUI_MODE : XPCS0_XPCS1_MODE;
+		break;
+
+	case PHY_SATA_MODE:
+		if (aggr == PHY_DL_MODE) {
+			dev_err(dev, "CBPHY%u mode:%u not support dual lane!\n",
+				cbphy->id, mode);
+			return -EINVAL;
+		}
+
+		cb_mode = SATA0_SATA1_MODE;
+		break;
+
+	default:
+		dev_err(dev, "CBPHY%u mode:%u not supported!\n",
+			cbphy->id, mode);
+		return -EINVAL;
+	}
+
+	ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
+	if (ret)
+		dev_err(dev, "Failed to set combo phy mode: %d\n", ret);
+
+	return ret;
+}
+
+static void intel_cbphy_rst_assert(struct intel_combo_phy *cbphy)
+{
+	reset_control_assert(cbphy->core_rst);
+	reset_control_assert(cbphy->phy_rst);
+}
+
+static void intel_cbphy_rst_deassert(struct intel_combo_phy *cbphy)
+{
+	reset_control_deassert(cbphy->core_rst);
+	reset_control_deassert(cbphy->phy_rst);
+	/* Delay to ensure reset process is done */
+	usleep_range(10, 20);
+}
+
+static int intel_cbphy_iphy_power_on(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct device *dev = iphy->dev;
+	int ret;
+
+	if (!atomic_read(&cbphy->init_cnt)) {
+		ret = clk_prepare_enable(cbphy->core_clk);
+		if (ret) {
+			dev_err(cbphy->dev, "Clock enable failed!\n");
+			return ret;
+		}
+
+		ret = clk_set_rate(cbphy->core_clk, cbphy->clk_rate);
+		if (ret) {
+			dev_err(cbphy->dev, "Clock freq set to %lu failed!\n",
+				cbphy->clk_rate);
+			goto clk_err;
+		}
+
+		intel_cbphy_rst_assert(cbphy);
+		ret = intel_cbphy_set_mode(cbphy);
+		if (ret)
+			goto clk_err;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, true);
+	if (ret) {
+		dev_err(dev, "Failed enabling Phy core\n");
+		goto clk_err;
+	}
+
+	if (!atomic_read(&cbphy->init_cnt))
+		intel_cbphy_rst_deassert(cbphy);
+
+	ret = reset_control_deassert(iphy->app_rst);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) phy deassert failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		goto clk_err;
+	}
+
+	/* Delay to ensure reset process is done */
+	udelay(1);
+
+	return 0;
+
+clk_err:
+	clk_disable_unprepare(cbphy->core_clk);
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_power_off(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct device *dev = iphy->dev;
+	int ret;
+
+	ret = reset_control_assert(iphy->app_rst);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) core assert failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return ret;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, false);
+	if (ret) {
+		dev_err(dev, "Failed disabling Phy core\n");
+		return ret;
+	}
+
+	if (!atomic_read(&cbphy->init_cnt)) {
+		clk_disable_unprepare(cbphy->core_clk);
+		intel_cbphy_rst_assert(cbphy);
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_init(struct phy *phy)
+{
+	struct intel_combo_phy *cbphy;
+	struct intel_cbphy_iphy *iphy;
+	int ret;
+
+	iphy = phy_get_drvdata(phy);
+	ret =  intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
+	if (ret)
+		return ret;
+
+	cbphy = iphy->parent;
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy,
+					   intel_cbphy_pcie_en_pad_refclk);
+		if (ret)
+			return ret;
+	}
+
+	atomic_inc(&cbphy->init_cnt);
+
+	return 0;
+}
+
+static int intel_cbphy_exit(struct phy *phy)
+{
+	struct intel_combo_phy *cbphy;
+	struct intel_cbphy_iphy *iphy;
+	int ret;
+
+	iphy = phy_get_drvdata(phy);
+	cbphy = iphy->parent;
+
+	atomic_dec(&cbphy->init_cnt);
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy,
+					   intel_cbphy_pcie_dis_pad_refclk);
+		if (ret)
+			return ret;
+	}
+
+	intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
+
+	return 0;
+}
+
+static int intel_cbphy_calibrate(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
+	struct intel_combo_phy *cbphy = iphy->parent;
+	void __iomem *cr_base = cbphy->cr_base;
+	struct device *dev = iphy->dev;
+	int val, ret, id;
+
+	if (cbphy->phy_mode != PHY_XPCS_MODE)
+		return 0;
+
+	id = PHY_ID(iphy);
+
+	/* trigger auto RX adaptation */
+	combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id),
+			       ADAPT_REQ_MSK, 3);
+	/* Wait RX adaptation to finish */
+	ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id),
+				 val, val & RX_ADAPT_ACK_BIT, 10, 5000);
+	if (ret)
+		dev_err(dev, "RX Adaptation failed!\n");
+	else
+		dev_info(dev, "RX Adaptation success!\n");
+
+	/* Stop RX adaptation */
+	combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id),
+			       ADAPT_REQ_MSK, 0);
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
+				     struct fwnode_handle *fwnode, int idx)
+{
+	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
+	struct platform_device *pdev;
+	struct device *dev;
+	int ret;
+
+	dev = get_dev_from_fwnode(fwnode);
+	pdev = to_platform_device(dev);
+	iphy->parent = cbphy;
+	iphy->dev = dev;
+	iphy->id = idx;
+
+	iphy->app_rst = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(iphy->app_rst)) {
+		ret = PTR_ERR(iphy->app_rst);
+		if (ret != -EPROBE_DEFER) {
+			dev_err(dev, "PHY[%u:%u] Get reset ctrl Err!\n",
+				COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		}
+
+		return ret;
+	}
+
+	iphy->enable = true;
+	platform_set_drvdata(pdev, iphy);
+
+	return 0;
+}
+
+static int intel_cbphy_dt_sanity_check(struct intel_combo_phy *cbphy)
+{
+	struct intel_cbphy_iphy *iphy0, *iphy1;
+
+	iphy0 = &cbphy->iphy[PHY_0];
+	iphy1 = &cbphy->iphy[PHY_1];
+
+	if (!iphy0->enable && !iphy1->enable) {
+		dev_err(cbphy->dev, "CBPHY%u both PHY disabled!\n", cbphy->id);
+		return -EINVAL;
+	}
+
+	if (cbphy->aggr_mode == PHY_DL_MODE) {
+		if (!iphy0->enable || !iphy1->enable) {
+			dev_err(cbphy->dev,
+				"Dual lane mode but lane0: %s, lane1: %s\n",
+				iphy0->enable ? "on" : "off",
+				iphy1->enable ? "on" : "off");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
+{
+	struct fwnode_reference_args ref;
+	struct device *dev = cbphy->dev;
+	struct fwnode_handle *fwnode;
+	struct platform_device *pdev;
+	int i, ret;
+	u32 prop;
+
+	cbphy->core_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(cbphy->core_clk)) {
+		ret = PTR_ERR(cbphy->core_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "CBPHY get clk failed:%d!\n", ret);
+		return ret;
+	}
+
+	cbphy->phy_rst = devm_reset_control_get_optional(dev, "phy");
+	if (IS_ERR(cbphy->phy_rst)) {
+		ret = PTR_ERR(cbphy->phy_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "get phy reset err: %d!\n", ret);
+		return ret;
+	}
+
+	cbphy->core_rst = devm_reset_control_get_optional(dev, "core");
+	if (IS_ERR(cbphy->core_rst)) {
+		ret = PTR_ERR(cbphy->core_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "get combo reset err: %d!\n", ret);
+		return ret;
+	}
+
+	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
+						 "intel,syscfg", NULL, 1, 0,
+						 &ref);
+	if (ret < 0)
+		return ret;
+
+	fwnode_handle_put(ref.fwnode);
+	cbphy->id = ref.args[0];
+	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);
+
+	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
+						 NULL, 1, 0, &ref);
+	if (ret < 0)
+		return ret;
+
+	fwnode_handle_put(ref.fwnode);
+	cbphy->bid = ref.args[0];
+	cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);
+
+	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
+		cbphy->phy_mode = prop;
+		if (cbphy->phy_mode >= PHY_MAX_MODE) {
+			dev_err(dev, "PHY mode: %u is invalid\n",
+				cbphy->phy_mode);
+			return -EINVAL;
+		}
+	}
+
+	cbphy->clk_rate = intel_iphy_clk_rates[cbphy->phy_mode];
+
+	pdev = to_platform_device(dev);
+	cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
+	if (IS_ERR(cbphy->app_base))
+		return PTR_ERR(cbphy->app_base);
+
+	cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");
+	if (IS_ERR(cbphy->cr_base))
+		return PTR_ERR(cbphy->cr_base);
+
+	if (device_property_read_bool(dev, "intel,aggregation"))
+		cbphy->aggr_mode = PHY_DL_MODE;
+	else
+		cbphy->aggr_mode = PHY_SL_MODE;
+
+	i = 0;
+	fwnode_for_each_available_child_node(dev_fwnode(dev), fwnode) {
+		if (i >= PHY_MAX_NUM) {
+			fwnode_handle_put(fwnode);
+			dev_err(dev, "Error: DT child number larger than %d\n",
+				PHY_MAX_NUM);
+			return -EINVAL;
+		}
+
+		ret = intel_cbphy_iphy_dt_parse(cbphy, fwnode, i);
+		if (ret) {
+			fwnode_handle_put(fwnode);
+			return ret;
+		}
+
+		i++;
+	}
+
+	return intel_cbphy_dt_sanity_check(cbphy);
+}
+
+static const struct phy_ops intel_cbphy_ops = {
+	.init =		intel_cbphy_init,
+	.exit =		intel_cbphy_exit,
+	.calibrate =	intel_cbphy_calibrate,
+	.owner =	THIS_MODULE,
+};
+
+static int intel_cbphy_iphy_create(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct phy_provider *phy_provider;
+	struct device *dev = iphy->dev;
+
+	/* In dual mode skip phy creation for the second phy */
+	if (cbphy->aggr_mode == PHY_DL_MODE && iphy->id)
+		return 0;
+
+	iphy->phy = devm_phy_create(dev, NULL, &intel_cbphy_ops);
+	if (IS_ERR(iphy->phy)) {
+		dev_err(dev, "PHY[%u:%u]: create PHY instance failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return PTR_ERR(iphy->phy);
+	}
+
+	phy_set_drvdata(iphy->phy, iphy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "PHY[%u:%u]: register phy provider failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+	}
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static int intel_cbphy_create(struct intel_combo_phy *cbphy)
+{
+	struct intel_cbphy_iphy *iphy;
+	int i, ret;
+
+	for (i = 0; i < PHY_MAX_NUM; i++) {
+		iphy = &cbphy->iphy[i];
+		if (iphy->enable) {
+			ret = intel_cbphy_iphy_create(iphy);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_combo_phy *cbphy;
+	int ret;
+
+	cbphy = devm_kzalloc(dev, sizeof(*cbphy), GFP_KERNEL);
+	if (!cbphy)
+		return -ENOMEM;
+
+	cbphy->dev = dev;
+	atomic_set(&cbphy->init_cnt, 0);
+	platform_set_drvdata(pdev, cbphy);
+	ret = intel_cbphy_dt_parse(cbphy);
+	if (ret)
+		return ret;
+
+	return intel_cbphy_create(cbphy);
+}
+
+static int intel_cbphy_remove(struct platform_device *pdev)
+{
+	struct intel_combo_phy *cbphy = platform_get_drvdata(pdev);
+
+	intel_cbphy_rst_assert(cbphy);
+	clk_disable_unprepare(cbphy->core_clk);
+	return 0;
+}
+
+static const struct of_device_id of_intel_cbphy_match[] = {
+	{ .compatible = "intel,combo-phy" },
+	{}
+};
+
+static struct platform_driver intel_cbphy_driver = {
+	.probe = intel_cbphy_probe,
+	.remove = intel_cbphy_remove,
+	.driver = {
+		.name = "intel-combo-phy",
+		.of_match_table = of_intel_cbphy_match,
+	}
+};
+
+module_platform_driver(intel_cbphy_driver);
+
+MODULE_DESCRIPTION("Intel Combo-phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:
> Combophy subsystem provides PHYs for various
> controllers like PCIe, SATA and EMAC.

...

> +static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};

+ blank line

> +#define CLK_100MHZ		100000000
> +#define CLK_156_25MHZ		156250000

...

> +enum {
> +	PHY_0 = 0,

Aren't enum:s start with 0 by the standard?
Ditto for all enum:s.
(Or, if it represents value from hardware, perhaps makes sense to put a comment
 to each of such enum and then all values must be explicit)

> +	PHY_1,
> +	PHY_MAX_NUM,
> +};

...

> +struct intel_cbphy_iphy {

> +	struct phy		*phy;
> +	struct device		*dev;

Can dev be derived from phy? Or phy from dev?

> +	bool			enable;
> +	struct intel_combo_phy	*parent;
> +	struct reset_control	*app_rst;
> +	u32			id;
> +};

...

> +static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
> +{
> +	struct intel_combo_phy *cbphy = iphy->parent;

> +	u32 val, bitn;
> +
> +	bitn = cbphy->phy_mode * 2 + iphy->id;

Why not

	u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
	u32 val;

> +	/* Register: 0 is enable, 1 is disable */
> +	val =  set ? 0 : BIT(bitn);

	val = set ? 0 : mask;

(why double space?)

> +
> +	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
> +				 BIT(bitn), val);

	return regmap_update_bits(..., mask, val);

?

> +}
> +
> +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> +{
> +	struct intel_combo_phy *cbphy = iphy->parent;
> +	const u32 pad_dis_cfg_off = 0x174;
> +	u32 val, bitn;
> +
> +	bitn = cbphy->id * 2 + iphy->id;
> +
> +	/* Register: 0 is enable, 1 is disable */
> +	val = set ? 0 : BIT(bitn);
> +
> +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
> +				 val);

Ditto.

> +}

...

> +static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,
> +				int (*phy_cfg)(struct intel_cbphy_iphy *))
> +{
> +	struct intel_combo_phy *cbphy = iphy->parent;
> +	struct intel_cbphy_iphy *sphy;
> +	int ret;
> +
> +	ret = phy_cfg(iphy);
> +	if (ret)
> +		return ret;
> +

> +	if (cbphy->aggr_mode == PHY_DL_MODE) {

	if (x != y)
		return 0;

> +		sphy = &cbphy->iphy[PHY_1];
> +		ret = phy_cfg(sphy);
> +	}
> +
> +	return ret;

	return phy_cfg(...);

> +}

...

> +	switch (mode) {
> +	case PHY_PCIE_MODE:

> +		cb_mode = (aggr == PHY_DL_MODE) ?
> +			  PCIE_DL_MODE : PCIE0_PCIE1_MODE;

I think one line is okay here.

> +		break;
> +
> +	case PHY_XPCS_MODE:
> +		cb_mode = (aggr == PHY_DL_MODE) ? RXAUI_MODE : XPCS0_XPCS1_MODE;
> +		break;
> +
> +	case PHY_SATA_MODE:
> +		if (aggr == PHY_DL_MODE) {
> +			dev_err(dev, "CBPHY%u mode:%u not support dual lane!\n",
> +				cbphy->id, mode);
> +			return -EINVAL;
> +		}
> +
> +		cb_mode = SATA0_SATA1_MODE;
> +		break;
> +
> +	default:
> +		dev_err(dev, "CBPHY%u mode:%u not supported!\n",
> +			cbphy->id, mode);
> +		return -EINVAL;
> +	}

...


> +	if (!atomic_read(&cbphy->init_cnt)) {

Here it can be 0.

> +		ret = clk_prepare_enable(cbphy->core_clk);
> +		if (ret) {
> +			dev_err(cbphy->dev, "Clock enable failed!\n");
> +			return ret;
> +		}
> +
> +		ret = clk_set_rate(cbphy->core_clk, cbphy->clk_rate);
> +		if (ret) {
> +			dev_err(cbphy->dev, "Clock freq set to %lu failed!\n",
> +				cbphy->clk_rate);
> +			goto clk_err;
> +		}
> +
> +		intel_cbphy_rst_assert(cbphy);
> +		ret = intel_cbphy_set_mode(cbphy);
> +		if (ret)
> +			goto clk_err;
> +	}
> +
> +	ret = intel_cbphy_iphy_enable(iphy, true);
> +	if (ret) {
> +		dev_err(dev, "Failed enabling Phy core\n");
> +		goto clk_err;
> +	}
> +

> +	if (!atomic_read(&cbphy->init_cnt))

Here it can be 1.

> +		intel_cbphy_rst_deassert(cbphy);

Is it correct way to go?

> +	ret = reset_control_deassert(iphy->app_rst);
> +	if (ret) {
> +		dev_err(dev, "PHY(%u:%u) phy deassert failed!\n",
> +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
> +		goto clk_err;
> +	}

...

> +		ret = intel_cbphy_iphy_cfg(iphy,
> +					   intel_cbphy_pcie_en_pad_refclk);

One line is fine here.

> +		if (ret)
> +			return ret;

...

> +		ret = intel_cbphy_iphy_cfg(iphy,
> +					   intel_cbphy_pcie_dis_pad_refclk);

Ditto.

> +		if (ret)
> +			return ret;

...

> +		return ret;
> +	}
> +
> +	iphy->enable = true;
> +	platform_set_drvdata(pdev, iphy);
> +
> +	return 0;
> +}

...

> +	if (cbphy->aggr_mode == PHY_DL_MODE) {
> +		if (!iphy0->enable || !iphy1->enable) {

	if (a) {
		if (b) {
			...
		}
	}

is the same as
	if (a && b) {
		...
	}

We have it many times discussed internally.

> +			dev_err(cbphy->dev,
> +				"Dual lane mode but lane0: %s, lane1: %s\n",
> +				iphy0->enable ? "on" : "off",
> +				iphy1->enable ? "on" : "off");
> +			return -EINVAL;
> +		}
> +	}

...

> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> +						 "intel,syscfg", NULL, 1, 0,
> +						 &ref);
> +	if (ret < 0)
> +		return ret;
> +

> +	fwnode_handle_put(ref.fwnode);

Why here?

> +	cbphy->id = ref.args[0];

> +	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);

You rather need to have fwnode_to_regmap(). It's easy to add as a preparatory patch.

> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
> +						 NULL, 1, 0, &ref);
> +	if (ret < 0)
> +		return ret;
> +
> +	fwnode_handle_put(ref.fwnode);
> +	cbphy->bid = ref.args[0];
> +	cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);

Ditto.

> +	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {

Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?

> +		cbphy->phy_mode = prop;
> +		if (cbphy->phy_mode >= PHY_MAX_MODE) {
> +			dev_err(dev, "PHY mode: %u is invalid\n",
> +				cbphy->phy_mode);
> +			return -EINVAL;
> +		}
> +	}

...

> +	.owner =	THIS_MODULE,

Do we still need this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Rob Herring]

On Wed, 19 Feb 2020 11:31:29 +0800, Dilip Kota wrote:
> Combophy subsystem provides PHY support to various
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
> 
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
> Changes on v2:
> 
>  Add custom 'select'
>  Pass hardware instance entries with phandles and
>    remove cell-index and bid entries
>  Clock, register address space, are same for the children.
>    So move them to parent node.
>  Two PHY instances cannot run in different modes,
>    so move the phy-mode entry to parent node.
>  Add second child entry in the DT example.
> 
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml: properties:intel,phy-mode: {'$ref': '/schemas/types.yaml#/definitions/uint32', 'minimum': 0, 'maximum': 2, 'description': 'Configure the mode of the PHY.\n  0 - PCIe\n  1 - xpcs\n  2 - sata\n'} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml: properties:intel,phy-mode: 'not' is a required property

Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/phy/intel,combo-phy.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/phy/intel,combo-phy.example.dts] Error 1
Makefile:1263: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1240526
Please check and re-submit.

Re: [PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Rob Herring]

On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
> Combophy subsystem provides PHY support to various
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
>
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
> Changes on v2:
>
>  Add custom 'select'
>  Pass hardware instance entries with phandles and
>    remove cell-index and bid entries
>  Clock, register address space, are same for the children.
>    So move them to parent node.
>  Two PHY instances cannot run in different modes,
>    so move the phy-mode entry to parent node.
>  Add second child entry in the DT example.
>
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> new file mode 100644
> index 000000000000..8e65a2a71e7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> @@ -0,0 +1,138 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Combophy Subsystem
> +
> +maintainers:
> +  - Dilip Kota <eswara.kota@linux.intel.com>
> +
> +description: |
> +  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
> +  controllers. A single Combophy provides two PHY instances.
> +
> +# We need a select here so we don't match all nodes with 'simple-bus'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: intel,combo-phy
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^combophy@[0-9]+$"

Unit addresses are hex.

> +
> +  compatible:
> +    items:
> +      - const: intel,combo-phy

Needs to be an SoC specific compatible.

> +      - const: simple-bus
> +
> +  clocks:
> +    description: |
> +      List of phandle and clock specifier pairs as listed
> +      in clock-names property. Configure the clocks according
> +      to the PHY mode.

How many?

No need to redefine a common property name, drop description. Plus,
where's clock-names?

> +
> +  reg:
> +    items:
> +      - description: ComboPhy core registers
> +      - description: PCIe app core control registers
> +
> +  reg-names:
> +    items:
> +      - const: core
> +      - const: app
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: phy
> +      - const: core
> +
> +  intel,syscfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Chip configuration registers handle and ComboPhy instance id
> +
> +  intel,hsio:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: HSIO registers handle and ComboPhy instance id on NOC
> +
> +  intel,aggregation:
> +    description: |
> +      Specify the flag to confiure ComboPHY in dual lane mode.
> +    type: boolean
> +
> +  intel,phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2
> +    description: |
> +      Configure the mode of the PHY.
> +        0 - PCIe
> +        1 - xpcs
> +        2 - sata

Doesn't this need to be per PHY? Or the 2 PHYs have to be the same mode?

Use the types defined in include/dt-bindings/phy/phy.h. You'll need to
add XPCS which maybe should be more specific to distinguish 1G, 10G,
etc. Also, we typically put the mode into the 'phys' cells so the mode
lives with the client node.

> +
> +patternProperties:
> +  "^cb[0-9]phy@[0-9]+$":

^phy@...

> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: intel,phydev
> +
> +      "#phy-cells":
> +        const: 0
> +
> +      resets:
> +        description: |
> +          reset handle according to the PHY mode.
> +          See ../reset/reset.txt for details.
> +
> +    required:
> +      - compatible
> +      - "#phy-cells"
> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - reg-names
> +  - intel,syscfg
> +  - intel,hsio
> +  - intel,phy-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    combophy@0 {
> +        compatible = "intel,combo-phy", "simple-bus";
> +        clocks = <&cgu0 1>;
> +        reg = <0xd0a00000 0x40000>,
> +              <0xd0a40000 0x1000>;
> +        reg-names = "core", "app";
> +        resets = <&rcu0 0x50 6>,
> +                <&rcu0 0x50 17>;
> +        reset-names = "phy", "core";
> +        intel,syscfg = <&sysconf 0>;
> +        intel,hsio = <&hsiol 0>;
> +        intel,phy-mode = <0>;
> +
> +        cb0phy0:cb0phy@0 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 23>;
> +        };
> +
> +        cb0phy1:cb0phy@1 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 24>;
> +        };
> +    };
> --
> 2.11.0
>

Re: [PATCH v2 1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Dilip Kota]

On 2/19/2020 10:42 PM, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota<eswara.kota@linux.intel.com>  wrote:
>> Combophy subsystem provides PHY support to various
>> controllers, viz. PCIe, SATA and EMAC.
>> Adding YAML schemas for the same.
>>
>> Signed-off-by: Dilip Kota<eswara.kota@linux.intel.com>
>> ---
>> Changes on v2:
>>
>>   Add custom 'select'
>>   Pass hardware instance entries with phandles and
>>     remove cell-index and bid entries
>>   Clock, register address space, are same for the children.
>>     So move them to parent node.
>>   Two PHY instances cannot run in different modes,
>>     so move the phy-mode entry to parent node.
>>   Add second child entry in the DT example.
>>
>>   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>>   1 file changed, 138 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>> new file mode 100644
>> index 000000000000..8e65a2a71e7f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>> @@ -0,0 +1,138 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Intel Combophy Subsystem
>> +
>> +maintainers:
>> +  - Dilip Kota<eswara.kota@linux.intel.com>
>> +
>> +description: |
>> +  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
>> +  controllers. A single Combophy provides two PHY instances.
>> +
>> +# We need a select here so we don't match all nodes with 'simple-bus'
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: intel,combo-phy
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^combophy@[0-9]+$"
> Unit addresses are hex.
Will fix it.
>
>> +
>> +  compatible:
>> +    items:
>> +      - const: intel,combo-phy
> Needs to be an SoC specific compatible.
Sure, will update it to intel, combophy-lgm
>
>> +      - const: simple-bus
>> +
>> +  clocks:
>> +    description: |
>> +      List of phandle and clock specifier pairs as listed
>> +      in clock-names property. Configure the clocks according
>> +      to the PHY mode.
> How many?
>
> No need to redefine a common property name, drop description. Plus,
> where's clock-names?
Its only one clock, i will add maxItems:1 and remove the description.
>
>> +
>> +  reg:
>> +    items:
>> +      - description: ComboPhy core registers
>> +      - description: PCIe app core control registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: core
>> +      - const: app
>> +
>> +  resets:
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: phy
>> +      - const: core
>> +
>> +  intel,syscfg:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Chip configuration registers handle and ComboPhy instance id
>> +
>> +  intel,hsio:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: HSIO registers handle and ComboPhy instance id on NOC
>> +
>> +  intel,aggregation:
>> +    description: |
>> +      Specify the flag to confiure ComboPHY in dual lane mode.
>> +    type: boolean
>> +
>> +  intel,phy-mode:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 2
>> +    description: |
>> +      Configure the mode of the PHY.
>> +        0 - PCIe
>> +        1 - xpcs
>> +        2 - sata
> Doesn't this need to be per PHY? Or the 2 PHYs have to be the same mode?
>
> Use the types defined in include/dt-bindings/phy/phy.h. You'll need to
Sure, will define the types in header file.
> add XPCS which maybe should be more specific to distinguish 1G, 10G,
> etc. Also, we typically put the mode into the 'phys' cells so the mode
> lives with the client node.
Two PHYs must be in same mode.
actual mode configuration is done for the ComboPhy to work as a two 
individual PHYs in PCIe/XPCS/SATA mode or as a single PHY providing dual 
physical lanes in PCIe/XPCS.
And mode configuration is dependent on the 'intel,aggregation' flag.
So, placed the phy-mode here itself, to make sure all the mode related 
parameters are at one location. Also, avoids setting individual PHYs in 
different modes.

>> +
>> +patternProperties:
>> +  "^cb[0-9]phy@[0-9]+$":
> ^phy@...

Sure, will fix it.

Regards,
Dilip

>

Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Dilip Kota]

On 2/19/2020 6:14 PM, Andy Shevchenko wrote:
> On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:
>> Combophy subsystem provides PHYs for various
>> controllers like PCIe, SATA and EMAC.
> ...
>
>> +static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
> + blank line
Typo, will fix it.
>
>> +#define CLK_100MHZ		100000000
>> +#define CLK_156_25MHZ		156250000
> ...
>
>> +enum {
>> +	PHY_0 = 0,
> Aren't enum:s start with 0 by the standard?
> Ditto for all enum:s.
> (Or, if it represents value from hardware, perhaps makes sense to put a comment
>   to each of such enum and then all values must be explicit)
Values are related to h/w registers, will add the description in the 
comments.
>
>> +	PHY_1,
>> +	PHY_MAX_NUM,
>> +};
> ...
>
>> +struct intel_cbphy_iphy {
>> +	struct phy		*phy;
>> +	struct device		*dev;
> Can dev be derived from phy? Or phy from dev?
I see, there is no need of storing phy. Will remove it in the next patch 
version.
>
>> +	bool			enable;
>> +	struct intel_combo_phy	*parent;
>> +	struct reset_control	*app_rst;
>> +	u32			id;
>> +};
> ...
>
>> +static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
>> +{
>> +	struct intel_combo_phy *cbphy = iphy->parent;
>> +	u32 val, bitn;
>> +
>> +	bitn = cbphy->phy_mode * 2 + iphy->id;
> Why not
>
> 	u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
> 	u32 val;
Looks more better, i will update it.
>
>> +	/* Register: 0 is enable, 1 is disable */
>> +	val =  set ? 0 : BIT(bitn);
> 	val = set ? 0 : mask;
>
> (why double space?)
Typo error. Will correct it.
>
>> +
>> +	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
>> +				 BIT(bitn), val);
> 	return regmap_update_bits(..., mask, val);
>
> ?
Still it is taking more than 80 characters with mask, need to be in 2 lines

return regmap_update_bits(...,
                                                      mask, val);

>
>> +}
>> +
>> +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
>> +{
>> +	struct intel_combo_phy *cbphy = iphy->parent;
>> +	const u32 pad_dis_cfg_off = 0x174;
>> +	u32 val, bitn;
>> +
>> +	bitn = cbphy->id * 2 + iphy->id;
>> +
>> +	/* Register: 0 is enable, 1 is disable */
>> +	val = set ? 0 : BIT(bitn);
>> +
>> +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
>> +				 val);
> Ditto.
Here it can with go in single line with mask,
>
>> +}
> ...
>
>> +static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,
>> +				int (*phy_cfg)(struct intel_cbphy_iphy *))
>> +{
>> +	struct intel_combo_phy *cbphy = iphy->parent;
>> +	struct intel_cbphy_iphy *sphy;
>> +	int ret;
>> +
>> +	ret = phy_cfg(iphy);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (cbphy->aggr_mode == PHY_DL_MODE) {
> 	if (x != y)
> 		return 0;
>
>> +		sphy = &cbphy->iphy[PHY_1];
>> +		ret = phy_cfg(sphy);
>> +	}
>> +
>> +	return ret;
> 	return phy_cfg(...);
>
>> +}
> ...
>
>> +	switch (mode) {
>> +	case PHY_PCIE_MODE:
>> +		cb_mode = (aggr == PHY_DL_MODE) ?
>> +			  PCIE_DL_MODE : PCIE0_PCIE1_MODE;
> I think one line is okay here.

its taking 82 characters.

>
>> +		break;
>> +
>> +	case PHY_XPCS_MODE:
>> +		cb_mode = (aggr == PHY_DL_MODE) ? RXAUI_MODE : XPCS0_XPCS1_MODE;
>> +		break;
>> +
>> +	case PHY_SATA_MODE:
>> +		if (aggr == PHY_DL_MODE) {
>> +			dev_err(dev, "CBPHY%u mode:%u not support dual lane!\n",
>> +				cbphy->id, mode);
>> +			return -EINVAL;
>> +		}
>> +
>> +		cb_mode = SATA0_SATA1_MODE;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "CBPHY%u mode:%u not supported!\n",
>> +			cbphy->id, mode);
>> +		return -EINVAL;
>> +	}
> ...
>
>
>> +	if (!atomic_read(&cbphy->init_cnt)) {
> Here it can be 0.
>
>> +		ret = clk_prepare_enable(cbphy->core_clk);
>> +		if (ret) {
>> +			dev_err(cbphy->dev, "Clock enable failed!\n");
>> +			return ret;
>> +		}
>> +
>> +		ret = clk_set_rate(cbphy->core_clk, cbphy->clk_rate);
>> +		if (ret) {
>> +			dev_err(cbphy->dev, "Clock freq set to %lu failed!\n",
>> +				cbphy->clk_rate);
>> +			goto clk_err;
>> +		}
>> +
>> +		intel_cbphy_rst_assert(cbphy);
>> +		ret = intel_cbphy_set_mode(cbphy);
>> +		if (ret)
>> +			goto clk_err;
>> +	}
>> +
>> +	ret = intel_cbphy_iphy_enable(iphy, true);
>> +	if (ret) {
>> +		dev_err(dev, "Failed enabling Phy core\n");
>> +		goto clk_err;
>> +	}
>> +
>> +	if (!atomic_read(&cbphy->init_cnt))
> Here it can be 1.
True,
I will fix this.
Thanks for pointing it.
>
>> +		intel_cbphy_rst_deassert(cbphy);
> Is it correct way to go?
>
>> +	ret = reset_control_deassert(iphy->app_rst);
>> +	if (ret) {
>> +		dev_err(dev, "PHY(%u:%u) phy deassert failed!\n",
>> +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
>> +		goto clk_err;
>> +	}
> ...
>
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_en_pad_refclk);
> One line is fine here.
It is taking 81 characters, so kept in 2 lines.
>
>> +		if (ret)
>> +			return ret;
> ...
>
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_dis_pad_refclk);
> Ditto.
82 characters here.
>
>> +		if (ret)
>> +			return ret;
> ...
>
>> +		return ret;
>> +	}
>> +
>> +	iphy->enable = true;
>> +	platform_set_drvdata(pdev, iphy);
>> +
>> +	return 0;
>> +}
> ...
>
>> +	if (cbphy->aggr_mode == PHY_DL_MODE) {
>> +		if (!iphy0->enable || !iphy1->enable) {
> 	if (a) {
> 		if (b) {
> 			...
> 		}
> 	}
>
> is the same as
> 	if (a && b) {
> 		...
> 	}
>
> We have it many times discussed internally.
Will fix it.
>
>> +			dev_err(cbphy->dev,
>> +				"Dual lane mode but lane0: %s, lane1: %s\n",
>> +				iphy0->enable ? "on" : "off",
>> +				iphy1->enable ? "on" : "off");
>> +			return -EINVAL;
>> +		}
>> +	}
> ...
>
>> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
>> +						 "intel,syscfg", NULL, 1, 0,
>> +						 &ref);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fwnode_handle_put(ref.fwnode);
> Why here?

Instructed to do:

" Caller is responsible to call fwnode_handle_put() on the returned   
args->fwnode pointer"

>
>> +	cbphy->id = ref.args[0];
>> +	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> You rather need to have fwnode_to_regmap(). It's easy to add as a preparatory patch.
Sure, I will add it.
>
>> +
>> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
>> +						 NULL, 1, 0, &ref);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fwnode_handle_put(ref.fwnode);
>> +	cbphy->bid = ref.args[0];
>> +	cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> Ditto.
>
>> +	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
> Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?
device_property_* are wrapper functions to fwnode_property_*().
Calling the fwnode_property_*() ending up doing the same work of 
device_property_*().

If the best practice is to maintain symmetry, will call fwnode_property_*().

>
>> +		cbphy->phy_mode = prop;
>> +		if (cbphy->phy_mode >= PHY_MAX_MODE) {
>> +			dev_err(dev, "PHY mode: %u is invalid\n",
>> +				cbphy->phy_mode);
>> +			return -EINVAL;
>> +		}
>> +	}
> ...
>
>> +	.owner =	THIS_MODULE,
> Do we still need this?
Present in all the PHY drivers,
Please let me know if it need to be removed.

Regards,
Dilip

>

Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Thu, Feb 20, 2020 at 05:58:41PM +0800, Dilip Kota wrote:
> On 2/19/2020 6:14 PM, Andy Shevchenko wrote:
> > On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:

...

> > 	return regmap_update_bits(..., mask, val);
> > 
> > ?
> Still it is taking more than 80 characters with mask, need to be in 2 lines
> 
> return regmap_update_bits(...,
>                                                      mask, val);

It's up to maintainer, I was talking about use of temporary variable for mask.

> > > +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> > > +{
> > > +	struct intel_combo_phy *cbphy = iphy->parent;
> > > +	const u32 pad_dis_cfg_off = 0x174;
> > > +	u32 val, bitn;
> > > +
> > > +	bitn = cbphy->id * 2 + iphy->id;
> > > +
> > > +	/* Register: 0 is enable, 1 is disable */
> > > +	val = set ? 0 : BIT(bitn);
> > > +
> > > +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
> > > +				 val);
> > Ditto.
> Here it can with go in single line with mask,

Here I meant all changes from previous function, yes, temporary variable mask
in particular.

> > > +}

...

> > > +	case PHY_PCIE_MODE:
> > > +		cb_mode = (aggr == PHY_DL_MODE) ?
> > > +			  PCIE_DL_MODE : PCIE0_PCIE1_MODE;
> > I think one line is okay here.

> its taking 82 characters.

Up to maintainer, but I consider the two lines approach is worse to read.

> > > +		break;

...

> > > +		ret = intel_cbphy_iphy_cfg(iphy,
> > > +					   intel_cbphy_pcie_en_pad_refclk);
> > One line is fine here.
> It is taking 81 characters, so kept in 2 lines.

Ditto.

> > > +		if (ret)
> > > +			return ret;

...

> > > +		ret = intel_cbphy_iphy_cfg(iphy,
> > > +					   intel_cbphy_pcie_dis_pad_refclk);
> > Ditto.
> 82 characters here.

Ditto.

> > > +		if (ret)
> > > +			return ret;

...

> > > +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> > > +						 "intel,syscfg", NULL, 1, 0,
> > > +						 &ref);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	fwnode_handle_put(ref.fwnode);
> > Why here?
> 
> Instructed to do:
> 
> " Caller is responsible to call fwnode_handle_put() on the returned  
> args->fwnode pointer"

Right...

> > > +	cbphy->id = ref.args[0];
> > > +	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);

...and here you called unreferenced one. Is it okay?
If it's still being referenced, that is fine, but otherwise it may gone already.


> > > +	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
> > > +						 NULL, 1, 0, &ref);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	fwnode_handle_put(ref.fwnode);
> > > +	cbphy->bid = ref.args[0];
> > > +	cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> > Ditto.
> > 
> > > +	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
> > Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?
> device_property_* are wrapper functions to fwnode_property_*().
> Calling the fwnode_property_*() ending up doing the same work of
> device_property_*().
> 
> If the best practice is to maintain symmetry, will call fwnode_property_*().

The best practice to keep consistency as much as possible.
If you call two similar APIs in one scope, it's not okay.

-- 
With Best Regards,
Andy Shevchenko