[PATCH v3 0/6] phy: qcom: Updates for USB PHYs on SDM845

[PATCH v3 0/6] phy: qcom: Updates for USB PHYs on SDM845

[Manu Gautam]

SDM845 has two USB instances each with QUSB2 and QMP PHYs.
One of the QMP PHY is USB-DP (DisplayPort) combo PHY where
as other one is single lane UNI-PHY (without DP support).
Changes are related to PHY configuration for electrical
parameters tuning to improve eye-diagram and some fixes.

Changes since v2:
 - Use separate phy_ops for USB to not register power_on op.
 - And other minor changes as per review comments from Stephen.

Changes since v1:
 - Updated qusb2 compatibility name as per comment from Vivek.

Manu Gautam (6):
  phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS
  phy: qcom-qusb2: Fix crash if nvmem cell not specified
  dt-bindings: phy-qcom-qmp: Update bindings for sdm845
  phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support for sdm845
  dt-bindings: phy-qcom-usb2: Update bindings for sdm845
  phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       |   4 +-
 .../devicetree/bindings/phy/qcom-qusb2-phy.txt     |   4 +-
 drivers/phy/qualcomm/phy-qcom-qmp.c                | 181 ++++++++++++++++++++-
 drivers/phy/qualcomm/phy-qcom-qmp.h                |   5 +
 drivers/phy/qualcomm/phy-qcom-qusb2.c              |  43 +++++
 5 files changed, 234 insertions(+), 3 deletions(-)

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

[PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
to take place. This clock is output from PHY to GCC clock_ctl and then
fed back to QMP PHY and is available from PHY only after PHY is reset
and initialized, hence it can't be enabled too early in initialization
sequence.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 6470c5d..5d8df6a 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
 	mask = cfg->mask_pcs_ready;
 
+	/*
+	 * USB3 PHY requires pipe_clk for PLL lock and calibration.
+	 * Enable it from here for USB. For UFS/PCIE, it gets enabled
+	 * from poweron.
+	 */
+	if (cfg->type == PHY_TYPE_USB3) {
+		ret = clk_prepare_enable(qphy->pipe_clk);
+		if (ret) {
+			dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret);
+			goto err_clk_enable;
+		}
+	}
+
 	ret = readl_poll_timeout(status, val, !(val & mask), 1,
 				 PHY_INIT_COMPLETE_TIMEOUT);
 	if (ret) {
@@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	return ret;
 
 err_pcs_ready:
+	if (cfg->type == PHY_TYPE_USB3)
+		clk_disable_unprepare(qphy->pipe_clk);
+err_clk_enable:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
 err_lane_rst:
@@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
 	.owner		= THIS_MODULE,
 };
 
+/* USB PHY doesn't require power_on op */
+static const struct phy_ops qcom_qmp_usb_phy_gen_ops = {
+	.init		= qcom_qmp_phy_init,
+	.exit		= qcom_qmp_phy_exit,
+	.set_mode	= qcom_qmp_phy_set_mode,
+	.owner		= THIS_MODULE,
+};
+
 static
 int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 {
 	struct qcom_qmp *qmp = dev_get_drvdata(dev);
+	const struct phy_ops *ops;
 	struct phy *generic_phy;
 	struct qmp_phy *qphy;
 	char prop_name[MAX_PROP_NAME];
@@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 		}
 	}
 
-	generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops);
+	/* USB PHY doesn't use power_on op */
+	if (qmp->cfg->type == PHY_TYPE_USB3)
+		ops = &qcom_qmp_usb_phy_gen_ops;
+	else
+		ops = &qcom_qmp_phy_gen_ops;
+
+	generic_phy = devm_phy_create(dev, np, ops);
 	if (IS_ERR(generic_phy)) {
 		ret = PTR_ERR(generic_phy);
 		dev_err(dev, "failed to create qphy %d\n", ret);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/6] phy: qcom-qusb2: Fix crash if nvmem cell not specified

[Manu Gautam]

Driver currently crashes due to NULL pointer deference
while updating PHY tune register if nvmem cell is NULL.
Since, fused value for Tune1/2 register is optional,
we'd rather bail out.

Fixes: ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips")
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: stable <stable@vger.kernel.org> # 4.14+
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 94afeac..40fdef8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -315,6 +315,10 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
 	const struct qusb2_phy_cfg *cfg = qphy->cfg;
 	u8 *val;
 
+	/* efuse register is optional */
+	if (!qphy->cell)
+		return;
+
 	/*
 	 * Read efuse register having TUNE2/1 parameter's high nibble.
 	 * If efuse register shows value as 0x0, or if we fail to find
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/6] dt-bindings: phy-qcom-qmp: Update bindings for sdm845

[Manu Gautam]

Update compatible strings for USB3 PHYs on SDM845.
One is QMPv3 DisplayPort-USB combo PHY and other one
is USB UNI PHY which is single lane USB3 PHY without
DP capability.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index dcf1b8f..cef8765 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -9,7 +9,9 @@ Required properties:
 	       "qcom,ipq8074-qmp-pcie-phy" for PCIe phy on IPQ8074
 	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
 	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
-	       "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy.
+	       "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy,
+	       "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
+	       "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
 
  - reg: offset and length of register set for PHY's common serdes block.
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 4/6] phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support for sdm845

[Manu Gautam]

QMP V3 UNI PHY is a single lane USB3 PHY without support
for DisplayPort (DP).
Main difference from DP combo QMPv3 PHY is that UNI PHY
doesn't have dual RX/TX lanes and no separate DP_COM
block for configuration related to type-c or DP.
While at it, fix has_pwrdn_delay attribute for USB-DP
PHY configuration.

Reviewed-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 148 ++++++++++++++++++++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-qmp.h |   5 ++
 2 files changed, 153 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 5d8df6a..d88331b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -490,6 +490,118 @@ enum qphy_reg_layout {
 	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_RUN_TIME, 0x13),
 };
 
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_serdes_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_IVCO, 0x07),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_EN_SEL, 0x14),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x04),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_SELECT, 0x30),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_RESETSM_CNTRL2, 0x08),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CMN_CONFIG, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SVS_MODE_CLK_SEL, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_HSCLK_SEL, 0x80),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DEC_START_MODE0, 0x82),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START1_MODE0, 0xab),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START2_MODE0, 0xea),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START3_MODE0, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CP_CTRL_MODE0, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_RCTRL_MODE0, 0x16),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_CCTRL_MODE0, 0x36),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN1_MODE0, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN0_MODE0, 0x3f),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE2_MODE0, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE1_MODE0, 0xc9),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CORECLK_DIV_MODE0, 0x0a),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP3_MODE0, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP2_MODE0, 0x34),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP1_MODE0, 0x15),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP_EN, 0x04),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CORE_CLK_EN, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP_CFG, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_BUF_ENABLE, 0x0a),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_EN_CENTER, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_PER1, 0x31),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_PER2, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_ADJ_PER1, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_ADJ_PER2, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_STEP_SIZE1, 0x85),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_STEP_SIZE2, 0x07),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_tx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_HIGHZ_DRVR_EN, 0x10),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RCV_DETECT_LVL_2, 0x12),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_LANE_MODE_1, 0xc6),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_RX, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x06),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_rx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_VGA_CAL_CNTRL2, 0x0c),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_MODE_00, 0x50),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL2, 0x0e),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL3, 0x4e),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL4, 0x18),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x77),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_OFFSET_ADAPTOR_CNTRL2, 0x80),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_CNTRL, 0x03),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL, 0x1c),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x75),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_pcs_tbl[] = {
+	/* FLL settings */
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL2, 0x83),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_L, 0x09),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_H_TOL, 0xa2),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_MAN_CODE, 0x40),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL1, 0x02),
+
+	/* Lock Det settings */
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG1, 0xd1),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG2, 0x1f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG3, 0x47),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_POWER_STATE_CONFIG2, 0x1b),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SIGDET_LVL, 0xba),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V0, 0x9f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V1, 0x9f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V2, 0xb5),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V3, 0x4c),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V4, 0x64),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_LS, 0x6a),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V0, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V0, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V1, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V1, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V2, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V2, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V3, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V3, 0x1d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V4, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V4, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_LS, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_LS, 0x0d),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RATE_SLEW_CNTRL, 0x02),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_PWRUP_RESET_DLY_TIME_AUXCLK, 0x04),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TSYNC_RSYNC_TIME, 0x44),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_PWRUP_RESET_DLY_TIME_AUXCLK, 0x04),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_L, 0xe7),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_H, 0x03),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_L, 0x40),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_H, 0x00),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_WAIT_TIME, 0x75),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LFPS_TX_ECSTART_EQTLOCK, 0x86),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_RUN_TIME, 0x13),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_REFGEN_REQ_CONFIG1, 0x21),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_REFGEN_REQ_CONFIG2, 0x60),
+};
+
+
 /* struct qmp_phy_cfg - per-PHY initialization config */
 struct qmp_phy_cfg {
 	/* phy-type - PCIE/UFS/USB */
@@ -766,6 +878,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
 	.pwrdn_ctrl		= SW_PWRDN,
 	.mask_pcs_ready		= PHYSTATUS,
 
+	.has_pwrdn_delay	= true,
 	.pwrdn_delay_min	= POWER_DOWN_DELAY_US_MIN,
 	.pwrdn_delay_max	= POWER_DOWN_DELAY_US_MAX,
 
@@ -774,6 +887,35 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
 	.rx_b_lane_offset	= 0x400,
 };
 
+static const struct qmp_phy_cfg qmp_v3_usb3_uniphy_cfg = {
+	.type			= PHY_TYPE_USB3,
+	.nlanes			= 1,
+
+	.serdes_tbl		= qmp_v3_usb3_uniphy_serdes_tbl,
+	.serdes_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_serdes_tbl),
+	.tx_tbl			= qmp_v3_usb3_uniphy_tx_tbl,
+	.tx_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_tx_tbl),
+	.rx_tbl			= qmp_v3_usb3_uniphy_rx_tbl,
+	.rx_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_rx_tbl),
+	.pcs_tbl		= qmp_v3_usb3_uniphy_pcs_tbl,
+	.pcs_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_pcs_tbl),
+	.clk_list		= qmp_v3_phy_clk_l,
+	.num_clks		= ARRAY_SIZE(qmp_v3_phy_clk_l),
+	.reset_list		= msm8996_usb3phy_reset_l,
+	.num_resets		= ARRAY_SIZE(msm8996_usb3phy_reset_l),
+	.vreg_list		= msm8996_phy_vreg_l,
+	.num_vregs		= ARRAY_SIZE(msm8996_phy_vreg_l),
+	.regs			= qmp_v3_usb3phy_regs_layout,
+
+	.start_ctrl		= SERDES_START | PCS_START,
+	.pwrdn_ctrl		= SW_PWRDN,
+	.mask_pcs_ready		= PHYSTATUS,
+
+	.has_pwrdn_delay	= true,
+	.pwrdn_delay_min	= POWER_DOWN_DELAY_US_MIN,
+	.pwrdn_delay_max	= POWER_DOWN_DELAY_US_MAX,
+};
+
 static void qcom_qmp_phy_configure(void __iomem *base,
 				   const unsigned int *regs,
 				   const struct qmp_phy_init_tbl tbl[],
@@ -1414,6 +1556,12 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 	}, {
 		.compatible = "qcom,qmp-v3-usb3-phy",
 		.data = &qmp_v3_usb3phy_cfg,
+	}, {
+		.compatible = "qcom,sdm845-qmp-usb3-phy",
+		.data = &qmp_v3_usb3phy_cfg,
+	}, {
+		.compatible = "qcom,sdm845-qmp-usb3-uni-phy",
+		.data = &qmp_v3_usb3_uniphy_cfg,
 	},
 	{ },
 };
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.h b/drivers/phy/qualcomm/phy-qcom-qmp.h
index d1c6905..5d78d43 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.h
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.h
@@ -214,6 +214,8 @@
 #define QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN		0x030
 #define QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE	0x034
 #define QSERDES_V3_RX_RX_TERM_BW			0x07c
+#define QSERDES_V3_RX_VGA_CAL_CNTRL1			0x0bc
+#define QSERDES_V3_RX_VGA_CAL_CNTRL2			0x0c0
 #define QSERDES_V3_RX_RX_EQ_GAIN2_LSB			0x0c8
 #define QSERDES_V3_RX_RX_EQ_GAIN2_MSB			0x0cc
 #define QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL2		0x0d4
@@ -227,6 +229,7 @@
 #define QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL		0x10c
 #define QSERDES_V3_RX_RX_BAND				0x110
 #define QSERDES_V3_RX_RX_INTERFACE_MODE			0x11c
+#define QSERDES_V3_RX_RX_MODE_00			0x164
 
 /* Only for QMP V3 PHY - PCS registers */
 #define QPHY_V3_PCS_POWER_DOWN_CONTROL			0x004
@@ -273,6 +276,8 @@
 #define QPHY_V3_PCS_FLL_CNT_VAL_H_TOL			0x0d0
 #define QPHY_V3_PCS_FLL_MAN_CODE			0x0d4
 #define QPHY_V3_PCS_RX_SIGDET_LVL			0x1d8
+#define QPHY_V3_PCS_REFGEN_REQ_CONFIG1			0x20c
+#define QPHY_V3_PCS_REFGEN_REQ_CONFIG2			0x210
 
 /* Only for QMP V3 PHY - PCS_MISC registers */
 #define QPHY_V3_PCS_MISC_CLAMP_ENABLE			0x0c
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 5/6] dt-bindings: phy-qcom-usb2: Update bindings for sdm845

[Manu Gautam]

Update compatible strings for USB2 PHYs on sdm845.
There are two QUSB2 PHYs present on sdm845. Few PHY registers
programming is different for these PHYs related to electrical
parameters, otherwise both are same.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
index 42c9742..b99a57f 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
@@ -6,7 +6,9 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
 Required properties:
  - compatible: compatible list, contains
 	       "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
-	       "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
+	       "qcom,qusb2-v2-phy" for QUSB2 V2 PHY,
+	       "qcom,sdm845-qusb2-phy-1" for primary PHY on sdm845,
+	       "qcom,sdm845-qusb2-phy-2" for secondary PHY on sdm845.
 
  - reg: offset and length of the PHY register set.
  - #phy-cells: must be 0.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 6/6] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Manu Gautam]

There are two QUSB2 PHYs present on sdm845. Update PHY
registers programming for both the PHYs related to
electrical parameters to improve eye diagram.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 39 +++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 40fdef8..020cbb2 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -174,6 +174,27 @@ enum qusb2phy_reg_layout {
 	QUSB2_PHY_INIT_CFG(QUSB2PHY_CHG_CTRL2, 0x0),
 };
 
+static const struct qusb2_phy_init_tbl sdm845_init_tbl_1[] = {
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_ANALOG_CONTROLS_TWO, 0x03),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CLOCK_INVERTERS, 0x7c),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CMODE, 0x80),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_LOCK_DELAY, 0x0a),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_DIGITAL_TIMERS_TWO, 0x19),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_BIAS_CONTROL_1, 0x40),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_BIAS_CONTROL_2, 0x20),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_PWR_CTRL2, 0x21),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_IMP_CTRL1, 0x8),
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_IMP_CTRL2, 0x58),
+
+	QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE1, 0x45),
+	QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE2, 0x29),
+	QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE3, 0xca),
+	QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE4, 0x04),
+	QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE5, 0x03),
+
+	QUSB2_PHY_INIT_CFG(QUSB2PHY_CHG_CTRL2, 0x0),
+};
+
 struct qusb2_phy_cfg {
 	const struct qusb2_phy_init_tbl *tbl;
 	/* number of entries in the table */
@@ -220,6 +241,18 @@ struct qusb2_phy_cfg {
 	.autoresume_en	  = BIT(0),
 };
 
+static const struct qusb2_phy_cfg sdm845_phy_cfg_1 = {
+	.tbl		= sdm845_init_tbl_1,
+	.tbl_num	= ARRAY_SIZE(sdm845_init_tbl_1),
+	.regs		= qusb2_v2_regs_layout,
+
+	.disable_ctrl	= (PWR_CTRL1_VREF_SUPPLY_TRIM | PWR_CTRL1_CLAMP_N_EN |
+			   POWER_DOWN),
+	.mask_core_ready = CORE_READY_STATUS,
+	.has_pll_override = true,
+	.autoresume_en	  = BIT(0),
+};
+
 static const char * const qusb2_phy_vreg_names[] = {
 	"vdda-pll", "vdda-phy-dpdm",
 };
@@ -649,6 +682,12 @@ static int qusb2_phy_exit(struct phy *phy)
 	}, {
 		.compatible	= "qcom,qusb2-v2-phy",
 		.data		= &qusb2_v2_phy_cfg,
+	}, {
+		.compatible	= "qcom,sdm845-qusb2-phy-1",
+		.data		= &sdm845_phy_cfg_1,
+	}, {
+		.compatible	= "qcom,sdm845-qusb2-phy-2",
+		.data		= &qusb2_v2_phy_cfg,
 	},
 	{ },
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Doug Anderson]

Manu

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
> to take place. This clock is output from PHY to GCC clock_ctl and then
> fed back to QMP PHY and is available from PHY only after PHY is reset
> and initialized, hence it can't be enabled too early in initialization
> sequence.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)

So it's now new with this patch, but it's more obvious with this
patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
controls its clock.  Specifically:

* If you init the PHY but don't power it on, then you "exit" the PHY:
you'll disable/unprepare "pipe_clk" even though you never
prepare/enabled it.

* If you init the PHY, power it on, power it off, power it on, and
exit the PHY: you'll leave the clock prepared one extra time.

Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
symmetric with the enable/prepare and should be in "power off", not in
exit.

...or did I miss something?


Interestingly, your patch fixes this problem for USB3 (where init/exit
are now symmetric), but leaves the problem there for UFS/PCIE.


-Doug

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

Hi Doug,


On 3/27/2018 9:56 AM, Doug Anderson wrote:
> Manu
>
> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>> to take place. This clock is output from PHY to GCC clock_ctl and then
>> fed back to QMP PHY and is available from PHY only after PHY is reset
>> and initialized, hence it can't be enabled too early in initialization
>> sequence.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>> ---
>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
> So it's now new with this patch, but it's more obvious with this
> patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
> controls its clock.  Specifically:
>
> * If you init the PHY but don't power it on, then you "exit" the PHY:
> you'll disable/unprepare "pipe_clk" even though you never
> prepare/enabled it.
>
> * If you init the PHY, power it on, power it off, power it on, and
> exit the PHY: you'll leave the clock prepared one extra time.
>
> Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
> symmetric with the enable/prepare and should be in "power off", not in
> exit.
>
> ...or did I miss something?
>
>
> Interestingly, your patch fixes this problem for USB3 (where init/exit
> are now symmetric), but leaves the problem there for UFS/PCIE.
>

Thanks for review.
One of the reason why pipe_clk is disabled as part of phy_exit is that
halt_check from clk_disable reports error if called after PHY has been
powered down or phy_exit.
I believe that warning should be ignored in qcom gcc-clock driver
(for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
for pipe_clk and performing clk_disable from power_off for UFS/PCIE.

I can implement that as separate patch once dependent gcc driver
patch(es) gets in. Would that be ok?

-Manu

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

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Vivek Gautam]

Hi Manu,


On 3/23/2018 11:41 AM, Manu Gautam wrote:
> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
> to take place.

AFAIK, that's not true. The pipe clock is the *output* of the PLL, and 
it's should not
be needed for PLL calibration and locking. The PLL locking happens when 
the phy is
configured and powered on.
Atleast that's what the PIPE spec also says.

            CLK
              |
              |
              Y
    ----------------
   |      PLL        |--------> PCLK (this is pipe clock 125/250/... MHz 
corresponding to the data width)
    ----------------

That's the reason we were enabling it after the PLL was locked.

> This clock is output from PHY to GCC clock_ctl and then
> fed back to QMP PHY and is available from PHY only after PHY is reset
> and initialized, hence it can't be enabled too early in initialization
> sequence.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 6470c5d..5d8df6a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
>   	mask = cfg->mask_pcs_ready;
>   
> +	/*
> +	 * USB3 PHY requires pipe_clk for PLL lock and calibration.
> +	 * Enable it from here for USB. For UFS/PCIE, it gets enabled
> +	 * from poweron.
> +	 */
> +	if (cfg->type == PHY_TYPE_USB3) {
> +		ret = clk_prepare_enable(qphy->pipe_clk);

As mentioned before AFAIU, pipe clock is just an output coming out of 
the PHY's PLL
and we shouldn't try to enable pipe clock before PHY even gets initialized.
We should may just try to first fix the unbalanced pipe clock 
enable/disable problem.

Moreover, lets take care of all USB, PCIe and DP phys when we want to 
enable/disbale
pipe clock as all of them use this clock.

Best regards
Vivek

> +		if (ret) {
> +			dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret);
> +			goto err_clk_enable;
> +		}
> +	}
> +
>   	ret = readl_poll_timeout(status, val, !(val & mask), 1,
>   				 PHY_INIT_COMPLETE_TIMEOUT);
>   	if (ret) {
> @@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
>   	return ret;
>   
>   err_pcs_ready:
> +	if (cfg->type == PHY_TYPE_USB3)
> +		clk_disable_unprepare(qphy->pipe_clk);
> +err_clk_enable:
>   	if (cfg->has_lane_rst)
>   		reset_control_assert(qphy->lane_rst);
>   err_lane_rst:
> @@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>   	.owner		= THIS_MODULE,
>   };
>   
> +/* USB PHY doesn't require power_on op */
> +static const struct phy_ops qcom_qmp_usb_phy_gen_ops = {
> +	.init		= qcom_qmp_phy_init,
> +	.exit		= qcom_qmp_phy_exit,
> +	.set_mode	= qcom_qmp_phy_set_mode,
> +	.owner		= THIS_MODULE,
> +};
> +
>   static
>   int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
>   {
>   	struct qcom_qmp *qmp = dev_get_drvdata(dev);
> +	const struct phy_ops *ops;
>   	struct phy *generic_phy;
>   	struct qmp_phy *qphy;
>   	char prop_name[MAX_PROP_NAME];
> @@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
>   		}
>   	}
>   
> -	generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops);
> +	/* USB PHY doesn't use power_on op */
> +	if (qmp->cfg->type == PHY_TYPE_USB3)
> +		ops = &qcom_qmp_usb_phy_gen_ops;
> +	else
> +		ops = &qcom_qmp_phy_gen_ops;
> +
> +	generic_phy = devm_phy_create(dev, np, ops);
>   	if (IS_ERR(generic_phy)) {
>   		ret = PTR_ERR(generic_phy);
>   		dev_err(dev, "failed to create qphy %d\n", ret);

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Vivek Gautam]

On 3/27/2018 10:37 AM, Manu Gautam wrote:
> Hi Doug,
>
>
> On 3/27/2018 9:56 AM, Doug Anderson wrote:
>> Manu
>>
>> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>>> to take place. This clock is output from PHY to GCC clock_ctl and then
>>> fed back to QMP PHY and is available from PHY only after PHY is reset
>>> and initialized, hence it can't be enabled too early in initialization
>>> sequence.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>> ---
>>>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>>   1 file changed, 32 insertions(+), 1 deletion(-)
>> So it's now new with this patch, but it's more obvious with this
>> patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
>> controls its clock.  Specifically:
>>
>> * If you init the PHY but don't power it on, then you "exit" the PHY:
>> you'll disable/unprepare "pipe_clk" even though you never
>> prepare/enabled it.
>>
>> * If you init the PHY, power it on, power it off, power it on, and
>> exit the PHY: you'll leave the clock prepared one extra time.
>>
>> Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
>> symmetric with the enable/prepare and should be in "power off", not in
>> exit.
>>
>> ...or did I miss something?
>>
>>
>> Interestingly, your patch fixes this problem for USB3 (where init/exit
>> are now symmetric), but leaves the problem there for UFS/PCIE.
>>
> Thanks for review.
> One of the reason why pipe_clk is disabled as part of phy_exit is that
> halt_check from clk_disable reports error if called after PHY has been
> powered down or phy_exit.
> I believe that warning should be ignored in qcom gcc-clock driver
> (for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
> for pipe_clk and performing clk_disable from power_off for UFS/PCIE.
UFS doesn't use PIPE clock.
But considering for PCIe, if we disable pipe clock when phy is still 
running, then
it shouldn't be a problem. We should also not see the halt warning as 
the gcc
driver should be able to just turn the gate off.
The reason why it will throw that error is when the parent clock to that 
gate
is gated, i.e. the pipe clock is not flowing on that branch.

Best regards
Vivek

>
> I can implement that as separate patch once dependent gcc driver
> patch(es) gets in. Would that be ok?
>
> -Manu
>

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

Hi Vivek,


On 3/27/2018 12:21 PM, Vivek Gautam wrote:
> Hi Manu,
>
>
> On 3/23/2018 11:41 AM, Manu Gautam wrote:
>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>> to take place.
>
> AFAIK, that's not true. The pipe clock is the *output* of the PLL, and it's should not
> be needed for PLL calibration and locking. The PLL locking happens when the phy is
> configured and powered on.
> Atleast that's what the PIPE spec also says.
>
>            CLK
>              |
>              |
>              Y
>    ----------------
>   |      PLL        |--------> PCLK (this is pipe clock 125/250/... MHz corresponding to the data width)
>    ----------------
>
> That's the reason we were enabling it after the PLL was locked.

I got this clarified by QMP PHY hardware designer that pipe_clk is indeed
needed for PHY to complete init sequence and reflect in PHY_STATUS (mainly
for calibration and I should remove PLL lock from commit subject).
It might work on some platforms without this change if boot code
leaves pipe clock enabled during bootup.


>
>> This clock is output from PHY to GCC clock_ctl and then
>> fed back to QMP PHY and is available from PHY only after PHY is reset
>> and initialized, hence it can't be enabled too early in initialization
>> sequence.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> index 6470c5d..5d8df6a 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> @@ -1008,6 +1008,19 @@ static int qcom_qmp_phy_init(struct phy *phy)
>>       status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
>>       mask = cfg->mask_pcs_ready;
>>   +    /*
>> +     * USB3 PHY requires pipe_clk for PLL lock and calibration.
>> +     * Enable it from here for USB. For UFS/PCIE, it gets enabled
>> +     * from poweron.
>> +     */
>> +    if (cfg->type == PHY_TYPE_USB3) {
>> +        ret = clk_prepare_enable(qphy->pipe_clk);
>
> As mentioned before AFAIU, pipe clock is just an output coming out of the PHY's PLL
> and we shouldn't try to enable pipe clock before PHY even gets initialized.
> We should may just try to first fix the unbalanced pipe clock enable/disable problem.
>
> Moreover, lets take care of all USB, PCIe and DP phys when we want to enable/disbale
> pipe clock as all of them use this clock.

I will get the pipe_clk requirement for PCIE as well and do it for both
(depending on gcc change).

>
> Best regards
> Vivek
>
>> +        if (ret) {
>> +            dev_err(qmp->dev, "pipe_clk enable err=%d\n", ret);
>> +            goto err_clk_enable;
>> +        }
>> +    }
>> +
>>       ret = readl_poll_timeout(status, val, !(val & mask), 1,
>>                    PHY_INIT_COMPLETE_TIMEOUT);
>>       if (ret) {
>> @@ -1019,6 +1032,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
>>       return ret;
>>     err_pcs_ready:
>> +    if (cfg->type == PHY_TYPE_USB3)
>> +        clk_disable_unprepare(qphy->pipe_clk);
>> +err_clk_enable:
>>       if (cfg->has_lane_rst)
>>           reset_control_assert(qphy->lane_rst);
>>   err_lane_rst:
>> @@ -1288,10 +1304,19 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>>       .owner        = THIS_MODULE,
>>   };
>>   +/* USB PHY doesn't require power_on op */
>> +static const struct phy_ops qcom_qmp_usb_phy_gen_ops = {
>> +    .init        = qcom_qmp_phy_init,
>> +    .exit        = qcom_qmp_phy_exit,
>> +    .set_mode    = qcom_qmp_phy_set_mode,
>> +    .owner        = THIS_MODULE,
>> +};
>> +
>>   static
>>   int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
>>   {
>>       struct qcom_qmp *qmp = dev_get_drvdata(dev);
>> +    const struct phy_ops *ops;
>>       struct phy *generic_phy;
>>       struct qmp_phy *qphy;
>>       char prop_name[MAX_PROP_NAME];
>> @@ -1354,7 +1379,13 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
>>           }
>>       }
>>   -    generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops);
>> +    /* USB PHY doesn't use power_on op */
>> +    if (qmp->cfg->type == PHY_TYPE_USB3)
>> +        ops = &qcom_qmp_usb_phy_gen_ops;
>> +    else
>> +        ops = &qcom_qmp_phy_gen_ops;
>> +
>> +    generic_phy = devm_phy_create(dev, np, ops);
>>       if (IS_ERR(generic_phy)) {
>>           ret = PTR_ERR(generic_phy);
>>           dev_err(dev, "failed to create qphy %d\n", ret);
>

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

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

Hi,


On 3/27/2018 12:26 PM, Vivek Gautam wrote:
>
>
> On 3/27/2018 10:37 AM, Manu Gautam wrote:
>> Hi Doug,
>>
>>
>> On 3/27/2018 9:56 AM, Doug Anderson wrote:
>>> Manu
>>>
>>> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>>>> to take place. This clock is output from PHY to GCC clock_ctl and then
>>>> fed back to QMP PHY and is available from PHY only after PHY is reset
>>>> and initialized, hence it can't be enabled too early in initialization
>>>> sequence.
>>>>
>>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>>> ---
>>>>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>> So it's now new with this patch, but it's more obvious with this
>>> patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
>>> controls its clock.  Specifically:
>>>
>>> * If you init the PHY but don't power it on, then you "exit" the PHY:
>>> you'll disable/unprepare "pipe_clk" even though you never
>>> prepare/enabled it.
>>>
>>> * If you init the PHY, power it on, power it off, power it on, and
>>> exit the PHY: you'll leave the clock prepared one extra time.
>>>
>>> Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
>>> symmetric with the enable/prepare and should be in "power off", not in
>>> exit.
>>>
>>> ...or did I miss something?
>>>
>>>
>>> Interestingly, your patch fixes this problem for USB3 (where init/exit
>>> are now symmetric), but leaves the problem there for UFS/PCIE.
>>>
>> Thanks for review.
>> One of the reason why pipe_clk is disabled as part of phy_exit is that
>> halt_check from clk_disable reports error if called after PHY has been
>> powered down or phy_exit.
>> I believe that warning should be ignored in qcom gcc-clock driver
>> (for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
>> for pipe_clk and performing clk_disable from power_off for UFS/PCIE.
> UFS doesn't use PIPE clock.

Yes, UFS PHY doesn't use one. But similar to pipe_clk there are rx/tx symbol_clk
output from PHY that is used by UFS controller. I will update code comments
to not refer UFS for pipe_clk.

> But considering for PCIe, if we disable pipe clock when phy is still running, then
> it shouldn't be a problem. We should also not see the halt warning as the gcc
> driver should be able to just turn the gate off.
> The reason why it will throw that error is when the parent clock to that gate
> is gated, i.e. the pipe clock is not flowing on that branch.

I got the confirmation that pipe_clk is needed for PCIE as well for its
initialization to happen successfully. So we do need clock driver change
to fix this in PHY driver.

>
> Best regards
> Vivek
>
>>
>> I can implement that as separate patch once dependent gcc driver
>> patch(es) gets in. Would that be ok?
>>
>> -Manu
>>
>

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

Re: [PATCH v3 5/6] dt-bindings: phy-qcom-usb2: Update bindings for sdm845

[Rob Herring]

On Fri, Mar 23, 2018 at 11:41:26AM +0530, Manu Gautam wrote:
> Update compatible strings for USB2 PHYs on sdm845.
> There are two QUSB2 PHYs present on sdm845. Few PHY registers
> programming is different for these PHYs related to electrical
> parameters, otherwise both are same.
> 
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

Re: [PATCH v3 6/6] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Evan Green]

On Thu, Mar 22, 2018 at 11:12 PM Manu Gautam <mgautam@codeaurora.org> wrote:

> There are two QUSB2 PHYs present on sdm845. Update PHY
> registers programming for both the PHYs related to
> electrical parameters to improve eye diagram.

> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qusb2.c | 39
+++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)


Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v3 2/6] phy: qcom-qusb2: Fix crash if nvmem cell not specified

[Evan Green]

On Thu, Mar 22, 2018 at 11:13 PM Manu Gautam <mgautam@codeaurora.org> wrote:

> Driver currently crashes due to NULL pointer deference
> while updating PHY tune register if nvmem cell is NULL.
> Since, fused value for Tune1/2 register is optional,
> we'd rather bail out.

> Fixes: ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom
chips")
> Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: stable <stable@vger.kernel.org> # 4.14+
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qusb2.c | 4 ++++
>   1 file changed, 4 insertions(+)


Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Doug Anderson]

Hi,

On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Hi,
>
>
> On 3/27/2018 12:26 PM, Vivek Gautam wrote:
>>
>>
>> On 3/27/2018 10:37 AM, Manu Gautam wrote:
>>> Hi Doug,
>>>
>>>
>>> On 3/27/2018 9:56 AM, Doug Anderson wrote:
>>>> Manu
>>>>
>>>> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>>>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>>>>> to take place. This clock is output from PHY to GCC clock_ctl and then
>>>>> fed back to QMP PHY and is available from PHY only after PHY is reset
>>>>> and initialized, hence it can't be enabled too early in initialization
>>>>> sequence.
>>>>>
>>>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>>>> ---
>>>>>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>>>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>>> So it's now new with this patch, but it's more obvious with this
>>>> patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
>>>> controls its clock.  Specifically:
>>>>
>>>> * If you init the PHY but don't power it on, then you "exit" the PHY:
>>>> you'll disable/unprepare "pipe_clk" even though you never
>>>> prepare/enabled it.
>>>>
>>>> * If you init the PHY, power it on, power it off, power it on, and
>>>> exit the PHY: you'll leave the clock prepared one extra time.
>>>>
>>>> Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
>>>> symmetric with the enable/prepare and should be in "power off", not in
>>>> exit.
>>>>
>>>> ...or did I miss something?
>>>>
>>>>
>>>> Interestingly, your patch fixes this problem for USB3 (where init/exit
>>>> are now symmetric), but leaves the problem there for UFS/PCIE.
>>>>
>>> Thanks for review.
>>> One of the reason why pipe_clk is disabled as part of phy_exit is that
>>> halt_check from clk_disable reports error if called after PHY has been
>>> powered down or phy_exit.
>>> I believe that warning should be ignored in qcom gcc-clock driver
>>> (for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
>>> for pipe_clk and performing clk_disable from power_off for UFS/PCIE.
>> UFS doesn't use PIPE clock.

Just to confirm: we no longer need to do this "BRANCH_HALT_DELAY" now
that we've figured everything out, right?


> Yes, UFS PHY doesn't use one. But similar to pipe_clk there are rx/tx symbol_clk
> output from PHY that is used by UFS controller. I will update code comments
> to not refer UFS for pipe_clk.
>
>> But considering for PCIe, if we disable pipe clock when phy is still running, then
>> it shouldn't be a problem. We should also not see the halt warning as the gcc
>> driver should be able to just turn the gate off.
>> The reason why it will throw that error is when the parent clock to that gate
>> is gated, i.e. the pipe clock is not flowing on that branch.
>
> I got the confirmation that pipe_clk is needed for PCIE as well for its
> initialization to happen successfully. So we do need clock driver change
> to fix this in PHY driver.

So basically if I'm understanding this correctly:

* Both USB and PCIE need the clk_enable() in qcom_qmp_phy_init()

* UFS doesn't even use a pipe clock (pipe_clk is NULL and thus these
calls are no-ops).

So that means the next version of this code will simply get rid of
qcom_qmp_phy_poweron() and we can now use the same phy_ops for both
everything again?  That also makes everything symmetric and gets rid
of the possible imbalance of clock enable/disable, so I'm happy.


Actually: I'll also throw out a drastic idea here.  Maybe instead of
having a NULL power_on/power_off, we should have a NULL init/exit.
Does anything break if all the stuff that happens today in
qcom_qmp_phy_com_init() happens at power_on() time instead of init()
time?  I suggest this because:

* It sounds like init() is supposed to be for initialization that can
happen _before_ power on of the PHY.

* Any initialization that happens after the PHY has been powered on
seems expected to just be in the power_on() function after the
regulator was enabled.


Presumably moving this stuff to power_on could save you some power in
some cases (since the client of the PHY presumably turns power off to
the PHY with the idea of saving power).


-Doug

Re: [PATCH v3 3/6] dt-bindings: phy-qcom-qmp: Update bindings for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Update compatible strings for USB3 PHYs on SDM845.
> One is QMPv3 DisplayPort-USB combo PHY and other one
> is USB UNI PHY which is single lane USB3 PHY without
> DP capability.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> index dcf1b8f..cef8765 100644
> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -9,7 +9,9 @@ Required properties:
>                "qcom,ipq8074-qmp-pcie-phy" for PCIe phy on IPQ8074
>                "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
>                "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
> -              "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy.
> +              "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy,
> +              "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
> +              "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.

I'm confused.  What value does "qcom,qmp-v3-usb3-phy" have as a
separate entry from "qcom,sdm845-qmp-usb3-phy"?  Is
"qcom,qmp-v3-usb3-phy" expected to work on some non-SDM845 based
device?

Personally I think you should remove "qcom,qmp-v3-usb3-phy" from the
bindings as part of this patch (replacing it with the new string
qcom,sdm845-qmp-usb3-phy)".  Yeah, yeah bindings are forever.  ...but
that particular string was added about a month ago and (I believe) it
was intended for SDM845 anyway.  As per
<https://patchwork.kernel.org/patch/10302759/> match to the exact same
PHY data which leads extra credence to my belief.

If later on you find that some future chip can use the exact same
driver / settings as the SDM845 you can always list the
"qcom,sdm845-qmp-usb3-phy" string as a secondary compatible anyway.


-Doug

Re: [PATCH v3 4/6] phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> @@ -1414,6 +1556,12 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
>         }, {
>                 .compatible = "qcom,qmp-v3-usb3-phy",
>                 .data = &qmp_v3_usb3phy_cfg,
> +       }, {
> +               .compatible = "qcom,sdm845-qmp-usb3-phy",
> +               .data = &qmp_v3_usb3phy_cfg,
> +       }, {
> +               .compatible = "qcom,sdm845-qmp-usb3-uni-phy",
> +               .data = &qmp_v3_usb3_uniphy_cfg,

As per my comments on the bindings patch, having two compatible
strings that both map to "qmp_v3_usb3phy_cfg" smells a little wrong.


* If the agreement in the bindings patch is that we somehow need to
keep "qcom,qmp-v3-usb3-phy" around then the sdm845 device tree file
should list:

  compatible = "qcom,sdm845-qmp-usb3-phy", "qcom,qmp-v3-usb3-phy";

...and then you can get rid of the "qcom,sdm845-qmp-usb3-phy" in this
table (it will use the secondary compatible string to pick the right
entry).


* If the agreement is to get rid of "qcom,qmp-v3-usb3-phy" then it
should go away from the table.


-Doug

Re: [PATCH v3 5/6] dt-bindings: phy-qcom-usb2: Update bindings for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Update compatible strings for USB2 PHYs on sdm845.
> There are two QUSB2 PHYs present on sdm845. Few PHY registers
> programming is different for these PHYs related to electrical
> parameters, otherwise both are same.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> index 42c9742..b99a57f 100644
> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -6,7 +6,9 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>  Required properties:
>   - compatible: compatible list, contains
>                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
> -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
> +              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY,
> +              "qcom,sdm845-qusb2-phy-1" for primary PHY on sdm845,
> +              "qcom,sdm845-qusb2-phy-2" for secondary PHY on sdm845.

Similar question to the one I posed on
<https://patchwork.kernel.org/patch/10302761/> for the QMP PHY.  What
is "qcom,qusb2-v2-phy"?  Is it some ideal abstract version of the PHY?
 Do we expect that anyone would actually use that compatible string?

In this case in <https://patchwork.kernel.org/patch/10302755/> it
looks as if you're using the same settings as
"qcom,sdm845-qusb2-phy-2", so presumably "qcom,qusb2-v2-phy" should
just be deleted.


-Doug

Re: [PATCH v3 6/6] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> There are two QUSB2 PHYs present on sdm845. Update PHY
> registers programming for both the PHYs related to
> electrical parameters to improve eye diagram.

This tuning difference is truly associated with the SoC itself?  Are
you sure?  Are the two different PHYs in the SoC somehow using
different silicon processes?  ...or is one close to another IP block
that is noisy?  ...or something else to account for this difference?

It seems more likely that this tuning difference is associated with
the board.  If you're _certain_ this is really due to internal SoC
differences you'll have to come up with some darn good evidence to
convince me...


If the tuning is truly associated with the board then:

1. You should have a single device tree compatible string.  IMHO it
should contain the name of the SoC in it, so "qcom,sdm845-qusb2-phy".
It's generally OK to name something in Linux using the name of the
first thing that happened to support it in Linux (even if later
processors use the exact same component).  Leaving it as just
"qcom,qusb2-v2-phy" is OK with me too if that's what everyone wants.


2. You should figure out how to describe the needed board-to-board
tuning in device tree.

The only two differences you have right now are:

QUSB2PHY_IMP_CTRL1: 0 => 0x8
QUSB2PHY_PORT_TUNE1: 0x30 => 0x48

I'm not sure I found all the correct documentation for the PHY (the
docs I have say that "TUNE1" bit 3 is "reserved") so I can't come up
with all of these for you.  But I think I found the difference
accounting for the upper nybble of TUNE1 changing from 0x3 to 0x4.
For this, I think you'd want a device tree property like:

qcom,hstx_trim_mv

...and the values of that property would be the values from 800 to 950
in 8 steps, or [800, 821, 842, 864, 885, 907, 928, 950].

You'd want to do similar things for the other differences.

You don't need to encode every possible difference right now.  When
you come up with something that needs to be different you add a new
optional device tree property (defaulting to whatever the driver used
to do) to describe your new property.


-Doug

Re: [PATCH v3 1/6] phy: qcom-qmp: Enable pipe_clk before checking USB3 PHY_STATUS

[Manu Gautam]

Hi,


On 3/28/2018 1:44 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Mar 27, 2018 at 12:50 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> Hi,
>>
>>
>> On 3/27/2018 12:26 PM, Vivek Gautam wrote:
>>>
>>> On 3/27/2018 10:37 AM, Manu Gautam wrote:
>>>> Hi Doug,
>>>>
>>>>
>>>> On 3/27/2018 9:56 AM, Doug Anderson wrote:
>>>>> Manu
>>>>>
>>>>> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>>>>> QMP PHY for USB mode requires pipe_clk for calibration and PLL lock
>>>>>> to take place. This clock is output from PHY to GCC clock_ctl and then
>>>>>> fed back to QMP PHY and is available from PHY only after PHY is reset
>>>>>> and initialized, hence it can't be enabled too early in initialization
>>>>>> sequence.
>>>>>>
>>>>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>>>>> ---
>>>>>>   drivers/phy/qualcomm/phy-qcom-qmp.c | 33 ++++++++++++++++++++++++++++++++-
>>>>>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>>>> So it's now new with this patch, but it's more obvious with this
>>>>> patch.  It seems like "UFS/PCIE" is kinda broken w/ respect to how it
>>>>> controls its clock.  Specifically:
>>>>>
>>>>> * If you init the PHY but don't power it on, then you "exit" the PHY:
>>>>> you'll disable/unprepare "pipe_clk" even though you never
>>>>> prepare/enabled it.
>>>>>
>>>>> * If you init the PHY, power it on, power it off, power it on, and
>>>>> exit the PHY: you'll leave the clock prepared one extra time.
>>>>>
>>>>> Specifically I'd expect: for UFS/PCIE the disable/unprepare should be
>>>>> symmetric with the enable/prepare and should be in "power off", not in
>>>>> exit.
>>>>>
>>>>> ...or did I miss something?
>>>>>
>>>>>
>>>>> Interestingly, your patch fixes this problem for USB3 (where init/exit
>>>>> are now symmetric), but leaves the problem there for UFS/PCIE.
>>>>>
>>>> Thanks for review.
>>>> One of the reason why pipe_clk is disabled as part of phy_exit is that
>>>> halt_check from clk_disable reports error if called after PHY has been
>>>> powered down or phy_exit.
>>>> I believe that warning should be ignored in qcom gcc-clock driver
>>>> (for applicable platforms) by using BRANCH_HALT_DELAY as halt_check
>>>> for pipe_clk and performing clk_disable from power_off for UFS/PCIE.
>>> UFS doesn't use PIPE clock.
> Just to confirm: we no longer need to do this "BRANCH_HALT_DELAY" now
> that we've figured everything out, right?

That is still needed as PHY might take some time to generate pipe_clk
after its PLL is locked (or while initialization sequence is carried out).
Performing clk_enable will throw a warning. Hence, it is better to
have halt_check that will allow to club pipe_clk with other clocks and
enable it at the beginning of phy_init.

>
>
>> Yes, UFS PHY doesn't use one. But similar to pipe_clk there are rx/tx symbol_clk
>> output from PHY that is used by UFS controller. I will update code comments
>> to not refer UFS for pipe_clk.
>>
>>> But considering for PCIe, if we disable pipe clock when phy is still running, then
>>> it shouldn't be a problem. We should also not see the halt warning as the gcc
>>> driver should be able to just turn the gate off.
>>> The reason why it will throw that error is when the parent clock to that gate
>>> is gated, i.e. the pipe clock is not flowing on that branch.
>> I got the confirmation that pipe_clk is needed for PCIE as well for its
>> initialization to happen successfully. So we do need clock driver change
>> to fix this in PHY driver.
> So basically if I'm understanding this correctly:
>
> * Both USB and PCIE need the clk_enable() in qcom_qmp_phy_init()
>
> * UFS doesn't even use a pipe clock (pipe_clk is NULL and thus these
> calls are no-ops).
>
> So that means the next version of this code will simply get rid of
> qcom_qmp_phy_poweron() and we can now use the same phy_ops for both
> everything again?  That also makes everything symmetric and gets rid
> of the possible imbalance of clock enable/disable, so I'm happy.
Yes.
>
>
> Actually: I'll also throw out a drastic idea here.  Maybe instead of
> having a NULL power_on/power_off, we should have a NULL init/exit.
> Does anything break if all the stuff that happens today in
> qcom_qmp_phy_com_init() happens at power_on() time instead of init()
> time?  I suggest this because:
>
> * It sounds like init() is supposed to be for initialization that can
> happen _before_ power on of the PHY.
>
> * Any initialization that happens after the PHY has been powered on
> seems expected to just be in the power_on() function after the
> regulator was enabled.
>
>
> Presumably moving this stuff to power_on could save you some power in
> some cases (since the client of the PHY presumably turns power off to
> the PHY with the idea of saving power).

This could be ok for DWC3 USB core driver which uses both phy_init and
power_on together on init/suspend.
But it looks like ufs-qcom and pcie-qcom (mainly ufs) handle power_on
and phy_init differently. They also reset core while running init/power_on.
Changing power_on/init could affect those cores.
Regarding power_management, I think we can just update ufc/pcie
qcom glue drivers to either use phy_init/exit for power_management.
Or PHY runtime_PM if client doesn't want to power_down or reset phy
during suspend/resume.

>
> -Doug

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

Re: [PATCH v3 3/6] dt-bindings: phy-qcom-qmp: Update bindings for sdm845

[Manu Gautam]

Hi,


On 3/28/2018 3:07 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> Update compatible strings for USB3 PHYs on SDM845.
>> One is QMPv3 DisplayPort-USB combo PHY and other one
>> is USB UNI PHY which is single lane USB3 PHY without
>> DP capability.
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> index dcf1b8f..cef8765 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -9,7 +9,9 @@ Required properties:
>>                "qcom,ipq8074-qmp-pcie-phy" for PCIe phy on IPQ8074
>>                "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
>>                "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
>> -              "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy.
>> +              "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy,
>> +              "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>> +              "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
> I'm confused.  What value does "qcom,qmp-v3-usb3-phy" have as a
> separate entry from "qcom,sdm845-qmp-usb3-phy"?  Is
> "qcom,qmp-v3-usb3-phy" expected to work on some non-SDM845 based
> device?
>
> Personally I think you should remove "qcom,qmp-v3-usb3-phy" from the
> bindings as part of this patch (replacing it with the new string
> qcom,sdm845-qmp-usb3-phy)".  Yeah, yeah bindings are forever.  ...but
> that particular string was added about a month ago and (I believe) it
> was intended for SDM845 anyway.  As per
> <https://patchwork.kernel.org/patch/10302759/> match to the exact same
> PHY data which leads extra credence to my belief.
>
> If later on you find that some future chip can use the exact same
> driver / settings as the SDM845 you can always list the
> "qcom,sdm845-qmp-usb3-phy" string as a secondary compatible anyway.
>

I agree. Will just remove "qcom,sdm845-qmp-usb3-phy"

> -Doug

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

Re: [PATCH v3 5/6] dt-bindings: phy-qcom-usb2: Update bindings for sdm845

[Manu Gautam]

Hi,


On 3/28/2018 3:27 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> Update compatible strings for USB2 PHYs on sdm845.
>> There are two QUSB2 PHYs present on sdm845. Few PHY registers
>> programming is different for these PHYs related to electrical
>> parameters, otherwise both are same.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> index 42c9742..b99a57f 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>> @@ -6,7 +6,9 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>>  Required properties:
>>   - compatible: compatible list, contains
>>                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
>> -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
>> +              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY,
>> +              "qcom,sdm845-qusb2-phy-1" for primary PHY on sdm845,
>> +              "qcom,sdm845-qusb2-phy-2" for secondary PHY on sdm845.
> Similar question to the one I posed on
> <https://patchwork.kernel.org/patch/10302761/> for the QMP PHY.  What
> is "qcom,qusb2-v2-phy"?  Is it some ideal abstract version of the PHY?
>  Do we expect that anyone would actually use that compatible string?
>
> In this case in <https://patchwork.kernel.org/patch/10302755/> it
> looks as if you're using the same settings as
> "qcom,sdm845-qusb2-phy-2", so presumably "qcom,qusb2-v2-phy" should
> just be deleted.
>

I will remove "qcom,qusb2-v2-phy".


> -Doug

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

Re: [PATCH v3 6/6] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Manu Gautam]

Hi,


On 3/28/2018 4:22 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 22, 2018 at 11:11 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> There are two QUSB2 PHYs present on sdm845. Update PHY
>> registers programming for both the PHYs related to
>> electrical parameters to improve eye diagram.
> This tuning difference is truly associated with the SoC itself?  Are
> you sure?  Are the two different PHYs in the SoC somehow using
> different silicon processes?  ...or is one close to another IP block
> that is noisy?  ...or something else to account for this difference?
>
> It seems more likely that this tuning difference is associated with
> the board.  If you're _certain_ this is really due to internal SoC
> differences you'll have to come up with some darn good evidence to
> convince me...

This difference must be due to board only.

>
> If the tuning is truly associated with the board then:
>
> 1. You should have a single device tree compatible string.  IMHO it
> should contain the name of the SoC in it, so "qcom,sdm845-qusb2-phy".
> It's generally OK to name something in Linux using the name of the
> first thing that happened to support it in Linux (even if later
> processors use the exact same component).  Leaving it as just
> "qcom,qusb2-v2-phy" is OK with me too if that's what everyone wants.
I will remove "qcom,qusb2-v2-phy" as I don't expect any users of that.
>
>
> 2. You should figure out how to describe the needed board-to-board
> tuning in device tree.
>
> The only two differences you have right now are:
>
> QUSB2PHY_IMP_CTRL1: 0 => 0x8
> QUSB2PHY_PORT_TUNE1: 0x30 => 0x48
>
> I'm not sure I found all the correct documentation for the PHY (the
> docs I have say that "TUNE1" bit 3 is "reserved") so I can't come up
> with all of these for you.  But I think I found the difference
> accounting for the upper nybble of TUNE1 changing from 0x3 to 0x4.
> For this, I think you'd want a device tree property like:
>
> qcom,hstx_trim_mv
>
> ...and the values of that property would be the values from 800 to 950
> in 8 steps, or [800, 821, 842, 864, 885, 907, 928, 950].
>
> You'd want to do similar things for the other differences.
>
> You don't need to encode every possible difference right now.  When
> you come up with something that needs to be different you add a new
> optional device tree property (defaulting to whatever the driver used
> to do) to describe your new property.

Sure. I will come up with separate device tree properties to specify
board-to-board differences in PHY tuning.


>
> -Doug

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