[PATCH v7 0/3] Add Intel ComboPhy driver

[PATCH v7 0/3] Add Intel ComboPhy driver

[Dilip Kota]

This patch series adds Intel ComboPhy driver, respective yaml schemas

Changes on v7:
  As per System control driver maintainer's inputs remove
    fwnode_to_regmap() definition and use device_node_get_regmap()
    
Changes on v6:
  Rebase patches on the latest maintainer's branch
  https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7
Dilip Kota (3):
  dt-bindings: phy: Add PHY_TYPE_XPCS definition
  dt-bindings: phy: Add YAML schemas for Intel ComboPhy
  phy: intel: Add driver support for ComboPhy

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 101 ++++
 drivers/phy/intel/Kconfig                          |  14 +
 drivers/phy/intel/Makefile                         |   1 +
 drivers/phy/intel/phy-intel-combo.c                | 627 +++++++++++++++++++++
 include/dt-bindings/phy/phy.h                      |   1 +
 5 files changed, 744 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
 create mode 100644 drivers/phy/intel/phy-intel-combo.c

-- 
2.11.0

[PATCH v7 1/3] dt-bindings: phy: Add PHY_TYPE_XPCS definition

[Dilip Kota]

Add definition for Ethernet PCS phy type.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes on v7:
  No Change

Changes on v6:
  Add Acked-by: Rob Herring <robh@kernel.org>

 include/dt-bindings/phy/phy.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
index 1f3f866fae7b..3727ef72138b 100644
--- a/include/dt-bindings/phy/phy.h
+++ b/include/dt-bindings/phy/phy.h
@@ -17,5 +17,6 @@
 #define PHY_TYPE_USB3		4
 #define PHY_TYPE_UFS		5
 #define PHY_TYPE_DP		6
+#define PHY_TYPE_XPCS		7
 
 #endif /* _DT_BINDINGS_PHY */
-- 
2.11.0

[PATCH v7 2/3] 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes on v7:
  No Change.

Changes on v6:
  Add Reviewed-by: Rob Herring <robh@kernel.org>

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 101 +++++++++++++++++++++
 1 file changed, 101 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..347d0cdfb80d
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
@@ -0,0 +1,101 @@
+# 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.
+
+properties:
+  $nodename:
+    pattern: "combophy(@.*|-[0-9a-f])*$"
+
+  compatible:
+    items:
+      - const: intel,combophy-lgm
+      - const: intel,combo-phy
+
+  clocks:
+    maxItems: 1
+
+  reg:
+    items:
+      - description: ComboPhy core registers
+      - description: PCIe app core control registers
+
+  reg-names:
+    items:
+      - const: core
+      - const: app
+
+  resets:
+    maxItems: 4
+
+  reset-names:
+    items:
+      - const: phy
+      - const: core
+      - const: iphy0
+      - const: iphy1
+
+  intel,syscfg:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: Chip configuration registers handle and ComboPhy instance id
+
+  intel,hsio:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: HSIO registers handle and ComboPhy instance id on NOC
+
+  intel,aggregation:
+    type: boolean
+    description: |
+      Specify the flag to configure ComboPHY in dual lane mode.
+
+  intel,phy-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Mode of the two phys in ComboPhy.
+      See dt-bindings/phy/phy.h for values.
+
+  "#phy-cells":
+    const: 1
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - reg-names
+  - intel,syscfg
+  - intel,hsio
+  - intel,phy-mode
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/phy/phy.h>
+    combophy@d0a00000 {
+        compatible = "intel,combophy-lgm", "intel,combo-phy";
+        clocks = <&cgu0 1>;
+        #phy-cells = <1>;
+        reg = <0xd0a00000 0x40000>,
+              <0xd0a40000 0x1000>;
+        reg-names = "core", "app";
+        resets = <&rcu0 0x50 6>,
+                 <&rcu0 0x50 17>,
+                 <&rcu0 0x50 23>,
+                 <&rcu0 0x50 24>;
+        reset-names = "phy", "core", "iphy0", "iphy1";
+        intel,syscfg = <&sysconf 0>;
+        intel,hsio = <&hsiol 0>;
+        intel,phy-mode = <PHY_TYPE_PCIE>;
+        intel,aggregation;
+    };
-- 
2.11.0

[PATCH v7 3/3] 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 v7:
  Use device_node_to_regmap instead of fwnode_to_regmap
  
Changes on v6:
  No changes

Changes on v5:
 Add changes as per inputs from Andy and Rob:
    DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h",
     add changes to handle it.
    ComboPhy no longer has children nodes, and children node properties(reset)
     moved to parent node, so do the code changes accordingly.
    Add _xlate() function to pass the appropriate phy handle.
    Fix couple of nitpicks.

Changes on v4:
 Address review comments
   Remove dependency on OF config
   Update copyright to 2019-2020
   Define register macro PAD_DIS_CFG instead of const variable inside function.
   Improve the error prints, and error returns.
   Call put_device(dev), for get_dev_from_fwnode()
   Move platform_set_drvdata() at the end of the probe().
   Correct alignment in phy_ops intel_cbphy_ops.
   Correct commented lines with proper vocabulary and punctuation.
   Add/remove commas for the required constant arrays and enums.
   Remove in driver:
     linux/kernel.h, not required
     macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL
     temp variable u32 prop;
   Change function names:
     intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse()
     intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check()
     intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse()

Changes on v3:
 Remove intel_iphy_names
 Remove struct phy in struct intel_cbphy_iphy
 Imporve if conditions logic
 Use fwnode_to_regmap()
 Call devm_of_platform_populate() to populate child nodes
 Fix reset sequence during phy_init
 Add SoC specific compatible "intel,combophy-lgm"
 Add description for enums
 Remove default case in switch {} intel_cbphy_set_mode() as it
  never happens.
 Use mutex_lock to synchronise combophy initialization across
  two phys.
 Change init_cnt to u32 datatype as it is within mutex lock.
 Correct error handling of
  fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...)

 drivers/phy/intel/Kconfig           |  14 +
 drivers/phy/intel/Makefile          |   1 +
 drivers/phy/intel/phy-intel-combo.c | 627 ++++++++++++++++++++++++++++++++++++
 3 files changed, 642 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..3b40eb7b4fb4 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -2,6 +2,20 @@
 #
 # Phy drivers for Intel Lightning Mountain(LGM) platform
 #
+config PHY_INTEL_COMBO
+	bool "Intel ComboPHY driver"
+	depends on X86 || COMPILE_TEST
+	depends on OF && HAS_IOMEM
+	select MFD_SYSCON
+	select GENERIC_PHY
+	select REGMAP
+	help
+	  Enable this to support Intel ComboPhy.
+
+	  This driver configures ComboPhy 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..04ad595e21e4
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-combo.c
@@ -0,0 +1,627 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Combo-PHY driver
+ *
+ * Copyright (C) 2019-2020 Intel Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <dt-bindings/phy/phy.h>
+
+#define PCIE_PHY_GEN_CTRL	0x00
+#define PCIE_PHY_CLK_PAD	BIT(17)
+
+#define PAD_DIS_CFG		0x174
+
+#define PCS_XF_ATE_OVRD_IN_2	0x3008
+#define ADAPT_REQ_MSK		GENMASK(5, 4)
+
+#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)
+
+#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,
+	PHY_1,
+	PHY_MAX_NUM
+};
+
+/*
+ * Clock Register bit fields to enable clocks
+ * for ComboPhy according to the mode.
+ */
+enum intel_phy_mode {
+	PHY_PCIE_MODE = 0,
+	PHY_XPCS_MODE,
+	PHY_SATA_MODE,
+};
+
+/* ComboPhy mode Register values */
+enum intel_combo_mode {
+	PCIE0_PCIE1_MODE = 0,
+	PCIE_DL_MODE,
+	RXAUI_MODE,
+	XPCS0_XPCS1_MODE,
+	SATA0_SATA1_MODE,
+};
+
+enum aggregated_mode {
+	PHY_SL_MODE,
+	PHY_DL_MODE,
+};
+
+struct intel_combo_phy;
+
+struct intel_cbphy_iphy {
+	struct phy		*phy;
+	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;
+	u32			init_cnt;
+	struct mutex		lock;
+};
+
+static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
+	u32 val;
+
+	/* Register: 0 is enable, 1 is disable */
+	val = set ? 0 : mask;
+
+	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
+				  mask, val);
+}
+
+static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 mask = BIT(cbphy->id * 2 + iphy->id);
+	u32 val;
+
+	/* Register: 0 is enable, 1 is disable */
+	val = set ? 0 : mask;
+
+	return regmap_update_bits(cbphy->syscfg, PAD_DIS_CFG, mask, 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;
+	int ret;
+
+	ret = phy_cfg(iphy);
+	if (ret)
+		return ret;
+
+	if (cbphy->aggr_mode != PHY_DL_MODE)
+		return 0;
+
+	return phy_cfg(&cbphy->iphy[PHY_1]);
+}
+
+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(cbphy->dev, "Failed to enable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (cbphy->init_cnt)
+		return 0;
+
+	combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL,
+			       PCIE_PHY_CLK_PAD, 0);
+
+	/* Delay for stable clock PLL */
+	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(cbphy->dev, "Failed to disable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (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 aggregated_mode aggr = cbphy->aggr_mode;
+	struct device *dev = cbphy->dev;
+	enum intel_combo_mode cb_mode;
+	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, "Mode:%u not support dual lane!\n", mode);
+			return -EINVAL;
+		}
+
+		cb_mode = SATA0_SATA1_MODE;
+		break;
+	}
+
+	ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
+	if (ret)
+		dev_err(dev, "Failed to set ComboPhy 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;
+	int ret;
+
+	if (!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);
+		intel_cbphy_rst_deassert(cbphy);
+		ret = intel_cbphy_set_mode(cbphy);
+		if (ret)
+			goto clk_err;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, true);
+	if (ret) {
+		dev_err(cbphy->dev, "Failed enabling PHY core\n");
+		goto clk_err;
+	}
+
+	ret = reset_control_deassert(iphy->app_rst);
+	if (ret) {
+		dev_err(cbphy->dev, "PHY(%u:%u) reset 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;
+	int ret;
+
+	ret = reset_control_assert(iphy->app_rst);
+	if (ret) {
+		dev_err(cbphy->dev, "PHY(%u:%u) reset assert failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return ret;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, false);
+	if (ret) {
+		dev_err(cbphy->dev, "Failed disabling PHY core\n");
+		return ret;
+	}
+
+	if (cbphy->init_cnt)
+		return 0;
+
+	clk_disable_unprepare(cbphy->core_clk);
+	intel_cbphy_rst_assert(cbphy);
+
+	return 0;
+}
+
+static int intel_cbphy_init(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	mutex_lock(&cbphy->lock);
+	ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
+	if (ret)
+		goto err;
+
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_pcie_en_pad_refclk);
+		if (ret)
+			goto err;
+	}
+
+	cbphy->init_cnt++;
+
+err:
+	mutex_unlock(&cbphy->lock);
+
+	return ret;
+}
+
+static int intel_cbphy_exit(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	mutex_lock(&cbphy->lock);
+	cbphy->init_cnt--;
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_pcie_dis_pad_refclk);
+		if (ret)
+			goto err;
+	}
+
+	ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
+
+err:
+	mutex_unlock(&cbphy->lock);
+
+	return ret;
+}
+
+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;
+	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(cbphy->dev, "RX Adaptation failed!\n");
+	else
+		dev_dbg(cbphy->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_fwnode_parse(struct intel_combo_phy *cbphy)
+{
+	struct device *dev = cbphy->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct fwnode_reference_args ref;
+	int ret;
+	u32 val;
+
+	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, "Get clk failed:%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 core reset control err: %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 control err: %d!\n", ret);
+		return ret;
+	}
+
+	cbphy->iphy[0].app_rst = devm_reset_control_get_optional(dev, "iphy0");
+	if (IS_ERR(cbphy->iphy[0].app_rst)) {
+		ret = PTR_ERR(cbphy->iphy[0].app_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Get phy0 reset control err: %d!\n", ret);
+		return ret;
+	}
+
+	cbphy->iphy[1].app_rst = devm_reset_control_get_optional(dev, "iphy1");
+	if (IS_ERR(cbphy->iphy[1].app_rst)) {
+		ret = PTR_ERR(cbphy->iphy[1].app_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Get phy1 reset control err: %d!\n", ret);
+		return ret;
+	}
+
+	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);
+
+	ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL,
+						 1, 0, &ref);
+	if (ret < 0)
+		return ret;
+
+	cbphy->id = ref.args[0];
+	cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode));
+	fwnode_handle_put(ref.fwnode);
+
+	ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1,
+						 0, &ref);
+	if (ret < 0)
+		return ret;
+
+	cbphy->bid = ref.args[0];
+	cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode));
+	fwnode_handle_put(ref.fwnode);
+
+	ret = fwnode_property_read_u32_array(fwnode, "intel,phy-mode", &val, 1);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case PHY_TYPE_PCIE:
+		cbphy->phy_mode = PHY_PCIE_MODE;
+		break;
+
+	case PHY_TYPE_SATA:
+		cbphy->phy_mode = PHY_SATA_MODE;
+		break;
+
+	case PHY_TYPE_XPCS:
+		cbphy->phy_mode = PHY_XPCS_MODE;
+		break;
+
+	default:
+		dev_err(dev, "Invalid PHY mode: %u\n", val);
+		return -EINVAL;
+	}
+
+	cbphy->clk_rate = intel_iphy_clk_rates[cbphy->phy_mode];
+
+	if (fwnode_property_present(fwnode, "intel,aggregation"))
+		cbphy->aggr_mode = PHY_DL_MODE;
+	else
+		cbphy->aggr_mode = PHY_SL_MODE;
+
+	return 0;
+}
+
+static const struct phy_ops intel_cbphy_ops = {
+	.init		= intel_cbphy_init,
+	.exit		= intel_cbphy_exit,
+	.calibrate	= intel_cbphy_calibrate,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *intel_cbphy_xlate(struct device *dev,
+				     struct of_phandle_args *args)
+{
+	struct intel_combo_phy *cbphy = dev_get_drvdata(dev);
+	u32 iphy_id;
+
+	if (args->args_count < 1) {
+		dev_err(dev, "Invalid number of arguments\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	iphy_id = args->args[0];
+	if (iphy_id >= PHY_MAX_NUM) {
+		dev_err(dev, "Invalid phy instance %d\n", iphy_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (cbphy->aggr_mode == PHY_DL_MODE && iphy_id == PHY_1) {
+		dev_err(dev, "Invalid. ComboPhy is in Dual lane mode %d\n", iphy_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return cbphy->iphy[iphy_id].phy;
+}
+
+static int intel_cbphy_create(struct intel_combo_phy *cbphy)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = cbphy->dev;
+	struct intel_cbphy_iphy *iphy;
+	int i;
+
+	for (i = 0; i < PHY_MAX_NUM; i++) {
+		iphy = &cbphy->iphy[i];
+		iphy->parent = cbphy;
+		iphy->id = i;
+
+		/* In dual lane mode skip phy creation for the second phy */
+		if (cbphy->aggr_mode == PHY_DL_MODE && iphy->id == PHY_1)
+			continue;
+
+		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);
+	}
+
+	dev_set_drvdata(dev, cbphy);
+	phy_provider = devm_of_phy_provider_register(dev, intel_cbphy_xlate);
+	if (IS_ERR(phy_provider))
+		dev_err(dev, "Register PHY provider failed!\n");
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+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;
+	cbphy->init_cnt = 0;
+	mutex_init(&cbphy->lock);
+	ret = intel_cbphy_fwnode_parse(cbphy);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, cbphy);
+
+	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" },
+	{ .compatible = "intel,combophy-lgm" },
+	{}
+};
+
+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 v7 0/3] Add Intel ComboPhy driver

[Lee Jones]

On Thu, 30 Apr 2020, Dilip Kota wrote:

> This patch series adds Intel ComboPhy driver, respective yaml schemas
> 
> Changes on v7:
>   As per System control driver maintainer's inputs remove
>     fwnode_to_regmap() definition and use device_node_get_regmap()
>     
> Changes on v6:
>   Rebase patches on the latest maintainer's branch
>   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7
> Dilip Kota (3):
>   dt-bindings: phy: Add PHY_TYPE_XPCS definition
>   dt-bindings: phy: Add YAML schemas for Intel ComboPhy
>   phy: intel: Add driver support for ComboPhy
> 
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 101 ++++
>  drivers/phy/intel/Kconfig                          |  14 +
>  drivers/phy/intel/Makefile                         |   1 +
>  drivers/phy/intel/phy-intel-combo.c                | 627 +++++++++++++++++++++
>  include/dt-bindings/phy/phy.h                      |   1 +

Why have you sent this to me?

>  5 files changed, 744 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>  create mode 100644 drivers/phy/intel/phy-intel-combo.c
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 0/3] Add Intel ComboPhy driver

[Dilip Kota]

On 4/30/2020 4:25 PM, Lee Jones wrote:
> On Thu, 30 Apr 2020, Dilip Kota wrote:
>
>> This patch series adds Intel ComboPhy driver, respective yaml schemas
>>
>> Changes on v7:
>>    As per System control driver maintainer's inputs remove
>>      fwnode_to_regmap() definition and use device_node_get_regmap()
>>      
>> Changes on v6:
>>    Rebase patches on the latest maintainer's branch
>>    https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7
>> Dilip Kota (3):
>>    dt-bindings: phy: Add PHY_TYPE_XPCS definition
>>    dt-bindings: phy: Add YAML schemas for Intel ComboPhy
>>    phy: intel: Add driver support for ComboPhy
>>
>>   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 101 ++++
>>   drivers/phy/intel/Kconfig                          |  14 +
>>   drivers/phy/intel/Makefile                         |   1 +
>>   drivers/phy/intel/phy-intel-combo.c                | 627 +++++++++++++++++++++
>>   include/dt-bindings/phy/phy.h                      |   1 +
> Why have you sent this to me?
The main reason for this patch series is removing fwnode_to_regmap() and 
using device_node_get_regmap() compared to previous patch series.
This has been done as per your review comments.
To keep you updated that changes are done as per your review comments, 
sent to you along with PHY maintainers.

Regards,
Dilip
>
>>   5 files changed, 744 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>   create mode 100644 drivers/phy/intel/phy-intel-combo.c
>>

Re: [PATCH v7 0/3] Add Intel ComboPhy driver

[Lee Jones]

On Thu, 30 Apr 2020, Dilip Kota wrote:

> 
> On 4/30/2020 4:25 PM, Lee Jones wrote:
> > On Thu, 30 Apr 2020, Dilip Kota wrote:
> > 
> > > This patch series adds Intel ComboPhy driver, respective yaml schemas
> > > 
> > > Changes on v7:
> > >    As per System control driver maintainer's inputs remove
> > >      fwnode_to_regmap() definition and use device_node_get_regmap()
> > > Changes on v6:
> > >    Rebase patches on the latest maintainer's branch
> > >    https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7
> > > Dilip Kota (3):
> > >    dt-bindings: phy: Add PHY_TYPE_XPCS definition
> > >    dt-bindings: phy: Add YAML schemas for Intel ComboPhy
> > >    phy: intel: Add driver support for ComboPhy
> > > 
> > >   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 101 ++++
> > >   drivers/phy/intel/Kconfig                          |  14 +
> > >   drivers/phy/intel/Makefile                         |   1 +
> > >   drivers/phy/intel/phy-intel-combo.c                | 627 +++++++++++++++++++++
> > >   include/dt-bindings/phy/phy.h                      |   1 +
> > Why have you sent this to me?

> The main reason for this patch series is removing fwnode_to_regmap() and
> using device_node_get_regmap() compared to previous patch series.
> This has been done as per your review comments.

I see.  Yes, dropping fwnode_to_regmap() was the right thing to do.

> To keep you updated that changes are done as per your review comments, sent
> to you along with PHY maintainers.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Vinod Koul]

On 30-04-20, 15:15, Dilip Kota wrote:

> +enum {
> +	PHY_0,
> +	PHY_1,
> +	PHY_MAX_NUM

PHY_MAX_NUM = PHY_1?

> +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);

bypassing regmap here... why?

> +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;
> +	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(cbphy->dev, "RX Adaptation failed!\n");

you want to continue her and not return error?
-- 
~Vinod

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Dilip Kota]

On 5/4/2020 3:29 PM, Vinod Koul wrote:
> On 30-04-20, 15:15, Dilip Kota wrote:
>
>> +enum {
>> +	PHY_0,
>> +	PHY_1,
>> +	PHY_MAX_NUM
> PHY_MAX_NUM = PHY_1?
Driver is using it for no. of PHYs/maximum PHY id.

Code snippets:

struct intel_combo_phy {
...
         struct reset_control    *phy_rst;
         struct reset_control    *core_rst;
         struct intel_cbphy_iphy iphy[PHY_MAX_NUM];
...
}

....

static int intel_cbphy_create(struct intel_combo_phy *cbphy)
{
         struct phy_provider *phy_provider;
         struct device *dev = cbphy->dev;
         struct intel_cbphy_iphy *iphy;
         int i;

         for (i = 0; i < PHY_MAX_NUM; i++) {

...

>
>> +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);
> bypassing regmap here... why?
It is not regmap address, one of the below two addresses are passed to 
this function.

struct intel_combo_phy {
...
         void __iomem            *app_base;
         void __iomem            *cr_base;
...
}


>
>> +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;
>> +	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(cbphy->dev, "RX Adaptation failed!\n");
> you want to continue her and not return error?

Next step is stopping the Adaptation, it should be done in both error 
and success case.

Regards,
Dilip

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Vinod Koul]

On 04-05-20, 16:26, Dilip Kota wrote:
> 
> On 5/4/2020 3:29 PM, Vinod Koul wrote:
> > On 30-04-20, 15:15, Dilip Kota wrote:
> > 
> > > +enum {
> > > +	PHY_0,
> > > +	PHY_1,
> > > +	PHY_MAX_NUM
> > PHY_MAX_NUM = PHY_1?
> Driver is using it for no. of PHYs/maximum PHY id.

Ok

> > > +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);
> > bypassing regmap here... why?
> It is not regmap address, one of the below two addresses are passed to this
> function.

okay, perhaps add a comment somewhere that regmap is not used for this
base?

> struct intel_combo_phy {
> ...
>         void __iomem            *app_base;
>         void __iomem            *cr_base;
> ...
> }


> > > +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;
> > > +	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(cbphy->dev, "RX Adaptation failed!\n");
> > you want to continue her and not return error?
> 
> Next step is stopping the Adaptation, it should be done in both error and
> success case.

Again documenting this helps, pls add some comments on this behaviour

-- 
~Vinod

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Dilip Kota]

On 5/4/2020 5:20 PM, Vinod Koul wrote:
> On 04-05-20, 16:26, Dilip Kota wrote:
>> On 5/4/2020 3:29 PM, Vinod Koul wrote:
>>> On 30-04-20, 15:15, Dilip Kota wrote:
>>>
>>>> +					  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);
>>> bypassing regmap here... why?
>> It is not regmap address, one of the below two addresses are passed to this
>> function.
> okay, perhaps add a comment somewhere that regmap is not used for this
> base?
I dont see a need of adding a comment, describing don't do regmap here.
>
>> struct intel_combo_phy {
>> ...
>>          void __iomem            *app_base;
>>          void __iomem            *cr_base;
>> ...
>> }
>
>>>> +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;
>>>> +	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(cbphy->dev, "RX Adaptation failed!\n");
>>> you want to continue her and not return error?
>> Next step is stopping the Adaptation, it should be done in both error and
>> success case.
> Again documenting this helps, pls add some comments on this behaviour
Comments are already in place, mentioning Start and Stop of Rx 
Adaptation. And Stop is being is done as Start is triggered, so not 
needed to mention error and success.

Regards,
Dilip
>

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Vinod Koul]

On 04-05-20, 17:32, Dilip Kota wrote:
> 
> On 5/4/2020 5:20 PM, Vinod Koul wrote:
> > On 04-05-20, 16:26, Dilip Kota wrote:
> > > On 5/4/2020 3:29 PM, Vinod Koul wrote:
> > > > On 30-04-20, 15:15, Dilip Kota wrote:
> > > > 
> > > > > +					  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);
> > > > bypassing regmap here... why?
> > > It is not regmap address, one of the below two addresses are passed to this
> > > function.
> > okay, perhaps add a comment somewhere that regmap is not used for this
> > base?
> I dont see a need of adding a comment, describing don't do regmap here.

Driver uses regmap except here, which seems odd hence explanation
required for this.

> > 
> > > struct intel_combo_phy {
> > > ...
> > >          void __iomem            *app_base;
> > >          void __iomem            *cr_base;
> > > ...
> > > }
> > 
> > > > > +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;
> > > > > +	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(cbphy->dev, "RX Adaptation failed!\n");
> > > > you want to continue her and not return error?
> > > Next step is stopping the Adaptation, it should be done in both error and
> > > success case.
> > Again documenting this helps, pls add some comments on this behaviour
> Comments are already in place, mentioning Start and Stop of Rx Adaptation.
> And Stop is being is done as Start is triggered, so not needed to mention
> error and success.

Ok

-- 
~Vinod

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Dilip Kota]

On 5/5/2020 1:21 PM, Vinod Koul wrote:
> On 04-05-20, 17:32, Dilip Kota wrote:
>> On 5/4/2020 5:20 PM, Vinod Koul wrote:
>>> On 04-05-20, 16:26, Dilip Kota wrote:
>>>> On 5/4/2020 3:29 PM, Vinod Koul wrote:
>>>>> On 30-04-20, 15:15, Dilip Kota wrote:
>>>>>
>>>>>> +					  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);
>>>>> bypassing regmap here... why?
>>>> It is not regmap address, one of the below two addresses are passed to this
>>>> function.
>>> okay, perhaps add a comment somewhere that regmap is not used for this
>>> base?
>> I dont see a need of adding a comment, describing don't do regmap here.
> Driver uses regmap except here, which seems odd hence explanation
> required for this.
During the driver Probe, the register phandles are stored in regmap 
datatype variables and PHY core addresses are stored in iomem datatype.
Since then, regmap access is performed for the regmap datatype variables 
and readl/writel access is performed on the iomem datatype variables. 
And nowhere in the driver iomem datatype address are converted to regmap 
address and performed regmap access.

Driver is not doing any 'regmap_init' on any physical address. Driver is 
getting the register address phandle from the device tree node and 
performing the regmap access.
ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 
1, 0, &ref);
[...]
cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode));

[...]
ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1, 
0, &ref);
[...]

cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode));

[...]
cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
  [...]
cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");

The DT parsing logic in the driver is explaining why the PHY driver 
should do regmap access and to whom should be done. For this reason i am 
a bit puzzled to what more is needed to explain in the comments and 
where to add it.
Please let me know your view.

Regards,
Dilip

Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Dilip Kota]

Hi Vinod,

On 5/5/2020 3:54 PM, Dilip Kota wrote:
>
> On 5/5/2020 1:21 PM, Vinod Koul wrote:
>> On 04-05-20, 17:32, Dilip Kota wrote:
>>> On 5/4/2020 5:20 PM, Vinod Koul wrote:
>>>> On 04-05-20, 16:26, Dilip Kota wrote:
>>>>> On 5/4/2020 3:29 PM, Vinod Koul wrote:
>>>>>> On 30-04-20, 15:15, Dilip Kota wrote:
>>>>>>
>>>>>>> +                      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);
>>>>>> bypassing regmap here... why?
>>>>> It is not regmap address, one of the below two addresses are 
>>>>> passed to this
>>>>> function.
>>>> okay, perhaps add a comment somewhere that regmap is not used for this
>>>> base?
>>> I dont see a need of adding a comment, describing don't do regmap here.
>> Driver uses regmap except here, which seems odd hence explanation
>> required for this.
> During the driver Probe, the register phandles are stored in regmap 
> datatype variables and PHY core addresses are stored in iomem datatype.
> Since then, regmap access is performed for the regmap datatype 
> variables and readl/writel access is performed on the iomem datatype 
> variables. And nowhere in the driver iomem datatype address are 
> converted to regmap address and performed regmap access.
>
> Driver is not doing any 'regmap_init' on any physical address. Driver 
> is getting the register address phandle from the device tree node and 
> performing the regmap access.
> ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 
> 1, 0, &ref);
> [...]
> cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 
> 1, 0, &ref);
> [...]
>
> cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
>  [...]
> cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");
>
> The DT parsing logic in the driver is explaining why the PHY driver 
> should do regmap access and to whom should be done. For this reason i 
> am a bit puzzled to what more is needed to explain in the comments and 
> where to add it.
> Please let me know your view.
>
Gentle Reminder!
Could you please update on this.

Regards,
Dilip

> Regards,
> Dilip