[PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets
@ 2016-11-22 12:02 Vivek Gautam
  2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-11-22 12:02 UTC (permalink / raw)
  To: kishon, robh+dt, mark.rutland, devicetree, linux-kernel
  Cc: srinivas.kandagatla, sboyd, linux-arm-msm, Vivek Gautam

This patch series adds couple of PHY drivers for Qualcomm chipsets.
a) qcom-qusb2 phy driver: that provides High Speed USB functionality.
b) qcom-qmp phy driver: that is a combo phy providing support for
   USB3, PCIe and UFS controllers.[1]

The patches are based on next branch of linux-phy tree.

These have been tested on Dragon board db820c hardware with
required set of patches on integration tree maintained by
Linaro landing team for Qualcomm [2]. The QUSB2 phy driver
is also based on qfprom patch [3]:
[PATCH v2] nvmem: qfprom: Allow single byte accesses for read/write

Changes since v1:
 - Moved device tree binding documentation to separate patches, as suggested
   by Rob.
 - Addressed review comment from Rob, regarding qfprom accesses by qusb2 phy driver,
   given by Rob.
 - Addressed review comments from Kishon.
 - Addressed review comments from Srinivas for QMP phy driver.
 - Addressed kbuild warning.
 - Please see patches 2 & 4 in the series for details on changes in v2.

[1] Currently the qcom-qmp phy driver supports only USB3 and PCIe
controllers. Later, we plan to add the UFS phy support as well to this.
[2] https://git.linaro.org/?p=landing-teams/working/qualcomm/kernel.git;a=shortlog;h=refs/heads/integration-linux-qcomlt
[3] https://lkml.org/lkml/2016/11/17/21

Vivek Gautam (4):
  dt-bindings: phy: Add support for QUSB2 phy
  phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  dt-bindings: phy: Add support for QMP phy
  phy: qcom-qmp: new qmp phy driver for qcom-chipsets

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       |   74 ++
 .../devicetree/bindings/phy/qcom-qusb2-phy.txt     |   55 +
 drivers/phy/Kconfig                                |   20 +
 drivers/phy/Makefile                               |    2 +
 drivers/phy/phy-qcom-qmp.c                         | 1141 ++++++++++++++++++++
 drivers/phy/phy-qcom-qusb2.c                       |  549 ++++++++++
 6 files changed, 1841 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
 create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
 create mode 100644 drivers/phy/phy-qcom-qmp.c
 create mode 100644 drivers/phy/phy-qcom-qusb2.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
  2016-11-22 12:02 [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
@ 2016-11-22 12:02 ` Vivek Gautam
  2016-11-28 14:19   ` Rob Herring
  2016-11-28 22:49   ` Stephen Boyd
  2016-11-22 12:02 ` [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-11-22 12:02 UTC (permalink / raw)
  To: kishon, robh+dt, mark.rutland, devicetree, linux-kernel
  Cc: srinivas.kandagatla, sboyd, linux-arm-msm, Vivek Gautam

Qualcomm chipsets have QUSB2 phy controller that provides
HighSpeed functionality for DWC3 controller.
Adding dt binding information for the same.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---

Changes since v1:
 - New patch, forked out of the original driver patch:
   "phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips"
 - Updated dt bindings to remove 'hstx-trim-bit-offset' and
   'hstx-trim-bit-len' bindings.

 .../devicetree/bindings/phy/qcom-qusb2-phy.txt     | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
new file mode 100644
index 0000000..38c8b30
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
@@ -0,0 +1,55 @@
+Qualcomm QUSB2 phy controller
+=============================
+
+QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
+
+Required properties:
+ - compatible: compatible list, contains "qcom,msm8996-qusb2-phy".
+ - reg: offset and length of the PHY register set.
+ - #phy-cells: must be 0.
+
+ - clocks: a list of phandles and clock-specifier pairs,
+	   one for each entry in clock-names.
+ - clock-names: must be "cfg_ahb" for phy config clock,
+			"ref_clk" for 19.2 MHz ref clk,
+			"ref_clk_src" reference clock source.
+			"iface" for phy interface clock (Optional).
+
+ - vdd-phy-supply: Phandle to a regulator supply to PHY core block.
+ - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
+ - vdda-phy-dpdm: Phandle to 3.1V regulator supply to Dp/Dm port signals.
+
+ - resets: a list of phandles and reset controller specifier pairs,
+	   one for each entry in reset-names.
+ - reset-names: must be "phy" for reset of phy block.
+
+Optional properties:
+ - nvmem-cells: a list of phandles to nvmem cells that contain fused
+		tuning parameters for qusb2 phy, one for each entry
+		in nvmem-cell-names.
+ - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
+		     HS Tx trim value.
+
+ - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
+
+Example:
+	hsphy: qusb2phy@7411000 {
+		compatible = "qcom,msm8996-qusb2-phy";
+		reg = <0x07411000 0x180>;
+		#phy-cells = <0>;
+
+		clocks = <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
+			<&gcc GCC_RX1_USB2_CLKREF_CLK>,
+			<&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>;
+		clock-names = "cfg_ahb_clk", "ref_clk", "ref_clk_src";
+
+		vdd-phy-supply = <&pm8994_s2>;
+		vdda-pll-supply = <&pm8994_l12>;
+		vdda-phy-dpdm-supply = <&pm8994_l24>;
+
+		resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
+		reset-names = "phy";
+
+		nvmem-cells = <&qusb2p_hstx_trim>;
+		nvmem-cell-names = "tune2_hstx_trim_efuse";
+        };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  2016-11-22 12:02 [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
  2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
@ 2016-11-22 12:02 ` Vivek Gautam
  2016-11-28 23:14   ` Stephen Boyd
  2016-11-22 12:02 ` [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
  2016-11-22 12:02 ` [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam
  3 siblings, 1 reply; 21+ messages in thread
From: Vivek Gautam @ 2016-11-22 12:02 UTC (permalink / raw)
  To: kishon, robh+dt, mark.rutland, devicetree, linux-kernel
  Cc: srinivas.kandagatla, sboyd, linux-arm-msm, Vivek Gautam

PHY transceiver driver for QUSB2 phy controller that provides
HighSpeed functionality for DWC3 controller present on
Qualcomm chipsets.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---

Changes since v1:
 - removed reference to clk_enabled/pwr_enabled.
 - moved clock and regulator enable code to phy_power_on/off() callbacks.
 - fixed return on EPROBE_DEFER in qusb2_phy_probe().
 - fixed phy create and phy register ordering.
 - removed references to non-lkml links from commit message.
 - took care of other minor nits.
 - Fixed coccinelle warnings -
   'PTR_ERR applied after initialization to constant'
 - Addressed review comment, regarding qfprom access for tune2 param value.
   This driver is now based on qfprom patch[1] that allows byte access now.

Comments not addressed in this version:
 -- Have not addressed Kishon's comment to move phy init table stuff
    to generic phy calibration bindings.

[1] https://lkml.org/lkml/2016/11/17/21

 drivers/phy/Kconfig          |  11 +
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-qcom-qusb2.c | 549 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 561 insertions(+)
 create mode 100644 drivers/phy/phy-qcom-qusb2.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index e8eb7f2..f1dcec1 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -430,6 +430,17 @@ config PHY_STIH407_USB
 	  Enable this support to enable the picoPHY device used by USB2
 	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
 
+config PHY_QCOM_QUSB2
+	tristate "Qualcomm QUSB2 PHY Driver"
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	select GENERIC_PHY
+	select RESET_CONTROLLER
+	help
+	  Enable this to support the HighSpeed QUSB2 PHY transceiver for USB
+	  controllers on Qualcomm chips. This driver supports the high-speed
+	  PHY which is usually paired with either the ChipIdea or Synopsys DWC3
+	  USB IPs on MSM SOCs.
+
 config PHY_QCOM_UFS
 	tristate "Qualcomm UFS PHY driver"
 	depends on OF && ARCH_QCOM
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 65eb2f4..dad1682 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
 obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
 obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
+obj-$(CONFIG_PHY_QCOM_QUSB2) 	+= phy-qcom-qusb2.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-20nm.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-14nm.o
diff --git a/drivers/phy/phy-qcom-qusb2.c b/drivers/phy/phy-qcom-qusb2.c
new file mode 100644
index 0000000..d3f9657
--- /dev/null
+++ b/drivers/phy/phy-qcom-qusb2.c
@@ -0,0 +1,549 @@
+/*
+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define QUSB2PHY_PLL_TEST		0x04
+#define CLK_REF_SEL			BIT(7)
+
+#define QUSB2PHY_PLL_TUNE		0x08
+#define QUSB2PHY_PLL_USER_CTL1		0x0c
+#define QUSB2PHY_PLL_USER_CTL2		0x10
+#define QUSB2PHY_PLL_AUTOPGM_CTL1	0x1c
+#define QUSB2PHY_PLL_PWR_CTRL		0x18
+
+#define QUSB2PHY_PLL_STATUS		0x38
+#define PLL_LOCKED			BIT(5)
+
+#define QUSB2PHY_PORT_TUNE1             0x80
+#define QUSB2PHY_PORT_TUNE2             0x84
+#define QUSB2PHY_PORT_TUNE3             0x88
+#define QUSB2PHY_PORT_TUNE4             0x8C
+#define QUSB2PHY_PORT_TUNE5		0x90
+#define QUSB2PHY_PORT_TEST2		0x9c
+
+#define QUSB2PHY_PORT_POWERDOWN		0xB4
+#define CLAMP_N_EN			BIT(5)
+#define FREEZIO_N			BIT(1)
+#define POWER_DOWN			BIT(0)
+
+#define QUSB2PHY_REFCLK_ENABLE		BIT(0)
+
+#define PHY_CLK_SCHEME_SEL		BIT(0)
+
+struct qusb2_phy_init_tbl {
+	unsigned int reg_offset;
+	unsigned int cfg_val;
+};
+#define QCOM_QUSB2_PHY_INIT_CFG(reg, val) \
+	{				\
+		.reg_offset = reg,	\
+		.cfg_val = val,		\
+	}
+
+static struct qusb2_phy_init_tbl msm8996_phy_init_tbl[] = {
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xF8),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xB3),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xC0),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9F),
+	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
+};
+
+struct qusb2_phy_init_cfg {
+	struct qusb2_phy_init_tbl *phy_init_tbl;
+	int phy_init_tbl_sz;
+	/* offset to PHY_CLK_SCHEME register in TCSR map. */
+	unsigned int clk_scheme_offset;
+};
+
+const struct qusb2_phy_init_cfg msm8996_phy_init_cfg = {
+	.phy_init_tbl = msm8996_phy_init_tbl,
+	.phy_init_tbl_sz = ARRAY_SIZE(msm8996_phy_init_tbl),
+};
+
+/**
+ * struct qusb2_phy: Structure holding qusb2 phy attributes.
+ *
+ * @phy: pointer to generic phy.
+ * @base: pointer to iomapped memory space for qubs2 phy.
+ *
+ * @cfg_ahb_clk: pointer to AHB2PHY interface clock.
+ * @ref_clk: pointer to reference clock.
+ * @ref_clk_src: pointer to source to reference clock.
+ * @iface_src: pointer to phy interface clock.
+ *
+ * @phy_reset: Pointer to phy reset control
+ *
+ * @vdda_phy: vdd supply to the phy core block.
+ * @vdda_pll: 1.8V vdd supply to ref_clk block.
+ * @vdda_phy_dpdm: 3.1V vdd supply to Dp/Dm port signals.
+ * @tcsr: pointer to TCSR syscon register map.
+ *
+ * @cfg: phy initialization config data
+ * @has_se_clk_scheme: indicate if PHY has Single-ended ref clock scheme
+ */
+struct qusb2_phy {
+	struct phy *phy;
+	void __iomem *base;
+
+	struct clk *cfg_ahb_clk;
+	struct clk *ref_clk;
+	struct clk *ref_clk_src;
+	struct clk *iface_clk;
+
+	struct reset_control *phy_reset;
+
+	struct regulator *vdd_phy;
+	struct regulator *vdda_pll;
+	struct regulator *vdda_phy_dpdm;
+
+	struct regmap *tcsr;
+
+	const struct qusb2_phy_init_cfg *cfg;
+	bool has_se_clk_scheme;
+};
+
+static inline void qusb2_setbits(void __iomem *reg, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl_relaxed(reg);
+	reg_val |= val;
+	writel_relaxed(reg_val, reg);
+
+	/* Ensure above write is completed */
+	mb();
+}
+
+static inline void qusb2_clrbits(void __iomem *reg, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl_relaxed(reg);
+	reg_val &= ~val;
+	writel_relaxed(reg_val, reg);
+
+	/* Ensure above write is completed */
+	mb();
+}
+
+static void qcom_qusb2_phy_configure(void __iomem *base,
+				struct qusb2_phy_init_tbl init_tbl[],
+				int init_tbl_sz)
+{
+	int i;
+
+	for (i = 0; i < init_tbl_sz; i++) {
+		writel_relaxed(init_tbl[i].cfg_val,
+				base + init_tbl[i].reg_offset);
+	}
+
+	/* flush buffered writes */
+	mb();
+}
+
+static void qusb2_phy_enable_clocks(struct qusb2_phy *qphy, bool on)
+{
+	if (on) {
+		clk_prepare_enable(qphy->iface_clk);
+		clk_prepare_enable(qphy->ref_clk_src);
+	} else {
+		clk_disable_unprepare(qphy->ref_clk_src);
+		clk_disable_unprepare(qphy->iface_clk);
+	}
+
+	dev_vdbg(&qphy->phy->dev, "%s(): clocks enabled\n", __func__);
+}
+
+static int qusb2_phy_enable_power(struct qusb2_phy *qphy, bool on)
+{
+	int ret;
+	struct device *dev = &qphy->phy->dev;
+
+	if (!on)
+		goto disable_vdda_phy_dpdm;
+
+	ret = regulator_enable(qphy->vdd_phy);
+	if (ret) {
+		dev_err(dev, "Unable to enable vdd-phy:%d\n", ret);
+		goto err_vdd_phy;
+	}
+
+	ret = regulator_enable(qphy->vdda_pll);
+	if (ret) {
+		dev_err(dev, "Unable to enable vdda-pll:%d\n", ret);
+		goto disable_vdd_phy;
+	}
+
+	ret = regulator_enable(qphy->vdda_phy_dpdm);
+	if (ret) {
+		dev_err(dev, "Unable to enable vdda-phy-dpdm:%d\n", ret);
+		goto disable_vdda_pll;
+	}
+
+	dev_vdbg(dev, "%s() regulators are turned on.\n", __func__);
+
+	return ret;
+
+disable_vdda_phy_dpdm:
+	regulator_disable(qphy->vdda_phy_dpdm);
+disable_vdda_pll:
+	regulator_disable(qphy->vdda_pll);
+disable_vdd_phy:
+	regulator_disable(qphy->vdd_phy);
+err_vdd_phy:
+	dev_vdbg(dev, "%s() regulators are turned off.\n", __func__);
+	return ret;
+}
+
+/*
+ * Fetches HS Tx tuning value from e-fuse and sets QUSB2PHY_PORT_TUNE2
+ * register.
+ * For any error case, skip setting the value and use the default value.
+ */
+static int qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
+{
+	struct device *dev = &qphy->phy->dev;
+	struct nvmem_cell *cell;
+	ssize_t len;
+	u8 *val;
+
+	/*
+	 * Read EFUSE register having TUNE2 parameter's high nibble.
+	 * If efuse register shows value as 0x0, or if we fail to find
+	 * a valid efuse register settings, then use default value
+	 * as 0xB for high nibble that we have already set while
+	 * configuring phy.
+	 */
+	cell = devm_nvmem_cell_get(dev, "tune2_hstx_trim_efuse");
+	if (IS_ERR(cell)) {
+		if (PTR_ERR(cell) == -EPROBE_DEFER)
+			return PTR_ERR(cell);
+		goto skip;
+	}
+
+	/*
+	 * we need to read only one byte here, since the required
+	 * parameter value fits in one nibble
+	 */
+	val = (u8 *)nvmem_cell_read(cell, &len);
+	if (!IS_ERR(val)) {
+		/* Fused TUNE2 value is the higher nibble only */
+		qusb2_setbits(qphy->base + QUSB2PHY_PORT_TUNE2,
+							val[0] << 0x4);
+	} else {
+		dev_dbg(dev, "failed reading hs-tx trim value: %ld\n",
+							PTR_ERR(val));
+	}
+
+skip:
+	return 0;
+}
+
+static int qusb2_phy_poweron(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+	int ret;
+
+	dev_vdbg(&phy->dev, "Powering-on QUSB2 phy\n");
+
+	ret = qusb2_phy_enable_power(qphy, true);
+	if (ret)
+		return ret;
+
+	qusb2_phy_enable_clocks(qphy, true);
+
+	return ret;
+}
+
+static int qusb2_phy_poweroff(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+
+	qusb2_phy_enable_clocks(qphy, false);
+	qusb2_phy_enable_power(qphy, false);
+
+	return 0;
+}
+
+static int qusb2_phy_init(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+	unsigned int reset_val;
+	unsigned int clk_scheme;
+	int ret;
+
+	dev_vdbg(&phy->dev, "Initializing QUSB2 phy\n");
+
+	/* enable ahb interface clock to program phy */
+	clk_prepare_enable(qphy->cfg_ahb_clk);
+
+	/* Perform phy reset */
+	ret = reset_control_assert(qphy->phy_reset);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to assert phy_reset\n");
+		return ret;
+	}
+	/* 100 us delay to keep PHY in reset mode */
+	usleep_range(100, 150);
+	ret = reset_control_deassert(qphy->phy_reset);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to de-assert phy_reset\n");
+		return ret;
+	}
+
+	/* Disable the PHY */
+	qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
+			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
+
+	/* save reset value to override based on clk scheme */
+	reset_val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST);
+
+	qcom_qusb2_phy_configure(qphy->base, qphy->cfg->phy_init_tbl,
+				qphy->cfg->phy_init_tbl_sz);
+
+	/* Check for efuse value for tuning the PHY */
+	ret = qusb2_phy_set_tune2_param(qphy);
+	if (ret)
+		return ret;
+
+	/* Enable the PHY */
+	qusb2_clrbits(qphy->base + QUSB2PHY_PORT_POWERDOWN, POWER_DOWN);
+
+	/* Require to get phy pll lock successfully */
+	usleep_range(150, 160);
+
+	/* Default is Single-ended clock on msm8996 */
+	qphy->has_se_clk_scheme = true;
+	/*
+	 * read TCSR_PHY_CLK_SCHEME register to check if Single-ended
+	 * clock scheme is selected. If yes, then disable differential
+	 * ref_clk and use single-ended clock, otherwise use differential
+	 * ref_clk only.
+	 */
+	if (qphy->tcsr) {
+		ret = regmap_read(qphy->tcsr, qphy->cfg->clk_scheme_offset,
+							&clk_scheme);
+		/* is it a differential clock scheme ? */
+		if (!(clk_scheme & PHY_CLK_SCHEME_SEL)) {
+			dev_vdbg(&phy->dev, "%s: select differential clk src\n",
+								__func__);
+			qphy->has_se_clk_scheme = false;
+		} else {
+			dev_vdbg(&phy->dev, "%s: select single-ended clk src\n",
+								__func__);
+		}
+	}
+
+	if (!qphy->has_se_clk_scheme) {
+		reset_val &= ~CLK_REF_SEL;
+		clk_prepare_enable(qphy->ref_clk);
+	} else {
+		reset_val |= CLK_REF_SEL;
+	}
+
+	writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
+
+	/* Make sure that above write is completed to get PLL source clock */
+	wmb();
+
+	/* Required to get PHY PLL lock successfully */
+	usleep_range(100, 110);
+
+	if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
+					PLL_LOCKED)) {
+		dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
+			readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int qusb2_phy_exit(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+
+	/* Disable the PHY */
+	qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
+			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
+
+	if (!qphy->has_se_clk_scheme)
+		clk_disable_unprepare(qphy->ref_clk);
+
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+
+	return 0;
+}
+
+static const struct phy_ops qusb2_phy_gen_ops = {
+	.init		= qusb2_phy_init,
+	.exit		= qusb2_phy_exit,
+	.power_on	= qusb2_phy_poweron,
+	.power_off	= qusb2_phy_poweroff,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id qusb2_phy_of_match_table[] = {
+	{
+		.compatible	= "qcom,msm8996-qusb2-phy",
+		.data		= &msm8996_phy_init_cfg,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qusb2_phy_of_match_table);
+
+static int qusb2_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qusb2_phy *qphy;
+	struct phy_provider *phy_provider;
+	struct phy *generic_phy;
+	const struct of_device_id *match;
+	struct resource *res;
+	int ret;
+
+	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
+	if (!qphy)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	qphy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(qphy->base))
+		return PTR_ERR(qphy->base);
+
+	qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb_clk");
+	if (IS_ERR(qphy->cfg_ahb_clk)) {
+		ret = PTR_ERR(qphy->cfg_ahb_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get cfg_ahb_clk\n");
+		return ret;
+	}
+
+	qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
+	if (IS_ERR(qphy->ref_clk_src)) {
+		ret = PTR_ERR(qphy->ref_clk_src);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "clk get failed for ref_clk_src\n");
+		return ret;
+	}
+
+	qphy->ref_clk = devm_clk_get(dev, "ref_clk");
+	if (IS_ERR(qphy->ref_clk)) {
+		ret = PTR_ERR(qphy->ref_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "clk get failed for ref_clk\n");
+		return ret;
+	} else {
+		clk_set_rate(qphy->ref_clk, 19200000);
+	}
+
+	qphy->iface_clk = devm_clk_get(dev, "iface_clk");
+	if (IS_ERR(qphy->iface_clk)) {
+		ret = PTR_ERR(qphy->iface_clk);
+		if (ret != -EPROBE_DEFER) {
+			qphy->iface_clk = NULL;
+			dev_dbg(dev, "clk get failed for iface_clk\n");
+		} else {
+			return ret;
+		}
+	}
+
+	qphy->phy_reset = devm_reset_control_get(&pdev->dev, "phy");
+	if (IS_ERR(qphy->phy_reset)) {
+		dev_err(dev, "failed to get phy core reset\n");
+		return PTR_ERR(qphy->phy_reset);
+	}
+
+	qphy->vdd_phy = devm_regulator_get(dev, "vdd-phy");
+	if (IS_ERR(qphy->vdd_phy)) {
+		dev_err(dev, "unable to get vdd-phy supply\n");
+		return PTR_ERR(qphy->vdd_phy);
+	}
+
+	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
+	if (IS_ERR(qphy->vdda_pll)) {
+		dev_err(dev, "unable to get vdda-pll supply\n");
+		return PTR_ERR(qphy->vdda_pll);
+	}
+
+	qphy->vdda_phy_dpdm = devm_regulator_get(dev, "vdda-phy-dpdm");
+	if (IS_ERR(qphy->vdda_phy_dpdm)) {
+		dev_err(dev, "unable to get vdda-phy-dpdm supply\n");
+		return PTR_ERR(qphy->vdda_phy_dpdm);
+	}
+
+	/* Get the specific init parameters of QMP phy */
+	match = of_match_node(qusb2_phy_of_match_table, dev->of_node);
+	qphy->cfg = match->data;
+
+	qphy->tcsr = syscon_regmap_lookup_by_phandle(dev->of_node,
+							"qcom,tcsr-syscon");
+	if (IS_ERR(qphy->tcsr)) {
+		dev_dbg(dev, "Failed to lookup TCSR regmap\n");
+		qphy->tcsr = NULL;
+	}
+
+	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
+	if (IS_ERR(generic_phy)) {
+		ret = PTR_ERR(generic_phy);
+		dev_err(dev, "%s: failed to create phy %d\n", __func__, ret);
+		return ret;
+	}
+	qphy->phy = generic_phy;
+
+	dev_set_drvdata(dev, qphy);
+	phy_set_drvdata(generic_phy, qphy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		dev_err(dev, "%s: failed to register phy %d\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver qusb2_phy_driver = {
+	.probe		= qusb2_phy_probe,
+	.driver = {
+		.name	= "qcom-qusb2-phy",
+		.of_match_table = of_match_ptr(qusb2_phy_of_match_table),
+	},
+};
+
+module_platform_driver(qusb2_phy_driver);
+
+MODULE_AUTHOR("Vivek Gautam <vivek.gautam@codeaurora.org>");
+MODULE_DESCRIPTION("Qualcomm QUSB2 PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-22 12:02 [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
  2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
  2016-11-22 12:02 ` [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam
@ 2016-11-22 12:02 ` Vivek Gautam
  2016-11-28 22:55   ` Stephen Boyd
  2016-11-28 23:19   ` Stephen Boyd
  2016-11-22 12:02 ` [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam
  3 siblings, 2 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-11-22 12:02 UTC (permalink / raw)
  To: kishon, robh+dt, mark.rutland, devicetree, linux-kernel
  Cc: srinivas.kandagatla, sboyd, linux-arm-msm, Vivek Gautam

Qualcomm chipsets have QMP phy controller that provides
support to a number of controller, viz. PCIe, UFS, and USB.
Adding dt binding information for the same.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes since v1:
 - New patch, forked out of the original driver patch:
   "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
 - updated bindings to include mem resource as a list of
   offset - length pair for serdes block and for each lane.
 - added a new binding for 'lane-offsets' that contains offsets
   to tx, rx and pcs blocks from each lane base address.

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 74 ++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
new file mode 100644
index 0000000..ffb173b
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -0,0 +1,74 @@
+Qualcomm QMP PHY
+----------------
+
+QMP phy controller supports physical layer functionality for a number of
+controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
+
+Required properties:
+ - compatible: compatible list, contains:
+	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
+	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
+ - reg: list of offset and length pair of the PHY register sets.
+	at index 0: offset and length of register set for PHY common
+		    serdes block.
+	from index 1 - N: offset and length of register set for each lane,
+			  for N number of phy lanes (ports).
+ - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
+ - #phy-cells: must be 1
+    - Cell after phy phandle should be the port (lane) number.
+ - clocks: a list of phandles and clock-specifier pairs,
+	   one for each entry in clock-names.
+ - clock-names: must be "cfg_ahb" for phy config clock,
+			"aux" for phy aux clock,
+			"ref_clk" for 19.2 MHz ref clk,
+			"ref_clk_src" for reference clock source,
+			"pipe<port-number>" for pipe clock specific to
+			each port/lane (Optional).
+ - resets: a list of phandles and reset controller specifier pairs,
+	   one for each entry in reset-names.
+ - reset-names: must be "phy" for reset of phy block,
+			"common" for phy common block reset,
+			"cfg" for phy's ahb cfg block reset (Optional).
+			"port<port-number>" for reset specific to
+			each port/lane (Optional).
+ - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
+ - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
+
+Optional properties:
+ - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
+			pll block.
+
+Example:
+	pcie_phy: pciephy@34000 {
+		compatible = "qcom,msm8996-qmp-pcie-phy";
+		reg = <0x034000 0x48f>,
+			<0x035000 0x5bf>,
+			<0x036000 0x5bf>,
+			<0x037000 0x5bf>;
+				/* tx, rx, pcs */
+		lane-offsets = <0x0 0x200 0x400>;
+		#phy-cells = <1>;
+
+		clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
+			<&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
+			<&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>,
+			<&gcc GCC_PCIE_CLKREF_CLK>,
+			<&gcc GCC_PCIE_0_PIPE_CLK>,
+			<&gcc GCC_PCIE_1_PIPE_CLK>,
+			<&gcc GCC_PCIE_2_PIPE_CLK>;
+		clock-names = "aux", "cfg_ahb",
+				"ref_clk_src", "ref_clk",
+				"pipe0", "pipe1", "pipe2";
+
+		vdda-phy-supply = <&pm8994_l28>;
+		vdda-pll-supply = <&pm8994_l12>;
+
+		resets = <&gcc GCC_PCIE_PHY_BCR>,
+			<&gcc GCC_PCIE_PHY_COM_BCR>,
+			<&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>,
+			<&gcc GCC_PCIE_0_PHY_BCR>,
+			<&gcc GCC_PCIE_1_PHY_BCR>,
+			<&gcc GCC_PCIE_2_PHY_BCR>;
+		reset-names = "phy", "common", "cfg",
+				"lane0", "lane1", "lane2";
+	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
  2016-11-22 12:02 [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
                   ` (2 preceding siblings ...)
  2016-11-22 12:02 ` [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
@ 2016-11-22 12:02 ` Vivek Gautam
  2016-11-29  0:35   ` Stephen Boyd
  3 siblings, 1 reply; 21+ messages in thread
From: Vivek Gautam @ 2016-11-22 12:02 UTC (permalink / raw)
  To: kishon, robh+dt, mark.rutland, devicetree, linux-kernel
  Cc: srinivas.kandagatla, sboyd, linux-arm-msm, Vivek Gautam

Qualcomm SOCs have QMP phy controller that provides support
to a number of controller, viz. PCIe, UFS, and USB.
Add a new driver, based on generic phy framework, for this
phy controller.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---

Changes since v1:
 - Fixed missing mutex_unlock() calls in error cases, reported by
   Julia Lawall.
 - Selecting CONFIG_RESET_CONTROLLER when this driver is enabled.
 - Added a boolean property to check if the phy has individual lane
   reset available.
 - Took care or EPROBE_DEFER, dev_vdbg() and other minor nits.
 - Removed references to non-lkml links from commit message.
 - Moved to use separate iomem resources for each lanes.
   Tx, Rx and PCS offsets per lane can now come from dt bindings.

Comments not addressed in this version:
 -- Have not addressed Kishon's comment to move phy init table stuff
    to generic phy calibration bindings.
    The qmp phy driver has 100 odd register writes (that do phy calibration
    at much finer level). Incorporating all such into generic phy calibration
    bindings does not seem possible.
    One way could have been to add dt-binding for the init table as
    'init-sequence'. But that is something strongly disliked by Rob and other
    dt maintainers.
 -- Have not addressed Kishon's comments to use phandle label based phy
    association to the consumer in order to get rid of index based PHY
    retrieval. This would require adding child nodes, something
    that we want to avoid.

 drivers/phy/Kconfig        |    9 +
 drivers/phy/Makefile       |    1 +
 drivers/phy/phy-qcom-qmp.c | 1141 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1151 insertions(+)
 create mode 100644 drivers/phy/phy-qcom-qmp.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index f1dcec1..8970d9e 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -430,6 +430,15 @@ config PHY_STIH407_USB
 	  Enable this support to enable the picoPHY device used by USB2
 	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
 
+config PHY_QCOM_QMP
+	tristate "Qualcomm QMP PHY Driver"
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	select GENERIC_PHY
+	select RESET_CONTROLLER
+	help
+	  Enable this to support the QMP PHY transceiver that is used
+	  with controllers such as PCIe, UFS, and USB on Qualcomm chips.
+
 config PHY_QCOM_QUSB2
 	tristate "Qualcomm QUSB2 PHY Driver"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index dad1682..dbe7731 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
 obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
 obj-$(CONFIG_PHY_QCOM_QUSB2) 	+= phy-qcom-qusb2.o
+obj-$(CONFIG_PHY_QCOM_QMP) 	+= phy-qcom-qmp.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-20nm.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-14nm.o
diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
new file mode 100644
index 0000000..f85289e
--- /dev/null
+++ b/drivers/phy/phy-qcom-qmp.c
@@ -0,0 +1,1141 @@
+/*
+ * Copyright (c) 2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/phy/phy.h>
+
+/* QMP PHY QSERDES COM registers */
+#define QSERDES_COM_BG_TIMER				0x00c
+#define QSERDES_COM_SSC_EN_CENTER			0x010
+#define QSERDES_COM_SSC_ADJ_PER1			0x014
+#define QSERDES_COM_SSC_ADJ_PER2			0x018
+#define QSERDES_COM_SSC_PER1				0x01c
+#define QSERDES_COM_SSC_PER2				0x020
+#define QSERDES_COM_SSC_STEP_SIZE1			0x024
+#define QSERDES_COM_SSC_STEP_SIZE2			0x028
+#define QSERDES_COM_BIAS_EN_CLKBUFLR_EN			0x034
+#define QSERDES_COM_CLK_ENABLE1				0x038
+#define QSERDES_COM_SYS_CLK_CTRL			0x03c
+#define QSERDES_COM_SYSCLK_BUF_ENABLE			0x040
+#define QSERDES_COM_PLL_IVCO				0x048
+#define QSERDES_COM_LOCK_CMP1_MODE0			0x04c
+#define QSERDES_COM_LOCK_CMP2_MODE0			0x050
+#define QSERDES_COM_LOCK_CMP3_MODE0			0x054
+#define QSERDES_COM_LOCK_CMP1_MODE1			0x058
+#define QSERDES_COM_LOCK_CMP2_MODE1			0x05c
+#define QSERDES_COM_LOCK_CMP3_MODE1			0x060
+#define QSERDES_COM_BG_TRIM				0x070
+#define QSERDES_COM_CLK_EP_DIV				0x074
+#define QSERDES_COM_CP_CTRL_MODE0			0x078
+#define QSERDES_COM_CP_CTRL_MODE1			0x07c
+#define QSERDES_COM_PLL_RCTRL_MODE0			0x084
+#define QSERDES_COM_PLL_RCTRL_MODE1			0x088
+#define QSERDES_COM_PLL_CCTRL_MODE0			0x090
+#define QSERDES_COM_PLL_CCTRL_MODE1			0x094
+#define QSERDES_COM_SYSCLK_EN_SEL			0x0ac
+#define QSERDES_COM_RESETSM_CNTRL			0x0b4
+#define QSERDES_COM_RESTRIM_CTRL			0x0bc
+#define QSERDES_COM_RESCODE_DIV_NUM			0x0c4
+#define QSERDES_COM_LOCK_CMP_EN				0x0c8
+#define QSERDES_COM_LOCK_CMP_CFG			0x0cc
+#define QSERDES_COM_DEC_START_MODE0			0x0d0
+#define QSERDES_COM_DEC_START_MODE1			0x0d4
+#define QSERDES_COM_DIV_FRAC_START1_MODE0		0x0dc
+#define QSERDES_COM_DIV_FRAC_START2_MODE0		0x0e0
+#define QSERDES_COM_DIV_FRAC_START3_MODE0		0x0e4
+#define QSERDES_COM_DIV_FRAC_START1_MODE1		0x0e8
+#define QSERDES_COM_DIV_FRAC_START2_MODE1		0x0ec
+#define QSERDES_COM_DIV_FRAC_START3_MODE1		0x0f0
+#define QSERDES_COM_INTEGLOOP_GAIN0_MODE0		0x108
+#define QSERDES_COM_INTEGLOOP_GAIN1_MODE0		0x10c
+#define QSERDES_COM_INTEGLOOP_GAIN0_MODE1		0x110
+#define QSERDES_COM_INTEGLOOP_GAIN1_MODE1		0x114
+#define QSERDES_COM_VCO_TUNE_CTRL			0x124
+#define QSERDES_COM_VCO_TUNE_MAP			0x128
+#define QSERDES_COM_VCO_TUNE1_MODE0			0x12c
+#define QSERDES_COM_VCO_TUNE2_MODE0			0x130
+#define QSERDES_COM_VCO_TUNE1_MODE1			0x134
+#define QSERDES_COM_VCO_TUNE2_MODE1			0x138
+#define QSERDES_COM_VCO_TUNE_TIMER1			0x144
+#define QSERDES_COM_VCO_TUNE_TIMER2			0x148
+#define QSERDES_COM_BG_CTRL				0x170
+#define QSERDES_COM_CLK_SELECT				0x174
+#define QSERDES_COM_HSCLK_SEL				0x178
+#define QSERDES_COM_CORECLK_DIV				0x184
+#define QSERDES_COM_CORE_CLK_EN				0x18c
+#define QSERDES_COM_C_READY_STATUS			0x190
+#define QSERDES_COM_CMN_CONFIG				0x194
+#define QSERDES_COM_SVS_MODE_CLK_SEL			0x19c
+#define QSERDES_COM_DEBUG_BUS0				0x1a0
+#define QSERDES_COM_DEBUG_BUS1				0x1a4
+#define QSERDES_COM_DEBUG_BUS2				0x1a8
+#define QSERDES_COM_DEBUG_BUS3				0x1ac
+#define QSERDES_COM_DEBUG_BUS_SEL			0x1b0
+#define QSERDES_COM_CORECLK_DIV_MODE1			0x1bc
+
+/* QMP PHY TX registers */
+#define QSERDES_TX_RES_CODE_LANE_OFFSET			0x054
+#define QSERDES_TX_DEBUG_BUS_SEL			0x064
+#define QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN	0x068
+#define QSERDES_TX_LANE_MODE				0x094
+#define QSERDES_TX_RCV_DETECT_LVL_2			0x0ac
+
+/* QMP PHY RX registers */
+#define QSERDES_RX_UCDR_SO_GAIN_HALF			0x010
+#define QSERDES_RX_UCDR_SO_GAIN				0x01c
+#define QSERDES_RX_UCDR_FASTLOCK_FO_GAIN		0x040
+#define QSERDES_RX_UCDR_SO_SATURATION_AND_ENABLE	0x048
+#define QSERDES_RX_RX_TERM_BW				0x090
+#define QSERDES_RX_RX_EQ_GAIN1_LSB			0x0c4
+#define QSERDES_RX_RX_EQ_GAIN1_MSB			0x0c8
+#define QSERDES_RX_RX_EQ_GAIN2_LSB			0x0cc
+#define QSERDES_RX_RX_EQ_GAIN2_MSB			0x0d0
+#define QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2		0x0d8
+#define QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3		0x0dc
+#define QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4		0x0e0
+#define QSERDES_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1		0x108
+#define QSERDES_RX_RX_OFFSET_ADAPTOR_CNTRL2		0x10c
+#define QSERDES_RX_SIGDET_ENABLES			0x110
+#define QSERDES_RX_SIGDET_CNTRL				0x114
+#define QSERDES_RX_SIGDET_LVL				0x118
+#define QSERDES_RX_SIGDET_DEGLITCH_CNTRL		0x11c
+#define QSERDES_RX_RX_BAND				0x120
+#define QSERDES_RX_RX_INTERFACE_MODE			0x12c
+
+/* QMP PHY PCS registers */
+#define QPHY_SW_RESET					0x00
+#define QPHY_POWER_DOWN_CONTROL				0x04
+#define QPHY_START_CTRL					0x08
+#define QPHY_TXDEEMPH_M6DB_V0				0x24
+#define QPHY_TXDEEMPH_M3P5DB_V0				0x28
+#define QPHY_ENDPOINT_REFCLK_DRIVE			0x54
+#define QPHY_RX_IDLE_DTCT_CNTRL				0x58
+#define QPHY_POWER_STATE_CONFIG1			0x60
+#define QPHY_POWER_STATE_CONFIG2			0x64
+#define QPHY_POWER_STATE_CONFIG4			0x6c
+#define QPHY_LOCK_DETECT_CONFIG1			0x80
+#define QPHY_LOCK_DETECT_CONFIG2			0x84
+#define QPHY_LOCK_DETECT_CONFIG3			0x88
+#define QPHY_PWRUP_RESET_DLY_TIME_AUXCLK		0xa0
+#define QPHY_LP_WAKEUP_DLY_TIME_AUXCLK			0xa4
+
+/* PHY_SW_RESET bit */
+#define PHY_SW_RESET				BIT(0)
+/* PHY_POWER_DOWN_CONTROL */
+#define PHY_SW_PWRDN				BIT(0)
+#define PHY_REFCLK_DRV_DSBL			BIT(1)
+/* PHY_START_CONTROL bits */
+#define PHY_SERDES_START			BIT(0)
+#define PHY_PCS_START				BIT(1)
+#define PHY_PLL_READY_GATE_EN			BIT(3)
+/* PHY_PCS_STATUS bit */
+#define MASK_PHYSTATUS				BIT(6)
+/* PCS_READY_STATUS bit */
+#define MASK_COM_PCS_READY			BIT(0)
+
+#define REFCLK_STABILIZATION_DELAY_US_MIN	1000
+#define REFCLK_STABILIZATION_DELAY_US_MAX	1005
+#define PHY_READY_TIMEOUT_COUNT			10
+#define POWER_DOWN_DELAY_US_MIN			10
+#define POWER_DOWN_DELAY_US_MAX			11
+
+#define MAX_PROP_NAME		32
+
+struct qmp_phy_init_tbl {
+	unsigned int reg_offset;
+	unsigned int cfg_val;
+	/*
+	 * register part of layout ?
+	 * if yes, then reg_offset gives index in the reg-layout
+	 */
+	int in_layout;
+};
+#define QCOM_QMP_PHY_INIT_CFG(reg, val) \
+	{				\
+		.reg_offset = reg,	\
+		.cfg_val = val,		\
+	}
+#define QCOM_QMP_PHY_INIT_CFG_L(reg, val) \
+	{				  \
+		.reg_offset = reg,	  \
+		.cfg_val = val,		  \
+		.in_layout = 1,		  \
+	}
+
+/* set of registers with offsets different per-PHY */
+enum qphy_reg_layout {
+	/* Common block control registers */
+	QPHY_COM_SW_RESET,
+	QPHY_COM_POWER_DOWN_CONTROL,
+	QPHY_COM_START_CONTROL,
+	QPHY_COM_PCS_READY_STATUS,
+	/* PCS registers */
+	QPHY_PLL_LOCK_CHK_DLY_TIME,
+	QPHY_FLL_CNTRL1,
+	QPHY_FLL_CNTRL2,
+	QPHY_FLL_CNT_VAL_L,
+	QPHY_FLL_CNT_VAL_H_TOL,
+	QPHY_FLL_MAN_CODE,
+	QPHY_PCS_READY_STATUS,
+};
+
+unsigned int pciephy_regs_layout[] = {
+	[QPHY_COM_SW_RESET]		= 0x400,
+	[QPHY_COM_POWER_DOWN_CONTROL]	= 0x404,
+	[QPHY_COM_START_CONTROL]	= 0x408,
+	[QPHY_COM_PCS_READY_STATUS]	= 0x448,
+	[QPHY_PLL_LOCK_CHK_DLY_TIME]	= 0xa8,
+	[QPHY_FLL_CNTRL1]		= 0xc4,
+	[QPHY_FLL_CNTRL2]		= 0xc8,
+	[QPHY_FLL_CNT_VAL_L]		= 0xcc,
+	[QPHY_FLL_CNT_VAL_H_TOL]	= 0xd0,
+	[QPHY_FLL_MAN_CODE]		= 0xd4,
+	[QPHY_PCS_READY_STATUS]		= 0x174,
+};
+
+unsigned int usb3phy_regs_layout[] = {
+	[QPHY_FLL_CNTRL1]		= 0xc0,
+	[QPHY_FLL_CNTRL2]		= 0xc4,
+	[QPHY_FLL_CNT_VAL_L]		= 0xc8,
+	[QPHY_FLL_CNT_VAL_H_TOL]	= 0xcc,
+	[QPHY_FLL_MAN_CODE]		= 0xd0,
+	[QPHY_PCS_READY_STATUS]		= 0x17c,
+};
+
+static struct qmp_phy_init_tbl pciephy_serdes_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x1c),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_ENABLE1, 0x10),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x33),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x06),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP_EN, 0x42),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_TIMER1, 0xff),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_TIMER2, 0x1f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SVS_MODE_CLK_SEL, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORE_CLK_EN, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORECLK_DIV, 0x0a),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TIMER, 0x09),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DEC_START_MODE0, 0x82),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START3_MODE0, 0x03),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START2_MODE0, 0x55),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START1_MODE0, 0x55),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE0, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP2_MODE0, 0x1a),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP1_MODE0, 0x0a),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x33),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYS_CLK_CTRL, 0x02),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_BUF_ENABLE, 0x1f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x04),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CP_CTRL_MODE0, 0x0b),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_RCTRL_MODE0, 0x16),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_CCTRL_MODE0, 0x28),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN1_MODE0, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN0_MODE0, 0x80),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_EN_CENTER, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER1, 0x31),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER2, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER1, 0x02),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER2, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE1, 0x2f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE2, 0x19),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_RESCODE_DIV_NUM, 0x15),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TRIM, 0x0f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_IVCO, 0x0f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_EP_DIV, 0x19),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_ENABLE1, 0x10),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_RESCODE_DIV_NUM, 0x40),
+};
+
+static struct qmp_phy_init_tbl pciephy_tx_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
+};
+
+static struct qmp_phy_init_tbl pciephy_rx_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_ENABLES, 0x1c),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4, 0xdb),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_BAND, 0x18),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN, 0x04),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN_HALF, 0x04),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x4b),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_DEGLITCH_CNTRL, 0x14),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x19),
+};
+
+static struct qmp_phy_init_tbl pciephy_pcs_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QPHY_RX_IDLE_DTCT_CNTRL, 0x4c),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_PWRUP_RESET_DLY_TIME_AUXCLK, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_LP_WAKEUP_DLY_TIME_AUXCLK, 0x01),
+
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_PLL_LOCK_CHK_DLY_TIME, 0x05),
+
+	QCOM_QMP_PHY_INIT_CFG(QPHY_ENDPOINT_REFCLK_DRIVE, 0x05),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_DOWN_CONTROL, 0x02),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG4, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG1, 0xa3),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_TXDEEMPH_M3P5DB_V0, 0x0e),
+};
+
+static struct qmp_phy_init_tbl usb3phy_serdes_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x14),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x08),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x06),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SVS_MODE_CLK_SEL, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TRIM, 0x0f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_IVCO, 0x0f),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYS_CLK_CTRL, 0x04),
+	/* PLL and Loop filter settings */
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DEC_START_MODE0, 0x82),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START1_MODE0, 0x55),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START2_MODE0, 0x55),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START3_MODE0, 0x03),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CP_CTRL_MODE0, 0x0b),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_RCTRL_MODE0, 0x16),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_CCTRL_MODE0, 0x28),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN0_MODE0, 0x80),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_CTRL, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP1_MODE0, 0x15),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP2_MODE0, 0x34),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE0, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORE_CLK_EN, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP_CFG, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TIMER, 0x0a),
+	/* SSC settings */
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_EN_CENTER, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER1, 0x31),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER2, 0x01),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER1, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER2, 0x00),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE1, 0xde),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE2, 0x07),
+};
+
+static struct qmp_phy_init_tbl usb3phy_tx_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_RCV_DETECT_LVL_2, 0x12),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
+};
+
+static struct qmp_phy_init_tbl usb3phy_rx_init_tbl[] = {
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN, 0x04),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x02),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x4c),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4, 0xbb),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x77),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_OFFSET_ADAPTOR_CNTRL2, 0x80),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x03),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x18),
+	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_DEGLITCH_CNTRL, 0x16),
+};
+
+static struct qmp_phy_init_tbl usb3phy_pcs_init_tbl[] = {
+	/* FLL settings */
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNTRL2, 0x03),
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNTRL1, 0x02),
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNT_VAL_L, 0x09),
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNT_VAL_H_TOL, 0x42),
+	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_MAN_CODE, 0x85),
+
+	/* Lock Det settings */
+	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG1, 0xd1),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG2, 0x1f),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG3, 0x47),
+	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG2, 0x08),
+};
+
+/**
+ * struct qmp_phy_init_cfg:- per-PHY init config.
+ */
+struct qmp_phy_init_cfg {
+	/* phy-type - PCIE/UFS/USB */
+	unsigned int type;
+	/* number of lanes provided by phy */
+	int nlanes;
+
+	/* Initialization sequence for PHY blocks - Serdes, tx, rx, pcs */
+	struct qmp_phy_init_tbl *phy_init_serdes_tbl;
+	int phy_init_serdes_tbl_sz;
+	struct qmp_phy_init_tbl *phy_init_tx_tbl;
+	int phy_init_tx_tbl_sz;
+	struct qmp_phy_init_tbl *phy_init_rx_tbl;
+	int phy_init_rx_tbl_sz;
+	struct qmp_phy_init_tbl *phy_init_pcs_tbl;
+	int phy_init_pcs_tbl_sz;
+
+	/* array of registers with different offsets */
+	unsigned int *regs;
+
+	unsigned int mask_start_ctrl;
+	unsigned int mask_pwr_dn_ctrl;
+	/* true, if PHY has a separate PHY_COM_CNTRL block */
+	bool has_phy_com_ctrl;
+	/* true, if PHY has a reset for individual lanes */
+	bool has_lane_rst;
+};
+
+/**
+ * struct qmp_phy_desc:- per-lane phy-descriptor.
+ *
+ * @phy: pointer to generic phy
+ * @tx: pointer to iomapped memory space for PHY's tx
+ * @rx: pointer to iomapped memory space for PHY's rx
+ * @pcs: pointer to iomapped memory space for PHY's pcs
+ * @pipe_clk: pointer to pipe lock
+ * @index: lane index
+ * @qphy: pointer to QMP phy to which this lane belongs
+ * @lane_rst: pointer to lane's reset controller
+ */
+struct qmp_phy_desc {
+	struct phy *phy;
+	void __iomem *tx;
+	void __iomem *rx;
+	void __iomem *pcs;
+	struct clk *pipe_clk;
+	unsigned int index;
+	struct qcom_qmp_phy *qphy;
+	struct reset_control *lane_rst;
+};
+
+/**
+ * struct qcom_qmp_phy:- structure holding QMP PHY attributes.
+ *
+ * @dev: pointer to device
+ * @serdes: pointer to iomapped memory space for phy's serdes
+ *
+ * @aux_clk: pointer to phy core clock
+ * @cfg_ahb_clk: pointer to AHB2PHY interface clock
+ * @ref_clk: pointer to reference clock
+ * @ref_clk_src: pointer to source to reference clock
+ *
+ * @vdda_phy: vdd supply to the phy core block
+ * @vdda_pll: 1.8V vdd supply to ref_clk block
+ * @vddp_ref_clk: vdd supply to specific ref_clk block (Optional)
+ *
+ * @phy_rst: Pointer to phy reset control
+ * @phycom_rst: Pointer to phy common reset control
+ * @phycfg_rst: Pointer to phy ahb cfg reset control (Optional)
+ *
+ * @cfg: pointer to init config for each phys
+ * @phys: array of pointer to per-lane phy descriptors
+ * @phy_mutex: mutex lock for PHY common block initialization
+ * @init_count: Phy common block initialization count
+ */
+struct qcom_qmp_phy {
+	struct device *dev;
+	void __iomem *serdes;
+
+	struct clk *aux_clk;
+	struct clk *cfg_ahb_clk;
+	struct clk *ref_clk;
+	struct clk *ref_clk_src;
+
+	struct regulator *vdda_phy;
+	struct regulator *vdda_pll;
+	struct regulator *vddp_ref_clk;
+
+	struct reset_control *phy_rst;
+	struct reset_control *phycom_rst;
+	struct reset_control *phycfg_rst;
+
+	const struct qmp_phy_init_cfg *cfg;
+	struct qmp_phy_desc **phys;
+
+	struct mutex phy_mutex;
+	int init_count;
+};
+
+static inline void qphy_setbits(void __iomem *reg, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl_relaxed(reg);
+	reg_val |= val;
+	writel_relaxed(reg_val, reg);
+}
+
+static inline void qphy_clrbits(void __iomem *reg, u32 val)
+{
+	u32 reg_val;
+
+	reg_val = readl_relaxed(reg);
+	reg_val &= ~val;
+	writel_relaxed(reg_val, reg);
+}
+
+const struct qmp_phy_init_cfg msm8996_pciephy_init_cfg = {
+	.type			= PHY_TYPE_PCIE,
+	.nlanes			= 3,
+
+	.phy_init_serdes_tbl	= pciephy_serdes_init_tbl,
+	.phy_init_serdes_tbl_sz	= ARRAY_SIZE(pciephy_serdes_init_tbl),
+	.phy_init_tx_tbl	= pciephy_tx_init_tbl,
+	.phy_init_tx_tbl_sz	= ARRAY_SIZE(pciephy_tx_init_tbl),
+	.phy_init_rx_tbl	= pciephy_rx_init_tbl,
+	.phy_init_rx_tbl_sz	= ARRAY_SIZE(pciephy_rx_init_tbl),
+	.phy_init_pcs_tbl	= pciephy_pcs_init_tbl,
+	.phy_init_pcs_tbl_sz	= ARRAY_SIZE(pciephy_pcs_init_tbl),
+	.regs			= pciephy_regs_layout,
+	.mask_start_ctrl	= (PHY_PCS_START | PHY_PLL_READY_GATE_EN),
+	.mask_pwr_dn_ctrl	= (PHY_SW_PWRDN | PHY_REFCLK_DRV_DSBL),
+
+	.has_phy_com_ctrl	= true,
+	.has_lane_rst		= true,
+};
+
+const struct qmp_phy_init_cfg msm8996_usb3phy_init_cfg = {
+	.type			= PHY_TYPE_USB3,
+	.nlanes			= 1,
+
+	.phy_init_serdes_tbl	= usb3phy_serdes_init_tbl,
+	.phy_init_serdes_tbl_sz	= ARRAY_SIZE(usb3phy_serdes_init_tbl),
+	.phy_init_tx_tbl	= usb3phy_tx_init_tbl,
+	.phy_init_tx_tbl_sz	= ARRAY_SIZE(usb3phy_tx_init_tbl),
+	.phy_init_rx_tbl	= usb3phy_rx_init_tbl,
+	.phy_init_rx_tbl_sz	= ARRAY_SIZE(usb3phy_rx_init_tbl),
+	.phy_init_pcs_tbl	= usb3phy_pcs_init_tbl,
+	.phy_init_pcs_tbl_sz	= ARRAY_SIZE(usb3phy_pcs_init_tbl),
+	.regs			= usb3phy_regs_layout,
+	.mask_start_ctrl	= (PHY_SERDES_START | PHY_PCS_START),
+	.mask_pwr_dn_ctrl	= PHY_SW_PWRDN,
+};
+
+static void qcom_qmp_phy_configure(void __iomem *base,
+				unsigned int *regs_layout,
+				struct qmp_phy_init_tbl init_tbl[],
+				int init_tbl_sz)
+{
+	int i;
+
+	for (i = 0; i < init_tbl_sz; i++) {
+		if (init_tbl[i].in_layout)
+			writel_relaxed(init_tbl[i].cfg_val,
+				base + regs_layout[init_tbl[i].reg_offset]);
+		else
+			writel_relaxed(init_tbl[i].cfg_val,
+				base + init_tbl[i].reg_offset);
+	}
+
+	/* flush buffered writes */
+	mb();
+}
+
+static int qcom_qmp_phy_poweron(struct phy *phy)
+{
+	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
+	struct qcom_qmp_phy *qphy = phydesc->qphy;
+	int ret;
+
+	dev_vdbg(&phy->dev, "Powering on QMP phy\n");
+
+	ret = regulator_enable(qphy->vdda_phy);
+	if (ret) {
+		dev_err(qphy->dev, "%s: vdda-phy enable failed, err=%d\n",
+				__func__, ret);
+		return ret;
+	}
+
+	ret = regulator_enable(qphy->vdda_pll);
+	if (ret) {
+		dev_err(qphy->dev, "%s: vdda-pll enable failed, err=%d\n",
+				__func__, ret);
+		goto err_vdda_pll;
+	}
+
+	if (qphy->vddp_ref_clk) {
+		ret = regulator_enable(qphy->vddp_ref_clk);
+		if (ret) {
+			dev_err(qphy->dev, "%s: vdda-ref-clk enable failed, err=%d\n",
+					__func__, ret);
+			goto err_vddp_refclk;
+		}
+	}
+
+	clk_prepare_enable(qphy->ref_clk_src);
+	clk_prepare_enable(qphy->ref_clk);
+	clk_prepare_enable(phydesc->pipe_clk);
+
+	return 0;
+
+err_vddp_refclk:
+	regulator_disable(qphy->vdda_pll);
+err_vdda_pll:
+	regulator_disable(qphy->vdda_phy);
+	return ret;
+}
+
+static int qcom_qmp_phy_poweroff(struct phy *phy)
+{
+	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
+	struct qcom_qmp_phy *qphy = phydesc->qphy;
+
+	clk_disable_unprepare(qphy->ref_clk_src);
+	clk_disable_unprepare(qphy->ref_clk);
+	clk_disable_unprepare(phydesc->pipe_clk);
+
+	if (qphy->vddp_ref_clk)
+		regulator_disable(qphy->vddp_ref_clk);
+
+	regulator_disable(qphy->vdda_pll);
+	regulator_disable(qphy->vdda_phy);
+
+	return 0;
+}
+
+static int qcom_qmp_phy_is_ready(struct qcom_qmp_phy *qphy,
+				void __iomem *pcs_status, u32 mask)
+{
+	unsigned int init_timeout;
+
+	init_timeout = PHY_READY_TIMEOUT_COUNT;
+	do {
+		if (readl_relaxed(pcs_status) & mask)
+			break;
+
+		usleep_range(REFCLK_STABILIZATION_DELAY_US_MIN,
+				 REFCLK_STABILIZATION_DELAY_US_MAX);
+	} while (--init_timeout);
+
+	if (!init_timeout)
+		return -EBUSY;
+
+	return 0;
+}
+
+static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
+{
+	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
+	void __iomem *serdes = qphy->serdes;
+	int ret;
+
+	mutex_lock(&qphy->phy_mutex);
+	if (qphy->init_count++) {
+		mutex_unlock(&qphy->phy_mutex);
+		return 0;
+	}
+
+	ret = reset_control_deassert(qphy->phy_rst);
+	if (ret) {
+		dev_err(qphy->dev, "phy reset deassert failed\n");
+		goto err;
+	}
+
+	ret = reset_control_deassert(qphy->phycom_rst);
+	if (ret) {
+		dev_err(qphy->dev, "common reset deassert failed\n");
+		goto err_phycom_rst;
+	}
+
+	if (qphy->phycfg_rst) {
+		ret = reset_control_deassert(qphy->phycfg_rst);
+		if (ret) {
+			dev_err(qphy->dev, "common reset deassert failed\n");
+			goto err_phycfg_rst;
+		}
+	}
+
+	if (cfg->has_phy_com_ctrl) {
+		qphy_setbits(serdes + cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
+				PHY_SW_PWRDN);
+		/* Make sure that above write is completed */
+		mb();
+	}
+
+	/* Serdes configuration */
+	qcom_qmp_phy_configure(serdes, cfg->regs, cfg->phy_init_serdes_tbl,
+				cfg->phy_init_serdes_tbl_sz);
+
+	if (cfg->has_phy_com_ctrl) {
+		qphy_clrbits(serdes + cfg->regs[QPHY_COM_SW_RESET],
+				PHY_SW_RESET);
+		qphy_setbits(serdes + cfg->regs[QPHY_COM_START_CONTROL],
+				PHY_SERDES_START | PHY_PCS_START);
+		/* Make sure that above write is completed */
+		mb();
+
+		ret = qcom_qmp_phy_is_ready(qphy, serdes +
+					cfg->regs[QPHY_COM_PCS_READY_STATUS],
+					MASK_COM_PCS_READY);
+		if (ret) {
+			dev_err(qphy->dev,
+				"common control block init timed-out\n");
+			goto err_phy_comctrl;
+		}
+	}
+
+	mutex_unlock(&qphy->phy_mutex);
+
+	return 0;
+
+err_phy_comctrl:
+	if (qphy->phycfg_rst)
+		reset_control_assert(qphy->phycfg_rst);
+err_phycfg_rst:
+	reset_control_assert(qphy->phycom_rst);
+err_phycom_rst:
+	reset_control_assert(qphy->phy_rst);
+err:
+	mutex_unlock(&qphy->phy_mutex);
+	return ret;
+}
+
+static int qcom_qmp_phy_com_exit(struct qcom_qmp_phy *qphy)
+{
+	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
+	void __iomem *serdes = qphy->serdes;
+
+	mutex_lock(&qphy->phy_mutex);
+	if (--qphy->init_count) {
+		mutex_unlock(&qphy->phy_mutex);
+		return 0;
+	}
+
+	if (cfg->has_phy_com_ctrl) {
+		qphy_setbits(serdes + cfg->regs[QPHY_COM_START_CONTROL],
+				PHY_SERDES_START | PHY_PCS_START);
+		qphy_clrbits(serdes + cfg->regs[QPHY_COM_SW_RESET],
+				PHY_SW_RESET);
+		qphy_setbits(serdes + cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
+				PHY_SW_PWRDN);
+
+		/* Make sure that above writes are completed */
+		mb();
+	}
+
+	reset_control_assert(qphy->phy_rst);
+	reset_control_assert(qphy->phycom_rst);
+	if (qphy->phycfg_rst)
+		reset_control_assert(qphy->phycfg_rst);
+
+	mutex_unlock(&qphy->phy_mutex);
+
+	return 0;
+}
+
+/* PHY Initialization */
+static int qcom_qmp_phy_init(struct phy *phy)
+{
+	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
+	struct qcom_qmp_phy *qphy = phydesc->qphy;
+	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
+	void __iomem *tx = phydesc->tx;
+	void __iomem *rx = phydesc->rx;
+	void __iomem *pcs = phydesc->pcs;
+	int ret;
+
+	dev_vdbg(qphy->dev, "Initializing QMP phy\n");
+
+	/* enable interface clocks to program phy */
+	clk_prepare_enable(qphy->aux_clk);
+	clk_prepare_enable(qphy->cfg_ahb_clk);
+
+	ret = qcom_qmp_phy_com_init(qphy);
+	if (ret)
+		goto err;
+
+	if (phydesc->lane_rst) {
+		ret = reset_control_deassert(phydesc->lane_rst);
+		if (ret) {
+			dev_err(qphy->dev, "lane<%d> reset deassert failed\n",
+					phydesc->index);
+			goto err_lane_rst;
+		}
+	}
+
+	/* Tx, Rx, and PCS configurations */
+	qcom_qmp_phy_configure(tx, cfg->regs, cfg->phy_init_tx_tbl,
+				cfg->phy_init_tx_tbl_sz);
+	qcom_qmp_phy_configure(rx, cfg->regs, cfg->phy_init_rx_tbl,
+				cfg->phy_init_rx_tbl_sz);
+	qcom_qmp_phy_configure(pcs, cfg->regs, cfg->phy_init_pcs_tbl,
+				cfg->phy_init_pcs_tbl_sz);
+
+	/*
+	 * Pull out PHY from POWER DOWN state:
+	 * This is active low enable signal to power-down PHY.
+	 */
+	qphy_setbits(pcs + QPHY_POWER_DOWN_CONTROL, cfg->mask_pwr_dn_ctrl);
+	/* XXX: 10 us delay; given in PCIE phy programming guide only */
+	usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
+
+	/* start SerDes and Phy-Coding-Sublayer */
+	qphy_setbits(pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
+
+	/* Pull PHY out of reset state */
+	qphy_clrbits(pcs + QPHY_SW_RESET, PHY_SW_RESET);
+	/* Make sure that above writes are completed */
+	mb();
+
+	ret = qcom_qmp_phy_is_ready(qphy, pcs +
+					cfg->regs[QPHY_PCS_READY_STATUS],
+					MASK_PHYSTATUS);
+	if (ret) {
+		dev_err(qphy->dev, "phy initialization timed-out\n");
+		goto err_pcs_ready;
+	}
+
+	return 0;
+
+err_pcs_ready:
+	if (phydesc->lane_rst)
+		reset_control_assert(phydesc->lane_rst);
+err_lane_rst:
+	qcom_qmp_phy_com_exit(qphy);
+err:
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+	clk_disable_unprepare(qphy->aux_clk);
+	return ret;
+}
+
+static int qcom_qmp_phy_exit(struct phy *phy)
+{
+	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
+	struct qcom_qmp_phy *qphy = phydesc->qphy;
+	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
+
+	/* PHY reset */
+	qphy_setbits(phydesc->pcs + QPHY_SW_RESET, PHY_SW_RESET);
+
+	/* stop SerDes and Phy-Coding-Sublayer */
+	qphy_clrbits(phydesc->pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
+
+	/* Put PHY into POWER DOWN state: active low */
+	qphy_clrbits(phydesc->pcs + QPHY_POWER_DOWN_CONTROL,
+			cfg->mask_pwr_dn_ctrl);
+
+	/* Make sure that above writes are completed */
+	mb();
+
+	if (phydesc->lane_rst)
+		reset_control_assert(phydesc->lane_rst);
+
+	qcom_qmp_phy_com_exit(qphy);
+
+	clk_disable_unprepare(qphy->aux_clk);
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+
+	return 0;
+}
+
+
+static int qcom_qmp_phy_regulator_init(struct device *dev)
+{
+	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
+	int ret;
+
+	qphy->vdda_phy = devm_regulator_get(dev, "vdda-phy");
+	if (IS_ERR(qphy->vdda_phy)) {
+		ret = PTR_ERR(qphy->vdda_phy);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get vdda-phy, %d\n", ret);
+		return ret;
+	}
+
+	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
+	if (IS_ERR(qphy->vdda_pll)) {
+		ret = PTR_ERR(qphy->vdda_pll);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get vdda-pll, %d\n", ret);
+		return ret;
+	}
+
+	/* optional regulator */
+	qphy->vddp_ref_clk = devm_regulator_get(dev, "vddp-ref-clk");
+	if (IS_ERR(qphy->vddp_ref_clk)) {
+		ret = PTR_ERR(qphy->vddp_ref_clk);
+		if (ret != -EPROBE_DEFER) {
+			dev_dbg(dev, "failed to get vddp-ref-clk, %d\n", ret);
+			qphy->vddp_ref_clk = NULL;
+		} else {
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int qcom_qmp_phy_clk_init(struct device *dev)
+{
+	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
+	int ret;
+
+	qphy->aux_clk = devm_clk_get(dev, "aux");
+	if (IS_ERR(qphy->aux_clk)) {
+		ret = PTR_ERR(qphy->aux_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get aux_clk\n");
+		return ret;
+	}
+
+	qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb");
+	if (IS_ERR(qphy->cfg_ahb_clk)) {
+		ret = PTR_ERR(qphy->cfg_ahb_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get cfg_ahb_clk\n");
+		return ret;
+	}
+
+	qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
+	if (IS_ERR(qphy->ref_clk_src)) {
+		ret = PTR_ERR(qphy->ref_clk_src);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get ref_clk_src\n");
+		return ret;
+	}
+
+	qphy->ref_clk = devm_clk_get(dev, "ref_clk");
+	if (IS_ERR(qphy->ref_clk)) {
+		ret = PTR_ERR(qphy->ref_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get ref_clk\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct phy *qcom_qmp_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
+	int i;
+
+	if (WARN_ON(args->args[0] >= qphy->cfg->nlanes))
+		return ERR_PTR(-ENODEV);
+
+	for (i = 0; i < qphy->cfg->nlanes; i++) {
+		if (qphy->phys[i]->index == args->args[0])
+			break;
+	}
+
+	if (i == qphy->cfg->nlanes)
+		return ERR_PTR(-ENODEV);
+
+	return qphy->phys[i]->phy;
+}
+
+static const struct phy_ops qcom_qmp_phy_gen_ops = {
+	.init		= qcom_qmp_phy_init,
+	.exit		= qcom_qmp_phy_exit,
+	.power_on	= qcom_qmp_phy_poweron,
+	.power_off	= qcom_qmp_phy_poweroff,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_qmp_phy_of_match_table[] = {
+	{
+		.compatible = "qcom,msm8996-qmp-pcie-phy",
+		.data = &msm8996_pciephy_init_cfg,
+	}, {
+		.compatible = "qcom,msm8996-qmp-usb3-phy",
+		.data = &msm8996_usb3phy_init_cfg,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_qmp_phy_of_match_table);
+
+static int qcom_qmp_phy_probe(struct platform_device *pdev)
+{
+	struct qcom_qmp_phy *qphy;
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	struct resource *res;
+	const struct of_device_id *match;
+	void __iomem *base;
+	int ret = 0;
+	int id;
+
+	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
+	if (!qphy)
+		return -ENOMEM;
+
+	qphy->dev = dev;
+	dev_set_drvdata(dev, qphy);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	/* per PHY serdes; usually located at base address */
+	qphy->serdes = base;
+
+	mutex_init(&qphy->phy_mutex);
+
+	/* Get the specific init parameters of QMP phy */
+	match = of_match_node(qcom_qmp_phy_of_match_table, dev->of_node);
+	qphy->cfg = match->data;
+
+	ret = qcom_qmp_phy_clk_init(dev);
+	if (ret) {
+		dev_err(dev, "clock init failed\n");
+		return ret;
+	}
+
+	ret = qcom_qmp_phy_regulator_init(dev);
+	if (ret) {
+		dev_err(dev, "regulator init failed\n");
+		return ret;
+	}
+
+	qphy->phy_rst = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(qphy->phy_rst)) {
+		dev_err(dev, "failed to get phy core reset\n");
+		return PTR_ERR(qphy->phy_rst);
+	}
+
+	qphy->phycom_rst = devm_reset_control_get(dev, "common");
+	if (IS_ERR(qphy->phycom_rst)) {
+		dev_err(dev, "failed to get phy common reset\n");
+		return PTR_ERR(qphy->phycom_rst);
+	}
+
+	qphy->phycfg_rst = devm_reset_control_get(dev, "cfg");
+	if (IS_ERR(qphy->phycfg_rst)) {
+		dev_dbg(dev, "failed to get phy ahb cfg reset\n");
+		qphy->phycfg_rst = NULL;
+	}
+
+	qphy->phys = devm_kcalloc(dev, qphy->cfg->nlanes,
+					sizeof(*qphy->phys), GFP_KERNEL);
+	if (!qphy->phys)
+		return -ENOMEM;
+
+	for (id = 0; id < qphy->cfg->nlanes; id++) {
+		struct phy *generic_phy;
+		struct qmp_phy_desc *phy_desc;
+		char prop_name[MAX_PROP_NAME];
+		unsigned int lane_offsets[3];
+
+		/* mem resources from index 1 to N for N number of lanes */
+		res = platform_get_resource(pdev, IORESOURCE_MEM, id + 1);
+		base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
+		if (!phy_desc)
+			return -ENOMEM;
+
+		/*
+		 * read offsets to Tx, Rx, and PCS blocks into a u32 array:
+		 *  ------------------------------------
+		 * | tx offset | rx offset | pcs offset |
+		 *  ------------------------------------
+		 */
+		ret = of_property_read_u32_array(dev->of_node, "lane-offsets",
+							   lane_offsets, 3);
+		if (ret) {
+			dev_err(dev,
+				"failed to get tx/rx/pcs offsets for lane%d\n",
+				id);
+				return ret;
+		}
+
+		phy_desc->tx = base + lane_offsets[0];
+		phy_desc->rx = base + lane_offsets[1];
+		phy_desc->pcs = base + lane_offsets[2];
+
+		/*
+		 * Get PHY's Pipe clock, if any; USB3 and PCIe are PIPE3
+		 * based phys, so they essentially have pipe clock. So,
+		 * we return error in case phy is USB3 or PIPE type.
+		 * Otherwise, we initialize pipe clock to NULL for
+		 * all phys that don't need this.
+		 */
+		memset(&prop_name, 0, sizeof(prop_name));
+		snprintf(prop_name, MAX_PROP_NAME, "pipe%d", id);
+		phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
+		if (IS_ERR(phy_desc->pipe_clk)) {
+			if (qphy->cfg->type == PHY_TYPE_PCIE ||
+			    qphy->cfg->type == PHY_TYPE_USB3) {
+				ret = PTR_ERR(phy_desc->pipe_clk);
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev,
+					"failed to get lane%d pipe_clk\n", id);
+				return ret;
+			} else {
+				phy_desc->pipe_clk = NULL;
+			}
+		}
+
+		/* Get lane reset, if any */
+		if (qphy->cfg->has_lane_rst) {
+			memset(&prop_name, 0, sizeof(prop_name));
+			snprintf(prop_name, MAX_PROP_NAME, "lane%d", id);
+			phy_desc->lane_rst = devm_reset_control_get(dev,
+								    prop_name);
+			if (IS_ERR(phy_desc->lane_rst)) {
+				dev_err(dev, "failed to get lane%d reset\n",
+					id);
+				return PTR_ERR(phy_desc->lane_rst);
+			}
+		}
+
+		generic_phy = devm_phy_create(dev, NULL, &qcom_qmp_phy_gen_ops);
+		if (IS_ERR(generic_phy)) {
+			ret = PTR_ERR(generic_phy);
+			dev_err(dev, "failed to create qphy %d\n", ret);
+			return ret;
+		}
+
+		phy_desc->phy = generic_phy;
+		phy_desc->index = id;
+		phy_desc->qphy = qphy;
+		phy_set_drvdata(generic_phy, phy_desc);
+		qphy->phys[id] = phy_desc;
+	}
+
+	phy_provider = devm_of_phy_provider_register(dev, qcom_qmp_phy_xlate);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		dev_err(dev, "failed to register qphy %d\n", ret);
+	}
+
+	return ret;
+}
+
+static struct platform_driver qcom_qmp_phy_driver = {
+	.probe		= qcom_qmp_phy_probe,
+	.driver = {
+		.name	= "qcom_qmp_phy",
+		.of_match_table = of_match_ptr(qcom_qmp_phy_of_match_table),
+	},
+};
+
+module_platform_driver(qcom_qmp_phy_driver);
+
+MODULE_AUTHOR("Vivek Gautam <vivek.gautam@codeaurora.org>");
+MODULE_DESCRIPTION("Qualcomm QMP PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
  2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
@ 2016-11-28 14:19   ` Rob Herring
  2016-11-29  5:20     ` Vivek Gautam
  2016-11-28 22:49   ` Stephen Boyd
  1 sibling, 1 reply; 21+ messages in thread
From: Rob Herring @ 2016-11-28 14:19 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, sboyd, linux-arm-msm

On Tue, Nov 22, 2016 at 05:32:40PM +0530, Vivek Gautam wrote:
> Qualcomm chipsets have QUSB2 phy controller that provides
> HighSpeed functionality for DWC3 controller.
> Adding dt binding information for the same.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
> 
> Changes since v1:
>  - New patch, forked out of the original driver patch:
>    "phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips"
>  - Updated dt bindings to remove 'hstx-trim-bit-offset' and
>    'hstx-trim-bit-len' bindings.
> 
>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt     | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> new file mode 100644
> index 0000000..38c8b30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -0,0 +1,55 @@
> +Qualcomm QUSB2 phy controller
> +=============================
> +
> +QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
> +
> +Required properties:
> + - compatible: compatible list, contains "qcom,msm8996-qusb2-phy".
> + - reg: offset and length of the PHY register set.
> + - #phy-cells: must be 0.
> +
> + - clocks: a list of phandles and clock-specifier pairs,
> +	   one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> +			"ref_clk" for 19.2 MHz ref clk,
> +			"ref_clk_src" reference clock source.
> +			"iface" for phy interface clock (Optional).
> +
> + - vdd-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> + - vdda-phy-dpdm: Phandle to 3.1V regulator supply to Dp/Dm port signals.

Needs '-supply'

> +
> + - resets: a list of phandles and reset controller specifier pairs,
> +	   one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block.
> +
> +Optional properties:
> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
> +		tuning parameters for qusb2 phy, one for each entry
> +		in nvmem-cell-names.
> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
> +		     HS Tx trim value.
> +
> + - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
> +
> +Example:
> +	hsphy: qusb2phy@7411000 {

usb-phy@...

> +		compatible = "qcom,msm8996-qusb2-phy";
> +		reg = <0x07411000 0x180>;
> +		#phy-cells = <0>;
> +
> +		clocks = <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +			<&gcc GCC_RX1_USB2_CLKREF_CLK>,
> +			<&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>;
> +		clock-names = "cfg_ahb_clk", "ref_clk", "ref_clk_src";
> +
> +		vdd-phy-supply = <&pm8994_s2>;
> +		vdda-pll-supply = <&pm8994_l12>;
> +		vdda-phy-dpdm-supply = <&pm8994_l24>;
> +
> +		resets = <&gcc GCC_QUSB2PHY_PRIM_BCR>;
> +		reset-names = "phy";
> +
> +		nvmem-cells = <&qusb2p_hstx_trim>;
> +		nvmem-cell-names = "tune2_hstx_trim_efuse";
> +        };
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
  2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
  2016-11-28 14:19   ` Rob Herring
@ 2016-11-28 22:49   ` Stephen Boyd
  2016-11-29  5:22     ` Vivek Gautam
  1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-11-28 22:49 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, linux-arm-msm

On 11/22, Vivek Gautam wrote:
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> new file mode 100644
> index 0000000..38c8b30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -0,0 +1,55 @@
> +Optional properties:
> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
> +		tuning parameters for qusb2 phy, one for each entry
> +		in nvmem-cell-names.
> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing

Do we really need efuse in the name? Seems redundant given this
is already an nvmem.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-22 12:02 ` [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
@ 2016-11-28 22:55   ` Stephen Boyd
  2016-11-29  5:25     ` Vivek Gautam
  2016-12-12 16:40     ` Vivek Gautam
  2016-11-28 23:19   ` Stephen Boyd
  1 sibling, 2 replies; 21+ messages in thread
From: Stephen Boyd @ 2016-11-28 22:55 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, linux-arm-msm

On 11/22, Vivek Gautam wrote:
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> new file mode 100644
> index 0000000..ffb173b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -0,0 +1,74 @@
> +Qualcomm QMP PHY
> +----------------
> +
> +QMP phy controller supports physical layer functionality for a number of
> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
> +
> +Required properties:
> + - compatible: compatible list, contains:
> +	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
> +	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
> + - reg: list of offset and length pair of the PHY register sets.
> +	at index 0: offset and length of register set for PHY common
> +		    serdes block.
> +	from index 1 - N: offset and length of register set for each lane,
> +			  for N number of phy lanes (ports).
> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
> + - #phy-cells: must be 1
> +    - Cell after phy phandle should be the port (lane) number.
> + - clocks: a list of phandles and clock-specifier pairs,
> +	   one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> +			"aux" for phy aux clock,
> +			"ref_clk" for 19.2 MHz ref clk,
> +			"ref_clk_src" for reference clock source,

We typically leave "clk" out of clk names because it's redundant.

> +			"pipe<port-number>" for pipe clock specific to
> +			each port/lane (Optional).

The pipe clocks are orphaned right now. We should add an output
clock from the phy to go into the controller and back into the
phy if I recall correctly. The phy should be a clock provider
itself so it can output the pipe clock source into GCC and back
into the phy and controller.

> + - resets: a list of phandles and reset controller specifier pairs,
> +	   one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block,
> +			"common" for phy common block reset,
> +			"cfg" for phy's ahb cfg block reset (Optional).
> +			"port<port-number>" for reset specific to
> +			each port/lane (Optional).
> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> +
> +Optional properties:
> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
> +			pll block.
> +
> +Example:
> +	pcie_phy: pciephy@34000 {

pcie-phy or just phy as the node name?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  2016-11-22 12:02 ` [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam
@ 2016-11-28 23:14   ` Stephen Boyd
  2016-12-01  8:42     ` Vivek Gautam
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-11-28 23:14 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, linux-arm-msm

On 11/22, Vivek Gautam wrote:
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index e8eb7f2..f1dcec1 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -430,6 +430,17 @@ config PHY_STIH407_USB
>  	  Enable this support to enable the picoPHY device used by USB2
>  	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
>  
> +config PHY_QCOM_QUSB2
> +	tristate "Qualcomm QUSB2 PHY Driver"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	select GENERIC_PHY
> +	select RESET_CONTROLLER

This shouldn't be necessary. We only need to select it if we're
providing resets.

> +	help
> +	  Enable this to support the HighSpeed QUSB2 PHY transceiver for USB
> +	  controllers on Qualcomm chips. This driver supports the high-speed
> +	  PHY which is usually paired with either the ChipIdea or Synopsys DWC3
> +	  USB IPs on MSM SOCs.
> +
>  config PHY_QCOM_UFS
>  	tristate "Qualcomm UFS PHY driver"
>  	depends on OF && ARCH_QCOM
> diff --git a/drivers/phy/phy-qcom-qusb2.c b/drivers/phy/phy-qcom-qusb2.c
> new file mode 100644
> index 0000000..d3f9657
> --- /dev/null
> +++ b/drivers/phy/phy-qcom-qusb2.c
> @@ -0,0 +1,549 @@
> +/*
> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define QUSB2PHY_PLL_TEST		0x04
> +#define CLK_REF_SEL			BIT(7)
> +
> +#define QUSB2PHY_PLL_TUNE		0x08
> +#define QUSB2PHY_PLL_USER_CTL1		0x0c
> +#define QUSB2PHY_PLL_USER_CTL2		0x10
> +#define QUSB2PHY_PLL_AUTOPGM_CTL1	0x1c
> +#define QUSB2PHY_PLL_PWR_CTRL		0x18
> +
> +#define QUSB2PHY_PLL_STATUS		0x38
> +#define PLL_LOCKED			BIT(5)
> +
> +#define QUSB2PHY_PORT_TUNE1             0x80
> +#define QUSB2PHY_PORT_TUNE2             0x84
> +#define QUSB2PHY_PORT_TUNE3             0x88
> +#define QUSB2PHY_PORT_TUNE4             0x8C
> +#define QUSB2PHY_PORT_TUNE5		0x90
> +#define QUSB2PHY_PORT_TEST2		0x9c

Please use lowercase or uppercase consistently (I'd prefer
lowercase).

> +
> +#define QUSB2PHY_PORT_POWERDOWN		0xB4
> +#define CLAMP_N_EN			BIT(5)
> +#define FREEZIO_N			BIT(1)
> +#define POWER_DOWN			BIT(0)
> +
> +#define QUSB2PHY_REFCLK_ENABLE		BIT(0)
> +
> +#define PHY_CLK_SCHEME_SEL		BIT(0)
> +
> +struct qusb2_phy_init_tbl {
> +	unsigned int reg_offset;
> +	unsigned int cfg_val;
> +};
> +#define QCOM_QUSB2_PHY_INIT_CFG(reg, val) \
> +	{				\
> +		.reg_offset = reg,	\
> +		.cfg_val = val,		\
> +	}
> +
> +static struct qusb2_phy_init_tbl msm8996_phy_init_tbl[] = {

const?

> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xF8),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xB3),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xC0),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9F),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
> +};
> +
> +struct qusb2_phy_init_cfg {
> +	struct qusb2_phy_init_tbl *phy_init_tbl;
> +	int phy_init_tbl_sz;
> +	/* offset to PHY_CLK_SCHEME register in TCSR map. */
> +	unsigned int clk_scheme_offset;
> +};
> +
> +const struct qusb2_phy_init_cfg msm8996_phy_init_cfg = {

static?

> +	.phy_init_tbl = msm8996_phy_init_tbl,
> +	.phy_init_tbl_sz = ARRAY_SIZE(msm8996_phy_init_tbl),
> +};
> +
> +/**
> + * struct qusb2_phy: Structure holding qusb2 phy attributes.
> + *
> + * @phy: pointer to generic phy.
> + * @base: pointer to iomapped memory space for qubs2 phy.
> + *
> + * @cfg_ahb_clk: pointer to AHB2PHY interface clock.
> + * @ref_clk: pointer to reference clock.
> + * @ref_clk_src: pointer to source to reference clock.
> + * @iface_src: pointer to phy interface clock.
> + *
> + * @phy_reset: Pointer to phy reset control
> + *
> + * @vdda_phy: vdd supply to the phy core block.
> + * @vdda_pll: 1.8V vdd supply to ref_clk block.
> + * @vdda_phy_dpdm: 3.1V vdd supply to Dp/Dm port signals.
> + * @tcsr: pointer to TCSR syscon register map.

Drop all the full stops on these comments please.

> + *
> + * @cfg: phy initialization config data
> + * @has_se_clk_scheme: indicate if PHY has Single-ended ref clock scheme

Why is single capitalized?

> + */
> +struct qusb2_phy {
> +	struct phy *phy;
> +	void __iomem *base;
> +
> +	struct clk *cfg_ahb_clk;
> +	struct clk *ref_clk;
> +	struct clk *ref_clk_src;
> +	struct clk *iface_clk;
> +
> +	struct reset_control *phy_reset;
> +
> +	struct regulator *vdd_phy;
> +	struct regulator *vdda_pll;
> +	struct regulator *vdda_phy_dpdm;
> +
> +	struct regmap *tcsr;
> +
> +	const struct qusb2_phy_init_cfg *cfg;
> +	bool has_se_clk_scheme;
> +};
> +
> +static inline void qusb2_setbits(void __iomem *reg, u32 val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl_relaxed(reg);
> +	reg_val |= val;
> +	writel_relaxed(reg_val, reg);
> +
> +	/* Ensure above write is completed */
> +	mb();
> +}
> +
> +static inline void qusb2_clrbits(void __iomem *reg, u32 val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl_relaxed(reg);
> +	reg_val &= ~val;
> +	writel_relaxed(reg_val, reg);
> +
> +	/* Ensure above write is completed */
> +	mb();
> +}
> +
> +static void qcom_qusb2_phy_configure(void __iomem *base,
> +				struct qusb2_phy_init_tbl init_tbl[],
> +				int init_tbl_sz)
> +{
> +	int i;
> +
> +	for (i = 0; i < init_tbl_sz; i++) {
> +		writel_relaxed(init_tbl[i].cfg_val,
> +				base + init_tbl[i].reg_offset);
> +	}
> +
> +	/* flush buffered writes */
> +	mb();
> +}
> +
> +static void qusb2_phy_enable_clocks(struct qusb2_phy *qphy, bool on)

Maybe s/enable/toggle/ because we're not always enabling.

> +{
> +	if (on) {
> +		clk_prepare_enable(qphy->iface_clk);
> +		clk_prepare_enable(qphy->ref_clk_src);
> +	} else {
> +		clk_disable_unprepare(qphy->ref_clk_src);
> +		clk_disable_unprepare(qphy->iface_clk);
> +	}
> +
> +	dev_vdbg(&qphy->phy->dev, "%s(): clocks enabled\n", __func__);

Heh or disabled!

> +}
> +
> +static int qusb2_phy_enable_power(struct qusb2_phy *qphy, bool on)

Maybe s/enable/toggle/ because we're not always enabling.

> +{
> +	int ret;
> +	struct device *dev = &qphy->phy->dev;
> +
> +	if (!on)
> +		goto disable_vdda_phy_dpdm;
> +
> +	ret = regulator_enable(qphy->vdd_phy);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdd-phy:%d\n", ret);
> +		goto err_vdd_phy;
> +	}
> +
> +	ret = regulator_enable(qphy->vdda_pll);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdda-pll:%d\n", ret);
> +		goto disable_vdd_phy;
> +	}
> +
> +	ret = regulator_enable(qphy->vdda_phy_dpdm);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdda-phy-dpdm:%d\n", ret);
> +		goto disable_vdda_pll;
> +	}
> +
> +	dev_vdbg(dev, "%s() regulators are turned on.\n", __func__);

Drop the full stop please.

> +
> +	return ret;
> +
> +disable_vdda_phy_dpdm:
> +	regulator_disable(qphy->vdda_phy_dpdm);
> +disable_vdda_pll:
> +	regulator_disable(qphy->vdda_pll);
> +disable_vdd_phy:
> +	regulator_disable(qphy->vdd_phy);
> +err_vdd_phy:
> +	dev_vdbg(dev, "%s() regulators are turned off.\n", __func__);
> +	return ret;
> +}
> +
> +/*
> + * Fetches HS Tx tuning value from e-fuse and sets QUSB2PHY_PORT_TUNE2
> + * register.
> + * For any error case, skip setting the value and use the default value.
> + */
> +static int qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
> +{
> +	struct device *dev = &qphy->phy->dev;
> +	struct nvmem_cell *cell;
> +	ssize_t len;
> +	u8 *val;
> +
> +	/*
> +	 * Read EFUSE register having TUNE2 parameter's high nibble.
> +	 * If efuse register shows value as 0x0, or if we fail to find
> +	 * a valid efuse register settings, then use default value
> +	 * as 0xB for high nibble that we have already set while
> +	 * configuring phy.
> +	 */
> +	cell = devm_nvmem_cell_get(dev, "tune2_hstx_trim_efuse");
> +	if (IS_ERR(cell)) {
> +		if (PTR_ERR(cell) == -EPROBE_DEFER)
> +			return PTR_ERR(cell);
> +		goto skip;

Why do we get the nvmem cell here? Wouldn't we want to get it
during probe? Returning probe defer here during init would be
odd.

> +	}
> +
> +	/*
> +	 * we need to read only one byte here, since the required
> +	 * parameter value fits in one nibble
> +	 */
> +	val = (u8 *)nvmem_cell_read(cell, &len);

Shouldn't need the cast here. Also it would be nice if
nvmem_cell_read() didn't require a second argument if we don't
care for it. We should update the API to allow NULL there.

> +	if (!IS_ERR(val)) {
> +		/* Fused TUNE2 value is the higher nibble only */
> +		qusb2_setbits(qphy->base + QUSB2PHY_PORT_TUNE2,
> +							val[0] << 0x4);
> +	} else {
> +		dev_dbg(dev, "failed reading hs-tx trim value: %ld\n",
> +							PTR_ERR(val));
> +	}
> +
> +skip:
> +	return 0;
> +}
> +
[...]
> +
> +static int qusb2_phy_init(struct phy *phy)
> +{
> +	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> +	unsigned int reset_val;
> +	unsigned int clk_scheme;
> +	int ret;
> +
> +	dev_vdbg(&phy->dev, "Initializing QUSB2 phy\n");
> +
> +	/* enable ahb interface clock to program phy */
> +	clk_prepare_enable(qphy->cfg_ahb_clk);

What if that fails?

> +
> +	/* Perform phy reset */
> +	ret = reset_control_assert(qphy->phy_reset);
> +	if (ret) {
> +		dev_err(&phy->dev, "Failed to assert phy_reset\n");
> +		return ret;
> +	}
> +	/* 100 us delay to keep PHY in reset mode */
> +	usleep_range(100, 150);
> +	ret = reset_control_deassert(qphy->phy_reset);
> +	if (ret) {
> +		dev_err(&phy->dev, "Failed to de-assert phy_reset\n");
> +		return ret;
> +	}
> +
> +	/* Disable the PHY */
> +	qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
> +			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
> +
> +	/* save reset value to override based on clk scheme */
> +	reset_val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST);
> +
> +	qcom_qusb2_phy_configure(qphy->base, qphy->cfg->phy_init_tbl,
> +				qphy->cfg->phy_init_tbl_sz);
> +
> +	/* Check for efuse value for tuning the PHY */
> +	ret = qusb2_phy_set_tune2_param(qphy);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable the PHY */
> +	qusb2_clrbits(qphy->base + QUSB2PHY_PORT_POWERDOWN, POWER_DOWN);
> +
> +	/* Require to get phy pll lock successfully */
> +	usleep_range(150, 160);
> +
> +	/* Default is Single-ended clock on msm8996 */
> +	qphy->has_se_clk_scheme = true;
> +	/*
> +	 * read TCSR_PHY_CLK_SCHEME register to check if Single-ended

Capital Single again?

> +	 * clock scheme is selected. If yes, then disable differential
> +	 * ref_clk and use single-ended clock, otherwise use differential
> +	 * ref_clk only.
> +	 */
> +	if (qphy->tcsr) {
> +		ret = regmap_read(qphy->tcsr, qphy->cfg->clk_scheme_offset,
> +							&clk_scheme);
> +		/* is it a differential clock scheme ? */
> +		if (!(clk_scheme & PHY_CLK_SCHEME_SEL)) {
> +			dev_vdbg(&phy->dev, "%s: select differential clk src\n",
> +								__func__);
> +			qphy->has_se_clk_scheme = false;
> +		} else {
> +			dev_vdbg(&phy->dev, "%s: select single-ended clk src\n",
> +								__func__);
> +		}
> +	}
> +
> +	if (!qphy->has_se_clk_scheme) {
> +		reset_val &= ~CLK_REF_SEL;
> +		clk_prepare_enable(qphy->ref_clk);

And if that fails?

> +	} else {
> +		reset_val |= CLK_REF_SEL;
> +	}
> +
> +	writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
> +
> +	/* Make sure that above write is completed to get PLL source clock */
> +	wmb();
> +
> +	/* Required to get PHY PLL lock successfully */
> +	usleep_range(100, 110);
> +
> +	if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
> +					PLL_LOCKED)) {
> +		dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
> +			readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));

Would be pretty funny if this was locked now when the error
printk runs. Are there other bits in there that are helpful?

> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qusb2_phy_exit(struct phy *phy)
> +{
> +	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> +
> +	/* Disable the PHY */
> +	qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
> +			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
> +
> +	if (!qphy->has_se_clk_scheme)
> +		clk_disable_unprepare(qphy->ref_clk);
> +
> +	clk_disable_unprepare(qphy->cfg_ahb_clk);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops qusb2_phy_gen_ops = {
> +	.init		= qusb2_phy_init,
> +	.exit		= qusb2_phy_exit,
> +	.power_on	= qusb2_phy_poweron,
> +	.power_off	= qusb2_phy_poweroff,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id qusb2_phy_of_match_table[] = {
> +	{
> +		.compatible	= "qcom,msm8996-qusb2-phy",
> +		.data		= &msm8996_phy_init_cfg,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qusb2_phy_of_match_table);
> +
> +static int qusb2_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qusb2_phy *qphy;
> +	struct phy_provider *phy_provider;
> +	struct phy *generic_phy;
> +	const struct of_device_id *match;
> +	struct resource *res;
> +	int ret;
> +
> +	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> +	if (!qphy)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	qphy->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(qphy->base))
> +		return PTR_ERR(qphy->base);
> +
> +	qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb_clk");
> +	if (IS_ERR(qphy->cfg_ahb_clk)) {
> +		ret = PTR_ERR(qphy->cfg_ahb_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get cfg_ahb_clk\n");
> +		return ret;
> +	}
> +
> +	qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
> +	if (IS_ERR(qphy->ref_clk_src)) {
> +		ret = PTR_ERR(qphy->ref_clk_src);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "clk get failed for ref_clk_src\n");
> +		return ret;
> +	}
> +
> +	qphy->ref_clk = devm_clk_get(dev, "ref_clk");
> +	if (IS_ERR(qphy->ref_clk)) {
> +		ret = PTR_ERR(qphy->ref_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "clk get failed for ref_clk\n");
> +		return ret;
> +	} else {
> +		clk_set_rate(qphy->ref_clk, 19200000);

Drop the else. Also what if clk_set_rate() fails?

> +	}
> +
> +	qphy->iface_clk = devm_clk_get(dev, "iface_clk");
> +	if (IS_ERR(qphy->iface_clk)) {
> +		ret = PTR_ERR(qphy->iface_clk);
> +		if (ret != -EPROBE_DEFER) {
> +			qphy->iface_clk = NULL;
> +			dev_dbg(dev, "clk get failed for iface_clk\n");
> +		} else {
> +			return ret;
> +		}

		if (PTR_ERR(qphy->iface_clk) == -EPROBE_DEFER)
			return -EPROBE_DEFER;
		qphy->iface_clk = NULL;
		dev_dbg(dev, "clk get failed for iface_clk\n");

Is shorter.
		
> +	}
> +
> +	qphy->phy_reset = devm_reset_control_get(&pdev->dev, "phy");
> +	if (IS_ERR(qphy->phy_reset)) {
> +		dev_err(dev, "failed to get phy core reset\n");
> +		return PTR_ERR(qphy->phy_reset);
> +	}
> +
> +	qphy->vdd_phy = devm_regulator_get(dev, "vdd-phy");
> +	if (IS_ERR(qphy->vdd_phy)) {
> +		dev_err(dev, "unable to get vdd-phy supply\n");
> +		return PTR_ERR(qphy->vdd_phy);
> +	}
> +
> +	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
> +	if (IS_ERR(qphy->vdda_pll)) {
> +		dev_err(dev, "unable to get vdda-pll supply\n");
> +		return PTR_ERR(qphy->vdda_pll);
> +	}
> +
> +	qphy->vdda_phy_dpdm = devm_regulator_get(dev, "vdda-phy-dpdm");
> +	if (IS_ERR(qphy->vdda_phy_dpdm)) {
> +		dev_err(dev, "unable to get vdda-phy-dpdm supply\n");
> +		return PTR_ERR(qphy->vdda_phy_dpdm);
> +	}
> +
> +	/* Get the specific init parameters of QMP phy */
> +	match = of_match_node(qusb2_phy_of_match_table, dev->of_node);
> +	qphy->cfg = match->data;

Use of_device_get_match_data() instead.

> +
> +	qphy->tcsr = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							"qcom,tcsr-syscon");
> +	if (IS_ERR(qphy->tcsr)) {
> +		dev_dbg(dev, "Failed to lookup TCSR regmap\n");
> +		qphy->tcsr = NULL;
> +	}
> +
> +	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
> +	if (IS_ERR(generic_phy)) {
> +		ret = PTR_ERR(generic_phy);
> +		dev_err(dev, "%s: failed to create phy %d\n", __func__, ret);
> +		return ret;
> +	}
> +	qphy->phy = generic_phy;
> +
> +	dev_set_drvdata(dev, qphy);
> +	phy_set_drvdata(generic_phy, qphy);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		dev_err(dev, "%s: failed to register phy %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-22 12:02 ` [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
  2016-11-28 22:55   ` Stephen Boyd
@ 2016-11-28 23:19   ` Stephen Boyd
  2016-12-13  9:18     ` Vivek Gautam
  1 sibling, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-11-28 23:19 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, linux-arm-msm

On 11/22, Vivek Gautam wrote:
> Qualcomm chipsets have QMP phy controller that provides
> support to a number of controller, viz. PCIe, UFS, and USB.
> Adding dt binding information for the same.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> Changes since v1:
>  - New patch, forked out of the original driver patch:
>    "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
>  - updated bindings to include mem resource as a list of
>    offset - length pair for serdes block and for each lane.
>  - added a new binding for 'lane-offsets' that contains offsets
>    to tx, rx and pcs blocks from each lane base address.
> 
>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 74 ++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> new file mode 100644
> index 0000000..ffb173b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -0,0 +1,74 @@
> +Qualcomm QMP PHY
> +----------------
> +
> +QMP phy controller supports physical layer functionality for a number of
> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
> +
> +Required properties:
> + - compatible: compatible list, contains:
> +	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
> +	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
> + - reg: list of offset and length pair of the PHY register sets.
> +	at index 0: offset and length of register set for PHY common
> +		    serdes block.
> +	from index 1 - N: offset and length of register set for each lane,
> +			  for N number of phy lanes (ports).
> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
> + - #phy-cells: must be 1
> +    - Cell after phy phandle should be the port (lane) number.
> + - clocks: a list of phandles and clock-specifier pairs,
> +	   one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> +			"aux" for phy aux clock,
> +			"ref_clk" for 19.2 MHz ref clk,
> +			"ref_clk_src" for reference clock source,
> +			"pipe<port-number>" for pipe clock specific to
> +			each port/lane (Optional).
> + - resets: a list of phandles and reset controller specifier pairs,
> +	   one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block,
> +			"common" for phy common block reset,
> +			"cfg" for phy's ahb cfg block reset (Optional).
> +			"port<port-number>" for reset specific to
> +			each port/lane (Optional).
> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> +
> +Optional properties:
> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
> +			pll block.
> +
> +Example:
> +	pcie_phy: pciephy@34000 {
> +		compatible = "qcom,msm8996-qmp-pcie-phy";
> +		reg = <0x034000 0x48f>,
> +			<0x035000 0x5bf>,
> +			<0x036000 0x5bf>,
> +			<0x037000 0x5bf>;
> +				/* tx, rx, pcs */
> +		lane-offsets = <0x0 0x200 0x400>;
> +		#phy-cells = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
> +			<&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
> +			<&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>,
> +			<&gcc GCC_PCIE_CLKREF_CLK>,
> +			<&gcc GCC_PCIE_0_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_2_PIPE_CLK>;
> +		clock-names = "aux", "cfg_ahb",
> +				"ref_clk_src", "ref_clk",

Does MSM8996_RPM_SMD_LN_BB_CLK supply the clock source for
GCC_PCIE_CLKREF_CLK? Did we mess up the parent/child relationship
in the GCC driver? We may want to fix that so that this node
only references clocks that actually go into the device, instead
of clock parents.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
  2016-11-22 12:02 ` [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam
@ 2016-11-29  0:35   ` Stephen Boyd
  2016-12-20  5:42     ` Vivek Gautam
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-11-29  0:35 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, mark.rutland, devicetree, linux-kernel,
	srinivas.kandagatla, linux-arm-msm

On 11/22, Vivek Gautam wrote:
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index f1dcec1..8970d9e 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -430,6 +430,15 @@ config PHY_STIH407_USB
>  	  Enable this support to enable the picoPHY device used by USB2
>  	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
>  
> +config PHY_QCOM_QMP
> +	tristate "Qualcomm QMP PHY Driver"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	select GENERIC_PHY
> +	select RESET_CONTROLLER

Again, probably should be dropped as we're not providing resets
here, just using them.

> diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
> new file mode 100644
> index 0000000..f85289e
> --- /dev/null
> +++ b/drivers/phy/phy-qcom-qmp.c
> @@ -0,0 +1,1141 @@
> +/*
> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/phy/phy.h>
> +
[...]
> +
> +/* set of registers with offsets different per-PHY */
> +enum qphy_reg_layout {
> +	/* Common block control registers */
> +	QPHY_COM_SW_RESET,
> +	QPHY_COM_POWER_DOWN_CONTROL,
> +	QPHY_COM_START_CONTROL,
> +	QPHY_COM_PCS_READY_STATUS,
> +	/* PCS registers */
> +	QPHY_PLL_LOCK_CHK_DLY_TIME,
> +	QPHY_FLL_CNTRL1,
> +	QPHY_FLL_CNTRL2,
> +	QPHY_FLL_CNT_VAL_L,
> +	QPHY_FLL_CNT_VAL_H_TOL,
> +	QPHY_FLL_MAN_CODE,
> +	QPHY_PCS_READY_STATUS,
> +};
> +
> +unsigned int pciephy_regs_layout[] = {

static? const?

> +	[QPHY_COM_SW_RESET]		= 0x400,
> +	[QPHY_COM_POWER_DOWN_CONTROL]	= 0x404,
> +	[QPHY_COM_START_CONTROL]	= 0x408,
> +	[QPHY_COM_PCS_READY_STATUS]	= 0x448,
> +	[QPHY_PLL_LOCK_CHK_DLY_TIME]	= 0xa8,
> +	[QPHY_FLL_CNTRL1]		= 0xc4,
> +	[QPHY_FLL_CNTRL2]		= 0xc8,
> +	[QPHY_FLL_CNT_VAL_L]		= 0xcc,
> +	[QPHY_FLL_CNT_VAL_H_TOL]	= 0xd0,
> +	[QPHY_FLL_MAN_CODE]		= 0xd4,
> +	[QPHY_PCS_READY_STATUS]		= 0x174,
> +};
> +
> +unsigned int usb3phy_regs_layout[] = {

static? const?

> +	[QPHY_FLL_CNTRL1]		= 0xc0,
> +	[QPHY_FLL_CNTRL2]		= 0xc4,
> +	[QPHY_FLL_CNT_VAL_L]		= 0xc8,
> +	[QPHY_FLL_CNT_VAL_H_TOL]	= 0xcc,
> +	[QPHY_FLL_MAN_CODE]		= 0xd0,
> +	[QPHY_PCS_READY_STATUS]		= 0x17c,
> +};
> +
> +static struct qmp_phy_init_tbl pciephy_serdes_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x1c),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_ENABLE1, 0x10),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x33),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x06),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP_EN, 0x42),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_TIMER1, 0xff),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_TIMER2, 0x1f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SVS_MODE_CLK_SEL, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORE_CLK_EN, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORECLK_DIV, 0x0a),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TIMER, 0x09),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DEC_START_MODE0, 0x82),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START3_MODE0, 0x03),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START2_MODE0, 0x55),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START1_MODE0, 0x55),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE0, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP2_MODE0, 0x1a),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP1_MODE0, 0x0a),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x33),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYS_CLK_CTRL, 0x02),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_BUF_ENABLE, 0x1f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x04),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CP_CTRL_MODE0, 0x0b),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_RCTRL_MODE0, 0x16),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_CCTRL_MODE0, 0x28),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN1_MODE0, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN0_MODE0, 0x80),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_EN_CENTER, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER1, 0x31),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER2, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER1, 0x02),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER2, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE1, 0x2f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE2, 0x19),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_RESCODE_DIV_NUM, 0x15),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TRIM, 0x0f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_IVCO, 0x0f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_EP_DIV, 0x19),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_ENABLE1, 0x10),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_RESCODE_DIV_NUM, 0x40),
> +};
> +
> +static struct qmp_phy_init_tbl pciephy_tx_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
> +};
> +
> +static struct qmp_phy_init_tbl pciephy_rx_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_ENABLES, 0x1c),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4, 0xdb),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_BAND, 0x18),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN, 0x04),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN_HALF, 0x04),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x4b),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_DEGLITCH_CNTRL, 0x14),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x19),
> +};
> +
> +static struct qmp_phy_init_tbl pciephy_pcs_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QPHY_RX_IDLE_DTCT_CNTRL, 0x4c),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_PWRUP_RESET_DLY_TIME_AUXCLK, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_LP_WAKEUP_DLY_TIME_AUXCLK, 0x01),
> +
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_PLL_LOCK_CHK_DLY_TIME, 0x05),
> +
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_ENDPOINT_REFCLK_DRIVE, 0x05),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_DOWN_CONTROL, 0x02),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG4, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG1, 0xa3),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_TXDEEMPH_M3P5DB_V0, 0x0e),
> +};
> +
> +static struct qmp_phy_init_tbl usb3phy_serdes_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x14),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x08),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x06),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SVS_MODE_CLK_SEL, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TRIM, 0x0f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_IVCO, 0x0f),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SYS_CLK_CTRL, 0x04),
> +	/* PLL and Loop filter settings */
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DEC_START_MODE0, 0x82),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START1_MODE0, 0x55),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START2_MODE0, 0x55),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START3_MODE0, 0x03),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CP_CTRL_MODE0, 0x0b),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_RCTRL_MODE0, 0x16),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_PLL_CCTRL_MODE0, 0x28),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN0_MODE0, 0x80),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_CTRL, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP1_MODE0, 0x15),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP2_MODE0, 0x34),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE0, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_CORE_CLK_EN, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP_CFG, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_BG_TIMER, 0x0a),
> +	/* SSC settings */
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_EN_CENTER, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER1, 0x31),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER2, 0x01),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER1, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER2, 0x00),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE1, 0xde),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE2, 0x07),
> +};
> +
> +static struct qmp_phy_init_tbl usb3phy_tx_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_RCV_DETECT_LVL_2, 0x12),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
> +};
> +
> +static struct qmp_phy_init_tbl usb3phy_rx_init_tbl[] = {

const?

> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN, 0x04),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x02),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x4c),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4, 0xbb),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x77),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_RX_OFFSET_ADAPTOR_CNTRL2, 0x80),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x03),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_LVL, 0x18),
> +	QCOM_QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_DEGLITCH_CNTRL, 0x16),
> +};
> +
> +static struct qmp_phy_init_tbl usb3phy_pcs_init_tbl[] = {

const?

> +	/* FLL settings */
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNTRL2, 0x03),
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNTRL1, 0x02),
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNT_VAL_L, 0x09),
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_CNT_VAL_H_TOL, 0x42),
> +	QCOM_QMP_PHY_INIT_CFG_L(QPHY_FLL_MAN_CODE, 0x85),
> +
> +	/* Lock Det settings */
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG1, 0xd1),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG2, 0x1f),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_LOCK_DETECT_CONFIG3, 0x47),
> +	QCOM_QMP_PHY_INIT_CFG(QPHY_POWER_STATE_CONFIG2, 0x08),
> +};
> +
> +/**
> + * struct qmp_phy_init_cfg:- per-PHY init config.

The colon and dash can't be right. One is kernel-doc, but of
course there aren't any other member descriptions.

> + */
> +struct qmp_phy_init_cfg {
> +	/* phy-type - PCIE/UFS/USB */
> +	unsigned int type;
> +	/* number of lanes provided by phy */
> +	int nlanes;
> +
> +	/* Initialization sequence for PHY blocks - Serdes, tx, rx, pcs */
> +	struct qmp_phy_init_tbl *phy_init_serdes_tbl;
> +	int phy_init_serdes_tbl_sz;
> +	struct qmp_phy_init_tbl *phy_init_tx_tbl;
> +	int phy_init_tx_tbl_sz;
> +	struct qmp_phy_init_tbl *phy_init_rx_tbl;
> +	int phy_init_rx_tbl_sz;
> +	struct qmp_phy_init_tbl *phy_init_pcs_tbl;
> +	int phy_init_pcs_tbl_sz;

_sz makes it sound like bytes, perhaps _num instead.

> +
> +	/* array of registers with different offsets */
> +	unsigned int *regs;

const?

> +
> +	unsigned int mask_start_ctrl;
> +	unsigned int mask_pwr_dn_ctrl;
> +	/* true, if PHY has a separate PHY_COM_CNTRL block */
> +	bool has_phy_com_ctrl;
> +	/* true, if PHY has a reset for individual lanes */
> +	bool has_lane_rst;
> +};
> +
> +/**
> + * struct qmp_phy_desc:- per-lane phy-descriptor.

Also kernel-doc requests full stop is left out on one line
descriptions.

> + *
> + * @phy: pointer to generic phy
> + * @tx: pointer to iomapped memory space for PHY's tx
> + * @rx: pointer to iomapped memory space for PHY's rx
> + * @pcs: pointer to iomapped memory space for PHY's pcs
> + * @pipe_clk: pointer to pipe lock
> + * @index: lane index
> + * @qphy: pointer to QMP phy to which this lane belongs
> + * @lane_rst: pointer to lane's reset controller

Not sure we care about "pointer to" as types can be figured out
other ways.

> + */
> +struct qmp_phy_desc {
> +	struct phy *phy;
> +	void __iomem *tx;
> +	void __iomem *rx;
> +	void __iomem *pcs;
> +	struct clk *pipe_clk;
> +	unsigned int index;
> +	struct qcom_qmp_phy *qphy;
> +	struct reset_control *lane_rst;
> +};
> +
> +/**
> + * struct qcom_qmp_phy:- structure holding QMP PHY attributes.
> + *
> + * @dev: pointer to device
> + * @serdes: pointer to iomapped memory space for phy's serdes
> + *
> + * @aux_clk: pointer to phy core clock
> + * @cfg_ahb_clk: pointer to AHB2PHY interface clock
> + * @ref_clk: pointer to reference clock
> + * @ref_clk_src: pointer to source to reference clock
> + *
> + * @vdda_phy: vdd supply to the phy core block
> + * @vdda_pll: 1.8V vdd supply to ref_clk block
> + * @vddp_ref_clk: vdd supply to specific ref_clk block (Optional)
> + *
> + * @phy_rst: Pointer to phy reset control
> + * @phycom_rst: Pointer to phy common reset control
> + * @phycfg_rst: Pointer to phy ahb cfg reset control (Optional)
> + *
> + * @cfg: pointer to init config for each phys
> + * @phys: array of pointer to per-lane phy descriptors
> + * @phy_mutex: mutex lock for PHY common block initialization
> + * @init_count: Phy common block initialization count
> + */
> +struct qcom_qmp_phy {
> +	struct device *dev;
> +	void __iomem *serdes;
> +
> +	struct clk *aux_clk;
> +	struct clk *cfg_ahb_clk;
> +	struct clk *ref_clk;
> +	struct clk *ref_clk_src;
> +
> +	struct regulator *vdda_phy;
> +	struct regulator *vdda_pll;
> +	struct regulator *vddp_ref_clk;
> +
> +	struct reset_control *phy_rst;
> +	struct reset_control *phycom_rst;
> +	struct reset_control *phycfg_rst;
> +
> +	const struct qmp_phy_init_cfg *cfg;
> +	struct qmp_phy_desc **phys;
> +
> +	struct mutex phy_mutex;
> +	int init_count;
> +};
> +
> +static inline void qphy_setbits(void __iomem *reg, u32 val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl_relaxed(reg);
> +	reg_val |= val;
> +	writel_relaxed(reg_val, reg);
> +}
> +
> +static inline void qphy_clrbits(void __iomem *reg, u32 val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = readl_relaxed(reg);
> +	reg_val &= ~val;
> +	writel_relaxed(reg_val, reg);
> +}
> +
> +const struct qmp_phy_init_cfg msm8996_pciephy_init_cfg = {

static?

> +	.type			= PHY_TYPE_PCIE,
> +	.nlanes			= 3,
> +
> +	.phy_init_serdes_tbl	= pciephy_serdes_init_tbl,
> +	.phy_init_serdes_tbl_sz	= ARRAY_SIZE(pciephy_serdes_init_tbl),
> +	.phy_init_tx_tbl	= pciephy_tx_init_tbl,
> +	.phy_init_tx_tbl_sz	= ARRAY_SIZE(pciephy_tx_init_tbl),
> +	.phy_init_rx_tbl	= pciephy_rx_init_tbl,
> +	.phy_init_rx_tbl_sz	= ARRAY_SIZE(pciephy_rx_init_tbl),
> +	.phy_init_pcs_tbl	= pciephy_pcs_init_tbl,
> +	.phy_init_pcs_tbl_sz	= ARRAY_SIZE(pciephy_pcs_init_tbl),
> +	.regs			= pciephy_regs_layout,
> +	.mask_start_ctrl	= (PHY_PCS_START | PHY_PLL_READY_GATE_EN),
> +	.mask_pwr_dn_ctrl	= (PHY_SW_PWRDN | PHY_REFCLK_DRV_DSBL),
> +
> +	.has_phy_com_ctrl	= true,
> +	.has_lane_rst		= true,
> +};
> +
> +const struct qmp_phy_init_cfg msm8996_usb3phy_init_cfg = {

static? Try running sparse.

> +	.type			= PHY_TYPE_USB3,
> +	.nlanes			= 1,
> +
> +	.phy_init_serdes_tbl	= usb3phy_serdes_init_tbl,
> +	.phy_init_serdes_tbl_sz	= ARRAY_SIZE(usb3phy_serdes_init_tbl),
> +	.phy_init_tx_tbl	= usb3phy_tx_init_tbl,
> +	.phy_init_tx_tbl_sz	= ARRAY_SIZE(usb3phy_tx_init_tbl),
> +	.phy_init_rx_tbl	= usb3phy_rx_init_tbl,
> +	.phy_init_rx_tbl_sz	= ARRAY_SIZE(usb3phy_rx_init_tbl),
> +	.phy_init_pcs_tbl	= usb3phy_pcs_init_tbl,
> +	.phy_init_pcs_tbl_sz	= ARRAY_SIZE(usb3phy_pcs_init_tbl),

Do we really need phy_init as a prefix. Could just be serdes_tbl,
tx_tbl, etc?

> +	.regs			= usb3phy_regs_layout,
> +	.mask_start_ctrl	= (PHY_SERDES_START | PHY_PCS_START),
> +	.mask_pwr_dn_ctrl	= PHY_SW_PWRDN,
> +};
> +
> +static void qcom_qmp_phy_configure(void __iomem *base,
> +				unsigned int *regs_layout,
> +				struct qmp_phy_init_tbl init_tbl[],
> +				int init_tbl_sz)

Shorter variable names would make this easier to read.

> +{
> +	int i;
> +
> +	for (i = 0; i < init_tbl_sz; i++) {
> +		if (init_tbl[i].in_layout)
> +			writel_relaxed(init_tbl[i].cfg_val,
> +				base + regs_layout[init_tbl[i].reg_offset]);
> +		else
> +			writel_relaxed(init_tbl[i].cfg_val,
> +				base + init_tbl[i].reg_offset);
> +	}
> +
	const struct qmp_phy_init_tbl *t = tbl;

	for (i = 0; i < num; i++, t++)
   		if (t->in_layout)
   			writel_relaxed(t->val, base + regs[t->offset]);
   		else
   			writel_relaxed(t->val, base + t->offset);

> +	/* flush buffered writes */
> +	mb();
> +}
> +
> +static int qcom_qmp_phy_poweron(struct phy *phy)
> +{
> +	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> +	struct qcom_qmp_phy *qphy = phydesc->qphy;
> +	int ret;
> +
> +	dev_vdbg(&phy->dev, "Powering on QMP phy\n");
> +
> +	ret = regulator_enable(qphy->vdda_phy);
> +	if (ret) {
> +		dev_err(qphy->dev, "%s: vdda-phy enable failed, err=%d\n",
> +				__func__, ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(qphy->vdda_pll);
> +	if (ret) {
> +		dev_err(qphy->dev, "%s: vdda-pll enable failed, err=%d\n",
> +				__func__, ret);
> +		goto err_vdda_pll;
> +	}
> +
> +	if (qphy->vddp_ref_clk) {
> +		ret = regulator_enable(qphy->vddp_ref_clk);
> +		if (ret) {
> +			dev_err(qphy->dev, "%s: vdda-ref-clk enable failed, err=%d\n",
> +					__func__, ret);
> +			goto err_vddp_refclk;
> +		}
> +	}
> +
> +	clk_prepare_enable(qphy->ref_clk_src);
> +	clk_prepare_enable(qphy->ref_clk);
> +	clk_prepare_enable(phydesc->pipe_clk);

And if these fail?

> +
> +	return 0;
> +
> +err_vddp_refclk:
> +	regulator_disable(qphy->vdda_pll);
> +err_vdda_pll:
> +	regulator_disable(qphy->vdda_phy);
> +	return ret;
> +}
> +
> +static int qcom_qmp_phy_poweroff(struct phy *phy)
> +{
> +	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> +	struct qcom_qmp_phy *qphy = phydesc->qphy;
> +
> +	clk_disable_unprepare(qphy->ref_clk_src);
> +	clk_disable_unprepare(qphy->ref_clk);
> +	clk_disable_unprepare(phydesc->pipe_clk);
> +
> +	if (qphy->vddp_ref_clk)
> +		regulator_disable(qphy->vddp_ref_clk);
> +
> +	regulator_disable(qphy->vdda_pll);
> +	regulator_disable(qphy->vdda_phy);
> +
> +	return 0;
> +}
> +
> +static int qcom_qmp_phy_is_ready(struct qcom_qmp_phy *qphy,
> +				void __iomem *pcs_status, u32 mask)
> +{
> +	unsigned int init_timeout;
> +
> +	init_timeout = PHY_READY_TIMEOUT_COUNT;
> +	do {
> +		if (readl_relaxed(pcs_status) & mask)
> +			break;
> +
> +		usleep_range(REFCLK_STABILIZATION_DELAY_US_MIN,
> +				 REFCLK_STABILIZATION_DELAY_US_MAX);
> +	} while (--init_timeout);

readl_poll_timeout?

> +
> +	if (!init_timeout)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
[...]
> +
> +/* PHY Initialization */
> +static int qcom_qmp_phy_init(struct phy *phy)
> +{
> +	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> +	struct qcom_qmp_phy *qphy = phydesc->qphy;
> +	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> +	void __iomem *tx = phydesc->tx;
> +	void __iomem *rx = phydesc->rx;
> +	void __iomem *pcs = phydesc->pcs;
> +	int ret;
> +
> +	dev_vdbg(qphy->dev, "Initializing QMP phy\n");
> +
> +	/* enable interface clocks to program phy */
> +	clk_prepare_enable(qphy->aux_clk);
> +	clk_prepare_enable(qphy->cfg_ahb_clk);
> +
> +	ret = qcom_qmp_phy_com_init(qphy);
> +	if (ret)
> +		goto err;
> +
> +	if (phydesc->lane_rst) {
> +		ret = reset_control_deassert(phydesc->lane_rst);
> +		if (ret) {
> +			dev_err(qphy->dev, "lane<%d> reset deassert failed\n",

Drop the brackets?

> +					phydesc->index);
> +			goto err_lane_rst;
> +		}
> +	}
> +
> +	/* Tx, Rx, and PCS configurations */
> +	qcom_qmp_phy_configure(tx, cfg->regs, cfg->phy_init_tx_tbl,
> +				cfg->phy_init_tx_tbl_sz);
> +	qcom_qmp_phy_configure(rx, cfg->regs, cfg->phy_init_rx_tbl,
> +				cfg->phy_init_rx_tbl_sz);
> +	qcom_qmp_phy_configure(pcs, cfg->regs, cfg->phy_init_pcs_tbl,
> +				cfg->phy_init_pcs_tbl_sz);
> +
> +	/*
> +	 * Pull out PHY from POWER DOWN state:
> +	 * This is active low enable signal to power-down PHY.
> +	 */
> +	qphy_setbits(pcs + QPHY_POWER_DOWN_CONTROL, cfg->mask_pwr_dn_ctrl);
> +	/* XXX: 10 us delay; given in PCIE phy programming guide only */

Is this a todo?

> +	usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
> +
> +	/* start SerDes and Phy-Coding-Sublayer */
> +	qphy_setbits(pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
> +
> +	/* Pull PHY out of reset state */
> +	qphy_clrbits(pcs + QPHY_SW_RESET, PHY_SW_RESET);
> +	/* Make sure that above writes are completed */
> +	mb();
> +
> +	ret = qcom_qmp_phy_is_ready(qphy, pcs +
> +					cfg->regs[QPHY_PCS_READY_STATUS],
> +					MASK_PHYSTATUS);
> +	if (ret) {
> +		dev_err(qphy->dev, "phy initialization timed-out\n");
> +		goto err_pcs_ready;
> +	}
> +
> +	return 0;
> +
> +err_pcs_ready:
> +	if (phydesc->lane_rst)
> +		reset_control_assert(phydesc->lane_rst);
> +err_lane_rst:
> +	qcom_qmp_phy_com_exit(qphy);
> +err:
> +	clk_disable_unprepare(qphy->cfg_ahb_clk);
> +	clk_disable_unprepare(qphy->aux_clk);
> +	return ret;
> +}
> +
> +static int qcom_qmp_phy_exit(struct phy *phy)
> +{
> +	struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> +	struct qcom_qmp_phy *qphy = phydesc->qphy;
> +	const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> +
> +	/* PHY reset */
> +	qphy_setbits(phydesc->pcs + QPHY_SW_RESET, PHY_SW_RESET);
> +
> +	/* stop SerDes and Phy-Coding-Sublayer */
> +	qphy_clrbits(phydesc->pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
> +
> +	/* Put PHY into POWER DOWN state: active low */
> +	qphy_clrbits(phydesc->pcs + QPHY_POWER_DOWN_CONTROL,
> +			cfg->mask_pwr_dn_ctrl);
> +
> +	/* Make sure that above writes are completed */
> +	mb();
> +
> +	if (phydesc->lane_rst)
> +		reset_control_assert(phydesc->lane_rst);
> +
> +	qcom_qmp_phy_com_exit(qphy);
> +
> +	clk_disable_unprepare(qphy->aux_clk);
> +	clk_disable_unprepare(qphy->cfg_ahb_clk);
> +
> +	return 0;
> +}
> +
> +
> +static int qcom_qmp_phy_regulator_init(struct device *dev)
> +{
> +	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> +	int ret;
> +
> +	qphy->vdda_phy = devm_regulator_get(dev, "vdda-phy");
> +	if (IS_ERR(qphy->vdda_phy)) {
> +		ret = PTR_ERR(qphy->vdda_phy);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get vdda-phy, %d\n", ret);
> +		return ret;
> +	}
> +
> +	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
> +	if (IS_ERR(qphy->vdda_pll)) {
> +		ret = PTR_ERR(qphy->vdda_pll);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get vdda-pll, %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* optional regulator */
> +	qphy->vddp_ref_clk = devm_regulator_get(dev, "vddp-ref-clk");
> +	if (IS_ERR(qphy->vddp_ref_clk)) {
> +		ret = PTR_ERR(qphy->vddp_ref_clk);
> +		if (ret != -EPROBE_DEFER) {
> +			dev_dbg(dev, "failed to get vddp-ref-clk, %d\n", ret);
> +			qphy->vddp_ref_clk = NULL;
> +		} else {
> +			return ret;
> +		}
> +	}

Regulator framework should insert a dummy regulator if this is
missing in DT. So we shouldn't need to do anything complicated
here right?

> +
> +	return 0;
> +}
> +
> +static int qcom_qmp_phy_clk_init(struct device *dev)
> +{
> +	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> +	int ret;
> +
> +	qphy->aux_clk = devm_clk_get(dev, "aux");
> +	if (IS_ERR(qphy->aux_clk)) {
> +		ret = PTR_ERR(qphy->aux_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get aux_clk\n");
> +		return ret;
> +	}
> +
> +	qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb");
> +	if (IS_ERR(qphy->cfg_ahb_clk)) {
> +		ret = PTR_ERR(qphy->cfg_ahb_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get cfg_ahb_clk\n");
> +		return ret;
> +	}
> +
> +	qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
> +	if (IS_ERR(qphy->ref_clk_src)) {
> +		ret = PTR_ERR(qphy->ref_clk_src);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get ref_clk_src\n");
> +		return ret;
> +	}
> +
> +	qphy->ref_clk = devm_clk_get(dev, "ref_clk");
> +	if (IS_ERR(qphy->ref_clk)) {
> +		ret = PTR_ERR(qphy->ref_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get ref_clk\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct phy *qcom_qmp_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> +	int i;
> +
> +	if (WARN_ON(args->args[0] >= qphy->cfg->nlanes))
> +		return ERR_PTR(-ENODEV);
> +
> +	for (i = 0; i < qphy->cfg->nlanes; i++) {
> +		if (qphy->phys[i]->index == args->args[0])
> +			break;
> +	}

Drop the braces please.

> +
> +	if (i == qphy->cfg->nlanes)
> +		return ERR_PTR(-ENODEV);
> +
> +	return qphy->phys[i]->phy;

Could be simplified:


	for (i = 0; i < qphy->cfg->nlanes; i++)
		if (qphy->phys[i]->index == args->args[0])
			return qphy->phys[i]->phy;

	return ERR_PTR(-ENODEV);


Also, isn't i == phys[i]->index so this could be a direct lookup?

> +}
> +
> +static const struct phy_ops qcom_qmp_phy_gen_ops = {
> +	.init		= qcom_qmp_phy_init,
> +	.exit		= qcom_qmp_phy_exit,
> +	.power_on	= qcom_qmp_phy_poweron,
> +	.power_off	= qcom_qmp_phy_poweroff,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id qcom_qmp_phy_of_match_table[] = {
> +	{
> +		.compatible = "qcom,msm8996-qmp-pcie-phy",
> +		.data = &msm8996_pciephy_init_cfg,
> +	}, {
> +		.compatible = "qcom,msm8996-qmp-usb3-phy",
> +		.data = &msm8996_usb3phy_init_cfg,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qmp_phy_of_match_table);
> +
> +static int qcom_qmp_phy_probe(struct platform_device *pdev)
> +{
> +	struct qcom_qmp_phy *qphy;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct resource *res;
> +	const struct of_device_id *match;
> +	void __iomem *base;
> +	int ret = 0;

Shouldn't need a default assignment here.

> +	int id;
> +
> +	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> +	if (!qphy)
> +		return -ENOMEM;
> +
> +	qphy->dev = dev;
> +	dev_set_drvdata(dev, qphy);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	/* per PHY serdes; usually located at base address */
> +	qphy->serdes = base;
> +
> +	mutex_init(&qphy->phy_mutex);
> +
> +	/* Get the specific init parameters of QMP phy */
> +	match = of_match_node(qcom_qmp_phy_of_match_table, dev->of_node);
> +	qphy->cfg = match->data;
> +
> +	ret = qcom_qmp_phy_clk_init(dev);
> +	if (ret) {
> +		dev_err(dev, "clock init failed\n");
> +		return ret;
> +	}
> +
> +	ret = qcom_qmp_phy_regulator_init(dev);
> +	if (ret) {
> +		dev_err(dev, "regulator init failed\n");
> +		return ret;
> +	}
> +
> +	qphy->phy_rst = devm_reset_control_get(dev, "phy");
> +	if (IS_ERR(qphy->phy_rst)) {
> +		dev_err(dev, "failed to get phy core reset\n");
> +		return PTR_ERR(qphy->phy_rst);
> +	}
> +
> +	qphy->phycom_rst = devm_reset_control_get(dev, "common");
> +	if (IS_ERR(qphy->phycom_rst)) {
> +		dev_err(dev, "failed to get phy common reset\n");
> +		return PTR_ERR(qphy->phycom_rst);
> +	}
> +
> +	qphy->phycfg_rst = devm_reset_control_get(dev, "cfg");
> +	if (IS_ERR(qphy->phycfg_rst)) {
> +		dev_dbg(dev, "failed to get phy ahb cfg reset\n");
> +		qphy->phycfg_rst = NULL;
> +	}
> +
> +	qphy->phys = devm_kcalloc(dev, qphy->cfg->nlanes,
> +					sizeof(*qphy->phys), GFP_KERNEL);
> +	if (!qphy->phys)
> +		return -ENOMEM;
> +
> +	for (id = 0; id < qphy->cfg->nlanes; id++) {
> +		struct phy *generic_phy;
> +		struct qmp_phy_desc *phy_desc;
> +		char prop_name[MAX_PROP_NAME];
> +		unsigned int lane_offsets[3];
> +
> +		/* mem resources from index 1 to N for N number of lanes */
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, id + 1);
> +		base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(base))
> +			return PTR_ERR(base);
> +
> +		phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
> +		if (!phy_desc)
> +			return -ENOMEM;
> +
> +		/*
> +		 * read offsets to Tx, Rx, and PCS blocks into a u32 array:
> +		 *  ------------------------------------
> +		 * | tx offset | rx offset | pcs offset |
> +		 *  ------------------------------------
> +		 */
> +		ret = of_property_read_u32_array(dev->of_node, "lane-offsets",
> +							   lane_offsets, 3);
> +		if (ret) {
> +			dev_err(dev,
> +				"failed to get tx/rx/pcs offsets for lane%d\n",
> +				id);
> +				return ret;
> +		}
> +
> +		phy_desc->tx = base + lane_offsets[0];
> +		phy_desc->rx = base + lane_offsets[1];
> +		phy_desc->pcs = base + lane_offsets[2];
> +
> +		/*
> +		 * Get PHY's Pipe clock, if any; USB3 and PCIe are PIPE3
> +		 * based phys, so they essentially have pipe clock. So,
> +		 * we return error in case phy is USB3 or PIPE type.
> +		 * Otherwise, we initialize pipe clock to NULL for
> +		 * all phys that don't need this.
> +		 */
> +		memset(&prop_name, 0, sizeof(prop_name));
> +		snprintf(prop_name, MAX_PROP_NAME, "pipe%d", id);
> +		phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
> +		if (IS_ERR(phy_desc->pipe_clk)) {
> +			if (qphy->cfg->type == PHY_TYPE_PCIE ||
> +			    qphy->cfg->type == PHY_TYPE_USB3) {
> +				ret = PTR_ERR(phy_desc->pipe_clk);
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(dev,
> +					"failed to get lane%d pipe_clk\n", id);
> +				return ret;
> +			} else {
> +				phy_desc->pipe_clk = NULL;
> +			}

Drop the else part.

> +		}
> +
> +		/* Get lane reset, if any */
> +		if (qphy->cfg->has_lane_rst) {
> +			memset(&prop_name, 0, sizeof(prop_name));
> +			snprintf(prop_name, MAX_PROP_NAME, "lane%d", id);
> +			phy_desc->lane_rst = devm_reset_control_get(dev,
> +								    prop_name);
> +			if (IS_ERR(phy_desc->lane_rst)) {
> +				dev_err(dev, "failed to get lane%d reset\n",
> +					id);
> +				return PTR_ERR(phy_desc->lane_rst);
> +			}
> +		}
> +
> +		generic_phy = devm_phy_create(dev, NULL, &qcom_qmp_phy_gen_ops);
> +		if (IS_ERR(generic_phy)) {
> +			ret = PTR_ERR(generic_phy);
> +			dev_err(dev, "failed to create qphy %d\n", ret);
> +			return ret;
> +		}
> +
> +		phy_desc->phy = generic_phy;
> +		phy_desc->index = id;
> +		phy_desc->qphy = qphy;
> +		phy_set_drvdata(generic_phy, phy_desc);
> +		qphy->phys[id] = phy_desc;

Probably should make the above part of this loop a function that
returns a phy for each lane (or an  error code on failure). This
probe function is long.

> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev, qcom_qmp_phy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		dev_err(dev, "failed to register qphy %d\n", ret);
> +	}
> +
> +	return ret;
> +}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
  2016-11-28 14:19   ` Rob Herring
@ 2016-11-29  5:20     ` Vivek Gautam
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-11-29  5:20 UTC (permalink / raw)
  To: Rob Herring
  Cc: kishon, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, Stephen Boyd, linux-arm-msm

Hi Rob,


On Mon, Nov 28, 2016 at 7:49 PM, Rob Herring <robh@kernel.org> wrote:

Thanks for reviewing the patch.

> On Tue, Nov 22, 2016 at 05:32:40PM +0530, Vivek Gautam wrote:
>> Qualcomm chipsets have QUSB2 phy controller that provides
>> HighSpeed functionality for DWC3 controller.
>> Adding dt binding information for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>
>> Changes since v1:
>>  - New patch, forked out of the original driver patch:
>>    "phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips"
>>  - Updated dt bindings to remove 'hstx-trim-bit-offset' and
>>    'hstx-trim-bit-len' bindings.
>>
>>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt     | 55 ++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> new file mode 100644
>> index 0000000..38c8b30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -0,0 +1,55 @@
>> +Qualcomm QUSB2 phy controller
>> +=============================
>> +
>> +QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>> +
>> +Required properties:
>> + - compatible: compatible list, contains "qcom,msm8996-qusb2-phy".
>> + - reg: offset and length of the PHY register set.
>> + - #phy-cells: must be 0.
>> +
>> + - clocks: a list of phandles and clock-specifier pairs,
>> +        one for each entry in clock-names.
>> + - clock-names: must be "cfg_ahb" for phy config clock,
>> +                     "ref_clk" for 19.2 MHz ref clk,
>> +                     "ref_clk_src" reference clock source.
>> +                     "iface" for phy interface clock (Optional).
>> +
>> + - vdd-phy-supply: Phandle to a regulator supply to PHY core block.
>> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
>> + - vdda-phy-dpdm: Phandle to 3.1V regulator supply to Dp/Dm port signals.
>
> Needs '-supply'

sure, will add.

>
>> +
>> + - resets: a list of phandles and reset controller specifier pairs,
>> +        one for each entry in reset-names.
>> + - reset-names: must be "phy" for reset of phy block.
>> +
>> +Optional properties:
>> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
>> +             tuning parameters for qusb2 phy, one for each entry
>> +             in nvmem-cell-names.
>> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
>> +                  HS Tx trim value.
>> +
>> + - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
>> +
>> +Example:
>> +     hsphy: qusb2phy@7411000 {
>
> usb-phy@...

Or may be just 'phy' for the node name, and then label could be 'usb_hs_phy'?

[...]


Thanks
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy
  2016-11-28 22:49   ` Stephen Boyd
@ 2016-11-29  5:22     ` Vivek Gautam
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-11-29  5:22 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi Stephen,

On Tue, Nov 29, 2016 at 4:19 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

Thanks for reviewing the patch-series.

> On 11/22, Vivek Gautam wrote:
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> new file mode 100644
>> index 0000000..38c8b30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -0,0 +1,55 @@
>> +Optional properties:
>> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
>> +             tuning parameters for qusb2 phy, one for each entry
>> +             in nvmem-cell-names.
>> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
>
> Do we really need efuse in the name? Seems redundant given this
> is already an nvmem.

Correct, we don't need 'efuse' in the name. Thanks for pointing out.


Best Regards
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-28 22:55   ` Stephen Boyd
@ 2016-11-29  5:25     ` Vivek Gautam
  2016-11-30 19:12       ` Stephen Boyd
  2016-12-12 16:40     ` Vivek Gautam
  1 sibling, 1 reply; 21+ messages in thread
From: Vivek Gautam @ 2016-11-29  5:25 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi,


On Tue, Nov 29, 2016 at 4:25 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/22, Vivek Gautam wrote:
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> new file mode 100644
>> index 0000000..ffb173b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -0,0 +1,74 @@
>> +Qualcomm QMP PHY
>> +----------------
>> +
>> +QMP phy controller supports physical layer functionality for a number of
>> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
>> +
>> +Required properties:
>> + - compatible: compatible list, contains:
>> +            "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
>> +            "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
>> + - reg: list of offset and length pair of the PHY register sets.
>> +     at index 0: offset and length of register set for PHY common
>> +                 serdes block.
>> +     from index 1 - N: offset and length of register set for each lane,
>> +                       for N number of phy lanes (ports).
>> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
>> + - #phy-cells: must be 1
>> +    - Cell after phy phandle should be the port (lane) number.
>> + - clocks: a list of phandles and clock-specifier pairs,
>> +        one for each entry in clock-names.
>> + - clock-names: must be "cfg_ahb" for phy config clock,
>> +                     "aux" for phy aux clock,
>> +                     "ref_clk" for 19.2 MHz ref clk,
>> +                     "ref_clk_src" for reference clock source,
>
> We typically leave "clk" out of clk names because it's redundant.

Right, will drop 'clk' from these names.

>
>> +                     "pipe<port-number>" for pipe clock specific to
>> +                     each port/lane (Optional).
>
> The pipe clocks are orphaned right now. We should add an output
> clock from the phy to go into the controller and back into the
> phy if I recall correctly. The phy should be a clock provider
> itself so it can output the pipe clock source into GCC and back
> into the phy and controller.
>
>> + - resets: a list of phandles and reset controller specifier pairs,
>> +        one for each entry in reset-names.
>> + - reset-names: must be "phy" for reset of phy block,
>> +                     "common" for phy common block reset,
>> +                     "cfg" for phy's ahb cfg block reset (Optional).
>> +                     "port<port-number>" for reset specific to
>> +                     each port/lane (Optional).
>> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
>> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
>> +
>> +Optional properties:
>> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
>> +                     pll block.
>> +
>> +Example:
>> +     pcie_phy: pciephy@34000 {
>
> pcie-phy or just phy as the node name?

How about just 'phy'? The label pcie_phy anyways explains the use.


Thanks
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-29  5:25     ` Vivek Gautam
@ 2016-11-30 19:12       ` Stephen Boyd
  0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2016-11-30 19:12 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

On 11/29, Vivek Gautam wrote:
> On Tue, Nov 29, 2016 at 4:25 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 11/22, Vivek Gautam wrote:
> >> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> >> new file mode 100644
> >> index 0000000..ffb173b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> >> @@ -0,0 +1,74 @@
> 
> >
> >> +                     "pipe<port-number>" for pipe clock specific to
> >> +                     each port/lane (Optional).
> >
> > The pipe clocks are orphaned right now. We should add an output
> > clock from the phy to go into the controller and back into the
> > phy if I recall correctly. The phy should be a clock provider
> > itself so it can output the pipe clock source into GCC and back
> > into the phy and controller.
> >
> >> + - resets: a list of phandles and reset controller specifier pairs,
> >> +        one for each entry in reset-names.
> >> + - reset-names: must be "phy" for reset of phy block,
> >> +                     "common" for phy common block reset,
> >> +                     "cfg" for phy's ahb cfg block reset (Optional).
> >> +                     "port<port-number>" for reset specific to
> >> +                     each port/lane (Optional).
> >> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
> >> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> >> +
> >> +Optional properties:
> >> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
> >> +                     pll block.
> >> +
> >> +Example:
> >> +     pcie_phy: pciephy@34000 {
> >
> > pcie-phy or just phy as the node name?
> 
> How about just 'phy'? The label pcie_phy anyways explains the use.
> 
> 

phy is a great color choice for the shed.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  2016-11-28 23:14   ` Stephen Boyd
@ 2016-12-01  8:42     ` Vivek Gautam
  2016-12-02 18:47       ` Stephen Boyd
  0 siblings, 1 reply; 21+ messages in thread
From: Vivek Gautam @ 2016-12-01  8:42 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi Stephen,


On Tue, Nov 29, 2016 at 4:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/22, Vivek Gautam wrote:
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index e8eb7f2..f1dcec1 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -430,6 +430,17 @@ config PHY_STIH407_USB
>>         Enable this support to enable the picoPHY device used by USB2
>>         and USB3 controllers on STMicroelectronics STiH407 SoC families.
>>
>> +config PHY_QCOM_QUSB2
>> +     tristate "Qualcomm QUSB2 PHY Driver"
>> +     depends on OF && (ARCH_QCOM || COMPILE_TEST)
>> +     select GENERIC_PHY
>> +     select RESET_CONTROLLER
>
> This shouldn't be necessary. We only need to select it if we're
> providing resets.

Ok, will drop this.

>
>> +     help
>> +       Enable this to support the HighSpeed QUSB2 PHY transceiver for USB
>> +       controllers on Qualcomm chips. This driver supports the high-speed
>> +       PHY which is usually paired with either the ChipIdea or Synopsys DWC3
>> +       USB IPs on MSM SOCs.
>> +
>>  config PHY_QCOM_UFS
>>       tristate "Qualcomm UFS PHY driver"
>>       depends on OF && ARCH_QCOM
>> diff --git a/drivers/phy/phy-qcom-qusb2.c b/drivers/phy/phy-qcom-qusb2.c
>> new file mode 100644
>> index 0000000..d3f9657
>> --- /dev/null
>> +++ b/drivers/phy/phy-qcom-qusb2.c
>> @@ -0,0 +1,549 @@
>> +/*
>> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#define QUSB2PHY_PLL_TEST            0x04
>> +#define CLK_REF_SEL                  BIT(7)
>> +
>> +#define QUSB2PHY_PLL_TUNE            0x08
>> +#define QUSB2PHY_PLL_USER_CTL1               0x0c
>> +#define QUSB2PHY_PLL_USER_CTL2               0x10
>> +#define QUSB2PHY_PLL_AUTOPGM_CTL1    0x1c
>> +#define QUSB2PHY_PLL_PWR_CTRL                0x18
>> +
>> +#define QUSB2PHY_PLL_STATUS          0x38
>> +#define PLL_LOCKED                   BIT(5)
>> +
>> +#define QUSB2PHY_PORT_TUNE1             0x80
>> +#define QUSB2PHY_PORT_TUNE2             0x84
>> +#define QUSB2PHY_PORT_TUNE3             0x88
>> +#define QUSB2PHY_PORT_TUNE4             0x8C
>> +#define QUSB2PHY_PORT_TUNE5          0x90
>> +#define QUSB2PHY_PORT_TEST2          0x9c
>
> Please use lowercase or uppercase consistently (I'd prefer
> lowercase).

Sure, lowercase.

>
>> +
>> +#define QUSB2PHY_PORT_POWERDOWN              0xB4
>> +#define CLAMP_N_EN                   BIT(5)
>> +#define FREEZIO_N                    BIT(1)
>> +#define POWER_DOWN                   BIT(0)
>> +
>> +#define QUSB2PHY_REFCLK_ENABLE               BIT(0)
>> +
>> +#define PHY_CLK_SCHEME_SEL           BIT(0)
>> +
>> +struct qusb2_phy_init_tbl {
>> +     unsigned int reg_offset;
>> +     unsigned int cfg_val;
>> +};
>> +#define QCOM_QUSB2_PHY_INIT_CFG(reg, val) \
>> +     {                               \
>> +             .reg_offset = reg,      \
>> +             .cfg_val = val,         \
>> +     }
>> +
>> +static struct qusb2_phy_init_tbl msm8996_phy_init_tbl[] = {
>
> const?

argh! shouldn't have missed these 'const' and 'static' assignments.
will update for all instances.

>
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xF8),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xB3),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xC0),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9F),
>> +     QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
>> +};
>> +
>> +struct qusb2_phy_init_cfg {
>> +     struct qusb2_phy_init_tbl *phy_init_tbl;
>> +     int phy_init_tbl_sz;
>> +     /* offset to PHY_CLK_SCHEME register in TCSR map. */
>> +     unsigned int clk_scheme_offset;
>> +};
>> +
>> +const struct qusb2_phy_init_cfg msm8996_phy_init_cfg = {
>
> static?
>
>> +     .phy_init_tbl = msm8996_phy_init_tbl,
>> +     .phy_init_tbl_sz = ARRAY_SIZE(msm8996_phy_init_tbl),
>> +};
>> +
>> +/**
>> + * struct qusb2_phy: Structure holding qusb2 phy attributes.
>> + *
>> + * @phy: pointer to generic phy.
>> + * @base: pointer to iomapped memory space for qubs2 phy.
>> + *
>> + * @cfg_ahb_clk: pointer to AHB2PHY interface clock.
>> + * @ref_clk: pointer to reference clock.
>> + * @ref_clk_src: pointer to source to reference clock.
>> + * @iface_src: pointer to phy interface clock.
>> + *
>> + * @phy_reset: Pointer to phy reset control
>> + *
>> + * @vdda_phy: vdd supply to the phy core block.
>> + * @vdda_pll: 1.8V vdd supply to ref_clk block.
>> + * @vdda_phy_dpdm: 3.1V vdd supply to Dp/Dm port signals.
>> + * @tcsr: pointer to TCSR syscon register map.
>
> Drop all the full stops on these comments please.

sure.

>
>> + *
>> + * @cfg: phy initialization config data
>> + * @has_se_clk_scheme: indicate if PHY has Single-ended ref clock scheme
>
> Why is single capitalized?

ok, 'single-ended'

>
>> + */
>> +struct qusb2_phy {
>> +     struct phy *phy;
>> +     void __iomem *base;
>> +
>> +     struct clk *cfg_ahb_clk;
>> +     struct clk *ref_clk;
>> +     struct clk *ref_clk_src;
>> +     struct clk *iface_clk;
>> +
>> +     struct reset_control *phy_reset;
>> +
>> +     struct regulator *vdd_phy;
>> +     struct regulator *vdda_pll;
>> +     struct regulator *vdda_phy_dpdm;
>> +
>> +     struct regmap *tcsr;
>> +
>> +     const struct qusb2_phy_init_cfg *cfg;
>> +     bool has_se_clk_scheme;
>> +};
>> +
>> +static inline void qusb2_setbits(void __iomem *reg, u32 val)
>> +{
>> +     u32 reg_val;
>> +
>> +     reg_val = readl_relaxed(reg);
>> +     reg_val |= val;
>> +     writel_relaxed(reg_val, reg);
>> +
>> +     /* Ensure above write is completed */
>> +     mb();
>> +}
>> +
>> +static inline void qusb2_clrbits(void __iomem *reg, u32 val)
>> +{
>> +     u32 reg_val;
>> +
>> +     reg_val = readl_relaxed(reg);
>> +     reg_val &= ~val;
>> +     writel_relaxed(reg_val, reg);
>> +
>> +     /* Ensure above write is completed */
>> +     mb();
>> +}
>> +
>> +static void qcom_qusb2_phy_configure(void __iomem *base,
>> +                             struct qusb2_phy_init_tbl init_tbl[],
>> +                             int init_tbl_sz)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < init_tbl_sz; i++) {
>> +             writel_relaxed(init_tbl[i].cfg_val,
>> +                             base + init_tbl[i].reg_offset);
>> +     }
>> +
>> +     /* flush buffered writes */
>> +     mb();
>> +}
>> +
>> +static void qusb2_phy_enable_clocks(struct qusb2_phy *qphy, bool on)
>
> Maybe s/enable/toggle/ because we're not always enabling.

yea, toggle is better; will modify.

>
>> +{
>> +     if (on) {
>> +             clk_prepare_enable(qphy->iface_clk);
>> +             clk_prepare_enable(qphy->ref_clk_src);
>> +     } else {
>> +             clk_disable_unprepare(qphy->ref_clk_src);
>> +             clk_disable_unprepare(qphy->iface_clk);
>> +     }
>> +
>> +     dev_vdbg(&qphy->phy->dev, "%s(): clocks enabled\n", __func__);
>
> Heh or disabled!

yup, a check for 'on' and relevant string - enabled/disabled. will do it.

>
>> +}
>> +
>> +static int qusb2_phy_enable_power(struct qusb2_phy *qphy, bool on)
>
> Maybe s/enable/toggle/ because we're not always enabling.

Yea, will update it to 'toggle'.

>
>> +{
>> +     int ret;
>> +     struct device *dev = &qphy->phy->dev;
>> +
>> +     if (!on)
>> +             goto disable_vdda_phy_dpdm;
>> +
>> +     ret = regulator_enable(qphy->vdd_phy);
>> +     if (ret) {
>> +             dev_err(dev, "Unable to enable vdd-phy:%d\n", ret);
>> +             goto err_vdd_phy;
>> +     }
>> +
>> +     ret = regulator_enable(qphy->vdda_pll);
>> +     if (ret) {
>> +             dev_err(dev, "Unable to enable vdda-pll:%d\n", ret);
>> +             goto disable_vdd_phy;
>> +     }
>> +
>> +     ret = regulator_enable(qphy->vdda_phy_dpdm);
>> +     if (ret) {
>> +             dev_err(dev, "Unable to enable vdda-phy-dpdm:%d\n", ret);
>> +             goto disable_vdda_pll;
>> +     }
>> +
>> +     dev_vdbg(dev, "%s() regulators are turned on.\n", __func__);
>
> Drop the full stop please.

ok.

>
>> +
>> +     return ret;
>> +
>> +disable_vdda_phy_dpdm:
>> +     regulator_disable(qphy->vdda_phy_dpdm);
>> +disable_vdda_pll:
>> +     regulator_disable(qphy->vdda_pll);
>> +disable_vdd_phy:
>> +     regulator_disable(qphy->vdd_phy);
>> +err_vdd_phy:
>> +     dev_vdbg(dev, "%s() regulators are turned off.\n", __func__);
>> +     return ret;
>> +}
>> +
>> +/*
>> + * Fetches HS Tx tuning value from e-fuse and sets QUSB2PHY_PORT_TUNE2
>> + * register.
>> + * For any error case, skip setting the value and use the default value.
>> + */
>> +static int qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
>> +{
>> +     struct device *dev = &qphy->phy->dev;
>> +     struct nvmem_cell *cell;
>> +     ssize_t len;
>> +     u8 *val;
>> +
>> +     /*
>> +      * Read EFUSE register having TUNE2 parameter's high nibble.
>> +      * If efuse register shows value as 0x0, or if we fail to find
>> +      * a valid efuse register settings, then use default value
>> +      * as 0xB for high nibble that we have already set while
>> +      * configuring phy.
>> +      */
>> +     cell = devm_nvmem_cell_get(dev, "tune2_hstx_trim_efuse");
>> +     if (IS_ERR(cell)) {
>> +             if (PTR_ERR(cell) == -EPROBE_DEFER)
>> +                     return PTR_ERR(cell);
>> +             goto skip;
>
> Why do we get the nvmem cell here? Wouldn't we want to get it
> during probe? Returning probe defer here during init would be
> odd.

Yea, my bad. This should be moved to probe().

>
>> +     }
>> +
>> +     /*
>> +      * we need to read only one byte here, since the required
>> +      * parameter value fits in one nibble
>> +      */
>> +     val = (u8 *)nvmem_cell_read(cell, &len);
>
> Shouldn't need the cast here. Also it would be nice if
> nvmem_cell_read() didn't require a second argument if we don't
> care for it. We should update the API to allow NULL there.

Will remove the u8 pointer cast.

Correct, it makes sense to allow the length pointer to be passed as NULL.
We don't care about this length. Will update the nvmem API, to allow this.

Also, we should add a check for 'cell' as well. This pointer can be
NULL, and the first thing that  nvmem_cell_read does is - deference
the pointer 'cell'

>
>> +     if (!IS_ERR(val)) {
>> +             /* Fused TUNE2 value is the higher nibble only */
>> +             qusb2_setbits(qphy->base + QUSB2PHY_PORT_TUNE2,
>> +                                                     val[0] << 0x4);
>> +     } else {
>> +             dev_dbg(dev, "failed reading hs-tx trim value: %ld\n",
>> +                                                     PTR_ERR(val));
>> +     }
>> +
>> +skip:
>> +     return 0;
>> +}
>> +
> [...]
>> +
>> +static int qusb2_phy_init(struct phy *phy)
>> +{
>> +     struct qusb2_phy *qphy = phy_get_drvdata(phy);
>> +     unsigned int reset_val;
>> +     unsigned int clk_scheme;
>> +     int ret;
>> +
>> +     dev_vdbg(&phy->dev, "Initializing QUSB2 phy\n");
>> +
>> +     /* enable ahb interface clock to program phy */
>> +     clk_prepare_enable(qphy->cfg_ahb_clk);
>
> What if that fails?

Yea, will add the necessary checks for failure here and in the rest of the patch
wherever necessary.

>
>> +
>> +     /* Perform phy reset */
>> +     ret = reset_control_assert(qphy->phy_reset);
>> +     if (ret) {
>> +             dev_err(&phy->dev, "Failed to assert phy_reset\n");
>> +             return ret;
>> +     }
>> +     /* 100 us delay to keep PHY in reset mode */
>> +     usleep_range(100, 150);
>> +     ret = reset_control_deassert(qphy->phy_reset);
>> +     if (ret) {
>> +             dev_err(&phy->dev, "Failed to de-assert phy_reset\n");
>> +             return ret;
>> +     }
>> +
>> +     /* Disable the PHY */
>> +     qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
>> +                     CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
>> +
>> +     /* save reset value to override based on clk scheme */
>> +     reset_val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST);
>> +
>> +     qcom_qusb2_phy_configure(qphy->base, qphy->cfg->phy_init_tbl,
>> +                             qphy->cfg->phy_init_tbl_sz);
>> +
>> +     /* Check for efuse value for tuning the PHY */
>> +     ret = qusb2_phy_set_tune2_param(qphy);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Enable the PHY */
>> +     qusb2_clrbits(qphy->base + QUSB2PHY_PORT_POWERDOWN, POWER_DOWN);
>> +
>> +     /* Require to get phy pll lock successfully */
>> +     usleep_range(150, 160);
>> +
>> +     /* Default is Single-ended clock on msm8996 */
>> +     qphy->has_se_clk_scheme = true;
>> +     /*
>> +      * read TCSR_PHY_CLK_SCHEME register to check if Single-ended
>
> Capital Single again?

will use lowercase.

>
>> +      * clock scheme is selected. If yes, then disable differential
>> +      * ref_clk and use single-ended clock, otherwise use differential
>> +      * ref_clk only.
>> +      */
>> +     if (qphy->tcsr) {
>> +             ret = regmap_read(qphy->tcsr, qphy->cfg->clk_scheme_offset,
>> +                                                     &clk_scheme);
>> +             /* is it a differential clock scheme ? */
>> +             if (!(clk_scheme & PHY_CLK_SCHEME_SEL)) {
>> +                     dev_vdbg(&phy->dev, "%s: select differential clk src\n",
>> +                                                             __func__);
>> +                     qphy->has_se_clk_scheme = false;
>> +             } else {
>> +                     dev_vdbg(&phy->dev, "%s: select single-ended clk src\n",
>> +                                                             __func__);
>> +             }
>> +     }
>> +
>> +     if (!qphy->has_se_clk_scheme) {
>> +             reset_val &= ~CLK_REF_SEL;
>> +             clk_prepare_enable(qphy->ref_clk);
>
> And if that fails?

will add the check.

>
>> +     } else {
>> +             reset_val |= CLK_REF_SEL;
>> +     }
>> +
>> +     writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
>> +
>> +     /* Make sure that above write is completed to get PLL source clock */
>> +     wmb();
>> +
>> +     /* Required to get PHY PLL lock successfully */
>> +     usleep_range(100, 110);
>> +
>> +     if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
>> +                                     PLL_LOCKED)) {
>> +             dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
>> +                     readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));
>
> Would be pretty funny if this was locked now when the error
> printk runs. Are there other bits in there that are helpful?

This is the only bit that's there to check the PLL locking status.
Should we rather poll ?

>
>> +             return -EBUSY;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int qusb2_phy_exit(struct phy *phy)
>> +{
>> +     struct qusb2_phy *qphy = phy_get_drvdata(phy);
>> +
>> +     /* Disable the PHY */
>> +     qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
>> +                     CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
>> +
>> +     if (!qphy->has_se_clk_scheme)
>> +             clk_disable_unprepare(qphy->ref_clk);
>> +
>> +     clk_disable_unprepare(qphy->cfg_ahb_clk);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct phy_ops qusb2_phy_gen_ops = {
>> +     .init           = qusb2_phy_init,
>> +     .exit           = qusb2_phy_exit,
>> +     .power_on       = qusb2_phy_poweron,
>> +     .power_off      = qusb2_phy_poweroff,
>> +     .owner          = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id qusb2_phy_of_match_table[] = {
>> +     {
>> +             .compatible     = "qcom,msm8996-qusb2-phy",
>> +             .data           = &msm8996_phy_init_cfg,
>> +     },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qusb2_phy_of_match_table);
>> +
>> +static int qusb2_phy_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct qusb2_phy *qphy;
>> +     struct phy_provider *phy_provider;
>> +     struct phy *generic_phy;
>> +     const struct of_device_id *match;
>> +     struct resource *res;
>> +     int ret;
>> +
>> +     qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
>> +     if (!qphy)
>> +             return -ENOMEM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     qphy->base = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(qphy->base))
>> +             return PTR_ERR(qphy->base);
>> +
>> +     qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb_clk");
>> +     if (IS_ERR(qphy->cfg_ahb_clk)) {
>> +             ret = PTR_ERR(qphy->cfg_ahb_clk);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get cfg_ahb_clk\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
>> +     if (IS_ERR(qphy->ref_clk_src)) {
>> +             ret = PTR_ERR(qphy->ref_clk_src);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "clk get failed for ref_clk_src\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->ref_clk = devm_clk_get(dev, "ref_clk");
>> +     if (IS_ERR(qphy->ref_clk)) {
>> +             ret = PTR_ERR(qphy->ref_clk);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "clk get failed for ref_clk\n");
>> +             return ret;
>> +     } else {
>> +             clk_set_rate(qphy->ref_clk, 19200000);
>
> Drop the else. Also what if clk_set_rate() fails?

Will drop the else.
we should fail in case clk_set_rate() fails.

>
>> +     }
>> +
>> +     qphy->iface_clk = devm_clk_get(dev, "iface_clk");
>> +     if (IS_ERR(qphy->iface_clk)) {
>> +             ret = PTR_ERR(qphy->iface_clk);
>> +             if (ret != -EPROBE_DEFER) {
>> +                     qphy->iface_clk = NULL;
>> +                     dev_dbg(dev, "clk get failed for iface_clk\n");
>> +             } else {
>> +                     return ret;
>> +             }
>
>                 if (PTR_ERR(qphy->iface_clk) == -EPROBE_DEFER)
>                         return -EPROBE_DEFER;
>                 qphy->iface_clk = NULL;
>                 dev_dbg(dev, "clk get failed for iface_clk\n");
>
> Is shorter.

Sure, will modify this as suggested.

>
>> +     }
>> +
>> +     qphy->phy_reset = devm_reset_control_get(&pdev->dev, "phy");
>> +     if (IS_ERR(qphy->phy_reset)) {
>> +             dev_err(dev, "failed to get phy core reset\n");
>> +             return PTR_ERR(qphy->phy_reset);
>> +     }
>> +
>> +     qphy->vdd_phy = devm_regulator_get(dev, "vdd-phy");
>> +     if (IS_ERR(qphy->vdd_phy)) {
>> +             dev_err(dev, "unable to get vdd-phy supply\n");
>> +             return PTR_ERR(qphy->vdd_phy);
>> +     }
>> +
>> +     qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
>> +     if (IS_ERR(qphy->vdda_pll)) {
>> +             dev_err(dev, "unable to get vdda-pll supply\n");
>> +             return PTR_ERR(qphy->vdda_pll);
>> +     }
>> +
>> +     qphy->vdda_phy_dpdm = devm_regulator_get(dev, "vdda-phy-dpdm");
>> +     if (IS_ERR(qphy->vdda_phy_dpdm)) {
>> +             dev_err(dev, "unable to get vdda-phy-dpdm supply\n");
>> +             return PTR_ERR(qphy->vdda_phy_dpdm);
>> +     }
>> +
>> +     /* Get the specific init parameters of QMP phy */
>> +     match = of_match_node(qusb2_phy_of_match_table, dev->of_node);
>> +     qphy->cfg = match->data;
>
> Use of_device_get_match_data() instead.

Okay.

>
>> +
>> +     qphy->tcsr = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                                                     "qcom,tcsr-syscon");
>> +     if (IS_ERR(qphy->tcsr)) {
>> +             dev_dbg(dev, "Failed to lookup TCSR regmap\n");
>> +             qphy->tcsr = NULL;
>> +     }
>> +
>> +     generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
>> +     if (IS_ERR(generic_phy)) {
>> +             ret = PTR_ERR(generic_phy);
>> +             dev_err(dev, "%s: failed to create phy %d\n", __func__, ret);
>> +             return ret;
>> +     }
>> +     qphy->phy = generic_phy;
>> +
>> +     dev_set_drvdata(dev, qphy);
>> +     phy_set_drvdata(generic_phy, qphy);
>> +
>> +     phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +     if (IS_ERR(phy_provider)) {
>> +             ret = PTR_ERR(phy_provider);
>> +             dev_err(dev, "%s: failed to register phy %d\n", __func__, ret);
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}

Thanks for a thorough review. Will respin the patch soon.


regards
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  2016-12-01  8:42     ` Vivek Gautam
@ 2016-12-02 18:47       ` Stephen Boyd
  2016-12-06  8:11         ` Vivek Gautam
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-12-02 18:47 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

On 12/01/2016 12:42 AM, Vivek Gautam wrote:
> On Tue, Nov 29, 2016 at 4:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 11/22, Vivek Gautam wrote:
>>> +     }
>>> +
>>> +     /*
>>> +      * we need to read only one byte here, since the required
>>> +      * parameter value fits in one nibble
>>> +      */
>>> +     val = (u8 *)nvmem_cell_read(cell, &len);
>> Shouldn't need the cast here. Also it would be nice if
>> nvmem_cell_read() didn't require a second argument if we don't
>> care for it. We should update the API to allow NULL there.
> Will remove the u8 pointer cast.
>
> Correct, it makes sense to allow the length pointer to be passed as NULL.
> We don't care about this length. Will update the nvmem API, to allow this.
>
> Also, we should add a check for 'cell' as well. This pointer can be
> NULL, and the first thing that  nvmem_cell_read does is - deference
> the pointer 'cell'

It would be pretty stupid to read a cell and pass NULL as the first
argument. I imagine things would blow up there like we want and we would
see a nice big stacktrace.

>>> +     } else {
>>> +             reset_val |= CLK_REF_SEL;
>>> +     }
>>> +
>>> +     writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
>>> +
>>> +     /* Make sure that above write is completed to get PLL source clock */
>>> +     wmb();
>>> +
>>> +     /* Required to get PHY PLL lock successfully */
>>> +     usleep_range(100, 110);
>>> +
>>> +     if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
>>> +                                     PLL_LOCKED)) {
>>> +             dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
>>> +                     readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));
>> Would be pretty funny if this was locked now when the error
>> printk runs. Are there other bits in there that are helpful?
> This is the only bit that's there to check the PLL locking status.
> Should we rather poll ?
>

I'm just saying that the printk may have the "correct" status but the
check would have failed earlier making the printk confusing. Perhaps
just save the register value from the first read and print it instead of
reading it again? Polling would probably be a better design anyway?
Hopefully the status bit isn't toggling back and forth during those
100-100us though, which may be the case here and that would explain why
it's not a polling design.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
  2016-12-02 18:47       ` Stephen Boyd
@ 2016-12-06  8:11         ` Vivek Gautam
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-12-06  8:11 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

On Sat, Dec 3, 2016 at 12:17 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/01/2016 12:42 AM, Vivek Gautam wrote:
>> On Tue, Nov 29, 2016 at 4:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 11/22, Vivek Gautam wrote:
>>>> +     }
>>>> +
>>>> +     /*
>>>> +      * we need to read only one byte here, since the required
>>>> +      * parameter value fits in one nibble
>>>> +      */
>>>> +     val = (u8 *)nvmem_cell_read(cell, &len);
>>> Shouldn't need the cast here. Also it would be nice if
>>> nvmem_cell_read() didn't require a second argument if we don't
>>> care for it. We should update the API to allow NULL there.
>> Will remove the u8 pointer cast.
>>
>> Correct, it makes sense to allow the length pointer to be passed as NULL.
>> We don't care about this length. Will update the nvmem API, to allow this.
>>
>> Also, we should add a check for 'cell' as well. This pointer can be
>> NULL, and the first thing that  nvmem_cell_read does is - deference
>> the pointer 'cell'
>
> It would be pretty stupid to read a cell and pass NULL as the first
> argument. I imagine things would blow up there like we want and we would
> see a nice big stacktrace.

Right, reading a 'NULL' cell doesn't make a sense at all.

>>>> +     } else {
>>>> +             reset_val |= CLK_REF_SEL;
>>>> +     }
>>>> +
>>>> +     writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
>>>> +
>>>> +     /* Make sure that above write is completed to get PLL source clock */
>>>> +     wmb();
>>>> +
>>>> +     /* Required to get PHY PLL lock successfully */
>>>> +     usleep_range(100, 110);
>>>> +
>>>> +     if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
>>>> +                                     PLL_LOCKED)) {
>>>> +             dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
>>>> +                     readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));
>>> Would be pretty funny if this was locked now when the error
>>> printk runs. Are there other bits in there that are helpful?
>> This is the only bit that's there to check the PLL locking status.
>> Should we rather poll ?
>>
>
> I'm just saying that the printk may have the "correct" status but the
> check would have failed earlier making the printk confusing. Perhaps
> just save the register value from the first read and print it instead of
> reading it again? Polling would probably be a better design anyway?
> Hopefully the status bit isn't toggling back and forth during those
> 100-100us though, which may be the case here and that would explain why
> it's not a polling design.

Okay, will save the register value.
Will also stick to just checking the status after the delay, like we have in
downstream kernel.


Regards
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-28 22:55   ` Stephen Boyd
  2016-11-29  5:25     ` Vivek Gautam
@ 2016-12-12 16:40     ` Vivek Gautam
  1 sibling, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-12-12 16:40 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi Stephen,

On Tue, Nov 29, 2016 at 4:25 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/22, Vivek Gautam wrote:
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> new file mode 100644
>> index 0000000..ffb173b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -0,0 +1,74 @@
>> +Qualcomm QMP PHY
>> +----------------
>> +
>> +QMP phy controller supports physical layer functionality for a number of
>> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
>> +
>> +Required properties:
>> + - compatible: compatible list, contains:
>> +            "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
>> +            "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
>> + - reg: list of offset and length pair of the PHY register sets.
>> +     at index 0: offset and length of register set for PHY common
>> +                 serdes block.
>> +     from index 1 - N: offset and length of register set for each lane,
>> +                       for N number of phy lanes (ports).
>> + - lane-offsets: array of offsets to tx, rx and pcs blocks for phy lanes.
>> + - #phy-cells: must be 1
>> +    - Cell after phy phandle should be the port (lane) number.
>> + - clocks: a list of phandles and clock-specifier pairs,
>> +        one for each entry in clock-names.
>> + - clock-names: must be "cfg_ahb" for phy config clock,
>> +                     "aux" for phy aux clock,
>> +                     "ref_clk" for 19.2 MHz ref clk,
>> +                     "ref_clk_src" for reference clock source,
>
> We typically leave "clk" out of clk names because it's redundant.
>
>> +                     "pipe<port-number>" for pipe clock specific to
>> +                     each port/lane (Optional).
>
> The pipe clocks are orphaned right now. We should add an output
> clock from the phy to go into the controller and back into the
> phy if I recall correctly. The phy should be a clock provider
> itself so it can output the pipe clock source into GCC and back
> into the phy and controller.

You are correct. The pipe clocks come out of PHY controllers and
go back to the gcc that gates them finally.

I will register the phy drivers as clock providers so that gcc can make
reference to it.


Best Regards
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy
  2016-11-28 23:19   ` Stephen Boyd
@ 2016-12-13  9:18     ` Vivek Gautam
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-12-13  9:18 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi Stephen,

On Tue, Nov 29, 2016 at 4:49 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 11/22, Vivek Gautam wrote:
>> Qualcomm chipsets have QMP phy controller that provides
>> support to a number of controller, viz. PCIe, UFS, and USB.
>> Adding dt binding information for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>
>> Changes since v1:
>>  - New patch, forked out of the original driver patch:
>>    "phy: qcom-qmp: new qmp phy driver for qcom-chipsets"
>>  - updated bindings to include mem resource as a list of
>>    offset - length pair for serdes block and for each lane.
>>  - added a new binding for 'lane-offsets' that contains offsets
>>    to tx, rx and pcs blocks from each lane base address.
>>
>>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 74 ++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> new file mode 100644
>> index 0000000..ffb173b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -0,0 +1,74 @@
>> +Qualcomm QMP PHY
>> +----------------
>> +
>> +QMP phy controller supports physical layer functionality for a number of
>> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.

[...]

>> +Example:
>> +     pcie_phy: pciephy@34000 {
>> +             compatible = "qcom,msm8996-qmp-pcie-phy";
>> +             reg = <0x034000 0x48f>,
>> +                     <0x035000 0x5bf>,
>> +                     <0x036000 0x5bf>,
>> +                     <0x037000 0x5bf>;
>> +                             /* tx, rx, pcs */
>> +             lane-offsets = <0x0 0x200 0x400>;
>> +             #phy-cells = <1>;
>> +
>> +             clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
>> +                     <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
>> +                     <&rpmcc MSM8996_RPM_SMD_LN_BB_CLK>,
>> +                     <&gcc GCC_PCIE_CLKREF_CLK>,
>> +                     <&gcc GCC_PCIE_0_PIPE_CLK>,
>> +                     <&gcc GCC_PCIE_1_PIPE_CLK>,
>> +                     <&gcc GCC_PCIE_2_PIPE_CLK>;
>> +             clock-names = "aux", "cfg_ahb",
>> +                             "ref_clk_src", "ref_clk",
>
> Does MSM8996_RPM_SMD_LN_BB_CLK supply the clock source for
> GCC_PCIE_CLKREF_CLK? Did we mess up the parent/child relationship
> in the GCC driver? We may want to fix that so that this node
> only references clocks that actually go into the device, instead
> of clock parents.

The clock documentations do show that the RPM_SMD_LN_BB_CLK provides
the 19.2 MHz phy clocks via pad. So, like you rightly said we may have to
fix the parents for phy reference clocks for difference phy controllers on 8996.

I will gather some more info on this to discuss it further.


Thanks
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
  2016-11-29  0:35   ` Stephen Boyd
@ 2016-12-20  5:42     ` Vivek Gautam
  0 siblings, 0 replies; 21+ messages in thread
From: Vivek Gautam @ 2016-12-20  5:42 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kishon, robh+dt, Mark Rutland, devicetree, linux-kernel,
	Srinivas Kandagatla, linux-arm-msm

Hi,

On Tue, Nov 29, 2016 at 6:05 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

Thanks for a thorough review. Please find my comments inline.

> On 11/22, Vivek Gautam wrote:
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index f1dcec1..8970d9e 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -430,6 +430,15 @@ config PHY_STIH407_USB
>>         Enable this support to enable the picoPHY device used by USB2
>>         and USB3 controllers on STMicroelectronics STiH407 SoC families.
>>
>> +config PHY_QCOM_QMP
>> +     tristate "Qualcomm QMP PHY Driver"
>> +     depends on OF && (ARCH_QCOM || COMPILE_TEST)
>> +     select GENERIC_PHY
>> +     select RESET_CONTROLLER
>
> Again, probably should be dropped as we're not providing resets
> here, just using them.

Sure, will drop in v3.

>
>> diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
>> new file mode 100644
>> index 0000000..f85289e
>> --- /dev/null
>> +++ b/drivers/phy/phy-qcom-qmp.c
>> @@ -0,0 +1,1141 @@
>> +/*
>> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#include <dt-bindings/phy/phy.h>
>> +
> [...]
>> +
>> +/* set of registers with offsets different per-PHY */
>> +enum qphy_reg_layout {
>> +     /* Common block control registers */
>> +     QPHY_COM_SW_RESET,
>> +     QPHY_COM_POWER_DOWN_CONTROL,
>> +     QPHY_COM_START_CONTROL,
>> +     QPHY_COM_PCS_READY_STATUS,
>> +     /* PCS registers */
>> +     QPHY_PLL_LOCK_CHK_DLY_TIME,
>> +     QPHY_FLL_CNTRL1,
>> +     QPHY_FLL_CNTRL2,
>> +     QPHY_FLL_CNT_VAL_L,
>> +     QPHY_FLL_CNT_VAL_H_TOL,
>> +     QPHY_FLL_MAN_CODE,
>> +     QPHY_PCS_READY_STATUS,
>> +};
>> +
>> +unsigned int pciephy_regs_layout[] = {
>
> static? const?

Will take care of these in v3.
[...]

>> +
>> +/**
>> + * struct qmp_phy_init_cfg:- per-PHY init config.
>
> The colon and dash can't be right. One is kernel-doc, but of
> course there aren't any other member descriptions.

Right, will convert this to a one-line comment.

>
>> + */
>> +struct qmp_phy_init_cfg {
>> +     /* phy-type - PCIE/UFS/USB */
>> +     unsigned int type;
>> +     /* number of lanes provided by phy */
>> +     int nlanes;
>> +
>> +     /* Initialization sequence for PHY blocks - Serdes, tx, rx, pcs */
>> +     struct qmp_phy_init_tbl *phy_init_serdes_tbl;
>> +     int phy_init_serdes_tbl_sz;
>> +     struct qmp_phy_init_tbl *phy_init_tx_tbl;
>> +     int phy_init_tx_tbl_sz;
>> +     struct qmp_phy_init_tbl *phy_init_rx_tbl;
>> +     int phy_init_rx_tbl_sz;
>> +     struct qmp_phy_init_tbl *phy_init_pcs_tbl;
>> +     int phy_init_pcs_tbl_sz;
>
> _sz makes it sound like bytes, perhaps _num instead.

sure.

>
>> +
>> +     /* array of registers with different offsets */
>> +     unsigned int *regs;
>
> const?

taking care of these in v3.

>
>> +
>> +     unsigned int mask_start_ctrl;
>> +     unsigned int mask_pwr_dn_ctrl;
>> +     /* true, if PHY has a separate PHY_COM_CNTRL block */
>> +     bool has_phy_com_ctrl;
>> +     /* true, if PHY has a reset for individual lanes */
>> +     bool has_lane_rst;
>> +};
>> +
>> +/**
>> + * struct qmp_phy_desc:- per-lane phy-descriptor.
>
> Also kernel-doc requests full stop is left out on one line
> descriptions.

sure, will remove it from here and from other places where a full-stop
is not required.

>
>> + *
>> + * @phy: pointer to generic phy
>> + * @tx: pointer to iomapped memory space for PHY's tx
>> + * @rx: pointer to iomapped memory space for PHY's rx
>> + * @pcs: pointer to iomapped memory space for PHY's pcs
>> + * @pipe_clk: pointer to pipe lock
>> + * @index: lane index
>> + * @qphy: pointer to QMP phy to which this lane belongs
>> + * @lane_rst: pointer to lane's reset controller
>
> Not sure we care about "pointer to" as types can be figured out
> other ways.

will drop "pointer to" string.

>
>> + */
>> +struct qmp_phy_desc {
>> +     struct phy *phy;
>> +     void __iomem *tx;
>> +     void __iomem *rx;
>> +     void __iomem *pcs;
>> +     struct clk *pipe_clk;
>> +     unsigned int index;
>> +     struct qcom_qmp_phy *qphy;
>> +     struct reset_control *lane_rst;
>> +};
>> +
>> +/**
>> + * struct qcom_qmp_phy:- structure holding QMP PHY attributes.
>> + *
>> + * @dev: pointer to device
>> + * @serdes: pointer to iomapped memory space for phy's serdes
>> + *
>> + * @aux_clk: pointer to phy core clock
>> + * @cfg_ahb_clk: pointer to AHB2PHY interface clock
>> + * @ref_clk: pointer to reference clock
>> + * @ref_clk_src: pointer to source to reference clock
>> + *
>> + * @vdda_phy: vdd supply to the phy core block
>> + * @vdda_pll: 1.8V vdd supply to ref_clk block
>> + * @vddp_ref_clk: vdd supply to specific ref_clk block (Optional)
>> + *
>> + * @phy_rst: Pointer to phy reset control
>> + * @phycom_rst: Pointer to phy common reset control
>> + * @phycfg_rst: Pointer to phy ahb cfg reset control (Optional)
>> + *
>> + * @cfg: pointer to init config for each phys
>> + * @phys: array of pointer to per-lane phy descriptors
>> + * @phy_mutex: mutex lock for PHY common block initialization
>> + * @init_count: Phy common block initialization count
>> + */
>> +struct qcom_qmp_phy {
>> +     struct device *dev;
>> +     void __iomem *serdes;
>> +
>> +     struct clk *aux_clk;
>> +     struct clk *cfg_ahb_clk;
>> +     struct clk *ref_clk;
>> +     struct clk *ref_clk_src;
>> +
>> +     struct regulator *vdda_phy;
>> +     struct regulator *vdda_pll;
>> +     struct regulator *vddp_ref_clk;
>> +
>> +     struct reset_control *phy_rst;
>> +     struct reset_control *phycom_rst;
>> +     struct reset_control *phycfg_rst;
>> +
>> +     const struct qmp_phy_init_cfg *cfg;
>> +     struct qmp_phy_desc **phys;
>> +
>> +     struct mutex phy_mutex;
>> +     int init_count;
>> +};
>> +
>> +static inline void qphy_setbits(void __iomem *reg, u32 val)
>> +{
>> +     u32 reg_val;
>> +
>> +     reg_val = readl_relaxed(reg);
>> +     reg_val |= val;
>> +     writel_relaxed(reg_val, reg);
>> +}
>> +
>> +static inline void qphy_clrbits(void __iomem *reg, u32 val)
>> +{
>> +     u32 reg_val;
>> +
>> +     reg_val = readl_relaxed(reg);
>> +     reg_val &= ~val;
>> +     writel_relaxed(reg_val, reg);
>> +}
>> +
>> +const struct qmp_phy_init_cfg msm8996_pciephy_init_cfg = {
>
> static?
>
>> +     .type                   = PHY_TYPE_PCIE,
>> +     .nlanes                 = 3,
>> +
>> +     .phy_init_serdes_tbl    = pciephy_serdes_init_tbl,
>> +     .phy_init_serdes_tbl_sz = ARRAY_SIZE(pciephy_serdes_init_tbl),
>> +     .phy_init_tx_tbl        = pciephy_tx_init_tbl,
>> +     .phy_init_tx_tbl_sz     = ARRAY_SIZE(pciephy_tx_init_tbl),
>> +     .phy_init_rx_tbl        = pciephy_rx_init_tbl,
>> +     .phy_init_rx_tbl_sz     = ARRAY_SIZE(pciephy_rx_init_tbl),
>> +     .phy_init_pcs_tbl       = pciephy_pcs_init_tbl,
>> +     .phy_init_pcs_tbl_sz    = ARRAY_SIZE(pciephy_pcs_init_tbl),
>> +     .regs                   = pciephy_regs_layout,
>> +     .mask_start_ctrl        = (PHY_PCS_START | PHY_PLL_READY_GATE_EN),
>> +     .mask_pwr_dn_ctrl       = (PHY_SW_PWRDN | PHY_REFCLK_DRV_DSBL),
>> +
>> +     .has_phy_com_ctrl       = true,
>> +     .has_lane_rst           = true,
>> +};
>> +
>> +const struct qmp_phy_init_cfg msm8996_usb3phy_init_cfg = {
>
> static? Try running sparse.

Right, will do that from now on.

>
>> +     .type                   = PHY_TYPE_USB3,
>> +     .nlanes                 = 1,
>> +
>> +     .phy_init_serdes_tbl    = usb3phy_serdes_init_tbl,
>> +     .phy_init_serdes_tbl_sz = ARRAY_SIZE(usb3phy_serdes_init_tbl),
>> +     .phy_init_tx_tbl        = usb3phy_tx_init_tbl,
>> +     .phy_init_tx_tbl_sz     = ARRAY_SIZE(usb3phy_tx_init_tbl),
>> +     .phy_init_rx_tbl        = usb3phy_rx_init_tbl,
>> +     .phy_init_rx_tbl_sz     = ARRAY_SIZE(usb3phy_rx_init_tbl),
>> +     .phy_init_pcs_tbl       = usb3phy_pcs_init_tbl,
>> +     .phy_init_pcs_tbl_sz    = ARRAY_SIZE(usb3phy_pcs_init_tbl),
>
> Do we really need phy_init as a prefix. Could just be serdes_tbl,
> tx_tbl, etc?

Right, shorter names improve readability.

>
>> +     .regs                   = usb3phy_regs_layout,
>> +     .mask_start_ctrl        = (PHY_SERDES_START | PHY_PCS_START),
>> +     .mask_pwr_dn_ctrl       = PHY_SW_PWRDN,
>> +};
>> +
>> +static void qcom_qmp_phy_configure(void __iomem *base,
>> +                             unsigned int *regs_layout,
>> +                             struct qmp_phy_init_tbl init_tbl[],
>> +                             int init_tbl_sz)
>
> Shorter variable names would make this easier to read.

ditto.

>
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < init_tbl_sz; i++) {
>> +             if (init_tbl[i].in_layout)
>> +                     writel_relaxed(init_tbl[i].cfg_val,
>> +                             base + regs_layout[init_tbl[i].reg_offset]);
>> +             else
>> +                     writel_relaxed(init_tbl[i].cfg_val,
>> +                             base + init_tbl[i].reg_offset);
>> +     }
>> +
>         const struct qmp_phy_init_tbl *t = tbl;
>
>         for (i = 0; i < num; i++, t++)
>                 if (t->in_layout)
>                         writel_relaxed(t->val, base + regs[t->offset]);
>                 else
>                         writel_relaxed(t->val, base + t->offset);
>

Updating this in v3.

>> +     /* flush buffered writes */
>> +     mb();
>> +}
>> +
>> +static int qcom_qmp_phy_poweron(struct phy *phy)
>> +{
>> +     struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
>> +     struct qcom_qmp_phy *qphy = phydesc->qphy;
>> +     int ret;
>> +
>> +     dev_vdbg(&phy->dev, "Powering on QMP phy\n");
>> +
>> +     ret = regulator_enable(qphy->vdda_phy);
>> +     if (ret) {
>> +             dev_err(qphy->dev, "%s: vdda-phy enable failed, err=%d\n",
>> +                             __func__, ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = regulator_enable(qphy->vdda_pll);
>> +     if (ret) {
>> +             dev_err(qphy->dev, "%s: vdda-pll enable failed, err=%d\n",
>> +                             __func__, ret);
>> +             goto err_vdda_pll;
>> +     }
>> +
>> +     if (qphy->vddp_ref_clk) {
>> +             ret = regulator_enable(qphy->vddp_ref_clk);
>> +             if (ret) {
>> +                     dev_err(qphy->dev, "%s: vdda-ref-clk enable failed, err=%d\n",
>> +                                     __func__, ret);
>> +                     goto err_vddp_refclk;
>> +             }
>> +     }
>> +
>> +     clk_prepare_enable(qphy->ref_clk_src);
>> +     clk_prepare_enable(qphy->ref_clk);
>> +     clk_prepare_enable(phydesc->pipe_clk);
>
> And if these fail?

Will add error handling for all instances of clk_prepare_enable()
across the driver in next patch version.

>
>> +
>> +     return 0;
>> +
>> +err_vddp_refclk:
>> +     regulator_disable(qphy->vdda_pll);
>> +err_vdda_pll:
>> +     regulator_disable(qphy->vdda_phy);
>> +     return ret;
>> +}
>> +
>> +static int qcom_qmp_phy_poweroff(struct phy *phy)
>> +{
>> +     struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
>> +     struct qcom_qmp_phy *qphy = phydesc->qphy;
>> +
>> +     clk_disable_unprepare(qphy->ref_clk_src);
>> +     clk_disable_unprepare(qphy->ref_clk);
>> +     clk_disable_unprepare(phydesc->pipe_clk);
>> +
>> +     if (qphy->vddp_ref_clk)
>> +             regulator_disable(qphy->vddp_ref_clk);
>> +
>> +     regulator_disable(qphy->vdda_pll);
>> +     regulator_disable(qphy->vdda_phy);
>> +
>> +     return 0;
>> +}
>> +
>> +static int qcom_qmp_phy_is_ready(struct qcom_qmp_phy *qphy,
>> +                             void __iomem *pcs_status, u32 mask)
>> +{
>> +     unsigned int init_timeout;
>> +
>> +     init_timeout = PHY_READY_TIMEOUT_COUNT;
>> +     do {
>> +             if (readl_relaxed(pcs_status) & mask)
>> +                     break;
>> +
>> +             usleep_range(REFCLK_STABILIZATION_DELAY_US_MIN,
>> +                              REFCLK_STABILIZATION_DELAY_US_MAX);
>> +     } while (--init_timeout);
>
> readl_poll_timeout?

Right, will use that.

>
>> +
>> +     if (!init_timeout)
>> +             return -EBUSY;
>> +
>> +     return 0;
>> +}
>> +
> [...]
>> +
>> +/* PHY Initialization */
>> +static int qcom_qmp_phy_init(struct phy *phy)
>> +{
>> +     struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
>> +     struct qcom_qmp_phy *qphy = phydesc->qphy;
>> +     const struct qmp_phy_init_cfg *cfg = qphy->cfg;
>> +     void __iomem *tx = phydesc->tx;
>> +     void __iomem *rx = phydesc->rx;
>> +     void __iomem *pcs = phydesc->pcs;
>> +     int ret;
>> +
>> +     dev_vdbg(qphy->dev, "Initializing QMP phy\n");
>> +
>> +     /* enable interface clocks to program phy */
>> +     clk_prepare_enable(qphy->aux_clk);
>> +     clk_prepare_enable(qphy->cfg_ahb_clk);
>> +
>> +     ret = qcom_qmp_phy_com_init(qphy);
>> +     if (ret)
>> +             goto err;
>> +
>> +     if (phydesc->lane_rst) {
>> +             ret = reset_control_deassert(phydesc->lane_rst);
>> +             if (ret) {
>> +                     dev_err(qphy->dev, "lane<%d> reset deassert failed\n",
>
> Drop the brackets?
sure.

>
>> +                                     phydesc->index);
>> +                     goto err_lane_rst;
>> +             }
>> +     }
>> +
>> +     /* Tx, Rx, and PCS configurations */
>> +     qcom_qmp_phy_configure(tx, cfg->regs, cfg->phy_init_tx_tbl,
>> +                             cfg->phy_init_tx_tbl_sz);
>> +     qcom_qmp_phy_configure(rx, cfg->regs, cfg->phy_init_rx_tbl,
>> +                             cfg->phy_init_rx_tbl_sz);
>> +     qcom_qmp_phy_configure(pcs, cfg->regs, cfg->phy_init_pcs_tbl,
>> +                             cfg->phy_init_pcs_tbl_sz);
>> +
>> +     /*
>> +      * Pull out PHY from POWER DOWN state:
>> +      * This is active low enable signal to power-down PHY.
>> +      */
>> +     qphy_setbits(pcs + QPHY_POWER_DOWN_CONTROL, cfg->mask_pwr_dn_ctrl);
>> +     /* XXX: 10 us delay; given in PCIE phy programming guide only */
>
> Is this a todo?

No, this delay seems to be required by the pcie phy only. USB3 phy guide
doesn't mention anything about this delay. Neither the downstream
USB3 qmp phy driver uses this delay.
Can add a check for the phy type to use this delay.

>
>> +     usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
>> +
>> +     /* start SerDes and Phy-Coding-Sublayer */
>> +     qphy_setbits(pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
>> +
>> +     /* Pull PHY out of reset state */
>> +     qphy_clrbits(pcs + QPHY_SW_RESET, PHY_SW_RESET);
>> +     /* Make sure that above writes are completed */
>> +     mb();
>> +
>> +     ret = qcom_qmp_phy_is_ready(qphy, pcs +
>> +                                     cfg->regs[QPHY_PCS_READY_STATUS],
>> +                                     MASK_PHYSTATUS);
>> +     if (ret) {
>> +             dev_err(qphy->dev, "phy initialization timed-out\n");
>> +             goto err_pcs_ready;
>> +     }
>> +
>> +     return 0;
>> +
>> +err_pcs_ready:
>> +     if (phydesc->lane_rst)
>> +             reset_control_assert(phydesc->lane_rst);
>> +err_lane_rst:
>> +     qcom_qmp_phy_com_exit(qphy);
>> +err:
>> +     clk_disable_unprepare(qphy->cfg_ahb_clk);
>> +     clk_disable_unprepare(qphy->aux_clk);
>> +     return ret;
>> +}
>> +
>> +static int qcom_qmp_phy_exit(struct phy *phy)
>> +{
>> +     struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
>> +     struct qcom_qmp_phy *qphy = phydesc->qphy;
>> +     const struct qmp_phy_init_cfg *cfg = qphy->cfg;
>> +
>> +     /* PHY reset */
>> +     qphy_setbits(phydesc->pcs + QPHY_SW_RESET, PHY_SW_RESET);
>> +
>> +     /* stop SerDes and Phy-Coding-Sublayer */
>> +     qphy_clrbits(phydesc->pcs + QPHY_START_CTRL, cfg->mask_start_ctrl);
>> +
>> +     /* Put PHY into POWER DOWN state: active low */
>> +     qphy_clrbits(phydesc->pcs + QPHY_POWER_DOWN_CONTROL,
>> +                     cfg->mask_pwr_dn_ctrl);
>> +
>> +     /* Make sure that above writes are completed */
>> +     mb();
>> +
>> +     if (phydesc->lane_rst)
>> +             reset_control_assert(phydesc->lane_rst);
>> +
>> +     qcom_qmp_phy_com_exit(qphy);
>> +
>> +     clk_disable_unprepare(qphy->aux_clk);
>> +     clk_disable_unprepare(qphy->cfg_ahb_clk);
>> +
>> +     return 0;
>> +}
>> +
>> +
>> +static int qcom_qmp_phy_regulator_init(struct device *dev)
>> +{
>> +     struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     qphy->vdda_phy = devm_regulator_get(dev, "vdda-phy");
>> +     if (IS_ERR(qphy->vdda_phy)) {
>> +             ret = PTR_ERR(qphy->vdda_phy);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get vdda-phy, %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
>> +     if (IS_ERR(qphy->vdda_pll)) {
>> +             ret = PTR_ERR(qphy->vdda_pll);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get vdda-pll, %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     /* optional regulator */
>> +     qphy->vddp_ref_clk = devm_regulator_get(dev, "vddp-ref-clk");
>> +     if (IS_ERR(qphy->vddp_ref_clk)) {
>> +             ret = PTR_ERR(qphy->vddp_ref_clk);
>> +             if (ret != -EPROBE_DEFER) {
>> +                     dev_dbg(dev, "failed to get vddp-ref-clk, %d\n", ret);
>> +                     qphy->vddp_ref_clk = NULL;
>> +             } else {
>> +                     return ret;
>> +             }
>> +     }
>
> Regulator framework should insert a dummy regulator if this is
> missing in DT. So we shouldn't need to do anything complicated
> here right?

Right, we don't need to set regulator to NULL. Will simplify this
error handling.

>
>> +
>> +     return 0;
>> +}
>> +
>> +static int qcom_qmp_phy_clk_init(struct device *dev)
>> +{
>> +     struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     qphy->aux_clk = devm_clk_get(dev, "aux");
>> +     if (IS_ERR(qphy->aux_clk)) {
>> +             ret = PTR_ERR(qphy->aux_clk);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get aux_clk\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb");
>> +     if (IS_ERR(qphy->cfg_ahb_clk)) {
>> +             ret = PTR_ERR(qphy->cfg_ahb_clk);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get cfg_ahb_clk\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
>> +     if (IS_ERR(qphy->ref_clk_src)) {
>> +             ret = PTR_ERR(qphy->ref_clk_src);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get ref_clk_src\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->ref_clk = devm_clk_get(dev, "ref_clk");
>> +     if (IS_ERR(qphy->ref_clk)) {
>> +             ret = PTR_ERR(qphy->ref_clk);
>> +             if (ret != -EPROBE_DEFER)
>> +                     dev_err(dev, "failed to get ref_clk\n");
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static struct phy *qcom_qmp_phy_xlate(struct device *dev,
>> +                                     struct of_phandle_args *args)
>> +{
>> +     struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
>> +     int i;
>> +
>> +     if (WARN_ON(args->args[0] >= qphy->cfg->nlanes))
>> +             return ERR_PTR(-ENODEV);
>> +
>> +     for (i = 0; i < qphy->cfg->nlanes; i++) {
>> +             if (qphy->phys[i]->index == args->args[0])
>> +                     break;
>> +     }
>
> Drop the braces please.

sure.

>
>> +
>> +     if (i == qphy->cfg->nlanes)
>> +             return ERR_PTR(-ENODEV);
>> +
>> +     return qphy->phys[i]->phy;
>
> Could be simplified:
>
>
>         for (i = 0; i < qphy->cfg->nlanes; i++)
>                 if (qphy->phys[i]->index == args->args[0])
>                         return qphy->phys[i]->phy;
>
>         return ERR_PTR(-ENODEV);
>
>
> Also, isn't i == phys[i]->index so this could be a direct lookup?

Thanks. Will modify this as suggested, and will use
if (i == args->args[0]) as the condition.

>
>> +}
>> +
>> +static const struct phy_ops qcom_qmp_phy_gen_ops = {
>> +     .init           = qcom_qmp_phy_init,
>> +     .exit           = qcom_qmp_phy_exit,
>> +     .power_on       = qcom_qmp_phy_poweron,
>> +     .power_off      = qcom_qmp_phy_poweroff,
>> +     .owner          = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id qcom_qmp_phy_of_match_table[] = {
>> +     {
>> +             .compatible = "qcom,msm8996-qmp-pcie-phy",
>> +             .data = &msm8996_pciephy_init_cfg,
>> +     }, {
>> +             .compatible = "qcom,msm8996-qmp-usb3-phy",
>> +             .data = &msm8996_usb3phy_init_cfg,
>> +     },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_qmp_phy_of_match_table);
>> +
>> +static int qcom_qmp_phy_probe(struct platform_device *pdev)
>> +{
>> +     struct qcom_qmp_phy *qphy;
>> +     struct device *dev = &pdev->dev;
>> +     struct phy_provider *phy_provider;
>> +     struct resource *res;
>> +     const struct of_device_id *match;
>> +     void __iomem *base;
>> +     int ret = 0;
>
> Shouldn't need a default assignment here.
will remove the assignment.

>
>> +     int id;
>> +
>> +     qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
>> +     if (!qphy)
>> +             return -ENOMEM;
>> +
>> +     qphy->dev = dev;
>> +     dev_set_drvdata(dev, qphy);
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     base = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(base))
>> +             return PTR_ERR(base);
>> +
>> +     /* per PHY serdes; usually located at base address */
>> +     qphy->serdes = base;
>> +
>> +     mutex_init(&qphy->phy_mutex);
>> +
>> +     /* Get the specific init parameters of QMP phy */
>> +     match = of_match_node(qcom_qmp_phy_of_match_table, dev->of_node);
>> +     qphy->cfg = match->data;
>> +
>> +     ret = qcom_qmp_phy_clk_init(dev);
>> +     if (ret) {
>> +             dev_err(dev, "clock init failed\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = qcom_qmp_phy_regulator_init(dev);
>> +     if (ret) {
>> +             dev_err(dev, "regulator init failed\n");
>> +             return ret;
>> +     }
>> +
>> +     qphy->phy_rst = devm_reset_control_get(dev, "phy");
>> +     if (IS_ERR(qphy->phy_rst)) {
>> +             dev_err(dev, "failed to get phy core reset\n");
>> +             return PTR_ERR(qphy->phy_rst);
>> +     }
>> +
>> +     qphy->phycom_rst = devm_reset_control_get(dev, "common");
>> +     if (IS_ERR(qphy->phycom_rst)) {
>> +             dev_err(dev, "failed to get phy common reset\n");
>> +             return PTR_ERR(qphy->phycom_rst);
>> +     }
>> +
>> +     qphy->phycfg_rst = devm_reset_control_get(dev, "cfg");
>> +     if (IS_ERR(qphy->phycfg_rst)) {
>> +             dev_dbg(dev, "failed to get phy ahb cfg reset\n");
>> +             qphy->phycfg_rst = NULL;
>> +     }
>> +
>> +     qphy->phys = devm_kcalloc(dev, qphy->cfg->nlanes,
>> +                                     sizeof(*qphy->phys), GFP_KERNEL);
>> +     if (!qphy->phys)
>> +             return -ENOMEM;
>> +
>> +     for (id = 0; id < qphy->cfg->nlanes; id++) {
>> +             struct phy *generic_phy;
>> +             struct qmp_phy_desc *phy_desc;
>> +             char prop_name[MAX_PROP_NAME];
>> +             unsigned int lane_offsets[3];
>> +
>> +             /* mem resources from index 1 to N for N number of lanes */
>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, id + 1);
>> +             base = devm_ioremap_resource(dev, res);
>> +             if (IS_ERR(base))
>> +                     return PTR_ERR(base);
>> +
>> +             phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
>> +             if (!phy_desc)
>> +                     return -ENOMEM;
>> +
>> +             /*
>> +              * read offsets to Tx, Rx, and PCS blocks into a u32 array:
>> +              *  ------------------------------------
>> +              * | tx offset | rx offset | pcs offset |
>> +              *  ------------------------------------
>> +              */
>> +             ret = of_property_read_u32_array(dev->of_node, "lane-offsets",
>> +                                                        lane_offsets, 3);
>> +             if (ret) {
>> +                     dev_err(dev,
>> +                             "failed to get tx/rx/pcs offsets for lane%d\n",
>> +                             id);
>> +                             return ret;
>> +             }
>> +
>> +             phy_desc->tx = base + lane_offsets[0];
>> +             phy_desc->rx = base + lane_offsets[1];
>> +             phy_desc->pcs = base + lane_offsets[2];
>> +
>> +             /*
>> +              * Get PHY's Pipe clock, if any; USB3 and PCIe are PIPE3
>> +              * based phys, so they essentially have pipe clock. So,
>> +              * we return error in case phy is USB3 or PIPE type.
>> +              * Otherwise, we initialize pipe clock to NULL for
>> +              * all phys that don't need this.
>> +              */
>> +             memset(&prop_name, 0, sizeof(prop_name));
>> +             snprintf(prop_name, MAX_PROP_NAME, "pipe%d", id);
>> +             phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
>> +             if (IS_ERR(phy_desc->pipe_clk)) {
>> +                     if (qphy->cfg->type == PHY_TYPE_PCIE ||
>> +                         qphy->cfg->type == PHY_TYPE_USB3) {
>> +                             ret = PTR_ERR(phy_desc->pipe_clk);
>> +                             if (ret != -EPROBE_DEFER)
>> +                                     dev_err(dev,
>> +                                     "failed to get lane%d pipe_clk\n", id);
>> +                             return ret;
>> +                     } else {
>> +                             phy_desc->pipe_clk = NULL;
>> +                     }
>
> Drop the else part.

sure.

>
>> +             }
>> +
>> +             /* Get lane reset, if any */
>> +             if (qphy->cfg->has_lane_rst) {
>> +                     memset(&prop_name, 0, sizeof(prop_name));
>> +                     snprintf(prop_name, MAX_PROP_NAME, "lane%d", id);
>> +                     phy_desc->lane_rst = devm_reset_control_get(dev,
>> +                                                                 prop_name);
>> +                     if (IS_ERR(phy_desc->lane_rst)) {
>> +                             dev_err(dev, "failed to get lane%d reset\n",
>> +                                     id);
>> +                             return PTR_ERR(phy_desc->lane_rst);
>> +                     }
>> +             }
>> +
>> +             generic_phy = devm_phy_create(dev, NULL, &qcom_qmp_phy_gen_ops);
>> +             if (IS_ERR(generic_phy)) {
>> +                     ret = PTR_ERR(generic_phy);
>> +                     dev_err(dev, "failed to create qphy %d\n", ret);
>> +                     return ret;
>> +             }
>> +
>> +             phy_desc->phy = generic_phy;
>> +             phy_desc->index = id;
>> +             phy_desc->qphy = qphy;
>> +             phy_set_drvdata(generic_phy, phy_desc);
>> +             qphy->phys[id] = phy_desc;
>
> Probably should make the above part of this loop a function that
> returns a phy for each lane (or an  error code on failure). This
> probe function is long.

Sure, will take this phy creation part to a separate function.


Best Regards
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2016-12-20  5:43 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22 12:02 [PATCH v2 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
2016-11-22 12:02 ` [PATCH v2 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
2016-11-28 14:19   ` Rob Herring
2016-11-29  5:20     ` Vivek Gautam
2016-11-28 22:49   ` Stephen Boyd
2016-11-29  5:22     ` Vivek Gautam
2016-11-22 12:02 ` [PATCH v2 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam
2016-11-28 23:14   ` Stephen Boyd
2016-12-01  8:42     ` Vivek Gautam
2016-12-02 18:47       ` Stephen Boyd
2016-12-06  8:11         ` Vivek Gautam
2016-11-22 12:02 ` [PATCH v2 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
2016-11-28 22:55   ` Stephen Boyd
2016-11-29  5:25     ` Vivek Gautam
2016-11-30 19:12       ` Stephen Boyd
2016-12-12 16:40     ` Vivek Gautam
2016-11-28 23:19   ` Stephen Boyd
2016-12-13  9:18     ` Vivek Gautam
2016-11-22 12:02 ` [PATCH v2 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam
2016-11-29  0:35   ` Stephen Boyd
2016-12-20  5:42     ` Vivek Gautam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).