Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1545294 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-20 18:20 +0100
    [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-20 18:20 +0100
      Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom  chips Stephen Boyd <sboyd@codeaurora.org> - 2016-12-29 00:10 +0100
        Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-29 08:00 +0100
          Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-29 08:10 +0100
    [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-20 18:20 +0100
      Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy Rob Herring <robh@kernel.org> - 2016-12-22 22:20 +0100
        Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-23 06:00 +0100
          Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy Stephen Boyd <sboyd@codeaurora.org> - 2016-12-28 02:20 +0100
            Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-28 06:50 +0100
    Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets Stephen Boyd <sboyd@codeaurora.org> - 2016-12-29 00:20 +0100
      Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam <vivek.gautam@codeaurora.org> - 2016-12-29 08:50 +0100
        Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-01-03 20:30 +0100
          Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-01-05 10:20 +0100
    Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-01-06 08:20 +0100
      Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-01-06 10:50 +0100
        Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-01-06 22:20 +0100
          Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for  qcom-chipsets vivek.gautam@codeaurora.org - 2017-01-07 19:50 +0100

#1545294 — [PATCH v3 0/4] phy: USB and PCIe phy drivers for Qcom chipsets

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, UFS and few other controllers.

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 [1].

Couple of other patches [2,3] for nvmem are also pulled in for testing.

Changes since v2:
 - Addressed review comments given by Rob and Stephen for bindings.
 - Addressed the review comments given by Stephen for the qusb2 and qmp
   phy drivers.
   Please see individual patches for detailed changelogs.

Changes since v1:
 - Moved device tree binding documentation to separate patches, as suggested
   by Rob.
 - Addressed review comment 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.

[1] https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/log/?h=integration-linux-qcomlt
[2] https://lkml.org/lkml/2016/11/17/21
[3] https://lkml.org/lkml/2016/12/19/18

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       |   71 ++
 .../devicetree/bindings/phy/qcom-qusb2-phy.txt     |   53 +
 drivers/phy/Kconfig                                |   18 +
 drivers/phy/Makefile                               |    2 +
 drivers/phy/phy-qcom-qmp.c                         | 1191 ++++++++++++++++++++
 drivers/phy/phy-qcom-qusb2.c                       |  540 +++++++++
 6 files changed, 1875 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

[toc] | [next] | [standalone]

#1545303 — [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

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 v2:
 - Removed selecting 'RESET_CONTROLLER' config.
 - Added error handling for clk_prepare_enable paths.
 - Removed explicitly setting ref_clk rate to 19.2 MHz. Don't need to
   do that since 'xo' is modeled as parent to this clock.
 - Removed 'ref_clk_src' handling. Driver doesn't need to request and
   handle this clock.
 - Moved nvmem_cell_get() to probe function.
 - Simplified phy pll status handling.
 - Using of_device_get_match_data() to get match data.
 - Uniformly using lowercase for hex numbers.
 - Fixed sparse warnings.
 - Using shorter variable names in structure and in functions.
 - Handling various comment style shortcomings.

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.

 drivers/phy/Kconfig          |  10 +
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-qcom-qusb2.c | 540 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 551 insertions(+)
 create mode 100644 drivers/phy/phy-qcom-qusb2.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index e8eb7f225a88..0ed53d018b23 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -430,6 +430,16 @@ 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
+	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 65eb2f436a41..dad1682b80e3 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 000000000000..ae7feae835fd
--- /dev/null
+++ b/drivers/phy/phy-qcom-qusb2.c
@@ -0,0 +1,540 @@
+/*
+ * 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/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.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 offset;
+	unsigned int val;
+};
+#define QUSB2_PHY_INIT_CFG(o, v) \
+	{			\
+		.offset = o,	\
+		.val = v,	\
+	}
+
+static const struct qusb2_phy_init_tbl msm8996_init_tbl[] = {
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xf8),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xb3),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xc0),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9f),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
+};
+
+struct qusb2_phy_cfg {
+	const struct qusb2_phy_init_tbl *tbl;
+	/* number of entries in the table */
+	unsigned int tbl_num;
+	/* offset to PHY_CLK_SCHEME register in TCSR map */
+	unsigned int clk_scheme_offset;
+};
+
+static const struct qusb2_phy_cfg msm8996_phy_cfg = {
+	.tbl = msm8996_init_tbl,
+	.tbl_num = ARRAY_SIZE(msm8996_init_tbl),
+};
+
+/**
+ * struct qusb2_phy - structure holding qusb2 phy attributes
+ *
+ * @phy: generic phy
+ * @base: iomapped memory space for qubs2 phy
+ *
+ * @cfg_ahb_clk: AHB2PHY interface clock
+ * @ref_clk: phy reference clock
+ * @iface_clk: phy interface clock
+ *
+ * @phy_reset: 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: TCSR syscon register map
+ * @cell: nvmem cell containing phy tuning value
+ *
+ * @cfg: phy 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 *iface_clk;
+
+	struct reset_control *phy_reset;
+
+	struct regulator *vdd_phy;
+	struct regulator *vdda_pll;
+	struct regulator *vdda_phy_dpdm;
+
+	struct regmap *tcsr;
+	struct nvmem_cell *cell;
+
+	const struct qusb2_phy_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 inline void qcom_qusb2_phy_configure(void __iomem *base,
+				const struct qusb2_phy_init_tbl tbl[],
+				int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++)
+		writel_relaxed(tbl[i].val, base + tbl[i].offset);
+
+	/* flush buffered writes */
+	mb();
+}
+
+static int qusb2_phy_toggle_power(struct qusb2_phy *qphy, bool on)
+{
+	struct device *dev = &qphy->phy->dev;
+	int ret;
+
+	if (!on) {
+		ret = 0;
+		goto disable_vdda_phy_dpdm;
+	}
+
+	ret = regulator_enable(qphy->vdd_phy);
+	if (ret) {
+		dev_err(dev, "failed to enable vdd-phy, %d\n", ret);
+		goto err_vdd_phy;
+	}
+
+	ret = regulator_enable(qphy->vdda_pll);
+	if (ret) {
+		dev_err(dev, "failed to enable vdda-pll, %d\n", ret);
+		goto disable_vdd_phy;
+	}
+
+	ret = regulator_enable(qphy->vdda_phy_dpdm);
+	if (ret) {
+		dev_err(dev, "failed 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 nvmem and sets the
+ * QUSB2PHY_PORT_TUNE2 register.
+ * For error case, skip setting the value and use the default value.
+ */
+static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
+{
+	struct device *dev = &qphy->phy->dev;
+	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.
+	 */
+	val = nvmem_cell_read(qphy->cell, NULL);
+	if (IS_ERR(val) || !val[0]) {
+		dev_dbg(dev, "failed to read a valid hs-tx trim value, %ld\n",
+								PTR_ERR(val));
+		return;
+	}
+
+	/* Fused TUNE2 value is the higher nibble only */
+	qusb2_setbits(qphy->base + QUSB2PHY_PORT_TUNE2, val[0] << 0x4);
+}
+
+static int qusb2_phy_poweron(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+	int ret;
+
+	dev_vdbg(&phy->dev, "%s(): Powering-on QUSB2 phy\n", __func__);
+
+	/* turn on regulator supplies */
+	ret = qusb2_phy_toggle_power(qphy, true);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(qphy->iface_clk);
+	if (ret)
+		dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret);
+
+	return ret;
+}
+
+static int qusb2_phy_poweroff(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(qphy->iface_clk);
+
+	qusb2_phy_toggle_power(qphy, false);
+
+	return 0;
+}
+
+static int qusb2_phy_init(struct phy *phy)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+	unsigned int val;
+	unsigned int clk_scheme;
+	int ret;
+
+	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
+
+	/* enable ahb interface clock to program phy */
+	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
+	if (ret) {
+		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
+		return ret;
+	}
+
+	/* Perform phy reset */
+	ret = reset_control_assert(qphy->phy_reset);
+	if (ret) {
+		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
+		goto err;
+	}
+
+	/* 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, %d\n", ret);
+		goto err;
+	}
+
+	/* Disable the PHY */
+	qusb2_setbits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
+			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
+
+	/* save reset value to override reference clock scheme later */
+	val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST);
+
+	qcom_qusb2_phy_configure(qphy->base, qphy->cfg->tbl,
+					qphy->cfg->tbl_num);
+
+	/* Set efuse value for tuning the PHY */
+	qusb2_phy_set_tune2_param(qphy);
+
+	/* Enable the PHY */
+	qusb2_clrbits(qphy->base + QUSB2PHY_PORT_POWERDOWN, POWER_DOWN);
+
+	/* Required 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);
+		if (ret) {
+			dev_err(&phy->dev, "failed to read clk scheme reg\n");
+			goto err1;
+		}
+
+		/* is it a differential clock scheme ? */
+		if (!(clk_scheme & PHY_CLK_SCHEME_SEL)) {
+			dev_vdbg(&phy->dev, "%s(): select differential clk\n",
+								__func__);
+			qphy->has_se_clk_scheme = false;
+		} else {
+			dev_vdbg(&phy->dev, "%s(): select single-ended clk\n",
+								__func__);
+		}
+	}
+
+	if (!qphy->has_se_clk_scheme) {
+		val &= ~CLK_REF_SEL;
+		ret = clk_prepare_enable(qphy->ref_clk);
+		if (ret) {
+			dev_err(&phy->dev, "failed to enable ref clk, %d\n",
+									ret);
+			goto err1;
+		}
+	} else {
+		val |= CLK_REF_SEL;
+	}
+
+	writel_relaxed(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);
+
+	val = readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS);
+	if (!(val & PLL_LOCKED)) {
+		dev_err(&phy->dev,
+			"QUSB2PHY pll lock failed: status reg = %x\n", val);
+		ret = -EBUSY;
+		goto err2;
+	}
+
+	return 0;
+
+err2:
+	if (!qphy->has_se_clk_scheme)
+		clk_disable_unprepare(qphy->ref_clk);
+err1:
+	reset_control_assert(qphy->phy_reset);
+err:
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+	return ret;
+}
+
+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);
+
+	reset_control_assert(qphy->phy_reset);
+
+	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_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;
+	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");
+	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, %d\n", ret);
+		return ret;
+	}
+
+	qphy->ref_clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(qphy->ref_clk)) {
+		ret = PTR_ERR(qphy->ref_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get ref clk, %d\n", ret);
+		return ret;
+	}
+
+	qphy->iface_clk = devm_clk_get(dev, "iface");
+	if (IS_ERR(qphy->iface_clk)) {
+		ret = PTR_ERR(qphy->iface_clk);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		qphy->iface_clk = NULL;
+		dev_dbg(dev, "failed to get iface clk, %d\n", 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))
+		return PTR_ERR(qphy->vdd_phy);
+
+	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
+	if (IS_ERR(qphy->vdda_pll))
+		return PTR_ERR(qphy->vdda_pll);
+
+	qphy->vdda_phy_dpdm = devm_regulator_get(dev, "vdda-phy-dpdm");
+	if (IS_ERR(qphy->vdda_phy_dpdm))
+		return PTR_ERR(qphy->vdda_phy_dpdm);
+
+	/* Get the specific init parameters of QMP phy */
+	qphy->cfg = of_device_get_match_data(dev);
+
+	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;
+	}
+
+	qphy->cell = devm_nvmem_cell_get(dev, "tune2_hstx_trim");
+	if (IS_ERR(qphy->cell)) {
+		if (PTR_ERR(qphy->cell) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		qphy->cell = NULL;
+		dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
+	}
+
+	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
+	if (IS_ERR(generic_phy)) {
+		ret = PTR_ERR(generic_phy);
+		dev_err(dev, "failed to create phy, %d\n", 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, "failed to register phy, %d\n", 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

[toc] | [prev] | [next] | [standalone]

#1548114 — Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

On 12/20, Vivek Gautam wrote:
> 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>

One comment below, but otherwise

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> +static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
> +{
> +	struct device *dev = &qphy->phy->dev;
> +	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.
> +	 */
> +	val = nvmem_cell_read(qphy->cell, NULL);
> +	if (IS_ERR(val) || !val[0]) {
> +		dev_dbg(dev, "failed to read a valid hs-tx trim value, %ld\n",
> +								PTR_ERR(val));

If val is 0 PTR_ERR(0) will be junk? I guess that's ok for debug
print.

> +		return;
> +	}
> +
> +	/* Fused TUNE2 value is the higher nibble only */
> +	qusb2_setbits(qphy->base + QUSB2PHY_PORT_TUNE2, val[0] << 0x4);
> +}

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

[toc] | [prev] | [next] | [standalone]

#1548208 — Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

On Thu, Dec 29, 2016 at 4:31 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/20, Vivek Gautam wrote:
>> 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>
>
> One comment below, but otherwise
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>
>> +static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
>> +{
>> +     struct device *dev = &qphy->phy->dev;
>> +     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.
>> +      */
>> +     val = nvmem_cell_read(qphy->cell, NULL);
>> +     if (IS_ERR(val) || !val[0]) {
>> +             dev_dbg(dev, "failed to read a valid hs-tx trim value, %ld\n",
>> +                                                             PTR_ERR(val));
>
> If val is 0 PTR_ERR(0) will be junk? I guess that's ok for debug
> print.

May be -EINVAL is better for debug print. Even when val[0]
is 0, val will still be a valid pointer, and so PTR_ERR(val) will
essentially be the pointer casted to long.



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

[toc] | [prev] | [next] | [standalone]

#1548209 — Re: [PATCH v3 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

Hi Stephen,

On Thu, Dec 29, 2016 at 12:27 PM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
> On Thu, Dec 29, 2016 at 4:31 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 12/20, Vivek Gautam wrote:
>>> 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>
>>
>> One comment below, but otherwise
>>
>> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks for the review.


Best Regards
Vivek

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

[toc] | [prev] | [next] | [standalone]

#1545305 — [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy

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 v2:
 - Removed binding for "ref_clk_src" since we don't request this
   clock in the driver.
 - Addressed s/vdda-phy-dpdm/vdda-phy-dpdm-supply.
 - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
 - Addressed s/tune2_hstx_trim_efuse/tune2_hstx_trim. Don't need to add
   'efuse' suffix to nvmem cell.
 - Addressed s/qusb2phy/phy for the node name.

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     | 53 ++++++++++++++++++++++
 1 file changed, 53 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 000000000000..594f2dcd12dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
@@ -0,0 +1,53 @@
+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" for 19.2 MHz ref clk,
+			"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-supply: 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" for cell containing
+		     HS Tx trim value.
+
+ - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
+
+Example:
+	hsusb_phy: phy@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>,
+		clock-names = "cfg_ahb", "ref";
+
+		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";
+        };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1546600 — Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy

On Tue, Dec 20, 2016 at 10:33:48PM +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 v2:
>  - Removed binding for "ref_clk_src" since we don't request this
>    clock in the driver.
>  - Addressed s/vdda-phy-dpdm/vdda-phy-dpdm-supply.
>  - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
>  - Addressed s/tune2_hstx_trim_efuse/tune2_hstx_trim. Don't need to add
>    'efuse' suffix to nvmem cell.
>  - Addressed s/qusb2phy/phy for the node name.
> 
> 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     | 53 ++++++++++++++++++++++
>  1 file changed, 53 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 000000000000..594f2dcd12dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -0,0 +1,53 @@
> +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" for 19.2 MHz ref clk,
> +			"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-supply: 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.

-names is pointless when only one.

> +
> +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" for cell containing
> +		     HS Tx trim value.

ditto.

With those dropped,

Acked-by: Rob Herring <robh@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1546751 — Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy

Hi Rob,


On Fri, Dec 23, 2016 at 2:46 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Dec 20, 2016 at 10:33:48PM +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 v2:
>>  - Removed binding for "ref_clk_src" since we don't request this
>>    clock in the driver.
>>  - Addressed s/vdda-phy-dpdm/vdda-phy-dpdm-supply.
>>  - Addressed s/ref_clk/ref. Don't need to add '_clk' suffix to clock names.
>>  - Addressed s/tune2_hstx_trim_efuse/tune2_hstx_trim. Don't need to add
>>    'efuse' suffix to nvmem cell.
>>  - Addressed s/qusb2phy/phy for the node name.
>>
>> 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     | 53 ++++++++++++++++++++++
>>  1 file changed, 53 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 000000000000..594f2dcd12dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -0,0 +1,53 @@
>> +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" for 19.2 MHz ref clk,
>> +                     "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-supply: 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.
>
> -names is pointless when only one.

Sure, will drop the -names property, and get the reset control by index.

>
>> +
>> +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" for cell containing
>> +                  HS Tx trim value.
>
> ditto.

nvmem doesn't allow, at this point, to get the cells by index.
Its APIs take 'const char' cell id and get the cell.

We should add this support to get the cell by index.
Will create a patch for that, and drop the '-names' property from bindings.

>
> With those dropped,
>
> Acked-by: Rob Herring <robh@kernel.org>

Thanks for the Ack.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

[toc] | [prev] | [next] | [standalone]

#1547804 — Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy

On 12/22/2016 08:52 PM, Vivek Gautam wrote:
>
>>> +
>>> +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" for cell containing
>>> +                  HS Tx trim value.
>> ditto.
> nvmem doesn't allow, at this point, to get the cells by index.
> Its APIs take 'const char' cell id and get the cell.
>
> We should add this support to get the cell by index.
> Will create a patch for that, and drop the '-names' property from bindings.
>

If we introduce a cells based API just for this case of one phandle it
may make sense to allow the cell id to be NULL and default to whatever
cell is there without a names property. We do something similar with
clks where a NULL connection id defaults to the first phandle in the
list. Then we can avoid having a new set of DT specific APIs here. Of
course, documentation should be updated to indicate that a NULL cell_id
means use index 0 with DT.

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

[toc] | [prev] | [next] | [standalone]

#1547839 — Re: [PATCH v3 1/4] dt-bindings: phy: Add support for QUSB2 phy

On Wed, Dec 28, 2016 at 6:43 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/22/2016 08:52 PM, Vivek Gautam wrote:
>>
>>>> +
>>>> +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" for cell containing
>>>> +                  HS Tx trim value.
>>> ditto.
>> nvmem doesn't allow, at this point, to get the cells by index.
>> Its APIs take 'const char' cell id and get the cell.
>>
>> We should add this support to get the cell by index.
>> Will create a patch for that, and drop the '-names' property from bindings.
>>
>
> If we introduce a cells based API just for this case of one phandle it
> may make sense to allow the cell id to be NULL and default to whatever
> cell is there without a names property. We do something similar with
> clks where a NULL connection id defaults to the first phandle in the
> list. Then we can avoid having a new set of DT specific APIs here. Of
> course, documentation should be updated to indicate that a NULL cell_id
> means use index 0 with DT.

Right. This makes sense. I didn't notice that we do something
similar in clocks.
I will post a new change for this (which should be pretty small
in comparison to earlier patchset that introduced new dt based APIs).

>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

[toc] | [prev] | [next] | [standalone]

#1548115 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On 12/20, Vivek Gautam wrote:
> 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>
> ---
> 
> +
> +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++)
> +		/* phys[i]->index */
> +		if (i == args->args[0])
> +			return qphy->phys[i]->phy;

What's the loop for? If args->arg[0] < qphy->cfg->nlanes then we
should be able to directly index the qphy->phys array with that
number and return it.
 
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
[...]
> +
> +/*
> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
> + * by the PHY driver for its operations.
> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
> + * Below picture shows this relationship.
> + *
> + *	   +--------------+
> + *	   |  PHY block   |<<---------------------------------------+
> + *	   |              |					    |
> + *	   |   +-------+  |		      +-----+		    |
> + *   I/P---^-->|  PLL  |--^--->pipe_clksrc--->| GCC |--->pipe_clk---+
> + *   clk   |   +-------+  |		      +-----+
> + *	   +--------------+

There are mixed tabs and spaces in this diagram causing
confusion in my editor. Please make it only spaces so the picture
comes out correctly.

> + *
> + */
> +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id)
> +{
> +	char clk_name[MAX_PROP_NAME];

I'm not sure MAX_PROP_NAME is the same as some max clk name but
ok. We should be able to calculate that the maximum is length of
usb3_phy_pipe_clk_src for now though?

> +	struct clk *clk;
> +
> +	memset(&clk_name, 0, sizeof(clk_name));
> +	switch (qphy->cfg->type) {
> +	case PHY_TYPE_USB3:
> +		snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src");
> +		break;
> +	case PHY_TYPE_PCIE:
> +		snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* controllers using QMP phys use 125MHz pipe clock interface */
> +	clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);

I was hoping you would be able to calculate the actual output
rate by reading hardware. This is ok too though. Just please use
clk_hw_register_fixed_rate() instead. And you'll probably need
some sort of devm() usage here to handle probe failure, so I
would probably roll my own and allocate a fixed_rate clk
structure and set the rate/name directly and then call
devm_clk_hw_register().

> +
> +	return PTR_ERR_OR_ZERO(clk);
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1548214 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

Hi Stephen,


On Thu, Dec 29, 2016 at 4:46 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/20, Vivek Gautam wrote:
>> 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>
>> ---
>>
>> +
>> +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++)
>> +             /* phys[i]->index */
>> +             if (i == args->args[0])
>> +                     return qphy->phys[i]->phy;
>
> What's the loop for? If args->arg[0] < qphy->cfg->nlanes then we
> should be able to directly index the qphy->phys array with that
> number and return it.

Right, will do that.

>
>> +
>> +     return ERR_PTR(-ENODEV);
>> +}
>> +
> [...]
>> +
>> +/*
>> + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate
>> + * controls it. The <s>_pipe_clk coming out of the GCC is requested
>> + * by the PHY driver for its operations.
>> + * We register the <s>_pipe_clksrc here. The gcc driver takes care
>> + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk.
>> + * Below picture shows this relationship.
>> + *
>> + *      +--------------+
>> + *      |  PHY block   |<<---------------------------------------+
>> + *      |              |                                         |
>> + *      |   +-------+  |                   +-----+               |
>> + *   I/P---^-->|  PLL  |--^--->pipe_clksrc--->| GCC |--->pipe_clk---+
>> + *   clk   |   +-------+  |                +-----+
>> + *      +--------------+
>
> There are mixed tabs and spaces in this diagram causing
> confusion in my editor. Please make it only spaces so the picture
> comes out correctly.

Sure, will do that.

>
>> + *
>> + */
>> +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id)
>> +{
>> +     char clk_name[MAX_PROP_NAME];
>
> I'm not sure MAX_PROP_NAME is the same as some max clk name but
> ok. We should be able to calculate that the maximum is length of
> usb3_phy_pipe_clk_src for now though?

Yea, i thought of using the same macro, considering that it provides
32 characters :-)
Will rather use the length of usb3_phy_pipe_clk_src for now. May be
#define MAX_CLK_NAME   24

>
>> +     struct clk *clk;
>> +
>> +     memset(&clk_name, 0, sizeof(clk_name));
>> +     switch (qphy->cfg->type) {
>> +     case PHY_TYPE_USB3:
>> +             snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src");
>> +             break;
>> +     case PHY_TYPE_PCIE:
>> +             snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id);
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* controllers using QMP phys use 125MHz pipe clock interface */
>> +     clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);
>
> I was hoping you would be able to calculate the actual output
> rate by reading hardware. This is ok too though.

Yea, I too was looking to understand the phy registers needed to
calculate and re-calibrate the pipe clock rate, but couldn't find much
from the IP programming guide. So, I had to fall back to registering
a fixed-rate clock, since we are sure that the pipe clock rate is fixed at
125 MHz for the controllers using pipe interface.
Once we find out the required information, we can as well register clk_ops
for this clock.

> Just please use
> clk_hw_register_fixed_rate() instead. And you'll probably need
> some sort of devm() usage here to handle probe failure, so I
> would probably roll my own and allocate a fixed_rate clk
> structure and set the rate/name directly and then call
> devm_clk_hw_register().

Sure, will do that.

>
>> +
>> +     return PTR_ERR_OR_ZERO(clk);
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project


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

[toc] | [prev] | [next] | [standalone]

#1550117 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On Wed 28 Dec 23:39 PST 2016, Vivek Gautam wrote:

> >> + *
> >> + */
> >> +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id)
> >> +{
> >> +     char clk_name[MAX_PROP_NAME];
> >
> > I'm not sure MAX_PROP_NAME is the same as some max clk name but
> > ok. We should be able to calculate that the maximum is length of
> > usb3_phy_pipe_clk_src for now though?
> 
> Yea, i thought of using the same macro, considering that it provides
> 32 characters :-)
> Will rather use the length of usb3_phy_pipe_clk_src for now. May be
> #define MAX_CLK_NAME   24
> 

Using a macro indicates that the size have some global meaning, but as
far as I can see it doesn't. Just write 24 and you will have enough
space for 100k pcie pipe clocks. And then use sizeof(clk_name) below.

> >
> >> +     struct clk *clk;
> >> +
> >> +     memset(&clk_name, 0, sizeof(clk_name));

There's no point in clearing clk_name, as it will be overwritten
and nul-terminated below.

> >> +     switch (qphy->cfg->type) {
> >> +     case PHY_TYPE_USB3:
> >> +             snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src");
> >> +             break;
> >> +     case PHY_TYPE_PCIE:
> >> +             snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id);
> >> +             break;
> >> +     default:
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     /* controllers using QMP phys use 125MHz pipe clock interface */
> >> +     clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1551792 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

Hi,

On Wed, Jan 4, 2017 at 12:54 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 28 Dec 23:39 PST 2016, Vivek Gautam wrote:
>
>> >> + *
>> >> + */
>> >> +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id)
>> >> +{
>> >> +     char clk_name[MAX_PROP_NAME];
>> >
>> > I'm not sure MAX_PROP_NAME is the same as some max clk name but
>> > ok. We should be able to calculate that the maximum is length of
>> > usb3_phy_pipe_clk_src for now though?
>>
>> Yea, i thought of using the same macro, considering that it provides
>> 32 characters :-)
>> Will rather use the length of usb3_phy_pipe_clk_src for now. May be
>> #define MAX_CLK_NAME   24
>>
>
> Using a macro indicates that the size have some global meaning, but as
> far as I can see it doesn't. Just write 24 and you will have enough
> space for 100k pcie pipe clocks. And then use sizeof(clk_name) below.

Sure, will do as suggested.

>
>> >
>> >> +     struct clk *clk;
>> >> +
>> >> +     memset(&clk_name, 0, sizeof(clk_name));
>
> There's no point in clearing clk_name, as it will be overwritten
> and null-terminated below.

Right. Will remove this.

>
>> >> +     switch (qphy->cfg->type) {
>> >> +     case PHY_TYPE_USB3:
>> >> +             snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src");
>> >> +             break;
>> >> +     case PHY_TYPE_PCIE:
>> >> +             snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id);
>> >> +             break;
>> >> +     default:
>> >> +             return -EINVAL;
>> >> +     }
>> >> +
>> >> +     /* controllers using QMP phys use 125MHz pipe clock interface */
>> >> +     clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

[toc] | [prev] | [next] | [standalone]

#1552572 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On Tue 20 Dec 09:03 PST 2016, Vivek Gautam wrote:

> diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
[..]
> +static int qcom_qmp_phy_poweron(struct phy *phy)
[..]
> +
> +err3:

Rather than naming your labels errX, it's idiomatic to give them
descriptive names, e.g. "disable_ref_clk" here.

> +	clk_disable_unprepare(qphy->ref_clk);
> +err2:
> +	regulator_disable(qphy->vddp_ref_clk);
> +err1:
> +	regulator_disable(qphy->vdda_pll);
> +err:
> +	regulator_disable(qphy->vdda_phy);
> +	return ret;
> +}
[..]
> +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
> +{
> +	const struct qmp_phy_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;
> +	}

As far as I can see phy_init() and phy_exit() already keep reference
count on the initialization and you only call this function from
phy_ops->init, so you should be able to drop this.

[..]
> +
> +/* PHY Initialization */
> +static int qcom_qmp_phy_init(struct phy *phy)
> +{
[..]
> +	/* phy power down delay; given in PCIE phy programming guide only */
> +	if (qphy->cfg->type == PHY_TYPE_PCIE)

Rather than basing this off "is this pcie" it's preferred if you have a
bool power_down_delay; (or optionally carrying the timeout value) to
control this.

> +		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->start_ctrl);
> +
> +	/* Pull PHY out of reset state */
> +	qphy_clrbits(pcs + QPHY_SW_RESET, SW_RESET);
> +
> +	status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
> +	mask = cfg->mask_pcs_ready;
> +
> +	ret = !mask ? 0 : readl_poll_timeout(status, val, !(val & mask), 1,
> +						PHY_INIT_COMPLETE_TIMEOUT);

I presume it's not okay to read the status register even once for pcie?
If it is you can skip the conditional as !(val & 0) will break the poll
after the first read.

[..]
> +static int qcom_qmp_phy_exit(struct phy *phy)
> +{
[..]
> +}
> +
> +

Extra blank line

> +static int qcom_qmp_phy_regulator_init(struct device *dev)
[..]
> +static
> +struct qmp_phy_desc *qcom_qmp_phy_create(struct platform_device *pdev, int 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) {

Is this check adding any value?

> +			ret = PTR_ERR(phy_desc->pipe_clk);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev,
> +					"failed to get lane%d pipe_clk, %d\n",
> +					id, ret);
> +			return ERR_PTR(ret);
> +		}
> +		phy_desc->pipe_clk = NULL;

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1552653 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

Hi,


On 01/06/2017 12:48 PM, Bjorn Andersson wrote:
> On Tue 20 Dec 09:03 PST 2016, Vivek Gautam wrote:
>
>> diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
> [..]
>> +static int qcom_qmp_phy_poweron(struct phy *phy)
> [..]
>> +
>> +err3:
> Rather than naming your labels errX, it's idiomatic to give them
> descriptive names, e.g. "disable_ref_clk" here.
Sure will change these labels and any such occurrence further in the code.

>
>> +	clk_disable_unprepare(qphy->ref_clk);
>> +err2:
>> +	regulator_disable(qphy->vddp_ref_clk);
>> +err1:
>> +	regulator_disable(qphy->vdda_pll);
>> +err:
>> +	regulator_disable(qphy->vdda_phy);
>> +	return ret;
>> +}
> [..]
>> +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
>> +{
>> +	const struct qmp_phy_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;
>> +	}
> As far as I can see phy_init() and phy_exit() already keep reference
> count on the initialization and you only call this function from
> phy_ops->init, so you should be able to drop this.
This is an intermediary function that does the common block initialization.
PHYs like PCIe have a separate common block (apart from SerDes)
for all phy channels. We shouldn't program this common block
multiple times for each channel. That's why this init_count.

>
> [..]
>> +
>> +/* PHY Initialization */
>> +static int qcom_qmp_phy_init(struct phy *phy)
>> +{
> [..]
>> +	/* phy power down delay; given in PCIE phy programming guide only */
>> +	if (qphy->cfg->type == PHY_TYPE_PCIE)
> Rather than basing this off "is this pcie" it's preferred if you have a
> bool power_down_delay; (or optionally carrying the timeout value) to
> control this.
Sure, will modify this.

>
>> +		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->start_ctrl);
>> +
>> +	/* Pull PHY out of reset state */
>> +	qphy_clrbits(pcs + QPHY_SW_RESET, SW_RESET);
>> +
>> +	status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
>> +	mask = cfg->mask_pcs_ready;
>> +
>> +	ret = !mask ? 0 : readl_poll_timeout(status, val, !(val & mask), 1,
>> +						PHY_INIT_COMPLETE_TIMEOUT);
> I presume it's not okay to read the status register even once for pcie?
> If it is you can skip the conditional as !(val & 0) will break the poll
> after the first read.
Yea, it is okay to read the status register for pcie.

Will leave just readl_poll_timeout() that exits on !(val & mask).
We anyways don't set mask_pcs_ready for pcie.

> [..]
>> +static int qcom_qmp_phy_exit(struct phy *phy)
>> +{
> [..]
>> +}
>> +
>> +
> Extra blank line
Will remove it.
>
>> +static int qcom_qmp_phy_regulator_init(struct device *dev)
> [..]
>> +static
>> +struct qmp_phy_desc *qcom_qmp_phy_create(struct platform_device *pdev, int 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) {
> Is this check adding any value?
The USB3 and PCIe phys are PIPE based phys, and hence the pipe
clock is mandatory for them. Other phys don't care about pipe clock.

>
>> +			ret = PTR_ERR(phy_desc->pipe_clk);
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err(dev,
>> +					"failed to get lane%d pipe_clk, %d\n",
>> +					id, ret);
>> +			return ERR_PTR(ret);
>> +		}
>> +		phy_desc->pipe_clk = NULL;
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards
Vivek

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

[toc] | [prev] | [next] | [standalone]

#1553135 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On Fri 06 Jan 01:47 PST 2017, Vivek Gautam wrote:

> > > +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
> > > +{
> > > +	const struct qmp_phy_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;
> > > +	}
> > As far as I can see phy_init() and phy_exit() already keep reference
> > count on the initialization and you only call this function from
> > phy_ops->init, so you should be able to drop this.
> This is an intermediary function that does the common block initialization.
> PHYs like PCIe have a separate common block (apart from SerDes)
> for all phy channels. We shouldn't program this common block
> multiple times for each channel. That's why this init_count.
> 

You're right!

Unfortunately it took me several minutes to wrap my head around the phy
vs multi-lane and I have a really hard time keeping "qcom_qmp_phy" and
"qmp_phy_desc" apart throughout the driver.

If I understand correctly the qcom_qmp_phy is the context representing a
"QMP block", while this is a PHY block it's not actually the phy in
Linux eyes. The qcom_phy_desc represents a "QMP lane", which in Linux
eyes is the phys, but as we think of QMP as the PHY this confused me.

How about naming them "struct qmp" and "struct qmp_lane" (or possibly
qmp_phy) instead? That way we remove the confusion of QMP PHY vs Linux
PHY and we make the lane part explicit.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1553716 — Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

On 2017-01-07 02:47, Bjorn Andersson wrote:
> On Fri 06 Jan 01:47 PST 2017, Vivek Gautam wrote:
> 
>> > > +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
>> > > +{
>> > > +	const struct qmp_phy_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;
>> > > +	}
>> > As far as I can see phy_init() and phy_exit() already keep reference
>> > count on the initialization and you only call this function from
>> > phy_ops->init, so you should be able to drop this.
>> This is an intermediary function that does the common block 
>> initialization.
>> PHYs like PCIe have a separate common block (apart from SerDes)
>> for all phy channels. We shouldn't program this common block
>> multiple times for each channel. That's why this init_count.
>> 
> 
> You're right!
> 
> Unfortunately it took me several minutes to wrap my head around the phy
> vs multi-lane and I have a really hard time keeping "qcom_qmp_phy" and
> "qmp_phy_desc" apart throughout the driver.
> 
> If I understand correctly the qcom_qmp_phy is the context representing 
> a
> "QMP block", while this is a PHY block it's not actually the phy in
> Linux eyes. The qcom_phy_desc represents a "QMP lane", which in Linux
> eyes is the phys, but as we think of QMP as the PHY this confused me.

That's correct. The qcom_qmp_phy structure represents the QMP phy block
as a whole and not the individual phy lane instances. These phy lanes
are represented by qcom_phy_desc, and are the actual PHYs in Linux eyes.

> 
> How about naming them "struct qmp" and "struct qmp_lane" (or possibly
> qmp_phy) instead? That way we remove the confusion of QMP PHY vs Linux
> PHY and we make the lane part explicit.

Sure, this sounds good to me - "struct qmp" and "struct qmp_phy" (will
call the respective variables as qblk for qmp block (struct qmp) and
qphy for struct qmp_phy).
Thanks for pointing out. Will change them.

Regards
Vivek

> 
> Regards,
> Bjorn
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web