Date:   Fri, 10 Mar 2017 14:12:04 +0530
From:   Vivek Gautam <vivek.gautam@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     robh+dt@...nel.org, kishon@...com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, mark.rutland@....com,
        sboyd@...eaurora.org, srinivas.kandagatla@...aro.org
Subject: Re: [PATCH v5 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom
 chips

Hi,


On 03/09/2017 04:31 PM, Bjorn Andersson wrote:
> 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.

Right, I think this is something i took from downstream and didn't
change. I should rather be using readl()/writel() and then readl()
again to ensure writes are through.
Thanks for pointing out.

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

Sure, will change it.

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

will correct it.

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

Right, it sort of minimized the code. :)
It just jumps to disable code for an 'off' call.

But, will amend it to use regulator_bulk_*() apis.

>
>> +
>> +	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().

Sure, will change this as suggested.

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

Sure.

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

Right, will do that.

>
>> +
>> +	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.

Right. Will add a read back in qusb2_clrbits() and qusb2_setbits().

>
>> +	/* 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.

Sure, will change this.

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


Regards
Vivek

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

Powered by blists - more mailing lists