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

[Bjorn Andersson]

On Thu 09 Mar 10:07 CET 2017, Vivek Gautam wrote:

[..]
> +static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
> +{
> +	u32 reg;
> +
> +	reg = readl_relaxed(base + offset);
> +	reg |= val;
> +	writel_relaxed(reg, base + offset);
> +
> +	/* Ensure above write is completed */
> +	mb();

mb() does not ensure that the operation is completed, it only ensures
ordering between this write and subsequent memory accesses.

Unless you have specific reasons to you should not use the _relaxed()
versions of read and write operations. Using the non-relaxed versions
will ensure that your memory operations are not reordered.

> +}
> +
> +static inline void qusb2_clrbits(void __iomem *base, u32 offset, u32 val)
> +{
> +	u32 reg;
> +
> +	reg = readl_relaxed(base + offset);
> +	reg &= ~val;
> +	writel_relaxed(reg, base + offset);
> +
> +	/* Ensure above write is completed */
> +	mb();

Dito.

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

Dito.

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

This is an ugly hack.

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

If you use the regulator_bulk interface for your regulators you can
replace this entire function with a call to regulator_bulk_enable() and
regulator_bulk_disable().

> +
> +/*
> + * 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\n");
> +		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);

Call regulator_bulk_enable()

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

Call regulator_bulk_disable()

> +
> +	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 disable_ahb_clk;
> +	}
> +
> +	/* 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 disable_ahb_clk;
> +	}
> +
> +	/* 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);
> +

The mb() in clrbits will not ensure that the write is finished before
the sleep, it will only ensure that above write is done before any later
memory operations. If you want to ensure that the value has hit the
hardware before the sleep you should simply read back the value from the
register.

> +	/* 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 assert_phy_reset;
> +		}
> +
> +		/* 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 assert_phy_reset;
> +		}
> +	} 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();
> +

As above.

> +	/* Required to get phy pll lock successfully */
> +	usleep_range(100, 110);
> +

Regards,
Bjorn