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

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

[Dilip Kota]

Combo phy subsystem provides PHY support to number of
controllers, viz. PCIe, SATA and EMAC.
Adding YAML schemas for the same.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
 1 file changed, 147 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..fc9cbad9dd88
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
@@ -0,0 +1,147 @@
+# 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 Combo phy Subsystem
+
+maintainers:
+  - Dilip Kota <eswara.kota@linux.intel.com>
+
+description: |
+  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
+  controllers. A single combo phy provides two PHY instances.
+
+properties:
+  $nodename:
+    pattern: "^combophy@[0-9]+$"
+
+  compatible:
+    items:
+      - const: intel,combo-phy
+      - const: simple-bus
+
+  cell-index:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Index of Combo phy hardware instance.
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: phy
+      - const: core
+
+  intel,syscfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Chip configuration registers handle
+
+  intel,hsio:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: HSIO registers handle
+
+  intel,bid:
+    description: Index of HSIO bus
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 1
+
+  intel,cap-pcie-only:
+    description: |
+      This flag specifies capability of the combo phy.
+      If it is set, combo phy has only PCIe capability.
+      Else it has PCIe, XPCS and SATA PHY capabilities.
+    type: boolean
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  ranges: true
+
+patternProperties:
+  "^cb[0-9]phy@[0-9]+$":
+    type: object
+
+    properties:
+      compatible:
+        const: intel,phydev
+
+      "#phy-cells":
+        const: 0
+
+      reg:
+        description: Offset and size of pcie phy control registers
+
+      intel,phy-mode:
+        description: |
+          Configure the mode of the PHY.
+            0 - PCIe
+            1 - xpcs
+            2 - sata
+        allOf:
+          - $ref: /schemas/types.yaml#/definitions/uint32
+          - minimum: 0
+          - maximum: 2
+
+      clocks:
+        description: |
+          List of phandle and clock specifier pairs as listed
+          in clock-names property. Configure the clocks according
+          to the PHY mode.
+
+      resets:
+        description: |
+          reset handle according to the PHY mode.
+          See ../reset/reset.txt for details.
+
+    required:
+      - compatible
+      - reg
+      - "#phy-cells"
+      - clocks
+      - intel,phy-mode
+
+required:
+  - compatible
+  - cell-index
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - intel,syscfg
+  - intel,hsio
+  - intel,bid
+
+additionalProperties: false
+
+examples:
+  - |
+    combophy@0 {
+        compatible = "intel,combo-phy", "simple-bus";
+        cell-index = <0>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+        resets = <&rcu0 0x50 6>,
+        	 <&rcu0 0x50 17>;
+        reset-names = "phy", "core";
+        intel,syscfg = <&sysconf>;
+        intel,hsio = <&hsiol>;
+        intel,bid = <0>;
+
+        cb0phy0:cb0phy@0 {
+            compatible = "intel,phydev";
+            #phy-cells = <0>;
+            reg = <0xd0a40000 0x1000>;
+            clocks = <&cgu0 1>;
+            resets = <&rcu0 0x50 23>;
+            intel,phy-mode = <0>;
+        };
+    };
+
+
-- 
2.11.0

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

[Dilip Kota]

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

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 drivers/phy/Kconfig                 |   1 +
 drivers/phy/Makefile                |   1 +
 drivers/phy/intel/Kconfig           |  15 +
 drivers/phy/intel/Makefile          |   2 +
 drivers/phy/intel/phy-intel-combo.c | 732 ++++++++++++++++++++++++++++++++++++
 5 files changed, 751 insertions(+)
 create mode 100644 drivers/phy/intel/Kconfig
 create mode 100644 drivers/phy/intel/Makefile
 create mode 100644 drivers/phy/intel/phy-intel-combo.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 0263db2ac874..cde11d1e57f4 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -55,6 +55,7 @@ source "drivers/phy/broadcom/Kconfig"
 source "drivers/phy/cadence/Kconfig"
 source "drivers/phy/freescale/Kconfig"
 source "drivers/phy/hisilicon/Kconfig"
+source "drivers/phy/intel/Kconfig"
 source "drivers/phy/lantiq/Kconfig"
 source "drivers/phy/marvell/Kconfig"
 source "drivers/phy/mediatek/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index c96a1afc95bd..310c149a9df5 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -18,6 +18,7 @@ obj-y					+= broadcom/	\
 					   cadence/	\
 					   freescale/	\
 					   hisilicon/	\
+					   intel/	\
 					   lantiq/	\
 					   marvell/	\
 					   motorola/	\
diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
new file mode 100644
index 000000000000..cfe7fb89f08b
--- /dev/null
+++ b/drivers/phy/intel/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Phy drivers for Intel X86 LGM platform
+#
+config PHY_INTEL_COMBO
+	bool "Intel Combo PHY driver"
+	depends on OF && HAS_IOMEM
+	select MFD_SYSCON
+	select GENERIC_PHY
+	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.
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
new file mode 100644
index 000000000000..960218133d84
--- /dev/null
+++ b/drivers/phy/intel/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.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..3fee02ab5e53
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-combo.c
@@ -0,0 +1,732 @@
+// 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/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#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 REG_COMBO_MODE(x)	((x) * 0x200)
+#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)
+
+#define CLK_100MHZ		100000000
+#define CLK_156_25MHZ		156250000
+
+#define COMBO_PHY_ID(x)	((x)->parent->id)
+#define PHY_ID(x)	((x)->id)
+
+static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
+static const unsigned long intel_iphy_clk_rate[] = {
+	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,
+};
+
+enum intel_phy_role {
+	PHY_INDIVIDUAL = 0,	/* Not aggr phy mode */
+	PHY_MASTER,		/* DL mode, PHY 0 */
+	PHY_SLAVE,		/* DL mode, PHY 1 */
+};
+
+#define	PHY_PCIE_CAP	BIT(PHY_PCIE_MODE)
+#define	PHY_XPCS_CAP	BIT(PHY_XPCS_MODE)
+#define	PHY_SATA_CAP	BIT(PHY_SATA_MODE)
+
+struct intel_combo_phy;
+
+struct intel_cbphy_iphy {
+	u32 id; /* Internal PHY idx 0 - 1 */
+	bool enable;
+	struct intel_combo_phy *parent;
+	struct platform_device *pdev;
+	struct device_node *np;
+	struct device	*dev;
+	struct phy *phy;
+	struct reset_control *app_rst;
+	void __iomem *app_base;
+	struct clk *freq_clk;
+	unsigned long clk_rate;
+	enum intel_phy_mode phy_mode;
+	enum intel_phy_role phy_role;
+	bool power_en;
+};
+
+struct intel_combo_phy {
+	u32 id; /* Physical COMBO PHY Index */
+	u32 phy_cap;
+	u32 bid; /* Bus ID */
+	struct device *dev;
+	struct regmap *syscfg;
+	struct regmap *hsiocfg;
+	struct reset_control *phy_rst;
+	struct reset_control *core_rst;
+	struct intel_cbphy_iphy iphy[PHY_MAX_NUM];
+	enum intel_combo_mode mode;
+	enum aggregated_mode aggr_mode;
+};
+
+static void intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 val, bitn;
+
+	bitn = iphy->phy_mode * 2 + iphy->id;
+
+	/* Register: 0 is enable, 1 is disable */
+	val =  set ? 0 : BIT(bitn);
+
+	regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
+			   BIT(bitn), val);
+}
+
+static void 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);
+
+	regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn), val);
+}
+
+static ssize_t intel_cbphy_info_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct intel_combo_phy *cbphy;
+	int i, off;
+
+	cbphy = dev_get_drvdata(dev);
+
+	off = sprintf(buf, "mode: %u\n", cbphy->mode);
+
+	off += sprintf(buf + off, "aggr mode: %s\n",
+		      cbphy->aggr_mode == PHY_DL_MODE ? "Yes" : "No");
+
+	off += sprintf(buf + off, "capability: ");
+	for (i = PHY_PCIE_MODE; i < PHY_MAX_MODE; i++) {
+		if (BIT(i) & cbphy->phy_cap)
+			off += sprintf(buf + off, "%s ", intel_iphy_names[i]);
+	}
+
+	off += sprintf(buf + off, "\n");
+
+	for (i = 0; i < PHY_MAX_NUM; i++) {
+		off += sprintf(buf + off, "PHY%d mode: %s, enable: %s\n",
+			       i, intel_iphy_names[cbphy->iphy[i].phy_mode],
+			       cbphy->iphy[i].enable ? "Yes" : "No");
+	}
+
+	return off;
+}
+
+static DEVICE_ATTR_RO(intel_cbphy_info);
+
+static struct attribute *intel_cbphy_attrs[] = {
+	&dev_attr_intel_cbphy_info.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(intel_cbphy);
+
+static int intel_cbphy_sysfs_init(struct intel_combo_phy *cbphy)
+{
+	return devm_device_add_groups(cbphy->dev, intel_cbphy_groups);
+}
+
+static inline void combo_phy_w32_off_mask(void __iomem *base,
+					  u32 mask, u32 set, unsigned int reg)
+{
+	u32 val;
+
+	val = readl(base + reg);
+	val &= ~mask;
+	val |= FIELD_PREP(mask, set);
+	writel(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)
+{
+	intel_cbphy_pcie_refclk_cfg(iphy, true);
+	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
+			       0, PCIE_PHY_GEN_CTRL);
+	/*
+	 * NB, 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)
+{
+	intel_cbphy_pcie_refclk_cfg(iphy, false);
+	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
+			       1, PCIE_PHY_GEN_CTRL);
+
+	return 0;
+}
+
+static int intel_cbphy_iphy_power_on(struct intel_cbphy_iphy *iphy)
+{
+	struct device *dev = iphy->dev;
+	int ret;
+
+	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));
+		return ret;
+	}
+
+	ret = clk_prepare_enable(iphy->freq_clk);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) freq clock enable failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return ret;
+	}
+
+	ret = clk_set_rate(iphy->freq_clk, iphy->clk_rate);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) clock frequency set to %lu failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy),
+			iphy->clk_rate);
+		goto clk_err;
+	}
+
+	intel_cbphy_iphy_enable(iphy, true);
+	iphy->power_en = true;
+
+	return ret;
+
+clk_err:
+	clk_disable_unprepare(iphy->freq_clk);
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_power_off(struct intel_cbphy_iphy *iphy)
+{
+	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;
+	}
+
+	intel_cbphy_iphy_enable(iphy, false);
+	clk_disable_unprepare(iphy->freq_clk);
+	iphy->power_en = false;
+
+	return 0;
+}
+
+static int intel_cbphy_init(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy;
+	int ret = 0;
+
+	iphy = phy_get_drvdata(phy);
+
+	if (iphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy,
+					   intel_cbphy_pcie_en_pad_refclk);
+	}
+
+	if (!ret)
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
+
+	return ret;
+}
+
+static int intel_cbphy_exit(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy;
+	int ret = 0;
+
+	iphy = phy_get_drvdata(phy);
+
+	if (iphy->power_en)
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
+
+	if (!ret && iphy->phy_mode == PHY_PCIE_MODE)
+		ret = intel_cbphy_iphy_cfg(iphy,
+					   intel_cbphy_pcie_dis_pad_refclk);
+
+	return ret;
+}
+
+static int intel_cbphy_power_on(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy;
+
+	iphy = phy_get_drvdata(phy);
+
+	if (iphy->power_en)
+		return 0;
+
+	return intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
+}
+
+static int intel_cbphy_power_off(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy;
+
+	iphy = phy_get_drvdata(phy);
+
+	if (!iphy->power_en)
+		return 0;
+
+	return intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
+}
+
+static int intel_cbphy_iphy_mem_resource(struct intel_cbphy_iphy *iphy)
+{
+	void __iomem *base;
+
+	base = devm_platform_ioremap_resource(iphy->pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	iphy->app_base = base;
+
+	return 0;
+}
+
+static int intel_cbphy_iphy_get_clks(struct intel_cbphy_iphy *iphy)
+{
+	enum intel_phy_mode mode = iphy->phy_mode;
+	struct device *dev = iphy->dev;
+	int ret = 0;
+
+	iphy->freq_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(iphy->freq_clk)) {
+		ret = PTR_ERR(iphy->freq_clk);
+		if (ret != -EPROBE_DEFER) {
+			dev_err(dev, "PHY[%u:%u] No %s freq clock\n",
+				COMBO_PHY_ID(iphy), PHY_ID(iphy),
+				intel_iphy_names[mode]);
+		}
+
+		return ret;
+	}
+
+	iphy->clk_rate = intel_iphy_clk_rate[mode];
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_get_reset(struct intel_cbphy_iphy *iphy)
+{
+	enum intel_phy_mode mode = iphy->phy_mode;
+	struct device *dev = iphy->dev;
+	int ret = 0;
+
+	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 %s reset ctrl Err!\n",
+				COMBO_PHY_ID(iphy),
+				PHY_ID(iphy), intel_iphy_names[mode]);
+		}
+
+		return ret;
+	}
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
+				     struct fwnode_handle *fwn, int idx)
+{
+	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
+	struct platform_device *pdev;
+	struct device *dev;
+	int ret = 0;
+	u32 prop;
+
+	iphy->id = idx;
+	iphy->enable = false;
+	iphy->power_en = false;
+	iphy->parent = cbphy;
+	iphy->np = to_of_node(fwn);
+
+	pdev = of_find_device_by_node(iphy->np);
+	if (!pdev) {
+		dev_warn(cbphy->dev, "Combo-PHY%u: PHY device: %d disabled!\n",
+			 cbphy->id, idx);
+		return 0;
+	}
+
+	dev = &pdev->dev;
+	iphy->pdev = pdev;
+	iphy->dev = dev;
+	platform_set_drvdata(pdev, iphy);
+
+	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
+		iphy->phy_mode = prop;
+		if (iphy->phy_mode >= PHY_MAX_MODE) {
+			dev_err(dev, "PHY mode: %u is invalid\n",
+				iphy->phy_mode);
+			return -EINVAL;
+		}
+	}
+
+	if (!(BIT(iphy->phy_mode) & cbphy->phy_cap)) {
+		dev_err(dev,
+			"PHY mode %u is not supported by CbPhy(%u) cap: 0x%x\n",
+			iphy->phy_mode, PHY_ID(iphy), cbphy->phy_cap);
+		return -EINVAL;
+	}
+
+	cbphy->aggr_mode = PHY_SL_MODE;
+	if (device_property_read_bool(dev, "intel,aggregation"))
+		cbphy->aggr_mode = PHY_DL_MODE;
+
+	if (cbphy->aggr_mode == PHY_DL_MODE)
+		iphy->phy_role = iphy->id ? PHY_SLAVE : PHY_MASTER;
+	else
+		iphy->phy_role = PHY_INDIVIDUAL;
+
+	ret = intel_cbphy_iphy_mem_resource(iphy);
+	if (ret)
+		return ret;
+
+	ret = intel_cbphy_iphy_get_clks(iphy);
+	if (ret)
+		return ret;
+
+	ret = intel_cbphy_iphy_get_reset(iphy);
+	if (ret)
+		return ret;
+
+	iphy->enable = fwnode_device_is_available(fwn);
+
+	dev_dbg(dev, "PHY(%u:%u) mode: %u, role: %u, enable %u\n",
+		COMBO_PHY_ID(iphy), PHY_ID(iphy),
+		iphy->phy_mode, iphy->phy_role, iphy->enable);
+
+	return ret;
+}
+
+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;
+		}
+
+		if (iphy0->phy_mode != iphy1->phy_mode) {
+			dev_err(cbphy->dev,
+				" Mode mismatch lane0 : %u, lane1 : %u\n",
+				iphy0->phy_mode, iphy1->phy_mode);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
+{
+	struct device *dev = cbphy->dev;
+	struct device_node *np = dev->of_node;
+	struct fwnode_handle *fwn;
+	int i = 0, ret = 0;
+
+	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;
+	}
+
+	cbphy->syscfg = syscon_regmap_lookup_by_phandle(np, "intel,syscfg");
+	if (IS_ERR(cbphy->syscfg)) {
+		dev_err(dev, "No syscon phandle for chip cfg registers\n");
+		return PTR_ERR(cbphy->syscfg);
+	}
+
+	cbphy->hsiocfg = syscon_regmap_lookup_by_phandle(np, "intel,hsio");
+	if (IS_ERR(cbphy->hsiocfg)) {
+		dev_err(dev, "No syscon phandle for hsio\n");
+		return PTR_ERR(cbphy->hsiocfg);
+	}
+
+	ret = device_property_read_u32(dev, "intel,bid", &cbphy->bid);
+	if (ret) {
+		dev_err(dev, "NO intel,bid provided!\n");
+		return ret;
+	}
+
+	device_for_each_child_node(dev, fwn) {
+		if (i >= PHY_MAX_NUM) {
+			fwnode_handle_put(fwn);
+			dev_err(dev, "Error: DT child number larger than %d\n",
+				PHY_MAX_NUM);
+			return -EINVAL;
+		}
+
+		ret = intel_cbphy_iphy_dt_parse(cbphy, fwn, i);
+		if (ret) {
+			fwnode_handle_put(fwn);
+			return ret;
+		}
+
+		i++;
+	}
+
+	return intel_cbphy_dt_sanity_check(cbphy);
+}
+
+static void intel_cbphy_deassert(struct intel_combo_phy *cbphy)
+{
+	reset_control_deassert(cbphy->core_rst);
+	reset_control_deassert(cbphy->phy_rst);
+	usleep_range(10, 20);
+}
+
+static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
+{
+	enum intel_combo_mode cb_mode = COMBO_PHY_MODE_MAX;
+	struct device *dev = cbphy->dev;
+	struct intel_cbphy_iphy *iphy0, *iphy1;
+	enum intel_phy_mode mode;
+	enum aggregated_mode aggr = cbphy->aggr_mode;
+
+	iphy0 = &cbphy->iphy[PHY_0];
+	iphy1 = &cbphy->iphy[PHY_1];
+
+	mode = iphy0->enable ? iphy0->phy_mode : iphy1->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;
+	}
+
+	cbphy->mode = cb_mode;
+	regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
+
+	return 0;
+}
+
+static const struct phy_ops intel_cbphy_ops = {
+	.init =		intel_cbphy_init,
+	.exit =		intel_cbphy_exit,
+	.power_on =	intel_cbphy_power_on,
+	.power_off =	intel_cbphy_power_off,
+};
+
+static int intel_cbphy_iphy_create(struct intel_cbphy_iphy *iphy)
+{
+	struct device *dev = iphy->dev;
+	struct phy_provider *phy_provider;
+
+	/* No phy instance required for slave */
+	if (iphy->phy_role == PHY_SLAVE)
+		return 0;
+
+	iphy->phy = devm_phy_create(dev, iphy->np, &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(phy_provider);
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_create(struct intel_combo_phy *cbphy)
+{
+	struct intel_cbphy_iphy *iphy;
+	int i, ret = 0;
+
+	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 ret;
+}
+
+static int intel_cbphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_combo_phy *cbphy;
+	int id, ret;
+
+	ret = of_property_read_u32(dev->of_node, "cell-index", &id);
+	if (ret) {
+		dev_err(dev, "cell index not specified:%d\n", ret);
+		return ret;
+	}
+
+	cbphy = devm_kzalloc(dev, sizeof(*cbphy), GFP_KERNEL);
+	if (!cbphy)
+		return -ENOMEM;
+
+	cbphy->phy_cap = PHY_PCIE_CAP;
+	if (!device_property_read_bool(dev, "intel,cap-pcie-only"))
+		cbphy->phy_cap |= PHY_XPCS_CAP | PHY_SATA_CAP;
+
+	cbphy->id = id;
+	cbphy->dev = dev;
+	platform_set_drvdata(pdev, cbphy);
+
+	ret = intel_cbphy_dt_parse(cbphy);
+	if (ret)
+		return ret;
+
+	intel_cbphy_deassert(cbphy);
+	ret = intel_cbphy_set_mode(cbphy);
+	if (ret)
+		return ret;
+
+	ret = intel_cbphy_create(cbphy);
+	if (ret)
+		return ret;
+
+	ret = intel_cbphy_sysfs_init(cbphy);
+
+	return ret;
+}
+
+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,
+	.driver = {
+		.name = "intel-combo-phy",
+		.of_match_table = of_intel_cbphy_match,
+	}
+};
+
+builtin_platform_driver(intel_cbphy_driver);
-- 
2.11.0

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

[Andy Shevchenko]

On Fri, Dec 20, 2019 at 03:28:28PM +0800, Dilip Kota wrote:
> Combo phy subsystem provides PHYs for various
> controllers like PCIe, SATA and EMAC.

...

> +#define REG_COMBO_MODE(x)	((x) * 0x200)

Perhaps  + 0x000

> +#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)

...

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

names (note S)
rate -> rates

> +	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ
> +};

...

> +static ssize_t intel_cbphy_info_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct intel_combo_phy *cbphy;
> +	int i, off;
> +
> +	cbphy = dev_get_drvdata(dev);
> +
> +	off = sprintf(buf, "mode: %u\n", cbphy->mode);
> +
> +	off += sprintf(buf + off, "aggr mode: %s\n",

> +		      cbphy->aggr_mode == PHY_DL_MODE ? "Yes" : "No");

Can't you do

static inline const char *yesno(bool choice)
{
	return choice ? "Yes" : "No";
}

and use it here and below?

Somebody already shared the idea that the above helper should be available
globally.

> +
> +	off += sprintf(buf + off, "capability: ");
> +	for (i = PHY_PCIE_MODE; i < PHY_MAX_MODE; i++) {
> +		if (BIT(i) & cbphy->phy_cap)
> +			off += sprintf(buf + off, "%s ", intel_iphy_names[i]);
> +	}
> +
> +	off += sprintf(buf + off, "\n");
> +
> +	for (i = 0; i < PHY_MAX_NUM; i++) {
> +		off += sprintf(buf + off, "PHY%d mode: %s, enable: %s\n",
> +			       i, intel_iphy_names[cbphy->iphy[i].phy_mode],
> +			       cbphy->iphy[i].enable ? "Yes" : "No");
> +	}
> +
> +	return off;
> +}

...

> +static struct attribute *intel_cbphy_attrs[] = {
> +	&dev_attr_intel_cbphy_info.attr,

> +	NULL,

Comma is redundant for terminator lines.

> +};


> +static int intel_cbphy_sysfs_init(struct intel_combo_phy *cbphy)
> +{
> +	return devm_device_add_groups(cbphy->dev, intel_cbphy_groups);
> +}

What the point?
Moreover, can't you use .dev_groups member of struct device_driver?

...

> +		ret =  phy_cfg(sphy);

In several places you have extra unneeded white spaces.

...

> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
> +			       0, PCIE_PHY_GEN_CTRL);

Configure your editor properly! There is plenty of room on the previous line.

...

> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
> +			       1, PCIE_PHY_GEN_CTRL);

Ditto.

...

> +static int intel_cbphy_init(struct phy *phy)
> +{
> +	struct intel_cbphy_iphy *iphy;


> +	int ret = 0;

Redundant assignment. See below.

> +
> +	iphy = phy_get_drvdata(phy);
> +
> +	if (iphy->phy_mode == PHY_PCIE_MODE) {
> +		ret = intel_cbphy_iphy_cfg(iphy,
> +					   intel_cbphy_pcie_en_pad_refclk);
> +	}
> +
> +	if (!ret)
> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
> +
> +	return ret;

Why not to simple do

	if (A) {
		ret = ...;
		if (ret)
			return ret;
	}

	return intel_...;

?

> +}
> +
> +static int intel_cbphy_exit(struct phy *phy)
> +{
> +	struct intel_cbphy_iphy *iphy;
> +	int ret = 0;
> +
> +	iphy = phy_get_drvdata(phy);
> +
> +	if (iphy->power_en)
> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
> +
> +	if (!ret && iphy->phy_mode == PHY_PCIE_MODE)
> +		ret = intel_cbphy_iphy_cfg(iphy,
> +					   intel_cbphy_pcie_dis_pad_refclk);
> +
> +	return ret;

Ditto.

> +}

...

> +static int intel_cbphy_iphy_mem_resource(struct intel_cbphy_iphy *iphy)
> +{
> +	void __iomem *base;
> +
> +	base = devm_platform_ioremap_resource(iphy->pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	iphy->app_base = base;
> +
> +	return 0;
> +}

What's the point of this helper?

...

> +static int intel_cbphy_iphy_get_clks(struct intel_cbphy_iphy *iphy)
> +{
> +	enum intel_phy_mode mode = iphy->phy_mode;
> +	struct device *dev = iphy->dev;

> +	int ret = 0;

Redundant. Simple return 0 explicitly at the end.
Ditto for other places in this patch.

> +	if (IS_ERR(iphy->freq_clk)) {
> +		ret = PTR_ERR(iphy->freq_clk);
> +		if (ret != -EPROBE_DEFER) {
> +			dev_err(dev, "PHY[%u:%u] No %s freq clock\n",
> +				COMBO_PHY_ID(iphy), PHY_ID(iphy),
> +				intel_iphy_names[mode]);
> +		}
> +
> +		return ret;
> +	}
> +
> +	iphy->clk_rate = intel_iphy_clk_rate[mode];
> +
> +	return ret;
> +}

...

> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
> +				     struct fwnode_handle *fwn, int idx)

fwn -> fwnode.

> +{
> +	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
> +	struct platform_device *pdev;
> +	struct device *dev;
> +	int ret = 0;
> +	u32 prop;
> +
> +	iphy->id = idx;
> +	iphy->enable = false;
> +	iphy->power_en = false;
> +	iphy->parent = cbphy;

> +	iphy->np = to_of_node(fwn);
> +	pdev = of_find_device_by_node(iphy->np);

Why? Can't it be done simpler?

> +	if (!pdev) {
> +		dev_warn(cbphy->dev, "Combo-PHY%u: PHY device: %d disabled!\n",
> +			 cbphy->id, idx);
> +		return 0;
> +	}

> +	if (!(BIT(iphy->phy_mode) & cbphy->phy_cap)) {

Yoda style?

...

> +				" Mode mismatch lane0 : %u, lane1 : %u\n",

Extra leading space.

...

> +static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
> +{
> +	struct device *dev = cbphy->dev;

> +	struct device_node *np = dev->of_node;

Why do you need this one? You have to device if it's OF centric driver or not.

> +	struct fwnode_handle *fwn;

Better name is fwnode as done in plenty other drivers.

> +	int i = 0, ret = 0;

i = 0 better to have near to its user.
ret = 0 is redundant assignment.


> +	ret = device_property_read_u32(dev, "intel,bid", &cbphy->bid);
> +	if (ret) {
> +		dev_err(dev, "NO intel,bid provided!\n");
> +		return ret;
> +	}
> +
> +	device_for_each_child_node(dev, fwn) {
> +		if (i >= PHY_MAX_NUM) {
> +			fwnode_handle_put(fwn);
> +			dev_err(dev, "Error: DT child number larger than %d\n",
> +				PHY_MAX_NUM);
> +			return -EINVAL;
> +		}
> +
> +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwn, i);
> +		if (ret) {
> +			fwnode_handle_put(fwn);
> +			return ret;
> +		}
> +
> +		i++;
> +	}
> +
> +	return intel_cbphy_dt_sanity_check(cbphy);
> +}

...

> +	regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);

No error check?

> +
> +	return 0;

...

> +	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(phy_provider);
> +	}
> +
> +	return 0;

return PTR_ERR_OR_ZERO(...);

...

> +	ret = of_property_read_u32(dev->of_node, "cell-index", &id);

You should decide either you go with OF centric API(s) or with device property
one as below.

> +	if (!device_property_read_bool(dev, "intel,cap-pcie-only"))
> +		cbphy->phy_cap |= PHY_XPCS_CAP | PHY_SATA_CAP;

...

> +	ret = intel_cbphy_sysfs_init(cbphy);
> +
> +	return ret;

return intel_...();

...

> +static struct platform_driver intel_cbphy_driver = {
> +	.probe = intel_cbphy_probe,
> +	.driver = {
> +		.name = "intel-combo-phy",
> +		.of_match_table = of_intel_cbphy_match,
> +	}
> +};
> +
> +builtin_platform_driver(intel_cbphy_driver);

Can we unbound it? Is it okay to do unbind/bind cycle? Had it been tested for
that?

-- 
With Best Regards,
Andy Shevchenko

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

[Dilip Kota]

On 12/20/2019 6:45 PM, Andy Shevchenko wrote:
> On Fri, Dec 20, 2019 at 03:28:28PM +0800, Dilip Kota wrote:
>> Combo phy subsystem provides PHYs for various
>> controllers like PCIe, SATA and EMAC.
> ...
>
>> +#define REG_COMBO_MODE(x)	((x) * 0x200)
> Perhaps  + 0x000
Yes, but i think not required to add explicitly.
>
>> +#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)
> ...
>
>> +static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
>> +static const unsigned long intel_iphy_clk_rate[] = {
> names (note S)
> rate -> rates
Ok, will correct it to rates.
>
>> +	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ
>> +};
> ...
>
>> +static ssize_t intel_cbphy_info_show(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	struct intel_combo_phy *cbphy;
>> +	int i, off;
>> +
>> +	cbphy = dev_get_drvdata(dev);
>> +
>> +	off = sprintf(buf, "mode: %u\n", cbphy->mode);
>> +
>> +	off += sprintf(buf + off, "aggr mode: %s\n",
>> +		      cbphy->aggr_mode == PHY_DL_MODE ? "Yes" : "No");
> Can't you do
>
> static inline const char *yesno(bool choice)
> {
> 	return choice ? "Yes" : "No";
> }
>
> and use it here and below?
>
> Somebody already shared the idea that the above helper should be available
> globally.
>
>> +
>> +	off += sprintf(buf + off, "capability: ");
>> +	for (i = PHY_PCIE_MODE; i < PHY_MAX_MODE; i++) {
>> +		if (BIT(i) & cbphy->phy_cap)
>> +			off += sprintf(buf + off, "%s ", intel_iphy_names[i]);
>> +	}
>> +
>> +	off += sprintf(buf + off, "\n");
>> +
>> +	for (i = 0; i < PHY_MAX_NUM; i++) {
>> +		off += sprintf(buf + off, "PHY%d mode: %s, enable: %s\n",
>> +			       i, intel_iphy_names[cbphy->iphy[i].phy_mode],
>> +			       cbphy->iphy[i].enable ? "Yes" : "No");
>> +	}
>> +
>> +	return off;
>> +}
> ...
>
>> +static struct attribute *intel_cbphy_attrs[] = {
>> +	&dev_attr_intel_cbphy_info.attr,
>> +	NULL,
> Comma is redundant for terminator lines.
>
>> +};
>
>> +static int intel_cbphy_sysfs_init(struct intel_combo_phy *cbphy)
>> +{
>> +	return devm_device_add_groups(cbphy->dev, intel_cbphy_groups);
>> +}
> What the point?
For debug purpose only... can be removed in upstream. I will clean it up 
in next patch version.
> Moreover, can't you use .dev_groups member of struct device_driver?
>
> ...
>
>> +		ret =  phy_cfg(sphy);
> In several places you have extra unneeded white spaces.
>
> ...
>
>> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
>> +			       0, PCIE_PHY_GEN_CTRL);
> Configure your editor properly! There is plenty of room on the previous line.
Sure, will fix at all the places.
>
> ...
>
>> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
>> +			       1, PCIE_PHY_GEN_CTRL);
> Ditto.
>
> ...
>
>> +static int intel_cbphy_init(struct phy *phy)
>> +{
>> +	struct intel_cbphy_iphy *iphy;
>
>> +	int ret = 0;
> Redundant assignment. See below.
Sure, will fix it.
>
>> +
>> +	iphy = phy_get_drvdata(phy);
>> +
>> +	if (iphy->phy_mode == PHY_PCIE_MODE) {
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_en_pad_refclk);
>> +	}
>> +
>> +	if (!ret)
>> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
>> +
>> +	return ret;
> Why not to simple do
>
> 	if (A) {
> 		ret = ...;
> 		if (ret)
> 			return ret;
> 	}
>
> 	return intel_...;
Looks good, i will update.
>
> ?
>
>> +}
>> +
>> +static int intel_cbphy_exit(struct phy *phy)
>> +{
>> +	struct intel_cbphy_iphy *iphy;
>> +	int ret = 0;
>> +
>> +	iphy = phy_get_drvdata(phy);
>> +
>> +	if (iphy->power_en)
>> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
>> +
>> +	if (!ret && iphy->phy_mode == PHY_PCIE_MODE)
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_dis_pad_refclk);
>> +
>> +	return ret;
> Ditto.
Ok
>
>> +}
> ...
>
>> +static int intel_cbphy_iphy_mem_resource(struct intel_cbphy_iphy *iphy)
>> +{
>> +	void __iomem *base;
>> +
>> +	base = devm_platform_ioremap_resource(iphy->pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	iphy->app_base = base;
>> +
>> +	return 0;
>> +}
> What's the point of this helper?
Defined as separate function for traversing memory entry from DT.
Similarly get_clks() and get_reset() functions.
>
> ...
>
>> +static int intel_cbphy_iphy_get_clks(struct intel_cbphy_iphy *iphy)
>> +{
>> +	enum intel_phy_mode mode = iphy->phy_mode;
>> +	struct device *dev = iphy->dev;
>> +	int ret = 0;
> Redundant. Simple return 0 explicitly at the end.
> Ditto for other places in this patch.
Ok, i will fix at all the places.
>
>> +	if (IS_ERR(iphy->freq_clk)) {
>> +		ret = PTR_ERR(iphy->freq_clk);
>> +		if (ret != -EPROBE_DEFER) {
>> +			dev_err(dev, "PHY[%u:%u] No %s freq clock\n",
>> +				COMBO_PHY_ID(iphy), PHY_ID(iphy),
>> +				intel_iphy_names[mode]);
>> +		}
>> +
>> +		return ret;
>> +	}
>> +
>> +	iphy->clk_rate = intel_iphy_clk_rate[mode];
>> +
>> +	return ret;
>> +}
> ...
>
>> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
>> +				     struct fwnode_handle *fwn, int idx)
> fwn -> fwnode.
Sure, i will replace it.
>
>> +{
>> +	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
>> +	struct platform_device *pdev;
>> +	struct device *dev;
>> +	int ret = 0;
>> +	u32 prop;
>> +
>> +	iphy->id = idx;
>> +	iphy->enable = false;
>> +	iphy->power_en = false;
>> +	iphy->parent = cbphy;
>> +	iphy->np = to_of_node(fwn);
>> +	pdev = of_find_device_by_node(iphy->np);
> Why? Can't it be done simpler?
There is no direct helper function to get platform device from fwnode,
I will simplify it to get  fwnode->device-> platform device. However 
iphy->np is no longer required.

>
>> +	if (!pdev) {
>> +		dev_warn(cbphy->dev, "Combo-PHY%u: PHY device: %d disabled!\n",
>> +			 cbphy->id, idx);
>> +		return 0;
>> +	}
>> +	if (!(BIT(iphy->phy_mode) & cbphy->phy_cap)) {
> Yoda style?

I will correct it to ...

if (!(cbphy->phy_cap & BIT(iphy->phy_node)))

>
> ...
>
>> +				" Mode mismatch lane0 : %u, lane1 : %u\n",
> Extra leading space.
Sure, i will fix it.
>
> ...
>
>> +static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
>> +{
>> +	struct device *dev = cbphy->dev;
>> +	struct device_node *np = dev->of_node;
> Why do you need this one? You have to device if it's OF centric driver or not.
Used during syscon_regmap call.
>
>> +	struct fwnode_handle *fwn;
> Better name is fwnode as done in plenty other drivers.
Sure will fix it.
>
>> +	int i = 0, ret = 0;
> i = 0 better to have near to its user.
> ret = 0 is redundant assignment.
Sure, will fix it.
>
>
>> +	ret = device_property_read_u32(dev, "intel,bid", &cbphy->bid);
>> +	if (ret) {
>> +		dev_err(dev, "NO intel,bid provided!\n");
>> +		return ret;
>> +	}
>> +
>> +	device_for_each_child_node(dev, fwn) {
>> +		if (i >= PHY_MAX_NUM) {
>> +			fwnode_handle_put(fwn);
>> +			dev_err(dev, "Error: DT child number larger than %d\n",
>> +				PHY_MAX_NUM);
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwn, i);
>> +		if (ret) {
>> +			fwnode_handle_put(fwn);
>> +			return ret;
>> +		}
>> +
>> +		i++;
>> +	}
>> +
>> +	return intel_cbphy_dt_sanity_check(cbphy);
>> +}
> ...
>
>> +	regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
> No error check?
Sure, will add it.
>
>> +
>> +	return 0;
> ...
>
>> +	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(phy_provider);
>> +	}
>> +
>> +	return 0;
> return PTR_ERR_OR_ZERO(...);
>
> ...
I will update it.
>
>> +	ret = of_property_read_u32(dev->of_node, "cell-index", &id);
> You should decide either you go with OF centric API(s) or with device property
> one as below.

I missed to change to device property.

I will correct it.

>
>> +	if (!device_property_read_bool(dev, "intel,cap-pcie-only"))
>> +		cbphy->phy_cap |= PHY_XPCS_CAP | PHY_SATA_CAP;
> ...
>
>> +	ret = intel_cbphy_sysfs_init(cbphy);
>> +
>> +	return ret;
> return intel_...();
Sure, will update it.
>
> ...
>
>> +static struct platform_driver intel_cbphy_driver = {
>> +	.probe = intel_cbphy_probe,
>> +	.driver = {
>> +		.name = "intel-combo-phy",
>> +		.of_match_table = of_intel_cbphy_match,
>> +	}
>> +};
>> +
>> +builtin_platform_driver(intel_cbphy_driver);
> Can we unbound it? Is it okay to do unbind/bind cycle? Had it been tested for
> that?

Unbound can be done here, i will add the respective code.

Thanks a lot for reviewing and providing the inputs.
Regards,
Dilip

>

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

[Rob Herring]

On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
> Combo phy subsystem provides PHY support to number of
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
> 
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
>  1 file changed, 147 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..fc9cbad9dd88
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> @@ -0,0 +1,147 @@
> +# 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 Combo phy Subsystem
> +
> +maintainers:
> +  - Dilip Kota <eswara.kota@linux.intel.com>
> +
> +description: |
> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
> +  controllers. A single combo phy provides two PHY instances.
> +
> +properties:
> +  $nodename:
> +    pattern: "^combophy@[0-9]+$"
> +
> +  compatible:
> +    items:
> +      - const: intel,combo-phy
> +      - const: simple-bus

This will cause the schema to be applied to every 'simple-bus'. You need 
a custom 'select' to prevent that. There's several examples in the tree.

Though I'm not sure you need child nodes here.

> +
> +  cell-index:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Index of Combo phy hardware instance.

Drop this. Not used for FDT.

> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: phy
> +      - const: core
> +
> +  intel,syscfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Chip configuration registers handle
> +
> +  intel,hsio:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: HSIO registers handle
> +
> +  intel,bid:
> +    description: Index of HSIO bus
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - minimum: 0
> +      - maximum: 1

If this is related to intel,hsio, just make it an args cell for 
intel,hsio.

> +
> +  intel,cap-pcie-only:
> +    description: |
> +      This flag specifies capability of the combo phy.
> +      If it is set, combo phy has only PCIe capability.
> +      Else it has PCIe, XPCS and SATA PHY capabilities.
> +    type: boolean
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +patternProperties:
> +  "^cb[0-9]phy@[0-9]+$":
> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: intel,phydev
> +
> +      "#phy-cells":
> +        const: 0
> +
> +      reg:
> +        description: Offset and size of pcie phy control registers
> +
> +      intel,phy-mode:
> +        description: |
> +          Configure the mode of the PHY.
> +            0 - PCIe
> +            1 - xpcs
> +            2 - sata

PHY mode is normally a cell in the client's phys property. There's 
already common defines for this.

> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32
> +          - minimum: 0
> +          - maximum: 2
> +
> +      clocks:
> +        description: |
> +          List of phandle and clock specifier pairs as listed
> +          in clock-names property. Configure the clocks according
> +          to the PHY mode.
> +
> +      resets:
> +        description: |
> +          reset handle according to the PHY mode.
> +          See ../reset/reset.txt for details.
> +
> +    required:
> +      - compatible
> +      - reg
> +      - "#phy-cells"
> +      - clocks
> +      - intel,phy-mode
> +
> +required:
> +  - compatible
> +  - cell-index
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +  - intel,syscfg
> +  - intel,hsio
> +  - intel,bid
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    combophy@0 {
> +        compatible = "intel,combo-phy", "simple-bus";
> +        cell-index = <0>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +        resets = <&rcu0 0x50 6>,
> +        	 <&rcu0 0x50 17>;
> +        reset-names = "phy", "core";
> +        intel,syscfg = <&sysconf>;
> +        intel,hsio = <&hsiol>;
> +        intel,bid = <0>;
> +
> +        cb0phy0:cb0phy@0 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            reg = <0xd0a40000 0x1000>;
> +            clocks = <&cgu0 1>;
> +            resets = <&rcu0 0x50 23>;
> +            intel,phy-mode = <0>;
> +        };

If you only have 1 child, then you don't need a child node here. Is this 
example complete?

> +    };
> +
> +
> -- 
> 2.11.0
> 

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

[Dilip Kota]

On 1/8/2020 10:18 PM, Rob Herring wrote:
> On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
>> Combo phy subsystem provides PHY support to number of
>> controllers, viz. PCIe, SATA and EMAC.
>> Adding YAML schemas for the same.
>>
>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
>> ---
>>   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
>>   1 file changed, 147 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..fc9cbad9dd88
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>> @@ -0,0 +1,147 @@
>> +# 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 Combo phy Subsystem
>> +
>> +maintainers:
>> +  - Dilip Kota <eswara.kota@linux.intel.com>
>> +
>> +description: |
>> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
>> +  controllers. A single combo phy provides two PHY instances.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^combophy@[0-9]+$"
>> +
>> +  compatible:
>> +    items:
>> +      - const: intel,combo-phy
>> +      - const: simple-bus
> This will cause the schema to be applied to every 'simple-bus'. You need
> a custom 'select' to prevent that. There's several examples in the tree.

Ok, i will add as below:

# 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

>
> Though I'm not sure you need child nodes here.
>
>> +
>> +  cell-index:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Index of Combo phy hardware instance.
> Drop this. Not used for FDT.
Ok, I will remove this and use the 'aliases' to get the hardware instance.
>
>> +
>> +  resets:
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: phy
>> +      - const: core
>> +
>> +  intel,syscfg:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Chip configuration registers handle
>> +
>> +  intel,hsio:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: HSIO registers handle
>> +
>> +  intel,bid:
>> +    description: Index of HSIO bus
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>> +      - minimum: 0
>> +      - maximum: 1
> If this is related to intel,hsio, just make it an args cell for
> intel,hsio.
No. Actually, this is specific to the combophy instance on the HSIO bus.
I see , this can be removed and value can be derived from the hardware 
instance value mentioned through 'aliases'
>> +
>> +  intel,cap-pcie-only:
>> +    description: |
>> +      This flag specifies capability of the combo phy.
>> +      If it is set, combo phy has only PCIe capability.
>> +      Else it has PCIe, XPCS and SATA PHY capabilities.
>> +    type: boolean
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  ranges: true
>> +
>> +patternProperties:
>> +  "^cb[0-9]phy@[0-9]+$":
>> +    type: object
>> +
>> +    properties:
>> +      compatible:
>> +        const: intel,phydev
>> +
>> +      "#phy-cells":
>> +        const: 0
>> +
>> +      reg:
>> +        description: Offset and size of pcie phy control registers
>> +
>> +      intel,phy-mode:
>> +        description: |
>> +          Configure the mode of the PHY.
>> +            0 - PCIe
>> +            1 - xpcs
>> +            2 - sata
> PHY mode is normally a cell in the client's phys property. There's
> already common defines for this.
Sure, will update the code to use phys property and remove this entry,
>
>> +        allOf:
>> +          - $ref: /schemas/types.yaml#/definitions/uint32
>> +          - minimum: 0
>> +          - maximum: 2
>> +
>> +      clocks:
>> +        description: |
>> +          List of phandle and clock specifier pairs as listed
>> +          in clock-names property. Configure the clocks according
>> +          to the PHY mode.
>> +
>> +      resets:
>> +        description: |
>> +          reset handle according to the PHY mode.
>> +          See ../reset/reset.txt for details.
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - "#phy-cells"
>> +      - clocks
>> +      - intel,phy-mode
>> +
>> +required:
>> +  - compatible
>> +  - cell-index
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - ranges
>> +  - intel,syscfg
>> +  - intel,hsio
>> +  - intel,bid
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    combophy@0 {
>> +        compatible = "intel,combo-phy", "simple-bus";
>> +        cell-index = <0>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +        resets = <&rcu0 0x50 6>,
>> +        	 <&rcu0 0x50 17>;
>> +        reset-names = "phy", "core";
>> +        intel,syscfg = <&sysconf>;
>> +        intel,hsio = <&hsiol>;
>> +        intel,bid = <0>;
>> +
>> +        cb0phy0:cb0phy@0 {
>> +            compatible = "intel,phydev";
>> +            #phy-cells = <0>;
>> +            reg = <0xd0a40000 0x1000>;
>> +            clocks = <&cgu0 1>;
>> +            resets = <&rcu0 0x50 23>;
>> +            intel,phy-mode = <0>;
>> +        };
> If you only have 1 child, then you don't need a child node here. Is this
> example complete?
2 children are required, as it is an example i just added one.
I will add the other child too.

Thanks for reviewing and giving the inputs.


Regards,
Dilip

>> +    };
>> +
>> +
>> -- 
>> 2.11.0
>>

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

[Rob Herring]

On Tue, Jan 14, 2020 at 3:18 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
>
> On 1/8/2020 10:18 PM, Rob Herring wrote:
> > On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
> >> Combo phy subsystem provides PHY support to number of
> >> controllers, viz. PCIe, SATA and EMAC.
> >> Adding YAML schemas for the same.
> >>
> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> >> ---
> >>   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
> >>   1 file changed, 147 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..fc9cbad9dd88
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> >> @@ -0,0 +1,147 @@
> >> +# 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 Combo phy Subsystem
> >> +
> >> +maintainers:
> >> +  - Dilip Kota <eswara.kota@linux.intel.com>
> >> +
> >> +description: |
> >> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
> >> +  controllers. A single combo phy provides two PHY instances.
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: "^combophy@[0-9]+$"
> >> +
> >> +  compatible:
> >> +    items:
> >> +      - const: intel,combo-phy
> >> +      - const: simple-bus
> > This will cause the schema to be applied to every 'simple-bus'. You need
> > a custom 'select' to prevent that. There's several examples in the tree.
>
> Ok, i will add as below:
>
> # 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
>
> >
> > Though I'm not sure you need child nodes here.
> >
> >> +
> >> +  cell-index:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description: Index of Combo phy hardware instance.
> > Drop this. Not used for FDT.
> Ok, I will remove this and use the 'aliases' to get the hardware instance.
> >
> >> +
> >> +  resets:
> >> +    maxItems: 2
> >> +
> >> +  reset-names:
> >> +    items:
> >> +      - const: phy
> >> +      - const: core
> >> +
> >> +  intel,syscfg:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description: Chip configuration registers handle
> >> +
> >> +  intel,hsio:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle
> >> +    description: HSIO registers handle
> >> +
> >> +  intel,bid:
> >> +    description: Index of HSIO bus
> >> +    allOf:
> >> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> +      - minimum: 0
> >> +      - maximum: 1
> > If this is related to intel,hsio, just make it an args cell for
> > intel,hsio.
> No. Actually, this is specific to the combophy instance on the HSIO bus.
> I see , this can be removed and value can be derived from the hardware
> instance value mentioned through 'aliases'

Generally, 'aliases' should be optional. Why do you need an index?
What's the difference between the blocks?

If it wasn't clear, I was suggesting doing:

intel,hsio = <&hsio 1>;

Rob

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

[Dilip Kota]

On 1/14/2020 10:31 PM, Rob Herring wrote:
> On Tue, Jan 14, 2020 at 3:18 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>>
>> On 1/8/2020 10:18 PM, Rob Herring wrote:
>>> On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
>>>> Combo phy subsystem provides PHY support to number of
>>>> controllers, viz. PCIe, SATA and EMAC.
>>>> Adding YAML schemas for the same.
>>>>
>>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
>>>> ---
>>>>    .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
>>>>    1 file changed, 147 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..fc9cbad9dd88
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>>> @@ -0,0 +1,147 @@
>>>> +# 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 Combo phy Subsystem
>>>> +
>>>> +maintainers:
>>>> +  - Dilip Kota <eswara.kota@linux.intel.com>
>>>> +
>>>> +description: |
>>>> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
>>>> +  controllers. A single combo phy provides two PHY instances.
>>>> +
>>>> +properties:
>>>> +  $nodename:
>>>> +    pattern: "^combophy@[0-9]+$"
>>>> +
>>>> +  compatible:
>>>> +    items:
>>>> +      - const: intel,combo-phy
>>>> +      - const: simple-bus
>>> This will cause the schema to be applied to every 'simple-bus'. You need
>>> a custom 'select' to prevent that. There's several examples in the tree.
>> Ok, i will add as below:
>>
>> # 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
>>
>>> Though I'm not sure you need child nodes here.
>>>
>>>> +
>>>> +  cell-index:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: Index of Combo phy hardware instance.
>>> Drop this. Not used for FDT.
>> Ok, I will remove this and use the 'aliases' to get the hardware instance.
>>>> +
>>>> +  resets:
>>>> +    maxItems: 2
>>>> +
>>>> +  reset-names:
>>>> +    items:
>>>> +      - const: phy
>>>> +      - const: core
>>>> +
>>>> +  intel,syscfg:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    description: Chip configuration registers handle
>>>> +
>>>> +  intel,hsio:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    description: HSIO registers handle
>>>> +
>>>> +  intel,bid:
>>>> +    description: Index of HSIO bus
>>>> +    allOf:
>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>> +      - minimum: 0
>>>> +      - maximum: 1
>>> If this is related to intel,hsio, just make it an args cell for
>>> intel,hsio.
>> No. Actually, this is specific to the combophy instance on the HSIO bus.
>> I see , this can be removed and value can be derived from the hardware
>> instance value mentioned through 'aliases'
> Generally, 'aliases' should be optional. Why do you need an index?
> What's the difference between the blocks?
>
> If it wasn't clear, I was suggesting doing:
>
> intel,hsio = <&hsio 1>;
On the SoC, total 4 combophy (0,1,2 and 3) instances are present -> 
'cell-index'
2 instances (0,1) are present on the HSIOL NoC
Other 2 instances (2,3) are present on the HSIOR NoC
On the both HSIO NoCs, combophy instances are referred as 0 and 1 -> 'bid'

'bid' is required while accessing the registers in hsio block, to 
configure the COMBOPHY mode and clock
'cell-index' is required while accessing sysconfig registers to enable 
the pcie phy pad ref clock.

<&hsio 1>
'bid' is specific to the combophy, not all the DT nodes using &hsio has 
a need.
I think it is better to pass the bid value as a entry of combophy DT node.

I will add dt entry something like 'hw-instance-id' instead of 
cell-index or aliases.

Regards,
Dilip

>
> Rob

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

[Rob Herring]

On Wed, Jan 15, 2020 at 1:52 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
>
> On 1/14/2020 10:31 PM, Rob Herring wrote:
> > On Tue, Jan 14, 2020 at 3:18 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
> >>
> >> On 1/8/2020 10:18 PM, Rob Herring wrote:
> >>> On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
> >>>> Combo phy subsystem provides PHY support to number of
> >>>> controllers, viz. PCIe, SATA and EMAC.
> >>>> Adding YAML schemas for the same.
> >>>>
> >>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> >>>> ---
> >>>>    .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
> >>>>    1 file changed, 147 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..fc9cbad9dd88
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> >>>> @@ -0,0 +1,147 @@
> >>>> +# 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 Combo phy Subsystem
> >>>> +
> >>>> +maintainers:
> >>>> +  - Dilip Kota <eswara.kota@linux.intel.com>
> >>>> +
> >>>> +description: |
> >>>> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
> >>>> +  controllers. A single combo phy provides two PHY instances.
> >>>> +
> >>>> +properties:
> >>>> +  $nodename:
> >>>> +    pattern: "^combophy@[0-9]+$"
> >>>> +
> >>>> +  compatible:
> >>>> +    items:
> >>>> +      - const: intel,combo-phy
> >>>> +      - const: simple-bus
> >>> This will cause the schema to be applied to every 'simple-bus'. You need
> >>> a custom 'select' to prevent that. There's several examples in the tree.
> >> Ok, i will add as below:
> >>
> >> # 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
> >>
> >>> Though I'm not sure you need child nodes here.
> >>>
> >>>> +
> >>>> +  cell-index:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    description: Index of Combo phy hardware instance.
> >>> Drop this. Not used for FDT.
> >> Ok, I will remove this and use the 'aliases' to get the hardware instance.
> >>>> +
> >>>> +  resets:
> >>>> +    maxItems: 2
> >>>> +
> >>>> +  reset-names:
> >>>> +    items:
> >>>> +      - const: phy
> >>>> +      - const: core
> >>>> +
> >>>> +  intel,syscfg:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description: Chip configuration registers handle
> >>>> +
> >>>> +  intel,hsio:
> >>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +    description: HSIO registers handle
> >>>> +
> >>>> +  intel,bid:
> >>>> +    description: Index of HSIO bus
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +      - minimum: 0
> >>>> +      - maximum: 1
> >>> If this is related to intel,hsio, just make it an args cell for
> >>> intel,hsio.
> >> No. Actually, this is specific to the combophy instance on the HSIO bus.
> >> I see , this can be removed and value can be derived from the hardware
> >> instance value mentioned through 'aliases'
> > Generally, 'aliases' should be optional. Why do you need an index?
> > What's the difference between the blocks?
> >
> > If it wasn't clear, I was suggesting doing:
> >
> > intel,hsio = <&hsio 1>;
> On the SoC, total 4 combophy (0,1,2 and 3) instances are present ->
> 'cell-index'
> 2 instances (0,1) are present on the HSIOL NoC
> Other 2 instances (2,3) are present on the HSIOR NoC
> On the both HSIO NoCs, combophy instances are referred as 0 and 1 -> 'bid'

So you would have:

<&hsiol 0>
<&hsiol 1>
<&hsior 0>
<&hsior 1>

However, if HSIO is a bus and the combo phys are not on any other bus,
then perhaps you should be describing the buses in DT and then this
property goes away as these would be child nodes of the bus and
whatever addressing identifiers there are would go in 'reg'.

> 'bid' is required while accessing the registers in hsio block, to
> configure the COMBOPHY mode and clock
> 'cell-index' is required while accessing sysconfig registers to enable
> the pcie phy pad ref clock.

Do the same thing for the sysconfig handle:

<&sysconfig 0|1>

This is the common pattern for these types of properties with misc
extra register bits to go configure. Though more typically the cell
value is a register offset and bit position.

> <&hsio 1>
> 'bid' is specific to the combophy, not all the DT nodes using &hsio has
> a need.
> I think it is better to pass the bid value as a entry of combophy DT node.

intel,hsio is an entry in the combo phy. The meaning of any arg cells
is defined by the combo phy binding (and driver).

> I will add dt entry something like 'hw-instance-id' instead of
> cell-index or aliases.

As I said, we don't do h/w index properties.

Rob

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

[Dilip Kota]

On 1/16/2020 3:51 AM, Rob Herring wrote:
> On Wed, Jan 15, 2020 at 1:52 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>>
>> On 1/14/2020 10:31 PM, Rob Herring wrote:
>>> On Tue, Jan 14, 2020 at 3:18 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>>>> On 1/8/2020 10:18 PM, Rob Herring wrote:
>>>>> On Fri, Dec 20, 2019 at 03:28:27PM +0800, Dilip Kota wrote:
>>>>>> Combo phy subsystem provides PHY support to number of
>>>>>> controllers, viz. PCIe, SATA and EMAC.
>>>>>> Adding YAML schemas for the same.
>>>>>>
>>>>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
>>>>>> ---
>>>>>>     .../devicetree/bindings/phy/intel,combo-phy.yaml   | 147 +++++++++++++++++++++
>>>>>>     1 file changed, 147 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..fc9cbad9dd88
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>>>>> @@ -0,0 +1,147 @@
>>>>>> +# 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 Combo phy Subsystem
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Dilip Kota <eswara.kota@linux.intel.com>
>>>>>> +
>>>>>> +description: |
>>>>>> +  Intel combo phy subsystem supports PHYs for PCIe, EMAC and SATA
>>>>>> +  controllers. A single combo phy provides two PHY instances.
>>>>>> +
>>>>>> +properties:
>>>>>> +  $nodename:
>>>>>> +    pattern: "^combophy@[0-9]+$"
>>>>>> +
>>>>>> +  compatible:
>>>>>> +    items:
>>>>>> +      - const: intel,combo-phy
>>>>>> +      - const: simple-bus
>>>>> This will cause the schema to be applied to every 'simple-bus'. You need
>>>>> a custom 'select' to prevent that. There's several examples in the tree.
>>>> Ok, i will add as below:
>>>>
>>>> # 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
>>>>
>>>>> Though I'm not sure you need child nodes here.
>>>>>
>>>>>> +
>>>>>> +  cell-index:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description: Index of Combo phy hardware instance.
>>>>> Drop this. Not used for FDT.
>>>> Ok, I will remove this and use the 'aliases' to get the hardware instance.
>>>>>> +
>>>>>> +  resets:
>>>>>> +    maxItems: 2
>>>>>> +
>>>>>> +  reset-names:
>>>>>> +    items:
>>>>>> +      - const: phy
>>>>>> +      - const: core
>>>>>> +
>>>>>> +  intel,syscfg:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> +    description: Chip configuration registers handle
>>>>>> +
>>>>>> +  intel,hsio:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> +    description: HSIO registers handle
>>>>>> +
>>>>>> +  intel,bid:
>>>>>> +    description: Index of HSIO bus
>>>>>> +    allOf:
>>>>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +      - minimum: 0
>>>>>> +      - maximum: 1
>>>>> If this is related to intel,hsio, just make it an args cell for
>>>>> intel,hsio.
>>>> No. Actually, this is specific to the combophy instance on the HSIO bus.
>>>> I see , this can be removed and value can be derived from the hardware
>>>> instance value mentioned through 'aliases'
>>> Generally, 'aliases' should be optional. Why do you need an index?
>>> What's the difference between the blocks?
>>>
>>> If it wasn't clear, I was suggesting doing:
>>>
>>> intel,hsio = <&hsio 1>;
>> On the SoC, total 4 combophy (0,1,2 and 3) instances are present ->
>> 'cell-index'
>> 2 instances (0,1) are present on the HSIOL NoC
>> Other 2 instances (2,3) are present on the HSIOR NoC
>> On the both HSIO NoCs, combophy instances are referred as 0 and 1 -> 'bid'
> So you would have:
>
> <&hsiol 0>
> <&hsiol 1>
> <&hsior 0>
> <&hsior 1>
>
> However, if HSIO is a bus and the combo phys are not on any other bus,
> then perhaps you should be describing the buses in DT and then this
> property goes away as these would be child nodes of the bus and
> whatever addressing identifiers there are would go in 'reg'.
>
>> 'bid' is required while accessing the registers in hsio block, to
>> configure the COMBOPHY mode and clock
>> 'cell-index' is required while accessing sysconfig registers to enable
>> the pcie phy pad ref clock.
> Do the same thing for the sysconfig handle:
>
> <&sysconfig 0|1>
>
> This is the common pattern for these types of properties with misc
> extra register bits to go configure. Though more typically the cell
> value is a register offset and bit position.
>
>> <&hsio 1>
>> 'bid' is specific to the combophy, not all the DT nodes using &hsio has
>> a need.
>> I think it is better to pass the bid value as a entry of combophy DT node.
> intel,hsio is an entry in the combo phy. The meaning of any arg cells
> is defined by the combo phy binding (and driver).
>
>> I will add dt entry something like 'hw-instance-id' instead of
>> cell-index or aliases.
> As I said, we don't do h/w index properties.

Ok, then i will pass it as you suggested -> <&hsiol 1>

Regards,
Dilip

>
> Rob